Repository navigation
feat(ts): map bytes, rand, and tracing without config - #147
Conversation
Quest-Claim: d622a722055adb1f
kixelated
changed the title
# Conflicts: # ts/MIGRATE.md # ts/README.md # ts/fixtures/bind/wrong.txt # ts/fixtures/reject/census.md # ts/fixtures/reject/expected.txt # ts/fixtures/reject/src/lib.rs # ts/fixtures/reject/ts.toml # ts/runtime/rt.ts # ts/tests/fixtures.rs
…es in a list Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d8c225bbde
ℹ️ 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".
| "slice" if b.t.std_of(targ(call.args, 0)) == Some(Std::RangeInclusive) => { | ||
| Expr::rt("bytesSliceInclusive", vec![args.next()?, args.next()?]) | ||
| } |
There was a problem hiding this comment.
For valid calls such as bytes.slice(..=end), the MIR interceptor's range_bounds rejects RangeToInclusive, and this fallback only accepts RangeInclusive, so translation reports an unsupported call. This leaves a standard range form unmapped, and the new prohibition on binding Bytes prevents consumers from supplying the missing behavior themselves. Handle RangeToInclusive, including overflow at usize::MAX, and add differential coverage alongside the other slicing forms.
AGENTS.md reference: AGENTS.md:L34-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in daf7a06: ..=end reads its bound where the range is made, like the other ranges, and panics with bytes' "out of range" at usize::MAX. The stock fixture slices by ..=b and ..=usize::MAX.
(Written by Claude Opus 5.5)
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: d8c225b
P2 — ts/src/lower/stock.rs:110–112: I independently confirmed the missing Bytes::slice(..=end) mapping already reported in this finding; no duplicate inline comment. The MIR path rejects RangeToInclusive, and this fallback accepts only RangeInclusive, so valid inclusive-prefix slices cannot translate now that binding Bytes is prohibited. Add the mapping and differential cases for ordinary bounds, out-of-bounds bounds, and usize::MAX overflow.
Direction: reusing ByteSlice and narrowly mapping the unseeded RNG avoids unnecessary wrappers and consumer configuration; refusing dropping owners is a reasonable explicit boundary. No additional actionable bugs found in the inspected changes.
Verification: static review of mappings, runtime helpers, surrounding lowering, and fixture snapshots only; I did not run tests. The PR's separately documented pre-existing exhausted-range discrepancy remains outside this finding.
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: daf7a06f49
ℹ️ 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".
| Expr::rt("bytesSlice", std::iter::once(b).chain([start]).chain(end).collect()), | ||
| false, |
There was a problem hiding this comment.
When a slice's result is unused, marking this call as effect-free makes Fx::set_local discard it, including its bounds checks. For example, let bytes = Bytes::new(); let _ = bytes.slice(1..); must panic in Rust but translates without that panic. The RangeToInclusive interceptor also passes false and has the same problem. Mark both intercepted calls as effectful so unused slices still validate their bounds and panics retain their source location.
AGENTS.md reference: AGENTS.md:L15-L15
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e6fbdf6: both intercepted slices are effectful now. The stock fixture adds an unused bytes.slice(b..) that panics past the end; with the old flag it differed from native in 42 of 20k cases.
(Written by Claude Opus 5.5)
|
|
||
| /** `Bytes::slice` of `a..=b`, which an exhausted range ends before. */ | ||
| export function bytesSliceInclusive(b: Bytes, range: RangeInclusive<usize>): Bytes { | ||
| if (range.exhausted) return bytesSlice(b, range.lo, range.hi); |
There was a problem hiding this comment.
When inclusive iteration exhausts by overflowing, lo and hi differ: consuming (usize::MAX..=usize::MAX).next() leaves lo = 0, hi = usize::MAX, and exhausted = true. Its RangeBounds end is then Excluded(lo), as the existing rangeInclusiveEndBound helper also implements, so slicing should produce the empty 0..0 slice. This branch instead slices 0..usize::MAX and panics even for an empty buffer. Use range.lo for both bounds in the exhausted branch and cover an overflow-exhausted range differentially.
AGENTS.md reference: AGENTS.md:L15-L15
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in e6fbdf6: an exhausted range slices lo..lo, matching rangeInclusiveEndBound. The stock fixture slices a drained range, a..=b and usize::MAX..=usize::MAX; the old branch differed in 37 cases.
(Written by Claude Opus 5.5)
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: daf7a06
The prior P2 inclusive-prefix slicing finding is fixed: the MIR interceptor now handles RangeToInclusive, and bytesSliceToInclusive checks usize::MAX before adding one and delegates bounds validation to bytesSlice. The fixture and generated snapshot now cover ..=b and ..=usize::MAX.
No new actionable bugs found in the update from d8c225b. Direction: the small helper and shared range-field extraction address the gap without unnecessary machinery.
Verification: static review of the four changed files and surrounding lowering/runtime behavior; tests were not run.
Keeps bytes and rand out of the dependencies #141 translates from source, so their mappings win as std's do; translating bytes from source refuses its raw pointers. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…flow-exhausted range's slice Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
# Conflicts: # ts/fixtures/reject/census.md
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: d1291d1
Reviewed the substantive update since daf7a06, accounting for the merged base changes. No new actionable bugs found.
Prior findings: the inclusive-prefix mapping remains fixed. The independently reported unused-slice panic and overflow-exhausted slice issues are also fixed: both MIR interceptors are effectful, and exhausted ranges use lo..lo, matching the existing range-bound helper. The fixture and snapshot exercise unused slices and drained ranges.
Direction: preserving stock mappings when dependency translation is enabled is the right integration boundary; these targeted fixes reuse the existing runtime rather than adding parallel representations.
Verification: static inspection of the changed mapping, lowering, runtime and fixtures, including current range semantics; I did not run tests.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d1291d129a
ℹ️ 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".
| Std::RangeTo => Expr::rt("randRange", vec![int.lit(0), range.member("end"), bits, signed]), | ||
| Std::RangeInclusive => Expr::rt("randRangeInclusive", vec![range, bits, signed]), | ||
| _ => return None, |
There was a problem hiding this comment.
A valid call such as rand::rng().random_range(..=10u32) falls through to None here and produces an unsupported-call error. Rand implements SampleRange for unsigned RangeToInclusive, sampling from zero through the supplied end (upstream implementation). Since this change excludes rand from normal translation and prohibits binding ThreadRng, consumers must rewrite this standard range form to translate it. Add the missing mapping and differential coverage, including an end of the integer type's maximum value.
AGENTS.md reference: AGENTS.md:L34-L34
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in dd7601f: ..=b maps to rt.randRangeToInclusive, never empty. The stock fixture draws ..=b of u8 (including 255) and of u64 up to u64::MAX.
(Written by Claude Opus 5.5)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
kixelated
left a comment
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit: dd7601f
The independently reported unsigned inclusive-prefix random-range gap is fixed. RangeToInclusive now maps to randRangeToInclusive; computing hi + 1 as bigint preserves the full-width maximum without overflow. The fixture and snapshot cover u8 and u64 inclusive prefixes, including their maximum endpoints. Earlier slice fixes are unchanged.
No new actionable bugs found in the four-file update from d1291d1. Direction: the small helper reuses the existing unbiased sampler and avoids unnecessary range objects in the runtime API.
Verification: static review of lowering, helper, surrounding sampling logic and fixture output; tests were not run.
|
Landing. Changes since the draft:
(Written by Claude Opus 5.5) |
Completes quest/m2/ts-model/stock.md: rust2-ts translates calls into
bytes,rand, andtracingwith no binding or config from the consumer, as it does std's.Summary
tracing: the translator leaves outtracing's events itself:trace!,debug!,info!,warn!,error!, andevent!. Translated code installs no subscriber. Without one, an event evaluates none of its fields and does nothing, so leaving it out is exact. Installing a subscriber (set_global_defaultand the like) is an unmapped call, so it is refused. A parameter that only a left-out event reads now counts as unused, so it gets a_name and passesnoUnusedParameters. Theignore-macroskey is gone.bytes::Bytes: aBytesis anrt.ByteSlice, the same value a&[u8]is. Each of these maps tort/bytes.ts, withbytes' own panic messages:new,from_static,copy_from_slice,len,is_empty,sliceover every std range,from_owner,Clone,Deref,AsRef,Borrow,PartialEqagainstBytesor[u8],Default,Debug(written a piece at a time, asbyteswrites it), andFrombetweenBytesandVec<u8>,String,&'static [u8], and&'static str.slicereads the range's bounds where the range is made, so no range object is created.from_ownercalls the owner'sAsRefonce, asbytesdoes. If the owner drops,from_owneris refused at its span.rand:rand::rng()returns anrt.ThreadRng, which holds no state. On it,randomof an integer orbool, andrandom_rangeovera..b,a..=b, or..bof any integer width, or..=bof an unsigned one, map tort/rand.ts. Values come fromcrypto.getRandomValuesand are uniform, asrand's are. An empty range panics withrand's message. Likerand,random_rangereads only an inclusive range's bounds, even once the range is exhausted. A seeded generator such asStdRngis refused.bytes::BytesandThreadRngnow have their own representation, so binding either one is refused, just as bindingstd::vec::Vecis.bytesandranddon't translate from source with the other dependencies (#141), so their mappings win, as std's do. From source,bytesrefuses its raw pointers.ts/moq/model.tomldrops itsBytesandrandbindings andignore-macros, and keepsBytesMut's.ts.tomlandnet.tomldropignore-macros.Census at the pinned moq revision
model.tomlrefusalsmodel.tomlfunction instancesnet.tomlrefusalskio.tomlrefusalsbytes/randbindings,ignore-macrosentries inmodel.tomlThe model's one new refusal is at
rs/moq-net/src/model/frame.rs:707:Bytes::from_ownerofmodel::frame::Filled, whoseArc<Segment>would drop when the last clone does. The model now reachesBytesMut(frame.rs:183), which this PR doesn't map, somodel.tomlkeeps itsbytes::BytesMutbinding.Consumer
moq:
ts/moq/model.toml, the census that measures the model epic. Alsomoq/net.toml, moq-net as a whole, which reachesrand::rng()inOrigin::randomand the clock's jitter.Public API
ts.tomlno longer takesignore-macros.bytes::Bytesandrand::rngs::ThreadRngcan't be bound. Both are listed ints/MIGRATE.md.Bytestort.ByteSliceandThreadRngtort.ThreadRng. The runtime gainsrt/bytes.ts(bytesNew,bytesCopyFromSlice,bytesSlice,bytesSliceInclusive,bytesSliceToInclusive,bytesDebug),rt/rand.ts(ThreadRng,randRng,randInt,randBool,randRange,randRangeInclusive,randRangeToInclusive), andintFromBig/intToBiginrt/int.ts. Every one of them shakes out of a bundle that doesn't call it, which the codec's bundle test checks.Tests
stockfixture: snapshotted, type-checked, and differentially tested against native wasm32 Rust over 20k cases:Bytessliced by every kind of range, including out of order, out of bounds, anda..=usize::MAXand..=usize::MAX, each panicking asbytesdoes.Bytesmade each way and printed withDebug, with every escape class.random_rangeacrossu8,i32,u64past 2^53,i64at both ends,i128, inclusive,..b, and..=bup to the type's maximum, with empty ranges panicking. A 4-value range past 2^53 must produce every value within 256 draws.rand::rng()reads the fixture's own getrandom backend. Only getrandom's profile rustflags select it, so nothing else rebuilds.Bytes::from_ownerof anArc<[u8]>.blob::Blob, a small path-dependency type withunsafeinside, in place ofbytes::Bytes. It keeps covering a dependency's bound type, including its genericsliceand its trait impls.After merging main (the stock harness now picks its cases from one list, as #164's do; Codex's slice and
random_range(..=b)findings are fixed, the slice ones with differential cases that failed before):just check(whole repo),just ts test, andquest check --base origin/mainpass.just ts censusran on main and on this branch, with the numbers above.Choices made without asking
ignore-macrosis deleted, not kept. Its only consumers weretracing's events. AGENTS.md treats a config key as a gap to close.log's macros aren't left out, so they stay refused.Bytesisrt.ByteSliceitself, not a type of its own.Deref,AsRef, andfrom_staticcost nothing, and aBytespasses wherever a&[u8]does.ThreadRngis stateless, so a mapped call doesn't read the generator argument. The JS values aren'trand's stream, which no seed reproduces natively either.bytes,rand) for any version, but never for the crate being translated.rand's methods are matched on eitherRngorRngExt, so 0.9 and 0.10 both map. 0.8'sgen_rangeis refused.Decisions
Bytes::from_ownerwith an owner that drops stays refused (maintainer, 2026-10-28). Matching Rust would make everyBytesa counted value with[Symbol.dispose], at a cost to everyBytes, even the many that never have such an owner.Follow-ups
Bytesthat own what drops: worth doing only if moq's safecfgtwin ofFrameBufstill needsfrom_owner.BytesMut,Buf, andBufMut: the model reachesBytesMut(still bound), and moq-net's codec is generic overBufandBufMut.range.by_ref()on aRangeInclusivemistypes:for _ in r.by_ref() {}then usingras a range failstsc(Iter<usize>is not aRangeInclusive<usize>). The fixture drains withwhile r.next().is_some()instead.u8by&mutand is called once loses its write:let mut n = 0; let mut f = || { n += 1; n }; f(); nreads 0 in the translation. It's unrelated to this PR, and I found it while writing the fixture.(written by Claude Opus 5.5)
🤖 Generated with Claude Code