Skip to content

Fix S1144 FN: Unused local functions #9071

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cristian-ambrosini-sonarsource merged 4 commits into from
Apr 11, 2024
Merged

Fix S1144 FN: Unused local functions #9071

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fbf65dc
Fix S1144 FN: Unused local functions
CristianAmbrosini Apr 11, 2024
85b39ab
Address 1st round of comments
CristianAmbrosini Apr 11, 2024
d6f2ce2
Revert changes in CSharpSyntaxHelper
CristianAmbrosini Apr 11, 2024
9c32ef0
Remove spaces
CristianAmbrosini Apr 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 8 additions & 16 deletions analyzers/src/SonarAnalyzer.CSharp/Helpers/CSharpSyntaxHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -167,23 +167,15 @@ public static bool ContainsMethodInvocation(this BaseMethodDeclarationSyntax met
.Any(symbolPredicate);
}

public static SyntaxToken? GetIdentifierOrDefault(this BaseMethodDeclarationSyntax methodDeclaration)
{
switch (methodDeclaration?.Kind())
public static SyntaxToken? GetIdentifierOrDefault(this BaseMethodDeclarationSyntax methodDeclaration) =>

@cristian-ambrosini-sonarsource cristian-ambrosini-sonarsource Apr 11, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not strictly related to unused local functions, but requested in the same ticket.

methodDeclaration?.Kind() switch
{
case SyntaxKind.ConstructorDeclaration:
return ((ConstructorDeclarationSyntax)methodDeclaration).Identifier;

case SyntaxKind.DestructorDeclaration:
return ((DestructorDeclarationSyntax)methodDeclaration).Identifier;

case SyntaxKind.MethodDeclaration:
return ((MethodDeclarationSyntax)methodDeclaration).Identifier;

default:
return null;
}
}
SyntaxKind.ConstructorDeclaration => ((ConstructorDeclarationSyntax)methodDeclaration)?.Identifier,
SyntaxKind.DestructorDeclaration => ((DestructorDeclarationSyntax)methodDeclaration)?.Identifier,
SyntaxKind.MethodDeclaration => ((MethodDeclarationSyntax)methodDeclaration)?.Identifier,
Comment thread
Tim-Pohlmann marked this conversation as resolved.
Outdated
_ when LocalFunctionStatementSyntaxWrapper.IsInstance(methodDeclaration) => ((LocalFunctionStatementSyntaxWrapper)methodDeclaration).Identifier,

@Tim-Pohlmann Tim-Pohlmann Apr 11, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should follow the same pattern as the cases above using SyntaxKindEx.LocalFunctionStatement.

@cristian-ambrosini-sonarsource cristian-ambrosini-sonarsource Apr 11, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We cannot cast to LocalFunctionStatementSyntax because we don't have it, so it would be something like:
SyntaxKindEx.LocalFunctionStatement => ((LocalFunctionStatementSyntaxWrapper)methodDeclaration).Identifier,
I'm not a fan of it, I prefer using LocalFunctionStatementSyntaxWrapper.IsInstance to reduce the assumptions.

@Tim-Pohlmann Tim-Pohlmann Apr 11, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But it's the same assumption as in the three cases above.

_ => null,
};

public static bool IsMethodInvocation(this InvocationExpressionSyntax invocation, KnownType type, string methodName, SemanticModel semanticModel) =>
invocation.Expression.NameIs(methodName) &&
Expand Down
9 changes: 7 additions & 2 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/UnusedPrivateMember.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,14 +320,19 @@ public CSharpRemovableSymbolWalker(Func<SyntaxTree, bool, SemanticModel> getSema
this.containingTypeAccessibility = containingTypeAccessibility;
}

// This override is needed because VisitRecordDeclaration is not available due to the Roslyn version.
// This override is needed because VisitRecordDeclaration and LocalFunctionStatementSyntax are not available due to the Roslyn version.
public override void Visit(SyntaxNode node)
{
if (node.IsAnyKind(SyntaxKindEx.RecordClassDeclaration, SyntaxKindEx.RecordStructDeclaration))
{
VisitBaseTypeDeclaration(node);
}

if (LocalFunctionStatementSyntaxWrapper.IsInstance(node))
Comment thread
Tim-Pohlmann marked this conversation as resolved.
Outdated
{
ConditionalStore((IMethodSymbol)GetDeclaredSymbol(node), IsRemovableMethod);
}

base.Visit(node);
}

Expand Down Expand Up @@ -488,7 +493,7 @@ static bool IsPartial(TypeDeclarationSyntax typeDeclaration) =>

private static bool IsRemovableMethod(IMethodSymbol methodSymbol) =>
IsRemovableMember(methodSymbol)
&& (methodSymbol.MethodKind == MethodKind.Ordinary || methodSymbol.MethodKind == MethodKind.Constructor)
&& (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.IsSerializationConstructor()
Expand Down
1 change: 1 addition & 0 deletions analyzers/src/SonarAnalyzer.Common/Helpers/SymbolHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,7 @@ public static string GetClassification(this ISymbol symbol) =>
{ MethodKind: MethodKind.Destructor } => "destructor",
{ MethodKind: MethodKind.PropertyGet } => "getter",
{ MethodKind: MethodKind.PropertySet } => "setter",
{ MethodKind: MethodKindEx.LocalFunction } => "local function",
_ => "method",
},
INamedTypeSymbol namedTypeSymbol => namedTypeSymbol switch
Expand Down
2 changes: 1 addition & 1 deletion analyzers/tests/SonarAnalyzer.Test/Helpers/SymbolHelperTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -398,7 +398,7 @@ public void GetClassification_Record(TypeKind typeKind, string expected)
[DataRow(MethodKind.ExplicitInterfaceImplementation, "method")]
[DataRow(MethodKind.FunctionPointerSignature, "method")]
[DataRow(MethodKind.LambdaMethod, "method")]
[DataRow(MethodKind.LocalFunction, "method")]
[DataRow(MethodKind.LocalFunction, "local function")]
[DataRow(MethodKind.Ordinary, "method")]
[DataRow(MethodKind.PropertyGet, "getter")]
[DataRow(MethodKind.PropertySet, "setter")]
Expand Down
2 changes: 1 addition & 1 deletion analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.CSharp10.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ static void Quix() { }
[Obsolete]
static void ForCoverage() { }

static void NoAttribute() { }
static void NoAttribute() { } // Noncompliant
}
}

Expand Down
9 changes: 0 additions & 9 deletions analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.CSharp7.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,15 +198,6 @@ private enum Y
}
}

// https://github.com/SonarSource/sonar-dotnet/issues/6699
public class Repro_6699
{
public void MethodUsingLocalMethod()
{
void LocalMethod() { } // FN
}
}

// https://github.com/SonarSource/sonar-dotnet/issues/6724
public class Repro_6724
{
Expand Down
4 changes: 2 additions & 2 deletions analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.CSharp9.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ static void Quix() { }
[Obsolete]
static void ForCoverage() { }

static void NoAttribute() { }
static void NoAttribute() { } // Noncompliant
}
}

Expand Down Expand Up @@ -158,7 +158,7 @@ public bool MethodWithLocalFunction()
{
return false;

bool PrintMembers(StringBuilder builder) => true; // FN - local functions are not handled
bool PrintMembers(StringBuilder builder) => true; // Noncompliant
}
}

Expand Down
6 changes: 4 additions & 2 deletions analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.Fixed.Batch.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,13 @@ private interface MyInterface
void Method();
}

// https://github.com/SonarSource/sonar-dotnet/issues/6699
public void MethodUsingLocalMethod()
{
void LocalMethod() // FN: local function is never used
{
LocalMethod();

void LocalMethod()
{
}
}
}
Expand Down
6 changes: 4 additions & 2 deletions analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.Fixed.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,11 +78,13 @@ private int MyProperty
[My]
private class Class1 { }

// https://github.com/SonarSource/sonar-dotnet/issues/6699
public void MethodUsingLocalMethod()
{
void LocalMethod() // FN: local function is never used
{
LocalMethod();

void LocalMethod()
{
}
}
}
Expand Down
8 changes: 7 additions & 1 deletion analyzers/tests/SonarAnalyzer.Test/TestCases/UnusedPrivateMember.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,11 +144,17 @@ internal class Class4 : MyInterface // Noncompliant {{Remove the unused internal
public void Method() { }
}

// https://github.com/SonarSource/sonar-dotnet/issues/6699
public void MethodUsingLocalMethod()
{
void LocalMethod() // FN: local function is never used
LocalMethod();

void LocalMethod()
{
}

void UnusedLocalMethod() // Noncompliant {{Remove the unused private local function 'UnusedLocalMethod'.}}
{
}
}
}
Expand Down