apeabody commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@jwatzman : this is runtime, not the typechecker

jwatzman commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

HHVM enables strict_types by default when you enable php7 mode.

apeabody commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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:

With declare(strict_types=1);, HHVM is consistent with PHP7, in that line 4, which passes an integer, throws a TypeError.
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.
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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

rtrim not taking null breaks Composer, FYI.

photodude commented 8 years ago

@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 commented 8 years ago

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 commented 7 years ago

@paulbiss Just checking back on this issue? It would be nice to be able to resume testing against hhvm in php7 mode and this is a blocking issue.

Kazanir commented 7 years ago

Right now this issue means that no one can use HHVM to build a Composer project which contains PHP7-only packages, FYI.

photodude commented 7 years ago

@aorenste Just checking back on this issue.... It would be nice to be able to resume testing against hhvm in php7 mode and this is a blocking issue since it breaks most composer based builds.

Orvid commented 7 years ago

We have a mostly functional diff up internally to deal with correctly enabling compliance with PHP7 strict mode, but there are still a few edge cases left to solve.

photodude commented 7 years ago

@Orvid that's great news. Looking forward to the eventual fix on this. (patiently, but eagerly waiting)

aorenste commented 7 years ago

@photodude Yeah - the "mostly easy" tag turns out to be quite false. The internal patch is passing tests and is making its rounds through review.

asuth commented 7 years ago

How's this fix coming along? Would love to see it land! Thanks for all your work on this.

For others following along, here's where we (Quizlet) got stuck with this issue:

Mockery doesn't believe parameter hinting and return types exist for functions when it thinks it's in PHP < 7
If you turn on PHP7 mode (hhvm.php7.all = 1) in HHVM, it switches to PHP7 "strict mode" which causes errors when you call builtins with bad types e.g. call strpos with an int instead of a string. This happens all over our code and in library code (e.g. Composer)
If you edit Mockery to do return types in php5 mode (aka what we're in now), it complains that eval() cannot use Hack code (aka code with return types)

These three problems are intractable if you want methods with return type hints to be mocked, until HHVM fixes this issue.

So for now, because we want to keep our typehints, and conveniently typehint errors are E_RECOVERABLE_ERROR which do not break php internals when they happen, we setup a set_error_handler to specifically ignore this exact typehinting error, but throw on everything else. It's definitely messy but the only workaround we could think of.

photodude commented 7 years ago

@asuth you should also look at Issues #7467 and #7544

aorenste commented 7 years ago

@asuth - It turns out to be much harder than I originally thought (a bunch of changes have to wind through the code). I still have it as a background task but am currently working on other issues.

photodude commented 7 years ago

@aorenste We look forward to the fix. Especially since some projects are now moving to requiring PHP 7.0+ through composer which make this an even bigger blocking issue.

photodude commented 7 years ago

Another function used by composer which is affected...

 strtolower() expects parameter 1 to be string, null given

photodude commented 7 years ago

@aorenste Just checking back on this issue.... How's the progress on getting the patch for this issue through review?

aorenste commented 7 years ago

@photodude I still haven't gotten back to it. It's not really a patch as much as a "work in progress". If I remember where I was at the time it works about 90% of the time but fails in cases of inlined functions and also has some problems in repo mode.

photodude commented 7 years ago

@aorenste thanks for the update. I learned of an HHVM internal related failure due to this issue

Fatal error: Uncaught TypeError: Argument 1 passed to dirname() must be an instance of string, bool given in phar://composer.phar/bin/../src/../vendor/symfony/finder/Iterator/RecursiveDirectoryIterator.php:141

the root of this from composer to symfony/finder comes from HHVM itself. The dirname call is not in the symfony/finder code, but in the implementation of SplFileInfo->getPath(), which is part of HHVM. see https://github.com/facebook/hhvm/blob/master/hphp/system/php/spl/file_handling/SplFileInfo.php#L40

digging deeper we are back to the rtrim issue at the class construct call to this method https://github.com/facebook/hhvm/blob/master/hphp/system/php/spl/file_handling/SplFileInfo.php#L438-L443

related issues

SCIF commented 7 years ago

@photodude , @aorenste , any forecast on the solution date? Also, will that solution be backported to previous releases? Thanks in advance!

photodude commented 7 years ago

@SCIF I'm just an interested party in the solution. I have no knowledge on the current progress.

@fredemmott Who is assigned to this issue now that you unassigned @aorenste ? This is a major blocking issue for any project that has moved on to only supporting php 7 and uses composer. (it's also another reason that major projects are or have dropped HHVM support)

fredemmott commented 7 years ago

No one is actively working on this as far as I'm aware - I'm no longer working on HHVM though.

fredemmott commented 7 years ago

I unassigned Aaron as I believe he's no longer working on compatibility issues; having it in the unassigned pile is better than having it incorrectly marked as being worked on.

photodude commented 7 years ago

If unassigned at the moment is best, then that is best. This is a long standing issue that needs to be addressed to continue php support in hhvm. More php projects will likely drop HHVM support the longer this issue continues to not be addressed.

Orvid commented 7 years ago

@aorenste Would you be willing to at least put the change up as a PR rather than a diff so anyone externally that wants to try to get things working has a decent idea of the scope and where to start?

aorenste commented 7 years ago

@Orvid The problem is that @paulbiss pointed out that the approach I took isn't even close because it fails if the called function is inlined. As such I'm not sure that my starting point would help someone else. I never got very far on the alternative approach.

fredemmott commented 7 years ago

dirname() completely breaks composer (https://gist.github.com/fredemmott/783596a9591f19d34940c223dc75dd5b)

given that the PHP ecosystem is rapidly moving on from PHP5, this is really a big deal.

photodude commented 7 years ago

@fredemmott I have also seen that, it comes back to the rtrim issue

Fatal error: Uncaught TypeError: Argument 1 passed to dirname() must be an instance of string, bool given in phar://composer.phar/bin/../src/../vendor/symfony/finder/Iterator/RecursiveDirectoryIterator.php:141

the root of this from composer to symfony/finder comes from HHVM itself. The dirname call is not in the symfony/finder code, but in the implementation of SplFileInfo->getPath(), which is part of HHVM. see https://github.com/facebook/hhvm/blob/master/hphp/system/php/spl/file_handling/SplFileInfo.php#L40

digging deeper we are back to the rtrim issue at the class construct call to this method https://github.com/facebook/hhvm/blob/master/hphp/system/php/spl/file_handling/SplFileInfo.php#L438-L443

related issues

photodude commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

This needed to have been a priority for the past year since the original issue was reported. Just yesterday @mofarrell noted in https://github.com/symfony/symfony/pull/22733#issuecomment-302516789 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 https://github.com/liip/LiipImagineBundle/pull/908#issuecomment-299654044.

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 commented 7 years ago

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 commented 7 years ago

And another has dropped support Joomla is now out. https://github.com/joomla/joomla-cms/pull/16232

oliverklee commented 7 years ago

Also phpList 4 and Emogrifier: https://github.com/phpList/phpList/pull/45 https://github.com/jjriv/emogrifier/pull/386

hikari-no-yume commented 7 years ago

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 commented 7 years ago

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?

facebook / hhvm

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

HHVM Version

Standalone code, or other way to reproduce the problem

Expected result

Actual result

Potential Solutions

0 {main}

related issues

related issues