Skip to content

Support generic type parameter when created with TypeBuilder #112372

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Feb 21, 2025
Merged

Support generic type parameter when created with TypeBuilder #112372

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
452eee6
Support generic type parameter when created with TypeBuilder
steveharter Feb 10, 2025
668a960
Address feedback; add tests to runtime
steveharter Feb 11, 2025
71afb64
Add SignatureType.IsEnum support; update tests
steveharter Feb 12, 2025
db3dbcd
Update tests
steveharter Feb 13, 2025
b701473
Return false for SigConstructedGenericType.IsEnum; remove unnecessary…
steveharter Feb 19, 2025
20a7784
Consistent validation across MakeGenericType() and MakeGenericSignatu…
steveharter Feb 20, 2025
5098883
Consistent validation across MakeGenericType() and MakeGenericSignatu…
steveharter Feb 20, 2025
095ece6
Support generic enums
steveharter Feb 20, 2025
10db98e
Apply consistent parameter naming to Mono
steveharter Feb 20, 2025
f0da313
Apply consistent exception to NativeAot
steveharter Feb 20, 2025
c339bb9
Remove 'typeArguments' param name in NullReferenceException if null a…
steveharter Feb 20, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions ...libraries/System.Private.CoreLib/src/System/Reflection/SignatureConstructedGenericType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ internal SignatureConstructedGenericType(Type genericTypeDefinition, Type[] type
protected sealed override bool IsArrayImpl() => false;
protected sealed override bool IsByRefImpl() => false;
public sealed override bool IsByRefLike => _genericTypeDefinition.IsByRefLike;
public sealed override bool IsEnum => _genericTypeDefinition.IsEnum;
protected sealed override bool IsPointerImpl() => false;
public sealed override bool IsSZArray => false;
public sealed override bool IsVariableBoundArray => false;
Expand All @@ -50,6 +51,7 @@ public sealed override bool ContainsGenericParameters
}
}

protected sealed override bool IsValueTypeImpl() => _genericTypeDefinition.IsValueType;
internal sealed override SignatureType? ElementType => null;
public sealed override int GetArrayRank() => throw new ArgumentException(SR.Argument_HasToBeArrayClass);
public sealed override Type GetGenericTypeDefinition() => _genericTypeDefinition;
Expand Down
2 changes: 2 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureGenericParameterType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,15 @@ protected SignatureGenericParameterType(int position)
protected sealed override bool IsArrayImpl() => false;
protected sealed override bool IsByRefImpl() => false;
public sealed override bool IsByRefLike => false;
public sealed override bool IsEnum => throw new NotSupportedException(SR.NotSupported_SignatureType);
protected sealed override bool IsPointerImpl() => false;
public sealed override bool IsSZArray => false;
public sealed override bool IsVariableBoundArray => false;
public sealed override bool IsConstructedGenericType => false;
public sealed override bool IsGenericParameter => true;
public abstract override bool IsGenericMethodParameter { get; }
public sealed override bool ContainsGenericParameters => true;
protected sealed override bool IsValueTypeImpl() => throw new NotSupportedException(SR.NotSupported_SignatureType);

internal sealed override SignatureType? ElementType => null;
public sealed override int GetArrayRank() => throw new ArgumentException(SR.Argument_HasToBeArrayClass);
Expand Down
2 changes: 2 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureHasElementType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ protected SignatureHasElementType(SignatureType elementType)
protected abstract override bool IsArrayImpl();
protected abstract override bool IsByRefImpl();
public sealed override bool IsByRefLike => false;
public sealed override bool IsEnum => false;
protected abstract override bool IsPointerImpl();
public abstract override bool IsSZArray { get; }
public abstract override bool IsVariableBoundArray { get; }
Expand All @@ -27,6 +28,7 @@ protected SignatureHasElementType(SignatureType elementType)
public sealed override bool IsGenericTypeParameter => false;
public sealed override bool IsGenericMethodParameter => false;
public sealed override bool ContainsGenericParameters => _elementType.ContainsGenericParameters;
protected sealed override bool IsValueTypeImpl() => false;

internal sealed override SignatureType? ElementType => _elementType;
public abstract override int GetArrayRank();
Expand Down
4 changes: 2 additions & 2 deletions src/libraries/System.Private.CoreLib/src/System/Reflection/SignatureType.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ public sealed override Type MakeArrayType(int rank)
public sealed override Type[] FindInterfaces(TypeFilter filter, object? filterCriteria) => throw new NotSupportedException(SR.NotSupported_SignatureType);
public sealed override InterfaceMapping GetInterfaceMap([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.NonPublicMethods)] Type interfaceType) => throw new NotSupportedException(SR.NotSupported_SignatureType);
protected sealed override bool IsContextfulImpl() => throw new NotSupportedException(SR.NotSupported_SignatureType);
public sealed override bool IsEnum => throw new NotSupportedException(SR.NotSupported_SignatureType);
public abstract override bool IsEnum { get; }
public sealed override bool IsEquivalentTo([NotNullWhen(true)] Type? other) => throw new NotSupportedException(SR.NotSupported_SignatureType);
public sealed override bool IsInstanceOfType([NotNullWhen(true)] object? o) => throw new NotSupportedException(SR.NotSupported_SignatureType);
protected sealed override bool IsMarshalByRefImpl() => throw new NotSupportedException(SR.NotSupported_SignatureType);
Expand All @@ -198,7 +198,7 @@ public sealed override Type MakeArrayType(int rank)
[Obsolete(Obsoletions.LegacyFormatterMessage, DiagnosticId = Obsoletions.LegacyFormatterDiagId, UrlFormat = Obsoletions.SharedUrlFormat)]
public sealed override bool IsSerializable => throw new NotSupportedException(SR.NotSupported_SignatureType);
public sealed override bool IsSubclassOf(Type c) => throw new NotSupportedException(SR.NotSupported_SignatureType);
protected sealed override bool IsValueTypeImpl() => throw new NotSupportedException(SR.NotSupported_SignatureType);
protected abstract override bool IsValueTypeImpl();

public sealed override StructLayoutAttribute StructLayoutAttribute => throw new NotSupportedException(SR.NotSupported_SignatureType);

Expand Down
2 changes: 2 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System/Reflection/TypeDelegator.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@ public TypeDelegator([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.
public override string? AssemblyQualifiedName => typeImpl.AssemblyQualifiedName;
public override Type? BaseType => typeImpl.BaseType;

public override int GetArrayRank() => typeImpl.GetArrayRank();

[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors | DynamicallyAccessedMemberTypes.NonPublicConstructors)]
protected override ConstructorInfo? GetConstructorImpl(BindingFlags bindingAttr, Binder? binder,
CallingConventions callConvention, Type[] types, ParameterModifier[]? modifiers)
Expand Down
46 changes: 44 additions & 2 deletions ...ies/System.Reflection.Emit/tests/PersistedAssemblyBuilder/AssemblySaveTypeBuilderTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -733,8 +733,6 @@ public void MethodBuilderGetParametersReturnParameterTest()
Assert.True(method3.ReturnParameter.IsRetval);
}

public class BaseType<T> { }

[Fact]
public void GenericTypeWithTypeBuilderGenericParameter_UsedAsParent()
{
Expand All @@ -749,9 +747,53 @@ public void GenericTypeWithTypeBuilderGenericParameter_UsedAsParent()

Assert.NotNull(type.GetConstructor(Type.EmptyTypes)); // Default constructor created
}

[Fact]
public void CreateGenericTypeFromMetadataLoadContextSignatureTypes()
{
using TempFile file = TempFile.Create();

PersistedAssemblyBuilder ab = AssemblySaveTools.PopulateAssemblyAndModule(out ModuleBuilder module);

TypeBuilder childType = module.DefineType("Child");
TypeBuilder parentType = module.DefineType("Parent");

// Get List<T> from MLC and make both reference and value type fields from that.
using MetadataLoadContext mlc = new MetadataLoadContext(new CoreMetadataAssemblyResolver());
Type listOfTType = mlc.CoreAssembly.GetType(typeof(List<>).FullName!);

// Currently MakeGenericSignatureType() must be used instead of MakeGenericType() for
// generic type parameters created with TypeBuilder.
Assert.Throws<ArgumentException>(() => listOfTType.MakeGenericType(childType));
Type listOfReferenceTypes = Type.MakeGenericSignatureType(listOfTType, childType);
parentType.DefineField("ReferenceTypeChildren", listOfReferenceTypes, FieldAttributes.Public);

// Pre-existing types can use MakeGenericType().
Type int32Type = mlc.CoreAssembly.GetType(typeof(int).FullName);
Type listOfValueTypes = listOfTType.MakeGenericType(int32Type);
parentType.DefineField("ValueTypeChildren", listOfValueTypes, FieldAttributes.Public);

parentType.CreateType();
childType.CreateType();

// Save and load the dynamically created assembly.
ab.Save(file.Path);
Module mlcModule = mlc.LoadFromAssemblyPath(file.Path).Modules.First();

Assert.Equal("Child", mlcModule.GetTypes()[0].Name);
Assert.Equal("Parent", mlcModule.GetTypes()[1].Name);

FieldInfo[] fields = mlcModule.GetTypes()[1].GetFields(BindingFlags.Public | BindingFlags.Instance);
Assert.Equal("ReferenceTypeChildren", fields[0].Name);
Assert.False(fields[0].FieldType.GetGenericArguments()[0].IsValueType);
Assert.Equal("ValueTypeChildren", fields[1].Name);
Assert.True(fields[1].FieldType.GetGenericArguments()[0].IsValueType);
}
}

// Test Types
public class BaseType<T> { }

public interface INoMethod
{
}
Expand Down
69 changes: 58 additions & 11 deletions src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Reflection/SignatureTypes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,24 +10,18 @@ namespace System.Reflection.Tests
{
public static class SignatureTypeTests
{
[Fact]
public static void IsSignatureType()
[Theory]
[MemberData(nameof(IsSignatureTypeTestData))]
public static void IsSignatureType(Type type, bool expected)
{
// Executing [Theory] logic manually. Signature Types cannot be used in theory data because Xunit preemptively invokes an unguarded
// System.Type pretty printer that invokes members that Signature Types don't support.
foreach (object[] pair in IsSignatureTypeTestData)
{
Type type = (Type)(pair[0]);
bool expected = (bool)(pair[1]);

Assert.Equal(expected, type.IsSignatureType);
}
Assert.Equal(expected, type.IsSignatureType);
}

public static IEnumerable<object[]> IsSignatureTypeTestData
{
get
{
// Standard reflection used as baseline.
yield return new object[] { typeof(int), false };
yield return new object[] { typeof(int).MakeArrayType(), false };
yield return new object[] { typeof(int).MakeArrayType(1), false };
Expand All @@ -37,6 +31,7 @@ public static IEnumerable<object[]> IsSignatureTypeTestData
yield return new object[] { typeof(List<>).MakeGenericType(typeof(int)), false };
yield return new object[] { typeof(List<>).GetGenericArguments()[0], false };

// SignatureTypes.
Type sigType = Type.MakeGenericMethodParameter(2);

yield return new object[] { sigType, true };
Expand All @@ -48,7 +43,45 @@ public static IEnumerable<object[]> IsSignatureTypeTestData
yield return new object[] { typeof(List<>).MakeGenericType(sigType), true };

yield return new object[] { Type.MakeGenericSignatureType(typeof(List<>), typeof(int)), true };
yield return new object[] { Type.MakeGenericSignatureType(typeof(List<>), typeof(int)).GetGenericArguments()[0], false };
yield return new object[] { Type.MakeGenericSignatureType(typeof(List<>), sigType), true };
yield return new object[] { Type.MakeGenericSignatureType(typeof(List<>), sigType).GetGenericArguments()[0], true };
}
}

[Theory]
[MemberData(nameof(IsValueTypeTestData))]
public static void IsValueType(Type type, bool expected)
{
Assert.Equal(expected, type.IsValueType);
}

public static IEnumerable<object[]> IsValueTypeTestData
{
get
{
// These have normal generic argument types, not SignatureTypes. Using a SignatureType from MakeGenericMethodParameter()
// as the generic type argument throws NotSupportedException which is verified in MakeSignatureConstructedGenericType().
yield return new object[] { Type.MakeGenericSignatureType(typeof(List<>), typeof(int)).GetGenericArguments()[0], true };
Copy link
Member

Choose a reason for hiding this comment

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

Type.MakeGenericSignatureType(typeof(List<>), typeof(int)).GetGenericArguments()[0] is going to be typeof(int), so this test does not seem to be actually testing IsValueType property on signature types.

Should this rather be something like:

yield return new object[] { Type.MakeGenericSignatureType(typeof(List<>), typeof(int)), false };
yield return new object[] { Type.MakeGenericSignatureType(typeof(KeyValuePair<,>), typeof(string), typeof(object)), true };

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct this is not verifying signature types but is verifying that when a non-signature type is used as a generic parameter then IsValueType works as expected. The test with GetGenericArguments() is done in MakeSignatureConstructedGenericType() as mentioned in the comment.

I'm not sure if your feedback here came before or after I pushed a new commit that changed this and added the comment.

Copy link
Member

Choose a reason for hiding this comment

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

verifying that when a non-signature type is used as a generic parameter then IsValueType works as expected.

Would it be better to verify that GetGenericArguments() returns the exact same Type instances as was passed into MakeGenericSignatureType?

If we do that, we do not need to worry about testing individual properties of the types returned by GetGenericArguments. We can depend on that testing done elsewhere (e.g. as part of runtime Type testing)

Copy link
Member

@jkotas jkotas Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

My higher level concerns in both this and the other comment are:

  • Missing test coverage for the code introduced in this PR. For example, if SignatureConstructedGenericType.IsEnum implementation is changed to return hardcoded false, no test is going to fail. These test gaps should be fixed. They are kind of hard to see because they are muddied by the unnecessary tests.
  • Unnecessary test coverage for Types that are not signature types. It would be best to delete most of it and assume that the non-signature Types are tested elsewhere. This is pre-existing problem in these tests. I would not mind if you delete tests from this file that are not relevant to testing of signature types.

Copy link
Member Author

Choose a reason for hiding this comment

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

I did remove newly added code that did not directly verify sig types.

Added tests for IsEnum for both true and false.

Also I found that the validation in MakeGenericSignatureType() did not check it the incoming type was generic or not, so I updated that and compared against MakeGenericType() which I also updated since the parameter names were different in the exception text (the internal override changed the parameter name from the public abstract). So in this case, I did add tests that are not sig-specific but it did catch this issue plus it's nice to see in one location.

Copy link
Member

Choose a reason for hiding this comment

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

Also I found that the validation in MakeGenericSignatureType() did not check it the incoming type was generic or not, so I updated that

This is technically a breaking change. Just pointing it out, I do not see a problem with it.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wonder if this breaking change meets the threshold to document it; I'll mark this breaking change for now and re-evaluate later as well as update the exception doc.

yield return new object[] { Type.MakeGenericSignatureType(typeof(List<>), typeof(object)).GetGenericArguments()[0], false };
}
}

[Theory]
[MemberData(nameof(IsEnumTestData))]
public static void IsEnum(Type type, bool expected)
{
Assert.Equal(expected, type.IsEnum);
}

public static IEnumerable<object[]> IsEnumTestData
{
get
{
// These have normal generic argument types, not SignatureTypes. Using a SignatureType from MakeGenericMethodParameter()
// as the generic type argument throws NotSupportedException which is verified in MakeSignatureConstructedGenericType().
yield return new object[] { Type.MakeGenericSignatureType(typeof(List<>), typeof(MyEnum)).GetGenericArguments()[0], true };
yield return new object[] { Type.MakeGenericSignatureType(typeof(List<>), typeof(object)).GetGenericArguments()[0], false };
}
}

Expand Down Expand Up @@ -211,6 +244,8 @@ public static void MakeGenericMethodParameter(int position)
Assert.False(t.IsGenericTypeParameter);
Assert.True(t.IsGenericMethodParameter);
Assert.Equal(position, t.GenericParameterPosition);
Assert.Throws<NotSupportedException>(() => t.IsValueType);
Assert.Throws<NotSupportedException>(() => t.IsEnum);
TestSignatureTypeInvariants(t);
}

Expand All @@ -237,6 +272,8 @@ public static void MakeSignatureArrayType()
Assert.True(et.IsGenericParameter);
Assert.False(et.IsGenericTypeParameter);
Assert.True(et.IsGenericMethodParameter);
Assert.Throws<NotSupportedException>(() => et.IsValueType);
Assert.Throws<NotSupportedException>(() => et.IsEnum);
Assert.Equal(5, et.GenericParameterPosition);

TestSignatureTypeInvariants(t);
Expand All @@ -253,6 +290,8 @@ public static void MakeSignatureMdArrayType(int rank)
Assert.True(t.IsArray);
Assert.True(t.IsVariableBoundArray);
Assert.Equal(rank, t.GetArrayRank());
Assert.False(t.IsValueType);
Assert.False(t.IsEnum);

TestSignatureTypeInvariants(t);
}
Expand All @@ -270,6 +309,8 @@ public static void MakeSignatureByRefType()
Assert.False(et.IsGenericTypeParameter);
Assert.True(et.IsGenericMethodParameter);
Assert.Equal(5, et.GenericParameterPosition);
Assert.False(t.IsValueType);
Assert.False(t.IsEnum);

TestSignatureTypeInvariants(t);
}
Expand All @@ -287,6 +328,8 @@ public static void MakeSignaturePointerType()
Assert.False(et.IsGenericTypeParameter);
Assert.True(et.IsGenericMethodParameter);
Assert.Equal(5, et.GenericParameterPosition);
Assert.False(t.IsValueType);
Assert.False(t.IsEnum);

TestSignatureTypeInvariants(t);
}
Expand All @@ -312,6 +355,8 @@ public static void MakeSignatureConstructedGenericType(Type genericTypeDefinitio
Assert.False(et.IsGenericTypeParameter);
Assert.True(et.IsGenericMethodParameter);
Assert.Equal(5, et.GenericParameterPosition);
Assert.Throws<NotSupportedException>(() => et.IsValueType);
Assert.Throws<NotSupportedException>(() => et.IsEnum);

TestSignatureTypeInvariants(t);
});
Expand Down Expand Up @@ -408,6 +453,8 @@ public static void Moo<M>(int p1, int p2) where M : NoOneSubclassesThisEither {
private class NoOneSubclasses { }
private class NoOneSubclassesThisEither { }

private enum MyEnum { }

private static void TestSignatureTypeInvariants(Type type)
{
Assert.True(type.IsSignatureType);
Expand Down
Loading