llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-clangd Author: Chuanqi Xu (ChuanqiXu9) <details> <summary>Changes</summary> According to https://github.com/ChuanqiXu9/clangd-for-modules/issues/9, I surprisingly found the support for C++20 modules doesn't support code completion well. After debugging, I found there are problems: (1) We forgot to call `adjustHeaderSearchOptions` in code complete. This may be an easy oversight. (2) In `CodeCompleteOptions::getClangCompleteOpts`, we may set `LoadExternal` as false when index is available. But we have support modules with index. So it is conflicting. Given modules are opt in now, I think it makes sense to to set LoadExternal as true when modules are enabled. This is a small fix and I wish it can land faster. --- Full diff: https://github.com/llvm/llvm-project/pull/110083.diff 3 Files Affected: - (modified) clang-tools-extra/clangd/CodeComplete.cpp (+13-5) - (modified) clang-tools-extra/clangd/CodeComplete.h (+1-1) - (modified) clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp (+40) ``````````diff diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp index 89eee392837af4..f6f0d9a1be3c37 100644 --- a/clang-tools-extra/clangd/CodeComplete.cpp +++ b/clang-tools-extra/clangd/CodeComplete.cpp @@ -892,8 +892,9 @@ static bool isExcludedMember(const NamedDecl &D) { // within the callback. struct CompletionRecorder : public CodeCompleteConsumer { CompletionRecorder(const CodeCompleteOptions &Opts, + bool ForceLoadExternal, llvm::unique_function<void()> ResultsCallback) - : CodeCompleteConsumer(Opts.getClangCompleteOpts()), + : CodeCompleteConsumer(Opts.getClangCompleteOpts(ForceLoadExternal)), CCContext(CodeCompletionContext::CCC_Other), Opts(Opts), CCAllocator(std::make_shared<GlobalCodeCompletionAllocator>()), CCTUInfo(CCAllocator), ResultsCallback(std::move(ResultsCallback)) { @@ -1409,6 +1410,9 @@ bool semaCodeComplete(std::unique_ptr<CodeCompleteConsumer> Consumer, Clang->getPreprocessorOpts().SingleFileParseMode = CompletingInPreamble; Clang->setCodeCompletionConsumer(Consumer.release()); + if (Input.Preamble.RequiredModules) + Input.Preamble.RequiredModules->adjustHeaderSearchOptions(Clang->getHeaderSearchOpts()); + SyntaxOnlyAction Action; if (!Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0])) { log("BeginSourceFile() failed when running codeComplete for {0}", @@ -1629,11 +1633,15 @@ class CodeCompleteFlow { SpecFuzzyFind->Result = startAsyncFuzzyFind(*Opts.Index, *SpecReq); } + // FIXME: If we're using C++20 modules, force the lookup process to load external decls, + // since currently the index doesn't support C++20 modules. + bool ForceLoadExternal = (bool)SemaCCInput.Preamble.RequiredModules; + // We run Sema code completion first. It builds an AST and calculates: // - completion results based on the AST. // - partial identifier and context. We need these for the index query. CodeCompleteResult Output; - auto RecorderOwner = std::make_unique<CompletionRecorder>(Opts, [&]() { + auto RecorderOwner = std::make_unique<CompletionRecorder>(Opts, ForceLoadExternal, [&]() { assert(Recorder && "Recorder is not set"); CCContextKind = Recorder->CCContext.getKind(); IsUsingDeclaration = Recorder->CCContext.isUsingDeclaration(); @@ -1691,7 +1699,7 @@ class CodeCompleteFlow { Recorder = RecorderOwner.get(); - semaCodeComplete(std::move(RecorderOwner), Opts.getClangCompleteOpts(), + semaCodeComplete(std::move(RecorderOwner), Opts.getClangCompleteOpts(ForceLoadExternal), SemaCCInput, &Includes); logResults(Output, Tracer); return Output; @@ -2108,7 +2116,7 @@ class CodeCompleteFlow { } // namespace -clang::CodeCompleteOptions CodeCompleteOptions::getClangCompleteOpts() const { +clang::CodeCompleteOptions CodeCompleteOptions::getClangCompleteOpts(bool ForceLoadExternal) const { clang::CodeCompleteOptions Result; Result.IncludeCodePatterns = EnableSnippets; Result.IncludeMacros = true; @@ -2122,7 +2130,7 @@ clang::CodeCompleteOptions CodeCompleteOptions::getClangCompleteOpts() const { // When an is used, Sema is responsible for completing the main file, // the index can provide results from the preamble. // Tell Sema not to deserialize the preamble to look for results. - Result.LoadExternal = !Index; + Result.LoadExternal = ForceLoadExternal || !Index; Result.IncludeFixIts = IncludeFixIts; return Result; diff --git a/clang-tools-extra/clangd/CodeComplete.h b/clang-tools-extra/clangd/CodeComplete.h index a7c1ae95dcbf49..336e84f0a7724c 100644 --- a/clang-tools-extra/clangd/CodeComplete.h +++ b/clang-tools-extra/clangd/CodeComplete.h @@ -41,7 +41,7 @@ struct CodeCompletion; struct CodeCompleteOptions { /// Returns options that can be passed to clang's completion engine. - clang::CodeCompleteOptions getClangCompleteOpts() const; + clang::CodeCompleteOptions getClangCompleteOpts(bool ForceLoadExternal) const; /// When true, completion items will contain expandable code snippets in /// completion (e.g. `return ${1:expression}` or `foo(${1:int a}, ${2:int diff --git a/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp b/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp index 7bbb95c8b8d67f..f1cdf9e899f4d8 100644 --- a/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp +++ b/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp @@ -402,6 +402,46 @@ import A; EXPECT_TRUE(D.isFromASTFile()); } +// An end to end test for code complete in modules +TEST_F(PrerequisiteModulesTests, CodeCompleteTest) { + MockDirectoryCompilationDatabase CDB(TestDir, FS); + + CDB.addFile("A.cppm", R"cpp( +export module A; +export void printA(); + )cpp"); + + llvm::StringLiteral UserContents = R"cpp( +import A; +void func() { + print^ +} +)cpp"; + + CDB.addFile("Use.cpp", UserContents); + Annotations Test(UserContents); + + 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 Result = codeComplete(getFullPath("Use.cpp"), Test.point(), + Preamble.get(), Use, {}); + EXPECT_FALSE(Result.Completions.empty()); + EXPECT_EQ(Result.Completions[0].Name, "printA"); +} + } // namespace } // namespace clang::clangd `````````` </details> https://github.com/llvm/llvm-project/pull/110083 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
