Skip to content

Commit 8fd46b2

Browse files
sbomersbomer
authored
Fix dataflow warnings for invalid trim annotations (#106672)
`DynamicallyAccessedMembers` annotations are only allowed on parameters of certain types, such as `Type` and `string`. While the analyzer correctly reports a warning about annotations on unsupported types, it still respects those annotations: ```csharp using System.Diagnostics.CodeAnalysis; class Program { static void Main() { RequireAll(GetFoo()); // Unexpected IL2072 because GetFoo return value doesn't satisfy All } static void RequireAll([DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)] Foo f) {} // IL2098 for bad annotation, expected static Foo GetFoo() => throw null; } class Foo {} ``` ``` Program.cs(8,17): warning IL2098: Parameter 'f' of method 'Program.RequireAll(Foo)' has 'DynamicallyAccessedMembersAttribute', but that attribute can only be applied to parameters of type 'System.Type' or 'System.String'. Program.cs(5,9): warning IL2072: 'f' argument does not satisfy 'DynamicallyAccessedMemberTypes.All' in call to 'Program.RequireAll(Foo)'. The return value of method 'Program.GetFoo()' does not have matching annotations. The source value must declare at least the same requirements as those declared on the target location it is assigned to. ``` IL2098 is expected, but IL2072 is not. The invalid annotations should not produce further warnings for assignments to that location. This fixes that.
1 parent 0a31fd7 commit 8fd46b2

16 files changed

Lines changed: 115 additions & 43 deletions

File tree

‎src/coreclr/tools/aot/ILCompiler.Compiler/Compiler/Dataflow/FlowAnnotations.cs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -440,6 +440,7 @@ protected override TypeAnnotations CreateValueFromKey(TypeDesc key)
440440
returnAnnotation = GetMemberTypesForDynamicallyAccessedMembersAttribute(reader, parameter.GetCustomAttributes());
441441
if (returnAnnotation != DynamicallyAccessedMemberTypes.None && !IsTypeInterestingForDataflow(signature.ReturnType))
442442
{
443+
returnAnnotation = DynamicallyAccessedMemberTypes.None;
443444
_logger.LogWarning(method, DiagnosticId.DynamicallyAccessedMembersOnMethodReturnValueCanOnlyApplyToTypesOrStrings, method.GetDisplayName());
444445
}
445446
}

‎src/tools/illink/src/ILLink.RoslynAnalyzer/DynamicallyAccessedMembersAnalyzer.cs‎

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
using ILLink.RoslynAnalyzer.TrimAnalysis;
1010
using ILLink.Shared;
1111
using ILLink.Shared.TrimAnalysis;
12+
using ILLink.Shared.TypeSystemProxy;
1213
using Microsoft.CodeAnalysis;
1314
using Microsoft.CodeAnalysis.Diagnostics;
1415

@@ -245,12 +246,16 @@ static void VerifyDamOnMethodsMatch (SymbolAnalysisContext context, IMethodSymbo
245246
}
246247
}
247248

248-
if (!overrideMethod.IsStatic && overrideMethod.GetDynamicallyAccessedMemberTypes () != baseMethod.GetDynamicallyAccessedMemberTypes ()) {
249-
var methodOrigin = origin ?? overrideMethod;
250-
context.ReportDiagnostic (Diagnostic.Create (
251-
DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersMismatchOnImplicitThisBetweenOverrides),
252-
GetPrimaryLocation (methodOrigin.Locations),
253-
overrideMethod.GetDisplayName (), baseMethod.GetDisplayName ()));
249+
if (!overrideMethod.IsStatic) {
250+
var overrideMethodThisAnnotation = FlowAnnotations.GetMethodParameterAnnotation (new ParameterProxy (new (overrideMethod), (ParameterIndex) 0));
251+
var baseMethodThisAnnotation = FlowAnnotations.GetMethodParameterAnnotation (new ParameterProxy (new (baseMethod), (ParameterIndex) 0));
252+
if (overrideMethodThisAnnotation != baseMethodThisAnnotation) {
253+
var methodOrigin = origin ?? overrideMethod;
254+
context.ReportDiagnostic (Diagnostic.Create (
255+
DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersMismatchOnImplicitThisBetweenOverrides),
256+
GetPrimaryLocation (methodOrigin.Locations),
257+
overrideMethod.GetDisplayName (), baseMethod.GetDisplayName ()));
258+
}
254259
}
255260
}
256261

@@ -274,7 +279,9 @@ static void VerifyDamOnInterfaceAndImplementationMethodsMatch (SymbolAnalysisCon
274279
static void VerifyDamOnPropertyAndAccessorMatch (SymbolAnalysisContext context, IMethodSymbol methodSymbol)
275280
{
276281
if ((methodSymbol.MethodKind != MethodKind.PropertyGet && methodSymbol.MethodKind != MethodKind.PropertySet)
277-
|| (methodSymbol.AssociatedSymbol?.GetDynamicallyAccessedMemberTypes () == DynamicallyAccessedMemberTypes.None))
282+
|| methodSymbol.AssociatedSymbol is not IPropertySymbol propertySymbol
283+
|| !propertySymbol.Type.IsTypeInterestingForDataflow (isByRef: propertySymbol.RefKind is not RefKind.None)
284+
|| propertySymbol.GetDynamicallyAccessedMemberTypes () == DynamicallyAccessedMemberTypes.None)
278285
return;
279286

280287
// None on the return type of 'get' matches unannotated
@@ -283,11 +290,10 @@ static void VerifyDamOnPropertyAndAccessorMatch (SymbolAnalysisContext context,
283290
// None on parameter of 'set' matches unannotated
284291
|| methodSymbol.MethodKind == MethodKind.PropertySet
285292
&& methodSymbol.Parameters[methodSymbol.Parameters.Length - 1].GetDynamicallyAccessedMemberTypes () != DynamicallyAccessedMemberTypes.None) {
286-
var associatedSymbol = methodSymbol.AssociatedSymbol!;
287293
context.ReportDiagnostic (Diagnostic.Create (
288294
DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersConflictsBetweenPropertyAndAccessor),
289-
GetPrimaryLocation (associatedSymbol.Locations),
290-
associatedSymbol.GetDisplayName (),
295+
GetPrimaryLocation (propertySymbol.Locations),
296+
propertySymbol.GetDisplayName (),
291297
methodSymbol.GetDisplayName ()
292298
));
293299
return;

‎src/tools/illink/src/ILLink.RoslynAnalyzer/ISymbolExtensions.cs‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,12 @@ internal static IEnumerable<AttributeData> GetAttributes (this ISymbol member, s
4747
}
4848
}
4949

50+
/// <summary>
51+
/// Gets DynamicallyAccessedMemberTypes for any DynamicallyAccessedMembers annotation on the symbol.
52+
/// This doesn't validate whether the annotation is valid based on the type of the annotated symbol.
53+
/// Use FlowAnnotations.Get*Annotation when getting annotations that are valid and should participate
54+
/// in dataflow analysis.
55+
/// </summary>
5056
internal static DynamicallyAccessedMemberTypes GetDynamicallyAccessedMemberTypes (this ISymbol symbol)
5157
{
5258
if (!TryGetAttribute (symbol, DynamicallyAccessedMembersAnalyzer.DynamicallyAccessedMembersAttribute, out var dynamicallyAccessedMembers))

‎src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/FieldValue.cs‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -15,11 +15,12 @@ public FieldValue (IFieldSymbol fieldSymbol)
1515
{
1616
FieldSymbol = fieldSymbol;
1717
StaticType = new (fieldSymbol.Type);
18+
DynamicallyAccessedMemberTypes = FlowAnnotations.GetFieldAnnotation (fieldSymbol);
1819
}
1920

2021
public readonly IFieldSymbol FieldSymbol;
2122

22-
public override DynamicallyAccessedMemberTypes DynamicallyAccessedMemberTypes => FieldSymbol.GetDynamicallyAccessedMemberTypes ();
23+
public override DynamicallyAccessedMemberTypes DynamicallyAccessedMemberTypes { get; }
2324

2425
public override IEnumerable<string> GetDiagnosticArgumentsForAnnotationMismatch ()
2526
=> new string[] { FieldSymbol.GetDisplayName () };

‎src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/FlowAnnotations.cs‎

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -104,7 +104,15 @@ static bool HasParameterAnnotation (IMethodSymbol method) {
104104

105105
static bool ShouldWarnWhenAccessedForReflection (IFieldSymbol field)
106106
{
107-
return field.GetDynamicallyAccessedMemberTypes () != DynamicallyAccessedMemberTypes.None;
107+
return GetFieldAnnotation (field) != DynamicallyAccessedMemberTypes.None;
108+
}
109+
110+
internal static DynamicallyAccessedMemberTypes GetFieldAnnotation (IFieldSymbol field)
111+
{
112+
if (!field.OriginalDefinition.Type.IsTypeInterestingForDataflow (isByRef: field.RefKind is not RefKind.None))
113+
return DynamicallyAccessedMemberTypes.None;
114+
115+
return field.GetDynamicallyAccessedMemberTypes ();
108116
}
109117

110118
internal static DynamicallyAccessedMemberTypes GetTypeAnnotations (INamedTypeSymbol type)
@@ -128,11 +136,17 @@ internal static DynamicallyAccessedMemberTypes GetTypeAnnotations (INamedTypeSym
128136

129137
internal static DynamicallyAccessedMemberTypes GetMethodParameterAnnotation (ParameterProxy param)
130138
{
131-
IMethodSymbol method = param.Method.Method;
132-
if (param.IsImplicitThis)
133-
return method.GetDynamicallyAccessedMemberTypes ();
139+
if (param.IsImplicitThis) {
140+
if (!param.Method.Method.ContainingType.IsTypeInterestingForDataflow (isByRef: false))
141+
return DynamicallyAccessedMemberTypes.None;
142+
return param.Method.Method.GetDynamicallyAccessedMemberTypes ();
143+
}
134144

135145
IParameterSymbol parameter = param.ParameterSymbol!;
146+
bool isByRef = parameter.RefKind is not RefKind.None;
147+
if (!parameter.OriginalDefinition.Type.IsTypeInterestingForDataflow (isByRef))
148+
return DynamicallyAccessedMemberTypes.None;
149+
136150
var damt = parameter.GetDynamicallyAccessedMemberTypes ();
137151

138152
var parameterMethod = (IMethodSymbol) parameter.ContainingSymbol;
@@ -155,6 +169,9 @@ internal static DynamicallyAccessedMemberTypes GetMethodParameterAnnotation (Par
155169

156170
public static DynamicallyAccessedMemberTypes GetMethodReturnValueAnnotation (IMethodSymbol method)
157171
{
172+
if (!method.OriginalDefinition.ReturnType.IsTypeInterestingForDataflow (isByRef: method.ReturnsByRef))
173+
return DynamicallyAccessedMemberTypes.None;
174+
158175
var returnDamt = method.GetDynamicallyAccessedMemberTypesOnReturnType ();
159176

160177
// Is this a property getter?

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -191,7 +191,7 @@ void GetDiagnosticsForField (Location location, IFieldSymbol fieldSymbol)
191191
if (fieldSymbol.TryGetRequiresUnreferencedCodeAttribute (out var requiresUnreferencedCodeAttributeData))
192192
ReportRequiresUnreferencedCodeDiagnostic (location, requiresUnreferencedCodeAttributeData, fieldSymbol);
193193

194-
if (fieldSymbol.GetDynamicallyAccessedMemberTypes () != DynamicallyAccessedMemberTypes.None) {
194+
if (FlowAnnotations.GetFieldAnnotation (fieldSymbol) != DynamicallyAccessedMemberTypes.None) {
195195
var diagnosticContext = new DiagnosticContext (location, _reportDiagnostic);
196196
diagnosticContext.AddDiagnostic (DiagnosticId.DynamicallyAccessedMembersFieldAccessedViaReflection, fieldSymbol.GetDisplayName ());
197197
}

‎src/tools/illink/src/ILLink.RoslynAnalyzer/TrimAnalysis/TrimAnalysisVisitor.cs‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -147,7 +147,7 @@ public override MultiValue VisitInstanceReference (IInstanceReferenceOperation i
147147
// It can also happen that we see this for a static method - for example a delegate creation
148148
// over a local function does this, even thought the "this" makes no sense inside a static scope.
149149
if (OwningSymbol is IMethodSymbol method && !method.IsStatic)
150-
return new MethodParameterValue (method, (ParameterIndex) 0, method.GetDynamicallyAccessedMemberTypes ());
150+
return new MethodParameterValue (method, (ParameterIndex) 0, FlowAnnotations.GetMethodParameterAnnotation (new ParameterProxy (new (method), (ParameterIndex) 0)));
151151

152152
return TopValue;
153153
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -267,6 +267,7 @@ TypeAnnotations BuildTypeAnnotations (TypeDefinition type)
267267

268268
DynamicallyAccessedMemberTypes returnAnnotation = GetMemberTypesForDynamicallyAccessedMembersAttribute (method, providerIfNotMember: method.MethodReturnType);
269269
if (returnAnnotation != DynamicallyAccessedMemberTypes.None && !IsTypeInterestingForDataflow (method.ReturnType)) {
270+
returnAnnotation = DynamicallyAccessedMemberTypes.None;
270271
_context.LogWarning (method, DiagnosticId.DynamicallyAccessedMembersOnMethodReturnValueCanOnlyApplyToTypesOrStrings, method.GetDisplayName ());
271272
}
272273

‎src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/AttributeConstructorDataflow.cs‎

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,6 @@ class AttributeRequiresTypeArrayAttribute : Attribute
170170
{
171171
[Kept]
172172
[ExpectedWarning ("IL2098")]
173-
[UnexpectedWarning ("IL2067", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
174173
public AttributeRequiresTypeArrayAttribute (
175174
[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
176175
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
@@ -191,7 +190,6 @@ static void RequirePublicFields (
191190

192191
[Kept]
193192
[KeptAttributeAttribute (typeof (AttributeRequiresTypeArrayAttribute))]
194-
[UnexpectedWarning ("IL2062", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
195193
[AttributeRequiresTypeArray (new Type[] { typeof (int) })]
196194
public static void Test () {
197195
typeof (AnnotationOnTypeArray).GetMethod ("Test").GetCustomAttribute (typeof (AttributeRequiresTypeArrayAttribute));

‎src/tools/illink/test/Mono.Linker.Tests.Cases/DataFlow/FieldDataFlow.cs‎

Lines changed: 13 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -338,13 +338,11 @@ static void RequirePublicFields (
338338
{
339339
}
340340

341-
[UnexpectedWarning ("IL2077", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
342341
static void TestFlowOutOfField ()
343342
{
344343
RequirePublicFields (unsupportedTypeInstance);
345344
}
346345

347-
[UnexpectedWarning ("IL2074", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
348346
static void TestUnsupportedType () {
349347
var t = GetUnsupportedTypeInstance ();
350348
unsupportedTypeInstance = t;
@@ -357,7 +355,6 @@ ref struct StringRef
357355
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
358356
public ref string stringRef;
359357

360-
[UnexpectedWarning ("IL2069", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
361358
public StringRef (ref string s)
362359
{
363360
stringRef = ref s;
@@ -372,7 +369,6 @@ static void RequirePublicFields (
372369
{
373370
}
374371

375-
[UnexpectedWarning ("IL2077", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
376372
void TestFlowOutOfField ()
377373
{
378374
RequirePublicFields (ref stringRef);
@@ -386,10 +382,23 @@ public static void Test ()
386382
}
387383
}
388384

385+
class GenericField<T>
386+
{
387+
[ExpectedWarning ("IL2097")]
388+
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
389+
public static T field;
390+
}
391+
392+
static void TestTypeGenericParameter ()
393+
{
394+
GenericField<Type>.field = GetUnknownType ();
395+
}
396+
389397
public static void Test ()
390398
{
391399
TestUnsupportedType ();
392400
StringRef.Test ();
401+
TestTypeGenericParameter ();
393402
}
394403
}
395404

0 commit comments

Comments
 (0)