Skip to content

[clangd] Do not collect macros when clang-tidy checks call into the preprocessor #106329

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Aug 29, 2024
Merged

[clangd] Do not collect macros when clang-tidy checks call into the preprocessor #106329

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions clang-tools-extra/clangd/CollectMacros.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
8 changes: 8 additions & 0 deletions clang-tools-extra/clangd/CollectMacros.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
8 changes: 7 additions & 1 deletion clang-tools-extra/clangd/ParsedAST.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand All @@ -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();
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 what doneParse() in the current patch does

?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// 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).
Expand Down
17 changes: 17 additions & 0 deletions clang-tools-extra/clangd/unittests/DiagnosticsTests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
Loading