Add 'try' versions of Set.minElement and Set.maxElement

[brikken]

I propose we add 'try' version of minElement and maxElement (and corresponding class methods) in Microsoft.FSharp.Collections.Set module.

The existing way of approaching this problem in F# is to first check if the set is empty. Alternatively wrap the current functions in a try .. with since they may throw an ArgumentException.

Pros and Cons

The advantages of making this adjustment to F# are readily available type safe options for retrieving set elements without exceptions. This is already idiomatic in other collection modules.

The disadvantages of making this adjustment to F# are enlarging the Set module and class.

Extra information

Estimated cost (XS, S, M, L, XL, XXL): XS

Affidavit (please submit!)

Please tick this by placing a cross in the box:

• This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
• I have searched both open and closed suggestions on this site and believe this is not a duplicate
• This is not something which has obviously "already been decided" in previous versions of F#. If you're questioning a fundamental design decision that has obviously already been taken (e.g. "Make F# untyped") then please don't submit it.

Please tick all that apply:

• This is not a breaking change to the F# language design
• I or my company would be willing to help implement and/or test this

[dsyme]

The existing way of approaching this problem in F# is to wrap the current methods in a try .. with since the existing functions will throw an ArgumentException on an empty set.

Isn't the existing way to check for emptiness first?

[brikken]

The existing way of approaching this problem in F# is to wrap the current methods in a try .. with since the existing functions will throw an ArgumentException on an empty set.

Isn't the existing way to check for emptiness first?

Good point - I'll edit my post.

[Tarmil]

I think that for the same reasons we should also add try versions for the List, Array and Seq module functions min, max, minBy and maxBy. As well as indexOfMin and indexOfMax from #702.

[abelbraaksma]

Perhaps we could add a tryNonEmpty and then simply map over the option. Otherwise we'll end up with a plethora of new functions for each collection type.

[gusty]

Or more generically we can add an ofPredicate function to the option module:

module Option = let ofPredicate p v = if p v then Some v else None
// use
set [42] |> Option.ofPredicate (not << Set.isEmpty) |> Option.map Set.maxElement
val it : int option = Some 42

[Tarmil]

@abelbraaksma I'm not a fan of having to throw and catch an exception (even implicitly) to handle empty collections :/

@gusty Interesting idea. It's longer than an explicit if though :/

set [42] |> fun s -> if Set.isEmpty s then None else Some (Set.maxElement s)

---

Another possibility that would provide this functionality without adding too many functions: adding a function ifEmpty to collection modules.

module Set =
    val ifEmpty : ifEmpty:'u -> ifNotEmpty:(Set<'t> -> 'u) -> collection:Set<'t> -> 'u

// Usage:
set [42] |> Set.ifEmpty None (Set.maxElement >> Some)

This also allows using something else than Option.

set [42] |> Set.ifEmpty 0 Set.maxElement
set [42] |> Set.ifEmpty ValueNone (Set.maxElement >> ValueSome)

[abelbraaksma]

@abelbraaksma I'm not a fan of having to throw and catch an exception (even implicitly) to handle empty collections :/

@Tarmil, I'm not suggesting try/catching. That is slow if the common case can be that a statement throws and exception, and it clutters code. I try (pun intended) to stay away from it where possible.

I meant something like this:

module Set =
    let tryNonEmpty s = if Set.isEmpty s then None else Some s

module List =
   let tryNonEmpty l = if List.isEmpty l then None else Some l

....

Usage:

set [42] |> Set.tryNonEmpty |> Option.map Set.maxElement
[42] |> List.tryNonEmpty |> Option.map (List.reduce (+))

I think it ends up rather concise and relatively readable.

Or more generically we can add an ofPredicate function to the option module:

@gusty, I actually have Option.whenFalse and Option.whenTrue which test a predicate. Which basically have the same signature as your proposal. It would become slightly easier, but I'm not sure if it becomes more readable:

set [42] |> Option.whenFalse Set.isEmpty |> Option.map Set.maxElement
[42] |> Option.whenFalse List.isEmpty |> Option.map (List.reduce (+))

side thought: I have these functions in my default Option extension project for a while, but looking through my code, I didn't end up using them very often. As @dsyme often mentions, oftentimes, explicit branching and explicit if statements can be more readable than piped Option.xxx that change conditions along the way. But that's of course very much a matter of taste and I've found myself going one way or the other from time to time.

[Tarmil]

@abelbraaksma OK I misunderstood, sorry. This is better indeed.

I still slightly prefer ifEmpty though; it allows you to do something else than Option, and you can easily do the equivalent of tryNonEmpty as ifEmpty None Some.

[abelbraaksma]

Another possibility that would provide this functionality without adding too many functions: adding a function ifEmpty to collection modules.

@Tarmil, this is somewhat in the same league as the recently added Option.defaultValue and Option.defaultWith. Using the same naming style, we could end up with List.defaultValue and List.defaultWith (and Set... etc). However, I think this serves a slightly different purpose than your original proposal:

1. Setting the default value for empty lists and sets deliberately creates a list or set with that default value and doesn't special-case empty lists or sets, which should often be considered a fault case (i.e., mathematically, the max value of the empty set is undefined, which is not the same as zero).
2. Turning empty sets, lists etc in to an option value using any of the aforementioned techniques serves the purpose for which options are typically used: an error case or ignorable case that should be treated as None. This seems to be idiomatic with empty lists and sets, as semantically I'd argued they're similar to None (in that, for readability, the contents is none, zero, nothing).

So perhaps we'd need both approaches ;).

As another aside: it may be wiser to simply skip to a NonEmptyList and NonEmptySet type. There are some examples lying around on the internet and they cannot be created empty, thus wouldn't run into these issues.

[Tarmil]

• Setting the default value for empty lists and sets deliberately creates a list or set with that default value and doesn't special-case empty lists or sets, which should often be considered a fault case (i.e., mathematically, the max value of the empty set is undefined, which is not the same as zero).

• Turning empty sets, lists etc in to an option value using any of the aforementioned techniques serves the purpose for which options are typically used: an error case or ignorable case that should be treated as None. This seems to be idiomatic with empty lists and sets, as semantically I'd argued they're similar to None (in that, for readability, the contents is none, zero, nothing).

The point of my original proposal is that it is flexible enough to do both and more.

// Setting the default value:
myList |> List.ifEmpty [1] id
// (an additional With variant would avoid allocating the list unless needed)

// Turning into an option:
myList |> List.ifEmpty None Some

// Call an aggregation or return a default value:
myList |> List.ifEmpty 0 List.min

// Call an aggregation and return an option:
myList |> List.ifEmpty None (List.min >> Some)

[Happypig375]

Needing to check for emptiness violates the F# pattern of try version of unsafe functions being absolutely safe. With ifEmpty, it isn't clear which functions need to be wrapped in such a function.

[Tarmil]

@Happypig375 Note that this is my alternative proposal if we don't want to add too many functions at once; I still think try versions would be good to have.

[abelbraaksma]

Using the same naming style, we could end up with List.defaultValue and List.defaultWith.

Actually, I think List.emptyValue and List.emptyWith could be a better naming.

Also, they would force the same type, while your List.ifEmpty proposal can change the type, which I think violates the principle of least surprise and/or the general monadic nature of these types. We have map and bind for that.

I think that both with my proposal and yours, they are not very flexible. @gusty's proposal, or whenFalse/whenTrue with a predicate function are far more flexible to a wide range of situations.

For instance, I have many requirements for "more-than-one" situations in my own field, which would easily be covered with the predicate-based functions:

// not necessarily the best way, but just to illustrate:
// this will run doSomeCalculations only when size of list is 2+, 
// without having to iterate the list manually
myList 
|> List.withPredicate
    (List.tryTail >> Option.bind List.tryTail >> Option.isSome) 
    doSomeCalculations

[abelbraaksma]

I still think try versions would be good to have.

I understand your reasoning but I disagree. If we go that way, for consistency, we should add the tryXXX functions for every function that can throw. I think it is better to just check for the precondition instead. Just for List, here are the functions that currently can throw but don't have a try variant:

List.average
List.averageBy
List.chunkBySize
List.exactlyOne
List.fold2
List.foldBack2
List.forall2
List.iter2
List.iteri2
List.map2
List.map3
List.mapi2
List.min
List.minBy
List.max
List.maxBy
List.permute
List.reduceBack
List.skip
List.split
List.splitInto
List.take
List.transpose
List.windowed
List.zip
List.zip3

To be fair, I'm a little surprised this list is so long, and I also noted that some on that list do not explicitly stated that they can raise an exception.

But my point is simple: allow for some function (name TBD) that allows easy composition with the existing functions for either a common predicate (empty, equal lengths) or a generalized predicate.

[gusty]

It's longer than an explicit if though :/

Yes, but that's mainly due to the verbose syntax of having to write repeatedly the name of the module Option. all the time, using generic functions like the ones in F#+ will make it way shorter, but anyway, going back to "idiomatic" F# code, the advantage is that is pipeable as opposed to an explicit if without having to write the fun s -> keyword.

Regarding the try functions, I think that's in theory the right way to go, so we have all total functions but as noted by @abelbraaksma the list of partial functions is already too long, though some of them like zip should be made more flexible IMHO in order to become total.

[theprash]

@abelbraaksma The problem with using a combinator to make a function safe is you still have to check carefully that you've combined the functions safely. I would prefer to have all of the try functions and have lint warnings for the unsafe versions.

I would go so far as to say I'd prefer to have the unsafe versions moved to an Unsafe namespace. Of course it's a breaking change but very easy to fix for old code. It's a bit embarrassing to talk about how type-safe and null-safe F# is only to have this kind of exception thrown all over the place.

[Happypig375]

The BCL is full of types and methods that can throw exceptions. We will have to deal with them in F# anyways.

[abelbraaksma]

The problem with using a combinator to make a function safe is you still have to check carefully that you've combined the functions safely

@theprash, I agree, but simple defensive programming means you use map/bind/filter and the like, which will never throw (unless called from C# with null, but let's ignore that for a moment).

Just using (for example) List.head when the list can be empty, is indeed unsafe. But considering the abundance of unsafe functions, I'm afraid that ship has sailed. Besides, there are many cases where its use is totally fine, and having to write List.Unsafe.head then may send mixed messages.

Totally safe code is still a task for the programmer to look out for. If all statements are safe, we'd be out of a job soon ;). Though it may be interesting to write a safe library of all core functions and use that as an alternative. Something like SafeF# ;).

[theprash]

@abelbraaksma We already have List.tryHead and several similar functions so it would increase consistency to add all of them. Is there a particular downside to that? I think that leaves us in a much better position overall.

Then we could optionally lint for the usage of the unsafe functions, giving people the chance to write code that forces them to consider all cases and use Option.get if they're sure they want to.

Totally safe code is still a task for the programmer to look out for.

The more cognitive load we can automate away from the programmer without significant cost the better. They will still have plenty of other things to think about.

[abelbraaksma]

We already have List.tryHead and several similar functions so it would increase consistency to add all of them. Is there a particular downside to that? I think that leaves us in a much better position overall.

@theprash I know, I was responding to your comment that we shouldn't have unsafe functions to begin with, it put them in Unsafe. That's what I meant with 'that ship has sailed'.

About the downside: only exponential growth of functions. It's already hard to weed through the list of your unsure what you need. But I'm not against the idea. Just that when I was proposing similar ideas (see tryExactlyOne), the discussion also went in the direction of not wanting to clutter the surface area too much. The conclusion then by @dsyme was that if it helps users because there's no trivial way to write a safe version yourself, then we should add it.

Then we could optionally lint for the usage of the unsafe functions,

Indeed, though currently, F#Lint doesn't work out of the box in the VS IDE. This would certainly help new users, but now they're probably not using it, I'd argue it'd be a fine addition and better user experience if it were added to the standard F# installation by default.

Overall I think we'd be much helped if we can create a list that's guaranteed 1+. It should be a list, so that you can use the same functions. Then Lint can understand the type and doesn't have to warn when using List.head on it.

[dsyme]

My feeling is that these are too corner case to add. Using minElement/maxElement is just really rare already, and use of the try versions would be vanishingly small