[Sema] Defer checking constexpr lambda until after we've finished the lambda class.

[erik.pilkington]

Summary

Previously, this caused ExprConstant to assert while verifying the lambda is constexpr:

void f() {
    int x = 0;
    [=]() constexpr {
        return x;
    };
}

The problem is that ActOnFinishFunctionBody evaluated the lambda's body before BuildLambdaExpr finished the lambda class. This patch fixes the problem by moving that check to BuildLambdaExpr.

PR36054

Thanks for taking a look!
Erik

[faisalv]

Thanks for working on this fairly embarrassing bug (let's fix this before the week is over :)

clang/lib/Sema/SemaDecl.cpp

12886 ↗

(On Diff #140722)

Hmm - as opposed to further leaking our lambda's implementation-details into ActOnFinishFunctionBody - and duplicating these checks for a lambda marked constexpr, I think I would prefer teaching the expression-evaluator (ExprConstant.cpp) to know how to check a lambda's function call operator for constexprness (i.e. avoid creating the capture-to-field-map when 'checkingPotentialConstantExpression()' and ignore evaluating expressions that refer to enclosing variables since their evaluation should not affect inferring constexprness).

Thoughts?

[erik.pilkington]

Handle this in ExprConstant instead of in Sema.

clang/lib/Sema/SemaDecl.cpp

12886 ↗

(On Diff #140722)

That's a really good point, makes this a lot simpler. Thanks!

[faisalv]

Thanks Erik!

clang/lib/AST/ExprConstant.cpp

4312 ↗

(On Diff #141001)

How about we add a comment here along the lines of: Do not attempt to create the variable-reference to closure data member map while 'constexpr' checking a lambda's function call operator (standard reference). Currently constpexpr checking is done right after the end of the function definition for the syntehsized call operator marked explicitly constexpr - which occurs prior to adding the captures map to the closure object. Alternatively we could have conditioned the check at the end of the function body to bypass lambda call operators and then invoke the constexpr check once the lambda is completely processed.

Between you and me, I'm a little torn about this approach - if you can make an argument to consider your approach over this one - i think i could be swayed (if i'm not already ;) - unless of course richard weighs in as a tie breaker.

5205 ↗

(On Diff #141001)

I think you might want to add a check here to determine if E refersToEnclosingVariableOrCapture()?

[erik.pilkington]

Add refersToEnclosingVariableOrCaptureCheck(), comments.

clang/lib/AST/ExprConstant.cpp

4312 ↗

(On Diff #141001)

Sure, added a version of that comment in the patch. I could go either way here too, but I like your approach because a) it makes the constant evaluator do just the work that is necessary when checkingPotentialConstantExpression(), and b) special-casing here is simpler and more precise than doing it in Sema.

[faisalv]

LGTM - can you commit?
Thank you!

clang/lib/AST/ExprConstant.cpp

4313 ↗

(On Diff #141042)

should we say something about only explicitly specified constexpr checking (as opposed to constexpr inference) is done prior to capture information being stored in the CXXRecordDecl? *shrug*.

Also the upside of having the expression evaluator evaluate these expressions is we could diagnose such lambdas as ill-formed (though no-diagnostic required per the standard):

void f() {
  volatile int v = 1;
  [&] () constexpr { return v; };  // can never be a core constant expression, right?
}

I think I agree w you though - so, for now lets stick with this version of the patch?

5212 ↗

(On Diff #141042)

How about something along the lines of : We don't always have a complete capture-map when checking or inferring if the function call operator meets the requirements of a constexpr function - and we don't need to evaluate the captures to determine constexprness (dcl.constexpr C++17)?

[erik.pilkington]

LGTM - can you commit?

Yep, I'll do that now. Thanks for reviewing!

clang/lib/AST/ExprConstant.cpp

5212 ↗

(On Diff #141042)

Sure, I just copied that verbatim into the commit.

cfe/trunk/lib/AST/ExprConstant.cpp

if (!handleLValueToRValueConversion(Info, Args[0], Args[0]->getType(),
                                         RHS, RHSValue))
       return false;
     if (!handleAssignment(Info, Args[0], *This, MD->getThisType(Info.Ctx),
                           RHSValue))
       return false;
     This->moveInto(Result);
     return true;
   } else if (MD && isLambdaCallOperator(MD)) {
-    // We're in a lambda; determine the lambda capture field maps.
+    // We're in a lambda; determine the lambda capture field maps unless we're
+    // just constexpr checking a lambda's call operator. constexpr checking is
+    // done before the captures have been added to the closure object (unless
+    // we're inferring constexpr-ness), so we don't have access to them in this
+    // case. But since we don't need the captures to constexpr check, we can
+    // just ignore them.
+    if (!Info.checkingPotentialConstantExpression())
       MD->getParent()->getCaptureFields(Frame.LambdaCaptureFields,
                                         Frame.LambdaThisCaptureField);
   }
 
   StmtResult Ret = {Result, ResultSlot};
   EvalStmtResult ESR = EvaluateStmt(Ret, Info, Body);
   if (ESR == ESR_Succeeded) {
     if (Callee->getReturnType()->isVoidType())
       return true;
     Info.FFDiag(Callee->getLocEnd(), diag::note_constexpr_no_return);

 
 
 bool LValueExprEvaluator::VisitVarDecl(const Expr *E, const VarDecl *VD) {
 
   // If we are within a lambda's call operator, check whether the 'VD' referred
   // to within 'E' actually represents a lambda-capture that maps to a
   // data-member/field within the closure object, and if so, evaluate to the
   // field or what the field refers to.
-  if (Info.CurrentCall && isLambdaCallOperator(Info.CurrentCall->Callee)) {
-    if (auto *FD = Info.CurrentCall->LambdaCaptureFields.lookup(VD)) {
-      if (Info.checkingPotentialConstantExpression())
-        return false;
+  if (Info.CurrentCall && isLambdaCallOperator(Info.CurrentCall->Callee) &&
+      isa<DeclRefExpr>(E) &&
+      cast<DeclRefExpr>(E)->refersToEnclosingVariableOrCapture()) {
+    // We don't always have a complete capture-map when checking or inferring if
+    // the function call operator meets the requirements of a constexpr function
+    // - but we don't need to evaluate the captures to determine constexprness
+    // (dcl.constexpr C++17).
+
+    if (auto *FD = Info.CurrentCall->LambdaCaptureFields.lookup(VD)) {
       // Start with 'Result' referring to the complete closure object...
       Result = *Info.CurrentCall->This;
       // ... then update it to refer to the field of the closure object
       // that represents the capture.
       if (!HandleLValueMember(Info, E, Result, FD))
         return false;
       // And if the field is of reference type, update 'Result' to refer to what
       // the field refers to.

cfe/trunk/test/SemaCXX/cxx1z-constexpr-lambdas.cpp

namespace ns1_test_lvalue_type {
   void f() {
     volatile int n;
     constexpr bool B = [&]{ return &n; }() == &n; // should be accepted
   }
 } // end ns1_unimplemented 
 
 } // end ns test_lambda_is_cce
 
+namespace PR36054 {
+constexpr int fn() {
+  int Capture = 42;
+  return [=]() constexpr { return Capture; }();
+}
+
+static_assert(fn() == 42, "");
+
+template <class T>
+constexpr int tfn() {
+  int Capture = 42;
+  return [=]() constexpr { return Capture; }();
+}
+
+static_assert(tfn<int>() == 42, "");
+
+constexpr int gfn() {
+  int Capture = 42;
+  return [=](auto P) constexpr { return Capture + P; }(58);
+}
+
+static_assert(gfn() == 100, "");
+
+constexpr bool OtherCaptures() {
+  int Capture = 42;
+  constexpr auto Outer = [](auto P) constexpr { return 42 + P; };
+  auto Inner = [&](auto O) constexpr { return O(58) + Capture; };
+  return Inner(Outer) == 142;
+}
+
+static_assert(OtherCaptures(), "");
+} // namespace PR36054
+
 #endif // ndef CPP14_AND_EARLIER