[RFC FS-1113] Additions to collection functions #11888

uxsoft · 2021-07-25T09:51:08Z

Suggestion approved in principle: fsharp/fslang-suggestions#1047

RFC FS-1113 https://github.com/fsharp/fslang-design/blob/main/preview/FS-1113-insert-remove-update-functions.md

dnfadmin · 2021-07-25T09:51:22Z

All CLA requirements met.

src/fsharp/FSharp.Core/array.fs

src/fsharp/FSharp.Core/map.fs

src/fsharp/FSharp.Core/map.fsi

src/fsharp/FSharp.Core/seq.fs

src/fsharp/FSharp.Core/list.fs

…ration

uxsoft · 2021-07-30T21:06:59Z

@dsyme I found out that the Map.Values implementation collides with internal Map.Values extension in illib.fs - MapAutoOpens module which returns a list instead of seq. This is the reason the solution right now doesn't build.

Since it's not part of the spec you provided I'll remove it from this PR. That said I see it's something that IS used, so let me know if you'd like me to take a look at it in a separate PR.

src/fsharp/FSharp.Core/array.fs

tests/FSharp.Core.UnitTests/SurfaceArea.fs

dsyme

This is looking good.

Based on the suggestion discussion and looknig at the implementation I think we should at least drop updateManyAt across the board. It is just too corner-case.

I'll discuss on the suggestion further

src/fsharp/FSharp.Core/map.fsi

src/fsharp/FSharp.Core/seq.fs

… illib.fs and NameResolution.fs to be compatible, update surface area

dsyme

This is looking great, just a few comments on the benchmarking

tests/benchmarks/MicroPerf/Collections.fs

# F# RFC FS-1113 - Add insert/remove/update functions for collections, also Keys/Values for Map The design suggestion [Additions to collections for insert/update/ remove](fsharp/fslang-suggestions#1047) has been marked "approved in principle". This RFC covers the detailed proposal for this suggestion. - [x] [Suggestion](fsharp/fslang-suggestions#1047) - [x] Approved in principle - [x] [Implementation](dotnet/fsharp#11888) - [Discussion](https://github.com/fsharp/fslang-design/discussions/FILL-ME-IN) # Summary 1. Add insert/remove/update functions to List, Array, Seq, Map to improve them as workhorse functional collections 2. Add `Keys` and `Values` properties to Map # Motivation All the above are commonly useful. # Detailed design We add the following: ```fsharp module List = /// Return a new list with the item at a given index removed /// If the index is outside the range of the list then it is ignored. val removeAt: index: int -> source: 'T list -> 'T list /// Return a new list with the number of items starting at a given index removed. /// If index is outside 0..source.Length-1 then it is ignored. val removeManyAt: index: int -> count: int -> source: 'T list -> 'T list /// Return a new list with the item at a given index set to the new value. If /// index is outside 0..source.Length-1 an exception is raised. val updateAt: index: int -> value: 'T -> source: 'T list -> 'T list /// Return a new list with a new item inserted before the given index.The index may be source.Length to /// include extra elements at the end of the list. If /// index is outside 0..source.Length an exception is raised. val insertAt: index: int -> value: 'T -> source: 'T list -> 'T list /// Return a new list with new items inserted before the given index. The index may be source.Length to /// include extra elements at the end of the list. /// If index is outside 0..source.Length an exception is raised. val insertManyAt: index: int -> values: seq<'T> -> source: 'T list -> 'T list module Array = /// Return a new array with the item at a given index removed /// If the index is outside the range of the array then it is ignored. val removeAt: index: int -> source: 'T[] -> 'T[] /// Return a new array with the number of items starting at a given index removed. /// If an implied item index is outside the range of the array then it is ignored. val removeManyAt: index: int -> count: int -> source: 'T[] -> 'T[] /// Return a new array with the item at a given index set to the new value. If /// index is outside 0..source.Length-1 an exception is raised. val updateAt: index: int -> value: 'T -> source: 'T[] -> 'T[] /// Return a new array with a new item inserted before the given index. The index may be source.Length to /// include extra elements at the end of the array. If /// index is outside 0..source.Length than source.Length an exception is raised. val insertAt: index: int -> value: 'T -> source: 'T[] -> 'T[] /// Return a new array with new items inserted before the given index. The index may be source.Length to /// include extra elements at the end of the array. If index is outside 0..source.Length an exception is raised. val insertManyAt: index: int -> values: seq<'T> -> source: 'T[] -> 'T[] module Seq = /// Return a new ssequence which, when iterated, will have the item at a given index removed /// If the index is outside the range of valid indexes for the sequence then it is ignored. val removeAt: index: int -> source: seq<'T> -> seq<'T> /// Return a new sequence which, when iterated, will have the given count of items starting at a given index removed. /// If any implied item index is outside the range of valid indexes for the sequence then it is ignored. val removeManyAt: index: int -> count: int -> source: seq<'T> -> seq<'T> /// Return a new sequence which, when iterated, will return the given item for the given index, and /// otherwise return the items from source. If index is below zero an exception is raised immediately. /// If the index is beyond the range on the source sequence the update is ignored. val updateAt: index: int -> value: 'T -> source: seq<'T> -> seq<'T> /// Return a new sequence which, when iterated, includes a new item inserted before the given index. The index may be source.Length to /// include extra elements at the end of the sequence. If /// index is outside 0..source.Length than source.Length an exception is raised. val insertAt: index: int -> value: 'T -> source: seq<'T> -> seq<'T> /// Return a new sequence which, when iterated, will include additional items given by values, starting /// at the given index. The index may be source.Length to /// include extra elements at the end of the sequence. /// If index is outside 0..source.Length an exception is raised. val insertManyAt: index: int -> values: seq<'T> -> source: seq<'T> -> seq<'T> ``` # Drawbacks Adds extra functions to FSharp.Core # Alternatives See suggestion discussion thread for discusssion on `updateManyAt` and also other naming alternatives. # Compatibility Not a breaking change # Unresolved questions None

dsyme · 2021-08-12T10:22:17Z

I've added an RFC for this here: https://github.com/fsharp/fslang-design/blob/main/preview/FS-1113-insert-remove-update-functions.md

src/fsharp/FSharp.Core/map.fsi

dsyme · 2021-08-12T10:33:31Z

@uxsoft I updated to

Add Map.values
Remove Map.Empty (in favour of existing Map.empty)

dsyme · 2021-08-12T18:53:49Z

@uxsoft Thank you for this contribution!

uxsoft · 2021-08-13T05:03:04Z

Thank you for your guidance!

OkkeHendriks · 2021-08-13T14:47:11Z

Nice work, now I can get rid of some of my 'extensions' in the near future :)

I am not sure if this is the right place to ask this, as this is already merged.
However, I see that at the toSeq function the summary states that The sequence will be ordered by the keys of the map.. This is also the case for Map.keys and Map.values, so I think it is an interesting property to note within the summary of those.

Also the tests should verify this if this is added.

dsyme · 2021-08-14T00:47:27Z

This is also the case for Map.keys and Map.values, so I think it is an interesting property to note within the summary of those.

Yes, I agree. It's also interesting to think whether things like "get me the middle key in the ordering of keys" will run efficiently or not

let midKey =
    let keys = map.Keys
    keys.[keys.Length/2]

I haven't checked. Probably not

uxsoft · 2021-08-14T07:31:23Z

I can open a new PR with these small changes if you'd like

OkkeHendriks · 2021-08-14T11:51:26Z

It's also interesting to think whether things like "get me the middle key in the ordering of keys" will run efficiently or not.

Correct me if I'm wrong but I think that is not even possible, directly, as ICollection does not implement Item:

So to get the middle one would have to MoveNext() through the sequence.

dsyme · 2021-08-14T13:49:10Z

Correct me if I'm wrong but I think that is not even possible, directly, as ICollection does not implement Item:

Yes you're correct, thanks

dsyme · 2021-08-14T13:49:40Z

I can open a new PR with these small changes if you'd like

Yes, please make the additions to the doc comments

auduchinok · 2021-09-13T17:13:20Z

src/fsharp/NameResolution.fs

-               |> List.filter (function
+               |> Seq.filter (function
                   | Item.UnqualifiedType _ -> false
                   | Item.Value v -> not v.IsMember
                   | _ -> true)
-               |> List.filter (ItemIsUnseen ad g ncenv.amap m >> not)
+               |> Seq.filter (ItemIsUnseen ad g ncenv.amap m >> not)
+               |> Seq.toList


Why this change was needed?

I doubt it was necessary

Notice the implementation of values in illib.fs returns a list and the public one an ICollection. The old code expecting a List would not compile if I didn't change it to seq.

If there was a better solution I missed, I'd be happy to learn.

auduchinok · 2021-09-13T17:16:09Z

src/fsharp/absil/illib.fs

+        
        static member Empty : Map<'Key, 'Value> = Map.empty
-
-        member x.Values = [ for KeyValue(_, v) in x -> v ]


@dsyme Do we expect all FCS clients to use FSharp.Core previews now? I don't think it's a good idea to tie FCS and FSharp.Core previews like this.

The tooling now shows errors (#12142) and it seems we won't be able to run FCS with a stable FSharp.Core version anymore if FCS is updated?

@auduchinok Hmmmm could you please open a separate issue about this. I agree public non-pre-release FCS should indeed only use a public non-pre-release FCS.

However if FCS in pre-release and the release plan is matching FSharp.Core than it is ok

uxsoft and others added 3 commits July 22, 2021 20:53

array implementation first draft

a45a011

Add map.keys and map.values

7a26fae

add list and seq implementations

49ad673

AviAvni reviewed Jul 25, 2021

View reviewed changes

src/fsharp/FSharp.Core/array.fs Outdated Show resolved Hide resolved

AviAvni reviewed Jul 25, 2021

View reviewed changes

src/fsharp/FSharp.Core/array.fs Outdated Show resolved Hide resolved

AviAvni reviewed Jul 25, 2021

View reviewed changes

src/fsharp/FSharp.Core/array.fs Outdated Show resolved Hide resolved

dsyme mentioned this pull request Jul 26, 2021

Additions to collections for insert/update/ remove (List, Map etc.) fsharp/fslang-suggestions#1047

Closed

5 tasks