Expand global feature validation to cover some edge cases that were initially missed.

Notably, @jhump identified the following holes:
- Non-messages can have message_encoding set
- Extensions can have implicit presence set
- Repeated fields can have explicit presence set

PiperOrigin-RevId: 557571375

src/google/protobuf/descriptor.cc

@@ -7875,23 +7875,43 @@ void DescriptorBuilder::ValidateFieldFeatures(
   }
 
   // Validate explicitly specified features on the field proto.
-  if ((field->containing_oneof() != nullptr || field->is_repeated() ||
-       field->message_type() != nullptr) &&
-      field->proto_features_->field_presence() == FeatureSet::IMPLICIT) {
-    AddError(
-        field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
-        "Only singular scalar fields can specify implicit field presence.");
-  }
-  if ((field->containing_oneof() != nullptr || field->is_repeated()) &&
-      field->proto_features_->field_presence() == FeatureSet::LEGACY_REQUIRED) {
-    AddError(
-        field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
-        "Only singular scalar fields can specify required field presence.");
+  if (field->proto_features_->has_field_presence()) {
+    if (field->containing_oneof() != nullptr) {
+      AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
+               "Oneof fields can't specify field presence.");
+    } else if (field->is_repeated()) {
+      AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
+               "Repeated fields can't specify field presence.");
+    } else if (field->is_extension() &&
+               field->proto_features_->field_presence() !=
+                   FeatureSet::LEGACY_REQUIRED) {
+      // Note: required extensions will fail elsewhere, so we skip reporting a
+      // second error here.
+      AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
+               "Extensions can't specify field presence.");
+    } else if (field->message_type() != nullptr &&
+               field->proto_features_->field_presence() ==
+                   FeatureSet::IMPLICIT) {
+      AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
+               "Message fields can't specify implicit presence.");
+    }
   }
   if (!field->is_repeated() &&
       field->proto_features_->has_repeated_field_encoding()) {
     AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
-             "Only repeated fields can specify `repeated_field_encoding`.");
+             "Only repeated fields can specify repeated field encoding.");
+  }
+  if (!field->is_packable() &&
+      field->proto_features_->repeated_field_encoding() == FeatureSet::PACKED) {
+    AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
+             "Only repeated primitive fields can specify PACKED repeated field "
+             "encoding.");
+  }
+  if ((field->cpp_type() != FieldDescriptor::CPPTYPE_MESSAGE ||
+       field->is_map_message_type()) &&
+      field->proto_features_->has_message_encoding()) {
+    AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
+             "Only message fields can specify message encoding.");
   }
 }
 

src/google/protobuf/descriptor_unittest.cc

@@ -9165,6 +9165,29 @@ TEST_F(FeaturesTest, InvalidFieldRequiredExtension) {
       "foo.proto: bar: NAME: Extensions can't be required.\n");
 }
 
+TEST_F(FeaturesTest, InvalidFieldImplicitExtension) {
+  BuildDescriptorMessagesInTestPool();
+  BuildFileWithErrors(
+      R"pb(
+        name: "foo.proto"
+        syntax: "editions"
+        edition: "2023"
+        message_type {
+          name: "Foo"
+          extension_range { start: 1 end: 100 }
+        }
+        extension {
+          name: "bar"
+          number: 1
+          label: LABEL_OPTIONAL
+          type: TYPE_STRING
+          options { features { field_presence: IMPLICIT } }
+          extendee: "Foo"
+        }
+      )pb",
+      "foo.proto: bar: NAME: Extensions can't specify field presence.\n");
+}
+
 TEST_F(FeaturesTest, InvalidFieldMessageImplicit) {
   BuildDescriptorMessagesInTestPool();
   BuildFileWithErrors(
@@ -9184,8 +9207,8 @@ TEST_F(FeaturesTest, InvalidFieldMessageImplicit) {
           }
         }
       )pb",
-      "foo.proto: Foo.bar: NAME: Only singular scalar fields can specify "
-      "implicit field presence.\n");
+      "foo.proto: Foo.bar: NAME: Message fields can't specify implicit "
+      "presence.\n");
 }
 
 TEST_F(FeaturesTest, InvalidFieldRepeatedImplicit) {
@@ -9206,8 +9229,8 @@ TEST_F(FeaturesTest, InvalidFieldRepeatedImplicit) {
           }
         }
       )pb",
-      "foo.proto: Foo.bar: NAME: Only singular scalar fields can specify "
-      "implicit field presence.\n");
+      "foo.proto: Foo.bar: NAME: Repeated fields can't specify field "
+      "presence.\n");
 }
 
 TEST_F(FeaturesTest, InvalidFieldMapImplicit) {
@@ -9233,10 +9256,9 @@ TEST_F(FeaturesTest, InvalidFieldMapImplicit) {
   proto.set_name("foo.proto");
 
   BuildDescriptorMessagesInTestPool();
-  BuildFileWithErrors(
-      proto.DebugString(),
-      "foo.proto: Foo.bar: NAME: Only singular scalar fields can specify "
-      "implicit field presence.\n");
+  BuildFileWithErrors(proto.DebugString(),
+                      "foo.proto: Foo.bar: NAME: Repeated fields can't specify "
+                      "field presence.\n");
 }
 
 TEST_F(FeaturesTest, InvalidFieldOneofImplicit) {
@@ -9259,8 +9281,7 @@ TEST_F(FeaturesTest, InvalidFieldOneofImplicit) {
           oneof_decl { name: "_foo" }
         }
       )pb",
-      "foo.proto: Foo.bar: NAME: Only singular scalar fields can specify "
-      "implicit field presence.\n");
+      "foo.proto: Foo.bar: NAME: Oneof fields can't specify field presence.\n");
 }
 
 TEST_F(FeaturesTest, InvalidFieldRepeatedRequired) {
@@ -9281,8 +9302,8 @@ TEST_F(FeaturesTest, InvalidFieldRepeatedRequired) {
           }
         }
       )pb",
-      "foo.proto: Foo.bar: NAME: Only singular scalar fields can specify "
-      "required field presence.\n");
+      "foo.proto: Foo.bar: NAME: Repeated fields can't specify field "
+      "presence.\n");
 }
 
 TEST_F(FeaturesTest, InvalidFieldOneofRequired) {
@@ -9305,8 +9326,7 @@ TEST_F(FeaturesTest, InvalidFieldOneofRequired) {
           oneof_decl { name: "_foo" }
         }
       )pb",
-      "foo.proto: Foo.bar: NAME: Only singular scalar fields can specify "
-      "required field presence.\n");
+      "foo.proto: Foo.bar: NAME: Oneof fields can't specify field presence.\n");
 }
 
 TEST_F(FeaturesTest, InvalidFieldNonRepeatedWithRepeatedEncoding) {
@@ -9323,12 +9343,85 @@ TEST_F(FeaturesTest, InvalidFieldNonRepeatedWithRepeatedEncoding) {
             number: 1
             label: LABEL_OPTIONAL
             type: TYPE_INT64
+            options { features { repeated_field_encoding: EXPANDED } }
+          }
+        }
+      )pb",
+      "foo.proto: Foo.bar: NAME: Only repeated fields can specify repeated "
+      "field encoding.\n");
+}
+
+TEST_F(FeaturesTest, InvalidFieldNonPackableWithPackedRepeatedEncoding) {
+  BuildDescriptorMessagesInTestPool();
+  BuildFileWithErrors(
+      R"pb(
+        name: "foo.proto"
+        syntax: "editions"
+        edition: "2023"
+        message_type {
+          name: "Foo"
+          field {
+            name: "bar"
+            number: 1
+            label: LABEL_REPEATED
+            type: TYPE_STRING
             options { features { repeated_field_encoding: PACKED } }
           }
         }
       )pb",
-      "foo.proto: Foo.bar: NAME: Only repeated fields can specify "
-      "`repeated_field_encoding`.\n");
+      "foo.proto: Foo.bar: NAME: Only repeated primitive fields can specify "
+      "PACKED repeated field encoding.\n");
+}
+
+TEST_F(FeaturesTest, InvalidFieldNonMessageWithMessageEncoding) {
+  BuildDescriptorMessagesInTestPool();
+  BuildFileWithErrors(
+      R"pb(
+        name: "foo.proto"
+        syntax: "editions"
+        edition: "2023"
+        message_type {
+          name: "Foo"
+          field {
+            name: "bar"
+            number: 1
+            label: LABEL_OPTIONAL
+            type: TYPE_INT64
+            options { features { message_encoding: DELIMITED } }
+          }
+        }
+      )pb",
+      "foo.proto: Foo.bar: NAME: Only message fields can specify message "
+      "encoding.\n");
+}
+
+TEST_F(FeaturesTest, InvalidFieldMapWithMessageEncoding) {
+  constexpr absl::string_view kProtoFile = R"schema(
+    edition = "2023";
+
+    message Foo {
+      map<string, Foo> bar = 1 [
+        features.message_encoding = DELIMITED
+      ];
+    }
+  )schema";
+  io::ArrayInputStream input_stream(kProtoFile.data(), kProtoFile.size());
+  SimpleErrorCollector error_collector;
+  io::Tokenizer tokenizer(&input_stream, &error_collector);
+  compiler::Parser parser;
+  parser.RecordErrorsTo(&error_collector);
+  FileDescriptorProto proto;
+  ASSERT_TRUE(parser.Parse(&tokenizer, &proto))
+      << error_collector.last_error() << "\n"
+      << kProtoFile;
+  ASSERT_EQ("", error_collector.last_error());
+  proto.set_name("foo.proto");
+
+  BuildDescriptorMessagesInTestPool();
+  BuildFileWithErrors(
+      proto.DebugString(),
+      "foo.proto: Foo.bar: NAME: Only message fields can specify message "
+      "encoding.\n");
 }
 
 TEST_F(FeaturesTest, InvalidOpenEnumNonZeroFirstValue) {