Detect variable reassignments in modules without side effects (#5658)

src/Graph.ts

@@ -172,6 +172,7 @@ export default class Graph {
 				this.needsTreeshakingPass = false;
 				for (const module of this.modules) {
 					if (module.isExecuted) {
+						module.hasTreeShakingPassStarted = true;
 						if (module.info.moduleSideEffects === 'no-treeshake') {
 							module.includeAllInBundle();
 						} else {

src/Module.ts

@@ -2,10 +2,8 @@ import { extractAssignedNames } from '@rollup/pluginutils';
 import { locate } from 'locate-character';
 import MagicString from 'magic-string';
 import { parseAsync } from '../native';
-import ExternalModule from './ExternalModule';
-import type Graph from './Graph';
-import { createInclusionContext } from './ast/ExecutionContext';
 import { convertProgram } from './ast/bufferParsers';
+import { createInclusionContext } from './ast/ExecutionContext';
 import { nodeConstructors } from './ast/nodes';
 import ExportAllDeclaration from './ast/nodes/ExportAllDeclaration';
 import ExportDefaultDeclaration from './ast/nodes/ExportDefaultDeclaration';
@@ -19,8 +17,8 @@ import Literal from './ast/nodes/Literal';
 import type MetaProperty from './ast/nodes/MetaProperty';
 import * as NodeType from './ast/nodes/NodeType';
 import type Program from './ast/nodes/Program';
-import VariableDeclaration from './ast/nodes/VariableDeclaration';
 import type { NodeBase } from './ast/nodes/shared/Node';
+import VariableDeclaration from './ast/nodes/VariableDeclaration';
 import ModuleScope from './ast/scopes/ModuleScope';
 import { type PathTracker, UNKNOWN_PATH } from './ast/utils/PathTracker';
 import ExportDefaultVariable from './ast/variables/ExportDefaultVariable';
@@ -29,6 +27,8 @@ import ExternalVariable from './ast/variables/ExternalVariable';
 import NamespaceVariable from './ast/variables/NamespaceVariable';
 import SyntheticNamedExportVariable from './ast/variables/SyntheticNamedExportVariable';
 import type Variable from './ast/variables/Variable';
+import ExternalModule from './ExternalModule';
+import type Graph from './Graph';
 import type {
 	AstNode,
 	CustomPluginOptions,
@@ -220,6 +220,7 @@ export default class Module {
 	readonly dynamicImports: DynamicImport[] = [];
 	excludeFromSourcemap: boolean;
 	execIndex = Infinity;
+	hasTreeShakingPassStarted = false;
 	readonly implicitlyLoadedAfter = new Set<Module>();
 	readonly implicitlyLoadedBefore = new Set<Module>();
 	readonly importDescriptions = new Map<string, ImportDescription>();

src/ast/nodes/Identifier.ts

@@ -20,7 +20,6 @@ import GlobalVariable from '../variables/GlobalVariable';
 import LocalVariable from '../variables/LocalVariable';
 import type Variable from '../variables/Variable';
 import * as NodeType from './NodeType';
-import type SpreadElement from './SpreadElement';
 import { Flag, isFlagSet, setFlag } from './shared/BitFlags';
 import {
 	type ExpressionEntity,
@@ -30,6 +29,7 @@ import {
 import { NodeBase } from './shared/Node';
 import type { PatternNode } from './shared/Pattern';
 import type { VariableKind } from './shared/VariableKinds';
+import type SpreadElement from './SpreadElement';
 
 export type IdentifierWithVariable = Identifier & { variable: Variable };
 
@@ -220,9 +220,10 @@ export default class Identifier extends NodeBase implements PatternNode {
 				this.variable instanceof LocalVariable &&
 				this.variable.kind &&
 				tdzVariableKinds.has(this.variable.kind) &&
-				// we ignore possible TDZs due to circular module dependencies as
-				// otherwise we get many false positives
-				this.variable.module === this.scope.context.module
+				// We ignore modules that did not receive a treeshaking pass yet as that
+				// causes many false positives due to circular dependencies or disabled
+				// moduleSideEffects.
+				this.variable.module.hasTreeShakingPassStarted
 			)
 		) {
 			return (this.isTDZAccess = false);
@@ -241,9 +242,7 @@ export default class Identifier extends NodeBase implements PatternNode {
 			return (this.isTDZAccess = true);
 		}
 
-		// We ignore the case where the module is not yet executed because
-		// moduleSideEffects are false.
-		if (!this.variable.initReached && this.scope.context.module.isExecuted) {
+		if (!this.variable.initReached) {
 			// Either a const/let TDZ violation or
 			// var use before declaration was encountered.
 			return (this.isTDZAccess = true);
@@ -294,6 +293,10 @@ export default class Identifier extends NodeBase implements PatternNode {
 	protected applyDeoptimizations(): void {
 		this.deoptimized = true;
 		if (this.variable instanceof LocalVariable) {
+			// When accessing a variable from a module without side effects, this
+			// means we use an export of that module and therefore need to potentially
+			// include it in the bundle.
+			this.variable.module.isExecuted = true;
 			this.variable.consolidateInitializers();
 			this.scope.context.requestTreeshakingPass();
 		}

test/function/samples/tree-shake-module-side-effects-update/_config.js

@@ -0,0 +1,8 @@
+module.exports = defineTest({
+	description: 'detects variable updates in modules without side effects (#5408)',
+	options: {
+		treeshake: {
+			moduleSideEffects: false
+		}
+	}
+});

test/function/samples/tree-shake-module-side-effects-update/dep.js

@@ -0,0 +1,4 @@
+export let direct = false;
+direct = true;
+
+export {indirect} from './dep2.js';

test/function/samples/tree-shake-module-side-effects-update/dep2.js

@@ -0,0 +1,4 @@
+export let indirect = false;
+(() => {
+	indirect = true;
+})();

test/function/samples/tree-shake-module-side-effects-update/main.js

@@ -0,0 +1,3 @@
+import { direct, indirect } from './dep.js';
+assert.ok(direct ? true : false, 'direct');
+assert.ok(indirect ? true : false, 'indirect');