bun:test: locate inline snapshots when the matcher call is a tail call

[robobun]

Fixes #39685

Problem

• test("x", () => expect(v).toMatchInlineSnapshot()) fails with toMatchInlineSnapshot called from file: "". A same-file helper return expect(v).toMatchInlineSnapshot() fails with Could not find 'toMatchInlineSnapshot' here. Both work in Jest.
• The matcher finds its location with a stack walk (Expect::inline_snapshot in src/runtime/test_runner/expect.rs). Both calls are proper tail calls, so that frame is gone when the matcher runs.

Fix

• The read of .toMatchInlineSnapshot happens before the call, while the frame is alive. The two inline matchers record their location when read and take it when called (InlineSnapshotSite, expect.rs). The stack walk remains the fallback, and nothing else pays.
• fn fields in .classes.ts get onGet (generate-classes.ts): the method moves under a private name, and the public name becomes a JSC CustomValue whose getter calls the hook and returns the method. JS cannot tell a CustomValue from a data property, so Object.defineProperty, assignment and expect.extend work as before.
• Bun__getPropertyReadSrcLoc (bindings.cpp) takes the name token's position from the expression info of the frame doing the read, checks that the source text there is the matcher name, and remaps it as the stack walk does. Otherwise it records nothing.
• Verified: test/js/bun/test/snapshot-tests/snapshots/snapshot.test.ts. Its 11 matcher call in tail position cases fail on the released binary. Also ran the other snapshot-tests/, expect*.test.*, the source lints and clippy.

Background

• Proper tail calls: in strict code JSC reuses the caller's frame for a call in tail position, so the callee cannot see its caller. V8 does not do this.
• Expression info: the source range and divot JSC stores per bytecode that can throw. For a.name the divot is the . and the range ends after the name.
• CustomValue: a property backed by a C getter that JS sees as a plain value. Its getter receives the prototype, not the receiver, so the site lives on the runner (Snapshots::inline_snapshot_site).
• Writeback: after the file ran, snapshot.rs re-reads it and expects the matcher name at each queued position.

Notes

Why this layer. Disabling tail calls was rejected in #26001. Capturing a location on every expect() call (#34978, closed in favour of this PR, and #39722) puts a stack walk on the hottest call in the runner, and still has to find the matcher from the expect() position: #34978 only consults its capture after the file check, so the shape in #39685 still fails there, and #39722 searches the file text for the matcher name, which the matcher name also appears in the argument case defeats. The read of the two inline matchers is the one event that is specific to this feature and still has the frame.

Why the site is taken at matcher entry. toThrowErrorMatchingInlineSnapshot runs the received function, and .resolves runs the event loop, before either reaches inline_snapshot. Matchers that run in between record and take sites of their own. Covered by inline snapshots inside the function the matcher runs.

Known limit. An inline snapshot matcher that runs inside the argument list of another one (expect(a).toMatchInlineSnapshot(helperThatSnapshots())) takes the slot first. The outer call then falls back to the stack walk, which is the behaviour on main.

Why CustomValue and not CustomAccessor. An earlier revision of this PR used a CustomAccessor. That reports an accessor descriptor, and Object.defineProperty(proto, "toMatchInlineSnapshot", { value }), which works on main, threw Attempting to change access mechanism for an unconfigurable property. A CustomValue has neither problem (JSC's PropertyDescriptor::setDescriptor masks the bit off on purpose), needs no generated setter (JSC replaces a setter-less CustomValue on assignment, and creates an own property when the receiver is an instance), and leaves the size of Expect alone.

Stack walk position. JSC positions a call at the end of its callee. The stack walk lands on the name token today only because the printer starts a source map mapping at every member name, so the remap moves the position back onto the name. The new function computes the name token directly, and both paths remap to the same place. The name on its own line and dot on the line before cases cover the layouts where the two differ before the remap.

Hash table. propRow emits the property as a GetterSetterType entry without the CustomAccessor bit. reifyStaticProperty (Lookup.h) turns that into putDirectCustomAccessor, which adds CustomValue itself (JSObject.cpp). The hook cannot throw or run JS, so the Rust thunk calls it directly, like memoryCost.

Source ref. getCallerSrcLoc passed a +1 ref into the remapper and then copied the result with toWTFString(), which leaked the ref on each call. The shared writeRemappedSrcLoc uses transferToWTFString(). The remapper derefs the old string when it substitutes one (VirtualMachine::remap_stack_frame_positions), so exactly one ref comes back either way.

JIT tiers. in helpers hot enough to be compiled by every JIT tier lowers the tier-up thresholds so the read goes through the baseline, DFG and FTL custom getter paths. All 600 reads have to resolve to the same position.

error snapshots test. The existing assertion hard-codes ANSI codes and only held under CI's FORCE_COLOR=1. It now branches on Bun.enableANSIColors, so a plain bun bd test of the file passes.

Also run against the debug build: every file in test/js/bun/test/snapshot-tests/, expect.test.js, expect-extend.test.js, expect-extend-preload.test.ts, expect-label.test.ts (535 pass), test/internal/source-lints/ (160 pass), cargo fmt --check, cargo clippy -p bun_runtime, prettier and clang-format on the touched files.

[coderabbitai]

Warning

Review limit reached

Your included review limit has been reached.

You’re in a promotional period — use the checkbox below to run this review for free:

• Run review for free

_{On-demand reviews are free for the next 31 days. After that, they cost $0.25 per reviewed file.}

How can I continue?

Run this review now using the option above, or comment @coderabbitai review --use-credits.

You can also wait for the limit to reset (next review available in 30 minutes), then comment @coderabbitai review or push new commits to the PR.

An organization admin can change what happens after included review limits in Billing.

How do review limits work?

CodeRabbit enforces per-developer PR review limits within each organization.

For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9f5c5a2d-3f02-4aff-9cbb-3bbdc5203706

📥 Commits

Reviewing files that changed from the base of the PR and between 6e906e4 and 414e0b4.

📒 Files selected for processing (10)

• src/codegen/class-definitions.ts
• src/codegen/generate-classes.ts
• src/js/builtins/BunBuiltinNames.h
• src/jsc/bindings/bindings.cpp
• src/runtime/test_runner/expect.rs
• src/runtime/test_runner/expect/toMatchInlineSnapshot.rs
• src/runtime/test_runner/expect/toThrowErrorMatchingInlineSnapshot.rs
• src/runtime/test_runner/jest.classes.ts
• src/runtime/test_runner/snapshot.rs
• test/js/bun/test/snapshot-tests/snapshots/snapshot.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Reproduced on the released binary (1.4.0-canary.1) with a two-line test file:

import { test, expect } from "bun:test";
test("inline", () => expect("v").toMatchInlineSnapshot());

CI=false bun test fails with toMatchInlineSnapshot called from file: ""; the block-bodied form writes "v". With this branch the expression-bodied form writes it too, as do the other tail-position shapes listed in the description, and the new cases in test/js/bun/test/snapshot-tests/snapshots/snapshot.test.ts fail on the released binary and pass with the fix.

Update: rebased onto main (the conflicts were in generate-classes.ts, where main had dropped the accessor/own field kinds) and carried the helper cases from #34978 over into the matcher call in tail position block; all ten cases there fail on the released binary and pass with this branch, and the whole file passes with the debug build. #34978 is closed in favour of this PR.

[github-actions]

This PR may be a duplicate of:

1. test: fix toMatchInlineSnapshot through a same-file tail-call helper #34978 - Fixes the same toMatchInlineSnapshot / toThrowErrorMatchingInlineSnapshot source-location failure caused by JSC proper tail calls eliding the caller frame, touching the same expect.rs / bindings.cpp / snapshot.test.ts, just via an expect()-time capture instead of an onGet property-read hook.

🤖 Generated with Claude Code

[robobun]

Not a duplicate of #34978, though it does cover the case that PR targets. #34978 captures a location on every expect() call and only consults it after the "called from the test file" check, so the shape this PR is about (test("x", () => expect(v).toMatchInlineSnapshot()), where no JS frame is left at all and the source URL comes back empty) still fails there; it also adds a stack walk plus expression info decode to every expect() call, which is the concern that review left open. Recording the location when the matcher property is read handles both shapes and costs nothing outside the two inline snapshot matchers, so if this lands, #34978 is superseded (its same-file helper case is covered by the "helper function in the test file" test here).

[claude]

I reviewed this PR and the automated bug hunt found no issues. Because it introduces a new codegen abstraction (onGet accessor-backed methods), relies on JSC-internal expression-info/divot semantics across tiers, and changes the property-descriptor shape of two matchers, a human look would still be worthwhile.

What was reviewed:

• writeRemappedSrcLoc refcount flow — transferToWTFString correctly consumes the ref returned by the remapper (no over-deref/UAF).
• Generated getter's globalObject->JSExpectPrototype() accessor exists via the codegen's prototypeInitializedOnMainThread lazy-init.
• InlineSnapshotSite::take() clears the cell on every matcher call so a stale location can't be reused; OwnedStringCell releases on Expect finalize.
• Bounds arithmetic in Bun__getPropertyReadSrcLoc (nameEnd - nameLength guarded by endOffset < nameLength; nameStart < info.divot and nameEnd > source.length() checked before indexing).

Extended reasoning...

Overview

This PR fixes inline snapshot location detection when the matcher call is a proper tail call (() => expect(v).toMatchInlineSnapshot()), where JSC has already dropped the caller's frame by the time the native matcher runs. It touches seven files across four layers: a new onGet option in the .classes.ts codegen (class-definitions.ts, generate-classes.ts) that installs a method as a CustomAccessor whose getter runs a native hook before returning the function; two new private builtin names; a new C++ binding Bun__getPropertyReadSrcLoc that decodes JSC's expression info at the property-read bytecode to locate the .name token, plus a refactor of getCallerSrcLoc to share the sourcemap-remap tail; a new InlineSnapshotSite cell on the Rust Expect struct with on_get_* hooks; and ~120 lines of new test coverage.

Security risks

None identified. The change is confined to bun:test's inline-snapshot machinery and reads source positions from JSC's own CodeBlock; it does not touch auth, network, filesystem writes outside the existing snapshot writeback, or parse untrusted external input.

Level of scrutiny

High. This is not a mechanical fix:

• It introduces a new cross-cutting codegen abstraction (onGet), which per this repo's review guidance warrants maintainer sign-off before landing inside a feature PR.
• It depends on undocumented JSC internals: that custom getters see the reader's frame via vm.topCallFrame in every tier, and that the expression-info divot for a dot access sits at the . with the range ending after the property name. The PR description argues this convincingly and cites ErrorStackFrame.cpp as precedent, but this coupling merits a maintainer's eye.
• It makes an observable behavior change: Object.getOwnPropertyDescriptor(Expect.prototype, "toMatchInlineSnapshot") becomes an accessor descriptor. The PR tests that identity, name, length, enumerability, expect.extend, and assignment still work, but a human should confirm this trade-off is acceptable.
• The description explicitly compares against two prior rejected/alternative approaches (#26001, #34978), which is a design decision a maintainer should ratify.

Other factors

The PR is exceptionally well-documented and well-tested: new tests cover expression-bodied callbacks, return, ternary, comma, test.each, multi-line .name, ?., same-file and cross-file helpers in tail position, and the method-shape contract. The author reports USE_SYSTEM_BUN=1 fails the new cases and the debug build passes the full snapshot suite plus expect.test.js, expect-extend.test.js, and stack.test.ts. The one candidate the finder raised (BunString over-deref in writeRemappedSrcLoc) was verified as a non-issue: transferToWTFString moves the ref rather than adding one. Given the scope of the codegen change and JSC-internal dependencies, deferring to a human is the right call despite the quality of the work.

[claude]

I reviewed this PR and didn't find any bugs. Because it introduces a new onGet codegen mechanism and relies on JSC expression-info/CustomAccessor internals across every JIT tier, a human look at the design would still be worthwhile; there are also outstanding comment-cop lints on several of the new multi-line comments.

What was reviewed:

• The onGet codegen path: private-name method + public CustomAccessor, setter putDirect override, and the expect.extend/assignment override tests.
• Bun__getPropertyReadSrcLoc: divot/endOffset arithmetic against source bounds, line/column advance over CR/LF/LS/PS, and the fallback to the existing stack walk when the name check fails.
• String ownership: toStringRef→transferToWTFString in the shared remap tail, and OwnedStringCell/take() clearing the recorded site so it can't leak into a later matcher call.

Extended reasoning...

Overview

The PR fixes toMatchInlineSnapshot/toThrowErrorMatchingInlineSnapshot failing when the matcher call is a proper tail call, by recording the source location at property-read time via a new onGet hook. Touches: the .classes.ts codegen (class-definitions.ts, generate-classes.ts) to add onGet; BunBuiltinNames.h for the two private names; bindings.cpp for the new Bun__getPropertyReadSrcLoc and a refactor of getCallerSrcLoc into shared helpers; expect.rs to store and consume the recorded site; jest.classes.ts to wire the two matchers; and ~150 lines of new test coverage in snapshot.test.ts.

Security risks

None identified. The change is scoped to the test runner's inline-snapshot machinery and reads only bytecode expression info and source text already held by the CodeBlock. No user-controlled input reaches the new code path outside a bun test process the user is already running on their own files.

Level of scrutiny

High. This is not a mechanical fix: it adds a new cross-cutting codegen abstraction (onGet turns a prototype method into a CustomAccessor-backed pair), changes the observable property descriptor of two matchers, and depends on JSC internals — that vm.topCallFrame and the stored call-site index are correct from a CustomAccessor in every tier (interpreter, baseline, DFG, FTL, inline caches), and that the expression-info divot for a dot access points at the . with endOffset reaching the end of the name. The PR argues each of these carefully and covers the JIT-tier concern with a dedicated test, but REVIEW.md flags "new cross-cutting abstractions need maintainer agreement before appearing inside a feature PR", and this is exactly that shape.

Other factors

• The bug-hunting pass found nothing; the offset arithmetic in Bun__getPropertyReadSrcLoc is bounds-checked against source.length() and falls back cleanly, and the recorded location is take()n on every matcher call so it cannot bleed across calls.
• The refactored remap tail also fixes a small ref leak (toWTFString → transferToWTFString), which is a correct incidental fix.
• Test coverage is thorough (creation + -u update, both matchers, .resolves, return, ternary/comma/test.each, multi-line dot placement, ?., same-file and cross-file helpers, method-shape contract, expect.extend/assignment overrides, and a JIT-tier warm-up case).
• There are ~12 outstanding comment-cop bot inline comments on the new multi-line doc comments; whether those need trimming is a maintainer call.
• There is a competing open PR (#34978) with a different approach; a maintainer should decide which lands.

[robobun]

^{Updated 3:18 AM PT - Aug 20th, 2026}

✅ @robobun, your commit 414e0b48f0539434addf07a451230a7aee06cca2 passed in Build #101579! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 39111

That installs a local version of the PR into your bun-39111 executable, so you can run:

bun-39111 --bun

[claude]

I reviewed this PR and didn't find any bugs. Because it introduces a new onGet codegen mechanism, changes how two matcher properties are installed on the prototype (data property → CustomAccessor), and adds non-trivial JSC expression-info decoding in bindings.cpp, a human look would still be worthwhile — particularly on the design choice vs. #34978 and the observable descriptor change.

What was reviewed:

• Bun__getPropertyReadSrcLoc: divot/endOffset arithmetic against source.length(), line/column advance over CR/LF/LS/PS, and the shared writeRemappedSrcLoc refactor (the transferToWTFString fix for the pre-existing ref leak).
• InlineSnapshotSite: the take()-on-every-call pattern so a stale site can't be applied to a later matcher, and OwnedStringCell ownership through Expect's finalize.
• Generated getter/setter: dynamicDowncast guard, private-name storage, and that expect.extend/assignment still override via putDirect — covered by the new "matchers can still be replaced" and "still look like methods" tests.
• Test coverage spans creation, -u update, JIT tiers, cross-file rejection, and the same-line conflict error.

Extended reasoning...

Overview

The PR fixes inline snapshot matchers failing when the matcher call is in tail position (JSC proper tail calls elide the caller's frame). It records the source location when the matcher property is read — while the frame still exists — via a new onGet hook in the .classes.ts codegen. Seven files touched: the codegen field definition and generator (~100 lines), two new builtin names, ~90 new lines in bindings.cpp (a new Bun__getPropertyReadSrcLoc plus a refactor of getCallerSrcLoc into shared helpers), ~110 lines in expect.rs for the InlineSnapshotSite slot, two onGet entries in jest.classes.ts, and ~230 lines of tests.

Security risks

None identified. The new C++ reads the frame's own source provider and expression info; all offsets are bounds-checked against source.length() before indexing, and the property name comparison is byte-for-byte against a static ASCII literal passed from Rust. No user-controlled data reaches an allocation size or a filesystem path here.

Level of scrutiny

High. This is not a mechanical change: it adds a new codegen primitive (onGet) that changes property installation from a hash-table data property to a CustomAccessor + private-name method pair, which is an observable prototype-shape change and a pattern future .classes.ts authors may reach for. The bindings.cpp additions depend on JSC internals (divot semantics for dot reads, vm.topCallFrame from custom getters across all JIT tiers, expressionInfoForBytecodeIndex) that are correct as described but subtle enough that a maintainer familiar with the JSC glue should confirm the approach. The PR also supersedes #34978 with a different design, which is exactly the kind of decision a human should sign off on.

Other factors

The test coverage is unusually thorough — expression-bodied callbacks, return, ternary/comma tail positions, .resolves, ?., name/dot on separate lines, same-file helpers (direct, nested, mixed, with decoy names in arguments), cross-file rejection, JIT tier-up via lowered thresholds, and the method-shape contract (name/length/identity/enumerability, expect.extend and assignment overrides). The refactor also fixes a pre-existing toStringRef→toWTFString ref leak in the remap tail. The remaining unresolved inline comments are from the automated comment-cop bot re-firing on already-tightened doc comments; robobun already explained each is API-contract documentation, not a workaround justification, and a "Tighten the comments" commit was pushed. Given the scope (new codegen surface, JSC-internal reasoning, design decision superseding another PR), deferring to human review is the right call despite no bugs found.

[robobun]

The two points flagged for a human look are covered in the PR body: the design comparison with #34978 (and with disabling tail calls, rejected in #26001) is the "Why this is the right layer" bullet under Fix, and the descriptor change is called out there as the only observable difference. The method-shape contract tests in snapshot.test.ts pin typeof, name, length, identity, prototype reads, enumerability, and the expect.extend and assignment overrides, so a regression in that surface would fail the suite.

[claude]

I reviewed this PR and the automated bug-hunting pass found no issues. Because it introduces a new codegen mechanism (onGet / CustomValue), reads JSC expression-info internals from a custom getter, and changes how the two inline-snapshot matchers are installed on the Expect prototype, a human look at the design would still be worthwhile.

What was reviewed:

• The Bun__getPropertyReadSrcLoc divot/endOffset arithmetic and line/column walk against JSC's dot-read expression-info shape, including the CRLF / U+2028 handling and the bounds check on source.length().
• Ref ownership through writeRemappedSrcLoc — the switch to transferToWTFString fixes the pre-existing +1 leak from toStringRef + toWTFString.
• Site lifetime: recorded on the global Snapshots (not per-Expect, since the CustomValue getter receives the prototype), and take_recorded() is called before .resolves/.rejects spin the loop and before toThrowErrorMatchingInlineSnapshot runs the received function, so a nested matcher can't steal the outer site.
• The generated CustomValue path: hash-table entry without the CustomAccessor bit so getOwnPropertyDescriptor still reports a data descriptor (pinned by the new "still look like methods" test), and the private-name putDirect for the actual JSFunction.

Extended reasoning...

Overview

The PR fixes toMatchInlineSnapshot / toThrowErrorMatchingInlineSnapshot when the matcher call is in tail position (JSC proper tail calls elide the caller frame, so the existing stack-walk finds no source location). The fix records the call site at property-read time via a new onGet codegen hook that installs the matcher as a JSC CustomValue on the prototype. It touches: the class-definitions codegen contract and generator, BunBuiltinNames.h, C++ bindings (new Bun__getPropertyReadSrcLoc, refactored getCallerSrcLoc), Expect / Snapshots in Rust, the two matcher entry points, jest.classes.ts, and ~230 new test lines.

Security risks

None identified. This is test-runner-only surface; the new C++ path reads source text from the CodeBlock's own SourceProvider using JSC-supplied offsets and bounds-checks nameEnd against source.length() before indexing. No user-controlled input reaches an allocation size or filesystem path.

Level of scrutiny

High. This is not a mechanical fix: it adds a new field kind to the .classes.ts codegen, relies on JSC CustomValue semantics (getter receives the prototype, reifies as a data descriptor, vm.topCallFrame is set before the getter runs from every tier), decodes expressionInfoForBytecodeIndex output by hand, and changes the JS-observable shape of two prototype properties. The PR description also appears slightly stale relative to the implementation (it describes a CustomAccessor with a setter and per-Expect storage; the code uses a CustomValue with no setter and stores the site on the global Snapshots), which is one more reason a maintainer should confirm the current shape is the intended one.

Other factors

Test coverage is thorough (tail-position variants, JIT-tier warmup, method-shape contract, override paths, cross-file rejection, nested matchers) and the gate evidence shows 10 fail-without / pass-with on both ASAN debug and release. The three unresolved comment-cop inline notes are automated comment-length flags on doc comments that earlier rounds already answered; they are not substantive review feedback. Given the codegen and JSC-internals scope, deferring rather than approving.

[robobun]

The staleness note is about an older body revision. The description was rewritten with the rebase and now matches the code: the matchers are CustomValue entries (data descriptor, no generated setter), the site lives on Snapshots::inline_snapshot_site, and each matcher takes it at entry, before .resolves spins the loop or the received function runs. The "Why CustomValue and not CustomAccessor" note in the body covers the design change, including the Object.defineProperty regression the CustomAccessor form had. CI is green on the rebased head (build 101579).