Limit `PNSE` by using Invariant `HashCode` in `HybridGlobalization` - #96354

Merged

ilonatommy merged 24 commits into

dotnet:mainfrom

ilonatommy:fix-95921

Jan 19, 2024

ilonatommy commented Dec 29, 2023•
edited
Loading

Member

Fixes paritally #95921.

We used to throw PNSE for all HashCode functions. The consequence was throwing PNSE for quite a few APIs, some of them crucial and far-connected with Globalization. With this PR the following APIs do not throw PNSE always (see the update in the doc for details):

- System.Collections.Hashtable.Add - System.Collections.Hashtable.GetHash - System.Collections.CaseInsensitiveHashCodeProvider.GetHashCode - System.Collections.Specialized.NameValueCollection.Add - System.Collections.Specialized.NameValueCollection.Remove - System.Collections.Specialized.NameValueCollection.Get - System.Collections.Specialized.NameValueCollection.Set - System.Collections.Specialized.NameObjectCollectionBase.BaseAdd - System.Collections.Specialized.NameObjectCollectionBase.BaseGet - System.Collections.Specialized.NameObjectCollectionBase.BaseRemove - System.Collections.Specialized.NameObjectCollectionBase.BaseSet - System.Collections.Specialized.OrderedDictionary.Add - System.Collections.Generic.HashSet - System.Collections.Generic.Dictionary

JS does not expose native, locale-sensitive hashing that we could use, so we decided to utilize Invariant version of functions instead with some limitations. Invariant hashing algorithm differs from ICU hashing with not ignoring empty unicode chars (e.g. zero width joiner), the strings get cleaned from them first.

Invariant locale - always supported with this PR by using invariant hashing method. This approach differs from the ICU approach where invariant culture was getting data from ICU4C.
Non-invariant locales with CompareOptions.None - use invariant hashing algo.
Non-invariant locales with CompareOptions.IgnoreCase - use JS-native toLower() and then invariant hashing algo.
Non-invariant locales with any other CompareOptions e.g. CompareOptions.IgnoreCase | CompareOptions.IgnoreSymbols - keep throwing PNSE, as it was without this PR.

It will be documented in the HG doc, so far I did not find an example where the behavior of APIs that threw PNSE so far differs between ICU and HG because of this change.

Out of scope (for follow-up PRs):

performance tests for methods using HashCode and performance improvements
SortKey - investigate similar change

ilonatommy added 2 commits

December 29, 2023 12:14

Unblock all tests.

7f5d711

SortKey is Invariant now.

709da38

ilonatommy added arch-wasm area-System.Globalization labels

ilonatommy self-assigned this

ghost commented Dec 29, 2023

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

Issue Details

Fixes #95921.

Author:	ilonatommy
Assignees:	ilonatommy
Labels:	`arch-wasm`, `area-System.Globalization`
Milestone:	-

ilonatommy added 3 commits

December 29, 2023 14:12

Missing SortKey changes + fix HashCode.

6e72858

Typo

2d7a037

Revert unbocking tests connected with CompareOptions PNSE.

b896c17

dotnet deleted a comment from azure-pipelinesBot

azure-pipelinesBot commented Dec 29, 2023

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Dec 29, 2023

System.Text.Json failing some large file tests #59678

Closed

[mono][tests] System.Runtime.Caching assert fails on linux-bionic #93106

Closed

[6.0] System.Random test failure: NextInt_AllValuesAreWithinSpecifiedRange #83421

Closed

Hashing uses Invariant mode -these tests should be skipped.

de22e70

azure-pipelinesBot commented Dec 29, 2023

Azure Pipelines successfully started running 1 pipeline(s).

ilonatommy mentioned this pull request

[browser] HybridGlobalization allows different hashes for strings that return true for Equals#96400

Closed

Add active issue.

5778cd0

ilonatommy requested a review from matouskozak

January 2, 2024 15:12

azure-pipelinesBot commented Jan 2, 2024

Azure Pipelines successfully started running 1 pipeline(s).

ilonatommy requested review from SamMonoRT, mkhamoyan and tarekgh

January 2, 2024 15:12

ilonatommy marked this pull request as ready for review

January 2, 2024 15:13

stephentoub reviewed

View reviewed changes

src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs Outdated

feedback

d0666cd

azure-pipelinesBot commented Jan 2, 2024

Azure Pipelines successfully started running 1 pipeline(s).

build-analysisBot mentioned this pull request

Checkout failure: "Git fetch failed with exit code 128" dotnet/arcade#9009

Open

2 tasks

pavelsavara approved these changes

View reviewed changes

...braries/System.Runtime/tests/System.Globalization.Extensions.Tests/GetStringComparerTests.cs Outdated

src/libraries/System.Runtime/tests/System.Runtime.Extensions.Tests/System/StringComparer.cs

src/libraries/System.Runtime/tests/System.Runtime.Tests/System/StringGetHashCodeTests.cs Outdated

src/libraries/System.Runtime/tests/System.Runtime.Tests/System/Text/RuneTests.cs

src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Icu.cs

Feedback

65236ab

build-analysisBot mentioned this pull request

BasicEventSourceTests.TestsManifestGeneration test failing in CI #97103

Closed

azure-pipelinesBot commented Jan 17, 2024

Azure Pipelines successfully started running 1 pipeline(s).

build-analysisBot mentioned this pull request

Test failure - System.NullReferenceException in System.Threading.Lock.TryInitializeStatics #94728

Closed

SortKey not supported for non-invariant cultures, HashCode suppor…

9423bcc

…ted for non-invariant cultures only with `IgnoreCase` or `None` options.

pavelsavara reviewed

View reviewed changes

pavelsavara left a comment

Member

Thanks for patience with my feedback.

src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.WebAssembly.cs

src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.cs

src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.WebAssembly.cs Outdated

src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.WebAssembly.cs Outdated

src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.Icu.cs Outdated

src/libraries/System.Private.CoreLib/src/System/Globalization/CompareInfo.WebAssembly.cs Outdated

ilonatommy changed the title ~~Limit PNSE by using Invariant SortKey in HybridGlobalization~~Limit PNSE by using Invariant HashCode and SortKey in HybridGlobalization

ilonatommy changed the title ~~Limit PNSE by using Invariant HashCode and SortKey in HybridGlobalization~~Limit PNSE by using Invariant HashCode in HybridGlobalization

ilonatommy added 4 commits

January 18, 2024 11:34

Feedback.

2887914

Fix build, add docs.

a599f77

Feedback@matouskozak@pavelsavara

c9dc938

Merge branch 'main' into fix-95921

bfb12b3

build-analysisBot mentioned this pull request

[browser][MT] various WebWorkerTest CI failures #96628

Closed

Added tests + fixed algo.

398d047

azure-pipelinesBot commented Jan 19, 2024

Azure Pipelines successfully started running 1 pipeline(s).

ilonatommy added 3 commits

January 19, 2024 16:16

Block failing tests for a follow-up PR.

ee1f542

Add more details to PNSE.

10e9060

Feedback - correct comment

80d9d37

ilonatommy commented Jan 19, 2024

MemberAuthor

/azp run runtime-wasm-libtests

azure-pipelinesBot commented Jan 19, 2024

Azure Pipelines successfully started running 1 pipeline(s).

pavelsavara approved these changes

View reviewed changes

build-analysisBot mentioned this pull request

[browser][AOT] convert the LLVM value V to DTYPE #97217

Closed

ilonatommy commented Jan 19, 2024

MemberAuthor

AOT lanes won't finish, they are generally broken. Failures are not related.

ilonatommy merged commit 993b23f into dotnet:main

EgorBo mentioned this pull request

Test failing: System.Globalization.Tests.CompareInfoHashCodeTests.CheckHashingOfSkippedChars #97241

Closed

ilonatommy mentioned this pull request

Hash code tests cleanup #97299

Merged

ilonatommy added a commit to ilonatommy/runtime that referenced this pull request

Fixed bydotnet#96354

91079fa

ilonatommy mentioned this pull request

[browser] HybridGlobalization correct HashCode ranges of skipped unicodes #97351

Closed

tmds pushed a commit to tmds/runtime that referenced this pull request

LimitPNSEby using InvariantHashCodeinHybridGlobalization(d…

2df0518

…otnet#96354)
* Unblock all tests.
* SortKey is Invariant now.
* Missing SortKey changes + fix HashCode.
* Typo
* Revert unbocking tests connected with CompareOptions PNSE.
* Hashing uses Invariant mode -these tests should be skipped.
* Add active issue.
* feedback
* Feedback
* Better documentation.
* Add new tests + sanitize string before invariant comparison.
* Comment + more cases.
* Clean CI.
* Missing change for clean CI commit.
* `SortKey` not supported for non-invariant cultures, `HashCode` supported for non-invariant cultures only with `IgnoreCase` or `None` options.
* Feedback.
* Fix build, add docs.
* Feedback @matouskozak@pavelsavara
* Added tests + fixed algo.
* Block failing tests for a follow-up PR.
* Add more details to PNSE.
* Feedback - correct comment

ilonatommy added a commit that referenced this pull request

Hash code tests cleanup (#97299)

75dc1b3

* Moving the tests, cleaning up.
* Removal.
* Rename: fix build.
* Fix order, apply feedback
* Missing changes.
* Fixed by #96354
* This test uses SortKey, should not be enabled.
* Nit
* These methods use `CompareOptions = IgnoreCase, IgnoreKanaType, IgnoreWidth`, should stay blocked.

github-actionsBot locked and limited conversation to collaborators

Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

arch-wasmarea-System.Globalization