[clangd] Do not collect macros when clang-tidy checks call into the preprocessor #106329
Conversation
|
@llvm/pr-subscribers-clangd @llvm/pr-subscribers-clang-tools-extra Author: Nathan Ridge (HighCommander4) ChangesFixes #99617 Full diff: https://github.com/llvm/llvm-project/pull/106329.diff 4 Files Affected:
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:
|
| @@ -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(); | |||
There was a problem hiding this comment.
The first approach I tried was to remove CollectMainFileMacros from the preprocessor callbacks at this point, however the current Preprocessor API does not make that easy to do (adding a new callback creates a PPChainedCallbacks, and there are further internal calls made to addPPCallbacks() that chain other ones on top of ours). So, I opted to "neutralize" our callback rather than try to remove it.
There was a problem hiding this comment.
thanks it's unfortunate but i guess makes sense.
in theory, we can have similar issues with all the others PPCallbcks that we installed. Moreover if others were to use clang-tidy as a library, they'll run into similar pitfalls. As the flow is:
- Inject PPCallbacks
- Build AST
- Pass AST to clang-ast-consumer (in this case clang-tidy)
- Invoke EndOfMainFile
So there might be some value in injecting an extra callback between Build AST and Pass AST to consumer. We could properly reset our PPCallbacks to recognize leaving main file for such situations. It's unfortunate that both FileChanged and LexedFileChanged are designed to operate with a contract that hints "new file/location" will be valid. It makes such a semantic possibly breaking. Leaving that idea here in case you want to follow up on that (I'd be happy to review), but I can see that it's much more involved, possibly without anything breaking (and if it does we can always ask people to turn that check off until we fix the issue).
There was a problem hiding this comment.
Just to make sure I understand correctly, the idea is:
- add a new method to
PPCallbacks, e.g.BuildASTDone()or such - add a call to this new method in [place in libTooling, e.g. FrontendAction or such]
- override
CollectMainFileMacros::BuildASTDone()to do whatdoneParse()in the current patch does
?
There was a problem hiding this comment.
add a new method to PPCallbacks, e.g. BuildASTDone() or such
I'd first see if we can extend existing (Lexed)FileChanged callbacks to fit this use case without breaking any users.
but if that doesn't work, yes, a new callback would be needed.
| @@ -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(); | |||
There was a problem hiding this comment.
thanks it's unfortunate but i guess makes sense.
in theory, we can have similar issues with all the others PPCallbcks that we installed. Moreover if others were to use clang-tidy as a library, they'll run into similar pitfalls. As the flow is:
- Inject PPCallbacks
- Build AST
- Pass AST to clang-ast-consumer (in this case clang-tidy)
- Invoke EndOfMainFile
So there might be some value in injecting an extra callback between Build AST and Pass AST to consumer. We could properly reset our PPCallbacks to recognize leaving main file for such situations. It's unfortunate that both FileChanged and LexedFileChanged are designed to operate with a contract that hints "new file/location" will be valid. It makes such a semantic possibly breaking. Leaving that idea here in case you want to follow up on that (I'd be happy to review), but I can see that it's much more involved, possibly without anything breaking (and if it does we can always ask people to turn that check off until we fix the issue).
Fixes #99617