[clangd] Retrieve documentation for member function instance from index #153337

HighCommander4 · 2025-08-13T02:55:23Z

Fixes clangd/clangd#2290

llvmbot · 2025-08-13T02:55:54Z

@llvm/pr-subscribers-clang
@llvm/pr-subscribers-clang-tools-extra

@llvm/pr-subscribers-clangd

Author: Nathan Ridge (HighCommander4)

Changes

Fixes clangd/clangd#2290

Full diff: https://github.com/llvm/llvm-project/pull/153337.diff

2 Files Affected:

(modified) clang-tools-extra/clangd/CodeComplete.cpp (+13-1)
(modified) clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp (+24-2)

diff --git a/clang-tools-extra/clangd/CodeComplete.cpp b/clang-tools-extra/clangd/CodeComplete.cpp
index 9c17b4ca9b706..da51998b9eec0 100644
--- a/clang-tools-extra/clangd/CodeComplete.cpp
+++ b/clang-tools-extra/clangd/CodeComplete.cpp
@@ -1887,7 +1887,19 @@ class CodeCompleteFlow {
         for (auto &Cand : C.first) {
           if (Cand.SemaResult &&
               Cand.SemaResult->Kind == CodeCompletionResult::RK_Declaration) {
-            auto ID = clangd::getSymbolID(Cand.SemaResult->getDeclaration());
+            const NamedDecl *DeclToLookup = Cand.SemaResult->getDeclaration();
+            // For instantiations of members of class templates, the
+            // documentation will be stored at the member's original
+            // declaration.
+            // FIXME: We'd like to handle fields too but FieldDecl is missing a
+            // method equivalent to getInstantiatedFromMemberFunction().
+            if (const auto *FD = dyn_cast<FunctionDecl>(DeclToLookup)) {
+              if (const auto *InstantiatedFrom =
+                      FD->getInstantiatedFromMemberFunction()) {
+                DeclToLookup = InstantiatedFrom;
+              }
+            }
+            auto ID = clangd::getSymbolID(DeclToLookup);
             if (!ID)
               continue;
             Req.IDs.insert(ID);
diff --git a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
index 5a5d815076e2a..57eb9fcdecb21 100644
--- a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -1154,23 +1154,45 @@ TEST(CompletionTest, CommentsOnMembersFromHeader) {
       /// This is a member function.
       int delta();
     };
+
+    template <typename T>
+    struct beta {
+      /// This is a member field inside a template.
+      int omega;
+
+      /// This is a member function inside a template.
+      int epsilon();
+    };
   )cpp";
 
   auto File = testPath("foo.cpp");
   Annotations Test(R"cpp(
 #include "foo.h"
 alpha a;
-int x = a.^
+beta<int> b;
+int x = a.$p1^;
+int y = b.$p2^;
      )cpp");
   runAddDocument(Server, File, Test.code());
   auto CompletionList =
-      llvm::cantFail(runCodeComplete(Server, File, Test.point(), {}));
+      llvm::cantFail(runCodeComplete(Server, File, Test.point("p1"), {}));
 
   EXPECT_THAT(CompletionList.Completions,
               Contains(AllOf(named("gamma"), doc("This is a member field."))));
   EXPECT_THAT(
       CompletionList.Completions,
       Contains(AllOf(named("delta"), doc("This is a member function."))));
+
+  CompletionList =
+      llvm::cantFail(runCodeComplete(Server, File, Test.point("p2"), {}));
+
+  EXPECT_THAT(CompletionList.Completions,
+              Contains(AllOf(named("omega")
+                             /* FIXME: Doc retrieval does not work yet*/)));
+  EXPECT_THAT(
+      CompletionList.Completions,
+      Contains(AllOf(named("epsilon"),
+                     doc("This is a member function inside a template."))));
 }
 
 TEST(CompletionTest, CommentsOnMembersFromHeaderOverloadBundling) {

HighCommander4 · 2025-08-13T06:25:28Z

clang-tools-extra/clangd/CodeComplete.cpp

Should I be using FunctionDecl::getTemplateInstantiationPattern instead?

I think so? getInstantiatedFromMemberFunction gives you the last pattern that this instantiation comes from, and that pattern could be a half-instantiated template declaration.

So if you're looking for the primary template, I think we should turn to getTemplateInstantiationPattern.

Also note that there are some edge cases where we lack a tracking of the pattern, e.g. a templated friend declaration

Makes sense, thanks! Done.

HighCommander4 · 2025-08-13T06:29:08Z

clang-tools-extra/clangd/CodeComplete.cpp

I found some libSema code that rolls its own equivalent for fields using a DeclContext::lookup() call; we could consider using that approach here as well.

How about merging them into HeuristicResolver?

I appreciate HeuristicResolver is a convenient place to house code that needs to be shared between parser code and clang API clients like clangd, but I'd also like to limit its scope to operations that seem heuristic in nature in some way.

This one just seems like a missing link in the AST, with nothing heuristic about it (the declaration it returns should definitely be one the input decl was instantiated from). So maybe a static method on ASTContext would make sense?

Anyways, I'm going to leave addressing this FIXME to a future patch; I expect methods are the more common / important use case here.

kadircet · 2025-08-14T13:15:49Z

clang-tools-extra/clangd/CodeComplete.cpp

do we know why the discrepancy exists between templated-code itself vs non-templated decls (i.e. how come we get the decl with comments when we're looking at foo<int>())? I wonder if we should change something in sema-codecomplete to refer to "spelled" declaration in completion-result for non-templated cases (e.g. bar in Foo<int>::bar()) rather than working around it here.

do we know why the discrepancy exists between templated-code itself vs non-templated decls (i.e. how come we get the decl with comments when we're looking at foo<int>())?

I'm not sure I'm following. Is foo is a function template here? In what context are you requesting completion?

In any case, here's a reason not to have SemaCodeComplete give us the "spelled" declaration:

template <typename T> struct vector { void push(T); }; int main() { vector<int> v; v.^ // signature shown is `push(int)` }

Returning the instantiated declaration allows us to e.g. show the instantiated signature (push(int) rather than push(T)) which is more helpful to the user.

In that sense, the instantiated declaration provides strictly more information than the spelled declaration; the consumer can always query the spelled declaration (as we're doing here) if desired.

sorry I think I was just testing this wrong (I had comments from AST, not from index), hence I was confused.

but that still led me to https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/ASTContext.cpp#L333-L401. Can we just use that instead to unify behavior with AST-based completion-comments here?

but that still led me to https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/ASTContext.cpp#L333-L401. Can we just use that instead to unify behavior with AST-based completion-comments here?

Done.

I chose DeclTemplate.h as the header to house the shared function, as it seemed like a better fit than ASTContext.h; let me know if you have a different preference.

HighCommander4 · 2025-09-08T06:18:48Z

Review ping

HighCommander4 · 2025-09-28T23:34:22Z

Review ping

kadircet

sorry for taking an eternity here :/

kadircet · 2025-09-29T14:35:39Z

clang-tools-extra/clangd/CodeComplete.cpp

sorry I think I was just testing this wrong (I had comments from AST, not from index), hence I was confused.

but that still led me to https://github.com/llvm/llvm-project/blob/main/clang/lib/AST/ASTContext.cpp#L333-L401. Can we just use that instead to unify behavior with AST-based completion-comments here?

Fixes clangd/clangd#2290

kadircet

thanks, LGTM!

thurstond · 2025-10-06T20:27:36Z

CodeCompleteTests.cpp is failing on one of the buildbots (https://lab.llvm.org/buildbot/#/builders/25/builds/12099/steps/11/logs/stdio), which I suspect might be related to this pull request. Could you please take a look?

******************** TEST 'Clangd Unit Tests :: ./ClangdTests/26/84' FAILED ********************
Script(shard):
--
GTEST_OUTPUT=json:/home/b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/tools/clang/tools/extra/clangd/unittests/./ClangdTests-Clangd Unit Tests-931212-26-84.json GTEST_SHUFFLE=0 GTEST_TOTAL_SHARDS=84 GTEST_SHARD_INDEX=26 /home/b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/tools/clang/tools/extra/clangd/unittests/./ClangdTests
--
Script:
--
/home/b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm_build_ubsan/tools/clang/tools/extra/clangd/unittests/./ClangdTests --gtest_filter=CompletionTest.DynamicIndexIncludeInsertion
--
ASTWorker building file /clangd-test/foo_impl.cpp version null with command 
[/clangd-test]
clang -ffreestanding /clangd-test/foo_impl.cpp
Driver produced command: cc1 -cc1 -triple x86_64-unknown-linux-gnu -fsyntax-only -disable-free -clear-ast-before-backend -main-file-name foo_impl.cpp -mrelocation-model pic -pic-level 2 -pic-is-pie -mframe-pointer=all -fmath-errno -ffp-contract=on -fno-rounding-math -mconstructor-aliases -ffreestanding -target-cpu x86-64 -tune-cpu generic -debugger-tuning=gdb -fdebug-compilation-dir=/clangd-test -fcoverage-compilation-dir=/clangd-test -resource-dir lib/clang/22 -internal-isystem lib/clang/22/include -internal-isystem /usr/local/include -internal-externc-isystem /include -internal-externc-isystem /usr/include -fdeprecated-macro -ferror-limit 19 -fgnuc-version=4.2.1 -fskip-odr-check-in-gmf -fcxx-exceptions -fexceptions -no-round-trip-args -faddrsig -D__GCC_HAVE_DWARF2_CFI_ASM=1 -x c++ /clangd-test/foo_impl.cpp
Building first preamble for /clangd-test/foo_impl.cpp version null
Built preamble of size 238484 for file /clangd-test/foo_impl.cpp version null in 5.45 seconds
indexed preamble AST for /clangd-test/foo_impl.cpp version null:
  symbol slab: 2 symbols, 4680 bytes
  ref slab: 0 symbols, 0 refs, 128 bytes
  relations slab: 0 relations, 24 bytes
Build dynamic index for header symbols with estimated memory usage of 7008 bytes
/home/b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:1034: Failure
Value of: Server.blockUntilIdleForTest()
  Actual: false
Expected: true
indexed file AST for /clangd-test/foo_impl.cpp version null:
  symbol slab: 1 symbols, 4448 bytes
  ref slab: 2 symbols, 2 refs, 4272 bytes
  relations slab: 0 relations, 24 bytes
Build dynamic index for main-file symbols with estimated memory usage of 11576 bytes
/home/b/sanitizer-x86_64-linux-bootstrap-ubsan/build/llvm-project/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp:1034
Value of: Server.blockUntilIdleForTest()
  Actual: false
Expected: true

HighCommander4 · 2025-10-06T21:47:56Z

CodeCompleteTests.cpp is failing on one of the buildbots (https://lab.llvm.org/buildbot/#/builders/25/builds/12099/steps/11/logs/stdio), which I suspect might be related to this pull request. Could you please take a look?

I had a look. While the failing test is in the same file as one modified by this PR, it's (1) in a different test that I don't expect to be affected by this PR, and (2) it's a timeout whose failure is more likely to indicate an unrelated intermittent issue.

A subsequent build on the same buildbot is successful. I'll keep an eye on a few more builds, but I suspect the linked failure is a pre-existing intermittent and the fact that it's in the same file that's touched by this PR was coincidental.

thurstond · 2025-10-07T00:16:37Z

CodeCompleteTests.cpp is failing on one of the buildbots (https://lab.llvm.org/buildbot/#/builders/25/builds/12099/steps/11/logs/stdio), which I suspect might be related to this pull request. Could you please take a look?

I had a look. While the failing test is in the same file as one modified by this PR, it's (1) in a different test that I don't expect to be affected by this PR, and (2) it's a timeout whose failure is more likely to indicate an unrelated intermittent issue.

A subsequent build on the same buildbot is successful. I'll keep an eye on a few more builds, but I suspect the linked failure is a pre-existing intermittent and the fact that it's in the same file that's touched by this PR was coincidental.

Thanks for checking! Sorry for the noise.

HighCommander4 requested review from kadircet, hokein and bevin-hansson August 13, 2025 02:55

llvmbot added clang-tools-extra clangd labels Aug 13, 2025

HighCommander4 mentioned this pull request Aug 13, 2025

Missing documentation for templated method defined in header during autocomplete clangd/clangd#2290

Closed

HighCommander4 commented Aug 13, 2025

View reviewed changes

kadircet requested changes Aug 14, 2025

View reviewed changes

HighCommander4 force-pushed the issue-2290 branch from 8f5fae9 to e0fac46 Compare August 27, 2025 04:01

HighCommander4 requested a review from kadircet August 27, 2025 04:04

kadircet reviewed Sep 29, 2025

View reviewed changes

[clangd] Retrieve documentation for class member instance from index

fc114ff

Fixes clangd/clangd#2290

HighCommander4 force-pushed the issue-2290 branch from e0fac46 to fc114ff Compare October 6, 2025 06:48

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 6, 2025

HighCommander4 requested a review from kadircet October 6, 2025 06:49

kadircet approved these changes Oct 6, 2025

View reviewed changes

HighCommander4 merged commit 255db37 into llvm:main Oct 6, 2025
11 checks passed

[clangd] Retrieve documentation for member function instance from index #153337

[clangd] Retrieve documentation for member function instance from index #153337

Conversation

HighCommander4 commented Aug 13, 2025

Uh oh!

llvmbot commented Aug 13, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HighCommander4 commented Sep 8, 2025

Uh oh!

HighCommander4 commented Sep 28, 2025

Uh oh!

kadircet left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kadircet left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thurstond commented Oct 6, 2025

Uh oh!

HighCommander4 commented Oct 6, 2025

Uh oh!

thurstond commented Oct 7, 2025

Uh oh!

Uh oh!

llvmbot commented Aug 13, 2025 •

edited

Loading