Fix/RCS1031, RCS1208 and RCS1061 (#1062)

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

ChangeLog.md

@@ -27,7 +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)).
+- Do not remove braces in the cases where there are overlapping local variables. ([RCS1031](https://github.com/JosefPihrt/Roslynator/blob/main/docs/analyzers/RCS1031.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), [RCS1061](https://github.com/JosefPihrt/Roslynator/blob/main/docs/analyzers/RCS1061.md)) ([#1039](https://github.com/JosefPihrt/Roslynator/pull/1039),[#1062](https://github.com/JosefPihrt/Roslynator/pull/1062)).
 - [CLI] Analyze command does not create the XML output file and returns incorrect exit code when only compiler diagnostics are reported ([#1056](https://github.com/JosefPihrt/Roslynator/pull/1056) by @PeterKaszab).
 - [CLI] Fix exit code when multiple projects are processed ([#1061](https://github.com/JosefPihrt/Roslynator/pull/1061) by @PeterKaszab).
 - Fix code fix for CS0164 ([#1031](https://github.com/JosefPihrt/Roslynator/pull/1031)).

src/Analyzers/CSharp/Analysis/MergeIfWithNestedIfAnalyzer.cs

@@ -7,7 +7,7 @@
 using Microsoft.CodeAnalysis.CSharp.Syntax;
 using Microsoft.CodeAnalysis.Diagnostics;
 using Microsoft.CodeAnalysis.Text;
-using Roslynator.CSharp;
+using Roslynator.CSharp.Analysis.ReduceIfNesting;
 using Roslynator.CSharp.Syntax;
 
 namespace Roslynator.CSharp.Analysis;
@@ -72,6 +72,9 @@ private static void AnalyzeIfStatement(SyntaxNodeAnalysisContext context)
         if (!CheckTrivia(ifStatement, nestedIf.IfStatement))
             return;
 
+        if (IfStatementLocalVariableAnalysis.DoDeclaredVariablesOverlapWithOuterScope(ifStatement, context.SemanticModel))
+            return;
+
         ReportDiagnostic(context, ifStatement, nestedIf.IfStatement);
     }
 

src/Analyzers/CSharp/Analysis/PattenMatchingAnalysis/PatternMatchingVariableDeclarationHelper.cs

@@ -0,0 +1,38 @@
+// 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 Microsoft.CodeAnalysis.CSharp.Syntax;
+
+namespace Roslynator.CSharp.Analysis;
+
+public static class PatternMatchingVariableDeclarationHelper
+{
+    public static bool AnyDeclaredVariablesMatch(PatternSyntax pattern, ImmutableHashSet<string> variableNames)
+    {
+        return pattern switch
+        {
+            RecursivePatternSyntax { PositionalPatternClause: var positionalPatternClause, PropertyPatternClause: var propertyPatternClause, Designation: var designation } =>
+                (designation is not null && AnyDeclaredVariablesMatch(designation, variableNames))
+                    || (propertyPatternClause?.Subpatterns.Any(p => AnyDeclaredVariablesMatch(p.Pattern, variableNames)) ?? false)
+                    || (positionalPatternClause?.Subpatterns.Any(p => AnyDeclaredVariablesMatch(p.Pattern, variableNames)) ?? false),
+            BinaryPatternSyntax binaryPattern =>
+                AnyDeclaredVariablesMatch(binaryPattern.Left, variableNames)
+                    || AnyDeclaredVariablesMatch(binaryPattern.Right, variableNames),
+            ParenthesizedPatternSyntax parenthesizedPattern => AnyDeclaredVariablesMatch(parenthesizedPattern.Pattern, variableNames),
+            DeclarationPatternSyntax { Designation: var variableDesignation } => AnyDeclaredVariablesMatch(variableDesignation, variableNames),
+            VarPatternSyntax { Designation: var variableDesignation } => AnyDeclaredVariablesMatch(variableDesignation, variableNames),
+            _ => false
+        };
+    }
+
+    internal static bool AnyDeclaredVariablesMatch(VariableDesignationSyntax designation, ImmutableHashSet<string> variableNames)
+    {
+        return designation switch
+        {
+            SingleVariableDesignationSyntax singleVariableDesignation => variableNames.Contains(singleVariableDesignation.Identifier.ValueText),
+            ParenthesizedVariableDesignationSyntax parenthesizedVariableDesignation => parenthesizedVariableDesignation.Variables.Any(variable => AnyDeclaredVariablesMatch(variable, variableNames)),
+            DiscardDesignationSyntax _ => false,
+            _ => false
+        };
+    }
+}

src/Analyzers/CSharp/Analysis/RemoveUnnecessaryBracesInSwitchSectionAnalyzer.cs

@@ -121,18 +121,26 @@ private static bool LocallyDeclaredVariablesOverlapWithAnyOtherSwitchSections(Sw
 
         foreach (SwitchSectionSyntax 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)
+            if (otherSection.Span.Contains(switchBlock.Span))
                 continue;
 
-            if (otherBlock.Span == switchBlock.Span)
-                continue;
-
-            foreach (ISymbol v in semanticModel.AnalyzeDataFlow(otherBlock)!.VariablesDeclared)
+            foreach (SwitchLabelSyntax label in otherSection.Labels)
             {
-                if (sectionDeclaredVariablesNames.Contains(v.Name))
+                if (label is not CasePatternSwitchLabelSyntax casePatternSwitchLabel)
+                    continue;
+
+                if (PatternMatchingVariableDeclarationHelper.AnyDeclaredVariablesMatch(casePatternSwitchLabel.Pattern, sectionDeclaredVariablesNames))
                     return true;
             }
+
+            foreach (StatementSyntax statement in otherSection.Statements)
+            {
+                foreach (ISymbol symbol in semanticModel.AnalyzeDataFlow(statement)!.VariablesDeclared)
+                {
+                    if (sectionDeclaredVariablesNames.Contains(symbol.Name))
+                        return true;
+                }
+            }
         }
 
         return false;

src/CSharp.Workspaces/CSharp/SyntaxLogicalInverter.cs

@@ -209,22 +209,22 @@ private ExpressionSyntax LogicallyInvertImpl(
                     return DefaultInvert(expression);
                 }
             case SyntaxKind.CoalesceExpression:
-            {
-                var binaryExpression = (BinaryExpressionSyntax)expression;
-                if (binaryExpression.Right.IsKind(SyntaxKind.FalseLiteralExpression))
-                {
-                    // !(x ?? false) === (x != true)
-                    return NotEqualsExpression(binaryExpression.Left, TrueLiteralExpression());
-                }
-
-                if (binaryExpression.Right.IsKind(SyntaxKind.TrueLiteralExpression))
                 {
-                    // !(x ?? true) === (x == false)
-                    return EqualsExpression(binaryExpression.Left, FalseLiteralExpression());
-                }
+                    var binaryExpression = (BinaryExpressionSyntax)expression;
+                    if (binaryExpression.Right.IsKind(SyntaxKind.FalseLiteralExpression))
+                    {
+                        // !(x ?? false) === (x != true)
+                        return NotEqualsExpression(binaryExpression.Left, TrueLiteralExpression());
+                    }
+
+                    if (binaryExpression.Right.IsKind(SyntaxKind.TrueLiteralExpression))
+                    {
+                        // !(x ?? true) === (x == false)
+                        return EqualsExpression(binaryExpression.Left, FalseLiteralExpression());
+                    }
 
-                return DefaultInvert(expression);
-            }
+                    return DefaultInvert(expression);
+                }
         }
 
         Debug.Fail($"Logical inversion of unknown kind '{expression.Kind()}'");

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

@@ -0,0 +1,40 @@
+// 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 Microsoft.CodeAnalysis;
+using Microsoft.CodeAnalysis.CSharp;
+using Microsoft.CodeAnalysis.CSharp.Syntax;
+
+namespace Roslynator.CSharp.Analysis.ReduceIfNesting;
+
+internal static class IfStatementLocalVariableAnalysis
+{
+    public static bool DoDeclaredVariablesOverlapWithOuterScope(
+        IfStatementSyntax ifStatement,
+        SemanticModel semanticModel
+    )
+    {
+        ImmutableArray<ISymbol> ifVariablesDeclared = semanticModel.AnalyzeDataFlow(ifStatement)!
+            .VariablesDeclared;
+
+        if (ifVariablesDeclared.IsEmpty)
+            return false;
+
+        foreach (StatementSyntax statement in SyntaxInfo.StatementListInfo(ifStatement).Statements)
+        {
+            if (statement == ifStatement)
+                continue;
+
+            foreach (ISymbol parentVariable in semanticModel.AnalyzeDataFlow(statement)!.VariablesDeclared)
+            {
+                foreach (ISymbol ifVariable in ifVariablesDeclared)
+                {
+                    if (ifVariable.Name == parentVariable.Name)
+                        return true;
+                }
+            }
+        }
+
+        return false;
+    }
+}

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

@@ -2,7 +2,6 @@
 
 using System.Collections.Immutable;
 using System.Diagnostics;
-using System.Linq;
 using System.Threading;
 using Microsoft.CodeAnalysis;
 using Microsoft.CodeAnalysis.CSharp;
@@ -73,7 +72,7 @@ private static ReduceIfNestingAnalysisResult AnalyzeCore(
                 return Fail(switchSection);
             }
 
-            if (LocallyDeclaredVariablesOverlapWithOuterScope(ifStatement, switchSection, semanticModel))
+            if (IfStatementLocalVariableAnalysis.DoDeclaredVariablesOverlapWithOuterScope(ifStatement, semanticModel))
                 return Fail(switchSection);
 
             return Success(jumpKind, switchSection);
@@ -109,7 +108,7 @@ private static ReduceIfNestingAnalysisResult AnalyzeCore(
                 return Fail(parent);
             }
 
-            if (LocallyDeclaredVariablesOverlapWithOuterScope(ifStatement, parent, semanticModel))
+            if (IfStatementLocalVariableAnalysis.DoDeclaredVariablesOverlapWithOuterScope(ifStatement, semanticModel))
                 return Fail(parent);
 
             return Success(jumpKind, parent);
@@ -135,7 +134,7 @@ private static ReduceIfNestingAnalysisResult AnalyzeCore(
                         return Fail(parent);
                     }
 
-                    if (LocallyDeclaredVariablesOverlapWithOuterScope(ifStatement, parent, semanticModel))
+                    if (IfStatementLocalVariableAnalysis.DoDeclaredVariablesOverlapWithOuterScope(ifStatement, semanticModel))
                         return Fail(parent);
 
                     return Success(jumpKind, parent);
@@ -147,7 +146,7 @@ private static ReduceIfNestingAnalysisResult AnalyzeCore(
                     if (jumpKind == SyntaxKind.None)
                         return Fail(parent);
 
-                    if (LocallyDeclaredVariablesOverlapWithOuterScope(ifStatement, parent, semanticModel))
+                    if (IfStatementLocalVariableAnalysis.DoDeclaredVariablesOverlapWithOuterScope(ifStatement, semanticModel))
                         return Fail(parent);
 
                     return Success(jumpKind, parent);
@@ -156,7 +155,7 @@ private static ReduceIfNestingAnalysisResult AnalyzeCore(
                 {
                     var methodDeclaration = (MethodDeclarationSyntax)parent;
 
-                    if (LocallyDeclaredVariablesOverlapWithOuterScope(ifStatement, methodDeclaration.Body, semanticModel))
+                    if (IfStatementLocalVariableAnalysis.DoDeclaredVariablesOverlapWithOuterScope(ifStatement, semanticModel))
                         return Fail(parent);
 
                     if (jumpKind != SyntaxKind.None)
@@ -190,7 +189,7 @@ private static ReduceIfNestingAnalysisResult AnalyzeCore(
                 {
                     var localFunction = (LocalFunctionStatementSyntax)parent;
 
-                    if (LocallyDeclaredVariablesOverlapWithOuterScope(ifStatement, localFunction.Body, semanticModel))
+                    if (IfStatementLocalVariableAnalysis.DoDeclaredVariablesOverlapWithOuterScope(ifStatement, semanticModel))
                         return Fail(parent);
 
                     if (jumpKind != SyntaxKind.None)
@@ -224,7 +223,7 @@ private static ReduceIfNestingAnalysisResult AnalyzeCore(
                 {
                     var anonymousFunction = (AnonymousFunctionExpressionSyntax)parent;
 
-                    if (LocallyDeclaredVariablesOverlapWithOuterScope(ifStatement, anonymousFunction.Block, semanticModel))
+                    if (IfStatementLocalVariableAnalysis.DoDeclaredVariablesOverlapWithOuterScope(ifStatement, semanticModel))
                         return Fail(parent);
 
                     if (jumpKind != SyntaxKind.None)
@@ -283,27 +282,6 @@ private static ReduceIfNestingAnalysisResult AnalyzeCore(
         return Fail(parent);
     }
 
-    private static bool LocallyDeclaredVariablesOverlapWithOuterScope(
-        IfStatementSyntax ifStatement,
-        SyntaxNode parent,
-        SemanticModel semanticModel
-    )
-    {
-        ImmutableArray<ISymbol> ifVariablesDeclared = semanticModel.AnalyzeDataFlow(ifStatement)!
-            .VariablesDeclared;
-
-        if (ifVariablesDeclared.IsEmpty)
-            return false;
-
-        ImmutableArray<ISymbol> 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

@@ -265,6 +265,66 @@ void M()
         }
     }
 }
+");
+    }
+
+    [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.RemoveUnnecessaryBracesInSwitchSection)]
+    public async Task TestNoDiagnostic_WhenOverlappingLocalVariableWithPatternMatchDeclaration()
+    {
+        await VerifyNoDiagnosticAsync(@"
+using System;
+
+class C
+{
+    void M()
+    {
+        object o = null;
+
+        switch (o)
+        {
+            case string s:
+                var x = 1;
+                break;
+            default:
+                {
+                    var s = 1;
+                    break;
+                }
+        }
+    }
+}
+");
+    }
+
+    [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.RemoveUnnecessaryBracesInSwitchSection)]
+    public async Task TestNoDiagnostic_WhenOverlappingLocalVariableWithRecursivePatternMatchDeclaration()
+    {
+        await VerifyNoDiagnosticAsync(@"
+using System;
+
+class C
+{
+    class Wrapper
+    {
+        public string S;
+    }
+    void M()
+    {
+        object o = null;
+
+        switch (o)
+        {
+            case Wrapper { S: var s }:
+                var x = 1;
+                break;
+            default:
+                {
+                    var s = 1;
+                    break;
+                }
+        }
+    }
+}
 ");
     }
 }