From 60d4d13f2c9022425e6db4837344224800cd3b64 Mon Sep 17 00:00:00 2001 From: Brett McBride Date: Fri, 29 Aug 2025 14:47:33 +1000 Subject: [PATCH 1/2] fix invocation of functions with return type hint Previously, storing "mysterious code" in common.arg_info was ok if nothing else was in arg_info. However, when a type-hint is registered against a function, the slot in arg_info is clobbered by the return type info. Previous tests for adding type-hints appeared correct because they were only inspecting the Reflection of the function, which appeared correct. Only when the function was invoked did it fail to locate the mysterious code, and assume the function was a method and could be found in globals().handler_map (which would panic because function handlers were not globally registered). To fix this, remove mysterious code entirely, and store function handlers globally, alongside methods and enums. --- phper-test/src/cli.rs | 2 + phper/src/functions.rs | 45 ++--------------------- phper/src/modules.rs | 7 ++++ tests/integration/src/typehints.rs | 26 ++++++++++--- tests/integration/tests/php/typehints.php | 25 ++++++++++++- 5 files changed, 58 insertions(+), 47 deletions(-) diff --git a/phper-test/src/cli.rs b/phper-test/src/cli.rs index 4f2d34f..d314756 100644 --- a/phper-test/src/cli.rs +++ b/phper-test/src/cli.rs @@ -179,6 +179,8 @@ pub fn test_php_scripts_with_condition( "execute php test command"); if !condition(output) { + eprintln!("--- stdout ---\n{}", stdout); + eprintln!("--- stderr ---\n{}", stderr); panic!("test php file `{}` failed", path); } } diff --git a/phper/src/functions.rs b/phper/src/functions.rs index d13e113..498568b 100644 --- a/phper/src/functions.rs +++ b/phper/src/functions.rs @@ -28,16 +28,11 @@ use std::{ collections::HashMap, ffi::{CStr, CString}, marker::PhantomData, - mem::{ManuallyDrop, size_of, transmute, zeroed}, + mem::{ManuallyDrop, transmute, zeroed}, ptr::{self, null_mut}, rc::Rc, - slice, }; -/// Used to mark the arguments obtained by the invoke function as mysterious -/// codes from phper -const INVOKE_MYSTERIOUS_CODE: &[u8] = b"PHPER"; - /// Used to find the handler in the invoke function. pub(crate) type HandlerMap = HashMap<(Option, CString), Rc>; @@ -371,9 +366,6 @@ impl FunctionEntry { infos.push(zeroed::()); - // Will be checked in `invoke` function. - infos.push(Self::create_mysterious_code()); - let raw_handler = handler.as_ref().map(|_| invoke as _); if let Some(handler) = handler { @@ -397,22 +389,12 @@ impl FunctionEntry { } } } - - unsafe fn create_mysterious_code() -> zend_internal_arg_info { - unsafe { - let mut mysterious_code = [0u8; size_of::()]; - for (i, n) in INVOKE_MYSTERIOUS_CODE.iter().enumerate() { - mysterious_code[i] = *n; - } - transmute(mysterious_code) - } - } } /// Builder for registering php function. pub struct FunctionEntity { - name: CString, - handler: Rc, + pub(crate) name: CString, + pub(crate) handler: Rc, arguments: Vec, return_type: Option, } @@ -797,7 +779,6 @@ impl ZFunc { pub(crate) union CallableTranslator { pub(crate) callable: *const dyn Callable, pub(crate) internal_arg_info: zend_internal_arg_info, - pub(crate) arg_info: zend_arg_info, } /// The entry for all registered PHP functions. @@ -806,25 +787,7 @@ unsafe extern "C" fn invoke(execute_data: *mut zend_execute_data, return_value: let execute_data = ExecuteData::from_mut_ptr(execute_data); let return_value = ZVal::from_mut_ptr(return_value); - let num_args = execute_data.common_num_args(); - let arg_info = execute_data.common_arg_info(); - - // should be mysterious code - let mysterious_arg_info = arg_info.offset((num_args + 1) as isize); - let mysterious_code = slice::from_raw_parts( - mysterious_arg_info as *const u8, - INVOKE_MYSTERIOUS_CODE.len(), - ); - - let handler = if mysterious_code == INVOKE_MYSTERIOUS_CODE { - // hiddden real handler - let last_arg_info = arg_info.offset((num_args + 2) as isize); - let translator = CallableTranslator { - arg_info: *last_arg_info, - }; - let handler = translator.callable; - handler.as_ref().expect("handler is null") - } else { + let handler = { let function_name = execute_data .func() .get_function_name() diff --git a/phper/src/modules.rs b/phper/src/modules.rs index 5c31f54..da76702 100644 --- a/phper/src/modules.rs +++ b/phper/src/modules.rs @@ -55,6 +55,13 @@ unsafe extern "C" fn module_startup(_type: c_int, module_number: c_int) -> c_int interface_entity.init(); } + for function_entity in &module.function_entities { + module.handler_map.insert( + (None, function_entity.name.clone()), + function_entity.handler.clone(), + ); + } + for class_entity in &module.class_entities { let ce = class_entity.init(); class_entity.declare_properties(ce); diff --git a/tests/integration/src/typehints.rs b/tests/integration/src/typehints.rs index 99f0f24..b68318a 100644 --- a/tests/integration/src/typehints.rs +++ b/tests/integration/src/typehints.rs @@ -9,12 +9,14 @@ // See the Mulan PSL v2 for more details. use phper::{ + arrays::ZArray, classes::{ClassEntity, Interface, InterfaceEntity, StateClass, Visibility}, functions::{Argument, ReturnType}, modules::Module, types::{ArgumentTypeHint, ReturnTypeHint}, values::ZVal, }; +use std::convert::Infallible; const I_FOO: &str = r"IntegrationTest\TypeHints\IFoo"; @@ -26,6 +28,20 @@ pub fn integrate(module: &mut Module) { module.add_class(make_arg_typehint_class()); module.add_class(make_return_typehint_class()); module.add_class(make_arg_default_value_class()); + module.add_function("integration_function_return_bool", |_| -> Result { Ok(true)} ) + .return_type(ReturnType::new(ReturnTypeHint::Bool)); + module.add_function("integration_function_return_int", |_| -> Result { Ok(42)} ) + .return_type(ReturnType::new(ReturnTypeHint::Int)); + module.add_function("integration_function_return_float", |_| -> Result { Ok(3.14)} ) + .return_type(ReturnType::new(ReturnTypeHint::Float)); + module.add_function("integration_function_return_string", |_| -> Result<&'static str, Infallible> { Ok("phper")} ) + .return_type(ReturnType::new(ReturnTypeHint::String)); + module.add_function("integration_function_return_array", |_| -> Result { Ok(ZArray::new()) } ) + .return_type(ReturnType::new(ReturnTypeHint::Array)); + module.add_function("integration_function_return_mixed", |_| -> Result { Ok(ZVal::from(1.23))} ) + .return_type(ReturnType::new(ReturnTypeHint::Mixed)); + module.add_function("integration_function_return_void", |_| phper::ok(())) + .return_type(ReturnType::new(ReturnTypeHint::Void)); module .add_function("integration_function_typehints", |_| phper::ok(())) .argument( @@ -458,7 +474,7 @@ fn make_return_typehint_class() -> ClassEntity<()> { .add_method( "returnString", Visibility::Public, - move |_, _| phper::ok(()), + move |_, _| phper::ok("phper"), ) .return_type(ReturnType::new(ReturnTypeHint::String)); @@ -469,7 +485,7 @@ fn make_return_typehint_class() -> ClassEntity<()> { .return_type(ReturnType::new(ReturnTypeHint::String).allow_null()); class - .add_method("returnBool", Visibility::Public, move |_, _| phper::ok(())) + .add_method("returnBool", Visibility::Public, move |_, _| phper::ok(true)) .return_type(ReturnType::new(ReturnTypeHint::Bool)); class @@ -479,7 +495,7 @@ fn make_return_typehint_class() -> ClassEntity<()> { .return_type(ReturnType::new(ReturnTypeHint::Bool).allow_null()); class - .add_method("returnInt", Visibility::Public, move |_, _| phper::ok(())) + .add_method("returnInt", Visibility::Public, move |_, _| phper::ok(42)) .return_type(ReturnType::new(ReturnTypeHint::Int)); class @@ -489,7 +505,7 @@ fn make_return_typehint_class() -> ClassEntity<()> { .return_type(ReturnType::new(ReturnTypeHint::Int).allow_null()); class - .add_method("returnFloat", Visibility::Public, move |_, _| phper::ok(())) + .add_method("returnFloat", Visibility::Public, move |_, _| phper::ok(3.14)) .return_type(ReturnType::new(ReturnTypeHint::Float)); class @@ -499,7 +515,7 @@ fn make_return_typehint_class() -> ClassEntity<()> { .return_type(ReturnType::new(ReturnTypeHint::Float).allow_null()); class - .add_method("returnArray", Visibility::Public, move |_, _| phper::ok(())) + .add_method("returnArray", Visibility::Public, move |_, _| phper::ok(ZArray::new())) .return_type(ReturnType::new(ReturnTypeHint::Array)); class diff --git a/tests/integration/tests/php/typehints.php b/tests/integration/tests/php/typehints.php index 1aa07c6..e36bdb5 100644 --- a/tests/integration/tests/php/typehints.php +++ b/tests/integration/tests/php/typehints.php @@ -209,4 +209,27 @@ public function setValue($value): void { echo "PASS" . PHP_EOL; } assert_eq('void', $reflection->getReturnType()->getName(), 'integration_function_typehints return type is void'); -} \ No newline at end of file +} + +//invoke type-hinted functions to exercise handlers +echo PHP_EOL . 'Testing return type-hinted function invocation' . PHP_EOL; +assert_true(integration_function_return_bool()); +assert_eq(42, integration_function_return_int()); +assert_eq(3.14, integration_function_return_float()); +assert_eq('phper', integration_function_return_string()); +assert_eq(array(), integration_function_return_array()); +assert_eq(1.23, integration_function_return_mixed()); + +//invoke type-hinted class methods to exercise handlers +echo PHP_EOL . 'Testing return type-hinted method invocation' . PHP_EOL; +$cls = new \IntegrationTest\TypeHints\ReturnTypeHintTest(); +assert_eq(true, $cls->returnBool(), 'returnBool'); +assert_eq(null, $cls->returnBoolNullable(), 'returnBoolNullable'); +assert_eq(42, $cls->returnInt(), 'returnInt'); +assert_eq(null, $cls->returnIntNullable(), 'returnIntNullable'); +assert_eq(3.14, $cls->returnFloat(), 'returnFloat'); +assert_eq(null, $cls->returnFloatNullable(), 'returnFloatNullable'); +assert_eq('phper', $cls->returnString(), 'returnString'); +assert_eq(null, $cls->returnStringNullable(), 'returnStringNullable'); +assert_eq(array(), $cls->returnArray(), 'returnArray'); +assert_eq(null, $cls->returnArrayNullable(), 'returnArrayNullable'); From 85ff663593df5ba40c5170493e27d03ac5048946 Mon Sep 17 00:00:00 2001 From: Brett McBride Date: Fri, 29 Aug 2025 15:01:13 +1000 Subject: [PATCH 2/2] fmt, clippy --- tests/integration/src/typehints.rs | 59 +++++++++++++++++------ tests/integration/tests/php/typehints.php | 4 +- 2 files changed, 46 insertions(+), 17 deletions(-) diff --git a/tests/integration/src/typehints.rs b/tests/integration/src/typehints.rs index b68318a..93b830c 100644 --- a/tests/integration/src/typehints.rs +++ b/tests/integration/src/typehints.rs @@ -28,19 +28,44 @@ pub fn integrate(module: &mut Module) { module.add_class(make_arg_typehint_class()); module.add_class(make_return_typehint_class()); module.add_class(make_arg_default_value_class()); - module.add_function("integration_function_return_bool", |_| -> Result { Ok(true)} ) + module + .add_function( + "integration_function_return_bool", + |_| -> Result { Ok(true) }, + ) .return_type(ReturnType::new(ReturnTypeHint::Bool)); - module.add_function("integration_function_return_int", |_| -> Result { Ok(42)} ) + module + .add_function( + "integration_function_return_int", + |_| -> Result { Ok(42) }, + ) .return_type(ReturnType::new(ReturnTypeHint::Int)); - module.add_function("integration_function_return_float", |_| -> Result { Ok(3.14)} ) + module + .add_function( + "integration_function_return_float", + |_| -> Result { Ok(1.234) }, + ) .return_type(ReturnType::new(ReturnTypeHint::Float)); - module.add_function("integration_function_return_string", |_| -> Result<&'static str, Infallible> { Ok("phper")} ) + module + .add_function( + "integration_function_return_string", + |_| -> Result<&'static str, Infallible> { Ok("phper") }, + ) .return_type(ReturnType::new(ReturnTypeHint::String)); - module.add_function("integration_function_return_array", |_| -> Result { Ok(ZArray::new()) } ) + module + .add_function( + "integration_function_return_array", + |_| -> Result { Ok(ZArray::new()) }, + ) .return_type(ReturnType::new(ReturnTypeHint::Array)); - module.add_function("integration_function_return_mixed", |_| -> Result { Ok(ZVal::from(1.23))} ) + module + .add_function( + "integration_function_return_mixed", + |_| -> Result { Ok(ZVal::from(1.23)) }, + ) .return_type(ReturnType::new(ReturnTypeHint::Mixed)); - module.add_function("integration_function_return_void", |_| phper::ok(())) + module + .add_function("integration_function_return_void", |_| phper::ok(())) .return_type(ReturnType::new(ReturnTypeHint::Void)); module .add_function("integration_function_typehints", |_| phper::ok(())) @@ -471,11 +496,9 @@ fn make_return_typehint_class() -> ClassEntity<()> { .return_type(ReturnType::new(ReturnTypeHint::Null)); class - .add_method( - "returnString", - Visibility::Public, - move |_, _| phper::ok("phper"), - ) + .add_method("returnString", Visibility::Public, move |_, _| { + phper::ok("phper") + }) .return_type(ReturnType::new(ReturnTypeHint::String)); class @@ -485,7 +508,9 @@ fn make_return_typehint_class() -> ClassEntity<()> { .return_type(ReturnType::new(ReturnTypeHint::String).allow_null()); class - .add_method("returnBool", Visibility::Public, move |_, _| phper::ok(true)) + .add_method("returnBool", Visibility::Public, move |_, _| { + phper::ok(true) + }) .return_type(ReturnType::new(ReturnTypeHint::Bool)); class @@ -505,7 +530,9 @@ fn make_return_typehint_class() -> ClassEntity<()> { .return_type(ReturnType::new(ReturnTypeHint::Int).allow_null()); class - .add_method("returnFloat", Visibility::Public, move |_, _| phper::ok(3.14)) + .add_method("returnFloat", Visibility::Public, move |_, _| { + phper::ok(1.234) + }) .return_type(ReturnType::new(ReturnTypeHint::Float)); class @@ -515,7 +542,9 @@ fn make_return_typehint_class() -> ClassEntity<()> { .return_type(ReturnType::new(ReturnTypeHint::Float).allow_null()); class - .add_method("returnArray", Visibility::Public, move |_, _| phper::ok(ZArray::new())) + .add_method("returnArray", Visibility::Public, move |_, _| { + phper::ok(ZArray::new()) + }) .return_type(ReturnType::new(ReturnTypeHint::Array)); class diff --git a/tests/integration/tests/php/typehints.php b/tests/integration/tests/php/typehints.php index e36bdb5..0d2e9a4 100644 --- a/tests/integration/tests/php/typehints.php +++ b/tests/integration/tests/php/typehints.php @@ -215,7 +215,7 @@ public function setValue($value): void { echo PHP_EOL . 'Testing return type-hinted function invocation' . PHP_EOL; assert_true(integration_function_return_bool()); assert_eq(42, integration_function_return_int()); -assert_eq(3.14, integration_function_return_float()); +assert_eq(1.234, integration_function_return_float()); assert_eq('phper', integration_function_return_string()); assert_eq(array(), integration_function_return_array()); assert_eq(1.23, integration_function_return_mixed()); @@ -227,7 +227,7 @@ public function setValue($value): void { assert_eq(null, $cls->returnBoolNullable(), 'returnBoolNullable'); assert_eq(42, $cls->returnInt(), 'returnInt'); assert_eq(null, $cls->returnIntNullable(), 'returnIntNullable'); -assert_eq(3.14, $cls->returnFloat(), 'returnFloat'); +assert_eq(1.234, $cls->returnFloat(), 'returnFloat'); assert_eq(null, $cls->returnFloatNullable(), 'returnFloatNullable'); assert_eq('phper', $cls->returnString(), 'returnString'); assert_eq(null, $cls->returnStringNullable(), 'returnStringNullable');