From a5ccc7da30fb0553afc7d4e9a673d5cda7992c8a Mon Sep 17 00:00:00 2001 From: Wenzhe Wang Date: Tue, 20 Jun 2023 22:25:28 +0800 Subject: [PATCH] feat(minifier): port undefined_comparison1 (#458) --- crates/oxc_hir/src/hir_util.rs | 157 ++++++++++++++++-- crates/oxc_minifier/src/compressor/fold.rs | 107 ++++++++++-- crates/oxc_minifier/src/compressor/mod.rs | 8 +- .../tests/closure/fold_constants.rs | 95 +++++++++++ 4 files changed, 333 insertions(+), 34 deletions(-) diff --git a/crates/oxc_hir/src/hir_util.rs b/crates/oxc_hir/src/hir_util.rs index 79e8f9b56..17baffe1e 100644 --- a/crates/oxc_hir/src/hir_util.rs +++ b/crates/oxc_hir/src/hir_util.rs @@ -101,19 +101,39 @@ pub trait CheckForStateChange<'a, 'b> { } impl<'a, 'b> CheckForStateChange<'a, 'b> for Expression<'a> { - fn check_for_state_change(&self, _check_for_new_objects: bool) -> bool { + fn check_for_state_change(&self, check_for_new_objects: bool) -> bool { match self { Self::NumberLiteral(_) | Self::BooleanLiteral(_) | Self::StringLiteral(_) | Self::BigintLiteral(_) | Self::NullLiteral(_) - | Self::RegExpLiteral(_) => false, + | Self::RegExpLiteral(_) + | Self::FunctionExpression(_) => false, Self::Identifier(ident) => { !matches!(ident.name.as_str(), "undefined" | "Infinity" | "NaN") } Self::UnaryExpression(unary_expr) => { - unary_expr.check_for_state_change(_check_for_new_objects) + unary_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, } @@ -121,14 +141,60 @@ impl<'a, 'b> CheckForStateChange<'a, 'b> for Expression<'a> { } impl<'a, 'b> CheckForStateChange<'a, 'b> for UnaryExpression<'a> { - fn check_for_state_change(&self, _check_for_new_objects: bool) -> bool { + 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); + return self.argument.check_for_state_change(check_for_new_objects); } true } } +impl<'a, 'b> CheckForStateChange<'a, 'b> 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), + Self::Expression(expr) => expr.check_for_state_change(check_for_new_objects), + Self::Elision(_) => false, + } + } +} + +impl<'a, 'b> CheckForStateChange<'a, 'b> 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, 'b> CheckForStateChange<'a, 'b> 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, 'b> CheckForStateChange<'a, 'b> 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, 'b> CheckForStateChange<'a, 'b> for PropertyKey<'a> { + fn check_for_state_change(&self, check_for_new_objects: bool) -> bool { + match self { + Self::Identifier(_) | Self::PrivateIdentifier(_) => false, + Self::Expression(expr) => expr.check_for_state_change(check_for_new_objects), + } + } +} + impl<'a, 'b> MayHaveSideEffects<'a, 'b> for Expression<'a> {} impl<'a, 'b> MayHaveSideEffects<'a, 'b> for UnaryExpression<'a> {} @@ -137,33 +203,90 @@ fn is_simple_unary_operator(operator: UnaryOperator) -> bool { operator != UnaryOperator::Delete } +#[derive(PartialEq)] +pub enum NumberValue { + Number(f64), + PositiveInfinity, + NegativeInfinity, + NaN, +} + +impl NumberValue { + #[must_use] + pub fn not(&self) -> Self { + match self { + Self::Number(num) => Self::Number(-num), + Self::PositiveInfinity => Self::NegativeInfinity, + Self::NegativeInfinity => Self::PositiveInfinity, + Self::NaN => Self::NaN, + } + } +} + /// port from [closure compiler](https://github.com/google/closure-compiler/blob/a4c880032fba961f7a6c06ef99daa3641810bfdd/src/com/google/javascript/jscomp/NodeUtil.java#L348) /// Gets the value of a node as a Number, or None if it cannot be converted. -pub fn get_number_value(expr: &Expression) -> Option { +/// This method does not consider whether `expr` may have side effects. +pub fn get_number_value(expr: &Expression) -> Option { match expr { - Expression::NumberLiteral(number_literal) => Some(number_literal.value), + Expression::NumberLiteral(number_literal) => { + Some(NumberValue::Number(number_literal.value)) + } Expression::UnaryExpression(unary_expr) => match unary_expr.operator { UnaryOperator::UnaryPlus => get_number_value(&unary_expr.argument), - UnaryOperator::UnaryNegation => get_number_value(&unary_expr.argument).map(|v| -v), - UnaryOperator::BitwiseNot => get_number_value(&unary_expr.argument) - .map(|value| f64::from(!NumberLiteral::ecmascript_to_int32(value))), - UnaryOperator::LogicalNot => { - get_boolean_value(expr).map(|boolean| if boolean { 1.0 } else { 0.0 }) - } + UnaryOperator::UnaryNegation => get_number_value(&unary_expr.argument).map(|v| v.not()), + UnaryOperator::BitwiseNot => get_number_value(&unary_expr.argument).map(|value| { + match value { + NumberValue::Number(num) => { + NumberValue::Number(f64::from(!NumberLiteral::ecmascript_to_int32(num))) + } + // ~Infinity -> -1 + // ~-Infinity -> -1 + // ~NaN -> -1 + _ => NumberValue::Number(-1_f64), + } + }), + UnaryOperator::LogicalNot => get_boolean_value(expr) + .map(|boolean| if boolean { 1_f64 } else { 0_f64 }) + .map(NumberValue::Number), + UnaryOperator::Void => Some(NumberValue::NaN), _ => None, }, Expression::BooleanLiteral(bool_literal) => { if bool_literal.value { - Some(1.0) + Some(NumberValue::Number(1.0)) } else { - Some(0.0) + Some(NumberValue::Number(0.0)) } } - Expression::NullLiteral(_) => Some(0.0), + Expression::NullLiteral(_) => Some(NumberValue::Number(0.0)), + Expression::Identifier(ident) => match ident.name.as_str() { + "Infinity" => Some(NumberValue::PositiveInfinity), + "NaN" | "undefined" => Some(NumberValue::NaN), + _ => None, + }, + // TODO: will be implemented in next PR, just for test pass now. + Expression::StringLiteral(string_literal) => string_literal + .value + .parse::() + .map_or(Some(NumberValue::NaN), |num| Some(NumberValue::Number(num))), _ => None, } } +/// port from [closure compiler](https://github.com/google/closure-compiler/blob/a4c880032fba961f7a6c06ef99daa3641810bfdd/src/com/google/javascript/jscomp/AbstractPeepholeOptimization.java#L104-L114) +/// Returns the number value of the node if it has one and it cannot have side effects. +pub fn get_side_free_number_value(expr: &Expression) -> Option { + let value = get_number_value(expr); + // Calculating the number value, if any, is likely to be faster than calculating side effects, + // 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() { + return value; + } + None +} + /// port from [closure compiler](https://github.com/google/closure-compiler/blob/a4c880032fba961f7a6c06ef99daa3641810bfdd/src/com/google/javascript/jscomp/NodeUtil.java#L109) /// Gets the boolean value of a node that represents an expression, or `None` if no /// such value can be determined by static analysis. @@ -239,7 +362,7 @@ pub fn get_boolean_value(expr: &Expression) -> Option { // +1 -> true // +0 -> false // -0 -> false - get_number_value(expr).map(|value| value != 0.0) + get_number_value(expr).map(|value| value != NumberValue::Number(0_f64)) } else if unary_expr.operator == UnaryOperator::LogicalNot { // !true -> false get_boolean_value(&unary_expr.argument).map(|boolean| !boolean) diff --git a/crates/oxc_minifier/src/compressor/fold.rs b/crates/oxc_minifier/src/compressor/fold.rs index 2a93fe246..f8d0bb2b2 100644 --- a/crates/oxc_minifier/src/compressor/fold.rs +++ b/crates/oxc_minifier/src/compressor/fold.rs @@ -4,7 +4,10 @@ #[allow(clippy::wildcard_imports)] use oxc_hir::hir::*; -use oxc_hir::hir_util::{get_boolean_value, get_number_value, IsLiteralValue, MayHaveSideEffects}; +use oxc_hir::hir_util::{ + get_boolean_value, get_number_value, get_side_free_number_value, IsLiteralValue, + MayHaveSideEffects, NumberValue, +}; use oxc_span::{Atom, Span}; use oxc_syntax::{ operator::{BinaryOperator, UnaryOperator}, @@ -21,6 +24,20 @@ enum Tri { Unknown, } +impl Tri { + pub fn not(self) -> Self { + match self { + Self::True => Self::False, + Self::False => Self::True, + Self::Unknown => Self::Unknown, + } + } + + pub fn for_boolean(boolean: bool) -> Self { + if boolean { Self::True } else { Self::False } + } +} + /// JavaScript Language Type /// /// @@ -44,10 +61,26 @@ impl<'a> From<&Expression<'a>> for Ty { Expression::BooleanLiteral(_) => Self::Boolean, Expression::NullLiteral(_) => Self::Null, Expression::NumberLiteral(_) => Self::Number, - Expression::ObjectExpression(_) => Self::Object, Expression::StringLiteral(_) => Self::Str, + Expression::ObjectExpression(_) + | Expression::ArrayExpression(_) + | Expression::RegExpLiteral(_) + | Expression::FunctionExpression(_) => Self::Object, Expression::Identifier(ident) => match ident.name.as_str() { "undefined" => Self::Void, + "NaN" | "Infinity" => Self::Number, + _ => Self::Undetermined, + }, + Expression::UnaryExpression(unary_expr) => match unary_expr.operator { + UnaryOperator::Void => Self::Void, + UnaryOperator::UnaryNegation => { + let argument_ty = Self::from(&unary_expr.argument); + if argument_ty == Self::BigInt { + return Self::BigInt; + } + Self::Number + } + UnaryOperator::LogicalNot => Self::Boolean, _ => Self::Undetermined, }, _ => Self::Undetermined, @@ -59,7 +92,14 @@ impl<'a> Compressor<'a> { pub(crate) fn fold_expression<'b>(&mut self, expr: &'b mut Expression<'a>) { let folded_expr = match expr { Expression::BinaryExpression(binary_expr) => match binary_expr.operator { - BinaryOperator::Equality => self.try_fold_comparison( + BinaryOperator::Equality + | BinaryOperator::Inequality + | BinaryOperator::StrictEquality + | BinaryOperator::StrictInequality + | BinaryOperator::LessThan + | BinaryOperator::LessEqualThan + | BinaryOperator::GreaterThan + | BinaryOperator::GreaterEqualThan => self.try_fold_comparison( binary_expr.span, binary_expr.operator, &binary_expr.left, @@ -69,8 +109,6 @@ impl<'a> Compressor<'a> { }, Expression::UnaryExpression(unary_expr) => match unary_expr.operator { UnaryOperator::Typeof => { - // typeof +-~! 0 -> typeof 2 - self.fold_expression(&mut unary_expr.argument); self.try_fold_typeof(unary_expr.span, &unary_expr.argument) } UnaryOperator::UnaryPlus @@ -112,8 +150,27 @@ impl<'a> Compressor<'a> { left: &'b Expression<'a>, right: &'b Expression<'a>, ) -> Tri { + if left.may_have_side_effects() || right.may_have_side_effects() { + return Tri::Unknown; + } + match op { BinaryOperator::Equality => self.try_abstract_equality_comparison(left, right), + BinaryOperator::Inequality => self.try_abstract_equality_comparison(left, right).not(), + BinaryOperator::StrictEquality => self.try_strict_equality_comparison(left, right), + BinaryOperator::StrictInequality => { + self.try_strict_equality_comparison(left, right).not() + } + BinaryOperator::LessThan => self.try_abstract_relational_comparison(left, right, false), + BinaryOperator::GreaterThan => { + self.try_abstract_relational_comparison(right, left, false) + } + BinaryOperator::LessEqualThan => { + self.try_abstract_relational_comparison(right, left, true).not() + } + BinaryOperator::GreaterEqualThan => { + self.try_abstract_relational_comparison(left, right, true).not() + } _ => Tri::Unknown, } } @@ -133,10 +190,33 @@ impl<'a> Compressor<'a> { if matches!((left, right), (Ty::Null, Ty::Void) | (Ty::Void, Ty::Null)) { return Tri::True; } + + return Tri::False; } Tri::Unknown } + /// + fn try_abstract_relational_comparison<'b>( + &self, + left: &'b Expression<'a>, + right: &'b Expression<'a>, + will_negative: bool, + ) -> Tri { + // try comparing as Numbers. + let left_num = get_side_free_number_value(left); + let right_num = get_side_free_number_value(right); + if let Some(left_num) = left_num && let Some(right_num) = right_num { + match (left_num, right_num) { + (NumberValue::NaN, _) | (_, NumberValue::NaN) => return Tri::for_boolean(will_negative), + (NumberValue::Number(left_num), NumberValue::Number(right_num)) => return Tri::for_boolean(left_num < right_num), + _ => {} + } + } + + Tri::Unknown + } + /// fn try_strict_equality_comparison<'b>( &self, @@ -177,10 +257,13 @@ impl<'a> Compressor<'a> { | Expression::ObjectExpression(_) | Expression::ArrayExpression(_) => Some("object"), Expression::Identifier(_) if argument.is_undefined() => Some("undefined"), - Expression::UnaryExpression(unary_expr) - if unary_expr.operator == UnaryOperator::Void => - { - Some("undefined") + Expression::UnaryExpression(unary_expr) => { + match unary_expr.operator { + UnaryOperator::Void => Some("undefined"), + // `unary_expr.argument` is literal value, so it's safe to fold + UnaryOperator::LogicalNot => Some("boolean"), + _ => None, + } } _ => None, }; @@ -198,9 +281,6 @@ impl<'a> Compressor<'a> { &mut self, unary_expr: &'b mut UnaryExpression<'a>, ) -> Option> { - // fold its children first, so that we can fold - -4. - self.fold_expression(&mut unary_expr.argument); - if let Some(boolean) = get_boolean_value(&unary_expr.argument) { match unary_expr.operator { // !100 -> false @@ -238,7 +318,8 @@ impl<'a> Compressor<'a> { // +true -> 1 // +false -> 0 // +null -> 0 - if let Some(value) = get_number_value(&unary_expr.argument) { + if let Some(value) = get_number_value(&unary_expr.argument) + && let NumberValue::Number(value) = value { let raw = self.hir.new_str(value.to_string().as_str()); let number_literal = self.hir.number_literal( unary_expr.span, diff --git a/crates/oxc_minifier/src/compressor/mod.rs b/crates/oxc_minifier/src/compressor/mod.rs index 201d09339..bc7d689b1 100644 --- a/crates/oxc_minifier/src/compressor/mod.rs +++ b/crates/oxc_minifier/src/compressor/mod.rs @@ -279,11 +279,11 @@ impl<'a, 'b> VisitMut<'a, 'b> for Compressor<'a> { } fn visit_expression(&mut self, expr: &'b mut Expression<'a>) { - self.fold_expression(expr); - if self.compress_undefined(expr) || self.compress_boolean(expr) { - return; - } self.visit_expression_match(expr); + self.fold_expression(expr); + if !self.compress_undefined(expr) { + self.compress_boolean(expr); + } } fn visit_binary_expression(&mut self, expr: &'b mut BinaryExpression<'a>) { diff --git a/crates/oxc_minifier/tests/closure/fold_constants.rs b/crates/oxc_minifier/tests/closure/fold_constants.rs index 19a33df88..f3c51dfa2 100644 --- a/crates/oxc_minifier/tests/closure/fold_constants.rs +++ b/crates/oxc_minifier/tests/closure/fold_constants.rs @@ -5,6 +5,101 @@ use crate::{test, test_same}; #[test] fn undefined_comparison1() { test("undefined == undefined", "!0"); + test("undefined == null", "!0"); + test("undefined == void 0", "!0"); + + test("undefined == 0", "!1"); + test("undefined == 1", "!1"); + test("undefined == 'hi'", "!1"); + test("undefined == true", "!1"); + test("undefined == false", "!1"); + + test("undefined === undefined", "!0"); + test("undefined === null", "!1"); + test("undefined === void 0", "!0"); + + // origin was `test_same("undefined == this");` + test("undefined == this", "void 0==this"); + // origin was `test_same("undefined == x");` + test("undefined == x", "void 0==x"); + + test("undefined != undefined", "!1"); + test("undefined != null", "!1"); + test("undefined != void 0", "!1"); + + test("undefined != 0", "!0"); + test("undefined != 1", "!0"); + test("undefined != 'hi'", "!0"); + test("undefined != true", "!0"); + test("undefined != false", "!0"); + + test("undefined !== undefined", "!1"); + test("undefined !== void 0", "!1"); + test("undefined !== null", "!0"); + + // origin was `test_same("undefined != this");` + test("undefined != this", "void 0!=this"); + // origin was `test_same("undefined != x");` + test("undefined != x", "void 0!=x"); + + test("undefined < undefined", "!1"); + test("undefined > undefined", "!1"); + test("undefined >= undefined", "!1"); + test("undefined <= undefined", "!1"); + + test("0 < undefined", "!1"); + test("true > undefined", "!1"); + test("'hi' >= undefined", "!1"); + test("null <= undefined", "!1"); + + test("undefined < 0", "!1"); + test("undefined > true", "!1"); + test("undefined >= 'hi'", "!1"); + test("undefined <= null", "!1"); + + test("null == undefined", "!0"); + test("0 == undefined", "!1"); + test("1 == undefined", "!1"); + test("'hi' == undefined", "!1"); + test("true == undefined", "!1"); + test("false == undefined", "!1"); + test("null === undefined", "!1"); + test("void 0 === undefined", "!0"); + + test("undefined == NaN", "!1"); + test("NaN == undefined", "!1"); + test("undefined == Infinity", "!1"); + test("Infinity == undefined", "!1"); + test("undefined == -Infinity", "!1"); + test("-Infinity == undefined", "!1"); + test("({}) == undefined", "!1"); + test("undefined == ({})", "!1"); + test("([]) == undefined", "!1"); + test("undefined == ([])", "!1"); + test("(/a/g) == undefined", "!1"); + test("undefined == (/a/g)", "!1"); + test("(function(){}) == undefined", "!1"); + test("undefined == (function(){})", "!1"); + + test("undefined != NaN", "!0"); + test("NaN != undefined", "!0"); + test("undefined != Infinity", "!0"); + test("Infinity != undefined", "!0"); + test("undefined != -Infinity", "!0"); + test("-Infinity != undefined", "!0"); + test("({}) != undefined", "!0"); + test("undefined != ({})", "!0"); + test("([]) != undefined", "!0"); + test("undefined != ([])", "!0"); + test("(/a/g) != undefined", "!0"); + test("undefined != (/a/g)", "!0"); + test("(function(){}) != undefined", "!0"); + test("undefined != (function(){})", "!0"); + + // origin was `test_same("this == undefined");` + test("this == undefined", "this==void 0"); + // origin was `test_same("x == undefined");` + test("x == undefined", "x==void 0"); } #[test]