feat(minifier): support property_write_side_effects option to drop unused property assignments#20730
feat(minifier): support property_write_side_effects option to drop unused property assignments#20730
property_write_side_effects option to drop unused property assignments#20730Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. This stack of pull requests is managed by Graphite. Learn more about stacking. |
Merging this PR will not alter performance
Comparing Footnotes
|
sapphi-red
left a comment
There was a problem hiding this comment.
I wonder if this would break codes like:
const a = {}
const b = { a }
b.a.add = 1
export { a }I guess it would output:
const a = {}
export { a }This would change the value of a.add.
It will affect when |
|
Ah, would this break codes like below as well? const a = {}
const b = a
b.add = 1
export { a }I guess it would output: const a = {}
export { a }If so, I think it'd be really difficult to enable this option in practice. |
fb8afbe to
3b1c4d2
Compare
Yes, let me expand the implementation a bit to cover this test. It looks more complicated than I thought. |
5ee0429 to
20fa9da
Compare
5b87f7f to
abff5d0
Compare
…unused property assignments
Add `property_write_side_effects` option to `TreeShakeOptions` (default `true`).
When set to `false`, property assignments like `A.from = () => {}` are considered
side-effect-free, enabling the minifier to drop constructed functions/objects
with property assignments when unused.
This works by:
1. Wiring the existing `property_write_side_effects` from `oxc_ecmascript`'s
`MayHaveSideEffectsContext` through the minifier's compress options
2. Falling through to `may_have_side_effects()` for member expression assignments
in `remove_unused_assignment_expr` instead of returning `false`
3. The fixed-point loop handles cascading: first pass drops the property
assignments, second pass sees the declarations are truly unused
The assumption (when `false`) is that `Object.prototype` / `Function.prototype`
do not have setters. This matches Rollup's default behavior and Rolldown's
`propertyWriteSideEffects` option.
Closes #14207
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
46b1f46 to
1f2e21e
Compare
3f08e6b to
8d10ae8
Compare
…unused property assignments (#20773) ## Summary Add `property_write_side_effects` option to `TreeShakeOptions` (default `true`). When set to `false`, property assignments like `A.from = () => {}` are considered side-effect-free, enabling the minifier to drop declarations and fresh value bindings with property assignments when unused. ### Examples ```js // Function declaration + property assignments → eliminated function A() {} A.from = () => {} A.to = () => {} // Output: (empty) // Class declaration + property assignment → eliminated class A {} A.foo = 1 // Output: (empty) // Const variable with arrow function → eliminated const A = () => {} A.foo = 1 // Output: (empty) // Const variable with object literal → eliminated const B = {} B.foo = 1 // Output: (empty) // Const variable with function expression → eliminated const C = function() {} C.foo = 1 // Output: (empty) ``` ### How it works 1. Wires the existing `property_write_side_effects` from `oxc_ecmascript`'s `MayHaveSideEffectsContext` through the minifier's compress options 2. For member expression assignments, checks `may_have_side_effects()` then `is_member_assign_to_unused_binding()` 3. `is_member_assign_to_unused_binding` verifies: - Single-level member expression only (`A.foo`, not `a.b.c`) - All references to the symbol have the `MemberWriteTarget` flag (from parent PR) - Symbol creates a fresh value (`is_fresh_value`) and is not exported 4. Fixed-point loop handles cascading: first pass drops assignments, `exit_program` cleans up references, second pass drops declarations ### Fresh value detection A symbol is considered "fresh" (cannot alias another binding and property writes are side-effect-free) if it is: - A **function declaration** - A **class declaration** without static setters, static accessor properties, or static property definitions with values - A variable initialized with an **array/arrow function/function expression** literal - A variable initialized with an **object literal** without setter/getter properties or property values containing setters/getters - A variable initialized with a **class expression** (same rules as class declarations above) Non-fresh values (e.g. `const b = a`) are conservatively preserved to prevent breaking aliased references. #### Setter/getter detection (addresses review feedback) Following how esbuild, Terser, Rollup, and SWC handle setter cases — all tools preserve property writes when setters are visible in the AST: - **Static setter methods**: `class A { static set foo(v) { ... } }` → not fresh - **Static accessor properties**: `class A { static accessor foo = 0 }` → not fresh (auto-generates getter+setter) - **Static property definitions with values**: `class A { static b = 0 }` → not fresh (matches SWC's conservative approach) - **Static property values containing setters/getters**: `class A { static b = { set x(v) { ... } } }` → not fresh - **Object literal setters/getters**: `{ set foo(v) { ... } }` or `{ get foo() { ... } }` → not fresh - **Nested setters/getters in object property values**: `{ bar: { set x(v) { ... } } }` → not fresh - **Non-static setters are fine**: `class A { set foo(v) { ... } }` → fresh (instance setters don't affect class-level property writes) ### Limitations **Only single-level member expressions are supported** (`A.foo`, not `a.b.c`). Deep nested assignments like `a.b.c = 1` are always preserved because intermediate properties could alias external objects: ```js const exported = {}; const a = { b: exported }; a.b.c = 1; // writes to exported.c — unsafe to drop! export { exported }; ``` Supporting deeper nesting would require full object graph tracking (like Rollup's `ObjectEntity` model), which is a much larger effort for marginal gain. ### Safety guards Addresses concerns raised in [PR #20730 review](#20730 (review)) and [comment](#20730 (comment)): - **Aliases**: `const b = a; b.add = 1; export { a }` → kept (not fresh, could mutate exported `a` through alias) - **Chained access**: `const b = { a }; b.a.add = 1; export { a }` → kept (only single-level member expressions allowed) - **Exported symbols**: `export function A() {} A.foo = 1;` → kept (observable by importers) - **Other reads**: `function A() {} A.foo = 1; console.log(A)` → kept (symbol has non-member-write references) - **Side-effectful RHS**: `function A() {} A.foo = sideEffect()` → kept (`may_have_side_effects` returns true) - **Globals**: `globalObj.foo = 1` → kept (no local binding) - **Script mode top-level**: top-level bindings in script mode are kept (they're globals visible to other scripts) - **Setters**: `class A { static set foo(v) { ... } } A.foo = 1` → kept (setter triggers side effects) - **Static properties**: `class A { static b = 0 } A.b = 1` → kept (matches SWC's conservative approach) ### Comparison with other tools | Tool | Approach | Option needed? | |------|----------|---------------| | **Rollup** | Structural aliasing analysis — tracks object identity through `LocalVariable.isReassigned` + `ObjectEntity` setter detection. Drops `A.foo = 1` automatically when `A` is a local unreassigned object with no setters. | No (always active) | | **Rolldown** | Adds explicit [`treeshake.propertyWriteSideEffects`](https://rolldown.rs/reference/InputOptions.treeshake#propertywritesideeffects) option (default `'always'`). Not in upstream Rollup. | Yes | | **Terser** | Checks `is_constant_expression` on the base object after unwrapping property chain. If the base is constant within scope, drops the assignment. | No (always active, but less precise) | | **esbuild** | All property assignments always preserved. Rationale: `Object.defineProperty` could install prototype setters. Only escape hatch is `/* @__PURE__ */` IIFE wrapping. | No option available | | **SWC** | `may_have_side_effects` returns `true` for all assignments unconditionally. Classes with static property definitions are conservatively preserved. | No option available | | **Oxc (this PR)** | Fresh value tracking (`is_fresh_value`) + `MemberWriteTarget` reference flags + explicit option. Closest to Rollup's aliasing analysis but gated behind an option like Rolldown. | Yes (`property_write_side_effects: false`) | Closes #14207 ## Test plan - [x] 20 test cases covering all safety scenarios including setter/getter edge cases - [x] All minifier tests pass (476/476) - [x] No minsize regressions - [x] Clippy clean 🤖 Generated with [Claude Code](https://claude.com/claude-code)
|
Merged #20773 instead |

Summary
Add
property_write_side_effectsoption toTreeShakeOptions(defaulttrue). When set tofalse, property assignments likeA.from = () => {}are considered side-effect-free, enabling the minifier to drop function/class declarations with property assignments when unused.How it works
property_write_side_effectsfromoxc_ecmascript'sMayHaveSideEffectsContextthrough the minifier's compress optionsmay_have_side_effects()AND that the root object is a function/class declaration (guaranteed fresh, no aliasing)Safety: alias protection
Property assignments to variable declarations are conservatively kept because they may be aliases:
Only function and class declarations are eligible for elimination, since they always create fresh values and cannot be aliases.
Assumption
When
false, assumesObject.prototype/Function.prototypedo not have setters for the assigned properties. This matches Rollup's default behavior (which drops these unconditionally for local bindings) and Rolldown'spropertyWriteSideEffectsoption.Cross-tool comparison
property_write_side_effects: falseCloses #14207
Test plan
truepreserves prior behavior)🤖 Generated with Claude Code