Skip to content

Commit

Permalink
fix extension resolution in custom options to match protoc (#29)
Browse files Browse the repository at this point in the history
Port jhump/protoreflect#484 to protocompile.

Co-authored-by: Joshua Humphries <jhumphries@buf.build>
  • Loading branch information
pkwarren and jhump authored Sep 15, 2022
1 parent aae326a commit 0b9251a
Show file tree
Hide file tree
Showing 6 changed files with 360 additions and 109 deletions.
98 changes: 98 additions & 0 deletions internal/message_context.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,98 @@
// Copyright 2020-2022 Buf Technologies, Inc.
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package internal

import (
"bytes"
"fmt"

"google.golang.org/protobuf/types/descriptorpb"

"github.com/bufbuild/protocompile/ast"
)

// ParsedFile wraps an optional AST and required FileDescriptorProto.
// This is used so types like parser.Result can be passed to this internal package avoiding circular imports.
// Additionally, it makes it less likely that users might specify one or the other.
type ParsedFile interface {
// AST returns the parsed abstract syntax tree. This returns nil if the
// Result was created without an AST.
AST() *ast.FileNode
// FileDescriptorProto returns the file descriptor proto.
FileDescriptorProto() *descriptorpb.FileDescriptorProto
}

// MessageContext provides information about the location in a descriptor
// hierarchy, for adding context to warnings and error messages.
type MessageContext struct {
// The relevant file
File ParsedFile

// The type and fully-qualified name of the element within the file.
ElementType string
ElementName string

// If the element being processed is an option (or *in* an option)
// on the named element above, this will be non-nil.
Option *descriptorpb.UninterpretedOption
// If the element being processed is inside a message literal in an
// option value, this will be non-empty and represent a traversal
// to the element in question.
OptAggPath string
}

func (c *MessageContext) String() string {
var ctx bytes.Buffer
if c.ElementType != "file" {
_, _ = fmt.Fprintf(&ctx, "%s %s: ", c.ElementType, c.ElementName)
}
if c.Option != nil && c.Option.Name != nil {
ctx.WriteString("option ")
writeOptionName(&ctx, c.Option.Name)
if c.File.AST() == nil {
// if we have no source position info, try to provide as much context
// as possible (if nodes != nil, we don't need this because any errors
// will actually have file and line numbers)
if c.OptAggPath != "" {
_, _ = fmt.Fprintf(&ctx, " at %s", c.OptAggPath)
}
}
ctx.WriteString(": ")
}
return ctx.String()
}

func writeOptionName(buf *bytes.Buffer, parts []*descriptorpb.UninterpretedOption_NamePart) {
first := true
for _, p := range parts {
if first {
first = false
} else {
buf.WriteByte('.')
}
nm := p.GetNamePart()
if nm[0] == '.' {
// skip leading dot
nm = nm[1:]
}
if p.GetIsExtension() {
buf.WriteByte('(')
buf.WriteString(nm)
buf.WriteByte(')')
} else {
buf.WriteString(nm)
}
}
}
6 changes: 6 additions & 0 deletions linker/descriptors.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
"google.golang.org/protobuf/types/descriptorpb"
"google.golang.org/protobuf/types/dynamicpb"

"github.com/bufbuild/protocompile/ast"
"github.com/bufbuild/protocompile/internal"
"github.com/bufbuild/protocompile/parser"
"github.com/bufbuild/protocompile/protoutil"
Expand All @@ -49,6 +50,11 @@ type result struct {
optionBytes map[proto.Message][]byte
srcLocs []protoreflect.SourceLocation
srcLocIndex map[interface{}]protoreflect.SourceLocation
// 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
}

var _ protoreflect.FileDescriptor = (*result)(nil)
Expand Down
17 changes: 11 additions & 6 deletions linker/linker.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import (
"google.golang.org/protobuf/reflect/protoreflect"
"google.golang.org/protobuf/types/descriptorpb"

"github.com/bufbuild/protocompile/ast"
"github.com/bufbuild/protocompile/parser"
"github.com/bufbuild/protocompile/reporter"
)
Expand Down Expand Up @@ -60,12 +61,13 @@ func Link(parsed parser.Result, dependencies Files, symbols *Symbols, handler *r
}

r := &result{
Result: parsed,
deps: dependencies,
descriptors: map[proto.Message]protoreflect.Descriptor{},
descriptorPool: map[string]proto.Message{},
usedImports: map[string]struct{}{},
prefix: prefix,
Result: parsed,
deps: dependencies,
descriptors: map[proto.Message]protoreflect.Descriptor{},
descriptorPool: map[string]proto.Message{},
usedImports: map[string]struct{}{},
prefix: prefix,
optionQualifiedNames: map[ast.IdentValueNode]string{},
}

// First, we put all symbols into a single pool, which lets us ensure there
Expand Down Expand Up @@ -118,6 +120,9 @@ type Result interface {
// extension that is available in this file. If no such element is available
// or if the named element is not an extension, nil is returned.
ResolveExtension(protoreflect.FullName) protoreflect.ExtensionTypeDescriptor
// ResolveMessageLiteralExtensionName returns the fully qualified name for
// an identifier for extension field names in message literals.
ResolveMessageLiteralExtensionName(ast.IdentValueNode) string
// ValidateOptions runs some validation checks on the descriptor that can only
// be done after options are interpreted. Any errors or warnings encountered
// will be reported via the given handler. If any error is reported, this
Expand Down
117 changes: 116 additions & 1 deletion linker/linker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -533,7 +533,7 @@ extend Foo { optional group Bar = 10 { optional string name = 1; } }
extend google.protobuf.MessageOptions { optional Foo foo = 10001; }
message Baz { option (foo) = { [Bar]< name: "abc" > }; }`,
},
expectedErr: "foo.proto:6:32: message Baz: option (foo): field Bar not found",
expectedErr: "foo.proto:6:33: message Baz: option (foo): invalid extension: Bar is a message, not an extension",
},
"failure_oneof_extension_already_set": {
input: map[string]string{
Expand Down Expand Up @@ -805,6 +805,121 @@ message Baz {
},
expectedErr: "foo.proto:8:6: syntax error: unexpected '.'",
},
"success_extension_resolution_custom_options": {
input: map[string]string{
"test.proto": `syntax="proto2";
package foo.bar;
import "google/protobuf/descriptor.proto";
message a { extensions 1 to 100; }
extend google.protobuf.MessageOptions { optional a msga = 10000; }
message b {
message c {
extend a { repeated int32 i = 1; repeated float f = 2; }
}
option (msga) = {
[foo.bar.b.c.i]: 123
[bar.b.c.i]: 234
[b.c.i]: 345
};
option (msga).(foo.bar.b.c.f) = 1.23;
option (msga).(bar.b.c.f) = 2.34;
option (msga).(b.c.f) = 3.45;
}`,
},
},
"failure_extension_resolution_custom_options": {
input: map[string]string{
"test.proto": `syntax="proto2";
package foo.bar;
import "google/protobuf/descriptor.proto";
message a { extensions 1 to 100; }
message b { extensions 1 to 100; }
extend google.protobuf.MessageOptions { optional a msga = 10000; }
message c {
extend a { optional b b = 1; }
extend b { repeated int32 i = 1; repeated float f = 2; }
option (msga) = {
[foo.bar.c.b] {
[foo.bar.c.i]: 123
[bar.c.i]: 234
[c.i]: 345
}
};
option (msga).(foo.bar.c.b).(foo.bar.c.f) = 1.23;
option (msga).(foo.bar.c.b).(bar.c.f) = 2.34;
option (msga).(foo.bar.c.b).(c.f) = 3.45;
}`,
},
expectedErr: "test.proto:9:10: extendee is invalid: foo.bar.c.b is a extension, not a message",
},
"failure_extension_resolution_unknown": {
input: map[string]string{
"test.proto": `syntax="proto2";
package foo.bar;
import "google/protobuf/descriptor.proto";
message a { extensions 1 to 100; }
extend google.protobuf.MessageOptions { optional a msga = 10000; }
message b {
message c {
extend a { repeated int32 i = 1; repeated float f = 2; }
}
option (msga) = {
[c.i]: 456
};
}`,
},
expectedErr: "test.proto:11:6: message foo.bar.b: option (foo.bar.msga): unknown extension c.i",
},
"failure_extension_resolution_unknown2": {
input: map[string]string{
"test.proto": `syntax="proto2";
package foo.bar;
import "google/protobuf/descriptor.proto";
message a { extensions 1 to 100; }
extend google.protobuf.MessageOptions { optional a msga = 10000; }
message b {
message c {
extend a { repeated int32 i = 1; repeated float f = 2; }
}
option (msga) = {
[i]: 567
};
}`,
},
expectedErr: "test.proto:11:6: message foo.bar.b: option (foo.bar.msga): unknown extension i",
},
"failure_extension_resolution_unknown3": {
input: map[string]string{
"test.proto": `syntax="proto2";
package foo.bar;
import "google/protobuf/descriptor.proto";
message a { extensions 1 to 100; }
extend google.protobuf.MessageOptions { optional a msga = 10000; }
message b {
message c {
extend a { repeated int32 i = 1; repeated float f = 2; }
}
option (msga).(c.f) = 4.56;
}`,
},
expectedErr: "test.proto:10:17: message foo.bar.b: unknown extension c.f",
},
"failure_extension_resolution_unknown4": {
input: map[string]string{
"test.proto": `syntax="proto2";
package foo.bar;
import "google/protobuf/descriptor.proto";
message a { extensions 1 to 100; }
extend google.protobuf.MessageOptions { optional a msga = 10000; }
message b {
message c {
extend a { repeated int32 i = 1; repeated float f = 2; }
}
option (msga).(f) = 5.67;
}`,
},
expectedErr: "test.proto:10:17: message foo.bar.b: unknown extension f",
},
}

for name, tc := range testCases {
Expand Down
Loading

0 comments on commit 0b9251a

Please sign in to comment.