Skip to content

Commit

Permalink
[browser] Fix HybridGlobalization, SoftHyphen is not ignored when i…
Browse files Browse the repository at this point in the history 
…t should be (dotnet#105946)

* Ignore SoftHyphen.

* Update docs.

* Implementation with `matchLength` should throw with PNSE, not ArgumentException.

* String.Replace uses IndexOf with matchLength, so expect it to throw.
  • Loading branch information
@ilonatommy
ilonatommy authored Aug 15, 2024
1 parent 0396978 commit a842025
Show file tree
Hide file tree
Showing 7 changed files with 81 additions and 37 deletions.
19 changes: 16 additions & 3 deletions docs/design/features/globalization-hybrid-mode.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -270,13 +270,12 @@ Dependencies:

Web API does not expose locale-sensitive endsWith/startsWith function. As a workaround, both strings get normalized and weightless characters are removed. Resulting strings are cut to the same length and comparison is performed. This approach, beyond having the same compare option limitations as described under **String comparison**, has additional limitations connected with the workaround used. Because we are normalizing strings to be able to cut them, we cannot calculate the match length on the original strings. Methods that calculate this information throw PlatformNotSupported exception:

- [CompareInfo.IsPrefix](https://learn.microsoft.com/dotnet/api/system.globalization.compareinfo.isprefix?view=net-8.0#system-globalization-compareinfo-isprefix(system-readonlyspan((system-char))-system-readonlyspan((system-char))-system-globalization-compareoptions-system-int32@))
- [CompareInfo.IsSuffix](https://learn.microsoft.com/dotnet/api/system.globalization.compareinfo.issuffix?view=net-8.0#system-globalization-compareinfo-issuffix(system-readonlyspan((system-char))-system-readonlyspan((system-char))-system-globalization-compareoptions-system-int32@))
- [CompareInfo.IsPrefix](https://learn.microsoft.com/dotnet/api/system.globalization.compareinfo.isprefix?view=#system-globalization-compareinfo-isprefix(system-readonlyspan((system-char))-system-readonlyspan((system-char))-system-globalization-compareoptions-system-int32@))
- [CompareInfo.IsSuffix](https://learn.microsoft.com/dotnet/api/system.globalization.compareinfo.issuffix?view=system-globalization-compareinfo-issuffix(system-readonlyspan((system-char))-system-readonlyspan((system-char))-system-globalization-compareoptions-system-int32@))

- `IgnoreSymbols`
Only comparisons that do not skip character types are allowed. E.g. `IgnoreSymbols` skips symbol-chars in comparison/indexing. All `CompareOptions` combinations that include `IgnoreSymbols` throw `PlatformNotSupportedException`.


**String indexing**

Affected public APIs:
Expand All @@ -287,6 +286,15 @@ Affected public APIs:

Web API does not expose locale-sensitive indexing function. There is a discussion on adding it: https://github.com/tc39/ecma402/issues/506. In the current state, as a workaround, locale-sensitive string segmenter combined with locale-sensitive comparison is used. This approach, beyond having the same compare option limitations as described under **String comparison**, has additional limitations connected with the workaround used. Information about additional limitations:

- Methods that calculate `matchLength` always return throw PlatformNotSupported exception:

[CompareInfo.IndexOf](https://learn.microsoft.com/en-us/dotnet/api/system.globalization.compareinfo.indexof?view=system-globalization-compareinfo-indexof(system-readonlyspan((system-char))-system-readonlyspan((system-char))-system-globalization-compareoptions-system-int32@))

[CompareInfo.LastIndexOf](https://learn.microsoft.com/en-us/dotnet/api/system.globalization.compareinfo.lastindexof?view=system-globalization-compareinfo-lastindexof(system-readonlyspan((system-char))-system-readonlyspan((system-char))-system-globalization-compareoptions-system-int32@))

- String.Replace that uses `StringComparison` argument relies internally on IndexOf with `matchLength` argument. From this reason, it throws PlatformNotSupportedException:
[String.Replace](https://learn.microsoft.com/en-us/dotnet/api/system.string.replace?view=system-string-replace(system-string-system-string-system-stringcomparison))

- Support depends on [`Intl.segmenter's support`](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/Segmenter#browser_compatibility).

- `IgnoreSymbols`
Expand Down Expand Up @@ -463,6 +471,11 @@ Affected public APIs:
- String.IndexOf
- String.LastIndexOf

Methods that calculate `matchLength` throw PlatformNotSupported exception:
[CompareInfo.IndexOf](https://learn.microsoft.com/en-us/dotnet/api/system.globalization.compareinfo.indexof?view=system-globalization-compareinfo-indexof(system-readonlyspan((system-char))-system-readonlyspan((system-char))-system-globalization-compareoptions-system-int32@))

[CompareInfo.LastIndexOf](https://learn.microsoft.com/en-us/dotnet/api/system.globalization.compareinfo.lastindexof?view=system-globalization-compareinfo-lastindexof(system-readonlyspan((system-char))-system-readonlyspan((system-char))-system-globalization-compareoptions-system-int32@))

Mapped to Apple Native API `rangeOfString:options:range:locale:`(https://developer.apple.com/documentation/foundation/nsstring/1417348-rangeofstring?language=objc)

In `rangeOfString:options:range:locale:` objects are compared by checking the Unicode canonical equivalence of their code point sequences.
Expand Down
6 changes: 1 addition & 5 deletions src/libraries/Common/tests/Tests/System/StringTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1730,7 +1730,6 @@ public static IEnumerable<object[]> EndsWith_StringComparison_TestData()

[Theory]
[MemberData(nameof(EndsWith_StringComparison_TestData))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/95473", typeof(PlatformDetection), nameof(PlatformDetection.IsHybridGlobalizationOnBrowser))]
public static void EndsWith_StringComparison(string s, string value, StringComparison comparisonType, bool expected)
{
if (comparisonType == StringComparison.CurrentCulture)
Expand Down Expand Up @@ -4925,9 +4924,7 @@ public static IEnumerable<object[]> StartsWith_StringComparison_TestData()

if (PlatformDetection.IsNotInvariantGlobalization && PlatformDetection.IsNotHybridGlobalizationOnApplePlatform)
{
// "https://github.com/dotnet/runtime/issues/95473"
if (PlatformDetection.IsNotHybridGlobalizationOnBrowser)
yield return new object[] { "Hello", SoftHyphen + "Hel", StringComparison.CurrentCulture, true };
yield return new object[] { "Hello", SoftHyphen + "Hel", StringComparison.CurrentCulture, true };
}

// CurrentCultureIgnoreCase
Expand Down Expand Up @@ -4999,7 +4996,6 @@ public static IEnumerable<object[]> StartsWith_StringComparison_TestData()

[Theory]
[MemberData(nameof(StartsWith_StringComparison_TestData))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/95473", typeof(PlatformDetection), nameof(PlatformDetection.IsHybridGlobalizationOnBrowser))]
public static void StartsWith_StringComparison(string s, string value, StringComparison comparisonType, bool expected)
{
if (comparisonType == StringComparison.CurrentCulture)
Expand Down
6 changes: 6 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1057,6 +1057,12 @@ private unsafe int IndexOf(ReadOnlySpan<char> source, ReadOnlySpan<char> value,
{
Debug.Assert(matchLengthPtr != null);
*matchLengthPtr = 0;
#if TARGET_BROWSER
if (GlobalizationMode.Hybrid)
{
throw new PlatformNotSupportedException(SR.PlatformNotSupported_HybridGlobalizationWithMatchLength);
}
#endif

int retVal = 0;

Expand Down
21 changes: 16 additions & 5 deletions ...s/System.Runtime/tests/System.Globalization.Tests/CompareInfo/CompareInfoTests.IndexOf.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -334,7 +334,22 @@ public void IndexOf_Invalid()
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's", 'b', 0, CompareOptions.StringSort));
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's", 'c', 0, 2, CompareOptions.StringSort));
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's".AsSpan(), "b".AsSpan(), CompareOptions.StringSort));
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's".AsSpan(), "b".AsSpan(), CompareOptions.StringSort, out _));
if (PlatformDetection.IsHybridGlobalizationOnBrowser)
{
Assert.Throws<PlatformNotSupportedException>(() => s_invariantCompare.IndexOf("Test's".AsSpan(), "b".AsSpan(), CompareOptions.StringSort, out _));
Assert.Throws<PlatformNotSupportedException>(() => s_invariantCompare.IndexOf("Test's".AsSpan(), "b".AsSpan(), CompareOptions.Ordinal | CompareOptions.IgnoreWidth, out _));
Assert.Throws<PlatformNotSupportedException>(() => s_invariantCompare.IndexOf("Test's".AsSpan(), "b".AsSpan(), (CompareOptions)(-1), out _));
Assert.Throws<PlatformNotSupportedException>(() => s_invariantCompare.IndexOf("Test's".AsSpan(), "b".AsSpan(), (CompareOptions)0x11111111, out _));
Assert.Throws<PlatformNotSupportedException>(() => s_invariantCompare.IndexOf("Test's".AsSpan(), "b".AsSpan(), CompareOptions.OrdinalIgnoreCase | CompareOptions.IgnoreWidth, out _));
}
else
{
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's".AsSpan(), "b".AsSpan(), CompareOptions.StringSort, out _));
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's".AsSpan(), "b".AsSpan(), CompareOptions.Ordinal | CompareOptions.IgnoreWidth, out _));
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's".AsSpan(), "b".AsSpan(), (CompareOptions)(-1), out _));
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's".AsSpan(), "b".AsSpan(), (CompareOptions)0x11111111, out _));
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's".AsSpan(), "b".AsSpan(), CompareOptions.OrdinalIgnoreCase | CompareOptions.IgnoreWidth, out _));
}

AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's", "Tests", CompareOptions.Ordinal | CompareOptions.IgnoreWidth));
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's", "Tests", 0, CompareOptions.Ordinal | CompareOptions.IgnoreWidth));
Expand All @@ -343,7 +358,6 @@ public void IndexOf_Invalid()
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's", 'b', 0, CompareOptions.Ordinal | CompareOptions.IgnoreWidth));
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's", 'c', 0, 2, CompareOptions.Ordinal | CompareOptions.IgnoreWidth));
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's".AsSpan(), "b".AsSpan(), CompareOptions.Ordinal | CompareOptions.IgnoreWidth));
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's".AsSpan(), "b".AsSpan(), CompareOptions.Ordinal | CompareOptions.IgnoreWidth, out _));

AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's", "Tests", CompareOptions.OrdinalIgnoreCase | CompareOptions.IgnoreWidth));
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's", "Tests", 0, CompareOptions.OrdinalIgnoreCase | CompareOptions.IgnoreWidth));
Expand All @@ -352,7 +366,6 @@ public void IndexOf_Invalid()
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's", 'b', 0, CompareOptions.OrdinalIgnoreCase | CompareOptions.IgnoreWidth));
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's", 'c', 0, 2, CompareOptions.OrdinalIgnoreCase | CompareOptions.IgnoreWidth));
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's".AsSpan(), "b".AsSpan(), CompareOptions.OrdinalIgnoreCase | CompareOptions.IgnoreWidth));
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's".AsSpan(), "b".AsSpan(), CompareOptions.OrdinalIgnoreCase | CompareOptions.IgnoreWidth, out _));

AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's", "Tests", (CompareOptions)(-1)));
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's", "Tests", 0, (CompareOptions)(-1)));
Expand All @@ -361,7 +374,6 @@ public void IndexOf_Invalid()
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's", 'a', 0, (CompareOptions)(-1)));
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's", 'a', 0, 2, (CompareOptions)(-1)));
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's".AsSpan(), "b".AsSpan(), (CompareOptions)(-1)));
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's".AsSpan(), "b".AsSpan(), (CompareOptions)(-1), out _));

AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's", "Tests", (CompareOptions)0x11111111));
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's", "Tests", 0, (CompareOptions)0x11111111));
Expand All @@ -370,7 +382,6 @@ public void IndexOf_Invalid()
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's", 'a', 0, (CompareOptions)0x11111111));
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's", 'a', 0, 2, (CompareOptions)0x11111111));
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's".AsSpan(), "b".AsSpan(), (CompareOptions)0x11111111));
AssertExtensions.Throws<ArgumentException>("options", () => s_invariantCompare.IndexOf("Test's".AsSpan(), "b".AsSpan(), (CompareOptions)0x11111111, out _));

// StartIndex < 0
AssertExtensions.Throws<ArgumentOutOfRangeException>("startIndex", () => s_invariantCompare.IndexOf("Test", "Test", -1, CompareOptions.None));
Expand Down
Loading

0 comments on commit a842025

Please sign in to comment.