[clang-tidy] Make the plugin honor NOLINT

[nik]

Summary

Instantiate a ClangTidyDiagnosticConsumer also for the plugin case and
let it forward the diagnostics to the external diagnostic engine that is
already in place.

One minor difference to the clang-tidy executable case is that the
compiler checks/diagnostics are referred to with their original name.
For example, for -Wunused-variable the plugin will refer to the check as
"-Wunused-variable" while the clang-tidy executable will refer to that
as "clang-diagnostic- unused-variable". This is because the compiler
diagnostics never reach ClangTidyDiagnosticConsumer.

[nik]

This one depends on https://reviews.llvm.org/D61486

[alexfh]

Apart from NOLINT handling there's more logic in ClangTidyDiagnosticConsumer::HandleDiagnostic, which isn't properly transferred to ClangTidyContext::diag in this patch. The logic that is transferred seems to change the behavior w.r.t. notes that can "unmute" ignored warnings (see https://reviews.llvm.org/D59135#1456108). I suspect that we're missing proper test coverage here. Another issue is that compiler diagnostics don't pass ClangTidyContext::diag in the non-plugin use case. Do all the existing tests pass with your patch?

A better way to implement diagnostic filtering in the plugin would be to make ClangTidyDiagnosticConsumer able to forward diagnostics to the external diagnostics engine. It will still need some sort of a buffering though to handle diagnostics and notes attached to them together.

[nik]

Thanks for the fast comments!

In D61487#1489308, @alexfh wrote:

I suspect that we're missing proper test coverage here.

True, ideally all the test scripts would also trigger the plugin case code path.

I've started with a modified copy of nolint.cpp as I wasn't able to have the two invocations (clang-tidy, c-index-test plugin) work with the same file. For example, the order of the diagnostics is different (seems solvable by appending using -DAG) and the c-index-test plugin case does not output "Suppressed 13 warnings (13 NOLINT)" - is there a way to exclude the check for this output for the c-index-test case and still having all in the same file?

I haven't tested the plugin case with nolintnextline.cpp yet, but at least this one does not seem to contain the unmute case as far as I see.

Another issue is that compiler diagnostics don't pass ClangTidyContext::diag in the non-plugin use case.

Right, but how is this an issue?

Do all the existing tests pass with your patch?

Yes.

A better way to implement diagnostic filtering in the plugin would be to make ClangTidyDiagnosticConsumer able to forward diagnostics to the external diagnostics engine. It will still need some sort of a buffering though to handle diagnostics and notes attached to them together.

Ahh, you suggest that the plugin should have a separate diagnostic engine and instantiate also a ClangTidyDiagnosticConsumer object, that forwards to the external one? Will check how this could work.

[nik]

The plugin makes use of ClangTidyDiagnosticConsumer and forwards diagnostics to
the external diagnostic engine.

@alexfh: What do you think? If that looks roughly OK for you, I'll finish it.

[nik]

Ping. alexfh?

[nik]

Ping.

[gribozavr]

clang-tidy/ClangTidyDiagnosticConsumer.cpp

464

Indentation is 2 spaces.

472

It seems like the checkFilters call should not be skipped even if we have another diagnostic engine.

clang-tidy/ClangTidyDiagnosticConsumer.h

197

Sorry, it is unclear what this struct is, and what its members are, especially given that the member names are L and string...

[nik]

As I've commented on, this change is not finished. However, I've addressed the inline comments nevertheless.

There is one TODO left for which I would like to have an opinion.

clang-tidy/ClangTidyDiagnosticConsumer.cpp

472

checkFilters() sets some state so that later finalizeLastError() can remove diagnostics/errors from ClangTidyDiagnosticConsumer::Errors and also track some statistics.

Statistics are not relevant for the pluginc case as they should not be printed out for that case.
The filtering happening in finalizeLastError() does not look relevant as the plugin currently only supports the "-checks=..." argument but not the e.g. header and line filter. When checks are created in ClangTidyCheckFactories::createChecks, the "-checks=..." argument is honored, so that the !Context.isCheckEnabled(...) in finalizeLastError() is also not relevant.

I agree that checkFilters()/finalizeLastError() will be needed once the plugin case should support the other filtering options (header + line filter), but note that this goes beyond the scope of this change here, which is NOLINT filtering.

This is all new to me, so if I miss something regarding the checkFilters()/finalizeLastError() thingy, please tell me, preferably with an example if possible :)

[nik]

Addressed comments.

[nik]

Ping :)

[gribozavr]

clang-tidy/ClangTidyDiagnosticConsumer.cpp

305

Did you consider adding this query API to DiagnosticIDs instead? To me it looks weird that creation of custom diag IDs is handled by one class (DiagnosticIDs), and queries -- by another (ClangTidyContext).

472

It might be not relevant right now, but that code has invariants about how those two functions are called. Even if things happen to work, I think this code is too complicated to allow breaking those invariants in some cases.

test/clang-tidy/nolintnextline-plugin.cpp

47 ↗

(On Diff #200736)

Why not on the first line?

[nik]

clang-tidy/ClangTidyDiagnosticConsumer.cpp

305

Huch, that query API is already there. For some reason, I've missed it.

Good catch, thanks! :)

test/clang-tidy/nolintnextline-plugin.cpp

47 ↗

(On Diff #200736)

As this file is based on test/clang-tidy/nolint.cpp, I left it at is. But having it at top looks more common, so I've moved it there now.

[nik]

Addressed inline comments.

[nik]

Thanks Dmitri!

clang-tidy/ClangTidyDiagnosticConsumer.h

public:
   bool canEnableAnalyzerAlphaCheckers() const {
     return AllowEnablingAnalyzerAlphaCheckers;
   }
 
 private:
   // Writes to Stats.
   friend class ClangTidyDiagnosticConsumer;
 
   DiagnosticsEngine *DiagEngine;
   std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider;
 
   std::string CurrentFile;
   ClangTidyOptions CurrentOptions;
   class CachedGlobList;
   std::unique_ptr<CachedGlobList> CheckFilter;
   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;
+
+  bool LastErrorWasIgnored;
 };
 
 /// \brief A diagnostic consumer that turns each \c Diagnostic into a
 /// \c SourceManager-independent \c ClangTidyError.
 //
 // FIXME: If we move away from unit-tests, this can be moved to a private
 // implementation file.
 class ClangTidyDiagnosticConsumer : public DiagnosticConsumer {
private:
   bool passesLineFilter(StringRef FileName, unsigned LineNumber) const;
 
   ClangTidyContext &Context;
   bool RemoveIncompatibleErrors;
   std::vector<ClangTidyError> Errors;
   std::unique_ptr<llvm::Regex> HeaderFilter;
   bool LastErrorRelatesToUserCode;
   bool LastErrorPassesLineFilter;
-  bool LastErrorWasIgnored;
 };
 
 } // end namespace tidy
 } // end namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANG_TIDY_CLANGTIDYDIAGNOSTICCONSUMER_H

clang-tidy/ClangTidyDiagnosticConsumer.cpp

private:
   llvm::StringMap<Tristate> Cache;
 };
 
 ClangTidyContext::ClangTidyContext(
     std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider,
     bool AllowEnablingAnalyzerAlphaCheckers)
     : DiagEngine(nullptr), OptionsProvider(std::move(OptionsProvider)),
       Profile(false),
-      AllowEnablingAnalyzerAlphaCheckers(AllowEnablingAnalyzerAlphaCheckers) {
+      AllowEnablingAnalyzerAlphaCheckers(AllowEnablingAnalyzerAlphaCheckers),
+      LastErrorWasIgnored(false) {
   // 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 bool IsNOLINTFound(StringRef NolintDirectiveText, StringRef Line,
+                          unsigned DiagID, 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 bool LineIsMarkedWithNOLINT(const SourceManager &SM, SourceLocation Loc,
+                                   unsigned DiagID,
+                                   const ClangTidyContext &Context) {
+  bool Invalid;
+  const char *CharacterData = SM.getCharacterData(Loc, &Invalid);
+  if (Invalid)
+    return false;
+
+  // Check if there's a NOLINT on this line.
+  const char *P = CharacterData;
+  while (*P != '\0' && *P != '\r' && *P != '\n')
+    ++P;
+  StringRef RestOfLine(CharacterData, P - CharacterData + 1);
+  if (IsNOLINTFound("NOLINT", RestOfLine, DiagID, Context))
+    return true;
+
+  // Check if there's a NOLINTNEXTLINE on the previous line.
+  const char *BufBegin =
+      SM.getCharacterData(SM.getLocForStartOfFile(SM.getFileID(Loc)), &Invalid);
+  if (Invalid || P == BufBegin)
+    return false;
+
+  // Scan backwards over the current line.
+  P = CharacterData;
+  while (P != BufBegin && *P != '\n')
+    --P;
+
+  // If we reached the begin of the file there is no line before it.
+  if (P == BufBegin)
+    return false;
+
+  // Skip over the newline.
+  --P;
+  const char *LineEnd = P;
+
+  // Now we're on the previous line. Skip to the beginning of it.
+  while (P != BufBegin && *P != '\n')
+    --P;
+
+  RestOfLine = StringRef(P, LineEnd - P + 1);
+  if (IsNOLINTFound("NOLINTNEXTLINE", RestOfLine, DiagID, Context))
+    return true;
+
+  return false;
+}
+
+static bool LineIsMarkedWithNOLINTinMacro(const SourceManager &SM,
+                                          SourceLocation Loc, unsigned DiagID,
+                                          const ClangTidyContext &Context) {
+  while (true) {
+    if (LineIsMarkedWithNOLINT(SM, Loc, DiagID, Context))
+      return true;
+    if (!Loc.isMacroID())
+      return false;
+    Loc = SM.getImmediateExpansionRange(Loc).getBegin();
+  }
+  return false;
+}
+
 DiagnosticBuilder ClangTidyContext::diag(
     StringRef CheckName, SourceLocation Loc, StringRef Description,
     DiagnosticIDs::Level Level /* = DiagnosticIDs::Warning*/) {
   assert(Loc.isValid());
+  if (LastErrorWasIgnored && Level == DiagnosticIDs::Level::Note)
+    return DiagnosticBuilder::getDummy(DiagEngine);
+
   unsigned ID = DiagEngine->getDiagnosticIDs()->getCustomDiagID(
       Level, (Description + " [" + CheckName + "]").str());
+  if (LineIsMarkedWithNOLINTinMacro(DiagEngine->getSourceManager(), Loc, ID,
+                                    *this)) {
+    ++Stats.ErrorsIgnoredNOLINT;
+    // Ignored a warning, should ignore related notes as well
+    LastErrorWasIgnored = true;
+    return DiagnosticBuilder::getDummy(DiagEngine);
+  }
+
+  LastErrorWasIgnored = false;
   CheckNamesByDiagnosticID.try_emplace(ID, CheckName);
   return DiagEngine->Report(Loc, ID);
 }
 
 void ClangTidyContext::setSourceManager(SourceManager *SourceMgr) {
   DiagEngine->setSourceManager(SourceMgr);
 }
 
 void ClangTidyContext::setCurrentFile(StringRef File) {
   CurrentFile = File;
std::string ClangTidyContext::getCheckName(unsigned DiagnosticID) const {
   if (I != CheckNamesByDiagnosticID.end())
     return I->second;
   return "";
 }
 
 ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer(
     ClangTidyContext &Ctx, bool RemoveIncompatibleErrors)
     : Context(Ctx), RemoveIncompatibleErrors(RemoveIncompatibleErrors),
-      LastErrorRelatesToUserCode(false), LastErrorPassesLineFilter(false),
-      LastErrorWasIgnored(false) {}
+      LastErrorRelatesToUserCode(false), LastErrorPassesLineFilter(false) {}
 
 void ClangTidyDiagnosticConsumer::finalizeLastError() {
   if (!Errors.empty()) {
     ClangTidyError &Error = Errors.back();
     if (!Context.isCheckEnabled(Error.DiagnosticName) &&
         Error.DiagLevel != ClangTidyError::Error) {
       ++Context.Stats.ErrorsIgnoredCheckFilter;
       Errors.pop_back();
     } else if (!LastErrorRelatesToUserCode) {
       ++Context.Stats.ErrorsIgnoredNonUserCode;
       Errors.pop_back();
     } else if (!LastErrorPassesLineFilter) {
       ++Context.Stats.ErrorsIgnoredLineFilter;
       Errors.pop_back();
     } else {
       ++Context.Stats.ErrorsDisplayed;
     }
   }
   LastErrorRelatesToUserCode = false;
   LastErrorPassesLineFilter = false;
 }
 
-static bool IsNOLINTFound(StringRef NolintDirectiveText, StringRef Line,
-                          unsigned DiagID, 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 bool LineIsMarkedWithNOLINT(const SourceManager &SM, SourceLocation Loc,
-                                   unsigned DiagID,
-                                   const ClangTidyContext &Context) {
-  bool Invalid;
-  const char *CharacterData = SM.getCharacterData(Loc, &Invalid);
-  if (Invalid)
-    return false;
-
-  // Check if there's a NOLINT on this line.
-  const char *P = CharacterData;
-  while (*P != '\0' && *P != '\r' && *P != '\n')
-    ++P;
-  StringRef RestOfLine(CharacterData, P - CharacterData + 1);
-  if (IsNOLINTFound("NOLINT", RestOfLine, DiagID, Context))
-    return true;
-
-  // Check if there's a NOLINTNEXTLINE on the previous line.
-  const char *BufBegin =
-      SM.getCharacterData(SM.getLocForStartOfFile(SM.getFileID(Loc)), &Invalid);
-  if (Invalid || P == BufBegin)
-    return false;
-
-  // Scan backwards over the current line.
-  P = CharacterData;
-  while (P != BufBegin && *P != '\n')
-    --P;
-
-  // If we reached the begin of the file there is no line before it.
-  if (P == BufBegin)
-    return false;
-
-  // Skip over the newline.
-  --P;
-  const char *LineEnd = P;
-
-  // Now we're on the previous line. Skip to the beginning of it.
-  while (P != BufBegin && *P != '\n')
-    --P;
-
-  RestOfLine = StringRef(P, LineEnd - P + 1);
-  if (IsNOLINTFound("NOLINTNEXTLINE", RestOfLine, DiagID, Context))
-    return true;
-
-  return false;
-}
-
-static bool LineIsMarkedWithNOLINTinMacro(const SourceManager &SM,
-                                          SourceLocation Loc, unsigned DiagID,
-                                          const ClangTidyContext &Context) {
-  while (true) {
-    if (LineIsMarkedWithNOLINT(SM, Loc, DiagID, Context))
-      return true;
-    if (!Loc.isMacroID())
-      return false;
-    Loc = SM.getImmediateExpansionRange(Loc).getBegin();
-  }
-  return false;
-}
-
 void ClangTidyDiagnosticConsumer::HandleDiagnostic(
     DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info) {
-  if (LastErrorWasIgnored && DiagLevel == DiagnosticsEngine::Note)
+  if (Context.LastErrorWasIgnored && DiagLevel == DiagnosticsEngine::Note)
     return;
 
   if (Info.getLocation().isValid() && DiagLevel != DiagnosticsEngine::Error &&
       DiagLevel != DiagnosticsEngine::Fatal &&
       LineIsMarkedWithNOLINTinMacro(Info.getSourceManager(),
                                     Info.getLocation(), Info.getID(),
                                     Context)) {
     ++Context.Stats.ErrorsIgnoredNOLINT;
     // Ignored a warning, should ignore related notes as well
-    LastErrorWasIgnored = true;
+    Context.LastErrorWasIgnored = true;
     return;
   }
 
-  LastErrorWasIgnored = false;
+  Context.LastErrorWasIgnored = false;
   // 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();
if (DiagLevel == DiagnosticsEngine::Note) {
     Errors.emplace_back(CheckName, Level, Context.getCurrentBuildDirectory(),
                         IsWarningAsError);
   }
 
   ClangTidyDiagnosticRenderer Converter(
       Context.getLangOpts(), &Context.DiagEngine->getDiagnosticOptions(),
       Errors.back());
   SmallString<100> Message;
   Info.FormatDiagnostic(Message);
   FullSourceLoc Loc;
   if (Info.getLocation().isValid() && Info.hasSourceManager())
     Loc = FullSourceLoc(Info.getLocation(), Info.getSourceManager());
   Converter.emitDiagnostic(Loc, DiagLevel, Message, Info.getRanges(),
                            Info.getFixItHints());
 
   if (Info.hasSourceManager())
     checkFilters(Info.getLocation(), Info.getSourceManager());
 }
 
 bool ClangTidyDiagnosticConsumer::passesLineFilter(StringRef FileName,
                                                    unsigned LineNumber) const {
   if (Context.getGlobalOptions().LineFilter.empty())
     return true;
   for (const FileFilter &Filter : Context.getGlobalOptions().LineFilter) {
     if (FileName.endswith(Filter.Name)) {

test/clang-tidy/nolint-plugin.cpp

+// REQUIRES: static-analyzer
+// RUN: c-index-test -test-load-source-reparse 2 all %s -Xclang -add-plugin -Xclang clang-tidy -Xclang -plugin-arg-clang-tidy -Xclang -checks='-*,google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult' -Wunused-variable -I%S/Inputs/nolint 2>&1 | FileCheck %s
+
+#include "trigger_warning.h"
+void I(int& Out) {
+  int In;
+  A1(In, Out);
+}
+// CHECK-NOT: trigger_warning.h:{{.*}} warning
+// CHECK-NOT: :[[@LINE-4]]:{{.*}} note
+
+class A { A(int i); };
+// CHECK-DAG: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+class B { B(int i); }; // NOLINT
+
+class C { C(int i); }; // NOLINT(for-some-other-check)
+// CHECK-DAG: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+class C1 { C1(int i); }; // NOLINT(*)
+
+class C2 { C2(int i); }; // NOLINT(not-closed-bracket-is-treated-as-skip-all
+
+class C3 { C3(int i); }; // NOLINT(google-explicit-constructor)
+
+class C4 { C4(int i); }; // NOLINT(some-check, google-explicit-constructor)
+
+class C5 { C5(int i); }; // NOLINT without-brackets-skip-all, another-check
+
+void f() {
+  int i;
+// CHECK-DAG: :[[@LINE-1]]:7: warning: unused variable 'i'
+  int j; // NOLINT
+  int k; // NOLINT(clang-diagnostic-unused-variable)
+}
+
+#define MACRO(X) class X { X(int i); };
+MACRO(D)
+// CHECK-DAG: :[[@LINE-1]]:7: warning: single-argument constructors must be marked explicit
+MACRO(E) // NOLINT
+
+#define MACRO_NOARG class F { F(int i); };
+MACRO_NOARG // NOLINT
+
+#define MACRO_NOLINT class G { G(int i); }; // NOLINT
+MACRO_NOLINT
+
+#define DOUBLE_MACRO MACRO(H) // NOLINT
+DOUBLE_MACRO