Zend incompatibility: yield syntax for sending values to generator

[stuartcarnie]

The following code fails to compile with hhvm

<?php

function get_gen() {
    echo "Starting generator\n";
    $a = yield;
    echo $a . "\n";
}

$gen = get_gen();
$gen->send(1);

Returns (hhvm):

HipHop Fatal error: syntax error, unexpected ';' in /home/contatta/tmp/generator_test.php on line 5

By adding a simple expression (which is not compatible with Zend), it then complains ->next() must be called, which is also incompatible with Zend:

<?php

function get_gen() {
    echo "Starting generator\n";
    $a = yield 0;
    echo $a . "\n";
}

$gen = get_gen();
$gen->send(1);

Returns (hhvm):

HipHop Fatal error: Uncaught exception 'Exception' with message 'Need to call next() first' in :
Stack trace:
#0 /home/contatta/tmp/generator_test.php(10): Continuation->send()
#1 {main}

The following is compatible with Zend (but not hhvm), and does not require ->next() be called:

<?php

function get_gen() {
    echo "Starting generator\n";
    $a = (yield 0);
    echo $a . "\n";
}

$gen = get_gen();
$gen->send(1);

Returns (hhvm):

HipHop Fatal error: syntax error, unexpected T_YIELD in /home/contatta/tmp/generator_test.php on line 5

Returns (Zend):

Starting generator
1
End

[scannell]

Our syntax is definitely different here. @paroski or @elgenie would be the best people to comment on whether this is an intentional difference.

[paroski]

The different is not intentional, we just haven't done the work yet to make "yield" work as a valid expression in various grammatical contexts. Right now, I think HHVM only supports "yield ..;" (as a statement by itself), "$x = yield ..", and "list(..) = yield ..".

It would be great to improve HHVM's parser/frontend to support using "yield" in other grammatical contexts. Pull requests welcome :)

[evert]

It would be great if the priority for this could be bumped!

The following two expressions currently give a fatal error for me:

function() {
   yield;
}

function() {
   $foo = (yield "hello");
}

[ezzatron]

Yielding a key is also currently broken I think:

$a = function () {
    yield 'x' => 'y';
};

(link)

and

$a = function () {
    $x = (yield 'y' => 'z');
};

(link)

both result in syntax errors for me in HHVM 3.4. Seems to be a regression on the first one as well, which appears to work in 3.3 (according to 3v4l.org)?

[paulbiss]

I have a diff I'm working on to add full support for yield expressions, I'll make sure the regression gets looked at. It should fix the other example as well (note that for now if you remove the parens around the yield it should work fine in hhvm but fail in PHP 5).

[ezzatron]

Awesome to hear! :thumbsup:

[SiebelsTim]

@paulbiss What happened to this? :)

[paulbiss]

Ran into a couple of issues with the way that we guard on stack types and couldn't decide on how it should behave for await. I'll try to get back to this.

[mtdowling]

I'm also running into this issue in the AWS SDK for PHP.

Fatal error: syntax error, unexpected T_YIELD in /home/travis/build/aws/aws-sdk-php/src/ResultPaginator.php on line 68

Why is this considered a "low-pri" issue?

[paulbiss]

We don't really use the priorities to track issues anymore beyond wishlist generally meaning that no one has plans to work on it. The TL;DR here is I plan to return to this once I reach the end of a long list of other priorities I have unless someone else wants to pick it up.

[dieend]

I also run into this problem when using AWS SDK for PHP, when trying to access instance profile credentials..

[EwanValentine]

@dieend Same here!

[steveh]

It's a documented issue on AWS http://docs.aws.amazon.com/aws-sdk-php/v3/guide/faq.html#does-the-sdk-work-on-hhvm

Does SDK v2 work?

[EwanValentine]

v2 works with HHVM, but v2 doesn't work with Laravel 5.1 unfortunately. I'll have to try the nightly HHVM build or revert back to php-fpm for now

[steveh]

@EwanValentine thanks for the info - what's the incompatibility with Laravel and v2?

[EwanValentine]

@steveh - Laravel uses league/flysystem-aws-s3-v3" as its main file storage adapter, which requires v3. Which then throws the compatibility issues with yield

[jeremeamia]

This is causing problems for a few different libraries:

• Guzzle 6 (and Guzzle Promises)
• Icicle
• AWS SDK for PHP (Version 3)
• Recoil

Basically, it makes anything that attempts to do coroutines impossible.

[peppy]

This is a pretty serious issue for multiple Laravel packages, as mentioned above. Not sure it should still be low priority.

[paulbiss]

From above:

We don't really use the priorities to track issues anymore beyond wishlist generally meaning that no one has plans to work on it. The TL;DR here is I plan to return to this once I reach the end of a long list of other priorities I have unless someone else wants to pick it up.

I honestly don't know why we haven't gotten rid of that label, I'm getting pretty tired of explaining that it's basically meaningless (as are the other priorities)...

[evert]

@paulbiss regardless of it being meaningless or not, it would appear that the low-pri label has been pretty accurate given the speed in which this issue is addressed.

18 months and counting.

[paulbiss]

Yeah, and I'm really sorry about that. There are only so many of us, and unfortunately no one has the bandwidth to do this right now. This is most certainly on my radar but it's going to be a non-trivial change.

In general the best way to get something prioritized is to send us a patch for it. We absolutely plan to look at this, but I can't drop everything I'm working on right now to do it. While I can understand your frustration this is by no means the oldest issue and I wouldn't characterize it as low-priority. The open source team simply isn't large enough to offer any kind of SLA on issues. Turnaround on reviewing pull-requests, is usually days to weeks, issues (particularly complicated issues) are generally open much longer.

[evert]

Totally get that. There's only so much you can do, and I am grateful for what you guys have done. I think the frustration with things like this is not the fact that it's not done yet, but rather the lack of transparency.

Labeling things by priority accurately would be a great help =)

[jeskew]

@paulbiss Where would you advise someone writing a patch for this to start looking?

[paulbiss]

@jeskew I had a patch some months ago that was still had some bugs to iron out. At this point there's little chance you'd be able to rebase it but I can try to dig it up if you want to use it as a guide.

@evert That's a fair point. We usually try to indicate when we're working on something (comments on the issue, assignments etc), and when we don't ever plan to work on something (wishlist, won't fix), but assigning some priorities could be useful. Part of the problem is that not all of the work we do comes out of the issue queue.

[jeskew]

@paulbiss That would be great.

[r3wt]

aww bum, so much for async uploads to s3

[paulbiss]

@jeskew here's the patch as it was the last time I had a chance to work on it: https://reviews.facebook.net/D25821 @r3wt I don't follow? s3?

[evert]

@r3wt is just talking about a very specific use-case that is presumably implemented by some code that takes advantage of this feature. Mostly a completely unhelpful comment.

[fredemmott]

Amazon's S3 PHP SDK depends on yield behavior that HHVM does not support.

[Orvid]

As a note, PHP7 does bring support for the non-parenthesized version, https://3v4l.org/pRDr9

[rowillia]

@fredemmott Which parts of yield behavior does HHVM not support? Once https://github.com/facebook/hhvm/commit/48f0982c35a4ff7ef29088ae62edb62a9974e9d7 and https://github.com/facebook/hhvm/pull/4915 hit we should have all of the behavior we need. If parsing works then HHVM should work out of the box, right?

[fredemmott]

No idea, was just adding more information re @r3wt 's comment.

[rowillia]

@fredemmott Ah OK. We're close though! I think it'd be pretty amazing for HHVM to support the aws-sdk, it would really help a bunch of startups that want to adopt HHVM :).

[Jameron]

I'm really interested in seeing this issue resolved. I moved my entire code base from Laravel 4.2 on Apache (9 Requests Per Second) to Lumen NGinx/FPM (18 RPS), then from FPM to HHVM (100RPS), needed an authentication system back so I moved the code base over to Laravel 5.1(71 RPS), realized AWS-SDK was not working, and went back to NGinx/FPM at around 18 RPS. Would be great to get it back to 71 RPS!

[fredemmott]

@scottrice, does your work touch this?

[jwatzman]

Some (but I think not all) of this will be solved by @nikic and @paulbiss's generator priming diff, which has been accepted internally and will land real soon now (working through some deployment issues on the FB side, since we rely on the incorrect behavior).

[shadowhand]

A lot of new code relies on Guzzle and the AWS-SDK these days. Looking forward to seeing this fixed!

[jwatzman]

@shadowhand can you try again with a recent nightly build? A couple of fixes have landed in the last couple of weeks (more recently than the last stable release), and I'm hopeful this is fixed now, or at least in a better state?

[fredemmott]

All the examples given in the original are fixed in 3.11, probably as part of the PHP7 work:

https://3v4l.org/rFJWQ https://3v4l.org/pRDr9 https://3v4l.org/0qWQW

If yield-related incompatibilities still exist, please open a separate issue with example code (small, isolated examples like this issue has are ideal).