[PATCH] D74130: [clang] fix consteval call in default arguments

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
Comment at: clang/lib/Sema/Sema.cpp:1026
 
+  HandleImmediateInvocations(ExprEvalContexts.back());
+
----------------
Tyker wrote:
> rsmith wrote:
> > What do we need this for? If I'm understanding the patch correctly, I think the only way we should propagate immediate invocations upwards should be from the `ExpressionEvaluationContext` of a lambda default argument into the `ExpressionEvaluationContext` of the lambda itself, so it should never reach the top level.
> This is fixing a separate bug that consteval call in to top-level ExprEvalContext are not handled.
> like this:
> ```
> struct S {
>   consteval S(bool b) {if (b) throw;}
> };
> S s2(true);
> ```
> this emits no error.
> because the immediate invocation are added to global scope and the global scope is never poped via PopExpressionEvaluationContext.
In this case, what's supposed to happen is that we create an `ExpressionEvaluationContext` for the initializer of `s2`. See `Sema::ActOnCXXEnterDeclInitializer` / `Sema::ActOnCXXExitDeclInitializer`. I guess that's not working somehow?

Ah, the problem is that we leave the context too early in `Parser::ParseDeclarationAfterDeclaratorAndAttributes`. We call `InitScope.pop()` before we add the initializer to the declaration, which is where we actually finish building the initialization full-expression -- that seems wrong in general. (We're also missing an `InitializerScopeRAII` object around the call to `ActOnUninitializedDecl` in the case where there is no initializer -- we need an `ExpressionEvaluationContext` for the default-initialization in that case too.)


================
Comment at: clang/lib/Sema/SemaExpr.cpp:16232
       SemaRef.Diag(Note.first, Note.second);
+    CE->markFailed();
     return;
----------------
Changing the dependence flags on an expression isn't safe to do this late: the `Error` dependence bit should be propagated to all transitive enclosing AST nodes when they're created, which has already happened at this point. If we need a way to represent an expression that's supposed to be a constant expression but isn't constant, perhaps adding a separate flag to `ConstantExpr` would be reasonable.


================
Comment at: clang/lib/Sema/SemaLambda.cpp:897-901
+    ExprEvalContexts.back().ReferenceToConsteval.insert(
+        LSI->ReferenceToConsteval.begin(), LSI->ReferenceToConsteval.end());
+    ExprEvalContexts.back().ImmediateInvocationCandidates.append(
+        LSI->ImmediateInvocationCandidates.begin(),
+        LSI->ImmediateInvocationCandidates.end());
----------------
Thanks! I think this can be cleaned up a little bit further:

* Move the handling of lambda parameter scopes from `HandleImmediateInvocations` to `PopExpressionEvaluationContext`
* Change `HandleImmediateInvocations` to take the `ReferenceToConsteval` and `ImmediateInvocationCandidates` data instead of an `ExpressionEvaluationContextRecord`
* Maybe consider wrapping `ReferenceToConsteval` and `ImmediateInvocationCandidates` in a struct to make it a bit easier to pass them around as a set.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74130/new/

https://reviews.llvm.org/D74130