Skip to content

Commit

Permalink
refactor(ecmascript): don't check side effects in constant_evaluation (…
Browse files Browse the repository at this point in the history 
…oxc-project#9122)
  • Loading branch information
@sapphi-red
sapphi-red committed Feb 26, 2025
1 parent e10fb97 commit faf966f
Show file tree
Hide file tree
Showing 4 changed files with 51 additions and 60 deletions.
8 changes: 4 additions & 4 deletions crates/oxc_ecmascript/src/constant_evaluation/equality_comparison.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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));
}
Expand Down
70 changes: 20 additions & 50 deletions crates/oxc_ecmascript/src/constant_evaluation/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand All @@ -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();
Expand All @@ -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")
Expand All @@ -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)?
Expand Down Expand Up @@ -406,9 +397,6 @@ impl<'a> ConstantEvaluation<'a> for UnaryExpression<'a> {
) -> Option<ConstantValue<'a>> {
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",
Expand All @@ -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)),
Expand All @@ -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),
},
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -531,10 +505,6 @@ fn is_less_than<'a>(
x: &Expression<'a>,
y: &Expression<'a>,
) -> Option<ConstantValue<'a>> {
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);
Expand Down
7 changes: 6 additions & 1 deletion crates/oxc_minifier/src/ctx.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down Expand Up @@ -44,7 +45,11 @@ impl<'a> Ctx<'a, '_> {
}

pub fn eval_binary(self, e: &BinaryExpression<'a>) -> Option<Expression<'a>> {
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(
Expand Down
26 changes: 21 additions & 5 deletions crates/oxc_minifier/src/peephole/fold_constants.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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))
}
}
}
}

Expand All @@ -58,15 +64,23 @@ impl<'a> PeepholeOptimizations {
ctx: Ctx<'a, '_>,
) -> Option<Expression<'a>> {
// 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(
e: &mut ComputedMemberExpression<'a>,
ctx: Ctx<'a, '_>,
) -> Option<Expression<'a>> {
// 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(
Expand Down Expand Up @@ -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<Expression<'a>> {
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);

Expand Down

0 comments on commit faf966f

Please sign in to comment.