Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: avoid downleveled dynamic import closing over specifier expression #49663

Merged
merged 4 commits into from Oct 18, 2022
Merged

fix: avoid downleveled dynamic import closing over specifier expression #49663

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e216c89
fix: evaluate dynamic import specifier expressions synchronously
Josh-Cena Jun 23, 2022
a4463a3
refactor
Josh-Cena Jun 29, 2022
b0576b2
Update src/compiler/transformers/module/module.ts
Josh-Cena Jul 31, 2022
3baf06d
[Experiment]
Josh-Cena Jul 31, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
40 changes: 24 additions & 16 deletions src/compiler/transformers/module/module.ts
View file
Open in desktop
Expand Up @@ -721,7 +721,7 @@ namespace ts {
return createImportCallExpressionUMD(argument ?? factory.createVoidZero(), containsLexicalThis);
case ModuleKind.CommonJS:
default:
return createImportCallExpressionCommonJS(argument, containsLexicalThis);
return createImportCallExpressionCommonJS(argument);
}
}

Expand All @@ -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)
);
Expand All @@ -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),
/*colonToken*/ undefined,
/*whenFalse*/ createImportCallExpressionAMD(temp, containsLexicalThis)
));
Expand Down Expand Up @@ -820,14 +820,25 @@ namespace ts {
return promise;
}

function createImportCallExpressionCommonJS(arg: Expression | undefined, containsLexicalThis: boolean): Expression {
// import("./blah")
function createImportCallExpressionCommonJS(arg: Expression | undefined): 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) ? 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);
}
Expand All @@ -851,16 +862,13 @@ 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);
}
// Note: because non-literal expressions are already evaluated
// synchronously, we don't need to capture `this`.
Josh-Cena marked this conversation as resolved.
Show resolved Hide resolved
}

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) {
Expand Down
5 changes: 3 additions & 2 deletions tests/baselines/reference/asyncImportNestedYield.js
View file
Open in desktop
Expand Up @@ -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*/];
}
});
Expand Down
31 changes: 31 additions & 0 deletions tests/baselines/reference/dynamicImportEvaluateSpecifier.js
View file
Open in desktop
@@ -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)));
};
29 changes: 29 additions & 0 deletions tests/baselines/reference/dynamicImportEvaluateSpecifier.symbols
View file
Open in desktop
@@ -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))

};

44 changes: 44 additions & 0 deletions tests/baselines/reference/dynamicImportEvaluateSpecifier.types
View file
Open in desktop
@@ -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>

};

3 changes: 2 additions & 1 deletion tests/baselines/reference/dynamicImportTrailingComma.js
View file
Open in desktop
Expand Up @@ -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); });
4 changes: 2 additions & 2 deletions tests/baselines/reference/dynamicImportWithNestedThis_es2015.js
View file
Open in desktop
Expand Up @@ -29,8 +29,8 @@ c.dynamic();
this._path = './other';
}
dynamic() {
var _a;
return _a = this._path, __syncRequire ? Promise.resolve().then(() => require(_a)) : new Promise((resolve_1, reject_1) => { require([_a], resolve_1, reject_1); });
var _a, _b;
return _a = this._path, __syncRequire ? (_b = _a, Promise.resolve().then(() => require(_b))) : new Promise((resolve_1, reject_1) => { require([_a], resolve_1, reject_1); });
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The additional aliasing here is unnecessary.

Copy link
Contributor Author

@Josh-Cena Josh-Cena Jul 31, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're correct. However I'm not sure if it's possible to be fixed on a generic level, since the temp variable created from the UMD transformer may be re-assigned. I've committed the new baselines in 3baf06d with the two unnecessary aliases fixed, but there are some other places where it looks suspicious. I can't really read the output to tell if it's unsafe, so I'll appreciate if you can help to review here.

Never mind, the temp variable will never be re-assigned because it isn't leaking anywhere

}
}
const c = new C();
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/dynamicImportWithNestedThis_es5.js
View file
Open in desktop
Expand Up @@ -29,8 +29,8 @@ c.dynamic();
this._path = './other';
}
C.prototype.dynamic = function () {
var _a;
return _a = this._path, __syncRequire ? Promise.resolve().then(function () { return require(_a); }) : new Promise(function (resolve_1, reject_1) { require([_a], resolve_1, reject_1); });
var _a, _b;
return _a = this._path, __syncRequire ? (_b = _a, Promise.resolve().then(function () { return require(_b); })) : new Promise(function (resolve_1, reject_1) { require([_a], resolve_1, reject_1); });
};
return C;
}());
Expand Down
12 changes: 7 additions & 5 deletions tests/baselines/reference/importCallExpressionDeclarationEmit1.js
View file
Open in desktop
Expand Up @@ -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));
}


Expand Down
5 changes: 3 additions & 2 deletions tests/baselines/reference/importCallExpressionGrammarError.js
View file
Open in desktop
Expand Up @@ -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)));
Comment on lines +15 to +16
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is invalid grammar, but the input is invalid grammar as well. I assume we can safely treat it as garbage in, garbage out?

const p2 = Promise.resolve().then(() => require());
const p4 = Promise.resolve().then(() => require("pathToModule"));
4 changes: 2 additions & 2 deletions tests/baselines/reference/importCallExpressionInUMD5.js
View file
Open in desktop
Expand Up @@ -51,9 +51,9 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, ge
// https://github.com/microsoft/TypeScript/issues/36780
function func() {
return __awaiter(this, void 0, void 0, function* () {
var _a;
var _a, _b;
const packageName = '.';
const packageJson = yield (_a = packageName + '/package.json', __syncRequire ? Promise.resolve().then(() => require(_a)) : new Promise((resolve_1, reject_1) => { require([_a], resolve_1, reject_1); }));
const packageJson = yield (_a = packageName + '/package.json', __syncRequire ? (_b = _a, Promise.resolve().then(() => require(_b))) : new Promise((resolve_1, reject_1) => { require([_a], resolve_1, reject_1); }));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The additional aliasing here is unnecessary.

});
}
});
3 changes: 2 additions & 1 deletion tests/baselines/reference/importCallExpressionNestedCJS.js
View file
Open in desktop
Expand Up @@ -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)));
});
}
3 changes: 2 additions & 1 deletion tests/baselines/reference/importCallExpressionNestedCJS2.js
View file
Open in desktop
Expand Up @@ -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()];
}
});
Expand Down
4 changes: 2 additions & 2 deletions tests/baselines/reference/importCallExpressionNestedUMD.js
View file
Open in desktop
Expand Up @@ -45,8 +45,8 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, P, ge
var __syncRequire = typeof module === "object" && typeof module.exports === "object";
function foo() {
return __awaiter(this, void 0, void 0, function* () {
var _a;
return yield (_a = (yield __syncRequire ? Promise.resolve().then(() => require("./foo")) : new Promise((resolve_1, reject_1) => { require(["./foo"], resolve_1, reject_1); })).default, __syncRequire ? Promise.resolve().then(() => require(_a)) : new Promise((resolve_2, reject_2) => { require([_a], resolve_2, reject_2); }));
var _a, _b;
return yield (_a = (yield __syncRequire ? Promise.resolve().then(() => require("./foo")) : new Promise((resolve_1, reject_1) => { require(["./foo"], resolve_1, reject_1); })).default, __syncRequire ? (_b = _a, Promise.resolve().then(() => require(_b))) : new Promise((resolve_2, reject_2) => { require([_a], resolve_2, reject_2); }));
});
}
});
4 changes: 2 additions & 2 deletions tests/baselines/reference/importCallExpressionNestedUMD2.js
View file
Open in desktop
Expand Up @@ -73,10 +73,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;
var _b, _c;
switch (_a.label) {
case 0: return [4 /*yield*/, __syncRequire ? Promise.resolve().then(function () { return require("./foo"); }) : new Promise(function (resolve_1, reject_1) { require(["./foo"], resolve_1, reject_1); })];
case 1: return [4 /*yield*/, (_b = (_a.sent()).default, __syncRequire ? Promise.resolve().then(function () { return require(_b); }) : new Promise(function (resolve_2, reject_2) { require([_b], resolve_2, reject_2); }))];
case 1: return [4 /*yield*/, (_b = (_a.sent()).default, __syncRequire ? (_c = _b, Promise.resolve().then(function () { return require(_c); })) : new Promise(function (resolve_2, reject_2) { require([_b], resolve_2, reject_2); }))];
case 2: return [2 /*return*/, _a.sent()];
}
});
Expand Down
18 changes: 10 additions & 8 deletions tests/baselines/reference/importCallExpressionReturnPromiseOfAny.js
View file
Open in desktop
Expand Up @@ -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));
}
}
11 changes: 6 additions & 5 deletions tests/baselines/reference/importCallExpressionSpecifierNotStringTypeError.js
View file
Open in desktop
Expand Up @@ -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)));
3 changes: 2 additions & 1 deletion tests/baselines/reference/jsdocInTypeScript.js
View file
Open in desktop
Expand Up @@ -58,6 +58,7 @@ var v = import(String());


//// [jsdocInTypeScript.js]
var _a;
var T = /** @class */ (function () {
function T() {
}
Expand Down Expand Up @@ -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); }));