Stop RCS1031, RCS1208 and RCS1211 from being applied if the local var…

…iables overlap. (#1039)

Co-authored-by: Josef Pihrt <[email protected]>

ChangeLog.md

@@ -27,6 +27,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Fix ([RCS1206](https://github.com/JosefPihrt/Roslynator/blob/main/docs/analyzers/RCS1206.md)) ([#1049](https://github.com/JosefPihrt/Roslynator/pull/1049)).
 - Prevent possible recursion in ([RCS1235](https://github.com/JosefPihrt/Roslynator/blob/main/docs/analyzers/RCS1235.md)) ([#1054](https://github.com/JosefPihrt/Roslynator/pull/1054)).
 - Fix ([RCS1223](https://github.com/JosefPihrt/Roslynator/blob/main/docs/analyzers/RCS1223.md)) ([#1051](https://github.com/JosefPihrt/Roslynator/pull/1051)).
+- Do not remove braces in the cases where there are overlapping local variables. ([RCS1031](https://github.com/JosefPihrt/Roslynator/blob/main/docs/analyzers/RCS1013.md), [RCS1211](https://github.com/JosefPihrt/Roslynator/blob/main/docs/analyzers/RCS1211.md), [RCS1208](https://github.com/JosefPihrt/Roslynator/blob/main/docs/analyzers/RCS1208.md)) ([#1039](https://github.com/JosefPihrt/Roslynator/pull/1039)).
 
 ## [4.2.0] - 2022-11-27
 

src/Analyzers/CSharp/Analysis/RemoveUnnecessaryBracesInSwitchSectionAnalyzer.cs

@@ -1,6 +1,7 @@
 // Copyright (c) Josef Pihrt and Contributors. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System.Collections.Immutable;
+using System.Linq;
 using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.CSharp;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
@@ -90,6 +91,13 @@ public static void AnalyzerSwitchSection(SyntaxNodeAnalysisContext context)
         if (!AnalyzeTrivia(closeBrace.TrailingTrivia))
             return;
 
+        // If any of the other case blocks contain a definition for the same local variables then removing the braces would introduce a new error.
+        if (switchSection.Parent is SwitchStatementSyntax switchStatement
+            && LocallyDeclaredVariablesOverlapWithAnyOtherSwitchSections(switchStatement, block, context.SemanticModel))
+        {
+            return;
+        }
+
         DiagnosticHelpers.ReportDiagnostic(context, DiagnosticRules.RemoveUnnecessaryBracesInSwitchSection, openBrace);
         DiagnosticHelpers.ReportDiagnostic(context, DiagnosticRules.RemoveUnnecessaryBracesInSwitchSectionFadeOut, closeBrace);
 
@@ -98,4 +106,36 @@ static bool AnalyzeTrivia(SyntaxTriviaList trivia)
             return trivia.All(f => f.IsKind(SyntaxKind.WhitespaceTrivia, SyntaxKind.EndOfLineTrivia, SyntaxKind.SingleLineCommentTrivia));
         }
     }
+
+    private static bool LocallyDeclaredVariablesOverlapWithAnyOtherSwitchSections(SwitchStatementSyntax switchStatement, BlockSyntax switchBlock, SemanticModel semanticModel)
+    {
+        var sectionVariablesDeclared = semanticModel.AnalyzeDataFlow(switchBlock)!
+            .VariablesDeclared;
+
+        if (sectionVariablesDeclared.IsEmpty)
+            return false;
+
+        var sectionDeclaredVariablesNames = sectionVariablesDeclared
+            .Select(s => s.Name)
+            .ToImmutableHashSet();
+
+        foreach (var otherSection in switchStatement.Sections)
+        {
+            // If the other section is not a block then we do not need to check as if there were overlapping variables then there would already be a error.
+            if (otherSection.Statements.SingleOrDefault(shouldThrow: false) is not BlockSyntax otherBlock)
+                continue;
+
+            if (otherBlock.Span == switchBlock.Span)
+                continue;
+
+            foreach (var v in semanticModel.AnalyzeDataFlow(otherBlock)!.VariablesDeclared)
+            {
+                if (sectionDeclaredVariablesNames.Contains(v.Name))
+                    return true;
+            }
+        }
+        return false;
+    }
+
+
 }

src/Analyzers/CSharp/Analysis/RemoveUnnecessaryElseAnalyzer.cs

@@ -1,6 +1,8 @@
 // Copyright (c) Josef Pihrt and Contributors. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
+using System;
 using System.Collections.Immutable;
+using System.Linq;
 using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.CSharp;
 using Microsoft.CodeAnalysis.CSharp.Syntax;
@@ -38,13 +40,13 @@ public static void Analyze(SyntaxNodeAnalysisContext context)
         if (elseClause.ContainsDiagnostics)
             return;
 
-        if (!IsFixable(elseClause))
+        if (!IsFixable(elseClause, context.SemanticModel))
             return;
 
         DiagnosticHelpers.ReportDiagnostic(context, DiagnosticRules.RemoveUnnecessaryElse, elseClause.ElseKeyword);
     }
 
-    public static bool IsFixable(ElseClauseSyntax elseClause)
+    private static bool IsFixable(ElseClauseSyntax elseClause, SemanticModel semanticModel)
     {
         if (elseClause.Statement?.IsKind(SyntaxKind.IfStatement) != false)
             return false;
@@ -55,12 +57,44 @@ public static bool IsFixable(ElseClauseSyntax elseClause)
         if (!ifStatement.IsTopmostIf())
             return false;
 
-        StatementSyntax statement = ifStatement.Statement;
+        StatementSyntax ifStatementStatement = ifStatement.Statement;
 
-        if (statement is BlockSyntax block)
-            statement = block.Statements.LastOrDefault();
+        if (ifStatementStatement is not BlockSyntax ifBlock)
+            return CSharpFacts.IsJumpStatement(ifStatementStatement.Kind());
+
+        if (elseClause.Statement is BlockSyntax elseBlock && LocalDeclaredVariablesOverlap(elseBlock, ifBlock, semanticModel))
+            return false;
+
+        var lastStatementInIf = ifBlock.Statements.LastOrDefault();
+
+        return lastStatementInIf is not null
+               && CSharpFacts.IsJumpStatement(lastStatementInIf.Kind());
+    }
+
+    private static bool LocalDeclaredVariablesOverlap(BlockSyntax elseBlock, BlockSyntax ifBlock, SemanticModel semanticModel)
+    {        
+        var elseVariablesDeclared = semanticModel.AnalyzeDataFlow(elseBlock)!
+            .VariablesDeclared;
+
+        if (elseVariablesDeclared.IsEmpty)
+            return false;
+
+        var ifVariablesDeclared = semanticModel.AnalyzeDataFlow(ifBlock)!
+            .VariablesDeclared;
+
+        if (ifVariablesDeclared.IsEmpty)
+            return false;
+
+        var elseVariableNames = elseVariablesDeclared
+            .Select(s => s.Name)
+            .ToImmutableHashSet();
+
+        foreach (var v in ifVariablesDeclared)
+        {
+            if (elseVariableNames.Contains(v.Name))
+                return true;
+        }
 
-        return statement is not null
-            && CSharpFacts.IsJumpStatement(statement.Kind());
+        return false;
     }
 }

src/Common/CSharp/Analysis/ReduceIfNesting/ReduceIfNestingAnalysis.cs

@@ -1,6 +1,7 @@
 // Copyright (c) Josef Pihrt and Contributors. Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information.
 
 using System.Diagnostics;
+using System.Linq;
 using System.Threading;
 using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.CSharp;
@@ -71,6 +72,9 @@ private static ReduceIfNestingAnalysisResult AnalyzeCore(
                 return Fail(switchSection);
             }
 
+            if (LocallyDeclaredVariablesOverlapWithOuterScope(ifStatement, switchSection, semanticModel))
+                return Fail(switchSection);
+
             return Success(jumpKind, switchSection);
         }
 
@@ -104,6 +108,9 @@ private static ReduceIfNestingAnalysisResult AnalyzeCore(
                 return Fail(parent);
             }
 
+            if (LocallyDeclaredVariablesOverlapWithOuterScope(ifStatement, parent, semanticModel))
+                return Fail(parent);
+
             return Success(jumpKind, parent);
         }
 
@@ -126,6 +133,9 @@ private static ReduceIfNestingAnalysisResult AnalyzeCore(
                     {
                         return Fail(parent);
                     }
+
+                    if (LocallyDeclaredVariablesOverlapWithOuterScope(ifStatement, parent, semanticModel))
+                        return Fail(parent);
 
                     return Success(jumpKind, parent);
                 }
@@ -135,12 +145,18 @@ private static ReduceIfNestingAnalysisResult AnalyzeCore(
                 {
                     if (jumpKind == SyntaxKind.None)
                         return Fail(parent);
-
+
+                    if (LocallyDeclaredVariablesOverlapWithOuterScope(ifStatement, parent, semanticModel))
+                        return Fail(parent);
+
                     return Success(jumpKind, parent);
                 }
             case SyntaxKind.MethodDeclaration:
                 {
                     var methodDeclaration = (MethodDeclarationSyntax)parent;
+
+                    if (LocallyDeclaredVariablesOverlapWithOuterScope(ifStatement, methodDeclaration.Body, semanticModel))
+                        return Fail(parent);
 
                     if (jumpKind != SyntaxKind.None)
                         return Success(jumpKind, parent);
@@ -172,6 +188,9 @@ private static ReduceIfNestingAnalysisResult AnalyzeCore(
             case SyntaxKind.LocalFunctionStatement:
                 {
                     var localFunction = (LocalFunctionStatementSyntax)parent;
+
+                    if (LocallyDeclaredVariablesOverlapWithOuterScope(ifStatement, localFunction.Body, semanticModel))
+                        return Fail(parent);
 
                     if (jumpKind != SyntaxKind.None)
                         return Success(jumpKind, parent);
@@ -204,6 +223,9 @@ private static ReduceIfNestingAnalysisResult AnalyzeCore(
                 {
                     var anonymousFunction = (AnonymousFunctionExpressionSyntax)parent;
 
+                    if (LocallyDeclaredVariablesOverlapWithOuterScope(ifStatement, anonymousFunction.Block, semanticModel))
+                        return Fail(parent);
+
                     if (jumpKind != SyntaxKind.None)
                         return Success(jumpKind, parent);
 
@@ -260,6 +282,27 @@ private static ReduceIfNestingAnalysisResult AnalyzeCore(
         return Fail(parent);
     }
 
+    private static bool LocallyDeclaredVariablesOverlapWithOuterScope(
+        IfStatementSyntax ifStatement,
+        SyntaxNode parent,
+        SemanticModel semanticModel
+    )
+    {
+        var ifVariablesDeclared = semanticModel.AnalyzeDataFlow(ifStatement)!
+            .VariablesDeclared;
+
+        if (ifVariablesDeclared.IsEmpty)
+            return false;
+
+        var parentStatementDeclared = semanticModel.AnalyzeDataFlow(parent)!
+            .VariablesDeclared;
+
+        // The parent's declared variables will include those from the if and so we have to check for any symbols occurring twice.
+        return ifVariablesDeclared.Any(variable =>
+            parentStatementDeclared.Count(s => s.Name == variable.Name) > 1
+        );
+    }
+
     private static bool IsNestedFix(SyntaxNode node, SemanticModel semanticModel, ReduceIfNestingOptions options, CancellationToken cancellationToken)
     {
         options |= ReduceIfNestingOptions.AllowNestedFix;

src/Tests/Analyzers.Tests/RCS1031RemoveUnnecessaryBracesInSwitchSectionTests.cs

@@ -139,6 +139,57 @@ void M()
         }
     }
 }
+");
+    }
+
+    [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.RemoveUnnecessaryBracesInSwitchSection)]
+    public async Task Test_WithLocalVariablesThatDoNotOverlap()
+    {
+        await VerifyDiagnosticAndFixAsync(@"
+using System;
+
+class C
+{
+    void M()
+    {
+        string s = null;
+
+        switch (s)
+        {
+            case """":
+                [|{|]
+                    var x = 1;
+                    break;
+                }
+            default:
+                [|{|]
+                    var y = 1;
+                    break;
+                }
+        }
+    }
+}
+",@"
+using System;
+
+class C
+{
+    void M()
+    {
+        string s = null;
+
+        switch (s)
+        {
+            case """":
+                var x = 1;
+                break;
+
+            default:
+                var y = 1;
+                break;
+        }
+    }
+}
 ");
     }
 
@@ -184,6 +235,36 @@ void M()
         }
     }
 }
+");
+    }
+
+    [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.RemoveUnnecessaryBracesInSwitchSection)]
+    public async Task TestNoDiagnostic_WhenOverlappingLocalVariableDeclaration()
+    {
+        await VerifyNoDiagnosticAsync(@"
+using System;
+
+class C
+{
+    void M()
+    {
+        string s = null;
+
+        switch (s)
+        {
+            case """":
+                {
+                    var x = 1;
+                    break;
+                }
+            default:
+                {
+                    var x = 1;
+                    break;
+                }
+        }
+    }
+}
 ");
     }
 }

src/Tests/Analyzers.Tests/RCS1208ReduceIfNestingTests.cs

@@ -47,6 +47,35 @@ void M2()
     {
     }
 }
+");
+    }
+
+    [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.ReduceIfNesting)]
+    public async Task TestNoDiagnostic_OverlappingLocalVariables()
+    {
+        await VerifyNoDiagnosticAsync(@"
+class C
+{
+    void M(bool p, bool q)
+    {
+        if (p)
+        {
+            var x = 1;
+            M2();
+        }
+
+        if(q)
+        {
+            var x = 2;
+            M2();
+        }
+
+    }
+
+    void M2()
+    {
+    }
+}
 ");
     }
 }