Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update to match Editions behavior of latest protoc (v25.0) #191

Merged
merged 8 commits into from
Oct 27, 2023
2 changes: 1 addition & 1 deletion .protoc_version
Original file line number Diff line number Diff line change
@@ -1 +1 @@
24.0-rc2
25.0-rc2
29 changes: 13 additions & 16 deletions compiler.go
Original file line number Diff line number Diff line change
Expand Up @@ -141,13 +141,12 @@ func (c *Compiler) Compile(ctx context.Context, files ...string) (linker.Files,
h := reporter.NewHandler(c.Reporter)

e := executor{
c: c,
h: h,
s: semaphore.NewWeighted(int64(par)),
cancel: cancel,
sym: &linker.Symbols{},
resCache: &options.FeaturesResolverCache{},
results: map[string]*result{},
c: c,
h: h,
s: semaphore.NewWeighted(int64(par)),
cancel: cancel,
sym: &linker.Symbols{},
results: map[string]*result{},
}

// We lock now and create all tasks under lock to make sure that no
Expand Down Expand Up @@ -232,12 +231,11 @@ func (r *result) getBlockedOn() []string {
}

type executor struct {
c *Compiler
h *reporter.Handler
s *semaphore.Weighted
cancel context.CancelFunc
sym *linker.Symbols
resCache *options.FeaturesResolverCache
c *Compiler
h *reporter.Handler
s *semaphore.Weighted
cancel context.CancelFunc
sym *linker.Symbols

descriptorProtoCheck sync.Once
descriptorProtoIsCustom bool
Expand Down Expand Up @@ -577,10 +575,9 @@ func (t *task) link(parseRes parser.Result, deps linker.Files, overrideDescripto
return nil, err
}

interpretOpts := make([]options.InterpreterOption, 1, 2)
interpretOpts[0] = options.WithFeaturesResolverCache(t.e.resCache)
var interpretOpts []options.InterpreterOption
if overrideDescriptorProtoRes != nil {
interpretOpts = append(interpretOpts, options.WithOverrideDescriptorProto(overrideDescriptorProtoRes))
interpretOpts = []options.InterpreterOption{options.WithOverrideDescriptorProto(overrideDescriptorProtoRes)}
}

optsIndex, err := options.InterpretOptions(file, t.h, interpretOpts...)
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ require (
github.com/google/go-cmp v0.6.0
github.com/stretchr/testify v1.8.4
golang.org/x/sync v0.4.0
google.golang.org/protobuf v1.31.1-0.20230727123859-6d0a5dbd9500
google.golang.org/protobuf v1.31.1-0.20231027082548-f4a6c1f6e5c1
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This latest version of protobuf-go pulls in the latest descriptor.proto changes.

)

require (
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ golang.org/x/sync v0.4.0 h1:zxkM55ReGkDlKSM+Fu41A+zmbZuaPVbGMzvvdUPznYQ=
golang.org/x/sync v0.4.0/go.mod h1:FU7BRWz2tNW+3quACPkgCx/L+uEAv1htQ0V83Z9Rj+Y=
golang.org/x/xerrors v0.0.0-20191204190536-9bdfabe68543/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0=
google.golang.org/protobuf v1.26.0-rc.1/go.mod h1:jlhhOSvTdKEhbULTjvd4ARK9grFBp09yW+WbY/TyQbw=
google.golang.org/protobuf v1.31.1-0.20230727123859-6d0a5dbd9500 h1:7y2/WECm0zxQchjsPsWLAajGJ77KApK/0JpIaBOeDvk=
google.golang.org/protobuf v1.31.1-0.20230727123859-6d0a5dbd9500/go.mod h1:HV8QOd/L58Z+nl8r43ehVNZIU/HEI6OcFqwMG9pJV4I=
google.golang.org/protobuf v1.31.1-0.20231027082548-f4a6c1f6e5c1 h1:fk72uXZyuZiTtW5tgd63jyVK6582lF61nRC/kGv6vCA=
google.golang.org/protobuf v1.31.1-0.20231027082548-f4a6c1f6e5c1/go.mod h1:HV8QOd/L58Z+nl8r43ehVNZIU/HEI6OcFqwMG9pJV4I=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM=
gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0=
gopkg.in/yaml.v3 v3.0.1 h1:fxVm/GzAzEWqLHuvctI91KS9hhNmmWOoWu0XTYJS7CA=
Expand Down
13 changes: 12 additions & 1 deletion internal/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,26 @@ type hasOptionNode interface {
FileNode() ast.FileDeclNode // needed in order to query for NodeInfo
}

func FindFirstOption(res hasOptionNode, handler *reporter.Handler, scope string, opts []*descriptorpb.UninterpretedOption, name string) (int, error) {
return findOption(res, handler, scope, opts, name, false, true)
}

func FindOption(res hasOptionNode, handler *reporter.Handler, scope string, opts []*descriptorpb.UninterpretedOption, name string) (int, error) {
return findOption(res, handler, scope, opts, name, true, false)
}

func findOption(res hasOptionNode, handler *reporter.Handler, scope string, opts []*descriptorpb.UninterpretedOption, name string, exact, first bool) (int, error) {
found := -1
for i, opt := range opts {
if len(opt.Name) != 1 {
if exact && len(opt.Name) != 1 {
continue
}
if opt.Name[0].GetIsExtension() || opt.Name[0].GetNamePart() != name {
continue
}
if first {
return i, nil
}
if found >= 0 {
optNode := res.OptionNode(opt)
fn := res.FileNode()
Expand Down
3 changes: 3 additions & 0 deletions internal/tags.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,9 @@ const (
// FileSyntaxTag is the tag number of the syntax element in a file
// descriptor proto.
FileSyntaxTag = 12
// FileEditionTag is the tag number of the edition element in a file
// descriptor proto.
FileEditionTag = 14
// MessageNameTag is the tag number of the name element in a message
// descriptor proto.
MessageNameTag = 1
Expand Down
Binary file modified internal/testdata/all.protoset
Binary file not shown.
Binary file modified internal/testdata/desc_test_complex.protoset
Binary file not shown.
Binary file modified internal/testdata/desc_test_proto3_optional.protoset
Binary file not shown.
Binary file modified internal/testdata/descriptor_impl_tests.protoset
Binary file not shown.
Binary file modified internal/testdata/editions/all.protoset
Binary file not shown.
2 changes: 1 addition & 1 deletion internal/testdata/editions/features_with_overrides.proto
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ message Foo {
string abc = 2 [features.field_presence = EXPLICIT];
int32 def = 3;
repeated bool ghi = 4 [features.repeated_field_encoding = EXPANDED];
map<string, string> jkl = 5 [features.string_field_validation = NONE];
map<string, string> jkl = 5 [features.utf8_validation = NONE];
Bar mno = 6 [features.message_encoding = DELIMITED];
repeated double pqr = 7;
map<string, Bar> stu = 8;
Expand Down
65 changes: 24 additions & 41 deletions linker/linker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2025,9 +2025,6 @@ func TestLinkerValidation(t *testing.T) {
`,
},
expectedErr: `test.proto:1:11: edition value "2024" not recognized; should be one of ["2023"]`,
// protoc v24.0-rc2 doesn't (yet?) reject unrecognized editions that
// sort *after* 2023; only ones before 2023 🤷
expectedDiffWithProtoc: true,
},
"failure_unknown_edition_past": {
input: map[string]string{
Expand All @@ -2041,44 +2038,6 @@ func TestLinkerValidation(t *testing.T) {
},
expectedErr: `test.proto:1:11: edition value "2022" not recognized; should be one of ["2023"]`,
},
"failure_use_of_features_without_editions": {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This moved into the parser's descriptor validation, so is done in parser tests instead of in linker tests.

input: map[string]string{
"test.proto": `
syntax = "proto3";
message Foo {
string foo = 1 [features.field_presence = LEGACY_REQUIRED];
int32 bar = 2 [features.field_presence = IMPLICIT];
}
`,
},
expectedErr: `test.proto:3:25: field Foo.foo: option 'features' may only be used with editions but file uses "proto3" syntax`,
},
"failure_direct_use_of_raw_features": {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no longer a raw_features field in google.protobuf.FeatureSet.

input: map[string]string{
"test.proto": `
edition = "2023";
message Foo {
string foo = 1 [features.raw_features.field_presence = LEGACY_REQUIRED];
}
`,
},
expectedErr: `test.proto:3:34: feature field "raw_features" may not be used explicitly`,
expectedDiffWithProtoc: true, // seems like a bug in protoc that it allows use of raw_features
},
"failure_direct_use_of_raw_features_in_message_literal": {
input: map[string]string{
"test.proto": `
edition = "2023";
message Foo {
string foo = 1 [features = {
raw_features: <field_presence: IMPLICIT>
}];
}
`,
},
expectedErr: `test.proto:4:5: feature field "raw_features" may not be used explicitly`,
expectedDiffWithProtoc: true, // seems like a bug in protoc that it allows use of raw_features
},
"success_proto2_packed": {
input: map[string]string{
"test.proto": `
Expand Down Expand Up @@ -2250,6 +2209,30 @@ func TestLinkerValidation(t *testing.T) {
},
expectedErr: `test.proto:3:3: packed option is only allowed on repeated fields`,
},
"failure_editions_feature_on_wrong_target_type": {
input: map[string]string{
"test.proto": `
edition = "2023";
message Foo {
int32 i32 = 1 [features.enum_type=OPEN];
}
`,
},
expectedErr: `test.proto:3:18: feature "enum_type" is allowed on [enum,file], not on field`,
},
"failure_editions_feature_on_wrong_target_type_msg_literal": {
input: map[string]string{
"test.proto": `
edition = "2023";
message Foo {
int32 i32 = 1 [features={
enum_type: OPEN
}];
}
`,
},
expectedErr: `test.proto:4:5: feature "enum_type" is allowed on [enum,file], not on field`,
},
}

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