Skip to content

Commit e640a9e

Browse files
vitek-karasvitek-karas
committed
Only warn on virtual methods with annotate return value
Annotated return value is mostly not problematic (the caller only "Reads" from it, which is always safe). The only case where it's problematic is if something would override the method at runtime (ref emit) in which case the trimmer can't validate the new code that it fulfills the return value requirements. But that can only happen if the method is virtual.
1 parent 4ddd3ed commit e640a9e

File tree

3 files changed

+94
-29
lines changed

3 files changed

+94
-29
lines changed

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

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -75,7 +75,11 @@ public DynamicallyAccessedMemberTypes GetGenericParameterAnnotation (GenericPara
7575

7676
public bool ShouldWarnWhenAccessedForReflection (MethodDefinition method) =>
7777
GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out var annotation) &&
78-
(annotation.ParameterAnnotations != null || annotation.ReturnParameterAnnotation != DynamicallyAccessedMemberTypes.None);
78+
(annotation.ParameterAnnotations != null || annotation.ReturnParameterAnnotation != DynamicallyAccessedMemberTypes.None);
79+
80+
public bool MethodHasNoAnnotatedParameters (MethodDefinition method) =>
81+
GetAnnotations (method.DeclaringType).TryGetAnnotation (method, out var annotation) &&
82+
annotation.ParameterAnnotations == null;
7983

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

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2820,6 +2820,14 @@ void ProcessAnalysisAnnotationsForMethod (MethodDefinition method, DependencyKin
28202820
break;
28212821
}
28222822

2823+
// If the method only has annotation on the return value and it's not virtual avoid warning.
2824+
// Return value annotations are "consumed" by the caller of a method, and as such there is nothing
2825+
// wrong calling these dynamically. The only problem can happen if something overrides a virtual
2826+
// method with annotated return value at runtime - in this case the trimmer can't validate
2827+
// that the method will return only types which fulfill the annotation's requirements.
2828+
if (!method.IsVirtual && _context.Annotations.FlowAnnotations.MethodHasNoAnnotatedParameters (method))
2829+
return;
2830+
28232831
_context.LogWarning (
28242832
$"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.",
28252833
2111,

‎test/Mono.Linker.Tests.Cases/DataFlow/AnnotatedMembersAccessedViaReflection.cs

Lines changed: 81 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -151,45 +151,90 @@ public static void Test ()
151151
class AnnotatedMethodReturnValue
152152
{
153153
[return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
154-
public static Type MethodWithAnnotatedReturnValue () => null;
154+
public static Type StaticMethodWithAnnotatedReturnValue () => null;
155155

156-
[ExpectedWarning ("IL2111", nameof (MethodWithAnnotatedReturnValue))]
157-
static void Reflection ()
156+
[return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
157+
public Type InstanceMethodWithAnnotatedReturnValue () => null;
158+
159+
[return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
160+
public virtual Type VirtualMethodWithAnnotatedReturnValue () => null;
161+
162+
// Only virtual methods should warn - the problem is only possible if something overrides a virtual method.
163+
// Getting an annotated value in itself is not dangerous in any way.
164+
165+
static void ReflectionOnStatic ()
158166
{
159-
typeof (AnnotatedMethodReturnValue).GetMethod (nameof (MethodWithAnnotatedReturnValue)).Invoke (null, null);
167+
typeof (AnnotatedMethodReturnValue).GetMethod (nameof (StaticMethodWithAnnotatedReturnValue)).Invoke (null, null);
160168
}
161169

162-
[ExpectedWarning ("IL2111", nameof (MethodWithAnnotatedReturnValue))]
170+
static void ReflectionOnInstance ()
171+
{
172+
typeof (AnnotatedMethodReturnValue).GetMethod (nameof (InstanceMethodWithAnnotatedReturnValue)).Invoke (null, null);
173+
}
174+
175+
[ExpectedWarning ("IL2111", nameof (VirtualMethodWithAnnotatedReturnValue))]
176+
static void ReflectionOnVirtual ()
177+
{
178+
typeof (AnnotatedMethodReturnValue).GetMethod (nameof (VirtualMethodWithAnnotatedReturnValue)).Invoke (null, null);
179+
}
180+
181+
[ExpectedWarning ("IL2111", nameof (VirtualMethodWithAnnotatedReturnValue))]
163182
[DynamicDependency (DynamicallyAccessedMemberTypes.PublicMethods, typeof (AnnotatedMethodReturnValue))]
164183
static void DynamicDependency ()
165184
{
166185
}
167186

168-
[ExpectedWarning ("IL2111", nameof (MethodWithAnnotatedReturnValue))]
169-
[DynamicDependency (nameof (MethodWithAnnotatedReturnValue), typeof (AnnotatedMethodReturnValue))]
170-
static void DynamicDependencyByName ()
187+
[DynamicDependency (nameof (StaticMethodWithAnnotatedReturnValue), typeof (AnnotatedMethodReturnValue))]
188+
static void DynamicDependencyByNameOnStatic ()
189+
{
190+
}
191+
192+
[DynamicDependency (nameof (InstanceMethodWithAnnotatedReturnValue), typeof (AnnotatedMethodReturnValue))]
193+
static void DynamicDependencyByNameOnInstance ()
194+
{
195+
}
196+
197+
[ExpectedWarning ("IL2111", nameof (VirtualMethodWithAnnotatedReturnValue))]
198+
[DynamicDependency (nameof (VirtualMethodWithAnnotatedReturnValue), typeof (AnnotatedMethodReturnValue))]
199+
static void DynamicDependencyByNameOnVirtual ()
171200
{
172201
}
173202

174-
[ExpectedWarning ("IL2111", nameof (MethodWithAnnotatedReturnValue))]
203+
[ExpectedWarning ("IL2111", nameof (VirtualMethodWithAnnotatedReturnValue))]
175204
static void DynamicallyAccessedMembers ()
176205
{
177206
typeof (AnnotatedMethodReturnValue).RequiresPublicMethods ();
178207
}
179208

180-
[ExpectedWarning ("IL2111", nameof (MethodWithAnnotatedReturnValue))]
181-
static void Ldftn ()
209+
static void LdftnOnStatic ()
182210
{
183-
var _ = new Func<Type> (AnnotatedMethodReturnValue.MethodWithAnnotatedReturnValue);
211+
var _ = new Func<Type> (AnnotatedMethodReturnValue.StaticMethodWithAnnotatedReturnValue);
212+
}
213+
214+
static void LdftnOnInstance ()
215+
{
216+
var _ = new Func<Type> ((new AnnotatedMethodReturnValue ()).InstanceMethodWithAnnotatedReturnValue);
217+
}
218+
219+
[ExpectedWarning ("IL2111", nameof (VirtualMethodWithAnnotatedReturnValue))]
220+
static void LdftnOnVirtual ()
221+
{
222+
var _ = new Func<Type> ((new AnnotatedMethodReturnValue ()).VirtualMethodWithAnnotatedReturnValue);
184223
}
185224

186225
public static void Test ()
187226
{
188-
Reflection ();
227+
ReflectionOnStatic ();
228+
ReflectionOnInstance ();
229+
ReflectionOnVirtual ();
189230
DynamicDependency ();
190-
DynamicDependencyByName ();
231+
DynamicDependencyByNameOnStatic ();
232+
DynamicDependencyByNameOnInstance ();
233+
DynamicDependencyByNameOnVirtual ();
191234
DynamicallyAccessedMembers ();
192-
Ldftn ();
235+
LdftnOnStatic ();
236+
LdftnOnInstance ();
237+
LdftnOnVirtual ();
193238
}
194239
}
195240

@@ -201,6 +246,9 @@ class AnnotatedProperty
201246
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicEvents)]
202247
public static Type PropertyWithAnnotationGetterOnly { get => null; }
203248

249+
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicEvents)]
250+
public virtual Type VirtualPropertyWithAnnotationGetterOnly { get => null; }
251+
204252
class AttributeWithPropertyWithAnnotation : Attribute
205253
{
206254
public AttributeWithPropertyWithAnnotation () { }
@@ -212,21 +260,20 @@ public AttributeWithPropertyWithAnnotation () { }
212260
[ExpectedWarning ("IL2111", nameof (PropertyWithAnnotation))]
213261
static void ReflectionOnPropertyItself ()
214262
{
215-
// TODO: Technically this should warn if one of the setter is annotated since
216-
// linker can't guarantee that it will not be used.
217263
typeof (AnnotatedProperty).GetProperty (nameof (PropertyWithAnnotation));
218264
}
219265

220-
[ExpectedWarning ("IL2111", nameof (PropertyWithAnnotationGetterOnly))]
221266
static void ReflectionOnPropertyWithGetterOnly ()
222267
{
223-
// Following the rules we maintain on normal methods, just returning annotated value is considered dangerous
224-
// (in theory one could use type builder to create an override for the method, and its body would not be validated
225-
// and would need to fulfill the annotation on the return value anyway)
226268
typeof (AnnotatedProperty).GetProperty (nameof (PropertyWithAnnotationGetterOnly));
227269
}
228270

229-
[ExpectedWarning ("IL2111", nameof (PropertyWithAnnotation) + ".get")]
271+
[ExpectedWarning ("IL2111", nameof (VirtualPropertyWithAnnotationGetterOnly))]
272+
static void ReflectionOnPropertyWithGetterOnlyOnVirtual ()
273+
{
274+
typeof (AnnotatedProperty).GetProperty (nameof (VirtualPropertyWithAnnotationGetterOnly));
275+
}
276+
230277
static void ReflectionOnGetter ()
231278
{
232279
typeof (AnnotatedProperty).GetMethod ("get_" + nameof (PropertyWithAnnotation));
@@ -238,23 +285,27 @@ static void ReflectionOnSetter ()
238285
typeof (AnnotatedProperty).GetMethod ("set_" + nameof (PropertyWithAnnotation));
239286
}
240287

288+
[ExpectedWarning ("IL2111", nameof (VirtualPropertyWithAnnotationGetterOnly) + ".get")]
289+
static void ReflectionOnVirtualGetter ()
290+
{
291+
typeof (AnnotatedProperty).GetMethod ("get_" + nameof (VirtualPropertyWithAnnotationGetterOnly));
292+
}
293+
241294
// Should not warn - there's nothing wrong with this
242295
[AttributeWithPropertyWithAnnotation (PropertyWithAnnotation = typeof (TestType))]
243296
static void AnnotatedAttributeProperty ()
244297
{
245298
}
246299

247-
[ExpectedWarning ("IL2111", nameof (PropertyWithAnnotation) + ".get")]
248300
[ExpectedWarning ("IL2111", nameof (PropertyWithAnnotation) + ".set")]
249-
[ExpectedWarning ("IL2111", nameof (PropertyWithAnnotationGetterOnly) + ".get")]
301+
[ExpectedWarning ("IL2111", nameof (VirtualPropertyWithAnnotationGetterOnly) + ".get")]
250302
[DynamicDependency (DynamicallyAccessedMemberTypes.PublicProperties, typeof (AnnotatedProperty))]
251303
static void DynamicDependency ()
252304
{
253305
}
254306

255-
[ExpectedWarning ("IL2111", nameof (PropertyWithAnnotation) + ".get")]
256307
[ExpectedWarning ("IL2111", nameof (PropertyWithAnnotation) + ".set")]
257-
[ExpectedWarning ("IL2111", nameof (PropertyWithAnnotationGetterOnly) + ".get")]
308+
[ExpectedWarning ("IL2111", nameof (VirtualPropertyWithAnnotationGetterOnly) + ".get")]
258309
static void DynamicallyAccessedMembers ()
259310
{
260311
typeof (AnnotatedProperty).RequiresPublicProperties ();
@@ -264,8 +315,10 @@ public static void Test ()
264315
{
265316
ReflectionOnPropertyItself ();
266317
ReflectionOnPropertyWithGetterOnly ();
318+
ReflectionOnPropertyWithGetterOnlyOnVirtual ();
267319
ReflectionOnGetter ();
268320
ReflectionOnSetter ();
321+
ReflectionOnVirtualGetter ();
269322
AnnotatedAttributeProperty ();
270323
DynamicDependency ();
271324
DynamicallyAccessedMembers ();
@@ -393,7 +446,7 @@ public static void Test ()
393446

394447
class AccessThroughLdToken
395448
{
396-
static Type PropertyWithLdToken {
449+
public virtual Type PropertyWithLdToken {
397450
[return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
398451
get {
399452
return null;
@@ -403,7 +456,7 @@ static Type PropertyWithLdToken {
403456
[ExpectedWarning ("IL2111", nameof (PropertyWithLdToken))]
404457
public static void Test ()
405458
{
406-
Expression<Func<Type>> getter = () => PropertyWithLdToken;
459+
Expression<Func<Type>> getter = () => (new AccessThroughLdToken ()).PropertyWithLdToken;
407460
}
408461
}
409462

0 commit comments

Comments
 (0)