https://github.com/ArcsinX created https://github.com/llvm/llvm-project/pull/221054
Feature modules may need to participate at points in AST construction that beforeExecute() and sawDiagnostic() cannot represent. This PR adds: - `beforePPCallbacks()` for installing preprocessing observers before clangd starts collecting include and macro events. - `afterExecute()` for work that needs a completed AST after token collection and traversal-scope restriction. - `finalizeDiagnostic()` for transformations that need the complete diagnostic, including its notes and fixes. For example, clang-tidy needs to register preprocessing callbacks before preamble events are replayed, run AST matchers after clangd restricts the traversal scope, and process diagnostics after their notes and fixes are attached. I added tests to demonstrate the need for new hooks, which made the tests somewhat bulky. I would be happy to simplify them (especially the `afterExecute` test) if there is no pressing need to prove that the current hooks lack sufficient functionality. These changes prepare moving the clang-tidy implementation into a FeatureModule. RFC: https://discourse.llvm.org/t/rfc-clangd-move-clang-tidy-integration-into-a-featuremodule/91707 >From 21b990e0a8087d45dbad5c8dc575810602c542bf Mon Sep 17 00:00:00 2001 From: Aleksandr Platonov <[email protected]> Date: Thu, 3 Sep 2026 16:01:06 +0300 Subject: [PATCH] [clangd] Extend FeatureModule AST lifecycle hooks Feature modules may need to participate at points in AST construction that beforeExecute() and sawDiagnostic() cannot represent. Add beforePPCallbacks() for installing preprocessing observers before clangd starts collecting include and macro events. This is required when an observer must be present in the callback chain captured for preamble replay. Add afterExecute() for work that needs a completed AST after token collection and traversal-scope restriction. Add finalizeDiagnostic() for transformations that need the complete diagnostic, including its notes and fixes, while retaining sawDiagnostic() as the early per-diagnostic hook. For example, clang-tidy needs to register preprocessing callbacks before preamble events are replayed, run AST matchers after clangd restricts the traversal scope, and process diagnostics after their notes and fixes are attached. These changes prepare moving the clang-tidy implementation into a FeatureModule. --- clang-tools-extra/clangd/Diagnostics.cpp | 3 + clang-tools-extra/clangd/Diagnostics.h | 8 +- clang-tools-extra/clangd/FeatureModule.h | 15 + clang-tools-extra/clangd/ParsedAST.cpp | 10 + clang-tools-extra/clangd/Preamble.cpp | 13 + .../clangd/unittests/FeatureModulesTests.cpp | 273 ++++++++++++++++++ 6 files changed, 320 insertions(+), 2 deletions(-) diff --git a/clang-tools-extra/clangd/Diagnostics.cpp b/clang-tools-extra/clangd/Diagnostics.cpp index 7dfc6ebb3fe0e..28351c7cc2246 100644 --- a/clang-tools-extra/clangd/Diagnostics.cpp +++ b/clang-tools-extra/clangd/Diagnostics.cpp @@ -622,6 +622,9 @@ std::vector<Diag> StoreDiags::take(const clang::tidy::ClangTidyContext *Tidy) { } setTags(Diag); } + if (Finalizer) + for (auto &Diag : Output) + Finalizer(Diag); // Deduplicate clang-tidy diagnostics -- some clang-tidy checks may emit // duplicated messages due to various reasons (e.g. the check doesn't handle // template instantiations well; clang-tidy alias checks). diff --git a/clang-tools-extra/clangd/Diagnostics.h b/clang-tools-extra/clangd/Diagnostics.h index d433abb530151..ae710f351dbfe 100644 --- a/clang-tools-extra/clangd/Diagnostics.h +++ b/clang-tools-extra/clangd/Diagnostics.h @@ -154,15 +154,18 @@ class StoreDiags : public DiagnosticConsumer { DiagnosticsEngine::Level, const clang::Diagnostic &)>; using DiagCallback = std::function<void(const clang::Diagnostic &, clangd::Diag &)>; + using DiagFinalizer = std::function<void(Diag &)>; /// If set, possibly adds fixes for diagnostics using \p Fixer. void contributeFixes(DiagFixer Fixer) { this->Fixer = Fixer; } /// If set, this allows the client of this class to adjust the level of /// diagnostics, such as promoting warnings to errors, or ignoring /// diagnostics. void setLevelAdjuster(LevelAdjuster Adjuster) { this->Adjuster = Adjuster; } - /// Invokes a callback every time a diagnostics is completely formed. Handler - /// of the callback can also mutate the diagnostic. + /// Invokes a callback when a main diagnostic is first formed, before notes + /// and fixes are attached. The callback can mutate the diagnostic. void setDiagCallback(DiagCallback CB) { DiagCB = std::move(CB); } + /// Invokes a callback after notes and fixes have been attached. + void setDiagFinalizer(DiagFinalizer F) { Finalizer = std::move(F); } private: void flushLastDiag(); @@ -170,6 +173,7 @@ class StoreDiags : public DiagnosticConsumer { DiagFixer Fixer = nullptr; LevelAdjuster Adjuster = nullptr; DiagCallback DiagCB = nullptr; + DiagFinalizer Finalizer = nullptr; std::vector<Diag> Output; std::optional<LangOptions> LangOpts; std::optional<Diag> LastDiag; diff --git a/clang-tools-extra/clangd/FeatureModule.h b/clang-tools-extra/clangd/FeatureModule.h index 55ca908a6ec51..84a8dcda051f9 100644 --- a/clang-tools-extra/clangd/FeatureModule.h +++ b/clang-tools-extra/clangd/FeatureModule.h @@ -108,15 +108,30 @@ class FeatureModule { /// Listeners are destroyed once the AST is built. virtual ~ASTListener() = default; + /// Called before every AST build, after the Preprocessor and ASTConsumer + /// are set up, but before clangd installs its include and macro collectors. + /// Modules should only use this when their PPCallbacks must observe + /// preamble events replayed during a main-file build. + virtual void beforePPCallbacks(CompilerInstance &CI) {} + /// Called before every AST build, both for main file and preamble. The call /// happens immediately before FrontendAction::Execute(), with Preprocessor /// set up already and after BeginSourceFile() on main file was called. virtual void beforeExecute(CompilerInstance &CI) {} + /// Called after FrontendAction::Execute() for a main-file build, once + /// clangd has collected tokens and restricted the AST traversal scope. + /// The preprocessor has not received EndSourceFile() yet. + virtual void afterExecute(CompilerInstance &CI) {} + /// Called everytime a diagnostic is encountered. Modules can use this /// modify the final diagnostic, or store some information to surface code /// actions later on. virtual void sawDiagnostic(const clang::Diagnostic &, clangd::Diag &) {} + + /// Called after a diagnostic is fully assembled, including notes and + /// fixes, and before it is returned to the caller. + virtual void finalizeDiagnostic(clangd::Diag &) {} }; /// Can be called asynchronously before building an AST. virtual std::unique_ptr<ASTListener> astListeners() { return nullptr; } diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp index d84c577c3842e..110aae575233b 100644 --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -470,6 +470,10 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, for (const auto &L : ASTListeners) L->sawDiagnostic(D, Diag); }); + ASTDiags.setDiagFinalizer([&ASTListeners](clangd::Diag &Diag) { + for (const auto &L : ASTListeners) + L->finalizeDiagnostic(Diag); + }); // Adjust header search options to load the built module files recorded // in RequiredModules. @@ -683,6 +687,10 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, } } + // ReplayPreamble must capture callbacks installed by feature modules. + for (const auto &L : ASTListeners) + L->beforePPCallbacks(*Clang); + IncludeStructure Includes; include_cleaner::PragmaIncludes PI; // If we are using a preamble, copy existing includes. @@ -752,6 +760,8 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, trace::Span Tracer("ClangTidyMatch"); CTFinder.matchAST(Clang->getASTContext()); } + for (const auto &L : ASTListeners) + L->afterExecute(*Clang); // XXX: This is messy: clang-tidy checks flush some diagnostics at EOF. // However Action->EndSourceFile() would destroy the ASTContext! diff --git a/clang-tools-extra/clangd/Preamble.cpp b/clang-tools-extra/clangd/Preamble.cpp index 31f141f62eb7f..7e68a721a9fca 100644 --- a/clang-tools-extra/clangd/Preamble.cpp +++ b/clang-tools-extra/clangd/Preamble.cpp @@ -90,9 +90,11 @@ class CppFilePreambleCallbacks : public PreambleCallbacks { public: CppFilePreambleCallbacks( PathRef File, PreambleBuildStats *Stats, bool ParseForwardingFunctions, + std::function<void(CompilerInstance &)> BeforePPCallbacks, std::function<void(CompilerInstance &)> BeforeExecuteCallback) : File(File), Stats(Stats), ParseForwardingFunctions(ParseForwardingFunctions), + BeforePPCallbacks(std::move(BeforePPCallbacks)), BeforeExecuteCallback(std::move(BeforeExecuteCallback)) {} IncludeStructure takeIncludes() { return std::move(Includes); } @@ -152,6 +154,8 @@ class CppFilePreambleCallbacks : public PreambleCallbacks { LangOpts = &CI.getLangOpts(); SourceMgr = &CI.getSourceManager(); PP = &CI.getPreprocessor(); + if (BeforePPCallbacks) + BeforePPCallbacks(CI); Includes.collect(CI); Pragmas.record(CI); if (BeforeExecuteCallback) @@ -204,6 +208,7 @@ class CppFilePreambleCallbacks : public PreambleCallbacks { const Preprocessor *PP = nullptr; PreambleBuildStats *Stats; bool ParseForwardingFunctions; + std::function<void(CompilerInstance &)> BeforePPCallbacks; std::function<void(CompilerInstance &)> BeforeExecuteCallback; std::optional<CapturedASTCtx> CapturedCtx; }; @@ -596,6 +601,10 @@ buildPreamble(PathRef FileName, CompilerInvocation CI, for (const auto &L : ASTListeners) L->sawDiagnostic(D, Diag); }); + PreambleDiagnostics.setDiagFinalizer([&ASTListeners](clangd::Diag &Diag) { + for (const auto &L : ASTListeners) + L->finalizeDiagnostic(Diag); + }); auto VFS = Inputs.TFS->view(Inputs.CompileCommand.Directory); llvm::IntrusiveRefCntPtr<DiagnosticsEngine> PreambleDiagsEngine = CompilerInstance::createDiagnostics(*VFS, CI.getDiagnosticOpts(), @@ -624,6 +633,10 @@ buildPreamble(PathRef FileName, CompilerInvocation CI, CppFilePreambleCallbacks CapturedInfo( FileName, Stats, Inputs.Opts.PreambleParseForwardingFunctions, + [&ASTListeners](CompilerInstance &CI) { + for (const auto &L : ASTListeners) + L->beforePPCallbacks(CI); + }, [&ASTListeners](CompilerInstance &CI) { for (const auto &L : ASTListeners) L->beforeExecute(CI); diff --git a/clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp b/clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp index 2d89c659110b9..3f11a1d8069b6 100644 --- a/clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp +++ b/clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp @@ -12,11 +12,19 @@ #include "TestTU.h" #include "refactor/Tweak.h" #include "support/Logger.h" +#include "clang/AST/ASTConsumer.h" +#include "clang/AST/Decl.h" +#include "clang/Frontend/FrontendOptions.h" +#include "clang/Frontend/MultiplexConsumer.h" +#include "clang/Lex/Lexer.h" +#include "clang/Lex/PPCallbacks.h" #include "clang/Lex/PreprocessorOptions.h" #include "llvm/Support/Error.h" #include "gmock/gmock.h" #include "gtest/gtest.h" +#include <array> #include <memory> +#include <optional> namespace clang { namespace clangd { @@ -86,6 +94,58 @@ TEST(FeatureModulesTest, SuppressDiags) { } } +TEST(FeatureModulesTest, BeforePPCallbacks) { + struct IncludeRecorder : public PPCallbacks { + IncludeRecorder(std::vector<std::string> &Includes) : Includes(Includes) {} + + void InclusionDirective(SourceLocation, const Token &, StringRef FileName, + bool, CharSourceRange, OptionalFileEntryRef, + StringRef, StringRef, const clang::Module *, bool, + SrcMgr::CharacteristicKind) override { + Includes.push_back(FileName.str()); + } + + private: + std::vector<std::string> &Includes; + }; + struct PPCallbackModule final : public FeatureModule { + struct Listener : public FeatureModule::ASTListener { + Listener(std::vector<std::string> &Includes) : Includes(Includes) {} + + void beforePPCallbacks(CompilerInstance &CI) override { + // The preamble build sees this include directly. Register only during + // the main-file build to verify the callback sees the replayed event. + if (CI.getFrontendOpts().ProgramAction == frontend::ParseSyntaxOnly) + CI.getPreprocessor().addPPCallbacks( + std::make_unique<IncludeRecorder>(Includes)); + } + + private: + std::vector<std::string> &Includes; + }; + + PPCallbackModule(std::vector<std::string> &Includes) : Includes(Includes) {} + std::unique_ptr<ASTListener> astListeners() override { + return std::make_unique<Listener>(Includes); + }; + + private: + std::vector<std::string> &Includes; + }; + + std::vector<std::string> Includes; + FeatureModuleSet FMS; + FMS.add(std::make_unique<PPCallbackModule>(Includes)); + + TestTU TU = TestTU::withCode(R"cpp( + #include "header.h" + )cpp"); + TU.AdditionalFiles["header.h"] = ""; + TU.FeatureModules = &FMS; + TU.build(); + EXPECT_THAT(Includes, testing::ElementsAre("header.h")); +} + TEST(FeatureModulesTest, BeforeExecute) { struct BeforeExecuteModule final : public FeatureModule { struct Listener : public FeatureModule::ASTListener { @@ -121,6 +181,219 @@ TEST(FeatureModulesTest, BeforeExecute) { } } +TEST(FeatureModulesTest, AfterExecute) { + struct AfterExecuteState { + bool ReenterPreprocessorInConsumer = false; + bool HandledTranslationUnit = false; + bool ConsumerSawWholeTranslationUnit = false; + bool AfterExecuteCalled = false; + bool ReenteredPreprocessor = false; + std::vector<std::string> DeclNames; + }; + struct AfterExecuteModule final : public FeatureModule { + struct Consumer : public ASTConsumer { + Consumer(AfterExecuteState &S, CompilerInstance &CI) : S(S), CI(CI) {} + + void HandleTranslationUnit(ASTContext &Ctx) override { + S.HandledTranslationUnit = true; + auto Scope = Ctx.getTraversalScope(); + S.ConsumerSawWholeTranslationUnit = + Scope.size() == 1 && Scope.front() == Ctx.getTranslationUnitDecl(); + Pending = &Ctx; + if (S.ReenterPreprocessorInConsumer) + reenterAtEOF(CI); + } + + void run(CompilerInstance &CI) { + if (!Pending) + return; + SourceLocation MainFileDeclLoc; + for (Decl *D : Pending->getTraversalScope()) { + if (const auto *ND = llvm::dyn_cast<NamedDecl>(D)) { + S.DeclNames.push_back(ND->getNameAsString()); + MainFileDeclLoc = ND->getLocation(); + } + } + + if (MainFileDeclLoc.isValid()) + S.ReenteredPreprocessor = reenterPreprocessor(CI, MainFileDeclLoc); + } + + private: + static void reenterAtEOF(CompilerInstance &CI) { + Token End; + End.startToken(); + auto &SM = CI.getSourceManager(); + End.setLocation(SM.getLocForEndOfFile(SM.getMainFileID())); + End.setKind(tok::eof); + std::array<Token, 1> Stream{End}; + auto &PP = CI.getPreprocessor(); + PP.EnterTokenStream(Stream, /*DisableMacroExpansion=*/false, + /*IsReinject=*/false); + PP.Lex(End); + } + + static bool reenterPreprocessor(CompilerInstance &CI, + SourceLocation MainFileDeclLoc) { + Token Reinjected; + if (Lexer::getRawToken(MainFileDeclLoc, Reinjected, + CI.getSourceManager(), CI.getLangOpts())) + return false; + auto &PP = CI.getPreprocessor(); + PP.LookUpIdentifierInfo(Reinjected); + Token End; + End.startToken(); + End.setKind(tok::eof); + std::array<Token, 2> Stream{Reinjected, End}; + PP.EnterTokenStream(Stream, /*DisableMacroExpansion=*/false, + /*IsReinject=*/false); + do { + PP.Lex(Reinjected); + } while (Reinjected.isNot(tok::eof)); + return true; + } + + AfterExecuteState &S; + CompilerInstance &CI; + ASTContext *Pending = nullptr; + }; + + struct Listener : public FeatureModule::ASTListener { + Listener(AfterExecuteState &S) : S(S) {} + + void beforeExecute(CompilerInstance &CI) override { + std::vector<std::unique_ptr<ASTConsumer>> Consumers; + Consumers.push_back(CI.takeASTConsumer()); + auto Deferred = std::make_unique<Consumer>(S, CI); + DeferredConsumer = Deferred.get(); + Consumers.push_back(std::move(Deferred)); + CI.setASTConsumer( + std::make_unique<MultiplexConsumer>(std::move(Consumers))); + } + + void afterExecute(CompilerInstance &CI) override { + S.AfterExecuteCalled = true; + if (DeferredConsumer) + DeferredConsumer->run(CI); + } + + private: + AfterExecuteState &S; + Consumer *DeferredConsumer = nullptr; + }; + + AfterExecuteModule(AfterExecuteState &S) : S(S) {} + std::unique_ptr<ASTListener> astListeners() override { + return std::make_unique<Listener>(S); + }; + + private: + AfterExecuteState &S; + }; + + // HandleTranslationUnit is too early for work that re-enters the + // preprocessor: clangd's token collector is still installed, observes the + // extra token, and cannot build a valid TokenBuffer afterwards. + AfterExecuteState EarlyState; + EarlyState.ReenterPreprocessorInConsumer = true; + FeatureModuleSet EarlyFMS; + EarlyFMS.add(std::make_unique<AfterExecuteModule>(EarlyState)); + TestTU EarlyTU = TestTU::withCode("int mainFileFunc();"); + EarlyTU.FeatureModules = &EarlyFMS; + EXPECT_DEATH_IF_SUPPORTED((void)EarlyTU.build(), + "Couldn't map expanded token"); + + AfterExecuteState State; + FeatureModuleSet FMS; + FMS.add(std::make_unique<AfterExecuteModule>(State)); + + TestTU TU = TestTU::withCode(R"cpp( + #include "header.h" + inline int mainFileFunc() { return 0; } + )cpp"); + TU.AdditionalFiles["header.h"] = "void headerFunc();"; + TU.FeatureModules = &FMS; + auto AST = TU.build(); + // The multiplexed consumer runs before clangd replaces the whole-TU + // traversal scope with the declarations originating in the main file. It + // therefore cannot observe the finalized scope used by afterExecute below. + EXPECT_TRUE(State.HandledTranslationUnit); + EXPECT_TRUE(State.ConsumerSawWholeTranslationUnit); + EXPECT_TRUE(State.AfterExecuteCalled); + EXPECT_TRUE(State.ReenteredPreprocessor); + + // afterExecute runs once clangd has restricted the traversal scope, so the + // declaration from the header is intentionally not visible here. + EXPECT_THAT(State.DeclNames, testing::ElementsAre("mainFileFunc")); + + // The deferred preprocessing does not affect the token buffer: every parsed + // main-file token is still present exactly once and in source order. + std::vector<std::string> Tokens; + for (const auto &Tok : AST.getTokens().expandedTokens()) + if (Tok.kind() != tok::eof) + Tokens.push_back(Tok.text(AST.getSourceManager()).str()); + EXPECT_THAT(Tokens, testing::ElementsAre("inline", "int", "mainFileFunc", "(", + ")", "{", "return", "0", ";", "}")); +} + +TEST(FeatureModulesTest, FinalizeDiagnostic) { + struct DiagnosticState { + std::optional<clangd::Diag> AtSawDiagnostic; + std::optional<clangd::Diag> AtFinalization; + } State; + struct DiagnosticModule final : public FeatureModule { + struct Listener : public FeatureModule::ASTListener { + Listener(DiagnosticState &State) : State(State) {} + + void sawDiagnostic(const clang::Diagnostic &, + clangd::Diag &Diag) override { + if (Diag.Message.find("undeclared identifier 'fooo'") == + std::string::npos) + return; + State.AtSawDiagnostic = Diag; + } + + void finalizeDiagnostic(clangd::Diag &Diag) override { + if (Diag.Message.find("undeclared identifier 'fooo'") == + std::string::npos) + return; + State.AtFinalization = Diag; + } + + private: + DiagnosticState &State; + }; + + DiagnosticModule(DiagnosticState &State) : State(State) {} + std::unique_ptr<ASTListener> astListeners() override { + return std::make_unique<Listener>(State); + }; + + private: + DiagnosticState &State; + }; + FeatureModuleSet FMS; + FMS.add(std::make_unique<DiagnosticModule>(State)); + + TestTU TU = TestTU::withCode(R"cpp( + void foo(); + void bar() { fooo(); } // error-ok + )cpp"); + TU.FeatureModules = &FMS; + EXPECT_THAT(TU.build().getDiagnostics(), testing::SizeIs(1)); + // sawDiagnostic runs as soon as clangd creates the primary diagnostic. The + // subsequent note and the typo correction have not been attached yet. + ASSERT_TRUE(State.AtSawDiagnostic); + EXPECT_THAT(State.AtSawDiagnostic->Notes, testing::IsEmpty()); + EXPECT_THAT(State.AtSawDiagnostic->Fixes, testing::IsEmpty()); + + // finalizeDiagnostic sees the assembled diagnostic after clangd has + // associated its note and fix with the primary diagnostic. + ASSERT_TRUE(State.AtFinalization); + EXPECT_THAT(State.AtFinalization->Notes, testing::SizeIs(1)); + EXPECT_THAT(State.AtFinalization->Fixes, testing::SizeIs(1)); +} + } // namespace } // namespace clangd } // namespace clang _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
