Fix bufimageutil issues regarding weak imports

.golangci.yml

@@ -476,6 +476,12 @@ linters:
         # and should be considered safe.
         path: private/bufpkg/bufcobra/markdown.go
         text: "printf: non-constant format string in call to p"
+      - linters:
+          - gosec
+        # This converts slice indexes in a FileDescriptorProto to int32,
+        # which are not an actual risk of overflow.
+        path: private/bufpkg/bufimage/bufimageutil/image_filter.go
+        text: "G115:"
 
 issues:
   max-same-issues: 0

private/bufpkg/bufimage/bufimageutil/bufimageutil.go

@@ -35,11 +35,11 @@ const (
 )
 
 var (
-	// ErrImageFilterTypeNotFound is returned from ImageFilteredByTypes when
+	// ErrImageFilterTypeNotFound is returned from FilterImage when
 	// a specified type cannot be found in an image.
 	ErrImageFilterTypeNotFound = errors.New("not found")
 
-	// ErrImageFilterTypeIsImport is returned from ImageFilteredByTypes when
+	// ErrImageFilterTypeIsImport is returned from FilterImage when
 	// a specified type name is declared in a module dependency.
 	ErrImageFilterTypeIsImport = errors.New("type declared in imported module")
 )
@@ -69,11 +69,11 @@ func FreeMessageRangeStrings(
 	return s, nil
 }
 
-// ImageFilterOption is an option that can be passed to ImageFilteredByTypesWithOptions.
+// ImageFilterOption is an option that can be passed to FilterImage.
 type ImageFilterOption func(*imageFilterOptions)
 
 // WithExcludeCustomOptions returns an option that will cause an image filtered via
-// ImageFilteredByTypesWithOptions to *not* include custom options unless they are
+// FilterImage to *not* include custom options unless they are
 // explicitly named in the list of filter types.
 func WithExcludeCustomOptions() ImageFilterOption {
 	return func(opts *imageFilterOptions) {
@@ -82,15 +82,15 @@ func WithExcludeCustomOptions() ImageFilterOption {
 }
 
 // WithExcludeKnownExtensions returns an option that will cause an image filtered via
-// ImageFilteredByTypesWithOptions to *not* include the known extensions for included
+// FilterImage to *not* include the known extensions for included
 // extendable messages unless they are explicitly named in the list of filter types.
 func WithExcludeKnownExtensions() ImageFilterOption {
 	return func(opts *imageFilterOptions) {
 		opts.includeKnownExtensions = false
 	}
 }
 
-// WithAllowIncludeOfImportedType returns an option for ImageFilteredByTypesWithOptions
+// WithAllowIncludeOfImportedType returns an option for FilterImage
 // that allows a named included type to be in an imported file or module. Without this
 // option, only types defined directly in the image to be filtered are allowed.
 // Excluded types are always allowed to be in imported files or modules.
@@ -100,7 +100,7 @@ func WithAllowIncludeOfImportedType() ImageFilterOption {
 	}
 }
 
-// WithIncludeTypes returns an option for ImageFilteredByTypesWithOptions that specifies
+// WithIncludeTypes returns an option for FilterImage that specifies
 // the set of types that should be included in the filtered image.
 //
 // May be provided multiple times. The type names should be fully qualified.
@@ -120,7 +120,7 @@ func WithIncludeTypes(typeNames ...string) ImageFilterOption {
 	}
 }
 
-// WithExcludeTypes returns an option for ImageFilteredByTypesWithOptions that
+// WithExcludeTypes returns an option for FilterImage that
 // specifies the set of types that should be excluded from the filtered image.
 //
 // May be provided multiple times. The type names should be fully qualified.
@@ -140,7 +140,7 @@ func WithExcludeTypes(typeNames ...string) ImageFilterOption {
 	}
 }
 
-// WithMutateInPlace returns an option for ImageFilteredByTypesWithOptions that specifies
+// WithMutateInPlace returns an option for FilterImage that specifies
 // that the filtered image should be mutated in place. This option is useful when the
 // unfiltered image is no longer needed and the caller wants to avoid the overhead of
 // copying the image.

private/bufpkg/bufimage/bufimageutil/bufimageutil_test.go

@@ -274,17 +274,17 @@ func TestOptionImports(t *testing.T) {
 
 	t.Run("exclude_foo", func(t *testing.T) {
 		t.Parallel()
-		generated := runFilterImage(t, image, WithExcludeTypes("message_foo"))
+		generated, _ := runFilterImage(t, image, WithExcludeTypes("message_foo"))
 		checkExpectation(t, context.Background(), generated, bucket, "foo.txtar")
 	})
 	t.Run("exclude_foo_bar", func(t *testing.T) {
 		t.Parallel()
-		generated := runFilterImage(t, image, WithExcludeTypes("message_foo", "message_bar"))
+		generated, _ := runFilterImage(t, image, WithExcludeTypes("message_foo", "message_bar"))
 		checkExpectation(t, context.Background(), generated, bucket, "foo_bar.txtar")
 	})
 	t.Run("exclude_bar", func(t *testing.T) {
 		t.Parallel()
-		generated := runFilterImage(t, image, WithIncludeTypes("pkg.Foo"), WithExcludeTypes("message_bar"))
+		generated, _ := runFilterImage(t, image, WithIncludeTypes("pkg.Foo"), WithExcludeTypes("message_bar"))
 		checkExpectation(t, context.Background(), generated, bucket, "bar.txtar")
 	})
 }
@@ -541,11 +541,11 @@ func runDiffTest(t *testing.T, testdataDir string, expectedFile string, opts ...
 	ctx := context.Background()
 	bucket, image, err := getImage(ctx, slogtestext.NewLogger(t), testdataDir, bufimage.WithExcludeSourceCodeInfo())
 	require.NoError(t, err)
-	generated := runFilterImage(t, image, opts...)
+	generated, _ := runFilterImage(t, image, opts...)
 	checkExpectation(t, ctx, generated, bucket, expectedFile)
 }
 
-func runFilterImage(t *testing.T, image bufimage.Image, opts ...ImageFilterOption) []byte {
+func runFilterImage(t *testing.T, image bufimage.Image, opts ...ImageFilterOption) ([]byte, bufimage.Image) {
 	filteredImage, err := FilterImage(image, opts...)
 	require.NoError(t, err)
 	assert.NotNil(t, filteredImage)
@@ -562,8 +562,9 @@ func runFilterImage(t *testing.T, image bufimage.Image, opts ...ImageFilterOptio
 
 	// We may have filtered out custom options from the set in the step above. However, the options messages
 	// still contain extension fields that refer to the custom options, as a result of building the image.
-	// So we serialize and then de-serialize, and use only the filtered results to parse extensions. That way, the result will omit custom options that aren't present in the filtered set (as they will be
-	// considered unrecognized fields).
+	// So we serialize and then de-serialize, and use only the filtered results to parse extensions. That way,
+	// the result will omit custom options that aren't present in the filtered set (as they will be considered
+	// unrecognized fields).
 	fileDescriptorSet := &descriptorpb.FileDescriptorSet{
 		File: xslices.Map(filteredImage.Files(), func(imageFile bufimage.ImageFile) *descriptorpb.FileDescriptorProto {
 			return imageFile.FileDescriptorProto()
@@ -594,7 +595,7 @@ func runFilterImage(t *testing.T, image bufimage.Image, opts ...ImageFilterOptio
 		return archive.Files[i].Name < archive.Files[j].Name
 	})
 	generated := txtar.Format(archive)
-	return generated
+	return generated, filteredImage
 }
 
 func checkExpectation(t *testing.T, ctx context.Context, actual []byte, bucket storage.ReadWriteBucket, expectedFile string) {
@@ -618,15 +619,19 @@ func runSourceCodeInfoTest(t *testing.T, typename string, expectedFile string, o
 	bucket, image, err := getImage(ctx, slogtestext.NewLogger(t), "testdata/sourcecodeinfo")
 	require.NoError(t, err)
 
-	filteredImage, err := FilterImage(image, append(opts, WithIncludeTypes(typename))...)
+	opts = append(opts, WithIncludeTypes(typename))
+	generated, image := runFilterImage(t, image, opts...)
+	filteredImage, err := FilterImage(image, opts...)
 	require.NoError(t, err)
 
 	imageFile := filteredImage.GetFile("test.proto")
 	sourceCodeInfo := imageFile.FileDescriptorProto().GetSourceCodeInfo()
 	actual, err := protoencoding.NewJSONMarshaler(nil, protoencoding.JSONMarshalerWithIndent()).Marshal(sourceCodeInfo)
 	require.NoError(t, err)
+	generated = append(generated, []byte("-- source_code_info.json --\n")...)
+	generated = append(generated, actual...)
 
-	checkExpectation(t, ctx, actual, bucket, expectedFile)
+	checkExpectation(t, ctx, generated, bucket, expectedFile)
 
 	resolver, err := protoencoding.NewResolver(bufimage.ImageToFileDescriptorProtos(filteredImage)...)
 	require.NoError(t, err)

private/bufpkg/bufimage/bufimageutil/image_filter.go

@@ -15,6 +15,7 @@
 package bufimageutil
 
 import (
+	"errors"
 	"fmt"
 	"slices"
 	"sort"
@@ -254,6 +255,10 @@ func (b *sourcePathsBuilder) remapDependencies(
 	dependencies := fileDescriptor.GetDependency()
 	publicDependencies := fileDescriptor.GetPublicDependency()
 	weakDependencies := fileDescriptor.GetWeakDependency()
+	// TODO: Add support for option dependencies when buf CLI supports edition 2024.
+	if len(fileDescriptor.GetOptionDependency()) > 0 {
+		return nil, nil, nil, false, errors.New("edition 2024 not yet supported: cannot filter a file descriptor with option dependencies")
+	}
 
 	// Check if the imports need to be remapped.
 	importsRequired := b.closure.imports[fileDescriptor.GetName()]
@@ -271,14 +276,15 @@ func (b *sourcePathsBuilder) remapDependencies(
 		return dependencies, publicDependencies, weakDependencies, false, nil
 	}
 
-	indexFrom, indexTo := int32(0), int32(0)
 	var newDependencies []string
 	if b.options.mutateInPlace {
 		newDependencies = dependencies[:0]
 	}
 	dependencyPath := append(sourcePath, fileDependencyTag)
 	dependencyChanges := make([]int32, len(dependencies))
-	for _, importPath := range dependencies {
+	indexTo := int32(0)
+	for i, importPath := range dependencies {
+		indexFrom := int32(i)
 		path := append(dependencyPath, indexFrom)
 		if _, ok := importsRequired[importPath]; ok {
 			dependencyChanges[indexFrom] = indexTo
@@ -293,7 +299,6 @@ func (b *sourcePathsBuilder) remapDependencies(
 			sourcePathsRemap.markDeleted(path)
 			dependencyChanges[indexFrom] = -1
 		}
-		indexFrom++
 	}
 	// Add imports picked up via a public import. The filtered files do not use public imports.
 	if publicImportCount := len(importsRequired); publicImportCount > 0 {
@@ -314,14 +319,16 @@ func (b *sourcePathsBuilder) remapDependencies(
 			newWeakDependencies = weakDependencies[:0]
 		}
 		weakDependencyPath := append(sourcePath, fileWeakDependencyTag)
-		for _, indexFrom := range weakDependencies {
-			path := append(weakDependencyPath, indexFrom)
+		for i, indexFrom := range weakDependencies {
+			weakFrom := int32(i)
+			path := append(weakDependencyPath, weakFrom)
 			indexTo := dependencyChanges[indexFrom]
 			if indexTo == -1 {
 				sourcePathsRemap.markDeleted(path)
 			} else {
-				if indexTo != indexFrom {
-					sourcePathsRemap.markMoved(path, indexTo)
+				weakTo := int32(len(newWeakDependencies))
+				if weakTo != weakFrom {
+					sourcePathsRemap.markMoved(path, weakTo)
 				}
 				newWeakDependencies = append(newWeakDependencies, indexTo)
 			}

private/bufpkg/bufimage/bufimageutil/testdata/sourcecodeinfo/Bar.txtar

@@ -1,3 +1,52 @@
+-- test.proto --
+// Keep 1: comment on syntax
+syntax = "proto2";
+// Keep 2: comment on package
+package foo.bar;
+// Keep 3: comment on option
+option go_package = "foo.bar/baz";
+// "Keep if Bar" are also kept for Svc and Svc.Do since those
+// elements depend on Bar.
+// Keep if Bar: comment on message Bar
+message Bar {
+  // Keep if Bar: comment on field foo
+  optional Foo foo = 1;
+  // Keep if Bar: comment on oneof status
+  oneof status {
+    // Keep if Bar: comment on field baz
+    Baz baz = 2;
+    // Keep if Bar: comment on field quz
+    Quz quz = 3;
+  }
+}
+// "Keep if Foo" are also kept for Bar, Svc, and Svc.Do since those
+// elements depend on Foo.
+// Keep if Foo: comment on message Foo
+message Foo {
+  // We keep the following comment for NestedFoo because we don't
+  // discard options or option comments even if we're only retaining
+  // a message as a namespace for another retained element.
+  // Keep if Foo: comment on field name
+  optional string name = 1;
+  // Keep if Foo: comment on field bits
+  repeated int64 bits = 2;
+  extensions 100 to max;
+  reserved 10 to 20;
+  reserved "abc", "def", "ghi";
+}
+// "Keep if Baz" and "Keep if Quz" are also kept for Bar, Svc, and Svc.Do
+// since those elements depend on Baz and Quz.
+// Keep if Baz: comment on enum Baz
+enum Baz {
+  // Keep if Baz: comment on enum value BAZ_UNSPECIFIED
+  BAZ_UNSPECIFIED = 0;
+}
+// Keep if Quz: comment on enum Quz
+enum Quz {
+  // Keep if Quz: comment on enum value QUZ_UNSPECIFIED
+  QUZ_UNSPECIFIED = 0;
+}
+-- source_code_info.json --
 {
   "location": [
     {
@@ -52,17 +101,6 @@
       ],
       "leadingComments": " Keep 3: comment on option\n"
     },
-    {
-      "path": [
-        11,
-        0
-      ],
-      "span": [
-        13,
-        7,
-        11
-      ]
-    },
     {
       "path": [
         4,

private/bufpkg/bufimage/bufimageutil/testdata/sourcecodeinfo/Baz.txtar

@@ -1,3 +1,18 @@
+-- test.proto --
+// Keep 1: comment on syntax
+syntax = "proto2";
+// Keep 2: comment on package
+package foo.bar;
+// Keep 3: comment on option
+option go_package = "foo.bar/baz";
+// "Keep if Baz" and "Keep if Quz" are also kept for Bar, Svc, and Svc.Do
+// since those elements depend on Baz and Quz.
+// Keep if Baz: comment on enum Baz
+enum Baz {
+  // Keep if Baz: comment on enum value BAZ_UNSPECIFIED
+  BAZ_UNSPECIFIED = 0;
+}
+-- source_code_info.json --
 {
   "location": [
     {
@@ -52,17 +67,6 @@
       ],
       "leadingComments": " Keep 3: comment on option\n"
     },
-    {
-      "path": [
-        11,
-        0
-      ],
-      "span": [
-        13,
-        7,
-        11
-      ]
-    },
     {
       "path": [
         5,