Skip to content
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.

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

CA1820 incorrectly fires in LINQ expressions #1508

Closed
KrisVandermotten opened this issue Jan 6, 2018 · 2 comments · Fixed by #4621
Closed

CA1820 incorrectly fires in LINQ expressions #1508

KrisVandermotten opened this issue Jan 6, 2018 · 2 comments · Fixed by #4621
Labels
Area-Microsoft.CodeAnalysis.NetAnalyzers Bug The product is not behaving according to its current intended design help wanted The issue is up-for-grabs, and can be claimed by commenting
Milestone

Comments

@KrisVandermotten
Copy link

KrisVandermotten commented Jan 6, 2018

Analyzer package

Microsoft Code Analysis, version 2.6.0.6241303

Analyzer

CA1820

Repro steps

Consider the following program:

using System.Linq;

class C
{
    void M(IQueryable<string> strings)
    {
        var q = from s in strings
                where s == ""
                select s;
    }
}

Expected behavior

CA1820 is not raised. Indeed, the comparison to "" happens in a LINQ expression.

Actual behavior

CA1820 is raised:

Test for empty strings using 'string.Length' property or 'string.IsNullOrEmpty' method instead of an Equality check.	
Comparing strings by using the String.Length property or the String.IsNullOrEmpty method is significantly faster than using Equals.

Given that the comparison happens in a LINQ expression, there is no way to tell if using string.Length will be faster or not. In fact, it is very much possible that the LINQ provider will not be able to translate a comparison to String.Length.

Note that U2U1100 from the U2U Consult Performance Analyzers for C# 7 does not have this problem.

@mavasani mavasani added Bug The product is not behaving according to its current intended design Area-Microsoft.CodeAnalysis.NetAnalyzers labels Jan 9, 2018
@mavasani mavasani added this to the 15.6 milestone Jan 9, 2018
@jinujoseph jinujoseph assigned ivanbasov and unassigned 333fred Jan 31, 2018
@jinujoseph jinujoseph modified the milestones: 15.6, 15.7 Jan 31, 2018
@jinujoseph jinujoseph modified the milestones: 15.7, 15.8 Feb 20, 2018
@jinujoseph jinujoseph modified the milestones: 15.8, 15.9 Apr 25, 2018
@mavasani mavasani removed this from the 15.9 milestone Jul 1, 2019
@sharwell sharwell added help wanted The issue is up-for-grabs, and can be claimed by commenting and removed Up for Grabs labels Aug 1, 2019
@Evangelink
Copy link
Member

@mavasani Do you think that's overkill to exclude all cases where the parent is a query syntax or a query method? Doing so all queries, even on simple collections, will start to ignore the issue. This could be tweaked by checking that the left type is IQueryable. WDYT?

@KrisVandermotten
Copy link
Author

This is not about IQueryable<T>, nor about query syntax, but rather about expression trees.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Microsoft.CodeAnalysis.NetAnalyzers Bug The product is not behaving according to its current intended design help wanted The issue is up-for-grabs, and can be claimed by commenting
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants