Skip to content

feat(ts): read zips by index as std does, and flatten's held inner iterators - #446

Merged
moq-bot[bot] merged 9 commits into
mainfrom
ts-drop-order-zips
Nov 3, 2026
Merged

moq-bot[bot] merged 9 commits into
mainfrom
ts-drop-order-zips

Conversation

@kixelated

@kixelated kixelated commented Nov 2, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

ts-drop-order listed three refusals where std's drop order was one the runtime couldn't follow:

  • A skip of a zip of two TrustedRandomAccess iterators of items that drop (reject fixture skipped_zip).
  • A collect in place of such a zip whose closures own values that drop (zip_in_place).
  • A flatten or flat_map whose inner iterators hold items once they end.

Approach

Flatten. The refusal is deleted. FlattenFold already modeled std's try_fold, which keeps an ended inner iterator until the next one replaces it. The one bug was TakeIter.folded's back-trim shortcut, which take(1).rfind reached directly. std's Take::try_rfold reads nth_back(len - n - 1) and drops the result, where next_back reads nth_back(len - n).

Zips by index. rt.Iter gains indexed(), a view of the iterator as std reads it by __iterator_get_unchecked. The quest called this "a reader by index beside folded". A view's advanceBackBy passes items without reading them, as index reads never touch them. Each reader by index goes through it:

  • A TRA zip's next, nth, and next_back, and so its folds.
  • A TrustedRandomAccessNoCoerce zip's fold, now rt.Trusted "index", distinct from a TrustedLen zip's "len".
  • take's TRA spec_fold: iterTake gains an indexes flag. The flag is set only where MAY_HAVE_SIDE_EFFECT holds, since otherwise the read is the same.
  • collect in place of a NoCoerce zip: rt.Take "index" reads the zip by index, then drops it whole, inner closures first.

Which adaptors override indexed():

  • Map: never calls f for an item it passes.
  • Enumerate and Fuse: pass the view through.
  • Skip: makes and drops each skipped item whole as it reads index 0, from either end.
  • The TRA zip: reads each side at its index, never past the shorter one, and drops nothing from the longer side.

The rule "indexed() !== this iff the iterator may have side effects" mirrors MAY_HAVE_SIDE_EFFECT.

It also fixes two pre-existing miscompiles that sat outside the refusals:

  • A zip of zips with unequal inner sides, read from the back, made and dropped the inner longer side's excess. std never reads it.
  • A skip of a non-zip, read by index from the back, never made its prefix. std makes and drops it at index 0.

SkipIter.folded drops its shortcut while it still skips, because std's try_fold is nth(n - 1) plus a whole read, which differs from nth(n) for a TRA zip.

Impact

  • Translation: both zip refusals and the flatten refusal are gone, and the reject fixture loses skipped_zip and zip_in_place. Only the B-tree collect is left in the quest. feat(ts): drop the closures std's consumers and entries own where std's do #438 removes the closures item.

  • Public API: additive runtime surface only, no MIGRATE.md entry. That surface is Iter.indexed(), "index" in rt.Trusted and rt.Take, and iterTake's optional indexes. Generated output changes where a NoCoerce zip now passes "index" and a side-effecting TRA take passes true.

  • Tests: two new task steps, both differential against native wasm32:

    • flattened, 10 scenarios.
    • indexed, 9 scenarios: by index from the front, the back, nth, take folds, NoCoerce folds, adaptors, uneven sides, and in-place collects.
  • Mutation checks: I reverted each runtime piece in turn, 17 in all; every one fails the task fixture. The lang fixture caught a bug the task fixture couldn't: a short-circuited dropA?.(a.next()) that skipped the read where nothing drops.

  • Speed: on Bun, a micro-measure of the old runtime against the new over 4096 bytes, the new is no slower:

    • mapped TRA zip, forward: 13-22 ms vs 21-23 ms
    • workload 7's by-index back: 31 ms vs 44-47 ms
    • mapped back: 21-23 ms vs 29 ms

    The codec bench gains iter/zip-map-filter-count, a forward by-index zip of mapped sides. The browser bench (just ts bench) was not run.

Alternatives

  • A full at(i) random-access read on every TRA class: about 20 std types, and more surface. The view keeps sequential iterators and overrides only the six adaptors whose by-index read differs.
  • Keeping narrower refusals for uneven inner zips. Dropped: the view handles them.

Follow-ups

None new. The B-tree collect stays with the sort port.

Review

mechanical fix by codex / Codex: Merged base and regenerated conflicting rejection snapshots without behavior changes; passed in ts/ with pinned nightly: UPDATE_EXPECT=1 cargo test --locked --test fixtures reject -- --exact; UPDATE_EXPECT=1 cargo test --locked --test fixtures census -- --exact; cargo test --locked; cargo fmt --all --check; tsc -p runtime; tsc --noEmit --strict --target esnext --module esnext --moduleResolution bundler --allowImportingTsExtensions --verbatimModuleSyntax --skipLibCheck bench/iter.ts bench/workloa (head 5ec48e0)

Local quest review: Codex, gpt-6.1-sol, medium effort. Round 1 on head 3ae00f48ddf548d0d5e869cdb7c4fe493594e6f4: no findings.

Checks: just ci passes locally on 3b4b9313. 3ae00f48 adds only the quest's PR link (quest check ok).

Merged origin/main through #462 and #459 (iter.rs's zip and flatten helpers now live in library.rs; SkipIter.folded takes main's version). Local quest review round 1 on a50060d9: an indexed Take fold read past its iterator's size (skip(4).take(1).fold made and dropped four pairs; native made none). Reproduced in the task fixture's indexed step 3, fixed by clamping the limit to the iterator's length as std's spec_fold does. Round 2 on d68f0adc: no findings. just ci passes locally on d68f0adc.

(written by Claude Opus 5.5)

🤖 Generated with Claude Code

kixelated and others added 4 commits November 2, 2026 05:39
…e they end

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…ect in place translate

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated marked this pull request as ready for review November 2, 2026 16:03
@coderabbitai

coderabbitai Bot commented Nov 2, 2026 •

Copy link
Copy Markdown

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 28 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

Run configuration
  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: bab03835-87e6-4f4e-98c7-0ff941103730
Commits

Reviewing files that changed from the base of the PR and between 00a273a and d68f0ad.

Files selected for processing: 27
  • quest/m3/README.md
  • quest/m3/ts-drop-order.md
  • ts/bench/iter.ts
  • ts/bench/workloads.ts
  • ts/fixtures/codec/expected/bench.ts
  • ts/fixtures/codec/src/bench.rs
  • ts/fixtures/lang/expected/iters.ts
  • ts/fixtures/lang/expected/lengths.ts
  • ts/fixtures/lang/expected/strs.ts
  • ts/fixtures/reject/census.md
  • ts/fixtures/reject/expected.txt
  • ts/fixtures/reject/src/lib.rs
  • ts/fixtures/reject/ts.toml
  • ts/fixtures/task/expected/core/option.ts
  • ts/fixtures/task/expected/drops.ts
  • ts/fixtures/task/expected/drops/flattened.ts
  • ts/fixtures/task/expected/drops/folds.ts
  • ts/fixtures/task/expected/drops/indexed.ts
  • ts/fixtures/task/expected/lib.ts
  • ts/fixtures/task/src/drops.rs
  • ts/fixtures/task/src/drops/flattened.rs
  • ts/fixtures/task/src/drops/indexed.rs
  • ts/fixtures/task/src/harness.rs
  • ts/fixtures/task/ts.toml
  • ts/runtime/rt/iter.ts
  • ts/src/lower/iter.rs
  • ts/tests/task.ts
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

CodeRabbit Verify

  • Run CodeRabbit Verify to test the changes in this pull request.

@kixelated
kixelated enabled auto-merge (squash) November 2, 2026 16:24
@moq-bot

moq-bot Bot commented Nov 2, 2026

Copy link
Copy Markdown

quest land: this pull request conflicts with a pull request queued ahead of it, in these files:

  • #424: ts/fixtures/reject/census.md, ts/fixtures/reject/expected.txt

Once one of them lands, this one conflicts with the base and is ejected when its turn comes. Resolve the overlap before then, such as by coordinating with that author, or by merging the base once it lands.

@moq-bot
moq-bot Bot disabled auto-merge November 2, 2026 17:14
@moq-bot
moq-bot Bot enabled auto-merge (squash) November 2, 2026 17:14
@moq-bot

moq-bot Bot commented Nov 2, 2026

Copy link
Copy Markdown

codex / Codex land fixer note:

Failure log, line 1:

Conflicts with the base: quest/m3/ts-drop-order.md, ts/fixtures/reject/census.md, ts/fixtures/reject/expected.txt, ts/fixtures/task/expected/drops.ts, ts/fixtures/task/src/drops.rs, ts/fixtures/task/src/harness.rs, ts/fixtures/task/ts.toml, ts/src/lower/iter.rs, ts/tests/task.ts

The PR changes the same iterator lowering, drop fixtures, and documentation changed on base. A trial merge preserved both sets of additions and regenerated the affected snapshots. The trial passed cargo build --locked --manifest-path ts/Cargo.toml, cargo clippy --locked --manifest-path ts/Cargo.toml --all-targets -- -D warnings, cargo fmt --manifest-path ts/Cargo.toml --all --check, cargo test --locked --manifest-path ts/Cargo.toml --test runtime (8 tests), and tsc -p ts/runtime. The changed benchmark modules also passed an independent strict TypeScript check. These Rust commands used the installed nightly-2026-10-15 toolchain.

The complete benchmark check, tsc -p ts/bench, failed with error TS2688: Cannot find type definition file for 'bun'. Its configured ts/target/moq checkout is absent. Fetching that benchmark checkout is outside the permitted dependency-fetch scope. The snapshot regeneration and fixture test run passed task, census, source_order, and sysroot before being stopped after this validation blocker was confirmed; the full fixture suite did not finish.

The trial merge was aborted and pr-446 remains at its pinned head. No fix is reported because full validation could not be completed.

(written by codex / Codex)

@moq-bot
moq-bot Bot disabled auto-merge November 2, 2026 18:35
@moq-bot

moq-bot Bot commented Nov 2, 2026

Copy link
Copy Markdown

codex / Codex land fixer: Nine conflicts with base; full benchmark validation is blocked: &#96;tsc -p ts/bench&#96; reports &#96;error TS2688: Cannot find type definition file for 'bun'.&#96;. Recommendation: Provide the pinned moq checkout and its Bun type dependencies, then repeat the mechanical merge resolution, regenerate snapshots, and complete build and test validation.

Conflicts with the base: quest/m3/ts-drop-order.md, ts/fixtures/reject/census.md, ts/fixtures/reject/expected.txt, ts/fixtures/task/expected/drops.ts, ts/fixtures/task/src/drops.rs, ts/fixtures/task/src/harness.rs, ts/fixtures/task/ts.toml, ts/src/lower/iter.rs, ts/tests/task.ts

(written by codex / Codex)

kixelated and others added 4 commits November 2, 2026 16:35
Take's and Skip's folds keep main's back trim read once per fold, with Take's
by-index front; Take no longer caches its fold's view, whose back now holds
per-fold state. collect_take returns the in-place source's element size.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated enabled auto-merge (squash) November 3, 2026 01:05
@moq-bot
moq-bot Bot merged commit 16d45fe into main Nov 3, 2026
5 checks passed
@moq-bot
moq-bot Bot deleted the ts-drop-order-zips branch November 3, 2026 01:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant