From e56bd1459c5c90d3c51e66b81fa7e9133fb7a3df Mon Sep 17 00:00:00 2001 From: Sebastien Marichal Date: Mon, 24 Jun 2024 15:53:22 +0200 Subject: [PATCH] Fix S1144 FP: Event with a concrete sender --- .../Rules/UnusedPrivateMember.cs | 36 +++----- .../Helpers/KnownMethods.cs | 39 ++++---- .../Rules/UnusedPrivateMemberTest.cs | 88 ++++++++++--------- .../UnusedPrivateMember.Fixed.Batch.cs | 38 ++++++++ .../TestCases/UnusedPrivateMember.Fixed.cs | 38 ++++++++ .../TestCases/UnusedPrivateMember.cs | 39 +++++++- 6 files changed, 189 insertions(+), 89 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs index 56248a87bba..d6866fa7fd8 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs @@ -195,11 +195,10 @@ private static HashSet GetUnusedSymbols(CSharpSymbolUsageCollector usag .ToHashSet(); private static bool IsAccessorUsed(ISymbol symbol, CSharpSymbolUsageCollector usageCollector) => - symbol is IMethodSymbol { } accessor - && accessor.AssociatedSymbol is IPropertySymbol { } property - && usageCollector.PropertyAccess.TryGetValue(property, out var access) - && ((access.HasFlag(AccessorAccess.Get) && accessor.MethodKind == MethodKind.PropertyGet) - || (access.HasFlag(AccessorAccess.Set) && accessor.MethodKind == MethodKind.PropertySet)); + symbol is IMethodSymbol { AssociatedSymbol: IPropertySymbol property } accessor + && usageCollector.PropertyAccess.TryGetValue(property, out var access) + && ((access.HasFlag(AccessorAccess.Get) && accessor.MethodKind == MethodKind.PropertyGet) + || (access.HasFlag(AccessorAccess.Set) && accessor.MethodKind == MethodKind.PropertySet)); private static string GetFieldAccessibilityForMessage(ISymbol symbol) => symbol.DeclaredAccessibility == Accessibility.Private ? SyntaxConstants.Private : "private class"; @@ -274,6 +273,7 @@ private static void ReportProperty(SonarCompilationReportingContextBas { context.ReportIssue(RuleS1144, getter.Keyword.GetLocation(), SyntaxConstants.Private, "get accessor in property", property.Name); } + static AccessorDeclarationSyntax GetAccessorSyntax(ISymbol symbol) => symbol?.DeclaringSyntaxReferences.FirstOrDefault()?.GetSyntax() as AccessorDeclarationSyntax; } @@ -487,8 +487,8 @@ private void ConditionalStore(TSymbol symbol, Func condi } } - private ISymbol GetDeclaredSymbol(SyntaxNode syntaxNode) => - getSemanticModel(syntaxNode).GetDeclaredSymbol(syntaxNode); + private ISymbol GetDeclaredSymbol(SyntaxNode node) => + getSemanticModel(node).GetDeclaredSymbol(node); private void StoreRemovableVariableDeclarations(BaseFieldDeclarationSyntax node) { @@ -505,26 +505,13 @@ private void StoreRemovableVariableDeclarations(BaseFieldDeclarationSyntax node) private static bool IsEmptyConstructor(BaseMethodDeclarationSyntax constructorDeclaration) => !constructorDeclaration.HasBodyOrExpressionBody() - || (constructorDeclaration.Body is { } && constructorDeclaration.Body.Statements.Count == 0); - - private static bool IsDeclaredInPartialClass(ISymbol methodSymbol) - { - return methodSymbol.DeclaringSyntaxReferences - .Select(GetContainingTypeDeclaration) - .Any(IsPartial); - - static TypeDeclarationSyntax GetContainingTypeDeclaration(SyntaxReference syntaxReference) => - syntaxReference.GetSyntax().FirstAncestorOrSelf(); - - static bool IsPartial(TypeDeclarationSyntax typeDeclaration) => - typeDeclaration.Modifiers.AnyOfKind(SyntaxKind.PartialKeyword); - } + || constructorDeclaration.Body is { Statements.Count: 0 }; private static bool IsRemovableMethod(IMethodSymbol methodSymbol) => IsRemovableMember(methodSymbol) && (methodSymbol.MethodKind is MethodKind.Ordinary or MethodKind.Constructor or MethodKindEx.LocalFunction) && !methodSymbol.IsMainMethod() - && (!methodSymbol.IsEventHandler() || !IsDeclaredInPartialClass(methodSymbol)) // Event handlers could be added in XAML and no method reference will be generated in the .g.cs file. + && !methodSymbol.IsEventHandler() // Event handlers could be added in XAML and no method reference will be generated in the .g.cs file. && !methodSymbol.IsSerializationConstructor() && !methodSymbol.IsRecordPrintMembers(); @@ -540,8 +527,7 @@ private static bool IsRemovable(ISymbol symbol) => private static bool HasAttributes(ISymbol symbol) { IEnumerable attributes = symbol.GetAttributes(); - if (symbol is IMethodSymbol { MethodKind: MethodKind.PropertyGet or MethodKind.PropertySet } method - && method.AssociatedSymbol is { } property) + if (symbol is IMethodSymbol { MethodKind: MethodKind.PropertyGet or MethodKind.PropertySet, AssociatedSymbol: { } property }) { attributes = attributes.Union(property.GetAttributes()); } @@ -554,7 +540,7 @@ private static bool IsRemovableMember(ISymbol symbol) => private static bool IsRemovableType(ISymbol typeSymbol) => typeSymbol.GetEffectiveAccessibility() is var accessibility - && typeSymbol.ContainingType is { } + && typeSymbol.ContainingType is not null && (accessibility is Accessibility.Private or Accessibility.Internal) && IsRemovable(typeSymbol); diff --git a/analyzers/src/SonarAnalyzer.Common/Helpers/KnownMethods.cs b/analyzers/src/SonarAnalyzer.Common/Helpers/KnownMethods.cs index 289d6885915..f81c9d3b674 100644 --- a/analyzers/src/SonarAnalyzer.Common/Helpers/KnownMethods.cs +++ b/analyzers/src/SonarAnalyzer.Common/Helpers/KnownMethods.cs @@ -28,7 +28,7 @@ public static bool IsMainMethod(this IMethodSymbol methodSymbol) { // Based on Microsoft definition: https://msdn.microsoft.com/en-us/library/1y814bzs.aspx // Adding support for new async main: https://blogs.msdn.microsoft.com/mazhou/2017/05/30/c-7-series-part-2-async-main - return methodSymbol != null + return methodSymbol is not null && methodSymbol.IsStatic && methodSymbol.Name.Equals("Main", StringComparison.OrdinalIgnoreCase) // VB.NET is case insensitive && HasMainParameters() @@ -46,7 +46,7 @@ bool HasMainReturnType() => } public static bool IsObjectEquals(this IMethodSymbol methodSymbol) => - methodSymbol != null + methodSymbol is not null && methodSymbol.MethodKind == MethodKind.Ordinary && methodSymbol.Name == nameof(object.Equals) && (methodSymbol.IsOverride || methodSymbol.IsInType(KnownType.System_Object)) @@ -56,7 +56,7 @@ public static bool IsObjectEquals(this IMethodSymbol methodSymbol) => public static bool IsStaticObjectEquals(this IMethodSymbol methodSymbol) { - return methodSymbol != null + return methodSymbol is not null && !methodSymbol.IsOverride && methodSymbol.IsStatic && methodSymbol.MethodKind == MethodKind.Ordinary @@ -72,17 +72,17 @@ bool HasCorrectParameters() => } public static bool IsObjectGetHashCode(this IMethodSymbol methodSymbol) => - methodSymbol != null + methodSymbol is not null && methodSymbol.MethodKind == MethodKind.Ordinary - && methodSymbol.Name == nameof(object.GetHashCode) + && methodSymbol.Name == nameof(GetHashCode) && (methodSymbol.IsOverride || methodSymbol.IsInType(KnownType.System_Object)) && methodSymbol.Parameters.Length == 0 && methodSymbol.ReturnType.Is(KnownType.System_Int32); public static bool IsObjectToString(this IMethodSymbol methodSymbol) => - methodSymbol != null + methodSymbol is not null && methodSymbol.MethodKind == MethodKind.Ordinary - && methodSymbol.Name == nameof(object.ToString) + && methodSymbol.Name == nameof(ToString) && (methodSymbol.IsOverride || methodSymbol.IsInType(KnownType.System_Object)) && methodSymbol.Parameters.Length == 0 && methodSymbol.ReturnType.Is(KnownType.System_String); @@ -116,7 +116,7 @@ methodSymbol is public static bool IsIEquatableEquals(this IMethodSymbol methodSymbol) { const string explicitName = "System.IEquatable.Equals"; - return methodSymbol != null + return methodSymbol is not null && (methodSymbol.Name == nameof(object.Equals) || methodSymbol.Name == explicitName) && methodSymbol.Parameters.Length == 1 && methodSymbol.ReturnType.Is(KnownType.System_Boolean); @@ -125,7 +125,7 @@ public static bool IsIEquatableEquals(this IMethodSymbol methodSymbol) public static bool IsGetObjectData(this IMethodSymbol methodSymbol) { const string explicitName = "System.Runtime.Serialization.ISerializable.GetObjectData"; - return methodSymbol != null + return methodSymbol is not null && (methodSymbol.Name == "GetObjectData" || methodSymbol.Name == explicitName) && methodSymbol.Parameters.Length == 2 && methodSymbol.Parameters[0].Type.Is(KnownType.System_Runtime_Serialization_SerializationInfo) @@ -134,14 +134,14 @@ public static bool IsGetObjectData(this IMethodSymbol methodSymbol) } public static bool IsSerializationConstructor(this IMethodSymbol methodSymbol) => - methodSymbol != null + methodSymbol is not null && methodSymbol.MethodKind == MethodKind.Constructor && methodSymbol.Parameters.Length == 2 && methodSymbol.Parameters[0].Type.Is(KnownType.System_Runtime_Serialization_SerializationInfo) && methodSymbol.Parameters[1].Type.Is(KnownType.System_Runtime_Serialization_StreamingContext); public static bool IsArrayClone(this IMethodSymbol methodSymbol) => - methodSymbol != null + methodSymbol is not null && methodSymbol.MethodKind == MethodKind.Ordinary && methodSymbol.Name == nameof(Array.Clone) && methodSymbol.Parameters.Length == 0 @@ -159,18 +159,18 @@ methodSymbol is && methodSymbol.ContainingType.IsRecord(); public static bool IsGcSuppressFinalize(this IMethodSymbol methodSymbol) => - methodSymbol != null + methodSymbol is not null && methodSymbol.Name == nameof(GC.SuppressFinalize) && methodSymbol.Parameters.Length == 1 && methodSymbol.ContainingType.Is(KnownType.System_GC); public static bool IsDebugAssert(this IMethodSymbol methodSymbol) => - methodSymbol != null + methodSymbol is not null && methodSymbol.Name == nameof(Debug.Assert) && methodSymbol.ContainingType.Is(KnownType.System_Diagnostics_Debug); public static bool IsDiagnosticDebugMethod(this IMethodSymbol methodSymbol) => - methodSymbol != null && methodSymbol.ContainingType.Is(KnownType.System_Diagnostics_Debug); + methodSymbol is not null && methodSymbol.ContainingType.Is(KnownType.System_Diagnostics_Debug); public static bool IsOperatorBinaryPlus(this IMethodSymbol methodSymbol) => methodSymbol is { MethodKind: MethodKind.BuiltinOperator or MethodKind.UserDefinedOperator, Name: "op_Addition", Parameters.Length: NumberOfParamsForBinaryOperator }; @@ -194,12 +194,12 @@ public static bool IsOperatorNotEquals(this IMethodSymbol methodSymbol) => methodSymbol is { MethodKind: MethodKind.BuiltinOperator or MethodKind.UserDefinedOperator, Name: "op_Inequality", Parameters.Length: NumberOfParamsForBinaryOperator }; public static bool IsConsoleWriteLine(this IMethodSymbol methodSymbol) => - methodSymbol != null + methodSymbol is not null && methodSymbol.Name == nameof(Console.WriteLine) && methodSymbol.IsInType(KnownType.System_Console); public static bool IsConsoleWrite(this IMethodSymbol methodSymbol) => - methodSymbol != null + methodSymbol is not null && methodSymbol.Name == nameof(Console.Write) && methodSymbol.IsInType(KnownType.System_Console); @@ -228,7 +228,7 @@ public static bool IsEnumerableUnion(this IMethodSymbol methodSymbol) => methodSymbol.IsEnumerableMethod(nameof(Enumerable.Union), 2, 3); public static bool IsListAddRange(this IMethodSymbol methodSymbol) => - methodSymbol != null + methodSymbol is not null && methodSymbol.Name == "AddRange" && methodSymbol.MethodKind == MethodKind.Ordinary && methodSymbol.Parameters.Length == 1 @@ -236,7 +236,6 @@ public static bool IsListAddRange(this IMethodSymbol methodSymbol) => public static bool IsEventHandler(this IMethodSymbol methodSymbol) => methodSymbol is { Parameters.Length: 2 } - && (methodSymbol.Parameters[0].Name.Equals("sender", StringComparison.OrdinalIgnoreCase) || methodSymbol.Parameters[0].Type.Is(KnownType.System_Object)) && (// Inheritance from EventArgs is not enough for UWP or Xamarin as it uses other kind of event args (e.g. ILeavingBackgroundEventArgs) methodSymbol.Parameters[1].Type.Name.EndsWith("EventArgs", StringComparison.Ordinal) || methodSymbol.Parameters[1].Type.DerivesFrom(KnownType.System_EventArgs)) @@ -248,9 +247,9 @@ public static bool IsEventHandler(this IMethodSymbol methodSymbol) => || methodSymbol.ReturnType.Is(KnownType.System_Reflection_Assembly)); private static bool IsEnumerableMethod(this IMethodSymbol methodSymbol, string methodName, params int[] parametersCount) => - methodSymbol != null + methodSymbol is not null && methodSymbol.Name == methodName - && parametersCount.Any(count => methodSymbol.HasExactlyNParameters(count)) + && Array.Exists(parametersCount, methodSymbol.HasExactlyNParameters) && methodSymbol.ContainingType.Is(KnownType.System_Linq_Enumerable); private static bool HasExactlyNParameters(this IMethodSymbol methodSymbol, int parametersCount) => diff --git a/analyzers/tests/SonarAnalyzer.Test/Rules/UnusedPrivateMemberTest.cs b/analyzers/tests/SonarAnalyzer.Test/Rules/UnusedPrivateMemberTest.cs index 09681bfb868..31036dccb07 100644 --- a/analyzers/tests/SonarAnalyzer.Test/Rules/UnusedPrivateMemberTest.cs +++ b/analyzers/tests/SonarAnalyzer.Test/Rules/UnusedPrivateMemberTest.cs @@ -30,24 +30,25 @@ public partial class UnusedPrivateMemberTest [TestMethod] public void UnusedPrivateMember_DebuggerDisplay_Attribute() => - builder.AddSnippet(@" -// https://github.com/SonarSource/sonar-dotnet/issues/1195 -[System.Diagnostics.DebuggerDisplay(""{field1}"", Name = ""{Property1} {Property3}"", Type = ""{Method1()}"")] -public class MethodUsages -{ - private int field1; - private int field2; // Noncompliant - private int Property1 { get; set; } - private int Property2 { get; set; } // Noncompliant - private int Property3 { get; set; } - private int Method1() { return 0; } - private int Method2() { return 0; } // Noncompliant - - public void Method() - { - var x = Property3; - } -}").Verify(); + builder.AddSnippet(""" + // https://github.com/SonarSource/sonar-dotnet/issues/1195 + [System.Diagnostics.DebuggerDisplay("{field1}", Name = "{Property1} {Property3}", Type = "{Method1()}")] + public class MethodUsages + { + private int field1; + private int field2; // Noncompliant + private int Property1 { get; set; } + private int Property2 { get; set; } // Noncompliant + private int Property3 { get; set; } + private int Method1() { return 0; } + private int Method2() { return 0; } // Noncompliant + + public void Method() + { + var x = Property3; + } + } + """).Verify(); [TestMethod] public void UnusedPrivateMember_Members_With_Attributes_Are_Not_Removable() => @@ -92,16 +93,17 @@ public void UnusedPrivateMember_Methods_EventHandler() => // could be added through XAML and no warning will be generated if the // method is removed, which could lead to serious problems that are hard // to diagnose. - builder.AddSnippet(@" -using System; -public class NewClass -{ - private void Handler(object sender, EventArgs e) { } // Noncompliant -} -public partial class PartialClass -{ - private void Handler(object sender, EventArgs e) { } // intentional False Negative -}").Verify(); + builder.AddSnippet(""" + using System; + public class NewClass + { + private void Handler(object sender, EventArgs e) { } // Compliant + } + public partial class PartialClass + { + private void Handler(object sender, EventArgs e) { } // intentional False Negative + } + """).VerifyNoIssues(); [TestMethod] public void UnusedPrivateMember_Unity3D_Ignored() => @@ -208,25 +210,25 @@ public void UnusedPrivateMember_FromCSharp12() => [TestMethod] public void UnusedPrivateMemeber_EntityFramework_DontRaiseOnUnusedEntityPropertiesPrivateSetters() => builder.AddSnippet(""" - // Repro https://github.com/SonarSource/sonar-dotnet/issues/9416 - using Microsoft.EntityFrameworkCore; + // Repro https://github.com/SonarSource/sonar-dotnet/issues/9416 + using Microsoft.EntityFrameworkCore; - internal class MyContext : DbContext - { - public DbSet Blogs { get; set; } - } + internal class MyContext : DbContext + { + public DbSet Blogs { get; set; } + } - public class Blog + public class Blog + { + public Blog(int id, string name) { - public Blog(int id, string name) - { - Name = name; - } - - public int Id { get; private set; } // Noncompliant FP - public string Name { get; private set; } + Name = name; } - """) + + public int Id { get; private set; } // Noncompliant FP + public string Name { get; private set; } + } + """) .AddReferences(NuGetMetadataReference.MicrosoftEntityFrameworkCore("8.0.6")) .Verify(); #endif diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.Fixed.Batch.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.Fixed.Batch.cs index e2e1a3fc60b..74a3f1d6a25 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.Fixed.Batch.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.Fixed.Batch.cs @@ -167,6 +167,7 @@ public static void Call(dynamic d) public class EventHandlerSample { + private void MyOnClick(object sender, EventArgs args) { } // Compliant } public partial class EventHandlerSample1 @@ -427,3 +428,40 @@ internal struct NestedStruct } } } + +// https://github.com/SonarSource/sonar-dotnet/issues/6990 +namespace Repro_6990 +{ + public class ChildEventArgs : EventArgs + { + // Some properties + } + + public class InheritedEvent : EventArgs + { + // Some properties + } + + public class SenderObject + { + // Some things + } + + public class Consumer + { + static void SenderObject_WithChildEvent(SenderObject senderObject, ChildEventArgs e) // Compliant + { + // Logic + } + + static void SenderObject_WithChildEvent(SenderObject senderObject, InheritedEvent e) // Compliant + { + // Logic + } + + static void Comsumer_WithChildEvent(object sender, ChildEventArgs e) // Compliant + { + // Logic + } + } +} diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.Fixed.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.Fixed.cs index 14c20a202bb..bedd31f8e9a 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.Fixed.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.Fixed.cs @@ -151,6 +151,7 @@ public static void Call(dynamic d) public class EventHandlerSample { + private void MyOnClick(object sender, EventArgs args) { } // Compliant } public partial class EventHandlerSample1 @@ -411,3 +412,40 @@ internal struct NestedStruct } } } + +// https://github.com/SonarSource/sonar-dotnet/issues/6990 +namespace Repro_6990 +{ + public class ChildEventArgs : EventArgs + { + // Some properties + } + + public class InheritedEvent : EventArgs + { + // Some properties + } + + public class SenderObject + { + // Some things + } + + public class Consumer + { + static void SenderObject_WithChildEvent(SenderObject senderObject, ChildEventArgs e) // Compliant + { + // Logic + } + + static void SenderObject_WithChildEvent(SenderObject senderObject, InheritedEvent e) // Compliant + { + // Logic + } + + static void Comsumer_WithChildEvent(object sender, ChildEventArgs e) // Compliant + { + // Logic + } + } +} diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.cs index 327767a8e14..421d5775e7b 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.cs @@ -227,7 +227,7 @@ public static void Call(dynamic d) public class EventHandlerSample { - private void MyOnClick(object sender, EventArgs args) { } // Noncompliant + private void MyOnClick(object sender, EventArgs args) { } // Compliant } public partial class EventHandlerSample1 @@ -522,3 +522,40 @@ public struct UnusedNestedType { } // Noncompliant } } } + +// https://github.com/SonarSource/sonar-dotnet/issues/6990 +namespace Repro_6990 +{ + public class ChildEventArgs : EventArgs + { + // Some properties + } + + public class InheritedEvent : EventArgs + { + // Some properties + } + + public class SenderObject + { + // Some things + } + + public class Consumer + { + static void SenderObject_WithChildEvent(SenderObject senderObject, ChildEventArgs e) // Compliant + { + // Logic + } + + static void SenderObject_WithChildEvent(SenderObject senderObject, InheritedEvent e) // Compliant + { + // Logic + } + + static void Comsumer_WithChildEvent(object sender, ChildEventArgs e) // Compliant + { + // Logic + } + } +}