[wasm] Throw exception if culture data does not exist in icu

[tqiu8]

Fix #47068
Fix #46327

Throw exception if culture data doesn't exist. Currently defaults to en-us.

[ghost]

Tagging subscribers to this area: @tarekgh, @safern, @krwq
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Fix #47068
Fix #46327

Throw exception if culture data doesn't exist. Currently defaults to en-us.

Author:	tqiu8
Assignees:	-
Labels:	area-System.Globalization
Milestone:	-

[tarekgh]

PredefinedOnly [](start = 29, length = 14)

are you adding this for Linux only? looks the calling site will call it even running on Windows. maybe you need to move this to the file GlobalizationMode.cs?

[tqiu8]

This is for Browser only.

[tarekgh]

Why we need to restrict this to the browser only? We may enable this flag globally if we need to. Note the calling site of this property is called from shared code which runs with ICU and Nls. Give me a few minutes and I can send you what I propose.

Also, does the browser always runs with Icu?

[tqiu8]

I guess I just assumed only Browser would need this use case, but I can definitely move to GlobalizationMode.cs. And yes, the browser always runs with icu.

[tarekgh]

I would suggest to move the checks

runtime/src/libraries/System.Private.CoreLib/src/System/Globalization/CultureInfo.Nls.cs

Line 14 in f1ad1b9

if (CultureData.GetLocaleInfoExInt(name, Interop.Kernel32.LOCALE_ICONSTRUCTEDLOCALE) == 1)

and

runtime/src/libraries/System.Private.CoreLib/src/System/Globalization/CultureInfo.Icu.cs

Line 14 in f1ad1b9

if (!Interop.Globalization.IsPredefinedLocale(name))

to its own methods, something like

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static IcuIsEnsurePredefinedLocaleName(string name) // may put this in CultureData.Icu.cs
{
    Debug.Assert(!GlobalizationMode.UseNls);

    if (!Interop.Globalization.IsPredefinedLocale(name))
    {
        throw new CultureNotFoundException(nameof(name), SR.Format(SR.Argument_InvalidPredefinedCultureName, name));
    }
}

[MethodImpl(MethodImplOptions.AggressiveInlining)]
internal static NlsIsEnsurePredefinedLocaleName(string name) // may put this in CultureData.Nls.cs
{
       Debug.Assert(GlobalizationMode.UseNls);

       if (CultureData.GetLocaleInfoExInt(name, Interop.Kernel32.LOCALE_ICONSTRUCTEDLOCALE) == 1)
       {
           throw new CultureNotFoundException(nameof(name), SR.Format(SR.Argument_InvalidPredefinedCultureName, name));
       }
}

Then replace the code here with

 if (GlobalizationMode.PredefinedOnly)
{
    if (GlobalizationMode.UseNls)
    {  
        NlsIsEnsurePredefinedLocaleName(cultureName);
    }
    else
    {  
        IcuIsEnsurePredefinedLocaleName(cultureName);
    }
}

Last, replace the checks in the links I mentioned above with the new methods calls. and move the code in GlobalizationMode.Unix.cs to GlobalizationMode.cs.

[tqiu8]

Ok yes that makes sense.

[marek-safar]

Why is it env variable if it's always set?

[tqiu8]

Just leaving the option open if we don't want it always set.

[marek-safar]

This won't be easy to change and it'll cost us on size. I think we should just compile the setting in

[tqiu8]

We want it always set for wasm but I'm not sure if other OS want this option by default. @tarekgh

[tarekgh]

outside WASM, we shouldn't turn this option on by default. Whoever need it would need to opt-in to get it.

[safern]

We could do this by setting a default value for the flag and making that default value be true for wasm based on an OS check when you are setting the PredefinedCulturesOnly flag. Or, just if def the code for WASM to always be ON.

[safern]

Here is a place where we do something similar for WASM:

runtime/src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs

Line 26 in e6105a1

if (loaded == 0 && !OperatingSystem.IsBrowser())

So we could do something like:

GetSwitchValue("System.Globalization.PredefinedCulturesOnly", "DOTNET_SYSTEM_GLOBALIZATION_PREDEFINED_CULTURES_ONLY") || OperatingSystem.IsBrowser()

[tarekgh]

CC @safern

[tarekgh]

Could you please replace the checks in

runtime/src/libraries/System.Private.CoreLib/src/System/Globalization/CultureInfo.Nls.cs

Line 14 in f1ad1b9

if (CultureData.GetLocaleInfoExInt(name, Interop.Kernel32.LOCALE_ICONSTRUCTEDLOCALE) == 1)

and

runtime/src/libraries/System.Private.CoreLib/src/System/Globalization/CultureInfo.Icu.cs

Line 14 in f1ad1b9

if (!Interop.Globalization.IsPredefinedLocale(name))

with the new introduced method calls?

thank you!

[tarekgh]

One last ask, could you please add a new test (not specific to the browser) to test the new environment variable? here is some test example which doing similar idea (testing setting environment variable).

runtime/src/libraries/System.Threading.Tasks.Extensions/tests/AsyncValueTaskMethodBuilderTests.cs

Line 568 in e63969d

var psi = new ProcessStartInfo();

[tqiu8]

You need to do this test with RemoteExecutor as I pointed before in my comment #47301 (comment). Otherwise this test can easily fail and also will cause other problems for other tests running after it.

I don't think RemoteExcutor or System.Diagnostics.Process are supported on wasm right now. I'm going to store the current env var and reset it back to that after the test?

[tarekgh]

I don't think RemoteExcutor or System.Diagnostics.Process are supported on wasm right now. I'm going to store the current env var and reset it back to that after the test?

Yes, just exclude the added test from running on WASM. just mark the test with the attribute [ConditionalFact(typeof(RemoteExecutor), nameof(RemoteExecutor.IsSupported))]

[tarekgh]

@tqiu8 I have added one more comment I hope you address it.
Also, please track documenting the new environment variable too. Thanks!

[tqiu8]

@tqiu8 I have added one more comment I hope you address it.
Also, please track documenting the new environment variable too. Thanks!

Thank you so much for your feedback!

[lewing]

This is looking good but the failures look like they might be related.