From 5c53e8ec9ca1a06a31c45936d3058fd6b1dc5961 Mon Sep 17 00:00:00 2001 From: Ilona Tomkowicz <32700855+ilonatommy@users.noreply.github.com> Date: Wed, 26 Jun 2024 12:41:05 +0000 Subject: [PATCH] [browser] Fix: `HybridGlobalization` Invariant function differs from non-Hybrid (#103037) * Remove invaraint cace change from JS and rely on methods used in InvariantGlobalization. * Fix - some internal libs are using `ChangeCaseCore` with invariant culture. * Fix NodeJS failures - it behaves like old chrome. * Remove duplicated code. --- .../src/Interop/Browser/Interop.TextInfo.cs | 2 - .../Common/tests/Tests/System/StringTests.cs | 2 - .../Globalization/TextInfo.WebAssembly.cs | 19 +++++-- .../src/System/Globalization/TextInfo.cs | 30 ++++++++++- .../System/StringTests.cs | 1 - .../FunctionalTests/RegexCultureTests.cs | 1 - src/mono/browser/runtime/corebindings.c | 2 - .../browser/runtime/globalization-stubs.ts | 8 --- src/mono/browser/runtime/globalization.ts | 3 +- .../hybrid-globalization/change-case.ts | 54 ------------------- .../hybrid-globalization/module-exports.ts | 3 +- src/mono/browser/runtime/types/internal.ts | 1 - 12 files changed, 47 insertions(+), 79 deletions(-) diff --git a/src/libraries/Common/src/Interop/Browser/Interop.TextInfo.cs b/src/libraries/Common/src/Interop/Browser/Interop.TextInfo.cs index a10d2897f75be..c9a0b3ac39fb4 100644 --- a/src/libraries/Common/src/Interop/Browser/Interop.TextInfo.cs +++ b/src/libraries/Common/src/Interop/Browser/Interop.TextInfo.cs @@ -7,8 +7,6 @@ internal static partial class Interop { internal static unsafe partial class JsGlobalization { - [MethodImplAttribute(MethodImplOptions.InternalCall)] - internal static extern unsafe nint ChangeCaseInvariant(char* src, int srcLen, char* dstBuffer, int dstBufferCapacity, bool bToUpper); [MethodImplAttribute(MethodImplOptions.InternalCall)] internal static extern unsafe nint ChangeCase(char* culture, int cultureLen, char* src, int srcLen, char* dstBuffer, int dstBufferCapacity, bool bToUpper); } diff --git a/src/libraries/Common/tests/Tests/System/StringTests.cs b/src/libraries/Common/tests/Tests/System/StringTests.cs index 8e465475a5200..709d7c9847403 100644 --- a/src/libraries/Common/tests/Tests/System/StringTests.cs +++ b/src/libraries/Common/tests/Tests/System/StringTests.cs @@ -1287,7 +1287,6 @@ public static void ContainsMatchDifferentSpans_StringComparison() [Fact] [ActiveIssue("https://github.com/dotnet/runtime/issues/95338", typeof(PlatformDetection), nameof(PlatformDetection.IsHybridGlobalizationOnApplePlatform))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/95471", typeof(PlatformDetection), nameof(PlatformDetection.IsHybridGlobalizationOnBrowser))] public static void ContainsNoMatch_StringComparison() { for (int length = 1; length < 150; length++) @@ -6041,7 +6040,6 @@ public static IEnumerable ToUpper_TurkishI_InvariantCulture_MemberData [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotInvariantGlobalization), nameof(PlatformDetection.IsNotHybridGlobalizationOnApplePlatform))] [MemberData(nameof(ToUpper_TurkishI_InvariantCulture_MemberData))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/95471", typeof(PlatformDetection), nameof(PlatformDetection.IsHybridGlobalizationOnBrowser))] public static void ToUpper_TurkishI_InvariantCulture(string s, string expected) { using (new ThreadCultureChange(CultureInfo.InvariantCulture)) diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/TextInfo.WebAssembly.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/TextInfo.WebAssembly.cs index 05e8cc3e1d049..459478f705504 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/TextInfo.WebAssembly.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/TextInfo.WebAssembly.cs @@ -14,12 +14,25 @@ internal unsafe void JsChangeCase(char* src, int srcLen, char* dstBuffer, int ds Debug.Assert(!GlobalizationMode.UseNls); Debug.Assert(GlobalizationMode.Hybrid); + if (HasEmptyCultureName) + { + ReadOnlySpan source = new ReadOnlySpan(src, srcLen); + Span destination = new Span(dstBuffer, dstBufferCapacity); + if (toUpper) + { + InvariantModeCasing.ToUpper(source, destination); + } + else + { + InvariantModeCasing.ToLower(source, destination); + } + return; + } + ReadOnlySpan cultureName = _cultureName.AsSpan(); fixed (char* pCultureName = &MemoryMarshal.GetReference(cultureName)) { - nint exceptionPtr = HasEmptyCultureName ? - Interop.JsGlobalization.ChangeCaseInvariant(src, srcLen, dstBuffer, dstBufferCapacity, toUpper) : - Interop.JsGlobalization.ChangeCase(pCultureName, cultureName.Length, src, srcLen, dstBuffer, dstBufferCapacity, toUpper); + nint exceptionPtr = Interop.JsGlobalization.ChangeCase(pCultureName, cultureName.Length, src, srcLen, dstBuffer, dstBufferCapacity, toUpper); Helper.MarshalAndThrowIfException(exceptionPtr); } } diff --git a/src/libraries/System.Private.CoreLib/src/System/Globalization/TextInfo.cs b/src/libraries/System.Private.CoreLib/src/System/Globalization/TextInfo.cs index a035c875f7290..45123f85d4341 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Globalization/TextInfo.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Globalization/TextInfo.cs @@ -140,7 +140,12 @@ public string ListSeparator /// public char ToLower(char c) { +#if TARGET_BROWSER + // for invariant culture _cultureName is empty - HybridGlobalization does not have to call JS + if (GlobalizationMode.Invariant || (GlobalizationMode.Hybrid && HasEmptyCultureName)) +#else if (GlobalizationMode.Invariant) +#endif { return InvariantModeCasing.ToLower(c); } @@ -173,7 +178,12 @@ public string ToLower(string str) { ArgumentNullException.ThrowIfNull(str); +#if TARGET_BROWSER + // for invariant culture _cultureName is empty - HybridGlobalization does not have to call JS + if (GlobalizationMode.Invariant || (GlobalizationMode.Hybrid && HasEmptyCultureName)) +#else if (GlobalizationMode.Invariant) +#endif { return InvariantModeCasing.ToLower(str); } @@ -184,7 +194,9 @@ public string ToLower(string str) private unsafe char ChangeCase(char c, bool toUpper) { Debug.Assert(!GlobalizationMode.Invariant); - +#if TARGET_BROWSER + Debug.Assert(!(GlobalizationMode.Hybrid && HasEmptyCultureName)); +#endif char dst = default; ChangeCaseCore(&c, 1, &dst, 1, toUpper); return dst; @@ -227,6 +239,9 @@ private unsafe void ChangeCaseCommon(ReadOnlySpan source, Spa { Debug.Assert(!GlobalizationMode.Invariant); Debug.Assert(typeof(TConversion) == typeof(ToUpperConversion) || typeof(TConversion) == typeof(ToLowerConversion)); +#if TARGET_BROWSER + Debug.Assert(!(GlobalizationMode.Hybrid && HasEmptyCultureName)); +#endif if (source.IsEmpty) { @@ -263,6 +278,9 @@ private unsafe string ChangeCaseCommon(string source) where TConver Debug.Assert(!GlobalizationMode.Invariant); Debug.Assert(source != null); +#if TARGET_BROWSER + Debug.Assert(!(GlobalizationMode.Hybrid && HasEmptyCultureName)); +#endif // If the string is empty, we're done. if (source.Length == 0) @@ -412,7 +430,12 @@ private static char ToLowerAsciiInvariant(char c) /// public char ToUpper(char c) { +#if TARGET_BROWSER + // for invariant culture _cultureName is empty - HybridGlobalization does not have to call JS + if (GlobalizationMode.Invariant || (GlobalizationMode.Hybrid && HasEmptyCultureName)) +#else if (GlobalizationMode.Invariant) +#endif { return InvariantModeCasing.ToUpper(c); } @@ -445,7 +468,12 @@ public string ToUpper(string str) { ArgumentNullException.ThrowIfNull(str); +#if TARGET_BROWSER + // for invariant culture _cultureName is empty - HybridGlobalization does not have to call JS + if (GlobalizationMode.Invariant || (GlobalizationMode.Hybrid && HasEmptyCultureName)) +#else if (GlobalizationMode.Invariant) +#endif { return InvariantModeCasing.ToUpper(str); } diff --git a/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/StringTests.cs b/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/StringTests.cs index 8f8643be5b550..a6e16e2fce3ca 100644 --- a/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/StringTests.cs +++ b/src/libraries/System.Runtime/tests/System.Runtime.Tests/System/StringTests.cs @@ -872,7 +872,6 @@ public static IEnumerable Replace_StringComparisonCulture_TestData() [Theory] [MemberData(nameof(Replace_StringComparisonCulture_TestData))] - [ActiveIssue("https://github.com/dotnet/runtime/issues/95471", typeof(PlatformDetection), nameof(PlatformDetection.IsHybridGlobalizationOnBrowser))] [ActiveIssue("https://github.com/dotnet/runtime/issues/95503", typeof(PlatformDetection), nameof(PlatformDetection.IsHybridGlobalizationOnBrowser))] public void Replace_StringComparisonCulture_ReturnsExpected(string original, string oldValue, string newValue, bool ignoreCase, CultureInfo culture, string expected) { diff --git a/src/libraries/System.Text.RegularExpressions/tests/FunctionalTests/RegexCultureTests.cs b/src/libraries/System.Text.RegularExpressions/tests/FunctionalTests/RegexCultureTests.cs index eb40521b07b5e..bfc4f4dc1cdaa 100644 --- a/src/libraries/System.Text.RegularExpressions/tests/FunctionalTests/RegexCultureTests.cs +++ b/src/libraries/System.Text.RegularExpressions/tests/FunctionalTests/RegexCultureTests.cs @@ -161,7 +161,6 @@ Regex[] Create(string input, CultureInfo info, RegexOptions additional) [SkipOnTargetFramework(TargetFrameworkMonikers.NetFramework, "Doesn't support NonBacktracking")] [Fact] - [ActiveIssue("https://github.com/dotnet/runtime/issues/95471", typeof(PlatformDetection), nameof(PlatformDetection.IsHybridGlobalizationOnBrowser))] [ActiveIssue("https://github.com/dotnet/runtime/issues/60568", TestPlatforms.Android | TestPlatforms.LinuxBionic)] public async Task TurkishI_Is_Differently_LowerUpperCased_In_Turkish_Culture_NonBacktracking() { diff --git a/src/mono/browser/runtime/corebindings.c b/src/mono/browser/runtime/corebindings.c index 1937eb114dd6d..15a185133ce36 100644 --- a/src/mono/browser/runtime/corebindings.c +++ b/src/mono/browser/runtime/corebindings.c @@ -59,7 +59,6 @@ extern void mono_wasm_invoke_jsimport_ST (int function_handle, void *args); #endif /* DISABLE_THREADS */ // HybridGlobalization -extern char16_t* mono_wasm_change_case_invariant (const uint16_t* src, int32_t srcLength, uint16_t* dst, int32_t dstLength, mono_bool bToUpper); extern char16_t* mono_wasm_change_case (const uint16_t* culture, int32_t cultureLength, const uint16_t* src, int32_t srcLength, uint16_t* dst, int32_t dstLength, mono_bool bToUpper); extern char16_t* mono_wasm_compare_string (const uint16_t* culture, int32_t cultureLength, const uint16_t* str1, int32_t str1Length, const uint16_t* str2, int32_t str2Length, int32_t options, int *resultPtr); extern char16_t* mono_wasm_starts_with (const uint16_t* culture, int32_t cultureLength, const uint16_t* str1, int32_t str1Length, const uint16_t* str2, int32_t str2Length, int32_t options, mono_bool *resultPtr); @@ -104,7 +103,6 @@ void bindings_initialize_internals (void) mono_add_internal_call ("System.ConsolePal::Clear", mono_wasm_console_clear); // HybridGlobalization - mono_add_internal_call ("Interop/JsGlobalization::ChangeCaseInvariant", mono_wasm_change_case_invariant); mono_add_internal_call ("Interop/JsGlobalization::ChangeCase", mono_wasm_change_case); mono_add_internal_call ("Interop/JsGlobalization::CompareString", mono_wasm_compare_string); mono_add_internal_call ("Interop/JsGlobalization::StartsWith", mono_wasm_starts_with); diff --git a/src/mono/browser/runtime/globalization-stubs.ts b/src/mono/browser/runtime/globalization-stubs.ts index 5544c3123d2b3..a1a42dc1cd414 100644 --- a/src/mono/browser/runtime/globalization-stubs.ts +++ b/src/mono/browser/runtime/globalization-stubs.ts @@ -5,14 +5,6 @@ import { globalizationHelpers } from "./globals"; import { Int32Ptr, VoidPtr } from "./types/emscripten"; import { VoidPtrNull } from "./types/internal"; - -export function mono_wasm_change_case_invariant (src: number, srcLength: number, dst: number, dstLength: number, toUpper: number) : VoidPtr { - if (typeof globalizationHelpers.mono_wasm_change_case_invariant === "function") { - return globalizationHelpers.mono_wasm_change_case_invariant(src, srcLength, dst, dstLength, toUpper); - } - return VoidPtrNull; -} - export function mono_wasm_change_case (culture: number, cultureLength: number, src: number, srcLength: number, dst: number, dstLength: number, toUpper: number) : VoidPtr { if (typeof globalizationHelpers.mono_wasm_change_case === "function") { return globalizationHelpers.mono_wasm_change_case(culture, cultureLength, src, srcLength, dst, dstLength, toUpper); diff --git a/src/mono/browser/runtime/globalization.ts b/src/mono/browser/runtime/globalization.ts index 95fac11927335..fe1b95be3ba73 100644 --- a/src/mono/browser/runtime/globalization.ts +++ b/src/mono/browser/runtime/globalization.ts @@ -1,11 +1,10 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -import { mono_wasm_change_case, mono_wasm_change_case_invariant, mono_wasm_compare_string, mono_wasm_ends_with, mono_wasm_get_calendar_info, mono_wasm_get_culture_info, mono_wasm_get_first_day_of_week, mono_wasm_get_first_week_of_year, mono_wasm_index_of, mono_wasm_starts_with } from "./globalization-stubs"; +import { mono_wasm_change_case, mono_wasm_compare_string, mono_wasm_ends_with, mono_wasm_get_calendar_info, mono_wasm_get_culture_info, mono_wasm_get_first_day_of_week, mono_wasm_get_first_week_of_year, mono_wasm_index_of, mono_wasm_starts_with } from "./globalization-stubs"; import { mono_wasm_get_locale_info } from "./locales-common"; export const mono_wasm_hybrid_globalization_imports = [ - mono_wasm_change_case_invariant, mono_wasm_change_case, mono_wasm_compare_string, mono_wasm_starts_with, diff --git a/src/mono/browser/runtime/hybrid-globalization/change-case.ts b/src/mono/browser/runtime/hybrid-globalization/change-case.ts index c5b1a425ac0fd..0024b5b45d5d1 100644 --- a/src/mono/browser/runtime/hybrid-globalization/change-case.ts +++ b/src/mono/browser/runtime/hybrid-globalization/change-case.ts @@ -6,60 +6,6 @@ import { runtimeHelpers } from "./module-exports"; import { VoidPtr } from "../types/emscripten"; import { isSurrogate } from "./helpers"; -export function mono_wasm_change_case_invariant (src: number, srcLength: number, dst: number, dstLength: number, toUpper: number): VoidPtr { - try { - const input = runtimeHelpers.utf16ToStringLoop(src, src + 2 * srcLength); - const result = toUpper ? input.toUpperCase() : input.toLowerCase(); - // Unicode defines some codepoints which expand into multiple codepoints, - // originally we do not support this expansion - if (result.length <= dstLength) { - runtimeHelpers.stringToUTF16(dst, dst + 2 * dstLength, result); - return VoidPtrNull; - } - - // workaround to maintain the ICU-like behavior - const heapI16 = runtimeHelpers.localHeapViewU16(); - let jump = 1; - if (toUpper) { - for (let i = 0; i < input.length; i += jump) { - // surrogate parts have to enter ToUpper/ToLower together to give correct output - if (isSurrogate(input, i)) { - jump = 2; - const surrogate = input.substring(i, i + 2); - const upperSurrogate = surrogate.toUpperCase(); - const appendedSurrogate = upperSurrogate.length > 2 ? surrogate : upperSurrogate; - appendSurrogateToMemory(heapI16, dst, appendedSurrogate, i); - - } else { - jump = 1; - const upperChar = input[i].toUpperCase(); - const appendedChar = upperChar.length > 1 ? input[i] : upperChar; - runtimeHelpers.setU16_local(heapI16, dst + i * 2, appendedChar.charCodeAt(0)); - } - } - } else { - for (let i = 0; i < input.length; i += jump) { - if (isSurrogate(input, i)) { - jump = 2; - const surrogate = input.substring(i, i + 2); - const upperSurrogate = surrogate.toLowerCase(); - const appendedSurrogate = upperSurrogate.length > 2 ? surrogate : upperSurrogate; - appendSurrogateToMemory(heapI16, dst, appendedSurrogate, i); - - } else { - jump = 1; - const upperChar = input[i].toLowerCase(); - const appendedChar = upperChar.length > 1 ? input[i] : upperChar; - runtimeHelpers.setU16_local(heapI16, dst + i * 2, appendedChar.charCodeAt(0)); - } - } - } - return VoidPtrNull; - } catch (ex: any) { - return runtimeHelpers.stringToUTF16Ptr(ex.toString()); - } -} - export function mono_wasm_change_case (culture: number, cultureLength: number, src: number, srcLength: number, dst: number, dstLength: number, toUpper: number): VoidPtr { try { const cultureName = runtimeHelpers.utf16ToString(culture, (culture + 2 * cultureLength)); diff --git a/src/mono/browser/runtime/hybrid-globalization/module-exports.ts b/src/mono/browser/runtime/hybrid-globalization/module-exports.ts index dc5b3aebf8fd4..f17bf90d4f245 100644 --- a/src/mono/browser/runtime/hybrid-globalization/module-exports.ts +++ b/src/mono/browser/runtime/hybrid-globalization/module-exports.ts @@ -1,6 +1,6 @@ import { GlobalizationHelpers, RuntimeHelpers } from "../types/internal"; import { mono_wasm_get_calendar_info } from "./calendar"; -import { mono_wasm_change_case, mono_wasm_change_case_invariant } from "./change-case"; +import { mono_wasm_change_case } from "./change-case"; import { mono_wasm_compare_string, mono_wasm_starts_with, mono_wasm_ends_with, mono_wasm_index_of } from "./collations"; import { mono_wasm_get_culture_info } from "./culture-info"; import { setSegmentationRulesFromJson } from "./grapheme-segmenter"; @@ -10,7 +10,6 @@ export let globalizationHelpers: GlobalizationHelpers; export let runtimeHelpers: RuntimeHelpers; export function initHybrid (gh: GlobalizationHelpers, rh: RuntimeHelpers) { - gh.mono_wasm_change_case_invariant = mono_wasm_change_case_invariant; gh.mono_wasm_change_case = mono_wasm_change_case; gh.mono_wasm_compare_string = mono_wasm_compare_string; gh.mono_wasm_starts_with = mono_wasm_starts_with; diff --git a/src/mono/browser/runtime/types/internal.ts b/src/mono/browser/runtime/types/internal.ts index 26f66bcf2d1f8..e21c7ea79fd43 100644 --- a/src/mono/browser/runtime/types/internal.ts +++ b/src/mono/browser/runtime/types/internal.ts @@ -254,7 +254,6 @@ export type RuntimeHelpers = { } export type GlobalizationHelpers = { - mono_wasm_change_case_invariant: (src: number, srcLength: number, dst: number, dstLength: number, toUpper: number) => VoidPtr; mono_wasm_change_case: (culture: number, cultureLength: number, src: number, srcLength: number, dst: number, dstLength: number, toUpper: number) => VoidPtr; mono_wasm_compare_string: (culture: number, cultureLength: number, str1: number, str1Length: number, str2: number, str2Length: number, options: number, resultPtr: Int32Ptr) => VoidPtr; mono_wasm_starts_with: (culture: number, cultureLength: number, str1: number, str1Length: number, str2: number, str2Length: number, options: number, resultPtr: Int32Ptr) => VoidPtr;