Skip to content

Commit

Permalink
Limit feature deprecation warnings to reduce noise.
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@mkruskal-google @copybara-github
mkruskal-google authored and copybara-github committed Aug 15, 2024
1 parent 6c2be3b commit 5cd9a46
Show file tree
Hide file tree
Showing 8 changed files with 184 additions and 47 deletions.
4 changes: 2 additions & 2 deletions src/google/protobuf/compiler/command_line_interface.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down Expand Up @@ -1643,7 +1643,7 @@ bool CommandLineInterface::ParseInputFiles(
}
}
}
descriptor_pool->ClearUnusedImportTrackFiles();
descriptor_pool->ClearDirectInputFiles();
return result;
}

Expand Down
48 changes: 48 additions & 0 deletions src/google/protobuf/compiler/command_line_interface_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down
9 changes: 3 additions & 6 deletions src/google/protobuf/compiler/importer.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(); }


// ===================================================================
Expand Down
16 changes: 13 additions & 3 deletions src/google/protobuf/compiler/importer.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
38 changes: 19 additions & 19 deletions src/google/protobuf/descriptor.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
}
}
Expand Down Expand Up @@ -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(
Expand All @@ -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_);
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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.");
Expand Down
26 changes: 19 additions & 7 deletions src/google/protobuf/descriptor.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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<std::string, bool> 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<std::string, bool> 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.
Expand Down
86 changes: 76 additions & 10 deletions src/google/protobuf/descriptor_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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\" "
Expand Down Expand Up @@ -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\""
Expand All @@ -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\""
Expand Down Expand Up @@ -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"
Expand Down Expand Up @@ -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' "
Expand Down Expand Up @@ -10866,6 +10866,7 @@ TEST_F(FeaturesTest, InvalidGroupLabel) {
}

TEST_F(FeaturesTest, DeprecatedFeature) {
pool_.AddDirectInputFile("foo.proto");
BuildDescriptorMessagesInTestPool();
BuildFileInTestPool(pb::TestFeatures::descriptor()->file());
BuildFileWithWarnings(
Expand All @@ -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",
Expand All @@ -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());
Expand Down
4 changes: 4 additions & 0 deletions src/google/protobuf/port_def.inc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 5cd9a46

Please sign in to comment.