Fix marshal recursive reference loading - #8501

crates/compiler-core/src/marshal.rs (3)

962-988: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider unifying the two dict read loops.

Both branches read the terminator byte, the key, and the value with identical code. Only the final action differs. You can read the pairs once and choose the action with a small closure or by collecting into the placeholder when it exists.

This keeps the terminator handling in one place, so a future change to the b'0' sentinel cannot diverge between the two paths.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/compiler-core/src/marshal.rs` around lines 962 - 988, Unify the
duplicated dictionary-deserialization loops in the surrounding function by
reading the terminator, key, and value once, then either inserting into the
existing placeholder or collecting pairs for bag.make_dict. Preserve placeholder
registration in refs and the existing b'0' termination behavior.

---

859-878: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider collapsing the four string arms.

The four arms differ only in the length width and in the interned flag. You can extract those two values and call one shared path.

The coding guidelines require this pattern: "When branches differ only in a value but share common logic, extract the differing value and call the common logic once."

♻️ Proposed refactor

-        Type::Ascii | Type::Unicode => {
-            let len = rdr.read_u32()?;
-            let value = rdr.read_wtf8(len)?;
-            bag.make_str(value)
-        }
-        Type::AsciiInterned | Type::Interned => {
-            let len = rdr.read_u32()?;
-            let value = rdr.read_wtf8(len)?;
-            bag.make_interned_str(value)
-        }
-        Type::ShortAscii => {
-            let len = rdr.read_u8()? as u32;
-            let value = rdr.read_wtf8(len)?;
-            bag.make_str(value)
-        }
-        Type::ShortAsciiInterned => {
-            let len = rdr.read_u8()? as u32;
-            let value = rdr.read_wtf8(len)?;
-            bag.make_interned_str(value)
-        }
+        Type::Ascii
+        | Type::Unicode
+        | Type::AsciiInterned
+        | Type::Interned
+        | Type::ShortAscii
+        | Type::ShortAsciiInterned => {
+            let short = matches!(typ, Type::ShortAscii | Type::ShortAsciiInterned);
+            let interned = matches!(
+                typ,
+                Type::AsciiInterned | Type::Interned | Type::ShortAsciiInterned
+            );
+            let len = if short {
+                rdr.read_u8()? as u32
+            } else {
+                rdr.read_u32()?
+            };
+            let value = rdr.read_wtf8(len)?;
+            if interned {
+                bag.make_interned_str(value)
+            } else {
+                bag.make_str(value)
+            }
+        }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/compiler-core/src/marshal.rs` around lines 859 - 878, Refactor the
string-handling match arms in the unmarshalling logic to extract the length
width and interned flag, then run the shared read_wtf8 and bag construction path
once. Preserve u32 lengths for Type::Ascii and Type::Unicode, u8 lengths for
Type::ShortAscii variants, and route interned types through
bag.make_interned_str while other types use bag.make_str.

Source: Coding guidelines

---

879-894: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the shared tuple decoding path.

The Type::SmallTuple and Type::Tuple arms are identical except for the length read width. Extract a helper that takes len and performs the placeholder-or-collect logic once.

The coding guidelines require this pattern: "When branches differ only in a value but share common logic, extract the differing value and call the common logic once."

♻️ Sketch of the shared helper

fn read_tuple_body<R: Read, Bag: MarshalBag>(
    rdr: &mut R,
    bag: Bag,
    depth: usize,
    refs: &mut Vec<Option<Bag::Value>>,
    slot: Option<usize>,
    len: usize,
) -> Result<Bag::Value> {
    let d = depth - 1;
    if let Some(index) = slot
        && let Some(tuple) = bag.make_tuple_placeholder(len)
    {
        refs[index] = Some(tuple.clone());
        for item_index in 0..len {
            let item = deserialize_value_depth(rdr, bag, d, refs)?;
            bag.set_tuple_item(&tuple, item_index, item)?;
        }
        Ok(tuple)
    } else {
        let it = (0..len).map(|_| deserialize_value_depth(rdr, bag, d, refs));
        itertools::process_results(it, |it| bag.make_tuple(it))
    }
}

Then each arm reads its length and calls the helper.

Also applies to: 903-919

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/compiler-core/src/marshal.rs` around lines 879 - 894, Extract the
duplicated tuple decoding logic from the Type::SmallTuple and Type::Tuple arms
into a shared read_tuple_body helper that accepts the already-read len and
preserves the placeholder, refs, recursive item decoding, and collected tuple
paths. Have each arm only read its length using its respective width, then call
the helper and return its Result without duplicating the body.

Source: Coding guidelines

crates/vm/src/stdlib/marshal.rs (1)

479-490: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Replace the panicking index with a checked write.

borrow_vec_mut()[index] = value panics if index is out of range. The current decoder always calls this with index from 0..len, and len matches the placeholder length, so the panic is unreachable today.

This decoder processes untrusted bytes. A panic aborts the interpreter instead of raising a Python exception. A checked write converts any future contract drift into a BadType marshal error, which deserialize_value already maps to ValueError("bad marshal data").

🛡️ Proposed change

             let list = list
                 .downcast_ref::<PyList>()
                 .ok_or(marshal::MarshalError::BadType)?;
-            list.borrow_vec_mut()[index] = value;
-            Ok(())
+            *list
+                .borrow_vec_mut()
+                .get_mut(index)
+                .ok_or(marshal::MarshalError::BadType)? = value;
+            Ok(())

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/vm/src/stdlib/marshal.rs` around lines 479 - 490, Update set_list_item
to perform a checked write when assigning the decoded value, returning
marshal::MarshalError::BadType if index is outside the list bounds instead of
panicking. Preserve the existing PyList downcast validation and successful
in-range assignment behavior.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes