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

[hokein]

Summary

The only subexpression that is considered an error now is TypoExpr, but
we plan to add expressions with errors to improve editor tooling on broken
code. We intend to use the same mechanism to guard against spurious
diagnostics on those as well.

See the follow-up revision for an actual usage of the flag.

[ilya-biryukov]

Gentle ping. @rsmith, please take a look when you have time.

[aaron.ballman]

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.

[riccibruno]

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).

clang/include/clang/AST/Expr.h

1543

/*ContainsError=*/false here and elsewhere ?

3584

What is this dead2 ?

clang/lib/AST/ExprObjC.cpp

29 ↗

(On Diff #212848)

s/ContainsErros/ContainsErrors

[ilya-biryukov]

In 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, @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).

[aaron.ballman]

In 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).

That's true. I had figured that answering "does this expression contain an invalid sub-expression" could be implemented with a walk of the expression tree rather than consuming a bit. To consumers of containsErrors(), there shouldn't be much difference aside from performance (and that may be sufficient reason to go with a bit, but I think I'd like to see performance measurements that show the bit is necessary).

[riccibruno]

In D65591#1625183, @aaron.ballman wrote:

In 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).

That's true. I had figured that answering "does this expression contain an invalid sub-expression" could be implemented with a walk of the expression tree rather than consuming a bit. To consumers of containsErrors(), there shouldn't be much difference aside from performance (and that may be sufficient reason to go with a bit, but I think I'd like to see performance measurements that show the bit is necessary).

Fair enough, especially given that IIRC there are not many bits left in these bit-fields classes.

[ilya-biryukov]

In D65591#1625183, @aaron.ballman wrote:

In 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).

That's true. I had figured that answering "does this expression contain an invalid sub-expression" could be implemented with a walk of the expression tree rather than consuming a bit. To consumers of containsErrors(), there shouldn't be much difference aside from performance (and that may be sufficient reason to go with a bit, but I think I'd like to see performance measurements that show the bit is necessary).

Are expression bits scarce?
We don't have any checks if expressions contain errors now, we simply drop too many invalid expressions and never put them into the AST.
It's impossible to do the measurements at this point, but it would be nice if adding those checks was cheap.

We can also assume they're cheap, use the visitor-based implementation and later switch if this turn out to be a problem.
I think we should prefer the approach that guarantees the compiler is not getting slow ever rather than chase the slowness later.

In D65591#1625192, @riccibruno wrote:

Fair enough, especially given that IIRC there are not many bits left in these bit-fields classes.

May be reasonable in that case, but we should be aware of a potential O(n^2) and even exponential complexities in the frontend arising from calling containsErrors in too many places in that case.
If we ever run out of bits, we could remove this bit and instead introduce a cache (something like set<Expr*> InvalidExprs) in ASTContext.

[riccibruno]

In D65591#1625228, @ilya-biryukov wrote:

In D65591#1625183, @aaron.ballman wrote:

In 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).

That's true. I had figured that answering "does this expression contain an invalid sub-expression" could be implemented with a walk of the expression tree rather than consuming a bit. To consumers of containsErrors(), there shouldn't be much difference aside from performance (and that may be sufficient reason to go with a bit, but I think I'd like to see performance measurements that show the bit is necessary).

Are expression bits scarce?
We don't have any checks if expressions contain errors now, we simply drop too many invalid expressions and never put them into the AST.
It's impossible to do the measurements at this point, but it would be nice if adding those checks was cheap.

Sort of... I went through the list of bit-fields classes and if I count correctly there is 4 (possibly 5) bits left in Stmt or Expr which can be used without doing anything else. I guess that using one of them is okay.

We can also assume they're cheap, use the visitor-based implementation and later switch if this turn out to be a problem.
I think we should prefer the approach that guarantees the compiler is not getting slow ever rather than chase the slowness later.

In D65591#1625192, @riccibruno wrote:

Fair enough, especially given that IIRC there are not many bits left in these bit-fields classes.

May be reasonable in that case, but we should be aware of a potential O(n^2) and even exponential complexities in the frontend arising from calling containsErrors in too many places in that case.
If we ever run out of bits, we could remove this bit and instead introduce a cache (something like set<Expr*> InvalidExprs) in ASTContext.

[aaron.ballman]

In D65591#1625228, @ilya-biryukov wrote:

In D65591#1625183, @aaron.ballman wrote:

In 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).

That's true. I had figured that answering "does this expression contain an invalid sub-expression" could be implemented with a walk of the expression tree rather than consuming a bit. To consumers of containsErrors(), there shouldn't be much difference aside from performance (and that may be sufficient reason to go with a bit, but I think I'd like to see performance measurements that show the bit is necessary).

Are expression bits scarce?
We don't have any checks if expressions contain errors now, we simply drop too many invalid expressions and never put them into the AST.
It's impossible to do the measurements at this point, but it would be nice if adding those checks was cheap.

We can also assume they're cheap, use the visitor-based implementation and later switch if this turn out to be a problem.
I think we should prefer the approach that guarantees the compiler is not getting slow ever rather than chase the slowness later.

The problem is: those bits are not infinite and we only have a handful left until bumping the allocation size; is this use case critical enough to use one of those bits? I don't think it will be -- it seems like premature optimization. Also, by calculating rather than using a bit, you don't have to touch every Expr constructor, which reduces the complexity of the patch.

Some other things I think are missing from the patch (regardless of whether you go with a bit or calculate on the fly):

• Do you need some changes to AST serialization and deserialization? Does anything special need to happen for modules?
• I would expect to see this information reflected in an AST dump
• How should this impact AST matching interfaces?
• Test cases

[ilya-biryukov]

In D65591#1625744, @aaron.ballman wrote:

The problem is: those bits are not infinite and we only have a handful left until bumping the allocation size; is this use case critical enough to use one of those bits? I don't think it will be -- it seems like premature optimization. Also, by calculating rather than using a bit, you don't have to touch every Expr constructor, which reduces the complexity of the patch.

Alternatively, we could keep an equivalent of set<Expr*> InvalidExprs in ASTContext. Gives the same computational complexity, but has a higher constant overhead factor.
Does that look reasonable?

Some other things I think are missing from the patch (regardless of whether you go with a bit or calculate on the fly):

• Do you need some changes to AST serialization and deserialization?

Good point, will update the patch.

• Does anything special need to happen for modules?

Not sure. What are the potential problems you foresee?

• I would expect to see this information reflected in an AST dump

Good point. Will do. Although it's a little hard to test in this patch, since it's hard to catch a TypoExpr in the AST dump.

• How should this impact AST matching interfaces?

We could add a matcher that filters on this flag, but I would start with adding more expressions first (something similar to ErrorExpr);
For the purposes of this patch, I'd keep the matcher interfaces untouched.

• Test cases

Again, since it's hard to catch a TypoExpr in the final AST dump, it's hard to catch this bit. See the dependent revision for a bogus diagnostic not being emitted anymore.

[aaron.ballman]

In D65591#1626638, @ilya-biryukov wrote:

In D65591#1625744, @aaron.ballman wrote:

The problem is: those bits are not infinite and we only have a handful left until bumping the allocation size; is this use case critical enough to use one of those bits? I don't think it will be -- it seems like premature optimization. Also, by calculating rather than using a bit, you don't have to touch every Expr constructor, which reduces the complexity of the patch.

Alternatively, we could keep an equivalent of set<Expr*> InvalidExprs in ASTContext. Gives the same computational complexity, but has a higher constant overhead factor.
Does that look reasonable?

Yup, that is also very reasonable.

Some other things I think are missing from the patch (regardless of whether you go with a bit or calculate on the fly):

• Do you need some changes to AST serialization and deserialization?

Good point, will update the patch.

• Does anything special need to happen for modules?

Not sure. What are the potential problems you foresee?

I'm not certain there are real problems there, but I am wondering whether something like a BMI should include serialized error AST nodes or not. Would a consumer of the module expect that? I suppose it could if we wanted it to.

• I would expect to see this information reflected in an AST dump

Good point. Will do. Although it's a little hard to test in this patch, since it's hard to catch a TypoExpr in the AST dump.

Ah, drat, I was hoping we had at least one test that already did this, but I don't see one.

• How should this impact AST matching interfaces?

We could add a matcher that filters on this flag, but I would start with adding more expressions first (something similar to ErrorExpr);
For the purposes of this patch, I'd keep the matcher interfaces untouched.

I think that makes sense. The sort of things I'm wondering about are: if we are going to start retaining error nodes in the AST more regularly, should AST matchers opt into matching on those nodes, or opt out of matching on them? I think the answer should be that we only AST match on valid AST nodes, but I could see arguments being made either way, so maybe this part needs an RFC for more opinions.

• Test cases

Again, since it's hard to catch a TypoExpr in the final AST dump, it's hard to catch this bit. See the dependent revision for a bogus diagnostic not being emitted anymore.

Yeah, I was hoping we'd have some AST dumping mechanism for testing this. If not, though, perhaps we could still use a unit test?

[ilya-biryukov]

I did some experiments with adding something similar to the ErrorExpr Aaron suggest. It has this flag set and does not get removed from the AST.
I believe the best way to address Aaron's comments is to add tests mentioning this expression instead of trying to catch TypoExpr, which normally get removed from the AST.

Would need some time to fix a few remaining test failures (bogus diagnostics), I'll update this patch when I have something (hopefully tomorrow).

[ilya-biryukov]

• Add the added bit to serialization
• Mention contains-errors in the AST dump. Still not tests in this revision, see D69330 for an expression that is actually preserved and has this bit.

[merge_guards_bot]

Build result: fail - 59344 tests passed, 2 failed and 812 were skipped.

failed: LLVM.tools/llvm-ar/mri-utf8.test
failed: LLVM.tools/llvm-objdump/X86/disassemble-functions.test

Log files: cmake-log.txt, ninja_check_all-log.txt, CMakeCache.txt

[rsmith]

In D65591#1625744, @aaron.ballman wrote:

In D65591#1625228, @ilya-biryukov wrote:

We can also assume they're cheap, use the visitor-based implementation and later switch if this turn out to be a problem.
I think we should prefer the approach that guarantees the compiler is not getting slow ever rather than chase the slowness later.

The problem is: those bits are not infinite and we only have a handful left until bumping the allocation size; is this use case critical enough to use one of those bits?

In the abstract, improving error recovery and toolability are among the more important things we could do, and it certainly seems worth spending a bit on every Expr on this to me, if there's no better alternative. (That is: I think this approach is OK if we don't find something better.)

It's not obvious to me whether recomputing this information by AST traversal would be fast enough: *if* we want to perform the "does this contain errors?" check from contexts we encounter frequently (eg, when deciding whether to produce warnings, whether we should skip certain semantic actions, and similar), we can't do that if it requires a traversal.

In D65591#1626969, @aaron.ballman wrote:

In D65591#1626638, @ilya-biryukov wrote:

Alternatively, we could keep an equivalent of set<Expr*> InvalidExprs in ASTContext. Gives the same computational complexity, but has a higher constant overhead factor.
Does that look reasonable?

Yup, that is also very reasonable.

I think this will be very expensive from a maintenance (and potentially performance) perspective: every constructor for every expression node will need to check whether its subexpressions are in the set, and if so, add themselves to the set.

As a variant on this, we could build a map<Expr*, bool> to track whether each expression is known to contain errors, or known to not contain errors, populate it lazily when queried, and shortcut the whole mechanism if we've never created an error node. That still seems a little expensive, but at least we'd only pay the cost for invalid programs. If we could produce a dense numbering of Exprs, this'd suddenly seem pretty good, though, and we know that Exprs are allocated on the ASTContext's bump allocator, so maybe there's something we can do there...

Another option is to represent "contains an error" with a specific Type. This'd mean we'd not preserve type information in less-common cases such as: f( T(some-erroneous-expression) ), and wouldn't be able to diagnose if there's no function f that takes a T, but I think that's an acceptable loss. The downside is that we would need logic to create representations with the appropriate "contains an error" type throughout Sema -- everywhere where we create nodes with dependent types, plus some additional cases like cast expressions that are never type-dependent (but can contain errors).

Here's my current thinking:

• Using dependence to model "contains errors" is a mistake. We should stop doing that.
• We therefore need a different type to indicate "couldn't determine the type of this because it contains errors", and should add a new builtin type for that.
• In cases like int( thing-containing-errors ), it's probably completely fine that we can't determine whether the expression contains errors, and we shouldn't provide an interface to ask that question. (The current flag bit doesn't help with that anyway, since the errors could be nested indirectly, say inside a lambda, and we don't propagate the flag through that.)

So my proposal would be: stop trying to track whether expressions contain errors. Instead, add a builtin type to indicate whether the type of an expression couldn't be determined due to an error, and propagate that in the same way we propagate type-dependence. Would that address the use cases here?

I'm not certain there are real problems there, but I am wondering whether something like a BMI should include serialized error AST nodes or not. Would a consumer of the module expect that? I suppose it could if we wanted it to.

We already do preserve invalid AST through AST files if we're asked to do so; this isn't new. (For example, the Invalid flag on Decls is preserved.) This is important for some tooling scenarios: you want to be able to produce and use a precompiled preamble even if it contains errors.

Ah, drat, I was hoping we had at least one test that already did this, but I don't see one.

Maybe we should introduce a __builtin_dump(expr) function that dumps its operand at the point where semantic analysis is first done? That'd be useful for other things too (#pragma clang __debug dump X is a little unwieldy).

[ilya-biryukov]

I would actually try to counter the type proposal and defend the containsErrors bit. Here's my thinking.

Not knowing that int(<something-that-contains-errors>) had errors inside can lead to situations with bad diagnostics.
We won't be able to suppress any non-type-related error, e.g. int *a = int(<something-that-contains-errors>) will probably complain that you can't assign an int to a pointer. This diagnostics is spurious, because <something-that-contains-error> might have actually evaluated to 0 and the users are better-off fixing the original error first and showing the other diagnostic is just noise.
The example might look a little contrived, but having an easy-to-use mechanism to suppress those errors is useful.

Expressions are not the only construct where we loose information. Something like containsErrors could generalize to types, template arguments, name qualifiers and other constructs that can potentially have valid sub-components, even if the compiler chooses to not produce them now due to semantic errors. This is forward-looking and I don't think we have concrete plans to make this happen, but it would be sad to go with a design that can't be extended to anything beyond expressions even theoretically.

Adding a new type, properly propagating it and checking for it in all places where we suppress diagnostics is a lot of work. containsErrors is a good way to incrementally find places that need to be tweaked and fix them to avoid producing extra diagnostics and running function that require well-formed ASTs. Having a new type seems like an all-or-nothing effort, i.e. one would have to spend considerable time building it and fixing everything that used to produce invalid expressions.
As for the lamda case: can we actually propagate containsErrors through lambdas? That probably means lifting this bit to the statement level, but that could definitely work. Knowing that lambda (or any other function, for that matter) produced an error somewhere inside its body actually looks like useful information and propagating it is not a lot of effort (in fact, I did try this and, while being more complicated than expressions, propagating this flag for statements seems doable).

[rsmith]

Summary of an off-line discussion with Ilya:

• The current situation where Clang can do delayed typo correction for C++ but not for C is not a good long-term position. The reason for that situation is that we use a dependent type to represent a type with errors. There are two natural paths out of this situation: (1) use something other than a dependent type, such as an error type, for this purpose, and (2) support dependence in C. Of these, (1) is pretty much strictly more expensive than (2): whatever work we need to do for the C-only parts of Clang in (2), we will need to do the same work in the C-only parts of Clang for (1), and much more work in addition. It's also worth noting that we will in fact want all the different flavors of dependence for error tracking too: we will ideally want to distinguish between "this expression can't be evaluated because its value depends on an error" and "this expression doesn't have a meaningful type because its type depends on an error". Conclusions: 1. we should support delayed typo-correction in C by supporting dependence in C, and 2. we should generalize what we mean by "dependent" to mean "depends on a template parameter or an error".
• Computing 4 (with this patch, 5) bits of status summarizing properties of subexpressions is highly undesirable: it's error-prone, repetitive, and expensive. It would be better if we packed these five bits into an enum and had a single consistent mechanism for computing them based on the corresponding properties of subexpressions, and if we had some way to do that that didn't require complexity in every constructor for every AST node. (We could, for example, set the bits from the FooExpr::Create functions by calling one centralized function that walks the children and accumulates properties from them -- with all exceptions to that general pattern in a single place.) Conclusions: 3. it's time to refactor these bits; the proof that we've done this right will be that adding the fifth such bit is easy.
• A transitive "does this or anything within it contain errors" flag has value. It may not be exactly the right information for any particular query, but finer-grained information (eg, "do we know the type of this expression?") can be obtained using the various dependence bits. Example: this bit lets us determine whether dependence means "depends on a template parameter" (bit is false) or whether it means "don't know what this depends on, suppress follow-on errors" (bit is true). Eg, we could stop producing a bogus diagnostic for a missing template keyword in typo.function<int>(). Example: this bit will permit us to distinguish between "function has a body and as far as we know it's valid" and "function has a body that contained errors", and suppress errors for (eg) missing return statements in a constexpr function, and likewise suppress control-flow-based warnings on function granularity rather than whole-compilation granularity. (These examples are all cases where we have a significant diagnostic quality problem today.)

Here's the plan we came up with:

1. (Notionally) generalize what we mean by "dependent" to mean "depends on a template parameter or on an error".
2. Refactor and centralize the computation and propagation of the dependence and "contains pack" bits.
3. Add the containsErrors bit (after (2) this should be easy) to all statements (not only expressions)
4. (Asynchronously) add support for dependent constructs to Clang's C-only codepaths and start doing delayed typo-correction (and preserving invalid subexpressions) even in C.

@ilya-biryukov Did I forget anything?

[aaron.ballman]

In D65591#1791876, @rsmith wrote:

Summary of an off-line discussion with Ilya:

<snip>

I think that summary sounds very sensible. Thank you both for working on this!

[ilya-biryukov]

In D65591#1791876, @rsmith wrote:

@ilya-biryukov Did I forget anything?

SG, I don't think anything is missing. Thanks for the write-up!

[ilya-biryukov]

Rebase on top of refactored dependence propagation

[ilya-biryukov]

As soon as I add a new enum value, this starts crashing some tests (even if we don't ever set the flag or serialize it).
Not sure what causes it and won't have time to figure it out before I leave. Sorry about that :(

[hokein]

• rebase to master
• fix the crash

[hokein]

rebase

[sammccall]

This is nice and simple, I do wonder whether we're missing cases in ComputeDependence.cpp for the new bit.

Can you take a pass through to check? (Assuming you haven't since picking up this patch from Ilya). I'll do the same.

clang/include/clang/AST/ASTDumperUtils.h

65

I wonder if coloring the whole subtree red is an overreaction, but it may not be. Let's see :)

clang/include/clang/AST/DependenceFlags.h

20

Nit: it seems weird to jam this right in the middle of an enum.
Instantiation/Type/Value are about template dependence, unexpandedpack is a bit different (but still strictly C++ semantics), Error is definitely different (outside language semantics).

Maybe make error the last element?
(Making it first is weird as it's so atypical)

If this is to make the bitcasting hacks work, I think we should write them out by hand instead.

20

this needs a comment explaining what it is - we're making it up, so you can hardly look it up on cppreference :-)

Suggestion:

This expr contains or references an error, and is considered dependent on how that error is resolved.

46

I'd like this comment to explain why it exists if not used in actual types.

Is this used for decltype(some-error-expr)? Is this used so toTypeDependence() returns something meaningful?

If this is used to make the bitcasting hacks work, we should just stop doing that.

clang/lib/AST/ComputeDependence.cpp

626

This pattern makes me nervous, and feel like we should be blacklisting bits rather than whitelisting them in general.

But no need to change it here, I think much of ComputeDependence could be written more expressively, project for another day.

[hokein]

• address comments
• remove the placeholder errorbit in non-Expr dependence

[hokein]

In D65591#1927076, @sammccall wrote:

This is nice and simple, I do wonder whether we're missing cases in ComputeDependence.cpp for the new bit.

Can you take a pass through to check? (Assuming you haven't since picking up this patch from Ilya). I'll do the same.

will do it. unfortunately, we are missing tests in this patch -- most AST nodes are dropped when clang sees semantic errors, so it is hard to write tests.
after https://reviews.llvm.org/D69330, it'd be better, I think we can add them afterwards.

clang/include/clang/AST/ASTDumperUtils.h

65

I used it for a while, didn't see any big issue about it, we only color the contains errors word.

clang/include/clang/AST/DependenceFlags.h

46

yeah, the main purpose of it is for convenient bitcast. AFAIK, we don't have a plan to use the error bit except in Expr. removing it for now.

[sammccall]

clang/include/clang/AST/DependenceFlags.h

46

I think we should plan to do this for Type too, though it's OK not to do so in this patch.

consider e.g. the expression decltype(foo){} where foo has errors. Today we're saying this has no errors, because the DeclTypeType node isn't error-dependent.

(This is true whether you add the enum value or not: the Type constructor takes a bunch of booleans in the constructor, it would need to be refactored to support TypeDependence and possibly the computeDependence pattern)

I think we should ensure that as far as possible, this code conceptually propagates error-dependence from types to expressions correctly, even if the error-dependence is not set in practice yet.
I'm not sure if this requires having the Error bit in TypeDependence now: if we never have to name it because we only blacklist bits, then it's probably OK.

clang/lib/AST/ComputeDependence.cpp

124

we should be propagating more bits here:

• error bit should be propagated from the type
• type is already propagated
• I think value/instantiation doesn't matter because if T is value/instantiation dependent we set both below
• not sure why unexpanded-pack isn't propagated

so consider something like (unconditional)

// FIXME: why is unexpanded-pack not propagated?
D |= E->getType()->getDependence() & ~ExprDependence::UnexpandedPack

174

I'm not terribly sure of the implications of not propagating the error bit here. I tend to think that "contains errors" most closely follows instantiation-dependence (i.e. it's fairly strict/lexical), so I'd consider propagating it here.

BTW, DesignatedInitUpdateExpr seems to have totally broken dependence computations - it's always non-dependent! (Not introduced by this refactoring, I think). Any idea what's up there?

216

I believe if we're creating an initializer_list<T> and T has errors, then the expr should have errors

376

I think we're missing error propagation from Type.

(we could also consider a declrefexpr to an invalid decl to be an error...)

500

this should be D |=... so that result expr errors propagate

582

(if we decide to make referring to an invalid decl an error, this is another spot... there are probably lots :-(. Deferring this is probably best)

[hokein]

address comments, and add more missing-error-bit cases

[hokein]

clang/include/clang/AST/DependenceFlags.h

46

OK, I'd prefer to not expand the scope of this patch right now, because we don't have proper tests :( until the final recovery expression patch landed. and I think adding the error-bit to Type would probably require more effort (more code changes).

updated the code in computeDependency to make sure the error-bit will be propagated from type-dependence to err-dependence even we don't have the error bit in TypeDependency now.

(This is true whether you add the enum value or not: the Type constructor takes a bunch of booleans in the constructor, it would need to be refactored to support TypeDependence and possibly the computeDependence pattern)

agree, I think the same to the NestedNameSpecifierDependence, TemplateNameDependence, TemplateArgumentDependence.

clang/lib/AST/ComputeDependence.cpp

174

you mean DesignatedInitExpr? I didn't see any problem there, why it is always non-dependent?

500

getResultExpr() requires ! isResultDependent()...

and D |= is not enough, for err-bit, I think we need to consider all subexpressions.

582

deferred this.

[sammccall]

clang/lib/AST/ComputeDependence.cpp

174

no, DesignatedInitUpdateExpr. There's no computeDependence function, it just marks itself as non-dependent in the constructor.

651

rather than whitelisting the error bit, blacklist the UnexpandedPack bit?

[hokein]

address comment.

clang/lib/AST/ComputeDependence.cpp

174

oh, no, that's totally wrong. I think it was there before, added a FIXME. Thanks for spotting it!

clang/include/clang/AST/ASTDumperUtils.h

 static const TerminalColor AddressColor = {llvm::raw_ostream::YELLOW, false};
 // Source locations
 static const TerminalColor LocationColor = {llvm::raw_ostream::YELLOW, false};
 
 // lvalue/xvalue
 static const TerminalColor ValueKindColor = {llvm::raw_ostream::CYAN, false};
 // bitfield/objcproperty/objcsubscript/vectorcomponent
 static const TerminalColor ObjectKindColor = {llvm::raw_ostream::CYAN, false};
+// contains-errors
+static const TerminalColor ErrorsColor = {llvm::raw_ostream::RED, true};
 
 // Null statements
 static const TerminalColor NullColor = {llvm::raw_ostream::BLUE, false};
 
 // Undeserialized entities
 static const TerminalColor UndeserializedColor = {llvm::raw_ostream::GREEN,
                                                   true};
 

clang/include/clang/AST/DependenceFlags.h

 #include "llvm/ADT/BitmaskEnum.h"
 #include <cstdint>
 
 namespace clang {
 struct ExprDependenceScope {
   enum ExprDependence : uint8_t {
     UnexpandedPack = 1,
     Instantiation = 2,
-    Type = 4,
-    Value = 8,
+    Error = 4,
+    Type = 8,
+    Value = 16,
 
     None = 0,
-    All = 15,
+    All = 31,
 
     TypeValue = Type | Value,
     TypeInstantiation = Type | Instantiation,
     ValueInstantiation = Value | Instantiation,
     TypeValueInstantiation = Type | Value | Instantiation,
 
     LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/Value)
   };
 };
 using ExprDependence = ExprDependenceScope::ExprDependence;
-static constexpr unsigned ExprDependenceBits = 4;
+static constexpr unsigned ExprDependenceBits = 5;
 
 struct TypeDependenceScope {
   enum TypeDependence : uint8_t {
     /// Whether this type contains an unexpanded parameter pack
     /// (for C++11 variadic templates)
     UnexpandedPack = 1,
     /// Whether this type somehow involves a template parameter, even
     /// if the resolution of the type does not depend on a template parameter.
     Instantiation = 2,
+    /// Placeholder, and is not used in actual Type.
+    Error = 4,
     /// Whether this type is a dependent type (C++ [temp.dep.type]).
-    Dependent = 4,
+    Dependent = 8,
     /// Whether this type is a variably-modified type (C99 6.7.5).
-    VariablyModified = 8,
+    VariablyModified = 16,
 
     None = 0,
-    All = 15,
+    All = 31,
 
     DependentInstantiation = Dependent | Instantiation,
 
     LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/VariablyModified)
   };
 };
 using TypeDependence = TypeDependenceScope::TypeDependence;
-static constexpr unsigned TypeDependenceBits = 4;
+static constexpr unsigned TypeDependenceBits = 5;
 
 #define LLVM_COMMON_DEPENDENCE(NAME)                                           \
   struct NAME##Scope {                                                         \
     enum NAME : uint8_t {                                                      \
       UnexpandedPack = 1,                                                      \
       Instantiation = 2,                                                       \
-      Dependent = 4,                                                           \
+      Error = 4,                                                               \
+      Dependent = 8,                                                           \
                                                                                \
       None = 0,                                                                \
       DependentInstantiation = Dependent | Instantiation,                      \
-      All = 7,                                                                 \
+      All = 15,                                                                \
                                                                                \
       LLVM_MARK_AS_BITMASK_ENUM(/*LargestValue=*/Dependent)                    \
     };                                                                         \
   };                                                                           \
   using NAME = NAME##Scope::NAME;                                              \
   static constexpr unsigned NAME##Bits = 3;
 
 LLVM_COMMON_DEPENDENCE(NestedNameSpecifierDependence)

clang/include/clang/AST/Expr.h

public:
   /// \endcode
   ///
   /// The expressions \c args and \c static_cast<Types&&>(args) both
   /// contain parameter packs.
   bool containsUnexpandedParameterPack() const {
     return static_cast<bool>(getDependence() & ExprDependence::UnexpandedPack);
   }
 
+  /// Whether this expression contains subexpressions which had errors, e.g. a
+  /// TypoExpr.
+  bool containsErrors() const {
+    return static_cast<bool>(getDependence() & ExprDependence::Error);
+  }
+
   /// getExprLoc - Return the preferred location for the arrow when diagnosing
   /// a problem with a generic expression.
   SourceLocation getExprLoc() const LLVM_READONLY;
 
   /// isUnusedResultAWarning - Return true if this immediate expression should
   /// be warned about if the result is unused.  If so, fill in expr, location,
   /// and ranges with expr to warn on and source locations/ranges appropriate
   /// for a warning.

 private:
   unsigned Value;
   SourceLocation Loc;
 public:
   // type should be IntTy
   CharacterLiteral(unsigned value, CharacterKind kind, QualType type,
                    SourceLocation l)
       : Expr(CharacterLiteralClass, type, VK_RValue, OK_Ordinary), Value(value),
         Loc(l) {
     CharacterLiteralBits.Kind = kind;
     setDependence(ExprDependence::None);
   }
 
   /// Construct an empty character literal.
   CharacterLiteral(EmptyShell Empty) : Expr(CharacterLiteralClass, Empty) { }
 
   SourceLocation getLocation() const { return Loc; }
public:
 
   // Get the FENV_ACCESS status of this operator. Only meaningful for
   // operations on floating point types.
   bool isFEnvAccessOn() const { return getFPFeatures().allowFEnvAccess(); }
 
 protected:
   BinaryOperator(Expr *lhs, Expr *rhs, Opcode opc, QualType ResTy,
                  ExprValueKind VK, ExprObjectKind OK, SourceLocation opLoc,
                  FPOptions FPFeatures, bool dead2)
       : Expr(CompoundAssignOperatorClass, ResTy, VK, OK) {
     BinaryOperatorBits.Opc = opc;
     BinaryOperatorBits.FPFeatures = FPFeatures.getInt();
     BinaryOperatorBits.OpLoc = opLoc;
     SubExprs[LHS] = lhs;
     SubExprs[RHS] = rhs;
     setDependence(computeDependence(this));
   }

 };
 
 /// TypoExpr - Internal placeholder for expressions where typo correction
 /// still needs to be performed and/or an error diagnostic emitted.
 class TypoExpr : public Expr {
 public:
   TypoExpr(QualType T) : Expr(TypoExprClass, T, VK_LValue, OK_Ordinary) {
     assert(T->isDependentType() && "TypoExpr given a non-dependent type");
-    setDependence(ExprDependence::TypeValueInstantiation);
+    setDependence(ExprDependence::TypeValueInstantiation |
+                  ExprDependence::Error);
   }
 
   child_range children() {
     return child_range(child_iterator(), child_iterator());
   }
   const_child_range children() const {
     return const_child_range(const_child_iterator(), const_child_iterator());
   }

clang/include/clang/AST/Type.h

Linkage getLinkage() const {
       return static_cast<Linkage>(CachedLinkage);
     }
 
     bool hasLocalOrUnnamedType() const {
       assert(isCacheValid() && "getting linkage from invalid cache");
       return CachedLocalOrUnnamed;
     }
   };
-  enum { NumTypeBits = 18 };
+  enum { NumTypeBits = 8 + TypeDependenceBits + 6 };
 
 protected:
   // These classes allow subclasses to somewhat cleanly pack bitfields
   // into Type.
 
   class ArrayTypeBitfields {
     friend class ArrayType;
 

clang/lib/AST/ComputeDependence.cpp

 }
 
 ExprDependence clang::computeDependence(BinaryConditionalOperator *E) {
   return E->getCommon()->getDependence() | E->getFalseExpr()->getDependence();
 }
 
 ExprDependence clang::computeDependence(StmtExpr *E, unsigned TemplateDepth) {
   auto D = ExprDependence::None;
   if (E->getType()->isDependentType())
     D |= ExprDependence::Type;
   // Note: we treat a statement-expression in a dependent context as always
   // being value- and instantiation-dependent. This matches the behavior of
   // lambda-expressions and GCC.
   if (TemplateDepth)
     D |= ExprDependence::ValueInstantiation;
   return D;
 }

 ExprDependence clang::computeDependence(VAArgExpr *E) {
   auto D =
       toExprDependence(E->getWrittenTypeInfo()->getType()->getDependence()) |
       (E->getSubExpr()->getDependence() & ~ExprDependence::Type);
   return D & ~ExprDependence::Value;
 }
 
 ExprDependence clang::computeDependence(NoInitExpr *E) {
   return toExprDependence(E->getType()->getDependence()) &
          ExprDependence::Instantiation;
 }
 
 ExprDependence clang::computeDependence(ArrayInitLoopExpr *E) {
   auto D = E->getCommonExpr()->getDependence() |
            E->getSubExpr()->getDependence() | ExprDependence::Instantiation;
   if (!E->getType()->isInstantiationDependentType())
     D &= ~ExprDependence::Instantiation;

 }
 
 ExprDependence clang::computeDependence(CXXRewrittenBinaryOperator *E) {
   return E->getSemanticForm()->getDependence();
 }
 
 ExprDependence clang::computeDependence(CXXStdInitializerListExpr *E) {
   auto D = turnTypeToValueDependence(E->getSubExpr()->getDependence());
   if (E->getType()->isDependentType())
     D |= ExprDependence::Type;
   return D;
 }
 
 ExprDependence clang::computeDependence(CXXTypeidExpr *E) {
   auto D = ExprDependence::None;
   if (E->isTypeOperand())
     D = toExprDependence(
ExprDependence clang::computeDependence(DeclRefExpr *E, const ASTContext &Ctx) {
 
   if (auto *FirstArg = E->getTemplateArgs()) {
     unsigned NumArgs = E->getNumTemplateArgs();
     for (auto *Arg = FirstArg, *End = FirstArg + NumArgs; Arg < End; ++Arg)
       Deps |= toExprDependence(Arg->getArgument().getDependence());
   }
 
   auto *Decl = E->getDecl();
   auto Type = E->getType();
 
   if (Decl->isParameterPack())
     Deps |= ExprDependence::UnexpandedPack;
 
   // (TD) C++ [temp.dep.expr]p3:
   //   An id-expression is type-dependent if it contains:
   //
   // and
ExprDependence clang::computeDependence(ShuffleVectorExpr *E) {
   return D;
 }
 
 ExprDependence clang::computeDependence(GenericSelectionExpr *E,
                                         bool ContainsUnexpandedPack) {
   auto D = ContainsUnexpandedPack ? ExprDependence::UnexpandedPack
                                   : ExprDependence::None;
   if (E->isResultDependent())
     return D | ExprDependence::TypeValueInstantiation;
   return D | (E->getResultExpr()->getDependence() &
               ~ExprDependence::UnexpandedPack);
 }
 
 ExprDependence clang::computeDependence(DesignatedInitExpr *E) {
   auto Deps = E->getInit()->getDependence();
   for (auto D : E->designators()) {
     auto DesignatorDeps = ExprDependence::None;
if (KnownDependent)
     Deps |= ExprDependence::TypeValue;
   if (KnownInstantiationDependent)
     Deps |= ExprDependence::Instantiation;
   if (KnownContainsUnexpandedParameterPack)
     Deps |= ExprDependence::UnexpandedPack;
   Deps |= getDependenceInExpr(E->getNameInfo());
   if (auto *Q = E->getQualifier())
     Deps |= toExprDependence(Q->getDependence());
   for (auto *D : E->decls()) {
     if (D->getDeclContext()->isDependentContext() ||
         isa<UnresolvedUsingValueDecl>(D))
       Deps |= ExprDependence::TypeValueInstantiation;
   }
   // If we have explicit template arguments, check for dependent
   // template arguments and whether they contain any unexpanded pack
   // expansions.
   for (auto A : E->template_arguments())

 }
 
 ExprDependence clang::computeDependence(CXXUnresolvedConstructExpr *E) {
   auto D = ExprDependence::ValueInstantiation;
   D |= toExprDependence(E->getType()->getDependence());
   if (E->getType()->getContainedDeducedType())
     D |= ExprDependence::Type;
   for (auto *A : E->arguments())
-    D |= A->getDependence() & ExprDependence::UnexpandedPack;
+    D |= A->getDependence() &
+         (ExprDependence::UnexpandedPack | ExprDependence::Error);
   return D;
 }
 
 ExprDependence clang::computeDependence(CXXDependentScopeMemberExpr *E) {
   auto D = ExprDependence::TypeValueInstantiation;
   if (!E->isImplicitAccess())
     D |= E->getBase()->getDependence();
   if (auto *Q = E->getQualifier())
     D |= toExprDependence(Q->getDependence());
   D |= getDependenceInExpr(E->getMemberNameInfo());
   for (auto A : E->template_arguments())
     D |= toExprDependence(A.getArgument().getDependence());
   return D;
 }
 
 ExprDependence clang::computeDependence(MaterializeTemporaryExpr *E) {
   return E->getSubExpr()->getDependence();
 }
 
 ExprDependence clang::computeDependence(TypeTraitExpr *E) {
   auto D = ExprDependence::None;
   for (const auto *A : E->getArgs())
     D |=
         toExprDependence(A->getType()->getDependence()) & ~ExprDependence::Type;
   return D;
 }
 
 ExprDependence clang::computeDependence(ConceptSpecializationExpr *E,
                                         bool ValueDependent) {
   auto TA = TemplateArgumentDependence::None;
   const auto InterestingDeps = TemplateArgumentDependence::Instantiation |
                                TemplateArgumentDependence::UnexpandedPack;

clang/lib/AST/TextNodeDumper.cpp

if (!Node) {
     OS << Node->getStmtClassName();
   }
   dumpPointer(Node);
   dumpSourceRange(Node->getSourceRange());
 
   if (const auto *E = dyn_cast<Expr>(Node)) {
     dumpType(E->getType());
 
+    if (E->containsErrors()) {
+      ColorScope Color(OS, ShowColors, ErrorsColor);
+      OS << " contains-errors";
+    }
+
     {
       ColorScope Color(OS, ShowColors, ValueKindColor);
       switch (E->getValueKind()) {
       case VK_RValue:
         break;
       case VK_LValue:
         OS << " lvalue";
         break;

clang/lib/Serialization/ASTReaderStmt.cpp

ASTStmtReader(ASTRecordReader &Record, llvm::BitstreamCursor &Cursor)
         : Record(Record), DeclsCursor(Cursor) {}
 
     /// The number of record fields required for the Stmt class
     /// itself.
     static const unsigned NumStmtFields = 0;
 
     /// The number of record fields required for the Expr class
     /// itself.
-    static const unsigned NumExprFields = NumStmtFields + 7;
+    static const unsigned NumExprFields =
+        NumStmtFields + ExprDependenceBits + 3;
 
     /// Read and initialize a ExplicitTemplateArgumentList structure.
     void ReadTemplateKWAndArgsInfo(ASTTemplateKWAndArgsInfo &Args,
                                    TemplateArgumentLoc *ArgsLocArray,
                                    unsigned NumTemplateArgs);
 
     /// Read and initialize a ExplicitTemplateArgumentList structure.
     void ReadExplicitTemplateArgumentList(ASTTemplateArgumentListInfo &ArgList,
void ASTStmtReader::VisitExpr(Expr *E) {
   VisitStmt(E);
   E->setType(Record.readType());
 
   // FIXME: write and read all DependentFlags with a single call.
   bool TypeDependent = Record.readInt();
   bool ValueDependent = Record.readInt();
   bool InstantiationDependent = Record.readInt();
   bool ContainsUnexpandedTemplateParameters = Record.readInt();
+  bool ContainsErrors = Record.readInt();
   auto Deps = ExprDependence::None;
   if (TypeDependent)
     Deps |= ExprDependence::Type;
   if (ValueDependent)
     Deps |= ExprDependence::Value;
   if (InstantiationDependent)
     Deps |= ExprDependence::Instantiation;
   if (ContainsUnexpandedTemplateParameters)
     Deps |= ExprDependence::UnexpandedPack;
+  if (ContainsErrors)
+    Deps |= ExprDependence::Error;
   E->setDependence(Deps);
 
   E->setValueKind(static_cast<ExprValueKind>(Record.readInt()));
   E->setObjectKind(static_cast<ExprObjectKind>(Record.readInt()));
   assert(Record.getIdx() == NumExprFields &&
          "Incorrect expression field count");
 }
 

clang/lib/Serialization/ASTWriterDecl.cpp

void ASTWriter::WriteDeclAbbrevs() {
   Abv->Add(BitCodeAbbrevOp(serialization::EXPR_DECL_REF));
   //Stmt
   // Expr
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // Type
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //TypeDependent
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //ValueDependent
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //InstantiationDependent
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //UnexpandedParamPack
+  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // ContainsErrors
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 3)); //GetValueKind
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 3)); //GetObjectKind
   //DeclRefExpr
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //HasQualifier
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //GetDeclFound
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //ExplicitTemplateArgs
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //HadMultipleCandidates
   Abv->Add(BitCodeAbbrevOp(0)); // RefersToEnclosingVariableOrCapture
   Abv->Add(BitCodeAbbrevOp(0)); // NonOdrUseReason
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // DeclRef
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // Location
   DeclRefExprAbbrev = Stream.EmitAbbrev(std::move(Abv));
 
   // Abbreviation for EXPR_INTEGER_LITERAL
   Abv = std::make_shared<BitCodeAbbrev>();
   Abv->Add(BitCodeAbbrevOp(serialization::EXPR_INTEGER_LITERAL));
   //Stmt
   // Expr
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // Type
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //TypeDependent
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //ValueDependent
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //InstantiationDependent
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //UnexpandedParamPack
+  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // ContainsErrors
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 3)); //GetValueKind
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 3)); //GetObjectKind
   //Integer Literal
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // Location
   Abv->Add(BitCodeAbbrevOp(32));                      // Bit Width
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // Value
   IntegerLiteralAbbrev = Stream.EmitAbbrev(std::move(Abv));
 
   // Abbreviation for EXPR_CHARACTER_LITERAL
   Abv = std::make_shared<BitCodeAbbrev>();
   Abv->Add(BitCodeAbbrevOp(serialization::EXPR_CHARACTER_LITERAL));
   //Stmt
   // Expr
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // Type
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //TypeDependent
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //ValueDependent
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //InstantiationDependent
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //UnexpandedParamPack
+  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // ContainsErrors
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 3)); //GetValueKind
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 3)); //GetObjectKind
   //Character Literal
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // getValue
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // Location
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 3)); // getKind
   CharacterLiteralAbbrev = Stream.EmitAbbrev(std::move(Abv));
 
   // Abbreviation for EXPR_IMPLICIT_CAST
   Abv = std::make_shared<BitCodeAbbrev>();
   Abv->Add(BitCodeAbbrevOp(serialization::EXPR_IMPLICIT_CAST));
   // Stmt
   // Expr
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::VBR, 6)); // Type
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //TypeDependent
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //ValueDependent
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //InstantiationDependent
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); //UnexpandedParamPack
+  Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // ContainsErrors
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 3)); //GetValueKind
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 3)); //GetObjectKind
   // CastExpr
   Abv->Add(BitCodeAbbrevOp(0)); // PathSize
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 6)); // CastKind
   Abv->Add(BitCodeAbbrevOp(BitCodeAbbrevOp::Fixed, 1)); // PartOfExplicitCast
   // ImplicitCastExpr
   ExprImplicitCastAbbrev = Stream.EmitAbbrev(std::move(Abv));

clang/lib/Serialization/ASTWriterStmt.cpp

 
 void ASTStmtWriter::VisitExpr(Expr *E) {
   VisitStmt(E);
   Record.AddTypeRef(E->getType());
   Record.push_back(E->isTypeDependent());
   Record.push_back(E->isValueDependent());
   Record.push_back(E->isInstantiationDependent());
   Record.push_back(E->containsUnexpandedParameterPack());
+  Record.push_back(E->containsErrors());
   Record.push_back(E->getValueKind());
   Record.push_back(E->getObjectKind());
 }
 
 void ASTStmtWriter::VisitConstantExpr(ConstantExpr *E) {
   VisitExpr(E);
   Record.push_back(static_cast<uint64_t>(E->ConstantExprBits.ResultKind));
   switch (E->ConstantExprBits.ResultKind) {