From fb685ddc088e7883bb1175e86a43ec93d976e43c Mon Sep 17 00:00:00 2001 From: Antonio Aversa Date: Tue, 23 Jan 2024 16:01:53 +0100 Subject: [PATCH 1/4] S1186: also inspect empty set and init --- .../Extensions/SyntaxNodeExtensions.cs | 18 ++++++ .../AbstractTypesShouldNotHaveConstructors.cs | 13 +--- .../SonarAnalyzer.CSharp/Rules/EmptyMethod.cs | 57 +++++++++-------- .../Rules/EmptyMethodCodeFix.cs | 33 ++++------ .../ReturnEmptyCollectionInsteadOfNull.cs | 33 ++++------ .../Rules/EmptyMethodTest.cs | 1 + .../EmptyMethod.CSharp9.Comment.Fixed.cs | 64 ++++++++++++++++--- .../EmptyMethod.CSharp9.Throw.Fixed.cs | 36 ++++++++--- .../TestCases/EmptyMethod.CSharp9.cs | 38 ++++++++--- .../TestCases/EmptyMethod.Comment.Fixed.cs | 10 +-- .../TestCases/EmptyMethod.Throw.Fixed.cs | 10 +-- .../TestCases/EmptyMethod.cs | 5 +- 12 files changed, 201 insertions(+), 117 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Extensions/SyntaxNodeExtensions.cs b/analyzers/src/SonarAnalyzer.CSharp/Extensions/SyntaxNodeExtensions.cs index f9c67fbed2e..e49abc905ff 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Extensions/SyntaxNodeExtensions.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Extensions/SyntaxNodeExtensions.cs @@ -19,6 +19,7 @@ */ using SonarAnalyzer.CFG.Roslyn; +using StyleCop.Analyzers.Lightup; namespace SonarAnalyzer.Extensions { @@ -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 { @@ -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 { diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/AbstractTypesShouldNotHaveConstructors.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/AbstractTypesShouldNotHaveConstructors.cs index 545050b970a..546858c21fe 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/AbstractTypesShouldNotHaveConstructors.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/AbstractTypesShouldNotHaveConstructors.cs @@ -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))); @@ -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); diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/EmptyMethod.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/EmptyMethod.cs index 2ad5f65883f..bf03da11f94 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/EmptyMethod.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/EmptyMethod.cs @@ -23,42 +23,43 @@ namespace SonarAnalyzer.Rules.CSharp [DiagnosticAnalyzer(LanguageNames.CSharp)] public sealed class EmptyMethod : EmptyMethodBase { + internal static readonly SyntaxKind[] SupportedSyntaxKinds = + { + SyntaxKind.MethodDeclaration, + SyntaxKindEx.LocalFunctionStatement, + SyntaxKind.SetAccessorDeclaration, + SyntaxKindEx.InitAccessorDeclaration + }; + protected override ILanguageFacade 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 + }; } } diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/EmptyMethodCodeFix.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/EmptyMethodCodeFix.cs index 416a7c709ff..01dbcba9a77 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/EmptyMethodCodeFix.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/EmptyMethodCodeFix.cs @@ -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(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(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) @@ -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); @@ -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() - .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 }); } } diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/ReturnEmptyCollectionInsteadOfNull.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/ReturnEmptyCollectionInsteadOfNull.cs index 32c6d923276..dbc808e956f 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/ReturnEmptyCollectionInsteadOfNull.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/ReturnEmptyCollectionInsteadOfNull.cs @@ -68,6 +68,9 @@ void ReportIfAny(List nullOrDefaultLiterals) } } + private static BlockSyntax GetBody(SyntaxNode node) => + node is PropertyDeclarationSyntax property ? GetAccessor(property)?.Body : node.GetBody(); + private static bool IsReturningCollection(SonarSyntaxNodeReportingContext context) => GetType(context) is { } type && !type.Is(KnownType.System_String) @@ -75,34 +78,22 @@ private static bool IsReturningCollection(SonarSyntaxNodeReportingContext contex && 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) => - 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)); private static IEnumerable GetReturnNullOrDefaultExpressions(SyntaxNode methodBlock) => methodBlock.DescendantNodes(n => diff --git a/analyzers/tests/SonarAnalyzer.Test/Rules/EmptyMethodTest.cs b/analyzers/tests/SonarAnalyzer.Test/Rules/EmptyMethodTest.cs index 8e7be3a9988..3bf2d7c846a 100644 --- a/analyzers/tests/SonarAnalyzer.Test/Rules/EmptyMethodTest.cs +++ b/analyzers/tests/SonarAnalyzer.Test/Rules/EmptyMethodTest.cs @@ -42,6 +42,7 @@ public void EmptyMethod() => public void EmptyMethod_CSharp9() => builderCS.AddPaths("EmptyMethod.CSharp9.cs") .WithTopLevelStatements() + .WithOptions(ParseOptionsHelper.FromCSharp9) .Verify(); [TestMethod] diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp9.Comment.Fixed.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp9.Comment.Fixed.cs index 20fbae9b0ce..52b477f484c 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp9.Comment.Fixed.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp9.Comment.Fixed.cs @@ -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] @@ -133,3 +125,59 @@ public partial void Qix() } } } + +class PropertyAccessors +{ + int NonEmptyInitProp { init { int x; } } + 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 NonEmptySetProp { set { int x; } } + 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 + } +} diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp9.Throw.Fixed.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp9.Throw.Fixed.cs index e085f88c6dc..467e9eb03f3 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp9.Throw.Fixed.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp9.Throw.Fixed.cs @@ -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] @@ -130,3 +122,31 @@ public partial void Qix() } } } + +class PropertyAccessors +{ + int NonEmptyInitProp { init { int x; } } + 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 NonEmptySetProp { set { int x; } } + 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 + } +} diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp9.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp9.cs index 10f4e9c49ae..39deda29442 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp9.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp9.cs @@ -70,14 +70,6 @@ public override void F2() } } -class WithProp -{ - public string Prop - { - init { } // FN https://github.com/SonarSource/sonar-dotnet/issues/3753 - } -} - class M { [ModuleInitializer] @@ -122,3 +114,33 @@ public partial void Qix() } } } + +class PropertyAccessors +{ + int NonEmptyInitProp { init { int x; } } + int EmptyInitProp { init { } } // Noncompliant + int EmptyInitPropWithGet { get => 42; init { } } // Noncompliant + // ^^^^ + int AutoInitPropWithGet { get; init; } // Compliant, auto-implemented, so not-empty + + int NonEmptySetProp { set { int x; } } + int EmptySetProp { set { } } // Noncompliant + int EmptySetPropWithGet { get => 42; set { } } // Noncompliant + // ^^^ + 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 { } } // Noncompliant + } + + class Hidden : Base + { + protected new int VirtualEmptyInitProp { init { } } // Noncompliant + } +} diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.Comment.Fixed.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.Comment.Fixed.cs index f1072140fcd..5f387eff10a 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.Comment.Fixed.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.Comment.Fixed.cs @@ -68,10 +68,7 @@ public override void F2() public interface IInterface { - public void F1() - { - // Method intentionally left empty. - } // Fixed + public void F1() { } // Compliant, implemented interface methods are virtual by default public virtual void F2() { } @@ -82,7 +79,10 @@ public class WithProp { public string Prop { - set { } // FN https://github.com/SonarSource/sonar-dotnet/issues/3753 + set + { + // Method intentionally left empty. + } // Fixed } } diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.Throw.Fixed.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.Throw.Fixed.cs index d1d8c542104..eead40436ad 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.Throw.Fixed.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.Throw.Fixed.cs @@ -72,10 +72,7 @@ public override void F2() public interface IInterface { - public void F1() - { - throw new NotSupportedException(); - } // Fixed + public void F1() { } // Compliant, implemented interface methods are virtual by default public virtual void F2() { } @@ -86,7 +83,10 @@ public class WithProp { public string Prop { - set { } // FN https://github.com/SonarSource/sonar-dotnet/issues/3753 + set + { + throw new NotSupportedException(); + } // Fixed } } diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.cs index 999c31faa26..86bfdd20e97 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.cs @@ -67,8 +67,7 @@ public override void F2() public interface IInterface { - public void F1() { } // Noncompliant -// ^^ + public void F1() { } // Compliant, implemented interface methods are virtual by default public virtual void F2() { } @@ -79,7 +78,7 @@ public class WithProp { public string Prop { - set { } // FN https://github.com/SonarSource/sonar-dotnet/issues/3753 + set { } // Noncompliant } } From 600bd12c6e87cd882dd6564bf4187a11e3e8ad2f Mon Sep 17 00:00:00 2001 From: Antonio Aversa Date: Wed, 24 Jan 2024 17:16:15 +0100 Subject: [PATCH 2/4] Tests for local functions --- .../TestCases/EmptyMethod.CSharp11.cs | 7 ++ .../EmptyMethod.CSharp9.Comment.Fixed.cs | 100 ++++++++++++++++++ .../EmptyMethod.CSharp9.Throw.Fixed.cs | 64 +++++++++++ .../TestCases/EmptyMethod.CSharp9.cs | 64 +++++++++++ 4 files changed, 235 insertions(+) diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp11.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp11.cs index 540590403ea..53d094e380c 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp11.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp11.cs @@ -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 +} diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp9.Comment.Fixed.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp9.Comment.Fixed.cs index 52b477f484c..dc5aa99fc9d 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp9.Comment.Fixed.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp9.Comment.Fixed.cs @@ -181,3 +181,103 @@ class Hidden : Base } // Fixed } } + +class EmptyProperty +{ + int EmptyProp { } // Error CS0548 property or indexer must have at least one accessor +} + +class LocalFunction +{ + void FirstLevelInMethod() + { + void NonEmpty() { int i; } // Compliant + void Empty() + { + // Method intentionally left empty. + } // Fixed + static void EmptyStatic() + { + // Method intentionally left empty. + } // Fixed + extern static void EmptyExternStatic(); // Compliant, no body + unsafe void EmptyUnsafe() + { + // Method intentionally left empty. + } // Fixed + async void EmptyAsync() + { + // Method intentionally left empty. + } // Fixed + } + + void NestedInMethod() + { + void FirstLevelLocalFunction() + { + void NonEmpty() { int i; } // Compliant + void Empty() + { + // Method intentionally left empty. + } // Fixed + + void SecondLevelLocalFunction() // Compliant, contains a local functions + { + void NonEmpty() { int i; } // Compliant + void Empty() + { + // Method intentionally left empty. + } // Fixed + } + } + } + + int FirstLevelInAccessor + { + set + { + void NonEmpty() { int i; } // Compliant + void Empty() + { + // Method intentionally left empty. + } // Fixed + static void EmptyStatic() + { + // Method intentionally left empty. + } // Fixed + extern static void EmptyExternStatic(); // Compliant, no body + unsafe void EmptyUnsafe() + { + // Method intentionally left empty. + } // Fixed + async void EmptyAsync() + { + // Method intentionally left empty. + } // Fixed + } + } + + int NestedInAccessor + { + init + { + void FirstLevelLocalFunction() + { + void NonEmpty() { int i; } // Compliant + void Empty() + { + // Method intentionally left empty. + } // Fixed + + void SecondLevelLocalFunction() // Compliant, contains local functions + { + void NonEmpty() { int i; } // Compliant + void Empty() + { + // Method intentionally left empty. + } // Fixed + } + } + } + } +} diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp9.Throw.Fixed.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp9.Throw.Fixed.cs index 467e9eb03f3..35ac3afcff4 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp9.Throw.Fixed.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp9.Throw.Fixed.cs @@ -150,3 +150,67 @@ class Hidden : Base protected new int VirtualEmptyInitProp { init { throw new NotSupportedException(); } } // Fixed } } + +class EmptyProperty +{ + int EmptyProp { } // Error CS0548 property or indexer must have at least one accessor +} + +class LocalFunction +{ + void FirstLevelInMethod() + { + void NonEmpty() { int i; } // Compliant + void Empty() { throw new NotSupportedException(); } // Fixed + static void EmptyStatic() { throw new NotSupportedException(); } // Fixed + extern static void EmptyExternStatic(); // Compliant, no body + unsafe void EmptyUnsafe() { throw new NotSupportedException(); } // Fixed + async void EmptyAsync() { throw new NotSupportedException(); } // Fixed + } + + void NestedInMethod() + { + void FirstLevelLocalFunction() + { + void NonEmpty() { int i; } // Compliant + void Empty() { throw new NotSupportedException(); } // Fixed + + void SecondLevelLocalFunction() // Compliant, contains a local functions + { + void NonEmpty() { int i; } // Compliant + void Empty() { throw new NotSupportedException(); } // Fixed + } + } + } + + int FirstLevelInAccessor + { + set + { + void NonEmpty() { int i; } // Compliant + void Empty() { throw new NotSupportedException(); } // Fixed + static void EmptyStatic() { throw new NotSupportedException(); } // Fixed + extern static void EmptyExternStatic(); // Compliant, no body + unsafe void EmptyUnsafe() { throw new NotSupportedException(); } // Fixed + async void EmptyAsync() { throw new NotSupportedException(); } // Fixed + } + } + + int NestedInAccessor + { + init + { + void FirstLevelLocalFunction() + { + void NonEmpty() { int i; } // Compliant + void Empty() { throw new NotSupportedException(); } // Fixed + + void SecondLevelLocalFunction() // Compliant, contains local functions + { + void NonEmpty() { int i; } // Compliant + void Empty() { throw new NotSupportedException(); } // Fixed + } + } + } + } +} diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp9.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp9.cs index 39deda29442..be71f36c3ed 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp9.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/EmptyMethod.CSharp9.cs @@ -144,3 +144,67 @@ class Hidden : Base protected new int VirtualEmptyInitProp { init { } } // Noncompliant } } + +class EmptyProperty +{ + int EmptyProp { } // Error CS0548 property or indexer must have at least one accessor +} + +class LocalFunction +{ + void FirstLevelInMethod() + { + void NonEmpty() { int i; } // Compliant + void Empty() { } // Noncompliant + static void EmptyStatic() { } // Noncompliant + extern static void EmptyExternStatic(); // Compliant, no body + unsafe void EmptyUnsafe() { } // Noncompliant + async void EmptyAsync() { } // Noncompliant + } + + void NestedInMethod() + { + void FirstLevelLocalFunction() + { + void NonEmpty() { int i; } // Compliant + void Empty() { } // Noncompliant + + void SecondLevelLocalFunction() // Compliant, contains a local functions + { + void NonEmpty() { int i; } // Compliant + void Empty() { } // Noncompliant + } + } + } + + int FirstLevelInAccessor + { + set + { + void NonEmpty() { int i; } // Compliant + void Empty() { } // Noncompliant + static void EmptyStatic() { } // Noncompliant + extern static void EmptyExternStatic(); // Compliant, no body + unsafe void EmptyUnsafe() { } // Noncompliant + async void EmptyAsync() { } // Noncompliant + } + } + + int NestedInAccessor + { + init + { + void FirstLevelLocalFunction() + { + void NonEmpty() { int i; } // Compliant + void Empty() { } // Noncompliant + + void SecondLevelLocalFunction() // Compliant, contains local functions + { + void NonEmpty() { int i; } // Compliant + void Empty() { } // Noncompliant + } + } + } + } +} From c7fc2c98d37e48775f79ee2417686a138a56e172 Mon Sep 17 00:00:00 2001 From: Antonio Aversa Date: Thu, 25 Jan 2024 15:22:35 +0100 Subject: [PATCH 3/4] Improve coverage --- .../Rules/ReturnEmptyCollectionInsteadOfNull.cs | 2 +- .../TestCases/ReturnEmptyCollectionInsteadOfNull.cs | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/ReturnEmptyCollectionInsteadOfNull.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/ReturnEmptyCollectionInsteadOfNull.cs index dbc808e956f..e9749fa54ba 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/ReturnEmptyCollectionInsteadOfNull.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/ReturnEmptyCollectionInsteadOfNull.cs @@ -93,7 +93,7 @@ private static ArrowExpressionClauseSyntax GetExpressionBody(SyntaxNode node) => }; private static AccessorDeclarationSyntax GetAccessor(PropertyDeclarationSyntax property) => - property.AccessorList?.Accessors.FirstOrDefault(x => x.IsKind(SyntaxKind.GetAccessorDeclaration)); + property.AccessorList.Accessors.FirstOrDefault(x => x.IsKind(SyntaxKind.GetAccessorDeclaration)); private static IEnumerable GetReturnNullOrDefaultExpressions(SyntaxNode methodBlock) => methodBlock.DescendantNodes(n => diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/ReturnEmptyCollectionInsteadOfNull.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/ReturnEmptyCollectionInsteadOfNull.cs index 9b776223c58..6a615b49537 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/ReturnEmptyCollectionInsteadOfNull.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/ReturnEmptyCollectionInsteadOfNull.cs @@ -29,6 +29,8 @@ IEnumerable Strings3 } } + IEnumerable PropertyNoGetter { set { } } + IEnumerable ArrowedGetStrings1() => null; // Noncompliant // ^^^^ IList ArrowedGetStrings2 => null; // Noncompliant From 1ae9e627d17b10e5a7c21c11fd5046c1e2cdd16d Mon Sep 17 00:00:00 2001 From: Antonio Aversa Date: Thu, 25 Jan 2024 16:00:15 +0100 Subject: [PATCH 4/4] Add incomplete property --- .../TestCases/ReturnEmptyCollectionInsteadOfNull.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/ReturnEmptyCollectionInsteadOfNull.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/ReturnEmptyCollectionInsteadOfNull.cs index 6a615b49537..aa56c382771 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/ReturnEmptyCollectionInsteadOfNull.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/ReturnEmptyCollectionInsteadOfNull.cs @@ -182,6 +182,11 @@ List NoncompliantMethodWithSecondaryLocation(string str) } } +class PropertyWithoutAccessorList +{ + IEnumerable Incomplete => ; // Error CS1525: Invalid expression term +} + // Reproducer for #6494 https://github.com/SonarSource/sonar-dotnet/issues/6494 namespace Issue_6494 {