[Clang] [Draft] Implement P0588R1 capture rules

[LYP951018]

Code Changes:

• Currently, Clang requires instantiation to determine captures of a packs (refer to 7bf8f6f), but P0588 allows delayed instantiation of if constexpr statements in generic lambdas, which causes packs inside if constexpr to not be captured correctly. Therefore, I reverted to the previous approach of instantiating implicit captures in TransformLambdaExpr and handled the following two cases:

  auto v = [](auto... c) {
    sink([&](auto ...b) {
      c;          // case 1
      sink(c...); // case 2
    }...);
  };

• P0588R1 allows us to determine the set of implicit captures when parsing the lambda expression, so there is no need to call tryCaptureVariable(/*BuildAndDiagnose=*/true) during template instantiation. Therefore:

  • The previous concept of "capture-capable" has been removed;
  • Redundant tryCaptureVariable calls in potential captures have been removed, but tryCaptureVariable in MarkVarDeclODRUsed has not been removed yet.

Behavioral changes

https://godbolt.org/z/e3Wb6hPja

• When there are nested generic lambdas, and whether a variable inside the lambda is odr-used depends on the outer generic lambda (the impact of removing capture-capable):

    const int x = 10;
    auto L = [=](auto a) {
        return [=](auto b) {
          DEFINE_SELECTOR(a);
          F_CALL(x, a); 
          return 0;        
        }; 
    };

    auto L0 = L('c');
    static_assert(sizeof(L0) == sizeof(int));
    auto L1 = L(1);
    static_assert(sizeof(L1) == sizeof(int)); // sizeof(L1) is 1 before, now it is 4.

• P0588R1 allows us to capture variables referenced inside typeid:

    auto ltid = [=]{ typeid(x); };
    static_assert(sizeof(ltid) == sizeof(int)); // sizeof(ltid) is 1 before, now it is 4.

Remaining issues

• Is a separate ABI flag needed?
• How to determine if the current variable is within a typeid expression in DoMarkVarDeclReferenced?

I would appreciate any insights or suggestions on this. Thank you in advance for your help!

This PR is a prerequisite for implmenting #61426

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff 08acc3f73b64bed578d18812a04015cb537c9c82 afdff4e15c518984206da5b7cdbe9b14bb1b3bea --extensions cpp,h -- clang/include/clang/Sema/Sema.h clang/include/clang/Sema/SemaLambda.h clang/lib/Sema/SemaExpr.cpp clang/lib/Sema/SemaExprCXX.cpp clang/lib/Sema/SemaLambda.cpp clang/lib/Sema/SemaTemplateVariadic.cpp clang/lib/Sema/TreeTransform.h clang/test/SemaCXX/cxx1y-generic-lambdas-capturing.cpp clang/test/SemaTemplate/lambda-capture-pack.cpp

View the diff from clang-format here.

diff --git a/clang/include/clang/Sema/Sema.h b/clang/include/clang/Sema/Sema.h
index 77f2f7e41d..3176f1c9c4 100644
--- a/clang/include/clang/Sema/Sema.h
+++ b/clang/include/clang/Sema/Sema.h
@@ -14118,7 +14118,7 @@ public:
 
   /// Collect decls expanded inside the lambda body.
   /// e.g.
-  /// 
+  ///
   /// \code
   ///   auto v = [](auto... c) {
   ///    sink([&](auto ...b) {
diff --git a/clang/lib/Sema/SemaExpr.cpp b/clang/lib/Sema/SemaExpr.cpp
index e99422e654..8bdc621ed7 100644
--- a/clang/lib/Sema/SemaExpr.cpp
+++ b/clang/lib/Sema/SemaExpr.cpp
@@ -19631,14 +19631,15 @@ static void DoMarkVarDeclReferenced(
   case OdrUseContext::Dependent:
     // If this is a dependent context, we don't need to mark variables as
     // odr-used, but we may still need to track them for lambda capture.
-    // 
+    //
     // If an expression potentially references a local entity within a
     // declarative region in which it is odr-usable, and the expression would be
     // potentially evaluated if the effect of any enclosing typeid expressions
     // ([expr.typeid]) were ignored, the entity is said to be implicitly
     // captured by each intervening lambda-expression with an associated
     // capture-default that does not explicitly capture it.
-    // TODO: How to determine if the current variable is within a typeid expression?
+    // TODO: How to determine if the current variable is within a typeid
+    // expression?
 
     DoMarkPotentialCapture(SemaRef, Loc, Var, E);
     break;
diff --git a/clang/lib/Sema/SemaExprCXX.cpp b/clang/lib/Sema/SemaExprCXX.cpp
index fd1b6d9f2d..caea1fc5b3 100644
--- a/clang/lib/Sema/SemaExprCXX.cpp
+++ b/clang/lib/Sema/SemaExprCXX.cpp
@@ -8663,7 +8663,6 @@ static void CheckIfAnyEnclosingLambdasMustCaptureAnyPotentialCaptures(
     if (!UnderlyingVar)
       return;
 
-
     // If the variable is clearly identified as non-odr-used and the full
     // expression is not instantiation dependent, only then do we not
     // need to check enclosing lambda's for speculative captures.
@@ -8695,30 +8694,29 @@ static void CheckIfAnyEnclosingLambdasMustCaptureAnyPotentialCaptures(
       }
     }
 
-
-     const bool IsVarNeverAConstantExpression =
-          VariableCanNeverBeAConstantExpression(UnderlyingVar, S.Context);
-      if (!IsFullExprInstantiationDependent || IsVarNeverAConstantExpression) {
-        // This full expression is not instantiation dependent or the variable
-        // can not be used in a constant expression - which means
-        // this variable must be odr-used here, so diagnose a
-        // capture violation early, if the variable is un-captureable.
-        // This is purely for diagnosing errors early.  Otherwise, this
-        // error would get diagnosed when the lambda becomes capture ready.
-        QualType CaptureType, DeclRefType;
-        SourceLocation ExprLoc = VarExpr->getExprLoc();
-        if (S.tryCaptureVariable(Var, ExprLoc, S.TryCapture_Implicit,
-                                 /*EllipsisLoc*/ SourceLocation(),
-                                 /*BuildAndDiagnose*/ false, CaptureType,
-                                 DeclRefType, nullptr)) {
-          // We will never be able to capture this variable, and we need
-          // to be able to in any and all instantiations, so diagnose it.
-          S.tryCaptureVariable(Var, ExprLoc, S.TryCapture_Implicit,
+    const bool IsVarNeverAConstantExpression =
+        VariableCanNeverBeAConstantExpression(UnderlyingVar, S.Context);
+    if (!IsFullExprInstantiationDependent || IsVarNeverAConstantExpression) {
+      // This full expression is not instantiation dependent or the variable
+      // can not be used in a constant expression - which means
+      // this variable must be odr-used here, so diagnose a
+      // capture violation early, if the variable is un-captureable.
+      // This is purely for diagnosing errors early.  Otherwise, this
+      // error would get diagnosed when the lambda becomes capture ready.
+      QualType CaptureType, DeclRefType;
+      SourceLocation ExprLoc = VarExpr->getExprLoc();
+      if (S.tryCaptureVariable(Var, ExprLoc, S.TryCapture_Implicit,
                                /*EllipsisLoc*/ SourceLocation(),
-                               /*BuildAndDiagnose*/ true, CaptureType,
-                               DeclRefType, nullptr);
-        }
+                               /*BuildAndDiagnose*/ false, CaptureType,
+                               DeclRefType, nullptr)) {
+        // We will never be able to capture this variable, and we need
+        // to be able to in any and all instantiations, so diagnose it.
+        S.tryCaptureVariable(Var, ExprLoc, S.TryCapture_Implicit,
+                             /*EllipsisLoc*/ SourceLocation(),
+                             /*BuildAndDiagnose*/ true, CaptureType,
+                             DeclRefType, nullptr);
       }
+    }
   });
 
   // Check if 'this' needs to be captured.
diff --git a/clang/lib/Sema/SemaTemplateVariadic.cpp b/clang/lib/Sema/SemaTemplateVariadic.cpp
index c6d3411618..0d46967a03 100644
--- a/clang/lib/Sema/SemaTemplateVariadic.cpp
+++ b/clang/lib/Sema/SemaTemplateVariadic.cpp
@@ -284,12 +284,11 @@ namespace {
     }
   };
 
-
   class CollectExpandedParameterPacksVisitor
       : public RecursiveASTVisitor<CollectExpandedParameterPacksVisitor> {
     typedef RecursiveASTVisitor<CollectExpandedParameterPacksVisitor> inherited;
 
-    SmallVectorImpl<Decl*> &Expanded;
+    SmallVectorImpl<Decl *> &Expanded;
     bool UnderExpanded = false;
 
     struct UnderExpandedRAII {
@@ -302,8 +301,7 @@ namespace {
       ~UnderExpandedRAII() { UnderExpanded = Old; }
     };
 
-
-     bool InLambda = false;
+    bool InLambda = false;
     unsigned DepthLimit = (unsigned)-1;
 
     void addExpanded(Decl *VD, SourceLocation Loc = SourceLocation()) {
@@ -354,11 +352,11 @@ namespace {
       return true;
     }
 
-    bool TraversePackExpansionExpr(PackExpansionExpr *E) { 
+    bool TraversePackExpansionExpr(PackExpansionExpr *E) {
       UnderExpandedRAII UnderExpandedRAII{UnderExpanded};
       return inherited::TraversePackExpansionExpr(E);
     }
-    bool TraverseCXXFoldExpr(CXXFoldExpr *E) { 
+    bool TraverseCXXFoldExpr(CXXFoldExpr *E) {
       UnderExpandedRAII UnderExpandedRAII{UnderExpanded};
       return inherited::TraverseCXXFoldExpr(E);
     }
@@ -387,11 +385,9 @@ namespace {
     }
 
     /// we don't need to traverse into lambdas
-    bool TraverseLambdaExpr(LambdaExpr *Lambda) {
-      return true;
-    }
+    bool TraverseLambdaExpr(LambdaExpr *Lambda) { return true; }
   };
-  }
+  } // namespace
 
 /// Determine whether it's possible for an unexpanded parameter pack to
 /// be valid in this location. This only happens when we're in a declaration
@@ -689,7 +685,7 @@ void Sema::collectUnexpandedParameterPacksFromLambdaBody(
 }
 
 void Sema::collectExpandedParameterPacksFromLambdaBody(
-      Stmt *Body, SmallVectorImpl<Decl *> &Expanded) {
+    Stmt *Body, SmallVectorImpl<Decl *> &Expanded) {
   CollectExpandedParameterPacksVisitor visitor(Expanded);
   visitor.TraverseStmt(Body);
 }
diff --git a/clang/lib/Sema/TreeTransform.h b/clang/lib/Sema/TreeTransform.h
index 7d8861444f..378cc64524 100644
--- a/clang/lib/Sema/TreeTransform.h
+++ b/clang/lib/Sema/TreeTransform.h
@@ -14578,7 +14578,6 @@ TreeTransform<Derived>::TransformLambdaExpr(LambdaExpr *E) {
                                                         PacksExpandedInBody);
   }
 
-
   // Transform captures.
   bool FinishedExplicitCaptures = false;
   for (LambdaExpr::capture_iterator C = E->capture_begin(),
@@ -14672,17 +14671,14 @@ TreeTransform<Derived>::TransformLambdaExpr(LambdaExpr *E) {
       bool ShouldExpand = false;
       bool RetainExpansion = false;
       std::optional<unsigned> NumExpansions;
-      if (getDerived().TryExpandParameterPacks(IsImplicitCapturePack ? C->getLocation() : C->getEllipsisLoc(),
-                                               C->getLocation(),
-                                               Unexpanded,
-                                               ShouldExpand, RetainExpansion,
-                                               NumExpansions)) {
+      if (getDerived().TryExpandParameterPacks(
+              IsImplicitCapturePack ? C->getLocation() : C->getEllipsisLoc(),
+              C->getLocation(), Unexpanded, ShouldExpand, RetainExpansion,
+              NumExpansions)) {
         Invalid = true;
         continue;
       }
 
-
-
       if (ShouldExpand) {
         // The transform has determined that we should perform an expansion;
         // transform and capture each of the arguments.
@@ -14727,12 +14723,12 @@ TreeTransform<Derived>::TransformLambdaExpr(LambdaExpr *E) {
           }
         }
 
-
         // FIXME: Retain a pack expansion if RetainExpansion is true.
         continue;
       }
 
-      EllipsisLoc = IsImplicitCapturePack ? C->getLocation() : C->getEllipsisLoc();
+      EllipsisLoc =
+          IsImplicitCapturePack ? C->getLocation() : C->getEllipsisLoc();
     }
 
     // Transform the captured variable.
@@ -14755,7 +14751,7 @@ TreeTransform<Derived>::TransformLambdaExpr(LambdaExpr *E) {
 
   if (!FinishedExplicitCaptures)
     getSema().finishLambdaExplicitCaptures(LSI);
-  
+
   // Transform the template parameters, and add them to the current
   // instantiation scope. The null case is handled correctly.
   auto TPL = getDerived().TransformTemplateParameterList(

[erichkeane]

Is a separate ABI flag needed?

I think this ends up needing one, yes. There is the ClangABI flag stuff that should handle this.

How to determine if the current variable is within a typeid expression in DoMarkVarDeclReferenced?

This I don't have a good idea of. Perhaps checking the context of evaluation?

[erichkeane]

Rather than catching them like this... could we save the packs when parsing/instantiating? It would be preferential to store the parameter packs (perhaps as a potential list?) in the Lambda scope info objects as we parse, then again as we instantiate.

[zygoloid]

As a general concern, I think various parts of our pack handling would be cleaner if the parser tracked the set of packs used in a pack expandable region and annotated them on the pack expansion construct, rather than recursively walking the AST to reconstruct the list after the fact. (The latter is working, but it's fragile and we've had various bugs with the walk not quite finding everything for various reasons. It's also inefficient to walk the complete expanded region to search for the packs -- but given that we'll need to do that walk anyway when we actually expand, it's not asymptotically bad, except in the case where the packs are empty.)

[LYP951018]

Thanks for the review comments. Sorry for the delayed response, I'm a little busy recently. I'm still fixing a clang-tidy unit test broken by this change. I'll take a deep dive into your suggestions a bit later ;)

[cor3ntin]

@EricWF can you give that a look and see if it would help with your current work?