Skip to content

Commit

Permalink
Fix S1144 FN: Unused private getters and private setters (#9098)
Browse files Browse the repository at this point in the history
  • Loading branch information
CristianAmbrosini committed Apr 18, 2024
1 parent 5a7f05e commit 692b1ee
Show file tree
Hide file tree
Showing 14 changed files with 157 additions and 24 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -264,6 +264,12 @@
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/CSharpLatest/CSharpLatest/CSharp9Features/S4015.cs#L12",
"Location": "Line 12 Position 20-28"
},
{
"Id": "S1144",
"Message": "Remove the unused private setter \u0027set_Property\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/CSharpLatest/CSharpLatest/CSharp9Features/S4015.cs#L12",
"Location": "Line 12 Position 36-49"
},
{
"Id": "S1144",
"Message": "Remove the unused private property \u0027Property\u0027.",
Expand Down
18 changes: 18 additions & 0 deletions analyzers/its/expected/akka.net/S1144-Akka-netstandard2.0.json
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
{
"Issues": [
{
"Id": "S1144",
"Message": "Remove the unused private setter \u0027set_TypeName\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/core/Akka/Actor/Props.cs#L229",
"Location": "Line 229 Position 13-61"
},
{
"Id": "S1144",
"Message": "Remove the unused private class \u0027FactoryConsumer\u0027.",
Expand Down Expand Up @@ -60,6 +66,18 @@
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/core/Akka/Helios.Concurrency.DedicatedThreadPool.cs#L569",
"Location": "Line 569 Position 13-58"
},
{
"Id": "S1144",
"Message": "Remove the unused private setter \u0027set_ExceptionHandler\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/core/Akka/Helios.Concurrency.DedicatedThreadPool.cs#L88",
"Location": "Line 88 Position 58-70"
},
{
"Id": "S1144",
"Message": "Remove the unused private setter \u0027set_ThreadMaxStackSize\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/core/Akka/Helios.Concurrency.DedicatedThreadPool.cs#L93",
"Location": "Line 93 Position 46-58"
},
{
"Id": "S1144",
"Message": "Remove the unused private property \u0027TotalBufferSize\u0027.",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
{
"Issues": [
{
"Id": "S1144",
"Message": "Remove the unused private setter \u0027set_Log\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/core/Akka.Cluster/AutoDown.cs#L270",
"Location": "Line 270 Position 43-55"
},
{
"Id": "S1144",
"Message": "Remove the unused private field \u0027_log\u0027.",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
{
"Issues": [
{
"Id": "S1144",
"Message": "Remove the unused private setter \u0027set_NextSink\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/core/Akka.MultiNodeTestRunner/Discovery.cs#L39",
"Location": "Line 39 Position 45-57"
},
{
"Id": "S1144",
"Message": "Remove the unused private field \u0027_validNetCorePlatform\u0027.",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
{
"Issues": [
{
"Id": "S1144",
"Message": "Remove the unused private setter \u0027set_NextSink\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/core/Akka.MultiNodeTestRunner/Discovery.cs#L39",
"Location": "Line 39 Position 45-57"
},
{
"Id": "S1144",
"Message": "Remove the unused private method \u0027ChangeDllPathPlatform\u0027.",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,11 @@
{
"Issues": [
{
"Id": "S1144",
"Message": "Remove the unused private setter \u0027set_NextSink\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/core/Akka.MultiNodeTestRunner/Discovery.cs#L39",
"Location": "Line 39 Position 45-57"
},
{
"Id": "S1144",
"Message": "Remove the unused private method \u0027ChangeDllPathPlatform\u0027.",
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
{
"Issues": [
{
"Id": "S1144",
"Message": "Remove the unused private setter \u0027set_TestRunCompleted\u0027.",
"Uri": "https://github.com/SonarSource/sonar-dotnet/blob/master/analyzers/its/Projects/akka.net/src/core/Akka.MultiNodeTestRunner.Shared/Reporting/TestRunCoordinator.cs#L80",
"Location": "Line 80 Position 53-65"
}
]
}
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ private enum SymbolAccess

public ISet<ISymbol> UsedSymbols { get; } = new HashSet<ISymbol>();
public IDictionary<ISymbol, SymbolUsage> FieldSymbolUsages { get; } = new Dictionary<ISymbol, SymbolUsage>();
public HashSet<string> DebuggerDisplayValues { get; } = new();
public Dictionary<IPropertySymbol, AccessorAccess> PropertyAccess { get; } = new();
public HashSet<string> DebuggerDisplayValues { get; } = [];
public Dictionary<IPropertySymbol, AccessorAccess> PropertyAccess { get; } = [];

public CSharpSymbolUsageCollector(Compilation compilation, IEnumerable<ISymbol> knownSymbols)
{
Expand Down
30 changes: 25 additions & 5 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ private static void NamedSymbolAction(SonarSymbolReportingContext context, HashS
var fieldLikeSymbols = new BidirectionalDictionary<ISymbol, SyntaxNode>();
if (GatherSymbols(namedType, context.Compilation, privateSymbols, removableInternalTypes, fieldLikeSymbols, context)
&& privateSymbols.Any()
&& new CSharpSymbolUsageCollector(context.Compilation, privateSymbols) is var usageCollector
&& new CSharpSymbolUsageCollector(context.Compilation, AssociatedSymbols(privateSymbols)) is var usageCollector
&& VisitDeclaringReferences(namedType, usageCollector, context, includeGeneratedFile: true))
{
foreach (var diagnostic in DiagnosticsForUnusedPrivateMembers(usageCollector, privateSymbols, SyntaxConstants.Private, fieldLikeSymbols))
Expand All @@ -98,6 +98,9 @@ private static void NamedSymbolAction(SonarSymbolReportingContext context, HashS
}
}

private static IEnumerable<ISymbol> AssociatedSymbols(IEnumerable<ISymbol> privateSymbols) =>
privateSymbols.Select(x => x is IMethodSymbol { AssociatedSymbol: IPropertySymbol property } ? property : x);

private static bool GatherSymbols(INamedTypeSymbol namedType,
Compilation compilation,
HashSet<ISymbol> privateSymbols,
Expand Down Expand Up @@ -174,12 +177,19 @@ private static IEnumerable<Diagnostic> DiagnosticsForUsedButUnreadFields(CSharpS
private static HashSet<ISymbol> GetUnusedSymbols(CSharpSymbolUsageCollector usageCollector, IEnumerable<ISymbol> removableSymbols) =>
removableSymbols
.Except(usageCollector.UsedSymbols)
.Where(symbol => !IsMentionedInDebuggerDisplay(symbol, usageCollector))
.Where(symbol => !IsMentionedInDebuggerDisplay(symbol, usageCollector) && !IsAccessorUsed(symbol, usageCollector))
.ToHashSet();

private static IEnumerable<Diagnostic> GetDiagnosticsForUnreadFields(IEnumerable<SymbolUsage> unreadFields) =>
unreadFields.Select(usage => Diagnostic.Create(RuleS4487, usage.Declaration.GetLocation(), GetFieldAccessibilityForMessage(usage.Symbol), usage.Symbol.Name));

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));

private static string GetFieldAccessibilityForMessage(ISymbol symbol) =>
symbol.DeclaredAccessibility == Accessibility.Private ? SyntaxConstants.Private : "private class";

Expand Down Expand Up @@ -314,9 +324,9 @@ private sealed class CSharpRemovableSymbolWalker : SafeCSharpSyntaxWalker
private readonly Func<SyntaxNode, SemanticModel> getSemanticModel;
private readonly Accessibility containingTypeAccessibility;

public Dictionary<ISymbol, SyntaxNode> FieldLikeSymbols { get; } = new();
public HashSet<ISymbol> InternalSymbols { get; } = new();
public HashSet<ISymbol> PrivateSymbols { get; } = new();
public Dictionary<ISymbol, SyntaxNode> FieldLikeSymbols { get; } = [];
public HashSet<ISymbol> InternalSymbols { get; } = [];
public HashSet<ISymbol> PrivateSymbols { get; } = [];

public CSharpRemovableSymbolWalker(Func<SyntaxTree, bool, SemanticModel> getSemanticModel, Accessibility containingTypeAccessibility)
{
Expand Down Expand Up @@ -430,6 +440,16 @@ public override void VisitMethodDeclaration(MethodDeclarationSyntax node)
base.VisitMethodDeclaration(node);
}

public override void VisitAccessorDeclaration(AccessorDeclarationSyntax node)
{
if (node.Modifiers.Any(SyntaxKind.PrivateKeyword))
{
ConditionalStore(GetDeclaredSymbol(node), IsRemovable);
}

base.VisitAccessorDeclaration(node);
}

public override void VisitPropertyDeclaration(PropertyDeclarationSyntax node)
{
if (IsPrivateOrInPrivateType(node.Modifiers))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -136,6 +136,60 @@ public void Method3()
private int Property12 { get; set; } = 42; // FN
}
").Verify();

[TestMethod]
public void UnusedPrivateMember_Properties_Accessors() =>
builder.AddSnippet(@"
using System;
public class PropertyUsages
{
public int AProperty { private get; set; } // Noncompliant {{Remove the unused private getter 'get_AProperty'.}}
public int BProperty { get; private set; } // Noncompliant {{Remove the unused private setter 'set_BProperty'.}}
public int CProperty { internal get; set; } // Compliant
public int DProperty { get; internal set; } // Compliant
public int EProperty { protected get; set; } // Compliant
public int E2Property { get; protected set; } // Compliant
public int FProperty { get; private set; } // Compliant
public int GProperty { private get; set; } // Noncompliant {{Remove the unused private getter 'get_GProperty'.}}
public int HProperty { get; private set; } // Noncompliant {{Remove the unused private setter 'set_HProperty'.}}
public int IProperty { private get; set; } // Compliant
public int JProperty { get; private set; } // Compliant: both read and write
public int KProperty { private get; set; } // Compliant: both read and write
public int LProperty { get; private set; } // FN: private set is used in the constructor, not necessary
protected int MProperty { private get; set; } // Noncompliant {{Remove the unused private getter 'get_MProperty'.}}
public PropertyUsages()
{
LProperty = 42;
}
public void Method()
{
FProperty = HProperty;
GProperty = IProperty;
JProperty = KProperty;
KProperty = JProperty;
}
public interface ISomeInterface
{
string Something { get; }
string SomethingElse { get; }
}
public class SomeClass : ISomeInterface
{
public string Something { get; private set; } // Compliant
public string SomethingElse { get; private set; } // Noncompliant
public void Method(string str)
{
Something = str;
}
}
}
").Verify();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -201,10 +201,10 @@ private enum Y
// https://github.com/SonarSource/sonar-dotnet/issues/6724
public class Repro_6724
{
public int PrivateGetter { private get; set; } // FN
public int PrivateSetter { get; private set; } // FN
public int PrivateGetter { private get; set; } // Noncompliant
public int PrivateSetter { get; private set; } // Noncompliant

public int ExpressionBodiedPropertyWithPrivateGetter { private get => 1; set => _ = value; } // FN
public int ExpressionBodiedPropertyWithPrivateSetter { get => 1; private set => _ = value; } // FN
public int ExpressionBodiedPropertyWithPrivateGetter { private get => 1; set => _ = value; } // Noncompliant
public int ExpressionBodiedPropertyWithPrivateSetter { get => 1; private set => _ = value; } // Noncompliant
}

Original file line number Diff line number Diff line change
Expand Up @@ -176,12 +176,12 @@ public class PropertyAccess
private int OnlyRead { get; } // Fixed
private int OnlySet { get; set; }
private int OnlySet2 { set { } } // Fixed
public int PrivateGetter { private get; set; } // FN - unused private getter
public int PrivateSetter { get; private set; } // FN - unused private setter
public int PrivateGetter { set; } // Fixed
public int PrivateSetter { get; } // Fixed
private int ExpressionBodiedProperty5 { set => _ = value; } // Fixed
private int ExpressionBodiedProperty6 { get => 1; } // Fixed
public int ExpressionBodiedProperty7 { private get => 1; set => _ = value; } // FN - unused private getter
public int ExpressionBodiedProperty8 { get => 1; private set => _ = value; } // FN - unused private setter
public int ExpressionBodiedProperty7 { set => _ = value; } // Fixed
public int ExpressionBodiedProperty8 { get => 1; } // Fixed

private int BothAccessed { get; set; }

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -160,12 +160,12 @@ public class PropertyAccess
private int OnlyRead { get; } // Fixed
private int OnlySet { get; set; }
private int OnlySet2 { set { } } // Fixed
public int PrivateGetter { private get; set; } // FN - unused private getter
public int PrivateSetter { get; private set; } // FN - unused private setter
public int PrivateGetter { set; } // Fixed
public int PrivateSetter { get; } // Fixed
private int ExpressionBodiedProperty5 { set => _ = value; } // Fixed
private int ExpressionBodiedProperty6 { get => 1; } // Fixed
public int ExpressionBodiedProperty7 { private get => 1; set => _ = value; } // FN - unused private getter
public int ExpressionBodiedProperty8 { get => 1; private set => _ = value; } // FN - unused private setter
public int ExpressionBodiedProperty7 { set => _ = value; } // Fixed
public int ExpressionBodiedProperty8 { get => 1; } // Fixed

private int BothAccessed { get; set; }

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -241,9 +241,10 @@ public class PropertyAccess
// ^^^
private int NotAccessed { get; set; } // Noncompliant {{Remove the unused private property 'NotAccessed'.}}
// ^^^^^^^^^^^
public int PrivateGetter { private get; set; } // FN - unused private getter
public int PrivateSetter { get; private set; } // FN - unused private setter

public int PrivateGetter { private get; set; } // Noncompliant {{Remove the unused private getter 'get_PrivateGetter'.}}
// ^^^^^^^^^^^^
public int PrivateSetter { get; private set; } // Noncompliant {{Remove the unused private setter 'set_PrivateSetter'.}}
// ^^^^^^^^^^^^
private int ExpressionBodiedProperty => 1; // Noncompliant {{Remove the unused private property 'ExpressionBodiedProperty'.}}
// ^^^^^^^^^^^^^^^^^^^^^^^^
private int ExpressionBodiedProperty2 { get => 1; } // Noncompliant
Expand All @@ -253,8 +254,8 @@ public class PropertyAccess
// ^^^
private int ExpressionBodiedProperty6 { get => 1; set => _ = value; } // Noncompliant
// ^^^
public int ExpressionBodiedProperty7 { private get => 1; set => _ = value; } // FN - unused private getter
public int ExpressionBodiedProperty8 { get => 1; private set => _ = value; } // FN - unused private setter
public int ExpressionBodiedProperty7 { private get => 1; set => _ = value; } // Noncompliant
public int ExpressionBodiedProperty8 { get => 1; private set => _ = value; } // Noncompliant

private int BothAccessed { get; set; }

Expand Down

0 comments on commit 692b1ee

Please sign in to comment.