Skip to content

Commit

Permalink
Updated new rule MiKo_5018 to simplify value comparisons
Browse files Browse the repository at this point in the history 
(resolves #1090)
  • Loading branch information
@RalfKoban
RalfKoban committed Oct 21, 2024
1 parent 0d5493b commit 0a19d6a
Show file tree
Hide file tree
Showing 3 changed files with 217 additions and 15 deletions.
8 changes: 7 additions & 1 deletion MiKo.Analyzer.Shared/Rules/Performance/MiKo_5018_CodeFixProvider.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ 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 };

public override string FixableDiagnosticId => "MiKo_5018";

protected override SyntaxNode GetSyntax(IEnumerable<SyntaxNode> syntaxNodes) => syntaxNodes.OfType<BinaryExpressionSyntax>().FirstOrDefault();
Expand All @@ -25,7 +27,11 @@ protected override SyntaxNode GetUpdatedSyntax(Document document, SyntaxNode syn
var left = FindProblematicNode(binary.Left);
var right = binary.Right;

return binary.ReplaceNodes(new[] { left, right }, (original, rewritten) => ReferenceEquals(original, left) ? right.WithTrailingSpace() : left.WithoutTrivia());
var updatedSyntax = binary.ReplaceNodes(new[] { left, right }, (original, rewritten) => ReferenceEquals(original, left) ? right.WithTriviaFrom(original) : left.WithTriviaFrom(original));

return binary.Parent.IsAnyKind(SpecialParentHandling)
? updatedSyntax.WithoutTrailingTrivia() // only remove trailing trivia if condition is direct child of 'if/return/arrow clause' so that semicolon fits
: updatedSyntax;
}

return syntax;
Expand Down
54 changes: 43 additions & 11 deletions MiKo.Analyzer.Shared/Rules/Performance/MiKo_5018_LogicalValueComparisonsComeFirstAnalyzer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -36,28 +36,60 @@ private static bool HasIssue(BinaryExpressionSyntax binary, SyntaxNodeAnalysisCo
{
if (binary.Right.WithoutParenthesis() is BinaryExpressionSyntax right && right.IsKind(SyntaxKind.EqualsExpression))
{
var semanticModel = context.SemanticModel;

if (right.Left.GetTypeSymbol(semanticModel)?.IsValueType is true && right.Right.GetTypeSymbol(semanticModel)?.IsValueType is true)
if (binary.Left.WithoutParenthesis() is ExpressionSyntax left)
{
if (binary.Left.WithoutParenthesis() is ExpressionSyntax left)
if (IsNullCheck(left))
{
if (IsNullCheck(left))
{
// do not report null checks
return false;
}
// do not report null checks
return false;
}

if (left is BinaryExpressionSyntax nested && nested.IsAnyKind(LogicalConditions) is 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 BinaryExpressionSyntax nested when nested.IsAnyKind(LogicalConditions) is false:
{
if (nested.Left.GetTypeSymbol(semanticModel)?.IsValueType is true && nested.Right.GetTypeSymbol(semanticModel)?.IsValueType is true)
var nestedLeft = nested.Left;
var nestedRight = nested.Right;

if (nestedLeft is InvocationExpressionSyntax
|| nestedRight is InvocationExpressionSyntax)
{
// report on method calls
return true;
}

if (nestedLeft.IsKind(SyntaxKind.StringLiteralExpression)
|| nestedRight.IsKind(SyntaxKind.StringLiteralExpression))
{
// do not report checks on strings and value types
return false;
}

if (nestedLeft.IsKind(SyntaxKind.NumericLiteralExpression)
|| nestedRight.IsKind(SyntaxKind.NumericLiteralExpression))
{
// do not report checks on value types
return false;
}

if (nestedLeft is MemberAccessExpressionSyntax
|| nestedRight is MemberAccessExpressionSyntax)
{
// do not report on members
return false;
}

break;
}
}
}

var semanticModel = context.SemanticModel;

if (right.Left.GetTypeSymbol(semanticModel)?.IsValueType is true && right.Right.GetTypeSymbol(semanticModel)?.IsValueType is true)
{
return true;
}
}
Expand Down
170 changes: 167 additions & 3 deletions ...alyzer.Tests/Rules/Performance/MiKo_5018_LogicalValueComparisonsComeFirstAnalyzerTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,26 @@ public void No_issue_is_reported_for_empty_class() => No_issue_is_reported_for(@
public class TestMe
{
}
");

[Test]
public void No_issue_is_reported_for_class_with_value_type_comparison_if_boolean_flag_comes_first() => No_issue_is_reported_for(@"
using System;
public class TestMe
{
public bool DoSomething(bool flag, int i) => flag && i == 42;
}
");

[Test]
public void No_issue_is_reported_for_class_with_value_type_comparison_if_string_comparison_comes_first() => No_issue_is_reported_for(@"
using System;
public class TestMe
{
public bool DoSomething(string s, int i) => s == ""whatever"" && i == 42;
}
");

[Test]
Expand All @@ -23,7 +43,19 @@ public void No_issue_is_reported_for_class_with_value_type_comparison() => No_is
public class TestMe
{
public bool DoSomething(int i) => i == 42 || i == 0815);
public bool DoSomething(int i) => i == 42 || i == 0815;
}
");

[Test]
public void No_issue_is_reported_for_class_with_pattern_matching_value_type_comparison() => No_issue_is_reported_for(@"
using System;
public class TestMe
{
public int Value { get; }
public bool DoSomething(object o) => o is TestMe t && t.Value == 42;
}
");

Expand All @@ -33,7 +65,33 @@ public void No_issue_is_reported_for_class_with_value_type_comparison_if_constan
public class TestMe
{
public bool DoSomething(int i) => 42 == i || 0815 == i);
public bool DoSomething(int i) => 42 == i || 0815 == i;
}
");

[Test]
public void No_issue_is_reported_for_public_static_readonly_field_comparison_when_field_comes_first() => No_issue_is_reported_for(@"
using System;
public class TestMe
{
private static void SetOwnerWindow(IntPtr intPtrOwned, IntPtr intPtrOwner)
{
if (IntPtr.Zero == intPtrOwned || IntPtr.Zero == intPtrOwner) return;
}
}
");

[Test]
public void No_issue_is_reported_for_public_static_readonly_field_field_comparison_when_field_comes_last() => No_issue_is_reported_for(@"
using System;
public class TestMe
{
private static void SetOwnerWindow(IntPtr intPtrOwned, IntPtr intPtrOwner)
{
if (intPtrOwned == IntPtr.Zero || intPtrOwner == IntPtr.Zero) return;
}
}
");

Expand All @@ -43,7 +101,7 @@ public void No_issue_is_reported_for_class_with_value_type_and_reference_type_OR
public class TestMe
{
public bool DoSomething(int i, object o) => i == 42 || o.ToString() == ""whatever"");
public bool DoSomething(int i, object o) => i == 42 || o.ToString() == ""whatever"";
}
");

Expand Down Expand Up @@ -205,6 +263,16 @@ public bool Equals(TestMe other)
return ((Data == other.Data) || (Data != null && Data.SequenceEqual(otherData))) && Id == other.Id;
}
}
");

[Test]
public void An_issue_is_reported_for_class_with_value_type_and_reference_type_OR_comparison_if_string_invocation_comes_first() => An_issue_is_reported_for(@"
using System;
public class TestMe
{
public bool DoSomething(int i, object o) => o.ToString() == ""whatever"" || i == 42;
}
");

[Test]
Expand Down Expand Up @@ -323,6 +391,50 @@ public class TestMe : IEquatable<TestMe>
VerifyCSharpFix(OriginalCode, FixedCode);
}

[Test]
public void Code_gets_fixed_for_complex_class_with_value_type_and_array_type_OR_comparison_in_if_clause_if_reference_type_comparison_comes_first_and_IsNotNull_pattern_expression()
{
const string OriginalCode = @"
using System;
using System.Linq;
public class TestMe : IEquatable<TestMe>
{
public int Value { get; set; }
public object Data { get; set; }
public bool Equals(TestMe other)
{
if (other is not null && Data == other.Data && Value == other.Value)
return true;
return false;
}
}
";

const string FixedCode = @"
using System;
using System.Linq;
public class TestMe : IEquatable<TestMe>
{
public int Value { get; set; }
public object Data { get; set; }
public bool Equals(TestMe other)
{
if (other is not null && Value == other.Value && Data == other.Data)
return true;
return false;
}
}
";

VerifyCSharpFix(OriginalCode, FixedCode);
}

[Test]
public void Code_gets_fixed_for_class_with_parenthesized_value_type_and_collection_type_comparison()
{
Expand Down Expand Up @@ -363,6 +475,58 @@ public bool Equals(TestMe other)
VerifyCSharpFix(OriginalCode, FixedCode);
}

[Test]
public void Code_gets_fixed_for_class_with_parenthesized_value_type_and_collection_type_comparison_when_spanning_multiple_lines()
{
const string OriginalCode = @"
using System;
using System.Collections.Generic;
using System.Linq;
public class TestMe
{
public List<Guid> Guids { get; set; }
public List<object> Objects { get; set; }
public bool Equals(TestMe other)
{
if (Guids.Count > 0 &&
Guids.All(_ => _ != Guid.Empty) &&
Objects.Count == 1 &&
Objects.First() != null)
{
return true;
}
}
}
";

const string FixedCode = @"
using System;
using System.Collections.Generic;
using System.Linq;
public class TestMe
{
public List<Guid> Guids { get; set; }
public List<object> Objects { get; set; }
public bool Equals(TestMe other)
{
if (Guids.Count > 0 &&
Objects.Count == 1 &&
Guids.All(_ => _ != Guid.Empty) &&
Objects.First() != null)
{
return true;
}
}
}
";

VerifyCSharpFix(OriginalCode, FixedCode);
}

//// TODO RKN: Add more complex AND/OR Comparisons with at least 3 comparisons and parenthesized ones

protected override string GetDiagnosticId() => MiKo_5018_LogicalValueComparisonsComeFirstAnalyzer.Id;
Expand Down

0 comments on commit 0a19d6a

Please sign in to comment.