implement --treeshake.propertyReadSideEffects=always (#3985)

docs/999-big-list-of-options.md

@@ -1353,7 +1353,7 @@ Default: `false`
 If this option is provided, bundling will not fail if bindings are imported from a file that does not define these bindings. Instead, new variables will be created for these bindings with the value `undefined`.
 
 #### treeshake
-Type: `boolean | { annotations?: boolean, moduleSideEffects?: ModuleSideEffectsOption, propertyReadSideEffects?: boolean, tryCatchDeoptimization?: boolean, unknownGlobalSideEffects?: boolean }`<br>
+Type: `boolean | { annotations?: boolean, moduleSideEffects?: ModuleSideEffectsOption, propertyReadSideEffects?: boolean | 'always', tryCatchDeoptimization?: boolean, unknownGlobalSideEffects?: boolean }`<br>
 CLI: `--treeshake`/`--no-treeshake`<br>
 Default: `true`
 
@@ -1467,12 +1467,16 @@ console.log(foo);
 Note that despite the name, this option does not "add" side effects to modules that do not have side effects. If it is important that e.g. an empty module is "included" in the bundle because you need this for dependency tracking, the plugin interface allows you to designate modules as being excluded from tree-shaking via the [`resolveId`](guide/en/#resolveid), [`load`](guide/en/#load) or [`transform`](guide/en/#transform) hook.
 
 **treeshake.propertyReadSideEffects**<br>
-Type: `boolean`<br>
+Type: `boolean | 'always'`<br>
 CLI: `--treeshake.propertyReadSideEffects`/`--no-treeshake.propertyReadSideEffects`<br>
 Default: `true`
 
+If `true`, retain unused property reads that Rollup can determine to have side-effects. This includes accessing properties of `null` or `undefined` or triggering explicit getters via property access. Note that this does not cover destructuring assignment or getters on objects passed as function parameters.
+
 If `false`, assume reading a property of an object never has side effects. Depending on your code, disabling this option can significantly reduce bundle size but can potentially break functionality if you rely on getters or errors from illegal property access.
 
+If `'always'`, assume all member property accesses, including destructuring, have side effects. This setting is recommended for code relying on getters with side effects. It typically results in larger bundle size, but smaller than disabling `treeshake` altogether.
+
 ```javascript
 // Will be removed if treeshake.propertyReadSideEffects === false
 const foo = {

src/ast/nodes/MemberExpression.ts

@@ -172,11 +172,12 @@ export default class MemberExpression extends NodeBase implements DeoptimizableE
 	}
 
 	hasEffects(context: HasEffectsContext): boolean {
+		const propertyReadSideEffects = (this.context.options.treeshake as NormalizedTreeshakingOptions).propertyReadSideEffects;
 		return (
+			propertyReadSideEffects === 'always' ||
 			this.property.hasEffects(context) ||
 			this.object.hasEffects(context) ||
-			((this.context.options.treeshake as NormalizedTreeshakingOptions).propertyReadSideEffects &&
-				this.object.hasEffectsWhenAccessedAtPath([this.propertyKey!], context))
+			(propertyReadSideEffects && this.object.hasEffectsWhenAccessedAtPath([this.propertyKey!], context))
 		);
 	}
 

src/ast/nodes/Property.ts

@@ -1,4 +1,5 @@
 import MagicString from 'magic-string';
+import { NormalizedTreeshakingOptions } from '../../rollup/types';
 import { RenderOptions } from '../../utils/renderHelpers';
 import { CallOptions, NO_ARGS } from '../CallOptions';
 import { DeoptimizableEntity } from '../DeoptimizableEntity';
@@ -84,7 +85,10 @@ export default class Property extends NodeBase implements DeoptimizableEntity, P
 	}
 
 	hasEffects(context: HasEffectsContext): boolean {
-		return this.key.hasEffects(context) || this.value.hasEffects(context);
+		const propertyReadSideEffects = (this.context.options.treeshake as NormalizedTreeshakingOptions).propertyReadSideEffects;
+		return this.parent.type === 'ObjectPattern' && propertyReadSideEffects === 'always' ||
+			this.key.hasEffects(context) ||
+			this.value.hasEffects(context);
 	}
 
 	hasEffectsWhenAccessedAtPath(path: ObjectPath, context: HasEffectsContext): boolean {

src/rollup/types.d.ts

@@ -477,7 +477,7 @@ export interface OutputPlugin extends Partial<OutputPluginHooks>, Partial<Output
 export interface TreeshakingOptions {
 	annotations?: boolean;
 	moduleSideEffects?: ModuleSideEffectsOption;
-	propertyReadSideEffects?: boolean;
+	propertyReadSideEffects?: boolean | 'always';
 	/** @deprecated Use `moduleSideEffects` instead */
 	pureExternalModules?: PureModulesOption;
 	tryCatchDeoptimization?: boolean;
@@ -487,7 +487,7 @@ export interface TreeshakingOptions {
 export interface NormalizedTreeshakingOptions {
 	annotations: boolean;
 	moduleSideEffects: HasModuleSideEffects;
-	propertyReadSideEffects: boolean;
+	propertyReadSideEffects: boolean | 'always';
 	tryCatchDeoptimization: boolean;
 	unknownGlobalSideEffects: boolean;
 }

src/utils/options/normalizeInputOptions.ts

@@ -237,7 +237,7 @@ const getTreeshake = (
 	| {
 			annotations: boolean;
 			moduleSideEffects: HasModuleSideEffects;
-			propertyReadSideEffects: boolean;
+			propertyReadSideEffects: boolean | 'always';
 			tryCatchDeoptimization: boolean;
 			unknownGlobalSideEffects: boolean;
 	  } => {
@@ -261,7 +261,9 @@ const getTreeshake = (
 				configTreeshake.pureExternalModules,
 				warn
 			),
-			propertyReadSideEffects: configTreeshake.propertyReadSideEffects !== false,
+			propertyReadSideEffects:
+				configTreeshake.propertyReadSideEffects === 'always' && 'always' ||
+				configTreeshake.propertyReadSideEffects !== false,
 			tryCatchDeoptimization: configTreeshake.tryCatchDeoptimization !== false,
 			unknownGlobalSideEffects: configTreeshake.unknownGlobalSideEffects !== false
 		};

test/cli/samples/propertyReadSideEffects-always/_config.js

@@ -0,0 +1,4 @@
+module.exports = {
+	description: 'verify property accesses are retained for getters with side effects',
+	command: `rollup main.js --validate --treeshake.propertyReadSideEffects=always`
+};

test/cli/samples/propertyReadSideEffects-always/_expected.js

@@ -0,0 +1,17 @@
+class C {
+	get x() {
+		console.log(`side effect ${++count}`);
+		return 42
+	}
+}
+let count = 0;
+const obj = new C;
+console.log(obj.x);
+
+// these statements should be retained
+if (obj.x) {
+	obj["x"];
+	const {x} = obj;
+}
+let x;
+({x} = obj);

test/cli/samples/propertyReadSideEffects-always/main.js

@@ -0,0 +1,21 @@
+class C {
+	get x() {
+		console.log(`side effect ${++count}`)
+		return 42
+	}
+}
+let count = 0
+const obj = new C
+console.log(obj.x)
+
+// these statements should be retained
+if (obj.x) {
+	obj["x"]
+	const {x} = obj
+}
+let x
+({x} = obj)
+
+// demonstrate that tree shaking still works
+const unused = x => x
+unused(123)

test/function/samples/propertyReadSideEffects-always/_config.js

@@ -0,0 +1,4 @@
+module.exports = {
+	description: 'verify property accesses are retained for getters with side effects',
+	options: { treeshake: { propertyReadSideEffects: 'always' } }
+};

test/function/samples/propertyReadSideEffects-always/main.js

@@ -0,0 +1,14 @@
+let effects = 0
+var obj = {}
+Object.defineProperty(obj, 'x', {
+	get() {
+		++effects
+	}
+})
+let value
+({x: value} = obj)
+obj.x
+obj["x"]
+const {x} = obj
+
+assert.strictEqual(effects, 4)