Skip to content

Commit

Permalink
More Flow linting for unnecessary optional chaining
Browse files Browse the repository at this point in the history
Summary:
The previous diff naively checks if `null` or `undefined` can appear on the LHS to judge if an optional chain is useful. However, consider the following:
```
type Foo = {bar: Bar};
type Bar = {baz: number};
declare var foo: ?Foo;
foo?.bar?.baz;
```
Our previous analysis judges the second `?.` to be useful, but writing `foo?.bar.baz` would be equivalent. Although `foo?.bar` can be `undefined`, it can only be undefined due to the semantics of `?.`, not due to `foo.bar` being `undefined`. Therefore, the second `?.` does not guard against anything beyond what short-circuiting would already catch.

To implement this, we add another flavor of `VoidT` which is produced solely by optional chains. We can thus detect whether a potential `undefined` is a legitimate one we should catch with `?.` or merely one produced by an earlier optional chain which we can handle with short-circuiting.

Reviewed By: avikchaudhuri

Differential Revision: D8122045

fbshipit-source-id: 20cd76f218f8467a16c8d930f68426ef6c581270
  • Loading branch information
Mayank Patke authored and facebook-github-bot committed May 31, 2018
1 parent 3bfe2d4 commit ed80625
Show file tree
Hide file tree
Showing 10 changed files with 110 additions and 56 deletions.
1 change: 1 addition & 0 deletions src/typing/codegen.ml
Original file line number Diff line number Diff line change
Expand Up @@ -310,6 +310,7 @@ let rec gen_type t env = Type.(
| DefT (_, TypeT t) -> gen_type t env
| DefT (_, UnionT union) -> gen_union_list union env
| DefT (_, VoidT) -> add_str "void" env
| InternalT (OptionalChainVoidT _) -> add_str "void" env

(**
* These types can't be expressed in code well so we fail back to `mixed`.
Expand Down
2 changes: 2 additions & 0 deletions src/typing/debug_js.ml
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,7 @@ and _json_of_t_impl json_cx t = Hh_json.(
"type", _json_of_t json_cx t
]

| InternalT (OptionalChainVoidT _) -> []
)
)

Expand Down Expand Up @@ -1793,6 +1794,7 @@ let rec dump_t_ (depth, tvars) cx t =
) (Key_map.elements p_neg))))
| ReposT (_, arg)
| InternalT (ReposUpperT (_, arg)) -> p ~extra:(kid arg) t
| InternalT (OptionalChainVoidT _) -> p t

and dump_use_t_ (depth, tvars) cx t =

Expand Down
3 changes: 2 additions & 1 deletion src/typing/flow_error.ml
Original file line number Diff line number Diff line change
Expand Up @@ -2003,7 +2003,8 @@ let rec error_of_msg ~trace_reasons ~source_file =
| EUnnecessaryOptionalChain (loc, lhs_reason) ->
mk_error ~trace_infos ~kind:(LintError Lints.UnnecessaryOptionalChain) loc [
text "This use of optional chaining ("; code "?."; text ") is unnecessary because ";
ref lhs_reason; text " cannot be null or undefined."
ref lhs_reason; text " cannot be nullish or because an earlier "; code "?.";
text " will short-circuit the nullish case.";
]

| EInexactSpread (reason, reason_op) ->
Expand Down
16 changes: 14 additions & 2 deletions src/typing/flow_js.ml
Original file line number Diff line number Diff line change
Expand Up @@ -1031,6 +1031,8 @@ module ResolvableTypeJob = struct
| InternalT (ReposUpperT (_, t)) ->
collect_of_type ?log_unresolved cx reason acc t

| InternalT (OptionalChainVoidT _) -> acc

| DefT (_, NumT _)
| DefT (_, StrT _)
| DefT (_, BoolT _)
Expand Down Expand Up @@ -2371,7 +2373,11 @@ let rec __flow cx ((l: Type.t), (u: Type.use_t)) trace =

| DefT (r, (NullT | VoidT)), OptionalChainT (r', lhs_reason, chain) ->
Context.mark_optional_chain cx (loc_of_reason r') lhs_reason ~useful:true;
Nel.iter (fun (_, t_out) -> rec_flow_t cx trace (DefT (r, VoidT), t_out)) chain;
Nel.iter (fun (_, t_out) -> rec_flow_t cx trace (InternalT (OptionalChainVoidT r), t_out)) chain;

| InternalT (OptionalChainVoidT _), OptionalChainT (r', lhs_reason, chain) ->
Context.mark_optional_chain cx (loc_of_reason r') lhs_reason ~useful:false;
Nel.iter (fun (_, t_out) -> rec_flow_t cx trace (l, t_out)) chain;

| _, OptionalChainT (r', lhs_reason, chain) when (
match l with
Expand All @@ -2391,6 +2397,9 @@ let rec __flow cx ((l: Type.t), (u: Type.use_t)) trace =
lhs_t := t_out';
) chain;

| InternalT (OptionalChainVoidT r), u ->
rec_flow cx trace (DefT (r, VoidT), u);

(***************)
(* maybe types *)
(***************)
Expand Down Expand Up @@ -7306,6 +7315,8 @@ and check_polarity cx ?trace polarity = function
| ExistsT _
-> ()

| InternalT (OptionalChainVoidT _) -> ()

| DefT (_, OptionalT t)
| ExactT (_, t)
| DefT (_, MaybeT t)
Expand Down Expand Up @@ -11752,7 +11763,8 @@ end = struct
let rec extract_type cx this_t = match this_t with
| DefT (_, MaybeT ty) ->
extract_type cx ty
| DefT (_, (NullT | VoidT)) ->
| DefT (_, (NullT | VoidT))
| InternalT (OptionalChainVoidT _) ->
FailureNullishType
| DefT (_, AnyT) ->
FailureAnyType
Expand Down
1 change: 1 addition & 0 deletions src/typing/ty_normalizer.ml
Original file line number Diff line number Diff line change
Expand Up @@ -1217,6 +1217,7 @@ end = struct
| ExtendsT _
| ReposUpperT _ ->
terr ~kind:BadInternalT (Some t)
| OptionalChainVoidT r -> type__ ~env (DefT (r, VoidT));

and param_bound ~env = function
| T.DefT (_, T.MixedT _) -> return None
Expand Down
4 changes: 4 additions & 0 deletions src/typing/type.ml
Original file line number Diff line number Diff line change
Expand Up @@ -274,6 +274,7 @@ module rec TypeTerm : sig
function *)
| IdxWrapper of reason * t
| ReposUpperT of reason * t
| OptionalChainVoidT of reason

and internal_use_op =
| CopyEnv
Expand Down Expand Up @@ -2070,6 +2071,7 @@ end = struct
| MatchingPropT (reason, _, _) -> reason
| OpaqueT (reason, _) -> reason
| OpenPredT (reason, _, _, _) -> reason
| InternalT (OptionalChainVoidT reason) -> reason
| ReposT (reason, _) -> reason
| InternalT (ReposUpperT (reason, _)) -> reason (* HUH? cf. mod_reason below *)
| ShapeT (t) -> reason_of_t t
Expand Down Expand Up @@ -2218,6 +2220,7 @@ end = struct
| MatchingPropT (reason, k, v) -> MatchingPropT (f reason, k, v)
| OpaqueT (reason, opaquetype) -> OpaqueT (f reason, opaquetype)
| OpenPredT (reason, t, p, n) -> OpenPredT (f reason, t, p, n)
| InternalT (OptionalChainVoidT reason) -> InternalT (OptionalChainVoidT (f reason))
| ReposT (reason, t) -> ReposT (f reason, t)
| InternalT (ReposUpperT (reason, t)) -> InternalT (ReposUpperT (reason, mod_reason_of_t f t))
| ShapeT t -> ShapeT (mod_reason_of_t f t)
Expand Down Expand Up @@ -2911,6 +2914,7 @@ let string_of_ctor = function
| MatchingPropT _ -> "MatchingPropT"
| OpaqueT _ -> "OpaqueT"
| OpenPredT _ -> "OpenPredT"
| InternalT (OptionalChainVoidT _) -> "OptionalChainVoidT"
| ReposT _ -> "ReposT"
| InternalT (ReposUpperT _) -> "ReposUpperT"
| ShapeT _ -> "ShapeT"
Expand Down
1 change: 1 addition & 0 deletions src/typing/type_mapper.ml
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ class ['a] t = object(self)
let t'' = self#type_ cx map_cx t' in
if t'' == t' then t
else InternalT (ReposUpperT (r, t''))
| InternalT (OptionalChainVoidT _) -> t

method tvar _cx _map_cx _r id = id

Expand Down
2 changes: 2 additions & 0 deletions src/typing/type_visitor.ml
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,8 @@ class ['a] t = object(self)
| InternalT (ReposUpperT (_, t)) ->
self#type_ cx pole acc t

| InternalT (OptionalChainVoidT _) -> acc

method def_type cx pole acc = function
| AnyT
| NumT _
Expand Down
Loading

0 comments on commit ed80625

Please sign in to comment.