error printer: remap frames when the original source is unavailable, and not twice after error.stack

src/jsc/VirtualMachine.rs

@@ -5715,7 +5715,8 @@ impl VirtualMachine {
                     &mut log,
                     FetchFlags::PrintSource,
                 ) else {
-                    return;
+                    // Source is gone; the frames still get remapped below.
+                    break 'code bun_core::ZigStringSlice::EMPTY;
                 };
                 *must_reset_parser_arena_later = true;
                 // Note: the transpile path `clone_utf8`s the source for
@@ -5789,7 +5790,8 @@ impl VirtualMachine {
 
         if frames.len() > 1 {
             for i in 0..frames.len() {
-                if i == top || frames[i].position.is_invalid() {
+                // `remapped`: frames parsed back out of a formatted `error.stack`.
+                if i == top || frames[i].remapped || frames[i].position.is_invalid() {
                     continue;
                 }
                 let source_url = frames[i].source_url.to_utf8();

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 <anonymous> ([dir]/inspect-error.test.js:5:20)
+      at <anonymous> ([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 <anonymous> ([dir]/inspect-error.test.js:4:19)
+      at <anonymous> ([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 <anonymous> ([dir]/inspect-error.test.js:32:19)
+      at <anonymous> ([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 <anonymous> ([dir]/inspect-error-fixture.min.js:26:2850)
             at <anonymous> ([dir]/inspect-error-fixture.min.js:26:2890)
-            at <anonymous> ([dir]/inspect-error.test.js:92:7)"
+            at <anonymous> ([dir]/inspect-error.test.js:86:7)"
     `);
   }
 });
@@ -140,7 +134,7 @@ test("Error inside minified file (color) ", () => {
       error: error inside long minified file!
             at <anonymous> ([dir]/inspect-error-fixture.min.js:26:2850)
             at <anonymous> ([dir]/inspect-error-fixture.min.js:26:2890)
-            at <anonymous> ([dir]/inspect-error.test.js:120:7)"
+            at <anonymous> ([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 <anonymous> ([file]:149:19)"
+    at <anonymous> ([file]:143:19)"
 `);
 });
 
@@ -188,3 +182,148 @@ test("error.stack throwing an error doesn't lead to a crash", () => {
     throw err;
   }).toThrow();
 });
+
+describe("source map remapping of the printed stack", () => {
+  // The "at ..." lines that mention one of `files`, with the temp dir removed
+  // from the paths.
+  function frames(text, dir, files) {
+    const prefix = dir.replaceAll("\\", "/") + "/";
+    return text
+      .replaceAll("\\", "/")
+      .split("\n")
+      .map(line => line.trim())
+      .filter(line => line.startsWith("at ") && files.some(file => line.includes(file)))
+      .map(line => line.replaceAll(prefix, ""));
+  }
+
+  async function run(files) {
+    using dir = tempDir("inspect-error-sourcemap", files);
+    await using proc = Bun.spawn({
+      cmd: [bunExe(), "main.js"],
+      cwd: String(dir),
+      env: bunEnv,
+      stdout: "pipe",
+      stderr: "pipe",
+    });
+    const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+    return { dir: String(dir), out: JSON.parse(stdout), stderr, exitCode };
+  }
+
+  // A prebuilt file whose map names a source that is neither on disk nor in
+  // `sourcesContent` (the shape of a deployed `bun build --target=bun
+  // --sourcemap` artifact with the sources stripped). The original source can't
+  // be shown, but the frames still have to be remapped, exactly like
+  // error.stack is.
+  test.concurrent("external map whose original source is unavailable", async () => {
+    const main = [
+      "// @bun",
+      'function thrower() { throw new Error("HOSTILE"); }',
+      "const out = {};",
+      "try { thrower(); } catch (e) { out.inspect = Bun.inspect(e); }",
+      "try { thrower(); } catch (e) { out.stack = e.stack; }",
+      "console.log(JSON.stringify(out));",
+      "thrower();",
+    ];
+    // One segment at column 0 of each generated line below, so every position on
+    // a line maps to the same original position: line 1 -> orig.ts:11:5, line 3
+    // -> 21:5, line 4 -> 31:5, line 6 -> 41:5. (Column 5 rather than 1 because
+    // error.stack prints no column at all for column 1.)
+    const map = {
+      version: 3,
+      sources: ["orig.ts"],
+      sourcesContent: [null],
+      names: [],
+      mappings: ";AAUI;;AAUA;AAUA;;AAUA",
+    };
+    const { dir, out, stderr, exitCode } = await run({
+      "main.js": main.join("\n") + "\n//# sourceMappingURL=main.js.map\n",
+      "main.js.map": JSON.stringify(map),
+    });
+
+    const files = ["orig.ts", "main.js"];
+    expect({
+      stack: frames(out.stack, dir, files),
+      inspect: frames(out.inspect, dir, files),
+      uncaught: frames(stderr, dir, files),
+    }).toEqual({
+      stack: ["at thrower (orig.ts:11:5)", "at orig.ts:31:5"],
+      inspect: ["at thrower (orig.ts:11:5)", "at orig.ts:21:5"],
+      uncaught: ["at thrower (orig.ts:11:5)", "at orig.ts:41:5"],
+    });
+    expect(exitCode).toBe(1);
+  });
+
+  // Modules bun transpiled itself. `present.ts` stays on disk; `deleted.ts` is
+  // removed after it was loaded, so the code frame can no longer be read back.
+  // Reading error.stack first makes the printer start from the already
+  // remapped frames of that string instead of the raw JSC frames; those must
+  // not be remapped a second time (https://github.com/oven-sh/bun/issues/15859).
+  test.concurrent("transpiled modules: source deleted, and frames already remapped by error.stack", async () => {
+    const module = message =>
+      [
+        "type Padding1 = { a: number };",
+        "type Padding2 = { b: string };",
+        "type Padding3 = { c: boolean };",
+        "export function thrower(): never {",
+        `  throw new Error(${JSON.stringify(message)});`,
+        "}",
+        "export function caller(onError: (e: Error) => string): string {",
+        "  try {",
+        "    thrower();",
+        "  } catch (e) {",
+        "    return onError(e as Error);",
+        "  }",
+        "  return 'unreachable';",
+        "}",
+        "",
+      ].join("\n");
+    const { dir, out, stderr, exitCode } = await run({
+      "present.ts": module("present"),
+      "deleted.ts": module("deleted"),
+      "main.js": [
+        'import { unlinkSync } from "node:fs";',
+        'import { join } from "node:path";',
+        'import * as present from "./present.ts";',
+        'import * as deleted from "./deleted.ts";',
+        'unlinkSync(join(import.meta.dir, "deleted.ts"));',
+        "const out = {",
+        "  presentStack: present.caller(e => e.stack),",
+        "  presentInspect: present.caller(e => Bun.inspect(e)),",
+        "  presentStackThenInspect: present.caller(e => (e.stack, Bun.inspect(e))),",
+        "  deletedStack: deleted.caller(e => e.stack),",
+        "  deletedInspect: deleted.caller(e => Bun.inspect(e)),",
+        "  deletedStackThenInspect: deleted.caller(e => (e.stack, Bun.inspect(e))),",
+        "};",
+        "console.log(JSON.stringify(out));",
+        "present.caller(e => { e.stack; throw e; });",
+        "",
+      ].join("\n"),
+    });
+
+    const files = ["present.ts", "deleted.ts"];
+    const positions = text => frames(text, dir, files);
+    // The throw is on line 5 and the call to thrower() on line 9 of the
+    // original module; after type stripping they are on lines 2 and 6.
+    const expected = file => [
+      expect.stringMatching(new RegExp(`^at thrower \\(${file}:5:\\d+\\)$`)),
+      expect.stringMatching(new RegExp(`^at caller \\(${file}:9:\\d+\\)$`)),
+    ];
+    expect(positions(out.presentStack)).toEqual(expected("present.ts"));
+    expect(positions(out.deletedStack)).toEqual(expected("deleted.ts"));
+
+    expect({
+      presentInspect: positions(out.presentInspect),
+      presentStackThenInspect: positions(out.presentStackThenInspect),
+      deletedInspect: positions(out.deletedInspect),
+      deletedStackThenInspect: positions(out.deletedStackThenInspect),
+      uncaughtAfterStack: positions(stderr),
+    }).toEqual({
+      presentInspect: positions(out.presentStack),
+      presentStackThenInspect: positions(out.presentStack),
+      deletedInspect: positions(out.deletedStack),
+      deletedStackThenInspect: positions(out.deletedStack),
+      uncaughtAfterStack: positions(out.presentStack),
+    });
+    expect(exitCode).toBe(1);
+  });
+});