Skip to content

Commit ad25cd0

Browse files
sbomersbomer
authored
ILLink: Fix order dependent logic for DAM on types (#104701)
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.
1 parent 9600a8c commit ad25cd0

File tree

3 files changed

+107
-32
lines changed

3 files changed

+107
-32
lines changed

‎src/tools/illink/src/linker/Linker.Dataflow/DynamicallyAccessedMembersTypeHierarchy.cs

Lines changed: 25 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ sealed class DynamicallyAccessedMembersTypeHierarchy
1414
{
1515
readonly LinkContext _context;
1616
readonly MarkStep _markStep;
17+
readonly ReflectionMarker _reflectionMarker;
1718

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

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

116116
return (annotation, apply);
@@ -126,10 +126,8 @@ public DynamicallyAccessedMemberTypes ApplyDynamicallyAccessedMembersToTypeHiera
126126
return annotation;
127127

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

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

163161
foreach (var candidateType in candidateTypes) {
164-
ApplyDynamicallyAccessedMembersToTypeHierarchyInner (reflectionMarker, candidateType);
162+
ApplyDynamicallyAccessedMembersToTypeHierarchyInner (candidateType);
165163
}
166164
}
167165

168166
return annotation;
169167
}
170168

171-
bool ApplyDynamicallyAccessedMembersToTypeHierarchyInner (
172-
in ReflectionMarker reflectionMarker,
173-
TypeDefinition type)
169+
bool ApplyDynamicallyAccessedMembersToTypeHierarchyInner (TypeDefinition type)
174170
{
175171
(var annotation, var applied) = GetCachedInfoForTypeInHierarchy (type);
176172

@@ -182,13 +178,13 @@ bool ApplyDynamicallyAccessedMembersToTypeHierarchyInner (
182178

183179
TypeDefinition? baseType = _context.TryResolve (type.BaseType);
184180
if (baseType != null)
185-
applied = ApplyDynamicallyAccessedMembersToTypeHierarchyInner (reflectionMarker, baseType);
181+
applied = ApplyDynamicallyAccessedMembersToTypeHierarchyInner (baseType);
186182

187183
if (!applied && type.HasInterfaces) {
188184
foreach (InterfaceImplementation iface in type.Interfaces) {
189185
var interfaceType = _context.TryResolve (iface.InterfaceType);
190186
if (interfaceType != null) {
191-
if (ApplyDynamicallyAccessedMembersToTypeHierarchyInner (reflectionMarker, interfaceType)) {
187+
if (ApplyDynamicallyAccessedMembersToTypeHierarchyInner (interfaceType)) {
192188
applied = true;
193189
break;
194190
}
@@ -197,31 +193,33 @@ bool ApplyDynamicallyAccessedMembersToTypeHierarchyInner (
197193
}
198194

199195
if (applied) {
200-
var origin = new MessageOrigin (type);
201196
// Report warnings on access to annotated members, with the annotated type as the origin.
202-
ApplyDynamicallyAccessedMembersToType (reflectionMarker, origin, type, annotation);
197+
ApplyDynamicallyAccessedMembersToType (type, annotation);
203198
_typesInDynamicallyAccessedMembersHierarchy[type] = (annotation, true);
204199
}
205200

206201
return applied;
207202
}
208203

209-
void ApplyDynamicallyAccessedMembersToType (in ReflectionMarker reflectionMarker, in MessageOrigin origin, TypeDefinition type, DynamicallyAccessedMemberTypes annotation)
204+
void ApplyDynamicallyAccessedMembersToType (TypeDefinition type, DynamicallyAccessedMemberTypes annotation)
210205
{
206+
var origin = new MessageOrigin (type);
211207
Debug.Assert (annotation != DynamicallyAccessedMemberTypes.None);
212208

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

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

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

236234
var interfaceAnnotation = GetCachedInfoForTypeInHierarchy (interfaceType);
237-
if (interfaceAnnotation.applied && interfaceAnnotation.annotation.HasFlag (annotationToApplyToInterfaces))
238-
continue;
239-
240-
// Apply All or Interfaces to the interface type.
241-
// DAMT.All may produce redundant warnings from implementing types, when the interface type already had some annotations.
242-
reflectionMarker.MarkTypeForDynamicallyAccessedMembers (origin, interfaceType, annotationToApplyToInterfaces, DependencyKind.DynamicallyAccessedMemberOnType, declaredOnly: false);
235+
if (interfaceAnnotation.annotation.HasFlag (annotationToApplyToInterfaces)) {
236+
if (!interfaceAnnotation.applied)
237+
ApplyDynamicallyAccessedMembersToType (interfaceType, interfaceAnnotation.annotation);
238+
} else {
239+
// Apply All or Interfaces to the interface type.
240+
// DAMT.All may produce redundant warnings from implementing types, when the interface type already had some annotations.
241+
_reflectionMarker.MarkTypeForDynamicallyAccessedMembers (origin, interfaceType, annotationToApplyToInterfaces, DependencyKind.DynamicallyAccessedMemberOnType, declaredOnly: false);
242+
}
243243
}
244244
}
245245

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

252252
(DynamicallyAccessedMemberTypes annotation, bool applied) GetCachedInfoForTypeInHierarchy (TypeDefinition type)

‎src/tools/illink/test/ILLink.RoslynAnalyzer.Tests/ReflectionTests.cs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -181,14 +181,14 @@ public Task TypeDelegator ()
181181
[Fact]
182182
public Task TypeHierarchyReflectionWarnings ()
183183
{
184-
// https://github.com/dotnet/linker/issues/2578
184+
// https://github.com/dotnet/runtime/issues/104742
185185
return RunTest (allowMissingWarnings: true);
186186
}
187187

188188
[Fact]
189189
public Task TypeHierarchySuppressions ()
190190
{
191-
// https://github.com/dotnet/linker/issues/2578
191+
// https://github.com/dotnet/runtime/issues/104742
192192
return RunTest (allowMissingWarnings: true);
193193
}
194194

‎src/tools/illink/test/Mono.Linker.Tests.Cases/Reflection/TypeHierarchyReflectionWarnings.cs

Lines changed: 80 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,8 @@ public static void Main ()
5858

5959
RUCOnVirtualOnAnnotatedBase.Test ();
6060
RUCOnVirtualOnAnnotatedBaseUsedByDerived.Test ();
61+
RUCOnVirtualOnAnnotatedInterface.Test ();
62+
RucOnVirtualOnAnnotatedInterfaceUsedByImplementation.Test ();
6163
UseByDerived.Test ();
6264

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

708708
[Kept]
709709
[KeptMember (".ctor()")]
710710
[KeptBaseType (typeof (Base))]
711-
[UnexpectedWarning ("IL2113", "--RUCOnVirtualMethodDerivedAnnotated.Base.RUCVirtualMethod--", Tool.Trimmer, "https://github.com/dotnet/runtime/issues/86580")]
712711
public class Derived : Base
713712
{
714713
[Kept]
@@ -729,6 +728,82 @@ public static void Test ()
729728
}
730729
}
731730

731+
[Kept]
732+
class RUCOnVirtualOnAnnotatedInterface
733+
{
734+
[Kept]
735+
[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
736+
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)]
737+
public interface Interface
738+
{
739+
[Kept]
740+
[KeptAttributeAttribute (typeof (RequiresUnreferencedCodeAttribute))]
741+
[RequiresUnreferencedCode ("--RUCOnVirtualOnAnnotatedInterface.Interface.RUCVirtualMethod--")]
742+
[ExpectedWarning ("IL2112", "--RUCOnVirtualOnAnnotatedInterface.Interface.RUCVirtualMethod--", Tool.Trimmer, "https://github.com/dotnet/runtime/issues/104740")]
743+
void RUCVirtualMethod () { }
744+
}
745+
746+
[Kept]
747+
[KeptMember (".ctor()")]
748+
[KeptInterface (typeof (Interface))]
749+
public class Implementation : Interface
750+
{
751+
[Kept]
752+
[KeptAttributeAttribute (typeof (RequiresUnreferencedCodeAttribute))]
753+
[RequiresUnreferencedCode ("--RUCOnVirtualOnAnnotatedInterface.Implementation.RUCVirtualMethod--")]
754+
[ExpectedWarning ("IL2112", "--RUCOnVirtualOnAnnotatedInterface.Implementation.RUCVirtualMethod--")]
755+
public void RUCVirtualMethod () { }
756+
}
757+
758+
[Kept]
759+
static Interface _interfaceInstance;
760+
761+
[Kept]
762+
public static void Test ()
763+
{
764+
_interfaceInstance = new Implementation ();
765+
_interfaceInstance.GetType ().RequiresAll ();
766+
}
767+
}
768+
769+
[Kept]
770+
class RucOnVirtualOnAnnotatedInterfaceUsedByImplementation
771+
{
772+
[Kept]
773+
[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
774+
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.All)]
775+
public interface Interface
776+
{
777+
[Kept]
778+
[KeptAttributeAttribute (typeof (RequiresUnreferencedCodeAttribute))]
779+
[RequiresUnreferencedCode ("--RucOnVirtualOnAnnotatedInterfaceUsedByImplementation.Interface.RUCVirtualMethod--")]
780+
[ExpectedWarning ("IL2112", "--RucOnVirtualOnAnnotatedInterfaceUsedByImplementation.Interface.RUCVirtualMethod--", Tool.Trimmer, "https://github.com/dotnet/runtime/issues/104740")]
781+
void RUCVirtualMethod () { }
782+
}
783+
784+
[Kept]
785+
[KeptMember (".ctor()")]
786+
[KeptInterface (typeof (Interface))]
787+
public class Implementation : Interface
788+
{
789+
[Kept]
790+
[KeptAttributeAttribute (typeof (RequiresUnreferencedCodeAttribute))]
791+
[RequiresUnreferencedCode ("--RucOnVirtualOnAnnotatedInterfaceUsedByImplementation.Implementation.RUCVirtualMethod--")]
792+
[ExpectedWarning ("IL2112", "--RucOnVirtualOnAnnotatedInterfaceUsedByImplementation.Implementation.RUCVirtualMethod--")]
793+
public void RUCVirtualMethod () { }
794+
}
795+
796+
[Kept]
797+
static Implementation _implementationInstance;
798+
799+
[Kept]
800+
public static void Test ()
801+
{
802+
_implementationInstance = new Implementation ();
803+
_implementationInstance.GetType ().RequiresAll ();
804+
}
805+
}
806+
732807
[Kept]
733808
class UseByDerived
734809
{
@@ -745,13 +820,13 @@ class AnnotatedBase
745820
[RequiresUnreferencedCode ("--AnnotatedBase.VirtualMethodWithRequires--")]
746821
[RequiresDynamicCode ("--AnnotatedBase.VirtualMethodWithRequires--")]
747822
[RequiresAssemblyFiles ("--AnnotatedBase.VirtualMethodWithRequires--")]
823+
[ExpectedWarning ("IL2112", "--AnnotatedBase.VirtualMethodWithRequires--", Tool.Trimmer, "https://github.com/dotnet/runtime/issues/104740")]
748824
public virtual void VirtualMethodWithRequires () { }
749825
}
750826

751827
[Kept]
752828
[KeptBaseType (typeof (AnnotatedBase))]
753829
[KeptMember (".ctor()")]
754-
[UnexpectedWarning ("IL2113", "--AnnotatedBase.VirtualMethodWithRequires--", Tool.Trimmer, "https://github.com/dotnet/runtime/issues/86580")]
755830
class Derived : AnnotatedBase
756831
{
757832
[Kept]

0 commit comments

Comments
 (0)