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

src/ast/known_global.rs

@@ -65,18 +65,6 @@ fn lookup(name: &[u8]) -> Option<KnownGlobal> {
 }
 
 impl KnownGlobal {
-    #[inline(always)]
-    fn call_from_new(e: &mut E::New, loc: crate::Loc) -> js_ast::Expr {
-        let call = E::Call {
-            target: e.target,
-            args: bun_alloc::AstAlloc::take(&mut e.args),
-            close_paren_loc: e.close_parens_loc,
-            can_be_unwrapped_if_unused: e.can_be_unwrapped_if_unused,
-            ..Default::default()
-        };
-        js_ast::Expr::init(call, loc)
-    }
-
     // `_bump` is unused; the `Vec` uses the global arena.
     #[inline(never)]
     pub fn minify_global_constructor(
@@ -102,19 +90,21 @@ impl KnownGlobal {
         let original_name = symbol.original_name.slice();
         let constructor = lookup(original_name)?;
 
+        // `new X(...)` is only ever folded into a literal, never rewritten into the call `X(...)`:
+        // in strict mode `return X(...)` is a proper tail call, so the frame creating the value is
+        // gone by the time `X` captures a stack trace, reads its source origin (`Function`) or throws.
         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
+            // `RegExp(re)` would also return `re` itself where `new RegExp(re)` copies it.
+            | KnownGlobal::RegExp => None,
 
             KnownGlobal::Object => {
                 let n = e.args.len_u32();
@@ -141,11 +131,20 @@ impl KnownGlobal {
                     }
                 }
 
-                // For other cases, just remove 'new'
-                Some(Self::call_from_new(e, loc))
+                None
             }
 
             KnownGlobal::Array => {
+                // `new Array(5, ...rest)` is `new Array(5)`, a length, when `rest` is empty.
+                if e
+                    .args
+                    .slice()
+                    .iter()
+                    .any(|arg| matches!(arg.data, js_ast::ExprData::ESpread(_)))
+                {
+                    return None;
+                }
+
                 let n = e.args.len_u32();
 
                 match n {
@@ -176,7 +175,6 @@ impl KnownGlobal {
                         // For other types, check via knownPrimitive
                         let primitive = arg.known_primitive();
                         // Only convert if we know for certain it's not a number
-                        // unknown could be a number at runtime, so we must preserve Array() call
                         match primitive {
                             js_ast::expr::PrimitiveType::Null
                             | js_ast::expr::PrimitiveType::Undefined
@@ -195,7 +193,7 @@ impl KnownGlobal {
                             js_ast::expr::PrimitiveType::Number => {
                                 let val = match arg.data {
                                     js_ast::ExprData::ENumber(num) => num.value(),
-                                    _ => return Some(Self::call_from_new(e, loc)),
+                                    _ => return None,
                                 };
                                 if
                                 // only want this with whitespace minification
@@ -231,13 +229,11 @@ impl KnownGlobal {
                                         loc,
                                     ));
                                 }
-                                Some(Self::call_from_new(e, loc))
+                                None
                             }
+                            // Could be a number, so `new Array(x)` may mean a length.
                             js_ast::expr::PrimitiveType::Unknown
-                            | js_ast::expr::PrimitiveType::Mixed => {
-                                // Could be a number, preserve Array() call
-                                Some(Self::call_from_new(e, loc))
-                            }
+                            | js_ast::expr::PrimitiveType::Mixed => None,
                         }
                     }
                     // > 1
@@ -255,18 +251,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
-                // - This affects object identity and lastIndex behavior
-                // - The difference only applies when flags are undefined
-                // Keep the original new RegExp() call to preserve correct semantics
-                None
-            }
             KnownGlobal::WeakSet | KnownGlobal::WeakMap => {
                 let n = e.args.len_u32();
 

src/jsc/RuntimeTranspilerCache.rs

@@ -51,7 +51,10 @@ bun_core::declare_scope!(cache, visible);
 /// Version 25: Every ModuleInfo record carries a trailing FetchParameters slot
 /// so ImportEntry/ExportEntry/StarExportEntry moduleRequestType matches JSC's
 /// after WebKit 90b2ecf79ae3 keyed m_loadedModules on (specifier, type).
-const EXPECTED_VERSION: u32 = 25;
+/// Version 26: `new Error(...)` (and the other native error constructors, `Function`,
+/// `Object`, `Array`) is no longer rewritten into a plain call; older entries hold the
+/// call form, which JSC tail-calls and so drops the creating function from the stack.
+const EXPECTED_VERSION: u32 = 26;
 
 /// Source files smaller than this are not written to / read from the on-disk
 /// transpiler cache. Originally 50 KiB, which excluded almost every file in a

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,41 @@ describe("bundler", () => {
     target: "bun",
   });
 
+  itBundled("minify/ReturnedConstructorKeepsItsFrame", {
+    files: {
+      "/entry.js": /* js */ `
+        function makeError() {
+          return new Error("made");
+        }
+        function makeTypeError() {
+          const err = new TypeError("made");
+          return err;
+        }
+        function makeArray(...lengths) {
+          return new Array(...lengths);
+        }
+        function thrownBy(make) {
+          try {
+            make(-1);
+          } catch (err) {
+            return err;
+          }
+        }
+        const frames = [
+          [makeError, makeError()],
+          [makeTypeError, makeTypeError()],
+          [makeArray, thrownBy(makeArray)],
+        ].map(([fn, err]) => err.stack.includes("at " + fn.name + " "));
+        console.log(JSON.stringify(frames));
+      `,
+    },
+    minifySyntax: true,
+    target: "bun",
+    run: {
+      stdout: "[true,true,true]",
+    },
+  });
+
   itBundled("minify/ErrorConstructorWithVariables", {
     files: {
       "/entry.js": /* js */ `
@@ -987,6 +1025,9 @@ describe("bundler", () => {
         // Test Array constructor
         capture(new Array());
         capture(new Array(3));
+        capture(new Array(unknownValue));
+        capture(new Array(...unknownValue));
+        capture(new Array(5, ...unknownValue));
         capture(new Array(1, 2, 3));
 
         // Test Array with non-numeric single arguments (should convert to literal)
@@ -1000,6 +1041,7 @@ describe("bundler", () => {
         capture(new Object());
         capture(new Object(null));
         capture(new Object({ a: 1 }));
+        capture(new Object(unknownValue));
 
         // Test Function constructor
         capture(new Function("return 42"));
@@ -1022,7 +1064,13 @@ describe("bundler", () => {
     },
     capture: [
       "[]", // new Array() -> []
-      "Array(3)", // new Array(3) stays as Array(3) because it creates sparse array
+      // A single argument (possibly what a spread leaves behind at runtime) may be a length, so these
+      // cannot become literals. They are not turned into `Array(...)` calls either (see
+      // ErrorConstructorKeepsNew); `new` stays as written.
+      "new Array(3)",
+      "new Array(unknownValue)",
+      "new Array(...unknownValue)",
+      "new Array(5, ...unknownValue)",
       `[
   1,
   2,
@@ -1046,8 +1094,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")',
+      "new Object(unknownValue)", // nothing to fold into a literal; kept as written
+      'new Function("return 42")',
+      'new Function("a", "b", "return a + b")',
       'new RegExp("test")',
       'new RegExp("test", "gi")',
       "new RegExp(/abc/)",
@@ -1090,8 +1139,8 @@ describe("bundler", () => {
       "[,,,,,,,,]", // new Array(8) -> [undefined x 8]
       "[,,,,,,,,,]", // new Array(9) -> [undefined x 9]
       "[,,,,,,,,,,]", // new Array(10) -> [undefined x 10]
-      "Array(11)", // new Array(11) -> Array(11)
-      "Array(4.5)", // new Array(4.5) is Array(4.5) because it's not an integer
+      "new Array(11)", // too long to spell out as a literal; kept as written
+      "new Array(4.5)", // not an integer; kept as written
     ],
     minifySyntax: true,
     minifyWhitespace: true,
@@ -1120,6 +1169,12 @@ describe("bundler", () => {
         const a3 = new Array(n);
         const a4 = Array(n);
         capture(a3.length === a4.length && a3.length === 3 && a3[0] === undefined);
+
+        // A spread can leave a single number behind at runtime, and then it is a length
+        const none = [];
+        const a5 = new Array(5, ...none);
+        capture(a5.length === 5);
+        capture(0 in a5 === false);
 
         // Test Object semantics
         const o1 = new Object();
@@ -1148,6 +1203,8 @@ describe("bundler", () => {
       "0 in sparse === !1",
       'JSON.stringify(sparse) === "[null,null,null,null,null]"',
       "a3.length === a4.length && a3.length === 3 && a3[0] === void 0",
+      "a5.length === 5",
+      "0 in a5 === !1",
       "typeof o1 === typeof o2",
       "o1.constructor === o2.constructor",
       "typeof f1 === typeof f2",
@@ -1158,7 +1215,7 @@ describe("bundler", () => {
     minifySyntax: true,
     target: "bun",
     run: {
-      stdout: "true\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue",
+      stdout: "true\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue\ntrue",
     },
   });
 

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:5637: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:7699:\'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": 222388,
     },
     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;