Support using signature types in reflection emit. - #132115
Conversation
|
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-emit |
There was a problem hiding this comment.
Pull request overview
This PR expands SignatureType support in CoreLib/Reflection.Emit so that Reflection.Emit can accept signature types (including those produced by Type.MakeGenericSignatureType) in more places, enabling mixed-reflection-stack scenarios (e.g., MetadataLoadContext + Reflection.Emit). It also aligns attribute reporting for signature types (notably Type.Attributes) and adds/updates tests to cover the new behavior.
Changes:
- Implement
SignatureType.GetAttributeFlagsImpl()across signature-type flavors soType.Attributesis supported (constructed generics mirror their generic definition; arrays return standard array flags; other signature types returnPublic). - Relax/extend Reflection.Emit helpers to accept signature types (e.g.,
TypeBuilder.GetMethod/GetField/GetConstructor,RuntimeTypeBuilderdefault-ctor path) and updateTypeBuilderImpl.IsValueTypeImplfor signature base types. - Add/update regression tests for signature-type generic parents and mixed-source generic bases; adjust some existing tests accordingly.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Reflection/SignatureTypes.cs | Adds assertion validating Attributes behavior for signature constructed generic types. |
| src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderGetMethod.cs | Removes a throw-validation test around TypeBuilder.GetMethod input type restrictions. |
| src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderGetField.cs | Removes a throw-validation test around TypeBuilder.GetField input type restrictions. |
| src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderGetConstructor.cs | Removes a throw-validation test around TypeBuilder.GetConstructor input type restrictions. |
| src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderDefineDefaultConstructor.cs | Extends default-constructor test to cover signature-type vs non-signature-type generic parent. |
| src/libraries/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveTypeBuilderTests.cs | Adds regression test covering SetParent with a base generic type sourced from MetadataLoadContext via signature types. |
| src/libraries/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveILGeneratorTests.cs | Uses MakeGenericSignatureType to express nested generic return type involving a TypeBuilder argument. |
| src/libraries/System.Reflection.Emit/src/System/Reflection/Emit/TypeBuilderImpl.cs | Updates IsValueTypeImpl to safely handle signature base types; adjusts AreTypesEqual nullability. |
| src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureType.cs | Makes GetAttributeFlagsImpl abstract so signature types can support Type.Attributes. |
| src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureModifiedType.cs | Implements attribute flags for modified signature types. |
| src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureHasElementType.cs | Implements attribute flags for pointer/byref signature types (left overridable for arrays). |
| src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureGenericParameterType.cs | Implements attribute flags for signature generic parameter types. |
| src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureFunctionPointerType.cs | Implements attribute flags for signature function pointer types. |
| src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureConstructedGenericType.cs | Implements attribute flags for signature constructed generic types (delegates to generic definition). |
| src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureArrayType.cs | Implements array attribute flags (including Serializable, with pragma suppression). |
| src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilderInstantiation.cs | Removes unused hashtable cache field from TypeBuilderInstantiation. |
| src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs | Allows SignatureType inputs for GetMethod/GetConstructor/GetField and returns wrapper instances directly. |
| src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/MethodOnTypeBuilderInstantiation.cs | Simplifies construction (removes static factory) and makes fields readonly. |
| src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/FieldOnTypeBuilderInstantiation.cs | Removes the old cached GetField helper and switches to storing Type instead of TypeBuilderInstantiation. |
| src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/ConstructorOnTypeBuilderInstantiation.cs | Simplifies construction (removes static factory) and makes fields readonly. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/RuntimeTypeBuilder.cs | Broadens handling of constructed generic parent types beyond TypeBuilderInstantiation. |
| src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/CustomAttributeBuilder.cs | Treats SignatureType similarly to TypeBuilderInstantiation when validating named members against ctor declaring type. |
Suppressed comments (1)
src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/FieldOnTypeBuilderInstantiation.cs:20
FieldOnTypeBuilderInstantiation.GetField(...)was removed, but Mono still calls it fromTypeBuilderInstantiation.Mono.cs, which will break Mono builds. Please restore a small forwarding helper (it can just returnnew FieldOnTypeBuilderInstantiation(...)).
internal sealed partial class FieldOnTypeBuilderInstantiation : FieldInfo
{
#region Private Data Members
private readonly FieldInfo _field;
private readonly Type _type;
#endregion
#region Constructor
internal FieldOnTypeBuilderInstantiation(FieldInfo field, Type type)
{
_field = field;
_type = type;
}
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 22 out of 22 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs:378
- Same issue as GetMethod: this should only accept SignatureType when it's a constructed generic signature type. Otherwise non-generic signature types can pass the guard and fail later with unrelated exceptions.
if (type is not (TypeBuilder or TypeBuilderInstantiation or SignatureType))
src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs:406
- Same issue as GetMethod: this should only accept SignatureType when it's a constructed generic signature type. Otherwise non-generic signature types can pass the guard and fail later with unrelated exceptions.
if (type is not (TypeBuilder or TypeBuilderInstantiation or SignatureType))
src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs:336
- Allowing any SignatureType through this guard means non-generic signature types (arrays, byrefs, pointers, modified types) will now get past the initial argument validation and can fail later with less helpful exceptions (e.g., InvalidOperationException from GetGenericTypeDefinition). Since this API only makes sense for constructed generic signature types, restrict the SignatureType allowance to
IsConstructedGenericTypeto preserve predictable ArgumentException behavior for other signature types.
This issue also appears in the following locations of the same file:
- line 378
- line 406
if (type is not (TypeBuilder or TypeBuilderInstantiation or SignatureType))
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Suppressed comments (5)
src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs:380
- Same as GetMethod: allowing any SignatureType here means non-generic signature types can throw InvalidOperationException from GetGenericTypeDefinition instead of the intended ArgumentException on "type". Restrict SignatureType to constructed generic signature types.
public static ConstructorInfo GetConstructor(Type type, ConstructorInfo constructor)
{
if (type is not (TypeBuilder or TypeBuilderInstantiation or SignatureType))
{
throw new ArgumentException(SR.Argument_MustBeTypeBuilder, nameof(type));
src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs:408
- Same as GetMethod/GetConstructor: allowing any SignatureType here means non-generic signature types can throw InvalidOperationException from GetGenericTypeDefinition instead of the intended ArgumentException on "type". Restrict SignatureType to constructed generic signature types.
public static FieldInfo GetField(Type type, FieldInfo field)
{
if (type is not (TypeBuilder or TypeBuilderInstantiation or SignatureType))
{
throw new ArgumentException(SR.Argument_MustBeTypeBuilder, nameof(type));
src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs:339
- The current type validation allows any SignatureType, but these helpers require a constructed generic type. Passing a non-generic signature type (e.g., ByRef/Pointer/FunctionPointer) will now throw InvalidOperationException from GetGenericTypeDefinition instead of the intended ArgumentException on "type". Consider restricting the accepted SignatureType shapes to constructed generic signature types.
This issue also appears in the following locations of the same file:
- line 376
- line 404
public static MethodInfo GetMethod(Type type, MethodInfo method)
{
if (type is not (TypeBuilder or TypeBuilderInstantiation or SignatureType))
{
throw new ArgumentException(SR.Argument_MustBeTypeBuilder, nameof(type));
src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs:427
- TypeBuilder.GetField now always returns a new FieldOnTypeBuilderInstantiation instance. Since MemberInfo equality is reference-based by default, repeated calls with the same inputs will no longer compare equal and will produce different hash codes, which can break consumers that use FieldInfo identity (e.g., as dictionary keys). Consider either restoring per-(type,field) identity caching for GetField wrappers or overriding Equals/GetHashCode on FieldOnTypeBuilderInstantiation to provide stable value-based identity.
{
type = type.MakeGenericType(type.GetGenericArguments());
}
return new FieldOnTypeBuilderInstantiation(field, type);
src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs:339
- SignatureType support was added to TypeBuilder.GetMethod/GetConstructor/GetField, but the PR removes the previous "type must be TypeBuilder" argument-validation tests without adding new coverage for the SignatureType scenarios. It would be good to add focused tests that pass a constructed generic signature type (Type.MakeGenericSignatureType(...)) into these helpers (and keep coverage for rejecting unrelated Type inputs).
public static MethodInfo GetMethod(Type type, MethodInfo method)
{
if (type is not (TypeBuilder or TypeBuilderInstantiation or SignatureType))
{
throw new ArgumentException(SR.Argument_MustBeTypeBuilder, nameof(type));
}
| throw new NotSupportedException(SR.NotSupported_DynamicModule); | ||
|
|
||
| Type inst = genericTypeDefinition.MakeGenericType(m_typeParent.GetGenericArguments()); | ||
|
|
||
| if (inst is TypeBuilderInstantiation) | ||
| if (inst is not RuntimeType) |
There was a problem hiding this comment.
You're not supposed to use types from MetadataLoadContext with runtime reflection emit.
|
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. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.
Suppressed comments (3)
src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderGetMethod.cs:42
- A negative-coverage test for the common argument-validation case (passing a non-TypeBuilder Type) was removed, but the production code still throws for this scenario. Keeping this test (or replacing it with an equivalent) helps prevent future accidental widening of the accepted type set beyond TypeBuilder/TypeBuilderInstantiation/SignatureType.
src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderGetField.cs:33 - A negative-coverage test for the argument-validation case (passing a non-TypeBuilder Type) was removed, but the API still throws for this scenario. Reintroducing this test (or an equivalent) would preserve coverage for the input-validation contract while adding SignatureType support.
src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderGetConstructor.cs:33 - A negative-coverage test for the argument-validation case (passing a non-TypeBuilder Type) was removed, but the API still throws for this scenario. Reintroducing this test (or an equivalent) would preserve coverage for the input-validation contract while adding SignatureType support.
| public static MethodInfo GetMethod(Type type, MethodInfo method) | ||
| { | ||
| if (type is not TypeBuilder && type is not TypeBuilderInstantiation) | ||
| if (type is not (TypeBuilder or TypeBuilderInstantiation or SignatureType)) | ||
| { | ||
| throw new ArgumentException(SR.Argument_MustBeTypeBuilder, nameof(type)); | ||
| } |
Fixes #125612
Implementing suggestion at #126182 (comment):
Type.Attributeson signature types is now supported. Matching Native AOT's reflection stack, we return:Attributeson constructed generic types.AutoLayout | AnsiClass | Class | Public | Sealed | Serializableon array types.Publicon other types (function pointers, pointers, byrefs, modified types)TypeBuilderImpl.IsValueTypeImplwas updated to support types with signature base types.TypeBuilderfactory methods, in order to accept signature types.