From d8fb407b2dd378f34cb0271939e98721ba540fc9 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 13 Aug 2026 21:35:47 +0000 Subject: [PATCH 1/5] node:vm: apply negative lineOffset/columnOffset to stack frames and the arrow header JSC::SourceCode clamps the line and column a source starts at to 1, so code compiled with a negative node:vm lineOffset or columnOffset reported physical positions: the offset only worked when it was positive. The SourceProvider still carries the requested start, so the stack formatters (error.stack, Error.prepareStackTrace call sites, Bun.inspect and the uncaught error printer) now add the negative part back, which is what V8 reports for the same options. handleException computed the physical line for the arrow header by subtracting the unclamped (negative) start, so it looked up a line that does not exist (no source line or caret) or, with lineOffset -1, showed the line after the throw. It now subtracts only the part JSC actually added and reports the line with the whole offset, like Node. compileFunction's wrapper shift is now unconditional, so body line N reports as N + lineOffset for zero and negative offsets as well. --- src/jsc/bindings/ErrorStackFrame.cpp | 37 +++- src/jsc/bindings/ErrorStackFrame.h | 18 ++ src/jsc/bindings/FormatStackTraceForJS.cpp | 16 +- src/jsc/bindings/NodeVM.cpp | 34 ++-- test/js/node/test/parallel/test-vm-basic.js | 11 +- test/js/node/vm/vm.test.ts | 210 +++++++++++++++++++- 6 files changed, 294 insertions(+), 32 deletions(-) diff --git a/src/jsc/bindings/ErrorStackFrame.cpp b/src/jsc/bindings/ErrorStackFrame.cpp index 806a340be246..3edcac3dd9c2 100644 --- a/src/jsc/bindings/ErrorStackFrame.cpp +++ b/src/jsc/bindings/ErrorStackFrame.cpp @@ -1,13 +1,33 @@ #include "root.h" +#include "ErrorStackFrame.h" #include "JavaScriptCore/CodeBlock.h" -#include "headers-handwritten.h" -#include "JavaScriptCore/BytecodeIndex.h" +#include "JavaScriptCore/StackFrame.h" #include "wtf/Assertions.h" #include "wtf/text/OrdinalNumber.h" namespace Bun { using namespace JSC; +void applyNegativeSourceStart(CodeBlock* code, int& lineZeroBased, int& columnZeroBased) +{ + auto* provider = code->source().provider(); + if (!provider) + return; + + // Function code blocks share the provider of the program they were parsed from, so this is + // the whole source's start even for a frame inside a function. + TextPosition start = provider->startPosition(); + int startLine = start.m_line.zeroBasedInt(); + int startColumn = start.m_column.zeroBasedInt(); + + // JSC only adds the start column to positions on the first physical line, which it reports + // as the clamped start line. + if (startColumn < 0 && lineZeroBased == std::max(startLine, 0)) + columnZeroBased += startColumn; + if (startLine < 0) + lineZeroBased += startLine; +} + /// Adjust a `ZigStackFramePosition` by a number of bytes. This accounts for when the adjustment /// crosses line boundaries, and thus requires the source code in order to properly compute /// the result. @@ -71,6 +91,7 @@ ZigStackFramePosition getAdjustedPositionForBytecode(JSC::CodeBlock* code, JSC:: .column_zero_based = OrdinalNumber::fromOneBasedInt(expr.lineColumn.column).zeroBasedInt(), .byte_position = (int)expr.divot, }; + applyNegativeSourceStart(code, pos.line_zero_based, pos.column_zero_based); auto inst = code->instructionAt(bc); @@ -98,4 +119,16 @@ ZigStackFramePosition getAdjustedPositionForBytecode(JSC::CodeBlock* code, JSC:: return pos; } +ZigStackFramePosition getLineColumnForStackFrame(const StackFrame& frame) +{ + auto lineColumn = frame.computeLineAndColumn(); + ZigStackFramePosition pos { + .line_zero_based = OrdinalNumber::fromOneBasedInt(lineColumn.line).zeroBasedInt(), + .column_zero_based = OrdinalNumber::fromOneBasedInt(lineColumn.column).zeroBasedInt(), + .byte_position = -1, + }; + applyNegativeSourceStart(frame.codeBlock(), pos.line_zero_based, pos.column_zero_based); + return pos; +} + } // namespace Bun diff --git a/src/jsc/bindings/ErrorStackFrame.h b/src/jsc/bindings/ErrorStackFrame.h index d7a04e83d6cd..e2c9e0d26bc6 100644 --- a/src/jsc/bindings/ErrorStackFrame.h +++ b/src/jsc/bindings/ErrorStackFrame.h @@ -1,9 +1,27 @@ +#pragma once + #include "root.h" #include "headers-handwritten.h" #include "JavaScriptCore/BytecodeIndex.h" +namespace JSC { +class CodeBlock; +class StackFrame; +} + namespace Bun { ZigStackFramePosition getAdjustedPositionForBytecode(JSC::CodeBlock* code, JSC::BytecodeIndex bc); +/// JSC::SourceCode counts from its start position clamped to line 1, column 1, so a source +/// created with a negative start (node:vm lineOffset / columnOffset below 0) reports physical +/// positions while its SourceProvider still carries the requested start. This adds back the part +/// JSC dropped, which is the position V8 reports for the same options. No-op for sources that +/// start at or after 1:1, i.e. everything that is not node:vm code. +void applyNegativeSourceStart(JSC::CodeBlock* code, int& lineZeroBased, int& columnZeroBased); + +/// StackFrame::computeLineAndColumn() with applyNegativeSourceStart applied; byte_position is -1. +/// The frame must have a code block (StackFrame::hasLineAndColumnInfo()). +ZigStackFramePosition getLineColumnForStackFrame(const JSC::StackFrame& frame); + } // namespace Bun diff --git a/src/jsc/bindings/FormatStackTraceForJS.cpp b/src/jsc/bindings/FormatStackTraceForJS.cpp index d622679f31aa..e481f622a675 100644 --- a/src/jsc/bindings/FormatStackTraceForJS.cpp +++ b/src/jsc/bindings/FormatStackTraceForJS.cpp @@ -21,6 +21,7 @@ #include "BunClientData.h" #include "CallSite.h" +#include "ErrorStackFrame.h" #include "ErrorStackTrace.h" #include "headers-handwritten.h" @@ -243,11 +244,11 @@ WTF::String formatStackTrace( // file's map once instead of per frame. WTF::Vector remappedFrames; WTF::Vector sourceURLs; - WTF::Vector originalLineColumns; + WTF::Vector originalPositions; remappedFrames.grow(framesCount); memset(remappedFrames.begin(), 0, sizeof(ZigStackFrame) * framesCount); sourceURLs.grow(framesCount); - originalLineColumns.grow(framesCount); + originalPositions.grow(framesCount); bool anyRemap = false; for (size_t i = 0; i < framesCount; i++) { @@ -260,11 +261,11 @@ WTF::String formatStackTrace( remappedFrame.position.line_zero_based = -1; remappedFrame.position.column_zero_based = -1; remappedFrame.position.byte_position = -1; - originalLineColumns[i] = {}; + originalPositions[i] = remappedFrame.position; if (!frame.hasLineAndColumnInfo()) continue; - originalLineColumns[i] = frame.computeLineAndColumn(); + originalPositions[i] = Bun::getLineColumnForStackFrame(frame); JSC::JSGlobalObject* globalObjectForFrame = lexicalGlobalObject; if (auto* callee = frame.callee()) { @@ -280,8 +281,7 @@ WTF::String formatStackTrace( if (isDefinitelyNotRunninginNodeVMGlobalObject || isDefaultGlobalObjectInAFinalizer) { // https://github.com/oven-sh/bun/issues/3595 if (!sourceURLs[i].isEmpty()) { - remappedFrame.position.line_zero_based = OrdinalNumber::fromOneBasedInt(originalLineColumns[i].line).zeroBasedInt(); - remappedFrame.position.column_zero_based = OrdinalNumber::fromOneBasedInt(originalLineColumns[i].column).zeroBasedInt(); + remappedFrame.position = originalPositions[i]; remappedFrame.source_url = Bun::toStringRef(sourceURLs[i]); anyRemap = true; } @@ -316,8 +316,8 @@ WTF::String formatStackTrace( WTF::String sourceURLForFrame = sourceURLs[i]; if (frame.hasLineAndColumnInfo()) { - originalLine = OrdinalNumber::fromOneBasedInt(originalLineColumns[i].line); - originalColumn = OrdinalNumber::fromOneBasedInt(originalLineColumns[i].column); + originalLine = originalPositions[i].line(); + originalColumn = originalPositions[i].column(); displayLine = originalLine; displayColumn = originalColumn; diff --git a/src/jsc/bindings/NodeVM.cpp b/src/jsc/bindings/NodeVM.cpp index 4ed1521cc4bd..3d82809d41e8 100644 --- a/src/jsc/bindings/NodeVM.cpp +++ b/src/jsc/bindings/NodeVM.cpp @@ -188,14 +188,15 @@ JSC::JSFunction* constructAnonymousFunction(JSC::JSGlobalObject* globalObject, c EXCEPTION_ASSERT(!!throwScope.exception() == code.isNull()); // The user's body starts on line 2 of the wrapped program (after the - // "(function () {\n" prefix). Shift the provider's start position up one - // line so reported positions line up with the body the way V8's - // CompileFunction does: body line 1 reports as lineOffset+1. JSC clamps - // non-positive provider start positions to zero, so once the input is - // already <= 0 there is nothing to gain by going further negative; clamp - // there to keep the value bounded for downstream arithmetic. - int lineZeroBased = position.m_line.zeroBasedInt(); - TextPosition wrappedPosition(OrdinalNumber::fromZeroBasedInt(lineZeroBased > 0 ? lineZeroBased - 1 : lineZeroBased), position.m_column); + // "(function () {\n" prefix). Start the program one line above the + // requested offset so body line N reports as N + lineOffset, the way V8's + // CompileFunction does. For lineOffset <= 0 this start is negative; JSC + // counts from line 1 regardless and the stack formatters add the negative + // part back (Bun::applyNegativeSourceStart). + int startLine = position.m_line.zeroBasedInt(); + if (startLine != std::numeric_limits::min()) + startLine--; + TextPosition wrappedPosition(OrdinalNumber::fromZeroBasedInt(startLine), position.m_column); SourceCode sourceCode( JSC::StringSourceProvider::create(code, sourceOrigin, WTF::move(options.filename), sourceTaintOrigin, wrappedPosition, SourceProviderSourceType::Program), @@ -570,23 +571,32 @@ bool handleException(JSGlobalObject* globalObject, VM& vm, NakedPtr:\n\n\n\n + int reportedLine = static_cast(line_and_column.line); String sourceLineText; unsigned caretColumn = 0; if (JSC::CodeBlock* codeBlock = stack_frame.codeBlock()) { if (JSC::SourceProvider* provider = codeBlock->source().provider()) { - int64_t startLineZeroBased = provider->startPosition().m_line.zeroBasedInt(); - int64_t physicalLine = static_cast(line_and_column.line) - startLineZeroBased; + // JSC::SourceCode clamps the start position it counts from to 1:1, + // so line_and_column only carries the non-negative part of + // lineOffset/columnOffset: undo that part to index the source + // text, then report the physical line with the whole offset, as + // Node does (and as Bun::applyNegativeSourceStart does for the + // frames below the header). + TextPosition start = provider->startPosition(); + int startLineZeroBased = start.m_line.zeroBasedInt(); + int64_t physicalLine = static_cast(line_and_column.line) - std::max(startLineZeroBased, 0); + reportedLine = static_cast(physicalLine + startLineZeroBased); sourceLineText = nthSourceLineForArrowHeader(provider->source(), physicalLine); if (!sourceLineText.isNull()) { caretColumn = line_and_column.column; - unsigned startColumnZeroBased = static_cast(provider->startPosition().m_column.zeroBasedInt()); + unsigned startColumnZeroBased = static_cast(std::max(start.m_column.zeroBasedInt(), 0)); if (physicalLine == 1 && caretColumn > startColumnZeroBased) caretColumn -= startColumnZeroBased; } } } - writeArrowHeaderStack(vm, errorInstance, source_url, static_cast(line_and_column.line), sourceLineText, caretColumn, stack); + writeArrowHeaderStack(vm, errorInstance, source_url, reportedLine, sourceLineText, caretColumn, stack); JSC::throwException(globalObject, throwScope, exception.get()); return true; diff --git a/test/js/node/test/parallel/test-vm-basic.js b/test/js/node/test/parallel/test-vm-basic.js index be409b937246..9641fc3b6096 100644 --- a/test/js/node/test/parallel/test-vm-basic.js +++ b/test/js/node/test/parallel/test-vm-basic.js @@ -278,16 +278,15 @@ const vm = require('vm'); Error.stackTraceLimit = 1; // Bun's compileFunction stack frames differ from Node's: JSC attributes - // the throw to a different column (and columnOffset is not applied), the - // wrapper costs one line when lineOffset is 0, and the anonymous source is - // labeled differently. + // the throw to a different column (and columnOffset is not applied), and + // the anonymous source is labeled differently. assert.throws(() => { vm.compileFunction('throw new Error("Sample Error")')(); }, { message: 'Sample Error', stack: typeof Bun === 'undefined' ? 'Error: Sample Error\n at :1:7' - : 'Error: Sample Error\n at (file:///:2:16)' + : 'Error: Sample Error\n at (file:///:1:16)' }); assert.throws(() => { @@ -313,7 +312,7 @@ const vm = require('vm'); message: 'Sample Error', stack: typeof Bun === 'undefined' ? 'Error: Sample Error\n at :1:10' - : 'Error: Sample Error\n at (file:///:2:16)' + : 'Error: Sample Error\n at (file:///:1:16)' }); assert.strictEqual( @@ -337,7 +336,7 @@ const vm = require('vm'); // Bun's compileFunction stack frames differ from Node's (see above). stack: typeof Bun === 'undefined' ? 'ReferenceError: varInContext is not defined\n at :1:1' - : 'ReferenceError: varInContext is not defined\n at (file:///:2:20)' + : 'ReferenceError: varInContext is not defined\n at (file:///:1:20)' }); assert.notDeepStrictEqual( diff --git a/test/js/node/vm/vm.test.ts b/test/js/node/vm/vm.test.ts index c82b29164fcf..a4d35d4b46ee 100644 --- a/test/js/node/vm/vm.test.ts +++ b/test/js/node/vm/vm.test.ts @@ -463,6 +463,181 @@ describe("Script", () => { }); }); +// Position of the first `::` frame in a stack. The header +// node:vm prepends to thrown errors has no column, so it never matches. +function frameAt(stack: string, filename: string) { + const match = stack.match(new RegExp(`${filename.replaceAll(".", "\\.")}:(-?\\d+):(\\d+)`)); + expect(match).not.toBeNull(); + return { line: Number(match![1]), column: Number(match![2]) }; +} + +// lineOffset / columnOffset are signed in Node: a negative offset is how a caller +// compensates for wrapper text it prepended to the source, and V8 reports the +// resulting positions as-is. JSC clamps the position a source starts at to 1:1, +// so the negative part has to be re-applied by Bun, both in the arrow header +// node:vm prepends to a thrown error's stack and in every rendering of the frames. +describe("negative lineOffset and columnOffset", () => { + const fourLines = "1;\n2;\n3;\nthrow new Error('x')"; + + // The header node:vm prepends to a thrown error (`:`, source line, + // caret line, blank line) and the rest of the stack. + function thrown(run: () => unknown) { + let stack: string | undefined; + try { + run(); + } catch (e: any) { + stack = e.stack; + } + expect(typeof stack).toBe("string"); + const lines = stack!.split("\n"); + return { header: lines.slice(0, 4), rest: lines.slice(4).join("\n") }; + } + + const runners: Record unknown> = { + "Script#runInThisContext": (code, options) => new Script(code, options).runInThisContext(), + "Script#runInNewContext": (code, options) => new Script(code, options).runInNewContext({}), + "vm.runInThisContext": (code, options) => runInThisContext(code, options), + }; + for (const [name, run] of Object.entries(runners)) { + test(`${name}: header line and frames carry a negative lineOffset`, () => { + const base = thrown(() => run(fourLines, { filename: "n.js" })); + expect(base.header).toEqual(["n.js:4", "throw new Error('x')", expect.stringMatching(/^ *\^$/), ""]); + expect(frameAt(base.rest, "n.js").line).toBe(4); + + const offset = thrown(() => run(fourLines, { filename: "n.js", lineOffset: -2 })); + expect(offset.header).toEqual(["n.js:2", "throw new Error('x')", base.header[2], ""]); + expect(frameAt(offset.rest, "n.js")).toEqual({ ...frameAt(base.rest, "n.js"), line: 2 }); + }); + } + + test("the source line shown is the physical one, not the one at line + |offset|", () => { + // Before the offset was taken into account here the header showed the line + // *after* the throw (with the caret under it) whenever it was long enough. + const code = "1;\nthrow new Error('x');\nconsole.log('a line long enough for the caret to fit under it');"; + const { header } = thrown(() => new Script(code, { filename: "w.js", lineOffset: -1 }).runInThisContext()); + expect(header).toEqual(["w.js:1", "throw new Error('x');", expect.stringMatching(/^ *\^$/), ""]); + }); + + test("frames inside functions defined by the script are offset too", () => { + const code = "function f() {\n throw new Error('x');\n}\nf();"; + const { header, rest } = thrown(() => new Script(code, { filename: "nf.js", lineOffset: -1 }).runInThisContext()); + expect(header).toEqual(["nf.js:1", " throw new Error('x');", expect.stringMatching(/^ *\^$/), ""]); + expect(rest).toMatch(/^ at f \(nf\.js:1:\d+\)$/m); + expect(rest).toMatch(/^ at nf\.js:3:\d+$/m); + }); + + test("columnOffset shifts the first line's frame columns either way; the caret stays on the text", () => { + const code = " throw new Error('x')"; + const run = (options: object) => + thrown(() => new Script(code, { filename: "c.js", ...options }).runInThisContext()); + const base = run({}); + expect(base.header).toEqual(["c.js:1", code, expect.stringMatching(/^ *\^$/), ""]); + const baseFrame = frameAt(base.rest, "c.js"); + + for (const columnOffset of [3, -3]) { + const offset = run({ columnOffset }); + // Same caret line: the caret marks a column of the text, which did not move. + expect(offset.header).toEqual(base.header); + expect(frameAt(offset.rest, "c.js")).toEqual({ line: 1, column: baseFrame.column + columnOffset }); + } + }); + + test("header lines at or below zero are reported like Node does", () => { + const far = thrown(() => new Script(fourLines, { filename: "nn.js", lineOffset: -10 }).runInThisContext()); + expect(far.header).toEqual(["nn.js:-6", "throw new Error('x')", expect.stringMatching(/^ *\^$/), ""]); + + const zero = thrown(() => + new Script(" throw new Error('x')", { filename: "z.js", lineOffset: -1, columnOffset: -1 }).runInThisContext(), + ); + expect(zero.header).toEqual(["z.js:0", " throw new Error('x')", expect.stringMatching(/^ *\^$/), ""]); + }); + + const makesError = "'line 1';\nfunction f() {\n return new Error('x');\n}\nf();"; + + test("Error.prepareStackTrace call sites see the offsets", () => { + // Positions of the call sites in `filename` as [line, column] pairs. + const callSites = (code: string, options: { filename: string; lineOffset?: number; columnOffset?: number }) => { + const previous = Error.prepareStackTrace; + Error.prepareStackTrace = (_, sites) => + sites.filter(s => s.getFileName() === options.filename).map(s => [s.getLineNumber(), s.getColumnNumber()]); + try { + return new Script(code, options).runInThisContext().stack as [number, number][]; + } finally { + Error.prepareStackTrace = previous; + } + }; + + const base = callSites(makesError, { filename: "cs.js" }); + expect(base.map(([line]) => line)).toEqual([3, 5]); + const lines = callSites(makesError, { filename: "cs.js", lineOffset: -1 }); + expect(lines).toEqual([ + [2, base[0][1]], + [4, base[1][1]], + ]); + + const code = " new Error('x')"; + const [[, baseColumn]] = callSites(code, { filename: "cc.js" }); + expect(callSites(code, { filename: "cc.js", columnOffset: -3 })).toEqual([[1, baseColumn - 3]]); + }); + + test("Bun.inspect() prints the offset line numbers", () => { + const error = new Script(makesError, { filename: "insp.js", lineOffset: -1 }).runInThisContext(); + const inspected = Bun.inspect(error); + expect(inspected).toMatch(/^2 \| +return new Error\('x'\);$/m); + expect(inspected).toMatch(/at f \(insp\.js:2:\d+\)$/m); + expect(inspected).toMatch(/at insp\.js:4:\d+$/m); + }); + + test("compileFunction reports body line N as N + lineOffset for every sign of the offset", () => { + const body = "1;\nthrow new Error('x')"; + const lineFor = (lineOffset: number) => { + let stack: string | undefined; + try { + compileFunction(body, [], { filename: "cf.js", lineOffset })(); + } catch (e: any) { + stack = e.stack; + } + return frameAt(stack!, "cf.js").line; + }; + expect([-1, 0, 3].map(lineFor)).toEqual([1, 2, 5]); + }); + + test.concurrent("uncaught errors are printed with the offset positions", async () => { + const run = async (code: string, options: object) => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + `require("node:vm").runInThisContext(${JSON.stringify(code)}, ${JSON.stringify(options)})`, + ], + env: bunEnv, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stdout).toBe(""); + return { stderr, exitCode }; + }; + + // displayErrors: false leaves err.stack alone, so the error printer shows its + // own source excerpt and frames instead of the header checked above. + const code = "1;\n2;\nfunction f() {\n throw new Error('x');\n}\nf();"; + const [excerpt, belowZero] = await Promise.all([ + run(code, { filename: "n.js", lineOffset: -2, displayErrors: false }), + run(fourLines, { filename: "nn.js", lineOffset: -10 }), + ]); + + expect(excerpt.stderr).toMatch(/^2 \| +throw new Error\('x'\);$/m); + expect(excerpt.stderr).toMatch(/at f \(n\.js:2:\d+\)$/m); + expect(excerpt.stderr).toMatch(/at n\.js:4:\d+$/m); + expect(excerpt.exitCode).toBe(1); + + // Offsets that push the lines below 1 still print the error and exit normally. + expect(belowZero.stderr).toContain("error: x"); + expect(belowZero.exitCode).toBe(1); + }); +}); + type TestRunInContextArg = | { fn: typeof runInContext; isIsolated: true; isNew?: boolean } | { fn: typeof runInThisContext; isIsolated?: false; isNew?: boolean }; @@ -671,11 +846,38 @@ function testRunInContext({ fn, isIsolated, isNew }: TestRunInContextArg) { test.todo("can specify filename", () => { // }); - test.todo("can specify lineOffset", () => { - // + // Runs `code`, which evaluates to a stack string, through whichever entry + // point this suite exercises and returns the position of its frame in `file`. + const positionOf = (code: string, offsets: { lineOffset?: number; columnOffset?: number } = {}) => { + const options = { filename: "offsets.js", ...offsets }; + const stack: string = isIsolated + ? (fn as typeof runInContext)(code, createContext({}), options) + : (fn as typeof runInThisContext)(code, options); + return frameAt(stack, options.filename); + }; + + test("can specify lineOffset", () => { + // `new Error()` sits on physical line 4; lineOffset is added to it in + // either direction (a negative offset is how callers compensate for + // wrapper lines they prepended to the source). + const code = "\n\n\n new Error().stack;"; + expect(positionOf(code).line).toBe(4); + expect(positionOf(code, { lineOffset: 10 }).line).toBe(14); + expect(positionOf(code, { lineOffset: -2 }).line).toBe(2); }); - test.todo("can specify columnOffset", () => { - // + test("can specify columnOffset", () => { + // columnOffset applies to the first physical line of the source only, in + // either direction. The column JSC attributes to the expression is taken + // from an un-offset run so only the offset arithmetic is asserted here. + const code = " new Error().stack;"; + const base = positionOf(code); + expect(base.line).toBe(1); + expect(positionOf(code, { columnOffset: 10 })).toEqual({ line: 1, column: base.column + 10 }); + expect(positionOf(code, { columnOffset: -2 })).toEqual({ line: 1, column: base.column - 2 }); + // Still the first physical line when lineOffset moves its number. + expect(positionOf(code, { lineOffset: 5, columnOffset: -2 })).toEqual({ line: 6, column: base.column - 2 }); + // Not the first physical line: the column is untouched. + expect(positionOf("\n" + code, { lineOffset: -1, columnOffset: -2 })).toEqual({ line: 1, column: base.column }); }); test.todo("can specify timeout", () => { // From 35feb7156eb217df281369e485eb2c166d277338 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 13 Aug 2026 22:09:08 +0000 Subject: [PATCH 2/5] Apply the negative source start after the construct position adjustment adjustPositionBackwards walks the position back to the `new` keyword in the coordinates JSC reported and recounts the column from the source when it crosses a line, so the offset has to be added once it is done: added first, a negative column offset could trigger that recount (and be dropped by it), and a position walked back onto the first line missed the column offset that applies there. --- src/jsc/bindings/ErrorStackFrame.cpp | 4 +++- test/js/node/vm/vm.test.ts | 8 ++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/src/jsc/bindings/ErrorStackFrame.cpp b/src/jsc/bindings/ErrorStackFrame.cpp index 3edcac3dd9c2..412718974ff7 100644 --- a/src/jsc/bindings/ErrorStackFrame.cpp +++ b/src/jsc/bindings/ErrorStackFrame.cpp @@ -91,7 +91,6 @@ ZigStackFramePosition getAdjustedPositionForBytecode(JSC::CodeBlock* code, JSC:: .column_zero_based = OrdinalNumber::fromOneBasedInt(expr.lineColumn.column).zeroBasedInt(), .byte_position = (int)expr.divot, }; - applyNegativeSourceStart(code, pos.line_zero_based, pos.column_zero_based); auto inst = code->instructionAt(bc); @@ -116,6 +115,9 @@ ZigStackFramePosition getAdjustedPositionForBytecode(JSC::CodeBlock* code, JSC:: break; } + // Last, so that the walk above runs on the coordinates JSC reported (it recounts the column + // from the source when it crosses a line) and the first-line test sees the line it ended on. + applyNegativeSourceStart(code, pos.line_zero_based, pos.column_zero_based); return pos; } diff --git a/test/js/node/vm/vm.test.ts b/test/js/node/vm/vm.test.ts index a4d35d4b46ee..7c2ea3cd010d 100644 --- a/test/js/node/vm/vm.test.ts +++ b/test/js/node/vm/vm.test.ts @@ -578,6 +578,14 @@ describe("negative lineOffset and columnOffset", () => { const code = " new Error('x')"; const [[, baseColumn]] = callSites(code, { filename: "cc.js" }); expect(callSites(code, { filename: "cc.js", columnOffset: -3 })).toEqual([[1, baseColumn - 3]]); + + // Call sites of a `new` expression are reported at the `new` keyword. JSC's + // own position for this one is on line 2, so the column offset has to be + // applied after the position has been moved back onto the first line. + const wrapped = " new\nError('x')"; + const [[wrappedLine, wrappedColumn]] = callSites(wrapped, { filename: "nl.js" }); + expect(wrappedLine).toBe(1); + expect(callSites(wrapped, { filename: "nl.js", columnOffset: -3 })).toEqual([[1, wrappedColumn - 3]]); }); test("Bun.inspect() prints the offset line numbers", () => { From 1c41504709b8e8c3e518d090d43984159f181d51 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 13 Aug 2026 22:14:37 +0000 Subject: [PATCH 3/5] Shorten the comments around the source start adjustment --- src/jsc/bindings/ErrorStackFrame.cpp | 9 +++------ src/jsc/bindings/ErrorStackFrame.h | 11 ++++------- src/jsc/bindings/NodeVM.cpp | 16 ++++------------ 3 files changed, 11 insertions(+), 25 deletions(-) diff --git a/src/jsc/bindings/ErrorStackFrame.cpp b/src/jsc/bindings/ErrorStackFrame.cpp index 412718974ff7..746a2fdfb363 100644 --- a/src/jsc/bindings/ErrorStackFrame.cpp +++ b/src/jsc/bindings/ErrorStackFrame.cpp @@ -14,14 +14,12 @@ void applyNegativeSourceStart(CodeBlock* code, int& lineZeroBased, int& columnZe if (!provider) return; - // Function code blocks share the provider of the program they were parsed from, so this is - // the whole source's start even for a frame inside a function. + // Nested functions share the program's provider, so this is the whole source's start. TextPosition start = provider->startPosition(); int startLine = start.m_line.zeroBasedInt(); int startColumn = start.m_column.zeroBasedInt(); - // JSC only adds the start column to positions on the first physical line, which it reports - // as the clamped start line. + // JSC applies the start column to the first physical line only, reported as the clamped start line. if (startColumn < 0 && lineZeroBased == std::max(startLine, 0)) columnZeroBased += startColumn; if (startLine < 0) @@ -115,8 +113,7 @@ ZigStackFramePosition getAdjustedPositionForBytecode(JSC::CodeBlock* code, JSC:: break; } - // Last, so that the walk above runs on the coordinates JSC reported (it recounts the column - // from the source when it crosses a line) and the first-line test sees the line it ended on. + // After the walk: it works in JSC's coordinates and may end on a different line. applyNegativeSourceStart(code, pos.line_zero_based, pos.column_zero_based); return pos; } diff --git a/src/jsc/bindings/ErrorStackFrame.h b/src/jsc/bindings/ErrorStackFrame.h index e2c9e0d26bc6..790be4cbcdfc 100644 --- a/src/jsc/bindings/ErrorStackFrame.h +++ b/src/jsc/bindings/ErrorStackFrame.h @@ -13,15 +13,12 @@ namespace Bun { ZigStackFramePosition getAdjustedPositionForBytecode(JSC::CodeBlock* code, JSC::BytecodeIndex bc); -/// JSC::SourceCode counts from its start position clamped to line 1, column 1, so a source -/// created with a negative start (node:vm lineOffset / columnOffset below 0) reports physical -/// positions while its SourceProvider still carries the requested start. This adds back the part -/// JSC dropped, which is the position V8 reports for the same options. No-op for sources that -/// start at or after 1:1, i.e. everything that is not node:vm code. +/// Adds the negative part of the provider's start position (node:vm lineOffset / columnOffset < 0) +/// to a position reported by JSC, which counts from the start clamped to 1:1 (JSC::SourceCode). +/// No-op for every other source. void applyNegativeSourceStart(JSC::CodeBlock* code, int& lineZeroBased, int& columnZeroBased); -/// StackFrame::computeLineAndColumn() with applyNegativeSourceStart applied; byte_position is -1. -/// The frame must have a code block (StackFrame::hasLineAndColumnInfo()). +/// StackFrame::computeLineAndColumn() of a frame with a code block, with the above applied. ZigStackFramePosition getLineColumnForStackFrame(const JSC::StackFrame& frame); } // namespace Bun diff --git a/src/jsc/bindings/NodeVM.cpp b/src/jsc/bindings/NodeVM.cpp index 3d82809d41e8..dba5a0068d57 100644 --- a/src/jsc/bindings/NodeVM.cpp +++ b/src/jsc/bindings/NodeVM.cpp @@ -187,12 +187,8 @@ JSC::JSFunction* constructAnonymousFunction(JSC::JSGlobalObject* globalObject, c String code = stringifyAnonymousFunction(globalObject, args, throwScope, &startOffset); EXCEPTION_ASSERT(!!throwScope.exception() == code.isNull()); - // The user's body starts on line 2 of the wrapped program (after the - // "(function () {\n" prefix). Start the program one line above the - // requested offset so body line N reports as N + lineOffset, the way V8's - // CompileFunction does. For lineOffset <= 0 this start is negative; JSC - // counts from line 1 regardless and the stack formatters add the negative - // part back (Bun::applyNegativeSourceStart). + // The body is line 2 of the wrapped program, so start the program one line + // early; a negative start is honored by Bun::applyNegativeSourceStart. int startLine = position.m_line.zeroBasedInt(); if (startLine != std::numeric_limits::min()) startLine--; @@ -576,12 +572,8 @@ bool handleException(JSGlobalObject* globalObject, VM& vm, NakedPtrsource().provider()) { - // JSC::SourceCode clamps the start position it counts from to 1:1, - // so line_and_column only carries the non-negative part of - // lineOffset/columnOffset: undo that part to index the source - // text, then report the physical line with the whole offset, as - // Node does (and as Bun::applyNegativeSourceStart does for the - // frames below the header). + // line_and_column includes the start position clamped to 1:1 (see + // Bun::applyNegativeSourceStart); the header shows the full offset. TextPosition start = provider->startPosition(); int startLineZeroBased = start.m_line.zeroBasedInt(); int64_t physicalLine = static_cast(line_and_column.line) - std::max(startLineZeroBased, 0); From d4ab733f67cb69e75e2543139bf1b08cf6794800 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Thu, 13 Aug 2026 22:17:07 +0000 Subject: [PATCH 4/5] Collapse the remaining multi-line comments to one line --- src/jsc/bindings/ErrorStackFrame.h | 4 +--- src/jsc/bindings/NodeVM.cpp | 6 ++---- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/src/jsc/bindings/ErrorStackFrame.h b/src/jsc/bindings/ErrorStackFrame.h index 790be4cbcdfc..74ce7582915a 100644 --- a/src/jsc/bindings/ErrorStackFrame.h +++ b/src/jsc/bindings/ErrorStackFrame.h @@ -13,9 +13,7 @@ namespace Bun { ZigStackFramePosition getAdjustedPositionForBytecode(JSC::CodeBlock* code, JSC::BytecodeIndex bc); -/// Adds the negative part of the provider's start position (node:vm lineOffset / columnOffset < 0) -/// to a position reported by JSC, which counts from the start clamped to 1:1 (JSC::SourceCode). -/// No-op for every other source. +/// Re-adds the part of a negative node:vm lineOffset / columnOffset that JSC::SourceCode clamps away; no-op for other sources. void applyNegativeSourceStart(JSC::CodeBlock* code, int& lineZeroBased, int& columnZeroBased); /// StackFrame::computeLineAndColumn() of a frame with a code block, with the above applied. diff --git a/src/jsc/bindings/NodeVM.cpp b/src/jsc/bindings/NodeVM.cpp index dba5a0068d57..bb010ebc897e 100644 --- a/src/jsc/bindings/NodeVM.cpp +++ b/src/jsc/bindings/NodeVM.cpp @@ -187,8 +187,7 @@ JSC::JSFunction* constructAnonymousFunction(JSC::JSGlobalObject* globalObject, c String code = stringifyAnonymousFunction(globalObject, args, throwScope, &startOffset); EXCEPTION_ASSERT(!!throwScope.exception() == code.isNull()); - // The body is line 2 of the wrapped program, so start the program one line - // early; a negative start is honored by Bun::applyNegativeSourceStart. + // The body is line 2 of the wrapped program; a negative start is honored by Bun::applyNegativeSourceStart. int startLine = position.m_line.zeroBasedInt(); if (startLine != std::numeric_limits::min()) startLine--; @@ -572,8 +571,7 @@ bool handleException(JSGlobalObject* globalObject, VM& vm, NakedPtrsource().provider()) { - // line_and_column includes the start position clamped to 1:1 (see - // Bun::applyNegativeSourceStart); the header shows the full offset. + // line_and_column was counted from the start clamped to 1:1 (see Bun::applyNegativeSourceStart). TextPosition start = provider->startPosition(); int startLineZeroBased = start.m_line.zeroBasedInt(); int64_t physicalLine = static_cast(line_and_column.line) - std::max(startLineZeroBased, 0); From 943e04b1565f2c78c2314d5ba719376475917eb9 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Fri, 14 Aug 2026 02:29:19 +0000 Subject: [PATCH 5/5] Render positions at or below line 1 without a position instead of garbage Offsets larger than the line now produce frames whose line is 0 or negative. error.stack prints them as V8 does; the renderers that cannot: - the error printer's stack-string parser read line numbers as unsigned, so "at f (x.js:-6:9)" came back as url "x.js:-6" with the column as its line; it now parses them signed, and such lines count as no position - the printer wrote the ":" separator when only the column was valid, leaving "at f (x.js:)" - error.line / error.column are unsigned in ErrorInstance and wrapped around; they now read as 0 populateStackFramePosition's no-bytecode-index fallback goes through getLineColumnForStackFrame like formatStackTrace does. --- src/jsc/ZigStackFrame.rs | 2 +- src/jsc/bindings/FormatStackTraceForJS.cpp | 14 +++++++--- src/jsc/bindings/ZigException.cpp | 17 +++++------- test/js/node/vm/vm.test.ts | 31 +++++++++++++++------- 4 files changed, 38 insertions(+), 26 deletions(-) diff --git a/src/jsc/ZigStackFrame.rs b/src/jsc/ZigStackFrame.rs index 4de668b7810d..3eb9adc4804b 100644 --- a/src/jsc/ZigStackFrame.rs +++ b/src/jsc/ZigStackFrame.rs @@ -173,7 +173,7 @@ impl<'a> fmt::Display for SourceURLFormatter<'a> { if self.line_column == LineColumn::Include && !source_slice.is_empty() - && (self.position.line.is_valid() || self.position.column.is_valid()) + && self.position.line.is_valid() { if self.enable_color { f.write_str(Output::pretty_fmt!(":", true))?; diff --git a/src/jsc/bindings/FormatStackTraceForJS.cpp b/src/jsc/bindings/FormatStackTraceForJS.cpp index e481f622a675..f7a21826a70f 100644 --- a/src/jsc/bindings/FormatStackTraceForJS.cpp +++ b/src/jsc/bindings/FormatStackTraceForJS.cpp @@ -575,6 +575,12 @@ static JSValue computeErrorInfoToJSValue(JSC::VM& vm, Vector& stackT return computeErrorInfoToJSValueWithoutSkipping(vm, stackTrace, line, column, sourceURL, errorInstance, bunErrorData); } +// ErrorInstance keeps error.line / error.column unsigned; a node:vm offset can put a position at or below 0, which reads as unset (0) there. +static unsigned toUnsignedOneBased(OrdinalNumber ordinal) +{ + return static_cast(std::max(ordinal.oneBasedInt(), 0)); +} + WTF::String computeErrorInfoWrapperToString(JSC::VM& vm, Vector& stackTrace, unsigned int& line_in, unsigned int& column_in, String& sourceURL, void* bunErrorData) { UNUSED_PARAM(bunErrorData); @@ -607,8 +613,8 @@ WTF::String computeErrorInfoWrapperToString(JSC::VM& vm, Vector& sta result = WTF::emptyString(); } - line_in = line.oneBasedInt(); - column_in = column.oneBasedInt(); + line_in = toUnsignedOneBased(line); + column_in = toUnsignedOneBased(column); return result; } @@ -644,8 +650,8 @@ JSC::JSValue computeErrorInfoWrapperToJSValue(JSC::VM& vm, Vector& s JSValue result = computeErrorInfoToJSValue(vm, stackTrace, line, column, sourceURL, errorInstance, bunErrorData); - line_in = line.oneBasedInt(); - column_in = column.oneBasedInt(); + line_in = toUnsignedOneBased(line); + column_in = toUnsignedOneBased(column); // materializeErrorInfoIfNeeded putDirect()s this unconditionally; an empty JSValue // in property storage crashes the next read. https://github.com/oven-sh/bun/issues/34095 diff --git a/src/jsc/bindings/ZigException.cpp b/src/jsc/bindings/ZigException.cpp index ea84d276eda9..abad429c0549 100644 --- a/src/jsc/bindings/ZigException.cpp +++ b/src/jsc/bindings/ZigException.cpp @@ -145,13 +145,7 @@ static void populateStackFramePosition(const JSC::StackFrame& stackFrame, BunStr return; if (!stackFrame.hasBytecodeIndex()) { - if (stackFrame.hasLineAndColumnInfo()) { - auto lineColumn = stackFrame.computeLineAndColumn(); - position.line_zero_based = OrdinalNumber::fromOneBasedInt(lineColumn.line).zeroBasedInt(); - position.column_zero_based = OrdinalNumber::fromOneBasedInt(lineColumn.column).zeroBasedInt(); - } - - position.byte_position = -1; + position = Bun::getLineColumnForStackFrame(stackFrame); return; } @@ -333,7 +327,8 @@ class V8StackTraceIterator { auto segment1 = StringView_slice(lineInner, marker1, marker2); auto segment2 = StringView_slice(lineInner, marker2 + 1, marker3); - if (auto int1 = WTF::parseIntegerAllowingTrailingJunk(segment2)) { + // Signed: a node:vm lineOffset can push positions to 0 or below. + if (auto int1 = WTF::parseIntegerAllowingTrailingJunk(segment2)) { frame.sourceURL = segment1; frame.lineNumber = WTF::OrdinalNumber::fromOneBasedInt(int1.value()); } else { @@ -363,8 +358,8 @@ class V8StackTraceIterator { auto segment2 = StringView_slice(lineInner, marker2 + 1, marker3); auto segment3 = StringView_slice(lineInner, marker3 + 1, marker4); - if (auto int1 = WTF::parseIntegerAllowingTrailingJunk(segment2)) { - if (auto int2 = WTF::parseIntegerAllowingTrailingJunk(segment3)) { + if (auto int1 = WTF::parseIntegerAllowingTrailingJunk(segment2)) { + if (auto int2 = WTF::parseIntegerAllowingTrailingJunk(segment3)) { frame.sourceURL = segment1; frame.lineNumber = WTF::OrdinalNumber::fromOneBasedInt(int1.value()); frame.columnNumber = WTF::OrdinalNumber::fromOneBasedInt(int2.value()); @@ -373,7 +368,7 @@ class V8StackTraceIterator { frame.lineNumber = WTF::OrdinalNumber::fromOneBasedInt(int1.value()); } } else { - if (auto int2 = WTF::parseIntegerAllowingTrailingJunk(segment3)) { + if (auto int2 = WTF::parseIntegerAllowingTrailingJunk(segment3)) { frame.sourceURL = StringView_slice(lineInner, marker1, marker3); frame.lineNumber = WTF::OrdinalNumber::fromOneBasedInt(int2.value()); } else { diff --git a/test/js/node/vm/vm.test.ts b/test/js/node/vm/vm.test.ts index 7c2ea3cd010d..6f2a966c7868 100644 --- a/test/js/node/vm/vm.test.ts +++ b/test/js/node/vm/vm.test.ts @@ -482,15 +482,15 @@ describe("negative lineOffset and columnOffset", () => { // The header node:vm prepends to a thrown error (`:`, source line, // caret line, blank line) and the rest of the stack. function thrown(run: () => unknown) { - let stack: string | undefined; + let error: any; try { run(); - } catch (e: any) { - stack = e.stack; + } catch (e) { + error = e; } - expect(typeof stack).toBe("string"); - const lines = stack!.split("\n"); - return { header: lines.slice(0, 4), rest: lines.slice(4).join("\n") }; + expect(typeof error?.stack).toBe("string"); + const lines: string[] = error.stack.split("\n"); + return { header: lines.slice(0, 4), rest: lines.slice(4).join("\n"), error }; } const runners: Record unknown> = { @@ -507,6 +507,8 @@ describe("negative lineOffset and columnOffset", () => { const offset = thrown(() => run(fourLines, { filename: "n.js", lineOffset: -2 })); expect(offset.header).toEqual(["n.js:2", "throw new Error('x')", base.header[2], ""]); expect(frameAt(offset.rest, "n.js")).toEqual({ ...frameAt(base.rest, "n.js"), line: 2 }); + // JSC's own error.line property follows the top frame. + expect([base.error.line, offset.error.line]).toEqual([4, 2]); }); } @@ -550,6 +552,9 @@ describe("negative lineOffset and columnOffset", () => { new Script(" throw new Error('x')", { filename: "z.js", lineOffset: -1, columnOffset: -1 }).runInThisContext(), ); expect(zero.header).toEqual(["z.js:0", " throw new Error('x')", expect.stringMatching(/^ *\^$/), ""]); + + // error.line is unsigned in JSC; such positions read as unset instead of wrapping around. + expect([far.error.line, zero.error.line]).toEqual([0, 0]); }); const makesError = "'line 1';\nfunction f() {\n return new Error('x');\n}\nf();"; @@ -630,9 +635,10 @@ describe("negative lineOffset and columnOffset", () => { // displayErrors: false leaves err.stack alone, so the error printer shows its // own source excerpt and frames instead of the header checked above. const code = "1;\n2;\nfunction f() {\n throw new Error('x');\n}\nf();"; - const [excerpt, belowZero] = await Promise.all([ + const [excerpt, belowZero, belowZeroDecorated] = await Promise.all([ run(code, { filename: "n.js", lineOffset: -2, displayErrors: false }), - run(fourLines, { filename: "nn.js", lineOffset: -10 }), + run(code, { filename: "nn.js", lineOffset: -10, displayErrors: false }), + run(code, { filename: "nn.js", lineOffset: -10 }), ]); expect(excerpt.stderr).toMatch(/^2 \| +throw new Error\('x'\);$/m); @@ -640,9 +646,14 @@ describe("negative lineOffset and columnOffset", () => { expect(excerpt.stderr).toMatch(/at n\.js:4:\d+$/m); expect(excerpt.exitCode).toBe(1); - // Offsets that push the lines below 1 still print the error and exit normally. - expect(belowZero.stderr).toContain("error: x"); + // The printer cannot show a line at or below 0, so such frames are printed + // without a position, whether it reads the frames directly or re-parses + // the `at f (nn.js:-6:9)` lines of the stack node:vm already decorated. + expect(belowZero.stderr).toMatch(/^\s+at f \(nn\.js\)$/m); + expect(belowZero.stderr).toMatch(/^\s+at nn\.js$/m); expect(belowZero.exitCode).toBe(1); + expect(belowZeroDecorated.stderr).toMatch(/^\s+at f \(nn\.js\)$/m); + expect(belowZeroDecorated.exitCode).toBe(1); }); });