Map fields and message values in map entries cannot be group-encoded (#312)

Author: jhump

Makefile

@@ -156,7 +156,7 @@ internal/testdata/desc_test_proto3_optional.protoset: $(PROTOC) internal/testdat
 internal/testdata/descriptor_impl_tests.protoset: $(PROTOC) internal/testdata/desc_test2.proto internal/testdata/desc_test_complex.proto internal/testdata/desc_test_defaults.proto internal/testdata/desc_test_proto3.proto internal/testdata/desc_test_proto3_optional.proto
 	cd $(@D) && $(PROTOC) --descriptor_set_out=$(@F) --include_imports -I. $(filter-out protoc,$(^F))
 
-internal/testdata/descriptor_editions_impl_tests.protoset: $(PROTOC) internal/testdata/editions/all_default_features.proto internal/testdata/editions/features_with_overrides.proto
+internal/testdata/descriptor_editions_impl_tests.protoset: $(PROTOC) internal/testdata/editions/all_default_features.proto internal/testdata/editions/features_with_overrides.proto internal/testdata/editions/file_default_delimited.proto
 	cd $(@D)/editions && $(PROTOC) --descriptor_set_out=../$(@F) --include_imports -I. $(filter-out protoc,$(^F))
 
 internal/testdata/editions/all.protoset: $(PROTOC) $(sort $(wildcard internal/testdata/editions/*.proto))

internal/testdata/descriptor_editions_impl_tests.protoset

@@ -0,0 +1,12 @@
+edition = "2023";
+
+package foo.bar.baz;
+
+option features.message_encoding = DELIMITED;
+
+message TestMessage {
+  TestMessage child = 1;
+  repeated TestMessage descendants = 2;
+  map<string, string> string_map = 3;
+  map<string, TestMessage> message_map = 4;
+}

internal/testdata/editions/all.protoset

@@ -1126,7 +1126,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 {
+	if f.proto.GetType() == descriptorpb.FieldDescriptorProto_TYPE_MESSAGE && f.Syntax() == protoreflect.Editions &&
+		!f.IsMap() && !f.parentIsMap() {
 		// 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)
@@ -1240,6 +1241,11 @@ func (f *fldDescriptor) isMapEntry() bool {
 	return f.Message().IsMapEntry()
 }
 
+func (f *fldDescriptor) parentIsMap() bool {
+	parent, ok := f.parent.(protoreflect.MessageDescriptor)
+	return ok && parent.IsMapEntry()
+}
+
 func (f *fldDescriptor) MapKey() protoreflect.FieldDescriptor {
 	if !f.IsMap() {
 		return nil

internal/testdata/editions/file_default_delimited.proto

@@ -55,6 +55,7 @@ func TestFields(t *testing.T) {
 		{"desc_test_proto3_optional.proto", files},
 		{"all_default_features.proto", editionFiles},
 		{"features_with_overrides.proto", editionFiles},
+		{"file_default_delimited.proto", editionFiles},
 	}
 	for _, testCase := range testCases {
 		testCase := testCase // must not capture loop variable below, for thread safety
@@ -150,7 +151,13 @@ func checkAttributesInFields(t *testing.T, exp, actual protoreflect.ExtensionDes
 		}
 		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())
+		if isMapOrMapEntryMessageValue(actFld) {
+			// TODO: Remove this branch and just use the check below once the protobuf-go runtime fixes
+			//       https://github.com/golang/protobuf/issues/1615
+			assert.Equal(t, protoreflect.MessageKind, actFld.Kind(), "%s: field kind at index %d (%s)", where, i, expFld.Name())
+		} else {
+			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())
@@ -241,3 +248,14 @@ func checkAttributesInEnums(t *testing.T, exp, actual protoreflect.EnumDescripto
 		assert.Equal(t, expEnum.IsClosed(), actEnum.IsClosed(), "%s: enum is closed at index %d (%s)", where, i, expEnum.Name())
 	}
 }
+
+func isMapOrMapEntryMessageValue(field protoreflect.FieldDescriptor) bool {
+	if field.IsMap() {
+		return true // map field
+	}
+	if field.Message() == nil {
+		return false // not a message field
+	}
+	parent, ok := field.Parent().(protoreflect.MessageDescriptor)
+	return ok && parent.IsMapEntry()
+}