-
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
Changes from 11 commits
12c49d4
43d058f
78dd3fc
b2d8e63
da7f219
af479a1
a8e4e12
5215eca
6818afe
fb60d96
c49813e
df85f92
8d95675
3a755e4
540c20b
8b1e18b
5715d6d
fa18201
0ad6700
044ce8b
8e0317d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
/* | ||
* <Your-Product-Name> | ||
* Copyright (c) <Year-From>-<Year-To> <Your-Company-Name> | ||
* | ||
* Please configure this header in your SonarCloud/SonarQube quality profile. | ||
* You can also set it in SonarLint.xml additional file for SonarLint or standalone NuGet analyzer. | ||
*/ | ||
|
||
namespace IntentionalFindings | ||
{ | ||
public static class S2166 | ||
{ | ||
public class CustomException { } // Noncompliant (S2166) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
' <Your-Product-Name> | ||
' Copyright (c) <Year-From>-<Year-To> <Your-Company-Name> | ||
' | ||
' Please configure this header in your SonarCloud/SonarQube quality profile. | ||
' You can also set it in SonarLint.xml additional file for SonarLint or standalone NuGet analyzer. | ||
|
||
Public Class S2166 | ||
Public Class CustomException ' Noncompliant (S2166) | ||
|
||
End Class | ||
End Class |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
<p>Clear, communicative naming is important in code. It helps maintainers and API users understand the intentions for and uses of a unit of code. | ||
Using "exception" in the name of a class that does not extend <code>Exception</code> or one of its subclasses is a clear violation of the expectation | ||
that a class' name will indicate what it is and/or does.</p> | ||
<h2>Noncompliant Code Example</h2> | ||
<pre> | ||
public class FruitException // Noncompliant - this has nothing to do with Exception | ||
{ | ||
private Fruit expected; | ||
private string unusualCharacteristics; | ||
private bool appropriateForCommercialExploitation; | ||
// ... | ||
} | ||
|
||
public class CarException // Noncompliant - does not derive from any Exception-based class | ||
{ | ||
public CarException(string message, Exception inner) | ||
{ | ||
// ... | ||
} | ||
} | ||
</pre> | ||
<h2>Compliant Solution</h2> | ||
<pre> | ||
public class FruitSport // Compliant - class name does not end with 'Exception' | ||
{ | ||
private Fruit expected; | ||
private string unusualCharacteristics; | ||
private bool appropriateForCommercialExploitation; | ||
// ... | ||
} | ||
|
||
public class CarException: Exception // Compliant - correctly extends System.Exception | ||
{ | ||
public CarException(string message, Exception inner): base(message, inner) | ||
{ | ||
// ... | ||
} | ||
} | ||
</pre> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
{ | ||
"title": "Classes named like \"Exception\" should extend \"Exception\" or a subclass", | ||
"type": "CODE_SMELL", | ||
"status": "ready", | ||
"remediation": { | ||
"func": "Constant\/Issue", | ||
"constantCost": "5min" | ||
}, | ||
"tags": [ | ||
"convention", | ||
"error-handling", | ||
"pitfall" | ||
], | ||
"defaultSeverity": "Major", | ||
"ruleSpecification": "RSPEC-2166", | ||
"sqKey": "S2166", | ||
"scope": "Main", | ||
"quickfix": "unknown" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -62,6 +62,7 @@ | |
"S2114", | ||
"S2115", | ||
"S2123", | ||
"S2166", | ||
"S2178", | ||
"S2183", | ||
"S2184", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,35 @@ | ||
<p>Clear, communicative naming is important in code. It helps maintainers and API users understand the intentions for and uses of a unit of code. | ||
Using "exception" in the name of a class that does not extend <code>Exception</code> or one of its subclasses is a clear violation of the expectation | ||
that a class' name will indicate what it is and/or does.</p> | ||
<h2>Noncompliant Code Example</h2> | ||
<pre> | ||
Public Class FruitException ' Noncompliant - this has nothing to do with Exception | ||
Private expected As Fruit | ||
Private unusualCharacteristics As String | ||
Private appropriateForCommercialExploitation As Boolean | ||
' ... | ||
End Class | ||
|
||
Public Class CarException ' Noncompliant - does not derive from any Exception-based class | ||
Public Sub New(ByVal message As String, ByVal inner As Exception) | ||
' ... | ||
End Sub | ||
End Class | ||
</pre> | ||
<h2>Compliant Solution</h2> | ||
<pre> | ||
Public Class FruitSport ' Compliant - class name does not end with 'Exception' | ||
Private expected As Fruit | ||
Private unusualCharacteristics As String | ||
Private appropriateForCommercialExploitation As Boolean | ||
' ... | ||
End Class | ||
|
||
Public Class CarException Inherits Exception ' Compliant - correctly extends System.Exception | ||
Public Sub New(ByVal message As String, ByVal inner As Exception) | ||
andrei-epure-sonarsource marked this conversation as resolved.
Show resolved
Hide resolved
|
||
MyBase.New(message, inner) | ||
' ... | ||
End Sub | ||
End Class | ||
</pre> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
{ | ||
"title": "Classes named like \"Exception\" should extend \"Exception\" or a subclass", | ||
"type": "CODE_SMELL", | ||
"status": "ready", | ||
"remediation": { | ||
"func": "Constant\/Issue", | ||
"constantCost": "5min" | ||
}, | ||
"tags": [ | ||
"convention", | ||
"error-handling", | ||
"pitfall" | ||
], | ||
"defaultSeverity": "Major", | ||
"ruleSpecification": "RSPEC-2166", | ||
"sqKey": "S2166", | ||
"scope": "Main", | ||
"quickfix": "unknown" | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -38,6 +38,7 @@ | |
"S1940", | ||
"S2068", | ||
"S2077", | ||
"S2166", | ||
"S2178", | ||
"S2222", | ||
"S2225", | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
/* | ||
* SonarAnalyzer for .NET | ||
* Copyright (C) 2015-2023 SonarSource SA | ||
* mailto: contact AT sonarsource DOT com | ||
* | ||
* This program is free software; you can redistribute it and/or | ||
* modify it under the terms of the GNU Lesser General Public | ||
* License as published by the Free Software Foundation; either | ||
* version 3 of the License, or (at your option) any later version. | ||
* | ||
* This program is distributed in the hope that it will be useful, | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU | ||
* Lesser General Public License for more details. | ||
* | ||
* You should have received a copy of the GNU Lesser General Public License | ||
* along with this program; if not, write to the Free Software Foundation, | ||
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. | ||
*/ | ||
|
||
namespace SonarAnalyzer.Rules.CSharp; | ||
|
||
[DiagnosticAnalyzer(LanguageNames.CSharp)] | ||
public sealed class ClassNamedException : ClassNamedExceptionBase<SyntaxKind> | ||
{ | ||
protected override ILanguageFacade<SyntaxKind> Language => CSharpFacade.Instance; | ||
|
||
protected override SyntaxKind[] AnalyzedSyntaxKinds => new[] { SyntaxKind.ClassDeclaration }; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
/* | ||
* SonarAnalyzer for .NET | ||
* Copyright (C) 2015-2023 SonarSource SA | ||
* mailto: contact AT sonarsource DOT com | ||
* | ||
* This program is free software; you can redistribute it and/or | ||
* modify it under the terms of the GNU Lesser General Public | ||
* License as published by the Free Software Foundation; either | ||
* version 3 of the License, or (at your option) any later version. | ||
* | ||
* This program is distributed in the hope that it will be useful, | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU | ||
* Lesser General Public License for more details. | ||
* | ||
* You should have received a copy of the GNU Lesser General Public License | ||
* along with this program; if not, write to the Free Software Foundation, | ||
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. | ||
*/ | ||
|
||
namespace SonarAnalyzer.Rules; | ||
|
||
public abstract class ClassNamedExceptionBase<TSyntaxKind> : SonarDiagnosticAnalyzer<TSyntaxKind> | ||
where TSyntaxKind : struct | ||
{ | ||
private const string DiagnosticId = "S2166"; | ||
|
||
protected abstract TSyntaxKind[] AnalyzedSyntaxKinds { get; } | ||
protected override string MessageFormat => "Rename this class to remove \"(e|E)xception\" or correct its inheritance."; | ||
andrei-epure-sonarsource marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
protected ClassNamedExceptionBase() : base(DiagnosticId) { } | ||
|
||
protected override void Initialize(SonarAnalysisContext context) => | ||
context.RegisterNodeAction( | ||
antonioaversa marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Language.GeneratedCodeRecognizer, | ||
c => | ||
{ | ||
antonioaversa marked this conversation as resolved.
Show resolved
Hide resolved
|
||
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.DerivesFrom(KnownType.System_Exception)) | ||
andrei-epure-sonarsource marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
c.ReportIssue(Diagnostic.Create(Rule, classIdentifier.GetLocation())); | ||
} | ||
}, | ||
AnalyzedSyntaxKinds); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 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 commentThe 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 commentThe 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. |
||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
/* | ||
* SonarAnalyzer for .NET | ||
* Copyright (C) 2015-2023 SonarSource SA | ||
* mailto: contact AT sonarsource DOT com | ||
* | ||
* This program is free software; you can redistribute it and/or | ||
* modify it under the terms of the GNU Lesser General Public | ||
* License as published by the Free Software Foundation; either | ||
* version 3 of the License, or (at your option) any later version. | ||
* | ||
* This program is distributed in the hope that it will be useful, | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU | ||
* Lesser General Public License for more details. | ||
* | ||
* You should have received a copy of the GNU Lesser General Public License | ||
* along with this program; if not, write to the Free Software Foundation, | ||
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. | ||
*/ | ||
|
||
namespace SonarAnalyzer.Rules.VisualBasic; | ||
|
||
[DiagnosticAnalyzer(LanguageNames.VisualBasic)] | ||
public sealed class ClassNamedException : ClassNamedExceptionBase<SyntaxKind> | ||
{ | ||
protected override ILanguageFacade<SyntaxKind> Language => VisualBasicFacade.Instance; | ||
|
||
protected override SyntaxKind[] AnalyzedSyntaxKinds => new[] { SyntaxKind.ClassBlock, SyntaxKind.ModuleBlock }; | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,57 @@ | ||
/* | ||
* SonarAnalyzer for .NET | ||
* Copyright (C) 2015-2023 SonarSource SA | ||
* mailto: contact AT sonarsource DOT com | ||
* | ||
* This program is free software; you can redistribute it and/or | ||
* modify it under the terms of the GNU Lesser General Public | ||
* License as published by the Free Software Foundation; either | ||
* version 3 of the License, or (at your option) any later version. | ||
* | ||
* This program is distributed in the hope that it will be useful, | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU | ||
* Lesser General Public License for more details. | ||
* | ||
* You should have received a copy of the GNU Lesser General Public License | ||
* along with this program; if not, write to the Free Software Foundation, | ||
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. | ||
*/ | ||
|
||
using CS = SonarAnalyzer.Rules.CSharp; | ||
using VB = SonarAnalyzer.Rules.VisualBasic; | ||
|
||
namespace SonarAnalyzer.UnitTest.Rules; | ||
|
||
[TestClass] | ||
public class ClassNamedExceptionTest | ||
{ | ||
private readonly VerifierBuilder builderCS = new VerifierBuilder<CS.ClassNamedException>(); | ||
private readonly VerifierBuilder builderVB = new VerifierBuilder<VB.ClassNamedException>(); | ||
|
||
[TestMethod] | ||
public void ClassNamedException_CS() => | ||
builderCS | ||
.AddPaths("ClassNamedException.cs") | ||
.Verify(); | ||
|
||
[TestMethod] | ||
public void ClassNamedException_FromCSharp9() => | ||
builderCS | ||
.AddPaths("ClassNamedException.CSharp9.cs") | ||
.WithOptions(ParseOptionsHelper.FromCSharp9) | ||
.Verify(); | ||
|
||
[TestMethod] | ||
public void ClassNamedException_FromCSharp10() => | ||
builderCS | ||
.AddPaths("ClassNamedException.CSharp10.cs") | ||
.WithOptions(ParseOptionsHelper.FromCSharp10) | ||
.Verify(); | ||
|
||
[TestMethod] | ||
public void ClassNamedException_VB() => | ||
builderVB | ||
.AddPaths("ClassNamedException.vb") | ||
.Verify(); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
record struct RecordStructException { } // Compliant | ||
record class RecordClassException { } // Compliant | ||
andrei-epure-sonarsource marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
record RecordException { } // Compliant |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,37 @@ | ||
using System; | ||
|
||
class CustomException { } // Noncompliant {{Rename this class to remove "(e|E)xception" or correct its inheritance.}} | ||
// ^^^^^^^^^^^^^^^ | ||
class Customexception { } // Noncompliant | ||
class CustomEXCEPTION { } // Noncompliant | ||
|
||
antonioaversa marked this conversation as resolved.
Show resolved
Hide resolved
|
||
class ExceptionHandler { } // Compliant - "Exception" is not at end of the name of the class | ||
class SimpleClass { } | ||
class SimpleExceptionClass: Exception { } | ||
andrei-epure-sonarsource marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
class OuterClass | ||
{ | ||
class InnerException { } // Noncompliant | ||
} | ||
|
||
class GenericClassDoesNotExtendException<T> { } // Noncompliant | ||
// ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | ||
class GenericClassExtendsException<T> : Exception { } // Compliant | ||
class SimpleGenericClass<T> { } // Compliant - "Exception" is not in the name of the class | ||
|
||
interface IEmptyInterfaceException { } // Compliant - interfaces cannot inherit from Exception | ||
struct StructException { } // Compliant - structs cannot inherit from Exception | ||
enum EnumException { } // Compliant - enums cannot inherit from Exception | ||
|
||
class ExtendsException: Exception { } // Compliant - direct subclass of Exception | ||
class ImplementsAnInterfaceAndExtendsException: Exception, IEmptyInterfaceException { } | ||
class ExtendsNullReferenceException : NullReferenceException { } // Compliant - indirect subclass of Exception | ||
|
||
class ExtendsCustomException: CustomException { } // Noncompliant - CustomException is not an Exception subclass | ||
|
||
partial class PartialClassDoesNotExtendException { } // Noncompliant | ||
|
||
partial class PartialClassExtendsException { } // Compliant - the other part of the class extends Exception | ||
partial class PartialClassExtendsException: Exception { } | ||
|
||
static class StaticException { } // Noncompliant - the static class should be renamed, as it cannot inherit from Exception |
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.