From 0b6e768f07c4cde376783b964022ec7e1ff3e772 Mon Sep 17 00:00:00 2001 From: Mike Kruskal Date: Mon, 9 Sep 2024 16:16:19 -0700 Subject: [PATCH] Introduce lifetimes for individual feature values. This enables enforcement of lifetime specifications on individual enum values for features. It will allow us to add new values to existing features, as well as deprecate/and remove existing values. By default, each value will be scoped to the lifetime spec of its corresponding feature field. However, individual lifetime boundaries can be overridden at the value-level for finer grained control. In the near-term, this will allow us to deprecate/remove required field presence, and add a stricter utf8 validation feature. PiperOrigin-RevId: 672710484 --- editions/defaults_test.cc | 2 +- .../protobuf/internal/descriptor_test.py | 1 + .../command_line_interface_unittest.cc | 2 +- src/google/protobuf/descriptor_unittest.cc | 88 ++-- src/google/protobuf/feature_resolver.cc | 248 ++++++---- src/google/protobuf/feature_resolver_test.cc | 435 +++++++++++++++++- src/google/protobuf/unittest_features.proto | 84 +++- 7 files changed, 702 insertions(+), 158 deletions(-) diff --git a/editions/defaults_test.cc b/editions/defaults_test.cc index b3fa62f5e765..3e4861cd0b64 100644 --- a/editions/defaults_test.cc +++ b/editions/defaults_test.cc @@ -103,7 +103,7 @@ TEST(DefaultsTest, CheckFuture) { TEST(DefaultsTest, CheckFarFuture) { auto defaults = ReadDefaults("test_defaults_far_future"); ASSERT_OK(defaults); - ASSERT_EQ(defaults->defaults().size(), 6); + ASSERT_EQ(defaults->defaults().size(), 7); ASSERT_EQ(defaults->minimum_edition(), EDITION_99997_TEST_ONLY); ASSERT_EQ(defaults->maximum_edition(), EDITION_99999_TEST_ONLY); diff --git a/python/google/protobuf/internal/descriptor_test.py b/python/google/protobuf/internal/descriptor_test.py index 2c8423efac79..02361195a620 100755 --- a/python/google/protobuf/internal/descriptor_test.py +++ b/python/google/protobuf/internal/descriptor_test.py @@ -1474,6 +1474,7 @@ class ReturnObject: file = descriptor_pb2.FileDescriptorProto() descriptor_pb2.DESCRIPTOR.CopyToProto(file) ret.pool.Add(file) + file.Clear() unittest_features_pb2.DESCRIPTOR.CopyToProto(file) ret.pool.Add(file) diff --git a/src/google/protobuf/compiler/command_line_interface_unittest.cc b/src/google/protobuf/compiler/command_line_interface_unittest.cc index fc30827647b4..84e727f424cb 100644 --- a/src/google/protobuf/compiler/command_line_interface_unittest.cc +++ b/src/google/protobuf/compiler/command_line_interface_unittest.cc @@ -2097,7 +2097,7 @@ TEST_F(CommandLineInterfaceTest, EditionDefaultsWithExtension) { FeatureSetDefaults defaults = ReadEditionDefaults("defaults"); EXPECT_EQ(defaults.minimum_edition(), EDITION_PROTO2); EXPECT_EQ(defaults.maximum_edition(), EDITION_99999_TEST_ONLY); - ASSERT_EQ(defaults.defaults_size(), 6); + ASSERT_EQ(defaults.defaults_size(), 7); EXPECT_EQ(defaults.defaults(0).edition(), EDITION_LEGACY); EXPECT_EQ(defaults.defaults(2).edition(), EDITION_2023); EXPECT_EQ(defaults.defaults(3).edition(), EDITION_2024); diff --git a/src/google/protobuf/descriptor_unittest.cc b/src/google/protobuf/descriptor_unittest.cc index 2f224e7f7440..30d15ddee5ff 100644 --- a/src/google/protobuf/descriptor_unittest.cc +++ b/src/google/protobuf/descriptor_unittest.cc @@ -10970,7 +10970,8 @@ TEST_F(FeaturesTest, RemovedFeature) { } )pb", "foo.proto: foo.proto: NAME: Feature " - "pb.TestFeatures.removed_feature has been removed in edition 2024\n"); + "pb.TestFeatures.removed_feature has been removed in edition 2024 and " + "can't be used in edition 2024\n"); } TEST_F(FeaturesTest, RemovedFeatureDefault) { @@ -11001,7 +11002,8 @@ TEST_F(FeaturesTest, FutureFeature) { } )pb", "foo.proto: foo.proto: NAME: Feature " - "pb.TestFeatures.future_feature wasn't introduced until edition 2024\n"); + "pb.TestFeatures.future_feature wasn't introduced until edition 2024 and " + "can't be used in edition 2023\n"); } TEST_F(FeaturesTest, FutureFeatureDefault) { @@ -12381,14 +12383,20 @@ TEST_F(DatabaseBackedPoolTest, UnittestProto) { } TEST_F(DatabaseBackedPoolTest, FeatureResolution) { - FileDescriptorProto proto; - FileDescriptorProto::descriptor()->file()->CopyTo(&proto); - std::string text_proto; - google::protobuf::TextFormat::PrintToString(proto, &text_proto); - AddToDatabase(&database_, text_proto); - pb::TestFeatures::descriptor()->file()->CopyTo(&proto); - google::protobuf::TextFormat::PrintToString(proto, &text_proto); - AddToDatabase(&database_, text_proto); + { + FileDescriptorProto proto; + FileDescriptorProto::descriptor()->file()->CopyTo(&proto); + std::string text_proto; + google::protobuf::TextFormat::PrintToString(proto, &text_proto); + AddToDatabase(&database_, text_proto); + } + { + FileDescriptorProto proto; + pb::TestFeatures::descriptor()->file()->CopyTo(&proto); + std::string text_proto; + google::protobuf::TextFormat::PrintToString(proto, &text_proto); + AddToDatabase(&database_, text_proto); + } AddToDatabase(&database_, R"pb( name: "features.proto" syntax: "editions" @@ -12432,14 +12440,20 @@ TEST_F(DatabaseBackedPoolTest, FeatureResolution) { } TEST_F(DatabaseBackedPoolTest, FeatureLifetimeError) { - FileDescriptorProto proto; - FileDescriptorProto::descriptor()->file()->CopyTo(&proto); - std::string text_proto; - google::protobuf::TextFormat::PrintToString(proto, &text_proto); - AddToDatabase(&database_, text_proto); - pb::TestFeatures::descriptor()->file()->CopyTo(&proto); - google::protobuf::TextFormat::PrintToString(proto, &text_proto); - AddToDatabase(&database_, text_proto); + { + FileDescriptorProto proto; + FileDescriptorProto::descriptor()->file()->CopyTo(&proto); + std::string text_proto; + google::protobuf::TextFormat::PrintToString(proto, &text_proto); + AddToDatabase(&database_, text_proto); + } + { + FileDescriptorProto proto; + pb::TestFeatures::descriptor()->file()->CopyTo(&proto); + std::string text_proto; + google::protobuf::TextFormat::PrintToString(proto, &text_proto); + AddToDatabase(&database_, text_proto); + } AddToDatabase(&database_, R"pb( name: "features.proto" syntax: "editions" @@ -12458,21 +12472,27 @@ 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\n"); + 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"); } TEST_F(DatabaseBackedPoolTest, FeatureLifetimeErrorUnknownDependencies) { - FileDescriptorProto proto; - FileDescriptorProto::descriptor()->file()->CopyTo(&proto); - std::string text_proto; - google::protobuf::TextFormat::PrintToString(proto, &text_proto); - AddToDatabase(&database_, text_proto); - pb::TestFeatures::descriptor()->file()->CopyTo(&proto); - google::protobuf::TextFormat::PrintToString(proto, &text_proto); - AddToDatabase(&database_, text_proto); + { + FileDescriptorProto proto; + FileDescriptorProto::descriptor()->file()->CopyTo(&proto); + std::string text_proto; + google::protobuf::TextFormat::PrintToString(proto, &text_proto); + AddToDatabase(&database_, text_proto); + } + { + FileDescriptorProto proto; + pb::TestFeatures::descriptor()->file()->CopyTo(&proto); + std::string text_proto; + google::protobuf::TextFormat::PrintToString(proto, &text_proto); + AddToDatabase(&database_, text_proto); + } AddToDatabase(&database_, R"pb( name: "option.proto" syntax: "editions" @@ -12522,10 +12542,10 @@ TEST_F(DatabaseBackedPoolTest, FeatureLifetimeErrorUnknownDependencies) { // Verify that the extension does trigger a lifetime error. 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\n"); + 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"); } TEST_F(DatabaseBackedPoolTest, DoesntRetryDbUnnecessarily) { diff --git a/src/google/protobuf/feature_resolver.cc b/src/google/protobuf/feature_resolver.cc index bd46eb137e62..7bb393a6a7bc 100644 --- a/src/google/protobuf/feature_resolver.cc +++ b/src/google/protobuf/feature_resolver.cc @@ -53,6 +53,123 @@ absl::Status Error(Args... args) { return absl::FailedPreconditionError(absl::StrCat(args...)); } +absl::Status ValidateFeatureSupport(const FieldOptions::FeatureSupport& support, + absl::string_view full_name) { + if (support.has_edition_deprecated()) { + if (support.edition_deprecated() < support.edition_introduced()) { + return Error("Feature ", full_name, + " was deprecated before it was introduced."); + } + if (!support.has_deprecation_warning()) { + return Error( + "Feature ", full_name, + " is deprecated but does not specify a deprecation warning."); + } + } + if (!support.has_edition_deprecated() && support.has_deprecation_warning()) { + return Error("Feature ", full_name, + " specifies a deprecation warning but is not marked " + "deprecated in any edition."); + } + if (support.has_edition_removed()) { + if (support.edition_deprecated() >= support.edition_removed()) { + return Error("Feature ", full_name, + " was deprecated after it was removed."); + } + if (support.edition_removed() < support.edition_introduced()) { + return Error("Feature ", full_name, + " was removed before it was introduced."); + } + } + + return absl::OkStatus(); +} + +absl::Status ValidateFieldFeatureSupport(const FieldDescriptor& field) { + if (!field.options().has_feature_support()) { + return Error("Feature field ", field.full_name(), + " has no feature support specified."); + } + + const FieldOptions::FeatureSupport& support = + field.options().feature_support(); + if (!support.has_edition_introduced()) { + return Error("Feature field ", field.full_name(), + " does not specify the edition it was introduced in."); + } + RETURN_IF_ERROR(ValidateFeatureSupport(support, field.full_name())); + + // Validate edition defaults specification wrt support windows. + for (const auto& d : field.options().edition_defaults()) { + if (d.edition() < Edition::EDITION_2023) { + // Allow defaults to be specified in proto2/proto3, predating + // editions. + continue; + } + if (d.edition() < support.edition_introduced()) { + return Error("Feature field ", field.full_name(), + " has a default specified for edition ", d.edition(), + ", before it was introduced."); + } + if (support.has_edition_removed() && + d.edition() > support.edition_removed()) { + return Error("Feature field ", field.full_name(), + " has a default specified for edition ", d.edition(), + ", after it was removed."); + } + } + + return absl::OkStatus(); +} + +absl::Status ValidateValueFeatureSupport( + const FieldOptions::FeatureSupport& parent, + const EnumValueDescriptor& value, absl::string_view field_name) { + if (!value.options().has_feature_support()) { + // We allow missing support windows on feature values, and they'll inherit + // from the feature spec. + return absl::OkStatus(); + } + + FieldOptions::FeatureSupport support = parent; + support.MergeFrom(value.options().feature_support()); + RETURN_IF_ERROR(ValidateFeatureSupport(support, value.full_name())); + + // Make sure the value doesn't expand any bounds. + if (support.edition_introduced() < parent.edition_introduced()) { + return Error("Feature value ", value.full_name(), + " was introduced before feature ", field_name, " was."); + } + if (parent.has_edition_removed() && + support.edition_removed() > parent.edition_removed()) { + return Error("Feature value ", value.full_name(), + " was removed after feature ", field_name, " was."); + } + if (parent.has_edition_deprecated() && + support.edition_deprecated() > parent.edition_deprecated()) { + return Error("Feature value ", value.full_name(), + " was deprecated after feature ", field_name, " was."); + } + + return absl::OkStatus(); +} + +absl::Status ValidateValuesFeatureSupport(const FieldDescriptor& field) { + // This only applies to enum features. + ABSL_CHECK(field.enum_type() != nullptr); + + const FieldOptions::FeatureSupport& parent = + field.options().feature_support(); + + for (int i = 0; i < field.enum_type()->value_count(); ++i) { + const EnumValueDescriptor& value = *field.enum_type()->value(i); + RETURN_IF_ERROR( + ValidateValueFeatureSupport(parent, value, field.full_name())); + } + + return absl::OkStatus(); +} + absl::Status ValidateDescriptor(const Descriptor& descriptor) { if (descriptor.oneof_decl_count() > 0) { return Error("Type ", descriptor.full_name(), @@ -92,62 +209,9 @@ absl::Status ValidateDescriptor(const Descriptor& descriptor) { "was introduced."); } - if (!field.options().has_feature_support()) { - return Error("Feature field ", field.full_name(), - " has no feature support specified."); - } - - const FieldOptions::FeatureSupport& support = - field.options().feature_support(); - if (!support.has_edition_introduced()) { - return Error("Feature field ", field.full_name(), - " does not specify the edition it was introduced in."); - } - if (support.has_edition_deprecated()) { - if (!support.has_deprecation_warning()) { - return Error( - "Feature field ", field.full_name(), - " is deprecated but does not specify a deprecation warning."); - } - if (support.edition_deprecated() < support.edition_introduced()) { - return Error("Feature field ", field.full_name(), - " was deprecated before it was introduced."); - } - } - if (!support.has_edition_deprecated() && - support.has_deprecation_warning()) { - return Error("Feature field ", field.full_name(), - " specifies a deprecation warning but is not marked " - "deprecated in any edition."); - } - if (support.has_edition_removed()) { - if (support.edition_deprecated() >= support.edition_removed()) { - return Error("Feature field ", field.full_name(), - " was deprecated after it was removed."); - } - if (support.edition_removed() < support.edition_introduced()) { - return Error("Feature field ", field.full_name(), - " was removed before it was introduced."); - } - } - - for (const auto& d : field.options().edition_defaults()) { - if (d.edition() < Edition::EDITION_2023) { - // Allow defaults to be specified in proto2/proto3, predating - // editions. - continue; - } - if (d.edition() < support.edition_introduced()) { - return Error("Feature field ", field.full_name(), - " has a default specified for edition ", d.edition(), - ", before it was introduced."); - } - if (support.has_edition_removed() && - d.edition() > support.edition_removed()) { - return Error("Feature field ", field.full_name(), - " has a default specified for edition ", d.edition(), - ", after it was removed."); - } + RETURN_IF_ERROR(ValidateFieldFeatureSupport(field)); + if (field.enum_type() != nullptr) { + RETURN_IF_ERROR(ValidateValuesFeatureSupport(field)); } } @@ -293,15 +357,40 @@ absl::Status ValidateMergedFeatures(const FeatureSet& features) { return absl::OkStatus(); } -void CollectLifetimeResults(Edition edition, const Message& message, - FeatureResolver::ValidationResults& results) { +void ValidateSingleFeatureLifetimes( + Edition edition, absl::string_view full_name, + const FieldOptions::FeatureSupport& 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())); + } +} + +void ValidateFeatureLifetimesImpl(Edition edition, const Message& message, + FeatureResolver::ValidationResults& results) { std::vector fields; message.GetReflection()->ListFields(message, &fields); for (const FieldDescriptor* field : fields) { // Recurse into message extension. if (field->is_extension() && field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE) { - CollectLifetimeResults( + ValidateFeatureLifetimesImpl( edition, message.GetReflection()->GetMessage(message, field), results); continue; @@ -310,39 +399,18 @@ void CollectLifetimeResults(Edition edition, const Message& message, if (field->enum_type() != nullptr) { int number = message.GetReflection()->GetEnumValue(message, field); auto value = field->enum_type()->FindValueByNumber(number); - if (value != nullptr) { - const FieldOptions::FeatureSupport& support = - value->options().feature_support(); - if (value->options().has_feature_support() && - support.edition_introduced() > edition) { - results.errors.emplace_back( - absl::StrCat("Feature ", value->full_name(), - " wasn't introduced until edition ", - support.edition_introduced())); - } + if (value == nullptr) { + results.errors.emplace_back(absl::StrCat( + "Feature ", field->full_name(), " has no known value ", number)); + continue; } + ValidateSingleFeatureLifetimes(edition, value->full_name(), + value->options().feature_support(), + results); } - // Skip fields that don't have feature support specified. - if (!field->options().has_feature_support()) continue; - - const FieldOptions::FeatureSupport& support = - field->options().feature_support(); - if (edition < support.edition_introduced()) { - results.errors.emplace_back(absl::StrCat( - "Feature ", field->full_name(), " wasn't introduced until edition ", - support.edition_introduced())); - } - if (support.has_edition_removed() && edition >= support.edition_removed()) { - results.errors.emplace_back(absl::StrCat("Feature ", field->full_name(), - " has been removed in edition ", - support.edition_removed())); - } else if (support.has_edition_deprecated() && - edition >= support.edition_deprecated()) { - results.warnings.emplace_back(absl::StrCat( - "Feature ", field->full_name(), " has been deprecated in edition ", - support.edition_deprecated(), ": ", support.deprecation_warning())); - } + ValidateSingleFeatureLifetimes(edition, field->full_name(), + field->options().feature_support(), results); } } @@ -504,7 +572,7 @@ FeatureResolver::ValidationResults FeatureResolver::ValidateFeatureLifetimes( ABSL_CHECK(pool_features != nullptr); ValidationResults results; - CollectLifetimeResults(edition, *pool_features, results); + ValidateFeatureLifetimesImpl(edition, *pool_features, results); return results; } diff --git a/src/google/protobuf/feature_resolver_test.cc b/src/google/protobuf/feature_resolver_test.cc index 6d8fad8ca093..60b1f7b565fd 100644 --- a/src/google/protobuf/feature_resolver_test.cc +++ b/src/google/protobuf/feature_resolver_test.cc @@ -636,11 +636,16 @@ TEST(FeatureResolverLifetimesTest, MultipleErrors) { TEST(FeatureResolverLifetimesTest, DynamicPool) { DescriptorPool pool; - FileDescriptorProto file; - FileDescriptorProto::GetDescriptor()->file()->CopyTo(&file); - ASSERT_NE(pool.BuildFile(file), nullptr); - pb::TestFeatures::GetDescriptor()->file()->CopyTo(&file); - ASSERT_NE(pool.BuildFile(file), nullptr); + { + FileDescriptorProto file; + FileDescriptorProto::GetDescriptor()->file()->CopyTo(&file); + ASSERT_NE(pool.BuildFile(file), nullptr); + } + { + FileDescriptorProto file; + pb::TestFeatures::GetDescriptor()->file()->CopyTo(&file); + ASSERT_NE(pool.BuildFile(file), nullptr); + } const Descriptor* feature_set = pool.FindMessageTypeByName("google.protobuf.FeatureSet"); ASSERT_NE(feature_set, nullptr); @@ -656,9 +661,9 @@ TEST(FeatureResolverLifetimesTest, DynamicPool) { ElementsAre(HasSubstr("pb.TestFeatures.removed_feature"))); } -TEST(FeatureResolverLifetimesTest, EmptyValueSupportInvalid2023) { +TEST(FeatureResolverLifetimesTest, EmptyValueSupportValid) { FeatureSet features = ParseTextOrDie(R"pb( - [pb.test] { file_feature: VALUE_EMPTY_SUPPORT } + [pb.test] { value_lifetime_feature: VALUE_LIFETIME_EMPTY_SUPPORT } )pb"); auto results = FeatureResolver::ValidateFeatureLifetimes(EDITION_2023, features, nullptr); @@ -666,19 +671,86 @@ TEST(FeatureResolverLifetimesTest, EmptyValueSupportInvalid2023) { EXPECT_THAT(results.warnings, IsEmpty()); } -TEST(FeatureResolverLifetimesTest, ValueSupportInvalid2023) { +TEST(FeatureResolverLifetimesTest, ValueSupportValid) { + FeatureSet features = ParseTextOrDie(R"pb( + [pb.test] { value_lifetime_feature: VALUE_LIFETIME_SUPPORT } + )pb"); + auto results = FeatureResolver::ValidateFeatureLifetimes( + EDITION_99997_TEST_ONLY, features, nullptr); + EXPECT_THAT(results.errors, IsEmpty()); + EXPECT_THAT(results.warnings, IsEmpty()); +} + +TEST(FeatureResolverLifetimesTest, ValueSupportBeforeIntroduced) { FeatureSet features = ParseTextOrDie(R"pb( - [pb.test] { file_feature: VALUE_FUTURE } + [pb.test] { value_lifetime_feature: VALUE_LIFETIME_FUTURE } )pb"); auto results = FeatureResolver::ValidateFeatureLifetimes(EDITION_2023, features, nullptr); EXPECT_THAT(results.errors, ElementsAre(AllOf( - HasSubstr("pb.VALUE_FUTURE"), + HasSubstr("pb.VALUE_LIFETIME_FUTURE"), HasSubstr("introduced until edition 99997_TEST_ONLY")))); EXPECT_THAT(results.warnings, IsEmpty()); } +TEST(FeatureResolverLifetimesTest, ValueSupportAfterRemoved) { + FeatureSet features = ParseTextOrDie(R"pb( + [pb.test] { value_lifetime_feature: VALUE_LIFETIME_REMOVED } + )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.warnings, IsEmpty()); +} + +TEST(FeatureResolverLifetimesTest, ValueSupportDeprecated) { + FeatureSet features = ParseTextOrDie(R"pb( + [pb.test] { value_lifetime_feature: VALUE_LIFETIME_DEPRECATED } + )pb"); + auto results = FeatureResolver::ValidateFeatureLifetimes( + EDITION_99997_TEST_ONLY, features, nullptr); + EXPECT_THAT(results.errors, IsEmpty()); + EXPECT_THAT( + results.warnings, + ElementsAre(AllOf(HasSubstr("pb.VALUE_LIFETIME_DEPRECATED"), + HasSubstr("deprecated in edition 99997_TEST_ONLY"), + HasSubstr("Custom feature deprecation warning")))); +} + +TEST(FeatureResolverLifetimesTest, ValueAndFeatureSupportDeprecated) { + FeatureSet features = ParseTextOrDie(R"pb( + [pb.test] { value_lifetime_feature: VALUE_LIFETIME_DEPRECATED } + )pb"); + auto results = FeatureResolver::ValidateFeatureLifetimes( + EDITION_99998_TEST_ONLY, features, nullptr); + EXPECT_THAT(results.errors, IsEmpty()); + EXPECT_THAT(results.warnings, + UnorderedElementsAre( + AllOf(HasSubstr("pb.VALUE_LIFETIME_DEPRECATED"), + HasSubstr("deprecated in edition 99997_TEST_ONLY"), + HasSubstr("Custom feature deprecation warning")), + AllOf(HasSubstr("pb.TestFeatures.value_lifetime_feature"), + HasSubstr("deprecated in edition 99998_TEST_ONLY"), + HasSubstr("Custom feature deprecation warning")))); +} + +TEST(FeatureResolverLifetimesTest, ValueSupportInvalidNumber) { + FeatureSet features; + features.MutableExtension(pb::test)->set_value_lifetime_feature( + static_cast(1234)); + auto results = FeatureResolver::ValidateFeatureLifetimes(EDITION_2023, + features, nullptr); + EXPECT_THAT( + results.errors, + ElementsAre(AllOf(HasSubstr("pb.TestFeatures.value_lifetime_feature"), + HasSubstr("1234")))); + EXPECT_THAT(results.warnings, IsEmpty()); +} + class FakeErrorCollector : public io::ErrorCollector { public: FakeErrorCollector() = default; @@ -1284,6 +1356,349 @@ TEST_F(FeatureResolverPoolTest, CompileDefaultsInvalidDefaultsTooEarly) { HasError(HasSubstr("Minimum edition 2_TEST_ONLY is not EDITION_LEGACY"))); } +TEST_F(FeatureResolverPoolTest, + CompileDefaultsInvalidValueWithMissingDeprecationWarning) { + const FileDescriptor* file = ParseSchema(R"schema( + syntax = "proto2"; + package test; + import "google/protobuf/descriptor.proto"; + + extend google.protobuf.FeatureSet { + optional Foo bar = 9999; + } + enum FooValues { + UNKNOWN = 0; + VALUE = 1 [feature_support.edition_deprecated = EDITION_2023]; + } + message Foo { + optional FooValues bool_field = 1 [ + targets = TARGET_TYPE_FIELD, + feature_support.edition_introduced = EDITION_2023, + edition_defaults = { edition: EDITION_LEGACY, value: "UNKNOWN" } + ]; + } + )schema"); + ASSERT_NE(file, nullptr); + + const FieldDescriptor* ext = file->extension(0); + EXPECT_THAT(FeatureResolver::CompileDefaults(feature_set_, {ext}, + EDITION_2023, EDITION_2023), + HasError(AllOf(HasSubstr("test.VALUE"), + HasSubstr("deprecation warning")))); +} + +TEST_F(FeatureResolverPoolTest, + CompileDefaultsInvalidValueWithMissingDeprecation) { + const FileDescriptor* file = ParseSchema(R"schema( + syntax = "proto2"; + package test; + import "google/protobuf/descriptor.proto"; + + extend google.protobuf.FeatureSet { + optional Foo bar = 9999; + } + enum FooValues { + UNKNOWN = 0; + VALUE = 1 [feature_support.deprecation_warning = "some message"]; + } + message Foo { + optional FooValues bool_field = 1 [ + targets = TARGET_TYPE_FIELD, + feature_support.edition_introduced = EDITION_2023, + edition_defaults = { edition: EDITION_LEGACY, value: "UNKNOWN" } + ]; + } + )schema"); + ASSERT_NE(file, nullptr); + + const FieldDescriptor* ext = file->extension(0); + EXPECT_THAT(FeatureResolver::CompileDefaults(feature_set_, {ext}, + EDITION_2023, EDITION_2023), + HasError(AllOf(HasSubstr("test.VALUE"), + HasSubstr("is not marked deprecated")))); +} + +TEST_F(FeatureResolverPoolTest, + CompileDefaultsInvalidValueDeprecatedBeforeIntroduced) { + const FileDescriptor* file = ParseSchema(R"schema( + syntax = "proto2"; + package test; + import "google/protobuf/descriptor.proto"; + + extend google.protobuf.FeatureSet { + optional Foo bar = 9999; + } + enum FooValues { + UNKNOWN = 0; + VALUE = 1 [feature_support = { + edition_introduced: EDITION_2024 + edition_deprecated: EDITION_2023 + deprecation_warning: "warning" + }]; + } + message Foo { + optional FooValues bool_field = 1 [ + targets = TARGET_TYPE_FIELD, + feature_support.edition_introduced = EDITION_2023, + edition_defaults = { edition: EDITION_LEGACY, value: "UNKNOWN" } + ]; + } + )schema"); + ASSERT_NE(file, nullptr); + + const FieldDescriptor* ext = file->extension(0); + EXPECT_THAT( + FeatureResolver::CompileDefaults(feature_set_, {ext}, EDITION_2023, + EDITION_2023), + HasError(AllOf(HasSubstr("test.VALUE"), + HasSubstr("deprecated before it was introduced")))); +} + +TEST_F(FeatureResolverPoolTest, + CompileDefaultsInvalidValueDeprecatedBeforeIntroducedInherited) { + const FileDescriptor* file = ParseSchema(R"schema( + syntax = "proto2"; + package test; + import "google/protobuf/descriptor.proto"; + + extend google.protobuf.FeatureSet { + optional Foo bar = 9999; + } + enum FooValues { + UNKNOWN = 0; + VALUE = 1 [feature_support = { + edition_deprecated: EDITION_2023 + deprecation_warning: "warning" + }]; + } + message Foo { + optional FooValues bool_field = 1 [ + targets = TARGET_TYPE_FIELD, + feature_support.edition_introduced = EDITION_2024, + edition_defaults = { edition: EDITION_LEGACY, value: "UNKNOWN" } + ]; + } + )schema"); + ASSERT_NE(file, nullptr); + + const FieldDescriptor* ext = file->extension(0); + EXPECT_THAT( + FeatureResolver::CompileDefaults(feature_set_, {ext}, EDITION_2023, + EDITION_2023), + HasError(AllOf(HasSubstr("test.VALUE"), + HasSubstr("deprecated before it was introduced")))); +} + +TEST_F(FeatureResolverPoolTest, + CompileDefaultsInvalidValueDeprecatedAfterRemoved) { + const FileDescriptor* file = ParseSchema(R"schema( + syntax = "proto2"; + package test; + import "google/protobuf/descriptor.proto"; + + extend google.protobuf.FeatureSet { + optional Foo bar = 9999; + } + enum FooValues { + UNKNOWN = 0; + VALUE = 1 [feature_support = { + edition_introduced: EDITION_2023 + edition_deprecated: EDITION_2024 + deprecation_warning: "warning" + edition_removed: EDITION_2024 + }]; + } + message Foo { + optional FooValues bool_field = 1 [ + targets = TARGET_TYPE_FIELD, + feature_support.edition_introduced = EDITION_2023, + edition_defaults = { edition: EDITION_LEGACY, value: "UNKNOWN" } + ]; + } + )schema"); + ASSERT_NE(file, nullptr); + + const FieldDescriptor* ext = file->extension(0); + EXPECT_THAT(FeatureResolver::CompileDefaults(feature_set_, {ext}, + EDITION_2023, EDITION_2023), + HasError(AllOf(HasSubstr("test.VALUE"), + HasSubstr("deprecated after it was removed")))); +} + +TEST_F(FeatureResolverPoolTest, + CompileDefaultsInvalidValueRemovedBeforeIntroduced) { + const FileDescriptor* file = ParseSchema(R"schema( + syntax = "proto2"; + package test; + import "google/protobuf/descriptor.proto"; + + extend google.protobuf.FeatureSet { + optional Foo bar = 9999; + } + enum FooValues { + UNKNOWN = 0; + VALUE = 1 [feature_support = { + edition_introduced: EDITION_2024 + edition_removed: EDITION_2023 + }]; + } + message Foo { + optional FooValues bool_field = 1 [ + targets = TARGET_TYPE_FIELD, + feature_support.edition_introduced = EDITION_2023, + edition_defaults = { edition: EDITION_LEGACY, value: "UNKNOWN" } + ]; + } + )schema"); + ASSERT_NE(file, nullptr); + + const FieldDescriptor* ext = file->extension(0); + EXPECT_THAT(FeatureResolver::CompileDefaults(feature_set_, {ext}, + EDITION_2023, EDITION_2023), + HasError(AllOf(HasSubstr("test.VALUE"), + HasSubstr("removed before it was introduced")))); +} + +TEST_F(FeatureResolverPoolTest, + CompileDefaultsInvalidValueIntroducedBeforeFeature) { + const FileDescriptor* file = ParseSchema(R"schema( + syntax = "proto2"; + package test; + import "google/protobuf/descriptor.proto"; + + extend google.protobuf.FeatureSet { + optional Foo bar = 9999; + } + enum FooValues { + UNKNOWN = 0; + VALUE = 1 [feature_support = { + edition_introduced: EDITION_2023 + }]; + } + message Foo { + optional FooValues bool_field = 1 [ + targets = TARGET_TYPE_FIELD, + feature_support.edition_introduced = EDITION_2024, + edition_defaults = { edition: EDITION_LEGACY, value: "UNKNOWN" } + ]; + } + )schema"); + ASSERT_NE(file, nullptr); + + const FieldDescriptor* ext = file->extension(0); + EXPECT_THAT( + FeatureResolver::CompileDefaults(feature_set_, {ext}, EDITION_2023, + EDITION_2023), + HasError(AllOf(HasSubstr("test.VALUE"), HasSubstr("introduced before"), + HasSubstr("test.Foo.bool_field")))); +} + +TEST_F(FeatureResolverPoolTest, + CompileDefaultsInvalidValueIntroducedAfterFeatureRemoved) { + const FileDescriptor* file = ParseSchema(R"schema( + syntax = "proto2"; + package test; + import "google/protobuf/descriptor.proto"; + + extend google.protobuf.FeatureSet { + optional Foo bar = 9999; + } + enum FooValues { + UNKNOWN = 0; + VALUE = 1 [feature_support = { + edition_introduced: EDITION_99997_TEST_ONLY + }]; + } + message Foo { + optional FooValues bool_field = 1 [ + targets = TARGET_TYPE_FIELD, + feature_support.edition_introduced = EDITION_2023, + feature_support.edition_removed = EDITION_2024, + edition_defaults = { edition: EDITION_LEGACY, value: "UNKNOWN" } + ]; + } + )schema"); + ASSERT_NE(file, nullptr); + + const FieldDescriptor* ext = file->extension(0); + EXPECT_THAT(FeatureResolver::CompileDefaults(feature_set_, {ext}, + EDITION_2023, EDITION_2023), + HasError(AllOf(HasSubstr("test.VALUE"), + HasSubstr("removed before it was introduced")))); +} + +TEST_F(FeatureResolverPoolTest, + CompileDefaultsInvalidValueRemovedAfterFeature) { + const FileDescriptor* file = ParseSchema(R"schema( + syntax = "proto2"; + package test; + import "google/protobuf/descriptor.proto"; + + extend google.protobuf.FeatureSet { + optional Foo bar = 9999; + } + enum FooValues { + UNKNOWN = 0; + VALUE = 1 [feature_support = { + edition_removed: EDITION_99997_TEST_ONLY + }]; + } + message Foo { + optional FooValues bool_field = 1 [ + targets = TARGET_TYPE_FIELD, + feature_support.edition_introduced = EDITION_2023, + feature_support.edition_removed = EDITION_2024, + edition_defaults = { edition: EDITION_LEGACY, value: "UNKNOWN" } + ]; + } + )schema"); + ASSERT_NE(file, nullptr); + + const FieldDescriptor* ext = file->extension(0); + EXPECT_THAT( + FeatureResolver::CompileDefaults(feature_set_, {ext}, EDITION_2023, + EDITION_2023), + HasError(AllOf(HasSubstr("test.VALUE"), HasSubstr("removed after"), + HasSubstr("test.Foo.bool_field")))); +} + +TEST_F(FeatureResolverPoolTest, + CompileDefaultsInvalidValueDeprecatedAfterFeature) { + const FileDescriptor* file = ParseSchema(R"schema( + syntax = "proto2"; + package test; + import "google/protobuf/descriptor.proto"; + + extend google.protobuf.FeatureSet { + optional Foo bar = 9999; + } + enum FooValues { + UNKNOWN = 0; + VALUE = 1 [feature_support = { + edition_deprecated: EDITION_99997_TEST_ONLY + deprecation_warning: "warning" + }]; + } + message Foo { + optional FooValues bool_field = 1 [ + targets = TARGET_TYPE_FIELD, + feature_support.edition_introduced = EDITION_2023, + feature_support.edition_deprecated = EDITION_2024, + feature_support.deprecation_warning = "warning", + edition_defaults = { edition: EDITION_LEGACY, value: "UNKNOWN" } + ]; + } + )schema"); + ASSERT_NE(file, nullptr); + + const FieldDescriptor* ext = file->extension(0); + EXPECT_THAT( + FeatureResolver::CompileDefaults(feature_set_, {ext}, EDITION_2023, + EDITION_2023), + HasError(AllOf(HasSubstr("test.VALUE"), HasSubstr("deprecated after"), + HasSubstr("test.Foo.bool_field")))); +} + TEST_F(FeatureResolverPoolTest, CompileDefaultsMinimumTooEarly) { const FileDescriptor* file = ParseSchema(R"schema( syntax = "proto2"; diff --git a/src/google/protobuf/unittest_features.proto b/src/google/protobuf/unittest_features.proto index ddf510c6b6ce..b4fef7606bf5 100644 --- a/src/google/protobuf/unittest_features.proto +++ b/src/google/protobuf/unittest_features.proto @@ -5,23 +5,23 @@ // license that can be found in the LICENSE file or at // https://developers.google.com/open-source/licenses/bsd -syntax = "proto2"; +edition = "2023"; package pb; import "google/protobuf/descriptor.proto"; extend google.protobuf.FeatureSet { - optional TestFeatures test = 9999; + TestFeatures test = 9999; } message TestMessage { extend google.protobuf.FeatureSet { - optional TestFeatures test_message = 9998; + TestFeatures test_message = 9998; } message Nested { extend google.protobuf.FeatureSet { - optional TestFeatures test_nested = 9997; + TestFeatures test_nested = 9997; } } } @@ -43,17 +43,32 @@ enum EnumFeature { VALUE13 = 13; VALUE14 = 14; VALUE15 = 15; - VALUE_EMPTY_SUPPORT = 98 [feature_support = {}]; - VALUE_FUTURE = 99 [feature_support = { +} + +enum ValueLifetimeFeature { + TEST_VALUE_LIFETIME_UNKNOWN = 0; + VALUE_LIFETIME_INHERITED = 1; + VALUE_LIFETIME_SUPPORT = 2 [feature_support = { edition_introduced: EDITION_99997_TEST_ONLY edition_deprecated: EDITION_99998_TEST_ONLY deprecation_warning: "Custom feature deprecation warning" edition_removed: EDITION_99999_TEST_ONLY }]; + VALUE_LIFETIME_EMPTY_SUPPORT = 3 [feature_support = {}]; + VALUE_LIFETIME_FUTURE = 4 + [feature_support.edition_introduced = EDITION_99997_TEST_ONLY]; + VALUE_LIFETIME_DEPRECATED = 5 [feature_support = { + edition_deprecated: EDITION_99997_TEST_ONLY + deprecation_warning: "Custom feature deprecation warning" + }]; + VALUE_LIFETIME_REMOVED = 6 [feature_support = { + edition_deprecated: EDITION_2023 + edition_removed: EDITION_99997_TEST_ONLY + }]; } message TestFeatures { - optional EnumFeature file_feature = 1 [ + EnumFeature file_feature = 1 [ retention = RETENTION_RUNTIME, targets = TARGET_TYPE_FILE, feature_support.edition_introduced = EDITION_2023, @@ -63,55 +78,55 @@ message TestFeatures { edition_defaults = { edition: EDITION_99997_TEST_ONLY, value: "VALUE4" }, edition_defaults = { edition: EDITION_99998_TEST_ONLY, value: "VALUE5" } ]; - optional EnumFeature extension_range_feature = 2 [ + EnumFeature extension_range_feature = 2 [ retention = RETENTION_RUNTIME, targets = TARGET_TYPE_EXTENSION_RANGE, feature_support.edition_introduced = EDITION_2023, edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" } ]; - optional EnumFeature message_feature = 3 [ + EnumFeature message_feature = 3 [ retention = RETENTION_RUNTIME, targets = TARGET_TYPE_MESSAGE, feature_support.edition_introduced = EDITION_2023, edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" } ]; - optional EnumFeature field_feature = 4 [ + EnumFeature field_feature = 4 [ retention = RETENTION_RUNTIME, targets = TARGET_TYPE_FIELD, feature_support.edition_introduced = EDITION_2023, edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" } ]; - optional EnumFeature oneof_feature = 5 [ + EnumFeature oneof_feature = 5 [ retention = RETENTION_RUNTIME, targets = TARGET_TYPE_ONEOF, feature_support.edition_introduced = EDITION_2023, edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" } ]; - optional EnumFeature enum_feature = 6 [ + EnumFeature enum_feature = 6 [ retention = RETENTION_RUNTIME, targets = TARGET_TYPE_ENUM, feature_support.edition_introduced = EDITION_2023, edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" } ]; - optional EnumFeature enum_entry_feature = 7 [ + EnumFeature enum_entry_feature = 7 [ retention = RETENTION_RUNTIME, targets = TARGET_TYPE_ENUM_ENTRY, feature_support.edition_introduced = EDITION_2023, edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" } ]; - optional EnumFeature service_feature = 8 [ + EnumFeature service_feature = 8 [ retention = RETENTION_RUNTIME, targets = TARGET_TYPE_SERVICE, feature_support.edition_introduced = EDITION_2023, edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" } ]; - optional EnumFeature method_feature = 9 [ + EnumFeature method_feature = 9 [ retention = RETENTION_RUNTIME, targets = TARGET_TYPE_METHOD, feature_support.edition_introduced = EDITION_2023, edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" } ]; - optional EnumFeature multiple_feature = 10 [ + EnumFeature multiple_feature = 10 [ retention = RETENTION_RUNTIME, targets = TARGET_TYPE_FILE, targets = TARGET_TYPE_FIELD, @@ -126,7 +141,7 @@ message TestFeatures { edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" } ]; - optional bool bool_field_feature = 11 [ + bool bool_field_feature = 11 [ retention = RETENTION_RUNTIME, targets = TARGET_TYPE_FIELD, feature_support.edition_introduced = EDITION_2023, @@ -134,7 +149,7 @@ message TestFeatures { edition_defaults = { edition: EDITION_99997_TEST_ONLY, value: "true" } ]; - optional EnumFeature source_feature = 15 [ + EnumFeature source_feature = 15 [ retention = RETENTION_SOURCE, targets = TARGET_TYPE_FILE, targets = TARGET_TYPE_FIELD, @@ -149,7 +164,7 @@ message TestFeatures { edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" } ]; - optional EnumFeature source_feature2 = 16 [ + EnumFeature source_feature2 = 16 [ retention = RETENTION_SOURCE, targets = TARGET_TYPE_FILE, targets = TARGET_TYPE_FIELD, @@ -164,7 +179,7 @@ message TestFeatures { edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" } ]; - optional EnumFeature removed_feature = 17 [ + EnumFeature removed_feature = 17 [ retention = RETENTION_RUNTIME, targets = TARGET_TYPE_FILE, targets = TARGET_TYPE_FIELD, @@ -179,7 +194,7 @@ message TestFeatures { edition_defaults = { edition: EDITION_2024, value: "VALUE3" } ]; - optional EnumFeature future_feature = 18 [ + EnumFeature future_feature = 18 [ retention = RETENTION_RUNTIME, targets = TARGET_TYPE_FILE, targets = TARGET_TYPE_FIELD, @@ -188,7 +203,7 @@ message TestFeatures { edition_defaults = { edition: EDITION_2024, value: "VALUE2" } ]; - optional EnumFeature legacy_feature = 19 [ + EnumFeature legacy_feature = 19 [ retention = RETENTION_RUNTIME, targets = TARGET_TYPE_FILE, targets = TARGET_TYPE_FIELD, @@ -199,4 +214,29 @@ message TestFeatures { edition_defaults = { edition: EDITION_LEGACY, value: "VALUE1" }, edition_defaults = { edition: EDITION_2023, value: "VALUE2" } ]; + + ValueLifetimeFeature value_lifetime_feature = 20 [ + retention = RETENTION_RUNTIME, + targets = TARGET_TYPE_FILE, + feature_support = { + edition_introduced: EDITION_2023 + edition_deprecated: EDITION_99998_TEST_ONLY + deprecation_warning: "Custom feature deprecation warning" + edition_removed: EDITION_99999_TEST_ONLY + }, + edition_defaults = { + edition: EDITION_LEGACY, + value: "VALUE_LIFETIME_INHERITED" + }, + // Verify edition defaults can use future values. + edition_defaults = { + edition: EDITION_2023, + value: "VALUE_LIFETIME_FUTURE" + }, + // Verify edition defaults can use removed values. + edition_defaults = { + edition: EDITION_99999_TEST_ONLY, + value: "VALUE_LIFETIME_FUTURE" + } + ]; }