diff --git a/crates/macros/src/function.rs b/crates/macros/src/function.rs index bcc4865cc..24af3307a 100644 --- a/crates/macros/src/function.rs +++ b/crates/macros/src/function.rs @@ -540,9 +540,11 @@ impl<'a> Args<'a> { .and_then(|ga| match ga { GenericArgument::Type(ty) => Some(match ty { Type::Reference(r) => { + // Only mark as_ref for mutable references + // (Option<&mut T>), not immutable ones (Option<&T>) + as_ref = r.mutability.is_some(); let mut new_ref = r.clone(); new_ref.mutability = None; - as_ref = true; Type::Reference(new_ref) } _ => ty.clone(), diff --git a/src/types/array/mod.rs b/src/types/array/mod.rs index d19bcdebb..a43e024a3 100644 --- a/src/types/array/mod.rs +++ b/src/types/array/mod.rs @@ -5,7 +5,7 @@ use std::{convert::TryFrom, ffi::CString, fmt::Debug, ptr}; use crate::{ boxed::{ZBox, ZBoxable}, - convert::{FromZval, IntoZval}, + convert::{FromZval, FromZvalMut, IntoZval}, error::Result, ffi::zend_ulong, ffi::{ @@ -711,3 +711,11 @@ impl<'a> FromZval<'a> for &'a ZendHashTable { zval.array() } } + +impl<'a> FromZvalMut<'a> for &'a mut ZendHashTable { + const TYPE: DataType = DataType::Array; + + fn from_zval_mut(zval: &'a mut Zval) -> Option { + zval.array_mut() + } +} diff --git a/src/types/zval.rs b/src/types/zval.rs index ac7d734fb..a96310bfa 100644 --- a/src/types/zval.rs +++ b/src/types/zval.rs @@ -14,8 +14,8 @@ use crate::{ error::{Error, Result}, ffi::{ _zval_struct__bindgen_ty_1, _zval_struct__bindgen_ty_2, ext_php_rs_zend_string_release, - zend_is_callable, zend_is_identical, zend_is_iterable, zend_resource, zend_value, zval, - zval_ptr_dtor, + zend_array_dup, zend_is_callable, zend_is_identical, zend_is_iterable, zend_resource, + zend_value, zval, zval_ptr_dtor, }, flags::DataType, flags::ZvalTypeFlags, @@ -224,9 +224,30 @@ impl Zval { /// Returns a mutable reference to the underlying zval hashtable if the zval /// contains an array. + /// + /// # Array Separation + /// + /// PHP arrays use copy-on-write (COW) semantics. Before returning a mutable + /// reference, this method checks if the array is shared (refcount > 1) and + /// if so, creates a private copy. This is equivalent to PHP's + /// `SEPARATE_ARRAY()` macro and prevents the "Assertion failed: + /// `zend_gc_refcount` == 1" error that occurs when modifying shared arrays. pub fn array_mut(&mut self) -> Option<&mut ZendHashTable> { if self.is_array() { - unsafe { self.value.arr.as_mut() } + unsafe { + let arr = self.value.arr; + // Check if the array is shared (refcount > 1) + // If so, we need to separate it (copy-on-write) + if (*arr).gc.refcount > 1 { + // Decrement the refcount of the original array + (*arr).gc.refcount -= 1; + // Duplicate the array to get our own private copy + let new_arr = zend_array_dup(arr); + // Update the zval to point to the new array + self.value.arr = new_arr; + } + self.value.arr.as_mut() + } } else { None } diff --git a/tests/src/integration/array/array.php b/tests/src/integration/array/array.php index 4969e3e9d..68c6532f7 100644 --- a/tests/src/integration/array/array.php +++ b/tests/src/integration/array/array.php @@ -79,3 +79,18 @@ assert(array_key_exists('00', $leading_zeros), '"00" should stay as string key'); assert($leading_zeros['00'] === 'zerozero', 'Value at key "00" should be "zerozero"'); + +// Test Option<&ZendHashTable> with literal array (issue #515) +// This should work without "could not be passed by reference" error +assert(test_optional_array_ref([1, 2, 3]) === 3, 'Option<&ZendHashTable> should accept literal array'); +assert(test_optional_array_ref(null) === -1, 'Option<&ZendHashTable> should accept null'); +$arr = ['a', 'b', 'c', 'd']; +assert(test_optional_array_ref($arr) === 4, 'Option<&ZendHashTable> should accept variable array'); + +// Test Option<&mut ZendHashTable> (anti-regression for issue #515) +$mut_arr = ['x', 'y']; +assert(test_optional_array_mut_ref($mut_arr) === 3, 'Option<&mut ZendHashTable> should accept variable array and add element'); +assert(array_key_exists('added_by_rust', $mut_arr), 'Rust should have added a key to the array'); +assert($mut_arr['added_by_rust'] === 'value', 'Added value should be correct'); +$null_arr = null; +assert(test_optional_array_mut_ref($null_arr) === -1, 'Option<&mut ZendHashTable> should accept null'); diff --git a/tests/src/integration/array/mod.rs b/tests/src/integration/array/mod.rs index b8922a176..2e3f39be2 100644 --- a/tests/src/integration/array/mod.rs +++ b/tests/src/integration/array/mod.rs @@ -5,7 +5,7 @@ use ext_php_rs::{ ffi::HashTable, php_function, prelude::ModuleBuilder, - types::{ArrayKey, Zval}, + types::{ArrayKey, ZendHashTable, Zval}, wrap_function, }; @@ -41,6 +41,25 @@ pub fn test_array_keys() -> Zval { ht.into_zval(false).unwrap() } +/// Test that `Option<&ZendHashTable>` can accept literal arrays (issue #515) +#[php_function] +pub fn test_optional_array_ref(arr: Option<&ZendHashTable>) -> i64 { + arr.map_or(-1, |ht| i64::try_from(ht.len()).unwrap_or(i64::MAX)) +} + +/// Test that `Option<&mut ZendHashTable>` works correctly (anti-regression for issue #515) +#[php_function] +pub fn test_optional_array_mut_ref(arr: Option<&mut ZendHashTable>) -> i64 { + match arr { + Some(ht) => { + // Add an element to verify mutation works + ht.insert("added_by_rust", "value").ok(); + i64::try_from(ht.len()).unwrap_or(i64::MAX) + } + None => -1, + } +} + pub fn build_module(builder: ModuleBuilder) -> ModuleBuilder { builder .function(wrap_function!(test_array)) @@ -48,6 +67,8 @@ pub fn build_module(builder: ModuleBuilder) -> ModuleBuilder { .function(wrap_function!(test_array_assoc_array_keys)) .function(wrap_function!(test_btree_map)) .function(wrap_function!(test_array_keys)) + .function(wrap_function!(test_optional_array_ref)) + .function(wrap_function!(test_optional_array_mut_ref)) } #[cfg(test)]