Correctness bug: String.EndsWith may ignore trailing null characters

[Smaug123]

Description

String.EndsWith can give the wrong answer for a string which ends with a null character.

Reproduction Steps

> dotnet fsi
> "\n\000".EndsWith "\n";;
val it: bool = true

> "\n\000".EndsWith '\n';;
val it: bool = false

(The problem also manifests in the invariant culture.)

Expected behavior

The string consisting of a newline should not be considered a suffix of the string consisting of a newline and then a null character.

Actual behavior

The string consisting of a newline is considered to be a suffix of the string consisting of a newline and then a null character.

Regression?

No response

Known Workarounds

Use s.EndsWith '\n' instead.

Configuration

> dotnet --version     
7.0.203

Running on darwin-aarch64, obtained through Nixpkgs at commit 639d4f17218568afd6494dbd807bebb2beb9d6b3, macOS Ventura 13.3.1 (22E261).

Observed in .NET 6 on Windows x86-64 as well.

Other information

This can also be triggered with \001 or \010 instead of \000, but not \r.

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

Issue Details

---

Description

String.EndsWith can give the wrong answer for a string which ends with a null character.

Reproduction Steps

> dotnet fsi
> "\n\000".EndsWith "\n";;
val it: bool = true

> "\n\000".EndsWith '\n';;
val it: bool = false

(The problem also manifests in the invariant culture.)

Expected behavior

The string consisting of a newline should not be considered a suffix of the string consisting of a newline and then a null character.

Actual behavior

The string consisting of a newline is considered to be a suffix of the string consisting of a newline and then a null character.

Regression?

No response

Known Workarounds

Use s.EndsWith '\n' instead.

Configuration

> dotnet --version     
7.0.203

Running on darwin-aarch64, obtained through Nixpkgs at commit 639d4f17218568afd6494dbd807bebb2beb9d6b3, macOS Ventura 13.3.1 (22E261).

Observed in .NET 6 on Windows x86-64 as well.

Other information

This can also be triggered with \001 or \010 instead of \000, but not \r.

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

[Smaug123]

The following test fails flakily and finds the problem when it does. For me, the first example it found was "2\004".EndsWith "2".

namespace Test.TestString

open FsCheck
open NUnit.Framework

module Tests =
    [<Test>]
    let ``String manipulations`` () =
        let property (NonNull a) (NonNull b) (NonNull (s : string)) =
            let concat = sprintf "%s%s" a b
            let s = s.Substring (0, min s.Length b.Length)
            concat.EndsWith s = b.EndsWith s

        Check.One ({ Config.QuickThrowOnFailure with MaxTest = 100000 }, property)

(Though of course for production, a better test would be constructed by manipulating the generators such that the following reliably fails before the bugfix and succeeds after the fix:

[<Test>]
    let ``String manipulations`` () =
        let pos = ref 0
        let neg = ref 0
        let property (NonNull a) (NonNull b) bNonEmpty (NonNull (s : string)) =
            let b = sprintf "%s%c" b bNonEmpty
            let concat = sprintf "%s%s" a b
            let s = s.Substring (0, min s.Length b.Length)
            if concat.EndsWith s then
                pos.Value <- pos.Value + 1
            else
                neg.Value <- neg.Value + 1
            concat.EndsWith s = b.EndsWith s

        Check.One ({ Config.QuickThrowOnFailure with MaxTest = 1000 }, property)
        // Verify that we tested enough cases where EndsWith was true
        float pos.Value / (float pos.Value + float neg.Value) |> shouldBeGreaterThan 0.3

)

[Smaug123]

That test is also easily adjusted to check that there is also a StartsWith bug, by the way:

    let property (NonNull a) (NonNull b) (NonNull (s : string)) =
        let concat = sprintf "%s%s" a b
        let s = s.Substring (0, min s.Length a.Length)
        concat.StartsWith s = a.StartsWith s

It quickly finds that "\000o".StartsWith "o" is incorrectly true.

[tarekgh]

@Smaug123 You are calling the API with "\n" which uses linguistic comparison. null is ignorable character by Unicode which acts as if it does not exist in the string at all. You can try to use Ordinal string comparison option to force the operation to be ordinal instead.
When using the API that takes a character '\n' this operation is forced to be ordinal.

I encourage you reading the doc for more info and best practices

#4673

[Smaug123]

Ah, thanks. From the issue you posted and the various issues linked off that one, I think it's clear that this behaviour is extremely non-obvious, but that is unfixable. More usefully, I claim that it remains extremely non-obvious even after having read the doc you posted. I do agree that the linked doc teaches the extremely important lesson that "string operations will not in general align with your intuition unless you use Ordinal comparison; even the invariant culture will not save you"; however (as best as I can claim to understand a 3800-word document), while it does say that non-Ordinal comparisons ignore null characters, it does not say that they also ignore various control characters. Moreover, it says "non-Ordinal will not do what you expect" inside the section on Ordinal comparison, which rather means you have to know what you're looking for in order to find it.

For documentation which would have helped me personally at the time I discovered this issue, rather than after the fact: the docs at https://learn.microsoft.com/en-us/dotnet/api/system.string.endswith?view=net-8.0 don't really contain anything that would lead anyone to discover this behaviour unless they somehow already have an encyclopaedic knowledge of Unicode. In particular, the following documentation (which is where one would naturally look to find this kind of 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 your preferred culture's string comparison rules are, even if you think you do". (Note that I raised this issue and explicitly mentioned that it was a thing in the invariant culture! I believed I had fulfilled the recommendations of that section.) I would strongly recommend including in a Note to Callers something like "Note that most StringComparison cultures, including the invariant culture, apply linguistic rules which may not do what you expect. It is strongly recommended to read Best Practices for Using Strings in full before using strings." or similar.

[tarekgh]

We are tracking to improve that, you may look at dotnet/designs#207 for more info. Thanks for the feedback.

[tarekgh]

If you have any suggestion for improving the current docs too feel free to open a doc PR and we can discuss it.