Repository: jhump/protoreflect
Pull request #484 (2022-02-03 23:59:46)

protoparse: fix extension resolution in custom options
The way protoparse resolved references to extension names in custom options (both in option names and also in field names inside message literal values) did not work exactly like protoc.

Example file:
```proto
syntax="proto2";
package foo.bar;
import "google/protobuf/descriptor.proto";

message a { extensions 1 to 100; }
extend google.protobuf.MessageOptions { optional a opt = 10000; }

message b {
  message c {
    extend a { repeated int32 i = 1; repeated float f = 2; }
  }

  // extension references inside a message literal:
  option (opt) = {
    [foo.bar.b.c.i]: 123
    [bar.b.c.i]: 234  // * this form is accepted by protoc but erroneously rejected by protoparse
    [b.c.i]: 345.     // * this form is accepted by protoc but erroneously rejected by protoparse

    // these forms are accepted by neither
    //[c.i]: 456.     
    //[i]: 567
  };

  // extension references inside the option name:
  option (opt).(foo.bar.b.c.f) = 1.23;
  option (opt).(bar.b.c.f) = 2.34;
  option (opt).(b.c.f) = 3.45;
  option (opt).(c.f) = 4.56;  // ** this form is rejected by protoc but erroneously accepted by protoparse

  // this form is accepted by neither
  //option (opt).(f) = 5.67;
}
```

As seen in comments above, there were some names that protoc resolves that cause errors in protoparse. And vice versa: there is one extension that protoparse was able to resolve, but should not have been if adhering to the same scoping rules that protoc implements.

Also visible above: protoc is consistent with how names are resolved, whether they are field names in message literals vs. in option names. But protoparse was quite _inconsistent_.

This PR fixes this and includes lots of fun tests to verify that all forms accepted by protoc are accepted by protoparse and also that forms rejected by protoc are also rejected by protoparse.




Patch:
diff --git a/desc/protoparse/linker.go b/desc/protoparse/linker.go
index 288b79e..3435026 100644
--- a/desc/protoparse/linker.go
+++ b/desc/protoparse/linker.go
@@ -311,7 +311,7 @@ func (l *linker) resolveReferences() error {
 			prefix += "."
 		}
 		if fd.Options != nil {
-			if err := l.resolveOptions(r, fd, "file", fd.GetName(), proto.MessageName(fd.Options), fd.Options.UninterpretedOption, scopes); err != nil {
+			if err := l.resolveOptions(r, fd, "file", fd.GetName(), fd.Options.UninterpretedOption, scopes); err != nil {
 				return err
 			}
 		}
@@ -342,14 +342,14 @@ func (l *linker) resolveReferences() error {
 func (l *linker) resolveEnumTypes(r *parseResult, fd *dpb.FileDescriptorProto, prefix string, ed *dpb.EnumDescriptorProto, scopes []scope) error {
 	enumFqn := prefix + ed.GetName()
 	if ed.Options != nil {
-		if err := l.resolveOptions(r, fd, "enum", enumFqn, proto.MessageName(ed.Options), ed.Options.UninterpretedOption, scopes); err != nil {
+		if err := l.resolveOptions(r, fd, "enum", enumFqn, ed.Options.UninterpretedOption, scopes); err != nil {
 			return err
 		}
 	}
 	for _, evd := range ed.Value {
 		if evd.Options != nil {
 			evFqn := enumFqn + "." + evd.GetName()
-			if err := l.resolveOptions(r, fd, "enum value", evFqn, proto.MessageName(evd.Options), evd.Options.UninterpretedOption, scopes); err != nil {
+			if err := l.resolveOptions(r, fd, "enum value", evFqn, evd.Options.UninterpretedOption, scopes); err != nil {
 				return err
 			}
 		}
@@ -359,16 +359,21 @@ func (l *linker) resolveEnumTypes(r *parseResult, fd *dpb.FileDescriptorProto, p
 
 func (l *linker) resolveMessageTypes(r *parseResult, fd *dpb.FileDescriptorProto, prefix string, md *dpb.DescriptorProto, scopes []scope) error {
 	fqn := prefix + md.GetName()
-	scope := messageScope(fqn, isProto3(fd), l, fd)
-	scopes = append(scopes, scope)
-	prefix = fqn + "."
 
+	// Strangely, when protoc resolves extension names, it uses the *enclosing* scope
+	// instead of the message's scope. So if the message contains an extension named "i",
+	// an option cannot refer to it as simply "i" but must qualify it (at a minimum "Msg.i").
+	// So we don't add this messages scope to our scopes slice until *after* we do options.
 	if md.Options != nil {
-		if err := l.resolveOptions(r, fd, "message", fqn, proto.MessageName(md.Options), md.Options.UninterpretedOption, scopes); err != nil {
+		if err := l.resolveOptions(r, fd, "message", fqn, md.Options.UninterpretedOption, scopes); err != nil {
 			return err
 		}
 	}
 
+	scope := messageScope(fqn, isProto3(fd), l, fd)
+	scopes = append(scopes, scope)
+	prefix = fqn + "."
+
 	for _, nmd := range md.NestedType {
 		if err := l.resolveMessageTypes(r, fd, prefix, nmd, scopes); err != nil {
 			return err
@@ -387,7 +392,7 @@ func (l *linker) resolveMessageTypes(r *parseResult, fd *dpb.FileDescriptorProto
 	for _, ood := range md.OneofDecl {
 		if ood.Options != nil {
 			ooName := fmt.Sprintf("%s.%s", fqn, ood.GetName())
-			if err := l.resolveOptions(r, fd, "oneof", ooName, proto.MessageName(ood.Options), ood.Options.UninterpretedOption, scopes); err != nil {
+			if err := l.resolveOptions(r, fd, "oneof", ooName, ood.Options.UninterpretedOption, scopes); err != nil {
 				return err
 			}
 		}
@@ -400,7 +405,7 @@ func (l *linker) resolveMessageTypes(r *parseResult, fd *dpb.FileDescriptorProto
 	for _, er := range md.ExtensionRange {
 		if er.Options != nil {
 			erName := fmt.Sprintf("%s:%d-%d", fqn, er.GetStart(), er.GetEnd()-1)
-			if err := l.resolveOptions(r, fd, "extension range", erName, proto.MessageName(er.Options), er.Options.UninterpretedOption, scopes); err != nil {
+			if err := l.resolveOptions(r, fd, "extension range", erName, er.Options.UninterpretedOption, scopes); err != nil {
 				return err
 			}
 		}
@@ -459,7 +464,7 @@ func (l *linker) resolveFieldTypes(r *parseResult, fd *dpb.FileDescriptorProto,
 	}
 
 	if fld.Options != nil {
-		if err := l.resolveOptions(r, fd, elemType, thisName, proto.MessageName(fld.Options), fld.Options.UninterpretedOption, scopes); err != nil {
+		if err := l.resolveOptions(r, fd, elemType, thisName, fld.Options.UninterpretedOption, scopes); err != nil {
 			return err
 		}
 	}
@@ -501,7 +506,7 @@ func (l *linker) resolveFieldTypes(r *parseResult, fd *dpb.FileDescriptorProto,
 func (l *linker) resolveServiceTypes(r *parseResult, fd *dpb.FileDescriptorProto, prefix string, sd *dpb.ServiceDescriptorProto, scopes []scope) error {
 	svcFqn := prefix + sd.GetName()
 	if sd.Options != nil {
-		if err := l.resolveOptions(r, fd, "service", svcFqn, proto.MessageName(sd.Options), sd.Options.UninterpretedOption, scopes); err != nil {
+		if err := l.resolveOptions(r, fd, "service", svcFqn, sd.Options.UninterpretedOption, scopes); err != nil {
 			return err
 		}
 	}
@@ -512,7 +517,7 @@ func (l *linker) resolveServiceTypes(r *parseResult, fd *dpb.FileDescriptorProto
 
 	for _, mtd := range sd.Method {
 		if mtd.Options != nil {
-			if err := l.resolveOptions(r, fd, "method", svcFqn+"."+mtd.GetName(), proto.MessageName(mtd.Options), mtd.Options.UninterpretedOption, scopes); err != nil {
+			if err := l.resolveOptions(r, fd, "method", svcFqn+"."+mtd.GetName(), mtd.Options.UninterpretedOption, scopes); err != nil {
 				return err
 			}
 		}
@@ -558,48 +563,108 @@ func (l *linker) resolveServiceTypes(r *parseResult, fd *dpb.FileDescriptorProto
 	return nil
 }
 
-func (l *linker) resolveOptions(r *parseResult, fd *dpb.FileDescriptorProto, elemType, elemName, optType string, opts []*dpb.UninterpretedOption, scopes []scope) error {
-	var scope string
-	if elemType != "file" {
-		scope = fmt.Sprintf("%s %s: ", elemType, elemName)
+func (l *linker) resolveOptions(r *parseResult, fd *dpb.FileDescriptorProto, elemType, elemName string, opts []*dpb.UninterpretedOption, scopes []scope) error {
+	mc := &messageContext{
+		res:         r,
+		elementName: elemName,
+		elementType: elemType,
 	}
 opts:
 	for _, opt := range opts {
+		// resolve any extension names found in option names
 		for _, nm := range opt.Name {
 			if nm.GetIsExtension() {
-				node := r.getOptionNamePartNode(nm)
-				fqn, dsc, _ := l.resolve(fd, nm.GetNamePart(), false, scopes)
-				if dsc == nil {
-					if err := l.errs.handleErrorWithPos(node.Start(), "%sunknown extension %s", scope, nm.GetNamePart()); err != nil {
-						return err
-					}
-					continue opts
-				}
-				if dsc == sentinelMissingSymbol {
-					if err := l.errs.handleErrorWithPos(node.Start(), "%sunknown extension %s; resolved to %s which is not defined; consider using a leading dot", scope, nm.GetNamePart(), fqn); err != nil {
+				fqn, err := l.resolveExtensionName(nm.GetNamePart(), fd, scopes)
+				if err != nil {
+					node := r.getOptionNamePartNode(nm)
+					if err := l.errs.handleErrorWithPos(node.Start(), "%v%v", mc, err); err != nil {
 						return err
 					}
 					continue opts
 				}
-				if ext, ok := dsc.(*dpb.FieldDescriptorProto); !ok {
-					otherType := descriptorType(dsc)
-					if err := l.errs.handleErrorWithPos(node.Start(), "%sinvalid extension: %s is a %s, not an extension", scope, nm.GetNamePart(), otherType); err != nil {
-						return err
-					}
-					continue opts
-				} else if ext.GetExtendee() == "" {
-					if err := l.errs.handleErrorWithPos(node.Start(), "%sinvalid extension: %s is a field but not an extension", scope, nm.GetNamePart()); err != nil {
+				nm.NamePart = proto.String(fqn)
+			}
+		}
+		// also resolve any extension names found inside message literals in option values
+		mc.option = opt
+		optVal := r.getOptionNode(opt).GetValue()
+		if err := l.resolveOptionValue(r, mc, fd, optVal, scopes); err != nil {
+			return err
+		}
+		mc.option = nil
+	}
+	return nil
+}
+
+func (l *linker) resolveOptionValue(r *parseResult, mc *messageContext, fd *dpb.FileDescriptorProto, val ast.ValueNode, scopes []scope) error {
+	optVal := val.Value()
+	switch optVal := optVal.(type) {
+	case []ast.ValueNode:
+		origPath := mc.optAggPath
+		defer func() {
+			mc.optAggPath = origPath
+		}()
+		for i, v := range optVal {
+			mc.optAggPath = fmt.Sprintf("%s[%d]", origPath, i)
+			if err := l.resolveOptionValue(r, mc, fd, v, scopes); err != nil {
+				return err
+			}
+		}
+	case []*ast.MessageFieldNode:
+		origPath := mc.optAggPath
+		defer func() {
+			mc.optAggPath = origPath
+		}()
+		for _, fld := range optVal {
+			// check for extension name
+			if fld.Name.IsExtension() {
+				fqn, err := l.resolveExtensionName(string(fld.Name.Name.AsIdentifier()), fd, scopes)
+				if err != nil {
+					if err := l.errs.handleErrorWithPos(fld.Name.Name.Start(), "%v%v", mc, err); err != nil {
 						return err
 					}
-					continue opts
+				} else {
+					r.optionQualifiedNames[fld.Name.Name] = fqn
 				}
-				nm.NamePart = proto.String("." + fqn)
+			}
+
+			// recurse into value
+			mc.optAggPath = origPath
+			if origPath != "" {
+				mc.optAggPath += "."
+			}
+			if fld.Name.IsExtension() {
+				mc.optAggPath = fmt.Sprintf("%s[%s]", mc.optAggPath, string(fld.Name.Name.AsIdentifier()))
+			} else {
+				mc.optAggPath = fmt.Sprintf("%s%s", mc.optAggPath, string(fld.Name.Name.AsIdentifier()))
+			}
+
+			if err := l.resolveOptionValue(r, mc, fd, fld.Val, scopes); err != nil {
+				return err
 			}
 		}
 	}
+
 	return nil
 }
 
+func (l *linker) resolveExtensionName(name string, fd *dpb.FileDescriptorProto, scopes []scope) (string, error) {
+	fqn, dsc, _ := l.resolve(fd, name, false, scopes)
+	if dsc == nil {
+		return "", fmt.Errorf("unknown extension %s", name)
+	}
+	if dsc == sentinelMissingSymbol {
+		return "", fmt.Errorf("unknown extension %s; resolved to %s which is not defined; consider using a leading dot", name, fqn)
+	}
+	if ext, ok := dsc.(*dpb.FieldDescriptorProto); !ok {
+		otherType := descriptorType(dsc)
+		return "", fmt.Errorf("invalid extension: %s is a %s, not an extension", name, otherType)
+	} else if ext.GetExtendee() == "" {
+		return "", fmt.Errorf("invalid extension: %s is a field but not an extension", name)
+	}
+	return "." + fqn, nil
+}
+
 func (l *linker) resolve(fd *dpb.FileDescriptorProto, name string, onlyTypes bool, scopes []scope) (fqn string, element proto.Message, proto3 bool) {
 	if strings.HasPrefix(name, ".") {
 		// already fully-qualified
diff --git a/desc/protoparse/options.go b/desc/protoparse/options.go
index 646f499..5a53ac7 100644
--- a/desc/protoparse/options.go
+++ b/desc/protoparse/options.go
@@ -1348,10 +1348,15 @@ func fieldValue(res *parseResult, mc *messageContext, fld fldDescriptorish, val
 				}
 				var ffld *desc.FieldDescriptor
 				if a.Name.IsExtension() {
-					n := string(a.Name.Name.AsIdentifier())
+					n := res.optionQualifiedNames[a.Name.Name]
+					if n == "" {
+						// this should not be possible!
+						n = string(a.Name.Name.AsIdentifier())
+					}
 					ffld = findExtension(mc.file, n, false, map[fileDescriptorish]struct{}{})
 					if ffld == nil {
 						// may need to qualify with package name
+						// (this should not be necessary!)
 						pkg := mc.file.GetPackage()
 						if pkg != "" {
 							ffld = findExtension(mc.file, pkg+"."+n, false, map[fileDescriptorish]struct{}{})
diff --git a/desc/protoparse/parser.go b/desc/protoparse/parser.go
index 338a0a7..d9801cd 100644
--- a/desc/protoparse/parser.go
+++ b/desc/protoparse/parser.go
@@ -634,6 +634,12 @@ type parseResult struct {
 	// a map of uninterpreted option AST nodes to their relative path
 	// in the resulting options message
 	interpretedOptions map[*ast.OptionNode][]int32
+
+	// a map of AST nodes that represent identifiers in ast.FieldReferenceNodes
+	// to their fully-qualified name. The identifiers are for field names in
+	// message literals (in option values) that are extension fields. These names
+	// are resolved during linking and stored here, to be used to interpret options.
+	optionQualifiedNames map[ast.IdentValueNode]string
 }
 
 func (r *parseResult) getFileNode(f *dpb.FileDescriptorProto) ast.FileDeclNode {
@@ -788,10 +794,11 @@ func parseProto(filename string, r io.Reader, errs *errorHandler, validate, crea
 
 func createParseResult(filename string, file *ast.FileNode, errs *errorHandler, createProtos bool) *parseResult {
 	res := &parseResult{
-		errs:               errs,
-		root:               file,
-		nodes:              map[proto.Message]ast.Node{},
-		interpretedOptions: map[*ast.OptionNode][]int32{},
+		errs:                 errs,
+		root:                 file,
+		nodes:                map[proto.Message]ast.Node{},
+		interpretedOptions:   map[*ast.OptionNode][]int32{},
+		optionQualifiedNames: map[ast.IdentValueNode]string{},
 	}
 	if createProtos {
 		res.createFileDescriptor(filename, file)


Test patch:
diff --git a/desc/protoparse/linker_test.go b/desc/protoparse/linker_test.go
index 78e263e..f27f09e 100644
--- a/desc/protoparse/linker_test.go
+++ b/desc/protoparse/linker_test.go
@@ -521,7 +521,7 @@ func TestLinkerValidation(t *testing.T) {
 					"extend google.protobuf.MessageOptions { optional Foo foo = 10001; }\n" +
 					"message Baz { option (foo) = { [Bar]< name: \"abc\" > }; }\n",
 			},
-			"foo.proto:6:30: message Baz: option (foo): field Bar not found",
+			"foo.proto:6:33: message Baz: option (foo): invalid extension: Bar is a message, not an extension",
 		},
 		{
 			map[string]string{
@@ -559,13 +559,130 @@ func TestLinkerValidation(t *testing.T) {
 		},
 		{
 			map[string]string{
-				"a.proto": "syntax=\"proto3\";\nmessage m{\n" +
+				"a.proto": "syntax=\"proto3\";\n" +
+					"message m{\n" +
 					"  string z = 1;\n" +
 					"  oneof z{int64 b=2;}\n" +
 					"}",
 			},
 			`a.proto:3:3: duplicate symbol m.z: already defined as oneof`,
 		},
+		{
+			map[string]string{
+				"test.proto": "syntax=\"proto2\";\n" +
+					"package foo.bar;\n" +
+					"import \"google/protobuf/descriptor.proto\";\n" +
+					"message a { extensions 1 to 100; }\n" +
+					"extend google.protobuf.MessageOptions { optional a msga = 10000; }\n" +
+					"message b {\n" +
+					"  message c {\n" +
+					"    extend a { repeated int32 i = 1; repeated float f = 2; }\n" +
+					"  }\n" +
+					"  option (msga) = {\n" +
+					"    [foo.bar.b.c.i]: 123\n" +
+					"    [bar.b.c.i]: 234\n" +
+					"    [b.c.i]: 345\n" +
+					"  };\n" +
+					"  option (msga).(foo.bar.b.c.f) = 1.23;\n" +
+					"  option (msga).(bar.b.c.f) = 2.34;\n" +
+					"  option (msga).(b.c.f) = 3.45;\n" +
+					"}",
+			},
+			"", // should succeed
+		},
+		{
+			map[string]string{
+				"test.proto": "syntax=\"proto2\";\n" +
+					"package foo.bar;\n" +
+					"import \"google/protobuf/descriptor.proto\";\n" +
+					"message a { extensions 1 to 100; }\n" +
+					"message b { extensions 1 to 100; }\n" +
+					"extend google.protobuf.MessageOptions { optional a msga = 10000; }\n" +
+					"message c {\n" +
+					"  extend a { optional b b = 1; }\n" +
+					"  extend b { repeated int32 i = 1; repeated float f = 2; }\n" +
+					"  option (msga) = {\n" +
+					"    [foo.bar.c.b] {\n" +
+					"      [foo.bar.c.i]: 123\n" +
+					"      [bar.c.i]: 234\n" +
+					"      [c.i]: 345\n" +
+					"    }\n" +
+					"  };\n" +
+					"  option (msga).(foo.bar.c.b).(foo.bar.c.f) = 1.23;\n" +
+					"  option (msga).(foo.bar.c.b).(bar.c.f) = 2.34;\n" +
+					"  option (msga).(foo.bar.c.b).(c.f) = 3.45;\n" +
+					"}",
+			},
+			"", // should succeed
+		},
+		{
+			map[string]string{
+				"test.proto": "syntax=\"proto2\";\n" +
+					"package foo.bar;\n" +
+					"import \"google/protobuf/descriptor.proto\";\n" +
+					"message a { extensions 1 to 100; }\n" +
+					"extend google.protobuf.MessageOptions { optional a msga = 10000; }\n" +
+					"message b {\n" +
+					"  message c {\n" +
+					"    extend a { repeated int32 i = 1; repeated float f = 2; }\n" +
+					"  }\n" +
+					"  option (msga) = {\n" +
+					"    [c.i]: 456\n" +
+					"  };\n" +
+					"}",
+			},
+			"test.proto:11:6: message foo.bar.b: option (foo.bar.msga): unknown extension c.i",
+		},
+		{
+			map[string]string{
+				"test.proto": "syntax=\"proto2\";\n" +
+					"package foo.bar;\n" +
+					"import \"google/protobuf/descriptor.proto\";\n" +
+					"message a { extensions 1 to 100; }\n" +
+					"extend google.protobuf.MessageOptions { optional a msga = 10000; }\n" +
+					"message b {\n" +
+					"  message c {\n" +
+					"    extend a { repeated int32 i = 1; repeated float f = 2; }\n" +
+					"  }\n" +
+					"  option (msga) = {\n" +
+					"    [i]: 567\n" +
+					"  };\n" +
+					"}",
+			},
+			"test.proto:11:6: message foo.bar.b: option (foo.bar.msga): unknown extension i",
+		},
+		{
+			map[string]string{
+				"test.proto": "syntax=\"proto2\";\n" +
+					"package foo.bar;\n" +
+					"import \"google/protobuf/descriptor.proto\";\n" +
+					"message a { extensions 1 to 100; }\n" +
+					"extend google.protobuf.MessageOptions { optional a msga = 10000; }\n" +
+					"message b {\n" +
+					"  message c {\n" +
+					"    extend a { repeated int32 i = 1; repeated float f = 2; }\n" +
+					"  }\n" +
+					"  option (msga).(c.f) = 4.56;\n" +
+					"}",
+			},
+			"test.proto:10:17: message foo.bar.b: unknown extension c.f",
+		},
+		{
+			map[string]string{
+				"test.proto": "syntax=\"proto2\";\n" +
+					"package foo.bar;\n" +
+					"import \"google/protobuf/descriptor.proto\";\n" +
+					"message a { extensions 1 to 100; }\n" +
+					"extend google.protobuf.MessageOptions { optional a msga = 10000; }\n" +
+					"message b {\n" +
+					"  message c {\n" +
+					"    extend a { repeated int32 i = 1; repeated float f = 2; }\n" +
+					"  }\n" +
+					"  option (msga).(f) = 5.67;\n" +
+					"}",
+			},
+			"test.proto:10:17: message foo.bar.b: unknown extension f",
+		},
 		{
 			map[string]string{
 				"a.proto": "syntax=\"proto3\";\nmessage m{\n" +