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

…iables overlap. (dotnet#1039)

Co-authored-by: Josef Pihrt <josef@pihrt.net>

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()
+ {
+ }
+}
 ");
  }
 }