fix(ecmascript): apply coercion-is-pure assumption to constructor side-effect detection#20420
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via 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. |
sapphi-red
left a comment
There was a problem hiding this comment.
(probably there's other places, but pointing out these for now)
9e63e4a to
ded6e3a
Compare
77dd29d to
5267094
Compare
ded6e3a to
0fb31df
Compare
5267094 to
171a35c
Compare
Merging this PR will not alter performance🎉 Hooray!
|
171a35c to
5ac6ad0
Compare
76e76c8 to
a67e3b1
Compare
Thank you for reviewing! |
Merge activity
|
e67ca72 to
0922623
Compare
a67e3b1 to
39c77de
Compare
…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
0922623 to
878eace
Compare
39c77de to
4ae3f3f
Compare
### 🚀 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>

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 vianew_expr_first_arg_may_be_symbol. BigInt is fine for both (ToString(BigInt)works, Number converts BigInt→Number).new Date/ArrayBuffer(arg)—ToNumber(Symbol)ANDToNumber(BigInt)throw TypeError. Check vianew_expr_first_arg_may_be_symbol_or_bigint.Error constructors —
new Error(Symbol())/Error(Symbol())throw (ToString(Symbol)). Removed fromis_unconditionally_pure_constructorandis_pure_callable_constructor. Addedis_error_constructor()helper with Symbol check.TypedArray constructors — use
value_type()instead ofto_primitive(): object args call@@iterator(NOT a coercion method), BigInt args causeToNumberto 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 tois_pure_callable_constructor.Number(x)/Symbol(x)calls — keep special handling (ToNumeric(Symbol)/ToString(Symbol)throw).is_pure_collection_constructor— now verifiesundefinedis actually global (not shadowed). Removed invalid issue link (XXXX).Also fixes: typo
"BitUint64Array"→"BigUint64Array"inis_known_global_constructor.Cross-tool comparison
new String(x)new Number(x)new Date(x)new Date(0n)new ArrayBuffer(x)new Error(x)new Uint8Array(x)new Uint8Array({})String(x)callError(x)callOxc 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),@@iteratoron objects).Test plan
oxc_minifiertests pass (39 ignored)oxc_ecmascripttests passnew Date(0n),new ArrayBuffer(0n),new Error(x),Error(x))