Skip to content

Commit

Permalink
Warn on DAM mismatch between overrides (#2656)
Browse files Browse the repository at this point in the history 
* Warn on DAM mismatch between overrides
  • Loading branch information
@jtschuster
jtschuster authored Mar 11, 2022
1 parent 98c81d5 commit ac6cfb3
Show file tree
Hide file tree
Showing 8 changed files with 208 additions and 46 deletions.
81 changes: 80 additions & 1 deletion src/ILLink.RoslynAnalyzer/DynamicallyAccessedMembersAnalyzer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,12 @@ static ImmutableArray<DiagnosticDescriptor> GetSupportedDiagnostics ()
diagDescriptorsArrayBuilder.Add (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersFieldAccessedViaReflection));
diagDescriptorsArrayBuilder.Add (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersMethodAccessedViaReflection));
diagDescriptorsArrayBuilder.Add (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.UnrecognizedTypeInRuntimeHelpersRunClassConstructor));
diagDescriptorsArrayBuilder.Add (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersMismatchOnMethodReturnValueBetweenOverrides));
diagDescriptorsArrayBuilder.Add (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersMismatchOnMethodParameterBetweenOverrides));
diagDescriptorsArrayBuilder.Add (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersMismatchOnGenericParameterBetweenOverrides));
diagDescriptorsArrayBuilder.Add (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersMismatchOnImplicitThisBetweenOverrides));
diagDescriptorsArrayBuilder.Add (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersConflictsBetweenPropertyAndAccessor));
diagDescriptorsArrayBuilder.Add (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.PropertyAccessorParameterInLinqExpressionsCannotBeStaticallyDetermined));

return diagDescriptorsArrayBuilder.ToImmutable ();

void AddRange (DiagnosticId first, DiagnosticId last)
Expand Down Expand Up @@ -103,7 +107,12 @@ public override void Initialize (AnalysisContext context)
}, SyntaxKind.GenericName);
context.RegisterSymbolAction (context => {
VerifyMemberOnlyApplyToTypesOrStrings (context, context.Symbol);
VerifyDamOnPropertyAndAccessorMatch (context, (IMethodSymbol) context.Symbol);
VerifyDamOnDerivedAndBaseMethodsMatch (context, (IMethodSymbol) context.Symbol);
}, SymbolKind.Method);
context.RegisterSymbolAction (context => {
VerifyDamOnInterfaceAndImplementationMethodsMatch (context, (INamedTypeSymbol) context.Symbol);
}, SymbolKind.NamedType);
context.RegisterSymbolAction (context => {
VerifyMemberOnlyApplyToTypesOrStrings (context, context.Symbol);
}, SymbolKind.Property);
Expand Down Expand Up @@ -209,5 +218,75 @@ static void VerifyMemberOnlyApplyToTypesOrStrings (SymbolAnalysisContext context
context.ReportDiagnostic (Diagnostic.Create (DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersOnPropertyCanOnlyApplyToTypesOrStrings), member.Locations[0], member.GetDisplayName ()));
}
}

static void VerifyDamOnDerivedAndBaseMethodsMatch (SymbolAnalysisContext context, IMethodSymbol methodSymbol)
{
if (methodSymbol.TryGetOverriddenMember (out var overriddenSymbol) && overriddenSymbol is IMethodSymbol overriddenMethod
&& context.Symbol is IMethodSymbol method) {
VerifyDamOnMethodsMatch (context, method, overriddenMethod);
}
}

static void VerifyDamOnMethodsMatch (SymbolAnalysisContext context, IMethodSymbol method, IMethodSymbol overriddenMethod)
{
if (FlowAnnotations.GetMethodReturnValueAnnotation (method) != FlowAnnotations.GetMethodReturnValueAnnotation (overriddenMethod))
context.ReportDiagnostic (Diagnostic.Create (
DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersMismatchOnMethodReturnValueBetweenOverrides),
method.Locations[0], method.GetDisplayName (), overriddenMethod.GetDisplayName ()));

for (int i = 0; i < method.Parameters.Length; i++) {
if (FlowAnnotations.GetMethodParameterAnnotation (method.Parameters[i]) != FlowAnnotations.GetMethodParameterAnnotation (overriddenMethod.Parameters[i]))
context.ReportDiagnostic (Diagnostic.Create (
DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersMismatchOnMethodParameterBetweenOverrides),
method.Parameters[i].Locations[0],
method.Parameters[i].GetDisplayName (), method.GetDisplayName (), overriddenMethod.Parameters[i].GetDisplayName (), overriddenMethod.GetDisplayName ()));
}

for (int i = 0; i < method.TypeParameters.Length; i++) {
if (method.TypeParameters[i].GetDynamicallyAccessedMemberTypes () != overriddenMethod.TypeParameters[i].GetDynamicallyAccessedMemberTypes ())
context.ReportDiagnostic (Diagnostic.Create (
DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersMismatchOnGenericParameterBetweenOverrides),
method.TypeParameters[i].Locations[0],
method.TypeParameters[i].GetDisplayName (), method.GetDisplayName (),
overriddenMethod.TypeParameters[i].GetDisplayName (), overriddenMethod.GetDisplayName ()));
}

if (!method.IsStatic && method.GetDynamicallyAccessedMemberTypes () != overriddenMethod.GetDynamicallyAccessedMemberTypes ())
context.ReportDiagnostic (Diagnostic.Create (
DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersMismatchOnImplicitThisBetweenOverrides),
method.Locations[0],
method.GetDisplayName (), overriddenMethod.GetDisplayName ()));
}

static void VerifyDamOnInterfaceAndImplementationMethodsMatch (SymbolAnalysisContext context, INamedTypeSymbol type)
{
foreach (var (interfaceMember, implementationMember) in type.GetMemberInterfaceImplementationPairs ()) {
if (implementationMember is IMethodSymbol implementationMethod
&& interfaceMember is IMethodSymbol interfaceMethod)
VerifyDamOnMethodsMatch (context, implementationMethod, interfaceMethod);
}
}

static void VerifyDamOnPropertyAndAccessorMatch (SymbolAnalysisContext context, IMethodSymbol methodSymbol)
{
if ((methodSymbol.MethodKind != MethodKind.PropertyGet && methodSymbol.MethodKind != MethodKind.PropertySet)
|| (methodSymbol.AssociatedSymbol?.GetDynamicallyAccessedMemberTypes () == DynamicallyAccessedMemberTypes.None))
return;

// None on the return type of 'get' matches unannotated
if (methodSymbol.MethodKind == MethodKind.PropertyGet
&& methodSymbol.GetDynamicallyAccessedMemberTypesOnReturnType () != DynamicallyAccessedMemberTypes.None
// None on parameter of 'set' matches unannotated
|| methodSymbol.MethodKind == MethodKind.PropertySet
&& methodSymbol.Parameters[0].GetDynamicallyAccessedMemberTypes () != DynamicallyAccessedMemberTypes.None) {
context.ReportDiagnostic (Diagnostic.Create (
DiagnosticDescriptors.GetDiagnosticDescriptor (DiagnosticId.DynamicallyAccessedMembersConflictsBetweenPropertyAndAccessor),
methodSymbol.AssociatedSymbol!.Locations[0],
methodSymbol.AssociatedSymbol!.GetDisplayName (),
methodSymbol.GetDisplayName ()
));
return;
}
}
}
}
26 changes: 26 additions & 0 deletions src/ILLink.RoslynAnalyzer/INamedTypeSymbolExtensions.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Generic;
using Microsoft.CodeAnalysis;

namespace ILLink.RoslynAnalyzer
Expand All @@ -28,5 +29,30 @@ internal static bool HasName (this INamedTypeSymbol type, string typeName)

return true;
}

internal static IEnumerable<(ISymbol InterfaceMember, ISymbol ImplementationMember)> GetMemberInterfaceImplementationPairs (this INamedTypeSymbol namedType)
{
var interfaces = namedType.Interfaces;
foreach (INamedTypeSymbol iface in interfaces) {
foreach (var pair in GetMatchingMembers (namedType, iface)) {
yield return pair;
}
}
}

private static IEnumerable<(ISymbol InterfaceMember, ISymbol ImplementationMember)> GetMatchingMembers (INamedTypeSymbol implementationSymbol, INamedTypeSymbol interfaceSymbol)
{
var members = interfaceSymbol.GetMembers ();
foreach (ISymbol interfaceMember in members) {
if (implementationSymbol.FindImplementationForInterfaceMember (interfaceMember) is ISymbol implementationMember) {
yield return (InterfaceMember: interfaceMember, ImplementationMember: implementationMember);
}
}
foreach (var iface in interfaceSymbol.Interfaces) {
foreach (var pair in GetMatchingMembers (implementationSymbol, iface)) {
yield return pair;
}
}
}
}
}
12 changes: 3 additions & 9 deletions src/ILLink.RoslynAnalyzer/RequiresAnalyzerBase.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -263,15 +263,9 @@ void CheckMatchingAttributesInInterfaces (
SymbolAnalysisContext symbolAnalysisContext,
INamedTypeSymbol type)
{
ImmutableArray<INamedTypeSymbol> interfaces = type.Interfaces;
foreach (INamespaceOrTypeSymbol iface in interfaces) {
var members = iface.GetMembers ();
foreach (var member in members) {
var implementation = type.FindImplementationForInterfaceMember (member);
// In case the implementation is null because the user code is missing an implementation, we dont provide diagnostics.
// The compiler will provide an error
if (implementation != null && HasMismatchingAttributes (member, implementation))
ReportMismatchInAttributesDiagnostic (symbolAnalysisContext, implementation, member, isInterface: true);
foreach (var memberpair in type.GetMemberInterfaceImplementationPairs ()) {
if (HasMismatchingAttributes (memberpair.InterfaceMember, memberpair.ImplementationMember)) {
ReportMismatchInAttributesDiagnostic (symbolAnalysisContext, memberpair.ImplementationMember, memberpair.InterfaceMember, isInterface: true);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion test/ILLink.RoslynAnalyzer.Tests/DataFlowTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ public Task UnresolvedMembers ()
return RunTest (allowMissingWarnings: true);
}

[Fact (Skip = "/~https://github.com/dotnet/linker/issues/2273")]
[Fact]
public Task VirtualMethodHierarchyDataflowAnnotationValidation ()
{
return RunTest (nameof (VirtualMethodHierarchyDataflowAnnotationValidation));
Expand Down
42 changes: 32 additions & 10 deletions test/ILLink.RoslynAnalyzer.Tests/DynamicallyAccessedMembersAnalyzerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -511,6 +511,7 @@ static string GetSystemTypeBase ()
using System.Globalization;
using System.Reflection;
using System.Diagnostics.CodeAnalysis;
namespace System
{
Expand All @@ -534,6 +535,8 @@ public class TestSystemTypeBase : Type
public override string Name => throw new NotImplementedException ();
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors
| DynamicallyAccessedMemberTypes.NonPublicConstructors)]
public override ConstructorInfo[] GetConstructors (BindingFlags bindingAttr)
{
throw new NotImplementedException ();
Expand All @@ -554,61 +557,76 @@ public override Type GetElementType ()
throw new NotImplementedException ();
}
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicEvents
| DynamicallyAccessedMemberTypes.NonPublicEvents)]
public override EventInfo GetEvent (string name, BindingFlags bindingAttr)
{
throw new NotImplementedException ();
}
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicEvents | DynamicallyAccessedMemberTypes.NonPublicEvents)]
public override EventInfo[] GetEvents (BindingFlags bindingAttr)
{
throw new NotImplementedException ();
}
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields | DynamicallyAccessedMemberTypes.NonPublicFields)]
public override FieldInfo GetField (string name, BindingFlags bindingAttr)
{
throw new NotImplementedException ();
}
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicFields
| DynamicallyAccessedMemberTypes.NonPublicFields)]
public override FieldInfo[] GetFields (BindingFlags bindingAttr)
{
throw new NotImplementedException ();
}
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.Interfaces)]
[return: DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.Interfaces)]
public override Type GetInterface (string name, bool ignoreCase)
{
throw new NotImplementedException ();
}
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.Interfaces)]
public override Type[] GetInterfaces ()
{
throw new NotImplementedException ();
}
[DynamicallyAccessedMembers((DynamicallyAccessedMemberTypes)0x1FFF)]
public override MemberInfo[] GetMembers (BindingFlags bindingAttr)
{
throw new NotImplementedException ();
}
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.NonPublicMethods)]
public override MethodInfo[] GetMethods (BindingFlags bindingAttr)
{
throw new NotImplementedException ();
}
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicNestedTypes | DynamicallyAccessedMemberTypes.NonPublicNestedTypes)]
public override Type GetNestedType (string name, BindingFlags bindingAttr)
{
throw new NotImplementedException ();
}
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicNestedTypes | DynamicallyAccessedMemberTypes.NonPublicNestedTypes)]
public override Type[] GetNestedTypes (BindingFlags bindingAttr)
{
throw new NotImplementedException ();
}
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.NonPublicProperties)]
public override PropertyInfo[] GetProperties (BindingFlags bindingAttr)
{
throw new NotImplementedException ();
}
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.All)]
public override object InvokeMember (string name, BindingFlags invokeAttr, Binder binder, object target, object[] args, ParameterModifier[] modifiers, CultureInfo culture, string[] namedParameters)
{
throw new NotImplementedException ();
Expand All @@ -624,16 +642,20 @@ protected override TypeAttributes GetAttributeFlagsImpl ()
throw new NotImplementedException ();
}
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicConstructors
| DynamicallyAccessedMemberTypes.NonPublicConstructors)]
protected override ConstructorInfo GetConstructorImpl (BindingFlags bindingAttr, Binder binder, CallingConventions callConvention, Type[] types, ParameterModifier[] modifiers)
{
throw new NotImplementedException ();
}
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicMethods | DynamicallyAccessedMemberTypes.NonPublicMethods)]
protected override MethodInfo GetMethodImpl (string name, BindingFlags bindingAttr, Binder binder, CallingConventions callConvention, Type[] types, ParameterModifier[] modifiers)
{
throw new NotImplementedException ();
}
[DynamicallyAccessedMembers(DynamicallyAccessedMemberTypes.PublicProperties | DynamicallyAccessedMemberTypes.NonPublicProperties)]
protected override PropertyInfo GetPropertyImpl (string name, BindingFlags bindingAttr, Binder binder, Type returnType, Type[] types, ParameterModifier[] modifiers)
{
throw new NotImplementedException ();
Expand Down Expand Up @@ -698,12 +720,12 @@ private static void M2(
}
}";

// (178,16): warning IL2082: 'type' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods' in call to 'System.C.M2(Type)'.
// (200,16): warning IL2082: 'type' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods' in call to 'System.C.M2(Type)'.
// The implicit 'this' argument of method 'System.C.M1()' 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.
return VerifyDynamicallyAccessedMembersAnalyzer (string.Concat (GetSystemTypeBase (), TargetParameterWithAnnotations),
VerifyCS.Diagnostic (DiagnosticId.DynamicallyAccessedMembersMismatchThisParameterTargetsParameter)
.WithSpan (178, 13, 178, 21)
.WithSpan (200, 13, 200, 21)
.WithArguments ("type", "System.C.M2(Type)", "System.C.M1()", "'DynamicallyAccessedMemberTypes.PublicMethods'"));
}

Expand Down Expand Up @@ -740,7 +762,7 @@ private static void M2(

return VerifyDynamicallyAccessedMembersAnalyzer (string.Concat (GetSystemTypeBase (), ConversionOperation),
VerifyCS.Diagnostic (DiagnosticId.DynamicallyAccessedMembersMismatchMethodReturnTypeTargetsParameter)
.WithSpan (183, 13, 183, 37)
.WithSpan (205, 13, 205, 37)
.WithArguments ("type", "System.C.M2(Type)", "System.ConvertsToType.implicit operator Type(ConvertsToType)", "'DynamicallyAccessedMemberTypes.PublicMethods'"));
}

Expand Down Expand Up @@ -780,7 +802,7 @@ private static void M2(

return VerifyDynamicallyAccessedMembersAnalyzer (string.Concat (GetSystemTypeBase (), AnnotatedConversionOperation),
VerifyCS.Diagnostic (DiagnosticId.DynamicallyAccessedMembersMismatchMethodReturnTypeTargetsParameter)
.WithSpan (185, 13, 185, 37)
.WithSpan (207, 13, 207, 37)
.WithArguments ("type", "System.C.M2(Type)", "System.ConvertsToType.implicit operator Type(ConvertsToType)", "'DynamicallyAccessedMemberTypes.PublicMethods'"));
}

Expand Down Expand Up @@ -843,12 +865,12 @@ private Type M()
}
}";

// (180,13): warning IL2083: 'System.C.M()' method return value does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods' requirements.
// (202,13): warning IL2083: 'System.C.M()' method return value does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods' requirements.
// The implicit 'this' argument of method 'System.C.M()' 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.
return VerifyDynamicallyAccessedMembersAnalyzer (string.Concat (GetSystemTypeBase (), TargetMethodReturnTypeWithAnnotations),
VerifyCS.Diagnostic (DiagnosticId.DynamicallyAccessedMembersMismatchThisParameterTargetsMethodReturnType)
.WithSpan (180, 20, 180, 24)
.WithSpan (202, 20, 202, 24)
.WithArguments ("System.C.M()", "System.C.M()", "'DynamicallyAccessedMemberTypes.PublicMethods'"));
}

Expand Down Expand Up @@ -876,12 +898,12 @@ private void M()
}
}";

// (178,13): warning IL2084: value stored in field 'System.C.f' does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods' requirements.
// (200,13): warning IL2084: value stored in field 'System.C.f' does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods' requirements.
// The implicit 'this' argument of method 'System.C.M()' 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.
return VerifyDynamicallyAccessedMembersAnalyzer (string.Concat (GetSystemTypeBase (), TargetFieldWithAnnotations),
VerifyCS.Diagnostic (DiagnosticId.DynamicallyAccessedMembersMismatchThisParameterTargetsField)
.WithSpan (178, 13, 178, 21)
.WithSpan (200, 13, 200, 21)
.WithArguments ("System.C.f",
"System.C.M()",
"'DynamicallyAccessedMemberTypes.PublicMethods'"));
Expand All @@ -907,12 +929,12 @@ private void M()
}
}";

// (178,13): warning IL2085: 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods' in call to 'System.Type.GetMethods()'.
// (200,13): warning IL2085: 'this' argument does not satisfy 'DynamicallyAccessedMemberTypes.PublicMethods' in call to 'System.Type.GetMethods()'.
// The implicit 'this' argument of method 'System.C.M()' 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.
return VerifyDynamicallyAccessedMembersAnalyzer (string.Concat (GetSystemTypeBase (), TargetMethodWithAnnotations),
VerifyCS.Diagnostic (DiagnosticId.DynamicallyAccessedMembersMismatchThisParameterTargetsThisParameter)
.WithSpan (178, 13, 178, 30)
.WithSpan (200, 13, 200, 30)
.WithArguments ("System.Type.GetMethods()", "System.C.M()", "'DynamicallyAccessedMemberTypes.PublicMethods'"));
}
#endregion
Expand Down
Loading

0 comments on commit ac6cfb3

Please sign in to comment.