Skip to content

Commit

Permalink
Prefer locally defined exports and reexports over external namespaces (…
Browse files Browse the repository at this point in the history 
…#4064)

* Prefer locally defined exports and reexports over external namespaces

* Refine namespace conflict handling to ignore and warn for known conflicts

* Do not include namespace for conflicting member access
  • Loading branch information
@lukastaegert
lukastaegert committed May 4, 2021
1 parent 7927f3d commit e249c78
Show file tree
Hide file tree
Showing 85 changed files with 1,098 additions and 267 deletions.
36 changes: 15 additions & 21 deletions cli/run/batchWarnings.ts
View file
@@ -1,6 +1,7 @@
import { bold, gray, yellow } from 'colorette';
import { RollupWarning } from '../../src/rollup/types';
import { getOrCreate } from '../../src/utils/getOrCreate';
import { printQuotedStringList } from '../../src/utils/printStringList';
import relativeId from '../../src/utils/relativeId';
import { stderr } from '../logging';

Expand Down Expand Up @@ -79,15 +80,10 @@ const immediateHandlers: {
MISSING_NODE_BUILTINS: warning => {
title(`Missing shims for Node.js built-ins`);

const detail =
warning.modules!.length === 1
? `'${warning.modules![0]}'`
: `${warning
.modules!.slice(0, -1)
.map((name: string) => `'${name}'`)
.join(', ')} and '${warning.modules!.slice(-1)}'`;
stderr(
`Creating a browser bundle that depends on ${detail}. You might need to include https://github.com/ionic-team/rollup-plugin-node-polyfills`
`Creating a browser bundle that depends on ${printQuotedStringList(
warning.modules!
)}. You might need to include https://github.com/ionic-team/rollup-plugin-node-polyfills`
);
}
};
Expand Down Expand Up @@ -162,11 +158,11 @@ const deferredHandlers: {
title(`Conflicting re-exports`);
for (const warning of warnings) {
stderr(
`${bold(relativeId(warning.reexporter!))} re-exports '${
`"${bold(relativeId(warning.reexporter!))}" re-exports "${
warning.name
}' from both ${relativeId(warning.sources![0])} and ${relativeId(
}" from both "${relativeId(warning.sources![0])}" and "${relativeId(
warning.sources![1]
)} (will be ignored)`
)}" (will be ignored)`
);
}
},
Expand Down Expand Up @@ -207,16 +203,14 @@ const deferredHandlers: {
title(`Broken sourcemap`);
info('https://rollupjs.org/guide/en/#warning-sourcemap-is-likely-to-be-incorrect');

const plugins = Array.from(new Set(warnings.map(w => w.plugin).filter(Boolean)));
const detail =
plugins.length > 1
? ` (such as ${plugins
.slice(0, -1)
.map(p => `'${p}'`)
.join(', ')} and '${plugins.slice(-1)}')`
: ` (such as '${plugins[0]}')`;

stderr(`Plugins that transform code${detail} should generate accompanying sourcemaps`);
const plugins = [
...new Set(warnings.map(warning => warning.plugin).filter(Boolean))
] as string[];
stderr(
`Plugins that transform code (such as ${printQuotedStringList(
plugins
)}) should generate accompanying sourcemaps`
);
},

THIS_IS_UNDEFINED(warnings) {
Expand Down
31 changes: 12 additions & 19 deletions src/ExternalModule.ts
View file
Expand Up @@ -8,6 +8,8 @@ import {
import { EMPTY_ARRAY } from './utils/blank';
import { makeLegal } from './utils/identifierHelpers';
import { normalize, relative } from './utils/path';
import { printQuotedStringList } from './utils/printStringList';
import relativeId from './utils/relativeId';

export default class ExternalModule {
chunk: void;
Expand Down Expand Up @@ -99,32 +101,23 @@ export default class ExternalModule {
const declaration = this.declarations[name];
return !declaration.included && !this.reexported && !declaration.referenced;
});

if (unused.length === 0) return;

const names =
unused.length === 1
? `'${unused[0]}' is`
: `${unused
.slice(0, -1)
.map(name => `'${name}'`)
.join(', ')} and '${unused.slice(-1)}' are`;

const importersSet = new Set<string>();
for (const name of unused) {
const {importers, dynamicImporters} = this.declarations[name].module;

if (Array.isArray(importers)) importers.forEach(v => importersSet.add(v));
if (Array.isArray(dynamicImporters)) dynamicImporters.forEach(v => importersSet.add(v));
const { importers } = this.declarations[name].module;
for (const importer of importers) {
importersSet.add(importer);
}
}

const importersArray = Array.from(importersSet);

const importerList = ' in' + importersArray.map(s => `\n\t${s};`);

const importersArray = [...importersSet];
this.options.onwarn({
code: 'UNUSED_EXTERNAL_IMPORT',
message: `${names} imported from external module '${this.id}' but never used${importerList}`,
message: `${printQuotedStringList(unused, ['is', 'are'])} imported from external module "${
this.id
}" but never used in ${printQuotedStringList(
importersArray.map(importer => relativeId(importer))
)}.`,
names: unused,
source: this.id,
sources: importersArray
Expand Down
155 changes: 110 additions & 45 deletions src/Module.ts
View file
Expand Up @@ -45,8 +45,10 @@ import {
RollupWarning,
TransformModuleJSON
} from './rollup/types';
import { EMPTY_OBJECT } from './utils/blank';
import {
augmentCodeLocation,
errAmbiguousExternalNamespaces,
errCircularReexport,
errMissingExport,
errNamespaceConflict,
Expand Down Expand Up @@ -129,8 +131,11 @@ function findSourceMappingURLComments(ast: acorn.Node, code: string): [number, n

let sourcemappingUrlMatch;
const interStatmentCode = code.slice(start, end);
while (sourcemappingUrlMatch = SOURCEMAPPING_URL_COMMENT_RE.exec(interStatmentCode)) {
ret.push([start + sourcemappingUrlMatch.index, start + SOURCEMAPPING_URL_COMMENT_RE.lastIndex]);
while ((sourcemappingUrlMatch = SOURCEMAPPING_URL_COMMENT_RE.exec(interStatmentCode))) {
ret.push([
start + sourcemappingUrlMatch.index,
start + SOURCEMAPPING_URL_COMMENT_RE.lastIndex
]);
}
};

Expand All @@ -149,7 +154,8 @@ function getVariableForExportNameRecursive(
name: string,
importerForSideEffects: Module | undefined,
isExportAllSearch: boolean,
searchedNamesAndModules = new Map<string, Set<Module | ExternalModule>>()
searchedNamesAndModules = new Map<string, Set<Module | ExternalModule>>(),
skipExternalNamespaceReexports: boolean | undefined
): Variable | null {
const searchedModules = searchedNamesAndModules.get(name);
if (searchedModules) {
Expand All @@ -160,12 +166,12 @@ function getVariableForExportNameRecursive(
} else {
searchedNamesAndModules.set(name, new Set([target]));
}
return target.getVariableForExportName(
name,
return target.getVariableForExportName(name, {
importerForSideEffects,
isExportAllSearch,
searchedNamesAndModules
);
searchedNamesAndModules,
skipExternalNamespaceReexports
});
}

function getAndExtendSideEffectModules(variable: Variable, module: Module): Set<Module> {
Expand Down Expand Up @@ -253,6 +259,7 @@ export default class Module {
private exportNamesByVariable: Map<Variable, string[]> | null = null;
private exportShimVariable: ExportShimVariable = new ExportShimVariable(this);
private magicString!: MagicString;
private namespaceReexportsByName: Record<string, Variable | null> = Object.create(null);
private relevantDependencies: Set<Module | ExternalModule> | null = null;
private syntheticExports = new Map<string, SyntheticNamedExportVariable>();
private syntheticNamespace: Variable | null | undefined = null;
Expand Down Expand Up @@ -364,7 +371,10 @@ export default class Module {
) {
dependencyVariables = new Set(dependencyVariables);
for (const exportName of [...this.getReexports(), ...this.getExports()]) {
dependencyVariables.add(this.getVariableForExportName(exportName)!);
const exportedVariable = this.getVariableForExportName(exportName);
if (exportedVariable) {
dependencyVariables.add(exportedVariable);
}
}
}
for (let variable of dependencyVariables) {
Expand Down Expand Up @@ -482,9 +492,17 @@ export default class Module {

getVariableForExportName(
name: string,
importerForSideEffects?: Module,
isExportAllSearch?: boolean,
searchedNamesAndModules?: Map<string, Set<Module | ExternalModule>>
{
importerForSideEffects,
isExportAllSearch,
searchedNamesAndModules,
skipExternalNamespaceReexports
}: {
importerForSideEffects?: Module;
isExportAllSearch?: boolean;
searchedNamesAndModules?: Map<string, Set<Module | ExternalModule>>;
skipExternalNamespaceReexports?: boolean;
} = EMPTY_OBJECT
): Variable | null {
if (name[0] === '*') {
if (name.length === 1) {
Expand All @@ -505,7 +523,8 @@ export default class Module {
reexportDeclaration.localName,
importerForSideEffects,
false,
searchedNamesAndModules
searchedNamesAndModules,
false
);

if (!variable) {
Expand Down Expand Up @@ -539,27 +558,17 @@ export default class Module {
}

if (name !== 'default') {
let foundSyntheticDeclaration: SyntheticNamedExportVariable | null = null;
for (const module of this.exportAllModules) {
const declaration = getVariableForExportNameRecursive(
module,
name,
importerForSideEffects,
true,
searchedNamesAndModules
);

if (declaration) {
if (!(declaration instanceof SyntheticNamedExportVariable)) {
return declaration;
}
if (!foundSyntheticDeclaration) {
foundSyntheticDeclaration = declaration;
}
}
}
if (foundSyntheticDeclaration) {
return foundSyntheticDeclaration;
const foundNamespaceReexport =
name in this.namespaceReexportsByName
? this.namespaceReexportsByName[name]
: (this.namespaceReexportsByName[name] = this.getVariableFromNamespaceReexports(
name,
importerForSideEffects,
searchedNamesAndModules,
skipExternalNamespaceReexports
));
if (foundNamespaceReexport) {
return foundNamespaceReexport;
}
}

Expand Down Expand Up @@ -618,13 +627,15 @@ export default class Module {
}

for (const name of this.getReexports()) {
const variable = this.getVariableForExportName(name)!;
variable.deoptimizePath(UNKNOWN_PATH);
if (!variable.included) {
this.includeVariable(variable);
}
if (variable instanceof ExternalVariable) {
variable.module.reexported = true;
const variable = this.getVariableForExportName(name);
if (variable) {
variable.deoptimizePath(UNKNOWN_PATH);
if (!variable.included) {
this.includeVariable(variable);
}
if (variable instanceof ExternalVariable) {
variable.module.reexported = true;
}
}
}

Expand Down Expand Up @@ -798,10 +809,9 @@ export default class Module {
return otherModule.namespace;
}

const declaration = otherModule.getVariableForExportName(
importDeclaration.name,
importerForSideEffects || this
);
const declaration = otherModule.getVariableForExportName(importDeclaration.name, {
importerForSideEffects: importerForSideEffects || this
});

if (!declaration) {
return this.error(
Expand Down Expand Up @@ -837,7 +847,6 @@ export default class Module {
}
}


updateOptions({
meta,
moduleSideEffects,
Expand Down Expand Up @@ -1032,6 +1041,62 @@ export default class Module {
addSideEffectDependencies(alwaysCheckedDependencies);
}

private getVariableFromNamespaceReexports(
name: string,
importerForSideEffects?: Module,
searchedNamesAndModules?: Map<string, Set<Module | ExternalModule>>,
skipExternalNamespaceReexports = false
): Variable | null {
let foundSyntheticDeclaration: SyntheticNamedExportVariable | null = null;
const skipExternalNamespaceValues = new Set([true, skipExternalNamespaceReexports]);
for (const skipExternalNamespaces of skipExternalNamespaceValues) {
const foundDeclarations = new Set<Variable>();
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;
}
}
}
}
if (foundDeclarations.size === 1) {
return [...foundDeclarations][0];
}
if (foundDeclarations.size > 1) {
if (skipExternalNamespaces) {
return null;
}
const foundDeclarationList = [...(foundDeclarations as Set<ExternalVariable>)];
const usedDeclaration = foundDeclarationList[0];
this.options.onwarn(
errAmbiguousExternalNamespaces(
name,
this.id,
usedDeclaration.module.id,
foundDeclarationList.map(declaration => declaration.module.id)
)
);
return usedDeclaration;
}
}
if (foundSyntheticDeclaration) {
return foundSyntheticDeclaration;
}
return null;
}

private includeAndGetAdditionalMergedNamespaces(): Variable[] {
const mergedNamespaces: Variable[] = [];
for (const module of this.exportAllModules) {
Expand Down
11 changes: 3 additions & 8 deletions src/ast/nodes/MemberExpression.ts
View file
Expand Up @@ -123,14 +123,9 @@ export default class MemberExpression extends NodeBase implements DeoptimizableE
if (path.length === 0) this.disallowNamespaceReassignment();
if (this.variable) {
this.variable.deoptimizePath(path);
} else {
const propertyKey = this.getPropertyKey();
if (propertyKey === UnknownKey) {
this.object.deoptimizePath(UNKNOWN_PATH);
} else {
this.wasPathDeoptimizedWhileOptimized = true;
this.object.deoptimizePath([propertyKey, ...path]);
}
} else if (!this.replacement) {
this.wasPathDeoptimizedWhileOptimized = true;
this.object.deoptimizePath([this.getPropertyKey(), ...path]);
}
}

Expand Down

0 comments on commit e249c78

Please sign in to comment.