Add debug visualiser for 'dict' and implement IReadOnlyDictionary/IReadOnlyCollection #3988

saul · 2017-11-20T21:57:44Z

saul · 2017-11-21T11:01:31Z

@cartermp this can be merged

dhwed · 2017-11-21T15:06:44Z

While you're at it, can you also implement IReadOnlyDictionary<'Key,'T>?

cartermp · 2017-11-22T01:46:44Z

@saul @0x53A does this work with the following program?

[<EntryPoint>]
let main argv = 
    let d = dict [("uh",1);("uhhh",2);("uhhhhhhhhhh",3)]
    let d2 = dict [("hello", 15); ("Wow", 10)]
    (d, d2) |> ignore // breakpoint here
    0 // return an integer exit code

Perhaps it's environmental, but when I install this PR into VS 15.5 Preview 4 I still see the old behavior.

saul · 2017-11-22T07:54:49Z

Can’t test it at the moment - my screenshot is from manually linking the new FSharp.Core into my project.

0x53A · 2017-11-22T16:20:56Z

@cartermp I've downloaded FSharp.Core.dll from https://ci2.dot.net/job/Microsoft_visualfsharp/job/master/job/release_net40_no_vs_windows_nt_prtest/4613/artifact/release/net40/bin/

And it looks really good:

Since this is part of FSharp.Core, not the VF#Tools, this would need a new nuget release.

cartermp · 2017-11-22T16:30:12Z

I did a git clean -xdfsmorgasburg, rebuilt things, and it all works. 👍

cartermp · 2017-11-22T16:38:52Z

@dhwed For IReadOnlyDictionary, I've created a tracking issue: #3999. I'd rather that be done separately from this PR, but if @saul is open to quickly doing it or accepting a PR into his branch from you, it won't be the end of the world if it comes in through this PR.

forki · 2017-11-25T07:02:38Z

src/fsharp/FSharp.Core/fslib-extra-pervasives.fs

+        [<DebuggerBrowsable(DebuggerBrowsableState.RootHidden)>]
+        member x.Items = Array.ofSeq d
+
+    let dictImpl (comparer:IEqualityComparer<'SafeKey>) (makeSafeKey : 'Key->'SafeKey) (getKey : 'SafeKey->'Key) (l:seq<'Key*'T>) : IDictionary<'Key,'T> =


No longer inlined.

Good catch, fixed

saul · 2017-11-25T12:30:30Z

Implemented IReadOnlyDictionary and IReadOnlyCollection - I'll go around adding them to list, Map and Set (for a separate PR).

cartermp · 2017-11-25T22:50:08Z

fsharp/fslang-design#234

dsyme

Please

fix bug in TryGetValue mentioned above
add testing for all of IReadOnlyDictionary and IReadOnlyCollection,
review all existing testing for dict interfaces

thanks!

dsyme · 2017-11-27T17:01:26Z

src/fsharp/FSharp.Core/fslib-extra-pervasives.fs

+        interface IReadOnlyDictionary<'Key, 'T> with
+            member __.Item with get key = t.[makeSafeKey key]
+            member __.Keys = t.Keys |> Seq.map getKey
+            member __.TryGetValue(key, value) = t.TryGetValue(makeSafeKey key, ref value)


ref value is incorrect here - please use &value. Please also add a test for this and review all testing as there must be a test hole here.

Ref seems to work fine for me? I've added tests to validate it - I'll use the & instead though.

It is my understanding that ref will not copy-out the resulting value to the byref passed as an argument

saul · 2017-11-27T21:59:10Z

@dsyme there's a bit of a hiccup - dict currently returns IDictionary - this doesn't implement IReadOnly* interfaces. We can either create a new function called something like readOnlyDict that returns an IReadOnlyDictionary, or we can change dict to return some type in FSharp.Core that implements IDictionary and IReadOnly*, but I think that would break backwards compat.

How do you want to proceed?

dsyme · 2017-11-27T23:04:28Z

@dsyme there's a bit of a hiccup - dict currently returns IDictionary - this doesn't implement IReadOnly* interfaces. We can either create a new function called something like readOnlyDict that returns an IReadOnlyDictionary, or we can change dict to return some type in FSharp.Core that implements IDictionary and IReadOnly*, but I think that would break backwards compat.

My assumption was that for now the return type should be only IDictionary and that would allow the change to be integrated straight away.

If you want to add readOnlyDict then please either add that to the RFC or make a new RFC and lets discuss there? I've no real issue with adding it but new API should always have multiple eyes and an RFC

saul · 2017-11-28T07:44:39Z

Makes sense - will update the RFC tonight and use this PR as its implementation.

KevinRansom · 2017-12-15T20:22:26Z

@dsyme, @saul saul ... is this ready? It looks like you agreed to a change, but I don't see the change.

Thanks

Kevin

saul · 2017-12-17T10:47:11Z

@KevinRansom it's not quite ready yet - I started writing tests and fell down the rabbit hole of adding support for IEnumerable.Reset on seq {} enumerators. I'll see if I can make this change without that 😄

saul · 2017-12-17T11:24:22Z

Pushed tests and new readOnlyDict top-level operator. Discussion to continue in fsharp/fslang-suggestions#624 and then the RFC will need updating, too.

…ug-visualiser

saul · 2017-12-18T09:46:23Z

Hmm not sure why the CI is failing... I've added that file to source control? cc @KevinRansom

cloudRoutine · 2017-12-18T11:31:25Z

@saul what do you think of creating an additional project just for debugger visualizers? (not necessarily in this PR, but something to consider for the future)
The vsix packaging would install the dll of the visualizers to the right directory and vs should auto-load it
Although I'm not sure if these visualizers would show up in C# as well, but if we only write custom visualizers for F# types it'll probably be fine.

saul · 2017-12-18T12:22:25Z

Visualizer in that sense is completely different to debug type proxies that are used in FSharp.Core.

dsyme · 2017-12-19T14:17:29Z

Yes, I still believe this line is buggy: https://github.com/Microsoft/visualfsharp/pull/3988/files#diff-cf1d05f204f61f31020312fcec812dbaR84. That ref is reading the provided byref argument and creating a new ref cell, and never writing the result back to the byref argument.

saul · 2017-12-19T19:24:49Z

Yeah @dsyme I’m happy to change it, but the TryGetValue tests pass (if you try it locally, the CI is failing)

0x53A · 2017-12-19T20:32:11Z

...sharp/FSharp.Core.Unittests/FSharp.Core/Microsoft.FSharp.Core/ExtraTopLevelOperatorsTests.fs

+        Assert.AreEqual(irod.Values, [| 1; 4; 9|])
+
+        Assert.IsTrue(irod.TryGetValue(1, ref 1))
+        Assert.IsFalse(irod.TryGetValue(100, ref 1))


As I read it, the test only verifies that the key is found, not that the correct value is returned. That may explain why it passes even though @dsyme is sure that the ref is incorrect.

A better test may be

let b,i = irod.TryGetValue(2) Assert.AreEqual(true, b) Assert.AreEqual(2, i)

cartermp · 2018-02-05T01:08:30Z

(For posterity, since the jenkins servers to wipe results after a time)

11:42:56 Errors and Failures
11:42:56 
11:42:56 1) Failed : FSharp.Core.UnitTests.FSharp_Core.Microsoft_FSharp_Core.DictTests.IReadOnlyDictionary on readOnlyDict
11:42:56   Expected: 4
11:42:56   But was:  0
11:42:56 at FSharp.Core.UnitTests.FSharp_Core.Microsoft_FSharp_Core.DictTests.IReadOnlyDictionary on readOnlyDict() in D:\j\w\release_ci_pa---866fd2c3\tests\FSharp.Core.UnitTests\FSharp.Core\Microsoft.FSharp.Core\ExtraTopLevelOperatorsTests.fs:line 116
11:42:56 
11:42:56 2) Failed : FSharp.Core.UnitTests.SurfaceArea.SurfaceAreaTest.VerifyArea
11:42:56 Assembly: FSharp.Core, Version=4.4.1.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a
11:42:56 Expected and actual surface area don't match. To see the delta, run:
11:42:56     windiff D:\j\w\release_ci_pa---866fd2c3\tests\FSharp.Core.UnitTests\SurfaceArea.net40.fs D:\j\w\release_ci_pa---866fd2c3\release\net40\bin\CoreUnit_net40_Xml.xml
11:42:56 
11:42:56 Unexpectedly missing (expected, not actual):
11:42:56 
11:42:56 Unexpectedly present (actual, not expected):
11:42:56     Microsoft.FSharp.Core.ExtraTopLevelOperators: System.Collections.Generic.IReadOnlyDictionary`2[TKey,TValue] CreateReadOnlyDictionary[TKey,TValue](System.Collections.Generic.IEnumerable`1[System.Tuple`2[TKey,TValue]])
11:42:56 at FSharp.Core.UnitTests.LibraryTestFx.SurfaceArea.verify(String expected, String platform, String fileName) in D:\j\w\release_ci_pa---866fd2c3\tests\FSharp.Core.UnitTests\LibraryTestFx.fs:line 142
11:42:56 
11:42:56 Run Settings
11:42:56     RuntimeFramework: V4.0
11:42:56     WorkDirectory: D:\j\w\release_ci_pa---866fd2c3\release\net40\bin
11:42:56     Verbose: True
11:42:56     NumberOfTestWorkers: 4
11:42:56 
11:42:56 Test Run Summary
11:42:56     Overall result: Failed
11:42:56    Tests run: 5344, Passed: 5342, Errors: 0, Failures: 2, Inconclusive: 0
11:42:56      Not run: 3, Invalid: 0, Ignored: 0, Explicit: 3, Skipped: 0
11:42:56   Start time: 2018-02-02 19:36:10Z
11:42:56     End time: 2018-02-02 19:41:41Z
11:42:56     Duration: 330.699 seconds
11:42:56 
11:42:56 Results (nunit3) saved as D:\j\w\release_ci_pa---866fd2c3\tests\TestResults\test-net40-coreunit-results.xml

saul · 2018-02-05T07:30:44Z

At least it builds now :) the problem here is that seq computation expressions / Seq.map etc don’t support IEnumerable.Reset(). I just need to refactor the tests keeping this in mind

saul · 2018-02-05T07:33:19Z

Scratch that, no it’s not. It’s the TryGetValue issue dsyme raised and the surface area change.

saul · 2018-02-18T10:43:14Z

Okay, I think I've fixed the broken tests here. Sorry for the delay - my laptop makes it impossible to do dev work and I've just fixed my workstation after 3 months of it being broken 😄

saul · 2018-02-18T12:32:01Z

@dotnet-bot test Ubuntu14.04 Release_default please

cartermp · 2018-02-18T15:13:32Z

Sorry for the delay

No worries at all!

cartermp · 2018-02-18T15:13:57Z

@dsyme can you re-review? Thanks!

dsyme · 2018-02-21T14:38:44Z

LGTM

cartermp · 2018-03-14T17:16:28Z

@dotnet-bot test Windows_NT Release_ci_part3 Build please

KevinRansom · 2018-03-14T23:19:37Z

The issue is this:

an unexpected new public API:
Microsoft.FSharp.Core.ExtraTopLevelOperators: System.Collections.Generic.IReadOnlyDictionary2[TKey,TValue] CreateReadOnlyDictionary[TKey,TValue](System.Collections.Generic.IEnumerable1[System.Tuple`2[TKey,TValue]])

1) Failed : FSharp.Core.UnitTests.Portable.SurfaceArea.SurfaceAreaTest.VerifyArea
Assembly: FSharp.Core, Version=4.4.3.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a
Expected and actual surface area don't match. To see the delta, run:
    windiff D:\j\w\release_ci_pa---c6929ad7\tests\FSharp.Core.UnitTests\SurfaceArea.coreclr.fs \My Documents\CoreUnit_coreclr_Xml.xml

Unexpectedly missing (expected, not actual):

Unexpectedly present (actual, not expected):
    Microsoft.FSharp.Core.ExtraTopLevelOperators: System.Collections.Generic.IReadOnlyDictionary`2[TKey,TValue] CreateReadOnlyDictionary[TKey,TValue](System.Collections.Generic.IEnumerable`1[System.Tuple`2[TKey,TValue]])
at FSharp.Core.UnitTests.LibraryTestFx.SurfaceArea.verify(String expected, String platform, String fileName) in D:\j\w\release_ci_pa---c6929ad7\tests\FSharp.Core.UnitTests\LibraryTestFx.fs:line 171

Run Settings
    Internal Trace: Off

Test Run Summary
  Overall result: Failed
  Test Count: 5389, Passed: 5384, Failed: 1, Inconclusive: 0, Skipped: 4
    Failed Tests - Failures: 1, Errors: 0, Invalid: 0
    Skipped Tests - Ignored: 2, Explicit: 2, Other: 0
  Start time: 2018-03-14 17:52:10Z
    End time: 2018-03-14 17:56:18Z
    Duration: 248.537 seconds

KevinRansom · 2018-03-17T07:28:00Z

@dotnet-bot test this please

…aul/visualfsharp into 3981-add-dict-debug-visualiser

saul · 2018-04-08T17:28:14Z

@KevinRansom @cartermp this is ready to merge

KevinRansom · 2018-04-09T06:52:13Z

@saul

Thanks for this.

Kevin

Add debug visualiser for 'dict'

94ea46d

cartermp approved these changes Nov 20, 2017

View reviewed changes

forki approved these changes Nov 25, 2017

View reviewed changes

Review comments + implement IReadOnlyDictionary and IReadOnlyCollection

7ea473e

saul changed the title ~~Add debug visualiser for 'dict'~~ Add debug visualiser for 'dict' and implement IReadOnlyDictionary/IReadOnlyCollection Nov 25, 2017

cartermp mentioned this pull request Nov 25, 2017

Create FS-1041-ireadonly.md fsharp/fslang-design#234

Merged

dsyme requested changes Nov 27, 2017

View reviewed changes

saul mentioned this pull request Dec 1, 2017

[FS-1041] Add IReadOnlyCollection/List/Dictionary implementations to FSharp.Core collections #4014

Merged

cartermp mentioned this pull request Dec 5, 2017

Update references and versions to allow for us to take new FSharp.Core changes #4061

Closed

Add readOnlyDict to ExtraTopLevelOperators

5fa181e

Merge remote-tracking branch 'upstream/master' into 3981-add-dict-deb…

51cbb59

…ug-visualiser

0x53A reviewed Dec 19, 2017

View reviewed changes

Move extra top level operator tests

f584b3f

saul added 2 commits February 18, 2018 10:12

Merge branch 'master' into 3981-add-dict-debug-visualiser

5e1969d

Fix broken tests

240f5d5

dsyme approved these changes Feb 21, 2018

View reviewed changes

Merge branch 'master' into 3981-add-dict-debug-visualiser

8e217f4

saul added 2 commits March 17, 2018 10:49

Add CreateReadOnlyDictionary to coreclr surface area

9b575dd

Merge branch '3981-add-dict-debug-visualiser' of https://github.com/s…

ba5866e

…aul/visualfsharp into 3981-add-dict-debug-visualiser

KevinRansom merged commit 2ccc1b3 into dotnet:master Apr 9, 2018

Add debug visualiser for 'dict' and implement IReadOnlyDictionary/IReadOnlyCollection #3988

Add debug visualiser for 'dict' and implement IReadOnlyDictionary/IReadOnlyCollection #3988

Uh oh!

Conversation

saul commented Nov 20, 2017

Uh oh!

saul commented Nov 21, 2017

Uh oh!

dhwed commented Nov 21, 2017

Uh oh!

cartermp commented Nov 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saul commented Nov 22, 2017

Uh oh!

0x53A commented Nov 22, 2017

Uh oh!

cartermp commented Nov 22, 2017

Uh oh!

cartermp commented Nov 22, 2017

Uh oh!

forki Nov 25, 2017

Choose a reason for hiding this comment

Uh oh!

forki Nov 25, 2017

Choose a reason for hiding this comment

Uh oh!

saul Nov 25, 2017

Choose a reason for hiding this comment

Uh oh!

saul commented Nov 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cartermp commented Nov 25, 2017

Uh oh!

dsyme left a comment

Choose a reason for hiding this comment

Uh oh!

dsyme Nov 27, 2017

Choose a reason for hiding this comment

Uh oh!

saul Nov 27, 2017

Choose a reason for hiding this comment

Uh oh!

dsyme Nov 27, 2017

Choose a reason for hiding this comment

Uh oh!

saul commented Nov 27, 2017

Uh oh!

dsyme commented Nov 27, 2017

Uh oh!

saul commented Nov 28, 2017

Uh oh!

KevinRansom commented Dec 15, 2017

Uh oh!

saul commented Dec 17, 2017

Uh oh!

saul commented Dec 17, 2017

Uh oh!

saul commented Dec 18, 2017

Uh oh!

cloudRoutine commented Dec 18, 2017

Uh oh!

saul commented Dec 18, 2017

Uh oh!

dsyme commented Dec 19, 2017

Uh oh!

saul commented Dec 19, 2017

Uh oh!

0x53A Dec 19, 2017

Choose a reason for hiding this comment

Uh oh!

cartermp commented Feb 5, 2018

Uh oh!

saul commented Feb 5, 2018

Uh oh!

saul commented Feb 5, 2018

Uh oh!

saul commented Feb 18, 2018

Uh oh!

cartermp commented Nov 22, 2017 •

edited

Loading

saul commented Nov 25, 2017 •

edited

Loading