From 38de65f9c1d383b3292b5d39872eca00ee4180fb Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Tue, 11 Apr 2023 13:23:35 -0700 Subject: [PATCH] Update the error message when a duplicate enum number is used to provide the next available enum number in addition to mentioning the option to allow aliasing. For enums that are large but are not sorted numerically it can be difficult to find the correct enum number to use. We use "next available" as the next number larger than the number that was duplicatively used which is not assigned a value. For sparse enums, there may be other enums past the suggested number. PiperOrigin-RevId: 523487238 --- .../protobuf/compiler/parser_unittest.cc | 2 +- src/google/protobuf/descriptor.cc | 15 +++++++++- src/google/protobuf/descriptor_unittest.cc | 29 +++++++++++++++++++ 3 files changed, 44 insertions(+), 2 deletions(-) diff --git a/src/google/protobuf/compiler/parser_unittest.cc b/src/google/protobuf/compiler/parser_unittest.cc index 1b74d774b4..4a5b7c9286 100644 --- a/src/google/protobuf/compiler/parser_unittest.cc +++ b/src/google/protobuf/compiler/parser_unittest.cc @@ -2231,7 +2231,7 @@ TEST_F(ParserValidationErrorTest, EnumValueAliasError) { "}\n", "2:8: \"BAZ\" uses the same enum value as \"BAR\". If this is " "intended, set 'option allow_alias = true;' to the enum " - "definition.\n"); + "definition. The next available enum value is 2.\n"); } TEST_F(ParserValidationErrorTest, ExplicitlyMapEntryError) { diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc index 2750653ee8..5242da004e 100644 --- a/src/google/protobuf/descriptor.cc +++ b/src/google/protobuf/descriptor.cc @@ -7382,12 +7382,25 @@ void DescriptorBuilder::ValidateEnumOptions(EnumDescriptor* enm, if (!enm->options().allow_alias()) { // Generate error if duplicated enum values are explicitly disallowed. auto make_error = [&] { - return absl::StrCat( + // Find the next free number. + absl::flat_hash_set used; + for (int j = 0; j < enm->value_count(); ++j) { + used.insert(enm->value(j)->number()); + } + int64_t next_value = static_cast(enum_value->number()) + 1; + while (used.contains(next_value)) ++next_value; + + std::string error = absl::StrCat( "\"", enum_value->full_name(), "\" uses the same enum value as \"", insert_result.first->second, "\". If this is intended, set " "'option allow_alias = true;' to the enum definition."); + if (next_value < std::numeric_limits::max()) { + absl::StrAppend(&error, " The next available enum value is ", + next_value, "."); + } + return error; }; AddError(enm->full_name(), proto.value(i), DescriptorPool::ErrorCollector::NUMBER, make_error); diff --git a/src/google/protobuf/descriptor_unittest.cc b/src/google/protobuf/descriptor_unittest.cc index 36fa422852..ba00a44ca4 100644 --- a/src/google/protobuf/descriptor_unittest.cc +++ b/src/google/protobuf/descriptor_unittest.cc @@ -6087,6 +6087,35 @@ TEST_F(ValidationErrorTest, DisallowEnumAlias) { "foo.proto: Bar: NUMBER: " "\"ENUM_B\" uses the same enum value as \"ENUM_A\". " "If this is intended, set 'option allow_alias = true;' to the enum " + "definition. The next available enum value is 1.\n"); + + BuildFileWithErrors( + R"pb( + name: "foo.proto" + enum_type { + name: "Bar" + value { name: "ENUM_A" number: 10 } + value { name: "ENUM_B" number: 10 } + value { name: "ENUM_C" number: 11 } + value { name: "ENUM_D" number: 20 } + })pb", + "foo.proto: Bar: NUMBER: " + "\"ENUM_B\" uses the same enum value as \"ENUM_A\". " + "If this is intended, set 'option allow_alias = true;' to the enum " + "definition. The next available enum value is 12.\n"); + + BuildFileWithErrors( + absl::Substitute(R"pb( + name: "foo.proto" + enum_type { + name: "Bar" + value { name: "ENUM_A" number: $0 } + value { name: "ENUM_B" number: $0 } + })pb", + std::numeric_limits::max()), + "foo.proto: Bar: NUMBER: " + "\"ENUM_B\" uses the same enum value as \"ENUM_A\". " + "If this is intended, set 'option allow_alias = true;' to the enum " "definition.\n"); }