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

New rule S2166: Classes named like "Exception" should extend "Exception" or a subclass #6727

Merged
merged 21 commits into from Feb 9, 2023
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
12c49d4
Initial scaffolding
zsolt-kolbay-sonarsource Feb 6, 2023
43d058f
Add test cases
zsolt-kolbay-sonarsource Feb 6, 2023
78dd3fc
Implement C# rule
zsolt-kolbay-sonarsource Feb 6, 2023
b2d8e63
Update RSPEC
zsolt-kolbay-sonarsource Feb 6, 2023
da7f219
Add VB test cases
zsolt-kolbay-sonarsource Feb 7, 2023
af479a1
Fix indentation
zsolt-kolbay-sonarsource Feb 7, 2023
a8e4e12
Add test case for generic classes
zsolt-kolbay-sonarsource Feb 7, 2023
5215eca
Different handling of static classes / modules
zsolt-kolbay-sonarsource Feb 7, 2023
6818afe
Add record class test case
zsolt-kolbay-sonarsource Feb 7, 2023
fb60d96
Add intentional finding test cases
zsolt-kolbay-sonarsource Feb 7, 2023
c49813e
Fix unit tests
zsolt-kolbay-sonarsource Feb 7, 2023
df85f92
Remove empty line
zsolt-kolbay-sonarsource Feb 8, 2023
8d95675
Introduce ClassAndModuleDeclarations to ISyntaxKindFacade
zsolt-kolbay-sonarsource Feb 8, 2023
3a755e4
Remove empty line
zsolt-kolbay-sonarsource Feb 8, 2023
540c20b
Update RSPEC and rule message
zsolt-kolbay-sonarsource Feb 9, 2023
8b1e18b
Update test cases
zsolt-kolbay-sonarsource Feb 9, 2023
5715d6d
Add explanation to test cases
zsolt-kolbay-sonarsource Feb 9, 2023
fa18201
Add compilation error test cases
zsolt-kolbay-sonarsource Feb 9, 2023
0ad6700
Add test case for System.Runtime.InteropServices._Exception
zsolt-kolbay-sonarsource Feb 9, 2023
044ce8b
Move up ClassDeclaration in CSharpSyntaxKindFacade
zsolt-kolbay-sonarsource Feb 9, 2023
8e0317d
Fix formatting
zsolt-kolbay-sonarsource Feb 9, 2023
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
@@ -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)
}
}
@@ -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)

Copy link
Contributor

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it.

End Class
End Class
40 changes: 40 additions & 0 deletions analyzers/rspec/cs/S2166_c#.html
@@ -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>

19 changes: 19 additions & 0 deletions analyzers/rspec/cs/S2166_c#.json
@@ -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"
}
1 change: 1 addition & 0 deletions analyzers/rspec/cs/Sonar_way_profile.json
Expand Up @@ -62,6 +62,7 @@
"S2114",
"S2115",
"S2123",
"S2166",
"S2178",
"S2183",
"S2184",
Expand Down
35 changes: 35 additions & 0 deletions analyzers/rspec/vbnet/S2166_vb.net.html
@@ -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>

19 changes: 19 additions & 0 deletions analyzers/rspec/vbnet/S2166_vb.net.json
@@ -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"
}
1 change: 1 addition & 0 deletions analyzers/rspec/vbnet/Sonar_way_profile.json
Expand Up @@ -38,6 +38,7 @@
"S1940",
"S2068",
"S2077",
"S2166",
"S2178",
"S2222",
"S2225",
Expand Down
29 changes: 29 additions & 0 deletions analyzers/src/SonarAnalyzer.CSharp/Rules/ClassNamedException.cs
@@ -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 };
}
@@ -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);
Copy link
Contributor

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?

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 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

Copy link
Contributor

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.

Copy link
Contributor Author

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.


}
Expand Up @@ -168,6 +168,7 @@ private static bool IsOn(this ExpressionSyntax expression, SyntaxKind onKind)
MemberAccessExpressionSyntax x => x.Name.Identifier,
MethodBlockSyntax x => x.SubOrFunctionStatement?.GetIdentifier(),
MethodStatementSyntax x => x.Identifier,
ModuleBlockSyntax x => x.ModuleStatement?.Identifier,
andrei-epure-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
EnumStatementSyntax x => x.Identifier,
EnumMemberDeclarationSyntax x => x.Identifier,
InvocationExpressionSyntax x => x.Expression?.GetIdentifier(),
Expand Down
@@ -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 };
}
Expand Up @@ -2090,7 +2090,7 @@ internal static class RuleTypeMappingCS
// ["S2163"],
// ["S2164"],
// ["S2165"],
// ["S2166"],
["S2166"] = "CODE_SMELL",
// ["S2167"],
// ["S2168"],
// ["S2169"],
Expand Down
Expand Up @@ -2090,7 +2090,7 @@ internal static class RuleTypeMappingVB
// ["S2163"],
// ["S2164"],
// ["S2165"],
// ["S2166"],
["S2166"] = "CODE_SMELL",
// ["S2167"],
// ["S2168"],
// ["S2169"],
Expand Down
@@ -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();
}
@@ -0,0 +1,2 @@
record struct RecordStructException { } // Compliant
record class RecordClassException { } // Compliant
andrei-epure-sonarsource marked this conversation as resolved.
Show resolved Hide resolved
@@ -0,0 +1 @@
record RecordException { } // Compliant
@@ -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