From 41086b3b3076088c367eae381d96abcc4aa4e4d3 Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Mon, 24 Jan 2022 09:35:12 +0100 Subject: [PATCH] Merge conflict detection into export resolution --- src/ExternalModule.ts | 6 +- src/Graph.ts | 2 +- src/Module.ts | 199 +++++++----------- src/utils/error.ts | 23 +- .../namespace-import/_config.js | 2 +- .../samples/shims-missing-exports/dep2.js | 0 .../samples/shims-missing-exports/main.js | 2 +- .../warn-on-namespace-conflict/_config.js | 4 +- 8 files changed, 98 insertions(+), 140 deletions(-) delete mode 100644 test/function/samples/shims-missing-exports/dep2.js diff --git a/src/ExternalModule.ts b/src/ExternalModule.ts index 80e60e26fe1..8071e2c61d9 100644 --- a/src/ExternalModule.ts +++ b/src/ExternalModule.ts @@ -64,13 +64,13 @@ export default class ExternalModule { }; } - getVariableForExportName(name: string): ExternalVariable { + getVariableForExportName(name: string): [variable: ExternalVariable] { let declaration = this.declarations[name]; - if (declaration) return declaration; + if (declaration) return [declaration]; this.declarations[name] = declaration = new ExternalVariable(this, name); this.exportedVariables.set(declaration, name); - return declaration; + return [declaration]; } setRenderPath(options: NormalizedOutputOptions, inputBase: string): string { diff --git a/src/Graph.ts b/src/Graph.ts index 38fc7a0d7cc..df2ed2fb29c 100644 --- a/src/Graph.ts +++ b/src/Graph.ts @@ -243,7 +243,7 @@ export default class Graph { for (const importDescription of Object.values(module.importDescriptions)) { if ( importDescription.name !== '*' && - !importDescription.module.getVariableForExportName(importDescription.name) + !importDescription.module.getVariableForExportName(importDescription.name)[0] ) { module.warn( { diff --git a/src/Module.ts b/src/Module.ts index 26b19416526..92eee40b2c8 100644 --- a/src/Module.ts +++ b/src/Module.ts @@ -134,13 +134,12 @@ function getVariableForExportNameRecursive( name: string, importerForSideEffects: Module | undefined, isExportAllSearch: boolean, - searchedNamesAndModules = new Map>(), - skipExternalNamespaceReexports: boolean | undefined -): Variable | null { + searchedNamesAndModules = new Map>() +): [variable: Variable | null, indirectExternal?: boolean] { const searchedModules = searchedNamesAndModules.get(name); if (searchedModules) { if (searchedModules.has(target)) { - return isExportAllSearch ? null : error(errCircularReexport(name, target.id)); + return isExportAllSearch ? [null] : error(errCircularReexport(name, target.id)); } searchedModules.add(target); } else { @@ -149,8 +148,7 @@ function getVariableForExportNameRecursive( return target.getVariableForExportName(name, { importerForSideEffects, isExportAllSearch, - searchedNamesAndModules, - skipExternalNamespaceReexports + searchedNamesAndModules }); } @@ -200,7 +198,6 @@ export default class Module { execIndex = Infinity; readonly exportAllSources = new Set(); readonly exports: { [name: string]: ExportDescription } = Object.create(null); - readonly exportsAll: { [name: string]: string } = Object.create(null); readonly implicitlyLoadedAfter = new Set(); readonly implicitlyLoadedBefore = new Set(); readonly importDescriptions: { [name: string]: ImportDescription } = Object.create(null); @@ -235,7 +232,10 @@ export default class Module { private exportNamesByVariable: Map | null = null; private readonly exportShimVariable: ExportShimVariable = new ExportShimVariable(this); private declare magicString: MagicString; - private namespaceReexportsByName: Record = Object.create(null); + private namespaceReexportsByName: Record< + string, + [variable: Variable | null, indirectExternal?: boolean] + > = Object.create(null); private relevantDependencies: Set | null = null; private readonly syntheticExports = new Map(); private syntheticNamespace: Variable | null | undefined = null; @@ -369,7 +369,7 @@ export default class Module { this.implicitlyLoadedAfter.size > 0 ) { for (const exportName of [...this.getReexports(), ...this.getExports()]) { - const exportedVariable = this.getVariableForExportName(exportName); + const [exportedVariable] = this.getVariableForExportName(exportName); if (exportedVariable) { dependencyVariables.add(exportedVariable); } @@ -413,7 +413,7 @@ export default class Module { const exportNamesByVariable = new Map(); for (const exportName of this.getAllExportNames()) { if (exportName === this.info.syntheticNamedExports) continue; - let tracedVariable = this.getVariableForExportName(exportName); + let [tracedVariable] = this.getVariableForExportName(exportName); if (tracedVariable instanceof ExportDefaultVariable) { tracedVariable = tracedVariable.getOriginalVariable(); } @@ -465,7 +465,7 @@ export default class Module { const renderedExports: string[] = []; const removedExports: string[] = []; for (const exportName in this.exports) { - const variable = this.getVariableForExportName(exportName); + const [variable] = this.getVariableForExportName(exportName); (variable && variable.included ? renderedExports : removedExports).push(exportName); } return { removedExports, renderedExports }; @@ -474,7 +474,7 @@ export default class Module { getSyntheticNamespace(): Variable { if (this.syntheticNamespace === null) { this.syntheticNamespace = undefined; - this.syntheticNamespace = this.getVariableForExportName( + [this.syntheticNamespace] = this.getVariableForExportName( typeof this.info.syntheticNamedExports === 'string' ? this.info.syntheticNamedExports : 'default' @@ -493,19 +493,17 @@ export default class Module { { importerForSideEffects, isExportAllSearch, - searchedNamesAndModules, - skipExternalNamespaceReexports + searchedNamesAndModules }: { importerForSideEffects?: Module; isExportAllSearch?: boolean; searchedNamesAndModules?: Map>; - skipExternalNamespaceReexports?: boolean; } = EMPTY_OBJECT - ): Variable | null { + ): [variable: Variable | null, indirectExternal?: boolean] { if (name[0] === '*') { if (name.length === 1) { // export * from './other' - return this.namespace; + return [this.namespace]; } else { // export * from 'external' const module = this.graph.modulesById.get(name.slice(1)) as ExternalModule; @@ -516,13 +514,12 @@ export default class Module { // export { foo } from './other' const reexportDeclaration = this.reexportDescriptions[name]; if (reexportDeclaration) { - const variable = getVariableForExportNameRecursive( + const [variable] = getVariableForExportNameRecursive( reexportDeclaration.module, reexportDeclaration.localName, importerForSideEffects, false, - searchedNamesAndModules, - false + searchedNamesAndModules ); if (!variable) { @@ -534,13 +531,13 @@ export default class Module { if (importerForSideEffects) { setAlternativeExporterIfCyclic(variable, importerForSideEffects, this); } - return variable; + return [variable]; } const exportDeclaration = this.exports[name]; if (exportDeclaration) { if (exportDeclaration === MISSING_EXPORT_SHIM_DESCRIPTION) { - return this.exportShimVariable; + return [this.exportShimVariable]; } const name = exportDeclaration.localName; const variable = this.traceVariable(name, importerForSideEffects)!; @@ -552,7 +549,7 @@ export default class Module { ).add(this); setAlternativeExporterIfCyclic(variable, importerForSideEffects, this); } - return variable; + return [variable]; } if (name !== 'default') { @@ -562,30 +559,23 @@ export default class Module { : this.getVariableFromNamespaceReexports( name, importerForSideEffects, - searchedNamesAndModules, - skipExternalNamespaceReexports + searchedNamesAndModules ); - if (!skipExternalNamespaceReexports) { - this.namespaceReexportsByName[name] = foundNamespaceReexport; - } - if (foundNamespaceReexport) { + this.namespaceReexportsByName[name] = foundNamespaceReexport; + if (foundNamespaceReexport[0]) { return foundNamespaceReexport; } } if (this.info.syntheticNamedExports) { - let syntheticExport = this.syntheticExports.get(name); - if (!syntheticExport) { - const syntheticNamespace = this.getSyntheticNamespace(); - syntheticExport = new SyntheticNamedExportVariable( - this.astContext, + return [ + getOrCreate( + this.syntheticExports, name, - syntheticNamespace - ); - this.syntheticExports.set(name, syntheticExport); - return syntheticExport; - } - return syntheticExport; + () => + new SyntheticNamedExportVariable(this.astContext, name, this.getSyntheticNamespace()) + ) + ]; } // we don't want to create shims when we are just @@ -593,10 +583,10 @@ export default class Module { if (!isExportAllSearch) { if (this.options.shimMissingExports) { this.shimMissingExport(name); - return this.exportShimVariable; + return [this.exportShimVariable]; } } - return null; + return [null]; } hasEffects(): boolean { @@ -619,7 +609,7 @@ export default class Module { for (const exportName of this.getExports()) { if (includeNamespaceMembers || exportName !== this.info.syntheticNamedExports) { - const variable = this.getVariableForExportName(exportName)!; + const variable = this.getVariableForExportName(exportName)[0]!; variable.deoptimizePath(UNKNOWN_PATH); if (!variable.included) { this.includeVariable(variable); @@ -628,7 +618,7 @@ export default class Module { } for (const name of this.getReexports()) { - const variable = this.getVariableForExportName(name); + const [variable] = this.getVariableForExportName(name); if (variable) { variable.deoptimizePath(UNKNOWN_PATH); if (!variable.included) { @@ -657,20 +647,7 @@ export default class Module { linkImports(): void { this.addModulesToImportDescriptions(this.importDescriptions); this.addModulesToImportDescriptions(this.reexportDescriptions); - for (const name in this.exports) { - if (name !== 'default' && name !== this.info.syntheticNamedExports) { - this.exportsAll[name] = `${this.id}*${name}`; - } - } - for (const name in this.reexportDescriptions) { - if (name !== 'default' && name !== this.info.syntheticNamedExports) { - const module = this.reexportDescriptions[name].module; - this.exportsAll[name] = - module instanceof ExternalModule ? `${module.id}*${name}` : module.exportsAll[name]; - } - } const externalExportAllModules: ExternalModule[] = []; - const reexportsAll = Object.create(null); for (const source of this.exportAllSources) { const module = this.graph.modulesById.get(this.resolvedIds[source].id)!; if (module instanceof ExternalModule) { @@ -678,20 +655,6 @@ export default class Module { continue; } this.exportAllModules.push(module); - for (const name in module.exportsAll) { - if (!(name in this.exportsAll)) { - if (name in reexportsAll) { - if (reexportsAll[name] !== module.exportsAll[name]) { - this.options.onwarn(errNamespaceConflict(name, this.id, reexportsAll, module)); - } - } else { - reexportsAll[name] = module.exportsAll[name]; - } - } - } - } - for (const name in reexportsAll) { - this.exportsAll[name] = reexportsAll[name]; } this.exportAllModules.push(...externalExportAllModules); } @@ -772,7 +735,7 @@ export default class Module { moduleContext: this.context, options: this.options, requestTreeshakingPass: () => (this.graph.needsTreeshakingPass = true), - traceExport: this.getVariableForExportName.bind(this), + traceExport: (name: string) => this.getVariableForExportName(name)[0], traceVariable: this.traceVariable.bind(this), usesTopLevelAwait: false, warn: this.warn.bind(this) @@ -819,7 +782,7 @@ export default class Module { return otherModule.namespace; } - const declaration = otherModule.getVariableForExportName(importDeclaration.name, { + const [declaration] = otherModule.getVariableForExportName(importDeclaration.name, { importerForSideEffects: importerForSideEffects || this }); @@ -1052,52 +1015,50 @@ export default class Module { private getVariableFromNamespaceReexports( name: string, importerForSideEffects?: Module, - searchedNamesAndModules?: Map>, - skipExternalNamespaceReexports = false - ): Variable | null { + searchedNamesAndModules?: Map> + ): [variable: Variable | null, indirectExternal?: boolean] { let foundSyntheticDeclaration: SyntheticNamedExportVariable | null = null; - const skipExternalNamespaceValues = [{ searchedNamesAndModules, skipExternalNamespaces: true }]; - if (!skipExternalNamespaceReexports) { - const clonedSearchedNamesAndModules = new Map>(); - for (const [name, modules] of searchedNamesAndModules || []) { - clonedSearchedNamesAndModules.set(name, new Set(modules)); - } - skipExternalNamespaceValues.push({ - searchedNamesAndModules: clonedSearchedNamesAndModules, - skipExternalNamespaces: false - }); - } - for (const { skipExternalNamespaces, searchedNamesAndModules } of skipExternalNamespaceValues) { - const foundDeclarations = new Set(); - for (const module of this.exportAllModules) { - if (module instanceof Module || !skipExternalNamespaces) { - const declaration = getVariableForExportNameRecursive( - module, - name, - importerForSideEffects, - true, - searchedNamesAndModules, - skipExternalNamespaces - ); - - if (declaration) { - if (!(declaration instanceof SyntheticNamedExportVariable)) { - foundDeclarations.add(declaration); - } else if (!foundSyntheticDeclaration) { - foundSyntheticDeclaration = declaration; - } - } + const foundInternalDeclarations = new Map(); + const foundExternalDeclarations = new Set(); + for (const module of this.exportAllModules) { + const [variable, indirectExternal] = getVariableForExportNameRecursive( + module, + name, + importerForSideEffects, + true, + searchedNamesAndModules + ); + + if (module instanceof ExternalModule || indirectExternal) { + foundExternalDeclarations.add(variable as ExternalVariable); + } else if (variable instanceof SyntheticNamedExportVariable) { + if (!foundSyntheticDeclaration) { + foundSyntheticDeclaration = variable; } + } else if (variable) { + foundInternalDeclarations.set(variable, module); } - if (foundDeclarations.size === 1) { - return [...foundDeclarations][0]; + } + if (foundInternalDeclarations.size > 0) { + const foundDeclarationList = [...foundInternalDeclarations]; + const usedDeclaration = foundDeclarationList[0][0]; + if (foundDeclarationList.length === 1) { + return [usedDeclaration]; } - if (foundDeclarations.size > 1) { - if (skipExternalNamespaces) { - return null; - } - const foundDeclarationList = [...(foundDeclarations as Set)]; - const usedDeclaration = foundDeclarationList[0]; + this.options.onwarn( + errNamespaceConflict( + name, + this.id, + foundDeclarationList.map(([, module]) => module.id) + ) + ); + // TODO we are pretending it was not found while it should behave like "undefined" + return [null]; + } + if (foundExternalDeclarations.size > 0) { + const foundDeclarationList = [...foundExternalDeclarations]; + const usedDeclaration = foundDeclarationList[0]; + if (foundDeclarationList.length > 1) { this.options.onwarn( errAmbiguousExternalNamespaces( name, @@ -1106,13 +1067,13 @@ export default class Module { foundDeclarationList.map(declaration => declaration.module.id) ) ); - return usedDeclaration; } + return [usedDeclaration, true]; } if (foundSyntheticDeclaration) { - return foundSyntheticDeclaration; + return [foundSyntheticDeclaration]; } - return null; + return [null]; } private includeAndGetAdditionalMergedNamespaces(): Variable[] { @@ -1120,7 +1081,7 @@ export default class Module { const syntheticNamespaces = new Set(); for (const module of [this, ...this.exportAllModules]) { if (module instanceof ExternalModule) { - const externalVariable = module.getVariableForExportName('*'); + const [externalVariable] = module.getVariableForExportName('*'); externalVariable.include(); this.imports.add(externalVariable); externalNamespaces.add(externalVariable); diff --git a/src/utils/error.ts b/src/utils/error.ts index 198bbf37785..4fa302372c3 100644 --- a/src/utils/error.ts +++ b/src/utils/error.ts @@ -341,40 +341,37 @@ export function errMixedExport(facadeModuleId: string, name?: string): RollupLog export function errNamespaceConflict( name: string, reexportingModuleId: string, - reexportsAll: Record, - additionalExportAllModule: Module + sources: string[] ): RollupWarning { - const [firstId] = reexportsAll[name].split('*'); - const [secondId] = additionalExportAllModule.exportsAll[name].split('*'); return { code: Errors.NAMESPACE_CONFLICT, message: `Conflicting namespaces: "${relativeId( reexportingModuleId - )}" re-exports "${name}" from both "${relativeId(firstId)}" and "${relativeId( - secondId - )}" (will be ignored)`, + )}" re-exports "${name}" from one of the modules ${printQuotedStringList( + sources.map(moduleId => relativeId(moduleId)) + )} (will be ignored)`, name, reexporter: reexportingModuleId, - sources: [firstId, secondId] + sources }; } export function errAmbiguousExternalNamespaces( name: string, reexportingModule: string, - usedExternalModule: string, - externalModules: string[] + usedModule: string, + sources: string[] ): RollupWarning { return { code: Errors.AMBIGUOUS_EXTERNAL_NAMESPACES, message: `Ambiguous external namespace resolution: "${relativeId( reexportingModule )}" re-exports "${name}" from one of the external modules ${printQuotedStringList( - externalModules.map(module => relativeId(module)) - )}, guessing "${relativeId(usedExternalModule)}".`, + sources.map(module => relativeId(module)) + )}, guessing "${relativeId(usedModule)}".`, name, reexporter: reexportingModule, - sources: externalModules + sources }; } diff --git a/test/function/samples/conflicting-reexports/namespace-import/_config.js b/test/function/samples/conflicting-reexports/namespace-import/_config.js index a83afe3ee0c..def8efa6927 100644 --- a/test/function/samples/conflicting-reexports/namespace-import/_config.js +++ b/test/function/samples/conflicting-reexports/namespace-import/_config.js @@ -6,7 +6,7 @@ module.exports = { { code: 'NAMESPACE_CONFLICT', message: - 'Conflicting namespaces: "reexport.js" re-exports "foo" from both "first.js" and "second.js" (will be ignored)', + 'Conflicting namespaces: "reexport.js" re-exports "foo" from one of the modules "first.js" and "second.js" (will be ignored)', name: 'foo', reexporter: path.join(__dirname, 'reexport.js'), sources: [path.join(__dirname, 'first.js'), path.join(__dirname, 'second.js')] diff --git a/test/function/samples/shims-missing-exports/dep2.js b/test/function/samples/shims-missing-exports/dep2.js deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/test/function/samples/shims-missing-exports/main.js b/test/function/samples/shims-missing-exports/main.js index cc652c34e2e..7db98642c1c 100644 --- a/test/function/samples/shims-missing-exports/main.js +++ b/test/function/samples/shims-missing-exports/main.js @@ -1,3 +1,3 @@ import { missing } from './dep1.js'; -export {missing} +export { missing }; diff --git a/test/function/samples/warn-on-namespace-conflict/_config.js b/test/function/samples/warn-on-namespace-conflict/_config.js index bf046da2768..0f6139a3501 100644 --- a/test/function/samples/warn-on-namespace-conflict/_config.js +++ b/test/function/samples/warn-on-namespace-conflict/_config.js @@ -7,8 +7,8 @@ module.exports = { code: 'NAMESPACE_CONFLICT', name: 'foo', reexporter: path.join(__dirname, 'main.js'), - sources: [path.join(__dirname, 'foo.js'), path.join(__dirname, 'deep.js')], - message: `Conflicting namespaces: "main.js" re-exports "foo" from both "foo.js" and "deep.js" (will be ignored)` + sources: [path.join(__dirname, 'foo.js'), path.join(__dirname, 'bar.js')], + message: `Conflicting namespaces: "main.js" re-exports "foo" from one of the modules "foo.js" and "bar.js" (will be ignored)` } ] };