Skip to content

Commit

Permalink
Don't keep members of pointer or byref element types (#106215)
Browse files Browse the repository at this point in the history
Fixes the following warnings in ILLink:

```csharp
var type = Type.GetType ("ElementType&, test");
RequireConstructor(type); // IL2026

[RequiresUnreferencedCode("ElementType")]
class ElementType {}

static void RequireConstructor([DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicParameterlessConstructor)] Type type) { }
```

This warns for reflection access to the ElementType constructor, even
though the byref type has no constructor. NativeAot doesn't produce
this warning.
  • Loading branch information
sbomer authored Aug 29, 2024
1 parent fbf4672 commit 2694613
Show file tree
Hide file tree
Showing 6 changed files with 63 additions and 49 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -245,8 +245,8 @@ private partial bool TryResolveTypeNameForCreateInstanceAndMark (in MethodProxy
return false;
}

if (!_reflectionMarker.TryResolveTypeNameAndMark (resolvedAssembly, typeName, _diagnosticContext, out TypeDefinition? resolvedTypeDefinition)
|| resolvedTypeDefinition.IsTypeOf (WellKnownType.System_Array)) {
if (!_reflectionMarker.TryResolveTypeNameAndMark (resolvedAssembly, typeName, _diagnosticContext, out TypeReference? foundType)
|| foundType.IsTypeOf (WellKnownType.System_Array)) {
// 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 ILLink config problem, it's either intentional, or wrong versions of assemblies
// but ILLink can't know that. In case a user tries to create an array using System.Activator we should simply ignore it, the user
Expand All @@ -255,7 +255,7 @@ private partial bool TryResolveTypeNameForCreateInstanceAndMark (in MethodProxy
return false;
}

resolvedType = new TypeProxy (resolvedTypeDefinition, _context);
resolvedType = new TypeProxy (foundType, _context);
return true;
}

Expand Down
27 changes: 10 additions & 17 deletions src/tools/illink/src/linker/Linker.Dataflow/ReflectionMarker.cs
Original file line number Diff line number Diff line change
Expand Up @@ -58,39 +58,32 @@ internal void MarkTypeForDynamicallyAccessedMembers (in MessageOrigin origin, Ty

// Resolve a (potentially assembly qualified) type name based on the current context (taken from DiagnosticContext) and mark the type for reflection.
// This method will probe the current context assembly and if that fails CoreLib for the specified type. Emulates behavior of Type.GetType.
internal bool TryResolveTypeNameAndMark (string typeName, in DiagnosticContext diagnosticContext, bool needsAssemblyName, [NotNullWhen (true)] out TypeDefinition? type)
internal bool TryResolveTypeNameAndMark (string typeName, in DiagnosticContext diagnosticContext, bool needsAssemblyName, [NotNullWhen (true)] out TypeReference? type)
{
if (!_context.TypeNameResolver.TryResolveTypeName (typeName, diagnosticContext, out TypeReference? typeRef, out var typeResolutionRecords, needsAssemblyName)
|| typeRef.ResolveToTypeDefinition (_context) is not TypeDefinition foundType) {
if (!_context.TypeNameResolver.TryResolveTypeName (typeName, diagnosticContext, out type, out var typeResolutionRecords, needsAssemblyName)) {
type = default;
return false;
}

MarkResolvedType (diagnosticContext, typeRef, foundType, typeResolutionRecords);

type = foundType;
MarkType (diagnosticContext, type, typeResolutionRecords);
return true;
}

// Resolve a type from the specified assembly and mark it for reflection.
internal bool TryResolveTypeNameAndMark (AssemblyDefinition assembly, string typeName, in DiagnosticContext diagnosticContext, [NotNullWhen (true)] out TypeDefinition? type)
internal bool TryResolveTypeNameAndMark (AssemblyDefinition assembly, string typeName, in DiagnosticContext diagnosticContext, [NotNullWhen (true)] out TypeReference? type)
{
if (!_context.TypeNameResolver.TryResolveTypeName (assembly, typeName, out TypeReference? typeRef, out var typeResolutionRecords)
|| typeRef.ResolveToTypeDefinition (_context) is not TypeDefinition foundType) {
if (!_context.TypeNameResolver.TryResolveTypeName (assembly, typeName, out type, out var typeResolutionRecords)) {
type = default;
return false;
}

MarkResolvedType (diagnosticContext, typeRef, foundType, typeResolutionRecords);

type = foundType;
MarkType (diagnosticContext, type, typeResolutionRecords);
return true;
}

void MarkResolvedType (
void MarkType (
in DiagnosticContext diagnosticContext,
TypeReference typeReference,
TypeDefinition typeDefinition,
List<TypeNameResolver.TypeResolutionRecord> typeResolutionRecords)
{
if (_enabled) {
Expand All @@ -100,9 +93,9 @@ void MarkResolvedType (
// This is necessary because if the app's code contains the input string as literal (which is pretty much always the case)
// that string has to work at runtime, and if it relies on type forwarders we need to preserve those as well.
var origin = diagnosticContext.Origin;
_markStep.MarkTypeVisibleToReflection (typeReference, typeDefinition, new DependencyInfo (DependencyKind.AccessedViaReflection, origin.Provider), origin);
_markStep.MarkTypeVisibleToReflection (typeReference, new DependencyInfo (DependencyKind.AccessedViaReflection, origin.Provider), origin);
foreach (var typeResolutionRecord in typeResolutionRecords) {
_context.MarkingHelpers.MarkMatchingExportedType (typeResolutionRecord.ResolvedType, typeResolutionRecord.ReferringAssembly, new DependencyInfo (DependencyKind.DynamicallyAccessedMember, typeDefinition), origin);
_context.MarkingHelpers.MarkMatchingExportedType (typeResolutionRecord.ResolvedType, typeResolutionRecord.ReferringAssembly, new DependencyInfo (DependencyKind.DynamicallyAccessedMember, typeReference), origin);
}
}
}
Expand All @@ -115,7 +108,7 @@ internal void MarkType (in MessageOrigin origin, TypeReference typeRef, Dependen
if (typeRef.ResolveToTypeDefinition (_context) is not TypeDefinition type)
return;

_markStep.MarkTypeVisibleToReflection (type, type, new DependencyInfo (dependencyKind, origin.Provider), origin);
_markStep.MarkTypeVisibleToReflection (type, new DependencyInfo (dependencyKind, origin.Provider), origin);
}

internal void MarkMethod (in MessageOrigin origin, MethodReference methodRef, DependencyKind dependencyKind = DependencyKind.AccessedViaReflection)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ public RequireDynamicallyAccessedMembersAction (

public partial bool TryResolveTypeNameAndMark (string typeName, bool needsAssemblyName, out TypeProxy type)
{
if (_reflectionMarker.TryResolveTypeNameAndMark (typeName, _diagnosticContext, needsAssemblyName, out TypeDefinition? foundType)) {
if (_reflectionMarker.TryResolveTypeNameAndMark (typeName, _diagnosticContext, needsAssemblyName, out TypeReference? foundType)) {
type = new (foundType, _resolver);
return true;
} else {
Expand Down
29 changes: 13 additions & 16 deletions src/tools/illink/src/linker/Linker.Steps/MarkStep.cs
Original file line number Diff line number Diff line change
Expand Up @@ -325,7 +325,7 @@ internal void MarkEntireType (TypeDefinition type, in DependencyInfo reason, Mes
MarkEntireType (nested, new DependencyInfo (DependencyKind.NestedType, type), origin);
}

MarkTypeVisibleToReflection (type, type, reason, origin);
MarkTypeVisibleToReflection (type, reason, origin);
MarkCustomAttributes (type, new DependencyInfo (DependencyKind.CustomAttribute, type), origin);
MarkTypeSpecialCustomAttributes (type, origin);

Expand Down Expand Up @@ -914,7 +914,7 @@ void MarkMembersVisibleToReflection (IEnumerable<IMetadataTokenProvider> members
foreach (var member in members) {
switch (member) {
case TypeDefinition type:
MarkTypeVisibleToReflection (type, type, reason, origin);
MarkTypeVisibleToReflection (type, reason, origin);
break;
case MethodDefinition method:
MarkMethodVisibleToReflection (method, reason, origin);
Expand Down Expand Up @@ -1795,18 +1795,17 @@ protected virtual void MarkSerializable (TypeDefinition type, MessageOrigin orig
MarkMethodsIf (type.Methods, HasOnSerializeOrDeserializeAttribute, new DependencyInfo (DependencyKind.SerializationMethodForType, type), origin);
}

protected internal virtual TypeDefinition? MarkTypeVisibleToReflection (TypeReference type, TypeDefinition definition, in DependencyInfo reason, in MessageOrigin origin)
protected internal virtual void MarkTypeVisibleToReflection (TypeReference type, in DependencyInfo reason, in MessageOrigin origin)
{
// If a type is visible to reflection, we need to stop doing optimization that could cause observable difference
// in reflection APIs. This includes APIs like MakeGenericType (where variant castability of the produced type
// could be incorrect) or IsAssignableFrom (where assignability of unconstructed types might change).
Annotations.MarkRelevantToVariantCasting (definition);

Annotations.MarkReflectionUsed (definition);

MarkImplicitlyUsedFields (definition, origin);

return MarkType (type, reason, origin);
TypeDefinition? definition = MarkType (type, reason, origin);
if (definition is not null) {
// If a type is visible to reflection, we need to stop doing optimization that could cause observable difference
// in reflection APIs. This includes APIs like MakeGenericType (where variant castability of the produced type
// could be incorrect) or IsAssignableFrom (where assignability of unconstructed types might change).
Annotations.MarkRelevantToVariantCasting (definition);
Annotations.MarkReflectionUsed (definition);
MarkImplicitlyUsedFields (definition, origin);
}
}

internal void MarkMethodVisibleToReflection (MethodReference method, in DependencyInfo reason, in MessageOrigin origin)
Expand Down Expand Up @@ -3640,9 +3639,7 @@ protected virtual void MarkInstruction (Instruction instruction, MethodDefinitio
origin = new MessageOrigin (origin, instruction.Offset);

if (token is TypeReference typeReference) {
// Error will be reported as part of MarkType
if (Context.TryResolve (typeReference) is TypeDefinition type)
MarkTypeVisibleToReflection (typeReference, type, reason, origin);
MarkTypeVisibleToReflection (typeReference, reason, origin);
} else if (token is MethodReference methodReference) {
MarkMethodVisibleToReflection (methodReference, reason, origin);
} else {
Expand Down
13 changes: 9 additions & 4 deletions src/tools/illink/src/linker/Linker/TypeReferenceExtensions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -432,13 +432,18 @@ public static bool IsNamedType (this TypeReference typeReference) {
return true;
}

// 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.
/// <summary>
/// Resolves a TypeReference to a TypeDefinition if possible. Non-named types other than arrays (pointers, byrefs, function pointers) return null.
/// Array types that are dynamically accessed 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.
/// </summary>
public static TypeDefinition? ResolveToTypeDefinition (this TypeReference typeReference, LinkContext context)
=> typeReference is ArrayType
? BCL.FindPredefinedType (WellKnownType.System_Array, context)
: context.TryResolve (typeReference);
: typeReference.IsNamedType ()
? context.TryResolve (typeReference)
: null;

public static bool IsByRefOrPointer (this TypeReference typeReference)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,7 @@ public static void Main ()
TestInvalidTypeCombination ();
TestEscapedTypeName ();
AssemblyTypeResolutionBehavior.Test ();
InstantiatedGenericEquality.Test ();
}

[Kept]
Expand Down Expand Up @@ -213,15 +214,11 @@ public static void TestType ()
}

[Kept]
#if !NATIVEAOT // https://github.com/dotnet/runtime/issues/106214
[KeptMember (".ctor()")]
#endif
[KeptAttributeAttribute (typeof (RequiresUnreferencedCodeAttribute))]
[RequiresUnreferencedCode (nameof (Pointer))]
public class Pointer { }

[Kept]
[UnexpectedWarning ("IL2026", nameof (Pointer), Tool.Trimmer, "https://github.com/dotnet/runtime/issues/106214")]
public static void TestPointer ()
{
const string reflectionTypeKeptString = "Mono.Linker.Tests.Cases.Reflection.TypeUsedViaReflection+Pointer*";
Expand All @@ -230,15 +227,11 @@ public static void TestPointer ()
}

[Kept]
#if !NATIVEAOT // https://github.com/dotnet/runtime/issues/106214
[KeptMember (".ctor()")]
#endif
[KeptAttributeAttribute (typeof (RequiresUnreferencedCodeAttribute))]
[RequiresUnreferencedCode (nameof (Reference))]
public class Reference { }

[Kept]
[UnexpectedWarning ("IL2026", nameof (Reference), Tool.Trimmer, "https://github.com/dotnet/runtime/issues/106214")]
public static void TestReference ()
{
const string reflectionTypeKeptString = "Mono.Linker.Tests.Cases.Reflection.TypeUsedViaReflection+Reference&";
Expand Down Expand Up @@ -686,6 +679,32 @@ class PointerElementGenericArgumentType {}
class ByRefElementGenericArgumentType {}
}

[Kept]
class InstantiatedGenericEquality
{
[Kept]
class Generic<T> {
[Kept]
public void Method () { }
}

// Regression test for an issue where ILLink's representation of a generic instantiated type
// was using reference equality. The test uses a lambda to ensure that it goes through the
// interprocedural analysis code path that merges patterns and relies on a correct implementation
// of equality.
[Kept]
public static void Test ()
{
var type = Type.GetType("Mono.Linker.Tests.Cases.Reflection.TypeUsedViaReflection+InstantiatedGenericEquality+Generic`1[[System.Int32]]");

var lambda = () => {
type.GetMethod ("Method");
};

lambda ();
}
}

[Kept]
static void RequireConstructor (
[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
Expand Down

0 comments on commit 2694613

Please sign in to comment.