Skip to content

Commit d6c2833

Browse files
mkruskal-googlecopybara-github
authored andcommitted
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 0dbd99a commit d6c2833

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
@@ -8084,16 +8084,16 @@ void DescriptorBuilder::ValidateFieldFeatures(
80848084
}
80858085

80868086
// Validate fully resolved features.
8087-
if (field->has_default_value() &&
8088-
field->features().field_presence() == FeatureSet::IMPLICIT) {
8089-
AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
8090-
"Implicit presence fields can't specify defaults.");
8091-
}
8092-
if (field->enum_type() != nullptr &&
8093-
field->enum_type()->features().enum_type() != FeatureSet::OPEN &&
8094-
field->features().field_presence() == FeatureSet::IMPLICIT) {
8095-
AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
8096-
"Implicit presence enum fields must always be open.");
8087+
if (!field->is_repeated() && !field->has_presence()) {
8088+
if (field->has_default_value()) {
8089+
AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
8090+
"Implicit presence fields can't specify defaults.");
8091+
}
8092+
if (field->enum_type() != nullptr &&
8093+
field->enum_type()->features().enum_type() != FeatureSet::OPEN) {
8094+
AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME,
8095+
"Implicit presence enum fields must always be open.");
8096+
}
80978097
}
80988098
if (field->is_extension() &&
80998099
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
@@ -9830,7 +9830,62 @@ TEST_F(FeaturesTest, InvalidFieldImplicitDefault) {
98309830
"defaults.\n");
98319831
}
98329832

9833-
TEST_F(FeaturesTest, InvalidFieldImplicitOpen) {
9833+
TEST_F(FeaturesTest, ValidExtensionFieldImplicitDefault) {
9834+
BuildDescriptorMessagesInTestPool();
9835+
const FileDescriptor* file = BuildFile(
9836+
R"pb(
9837+
name: "foo.proto"
9838+
syntax: "editions"
9839+
edition: EDITION_2023
9840+
options { features { field_presence: IMPLICIT } }
9841+
message_type {
9842+
name: "Foo"
9843+
extension_range { start: 1 end: 100 }
9844+
}
9845+
extension {
9846+
name: "bar"
9847+
number: 1
9848+
label: LABEL_OPTIONAL
9849+
type: TYPE_STRING
9850+
default_value: "Hello world"
9851+
extendee: "Foo"
9852+
}
9853+
)pb");
9854+
ASSERT_THAT(file, NotNull());
9855+
9856+
EXPECT_TRUE(file->extension(0)->has_presence());
9857+
EXPECT_EQ(file->extension(0)->default_value_string(), "Hello world");
9858+
}
9859+
9860+
TEST_F(FeaturesTest, ValidOneofFieldImplicitDefault) {
9861+
BuildDescriptorMessagesInTestPool();
9862+
const FileDescriptor* file = BuildFile(
9863+
R"pb(
9864+
name: "foo.proto"
9865+
syntax: "editions"
9866+
edition: EDITION_2023
9867+
options { features { field_presence: IMPLICIT } }
9868+
message_type {
9869+
name: "Foo"
9870+
field {
9871+
name: "bar"
9872+
number: 1
9873+
label: LABEL_OPTIONAL
9874+
type: TYPE_STRING
9875+
default_value: "Hello world"
9876+
oneof_index: 0
9877+
}
9878+
oneof_decl { name: "_foo" }
9879+
}
9880+
)pb");
9881+
ASSERT_THAT(file, NotNull());
9882+
9883+
EXPECT_TRUE(file->message_type(0)->field(0)->has_presence());
9884+
EXPECT_EQ(file->message_type(0)->field(0)->default_value_string(),
9885+
"Hello world");
9886+
}
9887+
9888+
TEST_F(FeaturesTest, InvalidFieldImplicitClosed) {
98349889
BuildDescriptorMessagesInTestPool();
98359890
BuildFileWithErrors(
98369891
R"pb(
@@ -9858,6 +9913,68 @@ TEST_F(FeaturesTest, InvalidFieldImplicitOpen) {
98589913
"be open.\n");
98599914
}
98609915

9916+
TEST_F(FeaturesTest, ValidRepeatedFieldImplicitClosed) {
9917+
BuildDescriptorMessagesInTestPool();
9918+
const FileDescriptor* file = BuildFile(
9919+
R"pb(
9920+
name: "foo.proto"
9921+
syntax: "editions"
9922+
edition: EDITION_2023
9923+
options { features { field_presence: IMPLICIT } }
9924+
message_type {
9925+
name: "Foo"
9926+
field {
9927+
name: "bar"
9928+
number: 1
9929+
label: LABEL_REPEATED
9930+
type: TYPE_ENUM
9931+
type_name: "Enum"
9932+
}
9933+
}
9934+
enum_type {
9935+
name: "Enum"
9936+
value { name: "BAR" number: 0 }
9937+
options { features { enum_type: CLOSED } }
9938+
}
9939+
)pb");
9940+
ASSERT_THAT(file, NotNull());
9941+
9942+
EXPECT_FALSE(file->message_type(0)->field(0)->has_presence());
9943+
EXPECT_TRUE(file->enum_type(0)->is_closed());
9944+
}
9945+
9946+
TEST_F(FeaturesTest, ValidOneofFieldImplicitClosed) {
9947+
BuildDescriptorMessagesInTestPool();
9948+
const FileDescriptor* file = BuildFile(
9949+
R"pb(
9950+
name: "foo.proto"
9951+
syntax: "editions"
9952+
edition: EDITION_2023
9953+
options { features { field_presence: IMPLICIT } }
9954+
message_type {
9955+
name: "Foo"
9956+
field {
9957+
name: "bar"
9958+
number: 1
9959+
label: LABEL_OPTIONAL
9960+
type: TYPE_ENUM
9961+
type_name: "Enum"
9962+
oneof_index: 0
9963+
}
9964+
oneof_decl { name: "_foo" }
9965+
}
9966+
enum_type {
9967+
name: "Enum"
9968+
value { name: "BAR" number: 0 }
9969+
options { features { enum_type: CLOSED } }
9970+
}
9971+
)pb");
9972+
ASSERT_THAT(file, NotNull());
9973+
9974+
EXPECT_TRUE(file->message_type(0)->field(0)->has_presence());
9975+
EXPECT_TRUE(file->enum_type(0)->is_closed());
9976+
}
9977+
98619978
TEST_F(FeaturesTest, InvalidFieldRequiredExtension) {
98629979
BuildDescriptorMessagesInTestPool();
98639980
BuildFileWithErrors(

0 commit comments

Comments
 (0)