Repository navigation
fix(ts): std's formatted panic messages hold std's capacity - #641
Conversation
Quest-Claim: 2f88f610f571839b
The runtime passed std's formatted panic messages to a hook as one string, so the `String` payload grew in one push where std's grows with each piece and number it writes. Each such site now formats its pieces as std's `panic!` does, and the hooked bounds check emits them too. Intercepted std calls are located at their call, as rustc locates `#[track_caller]` ones, rather than at the statement. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars.
|
kixelated
left a comment
There was a problem hiding this comment.
Automated review by review (OpenAI)
Reviewed commit 317de7c against base efa6300.
No actionable findings identified in this static review. Routing formatted panic pieces through the existing formatter is the right direction for preserving observable String capacity instead of patching capacity afterward. I checked the runtime-site conversions, signed/wide integer and padded-character formatting paths, assertFailed's literal boundaries, fmtOwned/write plumbing, hooked bounds-check emission, and the intercepted-call location change. The expanded hook fixture now records capacity for every String payload and adds coverage for the newly exercised sites.
Verification limits: GitHub source/diff inspection only. I did not run native differential cases, TypeScript checks, benchmarks, or just ci; the reported zero differences, performance measurements, and checkout-dependent snapshot failures have not been independently reproduced here. This is a COMMENT, not an approval.
(Written by review (OpenAI))
|
quest land watch: the merge queue removed this pull request at 317de7c because it conflicts with The land fixer works on it in the background, and the watcher re-enqueues it at the back of the queue once it is fixed or judged flaky, or hands it back here with what it needs. (written by quest land watch) |
|
quest land watch: armed auto-merge on this pull request at 8318278, so it joins the back of the merge queue once its required checks pass, with the land fixer's fix ( (written by quest land watch) |

Problem
A panic hook reading a
Stringpayload's capacity got a different number than native Rust for std's own formatted panics. std writes a panic message into itsStringone piece at a time: each literal piece and each number is its ownpush_str, and each push grows the capacity byRawVec's amortized rule. The runtime passed the whole message as one string, so"slice index starts at 3 but ends at 1"held 37 where std holds 44. moq-dev/rust2#574 gaveStringits capacity and found this; the quest is quest/m3/ts-panic-payload-capacity.md.Approach
panicFmtstd's own pieces, so the hook'sStringgrows write by write as std's does. The pieces come from the pinned nightly's std source andbytes1.12.1. That covers:strslicing,VecandByteVec(insert,remove,swap_remove,split_off),copy_from_slice,copy_within,write_clone_of_slice, integer and floatclamp,encode_utf8/encode_utf16({code:04X}pads one'0'at a time), the wasm32MutexandExactSizeIterator::lenassert_eq!s, andVecDequeranges;bytes: every message with a number in it;Result::expectbehindformat!andto_string, which std writes asmsg,": ",Error.rs.panicSizes(pieces, at)is new: literal pieces with theusizes between them, which most sites need. The rest buildArgumentsdirectly. Values go throughfmtOwnedrather than closures, so a hot function such asidxcaptures nothing and its non-panicking path is unchanged.assertFailedwrites" rightfailed: "` as one piece, as std's format string does. Before, it was two writes.rs.panicSizesfor a hooked inline bounds check (ts/src/lower/mir/mod.rs). Without a hook it keeps the string concatenation, since nothing can read the payload.ts/src/lower/mir/call.rs): a callcall.rslowers itself, such ascopy_within, was located where its MIR statement starts, at the receiver (vinv.copy_within(..)). It is now located at the call'sstd_site, fromfn_span, the same span rustc uses for#[track_caller]andBody::library_callalready used. The newcopy_withincase exposed this.hooked::recordnow writes everyStringpayload's capacity, so every hooked case checks it against native, replacingsized(), which checked onlypanic!andpanic_any.hooked::sitedgains cases for the sites no case reached:Vec<u8>andVec<u16>insert/remove/swap_remove/split_off,copy_within,u64and negativei32clamp, andformat!/to_stringof a failingDisplay.Measured
157 differencesinhooked::*, every one a capacity, e.g.index out of bounds: the len is 2 but the index is 2at 52 against native's 64. This branch: 0.process.cpuUsage; the host is shared and loaded, so treat this as a coarse check): 2e7 rounds ofidx,rangeCheck, andclamp, plusvecInsert/vecRemoveandfmtFormat, over five alternating runs, gave base 477-572 ms and this branch 473-563 ms. No difference shows above the noise.Impact
rs.panicSizes.rs.panicSizesinstead ofrs.panic, and some located panics move from the receiver to the method name, matching native.Caused by
Stringpayload a capacity, but wrote std's formatted messages into it in one push.Choices made without asking
rs.panicSizestakes the message as an array, thenat, the same shape aspanicFmt, rather than as rest parameters.copy_withinlocation in this PR instead of leaving it as a follow-up, since the PR's new case exposed it. The fix applies only where a std site is known, so the existing statement location stays as the fallback.Alternatives
(f) => usizeDisplay(n, f)): shorter call sites, but V8 then context-allocates the captured locals for the whole hot function.fmt::write's growth and breaks for padded or signed values.Follow-ups
None.
Validation
just ciat317de7ce6: everything passes exceptlang::dyns(ids.ts) andlang::panics(hook/expected/lib.ts). Each differs from its snapshot only in TypeId hashes that depend on the checkout's path. The same failure reproduces on bare origin/main and is fixed by moq-dev/rust2#633. This PR leaves both snapshots as main has them.cargo test --test fixtures panicswith the new capacity checks: 0 differences from native at this branch, and 157 against main's runtime and translator.Models
Size: M
Implementer: Claude Code, claude-opus-5-5, high effort
Review
mechanical fix by codex, gpt-6.1-sol, medium effort:
Merged base and re·solved generated hooked.ts conflict, preserving panic payload capacity output and base TypeId masking; ran just check base and just test base.(head 8318278)Rewritten by
quest reviewafter each round.01a1b61b-136d-7343-903a-f400facd5971, round 1No actionable defects found in the specified diff; runtime TypeScript checking and 755 focused native differential cases passed, while the broader suite was blocked by the read-only sandbox.🤖 Generated with Claude Code
(Written by Claude Opus 5.5 with high effort)