diff --git a/sonaranalyzer-dotnet/its/expected/Nancy/Nancy-{34576216-0DCA-4B0F-A0DC-9075E75A676F}-S3693.json b/sonaranalyzer-dotnet/its/expected/Nancy/Nancy-{34576216-0DCA-4B0F-A0DC-9075E75A676F}-S3693.json new file mode 100644 index 00000000000..eb6b42b4b1a --- /dev/null +++ b/sonaranalyzer-dotnet/its/expected/Nancy/Nancy-{34576216-0DCA-4B0F-A0DC-9075E75A676F}-S3693.json @@ -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 +} +} +} +] +} diff --git a/sonaranalyzer-dotnet/rspec/cs/S3693_c#.html b/sonaranalyzer-dotnet/rspec/cs/S3693_c#.html new file mode 100644 index 00000000000..426141212bb --- /dev/null +++ b/sonaranalyzer-dotnet/rspec/cs/S3693_c#.html @@ -0,0 +1,18 @@ +

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.

+

Noncompliant Code Example

+
+class MyException: Exception
+{
+    public void MyException()
+    {
+         if (bad_thing)
+         {
+             throw new Exception("A bad thing happened");  // Noncompliant
+          }
+    }
+}
+
+ diff --git a/sonaranalyzer-dotnet/rspec/cs/S3693_c#.json b/sonaranalyzer-dotnet/rspec/cs/S3693_c#.json new file mode 100644 index 00000000000..0d8dfdef770 --- /dev/null +++ b/sonaranalyzer-dotnet/rspec/cs/S3693_c#.json @@ -0,0 +1,13 @@ +{ + "title": "Exception constructors should not throw exceptions", + "type": "BUG", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "15min" + }, + "tags": [ + + ], + "defaultSeverity": "Blocker" +} diff --git a/sonaranalyzer-dotnet/rspec/cs/Sonar_way_profile.json b/sonaranalyzer-dotnet/rspec/cs/Sonar_way_profile.json index 5e80d618bff..2d548f48bb9 100644 --- a/sonaranalyzer-dotnet/rspec/cs/Sonar_way_profile.json +++ b/sonaranalyzer-dotnet/rspec/cs/Sonar_way_profile.json @@ -149,6 +149,7 @@ "S3626", "S3649", "S3655", + "S3693", "S3776", "S3869", "S3871", diff --git a/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/RspecStrings.resx b/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/RspecStrings.resx index 72d39545ee8..d5693ccc631 100644 --- a/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/RspecStrings.resx +++ b/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/RspecStrings.resx @@ -5613,6 +5613,33 @@ BUG + + Sonar Bug + + + 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. + + + True + + + Constant/Issue + + + 15min + + + Blocker + + + + + + Exception constructors should not throw exceptions + + + BUG + Sonar Code Smell diff --git a/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/ExceptionConstructorShouldNotThrow.cs b/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/ExceptionConstructorShouldNotThrow.cs new file mode 100644 index 00000000000..961e69a9534 --- /dev/null +++ b/sonaranalyzer-dotnet/src/SonarAnalyzer.CSharp/Rules/ExceptionConstructorShouldNotThrow.cs @@ -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 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() + .Select(ctor => ctor.DescendantNodes().OfType().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); + } + } +} diff --git a/sonaranalyzer-dotnet/src/SonarAnalyzer.Utilities/Rules.Description/S3693.html b/sonaranalyzer-dotnet/src/SonarAnalyzer.Utilities/Rules.Description/S3693.html new file mode 100644 index 00000000000..426141212bb --- /dev/null +++ b/sonaranalyzer-dotnet/src/SonarAnalyzer.Utilities/Rules.Description/S3693.html @@ -0,0 +1,18 @@ +

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.

+

Noncompliant Code Example

+
+class MyException: Exception
+{
+    public void MyException()
+    {
+         if (bad_thing)
+         {
+             throw new Exception("A bad thing happened");  // Noncompliant
+          }
+    }
+}
+
+ diff --git a/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeTests.cs b/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeTests.cs index 0fa19cdb954..49318cb684b 100644 --- a/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeTests.cs +++ b/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/PackagingTests/RuleTypeTests.cs @@ -3690,7 +3690,7 @@ private static void DetectTypeChanges(ResourceManager resourceManager, IImmutabl //["3690"], //["3691"], //["3692"], - //["3693"], + ["3693"] = "BUG", //["3694"], //["3695"], //["3696"], diff --git a/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/Rules/ExceptionConstructorShouldNotThrowTest.cs b/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/Rules/ExceptionConstructorShouldNotThrowTest.cs new file mode 100644 index 00000000000..2d5ea80ba8d --- /dev/null +++ b/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/Rules/ExceptionConstructorShouldNotThrowTest.cs @@ -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()); + } + } +} diff --git a/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/TestCases/ExceptionConstructorShouldNotThrow.cs b/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/TestCases/ExceptionConstructorShouldNotThrow.cs new file mode 100644 index 00000000000..dedb87481a8 --- /dev/null +++ b/sonaranalyzer-dotnet/src/Tests/SonarAnalyzer.UnitTest/TestCases/ExceptionConstructorShouldNotThrow.cs @@ -0,0 +1,77 @@ +using System; + +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 + } + } + + class MyException5 : Exception + { + public MyException5() + { + var ex = new Exception(); + throw ex; // Noncompliant + } + } + + class Something + { + public Something() + { + throw new Exception(); // Compliant + } + } +}