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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 27 commits into from
Mar 10, 2021
Merged

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

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
164db20
[wasm] Throw exception if culture data does not exist in icu
tqiu8 Jan 9, 2021
fd96d0c
add lines
tqiu8 Jan 9, 2021
68a4fa3
add lines
tqiu8 Jan 9, 2021
82759a8
remove blank spaces
tqiu8 Jan 9, 2021
d92afab
Add check for culture data in ICU
tqiu8 Jan 22, 2021
df4d910
include documentation link in exception message
tqiu8 Jan 22, 2021
7167047
Add PredefinedOnly to Windows
tqiu8 Jan 22, 2021
7c677dc
move checks outside of CultureData.cs
tqiu8 Jan 22, 2021
53151f8
Add test for predefined culture env var
tqiu8 Jan 25, 2021
42c85fb
switch test to remoteexecutor
tqiu8 Jan 25, 2021
e0f14e8
try catch for all culture not supported exceptions
tqiu8 Jan 27, 2021
bb3f356
Fix test failures: Check for RegionInfo first before creating culture…
tqiu8 Feb 8, 2021
e894382
add another condition to env var test
tqiu8 Feb 8, 2021
462f7ad
fix conditional theory
tqiu8 Feb 9, 2021
18faed1
fix TypeConverter test failures
tqiu8 Feb 10, 2021
ef75159
change assembly test data to pl-PL to avoid culturenotfound exception
tqiu8 Feb 16, 2021
8cdeb96
Merge branch 'culture-dt-fix' of github.com:tqiu8/runtime into cultur…
tqiu8 Mar 2, 2021
84b8429
remove exception filters from tests and add additional parameter to t…
tqiu8 Mar 2, 2021
f8163f7
move exception out of CreateCulturedata
tqiu8 Mar 4, 2021
451a3ad
remove else block
tqiu8 Mar 4, 2021
0bc3a5c
remove redundant methods
tqiu8 Mar 4, 2021
c40d610
revert logic in nls
tqiu8 Mar 4, 2021
eeeda5f
Update src/libraries/System.Reflection.MetadataLoadContext/tests/src/…
tqiu8 Mar 5, 2021
b956ca6
Merge branch 'master' into culture-dt-fix
tqiu8 Mar 5, 2021
3b065d2
Merge branch 'culture-dt-fix' of github.com:tqiu8/runtime into cultur…
tqiu8 Mar 5, 2021
0aab18e
fix invariant tests
tqiu8 Mar 9, 2021
23842ee
remove aggressive inlining
tqiu8 Mar 10, 2021
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
10 changes: 9 additions & 1 deletion src/libraries/Common/tests/Tests/System/StringTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2567,8 +2567,16 @@ public void Equals_Encyclopaedia_ReturnsExpected(StringComparison comparison, bo
{
string source = "encyclop\u00e6dia";
string target = "encyclopaedia";
ThreadCultureChange culture;
if (PlatformDetection.IsBrowser)
{
culture = new ThreadCultureChange("pl-PL");
} else
{
culture = new ThreadCultureChange("se-SE");
}

using (new ThreadCultureChange("se-SE"))
tqiu8 marked this conversation as resolved.
Show resolved Hide resolved
using (culture)
{
Assert.Equal(expected, string.Equals(source, target, comparison));
Assert.Equal(expected, source.AsSpan().Equals(target.AsSpan(), comparison));
Expand Down
8 changes: 8 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System/Globalization/CultureData.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -803,6 +803,14 @@ private static string NormalizeCultureName(string name, out bool isNeutralName)
return CultureData.Invariant;
}

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

CultureData culture = new CultureData();
culture._sRealName = cultureName;
culture._bUseOverridesUserSetting = useUserOverride;
Expand Down
2 changes: 2 additions & 0 deletions src/libraries/System.Private.CoreLib/src/System/Globalization/GlobalizationMode.Unix.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ internal static partial class GlobalizationMode

internal static bool UseNls => false;

internal static bool PredefinedOnly { get; } = GetSwitchValue("System.Globalization.PredefinedOnly", "DOTNET_SYSTEM_GLOBALIZATION_PREDEFINED_ONLY");
Copy link
Member

@tarekgh tarekgh Jan 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is for Browser only.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@tarekgh tarekgh Jan 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok yes that makes sense.


private static bool GetGlobalizationInvariantMode()
tqiu8 marked this conversation as resolved.
Show resolved Hide resolved
tqiu8 marked this conversation as resolved.
Show resolved Hide resolved
{
bool invariantEnabled = GetInvariantSwitchValue();
Expand Down
22 changes: 19 additions & 3 deletions src/libraries/System.Runtime/tests/System/DateTimeTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1551,7 +1551,15 @@ public static void TryParseExact_EmptyAMPMDesignator()
[Fact]
public static void ParseExact_EscapedSingleQuotes()
{
var formatInfo = DateTimeFormatInfo.GetInstance(new CultureInfo("mt-MT"));
DateTimeFormatInfo formatInfo;
if (PlatformDetection.IsBrowser)
{
formatInfo = DateTimeFormatInfo.GetInstance(new CultureInfo("id-ID"));
}
else
{
formatInfo = DateTimeFormatInfo.GetInstance(new CultureInfo("mt-MT"));
}
const string format = @"dddd, d' ta\' 'MMMM yyyy";

DateTime expected = new DateTime(1999, 2, 28, 17, 00, 01);
Expand Down Expand Up @@ -1735,8 +1743,16 @@ public static IEnumerable<object[]> Parse_ValidInput_Succeeds_MemberData()
hebrewCulture.DateTimeFormat.Calendar = new HebrewCalendar();
yield return new object[] { today.ToString(hebrewCulture), hebrewCulture, today };

var mongolianCulture = new CultureInfo("mn-MN");
yield return new object[] { today.ToString(mongolianCulture), mongolianCulture, today };
CultureInfo culture;
if (PlatformDetection.IsBrowser)
{
culture = new CultureInfo("pl-PL");
}
else
{
culture = new CultureInfo("mn-MN");
}
yield return new object[] { today.ToString(culture), culture, today };
}
}

Expand Down
3 changes: 3 additions & 0 deletions src/mono/wasm/runtime/library_mono.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1904,6 +1904,9 @@ var MonoSupportLib = {

if (invariantMode)
this.mono_wasm_setenv ("DOTNET_SYSTEM_GLOBALIZATION_INVARIANT", "1");

// Set globalization mode to PredefinedOnly
this.mono_wasm_setenv ("DOTNET_SYSTEM_GLOBALIZATION_PREDEFINED_ONLY", "1");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@tarekgh tarekgh Jan 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

@safern safern Jan 29, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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()

},

// Used by the debugger to enumerate loaded dlls and pdbs
Expand Down