Skip to content

Propagate an exception thrown while global.throw formats its message - #48119

Open
robobun wants to merge 2 commits into
mainfrom
robobun/04b81e3a/matcher-message-when-printing-throws
Open

robobun wants to merge 2 commits into
mainfrom
robobun/04b81e3a/matcher-message-when-printing-throws

Conversation

@robobun

@robobun robobun commented Nov 7, 2026 •

Copy link
Copy Markdown
Collaborator

Merge order: after #48114, or Bun.$ throws TypeError: No default value for an object with no prototype.

Decision asked: this changes JSGlobalObject::throw for all of Bun. Only 20 test-runner call sites reach the new branch. Fallback: a throw helper inside the test runner.

Problem

  • In bun test, a value whose [Bun.inspect.custom] method throws gives two outcomes. expect(v).toBeNull() throws that error (bun test: Vitest compatibility #44833). expect(v).toHaveLength(1) throws Received value does not have a length property: and drops it.
  • 16 sites do the second. They print the value inside global.throw, and JSGlobalObject::error_message (src/jsc/JSGlobalObject.rs:744) clears the pending exception. No doc states the rule.

Fix

  • throw and throw_type_error return the pending exception when formatting their message threw. create_*_error_instance still clears it: it must return a value.
  • Expect::throw_rendered drops its own check and message copy. throw_invalid_matcher_error prints the returned value first. Docs state the rule.
  • Verified: test/js/bun/test/expect.test.js (16 of 26 new tests fail on main), jest-each.test.ts, and the suites in Notes.
  • Self-reviewed: 9 concerns raised, 8 addressed. Not taken: an optional debug_assert! on the clear that stays.

Background

Downsides

  • A test that expects the cut message fails: toThrow("length property") around such a value.
  • expect(Buffer.prototype).toHaveBeenCalled() now throws ERR_INVALID_THIS from Buffer's inspect method, as toBeNull() does on main (console: do not call inspect.custom on a prototype object #41281).
  • Release .text grows 256 B. An invalid expect.extend result costs 1 more allocation.
Notes

The question this answers

When a failing built-in matcher prints a value and code of that value throws, does the matcher report its own message (as before #44833) or the thrown error (as on main)? The thrown error. The check in Expect::throw_rendered is a commented edit of #44833, and test/js/bun/test/printing/dom-nodes.test.ts:732 and :764 pin it for DOM nodes. #40068 made the diff printers do the same in v1.4.1. This PR applies that rule to the messages that still dropped the error, and writes it down.

Measured on other runners (Jest 30.5.2 and Vitest 5.1.0 on Node v26.3.0, one test file per row)

Value ; matcher before #44833 main Jest Vitest
inspect method throws ; toBeNull() own message, cut method's error own message, method not called method's error
inspect method throws ; toBe(1) own message, cut method's error own message method's error
getter for the inspect method throws ; toBeNull() own message, cut getter's error getter's error getter's error
array index getter throws ; toBeNull() own message, cut getter's error getter's error getter's error
Map subclass whose entries() throws ; toBeNull() own message, cut its error its error its error
own getter throws ; toBeNull() own message, [Getter] own message, [Getter] getter's error getter's error

The nine throw! rows of the new test block pass on Vitest 5.1.0. On Jest 30.5.2 all nine keep Jest's message, so the block skips them there.

The 16 sites that now throw the value's error (each printed a message cut at the colon on main)

  • expect/toHaveLength.rs:31 and :56, expect/toMatch.rs:21 and :28, expect/toBeInstanceOf.rs:23
  • expect.rs:3224 (toHaveBeenCalled(), toHaveReturned() and the other mock matchers on a value that is not a mock), :3247 (an entry of mock.results that is not a result)
  • expect/toHaveBeenCalledBefore.rs:44, expect/toHaveBeenLastCalledWith.rs:37, expect/toHaveResolved.rs:56
  • expect.rs:562, :578, :597 (.resolves and .rejects on a value that is not a promise, or on a promise settled the other way)
  • expect.rs:1862 (expect.assertions(v)), expect.rs:1602 (an invalid expect.extend result), ScopeFunctions.rs:254 (test.each(v))

A scan with one row per site: 25 of 58 rows change, the other 33 are byte-identical. The inspect method is called the same number of times on all 34 rows of a call-count probe.

Call sites that can take the new branch

Measurements (release builds of main 7ca0042 and of this branch, same build directory)

  • .text: 110,627,497 B to 110,627,753 B. The stripped binary has the same file size.
  • 5 functions change size: throw_formatted +290 B (new), Expect::throw_rendered 267 to 155, execute_custom_matcher +52, throw 42 to 21, throw_type_error 45 to 24.
  • #[inline(never)] on throw_rendered: without it the smaller function is inlined into 13 matchers and .text grows 4,352 B.
  • A throw whose message formats normally (expect.assertions(-1), gdb single-step, 6 consecutive calls after 500 warm-ups, JIT off): 3,764 instructions and 486 conditional branches on main, 3,760 and 486 here.
  • has_exception() compiles to an inlined load and compare. The new one runs only after a failed write.
  • Allocator entries per call (mimalloc breakpoints, N=1000 against N=2000):
    • failing toBeNull() on { a: 1 }: 9 malloc and 1 realloc on main, 8 malloc and 0 realloc here
    • failing toHaveLength(1) on { a: 1 }: 9 malloc on both
    • invalid expect.extend result: 10 malloc on main, 11 malloc and 1 realloc here
  • A matcher that passes: the bodies of toBeNull, toHaveLength, toMatch, toBe and toEqual are instruction-identical in both builds (108, 306, 300, 356 and 147 instructions).

Node (v26.3.0, same value): assert.match(v, /a/) and assert.ifError(v) throw the method's error after one call. util.inspect(v) and console.log(v) do too, in Node and in Bun.

Not in this PR

Self-review

  • Split out: the Bun.$ message is its own PR (Bun.$: name a rejected object as Object.prototype.toString() does #48114), with Object.prototype.toString() in place of the C++ class name.
  • Docs and comments no longer claim Jest parity for the inspect method. The rule is also at Bun.inspect.custom (docs/runtime/utils.mdx, bun.d.ts).
  • Not taken: debug_assert!(!failed) before the clear in error_message, and a lint for to_fmt inside create_*_error_instance. Both are optional and change what a debug build does for every other caller.

Suites (debug build)

  • expect.test.js (490 pass), mock-fn.test.js (848), spyMatchers.test.ts (150), vitest-apis.test.ts (151), jest-each.test.ts (36), printing/dom-nodes.test.ts (79), expect-extend.test.js (122), expect-toHaveReturnedWith.test.js (13)
  • Under BUN_JSC_validateExceptionChecks=1: the new tests, dom-nodes.test.ts, expect-extend.test.js, expect-extend-matcher-utils-throw.test.ts, expect-symbol-toPrimitive-crash.test.ts
  • expect-deferred.test.ts and vitest-flavor.test.ts: all pass except subprocess tests that hit the 5 s timeout, which main's debug build also hits on the same machine
  • tsc --noEmit in test/, test/integration/bun-types/bun-types.test.ts

[human-review] gate passed · iteration 3 · 7 files touched

fails on main (without fix)
ASAN without fix: 17 failed, 2 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/test/expect.test.js test/js/bun/test/jest-each.test.ts
bun test v1.4.4 (367d939d9)

test/js/bun/test/expect.test.js:
(pass) expect() > () [186.24ms]
(pass) expect() > toBe() > expect(0).toBe(0) == true [1.98ms]
(pass) expect() > toBe() > expect(0).toBe(0) == true [0.41ms]
(pass) expect() > toBe() > expect(0).toBe(0) == true [0.18ms]
(pass) expect() > toBe() > expect(-0).toBe(-0) == true [0.17ms]
(pass) expect() > toBe() > expect(1).toBe(1) == true [0.18ms]
(pass) expect() > toBe() > expect(1).toBe(1) == true [0.12ms]
(pass) expect() > toBe() > expect(NaN).toBe(NaN) == true [0.10ms]
(pass) expect() > toBe() > expect(Infinity).toBe(Infinity) == true [0.10ms]
(pass) expect() > toBe() > expect({}).toBe({}) == true [0.10ms]
(pass) expect() > toBe() > expect(Symbol(a)).toBe(Symbol(a)) == true [0.10ms]
(pass) expect() > toBe() > expect(0).toBe(false) == false [1.12ms]
(pass) expect() > toBe() > expect(0).toBe("") == false [0.30ms]
(pass) expect() > toBe() > expect(0).toBe(-0) == false [0.17ms]
(pass) expect() > toBe() > expect(0).t
... (truncated)

release without fix: BUILD FAILED (no junit output)
bun test v1.4.3-canary.1 (367d939d9)

test/js/bun/test/expect.test.js:
(pass) expect() > () [1.57ms]
(pass) expect() > toBe() > expect(0).toBe(0) == true [0.05ms]
(pass) expect() > toBe() > expect(0).toBe(0) == true
(pass) expect() > toBe() > expect(0).toBe(0) == true
(pass) expect() > toBe() > expect(-0).toBe(-0) == true
(pass) expect() > toBe() > expect(1).toBe(1) == true
(pass) expect() > toBe() > expect(1).toBe(1) == true
(pass) expect() > toBe() > expect(NaN).toBe(NaN) == true
(pass) expect() > toBe() > expect(Infinity).toBe(Infinity) == true
(pass) expect() > toBe() > expect({}).toBe({}) == true
(pass) expect() > toBe() > expect(Symbol(a)).toBe(Symbol(a)) == true
(pass) expect() > toBe() > expect(0).toBe(false) == false
(pass) expect() > toBe() > expect(0).toBe("") == false
(pass) expect() > toBe() > expect(0).toBe(-0) == false
(pass) expect() > toBe() > expect(0).toBe(-0) == false
(pass) expect() > toBe() > expect(1).toBe(2) == false
(pass) expect() > toBe() > expect(1).toBe(true) == false
(pass) expect() > toBe() > expect(1).toBe("1") == false
(pass) expect() > toBe() > expect(Infinity).toBe(-Infinity) == false
(pass) expect() > toBe() > expect("foo").toBe("
... (truncated)
passes on PR (with fix)
ASAN with fix: 2 skipped
$ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/bun/test/expect.test.js test/js/bun/test/jest-each.test.ts
bun test v1.4.4 (367d939d9)

test/js/bun/test/expect.test.js:
(pass) expect() > () [172.08ms]
(pass) expect() > toBe() > expect(0).toBe(0) == true [1.43ms]
(pass) expect() > toBe() > expect(0).toBe(0) == true [0.33ms]
(pass) expect() > toBe() > expect(0).toBe(0) == true [0.12ms]
(pass) expect() > toBe() > expect(-0).toBe(-0) == true [0.10ms]
(pass) expect() > toBe() > expect(1).toBe(1) == true [0.09ms]
(pass) expect() > toBe() > expect(1).toBe(1) == true [0.10ms]
(pass) expect() > toBe() > expect(NaN).toBe(NaN) == true [0.09ms]
(pass) expect() > toBe() > expect(Infinity).toBe(Infinity) == true [0.10ms]
(pass) expect() > toBe() > expect({}).toBe({}) == true [0.11ms]
(pass) expect() > toBe() > expect(Symbol(a)).toBe(Symbol(a)) == true [0.10ms]
(pass) expect() > toBe() > expect(0).toBe(false) == false [0.79ms]
(pass) expect() > toBe() > expect(0).toBe("") == false [0.26ms]
(pass) expect() > toBe() > expect(0).toBe(-0) == false [0.18ms]
(pass) expect() > toBe() > expect(0).t
... (truncated)

release with fix: 2 skipped
$ bun scripts/build.ts --profile=release
[configured] bun-profile → bun (stripped)
  target       linux-x64-gnu
  build type   Release
  build dir    ./build/release
  revision     95b90b8df6
  features     lto, baseline

20 deps, 447 codegen, 1236 objects in 780ms

ninja: Entering directory `/workspace/bun/build/release'
[1/5] fetch lolhtml
[lolhtml] up to date
[2/5] fetch rust-argon2
[rust-argon2] up to date
[2/5] cargo plan → /workspace/bun/build/release/rust-target/plan.json
303 units: 205 lib, 32 custom-build (host), 28 run custom-build, 16 proc-macro (host), 17 lib (host), 4 run custom-build (host), 1 rlib
[4/5] fetch WebKit (prebuilt)
[WebKit] fetching https://github.com/oven-sh/WebKit/releases/download/autobuild-5a61071f79134bf41844e7ea31a8449147d85dd4/bun-webkit-linux-amd64-lto-aot.tar.gz
[WebKit] extracted to /root/.bun/build-cache/webkit-5a61071f79134bf4-lto-aot (20.4s)
[4/5] reconfigure
[1/7] fetch WebKit (prebuilt)
[WebKit] up to date
[1/1631] rustc bun_output_tags 
[2/1631] rustc fnv 
[3/1631] rustc unicode_xid 
[4/1631] rustc unicode_ident 
[5/1631] rustc ident_case 
[6/1631] mkdir codegen
[7/1631] mkdir stamps
[8/1631] rustc heck 
[9/1631] rustc 
... (truncated)
diff hotspot
docs/runtime/utils.mdx             |  2 +
 docs/test/writing-tests.mdx        |  2 +
 packages/bun-types/bun.d.ts        |  3 ++
 src/jsc/JSGlobalObject.rs          | 36 ++++++++++++++---
 src/runtime/test_runner/expect.rs  | 24 ++++++------
 test/js/bun/test/expect.test.js    | 79 ++++++++++++++++++++++++++++++++++++++
 test/js/bun/test/jest-each.test.ts | 21 ++++++++++
 7 files changed, 149 insertions(+), 18 deletions(-)

gate history · 1 passed · 0 rejected · iteration 3

evidence per changed file
file                                reads  edits  tests
docs/runtime/utils.mdx                  0      0     32
docs/test/writing-tests.mdx             1      1     33
packages/bun-types/bun.d.ts             0      0     32
src/jsc/JSGlobalObject.rs               1      1     35
src/runtime/test_runner/expect.rs       2      3     32
test/js/bun/test/expect.test.js         1      2     30
test/js/bun/test/jest-each.test.ts      1      1     18

@coderabbitai

coderabbitai Bot commented Nov 7, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

Message formatting and matcher error handling now preserve errors thrown by custom inspection methods. New tests cover matcher and parameterized-test calls, and documentation describes the behavior.

Changes

Custom inspection error propagation

Layer / File(s) Summary
Preserve formatting errors
src/jsc/JSGlobalObject.rs, src/runtime/test_runner/expect.rs
Message formatting returns bytes and a failure flag. Throw and matcher error paths propagate formatting errors rather than replacing them with a new error or matcher message.
Test and document propagation
test/js/bun/test/expect.test.js, test/js/bun/test/jest-each.test.ts, docs/runtime/utils.mdx, docs/test/writing-tests.mdx, packages/bun-types/bun.d.ts
Tests check error propagation from matcher and parameterized-test calls, and verify that selected passing matchers do not invoke inspection. Documentation describes the behavior.

Possibly related issues

  • oven-sh/bun#47606 — The change propagates errors raised during matcher message formatting, which addresses the reported swallowed-formatting-error behavior.
  • oven-sh/bun#44880 — The change propagates errors from custom inspection when matchers call those hooks, but does not address the reported cases where diff matchers do not call them.
Merge Risk: Minimal · up to 028d8

No concrete merge-blocking issue was identified. The change appears mergeable subject to normal build and test checks.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title clearly describes the main exception-propagation change.
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description follows the required template. It has the "What does this PR do?" section, which covers the problem, the fix, and the background. It also has the "How did you verify your code works?" …
Full details: Description check

Explanation

The description follows the required template. It has the "What does this PR do?" section, which covers the problem, the fix, and the background. It also has the "How did you verify your code works?" section, which lists the failing-on-main and passing-on-branch tests, the debug-build and release-build suites, the type checks, and the allocation and size measurements. The author also states the merge dependency on #48114 and lists the downsides, including the tests that expect the old truncated message and the 256 B increase in release .text. The description is detailed and specific, so it is mostly complete. Its main gap is structural: the required headings are not used as-is, and the evidence sits in a long Notes block. This does not prevent a reviewer from deciding on the change.

  • 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

Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 7ca0042 and 028d87a.

Files selected for processing: 7

  • docs/runtime/utils.mdx
  • docs/test/writing-tests.mdx
  • packages/bun-types/bun.d.ts
  • src/jsc/JSGlobalObject.rs
  • src/runtime/test_runner/expect.rs
  • test/js/bun/test/expect.test.js
  • test/js/bun/test/jest-each.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.

@github-actions github-actions Bot added the claude label Nov 7, 2026
@robobun

robobun commented Nov 7, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 3:41 AM PT - Nov 9th, 2026

❌ @robobun, your commit 95b90b8 has 5 failures in Build #139162 (All Failures):


🧪   To try this PR locally:

bunx bun-pr 48119

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

bun-48119 --bun

@robobun

robobun commented Nov 7, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status

Reproduced on main (7ca0042, debug build) with one test file:

import { test, expect } from "bun:test";
const v = {
  a: 1,
  [Symbol.for("nodejs.util.inspect.custom")]() {
    throw new Error("zz-boom");
  },
};
test("a failing matcher", () => {
  expect(v).toBeNull();
});
test("an argument check", () => {
  expect(v).toHaveLength(1);
});

Main reports error: zz-boom for the first test and error: Received value does not have a length property: for the second. This branch reports error: zz-boom for both.

16 of the 26 new tests in test/js/bun/test/expect.test.js fail on main, and all 26 pass on this branch. The new test in test/js/bun/test/jest-each.test.ts fails on main and passes on this branch.

PR: #48119. Merge #48114 first.

Rebased onto main 2596ac5. The diff did not change.

CI (build 139162, head 95b90b8, finished)

No red test comes from this PR. test/js/bun/test/expect.test.js and test/js/bun/test/jest-each.test.ts pass on every lane. 182 jobs passed and 6 failed.

  • test/js/third_party/grpc-js/test-tonic.test.ts, test/js/bun/test/parallel/test-docker-build-alpine.ts and test/js/bun/test/parallel/test-docker-build-debian.ts download from GitHub releases. So does the step linux aarch64-musl - verify-baseline. GitHub returned HTTP 504 for release downloads during the run. The log of that step shows curl: (22) The requested URL returned error: 504.
  • test/aot/heap-limit.test.ts failed on alpine 3.23 x64 with memory allocation of 8 bytes failed and exit code 134. Build 138106 of Check the xwin archive's sha256; ignore bin paths that contain a NUL #48050 shows the same crash in this file.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

Comment thread src/jsc/JSGlobalObject.rs Resolved

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

@robobun

robobun commented Nov 7, 2026

Copy link
Copy Markdown
Collaborator Author

On the two issues listed as possibly related: this PR leaves both as they are.

JSGlobalObject::throw and throw_type_error return the pending exception when
a Display impl ran JS that threw while the message was formatted. They used
to clear it and throw the cut message. create_*_error_instance still clears
it, because it has to return a value.

bun:test: every message a built-in matcher builds now follows one rule. If a
value's inspect method throws while the value is printed, the matcher throws
that error. Expect::throw_rendered no longer renders the message twice to
check for this, and InvalidMatcherError prints the returned value before it
builds its error.

docs: state the rule under Matchers and at Bun.inspect.custom.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants