Skip to content

Commit 18561e4

Browse files
pizzudcopybara-github
authored andcommitted
SymbolChecker: Bugfix to take parent message visibility into account.
The early exit in IsNamespacedEnum would trigger examples like edition = "2024"; local message Parent { export enum Child { ZERO = 0; } reserved 1 to max; } to be considered not namespaced, as the default-to-local early exit would fire. New unit tests are added for this and related cases. PiperOrigin-RevId: 871320466
1 parent ddfdff4 commit 18561e4

2 files changed

Lines changed: 68 additions & 7 deletions

File tree

src/google/protobuf/symbol_checker.cc

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ bool SymbolChecker::IsEnumNamespaceMessage(const Descriptor& container) {
132132
}
133133

134134
bool SymbolChecker::IsNamespacedEnum(const EnumDescriptor& enm) {
135-
// Only allowed for top-level messages
135+
// If the enum is top-level, it definitely isn't namespaced.
136136
if (enm.containing_type() == nullptr) {
137137
return false;
138138
}
@@ -147,12 +147,10 @@ bool SymbolChecker::IsNamespacedEnum(const EnumDescriptor& enm) {
147147
// This check only cares if the enum is explicitly marked export, the
148148
// containing message can be local or unset with the file default making it
149149
// local.
150-
if (!default_to_local ||
151-
enm.visibility_keyword() != SymbolVisibility::VISIBILITY_EXPORT) {
152-
return false;
153-
}
154-
155-
return IsEnumNamespaceMessage(*enm.containing_type());
150+
bool is_exported =
151+
!default_to_local ||
152+
enm.visibility_keyword() == SymbolVisibility::VISIBILITY_EXPORT;
153+
return is_exported && IsEnumNamespaceMessage(*enm.containing_type());
156154
}
157155

158156
void SymbolChecker::Initialize() {

src/google/protobuf/symbol_checker_test.cc

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,69 @@ TEST_F(SymbolCheckerTest, IsEnumNamespaceMessage) {
242242
ASSERT_FALSE(SymbolChecker::IsNamespacedEnum(*top_enum_descriptor));
243243
}
244244

245+
TEST_F(SymbolCheckerTest, IsNamespacedEnumLocalDefault) {
246+
pool_.EnforceSymbolVisibility(true);
247+
const FileDescriptor* file = ParseAndBuildFile("vis.proto", R"schema(
248+
edition = "2024";
249+
package vis.test;
250+
251+
option features.default_symbol_visibility = LOCAL_ALL;
252+
message EnumNamespaceMessage {
253+
export enum Enum {
254+
FOO = 0;
255+
}
256+
reserved 1 to max;
257+
}
258+
259+
enum TopLevelEnum {
260+
BAR = 0;
261+
}
262+
)schema");
263+
264+
ASSERT_THAT(file, NotNull());
265+
266+
const Descriptor* namespace_message =
267+
file->FindMessageTypeByName("EnumNamespaceMessage");
268+
ASSERT_THAT(namespace_message, NotNull());
269+
270+
const EnumDescriptor* enum_descriptor =
271+
namespace_message->FindEnumTypeByName("Enum");
272+
ASSERT_THAT(enum_descriptor, NotNull());
273+
274+
ASSERT_TRUE(SymbolChecker::IsNamespacedEnum(*enum_descriptor));
275+
}
276+
277+
TEST_F(SymbolCheckerTest, IsNamespacedEnumExplicitlyLocalParent) {
278+
pool_.EnforceSymbolVisibility(true);
279+
const FileDescriptor* file = ParseAndBuildFile("vis.proto", R"schema(
280+
edition = "2024";
281+
package vis.test;
282+
283+
local message EnumNamespaceMessage {
284+
enum Enum {
285+
FOO = 0;
286+
}
287+
reserved 1 to max;
288+
}
289+
290+
enum TopLevelEnum {
291+
BAR = 0;
292+
}
293+
)schema");
294+
295+
ASSERT_THAT(file, NotNull());
296+
297+
const Descriptor* namespace_message =
298+
file->FindMessageTypeByName("EnumNamespaceMessage");
299+
ASSERT_THAT(namespace_message, NotNull());
300+
301+
const EnumDescriptor* enum_descriptor =
302+
namespace_message->FindEnumTypeByName("Enum");
303+
ASSERT_THAT(enum_descriptor, NotNull());
304+
305+
ASSERT_TRUE(SymbolChecker::IsNamespacedEnum(*enum_descriptor));
306+
}
307+
245308
TEST_F(SymbolCheckerTest, IsNotEnumNamespaceMessage) {
246309
pool_.EnforceSymbolVisibility(true);
247310

0 commit comments

Comments
 (0)