-
Notifications
You must be signed in to change notification settings - Fork 226
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 S2166: Classes named like "Exception" should extend "Exception" or a subclass #6727
New rule S2166: Classes named like "Exception" should extend "Exception" or a subclass #6727
Conversation
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 some indenting to fix. The rest are just suggestions.
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. | ||
*/ | ||
|
||
using Microsoft.CodeAnalysis.CSharp; |
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.
This using seems unnecessary.
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.
Good catch. Removed it.
if (Language.Syntax.NodeIdentifier(c.Node) is { IsMissing: false } classIdentifier | ||
&& classIdentifier.ValueText.EndsWith("Exception", StringComparison.InvariantCultureIgnoreCase) | ||
&& c.SemanticModel.GetDeclaredSymbol(c.Node) is INamedTypeSymbol { } classSymbol | ||
&& !classSymbol.IsStatic |
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.
Feel free to ignore this remark if you disagree.
If I understand well, this disables the rule for static classes.
Why would you think it's appropriate, for static classes, to have a name ending in Exception
? To me, that seems to violate the principle of least astonishment.
As for addressing the issue in this scenario: while you can't inherit from Exception
, you can still rename the class and remove the suffix.
The same reasoning would also apply to Module
, in my opinion.
As for struct
, enum
and record struct
: seems OK to me to not consider them in this rule, as it mentions class
in its title (although the other languages for which the rule is implemented, namely Java and PHP, don't have to distinguish on such cases).
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.
Agreed. I changed the test cases to reflect this.
private readonly VerifierBuilder builderVB = new VerifierBuilder<VB.ClassNamedException>(); | ||
|
||
[TestMethod] | ||
public void ClassNamedException_CS() => builderCS |
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.
builderCS
should go to the new line.
Same in lines below.
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.
Moved them to new lines.
I would suggest to add two fixes: on that removes the suffix, and one that inherits from exception. (That can be a follow-up PR). |
c.ReportIssue(Diagnostic.Create(Rule, classIdentifier.GetLocation())); | ||
} | ||
}, | ||
AnalyzedSyntaxKinds); |
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.
Why are these not in the Language.SyntaxKinds?
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.
I wanted to cover static classes with this rule. In C# both regular classes and static classes are covered by SyntaxKind.ClassDeclaration
. In VB.NET the equivalent of static classes are Modules*, so if I want to cover both classes and modules, I need to use SyntaxKind.ClassBlock
and SyntaxKind.ModuleBlock
. I couldn't come up with any good name for this in Language.SyntaxKinds
(ClassOrStaticClassDeclaration
?), so I ended up adding it to the rule class itself.
What do you think would be the right name for this declaration?
Thank you for the suggestion, we'll consider it in a follow-up PR.
* there's a nice debate here whether that's actually accurate, but they generate the same IL code, so I consider them equivalent
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 Modules are a pain in the @$$ indeed. That being said, the C# equivalent also triggers on the declaration of static classes, so why not the VB.NET one?
After all, if you have a base rule both for C# and VB.NET that does not apply on static classes/modules, you need to deal with that 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.
Fair enough. And we might need in the future for other rules which need both C# and VB.NET implementation.
I've added it to Language.SyntaxKind
as ClassAndModuleDeclarations
.
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 single comment about empty line.
Approving even though it requires a second review.
After the comment by @Corniel has been addressed, the PR can go to @andrei-epure-sonarsource.
|
||
Public Class S2166 | ||
Public Class CustomException ' Noncompliant (S2166) | ||
|
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.
This empty line can be removed
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.
Removed it.
analyzers/src/SonarAnalyzer.Common/Rules/ClassNamedExceptionBase.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.Common/Rules/ClassNamedExceptionBase.cs
Outdated
Show resolved
Hide resolved
analyzers/src/SonarAnalyzer.VisualBasic/Helpers/VisualBasicSyntaxHelper.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ClassNamedException.CSharp10.cs
Outdated
Show resolved
Hide resolved
analyzers/tests/SonarAnalyzer.UnitTest/TestCases/ClassNamedException.cs
Outdated
Show resolved
Hide resolved
(also please check the minor code smell on SonarCloud) |
@@ -29,6 +31,7 @@ internal sealed class CSharpSyntaxKindFacade : ISyntaxKindFacade<SyntaxKind> | |||
SyntaxKindEx.RecordClassDeclaration, | |||
}; | |||
public SyntaxKind ClassDeclaration => SyntaxKind.ClassDeclaration; |
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.
nitpick: now ClassDeclaration
is between ClassAndRecordDeclaration
and ClassAndModuleDeclarations
, which for me makes it a bit harder to read
Could you move ClassDeclaration
above ClassAndRecordDeclaration
? (also in interface etc)
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.
Moved it.
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.
note: by "(also in interface etc)" I meant in all files where this applies
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, please address last comments and you can merge after.
@@ -26,6 +26,7 @@ public interface ISyntaxKindFacade<out TSyntaxKind> | |||
abstract TSyntaxKind Attribute { get; } | |||
abstract TSyntaxKind[] ClassAndRecordDeclaration { get; } | |||
abstract TSyntaxKind ClassDeclaration { get; } |
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.
please move ClassDeclaration
above ClassAndRecordDeclaration
@@ -25,6 +25,11 @@ internal sealed class VisualBasicSyntaxKindFacade : ISyntaxKindFacade<SyntaxKind | |||
public SyntaxKind Attribute => SyntaxKind.Attribute; | |||
public SyntaxKind[] ClassAndRecordDeclaration => new[] { SyntaxKind.ClassBlock }; | |||
public SyntaxKind ClassDeclaration => SyntaxKind.ClassBlock; |
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.
please move ClassDeclaration
above ClassAndRecordDeclaration
@@ -29,6 +31,7 @@ internal sealed class CSharpSyntaxKindFacade : ISyntaxKindFacade<SyntaxKind> | |||
SyntaxKindEx.RecordClassDeclaration, | |||
}; | |||
public SyntaxKind ClassDeclaration => SyntaxKind.ClassDeclaration; |
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.
note: by "(also in interface etc)" I meant in all files where this applies
.AddPaths("ClassNamedException.vb") | ||
.Verify(); | ||
|
||
#if NETFRAMEWORK |
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.
please add an empty line after #if
and before #endif
see https://github.com/SonarSource/sonar-dotnet/blob/master/docs/coding-style.md#code-structure
Each compiler directive outside method body (namely #if/#endif) should be preceded and followed by an empty line.
(off-topic - this could be a rule in https://github.com/DotNetAnalyzers/StyleCopAnalyzers/blob/master/documentation/SpacingRules.md)
Kudos, SonarCloud Quality Gate passed! |
Kudos, SonarCloud Quality Gate passed! |
Fixes #6705
Related RSPEC PR