Skip to content

Commit 1bf83eb

Browse files
sapphi-redsapphi-red
committed
fix(minifier): bail out arguments copy loop substitution if the temporary variables are referenced outside the for loop (#14613)
fixes rolldown/rolldown#6458 refs #13114
1 parent 5e0ab1b commit 1bf83eb

File tree

1 file changed

+83
-19
lines changed

1 file changed

+83
-19
lines changed

‎crates/oxc_minifier/src/peephole/substitute_alternate_syntax.rs‎

Lines changed: 83 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -504,26 +504,48 @@ impl<'a> PeepholeOptimizations {
504504
/// r[a - 1] = arguments[a];
505505
/// ```
506506
fn try_rewrite_arguments_copy_loop(for_stmt: &mut ForStatement<'a>, ctx: &mut Ctx<'a, '_>) {
507+
#[derive(PartialEq, Eq)]
508+
enum VerifyArrayArgResult {
509+
WithOffset,
510+
WithoutOffset,
511+
Invalid,
512+
}
513+
507514
/// Verify whether `arg_expr` is `e > offset ? e - offset : 0` or `e`
508-
fn verify_array_arg(arg_expr: &Expression, name_e: &str, offset: f64) -> bool {
515+
fn verify_array_arg(
516+
arg_expr: &Expression,
517+
name_e: &str,
518+
offset: f64,
519+
) -> VerifyArrayArgResult {
509520
match arg_expr {
510-
Expression::Identifier(id) => offset == 0.0 && id.name == name_e,
521+
Expression::Identifier(id) => {
522+
if offset == 0.0 && id.name == name_e {
523+
VerifyArrayArgResult::WithoutOffset
524+
} else {
525+
VerifyArrayArgResult::Invalid
526+
}
527+
}
511528
Expression::ConditionalExpression(cond_expr) => {
512529
let Expression::BinaryExpression(test_expr) = &cond_expr.test else {
513-
return false;
530+
return VerifyArrayArgResult::Invalid;
514531
};
515532
let Expression::BinaryExpression(cons_expr) = &cond_expr.consequent else {
516-
return false;
533+
return VerifyArrayArgResult::Invalid;
517534
};
518-
test_expr.operator == BinaryOperator::GreaterThan
535+
if test_expr.operator == BinaryOperator::GreaterThan
519536
&& test_expr.left.is_specific_id(name_e)
520537
&& matches!(&test_expr.right, Expression::NumericLiteral(n) if n.value == offset)
521538
&& cons_expr.operator == BinaryOperator::Subtraction
522539
&& matches!(&cons_expr.left, Expression::Identifier(id) if id.name == name_e)
523540
&& matches!(&cons_expr.right, Expression::NumericLiteral(n) if n.value == offset)
524541
&& matches!(&cond_expr.alternate, Expression::NumericLiteral(n) if n.value == 0.0)
542+
{
543+
VerifyArrayArgResult::WithOffset
544+
} else {
545+
VerifyArrayArgResult::Invalid
546+
}
525547
}
526-
_ => false,
548+
_ => VerifyArrayArgResult::Invalid,
527549
}
528550
}
529551

@@ -553,6 +575,10 @@ impl<'a> PeepholeOptimizations {
553575
assign_expr
554576
};
555577

578+
// reference counts in the for-loop
579+
let mut a_ref_count = 0;
580+
let mut e_ref_count = 0;
581+
556582
let (r_id_name, a_id_name, offset) = {
557583
let AssignmentTarget::ComputedMemberExpression(lhs_member_expr) =
558584
&body_assign_expr.left
@@ -602,6 +628,7 @@ impl<'a> PeepholeOptimizations {
602628
}
603629
rhs_member_expr_obj
604630
};
631+
a_ref_count += 2;
605632

606633
// Parse update: `a++`
607634
{
@@ -614,10 +641,11 @@ impl<'a> PeepholeOptimizations {
614641
if a_id_name != id.name {
615642
return;
616643
}
644+
a_ref_count += 1;
617645
};
618646

619647
// Parse test: `a < e` or `a < arguments.length`
620-
let e_id_name = {
648+
let e_id_info = {
621649
let Some(Expression::BinaryExpression(b)) = &for_stmt.test else {
622650
return;
623651
};
@@ -629,7 +657,10 @@ impl<'a> PeepholeOptimizations {
629657
return;
630658
}
631659
match &b.right {
632-
Expression::Identifier(right) => Some(&right.name),
660+
Expression::Identifier(right) => Some((
661+
&right.name,
662+
ctx.scoping().get_reference(right.reference_id()).symbol_id(),
663+
)),
633664
Expression::StaticMemberExpression(sm) => {
634665
let Expression::Identifier(id) = &sm.object else {
635666
return;
@@ -645,8 +676,12 @@ impl<'a> PeepholeOptimizations {
645676
_ => return,
646677
}
647678
};
679+
if e_id_info.is_some() {
680+
e_ref_count += 1;
681+
}
682+
a_ref_count += 1;
648683

649-
let init_decl_len = if e_id_name.is_some() { 3 } else { 2 };
684+
let init_decl_len = if e_id_info.is_some() { 3 } else { 2 };
650685

651686
let Some(init) = &mut for_stmt.init else { return };
652687
let ForStatementInit::VariableDeclaration(var_init) = init else { return };
@@ -664,7 +699,7 @@ impl<'a> PeepholeOptimizations {
664699
let mut idx = 0usize;
665700

666701
// Check `e = arguments.length`
667-
if let Some(e_id_name) = e_id_name {
702+
if let Some((e_id_name, _)) = e_id_info {
668703
let de = var_init
669704
.declarations
670705
.get(idx)
@@ -686,7 +721,7 @@ impl<'a> PeepholeOptimizations {
686721
}
687722

688723
// Check `a = 0` or `a = k`
689-
{
724+
let a_id_symbol_id = {
690725
let de_a = var_init
691726
.declarations
692727
.get(idx + 1)
@@ -698,10 +733,11 @@ impl<'a> PeepholeOptimizations {
698733
if !matches!(&de_a.init, Some(Expression::NumericLiteral(n)) if n.value == offset) {
699734
return;
700735
}
701-
}
736+
de_id.symbol_id()
737+
};
702738

703739
// Check `r = Array(e > 1 ? e - 1 : 0)`, or `r = []`
704-
let r_id_pat = {
740+
let r_id_pat_with_info = {
705741
let de_r = var_init
706742
.declarations
707743
.get_mut(idx)
@@ -716,11 +752,13 @@ impl<'a> PeepholeOptimizations {
716752
if call.arguments.len() != 1 {
717753
return;
718754
}
719-
let Some(e_id_name) = e_id_name else { return };
755+
let Some((e_id_name, _)) = e_id_info else { return };
720756
let Some(arg_expr) = call.arguments[0].as_expression() else { return };
721-
if !verify_array_arg(arg_expr, e_id_name, offset) {
757+
let result = verify_array_arg(arg_expr, e_id_name, offset);
758+
if result == VerifyArrayArgResult::Invalid {
722759
return;
723760
}
761+
e_ref_count += if result == VerifyArrayArgResult::WithOffset { 2 } else { 1 };
724762
}
725763
Some(Expression::ArrayExpression(arr)) => {
726764
if !arr.elements.is_empty() {
@@ -733,15 +771,35 @@ impl<'a> PeepholeOptimizations {
733771
if de_id.name != r_id_name {
734772
return;
735773
}
774+
let de_id_symbol_id = de_id.symbol_id();
775+
(&mut de_r.id, de_id_symbol_id)
776+
};
777+
778+
// bail out if `e` or `a` is used outside the for-loop
779+
{
780+
if let Some((_, e_id_symbol_id)) = e_id_info
781+
&& e_id_symbol_id.is_none_or(|id| {
782+
ctx.scoping().get_resolved_references(id).count() != e_ref_count
783+
})
784+
{
785+
return;
786+
}
787+
if ctx.scoping().get_resolved_references(a_id_symbol_id).count() != a_ref_count {
788+
return;
789+
}
790+
}
791+
792+
// Build `var r = [...arguments]` (with optional `.slice(offset)`) as the only declarator and drop test/update/body.
793+
794+
let r_id_pat = {
795+
let (r_id, de_id_symbol_id) = r_id_pat_with_info;
736796
// `var r = [...arguments]` / `var r = [...arguments].slice(n)` is not needed
737797
// if r is not used by other places because `[...arguments]` does not have a sideeffect
738798
// `r` is used once in the for-loop (assignment for each index)
739-
(ctx.scoping().get_resolved_references(de_id.symbol_id()).count() > 1)
740-
.then(|| de_r.id.take_in(ctx.ast))
799+
(ctx.scoping().get_resolved_references(de_id_symbol_id).count() > 1)
800+
.then(|| r_id.take_in(ctx.ast))
741801
};
742802

743-
// Build `var r = [...arguments]` (with optional `.slice(offset)`) as the only declarator and drop test/update/body.
744-
745803
let base_arr = ctx.ast.expression_array(
746804
SPAN,
747805
ctx.ast.vec1(ctx.ast.array_expression_element_spread_element(
@@ -2367,6 +2425,12 @@ mod test {
23672425
"function _() { { let _; for (var e = arguments.length, r = Array(e), a = 0; a < e; a++) r[a] = arguments[a]; console.log(r) } }",
23682426
"function _() { { let _; var r = [...arguments]; console.log(r) } }",
23692427
);
2428+
test_same(
2429+
"function _() { for (var e = arguments.length, r = Array(e), a = 0; a < e; a++) r[a] = arguments[a]; console.log(r, e) }",
2430+
);
2431+
test_same(
2432+
"function _() { for (var e = arguments.length, r = Array(e), a = 0; a < e; a++) r[a] = arguments[a]; console.log(r, a) }",
2433+
);
23702434
}
23712435

23722436
#[test]

0 commit comments

Comments
 (0)