Skip to content

Fix takeLast(0) on a Seq of unknown size - #2313

Merged
bdurrer merged 1 commit into
immutable-js:mainfrom
b84330808:fix-slice-negative-zero-unknown-size
Nov 2, 2026
Merged

bdurrer merged 1 commit into
immutable-js:mainfrom
b84330808:fix-slice-negative-zero-unknown-size

Conversation

@b84330808

Copy link
Copy Markdown
Contributor

takeLast(0) returns the whole Seq when its size is unknown:

Seq([1, 2, 3]).filter(() => true).takeLast(0).toArray(); // [1, 2, 3], expected []
Seq([1, 2, 3]).takeLast(0).toArray(); // []

#1245 fixed this for known sizes: takeLast(0) calls slice(-0), and resolveIndex treats -0 as negative through isNeg. But sliceFactory decides whether to cache a Seq of unknown size with begin < 0 || end < 0, which is false for -0, so resolveBegin(-0, undefined) comes out as 0 and the slice starts at the beginning.

This uses isNeg for that check (now exported from TrieUtils). I applied it to end as well: slice(0, -0) on a Seq of unknown size returned [], while the same call on a Seq of known size returns everything; now both agree.

Zipped Seqs of unknown size also need #2311 for this, since they couldn't be cached before.

The test in __tests__/slice.ts fails on main and passes with this change. The same check is on 6.x.

I used Claude Code to write this change.

`takeLast(0)` slices from `-0`. `sliceFactory` only caches a Seq of
unknown size when `begin < 0 || end < 0`, which is false for `-0`, so
`resolveBegin` got an undefined size and resolved `-0` to 0. The whole
Seq came back instead of an empty one. `end` of `-0` had the same
problem. Use `isNeg`, which the index resolution already uses.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
@bdurrer

bdurrer commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

@jdeniau What's the target of the "Output diff for TS files" job? Verifying prettier formatting?

@jdeniau

jdeniau commented Nov 1, 2026

Copy link
Copy Markdown
Member

@bdurrer the purpose is to check that a file migrated to TS has the same output (and runtime) than main.
In your case it can be ignore if the purpose is to change Runtime!
It is here as I'm migrating files to TS from time to time and things that change the output should be on the 6.x branch, or reviewed with caution.

@bdurrer

bdurrer commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

Ah thanks for clearing that up. So yeah, LGTM, I'll approve this

@bdurrer
bdurrer merged commit 760c2d7 into immutable-js:main Nov 2, 2026
5 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants