Fix sharing of upper bound edges under speculation

Summary:
## The problem

Short-circuiting constraints that have been seen before in different speculation
branches can be unsound. Consider for example the following
```
type X = (x: $ElementType<{name: string}, 'name'>) => void;
declare var foo: X & X;
foo(1);
```
Here the following sequence happens leading to an unsound result:
```
Speculation begins

Branch A:
A1. new tvar v for EvalT
[...]
A2. v ~> ReposUseT
A3. ReposUseT is added to v.upper
[...]
A4. Speculative Error recorded

Branch B:
B1. reuse cached v for EvalT
[...] (same as in case A)
B2. v ~> ReposUseT
B3. immediate success, because ReposUseT is already in v.upper
```
The problem with the above is that constraint `v ~> ReposUseT` was reused
across speculation branches.

## The fix

This diff ensures that we only short-circuit upper-bounds, when the previous
edge was discovered on the same speculation path (same or ancestor branch).

To do this, it adds an optional (speculation id, case id) pair as part of the key to
the map that stores upper bounds of type variables.

This way, in the example above, the edge created in A2, will not be reused in B3,
since the speculation cases differ.

Reviewed By: mvitousek

Differential Revision: D24848916

fbshipit-source-id: 8c800e7ef6ad691dbf18aa1766c746dbf1a0fd5c

Author: panagosg7

src/typing/constraint.ml

@@ -9,6 +9,20 @@ open Type
 
 type ident = int
 
+module UseTypeKey = struct
+  type speculation_id = int
+
+  type case_id = int
+
+  type key = TypeTerm.use_t * (speculation_id * case_id) option
+
+  type t = key
+
+  let compare = Stdlib.compare
+end
+
+module UseTypeMap = WrappedMap.Make (UseTypeKey)
+
 (** Type variables are unknowns, and we are ultimately interested in constraints
     on their solutions for type inference.
 

src/typing/constraint.mli

@@ -7,6 +7,22 @@
 
 type ident = int
 
+module UseTypeKey : sig
+  type speculation_id = int
+
+  type case_id = int
+
+  type key = Type.use_t * (speculation_id * case_id) option
+
+  type t = key
+
+  val compare : key -> key -> int
+end
+
+module UseTypeMap : sig
+  include module type of WrappedMap.Make (UseTypeKey)
+end
+
 (***************************************)
 
 type node =
@@ -25,7 +41,7 @@ and constraints =
 
 and bounds = {
   mutable lower: (Trace.t * Type.use_op) Type.TypeMap.t;
-  mutable upper: Trace.t Type.UseTypeMap.t;
+  mutable upper: Trace.t UseTypeMap.t;
   mutable lowertvars: (Trace.t * Type.use_op) IMap.t;
   mutable uppertvars: (Trace.t * Type.use_op) IMap.t;
 }

src/typing/context.ml

@@ -891,3 +891,9 @@ let fix_cache cx = cx.ccx.fix_cache
 let spread_cache cx = cx.ccx.spread_cache
 
 let speculation_state cx = cx.ccx.speculation_state
+
+let speculation_id cx =
+  let open Speculation_state in
+  match !(speculation_state cx) with
+  | [] -> None
+  | { speculation_id; case = { case_id; _ }; _ } :: _ -> Some (speculation_id, case_id)

src/typing/context.mli

@@ -463,3 +463,5 @@ val fix_cache : t -> (Reason.t * Type.t, Type.t) Hashtbl.t
 val spread_cache : t -> Spread_cache.t
 
 val speculation_state : t -> Speculation_state.t
+
+val speculation_id : t -> (int * int) option

src/typing/debug_js.ml

@@ -914,7 +914,7 @@ and dump_tvar_ (depth, tvars) cx id =
                  "; "
                  (List.rev
                     (UseTypeMap.fold
-                       (fun use_t _ acc -> dump_use_t_ (depth - 1, stack) cx use_t :: acc)
+                       (fun (use_t, _) _ acc -> dump_use_t_ (depth - 1, stack) cx use_t :: acc)
                        upper
                        [])))
       with Context.Tvar_not_found _ -> spf "Not Found: %d" id)

src/typing/flow_js.ml

@@ -172,7 +172,7 @@ let possible_types_of_type cx = function
 
 let uses_of constraints =
   match constraints with
-  | Unresolved { upper; _ } -> UseTypeMap.keys upper
+  | Unresolved { upper; _ } -> Base.List.map ~f:fst (UseTypeMap.keys upper)
   | Resolved (use_op, t)
   | FullyResolved (use_op, t) ->
     [UseT (use_op, t)]
@@ -11033,7 +11033,7 @@ struct
   (* for each u in us: l => u *)
   and flows_from_t cx trace ~new_use_op l us =
     us
-    |> UseTypeMap.iter (fun u trace_u ->
+    |> UseTypeMap.iter (fun (u, _) trace_u ->
            let u = flow_use_op cx new_use_op u in
            join_flow cx [trace; trace_u] (l, u))
 
@@ -11042,12 +11042,13 @@ struct
     ls
     |> TypeMap.iter (fun l (trace_l, use_op') ->
            us
-           |> UseTypeMap.iter (fun u trace_u ->
+           |> UseTypeMap.iter (fun (u, _) trace_u ->
                   let u = flow_use_op cx use_op' (flow_use_op cx use_op u) in
                   join_flow cx [trace_l; trace; trace_u] (l, u)))
 
   (* bounds.upper += u *)
-  and add_upper u trace bounds = bounds.upper <- UseTypeMap.add u trace bounds.upper
+  and add_upper cx u trace bounds =
+    bounds.upper <- UseTypeMap.add (u, Context.speculation_id cx) trace bounds.upper
 
   (* bounds.lower += l *)
   and add_lower l (trace, use_op) bounds =
@@ -11076,10 +11077,10 @@ struct
       goto node (so that updating its bounds is unnecessary). **)
   and edges_to_t cx trace ?(opt = false) (id1, bounds1) t2 =
     let max = Context.max_trace_depth cx in
-    if not opt then add_upper t2 trace bounds1;
+    if not opt then add_upper cx t2 trace bounds1;
     iter_with_filter cx bounds1.lowertvars id1 (fun (_, bounds) (trace_l, use_op) ->
         let t2 = flow_use_op cx use_op t2 in
-        add_upper t2 (Trace.concat_trace ~max [trace_l; trace]) bounds)
+        add_upper cx t2 (Trace.concat_trace ~max [trace_l; trace]) bounds)
     (* for each id in id2 + bounds2.uppertvars:
        id.bounds.lower += t1
     *)
@@ -11095,17 +11096,17 @@ struct
         add_lower t1 (Trace.concat_trace ~max [trace; trace_u], use_op) bounds)
 
   (* for each id' in id + bounds.lowertvars:
-     id'.bounds.upper += us
+       id'.bounds.upper += us
   *)
   and edges_to_ts ~new_use_op cx trace ?(opt = false) (id, bounds) us =
     let max = Context.max_trace_depth cx in
     us
-    |> UseTypeMap.iter (fun u trace_u ->
+    |> UseTypeMap.iter (fun (u, _) trace_u ->
            let u = flow_use_op cx new_use_op u in
            edges_to_t cx (Trace.concat_trace ~max [trace; trace_u]) ~opt (id, bounds) u)
 
   (* for each id' in id + bounds.uppertvars:
-     id'.bounds.lower += ls
+       id'.bounds.lower += ls
   *)
   and edges_from_ts cx trace ~new_use_op ?(opt = false) ls (id, bounds) =
     let max = Context.max_trace_depth cx in
@@ -11114,20 +11115,29 @@ struct
            let new_use_op = pick_use_op cx use_op new_use_op in
            edges_from_t cx (Trace.concat_trace ~max [trace_l; trace]) ~new_use_op ~opt l (id, bounds))
     (* for each id in id1 + bounds1.lowertvars:
-       id.bounds.upper += t2
-       for each l in bounds1.lower: l => t2
+         id.bounds.upper += t2
+         for each l in bounds1.lower: l => t2
     *)
 
   (** As an invariant, bounds1.lower should already contain id.bounds.lower for
       each id in bounds1.lowertvars. **)
   and edges_and_flows_to_t cx trace ?(opt = false) (id1, bounds1) t2 =
-    if not (UseTypeMap.mem t2 bounds1.upper) then (
+    (* Skip iff edge exists as part of the speculation path to the current branch *)
+    let skip =
+      List.exists
+        (fun branch ->
+          let Speculation_state.{ speculation_id; case = { case_id; _ }; _ } = branch in
+          UseTypeMap.mem (t2, Some (speculation_id, case_id)) bounds1.upper)
+        !(Context.speculation_state cx)
+      || UseTypeMap.mem (t2, None) bounds1.upper
+    in
+    if not skip then (
       edges_to_t cx trace ~opt (id1, bounds1) t2;
       flows_to_t cx trace bounds1.lower t2
     )
     (* for each id in id2 + bounds2.uppertvars:
-       id.bounds.lower += t1
-       for each u in bounds2.upper: t1 => u
+         id.bounds.lower += t1
+         for each u in bounds2.upper: t1 => u
     *)
 
   (** As an invariant, bounds2.upper should already contain id.bounds.upper for
@@ -11173,9 +11183,9 @@ struct
         add_lowertvar id1 (Trace.concat_trace ~max [trace; trace_u]) use_op bounds)
 
   (* for each id in id1 + bounds1.lowertvars:
-     id.bounds.upper += bounds2.upper
-     id.bounds.uppertvars += id2
-     id.bounds.uppertvars += bounds2.uppertvars
+       id.bounds.upper += bounds2.upper
+       id.bounds.uppertvars += id2
+       id.bounds.uppertvars += bounds2.uppertvars
   *)
   and add_upper_edges ~new_use_op cx trace ?(opt = false) (id1, bounds1) (id2, bounds2) =
     let max = Context.max_trace_depth cx in
@@ -11187,9 +11197,9 @@ struct
         edges_to_tvar cx trace ~new_use_op ~opt (id1, bounds1) tvar)
 
   (* for each id in id2 + bounds2.uppertvars:
-     id.bounds.lower += bounds1.lower
-     id.bounds.lowertvars += id1
-     id.bounds.lowertvars += bounds1.lowertvars
+       id.bounds.lower += bounds1.lower
+       id.bounds.lowertvars += id1
+       id.bounds.lowertvars += bounds1.lowertvars
   *)
   and add_lower_edges cx trace ~new_use_op ?(opt = false) (id1, bounds1) (id2, bounds2) =
     let max = Context.max_trace_depth cx in

src/typing/ty_normalizer.ml

@@ -953,7 +953,7 @@ end = struct
       >>| Base.List.dedup_and_sort ~compare:Stdlib.compare
 
     and empty_with_upper_bounds ~env bounds =
-      let uses = T.UseTypeMap.keys bounds.Constraint.upper in
+      let uses = Base.List.map ~f:fst (Constraint.UseTypeMap.keys bounds.Constraint.upper) in
       let%map use_kind = uses_t ~env uses in
       Ty.Bot (Ty.NoLowerWithUpper use_kind)
 

src/typing/type.ml

@@ -2308,14 +2308,6 @@ and UseTypeSet : (Set.S with type elt = TypeTerm.use_t) = Set.Make (struct
   let compare = Stdlib.compare
 end)
 
-and UseTypeMap : (WrappedMap.S with type key = TypeTerm.use_t) = WrappedMap.Make (struct
-  type key = TypeTerm.use_t
-
-  type t = key
-
-  let compare = Stdlib.compare
-end)
-
 and Object : sig
   type resolve_tool =
     (* Each part of a spread must be resolved in order to compute the result *)

src/typing/type_mapper.ml

@@ -701,7 +701,17 @@ class virtual ['a] t =
       Constraint.(
         let lower' = TypeMap.ident_map_key (self#type_ cx map_cx) t.lower in
         if lower' != t.lower then t.lower <- lower';
-        let upper' = UseTypeMap.ident_map_key (self#use_type cx map_cx) t.upper in
+        let upper' =
+          UseTypeMap.ident_map_key
+            (fun u ->
+              let (t, speculation) = u in
+              let t' = self#use_type cx map_cx t in
+              if t == t' then
+                u
+              else
+                (t', speculation))
+            t.upper
+        in
         if upper' != t.upper then t.upper <- upper';
         t)
 

tests/error_printing/error_printing.exp

@@ -1,75 +1,3 @@
-Error ----------------------------------------------------------------------------------------------- positining.js:28:2
-
-Cannot cast `x` to `$RelayProps` because property `$fragmentRefs` is missing in object type [1] but exists in
-`$FragmentRef` [2] in property `selectedValue`. [prop-missing]
-
-   positining.js:28:2
-   28| (x: $RelayProps<Props>);
-        ^
-
-References:
-   positining.js:23:18
-   23|   selectedValue: X,
-                        ^ [1]
-   positining.js:19:49
-   19|     (<TRef: 3, T: {+$refType: TRef, ...}>(T) => $FragmentRef<T>),
-                                                       ^^^^^^^^^^^^^^^ [2]
-
-
-Error ----------------------------------------------------------------------------------------------- positining.js:28:2
-
-Cannot cast `x` to `$RelayProps` because property `$refType` is missing in `$FragmentRef` [1] but exists in object
-type [2] in property `selectedValue`. [prop-missing]
-
-   positining.js:28:2
-   28| (x: $RelayProps<Props>);
-        ^
-
-References:
-   positining.js:19:49
-   19|     (<TRef: 3, T: {+$refType: TRef, ...}>(T) => $FragmentRef<T>),
-                                                       ^^^^^^^^^^^^^^^ [1]
-   positining.js:23:18
-   23|   selectedValue: X,
-                        ^ [2]
-
-
-Error ----------------------------------------------------------------------------------------------- positining.js:28:2
-
-Cannot cast `x` to `$RelayProps` because number literal `3` [1] is incompatible with empty [2] in the first argument.
-[incompatible-type]
-
-   positining.js:28:2
-   28| (x: $RelayProps<Props>);
-        ^
-
-References:
-   positining.js:12:14
-   12|   +$refType: 3,
-                    ^ [1]
-   positining.js:17:20
-   17|   (<T: {+$refType: empty, ...}>(T) => T) &
-                          ^^^^^ [2]
-
-
-Error ----------------------------------------------------------------------------------------------- positining.js:28:2
-
-Cannot cast `x` to `$RelayProps` because number literal `3` [1] is incompatible with empty [2] in type argument `T` of
-the first argument. [incompatible-type]
-
-   positining.js:28:2
-   28| (x: $RelayProps<Props>);
-        ^
-
-References:
-   positining.js:12:14
-   12|   +$refType: 3,
-                    ^ [1]
-   positining.js:18:22
-   18|     (<T: {+$refType: empty, ...}>(?T) => ?T) &
-                            ^^^^^ [2]
-
-
 Error ----------------------------------------------------------------------------------------------------- test.js:18:6
 
 Cannot create `Foo` element because number [1] is incompatible with boolean [2] in property `a`. [incompatible-type]
@@ -85,7 +13,4 @@ References:
 
 
 
-Found 5 errors
-
-Only showing the most relevant union/intersection branches.
-To see all branches, re-run Flow with --show-all-branches
+Found 1 error