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

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

rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.

In D69330#1752089 <https://reviews.llvm.org/D69330#1752089>, @ilya-biryukov wrote:

> In D69330#1746137 <https://reviews.llvm.org/D69330#1746137>, @rsmith wrote:
>
> > I would prefer that we have dedicated tests for them rather than scattering the tests throughout the existing test suite (for example, consider adding `-Wno-unused` to the tests producing "result unused" warnings and adding a dedicated test).
>
>
> Most updated tests (including those with more "result unused" warnings) are actually not intended to test recovery expressions, they just happen to produce different results now and need to be updated.
>  The only new dedicated tests here are `clang/test/AST/ast-dump-recovery.cpp` and `clang/test/Index/getcursor-recovery.cpp`.
>
> Could technically move them into the same directory, but wanted to make sure I got your point first. Could you elaborate on what testing strategy you'd prefer?


For the tests where you added `expected-warning {{unused}}` that are testing things other than parser recovery, I would instead add `-Wno-unused` or a cast to `void`. `expect`s unrelated to the intent of tests make them less readable and more fragile.

I think it's fine to use `test/AST/ast-dump-recovery.cpp` as the primary test for the various forms of recovery that you added here. I've not thought of anything better, at least :)



================
Comment at: clang/lib/Sema/TreeTransform.h:9470
+ExprResult TreeTransform<Derived>::TransformRecoveryExpr(RecoveryExpr *E) {
+  return E;
+}
----------------
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
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69330