Skip to content

Commit 233ca81

Browse files
Add new removal error field to feature support error message
PiperOrigin-RevId: 824605974
1 parent e994af6 commit 233ca81

File tree

6 files changed

+95
-50
lines changed

6 files changed

+95
-50
lines changed

src/google/protobuf/compiler/command_line_interface_unittest.cc

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1762,8 +1762,9 @@ TEST_F(CommandLineInterfaceTest, Plugin_DeprecatedFeature) {
17621762
Run("protocol_compiler --test_out=$tmpdir "
17631763
"--proto_path=$tmpdir foo.proto");
17641764
ExpectWarningSubstring(
1765-
"foo.proto:4:5: warning: Feature pb.TestFeatures.removed_feature has "
1766-
"been deprecated in edition 2023: Custom feature deprecation warning\n");
1765+
"foo.proto:4:5: warning: pb.TestFeatures.removed_feature has "
1766+
"been deprecated in edition 2023: Custom feature "
1767+
"deprecation warning\n");
17671768
}
17681769

17691770
TEST_F(CommandLineInterfaceTest, Plugin_TransitiveDeprecatedFeature) {
@@ -2031,8 +2032,8 @@ TEST_F(CommandLineInterfaceTest, GeneratorFeatureLifetimeError) {
20312032
Run("protocol_compiler --experimental_editions --proto_path=$tmpdir "
20322033
"--test_out=$tmpdir foo.proto");
20332034
ExpectErrorSubstring(
2034-
"foo.proto:6:13: Feature pb.TestFeatures.removed_feature has been "
2035-
"removed in edition 2024");
2035+
"foo.proto:6:13: pb.TestFeatures.removed_feature has been "
2036+
"removed in edition 2024:");
20362037
}
20372038

20382039
TEST_F(CommandLineInterfaceTest, PluginFeatureLifetimeError) {
@@ -2064,8 +2065,8 @@ TEST_F(CommandLineInterfaceTest, PluginFeatureLifetimeError) {
20642065
"foo.proto --plugin=prefix-gen-fake_plugin=",
20652066
plugin_path));
20662067
ExpectErrorSubstring(
2067-
"foo.proto:6:13: Feature pb.TestFeatures.future_feature wasn't "
2068-
"introduced until edition 2024");
2068+
"foo.proto:6:13: pb.TestFeatures.future_feature wasn't introduced "
2069+
"until edition 2024 and can't be used in edition 2023");
20692070
}
20702071

20712072
TEST_F(CommandLineInterfaceTest, GeneratorNoEditionsSupport) {

src/google/protobuf/compiler/cpp/generator_unittest.cc

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -100,8 +100,8 @@ TEST_F(CppGeneratorTest, LegacyClosedEnum) {
100100
"protocol_compiler --proto_path=$tmpdir --cpp_out=$tmpdir foo.proto");
101101

102102
ExpectWarningSubstring(
103-
"foo.proto:9:16: warning: Feature pb.CppFeatures.legacy_closed_enum has "
104-
"been deprecated in edition 2023");
103+
"foo.proto:9:16: warning: pb.CppFeatures.legacy_closed_enum has "
104+
"been deprecated in edition 2023:");
105105
}
106106

107107
TEST_F(CppGeneratorTest, LegacyClosedEnumInherited) {
@@ -123,7 +123,7 @@ TEST_F(CppGeneratorTest, LegacyClosedEnumInherited) {
123123
"protocol_compiler --proto_path=$tmpdir --cpp_out=$tmpdir foo.proto");
124124

125125
ExpectWarningSubstring(
126-
"foo.proto: warning: Feature pb.CppFeatures.legacy_closed_enum has "
126+
"foo.proto: warning: pb.CppFeatures.legacy_closed_enum has "
127127
"been deprecated in edition 2023");
128128
}
129129

src/google/protobuf/descriptor_unittest.cc

Lines changed: 15 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -12200,7 +12200,7 @@ TEST_F(FeaturesTest, DeprecatedFeature) {
1220012200
}
1220112201
}
1220212202
)pb",
12203-
"foo.proto: foo.proto: NAME: Feature "
12203+
"foo.proto: foo.proto: NAME: "
1220412204
"pb.TestFeatures.removed_feature has been deprecated in edition 2023: "
1220512205
"Custom feature deprecation warning\n");
1220612206
const FileDescriptor* file = pool_.FindFileByName("foo.proto");
@@ -12287,9 +12287,9 @@ TEST_F(FeaturesTest, RemovedFeature) {
1228712287
}
1228812288
}
1228912289
)pb",
12290-
"foo.proto: foo.proto: NAME: Feature "
12291-
"pb.TestFeatures.removed_feature has been removed in edition 2024 and "
12292-
"can't be used in edition 2024\n");
12290+
"foo.proto: foo.proto: NAME: "
12291+
"pb.TestFeatures.removed_feature has been removed in edition 2024: "
12292+
"Custom feature removal error\n");
1229312293
}
1229412294

1229512295
TEST_F(FeaturesTest, RemovedFeatureDefault) {
@@ -12319,9 +12319,9 @@ TEST_F(FeaturesTest, FutureFeature) {
1231912319
}
1232012320
}
1232112321
)pb",
12322-
"foo.proto: foo.proto: NAME: Feature "
12323-
"pb.TestFeatures.future_feature wasn't introduced until edition 2024 and "
12324-
"can't be used in edition 2023\n");
12322+
"foo.proto: foo.proto: NAME: "
12323+
"pb.TestFeatures.future_feature wasn't introduced until edition "
12324+
"2024 and can't be used in edition 2023\n");
1232512325
}
1232612326

1232712327
TEST_F(FeaturesTest, FutureFeatureDefault) {
@@ -14121,10 +14121,11 @@ TEST_F(DatabaseBackedPoolTest, FeatureLifetimeError) {
1412114121
DescriptorPool pool(&database_, &error_collector);
1412214122

1412314123
EXPECT_TRUE(pool.FindMessageTypeByName("FooFeatures") == nullptr);
14124-
EXPECT_EQ(error_collector.text_,
14125-
"features.proto: FooFeatures: NAME: Feature "
14126-
"pb.TestFeatures.future_feature wasn't introduced until edition "
14127-
"2024 and can't be used in edition 2023\n");
14124+
EXPECT_EQ(
14125+
error_collector.text_,
14126+
"features.proto: FooFeatures: NAME: "
14127+
"pb.TestFeatures.future_feature wasn't introduced until edition 2024 "
14128+
"and can't be used in edition 2023\n");
1412814129
}
1412914130

1413014131
TEST_F(DatabaseBackedPoolTest, FeatureLifetimeErrorUnknownDependencies) {
@@ -14193,9 +14194,9 @@ TEST_F(DatabaseBackedPoolTest, FeatureLifetimeErrorUnknownDependencies) {
1419314194
error_collector.text_.clear();
1419414195
ASSERT_EQ(pool.FindExtensionByName("foo_extension"), nullptr);
1419514196
EXPECT_EQ(error_collector.text_,
14196-
"option.proto: foo_extension: NAME: Feature "
14197-
"pb.TestFeatures.legacy_feature has been removed in edition 2023 "
14198-
"and can't be used in edition 2023\n");
14197+
"option.proto: foo_extension: NAME: "
14198+
"pb.TestFeatures.legacy_feature has been removed in edition 2023: "
14199+
"Custom feature removal error\n");
1419914200
}
1420014201

1420114202
TEST_F(DatabaseBackedPoolTest, DoesntRetryDbUnnecessarily) {

src/google/protobuf/feature_resolver.cc

Lines changed: 29 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
#include "absl/strings/str_join.h"
2727
#include "absl/strings/str_split.h"
2828
#include "absl/strings/string_view.h"
29+
#include "absl/strings/substitute.h"
2930
#include "absl/types/span.h"
3031
#include "google/protobuf/cpp_features.pb.h"
3132
#include "google/protobuf/descriptor.h"
@@ -364,26 +365,36 @@ absl::Status ValidateMergedFeatures(const FeatureSet& features) {
364365

365366
void ValidateSingleFeatureLifetimes(
366367
Edition edition, absl::string_view full_name,
367-
const FieldOptions::FeatureSupport& support,
368+
const FieldOptions::FeatureSupport& feature_support,
368369
FeatureResolver::ValidationResults& results) {
369370
// Skip fields that don't have feature support specified.
370-
if (&support == &FieldOptions::FeatureSupport::default_instance()) return;
371-
372-
if (edition < support.edition_introduced()) {
373-
results.errors.emplace_back(
374-
absl::StrCat("Feature ", full_name, " wasn't introduced until edition ",
375-
support.edition_introduced(),
376-
" and can't be used in edition ", edition));
377-
}
378-
if (support.has_edition_removed() && edition >= support.edition_removed()) {
379-
results.errors.emplace_back(absl::StrCat(
380-
"Feature ", full_name, " has been removed in edition ",
381-
support.edition_removed(), " and can't be used in edition ", edition));
382-
} else if (support.has_edition_deprecated() &&
383-
edition >= support.edition_deprecated()) {
384-
results.warnings.emplace_back(absl::StrCat(
385-
"Feature ", full_name, " has been deprecated in edition ",
386-
support.edition_deprecated(), ": ", support.deprecation_warning()));
371+
if (&feature_support == &FieldOptions::FeatureSupport::default_instance())
372+
return;
373+
// safe guarding new features that aren't available yet
374+
if (edition < feature_support.edition_introduced()) {
375+
std::string error_message = absl::Substitute(
376+
"$0 wasn't introduced until edition $1 and can't be used in "
377+
"edition $2",
378+
full_name, feature_support.edition_introduced(), edition);
379+
results.errors.emplace_back(std::move(error_message));
380+
}
381+
if (feature_support.has_edition_removed() &&
382+
edition >= feature_support.edition_removed()) {
383+
std::string error_message = absl::Substitute(
384+
"$0 has been removed in edition $1$2", full_name,
385+
feature_support.edition_removed(),
386+
(feature_support.has_removal_error())
387+
? absl::StrCat(": ", feature_support.removal_error())
388+
: "");
389+
results.errors.emplace_back(std::move(error_message));
390+
} else if (feature_support.has_edition_deprecated() &&
391+
edition >= feature_support.edition_deprecated()) {
392+
std::string error_message = absl::Substitute(
393+
"$0 has been deprecated in edition "
394+
"$1: $2",
395+
full_name, feature_support.edition_deprecated(),
396+
feature_support.deprecation_warning());
397+
results.warnings.emplace_back(std::move(error_message));
387398
}
388399
}
389400

src/google/protobuf/feature_resolver_test.cc

Lines changed: 26 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -634,7 +634,21 @@ TEST(FeatureResolverLifetimesTest, RemovedFeature) {
634634
features, nullptr);
635635
EXPECT_THAT(results.errors,
636636
ElementsAre(AllOf(HasSubstr("pb.TestFeatures.removed_feature"),
637-
HasSubstr("removed in edition 2024"))));
637+
HasSubstr("removed in edition 2024:"),
638+
HasSubstr("Custom feature removal error"))));
639+
EXPECT_THAT(results.warnings, IsEmpty());
640+
}
641+
642+
TEST(FeatureResolverLifetimesTest, RemovedFeatureWithNoRemovalError) {
643+
FeatureSet features = ParseTextOrDie(R"pb(
644+
[pb.test] { same_edition_removed_feature: VALUE1 }
645+
)pb");
646+
auto results = FeatureResolver::ValidateFeatureLifetimes(EDITION_2023,
647+
features, nullptr);
648+
EXPECT_THAT(results.errors,
649+
ElementsAre(AllOf(
650+
HasSubstr("pb.TestFeatures.same_edition_removed_feature"),
651+
HasSubstr("removed in edition 2023"), Not(HasSubstr(":")))));
638652
EXPECT_THAT(results.warnings, IsEmpty());
639653
}
640654

@@ -644,9 +658,11 @@ TEST(FeatureResolverLifetimesTest, NotIntroduced) {
644658
)pb");
645659
auto results = FeatureResolver::ValidateFeatureLifetimes(EDITION_2023,
646660
features, nullptr);
647-
EXPECT_THAT(results.errors,
648-
ElementsAre(AllOf(HasSubstr("pb.TestFeatures.future_feature"),
649-
HasSubstr("introduced until edition 2024"))));
661+
EXPECT_THAT(
662+
results.errors,
663+
ElementsAre(AllOf(HasSubstr("pb.TestFeatures.future_feature"),
664+
HasSubstr("wasn't introduced until edition 2024"),
665+
HasSubstr("can't be used in edition 2023"))));
650666
EXPECT_THAT(results.warnings, IsEmpty());
651667
}
652668

@@ -730,7 +746,8 @@ TEST(FeatureResolverLifetimesTest, ValueSupportBeforeIntroduced) {
730746
EXPECT_THAT(results.errors,
731747
ElementsAre(AllOf(
732748
HasSubstr("pb.VALUE_LIFETIME_FUTURE"),
733-
HasSubstr("introduced until edition 99997_TEST_ONLY"))));
749+
HasSubstr("wasn't introduced until edition 99997_TEST_ONLY"),
750+
HasSubstr("can't be used in edition 2023"))));
734751
EXPECT_THAT(results.warnings, IsEmpty());
735752
}
736753

@@ -740,10 +757,10 @@ TEST(FeatureResolverLifetimesTest, ValueSupportAfterRemoved) {
740757
)pb");
741758
auto results = FeatureResolver::ValidateFeatureLifetimes(
742759
EDITION_99997_TEST_ONLY, features, nullptr);
743-
EXPECT_THAT(
744-
results.errors,
745-
ElementsAre(AllOf(HasSubstr("pb.VALUE_LIFETIME_REMOVED"),
746-
HasSubstr("removed in edition 99997_TEST_ONLY"))));
760+
EXPECT_THAT(results.errors,
761+
ElementsAre(AllOf(HasSubstr("pb.VALUE_LIFETIME_REMOVED"),
762+
HasSubstr("removed in edition 99997_TEST_ONLY"),
763+
HasSubstr("Custom feature removal error"))));
747764
EXPECT_THAT(results.warnings, IsEmpty());
748765
}
749766

src/google/protobuf/unittest_features.proto

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -66,6 +66,7 @@ enum ValueLifetimeFeature {
6666
VALUE_LIFETIME_REMOVED = 6 [feature_support = {
6767
edition_deprecated: EDITION_2023
6868
edition_removed: EDITION_99997_TEST_ONLY
69+
removal_error: "Custom feature removal error"
6970
}];
7071
}
7172

@@ -190,12 +191,24 @@ message TestFeatures {
190191
edition_deprecated: EDITION_2023
191192
deprecation_warning: "Custom feature deprecation warning"
192193
edition_removed: EDITION_2024
194+
removal_error: "Custom feature removal error"
193195
},
194196
edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" },
195197
edition_defaults = { edition: EDITION_2023, value: "VALUE2" },
196198
edition_defaults = { edition: EDITION_2024, value: "VALUE3" }
197199
];
198200

201+
EnumFeature same_edition_removed_feature = 21 [
202+
retention = RETENTION_RUNTIME,
203+
targets = TARGET_TYPE_FILE,
204+
targets = TARGET_TYPE_FIELD,
205+
feature_support = {
206+
edition_introduced: EDITION_2023
207+
edition_removed: EDITION_2023
208+
},
209+
edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" }
210+
];
211+
199212
EnumFeature future_feature = 18 [
200213
retention = RETENTION_RUNTIME,
201214
targets = TARGET_TYPE_FILE,
@@ -212,6 +225,7 @@ message TestFeatures {
212225
feature_support = {
213226
edition_introduced: EDITION_PROTO3
214227
edition_removed: EDITION_2023
228+
removal_error: "Custom feature removal error"
215229
},
216230
edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" },
217231
edition_defaults = { edition: EDITION_2023, value: "VALUE2" }
@@ -225,6 +239,7 @@ message TestFeatures {
225239
edition_deprecated: EDITION_99998_TEST_ONLY
226240
deprecation_warning: "Custom feature deprecation warning"
227241
edition_removed: EDITION_99999_TEST_ONLY
242+
removal_error: "Custom feature removal error"
228243
},
229244
edition_defaults = {
230245
edition: EDITION_LEGACY,

0 commit comments

Comments
 (0)