Use ordered set for imports to iterate deterministically when filteri…

…ng an image (#3507)

CHANGELOG.md

@@ -4,6 +4,8 @@
 
 - Add `buf registry plugin {create,delete,info,update}` commands to manage BSR plugins.
 - Breaking analysis support for `buf beta lsp`.
+- Fix bug when using the `--type` flag filter for `buf build` where import ordering is not
+  determinisitic.
 
 ## [v1.47.2] - 2024-11-14
 

private/bufpkg/bufimage/bufimageutil/bufimageutil.go

@@ -266,16 +266,20 @@ func ImageFilteredByTypesWithOptions(image bufimage.Image, types []string, opts
 		// the file's WeakDependency field.
 		indexFromTo := make(map[int32]int32)
 		indexTo := 0
+		// Only handle imports and dependencies if there are any.
 		for indexFrom, importPath := range imageFileDescriptor.GetDependency() {
 			path := append(basePath, int32(indexFrom))
-			if _, ok := importsRequired[importPath]; ok {
+			// We check if the import path exists among required imports. If yes, we
+			// move and then delete from required imports as we go.
+			if importsRequired != nil && importsRequired.index(importPath) != -1 {
 				sourcePathRemapper.markMoved(path, int32(indexTo))
 				indexFromTo[int32(indexFrom)] = int32(indexTo)
 				imageFileDescriptor.Dependency[indexTo] = importPath
 				indexTo++
-				// markDeleted them as we go, so we know which ones weren't in the list
-				delete(importsRequired, importPath)
+				// delete them as we go, so we know which ones weren't in the list
+				importsRequired.delete(importPath)
 			} else {
+				// Path did not exist in required imports, we mark as deleted.
 				sourcePathRemapper.markDeleted(path)
 			}
 		}
@@ -284,9 +288,12 @@ func ImageFilteredByTypesWithOptions(image bufimage.Image, types []string, opts
 		// Add any other imports (which may not have been in the list because
 		// they were picked up via a public import). The filtered files will not
 		// use public imports.
-		for importPath := range importsRequired {
-			imageFileDescriptor.Dependency = append(imageFileDescriptor.Dependency, importPath)
+		// The imports are added in the order they are encountered when importing
+		// to maintain a deterministic ordering.
+		if importsRequired != nil {
+			imageFileDescriptor.Dependency = append(imageFileDescriptor.Dependency, importsRequired.keys()...)
 		}
+
 		imageFileDescriptor.PublicDependency = nil
 		sourcePathRemapper.markDeleted([]int32{filePublicDependencyTag})
 
@@ -458,9 +465,9 @@ type transitiveClosure struct {
 	// entire package being included). The above fields are used to filter the
 	// contents of files. But files named in this set will not be filtered.
 	completeFiles map[string]struct{}
-	// The set of imports for each file. This allows for re-writing imports
+	// The ordered set of imports for each file. This allows for re-writing imports
 	// for files whose contents have been pruned.
-	imports map[string]map[string]struct{}
+	imports map[string]*orderedImports
 }
 
 type closureInclusionMode int
@@ -485,7 +492,7 @@ func newTransitiveClosure() *transitiveClosure {
 		elements:      map[namedDescriptor]closureInclusionMode{},
 		files:         map[string]struct{}{},
 		completeFiles: map[string]struct{}{},
-		imports:       map[string]map[string]struct{}{},
+		imports:       map[string]*orderedImports{},
 	}
 }
 
@@ -495,10 +502,10 @@ func (t *transitiveClosure) addImport(fromPath, toPath string) {
 	}
 	imps := t.imports[fromPath]
 	if imps == nil {
-		imps = map[string]struct{}{}
+		imps = newOrderedImports()
 		t.imports[fromPath] = imps
 	}
-	imps[toPath] = struct{}{}
+	imps.add(toPath)
 }
 
 func (t *transitiveClosure) addFile(file string, imageIndex *imageIndex, opts *imageFilterOptions) error {
@@ -1000,3 +1007,55 @@ func stripSourceRetentionOptionsFromFile(imageFile bufimage.ImageFile) (bufimage
 		imageFile.UnusedDependencyIndexes(),
 	)
 }
+
+// orderedImports is a structure to maintain an ordered set of imports. This is needed
+// because we want to be able to iterate through imports in a deterministic way when filtering
+// the image.
+type orderedImports struct {
+	pathToIndex map[string]int
+	paths       []string
+}
+
+// newOrderedImports creates a new orderedImports structure.
+func newOrderedImports() *orderedImports {
+	return &orderedImports{
+		pathToIndex: map[string]int{},
+	}
+}
+
+// index returns the index for a given path. If the path does not exist in index map, -1
+// is returned and should be considered deleted.
+func (o *orderedImports) index(path string) int {
+	if index, ok := o.pathToIndex[path]; ok {
+		return index
+	}
+	return -1
+}
+
+// add appends a path to the paths list and the index in the map. If a key already exists,
+// then this is a no-op.
+func (o *orderedImports) add(path string) {
+	if _, ok := o.pathToIndex[path]; !ok {
+		o.pathToIndex[path] = len(o.paths)
+		o.paths = append(o.paths, path)
+	}
+}
+
+// delete removes a key from the index map of ordered imports. If a non-existent path is
+// set for deletion, then this is a no-op.
+// Note that the path is not removed from the paths list. If you want to iterate through
+// the paths, use keys() to get all non-deleted keys.
+func (o *orderedImports) delete(path string) {
+	delete(o.pathToIndex, path)
+}
+
+// keys provides all non-deleted keys from the ordered imports.
+func (o *orderedImports) keys() []string {
+	keys := make([]string, 0, len(o.pathToIndex))
+	for _, path := range o.paths {
+		if _, ok := o.pathToIndex[path]; ok {
+			keys = append(keys, path)
+		}
+	}
+	return keys
+}