[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.
dotnet · Jun 26, 2024 · 5c53e8e · 5c53e8e
1 parent b2f0b7a
commit 5c53e8e
Show file tree

Hide file tree

Showing 12 changed files with 47 additions and 79 deletions.
diff --git 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
@@ -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<object[]> 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
@@ -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<char> source = new ReadOnlySpan<char>(src, srcLen);
+                Span<char> destination = new Span<char>(dstBuffer, dstBufferCapacity);
+                if (toUpper)
+                {
+                    InvariantModeCasing.ToUpper(source, destination);
+                }
+                else
+                {
+                    InvariantModeCasing.ToLower(source, destination);
+                }
+                return;
+            }
+
             ReadOnlySpan<char> 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
@@ -140,7 +140,12 @@ public string ListSeparator
         /// </summary>
         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<TConversion>(ReadOnlySpan<char> 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<TConversion>(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)
         /// </summary>
         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
@@ -872,7 +872,6 @@ public static IEnumerable<object[]> 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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(<any>culture, <any>(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
@@ -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
@@ -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;