Skip to content

Commit

Permalink
refactor(minifier): unify ValueType (#6545)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Boshen
Boshen committed Oct 14, 2024
1 parent 521d295 commit 67ad08a
Show file tree
Hide file tree
Showing 4 changed files with 63 additions and 80 deletions.
69 changes: 40 additions & 29 deletions crates/oxc_minifier/src/ast_passes/peephole_fold_constants.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ use oxc_syntax::{
use oxc_traverse::{Ancestor, Traverse, TraverseCtx};

use crate::{
node_util::{is_exact_int64, IsLiteralValue, MayHaveSideEffects, NodeUtil, ValueType},
node_util::{is_exact_int64, IsLiteralValue, MayHaveSideEffects, NodeUtil},
tri::Tri,
ty::Ty,
value_type::ValueType,
CompressorPass,
};

Expand Down Expand Up @@ -365,9 +365,9 @@ impl<'a> PeepholeFoldConstants {
) -> Option<Expression<'a>> {
debug_assert_eq!(logical_expr.operator, LogicalOperator::Coalesce);
let left = &logical_expr.left;
let left_val = ctx.get_known_value_type(left);
let left_val = ValueType::from(left);
match left_val {
ValueType::Null | ValueType::Void => {
ValueType::Null | ValueType::Undefined => {
Some(if left.may_have_side_effects() {
// e.g. `(a(), null) ?? 1` => `(a(), null, 1)`
let expressions = ctx.ast.vec_from_iter([
Expand All @@ -381,7 +381,7 @@ impl<'a> PeepholeFoldConstants {
})
}
ValueType::Number
| ValueType::Bigint
| ValueType::BigInt
| ValueType::String
| ValueType::Boolean
| ValueType::Object => {
Expand Down Expand Up @@ -435,13 +435,13 @@ impl<'a> PeepholeFoldConstants {
return None;
}

let left_type = Ty::from(left);
let right_type = Ty::from(right);
let left_type = ValueType::from(left);
let right_type = ValueType::from(right);
match (left_type, right_type) {
(Ty::Undetermined, _) | (_, Ty::Undetermined) => None,
(ValueType::Undetermined, _) | (_, ValueType::Undetermined) => None,

// string concatenation
(Ty::Str, _) | (_, Ty::Str) => {
(ValueType::String, _) | (_, ValueType::String) => {
// no need to use get_side_effect_free_string_value b/c we checked for side effects
// at the beginning
let left_string = ctx.get_string_value(left)?;
Expand All @@ -452,9 +452,9 @@ impl<'a> PeepholeFoldConstants {
},

// number addition
(Ty::Number, _) | (_, Ty::Number)
(ValueType::Number, _) | (_, ValueType::Number)
// when added, booleans get treated as numbers where `true` is 1 and `false` is 0
| (Ty::Boolean, Ty::Boolean) => {
| (ValueType::Boolean, ValueType::Boolean) => {
let left_number = ctx.get_number_value(left)?;
let right_number = ctx.get_number_value(right)?;
let Ok(value) = TryInto::<f64>::try_into(left_number + right_number) else { return None };
Expand Down Expand Up @@ -651,17 +651,22 @@ impl<'a> PeepholeFoldConstants {
right_expr: &'b Expression<'a>,
ctx: &mut TraverseCtx<'a>,
) -> Tri {
let left = Ty::from(left_expr);
let right = Ty::from(right_expr);
if left != Ty::Undetermined && right != Ty::Undetermined {
let left = ValueType::from(left_expr);
let right = ValueType::from(right_expr);
if left != ValueType::Undetermined && right != ValueType::Undetermined {
if left == right {
return Self::try_strict_equality_comparison(left_expr, right_expr, ctx);
}
if matches!((left, right), (Ty::Null, Ty::Void) | (Ty::Void, Ty::Null)) {
if matches!(
(left, right),
(ValueType::Null, ValueType::Undefined) | (ValueType::Undefined, ValueType::Null)
) {
return Tri::True;
}

if matches!((left, right), (Ty::Number, Ty::Str)) || matches!(right, Ty::Boolean) {
if matches!((left, right), (ValueType::Number, ValueType::String))
|| matches!(right, ValueType::Boolean)
{
let right_number = ctx.get_side_free_number_value(right_expr);

if let Some(num) = right_number {
Expand All @@ -682,7 +687,9 @@ impl<'a> PeepholeFoldConstants {
return Tri::Unknown;
}

if matches!((left, right), (Ty::Str, Ty::Number)) || matches!(left, Ty::Boolean) {
if matches!((left, right), (ValueType::String, ValueType::Number))
|| matches!(left, ValueType::Boolean)
{
let left_number = ctx.get_side_free_number_value(left_expr);

if let Some(num) = left_number {
Expand All @@ -703,7 +710,7 @@ impl<'a> PeepholeFoldConstants {
return Tri::Unknown;
}

if matches!(left, Ty::BigInt) || matches!(right, Ty::BigInt) {
if matches!(left, ValueType::BigInt) || matches!(right, ValueType::BigInt) {
let left_bigint = ctx.get_side_free_bigint_value(left_expr);
let right_bigint = ctx.get_side_free_bigint_value(right_expr);

Expand All @@ -712,11 +719,15 @@ impl<'a> PeepholeFoldConstants {
}
}

if matches!(left, Ty::Str | Ty::Number) && matches!(right, Ty::Object) {
if matches!(left, ValueType::String | ValueType::Number)
&& matches!(right, ValueType::Object)
{
return Tri::Unknown;
}

if matches!(left, Ty::Object) && matches!(right, Ty::Str | Ty::Number) {
if matches!(left, ValueType::Object)
&& matches!(right, ValueType::String | ValueType::Number)
{
return Tri::Unknown;
}

Expand All @@ -732,11 +743,11 @@ impl<'a> PeepholeFoldConstants {
will_negative: bool,
ctx: &mut TraverseCtx<'a>,
) -> Tri {
let left = Ty::from(left_expr);
let right = Ty::from(right_expr);
let left = ValueType::from(left_expr);
let right = ValueType::from(right_expr);

// First, check for a string comparison.
if left == Ty::Str && right == Ty::Str {
if left == ValueType::String && right == ValueType::String {
let left_string = ctx.get_side_free_string_value(left_expr);
let right_string = ctx.get_side_free_string_value(right_expr);
if let (Some(left_string), Some(right_string)) = (left_string, right_string) {
Expand Down Expand Up @@ -842,15 +853,15 @@ impl<'a> PeepholeFoldConstants {
right_expr: &'b Expression<'a>,
ctx: &mut TraverseCtx<'a>,
) -> Tri {
let left = Ty::from(left_expr);
let right = Ty::from(right_expr);
if left != Ty::Undetermined && right != Ty::Undetermined {
let left = ValueType::from(left_expr);
let right = ValueType::from(right_expr);
if left != ValueType::Undetermined && right != ValueType::Undetermined {
// Strict equality can only be true for values of the same type.
if left != right {
return Tri::False;
}
return match left {
Ty::Number => {
ValueType::Number => {
let left_number = ctx.get_side_free_number_value(left_expr);
let right_number = ctx.get_side_free_number_value(right_expr);

Expand All @@ -864,7 +875,7 @@ impl<'a> PeepholeFoldConstants {

Tri::Unknown
}
Ty::Str => {
ValueType::String => {
let left_string = ctx.get_side_free_string_value(left_expr);
let right_string = ctx.get_side_free_string_value(right_expr);
if let (Some(left_string), Some(right_string)) = (left_string, right_string) {
Expand Down Expand Up @@ -895,7 +906,7 @@ impl<'a> PeepholeFoldConstants {

Tri::Unknown
}
Ty::Void | Ty::Null => Tri::True,
ValueType::Undefined | ValueType::Null => Tri::True,
_ => Tri::Unknown,
};
}
Expand Down
2 changes: 1 addition & 1 deletion crates/oxc_minifier/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ mod keep_var;
mod node_util;
mod options;
mod tri;
mod ty;
mod value_type;

#[cfg(test)]
mod tester;
Expand Down
35 changes: 0 additions & 35 deletions crates/oxc_minifier/src/node_util/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,6 @@ pub fn is_exact_int64(num: f64) -> bool {
num.fract() == 0.0
}

#[derive(Debug, Eq, PartialEq)]
pub enum ValueType {
Undetermined,
Null,
Void,
Number,
Bigint,
String,
Boolean,
Object,
}

pub trait NodeUtil<'a> {
fn symbols(&self) -> &SymbolTable;

Expand Down Expand Up @@ -128,27 +116,4 @@ pub trait NodeUtil<'a> {
fn get_string_bigint_value(&self, raw_string: &str) -> Option<BigInt> {
raw_string.string_to_big_int()
}

/// Evaluate and attempt to determine which primitive value type it could resolve to.
/// Without proper type information some assumptions had to be made for operations that could
/// result in a BigInt or a Number. If there is not enough information available to determine one
/// or the other then we assume Number in order to maintain historical behavior of the compiler and
/// avoid breaking projects that relied on this behavior.
fn get_known_value_type(&self, e: &Expression<'_>) -> ValueType {
match e {
Expression::NumericLiteral(_) => ValueType::Number,
Expression::NullLiteral(_) => ValueType::Null,
Expression::ArrayExpression(_) | Expression::ObjectExpression(_) => ValueType::Object,
Expression::BooleanLiteral(_) => ValueType::Boolean,
Expression::Identifier(ident) if self.is_identifier_undefined(ident) => ValueType::Void,
Expression::SequenceExpression(e) => e
.expressions
.last()
.map_or(ValueType::Undetermined, |e| self.get_known_value_type(e)),
Expression::BigIntLiteral(_) => ValueType::Bigint,
Expression::StringLiteral(_) | Expression::TemplateLiteral(_) => ValueType::String,
// TODO: complete this
_ => ValueType::Undetermined,
}
}
}
37 changes: 22 additions & 15 deletions crates/oxc_minifier/src/ty.rs → crates/oxc_minifier/src/value_type.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,37 +5,44 @@ use oxc_syntax::operator::{BinaryOperator, UnaryOperator};
///
/// <https://tc39.es/ecma262/#sec-ecmascript-language-types>
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum Ty {
BigInt,
Boolean,
pub enum ValueType {
Undefined, // a.k.a `Void` in closure compiler
Null,
Number,
BigInt,
String,
Boolean,
Object,
Str,
Void,
Undetermined,
}

impl<'a> From<&Expression<'a>> for Ty {
/// `get_known_value_type`
///
/// Evaluate and attempt to determine which primitive value type it could resolve to.
/// Without proper type information some assumptions had to be made for operations that could
/// result in a BigInt or a Number. If there is not enough information available to determine one
/// or the other then we assume Number in order to maintain historical behavior of the compiler and
/// avoid breaking projects that relied on this behavior.
impl<'a> From<&Expression<'a>> for ValueType {
fn from(expr: &Expression<'a>) -> Self {
// TODO: complete this
match expr {
Expression::BigIntLiteral(_) => Self::BigInt,
Expression::BooleanLiteral(_) => Self::Boolean,
Expression::NullLiteral(_) => Self::Null,
Expression::NumericLiteral(_) => Self::Number,
Expression::StringLiteral(_) => Self::Str,
Expression::StringLiteral(_) => Self::String,
Expression::ObjectExpression(_)
| Expression::ArrayExpression(_)
| Expression::RegExpLiteral(_)
| Expression::FunctionExpression(_) => Self::Object,
Expression::Identifier(ident) => match ident.name.as_str() {
"undefined" => Self::Void,
"undefined" => Self::Undefined,
"NaN" | "Infinity" => Self::Number,
_ => Self::Undetermined,
},
Expression::UnaryExpression(unary_expr) => match unary_expr.operator {
UnaryOperator::Void => Self::Void,
UnaryOperator::Void => Self::Undefined,
UnaryOperator::UnaryNegation => {
let argument_ty = Self::from(&unary_expr.argument);
if argument_ty == Self::BigInt {
Expand All @@ -45,29 +52,29 @@ impl<'a> From<&Expression<'a>> for Ty {
}
UnaryOperator::UnaryPlus => Self::Number,
UnaryOperator::LogicalNot => Self::Boolean,
UnaryOperator::Typeof => Self::Str,
UnaryOperator::Typeof => Self::String,
_ => Self::Undetermined,
},
Expression::BinaryExpression(binary_expr) => match binary_expr.operator {
BinaryOperator::Addition => {
let left_ty = Self::from(&binary_expr.left);
let right_ty = Self::from(&binary_expr.right);

if left_ty == Self::Str || right_ty == Self::Str {
return Self::Str;
if left_ty == Self::String || right_ty == Self::String {
return Self::String;
}

// There are some pretty weird cases for object types:
// {} + [] === "0"
// [] + {} === "[object Object]"
if left_ty == Self::Object || right_ty == Self::Object {
return Self::Undetermined;
}

Self::Undetermined
}
_ => Self::Undetermined,
},
Expression::SequenceExpression(e) => {
e.expressions.last().map_or(ValueType::Undetermined, Self::from)
}
_ => Self::Undetermined,
}
}
Expand Down

0 comments on commit 67ad08a

Please sign in to comment.