[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

457 ↗

(On Diff #198637)

Indentation is 2 spaces.

472 ↗

(On Diff #198637)

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

clang-tidy/ClangTidyDiagnosticConsumer.h

197 ↗

(On Diff #198637)

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 ↗

(On Diff #198637)

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

207 ↗

(On Diff #200736)

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 ↗

(On Diff #198637)

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

207 ↗

(On Diff #200736)

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-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.h

public:
   }
 
   /// \brief If the experimental alpha checkers from the static analyzer can be
   /// enabled.
   bool canEnableAnalyzerAlphaCheckers() const {
     return AllowEnablingAnalyzerAlphaCheckers;
   }
 
+  using DiagLevelAndFormatString = std::pair<DiagnosticIDs::Level, std::string>;
+  DiagLevelAndFormatString getDiagLevelAndFormatString(unsigned DiagnosticID,
+                                                       SourceLocation Loc) {
+    return DiagLevelAndFormatString(
+        static_cast<DiagnosticIDs::Level>(
+            DiagEngine->getDiagnosticLevel(DiagnosticID, Loc)),
+        DiagEngine->getDiagnosticIDs()->getDescription(DiagnosticID));
+  }
+
 private:
   // Writes to Stats.
   friend class ClangTidyDiagnosticConsumer;
 
   DiagnosticsEngine *DiagEngine;
   std::unique_ptr<ClangTidyOptionsProvider> OptionsProvider;
 
   std::string CurrentFile;

 /// \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 {
 public:
   ClangTidyDiagnosticConsumer(ClangTidyContext &Ctx,
+                              DiagnosticsEngine *ExternalDiagEngine = nullptr,
                               bool RemoveIncompatibleErrors = true);
 
   // FIXME: The concept of converting between FixItHints and Replacements is
   // more generic and should be pulled out into a more useful Diagnostics
   // library.
   void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                         const Diagnostic &Info) override;
 
   // Retrieve the diagnostics that were captured.
   std::vector<ClangTidyError> take();
 
 private:
   void finalizeLastError();
   void removeIncompatibleErrors();
 
   /// \brief Returns the \c HeaderFilter constructed for the options set in the
   /// context.
   llvm::Regex *getHeaderFilter();
 
   /// \brief Updates \c LastErrorRelatesToUserCode and LastErrorPassesLineFilter
   /// according to the diagnostic \p Location.
   void checkFilters(SourceLocation Location, const SourceManager &Sources);
   bool passesLineFilter(StringRef FileName, unsigned LineNumber) const;
 
+  void forwardDiagnostic(const Diagnostic &Info);
+
   ClangTidyContext &Context;
+  DiagnosticsEngine *ExternalDiagEngine;
   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-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp

std::string ClangTidyContext::getCheckName(unsigned DiagnosticID) const {
   llvm::DenseMap<unsigned, std::string>::const_iterator I =
       CheckNamesByDiagnosticID.find(DiagnosticID);
   if (I != CheckNamesByDiagnosticID.end())
     return I->second;
   return "";
 }
 
 ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer(
-    ClangTidyContext &Ctx, bool RemoveIncompatibleErrors)
-    : Context(Ctx), RemoveIncompatibleErrors(RemoveIncompatibleErrors),
+    ClangTidyContext &Ctx, DiagnosticsEngine *ExternalDiagEngine,
+    bool RemoveIncompatibleErrors)
+    : Context(Ctx), ExternalDiagEngine(ExternalDiagEngine),
+      RemoveIncompatibleErrors(RemoveIncompatibleErrors),
       LastErrorRelatesToUserCode(false), LastErrorPassesLineFilter(false),
       LastErrorWasIgnored(false) {}
 
 void ClangTidyDiagnosticConsumer::finalizeLastError() {
   if (!Errors.empty()) {
     ClangTidyError &Error = Errors.back();
     if (!Context.isCheckEnabled(Error.DiagnosticName) &&
         Error.DiagLevel != ClangTidyError::Error) {
if (DiagLevel == DiagnosticsEngine::Error ||
       LastErrorPassesLineFilter = true;
     }
     bool IsWarningAsError = DiagLevel == DiagnosticsEngine::Warning &&
                             Context.treatAsError(CheckName);
     Errors.emplace_back(CheckName, Level, Context.getCurrentBuildDirectory(),
                         IsWarningAsError);
   }
 
+  if (ExternalDiagEngine) {
+    // If there is an external diagnostics engine, like in the
+    // ClangTidyPluginAction case, forward the diagnostics to it.
+    forwardDiagnostic(Info);
+  } else {
     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)) {
       if (Filter.LineRanges.empty())
         return true;
       for (const FileFilter::LineRange &Range : Filter.LineRanges) {
         if (Range.first <= LineNumber && LineNumber <= Range.second)
           return true;
       }
       return false;
     }
   }
   return false;
 }
 
+void ClangTidyDiagnosticConsumer::forwardDiagnostic(const Diagnostic &Info) {
+  // Acquire a diagnostic ID also in the external diagnostics engine.
+  auto DiagLevelAndFormatString =
+      Context.getDiagLevelAndFormatString(Info.getID(), Info.getLocation());
+  unsigned ExternalID = ExternalDiagEngine->getDiagnosticIDs()->getCustomDiagID(
+      DiagLevelAndFormatString.first, DiagLevelAndFormatString.second);
+
+  // Forward the details.
+  auto builder = ExternalDiagEngine->Report(Info.getLocation(), ExternalID);
+  for (auto Hint : Info.getFixItHints())
+    builder << Hint;
+  for (auto Range : Info.getRanges())
+    builder << Range;
+  for (unsigned Index = 0; Index < Info.getNumArgs(); ++Index) {
+    DiagnosticsEngine::ArgumentKind kind = Info.getArgKind(Index);
+    switch (kind) {
+    case clang::DiagnosticsEngine::ak_std_string:
+      builder << Info.getArgStdStr(Index);
+      break;
+    case clang::DiagnosticsEngine::ak_c_string:
+      builder << Info.getArgCStr(Index);
+      break;
+    case clang::DiagnosticsEngine::ak_sint:
+      builder << Info.getArgSInt(Index);
+      break;
+    case clang::DiagnosticsEngine::ak_uint:
+      builder << Info.getArgUInt(Index);
+      break;
+    case clang::DiagnosticsEngine::ak_tokenkind:
+      builder << static_cast<tok::TokenKind>(Info.getRawArg(Index));
+      break;
+    case clang::DiagnosticsEngine::ak_identifierinfo:
+      builder << Info.getArgIdentifier(Index);
+      break;
+    case clang::DiagnosticsEngine::ak_qual:
+      builder << Qualifiers::fromOpaqueValue(Info.getRawArg(Index));
+      break;
+    case clang::DiagnosticsEngine::ak_qualtype:
+      builder << QualType::getFromOpaquePtr((void *)Info.getRawArg(Index));
+      break;
+    case clang::DiagnosticsEngine::ak_declarationname:
+      builder << DeclarationName::getFromOpaqueInteger(Info.getRawArg(Index));
+      break;
+    case clang::DiagnosticsEngine::ak_nameddecl:
+      builder << reinterpret_cast<const NamedDecl *>(Info.getRawArg(Index));
+      break;
+    case clang::DiagnosticsEngine::ak_nestednamespec:
+      builder << reinterpret_cast<NestedNameSpecifier *>(Info.getRawArg(Index));
+      break;
+    case clang::DiagnosticsEngine::ak_declcontext:
+      builder << reinterpret_cast<DeclContext *>(Info.getRawArg(Index));
+      break;
+    case clang::DiagnosticsEngine::ak_qualtype_pair:
+      assert(false); // This one is not passed around.
+      break;
+    case clang::DiagnosticsEngine::ak_attr:
+      builder << reinterpret_cast<Attr *>(Info.getRawArg(Index));
+      break;
+    }
+  }
+}
+
 void ClangTidyDiagnosticConsumer::checkFilters(SourceLocation Location,
                                                const SourceManager &Sources) {
   // Invalid location may mean a diagnostic in a command line, don't skip these.
   if (!Location.isValid()) {
     LastErrorRelatesToUserCode = true;
     LastErrorPassesLineFilter = true;
     return;
   }

clang-tools-extra/trunk/clang-tidy/plugin/ClangTidyPlugin.cpp

 //===- ClangTidyPlugin.cpp - clang-tidy as a clang plugin -----------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "../ClangTidy.h"
+#include "../ClangTidyDiagnosticConsumer.h"
 #include "../ClangTidyForceLinker.h"
 #include "../ClangTidyModule.h"
 #include "clang/Frontend/CompilerInstance.h"
 #include "clang/Frontend/FrontendPluginRegistry.h"
 #include "clang/Frontend/MultiplexConsumer.h"
 
 namespace clang {
 namespace tidy {
 
 /// The core clang tidy plugin action. This just provides the AST consumer and
 /// command line flag parsing for using clang-tidy as a clang plugin.
 class ClangTidyPluginAction : public PluginASTAction {
-  /// Wrapper to grant the context the same lifetime as the action. We use
-  /// MultiplexConsumer to avoid writing out all the forwarding methods.
+  /// Wrapper to grant the context and diagnostics engine the same lifetime as
+  /// the action.
+  /// We use MultiplexConsumer to avoid writing out all the forwarding methods.
   class WrapConsumer : public MultiplexConsumer {
     std::unique_ptr<ClangTidyContext> Context;
+    std::unique_ptr<DiagnosticsEngine> DiagEngine;
 
   public:
     WrapConsumer(std::unique_ptr<ClangTidyContext> Context,
+                 std::unique_ptr<DiagnosticsEngine> DiagEngine,
                  std::vector<std::unique_ptr<ASTConsumer>> Consumer)
-        : MultiplexConsumer(std::move(Consumer)), Context(std::move(Context)) {}
+        : MultiplexConsumer(std::move(Consumer)), Context(std::move(Context)),
+          DiagEngine(std::move(DiagEngine)) {}
   };
 
 public:
   std::unique_ptr<ASTConsumer> CreateASTConsumer(CompilerInstance &Compiler,
                                                  StringRef File) override {
-    // Insert the current diagnostics engine.
-    Context->setDiagnosticsEngine(&Compiler.getDiagnostics());
+    // Create and set diagnostics engine
+    auto ExternalDiagEngine = &Compiler.getDiagnostics();
+    auto DiagConsumer =
+        new ClangTidyDiagnosticConsumer(*Context, ExternalDiagEngine);
+    auto DiagEngine = llvm::make_unique<DiagnosticsEngine>(
+        new DiagnosticIDs, new DiagnosticOptions, DiagConsumer);
+    Context->setDiagnosticsEngine(DiagEngine.get());
 
     // Create the AST consumer.
     ClangTidyASTConsumerFactory Factory(*Context);
     std::vector<std::unique_ptr<ASTConsumer>> Vec;
     Vec.push_back(Factory.CreateASTConsumer(Compiler, File));
 
-    return llvm::make_unique<WrapConsumer>(std::move(Context), std::move(Vec));
+    return llvm::make_unique<WrapConsumer>(
+        std::move(Context), std::move(DiagEngine), std::move(Vec));
   }
 
   bool ParseArgs(const CompilerInstance &,
                  const std::vector<std::string> &Args) override {
     ClangTidyGlobalOptions GlobalOptions;
     ClangTidyOptions DefaultOptions;
     ClangTidyOptions OverrideOptions;
 

clang-tools-extra/trunk/test/clang-tidy/basic.cpp

 // RUN: clang-tidy %s -checks='-*,llvm-namespace-comment' -- | FileCheck %s
+// RUN: c-index-test -test-load-source-reparse 2 all %s -Xclang -add-plugin -Xclang clang-tidy -Xclang -plugin-arg-clang-tidy -Xclang -checks='-*,llvm-namespace-comment' 2>&1 | FileCheck %s
 
 namespace i {
 }
 // CHECK: warning: namespace 'i' not terminated with a closing comment [llvm-namespace-comment]

clang-tools-extra/trunk/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' [-Wunused-variable]
+//                          31:7: warning: unused variable 'i' [-Wunused-variable]
+//  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

clang-tools-extra/trunk/test/clang-tidy/nolintnextline-plugin.cpp

+// 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' 2>&1 | FileCheck %s
+
+class A { A(int i); };
+// CHECK: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTNEXTLINE
+class B { B(int i); };
+
+// NOLINTNEXTLINE(for-some-other-check)
+class C { C(int i); };
+// CHECK: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTNEXTLINE(*)
+class C1 { C1(int i); };
+
+// NOLINTNEXTLINE(not-closed-bracket-is-treated-as-skip-all
+class C2 { C2(int i); };
+
+// NOLINTNEXTLINE(google-explicit-constructor)
+class C3 { C3(int i); };
+
+// NOLINTNEXTLINE(some-check, google-explicit-constructor)
+class C4 { C4(int i); };
+
+// NOLINTNEXTLINE without-brackets-skip-all, another-check
+class C5 { C5(int i); };
+
+
+// NOLINTNEXTLINE
+
+class D { D(int i); };
+// CHECK: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+// NOLINTNEXTLINE
+//
+class E { E(int i); };
+// CHECK: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
+
+#define MACRO(X) class X { X(int i); };
+MACRO(F)
+// CHECK: :[[@LINE-1]]:7: warning: single-argument constructors must be marked explicit
+// NOLINTNEXTLINE
+MACRO(G)
+
+#define MACRO_NOARG class H { H(int i); };
+// NOLINTNEXTLINE
+MACRO_NOARG
+