Skip to content

Commit

Permalink
Support field access on GetType() of T constrained to be Enum (#105351)
Browse files Browse the repository at this point in the history
Adds trimming support for instance.GetType().GetFields(), where
instance is a variable of type `T` that is constrained to System.Enum.

This includes a change to have ILLink's TypeProxy track a
TypeReference instead of TypeDefinition, which was necessary to allow
TypeProxy to represent a generic parameter.

Note that this only supports the specific case where `GetType()` is
called on a variable of type `T` that is constrained to `Enum`. A
variable of type `Enum` is not supported, so the following will still
warn:


```csharp
static void M(Enum v) {
    v.GetType().GetFields();
}
```
  • Loading branch information
sbomer authored Jul 25, 2024
1 parent 07e95aa commit 1d27457
Show file tree
Hide file tree
Showing 13 changed files with 363 additions and 53 deletions.
4 changes: 3 additions & 1 deletion src/coreclr/tools/Common/Compiler/Dataflow/ParameterProxy.cs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,9 @@ namespace ILLink.Shared.TypeSystemProxy
internal partial struct ParameterProxy
{
public partial ReferenceKind GetReferenceKind() => Method.Method.ParameterReferenceKind((int)Index);
public TypeDesc ParameterType => IsImplicitThis ? Method.Method.OwningType : Method.Method.Signature[MetadataIndex];
public TypeDesc ParameterType => IsImplicitThis
? Method.Method.OwningType
: Method.Method.Signature[MetadataIndex].InstantiateSignature (Method.Method.OwningType.Instantiation, Method.Method.Instantiation);

public partial string GetDisplayName() => IsImplicitThis ? "this"
: (Method.Method is EcmaMethod ecmaMethod) ? ecmaMethod.GetParameterDisplayName(MetadataIndex)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Reflection;
using ILCompiler;
using ILCompiler.Dataflow;
Expand Down Expand Up @@ -377,8 +378,7 @@ private partial bool TryHandleIntrinsic (
// Note that valueNode can be statically typed in IL as some generic argument type.
// For example:
// void Method<T>(T instance) { instance.GetType().... }
// Currently this case will end up with null StaticType - since there's no typedef for the generic argument type.
// But it could be that T is annotated with for example PublicMethods:
// It could be that T is annotated with for example PublicMethods:
// void Method<[DAM(PublicMethods)] T>(T instance) { instance.GetType().GetMethod("Test"); }
// In this case it's in theory possible to handle it, by treating the T basically as a base class
// for the actual type of "instance". But the analysis for this would be pretty complicated (as the marking
Expand All @@ -392,8 +392,28 @@ private partial bool TryHandleIntrinsic (
TypeDesc? staticType = (valueNode as IValueWithStaticType)?.StaticType?.Type;
if (staticType is null || (!staticType.IsDefType && !staticType.IsArray))
{
// We don't know anything about the type GetType was called on. Track this as a usual "result of a method call without any annotations"
AddReturnValue(_reflectionMarker.Annotations.GetMethodReturnValue(calledMethod, _isNewObj));
DynamicallyAccessedMemberTypes annotation = default;
if (staticType is GenericParameterDesc genericParam)
{
foreach (TypeDesc constraint in genericParam.TypeConstraints)
{
if (constraint.IsWellKnownType(Internal.TypeSystem.WellKnownType.Enum))
{
annotation = DynamicallyAccessedMemberTypes.PublicFields;
break;
}
}
}

if (annotation != default)
{
AddReturnValue(_reflectionMarker.Annotations.GetMethodReturnValue(calledMethod, _isNewObj, annotation));
}
else
{
// We don't know anything about the type GetType was called on. Track this as a usual "result of a method call without any annotations"
AddReturnValue(_reflectionMarker.Annotations.GetMethodReturnValue(calledMethod, _isNewObj));
}
}
else if (staticType.IsSealed() || staticType.IsTypeOf("System", "Delegate"))
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,9 @@ namespace Mono.Linker.Tests.TestCasesRunner

public AssemblyQualifiedToken (string? assemblyName, int token) => (AssemblyName, Token) = (assemblyName, token);

public AssemblyQualifiedToken (TypeSystemEntity entity) =>
public AssemblyQualifiedToken (TypeSystemEntity entity) {
if (entity is MethodForInstantiatedType instantiatedMethod)
entity = instantiatedMethod.GetTypicalMethodDefinition ();
(AssemblyName, Token) = entity switch {
EcmaType type => (type.Module.Assembly.GetName ().Name, MetadataTokens.GetToken (type.Handle)),
EcmaMethod method => (method.Module.Assembly.GetName ().Name, MetadataTokens.GetToken (method.Handle)),
Expand All @@ -31,6 +33,7 @@ public AssemblyQualifiedToken (TypeSystemEntity entity) =>
MetadataType mt when mt.GetType().Name == "BoxedValueType" => (null, 0),
_ => throw new NotSupportedException ($"The infra doesn't support getting a token for {entity} yet.")
};
}

public AssemblyQualifiedToken (IMemberDefinition member) =>
(AssemblyName, Token) = member switch {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System.Collections.Generic;
using System.Diagnostics;
using System.Diagnostics.CodeAnalysis;
using System.Reflection;
using ILLink.RoslynAnalyzer;
using ILLink.RoslynAnalyzer.DataFlow;
Expand Down Expand Up @@ -78,16 +79,23 @@ private partial bool TryHandleIntrinsic (
// In this case to get correct results, trimmer would have to mark all public methods on Derived. Which
// currently it won't do.

// To emulate IL tools behavior (trimmer, NativeAOT compiler), we're going to intentionally "forget" the static type
// if it is a generic argument type.

ITypeSymbol? staticType = (valueNode as IValueWithStaticType)?.StaticType?.Type;
if (staticType?.TypeKind == TypeKind.TypeParameter)
staticType = null;

if (staticType is null) {
// We don't know anything about the type GetType was called on. Track this as a usual "result of a method call without any annotations"
AddReturnValue (FlowAnnotations.Instance.GetMethodReturnValue (calledMethod, _isNewObj));
ITypeSymbol? staticTypeDef = staticType?.OriginalDefinition;
if (staticType is null || staticTypeDef is null || staticType is ITypeParameterSymbol) {
DynamicallyAccessedMemberTypes annotation = default;
if (staticType is ITypeParameterSymbol genericParam) {
foreach (var constraintType in genericParam.ConstraintTypes) {
if (constraintType.IsTypeOf ("System", "Enum"))
annotation = DynamicallyAccessedMemberTypes.PublicFields;
}
}

if (annotation != default) {
AddReturnValue (FlowAnnotations.Instance.GetMethodReturnValue (calledMethod, _isNewObj, annotation));
} else {
// We don't know anything about the type GetType was called on. Track this as a usual "result of a method call without any annotations"
AddReturnValue (FlowAnnotations.Instance.GetMethodReturnValue (calledMethod, _isNewObj));
}
} else if (staticType.IsSealed || staticType.IsTypeOf ("System", "Delegate") || staticType.TypeKind == TypeKind.Array) {
// We can treat this one the same as if it was a typeof() expression

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ public static IEnumerable<IMetadataTokenProvider> GetDynamicallyAccessedMembers
}

if (memberTypes.HasFlag (DynamicallyAccessedMemberTypes.NonPublicNestedTypes)) {
foreach (var nested in typeDefinition.GetNestedTypesOnType (filter: null, bindingFlags: BindingFlags.NonPublic)) {
foreach (var nested in typeDefinition.GetNestedTypesOnType (context, filter: null, bindingFlags: BindingFlags.NonPublic)) {
yield return nested;
var members = new List<IMetadataTokenProvider> ();
nested.GetAllOnType (context, declaredOnly: false, members);
Expand All @@ -76,7 +76,7 @@ public static IEnumerable<IMetadataTokenProvider> GetDynamicallyAccessedMembers
}

if (memberTypes.HasFlag (DynamicallyAccessedMemberTypes.PublicNestedTypes)) {
foreach (var nested in typeDefinition.GetNestedTypesOnType (filter: null, bindingFlags: BindingFlags.Public)) {
foreach (var nested in typeDefinition.GetNestedTypesOnType (context, filter: null, bindingFlags: BindingFlags.Public)) {
yield return nested;
var members = new List<IMetadataTokenProvider> ();
nested.GetAllOnType (context, declaredOnly: false, members);
Expand Down Expand Up @@ -136,9 +136,9 @@ public static IEnumerable<MethodDefinition> GetConstructorsOnType (this TypeDefi
}
}

public static IEnumerable<MethodDefinition> GetMethodsOnTypeHierarchy (this TypeDefinition thisType, LinkContext context, Func<MethodDefinition, bool>? filter, BindingFlags? bindingFlags = null)
public static IEnumerable<MethodDefinition> GetMethodsOnTypeHierarchy (this TypeReference thisType, LinkContext context, Func<MethodDefinition, bool>? filter, BindingFlags? bindingFlags = null)
{
TypeDefinition? type = thisType;
TypeDefinition? type = thisType.ResolveToTypeDefinition (context);
bool onBaseType = false;
while (type != null) {
foreach (var method in type.Methods) {
Expand Down Expand Up @@ -220,8 +220,11 @@ public static IEnumerable<FieldDefinition> GetFieldsOnTypeHierarchy (this TypeDe
}
}

public static IEnumerable<TypeDefinition> GetNestedTypesOnType (this TypeDefinition type, Func<TypeDefinition, bool>? filter, BindingFlags? bindingFlags = BindingFlags.Default)
public static IEnumerable<TypeDefinition> GetNestedTypesOnType (this TypeReference typeRef, LinkContext context, Func<TypeDefinition, bool>? filter, BindingFlags? bindingFlags = BindingFlags.Default)
{
if (typeRef.ResolveToTypeDefinition (context) is not TypeDefinition type)
yield break;

foreach (var nestedType in type.NestedTypes) {
if (filter != null && !filter (nestedType))
continue;
Expand Down
4 changes: 2 additions & 2 deletions src/tools/illink/src/linker/Linker.Dataflow/FieldValue.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
using ILLink.Shared.DataFlow;
using Mono.Linker;
using FieldDefinition = Mono.Cecil.FieldDefinition;
using TypeDefinition = Mono.Cecil.TypeDefinition;
using TypeReference = Mono.Cecil.TypeReference;


namespace ILLink.Shared.TrimAnalysis
Expand All @@ -17,7 +17,7 @@ namespace ILLink.Shared.TrimAnalysis
/// </summary>
internal sealed partial record FieldValue
{
public FieldValue (TypeDefinition? staticType, FieldDefinition fieldToLoad, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
public FieldValue (TypeReference? staticType, FieldDefinition fieldToLoad, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
{
StaticType = staticType == null ? null : new (staticType);
Field = fieldToLoad;
Expand Down
10 changes: 7 additions & 3 deletions src/tools/illink/src/linker/Linker.Dataflow/FlowAnnotations.cs
Original file line number Diff line number Diff line change
Expand Up @@ -687,8 +687,10 @@ public FieldAnnotation (FieldDefinition field, DynamicallyAccessedMemberTypes an
internal partial bool MethodRequiresDataFlowAnalysis (MethodProxy method)
=> RequiresDataFlowAnalysis (method.Method);

#pragma warning disable CA1822 // Mark members as static - Should be an instance method for consistency
internal partial MethodReturnValue GetMethodReturnValue (MethodProxy method, bool isNewObj, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
=> MethodReturnValue.Create (method.Method, isNewObj, dynamicallyAccessedMemberTypes, _context);
=> MethodReturnValue.Create (method.Method, isNewObj, dynamicallyAccessedMemberTypes);
#pragma warning restore CA1822 // Mark members as static

internal partial MethodReturnValue GetMethodReturnValue (MethodProxy method, bool isNewObj)
=> GetMethodReturnValue (method, isNewObj, GetReturnParameterAnnotation (method.Method));
Expand All @@ -701,8 +703,10 @@ internal partial GenericParameterValue GetGenericParameterValue (GenericParamete
internal partial GenericParameterValue GetGenericParameterValue (GenericParameterProxy genericParameter)
=> new GenericParameterValue (genericParameter.GenericParameter, GetGenericParameterAnnotation (genericParameter.GenericParameter));

#pragma warning disable CA1822 // Mark members as static - Should be an instance method for consistency
internal partial MethodParameterValue GetMethodParameterValue (ParameterProxy param, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
=> new (param.ParameterType.ResolveToTypeDefinition (_context), param, dynamicallyAccessedMemberTypes);
=> new (param.ParameterType, param, dynamicallyAccessedMemberTypes);
#pragma warning restore CA1822 // Mark members as static

internal partial MethodParameterValue GetMethodParameterValue (ParameterProxy param)
=> GetMethodParameterValue (param, GetParameterAnnotation (param));
Expand Down Expand Up @@ -730,7 +734,7 @@ internal SingleValue GetFieldValue (FieldDefinition field)
=> field.Name switch {
"EmptyTypes" when field.DeclaringType.IsTypeOf (WellKnownType.System_Type) => ArrayValue.Create (0, field.DeclaringType),
"Empty" when field.DeclaringType.IsTypeOf (WellKnownType.System_String) => new KnownStringValue (string.Empty),
_ => new FieldValue (field.FieldType.ResolveToTypeDefinition (_context), field, GetFieldAnnotation (field))
_ => new FieldValue (field.FieldType, field, GetFieldAnnotation (field))
};

internal SingleValue GetTypeValueFromGenericArgument (TypeReference genericArgument)
Expand Down
32 changes: 22 additions & 10 deletions src/tools/illink/src/linker/Linker.Dataflow/HandleCallAction.cs
Original file line number Diff line number Diff line change
Expand Up @@ -108,8 +108,7 @@ private partial bool TryHandleIntrinsic (
// Note that valueNode can be statically typed in IL as some generic argument type.
// For example:
// void Method<T>(T instance) { instance.GetType().... }
// Currently this case will end up with null StaticType - since there's no typedef for the generic argument type.
// But it could be that T is annotated with for example PublicMethods:
// It could be that T is annotated with for example PublicMethods:
// void Method<[DAM(PublicMethods)] T>(T instance) { instance.GetType().GetMethod("Test"); }
// In this case it's in theory possible to handle it, by treating the T basically as a base class
// for the actual type of "instance". But the analysis for this would be pretty complicated (as the marking
Expand All @@ -120,11 +119,24 @@ private partial bool TryHandleIntrinsic (
// In this case to get correct results, trimmer would have to mark all public methods on Derived. Which
// currently it won't do.

TypeDefinition? staticType = (valueNode as IValueWithStaticType)?.StaticType?.Type;
if (staticType is null) {
// We don't know anything about the type GetType was called on. Track this as a usual result of a method call without any annotations
AddReturnValue (_context.Annotations.FlowAnnotations.GetMethodReturnValue (calledMethod, _isNewObj));
} else if (staticType.IsSealed || staticType.IsTypeOf ("System", "Delegate") || staticType.IsTypeOf ("System", "Array")) {
TypeReference? staticType = (valueNode as IValueWithStaticType)?.StaticType?.Type;
TypeDefinition? staticTypeDef = staticType?.ResolveToTypeDefinition (_context);
if (staticType is null || staticTypeDef is null) {
DynamicallyAccessedMemberTypes annotation = default;
if (staticType is GenericParameter genericParam && genericParam.HasConstraints) {
foreach (var constraint in genericParam.Constraints) {
if (constraint.ConstraintType.IsTypeOf ("System", "Enum"))
annotation = DynamicallyAccessedMemberTypes.PublicFields;
}
}

if (annotation != default) {
AddReturnValue (_context.Annotations.FlowAnnotations.GetMethodReturnValue (calledMethod, _isNewObj, annotation));
} else {
// We don't know anything about the type GetType was called on. Track this as a usual result of a method call without any annotations
AddReturnValue (_context.Annotations.FlowAnnotations.GetMethodReturnValue (calledMethod, _isNewObj));
}
} else if (staticTypeDef.IsSealed || staticTypeDef.IsTypeOf ("System", "Delegate") || staticTypeDef.IsTypeOf ("System", "Array")) {
// We can treat this one the same as if it was a typeof() expression

// We can allow Object.GetType to be modeled as System.Delegate because we keep all methods
Expand All @@ -147,7 +159,7 @@ private partial bool TryHandleIntrinsic (
_reflectionMarker.MarkType (_diagnosticContext.Origin, staticType);

var annotation = _markStep.DynamicallyAccessedMembersTypeHierarchy
.ApplyDynamicallyAccessedMembersToTypeHierarchy (staticType);
.ApplyDynamicallyAccessedMembersToTypeHierarchy (staticTypeDef);

// Return a value which is "unknown type" with annotation. For now we'll use the return value node
// for the method, which means we're loosing the information about which staticType this
Expand Down Expand Up @@ -200,13 +212,13 @@ private partial IEnumerable<SystemReflectionMethodBaseValue> GetMethodsOnTypeHie

private partial IEnumerable<SystemTypeValue> GetNestedTypesOnType (TypeProxy type, string name, BindingFlags? bindingFlags)
{
foreach (var nestedType in type.Type.GetNestedTypesOnType (t => t.Name == name, bindingFlags))
foreach (var nestedType in type.Type.GetNestedTypesOnType (_context, t => t.Name == name, bindingFlags))
yield return new SystemTypeValue (new TypeProxy (nestedType));
}

private partial bool TryGetBaseType (TypeProxy type, out TypeProxy? baseType)
{
if (type.Type.BaseType is TypeReference baseTypeRef && _context.TryResolve (baseTypeRef) is TypeDefinition baseTypeDefinition) {
if (type.Type.ResolveToTypeDefinition (_context)?.BaseType is TypeReference baseTypeRef && _context.TryResolve (baseTypeRef) is TypeDefinition baseTypeDefinition) {
baseType = new TypeProxy (baseTypeDefinition);
return true;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@

using System.Diagnostics.CodeAnalysis;
using ILLink.Shared.TypeSystemProxy;
using Mono.Linker.Dataflow;
using TypeDefinition = Mono.Cecil.TypeDefinition;
using TypeReference = Mono.Cecil.TypeReference;


namespace ILLink.Shared.TrimAnalysis
Expand All @@ -15,7 +14,7 @@ namespace ILLink.Shared.TrimAnalysis
/// </summary>
internal partial record MethodParameterValue
{
public MethodParameterValue (TypeDefinition? staticType, ParameterProxy param, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
public MethodParameterValue (TypeReference? staticType, ParameterProxy param, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
{
StaticType = staticType == null ? null : new (staticType);
DynamicallyAccessedMemberTypes = dynamicallyAccessedMemberTypes;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,8 @@
using System.Diagnostics.CodeAnalysis;
using ILLink.Shared.DataFlow;
using Mono.Cecil;
using Mono.Linker;
using Mono.Linker.Dataflow;
using TypeDefinition = Mono.Cecil.TypeDefinition;
using TypeReference = Mono.Cecil.TypeReference;


namespace ILLink.Shared.TrimAnalysis
Expand All @@ -18,14 +17,14 @@ namespace ILLink.Shared.TrimAnalysis
/// </summary>
internal partial record MethodReturnValue
{
public static MethodReturnValue Create (MethodDefinition method, bool isNewObj, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes, LinkContext context)
public static MethodReturnValue Create (MethodDefinition method, bool isNewObj, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
{
Debug.Assert (!isNewObj || method.IsConstructor, "isNewObj can only be true for constructors");
var staticType = isNewObj ? method.DeclaringType : method.ReturnType.ResolveToTypeDefinition (context);
var staticType = isNewObj ? method.DeclaringType : method.ReturnType;
return new MethodReturnValue (staticType, method, dynamicallyAccessedMemberTypes);
}

private MethodReturnValue (TypeDefinition? staticType, MethodDefinition method, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
private MethodReturnValue (TypeReference? staticType, MethodDefinition method, DynamicallyAccessedMemberTypes dynamicallyAccessedMemberTypes)
{
StaticType = staticType == null ? null : new (staticType);
Method = method;
Expand Down
Loading

0 comments on commit 1d27457

Please # to comment.