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

src/jsc/bindings/NodeVM.cpp

@@ -183,19 +183,25 @@
     }
 
     // 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());
-
-    // 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);
+    RETURN_IF_EXCEPTION(throwScope, nullptr);
+
+    // The body starts on the wrapped program's first line (see
+    // stringifyAnonymousFunction), so the program starts at lineOffset itself
+    // and body line N is reported as lineOffset + N. Columns on body line 1 are
+    // physical columns of the wrapped line, so they already include the
+    // wrapper prefix; Node reports columnOffset + column there, so apply only
+    // the part of columnOffset that exceeds the prefix (a SourceCode cannot
+    // start at a negative column).
+    int columnOffset = position.m_column.zeroBasedInt();
+    TextPosition wrappedPosition(position.m_line, OrdinalNumber::fromZeroBasedInt(columnOffset > wrapperPrefixLength ? columnOffset - wrapperPrefixLength : 0));
+
+    // Lets handleException map a frame on the first line back to the user's
+    // source when decorating a runtime error with the offending line.
+    if (auto* fetcher = sourceOrigin.fetcher(); fetcher && fetcher->fetcherType() == ScriptFetcher::Type::NodeVM)
+        static_cast<NodeVMScriptFetcher*>(fetcher)->setWrapperPrefixLength(static_cast<unsigned>(wrapperPrefixLength));
 
     SourceCode sourceCode(
         JSC::StringSourceProvider::create(code, sourceOrigin, WTF::move(options.filename), sourceTaintOrigin, wrappedPosition, SourceProviderSourceType::Program),
@@ -380,21 +386,30 @@
     RELEASE_AND_RETURN(scope, JSPromise::resolvedPromise(globalObject, thenResult));
 }
 
-// Helper function to create an anonymous function expression with parameters
+// Helper function to create an anonymous function expression with parameters.
+//
+// The body follows `{` on the same line rather than on a line of its own: JSC
+// clamps a SourceCode's first line to 1, so a wrapper line cannot be
+// compensated for when lineOffset is 0 (the default) and every body line would
+// be reported one too high. This way body line N is physical line N of the
+// program. The price is that columns on body line 1 include the wrapper text;
+// *outOffset receives its length. The "\n" before `})` keeps a trailing `//`
+// comment in the body from swallowing the closing of the wrapper.
 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 +434,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);
@@ -574,14 +589,25 @@
         unsigned caretColumn = 0;
         if (JSC::CodeBlock* codeBlock = stack_frame.codeBlock()) {
             if (JSC::SourceProvider* provider = codeBlock->source().provider()) {
+                // The user's source starts after compileFunction's wrapper prefix
+                // (if any), which shares the first line with it; JSC's columns on
+                // that line count the prefix too.
+                unsigned wrapperPrefixLength = 0;
+                if (auto* fetcher = provider->sourceOrigin().fetcher(); fetcher && fetcher->fetcherType() == ScriptFetcher::Type::NodeVM)
+                    wrapperPrefixLength = static_cast<NodeVMScriptFetcher*>(fetcher)->wrapperPrefixLength();
+                StringView userSource = provider->source().substring(wrapperPrefixLength);
+
                 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(userSource, physicalLine);
                 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

@@ -41,6 +41,14 @@ class NodeVMScriptFetcher : public JSC::ScriptFetcher {
         });
     }
 
+    // vm.compileFunction compiles `(function (<params>) {<body>` with the body
+    // starting on the same line as the wrapper so that body line N is reported
+    // as line lineOffset + N. This is the length of that wrapper text, which
+    // precedes the user's source on the provider's first line; 0 for sources
+    // that are compiled as written (vm.Script, modules).
+    unsigned wrapperPrefixLength() const { return m_wrapperPrefixLength; }
+    void setWrapperPrefixLength(unsigned length) { m_wrapperPrefixLength = length; }
+
 private:
     JSC::Strong<JSC::Unknown> m_dynamicImportCallback;
     // m_owner is the NodeVMScript / JSFunction / module wrapper that holds this
@@ -50,6 +58,7 @@ 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;
+    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,155 @@ 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,
+        "",
+      ]);
+    });
   });
 });