From 1f8959f5dc04b2b2c2fc8a7dc53b6ac761e1f754 Mon Sep 17 00:00:00 2001 From: Joshua Chen Date: Tue, 18 Oct 2022 16:46:51 -0400 Subject: [PATCH] fix: avoid downleveled dynamic import closing over specifier expression (#49663) * fix: evaluate dynamic import specifier expressions synchronously * refactor * Update src/compiler/transformers/module/module.ts Co-authored-by: Ron Buckton * [Experiment] Co-authored-by: Ron Buckton --- src/compiler/transformers/module/module.ts | 38 +++++++++------- .../reference/asyncImportNestedYield.js | 5 ++- .../dynamicImportEvaluateSpecifier.js | 31 +++++++++++++ .../dynamicImportEvaluateSpecifier.symbols | 29 ++++++++++++ .../dynamicImportEvaluateSpecifier.types | 44 +++++++++++++++++++ .../reference/dynamicImportTrailingComma.js | 3 +- .../importCallExpressionDeclarationEmit1.js | 12 ++--- .../importCallExpressionGrammarError.js | 5 ++- .../importCallExpressionNestedCJS.js | 3 +- .../importCallExpressionNestedCJS2.js | 3 +- .../importCallExpressionReturnPromiseOfAny.js | 18 ++++---- ...llExpressionSpecifierNotStringTypeError.js | 11 ++--- .../baselines/reference/jsdocInTypeScript.js | 3 +- .../dynamicImportEvaluateSpecifier.ts | 17 +++++++ 14 files changed, 180 insertions(+), 42 deletions(-) create mode 100644 tests/baselines/reference/dynamicImportEvaluateSpecifier.js create mode 100644 tests/baselines/reference/dynamicImportEvaluateSpecifier.symbols create mode 100644 tests/baselines/reference/dynamicImportEvaluateSpecifier.types create mode 100644 tests/cases/compiler/dynamicImportEvaluateSpecifier.ts diff --git a/src/compiler/transformers/module/module.ts b/src/compiler/transformers/module/module.ts index 9b3b8e6ee1283..fbab915e871d4 100644 --- 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 index b92065950fbf3..a9d64fbd3fa96 100644 --- 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 new file mode 100644 index 0000000000000..60c4a9f3d1f44 --- /dev/null +++ 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 new file mode 100644 index 0000000000000..0ce2c94fdd31e --- /dev/null +++ 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 new file mode 100644 index 0000000000000..20443b78f9294 --- /dev/null +++ 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 +>String(i++) : string +>String : StringConstructor +>i++ : number +>i : number + +import(String(i++)); +>import(String(i++)) : Promise +>String(i++) : string +>String : StringConstructor +>i++ : number +>i : number + +const getPath = async () => { +>getPath : () => Promise +>async () => { /* in reality this would do some async FS operation, or a web request */ return "/root/my/cool/path";} : () => Promise + + /* 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 +>async () => { const result = await import(await getPath());} : () => Promise + + const result = await import(await getPath()); +>result : any +>await import(await getPath()) : any +>import(await getPath()) : Promise +>await getPath() : string +>getPath() : Promise +>getPath : () => Promise + +}; + diff --git a/tests/baselines/reference/dynamicImportTrailingComma.js b/tests/baselines/reference/dynamicImportTrailingComma.js index bbbc9794af746..d28e9be57761d 100644 --- 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 index 154e2f0a5e9f4..89e42ee7f63de 100644 --- 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 index 61b47759581ff..0a502e5e2891a 100644 --- 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 index 5b6abef5e0e65..f099c9bc5fc53 100644 --- 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 index 104a01b5bd946..b4dc9745b413f 100644 --- 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 index b7db3e18d22d4..b3d787c94a54e 100644 --- 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 index 5e2ace1c4017f..ab2785a63d208 100644 --- 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 index 037d655ccf030..39b8f917e0585 100644 --- 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 new file mode 100644 index 0000000000000..0acd8b118810e --- /dev/null +++ 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()); +};