From 0a50eb28669d865c18e5bf2218a89c2e5b24fa52 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Thu, 3 Aug 2023 10:02:34 +0200 Subject: [PATCH] Fix codeFix --- .../Extensions/SyntaxNodeExtensions.cs | 24 +++++++++++++++++++ .../Rules/BooleanLiteralUnnecessaryCodeFix.cs | 17 +++++-------- ...BooleanLiteralUnnecessary.CSharp9.Fixed.cs | 12 ++++++++++ .../BooleanLiteralUnnecessary.CSharp9.cs | 12 ++++++++++ .../BooleanLiteralUnnecessary.Fixed.cs | 7 ++++++ .../TestCases/BooleanLiteralUnnecessary.cs | 10 +++++--- 6 files changed, 68 insertions(+), 14 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Extensions/SyntaxNodeExtensions.cs b/analyzers/src/SonarAnalyzer.CSharp/Extensions/SyntaxNodeExtensions.cs index 9f76fd6f828..78f226f2ed4 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Extensions/SyntaxNodeExtensions.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Extensions/SyntaxNodeExtensions.cs @@ -449,6 +449,30 @@ private static string GetUnknownType(SyntaxKind kind) => #endif + public static bool IsTrue(this SyntaxNode node) => + node switch + { + { RawKind: (int)SyntaxKind.TrueLiteralExpression } => true, // true + { RawKind: (int)SyntaxKind.LogicalNotExpression } => IsFalse(((PrefixUnaryExpressionSyntax)node).Operand), // !false + { RawKind: (int)SyntaxKindEx.ConstantPattern } => IsTrue(((ConstantPatternSyntaxWrapper)node).Expression), // is true + { RawKind: (int)SyntaxKindEx.NotPattern } => IsFalse(((UnaryPatternSyntaxWrapper)node).Pattern), // is not false + { RawKind: (int)SyntaxKind.ParenthesizedExpression } => IsTrue(((ParenthesizedExpressionSyntax)node).Expression), // (true) + { RawKind: (int)SyntaxKindEx.ParenthesizedPattern } => IsTrue(((ParenthesizedPatternSyntaxWrapper)node).Pattern), // is (true) + _ => false, + }; + + public static bool IsFalse(this SyntaxNode node) => + node switch + { + { RawKind: (int)SyntaxKind.FalseLiteralExpression } => true, // false + { RawKind: (int)SyntaxKind.LogicalNotExpression } => IsTrue(((PrefixUnaryExpressionSyntax)node).Operand), // !true + { RawKind: (int)SyntaxKindEx.ConstantPattern } => IsFalse(((ConstantPatternSyntaxWrapper)node).Expression), // is false + { RawKind: (int)SyntaxKindEx.NotPattern } => IsTrue(((UnaryPatternSyntaxWrapper)node).Pattern), // is not true + { RawKind: (int)SyntaxKind.ParenthesizedExpression } => IsFalse(((ParenthesizedExpressionSyntax)node).Expression), // (false) + { RawKind: (int)SyntaxKindEx.ParenthesizedPattern } => IsFalse(((ParenthesizedPatternSyntaxWrapper)node).Pattern), // is (false) + _ => false, + }; + private readonly record struct PathPosition(int Index, int TupleLength); private sealed class ControlFlowGraphCache : ControlFlowGraphCacheBase diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessaryCodeFix.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessaryCodeFix.cs index 4c22a773777..ea69b725362 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessaryCodeFix.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessaryCodeFix.cs @@ -250,8 +250,7 @@ private static SyntaxNode ReplaceExpressionWithBinary(SyntaxNode nodeToReplace, private static Document RewriteConditional(Document document, SyntaxNode root, SyntaxNode syntaxNode, ConditionalExpressionSyntax conditional) { var whenTrue = conditional.WhenTrue.RemoveParentheses(); - if (whenTrue.Equals(syntaxNode) - && CSharpEquivalenceChecker.AreEquivalent(syntaxNode, CSharpSyntaxHelper.TrueLiteralExpression)) + if (whenTrue.Equals(syntaxNode) && syntaxNode.IsTrue()) { var newRoot = ReplaceExpressionWithBinary( conditional, @@ -263,8 +262,7 @@ private static Document RewriteConditional(Document document, SyntaxNode root, S return document.WithSyntaxRoot(newRoot); } - if (whenTrue.Equals(syntaxNode) - && CSharpEquivalenceChecker.AreEquivalent(syntaxNode, CSharpSyntaxHelper.FalseLiteralExpression)) + if (whenTrue.Equals(syntaxNode) && syntaxNode.IsFalse()) { var newRoot = ReplaceExpressionWithBinary( conditional, @@ -278,8 +276,7 @@ private static Document RewriteConditional(Document document, SyntaxNode root, S var whenFalse = conditional.WhenFalse.RemoveParentheses(); - if (whenFalse.Equals(syntaxNode) - && CSharpEquivalenceChecker.AreEquivalent(syntaxNode, CSharpSyntaxHelper.TrueLiteralExpression)) + if (whenFalse.Equals(syntaxNode) && syntaxNode.IsTrue()) { var newRoot = ReplaceExpressionWithBinary( conditional, @@ -291,8 +288,7 @@ private static Document RewriteConditional(Document document, SyntaxNode root, S return document.WithSyntaxRoot(newRoot); } - if (whenFalse.Equals(syntaxNode) - && CSharpEquivalenceChecker.AreEquivalent(syntaxNode, CSharpSyntaxHelper.FalseLiteralExpression)) + if (whenFalse.Equals(syntaxNode) && syntaxNode.IsFalse()) { var newRoot = ReplaceExpressionWithBinary( conditional, @@ -310,12 +306,11 @@ private static Document RewriteConditional(Document document, SyntaxNode root, S private static ExpressionSyntax GetNegatedExpression(ExpressionSyntax expression) { var exp = expression; - if (expression is BinaryExpressionSyntax - || expression is ConditionalExpressionSyntax) + if (expression is BinaryExpressionSyntax or ConditionalExpressionSyntax + || IsPatternExpressionSyntaxWrapper.IsInstance(expression)) { exp = SyntaxFactory.ParenthesizedExpression(expression); } - return SyntaxFactory.PrefixUnaryExpression(SyntaxKind.LogicalNotExpression, exp); } } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.CSharp9.Fixed.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.CSharp9.Fixed.cs index 9f175d36139..75db8ebedbb 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.CSharp9.Fixed.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.CSharp9.Fixed.cs @@ -29,5 +29,17 @@ public bool SomeMethod2() { return true; } + + // https://github.com/SonarSource/sonar-dotnet/issues/2618 + public void Repro_2618(Item item) + { + var booleanVariable = !(item is not Item myItem) && myItem.Required; // Fixed + booleanVariable = item is not Item myItem2 || myItem2.Required; // Fixed + } + } + + public class Item + { + public bool Required { get; set; } } } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.CSharp9.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.CSharp9.cs index 6342c36cac7..8ec8939042f 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.CSharp9.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.CSharp9.cs @@ -29,5 +29,17 @@ public bool SomeMethod2() { return true; } + + // https://github.com/SonarSource/sonar-dotnet/issues/2618 + public void Repro_2618(Item item) + { + var booleanVariable = item is not Item myItem ? false : myItem.Required; // Noncompliant + booleanVariable = item is not Item myItem2 ? true : myItem2.Required; // Noncompliant + } + } + + public class Item + { + public bool Required { get; set; } } } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.Fixed.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.Fixed.cs index 7ee761984ed..b5deb6d56bb 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.Fixed.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.Fixed.cs @@ -84,6 +84,13 @@ public BooleanLiteralUnnecessary(bool a, bool b, bool? c, Item item) }; } + // https://github.com/SonarSource/sonar-dotnet/issues/2618 + public void Repro_2618(Item item) + { + var booleanVariable = item is Item myItem && myItem.Required; // Fixed + booleanVariable = !(item is Item myItem2) || myItem2.Required; // Fixed + } + public static void SomeFunc(bool x) { } private bool Foo() diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.cs index 2b775cc5e56..67416c816c4 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.cs @@ -76,9 +76,6 @@ public BooleanLiteralUnnecessary(bool a, bool b, bool? c, Item item) booleanVariable = condition ? throw new Exception() : true; // Compliant, we don't raise for throw expressions booleanVariable = condition ? throw new Exception() : false; // Compliant, we don't raise for throw expressions - // Reproducer for https://github.com/SonarSource/sonar-dotnet/issues/2618 - booleanVariable = item is Item myItem ? myItem.Required : false; // Noncompliant - booleanVariable = condition ? exp : exp2; b = x || booleanVariable ? false : true; // Noncompliant @@ -99,6 +96,13 @@ public BooleanLiteralUnnecessary(bool a, bool b, bool? c, Item item) }; } + // https://github.com/SonarSource/sonar-dotnet/issues/2618 + public void Repro_2618(Item item) + { + var booleanVariable = item is Item myItem ? myItem.Required : false; // Noncompliant + booleanVariable = item is Item myItem2 ? myItem2.Required : true; // Noncompliant + } + public static void SomeFunc(bool x) { } private bool Foo()