[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

219–221

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

312–313

this part of the comment seems stale

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

276

you may want a case with two levels of expansion

[nridge]

Address review comments

[nridge]

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

219–221

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

299

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

224

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

301

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

[njames93]

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

708–710

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

64

ditto

264–265

ditto

841–842

ditto

[nridge]

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

708–710

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

708–710

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

708–710

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

708–710

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

708–710

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 `AllowIO` is false, the function does not attempt to read source files
+/// from disk which are not already mapped into memory; such files are treated
+/// as not containing a suppression comment.
 bool shouldSuppressDiagnostic(DiagnosticsEngine::Level DiagLevel,
                               const Diagnostic &Info, ClangTidyContext &Context,
-                              bool CheckMacroExpansion = true);
+                              bool AllowIO = true);
 
 /// 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;
 }
 
+llvm::Optional<StringRef> getBuffer(const SourceManager &SM, FileID File,
+                                    bool AllowIO) {
+  // This is similar to the implementation of SourceManager::getCharacterData(),
+  // but uses ContentCache::getRawBuffer() rather than getBuffer() if
+  // AllowIO=false, to avoid triggering file I/O if the file contents aren't
+  // already mapped.
+  bool CharDataInvalid = false;
+  const SrcMgr::SLocEntry &Entry = SM.getSLocEntry(File, &CharDataInvalid);
+  if (CharDataInvalid || !Entry.isFile())
+    return llvm::None;
+  const SrcMgr::ContentCache *Cache = Entry.getFile().getContentCache();
+  const llvm::MemoryBuffer *Buffer =
+      AllowIO ? Cache->getBuffer(SM.getDiagnostics(), SM.getFileManager(),
+                                 SourceLocation(), &CharDataInvalid)
+              : Cache->getRawBuffer();
+  if (!Buffer || CharDataInvalid)
+    return llvm::None;
+  return Buffer->getBuffer();
+}
+
 static bool LineIsMarkedWithNOLINT(const SourceManager &SM, SourceLocation Loc,
                                    unsigned DiagID,
-                                   const ClangTidyContext &Context) {
-  bool Invalid;
-  const char *CharacterData = SM.getCharacterData(Loc, &Invalid);
-  if (Invalid)
+                                   const ClangTidyContext &Context,
+                                   bool AllowIO) {
+  FileID File;
+  unsigned Offset;
+  std::tie(File, Offset) = SM.getDecomposedSpellingLoc(Loc);
+  llvm::Optional<StringRef> Buffer = getBuffer(SM, File, AllowIO);
+  if (!Buffer)
     return false;
 
   // Check if there's a NOLINT on this line.
-  const char *P = CharacterData;
-  while (*P != '\0' && *P != '\r' && *P != '\n')
-    ++P;
-  StringRef RestOfLine(CharacterData, P - CharacterData + 1);
+  StringRef RestOfLine = Buffer->substr(Offset).split('\n').first;
   if (IsNOLINTFound("NOLINT", RestOfLine, DiagID, Context))
     return true;
 
   // Check if there's a NOLINTNEXTLINE on the previous line.
-  const char *BufBegin =
-      SM.getCharacterData(SM.getLocForStartOfFile(SM.getFileID(Loc)), &Invalid);
-  if (Invalid || P == BufBegin)
-    return false;
-
-  // Scan backwards over the current line.
-  P = CharacterData;
-  while (P != BufBegin && *P != '\n')
-    --P;
-
-  // If we reached the begin of the file there is no line before it.
-  if (P == BufBegin)
-    return false;
-
-  // Skip over the newline.
-  --P;
-  const char *LineEnd = P;
-
-  // Now we're on the previous line. Skip to the beginning of it.
-  while (P != BufBegin && *P != '\n')
-    --P;
-
-  RestOfLine = StringRef(P, LineEnd - P + 1);
-  if (IsNOLINTFound("NOLINTNEXTLINE", RestOfLine, DiagID, Context))
-    return true;
-
-  return false;
+  StringRef PrevLine =
+      Buffer->substr(0, Offset).rsplit('\n').first.rsplit('\n').second;
+  return IsNOLINTFound("NOLINTNEXTLINE", PrevLine, DiagID, Context);
 }
 
 static bool LineIsMarkedWithNOLINTinMacro(const SourceManager &SM,
                                           SourceLocation Loc, unsigned DiagID,
-                                          const ClangTidyContext &Context) {
+                                          const ClangTidyContext &Context,
+                                          bool AllowIO) {
   while (true) {
-    if (LineIsMarkedWithNOLINT(SM, Loc, DiagID, Context))
+    if (LineIsMarkedWithNOLINT(SM, Loc, DiagID, Context, AllowIO))
       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) {
+                              bool AllowIO) {
   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, AllowIO);
 }
 
 } // 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);
   }
 };

clang-tools-extra/clangd/ParsedAST.cpp

ASTDiags.setLevelAdjuster([&CTContext](DiagnosticsEngine::Level DiagLevel,
           // to match clang-tidy's behaviour.
           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.
+          // shouldSuppressDiagnostic to avoid I/O.
           bool IsInsideMainFile =
               Info.hasSourceManager() &&
               isInsideMainFile(Info.getLocation(), Info.getSourceManager());
-          if (IsInsideMainFile && tidy::shouldSuppressDiagnostic(
-                                      DiagLevel, Info, *CTContext,
-                                      /* CheckMacroExpansion = */ false)) {
+          if (IsInsideMainFile &&
+              tidy::shouldSuppressDiagnostic(DiagLevel, Info, *CTContext,
+                                             /*AllowIO=*/false)) {
             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(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(
                    "macro expansion"),
               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'"),
           AllOf(
               Diag(Test.range("main"),
                    "use a trailing return type for this function"),
               DiagSource(Diag::ClangTidy),
               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")))
-          ));
+              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;
+      #define BAD2 BAD
+      double h = BAD2;  // NOLINT
     }
   )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 "
struct A {
       A() { [[a]]; /*error-ok*/ } // crash if we compute scopes lazily.
     };
   )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'")));
+  EXPECT_THAT(
+      TU.build().getDiagnostics(),
 }
 
 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());