TryAdd exposure/implementation #16642

jamesqo · 2017-03-03T00:34:29Z

This is the matching PR for dotnet/coreclr#9923.
Fixes #1942

/cc @stephentoub, @ianhays, @safern, @danmosemsft

ahsonkhan · 2017-03-03T01:53:13Z

@jamesqo, would you have to add it in coreclr as well? Or maybe just add an ApiCompatBaseline.txt (example: https://github.com/dotnet/corefx/blob/master/src/System.IO/src/ApiCompatBaseline.txt).

https://github.com/dotnet/coreclr/blob/68f72dd2587c3365a9fe74d1991f93612c3bc62a/src/mscorlib/src/System/Collections/Generic/Dictionary.cs

From CI:
D:\j\workspace\windows_nt_re---37265eab\Tools\ApiCompat.targets(53,5): error : MembersMustExist : Member 'System.Collections.Generic.Dictionary<TKey, TValue>.TryAdd(TKey, TValue)' does not exist in the implementation but it does exist in the contract. [D:\j\workspace\windows_nt_re---37265eab\src\System.Collections\src\System.Collections.csproj]
16:49:53 D:\j\workspace\windows_nt_re---37265eab\Tools\ApiCompat.targets(67,5): error : ApiCompat failed for 'D:\j\workspace\windows_nt_re---37265eab\bin\Windows_NT.AnyCPU.Release\System.Collections\netcoreapp\System.Collections.dll' [D:\j\workspace\windows_nt_re---37265eab\src\System.Collections\src\System.Collections.csproj]

jamesqo · 2017-03-03T01:56:26Z

@ahsonkhan I already made such a PR at dotnet/coreclr#9923; @danmosemsft wanted me to submit the matching tests in corefx before the coreclr implementation was merged. This PR will fail CI until that is merged.

ahsonkhan · 2017-03-03T01:58:50Z

Got it. Thanks for clarifying :)

karelz · 2017-03-03T03:39:13Z

@jamesqo I added "Fixes #1942" to top post to enable auto-closing upon merge. Please shout out if it is wrong.

TylerBrinkley · 2017-03-03T21:19:10Z

Just curious, where does the XML documentation go for this new method?

safern

LGTM. When we ingest the new coreclr with your changes you'll need to update the uapaot baseline file so that you don't get any failures in CI in the Build for all configurations.

You can do that by running:

msbuild src\System.Collections\src\System.Collections.csproj /p:BaselineAllApiCompatError=true /p:TargetGroup=uapaot

Thanks @jamesqo

safern · 2017-03-03T21:24:52Z

src/System.Collections/tests/Generic/Dictionary/Dictionary.Generic.Tests.netcoreapp.cs

@@ -33,5 +65,4 @@ public void Dictionary_Generic_Constructor_IEnumerable_IEqualityComparer(int cou
            Assert.Equal(collection, copied);
        }
    }
-
 }


nit: end of line

jamesqo · 2017-03-03T22:42:43Z

@TylerBrinkley

Just curious, where does the XML documentation go for this new method?

I'm not sure because I don't work at Microsoft. @karelz Would you know?

karelz · 2017-03-03T22:59:28Z

@TylerBrinkley do you mean /// comments docs?

TylerBrinkley · 2017-03-03T23:04:41Z

@karelz yes.

karelz · 2017-03-03T23:13:28Z

I poked at the topic in December. The story is not 100% clear yet (or rather I didn't quite like the story I heard and I plan to poke at it more ;-)).
There is repo for API docs - for all .NET platforms. How it is going to be correlated with /// comments in CoreFX repo is not yet finalized (One time sync or continuous sync? One way or both ways?).
I expect the discussion to restart by end of March (some prereq docs tooling will be in place by then).

So, that's where we are. Does it help? Do you have any immediate concerns or questions?

TylerBrinkley · 2017-03-03T23:19:24Z

@karelz, does that mean new APIs provided in .NET Core implemented in CoreCLR repo will not have XML comments documentation for the time being as those APIs are not in Net FX or Net Standard?

karelz · 2017-03-03T23:37:30Z

@TylerBrinkley I assume that at certain point (before shipping) we will create the documentation and also add it into IntelliSense.
It is not clear when it will happen and what will be the source of the IntelliSense docs (/// comments in source code, pre-populated from Desktop? Or the full API documentation in the docs repo?)

danmoseley · 2017-03-06T18:45:23Z

I merged the implementation so we should be able to merge this in a day or so if it gets okayed

stephentoub · 2017-03-06T18:47:03Z

src/System.Collections/tests/Generic/Dictionary/Dictionary.Generic.Tests.netcoreapp.cs

+            TKey key = CreateTKey(seed: count);
+            TValue value = dictionary.ContainsKey(key) ? dictionary[key] : default(TValue);
+            int originalCount = dictionary.Count;
+            bool alreadyContainsKey = dictionary.ContainsKey(key);


Why call ContainsKey(key) twice?

Now that GetValueOrDefault is in I no longer have to 😄

What does it have to do with GetValueOrDefault? Why did you ever need to call it twice?

You could do:

bool alreadyContainsKey = dictionary.ContainsKey(key); TValue value = alreadyContainsKey ? dictionary[key] : default(TValue); ... Assert.Equal(!alreadyContainsKey, dictionary.TryAdd(key, default(TValue)));

I'm with @stephentoub that there is no need in calling ContainsKey(key) twice.

@stephentoub

What does it have to do with GetValueOrDefault?

TValue value = dictionary.GetValueOrDefault(key)

Why did you ever need to call it twice?

I didn't think it mattered because this is test code. Are you concerned about code duplication in calling the method twice?

Are you concerned about code duplication in calling the method twice?

That it's confusing seeing it called twice when it's not necessary.

@stephentoub Ok. Will try to keep in mind for later PRs

stephentoub · 2017-03-06T18:48:38Z

src/System.Collections/tests/Generic/Dictionary/Dictionary.Generic.Tests.netcoreapp.cs

+            Assert.Equal(alreadyContainsKey ? originalCount : originalCount + 1, dictionary.Count);
+            Assert.Equal(dictionary[key], value);
+
+            if (!dictionary.Any())


In what situation will Any here return false?

Was a logical mistake on my part. Updated

stephentoub · 2017-03-06T18:49:34Z

src/System.Collections/tests/Generic/Dictionary/Dictionary.Generic.Tests.netcoreapp.cs

+                return;
+            }
+
+            // It's unlikely that randomly generated data will already be in the dictionary,


I don't understand this comment... we already added key to the dictionary, so don't we know it's in there?

Good idea, I should have thought of simply using the key that was added instead of finding a new key.

danmoseley · 2017-03-06T19:01:31Z

As an aside @jamesqo if you want you could follow up with issue(s) for places you believe it would be worthwhile for us to start using this API.

safern · 2017-03-07T19:36:48Z

Coreclr should now be updated to have your changes. So you can move forward on updating this one @jamesqo

jamesqo · 2017-03-07T23:22:56Z

@safern I am busy as of late, so I might not have time to respond to @stephentoub's feedback and fix the conflicts for a day or so.

safern · 2017-03-08T23:32:16Z

@jamesqo the failures in CI are because the new API is not implemented in uap yet but it is in the contract. To fix that you need to do a baseline by running:
msbuild src\System.Collections\src\System.Collections.csproj /p:BaselineAllApiCompatError=true /p:TargetGroup=uapaot

and update the PR with the changes to the ApiCompatBaseline.uapaot.txt file.

jamesqo · 2017-03-08T23:36:00Z

@safern Will the failures be fixed if I submit a PR to the corert Dictionary?

safern · 2017-03-08T23:38:47Z

Will the failures be fixed if I submit a PR to the corert Dictionary?

@jamesqo I'm not sure about that.

cc: @joperezr that can answer that.

danmoseley · 2017-03-08T23:48:08Z

I think the answer is yes please do, but meantime update the baseline to unblock this PR.

safern · 2017-03-08T23:50:23Z

+1.

@jamesqo please update the baseline so that we can unblock this PR and not wait to another PR in other repo and when you do the PR in CoreRT and it is ingested then we can update the baseline.

jamesqo · 2017-03-08T23:50:50Z

@danmosemsft I have already submitted a corert PR, actually: dotnet/corert#2926 How long does it take for the corert build to get updated? (/cc @jkotas) It'd be preferable to avoid adding more changes and submitting another PR to delete them later.

jamesqo · 2017-03-08T23:53:56Z

@safern You posted just before I did... how long does it take for the corert build to update? @jkotas usually merges the corert PRs really quickly because they're straight ports from coreclr, so I don't think this should block on that.

danmoseley · 2017-03-09T00:03:55Z

@jamesqo after commit to CoreRT I believe they have to get ported to our internal "Project N" repo (where we do UWP runtime work) after which a bot puts up a PR like #16866

The baselines don't trigger an error if they have unnecessary lines, so there'll be no need to remove the excess entries later -- it will happen sooner or later -- and that's why we don't generally wait for such codeflow.

joperezr · 2017-03-09T00:39:58Z

Will the failures be fixed if I submit a PR to the corert Dictionary?

Seems like it has been answered already, but you do want to update the baseline as part of this PR since even if you have the corert change merged, it will take some time (usually around a day) for ProjectN to produce a new Targeting pack and then for us to ingest it. Once we do that, we can update the baseline that you will change here.

stephentoub · 2017-03-10T14:09:44Z

src/System.Collections/tests/Generic/Dictionary/Dictionary.Generic.Tests.netcoreapp.cs

+            TKey key = CreateTKey(seed: count);
+            TValue value = dictionary.GetValueOrDefault(key);
+            int originalCount = dictionary.Count;
+            bool alreadyContainsKey = dictionary.ContainsKey(key);


In what situation will this already contain the key? And isn't / shouldn't this be deterministic? Seems like the test should either assert that it contains the key or assert that it doesn't, based on the test's construction, rather than having to account for the possibility that it may or may not.

Even if you can't predict whether GenericIDictionaryFactory(count) will include the relevant key, we can still set up the test to ensure the key isn't there, e.g.

var dictionary = (Dictionary<TKey, TValue>)GenericIDictionaryFactory(count); TKey key = CreateTKey(seed: count); dictionary.Remove(key); int originalCount = dictionary.Count; TValue value = CreateTValue(seed: count); Assert.True(dictionary.TryAdd(key, value)); Assert.Equal(originalCount + 1, dictionary.Count); Assert.Equal(dictionary[key], value); Assert.False(dictionary.TryAdd(key, default(TValue))); Assert.Equal(originalCount + 1, dictionary.Count); Assert.Equal(dictionary[key], value);

You're right, it's probably not wise to have code that we're unsure will run floating around. Updated.

stephentoub · 2017-03-12T23:45:05Z

LGTM. Thanks, @jamesqo.

* Exposure/implementation of Dictionary.TryAdd Commit migrated from dotnet/corefx@fa66c2e

dnfclas added the cla-already-signed label Mar 3, 2017

karelz added the area-System.Collections label Mar 3, 2017

karelz assigned jamesqo, ianhays and safern Mar 3, 2017

safern reviewed Mar 3, 2017

View reviewed changes

jkotas mentioned this pull request Mar 6, 2017

Implementation for Dictionary.TryAdd dotnet/coreclr#9923

Merged

stephentoub reviewed Mar 6, 2017

View reviewed changes

jamesqo added 2 commits March 8, 2017 17:23

Exposure/implementation of Dictionary.TryAdd

50c73aa

Respond to PR feedback

e9ba0ff

jamesqo force-pushed the try.add.tests branch from 16f2673 to e9ba0ff Compare March 8, 2017 22:39

stephentoub reviewed Mar 10, 2017

View reviewed changes

Respond to PR feedback

51040d1

stephentoub merged commit fa66c2e into dotnet:master Mar 12, 2017

jamesqo deleted the try.add.tests branch March 12, 2017 23:48

karelz modified the milestone: 2.0.0 Mar 15, 2017

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022

TryAdd exposure/implementation (dotnet/corefx#16642)

4280fe6

* Exposure/implementation of Dictionary.TryAdd Commit migrated from dotnet/corefx@fa66c2e

TryAdd exposure/implementation #16642

TryAdd exposure/implementation #16642

Conversation

jamesqo commented Mar 3, 2017 • edited by karelz Loading

ahsonkhan commented Mar 3, 2017

jamesqo commented Mar 3, 2017

ahsonkhan commented Mar 3, 2017

karelz commented Mar 3, 2017

TylerBrinkley commented Mar 3, 2017

safern left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesqo commented Mar 3, 2017

karelz commented Mar 3, 2017

TylerBrinkley commented Mar 3, 2017

karelz commented Mar 3, 2017 • edited Loading

TylerBrinkley commented Mar 3, 2017

karelz commented Mar 3, 2017

danmoseley commented Mar 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Mar 6, 2017

safern commented Mar 7, 2017 • edited Loading

jamesqo commented Mar 7, 2017 • edited Loading

safern commented Mar 8, 2017 • edited Loading

jamesqo commented Mar 8, 2017

safern commented Mar 8, 2017

danmoseley commented Mar 8, 2017

safern commented Mar 8, 2017

jamesqo commented Mar 8, 2017

jamesqo commented Mar 8, 2017

danmoseley commented Mar 9, 2017

joperezr commented Mar 9, 2017

Choose a reason for hiding this comment

jamesqo Mar 12, 2017 • edited Loading

Choose a reason for hiding this comment

stephentoub commented Mar 12, 2017

jamesqo commented Mar 3, 2017 •

edited by karelz

Loading

karelz commented Mar 3, 2017 •

edited

Loading

safern commented Mar 7, 2017 •

edited

Loading

jamesqo commented Mar 7, 2017 •

edited

Loading

safern commented Mar 8, 2017 •

edited

Loading

jamesqo Mar 12, 2017 •

edited

Loading