From d76761dfbcf42886662eba0f7f18778ed4737864 Mon Sep 17 00:00:00 2001 From: Tobias Koppers Date: Fri, 21 Feb 2020 16:28:13 +0100 Subject: [PATCH] hoist exports to the top of a concatenated module to handle circular dependencies with non-concatenated modules fixes #10409 --- lib/optimize/ConcatenatedModule.js | 291 ++++++++---------- .../__snapshots__/StatsTestCases.test.js.snap | 24 +- .../cases/errors/harmony-import-missing2/a.js | 1 + .../errors/harmony-import-missing2/errors.js | 4 + .../errors/harmony-import-missing2/index.js | 11 + .../errors/harmony-import-missing2/module1.js | 2 + .../errors/harmony-import-missing2/module2.js | 2 + .../circular-root-export/external-ref.js | 1 + .../circular-root-export/external.js | 13 + .../circular-root-export/index.js | 7 + .../circular-root-export/module.js | 6 + .../circular-root-export/root-ref.js | 1 + .../circular-root-export/root.js | 13 + test/cases/scope-hoisting/issue-10409/a.js | 2 + test/cases/scope-hoisting/issue-10409/b.js | 2 + test/cases/scope-hoisting/issue-10409/c.js | 3 + test/cases/scope-hoisting/issue-10409/cts.js | 6 + .../cases/scope-hoisting/issue-10409/index.js | 5 + test/cases/scope-hoisting/issue-10409/main.js | 2 + 19 files changed, 225 insertions(+), 171 deletions(-) create mode 100644 test/cases/errors/harmony-import-missing2/a.js create mode 100644 test/cases/errors/harmony-import-missing2/errors.js create mode 100644 test/cases/errors/harmony-import-missing2/index.js create mode 100644 test/cases/errors/harmony-import-missing2/module1.js create mode 100644 test/cases/errors/harmony-import-missing2/module2.js create mode 100644 test/cases/scope-hoisting/circular-root-export/external-ref.js create mode 100644 test/cases/scope-hoisting/circular-root-export/external.js create mode 100644 test/cases/scope-hoisting/circular-root-export/index.js create mode 100644 test/cases/scope-hoisting/circular-root-export/module.js create mode 100644 test/cases/scope-hoisting/circular-root-export/root-ref.js create mode 100644 test/cases/scope-hoisting/circular-root-export/root.js create mode 100644 test/cases/scope-hoisting/issue-10409/a.js create mode 100644 test/cases/scope-hoisting/issue-10409/b.js create mode 100644 test/cases/scope-hoisting/issue-10409/c.js create mode 100644 test/cases/scope-hoisting/issue-10409/cts.js create mode 100644 test/cases/scope-hoisting/issue-10409/index.js create mode 100644 test/cases/scope-hoisting/issue-10409/main.js diff --git a/lib/optimize/ConcatenatedModule.js b/lib/optimize/ConcatenatedModule.js index c56fc6bcece..df4d216b129 100644 --- a/lib/optimize/ConcatenatedModule.js +++ b/lib/optimize/ConcatenatedModule.js @@ -22,6 +22,23 @@ const createHash = require("../util/createHash"); /** @typedef {import("../Dependency")} Dependency */ /** @typedef {import("../Compilation")} Compilation */ /** @typedef {import("../util/createHash").Hash} Hash */ +/** @typedef {import("../RequestShortener")} RequestShortener */ + +const joinIterableWithComma = iterable => { + // This is more performant than Array.from().join(", ") + // as it doesn't create an array + let str = ""; + let first = true; + for (const item of iterable) { + if (first) { + first = false; + } else { + str += ", "; + } + str += item; + } + return str; +}; /** * @typedef {Object} ConcatenationEntry @@ -287,6 +304,41 @@ const getPathInAst = (ast, node) => { } }; +const getHarmonyExportImportedSpecifierDependencyExports = dep => { + const importModule = dep._module; + if (!importModule) return []; + if (dep._id) { + // export { named } from "module" + return [ + { + name: dep.name, + id: dep._id, + module: importModule + } + ]; + } + if (dep.name) { + // export * as abc from "module" + return [ + { + name: dep.name, + id: true, + module: importModule + } + ]; + } + // export * from "module" + return importModule.buildMeta.providedExports + .filter(exp => exp !== "default" && !dep.activeExports.has(exp)) + .map(exp => { + return { + name: exp, + id: exp, + module: importModule + }; + }); +}; + class ConcatenatedModule extends Module { constructor(rootModule, modules, concatenationList) { super("javascript/esm", null); @@ -626,10 +678,7 @@ class ConcatenatedModule extends Module { ); innerDependencyTemplates.set( HarmonyExportSpecifierDependency, - new HarmonyExportSpecifierDependencyConcatenatedTemplate( - dependencyTemplates.get(HarmonyExportSpecifierDependency), - this.rootModule - ) + new NullTemplate() ); innerDependencyTemplates.set( HarmonyExportExpressionDependency, @@ -640,19 +689,11 @@ class ConcatenatedModule extends Module { ); innerDependencyTemplates.set( HarmonyExportImportedSpecifierDependency, - new HarmonyExportImportedSpecifierDependencyConcatenatedTemplate( - dependencyTemplates.get(HarmonyExportImportedSpecifierDependency), - this.rootModule, - moduleToInfoMap - ) + new NullTemplate() ); innerDependencyTemplates.set( HarmonyCompatibilityDependency, - new HarmonyCompatibilityDependencyConcatenatedTemplate( - dependencyTemplates.get(HarmonyCompatibilityDependency), - this.rootModule, - moduleToInfoMap - ) + new NullTemplate() ); // Must use full identifier in our cache here to ensure that the source @@ -1105,11 +1146,62 @@ class ConcatenatedModule extends Module { } } + // Map with all root exposed used exports + /** @type {Map} */ + const exportsMap = new Map(); + + // Set with all root exposed unused exports + /** @type {Set} */ + const unusedExports = new Set(); + + for (const dep of this.rootModule.dependencies) { + if (dep instanceof HarmonyExportSpecifierDependency) { + const used = this.rootModule.isUsed(dep.name); + if (used) { + const info = moduleToInfoMap.get(this.rootModule); + if (!exportsMap.has(used)) { + exportsMap.set( + used, + () => `/* binding */ ${info.internalNames.get(dep.id)}` + ); + } + } else { + unusedExports.add(dep.name || "namespace"); + } + } else if (dep instanceof HarmonyExportImportedSpecifierDependency) { + const exportDefs = getHarmonyExportImportedSpecifierDependencyExports( + dep + ); + for (const def of exportDefs) { + const info = moduleToInfoMap.get(def.module); + const used = dep.originModule.isUsed(def.name); + if (used) { + if (!exportsMap.has(used)) { + exportsMap.set(used, requestShortener => { + const finalName = getFinalName( + info, + def.id, + moduleToInfoMap, + requestShortener, + false, + this.rootModule.buildMeta.strictHarmonyModule + ); + return `/* reexport */ ${finalName}`; + }); + } + } else { + unusedExports.add(def.name); + } + } + } + } + const result = new ConcatSource(); // add harmony compatibility flag (must be first because of possible circular dependencies) const usedExports = this.rootModule.usedExports; if (usedExports === true || usedExports === null) { + result.add(`// ESM COMPAT FLAG\n`); result.add( runtimeTemplate.defineEsModuleFlagStatement({ exportsArgument: this.exportsArgument @@ -1117,9 +1209,33 @@ class ConcatenatedModule extends Module { ); } + // define exports + if (exportsMap.size > 0) { + result.add(`\n// EXPORTS\n`); + for (const [key, value] of exportsMap) { + result.add( + `__webpack_require__.d(${this.exportsArgument}, ${JSON.stringify( + key + )}, function() { return ${value(requestShortener)}; });\n` + ); + } + } + + // list unused exports + if (unusedExports.size > 0) { + result.add( + `\n// UNUSED EXPORTS: ${joinIterableWithComma(unusedExports)}\n` + ); + } + // define required namespace objects (must be before evaluation modules) for (const info of modulesWithInfo) { if (info.namespaceObjectSource) { + result.add( + `\n// NAMESPACE OBJECT: ${info.module.readableIdentifier( + requestShortener + )}\n` + ); result.add(info.namespaceObjectSource); } } @@ -1321,38 +1437,6 @@ class HarmonyImportSideEffectDependencyConcatenatedTemplate { } } -class HarmonyExportSpecifierDependencyConcatenatedTemplate { - constructor(originalTemplate, rootModule) { - this.originalTemplate = originalTemplate; - this.rootModule = rootModule; - } - - getHarmonyInitOrder(dep) { - if (dep.originModule === this.rootModule) { - return this.originalTemplate.getHarmonyInitOrder(dep); - } - return NaN; - } - - harmonyInit(dep, source, runtime, dependencyTemplates) { - if (dep.originModule === this.rootModule) { - this.originalTemplate.harmonyInit( - dep, - source, - runtime, - dependencyTemplates - ); - return; - } - } - - apply(dep, source, runtime, dependencyTemplates) { - if (dep.originModule === this.rootModule) { - this.originalTemplate.apply(dep, source, runtime, dependencyTemplates); - } - } -} - class HarmonyExportExpressionDependencyConcatenatedTemplate { constructor(originalTemplate, rootModule) { this.originalTemplate = originalTemplate; @@ -1386,119 +1470,8 @@ class HarmonyExportExpressionDependencyConcatenatedTemplate { } } -class HarmonyExportImportedSpecifierDependencyConcatenatedTemplate { - constructor(originalTemplate, rootModule, modulesMap) { - this.originalTemplate = originalTemplate; - this.rootModule = rootModule; - this.modulesMap = modulesMap; - } - - getExports(dep) { - const importModule = dep._module; - if (dep._id) { - // export { named } from "module" - return [ - { - name: dep.name, - id: dep._id, - module: importModule - } - ]; - } - if (dep.name) { - // export * as abc from "module" - return [ - { - name: dep.name, - id: true, - module: importModule - } - ]; - } - // export * from "module" - return importModule.buildMeta.providedExports - .filter(exp => exp !== "default" && !dep.activeExports.has(exp)) - .map(exp => { - return { - name: exp, - id: exp, - module: importModule - }; - }); - } - - getHarmonyInitOrder(dep) { - const module = dep._module; - const info = this.modulesMap.get(module); - if (!info) { - return this.originalTemplate.getHarmonyInitOrder(dep); - } - return NaN; - } - - harmonyInit(dep, source, runtime, dependencyTemplates) { - const module = dep._module; - const info = this.modulesMap.get(module); - if (!info) { - this.originalTemplate.harmonyInit( - dep, - source, - runtime, - dependencyTemplates - ); - return; - } - } - - apply(dep, source, runtime, dependencyTemplates) { - if (dep.originModule === this.rootModule) { - if (this.modulesMap.get(dep._module)) { - const exportDefs = this.getExports(dep); - for (const def of exportDefs) { - const info = this.modulesMap.get(def.module); - const used = dep.originModule.isUsed(def.name); - if (!used) { - source.insert( - -1, - `/* unused concated harmony import ${def.name} */\n` - ); - continue; - } - let finalName; - const strictFlag = dep.originModule.buildMeta.strictHarmonyModule - ? "_strict" - : ""; - if (def.id === true) { - finalName = `__WEBPACK_MODULE_REFERENCE__${info.index}_ns${strictFlag}__`; - } else { - const exportData = Buffer.from(def.id, "utf-8").toString("hex"); - finalName = `__WEBPACK_MODULE_REFERENCE__${info.index}_${exportData}${strictFlag}__`; - } - const exportsName = this.rootModule.exportsArgument; - const content = - `/* concated harmony reexport ${def.name} */` + - `__webpack_require__.d(${exportsName}, ` + - `${JSON.stringify(used)}, ` + - `function() { return ${finalName}; });\n`; - source.insert(-1, content); - } - } else { - this.originalTemplate.apply(dep, source, runtime, dependencyTemplates); - } - } - } -} - -class HarmonyCompatibilityDependencyConcatenatedTemplate { - constructor(originalTemplate, rootModule, modulesMap) { - this.originalTemplate = originalTemplate; - this.rootModule = rootModule; - this.modulesMap = modulesMap; - } - - apply(dep, source, runtime, dependencyTemplates) { - // do nothing - } +class NullTemplate { + apply() {} } module.exports = ConcatenatedModule; diff --git a/test/__snapshots__/StatsTestCases.test.js.snap b/test/__snapshots__/StatsTestCases.test.js.snap index 811e0a245a0..d67186b3f39 100644 --- a/test/__snapshots__/StatsTestCases.test.js.snap +++ b/test/__snapshots__/StatsTestCases.test.js.snap @@ -651,7 +651,7 @@ Child Time: Xms Built at: Thu Jan 01 1970 00:00:00 GMT Asset Size Chunks Chunk Names - app.js 6.76 KiB 0 [emitted] app + app.js 6.78 KiB 0 [emitted] app vendor.aa94f0c872c214f6cb2e.js 619 bytes 1 [emitted] [immutable] vendor Entrypoint app = vendor.aa94f0c872c214f6cb2e.js app.js [./constants.js] 87 bytes {1} [built] @@ -664,7 +664,7 @@ Child Time: Xms Built at: Thu Jan 01 1970 00:00:00 GMT Asset Size Chunks Chunk Names - app.js 6.78 KiB 0 [emitted] app + app.js 6.8 KiB 0 [emitted] app vendor.aa94f0c872c214f6cb2e.js 619 bytes 1 [emitted] [immutable] vendor Entrypoint app = vendor.aa94f0c872c214f6cb2e.js app.js [./constants.js] 87 bytes {1} [built] @@ -699,8 +699,8 @@ Child Time: Xms Built at: Thu Jan 01 1970 00:00:00 GMT Asset Size Chunks Chunk Names - 1-4dcf1f7bb7b9da3c54c7.js 424 bytes 1 [emitted] [immutable] - 1-4dcf1f7bb7b9da3c54c7.js.map 336 bytes 1 [emitted] [dev] + 1-4dcf1f7bb7b9da3c54c7.js 443 bytes 1 [emitted] [immutable] + 1-4dcf1f7bb7b9da3c54c7.js.map 337 bytes 1 [emitted] [dev] main-95e076639e5472415472.js 8.32 KiB 0 [emitted] [immutable] main main-95e076639e5472415472.js.map 8.32 KiB 0 [emitted] [dev] main Entrypoint main = main-95e076639e5472415472.js main-95e076639e5472415472.js.map @@ -713,8 +713,8 @@ Child Time: Xms Built at: Thu Jan 01 1970 00:00:00 GMT Asset Size Chunks Chunk Names - 1-4dcf1f7bb7b9da3c54c7.js 424 bytes 1 [emitted] [immutable] - 1-4dcf1f7bb7b9da3c54c7.js.map 336 bytes 1 [emitted] [dev] + 1-4dcf1f7bb7b9da3c54c7.js 443 bytes 1 [emitted] [immutable] + 1-4dcf1f7bb7b9da3c54c7.js.map 337 bytes 1 [emitted] [dev] main-95e076639e5472415472.js 8.32 KiB 0 [emitted] [immutable] main main-95e076639e5472415472.js.map 8.32 KiB 0 [emitted] [dev] main Entrypoint main = main-95e076639e5472415472.js main-95e076639e5472415472.js.map @@ -727,7 +727,7 @@ Child Time: Xms Built at: Thu Jan 01 1970 00:00:00 GMT Asset Size Chunks Chunk Names - 1-4dcf1f7bb7b9da3c54c7.js 940 bytes 1 [emitted] [immutable] + 1-4dcf1f7bb7b9da3c54c7.js 960 bytes 1 [emitted] [immutable] main-95e076639e5472415472.js 8.66 KiB 0 [emitted] [immutable] main Entrypoint main = main-95e076639e5472415472.js [0] ./a/index.js 40 bytes {0} [built] @@ -739,7 +739,7 @@ Child Time: Xms Built at: Thu Jan 01 1970 00:00:00 GMT Asset Size Chunks Chunk Names - 1-4dcf1f7bb7b9da3c54c7.js 940 bytes 1 [emitted] [immutable] + 1-4dcf1f7bb7b9da3c54c7.js 960 bytes 1 [emitted] [immutable] main-95e076639e5472415472.js 8.66 KiB 0 [emitted] [immutable] main Entrypoint main = main-95e076639e5472415472.js [0] ./b/index.js 40 bytes {0} [built] @@ -1677,7 +1677,7 @@ chunk {7} main.js (main) 523 bytes >{0}< >{1}< >{2}< >{5}< [entry] [rendered] exports[`StatsTestCases should print correct stats for parse-error 1`] = ` " Asset Size Chunks Chunk Names -main.js 4.11 KiB 0 main +main.js 4.13 KiB 0 main Entrypoint main = main.js [0] ./b.js 271 bytes {0} [built] [failed] [1 error] [1] ./index.js + 1 modules 35 bytes {0} [built] @@ -2419,7 +2419,7 @@ Time: Xms Built at: Thu Jan 01 1970 00:00:00 GMT Asset Size Chunks Chunk Names 1.js 481 bytes 1 [emitted] -main.js 9.51 KiB 0 [emitted] main +main.js 9.53 KiB 0 [emitted] main Entrypoint main = main.js [0] ./components/src/CompAB/index.js 87 bytes [built] [no exports used] @@ -2466,8 +2466,8 @@ exports[`StatsTestCases should print correct stats for side-effects-simple-unuse "Hash: a5866e21e8cfa3ae1a89 Time: Xms Built at: Thu Jan 01 1970 00:00:00 GMT - Asset Size Chunks Chunk Names -main.js 3.9 KiB 0 [emitted] main + Asset Size Chunks Chunk Names +main.js 3.92 KiB 0 [emitted] main Entrypoint main = main.js [0] ./node_modules/pmodule/b.js 69 bytes [built] [no exports used] diff --git a/test/cases/errors/harmony-import-missing2/a.js b/test/cases/errors/harmony-import-missing2/a.js new file mode 100644 index 00000000000..173df5cb056 --- /dev/null +++ b/test/cases/errors/harmony-import-missing2/a.js @@ -0,0 +1 @@ +export var test = "test"; diff --git a/test/cases/errors/harmony-import-missing2/errors.js b/test/cases/errors/harmony-import-missing2/errors.js new file mode 100644 index 00000000000..67d1f2321a3 --- /dev/null +++ b/test/cases/errors/harmony-import-missing2/errors.js @@ -0,0 +1,4 @@ +module.exports = [ + [/Can't resolve '.\/missing1'/], + [/Can't resolve '.\/missing2'/] +]; diff --git a/test/cases/errors/harmony-import-missing2/index.js b/test/cases/errors/harmony-import-missing2/index.js new file mode 100644 index 00000000000..77cb8d2ed72 --- /dev/null +++ b/test/cases/errors/harmony-import-missing2/index.js @@ -0,0 +1,11 @@ +it("should not crash on importing missing modules", function() { + expect(function() { + require("./module1"); + }).toThrowError(); +}); + +it("should not crash on importing missing modules", function() { + expect(function() { + require("./module2"); + }).toThrowError(); +}); diff --git a/test/cases/errors/harmony-import-missing2/module1.js b/test/cases/errors/harmony-import-missing2/module1.js new file mode 100644 index 00000000000..cc6275d1650 --- /dev/null +++ b/test/cases/errors/harmony-import-missing2/module1.js @@ -0,0 +1,2 @@ +export * from "./missing1"; +export * from "./a?1"; diff --git a/test/cases/errors/harmony-import-missing2/module2.js b/test/cases/errors/harmony-import-missing2/module2.js new file mode 100644 index 00000000000..3b41b648333 --- /dev/null +++ b/test/cases/errors/harmony-import-missing2/module2.js @@ -0,0 +1,2 @@ +export { a } from "./missing2"; +export * from "./a?2"; diff --git a/test/cases/scope-hoisting/circular-root-export/external-ref.js b/test/cases/scope-hoisting/circular-root-export/external-ref.js new file mode 100644 index 00000000000..1ef8a6f61e6 --- /dev/null +++ b/test/cases/scope-hoisting/circular-root-export/external-ref.js @@ -0,0 +1 @@ +import "./external"; diff --git a/test/cases/scope-hoisting/circular-root-export/external.js b/test/cases/scope-hoisting/circular-root-export/external.js new file mode 100644 index 00000000000..8f187dd37a0 --- /dev/null +++ b/test/cases/scope-hoisting/circular-root-export/external.js @@ -0,0 +1,13 @@ +import { a, b, c, default as d } from "./root"; + +expect(a()).toBe("a"); +if (process.env.NODE_ENV === "production") { + // These two cases only work correctly when scope hoisted + expect(b()).toBe("b"); + expect(Object(c).b()).toBe("b"); +} +expect(d).toBe(undefined); + +export function test() { + expect(d).toBe(d); +} diff --git a/test/cases/scope-hoisting/circular-root-export/index.js b/test/cases/scope-hoisting/circular-root-export/index.js new file mode 100644 index 00000000000..82c6f8fa9cf --- /dev/null +++ b/test/cases/scope-hoisting/circular-root-export/index.js @@ -0,0 +1,7 @@ +it("should hoist exports in a concatenated module", () => { + return import("./root-ref").then(m => { + m.test(); + }); +}); + +if (Math.random() < 0) import("./external-ref"); diff --git a/test/cases/scope-hoisting/circular-root-export/module.js b/test/cases/scope-hoisting/circular-root-export/module.js new file mode 100644 index 00000000000..6ff994e5f04 --- /dev/null +++ b/test/cases/scope-hoisting/circular-root-export/module.js @@ -0,0 +1,6 @@ +export function b() { + return "b"; +} +export function bb() { + return "bb"; +} diff --git a/test/cases/scope-hoisting/circular-root-export/root-ref.js b/test/cases/scope-hoisting/circular-root-export/root-ref.js new file mode 100644 index 00000000000..63a76f1cbc8 --- /dev/null +++ b/test/cases/scope-hoisting/circular-root-export/root-ref.js @@ -0,0 +1 @@ +export { test } from "./root"; diff --git a/test/cases/scope-hoisting/circular-root-export/root.js b/test/cases/scope-hoisting/circular-root-export/root.js new file mode 100644 index 00000000000..96a61a86c51 --- /dev/null +++ b/test/cases/scope-hoisting/circular-root-export/root.js @@ -0,0 +1,13 @@ +export { test } from "./external"; +import * as c from "./module"; +export { c }; +import * as cc from "./module"; +export { cc }; +export * from "./module"; +export default "d"; +export function a() { + return "a"; +} +export function aa() { + return "aa"; +} diff --git a/test/cases/scope-hoisting/issue-10409/a.js b/test/cases/scope-hoisting/issue-10409/a.js new file mode 100644 index 00000000000..087e90023e4 --- /dev/null +++ b/test/cases/scope-hoisting/issue-10409/a.js @@ -0,0 +1,2 @@ +import cts from "./cts"; +export default cts.connectData(function() {}); diff --git a/test/cases/scope-hoisting/issue-10409/b.js b/test/cases/scope-hoisting/issue-10409/b.js new file mode 100644 index 00000000000..6ad5255ada8 --- /dev/null +++ b/test/cases/scope-hoisting/issue-10409/b.js @@ -0,0 +1,2 @@ +import cts from "./cts"; +export function b() {} diff --git a/test/cases/scope-hoisting/issue-10409/c.js b/test/cases/scope-hoisting/issue-10409/c.js new file mode 100644 index 00000000000..99eff168d1a --- /dev/null +++ b/test/cases/scope-hoisting/issue-10409/c.js @@ -0,0 +1,3 @@ +import cts from "./cts"; +import a from "./a"; +export function c() {} diff --git a/test/cases/scope-hoisting/issue-10409/cts.js b/test/cases/scope-hoisting/issue-10409/cts.js new file mode 100644 index 00000000000..df577722c0a --- /dev/null +++ b/test/cases/scope-hoisting/issue-10409/cts.js @@ -0,0 +1,6 @@ +import * as cts from "./cts"; +export { cts as default }; +export function connectData() {} +export function yyy() {} +export { b } from "./b"; +export { c } from "./c"; diff --git a/test/cases/scope-hoisting/issue-10409/index.js b/test/cases/scope-hoisting/issue-10409/index.js new file mode 100644 index 00000000000..aee4b05ceeb --- /dev/null +++ b/test/cases/scope-hoisting/issue-10409/index.js @@ -0,0 +1,5 @@ +it("should import these modules correctly", () => { + return import("./main"); +}); + +if (Math.random() < 0) import("./b"); diff --git a/test/cases/scope-hoisting/issue-10409/main.js b/test/cases/scope-hoisting/issue-10409/main.js new file mode 100644 index 00000000000..c839869b169 --- /dev/null +++ b/test/cases/scope-hoisting/issue-10409/main.js @@ -0,0 +1,2 @@ +import cts from "./cts"; +import a from "./a";