reflect/protodesc: add FileOptions

The FileOptions type provides the ability to specify specialized options
for how a file descriptor is constructed. It follows the same optional
arguments pattern as used in the proto package.

The resolver is not an option since it almost always necessary
when constructing a file descriptor.

Change-Id: Ib98ac6289881ad8402dd615f6c895da5899cb8d9
Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/218940
Reviewed-by: Damien Neil <dneil@google.com>

compiler/protogen/protogen_test.go

@@ -66,7 +66,7 @@ func TestNoGoPackage(t *testing.T) {
 				Name:       proto.String("testdata/go_package/no_go_package_import.proto"),
 				Syntax:     proto.String(protoreflect.Proto3.String()),
 				Package:    proto.String("goproto.testdata"),
-				Dependency: []string{"go_package/no_go_package.proto"},
+				Dependency: []string{"testdata/go_package/no_go_package.proto"},
 			},
 		},
 	}, nil)

reflect/protodesc/desc.go

@@ -28,37 +28,40 @@ type Resolver interface {
 	FindDescriptorByName(protoreflect.FullName) (protoreflect.Descriptor, error)
 }
 
-// option is an optional argument that may be provided to NewFile.
-type option struct {
-	pragma.DoNotCompare
-	allowUnresolvable bool
-}
+// FileOptions configures the construction of file descriptors.
+type FileOptions struct {
+	pragma.NoUnkeyedLiterals
 
-// allowUnresolvable configures NewFile to permissively allow unresolvable
-// file, enum, or message dependencies. Unresolved dependencies are replaced by
-// placeholder equivalents.
+	// AllowUnresolvable configures New to permissively allow unresolvable
+	// file, enum, or message dependencies. Unresolved dependencies are replaced
+	// by placeholder equivalents.
 	//
 	// The following dependencies may be left unresolved:
 	//	• Resolving an imported file.
 	//	• Resolving the type for a message field or extension field.
 	//	If the kind of the field is unknown, then a placeholder is used for both
-//	protoreflect.FieldDescriptor.Enum and protoreflect.FieldDescriptor.Message.
-//	• Resolving the enum value set as the default for an optional enum field.
+	//	the Enum and Message accessors on the protoreflect.FieldDescriptor.
+	//	• Resolving an enum value set as the default for an optional enum field.
 	//	If unresolvable, the protoreflect.FieldDescriptor.Default is set to the
-//	first enum value in the associated enum (or zero if the also enum dependency
+	//	first value in the associated enum (or zero if the also enum dependency
 	//	is also unresolvable). The protoreflect.FieldDescriptor.DefaultEnumValue
-//	is set as a placeholder.
+	//	is populated with a placeholder.
 	//	• Resolving the extended message type for an extension field.
 	//	• Resolving the input or output message type for a service method.
 	//
-// If the unresolved dependency uses a relative name, then the placeholder will
-// contain an invalid FullName with a "*." prefix, indicating that the starting
-// prefix of the full name is unknown.
-func allowUnresolvable() option {
-	return option{allowUnresolvable: true}
+	// If the unresolved dependency uses a relative name,
+	// then the placeholder will contain an invalid FullName with a "*." prefix,
+	// indicating that the starting prefix of the full name is unknown.
+	AllowUnresolvable bool
 }
 
 // NewFile creates a new protoreflect.FileDescriptor from the provided
+// file descriptor message. See FileOptions.New for more information.
+func NewFile(fd *descriptorpb.FileDescriptorProto, r Resolver) (protoreflect.FileDescriptor, error) {
+	return FileOptions{}.New(fd, r)
+}
+
+// New creates a new protoreflect.FileDescriptor from the provided
 // file descriptor message. The file must represent a valid proto file according
 // to protobuf semantics. The returned descriptor is a deep copy of the input.
 //
@@ -66,16 +69,7 @@ func allowUnresolvable() option {
 // resolved using the provided registry. When looking up an import file path,
 // the path must be unique. The newly created file descriptor is not registered
 // back into the provided file registry.
-func NewFile(fd *descriptorpb.FileDescriptorProto, r Resolver) (protoreflect.FileDescriptor, error) {
-	// TODO(blocks): remove setting allowUnresolvable once naughty users are migrated.
-	return newFile(fd, r, allowUnresolvable())
-}
-func newFile(fd *descriptorpb.FileDescriptorProto, r Resolver, opts ...option) (protoreflect.FileDescriptor, error) {
-	// Process the options.
-	var allowUnresolvable bool
-	for _, o := range opts {
-		allowUnresolvable = allowUnresolvable || o.allowUnresolvable
-	}
+func (o FileOptions) New(fd *descriptorpb.FileDescriptorProto, r Resolver) (protoreflect.FileDescriptor, error) {
 	if r == nil {
 		r = (*protoregistry.Files)(nil) // empty resolver
 	}
@@ -120,7 +114,7 @@ func newFile(fd *descriptorpb.FileDescriptorProto, r Resolver, opts ...option) (
 	for i, path := range fd.GetDependency() {
 		imp := &f.L2.Imports[i]
 		f, err := r.FindFileByPath(path)
-		if err == protoregistry.NotFound && (allowUnresolvable || imp.IsWeak) {
+		if err == protoregistry.NotFound && (o.AllowUnresolvable || imp.IsWeak) {
 			f = filedesc.PlaceholderFile(path)
 		} else if err != nil {
 			return nil, errors.New("could not resolve import %q: %v", path, err)
@@ -188,7 +182,7 @@ func newFile(fd *descriptorpb.FileDescriptorProto, r Resolver, opts ...option) (
 	}
 
 	// Step 2: Resolve every dependency reference not handled by step 1.
-	r2 := &resolver{local: r1, remote: r, imports: imps, allowUnresolvable: allowUnresolvable}
+	r2 := &resolver{local: r1, remote: r, imports: imps, allowUnresolvable: o.AllowUnresolvable}
 	if err := r2.resolveMessageDependencies(f.L1.Messages.List, fd.GetMessageType()); err != nil {
 		return nil, err
 	}

reflect/protodesc/file_test.go

@@ -88,7 +88,7 @@ func TestNewFile(t *testing.T) {
 		label    string
 		inDeps   []*descriptorpb.FileDescriptorProto
 		inDesc   *descriptorpb.FileDescriptorProto
-		inOpts   []option
+		inOpts   FileOptions
 		wantDesc *descriptorpb.FileDescriptorProto
 		wantErr  string
 	}{{
@@ -121,7 +121,7 @@ func TestNewFile(t *testing.T) {
 			package:    ""
 			dependency: "dep.proto"
 		`),
-		inOpts: []option{allowUnresolvable()},
+		inOpts: FileOptions{AllowUnresolvable: true},
 	}, {
 		label: "duplicate import",
 		inDesc: mustParseFile(`
@@ -129,7 +129,7 @@ func TestNewFile(t *testing.T) {
 			package:    ""
 			dependency: ["dep.proto", "dep.proto"]
 		`),
-		inOpts:  []option{allowUnresolvable()},
+		inOpts:  FileOptions{AllowUnresolvable: true},
 		wantErr: `already imported "dep.proto"`,
 	}, {
 		label: "invalid weak import",
@@ -139,7 +139,7 @@ func TestNewFile(t *testing.T) {
 			dependency:      "dep.proto"
 			weak_dependency: [-23]
 		`),
-		inOpts:  []option{allowUnresolvable()},
+		inOpts:  FileOptions{AllowUnresolvable: true},
 		wantErr: `invalid or duplicate weak import index: -23`,
 	}, {
 		label: "normal weak and public import",
@@ -150,7 +150,7 @@ func TestNewFile(t *testing.T) {
 			weak_dependency:   [0]
 			public_dependency: [0]
 		`),
-		inOpts: []option{allowUnresolvable()},
+		inOpts: FileOptions{AllowUnresolvable: true},
 	}, {
 		label: "import public indirect dependency duplicate",
 		inDeps: []*descriptorpb.FileDescriptorProto{
@@ -306,7 +306,7 @@ func TestNewFile(t *testing.T) {
 				enum_type: [{name:"E" value:[{name:"V0" number:0}]}]
 			}]
 		`),
-		inOpts: []option{allowUnresolvable()},
+		inOpts: FileOptions{AllowUnresolvable: true},
 	}, {
 		label: "unresolved extendee",
 		inDesc: mustParseFile(`
@@ -342,7 +342,7 @@ func TestNewFile(t *testing.T) {
 				method: [{name:"M" input_type:"foo.bar.input" output_type:".absolute.foo.bar.output"}]
 			}]
 		`),
-		inOpts: []option{allowUnresolvable()},
+		inOpts: FileOptions{AllowUnresolvable: true},
 	}, {
 		label: "resolved but not imported",
 		inDeps: []*descriptorpb.FileDescriptorProto{mustParseFile(`
@@ -856,7 +856,7 @@ func TestNewFile(t *testing.T) {
 				]
 			}]
 		`),
-		inOpts: []option{allowUnresolvable()},
+		inOpts: FileOptions{AllowUnresolvable: true},
 		// TODO: Test field and oneof handling in validateMessageDeclarations
 		// TODO: Test unmarshalDefault
 		// TODO: Test validateExtensionDeclarations
@@ -883,7 +883,7 @@ func TestNewFile(t *testing.T) {
 				}]
 			}]
 		`),
-		inOpts: []option{allowUnresolvable()},
+		inOpts: FileOptions{AllowUnresolvable: true},
 	}, {
 		label: "service with wrong reference type",
 		inDeps: []*descriptorpb.FileDescriptorProto{
@@ -910,7 +910,7 @@ func TestNewFile(t *testing.T) {
 		t.Run(tt.label, func(t *testing.T) {
 			r := new(protoregistry.Files)
 			for i, dep := range tt.inDeps {
-				f, err := newFile(dep, r)
+				f, err := tt.inOpts.New(dep, r)
 				if err != nil {
 					t.Fatalf("dependency %d: unexpected NewFile() error: %v", i, err)
 				}
@@ -922,7 +922,7 @@ func TestNewFile(t *testing.T) {
 			if tt.wantErr == "" && tt.wantDesc == nil {
 				tt.wantDesc = cloneFile(tt.inDesc)
 			}
-			gotFile, err := newFile(tt.inDesc, r, tt.inOpts...)
+			gotFile, err := tt.inOpts.New(tt.inDesc, r)
 			if gotFile != nil {
 				gotDesc = ToFileDescriptorProto(gotFile)
 			}