distinguish between union branches for polymorphic instantiation caching

Summary:
Consider the following code:
```
type Filter = [number] | [string];

function convert(filter: Filter) {
    filter.slice(0);
}
```

When we call `slice` on the `filter` object, this produces a `UnionT ~> MethodT` flow, which decomposes into two separate `t ~> MethodT` flows, one for each branch of the union. It is important to note, however, that because `t` in this case is a tuple (this also applies to arrays and read-only arrays), the two `t`s in each of these flows have the same reason.

When we later attempt to instantiate the polymorphic type of the return of `slice`, we unify these two `t`s with a tvar representing the return of `slice`. Unfortunately, because we use reasons to implement the polymorphic instantiation cache, and because these two `t`s have the same reason, we attempt to unify both `t`s with the same type variable, and thus get an incompatibility between `number` and `string`.

This changes the instantiation cache to take into account the `desc` of the `reason_tapp`, and changes the union lower bound decomposition logic to modify the reason of each of the lower bounds with a "transparent" (in that it will not show up in any error output) description that differentiates between the different branches of the union. This way, the cache will properly recognize the difference between these two types and correctly produce a different unification variable for each case.

Fixes facebook#8263

Reviewed By: mvitousek

Differential Revision: D19436341

fbshipit-source-id: da44513d8727f3339c71a453f46b536912d295bc

src/common/reason.ml

@@ -228,6 +228,7 @@ type 'loc virtual_reason_desc =
   | RReactConfig
   | RPossiblyMissingPropFromObj of string * 'loc virtual_reason_desc
   | RWidenedObjProp of 'loc virtual_reason_desc
+  | RUnionBranching of 'loc virtual_reason_desc * int
 [@@deriving eq]
 
 and reason_desc_function =
@@ -310,6 +311,7 @@ let rec map_desc_locs f = function
   | RPossiblyMissingPropFromObj (propname, desc) ->
     RPossiblyMissingPropFromObj (propname, map_desc_locs f desc)
   | RWidenedObjProp desc -> RWidenedObjProp (map_desc_locs f desc)
+  | RUnionBranching (desc, i) -> RUnionBranching (map_desc_locs f desc, i)
 
 type 'loc virtual_reason = {
   test_id: int option;
@@ -703,6 +705,7 @@ let rec string_of_desc = function
   | RPossiblyMissingPropFromObj (propname, desc) ->
     spf "possibly missing property `%s` in %s" propname (string_of_desc desc)
   | RWidenedObjProp desc -> string_of_desc desc
+  | RUnionBranching (desc, _) -> string_of_desc desc
 
 let string_of_reason ?(strip_root = None) r =
   let spos = string_of_aloc ~strip_root (aloc_of_reason r) in
@@ -728,6 +731,7 @@ let dump_reason ?(strip_root = None) r =
 let desc_of_reason =
   let rec loop = function
     | RTypeAlias (_, _, desc)
+    | RUnionBranching (desc, _)
     | RPolyTest (_, desc) ->
       loop desc
     | desc -> desc
@@ -1424,6 +1428,7 @@ let classification_of_reason r =
   | RReactConfig
   | RPossiblyMissingPropFromObj _
   | RWidenedObjProp _
+  | RUnionBranching _
   | RTrusted _
   | RPrivate _
   | REnum _

src/common/reason.mli

@@ -176,6 +176,7 @@ type 'loc virtual_reason_desc =
   | RReactConfig
   | RPossiblyMissingPropFromObj of string * 'loc virtual_reason_desc
   | RWidenedObjProp of 'loc virtual_reason_desc
+  | RUnionBranching of 'loc virtual_reason_desc * int
 
 and reason_desc_function =
   | RAsync

src/typing/flow_cache.ml

@@ -88,18 +88,17 @@ end
    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
-  type cache_key = ALoc.t * reason * op_reason
+  type cache_key = reason * reason * op_reason
 
   and op_reason = reason Nel.t
 
   let cache : (cache_key, Type.t) Hashtbl.t = Hashtbl.create 0
 
   let find cx reason_tapp typeparam op_reason =
-    let loc = def_aloc_of_reason reason_tapp in
-    try Hashtbl.find cache (loc, typeparam.reason, op_reason)
+    try Hashtbl.find cache (reason_tapp, typeparam.reason, op_reason)
     with _ ->
       let t = ImplicitTypeArgument.mk_targ cx typeparam (Nel.hd op_reason) reason_tapp in
-      Hashtbl.add cache (loc, typeparam.reason, op_reason) t;
+      Hashtbl.add cache (reason_tapp, typeparam.reason, op_reason) t;
       t
 end
 

src/typing/flow_js.ml

@@ -11869,7 +11869,11 @@ struct
     get_builtin_type cx ?trace reason x
 
   and flow_all_in_union cx trace rep u =
-    UnionRep.members rep |> Base.List.iter ~f:(mk_tuple_swapped u %> rec_flow cx trace)
+    (* This is required so that our caches don't treat different branches of unions as the same type *)
+    let union_reason i r = replace_desc_reason (RUnionBranching (desc_of_reason r, i)) r in
+    UnionRep.members rep
+    |> Base.List.iteri ~f:(fun i ->
+           mod_reason_of_t (union_reason i) %> mk_tuple_swapped u %> rec_flow cx trace)
 
   and call_args_iter f =
     List.iter (function

tests/poly/.flowconfig

@@ -1,3 +1,4 @@
 [options]
 module.system=haste
 all=true
+no_flowlib=false

tests/poly/union.js

@@ -0,0 +1,14 @@
+// @flow
+
+
+type Filter = [number] | [string];
+
+function convert(filter: Filter) {
+    filter.slice(0);
+}
+
+type Filter2 = Array<number> | Array<string>;
+
+function convert(filter: Filter2) : Filter2 {
+    return filter.slice(0);
+}

tests/read_only_object/read_only_object.exp

@@ -78,7 +78,7 @@ Cannot assign `42` to `spreadUnionObj.z` because property `z` is not writable.
 
 Error ----------------------------------------------------------------------------------------------------- test.js:40:4
 
-Cannot get `spreadUnionObj.q` because property `q` is missing in `SpreadUnionObj` [1].
+Cannot get `spreadUnionObj.q` because property `q` is missing in object type [1].
 
    test.js:40:4
    40|   (spreadUnionObj.q: number); // Should error!

tests/rec/rec.exp

@@ -1,25 +1,3 @@
-Error ----------------------------------------------------------------------------------------------- eval_union.js:9:18
-
-Cannot instantiate array type because:
- - Either null or undefined [1] is incompatible with empty [2].
- - Or null or undefined [1] is incompatible with number [3].
-
-   eval_union.js:9:18
-   9| arr.forEach(x => acc.push(x.f));
-                       ^^^
-
-References:
-   eval_union.js:7:30
-   7| declare var arr: Array<{ +f: ?number }>;
-                                   ^^^^^^^ [1]
-   eval_union.js:9:27
-   9| arr.forEach(x => acc.push(x.f));
-                                ^^^ [2]
-   eval_union.js:7:31
-   7| declare var arr: Array<{ +f: ?number }>;
-                                    ^^^^^^ [3]
-
-
 Error ----------------------------------------------------------------------------------------------- issue-4070.js:6:17
 
 Property `@@iterator` is missing in undefined [1] but exists in `$Iterable` [2].
@@ -295,4 +273,4 @@ References:
 
 
 
-Found 24 errors
+Found 23 errors