-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Unify KC/KD normalization behavior on Wasm #121110
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes a bug where Browser/WASI platforms inconsistently threw PlatformNotSupportedException for FormKC/FormKD normalization forms. The issue occurred when the input string was ASCII-only, as the exception was only thrown after the ASCII fast path check. The fix ensures that the exception is thrown consistently regardless of input by checking for unsupported forms earlier in the validation process.
Key changes:
- Moved compatibility form validation before the ASCII fast path to ensure consistent exception behavior
- Consolidated duplicate Browser/WASI guard logic into a reusable
ThrowIfCompatibilityFormUnsupportedmethod - Added additional validation checks (Invariant and UseNls modes) to the guard conditions
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/libraries/System.Private.CoreLib/src/System/Globalization/Normalization.cs | Added new ThrowIfCompatibilityFormUnsupported method and called it from CheckNormalizationForm to validate compatibility forms before ASCII fast path |
| src/libraries/System.Private.CoreLib/src/System/Globalization/Normalization.Icu.cs | Replaced inline Browser/WASI check with call to the new consolidated ThrowIfCompatibilityFormUnsupported method |
src/libraries/System.Private.CoreLib/src/System/Globalization/Normalization.cs
Outdated
Show resolved
Hide resolved
|
Tagging subscribers to this area: @dotnet/area-system-globalization |
src/libraries/System.Private.CoreLib/src/System/Globalization/Normalization.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Globalization/Normalization.cs
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Globalization/Normalization.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Private.CoreLib/src/System/Globalization/Normalization.Icu.cs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one final comment. LGTM otherwise.
|
/ba-g |
Fixes #117116.
CheckNormalizationFormbefore the ASCII fast path so Browser/WASI throwPlatformNotSupportedExceptionfor FormKC/FormKD regardless of input.Why didn't we catch it in tests?
Because we do not test KC/KD at all, considering them not supported (
IsNotUsingLimitedCultures-> Browser with its ICU):runtime/src/libraries/System.Runtime/tests/System.Globalization.Extensions.Tests/Normalization/StringNormalizationTests.cs
Line 60 in 0c185bc