[clangd] [C++20] [Modules] Support code complete for C++20 modules #110083
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@llvm/pr-subscribers-clang-tools-extra @llvm/pr-subscribers-clang-modules Author: Chuanqi Xu (ChuanqiXu9) ChangesAccording to ChuanqiXu9/clangd-for-modules#9, I surprisingly found the support for C++20 modules doesn't support code completion well. After debugging, I found there are problems: 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:
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
|
|
@llvm/pr-subscribers-clangd Author: Chuanqi Xu (ChuanqiXu9) ChangesAccording to ChuanqiXu9/clangd-for-modules#9, I surprisingly found the support for C++20 modules doesn't support code completion well. After debugging, I found there are problems: 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:
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
|
You can test this locally with the following command:git-clang-format --diff 2b0a708f41dd6291ee744704d43febc975e3d026 178607e217a084b51efacefa92a1714f12b662db --extensions h,cpp -- clang-tools-extra/clangd/CodeComplete.cpp clang-tools-extra/clangd/CodeComplete.h clang-tools-extra/clangd/tool/ClangdMain.cpp clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cppView the diff from clang-format here.diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp
index 6711eb7dc1..6d403b6d2d 100644
--- a/clang-tools-extra/clangd/CodeComplete.cpp
+++ b/clang-tools-extra/clangd/CodeComplete.cpp
@@ -1410,7 +1410,8 @@ bool semaCodeComplete(std::unique_ptr<CodeCompleteConsumer> Consumer,
Clang->setCodeCompletionConsumer(Consumer.release());
if (Input.Preamble.RequiredModules)
- Input.Preamble.RequiredModules->adjustHeaderSearchOptions(Clang->getHeaderSearchOpts());
+ Input.Preamble.RequiredModules->adjustHeaderSearchOptions(
+ Clang->getHeaderSearchOpts());
SyntaxOnlyAction Action;
if (!Action.BeginSourceFile(*Clang, Clang->getFrontendOpts().Inputs[0])) {
diff --git a/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp b/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp
index 691a93e7ac..046a145fda 100644
--- a/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp
+++ b/clang-tools-extra/clangd/unittests/PrerequisiteModulesTest.cpp
@@ -435,7 +435,7 @@ void func() {
/*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());
@@ -474,7 +474,7 @@ void func() {
/*Callback=*/nullptr);
EXPECT_TRUE(Preamble);
EXPECT_TRUE(Preamble->RequiredModules);
-
+
auto Result = signatureHelp(getFullPath("Use.cpp"), Test.point(),
*Preamble.get(), Use, MarkupKind::PlainText);
EXPECT_FALSE(Result.signatures.empty());
|
kadircet
approved these changes
Sep 30, 2024
Comment on lines
56
to
57
| /// This is helpful for cases the index can't provide symbols, e.g., the | ||
| /// unsupported yet modules. |
There was a problem hiding this comment.
e.g. with experimental c++20 modules
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
According to ChuanqiXu9/clangd-for-modules#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
adjustHeaderSearchOptionsin code complete. This may be an easy oversight.(2) In
CodeCompleteOptions::getClangCompleteOpts, we may setLoadExternalas 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.