error printer: fix the code frame lines above errors thrown from vm/eval sources

src/jsc/bindings/ZigException.cpp

@@ -161,22 +161,34 @@ static void populateStackFramePosition(const JSC::StackFrame& stackFrame, BunStr
         return;
 
     if (source_lines_count > 1 && source_lines != nullptr && sourceString.is8Bit()) {
-        // Search for the beginning of the line
-        unsigned int lineStart = location.byte_position;
-        while (lineStart > 0 && sourceString[lineStart] != '\n') {
-            lineStart--;
-        }
+        const std::span<const Latin1Character> bytes = sourceString.span8();
+        const unsigned length = sourceString.length();
 
-        // Search for the end of the line
-        unsigned int lineEnd = location.byte_position;
-        unsigned int maxSearch = sourceString.length();
-        while (lineEnd < maxSearch && sourceString[lineEnd] != '\n') {
-            lineEnd++;
-        }
+        // JSC may position an expression one past its end: on the '\n' ending its line, or at `length`.
+        const unsigned divot = std::min(static_cast<unsigned>(std::max(location.byte_position, 0)), length);
 
-        const unsigned char* bytes = sourceString.span8().data();
+        // `end` is the offset of a line's terminating '\n', or `length` for the last line.
+        auto startOfLineEndingAt = [&](unsigned end) -> unsigned {
+            unsigned start = end;
+            while (start > 0 && bytes[start - 1] != '\n') {
+                start--;
+            }
+            return start;
+        };
 
-        // Most of the time, when you look at a stack trace, you want a couple lines above.
+        // The line's text without its terminator ("\r\n" included).
+        auto lineText = [&](unsigned start, unsigned end) -> BunString {
+            if (end > start && bytes[end - 1] == '\r') {
+                end--;
+            }
+            return Bun::toStringView(StringView_slice(sourceString, start, end));
+        };
+
+        unsigned lineEnd = divot;
+        while (lineEnd < length && bytes[lineEnd] != '\n') {
+            lineEnd++;
+        }
+        unsigned lineStart = startOfLineEndingAt(lineEnd);
 
         // It is key to not clone this data because source code strings are large.
         // Usage of toStringView (non-owning) is safe as we ref the provider.
@@ -185,41 +197,16 @@ static void populateStackFramePosition(const JSC::StackFrame& stackFrame, BunStr
             (*referenced_source_provider)->deref();
         }
         *referenced_source_provider = provider;
-        source_lines[0] = Bun::toStringView(sourceString.substring(lineStart, lineEnd - lineStart));
+        source_lines[0] = lineText(lineStart, lineEnd);
         source_line_numbers[0] = location.line();
 
-        if (lineStart > 0) {
-            auto byte_offset_in_source_string = lineStart - 1;
-            uint8_t source_line_i = 1;
-            auto remaining_lines_to_grab = source_lines_count - 1;
-
-            {
-                // This should probably be code points instead of newlines
-                while (byte_offset_in_source_string > 0 && bytes[byte_offset_in_source_string] != '\n') {
-                    byte_offset_in_source_string--;
-                }
-
-                byte_offset_in_source_string -= byte_offset_in_source_string > 0;
-            }
-
-            while (byte_offset_in_source_string > 0 && remaining_lines_to_grab > 0) {
-                unsigned int end_of_line_offset = byte_offset_in_source_string;
-
-                // This should probably be code points instead of newlines
-                while (byte_offset_in_source_string > 0 && bytes[byte_offset_in_source_string] != '\n') {
-                    byte_offset_in_source_string--;
-                }
-
-                // We are at the beginning of the line
-                source_lines[source_line_i] = Bun::toStringView(sourceString.substring(byte_offset_in_source_string, end_of_line_offset - byte_offset_in_source_string + 1));
-
-                source_line_numbers[source_line_i] = location.line().fromZeroBasedInt(location.line().zeroBasedInt() - source_line_i);
-                source_line_i++;
-
-                remaining_lines_to_grab--;
-
-                byte_offset_in_source_string -= byte_offset_in_source_string > 0;
-            }
+        // Lines above: `bytes[lineStart - 1]` is the '\n' ending the line above the one just collected.
+        for (uint8_t i = 1; i < source_lines_count && lineStart > 0; i++) {
+            const unsigned aboveEnd = lineStart - 1;
+            const unsigned aboveStart = startOfLineEndingAt(aboveEnd);
+            source_lines[i] = lineText(aboveStart, aboveEnd);
+            source_line_numbers[i] = OrdinalNumber::fromZeroBasedInt(location.line_zero_based - i);
+            lineStart = aboveStart;
         }
     }
 }

test/js/node/vm/__snapshots__/vm-sourceUrl.test.ts.snap

@@ -12,7 +12,10 @@ Error: hello
 `;
 
 exports[`can get sourceURL inside node:vm 1`] = `
-"4 |     return Bun.inspect(new Error("hello"));
+"1 | 
+2 | 
+3 | function hello() {
+4 |     return Bun.inspect(new Error("hello"));
                            ^
 error: hello
       at hello (hellohello.js:4:24)
@@ -22,7 +25,10 @@ error: hello
 `;
 
 exports[`eval sourceURL is correct 1`] = `
-"4 |     return Bun.inspect(new Error("hello"));
+"1 | 
+2 | 
+3 | function hello() {
+4 |     return Bun.inspect(new Error("hello"));
                            ^
 error: hello
       at hello (hellohello.js:4:24)

test/js/node/vm/vm.test.ts

@@ -1,5 +1,5 @@
 import { describe, expect, test } from "bun:test";
-import { bunEnv, bunExe, normalizeBunSnapshot } from "harness";
+import { bunEnv, bunExe, isWindows, normalizeBunSnapshot } from "harness";
 import {
   compileFunction,
   constants,
@@ -1484,3 +1484,101 @@ test("node:vm Object.defineProperty on the context global when the sandbox is an
   expect(stdout.trim()).toBe(JSON.stringify({ result: 1, sandboxArray: 1 }));
   expect(exitCode).toBe(0);
 });
+
+// A vm script has no source map, so the code frame bun prints above an error
+// thrown from it is cut out of the script text itself (the same path serves
+// eval and new Function). Bun.inspect renders the same frame as the
+// uncaught-error printer.
+describe("code frame of an error thrown from a vm script", () => {
+  function codeFrame(source: string, options: { lineOffset?: number } = {}): string[] {
+    let error: unknown;
+    try {
+      // With displayErrors, node:vm replaces err.stack with node's own header.
+      runInThisContext(source, { filename: "frame.js", displayErrors: false, ...options });
+    } catch (e) {
+      error = e;
+    }
+    return Bun.inspect(error).split("\n");
+  }
+
+  const throwLine = "throw new Error('x');";
+  const numbered = (count: number) => Array.from({ length: count }, (_, i) => `'L${i + 1}';`);
+  // JSC positions a ReferenceError one past the end of the identifier.
+  const fooFrame = ["1 | 'L1';", "2 | foo", "       ^", "ReferenceError: foo is not defined"];
+  const fooAtEndOfSource = "'L1';\nfoo";
+
+  test.each([
+    [
+      "every line above the error, numbered",
+      [...numbered(4), throwLine].join("\n"),
+      ["1 | 'L1';", "2 | 'L2';", "3 | 'L3';", "4 | 'L4';", `5 | ${throwLine}`, "          ^", "error: x"],
+    ],
+    ["error on line 1", [throwLine, "'L2';"].join("\n"), [`1 | ${throwLine}`, "          ^", "error: x"]],
+    ["error on line 2", ["'L1';", throwLine].join("\n"), ["1 | 'L1';", `2 | ${throwLine}`, "          ^", "error: x"]],
+    [
+      "at most five lines above the error",
+      [...numbered(9), throwLine].join("\n"),
+      [
+        " 5 | 'L5';",
+        " 6 | 'L6';",
+        " 7 | 'L7';",
+        " 8 | 'L8';",
+        " 9 | 'L9';",
+        `10 | ${throwLine}`,
+        "           ^",
+        "error: x",
+      ],
+    ],
+    [
+      "blank lines, including a blank first line",
+      ["", "'L2';", "", throwLine].join("\n"),
+      ["1 | ", "2 | 'L2';", "3 | ", `4 | ${throwLine}`, "          ^", "error: x"],
+    ],
+    [
+      "CRLF line endings, including a blank line",
+      ["'L1';", "'L2';", "", throwLine, "'L5';", ""].join("\r\n"),
+      ["1 | 'L1';", "2 | 'L2';", "3 | ", `4 | ${throwLine}`, "          ^", "error: x"],
+    ],
+    ["error positioned on the newline ending its line", "'L1';\nfoo\n'L3';", fooFrame],
+    ["error positioned at the end of the source", fooAtEndOfSource, fooFrame],
+  ])("%s", (_, source, expected) => {
+    expect(codeFrame(source).slice(0, expected.length)).toEqual(expected);
+  });
+
+  test("line numbers include lineOffset", () => {
+    const expected = ["11 | 'L1';", "12 | 'L2';", `13 | ${throwLine}`, "           ^", "error: x"];
+    expect(codeFrame([...numbered(2), throwLine].join("\n"), { lineOffset: 10 }).slice(0, expected.length)).toEqual(
+      expected,
+    );
+  });
+
+  test("uncaught error", async () => {
+    await using proc = Bun.spawn({
+      cmd: [
+        bunExe(),
+        "-e",
+        `require("node:vm").runInThisContext(${JSON.stringify(fooAtEndOfSource)}, { filename: "frame.js", displayErrors: false })`,
+      ],
+      env: {
+        ...bunEnv,
+        // Malloc=1 routes JSC's allocations through the system allocator so ASAN
+        // sees a read past the end of the source string (bmalloc's system heap
+        // is unimplemented on Windows, which has no ASAN lane anyway);
+        // detect_leaks=0 because it also exposes JSC's never-freed startup
+        // allocations to LSAN.
+        ...(isWindows ? {} : { Malloc: "1" }),
+        ASAN_OPTIONS: [bunEnv.ASAN_OPTIONS, "detect_leaks=0"].filter(Boolean).join(":"),
+      },
+      stdout: "pipe",
+      stderr: "pipe",
+    });
+
+    const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
+
+    expect({ stdout, frame: stderr.split("\n").slice(0, fooFrame.length), exitCode }).toEqual({
+      stdout: "",
+      frame: fooFrame,
+      exitCode: 1,
+    });
+  });
+});