Some imprecise points in this spec

[dmitriz]

Hopefully these can be made more precise (I'll be happy to attempt a PR if desired):

---

but does not imply deep immutability.

Probably clear enough to most, but a quick example would make it even more clear to people with less experience.

---

2. If `onFulfilled` is a function:
    i. it must be called after promise is fulfilled, with promise's value as its first argument.

Does it actually mean "as its only argument"? The same for 3.i.

---

5. `onFulfilled` and `onRejected` must be called as functions (i.e. with no `this` value). [3.2]

Would saying "with this value being undefined" make it more precise?

---

6. `then` may be called multiple times on the same promise.

Very confusing. Any example of how this can happen?

---

6.1. If/when `promise` is fulfilled, all respective `onFulfilled` callbacks must execute in the order of their originating calls to `then`

Isn't there always a single onFulfilled callback inside then? What is the meaning of the "all respective callbacks"?

What is the precise meaning of "their originating call to then? Any example?

---

7.i. If either `onFulfilled` or `onRejected` returns a value `x`, run the Promise Resolution Procedure [[Resolve]](promise2, x).

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.

---

7.iii. If `onFulfilled` is not a function...

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?

---

... assumption that x behaves at least somewhat like a promise

Obviously "somewhat like promise" is imprecise. Any way to make it more clear?

---

1. If `promise` and `x` refer to the same object, reject `promise` with a `TypeError` as the reason.

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?

---

2. If x is a promise, adopt its state [3.4]:

This feels like a circular logic, since it is inside the spec defining what promise is. Can this definition made avoid referring to promise?

[domenic]

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:

Does it actually mean "as its only argument"? The same for 3.i.

No

Would saying "with this value being undefined" make it more precise?

I don't see a difference in precision here.

Isn't there always a single onFulfilled callback inside then? What is the meaning of the "all respective callbacks"?

No, you can call then multiple times, as stated by 2.2.6, so there can be multiple callbacks.

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.

I don't think this would be more clear. As you point out, they are equivalent.

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?

I don't think that's necessary. Only functions can return values.

Obviously "somewhat like promise" is imprecise. Any way to make it more clear?

This is in a noteexplanation of the purpose of an algorithm intended to give you an intuition for it, so there's no need to be precise. The precision is accomplished via the actual algorithm steps.

This feels like a circular logic, since it is inside the spec defining what promise is. Can this definition made avoid referring to promise?

There's a note, 3.4, explaining what this means.

[dmitriz]

Many thanks @domenic for your responses, greatly appreciated!

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 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.

---

Does it actually mean "as its only argument"? The same for 3.i.

No

Hm... so it is not fully guaranteed how onFulfilled is called?
For instance, if I pass the function

onFulfilled = (one, two) => {console.log("I only care about:", two)}

what will be the resolved value?
I have never seen it being called with more arguments, is there any use of that?

---

I agree with your other answers, thank you.
But these are still causing some confusion:

Obviously "somewhat like promise" is imprecise. Any way to make it more clear?

This is in a explanation of the purpose of an algorithm intended to give you an intuition for it, so there's no need to be precise. The precision is accomplished via the actual algorithm steps.

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?

This feels like a circular logic, since it is inside the spec defining what promise is. Can this definition made avoid referring to promise?

There's a note, 3.4, explaining what this means.

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
The result is somewhat surprising:

"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 .then method as function, and if yes, calling it to get the resolved value. Which, however, would have made the second example resolved with undefined, that does seem to happen as the console.log function is never called!

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?

[donhatch]

This feels like a circular logic, since it is inside the spec defining what promise is. Can this definition > made avoid referring to promise?

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:

There's a note, 3.4, explaining what this means.

It tries and fails utterly.

[dmitriz]

Thank you @donhatch

If I understand correctly, you are proposing to change the spec to saying

(Recommended) If x is known to be a promise

which does not seem to solve the circularity problem?
Because (1) it still refers to "being a promise", that is what we are defining,
and (2) adding "recommended" to the spec creates the confusion
about what should be called "conformant".

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.
If it is so, the derived question is whether the spec aims to put any precise restriction on that implementer's decision.

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.

[ForbesLindesay]

Is it left up to the implementers to decide how to detect whether a value is a promise?

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 .then on it.

If you do not detect it as a promise, you must check for a .then method and follow the remaining steps in the algorithm. This means that some promises could take an implementation specific approach to adopting the native Promise's state, but they cannot fail to recognise it as a Promise, because if they don't take an implementation specific approach, they will be required to follow the algorithm, detect the then method, and call it appropriately.

Does it actually mean "as its only argument"? The same for 3.i.

No

Hm... so it is not fully guaranteed how onFulfilled is called?

It does not, but it guarantees enough for two key things:

1. all conformant promise implementations will be interoperable
2. no conformant promise implementation will miss out on the most important learnings of the Promise community (e.g. the callback is always invoked asynchronously)

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 onFulfilled if you thought that doing so represented a good API decision. Your implementation would remain Promises/A+ compliant.

[donhatch]

@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.
The proposed wording of my pull request is an attempt to fix the real problem I see, pragmatically, with a minimal clean footprint and requiring minimal attention from committee members who really aren't that interested.

| and (2) adding "recommended" to the spec creates the confusion
| about what should be called "conformant".

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?

[dmitriz]

@ForbesLindesay
Thank you for an excellent answer!
It does contain a lot of useful information that the official spec sadly does not say,
that perhaps should be there mentioned more explicitly?

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 .then on it.

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 then by assuming the value is still stored at the old key. Isn't this a potential for future bugs and some general lack of safety, that wouldn't be there if the library X were required to call then.

If you do not detect it as a promise, you must check for a .then method and follow the remaining steps in the algorithm. This means that some promises could take an implementation specific approach to adopting the native Promise's state, but they cannot fail to recognise it as a Promise, because if they don't take an implementation specific approach, they will be required to follow the algorithm, detect the then method, and call it appropriately.

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 x is a promise ...", which means it must be known at that stage, whether x is a promise or not. While I still have my safety concerns using such library, at the spec level, that would have been more precise, clear and explicit. Also it would avoid the currently present circular dependency, if the promise detection becomes at the implementers' discretion.

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 onFulfilled if you thought that doing so represented a good API decision. Your implementation would remain Promises/A+ compliant.

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 onFulfilled is called, its first argument must be assumed as the fulfilled value. Whereas you would want to cancel or receive progress before the function is called.

That could have been made possible, if the onFulfilled callback was allowed to be called earlier with e.g. some truthy second argument, without change the promise state. Which is, however, not allowed as far as I understand.

[dmitriz]

@donhatch

Yes. The crucial additions are the words "(Recommended)" and "known to be".

That still feels vague. Known to whom?

According to @ForbesLindesay answer, it is up to the implementer's to decide,
which values they want to treat as promise and which as thenable.
So it could be "known" to them that x is a promise but they could still decide to treat it uniformly as any other thenable.

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.

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 :-) )

Things work fine until they don't ;)
If I run into bug with my users when using library X,
due to any kind of existing or introduced incompatibility,
that will become my problem or the company running that business.
What if the customers stop getting paid because that promise library
changed their internal details that other library was using?
That minor performance optimisation,
to save the customer 3 sec to wait for payment,
could easily become costly.

All problems here are actually easy to solve by removing that section completely
and leaving only the same uniform treatment of promises as thenables.

[donhatch]

Yes. The crucial additions are the words "(Recommended)" and "known to be".

That still feels vague. Known to whom?

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
and leaving only the same uniform treatment of promises as thenables.

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.

[dmitriz]

@donhatch

That's true, but then the spec would effectively forbid the optimization.

Not at all.
The spec should always describe the intended functionality,
not the implementation (why should it?).

The spec is what you have in your tests, not how to write code to make these tests pass.
Otherwise it becomes an implementation recommendation ;)

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.
But this should be separate from spec, rather than buried deep inside the spec,
where it is encouraging bugs and unsafe coding practices, I think

[ForbesLindesay]

The spec should always describe the intended functionality, not the implementation (why should it?).

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

That sounds like library X can rely on the unspecified implementation details of library Y.

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 .getState() API, that can be used, since it's part of the public API, but is not specified by Promises/A+.

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 .then in order to do so.

[dmitriz]

@ForbesLindesay

Generally specs actually specify algorithm steps, at a fairly high level.

That is still what I would call the functionality, which is described via the algorithm.
For instance, the absolute value function

const abs = number =>
     (number > 0) ? number : -number

is prescribed through an algorithm, but I can obviously use another implementation
for the same functionality, such as

const equivAbs = number =>
    (number < 0) ? -number : number

Now I can use the spec defined by the abs to test the equivAbs function.

I think a clear separation between the spec and implementation
will benefit the clarity for both users and implementers.

It's only ever ok to ignore these steps if the different approach is undetectable to users.

That is exactly what I mean by different implementation of the same functionality.

This is generally extremely difficult to prove,

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.

so the spec explicitly says that you can take a different approach if you know what implementation of Promise you are dealing with.

Does it really say that explicitly?

For example, the following code triggers a memory leak if you follow the algorithm steps outlined in the spec:

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.

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.

Why would you not be allowed it?
Maybe this is again the confusion caused by mixing the spec and implementation.
Memory leak is a common exception in any language that should be dealt with according to the standards of the language. It is an implementation concern, not related to the specific functionality provided by the spec. The role of the spec is only to alert to the danger of such leaks happening, due to the spec's recursive nature, but not to mix in any prescription how to deal with it.

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#L151

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 Promise without even explicitly calling any function recursively. Your trick here only guards for specific ways of getting to that but there may be others that you can't predict. Which is ok as it is expected, given the spec's recursive nature.

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 .getState() API, that can be used, since it's part of the public API, but is not specified by Promises/A+.

That could also be useful to mention under "implementation recommendations" ;)
Or maybe not, to prevent encouraging something potentially unsafe.
Commitments like that can easily become forgotten, maintainers change, etc.
The actual tangible optimization benefits are always questionable, whereas bugs are real.
You clearly can't prevent libraries from doing it, but encouraging it will not make the software any safer, even if the implementers think they are aware of the risks.

[donhatch]

All problems here are actually easy to solve by removing that section completely
and leaving only the same uniform treatment of promises as thenables.

That's true, but then the spec would effectively forbid the optimization.

Not at all.

@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 then property), as I said. That is, unless you're proposing removing all of 2.3.3 too?? If you are, then you're proposing a major overhaul of the spec, in which case I'll exit the discussion here since this seems very far afield of your original issue "some imprecise points in this spec".

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.

[donhatch]

@ForbesLindesay wrote:

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 .then in order to do so.

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).

[dmitriz]

@donhatch

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, ..."

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.

I.e. the spec would forbid the optimization (of skipping all the explicit testing and evaluating of the then property), as I said. That is, unless you're proposing removing all of 2.3.3 too?? If you are, then you're proposing a major overhaul of the spec, in which case I'll exit the discussion here since this seems very far afield of your original issue "some imprecise points in this spec".

Optimization has nothing to do with the spec, see my explanation in other comments.
It is in fact, nearly identical with your proposition, sans confusion.
If that step is only "recommended", and misregarding that recommendation does not change anything, then it is redundant and needless.

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.

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.