[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

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

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.

clang/lib/Sema/SemaDecl.cpp

     if (!getDiagnostics().hasUncompilableErrorOccurred() &&
         !isa<FunctionTemplateDecl>(dcl)) {
       // Since the body is valid, issue any analysis-based warnings that are
       // enabled.
       ActivePolicy = &WP;
     }
 
     if (!IsInstantiation && FD && FD->isConstexpr() && !FD->isInvalidDecl() &&
+        !isLambdaCallOperator(FD) &&
         (!CheckConstexprFunctionDecl(FD) ||
          !CheckConstexprFunctionBody(FD, Body)))
       FD->setInvalidDecl();
 
     if (FD && FD->hasAttr<NakedAttr>()) {
       for (const Stmt *S : Body->children()) {
         // Allow local register variables without initializer as they don't
         // require prologue.

clang/lib/Sema/SemaLambda.cpp

   Cleanup.mergeFrom(LambdaCleanup);
 
   LambdaExpr *Lambda = LambdaExpr::Create(Context, Class, IntroducerRange, 
                                           CaptureDefault, CaptureDefaultLoc,
                                           Captures, 
                                           ExplicitParams, ExplicitResultType,
                                           CaptureInits, EndLoc,
                                           ContainsUnexpandedParameterPack);
-  // If the lambda expression's call operator is not explicitly marked constexpr
-  // and we are not in a dependent context, analyze the call operator to infer
-  // its constexpr-ness, suppressing diagnostics while doing so.
-  if (getLangOpts().CPlusPlus17 && !CallOperator->isInvalidDecl() &&
-      !CallOperator->isConstexpr() &&
-      !isa<CoroutineBodyStmt>(CallOperator->getBody()) &&
-      !Class->getDeclContext()->isDependentContext()) {
-    TentativeAnalysisScope DiagnosticScopeGuard(*this);
-    CallOperator->setConstexpr(
-        CheckConstexprFunctionDecl(CallOperator) &&
-        CheckConstexprFunctionBody(CallOperator, CallOperator->getBody()));
+
+  if (!CallOperator->isInvalidDecl()) {
+    // If the call operator is explicitly marked constexpr, verify that that is
+    // actually the case. This is done here instead of in
+    // ActOnFinishFunctionBody because the constexpr evaluator needs access to
+    // the completed lambda class to check this.
+    if (CallOperator->isConstexpr()) {
+      if (!CheckConstexprFunctionDecl(CallOperator) ||
+          !CheckConstexprFunctionBody(CallOperator, CallOperator->getBody()))
+        CallOperator->setInvalidDecl();
+    }
+    // Otherwise, if we're not in a dependent context, analyze the call operator
+    // to infer its constexpr-ness, suppressing diagnostics while doing so.
+    else if (getLangOpts().CPlusPlus17 &&
+             !isa<CoroutineBodyStmt>(CallOperator->getBody()) &&
+             !Class->getDeclContext()->isDependentContext()) {
+      TentativeAnalysisScope DiagnosticScopeGuard(*this);
+      CallOperator->setConstexpr(
+          CheckConstexprFunctionDecl(CallOperator) &&
+          CheckConstexprFunctionBody(CallOperator, CallOperator->getBody()));
+    }
   }
 
   // Emit delayed shadowing warnings now that the full capture list is known.
   DiagnoseShadowingLambdaDecls(LSI);
 
   if (!CurContext->isDependentContext()) {
     switch (ExprEvalContexts.back().Context) {
     // C++11 [expr.prim.lambda]p2:

clang/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