Skip to content

Commit 60d4d13

Browse files
brettmcbrettmc
committed
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.
1 parent 84adb0b commit 60d4d13

File tree

5 files changed

+58
-47
lines changed

5 files changed

+58
-47
lines changed

‎phper-test/src/cli.rs‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -179,6 +179,8 @@ pub fn test_php_scripts_with_condition(
179179
"execute php test command");
180180

181181
if !condition(output) {
182+
eprintln!("--- stdout ---\n{}", stdout);
183+
eprintln!("--- stderr ---\n{}", stderr);
182184
panic!("test php file `{}` failed", path);
183185
}
184186
}

‎phper/src/functions.rs‎

Lines changed: 4 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -28,16 +28,11 @@ use std::{
2828
collections::HashMap,
2929
ffi::{CStr, CString},
3030
marker::PhantomData,
31-
mem::{ManuallyDrop, size_of, transmute, zeroed},
31+
mem::{ManuallyDrop, transmute, zeroed},
3232
ptr::{self, null_mut},
3333
rc::Rc,
34-
slice,
3534
};
3635

37-
/// Used to mark the arguments obtained by the invoke function as mysterious
38-
/// codes from phper
39-
const INVOKE_MYSTERIOUS_CODE: &[u8] = b"PHPER";
40-
4136
/// Used to find the handler in the invoke function.
4237
pub(crate) type HandlerMap = HashMap<(Option<CString>, CString), Rc<dyn Callable>>;
4338

@@ -371,9 +366,6 @@ impl FunctionEntry {
371366

372367
infos.push(zeroed::<zend_internal_arg_info>());
373368

374-
// Will be checked in `invoke` function.
375-
infos.push(Self::create_mysterious_code());
376-
377369
let raw_handler = handler.as_ref().map(|_| invoke as _);
378370

379371
if let Some(handler) = handler {
@@ -397,22 +389,12 @@ impl FunctionEntry {
397389
}
398390
}
399391
}
400-
401-
unsafe fn create_mysterious_code() -> zend_internal_arg_info {
402-
unsafe {
403-
let mut mysterious_code = [0u8; size_of::<zend_internal_arg_info>()];
404-
for (i, n) in INVOKE_MYSTERIOUS_CODE.iter().enumerate() {
405-
mysterious_code[i] = *n;
406-
}
407-
transmute(mysterious_code)
408-
}
409-
}
410392
}
411393

412394
/// Builder for registering php function.
413395
pub struct FunctionEntity {
414-
name: CString,
415-
handler: Rc<dyn Callable>,
396+
pub(crate) name: CString,
397+
pub(crate) handler: Rc<dyn Callable>,
416398
arguments: Vec<Argument>,
417399
return_type: Option<ReturnType>,
418400
}
@@ -797,7 +779,6 @@ impl ZFunc {
797779
pub(crate) union CallableTranslator {
798780
pub(crate) callable: *const dyn Callable,
799781
pub(crate) internal_arg_info: zend_internal_arg_info,
800-
pub(crate) arg_info: zend_arg_info,
801782
}
802783

803784
/// The entry for all registered PHP functions.
@@ -806,25 +787,7 @@ unsafe extern "C" fn invoke(execute_data: *mut zend_execute_data, return_value:
806787
let execute_data = ExecuteData::from_mut_ptr(execute_data);
807788
let return_value = ZVal::from_mut_ptr(return_value);
808789

809-
let num_args = execute_data.common_num_args();
810-
let arg_info = execute_data.common_arg_info();
811-
812-
// should be mysterious code
813-
let mysterious_arg_info = arg_info.offset((num_args + 1) as isize);
814-
let mysterious_code = slice::from_raw_parts(
815-
mysterious_arg_info as *const u8,
816-
INVOKE_MYSTERIOUS_CODE.len(),
817-
);
818-
819-
let handler = if mysterious_code == INVOKE_MYSTERIOUS_CODE {
820-
// hiddden real handler
821-
let last_arg_info = arg_info.offset((num_args + 2) as isize);
822-
let translator = CallableTranslator {
823-
arg_info: *last_arg_info,
824-
};
825-
let handler = translator.callable;
826-
handler.as_ref().expect("handler is null")
827-
} else {
790+
let handler = {
828791
let function_name = execute_data
829792
.func()
830793
.get_function_name()

‎phper/src/modules.rs‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,13 @@ unsafe extern "C" fn module_startup(_type: c_int, module_number: c_int) -> c_int
5555
interface_entity.init();
5656
}
5757

58+
for function_entity in &module.function_entities {
59+
module.handler_map.insert(
60+
(None, function_entity.name.clone()),
61+
function_entity.handler.clone(),
62+
);
63+
}
64+
5865
for class_entity in &module.class_entities {
5966
let ce = class_entity.init();
6067
class_entity.declare_properties(ce);

‎tests/integration/src/typehints.rs‎

Lines changed: 21 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,12 +9,14 @@
99
// See the Mulan PSL v2 for more details.
1010

1111
use phper::{
12+
arrays::ZArray,
1213
classes::{ClassEntity, Interface, InterfaceEntity, StateClass, Visibility},
1314
functions::{Argument, ReturnType},
1415
modules::Module,
1516
types::{ArgumentTypeHint, ReturnTypeHint},
1617
values::ZVal,
1718
};
19+
use std::convert::Infallible;
1820

1921
const I_FOO: &str = r"IntegrationTest\TypeHints\IFoo";
2022

@@ -26,6 +28,20 @@ pub fn integrate(module: &mut Module) {
2628
module.add_class(make_arg_typehint_class());
2729
module.add_class(make_return_typehint_class());
2830
module.add_class(make_arg_default_value_class());
31+
module.add_function("integration_function_return_bool", |_| -> Result<bool, Infallible> { Ok(true)} )
32+
.return_type(ReturnType::new(ReturnTypeHint::Bool));
33+
module.add_function("integration_function_return_int", |_| -> Result<i64, Infallible> { Ok(42)} )
34+
.return_type(ReturnType::new(ReturnTypeHint::Int));
35+
module.add_function("integration_function_return_float", |_| -> Result<f64, Infallible> { Ok(3.14)} )
36+
.return_type(ReturnType::new(ReturnTypeHint::Float));
37+
module.add_function("integration_function_return_string", |_| -> Result<&'static str, Infallible> { Ok("phper")} )
38+
.return_type(ReturnType::new(ReturnTypeHint::String));
39+
module.add_function("integration_function_return_array", |_| -> Result<ZArray, Infallible> { Ok(ZArray::new()) } )
40+
.return_type(ReturnType::new(ReturnTypeHint::Array));
41+
module.add_function("integration_function_return_mixed", |_| -> Result<ZVal, Infallible> { Ok(ZVal::from(1.23))} )
42+
.return_type(ReturnType::new(ReturnTypeHint::Mixed));
43+
module.add_function("integration_function_return_void", |_| phper::ok(()))
44+
.return_type(ReturnType::new(ReturnTypeHint::Void));
2945
module
3046
.add_function("integration_function_typehints", |_| phper::ok(()))
3147
.argument(
@@ -458,7 +474,7 @@ fn make_return_typehint_class() -> ClassEntity<()> {
458474
.add_method(
459475
"returnString",
460476
Visibility::Public,
461-
move |_, _| phper::ok(()),
477+
move |_, _| phper::ok("phper"),
462478
)
463479
.return_type(ReturnType::new(ReturnTypeHint::String));
464480

@@ -469,7 +485,7 @@ fn make_return_typehint_class() -> ClassEntity<()> {
469485
.return_type(ReturnType::new(ReturnTypeHint::String).allow_null());
470486

471487
class
472-
.add_method("returnBool", Visibility::Public, move |_, _| phper::ok(()))
488+
.add_method("returnBool", Visibility::Public, move |_, _| phper::ok(true))
473489
.return_type(ReturnType::new(ReturnTypeHint::Bool));
474490

475491
class
@@ -479,7 +495,7 @@ fn make_return_typehint_class() -> ClassEntity<()> {
479495
.return_type(ReturnType::new(ReturnTypeHint::Bool).allow_null());
480496

481497
class
482-
.add_method("returnInt", Visibility::Public, move |_, _| phper::ok(()))
498+
.add_method("returnInt", Visibility::Public, move |_, _| phper::ok(42))
483499
.return_type(ReturnType::new(ReturnTypeHint::Int));
484500

485501
class
@@ -489,7 +505,7 @@ fn make_return_typehint_class() -> ClassEntity<()> {
489505
.return_type(ReturnType::new(ReturnTypeHint::Int).allow_null());
490506

491507
class
492-
.add_method("returnFloat", Visibility::Public, move |_, _| phper::ok(()))
508+
.add_method("returnFloat", Visibility::Public, move |_, _| phper::ok(3.14))
493509
.return_type(ReturnType::new(ReturnTypeHint::Float));
494510

495511
class
@@ -499,7 +515,7 @@ fn make_return_typehint_class() -> ClassEntity<()> {
499515
.return_type(ReturnType::new(ReturnTypeHint::Float).allow_null());
500516

501517
class
502-
.add_method("returnArray", Visibility::Public, move |_, _| phper::ok(()))
518+
.add_method("returnArray", Visibility::Public, move |_, _| phper::ok(ZArray::new()))
503519
.return_type(ReturnType::new(ReturnTypeHint::Array));
504520

505521
class

‎tests/integration/tests/php/typehints.php‎

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -209,4 +209,27 @@ public function setValue($value): void {
209209
echo "PASS" . PHP_EOL;
210210
}
211211
assert_eq('void', $reflection->getReturnType()->getName(), 'integration_function_typehints return type is void');
212-
}
212+
}
213+
214+
//invoke type-hinted functions to exercise handlers
215+
echo PHP_EOL . 'Testing return type-hinted function invocation' . PHP_EOL;
216+
assert_true(integration_function_return_bool());
217+
assert_eq(42, integration_function_return_int());
218+
assert_eq(3.14, integration_function_return_float());
219+
assert_eq('phper', integration_function_return_string());
220+
assert_eq(array(), integration_function_return_array());
221+
assert_eq(1.23, integration_function_return_mixed());
222+
223+
//invoke type-hinted class methods to exercise handlers
224+
echo PHP_EOL . 'Testing return type-hinted method invocation' . PHP_EOL;
225+
$cls = new \IntegrationTest\TypeHints\ReturnTypeHintTest();
226+
assert_eq(true, $cls->returnBool(), 'returnBool');
227+
assert_eq(null, $cls->returnBoolNullable(), 'returnBoolNullable');
228+
assert_eq(42, $cls->returnInt(), 'returnInt');
229+
assert_eq(null, $cls->returnIntNullable(), 'returnIntNullable');
230+
assert_eq(3.14, $cls->returnFloat(), 'returnFloat');
231+
assert_eq(null, $cls->returnFloatNullable(), 'returnFloatNullable');
232+
assert_eq('phper', $cls->returnString(), 'returnString');
233+
assert_eq(null, $cls->returnStringNullable(), 'returnStringNullable');
234+
assert_eq(array(), $cls->returnArray(), 'returnArray');
235+
assert_eq(null, $cls->returnArrayNullable(), 'returnArrayNullable');

0 commit comments

Comments
 (0)