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

New Analyzer to avoid StringComparison #31059

Closed
EdLichtman opened this issue Jun 9, 2023 · 2 comments
Closed

New Analyzer to avoid StringComparison #31059

EdLichtman opened this issue Jun 9, 2023 · 2 comments
Labels
closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. customer-reported

Comments

@EdLichtman
Copy link

When using DbSet.Where(x => x.Equals(y)), then the dotnet recommended rules give a warning that you should have the code implement a String Case Comparison, such as: DbSet.Where(x => x.Equals(y, StringComparison.OrdinalIgnoreCase))

When you TreatWarningsAsErrors, like my company, you're left with disabling the pragma, or, using the == comparator.

However, our company has historically required you compare strings with .Equals, instead of == based on our coding conventions.

The problem is, if we're comparing 2 values that are strings, we shouldn't find out at runtime that a problem occurs if we include OrdinalIgnoreCase. It would be far better to find out at development time.

I'd like to see an analyzer that requires that you use == instead of .Equals if your delegate within the Linq statement compares string values. Or, if it's possible to counteract CA1309, then a rule that counteracts it if you're within that statement.

@roji
Copy link
Member

roji commented Jun 10, 2023

@EdLichtman there are several things that make this more complex than it seems.

First, from a feasibility/implementation point of view, the analyzer would need to identify string Equals specifically within EF LINQ queries (as opposed to other LINQ queries, e.g. regular LINQ-to-object or possibly even other non-EF queryable providers). This isn't an easy thing to do (at least, not efficiently), and we don't currently have that capability. See "Scenarios requiring identifying EF Core LINQ queries" in #22086.

But more importantly, the requirement to not use Equals isn't universal by any means, even within EF. First, some databases are case-insensitive by default (e.g. SQL Server), while others are case-sensitive (e.g. PostgreSQL, SQLite). An analyzer therefore can't propose that you replace Equals(x, StringComparison.OrdinalIgnoreCase) with equality, since that would could change the meaning of the query, depending on your database. And making the analyzer aware of the database being targeted is even a more difficult problem (not to mention that the same code could be used against multiple providers).

In fact, the Cosmos EF provider actually does support StringComparison.OrdinalIgnoreCase, since that makes sense on that particular database (it does not for relational databases, where column collations are usually the right way to manage case sensitivity, see docs).

The problem is, if we're comparing 2 values that are strings, we shouldn't find out at runtime that a problem occurs if we include OrdinalIgnoreCase. It would be far better to find out at development time.

I agree that ideally you get analyzers identifying all problems for you - but that's unfortunately not always feasible. Doing proper testing can ensure that such runtime errors are located very early as well.

@ajcvickers
Copy link
Member

Note from triage: this is not something we plan to support.

@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Jun 19, 2023
@ajcvickers ajcvickers added closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. and removed type-enhancement labels Jun 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. customer-reported
Projects
None yet
Development

No branches or pull requests

3 participants