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 S2094: Classes should not be empty #6754
Conversation
@zsolt-kolbay-sonarsource : you may want to have a look at the failing GitHub actions checks. I think I can start the review anyway. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly tests to add and code conventions remarks.
Source generation and abstract classes without abstract methods may require further analysis.
Another general remarks is whether this rule should also consider struct
and record struct
. While the rule explicitly mention classes, other languages such as Java don't have the struct
construct, while others, such as C++, are excluded from the rule for reasons explained in the comments.
analyzers/src/SonarAnalyzer.CSharp/Rules/ClassShouldNotBeEmpty.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Facade/CSharpSyntaxKindFacade.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/ClassShouldNotBeEmpty.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ClassShouldNotBeEmpty.CSharp10.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ClassShouldNotBeEmpty.CSharp9.cs
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ClassShouldNotBeEmpty.Inheritance.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ClassShouldNotBeEmpty.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/ClassShouldNotBeEmptyBase.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a couple of minor remarks + the "source generators" topic, on which I let @andrei-epure-sonarsource answer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interim comments on java IT
its/src/test/java/com/sonar/it/csharp/IncrementalAnalysisTest.java
Outdated
Show resolved
Hide resolved
its/src/test/java/com/sonar/it/csharp/ProjectLevelDuplicationTest.java
Outdated
Show resolved
Hide resolved
its/src/test/java/com/sonar/it/csharp/IncrementalAnalysisTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interim comments
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ClassShouldNotBeEmpty.Inheritance.cs
Show resolved
Hide resolved
its/src/test/java/com/sonar/it/csharp/IncrementalAnalysisTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interim
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ClassShouldNotBeEmpty.vb
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ClassShouldNotBeEmpty.vb
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ClassShouldNotBeEmpty.vb
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ClassShouldNotBeEmpty.Inheritance.cs
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ClassShouldNotBeEmpty.CSharp10.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/Rules/ClassShouldNotBeEmptyTest.cs
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/ClassShouldNotBeEmpty.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.CSharp/Rules/ClassShouldNotBeEmpty.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/ClassShouldNotBeEmptyBase.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The build is failing, you probably need to update an UT.
All good, only one small comment left #6754 (comment)
Request changes as the PR cannot be merged as is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but please remove the redundant safety check - I've looked into the types and it's not necessary (see #6754 (comment) )
Kudos, SonarCloud Quality Gate passed! |
SonarCloud Quality Gate failed. |
|
||
protected override bool IsClassWithDeclaredBaseClass(SyntaxNode node) => node is ClassDeclarationSyntax { BaseList: not null }; | ||
|
||
protected override string DeclarationTypeKeyword(SyntaxNode node) => ((TypeDeclarationSyntax)node).Keyword.ValueText; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zsolt-kolbay-sonarsource I was looking at coverage and noticed this new line.
According to coding style, the expression body should have been on a new line. Please fix it in any next random PR. Same above and on VB.
protected override string DeclarationTypeKeyword(SyntaxNode node) =>
((TypeDeclarationSyntax)node).Keyword.ValueText;
Fixes #6704
Related RSPEC PR