Repository navigation
Encode modified types in TypeBuilder signatures - #136450
caraioniurie47 wants to merge 6 commits into
Conversation
The runtime emitter's SignatureHelper treated a modified type (from GetModifiedParameterType, GetModifiedFieldType or Type.MakeModifiedSignatureType) as an ordinary type and asked the module for a token for it, so a TypeBuilder member signature containing one threw NotSupportedException. In a module's member, local and calli signatures, SignatureHelper now writes a modified type's custom modifiers and then the unmodified type, also for modified element and generic argument types. As in PersistedAssemblyBuilder, each kind is written in signature order (the reverse of the order reflection returns them), and a modifier array passed with a modified type replaces the type's own modifiers of that kind, its elements given in the order reflection returns. A modified type read from a signature and passed without modifier arrays keeps that signature's order of required and optional modifiers, read from the signature blob; otherwise required modifiers come first, as PersistedAssemblyBuilder writes them. The type's own modifiers get the checks the arrays have. Signatures without modified types and signatures without a module are encoded as before. Type signatures, method instantiations and type tokens, such as an ldtoken operand in a TypeBuilder method, drop a modified type's modifiers and take the unmodified type; a modified Int32, for example, threw NotSupportedException in each. Fix dotnet#136449 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dotnet-policy-service
Bot
added
the
community-contribution
|
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 |
There was a problem hiding this comment.
🔵 Needs a closer look
Changes to shared signature encoding, modifier ordering, and type-token semantics warrant maintainer review because they affect runtime method matching.
No open issues 🎉
What changed in this PR
This PR enables CoreCLR’s runtime Reflection.Emit implementation to encode modified types in member, local, and calli signatures, addressing failures when copying reflected signatures or using Type.MakeModifiedSignatureType.
Changes:
- Preserve custom modifiers, including their original ordering for signature-backed types and modifiers on nested types.
- Strip modifiers from type-token and method-instantiation paths while preserving existing behavior for signatures without a module.
- Add round-trip, interface-override, validation, and IL-operand tests, plus an IL fixture for interleaved modifier ordering.
No builds or tests were run during this review.
| File | Description |
|---|---|
| src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineProperty.cs | Tests property modifier round-tripping. |
| src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineMethod.cs | Tests modifier ordering, overrides, locals, replacement arrays, and IL operands. |
| src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineField.cs | Tests field modifiers and invalid modifier rejection. |
| src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineConstructor.cs | Tests constructor parameter modifiers. |
| src/libraries/System.Reflection.Emit/tests/TestModifierOrder/System.Reflection.Emit.TestModifierOrder.ilproj | Builds the modifier-order IL fixture. |
| src/libraries/System.Reflection.Emit/tests/TestModifierOrder/System.Reflection.Emit.TestModifierOrder.il | Defines interfaces with interleaved and nested modifiers. |
| src/libraries/System.Reflection.Emit/tests/System.Reflection.Emit.Tests.csproj | References the new IL fixture project. |
| src/libraries/System.Reflection.Emit.ILGeneration/tests/SignatureHelper/SignatureHelperAddArgument.cs | Tests continued rejection of modified types without a module. |
| src/coreclr/System.Private.CoreLib/src/System/RuntimeHandles.cs | Reads modifier kinds in signature order. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/ModifiedType.CoreCLR.cs | Exposes modifier ordering from the underlying signature. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/SignatureHelper.cs | Encodes modified types according to signature context. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeModuleBuilder.cs | Resolves modified type tokens through the unmodified type. |
Review effort: Balanced
SignatureHelper no longer has a flag for type and method instantiation signatures: GetTypeTokenWorkerNoLock takes a modified type's unmodified type, and a method instantiation writes its type arguments as a member signature does. A modified type used to construct another type now keeps its modifiers in that type's token, as in a member signature, so a member signature matches it only with the same modified type. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The comments this change adds now give the reason the code can't show: why modifier order is kept, when the counts differ, why an element's or type argument's modifiers stay wrapped, and where the modifier tokens come from. Comments that restated the code are rewritten or cut. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| AddElementType(CorElementType.ELEMENT_TYPE_CMOD_REQD); | ||
| // The counts differ when ELEMENT_TYPE_CMOD_INTERNAL or ELEMENT_TYPE_SENTINEL sits among the modifiers: | ||
| // the kinds stop there, while reflection counts the first and steps over the second. | ||
| bool[]? kinds = (type as ModifiedType)?.GetCustomModifierKinds(); |
There was a problem hiding this comment.
I do not think we want to be reaching into ModifiedType internal details to fix the issue with modopt/modreq ordering. In cases like this, PersistedAssemblyBuilder and in-memory AssemblyBuilder should have same behavior.
PersistedAssemblyBuilder would not be even able to reach into ModifiedType internal details like this.
If we wanted to fix the modopt/modreq ordering, it would require a new reflection API on System.Type.
There was a problem hiding this comment.
Agreed. The next push removes ModifiedType.GetCustomModifierKinds, Signature.GetCustomModifierKindsAtOffset and the tests that depend on them, so a modified type's required modifiers are written first, then its optional ones, each kind in signature order, as PersistedAssemblyBuilder writes them. A signature with an optional modifier before a required one at the same position comes out reordered; the description will say so.
The DispatchProxy switch to GetModifiedParameterType() discussed in #135567 loses a case with this, as I noted there; the switch follows this PR. On CoreCLR, DispatchProxy.Create would then handle modifiers nested in arrays, by-refs and generic arguments, and a position's required modifiers before its optional ones, but throw TypeLoadException for the reverse order, which #135567 handles today; an interleaved order fails either way. I'd go with that and state it in the switch's PR. Is that acceptable, or should parameter and return types without nested modifiers stay as in #135567?
There was a problem hiding this comment.
On CoreCLR, #135567 passes the modifiers to DefineMethod as arrays, and DefineMethod writes a position's optional modifiers before its required ones in the signature. So a proxy for an interface whose signature has that order is created, and one with required before optional throws TypeLoadException. I mentioned #135567 because moving DispatchProxy to GetModifiedParameterType(), as discussed there, would replace that code. Required modifiers would then be written first, as PersistedAssemblyBuilder writes them, so on CoreCLR an interface with required before optional in the signature would get a proxy, and one with optional before required would throw.
There was a problem hiding this comment.
Can you please submit PR to fix this mismatch between PersistedAssemblyBuilder and RuntimeAssemblyBuilder? We want both to use the same order.
There was a problem hiding this comment.
I agree that 1 makes most sense. Note that there are other places in the repo where Array.Reverse will need to be deleted (\src\libraries\System.ComponentModel.Composition\src\System\ComponentModel\Composition\MetadataViewGenerator.cs).
It is technically a breaking change, but I would not worry about it much - C# compiler does not generate signatures that would be affected by it.
There was a problem hiding this comment.
Opened #136699: CoreCLR's AssemblyBuilder now writes custom modifiers in PersistedAssemblyBuilder's order, and DispatchProxy and MEF's MetadataViewGenerator no longer reverse the arrays they read from reflection. This PR will be rebased on it if it merges first.
There was a problem hiding this comment.
@caraioniurie47 We can continue with this one now that #136699 is merged
There was a problem hiding this comment.
Merged main (no force push to an open PR). A modified type's modifiers now go through AddCustomModifiers from #136699, so they are written as the custom modifier arrays are, required ones first; this PR's own copies of that helper are gone. As said above, SignatureHelper no longer reads the modifier order from the signature a modified type came from, and the tests that relied on it are removed. DefineMethod_ModifiedParameterTypes_CopiesSignature defines its source method with modifier arrays, so its three assertions now expect the arrays' order. A new test covers a modified List<int> from Type.MakeModifiedSignatureType, which CoreCLR wrote without its modifiers on .NET 11 RC1.
Two questions. AddDynamicCustomModifiers still throws the old messages that AddCustomModifiers now replaces; change it too? And both builders write a constructed generic modifier such as List<int> as a TypeSpec, which Ecma-335-Augments.md disallows; a separate issue for that?
Tests ran on Windows 11 x64, Release.
There was a problem hiding this comment.
And both builders write a constructed generic modifier such as List as a TypeSpec, which Ecma-335-Augments.md disallows; a separate issue for that?
In general, Reflection.Emit does not prevent you from generating invalid IL or metadata. There are many ways to generate invalid IL or metadata data that will cause crash at runtime using Reflection.Emit. So these checks do not form a coherent story. I would not mind dropping them, or at least keeping them as simple as possible (e.g. using one generic message for all sorts of invalid modifier types).
SignatureHelper no longer reads the order of a modified type's required and optional modifiers from the signature it was read from: no System.Type API gives that order, and PersistedAssemblyBuilder can't read it. A modified type's required modifiers come first, then its optional ones, as PersistedAssemblyBuilder writes them, so an optional modifier before a required one at the same position comes out reordered. The custom modifier checks, which this PR also runs on a modified type's own modifiers, called a by-ref modifier an array or pointer type and a generic parameter a generic type, and named neither. Their messages now name the modifier and say what is wrong with it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Conflict in SignatureHelper.cs with dotnet#136699, resolved by keeping its AddCustomModifiers and dropping this PR's AddReflectedCustomModifiers and AddCustomModifier, which did the same. A modified type's own modifiers now go through AddCustomModifiers, whose messages name the modifier and whose paramName may be null for them. The comments in the resolved code no longer give PersistedAssemblyBuilder as the reason. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
DefineMethod_ModifiedParameterTypes_CopiesSignature defines its source method with modifier arrays. Since dotnet#136699, DefineMethod takes each array in the order reflection returns it, so the test now expects the source's and the copy's modifiers in the arrays' order. A modified List<int> from Type.MakeModifiedSignatureType was written without its modifiers on .NET 11 RC1. A test of its own now checks that its modifier comes back, and fails without this PR. The constructed-generic ldtoken test now says what its first-byte check shows. The comment on the worker call now says that the other overload would also write modifiers an array replaced, and a comment sentence that restated the code is gone. The worker calls name lastWasGenericInst, and the module check uses "is not null", as the repository's C# guidelines ask. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
| private void AddOneArgTypeHelper(Type clsArgument, DynamicScope? scope = null) { AddOneArgTypeHelperWorker(clsArgument, false, scope); } | ||
| // Modifier tokens come from m_module, which a DynamicMethod's signature doesn't have. | ||
| private bool IsEncodedModifiedType(Type type) => | ||
| m_module is not null && type is ModifiedType or SignatureModifiedType; |
There was a problem hiding this comment.
SignatureHelper is not limited runtime types. Can a modified type that came from System.Reflection.MetadataLoadContext show up here? If it can, this condition won't work correctly.
With CoreCLR's runtime
AssemblyBuilderon .NET 11 RC1, aTypeBuildermember signature containing a modifiedint, fromGetModifiedFieldType()orType.MakeModifiedSignatureType, threwNotSupportedException: for a method atCreateType, and fromDefineConstructor,DefineFieldandDefineProperty. A modifiedList<int>fromType.MakeModifiedSignatureTypewas accepted and written without its modifiers; it now keeps them.In a module's member, local and
callisignatures,SignatureHelpernow writes a modified type's custom modifiers and then the unmodified type, also for modified element and generic argument types. The modifiers are written as the custom modifier arrays are, required ones first, so a signature with an optional modifier before a required one at the same position comes out reordered. A modifier array passed with a modified type replaces the type's own modifiers of that kind. The custom modifier checks now also run on a modified type's own modifiers, and their messages for an array, pointer, by-ref or generic modifier name it.Type tokens, such as
ldtokenoperands, take a modified type's unmodified type. A modified type used to construct another type, such astypeof(IEquatable<>).MakeGenericType(modifiedType), keeps its modifiers in that type's token, as in a member signature.Signatures without modified types, and signatures built without a module such as a
DynamicMethod'sEmitCallisignature, are encoded as before.The new tests are in
TypeBuilderDefine*.csandSignatureHelperAddArgument.cs. TheTypeBuildertests are markedSkipOnMono: on aModifiedTypeparameter, Mono'sCreateTypeends the process (#135539).Without the change, the new
TypeBuildertests fail withNotSupportedException, except the constructed-genericldtokenand null-entry tests, which pass either way, and theList<int>test, which fails on the dropped modifier; the no-module test passes too.Benchmark of existing signature paths with unmodified types:
SignatureHelper.GetMethodSigHelper(module, typeof(int), parameterTypes).GetSignature(), and the same withAddArgumentsand four modifiers.fixis this PR's first version,baseit withSignatureHelper.csfrom 055fefa; the code now makes the same type checks per argument, or fewer when modifier arrays are passed. Mean ± StdDev in µs, runs in the order shown; allocations are equal:MethodSignatureMethodSignatureWithModifierArraysEvery gap between a fix run's mean and a base run's is smaller than each run's StdDev.
Resolves #136449
Note
AI-generated, written at my direction and reviewed by me before posting.
Tests ran on Windows 11 x64, Release, where an ILLink trim build also passed; the benchmark ran on Ubuntu 24.04.