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.

[plstand]

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.

rtrim is the same:

<?php

var_dump(rtrim(''));
var_dump(rtrim(1));
var_dump(rtrim(null));

---

Actual result in PHP7:

string(0) ""
string(1) "1"
string(0) ""

Changing line 2 to declare(strict_types=0); makes no difference.

Actual result in HHVM, with hhvm.php7.all on:

string(0) ""
string(1) "1"

Warning: rtrim() expects parameter 1 to be string, null given in /home/ki/Projects/hhvm/ttt3.php on line 5
NULL

Changing line 2 to declare(strict_types=0); makes no difference.

---

Actual result in PHP7, with line 2 changed to declare(strict_types=1);:

string(0) ""

Fatal error: Uncaught TypeError: rtrim() expects parameter 1 to be string, integer given in /home/ki/Projects/hhvm/ttt3.php:4
Stack trace:
#0 /home/ki/Projects/hhvm/ttt3.php(4): rtrim(1)
#1 {main}
  thrown in /home/ki/Projects/hhvm/ttt3.php on line 4

Actual result in HHVM, with hhvm.php7.all on, and with line 2 changed to declare(strict_types=1);:

string(0) ""

Fatal error: Uncaught TypeError: Argument 1 passed to rtrim() must be an instance of string, int given in /home/ki/Projects/hhvm/ttt3.php:4
Stack trace:
#0 /home/ki/Projects/hhvm/ttt3.php(4): rtrim()
#1 {main}

---

Note the following:

1. With declare(strict_types=1);, HHVM is consistent with PHP7, in that line 4, which passes an integer, throws a TypeError.
2. With declare(strict_types=0); (the default), PHP7 accepts the string, the integer, and NULL. If the function were a user-defined function, NULL would not have been accepted (by either PHP7 or HHVM), and a TypeError would have been generated.
3. With declare(strict_types=0); (the default), HHVM accepts the string and the integer, but not NULL. Essentially, HHVM treats the built-in function like a user-defined function, whereas in PHP7, built-in functions are a special case, and NULL is accepted. For scalar (int, float, string, or bool) parameters to user-defined functions, neither PHP7 nor HHVM accept NULL. (Built-in functions are also a special case in that warnings are generated rather than TypeErrors.)

[apeabody]

OK, got it.

So guess the guess the question is in HHVM's PHP7 mode with declare(strict_types=0), is the absence of the special case for built-in's (str_pad, rtrim, etc) to accept NULL for a strings and integers by design or accident?

[paulbiss]

So the current behavior is definitely wrong. There are a number of issues with the way that we handle types on builtins. Right now for builtins HHVM will follow the correct PHP7 behavior for declare(strict_types=1), but falls back to PHP5 behavior for declare(strict_types=0) (hence the warning rather than a fatal).

The fix for this shouldn't be overly complicated, should just require some updates to tv-helpers.cpp and native.cpp. Will try to give it a look this week.

[photodude]

Related Error

Fatal error: Uncaught TypeError: Argument 1 passed to unserialize() must be an instance of string, null given in /home/travis/.phpenv/versions/hhvm-stable/bin/composer:23
Stack trace:
#0 (): unserialize()
#1 (): __SystemLib\PharArchiveHandler->parsePhar()
#2 (): __SystemLib\PharArchiveHandler->__construct()
#3 (): Phar->__construct()
#4 /home/travis/.phpenv/versions/hhvm-stable/bin/composer(23): Phar::mapPhar()
#5 {main}

https://3v4l.org/lHviH

[iangcarroll]

rtrim not taking null breaks Composer, FYI.

[photodude]

@paulbiss Is there any progress on this issue? It would be nice to be able to resume testing against hhvm in php7 mode and this is a blocking issue.

[photodude]

related error

Warning: ini_get_all() expects parameter 1 to be string, null given in /phpunit/src/Util/GlobalState.php on line 79
    #0 PHPUnit_Util_GlobalState::getIniSettingsAsString(), called at [/phpunit/src/Framework/TestCase.php:809]
    #1 PHPUnit_Framework_TestCase->run(), called at [/phpunit/src/Framework/TestSuite.php:753]
    #2 PHPUnit_Framework_TestSuite->run(), called at [/phpunit/src/Framework/TestSuite.php:753]
    #3 PHPUnit_Framework_TestSuite->run(), called at [/phpunit/src/Framework/TestSuite.php:753]
    #4 PHPUnit_Framework_TestSuite->run(), called at [/phpunit/src/TextUI/TestRunner.php:465]
    #5 PHPUnit_TextUI_TestRunner->doRun(), called at [/phpunit/src/TextUI/Command.php:185]
    #6 PHPUnit_TextUI_Command->run(), called at [/phpunit/src/TextUI/Command.php:115]
    #7 PHPUnit_TextUI_Command::main(), called at [/phpunit/phpunit:47]

https://3v4l.org/s3QtO

[photodude]

Looks like another couple of projects have dropped HHVM. Breaks like this issue are one of the reasons

• Symfony
  • https://twitter.com/fabpot/status/865263458903875584
• doctrine
  • https://twitter.com/lcobucci/status/865081210128793601

[asuth]

We depend on HHVM and also a lot of open-source code, so this is really bad/stressful for us. Is there anything we can do to vote for this or otherwise support this?

[mxw]

Hi folks!

First off, sorry that the communication on this issue has been a bit choppy and less than upfront. We're going to be meeting very soon to discuss how to prioritize PHP7 compat issues, this one being the biggest of the bunch.

I'll try to update here early next week about when we expect we'll be able to get this fixed (it sounds like this is the main remaining incompatibility). As @aorenste mentioned, we had an almost-working patch for the fix, but it's been stuck in rebase hell for a while. That's on us (obviously), and I'm hoping we'll be able to get it sorted out for you all soon. Thanks for your patience, to everyone who's been sticking it out.

[robfrawley]

This needed to have been a priority for the past year since the original issue was reported. Just yesterday @mofarrell noted in symfony/symfony#22733 (comment) that:

It happens to not be trivial to fix, and also currently not at the top of our priority list.

This leaves me particularly confused about the weight being given to this issue, as well as the intended public perception and expectation you are trying to convey.

As this issue makes it impossible to properly support PHP7-mode code in combination with HHVM, this must be prioritized if there is any expectation for library authors to support HHVM.

Since many of the projects I help maintain require Symfony components, they too will be dropping support due to the recent decision to drop HHVM support in Symfony's upcoming version 4.x releases. For example, the liip/imagine-bundle package removed HHVM support from the 2.x development branch two weeks ago, as discussed in liip/LiipImagineBundle#908 (comment).

I really do appreciate your work, and I also understand that this is a complex issue, but this is causing warranted frustration by many.

[FractalizeR]

It's a very sad thing that because of the lack of attention to such issues major frameworks just drop HHVM compatibility. Below is a quote from Fabien's post about Symfony 4.0

Symfony is not the first PHP project to drop HHVM support. Doctrine, CakePHP, and MongoDB dropped or will soon drop HHVM support (as commented on the Twitter poll). Laravel also dropped support as of version 5.3 (released 9 months ago). I can imagine that more PHP libraries will drop support as they will face the same issues as Symfony when requiring PHP 7.

That's also why I have decided to drop HHVM support for all my other significant projects like Twig (as of version 2), Silex, and Swiftmailer.

[photodude]

And another has dropped support Joomla is now out.
joomla/joomla-cms#16232

[oliverklee]

Also phpList 4 and Emogrifier: phpList/core#45 MyIntervals/emogrifier#386

[hikari-no-yume]

I think I remember pointing out, before HHVM implemented this, that it would need to handle null differently here. I guess that was overlooked.

[photodude]

Thanks @mofarrell for pushing a fix for this. With the fix merged, What versions will be getting it? Will all of the current LTS versions be fixed?

[fredemmott]

This is probably too intrusive a change for LTS backporting: it's much more important that 3.x.y can run code that 3.x.(y-1) could than for 3.x.y to be able to run /more/ code, even if the code running on 3.x.(y-1) was 'wrong' in some way. Exceptions are rare, except for security fixes.

[robfrawley]

The was a insurmountable language bug that caused a divergence between HHVM's internal function type handling from that of PHP's; it should be back-ported IMHO.

If it isn't back-ported to the latest LTS it won't be natively available on any distribution until their next LTS release, which will cause more project to abandon HHVM. It's an important fix that would ease concern from the PHP community and hopefully allow you to win back some support. Again, it should be back-ported.

[Orvid]

@fredemmott My understanding is that we do have plans to backport it to at least the current LTS.

[fredemmott]

If you're reasonably confident that this won't break existing code written to run on HHVM, I don't mind this being backported.

Regardless of the type of bug, unless there's a security issue that makes code changes required, upgrading from 3.x.y to 3.x.(y+1) should never break code that is already running. If it does, that breaks the entire purpose of having LTS releases (and to a lesser extent, .y releases in general).

[simonwelsh]

If this does end up in 3.18, can you also make sure it ends up in 3.19/3.20? It'd be rather strange to have things work in 3.18 but not 3.19, and will make version constraints that much more annoying.

[fredemmott]

The general problem was fixed, but composer isn't; repro case for that issue: https://3v4l.org/ahpE3 (based on @photodude's comment above)

[fredemmott]

Filed #7869 for composer as the original issue here is fixed; working on the splfileinfo issue, and whatever else comes up when testing composer.

[photodude]

Also broken if null is passed to splfileinfo() resulting in the same rtrim() issue originally reported (since splfileinfo() is dependent on rtrim() as noted above ).
https://3v4l.org/4ljZ8

[fredemmott]

yeah, we differ on any non-string input to splfileinfo

[hopeseekr]

This issue is needed, because *WE ARE NOT IN A PHP5 WORLD ANY LONGER.

The HHVM team, and Facebook Corp., need to be galvanized to committing to stay compatible with PHP7 or they need to break all pretenses and start marking Hack Language as its own thing.

Otherwise, once all PHP5 code is collecting dust, relatively soon, all of your, my, and their HHVM efforts will have been in vain!

Hopefully this PR is a wakeup call!

[photodude]

@hopeseekr PHP 5.6 is EOL the same month as PHP 7.0 at the end of 2018. I agree primary efforts should be on compatibility issues with PHP 7.1 and with the alpha/beta release of PHP 7.2.

the majority of new development in PHP land has moved to just PHP 7+ support, I agree HHVM needs to make more effort to do the same.