New rule S2166: Classes named like "Exception" should extend "Exception" or a subclass #6727
Conversation
| * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. | ||
| */ | ||
|
|
||
| using Microsoft.CodeAnalysis.CSharp; |
There was a problem hiding this comment.
This using seems unnecessary.
There was a problem hiding this comment.
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.
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.
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.
builderCS should go to the new line.
Same in lines below.
There was a problem hiding this comment.
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.
Why are these not in the Language.SyntaxKinds?
There was a problem hiding this comment.
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.
@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.
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.
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.
This empty line can be removed
There was a problem hiding this comment.
Removed it.
zsolt-kolbay-sonarsource
requested review from
andrei-epure-sonarsource
and removed request for
Corniel
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.
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.
Moved it.
There was a problem hiding this comment.
note: by "(also in interface etc)" I meant in all files where this applies
zsolt-kolbay-sonarsource
requested review from
andrei-epure-sonarsource
and removed request for
Corniel
| @@ -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.
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.
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.
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.
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! |
zsolt-kolbay-sonarsource
deleted the
Zsolt/S2166-class-named-exception
branch
Fixes #6705
Related RSPEC PR