Repository navigation
Implement IsByRefLike on the remaining Reflection.Emit Type subclasses - #133819
Conversation
SymbolType and TypeBuilderInstantiation did not override
Type.IsByRefLike, so reading it on a type returned by MakeArrayType,
MakePointerType, MakeByRefType or MakeGenericType on a TypeBuilder
threw NotSupportedException ("Derived classes must provide an
implementation"). dotnet#34846 added the override to the builder classes
themselves but not to these internal types.
SymbolType represents an array, pointer or byref type, which is never
by-ref-like, so it returns false. TypeBuilderInstantiation forwards to
its generic type definition: that is a builder, which returns false,
or a runtime type when a runtime generic type is instantiated over a
builder, as in typeof(Span<>).MakeGenericType(typeBuilder), which is
by-ref-like.
Fix dotnet#91532
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
TypeBuilderImpl and GenericTypeParameterBuilderImpl, the builders returned by PersistedAssemblyBuilder, also threw NotSupportedException from Type.IsByRefLike. Return false, matching RuntimeTypeBuilder, RuntimeGenericTypeParameterBuilder and EnumBuilderImpl. Contributes to dotnet#91532 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @steveisok, @dotnet/area-system-reflection |
|
@dotnet-policy-service agree |
|
Azure Pipelines: Successfully started running 3 pipeline(s). 13 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
CI: none of the failed tests is in Reflection.Emit, and
Note AI-generated, written at my direction and reviewed by me before posting. The results are from Azure DevOps build 1594658 on commit e327094, and from Azure DevOps test results and test history for other dotnet/runtime PR builds, read through the Azure DevOps and Helix APIs. |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The implementations match established sibling-type behavior and the tests cover all affected subclasses.
Review effort: Balanced
Findings: None
What changed in this PR
Completes Type.IsByRefLike support across remaining Reflection.Emit type implementations, replacing inherited exceptions with consistent values.
Changes:
- Returns
falsefor builder, generic-parameter, and compound symbol types. - Forwards constructed generic types to their generic definition.
- Adds regression coverage for runtime and persisted builders.
| File | Description |
|---|---|
TypeBuilderIsByRefLike.cs |
Tests runtime builder-derived types. |
AssemblySaveTypeBuilderAPIsTests.cs |
Tests persisted builder types. |
TypeBuilderImpl.cs |
Returns false for persisted builders. |
GenericTypeParameterBuilderImpl.cs |
Returns false for persisted generic parameters. |
TypeBuilderInstantiation.cs |
Forwards to the generic definition. |
SymbolType.cs |
Returns false for compound types. |
Fixes #91532
Type.IsByRefLikethrewNotSupportedException: Derived classes must provide an implementation.for four Reflection.Emit types. #34846 added the override to the runtime builder classes.IsByRefLikenowSymbolTypeMakeArrayType,MakePointerType,MakeByRefTypeon aTypeBuilderfalseTypeBuilderInstantiationMakeGenericTypeon aTypeBuilder, orMakeGenericTypeon a runtime generic type with a builder argumentTypeBuilderImplDefineTypeon aPersistedAssemblyBuildermodulefalseGenericTypeParameterBuilderImplDefineGenericParameterson aPersistedAssemblyBuildertypefalseTypeBuilderInstantiationforwards instead of returningfalse. A runtime generic definition instantiated over a builder also produces one (RuntimeType.CoreCLR.cs:3600,RuntimeType.Mono.cs:1460), sotypeof(Span<>).MakeGenericType(typeBuilder)is aTypeBuilderInstantiationoverSpan<>and is by-ref-like. Forwarding matchesSignatureConstructedGenericType.IsByRefLike. When the definition is itself a builder, the result isfalse.The persisted builder types are a separate commit. In the issue, buyaa-n suggested leaving
TypeBuilderImplandGenericTypeParameterBuilderImplout as incomplete and not yet public.PersistedAssemblyBuilderis public now and both throw the same way, so they are included; if they should still stay out, the second commit can be dropped, thoughMakeGenericTypeon a persistedTypeBuilderthen still throws, becauseTypeBuilderInstantiationforwards to it.The observable change is that these getters return a value where they threw
NotSupportedException.Tests
New cases in
TypeBuilderIsByRefLike.cs(runtime builders) andAssemblySaveTypeBuilderAPIsTests.cs(persisted builders) cover the persisted builder and its generic parameter,MakeGenericTypeon a builder, andMakeArrayType,MakePointerTypeandMakeByRefTypeon a builder and its generic parameter, plusSpan<>,ReadOnlySpan<>andList<>instantiated over a builder. The 8 new test cases failed before the fix and pass with it.Run locally on Windows x64:
System.Reflection.Emit.Tests: all pass on aclr+libs -rc checkedbuild and on aclr+libs -rc release -lc releasebuild.System.Linq.Expressions.Tests(Release runtime only): all pass.IsByRefLike, among themSystem.Runtime.Tests,System.Reflection.Tests,System.Reflection.Emit.ILGeneration.Tests,System.Reflection.DispatchProxy.TestsandSystem.Reflection.MetadataLoadContext.Tests: all pass exceptXmlSystemPathResolverTests.TestResolveInvalidPathinSystem.Private.Xml.Tests, which needsnotfound.invalid.corp.microsoft.comnot to resolve, and my network's DNS returns an address for it.Not run locally: Mono and NativeAOT.
Not in this PR
System.Reflection.Context has the same gap: types mapped by a
CustomReflectionContextthrow fromIsByRefLikeand three otherTypemembers. That is #133816.Note
AI-generated, written at my direction and reviewed by me before posting. The test results are from local builds of this branch on Windows x64, on the configurations named above; that the new tests fail without the fix was checked on a checked build without the product change.