Skip to content

feat(minifier): improve peephole-replace-known-methods #6490

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

Closed
shulaoda wants to merge 6 commits into from
Closed

feat(minifier): improve peephole-replace-known-methods #6490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
97ef04e
feat(minifier): improve peephole-replace-known-methods
shulaoda Oct 13, 2024
995f792
chore: solve conflicts
shulaoda Oct 13, 2024
eac9b11
chore: use get_string_value instead
shulaoda Oct 13, 2024
9b266b2
fix
shulaoda Oct 13, 2024
fba1884
add test cases for get_string_literal
shulaoda Oct 13, 2024
2a96897
fix
shulaoda Oct 14, 2024
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
48 changes: 48 additions & 0 deletions crates/oxc_minifier/src/ast_passes/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,3 +39,51 @@ pub trait CompressorPass<'a>: Traverse<'a> {

fn build(&mut self, program: &mut Program<'a>, ctx: &mut TraverseCtx<'a>);
}

#[cfg(test)]
mod test {
use super::*;
use oxc_allocator::Allocator;
use oxc_ast::ast::Statement;
use oxc_parser::Parser;
use oxc_semantic::SemanticBuilder;
use oxc_span::SourceType;

#[derive(Default)]
pub struct Tester;

fn build<'a>(allocator: &'a Allocator, source_text: &'a str) -> (TraverseCtx<'a>, Program<'a>) {
let source_type = SourceType::mjs();

let program = Parser::new(allocator, source_text, source_type).parse().program;
let (symbols, scopes) =
SemanticBuilder::new().build(&program).semantic.into_symbol_table_and_scope_tree();

(TraverseCtx::new(scopes, symbols, allocator), program)
}

fn get_string_literal(source_text: &str) -> Option<String> {
let allocator = Allocator::default();
let (ctx, program) = build(&allocator, source_text);

let Some(Statement::ExpressionStatement(expr_stmt)) = program.body.first() else {
return None;
};

ctx.get_string_literal(&expr_stmt.expression).map(Into::into)
}

#[test]
fn test_get_string_literal() {
Comment on lines +76 to +77
Copy link
Member Author

@shulaoda shulaoda Oct 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how to implement the test template and where to place it.

Copy link
Member Author

@shulaoda shulaoda Oct 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have implemented a simple test case and temporarily placed it here.

Copy link
Member Author

@shulaoda shulaoda Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@camc314 cc

Copy link
Contributor

@7086cmd 7086cmd Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend you to put it to src/node_util/mod.rs instead.

Copy link
Member Author

@shulaoda shulaoda Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, I'm not sure if it's still necessary to exist. Because our TemplateLiteral seems to be able to be compressed into StringLiteral. I don't know if there are any specific scenarios that require this method.

assert_eq!(get_string_literal("`abc`"), Some("abc".to_string()));
assert_ne!(get_string_literal("`a${b}`"), Some("ab".to_string()));
assert_eq!(get_string_literal("`${null}`"), Some("null".to_string()));
assert_eq!(get_string_literal("`${undefined}`"), Some("undefined".to_string()));
assert_eq!(get_string_literal("`${{}}123`"), Some("[object Object]123".to_string()));
assert_eq!(get_string_literal("`a${1}${true}${NaN}0`"), Some("a1trueNaN0".to_string()));

// assert_eq!(get_string_literal("`${1,2}`"), Some("2".to_string()));
// assert_eq!(get_string_literal("`${[]}${[1,2]}`"), Some("1,2".to_string()));
// assert_eq!(get_string_literal("`${new Set()}`"), Some("[object Set]".to_string()));
}
}
118 changes: 53 additions & 65 deletions crates/oxc_minifier/src/ast_passes/peephole_replace_known_methods.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use oxc_ast::ast::*;
use oxc_ecmascript::{StringCharAt, StringIndexOf, StringLastIndexOf};
use oxc_traverse::{Traverse, TraverseCtx};

use crate::CompressorPass;
use crate::{node_util::NodeUtil, CompressorPass};

/// Minimize With Known Methods
/// <https://github.com/google/closure-compiler/blob/master/src/com/google/javascript/jscomp/PeepholeReplaceKnownMethods.java>
Expand Down Expand Up @@ -40,62 +40,56 @@ impl PeepholeReplaceKnownMethods {
) {
let Expression::CallExpression(call_expr) = node else { return };

let Expression::StaticMemberExpression(member) = &call_expr.callee else { return };
if let Expression::StringLiteral(string_lit) = &member.object {
#[expect(clippy::match_same_arms)]
let replacement = match member.property.name.as_str() {
"toLowerCase" | "toUpperCase" | "trim" => {
let transformed_value =
match member.property.name.as_str() {
"toLowerCase" => Some(ctx.ast.string_literal(
call_expr.span,
string_lit.value.cow_to_lowercase(),
)),
"toUpperCase" => Some(ctx.ast.string_literal(
call_expr.span,
string_lit.value.cow_to_uppercase(),
)),
"trim" => Some(
ctx.ast.string_literal(call_expr.span, string_lit.value.trim()),
),
_ => None,
};

transformed_value.map(|transformed_value| {
ctx.ast.expression_from_string_literal(transformed_value)
})
}
"indexOf" | "lastIndexOf" => Self::try_fold_string_index_of(
call_expr.span,
call_expr,
member,
string_lit,
ctx,
),
// TODO: Implement the rest of the string methods
"substr" => None,
"substring" | "slice" => None,
"charAt" => {
Self::try_fold_string_char_at(call_expr.span, call_expr, string_lit, ctx)
}
"charCodeAt" => None,
"replace" => None,
"replaceAll" => None,
_ => None,
};

if let Some(replacement) = replacement {
self.changed = true;
*node = replacement;
let Some(mem_expr) = call_expr.callee.as_member_expression() else { return };
let Some(string_lit) = ctx.get_string_literal(mem_expr.object()) else { return };
let Some(method_name) = mem_expr.static_property_name() else { return };

#[expect(clippy::match_same_arms)]
let replacement = match method_name {
"toLowerCase" | "toUpperCase" | "trim" => {
let transformed_value = match method_name {
"toLowerCase" => {
Some(ctx.ast.string_literal(call_expr.span, string_lit.cow_to_lowercase()))
}
"toUpperCase" => {
Some(ctx.ast.string_literal(call_expr.span, string_lit.cow_to_uppercase()))
}
"trim" => Some(ctx.ast.string_literal(call_expr.span, string_lit.trim())),
_ => None,
};

transformed_value.map(|transformed_value| {
ctx.ast.expression_from_string_literal(transformed_value)
})
}
"indexOf" | "lastIndexOf" => Self::try_fold_string_index_of(
call_expr.span,
&string_lit,
method_name,
call_expr,
ctx,
),
// TODO: Implement the rest of the string methods
"substr" => None,
"substring" | "slice" => None,
"charAt" => Self::try_fold_string_char_at(call_expr.span, &string_lit, call_expr, ctx),
"charCodeAt" => None,
"replace" => None,
"replaceAll" => None,
_ => None,
};

if let Some(replacement) = replacement {
self.changed = true;
*node = replacement;
}
}

fn try_fold_string_index_of<'a>(
span: Span,
string_lit: &str,
method_name: &str,
call_expr: &CallExpression<'a>,
member: &StaticMemberExpression<'a>,
string_lit: &StringLiteral<'a>,
ctx: &mut TraverseCtx<'a>,
) -> Option<Expression<'a>> {
let search_value = match call_expr.arguments.first() {
Expand All @@ -110,11 +104,9 @@ impl PeepholeReplaceKnownMethods {
_ => return None,
};

let result = match member.property.name.as_str() {
"indexOf" => string_lit.value.as_str().index_of(search_value, search_start_index),
"lastIndexOf" => {
string_lit.value.as_str().last_index_of(search_value, search_start_index)
}
let result = match method_name {
"indexOf" => string_lit.index_of(search_value, search_start_index),
"lastIndexOf" => string_lit.last_index_of(search_value, search_start_index),
_ => unreachable!(),
};

Expand All @@ -129,8 +121,8 @@ impl PeepholeReplaceKnownMethods {

fn try_fold_string_char_at<'a>(
span: Span,
string_lit: &str,
call_expr: &CallExpression<'a>,
string_lit: &StringLiteral<'a>,
ctx: &mut TraverseCtx<'a>,
) -> Option<Expression<'a>> {
let char_at_index: Option<f64> = match call_expr.arguments.first() {
Expand All @@ -147,11 +139,7 @@ impl PeepholeReplaceKnownMethods {
_ => return None,
};

let result = &string_lit
.value
.as_str()
.char_at(char_at_index)
.map_or(String::new(), |v| v.to_string());
let result = string_lit.char_at(char_at_index).map_or(String::new(), |v| v.to_string());

return Some(ctx.ast.expression_from_string_literal(ctx.ast.string_literal(span, result)));
}
Expand Down Expand Up @@ -217,7 +205,7 @@ mod test {
fold_same("x = 'abcdef'.indexOf([1,2])");

// Template Strings
fold_same("x = `abcdef`.indexOf('b')");
fold("x = `abcdef`.indexOf('b')", "x = 1;");
Comment on lines -220 to +208
Copy link
Contributor

@7086cmd 7086cmd Oct 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our test infra are ported from Google Closure Compiler, where it does not fold the template strings, and related code here: https://github.com/google/closure-compiler/blob/master/test/com/google/javascript/jscomp/PeepholeReplaceKnownMethodsTest.java#L114.

@Boshen any suggestions?

Copy link
Contributor

@7086cmd 7086cmd Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@shulaoda The template string contains some expressions, but some of them can't be handled. There are already methods in place to replace template strings with normal strings when possible, and the minification process iterates multiple times rather than being a single pass. So, I believe we can focus on the string itself and not worry about template strings here. Everything else looks fantastic—thanks for your contribution!

Copy link
Contributor

@7086cmd 7086cmd Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So don't worry these tests here - after several times, from the template string, to the normal string, the minifier can handle this scenario :-)

Copy link
Member Author

@shulaoda shulaoda Oct 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are correct, I saw that the closure-compiler did recognize and convert the TemplateLiteral.

fold_same("x = `Hello ${name}`.indexOf('a')");
fold_same("x = tag `Hello ${name}`.indexOf('a')");
}
Expand Down Expand Up @@ -431,7 +419,7 @@ mod test {
// fold("x = '\\ud834\udd1e'.charAt(1)", "x = '\\udd1e'");

// Template strings
fold_same("x = `abcdef`.charAt(0)");
fold("x = `abcdef`.charAt(0)", "x = 'a'");
fold_same("x = `abcdef ${abc}`.charAt(0)");
}

Expand Down Expand Up @@ -546,7 +534,7 @@ mod test {
fold("'A'.toUpperCase()", "'A'");
fold("'aBcDe'.toUpperCase()", "'ABCDE'");

fold_same("`abc`.toUpperCase()");
fold("`abc`.toUpperCase()", "'ABC';");
fold_same("`a ${bc}`.toUpperCase()");

/*
Expand Down Expand Up @@ -578,7 +566,7 @@ mod test {
fold("'a'.toLowerCase()", "'a'");
fold("'aBcDe'.toLowerCase()", "'abcde'");

fold_same("`ABC`.toLowerCase()");
fold("`ABC`.toLowerCase()", "'abc'");
fold_same("`A ${BC}`.toLowerCase()");

/*
Expand Down
9 changes: 9 additions & 0 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 @@ -116,6 +116,15 @@ pub trait NodeUtil<'a> {
expr.to_big_int()
}

/// Retrieve the literal value of a string, such as `abc` or "abc".
fn get_string_literal(&self, expr: &Expression<'a>) -> Option<Cow<'a, str>> {
match expr {
Expression::StringLiteral(lit) => Some(Cow::Borrowed(lit.value.as_str())),
Expression::TemplateLiteral(_) => Some(self.get_string_value(expr)?),
_ => None,
}
}

/// Port from [closure-compiler](https://github.com/google/closure-compiler/blob/e13f5cd0a5d3d35f2db1e6c03fdf67ef02946009/src/com/google/javascript/jscomp/NodeUtil.java#L234)
/// Gets the value of a node as a String, or `None` if it cannot be converted. When it returns a
/// String, this method effectively emulates the `String()` JavaScript cast function.
Expand Down