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

@@ -2803,12 +2803,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.
 
 <aid="ERR_REQUIRE_CYCLE_MODULE"></a>
 

doc/api/modules.md

@@ -316,10 +316,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

@@ -1714,20 +1714,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');
 constrequireStack=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');
 constframes=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,
+});
 }
 returnmessage;
 },Error);

lib/internal/modules/esm/loader.js

@@ -291,8 +291,8 @@ class ModuleLoader {
 conststatus=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.
 thrownewERR_REQUIRE_ESM_RACE_CONDITION(filename,parentFilename,false);
 }
 letmessage=`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) {
 returnfound;
 }
 
+/**
+ * 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.
+ */
+functionfindModulesWithTopLevelAwait(root){
+constfound=[];
+constseen=newSafeSet();
+conststack=[root];
+while(stack.length>0){
+constjob=ArrayPrototypePop(stack);
+if(seen.has(job)){continue;}
+seen.add(job);
+if(job.module?.hasTopLevelAwait){
+ArrayPrototypePush(found,job.module);
+}
+letlinked=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(leti=0;i<linked.length;i++){
+ArrayPrototypePush(stack,linked[i]);
+}
+}
+}
+returnfound;
+}
+
 /**
  * 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.
  */
-functiongetTopLevelAwaitLocations(modules){
+functiongetTopLevelAwaitLocations(root){
+constmodules=findModulesWithTopLevelAwait(root);
 constlocations=[];
 for(leti=0;i<modules.length;i++){
 constmodule=modules[i];
-constsource=module.source;
+constsource=module[kModuleSource];
 if(typeofsource!=='string'){continue;}// Not retained during compilation. Skip.
 constfound=findTopLevelAwait(source);
 if(found.length===0){continue;}
-constlines=StringPrototypeSplit(source,'\n');
+constlines=RegExpPrototypeSymbolSplit(/\r?\n/,source);
 for(letj=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(){
-constfound=[];
-constseen=newSafeSet();
-conststack=[this];
-while(stack.length>0){
-constjob=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(leti=0;i<job.linked.length;i++){
-ArrayPrototypePush(stack,job.linked[i]);
-}
-}
-}
-returnfound;
-}
-
-/**
- * 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()){
-constlocations=getOptionValue('--experimental-print-required-tla') ? getTopLevelAwaitLocations(modules) : [];
-constfilename=urlToFilename(this.url);
-thrownewERR_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){
-constmodules=this.findModulesWithTopLevelAwait();
-if(modules.length>0){
-this.throwAsyncGraphError(parent,modules);
+if(!this.module.hasAsyncGraph){
+return;
 }
+constlocations=getOptionValue('--experimental-print-required-tla') ? getTopLevelAwaitLocations(this) : [];
+constfilename=urlToFilename(this.url);
+thrownewERR_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();
 constnamespace=this.module.evaluateSync();
 return{__proto__: null,module: this.module, namespace };
 }
 throwthis.module.getError();
 }elseif(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};
 }elseif(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{
 constnamespace=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.