Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[browser] Fix: HybridGlobalization Invariant function differs from non-Hybrid #103037

Merged
merged 7 commits into from
Jun 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions src/libraries/Common/src/Interop/Browser/Interop.TextInfo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
2 changes: 0 additions & 2 deletions src/libraries/Common/tests/Tests/System/StringTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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++)
Expand Down Expand Up @@ -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))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}
Expand All @@ -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;
Expand Down Expand Up @@ -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)
{
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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()
{
Expand Down
2 changes: 0 additions & 2 deletions src/mono/browser/runtime/corebindings.c
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Expand Down
8 changes: 0 additions & 8 deletions src/mono/browser/runtime/globalization-stubs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
3 changes: 1 addition & 2 deletions src/mono/browser/runtime/globalization.ts
Original file line number Diff line number Diff line change
@@ -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,
Expand Down
54 changes: 0 additions & 54 deletions src/mono/browser/runtime/hybrid-globalization/change-case.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
Original file line number Diff line number Diff line change
@@ -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";
Expand All @@ -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;
Expand Down
1 change: 0 additions & 1 deletion src/mono/browser/runtime/types/internal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Loading