Skip to content

refactor(minifier): centralize reorder stability checks#24699

Merged
graphite-app[bot] merged 1 commit into
mainfrom
codex/refactor-reorder-stability-checks
Jul 20, 2026
Merged

refactor(minifier): centralize reorder stability checks#24699
graphite-app[bot] merged 1 commit into
mainfrom
codex/refactor-reorder-stability-checks

Conversation

@Dunqing

@Dunqing Dunqing commented Jul 20, 2026

Copy link
Copy Markdown
Member

Single-use substitution checked the same reorder-safety decision in separate forms for ordinary identifier reads and member assignment targets. That duplication made future stability rules easy to apply to one consumer but miss in another.

Route both consumers through shared resolved-symbol stability and closed-over block-scoped-read predicates. Each identifier reference is resolved once, after which the symbol-level checks share the result. The refactor preserves three non-obvious boundaries: unresolved references remain blocked through is_none_or, non-simple member objects remain conservative through the _ => true arm, and the inverted ordinary-read condition is equivalent to the previous resolved && !mutated && !closed_over check. The closed-over predicate is named for what it structurally proves rather than implying that every such read is currently in the TDZ. TDZ fixtures use parameter callees so future import handling cannot make them pass vacuously.

There is no behavior or output change. Verified with 621 passing oxc_minifier tests and 42 ignored tests, clippy with warnings denied, formatting, just minsize, and allocation regeneration; minsize and allocation snapshots are byte-identical.

Stack

  1. refactor(minifier): centralize reorder stability checks #24699 — centralize reorder stability checks
  2. fix(minifier): guard reordered identifier reads #24698 — guard reordered identifier reads

AI assistance: Codex and Claude were used for implementation, adversarial review, and verification. The contributor remains responsible for reviewing, understanding, and submitting the changes under the repository AI usage policy.

@github-actions github-actions Bot added the A-minifier Area - Minifier label Jul 20, 2026

Dunqing commented Jul 20, 2026

Copy link
Copy Markdown
Member Author

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

codspeed-hq Bot commented Jul 20, 2026

Copy link
Copy Markdown

Merging this PR will not alter performance

✅ 57 untouched benchmarks
⏩ 19 skipped benchmarks1


Comparing codex/refactor-reorder-stability-checks (5c65a14) with main (7b045cd)2

Open in CodSpeed

Footnotes

  1. 19 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 main (22f6523) during the generation of this report, so 7b045cd 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 codex/refactor-reorder-stability-checks branch 2 times, most recently from 5ef3e63 to 5c65a14 Compare July 20, 2026 06:59
@Dunqing
Dunqing marked this pull request as ready for review July 20, 2026 07:07
@graphite-app graphite-app Bot added the 0-merge Merge with Graphite Merge Queue label Jul 20, 2026
@graphite-app

graphite-app Bot commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Merge activity

Single-use substitution checked the same reorder-safety decision in separate forms for ordinary identifier reads and member assignment targets. That duplication made future stability rules easy to apply to one consumer but miss in another.

Route both consumers through shared resolved-symbol stability and closed-over block-scoped-read predicates. Each identifier reference is resolved once, after which the symbol-level checks share the result. The refactor preserves three non-obvious boundaries: unresolved references remain blocked through `is_none_or`, non-simple member objects remain conservative through the `_ => true` arm, and the inverted ordinary-read condition is equivalent to the previous `resolved && !mutated && !closed_over` check. The closed-over predicate is named for what it structurally proves rather than implying that every such read is currently in the TDZ. TDZ fixtures use parameter callees so future import handling cannot make them pass vacuously.

There is no behavior or output change. Verified with 621 passing `oxc_minifier` tests and 42 ignored tests, clippy with warnings denied, formatting, `just minsize`, and allocation regeneration; minsize and allocation snapshots are byte-identical.

## Stack

1. **#24699 — centralize reorder stability checks**
2. #24698 — guard reordered identifier reads

AI assistance: Codex and Claude were used for implementation, adversarial review, and verification. The contributor remains responsible for reviewing, understanding, and submitting the changes under the repository AI usage policy.
@graphite-app
graphite-app Bot force-pushed the codex/refactor-reorder-stability-checks branch from 5c65a14 to 6e92cc0 Compare July 20, 2026 09:07
graphite-app Bot pushed a commit that referenced this pull request Jul 20, 2026
A locally read-only identifier is not necessarily stable across a side-effecting call. ESM imports are live bindings, Script-root bindings can be changed by another script, and computed assignment keys bypassed the existing closed-over-TDZ guard. Single-use substitution could therefore move a binding read before a call and change program behavior.

The same incomplete stability query affected logical-assignment folding. A live imported member base could be cached before a call that reassigns the export, making `||=` or `??=` update the old object instead of the current binding.

Build on the shared reorder predicates from #24699. Imports, Script-root bindings, unresolved references, syntactically mutated symbols, and closed-over block-scoped reads are treated conservatively across ordinary reads, assignment-target objects, computed keys, and member-base folding. Stable module bindings and pure replacements remain optimizable.

## Example

```js
var key = "x";
function init(obj) {
  let value = mutate();
  obj[key] = value;
}
```

The previous substitution could produce:

```js
var key = "x";
function init(obj) {
  obj[key] = mutate();
}
```

That captures `key` before `mutate()` runs, although another script can change the global binding during the call. The declaration is now retained so the read remains after `mutate()`.

For imported member bases, main can rewrite `x.y || (mutate(), x.y = 3)` to `x.y ||= (mutate(), 3)`. When `mutate` reassigns the exported `x`, the original two-module runtime repro produces `3`, while the rewritten program produces `0`. The shared stability guard now prevents that fold.

Plain computed assignments still inline stable identifiers and literals. ECMAScript evaluates the key expression and `GetValue` before the right-hand side, but defers `ToPropertyKey` until the final `PutValue`, so arbitrary key expressions remain conservative without losing safe simple-key substitutions.

This improves the existing reorder guard; it does not add a general TDZ model. Sloppy mapped `arguments` mutation remains a pre-existing unsupported case and is outside this stack.

Verified with the full `oxc_minifier` suite (622 passed, 42 ignored), clippy with warnings denied, formatting, minsize, allocation snapshots, runtime ordering probes, and repeat-compression checks. `cargo minsize` measured 18 additional raw bytes across `echarts.js` and `typescript.js`, with no iteration-count changes. System-allocation metrics are unchanged; arena changes are dominated by retained code with minor fold-path shifts.

## Stack

1. #24699 — centralize reorder stability checks
2. **#24698 — guard reordered identifier reads**

AI assistance: Codex and Claude were used for implementation, adversarial review, and verification. The contributor remains responsible for reviewing, understanding, and submitting the changes under the repository AI usage policy.
@graphite-app
graphite-app Bot merged commit 6e92cc0 into main Jul 20, 2026
29 checks passed
@graphite-app graphite-app Bot removed the 0-merge Merge with Graphite Merge Queue label Jul 20, 2026
@graphite-app
graphite-app Bot deleted the codex/refactor-reorder-stability-checks branch July 20, 2026 09:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-minifier Area - Minifier

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant