Skip to content

Fix keys of a filtered indexed Seq iterated in reverse - #2309

Merged
bdurrer merged 2 commits into
immutable-js:mainfrom
b84330808:fix/filter-reverse-indexed-keys
Nov 2, 2026
Merged

bdurrer merged 2 commits into
immutable-js:mainfrom
b84330808:fix/filter-reverse-indexed-keys

Conversation

@b84330808

Copy link
Copy Markdown
Contributor

A filtered indexed Seq renumbers its keys from 0, but when it was iterated backwards the counter still started at 0, so the last element got key 0 and every "last" helper returned positions counted from the end:

const seq = Seq([1, 2, 3, 1, 5]).filter((x) => x !== 2); // [1, 3, 1, 5]
seq.lastIndexOf(1);                 // 1, expected 2
seq.findLastIndex((x) => x === 3);  // 2, expected 1

List is fine because its filter is eager. This is the same symptom #2268 fixed for reverse(): here the filter counts down from its size, materialized with ensureSize the same way. The regression test covers both the __iterate path (reduceRight, findLast*) and the __iterator path (toKeyedSeq().reverse().entries()).

I used Claude Code to write this change.

A filtered indexed Seq renumbers its keys from 0, but when it was
iterated backwards the counter still started at 0, so the last element
got key 0. lastIndexOf, findLastIndex, findLastKey, findLastEntry and
reduceRight then returned positions counted from the end, e.g.
Seq([1, 2, 3, 1, 5]).filter((x) => x !== 2).lastIndexOf(1) was 1
instead of 2.

Count down from the filtered size instead, materialized with
ensureSize like reverseFactory does.

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

bdurrer commented Oct 30, 2026

Copy link
Copy Markdown
Contributor

This sounds a bug indeed, Thanks for your tests and MRs around this!

Comment thread src/Operations.js Resolved
@bdurrer

bdurrer commented Nov 1, 2026

Copy link
Copy Markdown
Contributor

Hm, it fails at least one test

@b84330808

Copy link
Copy Markdown
Contributor Author

That's the toJS isomorphic value property test in Conversion.ts: fast-check happened to generate {"__proto__": 0}, and toJS() doesn't keep a __proto__ key. It's random input and unrelated to this change, which only touches the reversed filter in Operations.js, so a re-run should pass. Happy to look at the __proto__ case separately if you want.

@bdurrer
bdurrer merged commit a882941 into immutable-js:main Nov 2, 2026
9 of 10 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.

2 participants