Updated new rule MiKo_5018 to simplify value comparisons for parameters and properties

RalfKoban · RalfKoban · commit 51560f060b0f · 2024-10-25T10:59:19.000+02:00
(resolves #1090)
diff --git a/MiKo.Analyzer.Shared/Rules/Performance/MiKo_5018_CodeFixProvider.cs b/MiKo.Analyzer.Shared/Rules/Performance/MiKo_5018_CodeFixProvider.cs
@@ -14,7 +14,7 @@ public sealed class MiKo_5018_CodeFixProvider : PerformanceCodeFixProvider
     {
         private static readonly SyntaxKind[] LogicalConditions = { SyntaxKind.LogicalAndExpression, SyntaxKind.LogicalOrExpression };
 
-        private static readonly SyntaxKind[] SpecialParentHandling = { SyntaxKind.ArrowExpressionClause, SyntaxKind.ReturnStatement, SyntaxKind.IfStatement };
+        private static readonly SyntaxKind[] SpecialParents = { SyntaxKind.ArrowExpressionClause, SyntaxKind.ReturnStatement, SyntaxKind.IfStatement };
 
         public override string FixableDiagnosticId => "MiKo_5018";
 
@@ -25,11 +25,11 @@ protected override SyntaxNode GetUpdatedSyntax(Document document, SyntaxNode syn
             if (syntax is BinaryExpressionSyntax binary)
             {
                 var left = FindProblematicNode(binary.Left); // TODO RKN: left will be "values.Length == 8"
-                var right = binary.Right;
+                var right = FindProblematicNode(binary.Right);
 
                 var updatedSyntax = binary.ReplaceNodes(new[] { left, right }, (original, rewritten) => ReferenceEquals(original, left) ? right.WithTriviaFrom(original) : left.WithTriviaFrom(original));
 
-                return binary.Parent.IsAnyKind(SpecialParentHandling)
+                return binary.Parent.IsAnyKind(SpecialParents)
                        ? updatedSyntax.WithoutTrailingTrivia() // only remove trailing trivia if condition is direct child of 'if/return/arrow clause' so that semicolon fits
                        : updatedSyntax;
             }
@@ -45,15 +45,18 @@ private static ExpressionSyntax FindProblematicNode(ExpressionSyntax expression)
             {
                 if (node is BinaryExpressionSyntax binary && binary.IsAnyKind(LogicalConditions))
                 {
-                    var right = binary.Right;
+                    var next = binary.Right;
 
-                    if (right is BinaryExpressionSyntax binaryRight && (binaryRight.Left is ElementAccessExpressionSyntax || binaryRight.Right is ElementAccessExpressionSyntax))
+                    if (next is BinaryExpressionSyntax nested)
                     {
-                        // we have some element access, so we need to replace the complete node
-                        return expression;
+                        if (nested.Left is ElementAccessExpressionSyntax || nested.Right is ElementAccessExpressionSyntax)
+                        {
+                            // we have some null checks or some element access, so we need to replace the complete node
+                            return expression;
+                        }
                     }
 
-                    node = right;
+                    node = next;
                 }
                 else
                 {
diff --git a/MiKo.Analyzer.Shared/Rules/Performance/MiKo_5018_LogicalValueComparisonsComeFirstAnalyzer.cs b/MiKo.Analyzer.Shared/Rules/Performance/MiKo_5018_LogicalValueComparisonsComeFirstAnalyzer.cs
@@ -35,10 +35,42 @@ private static bool IsNullCheck(ExpressionSyntax expression)
 
         private static bool IsNullCheck(BinaryExpressionSyntax expression) => expression.Left.IsKind(SyntaxKind.NullLiteralExpression) || expression.Right.IsKind(SyntaxKind.NullLiteralExpression);
 
+        private static bool IsNumberCheck(ExpressionSyntax expression)
+        {
+            switch (expression.WithoutParenthesis())
+            {
+                case BinaryExpressionSyntax binary when IsNumberCheck(binary):
+                    return true;
+
+                default:
+                    return false;
+            }
+        }
+
+        private static bool IsNumberCheck(BinaryExpressionSyntax expression)
+        {
+            var left = expression.Left;
+            var right = expression.Right;
+
+            if (left.IsKind(SyntaxKind.NumericLiteralExpression) || right.IsKind(SyntaxKind.NumericLiteralExpression))
+            {
+                return true;
+            }
+
+            if (IsNumberCheck(left) && IsNumberCheck(right))
+            {
+                return true;
+            }
+
+            return false;
+        }
+
         private static bool HasIssue(BinaryExpressionSyntax binary, SyntaxNodeAnalysisContext context)
         {
             if (binary.Right.WithoutParenthesis() is BinaryExpressionSyntax right && right.IsKind(SyntaxKind.EqualsExpression))
             {
+                var semanticModel = context.SemanticModel;
+
                 if (binary.Left.WithoutParenthesis() is ExpressionSyntax left)
                 {
                     if (IsNullCheck(left))
@@ -47,18 +79,30 @@ private static bool HasIssue(BinaryExpressionSyntax binary, SyntaxNodeAnalysisCo
                         return false;
                     }
 
+                    if (IsNumberCheck(left))
+                    {
+                        // do not report nested number checks
+                        return false;
+                    }
+
                     switch (left)
                     {
                         case IdentifierNameSyntax _: return false; // do not report checks on boolean members
                         case IsPatternExpressionSyntax e when e.Pattern is DeclarationPatternSyntax: return false; // do not report pattern checks
+                        case MemberAccessExpressionSyntax m when m.GetTypeSymbol(semanticModel)?.IsValueType is true: return false; // do not report checks on value type members
                         case BinaryExpressionSyntax nested:
                         {
                             if (nested.IsAnyKind(LogicalConditions))
                             {
-                                if (IsNullCheck(nested.Right))
+                                if (IsNullCheck(nested.Right) && IsNullCheck(nested.Left))
                                 {
                                     return false; // do not report nested null checks
                                 }
+
+                                if (IsNumberCheck(nested.Right) && IsNumberCheck(nested.Left))
+                                {
+                                    return false; // do not report nested number checks
+                                }
                             }
                             else
                             {
@@ -78,7 +122,7 @@ private static bool HasIssue(BinaryExpressionSyntax binary, SyntaxNodeAnalysisCo
                                     return false;
                                 }
 
-                                if (nestedLeft.IsKind(SyntaxKind.NumericLiteralExpression) || nestedRight.IsKind(SyntaxKind.NumericLiteralExpression))
+                                if (IsNumberCheck(nested))
                                 {
                                     // do not report checks on value types
                                     return false;
@@ -89,6 +133,15 @@ private static bool HasIssue(BinaryExpressionSyntax binary, SyntaxNodeAnalysisCo
                                     // do not report on members
                                     return false;
                                 }
+
+                                if (nestedLeft is IdentifierNameSyntax && nestedRight is IdentifierNameSyntax)
+                                {
+                                    if (nestedLeft.GetTypeSymbol(semanticModel)?.IsValueType is true && nestedRight.GetTypeSymbol(semanticModel)?.IsValueType is true)
+                                    {
+                                        // do not report checks on value types
+                                        return false;
+                                    }
+                                }
                             }
 
                             break;
@@ -102,8 +155,6 @@ private static bool HasIssue(BinaryExpressionSyntax binary, SyntaxNodeAnalysisCo
                     return false;
                 }
 
-                var semanticModel = context.SemanticModel;
-
                 if (right.Left.GetTypeSymbol(semanticModel)?.IsValueType is true && right.Right.GetTypeSymbol(semanticModel)?.IsValueType is true)
                 {
                     return true;
diff --git a/MiKo.Analyzer.Tests/Rules/Performance/MiKo_5018_LogicalValueComparisonsComeFirstAnalyzerTests.cs b/MiKo.Analyzer.Tests/Rules/Performance/MiKo_5018_LogicalValueComparisonsComeFirstAnalyzerTests.cs
@@ -70,6 +70,19 @@ public TestMe Nested { get }
 
     public bool DoSomething(TestMe arg) => arg != null && arg.Nested != null && arg.Nested.Value == 42;
 }
+");
+
+        [Test]
+        public void No_issue_is_reported_for_class_with_conditional_null_checks_and_value_type_comparison() => No_issue_is_reported_for(@"
+using System;
+
+public class TestMe
+{
+    public TestMe Nested { get }
+    public int Value { get; }
+
+    public bool DoSomething(TestMe arg) => arg != null && arg.Nested?.Value == 42;
+}
 ");
 
         [Test]
@@ -234,6 +247,64 @@ public class TestMe
 
     public bool SomeComparison(TestMe other) => other is null || (Value != other.Value && Data != other.Data);
 }
+");
+
+        [Test]
+        public void No_issue_is_reported_for_complex_class_with_value_type_enum_OR_comparison() => No_issue_is_reported_for(@"
+using System;
+using System.Linq;
+
+public class TestMe
+{
+    public StringComparison Comparison { get; set; }
+
+    public Guid Id { get; set; }
+
+    public bool SomeComparison(TestMe other) => other.Id == Guid.Empty && (other.Comparison == StringComparison.Ordinal || other.Comparison == StringComparison.OrdinalIgnoreCase);
+}
+");
+
+        [Test]
+        public void No_issue_is_reported_for_parameters_only_value_type_comparisons() => No_issue_is_reported_for(@"
+using System;
+
+public class TestMe
+{
+    public static bool SomeComparison(Guid guid1, Guid guid2, Guid guid3, Guid guid4) => guid1 == guid3 && guid2 == guid4;
+}
+");
+
+        [Test]
+        public void No_issue_is_reported_for_CancellationToken_IsCancellationRequested_call() => No_issue_is_reported_for(@"
+using System;
+using System.Threading;
+
+public class TestMe
+{
+    public static bool SomeComparison(CancellationToken token, int value) => token.IsCancellationRequested || value == -1;
+}
+");
+
+        [Test]
+        public void No_issue_is_reported_for_version_comparison() => No_issue_is_reported_for(@"
+using System;
+using System.Threading;
+
+public class TestMe
+{
+    public static bool SomeComparison(Version version) => version.Major == 0 && version.Minor == 0 && version.Build == 0 && version.Revision == 0;
+}
+");
+
+        [Test]
+        public void No_issue_is_reported_for_version_comparison_when_numbers_are_on_left_side() => No_issue_is_reported_for(@"
+using System;
+using System.Threading;
+
+public class TestMe
+{
+    public static bool SomeComparison(Version version) => 0 == version.Major && 0 == version.Minor && 0 == version.Build && 0 == version.Revision;
+}
 ");
 
         [Test]
@@ -339,6 +410,32 @@ public class TestMe
 {
     public bool DoSomething(int i, object o) => o.ToString() == ""whatever"" || i == 42;
 }
+");
+
+        [Test]
+        public void An_issue_is_reported_for_class_with_reference_type_comparison_and_multiple_null_checks_and_value_type_comparison() => An_issue_is_reported_for(@"
+using System;
+
+public class TestMe
+{
+    public TestMe Nested { get }
+    public int Value { get; }
+
+    public bool DoSomething(TestMe arg, object o) => o.ToString() == ""whatever"" && arg != null && arg.Nested != null && arg.Nested.Value == 42;
+}
+");
+
+        [Test]
+        public void An_issue_is_reported_for_class_with_conditional_value_type_comparison_and_reference_type_comparison_if_string_invocation_comes_first() => An_issue_is_reported_for(@"
+using System;
+
+public class TestMe
+{
+    public TestMe Nested { get }
+    public int Value { get; }
+
+    public bool DoSomething(TestMe arg, object o) => o.ToString() == ""whatever"" && arg?.Nested?.Value == 42;
+}
 ");
 
         [Test]
@@ -651,7 +748,101 @@ public bool DoSomething(int i, int[] values)
             VerifyCSharpFix(OriginalText, FixedText);
         }
 
-        //// TODO RKN: Add more complex AND/OR Comparisons with at least 3 comparisons and parenthesized ones
+        [Test]
+        public void Code_gets_fixed_for_class_with_reference_type_comparison_and_multiple_null_checks_and_value_type_comparison()
+        {
+            const string OriginalCode = @"
+using System;
+
+public class TestMe
+{
+    public TestMe Nested { get }
+    public int Value { get; }
+
+    public bool DoSomething(TestMe arg, object o) => o.ToString() == ""whatever"" && arg != null && arg.Nested != null && arg.Nested.Value == 42;
+}
+";
+
+            const string FixedCode = @"
+using System;
+
+public class TestMe
+{
+    public TestMe Nested { get }
+    public int Value { get; }
+
+    public bool DoSomething(TestMe arg, object o) => arg != null && arg.Nested != null && arg.Nested.Value == 42 && o.ToString() == ""whatever"";
+}
+";
+
+            VerifyCSharpFix(OriginalCode, FixedCode);
+        }
+
+        [Test]
+        public void Code_gets_fixed_for_class_with_conditional_value_type_comparison_and_reference_type_comparison_if_string_invocation_comes_first()
+        {
+            const string OriginalCode = @"
+using System;
+
+public class TestMe
+{
+    public TestMe Nested { get }
+    public int Value { get; }
+
+    public bool DoSomething(TestMe arg, object o) => o.ToString() == ""whatever"" && arg?.Nested?.Value == 42;
+}
+";
+
+            const string FixedCode = @"
+using System;
+
+public class TestMe
+{
+    public TestMe Nested { get }
+    public int Value { get; }
+
+    public bool DoSomething(TestMe arg, object o) => arg?.Nested?.Value == 42 && o.ToString() == ""whatever"";
+}
+";
+
+            VerifyCSharpFix(OriginalCode, FixedCode);
+        }
+
+        [Test]
+        public void Code_gets_fixed_for_class_with_conditional_value_type_comparison_and_boolean_method_calls_if_method_calls_come_first()
+        {
+            const string OriginalCode = @"
+using System;
+
+public class TestMe
+{
+    public StringComparison Comparison { get }
+
+    public bool DoSomething(TestMe t) => IsSomething(t) && FindSomething(t) == t && t.Comparison == StringComparison.Ordinal;
+
+    public bool IsSomething(object o) => true;
+
+    public object FindSomething(object o) => o;
+}
+";
+
+            const string FixedCode = @"
+using System;
+
+public class TestMe
+{
+    public StringComparison Comparison { get }
+
+    public bool DoSomething(TestMe t) => t.Comparison == StringComparison.Ordinal && IsSomething(t) && FindSomething(t) == t;
+
+    public bool IsSomething(object o) => true;
+
+    public object FindSomething(object o) => o;
+}
+";
+
+            VerifyCSharpFix(OriginalCode, FixedCode);
+        }
 
         protected override string GetDiagnosticId() => MiKo_5018_LogicalValueComparisonsComeFirstAnalyzer.Id;
 

Original file line number	Diff line number	Diff line change
`@@ -14,7 +14,7 @@ public sealed class MiKo_5018_CodeFixProvider : PerformanceCodeFixProvider`
`14`	`14`	`{`
`15`	`15`	`private static readonly SyntaxKind[] LogicalConditions = { SyntaxKind.LogicalAndExpression, SyntaxKind.LogicalOrExpression };`
`16`	`16`
`17`		`- private static readonly SyntaxKind[] SpecialParentHandling = { SyntaxKind.ArrowExpressionClause, SyntaxKind.ReturnStatement, SyntaxKind.IfStatement };`
	`17`	`+ private static readonly SyntaxKind[] SpecialParents = { SyntaxKind.ArrowExpressionClause, SyntaxKind.ReturnStatement, SyntaxKind.IfStatement };`
`18`	`18`
`19`	`19`	`public override string FixableDiagnosticId => "MiKo_5018";`
`20`	`20`
`@@ -25,11 +25,11 @@ protected override SyntaxNode GetUpdatedSyntax(Document document, SyntaxNode syn`
`25`	`25`	`if (syntax is BinaryExpressionSyntax binary)`
`26`	`26`	`{`
`27`	`27`	`var left = FindProblematicNode(binary.Left); // TODO RKN: left will be "values.Length == 8"`
`28`		`- var right = binary.Right;`
	`28`	`+ var right = FindProblematicNode(binary.Right);`
`29`	`29`
`30`	`30`	`var updatedSyntax = binary.ReplaceNodes(new[] { left, right }, (original, rewritten) => ReferenceEquals(original, left) ? right.WithTriviaFrom(original) : left.WithTriviaFrom(original));`
`31`	`31`
`32`		`- return binary.Parent.IsAnyKind(SpecialParentHandling)`
	`32`	`+ return binary.Parent.IsAnyKind(SpecialParents)`
`33`	`33`	`? updatedSyntax.WithoutTrailingTrivia() // only remove trailing trivia if condition is direct child of 'if/return/arrow clause' so that semicolon fits`
`34`	`34`	`: updatedSyntax;`
`35`	`35`	`}`
`@@ -45,15 +45,18 @@ private static ExpressionSyntax FindProblematicNode(ExpressionSyntax expression)`
`45`	`45`	`{`
`46`	`46`	`if (node is BinaryExpressionSyntax binary && binary.IsAnyKind(LogicalConditions))`
`47`	`47`	`{`
`48`		`- var right = binary.Right;`
	`48`	`+ var next = binary.Right;`
`49`	`49`
`50`		`- if (right is BinaryExpressionSyntax binaryRight && (binaryRight.Left is ElementAccessExpressionSyntax \|\| binaryRight.Right is ElementAccessExpressionSyntax))`
	`50`	`+ if (next is BinaryExpressionSyntax nested)`
`51`	`51`	`{`
`52`		`- // we have some element access, so we need to replace the complete node`
`53`		`- return expression;`
	`52`	`+ if (nested.Left is ElementAccessExpressionSyntax \|\| nested.Right is ElementAccessExpressionSyntax)`
	`53`	`+ {`
	`54`	`+ // we have some null checks or some element access, so we need to replace the complete node`
	`55`	`+ return expression;`
	`56`	`+ }`
`54`	`57`	`}`
`55`	`58`
`56`		`- node = right;`
	`59`	`+ node = next;`
`57`	`60`	`}`
`58`	`61`	`else`
`59`	`62`	`{`