fix(values): make Value::to_ptr always return the inline representation (#1655)

For memory-allocated types (>=2-variant enums and aggregates containing
one), `Value::to_ptr` returned a *wrapper* pointer (a slot holding the
data pointer) while `Value::from_ptr` always reads the inline
representation. Every recursive consumer had to un-box the wrapper, and
four of six forgot:

- the Array arm copied `elem_layout.size()` bytes out of the 8-byte
  wrapper, so even `Array<bool>` was corrupted (the 16-byte-aligned enum
  body keeps the tag bit clear, decoding every element as variant 0);
- the Felt252Dict arm had the same omission (`Felt252Dict<bool>`);
- the `AbiArgument` Box and Nullable arms memcpy'd the wrapper bytes
  into the heap block (`Box<SomeEnum>`).

Enumerating all callers shows nothing consumes the wrapper — every call
site either stripped it immediately or was one of these bugs — so invert
the contract instead of patching each site: `to_ptr` now always returns
a pointer to the inline representation per `TypeBuilder::layout()`,
exactly what `from_ptr` reads, and the by-pointer ABI decision lives
only in `crate::arch`'s `AbiArgument` impl.

Also make the Felt252Dict arm follow the same convention: it returned
the `FeltDict*` itself instead of a slot holding it, so a dict nested in
an aggregate copied 8 bytes of HashMap internals instead of the dict
pointer. The arch.rs dict arm now dereferences the slot once. The
Nullable arm also now passes the payload type id like the Box arm does.

Only reachable via the `invoke_dynamic(&[Value])` API; the Starknet
contract path marshals felts directly and is unaffected.

Adds a VM-vs-native regression test for `Array<bool>` and
`to_ptr`->`from_ptr` round-trip unit tests (bool array, struct with
bool, nested enum, bool dict) that the new symmetric contract enables.

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

Author: TomerStarkware

src/arch.rs

@@ -15,7 +15,7 @@ use cairo_lang_sierra::{
     ids::ConcreteTypeId,
     program_registry::ProgramRegistry,
 };
-use std::ptr::{null, NonNull};
+use std::ptr::null;
 
 mod aarch64;
 mod x86_64;
@@ -76,7 +76,7 @@ impl AbiArgument for ValueWithInfoWrapper<'_> {
                 if matches!(value, Value::Null) {
                     null::<()>().to_bytes(buffer)?;
                 } else {
-                    let ptr = value.to_ptr(self.arena, self.registry, self.type_id)?;
+                    let ptr = value.to_ptr(self.arena, self.registry, &info.ty)?;
 
                     let layout = self.registry.get_type(&info.ty)?.layout(self.registry)?;
                     let heap_ptr = unsafe {
@@ -126,9 +126,9 @@ impl AbiArgument for ValueWithInfoWrapper<'_> {
             }
             (Value::Enum { tag, value, .. }, CoreTypeConcrete::Enum(info)) => {
                 if self.info.is_memory_allocated(self.registry)? {
+                    // Memory-allocated types are passed by pointer to their inline
+                    // representation.
                     let abi_ptr = self.value.to_ptr(self.arena, self.registry, self.type_id)?;
-
-                    let abi_ptr = unsafe { *abi_ptr.cast::<NonNull<()>>().as_ref() };
                     abi_ptr.as_ptr().to_bytes(buffer)?;
                 } else {
                     match info
@@ -158,10 +158,11 @@ impl AbiArgument for ValueWithInfoWrapper<'_> {
             (Value::Felt252Dict { .. }, CoreTypeConcrete::Felt252Dict(_)) => {
                 // TODO: Assert that `info.ty` matches all the values' types.
 
-                self.value
-                    .to_ptr(self.arena, self.registry, self.type_id)?
-                    .as_ptr()
-                    .to_bytes(buffer)?
+                let ptr = self.value.to_ptr(self.arena, self.registry, self.type_id)?;
+
+                // The dict's inline representation is a slot holding the `FeltDict`
+                // pointer; the ABI passes that pointer by value.
+                unsafe { *ptr.cast::<*mut ()>().as_ref() }.to_bytes(buffer)?
             }
             (
                 Value::Secp256K1Point(Secp256k1Point { x, y, is_infinity }),
@@ -186,9 +187,9 @@ impl AbiArgument for ValueWithInfoWrapper<'_> {
             (Value::Sint8(value), CoreTypeConcrete::Sint8(_)) => value.to_bytes(buffer)?,
             (Value::Struct { fields, .. }, CoreTypeConcrete::Struct(info)) => {
                 if self.info.is_memory_allocated(self.registry)? {
+                    // Memory-allocated types are passed by pointer to their inline
+                    // representation.
                     let abi_ptr = self.value.to_ptr(self.arena, self.registry, self.type_id)?;
-
-                    let abi_ptr = unsafe { *abi_ptr.cast::<NonNull<()>>().as_ref() };
                     abi_ptr.as_ptr().to_bytes(buffer)?;
                 } else {
                     fields

src/values.rs

@@ -157,6 +157,11 @@ impl Value {
     }
 
     /// Allocates the value in the given arena so it can be passed to the JIT engine or a compiled program.
+    ///
+    /// Invariant: the returned pointer always points to the value's inline representation as
+    /// given by [`TypeBuilder::layout`], which is exactly what [`Value::from_ptr`] reads. The
+    /// ABI decision of passing memory-allocated types by pointer is made by the `AbiArgument`
+    /// impl in `crate::arch`, not here.
     pub(crate) fn to_ptr(
         &self,
         arena: &Bump,
@@ -269,7 +274,6 @@ impl Value {
                         let mut layout: Option<Layout> = None;
                         let mut data = Vec::with_capacity(info.members.len());
 
-                        let mut is_memory_allocated = false;
                         for (member_type_id, member) in info.members.iter().zip(members) {
                             let member_ty = registry.get_type(member_type_id)?;
                             let member_layout = member_ty.layout(registry)?;
@@ -281,19 +285,7 @@ impl Value {
                             layout = Some(new_layout);
 
                             let member_ptr = member.to_ptr(arena, registry, member_type_id)?;
-                            data.push((
-                                member_layout,
-                                offset,
-                                if member_ty.is_memory_allocated(registry)? {
-                                    is_memory_allocated = true;
-
-                                    // Undo the wrapper pointer added because the member's memory
-                                    // allocated flag.
-                                    *member_ptr.cast::<NonNull<()>>().as_ref()
-                                } else {
-                                    member_ptr
-                                },
-                            ));
+                            data.push((member_layout, offset, member_ptr));
                         }
 
                         let ptr = arena
@@ -308,12 +300,7 @@ impl Value {
                             );
                         }
 
-                        if is_memory_allocated {
-                            // alloc returns a ref, so its never null
-                            NonNull::new_unchecked(arena.alloc(ptr) as *mut _).cast()
-                        } else {
-                            NonNull::new_unchecked(ptr).cast()
-                        }
+                        NonNull::new_unchecked(ptr).cast()
                     } else {
                         Err(Error::UnexpectedValue(format!(
                             "expected value of type {:?} but got a struct",
@@ -327,18 +314,8 @@ impl Value {
                         native_assert!(*tag < info.variants.len(), "Variant index out of range.");
 
                         let payload_type_id = &info.variants[*tag];
-                        let payload_ty = registry.get_type(payload_type_id)?;
                         let payload = value.to_ptr(arena, registry, payload_type_id)?;
 
-                        // Undo the wrapper pointer added when the payload is memory
-                        // allocated (e.g. a nested >=2-variant enum), so that the copy
-                        // below reads the payload data rather than the wrapper pointer.
-                        let payload = if payload_ty.is_memory_allocated(registry)? {
-                            *payload.cast::<NonNull<()>>().as_ref()
-                        } else {
-                            payload
-                        };
-
                         let (layout, tag_layout, variant_layouts) =
                             crate::types::r#enum::get_layout_for_variants(
                                 registry,
@@ -362,12 +339,7 @@ impl Value {
                             variant_layouts[*tag].size(),
                         );
 
-                        if resolved_ty.is_memory_allocated(registry)? {
-                            // alloc returns a reference so its never null
-                            NonNull::new_unchecked(arena.alloc(ptr) as *mut _).cast()
-                        } else {
-                            NonNull::new_unchecked(ptr).cast()
-                        }
+                        NonNull::new_unchecked(ptr).cast()
                     } else {
                         Err(Error::UnexpectedValue(format!(
                             "expected value of type {:?} but got an enum value",
@@ -417,7 +389,10 @@ impl Value {
                             );
                         }
 
-                        NonNull::new_unchecked(dict_ptr as *mut ()).cast()
+                        // The dict's inline representation is a single pointer to the
+                        // `FeltDict`, so return a slot holding that pointer.
+                        // alloc returns a reference so its never null
+                        NonNull::new_unchecked(arena.alloc(dict_ptr) as *mut _).cast()
                     } else {
                         Err(Error::UnexpectedValue(format!(
                             "expected value of type {:?} but got a felt dict",
@@ -1766,6 +1741,107 @@ mod test {
             _ => panic!("Unexpected error type: {:?}", result),
         }
     }
+
+    /// Helper for the round-trip tests below: a `bool` value (a 2-variant enum
+    /// of unit structs, hence memory-allocated).
+    fn bool_value(value: bool) -> Value {
+        Value::Enum {
+            tag: value as usize,
+            value: Box::new(Value::Struct {
+                fields: Vec::new(),
+                debug_name: None,
+            }),
+            debug_name: None,
+        }
+    }
+
+    /// `to_ptr` returns a pointer to the inline representation, which is
+    /// exactly what `from_ptr` reads — so any value must round-trip.
+    fn assert_roundtrip(program_src: &str, type_idx: usize, value: Value) {
+        let program = ProgramParser::new().parse(program_src).unwrap();
+        let registry = ProgramRegistry::<CoreType, CoreLibfunc>::new(&program).unwrap();
+        let type_id = &program.type_declarations[type_idx].id;
+
+        let arena = Bump::new();
+        let ptr = value.to_ptr(&arena, &registry, type_id).unwrap();
+        let result = Value::from_ptr(ptr, type_id, &registry).unwrap();
+
+        assert_eq!(result, value);
+    }
+
+    #[test]
+    fn test_roundtrip_bool_array() {
+        assert_roundtrip(
+            "type Unit = Struct<ut@Tuple>;
+            type bool = Enum<ut@core::bool, Unit, Unit>;
+            type BoolArray = Array<bool>;",
+            2,
+            Value::Array(vec![
+                bool_value(true),
+                bool_value(false),
+                bool_value(true),
+                bool_value(true),
+            ]),
+        );
+    }
+
+    #[test]
+    fn test_roundtrip_struct_with_bool() {
+        assert_roundtrip(
+            "type u8 = u8;
+            type Unit = Struct<ut@Tuple>;
+            type bool = Enum<ut@core::bool, Unit, Unit>;
+            type MyStruct = Struct<ut@MyStruct, u8, bool>;",
+            3,
+            Value::Struct {
+                fields: vec![Value::Uint8(123), bool_value(true)],
+                debug_name: None,
+            },
+        );
+    }
+
+    #[test]
+    fn test_roundtrip_nested_enum() {
+        let program_src = "type felt252 = felt252;
+            type Inner = Enum<ut@Inner, felt252, felt252>;
+            type Outer = Enum<ut@Outer, Inner, Inner>;";
+
+        for (outer_tag, inner_tag) in [(0, 0), (0, 1), (1, 0), (1, 1)] {
+            assert_roundtrip(
+                program_src,
+                2,
+                Value::Enum {
+                    tag: outer_tag,
+                    value: Box::new(Value::Enum {
+                        tag: inner_tag,
+                        value: Box::new(Value::Felt252(Felt::from(0x1234))),
+                        debug_name: None,
+                    }),
+                    debug_name: None,
+                },
+            );
+        }
+    }
+
+    #[test]
+    fn test_roundtrip_bool_dict() {
+        assert_roundtrip(
+            "type Unit = Struct<ut@Tuple>;
+            type bool = Enum<ut@core::bool, Unit, Unit>;
+            type BoolDict = Felt252Dict<bool>;",
+            2,
+            Value::Felt252Dict {
+                value: [
+                    (Felt::from(0), bool_value(true)),
+                    (Felt::from(1), bool_value(false)),
+                    (Felt::from(2), bool_value(true)),
+                ]
+                .into_iter()
+                .collect(),
+                debug_name: None,
+            },
+        );
+    }
 }
 
 mod range_serde {

test_data/programs/array_bool_arg.cairo

@@ -0,0 +1,9 @@
+fn run_test(data: Array<bool>) -> u32 {
+    let mut count: u32 = 0;
+    for value in data {
+        if value {
+            count += 1;
+        }
+    }
+    count
+}

tests/tests/enums.rs

@@ -99,3 +99,55 @@ fn nested_enum_argument_matches_vm() {
         });
     }
 }
+
+#[test]
+fn bool_array_argument_matches_vm() {
+    // `bool` is a 2-variant enum, so it is memory-allocated and `to_ptr` must
+    // un-wrap each element before copying it into the array's data buffer. The
+    // program counts the `true` elements, so any corruption changes the count.
+    let program = &load_program_and_runner("programs/array_bool_arg");
+
+    // The VM tag of a 2-variant enum equals the variant index.
+    let values = [true, false, true, true, false];
+
+    let result_vm = run_vm_program(
+        program,
+        "run_test",
+        vec![Arg::Array(
+            values
+                .iter()
+                .map(|&v| Arg::Value(Felt::from(v as u64)))
+                .collect(),
+        )],
+        Some(DEFAULT_GAS as usize),
+    )
+    .unwrap();
+
+    let result_native = run_native_program(
+        program,
+        "run_test",
+        &[Value::Array(
+            values
+                .iter()
+                .map(|&v| Value::Enum {
+                    tag: v as usize,
+                    value: Box::new(Value::Struct {
+                        fields: Vec::new(),
+                        debug_name: None,
+                    }),
+                    debug_name: None,
+                })
+                .collect(),
+        )],
+        Some(DEFAULT_GAS),
+        Option::<DummySyscallHandler>::None,
+    );
+
+    compare_outputs(
+        &program.1,
+        &program.2.find_function("run_test").unwrap().id,
+        &result_vm,
+        &result_native,
+    )
+    .expect("bool array argument must agree between VM and native");
+}