propagating opts from merge phase to check phase of types-first

Summary:
In types-first, the merge phase is repurposed to merge dependencies. Some of those dependencies discover that their signatures have not changed. However, this information was not previously propagated to the (new) check phase, which caused all files that were merged to be checked again. (To add insult to injury, we were actually also checking unchanged but previously checked files again, but count that in the clowniness bucket.)

This diff fixes this situation. In particular, it collects information about which files had their signatures unchanged from Merge_stream, and uses this information to filter out dependents whose dependencies have not changed their signatures. This has to be done carefully so that errors for the skipped files are preserved.

Reviewed By: nmote

Differential Revision: D15197843

fbshipit-source-id: f979db1358f7ada2bba5cb65e27ab5daa37ef88f

Author: avikchaudhuri

src/services/inference/merge_service.ml

@@ -18,7 +18,14 @@ type 'a merge_job =
   File_key.t Nel.t ->
   'a merge_job_result
 
-type 'a merge_results = 'a merge_job_results * int (* skipped count *)
+type sig_opts_data = {
+  skipped_count: int;
+  sig_new_or_changed: FilenameSet.t;
+}
+
+type 'a merge_results =
+  'a merge_job_results *
+   sig_opts_data
 
 type merge_strict_context_result = {
   cx: Context.t;
@@ -402,10 +409,11 @@ let merge_runner
     ~next:(Merge_stream.next stream)
   in
   let total_files = Merge_stream.total_files stream in
-  let skipped_files = Merge_stream.skipped_files stream in
-  Hh_logger.info "Merge skipped %d of %d modules" skipped_files total_files;
+  let skipped_count = Merge_stream.skipped_count stream in
+  let sig_new_or_changed = Merge_stream.sig_new_or_changed master_mutator in
+  Hh_logger.info "Merge skipped %d of %d modules" skipped_count total_files;
   let elapsed = Unix.gettimeofday () -. start_time in
   if Options.should_profile options then Hh_logger.info "merged (strict) in %f" elapsed;
-  Lwt.return (ret, skipped_files)
+  Lwt.return (ret, { skipped_count; sig_new_or_changed })
 
 let merge_strict = merge_runner ~job:merge_strict_component

src/services/inference/merge_service.mli

@@ -16,7 +16,14 @@ type 'a merge_job =
   File_key.t Nel.t ->
   'a merge_job_result
 
-type 'a merge_results = 'a merge_job_results * int (* skipped count *)
+type sig_opts_data = {
+  skipped_count: int;
+  sig_new_or_changed: FilenameSet.t;
+}
+
+type 'a merge_results =
+  'a merge_job_results *
+   sig_opts_data
 
 type merge_strict_context_result = {
   cx: Context.t;

src/services/inference/merge_stream.ml

@@ -232,5 +232,9 @@ let merge ~master_mutator ~reader stream =
     update_server_status stream;
     List.rev_append merged acc
 
+(* NOTE: call these functions only at the end of merge, not during. *)
 let total_files stream = stream.total_files
-let skipped_files stream = stream.skipped_files
+let skipped_count stream = stream.skipped_files
+(* See explanation in Context_heaps for why calling this function at the end of merge returns files
+   whose signatures are new or have changed. *)
+let sig_new_or_changed = Context_heaps.Merge_context_mutator.unrevived_files

src/services/inference/merge_stream.mli

@@ -33,4 +33,5 @@ val merge:
   'a merge_result -> 'a merge_result -> 'a merge_result
 
 val total_files: 'a t -> int
-val skipped_files: 'a t -> int
+val skipped_count: 'a t -> int
+val sig_new_or_changed: Context_heaps.Merge_context_mutator.master_mutator -> FilenameSet.t

src/services/inference/types_js.ml

@@ -323,7 +323,7 @@ let run_merge_service
     acc
     =
   with_timer_lwt ~options "Merge" profiling (fun () ->
-    let%lwt merged, skipped_count = Merge_service.merge_strict
+    let%lwt merged, { Merge_service.skipped_count; sig_new_or_changed } = Merge_service.merge_strict
       ~master_mutator ~worker_mutator ~reader ~intermediate_result_callback ~options ~workers
       dependency_graph component_map recheck_map
     in
@@ -349,7 +349,7 @@ let run_merge_service
         update_errset errors file new_errors, warnings, suppressions, coverage
     ) acc merged
     in
-    Lwt.return (errs, warnings, suppressions, coverage, skipped_count)
+    Lwt.return (errs, warnings, suppressions, coverage, skipped_count, sig_new_or_changed)
   )
 
 (* This function does some last minute preparation and then calls into the merge service, which
@@ -362,7 +362,6 @@ let merge
   ~profiling
   ~workers
   ~errors
-  ~unchanged_checked
   ~to_merge
   ~components
   ~recheck_map
@@ -371,6 +370,7 @@ let merge
   ~persistent_connections
   ~prep_merge =
   let { ServerEnv.local_errors; merge_errors; warnings; suppressions; } = errors in
+  let coverage = FilenameMap.empty in
 
   let%lwt send_errors_over_connection =
     match persistent_connections with
@@ -451,15 +451,14 @@ let merge
      initially.
   *)
   Hh_logger.info "to_merge: %s" (CheckedSet.debug_counts_to_string to_merge);
-  Hh_logger.info "unchanged_checked: %s" (CheckedSet.debug_counts_to_string unchanged_checked);
   Hh_logger.info "Calculating dependencies";
   MonitorRPC.status_update ~event:ServerStatus.Calculating_dependencies_progress;
   let files_to_merge = CheckedSet.all to_merge in
   let%lwt dependency_graph, component_map =
     calc_deps ~options ~profiling ~dependency_graph ~components files_to_merge in
 
   Hh_logger.info "Merging";
-  let%lwt merge_errors, warnings, suppressions, coverage, skipped_count =
+  let%lwt merge_errors, warnings, suppressions, coverage, skipped_count, sig_new_or_changed =
     let intermediate_result_callback results =
       let errors = lazy (
         Core_list.map ~f:(fun (file, result) ->
@@ -486,7 +485,7 @@ let merge
 
     let merge_start_time = Unix.gettimeofday () in
 
-    let%lwt merge_errors, warnings, suppressions, coverage, skipped_count =
+    let%lwt result =
       run_merge_service
         ~master_mutator
         ~worker_mutator
@@ -498,7 +497,7 @@ let merge
         dependency_graph
         component_map
         recheck_map
-        (merge_errors, warnings, suppressions, FilenameMap.empty)
+        (merge_errors, warnings, suppressions, coverage)
     in
     let%lwt () =
       if Options.should_profile options
@@ -515,24 +514,44 @@ let merge
     in
 
     Hh_logger.info "Done";
-    Lwt.return (merge_errors, warnings, suppressions, coverage, skipped_count)
+    Lwt.return result
   in
 
-  let checked = CheckedSet.union unchanged_checked to_merge in
-  Hh_logger.info "Checked set: %s" (CheckedSet.debug_counts_to_string checked);
-
   let errors = { ServerEnv.local_errors; merge_errors; warnings; suppressions } in
   let cycle_leaders = component_map
     |> Utils_js.FilenameMap.elements
     |> Core_list.map ~f:(fun (leader, members) -> (leader, Nel.length members))
     |> Core_list.filter ~f:(fun (_, member_count) -> member_count > 1) in
-  Lwt.return (checked, cycle_leaders, errors, coverage, skipped_count)
+  Lwt.return (cycle_leaders, errors, coverage, skipped_count, sig_new_or_changed)
 
-let check_files ~options ~reader ~workers errors coverage checked_files =
-  let files = FilenameSet.union (CheckedSet.focused checked_files) (CheckedSet.dependents checked_files) in
+let check_files
+  ~reader
+  ~options
+  ~workers
+  ~errors
+  ~updated_errors
+  ~coverage
+  ~merged_files
+  ~sig_new_or_changed
+  ~dependency_info =
   match options.Options.opt_arch with
-  | Options.Classic -> Lwt.return (errors, coverage)
+  | Options.Classic -> Lwt.return (updated_errors, coverage)
   | Options.TypesFirst ->
+    Hh_logger.info "Check prep";
+    Hh_logger.info "new or changed signatures: %d" (FilenameSet.cardinal sig_new_or_changed);
+    let focused_to_check = CheckedSet.focused merged_files in
+    let merged_dependents = CheckedSet.dependents merged_files in
+    let skipped_count = ref 0 in
+    let all_dependency_graph = Dependency_info.all_dependency_graph dependency_info in
+    (* skip dependents whenever none of their dependencies have new or changed signatures *)
+    let dependents_to_check =
+      FilenameSet.filter (fun f ->
+        (FilenameSet.exists (fun f' -> FilenameSet.mem f' sig_new_or_changed)
+         @@ FilenameMap.find_unsafe f all_dependency_graph)
+        || (incr skipped_count; false)
+      ) @@ merged_dependents in
+    Hh_logger.info "Check will skip %d files" !skipped_count;
+    let files = FilenameSet.union focused_to_check dependents_to_check in
     let job = List.fold_left (fun (errors, warnings, suppressions, coverage) file ->
       let new_errors, new_warnings, new_suppressions, new_coverage =
         Merge_service.check_file options ~reader file in
@@ -547,27 +566,32 @@ let check_files ~options ~reader ~workers errors coverage checked_files =
       Error_suppressions.empty,
       FilenameMap.empty in
     let merge
-        (errors, warnings, suppressions, coverage)
-        (new_errors, new_warnings, new_suppressions, new_coverage) =
-      FilenameMap.union errors new_errors,
-      FilenameMap.union warnings new_warnings,
+      (new_errors, new_warnings, new_suppressions, new_coverage)
+      (errors, warnings, suppressions, coverage) =
+      FilenameMap.union new_errors errors, (* WARNING! order matters (new wins over old) *)
+      FilenameMap.union new_warnings warnings, (* WARNING! order matters (new wins over old) *)
       Error_suppressions.update_suppressions suppressions new_suppressions,
-      FilenameMap.union coverage new_coverage
+      FilenameMap.union new_coverage coverage (* WARNING! order matters (new wins over old) *)
       in
 
     Hh_logger.info "Checking files";
     let progress_fn ~total ~start ~length:_ =
       MonitorRPC.status_update
         ServerStatus.(Checking_progress { total = Some total; finished = start })
     in
-    let%lwt merge_errors, warnings, suppressions, coverage = MultiWorkerLwt.call
+    let%lwt new_errors, new_warnings, new_suppressions, new_coverage = MultiWorkerLwt.call
         workers
         ~job
         ~neutral
         ~merge
         ~next:(MultiWorkerLwt.next ~progress_fn ~max_size:100 workers
                  (FilenameSet.elements files))
     in
+    let merge_errors, warnings, suppressions, coverage =
+      merge
+        (* NOTE: Order matters when there is overlap. We want new stuff to overwrite old stuff. *)
+        (new_errors, new_warnings, new_suppressions, new_coverage)
+        ServerEnv.(errors.merge_errors, errors.warnings, errors.suppressions, coverage) in
     Hh_logger.info "Done";
     let errors = { errors with
       ServerEnv.merge_errors;
@@ -626,12 +650,12 @@ let ensure_checked_dependencies ~options ~reader ~env file file_sig =
       else acc
     | None -> acc (* complain elsewhere about required module not found *)
   ) resolved_requires CheckedSet.empty in
-  let unchanged_checked = env.ServerEnv.checked_files in
+  let checked = env.ServerEnv.checked_files in
 
   (* Often, all dependencies have already been checked, so infer_input contains no unchecked files.
    * In that case, let's short-circuit typecheck, since a no-op typecheck still takes time on
    * large repos *)
-  let unchecked_dependencies = CheckedSet.diff infer_input unchanged_checked in
+  let unchecked_dependencies = CheckedSet.diff infer_input checked in
   if CheckedSet.is_empty unchecked_dependencies
   then Lwt.return_unit
   else begin
@@ -1466,14 +1490,13 @@ end = struct
 
     let dependency_graph = Dependency_info.dependency_graph dependency_info in
     (* recheck *)
-    let%lwt checked_files, cycle_leaders, errors, coverage, skipped_count = merge
+    let%lwt cycle_leaders, updated_errors, coverage, skipped_count, sig_new_or_changed = merge
       ~transaction
       ~reader
       ~options
       ~profiling
       ~workers
       ~errors
-      ~unchanged_checked
       ~to_merge
       ~components
       ~recheck_map
@@ -1493,7 +1516,20 @@ end = struct
       ))
     in
 
-    let%lwt errors, coverage = check_files ~options ~reader ~workers errors coverage checked_files in
+    let merged_files = to_merge in
+    let%lwt errors, coverage = check_files
+      ~reader
+      ~options
+      ~workers
+      ~errors
+      ~updated_errors
+      ~coverage
+      ~merged_files
+      ~sig_new_or_changed
+      ~dependency_info in
+
+    let checked_files = CheckedSet.union unchanged_checked merged_files in
+    Hh_logger.info "Checked set: %s" (CheckedSet.debug_counts_to_string checked_files);
 
     (* NOTE: unused fields are left in their initial empty state *)
     env.ServerEnv.collated_errors := None;
@@ -2073,12 +2109,10 @@ let full_check ~profiling ~options ~workers ?focus_targets env =
     let%lwt infer_input = files_to_infer
       ~options ~reader ?focus_targets ~profiling ~parsed ~dependency_info in
 
-    let unchanged_checked = CheckedSet.empty in
-
     let%lwt to_merge, components, recheck_map =
       include_dependencies_and_dependents
         ~options ~profiling
-        ~unchanged_checked
+        ~unchanged_checked:CheckedSet.empty
         ~infer_input
         ~dependency_info
         ~all_dependent_files:FilenameSet.empty
@@ -2090,21 +2124,32 @@ let full_check ~profiling ~options ~workers ?focus_targets env =
     let%lwt () = ensure_parsed ~options ~profiling ~workers ~reader to_merge in
 
     let dependency_graph = Dependency_info.dependency_graph dependency_info in
-    let%lwt (checked_files, _, errors, coverage, _) = merge
+    let%lwt _, updated_errors, coverage, _, sig_new_or_changed = merge
       ~transaction
       ~reader
       ~options
       ~profiling
       ~workers
       ~errors
-      ~unchanged_checked
       ~to_merge
       ~components
       ~recheck_map
       ~dependency_graph
       ~deleted:FilenameSet.empty
       ~persistent_connections:None
       ~prep_merge:None in
-    let%lwt errors, coverage = check_files ~options ~reader ~workers errors coverage checked_files in
+    let merged_files = to_merge in
+    let%lwt errors, coverage = check_files
+      ~reader
+      ~options
+      ~workers
+      ~errors
+      ~updated_errors
+      ~coverage
+      ~merged_files
+      ~sig_new_or_changed
+      ~dependency_info in
+    let checked_files = merged_files in
+    Hh_logger.info "Checked set: %s" (CheckedSet.debug_counts_to_string checked_files);
     Lwt.return { env with ServerEnv.checked_files; errors; coverage }
   )

src/state/heaps/context/context_heaps.ml

@@ -69,6 +69,7 @@ module Merge_context_mutator: sig
   val add_merge_on_exn:
     (worker_mutator -> options:Options.t -> File_key.t Nel.t -> unit) Expensive.t
   val revive_files: master_mutator -> Utils_js.FilenameSet.t -> unit
+  val unrevived_files: master_mutator -> Utils_js.FilenameSet.t
 end = struct
   type master_mutator = Utils_js.FilenameSet.t ref
   type worker_mutator = unit
@@ -149,6 +150,17 @@ end = struct
     assert (FilenameSet.is_empty (FilenameSet.diff files (!oldified_files)));
     oldified_files := FilenameSet.diff (!oldified_files) files;
     revive_merge_batch files
+
+  (* WARNING: Only call this function at the end of merge!!! Calling it during merge will return
+     meaningless results.
+
+     Initially, `oldified_files` contains the set of files to be merged (see Merge_stream). During
+     merge, we call `revive_files` for files whose signatures have not changed. So the remaining
+     `oldified_files` at the end of merge must contain the set of files whose signatures are new or
+     have changed. In principle, we could maintain this state separately in Merge_stream, but it
+     seems wasteful to do so. *)
+  let unrevived_files oldified_files =
+    !oldified_files
 end
 
 module type READER = sig

src/state/heaps/context/context_heaps.mli

@@ -36,4 +36,5 @@ module Merge_context_mutator: sig
   val add_merge_on_exn:
     (worker_mutator -> options:Options.t -> File_key.t Nel.t -> unit) Expensive.t
   val revive_files: master_mutator -> Utils_js.FilenameSet.t -> unit
+  val unrevived_files: master_mutator -> Utils_js.FilenameSet.t
 end