Make sure external re-exports are included for moduleSideEffects: fal…

…se (#2905)

* Always include external reexports even when moduleSideEffects are false

* Treat implicit external modules the same way as explicit ones for module
side-effects

src/Module.ts

@@ -456,7 +456,6 @@ export default class Module {
 
 		for (const exportName of this.getExports()) {
 			const variable = this.getVariableForExportName(exportName);
-
 			variable.deoptimizePath(UNKNOWN_PATH);
 			if (!variable.included) {
 				variable.include();
@@ -466,14 +465,14 @@ export default class Module {
 
 		for (const name of this.getReexports()) {
 			const variable = this.getVariableForExportName(name);
-
-			if (variable instanceof ExternalVariable) {
-				variable.reexported = variable.module.reexported = true;
-			} else if (!variable.included) {
+			variable.deoptimizePath(UNKNOWN_PATH);
+			if (!variable.included) {
 				variable.include();
-				variable.deoptimizePath(UNKNOWN_PATH);
 				this.graph.needsTreeshakingPass = true;
 			}
+			if (variable instanceof ExternalVariable) {
+				variable.module.reexported = true;
+			}
 		}
 	}
 

src/ModuleLoader.ts

@@ -398,7 +398,11 @@ export class ModuleLoader {
 				error(errUnresolvedImport(source, importer));
 			}
 			this.graph.warn(errUnresolvedImportTreatedAsExternal(source, importer));
-			return { id: source, external: true, moduleSideEffects: true };
+			return {
+				external: true,
+				id: source,
+				moduleSideEffects: this.hasModuleSideEffects(source, true)
+			};
 		}
 		return resolvedId;
 	}

src/ast/variables/Variable.ts

@@ -18,7 +18,6 @@ export default class Variable implements ExpressionEntity {
 	isReassigned = false;
 	module?: Module | ExternalModule;
 	name: string;
-	reexported = false;
 	renderBaseName: string | null = null;
 	renderName: string | null = null;
 	safeExportName: string | null = null;

test/function/samples/module-side-effects/external-false/_config.js

@@ -25,6 +25,9 @@ module.exports = {
 					if (id === 'internal') {
 						return id;
 					}
+					if (id === 'implicit-external') {
+						return null;
+					}
 					const moduleSideEffects = JSON.parse(id.split('-')[1]);
 					if (moduleSideEffects) {
 						return { id, moduleSideEffects, external: true };
@@ -38,5 +41,15 @@ module.exports = {
 				}
 			}
 		}
-	}
+	},
+	warnings: [
+		{
+			code: 'UNRESOLVED_IMPORT',
+			importer: 'main.js',
+			message:
+				"'implicit-external' is imported by main.js, but could not be resolved – treating it as an external dependency",
+			source: 'implicit-external',
+			url: 'https://rollupjs.org/guide/en#warning-treating-module-as-external-dependency'
+		}
+	]
 };

test/function/samples/module-side-effects/external-false/main.js

@@ -1,3 +1,4 @@
 import 'pluginsideeffects-false';
 import 'pluginsideeffects-true';
 import 'internal';
+import 'implicit-external';

test/function/samples/module-side-effects/reexport-from-external/_config.js

@@ -0,0 +1,24 @@
+const assert = require('assert');
+
+module.exports = {
+	description: 'handles reexporting external values when module side-effects are false',
+	context: {
+		require(id) {
+			return { value: id };
+		}
+	},
+	exports(exports) {
+		assert.deepStrictEqual(exports, {
+			external: 'external',
+			value: 'externalStar'
+		});
+	},
+	options: {
+		external() {
+			return true;
+		},
+		treeshake: {
+			moduleSideEffects: false
+		}
+	}
+};

test/function/samples/module-side-effects/reexport-from-external/main.js

@@ -0,0 +1,2 @@
+export { value as external } from 'external';
+export * from 'externalStar';