Skip to content

feat(ts): lend std's &mut to values JS copies as rt.Places - #149

Merged
kixelated merged 20 commits into
mainfrom
quest/m2/ts-model/std-places
Oct 28, 2026
Merged

kixelated merged 20 commits into
mainfrom
quest/m2/ts-model/std-places

Conversation

@kixelated

@kixelated kixelated commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

Completes quest/m2/ts-model/std-places.md, which this PR deletes.

Problem

#128 gave a &mut to a value JS copies an rt.Place, but refused every std call that makes one, yields one, or hands one to a closure, since std's mappings had the value instead. moq-net's census still hit that guard 68 times (Option::get_or_insert 26, as_mut 13, iter_mut/into_iter/find/zip 13, get_or_insert_with/_default 6, insert 2, values_mut, slice::get_mut, last_mut, ...), and kio once (slice::get_mut). Consumer: moq-net's model.

Approach

  • Option's &mut self methods lower in call.rs's intercepts, as mem::take does, on the path of the Option the &mut borrows:
    • get_or_insert, get_or_insert_with, get_or_insert_default, and insert all store in place: if (o === undefined) o = v; else drop(v). The payload they return is an alias of o as T whose stores rebuild the Option, as Cow::to_mut's does. Passed on or kept, it is an rt.Place.
    • They drop what they don't keep, as std does: the value get_or_insert doesn't insert, the closure get_or_insert_with doesn't call, and what insert replaces. A closure it calls drops as FnOnce does.
    • as_mut and iter_mut of a copied payload give the payload's rt.Place, or undefined.
    • into_iter of a &mut Option (for x in &mut o) yields rt.optionAsMut(p): the Option's own place while it is Some, as Some(x) is x.
    • Each tests for the payload's own None: rt.None where the payload can be undefined, after #134.
  • Slices: get_mut with an index, first_mut, and last_mut map to rt.sliceGetMut/FirstMut/LastMut, which lend an AtIndex place. Elements that are objects map to get, first, and the new sliceLast, which were refused before.
  • Iterators that lend places: slice iter_mut, and into_iter of a &mut slice, array, or Vec, make rt.iterSliceMut. HashMap/BTreeMap iter_mut, values_mut, and BTreeMap::range_mut, plus VecDeque iter_mut, get_mut, front_mut, and back_mut, get *Mut runtime functions. Each iterator makes its places lazily, as it reaches them: a B-tree's and a hash map's through a new PLACES pick, and a deque's through DequePlaces (from review). A stepped for loop leaves such iterators to the runtime.
  • Closures handed places: Entry::and_modify, map retain, and VecDeque::retain_mut pass the place. Each retain shares one body with its value form, and reads the value after f writes it.
  • The guard (std_places) now lets a mapping through when it lends places itself, or when its places come from what it was given. That means an iterator that yields them, or a value holding them, such as Vec<&mut u32>::into_iter, but not a &mut that is itself a place (from review). Iterator max/min over places is refused ("unsupported call"), since the runtime would compare the objects.
  • stable_path looks through the as T of a lent payload.
  • A store through a &mut to a tuple copies its parts over in place (Object.assign). #124 made a tuple a JS array changed in place, so hash.iter_mut() of tuple values yields the array itself, and *x = (a, b) had rebound the iterator item's slot instead.

Proof

  • lang's refs (differential against wasm32, 50,000 cases):
    • Each Option method on a local, a field, and an array element, with the payload passed on and kept. Option<(u8, u8)> payloads go through filter, map, and_then, and map_or closures, lowered from core's MIR.
    • Slice first_mut/last_mut/get_mut of u32 and of bytes.
    • iter_mut with rev, step_by, enumerate, zip, find, max_by_key, filter, and collect, plus for x in &mut v and nested arrays.
    • Hash maps keyed by numbers (of u32 values, which AtKey lends) and by tuples, which the runtime chains by hash, through iter_mut, values_mut, retain, and and_modify.
    • B-trees through iter_mut, values_mut().rev(), range_mut, retain, and and_modify.
    • Deques through iter_mut, get_mut, front_mut, back_mut, and retain_mut.
    • for x in &mut o and (&mut o).into_iter().count() of Some and None (from review). Breaking optionAsMut for None fails it with 861 differences.
    • for p in &mut o and (&mut o).into_iter().count() of an Option<(u8, u8)> and an Option<&mut u8>, Some and None, whose payloads JS doesn't copy (from review).
  • An Option<Option<u8>>, whose None is rt.None, through get_or_insert, as_mut, insert, and &mut iteration.
    • A mutation check: sliceLastMut pointed at index 0 fails the test with 768 differences.
  • *x = (x.1, x.0) through HashMap<u8, (u8, u8)>::iter_mut failed with 726 differences after merging feat(ts): translate Bound, Reverse, NonZero, and Vec's editing and sorting methods #124, and passes with the tuple store. swap_pair also replaces its whole tuple through the parameter.
  • task's options drop scenario, through the differential test and the host unwinding test, covers:
    • get_or_insert dropping the value it doesn't insert.
    • get_or_insert_with dropping the closure it doesn't call, and panicking inside the one it calls.
    • insert dropping what it replaces.
    • An Option<Result<Guard, u8>>, whose payload is a Place, as moq's answered is.
  • reject drops increment, last_bumped, and modified, which now translate. places_equal stays refused.

Measurements

  • just ts census: moq-net's refusals fall from 9,328 to 9,260. Its std guard refusals fall from 68 to 5, and kio's from 1 to 0. The 5 left are:
    • hashlink::LinkedHashMap::get_mut (2): a third-party crate, otherwise unsupported.
    • Pin::as_mut (3): over Pin<Box<dyn Future>>, and dyn is refused anyway.
  • just ts moq: the translated codec is byte-identical to main's, apart from rt.ts, rt/iter.ts, and rt/collections.ts. Source maps differ only by checkout path. That code needs no Place and gets none, so the just ts bench timings can't move, and I didn't rerun it.

Impact

Public API impact is in the translator's output and runtime only:

  • Calls that were refused now translate, as does a store of a whole tuple through a &mut. Nothing that translated before changes shape, so there is no MIGRATE.md entry.
  • rt.ts adds optionAsMut, sliceLast, sliceGetMut, sliceFirstMut, and sliceLastMut. rt/iter.ts adds iterSliceMut.
  • rt/collections.ts adds:
    • hashIterMut, hashValuesMut, and hashRetainMut.
    • btreeIterMut, btreeValuesMut, btreeRangeMut, and btreeRetainMut.
    • dequeIterMut, dequeGetMut, dequeEndMut, and dequeRetainMut.
    • entryAndModifyMut.
    • AtIndex, now exported for rt/iter.ts.

Choices made without asking

  • Comparing places stays refused. This covers Some(a) == Some(b) of Option<&mut u32> (places_equal) and iterator max/min over places. The quest's Plan listed comparison as "also refused", but its Goal covers the std methods that hand out a &mut, and moq has no such comparison. I deleted the quest rather than rewriting it to this.
  • A &mut to a local assigned more than once makes an rt.place per call (e.g. seen.get_or_insert(..) where seen is reassigned elsewhere). This follows feat(ts): translate a &mut to a value JS copies as an rt.Place #128's existing rule for such borrows, and isn't special to these methods.

Alternatives

  • Lower Option's &mut self methods from core's MIR. core's bodies borrow &mut and use unwrap_unchecked, which feat(ts): lower Option's and Result's combinators from core's MIR #129's std_refused refuses, so this would mean opening std bodies to writes. The intercepts are a few lines each and need no closures for an alias.
  • Return the receiver's own rt.Place cast to the payload's, as Option<T> is T in JS. This is cheaper where the receiver is already a Place, but it adds a second path, and the general make_place covers it.

Follow-ups

  • Comparing and ordering Options, tuples, and iterators of Places through the runtime's key functions (places_equal, iter_mut().max()).
  • The guard's message names "its mapping" for calls into third-party crates (hashlink), which have no mapping. It should read "unsupported call".

🤖 Generated with Claude Code

(written by Claude Opus 5.5)

kixelated and others added 4 commits October 27, 2026 19:36
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated kixelated changed the title Std's mutable references to values JS copies feat(ts): lend std's &mut to values JS copies as rt.Places Oct 28, 2026
@kixelated
kixelated marked this pull request as ready for review October 28, 2026 06:52
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Oct 28, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-10-28T13:50:08.587967Z ac8cd03 New commits
🔒 Security Review ✅ Completed 2026-10-28T13:50:05.842921Z ac8cd03 New commits
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b63880caa9

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ts/src/lower/mir/call.rs Resolved

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Automated review by review (OpenAI)

Reviewed commit: b63880caa90b8f79edfcdfbe90bf97f6cd1aa54b.

The existing Place representation is a sensible direction, and reusing AtIndex/collection storage avoids a second mutable-reference model. The added mutation and drop fixtures cover useful interactions.

One actionable issue remains: I verified the borrowed-Option path identified in the independent finding. The exemption at ts/src/lower/mir/call.rs:368–370 admits &mut Option<u32>, but ts/src/lower/iter.rs:176–178 forwards the outer Place to iterOption, which tests the wrapper rather than its payload. Thus (&mut None::<u32>).into_iter().count() is represented as one item. Handle borrowed Option iteration conditionally like iter_mut, or retain the refusal, and cover None and mutation through Some. No duplicate inline comment added; no additional actionable bugs found.

Verification: static review of the changed lowering/runtime paths and fixture diffs only; I did not run the translator, differential tests, or benchmarks.

(Written by OpenAI)

Takes #134's rt.None: Option intercepts test a payload's None as its own, and
into_iter of a &mut Option lends the payload's place only while it is Some.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Automated review by review (OpenAI)

Reviewed commit: d776073d004003da01bee593014e4b75d38a4263, compared with my previous review of b63880caa90b8f79edfcdfbe90bf97f6cd1aa54b, accounting for the main-branch merge.

The previous borrowed-Option finding is fixed for copied payloads: optionAsMut tests the payload against the correct None sentinel before lending its Place. The new fixtures cover empty/nonempty iteration, writes through Some, and nested Options. The narrowed exemption and reuse of the existing Place are a focused, appropriately small correction; the Option intercepts also now respect main's rt.None representation.

No new actionable bugs found in the fix and integration delta. Verification was static inspection of lowering, runtime, and generated/source fixture diffs; I did not run tests or independently reproduce the reported differential-test results.

(Written by OpenAI)

# Conflicts:
#	ts/fixtures/reject/census.md
#	ts/fixtures/reject/expected.txt
#	ts/fixtures/reject/src/lib.rs
#	ts/fixtures/reject/ts.toml
@kixelated

Copy link
Copy Markdown
Collaborator Author

Merged main (#148, #145, #135) into the branch, at c1f97362. The reject fixture conflicted and I resolved it by hand:

  • src/lib.rs and ts.toml: feat(ts): translate Duration's compound assignment, larger units, and Sum #135 replaced duration_assign (which now translates) with parts_summed, and this PR removed last_bumped and modified. The resolution keeps parts_summed and drops all three.
  • expected.txt and census.md: regenerated with UPDATE_EXPECT=1. The census goes from main's 78 refusals to 75, which are the 3 std-guard refusals this PR removes.

just check, just ts check, just ts test, and quest check --base origin/main pass locally.

(written by Claude Opus 5.5)

# Conflicts:
#	ts/fixtures/reject/census.md
#	ts/fixtures/reject/expected.txt
#	ts/fixtures/task/expected/drops.ts
@kixelated

Copy link
Copy Markdown
Collaborator Author

Merged main again (#158, #139), at 901ed165. Only generated snapshots conflicted: reject/census.md, reject/expected.txt, and task/expected/drops.ts. I regenerated them with UPDATE_EXPECT=1 from the merged sources. The census goes from main's 72 refusals to 69, which are the same 3 std-guard refusals. just check, just ts check, just ts test (including the task differential), and quest check pass locally.

(written by Claude Opus 5.5)

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 901ed16521

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ts/src/lower/iter.rs Resolved
kixelated and others added 2 commits October 28, 2026 02:09
Resolve #124's overlap: both added `sliceLast`, so keep one, and drop
#124's `first_mut`/`last_mut` slice arms, which the place-lending arm
now shadows.

#124 makes a tuple a JS array changed in place, so `hash.iter_mut()` of
tuple values yields the value itself, not an `rt.Place`. `*x = (a, b)`
through it rebound the iterator item's slot instead of the map's value.
A store through a reference to a tuple now copies its parts over in
place, as `Object.assign` does for a class.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated

Copy link
Copy Markdown
Collaborator Author

Design change: none, but this head (82adebfe) holds more than a conflict-free merge, so it needs a new review.

Merging #124 broke lang's differential test (726 differences). #124 makes a tuple a JS array changed in place, so this PR's hash.iter_mut() of (u8, u8) values now yields the array itself rather than an rt.Place. *x = (x.1, x.0) then lowered to item[1] = [...], which rebinds the iterator item's slot and leaves the map unchanged. The fix is in store (ts/src/lower/mir/place.rs): a store through a reference to a tuple copies its parts over in place with Object.assign, as a class already does. Only that site and a new swap_pair case change in the generated output.

I also resolved the conflict in rt.ts, where both PRs added sliceLast (one copy kept), and dropped #124's first_mut/last_mut slice arms in library.rs, which this PR's place-lending arm shadows. just check, just ts test, and quest check pass.

(written by Claude Opus 5.5)

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Automated review by review (OpenAI)

Reviewed commit: 82adebfedb03ab4159a5dbc69ef9aa8f1cc82687, against my last published review at d776073d004003da01bee593014e4b75d38a4263, separating merged main changes from this PR's integration changes.

The new tuple replacement path (ts/src/lower/mir/place.rs:619–622) is consistent with main's object-backed tuples: copying fields into the existing tuple preserves the caller's reference. The whole-tuple replacement fixture exercises this integration, and keeping the mutable slice mappings ahead of the nonmutable ones is appropriate. No new actionable bugs found in this delta.

Still open, not a new finding: the independent reference-payload Option finding remains at ts/src/lower/iter.rs:177–190. &mut Option<&mut u32> bypasses optionAsMut, so an empty borrowed Option still reaches iterOption as its outer Place. The earlier copied-payload fix remains intact; borrowed Options need handling independent of whether their payload needs a Place. No duplicate inline comment added.

Verification: static source/generated-fixture inspection only; I did not run tests or benchmarks.

(Written by OpenAI)

…s value

`(&mut o).into_iter()` of an `Option` of a tuple or a `&mut` handed
`iterOption` the `Option`'s place, which yields one item even for
`None`. It now reads the `Option` through the place.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Automated review by review (OpenAI)

Reviewed commit: 90e72e54e9af2f0834f48be33c8cf8494fe961a5, compared with 82adebfedb03ab4159a5dbc69ef9aa8f1cc82687.

The remaining borrowed-Option finding is fixed: the plain Option branch now reads the outer Place through pointee_value, so iterOption receives the actual payload/None. This preserves owned/shared inputs and leaves the copied-payload lending path intact. Reusing that helper is the simpler correction; the new fixtures cover Some/None reference payload counts and mutation through tuple payloads.

No new actionable bugs found in this delta. Verification: static lowering and source/generated-fixture review only; tests were not run by me.

(Written by OpenAI)

# Conflicts:
#	ts/fixtures/lang/expected/core/option.ts
#	ts/fixtures/reject/census.md
#	ts/fixtures/reject/expected.txt
Fold #167's atomics into `place_call`'s table, with VecDeque's methods
as one more row at index 0 rather than an early return.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c6c922b521

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread ts/runtime/rt/collections.ts Outdated
Comment on lines +733 to +736
const entries: [K, Place<V>][] = [];
for (const key of m.p.keys()) entries.push([key, new AtKey(m.p, key)]);
for (const first of m.o?.values() ?? []) for (let s: Slot<K, V> | undefined = first; s !== undefined; s = s.next) entries.push([s.k, s]);
return iterSlice(entries);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Construct mutable hash iterator entries lazily

For large maps of scalar values, even map.values_mut().next() now traverses the entire map and allocates an entry array and a Place per primitive-key entry before yielding anything. Repeated short-lived iterations therefore incur O(n) extra memory per call and can turn O(n) total work into O(n²). In Node, constructing this iterator over 100,000 entries visited all 100,000 and took about 59 ms, versus about 0.06 ms for the existing lazy hashIter. Yield Places on demand from a hash iterator instead of collecting them up front; hashValuesMut also inherits this cost.

AGENTS.md reference: AGENTS.md:L15-L15

Useful? React with 👍 / 👎.

@kixelated kixelated Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in 312b0e7b. hashIterMut is now a HashIter with the PLACES pick, which makes each [key, place] as it is reached: an AtKey for a primitive key and the slot for a chained one. hashValuesMut maps it lazily. dequeIterMut had the same up-front cost, so it is now a lazy DequePlaces. With 100,000 entries, the first item now takes 0.001 ms instead of 3.7 ms, and a full pass takes 1.3 ms instead of 7.0 ms (hash) and 0.6 ms instead of 2.6 ms (deque). refs adds a HashMap<u8, u32> through iter_mut and values_mut, since no case reached the AtKey path after #124. Pointing that path at a copy of the map now fails the differential test with 768 differences.

(written by Claude Opus 5.5)

kixelated and others added 2 commits October 28, 2026 06:28
…ach them

`hashIterMut`, `hashValuesMut`, and `dequeIterMut` made every place
before yielding the first, O(n) for `values_mut().next()`. They now make
each as it is reached, through `HashIter`'s `PLACES` pick and a
`DequePlaces` iterator. lang's `refs` adds a map of `u8` keys and `u32`
values, whose places `AtKey` lends, which no case reached since #124
made the tuple values objects.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Automated review by review (OpenAI)

Reviewed commit: 6e037533cf803f099adc2761c4c6afb454bd7dfc, compared with my last published review at 90e72e54e9af2f0834f48be33c8cf8494fe961a5, accounting for merged main changes.

The independent eager-allocation finding is fixed: HashIter now creates an AtKey or lends a chained slot only when reached, and hashValuesMut remains lazy. DequePlaces likewise avoids upfront allocation while preserving front/back bounds and remaining length. Reusing HashIter is a focused improvement; the added scalar-key map fixture covers the previously unexercised AtKey mutation path. Earlier borrowed-Option fixes remain intact.

No new actionable bugs found in this delta. Verification: static runtime, lowering integration, and source/generated-fixture inspection only. I did not run tests or independently verify the reported benchmarks.

(Written by OpenAI)

@kixelated

Copy link
Copy Markdown
Collaborator Author

Merge summary: landing this as a squash once Codex and CI pass on ac8cd037.

Changes since the original review, all from merging main or from review:

just check, just ts test (including the differential and unwinding tests), and quest check --base origin/main pass locally on ac8cd037.

(written by Claude Opus 5.5)

@kixelated
kixelated merged commit c85d9e5 into main Oct 28, 2026
1 check passed
@kixelated
kixelated deleted the quest/m2/ts-model/std-places branch October 28, 2026 14:04
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