[Sema] avoid merge error type

[HerrCai0907]

Summary

fixed: https://github.com/llvm/llvm-project/issues/62447
C don't support DependentSizedArrayType, use ConstantArrayType with nullptr as SizeExpr

[shafik]

clang/lib/Sema/SemaDecl.cpp

4399

I wonder why it is not an InvalidDecl() as well? I don't see that we check both of these anywhere else.

[HerrCai0907]

in SemaType.cpp#BuildArrayType, It will generate DependentSizedArrayType which cannot be merged.
So we can either add additional check in MergeVarDeclTypes or directly ignore to generate this type in BuildArrayType.
Which one is better?

} else if (ArraySize->isTypeDependent() || ArraySize->isValueDependent()) {
  T = Context.getDependentSizedArrayType(T, ArraySize, ASM, Quals, Brackets);

[HerrCai0907]

fix in Sema::BuildArrayType

[shafik]

In D149612#4314209, @HerrCai0907 wrote:

in SemaType.cpp#BuildArrayType, It will generate DependentSizedArrayType which cannot be merged.
So we can either add additional check in MergeVarDeclTypes or directly ignore to generate this type in BuildArrayType.
Which one is better?

} else if (ArraySize->isTypeDependent() || ArraySize->isValueDependent()) {
  T = Context.getDependentSizedArrayType(T, ArraySize, ASM, Quals, Brackets);

I am not sure where the right place to fix this is, or when it is correct to use containsErrors()

Maybe @aaron.ballman or @rsmith might have some advice

[erichkeane]

In D149612#4314536, @shafik wrote:

In D149612#4314209, @HerrCai0907 wrote:

in SemaType.cpp#BuildArrayType, It will generate DependentSizedArrayType which cannot be merged.
So we can either add additional check in MergeVarDeclTypes or directly ignore to generate this type in BuildArrayType.
Which one is better?

} else if (ArraySize->isTypeDependent() || ArraySize->isValueDependent()) {
  T = Context.getDependentSizedArrayType(T, ArraySize, ASM, Quals, Brackets);

I am not sure where the right place to fix this is, or when it is correct to use containsErrors()

Maybe @aaron.ballman or @rsmith might have some advice

so containsErrors on an expression just means it has a RecoveryExpr in it. MUCH of the time a RecoveryExpr is just created with a Dependent type, thus the Dependent Array type. I would suggest rather than just returning 'no type' (via the empty qual type), if we find that the Array Size contains errors (I don't get the if CPlusPlus check?), we could either:
1- Make it an IncompleteArrayType.
2- Make it a ConstantArrayType, with a correctly-typed RecoveryExpr (that is, not dependent, still a size_t/etc expression, BUT with no value) for the bounds type. I would think it is better to do this ~2584 though.

[HerrCai0907]

Dependent type for CPP is a common case due to template. But for mergeTypes in C, dependent type cannot be handled correctly.

[HerrCai0907]

getConstantArrayType instal QualType()

[HerrCai0907]

use nullpt as SizeExpr

[erichkeane]

clang/lib/Sema/SemaType.cpp

2582 ↗

(On Diff #519675)

So I'm still not sure this is the 'right' away about it. I think we should instead properly handle the containsErrors case and just always create a non-dependent sized array, except with the RecoveryExpr having the correct type.

[HerrCai0907]

clang/lib/Sema/SemaType.cpp

2582 ↗

(On Diff #519675)

For CPP, dependent sized array is acceptable and necessary.

For C, I don't think SemaType is a correctly way to resolve TypoExpr, It should be done by ActOnXXX.

[erichkeane]

clang/lib/Sema/SemaType.cpp

2582 ↗

(On Diff #519675)

Why is the dependent array necessary for C++? I also don't get your C logic here... I think that Nullptr should be a RecoveryExpr of the proper type.

[HerrCai0907]

update

[HerrCai0907]

So I'm still not sure this is the 'right' away about it. I think we should instead properly handle the containsErrors case and just always create a non-dependent sized array, except with the RecoveryExpr having the correct type.

You are right. Fix as your suggestion.

[erichkeane]

clang/lib/Sema/SemaType.cpp

2583 ↗

(On Diff #519972)

Actually thinking further... rather than create a NEW RecoveryExpr, is there a problem KEEPING the ArraySize expression? It'd likely give more information/keep more AST consistency.

ALSO, creating this as size-0 has some implications we probably don't want. I wonder if a different for the placeholder would be better?

I'D ALSO suggest hoisting this ArraySize->containsError out of the else-if and into its own branch (rather than inside the dependent checks).

[HerrCai0907]

use 1 replace 0 as length

clang/lib/Sema/SemaType.cpp

2583 ↗

(On Diff #519972)

KEEPING the ArraySize expression

Agree.

creating this as size-0 has some implications we probably don't want.

Do you think we can modify it as 1? But I don't find a better way to identifier it as undef or other.

hoisting this ArraySize->containsError out of the else-if

Done

[erichkeane]

clang/lib/Sema/SemaType.cpp

2583 ↗

(On Diff #519972)

I don't have good evidence for what else we could do (whether an incomplete or variable type would be better), but at least 1 is 'legal', so perhaps we can live with this for now and see if there is any fallout.

What happens if we try to 'merge' a valid and an invalid here? Does it make the experience worse? So same example you added below, but the 1st one has no error in its brackets?

[HerrCai0907]

add more test case

[HerrCai0907]

add more test cases

[HerrCai0907]

clang/lib/Sema/SemaType.cpp

2583 ↗

(On Diff #519972)

I add test case for this scenario. I think it will not confuse developer.

[erichkeane]

@aaron.ballman : Can you comment, particularly on the 'x2' and 'x3' examples here? I think our hackery here to get the AST back in a reasonable position here is unfortunate, and leads to some pretty awkward errors. I'm not sure what else we could do?

We can't make it a dependent type, as it can then not be merged correctly.
We can't really make it a variable array type, since we would then have those types in places where it isn't 'legal'.
We would have a perhaps similar problem with the incomplete array type as the VLA, but at least we would diagnose char[], right?

My original thoughts were to make this still be a constant-array-type, keeping the 'error' size expression, but with a '1' for the extent, but I'm second guessing here based on those errors.

clang/test/Sema/merge-decls.c

101

Newline at end of file still needed.

[HerrCai0907]

Maybe return QualType() as a temporary solution to avoid crash? @erichkeane

[erichkeane]

In D149612#4343440, @HerrCai0907 wrote:

Maybe return QualType() as a temporary solution to avoid crash? @erichkeane

I'm not sure at the moment... I'd like to have aaron take a look and chat it over with him.

[aaron.ballman]

clang/lib/Sema/SemaType.cpp

2582 ↗

(On Diff #519675)

Why is the dependent array necessary for C++?

It's how we handle error recovery. I agree with @HerrCai0907 that we should keep the dependent-sized array in C++.

To me, the real question here is: what happens when a recovery expression is needed in order to form a type? Should we even *get* a recovery expression in this case? (And should the answer be different between C and C++?)

CC @hokein and @sammccall who were active on https://reviews.llvm.org/D89046 which introduced this issue. I'm not certain what design y'all were thinking of for this sort of situation.

clang/test/AST/ast-dump-types-errors.cpp

5 ↗

(On Diff #520808)

I don't think we should be changing the behavior in C++, this is a C-specific issue.

[sammccall]

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

[hokein]

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

[HerrCai0907]

revert untill first version

[erichkeane]

I think this is looking fine to me. Thanks for your patience on this, I know it was a tough review.

clang/lib/Sema/SemaDecl.cpp

4399

NIT: 80 line character limit per line.

[hokein]

thanks, looks good to me.

clang/test/Sema/merge-decls.c

96

It would be helpful to mention the related github issue), can you rename the x to something like test8_gh62447 ?

96

you can simplify the test further, char x[d.undef == 8] should be enough to trigger the crash.

[HerrCai0907]

simplify test case

clang/docs/ReleaseNotes.rst

 - Fix crash when member function contains invalid default argument.
   (`#62122 <https://github.com/llvm/llvm-project/issues/62122>`_)
 - Fix crash when handling undefined template partial specialization
   (`#61356 <https://github.com/llvm/llvm-project/issues/61356>`_)
 - Fix premature substitution into the constraints of an inherited constructor.
 - Fix crash when attempting to perform parenthesized initialization of an
   aggregate with a base class with only non-public constructors.
   (`#62296 <https://github.com/llvm/llvm-project/issues/62296>`_)
+- Fix crash when redefine variant with invalid type as another invalid type.
+  (`#62447 <https://github.com/llvm/llvm-project/issues/62447>`_)
 - Fix a stack overflow issue when evaluating ``consteval`` default arguments.
   (`#60082` <https://github.com/llvm/llvm-project/issues/60082>`_)
 - Fix the assertion hit when generating code for global variable initializer of
   _BitInt(1) type.
   (`#62207 <https://github.com/llvm/llvm-project/issues/62207>`_)
 - Fix lambdas and other anonymous function names not respecting ``-fdebug-prefix-map``
   (`#62192 <https://github.com/llvm/llvm-project/issues/62192>`_)
 - Fix crash when attempting to pass a non-pointer type as first argument of

clang/lib/Sema/SemaDecl.cpp

 /// scope as a previous declaration 'Old'.  Figure out how to merge their types,
 /// emitting diagnostics as appropriate.
 ///
 /// Declarations using the auto type specifier (C++ [decl.spec.auto]) call back
 /// to here in AddInitializerToDecl. We can't check them before the initializer
 /// is attached.
 void Sema::MergeVarDeclTypes(VarDecl *New, VarDecl *Old,
                              bool MergeTypeWithOld) {
-  if (New->isInvalidDecl() || Old->isInvalidDecl())
+  if (New->isInvalidDecl() || Old->isInvalidDecl() || New->getType()->containsErrors() || Old->getType()->containsErrors())
     return;
 
   QualType MergedT;
   if (getLangOpts().CPlusPlus) {
     if (New->getType()->isUndeducedType()) {
       // We don't know what the new type is until the initializer is attached.
       return;
     } else if (Context.hasSameType(New->getType(), Old->getType())) {

clang/test/Sema/merge-decls.c

 }
 
 void test7_f(int (*)[10]);
 void test7_f(int (*)[]); // expected-note {{passing argument to parameter here}}
 void test7_g() {
   int x[5];
   test7_f(&x); // expected-warning {{incompatible pointer types passing 'int (*)[5]' to parameter of type 'int (*)[10]}}
 }
+
+char d;
+char test8_gh62447[d.undef == 8]; // expected-error {{member reference base type 'char' is not a structure or union}}
+char test8_gh62447[d.undef == 4]; // expected-error {{member reference base type 'char' is not a structure or union}}