Skip to content

fix(ecmascript): enhance side-effect detection for classes, TypedArrays, computed members, and spread#20213

Merged
graphite-app[bot] merged 1 commit intomainfrom
fix/computed-member-spread-side-effects
Mar 12, 2026
Merged

fix(ecmascript): enhance side-effect detection for classes, TypedArrays, computed members, and spread#20213
graphite-app[bot] merged 1 commit intomainfrom
fix/computed-member-spread-side-effects

Conversation

@Dunqing
Copy link
Member

@Dunqing Dunqing commented Mar 11, 2026

Summary

Part of #19673

Several side-effect detection improvements:

  • Computed member expressions: When property_read_side_effects is None, non-literal keys (e.g. obj[identRef]) now check the key expression and object for their own side effects instead of always returning true
  • ObjectExpression spread: When property_read_side_effects is None, {...expr} delegates to the argument's own side effects
  • Class MethodDefinition: Now checks for parameter decorators (TypeScript @foo on params)
  • Class AccessorProperty: Now checks accessor property values for side effects
  • TypedArray constructors: Added Int8Array, Uint8Array, Uint8ClampedArray, Int16Array, Uint16Array, Int32Array, Uint32Array, Float32Array, Float64Array, BigInt64Array, BigUint64Array to pure constructors (DataView excluded as it requires an ArrayBuffer argument)

Test plan

  • Added tests for computed member non-literal keys with property_read_side_effects
  • Added tests for ObjectExpression spread with property_read_side_effects
  • Added tests for class method parameter decorators
  • Added tests for class accessor property values
  • Added tests for TypedArray constructors
  • All existing tests pass

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings March 11, 2026 03:01
@github-actions github-actions bot added A-minifier Area - Minifier C-bug Category - Bug labels Mar 11, 2026
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

Improves may_have_side_effects accuracy in oxc_ecmascript (used by the minifier/DCE) for additional ECMAScript/TypeScript constructs and expands test coverage to prevent regressions.

Changes:

  • Refines side-effect detection for computed member expressions and object spread when property_read_side_effects == None.
  • Extends class-element handling (method parameter decorators, accessor property initializers).
  • Treats TypedArray constructors as pure (and adds minifier tests for the new cases).

Reviewed changes

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

File Description
crates/oxc_minifier/tests/ecmascript/may_have_side_effects.rs Adds regression tests for computed members, object spread, TS parameter decorators, accessor initializers, and TypedArray constructors.
crates/oxc_ecmascript/src/side_effects/expressions.rs Updates side-effect logic for object spread/computed members/class elements; expands pure constructors; introduces AssignmentExpression/UpdateExpression handling.
crates/oxc_ecmascript/src/side_effects/context.rs Adds a property_write_side_effects() hook to the side-effects context.

@Dunqing Dunqing changed the base branch from main to graphite-base/20213 March 11, 2026 03:05
@Dunqing Dunqing force-pushed the fix/computed-member-spread-side-effects branch from 1fe424a to f0ccc25 Compare March 11, 2026 03:06
@Dunqing Dunqing changed the base branch from graphite-base/20213 to fix/known-globals-side-effects March 11, 2026 03:06
Copy link
Member Author

Dunqing commented Mar 11, 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.

@codspeed-hq
Copy link

codspeed-hq bot commented Mar 11, 2026

Merging this PR will not alter performance

✅ 53 untouched benchmarks
⏩ 3 skipped benchmarks1


Comparing fix/computed-member-spread-side-effects (988d663) with fix/known-globals-side-effects (4c397f0)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 fix/known-globals-side-effects (4d3b18e) during the generation of this report, so 63f714d was used instead as the comparison base. There might be some changes unrelated to this pull request in this report.

@Dunqing Dunqing force-pushed the fix/known-globals-side-effects branch from 8f68e8b to 411b61a Compare March 11, 2026 04:22
@Dunqing Dunqing force-pushed the fix/computed-member-spread-side-effects branch from f0ccc25 to 3599797 Compare March 11, 2026 04:22
@Dunqing Dunqing force-pushed the fix/known-globals-side-effects branch 3 times, most recently from 3089aca to cacff2f Compare March 11, 2026 04:42
@Dunqing Dunqing force-pushed the fix/computed-member-spread-side-effects branch from 3599797 to e39ffe3 Compare March 11, 2026 04:42
@Dunqing Dunqing requested review from Boshen and sapphi-red March 12, 2026 05:36
@Dunqing Dunqing force-pushed the fix/known-globals-side-effects branch from cacff2f to 4c397f0 Compare March 12, 2026 05:57
@Dunqing Dunqing force-pushed the fix/computed-member-spread-side-effects branch from 01cb8fd to 1a433a8 Compare March 12, 2026 05:57
@Dunqing Dunqing force-pushed the fix/computed-member-spread-side-effects branch from 1a433a8 to 988d663 Compare March 12, 2026 06:57
@Dunqing Dunqing force-pushed the fix/known-globals-side-effects branch from 4c397f0 to 4d3b18e Compare March 12, 2026 06:58
@Dunqing Dunqing changed the base branch from fix/known-globals-side-effects to graphite-base/20213 March 12, 2026 07:33
@Dunqing Dunqing force-pushed the fix/computed-member-spread-side-effects branch from 988d663 to 1b9c059 Compare March 12, 2026 07:34
@Dunqing Dunqing force-pushed the graphite-base/20213 branch from 4d3b18e to 4d6a096 Compare March 12, 2026 07:34
@Dunqing Dunqing changed the base branch from graphite-base/20213 to fix/known-globals-side-effects March 12, 2026 07:34
@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Mar 12, 2026
Copy link
Member

Boshen commented Mar 12, 2026

Merge activity

@Dunqing Dunqing force-pushed the fix/known-globals-side-effects branch from 4d6a096 to d12655d Compare March 12, 2026 09:05
@Dunqing Dunqing force-pushed the fix/computed-member-spread-side-effects branch from 1b9c059 to 5b8ac85 Compare March 12, 2026 09:05
…ys, computed members, and spread (#20213)

## Summary

Part of #19673

Several side-effect detection improvements:

- **Computed member expressions**: When `property_read_side_effects` is `None`, non-literal keys (e.g. `obj[identRef]`) now check the key expression and object for their own side effects instead of always returning `true`
- **ObjectExpression spread**: When `property_read_side_effects` is `None`, `{...expr}` delegates to the argument's own side effects
- **Class MethodDefinition**: Now checks for parameter decorators (TypeScript `@foo` on params)
- **Class AccessorProperty**: Now checks accessor property values for side effects
- **TypedArray constructors**: Added `Int8Array`, `Uint8Array`, `Uint8ClampedArray`, `Int16Array`, `Uint16Array`, `Int32Array`, `Uint32Array`, `Float32Array`, `Float64Array`, `BigInt64Array`, `BigUint64Array` to pure constructors (`DataView` excluded as it requires an `ArrayBuffer` argument)

## Test plan

- [x] Added tests for computed member non-literal keys with `property_read_side_effects`
- [x] Added tests for ObjectExpression spread with `property_read_side_effects`
- [x] Added tests for class method parameter decorators
- [x] Added tests for class accessor property values
- [x] Added tests for TypedArray constructors
- [x] All existing tests pass

🤖 Generated with [Claude Code](https://claude.com/claude-code)
@graphite-app graphite-app bot force-pushed the fix/known-globals-side-effects branch from c4c894c to e7163b6 Compare March 12, 2026 09:23
@graphite-app graphite-app bot force-pushed the fix/computed-member-spread-side-effects branch from 0a97c72 to ade14d4 Compare March 12, 2026 09:23
Base automatically changed from fix/known-globals-side-effects to main March 12, 2026 09:34
@graphite-app graphite-app bot merged commit ade14d4 into main Mar 12, 2026
22 checks passed
@graphite-app graphite-app bot deleted the fix/computed-member-spread-side-effects branch March 12, 2026 09:34
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Mar 12, 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.

4 participants