Flow: [0.79.0] type inference for exported promises

[alunny-stripe]

Hello,

We're upgrading to flow 0.79.0, and it appears the change to require annotations on exports means that all Promise exports need type annotations.

For example, this code now causes an error:
export const p = new Promise((resolve) => resolve(2));

1: export const p = new Promise((resolve) => resolve(2));
                    ^ Missing type annotation for `R`.

https://flow.org/try/#0KYDwDg9gTgLgBAYwgOwM7zHAvHZwDucAClBALYCWqwAFDVMKhADYBuwAlNgHxwNNtaAJg4cA3EA

It is a similar case for exporting eg. classes with methods that return promises, eg:
https://flow.org/try/#0KYDwDg9gTgLgBAE2AMwIYFcA28DGnUDOBcAYhBHAN4BQcdYAFAJRW13tTAzpQB2cvYAHc4ABSgQAtgEsCwBlDgBeAHxwoDAExMmAbjZwAvtUNA

It's not clear to me from the 0.79 release notes if this is a bug or intentional. Either way the error message is a bit cryptic.

In most cases it's feasible enough to add the annotations manually, although it's a bit of a chore.

[jbrown215]

Taking a look at the types for promises, it looks like we complain about R showing up in an input position because of how it is used in the resolve argument in the constructor (it is an argument of an argument of an argument of a function).

Something is wrong here, since our variance sigil on the class is +, which indicates that we intend to be using R in only output positions.

I will dig deeper this week. Thanks for reporting!

cc @samwgoldman

[jbrown215]

A minimal repro:

//@flow
declare class Promise<+R> {
    constructor(callback: (
      resolve: (result: R) => void,
    ) => mixed): void;
}
export const p = new Promise((resolve) => resolve(2));

(run flow check --no-flowlib)

[jbrown215]

Following up: since you cannot call the construct() method explicitly on an instance, we should not be checking the promise constructor and emitting this error. We won't be requiring annotations on promises that are exported after this is fixed.

Thank you for reporting, @alunny-stripe !

[alunny-stripe]

Awesome, thanks for looking into this so quickly!

[jbrown215]

This is actually super tricky. This code is valid in JS:

class A {}
const x = new A();
const y = new x.constructor();

That makes this probably pretty non-trivial. I'm not quite sure what the best way to proceed is yet, but ignoring the constructor when checking for type annotations is unsound.

[jbrown215]

After doing some thinking, reading, and discussing with @samwgoldman, I've decided that instead of skipping the constructor we should instead just not error if the missing type annotation error is caused by a missing type argument.

[kevinSuttle]

@jbrown215 has this landed in v.81.0 or earlier?

[jbrown215]

@kevinSuttle unfortunately not, but this is something we are actively working on. This bug revealed that we actually handle instance.constructor a bit sloppily. We're fixing that and then we will have a fix for this on top of it.

[FlorianBergmann]

Is there any progress on this?

[jbrown215]

We are still making progress on our revised model for classes. As soon as that is done we'll be able to handle this.

Thank you for your patience!
(cc @samwgoldman)