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

src/jsc/bindings/ErrorStackFrame.cpp

@@ -1,65 +1,73 @@
 #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)
+// The LineTerminator set JSC's lexer counts lines with (LF, CR, U+2028, U+2029).
+static bool isLineTerminator(char16_t c)
 {
-    if (pos.byte_position - amount < 0) {
-        pos.line_zero_based = 0;
-        pos.column_zero_based = 0;
-        pos.byte_position = 0;
+    return c == '\n' || c == '\r' || c == 0x2028 || c == 0x2029;
+}
+
+/// Moves the divot in `pos` back `amount` code units, recounting line/column when that crosses a line break.
+static void adjustPositionBackwards(ZigStackFramePosition& pos, int amount, CodeBlock* code)
+{
+    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;
+
+    // Untranspiled sources (eval, new Function, node:vm) can be 16-bit; indexing handles both.
+    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 (!isLineTerminator(source[i]))
+            continue;
+        pos.line_zero_based--;
+        if (source[i] == '\r' && i + 1 < pos.byte_position && source[i + 1] == '\n')
+            i++;
     }
 
-    pos.byte_position -= amount;
+    int column = 0;
+    int i = start - 1;
+    for (; i >= 0 && !isLineTerminator(source[i]); i--)
+        column++;
+    // JSC's columns on the first line of a source include its start column (node:vm columnOffset).
+    if (i < 0)
+        column += provider->startPosition().m_column.zeroBasedInt();
+
+    pos.column_zero_based = column;
+    pos.byte_position = start;
+}
+
+// JavaScriptCore puts the divot of these at the `(` or the end of the callee; V8 reports the `new`.
+static bool isConstruct(JSC::CodeBlock* code, JSC::BytecodeIndex bc)
+{
+    switch (code->instructionAt(bc)->opcodeID()) {
+    case op_construct:
+    case op_construct_varargs:
+    case op_super_construct:
+    case op_super_construct_varargs:
+        return true;
+    default:
+        return false;
+    }
 }
 
 ZigStackFramePosition getAdjustedPositionForBytecode(JSC::CodeBlock* code, JSC::BytecodeIndex bc)
@@ -72,30 +80,47 @@ ZigStackFramePosition getAdjustedPositionForBytecode(JSC::CodeBlock* code, JSC::
         .byte_position = (int)expr.divot,
     };
 
-    auto inst = code->instructionAt(bc);
-
-    /// JavaScriptCore places error divots at different places than v8
-    // Uncomment to debug this:
-    // printf("lc = %d : %d (byte = %d)\n", pos.line.oneBasedInt(), pos.column.oneBasedInt(), expr.divot);
-    // printf("off = %d : %d\n", expr.startOffset, expr.endOffset);
-    // printf("name = %s\n", inst->name());
-
-    switch (inst->opcodeID()) {
-    case op_construct:
-    case op_construct_varargs:
-    case op_super_construct:
-    case op_super_construct_varargs:
-        // The divot by default is pointing at the `(` or the end of the class name.
-        // We want to point at the `new` keyword, which is conveniently at the
-        // expression start.
+    if (isConstruct(code, bc))
         adjustPositionBackwards(pos, expr.startOffset, code);
-        break;
 
-    default:
-        break;
+    return pos;
+}
+
+ZigStackFramePosition getAdjustedLineColumnForBytecode(JSC::CodeBlock* code, JSC::BytecodeIndex bc)
+{
+    if (isConstruct(code, bc)) {
+        auto pos = getAdjustedPositionForBytecode(code, bc);
+        pos.byte_position = -1;
+        return pos;
     }
 
-    return pos;
+    // Cached per bytecode index; expressionInfoForBytecodeIndex decodes a whole chapter every call.
+    auto lineColumn = code->lineColumnForBytecodeIndex(bc);
+    return ZigStackFramePosition {
+        .line_zero_based = OrdinalNumber::fromOneBasedInt(lineColumn.line).zeroBasedInt(),
+        .column_zero_based = OrdinalNumber::fromOneBasedInt(lineColumn.column).zeroBasedInt(),
+        .byte_position = -1,
+    };
+}
+
+static constexpr ZigStackFramePosition noPosition {
+    .line_zero_based = -1,
+    .column_zero_based = -1,
+    .byte_position = -1,
+};
+
+ZigStackFramePosition getAdjustedPositionForStackFrame(const JSC::StackFrame& frame)
+{
+    if (!frame.hasLineAndColumnInfo() || !frame.hasBytecodeIndex())
+        return noPosition;
+    return getAdjustedPositionForBytecode(frame.codeBlock(), frame.bytecodeIndex());
+}
+
+ZigStackFramePosition getAdjustedLineColumnForStackFrame(const JSC::StackFrame& frame)
+{
+    if (!frame.hasLineAndColumnInfo() || !frame.hasBytecodeIndex())
+        return noPosition;
+    return getAdjustedLineColumnForBytecode(frame.codeBlock(), frame.bytecodeIndex());
 }
 
 } // namespace Bun

src/jsc/bindings/ErrorStackFrame.h

@@ -1,9 +1,24 @@
+#pragma once
+
 #include "root.h"
 #include "headers-handwritten.h"
 #include "JavaScriptCore/BytecodeIndex.h"
 
+namespace JSC {
+class CodeBlock;
+class StackFrame;
+}
+
 namespace Bun {
 
+/// Position of the bytecode at `bc` where V8 would report it (`new X(...)` at `new`), with its source offset.
 ZigStackFramePosition getAdjustedPositionForBytecode(JSC::CodeBlock* code, JSC::BytecodeIndex bc);
 
+/// Line and column of the above only (byte_position is -1); cheap for frames that are not at a construct.
+ZigStackFramePosition getAdjustedLineColumnForBytecode(JSC::CodeBlock* code, JSC::BytecodeIndex bc);
+
+/// The two above for a captured frame; -1 in every field (an invalid position) without a code block.
+ZigStackFramePosition getAdjustedPositionForStackFrame(const JSC::StackFrame& frame);
+ZigStackFramePosition getAdjustedLineColumnForStackFrame(const JSC::StackFrame& frame);
+
 } // namespace Bun

src/jsc/bindings/ErrorStackTrace.cpp

@@ -423,7 +423,7 @@ bool JSCStackFrame::calculateSourcePositions()
         return false;
     }
 
-    auto location = Bun::getAdjustedPositionForBytecode(m_codeBlock, m_bytecodeIndex);
+    auto location = Bun::getAdjustedLineColumnForBytecode(m_codeBlock, m_bytecodeIndex);
     m_sourcePositions.line = location.line();
     m_sourcePositions.column = location.column();
 

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,12 @@ 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 as Bun.inspect and CallSite; source maps have a mapping at `new`, not at JSC's divot.
+        originalPositions[i] = Bun::getAdjustedLineColumnForStackFrame(frame);
 
         JSC::JSGlobalObject* globalObjectForFrame = lexicalGlobalObject;
         if (auto* callee = frame.callee()) {
@@ -280,8 +282,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;
             }
@@ -314,10 +315,12 @@ WTF::String formatStackTrace(
         OrdinalNumber displayLine = {};
         OrdinalNumber displayColumn = {};
         WTF::String sourceURLForFrame = sourceURLs[i];
+        // Still -1 for the frames pass 1 skipped (no code block) or could not position.
+        bool hasPosition = originalPositions[i].line_zero_based >= 0;
 
-        if (frame.hasLineAndColumnInfo()) {
-            originalLine = OrdinalNumber::fromOneBasedInt(originalLineColumns[i].line);
-            originalColumn = OrdinalNumber::fromOneBasedInt(originalLineColumns[i].column);
+        if (hasPosition) {
+            originalLine = originalPositions[i].line();
+            originalColumn = originalPositions[i].column();
             displayLine = originalLine;
             displayColumn = originalColumn;
 
@@ -370,14 +373,11 @@ WTF::String formatStackTrace(
 
         if (!sourceURLForFrame.isEmpty()) {
             sb.append(sourceURLForFrame);
-            if (displayLine.zeroBasedInt() > 0 || displayColumn.zeroBasedInt() > 0) {
+            if (hasPosition) {
                 sb.append(':');
                 sb.append(displayLine.oneBasedInt());
-
-                if (displayColumn.zeroBasedInt() > 0) {
-                    sb.append(':');
-                    sb.append(displayColumn.oneBasedInt());
-                }
+                sb.append(':');
+                sb.append(displayColumn.oneBasedInt());
             }
         }
 

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/bake/dev/server-sourcemap.test.ts

@@ -43,7 +43,9 @@ export async function getStaticPaths() {
     // Strip ANSI codes for cleaner checking
     const cleanLines = lines.replace(/\x1b\[[0-9;]*m/g, "");
 
-    const hasCorrectThrowLine = cleanLines.includes("myFunc") && cleanLines.includes("6:16");
+    // Frames remap to the declaration position here, like the `throwError`/`6:1`
+    // and `helperFunction`/`5:1` expectations below.
+    const hasCorrectThrowLine = /at myFunc \(.*pages[/\\]\[\.\.\.slug\]\.tsx:6:1\)/.test(cleanLines);
     // const hasCorrectCallLine = cleanLines.includes("MyPage") && cleanLines.includes("2") && cleanLines.includes("3");
     const hasCorrectFileName = cleanLines.includes("pages/[...slug].tsx");