Skip to content

Fix dataflow warnings for invalid trim annotations #106672

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Aug 23, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -440,6 +440,7 @@ protected override TypeAnnotations CreateValueFromKey(TypeDesc key)
returnAnnotation = GetMemberTypesForDynamicallyAccessedMembersAttribute(reader, parameter.GetCustomAttributes());
if (returnAnnotation != DynamicallyAccessedMemberTypes.None && !IsTypeInterestingForDataflow(signature.ReturnType))
{
returnAnnotation = DynamicallyAccessedMemberTypes.None;
_logger.LogWarning(method, DiagnosticId.DynamicallyAccessedMembersOnMethodReturnValueCanOnlyApplyToTypesOrStrings, method.GetDisplayName());
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using ILLink.RoslynAnalyzer.TrimAnalysis;
using ILLink.Shared;
using ILLink.Shared.TrimAnalysis;
using ILLink.Shared.TypeSystemProxy;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.Diagnostics;

Expand Down Expand Up @@ -245,12 +246,16 @@ static void VerifyDamOnMethodsMatch (SymbolAnalysisContext context, IMethodSymbo
}
}

if (!overrideMethod.IsStatic && overrideMethod.GetDynamicallyAccessedMemberTypes () != baseMethod.GetDynamicallyAccessedMemberTypes ()) {
var methodOrigin = origin ?? overrideMethod;
context.ReportDiagnostic (Diagnostic.Create (
DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersMismatchOnImplicitThisBetweenOverrides),
GetPrimaryLocation (methodOrigin.Locations),
overrideMethod.GetDisplayName (), baseMethod.GetDisplayName ()));
if (!overrideMethod.IsStatic) {
var overrideMethodThisAnnotation = FlowAnnotations.GetMethodParameterAnnotation (new ParameterProxy (new (overrideMethod), (ParameterIndex) 0));
var baseMethodThisAnnotation = FlowAnnotations.GetMethodParameterAnnotation (new ParameterProxy (new (baseMethod), (ParameterIndex) 0));
if (overrideMethodThisAnnotation != baseMethodThisAnnotation) {
var methodOrigin = origin ?? overrideMethod;
context.ReportDiagnostic (Diagnostic.Create (
DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersMismatchOnImplicitThisBetweenOverrides),
GetPrimaryLocation (methodOrigin.Locations),
overrideMethod.GetDisplayName (), baseMethod.GetDisplayName ()));
}
}
}

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

// None on the return type of 'get' matches unannotated
Expand All @@ -283,11 +290,10 @@ static void VerifyDamOnPropertyAndAccessorMatch (SymbolAnalysisContext context,
// None on parameter of 'set' matches unannotated
|| methodSymbol.MethodKind == MethodKind.PropertySet
&& methodSymbol.Parameters[methodSymbol.Parameters.Length - 1].GetDynamicallyAccessedMemberTypes () != DynamicallyAccessedMemberTypes.None) {
var associatedSymbol = methodSymbol.AssociatedSymbol!;
context.ReportDiagnostic (Diagnostic.Create (
DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersConflictsBetweenPropertyAndAccessor),
GetPrimaryLocation (associatedSymbol.Locations),
associatedSymbol.GetDisplayName (),
GetPrimaryLocation (propertySymbol.Locations),
propertySymbol.GetDisplayName (),
methodSymbol.GetDisplayName ()
));
return;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,12 @@ internal static IEnumerable<AttributeData> GetAttributes (this ISymbol member, s
}
}

/// <summary>
/// Gets DynamicallyAccessedMemberTypes for any DynamicallyAccessedMembers annotation on the symbol.
/// This doesn't validate whether the annotation is valid based on the type of the annotated symbol.
/// Use FlowAnnotations.Get*Annotation when getting annotations that are valid and should participate
/// in dataflow analysis.
/// </summary>
internal static DynamicallyAccessedMemberTypes GetDynamicallyAccessedMemberTypes (this ISymbol symbol)
{
if (!TryGetAttribute (symbol, DynamicallyAccessedMembersAnalyzer.DynamicallyAccessedMembersAttribute, out var dynamicallyAccessedMembers))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -15,11 +15,12 @@ public FieldValue (IFieldSymbol fieldSymbol)
{
FieldSymbol = fieldSymbol;
StaticType = new (fieldSymbol.Type);
DynamicallyAccessedMemberTypes = FlowAnnotations.GetFieldAnnotation (fieldSymbol);
}

public readonly IFieldSymbol FieldSymbol;

public override DynamicallyAccessedMemberTypes DynamicallyAccessedMemberTypes => FieldSymbol.GetDynamicallyAccessedMemberTypes ();
public override DynamicallyAccessedMemberTypes DynamicallyAccessedMemberTypes { get; }

public override IEnumerable<string> GetDiagnosticArgumentsForAnnotationMismatch ()
=> new string[] { FieldSymbol.GetDisplayName () };
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,15 @@ static bool HasParameterAnnotation (IMethodSymbol method) {

static bool ShouldWarnWhenAccessedForReflection (IFieldSymbol field)
{
return field.GetDynamicallyAccessedMemberTypes () != DynamicallyAccessedMemberTypes.None;
return GetFieldAnnotation (field) != DynamicallyAccessedMemberTypes.None;
}

internal static DynamicallyAccessedMemberTypes GetFieldAnnotation (IFieldSymbol field)
{
if (!field.OriginalDefinition.Type.IsTypeInterestingForDataflow (isByRef: field.RefKind is not RefKind.None))
return DynamicallyAccessedMemberTypes.None;

return field.GetDynamicallyAccessedMemberTypes ();
}

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

internal static DynamicallyAccessedMemberTypes GetMethodParameterAnnotation (ParameterProxy param)
{
IMethodSymbol method = param.Method.Method;
if (param.IsImplicitThis)
return method.GetDynamicallyAccessedMemberTypes ();
if (param.IsImplicitThis) {
if (!param.Method.Method.ContainingType.IsTypeInterestingForDataflow (isByRef: false))
return DynamicallyAccessedMemberTypes.None;
return param.Method.Method.GetDynamicallyAccessedMemberTypes ();
}

IParameterSymbol parameter = param.ParameterSymbol!;
bool isByRef = parameter.RefKind is not RefKind.None;
if (!parameter.OriginalDefinition.Type.IsTypeInterestingForDataflow (isByRef))
return DynamicallyAccessedMemberTypes.None;

var damt = parameter.GetDynamicallyAccessedMemberTypes ();

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

public static DynamicallyAccessedMemberTypes GetMethodReturnValueAnnotation (IMethodSymbol method)
{
if (!method.OriginalDefinition.ReturnType.IsTypeInterestingForDataflow (isByRef: method.ReturnsByRef))
return DynamicallyAccessedMemberTypes.None;

var returnDamt = method.GetDynamicallyAccessedMemberTypesOnReturnType ();

// Is this a property getter?
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ void GetDiagnosticsForField (Location location, IFieldSymbol fieldSymbol)
if (fieldSymbol.TryGetRequiresUnreferencedCodeAttribute (out var requiresUnreferencedCodeAttributeData))
ReportRequiresUnreferencedCodeDiagnostic (location, requiresUnreferencedCodeAttributeData, fieldSymbol);

if (fieldSymbol.GetDynamicallyAccessedMemberTypes () != DynamicallyAccessedMemberTypes.None) {
if (FlowAnnotations.GetFieldAnnotation (fieldSymbol) != DynamicallyAccessedMemberTypes.None) {
Copy link
Member

@jtschuster jtschuster Aug 20, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we change most of the uses of ISymbol.GetDynamicallyAccessedMemberTypes() to instead use FlowAnnotations.GetXAnnotation() (except for when we warn on annotations on unsupported types)? Or at least a doc comment on it pointing to the FlowAnnotations for most use cases.

Also, I think we might still have extra warnings about DAM annotation mismatch property definition and accessor, but that can be a separate issue.

if (!overrideMethod.IsStatic && overrideMethod.GetDynamicallyAccessedMemberTypes () != baseMethod.GetDynamicallyAccessedMemberTypes ()) {
var methodOrigin = origin ?? overrideMethod;
context.ReportDiagnostic (Diagnostic.Create (
DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersMismatchOnImplicitThisBetweenOverrides),
GetPrimaryLocation (methodOrigin.Locations),
overrideMethod.GetDisplayName (), baseMethod.GetDisplayName ()));
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I fixed the property/accessor mismatch, and one other case (virtual/override mismatch - this was still calling GetDynamicallyAccessedMemberTypes).

var diagnosticContext = new DiagnosticContext (location, _reportDiagnostic);
diagnosticContext.AddDiagnostic (DiagnosticId.DynamicallyAccessedMembersFieldAccessedViaReflection, fieldSymbol.GetDisplayName ());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,7 @@ public override MultiValue VisitInstanceReference (IInstanceReferenceOperation i
// It can also happen that we see this for a static method - for example a delegate creation
// over a local function does this, even thought the "this" makes no sense inside a static scope.
if (OwningSymbol is IMethodSymbol method && !method.IsStatic)
return new MethodParameterValue (method, (ParameterIndex) 0, method.GetDynamicallyAccessedMemberTypes ());
return new MethodParameterValue (method, (ParameterIndex) 0, FlowAnnotations.GetMethodParameterAnnotation (new ParameterProxy (new (method), (ParameterIndex) 0)));

return TopValue;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,7 @@ TypeAnnotations BuildTypeAnnotations (TypeDefinition type)

DynamicallyAccessedMemberTypes returnAnnotation = GetMemberTypesForDynamicallyAccessedMembersAttribute (method, providerIfNotMember: method.MethodReturnType);
if (returnAnnotation != DynamicallyAccessedMemberTypes.None && !IsTypeInterestingForDataflow (method.ReturnType)) {
returnAnnotation = DynamicallyAccessedMemberTypes.None;
_context.LogWarning (method, DiagnosticId.DynamicallyAccessedMembersOnMethodReturnValueCanOnlyApplyToTypesOrStrings, method.GetDisplayName ());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,6 @@ class AttributeRequiresTypeArrayAttribute : Attribute
{
[Kept]
[ExpectedWarning ("IL2098")]
[UnexpectedWarning ("IL2067", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
public AttributeRequiresTypeArrayAttribute (
[KeptAttributeAttribute (typeof (DynamicallyAccessedMembersAttribute))]
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
Expand All @@ -191,7 +190,6 @@ static void RequirePublicFields (

[Kept]
[KeptAttributeAttribute (typeof (AttributeRequiresTypeArrayAttribute))]
[UnexpectedWarning ("IL2062", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
[AttributeRequiresTypeArray (new Type[] { typeof (int) })]
public static void Test () {
typeof (AnnotationOnTypeArray).GetMethod ("Test").GetCustomAttribute (typeof (AttributeRequiresTypeArrayAttribute));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -338,13 +338,11 @@ static void RequirePublicFields (
{
}

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

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

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

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

class GenericField<T>
{
[ExpectedWarning ("IL2097")]
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
public static T field;
}

static void TestTypeGenericParameter ()
{
GenericField<Type>.field = GetUnknownType ();
}

public static void Test ()
{
TestUnsupportedType ();
StringRef.Test ();
TestTypeGenericParameter ();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,6 @@ class UnsupportedType
static UnsupportedType GetUnsupportedTypeInstance () => null;

[ExpectedWarning ("IL2098", nameof (UnsupportedType))]
[UnexpectedWarning ("IL2067", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
static void RequirePublicMethods (
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
UnsupportedType unsupportedTypeInstance)
Expand All @@ -261,7 +260,6 @@ static void RequirePublicFields (
{
}

[UnexpectedWarning ("IL2072", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
static void TestUnsupportedType ()
{
var t = GetUnsupportedTypeInstance ();
Expand All @@ -271,7 +269,6 @@ static void TestUnsupportedType ()
static Type[] GetTypeArray () => null;

[ExpectedWarning ("IL2098")]
[UnexpectedWarning ("IL2067", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
static void RequirePublicMethods (
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
Type[] types)
Expand All @@ -286,7 +283,6 @@ static void RequirePublicFields (
{
}

[UnexpectedWarning ("IL2072", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
static void TestTypeArray ()
{
var types = GetTypeArray ();
Expand All @@ -296,7 +292,6 @@ static void TestTypeArray ()
static unsafe Type* GetTypePtr () => throw null;

[ExpectedWarning ("IL2098")]
[UnexpectedWarning ("IL2067", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
static unsafe void RequirePublicMethods (
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
Type* typePtr)
Expand All @@ -311,7 +306,6 @@ static unsafe void RequirePublicFields (
{
}

[UnexpectedWarning ("IL2072", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
static unsafe void TestTypePointer ()
{
var typePtr = GetTypePtr ();
Expand All @@ -321,7 +315,6 @@ static unsafe void TestTypePointer ()
static T GetTConstrainedToType<T> () where T : Type => throw null;

[ExpectedWarning ("IL2098")]
[UnexpectedWarning ("IL2067", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
static void RequirePublicMethods<T> (
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
T t) where T : Type
Expand All @@ -336,7 +329,6 @@ static void RequirePublicFields<T> (
{
}

[UnexpectedWarning ("IL2072", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
static void TestTypeGenericParameter ()
{
var t = GetTConstrainedToType<Type> ();
Expand All @@ -346,7 +338,6 @@ static void TestTypeGenericParameter ()
static ref string GetStringRef () => throw null;

[ExpectedWarning ("IL2098")]
[UnexpectedWarning ("IL2067", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
static void RequirePublicMethods (
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
ref string stringRef)
Expand All @@ -361,7 +352,6 @@ static void RequirePublicFields (
{
}

[UnexpectedWarning ("IL2072", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
static void TestStringRef ()
{
var stringRef = GetStringRef ();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -190,7 +190,6 @@ class AnnotationOnUnsupportedReturnType
{
class UnsupportedType
{
[UnexpectedWarning ("IL2082", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
public UnsupportedType () {
RequirePublicFields (this);
}
Expand All @@ -199,9 +198,6 @@ public UnsupportedType () {
static UnsupportedType GetUnsupportedTypeInstance () => null;

[ExpectedWarning ("IL2106")]
// Linker and NativeAot should not produce IL2073
// They produce dataflow warnings despite the invalid annotations.
[UnexpectedWarning ("IL2073", Tool.Trimmer | Tool.NativeAot, "https://github.com/dotnet/runtime/issues/101211")]
[return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
static UnsupportedType GetWithPublicMethods () {
return GetUnsupportedTypeInstance ();
Expand All @@ -214,13 +210,11 @@ static void RequirePublicFields (
{
}

[UnexpectedWarning ("IL2072", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
static void TestMethodReturnValue () {
var t = GetWithPublicMethods ();
RequirePublicFields (t);
}

[UnexpectedWarning ("IL2072", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
static void TestCtorReturnValue () {
var t = new UnsupportedType ();
RequirePublicFields (t);
Expand All @@ -239,7 +233,6 @@ static void RequirePublicFields (
{
}

[UnexpectedWarning ("IL2072", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
public static void Test ()
{
var instance = new StringRefReturnValue ();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -122,7 +122,6 @@ static void RequirePublicFields (
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
UnsupportedType unsupportedTypeInstance) { }

[UnexpectedWarning ("IL2075", nameof (UnsupportedType), nameof (UnsupportedType.GetMethod), Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
static void TestMethodThisParameter () {
var t = GetUnsupportedTypeInstance ();
t.GetMethod ("foo");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -885,7 +885,6 @@ static void RequirePublicFields (
{
}

[UnexpectedWarning ("IL2072", Tool.Analyzer, "https://github.com/dotnet/runtime/issues/101211")]
public static void Test ()
{
var instance = new StringRefProperty ();
Expand All @@ -894,10 +893,23 @@ public static void Test ()
}
}

[ExpectedWarning ("IL2099")]
[DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicFields)]
static object UnsupportedPropertyAnnotationMismatch {
[ExpectedWarning ("IL2106")]
[return: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
get;
[ExpectedWarning ("IL2098")]
[param: DynamicallyAccessedMembers (DynamicallyAccessedMemberTypes.PublicMethods)]
set;
}

public static void Test ()
{
_ = PropertyWithUnsupportedType;
StringRefProperty.Test ();
_ = UnsupportedPropertyAnnotationMismatch;
UnsupportedPropertyAnnotationMismatch = null;
}
}

Expand Down
Loading
Loading