[PATCH] D63960: [C++20] Add consteval-specifique semantic

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

rsmith added inline comments.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2317
+def err_invalid_consteval_take_address : Error<
+  "cannot take address of consteval function %0 in non-constexpr context">;
+def err_consteval_address_accessible : Error<
----------------
Let's use the proper terminology here so people can search for it: "outside of an immediate invocation"


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2318-2320
+def err_consteval_address_accessible : Error<
+  "%select{return value|constructed object}0 contain pointer on consteval declaration;"
+  " this would make it accessible at runtime">;
----------------
This diagnostic should be moved to DiagnosticASTKinds (and produced by CheckConstantExpression; see my other comment on this). Please rephrase this to follow the same pattern as `note_constexpr_non_global`.


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2324
+def err_invalid_consteval_decl_kind : Error<
+  "operator %select{new|delete|new[]|delete[]}0 cannot be consteval specified">;
 def err_invalid_constexpr : Error<
----------------
"consteval specified" -> "declared consteval"


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:3771-3772
     "candidate function made ineligible by enable_if">;
+def note_addrof_ovl_consteval : Note<
+    "candidate function made ineligible by consteval specifier">;
 def note_ovl_candidate_deduced_mismatch : Note<
----------------
`consteval` should not affect overload resolution. I don't think this diagnostic makes sense. (We should first pick the function and *then* check whether it's usable.)


================
Comment at: clang/include/clang/Sema/Sema.h:2152
 
+  ExprResult ActOnConstevalCall(FunctionDecl *FD, Expr *Call);
+
----------------
This should be called `CheckImmediateInvocation` or similar. `ActOn*` functions should only be called by the parser in response to source constructs, not internally by `Sema`.


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:2232
       !Expr::isPotentialConstantExpr(Dcl, Diags)) {
-    SemaRef.Diag(Dcl->getLocation(),
-                 diag::ext_constexpr_function_never_constant_expr)
-        << isa<CXXConstructorDecl>(Dcl);
+    SemaRef.Diag(Dcl->getLocation(), diag::ext_constexpr_function_never_constant_expr)
+        << isa<CXXConstructorDecl>(Dcl) << Dcl->isConsteval();
----------------
Please keep lines to 80 columns or fewer.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:5599-5604
+ExprResult Sema::ActOnConstevalCall(FunctionDecl *FD, Expr *Call) {
+  Expr::EvalResult Result;
+  llvm::SmallVector<PartialDiagnosticAt, 8> Diags;
+  Result.Diag = &Diags;
+  Call->EvaluateAsConstantExpr(Result, Expr::EvaluateForCodeGen, Context);
+  if (!Result.Diag->empty()) {
----------------
I don't think this approach can work as-is. You need to know the complete bounds of the immediate invocation before you can check it. For example:

```
enum E {};
consteval int operator+(int (*f)(), E) { return f(); }
consteval int fn() { return 42; }
consteval auto get() -> int(*)() { return fn; }

int k = get() + E();
```

This is (or at least should be) valid code: the immediate invocation here is the entire `get() + E()` expression, and evaluating that produces `42`. But this approach will reject the code, because `get()` in isolation is a constant expression. (The current wording suggests that this code is invalid, but I'm trying to get that fixed; I'm confident that was not the intent.)

I think we need a different approach. Perhaps:

 * when we see a candidate immediate invocation, immediately create a `ConstantExpr` wrapping it, but don't evaluate it yet
 * add that `ConstantExpr` to a list on the expression evaluation context record
 * if there already were any expressions listed there, check to see if any of them is a subexpression of the current expression, and if so, perform a `TreeTransform` to remove the `ConstantExpr` wrappers from those subexpressions and remove them from the list on the evaluation context record
 * when we pop an expression evaluation context record, *then* evaluate all remaining `ConstantExpr`s for immediate invocations and check they're valid

(Or some other approach that handles this wrinkle; the above is just a suggestion.)


================
Comment at: clang/lib/Sema/SemaExpr.cpp:5612-5616
+  if (CheckPointerOnConsteval(Result.Val)) {
+    Diag(Call->getBeginLoc(), diag::err_consteval_address_accessible)
+        << isa<CXXConstructorDecl>(FD);
+    return ExprError();
+  }
----------------
This should be checked when determining whether the core constant expression is a constant expression, which happens in `CheckConstantExpression` in `ExprConstant.cpp`, not here.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:5620
+
 ExprResult Sema::ActOnCallExpr(Scope *Scope, Expr *Fn, SourceLocation LParenLoc,
                                MultiExprArg ArgExprs, SourceLocation RParenLoc,
----------------
This is the wrong place to call `ActOnConstevalCall` from: that needs to happen when we actually build the resolved call expression (which isn't always done by this function).


================
Comment at: clang/lib/Sema/SemaExpr.cpp:5641-5642
+  if (CallExpr *CExpr = dyn_cast<CallExpr>(Call.get()))
+    if (auto *FD = dyn_cast_or_null<FunctionDecl>(CExpr->getCalleeDecl()))
+      if (FD && FD->isConsteval() && !isConstantEvaluated())
+        Call = ActOnConstevalCall(FD, CExpr);
----------------
`!isConstantEvaluated()` should be checked in the `CXXConstructExpr` case too; consider moving that check into `ActOnConstevalCall`.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:5761-5762
       // in ArgExprs.
-      if ((FDecl =
-               rewriteBuiltinFunctionDecl(this, Context, FDecl, ArgExprs))) {
+      if ((FDecl = rewriteBuiltinFunctionDecl(this, Context, FDecl, ArgExprs,
+                                              /*Diagnose*/ false))) {
         NDecl = FDecl;
----------------
What's the purpose of this change?


================
Comment at: clang/lib/Sema/SemaOverload.cpp:9596-9607
+  if (!S.isConstantEvaluated() && FD->isConsteval()) {
+    if (Complain) {
+      if (InOverloadResolution)
+        S.Diag(FD->getBeginLoc(), diag::note_addrof_ovl_consteval);
+      else {
+        S.Diag(Loc, diag::err_invalid_consteval_take_address) << FD;
+        S.Diag(FD->getBeginLoc(), diag::note_declared_at);
----------------
This isn't the right way to deal with this, in part because we don't know whether we're in an immediate invocation or not at this point. Instead, we should track that we saw a use of a `consteval` function from `Sema::DiagnoseUseOfDecl`, and issue a diagnostic if that use is not within an immediate invocation / within a consteval function.


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

https://reviews.llvm.org/D63960