refactor(minifier): change AST in-place instead of returning Option<Expression>

Author: Boshen

crates/oxc_minifier/src/peephole/minimize_conditions.rs

@@ -20,46 +20,26 @@ impl<'a> PeepholeOptimizations {
         expr: &mut Expression<'a>,
         ctx: &mut Ctx<'a, '_>,
     ) {
-        let mut changed = false;
-        loop {
-            let mut local_change = false;
-            if let Some(folded_expr) = match expr {
-                Expression::UnaryExpression(e) => self.try_minimize_not(e, ctx),
-                Expression::BinaryExpression(e) => {
-                    if Self::try_compress_is_loose_boolean(e, ctx) {
-                        local_change = true;
-                    }
-                    Self::try_minimize_binary(e, ctx)
-                }
-                Expression::LogicalExpression(e) => self.minimize_logical_expression(e, ctx),
-                Expression::ConditionalExpression(logical_expr) => {
-                    if self.try_fold_expr_in_boolean_context(&mut logical_expr.test, ctx) {
-                        local_change = true;
-                    }
-                    self.try_minimize_conditional(logical_expr, ctx)
-                }
-                Expression::AssignmentExpression(e) => {
-                    if self.try_compress_normal_assignment_to_combined_logical_assignment(e, ctx) {
-                        local_change = true;
-                    }
-                    if Self::try_compress_normal_assignment_to_combined_assignment(e, ctx) {
-                        local_change = true;
-                    }
-                    Self::try_compress_assignment_to_update_expression(e, ctx)
+        match expr {
+            Expression::UnaryExpression(_) => self.try_minimize_not(expr, ctx),
+            Expression::BinaryExpression(e) => {
+                Self::try_compress_is_loose_boolean(e, ctx);
+                Self::try_minimize_binary(expr, ctx);
+            }
+            Expression::LogicalExpression(_) => self.minimize_logical_expression(expr, ctx),
+            Expression::ConditionalExpression(logical_expr) => {
+                self.try_fold_expr_in_boolean_context(&mut logical_expr.test, ctx);
+                if let Some(changed) = self.try_minimize_conditional(logical_expr, ctx) {
+                    *expr = changed;
+                    ctx.state.changed = true;
                 }
-                _ => None,
-            } {
-                *expr = folded_expr;
-                local_change = true;
             }
-            if local_change {
-                changed = true;
-            } else {
-                break;
+            Expression::AssignmentExpression(e) => {
+                self.try_compress_normal_assignment_to_combined_logical_assignment(e, ctx);
+                Self::try_compress_normal_assignment_to_combined_assignment(e, ctx);
+                Self::try_compress_assignment_to_update_expression(expr, ctx);
             }
-        }
-        if changed {
-            ctx.state.changed = true;
+            _ => {}
         }
     }
 
@@ -103,9 +83,9 @@ impl<'a> PeepholeOptimizations {
         }
         // "a op b" => "a op b"
         // "(a op b) op c" => "(a op b) op c"
-        let mut logic_expr = ctx.ast.logical_expression(span, a, op, b);
-        self.minimize_logical_expression(&mut logic_expr, ctx)
-            .unwrap_or_else(|| Expression::LogicalExpression(ctx.ast.alloc(logic_expr)))
+        let mut logic_expr = ctx.ast.expression_logical(span, a, op, b);
+        self.minimize_logical_expression(&mut logic_expr, ctx);
+        logic_expr
     }
 
     // `typeof foo === 'number'` -> `typeof foo == 'number'`
@@ -115,17 +95,15 @@ impl<'a> PeepholeOptimizations {
     //  ^^^^^^^^^^^^^^ `ctx.expression_value_type(&e.left).is_boolean()` is `true`.
     // `x >> +y !== 0` -> `x >> +y`
     //  ^^^^^^^ ctx.expression_value_type(&e.left).is_number()` is `true`.
-    fn try_minimize_binary(
-        e: &mut BinaryExpression<'a>,
-        ctx: &mut Ctx<'a, '_>,
-    ) -> Option<Expression<'a>> {
+    fn try_minimize_binary(expr: &mut Expression<'a>, ctx: &mut Ctx<'a, '_>) {
+        let Expression::BinaryExpression(e) = expr else { return };
         if !e.operator.is_equality() {
-            return None;
+            return;
         }
         let left = e.left.value_type(ctx);
         let right = e.right.value_type(ctx);
         if left.is_undetermined() || right.is_undetermined() {
-            return None;
+            return;
         }
         if left == right {
             match e.operator {
@@ -139,12 +117,14 @@ impl<'a> PeepholeOptimizations {
             }
         }
         if !left.is_boolean() {
-            return None;
+            return;
         }
         if e.right.may_have_side_effects(ctx) {
-            return None;
+            return;
         }
-        let mut b = e.right.evaluate_value(ctx).and_then(ConstantValue::into_boolean)?;
+        let Some(mut b) = e.right.evaluate_value(ctx).and_then(ConstantValue::into_boolean) else {
+            return;
+        };
         match e.operator {
             BinaryOperator::Inequality | BinaryOperator::StrictInequality => {
                 e.operator = BinaryOperator::Equality;
@@ -154,14 +134,15 @@ impl<'a> PeepholeOptimizations {
                 e.operator = BinaryOperator::Equality;
             }
             BinaryOperator::Equality => {}
-            _ => return None,
+            _ => return,
         }
-        Some(if b {
+        *expr = if b {
             e.left.take_in(ctx.ast)
         } else {
             let argument = e.left.take_in(ctx.ast);
             ctx.ast.expression_unary(e.span, UnaryOperator::LogicalNot, argument)
-        })
+        };
+        ctx.state.changed = true;
     }
 
     /// Compress `foo == true` into `foo == 1`.
@@ -171,19 +152,19 @@ impl<'a> PeepholeOptimizations {
     ///
     /// In `IsLooselyEqual`, `true` and `false` are converted to `1` and `0` first.
     /// <https://tc39.es/ecma262/multipage/abstract-operations.html#sec-islooselyequal>
-    fn try_compress_is_loose_boolean(e: &mut BinaryExpression<'a>, ctx: &mut Ctx<'a, '_>) -> bool {
+    fn try_compress_is_loose_boolean(e: &mut BinaryExpression<'a>, ctx: &mut Ctx<'a, '_>) {
         if !matches!(e.operator, BinaryOperator::Equality | BinaryOperator::Inequality) {
-            return false;
+            return;
         }
-
         if let Some(ConstantValue::Boolean(left_bool)) = e.left.evaluate_value(ctx) {
             e.left = ctx.ast.expression_numeric_literal(
                 e.left.span(),
                 if left_bool { 1.0 } else { 0.0 },
                 None,
                 NumberBase::Decimal,
             );
-            return true;
+            ctx.state.changed = true;
+            return;
         }
         if let Some(ConstantValue::Boolean(right_bool)) = e.right.evaluate_value(ctx) {
             e.right = ctx.ast.expression_numeric_literal(
@@ -192,9 +173,8 @@ impl<'a> PeepholeOptimizations {
                 None,
                 NumberBase::Decimal,
             );
-            return true;
+            ctx.state.changed = true;
         }
-        false
     }
 
     /// Returns the identifier or the assignment target's identifier of the given expression.
@@ -222,15 +202,15 @@ impl<'a> PeepholeOptimizations {
         &self,
         expr: &mut AssignmentExpression<'a>,
         ctx: &mut Ctx<'a, '_>,
-    ) -> bool {
+    ) {
         if ctx.options().target < ESTarget::ES2020 {
-            return false;
+            return;
         }
         if !matches!(expr.operator, AssignmentOperator::Assign) {
-            return false;
+            return;
         }
 
-        let Expression::LogicalExpression(logical_expr) = &mut expr.right else { return false };
+        let Expression::LogicalExpression(logical_expr) = &mut expr.right else { return };
         // NOTE: if the right hand side is an anonymous function, applying this compression will
         // set the `name` property of that function.
         // Since codes relying on the fact that function's name is undefined should be rare,
@@ -241,64 +221,64 @@ impl<'a> PeepholeOptimizations {
             Expression::Identifier(read_id_ref),
         ) = (&expr.left, &logical_expr.left)
         else {
-            return false;
+            return;
         };
         // It should also early return when the reference might refer to a reference value created by a with statement
         // when the minifier supports with statements
         if write_id_ref.name != read_id_ref.name || ctx.is_global_reference(write_id_ref) {
-            return false;
+            return;
         }
 
         let new_op = logical_expr.operator.to_assignment_operator();
         expr.operator = new_op;
         expr.right = logical_expr.right.take_in(ctx.ast);
-        true
+        ctx.state.changed = true;
     }
 
     /// Compress `a = a + b` to `a += b`
     fn try_compress_normal_assignment_to_combined_assignment(
         expr: &mut AssignmentExpression<'a>,
         ctx: &mut Ctx<'a, '_>,
-    ) -> bool {
+    ) {
         if !matches!(expr.operator, AssignmentOperator::Assign) {
-            return false;
+            return;
         }
-
-        let Expression::BinaryExpression(binary_expr) = &mut expr.right else { return false };
-        let Some(new_op) = binary_expr.operator.to_assignment_operator() else { return false };
-
+        let Expression::BinaryExpression(binary_expr) = &mut expr.right else { return };
+        let Some(new_op) = binary_expr.operator.to_assignment_operator() else { return };
         if !Self::has_no_side_effect_for_evaluation_same_target(&expr.left, &binary_expr.left, ctx)
         {
-            return false;
+            return;
         }
-
         expr.operator = new_op;
         expr.right = binary_expr.right.take_in(ctx.ast);
-        true
+        ctx.state.changed = true;
     }
 
     /// Compress `a -= 1` to `--a` and `a -= -1` to `++a`
     #[expect(clippy::float_cmp)]
     fn try_compress_assignment_to_update_expression(
-        expr: &mut AssignmentExpression<'a>,
+        expr: &mut Expression<'a>,
         ctx: &mut Ctx<'a, '_>,
-    ) -> Option<Expression<'a>> {
-        if !matches!(expr.operator, AssignmentOperator::Subtraction) {
-            return None;
+    ) {
+        let Expression::AssignmentExpression(e) = expr else { return };
+        if !matches!(e.operator, AssignmentOperator::Subtraction) {
+            return;
         }
-        let Expression::NumericLiteral(num) = &expr.right else {
-            return None;
-        };
-        let target = expr.left.as_simple_assignment_target_mut()?;
-        let operator = if num.value == 1.0 {
-            UpdateOperator::Decrement
-        } else if num.value == -1.0 {
-            UpdateOperator::Increment
+        let operator = if let Expression::NumericLiteral(num) = &e.right {
+            if num.value == 1.0 {
+                UpdateOperator::Decrement
+            } else if num.value == -1.0 {
+                UpdateOperator::Increment
+            } else {
+                return;
+            }
         } else {
-            return None;
+            return;
         };
+        let Some(target) = e.left.as_simple_assignment_target_mut() else { return };
         let target = target.take_in(ctx.ast);
-        Some(ctx.ast.expression_update(expr.span, operator, true, target))
+        *expr = ctx.ast.expression_update(e.span, operator, true, target);
+        ctx.state.changed = true;
     }
 }
 

crates/oxc_minifier/src/peephole/minimize_expression_in_boolean_context.rs

@@ -33,7 +33,7 @@ impl<'a> PeepholeOptimizations {
         &self,
         expr: &mut Expression<'a>,
         ctx: &mut Ctx<'a, '_>,
-    ) -> bool {
+    ) {
         match expr {
             // "!!a" => "a"
             Expression::UnaryExpression(u1) if u1.operator.is_not() => {
@@ -42,7 +42,7 @@ impl<'a> PeepholeOptimizations {
                         let mut e = u2.argument.take_in(ctx.ast);
                         self.try_fold_expr_in_boolean_context(&mut e, ctx);
                         *expr = e;
-                        return true;
+                        ctx.state.changed = true;
                     }
                 }
             }
@@ -62,7 +62,7 @@ impl<'a> PeepholeOptimizations {
                     // `if ((a | b) === 0);", "if (!(a | b));")`
                     ctx.ast.expression_unary(e.span, UnaryOperator::LogicalNot, argument)
                 };
-                return true;
+                ctx.state.changed = true;
             }
             // "if (!!a && !!b)" => "if (a && b)"
             Expression::LogicalExpression(e) if e.operator.is_and() => {
@@ -71,7 +71,7 @@ impl<'a> PeepholeOptimizations {
                 // "if (anything && truthyNoSideEffects)" => "if (anything)"
                 if e.right.get_side_free_boolean_value(ctx) == Some(true) {
                     *expr = e.left.take_in(ctx.ast);
-                    return true;
+                    ctx.state.changed = true;
                 }
             }
             // "if (!!a ||!!b)" => "if (a || b)"
@@ -81,7 +81,7 @@ impl<'a> PeepholeOptimizations {
                 // "if (anything || falsyNoSideEffects)" => "if (anything)"
                 if e.right.get_side_free_boolean_value(ctx) == Some(false) {
                     *expr = e.left.take_in(ctx.ast);
-                    return true;
+                    ctx.state.changed = true;
                 }
             }
             Expression::ConditionalExpression(e) => {
@@ -100,7 +100,8 @@ impl<'a> PeepholeOptimizations {
                         (LogicalOperator::And, self.minimize_not(left.span(), left, ctx))
                     };
                     *expr = self.join_with_left_associative_op(span, op, left, right, ctx);
-                    return true;
+                    ctx.state.changed = true;
+                    return;
                 }
                 if let Some(boolean) = e.alternate.get_side_free_boolean_value(ctx) {
                     let left = e.test.take_in(ctx.ast);
@@ -114,17 +115,16 @@ impl<'a> PeepholeOptimizations {
                         (LogicalOperator::And, left)
                     };
                     *expr = self.join_with_left_associative_op(span, op, left, right, ctx);
-                    return true;
+                    ctx.state.changed = true;
                 }
             }
             Expression::SequenceExpression(seq_expr) => {
                 if let Some(last) = seq_expr.expressions.last_mut() {
-                    return self.try_fold_expr_in_boolean_context(last, ctx);
+                    self.try_fold_expr_in_boolean_context(last, ctx);
                 }
             }
             _ => {}
         }
-        false
     }
 }