module: fix sync hook short-circuit in require() in imported CJS

- For imported CJS, if it's not customized by asynchronous hooks,
  make sure it won't use the quirky re-invented require in all
  cases.
- When the imported CJS module is customized by synchronous hooks,
  in the synthetic module evalutation step, avoid calling the
  respective default step again.
- Make the branching of loadCJSModuleWithModuleLoad() and
  loadCJSModuleWithSpecialRequire() more explicit, and fold
  the tentative fs read in the 'commonjs' translator into the
  share createCJSModuleWrap() helper instead of checking it
  twice in the same path.

Signed-off-by: Joyee Cheung <joyeec9h3@gmail.com>
PR-URL: #62920
Fixes: #63060
Reviewed-By: Paolo Insogna <paolo@cowtech.it>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: Gürgün Dayıoğlu <hey@gurgun.day>

Author: joyeecheung

lib/internal/modules/cjs/loader.js

@@ -1095,20 +1095,26 @@ function defaultResolveImplForCJSLoading(specifier, parent, isMain, options) {
   return wrapResolveFilename(specifier, parent, isMain, options);
 }
 
+/**
+ * @typedef {{
+ *   resolved?: {url?: string, format?: string, filename: string},
+ *   shouldSkipModuleHooks?: boolean,
+ *   source?: string|ArrayBufferView|ArrayBuffer,
+ *   requireResolveOptions?: ResolveFilenameOptions,
+ * }} CJSModuleLoadInternalOptions
+ */
+
 /**
  * Resolve a module request for CommonJS, invoking hooks from module.registerHooks()
  * if necessary.
  * @param {string} specifier
  * @param {Module|undefined} parent
  * @param {boolean} isMain
- * @param {object} internalResolveOptions
- * @param {boolean} internalResolveOptions.shouldSkipModuleHooks Whether to skip module hooks.
- * @param {ResolveFilenameOptions} internalResolveOptions.requireResolveOptions Options from require.resolve().
- *   Only used when it comes from require.resolve().
+ * @param {CJSModuleLoadInternalOptions} internalOptions
  * @returns {{url?: string, format?: string, parentURL?: string, filename: string}}
  */
-function resolveForCJSWithHooks(specifier, parent, isMain, internalResolveOptions) {
-  const { requireResolveOptions, shouldSkipModuleHooks } = internalResolveOptions;
+function resolveForCJSWithHooks(specifier, parent, isMain, internalOptions) {
+  const { requireResolveOptions, shouldSkipModuleHooks } = internalOptions;
   const defaultResolveImpl = requireResolveOptions ?
     wrapResolveFilename : defaultResolveImplForCJSLoading;
   // Fast path: no hooks, just return simple results.
@@ -1255,10 +1261,10 @@ function loadBuiltinWithHooks(id, url, format) {
  * @param {string} request Specifier of module to load via `require`
  * @param {Module} parent Absolute path of the module importing the child
  * @param {boolean} isMain Whether the module is the main entry point
- * @param {object|undefined} internalResolveOptions Additional options for loading the module
+ * @param {CJSModuleLoadInternalOptions|undefined} internalOptions Additional options for loading the module
  * @returns {object}
  */
-Module._load = function(request, parent, isMain, internalResolveOptions = kEmptyObject) {
+Module._load = function(request, parent, isMain, internalOptions = kEmptyObject) {
   let relResolveCacheIdentifier;
   if (parent) {
     debug('Module._load REQUEST %s parent: %s', request, parent.id);
@@ -1282,7 +1288,10 @@ Module._load = function(request, parent, isMain, internalResolveOptions = kEmpty
     }
   }
 
-  const resolveResult = resolveForCJSWithHooks(request, parent, isMain, internalResolveOptions);
+  // If the module has been resolved by a short-circuiting synchronous resolve hook,
+  // avoid running the default resolution from disk again.
+  const resolveResult = internalOptions.resolved ??
+    resolveForCJSWithHooks(request, parent, isMain, internalOptions);
   let { format } = resolveResult;
   const { url, filename } = resolveResult;
 
@@ -1370,6 +1379,14 @@ Module._load = function(request, parent, isMain, internalResolveOptions = kEmpty
     module[kLastModuleParent] = parent;
   }
 
+  // The module source was provided by a short-circuiting synchronous hook,
+  // assign them into the module to avoid triggering the default load step again.
+  if (internalOptions.source !== undefined) {
+    module[kModuleSource] ??= internalOptions.source;
+    module[kURL] ??= url;
+    module[kFormat] ??= format;
+  }
+
   if (parent !== undefined) {
     relativeResolveCache[relResolveCacheIdentifier] = filename;
   }

lib/internal/modules/esm/load.js

@@ -145,7 +145,6 @@ function defaultLoadSync(url, context = kEmptyObject) {
 
   throwIfUnsupportedURLScheme(urlInstance, false);
 
-  let shouldBeReloadedByCJSLoader = false;
   if (urlInstance.protocol === 'node:') {
     source = null;
     format ??= 'builtin';
@@ -160,10 +159,6 @@ function defaultLoadSync(url, context = kEmptyObject) {
 
     // Now that we have the source for the module, run `defaultGetFormat` to detect its format.
     format ??= defaultGetFormat(urlInstance, context);
-
-    // For backward compatibility reasons, we need to let go through Module._load
-    // again.
-    shouldBeReloadedByCJSLoader = (format === 'commonjs');
   }
   validateAttributes(url, format, importAttributes);
 
@@ -172,7 +167,6 @@ function defaultLoadSync(url, context = kEmptyObject) {
     format,
     responseURL,
     source,
-    shouldBeReloadedByCJSLoader,
   };
 }
 

lib/internal/modules/esm/loader.js

@@ -120,7 +120,19 @@ const { defaultLoadSync, throwUnknownModuleFormat } = require('internal/modules/
  */
 
 /**
- * @typedef {{ format: ModuleFormat, source: ModuleSource, translatorKey: string }} TranslateContext
+ * @typedef {{format: string, url: string, isResolvedBySyncHooks: boolean}} ResolveResult
+ */
+
+/**
+ * @typedef {{
+ *   url: string,
+ *   format: ModuleFormat,
+ *   source: ModuleSource,
+ *   responseURL?: string,
+ *   translatorKey: string,
+ *   isResolvedBySyncHooks: boolean,
+ *   isSourceLoadedSynchronously: boolean,
+ * }} TranslateContext
  */
 
 /**
@@ -382,19 +394,25 @@ class ModuleLoader {
   /**
    * Load a module and translate it into a ModuleWrap for require(esm).
    * This is run synchronously, and the translator always return a ModuleWrap synchronously.
-   * @param {string} url URL of the module to be translated.
+   * @param {ResolveResult} resolveResult Result from the resolve step.
    * @param {object} loadContext See {@link load}
    * @param {string|undefined} parentURL URL of the parent module. Undefined if it's the entry point.
    * @param {ModuleRequest} request Module request.
    * @returns {ModuleWrap}
    */
-  loadAndTranslateForImportInRequiredESM(url, loadContext, parentURL, request) {
+  loadAndTranslateForImportInRequiredESM(resolveResult, loadContext, parentURL, request) {
+    const { url } = resolveResult;
     const loadResult = this.#loadSync(url, loadContext);
     // Use the synchronous commonjs translator which can deal with cycles.
     const formatFromLoad = loadResult.format;
     const translatorKey = (formatFromLoad === 'commonjs' || formatFromLoad === 'commonjs-typescript') ?
       'commonjs-sync' : formatFromLoad;
-    const translateContext = { ...loadResult, translatorKey, __proto__: null };
+    const translateContext = {
+      ...resolveResult,
+      ...loadResult,
+      translatorKey,
+      __proto__: null,
+    };
     const wrap = this.#translate(url, translateContext, parentURL);
     assert(wrap instanceof ModuleWrap, `Translator used for require(${url}) should not be async`);
 
@@ -443,12 +461,13 @@ class ModuleLoader {
   /**
    * Load a module and translate it into a ModuleWrap for require() in imported CJS.
    * This is run synchronously, and the translator always return a ModuleWrap synchronously.
-   * @param {string} url URL of the module to be translated.
+   * @param {ResolveResult} resolveResult Result from the resolve step.
    * @param {object} loadContext See {@link load}
    * @param {string|undefined} parentURL URL of the parent module. Undefined if it's the entry point.
    * @returns {ModuleWrap}
    */
-  loadAndTranslateForRequireInImportedCJS(url, loadContext, parentURL) {
+  loadAndTranslateForRequireInImportedCJS(resolveResult, loadContext, parentURL) {
+    const { url } = resolveResult;
     const loadResult = this.#loadSync(url, loadContext);
     const formatFromLoad = loadResult.format;
 
@@ -470,7 +489,12 @@ class ModuleLoader {
       translatorKey = 'require-commonjs-typescript';
     }
 
-    const translateContext = { ...loadResult, translatorKey, __proto__: null };
+    const translateContext = {
+      ...resolveResult,
+      ...loadResult,
+      translatorKey,
+      __proto__: null,
+    };
     const wrap = this.#translate(url, translateContext, parentURL);
     assert(wrap instanceof ModuleWrap, `Translator used for require(${url}) should not be async`);
     return wrap;
@@ -479,15 +503,21 @@ class ModuleLoader {
   /**
    * Load a module and translate it into a ModuleWrap for ordinary imported ESM.
    * This may be run asynchronously if there are asynchronous module loader hooks registered.
-   * @param {string} url URL of the module to be translated.
+   * @param {ResolveResult} resolveResult Result from the resolve step.
    * @param {object} loadContext See {@link load}
    * @param {string|undefined} parentURL URL of the parent module. Undefined if it's the entry point.
    * @returns {Promise<ModuleWrap>|ModuleWrap}
    */
-  loadAndTranslate(url, loadContext, parentURL) {
+  loadAndTranslate(resolveResult, loadContext, parentURL) {
+