SinceLazyList is not strict, there is not reason to makerange delegate throughfrom(NumericRange) which doesn't allow more thanInt.MaxValue elements.

BalmungSan added2 commits

April 29, 2024 19:06

Override LazyList.range

6de2bf8

Test LazyList.range

91088b5

BalmungSan requested a review fromNthPortal as acode owner

April 30, 2024 00:24

scala-jenkins added this to the2.13.15 milestone

Apr 30, 2024

SethTisue requested a review froma team

April 30, 2024 00:28

SethTisue added the library:collectionsPRs involving changes to the standard collection library label

Apr 30, 2024

som-snytt reviewed

Apr 30, 2024

View reviewed changes

test/junit/scala/collection/immutable/LazyListTest.scala Outdated

		.range(start = 0L, end = totalElements)
		.foldLeft(0L) { case (acc, _) =>
		acc + 1L
		}

Copy link

Contributor

Choose a reason for hiding this comment

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

even with inlining turned on, it's a lot to ask of a unit test. Maybe it's sufficient to test construction and head.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Yeah I was wondering the same.
However, not sure if just asking for the head ensures there are more thanInt.MaxValue elements.

So not sure if there is a smart way to test this without it being a CPU trap.

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Using a reverserange we don't need to compute a lot of elements.
Let me know what you think of the new test.
C.C.@NthPortal

som-snytt changed the title~~FixLazyList.range~~FixLazyList.range [ci: last-only]

Apr 30, 2024

Copy link

Contributor

som-snytt commentedApr 30, 2024

added the magic title to avoid testing every commit

BalmungSan commented

Apr 30, 2024

View reviewed changes

src/library/scala/collection/immutable/LazyList.scala OutdatedShow resolvedHide resolved

test/junit/scala/collection/immutable/LazyListTest.scala Outdated

		.range(start = 0L, end = totalElements)
		.foldLeft(0L) { case (acc, _) =>
		acc + 1L
		}

Copy link

ContributorAuthor

Choose a reason for hiding this comment

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

Yeah I was wondering the same.
However, not sure if just asking for the head ensures there are more thanInt.MaxValue elements.

So not sure if there is a smart way to test this without it being a CPU trap.

NthPortal reviewed

Apr 30, 2024

View reviewed changes

src/library/scala/collection/immutable/LazyList.scala OutdatedShow resolvedHide resolved

BalmungSanand others added2 commits

April 30, 2024 11:32

Better LazyList.range impl

bd3db0d

Co-authored-by: Marissa | April <7505383+NthPortal@users.noreply.github.com>

Move LazyList.range implementation closer to other overrides

f7745a4

BalmungSan requested review fromsom-snytt andNthPortal

April 30, 2024 16:34

NthPortal requested changes

Apr 30, 2024

View reviewed changes

Copy link

Contributor

NthPortal left a comment

Choose a reason for hiding this comment

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

just two small changes; otherwise lgtm.

I agree with@som-snytt's concern about the cost of iterating overInt.MaxValue elements, though I'm not really sure what the alternative is. I will defer to others' judgement about whether or not to just skip it

src/library/scala/collection/immutable/LazyList.scala OutdatedShow resolvedHide resolved

BalmungSanand others added2 commits

April 30, 2024 14:44

Ensure lazyness ofIntegral[A].one for overridedrange implementa…

2028dae

…tionCo-authored-by: Marissa | April <7505383+NthPortal@users.noreply.github.com>

Ensure step can't be zero

fbc2623

Co-authored-by: Marissa | April <7505383+NthPortal@users.noreply.github.com>

BalmungSan commented

Apr 30, 2024

View reviewed changes

src/library/scala/collection/immutable/LazyList.scala Outdated

		}

		private[this] def rangeImpl[A](start: A, end: A, step: A)(implicit ev: Integral[A]): State[A] =
		if (ev.lt(start, end)) sCons(start, newLL(rangeImpl(ev.plus(start, step), end, step)))

Copy link

ContributorAuthor

BalmungSanApr 30, 2024

Choose a reason for hiding this comment

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

Uhm, wait, this does not allow reversed ranges, does it? LikeLazyList.range(start = 100, end = 0, step = -2)
I guess rather than check forlt we have to check for!ev.equiv(start, end)?

Copy link

ContributorAuthor

BalmungSanApr 30, 2024

Choose a reason for hiding this comment

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

Also, this made me think. This could be a good way to test this without a CPU trap. We just request the reversed range and then we can indeed ask for thehead and check it is bigger thanInt.MaxValue

Copy link

Contributor

NthPortalApr 30, 2024

Choose a reason for hiding this comment

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

good point, the check is incorrect, hmm. probably need to check the sign or something as well. I'll have another look shortly

Copy link

Contributor

NthPortalApr 30, 2024

Choose a reason for hiding this comment

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

my thought is to check the sign ofstep in the 3 parameter overload ofrange, and then pass it as aBoolean parameter (probably namedpositiveStep) torangeImpl, thus checking the condition only once rather than for each element

Copy link

ContributorAuthor

BalmungSanApr 30, 2024•
edited
Loading

Choose a reason for hiding this comment

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

I just pushed an implementation that allows for reverseranges.
Let me know what you think about it.

BalmungSan added2 commits

April 30, 2024 17:45

Allow reverse LazyList.range

1ac4f64

Make the lazyRangeAllowsMoreThanIntMaxValue test run fast

b4eb90a

BalmungSan requested a review fromNthPortal

April 30, 2024 22:48

Copy link

Member

SethTisue commentedJul 18, 2024

@BalmungSan have you looked at the MiMa failures...?

SethTisue modified the milestones:2.13.15,2.13.16

Aug 14, 2024

Copy link

ContributorAuthor

BalmungSan commentedAug 24, 2024•
edited
Loading

@SethTisue hey, thanks for the pointer, I actually haven't noticed about the MiMa errors and was thinking the reason why the CI was red was that it was testing each commit.
Should I add the suggested filters? Or does that mean the changes can't be made until a future stdlib version?

Copy link

Member

SethTisue commentedSep 3, 2024

It looks to me like we're running into the problem described athttps://github.com/scala/improvement-proposals/blob/main/content/drop-stdlib-forwards-bin-compat.md#adding-overrides-for-performance . The overridden methods have return types ofCC which erases toObject, but the overrides have return types ofLazyList which erases toLazyList.

SIP-51 is accepted and has been implemented in at least one build tool (sbt), but we haven't actually started taking advantage of it yet.

At the moment we are still trying to ship 2.13.15, but once that is done, we can consider whether 2.13.16 is the release where we start actually using the new freedom.