Generator behavior doesn't match PHP5

[asm89]

Apart from syntax (#1627) and instanceof (#1787) issues, generators in hhvm and zend behave differently.

The following gist contains three examples that show different behavior/output between hhvm and zend:
https://gist.github.com/asm89/9102248

[scannell]

(CC @jano / @paroski)

Thanks for reporting this. @jano is likely in the best position to comment about the future direction here.

[paroski]

Yup, we definitely want to fix this issue. There's no compelling reason to not match Zend PHP for this stuff.

[jano]

I believe our semantics introduced long before Zend's is better (in which sane world would current() run the first step of the generator and send() run 2 steps at once?), but I guess we missed that train by not providing feedback to Zend's generator RFC. The only reasonable step now is to match Zend.

[fredemmott]

Just checked all 3 examples, and we're still not matching. We're throwing fatal errors in some cases.

[jano]

@fredemmott: we need to emit extra if() check in Generator's send()/current()/key() functions, something like:

ContStarted // new opcode needed
JmpNZ started
Null
ContEnter
started:
<original body of send()/current()/key()>

[dave1010]

I'm manually calling next() for HHVM. This seems to be giving the same results but more testing is needed.

if (defined('HHVM_VERSION')) {
    $generator->next();
}

[dave1010]

Is there a way to account for this behaviour in a future-compatible way? Eg

if ($generator->hasBeenPrimed()) {}

At the moment I'm doing a fair few checks for HHVM_VERSION to get tests passing on hhvm but this will break if hhvm changes (which I assume it will at some point, otherwise the issue would be closed as wontfix).

[vicb]

@paroski @jano What are your plan to fix this ? This is a major difference in bevahior vs PHP.net.
Any ETA / plan for fixing this ?
Thanks !

[jano]

Sorry, I don't have any bandwidth to work on this in the foreseeable future :(.

If anyone is interesting in fixing this, I will be happy to help with code review and guidance.

[vicb]

@jano If you can give some quick guidance on where (which file) the change should occur, I may find some time to look at this. Thanks.

[jano]

@vicb That would be awesome!

Suggested solution:

• adjust logic of Generator's next()/send()/raise()/valid()/current()/key() methods implemented in emitGeneratorMethod() in compiler/analysis/emitter.cpp so that if the Generator was not started yet, they do an extra step; the emitted code would be something like:

ContStarted
JmpNZ started
Null
ContEnter
started:
// original code here

• the ContStarted opcode does not exist and needs to be implemented; its implementation should be very similar to the existing ContValid opcode, so you can grep codebase for ContValid, copy&paste most of the code and do few minor modifications (expected value of state; see runtime/ext/ext_generator.h)

[dave1010]

If HHVM's Generator is updated to match PHP's, isn't it going to be aa big BC break? Any ideas how this will be managed with the release?

[fredemmott]

• these cases aren't actually that common
• we're continually breaking backwards compatibility with previous versions of HHVM to improve compatibility with PHP5
• our external users should expect that - and mostly do, as their code almost always also runs on PHP5
• we've got a lot of experience fixing Facebook's internal code for these situations :)

For Facebook, we'll deal with that. For other code that's working around this currently, checking HHVM_VERSION is probably the best bet once it's fixed.

[vicb]

I had a free slot when I last commented but didn't manage to fit that in... Not sure when I can look at it again. I'll comment here when I can. Sorry.

[reeze]

we've got a lot of experience fixing Facebook's internal code for these situations :)

@fredemmott I am quite interested on this, how do you upgrade so large code base without breaks things? something like this may take a lot of time to upgrade, do testing. how do you guys reduce cost?

[ptarjan]

@reeze we have a pretty good unit test coverage, as well as canary builds which the employees use for a few days before it goes out to the public.

Also, since the introduction of Hack, the typechecker is really helpful for finding incorrectly transformed code.

The tools we use are codemod, spatch, and regular old perl -pi -e.

[JeroenDeDauw]

+1 I'm also running into this

[noony]

https://3v4l.org/CjaLS
https://3v4l.org/95Q6f
https://3v4l.org/Clno2

Seems ok in 3.11. Can we close ?

[fredemmott]

Yep, likely fixed at the same time as the PHP7 changes were implemented.