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.

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
eerhardt merged 16 commits into from
Nov 2, 2021
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
d115433
Reorder checks for TypeBuilder.GetConstructor()
BartoszKlonowski May 31, 2021
22cf90a
Reorder checks for TypeBuilder.GetField()
BartoszKlonowski May 31, 2021
f2efe72
Adjust unit test to reordered TypeBuilder checks
BartoszKlonowski Jun 3, 2021
4940fc1
Use 'not' keyword for pattern matching
BartoszKlonowski Jun 14, 2021
3472615
Apply the same Type checks for Mono
BartoszKlonowski Jun 14, 2021
fd50ec9
Check whether a declaring type isn't type's generic definition
BartoszKlonowski Jul 13, 2021
6fcfbf5
Make generic type if contains generic parameters
BartoszKlonowski Jul 14, 2021
f039dc9
Suppres UnrecognizedReflectionPattern for GetField and GetConstructor
BartoszKlonowski Jul 14, 2021
863b984
Move GetMethodType validation method at original order
BartoszKlonowski Jul 26, 2021
f806283
Test GetConstructor against throwing an exception
BartoszKlonowski Jul 26, 2021
70f45eb
Fix: copy-paste error 'type' -> 'constructor'
BartoszKlonowski Jul 26, 2021
b2c2b4a
Align Mono and CoreCLR with nameof for GetConstructor
BartoszKlonowski Jul 27, 2021
98036aa
Use type for argument exception and test against type parameter
BartoszKlonowski Jul 27, 2021
474a10b
Merge remote-tracking branch 'upstream/main' into fix-typebuilder-che…
eerhardt Oct 4, 2021
23795a0
Get tests passing on both coreclr and mono.
eerhardt Oct 6, 2021
f7ed1ac
Fix mono exception messages to use SR resources.
eerhardt Nov 2, 2021
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
40 changes: 20 additions & 20 deletions src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,8 @@ 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))
throw new ArgumentException(SR.Argument_MustBeTypeBuilder);
if (type is not TypeBuilder && type is not TypeBuilderInstantiation)
throw new ArgumentException(SR.Argument_MustBeTypeBuilder, nameof(type));

// The following checks establishes invariants that more simply put require type to be generic and
// method to be a generic method definition declared on the generic type definition of type.
Expand All @@ -91,56 +91,56 @@ 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 typeBuilderInstantiation)
throw new ArgumentException(SR.Argument_NeedNonGenericType, nameof(type));

return MethodOnTypeBuilderInstantiation.GetMethod(method, (type as TypeBuilderInstantiation)!);
return MethodOnTypeBuilderInstantiation.GetMethod(method, typeBuilderInstantiation);
}

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2055:UnrecognizedReflectionPattern",
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))
throw new ArgumentException(SR.Argument_MustBeTypeBuilder);
if (type is not TypeBuilder && type is not TypeBuilderInstantiation)
throw new ArgumentException(SR.Argument_MustBeTypeBuilder, nameof(type));

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)
if (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 typeBuilderInstantiation)
throw new ArgumentException(SR.Argument_NeedNonGenericType, nameof(type));

return ConstructorOnTypeBuilderInstantiation.GetConstructor(constructor, (type as TypeBuilderInstantiation)!);
return ConstructorOnTypeBuilderInstantiation.GetConstructor(constructor, typeBuilderInstantiation);
}

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2055:UnrecognizedReflectionPattern",
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))
throw new ArgumentException(SR.Argument_MustBeTypeBuilder);
if (type is not TypeBuilder and not TypeBuilderInstantiation)
throw new ArgumentException(SR.Argument_MustBeTypeBuilder, nameof(type));

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)
if (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 typeBuilderInstantiation)
throw new ArgumentException(SR.Argument_NeedNonGenericType, nameof(type));

return FieldOnTypeBuilderInstantiation.GetField(field, (type as TypeBuilderInstantiation)!);
return FieldOnTypeBuilderInstantiation.GetField(field, typeBuilderInstantiation);
}
#endregion

Expand Down
11 changes: 4 additions & 7 deletions src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderGetConstructor.cs
View file
Open in desktop
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_DeclaringTypeOfConstructorGenericTypeDefinition()
{
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 All @@ -32,14 +32,12 @@ public void GetConstructor()
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/2389", TestRuntimes.Mono)]
public void GetConstructor_TypeNotTypeBuilder_ThrowsArgumentException()
{
AssertExtensions.Throws<ArgumentException>(null, () => TypeBuilder.GetConstructor(typeof(int), typeof(int).GetConstructor(new Type[0])));
AssertExtensions.Throws<ArgumentException>("type", () => TypeBuilder.GetConstructor(typeof(int), typeof(int).GetConstructor(new Type[0])));
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/2389", TestRuntimes.Mono)]
public void GetConstructor_DeclaringTypeOfConstructorNotGenericTypeDefinitionOfType_ThrowsArgumentException()
{
ModuleBuilder module = Helpers.DynamicModule();
Expand All @@ -58,7 +56,6 @@ public void GetConstructor_DeclaringTypeOfConstructorNotGenericTypeDefinitionOfT
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/2389", TestRuntimes.Mono)]
public void GetConstructor_TypeNotGeneric_ThrowsArgumentException()
{
TypeBuilder type = Helpers.DynamicType(TypeAttributes.Class | TypeAttributes.Public);
Expand Down
9 changes: 3 additions & 6 deletions src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderGetField.cs
View file
Open in desktop
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_DeclaringTypeOfFieldGeneric()
{
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 All @@ -32,14 +32,12 @@ public void GetField()
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/2389", TestRuntimes.Mono)]
public void GetField_TypeNotTypeBuilder_ThrowsArgumentException()
{
AssertExtensions.Throws<ArgumentException>(null, () => TypeBuilder.GetField(typeof(int), typeof(int).GetField("MaxValue")));
AssertExtensions.Throws<ArgumentException>("type", () => TypeBuilder.GetField(typeof(int), typeof(int).GetField("MaxValue")));
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/2389", TestRuntimes.Mono)]
public void GetField_DeclaringTypeOfFieldNotGenericTypeDefinitionOfType_ThrowsArgumentException()
{
ModuleBuilder module = Helpers.DynamicModule();
Expand All @@ -57,7 +55,6 @@ public void GetField_DeclaringTypeOfFieldNotGenericTypeDefinitionOfType_ThrowsAr
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/2389", TestRuntimes.Mono)]
public void GetField_TypeNotGeneric_ThrowsArgumentException()
{
TypeBuilder type = Helpers.DynamicType(TypeAttributes.Class | TypeAttributes.Public);
Expand Down
6 changes: 1 addition & 5 deletions src/libraries/System.Reflection.Emit/tests/TypeBuilder/TypeBuilderGetMethod.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,12 @@ public void GetMethod_ConstructedTypeMethod()
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/2389", TestRuntimes.Mono)]
public void GetMethod_TypeNotTypeBuilder_ThrowsArgumentException()
{
AssertExtensions.Throws<ArgumentException>(null, () => TypeBuilder.GetMethod(typeof(int), typeof(int).GetMethod("Parse", new Type[] { typeof(string) })));
AssertExtensions.Throws<ArgumentException>("type", () => TypeBuilder.GetMethod(typeof(int), typeof(int).GetMethod("Parse", new Type[] { typeof(string) })));
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/2389", TestRuntimes.Mono)]
public void GetMethod_MethodDefinitionNotInTypeGenericDefinition_ThrowsArgumentException()
{
TypeBuilder type = Helpers.DynamicType(TypeAttributes.Class | TypeAttributes.Public);
Expand All @@ -64,7 +62,6 @@ public void GetMethod_MethodDefinitionNotInTypeGenericDefinition_ThrowsArgumentE
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/2389", TestRuntimes.Mono)]
public void GetMethod_MethodNotGenericTypeDefinition_ThrowsArgumentException()
{
ModuleBuilder module = Helpers.DynamicModule();
Expand All @@ -87,7 +84,6 @@ public void GetMethod_MethodNotGenericTypeDefinition_ThrowsArgumentException()
}

[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/2389", TestRuntimes.Mono)]
public void GetMethod_TypeIsNotGeneric_ThrowsArgumentException()
{
TypeBuilder type = Helpers.DynamicType(TypeAttributes.Class | TypeAttributes.Public);
Expand Down
57 changes: 30 additions & 27 deletions src/mono/System.Private.CoreLib/src/System/Reflection/Emit/TypeBuilder.Mono.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1845,25 +1845,21 @@ public GenericTypeParameterBuilder[] DefineGenericParameters(params string[] nam

[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.")]
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2055:UnrecognizedReflectionPattern",
Justification = "Type.MakeGenericType is used to create a typical instantiation")]
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 (!IsValidGetMethodType(type))
throw new ArgumentException(SR.Argument_MustBeTypeBuilder, nameof(type));

if (constructor == null)
throw new NullReferenceException(); //MS raises this instead of an ArgumentNullException
if (type is TypeBuilder && type.ContainsGenericParameters)
type = type.MakeGenericType(type.GetGenericArguments());

if (!constructor.DeclaringType!.IsGenericTypeDefinition)
throw new ArgumentException("constructor declaring type is not a generic type definition", nameof(constructor));
throw new ArgumentException(SR.Argument_ConstructorNeedGenericDeclaringType, nameof(constructor));

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

ConstructorInfo res = type.GetConstructor(constructor);
if (res == null)
Expand All @@ -1874,6 +1870,9 @@ public static ConstructorInfo GetConstructor(Type type, ConstructorInfo construc

private static bool IsValidGetMethodType(Type type)
{
if (type == null)
return false;

if (type is TypeBuilder || type is TypeBuilderInstantiation)
return true;
/*GetMethod() must work with TypeBuilders after CreateType() was called.*/
Expand All @@ -1898,20 +1897,19 @@ private static bool IsValidGetMethodType(Type type)
public static MethodInfo GetMethod(Type type, MethodInfo method)
{
if (!IsValidGetMethodType(type))
throw new ArgumentException("type is not TypeBuilder but " + type.GetType(), nameof(type));
throw new ArgumentException(SR.Argument_MustBeTypeBuilder, nameof(type));

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

if (!type.IsGenericType)
throw new ArgumentException("type is not a generic type", nameof(type));
if (method.IsGenericMethod && !method.IsGenericMethodDefinition)
throw new ArgumentException(SR.Argument_NeedGenericMethodDefinition, nameof(method));

if (!method.DeclaringType!.IsGenericTypeDefinition)
throw new ArgumentException("method declaring type is not a generic type definition", nameof(method));
throw new ArgumentException(SR.Argument_MethodNeedGenericDeclaringType, nameof(method));

if (method.DeclaringType != type.GetGenericTypeDefinition())
throw new ArgumentException("method declaring type is not the generic type definition of type", nameof(method));
if (method == null)
throw new NullReferenceException(); //MS raises this instead of an ArgumentNullException
throw new ArgumentException(SR.Argument_InvalidMethodDeclaringType, nameof(type));

MethodInfo res = type.GetMethod(method);
if (res == null)
Expand All @@ -1920,19 +1918,24 @@ public static MethodInfo GetMethod(Type type, MethodInfo method)
return res;
}

[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2055:UnrecognizedReflectionPattern",
Justification = "Type.MakeGenericType is used to create a typical instantiation")]
public static FieldInfo GetField(Type type, FieldInfo field)
{
if (!type.IsGenericType)
throw new ArgumentException("Type is not a generic type", nameof(type));
if (!IsValidGetMethodType(type))
throw new ArgumentException(SR.Argument_MustBeTypeBuilder, nameof(type));

if (type.IsGenericTypeDefinition)
throw new ArgumentException("Type cannot be a generic type definition", nameof(type));
if (type is TypeBuilder && type.ContainsGenericParameters)
type = type.MakeGenericType(type.GetGenericArguments());

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

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

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

FieldInfo res = type.GetField(field);
if (res == null)
Expand Down