diff --git a/ChangeLog.md b/ChangeLog.md index a9e92e26c2..956bbeb0c7 100644 --- a/ChangeLog.md +++ b/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 diff --git a/src/Analyzers/CSharp/Analysis/RemoveUnnecessaryBracesInSwitchSectionAnalyzer.cs b/src/Analyzers/CSharp/Analysis/RemoveUnnecessaryBracesInSwitchSectionAnalyzer.cs index ca2b9b695c..59e4ae08cf 100644 --- a/src/Analyzers/CSharp/Analysis/RemoveUnnecessaryBracesInSwitchSectionAnalyzer.cs +++ b/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; + } + + } diff --git a/src/Analyzers/CSharp/Analysis/RemoveUnnecessaryElseAnalyzer.cs b/src/Analyzers/CSharp/Analysis/RemoveUnnecessaryElseAnalyzer.cs index e38ef5e17c..de8bcd29b8 100644 --- a/src/Analyzers/CSharp/Analysis/RemoveUnnecessaryElseAnalyzer.cs +++ b/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; } } diff --git a/src/Common/CSharp/Analysis/ReduceIfNesting/ReduceIfNestingAnalysis.cs b/src/Common/CSharp/Analysis/ReduceIfNesting/ReduceIfNestingAnalysis.cs index 948e1f9d10..68484accf7 100644 --- a/src/Common/CSharp/Analysis/ReduceIfNesting/ReduceIfNestingAnalysis.cs +++ b/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; diff --git a/src/Tests/Analyzers.Tests/RCS1031RemoveUnnecessaryBracesInSwitchSectionTests.cs b/src/Tests/Analyzers.Tests/RCS1031RemoveUnnecessaryBracesInSwitchSectionTests.cs index d33f8f1ed0..e77d79ce4b 100644 --- a/src/Tests/Analyzers.Tests/RCS1031RemoveUnnecessaryBracesInSwitchSectionTests.cs +++ b/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; + } + } + } +} "); } } diff --git a/src/Tests/Analyzers.Tests/RCS1208ReduceIfNestingTests.cs b/src/Tests/Analyzers.Tests/RCS1208ReduceIfNestingTests.cs index 044f2856a4..c8a3df99c2 100644 --- a/src/Tests/Analyzers.Tests/RCS1208ReduceIfNestingTests.cs +++ b/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() + { + } +} "); } } diff --git a/src/Tests/Analyzers.Tests/RCS1211RemoveUnnecessaryElseTests.cs b/src/Tests/Analyzers.Tests/RCS1211RemoveUnnecessaryElseTests.cs new file mode 100644 index 0000000000..ad35b3753f --- /dev/null +++ b/src/Tests/Analyzers.Tests/RCS1211RemoveUnnecessaryElseTests.cs @@ -0,0 +1,71 @@ +// 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.Threading.Tasks; +using Microsoft.CodeAnalysis; +using Roslynator.CSharp.CodeFixes; +using Roslynator.Testing.CSharp; +using Xunit; + +namespace Roslynator.CSharp.Analysis.Tests; + +public class RCS1211RemoveUnnecessaryElseTests : AbstractCSharpDiagnosticVerifier +{ + public override DiagnosticDescriptor Descriptor { get; } = DiagnosticRules.RemoveUnnecessaryElse; + + [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.RemoveUnnecessaryElse)] + public async Task Test_UnnecessaryElse_Removed() + { + await VerifyDiagnosticAndFixAsync(@" +class C +{ + int M(bool flag) + { + if(flag) + { + return 1; + } + [|else|] + { + return 0; + } + } +} +", @" +class C +{ + int M(bool flag) + { + if(flag) + { + return 1; + } + + return 0; + } +} +"); + } + [Fact, Trait(Traits.Analyzer, DiagnosticIdentifiers.RemoveUnnecessaryElse)] + public async Task TestNoDiagnostic_OverlappingLocalVariables() + { + await VerifyNoDiagnosticAsync(@" +class C +{ + int M(bool flag) + { + if(flag) + { + var z = 1; + return z; + } + else + { + var z = 0; + return z; + } + } +} +"); + } + +}