[clang][CodeComplete] skip explicit obj param in SignatureHelp #146649

MythreyaK · 2025-07-02T08:33:03Z

Seems like the code segment here is similar to the one that I updated in the previous PR #146258. It seems like the code is dissimilar enough in both the function bodies that it probably isn't possible to cleanly move it into a single place to prevent repetition.

My current understanding is that this line checks that the given overload exists, but not that other overloads do not, right? How do I ensure that this is the only overload?

CHECK-CC2: OVERLOAD: [#void#]foo(int arg)

Please let me know if any other improvements can be made!

llvmbot · 2025-07-02T08:33:35Z

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

@llvm/pr-subscribers-clang

Author: Mythreya Kuricheti (MythreyaK)

Changes

Fixes clangd/clangd#2284.

Seems like the code segment here is similar to the one that I updated in the previous PR #146258. It seems like the code is dissimilar enough in both the function bodies that it probably isn't possible to cleanly move it into a single place to prevent repetition.

Added 2 tests. Please let me know if any other improvements can be made!

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

3 Files Affected:

(modified) clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp (+28)
(modified) clang/lib/Sema/SemaCodeComplete.cpp (+10-1)
(modified) clang/test/CodeCompletion/skip-explicit-object-parameter.cpp (+18-6)

diff --git a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
index b7c64c7a06745..b18d712ee9aef 100644
--- a/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
+++ b/clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp
@@ -3266,6 +3266,34 @@ TEST(SignatureHelpTest, VariadicType) {
   }
 }
 
+TEST(SignatureHelpTest, SkipExplicitObjectParameter) {
+  Annotations Code(R"cpp(
+    struct A {
+      void foo(this auto&& self, int arg); 
+    };
+    int main() {
+      A a {};
+      a.foo(^);
+    }
+  )cpp");
+
+  auto TU = TestTU::withCode(Code.code());
+  TU.ExtraArgs = {"-std=c++23"};
+
+  MockFS FS;
+  auto Inputs = TU.inputs(FS);
+
+  auto Preamble = TU.preamble();
+  ASSERT_TRUE(Preamble);
+
+  const auto Result = signatureHelp(testPath(TU.Filename), Code.point(),
+                                    *Preamble, Inputs, MarkupKind::PlainText);
+
+  EXPECT_EQ(1, Result.signatures.size());
+
+  EXPECT_THAT(Result.signatures[0], AllOf(sig("foo([[int arg]]) -> void")));
+}
+
 TEST(CompletionTest, IncludedCompletionKinds) {
   Annotations Test(R"cpp(#include "^)cpp");
   auto TU = TestTU::withCode(Test.code());
diff --git a/clang/lib/Sema/SemaCodeComplete.cpp b/clang/lib/Sema/SemaCodeComplete.cpp
index b5d4a94da83df..97fefd3c93897 100644
--- a/clang/lib/Sema/SemaCodeComplete.cpp
+++ b/clang/lib/Sema/SemaCodeComplete.cpp
@@ -4019,7 +4019,9 @@ static void AddOverloadParameterChunks(
       Function ? Function->getNumParams() : Prototype->getNumParams();
 
   for (unsigned P = Start; P != NumParams; ++P) {
-    if (Function && Function->getParamDecl(P)->hasDefaultArg() && !InOptional) {
+    const ParmVarDecl *Param = Function->getParamDecl(P);
+
+    if (Function && Param->hasDefaultArg() && !InOptional) {
       // When we see an optional default argument, put that argument and
       // the remaining default arguments into a new, optional string.
       CodeCompletionBuilder Opt(Result.getAllocator(),
@@ -4034,6 +4036,13 @@ static void AddOverloadParameterChunks(
       return;
     }
 
+    // C++23 introduces an explicit object parameter, a.k.a. "deducing this"
+    // Skip it for autocomplete and treat the next parameter as the first
+    // parameter
+    if (FirstParameter && Param->isExplicitObjectParameter()) {
+      continue;
+    }
+
     if (FirstParameter)
       FirstParameter = false;
     else
diff --git a/clang/test/CodeCompletion/skip-explicit-object-parameter.cpp b/clang/test/CodeCompletion/skip-explicit-object-parameter.cpp
index 55c16bb126fee..0eb71dce95849 100644
--- a/clang/test/CodeCompletion/skip-explicit-object-parameter.cpp
+++ b/clang/test/CodeCompletion/skip-explicit-object-parameter.cpp
@@ -6,9 +6,21 @@ int main() {
   A a {};
   a.
 }
-// RUN: %clang_cc1 -cc1 -fsyntax-only -code-completion-at=%s:%(line-2):5 -std=c++23 %s | FileCheck %s
-// CHECK: COMPLETION: A : A::
-// CHECK-NEXT: COMPLETION: foo : [#void#]foo(<#int arg#>)
-// CHECK-NEXT: COMPLETION: operator= : [#A &#]operator=(<#const A &#>)
-// CHECK-NEXT: COMPLETION: operator= : [#A &#]operator=(<#A &&#>)
-// CHECK-NEXT: COMPLETION: ~A : [#void#]~A()
+// RUN: %clang_cc1 -cc1 -fsyntax-only -code-completion-at=%s:%(line-2):5 -std=c++23 %s | FileCheck -check-prefix=CHECK-CC1 %s
+// CHECK-CC1: COMPLETION: A : A::
+// CHECK-NEXT-CC1: COMPLETION: foo : [#void#]foo(<#int arg#>)
+// CHECK-NEXT-CC1: COMPLETION: operator= : [#A &#]operator=(<#const A &#>)
+// CHECK-NEXT-CC1: COMPLETION: operator= : [#A &#]operator=(<#A &&#>)
+// CHECK-NEXT-CC1: COMPLETION: ~A : [#void#]~A()
+
+struct B {
+  template <typename T>
+  void foo(this T&& self, int arg);
+};
+
+int main2() {
+  B b {};
+  b.foo();
+}
+// RUN: %clang_cc1 -cc1 -fsyntax-only -code-completion-at=%s:%(line-2):9 -std=c++23 %s | FileCheck -check-prefix=CHECK-CC2 %s
+// CHECK-CC2: OVERLOAD: [#void#]foo(int arg)

zwuis

Thank you for the fix!

zwuis · 2025-07-02T10:43:40Z

clang/test/CodeCompletion/skip-explicit-object-parameter.cpp

-// CHECK-NEXT: COMPLETION: operator= : [#A &#]operator=(<#const A &#>)
-// CHECK-NEXT: COMPLETION: operator= : [#A &#]operator=(<#A &&#>)
-// CHECK-NEXT: COMPLETION: ~A : [#void#]~A()
+// RUN: %clang_cc1 -cc1 -fsyntax-only -code-completion-at=%s:%(line-2):5 -std=c++23 %s | FileCheck -check-prefix=CHECK-CC1 %s


-cc1 can be removed.

Done, thanks!

cor3ntin · 2025-07-02T12:30:45Z

How does completion handle things like this?

struct S {
    void f(this S, int);
};

int main() {
    (&S::f)(S{}, 0);
}

MythreyaK · 2025-07-02T13:25:03Z

How does completion handle things like this?

@cor3ntin Oh I totally forgot about this! Thanks! I'll check it in the evening.

MythreyaK · 2025-07-03T02:02:46Z

Seems to be working as expected. I'll add a test case for this as well.

MythreyaK · 2025-07-03T09:36:34Z

The completion for (&A::foo2) is just (A, int), not foo(A, int). I tried to add a test case, like in this godbolt example,

struct A {
    void foo1(this A self, int arg) {}
    template <typename T>
    void foo2(this T&& self, int arg, float arg2) {}
};

int main() {
    A a {};
    (&A::foo1)(a, 1);
    (&A::foo2<A&>)(a, 1, 3.4); // possible?
    // (&A::foo2<A&>)(a, 1 /* , 3.4*/); // causes a compiler crash
    return 0;
}

but causes a crash (example). So I haven't added those cases, but left a TODO. Should I leave a TODO: PR 146649 instead?

MythreyaK · 2025-07-03T09:50:48Z

clang/test/CodeCompletion/skip-explicit-object-parameter.cpp

+  b.foo();
+}
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:%(line-2):9 -std=c++23 %s | FileCheck -check-prefix=CHECK-CC2 %s
+// CHECK-CC2: OVERLOAD: [#void#]foo(int arg)


My current understanding is that this line checks that the given overload exists, but not that other overloads do not. Do I need to ensure that this is the only overload?

It may be possible with CHECK-NOT, but to be honest I wouldn't bother trying to express any non-trivial logic in this test suite. The clangd unit test has coverage for this (it will start failing if there are multiple overloads due to our use of ElementsAre().)

cor3ntin · 2025-07-04T13:50:40Z

but causes a crash (example). So I haven't added those cases, but left a TODO. Should I leave a TODO: PR 146649 instead?

I opened 147046

cor3ntin · 2025-07-11T07:34:19Z

@MythreyaK you should be able to add these tess now

HighCommander4

Thanks for the patch and the nice test coverage! A few comments/questions:

HighCommander4 · 2025-07-28T05:30:01Z

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

    };

    int main() {
      A a {};
-      a.^
+      a.$c1^s


(nit: why is there an s after the invocation point?)

Removed 👍

HighCommander4 · 2025-07-28T05:35:16Z

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

-      a.^
+      a.$c1^s
+      (&A::ba$c2^;
+      // TODO: (&A::fo$c3^


Could you elaborate on this TODO? Is this something the frontend crashes on or incorrectly rejects? Or just something we need to handle in our implementation? (In the latter case it may be better to still test it and put a TODO in the place where we're making assertion about the results.)

This originally resulted in a crash, but was fixed by cor3ntin.

HighCommander4 · 2025-07-28T05:49:01Z

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

+    // TODO: snippet suffix is empty for c2
+    EXPECT_THAT(Result.Completions,
+                ElementsAre(AllOf(named("bar"), signature("(int arg)"),
+                                  snippetSuffix(""))));


We will want snippetSuffix to be (${1:A self}, ${2: int arg}), right (since the user will need to write both arguments in this scenario)?

Given that, would it be fair to say that signature being (int arg) is also wrong (it should probably be consistent with the snippet suffix)?

(Maybe just amend the TODO comment to talk about both?)

We will want snippetSuffix to be (${1:A self}, ${2: int arg})
signature being (int arg) is also wrong

Yep, the snippet suffix is missing, I wasn't sure how to handle this--in this PR, or a new one?

I updated the tests to "pass", to sync up with a more recent main, but there seems to be a regression (I had to update my test in the latest commit, here, comment here). Snippet suffix that existed previously is now missing. Should we investigate this in this PR or in a new one?

Yep, the snippet suffix is missing, I wasn't sure how to handle this--in this PR, or a new one?

I'm ok with either option.

HighCommander4 · 2025-07-29T05:20:25Z

clang/test/CodeCompletion/skip-explicit-object-parameter.cpp

+  b.foo();
+}
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:%(line-2):9 -std=c++23 %s | FileCheck -check-prefix=CHECK-CC2 %s
+// CHECK-CC2: OVERLOAD: [#void#]foo(int arg)


It may be possible with CHECK-NOT, but to be honest I wouldn't bother trying to express any non-trivial logic in this test suite. The clangd unit test has coverage for this (it will start failing if there are multiple overloads due to our use of ElementsAre().)

MythreyaK · 2025-07-29T10:05:23Z

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

+    EXPECT_THAT(
+        Result.Completions,
+        UnorderedElementsAre(
+            AllOf(named("foo"), signature("(int arg)"), snippetSuffix("")),
+            AllOf(named("bar"), signature("(int arg)"), snippetSuffix(""))));


potential regression?

Hah, it's from the removal of the s at the end of a.$c1^s! I guess there was a reason for it 😆

I guess the reason is that now that we have a subsequent line that starts with (, the parser gets confused by the next token after the invocation point being (.

A semicolon (;) at the end of the line seems to do the trick as well, I suggest we go with that rather than an s (which confused me as a seemingly random character).

HighCommander4 · 2025-07-30T00:44:41Z

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

+    const auto Result = signatureHelp(testPath(TU.Filename), Code.point("c3"),
+                                      *Preamble, Inputs, MarkupKind::PlainText);
+    // TODO: We expect 1 signature here
+    // EXPECT_EQ(1U, Result.signatures.size());


In situations like this, I find it useful to assert the actual results (e.g. EXPECT_EQ(0U, Result.signatures.size())) even if we know they're wrong.

That makes it more likely we that we remember to update the test if the issue gets fixed (since the assertion will start failing).

HighCommander4 · 2025-07-30T01:01:10Z

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

+    // TODO: We expect 1 signature here
+    // EXPECT_EQ(1U, Result.signatures.size());
+
+    // EXPECT_THAT(Result.signatures[0], AllOf(sig("([[A]], [[int]]) ->


Is (A, int) -> void really the signature we'd like here?

The compiler seems to accept any type for self when using this syntax, e.g. (&A::foo)(42, 42); compiles (which is admittedly a bit strange).

So I think the expectation will need to be something like (auto&&, int) -> void.

The compiler seems to accept any type for self when using this syntax, e.g. (&A::foo)(42, 42); compiles (which is admittedly a bit strange).

Oh yeah, I missed that. @cor3ntin, would this be a bug?

So I think the expectation will need to be something like (auto&&, int) -> void.

If this is a bug, then would (A&&, int) -> void be more accurate?

HighCommander4 · 2025-07-30T01:15:11Z

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

+    EXPECT_THAT(
+        Result.Completions,
+        UnorderedElementsAre(
+            AllOf(named("foo"), signature("(int arg)"), snippetSuffix("")),
+            AllOf(named("bar"), signature("(int arg)"), snippetSuffix(""))));


Hah, it's from the removal of the s at the end of a.$c1^s! I guess there was a reason for it 😆

I guess the reason is that now that we have a subsequent line that starts with (, the parser gets confused by the next token after the invocation point being (.

A semicolon (;) at the end of the line seems to do the trick as well, I suggest we go with that rather than an s (which confused me as a seemingly random character).

HighCommander4 · 2025-07-30T01:18:43Z

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

+    // TODO: snippet suffix is empty for c2
+    EXPECT_THAT(Result.Completions,
+                ElementsAre(AllOf(named("bar"), signature("(int arg)"),
+                                  snippetSuffix(""))));


Yep, the snippet suffix is missing, I wasn't sure how to handle this--in this PR, or a new one?

I'm ok with either option.

HighCommander4 · 2025-07-30T01:28:25Z

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

+    EXPECT_THAT(
+        Result.Completions,
+        ElementsAre(AllOf(named("foo"), signature("<class self:auto>(int arg)"),
+                          snippetSuffix("<${1:class self:auto}>"))));


Multiple things about this seem to be wrong:

We should not get an explicit template parameter list in the snippet, because the template parameter can be deduced from the first argument. (We're expecting the user to write (&A::foo)(a, 42); most of the time, not (&A::foo<A>)(a, 42);).

The snippet suffix is missing function parameters altogether.

The signature should have two function parameters, not one.

It's fine to assert the current (buggy) behaviour, but please add a comment describing why it's wrong, and what we should get instead.

HighCommander4 · 2025-07-30T01:31:00Z

clang/test/CodeCompletion/skip-explicit-object-parameter.cpp

-// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:%(line-2):10 -std=c++23 %s | FileCheck -check-prefix=CHECK-CC3 %s
-// CHECK-CC3: COMPLETION: bar : [#void#]bar(<#int arg#>)
+// RUN: %clang_cc1 -fsyntax-only -code-completion-at=%s:%(line-3):10 -std=c++23 %s | FileCheck -check-prefix=CHECK-CC3 %s
+// CHECK-CC3: COMPLETION: foo : [#void#]foo<<#class self:auto#>>(<#int arg#>)


(As discussed in the unit test, this signature is not what we want and we should add a comment saying so.)

MythreyaK · 2025-07-30T07:04:23Z

Hah, it's from the removal of the s at the end of a.$c1^s! I guess there was a reason for it 😆

You're right, nice catch! I thought the 's' was from me not Ctrl+S correctly 😆.

I'm ok with either option.

I'm not sure what the changes might be, so I my thought is that if we can get the tests in, we'll be able to monitor behavior changes.

It's fine to assert the current (buggy) behaviour, but please add a comment describing why it's wrong, and what we should get instead.

Added a few TODOs, do they look okay?

HighCommander4

Thanks -- I think this looks pretty good!

HighCommander4 · 2025-07-31T05:26:29Z

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

+                                      *Preamble, Inputs, MarkupKind::PlainText);
+    // TODO: We expect 1 signature here, with this signature
+    EXPECT_EQ(0U, Result.signatures.size());
+    // EXPECT_THAT(Result.signatures[0], AllOf(sig("([[A&&]], [[int]]) ->


nit: auto&& rather than A&&, I believe

HighCommander4 · 2025-07-31T05:26:39Z

clang-tools-extra/clangd/unittests/CodeCompleteTests.cpp

+  // TODO: llvm/llvm-project/146649
+  // This is incorrect behavior. Correct Result should be a variant of,
+  // c2: signature = (A&& self, int arg)
+  //     snippet = (${1: A&& self}, ${2: int arg})


likewise here

llvmbot added clang Clang issues not falling into any other category clang-tools-extra clangd clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Jul 2, 2025

MythreyaK force-pushed the mythreyak/skip-expl-obj-param-signature-help branch from 592ba1a to 7f8581f Compare July 2, 2025 09:03

zwuis reviewed Jul 2, 2025

View reviewed changes

zwuis requested a review from HighCommander4 July 2, 2025 10:44

MythreyaK commented Jul 3, 2025

View reviewed changes

HighCommander4 reviewed Jul 29, 2025

View reviewed changes

MythreyaK added 3 commits July 29, 2025 00:48

[clang][CodeComplete] skip explicit obj param in SignatureHelp

816845b

Update tests from code review

caf4482

rebase updates

e08005e

MythreyaK force-pushed the mythreyak/skip-expl-obj-param-signature-help branch from a7fae22 to e08005e Compare July 29, 2025 09:59

MythreyaK commented Jul 29, 2025

View reviewed changes

HighCommander4 requested changes Jul 30, 2025

View reviewed changes

code review

203317b

HighCommander4 approved these changes Jul 31, 2025

View reviewed changes

code review

d719247

HighCommander4 merged commit 3466cdb into llvm:main Jul 31, 2025
9 checks passed

MythreyaK deleted the mythreyak/skip-expl-obj-param-signature-help branch July 31, 2025 21:52

[clang][CodeComplete] skip explicit obj param in SignatureHelp #146649

[clang][CodeComplete] skip explicit obj param in SignatureHelp #146649

Conversation

MythreyaK commented Jul 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

zwuis left a comment

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!

cor3ntin commented Jul 2, 2025

Uh oh!

MythreyaK commented Jul 2, 2025

Uh oh!

MythreyaK commented Jul 3, 2025

Uh oh!

MythreyaK commented Jul 3, 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!

cor3ntin commented Jul 4, 2025

Uh oh!

cor3ntin commented Jul 11, 2025

Uh oh!

HighCommander4 left a comment

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!

MythreyaK Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

MythreyaK Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

MythreyaK commented Jul 30, 2025

Uh oh!

HighCommander4 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

MythreyaK commented Jul 2, 2025 •

edited

Loading

llvmbot commented Jul 2, 2025 •

edited

Loading

MythreyaK commented Jul 3, 2025 •

edited

Loading

MythreyaK Jul 29, 2025 •

edited

Loading

MythreyaK Jul 30, 2025 •

edited

Loading