[PATCH] D60953: [clangd] Respect clang-tidy suppression comments

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Thanks for doing this!



================
Comment at: clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp:395
+         DiagLevel != DiagnosticsEngine::Fatal &&
+         LineIsMarkedWithNOLINTinMacro(Info.getSourceManager(),
+                                       Info.getLocation(), Info.getID(),
----------------
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.


================
Comment at: clang-tools-extra/clangd/ClangdUnit.cpp:254
+          tidy::ShouldSuppressDiagnostic(DiagLevel, Info, *CTContext)) {
+        // Ignored a warning, should ignore related notes as well
+        LastErrorWasIgnored = true;
----------------
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D60953/new/

https://reviews.llvm.org/D60953