Skip to content

Commit

Permalink
refactor(minifier): align code with closure compiler (#5717)
Browse files Browse the repository at this point in the history 
also fixes #4341
  • Loading branch information
@Boshen
Boshen committed Sep 12, 2024
1 parent 4bdc202 commit 746f7b3
Show file tree
Hide file tree
Showing 6 changed files with 128 additions and 80 deletions.
67 changes: 38 additions & 29 deletions crates/oxc_minifier/src/ast_passes/fold_constants.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,11 @@ impl<'a> FoldConstants<'a> {
pub fn fold_expression(&self, expr: &mut Expression<'a>, ctx: &mut TraverseCtx<'a>) {
if let Some(folded_expr) = match expr {
Expression::BinaryExpression(e) => self.try_fold_binary_operator(e, ctx),
Expression::LogicalExpression(e) => self.try_fold_logical_expression(e, ctx),
Expression::LogicalExpression(e)
if matches!(e.operator, LogicalOperator::And | LogicalOperator::Or) =>
{
self.try_fold_and_or(e, ctx)
}
// TODO: move to `PeepholeMinimizeConditions`
Expression::ConditionalExpression(e) => self.try_fold_conditional_expression(e, ctx),
Expression::UnaryExpression(e) => self.try_fold_unary_expression(e, ctx),
Expand Down Expand Up @@ -111,7 +115,7 @@ impl<'a> FoldConstants<'a> {
ctx: &mut TraverseCtx<'a>,
) -> Option<bool> {
self.fold_expression(expr, ctx);
ctx.get_boolean_value(expr)
ctx.get_boolean_value(expr).to_option()
}

fn fold_if_statement(&self, stmt: &mut Statement<'a>, ctx: &mut TraverseCtx<'a>) {
Expand Down Expand Up @@ -150,11 +154,15 @@ impl<'a> FoldConstants<'a> {
expr: &mut ConditionalExpression<'a>,
ctx: &mut TraverseCtx<'a>,
) -> Option<Expression<'a>> {
if ctx.ancestry.parent().is_tagged_template_expression() {
return None;
}
match self.fold_expression_and_get_boolean_value(&mut expr.test, ctx) {
Some(true) => Some(self.ast.move_expression(&mut expr.consequent)),
Some(true) => {
// Bail `let o = { f() { assert.ok(this !== o); } }; (true ? o.f : false)(); (true ? o.f : false)``;`
let parent = ctx.ancestry.parent();
if parent.is_tagged_template_expression() || parent.is_call_expression() {
return None;
}
Some(self.ast.move_expression(&mut expr.consequent))
}
Some(false) => Some(self.ast.move_expression(&mut expr.alternate)),
_ => None,
}
Expand Down Expand Up @@ -384,7 +392,7 @@ impl<'a> FoldConstants<'a> {
let right_bigint = ctx.get_side_free_bigint_value(right_expr);

if let (Some(l_big), Some(r_big)) = (left_bigint, right_bigint) {
return Tri::for_boolean(l_big.eq(&r_big));
return Tri::from(l_big.eq(&r_big));
}
}

Expand Down Expand Up @@ -421,7 +429,7 @@ impl<'a> FoldConstants<'a> {
return Tri::Unknown;
}

return Tri::for_boolean(left_string.cmp(&right_string) == Ordering::Less);
return Tri::from(left_string.cmp(&right_string) == Ordering::Less);
}

if let (Expression::UnaryExpression(left), Expression::UnaryExpression(right)) =
Expand Down Expand Up @@ -450,14 +458,14 @@ impl<'a> FoldConstants<'a> {
match (left_bigint, right_bigint, left_num, right_num) {
// Next, try to evaluate based on the value of the node. Try comparing as BigInts first.
(Some(l_big), Some(r_big), _, _) => {
return Tri::for_boolean(l_big < r_big);
return Tri::from(l_big < r_big);
}
// try comparing as Numbers.
(_, _, Some(l_num), Some(r_num)) => match (l_num, r_num) {
(NumberValue::NaN, _) | (_, NumberValue::NaN) => {
return Tri::for_boolean(will_negative);
return Tri::from(will_negative);
}
(NumberValue::Number(l), NumberValue::Number(r)) => return Tri::for_boolean(l < r),
(NumberValue::Number(l), NumberValue::Number(r)) => return Tri::from(l < r),
_ => {}
},
// Finally, try comparisons between BigInt and Number.
Expand All @@ -484,7 +492,7 @@ impl<'a> FoldConstants<'a> {
// if invert is false, then the number is on the right in tryAbstractRelationalComparison
// if it's true, then the number is on the left
match number_value {
NumberValue::NaN => Tri::for_boolean(will_negative),
NumberValue::NaN => Tri::from(will_negative),
NumberValue::PositiveInfinity => Tri::True.xor(invert),
NumberValue::NegativeInfinity => Tri::False.xor(invert),
NumberValue::Number(num) => {
Expand All @@ -502,7 +510,7 @@ impl<'a> FoldConstants<'a> {
if is_exact_int64(*num) {
Tri::False
} else {
Tri::for_boolean(num.is_sign_positive()).xor(invert)
Tri::from(num.is_sign_positive()).xor(invert)
}
}
}
Expand Down Expand Up @@ -534,7 +542,7 @@ impl<'a> FoldConstants<'a> {
return Tri::False;
}

return Tri::for_boolean(l_num == r_num);
return Tri::from(l_num == r_num);
}

Tri::Unknown
Expand All @@ -548,7 +556,7 @@ impl<'a> FoldConstants<'a> {
return Tri::Unknown;
}

return Tri::for_boolean(left_string == right_string);
return Tri::from(left_string == right_string);
}

if let (Expression::UnaryExpression(left), Expression::UnaryExpression(right)) =
Expand Down Expand Up @@ -640,22 +648,20 @@ impl<'a> FoldConstants<'a> {
/// Try to fold a AND / OR node.
///
/// port from [closure-compiler](https://github.com/google/closure-compiler/blob/09094b551915a6487a980a783831cba58b5739d1/src/com/google/javascript/jscomp/PeepholeFoldConstants.java#L587)
pub fn try_fold_logical_expression(
pub fn try_fold_and_or(
&self,
logical_expr: &mut LogicalExpression<'a>,
ctx: &mut TraverseCtx<'a>,
) -> Option<Expression<'a>> {
if ctx.ancestry.parent().is_tagged_template_expression() {
return None;
}
let op = logical_expr.operator;
if !matches!(op, LogicalOperator::And | LogicalOperator::Or) {
return None;
}
debug_assert!(matches!(op, LogicalOperator::And | LogicalOperator::Or));

if let Some(boolean_value) = ctx.get_boolean_value(&logical_expr.left) {
let left = &logical_expr.left;
let left_val = ctx.get_boolean_value(left).to_option();

if let Some(lval) = left_val {
// Bail `0 && (module.exports = {})` for `cjs-module-lexer`.
if !boolean_value {
if !lval {
if let Expression::AssignmentExpression(assign_expr) = &logical_expr.right {
if let Some(member_expr) = assign_expr.left.as_member_expression() {
if member_expr.is_specific_member_access("module", "exports") {
Expand All @@ -667,11 +673,14 @@ impl<'a> FoldConstants<'a> {

// (TRUE || x) => TRUE (also, (3 || x) => 3)
// (FALSE && x) => FALSE
if (boolean_value && op == LogicalOperator::Or)
|| (!boolean_value && op == LogicalOperator::And)
{
if if lval { op == LogicalOperator::Or } else { op == LogicalOperator::And } {
return Some(self.ast.move_expression(&mut logical_expr.left));
} else if !logical_expr.left.may_have_side_effects() {
} else if !left.may_have_side_effects() {
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() || parent.is_call_expression() {
return None;
}
// (FALSE || x) => x
// (TRUE && x) => x
return Some(self.ast.move_expression(&mut logical_expr.right));
Expand All @@ -688,7 +697,7 @@ impl<'a> FoldConstants<'a> {
return Some(sequence_expr);
} else if let Expression::LogicalExpression(left_child) = &mut logical_expr.left {
if left_child.operator == logical_expr.operator {
let left_child_right_boolean = ctx.get_boolean_value(&left_child.right);
let left_child_right_boolean = ctx.get_boolean_value(&left_child.right).to_option();
let left_child_op = left_child.operator;
if let Some(right_boolean) = left_child_right_boolean {
if !left_child.right.may_have_side_effects() {
Expand Down
3 changes: 2 additions & 1 deletion crates/oxc_minifier/src/node_util/check_for_state_change.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,11 @@ fn is_simple_unary_operator(operator: UnaryOperator) -> bool {
operator != UnaryOperator::Delete
}

/// port from [closure-compiler](https://github.com/google/closure-compiler/blob/f3ce5ed8b630428e311fe9aa2e20d36560d975e2/src/com/google/javascript/jscomp/AstAnalyzer.java#L241)
/// 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;
}
Expand Down
5 changes: 3 additions & 2 deletions crates/oxc_minifier/src/node_util/may_have_side_effects.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,17 @@ use oxc_ast::ast::*;

use super::check_for_state_change::CheckForStateChange;

/// port from [closure-compiler](https://github.com/google/closure-compiler/blob/f3ce5ed8b630428e311fe9aa2e20d36560d975e2/src/com/google/javascript/jscomp/AstAnalyzer.java#L94)
/// 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).
///
/// 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(false)
self.check_for_state_change(/* check_for_new_objects */ false)
}
}

Expand Down
66 changes: 36 additions & 30 deletions crates/oxc_minifier/src/node_util/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,8 @@ use oxc_ast::ast::*;
use oxc_semantic::{IsGlobalReference, ScopeTree, SymbolTable};
use oxc_syntax::operator::{AssignmentOperator, LogicalOperator, UnaryOperator};

use crate::tri::Tri;

pub use self::{
is_literal_value::IsLiteralValue, may_have_side_effects::MayHaveSideEffects,
number_value::NumberValue,
Expand Down Expand Up @@ -89,20 +91,22 @@ pub trait NodeUtil {
/// Gets the boolean value of a node that represents an expression, or `None` if no
/// such value can be determined by static analysis.
/// This method does not consider whether the node may have side-effects.
fn get_boolean_value(&self, expr: &Expression) -> Option<bool> {
fn get_boolean_value(&self, expr: &Expression) -> Tri {
match expr {
Expression::RegExpLiteral(_)
| Expression::ArrayExpression(_)
| Expression::ArrowFunctionExpression(_)
| Expression::ClassExpression(_)
| Expression::FunctionExpression(_)
| Expression::NewExpression(_)
| Expression::ObjectExpression(_) => Some(true),
Expression::NullLiteral(_) => Some(false),
Expression::BooleanLiteral(boolean_literal) => Some(boolean_literal.value),
Expression::NumericLiteral(number_literal) => Some(number_literal.value != 0.0),
Expression::BigIntLiteral(big_int_literal) => Some(!big_int_literal.is_zero()),
Expression::StringLiteral(string_literal) => Some(!string_literal.value.is_empty()),
| Expression::ObjectExpression(_) => Tri::True,
Expression::NullLiteral(_) => Tri::False,
Expression::BooleanLiteral(boolean_literal) => Tri::from(boolean_literal.value),
Expression::NumericLiteral(number_literal) => Tri::from(number_literal.value != 0.0),
Expression::BigIntLiteral(big_int_literal) => Tri::from(!big_int_literal.is_zero()),
Expression::StringLiteral(string_literal) => {
Tri::from(!string_literal.value.is_empty())
}
Expression::TemplateLiteral(template_literal) => {
// only for ``
template_literal
Expand All @@ -111,16 +115,17 @@ pub trait NodeUtil {
.filter(|quasi| quasi.tail)
.and_then(|quasi| quasi.value.cooked.as_ref())
.map(|cooked| !cooked.is_empty())
.into()
}
Expression::Identifier(ident) => match ident.name.as_str() {
"NaN" => Some(false),
"Infinity" => Some(true),
"undefined" if self.is_identifier_undefined(ident) => Some(false),
_ => None,
"NaN" => Tri::False,
"Infinity" => Tri::True,
"undefined" if self.is_identifier_undefined(ident) => Tri::False,
_ => Tri::Unknown,
},
Expression::AssignmentExpression(assign_expr) => {
match assign_expr.operator {
AssignmentOperator::LogicalAnd | AssignmentOperator::LogicalOr => None,
AssignmentOperator::LogicalAnd | AssignmentOperator::LogicalOr => Tri::Unknown,
// For ASSIGN, the value is the value of the RHS.
_ => self.get_boolean_value(&assign_expr.right),
}
Expand All @@ -133,36 +138,35 @@ pub trait NodeUtil {
LogicalOperator::And => {
let left = self.get_boolean_value(&logical_expr.left);
let right = self.get_boolean_value(&logical_expr.right);

match (left, right) {
(Some(true), Some(true)) => Some(true),
(Some(false), _) | (_, Some(false)) => Some(false),
(None, _) | (_, None) => None,
(Tri::True, Tri::True) => Tri::True,
(Tri::False, _) | (_, Tri::False) => Tri::False,
(Tri::Unknown, _) | (_, Tri::Unknown) => Tri::Unknown,
}
}
// true || false -> true
// false || false -> false
// a || b -> None
// a || b -> Tri::Unknown
LogicalOperator::Or => {
let left = self.get_boolean_value(&logical_expr.left);
let right = self.get_boolean_value(&logical_expr.right);

match (left, right) {
(Some(true), _) | (_, Some(true)) => Some(true),
(Some(false), Some(false)) => Some(false),
(None, _) | (_, None) => None,
(Tri::True, _) | (_, Tri::True) => Tri::True,
(Tri::False, Tri::False) => Tri::False,
(Tri::Unknown, _) | (_, Tri::Unknown) => Tri::Unknown,
}
}
LogicalOperator::Coalesce => None,
LogicalOperator::Coalesce => Tri::Unknown,
}
}
Expression::SequenceExpression(sequence_expr) => {
// For sequence expression, the value is the value of the RHS.
sequence_expr.expressions.last().and_then(|e| self.get_boolean_value(e))
sequence_expr.expressions.last().map_or(Tri::Unknown, |e| self.get_boolean_value(e))
}
Expression::UnaryExpression(unary_expr) => {
if unary_expr.operator == UnaryOperator::Void {
Some(false)
Tri::False
} else if matches!(
unary_expr.operator,
UnaryOperator::BitwiseNot
Expand All @@ -173,15 +177,17 @@ pub trait NodeUtil {
// +1 -> true
// +0 -> false
// -0 -> false
self.get_number_value(expr).map(|value| value != NumberValue::Number(0_f64))
self.get_number_value(expr)
.map(|value| value != NumberValue::Number(0_f64))
.into()
} else if unary_expr.operator == UnaryOperator::LogicalNot {
// !true -> false
self.get_boolean_value(&unary_expr.argument).map(|boolean| !boolean)
self.get_boolean_value(&unary_expr.argument).not()
} else {
None
Tri::Unknown
}
}
_ => None,
_ => Tri::Unknown,
}
}

Expand Down Expand Up @@ -213,7 +219,7 @@ pub trait NodeUtil {
}
UnaryOperator::LogicalNot => self
.get_boolean_value(expr)
.map(|boolean| if boolean { 1_f64 } else { 0_f64 })
.map(|tri| if tri.is_true() { 1_f64 } else { 0_f64 })
.map(NumberValue::Number),
UnaryOperator::Void => Some(NumberValue::NaN),
_ => None,
Expand Down Expand Up @@ -264,7 +270,7 @@ pub trait NodeUtil {
}
Expression::UnaryExpression(unary_expr) => match unary_expr.operator {
UnaryOperator::LogicalNot => self.get_boolean_value(expr).map(|boolean| {
if boolean {
if boolean.is_true() {
BigInt::one()
} else {
BigInt::zero()
Expand Down Expand Up @@ -336,7 +342,7 @@ pub trait NodeUtil {
UnaryOperator::LogicalNot => {
self.get_boolean_value(&unary_expr.argument).map(|boolean| {
// need reversed.
if boolean {
if boolean.is_true() {
Cow::Borrowed("false")
} else {
Cow::Borrowed("true")
Expand Down
Loading

0 comments on commit 746f7b3

Please sign in to comment.