PhpToken::is(): switch @param to typehint
#16218
Conversation
The type is enforced, and `TypeError`s are already thrown, but the information about the required type is not provided to Reflection. Replace the `@param` comment with a real typehint so that the information is also available via Reflection.
| @@ -24,8 +24,7 @@ public static function tokenize(string $code, int $flags = 0): array {} | |||
|
|
|||
| final public function __construct(int $id, string $text, int $line = -1, int $pos = -1) {} | |||
|
|
|||
| /** @param int|string|array $kind */ | |||
| public function is($kind): bool {} | |||
| public function is(int|string|array $kind): bool {} | |||
There was a problem hiding this comment.
In order to be able to add the type declaration, the custom parameter parsing should be consistent with the "real" ZPP if it contained Z_PARAM_LONG() which ultimately runs the zend_parse_arg_long_weak function for type juggling.
There was a problem hiding this comment.
I'm sorry, I don't really understand what you're saying - is the typehint not allowed because integer-like strings are not cast to integers?
There was a problem hiding this comment.
So what I really wanted to say is that ideally, a native type declaration should only be added when ZPP behaves exactly how the same type declaration would be treated in userland. However, the validation in debug mode only checks whether the function doesn't throw when in fact it should based on the type declaration: https://github.com/php/php-src/blob/0b3684c48e8dd1b960bd179f7957d69d0b882ea2/Zend/zend_vm_execute.h#L2043
PhpToken::is() always parses parameters strictly, so it rather throws when it shouldn't (e.g. when a float or a numeric string is passed to it), so it slightly differs from what a userland int|string|array type check would do. That's why in the past we changed a lot of Z_PARAM_ZVAL() calls to more precise Z_PARAM_* calls, but I just cannot remember whether it was strictly necessary to do, or if we just did it because it was better for consistency (since the zend_internal_call_arginfo_violation check doesn't always find the issue). @Girgias could you please refresh my mind? Do you think PhpToken::is() can have the native type declaration?
There was a problem hiding this comment.
Huuuum, we have been using native types for GMP functions, but it does handle the deprecations for null and weak type semantics.
I know we don't use native types explicitly for method_exists and property_exists so that we can by-pass known wrong values of int and bool. (and maybe we should revisit this for those cases.)
So I think you are correct @kocsismate in that we don't do it for consistency with weak mode.
The type is enforced, and
TypeErrors are already thrown, but the information about the required type is not provided to Reflection. Replace the@paramcomment with a real typehint so that the information is also available via Reflection.