Remove nullability annotation from MetadataLoadContext.CoreAssembly.#126142
Remove nullability annotation from MetadataLoadContext.CoreAssembly.#126142teo-tsirpanis wants to merge 4 commits intodotnet:mainfrom
MetadataLoadContext.CoreAssembly.#126142Conversation
dotnet-policy-service
bot
added
the
community-contribution
There was a problem hiding this comment.
Pull request overview
This PR updates MetadataLoadContext.CoreAssembly to be non-nullable by making core-assembly resolution an up-front, throwing operation, and refactors related initialization to reflect that invariant.
Changes:
- Change
CoreAssemblyfromAssembly?toAssembly(including the ref assembly). - Resolve and store the core assembly during
MetadataLoadContextconstruction via a newLoadCoreAssemblyhelper. - Refactor
CoreTypesinitialization to accept a resolvedRoAssemblydirectly.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/TypeLoading/General/CoreTypes.cs |
Refactors CoreTypes ctor to take a resolved core RoAssembly and simplifies initialization loop. |
src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/MetadataLoadContext.CoreAssembly.cs |
Replaces TryGetCoreAssembly with LoadCoreAssembly and changes _coreAssembly to non-nullable readonly. |
src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/MetadataLoadContext.Apis.cs |
Loads core assembly up-front in the constructor and updates CoreAssembly property signature to non-nullable. |
src/libraries/System.Reflection.MetadataLoadContext/ref/System.Reflection.MetadataLoadContext.cs |
Updates public API surface for CoreAssembly nullability in the reference assembly. |
Comments suppressed due to low confidence (1)
src/libraries/System.Reflection.MetadataLoadContext/src/System/Reflection/MetadataLoadContext.Apis.cs:248
- The XML doc comment for
CoreAssemblystill states that the value may be null and that the core assembly is loaded lazily. With the property now non-nullable and the constructor resolving the core assembly up-front (throwing if it can’t be found), the docs should be updated to match the actual behavior/contract.
public Assembly CoreAssembly
{
get
{
if (IsDisposed)
teo-tsirpanis
added
area-System.Reflection
and removed
area-System.Reflection.Metadata
labels
|
Tagging subscribers to this area: @dotnet/area-system-reflection |
|
Could you please address copilot feedback? |
teo-tsirpanis
force-pushed
the
mlc-core-assembly-nullability
branch
from
c07808a to
cde9080
Compare
| /// If a core assembly cannot be found or if the core assembly is missing types, this will affect the behavior of the MetadataLoadContext as follows: | ||
| /// |
There was a problem hiding this comment.
The XML docs still describe behavior when a core assembly cannot be found, but MetadataLoadContext now resolves the core assembly during construction and throws if it can’t be resolved. Please update this section to reflect that failure to resolve the core assembly results in an exception at construction time, and keep the remaining guidance focused on the “core assembly missing types” case (which can still happen).
| /// The CoreAssembly is determined by passing the coreAssemblyName parameter passed to the MetadataAssemblyResolver constructor | ||
| /// to the MetadataAssemblyResolver's Resolve method. |
There was a problem hiding this comment.
The docs say the coreAssemblyName parameter is “passed to the MetadataAssemblyResolver constructor”, but coreAssemblyName is a MetadataLoadContext constructor parameter and is what gets passed to MetadataAssemblyResolver.Resolve. Please correct this wording to avoid confusing API consumers.
| internal RoAssembly LoadCoreAssembly(string? coreAssemblyName) | ||
| { | ||
| Debug.Assert(_coreAssembly == null); | ||
| RoAssembly? coreAssembly; |
There was a problem hiding this comment.
LoadCoreAssembly no longer uses Debug/System.Diagnostics in this file. Please remove the now-unused using System.Diagnostics; directive to avoid CS8019 warnings (which may be treated as errors in this repo’s build settings).
|
More copilot nits |
This property will never be null, because failing to resolve the core assembly would throw when creating the MLC. We update the property's nullability annotation, and simplify code flow to make this evident.