Fix weightless code point handling in IndexOf and friends, plus other CompareInfo improvements #1514

GrabYourPitchforks · 2020-01-09T02:06:41Z

Fixes #25428 (new span-based APIs on CompareInfo).
Fixes #26621 (remove code duplicate in CompareInfo).
Contributes to #27912 (flow Rune through more APIs).
Fixes #34828 (CompareInfo.IndexOf zero-weight code point issue).

The string and CompareInfo classes have some incorrect shortcut logic when dealing with APIs like IndexOf. This logic doesn't account for the fact that certain non-empty strings may be linguistically equivalent to empty (i.e., consist only of weightless Unicode code points under some locale). See #1062 for a related issue affecting string.Equals and string.GetHashCode. This may cause the methods to return "no match" when they should have returned "a match exists". In the case of string.Replace, it could cause an endless loop.

The weightless code point issue only occurs when a culture-aware (i.e., not Ordinal or OrdinalIgnoreCase, and not under GlobalizationMode.Invariant) comparer is used.

Additionally, the LastIndexOf APIs have historically returned incorrect values when given empty string (or empty-equivalent) inputs. For example:

// U+200D below is the weightless zero-width joiner (ZWJ) code point.

Console.WriteLine("abcde".LastIndexOf(string.Empty, StringComparison.InvariantCulture)); // prints 4 (should print 5)
Console.WriteLine("abcde".LastIndexOf("\u200d", StringComparison.InvariantCulture));     // prints 5 (should print 5)

Console.WriteLine(string.Empty.LastIndexOf(string.Empty, StringComparison.InvariantCulture)); // prints 0  (should print 0)
Console.WriteLine(string.Empty.LastIndexOf("\u200d", StringComparison.InvariantCulture));     // prints -1 (should print 0)

See #13383 for more information on why the results above should be 5 and 0, respectively.

Below is the list of all existing public APIs affected by this PR.

string.IndexOf and string.LastIndexOf
string.StartsWith and string.EndsWith
string.Contains
string.Replace
CompareInfo.IndexOf and CompareInfo.LastIndexOf
CompareInfo.IsPrefix and CompareInfo.IsSuffix
MemoryExtensions.IndexOf and MemoryExtensions.LastIndexOf
MemoryExtensions.StartsWith and MemoryExtensions.EndsWith
MemoryExtensions.Contains

StringBuilder is not affected since it neither exposes a LastIndexOf method nor has culture-aware logic.

This PR also removes most duplicate code from the CompareInfo code paths, favoring span-based code wherever possible. Recent improvements to the JIT stack zeroing logic also allows us to remove our workaround for #8890.

Finally, this PR introduces some Rune-based APIs in the CompareInfo class. Not all of these APIs are yet approved so I'll need to get a quick email signoff. The pattern I followed is that if there was a CompareInfo API that accepted char as input, there should be an equivalent which accepts Rune as input in the same location. There's also an allocation-free variant of GetSortKey for folks who need it.

tarekgh · 2020-01-09T17:21:07Z

src/libraries/System.Private.CoreLib/src/System/String.Manipulation.cs

It would be better to move this check outside the loop for the perf sake. it is enough to check this condition first time only.

I couldn't quite figure out how to do this. The alternative would be to do an "is this equivalent to empty?" check at the beginning of the method, but that's a full p/invoke and call into NLS and I think it would vastly outweigh a simple if check in the middle of a loop.

Do you have a recommendation for how to do this?

You may do the following:
index = ci.IndexOf(this, oldValue, startIndex, this.Length - startIndex, options, &matchLength);
if (matchLength == 0)
{
throw....
}

do { if (index >= 0) { ... } .... index = ci.IndexOf(this, oldValue, startIndex, this.Length - startIndex, options, &matchLength); }

In reply to: 364889287 [](ancestors = 364889287)

My concern is that the call to IndexOf is itself a p/invoke, which has significant overhead. Certainly more overhead than a single if check (which the branch predictor will optimize not-taken) in the loop.

I am fine if you think it is not worth it.

tarekgh · 2020-01-09T17:40:45Z

src/libraries/System.Private.CoreLib/src/System/String.Searching.cs

I am not sure if this is correct but this something you want to double check. here is the scenario

this = ""
value = "A"
startIndex = -1

I believe this should return -1 but now we'll return 0 which is wrong I guess.