Skip to content

fix(minifier): recognize object spread of object literals as side-effect-free#20299

Merged
graphite-app[bot] merged 1 commit intomainfrom
fix/object-spread-side-effects
Mar 13, 2026
Merged

fix(minifier): recognize object spread of object literals as side-effect-free#20299
graphite-app[bot] merged 1 commit intomainfrom
fix/object-spread-side-effects

Conversation

@Boshen
Copy link
Member

@Boshen Boshen commented Mar 12, 2026

Summary

  • Add ObjectExpression handling in ObjectPropertyKind::SpreadProperty's may_have_side_effects — spreading an object literal is side-effect-free when all its properties are
  • Fix remove_unused_object_expr to check side effects for all-spread objects instead of unconditionally bailing

Closes rolldown/rolldown#8582

@github-actions github-actions bot added A-minifier Area - Minifier C-bug Category - Bug labels Mar 12, 2026
@Boshen Boshen self-assigned this Mar 12, 2026
@codspeed-hq
Copy link

codspeed-hq bot commented Mar 12, 2026

Merging this PR will not alter performance

✅ 53 untouched benchmarks
⏩ 3 skipped benchmarks1


Comparing fix/object-spread-side-effects (c92399b) with main (1c07b3b)2

Open in CodSpeed

Footnotes

  1. 3 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.

  2. No successful run was found on main (910c941) during the generation of this report, so 1c07b3b was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@Boshen Boshen marked this pull request as ready for review March 13, 2026 04:41
Copilot AI review requested due to automatic review settings March 13, 2026 04:41
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Mar 13, 2026
Copy link
Member Author

Boshen commented Mar 13, 2026

Merge activity

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes the minifier to recognize that spreading object literals (e.g., ({...{}}), ({...{a: 1}})) is side-effect-free when all the inner properties are side-effect-free. Previously, object spreads were conservatively treated as always having side effects (since they could invoke getters on unknown values), which prevented removing unused expressions like ({...{}}).

Changes:

  • Add ObjectExpression handling in ObjectPropertyKind::SpreadProperty's may_have_side_effects to recursively check inner properties
  • Fix remove_unused_object_expr to check side effects for all-spread objects instead of unconditionally bailing out
  • Add tests for both the side-effects analysis and peephole optimization

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
crates/oxc_ecmascript/src/side_effects/expressions.rs Adds ObjectExpression case in spread property side-effect analysis, recursively checking inner properties
crates/oxc_minifier/src/peephole/remove_unused_expression.rs Changes the all-spread early return from return false to checking if any spread has side effects
crates/oxc_minifier/tests/ecmascript/may_have_side_effects.rs Updates existing tests and adds new test cases for spreading object literals
crates/oxc_minifier/tests/peephole/remove_unused_expression.rs Adds peephole optimization tests for spreading object literals

You can also share your feedback on Copilot code review. Take the survey.

Copy link

@chatgpt-codex-connector chatgpt-codex-connector bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c92399b05e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +653 to +656
Expression::ObjectExpression(obj) => obj
.properties
.iter()
.any(|property| property.may_have_side_effects(ctx)),

Choose a reason for hiding this comment

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

P1 Badge Treat getter-backed object spreads as side-effectful

When property_read_side_effects is All (the default), spreading an object literal with accessor properties must be considered effectful because object spread performs a property read (Get) for each enumerable key. This new branch only checks property.may_have_side_effects(ctx), which does not account for PropertyKind::Get, so expressions like ({...{ get a() { foo() } }}) can be misclassified as side-effect-free and then removed by remove_unused_object_expr, dropping the getter invocation.

Useful? React with 👍 / 👎.

…ect-free (#20299)

## Summary

- Add `ObjectExpression` handling in `ObjectPropertyKind::SpreadProperty`'s `may_have_side_effects` — spreading an object literal is side-effect-free when all its properties are
- Fix `remove_unused_object_expr` to check side effects for all-spread objects instead of unconditionally bailing

Closes rolldown/rolldown#8582
@graphite-app graphite-app bot force-pushed the fix/object-spread-side-effects branch from c92399b to 5c97b14 Compare March 13, 2026 04:56
@graphite-app graphite-app bot merged commit 5c97b14 into main Mar 13, 2026
21 checks passed
@graphite-app graphite-app bot deleted the fix/object-spread-side-effects branch March 13, 2026 05:07
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Mar 13, 2026
Boshen added a commit that referenced this pull request Mar 14, 2026
### 🚀 Features

- e7163b6 ecmascript: Add known-globals to side-effect-free property
reads (#20212) (Dunqing)
- 139ab68 ecmascript: Add `property_write_side_effects` to
`MayHaveSideEffectsContext` (#20217) (Dunqing)

### 🐛 Bug Fixes

- 78c264a parser: Fix conditional expressions with arrow-function
alternates in TS (#20356) (camc314)
- 5c97b14 minifier: Recognize object spread of object literals as
side-effect-free (#20299) (Boshen)
- 1ff5c1d transformer/typescript: Rewrite extensions in dynamic
`import()` expressions (#20121) (Sverre Johansen)
- 1c07b3b diagnostics: Handle `WouldBlock` in stdout writes to prevent
panic (#20295) (Boshen)
- ade14d4 ecmascript: Enhance side-effect detection for classes,
TypedArrays, computed members, and spread (#20213) (Dunqing)

### ⚡ Performance

- 5474d0a semantic: V8-style walk-up reference resolution (#20292)
(Boshen)

### 📚 Documentation

- e4aa5b5 parser/napi, linter/plugins: Add JSDoc comments to raw
transfer constants (#20286) (overlookmotel)

Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-minifier Area - Minifier C-bug Category - Bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: effect v4 is tree shaken worse than rollup

2 participants