diff --git a/src/google/protobuf/compiler/command_line_interface.cc b/src/google/protobuf/compiler/command_line_interface.cc index cd95c8b413460..6d95e9e5336c1 100644 --- a/src/google/protobuf/compiler/command_line_interface.cc +++ b/src/google/protobuf/compiler/command_line_interface.cc @@ -1596,7 +1596,7 @@ bool CommandLineInterface::ParseInputFiles( // exclusively reading from descriptor sets, we can eliminate this failure // condition. for (const auto& input_file : input_files_) { - descriptor_pool->AddUnusedImportTrackFile(input_file); + descriptor_pool->AddDirectInputFile(input_file); } } @@ -1643,7 +1643,7 @@ bool CommandLineInterface::ParseInputFiles( } } } - descriptor_pool->ClearUnusedImportTrackFiles(); + descriptor_pool->ClearDirectInputFiles(); return result; } diff --git a/src/google/protobuf/compiler/command_line_interface_unittest.cc b/src/google/protobuf/compiler/command_line_interface_unittest.cc index 99fe927644af3..b55dd3337ed78 100644 --- a/src/google/protobuf/compiler/command_line_interface_unittest.cc +++ b/src/google/protobuf/compiler/command_line_interface_unittest.cc @@ -1551,6 +1551,54 @@ TEST_F(CommandLineInterfaceTest, Plugin_DeprecatedEdition) { "edition 99997_TEST_ONLY."); } +TEST_F(CommandLineInterfaceTest, Plugin_DeprecatedFeature) { + CreateTempFile("google/protobuf/descriptor.proto", + google::protobuf::DescriptorProto::descriptor()->file()->DebugString()); + CreateTempFile("google/protobuf/unittest_features.proto", + pb::TestFeatures::descriptor()->file()->DebugString()); + CreateTempFile("foo.proto", + R"schema( + edition = "2023"; + import "google/protobuf/unittest_features.proto"; + package foo; + option features.(pb.test).removed_feature = VALUE9; + )schema"); + + 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"); +} + +TEST_F(CommandLineInterfaceTest, Plugin_TransitiveDeprecatedFeature) { + CreateTempFile("google/protobuf/descriptor.proto", + google::protobuf::DescriptorProto::descriptor()->file()->DebugString()); + CreateTempFile("google/protobuf/unittest_features.proto", + pb::TestFeatures::descriptor()->file()->DebugString()); + CreateTempFile("unused.proto", + R"schema( + edition = "2023"; + import "google/protobuf/unittest_features.proto"; + package foo; + option features.(pb.test).removed_feature = VALUE9; + message Foo {} + )schema"); + CreateTempFile("foo.proto", + R"schema( + edition = "2023"; + import "unused.proto"; + package foo; + message Bar { + Foo foo = 1; + } + )schema"); + + Run("protocol_compiler --test_out=$tmpdir " + "--proto_path=$tmpdir foo.proto"); + ExpectNoErrors(); +} + TEST_F(CommandLineInterfaceTest, Plugin_FutureEdition) { CreateTempFile("foo.proto", R"schema( edition = "2023"; diff --git a/src/google/protobuf/compiler/importer.cc b/src/google/protobuf/compiler/importer.cc index 3c4f9ac174cc0..4b38df4e04126 100644 --- a/src/google/protobuf/compiler/importer.cc +++ b/src/google/protobuf/compiler/importer.cc @@ -216,14 +216,11 @@ const FileDescriptor* Importer::Import(const std::string& filename) { return pool_.FindFileByName(filename); } -void Importer::AddUnusedImportTrackFile(const std::string& file_name, - bool is_error) { - pool_.AddUnusedImportTrackFile(file_name, is_error); +void Importer::AddDirectInputFile(absl::string_view file_name, bool is_error) { + pool_.AddDirectInputFile(file_name, is_error); } -void Importer::ClearUnusedImportTrackFiles() { - pool_.ClearUnusedImportTrackFiles(); -} +void Importer::ClearDirectInputFiles() { pool_.ClearDirectInputFiles(); } // =================================================================== diff --git a/src/google/protobuf/compiler/importer.h b/src/google/protobuf/compiler/importer.h index 6bb127a776c4c..e63a7b7b30a79 100644 --- a/src/google/protobuf/compiler/importer.h +++ b/src/google/protobuf/compiler/importer.h @@ -159,9 +159,19 @@ class PROTOBUF_EXPORT Importer { // contents are stored. inline const DescriptorPool* pool() const { return &pool_; } - void AddUnusedImportTrackFile(const std::string& file_name, - bool is_error = false); - void ClearUnusedImportTrackFiles(); + void AddDirectInputFile(absl::string_view file_name, + bool unused_import_is_error = false); + void ClearDirectInputFiles(); + +#if !defined(PROTOBUF_FUTURE_RENAME_ADD_UNUSED_IMPORT) && !defined(SWIG) + ABSL_DEPRECATED("Use AddDirectInputFile") + void AddUnusedImportTrackFile(absl::string_view file_name, + bool is_error = false) { + AddDirectInputFile(file_name, is_error); + } + ABSL_DEPRECATED("Use AddDirectInputFile") + void ClearUnusedImportTrackFiles() { ClearDirectInputFiles(); } +#endif // !PROTOBUF_FUTURE_RENAME_ADD_UNUSED_IMPORT && !SWIG private: diff --git a/src/google/protobuf/descriptor.cc b/src/google/protobuf/descriptor.cc index 62a2dd58cc5c5..31c8e74422fce 100644 --- a/src/google/protobuf/descriptor.cc +++ b/src/google/protobuf/descriptor.cc @@ -1382,14 +1382,17 @@ class DescriptorPool::DeferredValidation { DescriptorPool::ErrorCollector::NAME, error); } } - for (const auto& warning : results.warnings) { - if (error_collector_ == nullptr) { - ABSL_LOG(WARNING) - << info.filename << " " << info.full_name << ": " << warning; - } else { - error_collector_->RecordWarning( - info.filename, info.full_name, info.proto, - DescriptorPool::ErrorCollector::NAME, warning); + if (pool_->direct_input_files_.find(file->name()) != + pool_->direct_input_files_.end()) { + for (const auto& warning : results.warnings) { + if (error_collector_ == nullptr) { + ABSL_LOG(WARNING) + << info.filename << " " << info.full_name << ": " << warning; + } else { + error_collector_->RecordWarning( + info.filename, info.full_name, info.proto, + DescriptorPool::ErrorCollector::NAME, warning); + } } } } @@ -2132,9 +2135,9 @@ void DescriptorPool::InternalDontEnforceDependencies() { enforce_dependencies_ = false; } -void DescriptorPool::AddUnusedImportTrackFile(absl::string_view file_name, - bool is_error) { - unused_import_track_files_[file_name] = is_error; +void DescriptorPool::AddDirectInputFile(absl::string_view file_name, + bool is_error) { + direct_input_files_[file_name] = is_error; } bool DescriptorPool::IsReadyForCheckingDescriptorExtDecl( @@ -2155,9 +2158,7 @@ bool DescriptorPool::IsReadyForCheckingDescriptorExtDecl( } -void DescriptorPool::ClearUnusedImportTrackFiles() { - unused_import_track_files_.clear(); -} +void DescriptorPool::ClearDirectInputFiles() { direct_input_files_.clear(); } bool DescriptorPool::InternalIsFileLoaded(absl::string_view filename) const { absl::MutexLockMaybe lock(mutex_); @@ -6022,8 +6023,8 @@ FileDescriptor* DescriptorBuilder::BuildFileImpl( // Add to unused_dependency_ to track unused imported files. // Note: do not track unused imported files for public import. if (pool_->enforce_dependencies_ && - (pool_->unused_import_track_files_.find(proto.name()) != - pool_->unused_import_track_files_.end()) && + (pool_->direct_input_files_.find(proto.name()) != + pool_->direct_input_files_.end()) && (dependency->public_dependency_count() == 0)) { unused_dependency_.insert(dependency); } @@ -9593,9 +9594,8 @@ void DescriptorBuilder::LogUnusedDependency(const FileDescriptorProto& proto, (void)result; // Parameter is used by Google-internal code. if (!unused_dependency_.empty()) { - auto itr = pool_->unused_import_track_files_.find(proto.name()); - bool is_error = - itr != pool_->unused_import_track_files_.end() && itr->second; + auto itr = pool_->direct_input_files_.find(proto.name()); + bool is_error = itr != pool_->direct_input_files_.end() && itr->second; for (const auto* unused : unused_dependency_) { auto make_error = [&] { return absl::StrCat("Import ", unused->name(), " is unused."); diff --git a/src/google/protobuf/descriptor.h b/src/google/protobuf/descriptor.h index 08a3d5c510279..f6ac341556251 100644 --- a/src/google/protobuf/descriptor.h +++ b/src/google/protobuf/descriptor.h @@ -2370,11 +2370,23 @@ class PROTOBUF_EXPORT DescriptorPool { // lazy descriptor initialization behavior. bool InternalIsFileLoaded(absl::string_view filename) const; - // Add a file to unused_import_track_files_. DescriptorBuilder will log - // warnings or errors for those files if there is any unused import. + // Add a file to to apply more strict checks to. + // - unused imports will log either warnings or errors. + // - deprecated features will log warnings. + void AddDirectInputFile(absl::string_view file_name, + bool unused_import_is_error = false); + void ClearDirectInputFiles(); + +#if !defined(PROTOBUF_FUTURE_RENAME_ADD_UNUSED_IMPORT) && !defined(SWIG) + ABSL_DEPRECATED("Use AddDirectInputFile") void AddUnusedImportTrackFile(absl::string_view file_name, - bool is_error = false); - void ClearUnusedImportTrackFiles(); + bool is_error = false) { + AddDirectInputFile(file_name, is_error); + } + ABSL_DEPRECATED("Use AddDirectInputFile") + void ClearUnusedImportTrackFiles() { ClearDirectInputFiles(); } +#endif // !PROTOBUF_FUTURE_RENAME_ADD_UNUSED_IMPORT && !SWIG + private: friend class Descriptor; @@ -2476,9 +2488,9 @@ class PROTOBUF_EXPORT DescriptorPool { bool deprecated_legacy_json_field_conflicts_; mutable bool build_started_ = false; - // Set of files to track for unused imports. The bool value when true means - // unused imports are treated as errors (and as warnings when false). - absl::flat_hash_map unused_import_track_files_; + // Set of files to track for additional validation. The bool value when true + // means unused imports are treated as errors (and as warnings when false). + absl::flat_hash_map direct_input_files_; // Specification of defaults to use for feature resolution. This defaults to // just the global and C++ features, but can be overridden for other runtimes. diff --git a/src/google/protobuf/descriptor_unittest.cc b/src/google/protobuf/descriptor_unittest.cc index e3360e171dccb..3ee6e9667410e 100644 --- a/src/google/protobuf/descriptor_unittest.cc +++ b/src/google/protobuf/descriptor_unittest.cc @@ -3898,7 +3898,7 @@ TEST(CustomOptions, UnusedImportError) { &file_proto); ASSERT_TRUE(pool.BuildFile(file_proto) != nullptr); - pool.AddUnusedImportTrackFile("custom_options_import.proto", true); + pool.AddDirectInputFile("custom_options_import.proto", true); ASSERT_TRUE(TextFormat::ParseFromString( "name: \"custom_options_import.proto\" " "package: \"protobuf_unittest\" " @@ -6330,22 +6330,22 @@ TEST_F(ValidationErrorTest, AllowEnumAlias) { } TEST_F(ValidationErrorTest, UnusedImportWarning) { - pool_.AddUnusedImportTrackFile("bar.proto"); + pool_.AddDirectInputFile("bar.proto"); BuildFile( "name: \"bar.proto\" " "message_type { name: \"Bar\" }"); - pool_.AddUnusedImportTrackFile("base.proto"); + pool_.AddDirectInputFile("base.proto"); BuildFile( "name: \"base.proto\" " "message_type { name: \"Base\" }"); - pool_.AddUnusedImportTrackFile("baz.proto"); + pool_.AddDirectInputFile("baz.proto"); BuildFile( "name: \"baz.proto\" " "message_type { name: \"Baz\" }"); - pool_.AddUnusedImportTrackFile("public.proto"); + pool_.AddDirectInputFile("public.proto"); BuildFile( "name: \"public.proto\" " "dependency: \"bar.proto\"" @@ -6360,7 +6360,7 @@ TEST_F(ValidationErrorTest, UnusedImportWarning) { // optional Base base = 1; // } // - pool_.AddUnusedImportTrackFile("forward.proto"); + pool_.AddDirectInputFile("forward.proto"); BuildFileWithWarnings( "name: \"forward.proto\"" "dependency: \"base.proto\"" @@ -6391,7 +6391,7 @@ TEST_F(ValidationErrorTest, SamePackageUnusedImportError) { message_type { name: "Bar" } )pb"); - pool_.AddUnusedImportTrackFile("import.proto", true); + pool_.AddDirectInputFile("import.proto", true); BuildFileWithErrors(R"pb( name: "import.proto" package: "protobuf_unittest" @@ -7344,7 +7344,7 @@ TEST_F(ValidationErrorTest, UnusedImportWithOtherError) { " name: 'Bar'" "}"); - pool_.AddUnusedImportTrackFile("foo.proto", true); + pool_.AddDirectInputFile("foo.proto", true); BuildFileWithErrors( "name: 'foo.proto' " "dependency: 'bar.proto' " @@ -10866,6 +10866,7 @@ TEST_F(FeaturesTest, InvalidGroupLabel) { } TEST_F(FeaturesTest, DeprecatedFeature) { + pool_.AddDirectInputFile("foo.proto"); BuildDescriptorMessagesInTestPool(); BuildFileInTestPool(pb::TestFeatures::descriptor()->file()); BuildFileWithWarnings( @@ -10875,8 +10876,11 @@ TEST_F(FeaturesTest, DeprecatedFeature) { edition: EDITION_2023 dependency: "google/protobuf/unittest_features.proto" options { - features { - [pb.test] { removed_feature: VALUE9 } + uninterpreted_option { + name { name_part: "features" is_extension: false } + name { name_part: "pb.test" is_extension: true } + name { name_part: "removed_feature" is_extension: false } + identifier_value: "VALUE9" } } )pb", @@ -10890,6 +10894,68 @@ TEST_F(FeaturesTest, DeprecatedFeature) { pb::VALUE9); } +TEST_F(FeaturesTest, IgnoreDeprecatedFeature) { + BuildDescriptorMessagesInTestPool(); + BuildFileInTestPool(pb::TestFeatures::descriptor()->file()); + BuildFileWithWarnings( + R"pb( + name: "foo.proto" + syntax: "editions" + edition: EDITION_2023 + dependency: "google/protobuf/unittest_features.proto" + options { + uninterpreted_option { + name { name_part: "features" is_extension: false } + name { name_part: "pb.test" is_extension: true } + name { name_part: "removed_feature" is_extension: false } + identifier_value: "VALUE9" + } + } + )pb", + ""); +} + +TEST_F(FeaturesTest, IgnoreTransitiveFeature) { + pool_.AddDirectInputFile("bar.proto"); + BuildDescriptorMessagesInTestPool(); + BuildFileInTestPool(pb::TestFeatures::descriptor()->file()); + BuildFileWithWarnings( + R"pb( + name: "foo.proto" + syntax: "editions" + edition: EDITION_2023 + dependency: "google/protobuf/unittest_features.proto" + options { + uninterpreted_option { + name { name_part: "features" is_extension: false } + name { name_part: "pb.test" is_extension: true } + name { name_part: "removed_feature" is_extension: false } + identifier_value: "VALUE9" + } + } + message_type { name: "Foo" } + )pb", + ""); + BuildFileWithWarnings( + R"pb( + name: "bar.proto" + syntax: "editions" + edition: EDITION_2023 + dependency: "foo.proto" + message_type { + name: "Bar" + field { + name: "bar" + number: 1 + label: LABEL_OPTIONAL + type: TYPE_MESSAGE + type_name: ".Foo" + } + } + )pb", + ""); +} + TEST_F(FeaturesTest, RemovedFeature) { BuildDescriptorMessagesInTestPool(); BuildFileInTestPool(pb::TestFeatures::descriptor()->file()); diff --git a/src/google/protobuf/port_def.inc b/src/google/protobuf/port_def.inc index 3377937f3b132..c32c8e9cf9f5c 100644 --- a/src/google/protobuf/port_def.inc +++ b/src/google/protobuf/port_def.inc @@ -147,6 +147,10 @@ static_assert(PROTOBUF_ABSL_MIN(20230125, 3), // Owner: ckennelly@, mkruskal@ #define PROTOBUF_FUTURE_REMOVE_CREATEMESSAGE 1 +// Renames DescriptorPool::AddUnusedImportTrackFile +// Owner: mkruskal@ +#define PROTOBUF_FUTURE_RENAME_ADD_UNUSED_IMPORT 1 + #endif #ifdef PROTOBUF_FUTURE_STRING_VIEW_RETURN_TYPE