Teach clang-tidy how to -Werror checks.

[aaron.ballman]

Aside from two tiny nits, LGTM. Thank you for working on this, it's a great feature! I think we should add something to the release notes to make sure it gets called out.

tools/llvm-project/extra/clang-tidy/ClangTidy.cpp

156

Method can be const.

tools/llvm-project/extra/test/clang-tidy/werrors-plural.cpp

7

One of these tests should complete the -Werrors= chunk just to be sure it's getting printed properly.

[jroelofs]

tools/llvm-project/extra/clang-tidy/ClangTidy.cpp

119

hardcoding happens here ^

156

ok

tools/llvm-project/extra/test/clang-tidy/werrors-plural.cpp

7

Not sure what you mean here. Should I be printing the -Werrors argument as the user wrote it? (it isn't currently... this is just a hardcoded string).

[aaron.ballman]

tools/llvm-project/extra/test/clang-tidy/werrors-plural.cpp

7

Ah, I didn't pick up on that. What is the = for, then?

[alexfh]

Jonathan, can you explain what specific use case does this patch address? Why one severity level of native clang-tidy warnings (the current situation) is not enough, and two levels are enough?

[jroelofs]

In D15528#311019, @alexfh wrote:

Jonathan, can you explain what specific use case does this patch address? Why one severity level of native clang-tidy warnings (the current situation) is not enough, and two levels are enough?

I have out-of-tree checkers for a very strange out-of-tree target. Some of the checks are on the level of "this should break the build because it cannot possibly work on this target" and others on the level of "tell me about it, but don't force me to fix it". All of these checks are things that don't even remotely apply to other targets.

If you're wondering why I haven't hacked up Clang's sema to enforce these constraints, see again: out-of-tree backend... maintaining OOT changes there is expected to be very difficult. Clang-tidy however provides a very nice framework where they can be kept neatly off to the side, away from most of the merge hassles.

It'd be nice not to have to run clang-tidy twice & parse its output in order to achieve all of that, hence this patch.

tools/llvm-project/extra/test/clang-tidy/werrors-plural.cpp

7

I considered that part of the name of the flag.

I can drop that if you think it looks better. Or print out the whole thing as the user wrote it, if you think that's useful (though -checks= and -Werrors= lines are probably pretty long...).

[aaron.ballman]

tools/llvm-project/extra/test/clang-tidy/werrors-plural.cpp

7

I would just drop the =; it seems like that suggests there should be more text there. That would also be consistent with the way clang reports -Werror diagnostics (error: unused variable 'i' [-Werror,-Wunused-variable])

[alexfh]

In D15528#311053, @jroelofs wrote:

In D15528#311019, @alexfh wrote:

Jonathan, can you explain what specific use case does this patch address? Why one severity level of native clang-tidy warnings (the current situation) is not enough, and two levels are enough?

I have out-of-tree checkers for a very strange out-of-tree target. Some of the checks are on the level of "this should break the build because it cannot possibly work on this target" and others on the level of "tell me about it, but don't force me to fix it". All of these checks are things that don't even remotely apply to other targets.

Thank you for the explanation. One more question: do you need to define Werrors differently in different directories?

If you're wondering why I haven't hacked up Clang's sema to enforce these constraints, see again: out-of-tree backend... maintaining OOT changes there is expected to be very difficult.

No, a sane person wouldn't suggest maintaining a local patch for Clang as a good solution ;)

Clang-tidy however provides a very nice framework where they can be kept neatly off to the side, away from most of the merge hassles.

It's one of the goals of clang-tidy to provide an easy way to maintain out-of-tree checks.

It'd be nice not to have to run clang-tidy twice & parse its output in order to achieve all of that, hence this patch.

Agreed, I want to ensure though, that this is the right approach. In particular, I wonder whether a way to assign labels or severities to clang-tidy diagnostics would be better. Another question is whether we can reuse something from the Clang diagnostic subsystem to map warning levels.

[jroelofs]

In D15528#311135, @alexfh wrote:

In D15528#311053, @jroelofs wrote:

In D15528#311019, @alexfh wrote:

Jonathan, can you explain what specific use case does this patch address? Why one severity level of native clang-tidy warnings (the current situation) is not enough, and two levels are enough?

I have out-of-tree checkers for a very strange out-of-tree target. Some of the checks are on the level of "this should break the build because it cannot possibly work on this target" and others on the level of "tell me about it, but don't force me to fix it". All of these checks are things that don't even remotely apply to other targets.

Thank you for the explanation. One more question: do you need to define Werrors differently in different directories?

Yeah, for things like "if you do this, you will get terribad performance", it's useful to be able to let the user's build system control them independently with that kind of granularity. In some places it's acceptable to fail these checks (i.e. while porting code to this target), and in others it's mission-critical.

I could see this being useful in other contexts too. For example, hypothetically, one could imagine turning on the llvm-include-order check for all of llvm, and then making it -Werror for subdirectories where it has been cleaned up, ensuring monotonic progress.

If you're wondering why I haven't hacked up Clang's sema to enforce these constraints, see again: out-of-tree backend... maintaining OOT changes there is expected to be very difficult.

No, a sane person wouldn't suggest maintaining a local patch for Clang as a good solution ;)

:)

Clang-tidy however provides a very nice framework where they can be kept neatly off to the side, away from most of the merge hassles.

It's one of the goals of clang-tidy to provide an easy way to maintain out-of-tree checks.

Seems to be working out very well on my end... thanks for designing it that way!

It'd be nice not to have to run clang-tidy twice & parse its output in order to achieve all of that, hence this patch.

Agreed, I want to ensure though, that this is the right approach. In particular, I wonder whether a way to assign labels or severities to clang-tidy diagnostics would be better. Another question is whether we can reuse something from the Clang diagnostic subsystem to map warning levels.

For my purposes, this kind of trimodal { "it's definitely broken, and cannot possibly work", "hey, have you considered this?", "the tool didn't spot any problems" } granularity seems like the right fit, and lines up with Clang's diagnostics: notes usually stay notes, remarks stay remarks, warnings are sometimes errors and other times just warnings, and errors are errors.

Something else to think about: grouping a la -Wpedantic/-Wall/-Weverything might be useful. Some of that can be achieved by giving structure to the names of the checks, i.e.:

• footarget-perf-check1
• footarget-perf-check2
• footarget-correctness-check1
• footarget-correctness-check2
• footarget-portability-check1
  
  (pardon my vagueness here)

But that kind of breaks down when you start to talk about putting existing upstream checks into these groups, because then you have to write things like: `-checks='-*,footarget-portability-*,google-runtime-int-std' (off the cuff example). Not sure if that's what you meant by "labels", but I see that as being orthogonal to the question of what a user should be forced to take action on.

tools/llvm-project/extra/test/clang-tidy/werrors-plural.cpp

7

ok, sounds good.

[alexfh]

Thank you for explaining. The use case seems to be important enough to support it. And the solution seems to be good for now. A few concerns:

1. Werrors isn't a good name for this. The only reason why a similar thing is called -Werror in compilers is that they use -W for warnings. I'd suggest TreatAsErrors, WarningsAsErrors or anything similar (with the proper casing change for the command line argument).
2. Documentation is missing.

[jroelofs]

Thanks for the review! I'll rework this a bit early next week.

[jroelofs]

Address review comments.

[jroelofs]

In D15528#312031, @alexfh wrote:

Thank you for explaining. The use case seems to be important enough to support it. And the solution seems to be good for now. A few concerns:

1. Werrors isn't a good name for this. The only reason why a similar thing is called -Werror in compilers is that they use -W for warnings. I'd suggest TreatAsErrors, WarningsAsErrors or anything similar (with the proper casing change for the command line argument).

Renamed.

2. Documentation is missing.

Working on that now.

[jroelofs]

Added docs.

[alexfh]

Looks good with a couple of nits. I'll be happy to submit your patch once you address the comments.

Also, please include more context in your patches. See http://llvm.org/docs/Phabricator.html#requesting-a-review-via-the-web-interface.

clang-tidy/ClangTidy.cpp

156 ↗

(On Diff #44650)

I'd call it getWarningsAsErrorsCount()

clang-tidy/ClangTidyOptions.cpp

130 ↗

(On Diff #44650)

This line exceeds column limit. Please run clang-format on the changed files.

[jroelofs]

clang-format the patch, and rename method.

[jroelofs]

Thanks for the review! Committed r257642.

[jroelofs]

Hmm. There's no "close revision" button. Abandoning it in lieu of closing it.

[alexfh]

In D15528#326031, @jroelofs wrote:

Hmm. There's no "close revision" button. Abandoning it in lieu of closing it.

Weird. That's probably because I've not marked it "Accepted". Now I tried a few other actions on the revision and it now shows as authored by myself. Really weird.

Anyway, thank you for working on this!

[njames93]

clang-tidy/tool/ClangTidyMain.cpp

362 ↗

(On Diff #44759)

Was there any specific reason for returning the error count instead of returning 1. It results in undefined behaviour on POSIX shells if a value is returned outside the range of 0<=n<=255. See https://bugs.llvm.org/show_bug.cgi?id=46305

tools/llvm-project/extra/clang-tidy/ClangTidy.h

 //
 /// \brief Displays the found \p Errors to the users. If \p Fix is true, \p
 /// Errors containing fixes are automatically applied.
-void handleErrors(const std::vector<ClangTidyError> &Errors, bool Fix);
+void handleErrors(const std::vector<ClangTidyError> &Errors, bool Fix, unsigned &WErrorCount);
 
 /// \brief Serializes replacements into YAML and writes them to the specified
 /// output stream.

tools/llvm-project/extra/clang-tidy/ClangTidy.cpp

         Diags(IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs), &*DiagOpts,
               DiagPrinter),
         SourceMgr(Diags, Files), Rewrite(SourceMgr, LangOpts),
-        ApplyFixes(ApplyFixes), TotalFixes(0), AppliedFixes(0) {
+        ApplyFixes(ApplyFixes), TotalFixes(0), AppliedFixes(0), WErrors(0) {
     DiagOpts->ShowColors = llvm::sys::Process::StandardOutHasColors();
     DiagPrinter->BeginSourceFile(LangOpts);
   }

     SmallVector<std::pair<SourceLocation, bool>, 4> FixLocations;
     {
       auto Level = static_cast<DiagnosticsEngine::Level>(Error.DiagLevel);
+      std::string Name = Error.CheckName;
+      if (Error.IsWError) {
+        Name += ", -Werrors=";
+        Level = DiagnosticsEngine::Error;
+        WErrors++;
+      }
       auto Diag = Diags.Report(Loc, Diags.getCustomDiagID(Level, "%0 [%1]"))
-                  << Message.Message << Error.CheckName;
+                  << Message.Message << Name;
       for (const tooling::Replacement &Fix : Error.Fix) {
         SourceLocation FixLoc = getLocation(Fix.getFilePath(), Fix.getOffset());
         SourceLocation FixEndLoc = FixLoc.getLocWithOffset(Fix.getLength());

     }
   }
 
+  unsigned WErrorCount() { return WErrors; }
+
 private:
   SourceLocation getLocation(StringRef FilePath, unsigned Offset) {
     if (FilePath.empty())

   bool ApplyFixes;
   unsigned TotalFixes;
   unsigned AppliedFixes;
+  unsigned WErrors;
 };
 
 class ClangTidyASTConsumer : public MultiplexConsumer {

   return Context.getStats();
 }
 
-void handleErrors(const std::vector<ClangTidyError> &Errors, bool Fix) {
+void handleErrors(const std::vector<ClangTidyError> &Errors, bool Fix, unsigned &WErrorCount) {
   ErrorReporter Reporter(Fix);
   for (const ClangTidyError &Error : Errors)
     Reporter.reportDiagnostic(Error);
   Reporter.Finish();
+  WErrorCount += Reporter.WErrorCount();
 }
 
 void exportReplacements(const std::vector<ClangTidyError> &Errors,

tools/llvm-project/extra/clang-tidy/ClangTidyDiagnosticConsumer.h

     Error = DiagnosticsEngine::Error
   };
 
-  ClangTidyError(StringRef CheckName, Level DiagLevel);
+  ClangTidyError(StringRef CheckName, Level DiagLevel, bool IsWError);
 
   std::string CheckName;
   ClangTidyMessage Message;

   SmallVector<ClangTidyMessage, 1> Notes;
 
   Level DiagLevel;
+  bool IsWError;
 };
 
 /// \brief Read-only set of strings represented as a list of positive and

   /// The \c CurrentFile can be changed using \c setCurrentFile.
   GlobList &getChecksFilter();
 
+  /// \brief Returns check filter for the \c CurrentFile which
+  /// selects checks for upgrade to error.
+  GlobList &getWErrorFilter();
+
   /// \brief Returns global options.
   const ClangTidyGlobalOptions &getGlobalOptions() const;
 

   std::string CurrentFile;
   ClangTidyOptions CurrentOptions;
   std::unique_ptr<GlobList> CheckFilter;
+  std::unique_ptr<GlobList> WErrorFilter;
 
   LangOptions LangOpts;
 

tools/llvm-project/extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp

 }
 
 ClangTidyError::ClangTidyError(StringRef CheckName,
-                               ClangTidyError::Level DiagLevel)
-    : CheckName(CheckName), DiagLevel(DiagLevel) {}
+                               ClangTidyError::Level DiagLevel,
+                               bool IsWError)
+    : CheckName(CheckName), DiagLevel(DiagLevel), IsWError(IsWError) {}
 
 // Returns true if GlobList starts with the negative indicator ('-'), removes it
 // from the GlobList.

   CurrentFile = File;
   CurrentOptions = getOptionsForFile(CurrentFile);
   CheckFilter.reset(new GlobList(*getOptions().Checks));
+  WErrorFilter.reset(new GlobList(*getOptions().WErrors));
 }
 
 void ClangTidyContext::setASTContext(ASTContext *Context) {

   return *CheckFilter;
 }
 
+GlobList &ClangTidyContext::getWErrorFilter() {
+  assert(WErrorFilter != nullptr);
+  return *WErrorFilter;
+}
+
 /// \brief Store a \c ClangTidyError.
 void ClangTidyContext::storeError(const ClangTidyError &Error) {
   Errors.push_back(Error);

       LastErrorRelatesToUserCode = true;
       LastErrorPassesLineFilter = true;
     }
-    Errors.push_back(ClangTidyError(CheckName, Level));
+    bool IsWError = DiagLevel == DiagnosticsEngine::Warning &&
+                    Context.getWErrorFilter().contains(CheckName);
+    Errors.push_back(ClangTidyError(CheckName, Level, IsWError));
   }
 
   // FIXME: Provide correct LangOptions for each file.

tools/llvm-project/extra/clang-tidy/ClangTidyOptions.h

   /// \brief Checks filter.
   llvm::Optional<std::string> Checks;
 
+  /// \brief WErrors filter.
+  llvm::Optional<std::string> WErrors;
+
   /// \brief Output warnings from headers matching this filter. Warnings from
   /// main files will always be displayed.
   llvm::Optional<std::string> HeaderFilterRegex;

tools/llvm-project/extra/clang-tidy/ClangTidyOptions.cpp

     MappingNormalization<NOptionMap, ClangTidyOptions::OptionMap> NOpts(
         IO, Options.CheckOptions);
     IO.mapOptional("Checks", Options.Checks);
+    IO.mapOptional("WErrors", Options.WErrors);
     IO.mapOptional("HeaderFilterRegex", Options.HeaderFilterRegex);
     IO.mapOptional("AnalyzeTemporaryDtors", Options.AnalyzeTemporaryDtors);
     IO.mapOptional("User", Options.User);

 ClangTidyOptions ClangTidyOptions::getDefaults() {
   ClangTidyOptions Options;
   Options.Checks = "";
+  Options.WErrors = "";
   Options.HeaderFilterRegex = "";
   Options.SystemHeaders = false;
   Options.AnalyzeTemporaryDtors = false;

     Result.Checks =
         (Result.Checks && !Result.Checks->empty() ? *Result.Checks + "," : "") +
         *Other.Checks;
+  if (Other.WErrors)
+    Result.WErrors =
+        (Result.WErrors && !Result.WErrors->empty() ? *Result.WErrors + "," : "") +
+        *Other.WErrors;
 
   if (Other.HeaderFilterRegex)
     Result.HeaderFilterRegex = Other.HeaderFilterRegex;

tools/llvm-project/extra/clang-tidy/tool/ClangTidyMain.cpp

        cl::init(""), cl::cat(ClangTidyCategory));
 
 static cl::opt<std::string>
+WErrors("Werrors", cl::desc("Upgrades warnings to errors. Same format as '-checks'."),
+        cl::init(""), cl::cat(ClangTidyCategory));
+
+static cl::opt<std::string>
 HeaderFilter("header-filter",
              cl::desc("Regular expression matching the names of the\n"
                       "headers to output diagnostics from. Diagnostics\n"

 
   ClangTidyOptions DefaultOptions;
   DefaultOptions.Checks = DefaultChecks;
+  DefaultOptions.WErrors = "";
   DefaultOptions.HeaderFilterRegex = HeaderFilter;
   DefaultOptions.SystemHeaders = SystemHeaders;
   DefaultOptions.AnalyzeTemporaryDtors = AnalyzeTemporaryDtors;

   ClangTidyOptions OverrideOptions;
   if (Checks.getNumOccurrences() > 0)
     OverrideOptions.Checks = Checks;
+  if (WErrors.getNumOccurrences() > 0)
+    OverrideOptions.WErrors = WErrors;
   if (HeaderFilter.getNumOccurrences() > 0)
     OverrideOptions.HeaderFilterRegex = HeaderFilter;
   if (SystemHeaders.getNumOccurrences() > 0)

 
   const bool DisableFixes = Fix && FoundErrors && !FixErrors;
 
+  unsigned WErrorCount = 0;
+
   // -fix-errors implies -fix.
-  handleErrors(Errors, (FixErrors || Fix) && !DisableFixes);
+  handleErrors(Errors, (FixErrors || Fix) && !DisableFixes, WErrorCount);
 
   if (!ExportFixes.empty() && !Errors.empty()) {
     std::error_code EC;

   if (EnableCheckProfile)
     printProfileData(Profile, llvm::errs());
 
+  if (WErrorCount) {
+    StringRef Plural = WErrorCount == 1 ? "" : "s";
+    llvm::errs() << WErrorCount << " warning" << Plural
+                 << " treated as error" << Plural << "\n";
+    return WErrorCount;
+  }
+
   return 0;
 }
 

tools/llvm-project/extra/test/clang-tidy/werrors-plural.cpp

+// RUN: clang-tidy %s -checks='-*,llvm-namespace-comment' -- 2>&1 | FileCheck %s --check-prefix=CHECK-WARN
+// RUN: not clang-tidy %s -checks='-*,llvm-namespace-comment' -Werrors='llvm-namespace-comment' -- 2>&1 | FileCheck %s --check-prefix=CHECK-WERR
+
+namespace j {
+}
+// CHECK-WARN: warning: namespace 'j' not terminated with a closing comment [llvm-namespace-comment]
+// CHECK-WERR: error: namespace 'j' not terminated with a closing comment [llvm-namespace-comment, -Werrors=]
+
+namespace k {
+}
+// CHECK-WARN: warning: namespace 'k' not terminated with a closing comment [llvm-namespace-comment]
+// CHECK-WERR: error: namespace 'k' not terminated with a closing comment [llvm-namespace-comment, -Werrors=]
+
+// CHECK-WARN-NOT: treated as
+// CHECK-WERR: 2 warnings treated as errors

tools/llvm-project/extra/test/clang-tidy/werrors.cpp

+// RUN: clang-tidy %s -checks='-*,llvm-namespace-comment' -- 2>&1 | FileCheck %s --check-prefix=CHECK-WARN
+// RUN: not clang-tidy %s -checks='-*,llvm-namespace-comment' -Werrors='llvm-namespace-comment' -- 2>&1 | FileCheck %s --check-prefix=CHECK-WERR
+
+namespace i {
+}
+// CHECK-WARN: warning: namespace 'i' not terminated with a closing comment [llvm-namespace-comment]
+// CHECK-WERR: error: namespace 'i' not terminated with a closing comment [llvm-namespace-comment, -Werrors=]
+
+// CHECK-WARN-NOT: treated as
+// CHECK-WERR: 1 warning treated as error