Add --fatal_warnings flag to treat warnings as errors

Add a --fatal_warnings flag that requests that protoc exit with a failing status code if any warnings are generated during compilation. Partially address protocolbuffers#3980.
perlun · Mar 7, 2021 · 4d25f2d · 4d25f2d
1 parent 35bdcab
commit 4d25f2d
Show file tree

Hide file tree

Showing 3 changed files with 47 additions and 5 deletions.
diff --git a/src/google/protobuf/compiler/command_line_interface.cc b/src/google/protobuf/compiler/command_line_interface.cc
@@ -288,7 +288,7 @@ class CommandLineInterface::ErrorPrinter
       public DescriptorPool::ErrorCollector {
  public:
   ErrorPrinter(ErrorFormat format, DiskSourceTree* tree = NULL)
-      : format_(format), tree_(tree), found_errors_(false) {}
+      : format_(format), tree_(tree), found_errors_(false), found_warnings_(false) {}
   ~ErrorPrinter() {}
 
   // implements MultiFileErrorCollector ------------------------------
@@ -300,6 +300,7 @@ class CommandLineInterface::ErrorPrinter
 
   void AddWarning(const std::string& filename, int line, int column,
                   const std::string& message) {
+    found_warnings_ = true;
     AddErrorOrWarning(filename, line, column, message, "warning", std::clog);
   }
 
@@ -327,6 +328,8 @@ class CommandLineInterface::ErrorPrinter
 
   bool FoundErrors() const { return found_errors_; }
 
+  bool FoundWarnings() const { return found_warnings_; }
+
  private:
   void AddErrorOrWarning(const std::string& filename, int line, int column,
                          const std::string& message, const std::string& type,
@@ -365,6 +368,7 @@ class CommandLineInterface::ErrorPrinter
   const ErrorFormat format_;
   DiskSourceTree* tree_;
   bool found_errors_;
+  bool found_warnings_;
 };
 
 // -------------------------------------------------------------------
@@ -1117,7 +1121,8 @@ int CommandLineInterface::Run(int argc, const char* const argv[]) {
     }
   }
 
-  if (error_collector->FoundErrors()) {
+  if (error_collector->FoundErrors() ||
+      (fatal_warnings_ && error_collector->FoundWarnings())) {
     return 1;
   }
 
@@ -1630,7 +1635,8 @@ bool CommandLineInterface::ParseArgument(const char* arg, std::string* name,
       *name == "--version" || *name == "--decode_raw" ||
       *name == "--print_free_field_numbers" ||
       *name == "--experimental_allow_proto3_optional" ||
-      *name == "--deterministic_output") {
+      *name == "--deterministic_output" ||
+      *name == "--fatal_warnings") {
     // HACK:  These are the only flags that don't take a value.
     //   They probably should not be hard-coded like this but for now it's
     //   not worth doing better.
@@ -1883,6 +1889,12 @@ CommandLineInterface::InterpretArgument(const std::string& name,
       return PARSE_ARGUMENT_FAIL;
     }
 
+  } else if (name == "--fatal_warnings") {
+    if (fatal_warnings_) {
+      std::cerr << name << " may only be passed once." << std::endl;
+      return PARSE_ARGUMENT_FAIL;
+    }
+    fatal_warnings_ = true;
   } else if (name == "--plugin") {
     if (plugin_prefix_.empty()) {
       std::cerr << "This compiler does not support plugins." << std::endl;

diff --git a/src/google/protobuf/compiler/command_line_interface.h b/src/google/protobuf/compiler/command_line_interface.h
@@ -392,6 +392,9 @@ class PROTOC_EXPORT CommandLineInterface {
 
   ErrorFormat error_format_ = ERROR_FORMAT_GCC;
 
+  // True if we should treat warnings as errors that fail the compilation.
+  bool fatal_warnings_;
+
   std::vector<std::pair<std::string, std::string> >
       proto_path_;                        // Search path for proto files.
   std::vector<std::string> input_files_;  // Names of the input proto files.

diff --git a/src/google/protobuf/compiler/command_line_interface_unittest.cc b/src/google/protobuf/compiler/command_line_interface_unittest.cc
@@ -132,6 +132,9 @@ class CommandLineInterfaceTest : public testing::Test {
   // -----------------------------------------------------------------
   // Methods to check the test results (called after Run()).
 
+  // Checks that Run() returned code r.
+  void ExpectReturnCode(int r);
+
   // Checks that no text was written to stderr during Run(), and Run()
   // returned 0.
   void ExpectNoErrors();
@@ -406,6 +409,10 @@ void CommandLineInterfaceTest::CreateTempDir(const std::string& name) {
 
 // -------------------------------------------------------------------
 
+void CommandLineInterfaceTest::ExpectReturnCode(int r) {
+  EXPECT_EQ(r, return_code_);
+}
+
 void CommandLineInterfaceTest::ExpectNoErrors() {
   EXPECT_EQ(0, return_code_);
   EXPECT_EQ("", error_text_);
@@ -2309,12 +2316,32 @@ TEST_F(CommandLineInterfaceTest, InvalidErrorFormat) {
                  "syntax = \"proto2\";\n"
                  "badsyntax\n");
 
-  Run("protocol_compiler --test_out=$tmpdir "
-      "--proto_path=$tmpdir --error_format=invalid foo.proto");
+  Run("protocol_compiler --test_out=$tmpdir --proto_path=$tmpdir foo.proto");
 
   ExpectErrorText("Unknown error format: invalid\n");
 }
 
+TEST_F(CommandLineInterfaceTest, Warnings) {
+  // Test --fatal_warnings.
+
+  CreateTempFile("foo.proto",
+    "syntax = \"proto2\";\n"
+    "import \"bar.proto\";\n");
+  CreateTempFile("bar.proto",
+    "syntax = \"proto2\";\n");
+
+  Run("protocol_compiler --test_out=$tmpdir "
+    "--proto_path=$tmpdir foo.proto");
+  ExpectReturnCode(0);
+  ExpectErrorSubstringWithZeroReturnCode(
+    "foo.proto: warning: Import bar.proto but not used.");
+
+  Run("protocol_compiler --test_out=$tmpdir --fatal_warnings "
+    "--proto_path=$tmpdir foo.proto");
+  ExpectErrorSubstring(
+    "foo.proto: warning: Import bar.proto but not used.");
+}
+
 // -------------------------------------------------------------------
 // Flag parsing tests