From faf966f2f1a70c84143f51d24a648bf84d9481fb Mon Sep 17 00:00:00 2001 From: sapphi-red <49056869+sapphi-red@users.noreply.github.com> Date: Wed, 26 Feb 2025 08:16:31 +0000 Subject: [PATCH] refactor(ecmascript): don't check side effects in constant_evaluation (#9122) --- .../equality_comparison.rs | 8 +-- .../src/constant_evaluation/mod.rs | 70 ++++++------------- crates/oxc_minifier/src/ctx.rs | 7 +- .../src/peephole/fold_constants.rs | 26 +++++-- 4 files changed, 51 insertions(+), 60 deletions(-) diff --git a/crates/oxc_ecmascript/src/constant_evaluation/equality_comparison.rs b/crates/oxc_ecmascript/src/constant_evaluation/equality_comparison.rs index 51d98cba021ad..11d7e37b0af09 100644 --- a/crates/oxc_ecmascript/src/constant_evaluation/equality_comparison.rs +++ b/crates/oxc_ecmascript/src/constant_evaluation/equality_comparison.rs @@ -24,7 +24,7 @@ pub(super) fn abstract_equality_comparison<'a>( if matches!((left, right), (ValueType::Number, ValueType::String)) || matches!(right, ValueType::Boolean) { - if let Some(num) = right_expr.get_side_free_number_value(ctx) { + if let Some(num) = right_expr.evaluate_value_to_number(ctx) { let number_literal_expr = ctx.ast().expression_numeric_literal( oxc_span::SPAN, num, @@ -39,7 +39,7 @@ pub(super) fn abstract_equality_comparison<'a>( if matches!((left, right), (ValueType::String, ValueType::Number)) || matches!(left, ValueType::Boolean) { - if let Some(num) = left_expr.get_side_free_number_value(ctx) { + if let Some(num) = left_expr.evaluate_value_to_number(ctx) { let number_literal_expr = ctx.ast().expression_numeric_literal( oxc_span::SPAN, num, @@ -52,8 +52,8 @@ pub(super) fn abstract_equality_comparison<'a>( } if matches!(left, ValueType::BigInt) || matches!(right, ValueType::BigInt) { - let left_bigint = left_expr.get_side_free_bigint_value(ctx); - let right_bigint = right_expr.get_side_free_bigint_value(ctx); + let left_bigint = left_expr.evaluate_value_to_bigint(ctx); + let right_bigint = right_expr.evaluate_value_to_bigint(ctx); if let (Some(l_big), Some(r_big)) = (left_bigint, right_bigint) { return Some(l_big.eq(&r_big)); } diff --git a/crates/oxc_ecmascript/src/constant_evaluation/mod.rs b/crates/oxc_ecmascript/src/constant_evaluation/mod.rs index 44aa9b68bdd8a..f4e199accc060 100644 --- a/crates/oxc_ecmascript/src/constant_evaluation/mod.rs +++ b/crates/oxc_ecmascript/src/constant_evaluation/mod.rs @@ -188,9 +188,6 @@ fn binary_operation_evaluate_value_to<'a>( match operator { BinaryOperator::Addition => { use crate::to_primitive::ToPrimitive; - if left.may_have_side_effects(ctx) || right.may_have_side_effects(ctx) { - return None; - } let left_to_primitive = left.to_primitive(ctx); let right_to_primitive = right.to_primitive(ctx); if left_to_primitive.is_string() == Some(true) @@ -249,8 +246,8 @@ fn binary_operation_evaluate_value_to<'a>( BinaryOperator::ShiftLeft | BinaryOperator::ShiftRight | BinaryOperator::ShiftRightZeroFill => { - let left = left.get_side_free_number_value(ctx)?; - let right = right.get_side_free_number_value(ctx)?; + let left = left.evaluate_value_to_number(ctx)?; + let right = right.evaluate_value_to_number(ctx)?; let left = left.to_int_32(); let right = (right.to_int_32() as u32) & 31; Some(ConstantValue::Number(match operator { @@ -286,8 +283,8 @@ fn binary_operation_evaluate_value_to<'a>( } BinaryOperator::BitwiseAnd | BinaryOperator::BitwiseOR | BinaryOperator::BitwiseXOR => { if left.value_type(ctx).is_bigint() && right.value_type(ctx).is_bigint() { - let left_val = left.get_side_free_bigint_value(ctx)?; - let right_val = right.get_side_free_bigint_value(ctx)?; + let left_val = left.evaluate_value_to_bigint(ctx)?; + let right_val = right.evaluate_value_to_bigint(ctx)?; let result_val: BigInt = match operator { BinaryOperator::BitwiseAnd => left_val & right_val, BinaryOperator::BitwiseOR => left_val | right_val, @@ -296,8 +293,8 @@ fn binary_operation_evaluate_value_to<'a>( }; return Some(ConstantValue::BigInt(result_val)); } - let left_num = left.get_side_free_number_value(ctx); - let right_num = right.get_side_free_number_value(ctx); + let left_num = left.evaluate_value_to_number(ctx); + let right_num = right.evaluate_value_to_number(ctx); if let (Some(left_val), Some(right_val)) = (left_num, right_num) { let left_val_int = left_val.to_int_32(); let right_val_int = right_val.to_int_32(); @@ -313,9 +310,6 @@ fn binary_operation_evaluate_value_to<'a>( None } BinaryOperator::Instanceof => { - if left.may_have_side_effects(ctx) { - return None; - } if let Expression::Identifier(right_ident) = right { let name = right_ident.name.as_str(); if matches!(name, "Object" | "Number" | "Boolean" | "String") @@ -336,9 +330,6 @@ fn binary_operation_evaluate_value_to<'a>( | BinaryOperator::StrictInequality | BinaryOperator::Equality | BinaryOperator::Inequality => { - if left.may_have_side_effects(ctx) || right.may_have_side_effects(ctx) { - return None; - } let value = match operator { BinaryOperator::StrictEquality | BinaryOperator::StrictInequality => { strict_equality_comparison(ctx, left, right)? @@ -406,9 +397,6 @@ impl<'a> ConstantEvaluation<'a> for UnaryExpression<'a> { ) -> Option> { match self.operator { UnaryOperator::Typeof => { - if self.argument.may_have_side_effects(ctx) { - return None; - } let arg_ty = self.argument.value_type(ctx); let s = match arg_ty { ValueType::BigInt => "bigint", @@ -429,26 +417,22 @@ impl<'a> ConstantEvaluation<'a> for UnaryExpression<'a> { }; Some(ConstantValue::String(Cow::Borrowed(s))) } - UnaryOperator::Void => { - (!self.argument.may_have_side_effects(ctx)).then_some(ConstantValue::Undefined) + UnaryOperator::Void => Some(ConstantValue::Undefined), + UnaryOperator::LogicalNot => { + self.argument.evaluate_value_to_boolean(ctx).map(|b| !b).map(ConstantValue::Boolean) } - UnaryOperator::LogicalNot => self - .argument - .get_side_free_boolean_value(ctx) - .map(|b| !b) - .map(ConstantValue::Boolean), UnaryOperator::UnaryPlus => { - self.argument.get_side_free_number_value(ctx).map(ConstantValue::Number) + self.argument.evaluate_value_to_number(ctx).map(ConstantValue::Number) } UnaryOperator::UnaryNegation => match self.argument.value_type(ctx) { ValueType::BigInt => self .argument - .get_side_free_bigint_value(ctx) + .evaluate_value_to_bigint(ctx) .map(|v| -v) .map(ConstantValue::BigInt), ValueType::Number => self .argument - .get_side_free_number_value(ctx) + .evaluate_value_to_number(ctx) .map(|v| if v.is_nan() { v } else { -v }) .map(ConstantValue::Number), ValueType::Undefined => Some(ConstantValue::Number(f64::NAN)), @@ -458,13 +442,13 @@ impl<'a> ConstantEvaluation<'a> for UnaryExpression<'a> { UnaryOperator::BitwiseNot => match self.argument.value_type(ctx) { ValueType::BigInt => self .argument - .get_side_free_bigint_value(ctx) + .evaluate_value_to_bigint(ctx) .map(|v| !v) .map(ConstantValue::BigInt), #[expect(clippy::cast_lossless)] _ => self .argument - .get_side_free_number_value(ctx) + .evaluate_value_to_number(ctx) .map(|v| (!v.to_int_32()) as f64) .map(ConstantValue::Number), }, @@ -483,15 +467,10 @@ impl<'a> ConstantEvaluation<'a> for StaticMemberExpression<'a> { "length" => { if let Some(ConstantValue::String(s)) = self.object.evaluate_value(ctx) { Some(ConstantValue::Number(s.encode_utf16().count().to_f64().unwrap())) + } else if let Expression::ArrayExpression(arr) = &self.object { + Some(ConstantValue::Number(arr.elements.len().to_f64().unwrap())) } else { - if self.object.may_have_side_effects(ctx) { - return None; - } - if let Expression::ArrayExpression(arr) = &self.object { - Some(ConstantValue::Number(arr.elements.len().to_f64().unwrap())) - } else { - None - } + None } } _ => None, @@ -509,15 +488,10 @@ impl<'a> ConstantEvaluation<'a> for ComputedMemberExpression<'a> { Expression::StringLiteral(s) if s.value == "length" => { if let Some(ConstantValue::String(s)) = self.object.evaluate_value(ctx) { Some(ConstantValue::Number(s.encode_utf16().count().to_f64().unwrap())) + } else if let Expression::ArrayExpression(arr) = &self.object { + Some(ConstantValue::Number(arr.elements.len().to_f64().unwrap())) } else { - if self.object.may_have_side_effects(ctx) { - return None; - } - if let Expression::ArrayExpression(arr) = &self.object { - Some(ConstantValue::Number(arr.elements.len().to_f64().unwrap())) - } else { - None - } + None } } _ => None, @@ -531,10 +505,6 @@ fn is_less_than<'a>( x: &Expression<'a>, y: &Expression<'a>, ) -> Option> { - if x.may_have_side_effects(ctx) || y.may_have_side_effects(ctx) { - return None; - } - // a. Let px be ? ToPrimitive(x, NUMBER). // b. Let py be ? ToPrimitive(y, NUMBER). let px = x.value_type(ctx); diff --git a/crates/oxc_minifier/src/ctx.rs b/crates/oxc_minifier/src/ctx.rs index 755dc85fd4db6..b65c25a1cedf0 100644 --- a/crates/oxc_minifier/src/ctx.rs +++ b/crates/oxc_minifier/src/ctx.rs @@ -4,6 +4,7 @@ use oxc_ast::{AstBuilder, ast::*}; use oxc_ecmascript::constant_evaluation::{ ConstantEvaluation, ConstantEvaluationCtx, ConstantValue, binary_operation_evaluate_value, }; +use oxc_ecmascript::side_effects::MayHaveSideEffects; use oxc_semantic::{IsGlobalReference, SymbolTable}; use oxc_traverse::TraverseCtx; @@ -44,7 +45,11 @@ impl<'a> Ctx<'a, '_> { } pub fn eval_binary(self, e: &BinaryExpression<'a>) -> Option> { - e.evaluate_value(&self).map(|v| self.value_to_expr(e.span, v)) + if e.may_have_side_effects(&self) { + None + } else { + e.evaluate_value(&self).map(|v| self.value_to_expr(e.span, v)) + } } pub fn eval_binary_operation( diff --git a/crates/oxc_minifier/src/peephole/fold_constants.rs b/crates/oxc_minifier/src/peephole/fold_constants.rs index 0a20488f28314..e5769375e1988 100644 --- a/crates/oxc_minifier/src/peephole/fold_constants.rs +++ b/crates/oxc_minifier/src/peephole/fold_constants.rs @@ -49,7 +49,13 @@ impl<'a> PeepholeOptimizations { } // Do not fold big int. UnaryOperator::UnaryNegation if e.argument.is_big_int_literal() => None, - _ => e.evaluate_value(&ctx).map(|v| ctx.value_to_expr(e.span, v)), + _ => { + if e.may_have_side_effects(&ctx) { + None + } else { + e.evaluate_value(&ctx).map(|v| ctx.value_to_expr(e.span, v)) + } + } } } @@ -58,7 +64,11 @@ impl<'a> PeepholeOptimizations { ctx: Ctx<'a, '_>, ) -> Option> { // TODO: tryFoldObjectPropAccess(n, left, name) - e.evaluate_value(&ctx).map(|value| ctx.value_to_expr(e.span, value)) + if e.object.may_have_side_effects(&ctx) { + None + } else { + e.evaluate_value(&ctx).map(|value| ctx.value_to_expr(e.span, value)) + } } fn try_fold_computed_member_expr( @@ -66,7 +76,11 @@ impl<'a> PeepholeOptimizations { ctx: Ctx<'a, '_>, ) -> Option> { // TODO: tryFoldObjectPropAccess(n, left, name) - e.evaluate_value(&ctx).map(|value| ctx.value_to_expr(e.span, value)) + if e.object.may_have_side_effects(&ctx) || e.expression.may_have_side_effects(&ctx) { + None + } else { + e.evaluate_value(&ctx).map(|value| ctx.value_to_expr(e.span, value)) + } } fn try_fold_logical_expr( @@ -304,8 +318,10 @@ impl<'a> PeepholeOptimizations { // Simplified version of `tryFoldAdd` from closure compiler. fn try_fold_add(e: &mut BinaryExpression<'a>, ctx: Ctx<'a, '_>) -> Option> { - if let Some(v) = e.evaluate_value(&ctx) { - return Some(ctx.value_to_expr(e.span, v)); + if !e.may_have_side_effects(&ctx) { + if let Some(v) = e.evaluate_value(&ctx) { + return Some(ctx.value_to_expr(e.span, v)); + } } debug_assert_eq!(e.operator, BinaryOperator::Addition);