esm: add line and column

Refactor import trace output.
Add line and column information.
Remove unnecessary duplicates.

Update the tests.
Add line and column info.
Add test for module with multiple parents.

Refs: #46992 (comment)
Fixes: #61612 (comment)
Fixes: #61612 (comment)

Author: mattskel

lib/internal/modules/esm/module_job.js

@@ -42,14 +42,28 @@ function buildImportTrace(importParents, startURL) {
   const seen = new SafeSet([current]);
 
   while (true) {
-    const parent = importParents.get(current);
-    if (!parent || seen.has(parent)) {
+    const parentInfo = importParents.get(current);
+    let parentURL;
+    if (typeof parentInfo === 'object' && parentInfo !== null && typeof parentInfo.url === 'string') {
+      parentURL = parentInfo.url;
+    } else {
+      parentURL = parentInfo;
+    }
+    if (!parentInfo || seen.has(parentURL)) {
       break;
     }
-
-    trace.push({ child: current, parent });
-    seen.add(current);
-    current = parent;
+    let parentDisplay;
+    if (typeof parentInfo === 'object' && parentInfo !== null && typeof parentInfo.url === 'string') {
+      parentDisplay = parentInfo.url;
+      if (typeof parentInfo.line === 'number' && typeof parentInfo.column === 'number') {
+        parentDisplay += `:${parentInfo.line + 1}:${parentInfo.column + 1}`;
+      }
+    } else {
+      parentDisplay = parentInfo;
+    }
+    trace.push({ child: current, parent: parentDisplay });
+    seen.add(parentURL);
+    current = parentURL;
   }
 
   return trace.length ? trace : null;
@@ -61,7 +75,7 @@ function buildImportTrace(importParents, startURL) {
  */
 function formatImportTrace(trace) {
   return trace
-    .map(({ child, parent }) => `  ${child} imported by ${parent}`)
+    .map(({ child, parent }) => `    ${parent}`)
     .join('\n');
 }
 
@@ -93,7 +107,7 @@ function decorateErrorWithImportTrace(e, importParents) {
       return stack;
     }
 
-    return `${stack}\n\nImport trace:\n${formatImportTrace(importTrace)}`;
+    return `${stack}\n\nImported by:\n${formatImportTrace(importTrace)}`;
   });
 }
 
@@ -230,7 +244,16 @@ class ModuleJobBase {
         // that hooks can pre-fetch sources off-thread.
         const job = this.loader.getOrCreateModuleJob(this.url, request, requestType);
         debug(`ModuleJobBase.syncLink() ${this.url} -> ${request.specifier}`, job);
-        this.loader.importParents.set(job.url, this.url);
+
+        const line = request.line;
+        const column = request.column;
+        // console.log(`Import at ${request.specifier} (line: ${line}, column: ${column})`);
+        // Set the parent info with line/column
+        let parentInfo = this.url;
+        if (typeof line === 'number' && typeof column === 'number') {
+          parentInfo = { url: this.url, line, column };
+        }
+        this.loader.importParents.set(job.url, parentInfo);
         assert(!isPromise(job));
         assert(job.module instanceof ModuleWrap);
         if (request.phase === kEvaluationPhase) {

src/env_properties.h

@@ -100,6 +100,7 @@
     "transferList")                                                            \
   V(clone_untransferable_str, "Found invalid value in transferList.")          \
   V(code_string, "code")                                                       \
+  V(column_string, "column")                                                   \
   V(config_string, "config")                                                   \
   V(constants_string, "constants")                                             \
   V(crypto_dh_string, "dh")                                                    \
@@ -239,6 +240,7 @@
   V(length_string, "length")                                                   \
   V(limits_string, "limits")                                                   \
   V(library_string, "library")                                                 \
+  V(line_string, "line")                                                       \
   V(loop_count, "loopCount")                                                   \
   V(max_buffer_string, "maxBuffer")                                            \
   V(max_concurrent_streams_string, "maxConcurrentStreams")                     \

src/module_wrap.cc

@@ -40,6 +40,7 @@ using v8::Isolate;
 using v8::Just;
 using v8::JustVoid;
 using v8::Local;
+using v8::Location;
 using v8::LocalVector;
 using v8::Maybe;
 using v8::MaybeLocal;
@@ -565,7 +566,7 @@ static Local<Object> createImportAttributesContainer(
 }
 
 static Local<Array> createModuleRequestsContainer(
-    Realm* realm, Isolate* isolate, Local<FixedArray> raw_requests) {
+    Realm* realm, Isolate* isolate, Local<Module> module, Local<FixedArray> raw_requests) {
   EscapableHandleScope scope(isolate);
   Local<Context> context = realm->context();
   LocalVector<Value> requests(isolate, raw_requests->Length());
@@ -584,15 +585,22 @@ static Local<Array> createModuleRequestsContainer(
         createImportAttributesContainer(realm, isolate, raw_attributes, 3);
     ModuleImportPhase phase = module_request->GetPhase();
 
+    int source_offset = module_request->GetSourceOffset();
+    Location loc = module->SourceOffsetToLocation(source_offset);
+
     Local<Name> names[] = {
         realm->isolate_data()->specifier_string(),
         realm->isolate_data()->attributes_string(),
         realm->isolate_data()->phase_string(),
+        realm->isolate_data()->line_string(),
+        realm->isolate_data()->column_string(),
     };
     Local<Value> values[] = {
         specifier,
         attributes,
         Integer::New(isolate, to_phase_constant(phase)),
+        Integer::New(isolate, loc.GetLineNumber()),
+        Integer::New(isolate, loc.GetColumnNumber()),
     };
     DCHECK_EQ(arraysize(names), arraysize(values));
 
@@ -616,7 +624,7 @@ void ModuleWrap::GetModuleRequests(const FunctionCallbackInfo<Value>& args) {
 
   Local<Module> module = obj->module_.Get(isolate);
   args.GetReturnValue().Set(createModuleRequestsContainer(
-      realm, isolate, module->GetModuleRequests()));
+      realm, isolate, module, module->GetModuleRequests()));
 }
 
 // moduleWrap.link(moduleWraps)

test/es-module/test-esm-import-trace.mjs

@@ -16,9 +16,9 @@ test('includes import trace for evaluation-time errors', () => {
   );
 
   assert.notStrictEqual(result.status, 0);
-  assert.match(result.stderr, /Import trace:/);
-  assert.match(result.stderr, /bar\.mjs imported by .*foo\.mjs/);
-  assert.match(result.stderr, /foo\.mjs imported by .*entry\.mjs/);
+  assert.match(result.stderr, /Imported by:/);
+  assert.match(result.stderr, /.*foo\.mjs:1:8/);
+  assert.match(result.stderr, /.*entry\.mjs:1:8/);
 });
 
 const multiFixture = path.join(
@@ -34,18 +34,21 @@ test('import trace matches actual code path for multiple parents', () => {
   );
 
   assert.notStrictEqual(result.status, 0);
-  // Should show the actual import chain that led to the error
-  const traceFoo = /bar\.mjs imported by .*foo\.mjs/;
-  const traceAlt = /bar\.mjs imported by .*alt\.mjs/;
-  const entryFoo = /foo\.mjs imported by .*entry\.mjs/;
-  const entryAlt = /alt\.mjs imported by .*entry\.mjs/;
+  const traceFoo = /.*foo\.mjs:1:8/;
+  const traceAlt = /.*alt\.mjs:1:8/;
+  const entryFoo = /.*entry\.mjs:1:8/;
+  const entryAlt = /.*entry\.mjs:2:8/;
 
-  assert(
-    traceFoo.test(result.stderr) || traceAlt.test(result.stderr),
-    'Import trace should show either foo.mjs or alt.mjs as parent'
-  );
-  assert(
-    entryFoo.test(result.stderr) || entryAlt.test(result.stderr),
-    'Import trace should show either foo.mjs or alt.mjs imported by entry.mjs'
-  );
+  let parentMatched;
+  if (traceFoo.test(result.stderr)) {
+    parentMatched = 'foo';
+    assert(entryFoo.test(result.stderr),
+      'Import trace should show foo.mjs imported by entry.mjs with line/column');
+  } else if (traceAlt.test(result.stderr)) {
+    parentMatched = 'alt';
+    assert(entryAlt.test(result.stderr),
+      'Import trace should show alt.mjs imported by entry.mjs with line/column');
+  } else {
+    assert.fail('Import trace should show either foo.mjs or alt.mjs as parent with line/column');
+  }
 });