node:vm: report compileFunction body lines relative to lineOffset when it is 0

src/jsc/bindings/NodeVM.cpp

@@ -183,23 +183,21 @@
     }
 
     // wrap the arguments in an anonymous function expression
-    int startOffset = 0;
-    String code = stringifyAnonymousFunction(globalObject, args, throwScope, &startOffset);
+    int wrapperPrefixLength = 0;
+    String code = stringifyAnonymousFunction(globalObject, args, throwScope, &wrapperPrefixLength);
     EXCEPTION_ASSERT(!!throwScope.exception() == code.isNull());
+    RETURN_IF_EXCEPTION(throwScope, nullptr);
 
-    // The user's body starts on line 2 of the wrapped program (after the
-    // "(function () {\n" prefix). Shift the provider's start position up one
-    // line so reported positions line up with the body the way V8's
-    // CompileFunction does: body line 1 reports as lineOffset+1. JSC clamps
-    // non-positive provider start positions to zero, so once the input is
-    // already <= 0 there is nothing to gain by going further negative; clamp
-    // there to keep the value bounded for downstream arithmetic.
-    int lineZeroBased = position.m_line.zeroBasedInt();
-    TextPosition wrappedPosition(OrdinalNumber::fromZeroBasedInt(lineZeroBased > 0 ? lineZeroBased - 1 : lineZeroBased), position.m_column);
+    // Line 1's columns already include the wrapper, so only the excess of columnOffset over it is applied.
+    int columnOffset = position.m_column.zeroBasedInt();
+    TextPosition wrappedPosition(position.m_line, OrdinalNumber::fromZeroBasedInt(columnOffset > wrapperPrefixLength ? columnOffset - wrapperPrefixLength : 0));
 
-    SourceCode sourceCode(
-        JSC::StringSourceProvider::create(code, sourceOrigin, WTF::move(options.filename), sourceTaintOrigin, wrappedPosition, SourceProviderSourceType::Program),
-        wrappedPosition.m_line.oneBasedInt(), wrappedPosition.m_column.oneBasedInt());
+    Ref<JSC::SourceProvider> provider = JSC::StringSourceProvider::create(code, sourceOrigin, WTF::move(options.filename), sourceTaintOrigin, wrappedPosition, SourceProviderSourceType::Program);
+
+    if (auto* fetcher = sourceOrigin.fetcher(); fetcher && fetcher->fetcherType() == ScriptFetcher::Type::NodeVM)
+        static_cast<NodeVMScriptFetcher*>(fetcher)->setWrapper(provider.get(), static_cast<unsigned>(wrapperPrefixLength));
+
+    SourceCode sourceCode(WTF::move(provider), wrappedPosition.m_line.oneBasedInt(), wrappedPosition.m_column.oneBasedInt());
 
     CodeCache* cache = vm.codeCache();
     ProgramExecutable* programExecutable = ProgramExecutable::create(globalObject, sourceCode);
@@ -380,21 +378,22 @@
     RELEASE_AND_RETURN(scope, JSPromise::resolvedPromise(globalObject, thenResult));
 }
 
-// Helper function to create an anonymous function expression with parameters
+// The body deliberately shares the wrapper's line: JSC clamps a source's first line to 1, so a wrapper line could not be compensated for at lineOffset 0.
 String stringifyAnonymousFunction(JSGlobalObject* globalObject, const ArgList& args, ThrowScope& scope, int* outOffset)
 {
     // How we stringify functions is important for creating anonymous function expressions
     String program;
     if (args.isEmpty()) {
         // No arguments, just an empty function body
-        program = "(function () {\n\n})"_s;
+        program = "(function () {\n})"_s;
+        *outOffset = "(function () {"_s.length();
     } else if (args.size() == 1) {
         // Just the function body
         auto body = args.at(0).toWTFString(globalObject);
         RETURN_IF_EXCEPTION(scope, {});
 
-        program = tryMakeString("(function () {\n"_s, body, "\n})"_s);
-        *outOffset = "(function () {\n"_s.length();
+        program = tryMakeString("(function () {"_s, body, "\n})"_s);
+        *outOffset = "(function () {"_s.length();
 
         if (!program) [[unlikely]] {
             throwOutOfMemoryError(globalObject, scope);
@@ -419,8 +418,8 @@
         auto body = args.at(parameterCount).toWTFString(globalObject);
         RETURN_IF_EXCEPTION(scope, {});
 
-        program = tryMakeString("(function ("_s, paramString.toString(), ") {\n"_s, body, "\n})"_s);
-        *outOffset = "(function ("_s.length() + paramString.length() + ") {\n"_s.length();
+        program = tryMakeString("(function ("_s, paramString.toString(), ") {"_s, body, "\n})"_s);
+        *outOffset = "(function ("_s.length() + paramString.length() + ") {"_s.length();
 
         if (!program) [[unlikely]] {
             throwOutOfMemoryError(globalObject, scope);
@@ -490,7 +489,8 @@
 // AppendExceptionLine helpers shared by the runtime (handleException) and
 // compile-time (decorateParseErrorStack) paths — a single implementation of
 // Node's arrow-header format so the two call sites cannot drift.
-static String nthSourceLineForArrowHeader(StringView source, int64_t physicalLine1Based)
+// firstLineSkip: compileFunction's wrapper text in front of the user's first line.
+static String nthSourceLineForArrowHeader(StringView source, int64_t physicalLine1Based, unsigned firstLineSkip = 0)
 {
     if (physicalLine1Based < 1 || physicalLine1Based > static_cast<int64_t>(source.length()) + 1)
         return {};
@@ -505,6 +505,8 @@
     if (lineEnd == WTF::notFound)
         lineEnd = source.length();
     StringView lineView = source.substring(lineStart, lineEnd - lineStart);
+    if (physicalLine1Based == 1)
+        lineView = lineView.substring(firstLineSkip);
     if (lineView.endsWith('\r'))
         lineView = lineView.left(lineView.length() - 1);
     // Like Node, skip the decoration for excessively long lines.
@@ -574,14 +576,21 @@
         unsigned caretColumn = 0;
         if (JSC::CodeBlock* codeBlock = stack_frame.codeBlock()) {
             if (JSC::SourceProvider* provider = codeBlock->source().provider()) {
+                unsigned wrapperPrefixLength = 0;
+                if (auto* fetcher = provider->sourceOrigin().fetcher(); fetcher && fetcher->fetcherType() == ScriptFetcher::Type::NodeVM)
+                    wrapperPrefixLength = static_cast<NodeVMScriptFetcher*>(fetcher)->wrapperPrefixLength(*provider);
+
                 int64_t startLineZeroBased = provider->startPosition().m_line.zeroBasedInt();
                 int64_t physicalLine = static_cast<int64_t>(line_and_column.line) - startLineZeroBased;
-                sourceLineText = nthSourceLineForArrowHeader(provider->source(), physicalLine);
+                sourceLineText = nthSourceLineForArrowHeader(provider->source(), physicalLine, wrapperPrefixLength);
                 if (!sourceLineText.isNull()) {
                     caretColumn = line_and_column.column;
-                    unsigned startColumnZeroBased = static_cast<unsigned>(provider->startPosition().m_column.zeroBasedInt());
-                    if (physicalLine == 1 && caretColumn > startColumnZeroBased)
-                        caretColumn -= startColumnZeroBased;
+                    if (physicalLine == 1) {
+                        // SourceCode clamps a negative start column to 0, so that is what the frame's column includes.
+                        unsigned startColumnZeroBased = static_cast<unsigned>(std::max(0, provider->startPosition().m_column.zeroBasedInt()));
+                        unsigned firstLineShift = startColumnZeroBased + wrapperPrefixLength;
+                        caretColumn = caretColumn > firstLineShift ? caretColumn - firstLineShift : 0;
+                    }
                 }
             }
         }

src/jsc/bindings/NodeVMScriptFetcher.h

@@ -3,6 +3,7 @@
 #include "root.h"
 
 #include <JavaScriptCore/ScriptFetcher.h>
+#include <JavaScriptCore/SourceProvider.h>
 #include <JavaScriptCore/Weak.h>
 #include <JavaScriptCore/WeakInlines.h>
 #include <wtf/Scope.h>
@@ -41,6 +42,17 @@ class NodeVMScriptFetcher : public JSC::ScriptFetcher {
         });
     }
 
+    // Keyed by provider because eval() and new Function() code inherits this fetcher without the wrapper.
+    void setWrapper(JSC::SourceProvider& provider, unsigned prefixLength)
+    {
+        m_wrapperSourceID = provider.asID();
+        m_wrapperPrefixLength = prefixLength;
+    }
+    unsigned wrapperPrefixLength(JSC::SourceProvider& provider) const
+    {
+        return m_wrapperPrefixLength && provider.asID() == m_wrapperSourceID ? m_wrapperPrefixLength : 0;
+    }
+
 private:
     JSC::Strong<JSC::Unknown> m_dynamicImportCallback;
     // m_owner is the NodeVMScript / JSFunction / module wrapper that holds this
@@ -50,6 +62,8 @@ class NodeVMScriptFetcher : public JSC::ScriptFetcher {
     // as a GC root). Use Weak instead: when the owner is collected its
     // SourceCode chain drops the last RefPtr to this fetcher.
     JSC::Weak<JSC::JSCell> m_owner;
+    JSC::SourceID m_wrapperSourceID = 0;
+    unsigned m_wrapperPrefixLength = 0;
     bool m_isUsingDefaultLoader = false;
 
     NodeVMScriptFetcher(JSC::VM& vm, JSC::JSValue dynamicImportCallback, JSC::JSValue owner)

test/js/node/test/parallel/test-vm-basic.js

@@ -131,9 +131,14 @@ const vm = require('vm');
 
 // vm.compileFunction
 {
+  // Bun compiles the body on the same line as the wrapper so that stack
+  // frames report body line N as lineOffset + N (the engine cannot start a
+  // source at line 0), which is what toString() reflects.
   assert.strictEqual(
     vm.compileFunction('console.log("Hello, World!")').toString(),
-    'function () {\nconsole.log("Hello, World!")\n}'
+    typeof Bun === 'undefined'
+      ? 'function () {\nconsole.log("Hello, World!")\n}'
+      : 'function () {console.log("Hello, World!")\n}'
   );
 
   assert.strictEqual(
@@ -277,17 +282,18 @@ const vm = require('vm');
   // Setting value to run the last three tests
   Error.stackTraceLimit = 1;
 
-  // Bun's compileFunction stack frames differ from Node's: JSC attributes
-  // the throw to a different column (and columnOffset is not applied), the
-  // wrapper costs one line when lineOffset is 0, and the anonymous source is
-  // labeled differently.
+  // Bun's compileFunction stack frames differ from Node's in their columns:
+  // JSC attributes the throw to a different column than V8, and columns on
+  // body line 1 also count the `(function () {` wrapper the body shares its
+  // line with (so a columnOffset shorter than the wrapper has no effect).
+  // The anonymous source is labeled differently too. Lines match Node.
   assert.throws(() => {
     vm.compileFunction('throw new Error("Sample Error")')();
   }, {
     message: 'Sample Error',
     stack: typeof Bun === 'undefined'
       ? 'Error: Sample Error\n    at <anonymous>:1:7'
-      : 'Error: Sample Error\n    at <anonymous> (file:///:2:16)'
+      : 'Error: Sample Error\n    at <anonymous> (file:///:1:30)'
   });
 
   assert.throws(() => {
@@ -300,7 +306,7 @@ const vm = require('vm');
     message: 'Sample Error',
     stack: typeof Bun === 'undefined'
       ? 'Error: Sample Error\n    at <anonymous>:4:7'
-      : 'Error: Sample Error\n    at <anonymous> (file:///:4:16)'
+      : 'Error: Sample Error\n    at <anonymous> (file:///:4:30)'
   });
 
   assert.throws(() => {
@@ -313,7 +319,7 @@ const vm = require('vm');
     message: 'Sample Error',
     stack: typeof Bun === 'undefined'
       ? 'Error: Sample Error\n    at <anonymous>:1:10'
-      : 'Error: Sample Error\n    at <anonymous> (file:///:2:16)'
+      : 'Error: Sample Error\n    at <anonymous> (file:///:1:30)'
   });
 
   assert.strictEqual(
@@ -337,7 +343,7 @@ const vm = require('vm');
     // Bun's compileFunction stack frames differ from Node's (see above).
     stack: typeof Bun === 'undefined'
       ? 'ReferenceError: varInContext is not defined\n    at <anonymous>:1:1'
-      : 'ReferenceError: varInContext is not defined\n    at <anonymous> (file:///:2:20)'
+      : 'ReferenceError: varInContext is not defined\n    at <anonymous> (file:///:1:34)'
   });
 
   assert.notDeepStrictEqual(

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

@@ -269,6 +269,169 @@ describe("vm", () => {
         expect(e).toBeTruthy();
       }
     });
+
+    // Positions of runtime errors thrown by the compiled function. Node
+    // reports body line N as lineOffset + N; the body is line 1, not the
+    // line after a wrapper.
+    function thrownPosition(filename: string, fn: () => unknown) {
+      let stack: string = "";
+      try {
+        fn();
+      } catch (e: any) {
+        stack = e.stack;
+      }
+      const match = stack.match(new RegExp(`${filename.replaceAll(".", "\\.")}:(\\d+):(\\d+)`));
+      if (!match) throw new Error(`no ${filename} frame in:\n${stack}`);
+      return { line: Number(match[1]), column: Number(match[2]) };
+    }
+
+    test("runtime errors report body line N as lineOffset + N", () => {
+      const filename = "cf-lines.js";
+      const results: unknown[] = [];
+      const expected: unknown[] = [];
+      for (const lineOffset of [undefined, 0, 1, 7]) {
+        const options = lineOffset === undefined ? { filename } : { filename, lineOffset };
+        const base = lineOffset ?? 0;
+        results.push({
+          lineOffset,
+          line1: thrownPosition(filename, compileFunction('throw new Error("line 1")', [], options)).line,
+          line3: thrownPosition(filename, compileFunction('1;\n2;\nthrow new Error("line 3")', [], options)).line,
+          line1WithParams: thrownPosition(filename, () =>
+            compileFunction("throw new Error(String(a + b))", ["a", "b"], options)(1, 2),
+          ).line,
+          // Functions nested in the body are positioned relative to the same origin.
+          nestedArrowOnLine1: thrownPosition(
+            filename,
+            compileFunction('return () => { throw new Error("arrow") };', [], options)(),
+          ).line,
+          nestedFunctionOnLine2: thrownPosition(
+            filename,
+            compileFunction('return function inner() {\n  throw new Error("inner");\n};', [], options)(),
+          ).line,
+        });
+        expected.push({
+          lineOffset,
+          line1: base + 1,
+          line3: base + 3,
+          line1WithParams: base + 1,
+          nestedArrowOnLine1: base + 1,
+          nestedFunctionOnLine2: base + 2,
+        });
+      }
+      expect(results).toEqual(expected);
+    });
+
+    test("Error.prepareStackTrace call sites see the same body lines", () => {
+      const previous = Error.prepareStackTrace;
+      Error.prepareStackTrace = (_, callSites) =>
+        callSites.map(site => `${site.getFileName()}:${site.getLineNumber()}`);
+      try {
+        const fn = compileFunction("return [new Error('a').stack[0], (() => new Error('b').stack[0])()];", [], {
+          filename: "cf-callsites.js",
+        });
+        expect(fn()).toEqual(["cf-callsites.js:1", "cf-callsites.js:1"]);
+        const offsetFn = compileFunction("\nreturn new Error('c').stack[0];", [], {
+          filename: "cf-callsites.js",
+          lineOffset: 10,
+        });
+        expect(offsetFn()).toBe("cf-callsites.js:12");
+      } finally {
+        Error.prepareStackTrace = previous;
+      }
+    });
+
+    test("columnOffset shifts body line 1 only", () => {
+      const filename = "cf-columns.js";
+      const statement = 'throw new Error("column")';
+      // The column JSC assigns to this statement when it starts a line that no
+      // offset applies to.
+      const { column } = thrownPosition(filename, compileFunction("\n" + statement, [], { filename }));
+      // Body line 1 shares its line with the `(function () {` wrapper, so its
+      // columns include that text; a columnOffset at least that long is
+      // applied exactly, as in Node.
+      expect(thrownPosition(filename, compileFunction(statement, [], { filename, columnOffset: 100 }))).toEqual({
+        line: 1,
+        column: column + 100,
+      });
+      expect(
+        thrownPosition(filename, () => compileFunction(statement, ["a", "b"], { filename, columnOffset: 100 })()),
+      ).toEqual({ line: 1, column: column + 100 });
+      // Later lines are never shifted.
+      expect(thrownPosition(filename, compileFunction("\n" + statement, [], { filename, columnOffset: 100 }))).toEqual({
+        line: 2,
+        column,
+      });
+      // Without a columnOffset, line 1 still reports line 1.
+      expect(thrownPosition(filename, compileFunction(statement, [], { filename })).line).toBe(1);
+    });
+
+    test("runtime arrow header shows the body line when called from a vm script", () => {
+      // Node decorates an error escaping a vm run with `<url>:<line>`, the
+      // offending source line and a caret. For a function from compileFunction
+      // the line and source text are those of the body, not of the wrapper.
+      const header = (fn: () => unknown) => {
+        let stack: string = "";
+        try {
+          fn();
+        } catch (e: any) {
+          stack = e.stack;
+        }
+        return stack.split("\n").slice(0, 4);
+      };
+      const line1 = 'throw new Error("line 1")';
+      const line2 = '0;\n  throw new Error("line 2")';
+
+      // Reference: the same statements compiled as a script, where no wrapper exists.
+      const [, , scriptCaret] = header(() => new Script(line1, { filename: "ref.js" }).runInThisContext());
+      expect(scriptCaret).toMatch(/^ *\^$/);
+      const [, , scriptCaret2] = header(() => new Script(line2, { filename: "ref.js" }).runInThisContext());
+      expect(scriptCaret2).toMatch(/^ +\^$/);
+
+      const callFromScript = (fn: Function) => () =>
+        new Script("fn()", { filename: "outer.js" }).runInNewContext({ fn });
+
+      expect(header(callFromScript(compileFunction(line1, [], { filename: "cf-header.js" })))).toEqual([
+        "cf-header.js:1",
+        line1,
+        scriptCaret,
+        "",
+      ]);
+      expect(header(callFromScript(compileFunction(line1, ["a", "b"], { filename: "cf-header.js" })))).toEqual([
+        "cf-header.js:1",
+        line1,
+        scriptCaret,
+        "",
+      ]);
+      expect(
+        header(callFromScript(compileFunction(line1, [], { filename: "cf-header.js", columnOffset: 100 }))),
+      ).toEqual(["cf-header.js:1", line1, scriptCaret, ""]);
+      expect(header(callFromScript(compileFunction(line2, [], { filename: "cf-header.js" })))).toEqual([
+        "cf-header.js:2",
+        '  throw new Error("line 2")',
+        scriptCaret2,
+        "",
+      ]);
+      expect(header(callFromScript(compileFunction(line1, [], { filename: "cf-header.js", lineOffset: 4 })))).toEqual([
+        "cf-header.js:5",
+        line1,
+        scriptCaret,
+        "",
+      ]);
+
+      // Code eval'd from inside the body gets its own source, which has no
+      // wrapper, even though it inherits the function's origin.
+      for (const evalCall of ["eval", "(0, eval)"]) {
+        const [headerLine, sourceLine, caret] = header(
+          callFromScript(compileFunction(`${evalCall}(${JSON.stringify(line1)})`, [], { filename: "cf-header.js" })),
+        );
+        expect({ evalCall, headerLine: headerLine.replace(/^.*:/, ":"), sourceLine, caret }).toEqual({
+          evalCall,
+          headerLine: ":1",
+          sourceLine: line1,
+          caret: scriptCaret,
+        });
+      }
+    });
   });
 });