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

src/jsc/bindings/NodeVM.cpp

@@ -183,23 +183,29 @@ JSC::JSFunction* constructAnonymousFunction(JSC::JSGlobalObject* globalObject, c
     }
 
     // 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);
+    // 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));
 
-    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);
+
+    // 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)->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 +386,30 @@ static JSPromise* importModuleInner(JSGlobalObject* globalObject, JSString* modu
     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 @@ String stringifyAnonymousFunction(JSGlobalObject* globalObject, const ArgList& a
         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 +505,9 @@ JSC::EncodedJSValue createCachedData(JSGlobalObject* globalObject, const JSC::So
 // 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 is the length of compileFunction's wrapper, which shares the
+// first line with the user's source and is not part of it.
+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 +522,8 @@ static String nthSourceLineForArrowHeader(StringView source, int64_t physicalLin
     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 +593,25 @@ bool handleException(JSGlobalObject* globalObject, VM& vm, NakedPtr<JSC::Excepti
         unsigned caretColumn = 0;
         if (JSC::CodeBlock* codeBlock = stack_frame.codeBlock()) {
             if (JSC::SourceProvider* provider = codeBlock->source().provider()) {
+                // In a compileFunction program the user's first line starts after
+                // the wrapper prefix, and JSC's columns on that line count the
+                // prefix too. Zero for every other provider, including eval() code
+                // from inside such a function, which shares the fetcher.
+                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,23 @@ 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. Records the provider holding that program and the
+    // length of the wrapper text preceding the user's source on its first line.
+    // Keyed by provider because eval() and new Function() inside the body create
+    // providers that inherit this fetcher through the SourceOrigin but contain
+    // no wrapper; for those, and for vm.Script sources, this returns 0.
+    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 +68,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(