Skip to content

Commit bdf6b10

Browse files
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 #16664 PiperOrigin-RevId: 630206208
1 parent 61c9187 commit bdf6b10

File tree

2 files changed

+128
-11
lines changed

2 files changed

+128
-11
lines changed

src/google/protobuf/descriptor.cc

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -8074,16 +8074,16 @@ void DescriptorBuilder::ValidateFieldFeatures(
80748074
}
80758075

80768076
// Validate fully resolved features.
8077-
if (field->has_default_value() &&
8078-
field->features().field_presence() == FeatureSet::IMPLICIT) {
8079-
AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
8080-
"Implicit presence fields can't specify defaults.");
8081-
}
8082-
if (field->enum_type() != nullptr &&
8083-
field->enum_type()->features().enum_type() != FeatureSet::OPEN &&
8084-
field->features().field_presence() == FeatureSet::IMPLICIT) {
8085-
AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
8086-
"Implicit presence enum fields must always be open.");
8077+
if (!field->is_repeated() && !field->has_presence()) {
8078+
if (field->has_default_value()) {
8079+
AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
8080+
"Implicit presence fields can't specify defaults.");
8081+
}
8082+
if (field->enum_type() != nullptr &&
8083+
field->enum_type()->features().enum_type() != FeatureSet::OPEN) {
8084+
AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
8085+
"Implicit presence enum fields must always be open.");
8086+
}
80878087
}
80888088
if (field->is_extension() &&
80898089
field->features().field_presence() == FeatureSet::LEGACY_REQUIRED) {

src/google/protobuf/descriptor_unittest.cc

Lines changed: 118 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9759,7 +9759,62 @@ TEST_F(FeaturesTest, InvalidFieldImplicitDefault) {
97599759
"defaults.\n");
97609760
}
97619761

9762-
TEST_F(FeaturesTest, InvalidFieldImplicitOpen) {
9762+
TEST_F(FeaturesTest, ValidExtensionFieldImplicitDefault) {
9763+
BuildDescriptorMessagesInTestPool();
9764+
const FileDescriptor* file = BuildFile(
9765+
R"pb(
9766+
name: "foo.proto"
9767+
syntax: "editions"
9768+
edition: EDITION_2023
9769+
options { features { field_presence: IMPLICIT } }
9770+
message_type {
9771+
name: "Foo"
9772+
extension_range { start: 1 end: 100 }
9773+
}
9774+
extension {
9775+
name: "bar"
9776+
number: 1
9777+
label: LABEL_OPTIONAL
9778+
type: TYPE_STRING
9779+
default_value: "Hello world"
9780+
extendee: "Foo"
9781+
}
9782+
)pb");
9783+
ASSERT_THAT(file, NotNull());
9784+
9785+
EXPECT_TRUE(file->extension(0)->has_presence());
9786+
EXPECT_EQ(file->extension(0)->default_value_string(), "Hello world");
9787+
}
9788+
9789+
TEST_F(FeaturesTest, ValidOneofFieldImplicitDefault) {
9790+
BuildDescriptorMessagesInTestPool();
9791+
const FileDescriptor* file = BuildFile(
9792+
R"pb(
9793+
name: "foo.proto"
9794+
syntax: "editions"
9795+
edition: EDITION_2023
9796+
options { features { field_presence: IMPLICIT } }
9797+
message_type {
9798+
name: "Foo"
9799+
field {
9800+
name: "bar"
9801+
number: 1
9802+
label: LABEL_OPTIONAL
9803+
type: TYPE_STRING
9804+
default_value: "Hello world"
9805+
oneof_index: 0
9806+
}
9807+
oneof_decl { name: "_foo" }
9808+
}
9809+
)pb");
9810+
ASSERT_THAT(file, NotNull());
9811+
9812+
EXPECT_TRUE(file->message_type(0)->field(0)->has_presence());
9813+
EXPECT_EQ(file->message_type(0)->field(0)->default_value_string(),
9814+
"Hello world");
9815+
}
9816+
9817+
TEST_F(FeaturesTest, InvalidFieldImplicitClosed) {
97639818
BuildDescriptorMessagesInTestPool();
97649819
BuildFileWithErrors(
97659820
R"pb(
@@ -9787,6 +9842,68 @@ TEST_F(FeaturesTest, InvalidFieldImplicitOpen) {
97879842
"be open.\n");
97889843
}
97899844

9845+
TEST_F(FeaturesTest, ValidRepeatedFieldImplicitClosed) {
9846+
BuildDescriptorMessagesInTestPool();
9847+
const FileDescriptor* file = BuildFile(
9848+
R"pb(
9849+
name: "foo.proto"
9850+
syntax: "editions"
9851+
edition: EDITION_2023
9852+
options { features { field_presence: IMPLICIT } }
9853+
message_type {
9854+
name: "Foo"
9855+
field {
9856+
name: "bar"
9857+
number: 1
9858+
label: LABEL_REPEATED
9859+
type: TYPE_ENUM
9860+
type_name: "Enum"
9861+
}
9862+
}
9863+
enum_type {
9864+
name: "Enum"
9865+
value { name: "BAR" number: 0 }
9866+
options { features { enum_type: CLOSED } }
9867+
}
9868+
)pb");
9869+
ASSERT_THAT(file, NotNull());
9870+
9871+
EXPECT_FALSE(file->message_type(0)->field(0)->has_presence());
9872+
EXPECT_TRUE(file->enum_type(0)->is_closed());
9873+
}
9874+
9875+
TEST_F(FeaturesTest, ValidOneofFieldImplicitClosed) {
9876+
BuildDescriptorMessagesInTestPool();
9877+
const FileDescriptor* file = BuildFile(
9878+
R"pb(
9879+
name: "foo.proto"
9880+
syntax: "editions"
9881+
edition: EDITION_2023
9882+
options { features { field_presence: IMPLICIT } }
9883+
message_type {
9884+
name: "Foo"
9885+
field {
9886+
name: "bar"
9887+
number: 1
9888+
label: LABEL_OPTIONAL
9889+
type: TYPE_ENUM
9890+
type_name: "Enum"
9891+
oneof_index: 0
9892+
}
9893+
oneof_decl { name: "_foo" }
9894+
}
9895+
enum_type {
9896+
name: "Enum"
9897+
value { name: "BAR" number: 0 }
9898+
options { features { enum_type: CLOSED } }
9899+
}
9900+
)pb");
9901+
ASSERT_THAT(file, NotNull());
9902+
9903+
EXPECT_TRUE(file->message_type(0)->field(0)->has_presence());
9904+
EXPECT_TRUE(file->enum_type(0)->is_closed());
9905+
}
9906+
97909907
TEST_F(FeaturesTest, InvalidFieldRequiredExtension) {
97919908
BuildDescriptorMessagesInTestPool();
97929909
BuildFileWithErrors(

0 commit comments

Comments
 (0)