Fix S2737 FP: Raised when exception filter is used (#9385)

SonarSource · Jun 4, 2024 · 1e02c88 · 1e02c88
1 parent c8fc28e
commit 1e02c88
Show file tree

Hide file tree

Showing 7 changed files with 104 additions and 133 deletions.
diff --git 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<SyntaxKind, CatchClauseSyntax>
 {
-    [DiagnosticAnalyzer(LanguageNames.CSharp)]
-    public sealed class CatchRethrow : CatchRethrowBase<CatchClauseSyntax>
-    {
-        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<SyntaxKind> 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<CatchClauseSyntax> 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
@@ -18,100 +18,69 @@
  * Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
  */
 
-namespace SonarAnalyzer.Rules
-{
-    public abstract class CatchRethrowBase<TCatchClause> : 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<TSyntaxKind, TCatchClause> : SonarDiagnosticAnalyzer<TSyntaxKind>
+    where TSyntaxKind : struct
+    where TCatchClause : SyntaxNode
+{
+    internal const string DiagnosticId = "S2737";
 
-        public override ImmutableArray<DiagnosticDescriptor> 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<TCatchClause> 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<INamedTypeSymbol[]>(() => ComputeExceptionTypes(catches, context.SemanticModel));
+        var redundantCatches = new HashSet<TCatchClause>();
+        // 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<List<INamedTypeSymbol>>(() =>
-                ComputeExceptionTypesIfNeeded(catches, context.SemanticModel));
-            var redundantCatches = new HashSet<TCatchClause>();
-
-            // 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<TCatchClause> catches,
-            List<INamedTypeSymbol> caughtExceptionTypes, ISet<TCatchClause> 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<INamedTypeSymbol> ComputeExceptionTypesIfNeeded(IEnumerable<TCatchClause> catches,
-            SemanticModel semanticModel)
+    private static bool IsRedundantToFollowingCatches(int catchIndex, TCatchClause[] catches, INamedTypeSymbol[] caughtExceptionTypes, ISet<TCatchClause> 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<TCatchClause> 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
@@ -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<SyntaxKind, CatchBlockSyntax>
 {
-    [DiagnosticAnalyzer(LanguageNames.VisualBasic)]
-    public sealed class CatchRethrow : CatchRethrowBase<CatchBlockSyntax>
-    {
-        private static readonly SyntaxList<StatementSyntax> ThrowBlock = new SyntaxList<StatementSyntax>().Add(SyntaxFactory.ThrowStatement());
+    private static readonly SyntaxList<ThrowStatementSyntax> ThrowBlock = SyntaxFactory.List([SyntaxFactory.ThrowStatement()]);
 
-        protected override DiagnosticDescriptor Rule { get; } =
-            DescriptorFactory.Create(DiagnosticId, MessageFormat);
+    protected override ILanguageFacade<SyntaxKind> 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<CatchBlockSyntax> 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
@@ -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<CS.CatchRethrow>();
+    private readonly VerifierBuilder builderCS = new VerifierBuilder<CS.CatchRethrow>();
 
-        [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<CS.CatchRethrowCodeFix>()
-                .WithCodeFixedPaths("CatchRethrow.Fixed.cs")
-                .VerifyCodeFix();
+    [TestMethod]
+    public void CatchRethrow_CodeFix() =>
+        builderCS.AddPaths("CatchRethrow.cs")
+            .WithCodeFix<CS.CatchRethrowCodeFix>()
+            .WithCodeFixedPaths("CatchRethrow.Fixed.cs")
+            .VerifyCodeFix();
 
-        [TestMethod]
-        public void CatchRethrow_VB() =>
-            new VerifierBuilder<VB.CatchRethrow>().AddPaths("CatchRethrow.vb").Verify();
-    }
+    [TestMethod]
+    public void CatchRethrow_VB() =>
+        new VerifierBuilder<VB.CatchRethrow>().AddPaths("CatchRethrow.vb").Verify();
 }
diff --git 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
@@ -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
@@ -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