Skip to content

Fix Nullable.GetUnderlyingType for MetadataLoadContext types#125356

Closed
AaronRobinsonMSFT wants to merge 1 commit intodotnet:mainfrom
AaronRobinsonMSFT:fix/124216-nullable-getunderlyingtype
Closed

Fix Nullable.GetUnderlyingType for MetadataLoadContext types#125356
AaronRobinsonMSFT wants to merge 1 commit intodotnet:mainfrom
AaronRobinsonMSFT:fix/124216-nullable-getunderlyingtype

Conversation

@AaronRobinsonMSFT
Copy link
Copy Markdown
Member

Summary

Nullable.GetUnderlyingType() returns null for Nullable<T> types loaded via MetadataLoadContext because it uses ReferenceEquals(genericType, typeof(Nullable<>)) to identify nullable types. MLC types are RoType instances (not RuntimeType), 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. RuntimeType keeps the existing zero-overhead ReferenceEquals fast path.

Bug Details

The ReferenceEquals check on line 111 of Nullable.cs compares a RoType/EcmaDefinitionType against typeof(Nullable<>) (a RuntimeType). These are always different object references, even though they represent the same logical type. Type.Equals() also does not work cross-context because RoType.Equals() checks obj is RoType first, rejecting RuntimeType.

This cascades into NullabilityInfoContext, which calls GetUnderlyingType() at three callsites (lines 352, 565, 600) — causing completely wrong nullability analysis for MLC-loaded types:

  • Nullable<int> is reported as NullabilityState.NotNull instead of Nullable
  • Index mismatches in NullableAttribute.NullableFlags corrupt results for all subsequent generic type arguments in types like ValueTuple<int, int?, string, string?>

Fix

if (ReferenceEquals(genericType, typeof(Nullable<>))
    || (genericType is not RuntimeType
        && genericType.Namespace == "System"
        && genericType.Name == "Nullable`1"))
  • Zero overhead for RuntimeTypeReferenceEquals short-circuits; the is not RuntimeType guard ensures the name check is never reached for runtime types.
  • Correct for all Type implementations — MLC types, custom Type subclasses, etc.
  • Proven patternNullabilityInfoContext already uses name-based matching for attribute detection.

Tests

Added three new tests to NullableTests.cs:

  • GetUnderlyingType_MetadataLoadContext_NullableInt_ReturnsUnderlyingType — positive case
  • GetUnderlyingType_MetadataLoadContext_NonNullableTypes_ReturnsNull — negative cases (int, string, KeyValuePair)
  • GetUnderlyingType_MetadataLoadContext_OpenNullable_ReturnsNull — open generic edge case

All 21 NullableTests and 267 NullabilityInfoContextTests pass.

Fixes #124216

@dotnet-policy-service
Copy link
Copy Markdown
Contributor

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Comment thread src/libraries/System.Private.CoreLib/src/System/Nullable.cs Outdated
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 recognize Nullable<T> when the generic type definition is not a RuntimeType.
  • Add MetadataLoadContext-based regression tests for Nullable.GetUnderlyingType() (positive/negative/open generic cases).
  • Reference System.Reflection.MetadataLoadContext from System.Runtime.Tests to 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.

Comment thread src/libraries/System.Runtime/tests/System.Runtime.Tests/System/NullableTests.cs Outdated
Comment thread src/libraries/System.Runtime/tests/System.Runtime.Tests/System/NullableTests.cs Outdated
Comment thread src/libraries/System.Runtime/tests/System.Runtime.Tests/System/NullableTests.cs Outdated
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>
@AaronRobinsonMSFT
Copy link
Copy Markdown
Member Author

Updated this PR based on review feedback from @jkotas and @MichalStrehovsky.

New approach: Added a public virtual Type.GetNullableUnderlyingType() method following the Enum.GetUnderlyingType()Type.GetEnumUnderlyingType() pattern. Nullable.GetUnderlyingType() now forwards to this virtual.

API Proposal: #125388

Changes:

  • Type.cs — New virtual Type? GetNullableUnderlyingType() with ReferenceEquals default
  • Nullable.csGetUnderlyingType forwards to the new virtual
  • RoType.cssealed override using CoreType.NullableT identity comparison
  • RuntimeType.Mono.csIsNullableOfT updated to use new virtual
  • System.Runtime.cs — API added to ref assembly
  • NullableTests.cs — Tests for both RuntimeType and MLC paths, ConditionalFact added

All 24 NullableTests + 267 NullabilityInfoContextTests pass.

@AaronRobinsonMSFT AaronRobinsonMSFT marked this pull request as draft March 10, 2026 17:47
@dotnet-policy-service dotnet-policy-service Bot removed this from the 11.0.0 milestone Apr 9, 2026
@dotnet-policy-service
Copy link
Copy Markdown
Contributor

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

MetadataLoadContext: Nullable.GetUnderlyingType() always returns null

4 participants