Fix validation checks of implicit presence.

Instead of checking the resolved features, we should really be checking the has_presence helper. Repeated fields, oneofs, and extensions can trigger these conditions when they inherit IMPLICIT, even though it's ignored. Closes protocolbuffers#16664 PiperOrigin-RevId: 630206208
mkruskal-google · May 2, 2024 · d6c2833 · d6c2833
1 parent 0dbd99a
commit d6c2833
Show file tree

Hide file tree

Showing 2 changed files with 128 additions and 11 deletions.
diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc
@@ -8084,16 +8084,16 @@ void DescriptorBuilder::ValidateFieldFeatures(
   }
 
   // Validate fully resolved features.
-  if (field->has_default_value() &&
-      field->features().field_presence() == FeatureSet::IMPLICIT) {
-    AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
-             "Implicit presence fields can't specify defaults.");
-  }
-  if (field->enum_type() != nullptr &&
-      field->enum_type()->features().enum_type() != FeatureSet::OPEN &&
-      field->features().field_presence() == FeatureSet::IMPLICIT) {
-    AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
-             "Implicit presence enum fields must always be open.");
+  if (!field->is_repeated() && !field->has_presence()) {
+    if (field->has_default_value()) {
+      AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
+               "Implicit presence fields can't specify defaults.");
+    }
+    if (field->enum_type() != nullptr &&
+        field->enum_type()->features().enum_type() != FeatureSet::OPEN) {
+      AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
+               "Implicit presence enum fields must always be open.");
+    }
   }
   if (field->is_extension() &&
       field->features().field_presence() == FeatureSet::LEGACY_REQUIRED) {

diff --git a/src/google/protobuf/descriptor_unittest.cc b/src/google/protobuf/descriptor_unittest.cc
@@ -9830,7 +9830,62 @@ TEST_F(FeaturesTest, InvalidFieldImplicitDefault) {
       "defaults.\n");
 }
 
-TEST_F(FeaturesTest, InvalidFieldImplicitOpen) {
+TEST_F(FeaturesTest, ValidExtensionFieldImplicitDefault) {
+  BuildDescriptorMessagesInTestPool();
+  const FileDescriptor* file = BuildFile(
+      R"pb(
+        name: "foo.proto"
+        syntax: "editions"
+        edition: EDITION_2023
+        options { features { field_presence: IMPLICIT } }
+        message_type {
+          name: "Foo"
+          extension_range { start: 1 end: 100 }
+        }
+        extension {
+          name: "bar"
+          number: 1
+          label: LABEL_OPTIONAL
+          type: TYPE_STRING
+          default_value: "Hello world"
+          extendee: "Foo"
+        }
+      )pb");
+  ASSERT_THAT(file, NotNull());
+
+  EXPECT_TRUE(file->extension(0)->has_presence());
+  EXPECT_EQ(file->extension(0)->default_value_string(), "Hello world");
+}
+
+TEST_F(FeaturesTest, ValidOneofFieldImplicitDefault) {
+  BuildDescriptorMessagesInTestPool();
+  const FileDescriptor* file = BuildFile(
+      R"pb(
+        name: "foo.proto"
+        syntax: "editions"
+        edition: EDITION_2023
+        options { features { field_presence: IMPLICIT } }
+        message_type {
+          name: "Foo"
+          field {
+            name: "bar"
+            number: 1
+            label: LABEL_OPTIONAL
+            type: TYPE_STRING
+            default_value: "Hello world"
+            oneof_index: 0
+          }
+          oneof_decl { name: "_foo" }
+        }
+      )pb");
+  ASSERT_THAT(file, NotNull());
+
+  EXPECT_TRUE(file->message_type(0)->field(0)->has_presence());
+  EXPECT_EQ(file->message_type(0)->field(0)->default_value_string(),
+            "Hello world");
+}
+
+TEST_F(FeaturesTest, InvalidFieldImplicitClosed) {
   BuildDescriptorMessagesInTestPool();
   BuildFileWithErrors(
       R"pb(
@@ -9858,6 +9913,68 @@ TEST_F(FeaturesTest, InvalidFieldImplicitOpen) {
       "be open.\n");
 }
 
+TEST_F(FeaturesTest, ValidRepeatedFieldImplicitClosed) {
+  BuildDescriptorMessagesInTestPool();
+  const FileDescriptor* file = BuildFile(
+      R"pb(
+        name: "foo.proto"
+        syntax: "editions"
+        edition: EDITION_2023
+        options { features { field_presence: IMPLICIT } }
+        message_type {
+          name: "Foo"
+          field {
+            name: "bar"
+            number: 1
+            label: LABEL_REPEATED
+            type: TYPE_ENUM
+            type_name: "Enum"
+          }
+        }
+        enum_type {
+          name: "Enum"
+          value { name: "BAR" number: 0 }
+          options { features { enum_type: CLOSED } }
+        }
+      )pb");
+  ASSERT_THAT(file, NotNull());
+
+  EXPECT_FALSE(file->message_type(0)->field(0)->has_presence());
+  EXPECT_TRUE(file->enum_type(0)->is_closed());
+}
+
+TEST_F(FeaturesTest, ValidOneofFieldImplicitClosed) {
+  BuildDescriptorMessagesInTestPool();
+  const FileDescriptor* file = BuildFile(
+      R"pb(
+        name: "foo.proto"
+        syntax: "editions"
+        edition: EDITION_2023
+        options { features { field_presence: IMPLICIT } }
+        message_type {
+          name: "Foo"
+          field {
+            name: "bar"
+            number: 1
+            label: LABEL_OPTIONAL
+            type: TYPE_ENUM
+            type_name: "Enum"
+            oneof_index: 0
+          }
+          oneof_decl { name: "_foo" }
+        }
+        enum_type {
+          name: "Enum"
+          value { name: "BAR" number: 0 }
+          options { features { enum_type: CLOSED } }
+        }
+      )pb");
+  ASSERT_THAT(file, NotNull());
+
+  EXPECT_TRUE(file->message_type(0)->field(0)->has_presence());
+  EXPECT_TRUE(file->enum_type(0)->is_closed());
+}
+
 TEST_F(FeaturesTest, InvalidFieldRequiredExtension) {
   BuildDescriptorMessagesInTestPool();
   BuildFileWithErrors(