navicore/patch-seq

Fork 0

variant.append (crates/runtime/src/variant_ops.rs): #380

Merged

navicore merged 2 commits from arc-migration-done into main

2026-03-29 01:53:12 +00:00

navicore commented

2026-03-28 23:12:57 +00:00

(Migrated from github.com)

Fast path via peek_heap_mut_second — variant at sp-2, same shape as list.push. Pops value, mutates fields in place, returns without touching the variant's stack slot.

list.set (crates/runtime/src/list_ops.rs):

Fast path via heap_value_mut(stack.sub(3)) — list at sp-3. Peeks at the index inline (it's a tagged int, no Arc involved) to validate bounds before popping. If sole-owned and in-bounds, mutates in place and pushes only the Bool result.

Results: 944 unit tests, 443 integration tests, 0 clippy warnings, fmt clean.

That completes all four candidates from the migration doc: list.push, map.set, map.remove, variant.append, and list.set.

- Fast path via peek_heap_mut_second — variant at sp-2, same shape as list.push. Pops value, mutates fields in place, returns without touching the variant's stack slot. list.set (crates/runtime/src/list_ops.rs): - Fast path via heap_value_mut(stack.sub(3)) — list at sp-3. Peeks at the index inline (it's a tagged int, no Arc involved) to validate bounds before popping. If sole-owned and in-bounds, mutates in place and pushes only the Bool result. Results: 944 unit tests, 443 integration tests, 0 clippy warnings, fmt clean. That completes all four candidates from the migration doc: list.push, map.set, map.remove, variant.append, and list.set.

claude[bot] commented

2026-03-28 23:21:14 +00:00

(Migrated from github.com)

Overall this is a well-structured optimization. The safety reasoning is sound and the fast/slow paths preserve identical stack semantics.

variant_ops.rs - variant.append fast path

No stack-depth guard before peek_heap_mut_second. The slow path also lacks one, so this is consistent, but a malformed program reaching this with fewer than 2 stack values would go out of bounds silently. A debug-mode assert_depth could help.

Return value: fast path returns stack after one pop, Variant ends up at new sp-1. Matches the slow path. Correct.

list_ops.rs - list.set fast path

Double Arc::get_mut is correct. Both Arc (heap_value_mut) and Arc (Arc::get_mut(variant_arc)) must show sole ownership before mutating. This is right.

Live data: &mut VariantData across pop calls. The safety claim - pop only touches sp-1/sp-2 while data lives in the Arc at sp-3 - is correct. Worth keeping the comment current if pop internals change.

Out-of-bounds fast path: after two pops, sp = old sp-2. Variant untouched at old sp-3. push(Bool(false)) at old sp-2. New sp = old sp-1. Result: ( -- Variant Bool(false) ), matches slow path. Correct.

Nits

Comment removals are fine, remaining code is self-evident. list.set doc says Stack must have Value on top, Int below, and Variant below that, but the fast path now inlines a peek at the Int without popping it. A brief doc note would align the docs with the implementation.

Test coverage

Worth confirming coverage of: (1) list.set with a shared Arc (refcount > 1 for Arc or Arc) to exercise the slow COW clone path; (2) variant.append on a shared Variant for the same slow path; (3) list.set out-of-bounds in the fast path (tagged-int index, valid Variant, index >= len).

Summary

Core logic is correct, safety invariants are well-stated, optimization is meaningful. Main asks: verify OOB and shared-Arc test cases are covered, and optionally update the list.set doc to mention the inline index peek.

Overall this is a well-structured optimization. The safety reasoning is sound and the fast/slow paths preserve identical stack semantics. **variant_ops.rs - variant.append fast path** No stack-depth guard before peek_heap_mut_second. The slow path also lacks one, so this is consistent, but a malformed program reaching this with fewer than 2 stack values would go out of bounds silently. A debug-mode assert_depth could help. Return value: fast path returns stack after one pop, Variant ends up at new sp-1. Matches the slow path. Correct. **list_ops.rs - list.set fast path** Double Arc::get_mut is correct. Both Arc<Value> (heap_value_mut) and Arc<VariantData> (Arc::get_mut(variant_arc)) must show sole ownership before mutating. This is right. Live data: &mut VariantData across pop calls. The safety claim - pop only touches sp-1/sp-2 while data lives in the Arc at sp-3 - is correct. Worth keeping the comment current if pop internals change. Out-of-bounds fast path: after two pops, sp = old sp-2. Variant untouched at old sp-3. push(Bool(false)) at old sp-2. New sp = old sp-1. Result: ( -- Variant Bool(false) ), matches slow path. Correct. **Nits** Comment removals are fine, remaining code is self-evident. list.set doc says Stack must have Value on top, Int below, and Variant below that, but the fast path now inlines a peek at the Int without popping it. A brief doc note would align the docs with the implementation. **Test coverage** Worth confirming coverage of: (1) list.set with a shared Arc (refcount > 1 for Arc<Value> or Arc<VariantData>) to exercise the slow COW clone path; (2) variant.append on a shared Variant for the same slow path; (3) list.set out-of-bounds in the fast path (tagged-int index, valid Variant, index >= len). **Summary** Core logic is correct, safety invariants are well-stated, optimization is meaningful. Main asks: verify OOB and shared-Arc test cases are covered, and optionally update the list.set doc to mention the inline index peek.

claude[bot] commented

2026-03-29 01:44:36 +00:00

(Migrated from github.com)

Code Review

(Note: A prior Claude review was posted 2026-03-28; this is a follow-up pass.)

Overall

The optimizations are correct and the pattern is consistent with the already-merged list.push, map.set, map.remove fast paths. Safety reasoning is well-documented. A few observations below.

`variant_ops.rs` — `variant.append` fast path

Correct. peek_heap_mut_second borrows the Variant at sp-2, pop reads sp-1 only (adjusts sp by 1), so the data reference into the Arc heap remains valid across the pop. Stack result: Variant stays at sp-1, value appended in place. Matches slow-path semantics.

The sole-owner check still happens in the slow path's match arm, so shared-Arc Variants still clone correctly.

`list_ops.rs` — `list.set` fast path

Correct overall, with one structural observation worth calling out:

Out-of-bounds early return is gated inside Arc::get_mut

if let Some(Value::Variant(variant_arc)) = heap_value_mut(stack.sub(3))
    && let Some(data) = Arc::get_mut(variant_arc)
{
    if is_tagged_int(index_sv) {
        let index = untag_int(index_sv);
        if in_bounds { /* mutate, return */ }
        // OOB path — but only reached if Arc is sole-owned
        pop; pop; return push(Bool(false));
    }
}
// slow path handles all other cases

If the list Arc is shared and the index is out-of-bounds, the fast path falls through to the slow path unnecessarily — the slow path then pops all three values, clones the Arc (needlessly), and returns false. The clone is wasted work. This is not a bug, but it's a mild missed optimization and a potential source of confusion: the OOB fast path only fires for sole-owned variants. A comment acknowledging this or restructuring to hoist the OOB check outside the Arc::get_mut guard would help.

data borrow live across pops in the OOB branch

In the OOB branch, data: &mut VariantData is technically still in scope when the two pop calls happen. The safety holds — pop only decrements sp and reads the top slot; it never touches Arc heap memory — but it creates a pattern where a mutable borrow is live across pointer-manipulating operations. Consider dropping data before the pops (it isn't used in the OOB branch) or adding a comment noting this explicitly:

if index >= 0 && (index as usize) < data.fields.len() {
    // in-bounds: use data
} else {
    drop(data); // no longer needed; pops below don't touch Arc heap
    let (stack, _) = pop(stack);
    let (stack, _) = pop(stack);
    return push(stack, Value::Bool(false));
}

This is a style/clarity point, not a correctness issue in unsafe code.

Test coverage

The PR reports 944 unit + 443 integration tests pass. Two cases worth confirming are explicitly covered:

list.set with a shared Arc<VariantData> (refcount > 1) to exercise the slow-path COW clone — ensures the fast path doesn't accidentally mutate shared state.
list.set with a shared Arc and an out-of-bounds index — currently hits the slow path; verify it returns (list, false) without panicking or cloning unnecessarily.

If these are already covered by existing integration tests, the PR is good to merge.

Minor

Removed comments (// Return the list and false, // COW: if we're the sole owner, // Shared — clone and replace, // Out of bounds - return original list and false) are fine to drop — the code is self-evident without them.
list.set doc comment still says "returns a new list" in the first sentence of the new summary. The updated summary block is good, but the one-liner at the top ("Set an element in a list by index with COW optimization") no longer says "returns new list", which is correct. No action needed.

## Code Review *(Note: A prior Claude review was posted 2026-03-28; this is a follow-up pass.)* ### Overall The optimizations are correct and the pattern is consistent with the already-merged `list.push`, `map.set`, `map.remove` fast paths. Safety reasoning is well-documented. A few observations below. --- ### `variant_ops.rs` — `variant.append` fast path **Correct.** `peek_heap_mut_second` borrows the `Variant` at `sp-2`, `pop` reads `sp-1` only (adjusts sp by 1), so the `data` reference into the Arc heap remains valid across the pop. Stack result: Variant stays at `sp-1`, value appended in place. Matches slow-path semantics. The sole-owner check still happens in the slow path's match arm, so shared-Arc Variants still clone correctly. --- ### `list_ops.rs` — `list.set` fast path **Correct overall**, with one structural observation worth calling out: **Out-of-bounds early return is gated inside `Arc::get_mut`** ```rust if let Some(Value::Variant(variant_arc)) = heap_value_mut(stack.sub(3)) && let Some(data) = Arc::get_mut(variant_arc) { if is_tagged_int(index_sv) { let index = untag_int(index_sv); if in_bounds { /* mutate, return */ } // OOB path — but only reached if Arc is sole-owned pop; pop; return push(Bool(false)); } } // slow path handles all other cases ``` If the list Arc is *shared* and the index is out-of-bounds, the fast path falls through to the slow path unnecessarily — the slow path then pops all three values, clones the Arc (needlessly), and returns `false`. The clone is wasted work. This is not a **bug**, but it's a mild missed optimization and a potential source of confusion: the OOB fast path only fires for sole-owned variants. A comment acknowledging this or restructuring to hoist the OOB check outside the `Arc::get_mut` guard would help. **`data` borrow live across pops in the OOB branch** In the OOB branch, `data: &mut VariantData` is technically still in scope when the two `pop` calls happen. The safety holds — `pop` only decrements `sp` and reads the top slot; it never touches Arc heap memory — but it creates a pattern where a mutable borrow is live across pointer-manipulating operations. Consider dropping `data` before the pops (it isn't used in the OOB branch) or adding a comment noting this explicitly: ```rust if index >= 0 && (index as usize) < data.fields.len() { // in-bounds: use data } else { drop(data); // no longer needed; pops below don't touch Arc heap let (stack, _) = pop(stack); let (stack, _) = pop(stack); return push(stack, Value::Bool(false)); } ``` This is a style/clarity point, not a correctness issue in `unsafe` code. --- ### Test coverage The PR reports 944 unit + 443 integration tests pass. Two cases worth confirming are explicitly covered: 1. `list.set` with a shared `Arc<VariantData>` (refcount > 1) to exercise the slow-path COW clone — ensures the fast path doesn't accidentally mutate shared state. 2. `list.set` with a *shared* Arc and an out-of-bounds index — currently hits the slow path; verify it returns `(list, false)` without panicking or cloning unnecessarily. If these are already covered by existing integration tests, the PR is good to merge. --- ### Minor - Removed comments (`// Return the list and false`, `// COW: if we're the sole owner`, `// Shared — clone and replace`, `// Out of bounds - return original list and false`) are fine to drop — the code is self-evident without them. - `list.set` doc comment still says "returns a new list" in the first sentence of the new summary. The updated summary block is good, but the one-liner at the top ("Set an element in a list by index with COW optimization") no longer says "returns new list", which is correct. No action needed.