Skip to content

Commit ae0d964

Browse files
Rachel Goldfingercopybara-github
Rachel Goldfinger
authored and
copybara-github
committed
Implement the Edition 2026 naming style enforcement feature. This will help prevent field name collisions.
PiperOrigin-RevId: 896038749
1 parent 0799a4e commit ae0d964

3 files changed

Lines changed: 320 additions & 2 deletions

File tree

‎src/google/protobuf/compiler/command_line_interface_unittest.cc‎

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1830,6 +1830,38 @@ TEST_F(CommandLineInterfaceTest, NamingStyleEnforced) {
18301830
ExpectErrorSubstring("Package name badPackage should be lower_snake_case");
18311831
}
18321832

1833+
TEST_F(CommandLineInterfaceTest, NamingStyleStartsWithHasCollisionExists) {
1834+
CreateTempFile("foo.proto",
1835+
R"schema(
1836+
edition = "UNSTABLE";
1837+
message Foo {
1838+
string has_bar = 1;
1839+
string bar = 2;
1840+
}
1841+
)schema");
1842+
Run("protocol_compiler --proto_path=$tmpdir --test_out=$tmpdir foo.proto");
1843+
ExpectErrorSubstring(
1844+
"Field name has_bar should not begin with has_ if a field named bar "
1845+
"exists. "
1846+
"This can cause collisions in generated code. "
1847+
"(features.enforce_naming_style = STYLE2024 can be used to opt out of "
1848+
"this check)");
1849+
}
1850+
1851+
TEST_F(CommandLineInterfaceTest, NamingStyleStartsWithHasNoCollision) {
1852+
CreateTempFile("foo.proto",
1853+
R"schema(
1854+
edition = "UNSTABLE";
1855+
message Foo {
1856+
string has_bar = 1;
1857+
string baz = 2;
1858+
}
1859+
)schema");
1860+
Run("protocol_compiler --proto_path=$tmpdir --experimental_editions "
1861+
"--test_out=$tmpdir foo.proto");
1862+
ExpectNoErrors();
1863+
}
1864+
18331865
TEST_F(CommandLineInterfaceTest, Plugin_InvalidFeatureExtensionError) {
18341866
CreateTempFile("foo.proto", R"schema(
18351867
edition = "2023";

‎src/google/protobuf/descriptor.cc‎

Lines changed: 89 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5060,6 +5060,17 @@ class DescriptorBuilder {
50605060
void ValidateNamingStyle(const DescriptorT* file,
50615061
const DescriptorProtoT& proto);
50625062

5063+
template <typename DescriptorT>
5064+
bool IsStyleOrGreater(const DescriptorT* descriptor,
5065+
FeatureSet::EnforceNamingStyle style) {
5066+
return internal::InternalFeatureHelper::GetFeatures(*descriptor)
5067+
.enforce_naming_style() >= style &&
5068+
// Required because STYLE_LEGACY comes after STYLE2024 in enum
5069+
// definition.
5070+
internal::InternalFeatureHelper::GetFeatures(*descriptor)
5071+
.enforce_naming_style() != FeatureSet::STYLE_LEGACY;
5072+
}
5073+
50635074
// Nothing to validate for extension ranges. This overload only exists
50645075
// so that VisitDescriptors can be exhaustive.
50655076
void ValidateNamingStyle(const Descriptor::ExtensionRange* ext_range,
@@ -6717,8 +6728,7 @@ FileDescriptor* DescriptorBuilder::BuildFileImpl(
67176728
if (!had_errors_ && pool_->enforce_naming_style_) {
67186729
internal::VisitDescriptors(
67196730
*result, proto, [&](const auto& descriptor, const auto& desc_proto) {
6720-
if (internal::InternalFeatureHelper::GetFeatures(descriptor)
6721-
.enforce_naming_style() == FeatureSet::STYLE2024) {
6731+
if (IsStyleOrGreater(&descriptor, FeatureSet::STYLE2024)) {
67226732
ValidateNamingStyle(&descriptor, desc_proto);
67236733
}
67246734
});
@@ -9298,12 +9308,73 @@ bool IsValidUpperSnakeCaseName(absl::string_view name, std::string* error) {
92989308
return true;
92999309
}
93009310

9311+
template <typename DescriptorType>
9312+
bool IsValidFieldNonCollisionName(const DescriptorType* descriptor,
9313+
std::string* error) {
9314+
ABSL_CHECK(descriptor != nullptr);
9315+
absl::string_view name = descriptor->name();
9316+
const Descriptor* message = descriptor->containing_type();
9317+
9318+
static const auto& kRestrictedFieldPrefixes =
9319+
*new absl::flat_hash_set<absl::string_view>({
9320+
"has_",
9321+
"get_",
9322+
"set_",
9323+
"clear_",
9324+
});
9325+
static const auto& kRestrictedFieldSuffixes =
9326+
*new absl::flat_hash_set<absl::string_view>({"_value"});
9327+
9328+
if (message != nullptr) {
9329+
for (absl::string_view prefix : kRestrictedFieldPrefixes) {
9330+
if (absl::StartsWith(name, prefix)) {
9331+
absl::string_view without_prefix = name;
9332+
without_prefix.remove_prefix(prefix.size());
9333+
if ((message->FindFieldByName(without_prefix) != nullptr ||
9334+
message->FindOneofByName(without_prefix) != nullptr)) {
9335+
*error = absl::StrCat("should not begin with ", prefix,
9336+
" if a field named ", without_prefix,
9337+
" exists. This can cause collisions in "
9338+
"generated code.");
9339+
return false;
9340+
}
9341+
}
9342+
}
9343+
for (absl::string_view suffix : kRestrictedFieldSuffixes) {
9344+
if (absl::EndsWith(name, suffix)) {
9345+
absl::string_view without_suffix = name;
9346+
without_suffix.remove_suffix(suffix.size());
9347+
if ((message->FindFieldByName(without_suffix) != nullptr) ||
9348+
(message->FindOneofByName(without_suffix) != nullptr)) {
9349+
*error = absl::StrCat("should not end with ", suffix,
9350+
" if a field named ", without_suffix,
9351+
" exists. This can cause collisions in "
9352+
"generated code.");
9353+
return false;
9354+
}
9355+
}
9356+
}
9357+
}
9358+
9359+
if (name == "descriptor") {
9360+
*error =
9361+
"should not be named descriptor. This can cause collisions in "
9362+
"generated code.";
9363+
return false;
9364+
}
9365+
return true;
9366+
}
9367+
93019368
constexpr absl::string_view kNamingStyleOptOutMessage =
93029369
" (features.enforce_naming_style = STYLE_LEGACY can be used to opt out of "
93039370
"this check)";
93049371

93059372
} // namespace
93069373

9374+
constexpr absl::string_view kNamingStyleCollisionsOptOutMessage =
9375+
" (features.enforce_naming_style = STYLE2024 can be used to opt out of "
9376+
"this check)";
9377+
93079378
template <>
93089379
void DescriptorBuilder::ValidateNamingStyle(const FileDescriptor* file,
93099380
const FileDescriptorProto& proto) {
@@ -9342,6 +9413,14 @@ void DescriptorBuilder::ValidateNamingStyle(const OneofDescriptor* oneof,
93429413
kNamingStyleOptOutMessage);
93439414
});
93449415
}
9416+
if (IsStyleOrGreater(oneof, FeatureSet::STYLE2026)) {
9417+
if (!IsValidFieldNonCollisionName(oneof, &error)) {
9418+
AddError(oneof->name(), proto, DescriptorPool::ErrorCollector::NAME, [&] {
9419+
return absl::StrCat("Oneof name ", oneof->name(), " ", error,
9420+
kNamingStyleCollisionsOptOutMessage);
9421+
});
9422+
}
9423+
}
93459424
}
93469425

93479426
template <>
@@ -9354,6 +9433,14 @@ void DescriptorBuilder::ValidateNamingStyle(const FieldDescriptor* field,
93549433
kNamingStyleOptOutMessage);
93559434
});
93569435
}
9436+
if (IsStyleOrGreater(field, FeatureSet::STYLE2026)) {
9437+
if (!IsValidFieldNonCollisionName(field, &error)) {
9438+
AddError(field->name(), proto, DescriptorPool::ErrorCollector::NAME, [&] {
9439+
return absl::StrCat("Field name ", field->name(), " ", error,
9440+
kNamingStyleCollisionsOptOutMessage);
9441+
});
9442+
}
9443+
}
93579444
}
93589445

93599446
template <>

‎src/google/protobuf/descriptor_unittest.cc‎

Lines changed: 199 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10479,6 +10479,205 @@ TEST_F(FeaturesTest, NoNamingStyleViolationsWithPoolOptInIfMessagesAreGood) {
1047910479
NotNull());
1048010480
}
1048110481

10482+
TEST_F(FeaturesTest, NoNamingStyleViolationsWithCollisionsInEdition2024) {
10483+
BuildDescriptorMessagesInTestPool();
10484+
pool_.EnforceNamingStyle(true);
10485+
10486+
// Check that naming collisions are allowed in edition 2024.
10487+
ASSERT_THAT(ParseAndBuildFile("naming.proto", R"schema(
10488+
edition = "2024";
10489+
package naming;
10490+
message Foo {
10491+
string has_bar = 1;
10492+
string bar = 2;
10493+
}
10494+
)schema"),
10495+
NotNull());
10496+
}
10497+
10498+
TEST_F(FeaturesTest, NoNamingStyleViolationsWithCollisionsInStyle2024) {
10499+
BuildDescriptorMessagesInTestPool();
10500+
pool_.EnforceNamingStyle(true);
10501+
10502+
// Check that naming collisions are allowed in STYLE2024.
10503+
ASSERT_THAT(ParseAndBuildFile("naming.proto", R"schema(
10504+
edition = "UNSTABLE";
10505+
option features.enforce_naming_style = STYLE2024;
10506+
package naming;
10507+
message Foo {
10508+
string has_bar = 1;
10509+
string bar = 2;
10510+
}
10511+
)schema"),
10512+
NotNull());
10513+
}
10514+
10515+
TEST_F(FeaturesTest, NoNamingStyleViolationsWithCollisionsInStyle2026) {
10516+
BuildDescriptorMessagesInTestPool();
10517+
pool_.EnforceNamingStyle(true);
10518+
10519+
// These are allowed because they don't collide with any existing field.
10520+
ASSERT_THAT(ParseAndBuildFile("naming.proto", R"schema(
10521+
edition = "UNSTABLE";
10522+
option features.enforce_naming_style = STYLE2026;
10523+
package naming;
10524+
message Foo {
10525+
string has_bar = 1;
10526+
string baz = 2;
10527+
string bar_value = 3;
10528+
}
10529+
)schema"),
10530+
NotNull());
10531+
}
10532+
10533+
TEST_F(FeaturesTest, NoNamingStyleViolationsWithCollisionsOneOfInStyle2026) {
10534+
BuildDescriptorMessagesInTestPool();
10535+
pool_.EnforceNamingStyle(true);
10536+
10537+
// This is allowed because the oneof doesn't collide with any existing field.
10538+
ASSERT_THAT(ParseAndBuildFile("naming.proto", R"schema(
10539+
edition = "UNSTABLE";
10540+
option features.enforce_naming_style = STYLE2026;
10541+
package naming;
10542+
message Foo {
10543+
oneof has_bar {
10544+
string test_field = 1;
10545+
}
10546+
}
10547+
)schema"),
10548+
NotNull());
10549+
}
10550+
10551+
struct CollisionNameParam {
10552+
std::string field_name;
10553+
std::string error_message_part;
10554+
};
10555+
10556+
class CollisionNameTest
10557+
: public FeaturesTest,
10558+
public testing::WithParamInterface<CollisionNameParam> {};
10559+
10560+
INSTANTIATE_TEST_SUITE_P(
10561+
CollisionNameTests, CollisionNameTest,
10562+
testing::Values(
10563+
CollisionNameParam{"has_test_field", "should not begin with has_"},
10564+
CollisionNameParam{"get_test_field", "should not begin with get_"},
10565+
CollisionNameParam{"set_test_field", "should not begin with set_"},
10566+
CollisionNameParam{"clear_test_field", "should not begin with clear_"},
10567+
CollisionNameParam{"test_field_value", "should not end with _value"}),
10568+
[](const testing::TestParamInfo<CollisionNameParam>& info) {
10569+
return info.param.field_name;
10570+
});
10571+
10572+
TEST_P(CollisionNameTest, NamingStyleViolationsWithCollisionsInStyle2026) {
10573+
BuildDescriptorMessagesInTestPool();
10574+
pool_.EnforceNamingStyle(true);
10575+
const CollisionNameParam& param = GetParam();
10576+
std::string schema = absl::Substitute(R"schema(
10577+
edition = "UNSTABLE";
10578+
message Foo {
10579+
string $0 = 1;
10580+
string test_field = 2;
10581+
}
10582+
)schema",
10583+
param.field_name);
10584+
ParseAndBuildFileWithErrorSubstr(
10585+
"foo.proto", schema,
10586+
absl::StrCat("Field name ", param.field_name, " ",
10587+
param.error_message_part,
10588+
" if a field named test_field exists. This can cause "
10589+
"collisions in generated code."));
10590+
}
10591+
10592+
TEST_P(CollisionNameTest, NamingStyleViolationsInvalidFieldNameInStyle2026) {
10593+
BuildDescriptorMessagesInTestPool();
10594+
pool_.EnforceNamingStyle(true);
10595+
std::string schema = absl::StrCat(R"schema(
10596+
edition = "UNSTABLE";
10597+
message Foo {
10598+
string descriptor = 1;
10599+
}
10600+
)schema");
10601+
ParseAndBuildFileWithErrorSubstr(
10602+
"foo.proto", schema,
10603+
"Field name descriptor should not be named descriptor. This can cause "
10604+
"collisions in generated code.");
10605+
}
10606+
10607+
TEST_P(CollisionNameTest,
10608+
NamingStyleViolationsOneOfFieldNameCollisionInStyle2026) {
10609+
BuildDescriptorMessagesInTestPool();
10610+
pool_.EnforceNamingStyle(true);
10611+
std::string schema = absl::StrCat(R"schema(
10612+
edition = "UNSTABLE";
10613+
message Foo {
10614+
oneof bar {
10615+
string has_test_field = 1;
10616+
string test_field = 2;
10617+
}
10618+
}
10619+
)schema");
10620+
ParseAndBuildFileWithErrorSubstr("foo.proto", schema,
10621+
"Field name has_test_field should not begin "
10622+
"with has_ if a field named test_field "
10623+
"exists. This can cause collisions in "
10624+
"generated code.");
10625+
}
10626+
10627+
TEST_P(CollisionNameTest,
10628+
NamingStyleViolationsOneOfNameFieldNameCollisionInStyle2026) {
10629+
BuildDescriptorMessagesInTestPool();
10630+
pool_.EnforceNamingStyle(true);
10631+
std::string schema = absl::StrCat(R"schema(
10632+
edition = "UNSTABLE";
10633+
message Foo {
10634+
oneof has_test_field {
10635+
string test_field = 1;
10636+
}
10637+
}
10638+
)schema");
10639+
ParseAndBuildFileWithErrorSubstr("foo.proto", schema,
10640+
"Oneof name has_test_field should not begin "
10641+
"with has_ if a field named test_field "
10642+
"exists. This can cause collisions in "
10643+
"generated code.");
10644+
}
10645+
10646+
TEST_P(CollisionNameTest,
10647+
NamingStyleViolationsFieldNameOneOfNameCollisionInStyle2026) {
10648+
BuildDescriptorMessagesInTestPool();
10649+
pool_.EnforceNamingStyle(true);
10650+
std::string schema = absl::StrCat(R"schema(
10651+
edition = "UNSTABLE";
10652+
message Foo {
10653+
oneof test_field {
10654+
string has_test_field = 1;
10655+
}
10656+
}
10657+
)schema");
10658+
ParseAndBuildFileWithErrorSubstr(
10659+
"foo.proto", schema,
10660+
"Field name has_test_field should not begin with has_ if a field named "
10661+
"test_field exists. This can cause collisions in generated code.");
10662+
}
10663+
10664+
TEST_P(CollisionNameTest, NamingStyleViolationsInvalidOneOfNameInStyle2026) {
10665+
BuildDescriptorMessagesInTestPool();
10666+
pool_.EnforceNamingStyle(true);
10667+
std::string schema = absl::StrCat(R"schema(
10668+
edition = "UNSTABLE";
10669+
message Foo {
10670+
oneof descriptor {
10671+
string test_field = 1;
10672+
}
10673+
}
10674+
)schema");
10675+
ParseAndBuildFileWithErrorSubstr(
10676+
"foo.proto", schema,
10677+
"Oneof name descriptor should not be named descriptor. This can cause "
10678+
"collisions in generated code.");
10679+
}
10680+
1048210681
TEST_F(FeaturesTest, VisibilityFeatureSetStrict) {
1048310682
pool_.EnforceSymbolVisibility(true);
1048410683
BuildDescriptorMessagesInTestPool();

0 commit comments

Comments
 (0)