HHVM and PHP7 differ on type annotations on internal functions (rtrim in particular)

[apeabody]

What is the expected behavior with regards to strict type checking? PHP7 defaults to “coercive mode” while hhvm.php7.all = 1 appears to act like "strict mode" in this example?

HHVM Version

HHVM 3.14.1 hhvm.php7.all = 1
PHP 7.0.4

Standalone code, or other way to reproduce the problem

<?php

$result = rtrim(null);
echo $result;

Expected result

identical (no output)

Actual result

hhvm --php test.php

Warning: rtrim() expects parameter 1 to be string, null given in /home/vagrant/hhvm/hphp/test.php on line 3

php7 test.php

Potential Solutions

Perhaps hhvm.php7.scalar_types should be left out of hhvm.php7.all? It appears currently to make HHVM act like PHP7's declare(strict_types=1); mode.

[jwatzman]

The typechecker understands only a few PHP7 features, and for all of the BC breaks, takes the PHP5 behaviour. Basically it follows the same logic as HHVM with hhvm.php7.all = 0.

[apeabody]

Hmm, well is there a more elegant way to have HHVM emulate the PHP7 default behavior example above? Currently the example is preventing code that should work in PHP7 from running under HHVM in php7.all mode, and I worry about the side effects from the below "work-around" configuration:

hhvm.php7.all = 1
hhvm.php7.scalar_types = 0

Otherwise at the very least I think it makes sense to better document the expected divergence in PHP7 behavior. Or if it is, I can't find the citation: https://docs.hhvm.com/hhvm/configuration/INI-settings#php-7-settings

[fredemmott]

@jwatzman : this is runtime, not the typechecker

[jwatzman]

Yeah looks like I misunderstood your question, ignore my comment above :-P

I remember there being some discussion on internal functions and type enforcement. Unfortunately I don't remember if it was in the context of PHP7, Hack types, or even a discussion on internals itself about PHP7's behaviour.

While HHVM's behaviour is different than PHP7, it's just a warning -- doesn't strict mode mean that it would throw some sort of exception? The difference seems to be that HHVM has a type annotation on an internal function and PHP7 does not. Maybe we should fix rtrim to accept null (eww though).

My recollection of how all of this fits together is super rusty, please correct anything above that sounds wrong. Also cc @paulbiss who implemented this for HHVM.

[apeabody]

Including @paulbiss per recommendation.

Here are some more samples, same rtrim(null) as above, but similar behavior occurs in other string functions such as substr().

PHP7.0.4
(no warning)

HHVM-nightly hhvm.php7.all = 1
Warning: rtrim() expects parameter 1 to be string, null given in /home/ubuntu/test.php on line 3

PHP7 declare(strict_types=1)
PHP Fatal error: strict_types declaration must not use block mode in /home/ubuntu/test.php on line 2

HHVM hhvm.php7.all = 1 declare(strict_types=1)
Fatal error: Uncaught Error: strict_types declaration must not use block mode in /home/ubuntu/test.php:2
Stack trace:
#0 {main}

The potential issue is some frameworks/unit tests (Composer is an this case: https://getcomposer.org) catch the warning from rtrim in HHVM's php7.all=1 mode above as a fatal error/incompatiblity. I don't know if true PHP7 compatibility is the goal, of if this should be a documented difference.

[plstand]

From the Scalar Type Declarations RFC (emphasis added):

The weak type checking rules for the new scalar type declarations are mostly the same as those of extension and built-in PHP functions. The only exception to this is the handling of NULL: in order to be consistent with our existing type declarations for classes, callables and arrays, NULL is not accepted by default, unless it is a parameter and is explicitly given a default value of NULL.

So if HHVM does apply the PHP7 weak type checking rules to built-in functions, for PHP7 compatibility, each and every parameter of a built-in function having a scalar type hint (to match zend_parse_parameters()) must accept NULL (unlike user-defined functions), EXCEPT in strict_types mode:

The strict type checking rules are quite straightforward: when the type of the value matches that specified by the type declaration it is accepted, otherwise it is not.

These strict type checking rules are used for userland scalar type declarations, and for extension and built-in PHP functions.

The one exception is that widening primitive conversion is allowed for int to float. This means that parameters that declare float can also accept int.

[plstand]

The potential issue is some frameworks/unit tests (Composer is an this case: https://getcomposer.org) catch the warning from rtrim in HHVM's php7.all=1 mode above as a fatal error/incompatiblity. I don't know if true PHP7 compatibility is the goal, of if this should be a documented difference.

This is not merely a warning that PHP7 does not generate, yet HHVM does. Rather, the function call fails because of the type error:

hphpd> =str_pad('', 8)
=str_pad('', 8)
"        "
hphpd> =str_pad(null, 8)
=str_pad(null, 8)

Warning: str_pad() expects parameter 1 to be string, null given
null

In PHP7, I get the same result in both cases:

php > var_dump(str_pad('', 8));
string(8) "        "
php > var_dump(str_pad(null, 8));
string(8) "        "

[Orvid]

HHVM enables strict_types by default when you enable php7 mode.

[apeabody]

Thanks - so it sounds like this is by design. If that's the case and desired, then I would suggest documenting (or at least point to) that HHVM's PHP7 is always/really PHP7 strict_types and isn't targeting default compatibility with PHP7's default “coercive mode”. That would hopefully assist with putting issues to rest about 3rd party code that consider HHVM's PHP7 as "broken" because the code doesn't work in strict_types.

[plstand]

HHVM enables strict_types by default.

Is that true? If so, I would expect lines 4 and 5 of the following test case to generate warnings. Instead, only line 5 generates a warning. (In PHP7, no warning or error is generated.)

<?php

var_dump(str_pad('', 8));
var_dump(str_pad(1, 8));
var_dump(str_pad(null, 8));

Also, changing line 2 (the blank line) to read declare(strict_types=0); makes no difference, and changing it to read declare(strict_types=1); causes line 4 to generate a TypeError, as in PHP7.

[apeabody]

I don't know about str_pad in particular, but feel free to test my example of using rtrim(null) above for the behavior I'm seeing.