From a21661ac39fe6e6843ab343c7484dcec45f67372 Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Tue, 4 Nov 2025 09:19:11 -0800 Subject: [PATCH] Add new removal error field to feature support error message PiperOrigin-RevId: 828004942 --- .../command_line_interface_unittest.cc | 13 ++--- .../compiler/cpp/generator_unittest.cc | 6 +-- .../compiler/java/generator_unittest.cc | 2 +- src/google/protobuf/descriptor_unittest.cc | 29 ++++++------ src/google/protobuf/feature_resolver.cc | 47 ++++++++++++------- src/google/protobuf/feature_resolver_test.cc | 35 ++++++++++---- src/google/protobuf/unittest_features.proto | 15 ++++++ 7 files changed, 96 insertions(+), 51 deletions(-) diff --git a/src/google/protobuf/compiler/command_line_interface_unittest.cc b/src/google/protobuf/compiler/command_line_interface_unittest.cc index f8dfdcee3b659..e71d445b6b66c 100644 --- a/src/google/protobuf/compiler/command_line_interface_unittest.cc +++ b/src/google/protobuf/compiler/command_line_interface_unittest.cc @@ -1762,8 +1762,9 @@ TEST_F(CommandLineInterfaceTest, Plugin_DeprecatedFeature) { Run("protocol_compiler --test_out=$tmpdir " "--proto_path=$tmpdir foo.proto"); ExpectWarningSubstring( - "foo.proto:4:5: warning: Feature pb.TestFeatures.removed_feature has " - "been deprecated in edition 2023: Custom feature deprecation warning\n"); + "foo.proto:4:5: warning: pb.TestFeatures.removed_feature has " + "been deprecated in edition 2023: Custom feature " + "deprecation warning\n"); } TEST_F(CommandLineInterfaceTest, Plugin_TransitiveDeprecatedFeature) { @@ -2031,8 +2032,8 @@ TEST_F(CommandLineInterfaceTest, GeneratorFeatureLifetimeError) { Run("protocol_compiler --experimental_editions --proto_path=$tmpdir " "--test_out=$tmpdir foo.proto"); ExpectErrorSubstring( - "foo.proto:6:13: Feature pb.TestFeatures.removed_feature has been " - "removed in edition 2024"); + "foo.proto:6:13: pb.TestFeatures.removed_feature has been " + "removed in edition 2024:"); } TEST_F(CommandLineInterfaceTest, PluginFeatureLifetimeError) { @@ -2064,8 +2065,8 @@ TEST_F(CommandLineInterfaceTest, PluginFeatureLifetimeError) { "foo.proto --plugin=prefix-gen-fake_plugin=", plugin_path)); ExpectErrorSubstring( - "foo.proto:6:13: Feature pb.TestFeatures.future_feature wasn't " - "introduced until edition 2024"); + "foo.proto:6:13: pb.TestFeatures.future_feature wasn't introduced " + "until edition 2024 and can't be used in edition 2023"); } TEST_F(CommandLineInterfaceTest, GeneratorNoEditionsSupport) { diff --git a/src/google/protobuf/compiler/cpp/generator_unittest.cc b/src/google/protobuf/compiler/cpp/generator_unittest.cc index 9b01e4b4b3ad1..0c8664da23a5b 100644 --- a/src/google/protobuf/compiler/cpp/generator_unittest.cc +++ b/src/google/protobuf/compiler/cpp/generator_unittest.cc @@ -100,8 +100,8 @@ TEST_F(CppGeneratorTest, LegacyClosedEnum) { "protocol_compiler --proto_path=$tmpdir --cpp_out=$tmpdir foo.proto"); ExpectWarningSubstring( - "foo.proto:9:16: warning: Feature pb.CppFeatures.legacy_closed_enum has " - "been deprecated in edition 2023"); + "foo.proto:9:16: warning: pb.CppFeatures.legacy_closed_enum has " + "been deprecated in edition 2023:"); } TEST_F(CppGeneratorTest, LegacyClosedEnumInherited) { @@ -123,7 +123,7 @@ TEST_F(CppGeneratorTest, LegacyClosedEnumInherited) { "protocol_compiler --proto_path=$tmpdir --cpp_out=$tmpdir foo.proto"); ExpectWarningSubstring( - "foo.proto: warning: Feature pb.CppFeatures.legacy_closed_enum has " + "foo.proto: warning: pb.CppFeatures.legacy_closed_enum has " "been deprecated in edition 2023"); } diff --git a/src/google/protobuf/compiler/java/generator_unittest.cc b/src/google/protobuf/compiler/java/generator_unittest.cc index 1426f3b1750d3..f4ecba2c5216e 100644 --- a/src/google/protobuf/compiler/java/generator_unittest.cc +++ b/src/google/protobuf/compiler/java/generator_unittest.cc @@ -354,7 +354,7 @@ BBB = 1; "--experimental_editions"); ExpectErrorSubstring( - "foo.proto:6:6: Feature pb.JavaFeatures.large_enum wasn't introduced " + "foo.proto:6:6: pb.JavaFeatures.large_enum wasn't introduced " "until edition 2024 and can't be used in edition 2023"); } diff --git a/src/google/protobuf/descriptor_unittest.cc b/src/google/protobuf/descriptor_unittest.cc index 64bc26a680255..e9686ab7dc424 100644 --- a/src/google/protobuf/descriptor_unittest.cc +++ b/src/google/protobuf/descriptor_unittest.cc @@ -12200,7 +12200,7 @@ TEST_F(FeaturesTest, DeprecatedFeature) { } } )pb", - "foo.proto: foo.proto: NAME: Feature " + "foo.proto: foo.proto: NAME: " "pb.TestFeatures.removed_feature has been deprecated in edition 2023: " "Custom feature deprecation warning\n"); const FileDescriptor* file = pool_.FindFileByName("foo.proto"); @@ -12287,9 +12287,9 @@ TEST_F(FeaturesTest, RemovedFeature) { } } )pb", - "foo.proto: foo.proto: NAME: Feature " - "pb.TestFeatures.removed_feature has been removed in edition 2024 and " - "can't be used in edition 2024\n"); + "foo.proto: foo.proto: NAME: " + "pb.TestFeatures.removed_feature has been removed in edition 2024: " + "Custom feature removal error\n"); } TEST_F(FeaturesTest, RemovedFeatureDefault) { @@ -12319,9 +12319,9 @@ TEST_F(FeaturesTest, FutureFeature) { } } )pb", - "foo.proto: foo.proto: NAME: Feature " - "pb.TestFeatures.future_feature wasn't introduced until edition 2024 and " - "can't be used in edition 2023\n"); + "foo.proto: foo.proto: NAME: " + "pb.TestFeatures.future_feature wasn't introduced until edition " + "2024 and can't be used in edition 2023\n"); } TEST_F(FeaturesTest, FutureFeatureDefault) { @@ -14121,10 +14121,11 @@ TEST_F(DatabaseBackedPoolTest, FeatureLifetimeError) { DescriptorPool pool(&database_, &error_collector); EXPECT_TRUE(pool.FindMessageTypeByName("FooFeatures") == nullptr); - EXPECT_EQ(error_collector.text_, - "features.proto: FooFeatures: NAME: Feature " - "pb.TestFeatures.future_feature wasn't introduced until edition " - "2024 and can't be used in edition 2023\n"); + EXPECT_EQ( + error_collector.text_, + "features.proto: FooFeatures: NAME: " + "pb.TestFeatures.future_feature wasn't introduced until edition 2024 " + "and can't be used in edition 2023\n"); } TEST_F(DatabaseBackedPoolTest, FeatureLifetimeErrorUnknownDependencies) { @@ -14193,9 +14194,9 @@ TEST_F(DatabaseBackedPoolTest, FeatureLifetimeErrorUnknownDependencies) { error_collector.text_.clear(); ASSERT_EQ(pool.FindExtensionByName("foo_extension"), nullptr); EXPECT_EQ(error_collector.text_, - "option.proto: foo_extension: NAME: Feature " - "pb.TestFeatures.legacy_feature has been removed in edition 2023 " - "and can't be used in edition 2023\n"); + "option.proto: foo_extension: NAME: " + "pb.TestFeatures.legacy_feature has been removed in edition 2023: " + "Custom feature removal error\n"); } TEST_F(DatabaseBackedPoolTest, DoesntRetryDbUnnecessarily) { diff --git a/src/google/protobuf/feature_resolver.cc b/src/google/protobuf/feature_resolver.cc index f5b8abae08d4f..380265e8031a5 100644 --- a/src/google/protobuf/feature_resolver.cc +++ b/src/google/protobuf/feature_resolver.cc @@ -26,6 +26,7 @@ #include "absl/strings/str_join.h" #include "absl/strings/str_split.h" #include "absl/strings/string_view.h" +#include "absl/strings/substitute.h" #include "absl/types/span.h" #include "google/protobuf/cpp_features.pb.h" #include "google/protobuf/descriptor.h" @@ -364,26 +365,36 @@ absl::Status ValidateMergedFeatures(const FeatureSet& features) { void ValidateSingleFeatureLifetimes( Edition edition, absl::string_view full_name, - const FieldOptions::FeatureSupport& support, + const FieldOptions::FeatureSupport& feature_support, FeatureResolver::ValidationResults& results) { // Skip fields that don't have feature support specified. - if (&support == &FieldOptions::FeatureSupport::default_instance()) return; - - if (edition < support.edition_introduced()) { - results.errors.emplace_back( - absl::StrCat("Feature ", full_name, " wasn't introduced until edition ", - support.edition_introduced(), - " and can't be used in edition ", edition)); - } - if (support.has_edition_removed() && edition >= support.edition_removed()) { - results.errors.emplace_back(absl::StrCat( - "Feature ", full_name, " has been removed in edition ", - support.edition_removed(), " and can't be used in edition ", edition)); - } else if (support.has_edition_deprecated() && - edition >= support.edition_deprecated()) { - results.warnings.emplace_back(absl::StrCat( - "Feature ", full_name, " has been deprecated in edition ", - support.edition_deprecated(), ": ", support.deprecation_warning())); + if (&feature_support == &FieldOptions::FeatureSupport::default_instance()) + return; + // safe guarding new features that aren't available yet + if (edition < feature_support.edition_introduced()) { + std::string error_message = absl::Substitute( + "$0 wasn't introduced until edition $1 and can't be used in " + "edition $2", + full_name, feature_support.edition_introduced(), edition); + results.errors.emplace_back(std::move(error_message)); + } + if (feature_support.has_edition_removed() && + edition >= feature_support.edition_removed()) { + std::string error_message = absl::Substitute( + "$0 has been removed in edition $1$2", full_name, + feature_support.edition_removed(), + (feature_support.has_removal_error()) + ? absl::StrCat(": ", feature_support.removal_error()) + : ""); + results.errors.emplace_back(std::move(error_message)); + } else if (feature_support.has_edition_deprecated() && + edition >= feature_support.edition_deprecated()) { + std::string error_message = absl::Substitute( + "$0 has been deprecated in edition " + "$1: $2", + full_name, feature_support.edition_deprecated(), + feature_support.deprecation_warning()); + results.warnings.emplace_back(std::move(error_message)); } } diff --git a/src/google/protobuf/feature_resolver_test.cc b/src/google/protobuf/feature_resolver_test.cc index 341b69ae72825..06aae5ef121b3 100644 --- a/src/google/protobuf/feature_resolver_test.cc +++ b/src/google/protobuf/feature_resolver_test.cc @@ -634,7 +634,21 @@ TEST(FeatureResolverLifetimesTest, RemovedFeature) { features, nullptr); EXPECT_THAT(results.errors, ElementsAre(AllOf(HasSubstr("pb.TestFeatures.removed_feature"), - HasSubstr("removed in edition 2024")))); + HasSubstr("removed in edition 2024:"), + HasSubstr("Custom feature removal error")))); + EXPECT_THAT(results.warnings, IsEmpty()); +} + +TEST(FeatureResolverLifetimesTest, RemovedFeatureWithNoRemovalError) { + FeatureSet features = ParseTextOrDie(R"pb( + [pb.test] { same_edition_removed_feature: VALUE1 } + )pb"); + auto results = FeatureResolver::ValidateFeatureLifetimes(EDITION_2023, + features, nullptr); + EXPECT_THAT(results.errors, + ElementsAre(AllOf( + HasSubstr("pb.TestFeatures.same_edition_removed_feature"), + HasSubstr("removed in edition 2023"), Not(HasSubstr(":"))))); EXPECT_THAT(results.warnings, IsEmpty()); } @@ -644,9 +658,11 @@ TEST(FeatureResolverLifetimesTest, NotIntroduced) { )pb"); auto results = FeatureResolver::ValidateFeatureLifetimes(EDITION_2023, features, nullptr); - EXPECT_THAT(results.errors, - ElementsAre(AllOf(HasSubstr("pb.TestFeatures.future_feature"), - HasSubstr("introduced until edition 2024")))); + EXPECT_THAT( + results.errors, + ElementsAre(AllOf(HasSubstr("pb.TestFeatures.future_feature"), + HasSubstr("wasn't introduced until edition 2024"), + HasSubstr("can't be used in edition 2023")))); EXPECT_THAT(results.warnings, IsEmpty()); } @@ -730,7 +746,8 @@ TEST(FeatureResolverLifetimesTest, ValueSupportBeforeIntroduced) { EXPECT_THAT(results.errors, ElementsAre(AllOf( HasSubstr("pb.VALUE_LIFETIME_FUTURE"), - HasSubstr("introduced until edition 99997_TEST_ONLY")))); + HasSubstr("wasn't introduced until edition 99997_TEST_ONLY"), + HasSubstr("can't be used in edition 2023")))); EXPECT_THAT(results.warnings, IsEmpty()); } @@ -740,10 +757,10 @@ TEST(FeatureResolverLifetimesTest, ValueSupportAfterRemoved) { )pb"); auto results = FeatureResolver::ValidateFeatureLifetimes( EDITION_99997_TEST_ONLY, features, nullptr); - EXPECT_THAT( - results.errors, - ElementsAre(AllOf(HasSubstr("pb.VALUE_LIFETIME_REMOVED"), - HasSubstr("removed in edition 99997_TEST_ONLY")))); + EXPECT_THAT(results.errors, + ElementsAre(AllOf(HasSubstr("pb.VALUE_LIFETIME_REMOVED"), + HasSubstr("removed in edition 99997_TEST_ONLY"), + HasSubstr("Custom feature removal error")))); EXPECT_THAT(results.warnings, IsEmpty()); } diff --git a/src/google/protobuf/unittest_features.proto b/src/google/protobuf/unittest_features.proto index dd0d4141ed0a9..1f58f675415e9 100644 --- a/src/google/protobuf/unittest_features.proto +++ b/src/google/protobuf/unittest_features.proto @@ -66,6 +66,7 @@ enum ValueLifetimeFeature { VALUE_LIFETIME_REMOVED = 6 [feature_support = { edition_deprecated: EDITION_2023 edition_removed: EDITION_99997_TEST_ONLY + removal_error: "Custom feature removal error" }]; } @@ -190,12 +191,24 @@ message TestFeatures { edition_deprecated: EDITION_2023 deprecation_warning: "Custom feature deprecation warning" edition_removed: EDITION_2024 + removal_error: "Custom feature removal error" }, edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" }, edition_defaults = { edition: EDITION_2023, value: "VALUE2" }, edition_defaults = { edition: EDITION_2024, value: "VALUE3" } ]; + EnumFeature same_edition_removed_feature = 21 [ + retention = RETENTION_RUNTIME, + targets = TARGET_TYPE_FILE, + targets = TARGET_TYPE_FIELD, + feature_support = { + edition_introduced: EDITION_2023 + edition_removed: EDITION_2023 + }, + edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" } + ]; + EnumFeature future_feature = 18 [ retention = RETENTION_RUNTIME, targets = TARGET_TYPE_FILE, @@ -212,6 +225,7 @@ message TestFeatures { feature_support = { edition_introduced: EDITION_PROTO3 edition_removed: EDITION_2023 + removal_error: "Custom feature removal error" }, edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" }, edition_defaults = { edition: EDITION_2023, value: "VALUE2" } @@ -225,6 +239,7 @@ message TestFeatures { edition_deprecated: EDITION_99998_TEST_ONLY deprecation_warning: "Custom feature deprecation warning" edition_removed: EDITION_99999_TEST_ONLY + removal_error: "Custom feature removal error" }, edition_defaults = { edition: EDITION_LEGACY,