[PATCH] D149612: [Sema] avoid merge error type

[Haojian Wu via Phabricator via cfe-commits]

hokein added a comment.

In D149612#4346068 <https://reviews.llvm.org/D149612#4346068>, @sammccall wrote:

> Unless I'm wildly misremembering (@hokein knows better), the type should also be dependent in C.
>
> The intuition is that the code has some meaning that depends on how the error is resolved (by the programmer changing the code!), and that "dependent" is a good first-approximation way to model this (defer further analysis, suppress diagnostics) and "contains-errors" allows us to specifically handle the cases that need it.
>
> Of course this allows dependent constructs to occur in new places, in non-template code in C++ (this was handled quite early), and also in C (this is why the "all languages" D89046 <https://reviews.llvm.org/D89046> came later).
>
> I'm surprised this only came up now! Ideally I think we'd use `DependentSizedArrayType` as the types of these variables, dependent types are supposed to work in C.

The dependent type should be able to work for C (we have extended clang to support dependence in C as `RecoveryExpr` heavily relies on clang's dependence mechanism). We're probably missing some places that need to handle dependent-type for C (like this case), we should fix it by making the C-only codepath properly support dependence.
That being said,  it is expected to see dependent types for C, we should not change the type in order to fix the crash.

> It seems OK to have mergeTypes() return QualType() for types that have errors in them - this will result in a followup diagnostic that ideally we'd drop, but we also emit that diagnostic in this case in C++. You could also make the case for returning the first type, or the type with errors, or the type without errors (i.e. *assume* that the types are the same after errors are fixed).

The `mergeTypes` is a widely-used method (for C and C++ code paths), and the "non-dependent-type" seems like an important invariant for this method. I'm nervous about removing it, I think it'd be better to keep it. Similar to other callers, we can make the caller (here it is `MergeVarDeclTypes`) guarantee this invariant.

An "easy" alternative, is to bail out if we see the `containsErrors` bit on the `Old` or `New` type in `MergeVarDeclTypes`. A bonus is that it can drop a not-quite-useful followup diagnostic (`redefinition of 'x' with a different type: 'char[((sizeof (<recovery-expr>(d))) == 8)]' vs 'char[((sizeof (<recovery-expr>(d)) == 8))]'`) for C++.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149612