Author: Aleksandr Platonov Date: 2026-09-21T09:55:32+03:00 New Revision: 01e57356732a91d804114acf4cb389d89e9f9635
URL: https://github.com/llvm/llvm-project/commit/01e57356732a91d804114acf4cb389d89e9f9635 DIFF: https://github.com/llvm/llvm-project/commit/01e57356732a91d804114acf4cb389d89e9f9635.diff LOG: [clangd] Extend FeatureModule hooks (#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. 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 Added: Modified: clang-tools-extra/clangd/Diagnostics.cpp clang-tools-extra/clangd/Diagnostics.h clang-tools-extra/clangd/FeatureModule.h clang-tools-extra/clangd/ParsedAST.cpp clang-tools-extra/clangd/Preamble.cpp clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp Removed: ################################################################################ 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..90714fe9101a8 100644 --- a/clang-tools-extra/clangd/FeatureModule.h +++ b/clang-tools-extra/clangd/FeatureModule.h @@ -108,15 +108,34 @@ 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. + /// Reusing a preamble skips the main file's initial preprocessing + /// directives. ReplayPreamble synthesizes callbacks for selected events + /// from that region and captures their recipients before beforeExecute() is + /// called. + 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 616a8d60dfb4a..a35d99e2a5ee2 100644 --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -475,6 +475,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. @@ -703,6 +707,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. @@ -772,6 +780,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..5da3dec41ebd2 100644 --- a/clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp +++ b/clang-tools-extra/clangd/unittests/FeatureModulesTests.cpp @@ -12,16 +12,55 @@ #include "TestTU.h" #include "refactor/Tweak.h" #include "support/Logger.h" +#include "clang/AST/Decl.h" +#include "clang/Frontend/FrontendOptions.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 <functional> #include <memory> namespace clang { namespace clangd { namespace { +struct TestModule final : FeatureModule { + struct Listener final : ASTListener { + Listener(TestModule &Module) : Module(Module) {} + + void beforePPCallbacks(CompilerInstance &CI) override { + if (Module.BeforePPCallbacks) + Module.BeforePPCallbacks(CI); + } + void beforeExecute(CompilerInstance &CI) override { + if (Module.BeforeExecute) + Module.BeforeExecute(CI); + } + void afterExecute(CompilerInstance &CI) override { + if (Module.AfterExecute) + Module.AfterExecute(CI); + } + void finalizeDiagnostic(clangd::Diag &Diag) override { + if (Module.FinalizeDiagnostic) + Module.FinalizeDiagnostic(Diag); + } + + private: + TestModule &Module; + }; + + std::unique_ptr<ASTListener> astListeners() override { + return std::make_unique<Listener>(*this); + } + + std::function<void(CompilerInstance &)> BeforePPCallbacks; + std::function<void(CompilerInstance &)> BeforeExecute; + std::function<void(CompilerInstance &)> AfterExecute; + std::function<void(clangd::Diag &)> FinalizeDiagnostic; +}; + TEST(FeatureModulesTest, ContributesTweak) { static constexpr const char *TweakID = "ModuleTweak"; struct TweakContributingModule final : public FeatureModule { @@ -86,19 +125,53 @@ 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; + }; + std::vector<std::string> Includes; + auto Module = std::make_unique<TestModule>(); + Module->BeforePPCallbacks = [&Includes](CompilerInstance &CI) { + // The preamble build processes the main file's initial directives, + // including #include "header.h", and the included header's contents. The + // main-file build reuses that preamble and skips those directives. + // ReplayPreamble synthesizes InclusionDirective callbacks for the saved + // direct includes. Register only during the main-file build to observe this + // replay, rather than the original include during preamble construction. + if (CI.getFrontendOpts().ProgramAction == frontend::ParseSyntaxOnly) + CI.getPreprocessor().addPPCallbacks( + std::make_unique<IncludeRecorder>(Includes)); + }; + FeatureModuleSet FMS; + FMS.add(std::move(Module)); + + TestTU TU = TestTU::withCode(R"cpp( + #include "header.h" + void mainFileFunc(); // Ends the preamble; parsed during the main-file build. + )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 { - void beforeExecute(CompilerInstance &CI) override { - CI.getPreprocessor().SetSuppressIncludeNotFoundError(true); - } - }; - std::unique_ptr<ASTListener> astListeners() override { - return std::make_unique<Listener>(); - }; + auto Module = std::make_unique<TestModule>(); + Module->BeforeExecute = [](CompilerInstance &CI) { + CI.getPreprocessor().SetSuppressIncludeNotFoundError(true); }; FeatureModuleSet FMS; - FMS.add(std::make_unique<BeforeExecuteModule>()); + FMS.add(std::move(Module)); TestTU TU = TestTU::withCode(R"cpp( /*error-ok*/ @@ -121,6 +194,53 @@ TEST(FeatureModulesTest, BeforeExecute) { } } +TEST(FeatureModulesTest, AfterExecute) { + std::vector<std::string> DeclNames; + auto Module = std::make_unique<TestModule>(); + Module->AfterExecute = [&DeclNames](CompilerInstance &CI) { + for (Decl *D : CI.getASTContext().getTraversalScope()) + if (const auto *ND = llvm::dyn_cast<NamedDecl>(D)) + DeclNames.push_back(ND->getNameAsString()); + }; + FeatureModuleSet FMS; + FMS.add(std::move(Module)); + + TestTU TU = TestTU::withCode(R"cpp( + #include "header.h" + void mainFileFunc(); + )cpp"); + TU.AdditionalFiles["header.h"] = "void headerFunc();"; + TU.FeatureModules = &FMS; + TU.build(); + + // afterExecute runs once clangd has restricted the traversal scope, so the + // declaration from the header is intentionally not visible here. + EXPECT_THAT(DeclNames, testing::ElementsAre("mainFileFunc")); +} + +TEST(FeatureModulesTest, FinalizeDiagnostic) { + unsigned Notes = 0; + unsigned Fixes = 0; + auto Module = std::make_unique<TestModule>(); + Module->FinalizeDiagnostic = [&](clangd::Diag &Diag) { + if (Diag.Message.find("undeclared identifier 'fooo'") == std::string::npos) + return; + Notes = Diag.Notes.size(); + Fixes = Diag.Fixes.size(); + }; + FeatureModuleSet FMS; + FMS.add(std::move(Module)); + + 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)); + EXPECT_EQ(Notes, 1u); + EXPECT_EQ(Fixes, 1u); +} + } // namespace } // namespace clangd } // namespace clang _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
