facebook / flow

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

Flow v0.34.0 regression: Flow check never completes in project #2724

Closed Macil closed 8 years ago

Macil commented 8 years ago

I just found that Flow v0.34.0 doesn't appear to ever terminate on the project react-menu-list. Flow v0.33.0 works fine and runs in under 10 seconds. I can reproduce all of this in OS X and Linux.

Here's the instructions to reproduce the issue yourself:

$ git clone https://github.com/StreakYC/react-menu-list.git
$ cd react-menu-list/
$ git checkout flow34
$ npm i
$ ./node_modules/.bin/flow
gabelevi commented 8 years ago

Thanks for the report with the detailed repro. I think @jeffmo is looking into this, but I'll confirm tomorrow. Stay tuned and sorry for the trouble!

gabelevi commented 8 years ago

I've taken a quick look, and it seems to be some bad interaction with the kefir-bus npm module. I'm able to get flow 34 working in this repository with

$ cd react-menu-list
$ npm install -g flow-typed
$ flow-typed create-stub kefir-bus

And then adding the following to the ignores section:

<PROJECT_ROOT>/node_modules/kefir-bus/.*

However, it doesn't repro in isolation. I tried something like

$ mkdir foo
$ cd foo
$ npm init -y
$ flow init
$ npm install kefir-bus flow-bin
$ ./node_modules/.bin/flow check

but flow terminated.

Here's a gist of the last 3000 lines after running flow check --verbose-indent --profile for a few minutes: https://gist.github.com/gabelevi/ccbee0c3503467bbf125a5b27bb10bea

We will keep investigating. Thanks again for the report!

gabelevi commented 8 years ago

For those reading, here is kefir-bus/index.js.flow

gabelevi commented 8 years ago

And kefir/kefir.js.flow

samwgoldman commented 8 years ago

OK, after some digging, I have a single-file repro. The issue isn't non-termination, but time complexity blowup. That is, given enough time Flow would finish, but the time required grows very quickly.

/* @flow */

class Observable {}

type Bus<T> = Observable & {
  +a: () => Bus<T>;
  +b: () => Bus<T>;
  +c: () => Bus<T>;
  // add more properties to observe blowup
};

declare var bus: Bus<any>;

(bus: Bus<any>);

To reproduce, it's sufficient to have a polymorphic intersection type where one of the branches refers back to the intersection itself. It's important that the object properties be covariant for this to reproduce. Invariant properties have the effect of pinning down types earlier, which prevents the blowup.

The reason this code exhibits this behavior now and not in earlier versions is because we changed the interpretation of "method syntax" properties in object types to be covariant. You can work around the issue by making those properties invariant.

I am still working on a real fix, because we shouldn't exhibit this behavior even with covariant properties, but wanted to report back with the repro and workaround in the mean time.

Macil commented 7 years ago

The commit referencing this says it only partially fixed the issue, so should this be re-opened?

hwfwalton commented 7 years ago

Still experiencing this on v0.35.0

gabelevi commented 7 years ago

I'm afraid the fix just missed the v0.35.0 branch cut. It will be in 0.36.0, which should be deployed this week.