Skip to content

Commit

Permalink
Limit PNSE by using Invariant HashCode in HybridGlobalization (d…
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
ilonatommy authored and tmds committed Jan 23, 2024
1 parent bc97ada commit 2df0518
Show file tree
Hide file tree
Showing 55 changed files with 416 additions and 214 deletions.
21 changes: 18 additions & 3 deletions docs/design/features/globalization-hybrid-mode.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,29 @@ For WebAssembly in Browser we are using Web API instead of some ICU data. Ideall

Hybrid has higher priority than sharding or custom modes, described in globalization-icu-wasm.md.

**HashCode**

Affected public APIs:
- System.Globalization.CompareInfo.GetHashCode

For invariant culture all `CompareOptions` are available.

For non-invariant cultures following `CompareOptions` are available:
- `CompareOption.None`
- `CompareOption.IgnoreCase`

The remaining combinations for non-invariant cultures throw `PlatformNotSupportedException`.

**SortKey**

Affected public APIs:
- System.Globalization.CompareInfo.GetSortKey
- System.Globalization.CompareInfo.GetSortKeyLength
- System.Globalization.CompareInfo.GetHashCode

For invariant culture all `CompareOptions` are available.

For non-invariant cultures `PlatformNotSupportedException` is thrown.

Indirectly affected APIs (the list might not be complete):
- Microsoft.VisualBasic.Collection.Add
- System.Collections.Hashtable.Add
Expand All @@ -43,8 +60,6 @@ Indirectly affected APIs (the list might not be complete):
- System.Net.Mail.MailAddress.GetHashCode
- System.Xml.Xsl.XslCompiledTransform.Transform

Web API does not have an equivalent, so they throw `PlatformNotSupportedException`.

**Case change**

Affected public APIs:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public static void Add_RelativeIndex()
Assert.Equal(item1, coll[3]);
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Fact]
public static void Add_RelativeKey()
{
var coll = new Collection();
Expand Down Expand Up @@ -175,7 +175,7 @@ public static void RemoveAt_InvalidIndex_ThrowsArgumentOutOfRangeException()
Assert.Throws<ArgumentOutOfRangeException>("Index", () => coll.RemoveAt(-1)); // Index < 0
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Fact]
public static void Remove_Key()
{
var coll = CreateKeyedCollection(10);
Expand All @@ -185,7 +185,7 @@ public static void Remove_Key()
Assert.False(coll.Contains("Key3"));
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Fact]
public static void Remove_InvalidKey_ThrowsArgumentException()
{
var coll = CreateKeyedCollection(10);
Expand Down Expand Up @@ -242,7 +242,7 @@ public static void Contains()
Assert.False(coll.Contains(new Foo()));
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Fact]
public static void Contains_ByKey()
{
var coll = CreateKeyedCollection(10);
Expand Down Expand Up @@ -275,7 +275,7 @@ public static void Item_Get_InvalidIndex_ThrowsIndexOutOfRangeException()
Assert.Throws<ArgumentException>(() => coll[(object)Guid.Empty]); // Neither string nor int
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Fact]
public static void Item_GetByKey()
{
Collection coll = CreateKeyedCollection(10);
Expand All @@ -291,7 +291,7 @@ public static void Item_GetByKey()
Assert.Equal(CreateValue(11), coll[(object)'X']);
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Fact]
public static void Item_GetByKey_InvalidIndex_ThrowsIndexOutOfRangeException()
{
Collection coll = CreateKeyedCollection(10);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public void LateSet(object obj, Type objType, string name, object[] args, string
Assert.Equal(expected, getResult(obj));
}

[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Theory]
[MemberData(nameof(LateSetComplex_TestData))]
public void LateSetComplex(object obj, Type objType, string name, object[] args, string[] paramNames, bool missing, bool valueType)
{
Expand Down Expand Up @@ -74,7 +74,7 @@ public void LateIndexSet(object obj, object[] args, string[] paramNames, Func<ob
Assert.Equal(expected, getResult(obj));
}

[Theory]
[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[MemberData(nameof(LateIndexSet_MissingMember_TestData))]
public void LateIndexSet_MissingMember(object obj, object[] args, string[] paramNames)
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ namespace System.Collections.Tests
{
public class CaseInsensitiveHashCodeProviderTests
{
[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Theory]
[InlineData("hello", "HELLO", true)]
[InlineData("hello", "hello", true)]
[InlineData("HELLO", "HELLO", true)]
Expand All @@ -29,7 +29,7 @@ public void Ctor_Empty_GetHashCodeCompare(object a, object b, bool expected)
Assert.Equal(expected, provider.GetHashCode(a) == provider.GetHashCode(b));
}

[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Theory]
[InlineData("hello", "HELLO", true)]
[InlineData("hello", "hello", true)]
[InlineData("HELLO", "HELLO", true)]
Expand Down Expand Up @@ -63,7 +63,7 @@ public void Ctor_Empty_ChangeCurrentCulture_GetHashCodeCompare(object a, object
}
}

[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Theory]
[InlineData("hello", "HELLO", true)]
[InlineData("hello", "hello", true)]
[InlineData("HELLO", "HELLO", true)]
Expand Down Expand Up @@ -94,9 +94,10 @@ public void Ctor_CultureInfo_ChangeCurrentCulture_GetHashCodeCompare(object a, o
}
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Fact]
[ActiveIssue("https://github.com/dotnet/runtime/issues/37069", TestPlatforms.Android | TestPlatforms.LinuxBionic)]
[ActiveIssue("https://github.com/dotnet/runtime/issues/95338", typeof(PlatformDetection), nameof(PlatformDetection.IsHybridGlobalizationOnApplePlatform))]
[ActiveIssue("https://github.com/dotnet/runtime/issues/95503", typeof(PlatformDetection), nameof(PlatformDetection.IsHybridGlobalizationOnBrowser))]
public void Ctor_CultureInfo_GetHashCodeCompare_TurkishI()
{
var cultureNames = Helpers.TestCultureNames;
Expand Down Expand Up @@ -135,7 +136,7 @@ public void GetHashCode_NullObj_ThrowsArgumentNullException()
AssertExtensions.Throws<ArgumentNullException>("obj", () => new CaseInsensitiveHashCodeProvider().GetHashCode(null));
}

[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Theory]
[InlineData("hello", "HELLO", true)]
[InlineData("hello", "hello", true)]
[InlineData("HELLO", "HELLO", true)]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ namespace System.Collections.Tests
{
public static class CollectionsUtilTests
{
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Fact]
public static void CreateCaseInsensitiveHashtable()
{
Hashtable hashtable = CollectionsUtil.CreateCaseInsensitiveHashtable();
Expand All @@ -20,7 +20,7 @@ public static void CreateCaseInsensitiveHashtable()
AssertExtensions.Throws<ArgumentException>(null, () => hashtable.Add("key1", "value1"));
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Fact]
public static void CreateCaseInsensitiveHashtable_Capacity()
{
Hashtable hashtable = CollectionsUtil.CreateCaseInsensitiveHashtable(15);
Expand All @@ -33,7 +33,7 @@ public static void CreateCaseInsensitiveHashtable_Capacity()
AssertExtensions.Throws<ArgumentException>(null, () => hashtable.Add("key1", "value1"));
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Fact]
public static void CreateCaseInsensitiveHashtable_IDictionary()
{
Hashtable hashtable1 = CollectionsUtil.CreateCaseInsensitiveHashtable();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -779,7 +779,7 @@ public void Values_ModifyingHashtable_ModifiesCollection()
}
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Fact]
public void HashCodeProvider_Set_ImpactsSearch()
{
var hash = new ComparableHashtable(CaseInsensitiveHashCodeProvider.DefaultInvariant, StringComparer.OrdinalIgnoreCase);
Expand Down Expand Up @@ -834,7 +834,7 @@ public void HashCodeProvider_Comparer_IncompatibleGetSet_Throws()
AssertExtensions.Throws<ArgumentException>(null, () => hash.Comparer = StringComparer.OrdinalIgnoreCase);
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Fact]
public void Comparer_Set_ImpactsSearch()
{
var hash = new ComparableHashtable(CaseInsensitiveHashCodeProvider.DefaultInvariant, StringComparer.OrdinalIgnoreCase);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public void Constructor_Provider_Comparer()
Assert.Equal(0, coll.Count);
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Fact]
public void Constructor_Int_Provider_Comparer()
{
#pragma warning disable CS0618 // Type or member is obsolete
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace System.Collections.Specialized.Tests
{
public class NameObjectCollectionBaseCopyToTests
{
[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Theory]
[InlineData(0, 0)]
[InlineData(0, 5)]
[InlineData(10, 0)]
Expand Down Expand Up @@ -39,7 +39,7 @@ public void CopyTo(int count, int index)
Assert.Equal(previousCount, copyArray.Length);
}

[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Theory]
[InlineData(0)]
[InlineData(10)]
public void CopyTo_Invalid(int count)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace System.Collections.Specialized.Tests
{
public class GetAllValuesTests
{
[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Theory]
[InlineData(0, typeof(object))]
[InlineData(0, typeof(Foo))]
[InlineData(10, typeof(object))]
Expand All @@ -33,7 +33,7 @@ private static void VerifyGetAllValues(NameObjectCollectionBase nameObjectCollec
}
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Fact]
public static void GetAllValues_Invalid()
{
MyNameObjectCollection nameObjectCollection = new MyNameObjectCollection();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace System.Collections.Specialized.Tests
{
public class NameObjectCollectionBaseGetEnumeratorTests
{
[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Theory]
[InlineData(0)]
[InlineData(10)]
public void GetEnumerator(int count)
Expand All @@ -29,7 +29,7 @@ public void GetEnumerator(int count)
}
}

[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Theory]
[InlineData(0)]
[InlineData(10)]
public void GetEnumerator_Invalid(int count)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace System.Collections.Specialized.Tests
{
public class NameObjectCollectionBaseKeysTests
{
[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Theory]
[InlineData(0)]
[InlineData(10)]
public void Keys_PreservesInstance(int count)
Expand All @@ -16,7 +16,7 @@ public void Keys_PreservesInstance(int count)
Assert.Same(nameObjectCollection.Keys, nameObjectCollection.Keys);
}

[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Theory]
[InlineData(0)]
[InlineData(10)]
public void Keys_GetEnumerator(int count)
Expand All @@ -40,7 +40,7 @@ public void Keys_GetEnumerator(int count)
}
}

[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Theory]
[InlineData(0)]
[InlineData(10)]
public void Keys_GetEnumerator_Invalid(int count)
Expand Down Expand Up @@ -85,7 +85,7 @@ public void Keys_GetEnumerator_Invalid(int count)
Assert.Throws<InvalidOperationException>(() => enumerator.Reset());
}

[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Theory]
[InlineData(0)]
[InlineData(10)]
public void Keys_Properties(int count)
Expand All @@ -97,7 +97,7 @@ public void Keys_Properties(int count)
Assert.False(keysCollection.IsSynchronized);
}

[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Theory]
[InlineData(0, 0)]
[InlineData(0, 5)]
[InlineData(10, 0)]
Expand Down Expand Up @@ -145,7 +145,7 @@ private static void Keys_CopyTo_Helper(MyNameObjectCollection nameObjectCollecti
Assert.Equal(previousCount, keysArray.Length);
}

[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Theory]
[InlineData(0)]
[InlineData(10)]
public void Keys_CopyTo_Invalid(int count)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace System.Collections.Specialized.Tests
{
public class NameObjectCollectionBaseReadOnlyTests
{
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Fact]
public void IsReadOnly_Set()
{
MyNameObjectCollection nameObjectCollection = Helpers.CreateNameObjectCollection(10);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace System.Collections.Specialized.Tests
{
public class NameObjectCollectionBaseRemoveAtTests
{
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Fact]
public void RemoveAt()
{
MyNameObjectCollection nameObjectCollection = Helpers.CreateNameObjectCollection(10);
Expand Down Expand Up @@ -59,7 +59,7 @@ public void RemoveAt()
}
}

[ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Theory]
[InlineData(0)]
[InlineData(10)]
public void RemoveAt_InvalidIndex_ThrowsArgumentOutOfRangeException(int count)
Expand All @@ -69,7 +69,7 @@ public void RemoveAt_InvalidIndex_ThrowsArgumentOutOfRangeException(int count)
AssertExtensions.Throws<ArgumentOutOfRangeException>("index", () => nameObjectCollection.RemoveAt(count));
}

[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Fact]
public void RemoveAt_ReadOnly_ThrowsNotSupportedException()
{
MyNameObjectCollection nameObjectCollection = Helpers.CreateNameObjectCollection(1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ namespace System.Collections.Specialized.Tests
{
public class NameObjectCollectionBaseSetItemTests
{
[ConditionalFact(typeof(PlatformDetection), nameof(PlatformDetection.IsNotHybridGlobalizationOnBrowser))]
[Fact]
public void Set_ObjectAtIndex_ModifiesCollection()
{
var noc = new MyNameObjectCollection();
Expand Down
Loading

0 comments on commit 2df0518

Please sign in to comment.