transpiler: stop minifying new Error() (and other known-global constructs) into calls that JSC tail-calls

src/ast/known_global.rs

@@ -103,17 +103,17 @@ impl KnownGlobal {
         let constructor = lookup(original_name)?;
 
         match constructor {
-            // Error constructors can be called without 'new' with identical behavior
             KnownGlobal::Error
             | KnownGlobal::TypeError
             | KnownGlobal::SyntaxError
             | KnownGlobal::RangeError
             | KnownGlobal::ReferenceError
             | KnownGlobal::EvalError
             | KnownGlobal::URIError
-            | KnownGlobal::AggregateError => {
-                // Convert `new Error(...)` to `Error(...)` to save bytes
-                Some(Self::call_from_new(e, loc))
+            | KnownGlobal::AggregateError
+            | KnownGlobal::Function => {
+                // Kept: these read the calling frame, which a strict-mode `return Error(...)` tail call has already popped.
+                None
             }
 
             KnownGlobal::Object => {
@@ -255,10 +255,6 @@ impl KnownGlobal {
                 }
             }
 
-            KnownGlobal::Function => {
-                // Just remove 'new' for Function
-                Some(Self::call_from_new(e, loc))
-            }
             KnownGlobal::RegExp => {
                 // Don't optimize RegExp - the semantics are too complex:
                 // - new RegExp(re) creates a copy, but RegExp(re) returns the same instance

test/bundler/bundler_minify.test.ts

@@ -819,7 +819,10 @@ describe("bundler", () => {
     },
   });
 
-  itBundled("minify/ErrorConstructorOptimization", {
+  // `Error(...)` creates the same object as `new Error(...)`, but in strict mode JSC compiles
+  // `return Error(...)` to a tail call, which removes the creating function from the stack trace.
+  // `new` is never a tail call, so it has to stay.
+  itBundled("minify/ErrorConstructorKeepsNew", {
     files: {
       "/entry.js": /* js */ `
         // Test all Error constructors
@@ -862,25 +865,25 @@ describe("bundler", () => {
       `,
     },
     capture: [
-      "Error()",
-      'Error("message")',
-      'Error("message", { cause: "cause" })',
-      "TypeError()",
-      'TypeError("type error")',
-      "SyntaxError()",
-      'SyntaxError("syntax error")',
-      "RangeError()",
-      'RangeError("range error")',
-      "ReferenceError()",
-      'ReferenceError("ref error")',
-      "EvalError()",
-      'EvalError("eval error")',
-      "URIError()",
-      'URIError("uri error")',
-      'AggregateError([], "aggregate error")',
-      'AggregateError([Error("e1")], "multiple")',
-      "Error(msg)",
-      "TypeError(getErrorMessage())",
+      "new Error",
+      'new Error("message")',
+      'new Error("message", { cause: "cause" })',
+      "new TypeError",
+      'new TypeError("type error")',
+      "new SyntaxError",
+      'new SyntaxError("syntax error")',
+      "new RangeError",
+      'new RangeError("range error")',
+      "new ReferenceError",
+      'new ReferenceError("ref error")',
+      "new EvalError",
+      'new EvalError("eval error")',
+      "new URIError",
+      'new URIError("uri error")',
+      'new AggregateError([], "aggregate error")',
+      'new AggregateError([new Error("e1")], "multiple")',
+      "new Error(msg)",
+      "new TypeError(getErrorMessage())",
       "/* @__PURE__ */ new Date",
       "/* @__PURE__ */ new Map",
       "/* @__PURE__ */ new Set",
@@ -889,6 +892,27 @@ describe("bundler", () => {
     target: "bun",
   });
 
+  itBundled("minify/ErrorReturnedFromFunctionKeepsItsFrame", {
+    files: {
+      "/entry.js": /* js */ `
+        function makeError() {
+          return new Error("made");
+        }
+        function makeTypeError() {
+          const err = new TypeError("made");
+          return err;
+        }
+        const frames = [makeError, makeTypeError].map(make => make().stack.includes("at " + make.name + " "));
+        console.log(JSON.stringify(frames));
+      `,
+    },
+    minifySyntax: true,
+    target: "bun",
+    run: {
+      stdout: "[true,true]",
+    },
+  });
+
   itBundled("minify/ErrorConstructorWithVariables", {
     files: {
       "/entry.js": /* js */ `
@@ -1046,8 +1070,9 @@ describe("bundler", () => {
       "{}", // new Object() -> {}
       "{}", // new Object(null) -> {}
       "{ a: 1 }", // new Object({ a: 1 }) -> { a: 1 }
-      'Function("return 42")',
-      'Function("a", "b", "return a + b")',
+      // kept for the same reason as the Error constructors: the body's source origin comes from the calling frame
+      'new Function("return 42")',
+      'new Function("a", "b", "return a + b")',
       'new RegExp("test")',
       'new RegExp("test", "gi")',
       "new RegExp(/abc/)",

test/bundler/bundler_npm.test.ts

@@ -57,17 +57,17 @@ describe("bundler", () => {
           "../entry.tsx",
         ],
         mappings: [
-          ["react.development.js:524:'getContextName'", "1:5623:at"],
+          ["react.development.js:524:'getContextName'", "1:5629:at"],
           ["react.development.js:2495:'actScopeDepth'", "23:4082:or++"],
-          ["react.development.js:696:''Component'", '1:7685:\'Component "%s"'],
-          ["entry.tsx:6:'\"Content-Type\"'", '100:18808:"Content-Type"'],
-          ["entry.tsx:11:'<html>'", "100:19062:void"],
-          ["entry.tsx:23:'await'", "100:19161:await"],
+          ["react.development.js:696:''Component'", '1:7691:\'Component "%s"'],
+          ["entry.tsx:6:'\"Content-Type\"'", '100:18848:"Content-Type"'],
+          ["entry.tsx:11:'<html>'", "100:19102:void"],
+          ["entry.tsx:23:'await'", "100:19201:await"],
         ],
       },
     },
     expectExactFilesize: {
-      "out/entry.js": 222000,
+      "out/entry.js": 222360,
     },
     run: {
       stdout: "<!DOCTYPE html><html><body><h1>Hello World</h1><p>This is an example.</p></body></html>",

test/bundler/transpiler/runtime-transpiler.test.ts

@@ -252,3 +252,37 @@ describe("unterminated string literals in large files", () => {
     expect(exitCode).toBe(1);
   });
 });
+
+// Rewriting `return new Function(...)` into `return Function(...)` makes it a proper tail call in
+// strict mode code, and the Function constructor then takes the source origin for the new body
+// from whichever frame is left: the caller's caller, in another file.
+test("a function body built with `return new Function()` resolves import() relative to the returning module", async () => {
+  using dir = tempDir("transpiler-new-function-origin", {
+    "main.mjs": /* js */ `
+      import { make } from "./lib/make.mjs";
+      const { which } = await make()();
+      console.log(which);
+    `,
+    "lib/make.mjs": /* js */ `
+      export function make() {
+        return new Function("return import('./which.mjs')");
+      }
+    `,
+    "lib/which.mjs": `export const which = "lib/which.mjs";`,
+    "which.mjs": `export const which = "which.mjs";`,
+  });
+
+  await using proc = Bun.spawn({
+    cmd: [bunExe(), "main.mjs"],
+    env: bunEnv,
+    cwd: String(dir),
+    stdout: "pipe",
+    stderr: "pipe",
+  });
+
+  const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+
+  expect(stderr).toBe("");
+  expect(stdout).toBe("lib/which.mjs\n");
+  expect(exitCode).toBe(0);
+});

test/cli/hot/hot.test.ts

@@ -567,7 +567,7 @@ ${Buffer.alloc(counter * 2, " ").toString()}throw new Error(${counter});`,
         const match = nextLine.match(/\s*at.*?:1003:(\d+)$/);
         if (!match) throw new Error("invalid string: " + nextLine);
         const col = match[1];
-        expect(Number(col)).toBe(1 + "throw new ".length + counter * 2);
+        expect(Number(col)).toBe(1 + "throw ".length + counter * 2);
       },
     });
     await runner.exited;
@@ -614,7 +614,7 @@ ${Buffer.alloc(counter * 2, " ").toString()}throw new Error('${counter}');`,
         const match = nextLine.match(/\s*at.*?:(\d+):(\d+)\)?$/);
         if (!match) throw new Error("no :line:col in: " + JSON.stringify(nextLine));
         if (match[1] !== "1003") throw new Error("expected :1003: but got: " + JSON.stringify(nextLine));
-        expect(Number(match[2])).toBe(1 + "throw new ".length + counter * 2);
+        expect(Number(match[2])).toBe(1 + "throw ".length + counter * 2);
       },
     });
     await runner.exited;

test/js/bun/http/serve.test.ts

@@ -1950,8 +1950,8 @@ it.concurrent("dev error page embeds the thrown error, its stack, and build/reso
   expect(exception.stack.frames[0]).toEqual({
     function_name: "inner",
     file: join(String(dir), "server.ts"),
-    // 1-based, pointing at `TypeError` — the same position `bun` prints to the terminal
-    position: { line: 3, column: 19 },
+    // 1-based, pointing at `new`, the same position `bun` prints to the terminal
+    position: { line: 3, column: 15 },
     scope: 3, // function
   });
   expect(exception.stack.source_lines).toContainEqual({

test/js/bun/test/stack.test.ts

@@ -77,7 +77,7 @@ test("err.line and err.column are set", async () => {
         line: 3,
         column: 17,
         originalLine: 1,
-        originalColumn: 18,
+        originalColumn: 22,
       },
       null,
       2,
@@ -149,3 +149,47 @@ test("Async functions frame should be included in stack trace", async () => {
         at async <anonymous> (file:NN:NN)"
   `);
 });
+
+// Modules are strict mode code, and in strict mode JSC turns `return f()` into a proper tail
+// call: the returning function's frame is gone by the time `f` captures the stack. `new f()` is
+// never a tail call, so the transpiler must not rewrite `new Error()` into `Error()` to save bytes.
+test("a function returning `new Error()` is in the error's stack", () => {
+  const factories: Record<string, () => Error> = {
+    error() {
+      return new Error("error");
+    },
+    typeError() {
+      return new TypeError("typeError");
+    },
+    syntaxError() {
+      return new SyntaxError("syntaxError");
+    },
+    rangeError() {
+      return new RangeError("rangeError");
+    },
+    referenceError() {
+      return new ReferenceError("referenceError");
+    },
+    evalError() {
+      return new EvalError("evalError");
+    },
+    uriError() {
+      return new URIError("uriError");
+    },
+    aggregateError() {
+      return new AggregateError([], "aggregateError");
+    },
+    viaConst() {
+      // the single-use binding gets inlined into the return statement
+      const e = new Error("viaConst");
+      return e;
+    },
+    arrow: () => new Error("arrow"),
+  };
+
+  const missingOwnFrame = Object.keys(factories).filter(
+    name => !factories[name]().stack!.includes(`\n    at ${name} (`),
+  );
+
+  expect(missingOwnFrame).toEqual([]);
+});

test/js/bun/test/test-error-code-done-callback.test.ts

@@ -49,7 +49,7 @@ test("verify we print error messages passed to done callbacks", () => {
     27 |   done(new Error(msg + "(sync)"));
     ^
     error: you should see this(sync)
-    at <anonymous> (<dir>/test-error-done-callback-fixture.ts:27:12)
+    at <anonymous> (<dir>/test-error-done-callback-fixture.ts:27:8)
     (fail) error done callback (sync)
     27 |   done(new Error(msg + "(sync)"));
     28 | });
@@ -59,7 +59,7 @@ test("verify we print error messages passed to done callbacks", () => {
     32 |   done(new Error(msg + "(async with await)"));
     ^
     error: you should see this(async with await)
-    at <anonymous> (<dir>/test-error-done-callback-fixture.ts:32:12)
+    at <anonymous> (<dir>/test-error-done-callback-fixture.ts:32:8)
     (fail) error done callback (async with await)
     32 |   done(new Error(msg + "(async with await)"));
     33 | });
@@ -69,7 +69,7 @@ test("verify we print error messages passed to done callbacks", () => {
     37 |   done(new Error(msg + "(async with Bun.sleep)"));
     ^
     error: you should see this(async with Bun.sleep)
-    at <anonymous> (<dir>/test-error-done-callback-fixture.ts:37:12)
+    at <anonymous> (<dir>/test-error-done-callback-fixture.ts:37:8)
     (fail) error done callback (async with Bun.sleep)
     37 |   done(new Error(msg + "(async with Bun.sleep)"));
     38 | });
@@ -79,7 +79,7 @@ test("verify we print error messages passed to done callbacks", () => {
     42 |     done(new Error(msg + "(async)"));
     ^
     error: you should see this(async)
-    at <anonymous> (<dir>/test-error-done-callback-fixture.ts:42:14)
+    at <anonymous> (<dir>/test-error-done-callback-fixture.ts:42:10)
     at <anonymous> (<dir>/test-error-done-callback-fixture.ts:37:3)
     (fail) error done callback (async)
     43 |   });
@@ -90,7 +90,7 @@ test("verify we print error messages passed to done callbacks", () => {
     48 |     done(new Error(msg + "(async, setTimeout)"));
     ^
     error: you should see this(async, setTimeout)
-    at <anonymous> (<dir>/test-error-done-callback-fixture.ts:48:14)
+    at <anonymous> (<dir>/test-error-done-callback-fixture.ts:48:10)
     (fail) error done callback (async, setTimeout)
     49 |   }, 0);
     50 | });
@@ -100,7 +100,7 @@ test("verify we print error messages passed to done callbacks", () => {
     54 |     done(new Error(msg + "(async, setImmediate)"));
     ^
     error: you should see this(async, setImmediate)
-    at <anonymous> (<dir>/test-error-done-callback-fixture.ts:54:14)
+    at <anonymous> (<dir>/test-error-done-callback-fixture.ts:54:10)
     (fail) error done callback (async, setImmediate)
     55 |   });
     56 | });
@@ -110,7 +110,7 @@ test("verify we print error messages passed to done callbacks", () => {
     60 |     done(new Error(msg + "(async, nextTick)"));
     ^
     error: you should see this(async, nextTick)
-    at <anonymous> (<dir>/test-error-done-callback-fixture.ts:60:14)
+    at <anonymous> (<dir>/test-error-done-callback-fixture.ts:60:10)
     at <anonymous> (<dir>/test-error-done-callback-fixture.ts:54:5)
     (fail) error done callback (async, nextTick)
     62 | });
@@ -121,7 +121,7 @@ test("verify we print error messages passed to done callbacks", () => {
     67 |       done(new Error(msg + "(async, setTimeout, Promise.resolve)"));
     ^
     error: you should see this(async, setTimeout, Promise.resolve)
-    at <anonymous> (<dir>/test-error-done-callback-fixture.ts:67:16)
+    at <anonymous> (<dir>/test-error-done-callback-fixture.ts:67:12)
     (fail) error done callback (async, setTimeout, Promise.resolve)
     70 | });
     71 |
@@ -131,7 +131,7 @@ test("verify we print error messages passed to done callbacks", () => {
     75 |       done(new Error(msg + "(async, setImmediate, Promise.resolve)"));
     ^
     error: you should see this(async, setImmediate, Promise.resolve)
-    at <anonymous> (<dir>/test-error-done-callback-fixture.ts:75:16)
+    at <anonymous> (<dir>/test-error-done-callback-fixture.ts:75:12)
     (fail) error done callback (async, setImmediate, Promise.resolve)
 
     0 pass

test/js/bun/test/test-test.test.ts

@@ -735,10 +735,10 @@ test("my-test", () => {
       const stackLines = output.split("\n").filter(line => line.trim().startsWith("at "));
       expect(stackLines.length).toBeGreaterThan(0);
       if (process.platform === "win32") {
-        expect(stackLines[0]).toContain(`<dir>\\my-test.test.js:5:15`.replace("<dir>", test_dir));
+        expect(stackLines[0]).toContain(`<dir>\\my-test.test.js:5:11`.replace("<dir>", test_dir));
       }
       if (process.platform !== "win32") {
-        expect(stackLines[0]).toContain(`<dir>/my-test.test.js:5:15`.replace("<dir>", test_dir));
+        expect(stackLines[0]).toContain(`<dir>/my-test.test.js:5:11`.replace("<dir>", test_dir));
       }
 
       expect(output).toContain("1 pass"); // since the error is unhandled and in a hook, the error does not get attributed to the hook and the test is still allowed to run