Throw proper error when indirectly reexporting a recursive binding (#4472)

lukastaegert · web-flow · commit 2bbdc8e136d6 · 2022-04-30T07:43:53.000+02:00
* Throw proper error when indirectly reexporting a recursive binding

* Fix unrelated typo
diff --git a/docs/05-plugin-development.md b/docs/05-plugin-development.md
@@ -905,7 +905,7 @@ Use Rollup's internal acorn instance to parse code to an AST.
 
 #### `this.resolve`
 
-**Type:** `(source: string, importer?: string, options?: {skipSelf?: boolean, isEntry?: boolean, isEntry?: boolean, custom?: {[plugin: string]: any}}) => Promise<{id: string, external: boolean | "absolute", moduleSideEffects: boolean | 'no-treeshake', syntheticNamedExports: boolean | string, meta: {[plugin: string]: any}} | null>`
+**Type:** `(source: string, importer?: string, options?: {skipSelf?: boolean, isEntry?: boolean, custom?: {[plugin: string]: any}}) => Promise<{id: string, external: boolean | "absolute", moduleSideEffects: boolean | 'no-treeshake', syntheticNamedExports: boolean | string, meta: {[plugin: string]: any}} | null>`
 
 Resolve imports to module ids (i.e. file names) using the same plugins that Rollup uses, and determine if an import should be external. If `null` is returned, the import could not be resolved by Rollup or any plugin but was not explicitly marked as external by the user. If an absolute external id is returned that should remain absolute in the output either via the [`makeAbsoluteExternalsRelative`](guide/en/#makeabsoluteexternalsrelative) option or by explicit plugin choice in the [`resolveId`](guide/en/#resolveid) hook, `external` will be `"absolute"` instead of `true`.
 
diff --git a/src/Module.ts b/src/Module.ts
@@ -134,7 +134,7 @@ function getVariableForExportNameRecursive(
 	target: Module | ExternalModule,
 	name: string,
 	importerForSideEffects: Module | undefined,
-	isExportAllSearch: boolean,
+	isExportAllSearch: boolean | undefined,
 	searchedNamesAndModules = new Map<string, Set<Module | ExternalModule>>()
 ): [variable: Variable | null, indirectExternal?: boolean] {
 	const searchedModules = searchedNamesAndModules.get(name);
@@ -561,7 +561,10 @@ export default class Module {
 				return [this.exportShimVariable];
 			}
 			const name = exportDeclaration.localName;
-			const variable = this.traceVariable(name, importerForSideEffects)!;
+			const variable = this.traceVariable(name, {
+				importerForSideEffects,
+				searchedNamesAndModules
+			})!;
 			if (importerForSideEffects) {
 				getOrCreate(
 					importerForSideEffects.sideEffectDependenciesByVariable,
@@ -793,7 +796,18 @@ export default class Module {
 		};
 	}
 
-	traceVariable(name: string, importerForSideEffects?: Module): Variable | null {
+	traceVariable(
+		name: string,
+		{
+			importerForSideEffects,
+			isExportAllSearch,
+			searchedNamesAndModules
+		}: {
+			importerForSideEffects?: Module;
+			isExportAllSearch?: boolean;
+			searchedNamesAndModules?: Map<string, Set<Module | ExternalModule>>;
+		} = EMPTY_OBJECT
+	): Variable | null {
 		const localVariable = this.scope.variables.get(name);
 		if (localVariable) {
 			return localVariable;
@@ -807,9 +821,13 @@ export default class Module {
 				return otherModule.namespace;
 			}
 
-			const [declaration] = otherModule.getVariableForExportName(importDeclaration.name, {
-				importerForSideEffects: importerForSideEffects || this
-			});
+			const [declaration] = getVariableForExportNameRecursive(
+				otherModule,
+				importDeclaration.name,
+				importerForSideEffects || this,
+				isExportAllSearch,
+				searchedNamesAndModules
+			);
 
 			if (!declaration) {
 				return this.error(
@@ -1057,7 +1075,9 @@ export default class Module {
 				name,
 				importerForSideEffects,
 				true,
-				searchedNamesAndModules
+				// We are creating a copy to handle the case where the same binding is
+				// imported through different namespace reexports gracefully
+				copyNameToModulesMap(searchedNamesAndModules)
 			);
 
 			if (module instanceof ExternalModule || indirectExternal) {
@@ -1199,3 +1219,9 @@ function setAlternativeExporterIfCyclic(
 		}
 	}
 }
+
+const copyNameToModulesMap = (
+	searchedNamesAndModules?: Map<string, Set<Module | ExternalModule>>
+): Map<string, Set<Module | ExternalModule>> | undefined =>
+	searchedNamesAndModules &&
+	new Map(Array.from(searchedNamesAndModules, ([name, modules]) => [name, new Set(modules)]));
diff --git a/test/function/samples/circular-reexport/_config.js b/test/function/samples/circular-reexport/_config.js
@@ -0,0 +1,12 @@
+const path = require('path');
+const ID_MAIN = path.join(__dirname, 'main.js');
+
+module.exports = {
+	description: 'throws proper error for circular reexports',
+	error: {
+		code: 'CIRCULAR_REEXPORT',
+		id: ID_MAIN,
+		message: '"foo" cannot be exported from main.js as it is a reexport that references itself.',
+		watchFiles: [ID_MAIN]
+	}
+};
diff --git a/test/function/samples/circular-reexport/main.js b/test/function/samples/circular-reexport/main.js
@@ -0,0 +1,3 @@
+import { foo } from './main.js';
+
+export { foo };

Original file line number	Diff line number	Diff line change
`@@ -0,0 +1,3 @@`
	`1`	`+import { foo } from './main.js';`
	`2`	`+`
	`3`	`+export { foo };`