facebook / flow

Adds static typing to JavaScript to improve developer productivity and code quality.
https://flow.org/
MIT License
22.08k stars 1.85k forks source link

"Could not decide which case to select" error when using "async" #5132

Closed stouf closed 3 weeks ago

stouf commented 6 years ago

Some background: I mock many things when writing unit tests for my libraries. In order to make sure I mock things the right way, I assign the type of the original functions to my mocks.

Let's imagine the following:

A model.js file:

// @flow

/*::
type User = {
  id: number,
  email: string
};
*/

const db = require('./db.js');

async function getByIds(ids/*: number*/) {
  if (ids.length === 0) return [];

  const users/*: Array<User>*/ = await db.get({ model: 'user', id: ids });
  return users;
}

exports.getByIds = getByIds;

A service.js file:

// @flow

const model = require('./model.js');

async function getEmails(ids/*: number*/) {
  return (await model.getByIds(ids)).map(x => x.email);
}

exports.getEmails = getEmails;

And the test file test.js:

// @flow

/*::
import * as model from './model.js';
*/

const proxyquire = require('proxyquire');

function runWithAsync() {
  const getByIds/*: typeof model.getByIds*/ = async () => {
    return [];
  };

  const service = proxyquire('./service.js', {
    './model.js': {
      getByIds
    }
  });

  // Test stuff...
}

function runWithoutAsync() {
  const getByIds/*: typeof model.getByIds*/ = () => {
    return Promise.resolve([]);
  };

  const service = proxyquire('./service.js', {
    './model.js': {
      getByIds
    }
  });

  // Test stuff...
}

exports.runWithAsync = runWithAsync;
exports.runWithoutAsync = runWithoutAsync;

And here is the funky thing: even though runWithAsync() and runWithoutAsync() do the same thing, I have the following error for runWithAsync() but no errors for runWithoutAsync():

Error: test.js:11
 11:     return [];
         ^^^^^^^^^^ Promise. Could not decide which case to select
 14: async function getByIds(ids/*: number*/) {
                                             ^ return. See: model.js:14
  Case 1 may work:
   18:   return users;
         ^^^^^^^^^^^^^ type application of async return. See: model.js:18
  But if it doesn't, case 2 looks promising too:
   15:   if (ids.length === 0) return [];
                               ^^^^^^^^^^ type application of async return. See: model.js:15
  Please provide additional annotation(s) to determine whether case 1 works (or consider merging it with case 2):
   11:     return [];
           ^^^^^^^^^^ async return

Found 1 error

When I inspect the type of getByIds() in both runWithAsync() and runWithoutAsync(), they seem to be the same for Flow:

(ids: number) => Promise<[]> | Promise<Array<{email: string, id: number}>>

The type for the original getByIds() function (in model.js) is:

(ids: number) => Promise<[]> | Promise<Array<{email: string, id: number}>>

Another thing I noticed when trying random things: if I declare the mock in model.js using async, there is no problem:

'use strict';

// @flow

/*::
type User = {
  id: number,
  email: string
};
*/

const db = require('./db.js');

async function getByIds(ids/*: number*/) {
  if (ids.length === 0) return [];

  const users/*: Array<User>*/ = await db.get({ model: 'user', id: ids });
  return users;
}

const mockedGetByIds/*: typeof getByIds*/ = async () => {
  return [];
};

exports.getByIds = getByIds;
exports.mockedGetByIds = mockedGetByIds;

Flow is ok with the above. The type computed by Flow for mockedGetByIds is

(ids: number) => Promise<[]> | Promise<Array<{email: string, id: number}>>

Does anyone have an idea why Flow can't make a decision for runWithAsync() but has no problem with runWithoutAsync() and with mockedGetByIds()?

stouf commented 6 years ago

I just ran into another case where union types are not involved this time.

file1.js

// @flow

async function isGreaterThanTen(x/*: number*/) {
  if (x > 10) {
    return true;
  } else {
    return false;
  }
}

exports.isGreaterThanTen = isGreaterThanTen;

file2.js

// @flow

/*::
import * as file1 from './file1.js';
*/

const test/*: typeof file1.isGreaterThanTen*/ = async (x) => {
  return true;
};

Flow outputs:

Error: file2.js:8
  8:   return true;
       ^^^^^^^^^^^^ Promise. Could not decide which case to select
  3: async function isGreaterThanTen(x/*: number*/) {
                                                   ^ return. See: file1.js:3
  Case 1 may work:
    7:     return false;
           ^^^^^^^^^^^^^ type application of async return. See: file1.js:7
  But if it doesn't, case 2 looks promising too:
    5:     return true;
           ^^^^^^^^^^^^ type application of async return. See: file1.js:5
  Please provide additional annotation(s) to determine whether case 1 works (or consider merging it with case 2):
    8:   return true;
         ^^^^^^^^^^^^ async return

Found 1 error

If I rewrite isGreaterThanTen() as below, Flow is happy.

async function isGreaterThanTen(x/*: number*/) {
  return x > 10;
}

Same as in the description, if I keep the first version of isGreaterThanTen() and uses Promise.resolve() instead of async in test2.js, Flow is happy:

// @flow

/*::
import * as file1 from './file1.js';
*/

const test/*: typeof file1.isGreaterThanTen*/ = (x) => {
  return Promise.resolve(true);
};
EugeneZ commented 6 years ago

I can't reproduce your problem in flowtry, even though it shows the same types. Are you on the latest version of Flow?

That said, I've had similar problems. When flow says it can't choose between two different cases, you just need to make it explicit:

async function getByIds(ids/*: number*/)/*: Array<User>*/ { // <----
  if (ids.length === 0) return [];

  return await db.get({ model: 'user', id: ids });
}
stouf commented 6 years ago

I can't reproduce your problem in flowtry, even though it shows the same types. Are you on the latest version of Flow?

Yep, as written in the description, the problem doesn't occur when the type of the function is being reused within the same file for some reasons. I guess it's more a matter of context than being in the same file or not. It's probably what is happening when you give it a try in flowtry I think ?

That said, I've had similar problems. When flow says it can't choose between two different cases, you just need to make it explicit:

Thanks 👍 I've run into this issue many times before as well and got around it by explicitly typing the return of my function as you just advised. I also confirm the errors would disappear in this case with that approach. However, it seems that Flow doesn't know what decision to make when a function re-using another function type uses async, but know what to do when async is not being used. I would like to know if this is a bug and if not a bug, I would like to understand why :)

bradennapier commented 6 years ago

@stouf have you looked at the inferred type when using await? I honestly did not read all of this but I have run into an issue with the way async functions return inferred responses which causes the Promise class itself to be returned.

The reason I think it might be related even though your errors don't show this issue specifically is because I have seen those same errors and it ended up being a masked version of this error in almost every case.

Flow then uses this in place of other objects at times (and therefore will get confused with unions) without further refinement.

Your issue only happens on multiple files - I also can not re-create the issue within a repro using try-flow so it is likely due to using async functions across files.

try using the following in your example to do refinement:

const whatever = await WhateverElse();
if (!whatever instanceof Promise) {
    //  ... now do you have the issue?
}

image

Here you can see that it is possibly returning Promise as a class. However, the return type should be Promise<void> | Promsie<void> | Promise<{ [key: string]: mixed }>

image


With the Refinement:

image

Without the Refinement:

image


The error I get is different but I am not using union types. I would expect them to match if I were:

Promise
  This type is incompatible with the expected param type of
  union: type application of class `Promise` | type parameter `T` of await
  ---
    Member 1:
      type application of class `Promise`
      ---
        Error:
        object type
        ---
        This type is incompatible with
        object type
        ---
          Property `connected` is incompatible:
    Member 2:
      type parameter `T` of await
      ---
      Error:
      property `connected`
      ---
      Property not found in
      Promise

that being said - i still can't re-create the situation evne though it has happened mutliple times. It appears to be when chaining and using .then and .catch at intermediate places in the chain.

Since I haven't beena ble to re-create it yet, I have not yet created an issue for it.

stouf commented 6 years ago

@bradennapier Thanks for commenting here!

have you looked at the inferred type when using await?

Yep, I did and the type definitions seem to perfectly match. If we use my second example (way shorter than my first one):

// file2.js

// @flow

/*::
import * as file1 from './file1.js';
*/

const test/*: typeof file1.isGreaterThanTen*/ = async (x) => {
  return true;
};

const a = test(5);

Here, the type of a is Promise<boolean>, which matches with the type of file1.isGreaterThanThen which Flow computes as (x: number) => Promise<boolean>.

However, your assumption about how the Promise class is being computed may be correct. With the same example, if I replace

// file1.js

async function isGreaterThanTen(x/*: number*/) {
  // the code
}

with

async function isGreaterThanTen(x/*: number*/)/*: Promise<*>*/ {
  // the code
}

Flow knows which type to pick up. I explicitly annotate that the function returns a Promise but I let the existential operator compute the type of the promise.

loyd commented 6 years ago

Any progress?

SamChou19815 commented 3 weeks ago

not enough info to repro