Editorial: spec refactor of PromiseReaction records/PromiseReactionJob #584
Conversation
The current spec creates fictional “equivalent functions” called “Identity” and “Thrower”, and passes these around with `PromiseReaction` records. Instead, this change adds a string “Type” field to `PromiseReaction` records, and sets the “Handler” field to `undefined` when none is provided, so that the behavior can be determined solely inside `PromiseReactionJob`. This change should be non-observable, and helps future Promise proposals.
|
I don't think the result here is much cleaner. Can you elaborate on how this helps I'd like to think a little bit about refactorings in general before committing them, as some implementations (including V8) are working to make some of our code parallel to the spec text. Churn, if it doesn't "pay for itself" by making things cleaner or more possible to take a future change, increases our maintenance burden, either by getting us out of sync from the spec or letting us change our code to match the new spec. There is still a bit of duplication in the Promise spec, and if there is a refactoring and we do want to pay for this churn, it might be nice to clean that up around the same time. cc @gsathya |
|
@littledan no, The spec text for |
|
Oh, I see, there are multiple lists still and the reaction type recorded. Seems like the information is held slightly redundantly. One list can have the fulfills or finallys, and the other list can have rejects or finallys. What if you accomplished this instead by having a bit indicating whether the reaction is a finally reaction? Or, for an even more localized change, just wrap the finally callback in something that returns/rethrows its input? |
|
That bit is the "Type" field on the PromiseReaction record. The finally spec is a bit more complex than that - it simply can't be represented by a single callback function at the time the "Thrower" and "Identity" are defined. |
|
Put another way, a "finally" reaction is both a fulfill and a reject reaction, so it pushes a function onto both stacks. |
|
I guess by 'bit' I meant one that doesn't redundantly include whether it's a fulfill or reject, just whether it's finally or not (since this information is redundant). Or, if you do include that redundant information, then there should be a lot of assertions that it's all redundant. I don't really see what pushing on both stacks has to do with whether you could specify it through wrapping the user's callback to avoid adding a representation of this bit. |
|
@littledan I'd love your thoughts on http://ljharb.github.io/proposal-promise-finally/ - this PR grew out of that spec text, and I feel pretty strongly that this PR is the right direction for it to go. If you could help me tweak the proposal text to use the "bit" you have in mind, I'd be happy to tweak this PR in kind. In general, though, I think it's important that we are free to do nonobservable spec refactors at any time, and that an implementation's choice to make their code look like the spec text not weigh in to how we word things. Spec refactors should be low cost. |
| 1. If _handler_ is *undefined*, then | ||
| 1. If _type_ is `"Fulfill"`, let _handlerResult_ be NormalCompletion(_argument_). | ||
| 1. Else if _type_ is `"Reject"`, let _handlerResult_ be Completion {[[Type]]: ~throw~, [[Value]]: _argument_, [[Target]]: ~empty~}. | ||
| 1. Assert: _handlerResult_ is defined. |
There was a problem hiding this comment.
This assert seems unhelpful as "defined" is not a real term.
There was a problem hiding this comment.
I wanted some kind of assertion so that implementations know that no other _type_ value is expected. How else could I word it?
There was a problem hiding this comment.
If you want that, I'd change the second bullet to "Otherwise, type is "Reject". Let handlerResult ..."
|
I think this is a good change, as it keeps the types more straight instead of having [[Handler]] be sometimes a function and sometimes a string that triggers special behavior. Instead the special behavior is kept in a sideband. In other words, right now the [[Handler]] slot is reused as a weird kind of union and you do type-testing on it to figure out what it's being used for. I'd rather keep that type information in an explicit field instead of storing both strings and functions there and branching based on whether it's a string or function. One of the first things an implementation does to optimize promises is presumably make promise reaction records to be much more lightweight than the spec. E.g. using a globally-reused identity/thrower function. I can't imagine this being any more harmful for implementations than the current spec is, which encourages polymorphic behavior for one of the most important fields in the promise spec. |
ljharb
force-pushed
the
promise_spec_tweak
branch
from
fea52ac to
559a421
Compare
|
I find this refactoring easier to understand as well. I might prefer a version where [[Handler]] is never undefined and we store a ginned-up identity/thrower function in the slot at appropriate times. In this case the finally spec could easily get by with an [[isFinally]] slot as resolution and rejection reactions would be handled uniformly. But as it stands this is an improvement so I'm happy to take it. |
| Either `"Fulfill"` or `"Reject"`. | ||
| </td> | ||
| <td> | ||
| The [[Type]] is used when [[Handler]] is *undefined*, to allow for behavior specific to the settlement type. |
There was a problem hiding this comment.
To me it makes the prose read better, but sure, I can remove it.
ljharb
force-pushed
the
promise_spec_tweak
branch
from
559a421 to
636aec2
Compare
This unobservable spec refactor allows the `Promise#finally` proposal to make a much smaller change, so it can reuse some of the logic in PerformPromiseThen. (following up on tc39#584)
Per tc39/ecma262#584, update spec diff.
The current spec creates fictional “equivalent functions” called “Identity” and “Thrower”, and passes these around with
PromiseReactionrecords.Instead, this change adds a string “Type” field to
PromiseReactionrecords, and sets the “Handler” field toundefinedwhen none is provided, so that the behavior can be determined solely insidePromiseReactionJob.This change should be non-observable, hopefully adds a bit more clarity, and helps future Promise proposals have smaller diffs against the spec.