[Apple] Preserve Norwegian locale names in hybrid globalization#130854
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 237ad744-83ac-4aac-a090-3b2622841b67
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 237ad744-83ac-4aac-a090-3b2622841b67
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 237ad744-83ac-4aac-a090-3b2622841b67
|
Azure Pipelines: Successfully started running 3 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @dotnet/area-system-globalization |
There was a problem hiding this comment.
Pull request overview
This PR adjusts Apple hybrid globalization locale canonicalization so that requests for Norwegian (no / no-NO) preserve the requested culture name and ISO language codes (instead of being canonicalized to nb), and adds regression coverage in System.Globalization tests.
Changes:
- Add Apple-hybrid helpers to resolve locale language code / identifier while preserving
nowhen Foundation canonicalizes tonb. - Use the preserved language code for ISO639 2- and 3-letter language name queries and for parent-culture computation.
- Add a regression theory verifying
CultureInfo("no")andCultureInfo("no-NO")language-code/name/parent expectations.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/native/libs/System.Globalization.Native/pal_locale.m | Centralizes Apple-hybrid language-code / locale-identifier handling and uses it for locale name, ISO codes, and ParentName. |
| src/libraries/System.Runtime/tests/System.Globalization.Tests/CultureInfo/CultureInfoCtor.cs | Adds Norwegian regression assertions for culture name, ISO language codes, and parent culture. |
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 237ad744-83ac-4aac-a090-3b2622841b67
The strict Name/ISO/Parent assertions are only guaranteed on ICU (including Apple hybrid); exclude the NLS and invariant legs where no-NO may be unsupported. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 237ad744-83ac-4aac-a090-3b2622841b67
|
Azure Pipelines: Successfully started running 3 pipeline(s). 12 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
/azp run runtime-extra-platforms,runtime |
|
Azure Pipelines: Successfully started running 2 pipeline(s). |
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "a6e37f0d06c26976573d0edc4f8db56564c5b273",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "f89572a970c9ca60a097abdedde4884f3df37822",
"last_reviewed_commit": "a6e37f0d06c26976573d0edc4f8db56564c5b273",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "f89572a970c9ca60a097abdedde4884f3df37822",
"last_recorded_worker_run_id": "29685659467",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "a6e37f0d06c26976573d0edc4f8db56564c5b273",
"review_id": 4730702847
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: Justified. This fixes a confirmed .NET 9 regression (#112249) where Apple hybrid globalization returns nb for the no culture because Foundation canonicalizes no→nb, unlike ICU/Windows/Android which preserve no. The result broke Norwegian resource lookup on iOS, a real, user-reported break with a clear repro.
Approach: Sound and appropriately narrow. The fix centralizes Apple locale language-code resolution into GetLocaleLanguageCode/GetLocaleIdentifier helpers and only overrides Foundation's canonicalization for the specific no→nb case, leaving all other locales untouched. Name, ISO 639-1/639-2, and parent-name paths all flow through the same helpers, keeping the culture name, two/three-letter codes, and no-NO→no parent fallback consistent.
Summary: ✅ LGTM. The special-case is tightly guarded (case-insensitive no subtag match plus exact nb canonical match), the identifier rewrite is defensively bounded (nil checks, equal-code short-circuit, and a length guard before the substring splice), and regression coverage is added as an ICU-gated theory plus the no entry restored to the general ctor matrix. No blocking concerns found.
Detailed Findings
✅ Correctness — Narrow, well-guarded override
GetLocaleLanguageCode only substitutes no when the original subtag is no (case-insensitive) AND Foundation canonicalized it to nb, so nb/nn inputs are unaffected. GetLocaleIdentifier rewrites the identifier prefix only when languageCode != canonicalLanguageCode, which today only fires for the no case; the localeIdentifier.length < canonicalLanguageCode.length guard prevents an out-of-range substringFromIndex:. Because no and nb are both length 2 and Foundation's identifier begins with the language code, the splice (no + suffix of nb_...) is safe. Empty-string handling in GetLocaleNameNative is preserved.
✅ Test quality — Appropriate gating and placement
The new Ctor_String_NorwegianLanguageNames theory asserting Name, TwoLetterISOLanguageName, ThreeLetterISOLanguageName, and Parent.Name is correctly gated to IsIcuGlobalization (per the commit message, NLS/invariant may not support no-NO), and uses [Theory]/[InlineData] per conventions. Moving no out of the IsNotHybridGlobalizationOnApplePlatform-only block into the general Ctor_String matrix now exercises the fixed Apple hybrid path directly.
💡 Observation — Generality of the identifier rewrite
The helper naming (GetLocaleIdentifier) reads as general-purpose, but its rewrite branch is effectively no-specific today. If future locales need similar alias handling, the length/prefix assumptions (language code is a leading prefix of the identifier and same length as the alias) would need re-validation. Non-blocking; the current guards make it safe for the only case that triggers it.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 71.2 AIC · ⌖ 10.5 AIC · ⊞ 10K
|
/ba-g failures are not related to changes in this PR |
Summary
noculture name and ISO language codes when Foundation canonicalizes it tonbno-NOparent fallback tonofor resource lookupFixes #112249
Testing
CultureInfoconstructor matrix: 348 passed, 0 failedGetCultureInfocanonicalization matrix: 22 passed, 0 failedNote
This PR description was generated with GitHub Copilot.