Repository navigation
feat(ts): lend std's &mut to values JS copies as rt.Places - #149
Conversation
Quest-Claim: 58b81bbeadac2d98
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 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".
kixelated
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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
|
Merged main (#148, #145, #135) into the branch, at
(written by Claude Opus 5.5) |
# Conflicts: # ts/fixtures/reject/census.md # ts/fixtures/reject/expected.txt # ts/fixtures/task/expected/drops.ts
|
Merged main again (#158, #139), at (written by Claude Opus 5.5) |
There was a problem hiding this comment.
💡 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".
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>
|
Design change: none, but this head ( Merging #124 broke I also resolved the conflict in (written by Claude Opus 5.5) |
kixelated
left a comment
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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>
There was a problem hiding this comment.
💡 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".
| 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); |
There was a problem hiding this comment.
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 👍 / 👎.
There was a problem hiding this comment.
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)
…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
left a comment
There was a problem hiding this comment.
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)
|
Merge summary: landing this as a squash once Codex and CI pass on Changes since the original review, all from merging main or from review:
(written by Claude Opus 5.5) |
Completes quest/m2/ts-model/std-places.md, which this PR deletes.
Problem
#128 gave a
&mutto a value JS copies anrt.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_insert26,as_mut13,iter_mut/into_iter/find/zip13,get_or_insert_with/_default6,insert2,values_mut,slice::get_mut,last_mut, ...), and kio once (slice::get_mut). Consumer: moq-net's model.Approach
Option's&mut selfmethods lower incall.rs's intercepts, asmem::takedoes, on the path of theOptionthe&mutborrows:get_or_insert,get_or_insert_with,get_or_insert_default, andinsertall store in place:if (o === undefined) o = v; else drop(v). The payload they return is an alias ofo as Twhose stores rebuild theOption, asCow::to_mut's does. Passed on or kept, it is anrt.Place.get_or_insertdoesn't insert, the closureget_or_insert_withdoesn't call, and whatinsertreplaces. A closure it calls drops asFnOncedoes.as_mutanditer_mutof a copied payload give the payload'srt.Place, orundefined.into_iterof a&mut Option(for x in &mut o) yieldsrt.optionAsMut(p): theOption's own place while it isSome, asSome(x)isx.None:rt.Nonewhere the payload can beundefined, after #134.get_mutwith an index,first_mut, andlast_mutmap tort.sliceGetMut/FirstMut/LastMut, which lend anAtIndexplace. Elements that are objects map toget,first, and the newsliceLast, which were refused before.iter_mut, andinto_iterof a&mutslice, array, orVec, makert.iterSliceMut.HashMap/BTreeMapiter_mut,values_mut, andBTreeMap::range_mut, plusVecDequeiter_mut,get_mut,front_mut, andback_mut, get*Mutruntime functions. Each iterator makes its places lazily, as it reaches them: a B-tree's and a hash map's through a newPLACESpick, and a deque's throughDequePlaces(from review). A steppedforloop leaves such iterators to the runtime.Entry::and_modify, mapretain, andVecDeque::retain_mutpass the place. Eachretainshares one body with its value form, and reads the value afterfwrites it.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 asVec<&mut u32>::into_iter, but not a&mutthat is itself a place (from review). Iteratormax/minover places is refused ("unsupported call"), since the runtime would compare the objects.stable_pathlooks through theas Tof a lent payload.&mutto a tuple copies its parts over in place (Object.assign). #124 made a tuple a JS array changed in place, sohash.iter_mut()of tuple values yields the array itself, and*x = (a, b)had rebound the iterator item's slot instead.Proof
lang'srefs(differential against wasm32, 50,000 cases):Optionmethod on a local, a field, and an array element, with the payload passed on and kept.Option<(u8, u8)>payloads go throughfilter,map,and_then, andmap_orclosures, lowered from core's MIR.first_mut/last_mut/get_mutofu32and of bytes.iter_mutwithrev,step_by,enumerate,zip,find,max_by_key,filter, andcollect, plusfor x in &mut vand nested arrays.u32values, whichAtKeylends) and by tuples, which the runtime chains by hash, throughiter_mut,values_mut,retain, andand_modify.iter_mut,values_mut().rev(),range_mut,retain, andand_modify.iter_mut,get_mut,front_mut,back_mut, andretain_mut.for x in &mut oand(&mut o).into_iter().count()ofSomeandNone(from review). BreakingoptionAsMutforNonefails it with 861 differences.for p in &mut oand(&mut o).into_iter().count()of anOption<(u8, u8)>and anOption<&mut u8>,SomeandNone, whose payloads JS doesn't copy (from review).Option<Option<u8>>, whoseNoneisrt.None, throughget_or_insert,as_mut,insert, and&mutiteration.sliceLastMutpointed at index 0 fails the test with 768 differences.*x = (x.1, x.0)throughHashMap<u8, (u8, u8)>::iter_mutfailed 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_pairalso replaces its whole tuple through the parameter.task'soptionsdrop scenario, through the differential test and the host unwinding test, covers:get_or_insertdropping the value it doesn't insert.get_or_insert_withdropping the closure it doesn't call, and panicking inside the one it calls.insertdropping what it replaces.Option<Result<Guard, u8>>, whose payload is a Place, as moq'sansweredis.rejectdropsincrement,last_bumped, andmodified, which now translate.places_equalstays 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): overPin<Box<dyn Future>>, anddynis refused anyway.just ts moq: the translated codec is byte-identical to main's, apart fromrt.ts,rt/iter.ts, andrt/collections.ts. Source maps differ only by checkout path. That code needs no Place and gets none, so thejust ts benchtimings can't move, and I didn't rerun it.Impact
Public API impact is in the translator's output and runtime only:
&mut. Nothing that translated before changes shape, so there is noMIGRATE.mdentry.rt.tsaddsoptionAsMut,sliceLast,sliceGetMut,sliceFirstMut, andsliceLastMut.rt/iter.tsaddsiterSliceMut.rt/collections.tsadds:hashIterMut,hashValuesMut, andhashRetainMut.btreeIterMut,btreeValuesMut,btreeRangeMut, andbtreeRetainMut.dequeIterMut,dequeGetMut,dequeEndMut, anddequeRetainMut.entryAndModifyMut.AtIndex, now exported forrt/iter.ts.Choices made without asking
Some(a) == Some(b)ofOption<&mut u32>(places_equal) and iteratormax/minover 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.&mutto a local assigned more than once makes anrt.placeper call (e.g.seen.get_or_insert(..)whereseenis 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
Option's&mut selfmethods from core's MIR. core's bodies borrow&mutand useunwrap_unchecked, which feat(ts): lower Option's and Result's combinators from core's MIR #129'sstd_refusedrefuses, so this would mean opening std bodies to writes. The intercepts are a few lines each and need no closures for an alias.rt.Placecast to the payload's, asOption<T>isTin JS. This is cheaper where the receiver is already a Place, but it adds a second path, and the generalmake_placecovers it.Follow-ups
Options, tuples, and iterators of Places through the runtime's key functions (places_equal,iter_mut().max()).hashlink), which have no mapping. It should read "unsupported call".🤖 Generated with Claude Code
(written by Claude Opus 5.5)