Consistently handle clang-tidy check names in ClangTidyError.

[alexfh]

Summary

This patch removes " [check-name]" from the end of
ClangTidyMessage::Message. The " [check-name]" part is only appended when
printing diagnostics on the console. Clang errors are now marked with
"clang-diagnostic-error" check name, remarks and unknown warnings are marked
with "clang-diagnostic-unknown".

[djasper]

clang-tidy/ClangTidyDiagnosticConsumer.cpp

278

So, this is true if Context.getCheckName(Info.getID()) returns "", right? In which cases does this happen?

298

nit: "Don't mute" does not seem to be exactly what this does. It seems to be more of a "Force reporting of".

[alexfh]

Added a comment.

clang-tidy/ClangTidyDiagnosticConsumer.cpp

278

Context.getCheckName returns non-empty string iff the diagnostic was generated using Context.diag, i.e. for all clang-tidy diagnostics. Compiler diagnostics may have warning option, which will be returned by getWarningOptionForDiag. If a compiler diagnostic doesn't have a warning option, CheckName remains empty. This is (almost?) always true for compiler errors (except for the ones mapped with -Werror). I've added a comment to the code to clarify this a bit.

298

Done.

[djasper]

Looks good!

clang-tidy/ClangTidy.cpp

ErrorReporter(bool ApplyFixes)
         Diags(IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs), &*DiagOpts,
               DiagPrinter),
         SourceMgr(Diags, Files), Rewrite(SourceMgr, LangOpts),
         ApplyFixes(ApplyFixes), TotalFixes(0), AppliedFixes(0) {
     DiagOpts->ShowColors = llvm::sys::Process::StandardOutHasColors();
     DiagPrinter->BeginSourceFile(LangOpts);
   }
 
-  void reportDiagnostic(const ClangTidyMessage &Message,
-                        DiagnosticsEngine::Level Level,
-                        const tooling::Replacements *Fixes = nullptr) {
+  void reportDiagnostic(const ClangTidyError &Error) {
+    const ClangTidyMessage &Message = Error.Message;
     SourceLocation Loc = getLocation(Message.FilePath, Message.FileOffset);
     // Contains a pair for each attempted fix: location and whether the fix was
     // applied successfully.
     SmallVector<std::pair<SourceLocation, bool>, 4> FixLocations;
     {
+      auto Level = static_cast<DiagnosticsEngine::Level>(Error.DiagLevel);
       DiagnosticBuilder Diag =
-          Diags.Report(Loc, Diags.getCustomDiagID(Level, "%0"))
-          << Message.Message;
-      if (Fixes != nullptr) {
-        for (const tooling::Replacement &Fix : *Fixes) {
-          SourceLocation FixLoc =
-              getLocation(Fix.getFilePath(), Fix.getOffset());
-          SourceLocation FixEndLoc = FixLoc.getLocWithOffset(Fix.getLength());
-          Diag << FixItHint::CreateReplacement(SourceRange(FixLoc, FixEndLoc),
-                                               Fix.getReplacementText());
-          ++TotalFixes;
-          if (ApplyFixes) {
-            bool Success = Fix.isApplicable() && Fix.apply(Rewrite);
-            if (Success)
-              ++AppliedFixes;
-            FixLocations.push_back(std::make_pair(FixLoc, Success));
-          }
-        }
-      }
-    }
+          Diags.Report(Loc, Diags.getCustomDiagID(Level, "%0 [%1]"))
+          << Message.Message << Error.CheckName;
+      for (const tooling::Replacement &Fix : Error.Fix) {
+        SourceLocation FixLoc = getLocation(Fix.getFilePath(), Fix.getOffset());
     for (auto Fix : FixLocations) {
       Diags.Report(Fix.first, Fix.second ? diag::note_fixit_applied
                                          : diag::note_fixit_failed);
     }
+    for (const ClangTidyMessage &Note : Error.Notes)
+      reportNote(Note);
   }
 
   void Finish() {
     // FIXME: Run clang-format on changes.
     if (ApplyFixes && TotalFixes > 0) {
       llvm::errs() << "clang-tidy applied " << AppliedFixes << " of "
                    << TotalFixes << " suggested fixes.\n";
       Rewrite.overwriteChangedFiles();
     }
   }
 
 private:
   SourceLocation getLocation(StringRef FilePath, unsigned Offset) {
     if (FilePath.empty())
       return SourceLocation();
 
     const FileEntry *File = SourceMgr.getFileManager().getFile(FilePath);
     FileID ID = SourceMgr.createFileID(File, SourceLocation(), SrcMgr::C_User);
     return SourceMgr.getLocForStartOfFile(ID).getLocWithOffset(Offset);
   }
 
+  void reportNote(const ClangTidyMessage &Message) {
+    SourceLocation Loc = getLocation(Message.FilePath, Message.FileOffset);
+    DiagnosticBuilder Diag =
+        Diags.Report(Loc, Diags.getCustomDiagID(DiagnosticsEngine::Note, "%0"))
+        << Message.Message;
+  }
+
   FileManager Files;
   LangOptions LangOpts; // FIXME: use langopts from each original file
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts;
   DiagnosticConsumer *DiagPrinter;
   DiagnosticsEngine Diags;
   SourceManager SourceMgr;
   Rewriter Rewrite;
   bool ApplyFixes;
ClangTidyStats runClangTidy(ClangTidyOptionsProvider *OptionsProvider,
 
   Tool.run(new ActionFactory(new ClangTidyASTConsumerFactory(Context)));
   *Errors = Context.getErrors();
   return Context.getStats();
 }
 
 void handleErrors(const std::vector<ClangTidyError> &Errors, bool Fix) {
   ErrorReporter Reporter(Fix);
-  for (const ClangTidyError &Error : Errors) {
-    Reporter.reportDiagnostic(
-        Error.Message, static_cast<DiagnosticsEngine::Level>(Error.DiagLevel),
-        &Error.Fix);
-    for (const ClangTidyMessage &Note : Error.Notes)
-      Reporter.reportDiagnostic(Note, DiagnosticsEngine::Note);
-  }
+  for (const ClangTidyError &Error : Errors)
+    Reporter.reportDiagnostic(Error);
   Reporter.Finish();
 }
 
 } // namespace tidy
 } // namespace clang

clang-tidy/ClangTidyDiagnosticConsumer.cpp

ClangTidyDiagnosticRenderer(const LangOptions &LangOpts,
       : DiagnosticRenderer(LangOpts, DiagOpts), Error(Error) {}
 
 protected:
   void emitDiagnosticMessage(SourceLocation Loc, PresumedLoc PLoc,
                              DiagnosticsEngine::Level Level, StringRef Message,
                              ArrayRef<CharSourceRange> Ranges,
                              const SourceManager *SM,
                              DiagOrStoredDiag Info) override {
+    // Remove check name from the message.
+    // FIXME: Remove this once there's a better way to pass check names than
+    // appending the check name to the message in ClangTidyContext::diag and
+    // using getCustomDiagID.
+    std::string CheckNameInMessage = " [" + Error.CheckName + "]";
+    if (Message.endswith(CheckNameInMessage))
+      Message = Message.substr(0, Message.size() - CheckNameInMessage.size());
+
     ClangTidyMessage TidyMessage = Loc.isValid()
                                        ? ClangTidyMessage(Message, *SM, Loc)
                                        : ClangTidyMessage(Message);
     if (Level == DiagnosticsEngine::Note) {
       Error.Notes.push_back(TidyMessage);
       return;
     }
     assert(Error.Message.Message.empty() &&

 }
 
 void ClangTidyDiagnosticConsumer::HandleDiagnostic(
     DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info) {
   if (DiagLevel == DiagnosticsEngine::Note) {
     assert(!Errors.empty() &&
            "A diagnostic note can only be appended to a message.");
   } else {
-    // FIXME: Pass all errors here regardless of filters and non-user code.
     finalizeLastError();
     StringRef WarningOption =
         Context.DiagEngine->getDiagnosticIDs()->getWarningOptionForDiag(
             Info.getID());
     std::string CheckName = !WarningOption.empty()
                                 ? ("clang-diagnostic-" + WarningOption).str()
                                 : Context.getCheckName(Info.getID()).str();
 
+    if (CheckName.empty()) {
+      // This is a compiler diagnostic without a warning option. Assign check
+      // name based on its level.
+      switch (DiagLevel) {
+        case DiagnosticsEngine::Error:
+        case DiagnosticsEngine::Fatal:
+          CheckName = "clang-diagnostic-error";
+          break;
+        case DiagnosticsEngine::Warning:
+          CheckName = "clang-diagnostic-warning";
+          break;
+        default:
+          CheckName = "clang-diagnostic-unknown";
+          break;
+      }
+    }
+
     ClangTidyError::Level Level = ClangTidyError::Warning;
     if (DiagLevel == DiagnosticsEngine::Error ||
         DiagLevel == DiagnosticsEngine::Fatal) {
+      // Force reporting of Clang errors regardless of filters and non-user
+      // code.
       Level = ClangTidyError::Error;
       LastErrorRelatesToUserCode = true;
       LastErrorPassesLineFilter = true;
     }
     Errors.push_back(ClangTidyError(CheckName, Level));
   }
 
   // FIXME: Provide correct LangOptions for each file.

test/clang-tidy/diagnostic.cpp

 // RUN: clang-tidy -checks='-*,misc-use-override' %s.nonexistent.cpp -- | FileCheck -check-prefix=CHECK1 %s
 // RUN: clang-tidy -checks='google-explicit-constructor' %s -- -fan-unknown-option | FileCheck -check-prefix=CHECK2 %s
 // RUN: clang-tidy -checks='-*,google-explicit-constructor,clang-diagnostic-literal-conversion' %s -- -fan-unknown-option | FileCheck -check-prefix=CHECK3 %s
 // RUN: clang-tidy -checks='-*,misc-use-override,clang-diagnostic-macro-redefined' %s -- -DMACRO_FROM_COMMAND_LINE | FileCheck -check-prefix=CHECK4 %s
 
 // CHECK1-NOT: warning
 // CHECK2-NOT: warning
 // CHECK3-NOT: warning
 
-// CHECK1: error: error reading '{{.*}}.nonexistent.cpp'
-// CHECK2: error: unknown argument: '-fan-unknown-option'
+// CHECK1: error: error reading '{{.*}}.nonexistent.cpp' [clang-diagnostic-error]
+// CHECK2: error: unknown argument: '-fan-unknown-option' [clang-diagnostic-error]
 
 // CHECK2: :[[@LINE+2]]:9: warning: implicit conversion from 'double' to 'int' changes value
 // CHECK3: :[[@LINE+1]]:9: warning: implicit conversion from 'double' to 'int' changes value
 int a = 1.5;
 
 // CHECK2: :[[@LINE+2]]:11: warning: Single-argument constructors must be explicit [google-explicit-constructor]
 // CHECK3: :[[@LINE+1]]:11: warning: Single-argument constructors must be explicit [google-explicit-constructor]
 class A { A(int) {} };
 
 // CHECK2-NOT: warning:
 // CHECK3-NOT: warning:
 
 #define MACRO_FROM_COMMAND_LINE
 // CHECK4: :[[@LINE-1]]:9: warning: 'MACRO_FROM_COMMAND_LINE' macro redefined