Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jack-pappas wants to merge 4 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
13e2e66
For Map and Set types, implement non-generic ICollection and
jack-pappas Dec 23, 2016
ecc7c14
Fix IDictionary item getter test for missing key
jack-pappas Dec 30, 2016
0499875
Decorate Map and Set with StructuredFormatDisplay
jack-pappas Jan 6, 2017
d8a327e
merge master
Nov 20, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 108 additions & 6 deletions src/fsharp/FSharp.Core.Unittests/FSharp.Core/Microsoft.FSharp.Collections/MapType.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,10 +84,37 @@ type MapType() =
CheckThrowsInvalidOperationExn(fun () -> enum.Current |> ignore)
Assert.AreEqual(enum.MoveNext(), false)
CheckThrowsInvalidOperationExn(fun () -> enum.Current |> ignore)


[<Test>]
member __.IDictionary() =
// Legit ID
let id = (Map.ofArray [|(1,1);(2,4);(3,9)|]) :> IDictionary

Assert.IsTrue(id.Contains(box 1))
Assert.IsFalse(id.Contains(box 5))
Assert.AreEqual(id.[box 1], 1)
Assert.AreEqual(id.[box 3], 9)
CollectionAssert.AreEqual(id.Keys, [| 1; 2; 3|])
CollectionAssert.AreEqual(id.Values, [| 1; 4; 9|])

CheckThrowsNotSupportedException(fun () -> id.[box 2] <-88)

CheckThrowsNotSupportedException(fun () -> id.Add(box 4, box 16))
Assert.AreEqual(id.[box 1], 1)
Assert.IsNull(id.[box 100]) // IDictionary.[x] returns null when key isn't found
CheckThrowsNotSupportedException(fun () -> id.Remove(box 1) |> ignore)

// Empty ID
let id = (Map.empty : Map<int,int>) :> IDictionary
Assert.IsFalse(id.Contains(box 5))
// IDictionary specifies .Item[x] should return null when the key isn't
// found, rather than raising KeyNotFoundException.
Assert.IsNull(id.[1])
CollectionAssert.AreEqual(id.Keys, [| |] )
CollectionAssert.AreEqual(id.Values, [| |] )

[<Test>]
member this.IDictionary() =
member __.IDictionary_T() =
// Legit ID
let id = (Map.ofArray [|(1,1);(2,4);(3,9)|]) :> IDictionary<_,_>

Expand All @@ -113,7 +140,27 @@ type MapType() =
Assert.AreEqual(id.Values, [| |] )

[<Test>]
member this.ICollection() =
member __.ICollection() =
// Legit IC
let ic = (Map.ofArray [|(1,1);(2,4);(3,9)|]) :> ICollection

Assert.AreEqual (3, ic.Count)

let newArr = Array.create 5 (new KeyValuePair<int,int>(3,9))
ic.CopyTo(newArr,0)

Assert.IsTrue(ic.IsSynchronized)
Assert.IsNotNull(ic.SyncRoot)

// Empty IC
let ic = (Map.empty : Map<int,int>) :> ICollection
Assert.AreEqual (0, ic.Count)

let newArr = Array.create 5 (new KeyValuePair<int,int>(0,0))
ic.CopyTo(newArr,0)

[<Test>]
member __.ICollection_T() =
// Legit IC
let ic = (Map.ofArray [|(1,1);(2,4);(3,9)|]) :> ICollection<KeyValuePair<_,_>>

Expand Down Expand Up @@ -150,7 +197,62 @@ type MapType() =
let ic = [] |> Map.ofList :> IComparable
Assert.AreEqual(ic.CompareTo([]|> Map.ofList),0)


[<Test>]
member __.IComparable_T() =
// Legit IC
let ic = (Map.ofArray [|(1,1);(2,4);(3,9)|]) :> IComparable<_>
Assert.AreEqual(ic.CompareTo([(1,1);(2,4);(3,9)]|> Map.ofList),0)
Assert.AreEqual(ic.CompareTo([(1,1);(3,9);(2,4)]|> Map.ofList),0)
Assert.AreEqual(ic.CompareTo([(1,1);(9,81);(2,4)]|> Map.ofList),-1)
Assert.AreEqual(ic.CompareTo([(1,1);(0,0);(2,4)]|> Map.ofList),1)

// Empty IC
let ic = [] |> Map.ofList :> IComparable<_>
Assert.AreEqual(ic.CompareTo([]|> Map.ofList),0)

[<Test>]
member __.IEquatable_T() =
// Legit IC
let ic = (Map.ofArray [|(1,1);(2,4);(3,9)|]) :> IComparable<_>
Assert.AreEqual(ic.CompareTo([(1,1);(2,4);(3,9)]|> Map.ofList),0)
Assert.AreEqual(ic.CompareTo([(1,1);(3,9);(2,4)]|> Map.ofList),0)
Assert.AreEqual(ic.CompareTo([(1,1);(9,81);(2,4)]|> Map.ofList),-1)
Assert.AreEqual(ic.CompareTo([(1,1);(0,0);(2,4)]|> Map.ofList),1)

// Empty IC
let ic = [] |> Map.ofList :> IComparable<_>
Assert.AreEqual(ic.CompareTo([]|> Map.ofList),0)

#if FX_ATLEAST_45
[<Test>]
member __.IReadOnlyCollection_T () =
Assert.Ignore ("Test not yet implemented.")

[<Test>]
member __.IReadOnlyDictionary_T () =
// Legit IROD
let id = (Map.ofArray [|(1,1);(2,4);(3,9)|]) :> IReadOnlyDictionary<_,_>

Assert.IsTrue(id.ContainsKey(1))
Assert.IsFalse(id.ContainsKey(5))

Assert.AreEqual(id.[1], 1)
Assert.AreEqual(id.[3], 9)

CollectionAssert.AreEqual(id.Keys, [| 1; 2; 3|])
CollectionAssert.AreEqual(id.Values, [| 1; 4; 9|]

Assert.IsTrue(id.TryGetValue(1, ref 1))
Assert.IsFalse(id.TryGetValue(100, ref 1))

// Empty ID
let id = Map.empty :> IReadOnlyDictionary<int, int> // Note no type args
Assert.IsFalse(id.ContainsKey(5))
CheckThrowsKeyNotFoundException(fun () -> id.[1] |> ignore)
CollectionAssert.AreEqual(id.Keys, [| |] )
CollectionAssert.AreEqual(id.Values, [| |] )
#endif

// Base class methods
[<Test>]
member this.ObjectGetHashCode() =
Expand Down Expand Up @@ -265,12 +367,12 @@ type MapType() =
for i = 0 to l.Count - 1 do
Assert.AreEqual(i*i, l.[i])
Assert.AreEqual(i*i, l.Item(i))

// Invalid index
let l = (Map.ofArray [|(1,1);(2,4);(3,9)|])
CheckThrowsKeyNotFoundException(fun () -> l.[ -1 ] |> ignore)
CheckThrowsKeyNotFoundException(fun () -> l.[1000] |> ignore)

[<Test>]
member this.Remove() =

Expand Down
170 changes: 153 additions & 17 deletions src/fsharp/FSharp.Core/map.fs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ namespace Microsoft.FSharp.Collections
open System
open System.Collections.Generic
open System.Diagnostics
open System.Reflection
open Microsoft.FSharp.Core
open Microsoft.FSharp.Core.LanguagePrimitives.IntrinsicOperators

Expand Down Expand Up @@ -401,23 +402,35 @@ namespace Microsoft.FSharp.Collections
i.started <- true (* The first call to MoveNext "starts" the enumeration. *)
not i.stack.IsEmpty

let mkIEnumerator s =
let i = ref (mkIterator s)
{ new IEnumerator<_> with
/// Enumerator for MapTree.
[<Sealed>]
type MapTreeEnumerator<'Key, 'Value when 'Key : comparison> (s : MapTree<'Key, 'Value>) =
let i = ref (mkIterator s)

interface IEnumerator<KeyValuePair<'Key, 'Value>> with
member __.Current = current !i
interface System.Collections.IEnumerator with
member __.Current = box (current !i)
member __.MoveNext() = moveNext !i
member __.Reset() = i := mkIterator s
interface System.Collections.IDictionaryEnumerator with
member __.Entry =
let kvp = current !i
System.Collections.DictionaryEntry (box kvp.Key, box kvp.Value)
member __.Key = box (current !i).Key
member __.Value = box (current !i).Value
interface System.IDisposable with
member __.Dispose() = ()}
member __.Dispose() = ()

let mkIEnumerator s =
new MapTreeEnumerator<_,_> (s)


[<System.Diagnostics.DebuggerTypeProxy(typedefof<MapDebugView<_,_>>)>]
[<System.Diagnostics.DebuggerDisplay("Count = {Count}")>]
[<Sealed>]
[<CodeAnalysis.SuppressMessage("Microsoft.Naming", "CA1710:IdentifiersShouldHaveCorrectSuffix")>]
[<StructuredFormatDisplay("{StructuredFormat}")>]
[<CompiledName("FSharpMap`2")>]
type Map<[<EqualityConditionalOn>]'Key,[<EqualityConditionalOn;ComparisonConditionalOn>]'Value when 'Key : comparison >(comparer: IComparer<'Key>, tree: MapTree<'Key,'Value>) =

Expand Down Expand Up @@ -555,22 +568,25 @@ namespace Microsoft.FSharp.Collections
res <- combineHash res (Unchecked.hash y)
abs res

override this.Equals(that) =
member private this.EqualsImpl (that : Map<'Key,'Value>) =
use e1 = (this :> seq<_>).GetEnumerator()
use e2 = (that :> seq<_>).GetEnumerator()
let rec loop () =
let m1 = e1.MoveNext()
let m2 = e2.MoveNext()
(m1 = m2) && (not m1 || let e1c, e2c = e1.Current, e2.Current in ((e1c.Key = e2c.Key) && (Unchecked.equals e1c.Value e2c.Value) && loop()))
loop()

override this.Equals (that) =
match that with
| :? Map<'Key,'Value> as that ->
use e1 = (this :> seq<_>).GetEnumerator()
use e2 = (that :> seq<_>).GetEnumerator()
let rec loop () =
let m1 = e1.MoveNext()
let m2 = e2.MoveNext()
(m1 = m2) && (not m1 || let e1c, e2c = e1.Current, e2.Current in ((e1c.Key = e2c.Key) && (Unchecked.equals e1c.Value e2c.Value) && loop()))
loop()
this.EqualsImpl that
| _ -> false

override this.GetHashCode() = this.ComputeHashCode()

interface IEnumerable<KeyValuePair<'Key, 'Value>> with
member __.GetEnumerator() = MapTree.mkIEnumerator tree
member __.GetEnumerator() = upcast (MapTree.mkIEnumerator tree)

interface System.Collections.IEnumerable with
member __.GetEnumerator() = (MapTree.mkIEnumerator tree :> System.Collections.IEnumerator)
Expand All @@ -580,7 +596,7 @@ namespace Microsoft.FSharp.Collections
with get x = m.[x]
and set x v = ignore(x,v); raise (NotSupportedException(SR.GetString(SR.mapCannotBeMutated)))

// REVIEW: this implementation could avoid copying the Values to an array
// REVIEW: this implementation could avoid copying the Keys to an array
member s.Keys = ([| for kvp in s -> kvp.Key |] :> ICollection<'Key>)

// REVIEW: this implementation could avoid copying the Values to an array
Expand All @@ -591,6 +607,62 @@ namespace Microsoft.FSharp.Collections
member s.TryGetValue(k,r) = if s.ContainsKey(k) then (r <- s.[k]; true) else false
member s.Remove(k : 'Key) = ignore(k); (raise (NotSupportedException(SR.GetString(SR.mapCannotBeMutated))) : bool)

interface System.Collections.IDictionary with
member m.Item
with get key =
match key with
| null ->
// According to the documentation for IDictionary, implementations should
// raise ArgumentNullException if the key passed to the indexer is null.
nullArg "key"
| :? 'Key as key ->
match m.TryFind key with
| Some v -> box v
| None -> null
| _ -> null
and set key value =
Copy link
Contributor

@KevinRansom KevinRansom Dec 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@smoothdeveloper smoothdeveloper Dec 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

@jack-pappas jack-pappas Dec 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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).

if Object.ReferenceEquals (null, key) then nullArg "key"
ignore value
raise (NotSupportedException(SR.GetString(SR.mapCannotBeMutated)))

// REVIEW: this implementation could avoid copying the Keys to an array
member s.Keys = ([| for kvp in s -> kvp.Key |] :> System.Collections.ICollection)

// REVIEW: this implementation could avoid copying the Values to an array
member s.Values = ([| for kvp in s -> kvp.Value |] :> System.Collections.ICollection)

member __.IsFixedSize = true
member __.IsReadOnly = true

member __.Add(key, value) =
if Object.ReferenceEquals (null, key) then nullArg "key"
ignore value
raise (NotSupportedException(SR.GetString(SR.mapCannotBeMutated)))
member __.Clear () = raise (NotSupportedException(SR.GetString(SR.mapCannotBeMutated)))
member m.Contains key =
// IDictionary MSDN documentation says:
// "Implementations can vary in whether they allow the key to be null."
match key with
| :? 'Key as k ->
m.ContainsKey k
| null when not
#if FX_RESHAPED_REFLECTION
((typeof<'Key>).GetTypeInfo().IsValueType)
#else
typeof<'Key>.IsValueType
#endif
->
// If this map's key type is a reference type, a null reference could be a
// legitimate key in the map so pass it through to ContainsKey.
// Need to call ContainsKey with default(Key) instead of null to satisfy type constraints.
m.ContainsKey Unchecked.defaultof<_>
| _ -> false

member __.GetEnumerator () = upcast (MapTree.mkIEnumerator tree)
member __.Remove key =
if Object.ReferenceEquals (null, key) then nullArg "key"
raise (NotSupportedException(SR.GetString(SR.mapCannotBeMutated)))

interface ICollection<KeyValuePair<'Key, 'Value>> with
member __.Add(x) = ignore(x); raise (NotSupportedException(SR.GetString(SR.mapCannotBeMutated)));
member __.Clear() = raise (NotSupportedException(SR.GetString(SR.mapCannotBeMutated)));
Expand All @@ -600,8 +672,26 @@ namespace Microsoft.FSharp.Collections
member s.IsReadOnly = true
member s.Count = s.Count

interface System.Collections.ICollection with
member s.Count = s.Count
member s.IsSynchronized = true
member s.SyncRoot = upcast s
member __.CopyTo(arr,i) =
// Check arguments in accordance with what's specified by the docs for ICollection.
if Object.ReferenceEquals (null, arr) then nullArg "arr"
elif i < 0 then raise (IndexOutOfRangeException ())

// Raise an ArgumentException if 'arr' is multidimensional,
// or has the wrong element type.
match arr with
| :? (KeyValuePair<'Key,'Value>[]) as kvpArr ->
MapTree.copyToArray tree kvpArr i
| _ ->
raise (ArgumentException("arr"))

interface System.IComparable with
member m.CompareTo(obj: obj) =
member m.CompareTo(obj: obj) =
// REVIEW: Docs for IComparable say implementations shouldn't raise exceptions for null arguments.
match obj with
| :? Map<'Key,'Value> as m2->
Seq.compareWith
Expand All @@ -612,15 +702,61 @@ namespace Microsoft.FSharp.Collections
| _ ->
invalidArg "obj" (SR.GetString(SR.notComparable))

override x.ToString() =
interface System.IComparable<Map<'Key,'Value>> with
member m.CompareTo other =
// REVIEW: Docs for IComparable say implementations shouldn't raise exceptions for null arguments.
(m, other)
||> Seq.compareWith (fun (kvp1 : KeyValuePair<_,_>) (kvp2 : KeyValuePair<_,_>) ->
let c = comparer.Compare(kvp1.Key,kvp2.Key)
if c <> 0 then c else Unchecked.compare kvp1.Value kvp2.Value)

interface System.IEquatable<Map<'Key,'Value>> with
member this.Equals other =
// Not equal if the other instance is null.
not (Object.ReferenceEquals (null, other)) && this.EqualsImpl other

#if FX_ATLEAST_45
interface IReadOnlyCollection<KeyValuePair<'Key, 'Value>> with
member s.Count = s.Count

interface IReadOnlyDictionary<'Key, 'Value> with
member m.Item
with get x = m.[x]

// REVIEW: this implementation could avoid copying the Keys to an array
member s.Keys = ([| for kvp in s -> kvp.Key |] :> IEnumerable<'Key>)
Copy link
Contributor

@smoothdeveloper smoothdeveloper Dec 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@jack-pappas jack-pappas Dec 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@saul saul Feb 6, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


// REVIEW: this implementation could avoid copying the Values to an array
member s.Values = ([| for kvp in s -> kvp.Value |] :> IEnumerable<'Value>)

member s.ContainsKey(k) = s.ContainsKey(k)
member s.TryGetValue(k,r) =
match s.TryFind k with
| None ->
// Like Dictionary<_,_>, clear the 'out' value if the key wasn't found.
r <- Unchecked.defaultof<_>
false
| Some v ->
r <- v
true
#endif

#if !FX_NO_DEBUG_DISPLAYS
[<DebuggerBrowsable(DebuggerBrowsableState.Never)>]
#endif
member x.StructuredFormat =
match List.ofSeq (Seq.truncate 4 x) with
| [] -> "map []"
| [KeyValue h1] -> System.Text.StringBuilder().Append("map [").Append(LanguagePrimitives.anyToStringShowingNull h1).Append("]").ToString()
| [KeyValue h1;KeyValue h2] -> System.Text.StringBuilder().Append("map [").Append(LanguagePrimitives.anyToStringShowingNull h1).Append("; ").Append(LanguagePrimitives.anyToStringShowingNull h2).Append("]").ToString()
| [KeyValue h1;KeyValue h2;KeyValue h3] -> System.Text.StringBuilder().Append("map [").Append(LanguagePrimitives.anyToStringShowingNull h1).Append("; ").Append(LanguagePrimitives.anyToStringShowingNull h2).Append("; ").Append(LanguagePrimitives.anyToStringShowingNull h3).Append("]").ToString()
| KeyValue h1 :: KeyValue h2 :: KeyValue h3 :: _ -> System.Text.StringBuilder().Append("map [").Append(LanguagePrimitives.anyToStringShowingNull h1).Append("; ").Append(LanguagePrimitives.anyToStringShowingNull h2).Append("; ").Append(LanguagePrimitives.anyToStringShowingNull h3).Append("; ... ]").ToString()

and
override x.ToString() =
x.StructuredFormat


and
[<Sealed>]
MapDebugView<'Key,'Value when 'Key : comparison>(v: Map<'Key,'Value>) =

Expand Down
Loading