From 96f3eeb91a56338dacbc11a556be9c59e350d3c6 Mon Sep 17 00:00:00 2001 From: Protobuf Team Bot Date: Mon, 5 Feb 2024 09:40:28 -0800 Subject: [PATCH] Add validation for string_type and ctype under edition 2024 and over. PiperOrigin-RevId: 604345202 --- src/google/protobuf/compiler/BUILD.bazel | 1 + .../protobuf/compiler/code_generator.cc | 6 ++ src/google/protobuf/compiler/code_generator.h | 3 + .../compiler/command_line_interface.cc | 15 ++- .../command_line_interface_unittest.cc | 6 +- src/google/protobuf/compiler/cpp/generator.cc | 23 +++++ .../compiler/cpp/generator_unittest.cc | 96 +++++++++++++++++++ src/google/protobuf/cpp_edition_defaults.h | 2 +- src/google/protobuf/descriptor.cc | 40 +++++--- src/google/protobuf/descriptor_unittest.cc | 50 ++++++++++ 10 files changed, 214 insertions(+), 28 deletions(-) diff --git a/src/google/protobuf/compiler/BUILD.bazel b/src/google/protobuf/compiler/BUILD.bazel index f53810f5f6..a564703626 100644 --- a/src/google/protobuf/compiler/BUILD.bazel +++ b/src/google/protobuf/compiler/BUILD.bazel @@ -83,6 +83,7 @@ cc_library( "//src/google/protobuf:port", "//src/google/protobuf:protobuf_lite", "//src/google/protobuf/compiler:retention", + "//src/google/protobuf/compiler/allowlists", "//src/google/protobuf/io", "//src/google/protobuf/io:io_win32", "@com_google_absl//absl/container:flat_hash_map", diff --git a/src/google/protobuf/compiler/code_generator.cc b/src/google/protobuf/compiler/code_generator.cc index 332cf53dfb..fd1f45f3e8 100644 --- a/src/google/protobuf/compiler/code_generator.cc +++ b/src/google/protobuf/compiler/code_generator.cc @@ -19,6 +19,7 @@ #include "absl/strings/str_split.h" #include "absl/strings/string_view.h" #include "absl/strings/strip.h" +#include "google/protobuf/compiler/allowlists/allowlists.h" #include "google/protobuf/compiler/plugin.pb.h" #include "google/protobuf/descriptor.h" #include "google/protobuf/feature_resolver.h" @@ -139,6 +140,11 @@ bool IsKnownFeatureProto(absl::string_view filename) { return false; } +bool CanSkipEditionCheck(absl::string_view filename) { + return absl::StartsWith(filename, "google/protobuf/") || + absl::StartsWith(filename, "upb/"); +} + } // namespace compiler } // namespace protobuf } // namespace google diff --git a/src/google/protobuf/compiler/code_generator.h b/src/google/protobuf/compiler/code_generator.h index a41bddd367..8111161c1b 100644 --- a/src/google/protobuf/compiler/code_generator.h +++ b/src/google/protobuf/compiler/code_generator.h @@ -238,6 +238,9 @@ PROTOC_EXPORT std::string StripProto(absl::string_view filename); // Returns true if the proto path corresponds to a known feature file. PROTOC_EXPORT bool IsKnownFeatureProto(absl::string_view filename); +// Returns true if the proto path can skip edition check. +PROTOC_EXPORT bool CanSkipEditionCheck(absl::string_view filename); + } // namespace compiler } // namespace protobuf } // namespace google diff --git a/src/google/protobuf/compiler/command_line_interface.cc b/src/google/protobuf/compiler/command_line_interface.cc index 8a60501010..af85ead3cd 100644 --- a/src/google/protobuf/compiler/command_line_interface.cc +++ b/src/google/protobuf/compiler/command_line_interface.cc @@ -1514,10 +1514,13 @@ bool CommandLineInterface::SetupFeatureResolution(DescriptorPool& pool) { // that support editions must agree on the supported edition range. std::vector feature_extensions; Edition minimum_edition = PROTOBUF_MINIMUM_EDITION; - Edition maximum_edition = PROTOBUF_MAXIMUM_EDITION; + // Override maximum_edition if experimental_editions is true. + Edition maximum_edition = + !experimental_editions_ ? PROTOBUF_MAXIMUM_EDITION : Edition::EDITION_MAX; for (const auto& output : output_directives_) { if (output.generator == nullptr) continue; - if ((output.generator->GetSupportedFeatures() & + if (!experimental_editions_ && + (output.generator->GetSupportedFeatures() & CodeGenerator::FEATURE_SUPPORTS_EDITIONS) != 0) { // Only validate min/max edition on generators that advertise editions // support. Generators still under development will always use the @@ -2576,15 +2579,11 @@ bool CommandLineInterface::EnforceEditionsSupport( for (const auto* fd : parsed_files) { Edition edition = ::google::protobuf::internal::InternalFeatureHelper::GetEdition(*fd); - if (edition < Edition::EDITION_2023) { - // Legacy proto2/proto3 files don't need any checks. + if (edition < Edition::EDITION_2023 || CanSkipEditionCheck(fd->name())) { + // Legacy proto2/proto3 or exempted files don't need any checks. continue; } - if (absl::StartsWith(fd->name(), "google/protobuf/") || - absl::StartsWith(fd->name(), "upb/")) { - continue; - } if ((supported_features & CodeGenerator::FEATURE_SUPPORTS_EDITIONS) == 0) { std::cerr << absl::Substitute( "$0: is an editions file, but code generator $1 hasn't been " diff --git a/src/google/protobuf/compiler/command_line_interface_unittest.cc b/src/google/protobuf/compiler/command_line_interface_unittest.cc index d993798d73..133a393df4 100644 --- a/src/google/protobuf/compiler/command_line_interface_unittest.cc +++ b/src/google/protobuf/compiler/command_line_interface_unittest.cc @@ -1483,8 +1483,7 @@ TEST_F(CommandLineInterfaceTest, InvalidMinimumEditionError) { mock_generator_->set_minimum_edition(EDITION_1_TEST_ONLY); - Run("protocol_compiler --proto_path=$tmpdir --test_out=$tmpdir " - "--experimental_editions foo.proto"); + Run("protocol_compiler --proto_path=$tmpdir --test_out=$tmpdir foo.proto"); ExpectErrorSubstring( "generator --test_out specifies a minimum edition 1_TEST_ONLY which is " "not the protoc minimum PROTO2"); @@ -1495,8 +1494,7 @@ TEST_F(CommandLineInterfaceTest, InvalidMaximumEditionError) { mock_generator_->set_maximum_edition(EDITION_99999_TEST_ONLY); - Run("protocol_compiler --proto_path=$tmpdir --test_out=$tmpdir " - "--experimental_editions foo.proto"); + Run("protocol_compiler --proto_path=$tmpdir --test_out=$tmpdir foo.proto"); ExpectErrorSubstring( "generator --test_out specifies a maximum edition 99999_TEST_ONLY which " "is not the protoc maximum 2023"); diff --git a/src/google/protobuf/compiler/cpp/generator.cc b/src/google/protobuf/compiler/cpp/generator.cc index 952ef19c14..382fa132ba 100644 --- a/src/google/protobuf/compiler/cpp/generator.cc +++ b/src/google/protobuf/compiler/cpp/generator.cc @@ -353,6 +353,7 @@ static bool IsEnumMapType(const FieldDescriptor& field) { absl::Status CppGenerator::ValidateFeatures(const FileDescriptor* file) const { absl::Status status = absl::OkStatus(); auto edition = GetEdition(*file); + absl::string_view filename = file->name(); google::protobuf::internal::VisitDescriptors(*file, [&](const FieldDescriptor& field) { const FeatureSet& resolved_features = GetResolvedSourceFeatures(field); @@ -382,6 +383,28 @@ absl::Status CppGenerator::ValidateFeatures(const FileDescriptor* file) const { } } + if (unresolved_features.has_string_type()) { + if (!CanSkipEditionCheck(filename) && edition < Edition::EDITION_2024) { + status = absl::FailedPreconditionError(absl::StrCat( + "Field ", field.full_name(), + " specifies string_type which is not currently allowed.")); + } else if (field.cpp_type() != FieldDescriptor::CPPTYPE_STRING) { + status = absl::FailedPreconditionError(absl::StrCat( + "Field ", field.full_name(), + " specifies string_type, but is not a string nor bytes field.")); + } else if (unresolved_features.string_type() == pb::CppFeatures::CORD && + field.is_extension()) { + status = absl::FailedPreconditionError( + absl::StrCat("Extension ", field.full_name(), + " specifies string_type=CORD which is not supported " + "for extensions.")); + } else if (field.options().has_ctype()) { + status = absl::FailedPreconditionError(absl::StrCat( + field.full_name(), + " specifies both string_type and ctype which is not supported.")); + } + } + // 'ctype' check has moved to DescriptorBuilder for Edition 2023 and above. if (edition < Edition::EDITION_2023 && field.options().has_ctype()) { if (field.cpp_type() != FieldDescriptor::CPPTYPE_STRING) { diff --git a/src/google/protobuf/compiler/cpp/generator_unittest.cc b/src/google/protobuf/compiler/cpp/generator_unittest.cc index f421b5cba5..1b7257ddae 100644 --- a/src/google/protobuf/compiler/cpp/generator_unittest.cc +++ b/src/google/protobuf/compiler/cpp/generator_unittest.cc @@ -148,6 +148,102 @@ TEST_F(CppGeneratorTest, LegacyClosedEnumImplicit) { "Field Foo.bar has a closed enum type with implicit presence."); } +TEST_F(CppGeneratorTest, NoStringTypeTillEdition2024) { + CreateTempFile("foo.proto", R"schema( + edition = "2023"; + import "google/protobuf/cpp_features.proto"; + + message Foo { + int32 bar = 1; + bytes baz = 2 [features.(pb.cpp).string_type = CORD]; + } + )schema"); + + RunProtoc( + "protocol_compiler --proto_path=$tmpdir --cpp_out=$tmpdir " + "--experimental_editions foo.proto"); + + ExpectErrorSubstring( + "Field Foo.baz specifies string_type which is not currently allowed."); +} + +TEST_F(CppGeneratorTest, StringTypeForCord) { + CreateTempFile("foo.proto", R"schema( + edition = "2024"; + import "google/protobuf/cpp_features.proto"; + + message Foo { + int32 bar = 1; + bytes baz = 2 [features.(pb.cpp).string_type = CORD]; + } + )schema"); + + RunProtoc( + "protocol_compiler --proto_path=$tmpdir --cpp_out=$tmpdir " + "--experimental_editions foo.proto"); + + ExpectNoErrors(); +} + +TEST_F(CppGeneratorTest, CtypeForCord) { + CreateTempFile("foo.proto", R"schema( + edition = "2023"; + + message Foo { + int32 bar = 1; + bytes baz = 2 [ctype = CORD]; + } + )schema"); + + RunProtoc( + "protocol_compiler --proto_path=$tmpdir --cpp_out=$tmpdir " + "--experimental_editions foo.proto"); + + ExpectNoErrors(); +} + +TEST_F(CppGeneratorTest, StringTypeForStringFieldsOnly) { + CreateTempFile("foo.proto", R"schema( + edition = "2024"; + import "google/protobuf/cpp_features.proto"; + + message Foo { + int32 bar = 1; + int32 baz = 2 [features.(pb.cpp).string_type = CORD]; + } + )schema"); + + RunProtoc( + "protocol_compiler --proto_path=$tmpdir --cpp_out=$tmpdir " + "--experimental_editions foo.proto"); + + ExpectErrorSubstring( + "Field Foo.baz specifies string_type, but is not a string nor bytes " + "field."); +} + +TEST_F(CppGeneratorTest, StringTypeCordNotForExtension) { + CreateTempFile("foo.proto", R"schema( + edition = "2024"; + import "google/protobuf/cpp_features.proto"; + + message Foo { + extensions 1 to max; + } + extend Foo { + bytes bar = 1 [features.(pb.cpp).string_type = CORD]; + } + )schema"); + + RunProtoc( + "protocol_compiler --proto_path=$tmpdir --cpp_out=$tmpdir " + "--experimental_editions foo.proto"); + + ExpectErrorSubstring( + "Extension bar specifies string_type=CORD which is not supported for " + "extensions."); +} + TEST_F(CppGeneratorTest, CtypeOnNoneStringFieldTest) { CreateTempFile("foo.proto", R"schema( diff --git a/src/google/protobuf/cpp_edition_defaults.h b/src/google/protobuf/cpp_edition_defaults.h index 23cae53eb3..40107bd4d7 100644 --- a/src/google/protobuf/cpp_edition_defaults.h +++ b/src/google/protobuf/cpp_edition_defaults.h @@ -5,7 +5,7 @@ // the C++ runtime. This is used for feature resolution under Editions. // NOLINTBEGIN // clang-format off -#define PROTOBUF_INTERNAL_CPP_EDITION_DEFAULTS "\n\030\022\023\010\001\020\002\030\002 \003(\0010\002\302>\004\010\001\020\003\030\346\007\n\030\022\023\010\002\020\001\030\001 \002(\0010\001\302>\004\010\000\020\003\030\347\007\n\030\022\023\010\001\020\001\030\001 \002(\0010\001\302>\004\010\000\020\003\030\350\007 \346\007(\350\007" +#define PROTOBUF_INTERNAL_CPP_EDITION_DEFAULTS "\n\030\022\023\010\001\020\002\030\002 \003(\0010\002\302>\004\010\001\020\003\030\346\007\n\030\022\023\010\002\020\001\030\001 \002(\0010\001\302>\004\010\000\020\003\030\347\007\n\030\022\023\010\001\020\001\030\001 \002(\0010\001\302>\004\010\000\020\003\030\350\007\n\030\022\023\010\001\020\001\030\001 \002(\0010\001\302>\004\010\000\020\001\030\351\007 \346\007(\351\007" // clang-format on // NOLINTEND diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc index 657cddbeda..4d7ce3df89 100644 --- a/src/google/protobuf/descriptor.cc +++ b/src/google/protobuf/descriptor.cc @@ -7658,25 +7658,34 @@ void DescriptorBuilder::ValidateOptions(const FieldDescriptor* field, ValidateFieldFeatures(field, proto); - if (field->file()->edition() >= Edition::EDITION_2023 && - field->options().has_ctype()) { - if (field->cpp_type() != FieldDescriptor::CPPTYPE_STRING) { - AddError( - field->full_name(), proto, DescriptorPool::ErrorCollector::TYPE, - absl::StrFormat( - "Field %s specifies ctype, but is not a string nor bytes field.", - field->full_name()) - .c_str()); - } - if (field->options().ctype() == FieldOptions::CORD) { - if (field->is_extension()) { + auto edition = field->file()->edition(); + auto& field_options = field->options(); + + if (field_options.has_ctype()) { + if (edition >= Edition::EDITION_2024) { + // "ctype" is no longer supported in edition 2024 and beyond. + AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME, + "ctype option is not allowed under edition 2024 and beyond. Use " + "the feature string_type = VIEW|CORD|STRING|... instead."); + } else if (edition == Edition::EDITION_2023) { + if (field->cpp_type() != FieldDescriptor::CPPTYPE_STRING) { AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::TYPE, - absl::StrFormat("Extension %s specifies ctype=CORD which is " - "not supported for extensions.", + absl::StrFormat("Field %s specifies ctype, but is not a " + "string nor bytes field.", field->full_name()) .c_str()); } + if (field_options.ctype() == FieldOptions::CORD) { + if (field->is_extension()) { + AddError(field->full_name(), proto, + DescriptorPool::ErrorCollector::TYPE, + absl::StrFormat("Extension %s specifies ctype=CORD which is " + "not supported for extensions.", + field->full_name()) + .c_str()); + } + } } } @@ -7866,8 +7875,9 @@ void DescriptorBuilder::ValidateFieldFeatures( "message_encoding = DELIMITED to control this behavior."); } + auto& field_options = field->options(); // Validate legacy options that have been migrated to features. - if (field->options().has_packed()) { + if (field_options.has_packed()) { AddError(field->full_name(), proto, DescriptorPool::ErrorCollector::NAME, "Field option packed is not allowed under editions. Use the " "repeated_field_encoding feature to control this behavior."); diff --git a/src/google/protobuf/descriptor_unittest.cc b/src/google/protobuf/descriptor_unittest.cc index e49e62ee0a..71aa90318a 100644 --- a/src/google/protobuf/descriptor_unittest.cc +++ b/src/google/protobuf/descriptor_unittest.cc @@ -7660,6 +7660,32 @@ TEST_F(FeaturesTest, Edition2023Defaults) { EXPECT_EQ(GetFeatures(file).GetExtension(pb::test).int_file_feature(), 1); } +TEST_F(FeaturesTest, Edition2024Defaults) { + FileDescriptorProto file_proto = ParseTextOrDie(R"pb( + name: "foo.proto" + syntax: "editions" + edition: EDITION_2024 + )pb"); + + BuildDescriptorMessagesInTestPool(); + const FileDescriptor* file = ABSL_DIE_IF_NULL(pool_.BuildFile(file_proto)); + EXPECT_THAT(file->options(), EqualsProto("")); + EXPECT_THAT(GetCoreFeatures(file), EqualsProto(R"pb( + field_presence: EXPLICIT + enum_type: OPEN + repeated_field_encoding: PACKED + utf8_validation: VERIFY + message_encoding: LENGTH_PREFIXED + json_format: ALLOW + [pb.cpp] { legacy_closed_enum: false string_type: VIEW } + )pb")); + + // Since pb::test is registered in the pool, it should end up with defaults in + // our FeatureSet. + EXPECT_TRUE(GetFeatures(file).HasExtension(pb::test)); + EXPECT_EQ(GetFeatures(file).GetExtension(pb::test).int_file_feature(), 1); +} + TEST_F(FeaturesBaseTest, DefaultEdition2023Defaults) { BuildDescriptorMessagesInTestPool(); BuildFileInTestPool(pb::TestFeatures::descriptor()->file()); @@ -9442,6 +9468,30 @@ TEST_F(FeaturesTest, InvalidFieldPacked) { "behavior.\n"); } +TEST_F(FeaturesTest, NoCtypeFromEdition2024) { + BuildDescriptorMessagesInTestPool(); + BuildFileWithErrors( + R"pb( + name: "foo.proto" + syntax: "editions" + edition: EDITION_2024 + message_type { + name: "Foo" + field { name: "foo" number: 1 label: LABEL_OPTIONAL type: TYPE_INT32 } + field { + name: "bar" + number: 2 + label: LABEL_OPTIONAL + type: TYPE_STRING + options { ctype: CORD } + } + } + )pb", + "foo.proto: Foo.bar: NAME: ctype option is not allowed under edition " + "2024 and beyond. Use the feature string_type = VIEW|CORD|STRING|... " + "instead.\n"); +} + TEST_F(FeaturesTest, InvalidFieldImplicitDefault) { BuildDescriptorMessagesInTestPool();