[clang-tidy] Ignore notes along with a warning when processing NOLINT

[nkakuev]

Summary

When clang-tidy suppresses a warning, ignored by NOLINT, it doesn't suppress related notes. This leads to an assertion/crash, as described in this bug: https://llvm.org/bugs/show_bug.cgi?id=30565

The minimal reproducible example is as follows (run with 'clang-tidy source.cpp -- -c'):

• header.h:

void TriggerWarning(const int& In, int& Out) {
  if (In > 123) // NOLINT
    Out = 123;
}

• source.cpp:

#include "header.h"
void MaybeInitialize(int &Out) {
  int In;
  TriggerWarning(In, Out);
}

Suppressing notes after suppressing a warning fixes the problem and produces a consistent diagnostics output.

[malcolm.parsons]

clang-tidy/ClangTidyDiagnosticConsumer.cpp

312

Change ShouldIgnoreNotes to a class member and rename to LastErrorWasIgnored or similar.

[nkakuev]

Fixed review comments: replaced a static variable with a class member.

[malcolm.parsons]

LGTM, with typo below fixed.

test/clang-tidy/nolint.cpp

36

Is header.h supposed to be trigger_warning.h?

[nkakuev]

Fix a typo.

[nkakuev]

@malcolm.parsons, can you please commit this patch? I don't have commit rights.

[malcolm.parsons]

Committed as r285861.
Thanks!

[alexfh]

test/clang-tidy/nolint.cpp

36

The test is too brittle: if the functionality breaks after the text of the message changes, it will still pass.

Two ways to deal with it:

1. add a positive test to ensure this pattern actually triggers a warning
2. make the CHECK-NOT pattern broader: // CHECK-NOT: warning:.

[nkakuev]

@alexfh I'll fix it in a separate review.

[nkakuev]

test/clang-tidy/nolint.cpp

36

Fixed in review D26466.

clang-tidy/ClangTidyDiagnosticConsumer.h

private:
   bool passesLineFilter(StringRef FileName, unsigned LineNumber) const;
 
   ClangTidyContext &Context;
   std::unique_ptr<DiagnosticsEngine> Diags;
   SmallVector<ClangTidyError, 8> 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

llvm::DenseMap<unsigned, std::string>::const_iterator I =
       CheckNamesByDiagnosticID.find(DiagnosticID);
   if (I != CheckNamesByDiagnosticID.end())
     return I->second;
   return "";
 }
 
 ClangTidyDiagnosticConsumer::ClangTidyDiagnosticConsumer(ClangTidyContext &Ctx)
     : Context(Ctx), LastErrorRelatesToUserCode(false),
-      LastErrorPassesLineFilter(false) {
+      LastErrorPassesLineFilter(false), LastErrorWasIgnored(false) {
   IntrusiveRefCntPtr<DiagnosticOptions> DiagOpts = new DiagnosticOptions();
   Diags.reset(new DiagnosticsEngine(
       IntrusiveRefCntPtr<DiagnosticIDs>(new DiagnosticIDs), &*DiagOpts, this,
       /*ShouldOwnClient=*/false));
   Context.setDiagnosticsEngine(Diags.get());
 }
 
 void ClangTidyDiagnosticConsumer::finalizeLastError() {
if (!Loc.isMacroID())
       return false;
     Loc = SM.getImmediateExpansionRange(Loc).first;
   }
   return false;
 }
 
 void ClangTidyDiagnosticConsumer::HandleDiagnostic(
     DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info) {
+  if (LastErrorWasIgnored && DiagLevel == DiagnosticsEngine::Note)
+    return;
+
   if (Info.getLocation().isValid() &&
       DiagLevel != DiagnosticsEngine::Error &&
       DiagLevel != DiagnosticsEngine::Fatal &&
       LineIsMarkedWithNOLINTinMacro(Diags->getSourceManager(), Info.getLocation())) {
     ++Context.Stats.ErrorsIgnoredNOLINT;
+    // Ignored a warning, should ignore related notes as well
+    LastErrorWasIgnored = true;
     return;
   }
+
+  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();

test/clang-tidy/Inputs/nolint/trigger_warning.h

+void A1(const int &In, int &Out) {
+  if (In > 123) // NOLINT
+    Out = 123;
+}
+

test/clang-tidy/nolint.cpp

-// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable %t -- -extra-arg=-Wunused-variable --
+// RUN: %check_clang_tidy %s google-explicit-constructor,clang-diagnostic-unused-variable,clang-analyzer-core.UndefinedBinaryOperatorResult %t -- -extra-arg=-Wunused-variable -- -I%S/Inputs/nolint
+
 
 class A { A(int i); };
 // CHECK-MESSAGES: :[[@LINE-1]]:11: warning: single-argument constructors must be marked explicit
 
 class B { B(int i); }; // NOLINT
 
 class C { C(int i); }; // NOLINT(we-dont-care-about-categories-yet)
 

 MACRO_NOARG // NOLINT
 
 #define MACRO_NOLINT class G { G(int i); }; // NOLINT
 MACRO_NOLINT
 
 #define DOUBLE_MACRO MACRO(H) // NOLINT
 DOUBLE_MACRO
 
-// CHECK-MESSAGES: Suppressed 7 warnings (7 NOLINT)
+#include "trigger_warning.h"
+void I(int& Out) {
+  int In;
+  A1(In, Out);
+}
+// CHECK-NOT: trigger_warning.h:{{.*}} warning: The left operand of '>' is a garbage value
+// CHECK-NOT: :[[@LINE-4]]:{{.*}} note
+
+// CHECK-MESSAGES: Suppressed 8 warnings (8 NOLINT)