[FS-1041] Add System.Collections.ICollection implementations to F# set/map

[jack-pappas]

I've implemented fsharp/fslang-suggestions#473 for the Set<'T> and Map<'Key,'Value> types in FSharp.Core; that is, I've implemented the non-generic ICollection interface on both types, along with some others (e.g., IDictionary, IReadOnlyCollection<'T>) which also seemed relevant to implement.

This PR does not include adding ICollection to 'T list -- I'll tackle that in a separate PR.

This is still a work in progress -- I've finished the implementation (as far as it seems reasonable and appears to work as expected) but I'd like to implement more unit tests for the new interfaces to verify the correctness of their behavior (and also that the behavior matches other .NET collection types as closely as possible).

[KevinRansom]

I have an emotional problem with these kind of partial implementations of contracts. it has always been one of my feefs with dotnet that it was so easy to do this kind of thing.. Throwing not implemented on an object that claims to implement a contract is .... well ... lame. Having said that adding mutability to map would have been worse.

I really have to wonder what the use case for this feature is. I could imagine implementing IReadOnlyDictionary and IReadOnlyCollection for interop purposes though.

[smoothdeveloper]

@KevinRansom considering the class already implements IDictionary<K,V> the same way (throwing on set value, etc.) I think this is reasonable addition, most people will know that passing a map to a place expecting a dictionary, only lookup / enumeration scenarios would be expected to work.

I've been facing situations where a type would implement IDictionary<K,V> but I'd actually needed the older interface to pass to some code, and doing the wrapper myself felt absurd.

The IReadOnly* stuff sadly came late in the game, it will take time for it to be used more readily where it should be used.

[jack-pappas]

@KevinRansom I agree, the partial contract implementations aren't ideal; in this case though, we're just adding (for the most part) the non-generic versions of interfaces already implemented by Set<_> and Map<_,_>.

@eiriktsarpalis gave some good reasons to justify implementing the non-generic versions of these interfaces in his suggestion (fsharp/fslang-suggestions#473).

[smoothdeveloper]

Good initiative!

[smoothdeveloper]

Can we somehow rely on the same member .Keys and .Values which are implemented above instead of duplicating the code?

maybe make an inline function to be used in both places instead?

[jack-pappas]

This seems like a good idea, I'll work on it and update the PR.

[saul]

If we're just casting to IEnumerable, then we should just be doing seq { for kvp in s -> kvp.Key }. I'm not sure whether that is faster or s |> Seq.map (fun kvp -> kvp.Key) is.

Same goes for values.

[smoothdeveloper]

@KevinRansom considering the class already implements IDictionary<K,V> the same way (throwing on set value, etc.) I think this is reasonable addition, most people will know that passing a map to a place expecting a dictionary, only lookup / enumeration scenarios would be expected to work.

I've been facing situations where a type would implement IDictionary<K,V> but I'd actually needed the older interface to pass to some code, and doing the wrapper myself felt absurd.

The IReadOnly* stuff sadly came late in the game, it will take time for it to be used more readily where it should be used.

[KevinRansom]

@dotnet-bot test this please

[jack-pappas]

@KevinRansom I fixed a syntax error and a failing unit test (it was a new test which I hadn't implemented correctly), and the builds look better now. I'm not sure why the ci_part2 build failed; the errors look unrelated to the new code here, unless the printing code (which is failing) is casting map/set instances to some interface and the behavior has been changed by adding these new interfaces.

I'm going to leave the [WIP] tag for now though; even if this code passes all existing tests, I'd like to implement some new unit tests to nail down the behavior of the new interfaces before merging it -- once the code ships, it'll be harder/infeasible to change the behavior.

[jack-pappas]

It looks like there is a difference in output caused by the changes in this PR, which affect the way FSI prints instances of Map<_,_>. I haven't been able to confirm (e.g., by attaching a debugger to a running fsi instance), but it looks like this is the offending code:
https://github.com/Microsoft/visualfsharp/blob/901923e755a39fb6b65116a8f5da028bd22d18b7/src/fsharp/fsi/fsi.fs#L245

The Map<_,_> type supports structured formatting, so it seems like that should take precedence over this default printer in FSI; in other words, FSI should apply this to only those IDictionary instances where the backing type doesn't implement structured formatting.

[jack-pappas]

Turned out that Map and Set didn't really support structured formatting -- they returned a structural-like formatted string from their ToString() overrides. I've moved those implementations into a property (which is now called by ToString() to preserve the same behavior). This has fixed the tests for printing IDictionary instances but broken those for printing map and set with the printfn "%A" formatting. It looks like that's caused by this special handling for Map and Set:
https://github.com/Microsoft/visualfsharp/blob/master/src/utils/sformat.fs#L1226

[jack-pappas]

@dsyme Any thoughts/suggestions on how best to resolve the remaining behavioral differences in printing which are causing the test failures? I've spot-checked some of the differences, and it looks like the main difference is that sformat prints more of the map/set elements than their ToString() overrides (which are now also used as the basis for their [<StructuredFormatDisplay(...)>] implementations).

One idea I had was to move the formatting code from sformat into new, private methods in the Map<_,_> and Set<_> types, where the methods would have a parameter indicating the max width / number of elements to print; then this could be used by both the ToString() overrides and the new .StructuredFormat members to produce the formatted output (but where they'd use different max. number of elements to print, to preserve the current behavior).

[KevinRansom]

@jack-pappas

there seem to be issues, is this still an active PR?

It should be noted that as a corlib Api change this will probably be for F# 4.2

[jack-pappas]

@KevinRansom Yes, this is still an active PR. I haven't had a chance to work on it the last couple of weeks but should have some time this week.

The only real holdup for completing this now, I think, is to figure out what, if any, changes in behavior are acceptable from this PR. The behavioral changes are likely limited to printing/structural formatting and FSI, which have some code which specially handles Map and Set as special cases. I'm trying to keep the output the same everywhere but that's proven to be easier said than done.

[KevinRansom]

@dotnet-bot test this please

[jack-pappas]

@KevinRansom There are still some broken tests here caused by differences in structured formatting for map/set within FSI. I'm happy to fix them (or adjust the tests) but I haven't been able to get a response from anyone on the F# team or @dsyme regarding what differences in output, if any, are acceptable here.

[KevinRansom]

Jack, Well … that’s because compatibility is a tough subject ---- Can you outline the differences … and perhaps when they might occur. In general ToString() is not required to produce the same output from release to release, however, because it is often used to produce human readable output for tools … E.g. in the debugger watch window ToString() is the default formatter, we try to avoid changes to it. Kevin

[dsyme]

@jack-pappas I went looking through the PR and issue for the diff in output - could you point me to it please? thanks

[KevinRansom]

@jack-pappas @dsyme

example:

6) Failed : FSharp-Tests-Core+CoreTests.printing-1
'z.output.test.default.stdout.txt' and 'z.output.test.default.stdout.bsl' differ; "diff between [D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharp\core\printing\z.output.test.default.stdout.bsl] and [D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharp\core\printing\z.output.test.default.stdout.txt]
line 174
 - > val sxs1 : Set<string> = set ["0"]
 + > val sxs1 : Set<string> = set [0]
diff between [D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharp\core\printing\z.output.test.default.stdout.bsl] and [D:\j\workspace\release_ci_pa---866fd2c3\tests\fsharp\core\printing\z.output.test.default.stdout.txt]
line 176
 - > val sxs2 : Set<string> = set ["0"; "1"]
 + > val sxs2 : Set<string> = set [0; 1]

It looks like it's stopped putting quotes around every value. In the string collection. So we should understand why this happens and fix it.

So the scenario here is this:
in a correctly working FSI

let sxs1  = List.init  1 string |> Set.ofList;; 
val sxs1 : Set<string> = set ["0"]

in FSI with this change

let sxs1  = List.init  1 string |> Set.ofList;; 
val sxs1 : Set<string> = set [0]

I think you will need to fix the code, the tests are right.

So in FSI you are using StructuredFormat to print the types, for set and map and the implementation of StructuredFormat calls the ToString() implementation .... well it's the other way round but you get the drift ... that means we use the ToString() format. In FSI output is typed with quotes around strings. So you will probably need to update the StructuredFormat implementation for collections to do the same thing.

That will mean duplicating the ToString code because the ToString format for Set does not emit quotes.

Does that make sense?

Kevin

[dsyme]

@jack-pappas Could you bring up-to-date with master please? THanks!

[dsyme]

@jack-pappas Failing tests?

@saul Does this conflict with any of your recent work? thanks

[saul]

This is similar to #4014, but it also adds non-generic implementations. I'm not sure how much use they are, considering that we've had generic collection interfaces since .Net 2.0

Edit: looks like this doesn't touch 'list' though

[KevinRansom]

@dsyme this didn't make it into F# Core 4.3. Do we want to take in the future?

Kevin

[KevinRansom]

@dsyme

ping do we want this for 4.5?

[dsyme]

I think we prefer what we've got in #4014