-
Notifications
You must be signed in to change notification settings - Fork 164
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
Some imprecise points in this spec #263
Comments
Hi @dmitriz, I think in general a lot of these are not something we want to address. For example, you are the first person to bring up that various things are unclear or ask for examples; most people have been able to figure them out from the spec and the tests. I'll address the other questions:
No
I don't see a difference in precision here.
No, you can call then multiple times, as stated by 2.2.6, so there can be multiple callbacks.
I don't think this would be more clear. As you point out, they are equivalent.
I don't think that's necessary. Only functions can return values.
This is in a
There's a note, 3.4, explaining what this means. |
Many thanks @domenic for your responses, greatly appreciated!
I am sorry for being the first one :) and imagine many people would have some guesses. My suggestion was to help people especially with less experience, with quick inline examples. However, in this case, people can actually misunderstand what is said, such as assuming that the value inside the promise must remain immutable as per spec. Which is not entirely correct: var obj = {aa: 12};
var promise = Promise.resolve(obj);
// testing
var obj = {};
var promise = Promise.resolve(obj);
// testing
promise
.then(res => {
console.log("Before:", res);
// mutating original object
obj.answer = 42;
// use the same promise
promise.then(res1 => {
console.log("After: ", res1);
})
}); produces "Before:" Object {}
"After: " Object {
answer: 42
} https://codepen.io/dmitriz/pen/wjvWwg?editors=0011 Here one might be misled that a promise is in charge of freezing the object at the top level, but not at any deeper level. Which, however, is not the case. That is where I see simple example being very helpful to clarify things.
Hm... so it is not fully guaranteed how onFulfilled = (one, two) => {console.log("I only care about:", two)} what will be the resolved value? I agree with your other answers, thank you.
Is there any algorithmic step in the current spec describing how promises are detected and their values extracted? Or is the spec intentionally vague leaving it to the implementers?
Here is a confusing example: let val = {
then: x => x
}
let val1 = {
then: x => x(1)
}
let val2 = {
then: {x:1}
}
Promise.resolve(val).then(result => {
console.log("Result is:", result);
})
Promise.resolve(val1).then(result => {
console.log("Result is:", result);
})
Promise.resolve(val2).then(result => {
console.log("Result2 is:", result);
}) https://codepen.io/dmitriz/pen/rvNWxw?editors=0012 "Result2 is:" Object {
then: Object {
x: 1
}
}
"Result is:" 1 Here it looks like the object passed is really checked for whether it has a Is that the intended behavior? Is it meant to be prescribed by the current spec? Or is the spec deliberately vague about it? In the latter case, would it make the possible "complying" libraries unsafe to use? |
There is already an open bug on this circularity: #240 'change "if x is a promise" to something non-circular and clear'. We've got what I think is quite a good fix for it in pull request #241 , but @domenic is blocking it. @domenic replies:
It tries and fails utterly. |
Thank you @donhatch If I understand correctly, you are proposing to change the spec to saying
which does not seem to solve the circularity problem? A basic question, whose answer I can't tell from reading the spec in its present form is this: Is it left up to the implementers to decide how to detect whether a value is a promise? It seems, the answer is "yes", as the spec reads deliberately vague about it. Otherwise, nothing would in principle prevent the implementers to declare their implementation conformant without recognizing e.g. the JS native promises in their resolution procedure. |
Yes, what this is intended to mean is that if you know that the value is a promise, of a recognised implementation, you may choose to use a different approach to adopt its state, i.e. you do not have to call If you do not detect it as a promise, you must check for a
It does not, but it guarantees enough for two key things:
The spec for native promises should be more specific, but the Promises/A+ spec intentionally allows room for experimentation in areas that we weren't confident of the right answer for. It does not specify any method for cancellation or progress, but you could add some kind of progress reporting object or cancellation token as a second argument to |
@DimitriZ wrote: | If I understand correctly, you are proposing to change the spec to saying | (Recommended) If x is known to be a promise Yes. The crucial additions are the words "(Recommended)" and "known to be". | which does not seem to solve the circularity problem? | Because (1) it still refers to "being a promise", that is what we are defining, You are right that the new wording is still technically circular, but it has now become a self-confirming "this sentence is true" kind of circular rather than a self-contradicting "this sentence is false" kind of circular. The former doesn't really cause any problem in practice-- that is, things generally work out fine if you consider "this sentence is true and X" to be true as long as X is true. (Yes, they'd also work out fine if you consider it to be false, but don't do that :-) ) I personally wouldn't object to removing the circularity altogether for purity's sake; however I think there's really no chance of that happening since we can't even get the glaring defect fixed. | and (2) adding "recommended" to the spec creates the confusion Not at all-- in fact the opposite; it makes it clear that the behavior in question is not required and therefore failure to follow it (or even nail down precisely what it means) will not hurt conformance, regardless of whatever else "conformant" might mean. This is absolutely necessary, given that the meaning of "If x is known to be a promise" (worlds better than "is a promise" which was self-contradictory and flat-out impossible to conform to, by The Halting Problem) still has not been precisely nailed down and, realistically, never will be, given the personalities involved and the current level of disinterest. As long as "recommended" is left out, you (the implementor) are required to implement the behavior in question, which means you really do have to know what it means-- but you don't know, and can't know, which puts you in an impossible position. The "recommended" gives you an escape hatch-- that is, you can simply refrain from trying to implement the behavior in question when you're not sure, and it won't kill your conformance. Make sense? |
@ForbesLindesay
Hm... That sounds like library X can rely on the unspecified implementation details of library Y. But what if those details change? For instance, lib Y decides to store the promise value under another key, which could break the code of X bypassing calling
I find your explanation here much more clear and precise than what is in the spec. The big difference is that your description allows a library to fail to recognize a Promise, whereas the spec does not seem to allow it by saying "If
That is a very interesting point. However, I can't see how that idea could be used along with the current spec, because the moment That could have been made possible, if the |
That still feels vague. Known to whom? According to @ForbesLindesay answer, it is up to the implementer's to decide, Also I mentioned in the above comment concerns about that recommendation making those libraries unsafe to use. I would actually not recommend it for that reason.
Things work fine until they don't ;) All problems here are actually easy to solve by removing that section completely |
Correct, "known to be" is vague, and I believe its intent is clarified reasonably in the added part of the footnote (which says the same thing @ForbesLindesay has said here, which you said you like-- the agreement is not surprising since @ForbesLindesay is in fact the author of that part of the pull request :-) ) This is a giant leap forward from the previous wording "is a promise", which is self-contradictory, and was contradicted in the footnote. If we could get this improvement in, I would declare victory in this area. | All problems here are actually easy to solve by removing that section completely That's true, but then the spec would effectively forbid the optimization. That's not an option, since the optimization is considered to be important and it simply isn't going to be removed or omitted from various existing libraries that Promises/A+ ties together. Promises/A+ certainly allows you to omit the optimization from a particular implementation (since the optimization is only recommended, not required, regardless of the currently sloppy wording), and such an implementation can be documented as such; i.e. its spec would forbid the optimization, making it strictly stronger than the Promises/A+ spec. Does this meet your needs at all? In any case, I'm afraid that's the most you're going to get from Promises/A+ in this regard. |
Not at all. The spec is what you have in your tests, not how to write code to make these tests pass. The way the spec is written now, you can't even test it. In case of any doubts, a note can be added that other optimized implementations are acceptable, as long as the same functionality is guaranteed. |
Generally specs actually specify algorithm steps, at a fairly high level. It's only ever ok to ignore these steps if the different approach is undetectable to users. This is generally extremely difficult to prove, so the spec explicitly says that you can take a different approach if you know what implementation of Promise you are dealing with. For example, the following code triggers a memory leak if you follow the algorithm steps outlined in the spec: function recurse() {
return Promise.resolve(null).then(() => recurse());
}
recurse(); This memory leak is user detectable, so you would not be allowed to fix it if you weren't allowed to user alternative methods to "adopt" the state of a promise. In then/promise, we use some clever trickery to prevent this leak, providing we detect that both promises being combined are then/promise promises: https://github.com/then/promise/blob/cf30e9f39476bbfa3dbaacb93127c1c958d5f255/src/core.js#L147-L154
The normal rules about not using another libraries private fields and methods still apply. However, If the maintainer of library X asks the maintainer of library Y, they might get a commitment that certain internal fields won't change. Alternatively, library Y might expose a public I take issue with the "Recommended" part you are attempting to add, since you are always required to adopt the state of any Promise, it's just that if you like, you can follow the default steps of simply calling |
That is still what I would call the functionality, which is described via the algorithm. const abs = number =>
(number > 0) ? number : -number is prescribed through an algorithm, but I can obviously use another implementation const equivAbs = number =>
(number < 0) ? -number : number Now I can use the spec defined by the I think a clear separation between the spec and implementation
That is exactly what I mean by different implementation of the same functionality.
It is actually sad that formal equivalence of sequences of expressions is still considered difficult to prove in 2018, despite of centuries of research in logic and other subjects.
Does it really say that explicitly?
Indeed, and memory leaks are common when recursion is present, they may or may not be dealt with, depending on implementations, but that would be a different concern, not related to the promise spec. For instance, you define the spec for the factorial function as const factorial = n =>
(n = 0) ? 1 : n * factorial(n-1) Whereas in real world implementation, you can run out of the integer range allowed by the engine. The latter is, however, a different concern, related to the range limits, that is not specific to the factorial function and should not be mixed into its spec. Of course, there would be nothing against discussing these issues in separate "implementation notes", which could be helpful. It is the mixing concerns part that makes things confusing and potentially unsafe.
Why would you not be allowed it?
This is nice but you agree that this way is not prescribed by the spec and should not be? On related note, I have actually managed to crash my browser by testing the native
That could also be useful to mention under "implementation recommendations" ;) |
@dmitriz How can you say that? You're suggesting removing "that section" which I understand to be step 2.3.2 (the poorly worded "if x is a promise, adopt its state", and subsections detailing what "adopt its state" means). I'm pointing out that that if 2.3.2 were omitted as you propose, then the implementation would be required to go on to step 2.3.3.2 "if retrieving x.then results in a thrown exception, ..." I.e. the spec would forbid the optimization (of skipping all the explicit testing and evaluating of the Keep in mind that this ship sailed a while ago. I think requests for clarification of the spec's wording are reasonable and should always be considered, but proposals to substantially change the spec are not-- you'd be proposing a new spec. |
@ForbesLindesay wrote:
You lost me there. Are you saying you take issue with pull request #241 which you helped write? The word "Recommended" in that context clearly refers to the optimization 2.3.2; it does not refer to the entire endeavor of adopting the promise's state one way or another (sections 2.3.2 and 2.3.3 combined), which is of course still required. The proposed new wording agrees with what you just said (whereas the existing spec does not, which is the whole reason for the pull request). |
If my understanding is correct, then the outcome functionality will not change when removing
Optimization has nothing to do with the spec, see my explanation in other comments.
There is no change to functionality, which I regard as the main point of the spec. I personally find that "recommendation" making this more dangerous for the end user, as I mentioned. |
Your understanding is not correct: the outcome functionality will change by removing 2.3.2. That's because the steps beginning with 2.3.3.2 (that is, fetching and calling the |
the outcome functionality *will* change by removing 2.3.2. That's because
the steps beginning with 2.3.3.2 (that is, fetching and calling the then
property) are detectable to users: users can tell whether those steps are
omitted, and omitting those steps would violate the spec (if 2.3.2 were
removed).
And how are they going to detect it without inspecting the code?
…On Mon, May 7, 2018 at 11:40 PM, donhatch ***@***.***> wrote:
If my understanding is correct, then the outcome functionality will not
change when removing 2.3.2, then it is indeed unnecessary and only creates
confusion.
Your understanding is not correct: the outcome functionality *will*
change by removing 2.3.2. That's because the steps beginning with 2.3.3.2
(that is, fetching and calling the then property) are detectable to
users: users can tell whether those steps are omitted, and omitting those
steps would violate the spec (if 2.3.2 were removed).
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#263 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ACZZKRJGruxNlGxvHyitN93BhRVhKbsKks5twHj0gaJpZM4TS7gC>
.
--
Dmitri Zaitsev
School of Mathematics
Trinity College Dublin
WWW: http://www.maths.tcd.ie/~zaitsev/
|
Hmm. My thinking was that, in general, retrieving x.then and calling it are code paths that the user has control over and can trace; therefore the user can tell when the implementation skips either or both of those steps. However, the case in question here is when x is a Promise of the current implementation (or of some other specific implementation known to be compliant)... in which case it seems the user does not have control over the code paths of retrieving and/or executing the Suddenly I am less sure of myself! So I will retreat to the more hedged position that @ForbesLindesay stated previously:
That is: the spec doesn't want to give the implementation the burden of proving its optimization 2.3.2 is undetectable to users, and the spec definitely does want to allow the optimization, so it takes the safe approach and explicitly states (clumsily worded) that the optimization is allowed (and in fact encouraged -- but "optional", "allowed", "recommended", "encouraged" are all logically the same when it comes to the question of compliance). Do you think you can prove that 2.3.2 is logically unnecessary in all cases? (Conversely, a question for the experts lurking: can you prove that 2.3.2 is logically necessary? @ForbesLindesay gave an example of a memory leak, but I'm not entirely convinced that counts as "detectable by users".) |
That's correct. This can be shown in function resolve(x) {
return new Promise(resolve => resolve(x));
// alternatively, using `then` specifically:
return Promise.resolve().then(() => x);
}
var count = 0;
var x = {
get then() { count++; return undefined }
};
var promise = resolve(x);
resolve(resolve(promise)).then(() => console.log(count)); The recursive semantics of the naive |
Yes, this is also what @ForbesLindesay had explained. |
@donhatch |
Hopefully these can be made more precise (I'll be happy to attempt a PR if desired):
Probably clear enough to most, but a quick example would make it even more clear to people with less experience.
Does it actually mean "as its only argument"? The same for 3.i.
Would saying "with
this
value beingundefined
" make it more precise?Very confusing. Any example of how this can happen?
Isn't there always a single
onFulfilled
callback insidethen
? What is the meaning of the "all respective callbacks"?What is the precise meaning of "their originating call to
then
? Any example?Maybe it would be more clear to say "... don't throw an exception", as
undefined
is also a value and both are functions, so they always return a value unless throw.However, it looks like in 7.i and 7.ii it was assumed this was a function. Maybe make it more precise by adding "If ... is a function" to those?
Obviously "somewhat like promise" is imprecise. Any way to make it more clear?
Any example how that is possible in this situation?
And why is that a
TypeError
if the reason is the objects identity, not the wrong type?This feels like a circular logic, since it is inside the spec defining what promise is. Can this definition made avoid referring to promise?
The text was updated successfully, but these errors were encountered: