Nullable: Comparers, Dictionary and Friends #23971

safern · 2019-04-13T19:20:40Z

Depends on latest compiler to be pulled in + all errors introduced in regression: dotnet/roslyn#34976 to be fixed.

src/System.Private.CoreLib/shared/System/Collections/DictionaryEntry.cs

src/System.Private.CoreLib/shared/System/Collections/Generic/KeyValuePair.cs

src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs

safern · 2019-04-15T21:26:18Z

We need to decide what to do with Dictionary because TKey should be non-nullable but the only way we have to do that is by constraining where TKey is object -- however we need to figure out if that causes boxing or some unexpected behaviour that would cause it to be a breaking change.

cc: @jkotas any input on this?

jkotas · 2019-04-15T21:43:28Z

where TKey is object

Is this the actual syntax? What is the compiler version I need for this?

Yes, whatever we do for annotating Dictionary should better not change the IL.

stephentoub · 2019-04-15T21:44:27Z

Is this the actual syntax?

where TKey : object

jkotas · 2019-04-15T23:19:56Z

where TKey : object

This results into IL change. In theory, it should not break anything. In practice, there is a chance that it will break some places in unexpected ways. We have not done a change like this before so it is hard to predict.

For example, typeof(Dictionary<>).GetGenericArguments()[0].GetGenericParameterConstraints().Length is 0 today and it is going to be 1 with this change. There may be code out there that depends on it to be 0.

safern · 2019-04-16T17:08:08Z

For example, typeof(Dictionary<>).GetGenericArguments()[0].GetGenericParameterConstraints().Length is 0 today and it is going to be 1 with this change. There may be code out there that depends on it to be 0.

So based on that, you think we shouldn't constrain TKey?

jkotas · 2019-04-16T17:18:44Z

It may ok, but we need to be prepared to explain and defend the change once/if breaks show up.

safern · 2019-04-22T22:39:02Z

I added the constraint and rebased to fix merge conflicts. This is ready for review, PTAL.

src/System.Private.CoreLib/shared/System/Array.cs

src/System.Private.CoreLib/shared/System/Collections/Generic/Comparer.cs

src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs

src/System.Private.CoreLib/shared/System/Collections/Generic/EqualityComparer.cs

src/System.Private.CoreLib/shared/System/Collections/IEqualityComparer.cs

src/System.Private.CoreLib/shared/System/Collections/IDictionaryEnumerator.cs

src/System.Private.CoreLib/shared/System/StringComparer.cs

safern · 2019-04-25T22:07:07Z

I've addressed feedback. Had to add a bunch of ! in ValueTuple due to IEqualityComparer.GetHashCode marked as not accepting nulls.

* Nullable: Comparers, Dictionary and Friends * Add object constraint to Dictionary * Fix warning from new compiler and annotating dictionary * PR Feedback

* Nullable: Comparers, Dictionary and Friends * Add object constraint to Dictionary * Fix warning from new compiler and annotating dictionary * PR Feedback Signed-off-by: dotnet-bot <[email protected]>

* Nullable: Comparers, Dictionary and Friends * Add object constraint to Dictionary * Fix warning from new compiler and annotating dictionary * PR Feedback Commit migrated from dotnet/coreclr@ad38bf7

safern commented Apr 13, 2019

View reviewed changes

src/System.Private.CoreLib/shared/System/Collections/DictionaryEntry.cs Outdated Show resolved Hide resolved

safern commented Apr 13, 2019

View reviewed changes

src/System.Private.CoreLib/shared/System/Collections/Generic/KeyValuePair.cs Show resolved Hide resolved

safern requested a review from a team April 13, 2019 19:23

danmoseley reviewed Apr 15, 2019

View reviewed changes

src/System.Private.CoreLib/shared/System/Collections/Generic/Dictionary.cs Outdated Show resolved Hide resolved

safern added 3 commits April 22, 2019 14:57

Nullable: Comparers, Dictionary and Friends

39cf326

Add object constraint to Dictionary

48e6f1d

Fix warning from new compiler and annotating dictionary

54cee1f

safern force-pushed the NullableCollections branch from 6a9d7f7 to 54cee1f Compare April 22, 2019 22:38