non termination of calls to polymorphic functions

Summary:
Various outstanding reports of non-termination had the same root cause: we were
not caching instantiations when executing calls to polymorphic functions, which
led to cycles where different instantiations would continue to spew
different-looking, but ultimately redundant, constraints.

We already solve a similar problem with method calls on type applications. Here
we start from the same solution, but need to extend and tweak it a bit to make
it work.

Reviewed By: samwgoldman

Differential Revision: D4521320

fbshipit-source-id: e074af41546b3c4d1ad38fb181062c3f347099f6

src/common/reason.ml

@@ -570,9 +570,11 @@ let is_constant_property_reason r =
     else is_not_lowercase x 0 (len - 1)
   | _ -> false
 
-let is_method_call_reason x r =
+let is_typemap_reason r =
   match desc_of_reason r with
-  | RMethodCall (Some y) -> x = y
+  | RTupleMap
+  | RObjectMap
+  | RObjectMapi -> true
   | _ -> false
 
 let is_derivable_reason r =

src/common/reason.mli

@@ -177,7 +177,7 @@ val is_instantiable_reason: reason -> bool
 
 val is_constant_property_reason: reason -> bool
 
-val is_method_call_reason: string -> reason -> bool
+val is_typemap_reason: reason -> bool
 
 val derivable_reason: reason -> reason
 val is_derivable_reason: reason -> bool

src/typing/class_sig.ml

@@ -322,7 +322,7 @@ let mk_super cx tparams_map c targs = Type.(
          this-specialize `c`. *)
       let reason = reason_of_t c in
       let c = Flow.mk_tvar_derivable_where cx reason (fun tvar ->
-        Flow.flow cx (c, SpecializeT (reason, reason, false, [], tvar))
+        Flow.flow cx (c, SpecializeT (reason, reason, None, [], tvar))
       ) in
       ThisTypeAppT (c, this, [])
   | Some params ->

src/typing/debug_js.ml

@@ -467,7 +467,7 @@ and _json_of_use_t_impl json_cx t = Hh_json.(
     ]
 
   | SpecializeT (_, _, cache, targs, tvar) -> [
-      "cache", JSON_Bool cache;
+      "cache", json_of_specialize_cache json_cx cache;
       "types", JSON_Array (List.map (_json_of_t json_cx) targs);
       "tvar", _json_of_t json_cx tvar
     ]
@@ -1180,6 +1180,19 @@ and json_of_lookup_action_impl json_cx action = Hh_json.(
   )
 )
 
+and json_of_specialize_cache json_cx =
+  check_depth json_of_specialize_cache_impl json_cx
+and json_of_specialize_cache_impl json_cx cache = Hh_json.(
+  JSON_Object (
+    match cache with
+    | None -> []
+    | Some rs -> [
+        "reasons", JSON_Array
+          (List.map (json_of_reason ~strip_root:json_cx.strip_root) rs);
+      ]
+  )
+)
+
 and json_of_obj_assign_kind json_cx =
   check_depth json_of_obj_assign_kind_impl json_cx
 
@@ -1510,6 +1523,12 @@ and dump_use_t_ (depth, tvars) cx t =
   | SuperProp p -> spf "Super %s" (prop p)
   in
 
+  let specialize_cache = function
+    | None -> "None"
+    | Some rs -> spf "Some [%s]"
+        (String.concat "; " @@ List.map (dump_reason cx) rs)
+  in
+
   let try_flow = function
     | UnionCases (t, ts) ->
         spf "(%s, [%s])" (kid t) (String.concat "; " (List.map kid ts))
@@ -1621,8 +1640,8 @@ and dump_use_t_ (depth, tvars) cx t =
   | SetPropT (_, prop, ptype) -> p ~extra:(spf "(%s), %s"
       (propref prop)
       (kid ptype)) t
-  | SpecializeT (_, _, b, args, ret) -> p ~extra:(spf "%b, [%s], %s"
-      b (String.concat "; " (List.map kid args)) (kid ret)) t
+  | SpecializeT (_, _, cache, args, ret) -> p ~extra:(spf "%s, [%s], %s"
+      (specialize_cache cache) (String.concat "; " (List.map kid args)) (kid ret)) t
   | TestPropT (_, prop, ptype) -> p ~extra:(spf "(%s), %s"
       (propref prop)
       (kid ptype)) t

src/typing/flow_js.ml

@@ -659,7 +659,7 @@ module ImplicitTypeArgument = struct
      polymorphic types need to be implicitly instantiated, because there was no
      explicit instantiation (via a type application), or when we want to cache a
      unique instantiation and unify it with other explicit instantiations. *)
-  let mk_targ cx (typeparam, reason_op) =
+  let mk_targ cx typeparam reason_op =
     let reason = replace_reason (fun desc ->
       RTypeParam (typeparam.name, desc)
     ) reason_op in
@@ -819,20 +819,23 @@ module Cache = struct
   (* Cache that limits instantiation of polymorphic definitions. Intuitively,
      for each operation on a polymorphic definition, we remember the type
      arguments we use to specialize the type parameters. An operation is
-     identified by its reason: we don't use the entire operation for caching
-     since it may contain the very type variables we are trying to limit the
-     creation of with the cache. In other words, the cache would be useless if
-     we considered those type variables as part of the identity of the
-     operation. *)
+     identified by its reason, and possibly the reasons of its arguments. We
+     don't use the entire operation for caching since it may contain the very
+     type variables we are trying to limit the creation of with the cache (e.g.,
+     those representing the result): the cache would be useless if we considered
+     those type variables as part of the identity of the operation. *)
   module PolyInstantiation = struct
-    let cache = Hashtbl.create 0
+    type cache_key = reason * op_reason
+    and op_reason = reason Nel.t
 
-    let find cx (typeparam, reason_op) =
+    let cache: (cache_key, Type.t) Hashtbl.t = Hashtbl.create 0
+
+    let find cx typeparam op_reason =
       try
-        Hashtbl.find cache (typeparam.reason, reason_op)
+        Hashtbl.find cache (typeparam.reason, op_reason)
       with _ ->
-        let t = ImplicitTypeArgument.mk_targ cx (typeparam, reason_op) in
-        Hashtbl.add cache (typeparam.reason, reason_op) t;
+        let t = ImplicitTypeArgument.mk_targ cx typeparam (Nel.hd op_reason) in
+        Hashtbl.add cache (typeparam.reason, op_reason) t;
         t
   end
 
@@ -2418,7 +2421,7 @@ let rec __flow cx ((l: Type.t), (u: Type.use_t)) trace =
         let reason_op = reason_of_use_t u in
         let reason_tapp = reason_of_t l in
         let t = mk_typeapp_instance cx
-          ~trace ~reason_op ~reason_tapp ~cache:true c ts in
+          ~trace ~reason_op ~reason_tapp ~cache:[] c ts in
         rec_flow cx trace (t, u)
 
     | (TypeAppT (c1, ts1), UseT (_, TypeAppT (c2, ts2)))
@@ -3144,7 +3147,7 @@ let rec __flow cx ((l: Type.t), (u: Type.use_t)) trace =
     | (PolyT (ids,t), SpecializeT(reason_op,reason_tapp,cache,ts,tvar))
         when ts <> [] || (poly_minimum_arity ids < List.length ids) ->
       let t_ = instantiate_poly_with_targs cx trace
-        ~reason_op ~reason_tapp ~cache (ids,t) ts in
+        ~reason_op ~reason_tapp ?cache (ids,t) ts in
       rec_flow_t cx trace (t_, tvar)
 
     | PolyT (tps, _), VarianceCheckT(_, ts, polarity) ->
@@ -3264,17 +3267,38 @@ let rec __flow cx ((l: Type.t), (u: Type.use_t)) trace =
           let inst = instantiate_poly_default_args
             cx trace ~reason_op ~reason_tapp (ids, t) in
           rec_flow cx trace (inst, u)
-      (* Special case for <fun>.apply and <fun>.call, which could be used to
-         simulate method calls on instances of generic classes, thereby
-         bypassing the specialization cache that is used in TypeAppT ~> MethodT
-         to avoid non-termination. So, we must use the specialization cache
-         here, too. *)
-      | CallT _ when
-          is_method_call_reason "apply" reason_op ||
-          is_method_call_reason "call" reason_op
-          ->
+      (* Calls to polymorphic functions may cause non-termination, e.g. when the
+         results of the calls feed back as subtle variations of the original
+         arguments. This is similar to how we may have non-termination with
+         method calls on type applications. Thus, it makes sense to replicate
+         the specialization caching mechanism used in TypeAppT ~> MethodT to
+         avoid non-termination in PolyT ~> CallT.
+
+         As it turns out, we need a bit more work here. A call may invoke
+         different cases of an overloaded polymorphic function on different
+         arguments, so we use the reasons of arguments in addition to the reason
+         of the call as keys for caching instantiations.
+
+         On the other hand, even the reasons of arguments may not offer sufficient
+         distinguishing power when the arguments have not been concretized:
+         differently typed arguments could be incorrectly summarized by common
+         type variables they flow to, causing spurious errors. In particular, we
+         don't cache calls involved in the execution of TypeMapT operations
+         ($TupleMap, $ObjectMap, $ObjectMapi) to avoid this problem.
+
+         NOTE: This is probably not the final word on non-termination with
+         generics. We need to separate the double duty of reasons in the current
+         implementation as error positions and as caching keys. As error
+         positions we should be able to subject reasons to arbitrary tweaking,
+         without fearing regressions in termination guarantees.
+      *)
+      | CallT (_, calltype) when not (is_typemap_reason reason_op) ->
+        let arg_reasons = List.map (function
+          | Arg t -> reason_of_t t
+          | SpreadArg t -> reason_of_t t
+        ) calltype.call_args_tlist in
         let t_ = instantiate_poly cx trace
-          ~reason_op ~reason_tapp ~cache:true (ids,t) in
+          ~reason_op ~reason_tapp ~cache:arg_reasons (ids,t) in
         rec_flow cx trace (t_, u)
       | _ ->
         let t_ = instantiate_poly cx trace ~reason_op ~reason_tapp (ids,t) in
@@ -6372,7 +6396,7 @@ and instantiate_poly_with_targs
   trace
   ~reason_op
   ~reason_tapp
-  ?(cache=false)
+  ?cache
   (xs,t)
   ts
   =
@@ -6398,7 +6422,7 @@ and instantiate_poly_with_targs
           AnyT reason_op, []
       | _, t::ts ->
           t, ts in
-      let t_ = cache_instantiate cx trace cache (typeparam, reason_op) t in
+      let t_ = cache_instantiate cx trace ?cache typeparam reason_op t in
       rec_flow_t cx trace (t_, subst cx map typeparam.bound);
       SMap.add typeparam.name t_ map, ts
     )
@@ -6410,12 +6434,13 @@ and instantiate_poly_with_targs
    reason for specialization, either return the type argument or, when directed,
    look up the instantiation cache for an existing type argument for the same
    purpose and unify it with the supplied type argument. *)
-and cache_instantiate cx trace cache (typeparam, reason_op) t =
-  if cache then
-    let t_ = Cache.PolyInstantiation.find cx (typeparam, reason_op) in
+and cache_instantiate cx trace ?cache typeparam reason_op t =
+  match cache with
+  | None -> t
+  | Some rs ->
+    let t_ = Cache.PolyInstantiation.find cx typeparam (reason_op, rs) in
     rec_unify cx trace t t_;
     t_
-  else t
 
 (* Instantiate a polymorphic definition with stated bound or 'any' for args *)
 (* Needed only for experimental.enforce_strict_type_args=false killswitch *)
@@ -6433,11 +6458,11 @@ and instantiate_poly_default_args cx trace ~reason_op ~reason_tapp (xs,t) =
   instantiate_poly_with_targs cx trace ~reason_op ~reason_tapp (xs,t) ts
 
 (* Instantiate a polymorphic definition by creating fresh type arguments. *)
-and instantiate_poly cx trace ~reason_op ~reason_tapp ?(cache=false) (xs,t) =
+and instantiate_poly cx trace ~reason_op ~reason_tapp ?cache (xs,t) =
   let ts = xs |> List.map (fun typeparam ->
-    ImplicitTypeArgument.mk_targ cx (typeparam, reason_op)
+    ImplicitTypeArgument.mk_targ cx typeparam reason_op
   ) in
-  instantiate_poly_with_targs cx trace ~reason_op ~reason_tapp ~cache (xs,t) ts
+  instantiate_poly_with_targs cx trace ~reason_op ~reason_tapp ?cache (xs,t) ts
 
 (* instantiate each param of a polymorphic type with its upper bound *)
 and instantiate_poly_param_upper_bounds cx typeparams =
@@ -6470,7 +6495,7 @@ and instantiate_this_class cx trace reason tc this =
 and specialize_class cx trace ~reason_op ~reason_tapp c ts =
   if ts = [] then c
   else mk_tvar_where cx reason_op (fun tvar ->
-    rec_flow cx trace (c, SpecializeT (reason_op, reason_tapp, false, ts, tvar))
+    rec_flow cx trace (c, SpecializeT (reason_op, reason_tapp, None, ts, tvar))
   )
 
 and mk_object_with_proto cx reason ?dict proto =
@@ -9241,9 +9266,9 @@ and get_builtin_typeapp cx ?trace reason x ts =
   TypeAppT(get_builtin cx ?trace x reason, ts)
 
 (* Specialize a polymorphic class, make an instance of the specialized class. *)
-and mk_typeapp_instance cx ?trace ~reason_op ~reason_tapp ?(cache=false) c ts =
+and mk_typeapp_instance cx ?trace ~reason_op ~reason_tapp ?cache c ts =
   let t = mk_tvar cx reason_op in
-  flow_opt cx ?trace (c, SpecializeT(reason_op,reason_tapp,cache,ts,t));
+  flow_opt cx ?trace (c, SpecializeT(reason_op,reason_tapp, cache, ts, t));
   mk_instance cx ?trace (reason_of_t c) t
 
 (* NOTE: the for_type flag is true when expecting a type (e.g., when processing

src/typing/type.ml

@@ -345,7 +345,7 @@ module rec TypeTerm : sig
         The first reason is the reason why we're specializing. The second
         reason points to the type application itself
     **)
-    | SpecializeT of reason * reason * bool * t list * t
+    | SpecializeT of reason * reason * specialize_cache * t list * t
     (* operation on this-abstracted classes *)
     | ThisSpecializeT of reason * t * t
     (* variance check on polymorphic types *)
@@ -537,6 +537,8 @@ module rec TypeTerm : sig
      * whatever type it resolves to *)
     | ResolveSpreadT of reason * resolve_spread_type
 
+  and specialize_cache = reason list option
+
   and predicate =
     | AndP of predicate * predicate
     | OrP of predicate * predicate

tests/call_caching1/.flowconfig

@@ -0,0 +1,9 @@
+[ignore]
+
+[include]
+
+[libs]
+lib
+
+[options]
+no_flowlib=true

tests/call_caching1/call_caching1.exp

@@ -0,0 +1 @@
+Found 0 errors

tests/call_caching1/lib/core.js

@@ -0,0 +1,25 @@
+declare class Array<T> {
+    @@iterator(): Iterator<T>;
+    map<U>(callbackfn: (value: T, index: number, array: Array<T>) => U, thisArg?: any): Array<U>;
+}
+
+type IteratorResult<Yield,Return> =
+  | { done: true, value?: Return }
+  | { done: false, value: Yield };
+
+interface $Iterator<+Yield,+Return,-Next> {
+    @@iterator(): $Iterator<Yield,Return,Next>;
+    next(value?: Next): IteratorResult<Yield,Return>;
+}
+type Iterator<+T> = $Iterator<T,void,void>;
+
+interface $Iterable<+Yield,+Return,-Next> {
+    @@iterator(): $Iterator<Yield,Return,Next>;
+}
+type Iterable<+T> = $Iterable<T,void,void>;
+
+declare class Map<K, V> {
+    @@iterator(): Iterator<[K, V]>;
+    constructor(iterable: ?Iterable<[K, V]>): void;
+    set(key: K, value: V): Map<K, V>;
+}

tests/call_caching1/lib/immutable.js

@@ -0,0 +1,8 @@
+declare module "immutable" {
+  declare class Map<K,V> {
+    static <K,V>(iter: Iterator<[K,V]>): Map<K,V>;
+    static <K:string,V>(object: {+[k:K]:V}): Map<K,V>;
+
+    set(key: K, value: V): this;
+  }
+}

tests/call_caching1/test.js

@@ -0,0 +1,10 @@
+// @flow
+
+const Immutable = require('immutable');
+
+const tasksPerStatusMap = new Map(
+  [].map(taskStatus => [taskStatus, new Map()]),
+);
+for (let [taskStatus, tasksMap] of tasksPerStatusMap) {
+  tasksPerStatusMap.set(taskStatus, Immutable.Map(tasksMap));
+}

tests/call_caching1/test2.js

@@ -0,0 +1,18 @@
+/* @flow */
+
+declare class Bar<K> {
+  update<K_>(updater: (value: this) => Bar<K_>): Bar<K_>;
+}
+
+declare function foo<U>(
+  initialValue: U,
+  callbackfn: (previousValue: U) => U
+): U;
+
+declare var items: Bar<string>;
+declare var updater: (value: Bar<string>) => Bar<string>;
+
+foo(
+  items,
+  (acc) => acc.update(updater)
+);

tests/call_caching1/test3.js

@@ -0,0 +1,16 @@
+// @flow
+
+declare class ImmBox<T> {
+  static <U>(x: any): ImmBox<U>;
+  static (x: any): any;
+}
+
+declare class Box<T> {
+  constructor(x: T): void;
+  set(value: T): void;
+  get(): T;
+}
+
+const outer = new Box();
+const inner = outer.get();
+outer.set(ImmBox(inner));

tests/call_caching2/.flowconfig

@@ -0,0 +1,9 @@
+[ignore]
+
+[include]
+
+[libs]
+lib
+
+[options]
+no_flowlib=true

tests/call_caching2/call_caching2.exp

@@ -0,0 +1 @@
+Found 0 errors