Automated rollback of commit 138b748279110e99082118561e572d7212e59ac1.
PiperOrigin-RevId: 843860118
diff --git a/src/google/protobuf/compiler/command_line_interface_unittest.cc b/src/google/protobuf/compiler/command_line_interface_unittest.cc
index e4c9557..846507c 100644
--- a/src/google/protobuf/compiler/command_line_interface_unittest.cc
+++ b/src/google/protobuf/compiler/command_line_interface_unittest.cc
@@ -2631,7 +2631,8 @@
Run("protocol_compiler --proto_path=$tmpdir "
"foo.proto --test_out=$tmpdir --experimental_editions");
ExpectErrorSubstring(
- "The `java_multiple_files` behavior is enabled by default");
+ "google.protobuf.FileOptions.java_multiple_files has been removed in edition "
+ "2024: This behavior is enabled by default");
}
@@ -2645,7 +2646,8 @@
Run("protocol_compiler --proto_path=$tmpdir "
"foo.proto --test_out=$tmpdir --experimental_editions");
ExpectErrorSubstring(
- "The `java_multiple_files` behavior is enabled by default");
+ "google.protobuf.FileOptions.java_multiple_files has been removed in edition "
+ "2024: This behavior is enabled by default");
}
diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc
index 2931c67..be1febc 100644
--- a/src/google/protobuf/descriptor.cc
+++ b/src/google/protobuf/descriptor.cc
@@ -1611,8 +1611,9 @@
}
struct LifetimesInfo {
- const FeatureSet* proto_features;
+ const Message* option_to_validate; // features or options to validate
const Message* proto;
+ bool is_feature;
absl::string_view full_name;
absl::string_view filename;
};
@@ -1640,16 +1641,19 @@
if (lifetimes_info_map_.empty()) return true;
static absl::string_view feature_set_name = "google.protobuf.FeatureSet";
- const Descriptor* feature_set =
- pool_->FindMessageTypeByName(feature_set_name);
-
bool has_errors = false;
for (const auto& it : lifetimes_info_map_) {
const FileDescriptor* file = it.first;
for (const auto& info : it.second) {
+ const Descriptor* feature_set_descriptor = nullptr;
+ // TODO: Support custom options
+ if (info.is_feature) {
+ feature_set_descriptor =
+ pool_->FindMessageTypeByName(feature_set_name);
+ }
auto results = FeatureResolver::ValidateFeatureLifetimes(
- file->edition(), *info.proto_features, feature_set);
+ file->edition(), *info.option_to_validate, feature_set_descriptor);
for (const auto& error : results.errors) {
has_errors = true;
if (error_collector_ == nullptr) {
@@ -6227,6 +6231,11 @@
}
}
+template <typename OptionT>
+bool IsDefaultInstance(const OptionT& opt) {
+ return &opt == &OptionT::default_instance();
+}
+
const FileDescriptor* DescriptorBuilder::BuildFile(
const FileDescriptorProto& proto) {
// Ensure the generated pool has been lazily initialized. This is most
@@ -6685,10 +6694,17 @@
if (!had_errors_ && !pool_->lazily_build_dependencies_) {
internal::VisitDescriptors(
*result, proto, [&](const auto& descriptor, const auto& desc_proto) {
- if (descriptor.proto_features_ != &FeatureSet::default_instance()) {
+ if (!IsDefaultInstance(*descriptor.proto_features_)) {
deferred_validation_.ValidateFeatureLifetimes(
- GetFile(descriptor), {descriptor.proto_features_, &desc_proto,
- GetFullName(descriptor), proto.name()});
+ GetFile(descriptor),
+ {descriptor.proto_features_, &desc_proto, /*is_feature=*/true,
+ GetFullName(descriptor), proto.name()});
+ }
+ if (!IsDefaultInstance(*descriptor.options_)) {
+ deferred_validation_.ValidateFeatureLifetimes(
+ GetFile(descriptor),
+ {descriptor.options_, &desc_proto,
+ /*is_feature=*/false, GetFullName(descriptor), proto.name()});
}
});
}
@@ -8394,13 +8410,6 @@
}
if (file->edition() >= Edition::EDITION_2024) {
- if (file->options().has_java_multiple_files()) {
- AddError(file->name(), proto, DescriptorPool::ErrorCollector::OPTION_NAME,
- "The `java_multiple_files` behavior is enabled by default in "
- "editions 2024 and above. To disable it, you can set "
- "`features.(pb.java).nest_in_file_class = YES` on individual "
- "messages, enums, or services.");
- }
if (file->weak_dependency_count() > 0) {
AddError("weak", proto, DescriptorPool::ErrorCollector::IMPORT,
"weak imports are not allowed under edition 2024 and beyond.");
diff --git a/src/google/protobuf/descriptor_unittest.cc b/src/google/protobuf/descriptor_unittest.cc
index 6ab352c..b35b00c 100644
--- a/src/google/protobuf/descriptor_unittest.cc
+++ b/src/google/protobuf/descriptor_unittest.cc
@@ -12323,6 +12323,49 @@
"Custom feature removal error\n");
}
+TEST_F(FeaturesTest, RemovedOption) {
+ BuildDescriptorMessagesInTestPool();
+ BuildFileWithErrors(
+ R"pb(
+ name: "foo.proto"
+ syntax: "editions"
+ edition: EDITION_2024
+ options { java_multiple_files: true }
+ )pb",
+ "foo.proto: foo.proto: NAME: "
+ "google.protobuf.FileOptions.java_multiple_files has been removed in edition "
+ "2024: This behavior is enabled by default in "
+ "editions 2024 and above. To disable it, you can set "
+ "`features.(pb.java).nest_in_file_class = YES` on individual messages, "
+ "enums, or services.\n");
+}
+
+TEST_F(FeaturesTest, RemoveOptionAndFeature) {
+ BuildDescriptorMessagesInTestPool();
+ BuildFileInTestPool(pb::TestFeatures::descriptor()->file());
+ BuildFileWithErrors(
+ R"pb(
+ name: "foo.proto"
+ syntax: "editions"
+ edition: EDITION_2024
+ dependency: "google/protobuf/unittest_features.proto"
+ options {
+ java_multiple_files: true
+ features {
+ [pb.test] { removed_feature: VALUE9 }
+ }
+ }
+ )pb",
+ "foo.proto: foo.proto: NAME: "
+ "pb.TestFeatures.removed_feature has been removed in edition 2024: "
+ "Custom feature removal error\n"
+ "foo.proto: foo.proto: NAME: google.protobuf.FileOptions.java_multiple_files has "
+ "been removed in edition 2024: This behavior is enabled by default in "
+ "editions 2024 and above. To disable it, you can set "
+ "`features.(pb.java).nest_in_file_class = YES` on individual messages, "
+ "enums, or services.\n");
+}
+
TEST_F(FeaturesTest, RemovedFeatureDefault) {
BuildDescriptorMessagesInTestPool();
BuildFileInTestPool(pb::TestFeatures::descriptor()->file());
diff --git a/src/google/protobuf/feature_resolver.cc b/src/google/protobuf/feature_resolver.cc
index 1ea9047..48f4e43 100644
--- a/src/google/protobuf/feature_resolver.cc
+++ b/src/google/protobuf/feature_resolver.cc
@@ -399,32 +399,40 @@
}
void ValidateFeatureLifetimesImpl(Edition edition, const Message& message,
- FeatureResolver::ValidationResults& results) {
+ FeatureResolver::ValidationResults& results,
+ bool is_feature) {
std::vector<const FieldDescriptor*> 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) {
- ValidateFeatureLifetimesImpl(
- edition, message.GetReflection()->GetMessage(message, field),
- results);
- continue;
- }
-
- if (field->enum_type() != nullptr) {
- int number = message.GetReflection()->GetEnumValue(message, field);
- auto value = field->enum_type()->FindValueByNumber(number);
- if (value == nullptr) {
- results.errors.emplace_back(absl::StrCat(
- "Feature ", field->full_name(), " has no known value ", number));
+ // TODO: Support repeated option enum values and custom options
+ // feature support
+ if (is_feature) {
+ // Recurse into message extension.
+ if (field->is_extension() &&
+ field->cpp_type() == FieldDescriptor::CPPTYPE_MESSAGE) {
+ ValidateFeatureLifetimesImpl(
+ edition, message.GetReflection()->GetMessage(message, field),
+ results, is_feature);
continue;
}
- ValidateSingleFeatureLifetimes(edition, value->full_name(),
- value->options().feature_support(),
- results);
- }
+ if (field->enum_type() != nullptr) {
+ int number = message.GetReflection()->GetEnumValue(message, field);
+ auto value = field->enum_type()->FindValueByNumber(number);
+ 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);
+ }
+ }
+ // TODO: Support custom options
+ if (field->is_extension()) {
+ continue;
+ }
ValidateSingleFeatureLifetimes(edition, field->full_name(),
field->options().feature_support(), results);
}
@@ -558,30 +566,34 @@
}
FeatureResolver::ValidationResults FeatureResolver::ValidateFeatureLifetimes(
- Edition edition, const FeatureSet& features,
- const Descriptor* pool_descriptor) {
- const Message* pool_features = nullptr;
+ Edition edition, const Message& option, const Descriptor* pool_descriptor) {
+ const Message* pool_option = nullptr;
DynamicMessageFactory factory;
- std::unique_ptr<Message> features_storage;
- if (pool_descriptor == nullptr) {
- // The FeatureSet descriptor can be null if no custom extensions are defined
- // in any transitive dependency. In this case, we can just use the
- // generated pool for validation, since there wouldn't be any feature
- // extensions defined anyway.
- pool_features = &features;
- } else {
- // Move the features back to the current pool so that we can reflect on any
- // extensions.
- features_storage =
+ std::unique_ptr<Message> message_storage;
+ bool is_feature =
+ option.GetDescriptor()->name() == FeatureSet::descriptor()->name();
+ // TODO: Support custom options
+ if (pool_descriptor != nullptr && is_feature) {
+ // Move the messages back to the current pool so that we can reflect on
+ // any extensions.
+ message_storage =
absl::WrapUnique(factory.GetPrototype(pool_descriptor)->New());
- // TODO: Remove this suppression.
- (void)features_storage->ParseFromString(features.SerializeAsString());
- pool_features = features_storage.get();
+ // TODO: Remove this suppression
+ (void)message_storage->ParseFromString(option.SerializeAsString());
+ pool_option = message_storage.get();
+ } else {
+ // The Message descriptor can be null if no custom extensions are
+ // defined in any transitive dependency. In this case, we can just use
+ // the generated pool for validation, since there wouldn't be any feature
+ // extensions defined anyway.
+ pool_option = &option;
}
- ABSL_CHECK(pool_features != nullptr);
+ ABSL_CHECK(pool_option != nullptr);
ValidationResults results;
- ValidateFeatureLifetimesImpl(edition, *pool_features, results);
+ // Validate feature support
+ ValidateFeatureLifetimesImpl(edition, *pool_option, results, is_feature);
+
return results;
}
diff --git a/src/google/protobuf/feature_resolver.h b/src/google/protobuf/feature_resolver.h
index d66c156..985e0bf 100644
--- a/src/google/protobuf/feature_resolver.h
+++ b/src/google/protobuf/feature_resolver.h
@@ -70,7 +70,7 @@
std::vector<std::string> warnings;
};
static ValidationResults ValidateFeatureLifetimes(
- Edition edition, const FeatureSet& features,
+ Edition edition, const Message& option,
const Descriptor* pool_descriptor);
private:
@@ -91,4 +91,3 @@
#include "google/protobuf/port_undef.inc"
#endif // GOOGLE_PROTOBUF_FEATURE_RESOLVER_H__
-
diff --git a/src/google/protobuf/feature_resolver_test.cc b/src/google/protobuf/feature_resolver_test.cc
index 6d5c658..131f939 100644
--- a/src/google/protobuf/feature_resolver_test.cc
+++ b/src/google/protobuf/feature_resolver_test.cc
@@ -10,6 +10,7 @@
#include <utility>
#include <vector>
+#include "google/protobuf/descriptor.pb.h"
#include <gmock/gmock.h>
#include <gtest/gtest.h>
#include "absl/log/absl_check.h"
@@ -598,7 +599,7 @@
EXPECT_THAT(edition_2023_feature, HasError(HasSubstr("No valid default")));
}
-TEST(FeatureResolverLifetimesTest, Valid) {
+TEST(FeatureResolverLifetimesTest, ValidFeature) {
FeatureSet features = ParseTextOrDie(R"pb(
[pb.test] { file_feature: VALUE1 }
)pb");
@@ -618,6 +619,16 @@
EXPECT_THAT(results.warnings, IsEmpty());
}
+TEST(FeatureResolverLifetimesTest, ValidOption) {
+ FileOptions options = ParseTextOrDie(R"pb(
+ java_multiple_files: true
+ )pb");
+ auto results =
+ FeatureResolver::ValidateFeatureLifetimes(EDITION_2023, options, nullptr);
+ EXPECT_THAT(results.errors, IsEmpty());
+ EXPECT_THAT(results.warnings, IsEmpty());
+}
+
TEST(FeatureResolverLifetimesTest, DeprecatedFeature) {
FeatureSet features = ParseTextOrDie(R"pb(
[pb.test] { removed_feature: VALUE1 }
@@ -632,6 +643,58 @@
HasSubstr("Custom feature deprecation warning"))));
}
+TEST(FeatureResolverLifetimesTest, CustomRemovedOptionNotSupported) {
+ MessageOptions option = ParseTextOrDie(R"pb(
+ [proto2_unittest.removed_option]: true
+ )pb");
+ auto results =
+ FeatureResolver::ValidateFeatureLifetimes(EDITION_2024, option, nullptr);
+ EXPECT_THAT(results.errors, IsEmpty());
+ EXPECT_THAT(results.warnings, IsEmpty());
+}
+
+TEST(FeatureResolverLifetimesTest, CustomDeprecatedOptionNotSupported) {
+ MessageOptions option = ParseTextOrDie(R"pb(
+ [proto2_unittest.deprecated_option]: true
+ )pb");
+ auto results =
+ FeatureResolver::ValidateFeatureLifetimes(EDITION_2024, option, nullptr);
+ EXPECT_THAT(results.errors, IsEmpty());
+ EXPECT_THAT(results.warnings, IsEmpty());
+}
+
+TEST(FeatureResolverLifetimesTest, CustomRemovedMessageOptionNotSupported) {
+ MessageOptions option = ParseTextOrDie(R"pb(
+ [proto2_unittest.custom_option] { removed_message_option: "test" }
+ )pb");
+ auto results =
+ FeatureResolver::ValidateFeatureLifetimes(EDITION_2024, option, nullptr);
+ EXPECT_THAT(results.errors, IsEmpty());
+ EXPECT_THAT(results.warnings, IsEmpty());
+}
+
+TEST(FeatureResolverLifetimesTest, CustomDeprecatedMessageOptionNotSupported) {
+ MessageOptions option = ParseTextOrDie(R"pb(
+ [proto2_unittest.custom_option] { deprecated_message_option: "test" }
+ )pb");
+ auto results =
+ FeatureResolver::ValidateFeatureLifetimes(EDITION_2024, option, nullptr);
+ EXPECT_THAT(results.errors, IsEmpty());
+ EXPECT_THAT(results.warnings, IsEmpty());
+}
+
+TEST(FeatureResolverLifetimesTest, CustomNestedMessageOptionNotSupported) {
+ MessageOptions option = ParseTextOrDie(R"pb(
+ [proto2_unittest.CustomOption.Nested.nested_custom_option] {
+ removed_message_option: "test"
+ }
+ )pb");
+ auto results =
+ FeatureResolver::ValidateFeatureLifetimes(EDITION_2024, option, nullptr);
+ EXPECT_THAT(results.errors, IsEmpty());
+ EXPECT_THAT(results.warnings, IsEmpty());
+}
+
TEST(FeatureResolverLifetimesTest, RemovedFeature) {
FeatureSet features = ParseTextOrDie(R"pb(
[pb.test] { removed_feature: VALUE1 }
@@ -659,6 +722,22 @@
EXPECT_THAT(results.warnings, IsEmpty());
}
+TEST(FeatureResolverLifetimesTest, RemovedOption) {
+ FileOptions options = ParseTextOrDie(R"pb(
+ java_multiple_files: true
+ )pb");
+ auto results =
+ FeatureResolver::ValidateFeatureLifetimes(EDITION_2024, options, nullptr);
+ EXPECT_THAT(
+ results.errors,
+ ElementsAre(AllOf(
+ HasSubstr("google.protobuf.FileOptions.java_multiple_files"),
+ HasSubstr("removed in edition 2024:"),
+ HasSubstr(
+ "you can set `features.(pb.java).nest_in_file_class = YES`"))));
+ EXPECT_THAT(results.warnings, IsEmpty());
+}
+
TEST(FeatureResolverLifetimesTest, RemovedFeatureWithNoRemovalError) {
FeatureSet features = ParseTextOrDie(R"pb(
[pb.test] { same_edition_removed_feature: VALUE1 }
@@ -672,7 +751,7 @@
EXPECT_THAT(results.warnings, IsEmpty());
}
-TEST(FeatureResolverLifetimesTest, NotIntroduced) {
+TEST(FeatureResolverLifetimesTest, NotIntroducedFeature) {
FeatureSet features = ParseTextOrDie(R"pb(
[pb.test] { future_feature: VALUE1 }
)pb");
@@ -686,6 +765,20 @@
EXPECT_THAT(results.warnings, IsEmpty());
}
+TEST(FeatureResolverLifetimesTest, NotIntroducedOption) {
+ FileOptions options = ParseTextOrDie(R"pb(
+ java_multiple_files: true
+ )pb");
+ auto results = FeatureResolver::ValidateFeatureLifetimes(EDITION_1_TEST_ONLY,
+ options, nullptr);
+ EXPECT_THAT(
+ results.errors,
+ ElementsAre(AllOf(HasSubstr("google.protobuf.FileOptions.java_multiple_files"),
+ HasSubstr("wasn't introduced until edition PROTO2"),
+ HasSubstr("can't be used in edition 1_TEST_ONLY"))));
+ EXPECT_THAT(results.warnings, IsEmpty());
+}
+
TEST(FeatureResolverLifetimesTest, WarningsAndErrors) {
FeatureSet features = ParseTextOrDie(R"pb(
[pb.test] { future_feature: VALUE1 removed_feature: VALUE1 }
@@ -694,8 +787,10 @@
features, nullptr);
EXPECT_THAT(results.errors,
ElementsAre(HasSubstr("pb.TestFeatures.future_feature")));
- EXPECT_THAT(results.warnings,
- ElementsAre(HasSubstr("pb.TestFeatures.removed_feature")));
+ EXPECT_THAT(
+ results.warnings,
+ ElementsAre(AllOf(HasSubstr("pb.TestFeatures.removed_feature"),
+ HasSubstr("Custom feature deprecation warning"))));
}
TEST(FeatureResolverLifetimesTest, MultipleErrors) {
@@ -710,7 +805,7 @@
EXPECT_THAT(results.warnings, IsEmpty());
}
-TEST(FeatureResolverLifetimesTest, DynamicPool) {
+TEST(FeatureResolverLifetimesTest, FeatureDynamicPool) {
DescriptorPool pool;
{
FileDescriptorProto file;
diff --git a/src/google/protobuf/unittest_custom_options.proto b/src/google/protobuf/unittest_custom_options.proto
index 3ebb8e3..fa39a1c 100644
--- a/src/google/protobuf/unittest_custom_options.proto
+++ b/src/google/protobuf/unittest_custom_options.proto
@@ -71,6 +71,35 @@
optional MethodOpt1 method_opt1 = 7890860;
}
+// Custom options with feature support
+extend google.protobuf.MessageOptions {
+ optional bool removed_option = 7733026 [feature_support = {
+ edition_removed: EDITION_2023
+ removal_error: "Custom feature removal warning"
+ }];
+ optional bool deprecated_option = 7737036 [feature_support = {
+ edition_deprecated: EDITION_2023
+ deprecation_warning: "Custom feature deprecation warning"
+ }];
+ optional CustomOption custom_option = 7737026;
+}
+
+message CustomOption {
+ optional string removed_message_option = 7734026 [feature_support = {
+ edition_removed: EDITION_2023
+ removal_error: "Custom feature removal warning"
+ }];
+ optional string deprecated_message_option = 7734016 [feature_support = {
+ edition_deprecated: EDITION_2023
+ deprecation_warning: "Custom feature deprecation warning"
+ }];
+ message Nested {
+ extend google.protobuf.MessageOptions {
+ optional CustomOption nested_custom_option = 7732016;
+ }
+ }
+}
+
// A test message with custom options at all possible locations (and also some
// regular options, to make sure they interact nicely).
message TestMessageWithCustomOptions {
