Skip to content

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

Merged
graphite-app[bot] merged 1 commit intomainfrom
fix/side-effects-coercion-pure
Mar 17, 2026
Merged

fix(ecmascript): apply coercion-is-pure assumption to constructor side-effect detection#20420
graphite-app[bot] merged 1 commit intomainfrom
fix/side-effects-coercion-pure

Conversation

@Dunqing
Copy link
Member

@Dunqing Dunqing commented Mar 16, 2026

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

  • All 473 oxc_minifier tests pass (39 ignored)
  • All 13 oxc_ecmascript tests pass
  • Updated test expectations with new cases (new Date(0n), new ArrayBuffer(0n), new Error(x), Error(x))
  • Cross-tool research verified against ES Spec, esbuild, Terser, Rollup, SWC

Copy link
Member Author

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

@Dunqing Dunqing marked this pull request as ready for review March 16, 2026 08:25
@Dunqing Dunqing requested review from Boshen and sapphi-red March 16, 2026 08:32
Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

(probably there's other places, but pointing out these for now)

@graphite-app graphite-app bot force-pushed the refactor/side-effects-consolidate-helpers branch from 9e63e4a to ded6e3a Compare March 16, 2026 09:03
@graphite-app graphite-app bot force-pushed the fix/side-effects-coercion-pure branch from 77dd29d to 5267094 Compare March 16, 2026 09:03
@Dunqing Dunqing force-pushed the refactor/side-effects-consolidate-helpers branch from ded6e3a to 0fb31df Compare March 16, 2026 15:00
@Dunqing Dunqing force-pushed the fix/side-effects-coercion-pure branch from 5267094 to 171a35c Compare March 16, 2026 15:00
@Dunqing Dunqing marked this pull request as draft March 16, 2026 15:03
@codspeed-hq
Copy link

codspeed-hq bot commented Mar 16, 2026

Merging this PR will not alter performance

🎉 Hooray! codspeed-rust just leveled up to 4.4.1!

A heads-up, this is a breaking change and it might affect your current performance baseline a bit. But here's the exciting part - it's packed with new, cool features and promises improved result stability 🥳!
Curious about what's new? Visit our releases page to delve into all the awesome details about this new version.

✅ 53 untouched benchmarks
⏩ 3 skipped benchmarks1


Comparing fix/side-effects-coercion-pure (a67e3b1) with refactor/side-effects-consolidate-helpers (9e63e4a)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 refactor/side-effects-consolidate-helpers (0fb31df) during the generation of this report, so c2e911e 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/side-effects-coercion-pure branch from 171a35c to 5ac6ad0 Compare March 16, 2026 15:59
@Dunqing Dunqing marked this pull request as ready for review March 16, 2026 16:25
@Dunqing Dunqing force-pushed the fix/side-effects-coercion-pure branch from 76e76c8 to a67e3b1 Compare March 17, 2026 14:45
Copy link
Member

@sapphi-red sapphi-red left a comment

Choose a reason for hiding this comment

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

LGTM!

@Dunqing
Copy link
Member Author

Dunqing commented Mar 17, 2026

LGTM!

Thank you for reviewing!

@Dunqing Dunqing added the 0-merge Merge with Graphite Merge Queue label Mar 17, 2026
Copy link
Member Author

Dunqing commented Mar 17, 2026

Merge activity

@graphite-app graphite-app bot force-pushed the refactor/side-effects-consolidate-helpers branch 2 times, most recently from e67ca72 to 0922623 Compare March 17, 2026 15:06
@graphite-app graphite-app bot force-pushed the fix/side-effects-coercion-pure branch from a67e3b1 to 39c77de Compare March 17, 2026 15:06
…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
@graphite-app graphite-app bot force-pushed the refactor/side-effects-consolidate-helpers branch from 0922623 to 878eace Compare March 17, 2026 15:44
@graphite-app graphite-app bot force-pushed the fix/side-effects-coercion-pure branch from 39c77de to 4ae3f3f Compare March 17, 2026 15:45
Base automatically changed from refactor/side-effects-consolidate-helpers to main March 17, 2026 15:56
@graphite-app graphite-app bot removed the 0-merge Merge with Graphite Merge Queue label Mar 17, 2026
@graphite-app graphite-app bot merged commit 4ae3f3f into main Mar 17, 2026
22 checks passed
@graphite-app graphite-app bot deleted the fix/side-effects-coercion-pure branch March 17, 2026 15:57
Boshen added a commit that referenced this pull request Mar 19, 2026
### 🚀 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>
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.

2 participants