Skip to content

feat(minifier): keep indirect access more precisely #9562

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

Merged
graphite-app merged 1 commit into from
Mar 5, 2025
Merged

feat(minifier): keep indirect access more precisely #9562

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
67 changes: 60 additions & 7 deletions crates/oxc_minifier/src/peephole/fold_constants.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ use oxc_ecmascript::{
};
use oxc_span::GetSpan;
use oxc_syntax::operator::{BinaryOperator, LogicalOperator};
use oxc_traverse::Ancestor;

use crate::ctx::Ctx;

Expand Down Expand Up @@ -126,12 +125,22 @@ impl<'a> PeepholeOptimizations {
if if lval { op.is_or() } else { op.is_and() } {
return Some(ctx.ast.move_expression(&mut logical_expr.left));
} else if !left.may_have_side_effects(&ctx) {
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()
|| matches!(parent, Ancestor::CallExpressionCallee(_))
{
return None;
let should_keep_indirect_access =
Self::should_keep_indirect_access(&logical_expr.right, ctx);
// (true && o.f) => (0, o.f)
if should_keep_indirect_access {
return Some(ctx.ast.expression_sequence(
logical_expr.span,
ctx.ast.vec_from_array([
ctx.ast.expression_numeric_literal(
logical_expr.left.span(),
0.0,
None,
NumberBase::Decimal,
),
ctx.ast.move_expression(&mut logical_expr.right),
]),
));
}
// (FALSE || x) => x
// (TRUE && x) => x
Expand Down Expand Up @@ -191,6 +200,23 @@ impl<'a> PeepholeOptimizations {
]);
ctx.ast.expression_sequence(logical_expr.span, expressions)
} else {
let should_keep_indirect_access =
Self::should_keep_indirect_access(&logical_expr.right, ctx);
// (null ?? o.f) => (0, o.f)
if should_keep_indirect_access {
return Some(ctx.ast.expression_sequence(
logical_expr.span,
ctx.ast.vec_from_array([
ctx.ast.expression_numeric_literal(
logical_expr.left.span(),
0.0,
None,
NumberBase::Decimal,
),
ctx.ast.move_expression(&mut logical_expr.right),
]),
));
}
// nullish condition => this expression evaluates to the right side.
ctx.ast.move_expression(&mut logical_expr.right)
})
Expand All @@ -200,6 +226,23 @@ impl<'a> PeepholeOptimizations {
| ValueType::String
| ValueType::Boolean
| ValueType::Object => {
let should_keep_indirect_access =
Self::should_keep_indirect_access(&logical_expr.left, ctx);
// (o.f ?? something) => (0, o.f)
if should_keep_indirect_access {
return Some(ctx.ast.expression_sequence(
logical_expr.span,
ctx.ast.vec_from_array([
ctx.ast.expression_numeric_literal(
logical_expr.right.span(),
0.0,
None,
NumberBase::Decimal,
),
ctx.ast.move_expression(&mut logical_expr.left),
]),
));
}
// non-nullish condition => this expression evaluates to the left side.
Some(ctx.ast.move_expression(&mut logical_expr.left))
}
Expand Down Expand Up @@ -1274,6 +1317,11 @@ mod test {
// 1 || 0 == 1, but true =/= 1
fold("x = foo() && true || bar()", "x = foo() && !0 || bar()");
fold("foo() && true || bar()", "foo() && !0 || bar()");

test("var y; x = (true && y)()", "var y; x = y()");
test("var y; x = (true && y.z)()", "var y; x = (0, y.z)()");
test("var y; x = (false || y)()", "var y; x = y()");
test("var y; x = (false || y.z)()", "var y; x = (0, y.z)()");
}

#[test]
Expand Down Expand Up @@ -1315,6 +1363,11 @@ mod test {

fold("a() ?? (1 ?? b())", "a() ?? 1");
fold("(a() ?? 1) ?? b()", "a() ?? 1 ?? b()");

test_same("var y; x = (y ?? 1)()"); // can compress to "var y; x = y()" if y is not null or undefined
test_same("var y; x = (y.z ?? 1)()"); // "var y; x = (0, y.z)()" if y is not null or undefined
test("var y; x = (null ?? y)()", "var y; x = y()");
test("var y; x = (null ?? y.z)()", "var y; x = (0, y.z)()");
}

#[test]
Expand Down
81 changes: 64 additions & 17 deletions crates/oxc_minifier/src/peephole/remove_dead_code.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,14 +379,6 @@ impl<'a, 'b> PeepholeOptimizations {
expr: &mut ConditionalExpression<'a>,
ctx: Ctx<'a, 'b>,
) -> Option<Expression<'a>> {
// 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()
|| matches!(parent, Ancestor::CallExpressionCallee(_))
{
return None;
}

expr.test.evaluate_value_to_boolean(&ctx).map(|v| {
if expr.test.may_have_side_effects(&ctx) {
// "(a, true) ? b : c" => "a, b"
Expand All @@ -404,7 +396,31 @@ impl<'a, 'b> PeepholeOptimizations {
]);
ctx.ast.expression_sequence(expr.span, exprs)
} else {
ctx.ast.move_expression(if v { &mut expr.consequent } else { &mut expr.alternate })
let result_expr = ctx.ast.move_expression(if v {
&mut expr.consequent
} else {
&mut expr.alternate
});

let should_keep_as_sequence_expr =
Self::should_keep_indirect_access(&result_expr, ctx);
// "(1 ? a.b : 0)()" => "(0, a.b)()"
if should_keep_as_sequence_expr {
ctx.ast.expression_sequence(
expr.span,
ctx.ast.vec_from_iter([
ctx.ast.expression_numeric_literal(
expr.span,
0.0,
None,
NumberBase::Decimal,
),
result_expr,
]),
)
} else {
result_expr
}
}
})
}
Expand All @@ -414,10 +430,10 @@ impl<'a, 'b> PeepholeOptimizations {
sequence_expr: &mut SequenceExpression<'a>,
ctx: Ctx<'a, 'b>,
) -> Option<Expression<'a>> {
let should_keep_as_sequence_expr = matches!(
ctx.parent(),
Ancestor::CallExpressionCallee(_) | Ancestor::TaggedTemplateExpressionTag(_)
);
let should_keep_as_sequence_expr = sequence_expr
.expressions
.last()
.is_some_and(|last_expr| Self::should_keep_indirect_access(last_expr, ctx));
if should_keep_as_sequence_expr
&& sequence_expr.expressions.len() == 2
&& sequence_expr.expressions.first().unwrap().is_number_0()
Expand Down Expand Up @@ -455,6 +471,22 @@ impl<'a, 'b> PeepholeOptimizations {
}
None
}

/// Whether the indirect access should be kept.
/// For example, `(0, foo.bar)()` should not be transformed to `foo.bar()`.
/// Example case: `let o = { f() { assert.ok(this !== o); } }; (true && o.f)(); (true && o.f)``;`
///
/// * `access_value` - The expression that may need to be kept as indirect reference (`foo.bar` in the example above)
pub fn should_keep_indirect_access(access_value: &Expression<'a>, ctx: Ctx<'a, 'b>) -> bool {
matches!(
ctx.parent(),
Ancestor::CallExpressionCallee(_) | Ancestor::TaggedTemplateExpressionTag(_)
) && match access_value {
Expression::Identifier(id) => id.name == "eval" && ctx.is_global_reference(id),
match_member_expression!(Expression) => true,
_ => false,
}
}
}

impl<'a> LatePeepholeOptimizations {
Expand Down Expand Up @@ -604,6 +636,11 @@ mod test {
test("false ? foo() : bar()", "bar()");
test_same("foo() ? bar() : baz()");
test("foo && false ? foo() : bar()", "(foo, bar());");

test("var a; (true ? a : 0)()", "var a; a()");
test("var a; (true ? a.b : 0)()", "var a; (0, a.b)()");
test("var a; (false ? 0 : a)()", "var a; a()");
test("var a; (false ? 0 : a.b)()", "var a; (0, a.b)()");
}

#[test]
Expand Down Expand Up @@ -635,10 +672,20 @@ mod test {
#[test]
fn remove_unused_expressions_in_sequence() {
test("true, foo();", "foo();");
test("(true, foo)();", "(0, foo)();");
test("(true, true, foo)();", "(0, foo)();");
test("var foo; (true, foo)();", "var foo; (0, foo)();");
test("var foo; (true, true, foo)();", "var foo; (0, foo)();");
test("(0, foo)();", "foo();");
test("(0, foo)``;", "foo``;");
test("(0, foo)?.();", "foo?.();");
test_same("(0, eval)();"); // this can be compressed to `eval?.()`
test_same("(0, eval)``;"); // this can be compressed to `eval?.()`
test_same("(0, eval)?.();"); // this can be compressed to `eval?.()`
test("var eval; (0, eval)();", "var eval; eval();");
test_same("(0, foo.bar)();");
test_same("(0, foo.bar)``;");
test_same("(0, foo.bar)?.();");
test("(true, foo.bar)();", "(0, foo.bar)();");
test("(true, true, foo.bar)();", "(0, foo.bar)();");
test("var foo; (true, foo.bar)();", "var foo; (0, foo.bar)();");
test("var foo; (true, true, foo.bar)();", "var foo; (0, foo.bar)();");
}

#[test]
Expand Down
50 changes: 25 additions & 25 deletions crates/oxc_minifier/tests/peephole/esbuild.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1281,18 +1281,18 @@ fn test_flatten_values() {
test("tag`a${x}b${'y'}c`", "tag`a${x}b${'y'}c`;");
test("tag`a${'x'}b${y}c`", "tag`a${'x'}b${y}c`;");
test("tag`a${'x'}b${'y'}c`", "tag`a${'x'}b${'y'}c`;");
// test("(1, x)``", "x``;");
test("(1, x)``", "x``;");
test("(1, x.y)``", "(0, x.y)``;");
test("(1, x[y])``", "(0, x[y])``;");
// test("(true && x)``", "x``;");
// test("(true && x.y)``", "(0, x.y)``;");
// test("(true && x[y])``", "(0, x[y])``;");
// test("(false || x)``", "x``;");
// test("(false || x.y)``", "(0, x.y)``;");
// test("(false || x[y])``", "(0, x[y])``;");
test("(true && x)``", "x``;");
test("(true && x.y)``", "(0, x.y)``;");
test("(true && x[y])``", "(0, x[y])``;");
test("(false || x)``", "x``;");
test("(false || x.y)``", "(0, x.y)``;");
test("(false || x[y])``", "(0, x[y])``;");
test("(null ?? x)``", "x``;");
// test("(null ?? x.y)``", "(0, x.y)``;");
// test("(null ?? x[y])``", "(0, x[y])``;");
test("(null ?? x.y)``", "(0, x.y)``;");
test("(null ?? x[y])``", "(0, x[y])``;");
// test("function f(a) { let c = a.b; return c`` }", "function f(a) { return (0, a.b)``;}");
// test(
// "function f(a) { let c = a.b; return c`${x}` }",
Expand Down Expand Up @@ -1525,26 +1525,26 @@ fn test_remove_dead_expr() {
test("let x = (y, 2)", "let x = (y, 2);");
test("let x = (/* @__PURE__ */ foo(bar), 2)", "let x = (bar, 2);");
test("let x = (2, y)", "let x = y;");
// test("let x = (2, y)()", "let x = y();");
// test("let x = (true && y)()", "let x = y();");
// test("let x = (false || y)()", "let x = y();");
test("let x = (2, y)()", "let x = y();");
test("let x = (true && y)()", "let x = y();");
test("let x = (false || y)()", "let x = y();");
test("let x = (null ?? y)()", "let x = y();");
// test("let x = (1 ? y : 2)()", "let x = y();");
// test("let x = (0 ? 1 : y)()", "let x = y();");
test("let x = (1 ? y : 2)()", "let x = y();");
test("let x = (0 ? 1 : y)()", "let x = y();");
test("let x = (2, y.z)", "let x = y.z;");
test("let x = (2, y.z)()", "let x = (0, y.z)();");
// test("let x = (true && y.z)()", "let x = (0, y.z)();");
// test("let x = (false || y.z)()", "let x = (0, y.z)();");
// test("let x = (null ?? y.z)()", "let x = (0, y.z)();");
// test("let x = (1 ? y.z : 2)()", "let x = (0, y.z)();");
// test("let x = (0 ? 1 : y.z)()", "let x = (0, y.z)();");
test("let x = (true && y.z)()", "let x = (0, y.z)();");
test("let x = (false || y.z)()", "let x = (0, y.z)();");
test("let x = (null ?? y.z)()", "let x = (0, y.z)();");
test("let x = (1 ? y.z : 2)()", "let x = (0, y.z)();");
test("let x = (0 ? 1 : y.z)()", "let x = (0, y.z)();");
test("let x = (2, y[z])", "let x = y[z];");
// test("let x = (2, y[z])()", "let x = (0, y[z])();");
// test("let x = (true && y[z])()", "let x = (0, y[z])();");
// test("let x = (false || y[z])()", "let x = (0, y[z])();");
// test("let x = (null ?? y[z])()", "let x = (0, y[z])();");
// test("let x = (1 ? y[z] : 2)()", "let x = (0, y[z])();");
// test("let x = (0 ? 1 : y[z])()", "let x = (0, y[z])();");
test("let x = (2, y[z])()", "let x = (0, y[z])();");
test("let x = (true && y[z])()", "let x = (0, y[z])();");
test("let x = (false || y[z])()", "let x = (0, y[z])();");
test("let x = (null ?? y[z])()", "let x = (0, y[z])();");
test("let x = (1 ? y[z] : 2)()", "let x = (0, y[z])();");
test("let x = (0 ? 1 : y[z])()", "let x = (0, y[z])();");
test("delete (x)", "delete x;");
test("delete (x); var x", "delete x;var x;");
test("delete (x.y)", "delete x.y;");
Expand Down
14 changes: 7 additions & 7 deletions crates/oxc_minifier/tests/peephole/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,10 +76,10 @@ fn fold() {
fn tagged_template() {
test("(1, o.f)()", "(0, o.f)()");
test("(1, o.f)``", "(0, o.f)``");
test_same("(!0 && o.f)()");
test_same("(!0 && o.f)``");
test("(!0 ? o.f : !1)()", "(0 ? !1: o.f)()");
test("(!0 ? o.f : !1)``", "(0 ? !1: o.f)``");
test("(!0 && o.f)()", "(0, o.f)()");
test("(!0 && o.f)``", "(0, o.f)``");
test("(!0 ? o.f : !1)()", "(0, o.f)()");
test("(!0 ? o.f : !1)``", "(0, o.f)``");

test("foo(true && o.f)", "foo(o.f)");
test("foo(true ? o.f : false)", "foo(o.f)");
Expand All @@ -88,9 +88,9 @@ fn tagged_template() {
#[test]
fn eval() {
// Keep indirect-eval syntaxes
test_same("(!0 && eval)(x)");
test_same("(1 ? eval : 2)(x)");
test_same("(1 ? eval : 2)?.(x)");
test("(!0 && eval)(x)", "(0, eval)(x)");
test("(1 ? eval : 2)(x)", "(0, eval)(x)");
test("(1 ? eval : 2)?.(x)", "(0, eval)?.(x)");
test("(1, eval)(x)", "(0, eval)(x)");
test("(1, eval)?.(x)", "(0, eval)?.(x)");
test("(3, eval)(x)", "(0, eval)(x)");
Expand Down
Loading