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

Where should we assimilate? #128

Open
erights opened this issue Jun 9, 2013 · 12 comments
Open

Where should we assimilate? #128

erights opened this issue Jun 9, 2013 · 12 comments
Milestone

Comments

@erights
Copy link

erights commented Jun 9, 2013

At http://lists.w3.org/Archives/Public/public-script-coord/2013AprJun/0598.html Tab describes well his AP2 proposal. Until AP2, if we leave aside the handling of promises-for-promises, generally the proposals have largely stayed within the subset currently specced by Promises/A+. But AP2 is a) both very attractive, as a compromise that has a good chance of acceptance in TC39, and b) even aside from promises-for-promises, would require an incompatible change to where we assimilate thenables. IMO, the change would break very little code today, so if we're going to consider it, now is the time.

I will phrase the suggestion in terms of a more complete Promise API than the subset specced by Promises/A+, such as by Q and by https://github.com/slightlyoff/Promises. This more complete API has additional elements that are rapidly becoming consensus elements as well:

  • A promise constructor that takes a callback, which it calls with at least a resolve function and a reject function, passed somehow. The promise constructor returns the promise whose resolution is controlled by the arguments it passed to this callback.
  • A static resolving operation, which for Q is the Q function itself, and for https://github.com/slightlyoff/Promises is spelled Promise.resolve.

These two extensions have both been discussed here on Promises/A+ as well. Since we're leaving aside promises-for-promises in this question for now, we don't need to worry yet about additional proposed API elements like .accept, .fulfill, .chain, or .flatMap.

Currently, the consensus place to do assimilation is in the static .resolve, the resolve function passed to the constructor callback, and in the return pathway of .then -- in the handling of the return value from .then's callbacks. The really clever thing about AP2 is that it suggests doing assimilation instead in the notification pathway of .then -- not calling .then's onFulfilled callback until a non-thenable value has been reached. With assimilation there, we'd no longer need assimilation elsewhere. The resolve functions need only do one level of flattening, and no assimilation at all. Were we to do this, we should define the notion of "fulfilled" in terms of reaching the end of the assimilation chain.

So, big question: Should Promises/A+ make this change? If TC39 is likely to adopt this change, I would really really like it and its implications to be discussed here soon.

We also need to discuss the implications of allowing promises-for-promises. However, I would like to do that elsewhere. In this thread, I would like to explore this shift in assimilation still under the assumption that there are no promises-for-promises.

@domenic
Copy link
Member

domenic commented Jun 9, 2013

Thanks for bringing this up. I'd thought of doing so a few times, but kept coming back to the question: does it make any operational difference in the semantics we have specified in Promises/A+? I.e., without promises-for-promises and their associated machinery, I'm not sure this shift actually changes any part of the spec.

The really clever thing about AP2 is that it suggests doing assimilation instead in the notification pathway of .then -- not calling .then's onFulfilled callback until a non-thenable value has been reached.

Case in point, I am pretty sure this is operationally indistinguishable from what we have specced now. Since assimilation and one-level flattening occur immediately prior to fulfilling the promise with the extracted value (per the [[Resolve]] algorithm), and fulfilling a promise immediately results in calling onFulfilled with the fulfillment value, it seems equivalent.

There is definitely a large shift in conceptual phrasing though, I agree. E.g., I think we would eliminate or at least deemphasize the state of "fulfilled", instead talking about the fates "unresolved" vs. "resolved". But I can't find the operational difference between:

  • Current Promises/A+: starts out in state pending; someone resolves the promise; when the resolved-to promise unwraps enough to yield a non-thenable value, state becomes fulfilled; when fulfilled, call onFulfilled
  • AP2: starts out with fate unresolved; someone resolves the promise; fate becomes resolved; when the resolved-to promise unwraps enough to yield a non-thenable value, call onFulfilled.

I think even in a universe that contains a Promise constructor which takes a resolve function, and the easily-derived Promise.resolve operation, you still couldn't distinguish. I think only with the introduction of promises-for-promises can you distinguish, because then the until-now-internal concepts of "resolution value" become important.

Am I wrong?

@domenic
Copy link
Member

domenic commented Jun 9, 2013

The AP2 idea seems a bit less elegant to me anyway, as it would, I think, require memoizing the unwrapping results so that subsequent calls to then receive the same fulfillment value.

@juandopazo
Copy link
Contributor

I don't think it has an impact on A+, but it will on implementations that provide a way to inspect the status of a promise.

@erights
Copy link
Author

erights commented Jun 9, 2013

In a world with getters and proxies, or even of time varying .then values (see below) it becomes observable when the .then is sampled.

In AP2, it would not be sampled on .resolve or on onFulfilled's or onReject's return. It would be sampled at the very last moment it could possibly be -- prior to calling onFulfilled. This change almost completely repairs the counter-example I posted previously to the claim that, with assimilation, onFulfilled will never be called with a thenable argument:

var notYetThenable = {};
Q(notYetThenable).then(shouldntBeThenable => ....);
notYetThenable.then = function....;

(Where Q(x) might be written Promise.resolve(x))

Under all proposals prior to AP2, shouldntBeThenable will be thenable.

Under AP2, .then would notice that notYetThenable is a thenable in the turn when it would otherwise have invoked onFulfilled. There's no reason for it to have even checked prior to this, so there's no extra work. .then would never notice that notYetThenable was ever not thenable.

The only remaining way to still violate the claim that I can see is for notYetThenable's .then property to be an accessor whose getter makes notYetThenable into a thenable next time. This remaining case is now sufficiently unlikely as to be beyond accident. If there were a reason to do this in an attack, we have not prevented the attack, so defenders cannot treat this regularity as a platform enforced invariant (which was unlikely anyway). But we've repaired all reasonable possibility of this hazard causing an accidental bug.

@erights
Copy link
Author

erights commented Jun 10, 2013

But the reasoning I'm replying to here does establish why the difference is hard to observe, and thus why we might still be able to change to AP2 without breaking hardly any code.

@briancavalier
Copy link
Member

@erights I haven't had time to think about it deeply yet, but this sounds really interesting. My initial reaction was similar to @domenic's in that I couldn't seen any operational difference, but your notYetThenable example has my wheels turning.

This issue is also timely for another reason. I've been thinking about the issue of promises and algebraic data types from another angle. I mentioned it briefly to @domenic at JSConf, but didn't have anything to show at the time. I don't want to hijack the thread, so I'll keep it short. I created an experiment that shows a monad transformer that can lift an algebraic data type and make it promise aware, while still obeying their algebraic laws. Give it a functor/applicative/foldable/monad/etc. and it will give you a new version that operates on promises. It assumes the fantasy-land interfaces, but could easily work with any interface names.

I don't know if that approach has been discussed by TC39, but I think it could be an interesting approach to promises and monads/etc. working together.

Ok, hijack over.

@novemberborn
Copy link
Contributor

If I'm understanding all this correctly, it sounds like what Legendary already does. Promises are assimilated using state inspection in the resolve method and when returned from callbacks, but with promise-for-thenable, state is only adopted in the first then() call. See https://github.com/novemberborn/legendary/blob/master/lib/ResolutionPropagator.js#L201.

@domenic
Copy link
Member

domenic commented Jun 10, 2013

@novemberborn very interesting!

I agree with @briancavalier's thoughts that the notYetThenable example is compelling enough to probably make this worthwhile.

It sounds like the next step is to do a major rewrite of the spec to reflect these and see what it ends up looking like. We were so close to getting a 1.1... sigh.

@juandopazo
Copy link
Contributor

I think the next step is probably to prototype it first. Maybe in @briancavalier's avow?

@erights
Copy link
Author

erights commented Jun 10, 2013

I suggest making this change no earlier than 1.2. It is still possible that TC39 will go with AP3 rather than AP2, in which case we would regret pushing this change too early. But by all means, let's continue discussing it.

@briancavalier
Copy link
Member

Ok, I made a quick attempt at it on line 134 here, which is now in avow's assimilate-jit branch. I simplified a few other things in the process (sorry for the noise), but basically the coerce() function is now only ever called just-in-time. It also effectively memoizes so that assimilation only happens once per promise resolution.

@bergus
Copy link

bergus commented Aug 6, 2014

@erights:

In AP2, it would not be sampled on .resolve or on onFulfilled's or onReject's return. It would be sampled at the very last moment it could possibly be -- prior to calling onFulfilled.

Would it? Since thenables are expected to be asynchronous in general, this sounds like it would impose an extra artificial delay. I had expected that the assimilation of thenables might occur as soon as the .then() method is called on the promise-for-maybe-thenable - i.e. breaking your example

var notYetThenable = {};
Q(notYetThenable).then(willBeThenable => ....);
notYetThenable.then = function....;

but working for

var notYetThenable = {},
    promise = Q(notYetThenable);
notYetThenable.then = function …;
promise.then(wontBeThenable => …);

I think implemenations should be free to do the steps of the [[resolve]] procedure whenever they want - even never in a lazy implementation when no further .then() calls on the result promise force the assimilation. For example my implementation currently does the is-thenable test synchronously, and also calls the then method right away, but defers the recursive [[resolve]] calls until other handlers on the result promise signal interest in the resolution value. Deferring the then call could be done easily - and every of these versions passes the A+ test suite.


I don't think we even need to edit the spec. As it stands, the spec already allows this, stating only that and not when [[resolve]] should be called. Also, none of the tests checks only for .then on a thenable being called, it always does promise.then(() => thenable).then(hasCorrectResult).

We might however add a note to "run the Promise Resolution Procedure", something along the lines of

Implementations do not need to meet the expectation that the steps of [[resolve]](promise, x) are immediately run. This allows for lazy evaluation, discarding the x value if promise is no more needed.

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

No branches or pull requests

6 participants