1905364 - Implement Promise.try

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement, P3)

Description

[Dan Minor [:dminor]]

Implementing the Promise.try proposal can likely be done completely in self-hosted JavaScript, making this a good project for someone new to contributing to SpiderMonkey.

Comment 1

[Martin Carolan]

Hi [:dminor],
I'm a first time contributor, and I'd love to pick this up if that's OK?

I've worked through Building Firefox on Linux and Building and testing spider monkey. I'm now familarising with the proposal, if you have any suggestions for the next step after that it would be really appreciated.

Thanks,

Martin

Comment 2

[Dan Minor [:dminor]]

Hi Martin,

Great, thanks for your interest :) For the implementation, I would take a look at this function that was added for the Promise.withResolvers proposal: https://searchfox.org/mozilla-central/rev/0b1d02b2cb5736511139cf0e40b318273e825899/js/src/builtin/Promise.cpp#4923, and the work that Tom did on this bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1845586. That should give you an idea of where to add the code and hopefully how to approach the implementation.

Please let me know if you have any questions, or get need some extra help getting things working :)

Daniel

Comment 3

[Martin Carolan]

Amazing, thanks for the pointers

Comment 4

[Martin Carolan]

Hi Daniel,

I wondered if you'd mind giving a few more points. So far I've added a JS_SELF_HOSTED_FN to promise_static_methods in js/src/builtin/Promise.cpp, and a new function:

/**
 * https://tc39.es/proposal-promise-try/
 */
function Promise_try(f, ...args) {
      //1. Let C be the this value.
      var C = this;

      //2. If C is not an Object, throw a TypeError exception.
      if (!IsObject(C)) {
        ThrowTypeError(JSMSG_INCOMPATIBLE_PROTO, "Promise", "try", "value");
      }

      /*
      3. Let promiseCapability be ? NewPromiseCapability(C).
      4. Let status be Completion(Call(callbackfn, undefined, args)).
      5. If status is an abrupt completion, then

          a. Perform ? Call(promiseCapability.[[Reject]], undefined, « status.[[Value]] »).

      6. Else,

          a. Perform ? Call(promiseCapability.[[Resolve]], undefined, « status.[[Value]] »).

      7. Return promiseCapability.[[Promise]].
      */
}

to js/src/builtin/Promise.js.

For

3. Let promiseCapability be ? NewPromiseCapability(C).

I was thinking that I'd need to expose NewPromiseCapability as an instrinsic function in js/src/vm/SelfHosting.cpp

Am I along the right lines so far, or way off piste?

Thanks,

Martin

Comment 5

[Dan Minor [:dminor]]

Sorry, I have been away and am just catching up on things. I'll try to have a look at this tomorrow.

Comment 6

[Dan Minor [:dminor]]

Hi Martin,

I'm very sorry about the delay here, this week was the TC39 plenary, and that's taken up a lot of my time.

I think your best bet is to add things directly to js/src/builtin/Promise.cpp because you will have direct access to NewPromiseCapability instead of having to expose it to self-hosted JavaScript. Here's a start of an implementation for you:

/**
 * https://tc39.es/proposal-promise-try/
 */
static bool Promise_static_try(JSContext* cx, unsigned argc, Value* vp) {
  CallArgs args = CallArgsFromVp(argc, vp);

  // 1. Let C be the this value.
  RootedValue cVal(cx, args.thisv());

  // 2. If C is not an Object, throw a TypeError exception.
  if (!cVal.isObject()) {
    JS_ReportErrorNumberASCII(cx, GetErrorMessage, nullptr,
                              JSMSG_OBJECT_REQUIRED,
                              "Receiver of Promise.try call");
    return false;
  }

  // 3. Let promiseCapability be ? NewPromiseCapability(C).
  RootedObject c(cx, &cVal.toObject());
  Rooted<PromiseCapability> promiseCapability(cx);
  if (!NewPromiseCapability(cx, c, &promiseCapability, false)) {
    return false;
  }

  // 4. Let status be Completion(Call(callbackfn, undefined, args)).

  // 5. If status is an abrupt completion, then
  // 5.a. Perform ? Call(promiseCapability.[[Reject]], undefined, « status.[[Value]] »).

  // 6. Else,
  // 6.a. Perform ? Call(promiseCapability.[[Resolve]], undefined, « status.[[Value]] »).
}

The next steps would be to figure out how to use Call() to call callbackfn with the appropriate arguments. I think after that you can use CallPromiseRejectFunction and CallPromiseResolveFunction to handle the reject and resolve cases. You should be able to find lots of examples in Promise.cpp. If you're not aware of it, we have a code search engine that can help find existing uses of these functions, e.g. here's a search for CallPromiseResolveFunction.

I hope this helps! Again, I'm really sorry about the delay here. Please let me know if you need any more help :)

Comment 7

[Martin Carolan]

No problem whatsoever. Thanks a lot for the help, I think that will set me on the right track. I'll give it a shot and let you know!

Thanks

Comment 8

[Martin Carolan]

Attachment: Bug 1905364 - Implement Promise.try. r?dminor

Comment 9

[Dan Minor [:dminor]]

Hi Martin, how are things going on this? If you're stuck, please let us know, and we'll do our best to unblock you :)

Comment 10

[Martin Carolan]

hey Dan,

Thanks for the message and sorry it's dragging a bit!

I just replied to the phabricator request. As I am a bit stuck trying to get my head around request for two extra requests. Any pointers would be appreciated!

Thanks,

Martin

Comment 11

[Dan Minor [:dminor]]

It looks like Arai is helping out over in phabricator, please let me know if there's anything I can do.

Comment 12

[tannal2409]

Hey, Martin,
I don't have the permission to comment on the reversion.
So I comment here.

https://tannal.github.io/comment.png

Thanks,

tannal

Comment 13

[Martin Carolan]

Thank you tannal,

I just gave that a go but couldn't get it to work.

I think the issue is that it is calling the callback function with a single argument that is an array, rather than packing the original arguments into separate InvokeArgs arguments?

Some more context below in case I've missed something obvious

...
  // 4. Let status be Completion(Call(callbackfn, undefined, args)).
  size_t argCount = args.length();
  if (argCount > 0) {
    argCount--;
  }

  HandleValueArray iargs = HandleValueArray::subarray(args, 1, argCount);

  HandleValue callbackfn = args.get(0);
  RootedValue rval(cx);
  bool ok = Call(cx, callbackfn, UndefinedHandleValue, iargs, &rval);
...

An example of one of the failing tests:

## test262/built-ins/Promise/try/args.js: rc = 0, run time = 0.019557
 FAILED!  TypeError: Promise is not a function
REGRESSION - test262/built-ins/Promise/try/args.js

Comment 14

[tannal2409]

I think the issue is that it is calling the callback function with a single argument that is an array, rather than packing the original arguments into separate InvokeArgs arguments?

Yes, we should copy them.
HandleValueArray is not a good idea.XD

Comment 15

[Martin Carolan]

Was good to try though, thank you

Comment 16

[Martin Carolan]

Hi Dan,

Thanks for the review and help :)

I tried to land it but get

You have insufficient permissions to land. Level 3 Commit Access is required. See the FAQ for help. Has an open ancestor revision that is blocked. The requested set of revisions are not landable. 

What are the next steps now?

Thanks,

Martin

Comment 17

[Dan Minor [:dminor]]

Hi Martin,

I'm trying the changes on CI right now: https://treeherder.mozilla.org/jobs?repo=try&selectedTaskRun=a2jHKcpHSbq7KWlMGRtRLA.0&revision=ad9985f7c143beb7c1b35b7f170fb473a63d407e.

I'll land the changes for you once that is complete, it looks fine.

Comment 18

[Dan Minor [:dminor]]

Hi Martin, unfortunately, your changes need to be rebased before I can land them: https://lando.services.mozilla.com/D218494/.

Comment 19

[Martin Carolan]

Thanks Dan, I've done that and resubmitted to phabricator. Was that the right thing to do?

Cheers

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d9d53f44c7f5
Implement Promise.try. r=dminor,arai

Comment 21

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/d9d53f44c7f5