Normative: Mark sync module evaluation promise as handled

[nicolo-ribaudo]

When we "unwrap" the promise returned by _module_.Evaluate(), we need to mark is as handled so that the host knows that the rejection is not unhandled. Note that this is not currently observable withing ECMA-262: it's only observable if a host provides a custom Module Record "subclass" other than Cyclic Module Record whose Evaluate() can return a rejected promise.

I extracted this "synchronously evaluate and unwrap the promise" into its own AO, which is currently also used in https://tc39.es/proposal-defer-import-eval/.

Fixes #3533.

[nicolo-ribaudo]

This assertion is because non-Cyclic Module Records are guaranteed to be evaluated synchronously. The import defer proposal will relax this assertion to the one in https://tc39.es/proposal-defer-import-eval/#sec-EvaluateSync.

[nicolo-ribaudo]

Marking as normative since the behavior is observable through host APIs, but this is really a fix for a spec bug caused by use promises for internal machinery.

This change is not observable on the web platform, because it has no no non-cyclic module records that throw when evaluated.

This change would be observable in Node.js through the following code, but Node.js/V8 already implements the behavior proposed by this PR (i.e. it does not fire the event).

// entrypoint
process.addListener("unhandledRejection", function (err) {
  console.error("ERR!", err);
});

import("./module-1.mjs").catch(() => {})

// module-1.mjs
import "./module-2.cjs";

// module-2.cjs
// NOTE: this is a Module Record that's not a Cyclic Module Record
throw new Error("Fail!");

[michaelficarra]

This is fine though I believe we want to avoid introducing AOs that are only called in one place. We may want to make an exception for this one since it'll be coming in anyway with the deferred module eval proposal.

[nicolo-ribaudo]

We may want to make an exception for this one since it'll be coming in anyway with the deferred module eval proposal.

Let me know what to do after the editor call -- I can keep the extracted AO in the proposal if preferred.

[jmdyck]

Passes my checks now.

[bakkot]

@nicolo-ribaudo Since this is technically normative we're going to ask for consensus. As far as I can tell this isn't actually an observable change in any real implementations, though, right? I don't think it's relevant for JSON or CSS modules, wasm modules are cyclic module records, node's import "./foo.cjs" is technically going down this path but doesn't trigger the unhandled promise rejection event in case of error (I think), and that's all the cases I can think of.

I want to tell the committee this change in the spec doesn't imply any changes in implementations but I want to make sure that's actually true before making that claim.

[nicolo-ribaudo]

Yes, that is correct. The only way this would be observable is in Node.js CJS imports, but it already behaves as defined by this PR. It is not observable on the web.

---

This is the Node.js test case:

// main.mjs
process.addListener("unhandledRejection", e => console.log("Unhandled", e));
import("./importer.mjs");

// importer.mjs
import "./foo.cjs";

// foo.cjs
throw new Error("Hi!")

With this PR there is only one unhandledRejection expected (the one for the dynamic import promise), and it's what Node.js does. Before this PR, the spec would have caused two unhandledRejection events: one for the internal promise that the "CJS module evaluation" gets wrapped in, and one for the dynamic import promise.