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

Fix S2094 FP: Primary constructors for classes should be handled equal to records #9019

Merged
Merged
Show file tree
Hide file tree
Changes from 2 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
35 changes: 27 additions & 8 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/ClassShouldNotBeEmpty.cs
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/

using System.Xml.Linq;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I hate it when Visual Studio adds usings without telling you. It has been removed now.


namespace SonarAnalyzer.Rules.CSharp;

[DiagnosticAnalyzer(LanguageNames.CSharp)]
Expand All @@ -28,7 +30,7 @@ public sealed class ClassShouldNotBeEmpty : ClassShouldNotBeEmptyBase<SyntaxKind
protected override bool IsEmptyAndNotPartial(SyntaxNode node) =>
node is TypeDeclarationSyntax { Members.Count: 0 } typeDeclaration
&& !typeDeclaration.Modifiers.Any(x => x.IsKind(SyntaxKind.PartialKeyword))
&& (node is ClassDeclarationSyntax || IsParameterlessRecord(node));
&& LacksParameterizedPrimaryConstructor(node);

protected override BaseTypeDeclarationSyntax GetIfHasDeclaredBaseClassOrInterface(SyntaxNode node) =>
node is TypeDeclarationSyntax { BaseList: not null } declaration
Expand All @@ -54,19 +56,36 @@ public sealed class ClassShouldNotBeEmpty : ClassShouldNotBeEmptyBase<SyntaxKind
SyntaxKind.ElseDirectiveTrivia,
SyntaxKind.EndIfDirectiveTrivia));

private static bool LacksParameterizedPrimaryConstructor(SyntaxNode node) =>
IsParameterlessClass(node)
|| IsParameterlessRecord(node);

private static bool IsParameterlessClass(SyntaxNode node) =>
ClassDeclarationSyntax(node) is { } declaration
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not just use the ClassDeclarationSyntax type directly? It's available without a wrapper.
The only change that it will need is to replace the declaration.ParameterList property with the declaration.ParameterList() method call.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zsolt-kolbay-sonarsource I thought I tried that, and that it did not work. Anyhow, now it does.

? LacksParameters(declaration.ParameterList, declaration.BaseList)
: node is ClassDeclarationSyntax;

private static bool IsParameterlessRecord(SyntaxNode node) =>
RecordDeclarationSyntax(node) is { } declaration
&& (declaration.ParameterList is null || declaration.ParameterList.Parameters.Count == 0)
&& BaseTypeSyntax(declaration) is not { ArgumentList.Arguments.Count: >= 1 };
&& LacksParameters(declaration.ParameterList, declaration.BaseList);

private static bool LacksParameters(ParameterListSyntax parameterList, BaseListSyntax baseList) =>
parameterList?.Parameters is not { Count: >= 1 }
Copy link
Contributor

Choose a reason for hiding this comment

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

[Optional] Count: > 0 is slightly less of a cognitive load for me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was { Count : >= 1 } at other places in this class, so I kept it the same. Now I updated all here to your suggestion.

&& BaseTypeSyntax(baseList) is not { ArgumentList.Arguments.Count: >= 1 };

private static ClassDeclarationSyntaxWrapper? ClassDeclarationSyntax(SyntaxNode node) =>
ClassDeclarationSyntaxWrapper.IsInstance(node)
? (ClassDeclarationSyntaxWrapper)node
: null;

private static RecordDeclarationSyntaxWrapper? RecordDeclarationSyntax(SyntaxNode node) =>
RecordDeclarationSyntaxWrapper.IsInstance(node)
? (RecordDeclarationSyntaxWrapper)node
: null;

private static PrimaryConstructorBaseTypeSyntaxWrapper? BaseTypeSyntax(RecordDeclarationSyntaxWrapper node) =>
node.BaseList?.Types.FirstOrDefault() is { } type
&& PrimaryConstructorBaseTypeSyntaxWrapper.IsInstance(type)
? (PrimaryConstructorBaseTypeSyntaxWrapper)type
: null;
private static PrimaryConstructorBaseTypeSyntaxWrapper? BaseTypeSyntax(BaseListSyntax list) =>
list?.Types.FirstOrDefault() is { } type
&& PrimaryConstructorBaseTypeSyntaxWrapper.IsInstance(type)
? (PrimaryConstructorBaseTypeSyntaxWrapper)type
: null;
}
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,12 @@ public class ClassShouldNotBeEmptyTest

#if NET

private static readonly MetadataReference[] AdditionalReferences = new[]
{
private static readonly MetadataReference[] AdditionalReferences =
[
AspNetCoreMetadataReference.MicrosoftAspNetCoreMvcAbstractions,
AspNetCoreMetadataReference.MicrosoftAspNetCoreMvcCore,
AspNetCoreMetadataReference.MicrosoftAspNetCoreRazorPages,
};
];

[TestMethod]
public void ClassShouldNotBeEmpty_CSharp9() =>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,18 @@
using System.Collections.Generic;

public class TestCaseData(List<int> list, int expectedCount)
namespace Compliant
{
public List<int> List { get; }
public int ExpectedCount { get; }
class ChildClass() : BaseClass(42) { } // Compliant

class ChildClassWithParameters(int value) : BaseClass(value) { } // Compliant

class BaseClass(int value) { }
}

// See https://github.com/SonarSource/sonar-dotnet/issues/9011
public class CountTestCaseData(List<int> list, int expectedCount) : TestCaseData(list, expectedCount); // Noncompliant - FP
namespace Noncompliant
{
class ChildClass() : BaseClass() { } // Noncompliant

class BaseClass()
{
public int Value { get; init; }
}
}