[PATCH] D83213: [AST][RecoveryExpr] Don't set the instantiation-bit.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

In D83213#2141387 <https://reviews.llvm.org/D83213#2141387>, @hokein wrote:

> I think this depends on how we interpret the instantiation-dependent bit. In clang, it currently has two semantics:
>
> 1. an expression (somehow) depends on a template parameter;
> 2. an expression is dependent;


Not trying to be difficult, but I'm not sure what you mean by "dependent" in #2. Informally, it means the same thing as #1. Formally, C++ defines type-dependence and value-dependence for expressions, and the ABI defines instantiation-dependence for expressions - AFAIK "dependent" doesn't have a formal meaning.

So I'd rather say it has two definitions:

- informally, it (somehow) depends on a template parameter.
- formally (from the C++ABI), an expression is instantiation-dependent if it is type-dependent or value-dependent, or it has a subexpression that is type-dependent or value-dependent.

To preserve the linkage between these two, **IMO** we need to extend the formal definition to errors if and only if we extend the informal definition to errors. Accidentally unlinking the two is almost certain to result in subtle bugs as either the implementation subtly differs from the spec, or it subtly differs from the mental model.
That is, we need to decide whether to make all of these changes:

- (informal) instantiation-dependent means it (somehow) depends on a template parameter or error
- (formal) value-dependent means that (informally) an expressions's value depends on a tparam or err
- (formal) type-dependent means that (informally) an expression's type depends on a tparam or err

The approach so far has been **YES**. This looks like:

- all RecoveryExprs are value-dependent, instantiation-dependent, and contains-errors
- RecoveryExprs are type-dependent if we didn't guess the type, or we guessed it was a dependent type

Here we make use of existing codepaths that turn off checking in the presence of dependent code. We mostly need to check contains-errors to prevent a bad-recovery cascade.

The alternate approach is **NO**. This looks like:

- all RecoveryExpr are contains-errors
- a RecoveryExpr are instantiation-dependent if a subexpression is (i.e. refers to a template param)
- a RecoveryExpr is value-dependent if one of its direct subexpressions is, or if the broken code is likely to yield different values in different template instantiations due to the context
- a RecoveryExpr is type-dependent if its type is known to be dependent, or if the broken code is likely to yield different types in different template instantiations due to the context

Here codepaths that turn off checking need to turn it off for errors explicitly.
There's subtlety in the value-depnedent and type-dependent bits, which probably needs to be resolved on a case-by-case basis.
And there's more machinery needed: when an expression's type is not known but isn't believed to be dependent, what is it? I think we end up needing to define RecoveryType here (the fundamental contains-errors type).

So this is doable and maybe even a good idea (not obvious to me what the practical differences are). But it's a large change, not a small one like this patch.

> **Option 1** (what this patch does):
> 
> instantiation-dependence still  implies the expression involves a template parameter -- if a recovery expression doesn't involves a template parameter, we don't set this flag

It doesn't make sense IMO to make this change without also saying "value-dependence implies the value dependds on a template parameter". It's too surprising and hard to reason about, and I don't see a principled reason for this distinction.

> Not sure what pros would buy us

That's the thing that surprises me, it's a richer model, it's principled and correct... but I have no intuition what we can use it for.

> **Option 2**:
> cons:
> 
> - keep using the instantiation name is confusing; we could do clarify the document of isInstantiation

Yeah. I don't think actually renaming it would be appropriate, but we should be really explicit about its non-template meaning, and also give a generalized intuitive definition that makes sense. ("Depends in any way on a template parameter or error. In a template context, this implies that the resolution of this expr may cause instantiation to fail")

> While writing this, I'm starting to like the option2, any other thoughts?

Yeah, I think we'd have to have some concrete benefits to pursue option 1. Doing it right is a lot of work.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D83213