[clangd] Respect clang-tidy suppression comments

[nridge]

Summary

This partially fixes https://bugs.llvm.org/show_bug.cgi?id=41218.

[nridge]

Changing reviewers as I understand @hokein is on vacation.

[sammccall]

Thanks for doing this!

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

395 ↗

(On Diff #196013)

There may be a trap here for clangd.

When building an AST with a preamble (which is when we run clang-tidy checks), the source code of the main file is mapped in the SourceManager as usual, but headers are not.
An attempt to get the buffer results in the SourceManager reading the file from disk. If the content has changed then all sorts of invariants break and clang will typically crash.

@ilya-biryukov knows the details here better than me.

Of course this is only true for trying to read from header file locations, and we only run clang-tidy over main-file-decls. But:

• clang-tidy checks can emit diagnostics anywhere, even outside the AST range they run over directly
• while StoreDiags does filter out diagnostics that aren't in the main file, this is pretty nuanced (e.g. a note in the main file is sufficient to include) and this logic runs after the filtering added by this patch
• the way that LineIsMarkedWithNOLINTinMacro loops suggests that even if the diagnostic is in the main-file, we'll look for NOLINT in other files

Maybe this means we can't accurately calculate suppression for clang-tidy warnings in macro locations, or outside the main file. I think *always* filtering these out (on the clangd side) might be OK.

clang-tools-extra/clangd/ClangdUnit.cpp

254 ↗

(On Diff #196013)

the interaction between ClangdDiagnosticConsumer and StoreDiags seems a little mysterious - I think the total behavior would be easier to understand (and debug) if StoreDiags knew about the filtering.

At the same time, I see why you want to keep the clang-tidy-specific logic in this file.

Maybe we could add a filterDiagnostics(function<bool(Diagnostic&)>) or so into StoreDiags, similar to contributeFixes? That way the policy stuff (which diagnostics to filter) stays here, and the mechanism (e.g. tracking associated notes) is visible in StoreDiags.

[nridge]

Address review comments

[nridge]

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

395 ↗

(On Diff #196013)

Thanks for pointing this out! This should be addressed in the updated patch.

[nridge]

Friendly review ping.

[sammccall]

Thanks, this looks nice. Main ideas here:

• it'd be nice to get rid of the subclassing in the diag consumer if we can
• i'm not sure the logic around LastErrorWasIgnored is completely accurate
• can we add a unit test for this? The existing clang-tidy diag tests are in unittests/DiagnosticsTests.cpp

clang-tools-extra/clangd/ClangdUnit.cpp

243 ↗

(On Diff #197040)

Thanks, this is much cleaner.

I think we don't need the subclass at all though - just a filter function here, and call setFilter after setting up clang-tidy?

338 ↗

(On Diff #197040)

I believe you can just call setFilter at the end of this block

clang-tools-extra/clangd/Diagnostics.cpp

309 ↗

(On Diff #197040)

do we need to set LastErrorWasIgnored here (in the case that it's not a note)?

I guess it's pretty unlikely that e.g. a problem with the command-line will get a note that has a source location...

314 ↗

(On Diff #197040)

Can you add a comment here, about notes attached to suppressed errors? This code is under-commented (sorry), but I think it would aid understanding.

319 ↗

(On Diff #197040)

If I read the code right, we're now applying the filter to a note (whose primary diagnostic was ignored), which we shouldn't. We're also updating the LastErrorWasIgnored flag, which we shouldn't for notes.

I think we might prefer to merge the new logic with the bit below the lambdas, which already looks at note/non-note status.

e.g.

if (!isNote(...)) {
  flushLastDiag();
  LastErrorWasIgnored = !Filter(...);
  if (LastErrorWasIgnored)
    return;
  ...
} else {
  // Handle a note...
  if (LastErrorWasIgnored)
    return;
  ...
}

clang-tools-extra/clangd/Diagnostics.h

118 ↗

(On Diff #197040)

nit: IsInsideMainFile is information already inside the diagnostic.

Diagnostics.cpp does compute this information (in an unneccesarily verbose way IMO) but I'd prefer not to expose that in this public interface - the filter function can recompute.

122 ↗

(On Diff #197040)

I know I suggested this name, but just an idea...

The difficulty with filter is it's unclear whether true means "this passes the filter" or "this is filtered out". It may be clearer to invert the sense and call this suppress.

Anyway, totally up to you, can definitely live with filter.

132 ↗

(On Diff #197040)

I think we're talking about last non-note diagnostic, right? Warnings count here I think.

At the risk of being verbose, maybe LastPrimaryDiagnosticWasIgnored?

[nridge]

Address review comments, except for the derived class one, about which I have a question

[nridge]

clang-tools-extra/clangd/ClangdUnit.cpp

243 ↗

(On Diff #197040)

I rely on the derived class more in the follow-up patch D61841 where I override HandleDiagnostic(). Should I still remove it from this patch?

clang-tools-extra/clangd/Diagnostics.cpp

309 ↗

(On Diff #197040)

If that does happen, then it looks like storing the note in that case is a pre-existing behaviour, which this patch just preserves.

clang-tools-extra/clangd/Diagnostics.h

132 ↗

(On Diff #197040)

I copied the name from ClangTidyDiagnosticConsumer::LastErrorWasIgnored, but sure, I can rename this as suggested.

[sammccall]

Thanks, this looks good. I think we should avoid the subclassing, but any of {generalize in the next patch, different approach for the next patch, subclass for now and clean up later} seems fine.

clang-tools-extra/clangd/ClangdUnit.cpp

243 ↗

(On Diff #197040)

Sorry about not understanding the interaction here, I wasn't aware of the warnings-to-errors work.
Yes, I'd prefer to avoid the subclassing for that too.

A couple of options spring to mind here:

• generalize from suppressDiagnostics(callback -> bool) to adjustLevel(callback -> DiagnosticLevel) or similar, where Ignored is the current suppression behavior.
• apply warnings-to-errors at the end, at the clangd::Diag level. This captures that a diag came from clang-tidy, and the check name, so seems feasible.

(sorry about the churn here, I wasn't aware of the followup patch)

clang-tools-extra/clangd/Diagnostics.h

115 ↗

(On Diff #199551)

nit: this really doesn't seem worth adding to a public interface, is D.hasSourceManager() && D.getSourceManager().isWrittenInMainFile(D.getLocation()) really too verbose?

(I don't think the helper function in Diagnostics.cpp is justified either, but unrelated to this patch)

clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp

771 ↗

(On Diff #199551)

nit: can we group this with the other ClangTidy test?
TEST(DiagnosticTest, ClangTidySuppressionComment) and move it up in the file?

[nridge]

Address latest review comments

[nridge]

Could someone merge this and D61841 now that they're approved? Thanks!

[nridge]

Thank you MaskRay!

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

private:
   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.
+/// This is exposed so that other tools that present clang-tidy diagnostics
+/// (such as clangd) can respect the same suppression rules as clang-tidy.
+/// This does not handle suppression of notes following a suppressed diagnostic;
+/// that is left to the caller is it requires maintaining state in between calls
+/// to this function.
+/// The `CheckMacroExpansion` parameter determines whether the function should
+/// handle the case where the diagnostic is inside a macro expansion. A degree
+/// of control over this is needed because handling this case can require
+/// examining source files other than the one in which the diagnostic is
+/// located, and in some use cases we cannot rely on such other files being
+/// mapped in the SourceMapper.
+bool ShouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
+                              const Diagnostic &Info, ClangTidyContext &Context,
+                              bool CheckMacroExpansion = true);
+
 /// \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,
private:
   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);
+  void checkFilters(SourceLocation Location, const SourceManager &Sources);
   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-tools-extra/trunk/clang-tidy/ClangTidyDiagnosticConsumer.cpp

 }
 
 class ClangTidyContext::CachedGlobList {
 public:
   CachedGlobList(StringRef Globs) : Globs(Globs) {}
 
   bool contains(StringRef S) {
     switch (auto &Result = Cache[S]) {
-      case Yes: return true;
-      case No: return false;
-      case None:
-        Result = Globs.contains(S) ? Yes : No;
-        return Result == Yes;
+    case Yes:
+      return true;
+    case No:
+      return false;
     }
     llvm_unreachable("invalid enum");
   }
 
 private:
   GlobList Globs;
   enum Tristate { None, Yes, No };
   llvm::StringMap<Tristate> Cache;
if (LineIsMarkedWithNOLINT(SM, Loc, DiagID, Context))
       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 CheckMacroExpansion) {
+  return Info.getLocation().isValid() &&
+         DiagLevel != DiagnosticsEngine::Error &&
+         DiagLevel != DiagnosticsEngine::Fatal &&
+         (CheckMacroExpansion ? LineIsMarkedWithNOLINTinMacro
+                              : LineIsMarkedWithNOLINT)(Info.getSourceManager(),
+                                                        Info.getLocation(),
+                                                        Info.getID(), Context);
+}
+
+} // namespace tidy
+} // namespace clang
+
 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(Info.getSourceManager(),
-                                    Info.getLocation(), Info.getID(),
-                                    Context)) {
+  if (ShouldSuppressDiagnostic(DiagLevel, Info, Context)) {
     ++Context.Stats.ErrorsIgnoredNOLINT;
     // Ignored a warning, should ignore related notes as well
     LastErrorWasIgnored = true;
     return;
   }
 
   LastErrorWasIgnored = false;
   // Count warnings/errors.

clang-tools-extra/trunk/clangd/ClangdUnit.cpp

public:
   }
 
   void MacroDefined(const Token &MacroName, const MacroDirective *MD) {
     if (InMainFile)
       MainFileMacros.insert(MacroName.getIdentifierInfo()->getName());
   }
 
   void EndOfMainFile() {
-    for (const auto& Entry : MainFileMacros)
+    for (const auto &Entry : MainFileMacros)
       Out->push_back(Entry.getKey());
     llvm::sort(*Out);
   }
 
- private:
   const SourceManager &SM;
   bool InMainFile = true;
   llvm::StringSet<> MainFileMacros;
   std::vector<std::string> *Out;
 };
 
 class CppFilePreambleCallbacks : public PreambleCallbacks {
 public:
llvm::Optional<tidy::ClangTidyContext> CTContext;
     for (const auto &E : tidy::ClangTidyModuleRegistry::entries())
       E.instantiate()->addCheckFactories(CTFactories);
     CTContext.emplace(llvm::make_unique<tidy::DefaultOptionsProvider>(
         tidy::ClangTidyGlobalOptions(), Opts.ClangTidyOpts));
     CTContext->setDiagnosticsEngine(&Clang->getDiagnostics());
     CTContext->setASTContext(&Clang->getASTContext());
     CTContext->setCurrentFile(MainInput.getFile());
     CTFactories.createChecks(CTContext.getPointer(), CTChecks);
+    ASTDiags.suppressDiagnostics([&CTContext](
+                                     DiagnosticsEngine::Level DiagLevel,
+                                     const clang::Diagnostic &Info) {
+      if (CTContext) {
+        bool IsClangTidyDiag = !CTContext->getCheckName(Info.getID()).empty();
+        if (IsClangTidyDiag) {
+          // Skip the ShouldSuppressDiagnostic 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 not to follow macro expansions, since
+          // those might take us into a preamble file as well.
+          bool IsInsideMainFile =
+              Info.hasSourceManager() &&
+              Info.getSourceManager().isWrittenInMainFile(
+                  Info.getSourceManager().getFileLoc(Info.getLocation()));
+          if (IsInsideMainFile && tidy::ShouldSuppressDiagnostic(
+                                      DiagLevel, Info, *CTContext,
+                                      /* CheckMacroExpansion = */ false)) {
+            return true;
+          }
+        }
+      }
+      return false;
+    });
     Preprocessor *PP = &Clang->getPreprocessor();
     for (const auto &Check : CTChecks) {
       // FIXME: the PP callbacks skip the entire preamble.
       // Checks that want to see #includes in the main file do not see them.
       Check->registerPPCallbacks(Clang->getSourceManager(), PP, PP);
       Check->registerMatchers(&CTFinder);
     }
   }

clang-tools-extra/trunk/clangd/Diagnostics.h

public:
 
   void BeginSourceFile(const LangOptions &Opts, const Preprocessor *) override;
   void EndSourceFile() override;
   void HandleDiagnostic(DiagnosticsEngine::Level DiagLevel,
                         const clang::Diagnostic &Info) override;
 
   using DiagFixer = std::function<std::vector<Fix>(DiagnosticsEngine::Level,
                                                    const clang::Diagnostic &)>;
+  using DiagFilter =
+      std::function<bool(DiagnosticsEngine::Level, const clang::Diagnostic &)>;
   /// If set, possibly adds fixes for diagnostics using \p Fixer.
   void contributeFixes(DiagFixer Fixer) { this->Fixer = Fixer; }
+  /// If set, ignore diagnostics for which \p SuppressionFilter returns true.
+  void suppressDiagnostics(DiagFilter SuppressionFilter) {
+    this->SuppressionFilter = SuppressionFilter;
+  }
 
 private:
   void flushLastDiag();
 
   DiagFixer Fixer = nullptr;
+  DiagFilter SuppressionFilter = nullptr;
   std::vector<Diag> Output;
   llvm::Optional<LangOptions> LangOpts;
   llvm::Optional<Diag> LastDiag;
   llvm::DenseSet<int> IncludeLinesWithErrors;
+  bool LastPrimaryDiagnosticWasSuppressed = false;
 };
 
 } // namespace clangd
 } // namespace clang
 
 #endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_DIAGNOSTICS_H

clang-tools-extra/trunk/clangd/Diagnostics.cpp

auto AddFix = [&](bool SyntheticMessage) -> bool {
     LastDiag->Fixes.push_back(Fix{Message.str(), std::move(Edits)});
     return true;
   };
 
   if (!isNote(DiagLevel)) {
     // Handle the new main diagnostic.
     flushLastDiag();
 
+    if (SuppressionFilter && SuppressionFilter(DiagLevel, Info)) {
+      LastPrimaryDiagnosticWasSuppressed = true;
+      return;
+    }
+    LastPrimaryDiagnosticWasSuppressed = false;
+
     LastDiag = Diag();
     LastDiag->ID = Info.getID();
     FillDiagBase(*LastDiag);
     adjustDiagFromHeader(*LastDiag, Info, *LangOpts);
 
     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(),
                              ExtraFixes.end());
     }
   } else {
     // Handle a note to an existing diagnostic.
+
+    // If a diagnostic was suppressed due to the suppression filter,
+    // also suppress notes associated with it.
+    if (LastPrimaryDiagnosticWasSuppressed) {
+      return;
+    }
+
     if (!LastDiag) {
       assert(false && "Adding a note without main diagnostic");
       IgnoreDiagnostics::log(DiagLevel, Info);
       return;
     }
 
     if (!Info.getFixItHints().empty()) {
       // A clang note with fix-it is not a separate diagnostic in clangd. We

clang-tools-extra/trunk/clangd/unittests/DiagnosticsTests.cpp

EXPECT_THAT(
               DiagSource(Diag::ClangTidy),
               DiagName("bugprone-macro-repeated-side-effects"),
               WithNote(
                   Diag(Test.range("macrodef"), "macro 'SQUARE' defined here"))),
           Diag(Test.range("macroarg"),
                "multiple unsequenced modifications to 'y'")));
 }
 
+TEST(DiagnosticTest, ClangTidySuppressionComment) {
+  Annotations Main(R"cpp(
+    int main() {
+      int i = 3;
+      double d = 8 / i;  // NOLINT
+      // NOLINTNEXTLINE
+      double e = 8 / i;
+      double f = [[8]] / i;
+    }
+  )cpp");
+  TestTU TU = TestTU::withCode(Main.code());
+  TU.ClangTidyChecks = "bugprone-integer-division";
+  EXPECT_THAT(
+      TU.build().getDiagnostics(),
+      UnorderedElementsAre(::testing::AllOf(
+          Diag(Main.range(), "result of integer division used in a floating "
+                             "point context; possible loss of precision"),
+          DiagSource(Diag::ClangTidy), DiagName("bugprone-integer-division"))));
+}
+
 TEST(DiagnosticsTest, Preprocessor) {
   // This looks like a preamble, but there's an #else in the middle!
   // Check that:
   //  - the #else doesn't generate diagnostics (we had this bug)
   //  - we get diagnostics from the taken branch
   //  - we get no diagnostics from the not taken branch
   Annotations Test(R"cpp(
     #ifndef FOO
TEST(DiagsInHeaders, OnlyErrorOrFatal) {
   TU.AdditionalFiles = {{"a.h", Header.code()}};
   auto diags = TU.build().getDiagnostics();
   EXPECT_THAT(TU.build().getDiagnostics(),
               UnorderedElementsAre(AllOf(
                   Diag(Main.range(), "in included file: C++ requires "
                                      "a type specifier for all declarations"),
                   WithNote(Diag(Header.range(), "error occurred here")))));
 }
+
 } // namespace
 
 } // namespace clangd
 } // namespace clang