Skip to content

Fix: Incorrect order of checks for TypeBuilder GetConstructor and GetField method #53645

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.

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 16 commits into from
Nov 2, 2021
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public void Bake(ModuleBuilder module, int token)
Justification = "MakeGenericType is only called on a TypeBuilder which is not subject to trimming")]
public static MethodInfo GetMethod(Type type, MethodInfo method)
{
if (!(type is TypeBuilder) && !(type is TypeBuilderInstantiation))
if (type is not TypeBuilder && type is not TypeBuilderInstantiation)
throw new ArgumentException(SR.Argument_MustBeTypeBuilder);

// The following checks establishes invariants that more simply put require type to be generic and
Expand All @@ -91,7 +91,7 @@ public static MethodInfo GetMethod(Type type, MethodInfo method)
if (type.IsGenericTypeDefinition)
type = type.MakeGenericType(type.GetGenericArguments());

if (!(type is TypeBuilderInstantiation))
if (type is not TypeBuilderInstantiation)
throw new ArgumentException(SR.Argument_NeedNonGenericType, nameof(type));

return MethodOnTypeBuilderInstantiation.GetMethod(method, (type as TypeBuilderInstantiation)!);
Expand All @@ -101,21 +101,21 @@ public static MethodInfo GetMethod(Type type, MethodInfo method)
Justification = "MakeGenericType is only called on a TypeBuilder which is not subject to trimming")]
public static ConstructorInfo GetConstructor(Type type, ConstructorInfo constructor)
{
if (!(type is TypeBuilder) && !(type is TypeBuilderInstantiation))
if (type is not TypeBuilder && type is not TypeBuilderInstantiation)
throw new ArgumentException(SR.Argument_MustBeTypeBuilder);

if (!constructor.DeclaringType!.IsGenericTypeDefinition)
throw new ArgumentException(SR.Argument_ConstructorNeedGenericDeclaringType, nameof(constructor));

if (!(type is TypeBuilderInstantiation))
throw new ArgumentException(SR.Argument_NeedNonGenericType, nameof(type));
if (type.GetGenericTypeDefinition() != constructor.DeclaringType)
throw new ArgumentException(SR.Argument_InvalidConstructorDeclaringType, nameof(type));

// TypeBuilder G<T> ==> TypeBuilderInstantiation G<T>
if (type is TypeBuilder && type.IsGenericTypeDefinition)
type = type.MakeGenericType(type.GetGenericArguments());

if (type.GetGenericTypeDefinition() != constructor.DeclaringType)
throw new ArgumentException(SR.Argument_InvalidConstructorDeclaringType, nameof(type));
if (type is not TypeBuilderInstantiation)
throw new ArgumentException(SR.Argument_NeedNonGenericType, nameof(type));

return ConstructorOnTypeBuilderInstantiation.GetConstructor(constructor, (type as TypeBuilderInstantiation)!);
}
Expand All @@ -124,21 +124,21 @@ public static ConstructorInfo GetConstructor(Type type, ConstructorInfo construc
Justification = "MakeGenericType is only called on a TypeBuilder which is not subject to trimming")]
public static FieldInfo GetField(Type type, FieldInfo field)
{
if (!(type is TypeBuilder) && !(type is TypeBuilderInstantiation))
if (type is not TypeBuilder and not TypeBuilderInstantiation)
throw new ArgumentException(SR.Argument_MustBeTypeBuilder);

if (!field.DeclaringType!.IsGenericTypeDefinition)
throw new ArgumentException(SR.Argument_FieldNeedGenericDeclaringType, nameof(field));

if (!(type is TypeBuilderInstantiation))
throw new ArgumentException(SR.Argument_NeedNonGenericType, nameof(type));
if (type.GetGenericTypeDefinition() != field.DeclaringType)
throw new ArgumentException(SR.Argument_InvalidFieldDeclaringType, nameof(type));

// TypeBuilder G<T> ==> TypeBuilderInstantiation G<T>
if (type is TypeBuilder && type.IsGenericTypeDefinition)
type = type.MakeGenericType(type.GetGenericArguments());

if (type.GetGenericTypeDefinition() != field.DeclaringType)
throw new ArgumentException(SR.Argument_InvalidFieldDeclaringType, nameof(type));
if (type is not TypeBuilderInstantiation)
throw new ArgumentException(SR.Argument_NeedNonGenericType, nameof(type));

return FieldOnTypeBuilderInstantiation.GetField(field, (type as TypeBuilderInstantiation)!);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,14 +8,14 @@ namespace System.Reflection.Emit.Tests
public class TypeBuilderGetConstructor
{
[Fact]
public void GetConstructor_DeclaringTypeOfConstructorNotGenericTypeDefinition_ThrowsArgumentException()
public void GetConstructor_DeclaringTypeOfConstructorNotGenericTypeDefinition()
{
TypeBuilder type = Helpers.DynamicType(TypeAttributes.Class | TypeAttributes.Public);
type.DefineGenericParameters("T");

ConstructorBuilder ctor = type.DefineDefaultConstructor(MethodAttributes.PrivateScope | MethodAttributes.Public | MethodAttributes.HideBySig | MethodAttributes.SpecialName | MethodAttributes.RTSpecialName);

AssertExtensions.Throws<ArgumentException>("type", () => TypeBuilder.GetConstructor(type.AsType(), ctor));
var constructor = TypeBuilder.GetConstructor(type.AsType(), ctor);
Assert.False(constructor.IsGenericMethodDefinition);
}

[Fact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,13 @@ namespace System.Reflection.Emit.Tests
public class TypeBuilderGetField
{
[Fact]
public void GetField_DeclaringTypeOfFieldNotGeneric_ThrowsArgumentException()
public void GetField_DeclaringTypeOfFieldNotGeneric()
{
TypeBuilder type = Helpers.DynamicType(TypeAttributes.Class | TypeAttributes.Public);
GenericTypeParameterBuilder[] typeParams = type.DefineGenericParameters("T");

FieldBuilder field = type.DefineField("Field", typeParams[0].AsType(), FieldAttributes.Public);
AssertExtensions.Throws<ArgumentException>("type", () => TypeBuilder.GetField(type.AsType(), field));
Assert.Equal("Field", TypeBuilder.GetField(type.AsType(), field).Name);
}

[Fact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1835,35 +1835,6 @@ public GenericTypeParameterBuilder[] DefineGenericParameters(params string[] nam
return generic_params;
}

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2070:UnrecognizedReflectionPattern",
Justification = "Linker thinks Type.GetConstructor(ConstructorInfo) is one of the public APIs because it doesn't analyze method signatures. We already have ConstructorInfo.")]
public static ConstructorInfo GetConstructor(Type type, ConstructorInfo constructor)
{
/*FIXME I would expect the same checks of GetMethod here*/
if (type == null)
throw new ArgumentException("Type is not generic", nameof(type));

if (!type.IsGenericType)
throw new ArgumentException("Type is not a generic type", nameof(type));

if (type.IsGenericTypeDefinition)
throw new ArgumentException("Type cannot be a generic type definition", nameof(type));

if (constructor == null)
throw new NullReferenceException(); //MS raises this instead of an ArgumentNullException

if (!constructor.DeclaringType!.IsGenericTypeDefinition)
throw new ArgumentException("constructor declaring type is not a generic type definition", nameof(constructor));
if (constructor.DeclaringType != type.GetGenericTypeDefinition())
throw new ArgumentException("constructor declaring type is not the generic type definition of type", nameof(constructor));

ConstructorInfo res = type.GetConstructor(constructor);
if (res == null)
throw new ArgumentException("constructor not found");

return res;
}

private static bool IsValidGetMethodType(Type type)
Copy link
Member

@eerhardt eerhardt Jul 14, 2021

Choose a reason for hiding this comment

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

Can you put this method back where it was originally declared? That will make reviewing the change much easier. Also when people look at source history, will be able to see what change was made here easily. #Closed

{
if (type is TypeBuilder || type is TypeBuilderInstantiation)
Expand All @@ -1885,6 +1856,34 @@ private static bool IsValidGetMethodType(Type type)
return false;
}

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2070:UnrecognizedReflectionPattern",
Justification = "Linker thinks Type.GetConstructor(ConstructorInfo) is one of the public APIs because it doesn't analyze method signatures. We already have ConstructorInfo.")]
public static ConstructorInfo GetConstructor(Type type, ConstructorInfo constructor)
{
if (!IsValidGetMethodType(type))
throw new ArgumentException("type is not TypeBuilder but " + type.GetType(), nameof(type));

if (type == null)
throw new ArgumentException("Type is not generic", nameof(type));

Copy link
Member

Choose a reason for hiding this comment

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

I think this needs the same code that GetMethod has:

            if (type is TypeBuilder && type.ContainsGenericParameters)
                type = type.MakeGenericType(type.GetGenericArguments());

(and same for GetField)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried, but it won't build.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like you also need

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2055:UnrecognizedReflectionPattern",
            Justification = "Type.MakeGenericType is used to create a typical instantiation")]

if (!type.IsGenericType)
throw new ArgumentException("Type is not a generic type", nameof(type));

if (constructor.DeclaringType != type.GetGenericTypeDefinition())
throw new ArgumentException("constructor declaring type is not the generic type definition of type", nameof(constructor));

if (!constructor.DeclaringType!.IsGenericTypeDefinition)
throw new ArgumentException("constructor declaring type is not a generic type definition", nameof(constructor));
if (constructor == null)
throw new NullReferenceException(); //MS raises this instead of an ArgumentNullException

ConstructorInfo res = type.GetConstructor(constructor);
if (res == null)
throw new ArgumentException("constructor not found");

return res;
}

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2055:UnrecognizedReflectionPattern",
Justification = "Type.MakeGenericType is used to create a typical instantiation")]
public static MethodInfo GetMethod(Type type, MethodInfo method)
Expand Down Expand Up @@ -1914,18 +1913,18 @@ public static MethodInfo GetMethod(Type type, MethodInfo method)

public static FieldInfo GetField(Type type, FieldInfo field)
{
if (!IsValidGetMethodType(type))
throw new ArgumentException("type is not TypeBuilder but " + type.GetType(), nameof(type));

if (!type.IsGenericType)
throw new ArgumentException("Type is not a generic type", nameof(type));

if (type.IsGenericTypeDefinition)
throw new ArgumentException("Type cannot be a generic type definition", nameof(type));
if (field.DeclaringType != type.GetGenericTypeDefinition())
throw new ArgumentException("field declaring type is not the generic type definition of type", nameof(field));

if (field is FieldOnTypeBuilderInst)
throw new ArgumentException("The specified field must be declared on a generic type definition.", nameof(field));

if (field.DeclaringType != type.GetGenericTypeDefinition())
throw new ArgumentException("field declaring type is not the generic type definition of type", nameof(field));

FieldInfo res = type.GetField(field);
if (res == null)
throw new System.Exception("field not found");
Expand Down