vm: make vm.Module.evaluate() conditionally synchronous

- Make sure that the vm.Module.evaluate() method is conditionally
  synchronous based on the specification. Previously, the returned
  promise is unconditionally pending (even for synthetic modules and
  source text modules without top-level await) instead of immediately
  fulfilled, making it harder to debug as it deviates from the
  specified behavior.
- Clarify the synchronicity of this method in the documentation
- Add more tests for the synchronicity of this method.

PR-URL: https://github.com/nodejs/node/pull/60205
Refs: https://github.com/nodejs/node/issues/59656
Refs: https://github.com/nodejs/node/issues/37648
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>

doc/api/vm.md

@@ -618,19 +618,47 @@ in the ECMAScript specification.
     work after that. **Default:** `false`.
 * Returns: {Promise} Fulfills with `undefined` upon success.
 
-Evaluate the module.
+Evaluate the module and its depenendencies. Corresponds to the [Evaluate() concrete method][] field of
+[Cyclic Module Record][]s in the ECMAScript specification.
 
-This must be called after the module has been linked; otherwise it will reject.
-It could be called also when the module has already been evaluated, in which
-case it will either do nothing if the initial evaluation ended in success
-(`module.status` is `'evaluated'`) or it will re-throw the exception that the
-initial evaluation resulted in (`module.status` is `'errored'`).
+If the module is a `vm.SourceTextModule`, `evaluate()` must be called after the module has been instantiated;
+otherwise `evaluate()` will return a rejected promise.
 
-This method cannot be called while the module is being evaluated
-(`module.status` is `'evaluating'`).
+For a `vm.SourceTextModule`, the promise returned by `evaluate()` may be fulfilled either
+synchronously or asynchronously:
 
-Corresponds to the [Evaluate() concrete method][] field of [Cyclic Module
-Record][]s in the ECMAScript specification.
+1. If the `vm.SourceTextModule` has no top-level `await` in itself or any of its dependencies, the promise will be
+   fulfilled _synchronously_ after the module and all its dependencies have been evaluated.
+   1. If the evaluation succeeds, the promise will be _synchronously_ resolved to `undefined`.
+   2. If the evaluation results in an exception, the promise will be _synchronously_ rejected with the exception
+      that causes the evaluation to fail, which is the same as `module.error`.
+2. If the `vm.SourceTextModule` has top-level `await` in itself or any of its dependencies, the promise will be
+   fulfilled _asynchronously_ after the module and all its dependencies have been evaluated.
+   1. If the evaluation succeeds, the promise will be _asynchronously_ resolved to `undefined`.
+   2. If the evaluation results in an exception, the promise will be _asynchronously_ rejected with the exception
+      that causes the evaluation to fail.
+
+If the module is a `vm.SyntheticModule`, `evaluate()` always returns a promise that fulfills synchronously, see
+the specification of [Evaluate() of a Synthetic Module Record][]:
+
+1. If the `evaluateCallback` passed to its constructor throws an exception synchronously, `evaluate()` returns
+   a promise that will be synchronously rejected with that exception.
+2. If the `evaluateCallback` does not throw an exception, `evaluate()` returns a promise that will be
+   synchronously resolved to `undefined`.
+
+The `evaluateCallback` of a `vm.SyntheticModule` is executed synchronously within the `evaluate()` call, and its
+return value is discarded. This means if `evaluateCallback` is an asynchronous function, the promise returned by
+`evaluate()` will not reflect its asynchronous behavior, and any rejections from an asynchronous
+`evaluateCallback` will be lost.
+
+`evaluate()` could also be called again after the module has already been evaluated, in which case:
+
+1. If the initial evaluation ended in success (`module.status` is `'evaluated'`), it will do nothing
+   and return a promise that resolves to `undefined`.
+2. If the initial evaluation resulted in an exception (`module.status` is `'errored'`), it will re-reject
+   the exception that the initial evaluation resulted in.
+
+This method cannot be called while the module is being evaluated (`module.status` is `'evaluating'`).
 
 ### `module.identifier`
 
@@ -2221,6 +2249,7 @@ const { Script, SyntheticModule } = require('node:vm');
 [Cyclic Module Record]: https://tc39.es/ecma262/#sec-cyclic-module-records
 [ECMAScript Module Loader]: esm.md#modules-ecmascript-modules
 [Evaluate() concrete method]: https://tc39.es/ecma262/#sec-moduleevaluation
+[Evaluate() of a Synthetic Module Record]: https://tc39.es/ecma262/#sec-smr-Evaluate
 [FinishLoadingImportedModule]: https://tc39.es/ecma262/#sec-FinishLoadingImportedModule
 [GetModuleNamespace]: https://tc39.es/ecma262/#sec-getmodulenamespace
 [HostLoadImportedModule]: https://tc39.es/ecma262/#sec-HostLoadImportedModule

lib/internal/vm/module.js

@@ -13,6 +13,7 @@ const {
   ObjectPrototypeHasOwnProperty,
   ObjectSetPrototypeOf,
   PromisePrototypeThen,
+  PromiseReject,
   PromiseResolve,
   ReflectApply,
   SafePromiseAllReturnArrayLike,
@@ -208,27 +209,31 @@ class Module {
     this[kWrap].instantiate();
   }
 
-  async evaluate(options = kEmptyObject) {
-    validateThisInternalField(this, kWrap, 'Module');
-    validateObject(options, 'options');
+  evaluate(options = kEmptyObject) {
+    try {
+      validateThisInternalField(this, kWrap, 'Module');
+      validateObject(options, 'options');
 
-    let timeout = options.timeout;
-    if (timeout === undefined) {
-      timeout = -1;
-    } else {
-      validateUint32(timeout, 'options.timeout', true);
+      let timeout = options.timeout;
+      if (timeout === undefined) {
+        timeout = -1;
+      } else {
+        validateUint32(timeout, 'options.timeout', true);
+      }
+      const { breakOnSigint = false } = options;
+      validateBoolean(breakOnSigint, 'options.breakOnSigint');
+      const status = this[kWrap].getStatus();
+      if (status !== kInstantiated &&
+          status !== kEvaluated &&
+          status !== kErrored) {
+        throw new ERR_VM_MODULE_STATUS(
+          'must be one of linked, evaluated, or errored',
+        );
+      }
+      return this[kWrap].evaluate(timeout, breakOnSigint);
+    } catch (e) {
+      return PromiseReject(e);
     }
-    const { breakOnSigint = false } = options;
-    validateBoolean(breakOnSigint, 'options.breakOnSigint');
-    const status = this[kWrap].getStatus();
-    if (status !== kInstantiated &&
-        status !== kEvaluated &&
-        status !== kErrored) {
-      throw new ERR_VM_MODULE_STATUS(
-        'must be one of linked, evaluated, or errored',
-      );
-    }
-    await this[kWrap].evaluate(timeout, breakOnSigint);
   }
 
   [customInspectSymbol](depth, options) {

test/parallel/test-vm-module-evaluate-source-text-module.js

@@ -0,0 +1,167 @@
+// Flags: --experimental-vm-modules
+'use strict';
+
+// This tests the result of evaluating a vm.SourceTextModule.
+const common = require('../common');
+
+const assert = require('assert');
+// To make testing easier we just use the public inspect API. If the output format
+// changes, update this test accordingly.
+const { inspect } = require('util');
+const vm = require('vm');
+
+globalThis.callCount = {};
+common.allowGlobals(globalThis.callCount);
+
+// Synchronous error during evaluation results in a synchronously rejected promise.
+{
+  globalThis.callCount.syncError = 0;
+  const mod = new vm.SourceTextModule(`
+    globalThis.callCount.syncError++;
+    throw new Error("synchronous source text module");
+    export const a = 1;
+  `);
+  mod.linkRequests([]);
+  mod.instantiate();
+  const promise = mod.evaluate();
+  assert.strictEqual(globalThis.callCount.syncError, 1);
+  assert.match(inspect(promise), /rejected/);
+  assert(mod.error, 'Expected mod.error to be set');
+  assert.strictEqual(mod.error.message, 'synchronous source text module');
+
+  promise.catch(common.mustCall((err) => {
+    assert.strictEqual(err, mod.error);
+    // Calling evaluate() again results in the same rejection synchronously.
+    const promise2 = mod.evaluate();
+    assert.match(inspect(promise2), /rejected/);
+    promise2.catch(common.mustCall((err2) => {
+      assert.strictEqual(err, err2);
+      // The module is only evaluated once.
+      assert.strictEqual(globalThis.callCount.syncError, 1);
+    }));
+  }));
+}
+
+// Successful evaluation of a module without top-level await results in a
+// promise synchronously resolved to undefined.
+{
+  globalThis.callCount.syncNamedExports = 0;
+  const mod = new vm.SourceTextModule(`
+    globalThis.callCount.syncNamedExports++;
+    export const a = 1, b = 2;
+  `);
+  mod.linkRequests([]);
+  mod.instantiate();
+  const promise = mod.evaluate();
+  assert.match(inspect(promise), /Promise { undefined }/);
+  assert.strictEqual(mod.namespace.a, 1);
+  assert.strictEqual(mod.namespace.b, 2);
+  assert.strictEqual(globalThis.callCount.syncNamedExports, 1);
+  promise.then(common.mustCall((value) => {
+    assert.strictEqual(value, undefined);
+
+    // Calling evaluate() again results in the same resolved promise synchronously.
+    const promise2 = mod.evaluate();
+    assert.match(inspect(promise2), /Promise { undefined }/);
+    assert.strictEqual(mod.namespace.a, 1);
+    assert.strictEqual(mod.namespace.b, 2);
+    promise2.then(common.mustCall((value) => {
+      assert.strictEqual(value, undefined);
+      // The module is only evaluated once.
+      assert.strictEqual(globalThis.callCount.syncNamedExports, 1);
+    }));
+  }));
+}
+
+{
+  globalThis.callCount.syncDefaultExports = 0;
+  // Modules with either named and default exports have the same behaviors.
+  const mod = new vm.SourceTextModule(`
+    globalThis.callCount.syncDefaultExports++;
+    export default 42;
+  `);
+  mod.linkRequests([]);
+  mod.instantiate();
+  const promise = mod.evaluate();
+  assert.match(inspect(promise), /Promise { undefined }/);
+  assert.strictEqual(mod.namespace.default, 42);
+  assert.strictEqual(globalThis.callCount.syncDefaultExports, 1);
+
+  promise.then(common.mustCall((value) => {
+    assert.strictEqual(value, undefined);
+
+    // Calling evaluate() again results in the same resolved promise synchronously.
+    const promise2 = mod.evaluate();
+    assert.match(inspect(promise2), /Promise { undefined }/);
+    assert.strictEqual(mod.namespace.default, 42);
+    promise2.then(common.mustCall((value) => {
+      assert.strictEqual(value, undefined);
+      // The module is only evaluated once.
+      assert.strictEqual(globalThis.callCount.syncDefaultExports, 1);
+    }));
+  }));
+}
+
+// Successful evaluation of a module with top-level await results in a promise
+// that is fulfilled asynchronously with undefined.
+{
+  globalThis.callCount.asyncEvaluation = 0;
+  const mod = new vm.SourceTextModule(`
+    globalThis.callCount.asyncEvaluation++;
+    await Promise.resolve();
+    export const a = 1;
+  `);
+  mod.linkRequests([]);
+  mod.instantiate();
+  const promise = mod.evaluate();
+  assert.match(inspect(promise), /<pending>/);
+  // Accessing the namespace before the promise is fulfilled throws ReferenceError.
+  assert.throws(() => mod.namespace.a, { name: 'ReferenceError' });
+  assert.strictEqual(globalThis.callCount.asyncEvaluation, 1);
+  promise.then(common.mustCall((value) => {
+    assert.strictEqual(value, undefined);
+    assert.strictEqual(globalThis.callCount.asyncEvaluation, 1);
+
+    // Calling evaluate() again results in a promise synchronously resolved to undefined.
+    const promise2 = mod.evaluate();
+    assert.match(inspect(promise2), /Promise { undefined }/);
+    assert.strictEqual(mod.namespace.a, 1);
+    promise2.then(common.mustCall((value) => {
+      assert.strictEqual(value, undefined);
+      // The module is only evaluated once.
+      assert.strictEqual(globalThis.callCount.asyncEvaluation, 1);
+    }));
+  }));
+}
+
+// Rejection of a top-level await promise results in a promise that is
+// rejected asynchronously with the same reason.
+{
+  globalThis.callCount.asyncRejection = 0;
+  const mod = new vm.SourceTextModule(`
+    globalThis.callCount.asyncRejection++;
+    await Promise.reject(new Error("asynchronous source text module"));
+    export const a = 1;
+  `);
+  mod.linkRequests([]);
+  mod.instantiate();
+  const promise = mod.evaluate();
+  assert.match(inspect(promise), /<pending>/);
+  // Accessing the namespace before the promise is fulfilled throws ReferenceError.
+  assert.throws(() => mod.namespace.a, { name: 'ReferenceError' });
+  promise.catch(common.mustCall((err) => {
+    assert.strictEqual(err, mod.error);
+    assert.strictEqual(err.message, 'asynchronous source text module');
+    assert.strictEqual(globalThis.callCount.asyncRejection, 1);
+
+    // Calling evaluate() again results in a promise synchronously rejected
+    // with the same reason.
+    const promise2 = mod.evaluate();
+    assert.match(inspect(promise2), /rejected/);
+    promise2.catch(common.mustCall((err2) => {
+      assert.strictEqual(err, err2);
+      // The module is only evaluated once.
+      assert.strictEqual(globalThis.callCount.asyncRejection, 1);
+    }));
+  }));
+}

test/parallel/test-vm-module-evaluate-synthethic-module-rejection.js

@@ -0,0 +1,42 @@
+// Flags: --experimental-vm-modules
+'use strict';
+
+// This tests the result of evaluating a vm.SyntheticModule with an async rejection
+// in the evaluation step.
+
+const common = require('../common');
+
+const assert = require('assert');
+// To make testing easier we just use the public inspect API. If the output format
+// changes, update this test accordingly.
+const { inspect } = require('util');
+const vm = require('vm');
+
+// The promise _synchronously_ resolves to undefined, because for a synthethic module,
+// the evaluation operation can only either resolve or reject immediately.
+// In this case, the asynchronously rejected promise can't be handled from the outside,
+// so we'll catch it with the isolate-level unhandledRejection handler.
+// See https://tc39.es/ecma262/#sec-smr-Evaluate
+process.on('unhandledRejection', common.mustCall((err) => {
+  assert.strictEqual(err.message, 'asynchronous source text module');
+}));
+
+const mod = new vm.SyntheticModule(['a'], common.mustCall(async () => {
+  throw new Error('asynchronous source text module');
+}));
+
+const promise = mod.evaluate();
+assert.match(inspect(promise), /Promise { undefined }/);
+// Accessing the uninitialized export of a synthetic module returns undefined.
+assert.strictEqual(mod.namespace.a, undefined);
+
+promise.then(common.mustCall((value) => {
+  assert.strictEqual(value, undefined);
+}));
+
+// Calling evaluate() again results in a promise _synchronously_ resolved to undefined again.
+const promise2 = mod.evaluate();
+assert.match(inspect(promise2), /Promise { undefined }/);
+promise2.then(common.mustCall((value) => {
+  assert.strictEqual(value, undefined);
+}));

test/parallel/test-vm-module-evaluate-synthethic-module.js

@@ -0,0 +1,83 @@
+// Flags: --experimental-vm-modules
+'use strict';
+
+// This tests the result of evaluating a vm.SynthethicModule.
+// See https://tc39.es/ecma262/#sec-smr-Evaluate
+const common = require('../common');
+
+const assert = require('assert');
+// To make testing easier we just use the public inspect API. If the output format
+// changes, update this test accordingly.
+const { inspect } = require('util');
+const vm = require('vm');
+
+// Synthetic modules with a synchronous evaluation step evaluate to a promise synchronously
+// resolved to undefined.
+{
+  const mod = new vm.SyntheticModule(['a'], common.mustCall(() => {
+    mod.setExport('a', 42);
+  }));
+  const promise = mod.evaluate();
+  assert.match(inspect(promise), /Promise { undefined }/);
+  assert.strictEqual(mod.namespace.a, 42);
+
+  promise.then(common.mustCall((value) => {
+    assert.strictEqual(value, undefined);
+
+    // Calling evaluate() again results in a promise synchronously resolved to undefined.
+    const promise2 = mod.evaluate();
+    assert.match(inspect(promise2), /Promise { undefined }/);
+    promise2.then(common.mustCall((value) => {
+      assert.strictEqual(value, undefined);
+    }));
+  }));
+}
+
+// Synthetic modules with an asynchronous evaluation step evaluate to a promise
+// _synchronously_ resolved to undefined.
+{
+  const mod = new vm.SyntheticModule(['a'], common.mustCall(async () => {
+    const result = await Promise.resolve(42);
+    mod.setExport('a', result);
+    return result;
+  }));
+  const promise = mod.evaluate();
+  assert.match(inspect(promise), /Promise { undefined }/);
+  // Accessing the uninitialized export of a synthetic module returns undefined.
+  assert.strictEqual(mod.namespace.a, undefined);
+
+  promise.then(common.mustCall((value) => {
+    assert.strictEqual(value, undefined);
+
+    // Calling evaluate() again results in a promise _synchronously_ resolved to undefined again.
+    const promise2 = mod.evaluate();
+    assert.match(inspect(promise2), /Promise { undefined }/);
+    promise2.then(common.mustCall((value) => {
+      assert.strictEqual(value, undefined);
+    }));
+  }));
+}
+
+// Synchronous error during the evaluation step of a synthetic module results
+// in a _synchronously_ rejected promise.
+{
+  const mod = new vm.SyntheticModule(['a'], common.mustCall(() => {
+    throw new Error('synchronous synthethic module');
+  }));
+  const promise = mod.evaluate();
+  assert.match(inspect(promise), /rejected/);
+  assert(mod.error, 'Expected mod.error to be set');
+  assert.strictEqual(mod.error.message, 'synchronous synthethic module');
+
+  promise.catch(common.mustCall((err) => {
+    assert.strictEqual(err, mod.error);
+
+    // Calling evaluate() again results in a promise _synchronously_ rejected
+    // with the same reason.
+    const promise2 = mod.evaluate();
+    assert.match(inspect(promise2), /rejected/);
+    promise2.catch(common.mustCall((err2) => {
+      assert.strictEqual(err, err2);
+    }));
+  }));
+}

test/parallel/test-vm-module-evaluate-while-evaluating.js

@@ -0,0 +1,34 @@
+// Flags: --experimental-vm-modules
+'use strict';
+
+// This tests the result of evaluating a vm.Module while it is evaluating.
+const common = require('../common');
+
+const assert = require('assert');
+const vm = require('vm');
+
+{
+  let mod;
+  globalThis.evaluate = common.mustCall(() => {
+    assert.rejects(() => mod.evaluate(), {
+      code: 'ERR_VM_MODULE_STATUS'
+    }).then(common.mustCall());
+  });
+  common.allowGlobals(globalThis.evaluate);
+  mod = new vm.SourceTextModule(`
+    globalThis.evaluate();
+    export const a = 42;
+  `);
+  mod.linkRequests([]);
+  mod.instantiate();
+  mod.evaluate();
+}
+
+{
+  const mod = new vm.SyntheticModule(['a'], common.mustCall(() => {
+    assert.rejects(() => mod.evaluate(), {
+      code: 'ERR_VM_MODULE_STATUS'
+    }).then(common.mustCall());
+  }));
+  mod.evaluate();
+}