[PATCH] D84637: [AST] Enhance the const expression evaluator to support error-dependent exprs.

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

rsmith added a comment.

There are a couple of cases where you're returning `EvalStmtResult` from a function with a `bool` return type, that I'd like to see fixed before this lands.

All the other comments are directed towards producing more precise behavior when evaluating a function containing errors / value-dependent constructs. I don't think there's any need to block fixing the crasher here on improving the diagnostics, so I'm happy if you ignore these and commit as-is (other than fixing the return type issue), but I think we'll want to look at these diagnostic improvements at some point.



================
Comment at: clang/lib/AST/ExprConstant.cpp:4585
 
 static bool EvaluateVarDecl(EvalInfo &Info, const VarDecl *VD) {
   // We don't need to evaluate the initializer for a static local.
----------------
I think it would be slightly better if the dependent cases in this function returned `noteSideEffect()` instead of `false`.


================
Comment at: clang/lib/AST/ExprConstant.cpp:4613-4625
 static bool EvaluateDecl(EvalInfo &Info, const Decl *D) {
   bool OK = true;
 
   if (const VarDecl *VD = dyn_cast<VarDecl>(D))
     OK &= EvaluateVarDecl(Info, VD);
 
   if (const DecompositionDecl *DD = dyn_cast<DecompositionDecl>(D))
----------------
This function should bail out as soon as it sees an error. Otherwise, I think
```
constexpr void f() { int a = error(), b = a; }
```
... will to produce a hard error when evaluating `b`, because its initializer appears to read from an uninitialized variable.


================
Comment at: clang/lib/AST/ExprConstant.cpp:4627
 
+static EvalStmtResult EvaluateDependentExpr(const Expr *E, EvalInfo &Info) {
+  assert(E->isValueDependent());
----------------
Given the number of places below that seem to need to branch on the return value rather than returning it directly, and the use from contexts other than statement evaluation, I think it might be better for this function to return `bool`.


================
Comment at: clang/lib/AST/ExprConstant.cpp:4867
+        if (Inc->isValueDependent())
+          return EvaluateDependentExpr(Inc, Info);
         FullExpressionRAII IncScope(Info);
----------------
If this returns `ESR_Succeeded`, we should keep going rather than returning that value. Otherwise we'll do the wrong thing for cases like:

```
constexpr bool f() {
  for (int n = 0; ; ++n, error()) { if (n == 1) return true; }
  throw "bad";
}
```

... because evaluation of the `for` loop (but not the enclosing function) will terminate at the value-dependent increment expression, so we'll think that all evaluations of this function unconditionally throw.


================
Comment at: clang/lib/AST/ExprConstant.cpp:5026
+      if (DS->getCond()->isValueDependent())
+        return EvaluateDependentExpr(DS->getCond(), Info);
       FullExpressionRAII CondScope(Info);
----------------
If the condition is value-dependent, I think we should return `ESR_Failed` here: we don't know whether to keep going or terminate the loop, and either answer will be wrong in some cases.


================
Comment at: clang/lib/AST/ExprConstant.cpp:5064
+        if (Inc->isValueDependent())
+          return EvaluateDependentExpr(Inc, Info);
         FullExpressionRAII IncScope(Info);
----------------
As above, I think we need to keep going on `ESR_Succeeded` here.


================
Comment at: clang/lib/AST/ExprConstant.cpp:5116
+        if (FS->getCond()->isValueDependent())
+          return EvaluateDependentExpr(FS->getCond(), Info);
         bool Continue = true;
----------------
We should return `ESR_Failed` unconditionally here, because we don't know which statement to execute next.


================
Comment at: clang/lib/AST/ExprConstant.cpp:5142
+      if (FS->getInc()->isValueDependent())
+        return EvaluateDependentExpr(FS->getInc(), Info);
       // Increment: ++__begin
----------------
As above, I think we need to keep going on `ESR_Succeeded` here.


================
Comment at: clang/lib/AST/ExprConstant.cpp:5896-5898
+    if ((*I)->getInit()->isValueDependent())
+      return EvaluateDependentExpr((*I)->getInit(), Info);
     {
----------------
This is returning an `EvalStmtResult` from a function with a `bool` return type. It'd also be a bit more precise to keep going after a value-dependent expression here, so that we can diagnose non-constant constructs in the body after a typo in the mem-initializer list.


================
Comment at: clang/lib/AST/ExprConstant.cpp:6042-6043
     const Expr *Init = I->getInit();
+    if (Init->isValueDependent())
+      return EvaluateDependentExpr(Init, Info);
     ThisOverrideRAII ThisOverride(*Info.CurrentCall, &SubobjectParent,
----------------
Similarly, it'd be a bit more precise to keep going if `EvaluateDependentExpr` says it succeeded.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84637