[PATCH] D61722: [AST] Add RecoveryExpr; produce it on overload resolution failure and missing member.

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

rsmith added a comment.

I expect we'll want a `ContainsErrors` bit on `Stmt`, propagated similarly to the existing `InstantiationDependent` / `ContainsUnexpandedParameterPack` / ... bits. For example, the constant evaluator will want to quickly bail out of evaluating expressions and statements containing errors, and the error recovery `TreeTransform` that we perform to fix typos will want that too (and maybe could be able to fix other kinds of errors for which we build these error nodes eventually?).



================
Comment at: include/clang/AST/BuiltinTypes.def:265
+// a template.
+BUILTIN_TYPE(Recovery, RecoveryTy)
+
----------------
erik.pilkington wrote:
> Why are you creating a new type as opposed to just using DependentTy (sorta like TypoExpr does)? It seems like if you want to recycle all the dependence-propagating code in Sema, then you need to fall back to DependentTy anyways, i.e. `1 + <recovery-expr>` will have dependent type with this patch, right?
> 
> 
Using `DependentTy` for `TypoExpr` is a mistake; it leads to all sorts of bad follow-on diagnostics incorrectly claiming that constructs have dependent types.

Rather than calling this `RecoveryTy`, I'd prefer to call it something a bit more general like `ErrorTy`, with the intent being that we eventually use it for `TypoExpr` too.


================
Comment at: include/clang/AST/Expr.h:5801-5809
+/// RecoveryExpr - a broken expression that we couldn't construct an AST for,
+/// e.g. because overload resolution failed.
+/// We capture:
+///  - the kind of expression that would have been produced
+///  - the valid subexpressions
+///  - the type, if known. If unknown, it is the built-in RecoveryTy, which
+///    is dependent (and so generally suppresses further diagnostics etc).
----------------
Do we need this at all, if we have a properly-propagated `ErrorTy` anyway? Instead of using this, it would seem that we could model an ill-formed expression as the corresponding AST node but with its type set to `ErrorTy`. Presumably the latter is what  we'll do when we find a subexpression containing an error, rather than creating a `RecoveryExpr` at every enclosing syntactic level, so AST clients will need to deal with that anyway.


================
Comment at: include/clang/AST/Type.h:2423-2424
+      : Type(Builtin, QualType(),
+             /*Dependent=*/(K == Dependent || K == Recovery),
+             /*InstantiationDependent=*/(K == Dependent || K == Recovery),
              /*VariablyModified=*/false,
----------------
Experience with `TypoExpr` suggests that treating non-dependent constructs as dependent is a mistake in the long term.


Repository:
  rC Clang

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

https://reviews.llvm.org/D61722