facebook / hhvm

A virtual machine for executing programs written in Hack.
https://hhvm.com
Other
18.21k stars 3k forks source link

Compatibility with PHP 7.1 nullable types #7467

Closed ezzatron closed 7 years ago

ezzatron commented 8 years ago

This is more of a question than a bug report, but I think I might have found some regressions in HHVM's type system with regards to nullable types.

I maintain a mocking library with support for HHVM >= 3.6 and PHP >= 5.3. In order to support newer language features, such as nullable types, I use feature detection where possible, rather than checking for hardcoded version numbers.

I have a check for nullable type support that boils down to this:

$reporting = error_reporting(E_ERROR | E_COMPILE_ERROR);
$result = false;

try {
    $result = eval('function(?int $a){};return true;');
} catch (Throwable $e) {
} catch (Exception $e) {
    // ignore
}

error_reporting($reporting);
$isSupported = true === $result;

var_dump($isSupported);

Which was working fine, or so I thought, until a user of the mocking library submitted a bug report because HHVM was throwing fatal errors when running this code.

I verified that the bug report was valid, and can reproduce it, but the strange thing is that it doesn't produce fatal errors for a couple of HHVM versions in between two versions that do produce fatals (see this build for details):

screen shot 2016-11-04 at 12 40 13 pm

And even stranger, this code is also executed in the Travis build for the mocking framework itself, where it works for all versions of HHVM shown above. In addition, it behaves differently again when run under 3v4l.org: https://3v4l.org/R3ZcY

So, there are a few questions that arise from the above issues:

  1. What factors could be causing the differences I'm seeing here across ostensibly identical versions of HHVM? Is it perhaps a compilation flag that changes this behavior?
  2. Why does support for this approach to feature detecting seem to get better around 3.9, but worse again with 3.15? Is there perhaps a regression somewhere?
  3. What are the plans for HHVM with regards to supporting PHP7 type system features (including nullable types) in the future? Will HHVM eventually "just work" with PHP7 syntax?

Any info or ideas appreciated. Thanks for reading!

aorenste commented 8 years ago

Hi,

As far as I'm aware HHVM has supported nullable types longer than PHP has. (@paulbiss can confirm).

I'm pretty sure HHVM should already "just work" with most if not all of PHP7 syntax already.

This is a weird issue - it seems to be tied in some way to the Hack.Lang.AutoprimeGenerators flag:

$ hhvm tmp.php

Fatal error: Syntax only allowed in Hack files (<?hh) or with -v Eval.EnableHipHopSyntax=true in tmp.php
$ hhvm -vHack.Lang.AutoprimeGenerators=true tmp.php

Fatal error: Syntax only allowed in Hack files (<?hh) or with -v Eval.EnableHipHopSyntax=true in tmp.php
$ hhvm -vHack.Lang.AutoprimeGenerators=false tmp.php
bool(true)

This might be a bug or maybe @paulbiss can explain...

photodude commented 8 years ago

@ezzatron you will likely be interested in issue #7198 which references the php7 mode type annotations issue.

Don't expect bug fixes for any HHVM before 3.15.x since all of those older LTS versions are EOL and will likely only ever get security patches if they get any patches at all.

Note: looking at your repo you are not actually running HHVM in php 7 mode so your issues here are with php 5 mode. Take a look at this travis.yml for an example of how to set up the HHVM php7 mode on Travis for testing, just note, the due to #7198 composer doesn't function and blocks testing repos with hhvm in php7 mode.

@aorenste please add the php5 incompatibility label since the issues here are from running in php5 mode.

ezzatron commented 8 years ago

@aorenste I couldn't find documentation for Hack.Lang.AutoprimeGenerators. I'm assuming it has to do with calling next() before iterating a generator?

@photodude I wasn't aware of the existence of 'PHP 7 mode'. I should probably be testing against both configurations of HHVM I suppose. Older versions not getting patched is no big deal, I was just hoping to find a way to detect support for nullable types without relying on version checks.

Thanks to both of you for your help :)

photodude commented 8 years ago

@ezzatron you are welcome. The php7 mode is documented in the HHVM ini settings info. it's only available since 3.11 but considering Travis only has LTS versions of HHVM you can only test both php 5 and php 7 modes with hhvm-3.12, hhvm-3.15, hhvm-nightly and hhvm 'current ... which is 3.15 until 3.16 is released'. I also would test with hhvm against the default php 5 mode and the php 7 mode, but like I said due to issue #7198 php 7 mode breaks composer and thus prevents testing. Once issue #7198 is resolved, testing with both php 5 mode and php 7 mode on hhvm can resume.

ezzatron commented 8 years ago

@photodude Sorry to bother you again, but I tried using hhvm.php7.all=1, and it seems to have no effect on nullable type syntax. I just tried with this simple script:

<?php function() : ?int {};

And I get:

$ php -d hhvm.php7.all=1 tmp.php

Fatal error: Uncaught Error: Syntax only allowed in Hack files (<?hh) or with -v Eval.EnableHipHopSyntax=true in /path/to/tmp.php:1
Stack trace:
#0 {main}

Am I missing something else I need to enable PHP 7 mode? I'm running HHVM from the hhvm/hhvm Docker repo, at version:

$ php --version
HipHop VM 3.15.2 (rel)
Compiler: tags/HHVM-3.15.2-0-g83ac3e5e3f5657be0cf4c55884044f86a7818b90
Repo schema: 608339137764e8365964a1adaa7a27d125b6076f

I also tried enabling hhvm.php7.all=1 from an actual .ini file, in case HHVM didn't support the -d flag, but I got the same result.

photodude commented 8 years ago

@ezzatron I get a parse error with that "simple script" in php 5.6 and 7.0 it only seems to work in php 7.1 https://3v4l.org/UKJJG (note: I believe 3v4l is running in php 7 mode as per this change https://3v4l.uservoice.com/forums/219058-general/suggestions/16257685-hhvm-in-php-5-mode )

I re-ran your original code https://3v4l.org/PbcYV and it seems to be working for hhvm-3.12.0 - 3.13.2 but not with 3.14

I also ran your original code snip under travis ci for a quick check of HHVM 3.15 - 3.16 with and without php 7 mode https://travis-ci.org/photodude/HHVM_memory_test/builds/173798693

both 3.15 and 3.16 in php 7 return bool(false) but in php 5 mode they error Fatal error: Syntax only allowed in Hack files (<?hh) or with -v Eval.EnableHipHopSyntax=true

The expected for hhvm should be

So something is not right in hhvm's compatibility with php 5. Here is a quick table to break it down

version expected actual
php 5.6 bool(false) bool(false)
php 7.0 bool(false) bool(false)
php 7.1 bool(true) bool(true)
HHVM php 5 mode bool(false) Fatal error:
HHVM php 7 mode bool(false) bool(false)
ezzatron commented 8 years ago

For some reason I was thinking that nullable types with the question mark syntax were a PHP 7.0 thing, but you're absolutely right, it's a PHP 7.1 feature. Sorry! That completely explains why the ini setting has no effect.

That begs the question; will hhvm.php7.all eventually cover PHP 7.1 features too? Or will there be a hhvm.php71.all or similar setting?

I'll also change the title of this issue to reflect the fact that it's really about a PHP 7.1 feature.

photodude commented 8 years ago

@ezzatron Even though nullable types with the question mark syntax is a PHP 7.1 feature we have a PHP 5 incompatibility as HHVM in php 5 mode should not throw a Fatal error on your code snip.

I've updated my table above showing the expected and actual highlighting the Fatal error with HHVM in php 5 mode for the code snip you provided.

Hopefully @aorenste can chime in on the question of php 7.1 features for HHVM and can add the PHP 5 label for the Fatal error issue.

ezzatron commented 8 years ago

I've been trying really hard to create a reproducible case for another aspect of this issue that's really been puzzling me, which is that under some circumstances, for HHVM in 'php 5 mode', this code does not produce a fatal error. I haven't been able to truly pin it down, but I believe I'm a step closer.

I found that when I ran this code via the hhvm/hhvm Docker image:

<?php // tmp.php

eval('function() : ?int {};');
echo "The reports of my death are greatly exaggerated.\n";

I can reproducibly get this output:

$ docker run -it --rm -v "${PWD}:/usr/src/hhvm-test" -w /usr/src/hhvm-test -e "PATH=/usr/bin" hhvm/hhvm php tmp.php

Fatal error: Syntax only allowed in Hack files (<?hh) or with -v Eval.EnableHipHopSyntax=true in /usr/src/hhvm-test/tmp.php(5)(1a5e476ea0f3fb9b5d5b1599f166cf72) : eval()'d code on line 1
$ echo $?
255

Note the exit code is 255, and that the echo statement is never run. But when I do something to affect the error handling, such as setting the error reporting level, it changes how HHVM behaves. So for example, with this code:

<?php // tmp.php

error_reporting(E_WARNING);

eval('function() : ?int {};');
echo "The reports of my death are greatly exaggerated.\n";

The output now looks like this:

$ docker run -it --rm -v "${PWD}:/usr/src/hhvm-test" -w /usr/src/hhvm-test -e "PATH=/usr/bin" hhvm/hhvm php tmp.php

Fatal error: Syntax only allowed in Hack files (<?hh) or with -v Eval.EnableHipHopSyntax=true in /usr/src/hhvm-test/tmp.php(5)(1a5e476ea0f3fb9b5d5b1599f166cf72) : eval()'d code on line 1
The reports of my death are greatly exaggerated.
$ echo $?
0

Note the exit code is now 0, and the echo statement was executed, which seems to suggest that this error is not a "real" fatal error. Or at least, that HHVM can be influenced to consider it to be non-fatal in some circumstances.

Further testing seems to indicate that once you set the error_reporting level, the fatal becomes an E_WARNING, because if you change the error_reporting to something that does not include E_WARNING, the error message is suppressed:

<?php // tmp.php

error_reporting(E_ALL ^ E_WARNING);

eval('function() : ?int {};');
echo "The reports of my death are greatly exaggerated.\n";

This produces the following output:

$ docker run -it --rm -v "${PWD}:/usr/src/hhvm-test" -w /usr/src/hhvm-test -e "PATH=/usr/bin" hhvm/hhvm php tmp.php
The reports of my death are greatly exaggerated.
$ echo $?
0

I also found that simply setting an error handler can affect the behavior, even though the error handler is never called. I.e. this script:

<?php // tmp.php

set_error_handler(function () {
    echo "I am never called.\n";
    exit;
});

eval('function() : ?int {};');
echo "The reports of my death are greatly exaggerated.\n";

Produces this output:

$ docker run -it --rm -v "${PWD}:/usr/src/hhvm-test" -w /usr/src/hhvm-test -e "PATH=/usr/bin" hhvm/hhvm php tmp.php

Fatal error: Syntax only allowed in Hack files (<?hh) or with -v Eval.EnableHipHopSyntax=true in /usr/src/hhvm-test/tmp.php(8)(1a5e476ea0f3fb9b5d5b1599f166cf72) : eval()'d code on line 1
The reports of my death are greatly exaggerated.
$ echo $?
0

So even though the fatal seems to be converted to an E_WARNING, it's some kind of weird warning that cannot be handled.

There's definitely something spooky going on here.

photodude commented 7 years ago

@ezzatron as this is a php 7.1 incompatibility issue you should look at issue #7544

fredemmott commented 7 years ago

@mofarrell : fixed by f02a287591d35ed4d846751f18d211272b859a66 ?

mofarrell commented 7 years ago

I believe so. We should probably add testing. I stopped it from being a hack only feature.

fredemmott commented 7 years ago

Closing given https://3v4l.org/A4AQ1