Skip to content
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.

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

New Rule S3398: Private static methods called only by inner class #6781

Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
49 changes: 49 additions & 0 deletions analyzers/rspec/cs/S3398_c#.html
@@ -0,0 +1,49 @@
<p>When a <code>private</code> method is only invoked by an inner class, there’s no reason not to move it into that class. It will still have the same
access to the outer class' members, but the outer class will be clearer and less cluttered.</p>
<h2>Noncompliant Code Example</h2>
<pre>
public class Outer
andrei-epure-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
{
public void OuterMethod()
{
Console.WriteLine("Called from an outer method.");
}

private static void Print(int num) // Noncompliant - method is only used by inner class, should be moved there
{
Console.WriteLine(num);
}

public class Inner
{
public void SomeMethod()
{
Outer.Print(42);
}
}
}
</pre>
<h2>Compliant Solution</h2>
<pre>
public class Outer
{
public void OuterMethod()
{
Console.WriteLine("Called from an outer method.");
}

public class Inner
{
public void SomeMethod()
{
Print(42);
}

private static void Print(int num)
{
Console.WriteLine(num);
}
}
}
</pre>

17 changes: 17 additions & 0 deletions analyzers/rspec/cs/S3398_c#.json
@@ -0,0 +1,17 @@
{
"title": "\"private\" methods called only by inner classes should be moved to those classes",
"type": "CODE_SMELL",
"status": "ready",
"remediation": {
"func": "Constant\/Issue",
"constantCost": "5min"
},
"tags": [
"confusing"
],
"defaultSeverity": "Minor",
"ruleSpecification": "RSPEC-3398",
"sqKey": "S3398",
"scope": "All",
"quickfix": "unknown"
}
1 change: 1 addition & 0 deletions analyzers/rspec/cs/Sonar_way_profile.json
Expand Up @@ -157,6 +157,7 @@
"S3358",
"S3376",
"S3397",
"S3398",
"S3400",
"S3415",
"S3427",
Expand Down
@@ -0,0 +1,126 @@
/*
* SonarAnalyzer for .NET
* Copyright (C) 2015-2023 SonarSource SA
* mailto: contact AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

namespace SonarAnalyzer.Rules.CSharp;

[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class PrivateStaticMethodUsedOnlyByNestedClass : SonarDiagnosticAnalyzer
{
private const string DiagnosticId = "S3398";
private const string MessageFormat = "Move the method inside '{1}'.";

private static readonly SyntaxKind[] AnalyzedSyntaxKinds = new[]
{
SyntaxKind.ClassDeclaration,
SyntaxKind.StructDeclaration,
SyntaxKind.InterfaceDeclaration,
SyntaxKindEx.RecordClassDeclaration,
SyntaxKindEx.RecordStructDeclaration
};

private static readonly DiagnosticDescriptor Rule = DescriptorFactory.Create(DiagnosticId, MessageFormat);

public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(Rule);

protected override void Initialize(SonarAnalysisContext context) =>
context.RegisterNodeAction(c =>
{
var declaredType = (TypeDeclarationSyntax)c.Node;
andrei-epure-sonarsource marked this conversation as resolved.
Show resolved Hide resolved

if (!IsPartial(declaredType)
&& NestedTypeDeclarationsOf(declaredType) is { Length: > 0 } nestedTypes
&& PrivateStaticMethodsOf(declaredType) is { Length: > 0 } candidates)
{
var references = PotentialReferencesOfMethodsInsideType(candidates, declaredType)
.Where(x => x.Value.Any(type => type.Item2 != declaredType))
.Select(x => (x.Key, c.SemanticModel.GetSymbolInfo(x.Key), x.Value.Select(t => c.SemanticModel.GetSymbolInfo(t.Item1))));
foreach (var reference in references)
{
var actualMethodReferences = reference.Item3.Where(x => x.Symbol == reference.Item2.Symbol).ToArray();
// scenario 1: used in outer class
// scenario 2: used only in one of the nested classes
// scenario 3: used by multiple nested classes
}

c.ReportIssue(Diagnostic.Create(Rule, declaredType.GetLocation(), "InnerClassName"));
}
},
AnalyzedSyntaxKinds);

private static MethodDeclarationSyntax[] PrivateStaticMethodsOf(TypeDeclarationSyntax type) =>
andrei-epure-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
type.Members
.OfType<MethodDeclarationSyntax>()
.Where(x => IsPrivateAndStatic(x, type))
.ToArray();

private static TypeDeclarationSyntax[] NestedTypeDeclarationsOf(TypeDeclarationSyntax type) =>
type.Members
.OfType<TypeDeclarationSyntax>()
.Where(x => x is ClassDeclarationSyntax or StructDeclarationSyntax or InterfaceDeclarationSyntax
|| RecordDeclarationSyntaxWrapper.IsInstance(x))
.ToArray();

private static bool IsPrivateAndStatic(MethodDeclarationSyntax method, TypeDeclarationSyntax containingType) =>
method.Modifiers.Any(x => x.IsKind(SyntaxKind.StaticKeyword))
&& ((HasAnyModifier(method, SyntaxKind.PrivateKeyword) && !HasAnyModifier(method, SyntaxKind.ProtectedKeyword))
|| (!HasAnyModifier(method, SyntaxKind.PublicKeyword, SyntaxKind.ProtectedKeyword, SyntaxKind.InternalKeyword) && IsClassOrRecordClassOrInterfaceDeclaration(containingType)));
andrei-epure-sonarsource marked this conversation as resolved.
Show resolved Hide resolved

private static bool IsPartial(TypeDeclarationSyntax type) =>
type.Modifiers.Any(x => x.IsKind(SyntaxKind.PartialKeyword));

private static bool IsClassOrRecordClassOrInterfaceDeclaration(TypeDeclarationSyntax type) =>
type is ClassDeclarationSyntax or InterfaceDeclarationSyntax
|| (RecordDeclarationSyntaxWrapper.IsInstance(type) && !((RecordDeclarationSyntaxWrapper)type).ClassOrStructKeyword.IsKind(SyntaxKind.StructKeyword));
andrei-epure-sonarsource marked this conversation as resolved.
Show resolved Hide resolved

private static bool HasAnyModifier(MethodDeclarationSyntax method, params SyntaxKind[] modifiers) =>
method.Modifiers.Any(x => x.IsAnyKind(modifiers));

private static IDictionary<MethodDeclarationSyntax, List<(IdentifierNameSyntax, TypeDeclarationSyntax)>> PotentialReferencesOfMethodsInsideType(IEnumerable<MethodDeclarationSyntax> methods, TypeDeclarationSyntax type)
{
var collector = new PotentialMethodReferenceCollector(methods);
collector.Visit(type);
return collector.PotentialMethodReferences;
}

private class PotentialMethodReferenceCollector : CSharpSyntaxWalker
{
private readonly ISet<MethodDeclarationSyntax> methodsToFind;
internal Dictionary<MethodDeclarationSyntax, List<(IdentifierNameSyntax, TypeDeclarationSyntax)>> PotentialMethodReferences { get; } = new();

public PotentialMethodReferenceCollector(IEnumerable<MethodDeclarationSyntax> methodsToFind)
{
this.methodsToFind = new HashSet<MethodDeclarationSyntax>(methodsToFind);
}

public override void VisitIdentifierName(IdentifierNameSyntax identifier)
{
if (methodsToFind.FirstOrDefault(x => x.Identifier.ValueText == identifier.Identifier.ValueText) is { } method)
{
var referenceList = PotentialMethodReferences.GetOrAdd(method, _ => new());
referenceList.Add((identifier, ContainingTypeDeclaration(identifier)));
}

static TypeDeclarationSyntax ContainingTypeDeclaration(IdentifierNameSyntax identifier)
{
return identifier.Ancestors().OfType<TypeDeclarationSyntax>().First();
}
}
}
}
Expand Up @@ -3322,7 +3322,7 @@ internal static class RuleTypeMappingCS
// ["S3395"],
// ["S3396"],
["S3397"] = "BUG",
// ["S3398"],
["S3398"] = "CODE_SMELL",
// ["S3399"],
["S3400"] = "CODE_SMELL",
// ["S3401"],
Expand Down
@@ -0,0 +1,60 @@
/*
* SonarAnalyzer for .NET
* Copyright (C) 2015-2023 SonarSource SA
* mailto: contact AT sonarsource DOT com
*
* This program is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using SonarAnalyzer.Rules.CSharp;

namespace SonarAnalyzer.UnitTest.Rules;

[TestClass]
public class PrivateStaticMethodUsedOnlyByNestedClassTest
{
private readonly VerifierBuilder builder = new VerifierBuilder<PrivateStaticMethodUsedOnlyByNestedClass>();

[TestMethod]
public void PrivateStaticMethodUsedOnlyByNestedClass_CS() =>
builder
.AddPaths("PrivateStaticMethodUsedOnlyByNestedClass.cs")
.Verify();

#if NET

[TestMethod]
public void PrivateStaticMethodUsedOnlyByNestedClass_CSharp8() =>
builder
.AddPaths("PrivateStaticMethodUsedOnlyByNestedClass.CSharp8.cs")
.WithOptions(ParseOptionsHelper.FromCSharp8)
.Verify();

#endif

[TestMethod]
public void PrivateStaticMethodUsedOnlyByNestedClass_CSharp9() =>
builder
.AddPaths("PrivateStaticMethodUsedOnlyByNestedClass.CSharp9.cs")
.WithOptions(ParseOptionsHelper.FromCSharp9)
.Verify();

[TestMethod]
public void PrivateStaticMethodUsedOnlyByNestedClass_CSharp10() =>
builder
.AddPaths("PrivateStaticMethodUsedOnlyByNestedClass.CSharp10.cs")
.WithOptions(ParseOptionsHelper.FromCSharp10)
.Verify();
}
@@ -0,0 +1,43 @@
record class OuterRecordClass
{
static void UsedOnlyByNestedClass() { } // Noncompliant
static void UsedOnlyByNestedRecord() { } // Noncompliant

class NestedClass
{
void Foo()
{
UsedOnlyByNestedClass();
}
}

record class NestedRecord
{
void Foo()
{
UsedOnlyByNestedRecord();
}
}
}

record struct OuterRecordStruct
{
private static void UsedOnlyByNestedClass() { } // Noncompliant
private static void UsedOnlyByNestedRecord() { } // Noncompliant

class NestedClass
{
void Foo()
{
UsedOnlyByNestedClass();
}
}

record struct NestedRecord
{
void Foo()
{
UsedOnlyByNestedRecord();
}
}
}
@@ -0,0 +1,27 @@
class OuterClass
{
static void OnlyUsedByNestedInterface() { } // Noncompliant
private protected static void PrivateProtectedMethod() { } // Compliant - method is not private

interface INestedInterface
{
void Foo()
{
OnlyUsedByNestedInterface();
PrivateProtectedMethod();
}
}
}

interface IOuterInterface
{
static void OnlyUsedByNestedClass() { } // Noncompliant

class NestedClass
{
void Foo()
{
OnlyUsedByNestedClass();
}
}
}
@@ -0,0 +1,21 @@
record OuterRecord
{
static void UsedOnlyByNestedClass() { } // Noncompliant
static void UsedOnlyByNestedRecord() { } // Noncompliant

class NestedClass
{
void Foo()
{
UsedOnlyByNestedClass();
}
}

record NestedRecord
{
void Foo()
{
UsedOnlyByNestedRecord();
}
}
}