Promise type should separate failure and success types separately

[Gozala]

Current type definition of Promise takes single polymorphic parameter R that seems to represent type of the value it will be successfully fulfilled with:

https://github.com/facebook/flow/blob/master/lib/core.js#L422

Unfortunately this mean error case is untyped:
https://github.com/facebook/flow/blob/master/lib/core.js#L423

Furthermore onFulfill handler can return rejected promise and there for make result of .then rejected promise.

I run into this issue with a code like:

class Future <error, value> {
  type: "Async";
  promise: () => Promise<value>;
  constructor(promise:() => Promise<value>) {
    super()
    this.type = "Async"
    this.promise = promise
  }
  await(resume:(task:Task<error, value>) => void):void {
    this
      .promise()
      .then(Task.succeed, fail)
      .then(Task.resume)
  }
}

Problem is I can no longer restrict errors that task can fail with. Which is why I'd like to propose altering a type definition for promises as follows:

// Bottomless type that can't be instantiated
type Never = Never

declare class Promise<R, X> {
    constructor(callback: (
      resolve: (result: R | Promise<R, X>) => void,
      reject:  (error: X) => void
    ) => mixed): void;

    then<U, Y>(
      onFulfill?: (value: R) => U | Promise<U, Y>,
      onReject?: (error: X) => U | Promise<U, Y>
    ): Promise<U, Y>;

    catch<U, Y>(
      onReject?: (error: X) => U | Promise<U, Y>
    ): Promise<U, Y>;

    static resolve<T>(object?: Promise<T> | T): Promise<T, Never>;
    static reject<Y>(error?: X): Promise<Never, Y>;
    static all<T, Y, Elem: Promise<T, Y> | T>(promises: Array<Elem>): Promise<Array<T>, Y>;
    static race<T, Y, Elem: Promise<T, Y> | T>(promises: Array<Elem>): Promise<T, Y>;

    // Non-standard APIs common in some libraries

    done(
      onFulfill?: (value: R) => mixed,
      onReject?: (error: X) => mixed
    ): void;

    static cast<T>(object?: T): Promise<T, Never>;
}

[Gozala]

I will submit a patch if such modifications seem acceptable.

[samwgoldman]

Promises can also fail if any exception is thrown from inside the promise. This is the reason why the error type is any.

[samwgoldman]

I suppose you could restrict the reject callback argument type, but not the onReject parameter type...

[Gozala]

Promises can also fail if any exception is thrown from inside the promise. This is the reason why the error type is any.

Sure but I implied that X type should also cover Errors that maybe thrown. Dos that makes sense ?

[Gozala]

I suppose you could restrict the reject callback argument type, but not the onReject parameter type...

Why so I'm not following I'm afraid.

[Gozala]

For example one could write code like:

var result:Promise<Buffer, String|TypeError > = new Promise((resolve, reject) => {
  fs.readFle(path, (error, data) => error == null ? resolve(data) : reject(error.message))
});

Note that above will produce rejected promise with a TypeError error because there was a typo in fs.readFile but that was already encoded in the Promise type signature.

Further more I think this kind of errors are handled by flow already. If you do intend on throwing exception manually from the Promise I don't see why you would not encode those possibilities in the type signature. And well if you don't want to you could always say Promise<T, any>.

[samwgoldman]

I suppose the best way to show this is to give examples. Here are 3 ways that Promises can be "rejected" which would need to be constrained to E under a Promise<T,E> scheme.

Explicit reject

This is simple to restrict and your typedefs are sufficient.

var p: Promise<*,number> = new Promise((resolve, reject) => {
  reject(""); // expect error: string ~> number
});

Throw

If a value is thrown from inside the promise, that value is passed to the onReject callback to then.

var p: Promise<*,number> = new Promise((resolve, reject) => {
  throw ""; // expect error: string ~> number
}).then(
  x => { console.log("onResolve:", typeof x) },
  e => { console.error("onReject:", typeof e) }
);
// Prints: onReject: string

Even if the value is thrown from a function call.

function f() {
  throw ""; // Is this the type error?
}

var p: Promise<*,number> = new Promise((resolve, reject) => {
  f(); // Or is this the type error?
}).then(
  x => { console.log("onResolve:", typeof x) },
  e => { console.error("onReject:", typeof e) }
);
// Prints: onReject: string

Note that we have no way (currently) to annotate whether a declared function throws, so using declared functions in a promise would be unsafe.

Runtime errors

Any runtime error also causes the onReject callback to fire. This behavior necessitates that the type parameter at least be bound by Error, which limits its utility.

var p: Promise<*,number> = new Promise((resolve, reject) => {
  var x: string[] = [];
  x[0].length; // This throws `TypeError`, which is incompatible with `number`, but Flow can't catch this error (yet)
}).then(
  x => { console.log("onResolve:", typeof x) },
  e => { console.error("onReject:", e.constructor.name) }
);
// Prints: onReject: TypeError

And here, again, the error can be thrown from a dependent function call.

[samwgoldman]

That said, while I don't personally believe there is a solution (edit: at least, a solution that doesn't require some significant preparatory work) that affords type safety given the above, I welcome you to attempt it. Happy to review a PR. My overall advice is this: any is a signal that type safety is in the hands of the programmer. That's what we have today. Any PR designed to deliver type safety to the Promise reject value should also be sound.

[samwgoldman]

Closing this pending a proposal that addresses the concerns I outlined above.

[unscriptable]

I agree that this would be a great feature. We've been longing to use a second type parameter in our promises. It's clear, though, from Sam's examples, that flow would have to handle exceptions at the function level before tackling Promises (if that happens at all).

In your last Runtime error example, @samwgoldman, you say, " This behavior necessitates that the type parameter at least be bound by Error, which limits its utility." Great observation, but I disagree that we've limited the utility. Rather, I feel you've uncovered a bit of clumsiness with ES6 Promises that flow would have to deal with.

I'm wondering if perhaps flow could enforce that developers specify at least a union with Error, i.e. var p: Promise<*,number|Error> = .... Does that make sense? If so, that's not a bad compromise when developers want to transform rejection values.

[samwgoldman]

@unscriptable Fair points, and thanks for the thoughtful discussion.

Say we could enforce that Error must be part of that type param. There's still an issue with explicit throw (non-Error type) from a function call inside the promise. For this we'd need call graph analysis to see if any of those calls might throw. Without that, implementing this soundly is intractable.

(Edit: I'm pretty curious to see the use case for transforming rejection values. Considering this design wart for Promises, I wonder if the better alternative is to roll an Either type and use that as the T in Promise<T>—leave the rejection stuff for propagating/handling fatal errors.)

[jasonkuhrt]

Just came here looking to see if there was a better story than in TypeScript microsoft/TypeScript#7588.

[Gozala]

What if we allowed Promises to take optional second error type defaulting to any ? That would at least allow type refinements when .catch or .then is used.

[callumlocke]

Why was this closed?

[Gozala]

Why was this closed?

@samwgoldman outlined in comment above that run-time exceptions could poison soundness. Like right now type guarantees by flow are sound in that even if you throw or run-time exceptions happens types will still match declarations. Unfortunately promises are different in a sense that exceptions will sneak back into run-time or in other words then handler could type check as expecting say type number but at runtime get Error instead (or whatever you happen to throw really).

I still think think it would still be improvement over what we have today, but I also understand provided counter arguments as that would alter overall soundness of flow.