Skip to content

Commit

Permalink
S1186: also inspect empty set and init and empty local functions
Browse files Browse the repository at this point in the history
  • Loading branch information
antonioaversa committed Jan 23, 2024
1 parent de6f22f commit f508ae5
Show file tree
Hide file tree
Showing 12 changed files with 192 additions and 106 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
*/

using SonarAnalyzer.CFG.Roslyn;
using StyleCop.Analyzers.Lightup;

namespace SonarAnalyzer.Extensions
{
Expand Down Expand Up @@ -488,6 +489,15 @@ private static string GetUnknownType(SyntaxKind kind) =>

#endif

public static BlockSyntax GetBody(this SyntaxNode node) =>
node switch
{
BaseMethodDeclarationSyntax method => method.Body,
AccessorDeclarationSyntax accessor => accessor.Body,
_ when LocalFunctionStatementSyntaxWrapper.IsInstance(node) => ((LocalFunctionStatementSyntaxWrapper)node).Body,
_ => null,
};

public static SyntaxNode GetInitializer(this SyntaxNode node) =>
node switch
{
Expand All @@ -496,6 +506,14 @@ public static SyntaxNode GetInitializer(this SyntaxNode node) =>
_ => null
};

public static SyntaxTokenList GetModifiers(this SyntaxNode? node) =>
node switch
{
AccessorDeclarationSyntax accessor => accessor.Modifiers,
MemberDeclarationSyntax member => member.Modifiers(),
_ => default,
};

public static bool IsTrue(this SyntaxNode node) =>
node switch
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,9 @@ protected override void Initialize(SonarAnalysisContext context) =>
context.RegisterNodeAction(
c =>
{
if (GetModifiers(c.Node.Parent).Any(SyntaxKind.AbstractKeyword))
if (c.Node.Parent.GetModifiers().Any(SyntaxKind.AbstractKeyword))
{
var invalidAccessModifier = GetModifiers(c.Node).FirstOrDefault(IsPublicOrInternal);
var invalidAccessModifier = c.Node.GetModifiers().FirstOrDefault(IsPublicOrInternal);
if (invalidAccessModifier != default)
{
c.ReportIssue(Diagnostic.Create(Rule, invalidAccessModifier.GetLocation(), SuggestModifier(invalidAccessModifier)));
Expand All @@ -45,15 +45,6 @@ protected override void Initialize(SonarAnalysisContext context) =>
},
SyntaxKind.ConstructorDeclaration);

private static SyntaxTokenList GetModifiers(SyntaxNode node) =>
node switch
{
ClassDeclarationSyntax classDeclaration => classDeclaration.Modifiers,
ConstructorDeclarationSyntax ctorDeclaration => ctorDeclaration.Modifiers,
{} syntaxNode when RecordDeclarationSyntaxWrapper.IsInstance(syntaxNode) => ((RecordDeclarationSyntaxWrapper)syntaxNode).Modifiers,
_ => default
};

private static bool IsPublicOrInternal(SyntaxToken token) =>
token.IsKind(SyntaxKind.PublicKeyword) || token.IsKind(SyntaxKind.InternalKeyword);

Expand Down
57 changes: 29 additions & 28 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/EmptyMethod.cs
Original file line number Diff line number Diff line change
Expand Up @@ -23,42 +23,43 @@ namespace SonarAnalyzer.Rules.CSharp
[DiagnosticAnalyzer(LanguageNames.CSharp)]
public sealed class EmptyMethod : EmptyMethodBase<SyntaxKind>
{
internal static readonly SyntaxKind[] SupportedSyntaxKinds =
{
SyntaxKind.MethodDeclaration,
SyntaxKindEx.LocalFunctionStatement,
SyntaxKind.SetAccessorDeclaration,
SyntaxKindEx.InitAccessorDeclaration
};

protected override ILanguageFacade<SyntaxKind> Language => CSharpFacade.Instance;

protected override SyntaxKind[] SyntaxKinds { get; } =
{
SyntaxKind.MethodDeclaration,
SyntaxKindEx.LocalFunctionStatement
};
protected override SyntaxKind[] SyntaxKinds => SupportedSyntaxKinds;

protected override void CheckMethod(SonarSyntaxNodeReportingContext context)
{
if (LocalFunctionStatementSyntaxWrapper.IsInstance(context.Node))
{
var wrapper = (LocalFunctionStatementSyntaxWrapper)context.Node;
if (wrapper.Body is { } body && body.IsEmpty())
{
context.ReportIssue(Diagnostic.Create(Rule, wrapper.Identifier.GetLocation()));
}
}
else
// No need to check for ExpressionBody as arrowed methods can't be empty
if (context.Node.GetBody() is { } body
&& body.IsEmpty()
&& !ShouldBeExcluded(context, context.Node, context.Node.GetModifiers()))
{
var methodDeclaration = (MethodDeclarationSyntax)context.Node;

// No need to check for ExpressionBody as arrowed methods can't be empty
if (methodDeclaration.Body is { } body
&& body.IsEmpty()
&& !ShouldMethodBeExcluded(context, methodDeclaration))
{
context.ReportIssue(Diagnostic.Create(Rule, methodDeclaration.Identifier.GetLocation()));
}
context.ReportIssue(Diagnostic.Create(Rule, ReportingToken(context.Node).GetLocation()));
}
}

private static bool ShouldMethodBeExcluded(SonarSyntaxNodeReportingContext context, BaseMethodDeclarationSyntax methodNode) =>
methodNode.Modifiers.Any(SyntaxKind.VirtualKeyword)
|| (context.SemanticModel.GetDeclaredSymbol(methodNode) is var symbol
&& (symbol is { IsOverride: true, OverriddenMethod.IsAbstract: true } || !symbol.ExplicitOrImplicitInterfaceImplementations().IsEmpty))
|| (methodNode.Modifiers.Any(SyntaxKind.OverrideKeyword) && context.IsTestProject());
private static bool ShouldBeExcluded(SonarSyntaxNodeReportingContext context, SyntaxNode node, SyntaxTokenList modifiers) =>
modifiers.Any(SyntaxKind.VirtualKeyword) // This quick check only works for methods, for accessors we need to check the symbol
|| (context.SemanticModel.GetDeclaredSymbol(node) is IMethodSymbol symbol
&& (symbol is { IsVirtual: true }
|| symbol is { IsOverride: true, OverriddenMethod.IsAbstract: true }
|| !symbol.ExplicitOrImplicitInterfaceImplementations().IsEmpty))
|| (modifiers.Any(SyntaxKind.OverrideKeyword) && context.IsTestProject());

private static SyntaxToken ReportingToken(SyntaxNode node) =>
node switch
{
MethodDeclarationSyntax method => method.Identifier,
AccessorDeclarationSyntax accessor => accessor.Keyword,
_ => ((LocalFunctionStatementSyntaxWrapper)node).Identifier
};
}
}
33 changes: 13 additions & 20 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/EmptyMethodCodeFix.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,20 +36,12 @@ public sealed class EmptyMethodCodeFix : SonarCodeFix

protected override async Task RegisterCodeFixesAsync(SyntaxNode root, SonarCodeFixContext context)
{
var diagnostic = context.Diagnostics.First();
var diagnosticSpan = diagnostic.Location.SourceSpan;
var syntaxNode = root.FindNode(diagnosticSpan, getInnermostNodeForTie: true);
var method = syntaxNode.FirstAncestorOrSelf<SyntaxNode>(x => x.IsAnyKind(SyntaxKind.MethodDeclaration, SyntaxKindEx.LocalFunctionStatement));
var methodBody = method.IsKind(SyntaxKind.MethodDeclaration)
? ((BaseMethodDeclarationSyntax)method).Body
: ((LocalFunctionStatementSyntaxWrapper)method).Body;

if (methodBody.CloseBraceToken.IsMissing || methodBody.OpenBraceToken.IsMissing)
if (root.FindNode(context.Diagnostics.First().Location.SourceSpan, getInnermostNodeForTie: true)
.FirstAncestorOrSelf<SyntaxNode>(x => x.IsAnyKind(EmptyMethod.SupportedSyntaxKinds))
.GetBody() is { CloseBraceToken.IsMissing: false, OpenBraceToken.IsMissing: false } body)
{
return;
await RegisterCodeFixesForMethodsAsync(context, root, body).ConfigureAwait(false);
}

await RegisterCodeFixesForMethodsAsync(context, root, methodBody).ConfigureAwait(false);
}

private static async Task RegisterCodeFixesForMethodsAsync(SonarCodeFixContext context, SyntaxNode root, BlockSyntax methodBody)
Expand All @@ -70,9 +62,13 @@ private static async Task RegisterCodeFixesForMethodsAsync(SonarCodeFixContext c
.Add(SyntaxFactory.Comment("// Method intentionally left empty."))
.Add(SyntaxFactory.EndOfLine(Environment.NewLine))));

var newRoot = root.ReplaceNode(
methodBody,
newMethodBody.WithTriviaFrom(methodBody).WithAdditionalAnnotations(Formatter.Annotation));
var newRoot = methodBody.Parent is AccessorDeclarationSyntax accessor
? root.ReplaceNode(
accessor,
accessor.WithBody(newMethodBody.WithTriviaFrom(accessor.Body)).WithAdditionalAnnotations(Formatter.Annotation))
: root.ReplaceNode(
methodBody,
newMethodBody.WithTriviaFrom(methodBody).WithAdditionalAnnotations(Formatter.Annotation));
return Task.FromResult(context.Document.WithSyntaxRoot(newRoot));
},
context.Diagnostics);
Expand Down Expand Up @@ -110,10 +106,7 @@ private static async Task RegisterCodeFixesForMethodsAsync(SonarCodeFixContext c
}

private static bool NamespaceNeedsToBeAdded(BlockSyntax methodBody, SemanticModel semanticModel) =>
!semanticModel.LookupNamespacesAndTypes(methodBody.CloseBraceToken.SpanStart)
.OfType<INamedTypeSymbol>()
.Any(x => x.IsType
&& x.Name == LiteralNotSupportedException
&& x.ContainingNamespace.Name == LiteralSystem);
semanticModel.LookupNamespacesAndTypes(methodBody.CloseBraceToken.SpanStart)
.All(x => x is not INamedTypeSymbol { IsType: true, Name: LiteralNotSupportedException, ContainingNamespace.Name: LiteralSystem });
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,13 @@ void ReportIfAny(List<Location> nullOrDefaultLiterals)
}
}

private static BlockSyntax GetBody(SyntaxNode node) =>
node switch
{
PropertyDeclarationSyntax property => GetAccessor(property)?.Body,
_ => node.GetBody(),
};

private static bool IsReturningCollection(SonarSyntaxNodeReportingContext context) =>
GetType(context) is { } type
&& !type.Is(KnownType.System_String)
Expand All @@ -92,17 +99,8 @@ var _ when LocalFunctionStatementSyntaxWrapper.IsInstance(node) => ((LocalFuncti
_ => null,
};

private static BlockSyntax GetBody(SyntaxNode node) =>
node switch
{
BaseMethodDeclarationSyntax method => method.Body,
PropertyDeclarationSyntax property => GetAccessor(property)?.Body,
var _ when LocalFunctionStatementSyntaxWrapper.IsInstance(node) => ((LocalFunctionStatementSyntaxWrapper)node).Body,
_ => null,
};

private static AccessorDeclarationSyntax GetAccessor(PropertyDeclarationSyntax property) =>
property.AccessorList?.Accessors.FirstOrDefault(a => a.IsKind(SyntaxKind.GetAccessorDeclaration));
property.AccessorList?.Accessors.FirstOrDefault(x => x.IsKind(SyntaxKind.GetAccessorDeclaration));

private static IEnumerable<SyntaxNode> GetReturnNullOrDefaultExpressions(SyntaxNode methodBlock) =>
methodBlock.DescendantNodes(n =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ public void EmptyMethod() =>
public void EmptyMethod_CSharp9() =>
builderCS.AddPaths("EmptyMethod.CSharp9.cs")
.WithTopLevelStatements()
.WithOptions(ParseOptionsHelper.FromCSharp9)
.Verify();

[TestMethod]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,14 +77,6 @@ public override void F2()
}
}

class WithProp
{
public string Prop
{
init { } // FN https://github.com/SonarSource/sonar-dotnet/issues/3753
}
}

class M
{
[ModuleInitializer]
Expand Down Expand Up @@ -133,3 +125,58 @@ public partial void Qix()
}
}
}

class PropertyAccessors
{
int EmptyInitProp { init
{
// Method intentionally left empty.
}
} // Fixed
int EmptyInitPropWithGet { get => 42;
init
{
// Method intentionally left empty.
}
} // Fixed
int AutoInitPropWithGet { get; init; } // Compliant, auto-implemented, so not-empty

int EmptySetProp {
set
{
// Method intentionally left empty.
}
} // Fixed
int EmptySetPropWithGet { get => 42;
set
{
// Method intentionally left empty.
}
} // Fixed
int AutoSetPropWithGet { get; set; } // Compliant, auto-implemented, so not-empty

class Base
{
protected virtual int VirtualEmptyInitProp { init { } } // Compliant, virtual
}

class Inherited : Base
{
protected override int VirtualEmptyInitProp {
init
{
// Method intentionally left empty.
}
} // Fixed
}

class Hidden : Base
{
protected new int VirtualEmptyInitProp {
init
{
// Method intentionally left empty.
}
} // Fixed
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -74,14 +74,6 @@ public override void F2()
}
}

class WithProp
{
public string Prop
{
init { } // FN https://github.com/SonarSource/sonar-dotnet/issues/3753
}
}

class M
{
[ModuleInitializer]
Expand Down Expand Up @@ -130,3 +122,29 @@ public partial void Qix()
}
}
}

class PropertyAccessors
{
int EmptyInitProp { init { throw new NotSupportedException(); } } // Fixed
int EmptyInitPropWithGet { get => 42; init { throw new NotSupportedException(); } } // Fixed
int AutoInitPropWithGet { get; init; } // Compliant, auto-implemented, so not-empty

int EmptySetProp { set { throw new NotSupportedException(); } } // Fixed
int EmptySetPropWithGet { get => 42; set { throw new NotSupportedException(); } } // Fixed
int AutoSetPropWithGet { get; set; } // Compliant, auto-implemented, so not-empty

class Base
{
protected virtual int VirtualEmptyInitProp { init { } } // Compliant, virtual
}

class Inherited : Base
{
protected override int VirtualEmptyInitProp { init { throw new NotSupportedException(); } } // Fixed
}

class Hidden : Base
{
protected new int VirtualEmptyInitProp { init { throw new NotSupportedException(); } } // Fixed
}
}
Loading

0 comments on commit f508ae5

Please sign in to comment.