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

Rule S3693: Exception constructors should not throw exceptions #629

Merged
merged 3 commits into from
Aug 2, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
{
"issues": [
{
"id": "S3693",
"message": "Avoid throwing exceptions in this constructor.",
"location": {
"uri": "Nancy\src\Nancy\ModelBinding\ModelBindingException.cs",
"region": {
"startLine": 36,
"startColumn": 17,
"endLine": 36,
"endColumn": 62
}
}
}
]
}
18 changes: 18 additions & 0 deletions sonaranalyzer-dotnet/rspec/cs/S3693_c#.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<p>It may be a good idea to raise an exception in a constructor if you're unable to fully flesh the object in question, but not in an
<code>exception</code> constructor. If you do, you'll interfere with the exception that was originally being thrown. Further, it is highly unlikely
that an exception raised in the creation of an exception will be properly handled in the calling code, and the unexpected, unhandled exception will
lead to program termination.</p>
<h2>Noncompliant Code Example</h2>
<pre>
class MyException: Exception
{
public void MyException()
{
if (bad_thing)
{
throw new Exception("A bad thing happened"); // Noncompliant
}
}
}
</pre>

13 changes: 13 additions & 0 deletions sonaranalyzer-dotnet/rspec/cs/S3693_c#.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
{
"title": "Exception constructors should not throw exceptions",
"type": "BUG",
"status": "ready",
"remediation": {
"func": "Constant\/Issue",
"constantCost": "15min"
},
"tags": [

],
"defaultSeverity": "Blocker"
}
1 change: 1 addition & 0 deletions sonaranalyzer-dotnet/rspec/cs/Sonar_way_profile.json
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,7 @@
"S3626",
"S3649",
"S3655",
"S3693",
"S3776",
"S3869",
"S3871",
Expand Down
27 changes: 27 additions & 0 deletions sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/RspecStrings.resx
Original file line number Diff line number Diff line change
Expand Up @@ -5613,6 +5613,33 @@
<data name="S3655_Type" xml:space="preserve">
<value>BUG</value>
</data>
<data name="S3693_Category" xml:space="preserve">
<value>Sonar Bug</value>
</data>
<data name="S3693_Description" xml:space="preserve">
<value>It may be a good idea to raise an exception in a constructor if you're unable to fully flesh the object in question, but not in an exception constructor. If you do, you'll interfere with the exception that was originally being thrown. Further, it is highly unlikely that an exception raised in the creation of an exception will be properly handled in the calling code, and the unexpected, unhandled exception will lead to program termination.</value>
</data>
<data name="S3693_IsActivatedByDefault" xml:space="preserve">
<value>True</value>
</data>
<data name="S3693_Remediation" xml:space="preserve">
<value>Constant/Issue</value>
</data>
<data name="S3693_RemediationCost" xml:space="preserve">
<value>15min</value>
</data>
<data name="S3693_Severity" xml:space="preserve">
<value>Blocker</value>
</data>
<data name="S3693_Tags" xml:space="preserve">
<value />
</data>
<data name="S3693_Title" xml:space="preserve">
<value>Exception constructors should not throw exceptions</value>
</data>
<data name="S3693_Type" xml:space="preserve">
<value>BUG</value>
</data>
<data name="S3717_Category" xml:space="preserve">
<value>Sonar Code Smell</value>
</data>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
/*
* SonarAnalyzer for .NET
* Copyright (C) 2015-2017 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 System.Collections.Immutable;
using System.Linq;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.CSharp.Syntax;
using Microsoft.CodeAnalysis.Diagnostics;
using SonarAnalyzer.Common;
using SonarAnalyzer.Helpers;

namespace SonarAnalyzer.Rules.CSharp
{
[DiagnosticAnalyzer(LanguageNames.CSharp)]
[Rule(DiagnosticId)]
public sealed class ExceptionConstructorShouldNotThrow : SonarDiagnosticAnalyzer
{
internal const string DiagnosticId = "S3693";
private const string MessageFormat = "Avoid throwing exceptions in this constructor.";

private static readonly DiagnosticDescriptor rule =
DiagnosticDescriptorBuilder.GetDescriptor(DiagnosticId, MessageFormat, RspecStrings.ResourceManager);
public override ImmutableArray<DiagnosticDescriptor> SupportedDiagnostics => ImmutableArray.Create(rule);

protected override void Initialize(SonarAnalysisContext context)
{
context.RegisterSyntaxNodeActionInNonGenerated(
c =>
{
var classDeclaration = (ClassDeclarationSyntax)c.Node;

var classSymbol = c.SemanticModel.GetDeclaredSymbol(classDeclaration);
if (classSymbol == null ||
!classSymbol.DerivesFrom(KnownType.System_Exception))
{
return;
}

var throwStatementsPerCtor = classDeclaration.Members
.OfType<ConstructorDeclarationSyntax>()
.Select(ctor => ctor.DescendantNodes().OfType<ThrowStatementSyntax>().ToList())
.Where(@throw => @throw.Count > 0)
.ToList();

foreach (var throwStatement in throwStatementsPerCtor)
{
c.ReportDiagnostic(Diagnostic.Create(rule, throwStatement.First().GetLocation(),
throwStatement.Skip(1).Select(@throw => @throw.GetLocation())));
}
},
SyntaxKind.ClassDeclaration);
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
<p>It may be a good idea to raise an exception in a constructor if you're unable to fully flesh the object in question, but not in an
<code>exception</code> constructor. If you do, you'll interfere with the exception that was originally being thrown. Further, it is highly unlikely
that an exception raised in the creation of an exception will be properly handled in the calling code, and the unexpected, unhandled exception will
lead to program termination.</p>
<h2>Noncompliant Code Example</h2>
<pre>
class MyException: Exception
{
public void MyException()
{
if (bad_thing)
{
throw new Exception("A bad thing happened"); // Noncompliant
}
}
}
</pre>

Original file line number Diff line number Diff line change
Expand Up @@ -3690,7 +3690,7 @@ private static void DetectTypeChanges(ResourceManager resourceManager, IImmutabl
//["3690"],
//["3691"],
//["3692"],
//["3693"],
["3693"] = "BUG",
//["3694"],
//["3695"],
//["3696"],
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
/*
* SonarAnalyzer for .NET
* Copyright (C) 2015-2017 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 Microsoft.VisualStudio.TestTools.UnitTesting;
using SonarAnalyzer.Rules.CSharp;

namespace SonarAnalyzer.UnitTest.Rules
{
[TestClass]
public class ExceptionConstructorShouldNotThrowTest
{
[TestMethod]
[TestCategory("Rule")]
public void ExceptionConstructorShouldNotThrow()
{
Verifier.VerifyAnalyzer(@"TestCases\ExceptionConstructorShouldNotThrow.cs",
new ExceptionConstructorShouldNotThrow());
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
using System;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a couple of tests:

  • Throw without object initialization, e.g. var a = new Exception(); throw a;
  • Rethrow, e.g. throw;.


namespace Tests.Diagnostics
{
class MyException : Exception
{
public MyException()
{
throw new Exception(); // Noncompliant {{Avoid throwing exceptions in this constructor.}}
// ^^^^^^^^^^^^^^^^^^^^^^
}
}

class MyException2 : Exception
{
public MyException2()
{

}

public MyException2(int i)
{
if (i == 42)
{
throw new Exception(); // Noncompliant
}
}
}

class MyException3 : Exception
{
public MyException3(int i)
{
if (i == 42)
{
throw new Exception(); // Noncompliant
}
else
{
throw new ArgumentException(); // Secondary
}
}
}

class SubException : MyException
{
public SubException()
{
throw new FieldAccessException(); // Noncompliant
}
}

class MyException4 : Exception
{
public MyException4()
{
throw; // Noncompliant
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a valid statement? what will be thrown here?

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 don't think this is a valid statement in this condition but I guess the idea was to be sure the analyzer doesn't crash.

}
}

class MyException5 : Exception
{
public MyException5()
{
var ex = new Exception();
throw ex; // Noncompliant
}
}

class Something
{
public Something()
{
throw new Exception(); // Compliant
}
}
}