Merge pull request #3415 from Evangelink/use-eventhandler-helper

Improve and use HasEventHandlerSignature helper

src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/UseGenericEventHandlerInstances.cs

@@ -1,8 +1,6 @@
 // Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
 
-using System;
 using System.Collections.Immutable;
-using System.Diagnostics.CodeAnalysis;
 using System.Linq;
 using Analyzer.Utilities;
 using Analyzer.Utilities.Extensions;
@@ -22,7 +20,8 @@ namespace Microsoft.CodeQuality.Analyzers.ApiDesignGuidelines
     ///       This rule recommends fixing the signature to use a valid non-generic event handler.
     ///       We do not report CA1009, but instead report CA1003 and recommend using a generic event handler.
     /// </remarks>
-    public abstract class UseGenericEventHandlerInstancesAnalyzer : DiagnosticAnalyzer
+    [DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)]
+    public sealed class UseGenericEventHandlerInstancesAnalyzer : DiagnosticAnalyzer
     {
         internal const string RuleId = "CA1003";
         private static readonly LocalizableString s_localizableTitle = new LocalizableResourceString(nameof(MicrosoftCodeQualityAnalyzersResources.UseGenericEventHandlerInstancesTitle), MicrosoftCodeQualityAnalyzersResources.ResourceManager, typeof(MicrosoftCodeQualityAnalyzersResources));
@@ -67,10 +66,6 @@ public abstract class UseGenericEventHandlerInstancesAnalyzer : DiagnosticAnalyz
             isEnabledByDefaultInFxCopAnalyzers: false);
 
         public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(RuleForDelegates, RuleForEvents, RuleForEvents2);
-        protected abstract bool IsAssignableTo(
-            [NotNullWhen(returnValue: true)] ITypeSymbol? fromSymbol,
-            [NotNullWhen(returnValue: true)] ITypeSymbol? toSymbol,
-            Compilation compilation);
 
         public override void Initialize(AnalysisContext analysisContext)
         {
@@ -95,39 +90,14 @@ public override void Initialize(AnalysisContext analysisContext)
                     static bool IsDelegateTypeWithInvokeMethod(INamedTypeSymbol namedType) =>
                         namedType.TypeKind == TypeKind.Delegate && namedType.DelegateInvokeMethod != null;
 
-                    bool IsEventArgsParameter(IParameterSymbol parameter)
-                    {
-                        var type = parameter.Type;
-                        if (IsAssignableTo(type, eventArgs, context.Compilation))
-                        {
-                            return true;
-                        }
-
-                        // FxCop compat: Struct with name ending with "EventArgs" are allowed.
-                        if (type.IsValueType)
-                        {
-                            return type.Name.EndsWith("EventArgs", StringComparison.Ordinal);
-                        }
-
-                        return false;
-                    }
-
-                    bool IsValidNonGenericEventHandler(IMethodSymbol delegateInvokeMethod)
-                    {
-                        return delegateInvokeMethod.ReturnsVoid &&
-                            delegateInvokeMethod.Parameters.Length == 2 &&
-                            delegateInvokeMethod.Parameters[0].Type.SpecialType == SpecialType.System_Object &&
-                            IsEventArgsParameter(delegateInvokeMethod.Parameters[1]);
-                    }
-
                     context.RegisterSymbolAction(symbolContext =>
                     {
                         // Note all the descriptors/rules for this analyzer have the same ID and category and hence
                         // will always have identical configured visibility.
                         var namedType = (INamedTypeSymbol)symbolContext.Symbol;
                         if (namedType.MatchesConfiguredVisibility(symbolContext.Options, RuleForDelegates, symbolContext.Compilation, symbolContext.CancellationToken) &&
                             IsDelegateTypeWithInvokeMethod(namedType) &&
-                            IsValidNonGenericEventHandler(namedType.DelegateInvokeMethod))
+                            namedType.DelegateInvokeMethod.HasEventHandlerSignature(eventArgs))
                         {
                             // CA1003: Remove '{0}' and replace its usage with a generic EventHandler, for e.g. EventHandler&lt;T&gt;, where T is a valid EventArgs
                             symbolContext.ReportDiagnostic(namedType.CreateDiagnostic(RuleForDelegates, namedType.Name));
@@ -159,7 +129,7 @@ eventSymbol.Type is INamedTypeSymbol eventType &&
                                 // CA1003: Change the event '{0}' to use a generic EventHandler by defining the event type explicitly, for e.g. Event MyEvent As EventHandler(Of MyEventArgs).
                                 symbolContext.ReportDiagnostic(eventSymbol.CreateDiagnostic(RuleForEvents2, eventSymbol.Name));
                             }
-                            else if (!IsValidNonGenericEventHandler(eventType.DelegateInvokeMethod))
+                            else if (!eventType.DelegateInvokeMethod.HasEventHandlerSignature(eventArgs))
                             {
                                 // CA1003: Change the event '{0}' to replace the type '{1}' with a generic EventHandler, for e.g. EventHandler&lt;T&gt;, where T is a valid EventArgs
                                 symbolContext.ReportDiagnostic(eventSymbol.CreateDiagnostic(RuleForEvents, eventSymbol.Name, eventType.ToDisplayString()));

src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/Maintainability/ReviewUnusedParameters.cs

@@ -204,10 +204,7 @@ private static bool ShouldAnalyzeMethod(
             }
 
             // Ignore event handler methods "Handler(object, MyEventArgs)"
-            if (method.Parameters.Length == 2 &&
-                method.Parameters[0].Type.SpecialType == SpecialType.System_Object &&
-                // UWP has specific EventArgs not inheriting from System.EventArgs. It was decided to go for a suffix match rather than a whitelist.
-                (method.Parameters[1].Type.Inherits(eventsArgSymbol) || method.Parameters[1].Type.Name.EndsWith("EventArgs", StringComparison.Ordinal)))
+            if (method.HasEventHandlerSignature(eventsArgSymbol))
             {
                 return false;
             }

src/NetAnalyzers/Core/Microsoft.CodeQuality.Analyzers/QualityGuidelines/MarkMembersAsStatic.cs

@@ -1,6 +1,5 @@
 // Copyright (c) Microsoft.  All Rights Reserved.  Licensed under the Apache License, Version 2.0.  See License.txt in the project root for license information.
 
-using System;
 using System.Collections.Immutable;
 using System.Linq;
 using Analyzer.Utilities;
@@ -204,9 +203,7 @@ private static bool ShouldAnalyze(IMethodSymbol methodSymbol, WellKnownTypeProvi
             // If this looks like an event handler don't flag such cases.
             // However, we do want to consider EventRaise accessor as a candidate
             // so we can flag the associated event if none of it's accessors need instance reference.
-            if (methodSymbol.Parameters.Length == 2 &&
-                methodSymbol.Parameters[0].Type.SpecialType == SpecialType.System_Object &&
-                IsEventArgs(methodSymbol.Parameters[1].Type, wellKnownTypeProvider) &&
+            if (methodSymbol.HasEventHandlerSignature(wellKnownTypeProvider.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemEventArgs)) &&
                 methodSymbol.MethodKind != MethodKind.EventRaise)
             {
                 return false;
@@ -220,21 +217,6 @@ private static bool ShouldAnalyze(IMethodSymbol methodSymbol, WellKnownTypeProvi
             return true;
         }
 
-        private static bool IsEventArgs(ITypeSymbol type, WellKnownTypeProvider wellKnownTypeProvider)
-        {
-            if (type.DerivesFrom(wellKnownTypeProvider.GetOrCreateTypeByMetadataName(WellKnownTypeNames.SystemEventArgs)))
-            {
-                return true;
-            }
-
-            if (type.IsValueType)
-            {
-                return type.Name.EndsWith("EventArgs", StringComparison.Ordinal);
-            }
-
-            return false;
-        }
-
         private static bool IsExplicitlyVisibleFromCom(IMethodSymbol methodSymbol, WellKnownTypeProvider wellKnownTypeProvider)
         {
             if (!methodSymbol.IsExternallyVisible() || methodSymbol.IsGenericMethod)

src/NetAnalyzers/Core/Microsoft.NetCore.Analyzers/Security/SetViewStateUserKey.cs

@@ -71,10 +71,7 @@ public override void Initialize(AnalysisContext context)
                                                                                                                     s.IsProtected() &&
                                                                                                                     !s.IsStatic));
                         var setViewStateUserKeyInPage_Init = SetViewStateUserKeyCorrectly(methods.FirstOrDefault(s => s.Name == "Page_Init" &&
-                                                                                                                        s.Parameters.Length == 2 &&
-                                                                                                                        s.Parameters[0].Type.SpecialType == SpecialType.System_Object &&
-                                                                                                                        s.Parameters[1].Type.Equals(eventArgsTypeSymbol) &&
-                                                                                                                        s.ReturnType.SpecialType == SpecialType.System_Void));
+                                                                                                                        s.HasEventHandlerSignature(eventArgsTypeSymbol)));
 
                         if (setViewStateUserKeyInOnInit || setViewStateUserKeyInPage_Init)
                         {

src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/IdentifiersShouldHaveCorrectSuffixTests.cs

@@ -1864,6 +1864,32 @@ public interface I
 }");
         }
 
+        [Fact]
+        public async Task EventArgsNotInheritingFromSystemEventArgs_Diagnostic()
+        {
+            await VerifyCS.VerifyAnalyzerAsync(@"
+// Reproduce UWP specific EventArgs
+namespace Windows.UI.Xaml
+{
+    public class RoutedEventArgs {}
+}
+public class C
+{
+    public delegate void Page_Loaded(object sender, Windows.UI.Xaml.RoutedEventArgs e);
+}");
+
+            await VerifyVB.VerifyAnalyzerAsync(@"
+' Reproduce UWP specific EventArgs
+Namespace Windows.UI.Xaml
+    Public Class RoutedEventArgs
+    End Class
+End Namespace
+
+Public Class C
+    Public Delegate Sub Page_Loaded(ByVal sender As Object, ByVal e As Windows.UI.Xaml.RoutedEventArgs)
+End Class");
+        }
+
         private static DiagnosticResult GetCA1710BasicResultAt(int line, int column, string typeName, string suffix, bool isSpecial = false) =>
             VerifyVB.Diagnostic(isSpecial ? IdentifiersShouldHaveCorrectSuffixAnalyzer.SpecialCollectionRule : IdentifiersShouldHaveCorrectSuffixAnalyzer.DefaultRule)
                 .WithLocation(line, column)

src/NetAnalyzers/UnitTests/Microsoft.CodeQuality.Analyzers/ApiDesignGuidelines/UseGenericEventHandlerInstancesTests.cs

@@ -5,11 +5,11 @@
 using Microsoft.CodeAnalysis.Testing;
 using Xunit;
 using VerifyCS = Test.Utilities.CSharpCodeFixVerifier<
-    Microsoft.CodeQuality.CSharp.Analyzers.ApiDesignGuidelines.CSharpUseGenericEventHandlerInstancesAnalyzer,
-    Microsoft.CodeQuality.CSharp.Analyzers.ApiDesignGuidelines.CSharpUseGenericEventHandlerInstancesFixer>;
+    Microsoft.CodeQuality.Analyzers.ApiDesignGuidelines.UseGenericEventHandlerInstancesAnalyzer,
+    Microsoft.CodeAnalysis.Testing.EmptyCodeFixProvider>;
 using VerifyVB = Test.Utilities.VisualBasicCodeFixVerifier<
-    Microsoft.CodeQuality.VisualBasic.Analyzers.ApiDesignGuidelines.BasicUseGenericEventHandlerInstancesAnalyzer,
-    Microsoft.CodeQuality.VisualBasic.Analyzers.ApiDesignGuidelines.BasicUseGenericEventHandlerInstancesFixer>;
+    Microsoft.CodeQuality.Analyzers.ApiDesignGuidelines.UseGenericEventHandlerInstancesAnalyzer,
+    Microsoft.CodeAnalysis.Testing.EmptyCodeFixProvider>;
 
 namespace Microsoft.CodeQuality.Analyzers.ApiDesignGuidelines.UnitTests
 {

src/Utilities/Compiler/Extensions/IMethodSymbolExtensions.cs

@@ -567,7 +567,10 @@ public static bool HasEventHandlerSignature(this IMethodSymbol method, [NotNullW
             => eventArgsType != null &&
                method.Parameters.Length == 2 &&
                method.Parameters[0].Type.SpecialType == SpecialType.System_Object &&
-               method.Parameters[1].Type.DerivesFrom(eventArgsType, baseTypesOnly: true);
+               // FxCop compat: Struct with name ending with "EventArgs" are allowed
+               // + UWP has specific EventArgs not inheriting from 'System.EventArgs'.
+               // See https://github.com/dotnet/roslyn-analyzers/issues/3106
+               (method.Parameters[1].Type.DerivesFrom(eventArgsType, baseTypesOnly: true) || method.Parameters[1].Type.Name.EndsWith("EventArgs", StringComparison.Ordinal));
 
         public static bool IsLockMethod(this IMethodSymbol method, [NotNullWhen(returnValue: true)] INamedTypeSymbol? systemThreadingMonitor)
         {