Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for Closure::bind is missing #1203

Closed
stof opened this issue Oct 21, 2013 · 32 comments
Closed

Support for Closure::bind is missing #1203

stof opened this issue Oct 21, 2013 · 32 comments

Comments

@stof
Copy link
Contributor

stof commented Oct 21, 2013

PHP 5.4 introduced Closure::bind() and Closure::bindTo() but they are not implemented in HHVM. This prevents some tools from working (even when they do a version check as HHVM reports a PHP version higher than 5.4).

See phpspec/prophecy#62 for such a report

@scannell
Copy link
Contributor

@paroski can you comment here? I seem to remember some specific difficulties with these.

@paroski
Copy link

paroski commented Oct 28, 2013

We can implement these, though using Closure::bind() and Closure::bindTo() will be incompatible with RepoAuthoritative mode.

@ghost ghost assigned ptarjan Oct 28, 2013
@henrikbjorn
Copy link

what is RepoAuthoritative mode ?

@danslo
Copy link
Contributor

danslo commented Nov 12, 2013

http://www.hhvm.com/blog/257/go-faster

Scroll down :)

@Majkl578
Copy link
Contributor

Bump. Closures binding is in PHP since 5.4.0 so I think HHVM should support it as well. And maybe put a red warning somewhere about the incompatibility with RepoAuthoritative. (Btw. isn't HipHop's goal to reach parity with Zend?)

@scannell
Copy link
Contributor

It is on the list of things to do for compatibility work but not that high at the moment. If someone wants to implement these without RepoAuthoritative support (which is parity with constructs like create_function() and eval()) before we do we'd be happy to review a PR.

@Ocramius
Copy link
Contributor

@paroski what is the reason for Repo.Authoritative incompatibility? May help in understanding the issue there

Also - does work on ext_closure.cpp and ext_closure.h suffice to get this running (besides tests, ofc)?

@scannell
Copy link
Contributor

I believe @paroski is on vacation and will be back after the US holidays.

Fundamentally, this feature is incompatible with RepoAuthoritative (with WholeProgram) mode where all dynamic code constructs are banned -- it is not an issue with a particular implementation. This is fine though; HHVM also supports eval(), create_function(), etc. but not in RepoAuthoritative.

I am guessing getting this to work is going to require diving more deeply into the internals than just the closure extension but I'm not entirely sure as I haven't looked at implementing this.

@scannell
Copy link
Contributor

This was an attempt (which probably needs to rebased) at this if anyone wants to pick it up: https://gist.github.com/scannell/24b3b018f7c1954991b0

It didn't work as-is; the JIT currently asserts.

@bountysource-support
Copy link

Facebook has posted a $200 bounty on this issue. The bounty will be paid out to the developer who closes the issue.

@paroski
Copy link

paroski commented Feb 1, 2014

We can probably get this implemented some time in the first half of 2014 if no one else gets around to it.

@milesj
Copy link
Contributor

milesj commented Feb 13, 2014

Just ran into this issue today. Probably the only error popping up in my hhvm unit tests.

@vlucas
Copy link

vlucas commented Feb 14, 2014

I need support for this. Bullet uses it extensively.

@jails
Copy link

jails commented Apr 12, 2014

No news on this subject ?

@lisachenko
Copy link
Contributor

@ptarjan, @paroski, @sgolemon ping ) This issue probably the main stopper for many libraries to run under HHVM.
Even the bounty of $200 can not help to solve this issue :)

@Ocramius
Copy link
Contributor

@lisachenko IIRC, someone at FB was working actively on this (read about it last week or so)

@LiraNuna
Copy link
Contributor

Sadly this has become a large task for me, here's a gist if anyone is interested in continuing my work: https://gist.github.com/LiraNuna/b17303992428a34dad57

Mind you that not all tests pass right now, while all the tests that answer this regex should pass:

hphp/test/zend/good/Zend/tests/closure_0*.php

@Ocramius
Copy link
Contributor

@LiraNuna argh, you were my only hope here :-( Is there anyone experienced with the JIT that can actually help on this? It is really the last major blocker for moving a lot of libraries to full HHVM support :\

@danielstjules
Copy link

Looks like @Ocramius upped the bounty to $300!

@Ocramius
Copy link
Contributor

Ocramius commented Sep 6, 2014

@danielstjules it's not like it will help much :-)

@fredemmott
Copy link
Contributor

I think @paulbiss is currently working on this.

@Majkl578
Copy link
Contributor

@paulbiss: Great job! One huge incompatibility less. :)
Any info whether this will land in 3.3.0 or not?

@simonwelsh
Copy link
Contributor

@Majkl578 It won't. 3.3.0 has already been delayed 4 days, and that's for crashers rather than missing features.

@paulbiss
Copy link
Contributor

/cc @fredemmott
There's zero chance of this making our internal release, but I would support cherry-picking it for 3.3.0.

@Ocramius
Copy link
Contributor

W00t! Going to compile and try it out later today \o/

@lisachenko
Copy link
Contributor

Great job, @paulbiss! And jackpot! :) I will also try a nightly-build later

@Ocramius
Copy link
Contributor

@fredemmott
Copy link
Contributor

Sorry, this won't be going into 3.3.0 - our internal and external releases don't diverge, except for build system fixes etc.

@paulbiss
Copy link
Contributor

@Ocramius that's very generous of you, but I can't accept a bounty for doing my job. :-P It would be a conflict of interest, and I worry it sends the wrong message about how we prioritize issues from the community. Would it be possible to apply it to another issue you care about? I think it's a terrific way to involve others in the project.

Also, please open issues for any bugs you find with the feature in your testing. We don't have great test coverage for it so I wouldn't be surprised if bugs slipped through. Thanks!

@Ocramius
Copy link
Contributor

Would it be possible to apply it to another issue you care about? I think it's a terrific way to involve others in the project.

Fine by me, I can simply revoke the bounty if it's allowed by that site. I really don't care about other issues right now :-)

Also, please open issues for any bugs you find with the feature in your testing. We don't have great test coverage for it so I wouldn't be surprised if bugs slipped through. Thanks!

Will do as soon as I get it built.

@Ocramius
Copy link
Contributor

Before:

Tests: 384, Assertions: 1046, Failures: 17, Skipped: 24.

After:

Tests: 403, Assertions: 1207, Skipped: 2.

I'd like to call that a great success :-)

@bobthecow
Copy link

👍

rrh pushed a commit to newrelic-forks/hhvm that referenced this issue Sep 24, 2014
Summary: Added support for Closure::bind and a runtime option to disable
mutating closure scopes. Closes facebook#1203

Reviewed By: @ptarjan

Differential Revision: D1538668
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests