Add --types flag to flow cycle

Summary:
Type dependencies are different than value ones. For example the import of `foo` in
```
// file b.js
import {foo} from 'a';
foo();
module.exports = class A {};
```
does not affect the type signature of file a.js since it does not affect its exports' type.

The edge
```
a.js --> b.js
```
is only a value dependency but not a type one.

This affects the behavior of `flow cycle` in that a cycle computed based on type
dependencies only might differ from one that accounts for both.

In Flow classic mode we took all dependencies into account to compute `flow cycle`.
This behavior had regressed in types-first since `flow cycle` only accounted for
type dependencies. This change restores the behavior for the default case
of this command, and also adds an option `--types` to only account for type dependencies
when computing cycles.

In addressing performance issues due to cycles in types-first, one should try to break
type-dependencies instead of the combination of the two.

Reviewed By: gabelevi

Differential Revision: D16018435

fbshipit-source-id: c059bc480be536e7185c8e92f8bc52cc3944c8b4

src/commands/cycleCommand.ml

@@ -25,11 +25,12 @@ let spec = {
     |> connect_flags
     |> root_flag
     |> strip_root_flag
+    |> flag "--types" no_arg ~doc:"Only consider type dependencies"
     |> anon "FILE..." (required string)
   )
 }
 
-let main base_flags option_values root strip_root file () =
+let main base_flags option_values root strip_root types_only file () =
   let flowconfig_name = base_flags.Base_flags.flowconfig_name in
   let file = expand_path file in
   let root = guess_root flowconfig_name root in
@@ -39,7 +40,7 @@ let main base_flags option_values root strip_root file () =
     else f
   in
   (* connect to server *)
-  let request = ServerProt.Request.CYCLE { filename = file; } in
+  let request = ServerProt.Request.CYCLE { filename = file; types_only } in
   match connect_and_make_request flowconfig_name option_values root request with
   | ServerProt.Response.CYCLE (Error msg) ->
     FlowExitStatus.(exit ~msg Unknown_error)

src/server/command_handler/commandHandler.ml

@@ -262,11 +262,15 @@ let output_dependencies ~env root strip_root outfile =
   let%lwt () = Lwt_io.close out in
   ok_unit |> Lwt.return
 
-let get_cycle ~env fn =
+let get_cycle ~env fn types_only =
   (* Re-calculate SCC *)
   let parsed = env.ServerEnv.files in
   let dependency_info = env.ServerEnv.dependency_info in
-  let dependency_graph = Dependency_info.dependency_graph dependency_info in
+  let dependency_graph =
+    if types_only
+    then Dependency_info.dependency_graph dependency_info
+    else Dependency_info.all_dependency_graph dependency_info
+  in
   Lwt.return (Ok (
     let components = Sort_js.topsort ~roots:parsed dependency_graph in
 
@@ -400,8 +404,8 @@ let handle_batch_coverage ~genv ~options ~profiling:_ ~env ~batch ~trust =
   let%lwt response = batch_coverage ~options ~genv ~env ~batch ~trust in
   Lwt.return (ServerProt.Response.BATCH_COVERAGE response, None)
 
-let handle_cycle ~fn ~profiling:_ ~env =
-  let%lwt response = get_cycle ~env fn in
+let handle_cycle ~fn ~types_only ~profiling:_ ~env =
+  let%lwt response = get_cycle ~env fn types_only in
   Lwt.return (env, ServerProt.Response.CYCLE response, None)
 
 let handle_dump_types ~options ~input ~profiling ~env =
@@ -575,11 +579,11 @@ let get_ephemeral_handler genv command =
   | ServerProt.Request.BATCH_COVERAGE { batch; wait_for_recheck; trust } ->
     mk_parallelizable ~wait_for_recheck ~options
       (handle_batch_coverage ~genv ~options ~trust ~batch)
-  | ServerProt.Request.CYCLE { filename; } ->
+  | ServerProt.Request.CYCLE { filename; types_only } ->
     (* The user preference is to make this wait for up-to-date data *)
     let file_options = Options.file_options options in
     let fn = Files.filename_from_string ~options:file_options filename in
-    Handle_nonparallelizable (handle_cycle ~fn)
+    Handle_nonparallelizable (handle_cycle ~fn ~types_only)
   | ServerProt.Request.DUMP_TYPES { input; wait_for_recheck; } ->
     mk_parallelizable ~wait_for_recheck ~options (handle_dump_types ~options ~input)
   | ServerProt.Request.FIND_MODULE { moduleref; filename; wait_for_recheck; } ->

src/server/protocol/serverProt.ml

@@ -20,7 +20,7 @@ module Request = struct
     }
   | COVERAGE of { input: File_input.t; force: bool; wait_for_recheck: bool option; trust : bool }
   | BATCH_COVERAGE of { batch : string list; wait_for_recheck: bool option; trust : bool }
-  | CYCLE of { filename: string; }
+  | CYCLE of { filename: string; types_only: bool }
   | DUMP_TYPES of { input: File_input.t; wait_for_recheck: bool option; }
   | FIND_MODULE of { moduleref: string; filename: string; wait_for_recheck: bool option; }
   | FIND_REFS of {
@@ -84,8 +84,8 @@ module Request = struct
       Printf.sprintf "%s" "batch-coverage"
   | COVERAGE { input; force=_; wait_for_recheck=_; trust=_ } ->
       Printf.sprintf "coverage %s" (File_input.filename_of_file_input input)
-  | CYCLE { filename; } ->
-      Printf.sprintf "cycle %s" filename
+  | CYCLE { filename; types_only } ->
+      Printf.sprintf "cycle (types_only: %b) %s" types_only filename
   | GRAPH_DEP_GRAPH _ ->
       Printf.sprintf "dep-graph"
   | DUMP_TYPES { input; wait_for_recheck=_; } ->

tests/cycle-command/.flowconfig

@@ -0,0 +1,13 @@
+[ignore]
+
+[include]
+
+[libs]
+
+[lints]
+
+[options]
+
+experimental.well_formed_exports=true
+
+[strict]

tests/cycle-command/.testconfig

@@ -0,0 +1,2 @@
+shell: test.sh
+skip_saved_state: true

tests/cycle-command/cycle-command.exp

@@ -0,0 +1,23 @@
+
+Start server in classic mode
+
+Value cycle should include both files
+digraph {
+  "fileB.js" -> "fileA.js"
+  "fileA.js" -> "fileB.js"
+}
+Type cycle should include both files
+digraph {
+  "fileB.js" -> "fileA.js"
+  "fileA.js" -> "fileB.js"
+}
+Start server in types-first mode
+
+Value cycle should include both files
+digraph {
+  "fileB.js" -> "fileA.js"
+  "fileA.js" -> "fileB.js"
+}
+Type cycle should be empty
+digraph {
+}

tests/cycle-command/fileA.js

@@ -0,0 +1,7 @@
+// @flow
+
+const B = require('./fileB');
+
+class A {}
+
+module.exports = A;

tests/cycle-command/fileB.js

@@ -0,0 +1,7 @@
+// @flow
+
+const A = require("./fileA");
+
+class B extends A {}
+
+module.exports = B;

tests/cycle-command/test.sh

@@ -0,0 +1,27 @@
+#!/bin/bash
+
+assert_ok "$FLOW" stop
+
+printf "\\nStart server in classic mode\\n"
+start_flow .
+
+printf "\\nValue cycle should include both files\\n"
+assert_ok "$FLOW" cycle --strip-root fileA.js
+
+printf "\\nType cycle should include both files\\n"
+assert_ok "$FLOW" cycle --strip-root --types fileA.js
+
+assert_ok "$FLOW" stop
+
+printf "\\nStart server in types-first mode\\n"
+start_flow . --types-first
+
+printf "\\nValue cycle should include both files\\n"
+assert_ok "$FLOW" cycle --strip-root fileA.js
+
+printf "\\nType cycle should be empty\\n"
+assert_ok "$FLOW" cycle --strip-root --types fileA.js
+
+assert_ok "$FLOW" stop
+
+printf "\\n"