Skip to content
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

Jump to bottom

Redesign NonEmptyLazyList to be maximally lazy #4504

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
+174 −34
Open

Redesign NonEmptyLazyList to be maximally lazy #4504

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
ef284ec
Make `NonEmptyLazyList` properly lazy
NthPortal May 4, 2023
ad69065
Redesign `NonEmptyLazyList` to be maximally lazy
NthPortal Sep 3, 2023
3c5e564
fixup! Redesign `NonEmptyLazyList` to be maximally lazy
NthPortal Sep 3, 2023
d80d959
fixup! fixup! Redesign `NonEmptyLazyList` to be maximally lazy
NthPortal Sep 3, 2023
e948a1d
Restore bincompat
armanbilge Sep 4, 2023
10f8c21
Stop old `#::` from being called
NthPortal Sep 4, 2023
4f842eb
fixup! Stop old `#::` from being called
NthPortal Sep 9, 2023
df8778e
fixup! fixup! Stop old `#::` from being called
NthPortal Sep 9, 2023
5e6b46e
fixup! fixup! fixup! Stop old `#::` from being called
NthPortal Sep 9, 2023
29d095b
fixup! fixup! fixup! fixup! Stop old `#::` from being called
NthPortal Sep 10, 2023
46754d4
fixup! fixup! fixup! fixup! fixup! Stop old `#::` from being called
NthPortal Sep 18, 2023
2bb52cf
fixup! fixup! fixup! fixup! fixup! fixup! Stop old `#::` from being c…
NthPortal Sep 18, 2023
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
178 changes: 144 additions & 34 deletions core/src/main/scala-2.13+/cats/data/NonEmptyLazyList.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,13 @@ object NonEmptyLazyList extends NonEmptyLazyListInstances {
s.asInstanceOf[LazyList[A]]

def fromLazyList[A](as: LazyList[A]): Option[NonEmptyLazyList[A]] =
if (as.nonEmpty) Option(create(as)) else None
if (as.nonEmpty) Some(create(as)) else None

def fromLazyListUnsafe[A](ll: LazyList[A]): NonEmptyLazyList[A] =
if (ll.nonEmpty) create(ll)
else throw new IllegalArgumentException("Cannot create NonEmptyLazyList from empty LazyList")
def fromLazyListUnsafe[A](ll: LazyList[A]): NonEmptyLazyList[A] = {
@inline def ex = new IllegalArgumentException("Cannot create NonEmptyLazyList from empty LazyList")
if (ll.knownSize == 0) throw ex
else create({ if (ll.isEmpty) throw ex else ll } #::: LazyList.empty)
}

def fromNonEmptyList[A](as: NonEmptyList[A]): NonEmptyLazyList[A] =
create(LazyList.from(as.toList))
Expand All @@ -62,14 +64,70 @@ object NonEmptyLazyList extends NonEmptyLazyListInstances {
def fromSeq[A](as: Seq[A]): Option[NonEmptyLazyList[A]] =
if (as.nonEmpty) Option(create(LazyList.from(as))) else None

def fromLazyListPrepend[A](a: A, ca: LazyList[A]): NonEmptyLazyList[A] =
create(a +: ca)
def fromLazyListPrepend[A](a: => A, ll: => LazyList[A]): NonEmptyLazyList[A] =
create(a #:: ll)

@deprecated("Use overload with by-name args", "2.11.0")
def fromLazyListPrepend[A]()(a: A, ll: LazyList[A]): NonEmptyLazyList[A] =
fromLazyListPrepend(a, ll)

def fromLazyListAppend[A](ll: => LazyList[A], a: => A): NonEmptyLazyList[A] =
create(ll #::: a #:: LazyList.empty)

def fromLazyListAppend[A](ca: LazyList[A], a: A): NonEmptyLazyList[A] =
create(ca :+ a)
@deprecated("Use overload with by-name args", "2.11.0")
def fromLazyListAppend[A]()(ll: LazyList[A], a: A): NonEmptyLazyList[A] =
fromLazyListAppend(ll, a)

def apply[A](a: => A, as: A*): NonEmptyLazyList[A] =
create(LazyList.concat(LazyList(a), LazyList.from(as)))
create(a #:: LazyList.from(as))
NthPortal marked this conversation as resolved.
Show resolved Hide resolved

/**
* Wraps a `LazyList` that may or may not be empty so that individual
* elements or `NonEmptyLazyList`s can be prepended to it to construct a
* result that is guaranteed not to be empty without evaluating any elements.
*/
def maybe[A](ll: => LazyList[A]): Maybe[A] = new Maybe(() => ll)

/**
* Wrapper around a `LazyList` that may or may not be empty so that individual
* elements or `NonEmptyLazyList`s can be prepended to it to construct a
* result that is guaranteed not to be empty without evaluating any elements.
*/
final class Maybe[A] private[NonEmptyLazyList] (mkLL: () => LazyList[A]) {
// because instances of this class are created explicitly, they might be
// reused, and we don't want to re-evaluate `mkLL`
private[this] lazy val ll = mkLL()

/** Prepends a single element, yielding a `NonEmptyLazyList`. */
def #::[AA >: A](elem: => AA): NonEmptyLazyList[AA] =
create(elem #:: ll)

/** Prepends a `LazyList`, yielding another [[Maybe]]. */
def #:::[AA >: A](prefix: => LazyList[AA]): Maybe[AA] =
new Maybe(() => prefix #::: ll)
Copy link
Contributor

@satorg satorg Sep 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I doubt this method belongs here.
Prepending one LazyList to another one lazily is the job that LazyList.Deferrer seems to be doing already. I.e.:

def a: LazyList[Int] = ???
def b: LazyList[Int] = ???

val maybe1 = NonEmptyList.maybe(a #::: b)
// the above seems to be the same as
val maybe2 = a #::: NonEmptyList.maybe(b)

Anyway a method that only involves LazyList on both sides does not seem to be a part of the NonEmptyLazyList DSL to me.

Copy link
Author

@NthPortal NthPortal Sep 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

so this question is... tough. conceivably, one might build up a result something like as follows:

def buildResult[A](maybe: NonEmptyLazyList.Maybe[A]): NonEmptyLazyList[A] =
  if (something) someNell #::: maybe
  else if (somethingElse) a #:: b #:: maybe
  else buildResult(someLL #::: maybe)
}
buildResult(NonEmptyLazyList.maybe(LazyList.empty))

now, you could argue that you still don't need it, and can construct the Maybe instance later. which is true. but that seems uglier and less intuitive to me. would love to get thoughts from more people.

Edit: this is example is awful, for reasons that I will explain in another comment below. I'm honestly ashamed to have written it

Copy link
Contributor

@satorg satorg Sep 10, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

you could argue that you still don't need it

Yeah, indeed, I would :)

Again just IMO: the entire Maybe does not seem that it should be a part of NonEmptyLazyList per se. Because it looks like a bunch of extension methods for LazyList that simply helps to construct a new NonEmptyLazyList out of it.

For example, see cats.syntax.ListSyntax and its corresponding ListOps.toNel, etc.

So perhaps it would make sense to consider creating a cats.syntax.LazyListSyntax which would provide all this functionality as a bunch of extension methods only.

Copy link
Author

Choose a reason for hiding this comment

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

the issue is, NonEmptyLazyList.fromLazyList evaluates the first state/element of the LazyList, which is Bad™; NonEmptyList.fromList does not have this concern as the List is already fully evaluated. the intent behind Maybe was to have a way to construct a NonEmptyLazyList without evaluating any elements. now that fromLazyListUnsafe has been changed to be lazy, Maybe isn't needed as much, though I maintain that it is still a useful tool. its design could perhaps use a bit more workshopping though

Copy link
Author

@NthPortal NthPortal Sep 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I've been sick for a week, so I haven't been able to address things, but I'm hopefully better enough to get back to this.

the reason we need some sort of wrapper around LazyList with a #::: operator is that, despite it syntactically being a right-associative prepend operator, #::: actually behaves semantically as a left-associative append operator.

this also means that my example earlier was terrible, and the design of Maybe isn't ideal either. I really have been off my game recently.

I'll re-evaluate and redesign this part of the API to be a bit more reasonable with this in mind


/** Prepends a `NonEmptyLazyList`, yielding a `NonEmptyLazyList`. */
def #:::[AA >: A](prefix: => NonEmptyLazyList[AA])(implicit d: DummyImplicit): NonEmptyLazyList[AA] =
create(prefix.toLazyList #::: ll)
}

final class Deferrer[A] private[NonEmptyLazyList] (private val nell: () => NonEmptyLazyList[A]) extends AnyVal {

/** Prepends a single element. */
def #::[AA >: A](elem: => AA): NonEmptyLazyList[AA] =
create(elem #:: nell().toLazyList)

/** Prepends a `LazyList`. */
def #:::[AA >: A](prefix: => LazyList[AA]): NonEmptyLazyList[AA] =
create(prefix #::: nell().toLazyList)
NthPortal marked this conversation as resolved.
Show resolved Hide resolved

/** Prepends a `NonEmptyLazyList`. */
def #:::[AA >: A](prefix: => NonEmptyLazyList[AA])(implicit d: DummyImplicit): NonEmptyLazyList[AA] =
create(prefix.toLazyList #::: nell().toLazyList)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really need this overload with NonEmptyLazyList type if we already have another one that takes LazyList? I am a bit concerned about this overload trick. Usually, overloaded method names are discouraged unless absolutely necessary. But it does not seem to be the case here, because prepending a NonEmptyLazyList can be easily achieved with just the single method above that takes LazyList only:

def a: NonEmptyLasyList[X] = ???
def b: NonEmptyLasyList[X] = ???

val c: NonEmptyLazyList[X] = a.toLazyList #::: b

Copy link
Author

Choose a reason for hiding this comment

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

it would be weird to have NonEmptyLazyList be a second-class citizen of its own API. however, the meaning of the methods are identical, yielding no additional conceptual weight and making the API easier to use. additionally, it's not possible to accidentally use the wrong method either, as they are only the same after erasure. I don't personally see any issue with it

Copy link
Contributor

Choose a reason for hiding this comment

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

But it pretty much corresponds to the way how other APIs are organized in Cats. See for example NonEmptyList.concat vs NonEmptyList.concatNel, also NonEmptyVector.concat vs concatNev, etc. Retrospectively, it would not be a problem to overload concat everywhere in the way as you're proposing. But for some reason the decision was to avoid overloading and provide clearer separate names wherever possible. I personally support it because feel a bit sour about having any sort of "hacks and tricks" in code – they are usually easy to add but tend to turn into maintenance hell in long-term run.

That said, NonEmptyList has the ::: which takes another NonEmptyList, not just List. Perhaps it is just a legacy of that particular class because it was the very first NonEmpty* one. Anyway, it is still not overloaded.

}

implicit def toDeferrer[A](nell: => NonEmptyLazyList[A]): Deferrer[A] =
new Deferrer(() => nell)

implicit def catsNonEmptyLazyListOps[A](value: NonEmptyLazyList[A]): NonEmptyLazyListOps[A] =
new NonEmptyLazyListOps(value)
NthPortal marked this conversation as resolved.
Show resolved Hide resolved
Expand Down Expand Up @@ -110,18 +168,16 @@ class NonEmptyLazyListOps[A](private val value: NonEmptyLazyList[A])
* Returns a new NonEmptyLazyList consisting of `a` followed by this
*/
final def prepend[AA >: A](a: AA): NonEmptyLazyList[AA] =
create(a #:: toLazyList)
create(toLazyList.prepended(a))
NthPortal marked this conversation as resolved.
Show resolved Hide resolved

/**
* Alias for [[prepend]].
*/
final def +:[AA >: A](a: AA): NonEmptyLazyList[AA] =
prepend(a)

/**
* Alias for [[prepend]].
*/
final def #::[AA >: A](a: AA): NonEmptyLazyList[AA] =
@deprecated("use Deferrer construction instead")
final def #::[AA >: A]()(a: AA): NonEmptyLazyList[AA] =
prepend(a)
NthPortal marked this conversation as resolved.
Show resolved Hide resolved

/**
Expand All @@ -137,54 +193,108 @@ class NonEmptyLazyListOps[A](private val value: NonEmptyLazyList[A])
append(a)

/**
* concatenates this with `ll`
* Concatenates this with `ll`; equivalent to `appendLazyList`
*/
final def concat[AA >: A](ll: LazyList[AA]): NonEmptyLazyList[AA] =
create(toLazyList ++ ll)
final def concat[AA >: A](ll: => LazyList[AA]): NonEmptyLazyList[AA] =
appendLazyList(ll)

@deprecated("Use overload with by-name args", "2.11.0")
final private[data] def concat[AA >: A]()(ll: LazyList[AA]): NonEmptyLazyList[AA] =
concat(ll)

/**
* Concatenates this with `nell`
* Alias for `concat`
*/
final def concatNell[AA >: A](nell: NonEmptyLazyList[AA]): NonEmptyLazyList[AA] =
create(toLazyList ++ nell.toLazyList)
final def ++[AA >: A](ll: => LazyList[AA]): NonEmptyLazyList[AA] =
concat(ll)

/**
* Alias for concatNell
* Concatenates this with `nell`; equivalent to `appendNell`
*/
final def ++[AA >: A](nell: NonEmptyLazyList[AA]): NonEmptyLazyList[AA] =
final def concatNell[AA >: A](nell: => NonEmptyLazyList[AA]): NonEmptyLazyList[AA] =
appendNell(nell)

@deprecated("Use overload with by-name args", "2.11.0")
final private[data] def concatNell[AA >: A]()(nell: NonEmptyLazyList[AA]): NonEmptyLazyList[AA] =
concatNell(nell)

/**
* Alias for `concatNell`
*/
final def ++[AA >: A](nell: => NonEmptyLazyList[AA])(implicit d: DummyImplicit): NonEmptyLazyList[AA] =
concatNell(nell)

@deprecated("Use overload with by-name args", "2.11.0")
final private[data] def ++[AA >: A]()(ll: NonEmptyLazyList[AA]): NonEmptyLazyList[AA] =
++(ll)

/**
* Appends the given LazyList
*/
final def appendLazyList[AA >: A](nell: LazyList[AA]): NonEmptyLazyList[AA] =
if (nell.isEmpty) value
else create(toLazyList ++ nell)
final def appendLazyList[AA >: A](ll: => LazyList[AA]): NonEmptyLazyList[AA] =
create(toLazyList #::: ll)

@deprecated("Use overload with by-name args", "2.11.0")
final private[data] def appendLazyList[AA >: A]()(ll: LazyList[AA]): NonEmptyLazyList[AA] =
appendLazyList(ll)

/**
* Alias for `appendLazyList`
*/
final def :++[AA >: A](c: LazyList[AA]): NonEmptyLazyList[AA] =
appendLazyList(c)
final def :++[AA >: A](ll: => LazyList[AA]): NonEmptyLazyList[AA] =
appendLazyList(ll)
NthPortal marked this conversation as resolved.
Show resolved Hide resolved

@deprecated("Use overload with by-name args", "2.11.0")
final private[data] def :++[AA >: A]()(ll: LazyList[AA]): NonEmptyLazyList[AA] =
appendLazyList(ll)

/**
* Prepends the given NonEmptyLazyList
*/
final def appendNell[AA >: A](nell: => NonEmptyLazyList[AA]): NonEmptyLazyList[AA] =
create(toLazyList #::: nell.toLazyList)

/**
* Alias for `appendNell`
*/
final def :++[AA >: A](nell: => NonEmptyLazyList[AA])(implicit d: DummyImplicit): NonEmptyLazyList[AA] =
appendNell(nell)

@deprecated("Use overload with by-name args", "2.11.0")
final private[data] def ++:[AA >: A]()(ll: NonEmptyLazyList[AA]): NonEmptyLazyList[AA] =
++:(ll)

/**
* Prepends the given LazyList
*/
final def prependLazyList[AA >: A](c: LazyList[AA]): NonEmptyLazyList[AA] =
if (c.isEmpty) value
else create(c ++ toLazyList)
final def prependLazyList[AA >: A](ll: => LazyList[AA]): NonEmptyLazyList[AA] =
create(ll #::: toLazyList)

@deprecated("Use overload with by-name args", "2.11.0")
final private[data] def prependLazyList[AA >: A]()(ll: LazyList[AA]): NonEmptyLazyList[AA] =
prependLazyList(ll)

/**
* Alias for `prependLazyList`
*/
final def ++:[AA >: A](ll: => LazyList[AA]): NonEmptyLazyList[AA] =
prependLazyList(ll)

/**
* Prepends the given NonEmptyLazyList
*/
final def prependNell[AA >: A](c: NonEmptyLazyList[AA]): NonEmptyLazyList[AA] =
create(c.toLazyList ++ toLazyList)
final def prependNell[AA >: A](nell: => NonEmptyLazyList[AA]): NonEmptyLazyList[AA] =
create(nell.toLazyList #::: toLazyList)

@deprecated("Use overload with by-name args", "2.11.0")
final private[data] def prependNell[AA >: A]()(nell: NonEmptyLazyList[AA]): NonEmptyLazyList[AA] =
prependNell(nell)

/**
* Alias for `prependNell`
*/
final def ++:[AA >: A](c: NonEmptyLazyList[AA]): NonEmptyLazyList[AA] =
prependNell(c)
final def ++:[AA >: A](nell: => NonEmptyLazyList[AA])(implicit d: DummyImplicit): NonEmptyLazyList[AA] =
prependNell(nell)

/**
* Converts this NonEmptyLazyList to a `NonEmptyList`.
Expand Down
20 changes: 20 additions & 0 deletions tests/shared/src/test/scala-2.13+/cats/tests/NonEmptyLazyListSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,26 @@ class NonEmptyLazyListSuite extends NonEmptyCollectionSuite[LazyList, NonEmptyLa

(1 to 100).sum === sum
}

test("NonEmptyLazyList#appendLazyList is properly lazy") {
var evaluated = false
val ll = { evaluated = true; 1 } #:: LazyList.empty
val _ = NonEmptyLazyList(0).appendLazyList(ll)
assert(!evaluated)
}

test("NonEmptyLazyList#prependLazyList is properly lazy") {
var evaluated = false
val ll = { evaluated = true; 0 } #:: LazyList.empty
val _ = NonEmptyLazyList(1).prependLazyList(ll)
assert(!evaluated)
}

test("NonEmptyLazyList.apply is properly lazy") {
var evaluated = false
val _ = NonEmptyLazyList({ evaluated = true; 0 }, Nil: _*)
assert(!evaluated)
}
}

class ReducibleNonEmptyLazyListSuite extends ReducibleSuite[NonEmptyLazyList]("NonEmptyLazyList") {
Expand Down