diff --git a/crates/perry-hir/src/lower/for_head.rs b/crates/perry-hir/src/lower/for_head.rs index aed23739a9..6e051fd1c6 100644 --- a/crates/perry-hir/src/lower/for_head.rs +++ b/crates/perry-hir/src/lower/for_head.rs @@ -263,21 +263,20 @@ pub(crate) fn guard_for_in_body( /// Returns `(init_lets, condition, body_prefix)`: /// - `init_lets` — declare the state temps; push BEFORE the `for`. /// - `condition` — replaces `idx < size`. Overwrites `idx_id` with the corrected -/// read index (fast cursor path when no delete shrank the collection; else -/// locate the last key — `+1` after it, or into its vacated slot if it was -/// itself deleted) and yields whether that index is in range. +/// read index (plain cursor while the previously-read key is still at +/// `cursor-1`; else locate the last key — `+1` after it, or into its vacated +/// slot if it was itself deleted) and yields whether that index is in range. /// - `body_prefix` — prepend to the loop body (runs after the in-range check): -/// records the visited key + the size just observed for the next iteration. +/// records the visited key for the next iteration's in-place check. /// -/// `find` is O(1) for numeric/string keys and only called when the collection -/// shrank, so normal / append-only iteration keeps the plain cursor path. +/// `find` is O(1) for numeric/string keys and only called when a shift is +/// detected, so normal / append-only iteration keeps the plain cursor path. pub(crate) fn map_set_delete_safe_for_of( ctx: &mut LoweringContext, arr_id: LocalId, idx_id: LocalId, is_set: bool, ) -> (Vec, Expr, Vec) { - let ls_id = ctx.fresh_local(); // size observed at the previous iteration (-1 = not started) let lk_id = ctx.fresh_local(); // last-returned key let sz_id = ctx.fresh_local(); // current size (spilled once per iteration) let fk_id = ctx.fresh_local(); // find() result @@ -329,16 +328,37 @@ pub(crate) fn map_set_delete_safe_for_of( } }; - // read_idx = (not started || size >= last_seen) ? cursor - // : (j = find(coll, last_key)) >= 0 ? j + 1 : cursor - 1 + // cursor-1 (index of the entry read on the previous iteration). + let prev_idx = |i: Expr| Expr::Binary { + op: BinaryOp::Sub, + left: Box::new(i), + right: Box::new(Expr::Number(1.0)), + }; + + // read_idx = cursor == 0 // not started + // ? cursor + // : key_at(coll, cursor-1) === last_key // last entry still in place + // ? cursor // no shift at/below cursor + // : (j = find(coll, last_key)) >= 0 ? j + 1 : cursor - 1 + // + // Comparing the entry now at cursor-1 to the last-returned key detects ANY + // delete that compacted an entry at/below the cursor — including a delete + // balanced by an add in the same turn (which leaves `size` unchanged), which + // a size-only gate would miss. Map/Set keys are unique under SameValueZero, + // so `===` here is exact except for a NaN key (which just forces the `find` + // path — still correct). let rederive = Expr::Conditional { - condition: Box::new(cmp(CompareOp::Lt, Expr::LocalGet(ls_id), Expr::Number(0.0))), + condition: Box::new(cmp( + CompareOp::Eq, + Expr::LocalGet(idx_id), + Expr::Number(0.0), + )), then_expr: Box::new(Expr::LocalGet(idx_id)), else_expr: Box::new(Expr::Conditional { condition: Box::new(cmp( - CompareOp::Ge, - Expr::LocalGet(sz_id), - Expr::LocalGet(ls_id), + CompareOp::Eq, + key_at(Expr::LocalGet(arr_id), prev_idx(Expr::LocalGet(idx_id))), + Expr::LocalGet(lk_id), )), then_expr: Box::new(Expr::LocalGet(idx_id)), else_expr: Box::new(Expr::Sequence(vec![ @@ -350,21 +370,30 @@ pub(crate) fn map_set_delete_safe_for_of( ])), ), Expr::Conditional { + // A delete only shifts entries down: a merely-shifted last key + // is now below the cursor (`0 <= j < cursor`) → resume after + // it. Deleted (`j < 0`) or deleted-then-re-added at the end + // (`j >= cursor`) → read the entry now in its old slot + // (`cursor - 1`). condition: Box::new(cmp( CompareOp::Ge, Expr::LocalGet(fk_id), Expr::Number(0.0), )), - then_expr: Box::new(Expr::Binary { - op: BinaryOp::Add, - left: Box::new(Expr::LocalGet(fk_id)), - right: Box::new(Expr::Number(1.0)), - }), - else_expr: Box::new(Expr::Binary { - op: BinaryOp::Sub, - left: Box::new(Expr::LocalGet(idx_id)), - right: Box::new(Expr::Number(1.0)), + then_expr: Box::new(Expr::Conditional { + condition: Box::new(cmp( + CompareOp::Lt, + Expr::LocalGet(fk_id), + Expr::LocalGet(idx_id), + )), + then_expr: Box::new(Expr::Binary { + op: BinaryOp::Add, + left: Box::new(Expr::LocalGet(fk_id)), + right: Box::new(Expr::Number(1.0)), + }), + else_expr: Box::new(prev_idx(Expr::LocalGet(idx_id))), }), + else_expr: Box::new(prev_idx(Expr::LocalGet(idx_id))), }, ])), }), @@ -376,13 +405,12 @@ pub(crate) fn map_set_delete_safe_for_of( cmp(CompareOp::Lt, Expr::LocalGet(idx_id), Expr::LocalGet(sz_id)), ]); - let body_prefix = vec![ - Stmt::Expr(Expr::LocalSet( - lk_id, - Box::new(key_at(Expr::LocalGet(arr_id), Expr::LocalGet(idx_id))), - )), - Stmt::Expr(Expr::LocalSet(ls_id, Box::new(Expr::LocalGet(sz_id)))), - ]; + // Record the key just read so the next iteration can check it is still in + // place at cursor-1. + let body_prefix = vec![Stmt::Expr(Expr::LocalSet( + lk_id, + Box::new(key_at(Expr::LocalGet(arr_id), Expr::LocalGet(idx_id))), + ))]; let mk_let = |id: LocalId, tag: &str, ty: Type, init: Expr| Stmt::Let { id, @@ -392,7 +420,6 @@ pub(crate) fn map_set_delete_safe_for_of( init: Some(init), }; let init_lets = vec![ - mk_let(ls_id, "ls", Type::Number, Expr::Number(-1.0)), mk_let(lk_id, "lk", Type::Any, Expr::Undefined), mk_let(sz_id, "sz", Type::Number, Expr::Number(0.0)), mk_let(fk_id, "fk", Type::Number, Expr::Number(0.0)), diff --git a/crates/perry-runtime/src/collection_iter_object.rs b/crates/perry-runtime/src/collection_iter_object.rs index 21f4022be8..1663a60b1f 100644 --- a/crates/perry-runtime/src/collection_iter_object.rs +++ b/crates/perry-runtime/src/collection_iter_object.rs @@ -181,23 +181,29 @@ unsafe fn make_pair_array(a: f64, b: f64) -> f64 { /// Compute the entries-array index to read next, self-correcting for a /// mid-iteration delete. `cursor` = index just past the last-returned entry; -/// `last_seen` = collection size at the previous `next()` (`< 0` ⇒ not started); -/// `size` = current size; `find_last` locates the last-returned key's current -/// index (or `< 0` if it was deleted). +/// `last_key_in_place` = the previously-read key is still at `cursor-1`; +/// `find_last` locates the last-returned key's current index (or `< 0` if it was +/// deleted). /// /// Deleting an entry compacts the backing array (entries after the hole shift /// down one slot, #2831), so a delete at index ≤ cursor would move an unvisited -/// entry below the cursor and skip it. Normal / append-only iteration keeps the -/// fast cursor path (no lookup); only a size DROP re-derives from the last key — -/// so object-keyed maps (whose key lookup is a linear scan) pay nothing unless a -/// delete actually happened. If the last key itself was deleted, the entry that -/// followed it now sits in the vacated slot (`cursor - 1`). (#6075) -fn next_read_index(cursor: u32, last_seen: f64, size: u32, find_last: impl FnOnce() -> i32) -> u32 { - if last_seen < 0.0 || (size as f64) >= last_seen { +/// entry below the cursor and skip it. If the last-returned key is still sitting +/// at `cursor-1`, no such shift happened and the plain cursor is correct — so +/// normal / append-only iteration keeps the fast path and object-keyed maps pay +/// no lookup. Otherwise re-derive from the last key: locate it (`+1` after it), +/// or, if it was itself deleted, read the entry that shifted into its slot +/// (`cursor-1`). Comparing the key (rather than the size) also catches a delete +/// balanced by an add in the same turn. (#6075 / #6165) +fn next_read_index(cursor: u32, last_key_in_place: bool, find_last: impl FnOnce() -> i32) -> u32 { + if cursor == 0 || last_key_in_place { return cursor; } let j = find_last(); - if j >= 0 { + // A delete only shifts entries DOWN, so a last key that merely shifted is now + // below the cursor (`j < cursor`) → resume after it. Otherwise it was deleted + // (`j < 0`) or deleted-then-re-added at the end (`j >= cursor`) — either way + // the entry that shifted into its old slot sits at `cursor-1`. + if j >= 0 && (j as u32) < cursor { (j as u32) + 1 } else { cursor.saturating_sub(1) @@ -215,22 +221,26 @@ pub unsafe fn dispatch_map_iterator_method(iter_obj: *mut ObjectHeader, method_n return make_iter_result(JSValue::undefined(), true); } let cursor = f64::from_bits(js_object_get_field(iter_obj, 1).bits()) as u32; - let last_seen = f64::from_bits(js_object_get_field(iter_obj, 3).bits()); let last_key = js_object_get_field(iter_obj, 4); let size = crate::map::js_map_size(map); - let idx = next_read_index(cursor, last_seen, size, || { + // Is the last-returned key still at cursor-1? (SameValueZero, so a + // NaN key matches itself.) If so, no delete shifted an entry at/below + // the cursor. + let in_place = cursor > 0 && { + let prev = crate::map::js_map_entry_key_at(map, cursor - 1); + crate::value::js_jsvalue_same_value_zero(prev, f64::from_bits(last_key.bits())) != 0 + }; + let idx = next_read_index(cursor, in_place, || { crate::map::find_key_index(map, f64::from_bits(last_key.bits())) }); if idx >= size { js_object_set_field(iter_obj, 1, JSValue::number(size as f64)); - js_object_set_field(iter_obj, 3, JSValue::number(size as f64)); return make_iter_result(JSValue::undefined(), true); } let entry_key = crate::map::js_map_entry_key_at(map, idx); // Record state for the next re-derive BEFORE any allocation below. js_object_set_field(iter_obj, 1, JSValue::number((idx + 1) as f64)); - js_object_set_field(iter_obj, 3, JSValue::number(size as f64)); js_object_set_field(iter_obj, 4, JSValue::from_bits(entry_key.to_bits())); let value = match kind { @@ -262,21 +272,22 @@ pub unsafe fn dispatch_set_iterator_method(iter_obj: *mut ObjectHeader, method_n return make_iter_result(JSValue::undefined(), true); } let cursor = f64::from_bits(js_object_get_field(iter_obj, 1).bits()) as u32; - let last_seen = f64::from_bits(js_object_get_field(iter_obj, 3).bits()); let last_val = js_object_get_field(iter_obj, 4); let size = crate::set::js_set_size(set); - let idx = next_read_index(cursor, last_seen, size, || { + let in_place = cursor > 0 && { + let prev = crate::set::js_set_value_at(set, cursor - 1); + crate::value::js_jsvalue_same_value_zero(prev, f64::from_bits(last_val.bits())) != 0 + }; + let idx = next_read_index(cursor, in_place, || { crate::set::find_value_index(set, f64::from_bits(last_val.bits())) }); if idx >= size { js_object_set_field(iter_obj, 1, JSValue::number(size as f64)); - js_object_set_field(iter_obj, 3, JSValue::number(size as f64)); return make_iter_result(JSValue::undefined(), true); } let elem = crate::set::js_set_value_at(set, idx); js_object_set_field(iter_obj, 1, JSValue::number((idx + 1) as f64)); - js_object_set_field(iter_obj, 3, JSValue::number(size as f64)); js_object_set_field(iter_obj, 4, JSValue::from_bits(elem.to_bits())); let value = match kind {