[clang-tidy] Move Checkname out of Diagnostic Message

[njames93]

Summary

Repurpose the FlagValue field in DiagnosticEngine to store the Checkname.
This removes the need to remove the checkname from the message when we process the diagnostic later.

I looked at a few options and FlagValue seems the simplest way to implement this.
Its only ever read when there is a warning option for the given DiagnosticID, which will never happen for clang-tidy checks.

Removing the Checkname is also a necessary first step if we ever want to support tablegen diagnostics route for clang-tidy.

[aaron.ballman]

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

170–171

[njames93]

@NoQ This may have a little impact on D95403, but from what I can see nothing mayor would conflict. It was fairly easy to fix up clang d integration.

[njames93]

Typo.

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

public:
   /// Sets ASTContext for the current translation unit.
   void setASTContext(ASTContext *Context);
 
   /// Gets the language options from the AST context.
   const LangOptions &getLangOpts() const { return LangOpts; }
 
   /// Returns the name of the clang-tidy check which produced this
   /// diagnostic ID.
-  std::string getCheckName(unsigned DiagnosticID) const;
+  static StringRef getDiagnosticName(const Diagnostic &Info,
+                                     SmallVectorImpl<char> &Buffer);
+
+  static std::string getDiagnosticName(const Diagnostic &Info);
 
   /// Returns \c true if the check is enabled for the \c CurrentFile.
   ///
   /// The \c CurrentFile can be changed using \c setCurrentFile.
   bool isCheckEnabled(StringRef CheckName) const;
 
   /// Returns \c true if the check should be upgraded to error for the
   /// \c CurrentFile.
private:
   std::unique_ptr<CachedGlobList> WarningAsErrorFilter;
 
   LangOptions LangOpts;
 
   ClangTidyStats Stats;
 
   std::string CurrentBuildDirectory;
 
-  llvm::DenseMap<unsigned, std::string> CheckNamesByDiagnosticID;
-
   bool Profile;
   std::string ProfilePrefix;
 
   bool AllowEnablingAnalyzerAlphaCheckers;
 };
 
 /// Check whether a given diagnostic should be suppressed due to the presence
 /// of a "NOLINT" suppression comment.

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

 #include "clang/Basic/DiagnosticOptions.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Frontend/DiagnosticRenderer.h"
 #include "clang/Tooling/Core/Diagnostic.h"
 #include "clang/Tooling/Core/Replacement.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/Support/FormatVariadic.h"
 #include "llvm/Support/Regex.h"
 #include <tuple>
 #include <vector>
 using namespace clang;
 using namespace tidy;
 
 namespace {
 class ClangTidyDiagnosticRenderer : public DiagnosticRenderer {
 public:
   ClangTidyDiagnosticRenderer(const LangOptions &LangOpts,
                               DiagnosticOptions *DiagOpts,
                               ClangTidyError &Error)
       : DiagnosticRenderer(LangOpts, DiagOpts), Error(Error) {}
 
 protected:
   void emitDiagnosticMessage(FullSourceLoc Loc, PresumedLoc PLoc,
                              DiagnosticsEngine::Level Level, StringRef Message,
                              ArrayRef<CharSourceRange> Ranges,
                              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.DiagnosticName + "]";
-    if (Message.endswith(CheckNameInMessage))
-      Message = Message.substr(0, Message.size() - CheckNameInMessage.size());
-
     auto TidyMessage =
         Loc.isValid()
             ? tooling::DiagnosticMessage(Message, Loc.getManager(), Loc)
             : tooling::DiagnosticMessage(Message);
     if (Level == DiagnosticsEngine::Note) {
       Error.Notes.push_back(TidyMessage);
       return;
     }
: DiagEngine(nullptr), OptionsProvider(std::move(OptionsProvider)),
       AllowEnablingAnalyzerAlphaCheckers(AllowEnablingAnalyzerAlphaCheckers) {
   // Before the first translation unit we can get errors related to command-line
   // parsing, use empty string for the file name in this case.
   setCurrentFile("");
 }
 
 ClangTidyContext::~ClangTidyContext() = default;
 
+static constexpr llvm::StringLiteral CheckPrefix("<Tidy>:");
+
 DiagnosticBuilder ClangTidyContext::diag(
     StringRef CheckName, SourceLocation Loc, StringRef Description,
     DiagnosticIDs::Level Level /* = DiagnosticIDs::Warning*/) {
   assert(Loc.isValid());
-  unsigned ID = DiagEngine->getDiagnosticIDs()->getCustomDiagID(
-      Level, (Description + " [" + CheckName + "]").str());
-  CheckNamesByDiagnosticID.try_emplace(ID, CheckName);
-  return DiagEngine->Report(Loc, ID);
+  unsigned ID =
+      DiagEngine->getDiagnosticIDs()->getCustomDiagID(Level, Description);
+  auto Res = DiagEngine->Report(Loc, ID);
+  // Repurpse the Flag value to store the check name. clang-tidy checks don't
+  // have command line flags so this field is safe to use, but add a prefix just
+  // to be sure.
+  Res << AddFlagValue(SmallString<64>{CheckPrefix, CheckName});
+  return Res;
 }
 
 DiagnosticBuilder ClangTidyContext::diag(
     StringRef CheckName, StringRef Description,
     DiagnosticIDs::Level Level /* = DiagnosticIDs::Warning*/) {
-  unsigned ID = DiagEngine->getDiagnosticIDs()->getCustomDiagID(
-      Level, (Description + " [" + CheckName + "]").str());
-  CheckNamesByDiagnosticID.try_emplace(ID, CheckName);
-  return DiagEngine->Report(ID);
+  unsigned ID =
+      DiagEngine->getDiagnosticIDs()->getCustomDiagID(Level, Description);
+  auto Res = DiagEngine->Report(ID);
+  Res.addFlagValue(SmallString<64>({CheckPrefix, CheckName}));
+  return Res;
 }
 
 DiagnosticBuilder ClangTidyContext::configurationDiag(
     StringRef Message,
     DiagnosticIDs::Level Level /* = DiagnosticIDs::Warning*/) {
   return diag("clang-tidy-config", Message, Level);
 }
 
bool ClangTidyContext::isCheckEnabled(StringRef CheckName) const {
   return CheckFilter->contains(CheckName);
 }
 
 bool ClangTidyContext::treatAsError(StringRef CheckName) const {
   assert(WarningAsErrorFilter != nullptr);
   return WarningAsErrorFilter->contains(CheckName);
 }
 
-std::string ClangTidyContext::getCheckName(unsigned DiagnosticID) const {
-  std::string ClangWarningOption = std::string(
-      DiagEngine->getDiagnosticIDs()->getWarningOptionForDiag(DiagnosticID));
-  if (!ClangWarningOption.empty())
-    return "clang-diagnostic-" + ClangWarningOption;
-  llvm::DenseMap<unsigned, std::string>::const_iterator I =
-      CheckNamesByDiagnosticID.find(DiagnosticID);
-  if (I != CheckNamesByDiagnosticID.end())
-    return I->second;
+StringRef ClangTidyContext::getDiagnosticName(const Diagnostic &Info,
+                                              SmallVectorImpl<char> &Buffer) {
+  auto CheckName = Info.getDiags()->getFlagValue();
+  if (!CheckName.empty() && CheckName.consume_front(CheckPrefix)) {
+    return CheckName;
+  }
+  StringRef ClangWarningOption =
+      DiagnosticIDs::getWarningOptionForDiag(Info.getID());
+  if (!ClangWarningOption.empty()) {
+    constexpr llvm::StringLiteral Prefix("clang-diagnostic-");
+    Buffer.resize_for_overwrite(Prefix.size() + ClangWarningOption.size());
+    memcpy(Buffer.data(), Prefix.data(), Prefix.size());
+    memcpy(Buffer.data() + Prefix.size(), ClangWarningOption.data(),
+           ClangWarningOption.size());
+    return {Buffer.begin(), Buffer.size()};
+  }
   return "";
 }
 
+std::string ClangTidyContext::getDiagnosticName(const Diagnostic &Info) {
+  SmallString<64> Buffer;
+  return getDiagnosticName(Info, Buffer).str();
+}
+
 ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer(
     ClangTidyContext &Ctx, DiagnosticsEngine *ExternalDiagEngine,
     bool RemoveIncompatibleErrors)
     : Context(Ctx), ExternalDiagEngine(ExternalDiagEngine),
       RemoveIncompatibleErrors(RemoveIncompatibleErrors),
       LastErrorRelatesToUserCode(false), LastErrorPassesLineFilter(false),
       LastErrorWasIgnored(false) {}
 
if (Error.DiagnosticName == "clang-tidy-config") {
       ++Context.Stats.ErrorsDisplayed;
     }
   }
   LastErrorRelatesToUserCode = false;
   LastErrorPassesLineFilter = false;
 }
 
 static bool IsNOLINTFound(StringRef NolintDirectiveText, StringRef Line,
-                          unsigned DiagID, const ClangTidyContext &Context) {
+                          StringRef CheckName,
+                          const ClangTidyContext &Context) {
   const size_t NolintIndex = Line.find(NolintDirectiveText);
   if (NolintIndex == StringRef::npos)
     return false;
 
   size_t BracketIndex = NolintIndex + NolintDirectiveText.size();
   // Check if the specific checks are specified in brackets.
   if (BracketIndex < Line.size() && Line[BracketIndex] == '(') {
     ++BracketIndex;
     const size_t BracketEndIndex = Line.find(')', BracketIndex);
     if (BracketEndIndex != StringRef::npos) {
       StringRef ChecksStr =
           Line.substr(BracketIndex, BracketEndIndex - BracketIndex);
       // Allow disabling all the checks with "*".
       if (ChecksStr != "*") {
-        std::string CheckName = Context.getCheckName(DiagID);
         // Allow specifying a few check names, delimited with comma.
         SmallVector<StringRef, 1> Checks;
         ChecksStr.split(Checks, ',', -1, false);
         llvm::transform(Checks, Checks.begin(),
                         [](StringRef S) { return S.trim(); });
         return llvm::find(Checks, CheckName) != Checks.end();
       }
     }
   }
   return true;
 }
 
 static llvm::Optional<StringRef> getBuffer(const SourceManager &SM, FileID File,
                                            bool AllowIO) {
   return AllowIO ? SM.getBufferDataOrNone(File)
                  : SM.getBufferDataIfLoaded(File);
 }
 
 static bool LineIsMarkedWithNOLINT(const SourceManager &SM, SourceLocation Loc,
-                                   unsigned DiagID,
+                                   StringRef CheckName,
                                    const ClangTidyContext &Context,
                                    bool AllowIO) {
   FileID File;
   unsigned Offset;
   std::tie(File, Offset) = SM.getDecomposedSpellingLoc(Loc);
   llvm::Optional<StringRef> Buffer = getBuffer(SM, File, AllowIO);
   if (!Buffer)
     return false;
 
   // Check if there's a NOLINT on this line.
   StringRef RestOfLine = Buffer->substr(Offset).split('\n').first;
-  if (IsNOLINTFound("NOLINT", RestOfLine, DiagID, Context))
+  if (IsNOLINTFound("NOLINT", RestOfLine, CheckName, Context))
     return true;
 
   // Check if there's a NOLINTNEXTLINE on the previous line.
   StringRef PrevLine =
       Buffer->substr(0, Offset).rsplit('\n').first.rsplit('\n').second;
-  return IsNOLINTFound("NOLINTNEXTLINE", PrevLine, DiagID, Context);
+  return IsNOLINTFound("NOLINTNEXTLINE", PrevLine, CheckName, Context);
 }
 
 static bool LineIsMarkedWithNOLINTinMacro(const SourceManager &SM,
-                                          SourceLocation Loc, unsigned DiagID,
+                                          SourceLocation Loc,
+                                          StringRef CheckName,
                                           const ClangTidyContext &Context,
                                           bool AllowIO) {
   while (true) {
-    if (LineIsMarkedWithNOLINT(SM, Loc, DiagID, Context, AllowIO))
+    if (LineIsMarkedWithNOLINT(SM, Loc, CheckName, Context, AllowIO))
       return true;
     if (!Loc.isMacroID())
       return false;
     Loc = SM.getImmediateExpansionRange(Loc).getBegin();
   }
   return false;
 }
 
 namespace clang {
 namespace tidy {
 
 bool shouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
                               const Diagnostic &Info, ClangTidyContext &Context,
                               bool AllowIO) {
+  SmallString<64> Buffer;
   return Info.getLocation().isValid() &&
          DiagLevel != DiagnosticsEngine::Error &&
          DiagLevel != DiagnosticsEngine::Fatal &&
-         LineIsMarkedWithNOLINTinMacro(Info.getSourceManager(),
-                                       Info.getLocation(), Info.getID(),
-                                       Context, AllowIO);
+         LineIsMarkedWithNOLINTinMacro(
+             Info.getSourceManager(), Info.getLocation(),
+             ClangTidyContext::getDiagnosticName(Info, Buffer), Context,
+             AllowIO);
 }
 
 } // namespace tidy
 } // namespace clang
 
 void ClangTidyDiagnosticConsumer::HandleDiagnostic(
     DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info) {
   if (LastErrorWasIgnored && DiagLevel == DiagnosticsEngine::Note)
void ClangTidyDiagnosticConsumer::HandleDiagnostic(
   // Count warnings/errors.
   DiagnosticConsumer::HandleDiagnostic(DiagLevel, Info);
 
   if (DiagLevel == DiagnosticsEngine::Note) {
     assert(!Errors.empty() &&
            "A diagnostic note can only be appended to a message.");
   } else {
     finalizeLastError();
-    std::string CheckName = Context.getCheckName(Info.getID());
+    SmallString<64> Buffer;
+    llvm::StringRef CheckName =
+        ClangTidyContext::getDiagnosticName(Info, Buffer);
     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;

clang-tools-extra/clangd/Diagnostics.cpp

if (const char *ClangDiag = getDiagnosticCode(Diag.ID)) {
         // Drop the err_ prefix for brevity.
         Name.consume_front("err_");
         Diag.Name = std::string(Name);
       }
       Diag.Source = Diag::Clang;
       continue;
     }
     if (Tidy != nullptr) {
-      std::string TidyDiag = Tidy->getCheckName(Diag.ID);
-      if (!TidyDiag.empty()) {
-        Diag.Name = std::move(TidyDiag);
+      if (!Diag.Name.empty()) {
         Diag.Source = Diag::ClangTidy;
-        // clang-tidy bakes the name into diagnostic messages. Strip it out.
-        // It would be much nicer to make clang-tidy not do this.
-        auto CleanMessage = [&](std::string &Msg) {
-          StringRef Rest(Msg);
-          if (Rest.consume_back("]") && Rest.consume_back(Diag.Name) &&
-              Rest.consume_back(" ["))
-            Msg.resize(Rest.size());
-        };
-        CleanMessage(Diag.Message);
-        for (auto &Note : Diag.Notes)
-          CleanMessage(Note.Message);
-        for (auto &Fix : Diag.Fixes)
-          CleanMessage(Fix.Message);
-        continue;
       }
     }
   }
   // Deduplicate clang-tidy diagnostics -- some clang-tidy checks may emit
   // duplicated messages due to various reasons (e.g. the check doesn't handle
   // template instantiations well; clang-tidy alias checks).
   std::set<std::pair<Range, std::string>> SeenDiags;
   llvm::erase_if(Output, [&](const Diag& D) {
if (Info.getLocation().isInvalid()) {
     }
 
     flushLastDiag();
 
     LastDiag = Diag();
     LastDiagLoc.reset();
     LastDiagOriginallyError = OriginallyError;
     LastDiag->ID = Info.getID();
+    LastDiag->Name = tidy::ClangTidyContext::getDiagnosticName(Info);
     fillNonLocationData(DiagLevel, Info, *LastDiag);
     LastDiag->InsideMainFile = true;
     // Put it at the start of the main file, for a lack of a better place.
     LastDiag->Range.start = Position{0, 0};
     LastDiag->Range.end = Position{0, 0};
     return;
   }
 
if (Adjuster) {
         LastPrimaryDiagnosticWasSuppressed = true;
         return;
       }
     }
     LastPrimaryDiagnosticWasSuppressed = false;
 
     LastDiag = Diag();
     FillDiagBase(*LastDiag);
+    LastDiag->Name = tidy::ClangTidyContext::getDiagnosticName(Info);
     LastDiagLoc.emplace(Info.getLocation(), Info.getSourceManager());
     LastDiagOriginallyError = OriginallyError;
 
     if (!Info.getFixItHints().empty())
       AddFix(true /* try to invent a message instead of repeating the diag */);
     if (Fixer) {
       auto ExtraFixes = Fixer(DiagLevel, Info);
       LastDiag->Fixes.insert(LastDiag->Fixes.end(), ExtraFixes.begin(),

clang-tools-extra/clangd/ParsedAST.cpp

llvm::Optional<tidy::ClangTidyContext> CTContext;
 
     const Config& Cfg = Config::current();
     ASTDiags.setLevelAdjuster([&](DiagnosticsEngine::Level DiagLevel,
                                   const clang::Diagnostic &Info) {
       if (Cfg.Diagnostics.SuppressAll ||
           isBuiltinDiagnosticSuppressed(Info.getID(), Cfg.Diagnostics.Suppress))
         return DiagnosticsEngine::Ignored;
       if (!CTChecks.empty()) {
-        std::string CheckName = CTContext->getCheckName(Info.getID());
-        bool IsClangTidyDiag = !CheckName.empty();
-        if (IsClangTidyDiag) {
+        SmallString<64> Buffer;
+        auto CheckName =
+            tidy::ClangTidyContext::getDiagnosticName(Info, Buffer);
+        if (!CheckName.empty()) {
           if (Cfg.Diagnostics.Suppress.contains(CheckName))
             return DiagnosticsEngine::Ignored;
           // Check for suppression comment. Skip the check for diagnostics not
           // in the main file, because we don't want that function to query the
           // source buffer for preamble files. For the same reason, we ask
           // shouldSuppressDiagnostic to avoid I/O.
           // We let suppression comments take precedence over warning-as-error
           // to match clang-tidy's behaviour.