CA1862 suggestion changing semantics of the comparison.

[buyaa-n]

CA1862: Prefer using 'StringComparer'/'StringComparison' to perform case-insensitive string comparisons suggestion changing semantics of the comparison

Warning: Prefer using 'string.Equals(string, StringComparison)' to perform a case-insensitive comparison

using System;

// These are both lower case Greek Sigmas
string a = "σ";
string b = "ς";

// Prints True
Console.WriteLine(a.Equals(b, StringComparison.InvariantCultureIgnoreCase));

string lowerA = a.ToLowerInvariant();
string lowerB = b.ToLowerInvariant();

// Prints False
Console.WriteLine(lowerA == lowerB);

Originally posted by @MichalStrehovsky in #89630 (comment):
This is changing semantics of the comparison. Do we really have an analyzer that suggests people blindly do this?

[ghost]

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

Issue Details

---

          This is changing semantics of the comparison. Do we really have an analyzer that suggests people blindly do this?

using System;

// These are both lower case Greek Sigmas
string a = "σ";
string b = "ς";

// Prints True
Console.WriteLine(a.Equals(b, StringComparison.InvariantCultureIgnoreCase));

string lowerA = a.ToLowerInvariant();
string lowerB = b.ToLowerInvariant();

// Prints False
Console.WriteLine(lowerA == lowerB);

Originally posted by @MichalStrehovsky in #89630 (comment)

Author:	buyaa-n
Assignees:	-
Labels:	area-System.Globalization, untriaged, needs-area-label
Milestone:	-

[buyaa-n]

Created the issue in runtime in order to discuss the appropriate fix for this issue @carlossanlop @tarekgh PTAL

[tarekgh]

Using σ and ς is very-special case in the Unicode. If anyone is doing ToLower for comparisons in general will already be broken. For example, encountering Σ and using ToLower, will hold Σ == σ and Σ != ς. which would be wrong result. Actually, in general we recommend users to use ToUpper for compare casing because of such issues. Although, it is correct that Equals(b, StringComparison.InvariantCultureIgnoreCase) can have semantic change in this case, but it is better to have the analyzer flag this use cases of the ToLower anyway.

I prefer to keep the analyzer and we add something to the analyzer message clarify exceptional cases can hold different results. At least get users to think about it.

[MichalStrehovsky]

Using σ and ς is very-special case in the Unicode. If anyone is doing ToLower for comparisons in general will already be broken. For example, encountering Σ and using ToLower, will hold Σ == σ and Σ != ς. which would be wrong result.

Yep, this is true - but for the code that I pointed out, this a behavior of the CoreCLR implementation of reflection (that is written in C++) that NativeAOT reflection (written in C#) is simply matching. "Fixing" it on NativeAOT would introduce a behavioral difference between the reflection stacks. Whenever this analyzer kicks in, people need to be aware it can change behavior. It is very subtle.

[tarekgh]

Whenever this analyzer kicks in, people need to be aware it can change behavior. It is very subtle.

That is what I suggested to address that we add something to the analyzer message clarify exceptional cases can hold different results. At least get users to think about it.

[Bellarmine-Head]

Prefer using 'string.Equals(string, StringComparison)' to perform a case-insensitive comparison

It is to be preferred to use string.Equals(string, StringComparison) but the statement on its own doesn't go far enough.

What I have seen in related issues, e.g.

• [Analyzer] Recommend string.ToLower{Invariant}().Contains/IndexOf/StartsWith("lowercasestring") be replaced by case-insensitive comparison #78606
• Recommend using Equals with StringComparison instead of string.ToLower() == otherString.ToLower() roslyn-analyzers#6720

is a literal or rote translation of existing code to the equivalent StringComparison options. What you end up with is a StringComparison option with the word "Culture" in it... e.g. StringComparison.CurrentCultureIgnoreCase.

But programmers have an extra choice when using StringComparison (and StringComparer) wrt case-insensitivity... and that choice or option is OrdinalIgnoreCase.

The problem with an issue like #78606 is that the word "ordinal" doesn't appear once.

So we need to get OrdinalIgnoreCase on to people's radars, and this option needs to be taken into account by the analyzers that concern themselves with case-insensitive string comparisons.

If the programmer is comparing strings case-insensitively because the strings are identifiers, then OrdinalIgnoreCase is the only correct choice. Any option with the word "Culture" in it is wrong. This is in fact standard, established (and correct) Microsoft advice, as seen here: https://learn.microsoft.com/en-us/dotnet/standard/base-types/best-practices-strings#choosing-a-stringcomparison-member-for-your-method-call

For all the following types of strings, OrdinalIgnoreCase is what you must be using:-

• Case-insensitive internal identifiers.
• Case-insensitive identifiers in standards such as XML and HTTP.
• File paths.
• Registry keys and values.
• Environment variables.
• Resource identifiers (for example, handle names).
• Case-insensitive security-related settings.

So when would you use a StringComparison option with the word "Culture" in it? For these:-

• Some persisted, linguistically relevant data.
• Display of linguistic data that requires a fixed sort order.
• Data displayed to the user.
• Most user input.

From experience (C# and .NET since 2002) I can say that comparing "linguistically relevant" strings case-insensitively is something I've never done, whereas comparing identifiers (etc.) using OrdinalIgnoreCase is something I do twenty times a day every day. It's the same for any/all code by others that I've reviewed.

The trouble with the table that seems to be at the heart of these new or revised analyzers, is that it the right-hand column is only correct if the string being operated on is cultural / linguistic. This is a dangerous assumption to make, I feel.

The analyzers in this space should be weighted towards recommending OrdinalIgnoreCase for all case-insensitive string comparisons, but may wish to mention the cultural string comparison options for the far less likely case that the string is cultural / linguistic.

[ericstj]

@carlossanlop were you planning to fix this for 8.0?

[carlossanlop]

Fixed by dotnet/roslyn-analyzers#6948