diff --git a/src/jsc/VirtualMachine.rs b/src/jsc/VirtualMachine.rs index ac42b0903872..8a1c49bfc3cc 100644 --- a/src/jsc/VirtualMachine.rs +++ b/src/jsc/VirtualMachine.rs @@ -5360,7 +5360,9 @@ impl VirtualMachine { } }; } - if has_name && !frame.position.is_invalid() { + // Frames parsed back out of error.stack can have a file without a + // position ("at foo (native)"); the formatter then prints the file alone. + if has_name && (!file.is_empty() || !frame.position.is_invalid()) { pretty_write!( " at {} ({})\n", frame.name_formatter(allow_ansi_colors), @@ -5630,13 +5632,11 @@ impl VirtualMachine { Some(bun_sourcemap::mapping::Lookup { mapping: bun_sourcemap::mapping::Mapping { generated: bun_sourcemap::LineColumnOffset::default(), + // Direct copy (both are `bun_core::Ordinal`) so that a frame + // without a position stays INVALID instead of becoming 1:1. original: bun_sourcemap::LineColumnOffset { - lines: bun_sourcemap::Ordinal::from_zero_based( - frames[top].position.line.zero_based().max(0), - ), - columns: bun_sourcemap::Ordinal::from_zero_based( - frames[top].position.column.zero_based().max(0), - ), + lines: frames[top].position.line, + columns: frames[top].position.column, }, source_index: 0, name_index: -1, @@ -5690,6 +5690,11 @@ impl VirtualMachine { // Avoid printing "export default 'native'" break 'code bun_core::ZigStringSlice::EMPTY; } + if !mapping.original.lines.is_valid() { + // No line to preview (a frame parsed out of error.stack that + // only names a file); line 1 would be an arbitrary choice. + break 'code bun_core::ZigStringSlice::EMPTY; + } let mut log = bun_ast::Log::default(); let Ok(original_source) = Self::fetch_without_on_load_plugins( self, @@ -6558,16 +6563,19 @@ impl VirtualMachine { let mut has_location = false; if let Some(frame) = top_frame { - if !frame.position.is_invalid() { + if frame.position.line.is_valid() { let source_url = frame.source_url.to_utf8(); let file = bun_paths::resolve_path::relative(dir, source_url.slice()); let _ = write!( writer, - "\n::error file={},line={},col={},title=", + "\n::error file={},line={},", bun_core::fmt::github_action_property(file), frame.position.line.one_based(), - frame.position.column.one_based(), ); + if frame.position.column.is_valid() { + let _ = write!(writer, "col={},", frame.position.column.one_based()); + } + let _ = writer.write_all(b"title="); has_location = true; } } diff --git a/src/jsc/bindings/ZigException.cpp b/src/jsc/bindings/ZigException.cpp index ea84d276eda9..430b095e4025 100644 --- a/src/jsc/bindings/ZigException.cpp +++ b/src/jsc/bindings/ZigException.cpp @@ -245,8 +245,11 @@ class V8StackTraceIterator { public: StringView functionName {}; StringView sourceURL {}; - WTF::OrdinalNumber lineNumber = WTF::OrdinalNumber::fromZeroBasedInt(0); - WTF::OrdinalNumber columnNumber = WTF::OrdinalNumber::fromZeroBasedInt(0); + // beforeFirst() (-1) is ZigStackFramePosition's "unknown", so a frame + // printed without a line or column ("at foo (native)") stays unknown + // instead of becoming line 1, column 1. + WTF::OrdinalNumber lineNumber = WTF::OrdinalNumber::beforeFirst(); + WTF::OrdinalNumber columnNumber = WTF::OrdinalNumber::beforeFirst(); bool isConstructor = false; bool isGlobalCode = false; @@ -611,6 +614,9 @@ static void fromErrorInstance(ZigException& except, JSC::JSGlobalObject* global, current.source_url = Bun::toStringRef(sourceURL); current.position.line_zero_based = frame.lineNumber.zeroBasedInt(); current.position.column_zero_based = frame.columnNumber.zeroBasedInt(); + // `current = {}` zeroes byte_position; -1 keeps a frame without a + // line and column equal to ZigStackFramePosition::INVALID. + current.position.byte_position = -1; current.remapped = true; current.is_async = frame.isAsync; diff --git a/test/js/bun/util/inspect-error.test.js b/test/js/bun/util/inspect-error.test.js index 4d3f488dac81..6f29f1f2207e 100644 --- a/test/js/bun/util/inspect-error.test.js +++ b/test/js/bun/util/inspect-error.test.js @@ -1,4 +1,5 @@ import { describe, expect, jest, test } from "bun:test"; +import { bunEnv, bunExe, tempDir } from "harness"; test("error.cause", () => { const err = new Error("error 1"); @@ -9,21 +10,23 @@ test("error.cause", () => { .replaceAll(import.meta.dir.replaceAll("\\", "/"), "[dir]"), ).toMatchInlineSnapshot(` "1 | import { describe, expect, jest, test } from "bun:test"; -2 | -3 | test("error.cause", () => { -4 | const err = new Error("error 1"); -5 | const err2 = new Error("error 2", { cause: err }); +2 | import { bunEnv, bunExe, tempDir } from "harness"; +3 | +4 | test("error.cause", () => { +5 | const err = new Error("error 1"); +6 | const err2 = new Error("error 2", { cause: err }); ^ error: error 2 - at ([dir]/inspect-error.test.js:5:20) + at ([dir]/inspect-error.test.js:6:20) 1 | import { describe, expect, jest, test } from "bun:test"; -2 | -3 | test("error.cause", () => { -4 | const err = new Error("error 1"); +2 | import { bunEnv, bunExe, tempDir } from "harness"; +3 | +4 | test("error.cause", () => { +5 | const err = new Error("error 1"); ^ error: error 1 - at ([dir]/inspect-error.test.js:4:19) + at ([dir]/inspect-error.test.js:5:19) " `); }); @@ -35,15 +38,15 @@ test("Error", () => { .replaceAll("\\", "/") .replaceAll(import.meta.dir.replaceAll("\\", "/"), "[dir]"), ).toMatchInlineSnapshot(` -"27 | " -28 | \`); -29 | }); -30 | -31 | test("Error", () => { -32 | const err = new Error("my message"); +"30 | " +31 | \`); +32 | }); +33 | +34 | test("Error", () => { +35 | const err = new Error("my message"); ^ error: my message - at ([dir]/inspect-error.test.js:32:19) + at ([dir]/inspect-error.test.js:35:19) " `); }); @@ -71,22 +74,13 @@ note: "duplicateConstDecl" was originally declared here } }); -const normalizeError = str => { - // remove debug-only stack trace frames - // like "at require (:1:21)" - if (str.includes(" (:")) { - const splits = str.split("\n"); - for (let i = 0; i < splits.length; i++) { - if (splits[i].includes(" (:")) { - splits.splice(i, 1); - i--; - } - } - return splits.join("\n"); - } - - return str; -}; +const normalizeError = str => + // remove debug-only stack trace frames of bun's own builtins, which have a + // position but no file, like "at require (51:24)" + str + .split("\n") + .filter(line => !/^\s*at \S+ \(:?\d+:\d+\)$/.test(line)) + .join("\n"); test("Error inside minified file (no color) ", () => { try { @@ -111,7 +105,7 @@ test("Error inside minified file (no color) ", () => { error: error inside long minified file! at ([dir]/inspect-error-fixture.min.js:26:2850) at ([dir]/inspect-error-fixture.min.js:26:2890) - at ([dir]/inspect-error.test.js:92:7)" + at ([dir]/inspect-error.test.js:86:7)" `); } }); @@ -140,7 +134,7 @@ test("Error inside minified file (color) ", () => { error: error inside long minified file! at ([dir]/inspect-error-fixture.min.js:26:2850) at ([dir]/inspect-error-fixture.min.js:26:2890) - at ([dir]/inspect-error.test.js:120:7)" + at ([dir]/inspect-error.test.js:114:7)" `); } }); @@ -154,7 +148,7 @@ test("Inserted originalLine and originalColumn do not appear in node:util.inspec .replaceAll(import.meta.path.replaceAll("\\", "/"), "[file]"), ).toMatchInlineSnapshot(` "Error: my message - at ([file]:149:19)" + at ([file]:143:19)" `); }); @@ -188,3 +182,95 @@ test("error.stack throwing an error doesn't lead to a crash", () => { throw err; }).toThrow(); }); + +// Once error.stack has been read (or assigned), JSC no longer has the frames of +// the error and the printer parses the " at ..." lines of that string instead. +// A frame printed without a line and column (native code, "unknown") has no +// position to parse, and must not come back out as line 1, column 1. +describe("printing the frames parsed back out of error.stack without a position", () => { + const printedFrames = text => + text + .split("\n") + .map(line => line.trim()) + .filter(line => line.startsWith("at ")); + + const stackWithFrames = frames => ["Error: boom", ...frames.map(frame => ` at ${frame}`)].join("\n"); + + const inspectStack = frames => { + const err = new Error("boom"); + err.stack = stackWithFrames(frames); + return Bun.inspect(err); + }; + + test("frames without a position, or with only a line, print as error.stack has them", () => { + const frames = [ + "first (native)", + "second (node:child_process)", + "unknown", + "third (/fake/lib.js:3:4)", + "fourth (/fake/lib.js:7)", + ]; + expect(printedFrames(inspectStack(frames))).toEqual(frames.map(frame => `at ${frame}`)); + }); + + test("a native frame is the only frame, so the source preview is looked up for it", () => { + expect(printedFrames(inspectStack(["first (native)"]))).toEqual(["at first (native)"]); + }); + + const inspectedLines = frames => inspectStack(frames).trimEnd().split("\n"); + + test("no source preview for a frame that names a file but no line in it", () => { + using dir = tempDir("inspect-error-no-position", { "lib.js": "// line 1\n// line 2\n" }); + const file = `${dir}/lib.js`; + expect(inspectedLines([`first (${file})`])).toEqual(["error: boom", ` at first (${file})`]); + }); + + test("the source preview and its caret belong to the first frame that has a position", () => { + using dir = tempDir("inspect-error-no-position", { "lib.js": "// line 1\n// line 2\n" }); + const file = `${dir}/lib.js`; + expect(inspectedLines(["first (native)", `second (${file}:2:5)`])).toEqual([ + "1 | // line 1", + "2 | // line 2", + " ^", + "error: boom", + " at first (native)", + ` at second (${file}:2:5)`, + ]); + }); + + describe.concurrent("uncaught exception printout and GitHub Actions annotation", () => { + const throwUncaught = async frames => { + await using proc = Bun.spawn({ + cmd: [ + bunExe(), + "-e", + `const err = new Error("boom"); err.stack = ${JSON.stringify(stackWithFrames(frames))}; throw err;`, + ], + env: { ...bunEnv, GITHUB_ACTIONS: "true", GITHUB_WORKSPACE: "/fake" }, + stdout: "pipe", + stderr: "pipe", + }); + const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + expect(stdout).toBe(""); + expect(exitCode).toBe(1); + return { + frames: printedFrames(stderr), + annotation: stderr.split("\n").find(line => line.startsWith("::error ")), + }; + }; + + test("top frame without a position: the annotation has no file, line or column", async () => { + expect(await throwUncaught(["first (native)", "unknown", "second (/fake/lib.js:3:4)"])).toEqual({ + frames: ["at first (native)", "at unknown", "at second (/fake/lib.js:3:4)"], + annotation: expect.stringMatching(/^::error title=error: boom::/), + }); + }); + + test("top frame with only a line: the annotation has no column", async () => { + expect(await throwUncaught(["first (/fake/lib.js:7)", "second (native)"])).toEqual({ + frames: ["at first (/fake/lib.js:7)", "at second (native)"], + annotation: expect.stringMatching(/^::error file=(.*[\\/])?lib\.js,line=7,title=error: boom::/), + }); + }); + }); +}); diff --git a/test/regression/issue/23022-stack-trace-iterator.test.ts b/test/regression/issue/23022-stack-trace-iterator.test.ts index 7ff0803ae945..46a94dafc3ec 100644 --- a/test/regression/issue/23022-stack-trace-iterator.test.ts +++ b/test/regression/issue/23022-stack-trace-iterator.test.ts @@ -27,7 +27,8 @@ test("V8StackTraceIterator handles frames without parentheses (issue #23022)", a const stackFrames = err.stack?.split("\n").filter(line => line.trim().startsWith("at")); expect(stackFrames?.length).toBeGreaterThan(3); - // Ensure both "unknown" frames and regular frames are present - expect(inspected).toContain("at unknown"); + // Ensure both "unknown" frames and regular frames are present. The "unknown" + // frame has no position in error.stack, so the printout must not invent one. + expect(inspected).toMatch(/^\s+at unknown$/m); expect(inspected).toContain("at _write"); });