fix: avoid downleveled dynamic import closing over specifier expressi…

…on (#49663) * fix: evaluate dynamic import specifier expressions synchronously * refactor * Update src/compiler/transformers/module/module.ts Co-authored-by: Ron Buckton <ron.buckton@microsoft.com> * [Experiment] Co-authored-by: Ron Buckton <ron.buckton@microsoft.com>
microsoft · Oct 18, 2022 · 1f8959f · 1f8959f
1 parent 11066b2
commit 1f8959f
Show file tree

Hide file tree

Showing 14 changed files with 180 additions and 42 deletions.
diff --git a/src/compiler/transformers/module/module.ts b/src/compiler/transformers/module/module.ts
@@ -721,7 +721,7 @@ namespace ts {
                     return createImportCallExpressionUMD(argument ?? factory.createVoidZero(), containsLexicalThis);
                 case ModuleKind.CommonJS:
                 default:
-                    return createImportCallExpressionCommonJS(argument, containsLexicalThis);
+                    return createImportCallExpressionCommonJS(argument);
             }
         }
 
@@ -745,7 +745,7 @@ namespace ts {
                 return factory.createConditionalExpression(
                     /*condition*/ factory.createIdentifier("__syncRequire"),
                     /*questionToken*/ undefined,
-                    /*whenTrue*/ createImportCallExpressionCommonJS(arg, containsLexicalThis),
+                    /*whenTrue*/ createImportCallExpressionCommonJS(arg),
                     /*colonToken*/ undefined,
                     /*whenFalse*/ createImportCallExpressionAMD(argClone, containsLexicalThis)
                 );
@@ -755,7 +755,7 @@ namespace ts {
                 return factory.createComma(factory.createAssignment(temp, arg), factory.createConditionalExpression(
                     /*condition*/ factory.createIdentifier("__syncRequire"),
                     /*questionToken*/ undefined,
-                    /*whenTrue*/ createImportCallExpressionCommonJS(temp, containsLexicalThis),
+                    /*whenTrue*/ createImportCallExpressionCommonJS(temp, /* isInlineable */ true),
                     /*colonToken*/ undefined,
                     /*whenFalse*/ createImportCallExpressionAMD(temp, containsLexicalThis)
                 ));
@@ -820,14 +820,25 @@ namespace ts {
             return promise;
         }
 
-        function createImportCallExpressionCommonJS(arg: Expression | undefined, containsLexicalThis: boolean): Expression {
-            // import("./blah")
+        function createImportCallExpressionCommonJS(arg: Expression | undefined, isInlineable?: boolean): Expression {
+            // import(x)
             // emit as
-            // Promise.resolve().then(function () { return require(x); }) /*CommonJs Require*/
+            // var _a;
+            // (_a = x, Promise.resolve().then(() => require(_a)) /*CommonJs Require*/
             // We have to wrap require in then callback so that require is done in asynchronously
             // if we simply do require in resolve callback in Promise constructor. We will execute the loading immediately
-            const promiseResolveCall = factory.createCallExpression(factory.createPropertyAccessExpression(factory.createIdentifier("Promise"), "resolve"), /*typeArguments*/ undefined, /*argumentsArray*/ []);
-            let requireCall: Expression = factory.createCallExpression(factory.createIdentifier("require"), /*typeArguments*/ undefined, arg ? [arg] : []);
+            // If the arg is not inlineable, we have to evaluate it in the current scope with a temp var
+            const temp = arg && !isSimpleInlineableExpression(arg) && !isInlineable ? factory.createTempVariable(hoistVariableDeclaration) : undefined;
+            const promiseResolveCall = factory.createCallExpression(
+                factory.createPropertyAccessExpression(factory.createIdentifier("Promise"), "resolve"),
+                /*typeArguments*/ undefined,
+                /*argumentsArray*/ [],
+            );
+            let requireCall: Expression = factory.createCallExpression(
+                factory.createIdentifier("require"),
+                /*typeArguments*/ undefined,
+                temp ? [temp] : arg ? [arg] : [],
+            );
             if (getESModuleInterop(compilerOptions)) {
                 requireCall = emitHelpers().createImportStarHelper(requireCall);
             }
@@ -851,16 +862,11 @@ namespace ts {
                     /*parameters*/ [],
                     /*type*/ undefined,
                     factory.createBlock([factory.createReturnStatement(requireCall)]));
-
-                // if there is a lexical 'this' in the import call arguments, ensure we indicate
-                // that this new function expression indicates it captures 'this' so that the
-                // es2015 transformer will properly substitute 'this' with '_this'.
-                if (containsLexicalThis) {
-                    setEmitFlags(func, EmitFlags.CapturesThis);
-                }
             }
 
-            return factory.createCallExpression(factory.createPropertyAccessExpression(promiseResolveCall, "then"), /*typeArguments*/ undefined, [func]);
+            const downleveledImport = factory.createCallExpression(factory.createPropertyAccessExpression(promiseResolveCall, "then"), /*typeArguments*/ undefined, [func]);
+
+            return temp === undefined ? downleveledImport : factory.createCommaListExpression([factory.createAssignment(temp, arg!), downleveledImport]);
         }
 
         function getHelperExpressionForExport(node: ExportDeclaration, innerExpr: Expression) {

diff --git a/tests/baselines/reference/asyncImportNestedYield.js b/tests/baselines/reference/asyncImportNestedYield.js
@@ -46,12 +46,13 @@ var __asyncGenerator = (this && this.__asyncGenerator) || function (thisArg, _ar
 function foo() {
     return __asyncGenerator(this, arguments, function foo_1() {
         return __generator(this, function (_a) {
+            var _b, _c;
             switch (_a.label) {
                 case 0: return [4 /*yield*/, __await("foo")];
                 case 1: return [4 /*yield*/, _a.sent()];
-                case 2: return [4 /*yield*/, __await.apply(void 0, [Promise.resolve().then(function () { return require(_a.sent()); })])];
+                case 2: return [4 /*yield*/, __await.apply(void 0, [(_b = _a.sent(), Promise.resolve().then(function () { return require(_b); }))])];
                 case 3:
-                    Promise.resolve().then(function () { return require((_a.sent())["default"]); });
+                    _c = (_a.sent())["default"], Promise.resolve().then(function () { return require(_c); });
                     return [2 /*return*/];
             }
         });

diff --git a/tests/baselines/reference/dynamicImportEvaluateSpecifier.js b/tests/baselines/reference/dynamicImportEvaluateSpecifier.js
@@ -0,0 +1,31 @@
+//// [dynamicImportEvaluateSpecifier.ts]
+// https://github.com/microsoft/TypeScript/issues/48285
+let i = 0;
+
+import(String(i++));
+import(String(i++));
+
+const getPath = async () => {
+	/* in reality this would do some async FS operation, or a web request */
+	return "/root/my/cool/path";
+};
+
+const someFunction = async () => {
+	const result = await import(await getPath());
+};
+
+
+//// [dynamicImportEvaluateSpecifier.js]
+var _a, _b;
+// https://github.com/microsoft/TypeScript/issues/48285
+let i = 0;
+_a = String(i++), Promise.resolve().then(() => require(_a));
+_b = String(i++), Promise.resolve().then(() => require(_b));
+const getPath = async () => {
+    /* in reality this would do some async FS operation, or a web request */
+    return "/root/my/cool/path";
+};
+const someFunction = async () => {
+    var _a;
+    const result = await (_a = await getPath(), Promise.resolve().then(() => require(_a)));
+};
diff --git a/tests/baselines/reference/dynamicImportEvaluateSpecifier.symbols b/tests/baselines/reference/dynamicImportEvaluateSpecifier.symbols
@@ -0,0 +1,29 @@
+=== tests/cases/compiler/dynamicImportEvaluateSpecifier.ts ===
+// https://github.com/microsoft/TypeScript/issues/48285
+let i = 0;
+>i : Symbol(i, Decl(dynamicImportEvaluateSpecifier.ts, 1, 3))
+
+import(String(i++));
+>String : Symbol(String, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --), Decl(lib.es2015.core.d.ts, --, --), Decl(lib.es2015.iterable.d.ts, --, --) ... and 3 more)
+>i : Symbol(i, Decl(dynamicImportEvaluateSpecifier.ts, 1, 3))
+
+import(String(i++));
+>String : Symbol(String, Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --), Decl(lib.es5.d.ts, --, --), Decl(lib.es2015.core.d.ts, --, --), Decl(lib.es2015.iterable.d.ts, --, --) ... and 3 more)
+>i : Symbol(i, Decl(dynamicImportEvaluateSpecifier.ts, 1, 3))
+
+const getPath = async () => {
+>getPath : Symbol(getPath, Decl(dynamicImportEvaluateSpecifier.ts, 6, 5))
+
+	/* in reality this would do some async FS operation, or a web request */
+	return "/root/my/cool/path";
+};
+
+const someFunction = async () => {
+>someFunction : Symbol(someFunction, Decl(dynamicImportEvaluateSpecifier.ts, 11, 5))
+
+	const result = await import(await getPath());
+>result : Symbol(result, Decl(dynamicImportEvaluateSpecifier.ts, 12, 6))
+>getPath : Symbol(getPath, Decl(dynamicImportEvaluateSpecifier.ts, 6, 5))
+
+};
+
diff --git a/tests/baselines/reference/dynamicImportEvaluateSpecifier.types b/tests/baselines/reference/dynamicImportEvaluateSpecifier.types
@@ -0,0 +1,44 @@
+=== tests/cases/compiler/dynamicImportEvaluateSpecifier.ts ===
+// https://github.com/microsoft/TypeScript/issues/48285
+let i = 0;
+>i : number
+>0 : 0
+
+import(String(i++));
+>import(String(i++)) : Promise<any>
+>String(i++) : string
+>String : StringConstructor
+>i++ : number
+>i : number
+
+import(String(i++));
+>import(String(i++)) : Promise<any>
+>String(i++) : string
+>String : StringConstructor
+>i++ : number
+>i : number
+
+const getPath = async () => {
+>getPath : () => Promise<string>
+>async () => {	/* in reality this would do some async FS operation, or a web request */	return "/root/my/cool/path";} : () => Promise<string>
+
+	/* in reality this would do some async FS operation, or a web request */
+	return "/root/my/cool/path";
+>"/root/my/cool/path" : "/root/my/cool/path"
+
+};
+
+const someFunction = async () => {
+>someFunction : () => Promise<void>
+>async () => {	const result = await import(await getPath());} : () => Promise<void>
+
+	const result = await import(await getPath());
+>result : any
+>await import(await getPath()) : any
+>import(await getPath()) : Promise<any>
+>await getPath() : string
+>getPath() : Promise<string>
+>getPath : () => Promise<string>
+
+};
+
diff --git a/tests/baselines/reference/dynamicImportTrailingComma.js b/tests/baselines/reference/dynamicImportTrailingComma.js
@@ -3,5 +3,6 @@ const path = './foo';
 import(path,);
 
 //// [dynamicImportTrailingComma.js]
+var _a;
 var path = './foo';
-Promise.resolve().then(function () { return require(path); });
+_a = path, Promise.resolve().then(function () { return require(_a); });
diff --git a/tests/baselines/reference/importCallExpressionDeclarationEmit1.js b/tests/baselines/reference/importCallExpressionDeclarationEmit1.js
@@ -15,12 +15,14 @@ function returnDynamicLoad(path: string) {
 }
 
 //// [importCallExpressionDeclarationEmit1.js]
-Promise.resolve().then(() => require(getSpecifier()));
-var p0 = Promise.resolve().then(() => require(`${directory}\\${moduleFile}`));
-var p1 = Promise.resolve().then(() => require(getSpecifier()));
-const p2 = Promise.resolve().then(() => require(whatToLoad ? getSpecifier() : "defaulPath"));
+var _a, _b, _c, _d;
+_a = getSpecifier(), Promise.resolve().then(() => require(_a));
+var p0 = (_b = `${directory}\\${moduleFile}`, Promise.resolve().then(() => require(_b)));
+var p1 = (_c = getSpecifier(), Promise.resolve().then(() => require(_c)));
+const p2 = (_d = whatToLoad ? getSpecifier() : "defaulPath", Promise.resolve().then(() => require(_d)));
 function returnDynamicLoad(path) {
-    return Promise.resolve().then(() => require(path));
+    var _a;
+    return _a = path, Promise.resolve().then(() => require(_a));
 }
 
 

diff --git a/tests/baselines/reference/importCallExpressionGrammarError.js b/tests/baselines/reference/importCallExpressionGrammarError.js
@@ -10,8 +10,9 @@ const p2 = import();
 const p4 = import("pathToModule", "secondModule");
 
 //// [importCallExpressionGrammarError.js]
+var _a, _b;
 var a = ["./0"];
-Promise.resolve().then(() => require(...["PathModule"]));
-var p1 = Promise.resolve().then(() => require(...a));
+_a = (...["PathModule"]), Promise.resolve().then(() => require(_a));
+var p1 = (_b = (...a), Promise.resolve().then(() => require(_b)));
 const p2 = Promise.resolve().then(() => require());
 const p4 = Promise.resolve().then(() => require("pathToModule"));
diff --git a/tests/baselines/reference/importCallExpressionNestedCJS.js b/tests/baselines/reference/importCallExpressionNestedCJS.js
@@ -24,6 +24,7 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, ge
 };
 function foo() {
     return __awaiter(this, void 0, void 0, function* () {
-        return yield Promise.resolve().then(() => require((yield Promise.resolve().then(() => require("./foo"))).default));
+        var _a;
+        return yield (_a = (yield Promise.resolve().then(() => require("./foo"))).default, Promise.resolve().then(() => require(_a)));
     });
 }
diff --git a/tests/baselines/reference/importCallExpressionNestedCJS2.js b/tests/baselines/reference/importCallExpressionNestedCJS2.js
@@ -52,9 +52,10 @@ var __generator = (this && this.__generator) || function (thisArg, body) {
 function foo() {
     return __awaiter(this, void 0, void 0, function () {
         return __generator(this, function (_a) {
+            var _b;
             switch (_a.label) {
                 case 0: return [4 /*yield*/, Promise.resolve().then(function () { return require("./foo"); })];
-                case 1: return [4 /*yield*/, Promise.resolve().then(function () { return require((_a.sent()).default); })];
+                case 1: return [4 /*yield*/, (_b = (_a.sent()).default, Promise.resolve().then(function () { return require(_b); }))];
                 case 2: return [2 /*return*/, _a.sent()];
             }
         });

diff --git a/tests/baselines/reference/importCallExpressionReturnPromiseOfAny.js b/tests/baselines/reference/importCallExpressionReturnPromiseOfAny.js
@@ -42,21 +42,23 @@ class C {
 exports.C = C;
 //// [1.js]
 "use strict";
+var _a, _b, _c, _d, _e, _f, _g;
 Object.defineProperty(exports, "__esModule", { value: true });
-Promise.resolve().then(() => require(`${directory}\\${moduleFile}`));
-Promise.resolve().then(() => require(getSpecifier()));
-var p1 = Promise.resolve().then(() => require(ValidSomeCondition() ? "./0" : "externalModule"));
-var p1 = Promise.resolve().then(() => require(getSpecifier()));
-var p11 = Promise.resolve().then(() => require(getSpecifier()));
-const p2 = Promise.resolve().then(() => require(whatToLoad ? getSpecifier() : "defaulPath"));
+_a = `${directory}\\${moduleFile}`, Promise.resolve().then(() => require(_a));
+_b = getSpecifier(), Promise.resolve().then(() => require(_b));
+var p1 = (_c = ValidSomeCondition() ? "./0" : "externalModule", Promise.resolve().then(() => require(_c)));
+var p1 = (_d = getSpecifier(), Promise.resolve().then(() => require(_d)));
+var p11 = (_e = getSpecifier(), Promise.resolve().then(() => require(_e)));
+const p2 = (_f = whatToLoad ? getSpecifier() : "defaulPath", Promise.resolve().then(() => require(_f)));
 p1.then(zero => {
     return zero.foo(); // ok, zero is any
 });
 let j;
-var p3 = Promise.resolve().then(() => require(j = getSpecifier()));
+var p3 = (_g = j = getSpecifier(), Promise.resolve().then(() => require(_g)));
 function* loadModule(directories) {
+    var _a;
     for (const directory of directories) {
         const path = `${directory}\\moduleFile`;
-        Promise.resolve().then(() => require(yield path));
+        _a = yield path, Promise.resolve().then(() => require(_a));
     }
 }
diff --git a/tests/baselines/reference/importCallExpressionSpecifierNotStringTypeError.js b/tests/baselines/reference/importCallExpressionSpecifierNotStringTypeError.js
@@ -14,12 +14,13 @@ var p3 = import(["path1", "path2"]);
 var p4 = import(()=>"PathToModule");
 
 //// [importCallExpressionSpecifierNotStringTypeError.js]
+var _a, _b, _c, _d, _e;
 // Error specifier is not assignable to string
-Promise.resolve().then(() => require(getSpecifier()));
-var p1 = Promise.resolve().then(() => require(getSpecifier()));
-const p2 = Promise.resolve().then(() => require(whatToLoad ? getSpecifier() : "defaulPath"));
+_a = getSpecifier(), Promise.resolve().then(() => require(_a));
+var p1 = (_b = getSpecifier(), Promise.resolve().then(() => require(_b)));
+const p2 = (_c = whatToLoad ? getSpecifier() : "defaulPath", Promise.resolve().then(() => require(_c)));
 p1.then(zero => {
     return zero.foo(); // ok, zero is any
 });
-var p3 = Promise.resolve().then(() => require(["path1", "path2"]));
-var p4 = Promise.resolve().then(() => require(() => "PathToModule"));
+var p3 = (_d = ["path1", "path2"], Promise.resolve().then(() => require(_d)));
+var p4 = (_e = () => "PathToModule", Promise.resolve().then(() => require(_e)));
diff --git a/tests/baselines/reference/jsdocInTypeScript.js b/tests/baselines/reference/jsdocInTypeScript.js
@@ -58,6 +58,7 @@ var v = import(String());
 
 
 //// [jsdocInTypeScript.js]
+var _a;
 var T = /** @class */ (function () {
     function T() {
     }
@@ -92,4 +93,4 @@ var E = {};
 E[""];
 // make sure import types in JSDoc are not resolved
 /** @type {import("should-not-be-resolved").Type} */
-var v = Promise.resolve().then(function () { return require(String()); });
+var v = (_a = String(), Promise.resolve().then(function () { return require(_a); }));
diff --git a/tests/cases/compiler/dynamicImportEvaluateSpecifier.ts b/tests/cases/compiler/dynamicImportEvaluateSpecifier.ts
@@ -0,0 +1,17 @@
+// @lib: es2019
+// @target: es2019
+// @module: commonjs
+// https://github.com/microsoft/TypeScript/issues/48285
+let i = 0;
+
+import(String(i++));
+import(String(i++));
+
+const getPath = async () => {
+	/* in reality this would do some async FS operation, or a web request */
+	return "/root/my/cool/path";
+};
+
+const someFunction = async () => {
+	const result = await import(await getPath());
+};