From c3e0d904b844c990d431cf6271c55f1351d2c6cf Mon Sep 17 00:00:00 2001 From: Costin Zaharia <costin.zaharia@sonarsource.com> Date: Mon, 22 Nov 2021 15:28:24 +0100 Subject: [PATCH 1/4] Rule S1144: remove duplicate issues in top level statements - the symbols were processed twice, once for the Program symbol and the second time for the types defined in the top level statement file. --- .../Rules/UnusedPrivateMember.cs | 39 ++++++++++++------- .../Rules/UnusedPrivateMemberTest.cs | 6 ++- .../UnusedPrivateMember.CSharp9.Second.cs | 7 ++++ ...rivateMember.CSharp9.TopLevelStatements.cs | 5 --- .../TestCases/UnusedPrivateMember.CSharp9.cs | 4 ++ 5 files changed, 42 insertions(+), 19 deletions(-) create mode 100644 analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedPrivateMember.CSharp9.Second.cs diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs index 8d2510a1801..470ee8f065a 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs @@ -72,16 +72,11 @@ protected override void Initialize(SonarAnalysisContext context) => { var namedType = (INamedTypeSymbol)cc.Symbol; - if (namedType.TypeKind != TypeKind.Struct - && namedType.TypeKind != TypeKind.Class - && namedType.TypeKind != TypeKind.Delegate - && namedType.TypeKind != TypeKind.Enum - && namedType.TypeKind != TypeKind.Interface) - { - return; - } - - if (namedType.ContainingType != null || namedType.DerivesFromAny(IgnoredTypes)) + if (namedType.ContainingType != null + // We skip top level statements since they cannot have fields. Other declared types are analyzed separately. + || IsTopLevelProgram(namedType) + || !IsType(namedType) + || namedType.DerivesFromAny(IgnoredTypes)) { return; } @@ -137,8 +132,24 @@ protected override void Initialize(SonarAnalysisContext context) => }); }); - private static IEnumerable<Diagnostic> GetDiagnosticsForUnusedPrivateMembers(CSharpSymbolUsageCollector usageCollector, ISet<ISymbol> removableSymbols, string accessibility, - BidirectionalDictionary<ISymbol, SyntaxNode> fieldLikeSymbols) + private static bool IsTopLevelProgram(ISymbol symbol) => + // If the symbol is named `Program` and the containing namespace is the global one, we will assume that we are analyzing a top level file. + // We cannot tell for sure, but in this case we prefer to have FNs instead of FPs, considering that there is a low chance to have + // a program class in the global namespace. + symbol.Name == "Program" + && symbol.ContainingNamespace.IsGlobalNamespace; + + private static bool IsType(ITypeSymbol namedType) => + namedType.TypeKind == TypeKind.Struct + || namedType.TypeKind == TypeKind.Class + || namedType.TypeKind == TypeKind.Delegate + || namedType.TypeKind == TypeKind.Enum + || namedType.TypeKind == TypeKind.Interface; + + private static IEnumerable<Diagnostic> GetDiagnosticsForUnusedPrivateMembers(CSharpSymbolUsageCollector usageCollector, + ISet<ISymbol> removableSymbols, + string accessibility, + BidirectionalDictionary<ISymbol, SyntaxNode> fieldLikeSymbols) { var unusedSymbols = GetUnusedSymbols(usageCollector, removableSymbols); @@ -181,7 +192,9 @@ private static IEnumerable<Diagnostic> GetDiagnosticsForUnreadFields(IEnumerable private static string GetFieldAccessibilityForMessage(ISymbol symbol) => symbol.DeclaredAccessibility == Accessibility.Private ? "private" : "private class"; - private static IEnumerable<Diagnostic> GetDiagnosticsForMembers(ICollection<ISymbol> unusedSymbols, string accessibility, BidirectionalDictionary<ISymbol, SyntaxNode> fieldLikeSymbols) + private static IEnumerable<Diagnostic> GetDiagnosticsForMembers(ICollection<ISymbol> unusedSymbols, + string accessibility, + BidirectionalDictionary<ISymbol, SyntaxNode> fieldLikeSymbols) { var diagnostics = new List<Diagnostic>(); var alreadyReportedFieldLikeSymbols = new HashSet<ISymbol>(); diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedPrivateMemberTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedPrivateMemberTest.cs index 9087a22ef92..a863a7f7afc 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedPrivateMemberTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedPrivateMemberTest.cs @@ -203,7 +203,11 @@ public void UnusedPrivateMember_FromCSharp8() => [TestMethod] public void UnusedPrivateMember_FromCSharp9() => Verifier.VerifyAnalyzerFromCSharp9Library( - @"TestCases\UnusedPrivateMember.CSharp9.cs", + new[] + { + @"TestCases\UnusedPrivateMember.CSharp9.cs", + @"TestCases\UnusedPrivateMember.CSharp9.Second.cs" + }, new UnusedPrivateMember()); [TestMethod] diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedPrivateMember.CSharp9.Second.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedPrivateMember.CSharp9.Second.cs new file mode 100644 index 00000000000..5fd8d0fab90 --- /dev/null +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedPrivateMember.CSharp9.Second.cs @@ -0,0 +1,7 @@ +namespace Tests.Diagnostics +{ + public partial class PartialMethods + { + partial void UnusedMethod(); // Noncompliant + } +} diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedPrivateMember.CSharp9.TopLevelStatements.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedPrivateMember.CSharp9.TopLevelStatements.cs index 11d0b0d394c..1c94da7bba1 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedPrivateMember.CSharp9.TopLevelStatements.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedPrivateMember.CSharp9.TopLevelStatements.cs @@ -13,19 +13,16 @@ public record Product(string Name, int CategoryId); public record Record { private int a; // Noncompliant {{Remove the unused private field 'a'.}} - // Noncompliant@-1 - duplicate issue reported private int b; public int B() => b; private nint Value { get; init; } private nint UnusedValue { get; init; } // Noncompliant - // Noncompliant@-1 - duplicate issue reported public Record Create() => new() { Value = 1 }; private interface IFoo // Noncompliant - // Noncompliant@-1 - duplicate issue reported { public void Bar() { } } @@ -45,7 +42,6 @@ public void UseNested2() } private record UnusedNested1(string Name, int CategoryId); // Noncompliant - // Noncompliant@-1 - duplicate issue reported internal record UnusedNested2(string Name, int CategoryId); // Noncompliant public record UnusedNested3(string Name, int CategoryId); @@ -76,7 +72,6 @@ private TargetTypedNew(int arg) } private TargetTypedNew(string arg) // Noncompliant - FP - // Noncompliant@-1 - duplicate issue reported { var x = arg; } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedPrivateMember.CSharp9.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedPrivateMember.CSharp9.cs index f988aa6c89a..21037b3750a 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedPrivateMember.CSharp9.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedPrivateMember.CSharp9.cs @@ -100,4 +100,8 @@ public static void Foo() PositionalRecord @record = new PositionalRecord(""); } } + + public partial class PartialMethods + { + } } From f030a08f99d01dfa2e78e7ba8ca83b4c5a74b889 Mon Sep 17 00:00:00 2001 From: Costin Zaharia <costin.zaharia@sonarsource.com> Date: Mon, 22 Nov 2021 19:23:33 +0100 Subject: [PATCH 2/4] Add test with enums --- .../TestCases/UnusedPrivateMember.CSharp7.cs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedPrivateMember.CSharp7.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedPrivateMember.CSharp7.cs index 84b7f814794..328ac268b56 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedPrivateMember.CSharp7.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/UnusedPrivateMember.CSharp7.cs @@ -159,4 +159,23 @@ public class EmptyCtor // That's invalid syntax, but it is still empty ctor and we should not raise for it, even if it is not used public EmptyCtor() => // Error [CS1525,CS1002] } + + public class WithEnums + { + private enum X // Noncompliant + { + A + } + + public void UseEnum() + { + var b = Y.B; + } + + private enum Y + { + A, + B + } + } } From 98e07153c1b21a3411e8757d0ea43b4ff1c08367 Mon Sep 17 00:00:00 2001 From: Costin Zaharia <costin.zaharia@sonarsource.com> Date: Mon, 22 Nov 2021 19:34:14 +0100 Subject: [PATCH 3/4] Remove type check - not needed --- .../src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs | 8 -------- 1 file changed, 8 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs index 470ee8f065a..e140a435eb6 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs @@ -75,7 +75,6 @@ protected override void Initialize(SonarAnalysisContext context) => if (namedType.ContainingType != null // We skip top level statements since they cannot have fields. Other declared types are analyzed separately. || IsTopLevelProgram(namedType) - || !IsType(namedType) || namedType.DerivesFromAny(IgnoredTypes)) { return; @@ -139,13 +138,6 @@ private static bool IsTopLevelProgram(ISymbol symbol) => symbol.Name == "Program" && symbol.ContainingNamespace.IsGlobalNamespace; - private static bool IsType(ITypeSymbol namedType) => - namedType.TypeKind == TypeKind.Struct - || namedType.TypeKind == TypeKind.Class - || namedType.TypeKind == TypeKind.Delegate - || namedType.TypeKind == TypeKind.Enum - || namedType.TypeKind == TypeKind.Interface; - private static IEnumerable<Diagnostic> GetDiagnosticsForUnusedPrivateMembers(CSharpSymbolUsageCollector usageCollector, ISet<ISymbol> removableSymbols, string accessibility, From 36a11bdef6fc037b7d999258d9f950ef7c2cfeda Mon Sep 17 00:00:00 2001 From: Costin Zaharia <costin.zaharia@sonarsource.com> Date: Tue, 23 Nov 2021 14:36:18 +0100 Subject: [PATCH 4/4] Extract IsTopLevelProgram --- .../ShimLayer/INamedTypeSymbolExtensions.cs | 33 ++++++++++--------- .../Rules/UnusedPrivateMember.cs | 9 +---- .../Rules/UnusedPrivateMemberTest.cs | 12 ++----- .../TestFramework/Verifier.cs | 10 +++--- 4 files changed, 26 insertions(+), 38 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CFG/ShimLayer/INamedTypeSymbolExtensions.cs b/analyzers/src/SonarAnalyzer.CFG/ShimLayer/INamedTypeSymbolExtensions.cs index afbb30853f2..8230e1b03e9 100644 --- a/analyzers/src/SonarAnalyzer.CFG/ShimLayer/INamedTypeSymbolExtensions.cs +++ b/analyzers/src/SonarAnalyzer.CFG/ShimLayer/INamedTypeSymbolExtensions.cs @@ -1,14 +1,19 @@ // Copyright (c) Tunnel Vision Laboratories, LLC. All Rights Reserved. // Licensed under the MIT License. See LICENSE in the project root for license information. +using System; +using System.Collections.Generic; +using System.Collections.Immutable; +using System.Linq; +using Microsoft.CodeAnalysis; + namespace StyleCop.Analyzers.Lightup { - using System; - using System.Collections.Immutable; - using Microsoft.CodeAnalysis; - public static class INamedTypeSymbolExtensions { + private const string MainMethodImplicitName = "<Main>$"; + + private static readonly HashSet<string> ProgramClassImplicitName = new HashSet<string> { "Program", "<Program>$" }; private static readonly Func<INamedTypeSymbol, INamedTypeSymbol> TupleUnderlyingTypeAccessor; private static readonly Func<INamedTypeSymbol, ImmutableArray<IFieldSymbol>> TupleElementsAccessor; private static readonly Func<INamedTypeSymbol, bool> IsSerializableAccessor; @@ -20,19 +25,15 @@ static INamedTypeSymbolExtensions() IsSerializableAccessor = LightupHelpers.CreateSyntaxPropertyAccessor<INamedTypeSymbol, bool>(typeof(INamedTypeSymbol), nameof(IsSerializable)); } - public static INamedTypeSymbol TupleUnderlyingType(this INamedTypeSymbol symbol) - { - return TupleUnderlyingTypeAccessor(symbol); - } + public static INamedTypeSymbol TupleUnderlyingType(this INamedTypeSymbol symbol) => TupleUnderlyingTypeAccessor(symbol); - public static ImmutableArray<IFieldSymbol> TupleElements(this INamedTypeSymbol symbol) - { - return TupleElementsAccessor(symbol); - } + public static ImmutableArray<IFieldSymbol> TupleElements(this INamedTypeSymbol symbol) => TupleElementsAccessor(symbol); - public static bool IsSerializable(this INamedTypeSymbol symbol) - { - return IsSerializableAccessor(symbol); - } + public static bool IsSerializable(this INamedTypeSymbol symbol) => IsSerializableAccessor(symbol); + + public static bool IsTopLevelProgram(this INamedTypeSymbol symbol) => + ProgramClassImplicitName.Contains(symbol.Name) + && symbol.ContainingNamespace.IsGlobalNamespace + && symbol.GetMembers(MainMethodImplicitName).Any(); } } diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs index e140a435eb6..626627a0776 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs @@ -74,7 +74,7 @@ protected override void Initialize(SonarAnalysisContext context) => if (namedType.ContainingType != null // We skip top level statements since they cannot have fields. Other declared types are analyzed separately. - || IsTopLevelProgram(namedType) + || namedType.IsTopLevelProgram() || namedType.DerivesFromAny(IgnoredTypes)) { return; @@ -131,13 +131,6 @@ protected override void Initialize(SonarAnalysisContext context) => }); }); - private static bool IsTopLevelProgram(ISymbol symbol) => - // If the symbol is named `Program` and the containing namespace is the global one, we will assume that we are analyzing a top level file. - // We cannot tell for sure, but in this case we prefer to have FNs instead of FPs, considering that there is a low chance to have - // a program class in the global namespace. - symbol.Name == "Program" - && symbol.ContainingNamespace.IsGlobalNamespace; - private static IEnumerable<Diagnostic> GetDiagnosticsForUnusedPrivateMembers(CSharpSymbolUsageCollector usageCollector, ISet<ISymbol> removableSymbols, string accessibility, diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedPrivateMemberTest.cs b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedPrivateMemberTest.cs index a863a7f7afc..de29ae8cdc9 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedPrivateMemberTest.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/Rules/UnusedPrivateMemberTest.cs @@ -212,21 +212,15 @@ public void UnusedPrivateMember_FromCSharp9() => [TestMethod] public void UnusedPrivateMember_FromCSharp9_TopLevelStatements() => - Verifier.VerifyAnalyzerFromCSharp9Console( - @"TestCases\UnusedPrivateMember.CSharp9.TopLevelStatements.cs", - new UnusedPrivateMember()); + Verifier.VerifyAnalyzerFromCSharp9Console(@"TestCases\UnusedPrivateMember.CSharp9.TopLevelStatements.cs", new UnusedPrivateMember()); [TestMethod] public void UnusedPrivateMember_FromCSharp10() => - Verifier.VerifyAnalyzerFromCSharp10Library( - @"TestCases\UnusedPrivateMember.CSharp10.cs", - new UnusedPrivateMember()); + Verifier.VerifyAnalyzerFromCSharp10Library(@"TestCases\UnusedPrivateMember.CSharp10.cs", new UnusedPrivateMember()); [TestMethod] public void UnusedPrivateMember_FromCSharpPreview() => - Verifier.VerifyAnalyzerCSharpPreviewLibrary( - @"TestCases\UnusedPrivateMember.CSharpPreview.cs", - new UnusedPrivateMember()); + Verifier.VerifyAnalyzerCSharpPreviewLibrary(@"TestCases\UnusedPrivateMember.CSharpPreview.cs", new UnusedPrivateMember()); #endif [TestMethod] diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestFramework/Verifier.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestFramework/Verifier.cs index 5f67c4c13af..6f97c8b809f 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestFramework/Verifier.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestFramework/Verifier.cs @@ -112,11 +112,11 @@ public static void VerifyAnalyzerFromCSharp9Console(string path, DiagnosticAnaly public static void VerifyAnalyzerFromCSharp10Console(string path, DiagnosticAnalyzer diagnosticAnalyzer, IEnumerable<MetadataReference> additionalReferences = null) => VerifyNonConcurrentAnalyzer(new[] { path }, - new[] { diagnosticAnalyzer }, - ParseOptionsHelper.FromCSharp10, - CompilationErrorBehavior.Default, - OutputKind.ConsoleApplication, - additionalReferences); + new[] { diagnosticAnalyzer }, + ParseOptionsHelper.FromCSharp10, + CompilationErrorBehavior.Default, + OutputKind.ConsoleApplication, + additionalReferences); /// <summary> /// Verify analyzer from C# 9 with top level statements in non-concurrent execution mode.