fix(minifier): propagate PURE annotation when inlining pure IIFE#21526
fix(minifier): propagate PURE annotation when inlining pure IIFE#21526gthb wants to merge 25 commits into
Conversation
…itializer Reproduces oxc-projectGH-17480: when a pure-annotated IIFE is inlined as a variable initializer, the annotation is dropped from the inner call, so the unused-declaration pass cannot remove it.
When substituting a pure-annotated IIFE with its body expression (e.g. `/* @__PURE__ */ (() => foo())()` -> `foo()`), the annotation was dropped. This prevented downstream passes from removing the call when its result is unused, so `var x = /* @__PURE__ */ (() => foo())()` became `foo();` instead of being removed entirely. Carry the annotation onto the inlined CallExpression or NewExpression so the unused-declaration pass can still drop it. Closes oxc-project#17480
Merging this PR will not alter performance
Comparing Footnotes
|
…notation
Add tests for `/* @__PURE__ */ (() => { foo() })()` in both the
inline-preservation and end-to-end unused-var-removal cases. This path
takes the sequence branch of `substitute_iife_call` that wasn't covered
by the initial fix-up tests. Fold the three take/propagate-pure call
sites into a single `take_propagating_pure` helper.
…e` paths Plug the two coverage gaps noted in branch review: a function-expression IIFE (which isn't inlined — its outer `pure` is what the unused-declaration pass keys off), and the `annotations: false` path (which must ignore PURE annotations end-to-end, including during IIFE body inlining).
…ng PURE When inlining a pure-annotated IIFE whose body is a sequence expression (e.g. `/* @__PURE__ */ (() => (foo(), bar()))()`), the outer PURE flag was dropped because `take_propagating_pure` only annotated a top-level `CallExpression` or `NewExpression`. Extract the annotation step into `mark_inlined_pure` and recurse into `SequenceExpression.expressions` so every call/new element inherits the outer annotation, letting later passes drop them.
If the inlined call already carries its own `/* @__PURE__ */`, the outer annotation simply lands on a `pure` flag that is already set. Lock that in so a future change to the propagation helper doesn't accidentally duplicate, clear, or require a conditional around the inner annotation.
Extend `mark_inlined_pure` so the outer `/* @__PURE__ */` of an arrow IIFE reaches inner calls inside more container shapes when the IIFE is inlined: - `ConditionalExpression` (test, consequent, alternate) - `LogicalExpression` (left, right) - `ChainExpression` (via a small `ChainElement` helper) - TS wrappers: `as`, `!`, `satisfies`, `<T>x`, and `x<T>` Each arm recurses with the same helper, so arbitrary nesting (e.g. `(() => c ? (a(), b()) : d())()`) is handled too. Without this, code like `/* @__PURE__ */ (() => c ? foo() : bar())()` was left with the conditional's call branches un-annotated and the surrounding unused variable could not be dropped. Cover the common body shapes in `remove_unused_expression.rs` (showing the annotation lands on the inner call) and in `remove_unused_declaration.rs` (showing the declaration now drops for conditional/logical/optional-chain bodies). The TS wrappers are covered at the expression level only — the unused-declaration pass does not strip them, so `var x = /* @__PURE__ */ a() as any;` stays put.
`TSAsExpression`, `TSNonNullExpression`, `TSSatisfiesExpression`, `TSTypeAssertion`, and `TSInstantiationExpression` are all runtime-transparent — they are stripped before execution — but the `Expression::may_have_side_effects` match was falling through to the conservative `_ => true` default for all of them. That kept e.g. `var x = /* @__PURE__ */ a() as any;` alive even when `x` is unused, because the unused-declaration pass consults that predicate before dropping the initializer. Delegate to the inner expression for each wrapper. Combined with the existing PURE propagation during IIFE inlining, declarations like `var x = /* @__PURE__ */ (() => a() as any)();` now drop entirely in TypeScript sources. This crate is shared; the minifier, linter, transformer, and isolated- declarations test suites all still pass, and `cargo coverage -- minifier` shows no snapshot regressions.
Pin the contract at the predicate layer, not just via the minifier's end-to-end drops: for each TS wrapper (`as`, `!`, `satisfies`, generic instantiation, and a nested mix), verify the side-effect answer matches the inner expression. This catches a future regression — a typo in one arm, a feature-flag gate, or accidental removal — at the unit level rather than needing the whole unused-declaration pipeline to surface it.
Extend `mark_inlined_pure` to recurse through `UnaryExpression` and `BinaryExpression` so a `/* @__PURE__ */`-annotated IIFE with a body like `!foo()` or `a() == b()` still has its annotation preserved on the inner calls when inlined. For `!foo()` the side-effect check delegates purely to the argument, so this is enough to drop an unused declaration; for comparison operators the check is a disjunction over operand side effects, likewise sufficient. Arithmetic binary operators still short-circuit in `may_have_side_effects` via `to_primitive` value-type checks that can't be satisfied by an unknown call's return, so PURE forwarding buys nothing there — harmless when it doesn't help. `TaggedTemplateExpression` has no `pure` flag, so the annotation is silently dropped for tagged-template IIFE bodies. Documented in the helper's doc comment and covered by a test showing the current output.
…ression` The same five TS wrapper variants were enumerated verbatim in `may_have_side_effects` and in `mark_inlined_pure`. Collapse each into a single multi-variant arm that delegates to `get_inner_expression` / `get_inner_expression_mut` — the canonical TS/paren-unwrap helpers in `oxc_ast`. No behavior change (the helpers skip the same set plus `ParenthesizedExpression`, which already has its own arm in `may_have_side_effects` and is stripped before `mark_inlined_pure` runs).
…gate_pure` Reads as two verbs rather than one ambiguous participle-as-adjective.
Previously all tests for `mark_inlined_pure` used a single-level body (call, unary+call, binary+calls, etc.). Add a test where the body stacks a `BinaryExpression` over a `UnaryExpression` over a call, so the recursion actually descends multiple container levels before reaching the call it marks.
…sitions `mark_inlined_pure` previously stopped at the outermost call/new and at member-access expressions. When the IIFE body was something like `foo(bar())` or `foo().bar`, only the outer call got the PURE mark, leaving the inner `bar()` or `foo()` unmarked even though the outer `/* @__PURE__ */` guaranteed the whole body was side-effect-free. Extend the recursion to: - `CallExpression` / `NewExpression`: descend into the callee and each argument (including spread elements) after marking the outer pure. - `StaticMemberExpression` / `ComputedMemberExpression` / `PrivateFieldExpression`: descend into the object, which may contain nested calls. - `ChainElement`: apply the same treatment to chain-element variants so optional chains with inner calls carry the annotation through. The outer mark alone was already enough to drop the declaration in the typical case; the added recursion keeps the PURE invariant consistent for every call in the body, so later passes can make the same decisions if the outer call is ever unwrapped.
…d_pure` The explicit match arm for the five TS wrapper variants was just calling `get_inner_expression_mut()` and recursing. Moving that unwrap to the top of the function folds all wrapper (and `ParenthesizedExpression`) handling into a single line, keeping the subsequent match focused on runtime-meaningful expression shapes. No behavior change.
The doc string describing the `pure` flag was sitting above `arguments`, so generated builder docs (e.g., `expression_new_with_pure`) attributed the "marked with /* @__PURE__ */" comment to the wrong parameter. Move the doc comment one field down to match `CallExpression`, and regenerate `ast_builder.rs`.
…aration` The positive cases assert that pure-annotated IIFEs around various body shapes drop entirely. Add the symmetric negative assertions so reviewers can see what stays behind when the body itself has side effects the outer PURE annotation can't cover: - An undeclared global in the conditional's test or the logical's left-hand side leaves a bare `undeclaredGlobal;` after the now-pure arms fold away. - A spread argument leaves `[...xs];` because the iterator protocol runs regardless of whether the surrounding call is pure. - A tagged-template body has no `pure` flag to receive the annotation, so the call is preserved as `foo``;`.
The unary / binary positive cases (`!a()`, `a() == b()`) drop because `==` and `!` are simple disjunctions over operand side effects. Add matching negative cases for `a() + b()` and `+a()` to make the arithmetic-side asymmetry visible: even with both inner calls marked PURE, `to_numeric` / `to_primitive` returns `Undetermined` for an unknown call, so the `+` / unary-plus stays side-effectful and only the `var` part collapses. The actual minified output — `/* @__PURE__ */ a() + /* @__PURE__ */ b();` — also doubles as visual evidence that the inner-call PURE marks did propagate. Also rewrite the inscrutable comment on the positive cases to forward to the new negative cases instead of describing the limitation in isolation.
The doc on `take_and_propagate_pure` only listed the original container shapes, omitting the call/new callee/argument and member-object recursion added later. Refresh the list and add an explicit "missed-optimization gaps" section covering `TaggedTemplateExpression`, `ArrayExpression`, `ObjectExpression`, and `TemplateLiteral` interpolations, so a future reader doesn't think those positions are deliberately ignored without context. Also note that the top-of-function `get_inner_expression_mut()` strip relies on `normalize.rs` having removed `ParenthesizedExpression` already, and add reciprocal "keep in sync" notes on `mark_inlined_pure` and `mark_chain_element_pure` to highlight that the two enums overlap on the call / TS-non-null / member variants.
…alse` The existing `treeshake_options_annotations_false` case exercised the no-inner-mark variant: the IIFE inlining happens but PURE is not forwarded onto the unwrapped call. Add the inner-marked variant to cover the orthogonal property — when the parser already attached PURE to the inner call, the inlining must leave it intact, since the minifier's annotations option only governs whether the minifier acts on the flag, not whether codegen prints it. Without this assertion, a future regression that naively cleared inner PURE flags during the no-propagate path would go unnoticed.
Pin the missed-optimization gap noted on `mark_inlined_pure`: `(() => g.x)()` inlines to `g.x`, which has no `pure` flag of its own, so the var declaration can't drop and we collapse to `g.x;`.
Two new cases for `mark_chain_element_pure`: - `(() => foo()?.bar())()` — the chain's tail call gets PURE (codegen prints it at the front of the chain) and the recursion through the member object also marks the inner `foo()`. - `(() => foo?.[bar()])()` — the computed-member index isn't recursed into, so the call in the index position keeps no PURE annotation.
Previously only exercised indirectly via `annotations: false`, which masks the outer PURE. A direct positive test documents the plain inline path.
… gap `mark_inlined_pure` doesn't recurse into `AssignmentExpression.right`. The assignment itself is side-effectful so the inlined expression can't drop anyway, but a later pass that substitutes the assigned value would lose the inner call's PURE mark. Add it to the listed gaps.
Dunqing
left a comment
There was a problem hiding this comment.
Review
Carefully traced through the recursion in mark_inlined_pure / mark_chain_element_pure and the negative-test surface. LGTM — recommend approval.
What this fixes
The outer pure = true was silently dropped when substitute_iife_call replaced *e = expr.take_in(ctx.ast). By the time the unused-declaration pass saw var x = foo(), the assertion was gone and the RHS couldn't be proved pure. The new helpers carry the annotation onto every shape that was syntactically inside the original IIFE call.
Strengths
- Both DCE-only and full minify benefit.
substitute_iife_callruns in both branches inpeephole/mod.rs(:317and:373), so rolldowntreeshake: true/minify: "dce-only"users — likely the ones who reported #17480 — get the fix too. MayHaveSideEffectschange for TS wrappers is a strict improvement. Previously fell through to_ => true, so any(pureCall as T)/pureCall!/pureCall satisfies Twas over-flagged. Now delegates viaget_inner_expression.- Negative tests pin the conservative cases: undeclared globals (
c ? a() : b()wherecis an unknown global), spread / iterator protocol, tagged templates, member access on unknown globals, arithmetic / unary-plus coercion. Each is a documented missed-optimization gap, not a correctness gap. - Idempotent under repeated PURE marks and under
annotations: false. The twoannotations: falsetests are the right shape: inlining still fires, but propagation is gated onis_pure, and an inner PURE that the parser already attached survives unchanged. mark_chain_element_pureis exhaustive onChainElement(no wildcard). New variants will fail compilation — right call for this kind of helper.get_inner_expression_mutat the top defends against pre-normalization callers — the inline comment correctly notes parens would be silently stripped if anyone wired this in beforenormalize.rs. Worth keeping as a guardrail.- The drive-by
NewExpression::puredoc-comment fix is correct.
Soundness check
The recursion preserves the developer's contract: /* @__PURE__ */ was a promise about the entire IIFE call. Unwrapping moves the body into the call's syntactic position, so any sub-expression that was originally covered by the assertion remains covered. The recursion stops at shapes the outer PURE couldn't have rescued anyway (assignment RHS, tagged-template tag, array/object/template-literal elements), which is also exactly the line the negative tests draw.
I couldn't find a case where marking a call pure = true here would change observable behavior.
Minor (non-blocking) observations
- Doubled annotations in output like
var a = /* @__PURE__ */ (/* @__PURE__ */ foo())?.bar()are semantically equivalent and helpful for follow-on passes; just slightly noisy. Not worth changing. SequenceExpressionwrapping in the{ stmt() }branch can produce((a(), b()), undefined)if the inlined body is already a sequence.remove_sequence_expressionflattens this later — just noting it.ConditionalExpression::testrecursion: marked pure even though the test is evaluated unconditionally. Correct (outer PURE covered the test too), and the inline comment already says this.
What
Fix bug: pure-annotated IIFEs that should be dead-code-eliminated were leaving behind a bare expression statement.
var removeThis = /* @__PURE__ */ (() => stuff())()minified tostuff();instead of being dropped entirely.Fixes #17480
Root Cause
IIFE substitution runs before the unused-declaration pass and was discarding the outer PURE when unwrapping
(() => foo())()tofoo(). By the time the unused-declaration pass sawvar x = foo()it could no longer prove the RHS pure.Fix
The three inlining branches in
substitute_iife_callnow go through a small helpertake_and_propagate_purethat, when the outer IIFE was pure, callsmark_inlined_pureon the taken expression.mark_inlined_purewalks every shape the outer PURE legitimately covers —Sequence/Conditional/Logical/Unary/Binary/Chaincontainers, call & new callee/argument positions (including spreads), member-object positions, and TS wrappers (stripped viaget_inner_expression_mut) — stampingpure = trueon eachCall/Newit finds. A siblingmark_chain_element_puremirrors the logic forChainElementchildren.Expression::may_have_side_effectsinoxc_ecmascriptis also taught that the five TS wrappers are runtime-transparent and delegates toget_inner_expression. Required for the IIFE fix to actually drop a(() => foo() as any)()repro.Not addressed
A few positions silently lose PURE because the AST has no
pureflag for them (TaggedTemplateExpression, array/object elements, template-literal interpolations, computed-member indices). Missed-optimization gaps, not correctness issues, and pinned by negative tests.Drive-by
The
NewExpression::puredoc comment was attached to theargumentsfield; moved it ontopurewhere it belongs.