error.stack: report frames at new X(...) at the new keyword - #37396
error.stack: report frames at new X(...) at the new keyword#37396robobun wants to merge 7 commits into
Conversation
formatStackTrace used JSC's raw divot (the end of the callee) for the .stack string and fed it into the source map remap, while Bun.inspect and Error.prepareStackTrace CallSites already go through getAdjustedPositionForBytecode, which moves construct positions back to the new keyword like V8. The divot has no source map mapping of its own, so the remap snapped to the callee identifier, or to another line when the callee had been inlined. Use the adjusted position in formatStackTrace too. adjustPositionBackwards now handles 16-bit sources (eval, new Function and node:vm code is not transpiled) instead of zeroing the position or hitting ASSERT_NOT_REACHED, counts only the line breaks between the expression start and the divot, fixes the column when the expression starts on the first line, and keeps the divot when it cannot recount.
WalkthroughChangesThis change centralizes stack-frame position adjustment, supports additional source-text formats, and reuses adjusted positions in stack formatting and exception handling. Tests cover constructors, source maps, virtual-machine offsets, Unicode text, and line endings. ChangesStack position adjustment
Possibly related PRs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Status: ready for review. CI is green on the current head b0386de (build 92164, 181/181) and was green on 2f732e4 before the last round (build 91958); every review thread is addressed or answered in place. Reproduced on bun 1.4.0 and a debug build of main with the fixture in Verified locally with the debug build: the new tests fail without the Review so far: the two findings on the line recount (other line terminators, node:vm columnOffset) are fixed in b7357f6 with tests; the 1:1 / column-1 rendering finding is fixed in 2f732e4 with tests; the comment-length and the two CodeRabbit findings are resolved in their threads. #37388 is independent but touches the same expectations, see the comment below for what needs refreshing when the second of the two lands. |
|
Updated 5:00 AM PT - Aug 11th, 2026
✅ @robobun, your commit b0386de2ddc4f125b310e67cdedc261e4402dff3 passed in 🧪 To try this PR locally: bunx bun-pr 37396That installs a local version of the PR into your bun-37396 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…ng construct positions adjustPositionBackwards only recognized LF when it had to recount the line and column of a construct spanning lines, while JSC counts CR, CRLF, U+2028 and U+2029 as well, and it dropped the source's start column (node:vm columnOffset) for expressions starting on the first line. Also update the dev server source map test, whose server frames now land on the column 1 mapping like the other cases in that file.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/jsc/bindings/FormatStackTraceForJS.cpp`:
- Around line 264-272: Update the remapping branch in FormatStackTraceForJS
around hasLineAndColumnInfo() to first check whether frame.hasBytecodeIndex() is
available; use Bun::getAdjustedPositionForStackFrame(frame) only when it is, and
fall back to frame.computeLineAndColumn() when the bytecode index is unset so
existing line and column data is preserved.
In `@test/js/node/test/parallel/test-vm-context.js`:
- Around line 115-117: Update the Bun-specific caret regex in the stack
assertion to match the observed exact indentation before ^ rather than allowing
zero or more spaces. Keep the V8 regex unchanged and preserve the shared
expected-filename.js:33:131 assertion, ensuring the Bun check can fail when the
caret column changes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b6c14230-2449-4a8e-87dc-9eb0678bddc1
⛔ Files ignored due to path filters (1)
test/js/node/vm/__snapshots__/vm-sourceUrl.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (9)
src/jsc/bindings/ErrorStackFrame.cppsrc/jsc/bindings/ErrorStackFrame.hsrc/jsc/bindings/FormatStackTraceForJS.cppsrc/jsc/bindings/ZigException.cpptest/bake/dev/server-sourcemap.test.tstest/js/bun/test/stack.test.tstest/js/node/test/parallel/test-vm-basic.jstest/js/node/test/parallel/test-vm-context.jstest/js/node/test/parallel/test-vm-run-in-new-context.js
|
Follow-ups pushed:
Related: #37388 stops the transpiler from rewriting |
formatStackTrace decided whether a frame has a position by checking for a non-zero line or column, so a frame at column 1 printed as file.js:8 and a frame at 1:1 printed no position at all. Constructs at the start of a line now legitimately land on column 1, so key the suffix on whether pass 1 produced a position instead. Calls at column 1 gain their :1 as well, which is what V8 prints.
|
2f732e4 addresses the remaining review finding, and it was a bit wider than the 1:1 example: |
There was a problem hiding this comment.
Reviewed again after 2f732e4 — no new findings. That commit addresses the earlier 1:1-position note (the :line:column suffix is now keyed on originalPositions[i].line_zero_based >= 0, and the byteZero case pins at byte-zero.js:1:1); the same commit also drops the inner displayColumn > 0 check so column-1 frames print :1, covered by callAtColumnOne/constructAtColumnOne. Given this rewrites adjustPositionBackwards and changes the .stack rendering path for every frame, a maintainer sign-off would still be worthwhile.
What was reviewed:
getAdjustedPositionForStackFrameguard vs.hasLineAndColumnInfo()— the code-block-without-bytecode-index case returns the -1 sentinel andhasPositionskips it, same net effect as before.adjustPositionBackwards: bounds check againstsource.length(), CRLF counted as one break, first-linestartPosition().m_columnadd, and the failure paths now leaveposunchanged instead of zeroing it.ZigException.cpp:byte_position < 0gate replaces the removedhasBytecodeIndex()branch so the source-line scan cannot run with a -1 offset.
Extended reasoning...
Overview
The PR routes formatStackTrace (the .stack string producer) through the same getAdjustedPositionForBytecode helper that Bun.inspect and Error.prepareStackTrace CallSites already use, so frames at new X(...) report the new keyword's column in all three renderings. adjustPositionBackwards is rewritten to index StringView directly (both encodings), count all four ECMAScript line terminators, honour the source's start column on line 1, and leave the position unchanged rather than zeroing it when it can't recount. ZigException.cpp's dead !hasBytecodeIndex() branch is folded into the new getAdjustedPositionForStackFrame helper. The latest commit (2f732e4) keys the :line:column suffix on whether pass 1 produced a position, fixing the 1:1 and column-1 cases raised in the previous review round.
Security risks
None. This is stack-frame position arithmetic over source text already held by JSC; no user-controlled sizes drive allocation, and the one bounds check (pos.byte_position > source.length()) bails out before indexing.
Level of scrutiny
Medium-high. formatStackTrace runs for every .stack materialization, and adjustPositionBackwards is now reachable from three consumers on 8- and 16-bit sources. The rewrite is small but subtle (line-terminator set, CRLF collapsing, first-line columnOffset, off-by-one at index 0) and each edge is covered by an assertion in stack.test.ts. Three vendored node:vm tests move back toward upstream expectations, which is the direction the repo prefers.
Other factors
Two prior automated-review rounds each found one issue and both were fixed with dedicated tests (b7357f6 for line terminators / columnOffset, 2f732e4 for the 1:1 guard). CodeRabbit's two findings were argued and withdrawn. The change also silently makes every column-1 frame print :1 (matching V8) and always prints the column when a line is printed — a user-visible format change beyond constructs, which is why a maintainer look is still warranted.
The .stack string and CallSite paths only need a line and column, and for every frame that is not at a construct that is JSC's own position, which CodeBlock::lineColumnForBytecodeIndex caches per bytecode index. Decoding the expression info entry (a linear walk through its chapter) is now limited to construct frames, where the divot and start offset are needed, and to the code frame printer, which needs the source offset. Also cover the parts of the recount that had no failing test: line terminators other than LF before the new keyword (where the column count has to stop), and a class expression callee, which the transpiler keeps on several lines so transpiled code reaches the recount as well.
|
CI for 2f732e4 was green (build 91958). A further self-review pass turned up three things, all pushed in 1fd3ef1 (b0386de is comments only):
|
There was a problem hiding this comment.
I re-reviewed after 1fd3ef1 / b0386de and didn't find any further issues — the three earlier findings (columnOffset on line 1, non-LF line terminators, and the 1:1 / column-1 suffix guard) are all fixed with covering tests. Given this rewrites the position-adjustment path that feeds .stack, source-map remapping, and err.line/err.column, and changes the output shape (always :line:column), a human sign-off would still be worthwhile.
What was reviewed:
adjustPositionBackwardsbounds, CRLF-as-one-break handling, and thei < 0startColumn add-back — indexing stays within[0, source.length()).getAdjustedLineColumnForBytecode: non-construct frames go throughlineColumnForBytecodeIndex(same lookup as pre-PRcomputeLineAndColumn); thehasBytecodeIndex()guard makes the removed ZigException fallback safe.hasPositioninformatStackTracecorrectly distinguishes the -1 sentinel from a real 0:0; remapped frames still print because the flag keys on the original, not the remapped, position.
Extended reasoning...
Overview
The PR unifies the three renderings of a stack frame at new X(...) (.stack string, Bun.inspect, CallSite objects) so they all report the new keyword's position, matching Node/V8. It rewrites adjustPositionBackwards in ErrorStackFrame.cpp (16-bit source support, all four ECMAScript line terminators, node:vm columnOffset on line 1, keep-divot-on-failure instead of zeroing), adds a getAdjustedLineColumnForBytecode variant that only decodes expression info for construct opcodes, wires formatStackTrace and JSCStackFrame::calculateSourcePositions to it, replaces the > 0 suffix guard in formatStackTrace with an explicit hasPosition flag, and folds ZigException.cpp's dead !hasBytecodeIndex fallback into the shared helper. Tests cover the transpiled matrix, eval/vm sources across encodings and terminators, column-1 and byte-0 frames, and three vendored node:vm tests move back toward upstream assertions.
Security risks
None. This is display-side stack formatting; inputs are JSC-produced positions and the source provider's own text. The only untrusted-length read is source[i] in the recount loops, which are bounded by start >= 0, i < pos.byte_position, and the explicit pos.byte_position > source.length() bail. No allocation sizing, no user-controlled indices.
Level of scrutiny
Moderate-high. error.stack is user-visible output that external tooling (error reporters, source-map remappers, test runners) parses, and this changes its shape for every frame that lands at column 1 (now :N:1 instead of :N). It's also on the .stack materialization path, so the opcode-first split in 1fd3ef1 was specifically added to keep non-construct frames on the cached lookup — that's a perf-sensitive choice a maintainer should confirm. The adjustPositionBackwards rewrite touches encoding-sensitive indexing over StringView.
Other factors
All three of my earlier inline findings were fixed with dedicated test cases; CodeRabbit's two findings were withdrawn after discussion. CI was reported green on 2f732e4. The PR description flags a known interaction with #37388 (two expectations to refresh whichever lands second) that whoever merges should be aware of. The change is well-scoped and thoroughly tested, but not mechanical — deferring rather than approving on that basis.
1 participant
Problem
For a frame that is sitting at a
new X(...)expression, the column in theerror.stackstring points atX(or somewhere else entirely), while Node,Bun.inspect(err)/ the uncaught error printer, andError.prepareStackTraceCallSites all point at thenewkeyword.err.stackBun.inspect(err)err.stackcustomError:4:9:4:13:4:9:4:9userConstructor:7:10:7:14:7:10:7:10localBinding:11:10:10:16(previous line):11:10:11:10The same applies to
new Map(1),new Map(...args),Error.captureStackTrace, anderr.line/err.column. In code that is not transpiled (node:vm,eval) it also applies tothrow new Error(...):vm.runInNewContext('throw new Error("foo")')reports:1:16where node reports:1:7.Cause
JSC's position for a construct is the divot at the end of the callee. Since #11581,
Bun::getAdjustedPositionForBytecode(ErrorStackFrame.cpp) movesop_construct*/op_super_construct*positions back to the start of the expression, which is where V8 reports them, and both the code frame printer (ZigException.cpp) and the CallSite path (JSCStackFrame::getSourcePositions) use it.formatStackTracein FormatStackTraceForJS.cpp, which produces the.stackstring, still used the rawframe.computeLineAndColumn()and fed that into the source map remap. The transpiler's source map has a mapping at the start of thenewexpression but none at the divot, so the remap snaps the divot to whatever mapping precedes it: normally the callee identifier, and when the callee was inlined (localBindingabove) the identifier's original location on another line.Fix
formatStackTracetakes its positions from the same adjustment the other two consumers use (newgetAdjustedLineColumnForStackFramewrapper in ErrorStackFrame.cpp), so the three renderings agree and the remap starts from a position the source map has.err.line/err.column/originalLine/originalColumnare derived from the same values and follow. The line/column variant looks at the opcode first: only construct frames decode the expression info (needed for the divot and start offset), every other frame keeps usingCodeBlock::lineColumnForBytecodeIndex, the cached lookupcomputeLineAndColumn()used before, so the.stackpath costs what it did before for the frames it does not change. The CallSite path (JSCStackFrame::calculateSourcePositions) is moved to the same variant, it was decoding every frame;ZigException.cppkeeps the full variant because the source preview needs the offset.formatStackTraceused to decide whether a frame has a position by checking for a non-zero line or column, so a frame at column 1 printed asfile.js:8and a frame at 1:1 printed no position at all. Constructs at the start of a line now legitimately land on column 1, so the:line:columnsuffix is keyed on whether pass 1 produced a position instead. Calls at column 1 gain their:1too (at file.js:8:1, which is what node prints;Bun.inspectalready printed it).ZigException.cppis switched to the same helper. No behavior change: its fallback for a frame with a code block but no bytecode index was unreachable (JSC always records both), and would have hit theRELEASE_ASSERTinlineColumnForBytecodeIndexif it ever ran; the helper reports no position instead.adjustPositionBackwards, which now has one more caller, is made to work whennewand the callee are on different lines. Transpiled code mostly takes the same-line path (the printer putsnewand a simple callee back on one line; a class or function expression callee still spans lines, covered by theclassExpressionCalleecase, which main reports two lines off), andeval,new Functionandnode:vmcode hits it with whatever text the user wrote, in the encoding of the string it came from. On a 16-bit source the function zeroed the position in release builds and hitASSERT_NOT_REACHEDin debug builds, reachable today throughBun.inspectandError.prepareStackTrace. It now indexes theStringViewdirectly (works for both encodings), counts the same line terminators JSC's lexer counts (LF, CR, CRLF, U+2028, U+2029) and only the ones between the expression start and the divot (the old loop also looked at the character at the divot, soreturn new\n Map\n (1)printedat construct (file.js:)with no position), counts the column correctly when the expression starts on the first line of the source (thei > 0loop bound was off by one, and the source's start column, node:vm'scolumnOffset, which JSC includes in first-line columns, was dropped), and leaves the divot position alone instead of reporting1:1when it cannot recount.Why this is the right behavior
The
.stackstring is what external tools read (source map remappers, error reporters, test runners building code frames from stack strings), and V8 reports thenewkeyword there. Bun already chose thenewkeyword in #11581 for its other two renderings of the same frame; the string was the odd one out, and because of the source map behavior above the old column was not a consistent JSC flavored answer either, just whatever mapping happened to sit before the divot. Three node:vm tests from Node's suite had been given Bun-specific expected columns for this (#32018): with this changetest-vm-run-in-new-context.jsandtest-vm-context.jsproduce Node's column and go back to the upstream assertion (the latter still needs its loose caret check), andtest-vm-basic.jsnow only differs in the ways its comment already lists.What does not change: frames at calls,
throw, property accesses and so on (only construct opcodes are adjusted);Bun.inspectand CallSite output for constructs on a single line;new Error(...)in transpiled code, which the runtime transpiler currently lowers to a call. Cost: for frames that are not at a construct, the.stackpath runs the same cachedlineColumnForBytecodeIndexlookup it ran before; an earlier revision of this PR decoded the expression info for every frame, which review flagged, hence the opcode check. Construct frames pay one expression info decode, whichBun.inspectand CallSites were already paying for every frame; the CallSite path now pays it only for constructs too. Release builds, CPU time pernew Error().stack, minimum of 8 interleaved runs on a heavily loaded machine (so only good to a few percent): 10 small frames: pre-PR 8.1us, decode-every-frame revision 8.4us, this revision 8.0us; error created at the end of a 3000-statement function: 7.0us / 7.4us / 7.2us (the pre-PR binary is the published canary of a slightly older base, the other two are local builds).Tests
test/js/bun/test/stack.test.ts:newwith spread (op_construct_varargs), a callee the transpiler inlines (wrong line before),newsplit across lines,Error.captureStackTrace, anderr.line/err.column. Before this change it reports13:13, 16:14, 19:14, 22:14, 25:16, 30:5, 34:7instead of13:9, 16:10, 19:10, 22:10, 26:10, 29:10, 34:3. Two more cases put the call and the construct at column 1; before this change they printfixture.js:39(no column) andfixture.js:42:5, now39:1and42:1.newon an earlier line than the callee (latin1, UTF-16, one where the expression starts on line 1 with a line break right after the callee, CR / CRLF / U+2028 separators betweennewand the callee and, withnewon line 2, CR / U+2028 / U+2029 before it as well (so the column count has to stop at them), a vm script withcolumnOffset: 100, and a vm script that starts with thenew, whose frame used to print asat byte-zero.js:1:8and now prints:1:1), checking.stack,Bun.inspectand CallSites. Before this change.stackreports the divot (4:8,4:8,2:6, ...); the UTF-16 source reports1:1fromBun.inspectand line 1 from CallSites in release builds and aborts onASSERT_NOT_REACHEDin debug builds; the line 1 source prints no position at all fromBun.inspect. Node reports3:10,3:10,1:31,1:31x3 and1:131for these, which is what they now produce everywhere.Updated expectations: the
vm-sourceUrl.test.tssnapshot (2:16->2:7, node prints2:7), the three node:vm tests above (which still pass on Node v26), and the first case oftest/bake/dev/server-sourcemap.test.ts. That file's server-side frames currently land one line above the statement (the server HMR chunk's source map accounts for the client chunk's one-line prefix, a pre-existing bug that is being handled separately), and the column is whatever mapping on that line precedes the frame's generated column: forthrow new Error(...)the old divot column picked the(offunction myFunc((6:16), thenewcolumn picks column 1 like thethrowError/6:1,helperFunction/5:1and churn cases in the same file already do.[review] gate passed · iteration 0 · 11 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 0 rejected · iteration 0
evidence per changed file