Skip to content

Commit

Permalink
Update to match Editions behavior of latest protoc (v25.0) (#191)
Browse files Browse the repository at this point in the history
Between v24 and v25, the implementation or editions inside of `protoc`
changed quite rather drastically. Most of the complexity having to do
with computation of feature defaults and feature set resolution has been
completely removed 🎉.

This still does not perform some of the checks for field features that
`protoc` performs, so that still needs to come in a subsequent branch.
But this does bring this repo up to the same level of
`protoc`-compatibility it had before, but this time with `protoc` v25
(instead of v24). It also does add one add'l check: that the `packed`
field option is not used with editions.
  • Loading branch information
jhump authored Oct 27, 2023
1 parent af2f079 commit 24bce88
Show file tree
Hide file tree
Showing 19 changed files with 355 additions and 759 deletions.
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
)

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": {
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": {
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

0 comments on commit 24bce88

Please sign in to comment.