add cycle method to LazyList

[SethTisue]

I'm looking for multiple kinds of review here:

1. is this the right way, in this repo, to add an extension method to an existing type? we need to have at least one demonstration of what the "right" way is
2. do we, in fact, want to add this method? (I do genuinely want to add it, as the tie-the-knot trick is non-obvious, and I have seen people get it wrong, or not realize that it's possible; see https://stackoverflow.com/questions/2097851/scala-repeat-a-finite-list-infinitely and https://www.reddit.com/r/scala/comments/eih0t7/does_213s_lazylist_have_an_api_like_haskells_cycle/)
3. why does testConstantMemory1 fail? @NthPortal is this as-expected?

if folks agree this should be added, I can do a better job on the Scaladoc

[NthPortal]

This does seem useful, though I don't like the fact that it requires you to construct a LazyList instance that doesn't get used (it doesn't bother me that much)

[NthPortal]

@SethTisue do you mind if I push some changes?

[SethTisue]

do you mind if I push some changes?

@NthPortal have at it

UPDATE: but note that I just rebased

[SethTisue]

Luis (@BalmungSan) noticed that I forgot extends AnyVal

[NthPortal]

1. I'm still thinking on this one. I think we might want it in a next sub-package?
2. I think it's quite valuable, and highly motivated given the links you provided.
3. The states were eq but not the LazyLists themselves; I've fixed the tests.

[SethTisue]

the Travis-CI .org to .com migration didn't happen smoothly here — separate ticket on that: #27

[SethTisue]

@NthPortal does the commit I just pushed line up with what you're proposing over at #24?

[NthPortal]

does the commit I just pushed line up with what you're proposing over at #24?

yup

[SethTisue]

@NthPortal additional commit pushed with more comments, more tests, more Scaladoc

one thing I discovered is that (even for finite xs) xs.cycle.cycle isn't itself a cycle — see line commented TODO. I'm thinking we probably don't care and it isn't worth adding a cyclical check to catch it? wdyt?

[NthPortal]

I don't know. I'm not sure what the cost of checking if it's a cycle is.

The more I look at this, the more I think cyclic LazyLists should be a separate type, but alas, it's a bit late for that. I think for now it's easiest not to worry about it? you could open a ticket and assign it to me and I'll see if I can figure it out at some point

[SethTisue]

you could open a ticket

meh, I'm fine with it as it stands

[NthPortal]

sorry. I'll try to open a ticket or something. tomorrow. if I remember

[SethTisue]

The more I look at this, the more I think cyclic LazyLists should be a separate type, but alas, it's a bit late for that

It depends on how deep one expects the support to go. For example, one might expect .map or .filter on a cycle to also return a cycle — they certainly could! But I don't think we want to go there.

It's me who (iirc) got us both thinking about cycles in the first place, before 2.13.0 was released — and honestly I was bit surprised that you ran with it and added the cycle-checking code to LazyList at all. I wouldn't say I regret that, but if we go one step further and add this cycle method, we probably need to limit user expectations in the Scaladoc for the new method. Like, do we document that tail and drop are cycle-preserving but other methods do not? Do we figure out exactly which methods are cycle-preserving, or just list a few that we know are cycle-preserving and issue some kind of "we don't know" disclaimer on the rest? Sigh.

[Ichoran]

@SethTisue - Actually, I think if we're going to support cycles at all, except as an accident, we should support them on map and filter and everything else. It's really not fun to have a feature that randomly explodes on you because you "used it wrong", despite using it wrong looking like ordinary correct usages.

[NthPortal]

honestly I was bit surprised that you ran with it and added the cycle-checking code to LazyList at all.

if I understand what you're referring to, I didn't end up doing that I don't think (other than what was already there in force and addString). what I added was a check for a self-referential (not necessarily cyclic) LazyList where you run out of elements but are still trying to derive something to append. I basically changed it from overflowing the stack to throwing a slightly nicer exception. unless I'm misunderstanding what you're talking about

[NthPortal]

we should support them on map and filter and everything else.

I would love to, but I genuinely do not think it's possible without leaking a ton of memory and/or taking a huge performance hit, which is also undesirable. perhaps we could add cycle-aware variants of operations?

I do think we should add documentation to the stdlib's docs (and have the docs for this point to that?) that most operations do not preserve cycles, and perhaps the few that do.

if we're going to support cycles at all, except as an accident

we have explicitly supported/handled cycles in force and addString/toString since the days of Stream (at least 2.11); we've just never documented or made an easy way to create them.

[SethTisue]

@NthPortal re: the history on this, I trust your memory over mine

perhaps we could add cycle-aware variants of operations?

I don't want to work on that, and think we probably don't want to clutter LazyList up with that. I lean towards the just-document-which-methods-are-known-to-preserve-cycles approach

Actually, I think if we're going to support cycles at all, except as an accident, we should support them on map and filter and everything else. It's really not fun to have a feature that randomly explodes on you because you "used it wrong", despite using it wrong looking like ordinary correct usages.

@Ichoran in practice, I don't think that's going to happen. I think our choices in practice are: add cycle, but document its limitations; or don't add it.

given that cycles are already "accidentally" supported, I feel like adding cycle is a good opportunity to: 1) make sure if that if someone wants a cycle they use our correct implementation instead of rolling their own, and 2) have an obvious, discoverable place to put documentation about how cycles are handled. so I lean towards going ahead with the addition, but it's only a mild leaning.

[Ichoran]

@NthPortal - Yes, I remember...I added the cycle-handling.

It would be a lot of programmer effort but not, I think, all that computationally expensive to add it to most methods, as long as we don't guarantee zero unrolling of loops. (If we did, we'd have to represent them explicitly.) If we instead guarantee at most one unroll of a loop, we'd spend at most 1/2 an extra traversal on non-cyclic LazyLists. Compared to the actual operation (map, etc.) which does a lot more work, that doesn't seem very expensive to me, especially on a collection that isn't really built for extreme performance to begin with.

I guess the real question about cycles with things like map and filter is whether the function is state-dependent. In general, it might be. So that would pretty much sink any attempt to re-cyclize the memoized version with the function applied.

Nonetheless, we should keep our eyes open for which methods might benefit from cycle handling in a non-problematic way.

[SethTisue]

added to Scaladoc:

+     *
+     * Note that some `LazyList` methods preserve cyclicality and others do not.
+     * So for example the `tail` of a cycle is still a cycle, but `map` and `filter`
+     * on a cycle do not return cycles.

I think I'm now satisfied with this.

[NthPortal]

looks pretty good to me; I think it needs just a few (mostly cosmetic) changes

[SethTisue]

review feedback, handled. also: squashed, rebased.

[NthPortal]

LGTM! ty Seth!

[SethTisue]

not for merge yet...

...since I'm not sure yet if we have sufficient consensus yet on actually adding this — I don't really know who watches this repo and who doesn't. so let's hold off on merging — I think we'll need to institute some (not-TOO-overly-formal) process for that involving signoff from both the Scala 2 team at Lightbend and the Scala 3 team at EPFL

[SethTisue]

Well this has now been sitting in limbo for a whole year since no one (including me) has stepped forward to formulate an approval process.

I'm going to close it to get it out of my personal PR list. We can find it again with the "status:pending" label.