vm: refactor SourceTextModule

- Removes redundant `instantiate` method
- Refactors `link` to match the spec linking steps more accurately
- Removes URL validation from SourceTextModule specifiers
- DRYs some dynamic import logic

Closes: https://github.com/nodejs/node/issues/29030

Co-Authored-By: Michaël Zasso <targos@protonmail.com>

PR-URL: https://github.com/nodejs/node/pull/29776
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Minwoo Jung <minwoo@nodesource.com>

4 files changed, 179 insertions, 131 deletions

diff --git a/lib/internal/errors.js b/lib/internal/errors.js
index 53f448cf2f..ab874cd533 100644
--- a/lib/internal/errors.js
+++ b/lib/internal/errors.js
@@ -1211,8 +1211,6 @@ E('ERR_VM_MODULE_DIFFERENT_CONTEXT',
   'Linked modules must use the same context', Error);
 E('ERR_VM_MODULE_LINKING_ERRORED',
   'Linking has already failed for the provided module', Error);
-E('ERR_VM_MODULE_NOT_LINKED',
-  'Module must be linked before it can be instantiated', Error);
 E('ERR_VM_MODULE_NOT_MODULE',
   'Provided module is not an instance of Module', Error);
 E('ERR_VM_MODULE_STATUS', 'Module status %s', Error);
diff --git a/lib/internal/modules/esm/loader.js b/lib/internal/modules/esm/loader.js
index 09109d3c71..9800e8a550 100644
--- a/lib/internal/modules/esm/loader.js
+++ b/lib/internal/modules/esm/loader.js
@@ -127,7 +127,7 @@ class Loader {
     this.moduleMap.set(url, job);
     const { module, result } = await job.run();
     return {
-      namespace: module.namespace(),
+      namespace: module.getNamespace(),
       result
     };
   }
@@ -135,7 +135,7 @@ class Loader {
   async import(specifier, parent) {
     const job = await this.getModuleJob(specifier, parent);
     const { module } = await job.run();
-    return module.namespace();
+    return module.getNamespace();
   }
 
   hook({ resolve, dynamicInstantiate }) {
diff --git a/lib/internal/vm/source_text_module.js b/lib/internal/vm/source_text_module.js
index 881c298ce5..61af2e7a29 100644
--- a/lib/internal/vm/source_text_module.js
+++ b/lib/internal/vm/source_text_module.js
@@ -3,14 +3,12 @@
 const { Object, SafePromise } = primordials;
 
 const { isModuleNamespaceObject } = require('internal/util/types');
-const { URL } = require('internal/url');
 const { isContext } = internalBinding('contextify');
 const {
   ERR_INVALID_ARG_TYPE,
   ERR_VM_MODULE_ALREADY_LINKED,
   ERR_VM_MODULE_DIFFERENT_CONTEXT,
   ERR_VM_MODULE_LINKING_ERRORED,
-  ERR_VM_MODULE_NOT_LINKED,
   ERR_VM_MODULE_NOT_MODULE,
   ERR_VM_MODULE_STATUS,
 } = require('internal/errors').codes;
@@ -22,7 +20,7 @@ const {
 const {
   validateInt32,
   validateUint32,
-  validateString
+  validateString,
 } = require('internal/validators');
 
 const binding = internalBinding('module_wrap');
@@ -37,29 +35,33 @@ const {
 } = binding;
 
 const STATUS_MAP = {
-  [kUninstantiated]: 'uninstantiated',
-  [kInstantiating]: 'instantiating',
-  [kInstantiated]: 'instantiated',
+  [kUninstantiated]: 'unlinked',
+  [kInstantiating]: 'linking',
+  [kInstantiated]: 'linked',
   [kEvaluating]: 'evaluating',
   [kEvaluated]: 'evaluated',
   [kErrored]: 'errored',
 };
 
 let globalModuleId = 0;
+const defaultModuleName = 'vm:module';
 const perContextModuleId = new WeakMap();
-const wrapMap = new WeakMap();
-const dependencyCacheMap = new WeakMap();
-const linkingStatusMap = new WeakMap();
-// ModuleWrap -> vm.SourceTextModule
 const wrapToModuleMap = new WeakMap();
-const defaultModuleName = 'vm:module';
 
-// TODO(devsnek): figure out AbstractModule class or protocol
+const kNoError = Symbol('kNoError');
+
 class SourceTextModule {
-  constructor(src, options = {}) {
+  #wrap;
+  #identifier;
+  #context;
+  #dependencySpecifiers;
+  #statusOverride;
+  #error = kNoError;
+
+  constructor(source, options = {}) {
     emitExperimentalWarning('vm.SourceTextModule');
 
-    validateString(src, 'src');
+    validateString(source, 'source');
     if (typeof options !== 'object' || options === null)
       throw new ERR_INVALID_ARG_TYPE('options', 'Object', options);
 
@@ -81,21 +83,6 @@ class SourceTextModule {
       }
     }
 
-    let { url } = options;
-    if (url !== undefined) {
-      validateString(url, 'options.url');
-      url = new URL(url).href;
-    } else if (context === undefined) {
-      url = `${defaultModuleName}(${globalModuleId++})`;
-    } else if (perContextModuleId.has(context)) {
-      const curId = perContextModuleId.get(context);
-      url = `${defaultModuleName}(${curId})`;
-      perContextModuleId.set(context, curId + 1);
-    } else {
-      url = `${defaultModuleName}(0)`;
-      perContextModuleId.set(context, 1);
-    }
-
     validateInt32(lineOffset, 'options.lineOffset');
     validateInt32(columnOffset, 'options.columnOffset');
 
@@ -111,114 +98,167 @@ class SourceTextModule {
         'options.importModuleDynamically', 'function', importModuleDynamically);
     }
 
-    const wrap = new ModuleWrap(src, url, context, lineOffset, columnOffset);
-    wrapMap.set(this, wrap);
-    linkingStatusMap.set(this, 'unlinked');
-    wrapToModuleMap.set(wrap, this);
+    let { identifier } = options;
+    if (identifier !== undefined) {
+      validateString(identifier, 'options.identifier');
+    } else if (context === undefined) {
+      identifier = `${defaultModuleName}(${globalModuleId++})`;
+    } else if (perContextModuleId.has(context)) {
+      const curId = perContextModuleId.get(context);
+      identifier = `${defaultModuleName}(${curId})`;
+      perContextModuleId.set(context, curId + 1);
+    } else {
+      identifier = `${defaultModuleName}(0)`;
+      perContextModuleId.set(context, 1);
+    }
 
-    binding.callbackMap.set(wrap, {
+    this.#wrap = new ModuleWrap(
+      source, identifier, context,
+      lineOffset, columnOffset,
+    );
+    wrapToModuleMap.set(this.#wrap, this);
+    this.#identifier = identifier;
+    this.#context = context;
+
+    binding.callbackMap.set(this.#wrap, {
       initializeImportMeta,
-      importModuleDynamically: importModuleDynamically ? async (...args) => {
-        const m = await importModuleDynamically(...args);
-        if (isModuleNamespaceObject(m)) {
-          return m;
-        }
-        if (!m || !wrapMap.has(m))
-          throw new ERR_VM_MODULE_NOT_MODULE();
-        const childLinkingStatus = linkingStatusMap.get(m);
-        if (childLinkingStatus === 'errored')
-          throw m.error;
-        return m.namespace;
-      } : undefined,
+      importModuleDynamically: importModuleDynamically ?
+        importModuleDynamicallyWrap(importModuleDynamically) :
+        undefined,
     });
+  }
 
-    Object.defineProperties(this, {
-      url: { value: url, enumerable: true },
-      context: { value: context, enumerable: true },
-    });
+  get status() {
+    try {
+      this.#error;
+    } catch {
+      throw new ERR_VM_MODULE_NOT_MODULE();
+    }
+    if (this.#error !== kNoError) {
+      return 'errored';
+    }
+    if (this.#statusOverride) {
+      return this.#statusOverride;
+    }
+    return STATUS_MAP[this.#wrap.getStatus()];
   }
 
-  get linkingStatus() {
-    return linkingStatusMap.get(this);
+  get identifier() {
+    try {
+      return this.#identifier;
+    } catch {
+      throw new ERR_VM_MODULE_NOT_MODULE();
+    }
   }
 
-  get status() {
-    return STATUS_MAP[wrapMap.get(this).getStatus()];
+  get context() {
+    try {
+      return this.#context;
+    } catch {
+      throw new ERR_VM_MODULE_NOT_MODULE();
+    }
   }
 
   get namespace() {
-    const wrap = wrapMap.get(this);
-    if (wrap.getStatus() < kInstantiated)
-      throw new ERR_VM_MODULE_STATUS(
-        'must not be uninstantiated or instantiating'
-      );
-    return wrap.namespace();
+    try {
+      this.#wrap;
+    } catch {
+      throw new ERR_VM_MODULE_NOT_MODULE();
+    }
+    if (this.#wrap.getStatus() < kInstantiated) {
+      throw new ERR_VM_MODULE_STATUS('must not be unlinked or linking');
+    }
+    return this.#wrap.getNamespace();
   }
 
   get dependencySpecifiers() {
-    let deps = dependencyCacheMap.get(this);
-    if (deps !== undefined)
-      return deps;
-
-    deps = wrapMap.get(this).getStaticDependencySpecifiers();
-    Object.freeze(deps);
-    dependencyCacheMap.set(this, deps);
-    return deps;
+    try {
+      this.#dependencySpecifiers;
+    } catch {
+      throw new ERR_VM_MODULE_NOT_MODULE();
+    }
+    if (this.#dependencySpecifiers === undefined) {
+      this.#dependencySpecifiers = this.#wrap.getStaticDependencySpecifiers();
+    }
+    return this.#dependencySpecifiers;
   }
 
   get error() {
-    const wrap = wrapMap.get(this);
-    if (wrap.getStatus() !== kErrored)
+    try {
+      this.#error;
+    } catch {
+      throw new ERR_VM_MODULE_NOT_MODULE();
+    }
+    if (this.#error !== kNoError) {
+      return this.#error;
+    }
+    if (this.#wrap.getStatus() !== kErrored) {
       throw new ERR_VM_MODULE_STATUS('must be errored');
-    return wrap.getError();
+    }
+    return this.#wrap.getError();
   }
 
   async link(linker) {
-    if (typeof linker !== 'function')
+    try {
+      this.#link;
+    } catch {
+      throw new ERR_VM_MODULE_NOT_MODULE();
+    }
+
+    if (typeof linker !== 'function') {
       throw new ERR_INVALID_ARG_TYPE('linker', 'function', linker);
-    if (linkingStatusMap.get(this) !== 'unlinked')
+    }
+    if (this.status !== 'unlinked') {
       throw new ERR_VM_MODULE_ALREADY_LINKED();
-    const wrap = wrapMap.get(this);
-    if (wrap.getStatus() !== kUninstantiated)
-      throw new ERR_VM_MODULE_STATUS('must be uninstantiated');
+    }
 
-    linkingStatusMap.set(this, 'linking');
+    await this.#link(linker);
 
-    const promises = wrap.link(async (specifier) => {
-      const m = await linker(specifier, this);
-      if (!m || !wrapMap.has(m))
+    this.#wrap.instantiate();
+  }
+
+  #link = async function(linker) {
+    this.#statusOverride = 'linking';
+
+    const promises = this.#wrap.link(async (identifier) => {
+      const module = await linker(identifier, this);
+      try {
+        module.#wrap;
+      } catch {
         throw new ERR_VM_MODULE_NOT_MODULE();
-      if (m.context !== this.context)
+      }
+      if (module.context !== this.context) {
         throw new ERR_VM_MODULE_DIFFERENT_CONTEXT();
-      const childLinkingStatus = linkingStatusMap.get(m);
-      if (childLinkingStatus === 'errored')
+      }
+      if (module.status === 'errored') {
         throw new ERR_VM_MODULE_LINKING_ERRORED();
-      if (childLinkingStatus === 'unlinked')
-        await m.link(linker);
-      return wrapMap.get(m);
+      }
+      if (module.status === 'unlinked') {
+        await module.#link(linker);
+      }
+      return module.#wrap;
     });
 
     try {
-      if (promises !== undefined)
+      if (promises !== undefined) {
         await SafePromise.all(promises);
-      linkingStatusMap.set(this, 'linked');
-    } catch (err) {
-      linkingStatusMap.set(this, 'errored');
-      throw err;
+      }
+    } catch (e) {
+      this.#error = e;
+      throw e;
+    } finally {
+      this.#statusOverride = undefined;
     }
-  }
+  };
 
-  instantiate() {
-    const wrap = wrapMap.get(this);
-    const status = wrap.getStatus();
-    if (status === kInstantiating || status === kEvaluating)
-      throw new ERR_VM_MODULE_STATUS('must not be instantiating or evaluating');
-    if (linkingStatusMap.get(this) !== 'linked')
-      throw new ERR_VM_MODULE_NOT_LINKED();
-    wrap.instantiate();
-  }
 
   async evaluate(options = {}) {
+    try {
+      this.#wrap;
+    } catch {
+      throw new ERR_VM_MODULE_NOT_MODULE();
+    }
+
     if (typeof options !== 'object' || options === null) {
       throw new ERR_INVALID_ARG_TYPE('options', 'Object', options);
     }
@@ -229,24 +269,41 @@ class SourceTextModule {
     } else {
       validateUint32(timeout, 'options.timeout', true);
     }
-
     const { breakOnSigint = false } = options;
     if (typeof breakOnSigint !== 'boolean') {
       throw new ERR_INVALID_ARG_TYPE('options.breakOnSigint', 'boolean',
                                      breakOnSigint);
     }
-
-    const wrap = wrapMap.get(this);
-    const status = wrap.getStatus();
+    const status = this.#wrap.getStatus();
     if (status !== kInstantiated &&
         status !== kEvaluated &&
         status !== kErrored) {
       throw new ERR_VM_MODULE_STATUS(
-        'must be one of instantiated, evaluated, and errored'
+        'must be one of linked, evaluated, or errored'
       );
     }
-    const result = wrap.evaluate(timeout, breakOnSigint);
-    return { result, __proto__: null };
+    const result = this.#wrap.evaluate(timeout, breakOnSigint);
+    return { __proto__: null, result };
+  }
+
+  static importModuleDynamicallyWrap(importModuleDynamically) {
+    // Named declaration for function name
+    const importModuleDynamicallyWrapper = async (...args) => {
+      const m = await importModuleDynamically(...args);
+      if (isModuleNamespaceObject(m)) {
+        return m;
+      }
+      try {
+        m.#wrap;
+      } catch {
+        throw new ERR_VM_MODULE_NOT_MODULE();
+      }
+      if (m.status === 'errored') {
+        throw m.error;
+      }
+      return m.namespace;
+    };
+    return importModuleDynamicallyWrapper;
   }
 
   [customInspectSymbol](depth, options) {
@@ -258,16 +315,19 @@ class SourceTextModule {
 
     const o = Object.create({ constructor: ctor });
     o.status = this.status;
-    o.linkingStatus = this.linkingStatus;
-    o.url = this.url;
+    o.identifier = this.identifier;
     o.context = this.context;
     return require('internal/util/inspect').inspect(o, options);
   }
 }
 
+// Declared as static to allow access to #wrap
+const importModuleDynamicallyWrap =
+  SourceTextModule.importModuleDynamicallyWrap;
+delete SourceTextModule.importModuleDynamicallyWrap;
+
 module.exports = {
   SourceTextModule,
   wrapToModuleMap,
-  wrapMap,
-  linkingStatusMap,
+  importModuleDynamicallyWrap,
 };
diff --git a/lib/vm.js b/lib/vm.js
index ec6614e661..90f332c775 100644
--- a/lib/vm.js
+++ b/lib/vm.js
@@ -31,10 +31,8 @@ const {
 } = internalBinding('contextify');
 const {
   ERR_INVALID_ARG_TYPE,
-  ERR_VM_MODULE_NOT_MODULE,
 } = require('internal/errors').codes;
 const {
-  isModuleNamespaceObject,
   isArrayBufferView,
 } = require('internal/util/types');
 const {
@@ -100,21 +98,13 @@ class Script extends ContextifyScript {
                                        'function',
                                        importModuleDynamically);
       }
-      const { wrapMap, linkingStatusMap } =
+      const { importModuleDynamicallyWrap } =
         require('internal/vm/source_text_module');
       const { callbackMap } = internalBinding('module_wrap');
-      callbackMap.set(this, { importModuleDynamically: async (...args) => {
-        const m = await importModuleDynamically(...args);
-        if (isModuleNamespaceObject(m)) {
-          return m;
-        }
-        if (!m || !wrapMap.has(m))
-          throw new ERR_VM_MODULE_NOT_MODULE();
-        const childLinkingStatus = linkingStatusMap.get(m);
-        if (childLinkingStatus === 'errored')
-          throw m.error;
-        return m.namespace;
-      } });
+      callbackMap.set(this, {
+        importModuleDynamically:
+          importModuleDynamicallyWrap(importModuleDynamically),
+      });
     }
   }