Conversation
Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
|
@copilot Please check and fix implementation of all public APIs from System.Reflection namespace for the same issue. |
.../System.Reflection.MetadataLoadContext/tests/src/Tests/Type/TypeTests.GetGenericArguments.cs
Outdated
Show resolved
Hide resolved
…urn type fixes Co-authored-by: jkotas <6668460+jkotas@users.noreply.github.com>
Done! Comprehensive search found and fixed 4 additional methods:
Added tests for function pointer types. All 686 tests pass. Commit: 18b64cd |
|
@copilot please run the code review skill |
|
Tagging subscribers to this area: @dotnet/area-system-reflection |
…w feedback Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
Code review completed. The changes are minimal and correct - they add explicit |
|
@copilot please invoke the code-review skill and post your analysis/results as comment on this PR |
🤖 Copilot Code ReviewHolistic AssessmentMotivation: The PR addresses a real API contract violation where Approach: The fix adds explicit Summary: ✅ LGTM. Minimal, correct, well-tested fix that follows established patterns. Detailed Findings✅ Correctness — All three source changes are correct
✅ Cross-cutting analysis — No remaining instancesSearched all ✅ Test quality — Integrated into existing invariant infrastructureThe test changes are well-placed in 💡 Observation — Two changes are defensive, not bug fixesThe changes to |
There was a problem hiding this comment.
Pull request overview
Fixes MetadataLoadContext System.Reflection APIs that return Type[] to ensure they physically return Type[] (not internal RoType[] / RoModifiedType[] arrays), preventing ArrayTypeMismatchException when consumers use span-based APIs like AsSpan().
Changes:
- Updated several
Type[]-returning APIs to useCloneArray<Type>()/CloneArray<Type>(...)so the returned array instance is actuallyType[]. - Added invariants in the existing type test infrastructure to validate returned array runtime types are
Type[](including function-pointer-related APIs when applicable).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/System.Reflection.MetadataLoadContext/tests/src/Tests/Type/TypeInvariants.cs | Adds assertions that various APIs return arrays with runtime type Type[]. |
| src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/Types/RoType.cs | Forces GetGenericArguments() to clone into a Type[] array. |
| src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/Types/RoModifiedType.cs | Ensures modifier arrays are cloned with an explicit Type element type. |
| src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/Types/RoModifiedFunctionPointerType.cs | Forces function-pointer parameter/calling-convention arrays to clone into Type[]. |
src/libraries/System.Reflection.MetadataLoadContext/tests/src/Tests/Type/TypeInvariants.cs
Show resolved
Hide resolved
…Tests/Type/TypeInvariants.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
/ba-g known pkg_resources issue |
Description
Multiple methods in MetadataLoadContext return internal implementation arrays (
RoType[],RoModifiedType[]) instead ofType[], violating the API contract and causingArrayTypeMismatchExceptionwhen using modern APIs likeAsSpan<Type>().For example, calling
GetGenericArguments()on a type:Changes
Fixed 5 methods in 3 files to explicitly specify
<Type>inCloneArraycalls:RoType.cs:
GetGenericArguments()-.CloneArray()→.CloneArray<Type>()RoModifiedFunctionPointerType.cs:
GetFunctionPointerParameterTypes()-.CloneArray()→.CloneArray<Type>()GetFunctionPointerCallingConventions()-.CloneArray()→.CloneArray<Type>()RoModifiedType.cs:
GetRequiredCustomModifiers()-.CloneArray()→.CloneArray<Type>()GetOptionalCustomModifiers()-.CloneArray()→.CloneArray<Type>()Tests: Integrated array type verification checks into existing
TestTypeCommonInvariantsmethod inTypeInvariants.cs. The checks verify thatGenericTypeArguments,GenericTypeParameters,GetGenericArguments(),GetInterfaces(),GetFunctionPointerParameterTypes(), andGetFunctionPointerCallingConventions()all returnType[]arrays. These checks run comprehensively across all type variants (generic types, function pointers, constructed types, etc.) as part of the existing test infrastructure.This aligns with the pattern already used in 5+ other methods in the codebase:
GenericTypeParameters,GenericTypeArguments,GetInterfaces(),RoMethod.GetGenericArguments(), andGetGenericParameterConstraints().All public APIs in System.Reflection namespace returning
Type[]have been systematically reviewed and fixed.Customer Impact
Code using modern span-based APIs with MetadataLoadContext types fails at runtime with array covariance exceptions.
Testing
TestTypeCommonInvariantsensure correct array types across all type variantsRisk
Minimal. Changes only the array wrapper type, not the element contents or behavior. Follows established pattern already used throughout the codebase.
Original prompt
This section details on the original issue you should resolve
<issue_title>MetadataLoadContext: Type.GetGenericArguments() returns RoType[] instead of Type[]</issue_title>
<issue_description>### Description
In
MetadataLoadContext, callingGetGenericArguments()on aTypeobject returns an array instance of typeRoType[](an internal implementation type) rather than a standardType[].This causes issues in code that expects the returned array to be a strict
Type[].For example, as shown in the reproduction code below, calling
AsSpan()on the result ofGetGenericArguments()throws anArrayTypeMismatchException.This occurs because
AsSpan<Type>()cannot be called on an array that is physicallyRoType[], even thoughRoTypeinherits fromType, due to array covariance limitations with spans.Reproduction Steps
As shown in the "Actual behavior" section, this code throws an
ArrayTypeMismatchExceptionwhen callingAsSpan().Expected behavior
Type.GetGenericArguments()should return aType[], and callingAsSpan()should succeed without throwing an exception.Actual behavior
Type.GetGenericArguments()returnsRoType[], which leads to anArrayTypeMismatchExceptionwhenAsSpan()is invoked.Regression?
No response
Known Workarounds
If a strict
Type[]is required, the array must be re-created usingCast<Type>().ToArray()or similar methods:Configuration
csproj:
Other information
In the current implementation of
RoType.GetGenericArguments(), the method returns a copy of the array usingCloneArray():runtime/src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/Types/RoType.cs
Line 103 in b7db56a
Specifying
Typeas the explicit generic argument forCloneArrayshould ensure the resulting array is created asType[]instead of preserving the originalRoType[]type.This change may resolve the issue (I haven't actually verified if it works):
</issue_description>
<agent_instructions>Fix the reported issues.
Also, check implementations of all public APIs in System.Reflection namespace for this issue.</agent_instructions>
Comments on the Issue (you are @copilot in this section)
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.