Skip to content

feat(minifier): support property_write_side_effects option to drop unused property assignments#20730

Closed
Dunqing wants to merge 2 commits intomainfrom
feat/minifier-property-write-side-effects
Closed

feat(minifier): support property_write_side_effects option to drop unused property assignments#20730
Dunqing wants to merge 2 commits intomainfrom
feat/minifier-property-write-side-effects

Conversation

@Dunqing
Copy link
Copy Markdown
Member

@Dunqing Dunqing commented Mar 25, 2026

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 function/class declarations with property assignments when unused.

// Input
function A() {}
A.from = () => {}
A.to = () => {}

// Output (with property_write_side_effects: false)
// (empty - all eliminated)

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 both may_have_side_effects() AND that the root object is a function/class declaration (guaranteed fresh, no aliasing)
  3. The fixed-point loop handles cascading: first pass drops the property assignments, second pass sees the declarations are truly unused

Safety: alias protection

Property assignments to variable declarations are conservatively kept because they may be aliases:

const a = {}
const b = a
b.add = 1      // kept — b aliases exported a
export { a }

Only function and class declarations are eligible for elimination, since they always create fresh values and cannot be aliases.

Assumption

When false, assumes Object.prototype / Function.prototype do not have setters for the assigned properties. This matches Rollup's default behavior (which drops these unconditionally for local bindings) and Rolldown's propertyWriteSideEffects option.

Cross-tool comparison

Tool Drops this pattern? Notes
Rollup Yes (always for locals) No config needed
Terser Partially Bugs with multiple assignments (#293, #776)
esbuild No Conservative, no option
SWC Yes (fragile) Depends on pass ordering
Oxc (this PR) Yes (opt-in, function/class only) Gated behind property_write_side_effects: false

Closes #14207

Test plan

  • Added 9 test cases covering function/class declarations, multiple assignments, read variables, side-effectful RHS, globals, alias safety, variable declarations, and default behavior
  • All minifier tests pass
  • Minsize snapshots unchanged (default true preserves prior behavior)
  • Clippy clean
  • NAPI bindings + TypeScript declarations updated

🤖 Generated with Claude Code

@github-actions github-actions Bot added the A-minifier Area - Minifier label Mar 25, 2026
Copy link
Copy Markdown
Member Author

Dunqing commented Mar 25, 2026


How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent changes, fast-track this PR to the front of 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.

@github-actions github-actions Bot added the C-enhancement Category - New feature or request label Mar 25, 2026
@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Mar 25, 2026

Merging this PR will not alter performance

✅ 48 untouched benchmarks
⏩ 8 skipped benchmarks1


Comparing feat/minifier-property-write-side-effects (3f08e6b) with main (9a5ff73)

Open in CodSpeed

Footnotes

  1. 8 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

@Dunqing Dunqing marked this pull request as ready for review March 26, 2026 03:31
@Dunqing Dunqing requested review from Boshen and sapphi-red March 26, 2026 03:31
Copy link
Copy Markdown
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Dunqing
Copy link
Copy Markdown
Member Author

Dunqing commented Mar 26, 2026

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 property_write_side_effects: false AND property_read_side_effects: None. But I think it is fine since these two are opt-in assumptions.

@sapphi-red
Copy link
Copy Markdown
Member

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.

@Dunqing Dunqing force-pushed the feat/minifier-property-write-side-effects branch 2 times, most recently from fb8afbe to 3b1c4d2 Compare March 26, 2026 05:31
@Dunqing Dunqing marked this pull request as draft March 26, 2026 05:45
@Dunqing
Copy link
Copy Markdown
Member Author

Dunqing commented Mar 26, 2026

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.

Yes, let me expand the implementation a bit to cover this test. It looks more complicated than I thought.

@Dunqing Dunqing force-pushed the feat/minifier-property-write-side-effects branch 3 times, most recently from 5ee0429 to 20fa9da Compare March 26, 2026 07:15
@github-actions github-actions Bot added the A-semantic Area - Semantic label Mar 26, 2026
@Dunqing Dunqing force-pushed the feat/minifier-property-write-side-effects branch from 5b87f7f to abff5d0 Compare March 26, 2026 07:32
…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>
@Dunqing Dunqing force-pushed the feat/minifier-property-write-side-effects branch from 46b1f46 to 1f2e21e Compare March 26, 2026 07:49
@Dunqing Dunqing force-pushed the feat/minifier-property-write-side-effects branch from 3f08e6b to 8d10ae8 Compare March 26, 2026 10:08
@github-actions github-actions Bot added the A-transformer Area - Transformer / Transpiler label Mar 26, 2026
graphite-app Bot pushed a commit that referenced this pull request Apr 9, 2026
…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)
@Dunqing
Copy link
Copy Markdown
Member Author

Dunqing commented Apr 10, 2026

Merged #20773 instead

@Dunqing Dunqing closed this Apr 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-minifier Area - Minifier A-semantic Area - Semantic A-transformer Area - Transformer / Transpiler C-enhancement Category - New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

minifier: drop constructed functions/objects

2 participants