Repository navigation
fix(compiler): support the single-argument implode() form - #158
Conversation
matyhtf
left a comment
There was a problem hiding this comment.
The single-argument array cases are fixed, but the new branch bypasses PHP's parameter validation by converting arbitrary values to arrays. Please address this before merging.
Reproducer tested against PR HEAD e746946fd66548e7184412e25e58e8ebc9c44703:
function test(mixed $value): string {
return implode($value);
}
function main(): void {
var_dump(test(42));
}PHP throws TypeError; the PR build prints string(2) "42". Strings, booleans, null, and objects are also incorrectly accepted. The join() alias has the same issue. Even a direct implode('ab') returns "ab" instead of throwing.
The new resolveArg(..., ARG_TYPE_ARRAY) call generates:
php::fn::implode("", php::toArray(value))Reflection describes the first parameter as array|string, so the existing argument safety check does not enforce the array-only contract of the single-argument form. Calling toArray() then erases the original type before Zend can reject it.
Please:
- Validate optimizer safety against the single-argument form's actual
arraycontract. Only optimize arguments that are statically proven to be arrays; otherwise fall back to Zend's normal parameter validation. - Add PHPT coverage for invalid direct arguments and invalid values passed through
mixed/var, for bothimplode()andjoin(). Include strings, integers, booleans, null, and objects, and retain coverage for valid arrays passed through a dynamic value.
Validation: all 74 stdlib PHPTs and 3 related PHPUnit tests pass on this PR. The added PHPT fails to compile on the base and passes on the PR. Additional valid named-argument, unpacking, dynamic-value, and single-evaluation cases match PHP. The existing tests do not cover the invalid-input cases above.
Thanks for the contribution and the regression test.
|
Both points addressed:
Full |
Observed failure
PHP's documented single-argument form
implode(array $array)(php.net lists it as an official alternative signature; the separator defaults to"") fails to compile:ZendPHP 8.4:
TypePHP master (c481e06):
The two-argument form
implode('-', $arr)works, so the failure is specific to the arity-1 call. Thejoin()alias shares the same code path and is equally affected.Root cause
phpx declares only the two-argument overload (
vendor/swoole/phpx/include/std/string.h):PHP's reflection reports
implode()as requiring only 1 parameter (getNumberOfRequiredParameters() === 1, verified), so TypePHP'sdispatchFuncCall()arity gate passes a single-argument call.buildArgList()then walks the two-entry reflection type list against one provided argument and emitsphp::fn::implode(<array>)— a call the C++ side cannot resolve.Fix
src/Optimizer/FuncCallOptimizer.php, indispatchFuncCall(): whenimplodeis called with exactly one argument, dispatch through the existing two-argument overload with an empty glue string:The array argument is converted by the same
resolveArg()machinery a normal two-argument call uses (ARG_TYPE_ARRAY), and the empty glue is emitted as a raw C++ literal — the same shape a literal two-argument call produces. Thejoin()alias resolves toimplodebefore this point, so both names are fixed. The two-argument path is untouched.