Adding a lint for branches returning the same value

Summary:
This produces a lint advice when all the return statements in a block return the same value. The lint prompts the user to write a single return statement. "Returning the same value" refers to the following:
1) Returning the same literal, i.e same boolean, same integer, same float, same string
2) Returning the same global constant
3) Returning the same enum constant

This lint also ignores the return statements that are within lambda statement.

Reviewed By: Wilfred

Differential Revision: D37937568

fbshipit-source-id: cf7d3643b624787afe9b48102d2d7279e674ec1f

Author: Eesha Shekhar

hphp/hack/src/lints/linter_branches_return_same_value.ml

@@ -0,0 +1,85 @@
+(*
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the "hack" directory of this source tree.
+ *
+ *)
+open Aast
+open Hh_prelude
+
+let is_expr_same ((_, _, expr1) : Tast.expr) ((_, _, expr2) : Tast.expr) : bool
+    =
+  match (expr1, expr2) with
+  | (Aast.True, Aast.True)
+  | (Aast.False, Aast.False) ->
+    true
+  | (Aast.Int s1, Aast.Int s2)
+  | (Aast.Float s1, Aast.Float s2)
+  | (Aast.String s1, Aast.String s2)
+  | (Aast.Id (_, s1), Aast.Id (_, s2))
+    when String.equal s1 s2 ->
+    true
+  | ( Aast.Class_const ((_, _, Aast.CI (_, enum1)), (_, name1)),
+      Aast.Class_const ((_, _, Aast.CI (_, enum2)), (_, name2)) )
+    when String.equal enum1 enum2 && String.equal name1 name2 ->
+    true
+  | _ -> false
+
+let rec are_return_expressions_same (ret_list : Tast.expr list) : bool =
+  match ret_list with
+  | expr1 :: expr2 :: tail ->
+    if is_expr_same expr1 expr2 then
+      are_return_expressions_same (expr2 :: tail)
+    else
+      false
+  | [_] -> true
+  | [] -> true
+
+(* This function returns a list of all the expressions that follow each of the return statements in a block *)
+let get_return_expr_visitor =
+  object
+    inherit [_] Aast.reduce as super
+
+    method zero = []
+
+    method plus = ( @ )
+
+    method! on_stmt env s =
+      match snd s with
+      | Aast.Return (Some expr) -> [expr]
+      | _ -> super#on_stmt env s
+
+    (* we ignore the return expressions within lambda statements *)
+    method! on_expr_ env e =
+      match e with
+      | Aast.Efun _ -> []
+      | Aast.Lfun _ -> []
+      | _ -> super#on_expr_ env e
+  end
+
+let get_return_exprs (stmts : Tast.stmt list) : Tast.expr list =
+  get_return_expr_visitor#on_block () stmts
+
+let handler =
+  object
+    inherit Tast_visitor.handler_base
+
+    method! at_fun_def _env f =
+      let ret_list = get_return_exprs f.fd_fun.f_body.fb_ast in
+      if List.length ret_list > 1 then (
+        if are_return_expressions_same ret_list then
+          List.iter ret_list ~f:(fun (_, pos, _) ->
+              Lints_errors.branches_return_same_value pos)
+      ) else
+        ()
+
+    method! at_method_ _env m =
+      let ret_list = get_return_exprs m.m_body.fb_ast in
+      if List.length ret_list > 1 then (
+        if are_return_expressions_same ret_list then
+          List.iter ret_list ~f:(fun (_, pos, _) ->
+              Lints_errors.branches_return_same_value pos)
+      ) else
+        ()
+  end

hphp/hack/src/lints/linting_main.ml

@@ -39,6 +39,7 @@ let typed_linters =
     Linter_pointless_booleans.handler;
     Linter_comparing_booleans.handler;
     Linter_unconditional_recursion.handler;
+    Linter_branches_return_same_value.handler;
   ]
   @ Linting_service.typed_linters
 

hphp/hack/src/lints/lints_codes.ml

@@ -102,4 +102,6 @@ module Codes = struct
   let pointless_booleans_expression = 5644
 
   let unconditional_recursion = 5645
+
+  let branch_return_same_value = 5646
 end

hphp/hack/src/lints/lints_errors.ml

@@ -441,3 +441,10 @@ let unconditional_recursion p =
     p
     ("This is a recursive function with no base case to terminate. This will result in a Stack Overflow error."
     ^ " Consider adding a base case.")
+
+let branches_return_same_value p =
+  Lints.add
+    Codes.branch_return_same_value
+    Lint_warning
+    p
+    "All of these statements are returning the same value. Consider keeping a single return statement outside of the branches."