From 34a4d5704b2181e0f4d9c1fddf1191f2f1d50a3c Mon Sep 17 00:00:00 2001 From: Sebastien Marichal Date: Wed, 26 Jun 2024 12:20:05 +0200 Subject: [PATCH 1/3] Fix S1144 FP: Ignore unused `Deconstruct` methods --- .../Rules/UnusedPrivateMember.cs | 5 ++++ .../TestCases/UnusedPrivateMember.CSharp7.cs | 25 ++++++++----------- .../TestCases/UnusedPrivateMember.CSharp8.cs | 4 +-- 3 files changed, 18 insertions(+), 16 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs index d6866fa7fd8..67fbb77a248 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs @@ -190,10 +190,15 @@ private static HashSet GetUnusedSymbols(CSharpSymbolUsageCollector usag .Except(usageCollector.UsedSymbols) .Where(x => !IsMentionedInDebuggerDisplay(x, usageCollector) && !IsAccessorUsed(x, usageCollector) + && !IsDeconstructMethod(x) && !usageCollector.PrivateAttributes.Contains(x) && !IsUsedWithReflection(x, usageCollector.TypesUsedWithReflection)) .ToHashSet(); + private static bool IsDeconstructMethod(ISymbol symbol) => + symbol is IMethodSymbol { Name: "Deconstruct", Parameters.Length: > 0 } method + && method.Parameters.All(x => x.RefKind == RefKind.Out); + private static bool IsAccessorUsed(ISymbol symbol, CSharpSymbolUsageCollector usageCollector) => symbol is IMethodSymbol { AssociatedSymbol: IPropertySymbol property } accessor && usageCollector.PropertyAccess.TryGetValue(property, out var access) diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.CSharp7.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.CSharp7.cs index 0a1e3fdba07..622496a7253 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.CSharp7.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.CSharp7.cs @@ -60,32 +60,29 @@ private sealed class PublicDeconstructWithInnerType { public void Deconstruct(out object a, out InternalDeconstruct b) { a = b = null; } - // deconstructors must be public, internal or protected internal - private void Deconstruct(out object a, out object b) { a = b = null; } // Noncompliant + private void Deconstruct(out object a, out object b) { a = b = null; } // Compliant, Deconstruct methods are ignored } internal sealed class InternalDeconstruct { internal void Deconstruct(out object a, out object b) { a = b = null; } - // deconstructors must be public, internal or protected internal - private void Deconstruct(out object a, out string b, out string c) { a = b = c = null; } // Noncompliant + private void Deconstruct(out object a, out string b, out string c) { a = b = c = null; } // Compliant, Deconstruct methods are ignored } private class PublicDeconstruct { public void Deconstruct(out object a, out object b, out object c) { a = b = c = null; } - // deconstructors must be public, internal or protected internal - protected void Deconstruct(out string a, out string b, out string c) { a = b = c = null; } // Noncompliant - private void Deconstruct(out object a, out string b, out string c) { a = b = c = null; } // Noncompliant + protected void Deconstruct(out string a, out string b, out string c) { a = b = c = null; } // Compliant, Deconstruct methods are ignored + private void Deconstruct(out object a, out string b, out string c) { a = b = c = null; } // Compliant, Deconstruct methods are ignored } private sealed class MultipleDeconstructors { public void Deconstruct(out object a, out object b, out object c) { a = b = c = null; } - public void Deconstruct(out object a, out object b) // Noncompliant + public void Deconstruct(out object a, out object b) // Compliant, Deconstruct methods are ignored { a = b = null; } @@ -95,25 +92,25 @@ private class ProtectedInternalDeconstruct { protected internal void Deconstruct(out object a, out object b) { a = b = null; } - protected internal void Deconstruct(out object a, out object b, out object c) { a = b = c = null; } // Noncompliant + protected internal void Deconstruct(out object a, out object b, out object c) { a = b = c = null; } // Compliant, Deconstruct methods are ignored } private class Ambiguous { public void Deconstruct(out string a, out string b, out string c) { a = b = c = null; } - public void Deconstruct(out object a, out object b, out object c) { a = b = c = null; } // Noncompliant FP, actually the one above is not used + public void Deconstruct(out object a, out object b, out object c) { a = b = c = null; } // Compliant, Deconstruct methods are ignored FP, actually the one above is not used } private class NotUsedDifferentArgumentCount { - public void Deconstruct(out string a, out string b, out string c) { a = b = c = null; } // Noncompliant - public void Deconstruct(out string a, out string b, out string c, out string d) { a = b = c = d = null; } // Noncompliant + public void Deconstruct(out string a, out string b, out string c) { a = b = c = null; } // Compliant, Deconstruct methods are ignored + public void Deconstruct(out string a, out string b, out string c, out string d) { a = b = c = d = null; } // Compliant, Deconstruct methods are ignored } private class NotUsedNotVisible { - protected void Deconstruct(out object a, out object b) { a = b = null; } // Noncompliant - private void Deconstruct(out string a, out string b) { a = b = null; } // Noncompliant + protected void Deconstruct(out object a, out object b) { a = b = null; } // Compliant, Deconstruct methods are ignored + private void Deconstruct(out string a, out string b) { a = b = null; } // Compliant, Deconstruct methods are ignored } private ForMethod ReturnFromMethod() => null; diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.CSharp8.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.CSharp8.cs index 7f646832d0b..621e6383886 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.CSharp8.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.CSharp8.cs @@ -33,12 +33,12 @@ public void SomeMethod() private sealed class ForSwitchArm { - public void Deconstruct(out object a, out object b) { a = b = null; } // Noncompliant FP + public void Deconstruct(out object a, out object b) { a = b = null; } // Compliant, Deconstruct methods are ignored } private sealed class ForIsPattern { - public void Deconstruct(out string a, out string b) { a = b = null; } // Noncompliant FP + public void Deconstruct(out string a, out string b) { a = b = null; } // Compliant } } } From 174f9d38da4ecf162bd59dade095226a35bb8a42 Mon Sep 17 00:00:00 2001 From: Sebastien Marichal Date: Wed, 26 Jun 2024 14:05:58 +0200 Subject: [PATCH 2/3] Add more test cases --- .../Rules/UnusedPrivateMember.cs | 1 + .../TestCases/UnusedPrivateMember.CSharp7.cs | 12 ++++++++++++ 2 files changed, 13 insertions(+) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs index 67fbb77a248..0c70ef038cc 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs @@ -197,6 +197,7 @@ private static HashSet GetUnusedSymbols(CSharpSymbolUsageCollector usag private static bool IsDeconstructMethod(ISymbol symbol) => symbol is IMethodSymbol { Name: "Deconstruct", Parameters.Length: > 0 } method + && method.ReturnType.Is(KnownType.Void) && method.Parameters.All(x => x.RefKind == RefKind.Out); private static bool IsAccessorUsed(ISymbol symbol, CSharpSymbolUsageCollector usageCollector) => diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.CSharp7.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.CSharp7.cs index 622496a7253..19837f1cf54 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.CSharp7.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.CSharp7.cs @@ -113,6 +113,18 @@ private class NotUsedNotVisible private void Deconstruct(out string a, out string b) { a = b = null; } // Compliant, Deconstruct methods are ignored } + public class InvalidDeconstruct + { + private void Deconstruct(object a, out object b, out object c) { b = c = a; } // Noncompliant + private void Deconstruct() { } // Noncompliant + + private int Deconstruct(out object a, out object b, out object c) // Noncompliant + { + a = b = c = null; + return 42; + } + } + private ForMethod ReturnFromMethod() => null; private sealed class ForMethod { From 0cc577b1765f2cd8424103e6d2a8ae6d2e5b2d54 Mon Sep 17 00:00:00 2001 From: Sebastien Marichal Date: Thu, 27 Jun 2024 14:18:20 +0200 Subject: [PATCH 3/3] Address comment --- .../SonarAnalyzer.Test/TestCases/UnusedPrivateMember.CSharp7.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.CSharp7.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.CSharp7.cs index 19837f1cf54..a4bad27cac1 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.CSharp7.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.CSharp7.cs @@ -98,7 +98,7 @@ private class ProtectedInternalDeconstruct private class Ambiguous { public void Deconstruct(out string a, out string b, out string c) { a = b = c = null; } - public void Deconstruct(out object a, out object b, out object c) { a = b = c = null; } // Compliant, Deconstruct methods are ignored FP, actually the one above is not used + public void Deconstruct(out object a, out object b, out object c) { a = b = c = null; } // Compliant, Deconstruct methods are ignored } private class NotUsedDifferentArgumentCount