[clangd] Handle clang-tidy suppression comments for diagnostics inside macro expansions

[nridge]

Summary

Not handling this was a side-effect of being overly cautious when trying
to avoid reading files for which clangd doesn't have the source mapped.

Fixes https://github.com/clangd/clangd/issues/266

[sammccall]

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

353

why are we breaking the loop if we hit another file ID, rather than just not checking it?
I think a macro token can be expanded into another macro (with a non-main file ID) which in turn expands into the main-file - we'd want to reach the main file in that case.

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

215

I do wonder a how hard it would be to approach this more directly: (conditionally )use some API for pulling code out of the SourceManager that *won't* silently do IO.
ContentCache::getRawBuffer() or something?

Up to you whether to look at this at all of course, this approach is better than what we have now.

clang-tools-extra/clangd/ParsedAST.cpp

288

this part of the comment seems stale

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

278

you may want a case with two levels of expansion

[nridge]

Address review comments

[nridge]

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

215

I gave this a try in the updated patch.

[sammccall]

Sorry for taking a while to come back to this again. A bit chaotic right now :-)

Looks good, some further simplification possible.

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

295

This seems more complicated than necessary - e.g. it replicates the "invalid" handling from SourceManager::getCharacterData, when we just bail out in this case. And keeping the raw char* interfaces seems unfortunate if we're going to the trouble of reimplementing most of this anyway.

What about implementing llvm::Optional<StringRef> getBuffer(FileID File, bool AllowIO)?
Where AllowIO determines whether you call getbuffer or getrawbuffer.

This is simpler than the current function, and the caller can be a bit more expressive/simpler like:

unsigned Offset = SM.getFileOffset(Loc);
StringRef RestOfLine = Data.substr(Offset).split('\n').first;
StringRef PrevLine = Data.substr(0, Offset).rsplit('\n').first.rsplit('\n').second;

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

222

nit: I'd use AllowIO=true to avoid the negative sense, up to you.

[nridge]

Address review comments

[nridge]

Thanks for the suggested simplification. As the change is not completely trivial, could you have one more look before landing?

[nridge]

Finish an incomplete variable extraction

[sammccall]

Thanks a lot!

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

297

nit: the peer function to reference is SourceManager::getBufferData

[njames93]

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

712–715

This looks like a clang-format artifact, there are several other below. Could these be removed from this patch

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

66

ditto

272–273

ditto

842–843

ditto

[nridge]

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

712–715

If you insist, I can move them to a separate patch. I don't want to leave it unmodified because the change will come back every time someone touches the file.

[sammccall]

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

712–715

I don't personally care about this too much, but the changes are somewhat distracting in review, blame etc.

The policy we've mostly followed is to format changed lines only (git clang-format, turn format-on-save off) and leave misformatted code alone until it's next touched.

(Not sure if LLVM offers any guidance either way, but that's what Google does internally and IME it's been great)

[nridge]

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

712–715

The issue I have with that is that turning format-on-save off means you inevitably end up doing a lot of manual formatting as you write the code. With format-on-save, you can just write the tokens for a statement not caring about the formatting, do a save, and have the statement be formatted before you start writing the next one.

[nridge]

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

712–715

I guess what we really want is "format on save for modified lines only". I found a VSCode extension which does that, I'll give that a try.

[njames93]

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

712–715

What I do (apart from when i forget) is run git clang-format before creating the patch, Then just don't worry about formatting while I'm writing the code.

[nridge]

Address last review comments

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

 
 /// 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.
+/// If a valid FileID is passed as `FileFilter`, the function does not attempt
+/// to read source files other than the one identified by the filter. A degree
+/// of control over this is needed because in some use cases we cannot rely on
+/// files other than the one containing the diagnostic being mapped in the
+/// SourceManager.
 bool shouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
                               const Diagnostic &Info, ClangTidyContext &Context,
-                              bool CheckMacroExpansion = true);
+                              FileID FileFilter = FileID{});
 
 /// 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:

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

 } // end anonymous namespace
 
 ClangTidyError::ClangTidyError(StringRef CheckName,
                                ClangTidyError::Level DiagLevel,
                                StringRef BuildDirectory, bool IsWarningAsError)
     : tooling::Diagnostic(CheckName, DiagLevel, BuildDirectory),
       IsWarningAsError(IsWarningAsError) {}
 
-
 class ClangTidyContext::CachedGlobList {
 public:
   CachedGlobList(StringRef Globs) : Globs(Globs) {}
 
   bool contains(StringRef S) {
     switch (auto &Result = Cache[S]) {
     case Yes:
       return true;
if (BracketEndIndex != StringRef::npos) {
                         [](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')
static bool LineIsMarkedWithNOLINT(const SourceManager &SM, SourceLocation Loc,
   if (IsNOLINTFound("NOLINTNEXTLINE", RestOfLine, DiagID, Context))
     return true;
 
   return false;
 }
 
 static bool LineIsMarkedWithNOLINTinMacro(const SourceManager &SM,
                                           SourceLocation Loc, unsigned DiagID,
-                                          const ClangTidyContext &Context) {
+                                          const ClangTidyContext &Context,
+                                          FileID FileFilter) {
   while (true) {
     if (LineIsMarkedWithNOLINT(SM, Loc, DiagID, Context))
       return true;
     if (!Loc.isMacroID())
       return false;
     Loc = SM.getImmediateExpansionRange(Loc).getBegin();
+    if (FileFilter.isValid() && Loc.isFileID() &&
+        SM.getFileID(Loc) != FileFilter) {
+      return false;
+    }
   }
   return false;
 }
 
 namespace clang {
 namespace tidy {
 
 bool shouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
                               const Diagnostic &Info, ClangTidyContext &Context,
-                              bool CheckMacroExpansion) {
+                              FileID FileFilter) {
   return Info.getLocation().isValid() &&
          DiagLevel != DiagnosticsEngine::Error &&
          DiagLevel != DiagnosticsEngine::Fatal &&
-         (CheckMacroExpansion ? LineIsMarkedWithNOLINTinMacro
-                              : LineIsMarkedWithNOLINT)(Info.getSourceManager(),
-                                                        Info.getLocation(),
-                                                        Info.getID(), Context);
+         LineIsMarkedWithNOLINTinMacro(Info.getSourceManager(),
+                                       Info.getLocation(), Info.getID(),
+                                       Context, FileFilter);
 }
 
 } // namespace tidy
 } // namespace clang
 
 void ClangTidyDiagnosticConsumer::HandleDiagnostic(
     DiagnosticsEngine::Level DiagLevel, const Diagnostic &Info) {
   if (LastErrorWasIgnored && DiagLevel == DiagnosticsEngine::Note)

 }
 
 namespace {
 struct LessClangTidyError {
   bool operator()(const ClangTidyError &LHS, const ClangTidyError &RHS) const {
     const tooling::DiagnosticMessage &M1 = LHS.Message;
     const tooling::DiagnosticMessage &M2 = RHS.Message;
 
-    return
-      std::tie(M1.FilePath, M1.FileOffset, LHS.DiagnosticName, M1.Message) <
+    return std::tie(M1.FilePath, M1.FileOffset, LHS.DiagnosticName,
+                    M1.Message) <
            std::tie(M2.FilePath, M2.FileOffset, RHS.DiagnosticName, M2.Message);
   }
 };
 struct EqualClangTidyError {
   bool operator()(const ClangTidyError &LHS, const ClangTidyError &RHS) const {
     LessClangTidyError Less;
     return !Less(LHS, RHS) && !Less(RHS, LHS);
   }
 };
 } // end anonymous namespace

clang-tools-extra/clangd/ParsedAST.cpp

ASTDiags.setLevelAdjuster([&CTContext](DiagnosticsEngine::Level DiagLevel,
           if (DiagLevel == DiagnosticsEngine::Warning &&
               CTContext->treatAsError(CheckName)) {
             return DiagnosticsEngine::Error;
           }
 
           // Check for suppression comment. Skip the 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() &&
               isInsideMainFile(Info.getLocation(), Info.getSourceManager());
-          if (IsInsideMainFile && tidy::shouldSuppressDiagnostic(
-                                      DiagLevel, Info, *CTContext,
-                                      /* CheckMacroExpansion = */ false)) {
+          if (IsInsideMainFile &&
+              tidy::shouldSuppressDiagnostic(
+                  Info.getSourceManager().getMainFileID())) {
             return DiagnosticsEngine::Ignored;
           }
         }
       }
       return DiagLevel;
     });
     Preprocessor *PP = &Clang->getPreprocessor();
     for (const auto &Check : CTChecks) {

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

MATCHER_P3(Fix, Range, Replacement, Message,
            "Fix " + llvm::to_string(Range) + " => " +
                ::testing::PrintToString(Replacement) + " = [" + Message + "]") {
   return arg.Message == Message && arg.Edits.size() == 1 &&
          arg.Edits[0].range == Range && arg.Edits[0].newText == Replacement;
 }
 
 MATCHER_P(FixMessage, Message, "") {
   return arg.Message == Message;
 }
 
 MATCHER_P(EqualToLSPDiag, LSPDiag,
           "LSP diagnostic " + llvm::to_string(LSPDiag)) {
   if (toJSON(arg) != toJSON(LSPDiag)) {
     *result_listener << llvm::formatv("expected:\n{0:2}\ngot\n{1:2}",
                                       toJSON(LSPDiag), toJSON(arg))
                             .str();
     return false;
EXPECT_THAT(
             DiagName("modernize-use-trailing-return-type"),
             // Verify that we don't have "[check-name]" suffix in the message.
             WithFix(FixMessage("use a trailing return type for this function")))
           ));
 }
 
 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;
+      #define BAD 8 / i
+      double f = BAD;  // NOLINT
+      double g = [[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 "
Annotations Test(R"cpp(
     };
   )cpp");
 
   auto TU = TestTU::withCode(Test.code());
   auto Index = buildIndexWithSymbol({});
   TU.ExternalIndex = Index.get();
 
   EXPECT_THAT(TU.build().getDiagnostics(),
               ElementsAre(Diag(Test.range(), "use of undeclared identifier 'a'")));
 }
 
 TEST(DiagsInHeaders, DiagInsideHeader) {
   Annotations Main(R"cpp(
     #include [["a.h"]]
     void foo() {})cpp");
   Annotations Header("[[no_type_spec]]; // error-ok");
   TestTU TU = TestTU::withCode(Main.code());
   TU.AdditionalFiles = {{"a.h", std::string(Header.code())}};