[PATCH] D69330: [AST] Add RecoveryExpr to retain expressions on semantic errors

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Fantastic :-)

@rsmith and others: would appreciate feedback on giving this a feature-flag (to decouple from LangOpts.CPlusPlus):

- whether it's OK to have one
- whether it should be cc1-only
- the name `-f[no]recovery-ast`



================
Comment at: clang/include/clang/AST/Expr.h:5920
+///
+/// RecoveryExpr is type-, value- and instantiation-dependent to take advantage
+/// of existing machinery to deal with dependent code in C++, e.g. RecoveryExpr
----------------
AIUI this should be "for now" with the goal of eliminating the use of template dependence concepts, right?


================
Comment at: clang/include/clang/AST/Expr.h:5935
+                              SourceLocation EndLoc, ArrayRef<Expr *> SubExprs);
+  static RecoveryExpr *CreateEmpty(ASTContext &Ctx, unsigned NumStmts);
+
----------------
nit: NumStmts -> NumSubExprs?


================
Comment at: clang/lib/AST/ComputeDependence.cpp:460
+ExprDependence clang::computeDependence(RecoveryExpr *E) {
+  auto D = ExprDependence::TypeValueInstantiation | ExprDependence::Error;
+  for (auto *S : E->subExpressions())
----------------
Probably worth echoing with a FIXME: drop type+value+instantiation once Error is sufficient to suppress checks.


================
Comment at: clang/lib/Sema/SemaExpr.cpp:18401
+  // FIXME: use containsErrors() to suppress unwanted diags in C.
+  if (!Context.getLangOpts().CPlusPlus)
+    return ExprError();
----------------
I think we should strongly consider a LangOption with an associated flag. (e.g. LangOptions.RecoveryAST, -f[no]recovery-ast).
If we're going to pay tho cost of letting expr creation fail, we might as well use it
Use cases:
 - Control rollout: we can check this in without (yet) flipping the flag on for all tools at once, if desired. If we flip the default and it causes problems for particular tools, we can turn it off for that tool rather than rolling the whole thing back.
 - turn on and off in lit tests to precisely test behavior and avoid dependence on defaults
 - allow incremental work on recovery in !CPlusPlus mode

If this makes sense to you, I'd suggest setting the default to off in this patch (and including some tests that pass `-frecovery-ast`), and then immediately following up with a patch that flips the default to on-for-C++.
This separation makes like easier for everyone if turning this on breaks something.

A bunch of the updates to existing tests would be deferred until that patch.


================
Comment at: clang/lib/Sema/TreeTransform.h:9470
+ExprResult TreeTransform<Derived>::TransformRecoveryExpr(RecoveryExpr *E) {
+  return E;
+}
----------------
rsmith wrote:
> ilya-biryukov wrote:
> > rsmith wrote:
> > > We should either transform the subexpressions or just return `ExprError()` here. With this approach, we can violate AST invariants (eg, by having the same `Expr` reachable through two different code paths in the same function body, or by retaining `DeclRefExpr`s referring to declarations from the wrong context, etc).
> > Thanks, I just blindly copied what TypoExpr was doing without considering the consequences here.
> > 
> > 
> > Added the code to rebuild `RecoveryExpr`.
> > 
> > Any good way to test this?
> Hmm, testing that the old failure mode doesn't happen seems a bit tricky; any test for that is going to be testing second-order effects of the code under test, so will be fragile. But you can test that we're substituting into the `RecoveryExpr` easily enough: put some expression for which substitution will fail into a context where we'll build a `RecoveryExpr` inside a context that we'll `TreeTransform`. For example, maybe:
> 
> ```
> template<typename T> int *p = &void(T::error); // should produce "cannot take address of void"
> int *q = p<int>; // should produce "'int' cannot be used prior to '::'" in instantiation
> ```
@hokein I don't think this test was added yet.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69330