Skip to content

Commit 8c0df91

Browse files
vitek-karasvitek-karas
authored
Share intrinsics for Expression.Property and Field (#2671)
Shares two more intrinsics, with supporting infra. Fixed a bug in the intrinsics - passing null to the name of a property/field will throw at runtime, so no need to validate anything. Modifies the existing tests to add warnings, since that is the only verifyable behavior for the analyzer.
1 parent a8f9532 commit 8c0df91

File tree

11 files changed

+194
-100
lines changed

11 files changed

+194
-100
lines changed

‎src/ILLink.RoslynAnalyzer/DynamicallyAccessedMembersAnalyzer.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,7 @@ static ImmutableArray<DiagnosticDescriptor> GetSupportedDiagnostics ()
3737
diagDescriptorsArrayBuilder.Add (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersFieldAccessedViaReflection));
3838
diagDescriptorsArrayBuilder.Add (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersMethodAccessedViaReflection));
3939
diagDescriptorsArrayBuilder.Add (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.UnrecognizedTypeInRuntimeHelpersRunClassConstructor));
40+
diagDescriptorsArrayBuilder.Add (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.PropertyAccessorParameterInLinqExpressionsCannotBeStaticallyDetermined));
4041

4142
return diagDescriptorsArrayBuilder.ToImmutable ();
4243

‎src/ILLink.RoslynAnalyzer/TrimAnalysis/HandleCallAction.cs‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
// The .NET Foundation licenses this file to you under the MIT license.
33

44
using System.Collections.Generic;
5+
using System.Diagnostics;
56
using System.Diagnostics.CodeAnalysis;
67
using System.Reflection;
78
using ILLink.RoslynAnalyzer;
@@ -79,6 +80,18 @@ private partial void MarkMethod (MethodProxy method)
7980
// TODO: Does the analyzer need to do something here?
8081
private partial void MarkType (TypeProxy type) { }
8182

83+
private partial bool MarkAssociatedProperty (MethodProxy method)
84+
{
85+
if (method.Method.MethodKind == MethodKind.PropertyGet || method.Method.MethodKind == MethodKind.PropertySet) {
86+
var property = (IPropertySymbol) method.Method.AssociatedSymbol!;
87+
Debug.Assert (property != null);
88+
ReflectionAccessAnalyzer.GetReflectionAccessDiagnosticsForProperty (_diagnosticContext, property!);
89+
return true;
90+
}
91+
92+
return false;
93+
}
94+
8295
private partial string GetContainingSymbolDisplayName () => _operation.FindContainingSymbol (_owningSymbol).GetDisplayName ();
8396
}
8497
}

‎src/ILLink.RoslynAnalyzer/TrimAnalysis/ReflectionAccessAnalyzer.cs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@ internal void GetReflectionAccessDiagnostics (in DiagnosticContext diagnosticCon
2424
GetDiagnosticsForField (diagnosticContext, field);
2525
break;
2626
case IPropertySymbol property:
27-
GetDiagnosticsForProperty (diagnosticContext, property);
27+
GetReflectionAccessDiagnosticsForProperty (diagnosticContext, property);
2828
break;
2929
/* Skip Type and InterfaceImplementation marking since doesnt seem relevant for diagnostic generation
3030
case ITypeSymbol nestedType:
@@ -56,7 +56,7 @@ internal void GetReflectionAccessDiagnosticsForFieldsOnTypeHierarchy (in Diagnos
5656
internal void GetReflectionAccessDiagnosticsForPropertiesOnTypeHierarchy (in DiagnosticContext diagnosticContext, ITypeSymbol typeSymbol, string name, BindingFlags? bindingFlags)
5757
{
5858
foreach (var prop in typeSymbol.GetPropertiesOnTypeHierarchy (p => p.Name == name, bindingFlags))
59-
GetDiagnosticsForProperty (diagnosticContext, prop);
59+
GetReflectionAccessDiagnosticsForProperty (diagnosticContext, prop);
6060
}
6161

6262
static void ReportRequiresUnreferencedCodeDiagnostic (in DiagnosticContext diagnosticContext, AttributeData requiresAttributeData, ISymbol member)
@@ -85,7 +85,7 @@ internal static void GetReflectionAccessDiagnosticsForMethod (in DiagnosticConte
8585
}
8686
}
8787

88-
static void GetDiagnosticsForProperty (in DiagnosticContext diagnosticContext, IPropertySymbol propertySymbol)
88+
internal static void GetReflectionAccessDiagnosticsForProperty (in DiagnosticContext diagnosticContext, IPropertySymbol propertySymbol)
8989
{
9090
if (propertySymbol.SetMethod is not null)
9191
GetReflectionAccessDiagnosticsForMethod (diagnosticContext, propertySymbol.SetMethod);

‎src/ILLink.Shared/TrimAnalysis/HandleCallAction.cs‎

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
using System.Collections.Generic;
66
using System.Diagnostics;
77
using System.Diagnostics.CodeAnalysis;
8+
using System.Linq;
89
using System.Reflection;
910
using System.Runtime.InteropServices;
1011
using ILLink.Shared.DataFlow;
@@ -479,6 +480,66 @@ public bool Invoke (MethodProxy calledMethod, MultiValue instanceValue, IReadOnl
479480
}
480481
break;
481482

483+
//
484+
// System.Linq.Expressions.Expression
485+
//
486+
// static Property (Expression, MethodInfo)
487+
//
488+
case IntrinsicId.Expression_Property when calledMethod.HasParameterOfType (1, "System.Reflection.MethodInfo"): {
489+
foreach (var value in argumentValues[1]) {
490+
if (value is SystemReflectionMethodBaseValue methodBaseValue) {
491+
// We have one of the accessors for the property. The Expression.Property will in this case search
492+
// for the matching PropertyInfo and store that. So to be perfectly correct we need to mark the
493+
// respective PropertyInfo as "accessed via reflection".
494+
if (MarkAssociatedProperty (methodBaseValue.MethodRepresented))
495+
continue;
496+
} else if (value == NullValue.Instance) {
497+
continue;
498+
}
499+
500+
// In all other cases we may not even know which type this is about, so there's nothing we can do
501+
// report it as a warning.
502+
_diagnosticContext.AddDiagnostic (DiagnosticId.PropertyAccessorParameterInLinqExpressionsCannotBeStaticallyDetermined,
503+
GetMethodParameterValue (calledMethod, 1, DynamicallyAccessedMemberTypes.None).GetDiagnosticArgumentsForAnnotationMismatch ().ToArray ());
504+
}
505+
}
506+
break;
507+
508+
//
509+
// System.Linq.Expressions.Expression
510+
//
511+
// static Field (Expression, Type, String)
512+
// static Property (Expression, Type, String)
513+
//
514+
case var fieldOrPropertyInstrinsic when fieldOrPropertyInstrinsic == IntrinsicId.Expression_Field || fieldOrPropertyInstrinsic == IntrinsicId.Expression_Property: {
515+
DynamicallyAccessedMemberTypes memberTypes = fieldOrPropertyInstrinsic == IntrinsicId.Expression_Property
516+
? DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.NonPublicProperties
517+
: DynamicallyAccessedMemberTypes.PublicFields | DynamicallyAccessedMemberTypes.NonPublicFields;
518+
519+
var targetValue = GetMethodParameterValue (calledMethod, 1, memberTypes);
520+
foreach (var value in argumentValues[1]) {
521+
if (value is SystemTypeValue systemTypeValue) {
522+
foreach (var stringParam in argumentValues[2]) {
523+
if (stringParam is KnownStringValue stringValue) {
524+
BindingFlags bindingFlags = argumentValues[0].AsSingleValue () is NullValue ? BindingFlags.Static : BindingFlags.Default;
525+
if (fieldOrPropertyInstrinsic == IntrinsicId.Expression_Property) {
526+
MarkPropertiesOnTypeHierarchy (systemTypeValue.RepresentedType, stringValue.Contents, bindingFlags);
527+
} else {
528+
MarkFieldsOnTypeHierarchy (systemTypeValue.RepresentedType, stringValue.Contents, bindingFlags);
529+
}
530+
} else if (stringParam is NullValue) {
531+
// Null name will always throw, so there's nothing to do
532+
} else {
533+
_requireDynamicallyAccessedMembersAction.Invoke (value, targetValue);
534+
}
535+
}
536+
} else {
537+
_requireDynamicallyAccessedMembersAction.Invoke (value, targetValue);
538+
}
539+
}
540+
}
541+
break;
542+
482543
case IntrinsicId.None:
483544
methodReturnValue = MultiValueLattice.Top;
484545
return false;
@@ -637,6 +698,8 @@ internal static DynamicallyAccessedMemberTypes GetDynamicallyAccessedMemberTypes
637698

638699
private partial void MarkType (TypeProxy type);
639700

701+
private partial bool MarkAssociatedProperty (MethodProxy method);
702+
640703
// Only used for internal diagnostic purposes (not even for warning messages)
641704
private partial string GetContainingSymbolDisplayName ();
642705
}

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

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,6 +86,16 @@ private partial void MarkMethod (MethodProxy method)
8686
private partial void MarkType (TypeProxy type)
8787
=> _reflectionMethodBodyScanner.MarkType (_analysisContext, type.Type);
8888

89+
private partial bool MarkAssociatedProperty (MethodProxy method)
90+
{
91+
if (method.Method.TryGetProperty (out PropertyDefinition? propertyDefinition)) {
92+
_reflectionMethodBodyScanner.MarkProperty (_analysisContext, propertyDefinition);
93+
return true;
94+
}
95+
96+
return false;
97+
}
98+
8999
private partial string GetContainingSymbolDisplayName () => _callingMethodDefinition.GetDisplayName ();
90100
}
91101
}

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

Lines changed: 4 additions & 63 deletions
Original file line numberDiff line numberDiff line change
@@ -287,7 +287,9 @@ public override bool HandleCall (MethodBody callingMethodBody, MethodReference c
287287
|| getRuntimeMember == IntrinsicId.RuntimeReflectionExtensions_GetRuntimeProperty:
288288
case IntrinsicId.Type_GetMember:
289289
case IntrinsicId.Type_GetMethod:
290-
case IntrinsicId.Type_GetNestedType: {
290+
case IntrinsicId.Type_GetNestedType:
291+
case IntrinsicId.Expression_Property when calledMethod.HasParameterOfType (1, "System.Reflection.MethodInfo"):
292+
case var fieldOrPropertyInstrinsic when fieldOrPropertyInstrinsic == IntrinsicId.Expression_Field || fieldOrPropertyInstrinsic == IntrinsicId.Expression_Property: {
291293
var instanceValue = MultiValueLattice.Top;
292294
IReadOnlyList<MultiValue> parameterValues = methodParams;
293295
if (calledMethodDefinition.HasImplicitThis ()) {
@@ -412,67 +414,6 @@ public override bool HandleCall (MethodBody callingMethodBody, MethodReference c
412414
}
413415
break;
414416

415-
//
416-
// System.Linq.Expressions.Expression
417-
//
418-
// static Property (Expression, MethodInfo)
419-
//
420-
case IntrinsicId.Expression_Property when calledMethod.HasParameterOfType (1, "System.Reflection.MethodInfo"): {
421-
foreach (var value in methodParams[1]) {
422-
if (value is SystemReflectionMethodBaseValue methodBaseValue) {
423-
// We have one of the accessors for the property. The Expression.Property will in this case search
424-
// for the matching PropertyInfo and store that. So to be perfectly correct we need to mark the
425-
// respective PropertyInfo as "accessed via reflection".
426-
if (methodBaseValue.MethodRepresented.Method.TryGetProperty (out PropertyDefinition? propertyDefinition)) {
427-
MarkProperty (analysisContext, propertyDefinition);
428-
continue;
429-
}
430-
} else if (value == NullValue.Instance) {
431-
continue;
432-
}
433-
434-
// In all other cases we may not even know which type this is about, so there's nothing we can do
435-
// report it as a warning.
436-
analysisContext.ReportWarning (DiagnosticId.PropertyAccessorParameterInLinqExpressionsCannotBeStaticallyDetermined,
437-
DiagnosticUtilities.GetParameterNameForErrorMessage (calledMethodDefinition.Parameters[1]),
438-
DiagnosticUtilities.GetMethodSignatureDisplayName (calledMethodDefinition));
439-
}
440-
}
441-
break;
442-
443-
//
444-
// System.Linq.Expressions.Expression
445-
//
446-
// static Field (Expression, Type, String)
447-
// static Property (Expression, Type, String)
448-
//
449-
case var fieldOrPropertyInstrinsic when fieldOrPropertyInstrinsic == IntrinsicId.Expression_Field || fieldOrPropertyInstrinsic == IntrinsicId.Expression_Property: {
450-
DynamicallyAccessedMemberTypes memberTypes = fieldOrPropertyInstrinsic == IntrinsicId.Expression_Property
451-
? DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.NonPublicProperties
452-
: DynamicallyAccessedMemberTypes.PublicFields | DynamicallyAccessedMemberTypes.NonPublicFields;
453-
454-
var targetValue = GetMethodParameterValue (calledMethodDefinition, 1, memberTypes);
455-
foreach (var value in methodParams[1]) {
456-
if (value is SystemTypeValue systemTypeValue) {
457-
foreach (var stringParam in methodParams[2]) {
458-
if (stringParam is KnownStringValue stringValue) {
459-
BindingFlags bindingFlags = methodParams[0].AsSingleValue () is NullValue ? BindingFlags.Static : BindingFlags.Default;
460-
if (fieldOrPropertyInstrinsic == IntrinsicId.Expression_Property) {
461-
MarkPropertiesOnTypeHierarchy (analysisContext, systemTypeValue.RepresentedType.Type, filter: p => p.Name == stringValue.Contents, bindingFlags);
462-
} else {
463-
MarkFieldsOnTypeHierarchy (analysisContext, systemTypeValue.RepresentedType.Type, filter: f => f.Name == stringValue.Contents, bindingFlags);
464-
}
465-
} else {
466-
RequireDynamicallyAccessedMembers (analysisContext, value, targetValue);
467-
}
468-
}
469-
} else {
470-
RequireDynamicallyAccessedMembers (analysisContext, value, targetValue);
471-
}
472-
}
473-
}
474-
break;
475-
476417
//
477418
// System.Linq.Expressions.Expression
478419
//
@@ -1151,7 +1092,7 @@ void MarkField (in AnalysisContext analysisContext, FieldDefinition field, Depen
11511092
_markStep.MarkFieldVisibleToReflection (field, new DependencyInfo (dependencyKind, analysisContext.Origin.Provider));
11521093
}
11531094

1154-
void MarkProperty (in AnalysisContext analysisContext, PropertyDefinition property, DependencyKind dependencyKind = DependencyKind.AccessedViaReflection)
1095+
internal void MarkProperty (in AnalysisContext analysisContext, PropertyDefinition property, DependencyKind dependencyKind = DependencyKind.AccessedViaReflection)
11551096
{
11561097
_markStep.MarkPropertyVisibleToReflection (property, new DependencyInfo (dependencyKind, analysisContext.Origin.Provider));
11571098
}

‎test/ILLink.RoslynAnalyzer.Tests/ReflectionTests.cs‎

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,12 @@ public Task ExpressionCallString ()
5454
return RunTest (allowMissingWarnings: true);
5555
}
5656

57+
[Fact]
58+
public Task ExpressionFieldString ()
59+
{
60+
return RunTest ();
61+
}
62+
5763
[Fact]
5864
public Task ExpressionNewType ()
5965
{
@@ -63,7 +69,13 @@ public Task ExpressionNewType ()
6369
[Fact]
6470
public Task ExpressionPropertyMethodInfo ()
6571
{
66-
return RunTest (allowMissingWarnings: true);
72+
return RunTest ();
73+
}
74+
75+
[Fact]
76+
public Task ExpressionPropertyString ()
77+
{
78+
return RunTest ();
6779
}
6880

6981
[Fact]

‎test/ILLink.RoslynAnalyzer.Tests/generated/ILLink.RoslynAnalyzer.Tests.Generator/ILLink.RoslynAnalyzer.Tests.TestCaseGenerator/ReflectionTests.g.cs‎

Lines changed: 0 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -43,18 +43,6 @@ public Task ExpressionCallStringAndLocals ()
4343
return RunTest (allowMissingWarnings: true);
4444
}
4545

46-
[Fact]
47-
public Task ExpressionFieldString ()
48-
{
49-
return RunTest (allowMissingWarnings: true);
50-
}
51-
52-
[Fact]
53-
public Task ExpressionPropertyString ()
54-
{
55-
return RunTest (allowMissingWarnings: true);
56-
}
57-
5846
[Fact]
5947
public Task ObjectGetTypeLibraryMode ()
6048
{

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

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -10,13 +10,16 @@ namespace Mono.Linker.Tests.Cases.Reflection
1010
[ExpectedNoWarnings]
1111
public class ExpressionFieldString
1212
{
13+
[ExpectedWarning ("IL2110", nameof (StaticWithDAM))]
14+
[ExpectedWarning ("IL2110", "_publicFieldOnBase")]
1315
[ExpectedWarning ("IL2072", nameof (Expression) + "." + nameof (Expression.Field))]
1416
public static void Main ()
1517
{
16-
Expression.Field (Expression.Parameter (typeof (int), ""), typeof (ExpressionFieldString), "Field");
18+
Expression.Field (Expression.Parameter (typeof (int), ""), typeof (ExpressionFieldString), "InstanceField");
1719
Expression.Field (null, typeof (ExpressionFieldString), "StaticField");
20+
Expression.Field (null, typeof (ExpressionFieldString), "StaticWithDAM"); // IL2110
1821
Expression.Field (null, typeof (Derived), "_protectedFieldOnBase");
19-
Expression.Field (null, typeof (Derived), "_publicFieldOnBase");
22+
Expression.Field (null, typeof (Derived), "_publicFieldOnBase"); // IL2110
2023
UnknownType.Test ();
2124
UnknownTypeNoAnnotation.Test ();
2225
UnknownString.Test ();
@@ -29,13 +32,20 @@ public static void Main ()
2932
}
3033

3134
[Kept]
32-
private int Field;
35+
private int InstanceField;
3336

3437
[Kept]
3538
static private int StaticField;
3639

40+
[Kept]
41+
[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
42+
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
43+
static private Type StaticWithDAM;
44+
3745
private int UnusedField;
3846

47+
public static int StaticField1 { get => StaticField; set => StaticField = value; }
48+
3949
[Kept]
4050
static Type GetType ()
4151
{
@@ -142,15 +152,16 @@ static void TestNoValueString ()
142152
Expression.Field (null, typeof (Base), noValue);
143153
}
144154

145-
146155
[Kept]
147156
class Base
148157
{
149158
[Kept]
150159
protected static bool _protectedFieldOnBase;
151160

152161
[Kept]
153-
public static bool _publicFieldOnBase;
162+
[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
163+
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
164+
public static Type _publicFieldOnBase;
154165
}
155166

156167
[Kept]

0 commit comments

Comments
 (0)