Skip to content

Remove TKey : notnull constraint from IDictionary<TKey, TValue> #793

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.

Sign up for GitHub

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

Jump to bottom
Merged
safern merged 3 commits into from
Dec 13, 2019
Merged

Remove TKey : notnull constraint from IDictionary<TKey, TValue> #793

safern merged 3 commits into from
Dec 13, 2019

Conversation

@stephentoub
Copy link
Member

@stephentoub stephentoub commented Dec 12, 2019

The interface explicitly documents that some implementations may choose to support null keys.

Fixes https://github.com/dotnet/corefx/issues/42386
cc: @buyaa-n, @safern, @sharwell

@stephentoub
Remove TKey : notnull constraint from IDictionary<TKey, TValue>
549263e 
The interface explicitly documents that some implementations may choose to support null keys.
safern
safern reviewed Dec 12, 2019
View reviewed changes
Copy link
Member

@safern safern left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about IReadOnlyDictionary and others?

https://github.com/dotnet/runtime/blob/master/src/libraries/System.Private.CoreLib/src/System/Collections/Generic/IReadOnlyDictionary.cs#L10

@stephentoub
Copy link
Member Author

stephentoub commented Dec 12, 2019

What about IReadOnlyDictionary and others?

Yes, we should make the same change for IReadOnlyDictionary. Good catch.

What "others"?

@safern
Copy link
Member

safern commented Dec 12, 2019

What "others"?

I don’t think there are others.

@safern
Copy link
Member

safern commented Dec 12, 2019

Actually I found some extension methos on IDictionary and IReadOnlyDictionary that have that constraint, should we remove it for those? Sorry I was on the phone earlier and couldn't do a good search on the repo.

https://github.com/dotnet/runtime/blob/master/src/libraries/System.Collections/ref/System.Collections.cs#L48
https://github.com/dotnet/runtime/blob/master/src/libraries/System.Collections/ref/System.Collections.cs#L51
https://github.com/dotnet/runtime/blob/master/src/libraries/System.Collections/ref/System.Collections.cs#L52

Those are all I could find.

@ahsonkhan ahsonkhan added this to the 5.0 milestone Dec 12, 2019
@stephentoub
Copy link
Member Author

stephentoub commented Dec 12, 2019

Thanks. I searched as well and pushed an update.

sharwell
sharwell reviewed Dec 12, 2019
View reviewed changes
src/libraries/System.Collections/src/System/Collections/Generic/CollectionExtensions.cs
return dictionary.GetValueOrDefault(key, default);
}

[return: MaybeNull]
Copy link
Contributor

@sharwell sharwell Dec 12, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting. This needs to be MaybeNullIfNull("defaultValue"), but that doesn't exist.

@safern safern merged commit 48a78bf into dotnet:master Dec 13, 2019
@stephentoub stephentoub deleted the idictionarynotnull branch January 22, 2020 01:47
@jpobst jpobst mentioned this pull request Apr 10, 2020
Merged
jonpryor pushed a commit to dotnet/android that referenced this pull request Apr 23, 2020
@jpobst
[Mono.Android] Add NRT annotations. (#4227)
d471b4a 
Context: dotnet/java-interop@01d0684

Annotates all API levels of `Mono.Android.dll` with
[C#8 Nullable Reference Type][0] (NRT) annotations, pulled directly
from the Java `.jar` files.

Additionally, our hand written code has been audited and updated to
include NRT annotations.

~~ Notes ~~

There are 8 new warnings caused by this change of the form:

	Android.Runtime/JavaDictionary.cs(655,24): warning CS8714: The type 'K' cannot be used as type parameter 'TKey' in the generic type or method 'IDictionary<TKey, TValue>'. Nullability of type argument 'K' doesn't match 'notnull' constraint.

This is due to the BCL being improperly annotated.  The change has
since been reverted, but it has not made it into various distribution
packs:

	dotnet/runtime#793

There are a considerable number of warnings (~400) caused by
mismatched annotations when members are overridden, such as:

	CS8610: Nullability of reference types in type of parameter 'baz' doesn't match overridden member.

While it may be desirable to fix these, it is a non-trivial job that
will be prioritized separately.

In the mean time, this set of warnings has been disabled:

	<NoWarn>8609;8610;8614;8617;8613;8764;8765;8766;8767</NoWarn>

Finally, when `$(AndroidGenerateJniMarshalMethods)`=True and
`jnimarshalmethod-gen.exe` is executed, the resulting app was crashing
because `MagicRegistrationMap.CallRegisterMethodByIndex()` had a
parameter change from `int` to `int?`.  Fix this by calling
`Nullable<int>.GetValueOrDefault()` with the `switch`.
`MonoDroidMarkStep.UpdateRegistrationSwitch()` now emits IL like:

	.method private hidebysig static bool
	        CallRegisterMethodByIndex([Java.Interop]Java.Interop.JniNativeMethodRegistrationArguments arguments,
	        [mscorlib]System.Nullable`1<int32> typeIdx) cil managed
	{
	  // Code size       9285 (0x2445)
	  .maxstack  2
	  .locals init ([mscorlib]System.Nullable`1<int32> V_0)
	  IL_0000:  ldarg.1
	  IL_0001:  stloc.0
	  IL_0002:  ldloca.s   V_0
	  IL_0004:  call       instance !0 [mscorlib]System.Nullable`1<int32>::GetValueOrDefault()
	  IL_0009:  switch     (
	  …

Co-authored-by: Radek Doulik <[email protected]>

[0]: https://docs.microsoft.com/en-us/dotnet/csharp/nullable-references
jonpryor pushed a commit to dotnet/android that referenced this pull request Apr 23, 2020
@jpobst @jonpryor
[Mono.Android] Add NRT annotations. (#4227)
0077c8f 
Context: dotnet/java-interop@01d0684

Annotates all API levels of `Mono.Android.dll` with
[C#8 Nullable Reference Type][0] (NRT) annotations, pulled directly
from the Java `.jar` files.

Additionally, our hand written code has been audited and updated to
include NRT annotations.

~~ Notes ~~

There are 8 new warnings caused by this change of the form:

	Android.Runtime/JavaDictionary.cs(655,24): warning CS8714: The type 'K' cannot be used as type parameter 'TKey' in the generic type or method 'IDictionary<TKey, TValue>'. Nullability of type argument 'K' doesn't match 'notnull' constraint.

This is due to the BCL being improperly annotated.  The change has
since been reverted, but it has not made it into various distribution
packs:

	dotnet/runtime#793

There are a considerable number of warnings (~400) caused by
mismatched annotations when members are overridden, such as:

	CS8610: Nullability of reference types in type of parameter 'baz' doesn't match overridden member.

While it may be desirable to fix these, it is a non-trivial job that
will be prioritized separately.

In the mean time, this set of warnings has been disabled:

	<NoWarn>8609;8610;8614;8617;8613;8764;8765;8766;8767</NoWarn>

Finally, when `$(AndroidGenerateJniMarshalMethods)`=True and
`jnimarshalmethod-gen.exe` is executed, the resulting app was crashing
because `MagicRegistrationMap.CallRegisterMethodByIndex()` had a
parameter change from `int` to `int?`.  Fix this by calling
`Nullable<int>.GetValueOrDefault()` with the `switch`.
`MonoDroidMarkStep.UpdateRegistrationSwitch()` now emits IL like:

	.method private hidebysig static bool
	        CallRegisterMethodByIndex([Java.Interop]Java.Interop.JniNativeMethodRegistrationArguments arguments,
	        [mscorlib]System.Nullable`1<int32> typeIdx) cil managed
	{
	  // Code size       9285 (0x2445)
	  .maxstack  2
	  .locals init ([mscorlib]System.Nullable`1<int32> V_0)
	  IL_0000:  ldarg.1
	  IL_0001:  stloc.0
	  IL_0002:  ldloca.s   V_0
	  IL_0004:  call       instance !0 [mscorlib]System.Nullable`1<int32>::GetValueOrDefault()
	  IL_0009:  switch     (
	  …

Co-authored-by: Radek Doulik <[email protected]>

[0]: https://docs.microsoft.com/en-us/dotnet/csharp/nullable-references
@ghost ghost locked as resolved and limited conversation to collaborators Dec 11, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@safern safern safern approved these changes

+1 more reviewer

@sharwell sharwell sharwell approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

area-System.Collections

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

5 participants

@stephentoub @safern @sharwell @ahsonkhan @Dotnet-GitSync-Bot