Repository navigation
Conversation
…a build or resolve error BuildMessage and ResolveMessage print from their Msg, so print_error_instance_body never ran for them and the hook that captures an error for the JUnit report never fired. A test that rejected with a syntax error or a missing module got <failure type="Error" /> with no message. print_error_from_maybe_private_data now calls the hook, when one is installed, with the class name, the message text and one frame made from the message's location.
|
Status: ready for review. First PR of a stack: #44434 (this one), #44435, #44436 (draft, optional). How I reproduced it:
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (3)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review. WalkthroughWhen side effects are allowed, ChangesMessage Exception Reporting
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The change appears mergeable after normal checks. The new JUnit test has not been executed in this review. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the lifetime of the synthesized ZigException: name/message/source_url are borrowed from the Msg and a stack array, but the only hook consumer (TestFailure::record in src/runtime/cli/test_command.rs) copies name, message and each frame into owned Vecs synchronously, so nothing outlives the call. The with_stack refactor in src/jsc/ZigException.rs is field-for-field identical to the initializer it replaced.
Extended reasoning...
The change adds a hook call for BuildMessage/ResolveMessage in src/jsc/VirtualMachine.rs, extracts a ZigException constructor in src/jsc/ZigException.rs, and adds one junit reporter test; it touches no security-sensitive surface. Two inline findings (missing JS frames for synchronous throws, and the test discarding stdout/stderr) were already posted, so a human look is signaled by those; this note records only what else was examined and ruled out.
|
Closing: one PR in place of the stack. This change is the first commit of #44435, which now targets main. |
Problem
<failure type="Error" />with no type, message or location.bun testcaptures a failure through the hookon_print_error_zig_exception, whichprint_error_instance_bodycalls (src/jsc/VirtualMachine.rs:6769).BuildMessageandResolveMessageprint from theirMsginprint_error_from_maybe_private_data(:6036) and return before that call.Fix
print_error_from_maybe_private_datacalls the hook for both classes. It passes the class name, the message text and one frame from the location of the message.allow_side_effects, like the existing call. It also runs when the message was logged before.test/js/junit-reporter/junit.test.jsfails on main. Also raninspect-error.test.js,resolve-error.test.ts,build-error.test.ts,reportError.test.ts.Background
BuildMessageandResolveMessageare what bun throws for a transpile error and a failed resolution. Each holds abun_ast::Msg: text, file, line, column.bun testsets it while it prints the error of a failed test, andTestFailure::record_cbfills<failure>. No other code sets it.nameandmessagefrom the JS value in the reporter. That can run user code, and bun already has the text.Downsides
BuildMessageorResolveMessageexecutes 7 more instructions when no hook is installed (89 to 96 inprint_error_from_maybe_private_data).nm -S).<failure>has one frame, without the notes of the message. The inspector and GitHub annotations still get nothing for these classes.Notes
<failure type="BuildMessage" message="Unexpected ;">there because of this PR.<failure type="Error" />. After:<failure type="BuildMessage" message="Unexpected ;">BuildMessage: Unexpected ;andat broken.js:1:11.print_error_instance_bodydoes three things for an error underallow_side_effects: it reports to the inspector, it calls this hook, and it prints a GitHub annotation. This PR adds only the hook call for the two classes. bun test: emit one anchored GitHub Actions annotation per failed test #41876 is open for the annotations.-one-insn-per-tb -d exec,nochain -dfilter <range>) forbun runof a file withconst x = ;, on release builds of the merge base and of this change. It is 90 to 97 for a missing import. The whole process executes about 2.5 million, with a spread of 3% between runs.perf,valgrindandbloatyare not installed on the machine I used.print_error_from_maybe_private_data1,520 to 1,668, plus 977 for the cold helper that builds the exception.ZigException::with_stackholds the field initialiser thatHolder::zig_exceptionhad. The hook call does not need aHolder.