Methods which take `StringComparison` are not sufficiently clear about how hard they are to use correctly

[Smaug123]

Methods which take StringComparison are not sufficiently clear about how hard they are to use correctly

I claim that it is easy to read e.g. https://learn.microsoft.com/en-us/dotnet/api/system.string.endswith?view=net-8.0 and come away thinking that "I'll be fine, I'll just stick with the invariant culture because I don't care about anything above code point 127". This is amply discussed in Best Practices for Using Strings, and the docs do vaguely suggest that perhaps you might want to read it; but I believe what is actually required in the Notes to Callers is "you must read Best Practices for Using Strings, in full, before using any method that can take a StringComparison".

In particular, for example, the following documentation (which is where one would naturally look to find the "non-printing characters are omitted in most configurations" footgun) is entirely unhelpful except insofar as it suggests that you might want to read the document whose purpose is to instil the correct sense of unease:

As explained in Best Practices for Using Strings, we recommend that you avoid calling string comparison methods that substitute default values and instead call methods that require parameters to be explicitly specified. To determine whether a string ends with a particular substring by using the string comparison rules of the current culture, call the EndsWith(String, StringComparison) method overload with a value of CurrentCulture for its comparisonType parameter.

I read this paragraph entirely as "string comparison is culture-specific, so be careful about which culture you're in", and not as the much more important "… and also bear in mind that you probably do not know what any given culture's string comparison rules are, even if you think you do".

I believe every method which has an overload which takes a StringComparison should contain a Note to Callers that warns the user that they must read the Best Practices. If I were writing the docs, they would say the following:

You must read Best Practices in full before using this function, even if you think you currently know what a culture-sensitive comparison is. Read it all; do not just read the section about the StringComparison you currently intend using. (You may skip this step if you can already articulate why the invariant culture is almost never the correct culture to use for string comparison and related operations.)

Target framework

True of .NET 5 through at least .NET 8.

• .NET Core
• [ ? ] .NET Framework
• .NET Standard

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

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

Issue Details

---

Methods which take StringComparison are not sufficiently clear about how hard they are to use correctly

I claim that it is easy to read e.g. https://learn.microsoft.com/en-us/dotnet/api/system.string.endswith?view=net-8.0 and come away thinking that "I'll be fine, I'll just stick with the invariant culture because I don't care about anything above code point 127". This is amply discussed in Best Practices for Using Strings, and the docs do vaguely suggest that perhaps you might want to read it; but I believe what is actually required in the Notes to Callers is "you must read Best Practices for Using Strings, in full, before using any method that can take a StringComparison".

In particular, for example, the following documentation (which is where one would naturally look to find the "non-printing characters are omitted in most configurations" footgun) is entirely unhelpful except insofar as it suggests that you might want to read the document whose purpose is to instil the correct sense of unease:

As explained in Best Practices for Using Strings, we recommend that you avoid calling string comparison methods that substitute default values and instead call methods that require parameters to be explicitly specified. To determine whether a string ends with a particular substring by using the string comparison rules of the current culture, call the EndsWith(String, StringComparison) method overload with a value of CurrentCulture for its comparisonType parameter.

I read this paragraph entirely as "string comparison is culture-specific, so be careful about which culture you're in", and not as the much more important "… and also bear in mind that you probably do not know what any given culture's string comparison rules are, even if you think you do".

I believe every method which has an overload which takes a StringComparison should contain a Note to Callers that warns the user that they must read the Best Practices. If I were writing the docs, they would say the following:

You must read Best Practices in full before using this function, even if you think you currently know what a culture-sensitive comparison is. Read it all; do not just read the section about the StringComparison you currently intend using. (You may skip this step if you can already articulate why the invariant culture is almost never the correct culture to use for string comparison and related operations.)

Target framework

True of .NET 5 through at least .NET 8.

• .NET Core
• [ ? ] .NET Framework
• .NET Standard

Author:	Smaug123
Assignees:	-
Labels:	untriaged, Pri3, area-System.Globalization, :watch: Not Triaged
Milestone:	-

[MSDN-WhiteKnight]

The suggested wording is useless as it consists solely of weasel words that do not add anything for the reader. API docs are supposed to be pretty consise. We should just state it is recommended to pass comparison explicitly and link to article that provides more details. It's not possible to understand all rules of culture-sensitive comparisons and i dont' think we should encourage it. Instead it's better for readers to focus on understanding scenarios where it's safe to use ordinal comparison (the majority of cases) and just stick with it.

The linked docs seems fine actually, maybe it should put more emphasis on Ordinal instead of CurrentCulture as it's least suprising.

[Smaug123]

I entirely agree that we shouldn't encourage understanding all rules of culture-sensitive comparisons! My assertion is that the current docs are insufficiently clear that "you must use ordinal comparison unless you know exactly what you are doing". Note, for example, that the portion of the docs I quoted do not even mention the word "ordinal", and your implied suggestion ("it is recommended to pass comparison explicitly") also does not mention the word "ordinal".

There's a reason I said "If I were writing the docs…", by the way, which is that I have very strong opinions on documentation which are likely incompatible with many other people's. I gave that example so as to gesture to what I would have preferred the documentation to say; I intended it to be entirely unspecified how the documentation ends up saying it.

(Out of interest, could you define "weasel word" for me? I am not sure we agree on what the phrase means. To me, inherited from Wikipedia, weasel words are phrases which avoid saying anything but do so in such a way that they look like they're saying something. A sentence which starts "You must", or indeed any sentence in the imperative mood, will struggle very hard to be a weasel phrase under my understanding.)

[Smaug123]

I would implore you to try and forget all your current understanding of the pitfalls of .NET strings, and come at this from the perspective of a user who is used to how every other language does it, i.e. non-culture-aware. Such people likely do not understand the (significant!) ramifications of the single sentence "This method performs a word (case-sensitive and culture-sensitive) comparison using the current culture.", and there's currently nothing on https://learn.microsoft.com/en-us/dotnet/api/system.string.endswith?view=net-8.0 which even suggests to such a user that they will make a mistake by default. (The important fact, "culture-sensitive", far from being emphasised, is in parentheses!) The only such clause is "we recommend that you avoid calling string comparison methods that substitute default values", and that could perfectly plausibly be intended as "… because it's generally good practice to be explicit rather than relying on implicit behaviour"; there is nothing in the current documentation to suggest that you will probably be writing actual bugs by default, unless you click through and read most of Best Practices. This is why I suggested being much more explicit that the user must read Best Practices.

[tarekgh]

It may be worth just adding a link to the doc https://learn.microsoft.com/en-us/dotnet/csharp/how-to/compare-strings when mentioning something like This method performs a word (case-sensitive and culture-sensitive or ordinal comparisons. Usually, users who are new or not familiar with the APIs will explore this link and get a better idea about the differences and the expectation.