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

[iOS] Set CurrentCulture locale to specific format based on system default when possible #111032

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

matouskozak
Copy link
Member

@matouskozak matouskozak commented Jan 2, 2025

Problem Description

On iOS (and other apple mobile platforms), we are taking the system default culture and using it inside .NET as CultureInfo.CurrentCulture.

Currently, the system default culture is truncated into neutral culture format (only language). This behavior was set in #104071, where we had an incorrect assumption that other ICU platforms also return just neutral locale name. However, that isn't true because the ICU uloc_getBaseName

uloc_getBaseName(defaultLocale, localeNameBuffer, ULOC_FULLNAME_CAPACITY, &status);
only strips collation information as described in https://github.com/unicode-org/icu/blob/d9d09db2a7167eea2b5749836ef32cdd45303fdd/icu4c/source/common/unicode/uloc.h#L948.

The problem of using all Apple's system default locales is that some of the retrieved identifiers are not part of https://github.com/dotnet/runtime/blob/main/src/libraries/System.Private.CoreLib/src/System/Globalization/IcuLocaleData.cs (e.g., en-CZ), causing the culture LCID number to be 0x1000 (LOCALE_CUSTOM_UNSPECIFIED). This can cause issues where the default CultureInfo.CurrentCulture causes crashes when using certain CultureInfo APIs that accept LCID instead of culture name, such as,

public static CultureInfo GetCultureInfo(int culture)

used by e.g. SqlString.

Proposed Change

This PR reverts the previous change and preserves the full (specific = language-region) culture locale format when the retrieved locale is part of IcuLocaleData.cs. Upon retrieving the system locale name, we verify that the culture name is part of the ICU data and if not, we use the parent culture (neutral) instead.

A test was added to CurrentCultureTests to assert that the default CultureInfo.CurrentCulture is never a custom type.

Previous behavior (.net 9):

Apple system culture CurrentCulture
en-US en
en-CZ en

New behavior:

Apple system culture CurrentCulture
en-US en-US
en-CZ en

This PR addresses the following issues:

Copy link
Contributor

Tagging subscribers to this area: @dotnet/area-system-globalization
See info in area-owners.md if you want to be subscribed.

@matouskozak matouskozak requested a review from Copilot January 2, 2025 12:10

Choose a reason for hiding this comment

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

Copilot reviewed 1 out of 2 changed files in this pull request and generated no comments.

Files not reviewed (1)
  • src/native/libs/System.Globalization.Native/pal_locale.m: Language not supported
@matouskozak
Copy link
Member Author

/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Comment on lines -789 to -795
static NSString* GetBaseName(NSString *localeIdentifier)
{
NSLocale *locale = [[NSLocale alloc] initWithLocaleIdentifier:localeIdentifier];
NSString *languageCode = [locale objectForKey:NSLocaleLanguageCode];
return languageCode;
}

Copy link
Member

Choose a reason for hiding this comment

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

Do we know what this was done before?
The PR description should be updated to describe this - what we did before (has that) and why, and what we're changing it to (and why it's OK to change given the previous reasoning).

Copy link
Member Author

@matouskozak matouskozak Jan 3, 2025

Choose a reason for hiding this comment

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

Indeed, I was trying this PR to see which test suit breaks if we switch to specific name because there are other dependencies such as SqlStrings that were previously affected. I'll update the issue description when I finalize the code changes.

Brief history what I found:


I'll try to make a middle way out of this, where we use system locale if it's recognized by .NET, otherwise we use parent culture which is neutral.

@matouskozak
Copy link
Member Author

/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@matouskozak matouskozak changed the title [iOS] Fix CurrentCulture locale to be in specific format [iOS] Set CurrentCulture locale to specific format based on system default when possible Jan 6, 2025
@matouskozak
Copy link
Member Author

/azp run runtime-ioslike,runtime-ioslikesimulator,runtime-maccatalyst

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@matouskozak matouskozak marked this pull request as ready for review January 6, 2025 11:05
@tarekgh
Copy link
Member

tarekgh commented Jan 6, 2025

I don't think we need to fix this. We encourage users to move away from LCIDs. Any users still using LCID should start migrating to use culture names instead. The change here is trying to work around the issue of using the parent which may benefit using the LCID but it can cause other issues when requesting any region-specific data from current culture.

This issue is not specific to iOS or mobile platforms. This can occur on Windows for example doing CultureInfo.CurrentCulture = new CultureInfo("en-AE");. Whoever is using LCID needs to fix their code and start using culture names or they need to make their code tolerant when getting custom LCID values.

Last, if IcuLocaleData.cs missing any data, we can try to update it as needed.

@matouskozak
Copy link
Member Author

matouskozak commented Jan 6, 2025

Any users still using LCID should start migrating to use culture names instead.

@tarekgh I know that using LCID is not recommended. What I was trying to work around here by using the parent is that any default culture set by .NET would be compatible with all .NET libraries code. We are currently relying on LCID inside SqlString codebase, causing failures if LCID is custom culture.

Note, this PR not only does the fallback to parent culture but also fixes the behavior that we didn't return specific system culture. I can remove the parent culture workaround and disable the failing tests until the affected library code migrates from LCID. However, because we need to backport this fix to .NET 9, we could be risking a breaking change to some customers using e.g., SqlStrings.

@tarekgh
Copy link
Member

tarekgh commented Jan 6, 2025

What I was trying to work around here by using the parent is that any default culture set by .NET would be compatible with all .NET libraries code. We are currently relying on LCID inside SqlString codebase, causing failures if LCID is custom culture.

I think the right thing to do is have SqlString get fixed. Either they migrate to use the culture name or to be tolerant to do the fallback themselves. As I mentioned, this issue is not specific to iOS. What happens if someone sets en-AE as a default culture on Windows? They will run into the exact same problem. Also, your fix can cause some different problems. Like someone is using the CurrentCulture name to create a RegionInfo object. If you are using the parent (which most likely be a neutral culture, RegionInfo creation will throw exception.

but also fixes the behavior that we didn't return specific system culture.

I am fine with the change in pal_locale.m if other reviewers see this is a correct fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants