feat(minifier): fold arithmetic over undefined and null operands#24485
Conversation
Merging this PR will not alter performance
Comparing Footnotes
|
Monitor OxcCommit:
|
1dbcde8 to
b7c2977
Compare
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7c2977c62
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
8a13ac3 to
b7c2977
Compare
b7c2977 to
6a3a0cd
Compare
Merge activity
|
) `extract_numeric_values` required two numeric literals, so arithmetic over values with no literal form never folded: `Infinity` and `NaN` identifiers normalize to non-finite numeric literals and fold, but `undefined` normalizes to `void 0` — a unary expression — so `void 0 * 2` stayed as-is, eight characters where `NaN` is three. The same gate kept tracked constants from folding: `let a; console.log(a * 2)` kept the read (and with it the declaration) alive where `console.log(NaN)` is smaller and lets the binding die. terser folds all of these; esbuild folds none. Surfaced by the review question on #24452. The extraction now falls back to the constant evaluator, which applies ToNumber (`undefined` → NaN, `null` → 0, `'2'` → 2) and refuses side-effectful operands; each arm's small-result size filters are unchanged. A cheap operand-kind gate keeps the fallback away from allocation-prone operands (a bigint literal heap-allocates only for ToNumber to bail; a call expression attempts string-method folds) — with it, the allocation snapshots are byte-identical to main. Addition already behaved this way (`void 0 + 1` folded), so this aligns the gated arms with it. Widening NaN-producing folds exposed a pre-existing miscompile that had to be fixed first: a NaN numeric literal prints as the identifier `NaN`, so `function f() { let NaN = 1; return 0 / 0; }` minified to `return NaN` — returning 1. `eval_binary` now bails when `NaN` / `Infinity` is shadowed at the fold site; bailing rather than emitting `0 / 0` keeps the fixed-point loop convergent, since replacing `0 / 0` with itself would set the changed flag on every pass. For non-binary producers (e.g. `parseInt` folds), `value_to_expr` materializes non-finite constants in shadowed scopes as `0/0` / `1/0` / `-1/0`, which the division fold then leaves alone. ## Example ```js let a; console.log(a * 2); console.log(void 0 * 2); ``` Before: ```js let a; console.log(a * 2), console.log(void 0 * 2); ``` After: ```js console.log(NaN), console.log(NaN); ``` Verified with the minifier unit suite (555 passing; the only expectation changes are the closure-compiler ports this enables: the commented-out `x = null * 1` → `x = 0` family now passes and `x = null ** 0` folds to `x = 1`), `--twice` idempotency, and byte-identical minsize and allocation snapshots. Fixes #24474 Implemented with AI assistance (Claude Code); design, review, and verification driven by the author per the repo AI-usage policy.
6a3a0cd to
2872366
Compare
) `extract_numeric_values` required two numeric literals, so arithmetic over values with no literal form never folded: `Infinity` and `NaN` identifiers normalize to non-finite numeric literals and fold, but `undefined` normalizes to `void 0` — a unary expression — so `void 0 * 2` stayed as-is, eight characters where `NaN` is three. The same gate kept tracked constants from folding: `let a; console.log(a * 2)` kept the read (and with it the declaration) alive where `console.log(NaN)` is smaller and lets the binding die. terser folds all of these; esbuild folds none. Surfaced by the review question on #24452. The extraction now falls back to the constant evaluator, which applies ToNumber (`undefined` → NaN, `null` → 0, `'2'` → 2) and refuses side-effectful operands; each arm's small-result size filters are unchanged. A cheap operand-kind gate keeps the fallback away from allocation-prone operands (a bigint literal heap-allocates only for ToNumber to bail; a call expression attempts string-method folds) — with it, the allocation snapshots are byte-identical to main. Addition already behaved this way (`void 0 + 1` folded), so this aligns the gated arms with it. Widening NaN-producing folds exposed a pre-existing miscompile that had to be fixed first: a NaN numeric literal prints as the identifier `NaN`, so `function f() { let NaN = 1; return 0 / 0; }` minified to `return NaN` — returning 1. `eval_binary` now bails when `NaN` / `Infinity` is shadowed at the fold site; bailing rather than emitting `0 / 0` keeps the fixed-point loop convergent, since replacing `0 / 0` with itself would set the changed flag on every pass. For non-binary producers (e.g. `parseInt` folds), `value_to_expr` materializes non-finite constants in shadowed scopes as `0/0` / `1/0` / `-1/0`, which the division fold then leaves alone. ## Example ```js let a; console.log(a * 2); console.log(void 0 * 2); ``` Before: ```js let a; console.log(a * 2), console.log(void 0 * 2); ``` After: ```js console.log(NaN), console.log(NaN); ``` Verified with the minifier unit suite (555 passing; the only expectation changes are the closure-compiler ports this enables: the commented-out `x = null * 1` → `x = 0` family now passes and `x = null ** 0` folds to `x = 1`), `--twice` idempotency, and byte-identical minsize and allocation snapshots. Fixes #24474 Implemented with AI assistance (Claude Code); design, review, and verification driven by the author per the repo AI-usage policy.
2872366 to
4d0c601
Compare
### 💥 BREAKING CHANGES - 54cc121 ast: [**BREAKING**] Split `MetaProperty` into `ImportMeta` and `NewTarget` (#24557) (camc314) ### 🚀 Features - 4c71560 parser: More friendly error for spread element in dynamic imports (#24705) (sapphi-red) - 7b045cd minfier: Drop last break from last switch case (#24673) (Armano) - 7d3c178 minifier: Remove unreachable recursive functions (#24125) (Dunqing) - 94f99b3 ast: Allow `NONE` to be passed to AST builder methods where `Option<ArenaVec>` is expected (#24629) (overlookmotel) - 77230c5 ast: Accept arrays for `ArenaVec` params of AST builder methods (#24621) (overlookmotel) - f08b152 allocator: Implement `FromIn` for array to `Vec` conversion (#24620) (overlookmotel) - 2338c13 track-memory-allocations: Track heap deallocs, alloc bytes, and peak growth (#24619) (Boshen) - 7aa4739 syntax,transformer: Move JSX entity decoder to `oxc_syntax` (#24617) (camc314) - 2b097c4 str: Export `Str` as `ArenaStr` (#24604) (overlookmotel) - 3acf4c1 minifier: Expand switch optimiation to remove empty cases (#24520) (Armano) - 129b759 parser: Improve diagnostics for unparenthesized LHS on exponential expr (#24569) (camc314) - 4d0c601 minifier: Fold arithmetic over undefined and null operands (#24485) (Dunqing) - 91541dd minifier: Drop empty switch statements (#24527) (Armano) - d05224d ast_visit: Generate VisitJs visitor that skips TypeScript type-space (#24499) (Boshen) - 3d22307 parser: Add `ParseOptions::enable_ident_hashes` (#24491) (Boshen) ### 🐛 Bug Fixes - 64c2241 minifier: Align class heritage removal with assumptions (#24533) (Dunqing) - 48b59f4 parser: Span ambient generator diagnostics (#24711) (camc314) - e750a82 ecmascript: Fix false negative for may_have_side_effects on dynamic property access (#24709) (sapphi-red) - f145d73 minifier: Guard reordered identifier reads (#24698) (Dunqing) - a2ef382 isolated-declarations: Reject `window.Symbol` as global symbol reference (#24689) (camc314) - b1bcf72 minifier: Invalidate facts for redeclared bindings (#24658) (Dunqing) - 921b834 minifier: Don't treat a conditionally-assigned var as write-once (#24650) (Dunqing) - 061af1f minifier: Avoid stale pure function summaries (#24636) (Dunqing) - 40c2f43 allocator: `Vec::from_array_in` do not allocate zero-length array (#24628) (overlookmotel) - 70994ae codegen: Preserve comments before expression operands (#24510) (Dunqing) - 7b4baff parser: Reject new import member access (#23459) (camc314) - 128b385 minifier: Clippy warning with no-debug-assertions (#24547) (camc314) - 8421feb parser: Use first `as` span for imported name (#24537) (leaysgur) - c517aa0 parser: Reject invalid accessor assertions (#24504) (camc314) ### ⚡ Performance - 884d9eb parser: Pre-size cover-grammar assignment target buffers (#24683) (Boshen) - d3f07a0 diagnostics: Box OxcDiagnosticInner to reduce binary size (#24665) (Boshen) - bcc9de0 parser: Defer diagnostic creation until parse exit (#24663) (Boshen) - c35d8ab allocator: Mark `ReplaceWith` panic path cold (#24515) (camc314) - ba65790 semantic, allocator: Branchless `clone_in` for semantic IDs (#24564) (overlookmotel) - 747feec parser: Build AST nodes with the AST builder instead of cloning (#24540) (Boshen) - ba35a0d react_compiler: Use IndexVec for dense id-keyed maps (#24549) (Boshen) - b685062 react_compiler: Keep small hot-path collections inline (#24514) (Marius Schulz) - a149e95 transformer: Outline rare expression exits (#24512) (camc314) - 7808a6e react_compiler: Make aliasing effects cheap to intern and clone (#24506) (Marius Schulz) - 3a36f2a react_compiler: Store AbstractValue reasons as a u16 bitmask (#24480) (Boshen) - 1c96753 react_compiler: Use FxHashMap for the lookup-only aliasing node map (#24490) (Boshen) Co-authored-by: Cameron <[email protected]>
extract_numeric_valuesrequired two numeric literals, so arithmetic over values with no literal form never folded:InfinityandNaNidentifiers normalize to non-finite numeric literals and fold, butundefinednormalizes tovoid 0— a unary expression — sovoid 0 * 2stayed as-is, eight characters whereNaNis three. The same gate kept tracked constants from folding:let a; console.log(a * 2)kept the read (and with it the declaration) alive whereconsole.log(NaN)is smaller and lets the binding die. terser folds all of these; esbuild folds none. Surfaced by the review question on #24452.The extraction now falls back to the constant evaluator, which applies ToNumber (
undefined→ NaN,null→ 0,'2'→ 2) and refuses side-effectful operands; each arm's small-result size filters are unchanged. A cheap operand-kind gate keeps the fallback away from allocation-prone operands (a bigint literal heap-allocates only for ToNumber to bail; a call expression attempts string-method folds) — with it, the allocation snapshots are byte-identical to main. Addition already behaved this way (void 0 + 1folded), so this aligns the gated arms with it.Widening NaN-producing folds exposed a pre-existing miscompile that had to be fixed first: a NaN numeric literal prints as the identifier
NaN, sofunction f() { let NaN = 1; return 0 / 0; }minified toreturn NaN— returning 1.eval_binarynow bails whenNaN/Infinityis shadowed at the fold site; bailing rather than emitting0 / 0keeps the fixed-point loop convergent, since replacing0 / 0with itself would set the changed flag on every pass. For non-binary producers (e.g.parseIntfolds),value_to_exprmaterializes non-finite constants in shadowed scopes as0/0/1/0/-1/0, which the division fold then leaves alone.Example
Before:
After:
Verified with the minifier unit suite (555 passing; the only expectation changes are the closure-compiler ports this enables: the commented-out
x = null * 1→x = 0family now passes andx = null ** 0folds tox = 1),--twiceidempotency, and byte-identical minsize and allocation snapshots.Fixes #24474
Implemented with AI assistance (Claude Code); design, review, and verification driven by the author per the repo AI-usage policy.