Do not generate unconstructed EEType for arrays that have a template #113951
Conversation
When `CanonFormTypeMayExist` was written, it was okay for arrays to return false since arrays were always compared structurally (it was allowed to have multiple `MethodTable`s for a single array type). We stopped allowing this some time ago and we now have to maintain the same invariant as for any other `MethodTable` - the address of the `MethodTable` is the unique identity. We cannot allow the compiler to generate unconstructed `MethodTable` for something that can be template-constructed at runtime.
There was a problem hiding this comment.
Pull Request Overview
This PR addresses issue #113750 by ensuring that unconstructed EETypes for arrays with a template are not generated, maintaining the invariant that a MethodTable's address uniquely identifies it. Key changes include adding a new regression test in the SmokeTests to cover this scenario and modifying the EETypeNode logic to correctly handle array types.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/tests/nativeaot/SmokeTests/Reflection/Reflection.cs | Adds the Test113750Regression to validate array behavior under the new invariant |
| src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/DependencyAnalysis/EETypeNode.cs | Replaces a generic instantiation check with a specific array type check to enforce the invariant |
Comments suppressed due to low confidence (1)
src/tests/nativeaot/SmokeTests/Reflection/Reflection.cs:1217
- Throwing a plain Exception without a message can make debugging harder. Consider including a descriptive message to clarify the failure condition.
if (!(arr is Atom[]))
| class Atom; | ||
|
|
There was a problem hiding this comment.
The declaration 'class Atom;' is incomplete in C# and may cause a compilation error. It should be defined with a body, e.g., 'class Atom { }'.
| class Atom; | |
| class Atom { } |
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
| @@ -253,7 +253,7 @@ private bool CanonFormTypeMayExist | |||
| { | |||
| get | |||
| { | |||
| if (!_type.HasInstantiation) | |||
| if (_type.IsArrayTypeWithoutGenericInterfaces()) | |||
There was a problem hiding this comment.
Consider adding a clarifying comment here to explain why array types without generic interfaces should immediately return false, aiding future maintenance.
Copilot is powered by AI, so mistakes are possible. Review output carefully before use.
|
/ba-g timeouts in unrelated leg |
Fixes #113750
When
CanonFormTypeMayExistwas written, it was okay for arrays to return false since arrays were always compared structurally (it was allowed to have multipleMethodTables for a single array type).We stopped allowing this some time ago and we now have to maintain the same invariant as for any other
MethodTable- the address of theMethodTableis the unique identity. We cannot allow the compiler to generate unconstructedMethodTablefor something that can be template-constructed at runtime.Cc @dotnet/ilc-contrib