From f49229d566973ce5d4ab514b38ddcd9fc0c702aa Mon Sep 17 00:00:00 2001 From: Kotaro Chikuba Date: Fri, 14 Aug 2026 10:23:22 +0900 Subject: [PATCH] fix(minifier): keep side effects when rotating bitwise operands (#25596) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by the semantic fuzzer added in #25594 (seed `29024`). ## What `try_fold_left_child_op` rewrites `(a op b) op right` (`&`, `|`, `^`) into `x op constant` by folding `right` together with whichever of `a`/`b` evaluates, discarding both. It never checked the discarded operands for side effects. Being constant-evaluable does not imply being side-effect free: `[(y = 9), 1].length` evaluates to `2` while still assigning `y`. ```js var y = 2, c = 0; (3 ^ c) ^ [(y = 9), 1].length; console.log(y); // 9, minified to 2 — the assignment is dropped ``` The minified output is `c ^ 1`: the array literal, and the assignment inside it, are gone. ## How Bail out when the discarded right operand — or the left-child operand folded with it — may have side effects. The shape check (`left.operator != op`) now runs before `may_have_side_effects`, which walks the subtree, so the common non-matching case stays as cheap as before. Note `fold_static_member_expr` already guards its `.length` fold with the same check; this restores the symmetry. ## Testing - New regression test `test_fold_left_child_op_keeps_side_effects`, covering `^`, `|`, `&` and both operand positions, plus a positive case asserting the fold still happens when nothing has side effects. - `cargo test -p oxc_minifier -p oxc_ecmascript`, `cargo clippy --all-targets`, `cargo fmt --check` all pass. - `just minsize`: no snapshot change, so no size regression. ## AI disclosure This change was produced with AI assistance (Claude Code). I have reviewed and understand every line; the root cause and the minimal reproduction were verified by hand against Node.js. --------- Co-authored-by: Claude Opus 5 (1M context) --- .../oxc_minifier/src/peephole/fold_constants.rs | 17 ++++++++++++++--- .../tests/peephole/fold_constants.rs | 16 ++++++++++++++++ 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/crates/oxc_minifier/src/peephole/fold_constants.rs b/crates/oxc_minifier/src/peephole/fold_constants.rs index af760ff741..9deae13714 100644 --- a/crates/oxc_minifier/src/peephole/fold_constants.rs +++ b/crates/oxc_minifier/src/peephole/fold_constants.rs @@ -725,17 +725,28 @@ impl<'a> PeepholeOptimizations { BinaryOperator::BitwiseAnd | BinaryOperator::BitwiseOR | BinaryOperator::BitwiseXOR )); - let Expression::BinaryExpression(left) = &mut e.left else { + // Shape check first: `may_have_side_effects` walks the subtree, so it + // must not run for the common non-matching case. + let Expression::BinaryExpression(left) = &e.left else { return None; }; if left.operator != op { return None; } + if e.right.may_have_side_effects(ctx) { + return None; + } + + let Expression::BinaryExpression(left) = &mut e.left else { unreachable!() }; let (v, expr_to_move); - if let Some(result) = ctx.eval_binary_operation(op, &left.left, &e.right) { + if !left.left.may_have_side_effects(ctx) + && let Some(result) = ctx.eval_binary_operation(op, &left.left, &e.right) + { (v, expr_to_move) = (result, &mut left.right); - } else if let Some(result) = ctx.eval_binary_operation(op, &left.right, &e.right) { + } else if !left.right.may_have_side_effects(ctx) + && let Some(result) = ctx.eval_binary_operation(op, &left.right, &e.right) + { (v, expr_to_move) = (result, &mut left.left); } else { return None; diff --git a/crates/oxc_minifier/tests/peephole/fold_constants.rs b/crates/oxc_minifier/tests/peephole/fold_constants.rs index 14140ac016..f65d78ab7d 100644 --- a/crates/oxc_minifier/tests/peephole/fold_constants.rs +++ b/crates/oxc_minifier/tests/peephole/fold_constants.rs @@ -1635,3 +1635,19 @@ mod bigint { fold("({ ...{ ['__proto__']: null } })", "({ ['__proto__']: null })"); } } + +/// Rotating `(k1 op x) op right` into `x op (k1 op right)` drops `k1` and +/// `right`, so it is only sound when neither has side effects. `[expr].length` +/// evaluates to a constant while still running `expr`. +#[test] +fn test_fold_left_child_op_keeps_side_effects() { + fold_same("(3 ^ x) ^ [(y = 9), 1].length"); + fold_same("(x ^ 3) ^ [(y = 9), 1].length"); + fold_same("(3 | x) | [(y = 9), 1].length"); + fold_same("(3 & x) & [(y = 9), 1].length"); + fold_same("(3 ^ [(y = 9), 1].length) ^ x"); + fold_same("([(y = 9), 1].length ^ x) ^ 3"); + fold_same("(x ^ [(y = 9), 1].length) ^ 3"); + // Still folds when nothing has side effects. + fold("(3 ^ x) ^ [1, 1].length", "x ^ 1"); +}