From 5a98976ba59861fce5feb2560b13bf2120ec7472 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Wed, 2 Aug 2023 17:53:32 +0200 Subject: [PATCH 1/5] Add reproducer --- .../TestCases/BooleanLiteralUnnecessary.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.cs index 0b6eaf1857a..df9224c29fe 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.cs @@ -90,6 +90,9 @@ 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 From cbdb8864f8cecfcb5eb9af1cb8f68df10b728647 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Thu, 3 Aug 2023 10:02:34 +0200 Subject: [PATCH 2/5] Fix codeFix --- .../Rules/BooleanLiteralUnnecessaryCodeFix.cs | 17 ++++++----------- .../BooleanLiteralUnnecessary.CSharp9.Fixed.cs | 12 ++++++++++++ .../BooleanLiteralUnnecessary.CSharp9.cs | 12 ++++++++++++ .../BooleanLiteralUnnecessary.Fixed.cs | 7 +++++++ .../TestCases/BooleanLiteralUnnecessary.cs | 10 +++++++--- 5 files changed, 44 insertions(+), 14 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessaryCodeFix.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessaryCodeFix.cs index 8b2544ac07b..d6eff3810ce 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessaryCodeFix.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessaryCodeFix.cs @@ -281,8 +281,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, @@ -294,8 +293,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, @@ -309,8 +307,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, @@ -322,8 +319,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, @@ -341,12 +337,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 59a86566fc0..00e47c764c1 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.CSharp9.Fixed.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.CSharp9.Fixed.cs @@ -61,5 +61,17 @@ void IsNotPattern(bool a, bool? b) var x = a is not true ? throw new Exception() : false; } + + // 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 677672ba85c..df7e001dc18 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.CSharp9.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.CSharp9.cs @@ -63,5 +63,17 @@ void IsNotPattern(bool a, bool? b) var x = a is not true ? throw new Exception() : false; } + + // 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 156284508a5..af01f4fc9e3 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.Fixed.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.Fixed.cs @@ -99,6 +99,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 df9224c29fe..edd588c66b0 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.cs @@ -90,9 +90,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 @@ -119,6 +116,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() From 5f8a55347d0c6016386bcd34fa392e8fa56eb38d Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Thu, 3 Aug 2023 10:24:34 +0200 Subject: [PATCH 3/5] Fix #7462 --- .../Rules/BooleanLiteralUnnecessaryCodeFix.cs | 26 +++++++++---------- .../BooleanLiteralUnnecessary.Fixed.cs | 6 +++++ .../TestCases/BooleanLiteralUnnecessary.cs | 6 +++++ 3 files changed, 24 insertions(+), 14 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessaryCodeFix.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessaryCodeFix.cs index d6eff3810ce..10602a11761 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessaryCodeFix.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessaryCodeFix.cs @@ -288,7 +288,7 @@ private static Document RewriteConditional(Document document, SyntaxNode root, S root, SyntaxKind.LogicalOrExpression, conditional.Condition, - conditional.WhenFalse); + ParenthesizeIfComplex(conditional.WhenFalse)); return document.WithSyntaxRoot(newRoot); } @@ -300,7 +300,7 @@ private static Document RewriteConditional(Document document, SyntaxNode root, S root, SyntaxKind.LogicalAndExpression, GetNegatedExpression(conditional.Condition), - conditional.WhenFalse); + ParenthesizeIfComplex(conditional.WhenFalse)); return document.WithSyntaxRoot(newRoot); } @@ -314,7 +314,7 @@ private static Document RewriteConditional(Document document, SyntaxNode root, S root, SyntaxKind.LogicalOrExpression, GetNegatedExpression(conditional.Condition), - conditional.WhenTrue); + ParenthesizeIfComplex(conditional.WhenTrue)); return document.WithSyntaxRoot(newRoot); } @@ -326,7 +326,7 @@ private static Document RewriteConditional(Document document, SyntaxNode root, S root, SyntaxKind.LogicalAndExpression, conditional.Condition, - conditional.WhenTrue); + ParenthesizeIfComplex(conditional.WhenTrue)); return document.WithSyntaxRoot(newRoot); } @@ -334,15 +334,13 @@ private static Document RewriteConditional(Document document, SyntaxNode root, S return document; } - private static ExpressionSyntax GetNegatedExpression(ExpressionSyntax expression) - { - var exp = expression; - if (expression is BinaryExpressionSyntax or ConditionalExpressionSyntax - || IsPatternExpressionSyntaxWrapper.IsInstance(expression)) - { - exp = SyntaxFactory.ParenthesizedExpression(expression); - } - return SyntaxFactory.PrefixUnaryExpression(SyntaxKind.LogicalNotExpression, exp); - } + private static ExpressionSyntax GetNegatedExpression(ExpressionSyntax expression) => + SyntaxFactory.PrefixUnaryExpression(SyntaxKind.LogicalNotExpression, ParenthesizeIfComplex(expression)); + + private static ExpressionSyntax ParenthesizeIfComplex(ExpressionSyntax expression) => + expression is BinaryExpressionSyntax or ConditionalExpressionSyntax + || IsPatternExpressionSyntaxWrapper.IsInstance(expression) + ? SyntaxFactory.ParenthesizedExpression(expression) + : expression; } } diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.Fixed.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.Fixed.cs index af01f4fc9e3..9acbf811c40 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.Fixed.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.Fixed.cs @@ -181,6 +181,12 @@ public void ThrowExpressionIsIgnored(bool condition, int number) x = condition is true ? throw new Exception() : false; } } + + // https://github.com/SonarSource/sonar-dotnet/issues/7462 + public void Repro_7462(object obj) + { + var a = !(obj == null) && (obj.ToString() == "x" || obj.ToString() == "y"); // Fixed + } } public class Item diff --git a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.cs b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.cs index edd588c66b0..b1f7813abfc 100644 --- a/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.cs +++ b/analyzers/tests/SonarAnalyzer.UnitTest/TestCases/BooleanLiteralUnnecessary.cs @@ -198,6 +198,12 @@ public void ThrowExpressionIsIgnored(bool condition, int number) x = condition is true ? throw new Exception() : false; } } + + // https://github.com/SonarSource/sonar-dotnet/issues/7462 + public void Repro_7462(object obj) + { + var a = obj == null ? false : obj.ToString() == "x" || obj.ToString() == "y"; // Noncompliant + } } public class Item From d9b504d93ff88c707154aedf01c42f5447918f4b Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Thu, 3 Aug 2023 15:50:58 +0200 Subject: [PATCH 4/5] Add always parenthesis and then simplify --- .../Rules/BooleanLiteralUnnecessaryCodeFix.cs | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessaryCodeFix.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessaryCodeFix.cs index 10602a11761..a343ebf72c0 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessaryCodeFix.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessaryCodeFix.cs @@ -288,7 +288,7 @@ private static Document RewriteConditional(Document document, SyntaxNode root, S root, SyntaxKind.LogicalOrExpression, conditional.Condition, - ParenthesizeIfComplex(conditional.WhenFalse)); + AddParenthesisIfNeeded(conditional.WhenFalse)); return document.WithSyntaxRoot(newRoot); } @@ -300,7 +300,7 @@ private static Document RewriteConditional(Document document, SyntaxNode root, S root, SyntaxKind.LogicalAndExpression, GetNegatedExpression(conditional.Condition), - ParenthesizeIfComplex(conditional.WhenFalse)); + AddParenthesisIfNeeded(conditional.WhenFalse)); return document.WithSyntaxRoot(newRoot); } @@ -314,7 +314,7 @@ private static Document RewriteConditional(Document document, SyntaxNode root, S root, SyntaxKind.LogicalOrExpression, GetNegatedExpression(conditional.Condition), - ParenthesizeIfComplex(conditional.WhenTrue)); + AddParenthesisIfNeeded(conditional.WhenTrue)); return document.WithSyntaxRoot(newRoot); } @@ -326,7 +326,7 @@ private static Document RewriteConditional(Document document, SyntaxNode root, S root, SyntaxKind.LogicalAndExpression, conditional.Condition, - ParenthesizeIfComplex(conditional.WhenTrue)); + AddParenthesisIfNeeded(conditional.WhenTrue)); return document.WithSyntaxRoot(newRoot); } @@ -335,12 +335,9 @@ private static Document RewriteConditional(Document document, SyntaxNode root, S } private static ExpressionSyntax GetNegatedExpression(ExpressionSyntax expression) => - SyntaxFactory.PrefixUnaryExpression(SyntaxKind.LogicalNotExpression, ParenthesizeIfComplex(expression)); + SyntaxFactory.PrefixUnaryExpression(SyntaxKind.LogicalNotExpression, AddParenthesisIfNeeded(expression)); - private static ExpressionSyntax ParenthesizeIfComplex(ExpressionSyntax expression) => - expression is BinaryExpressionSyntax or ConditionalExpressionSyntax - || IsPatternExpressionSyntaxWrapper.IsInstance(expression) - ? SyntaxFactory.ParenthesizedExpression(expression) - : expression; + private static ExpressionSyntax AddParenthesisIfNeeded(ExpressionSyntax expression) => + SyntaxFactory.ParenthesizedExpression(expression).WithAdditionalAnnotations(Simplifier.Annotation); } } From 1227edd8db19818570eb9914adfd56e0534ffe57 Mon Sep 17 00:00:00 2001 From: Cristian Ambrosini Date: Thu, 3 Aug 2023 16:31:39 +0200 Subject: [PATCH 5/5] Method renaming --- .../Rules/BooleanLiteralUnnecessaryCodeFix.cs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessaryCodeFix.cs b/analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessaryCodeFix.cs index a343ebf72c0..fc48cf67919 100644 --- a/analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessaryCodeFix.cs +++ b/analyzers/src/SonarAnalyzer.CSharp/Rules/BooleanLiteralUnnecessaryCodeFix.cs @@ -288,7 +288,7 @@ private static Document RewriteConditional(Document document, SyntaxNode root, S root, SyntaxKind.LogicalOrExpression, conditional.Condition, - AddParenthesisIfNeeded(conditional.WhenFalse)); + AddParenthesis(conditional.WhenFalse)); return document.WithSyntaxRoot(newRoot); } @@ -300,7 +300,7 @@ private static Document RewriteConditional(Document document, SyntaxNode root, S root, SyntaxKind.LogicalAndExpression, GetNegatedExpression(conditional.Condition), - AddParenthesisIfNeeded(conditional.WhenFalse)); + AddParenthesis(conditional.WhenFalse)); return document.WithSyntaxRoot(newRoot); } @@ -314,7 +314,7 @@ private static Document RewriteConditional(Document document, SyntaxNode root, S root, SyntaxKind.LogicalOrExpression, GetNegatedExpression(conditional.Condition), - AddParenthesisIfNeeded(conditional.WhenTrue)); + AddParenthesis(conditional.WhenTrue)); return document.WithSyntaxRoot(newRoot); } @@ -326,7 +326,7 @@ private static Document RewriteConditional(Document document, SyntaxNode root, S root, SyntaxKind.LogicalAndExpression, conditional.Condition, - AddParenthesisIfNeeded(conditional.WhenTrue)); + AddParenthesis(conditional.WhenTrue)); return document.WithSyntaxRoot(newRoot); } @@ -335,9 +335,9 @@ private static Document RewriteConditional(Document document, SyntaxNode root, S } private static ExpressionSyntax GetNegatedExpression(ExpressionSyntax expression) => - SyntaxFactory.PrefixUnaryExpression(SyntaxKind.LogicalNotExpression, AddParenthesisIfNeeded(expression)); + SyntaxFactory.PrefixUnaryExpression(SyntaxKind.LogicalNotExpression, AddParenthesis(expression)); - private static ExpressionSyntax AddParenthesisIfNeeded(ExpressionSyntax expression) => + private static ExpressionSyntax AddParenthesis(ExpressionSyntax expression) => SyntaxFactory.ParenthesizedExpression(expression).WithAdditionalAnnotations(Simplifier.Annotation); } }