Skip to content

Commit a6318f9

Browse files
sbomersbomer
committed
Unify with behavior for DAM warnings
- Don't warn on non-virtual methods that don't have return annotations - Use the helpers introduced in dotnet#2145
1 parent 69965ea commit a6318f9

File tree

3 files changed

+81
-62
lines changed

3 files changed

+81
-62
lines changed

‎src/linker/Linker.Dataflow/FlowAnnotations.cs

Lines changed: 51 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -65,13 +65,10 @@ public DynamicallyAccessedMemberTypes GetFieldAnnotation (FieldDefinition field)
6565
public DynamicallyAccessedMemberTypes GetTypeAnnotation (TypeDefinition type) =>
6666
GetAnnotations (type).TypeAnnotation;
6767

68-
public bool DoesMemberAccessRequireDynamicallyAccessedMembers (IMemberDefinition provider) =>
68+
public bool ShouldWarnWhenAccessedForReflection (IMemberDefinition provider) =>
6969
provider switch {
70-
MethodDefinition method =>
71-
GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out var annotation) &&
72-
(annotation.ParameterAnnotations != null || annotation.ReturnParameterAnnotation != DynamicallyAccessedMemberTypes.None),
73-
FieldDefinition field =>
74-
GetAnnotations (field.DeclaringType).TryGetAnnotation (field, out _),
70+
MethodDefinition method => ShouldWarnWhenAccessedForReflection (method),
71+
FieldDefinition field => ShouldWarnWhenAccessedForReflection (field),
7572
_ => false
7673
};
7774

@@ -93,13 +90,55 @@ public DynamicallyAccessedMemberTypes GetGenericParameterAnnotation (GenericPara
9390
return DynamicallyAccessedMemberTypes.None;
9491
}
9592

96-
public bool ShouldWarnWhenAccessedForReflection (MethodDefinition method) =>
97-
GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out var annotation) &&
98-
(annotation.ParameterAnnotations != null || annotation.ReturnParameterAnnotation != DynamicallyAccessedMemberTypes.None);
93+
public bool ShouldWarnWhenAccessedForReflection (MethodDefinition method)
94+
{
95+
if (!GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out var annotation))
96+
return false;
97+
98+
if (annotation.ParameterAnnotations == null && annotation.ReturnParameterAnnotation == DynamicallyAccessedMemberTypes.None)
99+
return false;
99100

100-
public bool MethodHasNoAnnotatedParameters (MethodDefinition method) =>
101-
GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out var annotation) &&
102-
annotation.ParameterAnnotations == null;
101+
// If the method only has annotation on the return value and it's not virtual avoid warning.
102+
// Return value annotations are "consumed" by the caller of a method, and as such there is nothing
103+
// wrong calling these dynamically. The only problem can happen if something overrides a virtual
104+
// method with annotated return value at runtime - in this case the trimmer can't validate
105+
// that the method will return only types which fulfill the annotation's requirements.
106+
// For example:
107+
// class BaseWithAnnotation
108+
// {
109+
// [return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields)]
110+
// public abstract Type GetTypeWithFields();
111+
// }
112+
//
113+
// class UsingTheBase
114+
// {
115+
// public void PrintFields(Base base)
116+
// {
117+
// // No warning here - GetTypeWithFields is correctly annotated to allow GetFields on the return value.
118+
// Console.WriteLine(string.Join(" ", base.GetTypeWithFields().GetFields().Select(f => f.Name)));
119+
// }
120+
// }
121+
//
122+
// If at runtime (through ref emit) something generates code like this:
123+
// class DerivedAtRuntimeFromBase
124+
// {
125+
// // No point in adding annotation on the return value - nothing will look at it anyway
126+
// // Linker will not see this code, so there are no checks
127+
// public override Type GetTypeWithFields() { return typeof(TestType); }
128+
// }
129+
//
130+
// If TestType from above is trimmed, it may note have all its fields, and there would be no warnings generated.
131+
// But there has to be code like this somewhere in the app, in order to generate the override:
132+
// class RuntimeTypeGenerator
133+
// {
134+
// public MethodInfo GetBaseMethod()
135+
// {
136+
// // This must warn - that the GetTypeWithFields has annotation on the return value
137+
// return typeof(BaseWithAnnotation).GetMethod("GetTypeWithFields");
138+
// }
139+
// }
140+
return method.IsVirtual || annotation.ParameterAnnotations != null;
141+
}
103142

104143
public bool ShouldWarnWhenAccessedForReflection (FieldDefinition field) =>
105144
GetAnnotations (field.DeclaringType).TryGetAnnotation (field, out _);

‎src/linker/Linker.Steps/MarkStep.cs

Lines changed: 1 addition & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -1582,7 +1582,7 @@ static bool IsDeclaredWithinType (IMemberDefinition member, TypeDefinition type)
15821582
_context.LogWarning (message, code, origin, MessageSubCategory.TrimAnalysis);
15831583
}
15841584

1585-
if (_context.Annotations.FlowAnnotations.DoesMemberAccessRequireDynamicallyAccessedMembers (member)) {
1585+
if (_context.Annotations.FlowAnnotations.ShouldWarnWhenAccessedForReflection (member)) {
15861586
var message = string.Format (
15871587
"'DynamicallyAccessedMembersAttribute' on '{0}' or one of its base types references '{1}' which has 'DynamicallyAccessedMembersAttribute' requirements.",
15881588
type.GetDisplayName (),
@@ -2885,48 +2885,6 @@ void ProcessAnalysisAnnotationsForMethod (MethodDefinition method, DependencyKin
28852885
break;
28862886
}
28872887

2888-
// If the method only has annotation on the return value and it's not virtual avoid warning.
2889-
// Return value annotations are "consumed" by the caller of a method, and as such there is nothing
2890-
// wrong calling these dynamically. The only problem can happen if something overrides a virtual
2891-
// method with annotated return value at runtime - in this case the trimmer can't validate
2892-
// that the method will return only types which fulfill the annotation's requirements.
2893-
// For example:
2894-
// class BaseWithAnnotation
2895-
// {
2896-
// [return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields)]
2897-
// public abstract Type GetTypeWithFields();
2898-
// }
2899-
//
2900-
// class UsingTheBase
2901-
// {
2902-
// public void PrintFields(Base base)
2903-
// {
2904-
// // No warning here - GetTypeWithFields is correctly annotated to allow GetFields on the return value.
2905-
// Console.WriteLine(string.Join(" ", base.GetTypeWithFields().GetFields().Select(f => f.Name)));
2906-
// }
2907-
// }
2908-
//
2909-
// If at runtime (through ref emit) something generates code like this:
2910-
// class DerivedAtRuntimeFromBase
2911-
// {
2912-
// // No point in adding annotation on the return value - nothing will look at it anyway
2913-
// // Linker will not see this code, so there are no checks
2914-
// public override Type GetTypeWithFields() { return typeof(TestType); }
2915-
// }
2916-
//
2917-
// If TestType from above is trimmed, it may note have all its fields, and there would be no warnings generated.
2918-
// But there has to be code like this somewhere in the app, in order to generate the override:
2919-
// class RuntimeTypeGenerator
2920-
// {
2921-
// public MethodInfo GetBaseMethod()
2922-
// {
2923-
// // This must warn - that the GetTypeWithFields has annotation on the return value
2924-
// return typeof(BaseWithAnnotation).GetMethod("GetTypeWithFields");
2925-
// }
2926-
// }
2927-
if (!method.IsVirtual && _context.Annotations.FlowAnnotations.MethodHasNoAnnotatedParameters (method))
2928-
return;
2929-
29302888
_context.LogWarning (
29312889
$"Method '{method.GetDisplayName ()}' with parameters or return value with `DynamicallyAccessedMembersAttribute` is accessed via reflection. Trimmer can't guarantee availability of the requirements of the field.",
29322890
2111,

‎test/Mono.Linker.Tests.Cases/Reflection/TypeHierarchyReflectionWarnings.cs

Lines changed: 29 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -120,6 +120,27 @@ public void DAMMethod (
120120
Type t
121121
)
122122
{ }
123+
124+
[Kept]
125+
// No warning for non-virtual method which only has DAM on return parameter
126+
[return: KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
127+
[return: DynamicallyAccessedMembersAttribute (DynamicallyAccessedMemberTypes.PublicMethods)]
128+
public Type DAMReturnMethod () => null;
129+
130+
[Kept]
131+
[ExpectedWarning ("IL2114", nameof (AnnotatedPublicMethods), nameof (DAMVirtualMethod))]
132+
public virtual void DAMVirtualMethod (
133+
[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
134+
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
135+
Type type
136+
)
137+
{ }
138+
139+
[Kept]
140+
[ExpectedWarning ("IL2114", nameof (AnnotatedPublicMethods), nameof (DAMReturnVirtualMethod))]
141+
[return: KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
142+
[return: DynamicallyAccessedMembersAttribute (DynamicallyAccessedMemberTypes.PublicMethods)]
143+
public virtual Type DAMReturnVirtualMethod () => null;
123144
}
124145

125146
[Kept]
@@ -136,7 +157,6 @@ class AnnotatedPublicFields
136157
}
137158

138159
[Kept]
139-
[KeptMember ("get_DAMProperty()")]
140160
[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
141161
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicProperties)]
142162
class AnnotatedPublicProperties
@@ -146,9 +166,13 @@ class AnnotatedPublicProperties
146166
[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
147167
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
148168
public static string DAMProperty {
149-
// Property access reports warnings on getter/setter
150-
[ExpectedWarning ("IL2114", nameof (AnnotatedPublicProperties), nameof (DAMProperty) + ".get")]
169+
[Kept]
170+
// No warning for getter since return value is not annotated
151171
get;
172+
[Kept]
173+
// Property access reports warnings on getter/setter
174+
[ExpectedWarning ("IL2114", nameof (AnnotatedPublicProperties), nameof (DAMProperty) + ".set")]
175+
set;
152176
}
153177
}
154178

@@ -253,7 +277,6 @@ public virtual void RUCVirtualMethod () { }
253277

254278
[KeptBaseType (typeof (Base))]
255279
[KeptMember (".ctor()")]
256-
[KeptMember ("get_DAMVirtualProperty()")]
257280
[ExpectedWarning ("IL2113", "--RUCBaseMethod--")]
258281
[ExpectedWarning ("IL2113", "--Base.RUCVirtualMethod--")]
259282
[ExpectedWarning ("IL2115", nameof (Base), nameof (Base.DAMVirtualProperty) + ".get")]
@@ -283,13 +306,12 @@ public override void RUCVirtualMethod () { }
283306
[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
284307
// shouldn't warn because we warn on the base getter instead
285308
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.NonPublicMethods)]
286-
public override string DAMVirtualProperty { get; }
309+
public override string DAMVirtualProperty { [Kept] get; }
287310

288311
}
289312

290313
[KeptBaseType (typeof (AnnotatedDerivedFromBase))]
291314
[KeptMember (".ctor()")]
292-
[KeptMember ("get_DAMVirtualProperty()")]
293315
[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
294316
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicFields)]
295317
// Warnings about base members could go away with https://github.com/mono/linker/issues/2175
@@ -324,7 +346,7 @@ public override void RUCVirtualMethod () { }
324346
[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
325347
// shouldn't warn because we warn on the base getter instead
326348
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.NonPublicMethods)]
327-
public override string DAMVirtualProperty { get; }
349+
public override string DAMVirtualProperty { [Kept] get; }
328350
}
329351

330352
[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]

0 commit comments

Comments
 (0)