Some things do not seem sound in “react to a Promise<T>”

[bathos]

In react to a Promise<T> promise, which is the basis of the shorthand variants upon fulfillment and upon rejection, the onFulfilledSteps begin:

1. Let value be the result of converting V to an IDL value of type T.
2. If there is a set of steps to be run if the promise was fulfilled, then let result be the result of performing them, given value if T is not undefined. Otherwise, let result be value.

Note that steps for handling fulfillment and rejection are both created regardless of whether there are “inner” steps that were supplied. The operation ends with PerformPromiseThen(promise.[[Promise]], onFulfilled, onRejected, newCapability) where these second and third args are the functions created from onFulfilledSteps and onRejectedSteps.

Two things surprised me here. The first is that onRejectedSteps won’t be invoked if conversion itself fails. Instead, the resulting Promise<T> will reject. In theory, a specification can still handle this scenario by subsequently reacting to that result promise, analogous to promise.then(onF, onR).catch(onR). Almost all current usage of Promise as an input-from-ES type is Promise<any> or Promise<undefined>, where the distinction doesn’t matter because conversion never fails. But there is at least one spec where there’s an input Promise whose inner type’s conversion can fail — and that one spec (Service Worker, fetchEvent.respondWith — Promise<Response>) doesn’t appear to account for this. In fact, the first step in its “upon fulfillment” steps is this:

If response is not a Response object, then set the respond-with error flag.

It seems like it would be impossible for response to ever not be a Response object here, since conversion would have thrown in step 1 of the onFulfilledSteps wrapper, and in Web IDL, even if ? or ! is not used, I’m pretty sure abrupt completion propagation is implicit, right? So if this were really implemented as written, respondWith wouldn’t be able to handle this scenario. To handle it (as “react to” is written now), it would need to react to the return value of “upon fulfillment” — that’s the promise that rejects if conversion fails. Currently this return value is ignored.

The second thing which seems odd to me is that a new conversion attempt is specified to occur for every reaction. So, looking at the same spec, there’s both an “upon fulfillment” and an “upon rejection”, each of which use “react to”, and each “react to” has its own onFulfilled steps that begin with attempted conversion of the inner ES value to the inner Web IDL type. These conversions are observable to ES code and could produce different results at different times. I’m guessing this should really only happen (at most) once.

It seems like a new PromiseCapability should be created the first time “react to” is called, and that this PromiseCapability — we can call its promise the “conversion promise” — should be considered part of the state of the Promise<T> value. Its own fulfillment steps would just attempt conversion. Both the first “react to” and any subsequent “react to” would PerformPromiseThen passing the conversion promise instead of the promise that fulfills with the ES value. With a pattern something like this, it would still be possible for rejection steps to not be called if an error is thrown during the (supplied) fulfillment steps, but that’s probably correct.

[domenic]

Previously the first issue was discussed in #782

Some ideas:

• Remove the <T> from promise types entirely. It seems this better matches implementations.
• Make failed type conversions be treated as rejected promises.
• Add a third set of steps, something like "type conversion failed" steps.

For your second issue, I feel like the spec is pretty OK as-is? The spec is saying that every time you write "react to", you are doing something equivalent to

promise.then(v => {
  const converted = typeConvert(v);
  runActualSteps(converted);
});

(modulo the first problem about what happens with errors) which seems rather natural. Are there specific specs where you can see there being a problem with the current behavior?

[bathos]

Previously the first issue was discussed [...]

Apologies, didn’t realize I was duplicating. Thanks for linking it.

[Re: second issue] Are there specific specs where you can see there being a problem with the current behavior?

As far as I know, there are no specs where the current behavior is a problem. This is because every usage I found of a Promise<T> type where a conversion from ES would take place was one of:

• Promise<undefined>
• Promise<any>
• Promise<Response> (interface)

For all three of these cases, the inner type’s conversion algorithm is one which is guaranteed to never invoke arbitrary ECMAScript code and to return the same result value. If there’s a requirement that Promise<T> can only be used with an inner type that exhibits these properties, then the current “react to” steps seem fine (apart from the fact that failed conversions produce different ES values each time, but that’s kinda tied up with the earlier stuff).

I don’t think there’s a requirement like that, though. Should there be? The duplication would cease to be theoretical the first time someone specs an operation to consume e.g. Promise<DOMString> and writes both “upon rejection” and “upon fulfillment”, since as currently written, if the ES value were { i: 0, toString() { this.i++ } }, the ES value now looks like { i: 2, ... } (and different values/completion types could be produced each time). If “react to” were used alone, passing both sets of steps, the ES value would instead look like { i: 1, ... }. I would have assumed that if a spec uses the two “shorthands,” behavior shouldn’t be observably different from if it had used one “react to” and passed both sets of steps.

[domenic]

As far as I know, there are no specs where the current behavior is a problem.

This is what kind of encourages me toward the "remove <T> from promise types entirely" path...

I would have assumed that if a spec uses the two “shorthands,” behavior shouldn’t be observably different from if it had used one “react to” and passed both sets of steps.

IMO the issue is that upon rejection shouldn't be doing type conversion to T at all. So one fix would be to guard the conversion behind the existence of "set of steps to be run if the promise is fulfilled".

[mkruisselbrink]

I think the constructor of ClipboardItem, were it actually specified, would be such a case. It take as argument a ClipboardItemData, which is a Promise. Presumable whatever steps that constructor would be doing would run into this issue?

[bathos]

@mkruisselbrink Yes, it looks like both the constructor operation and createDelayed are potential examples. If they were specified using both upon fulfillment and upon rejection, there’d be two observable conversions with potentially different results and failed conversion (either time) wouldn’t be handled by either set of steps. I’m guessing it wouldn’t actually end up implemented that way regardless of what the spec says, though.

@domenic I agree, I don’t see any reason for “upon rejection” to include the conversion. So if “upon rejection” were redefined as steps to be run if either the Promise state is rejected or if conversion in a corresponding “upon fulfillment” fails, it seems like that would be sufficient to make the behavior rational. It would imply a small change to how respondWith is specified — but currently what’s there doesn’t align with Web IDL anyway.

[domenic]

So if “upon rejection” were redefined as steps to be run if either the Promise state is rejected or if conversion in a corresponding “upon fulfillment” fails, it seems like that would be sufficient to make the behavior rational.

I don't think we want to have any connection between "upon rejection" and "upon fulfillment" in this manner. They're not meant to be used as a pair; if you want that, use "react".

[bathos]

@domenic I think I understand the intention there, but if that’s the case, it’s easy to miss that “upon rejection” steps will never execute if conversion fails unless the subject promise is the one returned by “upon fulfillment” instead of the original promise. This is the case currently (in the spec, but not in implementations afaik) and would continue to be the case even if conversion is removed from the rejection path. Perhaps an advisory note would be sufficient though.

Another way to put this is that if they remain disconnected, I think the text should make it more clear that neither “react to” nor the combo of “upon fulfillment” + “upon rejection” with a single promise p is analogous to p.then(uponFulfillment).catch(uponRejection) — it’s always p.then(uponFulfillment, uponRejection) or p.then(uponFulfillment), p.catch(uponRejection).

[domenic]

Yeah, that makes sense to me.

Are you interested in working on a pull request to help fix these issues? :)

[bathos]

Yep — I should have some time within the next week.

[jan-ivar]

Remove the <T> from promise types entirely. It seems this better matches implementations.

Hi, sorry for jumping into a rather technical discussion about things over my head, but I'd wager promises are predominantly output-to-JS, not input-from-ES, so I'm not convinced the value of <T> should hang in the balance over problems in the latter category.

In my view Promise<T> has normative value in declaring API outputs that user agents MUST implement even if WebIDL tools can't enforce anything about them. I'm very thankful that it exists.🙏 ❤️ See #951 (comment).

[bathos]

(Unrelated but oops, I totally blanked on this, thanks for commenting since I’d forgotten.)

I agree. If the sole value of Web IDL type descriptors was specifying conversions from ES, then it would seem to follow that there’d be no reason for readonly attributes or operations to indicate their types or return types. That said, Promise does seem like kind of a weird case since it’s so tied up with ES and I can see why it might be deemed more trouble than its worth.

[annevk]

I'm pretty sure that service workers rely on the type being enforced when an arbitrary promise is passed in (e.g., for respondWith(). And once we make ByteString map to a byte sequence and vice versa I could see it being useful for return values as well. I don't see why promises would be different from synchronous callbacks/functions.