fix(ecmascript): treat collection constructor with variable arg as side-effectful by Dunqing · Pull Request #20383 · oxc-project/oxc

Dunqing · 2026-03-14T15:25:02Z

Summary

new Map(x), new Set(x), new WeakMap(x), new WeakSet(x) were incorrectly considered side-effect-free when the argument is a variable reference. These constructors iterate their argument via Symbol.iterator, which can execute arbitrary user code (custom iterators, proxies, etc.).

Before: is_pure_constructor unconditionally included Map/Set/WeakMap/WeakSet, so new Map(variable) was treated as pure.

After: Collection constructors are only pure with provably safe arguments:

No arguments: new Set()
null or undefined: new Set(null)
Array literals: new Set([1, 2, 3]), new Map([[k, v]])
Variable references or other expressions: NOT pure

This matches the behavior of esbuild and Rollup:

esbuild: only marks pure with no args, null, undefined, or array literals (test cases)
Rollup: uses PURE_WITH_ARRAY — only array literals are trusted (knownGlobals.ts)

Found via: Rolldown's tree-shake-iterable test failing after delegating NewExpression side-effect analysis to Oxc.

Test plan

Added tests for all cases: null, undefined, array literals, variable refs, Map with non-array entries
All existing oxc_minifier tests pass (473 passed)
Verified against Rolldown's tree-shake-iterable and manual-pure-functions test suites

🤖 Generated with Claude Code

codspeed-hq · 2026-03-14T15:31:28Z

Merging this PR will not alter performance

✅ 53 untouched benchmarks
⏩ 3 skipped benchmarks¹

_{Comparing fix/collection-constructor-side-effects (74c9ddd) with main (e62524d)}

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. ↩

Dunqing · 2026-03-15T06:51:07Z

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.

crates/oxc_ecmascript/src/side_effects/expressions.rs

…e-effect detection Address review feedback from #20383 and #20395. The "Coercion Methods Are Pure" assumption (ASSUMPTIONS.md) means toString/valueOf/Symbol.toPrimitive are assumed side-effect-free, so per-constructor ToPrimitive validation is unnecessary for String, Number, Date, ArrayBuffer, and Error types. Changes: - String, Number, Date, ArrayBuffer, Error constructors now unconditionally pure (coercion assumed pure) - String(), Number(), Symbol() call expressions simplified to pure - TypedArray uses value_type() check: object args call @@iterator (NOT covered by coercion assumption), BigInt args cause ToNumber to throw - Removed 3 unnecessary helpers (new_expr_with_to_{string,number,primitive}_check) - Removed invalid issue link (XXXX) - Fixed is_pure_callable_constructor doc comment - BigInt() retains special-case validation (throws on invalid values) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

graphite-app · 2026-03-16T08:49:36Z

Merge activity

Mar 16, 8:49 AM UTC: graphite-app added this pull request to the Graphite merge queue.
Mar 16, 9:01 AM UTC: Merged by the Graphite merge queue.

…de-effectful (#20383) ## Summary `new Map(x)`, `new Set(x)`, `new WeakMap(x)`, `new WeakSet(x)` were incorrectly considered side-effect-free when the argument is a variable reference. These constructors iterate their argument via `Symbol.iterator`, which can execute arbitrary user code (custom iterators, proxies, etc.). **Before:** `is_pure_constructor` unconditionally included `Map`/`Set`/`WeakMap`/`WeakSet`, so `new Map(variable)` was treated as pure. **After:** Collection constructors are only pure with provably safe arguments: - No arguments: `new Set()` - `null` or `undefined`: `new Set(null)` - Array literals: `new Set([1, 2, 3])`, `new Map([[k, v]])` - Variable references or other expressions: **NOT pure** This matches the behavior of **esbuild** and **Rollup**: - esbuild: only marks pure with no args, null, undefined, or array literals ([test cases](https://github.com/evanw/esbuild/blob/main/internal/js_parser/js_parser_test.go)) - Rollup: uses `PURE_WITH_ARRAY` — only array literals are trusted ([knownGlobals.ts](https://github.com/rollup/rollup/blob/master/src/ast/nodes/shared/knownGlobals.ts)) **Found via:** Rolldown's `tree-shake-iterable` test failing after delegating `NewExpression` side-effect analysis to Oxc. ## Test plan - [x] Added tests for all cases: null, undefined, array literals, variable refs, Map with non-array entries - [x] All existing `oxc_minifier` tests pass (473 passed) - [x] Verified against Rolldown's `tree-shake-iterable` and `manual-pure-functions` test suites 🤖 Generated with [Claude Code](https://claude.com/claude-code)

…e-effect detection Address review feedback from #20383 and #20395. The "Coercion Methods Are Pure" assumption (ASSUMPTIONS.md) means toString/valueOf/Symbol.toPrimitive are assumed side-effect-free, so per-constructor ToPrimitive validation is unnecessary for String, Number, Date, ArrayBuffer, and Error types. Changes: - String, Number, Date, ArrayBuffer, Error constructors now unconditionally pure (coercion assumed pure) - String(), Number(), Symbol() call expressions simplified to pure - TypedArray uses value_type() check: object args call @@iterator (NOT covered by coercion assumption), BigInt args cause ToNumber to throw - Removed 3 unnecessary helpers (new_expr_with_to_{string,number,primitive}_check) - Removed invalid issue link (XXXX) - Fixed is_pure_callable_constructor doc comment - BigInt() retains special-case validation (throws on invalid values) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

### 🐛 Bug Fixes - edb8677 ecmascript: Treat collection constructor with variable arg as side-effectful (#20383) (Dunqing) - 1f65c3f transformer: Emit design:paramtypes when class has static anonymous class expression (#20382) (bab) - fa70d5c transformer: Use implementation signature for design:paramtypes when constructor is overloaded (#20394) (bab) - ed5a7fb parser: Report syntax error for `new super()` (#20384) (Boshen) - e62524d minifier: Treat object spread of getters as having side effects (#20380) (Boshen) - f8fbd6e linter/plugins: Remove `hashbang` property from AST (#20365) (overlookmotel) ### ⚡ Performance - 30a2b0f minifier: Use atom_from_strs_array for template literal concat (#20386) (Boshen) - 690ce17 minifier: Use Vec::with_capacity for inline template expressions (#20389) (Boshen) - 9cd612f linter/plugins: Recycle comment objects (#20362) (overlookmotel) Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>

…e-effect detection Address review feedback from #20383 and #20395. The "Coercion Methods Are Pure" assumption (ASSUMPTIONS.md) means toString/valueOf/Symbol.toPrimitive are assumed side-effect-free, so per-constructor ToPrimitive validation is unnecessary for String, Number, Date, ArrayBuffer, and Error types. Changes: - String, Number, Date, ArrayBuffer, Error constructors now unconditionally pure (coercion assumed pure) - String(), Number(), Symbol() call expressions simplified to pure - TypedArray uses value_type() check: object args call @@iterator (NOT covered by coercion assumption), BigInt args cause ToNumber to throw - Removed 3 unnecessary helpers (new_expr_with_to_{string,number,primitive}_check) - Removed invalid issue link (XXXX) - Fixed is_pure_callable_constructor doc comment - BigInt() retains special-case validation (throws on invalid values) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

…e-effect detection (#20420) ## Summary Address review feedback from #20383 and #20395. Apply the "Coercion Methods Are Pure" assumption (`ASSUMPTIONS.md`) — `.toString()`/`.valueOf()`/`[Symbol.toPrimitive]()` are assumed side-effect-free — while still checking for spec-mandated throws that are NOT covered by that assumption. ### Changes **`new String/Number(arg)`** — `ToString(Symbol)` / `ToNumber(Symbol)` throws TypeError. Check via `new_expr_first_arg_may_be_symbol`. BigInt is fine for both (`ToString(BigInt)` works, Number converts BigInt→Number). **`new Date/ArrayBuffer(arg)`** — `ToNumber(Symbol)` AND `ToNumber(BigInt)` throw TypeError. Check via `new_expr_first_arg_may_be_symbol_or_bigint`. **Error constructors** — `new Error(Symbol())` / `Error(Symbol())` throw (`ToString(Symbol)`). Removed from `is_unconditionally_pure_constructor` and `is_pure_callable_constructor`. Added `is_error_constructor()` helper with Symbol check. **TypedArray constructors** — use `value_type()` instead of `to_primitive()`: object args call `@@iterator` (NOT a coercion method), BigInt args cause `ToNumber` to throw. Only known-safe primitive value types (Number/String/Boolean/Null/Undefined) accepted. **`String(x)` call** — unconditionally pure. `String()` as a function has special Symbol handling per spec (`String(Symbol())` → `"Symbol()"` without throwing). Added to `is_pure_callable_constructor`. **`Number(x)` / `Symbol(x)` calls** — keep special handling (`ToNumeric(Symbol)` / `ToString(Symbol)` throw). **`is_pure_collection_constructor`** — now verifies `undefined` is actually global (not shadowed). Removed invalid issue link (`XXXX`). **Also fixes:** typo `"BitUint64Array"` → `"BigUint64Array"` in `is_known_global_constructor`. ### Cross-tool comparison | Expression | ES Spec | esbuild | Terser | Rollup | SWC | Oxc (this PR) | |---|---|---|---|---|---|---| | `new String(x)` | impure (Symbol) | impure | impure | pure | impure | **impure** (Symbol check) | | `new Number(x)` | impure (Symbol) | impure | impure | pure | impure | **impure** (Symbol check) | | `new Date(x)` | impure (Symbol/BigInt) | impure | impure | pure | impure | **impure** (Symbol+BigInt) | | `new Date(0n)` | impure (BigInt) | impure | impure | pure | impure | **impure** (BigInt check) | | `new ArrayBuffer(x)` | impure (Symbol/BigInt) | impure | impure | pure | impure | **impure** (Symbol+BigInt) | | `new Error(x)` | impure (Symbol) | impure | impure | pure | impure | **impure** (Symbol check) | | `new Uint8Array(x)` | impure (@@iterator) | impure | impure | pure | impure | **impure** (value_type) | | `new Uint8Array({})` | impure (@@iterator) | impure | impure | pure | impure | **impure** (value_type) | | `String(x)` call | safe (Symbol handled) | impure | impure | pure | impure | **pure** (coercion assumption) | | `Error(x)` call | impure (Symbol) | impure | impure | pure | impure | **impure** (Symbol check) | Oxc sits between esbuild (most conservative) and Rollup (most aggressive): uses the coercion assumption to skip ToPrimitive checks on objects, but catches spec-mandated throws (`ToString(Symbol)`, `ToNumber(Symbol)`, `ToNumber(BigInt)`, `@@iterator` on objects). ## Test plan - [x] All 473 `oxc_minifier` tests pass (39 ignored) - [x] All 13 `oxc_ecmascript` tests pass - [x] Updated test expectations with new cases (`new Date(0n)`, `new ArrayBuffer(0n)`, `new Error(x)`, `Error(x)`) - [x] Cross-tool research verified against ES Spec, esbuild, Terser, Rollup, SWC

github-actions bot added A-minifier Area - Minifier C-bug Category - Bug labels Mar 14, 2026

Dunqing mentioned this pull request Mar 15, 2026

fix(ecmascript): add argument validation for NewExpression side-effect detection #20395

Merged

Dunqing marked this pull request as ready for review March 15, 2026 07:06

Dunqing mentioned this pull request Mar 15, 2026

refactor(ecmascript): consolidate side-effect known globals into separate module #20401

Merged

3 tasks

Dunqing requested review from Boshen and sapphi-red March 15, 2026 14:25

sapphi-red approved these changes Mar 15, 2026

View reviewed changes

crates/oxc_ecmascript/src/side_effects/expressions.rs Show resolved Hide resolved

crates/oxc_ecmascript/src/side_effects/expressions.rs Show resolved Hide resolved

graphite-app bot added the 0-merge Merge with Graphite Merge Queue label Mar 15, 2026

Dunqing mentioned this pull request Mar 16, 2026

fix(ecmascript): apply coercion-is-pure assumption to constructor side-effect detection #20420

Merged

4 tasks

graphite-app bot force-pushed the fix/collection-constructor-side-effects branch from 74c9ddd to edb8677 Compare March 16, 2026 08:49

graphite-app bot merged commit edb8677 into main Mar 16, 2026
21 checks passed

graphite-app bot deleted the fix/collection-constructor-side-effects branch March 16, 2026 09:01

Boshen mentioned this pull request Mar 16, 2026

release(crates): oxc v0.120.0 #20422

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix(ecmascript): treat collection constructor with variable arg as side-effectful#20383

fix(ecmascript): treat collection constructor with variable arg as side-effectful#20383
graphite-app[bot] merged 1 commit intomainfrom
fix/collection-constructor-side-effects

Dunqing commented Mar 14, 2026

Uh oh!

codspeed-hq bot commented Mar 14, 2026

Uh oh!

Dunqing commented Mar 15, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

graphite-app bot commented Mar 16, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

Dunqing commented Mar 14, 2026

Summary

Test plan

Uh oh!

codspeed-hq bot commented Mar 14, 2026

Merging this PR will not alter performance

Footnotes

Uh oh!

Dunqing commented Mar 15, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

How to use the Graphite Merge Queue

Uh oh!

Uh oh!

Uh oh!

graphite-app bot commented Mar 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merge activity

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Dunqing commented Mar 15, 2026 •

edited

Loading

graphite-app bot commented Mar 16, 2026 •

edited

Loading