Implement promises #942
Implement promises #942
Conversation
|
This will fix #160 from back when I was a young boy. |
|
Make sure you actually link this to #160 so that it closes it 😃 I haven't looked closely yet, but does this include a lot of the same changes regarding the new LambdaConstructor stuff that was in your NativeError PR that was closed? |
|
Nice! As for the Java interop stuff: maybe this gives some inspiration: the Promises section at GraalVM JavaInteroperability (also see Asynchronous polyglot programming with Java and JavaScript on GraalVM) or at a higher level: Java Promise Just as FYI:
Shall I create follow-up cases for both of these? |
This adds the NativePromise class, supporting Promise.all, race, resolve, and reject, plus catch, finally, and then on the prototype.
Also, update to the latest test262 infrastructure.
gbrail
force-pushed
the
greg-promises-2021
branch
from
084abd5
to
29a028b
Compare
| @@ -4863,6 +4863,29 @@ public static JavaScriptException throwCustomError( | |||
| return new JavaScriptException(error, filename, linep[0]); | |||
| } | |||
|
|
|||
| /** | |||
| * Implement the ECMAScript abstract operation "SpeciesConstructor" defined in section 7.2.33 of | |||
There was a problem hiding this comment.
Consider moving this to o.m.j.AbstractEcmaObjectOperations, as it is one of the Abstract Object Operations defined by EcmaScript. When you do, please add a first param of type Context (as per the instructions outlines at the top of o.m.j.AbstractEcmaObjectOperations): while the function might not need Context atm, it might in the future
Consider adding a link to the specific section in the specific version of the EcmaScript spec implemented: the codebase is full of textual references to a section in the spec, but as the spec evolves, new chapters get added and things get renamed, so textual reference without context become meaningfull. A link to ES2021 Species Constructor however will (hopefully) remain valid and allows someone looking at this code x years from now to figure out the delta between the specific version implemented and the latest spec at that point in time
gradle.properties
Outdated
| @@ -4,5 +4,4 @@ version=1.7.14-SNAPSHOT | |||
| buildDir=buildGradle | |||
| mavenSnapshotRepo=https://oss.sonatype.org/content/repositories/snapshots | |||
| mavenReleaseRepo=https://oss.sonatype.org/service/local/staging/deploy/maven2/ | |||
| org.gradle.caching=true | |||
There was a problem hiding this comment.
Is this an intentional change? Because it means for example that when running ./gradlew jacocoReport, it wont do that on the cached results of the last testrun, but instead needs to run all the tests again (I think).
If this is intentional and will stay this way, the docs for updating the test262.properties file need a slight adjustment, because it now states you need to run with the --rerun-tasks param, because the -DupdateTest262properties option doesn't work based on cached results. So, if we're not doing caching anymore, the --rerun-tasks param is no longer needed
There was a problem hiding this comment.
I'll take this out of that particular PR. But I have been finding that the SpotBugs plugins are failing randomly on my machine and it seemed to be caching related -- I'd otherwise like to leave it on.
testsrc/test262.properties
Outdated
| @@ -1144,7 +1585,6 @@ built-ins/RegExp 926/1464 (63.25%) | |||
| prototype/Symbol.replace/y-init-lastindex.js | |||
| prototype/Symbol.replace/y-set-lastindex.js | |||
| prototype/Symbol.search/cstm-exec-return-index.js | |||
| prototype/Symbol.search/failure-return-val.js | |||
There was a problem hiding this comment.
I'm assuming that this is fixed because I didn't check in the file after merging the last PR that fixes a bug in Regexp.
There was a problem hiding this comment.
I noticed that one too today...
Yeah, if #931 fixed something due to which this test now started to pass and that was not accounted for in that PR, if you now rebased your Promises PR on the latest master and regenerated the .properties file, it makes sense that this delta now pops up
There was a problem hiding this comment.
Just in general (and we can open a new issue for this) -- if someone writes code that breaks test262 tests, I know that the tests will fail the first time, but will the test suite now "fix" test262.properties to indicate that there's a new set of failing tests?
If so, then we will have to monitor every incoming PR to see if it inadvertently disabled stuff in test262.properties.
There was a problem hiding this comment.
If someone writes code that breaks tests and then they actively regenerate test262.properties (which is a manual thing they have to do, by running the test with the extra -DupdateTest262properties argument) and then ignore the delta in test262.properties and put it up as a PR, yeah, then, if if the reviewer also doesn't pay attention, new broken tests might sneak in.
But that is not something new imho: before someone could do something like that manually, by just adding the broken tests to be excluded through test262.properties and put that up for PR.
There was a problem hiding this comment.
We could do something to prevent stuff like the prototype/Symbol.search/failure-return-val.js issue above: we could fail the build if tests excluded in test262.properties aren't failing
Or at least do so on the CI server.
Note that I've got #951 to bring back output of tests that pass, but are marked as expected to fail in test262.properties. We had that before, but I managed to get it lost and I just noticed today
There was a problem hiding this comment.
I think we should document them -- for instance, when adding promises I had to edit the file directly, and that would have had to trigger a "full regen" of the properties file, versus just removing exclusions.
There was a problem hiding this comment.
To be clear, when we updated the test262 stuff, I somehow thought that the properties file re-gen was going to be optional, but it seems to happen every time I do a "gradle check" which is why I wanted to have some more control.
There was a problem hiding this comment.
It is optional... or so it should be
The regenerate should only happen when the -DupdateTest262properties argument is added to ./gradlew tests and not under any other circumstance
There was a problem hiding this comment.
Double-checked: the code to update the test262.properties file is only triggered when the updateTest262properties System property is set, so not sure how it would get triggered when you run gradle check...
There was a problem hiding this comment.
Indeed, when running the tests through gradle, the regen of test262.properties was always triggered. Added a fix in #951
|
Will check by replacing HtmlUnits Promise impl with this one - but this requires some time. Hope i have it done in two weeks from now. |
|
Status update -- I'll be taking a break for the next two weeks but when I get back in mid-July I'll merge this unless anything else has come up. |
|
Enjoy your break! Just wondering: could JavaScript Promises maybe be a standard replacement for Continuations as Rhino supports them? Feel they are somewhat similar, but not sure how similar and if promises could actually replace continuations... |
|
@gbrail does this still look good to merge to you? |
|
Sorry -- I've been back but working on other things. I'm currently a bit
slow because I'd like the "compat-table" tests to work with this feature,
and for that I need a working "setTimeout," which I did but which doesn't
fix the problem yet. A few days more I hope!
…On Thu, Aug 5, 2021 at 5:56 AM Samuel Gaus ***@***.***> wrote:
@gbrail <https://github.com/gbrail> does this still look good to merge to
you?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#942 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAD7I25Z2OMVT6UETIKVQHTT3KDBNANCNFSM47ES57KQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
|
@gbrail found some time to try your impl with HtmlUnit - looks promising :-) But some of my tests are failing - looks like problems with the scope/thisObj. Will make some pr's. |
|
@gbrail anything you can share for the release notes on Java interoperability for Promises? Is there any? If not, I'll create a followup task Also see #942 (comment) |
|
just for the records - i finally found some time to do the switch from HtmlUnits own promise implementation. |
This is a working implementation of promises, built using the new LambdaConstructor stuff. It supports:
Promise.all, resolve, reject, and race
Promise.prototype.catch, finally, and then
Promise.allSettled isn't implemented yet -- it seems to be a bit newer and some of this code is a year old or more. That can come soon.
This is a little bit of a naive implementation in that it follows the spec closely. There might be some optimization possible because we are currently not taking advantage of the fact that most of the time, we are calling various methods of NativePromise, but because this is JavaScript we have to look up methods like "then" every time in case they change.
I think that given the amount of Java interoperability people do with Rhino, we also need some way to implement a Promise in Java code, or at least call the various methods to resolve the promise easily from Java. That could be a future PR. I'm hoping that some people who do the deep embedding with Java some more will have ideas of a good way to do it.
Closes #160