Skip to content

Commit

Permalink
Fix/RCS1031, RCS1208 and RCS1061 (#1062)
Browse files Browse the repository at this point in the history
Co-authored-by: Josef Pihrt <josef@pihrt.net>
  • Loading branch information
jamesHargreaves12 and josefpihrt committed Apr 21, 2023
1 parent e017499 commit 3b534ee
Show file tree
Hide file tree
Showing 11 changed files with 823 additions and 61 deletions.
2 changes: 1 addition & 1 deletion ChangeLog.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)).
Expand Down
5 changes: 4 additions & 1 deletion src/Analyzers/CSharp/Analysis/MergeIfWithNestedIfAnalyzer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
}

Expand Down
Original file line number Diff line number Diff line change
@@ -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
};
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
28 changes: 14 additions & 14 deletions src/CSharp.Workspaces/CSharp/SyntaxLogicalInverter.cs
Original file line number Diff line number Diff line change
Expand Up @@ -209,22 +209,22 @@ internal static SyntaxLogicalInverter GetInstance(Document document)
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()}'");
Expand Down
Original file line number Diff line number Diff line change
@@ -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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@

using System.Collections.Immutable;
using System.Diagnostics;
using System.Linq;
using System.Threading;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
Expand Down Expand Up @@ -73,7 +72,7 @@ private static ReduceIfNestingAnalysisResult Fail(SyntaxNode topNode)
return Fail(switchSection);
}

if (LocallyDeclaredVariablesOverlapWithOuterScope(ifStatement, switchSection, semanticModel))
if (IfStatementLocalVariableAnalysis.DoDeclaredVariablesOverlapWithOuterScope(ifStatement, semanticModel))
return Fail(switchSection);

return Success(jumpKind, switchSection);
Expand Down Expand Up @@ -109,7 +108,7 @@ private static ReduceIfNestingAnalysisResult Fail(SyntaxNode topNode)
return Fail(parent);
}

if (LocallyDeclaredVariablesOverlapWithOuterScope(ifStatement, parent, semanticModel))
if (IfStatementLocalVariableAnalysis.DoDeclaredVariablesOverlapWithOuterScope(ifStatement, semanticModel))
return Fail(parent);

return Success(jumpKind, parent);
Expand All @@ -135,7 +134,7 @@ private static ReduceIfNestingAnalysisResult Fail(SyntaxNode topNode)
return Fail(parent);
}

if (LocallyDeclaredVariablesOverlapWithOuterScope(ifStatement, parent, semanticModel))
if (IfStatementLocalVariableAnalysis.DoDeclaredVariablesOverlapWithOuterScope(ifStatement, semanticModel))
return Fail(parent);

return Success(jumpKind, parent);
Expand All @@ -147,7 +146,7 @@ private static ReduceIfNestingAnalysisResult Fail(SyntaxNode topNode)
if (jumpKind == SyntaxKind.None)
return Fail(parent);

if (LocallyDeclaredVariablesOverlapWithOuterScope(ifStatement, parent, semanticModel))
if (IfStatementLocalVariableAnalysis.DoDeclaredVariablesOverlapWithOuterScope(ifStatement, semanticModel))
return Fail(parent);

return Success(jumpKind, parent);
Expand All @@ -156,7 +155,7 @@ private static ReduceIfNestingAnalysisResult Fail(SyntaxNode topNode)
{
var methodDeclaration = (MethodDeclarationSyntax)parent;

if (LocallyDeclaredVariablesOverlapWithOuterScope(ifStatement, methodDeclaration.Body, semanticModel))
if (IfStatementLocalVariableAnalysis.DoDeclaredVariablesOverlapWithOuterScope(ifStatement, semanticModel))
return Fail(parent);

if (jumpKind != SyntaxKind.None)
Expand Down Expand Up @@ -190,7 +189,7 @@ private static ReduceIfNestingAnalysisResult Fail(SyntaxNode topNode)
{
var localFunction = (LocalFunctionStatementSyntax)parent;

if (LocallyDeclaredVariablesOverlapWithOuterScope(ifStatement, localFunction.Body, semanticModel))
if (IfStatementLocalVariableAnalysis.DoDeclaredVariablesOverlapWithOuterScope(ifStatement, semanticModel))
return Fail(parent);

if (jumpKind != SyntaxKind.None)
Expand Down Expand Up @@ -224,7 +223,7 @@ private static ReduceIfNestingAnalysisResult Fail(SyntaxNode topNode)
{
var anonymousFunction = (AnonymousFunctionExpressionSyntax)parent;

if (LocallyDeclaredVariablesOverlapWithOuterScope(ifStatement, anonymousFunction.Block, semanticModel))
if (IfStatementLocalVariableAnalysis.DoDeclaredVariablesOverlapWithOuterScope(ifStatement, semanticModel))
return Fail(parent);

if (jumpKind != SyntaxKind.None)
Expand Down Expand Up @@ -283,27 +282,6 @@ private static ReduceIfNestingAnalysisResult Fail(SyntaxNode topNode)
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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}
}
}
");
}
}
Loading

0 comments on commit 3b534ee

Please sign in to comment.