https://github.com/ChuanqiXu9 updated https://github.com/llvm/llvm-project/pull/187432
>From e7f762595cda45e1ddda2022e8788cee2198c695 Mon Sep 17 00:00:00 2001 From: Chuanqi Xu <[email protected]> Date: Thu, 19 Mar 2026 11:16:44 +0800 Subject: [PATCH] [clangd] [C++ Modules] Skip PCH when TU imports modules Close https://github.com/llvm/llvm-project/issues/181770 PCH doesn't preserve module visibility when modules are imported through #include'd headers, causing incorrect module_unimported_use diagnostics. Set preamble bounds to 0 for such TUs to avoid the issue. And also, I concern there are many problems mixing using PCH and C++20 modules. Although the root cause live in the clang side, we can disable them in clangd right now to give users better diagnostics. --- clang-tools-extra/clangd/ModulesBuilder.cpp | 10 ++ clang-tools-extra/clangd/ModulesBuilder.h | 2 + clang-tools-extra/clangd/ParsedAST.cpp | 17 +++- .../unittests/PrerequisiteModulesTest.cpp | 99 +++++++++++++++++++ 4 files changed, 124 insertions(+), 4 deletions(-) diff --git a/clang-tools-extra/clangd/ModulesBuilder.cpp b/clang-tools-extra/clangd/ModulesBuilder.cpp index 950392b91ac7c..c2a95cff5f9fe 100644 --- a/clang-tools-extra/clangd/ModulesBuilder.cpp +++ b/clang-tools-extra/clangd/ModulesBuilder.cpp @@ -650,6 +650,16 @@ llvm::Error ModulesBuilder::ModulesBuilderImpl::getOrBuildModuleFile( return llvm::Error::success(); } +bool ModulesBuilder::hasRequiredModules(PathRef File) { + std::unique_ptr<ProjectModules> MDB = Impl->getCDB().getProjectModules(File); + if (!MDB) + return false; + + CachingProjectModules CachedMDB(std::move(MDB), + Impl->getProjectModulesCache()); + return !CachedMDB.getRequiredModules(File).empty(); +} + std::unique_ptr<PrerequisiteModules> ModulesBuilder::buildPrerequisiteModulesFor(PathRef File, const ThreadsafeFS &TFS) { diff --git a/clang-tools-extra/clangd/ModulesBuilder.h b/clang-tools-extra/clangd/ModulesBuilder.h index f40a9006e9169..b4d1278e079df 100644 --- a/clang-tools-extra/clangd/ModulesBuilder.h +++ b/clang-tools-extra/clangd/ModulesBuilder.h @@ -94,6 +94,8 @@ class ModulesBuilder { ModulesBuilder &operator=(const ModulesBuilder &) = delete; ModulesBuilder &operator=(ModulesBuilder &&) = delete; + bool hasRequiredModules(PathRef File); + std::unique_ptr<PrerequisiteModules> buildPrerequisiteModulesFor(PathRef File, const ThreadsafeFS &TFS); diff --git a/clang-tools-extra/clangd/ParsedAST.cpp b/clang-tools-extra/clangd/ParsedAST.cpp index 4e873f1257a17..bf4fb5c909bf3 100644 --- a/clang-tools-extra/clangd/ParsedAST.cpp +++ b/clang-tools-extra/clangd/ParsedAST.cpp @@ -430,6 +430,15 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, const PrecompiledPreamble *PreamblePCH = Preamble ? &Preamble->Preamble : nullptr; + // Skip PCH when the TU requires C++20 named modules. Mixing PCH and modules + // may cause issues (e.g., module_decl_not_at_start error when the preamble + // contains 'module;' with #include directives followed by 'import'). + // Here is a simple workaround for better user experience. + if (PreamblePCH && Inputs.ModulesManager && + Inputs.ModulesManager->hasRequiredModules(Filename)) { + PreamblePCH = nullptr; + } + // This is on-by-default in windows to allow parsing SDK headers, but it // breaks many features. Disable it for the main-file (not preamble). CI->getLangOpts().DelayedTemplateParsing = false; @@ -459,7 +468,7 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, // dropped later on to not pay for extra latency by processing them. DiagnosticConsumer *DiagConsumer = &ASTDiags; IgnoreDiagnostics DropDiags; - if (Preamble) { + if (PreamblePCH) { Patch = PreamblePatch::createFullPatch(Filename, Inputs, *Preamble); Patch->apply(*CI); } @@ -663,7 +672,7 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, IncludeStructure Includes; include_cleaner::PragmaIncludes PI; // If we are using a preamble, copy existing includes. - if (Preamble) { + if (Preamble && Patch) { Includes = Preamble->Includes; Includes.MainFileIncludes = Patch->preambleIncludes(); // Replay the preamble includes so that clang-tidy checks can see them. @@ -682,7 +691,7 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, // with non-preamble macros below. MainFileMacros Macros; std::vector<PragmaMark> Marks; - if (Preamble) { + if (Patch) { Macros = Patch->mainFileMacros(); Marks = Patch->marks(); } @@ -744,7 +753,7 @@ ParsedAST::build(llvm::StringRef Filename, const ParseInputs &Inputs, // FIXME: Also skip generation of diagnostics altogether to speed up ast // builds when we are patching a stale preamble. // Add diagnostics from the preamble, if any. - if (Preamble) + if (Patch) llvm::append_range(Diags, Patch->patchedDiags()); // Finally, add diagnostics coming from the AST. { diff --git a/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp b/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp index 3132959a5967c..4728831eb8bd2 100644 --- a/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp +++ b/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp @@ -673,6 +673,105 @@ import M; EXPECT_EQ(HS.PrebuiltModuleFiles, HS2.PrebuiltModuleFiles); } +// Test that module visibility is preserved when importing modules through +// #include'd headers. See https://github.com/llvm/llvm-project/issues/181770 +TEST_F(PrerequisiteModulesTests, ModuleImportThroughInclude) { + MockDirectoryCompilationDatabase CDB(TestDir, FS); + + CDB.addFile("M.cppm", R"cpp( +export module M; +export struct MyType {}; + )cpp"); + + CDB.addFile("Header.h", R"cpp( +import M; + )cpp"); + + CDB.addFile("Use.cpp", R"cpp( +#include "Header.h" +void use() { + MyType t; +} + )cpp"); + + ModulesBuilder Builder(CDB); + + ParseInputs Use = getInputs("Use.cpp", CDB); + Use.ModulesManager = &Builder; + + std::unique_ptr<CompilerInvocation> CI = + buildCompilerInvocation(Use, DiagConsumer); + EXPECT_TRUE(CI); + + auto Preamble = + buildPreamble(getFullPath("Use.cpp"), *CI, Use, /*InMemory=*/true, + /*Callback=*/nullptr); + EXPECT_TRUE(Preamble); + EXPECT_TRUE(Preamble->RequiredModules); + + auto AST = ParsedAST::build(getFullPath("Use.cpp"), Use, std::move(CI), {}, + Preamble); + EXPECT_TRUE(AST); + + // Verify that MyType is usable (no module_unimported_use error) + const NamedDecl &D = findDecl(*AST, "MyType"); + EXPECT_TRUE(D.isFromASTFile()); +} + +// Test that mixing PCH and C++20 named modules doesn't cause +// module_decl_not_at_start error when the preamble contains 'module;' with +// #include directives followed by 'import'. See +// https://github.com/llvm/llvm-project/issues/181770 +TEST_F(PrerequisiteModulesTests, PCHWithNamedModulesIncludeAndImport) { + MockDirectoryCompilationDatabase CDB(TestDir, FS); + + CDB.addFile("A.cppm", R"cpp( +export module A; + )cpp"); + + CDB.addFile("myheader.h", R"cpp( +int x; + )cpp"); + + // Module B has 'module;' with #include, then imports A. + // This used to trigger module_decl_not_at_start error. + CDB.addFile("B.cppm", R"cpp( +module; + +#include "myheader.h" + +export module B; + +import A; + )cpp"); + + ModulesBuilder Builder(CDB); + + ParseInputs BInput = getInputs("B.cppm", CDB); + BInput.ModulesManager = &Builder; + + std::unique_ptr<CompilerInvocation> CI = + buildCompilerInvocation(BInput, DiagConsumer); + EXPECT_TRUE(CI); + + auto Preamble = + buildPreamble(getFullPath("B.cppm"), *CI, BInput, /*InMemory=*/true, + /*Callback=*/nullptr); + EXPECT_TRUE(Preamble); + EXPECT_TRUE(Preamble->RequiredModules); + + auto AST = ParsedAST::build(getFullPath("B.cppm"), BInput, std::move(CI), {}, + Preamble); + EXPECT_TRUE(AST); + + // Verify no module_decl_not_at_start error + for (const auto &D : AST->getDiagnostics()) { + EXPECT_FALSE(llvm::StringRef(D.Message).contains( + "module declaration must occur at the start")) + << "Unexpected error: " << D.Message; + } +} + } // namespace } // namespace clang::clangd _______________________________________________ cfe-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
