fix(minifier): don't treat a conditionally-assigned var as write-once#24650
Merged
Conversation
Member
Author
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. |
Merging this PR will not alter performance
Comparing Footnotes
|
Contributor
Monitor OxcCommit:
|
Dunqing
force-pushed
the
minifier-conditional-var-declarator
branch
2 times, most recently
from
July 19, 2026 05:28
05e1aea to
af4841c
Compare
Dunqing
force-pushed
the
minifier-conditional-var-declarator
branch
from
July 19, 2026 05:46
af4841c to
0aa2966
Compare
Dunqing
marked this pull request as ready for review
July 19, 2026 14:50
Contributor
Merge activity
|
…#24650) A `var` initializer nested under a conditional does not dominate later reads: when the branch is not taken, the binding still has its hoisted `undefined` value. After single-statement block flattening, the declarator can become a brace-less `if` body with function scope, so the previous scope-based check incorrectly treated the initializer as unconditional and inlined it. This PR requires a hoisted initializer to be a direct item in a function or program statement list before caching its constant or fresh-value fact. Conditional, loop, labeled, and other nested statement positions are rejected conservatively. This can retain safe freshness-based folds, but the minsize corpus is byte-identical. For a binding with exactly one runtime value declaration, a lone conditional falsy initializer remains foldable in boolean context because both possible values—the initializer and hoisted `undefined`—are falsy. ## Example ```js function test() { if (window.doesntExist) var callback = true; return () => console.log(callback ? "ng" : "ok"); } ``` The previous output replaced the callback body with `console.log("ng")`. This PR keeps the conditional read. It also prevents a conditional fresh initializer from hiding a required `TypeError` on a later member write. ## Stack boundary This lower PR does not cover bindings with multiple runtime value declarations. At this commit alone, source-ordered value facts for those bindings remain unsound; the required next stack entry, #24658, closes that gap. The two PRs should be reviewed and merged back-to-back. The full `oxc_minifier` suite passes with 616 tests and 42 ignored; clippy, formatting, runtime and repeat-compression repros pass, and regenerated minsize and allocation snapshots are byte-identical. Fixes #24531. ## Stack 1. **#24650 — conditional `var` initializer facts** 2. #24658 — redeclared binding facts Implemented with AI assistance (Claude Code and OpenAI Codex). The contributor remains responsible for reviewing, understanding, and submitting the changes under the repository AI usage policy.
graphite-app
Bot
force-pushed
the
minifier-conditional-var-declarator
branch
from
July 19, 2026 14:52
0aa2966 to
921b834
Compare
graphite-app Bot
pushed a commit
that referenced
this pull request
Jul 19, 2026
Multiple runtime declarations can share one semantic `SymbolId`, while source-order traversal stores each declaration's constant, boolean-falsy, and fresh-value facts in the same slot. A nested function can consume the first declaration's facts before traversal reaches a later declaration, so invalidating only on the later visit cannot repair an already-applied fold.
This PR checks semantic redeclaration metadata at the first producer. When a binding has multiple runtime value declarations, it withholds all declaration-derived value facts from every producer. Type-only merges remain eligible, and per-symbol reference counts are preserved.
## Example
```js
export function f(a) {
if (a) var x = true;
else var x = false;
return x ? "ok" : "fail";
}
```
After this PR, minification preserves the branch-dependent test:
```js
export function f(a){if(a)var x=!0;else var x=!1;return x?`ok`:`fail`}
```
The rule is deliberately conservative. It also protects member writes when one declaration may alias an external object, but it gives up valid source-order folds such as `var x = 1; var x = 2; return () => x`. A regression test pins that behavior. Recovering same-value or otherwise benign redeclarations would require reaching-definition or dominance analysis rather than this source-ordered fact cache.
The check only covers declarations sharing one `SymbolId`. Sloppy Annex B block functions can update an existing `var` through a separate semantic symbol; current `main` and this stack still mishandle that pre-existing case, tracked in #24659.
Together with required lower PR #24650, this makes cached declaration facts conservative for conditional positions and same-symbol redeclarations.
The full `oxc_minifier` suite passes with 621 tests and 42 ignored; clippy, formatting, the #24603 runtime and repeat-compression repros pass, and regenerated minsize and allocation snapshots are byte-identical.
Fixes #24603.
## Stack
1. #24650 — conditional `var` initializer facts
2. **#24658 — redeclared binding facts**
Implemented with AI assistance (Claude Code and OpenAI Codex). The contributor remains responsible for reviewing, understanding, and submitting the changes under the repository AI usage policy.
camc314
added a commit
that referenced
this pull request
Jul 21, 2026
### 💥 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]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

A
varinitializer nested under a conditional does not dominate later reads: when the branch is not taken, the binding still has its hoistedundefinedvalue. After single-statement block flattening, the declarator can become a brace-lessifbody with function scope, so the previous scope-based check incorrectly treated the initializer as unconditional and inlined it.This PR requires a hoisted initializer to be a direct item in a function or program statement list before caching its constant or fresh-value fact. Conditional, loop, labeled, and other nested statement positions are rejected conservatively. This can retain safe freshness-based folds, but the minsize corpus is byte-identical.
For a binding with exactly one runtime value declaration, a lone conditional falsy initializer remains foldable in boolean context because both possible values—the initializer and hoisted
undefined—are falsy.Example
The previous output replaced the callback body with
console.log("ng"). This PR keeps the conditional read. It also prevents a conditional fresh initializer from hiding a requiredTypeErroron a later member write.Stack boundary
This lower PR does not cover bindings with multiple runtime value declarations. At this commit alone, source-ordered value facts for those bindings remain unsound; the required next stack entry, #24658, closes that gap. The two PRs should be reviewed and merged back-to-back.
The full
oxc_minifiersuite passes with 616 tests and 42 ignored; clippy, formatting, runtime and repeat-compression repros pass, and regenerated minsize and allocation snapshots are byte-identical.Fixes #24531.
Stack
varinitializer factsImplemented with AI assistance (Claude Code and OpenAI Codex). The contributor remains responsible for reviewing, understanding, and submitting the changes under the repository AI usage policy.