Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(minifier): unify ValueType #6545

Merged
merged 1 commit into from
Oct 14, 2024
Merged

refactor(minifier): unify ValueType #6545

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 40 additions & 29 deletions crates/oxc_minifier/src/ast_passes/peephole_fold_constants.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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