Skip to content

Commit

Permalink
[flow] better handing of unions in type guard checks
Browse files Browse the repository at this point in the history
Summary:
This change tries to prevent performance regression in code like the following
```
declare var x: T;
declare var foo: (x: mixed) => x is T
if (foo(x)) {}
```
where T is a large enum-like union.

Before this change, `predicate_no_concretization` on a `LatentP` predicate would do concretize all types, including unions, leading to the breakup of the type of the large union `T`. In `types_differ`, each member of `T` is compared against each member of the type guard type, which also happens to be `T`. This effectively yields a O(n^2) operation which becomes too slow on large enough `T`s.

This diff tries to address this by introducing a layer of faster checks before we get to the slow check above. Specifically,
1. we make the initial concretization preserve enum-like unions (which tend to be large)
2. we use fast comparison of this concretized type against the guard (`try_intersect`)
3. if we don't reach a good result, we proceed to concretize the input again, this time using full concretization on unions.
4. for each part of the newly concretized input, we run the same comparison against the guard (`try_intersect`)

Changelog: [internal]

Reviewed By: SamChou19815

Differential Revision: D69762017

fbshipit-source-id: 39f0af1541f3646c977cdc8860290a2c298fd622
  • Loading branch information
panagosg7 authored and facebook-github-bot committed Feb 19, 2025
1 parent d3c7390 commit 32c180b
Show file tree
Hide file tree
Showing 4 changed files with 66 additions and 10 deletions.
2 changes: 1 addition & 1 deletion src/typing/flow_js.ml
Original file line number Diff line number Diff line change
Expand Up @@ -472,7 +472,7 @@ struct
ConcretizeT
{
reason = _;
kind = ConcretizeForPredicate ConcretizeForMaybeOrExistPredicateTest;
kind = ConcretizeForPredicate ConcretizeKeepOptimizedUnions;
seen = _;
collector;
}
Expand Down
47 changes: 41 additions & 6 deletions src/typing/predicate_kit.ml
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ let concretization_variant_of_predicate = function
| MaybeP
| NotP MaybeP
| TruthyP
| NotP TruthyP ->
ConcretizeForMaybeOrExistPredicateTest
| NotP TruthyP
| LatentP _
| NotP (LatentP _) ->
ConcretizeKeepOptimizedUnions
| _ -> ConcretizeForGeneralPredicateTest

type predicate_result_mut =
Expand Down Expand Up @@ -615,23 +617,56 @@ and intersect =
in
fun cx t1 t2 ->
let reason1 = TypeUtil.reason_of_t t1 in
(* Pre-processing of t1 has concretized it up to optimized unions. It is
* important to keep it this way to prevent expensive checks right away.
* Consider for example the code:
*
* declare var x: T;
* declare var foo: (x: mixed) => x is T
* if (foo(x)) {}
*
* where T is a really large enum-like union. `try_intersect` will quickly
* return `t1` as the result here, without us having to try to fully
* concretize `t1`.
*)
(* Input t1 is already concretized as input to the predicate mechanism *)
match try_intersect cx reason1 (C.wrap_unsafe t1) t2 with
| Some t -> t
| None ->
let r = update_desc_reason invalidate_rtype_alias reason1 in
IntersectionT (r, InterRep.make t2 t1 [])
(* No definitive refinement found. We fall back to more expensive
* concretization that breaks up all unions (including optimized ones). *)
possible_concrete_types_for_inspection cx reason1 t1
|> Base.List.map ~f:(fun t1 ->
(* t1 was just concretized *)
match try_intersect cx reason1 (C.wrap_unsafe t1) t2 with
| Some t -> t
| None ->
let r = update_desc_reason invalidate_rtype_alias reason1 in
IntersectionT (r, InterRep.make t2 t1 [])
)
|> TypeUtil.union_of_ts (update_desc_reason invalidate_rtype_alias reason1)

(* This utility is expected to be used when negating the refinement of a type [t1]
* with a type guard `x is t2`. The only case considered here is that of t1 <: t2.
* This means that the positive branch will always be taken, and so we are left with
* `empty` in the negated case. *)
and type_guard_diff cx t1 t2 =
let reason1 = TypeUtil.reason_of_t t1 in
if TypeUtil.quick_subtype t1 t2 || speculative_subtyping_succeeds cx t1 t2 then
let r = update_desc_reason invalidate_rtype_alias (TypeUtil.reason_of_t t1) in
let r = update_desc_reason invalidate_rtype_alias reason1 in
Type_filter.TypeFilterResult { type_ = DefT (r, EmptyT); changed = true }
else
Type_filter.TypeFilterResult { type_ = t1; changed = false }
let t1s_conc = possible_concrete_types_for_inspection cx reason1 t1 in
let (ts_rev, changed) =
Base.List.fold t1s_conc ~init:([], false) ~f:(fun (acc, changed) t1 ->
if TypeUtil.quick_subtype t1 t2 || speculative_subtyping_succeeds cx t1 t2 then
(acc, changed)
else
(t1 :: acc, true)
)
in
let r1 = update_desc_reason invalidate_rtype_alias reason1 in
Type_filter.TypeFilterResult { type_ = TypeUtil.union_of_ts r1 (List.rev ts_rev); changed }

and prop_exists_test cx key sense obj result_collector =
match has_prop cx (OrdinaryName key) obj with
Expand Down
4 changes: 2 additions & 2 deletions src/typing/type.ml
Original file line number Diff line number Diff line change
Expand Up @@ -1013,7 +1013,7 @@ module rec TypeTerm : sig

and predicate_concretizer_variant =
| ConcretizeForGeneralPredicateTest
| ConcretizeForMaybeOrExistPredicateTest
| ConcretizeKeepOptimizedUnions
| ConcretizeRHSForInstanceOfPredicateTest
| ConcretizeRHSForLiteralPredicateTest

Expand Down Expand Up @@ -4141,7 +4141,7 @@ let string_of_use_op_rec : use_op -> string =

let string_of_predicate_concretizer_variant = function
| ConcretizeForGeneralPredicateTest -> "ConcretizeForGeneralPredicateTest"
| ConcretizeForMaybeOrExistPredicateTest -> "ConcretizeForMaybeOrExistPredicateTest"
| ConcretizeKeepOptimizedUnions -> "ConcretizeKeepOptimizedUnions"
| ConcretizeRHSForInstanceOfPredicateTest -> "ConcretizeRHSForInstanceOfPredicateTest"
| ConcretizeRHSForLiteralPredicateTest -> "ConcretizeRHSForLiteralPredicateTest"

Expand Down
23 changes: 22 additions & 1 deletion tests/type_guards/refinement.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,7 @@ function sentinel_multi_tag() {
}


function negation() {
function negation1() {
declare function isNumber(x: mixed): x is number;
declare function isString(x: mixed): x is string;

Expand Down Expand Up @@ -296,3 +296,24 @@ function getRaccoon(s: string): ?Raccoon {
}
return null;
}

function negation2() {
declare var isA: (x: mixed) => x is 'a';
declare var x: 'a' | 'c';

if (isA(x)) { return; }
x as 'c'; // okay
}

function negation3() {
declare function isFalsey(value: ?mixed): value is null | void | false | '' | 0;

function foo(arr: ?Array<mixed> | $ReadOnlyArray<mixed>): void {
if (isFalsey(arr)) {
return;
}
arr as Array<mixed> | $ReadOnlyArray<mixed>; // okay
}

foo([]); // triggrers union optimization
}

0 comments on commit 32c180b

Please sign in to comment.