Fix Nullable.GetUnderlyingType for MetadataLoadContext types#125356
Fix Nullable.GetUnderlyingType for MetadataLoadContext types#125356AaronRobinsonMSFT wants to merge 1 commit intodotnet:mainfrom
Conversation
|
Tagging subscribers to this area: @dotnet/area-system-reflection |
There was a problem hiding this comment.
Pull request overview
Fixes Nullable.GetUnderlyingType() returning null for Nullable<T> types loaded via MetadataLoadContext by adding a non-RuntimeType fallback that identifies System.Nullable\1` by name/namespace.
Changes:
- Update
Nullable.GetUnderlyingType(Type)to recognizeNullable<T>when the generic type definition is not aRuntimeType. - Add
MetadataLoadContext-based regression tests forNullable.GetUnderlyingType()(positive/negative/open generic cases). - Reference
System.Reflection.MetadataLoadContextfromSystem.Runtime.Teststo enable the new tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/Nullable.cs | Adds a name/namespace fallback for non-RuntimeType generic type definitions so MLC-loaded Nullable<T> is detected correctly. |
| src/libraries/System.Runtime/tests/System.Runtime.Tests/System/NullableTests.cs | Adds three new regression tests using MetadataLoadContext to validate the corrected behavior. |
| src/libraries/System.Runtime/tests/System.Runtime.Tests/System.Runtime.Tests.csproj | Adds a project reference to System.Reflection.MetadataLoadContext for the new tests. |
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/NullableTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/NullableTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Runtime/tests/System.Runtime.Tests/System/NullableTests.cs
Outdated
Show resolved
Hide resolved
Add a new public virtual Type.GetNullableUnderlyingType() method that returns the underlying type T for Nullable<T>, or null otherwise. Nullable.GetUnderlyingType() now forwards to this virtual method. This follows the same pattern as Enum.GetUnderlyingType() forwarding to Type.GetEnumUnderlyingType(), enabling Type subclasses like MetadataLoadContext's RoType to provide correct implementations. Changes: - Type.cs: New virtual with ReferenceEquals default (works for RuntimeType) - Nullable.cs: Forward GetUnderlyingType to the new virtual - RoType.cs: Override using CoreType.NullableT identity comparison - RuntimeType.Mono.cs: Update IsNullableOfT to use new virtual - System.Runtime.cs: Add API to ref assembly - NullableTests.cs: Tests for both RuntimeType and MLC paths All 24 NullableTests + 267 NullabilityInfoContextTests pass. Fixes dotnet#124216 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
586a9b8 to
90ed195
Compare
|
Updated this PR based on review feedback from @jkotas and @MichalStrehovsky. New approach: Added a public virtual API Proposal: #125388 Changes:
All 24 NullableTests + 267 NullabilityInfoContextTests pass. |
Summary
Nullable.GetUnderlyingType()returnsnullforNullable<T>types loaded viaMetadataLoadContextbecause it usesReferenceEquals(genericType, typeof(Nullable<>))to identify nullable types. MLC types areRoTypeinstances (notRuntimeType), so the reference check always fails.This adds a name-based fallback guarded by
is not RuntimeType, so only non-runtime types reach the string comparison.RuntimeTypekeeps the existing zero-overheadReferenceEqualsfast path.Bug Details
The
ReferenceEqualscheck on line 111 ofNullable.cscompares aRoType/EcmaDefinitionTypeagainsttypeof(Nullable<>)(aRuntimeType). These are always different object references, even though they represent the same logical type.Type.Equals()also does not work cross-context becauseRoType.Equals()checksobj is RoTypefirst, rejectingRuntimeType.This cascades into
NullabilityInfoContext, which callsGetUnderlyingType()at three callsites (lines 352, 565, 600) — causing completely wrong nullability analysis for MLC-loaded types:Nullable<int>is reported asNullabilityState.NotNullinstead ofNullableNullableAttribute.NullableFlagscorrupt results for all subsequent generic type arguments in types likeValueTuple<int, int?, string, string?>Fix
ReferenceEqualsshort-circuits; theis not RuntimeTypeguard ensures the name check is never reached for runtime types.Typesubclasses, etc.NullabilityInfoContextalready uses name-based matching for attribute detection.Tests
Added three new tests to
NullableTests.cs:GetUnderlyingType_MetadataLoadContext_NullableInt_ReturnsUnderlyingType— positive caseGetUnderlyingType_MetadataLoadContext_NonNullableTypes_ReturnsNull— negative cases (int, string, KeyValuePair)GetUnderlyingType_MetadataLoadContext_OpenNullable_ReturnsNull— open generic edge caseAll 21 NullableTests and 267 NullabilityInfoContextTests pass.
Fixes #124216