Add Flow lint for unnecessary optional chaining

Summary:
This diff adds a Flow lint rule to flag unnecessary uses of optional chaining (`?.`) when a normal property access would have sufficed.

To do this, the context maintains a map of optional chain locations which marks them as useful or not. In `flow_js`, whenever we see null/undefined flowing to an optional chain, we mark that optional chain as useful. Whenever we see anything else flowing to an optional chain, we mark it non-useful (combining with `||` so that once an optional chain is marked useful, it stays that way).

At the end of `merge_js`, we simply extract the list of non-useful optional chains and emit a lint for each one.

This approach is limited: consider

```
// flow

class Foo {
  bar: Bar
}

class Bar {
  baz: number
}

declare var foo: ?Foo;
(foo?.bar?.baz: number | void);
```

Note that the `?.` between `bar` and `baz` might as well just be `.` and make use of short-circuiting: after all, it's only `foo` that's nullable, not its `bar` field. However, no lint is produced for this example.

Reviewed By: gabelevi

Differential Revision: D7969730

fbshipit-source-id: 25959203c9751aead30aa38447c2f09e8a726537

src/common/lints/lints.ml

@@ -20,6 +20,7 @@ type lint_kind =
   | DeprecatedType
   | UnsafeGettersSetters
   | InexactSpread
+  | UnnecessaryOptionalChain
 
 let string_of_sketchy_null_kind = function
   | SketchyBool -> "sketchy-null-bool"
@@ -36,6 +37,7 @@ let string_of_kind = function
   | DeprecatedType -> "deprecated-type"
   | UnsafeGettersSetters -> "unsafe-getters-setters"
   | InexactSpread -> "inexact-spread"
+  | UnnecessaryOptionalChain -> "unnecessary-optional-chain"
 
 let kinds_of_string = function
   | "sketchy-null" -> Some [
@@ -55,6 +57,7 @@ let kinds_of_string = function
   | "deprecated-type" -> Some [DeprecatedType]
   | "unsafe-getters-setters" -> Some [UnsafeGettersSetters]
   | "inexact-spread" -> Some [InexactSpread]
+  | "unnecessary-optional-chain" -> Some [UnnecessaryOptionalChain]
   | _ -> None
 
 module LintKind = struct

src/common/lints/lints.mli

@@ -20,6 +20,7 @@ type lint_kind =
  | DeprecatedType
  | UnsafeGettersSetters
  | InexactSpread
+ | UnnecessaryOptionalChain
 
 val string_of_kind: lint_kind -> string
 

src/typing/context.ml

@@ -94,7 +94,9 @@ type sig_t = {
    * it wouldn't be excused. *)
   mutable exists_excuses: ExistsCheck.t LocMap.t;
 
-  mutable test_prop_hits_and_misses: test_prop_hit_or_miss IMap.t
+  mutable test_prop_hits_and_misses: test_prop_hit_or_miss IMap.t;
+
+  mutable optional_chains_useful: (Reason.t * bool) LocMap.t;
 }
 
 type t = {
@@ -176,6 +178,7 @@ let make_sig () = {
   exists_checks = LocMap.empty;
   exists_excuses = LocMap.empty;
   test_prop_hits_and_misses = IMap.empty;
+  optional_chains_useful = LocMap.empty;
 }
 
 (* create a new context structure.
@@ -372,6 +375,7 @@ let clear_intermediates cx =
   cx.sig_cx.exists_checks <- LocMap.empty;
   cx.sig_cx.exists_excuses <- LocMap.empty;
   cx.sig_cx.test_prop_hits_and_misses <- IMap.empty;
+  cx.sig_cx.optional_chains_useful <- LocMap.empty;
   ()
 
 (* Given a sig context, it makes sense to clear the parts that are shared with
@@ -404,6 +408,16 @@ let test_prop_get_never_hit cx =
     | Miss (name, reasons, use_op) -> (name, reasons, use_op)::acc
   ) [] (IMap.bindings cx.sig_cx.test_prop_hits_and_misses)
 
+let mark_optional_chain cx loc lhs_reason ~useful =
+  cx.sig_cx.optional_chains_useful <- LocMap.add loc (lhs_reason, useful) ~combine:(
+    fun (r, u) (_, u') -> (r, u || u')
+  ) cx.sig_cx.optional_chains_useful
+
+let unnecessary_optional_chains cx =
+  LocMap.fold (fun loc (r, useful) acc ->
+    if useful then acc else (loc, r) :: acc
+  ) cx.sig_cx.optional_chains_useful []
+
 (* utils *)
 let iter_props cx id f =
   find_props cx id

src/typing/context.mli

@@ -172,6 +172,9 @@ val test_prop_hit: t -> Constraint.ident -> unit
 val test_prop_miss: t -> Constraint.ident -> string option -> (Reason.t * Reason.t) -> Type.use_op -> unit
 val test_prop_get_never_hit: t -> (string option * (Reason.t * Reason.t) * Type.use_op) list
 
+val mark_optional_chain: t -> Loc.t -> Reason.t -> useful:bool -> unit
+val unnecessary_optional_chains: t -> (Loc.t * Reason.t) list
+
 (* utils *)
 val iter_props: t -> Type.Properties.id -> (string -> Type.Property.t -> unit) -> unit
 val has_prop: t -> Type.Properties.id -> string -> bool

src/typing/debug_js.ml

@@ -788,7 +788,7 @@ and _json_of_use_t_impl json_cx t = Hh_json.(
       "t_out", _json_of_t json_cx t_out
     ]
 
-  | OptionalChainT (_, uses) -> [
+  | OptionalChainT (_, _, uses) -> [
       "chain", JSON_Array (Nel.to_list @@ Nel.map (fun (use, tout) ->
         _json_of_use_t json_cx (apply_opt_use use tout)
       ) uses);
@@ -2769,6 +2769,8 @@ let dump_flow_error =
         spf "EExperimentalOptionalChaining (%s)" (string_of_loc loc)
     | EOptionalChainingMethods loc ->
         spf "EOptionalChainingMethods (%s)" (string_of_loc loc)
+    | EUnnecessaryOptionalChain (loc, _) ->
+        spf "EUnnecessaryOptionalChain (%s)" (string_of_loc loc)
     | EInexactSpread (reason, reason_op) ->
       spf "EInexactSpread (%s, %s)"
           (dump_reason cx reason)

src/typing/flow_error.ml

@@ -162,6 +162,7 @@ type error_message =
   | EInvalidPrototype of reason
   | EExperimentalOptionalChaining of Loc.t
   | EOptionalChainingMethods of Loc.t
+  | EUnnecessaryOptionalChain of Loc.t * reason
   | EInexactSpread of reason * reason
 
 and binding_error =
@@ -381,6 +382,7 @@ let util_use_op_of_msg nope util = function
 | EInvalidPrototype (_)
 | EExperimentalOptionalChaining _
 | EOptionalChainingMethods _
+| EUnnecessaryOptionalChain _
 | EInexactSpread _
   -> nope
 
@@ -1998,6 +2000,12 @@ let rec error_of_msg ~trace_reasons ~source_file =
       text "Flow does not yet support method or property calls in optional chains."
     ]
 
+  | 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."
+    ]
+
   | EInexactSpread (reason, reason_op) ->
     mk_error ~kind:(LintError Lints.InexactSpread) (loc_of_reason reason) [
       text "Cannot determine the type of "; ref reason_op; text " because ";

src/typing/flow_js.ml

@@ -2369,14 +2369,20 @@ let rec __flow cx ((l: Type.t), (u: Type.use_t)) trace =
     (* optional chaining *)
     (*********************)
 
-    | DefT (r, (NullT | VoidT)), OptionalChainT (_, chain) ->
+    | 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;
 
-    | _, OptionalChainT (_, chain) when (
+    | _, OptionalChainT (r', lhs_reason, chain) when (
         match l with
         | DefT (_, (MaybeT _ | OptionalT _ | UnionT _ | IntersectionT _)) -> false
         | _ -> true
       ) ->
+      Context.mark_optional_chain cx (loc_of_reason r') lhs_reason ~useful:(
+        match l with
+        | DefT (_, (MixedT _ | AnyT | AnyObjT | AnyFunT)) -> true
+        | _ -> false
+      );
       let lhs_t = ref l in
       Nel.iter (fun (opt_use, t_out) ->
         let t_out' = Tvar.mk cx (reason_of_t t_out) in

src/typing/merge_js.ml

@@ -172,6 +172,11 @@ let detect_test_prop_misses cx =
     Flow_js.add_output cx (Flow_error.EPropNotFound (name, reasons, use_op))
   ) misses
 
+let detect_unnecessary_optional_chains cx =
+  List.iter (fun (loc, lhs_reason) ->
+    Flow_js.add_output cx (Flow_error.EUnnecessaryOptionalChain (loc, lhs_reason))
+  ) (Context.unnecessary_optional_chains cx)
+
 let apply_docblock_overrides (metadata: Context.metadata) docblock_info =
   let open Context in
 
@@ -343,6 +348,7 @@ let merge_component_strict ~metadata ~lint_severities ~strict_mode ~file_sigs
    *)
   detect_sketchy_null_checks cx;
   detect_test_prop_misses cx;
+  detect_unnecessary_optional_chains cx;
 
   cx, other_cxs