Found and fixed a bug in FieldDescriptor.Cardinality implementation (#270)

Updates the descriptor implementation tests to check a few other
attributes, which uncovered a bug in the `Cardinality` method, which
should report "required" for editions fields where
`features.field_presence == LEGACY_REQUIRED`.

Author: jhump

linker/descriptors.go

@@ -1047,6 +1047,14 @@ func (f *fldDescriptor) Cardinality() protoreflect.Cardinality {
 	case descriptorpb.FieldDescriptorProto_LABEL_REQUIRED:
 		return protoreflect.Required
 	case descriptorpb.FieldDescriptorProto_LABEL_OPTIONAL:
+		if f.Syntax() == protoreflect.Editions {
+			// Editions does not use label to indicate required. It instead
+			// uses a feature, and label is always optional.
+			fieldPresence := descriptorpb.FeatureSet_FieldPresence(resolveFeature(f, fieldPresenceField).Enum())
+			if fieldPresence == descriptorpb.FeatureSet_LEGACY_REQUIRED {
+				return protoreflect.Required
+			}
+		}
 		return protoreflect.Optional
 	default:
 		return 0
@@ -1055,6 +1063,8 @@ func (f *fldDescriptor) Cardinality() protoreflect.Cardinality {
 
 func (f *fldDescriptor) Kind() protoreflect.Kind {
 	if f.proto.GetType() == descriptorpb.FieldDescriptorProto_TYPE_MESSAGE && f.Syntax() == protoreflect.Editions {
+		// In editions, "group encoding" (aka "delimited encoding") is toggled
+		// via a feature. So we report group kind when that feature is enabled.
 		messageEncoding := resolveFeature(f, messageEncodingField)
 		if descriptorpb.FeatureSet_MessageEncoding(messageEncoding.Enum()) == descriptorpb.FeatureSet_DELIMITED {
 			return protoreflect.GroupKind

linker/descriptors_ext_test.go

@@ -148,11 +148,16 @@ func checkAttributesInFields(t *testing.T, exp, actual protoreflect.ExtensionDes
 		if !assert.Equal(t, expFld.Name(), actFld.Name(), "%s: field name at index %d", where, i) {
 			continue
 		}
+		assert.Equal(t, expFld.Number(), actFld.Number(), "%s: field number at index %d (%s)", where, i, expFld.Name())
+		assert.Equal(t, expFld.Cardinality(), actFld.Cardinality(), "%s: field cardinality at index %d (%s)", where, i, expFld.Name())
 		assert.Equal(t, expFld.Kind(), actFld.Kind(), "%s: field kind at index %d (%s)", where, i, expFld.Name())
 		assert.Equal(t, expFld.IsList(), actFld.IsList(), "%s: field is list at index %d (%s)", where, i, expFld.Name())
 		assert.Equal(t, expFld.IsMap(), actFld.IsMap(), "%s: field is map at index %d (%s)", where, i, expFld.Name())
 		assert.Equal(t, expFld.JSONName(), actFld.JSONName(), "%s: field json name at index %d (%s)", where, i, expFld.Name())
 		assert.Equal(t, expFld.HasJSONName(), actFld.HasJSONName(), "%s: field has json name at index %d (%s)", where, i, expFld.Name())
+		assert.Equal(t, expFld.IsExtension(), actFld.IsExtension(), "%s: field is extension at index %d (%s)", where, i, expFld.Name())
+		assert.Equal(t, expFld.IsPacked(), actFld.IsPacked(), "%s: field is packed at index %d (%s)", where, i, expFld.Name())
+		assert.Equal(t, expFld.ContainingOneof() == nil, actFld.ContainingOneof() == nil, "%s: field containing oneof at index %d (%s)", where, i, expFld.Name())
 
 		// default values