error.stack: report frames at new X(...) at the new keyword

src/jsc/bindings/ErrorStackFrame.cpp

@@ -1,65 +1,49 @@
 #include "root.h"
+#include "ErrorStackFrame.h"
 #include "JavaScriptCore/CodeBlock.h"
-#include "headers-handwritten.h"
-#include "JavaScriptCore/BytecodeIndex.h"
-#include "wtf/Assertions.h"
+#include "JavaScriptCore/StackFrame.h"
 #include "wtf/text/OrdinalNumber.h"
 
 namespace Bun {
 using namespace JSC;
 
-/// 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.
-void adjustPositionBackwards(ZigStackFramePosition& pos, int amount, CodeBlock* code)
+/// Moves `pos` (the divot of an expression) back `amount` code units, to the start of the
+/// expression. When that crosses a line boundary the line and column have to be recounted from
+/// the source text. If that is not possible, `pos` is left pointing at the divot.
+static void adjustPositionBackwards(ZigStackFramePosition& pos, int amount, CodeBlock* code)
 {
-    if (pos.byte_position - amount < 0) {
-        pos.line_zero_based = 0;
-        pos.column_zero_based = 0;
-        pos.byte_position = 0;
+    if (amount <= 0 || pos.byte_position < amount)
+        return;
+
+    int start = pos.byte_position - amount;
+
+    if (pos.column_zero_based >= amount) {
+        pos.column_zero_based -= amount;
+        pos.byte_position = start;
         return;
     }
 
-    pos.column_zero_based = pos.column_zero_based - amount;
-    if (pos.column_zero_based < 0) {
-        auto* provider = code->source().provider();
-        if (!provider) {
-            pos.line_zero_based = 0;
-            pos.column_zero_based = 0;
-            pos.byte_position = 0;
-            return;
-        }
-
-        auto source = provider->source();
-        if (!source.is8Bit()) {
-            // Debug-only assertion
-            // Bun does not yet use 16-bit sources anywhere. The transpiler ensures everything
-            // fit's into latin1 / 8-bit strings for on-average lower memory usage.
-            ASSERT_NOT_REACHED("16-bit source re-mapping is not implemented here.");
-
-            pos.line_zero_based = 0;
-            pos.column_zero_based = 0;
-            pos.byte_position = 0;
-            return;
-        }
-
-        for (int i = 0; i < amount; i++) {
-            if (source[pos.byte_position - i] == '\n') {
-                pos.line_zero_based = pos.line_zero_based - 1;
-            }
-        }
-
-        int columns = 0;
-        // Initial -1 to skip the newline that gets counted.
-        int i = pos.byte_position - amount - 1;
-        while (i > 0 && source[i] != '\n') {
-            columns += 1;
-            i -= 1;
-        }
-        pos.column_zero_based = columns;
+    auto* provider = code->source().provider();
+    if (!provider)
+        return;
+
+    // eval, new Function and node:vm code is not transpiled, so unlike transpiled modules
+    // its source can be 16-bit. StringView indexing handles both encodings.
+    WTF::StringView source = provider->source();
+    if (static_cast<unsigned>(pos.byte_position) > source.length())
+        return;
+
+    for (int i = start; i < pos.byte_position; i++) {
+        if (source[i] == '\n')
+            pos.line_zero_based--;
     }
 
-    pos.byte_position -= amount;
+    int column = 0;
+    for (int i = start - 1; i >= 0 && source[i] != '\n'; i--)
+        column++;
+
+    pos.column_zero_based = column;
+    pos.byte_position = start;
 }
 
 ZigStackFramePosition getAdjustedPositionForBytecode(JSC::CodeBlock* code, JSC::BytecodeIndex bc)
@@ -98,4 +82,17 @@
     return pos;
 }
 
+ZigStackFramePosition getAdjustedPositionForStackFrame(const JSC::StackFrame& frame)
+{
+    if (!frame.hasLineAndColumnInfo() || !frame.hasBytecodeIndex()) {
+        return ZigStackFramePosition {
+            .line_zero_based = -1,
+            .column_zero_based = -1,
+            .byte_position = -1,
+        };
+    }
+
+    return getAdjustedPositionForBytecode(frame.codeBlock(), frame.bytecodeIndex());
+}
+
 } // namespace Bun

src/jsc/bindings/ErrorStackFrame.h

@@ -1,9 +1,22 @@
+#pragma once
+
 #include "root.h"
 #include "headers-handwritten.h"
 #include "JavaScriptCore/BytecodeIndex.h"
 
+namespace JSC {
+class CodeBlock;
+class StackFrame;
+}
+
 namespace Bun {
 
+/// Source position of the bytecode at `bc`, moved to where V8 reports it
+/// (`new X(...)` is reported at `new`, JSC's divot is at the end of `X`).
 ZigStackFramePosition getAdjustedPositionForBytecode(JSC::CodeBlock* code, JSC::BytecodeIndex bc);
 
+/// Same for a captured stack frame. Frames without a code block (native, wasm) get a position
+/// with every field set to -1, which Bun__remapStackFramePositions skips.
+ZigStackFramePosition getAdjustedPositionForStackFrame(const JSC::StackFrame& frame);
+
 } // namespace Bun

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<ZigStackFrame, 8> remappedFrames;
     WTF::Vector<WTF::String, 8> sourceURLs;
-    WTF::Vector<LineColumn, 8> originalLineColumns;
+    WTF::Vector<ZigStackFramePosition, 8> 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,15 @@ 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();
+        // Same position Bun.inspect (ZigException.cpp) and CallSite use. Besides keeping
+        // the three in agreement, source maps have a mapping at the start of a `new`
+        // expression but usually not at JSC's divot (the end of the callee), so remapping
+        // the divot snaps to whatever mapping happens to precede it.
+        originalPositions[i] = Bun::getAdjustedPositionForStackFrame(frame);
 
         JSC::JSGlobalObject* globalObjectForFrame = lexicalGlobalObject;
         if (auto* callee = frame.callee()) {
@@ -280,8 +285,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 +320,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;
 

src/jsc/bindings/ZigException.cpp

@@ -144,20 +144,9 @@ static void populateStackFramePosition(const JSC::StackFrame& stackFrame, BunStr
     if (sourceString.isNull()) [[unlikely]]
         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;
-        return;
-    }
-
-    auto location = Bun::getAdjustedPositionForBytecode(code, stackFrame.bytecodeIndex());
-    memcpy(&position, &location, sizeof(ZigStackFramePosition));
-    if (flags == PopulateStackTraceFlags::OnlyPosition)
+    auto location = Bun::getAdjustedPositionForStackFrame(stackFrame);
+    position = location;
+    if (flags == PopulateStackTraceFlags::OnlyPosition || location.byte_position < 0)
         return;
 
     if (source_lines_count > 1 && source_lines != nullptr && sourceString.is8Bit()) {

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

@@ -1,6 +1,6 @@
 import { $ } from "bun";
-import { expect, test } from "bun:test";
-import { bunEnv, bunExe, bunRun, normalizeBunSnapshot } from "harness";
+import { describe, expect, test } from "bun:test";
+import { bunEnv, bunExe, bunRun, normalizeBunSnapshot, tempDir } from "harness";
 import { join } from "node:path";
 
 test("name property is used for function calls in Error.stack", () => {
@@ -149,3 +149,142 @@ test("Async functions frame should be included in stack trace", async () => {
         at async <anonymous> (file:NN:NN)"
   `);
 });
+
+// V8 reports a frame that is sitting at `new X(...)` at the `new` keyword. JSC's own position
+// is the end of `X`. Bun.inspect and Error.prepareStackTrace CallSites already move it back to
+// `new`; error.stack has to agree with them (and with Node).
+describe("error.stack column of a frame at `new X(...)` is the `new` keyword", () => {
+  // Keep this flush left: the expected line:column values below are positions in this source
+  // (the leading newline is dropped, so `class Thrower` is line 1).
+  const fixture = `
+class Thrower {
+  constructor() {
+    throw new Error("thrown by Thrower");
+  }
+}
+class MyError extends Error {}
+class Captures {
+  constructor(target) {
+    Error.captureStackTrace(target);
+  }
+}
+function customError() {
+  throw new MyError("custom");
+}
+function globalConstructor() {
+  return new Map(1);
+}
+function userConstructor() {
+  return new Thrower(1);
+}
+function spreadArguments(args) {
+  return new Map(...args);
+}
+function localBinding() {
+  const Ctor = Thrower;
+  return new Ctor(1);
+}
+function splitAcrossLines() {
+  return new
+    Map(1);
+}
+function viaCaptureStackTrace() {
+  const target = {};
+  new Captures(target);
+  return target.stack;
+}
+
+// "fixture.js:LINE:COLUMN" of the frame for the function called \`name\`.
+function frame(stack, name) {
+  const line = stack.split("\\n").find(line => line.includes("at " + name + " ("));
+  return line?.match(/fixture\\.js:\\d+:\\d+/)?.[0] ?? stack;
+}
+function caught(fn, ...args) {
+  try {
+    fn(...args);
+  } catch (e) {
+    return e;
+  }
+  throw new Error(fn.name + " did not throw");
+}
+
+const custom = caught(customError);
+console.log(
+  JSON.stringify({
+    customError: frame(custom.stack, "customError"),
+    customErrorLineColumn: [custom.line, custom.column],
+    globalConstructor: frame(caught(globalConstructor).stack, "globalConstructor"),
+    userConstructor: frame(caught(userConstructor).stack, "userConstructor"),
+    spreadArguments: frame(caught(spreadArguments, [1]).stack, "spreadArguments"),
+    localBinding: frame(caught(localBinding).stack, "localBinding"),
+    splitAcrossLines: frame(caught(splitAcrossLines).stack, "splitAcrossLines"),
+    viaCaptureStackTrace: frame(viaCaptureStackTrace(), "viaCaptureStackTrace"),
+  }),
+);
+`;
+
+  test.concurrent("in a transpiled file", async () => {
+    using dir = tempDir("stack-new-column", { "fixture.js": fixture.slice(1) });
+    const result = await bunRun(join(String(dir), "fixture.js"));
+    expect(result).toSpawn();
+    expect(JSON.parse(result.stdout)).toEqual({
+      customError: "fixture.js:13:9",
+      customErrorLineColumn: [13, 9],
+      globalConstructor: "fixture.js:16:10",
+      userConstructor: "fixture.js:19:10",
+      spreadArguments: "fixture.js:22:10",
+      localBinding: "fixture.js:26:10",
+      splitAcrossLines: "fixture.js:29:10",
+      viaCaptureStackTrace: "fixture.js:34:3",
+    });
+  });
+
+  // eval'd code is not transpiled, so `new` and its callee can really be on different lines and
+  // the position has to be recounted from the source text, which is 16-bit when the evaluated
+  // string is not latin1. The same position feeds error.stack, Bun.inspect and CallSites.
+  test.concurrent("in eval'd code with `new` on an earlier line than the callee", async () => {
+    const script = `
+const sources = {
+  latin1: "// latin1\\nfunction construct() {\\n  return new\\n    Map(1);\\n}\\nconstruct();\\n",
+  utf16: "// \\u4e2d\\u6587\\nfunction construct() {\\n  return new\\n    Map(1);\\n}\\nconstruct();\\n",
+  // \`new\` on the first line of the source, and a line break right after the callee.
+  firstLine: "function construct() { return new\\n  Map\\n  (1); }\\nconstruct();\\n",
+};
+const results = {};
+for (const [name, source] of Object.entries(sources)) {
+  const caught = () => {
+    try {
+      (0, eval)(source);
+    } catch (e) {
+      return e;
+    }
+  };
+  const lineColumn = text => text.match(/at construct \\(.*:(\\d+):(\\d+)\\)/).slice(1).join(":");
+
+  const stack = lineColumn(caught().stack);
+  const inspect = lineColumn(Bun.inspect(caught()));
+
+  const error = caught();
+  Error.prepareStackTrace = (_, callSites) => callSites;
+  const callSite = error.stack.find(callSite => callSite.getFunctionName() === "construct");
+  Error.prepareStackTrace = undefined;
+
+  results[name] = { stack, inspect, callSiteLine: callSite.getLineNumber() };
+}
+console.log(JSON.stringify(results));
+`;
+    await using proc = Bun.spawn({
+      cmd: [bunExe(), "-e", script],
+      env: bunEnv,
+      stderr: "pipe",
+    });
+    const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+    expect(stderr).toBe("");
+    expect(JSON.parse(stdout)).toEqual({
+      latin1: { stack: "3:10", inspect: "3:10", callSiteLine: 3 },
+      utf16: { stack: "3:10", inspect: "3:10", callSiteLine: 3 },
+      firstLine: { stack: "1:31", inspect: "1:31", callSiteLine: 1 },
+    });
+    expect(exitCode).toBe(0);
+  });
+});