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

S1186: also inspect empty set and init and empty local functions #8584

Merged
merged 4 commits into from
Jan 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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,41 +68,32 @@ void ReportIfAny(List<Location> nullOrDefaultLiterals)
}
}

private static BlockSyntax GetBody(SyntaxNode node) =>

Choose a reason for hiding this comment

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

Can't we remove this method and use the SyntaxExtensions?

Copy link
Member

Choose a reason for hiding this comment

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

Discussed offline, this always returns the body of the get accessor. We would have to return an array instead of a single BlockSyntax.

node is PropertyDeclarationSyntax property ? GetAccessor(property)?.Body : node.GetBody();

private static bool IsReturningCollection(SonarSyntaxNodeReportingContext context) =>
GetType(context) is { } type
&& !type.Is(KnownType.System_String)
&& !type.DerivesFrom(KnownType.System_Xml_XmlNode)
&& type.DerivesOrImplementsAny(CollectionTypes)
&& type.NullableAnnotation() != NullableAnnotation.Annotated;

private static ITypeSymbol GetType(SonarSyntaxNodeReportingContext context) =>
context.SemanticModel.GetDeclaredSymbol(context.Node) switch
{
IPropertySymbol property => property.Type,
IMethodSymbol method => method.ReturnType,
_ => null,
};
private static ITypeSymbol GetType(SonarSyntaxNodeReportingContext context)
{
var symbol = context.SemanticModel.GetDeclaredSymbol(context.Node);
return symbol is IPropertySymbol property ? property.Type : ((IMethodSymbol)symbol).ReturnType;
}

private static ArrowExpressionClauseSyntax GetExpressionBody(SyntaxNode node) =>

Choose a reason for hiding this comment

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

Wouldn't be better to move this to SyntaxNodeExtensions?

Choose a reason for hiding this comment

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

Similar with above.

node switch
{
BaseMethodDeclarationSyntax method => method.ExpressionBody(),
PropertyDeclarationSyntax property => property.ExpressionBody ?? GetAccessor(property)?.ExpressionBody(),
var _ when LocalFunctionStatementSyntaxWrapper.IsInstance(node) => ((LocalFunctionStatementSyntaxWrapper)node).ExpressionBody,
_ => 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,
BaseMethodDeclarationSyntax method => method.ExpressionBody(),
PropertyDeclarationSyntax property => property.ExpressionBody ?? GetAccessor(property)?.ExpressionBody(),
_ => ((LocalFunctionStatementSyntaxWrapper)node).ExpressionBody,
};

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

Choose a reason for hiding this comment

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

While the grammar defines the accessor_list production rule as optional, and an alternative to ((arrow_expression_clause | equals_value_clause) ';')), the AccessorList results non-null even on arrow expressions.
Since I can't produce any example of a property where AccessorList ends up being null (not even with an incomplete code example), I am removing the null propagation operator.


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 @@ -26,3 +26,10 @@ public unsafe partial class Externals
[System.Runtime.InteropServices.DllImportAttribute("ole32.dll", EntryPoint = "F", ExactSpelling = true)]
private static extern partial void F(); // Compliant
}

abstract class WithModifiers
{
public unsafe required int X { set { } } // Noncompliant
public virtual int Y { get => 42; private set { } } // Noncompliant
public abstract int Z { get; private protected set; } // Compliant: no body
}
Loading
Loading