[Clang] Fix nesting of discarded and immediate contexts.

[cor3ntin]

Summary

In C++23, discarded statements and if consteval statements can nest
arbitrarily. To support that, we
keep track of whether the parent of the current evaluation context
is discarded or immediate.

This is done at the construction of an evaluation context
to improve performance.

Fixes https://bugs.llvm.org/show_bug.cgi?id=52231

[cor3ntin]

Fix commit message

[aaron.ballman]

clang/include/clang/Sema/Sema.h

1300–1301

No real benefit to making these bit-fields, and it'd be good to add some documentation about why the fields exist.

clang/lib/Sema/SemaExpr.cpp

16571–16573

16575–16578

It took me a moment to understand why -2 was happening here. However, isn't this going to be UB when we go to push the first expr evaluation context?

[cor3ntin]

Add and fix comments

[cor3ntin]

Remove bitfields

[cor3ntin]

clang/lib/Sema/SemaExpr.cpp

16575–16578

No, there is always one root context created during initialization of sema. Because this pattern is used in a few places, I did not add a comment.

[aaron.ballman]

LGTM! Thank you for the fix!

clang/lib/Sema/SemaExpr.cpp

16575–16578

Ah, good to know, thank you!

[aaron.ballman]

I've commit on your behalf in 6eeda06c1d22da2b9fe96a2569a8a0f8e4f36880, thanks!

[hollinsky]

This change breaks return value deduction of template instantiations that first occur within a discarded context.

template<int S>
class Foo {
public:
	constexpr Foo(const char str[S]) : buf(str[0]) {}
	char get() const { return buf; }
	char buf;
};

template<int S>
static constexpr auto make(const char(&str)[S]) {
	return Foo<S>{str};
}

int main() {
	if constexpr(false) {
		return make("used to be Foo<29>, now void").get();
	}
	return 0;
}

This code now fails to compile with

regression.cpp:16:46: error: member reference base type 'void' is not a structure or union
                return make("used to be Foo<29>, now void").get();
                       ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~

The only return statement in the make() function template is not being considered anymore, making the resulting instantiation

template<>
static constexpr void make(const char(&str)[29]) {
	return Foo<29>{str};
}

Intuitively, I wouldn't think that the "discarded return statements are not considered when deducing a return type" rule should continue into new function scopes, though I'm not sure exactly how the spec backs that up.

[aaron.ballman]

In D113749#3172722, @hollinsky wrote:

This change breaks return value deduction of template instantiations that first occur within a discarded context.

Thank you for letting us know! I think we've fixed this issue in https://reviews.llvm.org/D115228 which was landed in 2334314550724812bb94e36c6b5df7d665bdbd9d. Can you let us know if that does not address your issue?

clang/include/clang/Sema/Sema.h

struct ExpressionEvaluationContextRecord {
     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
     } ExprContext;
 
+    // A context can be nested in both a discarded statement context and
+    // an immediate function context, so they need to be tracked independently.
+    bool InDiscardedStatement : 1;
+    bool InImmediateFunctionContext : 1;
+
     ExpressionEvaluationContextRecord(ExpressionEvaluationContext Context,
                                       unsigned NumCleanupObjects,
                                       CleanupInfo ParentCleanup,
                                       Decl *ManglingContextDecl,
                                       ExpressionKind ExprContext)
         : Context(Context), ParentCleanup(ParentCleanup),
           NumCleanupObjects(NumCleanupObjects), NumTypos(0),
-          ManglingContextDecl(ManglingContextDecl), ExprContext(ExprContext) {}
+          ManglingContextDecl(ManglingContextDecl), ExprContext(ExprContext),
+          InDiscardedStatement(false), InImmediateFunctionContext(false) {}
 
     bool isUnevaluated() const {
       return Context == ExpressionEvaluationContext::Unevaluated ||
              Context == ExpressionEvaluationContext::UnevaluatedAbstract ||
              Context == ExpressionEvaluationContext::UnevaluatedList;
     }
 
     bool isConstantEvaluated() const {
       return Context == ExpressionEvaluationContext::ConstantEvaluated ||
              Context == ExpressionEvaluationContext::ImmediateFunctionContext;
     }
 
     bool isImmediateFunctionContext() const {
-      return Context == ExpressionEvaluationContext::ImmediateFunctionContext;
+      return Context == ExpressionEvaluationContext::ImmediateFunctionContext ||
+             InImmediateFunctionContext;
+    }
+
+    bool isDiscardedStatementContext() const {
+      return Context == ExpressionEvaluationContext::DiscardedStatement ||
+             InDiscardedStatement;
     }
   };
 
   /// A stack of expression evaluation contexts.
   SmallVector<ExpressionEvaluationContextRecord, 8> ExprEvalContexts;
 
   /// Emit a warning for all pending noderef expressions that we recorded.
   void WarnOnPendingNoDerefs(ExpressionEvaluationContextRecord &Rec);
bool isUnevaluatedContext() const {
     assert(!ExprEvalContexts.empty() &&
            "Must be in an expression evaluation context");
     return ExprEvalContexts.back().isUnevaluated();
   }
 
   bool isImmediateFunctionContext() const {
     assert(!ExprEvalContexts.empty() &&
            "Must be in an expression evaluation context");
-    for (const ExpressionEvaluationContextRecord &context :
-         llvm::reverse(ExprEvalContexts)) {
-      if (context.isImmediateFunctionContext())
-        return true;
-      if (context.isUnevaluated())
-        return false;
-    }
-    return false;
+    return ExprEvalContexts.back().isImmediateFunctionContext();
   }
 
   /// RAII class used to determine whether SFINAE has
   /// trapped any errors that occur during template argument
   /// deduction.
   class SFINAETrap {
     Sema &SemaRef;
     unsigned PrevSFINAEErrors;

clang/lib/Sema/SemaExpr.cpp

 }
 
 void
 Sema::PushExpressionEvaluationContext(
     ExpressionEvaluationContext NewContext, Decl *LambdaContextDecl,
     ExpressionEvaluationContextRecord::ExpressionKind ExprContext) {
   ExprEvalContexts.emplace_back(NewContext, ExprCleanupObjects.size(), Cleanup,
                                 LambdaContextDecl, ExprContext);
+
+  // Discarded statements and immediate contexts nested in other
+  // discarded statements or immediate context are themselves
+  // a discarded statement or an immediate context, respectively.
+  ExprEvalContexts.back().InDiscardedStatement =
+      ExprEvalContexts[ExprEvalContexts.size() - 2]
+          .isDiscardedStatementContext();
+  ExprEvalContexts.back().InImmediateFunctionContext =
+      ExprEvalContexts[ExprEvalContexts.size() - 2]
+          .isImmediateFunctionContext();
+
   Cleanup.reset();
   if (!MaybeODRUseExprs.empty())
     std::swap(MaybeODRUseExprs, ExprEvalContexts.back().SavedMaybeODRUseExprs);
 }
 
 void
 Sema::PushExpressionEvaluationContext(
     ExpressionEvaluationContext NewContext, ReuseLambdaContextDecl_t,

 /// later.
 ///
 /// This routine should be used for all diagnostics that describe the run-time
 /// behavior of a program, such as passing a non-POD value through an ellipsis.
 /// Failure to do so will likely result in spurious diagnostics or failures
 /// during overload resolution or within sizeof/alignof/typeof/typeid.
 bool Sema::DiagRuntimeBehavior(SourceLocation Loc, ArrayRef<const Stmt*> Stmts,
                                const PartialDiagnostic &PD) {
+
+  if (ExprEvalContexts.back().isDiscardedStatementContext())
+    return false;
+
   switch (ExprEvalContexts.back().Context) {
   case ExpressionEvaluationContext::Unevaluated:
   case ExpressionEvaluationContext::UnevaluatedList:
   case ExpressionEvaluationContext::UnevaluatedAbstract:
   case ExpressionEvaluationContext::DiscardedStatement:
     // The argument will never be evaluated, so don't complain.
     break;
 

clang/lib/Sema/SemaStmt.cpp

StmtResult Sema::ActOnCapScopeReturnStmt(SourceLocation ReturnLoc,
   // If this is the first return we've seen, infer the return type.
   // [expr.prim.lambda]p4 in C++11; block literals follow the same rules.
   CapturingScopeInfo *CurCap = cast<CapturingScopeInfo>(getCurFunction());
   QualType FnRetType = CurCap->ReturnType;
   LambdaScopeInfo *CurLambda = dyn_cast<LambdaScopeInfo>(CurCap);
   bool HasDeducedReturnType =
       CurLambda && hasDeducedReturnType(CurLambda->CallOperator);
 
-  if (ExprEvalContexts.back().Context ==
-          ExpressionEvaluationContext::DiscardedStatement &&
+  if (ExprEvalContexts.back().isDiscardedStatementContext() &&
       (HasDeducedReturnType || CurCap->HasImplicitReturnType)) {
     if (RetValExp) {
       ExprResult ER =
           ActOnFinishFullExpr(RetValExp, ReturnLoc, /*DiscardedValue*/ false);
       if (ER.isInvalid())
         return StmtError();
       RetValExp = ER.get();
     }
Sema::ActOnReturnStmt(SourceLocation ReturnLoc, Expr *RetValExp,
                       Scope *CurScope) {
   // Correct typos, in case the containing function returns 'auto' and
   // RetValExp should determine the deduced type.
   ExprResult RetVal = CorrectDelayedTyposInExpr(
       RetValExp, nullptr, /*RecoverUncorrectedTypos=*/true);
   if (RetVal.isInvalid())
     return StmtError();
   StmtResult R = BuildReturnStmt(ReturnLoc, RetVal.get());
-  if (R.isInvalid() || ExprEvalContexts.back().Context ==
-                           ExpressionEvaluationContext::DiscardedStatement)
+  if (R.isInvalid() || ExprEvalContexts.back().isDiscardedStatementContext())
     return R;
 
   if (VarDecl *VD =
       const_cast<VarDecl*>(cast<ReturnStmt>(R.get())->getNRVOCandidate())) {
     CurScope->addNRVOCandidate(VD);
   } else {
     CurScope->setNoNRVO();
   }
if (MD->hasRelatedResultType() && MD->getClassInterface()) {
       RelatedRetType = Context.getObjCInterfaceType(MD->getClassInterface());
       RelatedRetType = Context.getObjCObjectPointerType(RelatedRetType);
     }
   } else // If we don't have a function/method context, bail.
     return StmtError();
 
   // C++1z: discarded return statements are not considered when deducing a
   // return type.
-  if (ExprEvalContexts.back().Context ==
-          ExpressionEvaluationContext::DiscardedStatement &&
+  if (ExprEvalContexts.back().isDiscardedStatementContext() &&
       FnRetType->getContainedAutoType()) {
     if (RetValExp) {
       ExprResult ER =
           ActOnFinishFullExpr(RetValExp, ReturnLoc, /*DiscardedValue*/ false);
       if (ER.isInvalid())
         return StmtError();
       RetValExp = ER.get();
     }

clang/test/SemaCXX/cxx2b-consteval-if.cpp

auto f() {
   if constexpr (false) {
     if consteval {
       // Immediate function context, so call to `make()` is valid.
       // Discarded statement context, so `return 0;` is valid too.
       delete make();
       return 0;
     }
   }
-  // FIXME: this error should not happen.
-  return 0.0; // expected-error {{'auto' in return type deduced as 'double' here but deduced as 'int' in earlier return statement}}
+  return 0.0;
 }