search

serokell / universum

:milky_way: Prelude written in @Serokell
MIT License
176 stars 26 forks source link

`Minimum` and `maximum` are unsafe #211

Closed Martoon-00 closed 4 years ago

Martoon-00 commented 5 years ago

We strive to get rid of partial functions so we have changed head and other functions operating on lists to make them accept NonEmpty a.

However minimum and maximum functions are currently part of Container type class and thus can fail when an empty structure is provided.

Proposals out of my head:

  1. Make those functions work only with NonEmpty a (I prefer this option).
  2. Add an empty type class IsNotEmpty which NonEmpty and other similar structures would inherit, and add it as a constraint to our methods.
  3. Make them return Maybe as their neighbor safeHead does (looks like a hack to me).
Martoon-00 commented 4 years ago

Okay, I got quite annoyed with the fact that this issue still exists.

Probably let's go with the same way we have for head: define maximum :: Ord a => NonEmpty a -> a and safeMaximum :: Ord a => t -> Element t (the latter would be part of Container). This seems like the most flexible solution.

In #5 there is an opinion that we better rename safe head to headMay. Probably it would worth applying here, but not earlier than head itself is renamed.

The same should be applied to minimum, foldr1 and foldl1.

A quite unpleasant thing is that I won't be able to write fold1 (+) [1..5] anymore (and no one mostly uses OverloadedLists extension). Probably the only way to mitigate this is adding nonEmptyUnsafe :: HasCallStack => [a] -> NonEmpty [a].

Martoon-00 commented 4 years ago

By the way, if we went with IsNonEmpty solution, then quite an interesting hack is possible: since this typeclass would not contain any methods, one could define withUnsafeListOps :: (IsNonEmpty [] => a) -> a which could allow doing unsafe head and stuff within a particular block of code in case developer wants so.

Though that is not a sufficient reason to go with IsNonEmpty solution, and just importing Unsafe module qualified provides more fine-grained control over where unsafe functions should be used and where they should not.

gromakovsky commented 4 years ago

Probably let's go with the same way we have for head: define maximum :: Ord a => NonEmpty a -> a and safeMaximum :: Ord a => t -> Element t (the latter would be part of Container). This seems like the most flexible solution.

Sounds good to me.

Martoon-00 commented 4 years ago

Closed via #223.