fix(ecmascript): add argument validation for NewExpression side-effect detection by Dunqing · Pull Request #20395 · oxc-project/oxc

Dunqing · 2026-03-15T06:50:54Z

Summary

is_pure_constructor treated many global constructors as unconditionally side-effect-free regardless of arguments. Cross-tool research (ES spec, esbuild, Terser, Rollup, SWC) confirmed this was the most permissive of all bundlers/minifiers and introduced real bugs.

Problem

These constructors perform observable operations on object arguments but were treated as pure:

Constructor	Side effect	ES Spec operation
`new String(obj)`	Calls `toString()`/`valueOf()`	`ToString` -> `ToPrimitive`
`new Number(obj)`	Calls `valueOf()`/`toString()`, throws on Symbol	`ToNumeric` -> `ToPrimitive`
`new Date(obj)`	Calls `[Symbol.toPrimitive]`/`valueOf()`/`toString()`	`ToPrimitive`
`new TypedArray(obj)`	Calls `[Symbol.iterator]` or `.length` getter	`GetMethod(@@iterator)`
`new ArrayBuffer(obj)`	Calls `valueOf()`/`toString()`	`ToIndex` -> `ToNumber`

Cross-tool comparison

Constructor	esbuild	Terser	Rollup	SWC	Oxc (before)
`new String(obj)`	impure	impure (`unsafe` only)	pure	pure	pure
`new Number(obj)`	impure	impure (`unsafe` only)	pure	pure	pure
`new Date(obj)`	impure	impure (`unsafe` only)	pure	impure	pure
`new TypedArray(obj)`	impure	impure	pure	impure	pure

Solution

Split is_pure_constructor into is_unconditionally_pure_constructor (Object, Boolean, Error types) and is_typed_array_constructor
Add per-constructor argument validation in NewExpression::may_have_side_effects using the same ToPrimitive-based pattern already used by CallExpression
Three shared helpers: new_expr_with_to_string_check, new_expr_with_to_number_check, new_expr_with_to_primitive_check

Also fixes

Typo "BitUint64Array" -> "BigUint64Array" in is_known_global_constructor
is_pure_collection_constructor now verifies undefined is actually global (not shadowed)

Test plan

Added 52 new test cases covering primitive args (safe), unknown/object args (side-effectful), and edge cases ({toString(){}}, {valueOf(){}})
All existing tests continue to pass
Verified no regressions in Rolldown (1639/1639 tests pass with this change patched in)

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

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 · 2026-03-15T06:58:41Z

Merging this PR will not alter performance

✅ 53 untouched benchmarks
⏩ 3 skipped benchmarks¹

_{Comparing fix/new-expression-argument-validation (ada6427) with fix/collection-constructor-side-effects (74c9ddd)}

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

crates/oxc_ecmascript/src/side_effects/expressions.rs

graphite-app · 2026-03-15T15:46:44Z

Merge activity

Mar 15, 3:46 PM UTC: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
Mar 17, 2:52 PM UTC: graphite-app added this pull request to the Graphite merge queue.
Mar 17, 3:05 PM UTC: Merged by the Graphite merge queue.

…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>

sapphi-red

Fixed in #20420

…t detection (#20395) ## Summary `is_pure_constructor` treated many global constructors as unconditionally side-effect-free regardless of arguments. Cross-tool research (ES spec, esbuild, Terser, Rollup, SWC) confirmed this was the most permissive of all bundlers/minifiers and introduced real bugs. ### Problem These constructors perform observable operations on object arguments but were treated as pure: | Constructor | Side effect | ES Spec operation | |---|---|---| | `new String(obj)` | Calls `toString()`/`valueOf()` | `ToString` -> `ToPrimitive` | | `new Number(obj)` | Calls `valueOf()`/`toString()`, throws on Symbol | `ToNumeric` -> `ToPrimitive` | | `new Date(obj)` | Calls `[Symbol.toPrimitive]`/`valueOf()`/`toString()` | `ToPrimitive` | | `new TypedArray(obj)` | Calls `[Symbol.iterator]` or `.length` getter | `GetMethod(@@iterator)` | | `new ArrayBuffer(obj)` | Calls `valueOf()`/`toString()` | `ToIndex` -> `ToNumber` | ### Cross-tool comparison | Constructor | esbuild | Terser | Rollup | SWC | Oxc (before) | |---|---|---|---|---|---| | `new String(obj)` | impure | impure (`unsafe` only) | pure | pure | **pure** | | `new Number(obj)` | impure | impure (`unsafe` only) | pure | pure | **pure** | | `new Date(obj)` | impure | impure (`unsafe` only) | pure | impure | **pure** | | `new TypedArray(obj)` | impure | impure | pure | impure | **pure** | ### Solution - Split `is_pure_constructor` into `is_unconditionally_pure_constructor` (Object, Boolean, Error types) and `is_typed_array_constructor` - Add per-constructor argument validation in `NewExpression::may_have_side_effects` using the same `ToPrimitive`-based pattern already used by `CallExpression` - Three shared helpers: `new_expr_with_to_string_check`, `new_expr_with_to_number_check`, `new_expr_with_to_primitive_check` ### Also fixes - Typo `"BitUint64Array"` -> `"BigUint64Array"` in `is_known_global_constructor` - `is_pure_collection_constructor` now verifies `undefined` is actually global (not shadowed) ## Test plan - Added 52 new test cases covering primitive args (safe), unknown/object args (side-effectful), and edge cases (`{toString(){}}`, `{valueOf(){}}`) - All existing tests continue to pass - Verified no regressions in Rolldown (1639/1639 tests pass with this change patched in)

…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

### 🚀 Features - 7215d9e transformer: Support lowering `accessor` with legacy decorators (#20348) (Dunqing) ### 🐛 Bug Fixes - 3bbd0cd transformer: Emit `Object` instead of `void 0` for untyped getter/setter `design:type` metadata (#20488) (Dunqing) - 4ae3f3f ecmascript: Apply coercion-is-pure assumption to constructor side-effect detection (#20420) (Dunqing) - 11f9695 transformer: Legacy decorator on computed property key leaves variable unassigned (#20430) (bab) - efeba28 ecmascript: Add argument validation for NewExpression side-effect detection (#20395) (Dunqing) Co-authored-by: Boshen <1430279+Boshen@users.noreply.github.com>

Dunqing mentioned this pull request Mar 15, 2026

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

Merged

3 tasks

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

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 force-pushed the fix/new-expression-argument-validation branch from 305e281 to ada6427 Compare March 15, 2026 14:03

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

sapphi-red requested 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

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 changed the base branch from fix/collection-constructor-side-effects to graphite-base/20395 March 16, 2026 08:49

graphite-app bot force-pushed the graphite-base/20395 branch from 74c9ddd to edb8677 Compare March 16, 2026 09:02

graphite-app bot force-pushed the fix/new-expression-argument-validation branch from ada6427 to 5ebee41 Compare March 16, 2026 09:02

graphite-app bot changed the base branch from graphite-base/20395 to main March 16, 2026 09:02

graphite-app bot force-pushed the fix/new-expression-argument-validation branch from 5ebee41 to b03fc1d Compare March 16, 2026 09:03

Dunqing force-pushed the fix/new-expression-argument-validation branch from b03fc1d to 7c0f44d Compare March 16, 2026 15:00

sapphi-red approved these changes Mar 17, 2026

View reviewed changes

graphite-app bot force-pushed the fix/new-expression-argument-validation branch from 7c0f44d to efeba28 Compare March 17, 2026 14:53

graphite-app bot merged commit efeba28 into main Mar 17, 2026
21 checks passed

graphite-app bot deleted the fix/new-expression-argument-validation branch March 17, 2026 15:05

graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Mar 17, 2026

Boshen mentioned this pull request Mar 19, 2026

release(crates): oxc v0.121.0 #20515

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

fix(ecmascript): add argument validation for NewExpression side-effect detection#20395
graphite-app[bot] merged 1 commit intomainfrom
fix/new-expression-argument-validation

Dunqing commented Mar 15, 2026 •

edited

Loading

Uh oh!

Dunqing commented Mar 15, 2026 •

edited

Loading

Uh oh!

codspeed-hq bot commented Mar 15, 2026 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

graphite-app bot commented Mar 15, 2026 •

edited

Loading

Uh oh!

sapphi-red left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

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

Uh oh!

Summary

Problem

Cross-tool comparison

Solution

Also fixes

Test plan

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!

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

Uh oh!

Merging this PR will not alter performance

Footnotes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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

Uh oh!

Merge activity

Uh oh!

sapphi-red left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Dunqing commented Mar 15, 2026 •

edited

Loading

Dunqing commented Mar 15, 2026 •

edited

Loading

codspeed-hq bot commented Mar 15, 2026 •

edited

Loading

graphite-app bot commented Mar 15, 2026 •

edited

Loading