Skip to content

fix(runtime,lower): Map/Set delete-during-iteration gates on key position, not size (#6165) #6166

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
proggeramlug merged 1 commit into from
Jul 9, 2026
Merged

fix(runtime,lower): Map/Set delete-during-iteration gates on key position, not size (#6165) #6166

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
87 changes: 57 additions & 30 deletions crates/perry-hir/src/lower/for_head.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Stmt>, Expr, Vec<Stmt>) {
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
Expand Down Expand Up @@ -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![
Expand All @@ -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))),
},
])),
}),
Expand All @@ -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,
Expand All @@ -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)),
Expand Down
49 changes: 30 additions & 19 deletions crates/perry-runtime/src/collection_iter_object.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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 {
Expand Down Expand Up @@ -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 {
Expand Down
Loading