[Clang] [Sema] Fix dependence of DREs in lambdas with an explicit object parameter (llvm#84473)

Sirraide · erikolofsson · commit 5b78d711d0e5 · 2024-08-07T16:56:40.000+02:00
This fixes some problems wrt dependence of captures in lambdas with an explicit object parameter. [temp.dep.expr] states that > An id-expression is type-dependent if [...] its terminal name is > - associated by name lookup with an entity captured by copy > ([expr.prim.lambda.capture]) in a lambda-expression that has > an explicit object parameter whose type is dependent [dcl.fct]. There were several issues with our implementation of this: 1. we were treating by-reference captures as dependent rather than by-value captures; 2. tree transform wasn't checking whether referring to such a by-value capture should make a DRE dependent; 3. when checking whether a DRE refers to such a by-value capture, we were only looking at the immediately enclosing lambda, and not at any parent lambdas; 4. we also forgot to check for implicit by-value captures; 5. lastly, we were attempting to determine whether a lambda has an explicit object parameter by checking the `LambdaScopeInfo`'s `ExplicitObjectParameter`, but it seems that that simply wasn't set (yet) by the time we got to the check. All of these should be fixed now. This fixes llvm#70604, llvm#79754, llvm#84163, llvm#84425, llvm#86054, llvm#86398, and llvm#86399.
diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
@@ -1119,6 +1119,10 @@ Bug Fixes to C++ Support
   incorrect constraint substitution.
   (`#86769 <https://github.com/llvm/llvm-project/issues/86769>`_)
 
+- Clang now correctly tracks type dependence of by-value captures in lambdas with an explicit
+  object parameter.
+  Fixes (#GH70604), (#GH79754), (#GH84163), (#GH84425), (#GH86054), (#GH86398), and (#GH86399).
+
 Bug Fixes to AST Handling
 ^^^^^^^^^^^^^^^^^^^^^^^^^
 - Fixed an import failure of recursive friend class template.
diff --git a/clang/include/clang/AST/ExprCXX.h b/clang/include/clang/AST/ExprCXX.h
@@ -1149,6 +1149,7 @@ class CXXThisExpr : public Expr {
   CXXThisExpr(SourceLocation L, QualType Ty, bool IsImplicit, ExprValueKind VK)
       : Expr(CXXThisExprClass, Ty, VK, OK_Ordinary) {
     CXXThisExprBits.IsImplicit = IsImplicit;
+    CXXThisExprBits.CapturedByCopyInLambdaWithExplicitObjectParameter = false;
     CXXThisExprBits.Loc = L;
     setDependence(computeDependence(this));
   }
@@ -1170,6 +1171,15 @@ class CXXThisExpr : public Expr {
   bool isImplicit() const { return CXXThisExprBits.IsImplicit; }
   void setImplicit(bool I) { CXXThisExprBits.IsImplicit = I; }
 
+  bool isCapturedByCopyInLambdaWithExplicitObjectParameter() const {
+    return CXXThisExprBits.CapturedByCopyInLambdaWithExplicitObjectParameter;
+  }
+
+  void setCapturedByCopyInLambdaWithExplicitObjectParameter(bool Set) {
+    CXXThisExprBits.CapturedByCopyInLambdaWithExplicitObjectParameter = Set;
+    setDependence(computeDependence(this));
+  }
+
   static bool classof(const Stmt *T) {
     return T->getStmtClass() == CXXThisExprClass;
   }
diff --git a/clang/include/clang/AST/Stmt.h b/clang/include/clang/AST/Stmt.h
@@ -782,6 +782,11 @@ class alignas(void *) Stmt {
     LLVM_PREFERRED_TYPE(bool)
     unsigned IsImplicit : 1;
 
+    /// Whether there is a lambda with an explicit object parameter that
+    /// captures this "this" by copy.
+    LLVM_PREFERRED_TYPE(bool)
+    unsigned CapturedByCopyInLambdaWithExplicitObjectParameter : 1;
+
     /// The location of the "this".
     SourceLocation Loc;
   };
diff --git a/clang/lib/AST/ComputeDependence.cpp b/clang/lib/AST/ComputeDependence.cpp
@@ -310,6 +310,16 @@ ExprDependence clang::computeDependence(CXXThisExpr *E) {
   // 'this' is type-dependent if the class type of the enclosing
   // member function is dependent (C++ [temp.dep.expr]p2)
   auto D = toExprDependenceForImpliedType(E->getType()->getDependence());
+
+  // If a lambda with an explicit object parameter captures '*this', then
+  // 'this' now refers to the captured copy of lambda, and if the lambda
+  // is type-dependent, so is the object and thus 'this'.
+  //
+  // Note: The standard does not mention this case explicitly, but we need
+  // to do this so we can mark NSDM accesses as dependent.
+  if (E->isCapturedByCopyInLambdaWithExplicitObjectParameter())
+    D |= ExprDependence::Type;
+
   assert(!(D & ExprDependence::UnexpandedPack));
   return D;
 }
diff --git a/clang/lib/AST/StmtProfile.cpp b/clang/lib/AST/StmtProfile.cpp
@@ -2009,6 +2009,7 @@ void StmtProfiler::VisitMSPropertySubscriptExpr(
 void StmtProfiler::VisitCXXThisExpr(const CXXThisExpr *S) {
   VisitExpr(S);
   ID.AddBoolean(S->isImplicit());
+  ID.AddBoolean(S->isCapturedByCopyInLambdaWithExplicitObjectParameter());
 }
 
 void StmtProfiler::VisitCXXThrowExpr(const CXXThrowExpr *S) {
diff --git a/clang/lib/AST/TextNodeDumper.cpp b/clang/lib/AST/TextNodeDumper.cpp
@@ -1158,8 +1158,11 @@ void TextNodeDumper::VisitDeclRefExpr(const DeclRefExpr *Node) {
   case NOUR_Constant: OS << " non_odr_use_constant"; break;
   case NOUR_Discarded: OS << " non_odr_use_discarded"; break;
   }
-  if (Node->refersToEnclosingVariableOrCapture())
+  if (Node->isCapturedByCopyInLambdaWithExplicitObjectParameter())
+    OS << " dependent_capture";
+  else if (Node->refersToEnclosingVariableOrCapture())
     OS << " refers_to_enclosing_variable_or_capture";
+
   if (Node->isImmediateEscalating())
     OS << " immediate-escalating";
 }
@@ -1315,6 +1318,8 @@ void TextNodeDumper::VisitCXXBoolLiteralExpr(const CXXBoolLiteralExpr *Node) {
 void TextNodeDumper::VisitCXXThisExpr(const CXXThisExpr *Node) {
   if (Node->isImplicit())
     OS << " implicit";
+  if (Node->isCapturedByCopyInLambdaWithExplicitObjectParameter())
+    OS << " dependent_capture";
   OS << " this";
 }
 
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
@@ -20668,20 +20668,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/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
@@ -1440,6 +1440,42 @@ Expr *Sema::BuildCXXThisExpr(SourceLocation Loc, QualType Type,
 
 void Sema::MarkThisReferenced(CXXThisExpr *This) {
   CheckCXXThisCapture(This->getExprLoc());
+  if (This->isTypeDependent())
+    return;
+
+  // Check if 'this' is captured by value in a lambda with a dependent explicit
+  // object parameter, and mark it as type-dependent as well if so.
+  auto IsDependent = [&]() {
+    for (auto *Scope : llvm::reverse(FunctionScopes)) {
+      auto *LSI = dyn_cast<sema::LambdaScopeInfo>(Scope);
+      if (!LSI)
+        continue;
+
+      if (LSI->Lambda && !LSI->Lambda->Encloses(CurContext) &&
+          LSI->AfterParameterList)
+        return false;
+
+      // If this lambda captures 'this' by value, then 'this' is dependent iff
+      // this lambda has a dependent explicit object parameter. If we can't
+      // determine whether it does (e.g. because the CXXMethodDecl's type is
+      // null), assume it doesn't.
+      if (LSI->isCXXThisCaptured()) {
+        if (!LSI->getCXXThisCapture().isCopyCapture())
+          continue;
+
+        const auto *MD = LSI->CallOperator;
+        if (MD->getType().isNull())
+          return false;
+
+        const auto *Ty = cast<FunctionProtoType>(MD->getType());
+        return Ty && MD->isExplicitObjectMemberFunction() &&
+               Ty->getParamType(0)->isDependentType();
+      }
+    }
+    return false;
+  }();
+
+  This->setCapturedByCopyInLambdaWithExplicitObjectParameter(IsDependent);
 }
 
 bool Sema::isThisOutsideMemberFunctionBody(QualType BaseType) {
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
@@ -10928,8 +10928,8 @@ TreeTransform<Derived>::TransformDeclRefExpr(DeclRefExpr *E) {
   }
 
   if (!getDerived().AlwaysRebuild() &&
-      QualifierLoc == E->getQualifierLoc() &&
-      ND == E->getDecl() &&
+      !E->isCapturedByCopyInLambdaWithExplicitObjectParameter() &&
+      QualifierLoc == E->getQualifierLoc() && ND == E->getDecl() &&
       Found == E->getFoundDecl() &&
       NameInfo.getName() == E->getDecl()->getDeclName() &&
       !E->hasExplicitTemplateArgs()) {
@@ -12392,9 +12392,17 @@ TreeTransform<Derived>::TransformCXXThisExpr(CXXThisExpr *E) {
   //
   // In other contexts, the type of `this` may be overrided
   // for type deduction, so we need to recompute it.
-  QualType T = getSema().getCurLambda() ?
-                   getDerived().TransformType(E->getType())
-                 : getSema().getCurrentThisType();
+  //
+  // Always recompute the type if we're in the body of a lambda, and
+  // 'this' is dependent on a lambda's explicit object parameter.
+  QualType T = [&]() {
+    auto &S = getSema();
+    if (E->isCapturedByCopyInLambdaWithExplicitObjectParameter())
+      return S.getCurrentThisType();
+    if (S.getCurLambda())
+      return getDerived().TransformType(E->getType());
+    return S.getCurrentThisType();
+  }();
 
   if (!getDerived().AlwaysRebuild() && T == E->getType()) {
     // Mark it referenced in the new context regardless.
diff --git a/clang/lib/Serialization/ASTReaderStmt.cpp b/clang/lib/Serialization/ASTReaderStmt.cpp
@@ -1849,6 +1849,7 @@ void ASTStmtReader::VisitCXXThisExpr(CXXThisExpr *E) {
   VisitExpr(E);
   E->setLocation(readSourceLocation());
   E->setImplicit(Record.readInt());
+  E->setCapturedByCopyInLambdaWithExplicitObjectParameter(Record.readInt());
 }
 
 void ASTStmtReader::VisitCXXThrowExpr(CXXThrowExpr *E) {
diff --git a/clang/lib/Serialization/ASTWriterStmt.cpp b/clang/lib/Serialization/ASTWriterStmt.cpp
@@ -1842,6 +1842,7 @@ void ASTStmtWriter::VisitCXXThisExpr(CXXThisExpr *E) {
   VisitExpr(E);
   Record.AddSourceLocation(E->getLocation());
   Record.push_back(E->isImplicit());
+  Record.push_back(E->isCapturedByCopyInLambdaWithExplicitObjectParameter());
 
   Code = serialization::EXPR_CXX_THIS;
 }
diff --git a/clang/test/CodeGenCXX/cxx2b-deducing-this.cpp b/clang/test/CodeGenCXX/cxx2b-deducing-this.cpp
@@ -109,3 +109,76 @@ void test_temporary() {
 //CHECK:    %ref.tmp = alloca %struct.MaterializedTemporary, align 1
 //CHECK:    call void @_ZN21MaterializedTemporaryC1Ev(ptr noundef nonnull align 1 dereferenceable(1) %ref.tmp){{.*}}
 //CHECK     invoke void @_ZNH21MaterializedTemporary3fooEOS_(ptr noundef nonnull align 1 dereferenceable(1) %ref.tmp){{.*}}
+
+namespace GH86399 {
+volatile int a = 0;
+struct function {
+  function& operator=(function const&) {
+    a = 1;
+    return *this;
+  }
+};
+
+void f() {
+  function list;
+
+  //CHECK-LABEL: define internal void @"_ZZN7GH863991f{{.*}}"(ptr %{{.*}})
+  //CHECK: call {{.*}} @_ZN7GH863998functionaSERKS0_
+  //CHECK-NEXT: ret void
+  [&list](this auto self) {
+    list = function{};
+  }();
+}
+}
+
+namespace GH84163 {
+// Just check that this doesn't crash (we were previously not instantiating
+// everything that needs instantiating in here).
+template <typename> struct S {};
+
+void a() {
+  int x;
+  const auto l = [&x](this auto&) { S<decltype(x)> q; };
+  l();
+}
+}
+
+namespace GH84425 {
+// As above.
+void do_thing(int x) {
+    auto second = [&](this auto const& self, int b) -> int {
+        if (x) return x;
+        else return self(x);
+    };
+
+     second(1);
+}
+
+void do_thing2(int x) {
+    auto second = [&](this auto const& self)  {
+        if (true) return x;
+        else return x;
+    };
+
+     second();
+}
+}
+
+namespace GH79754 {
+// As above.
+void f() {
+  int x;
+  [&x](this auto&&) {return x;}();
+}
+}
+
+namespace GH70604 {
+auto dothing(int num)
+{
+  auto fun =  [&num](this auto&& self) -> void {
+    auto copy = num;
+  };
+
+  fun();
+}
+}
diff --git a/clang/test/PCH/cxx23-deducing-this-lambda.cpp b/clang/test/PCH/cxx23-deducing-this-lambda.cpp
@@ -0,0 +1,35 @@
+// RUN: %clang_cc1 -emit-pch -std=c++23 -o %t %s
+// RUN: %clang_cc1 -include-pch %t -verify -fsyntax-only -DTEST -std=c++23 %s
+
+// Test that dependence of 'this' and DREs due to by-value capture by a
+// lambda with an explicit object parameter is serialised/deserialised
+// properly.
+
+#ifndef HEADER
+#define HEADER
+struct S {
+  int x;
+  auto f() {
+    return [*this] (this auto&&) {
+      int y;
+      x = 42;
+
+      const auto l = [y] (this auto&&) { y = 42; };
+      l();
+    };
+  }
+};
+#endif
+
+// expected-error@* {{read-only variable is not assignable}}
+// expected-error@* {{cannot assign to a variable captured by copy in a non-mutable lambda}}
+// expected-note@* 2 {{in instantiation of}}
+
+#ifdef TEST
+void f() {
+  const auto l = S{}.f();
+  l(); // expected-note {{in instantiation of}}
+}
+#endif
+
+
diff --git a/clang/test/SemaCXX/cxx2b-deducing-this.cpp b/clang/test/SemaCXX/cxx2b-deducing-this.cpp

Original file line number	Diff line number	Diff line change
`@@ -2009,6 +2009,7 @@ void StmtProfiler::VisitMSPropertySubscriptExpr(`
`2009`	`2009`	`void StmtProfiler::VisitCXXThisExpr(const CXXThisExpr *S) {`
`2010`	`2010`	`VisitExpr(S);`
`2011`	`2011`	`ID.AddBoolean(S->isImplicit());`
	`2012`	`+ ID.AddBoolean(S->isCapturedByCopyInLambdaWithExplicitObjectParameter());`
`2012`	`2013`	`}`
`2013`	`2014`
`2014`	`2015`	`void StmtProfiler::VisitCXXThrowExpr(const CXXThrowExpr *S) {`
Original file line number	Diff line number	Diff line change
`@@ -1849,6 +1849,7 @@ void ASTStmtReader::VisitCXXThisExpr(CXXThisExpr *E) {`
`1849`	`1849`	`VisitExpr(E);`
`1850`	`1850`	`E->setLocation(readSourceLocation());`
`1851`	`1851`	`E->setImplicit(Record.readInt());`
	`1852`	`+ E->setCapturedByCopyInLambdaWithExplicitObjectParameter(Record.readInt());`
`1852`	`1853`	`}`
`1853`	`1854`
`1854`	`1855`	`void ASTStmtReader::VisitCXXThrowExpr(CXXThrowExpr *E) {`