Skip to content

Commit

Permalink
Add final checks that were added to protoc between v27.0-rc3 and fina…
Browse files Browse the repository at this point in the history
…l v27.0 (bufbuild#309)

This change mirrors two of the changes in the final v27.0 of protoc
that were not previously implemented in this compiler:

1. A new `feature_support` field on `EnumValueOptions` to allow defining
the lifetime of a feature value. This is similar to the field of the
same name and type on `FieldOptions`, but it controls the actual enum
values and in which editions they are valid.
2. A new check that a feature is not used from the same file in which
it's defined.

This change updates this repo to use the latest (final) v27.0 release and
also updates the protobuf-go dependency to include the v27.0 version
of the `descriptorpb` package.
  • Loading branch information
jhump authored May 30, 2024
1 parent 85801e4 commit 4190af1
Show file tree
Hide file tree
Showing 10 changed files with 196 additions and 59 deletions.
2 changes: 1 addition & 1 deletion .protoc_version
Original file line number Diff line number Diff line change
@@ -1 +1 @@
27.0-rc1
27.0
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.9.0
golang.org/x/sync v0.7.0
google.golang.org/protobuf v1.34.1
google.golang.org/protobuf v1.34.2-0.20240529085009-ca837e5c658b
)

require (
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -8,8 +8,8 @@ github.com/stretchr/testify v1.9.0 h1:HtqpIVDClZ4nwg75+f6Lvsy/wHu+3BoSGCbBAcpTsT
github.com/stretchr/testify v1.9.0/go.mod h1:r2ic/lqez/lEtzL7wO/rwa5dbSLXVDPFyf8C91i36aY=
golang.org/x/sync v0.7.0 h1:YsImfSBoP9QPYL0xyKJPq0gcaJdG3rInoqxTWbfQu9M=
golang.org/x/sync v0.7.0/go.mod h1:Czt+wKu1gCyEFDUtn0jG5QVvpJ6rzVqr5aXyt9drQfk=
google.golang.org/protobuf v1.34.1 h1:9ddQBjfCyZPOHPUiPxpYESBLc+T8P3E+Vo4IbKZgFWg=
google.golang.org/protobuf v1.34.1/go.mod h1:c6P6GXX6sHbq/GpV6MGZEdwhWPcYBgnhAHhKbcUYpos=
google.golang.org/protobuf v1.34.2-0.20240529085009-ca837e5c658b h1:lkoSgiT5AF1nbA/WLFPX1H0fvBiNGQekpG17/3aIsck=
google.golang.org/protobuf v1.34.2-0.20240529085009-ca837e5c658b/go.mod h1:qYOHts0dSfpeUzUFpOMr/WGzszTmLH+DiWniOlNbLDw=
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
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.
106 changes: 74 additions & 32 deletions linker/linker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -490,13 +490,6 @@ func TestLinkerValidation(t *testing.T) {
repeated Enum ens = 3;
}`,
},
// This will be fixed before the v27.0 final release but is currently
// broken in v27.0-rc1. It reports issues with all three extensions:
// test.proto:12:10: Implicit presence fields can't specify defaults.
// test.proto:13:8: Implicit presence enum fields must always be open.
// test.proto:14:17: Implicit presence enum fields must always be open.
// https://github.com/protocolbuffers/protobuf/issues/16664
expectedDiffWithProtoc: true,
},
"failure_message_set_wire_format_scalar": {
input: map[string]string{
Expand Down Expand Up @@ -1585,10 +1578,6 @@ func TestLinkerValidation(t *testing.T) {
string FOO_BAR = 2;
}`,
},
// protodesc.NewFile is applying overly strict checks on name
// collisions in proto3 files.
// https://github.com/golang/protobuf/issues/1616
expectProtodescFail: true,
},
"failure_json_name_conflict_leading_underscores": {
input: map[string]string{
Expand Down Expand Up @@ -3647,10 +3636,10 @@ func TestLinkerValidation(t *testing.T) {
},
"success_custom_feature_within_lifetime": {
input: map[string]string{
"test.proto": `
"feature.proto": `
edition = "2023";
import "google/protobuf/descriptor.proto";
package foo;
import "google/protobuf/descriptor.proto";
message CustomFeatures {
bool flag = 1 [
feature_support = {
Expand All @@ -3661,13 +3650,18 @@ func TestLinkerValidation(t *testing.T) {
];
}
extend google.protobuf.FeatureSet {
CustomFeatures custom = 1000;
CustomFeatures custom = 9995;
}
`,
"test.proto": `
edition = "2023";
package foo;
import "feature.proto";
option features.(custom).flag = true;
`,
},
},
"success_custom_feature_deprecated": {
"failure_custom_feature_in_same_file": {
input: map[string]string{
"test.proto": `
edition = "2023";
Expand All @@ -3683,18 +3677,46 @@ func TestLinkerValidation(t *testing.T) {
];
}
extend google.protobuf.FeatureSet {
CustomFeatures custom = 1000;
CustomFeatures custom = 9995;
}
option features.(custom).flag = true;
`,
},
expectedErr: `test.proto:16:1: custom feature (foo.custom) cannot be used from the same file in which it is defined`,
},
"failure_custom_feature_not_yet_introduced": {
"success_custom_feature_deprecated": {
input: map[string]string{
"test.proto": `
"feature.proto": `
edition = "2023";
package foo;
import "google/protobuf/descriptor.proto";
message CustomFeatures {
bool flag = 1 [
feature_support = {
edition_introduced: EDITION_2023
edition_deprecated: EDITION_2023
edition_removed: EDITION_2024
}
];
}
extend google.protobuf.FeatureSet {
CustomFeatures custom = 9995;
}
`,
"test.proto": `
edition = "2023";
package foo;
import "feature.proto";
option features.(custom).flag = true;
`,
},
},
"failure_custom_feature_not_yet_introduced": {
input: map[string]string{
"feature.proto": `
edition = "2023";
package foo;
import "google/protobuf/descriptor.proto";
message CustomFeatures {
bool flag = 1 [
feature_support = {
Expand All @@ -3704,19 +3726,24 @@ func TestLinkerValidation(t *testing.T) {
];
}
extend google.protobuf.FeatureSet {
CustomFeatures custom = 1000;
CustomFeatures custom = 9995;
}
`,
"test.proto": `
edition = "2023";
package foo;
import "feature.proto";
option features.(custom).flag = true;
`,
},
expectedErr: `test.proto:15:1: field "foo.CustomFeatures.flag" was not introduced until edition 2024`,
expectedErr: `test.proto:4:1: field "foo.CustomFeatures.flag" was not introduced until edition 2024`,
},
"failure_custom_feature_not_yet_introduced_msg_literal": {
input: map[string]string{
"test.proto": `
"feature.proto": `
edition = "2023";
import "google/protobuf/descriptor.proto";
package foo;
import "google/protobuf/descriptor.proto";
message CustomFeatures {
bool flag = 1 [
feature_support = {
Expand All @@ -3726,19 +3753,24 @@ func TestLinkerValidation(t *testing.T) {
];
}
extend google.protobuf.FeatureSet {
CustomFeatures custom = 1000;
CustomFeatures custom = 9995;
}
`,
"test.proto": `
edition = "2023";
package foo;
import "feature.proto";
option features.(custom) = { flag: true };
`,
},
expectedErr: `test.proto:15:30: field "foo.CustomFeatures.flag" was not introduced until edition 2024`,
expectedErr: `test.proto:4:30: field "foo.CustomFeatures.flag" was not introduced until edition 2024`,
},
"failure_custom_feature_not_yet_introduced_msg_literal2": {
input: map[string]string{
"test.proto": `
"feature.proto": `
edition = "2023";
import "google/protobuf/descriptor.proto";
package foo;
import "google/protobuf/descriptor.proto";
message CustomFeatures {
bool flag = 1 [
feature_support = {
Expand All @@ -3748,19 +3780,24 @@ func TestLinkerValidation(t *testing.T) {
];
}
extend google.protobuf.FeatureSet {
CustomFeatures custom = 1000;
CustomFeatures custom = 9995;
}
`,
"test.proto": `
edition = "2023";
package foo;
import "feature.proto";
option features = { [foo.custom]: { flag: true } };
`,
},
expectedErr: `test.proto:15:37: field "foo.CustomFeatures.flag" was not introduced until edition 2024`,
expectedErr: `test.proto:4:37: field "foo.CustomFeatures.flag" was not introduced until edition 2024`,
},
"failure_custom_feature_removed": {
input: map[string]string{
"test.proto": `
"feature.proto": `
edition = "2023";
import "google/protobuf/descriptor.proto";
package foo;
import "google/protobuf/descriptor.proto";
message CustomFeatures {
bool flag = 1 [
feature_support = {
Expand All @@ -3770,12 +3807,17 @@ func TestLinkerValidation(t *testing.T) {
];
}
extend google.protobuf.FeatureSet {
CustomFeatures custom = 1000;
CustomFeatures custom = 9995;
}
`,
"test.proto": `
edition = "2023";
package foo;
import "feature.proto";
option features.(custom).flag = true;
`,
},
expectedErr: `test.proto:15:1: field "foo.CustomFeatures.flag" was removed in edition 2023`,
expectedErr: `test.proto:4:1: field "foo.CustomFeatures.flag" was removed in edition 2023`,
},
}

Expand Down
122 changes: 106 additions & 16 deletions options/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -804,35 +804,74 @@ func (interp *interpreter) validateRecursive(
chInFeatures := isFeatures || inFeatures
chIsFeatures := !chInFeatures && len(path) == 0 && fld.Name() == "features"

if (isFeatures || (inFeatures && fld.IsExtension())) &&
interp.file.FileNode().Name() == fld.ParentFile().Path() {
var what, name string
if fld.IsExtension() {
what = "custom feature"
name = "(" + string(fld.FullName()) + ")"
} else {
what = "feature"
name = string(fld.Name())
}
node := interp.findOptionNode(path, element)
err = interp.reporter.HandleErrorf(interp.nodeInfo(node), "%s %s cannot be used from the same file in which it is defined", what, name)
if err != nil {
return false
}
}

if chInFeatures {
// Validate feature usage against feature settings.

// First, check the feature support settings of the field.
opts, _ := fld.Options().(*descriptorpb.FieldOptions)
edition := interp.file.FileDescriptorProto().GetEdition()
if opts != nil && opts.FeatureSupport != nil {
edition := interp.file.FileDescriptorProto().GetEdition()
if opts.FeatureSupport.EditionIntroduced != nil && edition < opts.FeatureSupport.GetEditionIntroduced() {
node := interp.findOptionNode(chpath, element)
err = interp.reporter.HandleErrorf(interp.nodeInfo(node), "field %q was not introduced until edition %s", fld.FullName(), editionString(opts.FeatureSupport.GetEditionIntroduced()))
err = interp.validateFeatureSupport(edition, opts.FeatureSupport, "field", string(fld.FullName()), chpath, element)
if err != nil {
return false
}
}
// Then, if it's an enum or has an enum, check the feature support settings of the enum values.
var enum protoreflect.EnumDescriptor
if fld.Enum() != nil {
enum = fld.Enum()
} else if fld.IsMap() && fld.MapValue().Enum() != nil {
enum = fld.MapValue().Enum()
}
if enum != nil {
switch {
case fld.IsMap():
val.Map().Range(func(k protoreflect.MapKey, v protoreflect.Value) bool {
// Can't construct path to particular map entry since we don't this entry's index.
// So we leave chpath alone, and it will have to point to the whole map value (or
// the first entry if the map is de-structured across multiple option statements).
err = interp.validateEnumValueFeatureSupport(edition, enum, v.Enum(), chpath, element)
return err == nil
})
if err != nil {
return false
}
}
if opts.FeatureSupport.EditionRemoved != nil && edition >= opts.FeatureSupport.GetEditionRemoved() {
node := interp.findOptionNode(chpath, element)
err = interp.reporter.HandleErrorf(interp.nodeInfo(node), "field %q was removed in edition %s", fld.FullName(), editionString(opts.FeatureSupport.GetEditionRemoved()))
case fld.IsList():
sl := val.List()
for i := 0; i < sl.Len(); i++ {
v := sl.Get(i)
err = interp.validateEnumValueFeatureSupport(edition, enum, v.Enum(), append(chpath, int32(i)), element)
if err != nil {
return false
}
}
default:
err = interp.validateEnumValueFeatureSupport(edition, enum, val.Enum(), chpath, element)
if err != nil {
return false
}
}
if opts.FeatureSupport.EditionDeprecated != nil && edition >= opts.FeatureSupport.GetEditionDeprecated() {
node := interp.findOptionNode(chpath, element)
var suffix string
if opts.FeatureSupport.GetDeprecationWarning() != "" {
suffix = ": " + opts.FeatureSupport.GetDeprecationWarning()
}
interp.reporter.HandleWarningf(interp.nodeInfo(node), "field %q is deprecated as of edition %s%s", fld.FullName(), editionString(opts.FeatureSupport.GetEditionDeprecated()), suffix)
}
}
}

// If it's a message or contains a message, recursively validate fields in those messages.
switch {
case fld.IsMap() && fld.MapValue().Message() != nil:
val.Map().Range(func(k protoreflect.MapKey, v protoreflect.Value) bool {
Expand Down Expand Up @@ -868,6 +907,57 @@ func (interp *interpreter) validateRecursive(
return err
}

func (interp *interpreter) validateEnumValueFeatureSupport(
edition descriptorpb.Edition,
enum protoreflect.EnumDescriptor,
number protoreflect.EnumNumber,
path []int32,
element proto.Message,
) error {
enumVal := enum.Values().ByNumber(number)
if enumVal == nil {
return nil
}
enumValOpts, _ := enumVal.Options().(*descriptorpb.EnumValueOptions)
if enumValOpts == nil || enumValOpts.FeatureSupport == nil {
return nil
}
return interp.validateFeatureSupport(edition, enumValOpts.FeatureSupport, "enum value", string(enumVal.Name()), path, element)
}

func (interp *interpreter) validateFeatureSupport(
edition descriptorpb.Edition,
featureSupport *descriptorpb.FieldOptions_FeatureSupport,
what string,
name string,
path []int32,
element proto.Message,
) error {
if featureSupport.EditionIntroduced != nil && edition < featureSupport.GetEditionIntroduced() {
node := interp.findOptionNode(path, element)
err := interp.reporter.HandleErrorf(interp.nodeInfo(node), "%s %q was not introduced until edition %s", what, name, editionString(featureSupport.GetEditionIntroduced()))
if err != nil {
return err
}
}
if featureSupport.EditionRemoved != nil && edition >= featureSupport.GetEditionRemoved() {
node := interp.findOptionNode(path, element)
err := interp.reporter.HandleErrorf(interp.nodeInfo(node), "%s %q was removed in edition %s", what, name, editionString(featureSupport.GetEditionRemoved()))
if err != nil {
return err
}
}
if featureSupport.EditionDeprecated != nil && edition >= featureSupport.GetEditionDeprecated() {
node := interp.findOptionNode(path, element)
var suffix string
if featureSupport.GetDeprecationWarning() != "" {
suffix = ": " + featureSupport.GetDeprecationWarning()
}
interp.reporter.HandleWarningf(interp.nodeInfo(node), "%s %q is deprecated as of edition %s%s", what, name, editionString(featureSupport.GetEditionDeprecated()), suffix)
}
return nil
}

func (interp *interpreter) findOptionNode(
path []int32,
element proto.Message,
Expand Down
Loading

0 comments on commit 4190af1

Please sign in to comment.