Repository: babel/babel (TypeScript)

Issue:
[Bug]: Error in `toString` during dynamic import should result in rejected promise
### 💻

- [ ] Would you like to work on a fix?

### How are you using Babel?

Other (Next.js, Gatsby, vue-cli, ...)

### Input code

```js
var filename = {
  toString: () => { throw "toString failure"; }
}

import(filename)
.then(obj => {
  console.log("import success");
})
.catch(err => {
  console.log("import failed");
});
```

### Configuration file name

_No response_

### Configuration

_No response_

### Current and expected behavior

[REPL](https://babeljs.io/repl/#?browsers=&build=&builtIns=false&corejs=3.21&spec=false&loose=false&code_lz=G4QwTgBAZglgNgUwHYgLYIgXggbwFAQQAuA9gMpFgxIDmAXBABQCUWAfLsQBZgkDuEAESkKVWtBDwArmASCA3BAC-eFXhioADiTBFGsRCnTM8AOiJdkjEgCMAVu1wEIAYxJIAziUSm4JGoyCGtq6EB5SLi4IHh6CzPKqJqYuIEQuXIwIYJCYHPiEbp7eCL7-gcE6RBLwCAAmcQlK8UA&debug=false&forceAllTransforms=false&shippedProposals=false&circleciRepo=&evaluate=false&fileSize=false&timeTravel=false&sourceType=script&lineWrap=true&presets=env&prettier=true&targets=&version=7.20.6&externalPlugins=&assumptions=%7B%7D)

The code above is transpiled into
```js
...
Promise.resolve("".concat(filename))
...
```
which results in throwing "toString failure".

However, since the exception is thrown during `toString`, the original import expression should result in [rejected promise](https://262.ecma-international.org/13.0/#sec-import-call-runtime-semantics-evaluation), and "import failed" should be printed.

### Environment

Tried at REPL (7.20.6)

### Possible solution

_No response_

### Additional context

_No response_


Hints:
Hey @f52985! We really appreciate you taking the time to report an issue. The collaborators on this project attempt to help as many people as possible, but we're a limited number of volunteers, so it's possible this won't be addressed swiftly.

If you need any help, or just have general Babel or JavaScript questions, we have a vibrant [Slack community](https://babeljs.slack.com) that typically always has someone willing to help. You can sign-up [here](https://slack.babeljs.io/) for an invite.
[Spec](https://tc39.es/ecma262/#sec-import-call-runtime-semantics-evaluation):
> [ImportCall](https://tc39.es/ecma262/#prod-ImportCall) : import ( [AssignmentExpression](https://tc39.es/ecma262/#prod-AssignmentExpression) )
> 1. Let referencingScriptOrModule be [GetActiveScriptOrModule](https://tc39.es/ecma262/#sec-getactivescriptormodule)().
> 2. Let argRef be ? [Evaluation](https://tc39.es/ecma262/#sec-evaluation) of [AssignmentExpression](https://tc39.es/ecma262/#prod-AssignmentExpression).
> 3. Let specifier be ? [GetValue](https://tc39.es/ecma262/#sec-getvalue)(argRef).
> 4. Let promiseCapability be ! [NewPromiseCapability](https://tc39.es/ecma262/#sec-newpromisecapability)([%Promise%](https://tc39.es/ecma262/#sec-promise-constructor)).
> 5. Let specifierString be [Completion](https://tc39.es/ecma262/#sec-completion-ao)([ToString](https://tc39.es/ecma262/#sec-tostring)(specifier)).
> 6. [IfAbruptRejectPromise](https://tc39.es/ecma262/#sec-ifabruptrejectpromise)(specifierString, promiseCapability).
> 7. Perform [HostImportModuleDynamically](https://tc39.es/ecma262/#sec-hostimportmoduledynamically)(referencingScriptOrModule, specifierString, promiseCapability).
> 8. Return promiseCapability.[[Promise]].

So we should transform `import(source)` into something like:
```js
(s => Promise.resolve().then(() => require(`${s}`)})) // `.toString()` evaluated in Promise
    (source) // `source` itself should be evaluated immediately
```

Notice that we can't just use ```Promise.resolve(source).then(s => require(`${s}`)})```, because if `source` is evaluated to a `Promise`, it should not be resolved and should be convert to string directly, just like `import('[object Promise]')`.

Related code:
https://github.com/babel/babel/blob/f5b52208f05157a348fdfaa0222c07a9a83fb101/packages/babel-plugin-transform-modules-commonjs/src/dynamic-import.ts#L23-L35
> (s => Promise.resolve().then(() => require(`${s}`)})) // `.toString()` evaluated in Promise

The source should not be evaluated in the promise, because the evaluation order will be affected. Consider this example:

```js
var i = 0;
import(`./${++i}`)
++i;
```

If the source were evaluated in the promise, it will be `"./2"` instead of `"./1"`.

We could solve this by replacing `Promise.resolve()` to a new helper, which turns the abrupt completion into a rejected promise. But given that `import()` is a very popular feature and TS is also affected by this issue, a new compiler assumption (tentatively `importModuleNameDoesNotThrow`) to get rid of this helper sounds reasonable to me.
@JLHwung I mean this:
```js
var i = 0;
(s =>
    Promise.resolve().then(() => require(`${s}`)})
)(`./${++i}`);
++i;
```
The source itself is evaluated outside the promise, only the toString part is inside the promise.
Good point, but `toString` could execute arbitrary code:

```js
var i = 0;
import({ toString: () => `./${++i}` })
++i;
```
@JLHwung You are right, the `toString()` should be executed before the `++i;`, but if we evaluate it in `.then()`, it will be after the `++i;`.

We do need a helper like this:
```js
export default function _dynamicImportSpecifier(s) {
  try {
    return Promise.resolve('' + s);
  } catch(e) {
    return Promise.reject(e);
  }
}

_dynamicImportSpecifier(source).then(s => require(s)})
```

Patch:
diff --git a/packages/babel-helper-module-transforms/src/dynamic-import.ts b/packages/babel-helper-module-transforms/src/dynamic-import.ts
--- a/packages/babel-helper-module-transforms/src/dynamic-import.ts
+++ b/packages/babel-helper-module-transforms/src/dynamic-import.ts
@@ -4,6 +4,7 @@
 import * as t from "@babel/types";
 import template from "@babel/template";
 
+// TODO(Babel 8): Remove this
 export function getDynamicImportSource(
   node: t.CallExpression,
 ): t.StringLiteral | t.TemplateLiteral {
@@ -13,3 +14,49 @@ export function getDynamicImportSource(
     ? source
     : (template.expression.ast`\`\${${source}}\`` as t.TemplateLiteral);
 }
+
+export function buildDynamicImport(
+  node: t.CallExpression,
+  deferToThen: boolean,
+  wrapWithPromise: boolean,
+  builder: (specifier: t.Expression) => t.Expression,
+): t.Expression {
+  const [specifier] = node.arguments;
+
+  if (
+    t.isStringLiteral(specifier) ||
+    (t.isTemplateLiteral(specifier) && specifier.quasis.length === 0)
+  ) {
+    if (deferToThen) {
+      return template.expression.ast`
+        Promise.resolve().then(() => ${builder(specifier)})
+      `;
+    } else return builder(specifier);
+  }
+
+  const specifierToString = t.isTemplateLiteral(specifier)
+    ? t.identifier("specifier")
+    : t.templateLiteral(
+        [t.templateElement({ raw: "" }), t.templateElement({ raw: "" })],
+        [t.identifier("specifier")],
+      );
+
+  if (deferToThen) {
+    return template.expression.ast`
+      (specifier =>
+        new Promise(r => r(${specifierToString}))
+          .then(s => ${builder(t.identifier("s"))})
+      )(${specifier})
+    `;
+  } else if (wrapWithPromise) {
+    return template.expression.ast`
+      (specifier =>
+        new Promise(r => r(${builder(specifierToString)}))
+      )(${specifier})
+    `;
+  } else {
+    return template.expression.ast`
+      (specifier => ${builder(specifierToString)})(${specifier})
+    `;
+  }
+}
diff --git a/packages/babel-helper-module-transforms/src/index.ts b/packages/babel-helper-module-transforms/src/index.ts
--- a/packages/babel-helper-module-transforms/src/index.ts
+++ b/packages/babel-helper-module-transforms/src/index.ts
@@ -35,7 +35,7 @@ import type {
 } from "./normalize-and-load-metadata";
 import type { NodePath } from "@babel/traverse";
 
-export { getDynamicImportSource } from "./dynamic-import";
+export { buildDynamicImport, getDynamicImportSource } from "./dynamic-import";
 
 export { default as getModuleName } from "./get-module-name";
 export type { PluginOptions } from "./get-module-name";
diff --git a/packages/babel-plugin-transform-modules-amd/src/index.ts b/packages/babel-plugin-transform-modules-amd/src/index.ts
--- a/packages/babel-plugin-transform-modules-amd/src/index.ts
+++ b/packages/babel-plugin-transform-modules-amd/src/index.ts
@@ -1,5 +1,6 @@
 import { declare } from "@babel/helper-plugin-utils";
 import {
+  buildDynamicImport,
   isModule,
   rewriteModuleStatementsAndPrepareHeader,
   type RewriteModuleStatementsAndPrepareHeaderOptions,
@@ -9,7 +10,6 @@ import {
   ensureStatementsHoisted,
   wrapInterop,
   getModuleName,
-  getDynamicImportSource,
 } from "@babel/helper-module-transforms";
 import { template, types as t } from "@babel/core";
 import type { PluginOptions } from "@babel/helper-module-transforms";
@@ -99,14 +99,20 @@ export default declare<State>((api, options: Options) => {
         if (!noInterop) result = wrapInterop(path, result, "namespace");
 
         path.replaceWith(
-          template.expression.ast`
-            new Promise((${resolveId}, ${rejectId}) =>
-              ${requireId}(
-                [${getDynamicImportSource(path.node)}],
-                imported => ${t.cloneNode(resolveId)}(${result}),
-                ${t.cloneNode(rejectId)}
+          buildDynamicImport(
+            path.node,
+            false,
+            false,
+            specifier => template.expression.ast`
+              new Promise((${resolveId}, ${rejectId}) =>
+                ${requireId}(
+                  [${specifier}],
+                  imported => ${t.cloneNode(resolveId)}(${result}),
+                  ${t.cloneNode(rejectId)}
+                )
               )
-            )`,
+            `,
+          ),
         );
       },
 
diff --git a/packages/babel-plugin-transform-modules-commonjs/src/dynamic-import.ts b/packages/babel-plugin-transform-modules-commonjs/src/dynamic-import.ts
--- a/packages/babel-plugin-transform-modules-commonjs/src/dynamic-import.ts
+++ b/packages/babel-plugin-transform-modules-commonjs/src/dynamic-import.ts
@@ -3,7 +3,7 @@
 
 import type { NodePath } from "@babel/traverse";
 import { types as t, template, type File } from "@babel/core";
-import { getDynamicImportSource } from "@babel/helper-module-transforms";
+import { buildDynamicImport } from "@babel/helper-module-transforms";
 
 const requireNoInterop = (source: t.Expression) =>
   template.expression.ast`require(${source})`;
@@ -20,19 +20,9 @@ export function transformDynamicImport(
 ) {
   const buildRequire = noInterop ? requireNoInterop : requireInterop;
 
-  const source = getDynamicImportSource(path.node);
-
-  const replacement =
-    t.isStringLiteral(source) ||
-    (t.isTemplateLiteral(source) && source.quasis.length === 0)
-      ? template.expression.ast`
-        Promise.resolve().then(() => ${buildRequire(source, file)})
-      `
-      : template.expression.ast`
-        Promise.resolve(${source}).then(
-          s => ${buildRequire(t.identifier("s"), file)}
-        )
-      `;
-
-  path.replaceWith(replacement);
+  path.replaceWith(
+    buildDynamicImport(path.node, true, false, specifier =>
+      buildRequire(specifier, file),
+    ),
+  );
 }
diff --git a/packages/babel-plugin-transform-modules-systemjs/src/index.ts b/packages/babel-plugin-transform-modules-systemjs/src/index.ts
--- a/packages/babel-plugin-transform-modules-systemjs/src/index.ts
+++ b/packages/babel-plugin-transform-modules-systemjs/src/index.ts
@@ -2,9 +2,9 @@ import { declare } from "@babel/helper-plugin-utils";
 import hoistVariables from "@babel/helper-hoist-variables";
 import { template, types as t } from "@babel/core";
 import {
-  rewriteThis,
+  buildDynamicImport,
   getModuleName,
-  getDynamicImportSource,
+  rewriteThis,
 } from "@babel/helper-module-transforms";
 import type { PluginOptions } from "@babel/helper-module-transforms";
 import { isIdentifierName } from "@babel/helper-validator-identifier";
@@ -274,12 +274,14 @@ export default declare<PluginState>((api, options: Options) => {
           }
 
           path.replaceWith(
-            t.callExpression(
-              t.memberExpression(
-                t.identifier(state.contextIdent),
-                t.identifier("import"),
+            buildDynamicImport(path.node, false, true, specifier =>
+              t.callExpression(
+                t.memberExpression(
+                  t.identifier(state.contextIdent),
+                  t.identifier("import"),
+                ),
+                [specifier],
               ),
-              [getDynamicImportSource(path.node)],
             ),
           );
         }


Test patch:
diff --git a/packages/babel-plugin-proposal-dynamic-import/test/fixtures/amd/to-string/input.js b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/amd/to-string/input.js
new file mode 100644
--- /dev/null
+++ b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/amd/to-string/input.js
@@ -0,0 +1 @@
+import(2);
diff --git a/packages/babel-plugin-proposal-dynamic-import/test/fixtures/amd/to-string/output.js b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/amd/to-string/output.js
new file mode 100644
--- /dev/null
+++ b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/amd/to-string/output.js
@@ -0,0 +1,3 @@
+define(["require"], function (_require) {
+  (specifier => new Promise((_resolve, _reject) => _require([`${specifier}`], imported => _resolve(babelHelpers.interopRequireWildcard(imported)), _reject)))(2);
+});
diff --git a/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-template-literal/1.js b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-template-literal/1.js
new file mode 100644
--- /dev/null
+++ b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-template-literal/1.js
@@ -0,0 +1 @@
+module.exports = 1;
diff --git a/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-template-literal/exec.js b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-template-literal/exec.js
new file mode 100644
--- /dev/null
+++ b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-template-literal/exec.js
@@ -0,0 +1,3 @@
+let x = 1;
+return expect(import(`./${x}.js`))
+  .resolves.toHaveProperty("default", 1);
diff --git a/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-template-literal/options.json b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-template-literal/options.json
new file mode 100644
--- /dev/null
+++ b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-template-literal/options.json
@@ -0,0 +1,5 @@
+{
+  "parserOpts": {
+    "allowReturnOutsideFunction": true
+  }
+}
diff --git a/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-throw/exec.js b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-throw/exec.js
new file mode 100644
--- /dev/null
+++ b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-throw/exec.js
@@ -0,0 +1,11 @@
+expect(() => {
+  function f() { throw "should throw"; }
+  import(f());
+}).toThrow("should throw");
+
+expect(() => {
+  const a = {
+    get x() { throw "should throw"; },
+  };
+  import(a.x);
+}).toThrow("should throw");
diff --git a/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-primitive/exec.js b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-primitive/exec.js
new file mode 100644
--- /dev/null
+++ b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-primitive/exec.js
@@ -0,0 +1,6 @@
+return expect(import({
+  [Symbol.toPrimitive](hint) {
+    if (hint === "string") return "./foo.js";
+    return null;
+  }
+})).resolves.toHaveProperty("default", "foo");
diff --git a/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-primitive/foo.js b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-primitive/foo.js
new file mode 100644
--- /dev/null
+++ b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-primitive/foo.js
@@ -0,0 +1 @@
+module.exports = "foo";
diff --git a/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-primitive/options.json b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-primitive/options.json
new file mode 100644
--- /dev/null
+++ b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-primitive/options.json
@@ -0,0 +1,5 @@
+{
+  "parserOpts": {
+    "allowReturnOutsideFunction": true
+  }
+}
diff --git a/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-string-order/1.js b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-string-order/1.js
new file mode 100644
--- /dev/null
+++ b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-string-order/1.js
@@ -0,0 +1 @@
+module.exports = 1;
diff --git a/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-string-order/exec.js b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-string-order/exec.js
new file mode 100644
--- /dev/null
+++ b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-string-order/exec.js
@@ -0,0 +1,7 @@
+let i = 0;
+const promise = expect(import({ toString: () => `./${++i}.js` }))
+  .resolves.toHaveProperty("default", 1);
+expect(i).toBe(1);
+++i;
+expect(i).toBe(2);
+return promise;
diff --git a/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-string-order/options.json b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-string-order/options.json
new file mode 100644
--- /dev/null
+++ b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-string-order/options.json
@@ -0,0 +1,5 @@
+{
+  "parserOpts": {
+    "allowReturnOutsideFunction": true
+  }
+}
diff --git a/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-string-throw/exec.js b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-string-throw/exec.js
new file mode 100644
--- /dev/null
+++ b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-string-throw/exec.js
@@ -0,0 +1,2 @@
+return expect(import({ toString: () => { throw "toString failed"; } }))
+  .rejects.toBe("toString failed");
diff --git a/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-string-throw/options.json b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-string-throw/options.json
new file mode 100644
--- /dev/null
+++ b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/exec-to-string-throw/options.json
@@ -0,0 +1,5 @@
+{
+  "parserOpts": {
+    "allowReturnOutsideFunction": true
+  }
+}
diff --git a/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/template-literal/input.js b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/template-literal/input.js
new file mode 100644
--- /dev/null
+++ b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/template-literal/input.js
@@ -0,0 +1,2 @@
+const x = 1;
+import(`./${x}.js`);
diff --git a/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/template-literal/output.js b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/template-literal/output.js
new file mode 100644
--- /dev/null
+++ b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/template-literal/output.js
@@ -0,0 +1,2 @@
+const x = 1;
+(specifier => new Promise(r => r(specifier)).then(s => babelHelpers.interopRequireWildcard(require(s))))(`./${x}.js`);
diff --git a/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/to-string/output.js b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/to-string/output.js
--- a/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/to-string/output.js
+++ b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/commonjs/to-string/output.js
@@ -1 +1 @@
-Promise.resolve(`${2}`).then(s => babelHelpers.interopRequireWildcard(require(s)));
+(specifier => new Promise(r => r(`${specifier}`)).then(s => babelHelpers.interopRequireWildcard(require(s))))(2);
diff --git a/packages/babel-plugin-proposal-dynamic-import/test/fixtures/systemjs/to-string/input.js b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/systemjs/to-string/input.js
new file mode 100644
--- /dev/null
+++ b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/systemjs/to-string/input.js
@@ -0,0 +1 @@
+import(2);
diff --git a/packages/babel-plugin-proposal-dynamic-import/test/fixtures/systemjs/to-string/output.js b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/systemjs/to-string/output.js
new file mode 100644
--- /dev/null
+++ b/packages/babel-plugin-proposal-dynamic-import/test/fixtures/systemjs/to-string/output.js
@@ -0,0 +1,10 @@
+System.register([], function (_export, _context) {
+  "use strict";
+
+  return {
+    setters: [],
+    execute: function () {
+      (specifier => new Promise(r => r(_context.import(`${specifier}`))))(2);
+    }
+  };
+});