[clang-tools-extra] [clangd] Do not collect macros when clang-tidy checks call into the preprocessor (PR #106329)

[via cfe-commits]

llvmbot wrote:


<!--LLVM PR SUMMARY COMMENT-->

@llvm/pr-subscribers-clang-tools-extra

Author: Nathan Ridge (HighCommander4)

<details>
<summary>Changes</summary>

Fixes https://github.com/llvm/llvm-project/issues/99617

---
Full diff: https://github.com/llvm/llvm-project/pull/106329.diff


4 Files Affected:

- (modified) clang-tools-extra/clangd/CollectMacros.cpp (+1) 
- (modified) clang-tools-extra/clangd/CollectMacros.h (+8) 
- (modified) clang-tools-extra/clangd/ParsedAST.cpp (+7-1) 
- (modified) clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp (+17) 


``````````diff
diff --git a/clang-tools-extra/clangd/CollectMacros.cpp b/clang-tools-extra/clangd/CollectMacros.cpp
index c5ba8d903ba482..96298ee3ea50ae 100644
--- a/clang-tools-extra/clangd/CollectMacros.cpp
+++ b/clang-tools-extra/clangd/CollectMacros.cpp
@@ -32,6 +32,7 @@ void CollectMainFileMacros::add(const Token &MacroNameTok, const MacroInfo *MI,
   if (Loc.isInvalid() || Loc.isMacroID())
     return;
 
+  assert(isInsideMainFile(Loc, SM));
   auto Name = MacroNameTok.getIdentifierInfo()->getName();
   Out.Names.insert(Name);
   size_t Start = SM.getFileOffset(Loc);
diff --git a/clang-tools-extra/clangd/CollectMacros.h b/clang-tools-extra/clangd/CollectMacros.h
index e3900c08e5df7b..e7198641d8d53c 100644
--- a/clang-tools-extra/clangd/CollectMacros.h
+++ b/clang-tools-extra/clangd/CollectMacros.h
@@ -82,6 +82,14 @@ class CollectMainFileMacros : public PPCallbacks {
 
   void SourceRangeSkipped(SourceRange R, SourceLocation EndifLoc) override;
 
+  // Called when the AST build is done to disable further recording
+  // of macros by this class. This is needed because some clang-tidy
+  // checks can trigger PP callbacks by calling directly into the
+  // preprocessor. Such calls are not interleaved with FileChanged()
+  // in the expected way, leading this class to erroneously process
+  // macros that are not in the main file.
+  void doneParse() { InMainFile = false; }
+
 private:
   void add(const Token &MacroNameTok, const MacroInfo *MI,
            bool IsDefinition = false, bool InConditionalDirective = false);
diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp
index 2bd1fbcad2ada0..ee012846da9f5e 100644
--- a/clang-tools-extra/clangd/ParsedAST.cpp
+++ b/clang-tools-extra/clangd/ParsedAST.cpp
@@ -681,7 +681,9 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
     Marks = Patch->marks();
   }
   auto &PP = Clang->getPreprocessor();
-  PP.addPPCallbacks(std::make_unique<CollectMainFileMacros>(PP, Macros));
+  auto MacroCollector = std::make_unique<CollectMainFileMacros>(PP, Macros);
+  auto *MacroCollectorPtr = MacroCollector.get(); // so we can call doneParse()
+  PP.addPPCallbacks(std::move(MacroCollector));
 
   PP.addPPCallbacks(
       collectPragmaMarksCallback(Clang->getSourceManager(), Marks));
@@ -702,6 +704,10 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs,
     log("Execute() failed when building AST for {0}: {1}", MainInput.getFile(),
         toString(std::move(Err)));
 
+  // Disable the macro collector for the remainder of this function, e.g.
+  // clang-tidy checkers.
+  MacroCollectorPtr->doneParse();
+
   // We have to consume the tokens before running clang-tidy to avoid collecting
   // tokens from running the preprocessor inside the checks (only
   // modernize-use-trailing-return-type does that today).
diff --git a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
index 25d2f03e0b366b..096f77e414f5a5 100644
--- a/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
+++ b/clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
@@ -940,6 +940,23 @@ TEST(DiagnosticTest, ClangTidySelfContainedDiagsFormatting) {
                         withFix(equalToFix(ExpectedFix2))))));
 }
 
+TEST(DiagnosticsTest, ClangTidyCallingIntoPreprocessor) {
+  std::string Main = R"cpp(
+    extern "C" {
+    #include "b.h"
+    }
+  )cpp";
+  std::string Header = R"cpp(
+    #define EXTERN extern
+    EXTERN int waldo();
+  )cpp";
+  auto TU = TestTU::withCode(Main);
+  TU.AdditionalFiles["b.h"] = Header;
+  TU.ClangTidyProvider = addTidyChecks("modernize-use-trailing-return-type");
+  // Check that no assertion failures occur during the build
+  TU.build();
+}
+
 TEST(DiagnosticsTest, Preprocessor) {
   // This looks like a preamble, but there's an #else in the middle!
   // Check that:

``````````

</details>


https://github.com/llvm/llvm-project/pull/106329