[PATCH] D61790: [C++20] add consteval specifier

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

rsmith added a comment.

Thanks for working on this!

Comments on the general approach:

- We should only evaluate each immediate invocation once (this will become essential once we start supporting reflection -- and particularly operations that mutate the AST -- inside `consteval` functions).
- Each time an immediate invocation is formed, you should create a `ConstantExpr` AST node wrapping that call to reflect that it is a constant.
- You should change `ConstantExpr` to store an `APValue` representing the evaluated value of the expression.

Most of the above should be done as a separate preparatory change; we should be able to remove a lot of the existing ad-hoc caching of evaluated values (eg, for enumerators and the initializers of variables) at the same time.

Please also split this into smaller pieces. If you split off a patch to just add the keyword, parsing, and AST representation for the `consteval` specifier (but treat it identically to `constexpr` for constant evaluation purposes), that'd be a good first patch for this feature.



================
Comment at: clang/include/clang/AST/Decl.h:2102
   /// Whether this is a (C++11) constexpr function or constexpr constructor.
   bool isConstexpr() const { return FunctionDeclBits.IsConstexpr; }
   void setConstexpr(bool IC) { FunctionDeclBits.IsConstexpr = IC; }
----------------
Please rename this to `isConstexprSpecified()` (compare this to how we model `inline`, which has similar behavior: it can be explicit, or can be implied by other properties of the function).


================
Comment at: clang/include/clang/AST/Decl.h:2109
+
+  bool isConstexprOrConsteval() const { return isConstexpr() || isConsteval(); }
+
----------------
Both the `constexpr` and `consteval` specifiers make a function a "constexpr function", so I think this should be called simply `isConstexpr`; callers that want to distinguish `constexpr` from `consteval` can use `isConsteval()` (or we can add a `getConstexprKind()` or similar function).


================
Comment at: clang/include/clang/AST/DeclCXX.h:2115
+                QualType T, TypeSourceInfo *TInfo, StorageClass SC,
+                bool isInline, bool isConstexpr, bool isConsteval,
+                SourceLocation EndLocation)
----------------
Instead of the three bool flags in a row here (which will make call sites hard to read), consider using an enum, perhaps:

```
enum class ConstexprSpecifierKind { None, Constexpr, Consteval };
```


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2297-2300
+def err_consteval_cannot_be_constant_eval : Error<
+  "call to %0 cannot be constant evaluated">;
+def note_argument_n_cannot_be_constant_eval : Note<
+  "argument %0 cannot be constant evaluated">;
----------------
It would be useful in these diagnostics to explain why the call is required to be constant evaluated; please also use the standard terminology "constant expression" here. (Eg, "call to consteval function %0 is not a constant expression")


================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:2308-2314
 def err_constexpr_tag : Error<
   "%select{class|struct|interface|union|enum}0 cannot be marked constexpr">;
-def err_constexpr_dtor : Error<"destructor cannot be marked constexpr">;
+def err_constexpr_dtor : Error<"destructor cannot be marked %0">;
+def err_invalid_consteval_decl_kind : Error<
+  "operator %select{new|delete|new[]|delete[]}0 cannot be marked consteval">;
+def err_take_adress_of_consteval_decl : Error<
+  "taking address of a %0">;
----------------
In these diagnostics (including the existing one for tags), we should say "declared constexpr" not "marked constexpr".


================
Comment at: clang/include/clang/Basic/TokenKinds.def:390-391
 
+// C++ consteval proposal
+CXX2A_KEYWORD(consteval             , 0)
+
----------------
This and `char8_t` are both adopted C++2a features now (they'e not just proposals any more); please change the comment to just "C++2a keywords".


================
Comment at: clang/include/clang/Sema/DeclSpec.h:404
   SourceLocation FS_forceinlineLoc;
-  SourceLocation FriendLoc, ModulePrivateLoc, ConstexprLoc;
+  SourceLocation FriendLoc, ModulePrivateLoc, ConstexprLoc, ConstevalLoc;
   SourceLocation TQ_pipeLoc;
----------------
I think it would be preferable to track only one location here (for the `constexpr` / `consteval` specifier) and store a constexpr specifier kind instead of `Constexpr_specified`, like we do for the other kinds of specifier for which we allow only one of a set of keywords.


================
Comment at: clang/include/clang/Sema/Sema.h:985-986
+    ///   cases in a switch statement).
+    /// - "immediate function context" in C++2a terms, a call to a function
+    ///   marked as consteval
     ConstantEvaluated,
----------------
An "immediate function context" is also "potentially evaluated"; I think what we want to say here is something like "The current context is "potentially evaluated", but will only ever be constant evaluated; runtime code will never be generated for it. (Eg, the case values of a switch, or the body of a consteval function.)"


================
Comment at: clang/include/clang/Sema/Sema.h:4052-4053
 
+  /// CheckInvalidConstevalCall - detect and diagnose invalid calls to
+  /// consteval function.
+  /// return true if the call is invalid.
----------------
Please don't use the "FunctionName - " style in new code.


================
Comment at: clang/include/clang/Sema/Sema.h:4054
+  /// consteval function.
+  /// return true if the call is invalid.
+  bool CheckInvalidConstevalCall(FunctionDecl *FDecl, SourceLocation Loc,
----------------
return -> \return


================
Comment at: clang/include/clang/Sema/Sema.h:4055-4056
+  /// return true if the call is invalid.
+  bool CheckInvalidConstevalCall(FunctionDecl *FDecl, SourceLocation Loc,
+                                 ArrayRef<Expr *> Args, Expr *This = nullptr);
+
----------------
Drop the "Invalid" from this function name; it doesn't add anything. But I think this is the wrong interface anyway -- I'll get to that later.


================
Comment at: clang/lib/AST/ExprConstant.cpp:4474-4475
                                const LValue *ResultSlot) {
+  EvalInfo::EnterConstantContextRAII ContextRAII(
+      Info, Callee->isConsteval() || Info.InConstantContext);
   ArgVector ArgValues(Args.size());
----------------
We should not need to do this; instead, when the evaluator hits a `ConstantExpr` node, it should use the already-evaluated value of that node.


================
Comment at: clang/lib/Parse/ParseDecl.cpp:2480-2487
+      Diag(DS.getInlineSpecLoc(), diag::err_typename_invalid_specifier)
+          << "function";
     if (DS.isVirtualSpecified())
-      Diag(DS.getVirtualSpecLoc(), diag::err_typename_invalid_functionspec);
+      Diag(DS.getVirtualSpecLoc(), diag::err_typename_invalid_specifier)
+          << "function";
     if (DS.hasExplicitSpecifier())
+      Diag(DS.getExplicitSpecLoc(), diag::err_typename_invalid_specifier)
----------------
Do not pass random English words into diagnostics; this makes translation impossible. Use a `%select` or two different diagnostics instead. (Passing in C++ syntax such as `constexpr` or `consteval` is OK, since they don't need to be translated.)


================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:1073-1075
+    case tok::identifier:
+      if (P.getCurToken().getIdentifierInfo()->getName() != "consteval")
+        return;
----------------
It doesn't seem worthwhile to me to support `consteval` in earlier language modes only for lambdas; if we really want to allow `consteval` as an extension in earlier language modes, the better approach would be to add a `__consteval` keyword. But regardless of whether we do that, please remove this.


================
Comment at: clang/lib/Parse/ParseExprCXX.cpp:1303-1305
+    case tok::kw_consteval:
+      TokKind = 4;
+      break;
----------------
Please follow the formatting of the adjacent code.


================
Comment at: clang/lib/Sema/DeclSpec.cpp:1326-1332
+  // C++2a [dcl.spec]p2: The constexpr and consteval decl-specifiers shall not
+  // both appear in a decl-specifier-seq.
+  if (isConstevalSpecified() && isConstexprSpecified()) {
+    S.Diag(ConstexprLoc, diag::err_incompatible_constexpr_consteval)
+        << FixItHint::CreateRemoval(ConstexprLoc);
+    Constexpr_specified = false;
+  }
----------------
This should be handled by `SetConstevalSpec` instead; please look for calls to `BadSpecifier` to see how we handle this for other kinds of specifier.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:10471
 
+  bool IsConstantContext = S.ExprEvalContexts.back().isConstantEvaluated();
+
----------------
Are the changes to this file really specific to `consteval` evaluation? They look more general than that; I think this would fix the evaluation of `std::is_constant_evaluated()` in any constant-evaluated context that we analyzed. Can this be split out of this patch into a separate change?


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:640-648
   if (New->isConstexpr() != Old->isConstexpr()) {
     Diag(New->getLocation(), diag::err_constexpr_redecl_mismatch)
-      << New << New->isConstexpr();
+        << New << New->isConstexpr() << "constexpr";
+    Diag(Old->getLocation(), diag::note_previous_declaration);
+    Invalid = true;
+  } else if (New->isConsteval() != Old->isConsteval()) {
+    Diag(New->getLocation(), diag::err_constexpr_redecl_mismatch)
----------------
This will produce diagnostics such as "non-constexpr declaration follows constexpr declaration" for a `consteval` declaration following a `constexpr` declaration, which seems misleading since a `consteval` declaration does declare a `constexpr` function. I think the order of these two checks should be reversed, so that that case will instead be diagnosed as "consteval declaration follows non-consteval declaration".


================
Comment at: clang/lib/Sema/SemaDeclCXX.cpp:6655
       MD->getTemplatedKind() == FunctionDecl::TK_NonTemplate) {
     Diag(MD->getBeginLoc(), diag::err_incorrect_defaulted_constexpr) << CSM;
     // FIXME: Explain why the special member can't be constexpr.
----------------
The diagnostic text here is:

```
error: defaulted definition of <thing> is not constexpr
```

which might be confusing if the user wrote `consteval thing = default;`. Maybe change the diagnostic to:

"defaulted definition of <thing> cannot be declared %select{constexpr|consteval}1 because its implicit definition would not be constexpr"

or something like that?



================
Comment at: clang/lib/Sema/SemaExpr.cpp:5720-5769
+bool Sema::CheckInvalidConstevalCall(FunctionDecl *FDecl, SourceLocation Loc,
+                                     ArrayRef<Expr *> Args, Expr *This) {
+  bool Failed = false;
+  if (FDecl && FDecl->isConsteval() &&
+      !ExprEvalContexts.back().isConstantEvaluated()) {
+
+    SmallVector<PartialDiagnosticAt, 8> Diags;
----------------
This checking doesn't make sense: we should be checking that the invocation as a whole is a constant expression, not whether the arguments (evaluated in isolation) are constant expressions. This check should be applied to the `Expr` representing the call, and should wrap it in a `ConstantExpr` holding the evaluated value if evaluation succeeds.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:13417-13435
+bool Sema::CheckInvalidConstevalTakeAddress(Expr *Input) {
+  if (Input) {
+    if (auto *DeclRef = dyn_cast<DeclRefExpr>(Input->IgnoreParens())) {
+      if (auto *Function = dyn_cast<FunctionDecl>(DeclRef->getDecl())) {
+        if (Function->isConsteval()) {
+          const char *DeclName = Function->isOverloadedOperator()
+                                     ? "consteval operator"
----------------
This isn't correct: you can take the address of a `consteval` function within an immediate invocation. In general, we need to delay this checking until we've finished processing any potentially-enclosing immediate invocations. Trying to capture all of the places where we might form a reference to a `consteval` function name without forming an immediate invocation is also fragile: you'll miss some, and as Clang is extended, there'll be more cases introduced that you miss.

Here's how I was intending to handle this:

 * Change `Sema::MarkFunctionReferenced` to take an `Expr*` instead of a location, and update all of its callers. (We'll need a separate function for marking destructors referenced, because destructor calls typically don't have expressions, but destructors can't be `consteval` so that's OK.)
 * In `Sema::MarkFunctionReferenced`, if `Func` is a `consteval` function and our current context is not a `consteval` function, add it to a list on the `ExpressionEvaluationContext`.
 * When forming an immediate invocation, walk all of its subexpressions and remove them all from the list on the `ExpressionEvaluationContext`
 * When popping the `ExpressionEvaluationContext`, diagnose any remaining expressions in its list.

(The first step should be done in a separate patch to keep the diffs smaller and easier to review.)


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

https://reviews.llvm.org/D61790