From 5cd9a463f96478cc6203f02635368a7a8ecc22b3 Mon Sep 17 00:00:00 2001 From: Mike Kruskal Date: Thu, 15 Aug 2024 12:08:01 -0700 Subject: [PATCH] Limit feature deprecation warnings to reduce noise. This builds off of our existing pattern for unused imports. We will still warn when any deprecated features are used in a proto file passed directly to protoc, but will avoid them in the following situations: 1) Transitive imports pulled from the filesystem or descriptors will not trigger warnings. This will keep warnings isolated to the upstream builds instead of alerting all downstream clients. 2) Dynamic pool builds will not log deprecation warnings by default. PiperOrigin-RevId: 663396953 --- .../compiler/command_line_interface.cc | 4 +- .../command_line_interface_unittest.cc | 48 +++++++++++ src/google/protobuf/compiler/importer.cc | 9 +- src/google/protobuf/compiler/importer.h | 16 +++- src/google/protobuf/descriptor.cc | 38 ++++---- src/google/protobuf/descriptor.h | 26 ++++-- src/google/protobuf/descriptor_unittest.cc | 86 ++++++++++++++++--- src/google/protobuf/port_def.inc | 4 + 8 files changed, 184 insertions(+), 47 deletions(-) diff --git a/src/google/protobuf/compiler/command_line_interface.cc b/src/google/protobuf/compiler/command_line_interface.cc index cd95c8b41346..6d95e9e5336c 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 99fe927644af..b55dd3337ed7 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 3c4f9ac174cc..4b38df4e0412 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 6bb127a776c4..e63a7b7b30a7 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 62a2dd58cc5c..31c8e74422fc 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 08a3d5c51027..f6ac34155625 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 e3360e171dcc..3ee6e9667410 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 3377937f3b13..c32c8e9cf9f5 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