esm: improve ERR_REQUIRE_ASYNC_MODULE

This brings back several improvements that were reverted by mistake when
landing https://redirect.github.com/nodejs/node/pull/64154

- Update the documentation about how the removal of side effects of
  source collection
- Add non-enumerable `requireStack` and `topLevelAwaitLocations`
  properties to `ERR_REQUIRE_ASYNC_MODULE`, the latter is only
  populated when --experimental-print-required-tla is enabled
- Add "Required module: <url>" to the error message to identify the
  required ESM entry point regardless of whether the flag is enabled
- Fix TLA caret column from 0-based to 1-based
- Store module source via a private symbol instead of a public property
- Use `hasAsyncGraph` (post-instantiation) in `throwIfAsyncGraph`
  instead of walking the graph before instantiation
- Merge the require stack checking into the
  `common.expectRequiredTLAError` helper.
- Removed tests that are made redundant by the snapshot tests

Signed-off-by: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: #64260
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>

Author: joyeecheung

doc/api/errors.md

@@ -2823,12 +2823,30 @@ A QUIC session failed because version negotiation is required.
 
 ### `ERR_REQUIRE_ASYNC_MODULE`
 
+<!-- YAML
+changes:
+  - version: REPLACEME
+    pr-url: https://github.com/nodejs/node/pull/64260
+    description: Added the `requireStack` and `topLevelAwaitLocations` properties.
+-->
+
 When trying to `require()` a [ES Module][], the module turns out to be asynchronous.
 That is, it contains top-level await.
 
-To see where the top-level await is, use
-`--experimental-print-required-tla` (this would execute the modules
-before looking for the top-level awaits).
+When uncaught, the flag `--experimental-print-required-tla` prints
+the locations of the top-level awaits in the graph to stderr.
+
+This error has the following additional non-enumerable properties:
+
+* `requireStack` {string\[]} The chain of modules that led to the failing
+  `require()`, starting with the module that required the asynchronous module.
+* `topLevelAwaitLocations` {Object\[]} The locations of the top-level awaits in
+  the graph. Only populated when `--experimental-print-required-tla` is enabled.
+  Each entry has the following properties:
+  * `url` {string} The URL of the module containing the top-level await.
+  * `line` {number} The 1-based line number of the top-level await.
+  * `column` {number} The 1-based column number of the top-level await.
+  * `sourceLine` {string} The source line containing the top-level await.
 
 <a id="ERR_REQUIRE_CYCLE_MODULE"></a>
 

doc/api/modules.md

@@ -317,10 +317,9 @@ graph it `import`s contains top-level `await`,
 [`ERR_REQUIRE_ASYNC_MODULE`][] will be thrown. In this case, users should
 load the asynchronous module using [`import()`][].
 
-If `--experimental-print-required-tla` is enabled, instead of throwing
-`ERR_REQUIRE_ASYNC_MODULE` before evaluation, Node.js will evaluate the
-module, try to locate the top-level awaits, and print their location to
-help users fix them.
+If `--experimental-print-required-tla` is enabled and the error is uncaught,
+Node.js will try to locate the top-level `await`s in the `require()`'d module graph
+and print the locations in the stderr.
 
 If support for loading ES modules using `require()` results in unexpected
 breakage, it can be disabled using `--no-require-module`.

lib/internal/errors.js

@@ -1719,20 +1719,36 @@ E('ERR_REQUIRE_ASYNC_MODULE', function(filename, parent, locations) {
   if (!getOptionValue('--experimental-print-required-tla')) {
     message += ' To see where the top-level await comes from, use --experimental-print-required-tla.';
   }
+  if (filename) {
+    message += `\nRequired module: ${filename}`;
+  }
   if (parent) {
     const { getRequireStack } = require('internal/modules/helpers');
     const requireStack = getRequireStack(parent);
     if (requireStack.length > 0) {
       message += '\nRequire stack:\n- ' +
                 ArrayPrototypeJoin(requireStack, '\n- ');
     }
-    this.requireStack = requireStack;
+    ObjectDefineProperty(this, 'requireStack', {
+      __proto__: null,
+      enumerable: false,
+      configurable: true,
+      writable: true,
+      value: requireStack,
+    });
   }
   if (locations && locations.length > 0) {
     const { urlToFilename } = require('internal/modules/helpers');
     const frames = ArrayPrototypeMap(locations, ({ url, line, column, sourceLine }) =>
-      `${urlToFilename(url)}:${line}\n\n${sourceLine}\n${StringPrototypeRepeat(' ', column)}^\n`);
+      `${urlToFilename(url)}:${line}\n\n${sourceLine}\n${StringPrototypeRepeat(' ', column - 1)}^\n`);
     setArrowMessage(this, ArrayPrototypeJoin(frames, '\n'));
+    ObjectDefineProperty(this, 'topLevelAwaitLocations', {
+      __proto__: null,
+      enumerable: false,
+      configurable: true,
+      writable: true,
+      value: locations,
+    });
   }
   return message;
 }, Error);

lib/internal/modules/esm/loader.js

@@ -291,8 +291,8 @@ class ModuleLoader {
       const status = job.module.getStatus();
       debug('Module status', job, status);
       // hasAsyncGraph is available after module been instantiated.
-      if (status >= kInstantiated && job.module.hasAsyncGraph) {
-        job.throwAsyncGraphError(parent);
+      if (status >= kInstantiated) {
+        job.throwIfAsyncGraph(parent);
       }
       if (status === kEvaluated) {
         return { wrap: job.module, namespace: job.module.getNamespace() };
@@ -320,9 +320,11 @@ class ModuleLoader {
       }
       if (status !== kEvaluating) {
         assert(status === kUninstantiated, `Unexpected module status ${status}`);
-        // A previous require() of the same graph may have bailed out before
-        // instantiation because it contains top-level await.
-        job.throwIfAsyncGraph(parent);
+        // If we get here, either there's a race where the job is still being instantiated
+        // by an in-flight import(), or the cached module previously encountered an
+        // instantiation error during a prior load (e.g. due to a mismatched import).
+        // TODO(joyeecheung): the current check is too broad. We should attempt to
+        // get the potential instantiation error and throw it.
         throw new ERR_REQUIRE_ESM_RACE_CONDITION(filename, parentFilename, false);
       }
       let message = `Cannot require() ES Module ${filename} in a cycle.`;

lib/internal/modules/esm/module_job.js

@@ -2,6 +2,7 @@
 
 const {
   Array,
+  ArrayIsArray,
   ArrayPrototypeFind,
   ArrayPrototypeJoin,
   ArrayPrototypePop,
@@ -14,6 +15,7 @@ const {
   PromiseResolve,
   RegExpPrototypeExec,
   RegExpPrototypeSymbolReplace,
+  RegExpPrototypeSymbolSplit,
   SafePromiseAllReturnArrayLike,
   SafePromiseAllReturnVoid,
   SafeSet,
@@ -37,8 +39,10 @@ const {
   kUninstantiated,
 } = internalBinding('module_wrap');
 const {
+  getPromiseDetails,
   privateSymbols: {
     entry_point_module_private_symbol,
+    module_source_private_symbol: kModuleSource,
   },
 } = internalBinding('util');
 /**
@@ -135,12 +139,12 @@ const explainCommonJSGlobalLikeNotDefinedError = (e, url, hasTopLevelAwait) => {
  * @typedef {object} TopLevelAwaitLocation
  * @property {string} url URL of the module containing the top-level await.
  * @property {number} line 1-based line number of the top-level await.
- * @property {number} column 0-based column number of the top-level await.
+ * @property {number} column 1-based column number of the top-level await.
  * @property {string} sourceLine The source line containing the top-level await.
  */
 
 /**
- * Locate the top-level awaits in the given module by parsing the source with acron.
+ * Locate the top-level awaits in the given module by parsing the source with acorn.
  * @param {string} source Module source code.
  * @returns {object[]} The acorn AST nodes of the top-level awaits, in source order.
  */
@@ -174,27 +178,62 @@ function findTopLevelAwait(source) {
   return found;
 }
 
+/**
+ * Collect the modules that contain top-level await in the linked graph of a job.
+ * @param {ModuleJobBase} root The root of the module graph to search.
+ * @returns {ModuleWrap[]} Modules that contain top-level await.
+ */
+function findModulesWithTopLevelAwait(root) {
+  const found = [];
+  const seen = new SafeSet();
+  const stack = [root];
+  while (stack.length > 0) {
+    const job = ArrayPrototypePop(stack);
+    if (seen.has(job)) { continue; }
+    seen.add(job);
+    if (job.module?.hasTopLevelAwait) {
+      ArrayPrototypePush(found, job.module);
+    }
+    let linked = job.linked;
+    if (isPromise(linked)) {
+      linked = getPromiseDetails(linked)?.[1];
+    }
+    // If `require(esm)` comes from the deprecated async loader hook worker thread,
+    // linked may be pending at this point. In that case, this branch would be skipped -
+    // we just allow lossy reporting of TLA locations in an edge case when a deprecated
+    // feature is used in combination with another experimental flag.
+    if (ArrayIsArray(linked)) {
+      for (let i = 0; i < linked.length; i++) {
+        ArrayPrototypePush(stack, linked[i]);
+      }
+    }
+  }
+  return found;
+}
+
 /**
  * Locate the top-level awaits in the given modules.
- * @param {ModuleWrap[]} modules Modules that may contain top-level await.
+ * @param {ModuleJobBase} root The root of the module graph to search.
  * @returns {TopLevelAwaitLocation[]} The locations of the top-level awaits.
  */
-function getTopLevelAwaitLocations(modules) {
+function getTopLevelAwaitLocations(root) {
+  const modules = findModulesWithTopLevelAwait(root);
   const locations = [];
   for (let i = 0; i < modules.length; i++) {
     const module = modules[i];
-    const source = module.source;
+    const source = module[kModuleSource];
     if (typeof source !== 'string') { continue; }  // Not retained during compilation. Skip.
     const found = findTopLevelAwait(source);
     if (found.length === 0) { continue; }
-    const lines = StringPrototypeSplit(source, '\n');
+    const lines = RegExpPrototypeSymbolSplit(/\r?\n/, source);
     for (let j = 0; j < found.length; j++) {
       const { start } = found[j].loc;
       ArrayPrototypePush(locations, {
         __proto__: null,
         url: module.url,
         line: start.line,
-        column: start.column,
+        // Acorn reports 0-based columns, convert them to 1-based to match `line`.
+        column: start.column + 1,
         sourceLine: lines[start.line - 1],
       });
     }
@@ -261,61 +300,18 @@ class ModuleJobBase {
   }
 
   /**
-   * Collect the modules that contain top-level await in the linked graph of
-   * this job. Whether each module contains top-level await is known at
-   * compilation, so for a synchronously linked graph this finds asynchronous
-   * graphs before instantiation.
-   * On the (deprecated) async loader hook worker thread, linking may be asynchronous, in
-   * which case the subgraphs that are not synchronously linked are skipped
-   * and callers should still consult hasAsyncGraph after instantiation.
-   * @returns {ModuleWrap[]}
-   */
-  findModulesWithTopLevelAwait() {
-    const found = [];
-    const seen = new SafeSet();
-    const stack = [this];
-    while (stack.length > 0) {
-      const job = ArrayPrototypePop(stack);
-      if (seen.has(job)) { continue; }
-      seen.add(job);
-      if (job.module?.hasTopLevelAwait) {
-        ArrayPrototypePush(found, job.module);
-      }
-      // job.linked is the array of evaluation-phase dependency jobs when the
-      // linking is synchronous. Skip it if it's still a promise.
-      if (!isPromise(job.linked)) {
-        for (let i = 0; i < job.linked.length; i++) {
-          ArrayPrototypePush(stack, job.linked[i]);
-        }
-      }
-    }
-    return found;
-  }
-
-  /**
-   * Throw the ERR_REQUIRE_ASYNC_MODULE with metadata for a require()'d graph that
-   * contains top-level await.
-   * @param {Module|undefined} parent CommonJS module that require()'d this, if any.
-   * @param {ModuleWrap[]} [modules] Modules with top-level await, when already
-   *   collected by the caller, to avoid walking the graph again.
-   */
-  throwAsyncGraphError(parent, modules = this.findModulesWithTopLevelAwait()) {
-    const locations = getOptionValue('--experimental-print-required-tla') ? getTopLevelAwaitLocations(modules) : [];
-    const filename = urlToFilename(this.url);
-    throw new ERR_REQUIRE_ASYNC_MODULE(filename, parent, locations);
-  }
-
-  /**
-   * If the a require()'d graph contains top-level await, collect the source locations
+   * If the require()'d graph contains top-level await, collect the source locations
    * of the top-level awaits using source code retained during compilation and throw
-   * ERR_REQUIRE_ASYNC_MODULE. This can be run before instantiation is complete.
+   * ERR_REQUIRE_ASYNC_MODULE. The module must be at least instantiated.
    * @param {Module|undefined} parent CommonJS module that require()'d this, if any.
    */
   throwIfAsyncGraph(parent) {
-    const modules = this.findModulesWithTopLevelAwait();
-    if (modules.length > 0) {
-      this.throwAsyncGraphError(parent, modules);
+    if (!this.module.hasAsyncGraph) {
+      return;
     }
+    const locations = getOptionValue('--experimental-print-required-tla') ? getTopLevelAwaitLocations(this) : [];
+    const filename = urlToFilename(this.url);
+    throw new ERR_REQUIRE_ASYNC_MODULE(filename, parent, locations);
   }
 
   /**
@@ -537,19 +533,15 @@ class ModuleJob extends ModuleJobBase {
       status = this.module.getStatus();
     }
     if (status === kInstantiated || status === kErrored) {
-      if (this.module.hasAsyncGraph) {
-        this.throwAsyncGraphError(parent);
-      }
+      this.throwIfAsyncGraph(parent);
       if (status === kInstantiated) {
         setHasStartedUserESMExecution();
         const namespace = this.module.evaluateSync();
         return { __proto__: null, module: this.module, namespace };
       }
       throw this.module.getError();
     } else if (status === kEvaluating || status === kEvaluated) {
-      if (this.module.hasAsyncGraph) {
-        this.throwAsyncGraphError(parent);
-      }
+      this.throwIfAsyncGraph(parent);
       // kEvaluating can show up when this is being used to deal with CJS <-> CJS cycles.
       // Allow it for now, since we only need to ban ESM <-> CJS cycles which would be
       // detected earlier during the linking phase, though the CJS handling in the ESM
@@ -645,12 +637,11 @@ class ModuleJobSync extends ModuleJobBase {
       }
       return { __proto__: null, module: this.module };
     } else if (status === kInstantiated || status === kUninstantiated) {
-      // The require() of this (synchronously linked) module bailed out: either
-      // it was rejected for containing top-level await after instantiation
-      // (kInstantiated), or its instantiation failed and left it uninstantiated
-      // (kUninstantiated, e.g. a missing named export). When it's reached via async
-      // run() from import, finish the instantiation and evaluate it asynchronously,
-      // re-throwing any instantiation error.
+      // If we get here, the module was initially required and is now being imported.
+      // The require() module failed either because the graph has TLA (kInstantiated),
+      // or instantiation failed (kUninstantiated, e.g. missing named export).
+      // Try finishing the instantiation - if it succeeds, proceed to evaluation,
+      // otherwise the branch below re-throw any instantiation error.
       if (status === kUninstantiated) {
         this.module.instantiate();
       }
@@ -676,9 +667,7 @@ class ModuleJobSync extends ModuleJobBase {
     // On the deprecated async loader hook worker thread, dependencies linked by an
     // earlier import may not be walkable synchronously, so double-check with
     // V8 now that the graph is instantiated.
-    if (this.module.hasAsyncGraph) {
-      this.throwAsyncGraphError(parent);
-    }
+    this.throwIfAsyncGraph(parent);
     setHasStartedUserESMExecution();
     try {
       const namespace = this.module.evaluateSync();

lib/internal/modules/esm/utils.js

@@ -11,6 +11,7 @@ const {
 const {
   privateSymbols: {
     host_defined_option_symbol,
+    module_source_private_symbol: kModuleSource,
   },
 } = internalBinding('util');
 const {
@@ -368,7 +369,7 @@ function compileSourceTextModule(url, source, type, context = kEmptyObject) {
   // only serves as a shortcut.
   if (wrap.hasTopLevelAwait &&
       getOptionValue('--experimental-print-required-tla')) {
-    wrap.source = source;
+    wrap[kModuleSource] = source;
   }
 
   // Cache the source map for the module if present.