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 <[email protected]> LUCI-TryBot-Result: Go LUCI <[email protected]>
protocolbuffers · Mar 19, 2024 · 3039476 · 3039476
1 parent 7259b46
commit 3039476
Show file tree

Hide file tree

Showing 8 changed files with 43 additions and 24 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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) }