Skip to content

Commit 3941c21

Browse files
author
Ralph Küpper
committed
fix(transform): #5868 — switch inside async/generator state machines miscompiled
Two defects, one shared fix: 1. yield/await inside a case body was silently lowered to 0: the linearizer had no Stmt::Switch arm, so the switch fell to the catch-all, was emitted unsplit inside one state, and codegen's fallback lowered the residual Expr::Yield to 0.0. async f(x){ switch(x){ case 1: return await g() } } resolved to 0 without suspending; a generator yield inside a case vanished. 2. a loop-level continue inside a yield-free switch in a CPS'd loop survived as a raw Stmt::Continue the dispatch loop ignored, so the iteration's remainder ran anyway. New desugar_switch_to_ifs (break_continue.rs): discriminant temp + match-index + guarded-if chain preserving JS switch semantics (single-eval discriminant, in-order first-match-wins tests, fallthrough, default-in-the-middle, break — incl. from nested if/try — via done-flag with remainder guarding). Wired into (a) a new yielding-switch arm in linearize_body, (b) a splice in rewrite_break_continue_in_stmts for continue-bearing yield-free switches (next_local_id threaded through), and (c) the labeled arm maps 'break label' to plain break inside a labeled switch's cases. e2e: crates/perry/tests/issue_5868_switch_state_machine.rs (9 scenarios byte-for-byte vs node --experimental-strip-types). cargo test -p perry-transform: 45 passed.
1 parent bfa0f25 commit 3941c21

4 files changed

Lines changed: 657 additions & 18 deletions

File tree

crates/perry-transform/src/generator/break_continue.rs

Lines changed: 314 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,18 @@ const CONTINUE_SENTINEL: f64 = 1_000_002.0;
1717
/// into `[LocalSet(state_id, <sentinel>), Stmt::Continue]`. The trailing
1818
/// `Stmt::Continue` is the state-machine's dispatch-loop continue, which
1919
/// re-enters the while(true) and re-dispatches on the new state. Stops at
20-
/// nested loop / switch / closure boundaries — their own break/continue
21-
/// belong to those constructs, not to us.
22-
pub fn rewrite_break_continue_in_stmts(stmts: &mut Vec<Stmt>, state_id: LocalId) {
20+
/// nested loop / closure boundaries — their own break/continue belong to
21+
/// those constructs, not to us. A nested `switch` captures `break` but
22+
/// NEVER `continue`: a switch whose cases carry a loop-level `continue` is
23+
/// desugared into plain `if`s first (#5868 — previously the raw
24+
/// `Stmt::Continue` survived verbatim inside the switch in the state body
25+
/// and the dispatch lowering silently ignored it, so the rest of the loop
26+
/// iteration ran anyway).
27+
pub fn rewrite_break_continue_in_stmts(
28+
stmts: &mut Vec<Stmt>,
29+
state_id: LocalId,
30+
next_local_id: &mut u32,
31+
) {
2332
let mut i = 0;
2433
while i < stmts.len() {
2534
let stmt = std::mem::replace(&mut stmts[i], Stmt::Continue);
@@ -40,45 +49,61 @@ pub fn rewrite_break_continue_in_stmts(stmts: &mut Vec<Stmt>, state_id: LocalId)
4049
stmts.insert(i + 1, Stmt::Continue);
4150
i += 2;
4251
}
52+
Stmt::Switch {
53+
discriminant,
54+
cases,
55+
} if switch_cases_have_loop_continue(&cases) => {
56+
// Replace the switch (currently a placeholder after the
57+
// mem::replace) with its if-chain desugar and reprocess from
58+
// the same index: the desugared statements are plain `if`s,
59+
// so this rewriter descends them and converts the loop-level
60+
// `continue`s to sentinels; `break`s were already folded
61+
// into the desugar's done-flag.
62+
let desugared = desugar_switch_to_ifs(&discriminant, &cases, next_local_id);
63+
stmts.splice(i..=i, desugared);
64+
}
4365
mut other => {
44-
rewrite_break_continue_in_stmt(&mut other, state_id);
66+
rewrite_break_continue_in_stmt(&mut other, state_id, next_local_id);
4567
stmts[i] = other;
4668
i += 1;
4769
}
4870
}
4971
}
5072
}
5173

52-
pub fn rewrite_break_continue_in_stmt(stmt: &mut Stmt, state_id: LocalId) {
74+
pub fn rewrite_break_continue_in_stmt(stmt: &mut Stmt, state_id: LocalId, next_local_id: &mut u32) {
5375
match stmt {
5476
Stmt::If {
5577
then_branch,
5678
else_branch,
5779
..
5880
} => {
59-
rewrite_break_continue_in_stmts(then_branch, state_id);
81+
rewrite_break_continue_in_stmts(then_branch, state_id, next_local_id);
6082
if let Some(eb) = else_branch.as_mut() {
61-
rewrite_break_continue_in_stmts(eb, state_id);
83+
rewrite_break_continue_in_stmts(eb, state_id, next_local_id);
6284
}
6385
}
6486
Stmt::Try {
6587
body,
6688
catch,
6789
finally,
6890
} => {
69-
rewrite_break_continue_in_stmts(body, state_id);
91+
rewrite_break_continue_in_stmts(body, state_id, next_local_id);
7092
if let Some(c) = catch.as_mut() {
71-
rewrite_break_continue_in_stmts(&mut c.body, state_id);
93+
rewrite_break_continue_in_stmts(&mut c.body, state_id, next_local_id);
7294
}
7395
if let Some(f) = finally.as_mut() {
74-
rewrite_break_continue_in_stmts(f, state_id);
96+
rewrite_break_continue_in_stmts(f, state_id, next_local_id);
7597
}
7698
}
77-
// Inside nested loops / switch / labeled / closure expressions, the
78-
// user's `break`/`continue` belongs to that construct and not to the
79-
// outer loop the state machine is unrolling. Leave them as-is so the
80-
// inner linearize_body (if it yields) / regular codegen (if it
81-
// doesn't) handles them.
99+
// Inside nested loops / closure expressions, the user's
100+
// `break`/`continue` belongs to that construct and not to the outer
101+
// loop the state machine is unrolling. Leave them as-is so the inner
102+
// linearize_body (if it yields) / regular codegen (if it doesn't)
103+
// handles them. A `switch` reaching here carries no loop-level
104+
// `continue` (the stmts-level pass desugared those), and its
105+
// `break`s bind to the switch itself. `Labeled` is left as-is
106+
// (pre-existing single-sentinel limitation).
82107
Stmt::For { .. } | Stmt::While { .. } | Stmt::DoWhile { .. } => {}
83108
Stmt::Switch { .. } => {}
84109
Stmt::Labeled { .. } => {}
@@ -386,3 +411,277 @@ pub fn collect_vars_recursive(stmts: &[Stmt], vars: &mut Vec<(LocalId, String, T
386411
}
387412
}
388413
}
414+
415+
// ---------------------------------------------------------------------------
416+
// #5868: switch desugaring for state-machine bodies
417+
// ---------------------------------------------------------------------------
418+
419+
/// Does any case body carry a `continue` that binds to the ENCLOSING LOOP
420+
/// (i.e. at switch-case level, or nested only through `if`/`try`/inner
421+
/// `switch` — all constructs that do not capture `continue`)? Loops and
422+
/// labeled statements capture their own `continue`s, so descent stops there.
423+
fn switch_cases_have_loop_continue(cases: &[SwitchCase]) -> bool {
424+
cases
425+
.iter()
426+
.any(|c| stmts_have_loop_level_continue(&c.body))
427+
}
428+
429+
fn stmts_have_loop_level_continue(stmts: &[Stmt]) -> bool {
430+
stmts.iter().any(|s| match s {
431+
Stmt::Continue => true,
432+
Stmt::If {
433+
then_branch,
434+
else_branch,
435+
..
436+
} => {
437+
stmts_have_loop_level_continue(then_branch)
438+
|| else_branch
439+
.as_ref()
440+
.is_some_and(|e| stmts_have_loop_level_continue(e))
441+
}
442+
Stmt::Try {
443+
body,
444+
catch,
445+
finally,
446+
} => {
447+
stmts_have_loop_level_continue(body)
448+
|| catch
449+
.as_ref()
450+
.is_some_and(|c| stmts_have_loop_level_continue(&c.body))
451+
|| finally
452+
.as_ref()
453+
.is_some_and(|f| stmts_have_loop_level_continue(f))
454+
}
455+
Stmt::Switch { cases, .. } => switch_cases_have_loop_continue(cases),
456+
_ => false,
457+
})
458+
}
459+
460+
/// Desugar a `switch` into an equivalent match-index + guarded-`if` chain
461+
/// (#5868). Used in two places:
462+
///
463+
/// 1. `linearize_body`'s yielding-switch arm — a `yield`/`await` inside a
464+
/// case body previously fell through to the catch-all, was emitted
465+
/// unsplit inside one state, and codegen lowered the residual
466+
/// `Expr::Yield` to `0.0`.
467+
/// 2. `rewrite_break_continue_in_stmts` — a loop-level `continue` inside
468+
/// a (yield-free) switch in a linearized loop body previously survived
469+
/// as a raw `Stmt::Continue` the dispatch loop ignored.
470+
///
471+
/// Shape (JS switch semantics preserved):
472+
///
473+
/// ```text
474+
/// __sw_d = <discriminant>; // evaluated exactly once
475+
/// __sw_idx = UNMATCHED;
476+
/// // case tests, evaluated only while still unmatched (first match wins;
477+
/// // spec order == source order of the non-default clauses):
478+
/// if (__sw_idx === UNMATCHED) { __sw_t = <test_i>; if (__sw_d === __sw_t) __sw_idx = i; }
479+
/// ...
480+
/// if (__sw_idx === UNMATCHED) __sw_idx = <default position, or past-end>;
481+
/// __sw_done = false;
482+
/// // bodies in POSITIONAL order — `__sw_idx <= i` gives fallthrough;
483+
/// // `break` becomes `__sw_done = true` plus remainder-guarding:
484+
/// if (!__sw_done && __sw_idx <= i) { <guarded body_i> }
485+
/// ...
486+
/// ```
487+
///
488+
/// `continue` / `return` / `throw` in case bodies pass through untouched —
489+
/// after the desugar they sit in plain `if`s, where the loop machinery (or
490+
/// function-level lowering) handles them normally. Fresh locals follow the
491+
/// DoWhile-flag pattern (plain `LocalSet` on an `alloc_local` id; generator
492+
/// local persistence carries them across suspend states).
493+
pub fn desugar_switch_to_ifs(
494+
discriminant: &Expr,
495+
cases: &[SwitchCase],
496+
next_local_id: &mut u32,
497+
) -> Vec<Stmt> {
498+
let n = cases.len();
499+
let unmatched = (n + 1) as f64;
500+
let default_pos = cases.iter().position(|c| c.test.is_none());
501+
let start_when_unmatched = default_pos.unwrap_or(n) as f64;
502+
503+
let d_id = alloc_local(next_local_id);
504+
let idx_id = alloc_local(next_local_id);
505+
let done_id = alloc_local(next_local_id);
506+
507+
let idx_is_unmatched = || Expr::Compare {
508+
op: CompareOp::Eq,
509+
left: Box::new(Expr::LocalGet(idx_id)),
510+
right: Box::new(Expr::Number(unmatched)),
511+
};
512+
513+
let mut out = Vec::with_capacity(2 * n + 4);
514+
out.push(Stmt::Expr(Expr::LocalSet(
515+
d_id,
516+
Box::new(discriminant.clone()),
517+
)));
518+
out.push(Stmt::Expr(Expr::LocalSet(
519+
idx_id,
520+
Box::new(Expr::Number(unmatched)),
521+
)));
522+
523+
// Tests in source order over the non-default clauses — identical to the
524+
// spec's pre-default-then-post-default order, since the default clause
525+
// contributes no test. Each test evaluates only while unmatched, so
526+
// side-effecting tests after the first match are (correctly) skipped.
527+
for (i, case) in cases.iter().enumerate() {
528+
let Some(test) = &case.test else { continue };
529+
let t_id = alloc_local(next_local_id);
530+
out.push(Stmt::If {
531+
condition: idx_is_unmatched(),
532+
then_branch: vec![
533+
Stmt::Expr(Expr::LocalSet(t_id, Box::new(test.clone()))),
534+
Stmt::If {
535+
condition: Expr::Compare {
536+
op: CompareOp::Eq,
537+
left: Box::new(Expr::LocalGet(d_id)),
538+
right: Box::new(Expr::LocalGet(t_id)),
539+
},
540+
then_branch: vec![Stmt::Expr(Expr::LocalSet(
541+
idx_id,
542+
Box::new(Expr::Number(i as f64)),
543+
))],
544+
else_branch: None,
545+
},
546+
],
547+
else_branch: None,
548+
});
549+
}
550+
out.push(Stmt::If {
551+
condition: idx_is_unmatched(),
552+
then_branch: vec![Stmt::Expr(Expr::LocalSet(
553+
idx_id,
554+
Box::new(Expr::Number(start_when_unmatched)),
555+
))],
556+
else_branch: None,
557+
});
558+
out.push(Stmt::Expr(Expr::LocalSet(
559+
done_id,
560+
Box::new(Expr::Bool(false)),
561+
)));
562+
563+
for (i, case) in cases.iter().enumerate() {
564+
let mut guarded = Vec::new();
565+
guard_switch_breaks(&case.body, done_id, &mut guarded);
566+
out.push(Stmt::If {
567+
condition: Expr::Logical {
568+
op: LogicalOp::And,
569+
left: Box::new(Expr::Unary {
570+
op: UnaryOp::Not,
571+
operand: Box::new(Expr::LocalGet(done_id)),
572+
}),
573+
right: Box::new(Expr::Compare {
574+
op: CompareOp::Le,
575+
left: Box::new(Expr::LocalGet(idx_id)),
576+
right: Box::new(Expr::Number(i as f64)),
577+
}),
578+
},
579+
then_branch: guarded,
580+
else_branch: None,
581+
});
582+
}
583+
out
584+
}
585+
586+
/// Copy a case body into `out`, rewriting every `break` that binds to the
587+
/// switch being desugared into `__sw_done = true`, and guarding every
588+
/// statement that follows a potentially-breaking statement behind
589+
/// `if (!__sw_done)`. Descends `if`/`try` (which don't capture `break`);
590+
/// stops at nested loops, switches, and labeled statements (whose `break`
591+
/// binds to themselves). Statements directly after a bare `break` are
592+
/// unreachable and dropped.
593+
fn guard_switch_breaks(stmts: &[Stmt], done_id: LocalId, out: &mut Vec<Stmt>) {
594+
let mut i = 0;
595+
while i < stmts.len() {
596+
let s = &stmts[i];
597+
if matches!(s, Stmt::Break) {
598+
out.push(Stmt::Expr(Expr::LocalSet(
599+
done_id,
600+
Box::new(Expr::Bool(true)),
601+
)));
602+
return;
603+
}
604+
let may_break = stmt_may_break_switch(s);
605+
out.push(rewrite_switch_breaks_in_stmt(s, done_id));
606+
i += 1;
607+
if may_break && i < stmts.len() {
608+
let mut rest = Vec::new();
609+
guard_switch_breaks(&stmts[i..], done_id, &mut rest);
610+
out.push(Stmt::If {
611+
condition: Expr::Unary {
612+
op: UnaryOp::Not,
613+
operand: Box::new(Expr::LocalGet(done_id)),
614+
},
615+
then_branch: rest,
616+
else_branch: None,
617+
});
618+
return;
619+
}
620+
}
621+
}
622+
623+
/// Can executing this statement hit a `break` that binds to the switch
624+
/// being desugared? Mirrors `guard_switch_breaks`'s descent scoping.
625+
fn stmt_may_break_switch(s: &Stmt) -> bool {
626+
match s {
627+
Stmt::Break => true,
628+
Stmt::If {
629+
then_branch,
630+
else_branch,
631+
..
632+
} => {
633+
then_branch.iter().any(stmt_may_break_switch)
634+
|| else_branch
635+
.as_ref()
636+
.is_some_and(|e| e.iter().any(stmt_may_break_switch))
637+
}
638+
Stmt::Try {
639+
body,
640+
catch,
641+
finally,
642+
} => {
643+
body.iter().any(stmt_may_break_switch)
644+
|| catch
645+
.as_ref()
646+
.is_some_and(|c| c.body.iter().any(stmt_may_break_switch))
647+
|| finally
648+
.as_ref()
649+
.is_some_and(|f| f.iter().any(stmt_may_break_switch))
650+
}
651+
_ => false,
652+
}
653+
}
654+
655+
/// Rebuild one statement with switch-binding `break`s rewritten (via
656+
/// `guard_switch_breaks`) inside its `if`/`try` sub-bodies.
657+
fn rewrite_switch_breaks_in_stmt(s: &Stmt, done_id: LocalId) -> Stmt {
658+
let guarded = |body: &[Stmt]| {
659+
let mut v = Vec::new();
660+
guard_switch_breaks(body, done_id, &mut v);
661+
v
662+
};
663+
match s {
664+
Stmt::If {
665+
condition,
666+
then_branch,
667+
else_branch,
668+
} => Stmt::If {
669+
condition: condition.clone(),
670+
then_branch: guarded(then_branch),
671+
else_branch: else_branch.as_ref().map(|e| guarded(e)),
672+
},
673+
Stmt::Try {
674+
body,
675+
catch,
676+
finally,
677+
} => Stmt::Try {
678+
body: guarded(body),
679+
catch: catch.as_ref().map(|c| CatchClause {
680+
param: c.param.clone(),
681+
body: guarded(&c.body),
682+
}),
683+
finally: finally.as_ref().map(|f| guarded(f)),
684+
},
685+
other => other.clone(),
686+
}
687+
}

crates/perry-transform/src/generator/hoist_yields.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -247,7 +247,7 @@ fn logical_rhs_contains_yield(expr: &Expr) -> bool {
247247
false
248248
}
249249

250-
fn expr_contains_yield(expr: &Expr) -> bool {
250+
pub(super) fn expr_contains_yield(expr: &Expr) -> bool {
251251
if matches!(expr, Expr::Yield { .. }) {
252252
return true;
253253
}

0 commit comments

Comments
 (0)