mouse icon indicating copy to clipboard operation
mouse copied to clipboard

Functions for `list` and `set` to retrieve the tail in a non-exception-throwing manner

Open TobiasRoland opened this issue 3 years ago • 16 comments

Motivation I prefer to avoid methods that can throw runtime exceptions, so it'd be very nice to be able to get the tail of a list/set without having to rely on checking the length of the collection (or whatever your preferred way is).

However, the standard-lib's tail method will throw an exception when there's no tail. This makes some sense, since asking for the tail of something that has no tail is arguably a malformed question, in the same manner as asking to divide a number by zero - the behavior is simply undefined, so an exception has to be thrown.

Except, of course, we have the Option of making methods that don't throw. So, to me, arguably, the existing tail and head should've been called unsafeHead and unsafeTail, and head and tail should've returned Option.

Since that train has long since departed, and motivated by a recent discussion at $DAYJOB, I therefore suggest adding these two methods, .tailOrEmpty and tailOption (as a match to headOption), so that there's a safe, guaranteed exception-free way to get an empty/non-existent tail.

TobiasRoland avatar Aug 06 '22 14:08 TobiasRoland

Thank you for your contribution! I like those methods, and having them is reasonable to me. I just wonder if we can have it in the collections library straightforwardly to reduce allocations. I've opened a discussion https://github.com/scala/scala-library-next/issues/127. Let's wait for a while to get insights from Scala collections library mainteiners.

danicheg avatar Aug 07 '22 07:08 danicheg

Standard library Nil.drop(1) does not throw. Does it allocate? Sometimes slice is a view of the underlying collection.

som-snytt avatar Aug 07 '22 12:08 som-snytt

I rather meant allocations when using the list.tailOrEmpty syntax method (allocation for ListOps instance to be precise).

danicheg avatar Aug 07 '22 17:08 danicheg

Thanks for your well motivated contribution @TobiasRoland

@danicheg Sure, let's explore the std lib option first.. What sort of consult period would you expect, before proceeding with the change in mouse?

benhutchison avatar Aug 07 '22 22:08 benhutchison

According to my little investigation, we can't add new API methods to the std library for Scala 2 due to compatibility constraints. Also, we can't do it for Scala 3 without using extension methods, i.e. straightforwardly, at least at the moment. So greenlit from me to finish this work in mouse :+1:

danicheg avatar Aug 08 '22 08:08 danicheg

Of historical interest, we found this 2017 issue discussing making unsafe collection opt-in, too, concluding in:

However, we prefer to wait for having proper support for effects in the language rather than designing a poor man’s solution (e.g. based on implicit parameters). Consequently we will not address these issues in the current redesign.


I'm happy to help more with this PR to get it into the right shape, if you have a sort of checklist of changes you'd want made. Or if you want to just go ahead and do it, that's very fine by me too, I'm not overly precious about being the sole committer of this, I just took the path of least resistance and added list and set syntax with more-or-less duplicated implementations.

TobiasRoland avatar Aug 08 '22 10:08 TobiasRoland

💯 on the List methods. What use case do you envisage for the Set methods?

bplommer avatar Aug 08 '22 10:08 bplommer

@bplommer I'll be honest, I mostly added them because Set already has a .head, .headOption and .tail method so it felt reasonable to me to add these as a sort of... mirror?... of those methods. Arguably, if this goes in, it would make sense to add it for every collection type that supports .head and .tail currently

TobiasRoland avatar Aug 08 '22 12:08 TobiasRoland

I was intrigued by tailOption to mean non-empty tail. That enables a simpler test than

if vs.sizeCompare(1) > 0 then ???

som-snytt avatar Aug 08 '22 13:08 som-snytt

tailOption to mean non-empty tail

That's interesting. I'd suggest this would be even more intuitive from a contract point of view.

danicheg avatar Aug 08 '22 13:08 danicheg

You're thinking renaming tailOption: Option[List[A]] to nonEmpty: Option[List[A]] - or even to nonEmpty: Option[NonEmptyList[A]]?

TobiasRoland avatar Aug 08 '22 13:08 TobiasRoland

tailOption to mean non-empty tail

That's interesting. I'd suggest this would be even more intuitive from a contract point of view.

Oh I must have skimmed over that. I find that surprising to be honest - I'd expect l.tailOption <-> Either.catchNonFatal(l.tail).toOption. nonEmptyTail would also be a good method to have but the current implementation in this PR discards the information of whether the tail is empty vs undefined.

bplommer avatar Aug 08 '22 13:08 bplommer

This is a kinda controversial topic :) I mean, I was always getting stuck when faced Option[List[A]]: is Some(Nil) equal to None? But yeah, if we wanna be a literalist, then we should have this tailOption: Option[NonEmptyList[A]] 😄

danicheg avatar Aug 08 '22 13:08 danicheg

Mm, I see. I guess my question I want to answer is "does it have a tail, or does it not" - mapping neatly onto an Option - Whether the collection is empty, or has size 1 doesn't really matter if that's the question I want an answer to. But that's not the question being asked, then another interpretation might make sense, too.

e.g. the test cases:

    assertEquals(List.empty[Int].tailOption, Option.empty[List[Int]]) // None
    assertEquals(List(0).tailOption, Option.empty[List[Int]]) // None
    assertEquals(List(0, 1, 2).tailOption, Some(List(1, 2))) // Oh hey, we have a tail!

I did think about using NonEmptyList initially actually, but I didn't know if I wanted to "force" that data type onto people just naïvely using a List. Again because I mostly came at this as a "boy, I wish this was what I'd get from the std-lib".

... but given this is the companion library for Cats, maybe NEL does actually makes sense?

TobiasRoland avatar Aug 08 '22 13:08 TobiasRoland

It'd be cool if we have managed to call more people to get their thoughts on this topic. Because I'm 100% biased here.

"boy, I wish this was what I'd get from the std-lib"

I'm that boy that wishes non-empty collections to be part of std like it does in some programming languages... But since having mouse means one already has cats in the classpath, then NonEmptyList is pretty natural to use.

danicheg avatar Aug 08 '22 14:08 danicheg

Mouse is a small companion to Cats, so yes, NonEmptyList is available and encouraged IMO.

benhutchison avatar Aug 09 '22 18:08 benhutchison

Ok, I note discussion on this PR has paused..

I don't want perfect to be the enemy of good here. IMO the PR adds value in its present form and Im happy to merge.

@TobiasRoland note there are failing CI checks around source file headers that need to be added to the new files.

benhutchison avatar Aug 13 '22 06:08 benhutchison

If @TobiasRoland doesn't mind, I can help with moving this PR further.

danicheg avatar Aug 13 '22 07:08 danicheg

So, I've tweaked some things:

  1. tailOption returns Option[NonEmptyList[A]] and Option[NonEmptySet[A]] accordingly now
  2. all of these new syntaxes are available for Scalajs on Scala 3

It isn't strict and I'd be happy to change something if we decide to.

danicheg avatar Aug 15 '22 20:08 danicheg

@benhutchison I had to add a constructor for the creation of SortedSet due to changes in the API of the collections library in 2.13. The previous API of the creation of SortedSet from Set in 2.12 isn't available in 2.13.

danicheg avatar Aug 16 '22 06:08 danicheg

It'd be awesome to get your thoughts about recent changes @bplommer @TobiasRoland.

danicheg avatar Aug 16 '22 07:08 danicheg

Alright, I think we are good to save current progress. We had a quite deep discussion with many enthusiasts from the community. Thank you all! And, sure, special thanks to @TobiasRoland. Your work has opened new horizons for further development of the mouse library. That's pretty cool.

danicheg avatar Aug 18 '22 15:08 danicheg