From 3039476726e4d5c3b2d7a946b1a5612b278b4525 Mon Sep 17 00:00:00 2001 From: Lasse Folger Date: Tue, 19 Mar 2024 09:14:46 +0100 Subject: [PATCH] all: implement proto2/proto3 as editions [2/2] This change removes the remaining usages of Syntax() from Go Protobuf and uses edition features instead. It also adds a new function to the EnumDescriptor interface checking if the enum is using a Closed semantics. All of these changes were tested on the Google corpus. Change-Id: I7a8110f6f3b6ed24bf7ece500b4942371302c56c Reviewed-on: https://go-review.googlesource.com/c/protobuf/+/572695 Reviewed-by: Michael Stapelberg LUCI-TryBot-Result: Go LUCI --- internal/descfmt/stringer.go | 1 + internal/filedesc/desc.go | 3 ++ internal/filedesc/desc_test.go | 3 ++ internal/filedesc/placeholder.go | 1 + reflect/protodesc/desc.go | 4 +-- reflect/protodesc/desc_validate.go | 45 +++++++++++++++++------------- reflect/protodesc/file_test.go | 4 +-- reflect/protoreflect/type.go | 6 ++++ 8 files changed, 43 insertions(+), 24 deletions(-) diff --git a/internal/descfmt/stringer.go b/internal/descfmt/stringer.go index a45625c8d..87e46bd4d 100644 --- a/internal/descfmt/stringer.go +++ b/internal/descfmt/stringer.go @@ -252,6 +252,7 @@ func formatDescOpt(t protoreflect.Descriptor, isRoot, allowMulti bool, record fu {rv.MethodByName("Values"), "Values"}, {rv.MethodByName("ReservedNames"), "ReservedNames"}, {rv.MethodByName("ReservedRanges"), "ReservedRanges"}, + {rv.MethodByName("IsClosed"), "IsClosed"}, }...) case protoreflect.EnumValueDescriptor: diff --git a/internal/filedesc/desc.go b/internal/filedesc/desc.go index c17c128fa..469b3ed8e 100644 --- a/internal/filedesc/desc.go +++ b/internal/filedesc/desc.go @@ -202,6 +202,9 @@ func (ed *Enum) lazyInit() *EnumL2 { ed.L0.ParentFile.lazyInit() // implicitly initializes L2 return ed.L2 } +func (ed *Enum) IsClosed() bool { + return !ed.L1.EditionFeatures.IsOpenEnum +} func (ed *EnumValue) Options() protoreflect.ProtoMessage { if f := ed.L1.Options; f != nil { diff --git a/internal/filedesc/desc_test.go b/internal/filedesc/desc_test.go index 21919d1e3..4a8173086 100644 --- a/internal/filedesc/desc_test.go +++ b/internal/filedesc/desc_test.go @@ -718,6 +718,7 @@ func testFileFormat(t *testing.T, fd protoreflect.FileDescriptor) { {Name: FOO} {Name: BAR, Number: 1} ] + IsClosed: true }] Extensions: [{ Name: X @@ -739,6 +740,7 @@ func testFileFormat(t *testing.T, fd protoreflect.FileDescriptor) { ] ReservedNames: [FIZZ, BUZZ] ReservedRanges: [10:20, 30] + IsClosed: true }] Extensions: [{ Name: X @@ -772,6 +774,7 @@ func testFileFormat(t *testing.T, fd protoreflect.FileDescriptor) { ] ReservedNames: [FIZZ, BUZZ] ReservedRanges: [10:20, 30] + IsClosed: true }}` const wantExtensions = `Extensions{{ diff --git a/internal/filedesc/placeholder.go b/internal/filedesc/placeholder.go index 28240ebc5..bfb3b8417 100644 --- a/internal/filedesc/placeholder.go +++ b/internal/filedesc/placeholder.go @@ -63,6 +63,7 @@ func (e PlaceholderEnum) Options() protoreflect.ProtoMessage { return des func (e PlaceholderEnum) Values() protoreflect.EnumValueDescriptors { return emptyEnumValues } func (e PlaceholderEnum) ReservedNames() protoreflect.Names { return emptyNames } func (e PlaceholderEnum) ReservedRanges() protoreflect.EnumRanges { return emptyEnumRanges } +func (e PlaceholderEnum) IsClosed() bool { return false } func (e PlaceholderEnum) ProtoType(protoreflect.EnumDescriptor) { return } func (e PlaceholderEnum) ProtoInternal(pragma.DoNotImplement) { return } diff --git a/reflect/protodesc/desc.go b/reflect/protodesc/desc.go index 2e67db1da..6813d6b83 100644 --- a/reflect/protodesc/desc.go +++ b/reflect/protodesc/desc.go @@ -219,10 +219,10 @@ func (o FileOptions) New(fd *descriptorpb.FileDescriptorProto, r Resolver) (prot if err := validateEnumDeclarations(f.L1.Enums.List, fd.GetEnumType()); err != nil { return nil, err } - if err := validateMessageDeclarations(f.L1.Messages.List, fd.GetMessageType()); err != nil { + if err := validateMessageDeclarations(f, f.L1.Messages.List, fd.GetMessageType()); err != nil { return nil, err } - if err := validateExtensionDeclarations(f.L1.Extensions.List, fd.GetExtension()); err != nil { + if err := validateExtensionDeclarations(f, f.L1.Extensions.List, fd.GetExtension()); err != nil { return nil, err } diff --git a/reflect/protodesc/desc_validate.go b/reflect/protodesc/desc_validate.go index e4dcaf876..eacddfed0 100644 --- a/reflect/protodesc/desc_validate.go +++ b/reflect/protodesc/desc_validate.go @@ -45,11 +45,11 @@ func validateEnumDeclarations(es []filedesc.Enum, eds []*descriptorpb.EnumDescri if allowAlias && !foundAlias { return errors.New("enum %q allows aliases, but none were found", e.FullName()) } - if e.Syntax() == protoreflect.Proto3 { + if !e.IsClosed() { if v := e.Values().Get(0); v.Number() != 0 { - return errors.New("enum %q using proto3 semantics must have zero number for the first value", v.FullName()) + return errors.New("enum %q using open semantics must have zero number for the first value", v.FullName()) } - // Verify that value names in proto3 do not conflict if the + // Verify that value names in open enums do not conflict if the // case-insensitive prefix is removed. // See protoc v3.8.0: src/google/protobuf/descriptor.cc:4991-5055 names := map[string]protoreflect.EnumValueDescriptor{} @@ -58,7 +58,7 @@ func validateEnumDeclarations(es []filedesc.Enum, eds []*descriptorpb.EnumDescri v1 := e.Values().Get(i) s := strs.EnumValueName(strs.TrimEnumPrefix(string(v1.Name()), prefix)) if v2, ok := names[s]; ok && v1.Number() != v2.Number() { - return errors.New("enum %q using proto3 semantics has conflict: %q with %q", e.FullName(), v1.Name(), v2.Name()) + return errors.New("enum %q using open semantics has conflict: %q with %q", e.FullName(), v1.Name(), v2.Name()) } names[s] = v1 } @@ -80,7 +80,9 @@ func validateEnumDeclarations(es []filedesc.Enum, eds []*descriptorpb.EnumDescri return nil } -func validateMessageDeclarations(ms []filedesc.Message, mds []*descriptorpb.DescriptorProto) error { +func validateMessageDeclarations(file *filedesc.File, ms []filedesc.Message, mds []*descriptorpb.DescriptorProto) error { + // There are a few limited exceptions only for proto3 + isProto3 := file.L1.Edition == fromEditionProto(descriptorpb.Edition_EDITION_PROTO3) for i, md := range mds { m := &ms[i] @@ -107,10 +109,10 @@ func validateMessageDeclarations(ms []filedesc.Message, mds []*descriptorpb.Desc if isMessageSet && !flags.ProtoLegacy { return errors.New("message %q is a MessageSet, which is a legacy proto1 feature that is no longer supported", m.FullName()) } - if isMessageSet && (m.Syntax() == protoreflect.Proto3 || m.Fields().Len() > 0 || m.ExtensionRanges().Len() == 0) { + if isMessageSet && (isProto3 || m.Fields().Len() > 0 || m.ExtensionRanges().Len() == 0) { return errors.New("message %q is an invalid proto1 MessageSet", m.FullName()) } - if m.Syntax() == protoreflect.Proto3 { + if isProto3 { if m.ExtensionRanges().Len() > 0 { return errors.New("message %q using proto3 semantics cannot have extension ranges", m.FullName()) } @@ -149,7 +151,7 @@ func validateMessageDeclarations(ms []filedesc.Message, mds []*descriptorpb.Desc return errors.New("message field %q may not have extendee: %q", f.FullName(), fd.GetExtendee()) } if f.L1.IsProto3Optional { - if f.Syntax() != protoreflect.Proto3 { + if !isProto3 { return errors.New("message field %q under proto3 optional semantics must be specified in the proto3 syntax", f.FullName()) } if f.Cardinality() != protoreflect.Optional { @@ -162,26 +164,29 @@ func validateMessageDeclarations(ms []filedesc.Message, mds []*descriptorpb.Desc if f.IsWeak() && !flags.ProtoLegacy { return errors.New("message field %q is a weak field, which is a legacy proto1 feature that is no longer supported", f.FullName()) } - if f.IsWeak() && (f.Syntax() != protoreflect.Proto2 || !isOptionalMessage(f) || f.ContainingOneof() != nil) { + if f.IsWeak() && (!f.HasPresence() || !isOptionalMessage(f) || f.ContainingOneof() != nil) { return errors.New("message field %q may only be weak for an optional message", f.FullName()) } if f.IsPacked() && !isPackable(f) { return errors.New("message field %q is not packable", f.FullName()) } - if err := checkValidGroup(f); err != nil { + if err := checkValidGroup(file, f); err != nil { return errors.New("message field %q is an invalid group: %v", f.FullName(), err) } if err := checkValidMap(f); err != nil { return errors.New("message field %q is an invalid map: %v", f.FullName(), err) } - if f.Syntax() == protoreflect.Proto3 { + if isProto3 { if f.Cardinality() == protoreflect.Required { return errors.New("message field %q using proto3 semantics cannot be required", f.FullName()) } - if f.Enum() != nil && !f.Enum().IsPlaceholder() && f.Enum().Syntax() != protoreflect.Proto3 { - return errors.New("message field %q using proto3 semantics may only depend on a proto3 enum", f.FullName()) + if f.Enum() != nil && !f.Enum().IsPlaceholder() && f.Enum().IsClosed() { + return errors.New("message field %q using proto3 semantics may only depend on open enums", f.FullName()) } } + if f.Cardinality() == protoreflect.Optional && !f.HasPresence() && f.Enum() != nil && !f.Enum().IsPlaceholder() && f.Enum().IsClosed() { + return errors.New("message field %q with implicit presence may only use open enums", f.FullName()) + } } seenSynthetic := false // synthetic oneofs for proto3 optional must come after real oneofs for j := range md.GetOneofDecl() { @@ -215,17 +220,17 @@ func validateMessageDeclarations(ms []filedesc.Message, mds []*descriptorpb.Desc if err := validateEnumDeclarations(m.L1.Enums.List, md.GetEnumType()); err != nil { return err } - if err := validateMessageDeclarations(m.L1.Messages.List, md.GetNestedType()); err != nil { + if err := validateMessageDeclarations(file, m.L1.Messages.List, md.GetNestedType()); err != nil { return err } - if err := validateExtensionDeclarations(m.L1.Extensions.List, md.GetExtension()); err != nil { + if err := validateExtensionDeclarations(file, m.L1.Extensions.List, md.GetExtension()); err != nil { return err } } return nil } -func validateExtensionDeclarations(xs []filedesc.Extension, xds []*descriptorpb.FieldDescriptorProto) error { +func validateExtensionDeclarations(f *filedesc.File, xs []filedesc.Extension, xds []*descriptorpb.FieldDescriptorProto) error { for i, xd := range xds { x := &xs[i] // NOTE: Avoid using the IsValid method since extensions to MessageSet @@ -267,13 +272,13 @@ func validateExtensionDeclarations(xs []filedesc.Extension, xds []*descriptorpb. if x.IsPacked() && !isPackable(x) { return errors.New("extension field %q is not packable", x.FullName()) } - if err := checkValidGroup(x); err != nil { + if err := checkValidGroup(f, x); err != nil { return errors.New("extension field %q is an invalid group: %v", x.FullName(), err) } if md := x.Message(); md != nil && md.IsMapEntry() { return errors.New("extension field %q cannot be a map entry", x.FullName()) } - if x.Syntax() == protoreflect.Proto3 { + if f.L1.Edition == fromEditionProto(descriptorpb.Edition_EDITION_PROTO3) { switch x.ContainingMessage().FullName() { case (*descriptorpb.FileOptions)(nil).ProtoReflect().Descriptor().FullName(): case (*descriptorpb.EnumOptions)(nil).ProtoReflect().Descriptor().FullName(): @@ -309,12 +314,12 @@ func isPackable(fd protoreflect.FieldDescriptor) bool { // checkValidGroup reports whether fd is a valid group according to the same // rules that protoc imposes. -func checkValidGroup(fd protoreflect.FieldDescriptor) error { +func checkValidGroup(f *filedesc.File, fd protoreflect.FieldDescriptor) error { md := fd.Message() switch { case fd.Kind() != protoreflect.GroupKind: return nil - case fd.Syntax() == protoreflect.Proto3: + case f.L1.Edition == fromEditionProto(descriptorpb.Edition_EDITION_PROTO3): return errors.New("invalid under proto3 semantics") case md == nil || md.IsPlaceholder(): return errors.New("message must be resolvable") diff --git a/reflect/protodesc/file_test.go b/reflect/protodesc/file_test.go index eeb9b60be..ed75a800c 100644 --- a/reflect/protodesc/file_test.go +++ b/reflect/protodesc/file_test.go @@ -592,7 +592,7 @@ func TestNewFile(t *testing.T) { value: [{name:"baz" number:500}] }]}] `), - wantErr: `enum "M.baz" using proto3 semantics must have zero number for the first value`, + wantErr: `enum "M.baz" using open semantics must have zero number for the first value`, }, { label: "valid proto3 enum", inDesc: mustParseFile(` @@ -613,7 +613,7 @@ func TestNewFile(t *testing.T) { value: [{name:"e_Foo" number:0}, {name:"fOo" number:1}] }]}] `), - wantErr: `enum "M.E" using proto3 semantics has conflict: "fOo" with "e_Foo"`, + wantErr: `enum "M.E" using open semantics has conflict: "fOo" with "e_Foo"`, }, { label: "proto2 enum has name prefix check", inDesc: mustParseFile(` diff --git a/reflect/protoreflect/type.go b/reflect/protoreflect/type.go index 60ff62b4c..5b80afe52 100644 --- a/reflect/protoreflect/type.go +++ b/reflect/protoreflect/type.go @@ -544,6 +544,12 @@ type EnumDescriptor interface { // ReservedRanges is a list of reserved ranges of enum numbers. ReservedRanges() EnumRanges + // IsClosed reports whether this enum uses closed semantics. + // See https://protobuf.dev/programming-guides/enum/#definitions. + // Note: the Go protobuf implementation is not spec compliant and treats + // all enums as open enums. + IsClosed() bool + isEnumDescriptor } type isEnumDescriptor interface{ ProtoType(EnumDescriptor) }