Skip to content

Commit

Permalink
Mark generic arguments of dynamically accessed types passed as string (…
Browse files Browse the repository at this point in the history 
…dotnet#1566)

* Mark dynamically accessed generic arguments

* Clean test

* Use MarkType for keeping generic args
Enable ComplexTypeHandling tests

* Use MarkTypeVisibleToReflection for generic arguments
Clean tests

* MarkType always

* Check for array types in TypeNameResolver

* Mark System.Array instead of its element type

* Whitespace

* PR feedback

* Pattern match
Whitespace

* Add extension method ResolveToMainTypeDefinition

* Add comment

* Use ResolveToMainTypeDefinition
  • Loading branch information
@mateoatr
mateoatr committed Oct 26, 2020
1 parent 63377a0 commit 2a4eb66
Show file tree
Hide file tree
Showing 8 changed files with 112 additions and 30 deletions.
6 changes: 3 additions & 3 deletions ...l/corert/src/System.Private.CoreLib/src/System/Reflection/Runtime/TypeParsing/TypeName.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -140,15 +140,15 @@ internal sealed class MultiDimArrayTypeName : HasElementTypeName
public MultiDimArrayTypeName(TypeName elementTypeName, int rank)
: base(elementTypeName)
{
_rank = rank;
Rank = rank;
}

public sealed override string ToString()
{
return ElementTypeName + "[" + (_rank == 1 ? "*" : new string(',', _rank - 1)) + "]";
return ElementTypeName + "[" + (Rank == 1 ? "*" : new string(',', Rank - 1)) + "]";
}

private int _rank;
public int Rank { get; }
}

//
Expand Down
2 changes: 1 addition & 1 deletion src/linker/Linker.Dataflow/MethodBodyScanner.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -709,7 +709,7 @@ private static void ScanLdtoken (Instruction operation, Stack<StackSlot> current
}

if (operation.Operand is TypeReference typeReference) {
var resolvedReference = typeReference.Resolve ();
var resolvedReference = typeReference.ResolveToMainTypeDefinition ();
if (resolvedReference != null) {
StackSlot slot = new StackSlot (new RuntimeTypeHandleValue (resolvedReference));
currentStack.Push (slot);
Expand Down
36 changes: 23 additions & 13 deletions src/linker/Linker.Dataflow/ReflectionMethodBodyScanner.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ static ValueNode GetValueNodeForCustomAttributeArgument (CustomAttributeArgument
{
ValueNode valueNode;
if (argument.Type.Name == "Type") {
TypeDefinition referencedType = ((TypeReference) argument.Value).Resolve ();
TypeDefinition referencedType = ((TypeReference) argument.Value).ResolveToMainTypeDefinition ();
valueNode = referencedType == null ? null : new SystemTypeValue (referencedType);
} else if (argument.Type.MetadataType == MetadataType.String) {
valueNode = new KnownStringValue ((string) argument.Value);
Expand All @@ -130,22 +130,22 @@ public void ProcessGenericArgumentDataFlow (GenericParameter genericParameter, T
var annotation = _context.Annotations.FlowAnnotations.GetGenericParameterAnnotation (genericParameter);
Debug.Assert (annotation != DynamicallyAccessedMemberTypes.None);

ValueNode valueNode = GetValueNodeFromGenericArgument (genericArgument);
ValueNode valueNode = GetTypeValueNodeFromGenericArgument (genericArgument);
bool enableReflectionPatternReporting = !(source is MethodDefinition sourceMethod) || ShouldEnableReflectionPatternReporting (sourceMethod);

var reflectionContext = new ReflectionPatternContext (_context, enableReflectionPatternReporting, source, genericParameter);
reflectionContext.AnalyzingPattern ();
RequireDynamicallyAccessedMembers (ref reflectionContext, annotation, valueNode, genericParameter);
}

ValueNode GetValueNodeFromGenericArgument (TypeReference genericArgument)
ValueNode GetTypeValueNodeFromGenericArgument (TypeReference genericArgument)
{
if (genericArgument is GenericParameter inputGenericParameter) {
// Technically this should be a new value node type as it's not a System.Type instance representation, but just the generic parameter
// That said we only use it to perform the dynamically accessed members checks and for that purpose treating it as System.Type is perfectly valid.
return new SystemTypeForGenericParameterValue (inputGenericParameter, _context.Annotations.FlowAnnotations.GetGenericParameterAnnotation (inputGenericParameter));
} else {
TypeDefinition genericArgumentTypeDef = genericArgument.Resolve ();
TypeDefinition genericArgumentTypeDef = genericArgument.ResolveToMainTypeDefinition ();
if (genericArgumentTypeDef != null) {
return new SystemTypeValue (genericArgumentTypeDef);
} else {
Expand Down Expand Up @@ -752,13 +752,13 @@ public override bool HandleCall (MethodBody callingMethodBody, MethodReference c
if (methodParam.ParameterIndex == 0) {
staticType = callingMethodDefinition.DeclaringType;
} else {
staticType = callingMethodDefinition.Parameters[methodParam.ParameterIndex - 1].ParameterType.Resolve ();
staticType = callingMethodDefinition.Parameters[methodParam.ParameterIndex - 1].ParameterType.ResolveToMainTypeDefinition ();
}
} else {
staticType = callingMethodDefinition.Parameters[methodParam.ParameterIndex].ParameterType.Resolve ();
staticType = callingMethodDefinition.Parameters[methodParam.ParameterIndex].ParameterType.ResolveToMainTypeDefinition ();
}
} else if (methodParams[0] is LoadFieldValue loadedField) {
staticType = loadedField.Field.FieldType.Resolve ();
staticType = loadedField.Field.FieldType.ResolveToMainTypeDefinition ();
}

if (staticType != null) {
Expand Down Expand Up @@ -802,7 +802,7 @@ public override bool HandleCall (MethodBody callingMethodBody, MethodReference c
foreach (var typeNameValue in methodParams[0].UniqueValues ()) {
if (typeNameValue is KnownStringValue knownStringValue) {
TypeReference foundTypeRef = _context.TypeNameResolver.ResolveTypeName (knownStringValue.Contents);
TypeDefinition foundType = foundTypeRef?.Resolve ();
TypeDefinition foundType = foundTypeRef?.ResolveToMainTypeDefinition ();
if (foundType == null) {
// Intentionally ignore - it's not wrong for code to call Type.GetType on non-existing name, the code might expect null/exception back.
reflectionContext.RecordHandledPattern ();
Expand Down Expand Up @@ -1161,7 +1161,7 @@ public override bool HandleCall (MethodBody callingMethodBody, MethodReference c
RequireDynamicallyAccessedMembers (
ref reflectionContext,
DynamicallyAccessedMemberTypes.PublicParameterlessConstructor,
GetValueNodeFromGenericArgument (genericCalledMethod.GenericArguments[0]),
GetTypeValueNodeFromGenericArgument (genericCalledMethod.GenericArguments[0]),
calledMethodDefinition.GenericParameters[0]);
}
break;
Expand Down Expand Up @@ -1340,11 +1340,13 @@ void ProcessCreateInstanceByName (ref ReflectionPatternContext reflectionContext
continue;
}

var resolvedType = _context.TypeNameResolver.ResolveTypeName (resolvedAssembly, typeNameStringValue.Contents)?.Resolve ();
if (resolvedType == null) {
var typeRef = _context.TypeNameResolver.ResolveTypeName (resolvedAssembly, typeNameStringValue.Contents);
var resolvedType = typeRef?.Resolve ();
if (resolvedType == null || typeRef is ArrayType) {
// It's not wrong to have a reference to non-existing type - the code may well expect to get an exception in this case
// Note that we did find the assembly, so it's not a linker config problem, it's either intentional, or wrong versions of assemblies
// but linker can't know that.
// but linker can't know that. In case a user tries to create an array using System.Activator we should simply ignore it, the user
// might expect an exception to be thrown.
reflectionContext.RecordHandledPattern ();
continue;
}
Expand Down Expand Up @@ -1571,11 +1573,13 @@ void RequireDynamicallyAccessedMembers (ref ReflectionPatternContext reflectionC
} else if (uniqueValue is SystemTypeValue systemTypeValue) {
MarkTypeForDynamicallyAccessedMembers (ref reflectionContext, systemTypeValue.TypeRepresented, requiredMemberTypes);
} else if (uniqueValue is KnownStringValue knownStringValue) {
TypeDefinition foundType = _context.TypeNameResolver.ResolveTypeName (knownStringValue.Contents)?.Resolve ();
TypeReference typeRef = _context.TypeNameResolver.ResolveTypeName (knownStringValue.Contents);
TypeDefinition foundType = typeRef?.ResolveToMainTypeDefinition ();
if (foundType == null) {
// Intentionally ignore - it's not wrong for code to call Type.GetType on non-existing name, the code might expect null/exception back.
reflectionContext.RecordHandledPattern ();
} else {
MarkType (ref reflectionContext, typeRef);
MarkTypeForDynamicallyAccessedMembers (ref reflectionContext, foundType, requiredMemberTypes);
}
} else if (uniqueValue == NullValue.Instance) {
Expand Down Expand Up @@ -1649,6 +1653,12 @@ void MarkTypeForDynamicallyAccessedMembers (ref ReflectionPatternContext reflect
}
}

void MarkType (ref ReflectionPatternContext reflectionContext, TypeReference typeReference)
{
var source = reflectionContext.Source;
reflectionContext.RecordRecognizedPattern (typeReference?.Resolve (), () => _markStep.MarkTypeVisibleToReflection (typeReference, new DependencyInfo (DependencyKind.AccessedViaReflection, source), source));
}

void MarkMethod (ref ReflectionPatternContext reflectionContext, MethodDefinition method)
{
var source = reflectionContext.Source;
Expand Down
9 changes: 8 additions & 1 deletion src/linker/Linker/TypeNameResolver.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,14 @@ TypeReference ResolveTypeName (AssemblyDefinition assembly, TypeName typeName)
if (elementType == null)
return null;

return elementType;
return typeName switch
{
ArrayTypeName _ => new ArrayType (elementType),
MultiDimArrayTypeName multiDimArrayTypeName => new ArrayType (elementType, multiDimArrayTypeName.Rank),
ByRefTypeName _ => new ByReferenceType (elementType),
PointerTypeName _ => new PointerType (elementType),
_ => elementType
};
}

return assembly.MainModule.GetType (typeName.ToString ());
Expand Down
12 changes: 12 additions & 0 deletions src/linker/Linker/TypeReferenceExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -375,5 +375,17 @@ public static bool IsSubclassOf (this TypeReference type, string ns, string name

return false;
}

// Array types that are dynamically accessed should resolve to System.Array instead of its element type - which is what Cecil resolves to.
// Any data flow annotations placed on a type parameter which receives an array type apply to the array itself. None of the members in its
// element type should be marked.
public static TypeDefinition ResolveToMainTypeDefinition (this TypeReference type)
{
return type switch
{
ArrayType _ => type.Module.ImportReference (typeof (Array))?.Resolve (),
_ => type?.Resolve ()
};
}
}
}
51 changes: 48 additions & 3 deletions test/Mono.Linker.Tests.Cases/DataFlow/ApplyTypeAnnotations.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,7 @@ class FromStringConstantWithAnnotationTestType
{
}

// Issue: https://github.com/mono/linker/issues/1537
//[Kept]
//[KeptMember (".ctor()")]
[Kept]
class FromStringConstantWithGenericInner
{
}
Expand All @@ -143,10 +141,57 @@ class FromStringConstantWithGeneric<T>
public T GetValue () { return default (T); }
}

[Kept]
class FromStringConstantWithGenericInnerInner
{
[Kept]
public void Method ()
{
}

int unusedField;
}

[Kept]
class FromStringConstantWithGenericInnerOne<
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
T>
{
}

[Kept]
class FromStringConstantWithGenericInnerTwo
{
void UnusedMethod ()
{
}
}

[Kept]
class FromStringConstantWitGenericInnerMultiDimArray
{
}

[Kept]
class FromStringConstantWithMultiDimArray
{
public void UnusedMethod () { }
}

[Kept]
[KeptMember (".ctor()")]
class FromStringConstantWithGenericTwoParameters<T, S>
{
}

[Kept]
static void TestFromStringConstantWithGeneric ()
{
RequireCombinationOnString ("Mono.Linker.Tests.Cases.DataFlow.ApplyTypeAnnotations+FromStringConstantWithGeneric`1[[Mono.Linker.Tests.Cases.DataFlow.ApplyTypeAnnotations+FromStringConstantWithGenericInner]]");
RequireCombinationOnString ("Mono.Linker.Tests.Cases.DataFlow.ApplyTypeAnnotations+FromStringConstantWithGenericTwoParameters`2[Mono.Linker.Tests.Cases.DataFlow.ApplyTypeAnnotations+FromStringConstantWithGenericInnerOne`1[Mono.Linker.Tests.Cases.DataFlow.ApplyTypeAnnotations+FromStringConstantWithGenericInnerInner],Mono.Linker.Tests.Cases.DataFlow.ApplyTypeAnnotations+FromStringConstantWithGenericInnerTwo]");
RequireCombinationOnString ("Mono.Linker.Tests.Cases.DataFlow.ApplyTypeAnnotations+FromStringConstantWithGeneric`1[[Mono.Linker.Tests.Cases.DataFlow.ApplyTypeAnnotations+FromStringConstantWitGenericInnerMultiDimArray[,]]]");
RequireCombinationOnString ("Mono.Linker.Tests.Cases.DataFlow.ApplyTypeAnnotations+FromStringConstantWithMultiDimArray[,]");
}

[Kept]
Expand Down
15 changes: 6 additions & 9 deletions test/Mono.Linker.Tests.Cases/DataFlow/ComplexTypeHandling.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@

namespace Mono.Linker.Tests.Cases.DataFlow
{
[IgnoreTestCase ("Active issue https://github.com/mono/linker/issues/1559")]
public class ComplexTypeHandling
{
public static void Main ()
Expand All @@ -33,7 +32,6 @@ class ArrayElementType
{
public ArrayElementType () { }

[Kept]
public void PublicMethod () { }

private int _privateField;
Expand Down Expand Up @@ -62,7 +60,6 @@ class ArrayElementInGenericType
{
public ArrayElementInGenericType () { }

[Kept]
public void PublicMethod () { }

private int _privateField;
Expand All @@ -89,6 +86,7 @@ static void TestGenericArrayOnGeneric ()
RequirePublicMethodsOnArrayOfGenericParameter<ArrayElementInGenericType> ();
}

[Kept]
static void RequirePublicMethodsOnArrayOfGenericParameter<T> ()
{
_ = new RequirePublicMethodsGeneric<T[]> ();
Expand All @@ -97,7 +95,7 @@ static void RequirePublicMethodsOnArrayOfGenericParameter<T> ()
[Kept]
sealed class ArrayGetTypeFromMethodParamElement
{
[Kept] // This is a bug - the method should not be marked, instead Array.* should be marked
// This method should not be marked, instead Array.* should be marked
public void PublicMethod () { }
}

Expand All @@ -116,7 +114,7 @@ static void TestArrayGetTypeFromMethodParam ()
[Kept]
sealed class ArrayGetTypeFromFieldElement
{
[Kept] // This is a bug - the method should not be marked, instead Array.* should be marked
// This method should not be marked, instead Array.* should be marked
public void PublicMethod () { }
}

Expand All @@ -132,7 +130,7 @@ static void TestArrayGetTypeFromField ()
[Kept]
sealed class ArrayTypeGetTypeElement
{
[Kept] // This is a bug - the method should not be marked, instead Array.* should be marked
// This method should not be marked, instead Array.* should be marked
public void PublicMethod () { }
}

Expand All @@ -142,10 +140,9 @@ static void TestArrayTypeGetType ()
RequirePublicMethods (Type.GetType ("Mono.Linker.Tests.Cases.DataFlow.ComplexTypeHandling+ArrayTypeGetTypeElement[]"));
}

[Kept]
// Nothing should be marked as CreateInstance doesn't work on arrays
class ArrayCreateInstanceByNameElement
{
[Kept] // This is a bug - the .ctor should not be marked - in fact in this case nothing should be marked as CreateInstance doesn't work on arrays
public ArrayCreateInstanceByNameElement ()
{
}
Expand All @@ -160,7 +157,7 @@ static void TestArrayCreateInstanceByName ()
[Kept]
class ArrayInAttributeParamElement
{
[Kept] // This is a bug - the method should not be marked, instead Array.* should be marked
// This method should not be marked, instead Array.* should be marked
public void PublicMethod () { }
}

Expand Down
11 changes: 11 additions & 0 deletions test/Mono.Linker.Tests.Cases/Reflection/PropertyUsedViaReflection.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ public static void Main ()
TestNullName ();
TestEmptyName ();
TestNonExistingName ();
TestPropertyOfArray ();
TestNullType ();
TestDataFlowType ();
TestIfElse (1);
Expand Down Expand Up @@ -82,6 +83,16 @@ static void TestNonExistingName ()
var property = typeof (PropertyUsedViaReflection).GetProperty ("NonExisting");
}

[Kept]
[RecognizedReflectionAccessPattern (
typeof (Type), nameof (Type.GetProperty), new Type[] { typeof (string) },
typeof (Array), nameof (Array.LongLength))]
static void TestPropertyOfArray ()
{
var property = typeof (int[]).GetProperty ("LongLength");
property.GetValue (null);
}

[Kept]
static void TestNullType ()
{
Expand Down

0 comments on commit 2a4eb66

Please sign in to comment.