Skip to content

Fix SA1131 to not treat "complex" expressions as a literal #3760

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Dec 20, 2023
Merged

Fix SA1131 to not treat "complex" expressions as a literal #3760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
32646fb
Fix SA1131 to not treat "complex" expressions as a literal
bjornhellander Dec 20, 2023
e099358
Fix review comments
bjornhellander Dec 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 57 additions & 1 deletion StyleCop.Analyzers/StyleCop.Analyzers.Test/ReadabilityRules/SA1131UnitTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = $@"
Expand All @@ -546,6 +594,10 @@ private static void Method1()
private void Method2()
{{
}}

private static void Method3<T>()
{{
}}
}}
";

Expand All @@ -572,6 +624,10 @@ private static void Method1()
private void Method2()
{{
}}

private static void Method3<T>()
{{
}}
}}
";

Expand Down
6 changes: 5 additions & 1 deletion StyleCop.Analyzers/StyleCop.Analyzers/ReadabilityRules/SA1131UseReadableConditions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down