Skip to content

Commit ae308fc

Browse files
mkruskal-googlecopybara-github
mkruskal-google
authored and
copybara-github
committed
Disable symbol visibility enforcement by default in C++ runtime
This is breaking codegen in certain situations (e.g. for 2023 upgrades) because we strip the default_symbol_visibility feature, which is marked as source-retention. Plugins may fail to rebuild the descriptors after the feature value has changed. PiperOrigin-RevId: 804592838
1 parent 470522c commit ae308fc

5 files changed

Lines changed: 145 additions & 13 deletions

File tree

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1271,6 +1271,7 @@ int CommandLineInterface::Run(int argc, const char* const argv[]) {
12711271

12721272
descriptor_pool->EnforceWeakDependencies(true);
12731273
descriptor_pool->EnforceOptionDependencies(true);
1274+
descriptor_pool->EnforceSymbolVisibility(true);
12741275
descriptor_pool->EnforceNamingStyle(true);
12751276

12761277
if (!SetupFeatureResolution(*descriptor_pool)) {

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

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4748,6 +4748,74 @@ TEST_F(CommandLineInterfaceTest, VisibilityFromSame) {
47484748
ExpectNoErrors();
47494749
}
47504750

4751+
TEST_F(CommandLineInterfaceTest, NonDefaultSymbolVisibilityBuiltInCodegen) {
4752+
CreateTempFile("vis.proto", R"schema(
4753+
edition = "2024";
4754+
package vis.test;
4755+
option features.default_symbol_visibility = EXPORT_ALL;
4756+
4757+
message TopLevelMessage {
4758+
message NestedMessage {
4759+
}
4760+
enum NestedEnum {
4761+
NESTED_ENUM_UNKNOWN = 0;
4762+
NESTED_ENUM_BAR = 1;
4763+
}
4764+
}
4765+
)schema");
4766+
4767+
CreateTempFile("good_importer.proto", R"schema(
4768+
edition = "2024";
4769+
import "vis.proto";
4770+
option features.default_symbol_visibility = EXPORT_ALL;
4771+
4772+
message GoodImport {
4773+
vis.test.TopLevelMessage foo = 1;
4774+
vis.test.TopLevelMessage.NestedMessage bar = 2;
4775+
vis.test.TopLevelMessage.NestedEnum baz = 3;
4776+
}
4777+
)schema");
4778+
Run("protocol_compiler --test_out=$tmpdir "
4779+
"--experimental_editions " // remove when edition 2024 is valid
4780+
"--proto_path=$tmpdir good_importer.proto vis.proto");
4781+
4782+
ExpectNoErrors();
4783+
}
4784+
4785+
TEST_F(CommandLineInterfaceTest, NonDefaultSymbolVisibilityPluginCodegen) {
4786+
CreateTempFile("vis.proto", R"schema(
4787+
edition = "2024";
4788+
package vis.test;
4789+
option features.default_symbol_visibility = EXPORT_ALL;
4790+
4791+
message TopLevelMessage {
4792+
message NestedMessage {
4793+
}
4794+
enum NestedEnum {
4795+
NESTED_ENUM_UNKNOWN = 0;
4796+
NESTED_ENUM_BAR = 1;
4797+
}
4798+
}
4799+
)schema");
4800+
4801+
CreateTempFile("good_importer.proto", R"schema(
4802+
edition = "2024";
4803+
import "vis.proto";
4804+
option features.default_symbol_visibility = EXPORT_ALL;
4805+
4806+
message GoodImport {
4807+
vis.test.TopLevelMessage foo = 1;
4808+
vis.test.TopLevelMessage.NestedMessage bar = 2;
4809+
vis.test.TopLevelMessage.NestedEnum baz = 3;
4810+
}
4811+
)schema");
4812+
Run("protocol_compiler --plug_out=$tmpdir "
4813+
"--experimental_editions " // remove when edition 2024 is valid
4814+
"--proto_path=$tmpdir good_importer.proto vis.proto");
4815+
4816+
ExpectNoErrors();
4817+
}
4818+
47514819
TEST_F(CommandLineInterfaceTest, ExplicitVisibilityFromOther) {
47524820
CreateTempFile("vis.proto", R"schema(
47534821
edition = "2024";

‎src/google/protobuf/descriptor.cc‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6731,7 +6731,7 @@ FileDescriptor* DescriptorBuilder::BuildFileImpl(
67316731
}
67326732
});
67336733
}
6734-
if (!had_errors_) {
6734+
if (!had_errors_ && pool_->enforce_symbol_visibility_) {
67356735
// Check Symbol Visibility Rules.
67366736
CheckVisibilityRules(result, proto);
67376737
}
@@ -7929,7 +7929,8 @@ void DescriptorBuilder::CrossLinkField(FieldDescriptor* field,
79297929
"\" is not a message type.");
79307930
});
79317931
return;
7932-
} else if (!extendee.IsVisibleFrom(file_)) {
7932+
} else if (!extendee.IsVisibleFrom(file_) &&
7933+
pool_->enforce_symbol_visibility_) {
79337934
AddError(field->full_name(), proto,
79347935
DescriptorPool::ErrorCollector::EXTENDEE, [&] {
79357936
return extendee.GetVisibilityError(file_, "target of extend");
@@ -8041,7 +8042,7 @@ void DescriptorBuilder::CrossLinkField(FieldDescriptor* field,
80418042
field->is_map_ = sub_message->options().map_entry();
80428043
}
80438044

8044-
if (!type.IsVisibleFrom(file_)) {
8045+
if (!type.IsVisibleFrom(file_) && pool_->enforce_symbol_visibility_) {
80458046
AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::TYPE,
80468047
[&] { return type.GetVisibilityError(file_); });
80478048
return;

‎src/google/protobuf/descriptor.h‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2360,6 +2360,13 @@ class PROTOBUF_EXPORT DescriptorPool {
23602360
// of this enforcement.
23612361
void EnforceNamingStyle(bool enforce) { enforce_naming_style_ = enforce; }
23622362

2363+
// Enforce symbol visibility rules. This will enable enforcement of the
2364+
// `export` and `local` keywords added in edition 2024, honoring the behavior
2365+
// of the `default_symbol_visibility` feature.
2366+
void EnforceSymbolVisibility(bool enforce) {
2367+
enforce_symbol_visibility_ = enforce;
2368+
}
2369+
23632370
// By default, option imports are allowed to be missing.
23642371
// If you call EnforceOptionDependencies(true), however, the DescriptorPool
23652372
// will report a import not found error.
@@ -2675,6 +2682,7 @@ class PROTOBUF_EXPORT DescriptorPool {
26752682
bool disallow_enforce_utf8_;
26762683
bool deprecated_legacy_json_field_conflicts_;
26772684
bool enforce_naming_style_;
2685+
bool enforce_symbol_visibility_ = false;
26782686
mutable bool build_started_ = false;
26792687

26802688
// Set of files to track for additional validation. The bool value when true

‎src/google/protobuf/descriptor_unittest.cc‎

Lines changed: 64 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -10113,6 +10113,7 @@ TEST_F(FeaturesTest, NoNamingStyleViolationsWithPoolOptInIfMessagesAreGood) {
1011310113
}
1011410114

1011510115
TEST_F(FeaturesTest, VisibilityFeatureSetStrict) {
10116+
pool_.EnforceSymbolVisibility(true);
1011610117
BuildDescriptorMessagesInTestPool();
1011710118

1011810119
ASSERT_THAT(ParseAndBuildFile("vis.proto", R"schema(
@@ -10137,6 +10138,7 @@ TEST_F(FeaturesTest, VisibilityFeatureSetStrict) {
1013710138
}
1013810139

1013910140
TEST_F(FeaturesTest, VisibilityFeatureSetStrictBadNested) {
10141+
pool_.EnforceSymbolVisibility(true);
1014010142
BuildDescriptorMessagesInTestPool();
1014110143

1014210144
ParseAndBuildFileWithErrorSubstr(
@@ -10157,6 +10159,24 @@ TEST_F(FeaturesTest, VisibilityFeatureSetStrictBadNested) {
1015710159
"in order to be `export`.");
1015810160
}
1015910161

10162+
TEST_F(FeaturesTest, VisibilityFeatureSetStrictBadNestedDisabled) {
10163+
pool_.EnforceSymbolVisibility(false);
10164+
BuildDescriptorMessagesInTestPool();
10165+
10166+
EXPECT_THAT(ParseAndBuildFile("vis.proto", R"schema(
10167+
edition = "2024";
10168+
package naming;
10169+
10170+
option features.default_symbol_visibility = STRICT;
10171+
10172+
local message LocalOuter {
10173+
export message Inner {
10174+
}
10175+
}
10176+
)schema"),
10177+
NotNull());
10178+
}
10179+
1016010180
TEST_F(FeaturesTest, BadPackageName) {
1016110181
BuildDescriptorMessagesInTestPool();
1016210182

@@ -13188,7 +13208,8 @@ TEST_F(ValidationErrorTest, ExtensionDeclarationsFullNameMissingLeadingDot) {
1318813208
}
1318913209

1319013210
TEST_F(ValidationErrorTest, VisibilityFromSame) {
13191-
ParseAndBuildFile("vis.proto", R"schema(
13211+
pool_.EnforceSymbolVisibility(true);
13212+
EXPECT_THAT(ParseAndBuildFile("vis.proto", R"schema(
1319213213
edition = "2024";
1319313214
package vis.test;
1319413215

@@ -13197,19 +13218,22 @@ TEST_F(ValidationErrorTest, VisibilityFromSame) {
1319713218
export message ExportMessage {
1319813219
LocalMessage foo = 1;
1319913220
}
13200-
)schema");
13221+
)schema"),
13222+
NotNull());
1320113223
}
1320213224

1320313225
TEST_F(ValidationErrorTest, ExplicitVisibilityFromOther) {
13204-
ParseAndBuildFile("vis.proto", R"schema(
13226+
pool_.EnforceSymbolVisibility(true);
13227+
ASSERT_THAT(ParseAndBuildFile("vis.proto", R"schema(
1320513228
edition = "2024";
1320613229
package vis.test;
1320713230

1320813231
local message LocalMessage {
1320913232
}
1321013233
export message ExportMessage {
1321113234
}
13212-
)schema");
13235+
)schema"),
13236+
NotNull());
1321313237

1321413238
ParseAndBuildFileWithErrorSubstr(
1321513239
"importer.proto",
@@ -13227,25 +13251,53 @@ TEST_F(ValidationErrorTest, ExplicitVisibilityFromOther) {
1322713251
"file\n");
1322813252
}
1322913253

13254+
TEST_F(ValidationErrorTest, ExplicitVisibilityFromOtherDisabled) {
13255+
pool_.EnforceSymbolVisibility(false);
13256+
ASSERT_THAT(ParseAndBuildFile("vis.proto", R"schema(
13257+
edition = "2024";
13258+
package vis.test;
13259+
13260+
local message LocalMessage {
13261+
}
13262+
export message ExportMessage {
13263+
}
13264+
)schema"),
13265+
NotNull());
13266+
13267+
EXPECT_THAT(ParseAndBuildFile("importer.proto",
13268+
R"schema(
13269+
edition = "2024";
13270+
import "vis.proto";
13271+
13272+
message BadImport {
13273+
vis.test.LocalMessage foo = 1;
13274+
}
13275+
)schema"),
13276+
NotNull());
13277+
}
13278+
1323013279
TEST_F(ValidationErrorTest, Edition2024DefaultVisibilityFromOther) {
13231-
ParseAndBuildFile("vis.proto", R"schema(
13280+
pool_.EnforceSymbolVisibility(true);
13281+
ASSERT_THAT(ParseAndBuildFile("vis.proto", R"schema(
1323213282
edition = "2024";
1323313283
package vis.test;
1323413284

1323513285
message TopLevelMessage {
1323613286
message NestedMessage {
1323713287
}
1323813288
}
13239-
)schema");
13289+
)schema"),
13290+
NotNull());
1324013291

13241-
ParseAndBuildFile("good_importer.proto", R"schema(
13292+
ASSERT_THAT(ParseAndBuildFile("good_importer.proto", R"schema(
1324213293
edition = "2024";
1324313294
import "vis.proto";
1324413295

1324513296
message GoodImport {
1324613297
vis.test.TopLevelMessage foo = 1;
1324713298
}
13248-
)schema");
13299+
)schema"),
13300+
NotNull());
1324913301

1325013302
ParseAndBuildFileWithErrorSubstr(
1325113303
"bad_importer.proto", R"schema(
@@ -13265,14 +13317,16 @@ TEST_F(ValidationErrorTest, Edition2024DefaultVisibilityFromOther) {
1326513317
}
1326613318

1326713319
TEST_F(ValidationErrorTest, VisibilityFromLocalExtender) {
13268-
ParseAndBuildFile("vis.proto", R"schema(
13320+
pool_.EnforceSymbolVisibility(true);
13321+
ASSERT_THAT(ParseAndBuildFile("vis.proto", R"schema(
1326913322
edition = "2024";
1327013323
package vis.test;
1327113324

1327213325
local message LocalExtendee {
1327313326
extensions 1 to 100;
1327413327
}
13275-
)schema");
13328+
)schema"),
13329+
NotNull());
1327613330

1327713331
ParseAndBuildFileWithErrorSubstr(
1327813332
"bad_importer.proto", R"schema(

0 commit comments

Comments
 (0)