From 32646fb805f7c19d3096b998342c120b34fa7fdf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Hellander?= Date: Wed, 20 Dec 2023 21:16:43 +0100 Subject: [PATCH 1/2] Fix SA1131 to not treat "complex" expressions as a literal #3759 --- .../ReadabilityRules/SA1131UnitTests.cs | 24 +++++++++++++++++++ .../SA1131UseReadableConditions.cs | 6 ++++- 2 files changed, 29 insertions(+), 1 deletion(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test/ReadabilityRules/SA1131UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test/ReadabilityRules/SA1131UnitTests.cs index 1ccd8509a..d93691edb 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers.Test/ReadabilityRules/SA1131UnitTests.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers.Test/ReadabilityRules/SA1131UnitTests.cs @@ -584,5 +584,29 @@ private void Method2() await VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).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 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); + } } } diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/ReadabilityRules/SA1131UseReadableConditions.cs b/StyleCop.Analyzers/StyleCop.Analyzers/ReadabilityRules/SA1131UseReadableConditions.cs index e3af5070e..204ffe3cf 100644 --- 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 IdentifierNameSyntax: return true; default: From e099358f7d0e9bf67a815aaca22158e626cc584d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bj=C3=B6rn=20Hellander?= Date: Wed, 20 Dec 2023 22:20:37 +0100 Subject: [PATCH 2/2] Fix review comments #3759 --- .../ReadabilityRules/SA1131UnitTests.cs | 82 +++++++++++++------ .../SA1131UseReadableConditions.cs | 2 +- 2 files changed, 58 insertions(+), 26 deletions(-) diff --git a/StyleCop.Analyzers/StyleCop.Analyzers.Test/ReadabilityRules/SA1131UnitTests.cs b/StyleCop.Analyzers/StyleCop.Analyzers.Test/ReadabilityRules/SA1131UnitTests.cs index d93691edb..2565c73fd 100644 --- 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", "arg", true)] + [InlineData("TestClass.Method3", "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 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() + {{ + }} }} "; @@ -572,6 +624,10 @@ private static void Method1() private void Method2() {{ }} + + private static void Method3() + {{ + }} }} "; @@ -584,29 +640,5 @@ private void Method2() await VerifyCSharpDiagnosticAsync(testCode, DiagnosticResult.EmptyDiagnosticResults, CancellationToken.None).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 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); - } } } diff --git a/StyleCop.Analyzers/StyleCop.Analyzers/ReadabilityRules/SA1131UseReadableConditions.cs b/StyleCop.Analyzers/StyleCop.Analyzers/ReadabilityRules/SA1131UseReadableConditions.cs index 204ffe3cf..e6fe9ffde 100644 --- a/StyleCop.Analyzers/StyleCop.Analyzers/ReadabilityRules/SA1131UseReadableConditions.cs +++ b/StyleCop.Analyzers/StyleCop.Analyzers/ReadabilityRules/SA1131UseReadableConditions.cs @@ -90,7 +90,7 @@ private static bool IsLiteral(ExpressionSyntax expression, SemanticModel semanti // 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 IdentifierNameSyntax: + case IMethodSymbol when expression is NameSyntax or MemberAccessExpressionSyntax: return true; default: