Skip to content

fix(ts): std's formatted panic messages hold std's capacity - #641

Merged
moq-bot[bot] merged 4 commits into
mainfrom
quest/m3/ts-panic-payload-capacity
Nov 7, 2026
Merged

moq-bot[bot] merged 4 commits into
mainfrom
quest/m3/ts-panic-payload-capacity

Conversation

@kixelated

@kixelated kixelated commented Nov 7, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

A panic hook reading a String payload's capacity got a different number than native Rust for std's own formatted panics. std writes a panic message into its String one piece at a time: each literal piece and each number is its own push_str, and each push grows the capacity by RawVec'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 gave String its capacity and found this; the quest is quest/m3/ts-panic-payload-capacity.md.

Approach

  • Every runtime site that formats a message now hands panicFmt std's own pieces, so the hook's String grows write by write as std's does. The pieces come from the pinned nightly's std source and bytes 1.12.1. That covers:
    • std: slices, str slicing, Vec and ByteVec (insert, remove, swap_remove, split_off), copy_from_slice, copy_within, write_clone_of_slice, integer and float clamp, encode_utf8/encode_utf16 ({code:04X} pads one '0' at a time), the wasm32 Mutex and ExactSizeIterator::len assert_eq!s, and VecDeque ranges;
    • bytes: every message with a number in it;
    • the Result::expect behind format! and to_string, which std writes as msg, ": ", Error.
  • rs.panicSizes(pieces, at) is new: literal pieces with the usizes between them, which most sites need. The rest build Arguments directly. Values go through fmtOwned rather than closures, so a hot function such as idx captures nothing and its non-panicking path is unchanged.
  • assertFailed writes " right failed: "` as one piece, as std's format string does. Before, it was two writes.
  • The translator emits rs.panicSizes for 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.
  • Location of intercepted std calls (ts/src/lower/mir/call.rs): a call call.rs lowers itself, such as copy_within, was located where its MIR statement starts, at the receiver (v in v.copy_within(..)). It is now located at the call's std_site, from fn_span, the same span rustc uses for #[track_caller] and Body::library_call already used. The new copy_within case exposed this.
  • The fixture: hooked::record now writes every String payload's capacity, so every hooked case checks it against native, replacing sized(), which checked only panic! and panic_any. hooked::sited gains cases for the sites no case reached: Vec<u8> and Vec<u16> insert/remove/swap_remove/split_off, copy_within, u64 and negative i32 clamp, and format!/to_string of a failing Display.

Measured

  • Reproduced first: the new fixture against main's runtime and translator gives 157 differences in hooked::*, every one a capacity, e.g. index out of bounds: the len is 2 but the index is 2 at 52 against native's 64. This branch: 0.
  • Hot paths, Node CPU time (process.cpuUsage; the host is shared and loaded, so treat this as a coarse check): 2e7 rounds of idx, rangeCheck, and clamp, plus vecInsert/vecRemove and fmtFormat, over five alternating runs, gave base 477-572 ms and this branch 473-563 ms. No difference shows above the noise.

Impact

  • New runtime export: rs.panicSizes.
  • A hooked program's generated bounds checks call rs.panicSizes instead of rs.panic, and some located panics move from the receiver to the method name, matching native.
  • No other public API or wire change.

Caused by

  • moq-dev/rust2#574 - gave the hook's String payload a capacity, but wrote std's formatted messages into it in one push.

Choices made without asking

  • rs.panicSizes takes the message as an array, then at, the same shape as panicFmt, rather than as rest parameters.
  • An unhooked bounds check keeps its string concatenation, which leaves every unhooked fixture's snapshot and bundle unchanged.
  • Fixed the copy_within location 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

  • Build a closure per value ((f) => usizeDisplay(n, f)): shorter call sites, but V8 then context-allocates the captured locals for the whole hot function.
  • Correct the capacity after the fact by computing std's growth from piece lengths: that duplicates fmt::write's growth and breaks for padded or signed values.

Follow-ups

None.

Validation

  • just ci at 317de7ce6: everything passes except lang::dyns (ids.ts) and lang::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 panics with 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 review after each round.

  • Reviewer: Codex, gpt-6.1-sol, high effort
  • Session: 01a1b61b-136d-7343-903a-f400facd5971, round 1
  • Head: 317de7c
  • Base: origin/main at efa6300
  • Score: 10/10, No 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.
  • Findings: none

🤖 Generated with Claude Code

(Written by Claude Opus 5.5 with high effort)

@pullfrog

pullfrog Bot commented Nov 7, 2026

Copy link
Copy Markdown

Pullfrog  | Review this ➔

kixelated and others added 2 commits November 7, 2026 00:10
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>
@kixelated kixelated changed the title std's panic messages hold std's capacity fix(ts): std's formatted panic messages hold std's capacity Nov 7, 2026
@kixelated
kixelated marked this pull request as ready for review November 7, 2026 13:09
@coderabbitai

coderabbitai Bot commented Nov 7, 2026 •

Copy link
Copy Markdown

Important

  • Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

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: 85c1334f-da09-4531-b290-9473d7251367
  • 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

@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 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))

@kixelated
kixelated added this pull request to the merge queue Nov 7, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Nov 7, 2026
@moq-bot

moq-bot Bot commented Nov 7, 2026

Copy link
Copy Markdown

quest land watch: the merge queue removed this pull request at 317de7c because it conflicts with main (merge_conflict).

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)

@moq-bot
moq-bot Bot enabled auto-merge November 7, 2026 14:31
@moq-bot

moq-bot Bot commented Nov 7, 2026

Copy link
Copy Markdown

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 (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..

(written by quest land watch)

@moq-bot
moq-bot Bot added this pull request to the merge queue Nov 7, 2026
Merged via the queue into main with commit 48a0fae Nov 7, 2026
7 checks passed
@moq-bot
moq-bot Bot deleted the quest/m3/ts-panic-payload-capacity branch November 7, 2026 15:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant