fix(minifier): keep unary + when the other operand has side effects (#25645)

`substitute_unary_plus` removes a `+` when the enclosing binary operator
will do the `ToNumber` conversion anyway. The existing doc comment
reasons carefully about `ToNumber` vs `ToNumeric`, but not about *when*
each runs.

`+a` converts `a` while evaluating `+a`, before the right operand is
evaluated at all. `a - n` converts it in
`ApplyStringOrNumericBinaryOperator`, after `n` has been evaluated.
Dropping the `+` therefore moves the conversion across `n`, and a right
operand with a side effect can change what the conversion sees:

```js
var xs = [];
console.log((+xs) - (xs.push(1), 0)); // 0, converted while `xs` was still empty

// oxc emitted:
console.log(xs - (xs.push(1), 0));    // 1, converted after `xs` grew
```

This PR requires the right operand to be free of side effects before
dropping a `+` in the left position. It cannot observe the conversion
either: under the "Coercion Methods Are Pure" assumption the conversion
runs no user code, so there is nothing to observe.

The right position (`n - +a`) is left as is: `n` is evaluated first
either way, and its `ToNumeric` is a no-op because the existing guard
already established that `n` is a number, so nothing runs between
evaluating `a` and converting it.

`cargo minsize` moves the typescript.js gzip figure down by 10 bytes;
every minified size is identical.

The reproduction was found by the minifier fuzzer (#25594) at seed
12397.

### Differential testing

Every case below was run through oxc and then executed in Node against
the unminified source, comparing the result, the mutated state, and the
error constructor *and message*.

| shape | cases | value worlds | runs |
| --- | ---: | ---: | ---: |
| operator × 37 hand-written right operands | 814 | 41 | 33,374 |
| right operand reading a closure variable the conversion may write |
374 | 41 | 15,334 |
| argument shape (`+a.v`, `+f()`, `+(seq(), a)`, `+(+a)`, getter) ×
nesting | 3,168 | 41 | 129,888 |
| random expressions, 3 seeds, depth 2-3 | 7,500 | 41 | 307,500 |

The worlds cover `[]`, `{ valueOf: () => 10n }`, `Symbol()`,
`Object.create(null)`, objects with getters, `Proxy`, and the numeric
edge cases.

No difference survives, other than one that predates this pass and is
discussed in the doc comment: for a `BigInt` argument both forms throw a
`TypeError`, but `+a` says "Cannot convert a BigInt value to a number"
while `a - 1` says "Cannot mix BigInt and other types". The error kind
never changes.

---

Written with AI assistance (Claude Code). Reviewed by me, and the
before/after behaviour was checked against Node.js.

---------

Signed-off-by: Kotaro Chikuba <miz404@gmail.com>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

crates/oxc_minifier/src/peephole/substitute_alternate_syntax.rs

@@ -276,6 +276,10 @@ impl<'a> PeepholeOptimizations {
             Ancestor::BinaryExpressionLeft(e) => {
                 Self::is_binary_operator_that_does_number_conversion(*e.operator())
                     && e.right().value_type(ctx).is_number()
+                    // The right operand is evaluated between the argument of `+`
+                    // and the conversion the operator performs on it, so it must
+                    // not affect that conversion. See the note below.
+                    && !e.right().may_have_side_effects(ctx)
             }
             Ancestor::BinaryExpressionRight(e) => {
                 Self::is_binary_operator_that_does_number_conversion(*e.operator())
@@ -312,7 +316,26 @@ impl<'a> PeepholeOptimizations {
     /// - When `a` is not a object nor a BigInt, `ToNumeric(a)` and `ToNumber(a)` works the same.
     ///   Because the step 2 in `ToNumeric` is always `false`.
     ///
-    /// Thus, removing `+` is fine.
+    /// Thus, removing `+` is fine as far as the conversion itself goes.
+    ///
+    /// What it does change is *when* the conversion happens. Step 1 runs while
+    /// evaluating `+a`, before `n` is evaluated at all; step 2 runs after. So
+    /// evaluating `n` must not affect the conversion of `a`:
+    ///
+    /// ```js
+    /// var xs = [];
+    /// (+xs) - (xs.push(1), 0); // 0, converted while `xs` was still empty
+    /// xs - (xs.push(1), 0);    // 1, converted after `xs` grew
+    /// ```
+    ///
+    /// Requiring `n` to be free of side effects rules that out. `n` cannot
+    /// observe the conversion either, because the conversion runs no user code
+    /// under the "Coercion Methods Are Pure" assumption (see
+    /// `docs/ASSUMPTIONS.md`).
+    ///
+    /// For `n - +a` the ordering holds regardless: `n` is evaluated first
+    /// either way, and step 3 is a no-op because `n` is already a number, so
+    /// nothing runs between evaluating `a` and converting it.
     fn is_binary_operator_that_does_number_conversion(operator: BinaryOperator) -> bool {
         matches!(
             operator,

crates/oxc_minifier/tests/peephole/substitute_alternate_syntax.rs

@@ -631,6 +631,37 @@ fn test_remove_unary_plus() {
     test_same("v = +a | b");
 }
 
+/// `+a` converts `a` as soon as `a` is evaluated, while `a - n` converts it only
+/// after `n` has been evaluated too. A right operand with a side effect can
+/// therefore change what the conversion sees:
+///
+/// ```js
+/// var xs = [];
+/// (+xs) - (xs.push(1), 0); // 0, converted while `xs` was still empty
+/// xs - (xs.push(1), 0);    // 1, converted after `xs` grew
+/// ```
+///
+/// The right operand is evaluated before either conversion happens, so `+` in
+/// that position can still go.
+#[test]
+fn test_remove_unary_plus_evaluation_order() {
+    // The right operand affects the conversion.
+    test_same("v = +xs - (xs.push(1), 0)");
+    test_same("v = +xs * f()");
+    test_same("v = +xs | g.h");
+    test_same("v = +xs - obj.n");
+
+    // A side-effect-free right operand cannot.
+    test("v = +xs - 1", "v = xs - 1");
+    test("v = +xs * 1e3", "v = xs * 1e3");
+    test("v = +xs - (2 * 3)", "v = xs - 6");
+    test("function f(x, a) { return +a - +!!x }", "function f(x, a) { return a - +!!x }");
+    test("function f(x, a) { return +a * +(x > 1) }", "function f(x, a) { return a * +(x > 1) }");
+
+    // The left operand is evaluated first either way.
+    test("v = (xs.push(1), 0) - +xs", "v = (xs.push(1), 0 - xs)");
+}
+
 #[test]
 fn test_fold_loose_equals_undefined() {
     test_same("v = foo != null");

tasks/minsize/minsize.snap

@@ -23,5 +23,5 @@ Original   | minified   | minified   | gzip       | gzip       | Iterations | Fi
 
 6.69 MB    | 2.12 MB    | 2.31 MB    | 453.48 kB  | 488.28 kB  |          5 | antd.js
 
-10.95 MB   | 3.31 MB    | 3.49 MB    | 850.31 kB  | 915.50 kB  |          4 | typescript.js
+10.95 MB   | 3.31 MB    | 3.49 MB    | 850.30 kB  | 915.50 kB  |          4 | typescript.js