diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/CatchRethrow.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/CatchRethrow.cs index bbc629fb829..c210fe892b8 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/CatchRethrow.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/CatchRethrow.cs @@ -18,29 +18,27 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ -namespace SonarAnalyzer.Rules.CSharp +namespace SonarAnalyzer.Rules.CSharp; + +[DiagnosticAnalyzer(LanguageNames.CSharp)] +public sealed class CatchRethrow : CatchRethrowBase { - [DiagnosticAnalyzer(LanguageNames.CSharp)] - public sealed class CatchRethrow : CatchRethrowBase - { - private static readonly BlockSyntax ThrowBlock = SyntaxFactory.Block(SyntaxFactory.ThrowStatement()); + private static readonly BlockSyntax ThrowBlock = SyntaxFactory.Block(SyntaxFactory.ThrowStatement()); - protected override DiagnosticDescriptor Rule { get; } = - DescriptorFactory.Create(DiagnosticId, MessageFormat); + protected override ILanguageFacade Language => CSharpFacade.Instance; - protected override bool ContainsOnlyThrow(CatchClauseSyntax currentCatch) => - CSharpEquivalenceChecker.AreEquivalent(currentCatch.Block, ThrowBlock); + protected override bool ContainsOnlyThrow(CatchClauseSyntax currentCatch) => + CSharpEquivalenceChecker.AreEquivalent(currentCatch.Block, ThrowBlock); - protected override IReadOnlyList GetCatches(SyntaxNode syntaxNode) => - ((TryStatementSyntax)syntaxNode).Catches; + protected override CatchClauseSyntax[] AllCatches(SyntaxNode node) => + ((TryStatementSyntax)node).Catches.ToArray(); - protected override SyntaxNode GetDeclarationType(CatchClauseSyntax catchClause) => - catchClause.Declaration?.Type; + protected override SyntaxNode DeclarationType(CatchClauseSyntax catchClause) => + catchClause.Declaration?.Type; - protected override bool HasFilter(CatchClauseSyntax catchClause) => - catchClause.Filter != null; + protected override bool HasFilter(CatchClauseSyntax catchClause) => + catchClause.Filter is not null; - protected override void Initialize(SonarAnalysisContext context) => - context.RegisterNodeAction(RaiseOnInvalidCatch, SyntaxKind.TryStatement); - } + protected override void Initialize(SonarAnalysisContext context) => + context.RegisterNodeAction(RaiseOnInvalidCatch, SyntaxKind.TryStatement); } diff --git a/analyzers/src/SonarAnalyzer.Common/Rules/CatchRethrowBase.cs b/analyzers/src/SonarAnalyzer.Common/Rules/CatchRethrowBase.cs index 17f91b3af54..2085a7e26a9 100644 --- a/analyzers/src/SonarAnalyzer.Common/Rules/CatchRethrowBase.cs +++ b/analyzers/src/SonarAnalyzer.Common/Rules/CatchRethrowBase.cs @@ -18,100 +18,69 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ -namespace SonarAnalyzer.Rules -{ - public abstract class CatchRethrowBase : SonarDiagnosticAnalyzer - where TCatchClause : SyntaxNode - { - internal const string DiagnosticId = "S2737"; - protected const string MessageFormat = "Add logic to this catch clause or eliminate it and rethrow the exception automatically."; +namespace SonarAnalyzer.Rules; - protected abstract DiagnosticDescriptor Rule { get; } +public abstract class CatchRethrowBase : SonarDiagnosticAnalyzer + where TSyntaxKind : struct + where TCatchClause : SyntaxNode +{ + internal const string DiagnosticId = "S2737"; - public override ImmutableArray SupportedDiagnostics => - ImmutableArray.Create(Rule); + protected abstract TCatchClause[] AllCatches(SyntaxNode node); + protected abstract SyntaxNode DeclarationType(TCatchClause catchClause); + protected abstract bool HasFilter(TCatchClause catchClause); + protected abstract bool ContainsOnlyThrow(TCatchClause currentCatch); - protected abstract IReadOnlyList GetCatches(SyntaxNode syntaxNode); + protected override string MessageFormat => "Add logic to this catch clause or eliminate it and rethrow the exception automatically."; - protected abstract bool HasFilter(TCatchClause catchClause); + protected CatchRethrowBase() : base(DiagnosticId) { } - protected abstract SyntaxNode GetDeclarationType(TCatchClause catchClause); + protected void RaiseOnInvalidCatch(SonarSyntaxNodeReportingContext context) + { + var catches = AllCatches(context.Node); + var caughtExceptionTypes = new Lazy(() => ComputeExceptionTypes(catches, context.SemanticModel)); + var redundantCatches = new HashSet(); + // We handle differently redundant catch clauses (just throw inside) that are followed by a non-redundant catch clause, because if they are removed, the method behavior will change. + var followingCatchesOnlyThrow = true; - protected void RaiseOnInvalidCatch(SonarSyntaxNodeReportingContext context) + for (var i = catches.Length - 1; i >= 0; i--) { - var catches = GetCatches(context.Node); - var caughtExceptionTypes = new Lazy>(() => - ComputeExceptionTypesIfNeeded(catches, context.SemanticModel)); - var redundantCatches = new HashSet(); - - // We handle differently redundant catch clauses (just throw inside) that are - // followed by a non-redundant catch clause, because if they are removed, the - // method behavior will change. - var foundNotRedundantCatch = false; - - for (var i = catches.Count - 1; i >= 0; i--) + var currentCatch = catches[i]; + if (ContainsOnlyThrow(currentCatch)) { - var currentCatch = catches[i]; - if (ContainsOnlyThrow(currentCatch)) - { - if (foundNotRedundantCatch) - { - // Make sure we report only catch clauses that will not change - // the method behavior if removed. - if (!HasFilter(currentCatch) && - !IsMoreSpecificTypeThanANotRedundantCatch(i, catches, caughtExceptionTypes.Value, redundantCatches)) - { - redundantCatches.Add(currentCatch); - } - } - else - { - redundantCatches.Add(currentCatch); - } - } - else + if (!HasFilter(currentCatch) + // Make sure we report only catch clauses that will not change the method behavior if removed. + && (followingCatchesOnlyThrow || IsRedundantToFollowingCatches(i, catches, caughtExceptionTypes.Value, redundantCatches))) { - foundNotRedundantCatch = true; + redundantCatches.Add(currentCatch); } } - - foreach (var redundantCatch in redundantCatches) + else { - context.ReportIssue(Diagnostic.Create(Rule, redundantCatch.GetLocation())); + followingCatchesOnlyThrow = false; } } - protected abstract bool ContainsOnlyThrow(TCatchClause currentCatch); - - private static bool IsMoreSpecificTypeThanANotRedundantCatch(int catchIndex, IReadOnlyList catches, - List caughtExceptionTypes, ISet redundantCatches) + foreach (var redundantCatch in redundantCatches) { - var currentType = caughtExceptionTypes[catchIndex]; - for (var i = catchIndex + 1; i < caughtExceptionTypes.Count; i++) - { - var nextCatchType = caughtExceptionTypes[i]; - - if (nextCatchType == null || - currentType.DerivesOrImplements(nextCatchType)) - { - return !redundantCatches.Contains(catches[i]); - } - } - return false; + context.ReportIssue(Rule, redundantCatch); } + } - private List ComputeExceptionTypesIfNeeded(IEnumerable catches, - SemanticModel semanticModel) + private static bool IsRedundantToFollowingCatches(int catchIndex, TCatchClause[] catches, INamedTypeSymbol[] caughtExceptionTypes, ISet redundantCatches) + { + var currentType = caughtExceptionTypes[catchIndex]; + for (var i = catchIndex + 1; i < caughtExceptionTypes.Length; i++) { - return catches - .Select(clause => - { - var declarationType = GetDeclarationType(clause); - return declarationType != null - ? semanticModel.GetTypeInfo(declarationType).Type as INamedTypeSymbol - : null; - }) - .ToList(); + var nextCatchType = caughtExceptionTypes[i]; + if (nextCatchType is null || currentType.DerivesOrImplements(nextCatchType)) + { + return redundantCatches.Contains(catches[i]); + } } + return true; } + + private INamedTypeSymbol[] ComputeExceptionTypes(IEnumerable catches, SemanticModel model) => + catches.Select(x => DeclarationType(x) is { } declarationType ? model.GetTypeInfo(declarationType).Type as INamedTypeSymbol : null).ToArray(); } diff --git a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/CatchRethrow.cs b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/CatchRethrow.cs index 2230cfc009a..f369c861064 100644 --- a/analyzers/src/SonarAnalyzer.VisualBasic/Rules/CatchRethrow.cs +++ b/analyzers/src/SonarAnalyzer.VisualBasic/Rules/CatchRethrow.cs @@ -18,29 +18,27 @@ * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. */ -namespace SonarAnalyzer.Rules.VisualBasic +namespace SonarAnalyzer.Rules.VisualBasic; + +[DiagnosticAnalyzer(LanguageNames.VisualBasic)] +public sealed class CatchRethrow : CatchRethrowBase { - [DiagnosticAnalyzer(LanguageNames.VisualBasic)] - public sealed class CatchRethrow : CatchRethrowBase - { - private static readonly SyntaxList ThrowBlock = new SyntaxList().Add(SyntaxFactory.ThrowStatement()); + private static readonly SyntaxList ThrowBlock = SyntaxFactory.List([SyntaxFactory.ThrowStatement()]); - protected override DiagnosticDescriptor Rule { get; } = - DescriptorFactory.Create(DiagnosticId, MessageFormat); + protected override ILanguageFacade Language => VisualBasicFacade.Instance; - protected override bool ContainsOnlyThrow(CatchBlockSyntax currentCatch) => - VisualBasicEquivalenceChecker.AreEquivalent(currentCatch.Statements, ThrowBlock); + protected override bool ContainsOnlyThrow(CatchBlockSyntax currentCatch) => + VisualBasicEquivalenceChecker.AreEquivalent(currentCatch.Statements, ThrowBlock); - protected override IReadOnlyList GetCatches(SyntaxNode syntaxNode) => - ((TryBlockSyntax)syntaxNode).CatchBlocks; + protected override CatchBlockSyntax[] AllCatches(SyntaxNode node) => + ((TryBlockSyntax)node).CatchBlocks.ToArray(); - protected override SyntaxNode GetDeclarationType(CatchBlockSyntax catchClause) => - catchClause.CatchStatement?.AsClause?.Type; + protected override SyntaxNode DeclarationType(CatchBlockSyntax catchClause) => + catchClause.CatchStatement?.AsClause?.Type; - protected override bool HasFilter(CatchBlockSyntax catchClause) => - catchClause.CatchStatement?.WhenClause != null; + protected override bool HasFilter(CatchBlockSyntax catchClause) => + catchClause.CatchStatement?.WhenClause is not null; - protected override void Initialize(SonarAnalysisContext context) => - context.RegisterNodeAction(RaiseOnInvalidCatch, SyntaxKind.TryBlock); - } + protected override void Initialize(SonarAnalysisContext context) => + context.RegisterNodeAction(RaiseOnInvalidCatch, SyntaxKind.TryBlock); } diff --git a/analyzers/tests/SonarAnalyzer.Test/Rules/CatchRethrowTest.cs b/analyzers/tests/SonarAnalyzer.Test/Rules/CatchRethrowTest.cs index 409475c2af2..18c7e182386 100644 --- a/analyzers/tests/SonarAnalyzer.Test/Rules/CatchRethrowTest.cs +++ b/analyzers/tests/SonarAnalyzer.Test/Rules/CatchRethrowTest.cs @@ -21,26 +21,25 @@ using CS = SonarAnalyzer.Rules.CSharp; using VB = SonarAnalyzer.Rules.VisualBasic; -namespace SonarAnalyzer.Test.Rules +namespace SonarAnalyzer.Test.Rules; + +[TestClass] +public class CatchRethrowTest { - [TestClass] - public class CatchRethrowTest - { - private readonly VerifierBuilder builderCS = new VerifierBuilder(); + private readonly VerifierBuilder builderCS = new VerifierBuilder(); - [TestMethod] - public void CatchRethrow() => - builderCS.AddPaths("CatchRethrow.cs").Verify(); + [TestMethod] + public void CatchRethrow() => + builderCS.AddPaths("CatchRethrow.cs").Verify(); - [TestMethod] - public void CatchRethrow_CodeFix() => - builderCS.AddPaths("CatchRethrow.cs") - .WithCodeFix() - .WithCodeFixedPaths("CatchRethrow.Fixed.cs") - .VerifyCodeFix(); + [TestMethod] + public void CatchRethrow_CodeFix() => + builderCS.AddPaths("CatchRethrow.cs") + .WithCodeFix() + .WithCodeFixedPaths("CatchRethrow.Fixed.cs") + .VerifyCodeFix(); - [TestMethod] - public void CatchRethrow_VB() => - new VerifierBuilder().AddPaths("CatchRethrow.vb").Verify(); - } + [TestMethod] + public void CatchRethrow_VB() => + new VerifierBuilder().AddPaths("CatchRethrow.vb").Verify(); } diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/CatchRethrow.Fixed.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/CatchRethrow.Fixed.cs index b8340b59a79..b7dd43e1f59 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/CatchRethrow.Fixed.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/CatchRethrow.Fixed.cs @@ -98,7 +98,14 @@ public class Repro8199 public void CatchWithFilter() { - SomeMethod(); + try + { + SomeMethod(); + } + catch (Exception ex) when (LogException(ex)) + { + throw; + } } } } diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/CatchRethrow.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/CatchRethrow.cs index 1c5adaf21cf..00f7a8308b7 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/CatchRethrow.cs +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/CatchRethrow.cs @@ -156,7 +156,7 @@ public void CatchWithFilter() { SomeMethod(); } - catch (Exception ex) when (LogException(ex)) // Noncompliant - FP + catch (Exception ex) when (LogException(ex)) { throw; } diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/CatchRethrow.vb b/analyzers/tests/SonarAnalyzer.Test/TestCases/CatchRethrow.vb index a7ba28d1edb..95f172f0f6a 100644 --- a/analyzers/tests/SonarAnalyzer.Test/TestCases/CatchRethrow.vb +++ b/analyzers/tests/SonarAnalyzer.Test/TestCases/CatchRethrow.vb @@ -58,7 +58,7 @@ Namespace Tests.TestCases Try doSomething() - Catch exc As ArgumentException When True 'Noncompliant + Catch exc As ArgumentException When True Throw End Try @@ -110,7 +110,7 @@ Namespace Tests.TestCases Public Sub CatchWithFilter() Try SomeMethod() - Catch ex As Exception When LogException(ex) ' Noncompliant - FP + Catch ex As Exception When LogException(ex) Throw End Try End Sub