[PATCH] D65591: [AST] Add a flag indicating if any subexpression had errors

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

In D65591#1625154 <https://reviews.llvm.org/D65591#1625154>, @riccibruno wrote:

> It seems that these two options are not exactly the same right ? The `ContainsError` bit is useful to quickly answer "Does this expression contains an invalid sub-expression" without doing the search, while adding an `ErrorExpr` node is useful to note that //this// sub-expression is invalid (and as Aaron says the hypothetical `ErrorExpr` node can carry more info about the error).


Exactly right, thanks for summing this up!

In D65591#1625019 <https://reviews.llvm.org/D65591#1625019>, @aaron.ballman wrote:

> Rather than adding a bit onto `Expr` to specify whether it's erroneous, have you considered making this a property of the type system by introducing an `ErrorExpr` AST node that other nodes can inherit from? I think that approach would work more naturally for things like AST matchers while solving some problems we have with being able to pretty printing or AST dump erroneous ASTs. The `ErrorExpr` node could contain a subnode of a partially-valid `Expr` object that would retain as much of the information as we've been able to gather, but the error node (or its subclasses) could contain other information useful when handling errors, such as storing the original source text (or range) for the expression, potential fixes, etc.


Having something similar to `ErrorExpr` is actually the next logical step I'm currently exploring.  The idea of this bit is to carry information on whether any (recursive) child of an expression had an error that was already diagnosed.
Detecting those expression is useful to avoid producing additional irrelevant diagnostics (see the dependent revision that uses the bit to get rid of one such diagnostic) and avoid running various functions that are not prepared to handle subexpressions that have errors (e.g. constant evaluation).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65591