[Clang] Fix handling of placeholder variables name in init captures

[cor3ntin]

We were incorrectly not deduplicating results when looking up _ which, for a lambda init capture, would result in an ambiguous lookup.

The same bug caused some diagnostic notes to be emitted twice.

Fixes #107024

[llvmbot]

@llvm/pr-subscribers-clang

Author: cor3ntin (cor3ntin)

Changes

We were incorrectly not deduplicating results when looking up _ which, for a lambda init capture, would result in an ambiguous lookup.

The same bug caused some diagnostic notes to be emitted twice.

Fixes #107024

---

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

3 Files Affected:

• (modified) clang/docs/ReleaseNotes.rst (+1)
• (modified) clang/lib/Sema/SemaLookup.cpp (+1-1)
• (modified) clang/test/SemaCXX/cxx2c-placeholder-vars.cpp (+4-2)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index fc940db4813948..af4388eada8e5a 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -339,6 +339,7 @@ Bug Fixes to C++ Support
 - Template parameter names are considered in the name lookup of out-of-line class template
   specialization right before its declaration context. (#GH64082)
 - Fixed a constraint comparison bug for friend declarations. (#GH78101)
+- Fix handling of ``_`` as the name of a lambda's init capture variable. (#GH107024)
 
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
diff --git a/clang/lib/Sema/SemaLookup.cpp b/clang/lib/Sema/SemaLookup.cpp
index 7a6a64529f52ec..d3d4bf27ae7283 100644
--- a/clang/lib/Sema/SemaLookup.cpp
+++ b/clang/lib/Sema/SemaLookup.cpp
@@ -570,7 +570,7 @@ void LookupResult::resolveKind() {
 
     // For non-type declarations, check for a prior lookup result naming this
     // canonical declaration.
-    if (!D->isPlaceholderVar(getSema().getLangOpts()) && !ExistingI) {
+    if (!ExistingI) {
       auto UniqueResult = Unique.insert(std::make_pair(D, I));
       if (!UniqueResult.second) {
         // We've seen this entity before.
diff --git a/clang/test/SemaCXX/cxx2c-placeholder-vars.cpp b/clang/test/SemaCXX/cxx2c-placeholder-vars.cpp
index 5cf66b48784e91..29ca3b5ef3df72 100644
--- a/clang/test/SemaCXX/cxx2c-placeholder-vars.cpp
+++ b/clang/test/SemaCXX/cxx2c-placeholder-vars.cpp
@@ -50,14 +50,16 @@ void f() {
 
 void lambda() {
     (void)[_ = 0, _ = 1] { // expected-warning {{placeholder variables are incompatible with C++ standards before C++2c}} \
-                           // expected-note 4{{placeholder declared here}}
+                           // expected-note 2{{placeholder declared here}}
         (void)_++; // expected-error {{ambiguous reference to placeholder '_', which is defined multiple times}}
     };
 
     {
         int _ = 12;
-        (void)[_ = 0]{}; // no warning (different scope)
+        (void)[_ = 0]{ return _;}; // no warning (different scope)
     }
+
+    auto GH107024 = [_ = 42]() { return _; }();
 }
 
 namespace global_var {

[zyn0217]

One thing I'm not entirely clear about is why we would end up with two same placeholder Decls in LookupResult.

It seems that IdResolver results in two same duplicate Decls for such case, but I don't know why.

(However I'd still approve it because this reverts the condition to the state before the placeholder patch.)

[zyn0217]

I took a cursory look, and it seems that we pushed the same init capture to the scope chain three times:

1. Sema::ActOnLambdaExpressionAfterIntroducer
   

   llvm-project/clang/lib/Sema/SemaLambda.cpp

   Lines 1212 to 1214 in 6c59dfb

   	if (auto *V = dyn_cast<VarDecl>(Var))
   	CheckShadow(CurrentScope, V);
   	PushOnScopeChains(Var, CurrentScope, false);

2. Sema::ActOnLambdaExpressionAfterIntroducer

llvm-project/clang/lib/Sema/SemaLambda.cpp

Lines 1324 to 1327 in 6c59dfb

	if (C->Init.isUsable()) {
	addInitCapture(LSI, cast<VarDecl>(Var), C->Kind == LCK_ByRef);
	PushOnScopeChains(Var, CurScope, false);
	} else {

3. Sema::ActOnStartOfLambdaDefinition
   

   llvm-project/clang/lib/Sema/SemaLambda.cpp

   Lines 1468 to 1475 in 6c59dfb

   	for (auto &&C : LSI->Captures) {
   	if (!C.isVariableCapture())
   	continue;
   	ValueDecl *Var = C.getVariable();
   	if (Var && Var->isInitCapture()) {
   	PushOnScopeChains(Var, CurScope, false);
   	}
   	}

I'm confused if they're all necessary...

[zyn0217]

Yeah, 2 is a duplicate of 1, but 3 is necessary because that constitutes a lambda body scope.

[cor3ntin]

The second was clearly duplicated.
The first one introduce a scope for the captures, so that captures are available in the declaration of the lambda (before its compound statement).

But the body introduces a new scope wherein we inject the parameters - and we also need to reinject the captures to make sure they cannot be shadowed by a local variables

[a = 0] 
// capture scope
{
 // body scope
 int a = 42; // error!
}```

[cor3ntin]

Should we backport this?

[zyn0217]

I think so, yes.

[cor3ntin]

/cherry-pick eec1fac

[llvmbot]

Failed to cherry-pick: eec1fac

https://github.com/llvm/llvm-project/actions/runs/10688677657

Please manually backport the fix and push it to your github fork. Once this is done, please create a pull request

[nikic]

For reference, the manual cherry pick is #107214.