From 0af0267077bf1e50182333a0aa43aff5fdd02cf3 Mon Sep 17 00:00:00 2001 From: Boshen <1430279+Boshen@users.noreply.github.com> Date: Sat, 25 Jan 2025 15:00:59 +0000 Subject: [PATCH] refactor(minifier): side effect detection needs symbols resolution (#8715) --- .../src/constant_evaluation/mod.rs | 31 +- .../side_effects/check_for_state_change.rs | 348 ------------------ .../src/side_effects/may_have_side_effects.rs | 122 +++++- crates/oxc_ecmascript/src/side_effects/mod.rs | 2 - crates/oxc_minifier/examples/minifier.rs | 2 +- crates/oxc_minifier/src/ctx.rs | 11 +- .../src/peephole/fold_constants.rs | 10 +- .../src/peephole/minimize_conditions.rs | 1 + crates/oxc_minifier/src/peephole/normalize.rs | 2 +- .../src/peephole/remove_dead_code.rs | 8 +- .../src/peephole/replace_known_methods.rs | 4 +- .../src/peephole/statement_fusion.rs | 8 +- .../peephole/substitute_alternate_syntax.rs | 10 +- tasks/minsize/minsize.snap | 6 +- 14 files changed, 156 insertions(+), 409 deletions(-) delete mode 100644 crates/oxc_ecmascript/src/side_effects/check_for_state_change.rs diff --git a/crates/oxc_ecmascript/src/constant_evaluation/mod.rs b/crates/oxc_ecmascript/src/constant_evaluation/mod.rs index 7f69447e1f863..6f72a9ca3d5f6 100644 --- a/crates/oxc_ecmascript/src/constant_evaluation/mod.rs +++ b/crates/oxc_ecmascript/src/constant_evaluation/mod.rs @@ -14,11 +14,7 @@ pub use is_literal_value::IsLiteralValue; pub use value::ConstantValue; pub use value_type::ValueType; -pub trait ConstantEvaluation<'a> { - fn is_global_reference(&self, ident: &IdentifierReference<'a>) -> bool { - matches!(ident.name.as_str(), "undefined" | "NaN" | "Infinity") - } - +pub trait ConstantEvaluation<'a>: MayHaveSideEffects<'a> { fn resolve_binding(&self, ident: &IdentifierReference<'a>) -> Option> { match ident.name.as_str() { "undefined" if self.is_global_reference(ident) => Some(ConstantValue::Undefined), @@ -36,7 +32,7 @@ pub trait ConstantEvaluation<'a> { // and there are only a very few cases where we can compute a number value, but there could // also be side effects. e.g. `void doSomething()` has value NaN, regardless of the behavior // of `doSomething()` - if value.is_some() && expr.may_have_side_effects() { + if value.is_some() && self.expression_may_have_side_efffects(expr) { None } else { value @@ -45,7 +41,7 @@ pub trait ConstantEvaluation<'a> { fn get_side_free_string_value(&self, expr: &Expression<'a>) -> Option> { let value = expr.to_js_string(); - if value.is_some() && !expr.may_have_side_effects() { + if value.is_some() && !self.expression_may_have_side_efffects(expr) { return value; } None @@ -53,7 +49,7 @@ pub trait ConstantEvaluation<'a> { fn get_side_free_boolean_value(&self, expr: &Expression<'a>) -> Option { let value = self.get_boolean_value(expr); - if value.is_some() && !expr.may_have_side_effects() { + if value.is_some() && !self.expression_may_have_side_efffects(expr) { return value; } None @@ -61,7 +57,7 @@ pub trait ConstantEvaluation<'a> { fn get_side_free_bigint_value(&self, expr: &Expression<'a>) -> Option { let value = expr.to_big_int(); - if value.is_some() && expr.may_have_side_effects() { + if value.is_some() && self.expression_may_have_side_efffects(expr) { None } else { value @@ -205,7 +201,9 @@ pub trait ConstantEvaluation<'a> { ) -> Option> { match operator { BinaryOperator::Addition => { - if left.may_have_side_effects() || right.may_have_side_effects() { + if self.expression_may_have_side_efffects(left) + || self.expression_may_have_side_efffects(right) + { return None; } let left_type = ValueType::from(left); @@ -321,7 +319,7 @@ pub trait ConstantEvaluation<'a> { None } BinaryOperator::Instanceof => { - if left.may_have_side_effects() { + if self.expression_may_have_side_efffects(left) { return None; } if let Expression::Identifier(right_ident) = right { @@ -380,8 +378,9 @@ pub trait ConstantEvaluation<'a> { }; Some(ConstantValue::String(Cow::Borrowed(s))) } - UnaryOperator::Void => (expr.argument.is_literal() || !expr.may_have_side_effects()) - .then_some(ConstantValue::Undefined), + UnaryOperator::Void => (expr.argument.is_literal() + || !self.expression_may_have_side_efffects(&expr.argument)) + .then_some(ConstantValue::Undefined), UnaryOperator::LogicalNot => self .get_side_free_boolean_value(&expr.argument) .map(|b| !b) @@ -424,10 +423,9 @@ pub trait ConstantEvaluation<'a> { if let Some(ConstantValue::String(s)) = self.eval_expression(&expr.object) { Some(ConstantValue::Number(s.encode_utf16().count().to_f64().unwrap())) } else { - if expr.object.may_have_side_effects() { + if self.expression_may_have_side_efffects(&expr.object) { return None; } - if let Expression::ArrayExpression(arr) = &expr.object { Some(ConstantValue::Number(arr.elements.len().to_f64().unwrap())) } else { @@ -448,10 +446,9 @@ pub trait ConstantEvaluation<'a> { if let Some(ConstantValue::String(s)) = self.eval_expression(&expr.object) { Some(ConstantValue::Number(s.encode_utf16().count().to_f64().unwrap())) } else { - if expr.object.may_have_side_effects() { + if self.expression_may_have_side_efffects(&expr.object) { return None; } - if let Expression::ArrayExpression(arr) = &expr.object { Some(ConstantValue::Number(arr.elements.len().to_f64().unwrap())) } else { diff --git a/crates/oxc_ecmascript/src/side_effects/check_for_state_change.rs b/crates/oxc_ecmascript/src/side_effects/check_for_state_change.rs deleted file mode 100644 index 94dd82117aac4..0000000000000 --- a/crates/oxc_ecmascript/src/side_effects/check_for_state_change.rs +++ /dev/null @@ -1,348 +0,0 @@ -use oxc_ast::ast::*; -use oxc_syntax::operator::{BinaryOperator, UnaryOperator}; - -/// A "simple" operator is one whose children are expressions, has no direct side-effects. -fn is_simple_unary_operator(operator: UnaryOperator) -> bool { - operator != UnaryOperator::Delete -} - -/// Returns true if some node in n's subtree changes application state. If -/// `check_for_new_objects` is true, we assume that newly created mutable objects (like object -/// literals) change state. Otherwise, we assume that they have no side effects. -/// -/// Ported from [closure-compiler](https://github.com/google/closure-compiler/blob/f3ce5ed8b630428e311fe9aa2e20d36560d975e2/src/com/google/javascript/jscomp/AstAnalyzer.java#L241) -pub trait CheckForStateChange<'a, 'b> { - fn check_for_state_change(&self, check_for_new_objects: bool) -> bool; -} - -impl<'a> CheckForStateChange<'a, '_> for Expression<'a> { - fn check_for_state_change(&self, check_for_new_objects: bool) -> bool { - match self { - Self::NumericLiteral(_) - | Self::BooleanLiteral(_) - | Self::StringLiteral(_) - | Self::BigIntLiteral(_) - | Self::NullLiteral(_) - | Self::RegExpLiteral(_) - | Self::MetaProperty(_) - | Self::ThisExpression(_) - | Self::ArrowFunctionExpression(_) - | Self::FunctionExpression(_) => false, - Self::ClassExpression(class_expr) => class_expr - .body - .body - .iter() - .any(|method| method.check_for_state_change(check_for_new_objects)), - Self::TemplateLiteral(template) => template - .expressions - .iter() - .any(|expr| expr.check_for_state_change(check_for_new_objects)), - Self::Identifier(ident) => match ident.name.as_str() { - "undefined" | "NaN" | "Infinity" => false, - // Reference read can have a side effect. - _ => true, - }, - Self::UnaryExpression(unary_expr) => { - unary_expr.check_for_state_change(check_for_new_objects) - } - Self::ParenthesizedExpression(p) => { - p.expression.check_for_state_change(check_for_new_objects) - } - Self::ConditionalExpression(p) => { - p.test.check_for_state_change(check_for_new_objects) - || p.consequent.check_for_state_change(check_for_new_objects) - || p.alternate.check_for_state_change(check_for_new_objects) - } - Self::SequenceExpression(s) => { - s.expressions.iter().any(|expr| expr.check_for_state_change(check_for_new_objects)) - } - Self::BinaryExpression(binary_expr) => { - binary_expr.check_for_state_change(check_for_new_objects) - } - Self::ObjectExpression(object_expr) => { - if check_for_new_objects { - return true; - } - object_expr - .properties - .iter() - .any(|property| property.check_for_state_change(check_for_new_objects)) - } - Self::ArrayExpression(array_expr) => { - if check_for_new_objects { - return true; - } - array_expr - .elements - .iter() - .any(|element| element.check_for_state_change(check_for_new_objects)) - } - - _ => true, - } - } -} - -impl<'a> CheckForStateChange<'a, '_> for UnaryExpression<'a> { - fn check_for_state_change(&self, check_for_new_objects: bool) -> bool { - if is_simple_unary_operator(self.operator) { - return self.argument.check_for_state_change(check_for_new_objects); - } - true - } -} - -impl<'a> CheckForStateChange<'a, '_> for BinaryExpression<'a> { - fn check_for_state_change(&self, check_for_new_objects: bool) -> bool { - // `instanceof` and `in` can throw `TypeError` - if matches!(self.operator, BinaryOperator::In | BinaryOperator::Instanceof) { - return true; - } - let left = self.left.check_for_state_change(check_for_new_objects); - if left { - return true; - } - self.right.check_for_state_change(check_for_new_objects) - } -} - -impl<'a> CheckForStateChange<'a, '_> for ArrayExpressionElement<'a> { - fn check_for_state_change(&self, check_for_new_objects: bool) -> bool { - match self { - Self::SpreadElement(element) => element.check_for_state_change(check_for_new_objects), - match_expression!(Self) => { - self.to_expression().check_for_state_change(check_for_new_objects) - } - Self::Elision(_) => false, - } - } -} - -impl<'a> CheckForStateChange<'a, '_> for ObjectPropertyKind<'a> { - fn check_for_state_change(&self, check_for_new_objects: bool) -> bool { - match self { - Self::ObjectProperty(method) => method.check_for_state_change(check_for_new_objects), - Self::SpreadProperty(spread_element) => { - spread_element.check_for_state_change(check_for_new_objects) - } - } - } -} - -impl<'a> CheckForStateChange<'a, '_> for SpreadElement<'a> { - fn check_for_state_change(&self, _check_for_new_objects: bool) -> bool { - // Object-rest and object-spread may trigger a getter. - // TODO: Closure Compiler assumes that getters may side-free when set `assumeGettersArePure`. - // https://github.com/google/closure-compiler/blob/a4c880032fba961f7a6c06ef99daa3641810bfdd/src/com/google/javascript/jscomp/AstAnalyzer.java#L282 - true - } -} - -impl<'a> CheckForStateChange<'a, '_> for ObjectProperty<'a> { - fn check_for_state_change(&self, check_for_new_objects: bool) -> bool { - self.key.check_for_state_change(check_for_new_objects) - || self.value.check_for_state_change(check_for_new_objects) - } -} - -impl<'a> CheckForStateChange<'a, '_> for PropertyKey<'a> { - fn check_for_state_change(&self, check_for_new_objects: bool) -> bool { - match self { - Self::StaticIdentifier(_) | Self::PrivateIdentifier(_) => false, - match_expression!(Self) => { - self.to_expression().check_for_state_change(check_for_new_objects) - } - } - } -} - -impl<'a> CheckForStateChange<'a, '_> for ForStatementLeft<'a> { - fn check_for_state_change(&self, check_for_new_objects: bool) -> bool { - match self { - match_assignment_target!(Self) => { - self.to_assignment_target().check_for_state_change(check_for_new_objects) - } - ForStatementLeft::VariableDeclaration(variable_declaration) => { - variable_declaration.check_for_state_change(check_for_new_objects) - } - } - } -} - -impl<'a> CheckForStateChange<'a, '_> for VariableDeclaration<'a> { - fn check_for_state_change(&self, check_for_new_objects: bool) -> bool { - self.declarations.iter().any(|decl| decl.check_for_state_change(check_for_new_objects)) - } -} - -impl<'a> CheckForStateChange<'a, '_> for VariableDeclarator<'a> { - fn check_for_state_change(&self, check_for_new_objects: bool) -> bool { - self.id.check_for_state_change(check_for_new_objects) - || self - .init - .as_ref() - .is_some_and(|init| init.check_for_state_change(check_for_new_objects)) - } -} - -impl CheckForStateChange<'_, '_> for BindingPattern<'_> { - fn check_for_state_change(&self, check_for_new_objects: bool) -> bool { - match &self.kind { - BindingPatternKind::BindingIdentifier(_) => false, - BindingPatternKind::ObjectPattern(object_pattern) => { - object_pattern - .properties - .iter() - .any(|element| element.check_for_state_change(check_for_new_objects)) - || object_pattern - .rest - .as_ref() - .is_some_and(|rest| rest.check_for_state_change(check_for_new_objects)) - } - BindingPatternKind::ArrayPattern(array_pattern) => { - array_pattern.elements.iter().any(|element| { - element.as_ref().is_some_and(|element| { - element.check_for_state_change(check_for_new_objects) - }) - }) || array_pattern - .rest - .as_ref() - .is_some_and(|rest| rest.check_for_state_change(check_for_new_objects)) - } - BindingPatternKind::AssignmentPattern(assignment_pattern) => { - assignment_pattern.left.check_for_state_change(check_for_new_objects) - && assignment_pattern.right.check_for_state_change(check_for_new_objects) - } - } - } -} - -impl CheckForStateChange<'_, '_> for BindingRestElement<'_> { - fn check_for_state_change(&self, check_for_new_objects: bool) -> bool { - self.argument.check_for_state_change(check_for_new_objects) - } -} - -impl CheckForStateChange<'_, '_> for BindingProperty<'_> { - fn check_for_state_change(&self, check_for_new_objects: bool) -> bool { - self.key.check_for_state_change(check_for_new_objects) - || self.value.check_for_state_change(check_for_new_objects) - } -} - -impl CheckForStateChange<'_, '_> for AssignmentTarget<'_> { - fn check_for_state_change(&self, check_for_new_objects: bool) -> bool { - match self { - AssignmentTarget::AssignmentTargetIdentifier(_) => false, - AssignmentTarget::TSAsExpression(ts_as_expression) => { - ts_as_expression.expression.check_for_state_change(check_for_new_objects) - } - AssignmentTarget::TSSatisfiesExpression(ts_satisfies_expression) => { - ts_satisfies_expression.expression.check_for_state_change(check_for_new_objects) - } - AssignmentTarget::TSNonNullExpression(ts_non_null_expression) => { - ts_non_null_expression.expression.check_for_state_change(check_for_new_objects) - } - AssignmentTarget::TSTypeAssertion(ts_type_assertion) => { - ts_type_assertion.expression.check_for_state_change(check_for_new_objects) - } - AssignmentTarget::TSInstantiationExpression(ts_instantiation_expression) => { - ts_instantiation_expression.expression.check_for_state_change(check_for_new_objects) - } - AssignmentTarget::ComputedMemberExpression(computed_member_expression) => { - computed_member_expression.object.check_for_state_change(check_for_new_objects) - || computed_member_expression - .expression - .check_for_state_change(check_for_new_objects) - } - AssignmentTarget::StaticMemberExpression(static_member_expression) => { - static_member_expression.object.check_for_state_change(check_for_new_objects) - } - AssignmentTarget::PrivateFieldExpression(private_field_expression) => { - private_field_expression.object.check_for_state_change(check_for_new_objects) - } - AssignmentTarget::ArrayAssignmentTarget(array_assignment_target) => { - array_assignment_target.elements.iter().any(|element| { - element.as_ref().is_some_and(|element| { - element.check_for_state_change(check_for_new_objects) - }) - }) || array_assignment_target - .rest - .as_ref() - .is_some_and(|rest| rest.check_for_state_change(check_for_new_objects)) - } - AssignmentTarget::ObjectAssignmentTarget(object_assignment_target) => { - object_assignment_target - .properties - .iter() - .any(|property| property.check_for_state_change(check_for_new_objects)) - || object_assignment_target - .rest - .as_ref() - .is_some_and(|rest| rest.check_for_state_change(check_for_new_objects)) - } - } - } -} - -impl CheckForStateChange<'_, '_> for AssignmentTargetProperty<'_> { - fn check_for_state_change(&self, check_for_new_objects: bool) -> bool { - match self { - AssignmentTargetProperty::AssignmentTargetPropertyIdentifier( - assignment_target_property_identifier, - ) => assignment_target_property_identifier - .init - .as_ref() - .is_some_and(|init| init.check_for_state_change(check_for_new_objects)), - AssignmentTargetProperty::AssignmentTargetPropertyProperty( - assignment_target_property_property, - ) => { - assignment_target_property_property - .name - .check_for_state_change(check_for_new_objects) - || assignment_target_property_property - .binding - .check_for_state_change(check_for_new_objects) - } - } - } -} - -impl CheckForStateChange<'_, '_> for AssignmentTargetRest<'_> { - fn check_for_state_change(&self, check_for_new_objects: bool) -> bool { - self.target.check_for_state_change(check_for_new_objects) - } -} - -impl CheckForStateChange<'_, '_> for AssignmentTargetMaybeDefault<'_> { - fn check_for_state_change(&self, check_for_new_objects: bool) -> bool { - match self { - match_assignment_target!(Self) => { - self.to_assignment_target().check_for_state_change(check_for_new_objects) - } - Self::AssignmentTargetWithDefault(assignment_target_with_default) => { - assignment_target_with_default.binding.check_for_state_change(check_for_new_objects) - && assignment_target_with_default - .init - .check_for_state_change(check_for_new_objects) - } - } - } -} - -impl<'a> CheckForStateChange<'a, '_> for ClassElement<'a> { - fn check_for_state_change(&self, check_for_new_objects: bool) -> bool { - match self { - ClassElement::TSIndexSignature(_) | ClassElement::StaticBlock(_) => false, - ClassElement::MethodDefinition(method_definition) => { - method_definition.key.check_for_state_change(check_for_new_objects) - } - ClassElement::PropertyDefinition(property_definition) => { - property_definition.key.check_for_state_change(check_for_new_objects) - } - ClassElement::AccessorProperty(accessor_property) => { - accessor_property.key.check_for_state_change(check_for_new_objects) - } - } - } -} diff --git a/crates/oxc_ecmascript/src/side_effects/may_have_side_effects.rs b/crates/oxc_ecmascript/src/side_effects/may_have_side_effects.rs index d0faa02a94a58..139e266bc232d 100644 --- a/crates/oxc_ecmascript/src/side_effects/may_have_side_effects.rs +++ b/crates/oxc_ecmascript/src/side_effects/may_have_side_effects.rs @@ -1,22 +1,110 @@ -use oxc_ast::ast::{Expression, ForStatementLeft, PropertyKey, UnaryExpression}; +use oxc_ast::ast::*; -use super::check_for_state_change::CheckForStateChange; - -/// Returns true if the node which may have side effects when executed. -/// This version default to the "safe" assumptions when the compiler object -/// is not provided (RegExp have side-effects, etc). +/// Returns true if subtree changes application state. /// /// Ported from [closure-compiler](https://github.com/google/closure-compiler/blob/f3ce5ed8b630428e311fe9aa2e20d36560d975e2/src/com/google/javascript/jscomp/AstAnalyzer.java#L94) -pub trait MayHaveSideEffects<'a, 'b> -where - Self: CheckForStateChange<'a, 'b>, -{ - fn may_have_side_effects(&self) -> bool { - self.check_for_state_change(/* check_for_new_objects */ false) +pub trait MayHaveSideEffects<'a> { + fn is_global_reference(&self, ident: &oxc_ast::ast::IdentifierReference<'a>) -> bool { + matches!(ident.name.as_str(), "undefined" | "NaN" | "Infinity") + } + + fn expression_may_have_side_efffects(&self, e: &Expression<'a>) -> bool { + match e { + // Reference read can have a side effect. + Expression::Identifier(ident) => self.is_global_reference(ident), + Expression::NumericLiteral(_) + | Expression::BooleanLiteral(_) + | Expression::StringLiteral(_) + | Expression::BigIntLiteral(_) + | Expression::NullLiteral(_) + | Expression::RegExpLiteral(_) + | Expression::MetaProperty(_) + | Expression::ThisExpression(_) + | Expression::ArrowFunctionExpression(_) + | Expression::FunctionExpression(_) => false, + Expression::TemplateLiteral(template) => { + template.expressions.iter().any(|e| self.expression_may_have_side_efffects(e)) + } + Expression::UnaryExpression(e) => self.unary_expression_may_have_side_effects(e), + Expression::ParenthesizedExpression(e) => { + self.expression_may_have_side_efffects(&e.expression) + } + Expression::ConditionalExpression(e) => { + self.expression_may_have_side_efffects(&e.test) + || self.expression_may_have_side_efffects(&e.consequent) + || self.expression_may_have_side_efffects(&e.alternate) + } + Expression::SequenceExpression(e) => { + e.expressions.iter().any(|e| self.expression_may_have_side_efffects(e)) + } + Expression::BinaryExpression(e) => self.binary_expression_may_have_side_effects(e), + Expression::ObjectExpression(object_expr) => object_expr + .properties + .iter() + .any(|property| self.object_property_kind_may_have_side_effects(property)), + Expression::ArrayExpression(e) => e + .elements + .iter() + .any(|element| self.array_expression_element_may_have_side_effects(element)), + _ => true, + } + } + + fn unary_expression_may_have_side_effects(&self, e: &UnaryExpression<'a>) -> bool { + /// A "simple" operator is one whose children are expressions, has no direct side-effects. + fn is_simple_unary_operator(operator: UnaryOperator) -> bool { + operator != UnaryOperator::Delete + } + if is_simple_unary_operator(e.operator) { + return self.expression_may_have_side_efffects(&e.argument); + } + true + } + + fn binary_expression_may_have_side_effects(&self, e: &BinaryExpression<'a>) -> bool { + // `instanceof` and `in` can throw `TypeError` + if matches!(e.operator, BinaryOperator::In | BinaryOperator::Instanceof) { + return true; + } + self.expression_may_have_side_efffects(&e.left) + || self.expression_may_have_side_efffects(&e.right) } -} -impl<'a> MayHaveSideEffects<'a, '_> for Expression<'a> {} -impl<'a> MayHaveSideEffects<'a, '_> for UnaryExpression<'a> {} -impl<'a> MayHaveSideEffects<'a, '_> for ForStatementLeft<'a> {} -impl<'a> MayHaveSideEffects<'a, '_> for PropertyKey<'a> {} + fn array_expression_element_may_have_side_effects( + &self, + e: &ArrayExpressionElement<'a>, + ) -> bool { + match e { + ArrayExpressionElement::SpreadElement(e) => { + self.expression_may_have_side_efffects(&e.argument) + } + match_expression!(ArrayExpressionElement) => { + self.expression_may_have_side_efffects(e.to_expression()) + } + ArrayExpressionElement::Elision(_) => false, + } + } + + fn object_property_kind_may_have_side_effects(&self, e: &ObjectPropertyKind<'a>) -> bool { + match e { + ObjectPropertyKind::ObjectProperty(o) => self.object_property_may_have_side_effects(o), + ObjectPropertyKind::SpreadProperty(e) => { + self.expression_may_have_side_efffects(&e.argument) + } + } + } + + fn object_property_may_have_side_effects(&self, e: &ObjectProperty<'a>) -> bool { + self.property_key_may_have_side_effects(&e.key) + || self.expression_may_have_side_efffects(&e.value) + } + + fn property_key_may_have_side_effects(&self, key: &PropertyKey<'a>) -> bool { + match key { + PropertyKey::StaticIdentifier(_) | PropertyKey::PrivateIdentifier(_) => false, + match_expression!(PropertyKey) => { + self.expression_may_have_side_efffects(key.to_expression()) + } + } + } +} diff --git a/crates/oxc_ecmascript/src/side_effects/mod.rs b/crates/oxc_ecmascript/src/side_effects/mod.rs index 2993d15e2bdb3..2304589e9ce26 100644 --- a/crates/oxc_ecmascript/src/side_effects/mod.rs +++ b/crates/oxc_ecmascript/src/side_effects/mod.rs @@ -1,5 +1,3 @@ -mod check_for_state_change; mod may_have_side_effects; -pub use check_for_state_change::CheckForStateChange; pub use may_have_side_effects::MayHaveSideEffects; diff --git a/crates/oxc_minifier/examples/minifier.rs b/crates/oxc_minifier/examples/minifier.rs index 739819eafaade..4d02d16c93155 100644 --- a/crates/oxc_minifier/examples/minifier.rs +++ b/crates/oxc_minifier/examples/minifier.rs @@ -27,7 +27,7 @@ fn main() -> std::io::Result<()> { let mut allocator = Allocator::default(); let printed = minify(&allocator, &source_text, source_type, mangle, nospace); - // println!("{printed}"); + println!("{printed}"); if twice { allocator.reset(); diff --git a/crates/oxc_minifier/src/ctx.rs b/crates/oxc_minifier/src/ctx.rs index 18dcf0f17f3ce..e58e766a5a6bd 100644 --- a/crates/oxc_minifier/src/ctx.rs +++ b/crates/oxc_minifier/src/ctx.rs @@ -1,7 +1,10 @@ use std::ops::Deref; use oxc_ast::ast::*; -use oxc_ecmascript::constant_evaluation::{ConstantEvaluation, ConstantValue}; +use oxc_ecmascript::{ + constant_evaluation::{ConstantEvaluation, ConstantValue}, + side_effects::MayHaveSideEffects, +}; use oxc_semantic::{IsGlobalReference, SymbolTable}; use oxc_traverse::TraverseCtx; @@ -16,8 +19,10 @@ impl<'a, 'b> Deref for Ctx<'a, 'b> { } } -impl<'a> ConstantEvaluation<'a> for Ctx<'a, '_> { - fn is_global_reference(&self, ident: &oxc_ast::ast::IdentifierReference<'a>) -> bool { +impl<'a> ConstantEvaluation<'a> for Ctx<'a, '_> {} + +impl<'a> MayHaveSideEffects<'a> for Ctx<'a, '_> { + fn is_global_reference(&self, ident: &IdentifierReference<'a>) -> bool { ident.is_global_reference(self.0.symbols()) } } diff --git a/crates/oxc_minifier/src/peephole/fold_constants.rs b/crates/oxc_minifier/src/peephole/fold_constants.rs index 9145db9a97fff..e2685dae34062 100644 --- a/crates/oxc_minifier/src/peephole/fold_constants.rs +++ b/crates/oxc_minifier/src/peephole/fold_constants.rs @@ -125,7 +125,7 @@ impl<'a, 'b> PeepholeOptimizations { // (FALSE && x) => FALSE if if lval { op == LogicalOperator::Or } else { op == LogicalOperator::And } { return Some(ctx.ast.move_expression(&mut logical_expr.left)); - } else if !left.may_have_side_effects() { + } else if !ctx.expression_may_have_side_efffects(left) { let parent = ctx.ancestry.parent(); // Bail `let o = { f() { assert.ok(this !== o); } }; (true && o.f)(); (true && o.f)``;` if parent.is_tagged_template_expression() @@ -150,7 +150,7 @@ impl<'a, 'b> PeepholeOptimizations { let left_child_right_boolean = ctx.get_boolean_value(&left_child.right); let left_child_op = left_child.operator; if let Some(right_boolean) = left_child_right_boolean { - if !left_child.right.may_have_side_effects() { + if !ctx.expression_may_have_side_efffects(&left_child.right) { // a || false || b => a || b // a && true && b => a && b if !right_boolean && left_child_op == LogicalOperator::Or @@ -183,7 +183,7 @@ impl<'a, 'b> PeepholeOptimizations { let left_val = ValueType::from(left); match left_val { ValueType::Null | ValueType::Undefined => { - Some(if left.may_have_side_effects() { + Some(if ctx.expression_may_have_side_efffects(left) { // e.g. `(a(), null) ?? 1` => `(a(), null, 1)` let expressions = ctx.ast.vec_from_array([ ctx.ast.move_expression(&mut logical_expr.left), @@ -386,7 +386,9 @@ impl<'a, 'b> PeepholeOptimizations { let left = &e.left; let right = &e.right; let op = e.operator; - if left.may_have_side_effects() || right.may_have_side_effects() { + if ctx.expression_may_have_side_efffects(left) + || ctx.expression_may_have_side_efffects(right) + { return None; } let value = match op { diff --git a/crates/oxc_minifier/src/peephole/minimize_conditions.rs b/crates/oxc_minifier/src/peephole/minimize_conditions.rs index 35063eaddb44e..4bb52485245e8 100644 --- a/crates/oxc_minifier/src/peephole/minimize_conditions.rs +++ b/crates/oxc_minifier/src/peephole/minimize_conditions.rs @@ -2,6 +2,7 @@ use oxc_allocator::Vec; use oxc_ast::{ast::*, NONE}; use oxc_ecmascript::{ constant_evaluation::{ConstantEvaluation, ValueType}, + side_effects::MayHaveSideEffects, ToInt32, }; use oxc_span::{cmp::ContentEq, GetSpan}; diff --git a/crates/oxc_minifier/src/peephole/normalize.rs b/crates/oxc_minifier/src/peephole/normalize.rs index 375a0320f47f7..ddc1a203ad206 100644 --- a/crates/oxc_minifier/src/peephole/normalize.rs +++ b/crates/oxc_minifier/src/peephole/normalize.rs @@ -1,6 +1,6 @@ use oxc_allocator::Vec; use oxc_ast::ast::*; -use oxc_ecmascript::constant_evaluation::ConstantEvaluation; +use oxc_ecmascript::side_effects::MayHaveSideEffects; use oxc_semantic::IsGlobalReference; use oxc_span::GetSpan; use oxc_syntax::scope::ScopeFlags; diff --git a/crates/oxc_minifier/src/peephole/remove_dead_code.rs b/crates/oxc_minifier/src/peephole/remove_dead_code.rs index 8f54eda42026b..d6105558f45f4 100644 --- a/crates/oxc_minifier/src/peephole/remove_dead_code.rs +++ b/crates/oxc_minifier/src/peephole/remove_dead_code.rs @@ -482,7 +482,7 @@ impl<'a, 'b> PeepholeOptimizations { match ctx.get_boolean_value(&expr.test) { Some(v) => { - if expr.test.may_have_side_effects() { + if ctx.expression_may_have_side_efffects(&expr.test) { let mut exprs = ctx.ast.vec_with_capacity(2); exprs.push(ctx.ast.move_expression(&mut expr.test)); exprs.push(ctx.ast.move_expression(if v { @@ -519,7 +519,9 @@ impl<'a, 'b> PeepholeOptimizations { let (should_fold, new_len) = sequence_expr.expressions.iter().enumerate().fold( (false, 0), |(mut should_fold, mut new_len), (i, expr)| { - if i == sequence_expr.expressions.len() - 1 || expr.may_have_side_effects() { + if i == sequence_expr.expressions.len() - 1 + || ctx.expression_may_have_side_efffects(expr) + { new_len += 1; } else { should_fold = true; @@ -536,7 +538,7 @@ impl<'a, 'b> PeepholeOptimizations { let mut new_exprs = ctx.ast.vec_with_capacity(new_len); let len = sequence_expr.expressions.len(); for (i, expr) in sequence_expr.expressions.iter_mut().enumerate() { - if i == len - 1 || expr.may_have_side_effects() { + if i == len - 1 || ctx.expression_may_have_side_efffects(expr) { new_exprs.push(ctx.ast.move_expression(expr)); } } diff --git a/crates/oxc_minifier/src/peephole/replace_known_methods.rs b/crates/oxc_minifier/src/peephole/replace_known_methods.rs index 96567861a9ab6..f5cf175a998a2 100644 --- a/crates/oxc_minifier/src/peephole/replace_known_methods.rs +++ b/crates/oxc_minifier/src/peephole/replace_known_methods.rs @@ -4,8 +4,8 @@ use cow_utils::CowUtils; use oxc_ast::ast::*; use oxc_ecmascript::{ - constant_evaluation::ConstantEvaluation, StringCharAt, StringCharCodeAt, StringIndexOf, - StringLastIndexOf, StringSubstring, ToInt32, + constant_evaluation::ConstantEvaluation, side_effects::MayHaveSideEffects, StringCharAt, + StringCharCodeAt, StringIndexOf, StringLastIndexOf, StringSubstring, ToInt32, }; use oxc_span::SPAN; use oxc_syntax::es_target::ESTarget; diff --git a/crates/oxc_minifier/src/peephole/statement_fusion.rs b/crates/oxc_minifier/src/peephole/statement_fusion.rs index 20fa9b8198462..17c4b62cf9053 100644 --- a/crates/oxc_minifier/src/peephole/statement_fusion.rs +++ b/crates/oxc_minifier/src/peephole/statement_fusion.rs @@ -1,6 +1,5 @@ use oxc_allocator::Vec; use oxc_ast::ast::*; -use oxc_ecmascript::side_effects::MayHaveSideEffects; use oxc_span::GetSpan; use oxc_traverse::TraverseCtx; @@ -67,7 +66,10 @@ impl<'a> PeepholeOptimizations { for_stmt.init.is_none() || for_stmt.init.as_ref().is_some_and(ForStatementInit::is_expression) } - Statement::ForInStatement(for_in_stmt) => !for_in_stmt.left.may_have_side_effects(), + // Avoid cases where we have for(var x = foo() in a) { .... + Statement::ForInStatement(for_in_stmt) => { + !matches!(&for_in_stmt.left, ForStatementLeft::VariableDeclaration(_)) + } Statement::LabeledStatement(labeled_stmt) => { Self::is_fusable_control_statement(&labeled_stmt.body) } @@ -287,7 +289,7 @@ mod test { // Never fuse a statement into a block that contains let/const/class declarations, or you risk // colliding variable names. (unless the AST is normalized). test("a; {b;}", "a,b"); - test("a; {b; var a = 1;}", "{a,b; var a = 1;}"); + test("a; {b; var a = 1;}", "{b; var a = 1;}"); test_same("a; { b; let a = 1; }"); test_same("a; { b; const a = 1; }"); test_same("a; { b; class a {} }"); diff --git a/crates/oxc_minifier/src/peephole/substitute_alternate_syntax.rs b/crates/oxc_minifier/src/peephole/substitute_alternate_syntax.rs index 26ee54e2128aa..9ebc4931c3f7f 100644 --- a/crates/oxc_minifier/src/peephole/substitute_alternate_syntax.rs +++ b/crates/oxc_minifier/src/peephole/substitute_alternate_syntax.rs @@ -1,9 +1,7 @@ use oxc_allocator::{CloneIn, Vec}; use oxc_ast::{ast::*, NONE}; use oxc_ecmascript::{ - constant_evaluation::{ConstantEvaluation, ValueType}, - side_effects::MayHaveSideEffects, - ToJsString, ToNumber, + constant_evaluation::ValueType, side_effects::MayHaveSideEffects, ToJsString, ToNumber, }; use oxc_span::GetSpan; use oxc_span::SPAN; @@ -455,7 +453,9 @@ impl<'a> PeepholeOptimizations { let Some(argument) = &stmt.argument else { return }; if !match argument { Expression::Identifier(ident) => Ctx(ctx).is_identifier_undefined(ident), - Expression::UnaryExpression(e) => e.operator.is_void() && !e.may_have_side_effects(), + Expression::UnaryExpression(e) => { + e.operator.is_void() && !Ctx(ctx).expression_may_have_side_efffects(argument) + } _ => false, } { return; @@ -1439,7 +1439,7 @@ mod test { #[test] fn test_fold_arrow_function_return() { test("const foo = () => { return 'baz' }", "const foo = () => 'baz'"); - test("const foo = () => { foo; return 'baz' }", "const foo = () => (foo, 'baz');"); + test("const foo = () => { foo; return 'baz' }", "const foo = () => 'baz'"); } #[test] diff --git a/tasks/minsize/minsize.snap b/tasks/minsize/minsize.snap index 0513ae9b85a62..9a02cc0dd5add 100644 --- a/tasks/minsize/minsize.snap +++ b/tasks/minsize/minsize.snap @@ -3,7 +3,7 @@ Original | minified | minified | gzip | gzip | Fixture ------------------------------------------------------------------------------------- 72.14 kB | 23.61 kB | 23.70 kB | 8.55 kB | 8.54 kB | react.development.js -173.90 kB | 59.71 kB | 59.82 kB | 19.26 kB | 19.33 kB | moment.js +173.90 kB | 59.69 kB | 59.82 kB | 19.26 kB | 19.33 kB | moment.js 287.63 kB | 89.58 kB | 90.07 kB | 31.08 kB | 31.95 kB | jquery.js @@ -19,9 +19,9 @@ Original | minified | minified | gzip | gzip | Fixture 2.14 MB | 719.54 kB | 724.14 kB | 162.47 kB | 181.07 kB | victory.js -3.20 MB | 1.01 MB | 1.01 MB | 325.40 kB | 331.56 kB | echarts.js +3.20 MB | 1.01 MB | 1.01 MB | 325.41 kB | 331.56 kB | echarts.js 6.69 MB | 2.30 MB | 2.31 MB | 470.00 kB | 488.28 kB | antd.js -10.95 MB | 3.37 MB | 3.49 MB | 866.68 kB | 915.50 kB | typescript.js +10.95 MB | 3.37 MB | 3.49 MB | 866.65 kB | 915.50 kB | typescript.js