Merge pull request #3760 from bjornhellander/feature/sa1131-left-expr…

…essions Fix SA1131 to not treat "complex" expressions as a literal
DotNetAnalyzers · Dec 20, 2023 · 73a46ae · 73a46ae
2 parents 36cc2cc + e099358
commit 73a46ae
Show file tree

Hide file tree

Showing 2 changed files with 62 additions and 2 deletions.
diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test/ReadabilityRules/SA1131UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test/ReadabilityRules/SA1131UnitTests.cs
@@ -521,7 +521,55 @@ public void Test()
         [InlineData("Method1", "Const1", false)]
         [InlineData("Method2", "Const1", false)]
         [WorkItem(3677, "https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/3677")]
-        public async Task TestMethodsAsync(string expr1, string expr2, bool shouldTrigger)
+        public async Task TestSimpleMethodsAsync(string expr1, string expr2, bool shouldTrigger)
+        {
+            await this.TestMethodAsync(expr1, expr2, shouldTrigger).ConfigureAwait(false);
+        }
+
+        [Theory]
+        [InlineData("TestClass.Method1", "arg", true)]
+        [InlineData("this.Method2", "arg", true)]
+        [InlineData("TestClass.Method1", "field1", true)]
+        [InlineData("this.Method2", "field1", true)]
+        [InlineData("TestClass.Method1", "field2", true)]
+        [InlineData("this.Method2", "field2", true)]
+        [InlineData("Const1", "TestClass.Method1", false)]
+        [InlineData("Const1", "this.Method2", false)]
+        [InlineData("TestClass.Method1", "Const1", false)]
+        [InlineData("this.Method2", "Const1", false)]
+        [InlineData("Method3<int>", "arg", true)]
+        [InlineData("TestClass.Method3<int>", "arg", true)]
+        [WorkItem(3759, "https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/3759")]
+        public async Task TestComplexMethodsAsync(string expr1, string expr2, bool shouldTrigger)
+        {
+            await this.TestMethodAsync(expr1, expr2, shouldTrigger).ConfigureAwait(false);
+        }
+
+        [Theory]
+        [InlineData("==")]
+        [InlineData("!=")]
+        [InlineData(">=")]
+        [InlineData("<=")]
+        [InlineData(">")]
+        [InlineData("<")]
+        [WorkItem(3759, "https://github.com/DotNetAnalyzers/StyleCopAnalyzers/issues/3759")]
+        public async Task TestComplexLeftHandSideExpressionAsync(string @operator)
+        {
+            var testCode = $@"
+using System;
+public class TypeName
+{{
+    public void Test(int x, int y, Func<int> a)
+    {{
+        var r1 = x + 1 {@operator} y;
+        var r2 = -x {@operator} y;
+        var r3 = a() {@operator} y;
+    }}
+}}";
+            await VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).ConfigureAwait(false);
+        }
+
+        private async Task TestMethodAsync(string expr1, string expr2, bool shouldTrigger)
         {
             var testExpr = $"{expr1} == {expr2}";
             var testCode = $@"
@@ -546,6 +594,10 @@ private static void Method1()
     private void Method2()
     {{
     }}
+
+    private static void Method3<T>()
+    {{
+    }}
 }}
 ";
 
@@ -572,6 +624,10 @@ private static void Method1()
     private void Method2()
     {{
     }}
+
+    private static void Method3<T>()
+    {{
+    }}
 }}
 ";
 

diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/ReadabilityRules/SA1131UseReadableConditions.cs b/StyleCop.Analyzers/StyleCop.Analyzers/ReadabilityRules/SA1131UseReadableConditions.cs
@@ -86,7 +86,11 @@ private static bool IsLiteral(ExpressionSyntax expression, SemanticModel semanti
             switch (symbol)
             {
             case IFieldSymbol fieldSymbol when fieldSymbol.IsStatic && fieldSymbol.IsReadOnly:
-            case IMethodSymbol:
+                return true;
+
+            // NOTE: Without the when clause, this would also treat e.g unary or binary expressions as literals,
+            // since GetSymbolInfo returns the operator method in those cases.
+            case IMethodSymbol when expression is NameSyntax or MemberAccessExpressionSyntax:
                 return true;
 
             default: