[clang] [Clang] [Sema] WIP: Fix dependence of DREs in lambdas with an explicit object parameter (PR #84473)
via cfe-commits
cfe-commits at lists.llvm.org
Fri Mar 8 04:55:12 PST 2024
https://github.com/Sirraide created https://github.com/llvm/llvm-project/pull/84473
This is still WIP, but I figured I’d open a pr anyway to share what I’ve discovered so far. In short, dependence of captures in lambdas with an explicit object parameter is all kinds of broken at the moment. [[temp.dep.expr]](https://eel.is/c++draft/temp.dep.expr) states that
> An [id-expression](https://eel.is/c++draft/expr.prim.id.general#nt:id-expression) is type-dependent if [...] its terminal name is
> - associated by name lookup with an entity captured by copy ([[expr.prim.lambda.capture]](https://eel.is/c++draft/expr.prim.lambda.capture)) in a [lambda-expression](https://eel.is/c++draft/expr.prim.lambda.general#nt:lambda-expression) that has an explicit object parameter whose type is dependent ([[dcl.fct]](https://eel.is/c++draft/dcl.fct))
In issue #84163, it was observed that, if the lambda object is `const` but the explicit object parameter is not `const` but is type-dependent, any by-value captures are not treated as `const`, even though they should be:
```c++
const auto l1 = [x](this auto&) { x = 42; };
l1(); // Should error because `x` ends up being `const`, but we don’t.
```
This seems to be due to several compounding issues:
1. we’re treating by-reference captures as dependent rather than by-value captures;
2. tree transform doesn’t seem to check whether referring to such a by-value capture makes a DRE dependent;
3. when checking whether a DRE refers to an such a by-value capture, we only check the immediately enclosing lambda, and not any parent lambdas;
4. we also don’t check for implicit by-value captures;
5. attempting to determine whether a lambda has an explicit object parameter by checking the `LambdaScopeInfo`’s `ExplicitObjectParameter` doesn’t seem to work properly; I presume this is because this member refers to a `ParmVarDecl` that isn’t populated (yet) when we perform template instantiaion, which causes issues with nested lambdas that each have an explicit object parameter.
This pr should (eventually) fix all of these:
1. This is just due to a misplaced `!`, so that’s an easy fix.
2. This entails checking if the DRE is captured by value by a lambda with an explicit object parameter in `TreeTransform<Derived>::TransformDeclRefExpr`. That’s also an easy fix.
3. Currently, I’m simply iterating over all enclosing lambdas and checking if any of them capture the var decl that the DRE refers to by-value (explicitly or implicitly) rather than just looking at the parent lambda.
4. Check for implicit captures as well in 3.
5. Check for `isExplicitObjectMemberFunction()` on the lambda’s `CXXMethodDecl` instead.
This seems to have fixed all issues with captured variables; however, capturing `this` by value (and then accessing NSDMs) suffers from the same problem. My current approach for a fix is adding a `CapturedByCopyInLambdaWithExplicitObjectParameter` to `CXXThisExpr` and making `this` type-dependent, which is how this is tracked for DREs, but that’s still WIP. If anyone has a better idea, feel free to let me know.
Lastly, there’s also another issue (#84425) that seems to involve capturing lambdas with explicit object parameters; I have yet to determine if that one is related to this as well.
This fixes #84163.
>From 870e6a6def8c17859ffbb30906f91912268f872d Mon Sep 17 00:00:00 2001
From: Sirraide <aeternalmail at gmail.com>
Date: Fri, 8 Mar 2024 11:55:42 +0100
Subject: [PATCH 1/2] [Clang] Fix dependence of DREs in lambdas with an
explicit object parameter
---
clang/lib/Sema/SemaExpr.cpp | 44 +++++++++---
clang/lib/Sema/TreeTransform.h | 4 +-
clang/test/SemaCXX/cxx2b-deducing-this.cpp | 81 ++++++++++++++++++++++
3 files changed, 116 insertions(+), 13 deletions(-)
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index 47bb263f56aade..700769860aa806 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -20687,20 +20687,42 @@ void Sema::MarkVariableReferenced(SourceLocation Loc, VarDecl *Var) {
static void FixDependencyOfIdExpressionsInLambdaWithDependentObjectParameter(
Sema &SemaRef, ValueDecl *D, Expr *E) {
auto *ID = dyn_cast<DeclRefExpr>(E);
- if (!ID || ID->isTypeDependent())
+ if (!ID || ID->isTypeDependent() || !ID->refersToEnclosingVariableOrCapture())
return;
+ // If any enclosing lambda with a dependent explicit object parameter either
+ // explicitly captures the variable by value, or has a capture default of '='
+ // and does not capture the variable by reference, then the type of the DRE
+ // is dependent on the type of that lambda's explicit object parameter.
auto IsDependent = [&]() {
- const LambdaScopeInfo *LSI = SemaRef.getCurLambda();
- if (!LSI)
- return false;
- if (!LSI->ExplicitObjectParameter ||
- !LSI->ExplicitObjectParameter->getType()->isDependentType())
- return false;
- if (!LSI->CaptureMap.count(D))
- return false;
- const Capture &Cap = LSI->getCapture(D);
- return !Cap.isCopyCapture();
+ for (auto *Scope : llvm::reverse(SemaRef.FunctionScopes)) {
+ auto *LSI = dyn_cast<sema::LambdaScopeInfo>(Scope);
+ if (!LSI)
+ continue;
+
+ if (LSI->Lambda && !LSI->Lambda->Encloses(SemaRef.CurContext) &&
+ LSI->AfterParameterList)
+ return false;
+
+ const auto *MD = LSI->CallOperator;
+ if (MD->getType().isNull())
+ continue;
+
+ const auto *Ty = cast<FunctionProtoType>(MD->getType());
+ if (!Ty || !MD->isExplicitObjectMemberFunction() ||
+ !Ty->getParamType(0)->isDependentType())
+ continue;
+
+ if (auto *C = LSI->CaptureMap.count(D) ? &LSI->getCapture(D) : nullptr) {
+ if (C->isCopyCapture())
+ return true;
+ continue;
+ }
+
+ if (LSI->ImpCaptureStyle == LambdaScopeInfo::ImpCap_LambdaByval)
+ return true;
+ }
+ return false;
}();
ID->setCapturedByCopyInLambdaWithExplicitObjectParameter(
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index 7389a48fe56fcc..867efd1ce5c80a 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -11099,8 +11099,8 @@ TreeTransform<Derived>::TransformDeclRefExpr(DeclRefExpr *E) {
}
if (!getDerived().AlwaysRebuild() &&
- QualifierLoc == E->getQualifierLoc() &&
- ND == E->getDecl() &&
+ E->getDependence() == ExprDependence::None &&
+ QualifierLoc == E->getQualifierLoc() && ND == E->getDecl() &&
Found == E->getFoundDecl() &&
NameInfo.getName() == E->getDecl()->getDeclName() &&
!E->hasExplicitTemplateArgs()) {
diff --git a/clang/test/SemaCXX/cxx2b-deducing-this.cpp b/clang/test/SemaCXX/cxx2b-deducing-this.cpp
index b8ddb9ad300034..6c21954554d281 100644
--- a/clang/test/SemaCXX/cxx2b-deducing-this.cpp
+++ b/clang/test/SemaCXX/cxx2b-deducing-this.cpp
@@ -200,6 +200,87 @@ void TestMutationInLambda() {
[i = 0](this auto){ i++; }();
[i = 0](this const auto&){ i++; }();
// expected-error at -1 {{cannot assign to a variable captured by copy in a non-mutable lambda}}
+ // expected-note at -2 {{in instantiation of}}
+
+ int x;
+ const auto l1 = [x](this auto&) { x = 42; }; // expected-error {{cannot assign to a variable captured by copy in a non-mutable lambda}}
+ const auto l2 = [=](this auto&) { x = 42; }; // expected-error {{cannot assign to a variable captured by copy in a non-mutable lambda}}
+
+ const auto l3 = [&x](this auto&) {
+ const auto l3a = [x](this auto&) { x = 42; }; // expected-error {{cannot assign to a variable captured by copy in a non-mutable lambda}}
+ l3a(); // expected-note {{in instantiation of}}
+ };
+
+ const auto l4 = [&x](this auto&) {
+ const auto l4a = [=](this auto&) { x = 42; }; // expected-error {{cannot assign to a variable captured by copy in a non-mutable lambda}}
+ l4a(); // expected-note {{in instantiation of}}
+ };
+
+ const auto l5 = [x](this auto&) {
+ const auto l5a = [x](this auto&) { x = 42; }; // expected-error {{cannot assign to a variable captured by copy in a non-mutable lambda}}
+ l5a(); // expected-note {{in instantiation of}}
+ };
+
+ const auto l6 = [=](this auto&) {
+ const auto l6a = [=](this auto&) { x = 42; }; // expected-error {{cannot assign to a variable captured by copy in a non-mutable lambda}}
+ l6a(); // expected-note {{in instantiation of}}
+ };
+
+ const auto l7 = [x](this auto&) {
+ const auto l7a = [=](this auto&) { x = 42; }; // expected-error {{cannot assign to a variable captured by copy in a non-mutable lambda}}
+ l7a(); // expected-note {{in instantiation of}}
+ };
+
+ const auto l8 = [=](this auto&) {
+ const auto l8a = [x](this auto&) { x = 42; }; // expected-error {{cannot assign to a variable captured by copy in a non-mutable lambda}}
+ l8a(); // expected-note {{in instantiation of}}
+ };
+
+ const auto l9 = [&](this auto&) {
+ const auto l9a = [x](this auto&) { x = 42; }; // expected-error {{cannot assign to a variable captured by copy in a non-mutable lambda}}
+ l9a(); // expected-note {{in instantiation of}}
+ };
+
+ const auto l10 = [&](this auto&) {
+ const auto l10a = [=](this auto&) { x = 42; }; // expected-error {{cannot assign to a variable captured by copy in a non-mutable lambda}}
+ l10a(); // expected-note {{in instantiation of}}
+ };
+
+ const auto l11 = [x](this auto&) {
+ const auto l11a = [&x](this auto&) { x = 42; }; // expected-error {{cannot assign to a variable captured by copy in a non-mutable lambda}} expected-note {{while substituting}}
+ l11a();
+ };
+
+ const auto l12 = [x](this auto&) {
+ const auto l12a = [&](this auto&) { x = 42; }; // expected-error {{cannot assign to a variable captured by copy in a non-mutable lambda}} expected-note {{while substituting}}
+ l12a();
+ };
+
+ const auto l13 = [=](this auto&) {
+ const auto l13a = [&x](this auto&) { x = 42; }; // expected-error {{cannot assign to a variable captured by copy in a non-mutable lambda}} expected-note {{while substituting}}
+ l13a();
+ };
+
+ l1(); // expected-note {{in instantiation of}}
+ l2(); // expected-note {{in instantiation of}}
+ l3(); // expected-note {{in instantiation of}}
+ l4(); // expected-note {{in instantiation of}}
+ l5(); // expected-note {{in instantiation of}}
+ l6(); // expected-note {{in instantiation of}}
+ l7(); // expected-note {{in instantiation of}}
+ l8(); // expected-note {{in instantiation of}}
+ l9(); // expected-note {{in instantiation of}}
+ l10(); // expected-note {{in instantiation of}}
+ l11(); // expected-note {{in instantiation of}}
+ l12(); // expected-note {{in instantiation of}}
+ l13(); // expected-note {{in instantiation of}}
+
+ {
+ const auto l1 = [&x](this auto&) { x = 42; };
+ const auto l2 = [&](this auto&) { x = 42; };
+ l1();
+ l2();
+ }
}
struct Over_Call_Func_Example {
>From bf4c193f6e91d4dae4fa20c805b49955d95666cd Mon Sep 17 00:00:00 2001
From: Sirraide <aeternalmail at gmail.com>
Date: Fri, 8 Mar 2024 13:37:57 +0100
Subject: [PATCH 2/2] [Clang] Better dependence check for DREs in TreeTransform
---
clang/lib/Sema/TreeTransform.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index 867efd1ce5c80a..e537dfef767df8 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -11099,7 +11099,7 @@ TreeTransform<Derived>::TransformDeclRefExpr(DeclRefExpr *E) {
}
if (!getDerived().AlwaysRebuild() &&
- E->getDependence() == ExprDependence::None &&
+ !E->isCapturedByCopyInLambdaWithExplicitObjectParameter() &&
QualifierLoc == E->getQualifierLoc() && ND == E->getDecl() &&
Found == E->getFoundDecl() &&
NameInfo.getName() == E->getDecl()->getDeclName() &&
More information about the cfe-commits
mailing list