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

[Haojian Wu via Phabricator via cfe-commits]

hokein added inline comments.


================
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
----------------
sammccall wrote:
> AIUI this should be "for now" with the goal of eliminating the use of template dependence concepts, right?
yeah, I think so. 


================
Comment at: clang/lib/Sema/SemaExpr.cpp:18401
+  // FIXME: use containsErrors() to suppress unwanted diags in C.
+  if (!Context.getLangOpts().CPlusPlus)
+    return ExprError();
----------------
sammccall wrote:
> 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.
+1, I think this is a good idea -- particularly letting us incrementally working on it without breaking existing tools, I was highly suspected that this patch will fail internal tests during build copping.

I will wait for a while for other feedback @rsmith before making the actual change.


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69330