Skip to content

Commit

Permalink
ILLink: Fix order dependent logic for DAM on types (#104701)
Browse files Browse the repository at this point in the history 
When applying DynamicallyAccessedMembers annotations to a type, we
need to ensure that the base annotations are also applied to the base
type.

Previously this logic was determining which annotations to apply to
the base type (with the warning originating from the derived type)
based on whether the base annotations were already applied. This is
incorrect as it introduces an order dependence. Instead the base
annotations should use the base type as the origin.
  • Loading branch information
@sbomer @pull
sbomer authored and pull[bot] committed Aug 7, 2024
1 parent e85cf28 commit 4730875
Show file tree
Hide file tree
Showing 3 changed files with 107 additions and 32 deletions.
50 changes: 25 additions & 25 deletions src/tools/illink/src/linker/Linker.Dataflow/DynamicallyAccessedMembersTypeHierarchy.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ sealed class DynamicallyAccessedMembersTypeHierarchy
{
readonly LinkContext _context;
readonly MarkStep _markStep;
readonly ReflectionMarker _reflectionMarker;

// Cache of DynamicallyAccessedMembers annotations applied to types and their hierarchies
// Values
Expand Down Expand Up @@ -43,6 +44,7 @@ public DynamicallyAccessedMembersTypeHierarchy (LinkContext context, MarkStep ma
_context = context;
_markStep = markStep;
_typesInDynamicallyAccessedMembersHierarchy = new Dictionary<TypeDefinition, (DynamicallyAccessedMemberTypes, bool)> ();
_reflectionMarker = new ReflectionMarker (_context, _markStep, enabled: true);
}

public (DynamicallyAccessedMemberTypes annotation, bool applied) ProcessMarkedTypeForDynamicallyAccessedMembersHierarchy (TypeDefinition type)
Expand Down Expand Up @@ -107,10 +109,8 @@ public DynamicallyAccessedMembersTypeHierarchy (LinkContext context, MarkStep ma
if (apply) {
// One of the base/interface types is already marked as having the annotation applied
// so we need to apply the annotation to this type as well
var origin = new MessageOrigin (type);
var reflectionMarker = new ReflectionMarker (_context, _markStep, enabled: true);
// Report warnings on access to annotated members, with the annotated type as the origin.
ApplyDynamicallyAccessedMembersToType (reflectionMarker, origin, type, annotation);
ApplyDynamicallyAccessedMembersToType (type, annotation);
}

return (annotation, apply);
Expand All @@ -126,10 +126,8 @@ public DynamicallyAccessedMemberTypes ApplyDynamicallyAccessedMembersToTypeHiera
return annotation;

// Apply the effective annotation for the type
var origin = new MessageOrigin (type);
var reflectionMarker = new ReflectionMarker (_context, _markStep, enabled: true);
// Report warnings on access to annotated members, with the annotated type as the origin.
ApplyDynamicallyAccessedMembersToType (reflectionMarker, origin, type, annotation);
ApplyDynamicallyAccessedMembersToType (type, annotation);

// Mark it as applied in the cache
_typesInDynamicallyAccessedMembersHierarchy[type] = (annotation, true);
Expand Down Expand Up @@ -161,16 +159,14 @@ public DynamicallyAccessedMemberTypes ApplyDynamicallyAccessedMembersToTypeHiera
break;

foreach (var candidateType in candidateTypes) {
ApplyDynamicallyAccessedMembersToTypeHierarchyInner (reflectionMarker, candidateType);
ApplyDynamicallyAccessedMembersToTypeHierarchyInner (candidateType);
}
}

return annotation;
}

bool ApplyDynamicallyAccessedMembersToTypeHierarchyInner (
in ReflectionMarker reflectionMarker,
TypeDefinition type)
bool ApplyDynamicallyAccessedMembersToTypeHierarchyInner (TypeDefinition type)
{
(var annotation, var applied) = GetCachedInfoForTypeInHierarchy (type);

Expand All @@ -182,13 +178,13 @@ bool ApplyDynamicallyAccessedMembersToTypeHierarchyInner (

TypeDefinition? baseType = _context.TryResolve (type.BaseType);
if (baseType != null)
applied = ApplyDynamicallyAccessedMembersToTypeHierarchyInner (reflectionMarker, baseType);
applied = ApplyDynamicallyAccessedMembersToTypeHierarchyInner (baseType);

if (!applied && type.HasInterfaces) {
foreach (InterfaceImplementation iface in type.Interfaces) {
var interfaceType = _context.TryResolve (iface.InterfaceType);
if (interfaceType != null) {
if (ApplyDynamicallyAccessedMembersToTypeHierarchyInner (reflectionMarker, interfaceType)) {
if (ApplyDynamicallyAccessedMembersToTypeHierarchyInner (interfaceType)) {
applied = true;
break;
}
Expand All @@ -197,31 +193,33 @@ bool ApplyDynamicallyAccessedMembersToTypeHierarchyInner (
}

if (applied) {
var origin = new MessageOrigin (type);
// Report warnings on access to annotated members, with the annotated type as the origin.
ApplyDynamicallyAccessedMembersToType (reflectionMarker, origin, type, annotation);
ApplyDynamicallyAccessedMembersToType (type, annotation);
_typesInDynamicallyAccessedMembersHierarchy[type] = (annotation, true);
}

return applied;
}

void ApplyDynamicallyAccessedMembersToType (in ReflectionMarker reflectionMarker, in MessageOrigin origin, TypeDefinition type, DynamicallyAccessedMemberTypes annotation)
void ApplyDynamicallyAccessedMembersToType (TypeDefinition type, DynamicallyAccessedMemberTypes annotation)
{
var origin = new MessageOrigin (type);
Debug.Assert (annotation != DynamicallyAccessedMemberTypes.None);

// We need to apply annotations to this type, and its base/interface types (recursively)
// But the annotations on base/interfaces are already applied so we don't need to apply those
// But the annotations on base will be applied so we don't need to apply those
// again (and should avoid doing so as it would produce extra warnings).
var baseType = _context.TryResolve (type.BaseType);
if (baseType != null) {
var baseAnnotation = GetCachedInfoForTypeInHierarchy (baseType);
var annotationToApplyToBase = baseAnnotation.applied ? Annotations.GetMissingMemberTypes (annotation, baseAnnotation.annotation) : annotation;
if (!baseAnnotation.applied && baseAnnotation.annotation != DynamicallyAccessedMemberTypes.None)
ApplyDynamicallyAccessedMembersToType (baseType, baseAnnotation.annotation);
var annotationToApplyToBase = Annotations.GetMissingMemberTypes (annotation, baseAnnotation.annotation);

// Apply any annotations that didn't exist on the base type to the base type.
// This may produce redundant warnings when the annotation is DAMT.All or DAMT.PublicConstructors and the base already has a
// subset of those annotations.
reflectionMarker.MarkTypeForDynamicallyAccessedMembers (origin, baseType, annotationToApplyToBase, DependencyKind.DynamicallyAccessedMemberOnType, declaredOnly: false);
_reflectionMarker.MarkTypeForDynamicallyAccessedMembers (origin, baseType, annotationToApplyToBase, DependencyKind.DynamicallyAccessedMemberOnType, declaredOnly: false);
}

// Most of the DynamicallyAccessedMemberTypes don't select members on interfaces. We only need to apply
Expand All @@ -234,19 +232,21 @@ void ApplyDynamicallyAccessedMembersToType (in ReflectionMarker reflectionMarker
continue;

var interfaceAnnotation = GetCachedInfoForTypeInHierarchy (interfaceType);
if (interfaceAnnotation.applied && interfaceAnnotation.annotation.HasFlag (annotationToApplyToInterfaces))
continue;

// Apply All or Interfaces to the interface type.
// DAMT.All may produce redundant warnings from implementing types, when the interface type already had some annotations.
reflectionMarker.MarkTypeForDynamicallyAccessedMembers (origin, interfaceType, annotationToApplyToInterfaces, DependencyKind.DynamicallyAccessedMemberOnType, declaredOnly: false);
if (interfaceAnnotation.annotation.HasFlag (annotationToApplyToInterfaces)) {
if (!interfaceAnnotation.applied)
ApplyDynamicallyAccessedMembersToType (interfaceType, interfaceAnnotation.annotation);
} else {
// Apply All or Interfaces to the interface type.
// DAMT.All may produce redundant warnings from implementing types, when the interface type already had some annotations.
_reflectionMarker.MarkTypeForDynamicallyAccessedMembers (origin, interfaceType, annotationToApplyToInterfaces, DependencyKind.DynamicallyAccessedMemberOnType, declaredOnly: false);
}
}
}

// The annotations this type inherited from its base types or interfaces should not produce
// warnings on the respective base/interface members, since those are already covered by applying
// the annotations to those types. So we only need to handle the members directly declared on this type.
reflectionMarker.MarkTypeForDynamicallyAccessedMembers (origin, type, annotation, DependencyKind.DynamicallyAccessedMemberOnType, declaredOnly: true);
_reflectionMarker.MarkTypeForDynamicallyAccessedMembers (origin, type, annotation, DependencyKind.DynamicallyAccessedMemberOnType, declaredOnly: true);
}

(DynamicallyAccessedMemberTypes annotation, bool applied) GetCachedInfoForTypeInHierarchy (TypeDefinition type)
Expand Down
4 changes: 2 additions & 2 deletions src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/ReflectionTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -181,14 +181,14 @@ public Task TypeDelegator ()
[Fact]
public Task TypeHierarchyReflectionWarnings ()
{
// https://github.com/dotnet/linker/issues/2578
// https://github.com/dotnet/runtime/issues/104742
return RunTest (allowMissingWarnings: true);
}

[Fact]
public Task TypeHierarchySuppressions ()
{
// https://github.com/dotnet/linker/issues/2578
// https://github.com/dotnet/runtime/issues/104742
return RunTest (allowMissingWarnings: true);
}

Expand Down
85 changes: 80 additions & 5 deletions src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/TypeHierarchyReflectionWarnings.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ public static void Main ()

RUCOnVirtualOnAnnotatedBase.Test ();
RUCOnVirtualOnAnnotatedBaseUsedByDerived.Test ();
RUCOnVirtualOnAnnotatedInterface.Test ();
RucOnVirtualOnAnnotatedInterfaceUsedByImplementation.Test ();
UseByDerived.Test ();

CompilerGeneratedCodeRUC.Test (null);
Expand Down Expand Up @@ -699,16 +701,13 @@ public class Base
[Kept]
[KeptAttributeAttribute (typeof (RequiresUnreferencedCodeAttribute))]
[RequiresUnreferencedCode ("--RUCOnVirtualMethodDerivedAnnotated.Base.RUCVirtualMethod--")]
// Compare to the case above - the only difference is the type of the field
// and it causes different warnings to be produced.
[ExpectedWarning ("IL2112", "--RUCOnVirtualMethodDerivedAnnotated.Base.RUCVirtualMethod--", Tool.None, "https://github.com/dotnet/runtime/issues/86580")]
[ExpectedWarning ("IL2112", "--RUCOnVirtualMethodDerivedAnnotated.Base.RUCVirtualMethod--", Tool.Trimmer, "https://github.com/dotnet/runtime/issues/104740")]
public virtual void RUCVirtualMethod () { }
}

[Kept]
[KeptMember (".ctor()")]
[KeptBaseType (typeof (Base))]
[UnexpectedWarning ("IL2113", "--RUCOnVirtualMethodDerivedAnnotated.Base.RUCVirtualMethod--", Tool.Trimmer, "https://github.com/dotnet/runtime/issues/86580")]
public class Derived : Base
{
[Kept]
Expand All @@ -729,6 +728,82 @@ public static void Test ()
}
}

[Kept]
class RUCOnVirtualOnAnnotatedInterface
{
[Kept]
[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)]
public interface Interface
{
[Kept]
[KeptAttributeAttribute (typeof (RequiresUnreferencedCodeAttribute))]
[RequiresUnreferencedCode ("--RUCOnVirtualOnAnnotatedInterface.Interface.RUCVirtualMethod--")]
[ExpectedWarning ("IL2112", "--RUCOnVirtualOnAnnotatedInterface.Interface.RUCVirtualMethod--", Tool.Trimmer, "https://github.com/dotnet/runtime/issues/104740")]
void RUCVirtualMethod () { }
}

[Kept]
[KeptMember (".ctor()")]
[KeptInterface (typeof (Interface))]
public class Implementation : Interface
{
[Kept]
[KeptAttributeAttribute (typeof (RequiresUnreferencedCodeAttribute))]
[RequiresUnreferencedCode ("--RUCOnVirtualOnAnnotatedInterface.Implementation.RUCVirtualMethod--")]
[ExpectedWarning ("IL2112", "--RUCOnVirtualOnAnnotatedInterface.Implementation.RUCVirtualMethod--")]
public void RUCVirtualMethod () { }
}

[Kept]
static Interface _interfaceInstance;

[Kept]
public static void Test ()
{
_interfaceInstance = new Implementation ();
_interfaceInstance.GetType ().RequiresAll ();
}
}

[Kept]
class RucOnVirtualOnAnnotatedInterfaceUsedByImplementation
{
[Kept]
[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)]
public interface Interface
{
[Kept]
[KeptAttributeAttribute (typeof (RequiresUnreferencedCodeAttribute))]
[RequiresUnreferencedCode ("--RucOnVirtualOnAnnotatedInterfaceUsedByImplementation.Interface.RUCVirtualMethod--")]
[ExpectedWarning ("IL2112", "--RucOnVirtualOnAnnotatedInterfaceUsedByImplementation.Interface.RUCVirtualMethod--", Tool.Trimmer, "https://github.com/dotnet/runtime/issues/104740")]
void RUCVirtualMethod () { }
}

[Kept]
[KeptMember (".ctor()")]
[KeptInterface (typeof (Interface))]
public class Implementation : Interface
{
[Kept]
[KeptAttributeAttribute (typeof (RequiresUnreferencedCodeAttribute))]
[RequiresUnreferencedCode ("--RucOnVirtualOnAnnotatedInterfaceUsedByImplementation.Implementation.RUCVirtualMethod--")]
[ExpectedWarning ("IL2112", "--RucOnVirtualOnAnnotatedInterfaceUsedByImplementation.Implementation.RUCVirtualMethod--")]
public void RUCVirtualMethod () { }
}

[Kept]
static Implementation _implementationInstance;

[Kept]
public static void Test ()
{
_implementationInstance = new Implementation ();
_implementationInstance.GetType ().RequiresAll ();
}
}

[Kept]
class UseByDerived
{
Expand All @@ -745,13 +820,13 @@ class AnnotatedBase
[RequiresUnreferencedCode ("--AnnotatedBase.VirtualMethodWithRequires--")]
[RequiresDynamicCode ("--AnnotatedBase.VirtualMethodWithRequires--")]
[RequiresAssemblyFiles ("--AnnotatedBase.VirtualMethodWithRequires--")]
[ExpectedWarning ("IL2112", "--AnnotatedBase.VirtualMethodWithRequires--", Tool.Trimmer, "https://github.com/dotnet/runtime/issues/104740")]
public virtual void VirtualMethodWithRequires () { }
}

[Kept]
[KeptBaseType (typeof (AnnotatedBase))]
[KeptMember (".ctor()")]
[UnexpectedWarning ("IL2113", "--AnnotatedBase.VirtualMethodWithRequires--", Tool.Trimmer, "https://github.com/dotnet/runtime/issues/86580")]
class Derived : AnnotatedBase
{
[Kept]
Expand Down

0 comments on commit 4730875

Please sign in to comment.