Skip to content

Commit aad0daa

Browse files
mkruskal-googlemkruskal-google
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 c0b7491 commit aad0daa

File tree

5 files changed

+145
-13
lines changed

5 files changed

+145
-13
lines changed

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

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

12621262
descriptor_pool->EnforceWeakDependencies(true);
12631263
descriptor_pool->EnforceOptionDependencies(true);
1264+
descriptor_pool->EnforceSymbolVisibility(true);
12641265
descriptor_pool->EnforceNamingStyle(true);
12651266

12661267
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
@@ -4718,6 +4718,74 @@ TEST_F(CommandLineInterfaceTest, VisibilityFromSame) {
47184718
ExpectNoErrors();
47194719
}
47204720

4721+
TEST_F(CommandLineInterfaceTest, NonDefaultSymbolVisibilityBuiltInCodegen) {
4722+
CreateTempFile("vis.proto", R"schema(
4723+
edition = "2024";
4724+
package vis.test;
4725+
option features.default_symbol_visibility = EXPORT_ALL;
4726+
4727+
message TopLevelMessage {
4728+
message NestedMessage {
4729+
}
4730+
enum NestedEnum {
4731+
NESTED_ENUM_UNKNOWN = 0;
4732+
NESTED_ENUM_BAR = 1;
4733+
}
4734+
}
4735+
)schema");
4736+
4737+
CreateTempFile("good_importer.proto", R"schema(
4738+
edition = "2024";
4739+
import "vis.proto";
4740+
option features.default_symbol_visibility = EXPORT_ALL;
4741+
4742+
message GoodImport {
4743+
vis.test.TopLevelMessage foo = 1;
4744+
vis.test.TopLevelMessage.NestedMessage bar = 2;
4745+
vis.test.TopLevelMessage.NestedEnum baz = 3;
4746+
}
4747+
)schema");
4748+
Run("protocol_compiler --test_out=$tmpdir "
4749+
"--experimental_editions " // remove when edition 2024 is valid
4750+
"--proto_path=$tmpdir good_importer.proto vis.proto");
4751+
4752+
ExpectNoErrors();
4753+
}
4754+
4755+
TEST_F(CommandLineInterfaceTest, NonDefaultSymbolVisibilityPluginCodegen) {
4756+
CreateTempFile("vis.proto", R"schema(
4757+
edition = "2024";
4758+
package vis.test;
4759+
option features.default_symbol_visibility = EXPORT_ALL;
4760+
4761+
message TopLevelMessage {
4762+
message NestedMessage {
4763+
}
4764+
enum NestedEnum {
4765+
NESTED_ENUM_UNKNOWN = 0;
4766+
NESTED_ENUM_BAR = 1;
4767+
}
4768+
}
4769+
)schema");
4770+
4771+
CreateTempFile("good_importer.proto", R"schema(
4772+
edition = "2024";
4773+
import "vis.proto";
4774+
option features.default_symbol_visibility = EXPORT_ALL;
4775+
4776+
message GoodImport {
4777+
vis.test.TopLevelMessage foo = 1;
4778+
vis.test.TopLevelMessage.NestedMessage bar = 2;
4779+
vis.test.TopLevelMessage.NestedEnum baz = 3;
4780+
}
4781+
)schema");
4782+
Run("protocol_compiler --plug_out=$tmpdir "
4783+
"--experimental_editions " // remove when edition 2024 is valid
4784+
"--proto_path=$tmpdir good_importer.proto vis.proto");
4785+
4786+
ExpectNoErrors();
4787+
}
4788+
47214789
TEST_F(CommandLineInterfaceTest, ExplicitVisibilityFromOther) {
47224790
CreateTempFile("vis.proto", R"schema(
47234791
edition = "2024";

‎src/google/protobuf/descriptor.cc‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -6726,7 +6726,7 @@ FileDescriptor* DescriptorBuilder::BuildFileImpl(
67266726
}
67276727
});
67286728
}
6729-
if (!had_errors_) {
6729+
if (!had_errors_ && pool_->enforce_symbol_visibility_) {
67306730
// Check Symbol Visibility Rules.
67316731
CheckVisibilityRules(result, proto);
67326732
}
@@ -7914,7 +7914,8 @@ void DescriptorBuilder::CrossLinkField(FieldDescriptor* field,
79147914
"\" is not a message type.");
79157915
});
79167916
return;
7917-
} else if (!extendee.IsVisibleFrom(file_)) {
7917+
} else if (!extendee.IsVisibleFrom(file_) &&
7918+
pool_->enforce_symbol_visibility_) {
79187919
AddError(field->full_name(), proto,
79197920
DescriptorPool::ErrorCollector::EXTENDEE, [&] {
79207921
return extendee.GetVisibilityError(file_, "target of extend");
@@ -8026,7 +8027,7 @@ void DescriptorBuilder::CrossLinkField(FieldDescriptor* field,
80268027
field->is_map_ = sub_message->options().map_entry();
80278028
}
80288029

8029-
if (!type.IsVisibleFrom(file_)) {
8030+
if (!type.IsVisibleFrom(file_) && pool_->enforce_symbol_visibility_) {
80308031
AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::TYPE,
80318032
[&] { return type.GetVisibilityError(file_); });
80328033
return;

‎src/google/protobuf/descriptor.h‎

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

2360+
// Enforce symbol visibility rules. This will enable enforcement of the
2361+
// `export` and `local` keywords added in edition 2024, honoring the behavior
2362+
// of the `default_symbol_visibility` feature.
2363+
void EnforceSymbolVisibility(bool enforce) {
2364+
enforce_symbol_visibility_ = enforce;
2365+
}
2366+
23602367
// By default, option imports are allowed to be missing.
23612368
// If you call EnforceOptionDependencies(true), however, the DescriptorPool
23622369
// will report a import not found error.
@@ -2672,6 +2679,7 @@ class PROTOBUF_EXPORT DescriptorPool {
26722679
bool disallow_enforce_utf8_;
26732680
bool deprecated_legacy_json_field_conflicts_;
26742681
bool enforce_naming_style_;
2682+
bool enforce_symbol_visibility_ = false;
26752683
mutable bool build_started_ = false;
26762684

26772685
// 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
@@ -10094,6 +10094,7 @@ TEST_F(FeaturesTest, NoNamingStyleViolationsWithPoolOptInIfMessagesAreGood) {
1009410094
}
1009510095

1009610096
TEST_F(FeaturesTest, VisibilityFeatureSetStrict) {
10097+
pool_.EnforceSymbolVisibility(true);
1009710098
BuildDescriptorMessagesInTestPool();
1009810099

1009910100
ASSERT_THAT(ParseAndBuildFile("vis.proto", R"schema(
@@ -10118,6 +10119,7 @@ TEST_F(FeaturesTest, VisibilityFeatureSetStrict) {
1011810119
}
1011910120

1012010121
TEST_F(FeaturesTest, VisibilityFeatureSetStrictBadNested) {
10122+
pool_.EnforceSymbolVisibility(true);
1012110123
BuildDescriptorMessagesInTestPool();
1012210124

1012310125
ParseAndBuildFileWithErrorSubstr(
@@ -10138,6 +10140,24 @@ TEST_F(FeaturesTest, VisibilityFeatureSetStrictBadNested) {
1013810140
"in order to be `export`.");
1013910141
}
1014010142

10143+
TEST_F(FeaturesTest, VisibilityFeatureSetStrictBadNestedDisabled) {
10144+
pool_.EnforceSymbolVisibility(false);
10145+
BuildDescriptorMessagesInTestPool();
10146+
10147+
EXPECT_THAT(ParseAndBuildFile("vis.proto", R"schema(
10148+
edition = "2024";
10149+
package naming;
10150+
10151+
option features.default_symbol_visibility = STRICT;
10152+
10153+
local message LocalOuter {
10154+
export message Inner {
10155+
}
10156+
}
10157+
)schema"),
10158+
NotNull());
10159+
}
10160+
1014110161
TEST_F(FeaturesTest, BadPackageName) {
1014210162
BuildDescriptorMessagesInTestPool();
1014310163

@@ -13169,7 +13189,8 @@ TEST_F(ValidationErrorTest, ExtensionDeclarationsFullNameMissingLeadingDot) {
1316913189
}
1317013190

1317113191
TEST_F(ValidationErrorTest, VisibilityFromSame) {
13172-
ParseAndBuildFile("vis.proto", R"schema(
13192+
pool_.EnforceSymbolVisibility(true);
13193+
EXPECT_THAT(ParseAndBuildFile("vis.proto", R"schema(
1317313194
edition = "2024";
1317413195
package vis.test;
1317513196

@@ -13178,19 +13199,22 @@ TEST_F(ValidationErrorTest, VisibilityFromSame) {
1317813199
export message ExportMessage {
1317913200
LocalMessage foo = 1;
1318013201
}
13181-
)schema");
13202+
)schema"),
13203+
NotNull());
1318213204
}
1318313205

1318413206
TEST_F(ValidationErrorTest, ExplicitVisibilityFromOther) {
13185-
ParseAndBuildFile("vis.proto", R"schema(
13207+
pool_.EnforceSymbolVisibility(true);
13208+
ASSERT_THAT(ParseAndBuildFile("vis.proto", R"schema(
1318613209
edition = "2024";
1318713210
package vis.test;
1318813211

1318913212
local message LocalMessage {
1319013213
}
1319113214
export message ExportMessage {
1319213215
}
13193-
)schema");
13216+
)schema"),
13217+
NotNull());
1319413218

1319513219
ParseAndBuildFileWithErrorSubstr(
1319613220
"importer.proto",
@@ -13208,25 +13232,53 @@ TEST_F(ValidationErrorTest, ExplicitVisibilityFromOther) {
1320813232
"file\n");
1320913233
}
1321013234

13235+
TEST_F(ValidationErrorTest, ExplicitVisibilityFromOtherDisabled) {
13236+
pool_.EnforceSymbolVisibility(false);
13237+
ASSERT_THAT(ParseAndBuildFile("vis.proto", R"schema(
13238+
edition = "2024";
13239+
package vis.test;
13240+
13241+
local message LocalMessage {
13242+
}
13243+
export message ExportMessage {
13244+
}
13245+
)schema"),
13246+
NotNull());
13247+
13248+
EXPECT_THAT(ParseAndBuildFile("importer.proto",
13249+
R"schema(
13250+
edition = "2024";
13251+
import "vis.proto";
13252+
13253+
message BadImport {
13254+
vis.test.LocalMessage foo = 1;
13255+
}
13256+
)schema"),
13257+
NotNull());
13258+
}
13259+
1321113260
TEST_F(ValidationErrorTest, Edition2024DefaultVisibilityFromOther) {
13212-
ParseAndBuildFile("vis.proto", R"schema(
13261+
pool_.EnforceSymbolVisibility(true);
13262+
ASSERT_THAT(ParseAndBuildFile("vis.proto", R"schema(
1321313263
edition = "2024";
1321413264
package vis.test;
1321513265

1321613266
message TopLevelMessage {
1321713267
message NestedMessage {
1321813268
}
1321913269
}
13220-
)schema");
13270+
)schema"),
13271+
NotNull());
1322113272

13222-
ParseAndBuildFile("good_importer.proto", R"schema(
13273+
ASSERT_THAT(ParseAndBuildFile("good_importer.proto", R"schema(
1322313274
edition = "2024";
1322413275
import "vis.proto";
1322513276

1322613277
message GoodImport {
1322713278
vis.test.TopLevelMessage foo = 1;
1322813279
}
13229-
)schema");
13280+
)schema"),
13281+
NotNull());
1323013282

1323113283
ParseAndBuildFileWithErrorSubstr(
1323213284
"bad_importer.proto", R"schema(
@@ -13246,14 +13298,16 @@ TEST_F(ValidationErrorTest, Edition2024DefaultVisibilityFromOther) {
1324613298
}
1324713299

1324813300
TEST_F(ValidationErrorTest, VisibilityFromLocalExtender) {
13249-
ParseAndBuildFile("vis.proto", R"schema(
13301+
pool_.EnforceSymbolVisibility(true);
13302+
ASSERT_THAT(ParseAndBuildFile("vis.proto", R"schema(
1325013303
edition = "2024";
1325113304
package vis.test;
1325213305

1325313306
local message LocalExtendee {
1325413307
extensions 1 to 100;
1325513308
}
13256-
)schema");
13309+
)schema"),
13310+
NotNull());
1325713311

1325813312
ParseAndBuildFileWithErrorSubstr(
1325913313
"bad_importer.proto", R"schema(

0 commit comments

Comments
 (0)