Apply ConstantEvaluated evaluation contexts to more manifestly constant evaluated scopes

[wchilders]

Summary

This patch attempts to update application of evaluation contexts to better represent, what is manifestly constant evaluated.

There are some outstanding concerns I'll mark with inline comments.

[wchilders]

clang/lib/Sema/SemaExpr.cpp

15597

This is my main concern with this patch. There's something subtle here, and it's unclear from an outsider's (my) perspective what the root issue is here.

Effectively we get into trouble when we first enter ActOnCXXEnterDeclInitializer on the isManifestlyEvaluatedVar branch. We then exit at ActOnCXXExitDeclInitializer, triggering this branch, as Rec.isConstantEvaluated() is true. This results in the cleanups being dropped. Then when we enter the constexpr evaluator for VarDecl::evaluateValue. the evaluator is unhappy as we're missing a cleanup for the created temporary.

Any input that allows this to be resolved without the special case, or that otherwise informs how this should work, and why unevaluated and constant evaluated are treated equally here, would be awesome. I'll note that in reviewing the revision history, this condition dates back to when ConstantEvaluated was created, so perhaps this is a "historical artifact" of that time?

[rsmith]

clang/lib/Sema/SemaDeclCXX.cpp

16793–16797

We can't implement the checks for manifestly constant-evaluated initializers this way in general. Per [expr.const]/14, we need to treat the initializer as manifestly constant-evaluated if it's "the initializer of a variable that is usable in constant expressions or has constant initialization". We can't test either of those conditions in general until we know what the initializer is, because they can both depend on whether evaluation of the initializer succeeds. (This approach works for the case of constexpr variables, which are always usable in constant expressions, but not any of the other cases, and the approach we'll need for the other cases will also handle constexpr variables. There is a very modest benefit to special-casing constexpr variable initializers regardless -- we can avoid forming and then pruning out nested ConstantExpr nodes for immediate invocations inside the initializer -- but I think it's probably not worth the added complexity.)

To address the general problem, we should handle this in CheckCompleteVariableDeclaration, which is where we evaluate the initializer and generally determine whether the variable has constant initialization and / or is usable in constant expressions. Probably the cleanest approach -- and certainly the one I'd been intending to pursue -- would be to wrap the initializer with a ConstantExpr there in the relevant cases, and allow the usual handling of nested immediate invocations to prune out any ConstantExprs nested within the initializer representing inner calls to consteval functions. (I think I've mentioned elsewhere that I would like to remove the "evaluated value" storage on VarDecl in favor of using ConstantExpr for this purpose.)

[wchilders]

clang/lib/Sema/SemaDeclCXX.cpp

16793–16797

There is a very modest benefit to special-casing constexpr variable initializers regardless -- we can avoid forming and then pruning out nested ConstantExpr nodes for immediate invocations inside the initializer -- but I think it's probably not worth the added complexity.

So, this patch is motivated for us by the desire to check if a "meta type" variable, belongs to a compile time, or runtime. Additionally, this is used being used to verify our compile time, "injection statement", is not appearing in a runtime context. This prevents reflections/metaprogramming values and logic from leaking nonsensically into runtime code.

I think this can be accomplished as you said in the CheckCompleteVariableDeclaration. We're already checking for out of place "meta type" variables there, though we catch fragments, and injection statements more eagerly. In retrospect, I don't think there is any issue removing the eager check from the fragments, and I don't think the checking of the injection statements should be affected by this case.

I'll try and look more into this in the morning.

[wchilders]

Updated the patch to correct formatting issues, and removed application of the ConstantEvaluated evaluation context to var decls as it introduces too much complexity.

Additionally removed a fix for decltype with immediate functions that snuck into this patch. This will be submitted separately.

clang/include/clang/Sema/Sema.h

struct ExpressionEvaluationContextRecord {
 
     /// Set of DeclRefExprs referencing a consteval function when used in a
     /// context not already known to be immediately invoked.
     llvm::SmallPtrSet<DeclRefExpr *, 4> ReferenceToConsteval;
 
     /// \brief Describes whether we are in an expression constext which we have
     /// to handle differently.
     enum ExpressionKind {
-      EK_Decltype, EK_TemplateArgument, EK_Other
+      EK_Decltype, EK_TemplateArgument, EK_ConstexprVarInit, EK_Other
     } ExprContext;
 
     ExpressionEvaluationContextRecord(ExpressionEvaluationContext Context,
                                       unsigned NumCleanupObjects,
                                       CleanupInfo ParentCleanup,
                                       Decl *ManglingContextDecl,
                                       ExpressionKind ExprContext)
         : Context(Context), ParentCleanup(ParentCleanup),

clang/lib/Parse/ParseDeclCXX.cpp

 ///   defaulted/deleted function-definition:
 ///     '=' 'default'
 ///     '=' 'delete'
 ///
 /// Prior to C++0x, the assignment-expression in an initializer-clause must
 /// be a constant-expression.
 ExprResult Parser::ParseCXXMemberInitializer(Decl *D, bool IsFunction,
                                              SourceLocation &EqualLoc) {
+  using EEC = Sema::ExpressionEvaluationContext;
+
   assert(Tok.isOneOf(tok::equal, tok::l_brace)
          && "Data member initializer not starting with '=' or '{'");
 
+  bool IsConstexpr = false;
+  if (auto *VD = dyn_cast_or_null<VarDecl>(D))
+    IsConstexpr = VD->isConstexpr();
+
   EnterExpressionEvaluationContext Context(
-      Actions, Sema::ExpressionEvaluationContext::PotentiallyEvaluated, D);
+      Actions, getLangOpts().CPlusPlus2a && IsConstexpr ?
+      EEC::ConstantEvaluated : EEC::PotentiallyEvaluated, D);
   if (TryConsumeToken(tok::equal, EqualLoc)) {
     if (Tok.is(tok::kw_delete)) {
       // In principle, an initializer of '= delete p;' is legal, but it will
       // never type-check. It's better to diagnose it as an ill-formed expression
       // than as an ill-formed deleted non-function member.
       // An initializer of '= delete p, foo' will never be parsed, because
       // a top-level comma always ends the initializer expression.
       const Token &Next = NextToken();
       if (IsFunction || Next.isOneOf(tok::semi, tok::comma, tok::eof)) {
         if (IsFunction)
           Diag(ConsumeToken(), diag::err_default_delete_in_multiple_declaration)
             << 1 /* delete */;
         else
           Diag(ConsumeToken(), diag::err_deleted_non_function);
         return ExprError();
       }
     } else if (Tok.is(tok::kw_default)) {
       if (IsFunction)
         Diag(Tok, diag::err_default_delete_in_multiple_declaration)

clang/lib/Parse/ParseStmt.cpp

StmtResult Parser::ParseIfStatement(SourceLocation *TrailingElseLoc) {
   // while, for, and switch statements are local to the if, while, for, or
   // switch statement (including the controlled statement).
   //
   ParseScope IfScope(this, Scope::DeclScope | Scope::ControlScope, C99orCXX);
 
   // Parse the condition.
   StmtResult InitStmt;
   Sema::ConditionResult Cond;
+
+  {
+    EnterExpressionEvaluationContext Unevaluated(
+        Actions, Sema::ExpressionEvaluationContext::ConstantEvaluated,
+        /*LambdaContextDecl=*/nullptr, /*ExprContext=*/
+        Sema::ExpressionEvaluationContextRecord::EK_Other,
+        /*ShouldEnter=*/IsConstexpr);
+
     if (ParseParenExprOrCondition(&InitStmt, Cond, IfLoc,
                                   IsConstexpr ? Sema::ConditionKind::ConstexprIf
                                               : Sema::ConditionKind::Boolean))
       return StmtError();
+  }
 
   llvm::Optional<bool> ConstexprCondition;
   if (IsConstexpr)
     ConstexprCondition = Cond.getKnownValue();
 
   // C99 6.8.4p3 - In C99, the body of the if statement is a scope, even if
   // there is no compound stmt.  C90 does not have this clause.  We only do this
   // if the body isn't a compound statement to avoid push/pop in common cases.

clang/lib/Sema/SemaDeclCXX.cpp

void Sema::ActOnPureSpecifier(Decl *D, SourceLocation ZeroLoc) {
   if (D->getFriendObjectKind())
     Diag(D->getLocation(), diag::err_pure_friend);
   else if (auto *M = dyn_cast<CXXMethodDecl>(D))
     CheckPureMethod(M, ZeroLoc);
   else
     Diag(D->getLocation(), diag::err_illegal_initializer);
 }
 
+/// Determine whether the given declaration is a variable
+/// which will be manifestly constant evaluated due to
+/// a constexpr specifier.
+static bool isManifestlyEvaluatedVar(Sema &SemaRef, const Decl *D) {
+  if (!SemaRef.getLangOpts().CPlusPlus2a)
+    return false;
+
+  if (const VarDecl *Var = dyn_cast_or_null<VarDecl>(D))
+    return Var->isConstexpr();
+
+  return false;
+}
+
 /// Determine whether the given declaration is a global variable or
 /// static data member.
 static bool isNonlocalVariable(const Decl *D) {
   if (const VarDecl *Var = dyn_cast_or_null<VarDecl>(D))
     return Var->hasGlobalStorage();
 
   return false;
 }
void Sema::ActOnCXXEnterDeclInitializer(Scope *S, Decl *D) {
   //   extern int n;
   //   int ::n = 0;
   if (S && D->isOutOfLine())
     EnterDeclaratorContext(S, D->getDeclContext());
 
   // If we are parsing the initializer for a static data member, push a
   // new expression evaluation context that is associated with this static
   // data member.
-  if (isNonlocalVariable(D))
+  if (isManifestlyEvaluatedVar(*this, D)) {
+    using ExpressionKind = ExpressionEvaluationContextRecord::ExpressionKind;
+
+    PushExpressionEvaluationContext(
+        ExpressionEvaluationContext::ConstantEvaluated, D, ExpressionKind::EK_ConstexprVarInit);
+  } else if (isNonlocalVariable(D))
     PushExpressionEvaluationContext(
         ExpressionEvaluationContext::PotentiallyEvaluated, D);
 }
 
 /// Invoked after we are finished parsing an initializer for the declaration D.
 void Sema::ActOnCXXExitDeclInitializer(Scope *S, Decl *D) {
   // If there is no declaration, there was an error parsing it.
   if (!D || D->isInvalidDecl())
     return;
 
-  if (isNonlocalVariable(D))
+  if (isManifestlyEvaluatedVar(*this, D) || isNonlocalVariable(D))
     PopExpressionEvaluationContext();
 
   if (S && D->isOutOfLine())
     ExitDeclaratorContext(S);
 }
 
 /// ActOnCXXConditionDeclarationExpr - Parsed a condition declaration of a
 /// C++ if/switch/while/for statement.

clang/lib/Sema/SemaExpr.cpp

if (auto *BO = dyn_cast<BinaryOperator>(E->IgnoreParenImpCasts())) {
     if (BO->getOpcode() == BO_Assign) {
       auto &LHSs = ExprEvalContexts.back().VolatileAssignmentLHSs;
       LHSs.erase(std::remove(LHSs.begin(), LHSs.end(), BO->getLHS()),
                  LHSs.end());
     }
   }
 }
 
+// Check to see if this expression is part of a decltype specifier.
+// We're not interested in evaluating this expression, immediately or otherwise.
+static bool isInDeclType(Sema &SemaRef) {
+  return SemaRef.ExprEvalContexts.back().ExprContext ==
+    Sema::ExpressionEvaluationContextRecord::EK_Decltype;
+}
+
 ExprResult Sema::CheckForImmediateInvocation(ExprResult E, FunctionDecl *Decl) {
   if (!E.isUsable() || !Decl || !Decl->isConsteval() || isConstantEvaluated() ||
-      RebuildingImmediateInvocation)
+      isInDeclType(*this) || RebuildingImmediateInvocation)
     return E;
 
   /// Opportunistically remove the callee from ReferencesToConsteval if we can.
   /// It's OK if this fails; we'll also remove this in
   /// HandleImmediateInvocations, but catching it here allows us to avoid
   /// walking the AST looking for it in simple cases.
   if (auto *Call = dyn_cast<CallExpr>(E.get()->IgnoreImplicit()))
     if (auto *DeclRef =
for (auto DR : Rec.ReferenceToConsteval) {
     auto *FD = cast<FunctionDecl>(DR->getDecl());
     SemaRef.Diag(DR->getBeginLoc(), diag::err_invalid_consteval_take_address)
         << FD;
     SemaRef.Diag(FD->getLocation(), diag::note_declared_at);
   }
 }
 
 void Sema::PopExpressionEvaluationContext() {
+  using ExpressionKind = ExpressionEvaluationContextRecord::ExpressionKind;
+
   ExpressionEvaluationContextRecord& Rec = ExprEvalContexts.back();
   unsigned NumTypos = Rec.NumTypos;
 
   if (!Rec.Lambdas.empty()) {
-    using ExpressionKind = ExpressionEvaluationContextRecord::ExpressionKind;
     if (Rec.ExprContext == ExpressionKind::EK_TemplateArgument || Rec.isUnevaluated() ||
         (Rec.isConstantEvaluated() && !getLangOpts().CPlusPlus17)) {
       unsigned D;
       if (Rec.isUnevaluated()) {
         // C++11 [expr.prim.lambda]p2:
         //   A lambda-expression shall not appear in an unevaluated operand
         //   (Clause 5).
         D = diag::err_lambda_unevaluated_operand;
void Sema::PopExpressionEvaluationContext() {
   for (auto *BO : Rec.VolatileAssignmentLHSs)
     Diag(BO->getBeginLoc(), diag::warn_deprecated_simple_assign_volatile)
         << BO->getType();
 
   // When are coming out of an unevaluated context, clear out any
   // temporaries that we may have created as part of the evaluation of
   // the expression in that context: they aren't relevant because they
   // will never be constructed.
-  if (Rec.isUnevaluated() || Rec.isConstantEvaluated()) {
+  //
+  // FIXME: The condition Rec.ExprContext != EK_ConstexprVarInit
+  // has been added as a stop gap fix. After adding a ConstEvaluated
+  // context to constexpr variable initializers, an issue manifests here
+  // where the cleanups are dropped prior to VarDecl::evaluateValue, which
+  // subsequently requires them. This has been done as a temporary workaround
+  // as it was not obvious how to produce.
+  if (Rec.isUnevaluated() ||
+      (Rec.isConstantEvaluated() &&
+       Rec.ExprContext != ExpressionKind::EK_ConstexprVarInit)) {
     ExprCleanupObjects.erase(ExprCleanupObjects.begin() + Rec.NumCleanupObjects,
                              ExprCleanupObjects.end());
     Cleanup = Rec.ParentCleanup;
     CleanupVarDeclMarking();
     std::swap(MaybeODRUseExprs, Rec.SavedMaybeODRUseExprs);
   // Otherwise, merge the contexts together.
   } else {
     Cleanup.mergeFrom(Rec.ParentCleanup);

clang/lib/Sema/SemaTemplateInstantiateDecl.cpp

void Sema::InstantiateFunctionDefinition(SourceLocation PointOfInstantiation,
   // The instantiation is visible here, even if it was first declared in an
   // unimported module.
   Function->setVisibleDespiteOwningModule();
 
   // Copy the inner loc start from the pattern.
   Function->setInnerLocStart(PatternDecl->getInnerLocStart());
 
   EnterExpressionEvaluationContext EvalContext(
-      *this, Sema::ExpressionEvaluationContext::PotentiallyEvaluated);
+      *this,
+      PatternDecl->getConstexprKind() == CSK_consteval
+          ? Sema::ExpressionEvaluationContext::ConstantEvaluated
+          : Sema::ExpressionEvaluationContext::PotentiallyEvaluated);
 
   // Introduce a new scope where local variable instantiations will be
   // recorded, unless we're actually a member function within a local
   // class, in which case we need to merge our results with the parent
   // scope (of the enclosing function).
   bool MergeWithParentScope = false;
   if (CXXRecordDecl *Rec = dyn_cast<CXXRecordDecl>(Function->getDeclContext()))
     MergeWithParentScope = Rec->isLocalClass();
void Sema::InstantiateVariableInitializer(
   // would otherwise imply that the variable is a definition for a
   // non-static data member.
   if (OldVar->isInlineSpecified())
     Var->setInlineSpecified();
   else if (OldVar->isInline())
     Var->setImplicitlyInline();
 
   if (OldVar->getInit()) {
+    bool IsConstexpr = OldVar->isConstexpr() || isConstantEvaluated();
     EnterExpressionEvaluationContext Evaluated(
-        *this, Sema::ExpressionEvaluationContext::PotentiallyEvaluated, Var);
+        *this, getLangOpts().CPlusPlus2a && IsConstexpr ?
+        Sema::ExpressionEvaluationContext::ConstantEvaluated :
+        Sema::ExpressionEvaluationContext::PotentiallyEvaluated, Var);
 
     // Instantiate the initializer.
     ExprResult Init;
 
     {
       ContextRAII SwitchContext(*this, Var->getDeclContext());
       Init = SubstInitializer(OldVar->getInit(), TemplateArgs,
                               OldVar->getInitStyle() == VarDecl::CallInit);

clang/lib/Sema/TreeTransform.h

for (LambdaExpr::capture_iterator C = E->capture_begin(),
     getSema().tryCaptureVariable(CapturedVar, C->getLocation(), Kind,
                                  EllipsisLoc);
   }
   getSema().finishLambdaExplicitCaptures(LSI);
 
   // FIXME: Sema's lambda-building mechanism expects us to push an expression
   // evaluation context even if we're not transforming the function body.
   getSema().PushExpressionEvaluationContext(
+      E->getCallOperator()->getConstexprKind() == CSK_consteval ?
+      Sema::ExpressionEvaluationContext::ConstantEvaluated :
       Sema::ExpressionEvaluationContext::PotentiallyEvaluated);
 
   // Instantiate the body of the lambda expression.
   StmtResult Body =
       Invalid ? StmtError() : getDerived().TransformLambdaBody(E, E->getBody());
 
   // ActOnLambda* will pop the function scope for us.
   FuncScopeCleanup.disable();
 
   if (Body.isInvalid()) {
     SavedContext.pop();
     getSema().ActOnLambdaError(E->getBeginLoc(), /*CurScope=*/nullptr,
                                /*IsInstantiation=*/true);
     return ExprError();
   }
 
   // Copy the LSI before ActOnFinishFunctionBody removes it.
   // FIXME: This is dumb. Store the lambda information somewhere that outlives
   // the call operator.
   auto LSICopy = *LSI;
   getSema().ActOnFinishFunctionBody(NewCallOperator, Body.get(),
                                     /*IsInstantiation*/ true);
   SavedContext.pop();