Remove Dictionary<K,V>.GetValueOrDefault instance methods and IDictionary<K,V>.GetValueOrDefault extension methods

[safern]

IDictionary<TKey,TValue> already has:

partial class CollectionExtensions
{
  public static TValue GetValueOrDefault<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key);
  public static TValue GetValueOrDefault<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key, TValue defaultValue);
}

As extension methods in CollectionExtensions.

In this discussion: #20684 we've decided to remove this instance methods, see: https://github.com/dotnet/corefx/issues/17275#issuecomment-291636863

cc: @danmosemsft @ianhays

[karelz]

API review: Approved in https://github.com/dotnet/corefx/issues/17275#issuecomment-291636863

[karelz]

This is fairly easy, while important for 2.0 and interesting as it touches both CoreFX and CoreCLR:

Next steps:

1. Undo parts of Expose and test (I)Dictionary.GetValueOrDefault corefx#16605
   • Delete APIs from ref contract: source code
   • Update uapaot baseline by running msbuild src\System.Collections\src\System.Collections.csproj /p:TargetGroup=uapaot /p:BaselineAllApiCompatError=true
2. When that is checked in, undo also Dictionary.GetValueOrDefault coreclr#8641
   • Don't do it before [1], otherwise you will block continuous updates of CoreCLR new bits into CoreFX
3. Remove implementation from CoreRT as well. Code to remove
   • Don't do it before [1], otherwise you will block continuous updates of CoreRT new bits into CoreFX -- this can be done at the same time as [2]

[safern]

I'm already in progress :)

[karelz]

Ah, pity, would be great 'easy' issue - need a couple for new contributors ...

[safern]

Give this one to them then :) So that they can get in context with the code -- I was just taking it to clean up my area as fast as possible. Unassigned!!

[safern]

Edited your next step comment to add a little bit more detailed for new contributors.

[Pothulapati]

I will Grab this, It's just deleting ref's and some code, Nothing to Code. Right?

[safern]

Hi @Pothulapati thanks for willing to contribute. You'll need to accept an invitation to collaborate so that we can assign it to you. I will assign it to myself meanwhile but now this issue is yours :) feel free to start working on it.

How do we send the invitation? @karelz @danmosemsft

[karelz]

Invitation sent - ping us when you accept @Pothulapati.

Thanks @safern, I appreciate you giving it up for the greater good ;-)

[Pothulapati]

Accepted The Invitation. Looking forward to the Issue.

[safern]

@Pothulapati the issue is assigned to you. You can start working on it!

If you have any questions please feel free to ask them here. Hope you enjoy contributing!

Thanks @karelz

[Pothulapati]

@safern Forked The Repo, When I try to Build The Repo For the first time, The "Installing dotnet Cli..." is taking a long time and not showing anything. It's because of the slow download speeds of the dotnet cli. Is There any alternative way to install dotnet cli?

[safern]

Hi, @Pothulapati did you get it to download? It shouldn't take that long. My guess would be more of like a network speed problem on your side. Before calling build.cmd on your repo you could try init-tools.cmd which is the standalone command to install cli.

[Pothulapati]

No,Network speed on my side is fast. I also Tried init-tools.cmd . It's the same. Even after 5 hours of Installing Dotnet cli... ,It doesn't respond.

[joperezr]

5 hours is definitely not what it would take even with a slow network. I suspect something went wrong. Can you try these two things to try to get us more info on what is going on:

• There should be a init-tools.log that gets generated in the process, and that will most likely give us more info that will help diagnosing the issue.
• Can you try running set _echo=on before running build.cmd? that will give us a more verbose output and tell us what might be going on and which step is the one that is stuck.

[Pothulapati]

@joperezr Now If I Run init-tools.cmd I get the following
C:\Users\Tarun\Source\Repos\corefx\Tools\DOTNET~1\dotnet.tar - The process cannot access the file because it is being used by another process. Installing dotnet cli...
and the log file as
Running "C:\Users\Tarun\Source\Repos\corefx\init-tools.cmd" Installing 'https://dotnetcli.blob.core.windows.net/dotnet/preview/Binaries/1.0.0-preview2-1-003182/dotnet-dev-win-x64.1.0.0-preview2-1-003182.zip' to 'C:\Users\Tarun\Source\Repos\corefx\Tools\dotnetcli\dotnet-dev-win-x64.1.0.0-preview2-1-003182.zip'

[joperezr]

looks like some process is still getting a handle to that zip, is there a way for you to try find that process and kill it, and then clean your repo: clean.cmd -all

[Pothulapati]

@joperezr Now,That I have found the process and Killed it. It Just Shows Installing dotnet cli.. but still the download is taking a lot of time. Why don't you try Downloading the dotnet-dev-win-x64.1.0.0-preview2-1-003182.zip so as to make sure it's not a network problem from my side?

[safern]

@Pothulapati I'm able to download -- where you able to figure it out? After you kill the process and cleaned your repo what does the log file says?

[safern]

@jkotas this should be done in CoreRT as well right?

[jkotas]

Yes - since Dictionary was not moved to the shared corelib partition yet.

[safern]

Thanks @jkotas

I updated the instructions to follow in https://github.com/dotnet/corefx/issues/17917#issuecomment-291749382 so that you don't forget @Pothulapati

[karelz]

@Pothulapati sorry to hear about your troubles with getting started :( ... this is truly weird and exceptional. We have plenty of contributors and this is the first time we hit similar issue.

Please if you, can try another machine, VM, new directory, or anything you can think of -- there is something truly weird going on on your machine and I would like to first get you unblocked ASAP, and then deep dive into what happened and why, so that we can avoid such bad experience in future for new contributors. Thank you for you patience and sorry for that!
Note: This is definitely not our typical first-time contributor experience.

[Pothulapati]

@safern After killing the process and running the clean.cmd I still get "Installing dotnet cli..." in the clean cmd(and it doesn't respond too).Sorry For Bugging you.

[Pothulapati]

@karelz I tried in another machine but it shows the same thing but when I run it in an Azure VM, It Works Fine.

[karelz]

@dotnet/fxdc quick review ask: We are pulling out Dictionary.GetValueOrDefault, but we need to delete an extension method to avoid compile-time ambiguity between IDictionary.GetValueOrDefault and IReadOnlyDictionary.GetValueOrDefault. Can we get quick ack on the plan please?

First proposed API (Remove IReadOnlyDictionary extension method)

partial class CollectionExtensions
{
  public static bool TryAdd<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key, TValue value)
  public static bool Remove<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key, out TValue value)
  public static TValue GetValueOrDefault<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key);
  public static TValue GetValueOrDefault<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key, TValue defaultValue);
- public static TValue GetValueOrDefault<TKey, TValue>(this IReadOnlyDictionary<TKey, TValue> dictionary, TKey key);
- public static TValue GetValueOrDefault<TKey, TValue>(this IReadOnlyDictionary<TKey, TValue> dictionary, TKey key, TValue defaultValue);
}

Keep IDictionary<TKey, TValue> extension method as it is implemented by more types and it is used in more places.

Second Option

Delete all GetValueOrDefault<TKey, TValue> extension methods because first of all it is not a very used api, and it solves a very simple problem. This would give our costumers the option to implement their own extension methods for any interface they want without causing ambiguities.

partial class CollectionExtensions
{
  public static bool TryAdd<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key, TValue value)
  public static bool Remove<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key, out TValue value)
- public static TValue GetValueOrDefault<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key);
- public static TValue GetValueOrDefault<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key, TValue defaultValue);
- public static TValue GetValueOrDefault<TKey, TValue>(this IReadOnlyDictionary<TKey, TValue> dictionary, TKey key);
- public static TValue GetValueOrDefault<TKey, TValue>(this IReadOnlyDictionary<TKey, TValue> dictionary, TKey key, TValue defaultValue);
}

See details: in today's code

If we have to API reviewers' upvotes, let's go ahead. If there are questions, let's bring it to proper API review on Tuesday.

[safern]

Thanks @karelz for bringing that quick API Proposal, hopefully we can get an answer soon so that we can solve the ambiguity.

[jnm2]

While we are here, people will run into the exact same situation with all the classes in the framework which implement IReadOnlyDictionary<,> because they all happen to implement IDictionary<,> as well:

• ConcurrentDictionary<,>
• Dictionary<,>
• ImmutableDictionary<,>
• ImmutableDictionary<,>.Builder
• ImmutableSortedDictionary<,>
• ImmutableSortedDictionary<,>.Builder
• ReadOnlyDictionary<,>
• SortedDictionary<,>
• SortedList<,>

[safern]

@karelz did we get any update from the review group?

This seems to be a breaking change we would be introducing, it can be workaround from our costumers but not ideal.

cc: @terrajobst

[stephentoub]

Our having added this extension to both IDictionary and IReadOnlyDictionary causes a problem for any type that implements both interfaces, something we expect to be common. We shouldn't add a specific overload for every one of our own types that implements both, and we can't do so for types we don't own (any 3rd party collection that implements both). I suggest the right solution is to remove either the extensions on IDictionary or the extensions on IReadOnlyDictionary, leaving just one of them.

[jnm2]

I'd hope to encourage wider implementation of IReadOnlyDictionary rather than IDictionary.

[karelz]

I vaguely remember the discussions about IDictionary usage vs. IReadOnlyDictionary - was it on the original bug?

We didn't review it, because I didn't mark it for API review :( ... we may need to postpone it for next Tuesday.

[svick]

@karelz Are you referring to this comment: https://github.com/dotnet/corefx/issues/3482#issuecomment-263007579?

[karelz]

Yep, thanks @svick! I thought it was you doing the research, just wasn't 100% sure. And kudos for monitoring entire CoreFX (I don't understand how you handle the volume :)).

[safern]

So then we should mark this as api-ready-for-review and update the proposal to be deleting one of the extensions methods.

[karelz]

I guess something like that ...

[safern]

I've updated the API proposal, so I'm marking it ready for review: https://github.com/dotnet/corefx/issues/17917#issuecomment-296093667

[terrajobst]

We believe the best course is to remove the overloads that take IDictionary<K,V> and leave IReadOnlyDictionary<K,V>. Rationale:

• The important BCL types implementing IDictionary<K,V> also implement IReadOnlyDictionary<K,V>
• It seems likely that we'll be able to change the inheritance hierarchy to make IDictionary<K,V> extend IReadOnlyDictionary<K,V>

partial class CollectionExtensions
{
  public static bool TryAdd<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key, TValue value)
  public static bool Remove<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key, out TValue value)
- public static TValue GetValueOrDefault<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key);
- public static TValue GetValueOrDefault<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key, TValue defaultValue);
  public static TValue GetValueOrDefault<TKey, TValue>(this IReadOnlyDictionary<TKey, TValue> dictionary, TKey key);
  public static TValue GetValueOrDefault<TKey, TValue>(this IReadOnlyDictionary<TKey, TValue> dictionary, TKey key, TValue defaultValue);
}

[safern]

@Pothulapati could you please follow up with this issue? Next steps:

1. Remove IDictionary<TKey, TValue>.GetValueOrDefault() extension methods from CollectionExtensions.cs: https://github.com/dotnet/corefx/blob/master/src/System.Collections/src/System/Collections/Generic/CollectionExtensions.cs#L9-#L23

2. Remove APIs from ref contract: https://github.com/dotnet/corefx/blob/master/src/System.Collections/ref/System.Collections.cs#L47-L49

3. Remove specific tests for this APIs:
   https://github.com/dotnet/corefx/blob/master/src/System.Collections/tests/Generic/CollectionExtensionsTests.cs#L12-L43

Could you do this on a PR please?

P.S: You are still missing to remove Dictionary<TKey, TValue>.GetValueOrDefault() implementation methods from coreCLR (remove this) and coreRT (remove this).

[Pothulapati]

@safern In The CoreClr Repo,If I remove the GetValueOrDefault Methods from the Dictionary,I Get The Following Error

     4>src\System\RtType.cs(1359,83): error CS1061: 'Dictionary<string, List<RuntimePropertyInfo>>' does not contain a definition for 'GetValueOrDefault' and no extension method 'GetValueOrDefault' accepting a first argument of type 'Dictionary<string, List<RuntimePropertyInfo>>' could be found (are you missing a using directive or an assembly reference?) [D:\code\coreclr\src\mscorlib\System.Private.CoreLib.csproj]
     4>src\System\Reflection\CustomAttribute.cs(2043,31): error CS1061: 'Dictionary<RuntimeType, RuntimeType>' does not contain a definition for 'GetValueOrDefault' and no extension method 'GetValueOrDefault' accepting a first argument of type 'Dictionary<RuntimeType, RuntimeType>' could be found (are you missing a using directive or an assembly reference?) [D:\code\coreclr\src\mscorlib\System.Private.CoreLib.csproj]
     4>src\System\Reflection\CustomAttribute.cs(2081,31): error CS1061: 'Dictionary<RuntimeType, RuntimeType>' does not contain a definition for 'GetValueOrDefault' and no extension method 'GetValueOrDefault' accepting a first argument of type 'Dictionary<RuntimeType, RuntimeType>' could be found (are you missing a using directive or an assembly reference?) [D:\code\coreclr\src\mscorlib\System.Private.CoreLib.csproj]
     4>src\System\Reflection\CustomAttribute.cs(2110,31): error CS1061: 'Dictionary<RuntimeType, RuntimeType>' does not contain a definition for 'GetValueOrDefault' and no extension method 'GetValueOrDefault' accepting a first argument of type 'Dictionary<RuntimeType, RuntimeType>' could be found (are you missing a using directive or an assembly reference?) [D:\code\coreclr\src\mscorlib\System.Private.CoreLib.csproj]
     4>src\System\Reflection\CustomAttribute.cs(2143,31): error CS1061: 'Dictionary<RuntimeType, RuntimeType>' does not contain a definition for 'GetValueOrDefault' and no extension method 'GetValueOrDefault' accepting a first argument of type 'Dictionary<RuntimeType, RuntimeType>' could be found (are you missing a using directive or an assembly reference?) [D:\code\coreclr\src\mscorlib\System.Private.CoreLib.csproj]
     4>src\System\Reflection\CustomAttribute.cs(2173,31): error CS1061: 'Dictionary<RuntimeType, RuntimeType>' does not contain a definition for 'GetValueOrDefault' and no extension method 'GetValueOrDefault' accepting a first argument of type 'Dictionary<RuntimeType, RuntimeType>' could be found (are you missing a using directive or an assembly reference?) [D:\code\coreclr\src\mscorlib\System.Private.CoreLib.csproj]
     4>src\System\Reflection\CustomAttribute.cs(2204,31): error CS1061: 'Dictionary<RuntimeType, RuntimeType>' does not contain a definition for 'GetValueOrDefault' and no extension method 'GetValueOrDefault' accepting a first argument of type 'Dictionary<RuntimeType, RuntimeType>' could be found (are you missing a using directive or an assembly reference?) [D:\code\coreclr\src\mscorlib\System.Private.CoreLib.csproj]
     4>src\System\Reflection\CustomAttribute.cs(2234,31): error CS1061: 'Dictionary<RuntimeType, RuntimeType>' does not contain a definition for 'GetValueOrDefault' and no extension method 'GetValueOrDefault' accepting a first argument of type 'Dictionary<RuntimeType, RuntimeType>' could be found (are you missing a using directive or an assembly reference?) [D:\code\coreclr\src\mscorlib\System.Private.CoreLib.csproj]
     4>src\System\Reflection\CustomAttribute.cs(2286,31): error CS1061: 'Dictionary<RuntimeType, RuntimeType>' does not contain a definition for 'GetValueOrDefault' and no extension method 'GetValueOrDefault' accepting a first argument of type 'Dictionary<RuntimeType, RuntimeType>' could be found (are you missing a using directive or an assembly reference?) [D:\code\coreclr\src\mscorlib\System.Private.CoreLib.csproj]
     4>src\System\Reflection\CustomAttribute.cs(2312,31): error CS1061: 'Dictionary<RuntimeType, RuntimeType>' does not contain a definition for 'GetValueOrDefault' and no extension method 'GetValueOrDefault' accepting a first argument of type 'Dictionary<RuntimeType, RuntimeType>' could be found (are you missing a using directive or an assembly reference?) [D:\code\coreclr\src\mscorlib\System.Private.CoreLib.csproj]
     4>src\System\Reflection\CustomAttribute.cs(2337,31): error CS1061: 'Dictionary<RuntimeType, RuntimeType>' does not contain a definition for 'GetValueOrDefault' and no extension method 'GetValueOrDefault' accepting a first argument of type 'Dictionary<RuntimeType, RuntimeType>' could be found (are you missing a using directive or an assembly reference?) [D:\code\coreclr\src\mscorlib\System.Private.CoreLib.csproj]
     4>src\System\Reflection\CustomAttribute.cs(2360,31): error CS1061: 'Dictionary<RuntimeType, RuntimeType>' does not contain a definition for 'GetValueOrDefault' and no extension method 'GetValueOrDefault' accepting a first argument of type 'Dictionary<RuntimeType, RuntimeType>' could be found (are you missing a using directive or an assembly reference?) [D:\code\coreclr\src\mscorlib\System.Private.CoreLib.csproj]
     4>Done Building Project "D:\code\coreclr\src\mscorlib\System.Private.CoreLib.csproj" (Build target(s)) -- FAILED.
     2>Done Building Project "D:\code\coreclr\src\build.proj" (Build target(s)) -- FAILED.
     1>Done Building Project "D:\code\coreclr\build.proj" (default targets) -- FAILED.

It's Because src\System\Reflection\CustomAttributes.cs & src\System\RtType.cs uses the Dictionary.GetValueOrDefault Methods.

[jnm2]

(@Pothulapati That would be easier to read if fenced with ``` ... ``` on new lines.)

[Pothulapati]

@safern Sorry,I mentioned that the error is from CoreRt repo(edited the comment now),The error is from the Coreclr repo.
@jnm2 Oh Yes.I was thinking the same but didn't know how to change .Thanks,:D

[safern]

Maybe we should move CollectionExtensions implementation to S.Private.Corelib because it seems like CoreRT and CoreCLR implementations use Dictionary<TKey, TValue>.GetValueOrDefault()

cc: @stephentoub @jkotas @ianhays

[weshaggard]

I would not move CollectionExtensions down. We should just update them to not use this helper.

[safern]

@Pothulapati could you just update CostumAttribute and RtType to not use Dictionary<TKey, TValue>.GetValueOrDefault on your PR?

[jkotas]

not use Dictionary<TKey, TValue>.GetValueOrDefault on your PR?

It should be replaced by ContainsKey in most places - it should result in tiny bit faster and smaller code.

[safern]

Fixed with 3 PRs: dotnet/corefx#19247, dotnet/corert#3515, dotnet/coreclr#11388

Thanks @Pothulapati for your contribution.

[karelz]

Thank you @Pothulapati!

[Pothulapati]

@karelz @safern @jkotas Great Learning experience for me.Never expected the dotnet community to be this friendly and helping.Thank You Very Much.Looking forward to contribute more.

[karelz]

Very happy to hear you liked it @Pothulapati.

If you're interested, please help us make the new contributor docs better here: https://github.com/dotnet/corefx/wiki/New-contributor-Docs#main-page-section-content (writeable by anyone for easier collaboration, hacking on it with @haralabidis for last week)

You might also find this useful & interesting: http://rion.io/2017/04/28/contributing-to-net-for-dummies/