consteval if does not form a discarded statement

[aaron.ballman]

Summary

When we added support for if consteval, we accidentally formed a discarded statement evaluation context for the branch-not-taken. However, a discarded statement is a property of an if constexpr statement, not an if consteval statement (https://eel.is/c++draft/stmt.if#2.sentence-2). This turned out to cause issues when deducing the return type from a function with a consteval if statement -- we wouldn't consider the branch-not-taken when deducing the return type.

This fixes PR52206.

[cor3ntin]

Oups, Sorry I missed that in my earlier PR.
This fix LGTM.
Thanks Aaron

[rsmith]

clang/lib/Parse/ParseStmt.cpp

1445–1450

It looks to me like this is still incorrectly updating the context in some cases. We shouldn't enter an immediate function context if we're already in a discarded statement context. For example, Clang currently rejects this valid code:

auto f() {
    if constexpr (false) {
        if consteval {
            return 0;
        }
    }
    return 0.0;
}

... and it looks like it still will after this change. I think we should not enter a new context here if the existing context is a discarded statement context.

[rsmith]

clang/lib/Parse/ParseStmt.cpp

1445–1450

Hm, not updating the context also seems wrong; then we'd reject things like this:

consteval int *make() { return new int; }
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;
    }
  }
  return 0.0;
}

Perhaps we need to track whether we're in a discarded statement and whether we're in an immediate function context separately rather than treating them as mutually exclusive. I think it makes sense to go ahead with this patch as-is and deal with the bug that we overwrite a discarded statement context with an immediate function context separately.

[cor3ntin]

clang/lib/Parse/ParseStmt.cpp

1445–1450

Hum, I missed these scenarios entirely.
But I agree with you, not entering an immediate context would be equally bad.
I think the solution in both cases if to test whether any of the parent context is discarded or immediate, instead of just looking at the current context

[aaron.ballman]

I've commit in ab2ca8496d54573de1c8bec204009567ba2b4086 and filed https://bugs.llvm.org/show_bug.cgi?id=52231 as the bug for the remaining bits, thanks for the reviews and discussion!

clang/lib/Parse/ParseStmt.cpp

1445–1450

Perhaps we need to track whether we're in a discarded statement and whether we're in an immediate function context separately rather than treating them as mutually exclusive. I think it makes sense to go ahead with this patch as-is and deal with the bug that we overwrite a discarded statement context with an immediate function context separately.

I think you may be correct here. I've added your example to the test case with a FIXME comment so it's clear we've still got some work left to do here. Thanks!

clang/lib/Parse/ParseStmt.cpp

   MisleadingIndentationChecker MIChecker(*this, MSK_if, IfLoc);
 
   // Read the 'then' stmt.
   SourceLocation ThenStmtLoc = Tok.getLocation();
 
   SourceLocation InnerStatementTrailingElseLoc;
   StmtResult ThenStmt;
   {
-    bool ShouldEnter =
-        (ConstexprCondition && !*ConstexprCondition) || IsConsteval;
+    bool ShouldEnter = ConstexprCondition && !*ConstexprCondition;
     Sema::ExpressionEvaluationContext Context =
         Sema::ExpressionEvaluationContext::DiscardedStatement;
-    if (NotLocation.isInvalid() && IsConsteval)
+    if (NotLocation.isInvalid() && IsConsteval) {
       Context = Sema::ExpressionEvaluationContext::ImmediateFunctionContext;
+      ShouldEnter = true;
+    }
 
     EnterExpressionEvaluationContext PotentiallyDiscarded(
         Actions, Context, nullptr,
         Sema::ExpressionEvaluationContextRecord::EK_Other, ShouldEnter);
     ThenStmt = ParseStatement(&InnerStatementTrailingElseLoc);
   }
 
   if (Tok.isNot(tok::kw_else))

     // C++ 6.4p1:
     // The substatement in a selection-statement (each substatement, in the else
     // form of the if statement) implicitly defines a local scope.
     //
     ParseScope InnerScope(this, Scope::DeclScope, C99orCXX,
                           Tok.is(tok::l_brace));
 
     MisleadingIndentationChecker MIChecker(*this, MSK_else, ElseLoc);
-    bool ShouldEnter =
-        (ConstexprCondition && *ConstexprCondition) || IsConsteval;
+    bool ShouldEnter = ConstexprCondition && *ConstexprCondition;
     Sema::ExpressionEvaluationContext Context =
         Sema::ExpressionEvaluationContext::DiscardedStatement;
-    if (NotLocation.isValid() && IsConsteval)
+    if (NotLocation.isValid() && IsConsteval) {
       Context = Sema::ExpressionEvaluationContext::ImmediateFunctionContext;
+      ShouldEnter = true;
+    }
 
     EnterExpressionEvaluationContext PotentiallyDiscarded(
         Actions, Context, nullptr,
         Sema::ExpressionEvaluationContextRecord::EK_Other, ShouldEnter);
     ElseStmt = ParseStatement();
 
     if (ElseStmt.isUsable())
       MIChecker.Check();

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

+// RUN: %clang_cc1 -std=c++2b -verify %s
+
+namespace PR52206 {
+constexpr auto f() {
+  if consteval  { return 0;   }
+  if !consteval { return 0.0; } // expected-error {{'auto' in return type deduced as 'double' here but deduced as 'int' in earlier return statement}}
+}
+
+constexpr auto g() {
+  if !consteval { return 0;   }
+  if consteval  { return 0.0; } // expected-error {{'auto' in return type deduced as 'double' here but deduced as 'int' in earlier return statement}}
+}
+
+constexpr auto h() {
+  if consteval  { return 0; }
+  if !consteval { return 0; } // okay
+}
+
+constexpr auto i() {
+  if consteval {
+    if consteval { // expected-warning {{consteval if is always true in an immediate context}}
+    return 1;
+  }
+  return 2;
+  } else {
+    return 1.0; // expected-error {{'auto' in return type deduced as 'double' here but deduced as 'int' in earlier return statement}}
+  }
+}
+
+void test() {
+  auto x1 = f();
+  constexpr auto y1 = f();
+
+  auto x2 = g();
+  constexpr auto y2 = g();
+
+  auto x3 = h();
+  constexpr auto y3 = h();
+
+  auto x4 = i();
+  constexpr auto y4 = i();
+}
+} // namespace PR52206