Dictionary.GetValueOrDefault

[AlexRadch]

With dictionaries of class-types it is often desirable and expected to return null for a non-existent key.
Close https://github.com/dotnet/corefx/issues/3482

[justinvp]

Linq's extension methods throw ArgumentNullException when the source is null. I think we should do the same for these extension methods, throwing ArgumentNullException if the dictionary is null.

[AlexRadch]

I fixed this

[justinvp]

This is going to lookup the key twice. Consider something like:

TValue value;
return dictionary.TryGetValue(key, out value) ? value : defaultValue;

[AlexRadch]

Interface IDictionary does not have TryGetValue method

[justinvp]

IDictionary<TKey, TValue> has TryGetValue.

https://github.com/dotnet/coreclr/blob/f25bff4d1645d9e5f03c4810eaf141f8f245f563/src/mscorlib/src/System/Collections/Generic/IDictionary.cs#L56

[justinvp]

Dictionary in the coreclr repo will need to be updated as well.

[justinvp]

The APIs will need to be exposed in the contracts by adding them to src/System.Collections/ref/System.Collections.cs within #if netcoreapp11 (see other examples in that file).

There should also be tests added as part of this. The tests for the Dictionary<TKey, TValue> change won't work until the CoreCLR change has been merged and propagated.

[AlexRadch]

@justinvp I tried expose API to ref, but after that project can not be compiled. I do not know how to compile it with new contract. Can you help me to compile it?

[ianhays]

@AlexRadch this PR should be in coreclr. The Dictionary class in CoreFX isn't used for netcoreapp builds, it's only there for building UAP.

Sorry for the confusion about this. In the future you can figure out what we're actually taking from corefx and what we're taking from coreclr by looking at the csproj and seeing what TargetGroup the file is included under.

I'd suggest you just close this out and reopen it against the coreclr repo. Then, once that merges, you can open another PR in CoreFX adding the new methods to the ref file and adding tests.

[justinvp]

@ianhays, the CollectionExtensions part of the change should still go in CoreFX. Once the Dictionary change in dotnet/coreclr#8641 is merged and propagated to CoreFX, would it make sense to continue with this PR which would: add CollectionExtensions, include the Dictionary change for UAP, add the new APIs to the contracts, and add tests?

[AlexRadch]

@ianhays @justinvp thanks for answer.

[ianhays]

Once the Dictionary change in dotnet/coreclr#8641 is merged and propagated to CoreFX

Thanks @justinvp, I didn't know there was already a coreclr PR. You're correct on the order of operations.

@justinvp I tried expose API to ref, but after that project can not be compiled. I do not know how to compile it with new contract. Can you help me to compile it?

It won't compile until you merge the coreclr PR and CoreFX is updated with a new version of mscorlib that contains your coreclr changes.

[ianhays]

This can be implemented in terms of the other overload to prevent code duplication. Same for the IDictionary method.

[AlexRadch]

@ianhays I do not understand your suggestion.

[ianhays]

You already did it. I was talking about doing this:

public static TValue GetValueOrDefault<TKey, TValue>(this IDictionary<TKey, TValue> dictionary, TKey key) => dictionary.GetValueOrDefault(key, default(TValue));

[AlexRadch]

I understand your suggestion after wrote my comment and I changed code to remove duplication.

[ianhays]

Same comments from the coreclr PR apply here as well.

[ianhays]

Are there tests for this? You should be able to use the same ones as TryGetValue.

I had a few comments, but it mostly looks good to me. Thanks Alex. We'll need to wait a few days to consume the coreclr changes after they merge.

[ianhays]

Source changes LGTM once we have tests.

[danmoseley]

@dotnet-bot test this (updated clr)

[danmoseley]

Trying to trigger retest.

[karelz]

@AlexRadch do you plan to finish the work?

[karelz]

The PR seems to be abandoned for 1 month. Closing it for now.