There are 3 static methods on TypeBuilder to get an instantiated generic MemberInfo given a generic definition:
https://docs.microsoft.com/en-us/dotnet/api/system.reflection.emit.typebuilder.getconstructor
https://docs.microsoft.com/en-us/dotnet/api/system.reflection.emit.typebuilder.getfield
https://docs.microsoft.com/en-us/dotnet/api/system.reflection.emit.typebuilder.getmethod
The issue is, the first 2 method (GetConstructor and GetField) have their checks in the wrong order, resulting in dead code. For example:
|
if (!(type is TypeBuilderInstantiation)) |
|
throw new ArgumentException(SR.Argument_NeedNonGenericType, nameof(type)); |
|
|
|
// TypeBuilder G<T> ==> TypeBuilderInstantiation G<T> |
|
if (type is TypeBuilder && type.IsGenericTypeDefinition) |
|
type = type.MakeGenericType(type.GetGenericArguments()); |
The first check says "if the type isn't a TypeBuilderInstantiation, throw". Then the next check is checking if the type is a TypeBuilder. Both TypeBuilder and TypeBuilderInstantiation are sealed types that inherit from TypeInfo. So if the type isn't a TypeBuilderInstantiation, an exception is thrown, and the check for type is TypeBuilder is dead code.
I thought I could just remove the dead code from these two methods, but looking at the last one: TypeBuilder::GetMethod:
|
// The following converts from Type or TypeBuilder of G<T> to TypeBuilderInstantiation G<T>. These types |
|
// both logically represent the same thing. The runtime displays a similar convention by having |
|
// G<M>.M() be encoded by a typeSpec whose parent is the typeDef for G<M> and whose instantiation is also G<M>. |
|
if (type.IsGenericTypeDefinition) |
|
type = type.MakeGenericType(type.GetGenericArguments()); |
|
|
|
if (!(type is TypeBuilderInstantiation)) |
|
throw new ArgumentException(SR.Argument_NeedNonGenericType, nameof(type)); |
This has the checks flipped, which makes more sense. If the type passed in IsGenericTypeDefinition (which can only be true if it is a TypeBuilder), then it calls MakeGenericType, which will return a TypeBuilderInstantiation.
My assumption is the GetConstructor and GetField methods were supposed to be written this way, but it was a mistake. I looked back in internal source history, and it appears all 3 of these methods were written this way when the feature was first developed.
If the code is truly not necessary, we should just delete the dead code. But I think the checks should be flipped to match the GetMethod method.
Note: we should also add unit tests for this scenario when fixing this bug.
There are 3 static methods on TypeBuilder to get an instantiated generic MemberInfo given a generic definition:
https://docs.microsoft.com/en-us/dotnet/api/system.reflection.emit.typebuilder.getconstructor
https://docs.microsoft.com/en-us/dotnet/api/system.reflection.emit.typebuilder.getfield
https://docs.microsoft.com/en-us/dotnet/api/system.reflection.emit.typebuilder.getmethod
The issue is, the first 2 method (GetConstructor and GetField) have their checks in the wrong order, resulting in dead code. For example:
runtime/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs
Lines 105 to 110 in 53c1619
The first check says "if the type isn't a TypeBuilderInstantiation, throw". Then the next check is checking if the type is a
TypeBuilder. BothTypeBuilderandTypeBuilderInstantiationare sealed types that inherit fromTypeInfo. So if the type isn't aTypeBuilderInstantiation, an exception is thrown, and the check fortype is TypeBuilderis dead code.I thought I could just remove the dead code from these two methods, but looking at the last one:
TypeBuilder::GetMethod:runtime/src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs
Lines 86 to 93 in 53c1619
This has the checks flipped, which makes more sense. If the type passed in
IsGenericTypeDefinition(which can only be true if it is aTypeBuilder), then it callsMakeGenericType, which will return aTypeBuilderInstantiation.My assumption is the
GetConstructorandGetFieldmethods were supposed to be written this way, but it was a mistake. I looked back in internal source history, and it appears all 3 of these methods were written this way when the feature was first developed.If the code is truly not necessary, we should just delete the dead code. But I think the checks should be flipped to match the
GetMethodmethod.Note: we should also add unit tests for this scenario when fixing this bug.