Fix S1871 FN: Chained else if without else (#9430)

SonarSource · Jun 14, 2024 · 93d54f7 · 93d54f7
1 parent 6037c1a
commit 93d54f7
Show file tree

Hide file tree

Showing 2 changed files with 74 additions and 15 deletions.
diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/ConditionalStructureSameImplementation.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/ConditionalStructureSameImplementation.cs
@@ -40,31 +40,23 @@ protected override void Initialize(SonarAnalysisContext context)
             c =>
             {
                 var ifStatement = (IfStatementSyntax)c.Node;
+                var precedingStatements = ifStatement.GetPrecedingStatementsInConditionChain().ToList();
+                var hasElse = HasLeafElseClause(ifStatement);
 
-                var precedingStatements = ifStatement
-                    .GetPrecedingStatementsInConditionChain()
-                    .ToList();
+                CheckStatement(c, ifStatement.Statement, precedingStatements, c.SemanticModel, hasElse, "branch");
 
-                CheckStatement(c, ifStatement.Statement, precedingStatements, c.SemanticModel, ifStatement.Else is not null, "branch");
-
-                if (ifStatement.Else is null)
+                if (ifStatement.Else is not null)
                 {
-                    return;
+                    CheckStatement(c, ifStatement.Else.Statement, [..precedingStatements, ifStatement.Statement], c.SemanticModel, hasElse, "branch");
                 }
-
-                precedingStatements.Add(ifStatement.Statement);
-                CheckStatement(c, ifStatement.Else.Statement, precedingStatements, c.SemanticModel, true, "branch");
             },
             SyntaxKind.IfStatement);
 
         context.RegisterNodeAction(
             c =>
             {
                 var switchSection = (SwitchSectionSyntax)c.Node;
-
-                var precedingSections = switchSection
-                    .GetPrecedingSections()
-                    .ToList();
+                var precedingSections = switchSection.GetPrecedingSections().ToList();
 
                 CheckStatement(c, switchSection, precedingSections, c.SemanticModel, HasDefaultClause((SwitchStatementSyntax)switchSection.Parent), "case");
             },
@@ -74,14 +66,23 @@ protected override void Initialize(SonarAnalysisContext context)
     private static bool HasDefaultClause(SwitchStatementSyntax switchStatement) =>
         switchStatement.Sections.SelectMany(x => x.Labels).Any(x => x.IsKind(SyntaxKind.DefaultSwitchLabel));
 
+    private static bool HasLeafElseClause(IfStatementSyntax ifStatement)
+    {
+        while (ifStatement.Else?.Statement is IfStatementSyntax elseIfStatement)
+        {
+            ifStatement = elseIfStatement;
+        }
+        return ifStatement.Else is not null;
+    }
+
     private static void CheckStatement(SonarSyntaxNodeReportingContext context, SyntaxNode node, IReadOnlyList<SyntaxNode> precedingBranches, SemanticModel model, bool hasElse, string discriminator)
     {
         var numberOfStatements = GetStatementsCount(node);
         if (!hasElse && numberOfStatements == 1)
         {
             if (precedingBranches.Any() && precedingBranches.All(x => AreEquivalentStatements(node, x, model)))
             {
-                ReportSyntaxNode(context, node, precedingBranches[0], discriminator);
+                ReportSyntaxNode(context, node, precedingBranches[precedingBranches.Count - 1], discriminator);
             }
         }
         else if (numberOfStatements > 1 && precedingBranches.FirstOrDefault(x => AreEquivalentStatements(node, x, model)) is { } equivalentStatement)

diff --git a/analyzers/tests/SonarAnalyzer.Test/TestCases/ConditionalStructureSameImplementation_If.cs b/analyzers/tests/SonarAnalyzer.Test/TestCases/ConditionalStructureSameImplementation_If.cs
@@ -127,6 +127,64 @@ public void Exception(int a)
                 DoSomething1();
             }
         }
+
+        public bool ElseIfChain(int s)
+        {
+            if (s == 0)
+            { // Secondary [IfChain1]
+                DoSomething1();
+                return true;
+            }
+            else if (s > 0 && s < 11)
+            { // Noncompliant [IfChain1]
+                DoSomething1();
+                return true;
+            }
+            else if (s > 11 && s < 20)
+            {
+                DoSomething2();
+                return true;
+            }
+
+            if (s == 0)
+            { // Secondary [IfChain2]
+                DoSomething1();
+            }
+            else if (s > 0 && s < 11)
+            { // Noncompliant [IfChain2]
+                // Secondary@-1 [IfChain3]
+                DoSomething1();
+            }
+            else if (s > 11 && s < 20)
+            { // Noncompliant [IfChain3]
+                DoSomething1();
+            }
+
+            if (s == 0)
+            {
+                DoSomething1();
+            }
+            else
+            {
+                if (s > 0 && s < 11)
+                { // FN [NestedIfChain]
+                    DoSomething1();
+                }
+                else
+                {
+                    if (s > 11 && s < 20)
+                    { // FN [NestedIfChain2]
+                        DoSomething1();
+                    }
+                    else
+                    {
+                        DoSomething2();
+                    }
+                }
+            }
+
+            return false;
+        }
     }
 
     public static class IntExtension