Skip to content

Commit 5c8e506

Browse files
Eesha Shekharfacebook-github-bot
Eesha Shekhar
authored and
facebook-github-bot
committed
Adding a lint for Unconditional Recursion: checking the statements that appear before the recursive call
Summary: This produces a lint error of unconditional recursion when there is a top level recursive call and there is no return or throw statement to break out of the recursion before the recursive call. The return or throw statement could be within multiple nested conditionals. The expression following the return/throw statement is checked to ensure that it is anything other than a recursive call. If there are mutliple base cases, then it is considered unconditional recursion if each of the return or throw statements in the base cases are recursive function calls. The function/method may or may not have arguments. The different types of function/method calls detected by linter are: 1) return statements 2) throw statements 3) static methods 4) instance methods 5) Await Reviewed By: Wilfred Differential Revision: D37754411 fbshipit-source-id: c2767d1df54350072b8663de5705d786b7aa5313
1 parent 49cc341 commit 5c8e506

File tree

3 files changed

+287
-81
lines changed

3 files changed

+287
-81
lines changed

‎hphp/hack/src/lints/linter_unconditional_recursion.ml

Lines changed: 100 additions & 60 deletions
Original file line numberDiff line numberDiff line change
@@ -8,86 +8,126 @@
88
open Aast
99
open Hh_prelude
1010

11+
type method_prop = {
12+
method_name: string;
13+
class_name: string;
14+
pos: Pos.t;
15+
}
16+
17+
type fun_prop = {
18+
fun_name: string;
19+
pos: Pos.t;
20+
}
21+
22+
type is_fun_or_method =
23+
| Is_fun of fun_prop
24+
| Is_method of method_prop
25+
1126
let same_name_ignoring_ns (name1 : string) (name2 : string) =
1227
String.equal (Utils.strip_ns name1) (Utils.strip_ns name2)
1328

14-
let check_expr_fun (expr_ : Tast.expr_) (fun_name : string) =
15-
match expr_ with
16-
| Aast.Call ((_, _, Aast.Id (_, name)), _, _, _) ->
17-
same_name_ignoring_ns name fun_name
18-
| _ -> false
19-
20-
let check_expr_method
21-
(expr_ : Tast.expr_) (method_name : string) (c_name : string) =
22-
match expr_ with
23-
| Aast.Call
24-
( ( _,
29+
let check_expr (expr_ : Tast.expr_) (is_fun_or_method : is_fun_or_method) =
30+
match is_fun_or_method with
31+
| Is_fun f_prop ->
32+
(match expr_ with
33+
| Aast.Call ((_, _, Aast.Id (_, name)), _, _, _) ->
34+
same_name_ignoring_ns name f_prop.fun_name
35+
| _ -> false)
36+
| Is_method m_prop ->
37+
(match expr_ with
38+
| Aast.Call
39+
( ( _,
40+
_,
41+
Aast.Obj_get
42+
((_, _, Aast.This), (_, _, Aast.Id (_, name)), _, Aast.Is_method)
43+
),
2544
_,
26-
Aast.Obj_get
27-
((_, _, Aast.This), (_, _, Aast.Id (_, name)), _, Aast.Is_method) ),
28-
_,
29-
_,
30-
_ ) ->
31-
same_name_ignoring_ns name method_name
32-
| Aast.Call
33-
( (_, _, Aast.Class_const ((_, _, Aast.CI (_, name_of_class)), (_, name))),
34-
_,
35-
_,
36-
_ )
37-
when same_name_ignoring_ns name_of_class c_name ->
38-
same_name_ignoring_ns name method_name
39-
| Aast.Call
40-
((_, _, Aast.Class_const ((_, _, Aast.CIstatic), (_, name))), _, _, _) ->
41-
same_name_ignoring_ns name method_name
42-
| Aast.Call
43-
((_, _, Aast.Class_const ((_, _, Aast.CIself), (_, name))), _, _, _) ->
44-
same_name_ignoring_ns name method_name
45-
| _ -> false
45+
_,
46+
_ ) ->
47+
same_name_ignoring_ns name m_prop.method_name
48+
| Aast.Call
49+
( ( _,
50+
_,
51+
Aast.Class_const ((_, _, Aast.CI (_, name_of_class)), (_, name)) ),
52+
_,
53+
_,
54+
_ )
55+
when same_name_ignoring_ns name_of_class m_prop.class_name ->
56+
same_name_ignoring_ns name m_prop.method_name
57+
| Aast.Call
58+
((_, _, Aast.Class_const ((_, _, Aast.CIstatic), (_, name))), _, _, _)
59+
->
60+
same_name_ignoring_ns name m_prop.method_name
61+
| Aast.Call
62+
((_, _, Aast.Class_const ((_, _, Aast.CIself), (_, name))), _, _, _) ->
63+
same_name_ignoring_ns name m_prop.method_name
64+
| _ -> false)
4665

47-
let is_recursion_in_fun ((_, stmt_) : Tast.stmt) (fun_name : string) =
66+
let is_recursive_call
67+
((_, stmt_) : Tast.stmt) (is_fun_or_method : is_fun_or_method) =
4868
match stmt_ with
4969
| Aast.Return (Some (_, _, Aast.Await (_, _, await_exp))) ->
50-
check_expr_fun await_exp fun_name
51-
| Aast.Return (Some (_, _, e)) -> check_expr_fun e fun_name
70+
check_expr await_exp is_fun_or_method
71+
| Aast.Return (Some (_, _, e)) -> check_expr e is_fun_or_method
72+
| Aast.Throw (_ty, _pos, expr_) -> check_expr expr_ is_fun_or_method
5273
| Aast.Expr (_ty, _pos, Aast.Await (_, _, await_exp)) ->
53-
check_expr_fun await_exp fun_name
54-
| Aast.Expr (_ty, _pos, expr_) -> check_expr_fun expr_ fun_name
74+
check_expr await_exp is_fun_or_method
75+
| Aast.Expr (_ty, _pos, expr_) -> check_expr expr_ is_fun_or_method
5576
| _ -> false
5677

57-
let is_recursion_in_method
58-
((_, stmt_) : Tast.stmt) (method_name : string) (c_name : string) =
59-
match stmt_ with
60-
| Aast.Return (Some (_, _, Aast.Await (_, _, await_exp))) ->
61-
check_expr_method await_exp method_name c_name
62-
| Aast.Return (Some (_, _, e)) -> check_expr_method e method_name c_name
63-
| Aast.Expr (_ty, _pos, Aast.Await (_, _, await_exp)) ->
64-
check_expr_method await_exp method_name c_name
65-
| Aast.Expr (_ty, _pos, expr_) -> check_expr_method expr_ method_name c_name
66-
| _ -> false
78+
(* This function returns true if the recursion can be terminated. This occurs when we find at least one return or throw statment that is followed by any expression other than a recursive call*)
79+
let can_terminate_visitor is_fun_or_method =
80+
object
81+
inherit [_] Aast.reduce as super
6782

68-
let check_unconditional_recursion m c_name =
69-
match m.m_body.fb_ast with
70-
| head :: _ ->
71-
if is_recursion_in_method head (snd m.m_name) c_name then
72-
Lints_errors.unconditional_recursion m.m_span
73-
else
83+
method zero = false
84+
85+
method plus = ( || )
86+
87+
method! on_stmt env s =
88+
match snd s with
89+
| Aast.Return _ -> not (is_recursive_call s is_fun_or_method)
90+
| Aast.Throw _ -> not (is_recursive_call s is_fun_or_method)
91+
| _ -> super#on_stmt env s
92+
end
93+
94+
let can_terminate (s : Tast.stmt) (is_fun_or_method : is_fun_or_method) : bool =
95+
(can_terminate_visitor is_fun_or_method)#on_stmt () s
96+
97+
let rec check_unconditional_recursion
98+
(stmt_list : Tast.stmt list) (is_fun_or_method : is_fun_or_method) =
99+
match stmt_list with
100+
| head :: tail ->
101+
if can_terminate head is_fun_or_method then
74102
()
103+
else if is_recursive_call head is_fun_or_method then
104+
match is_fun_or_method with
105+
| Is_fun f_prop -> Lints_errors.unconditional_recursion f_prop.pos
106+
| Is_method m_prop -> Lints_errors.unconditional_recursion m_prop.pos
107+
else
108+
check_unconditional_recursion tail is_fun_or_method
75109
| [] -> ()
76110

77111
let handler =
78112
object
79113
inherit Tast_visitor.handler_base
80114

81115
method! at_fun_def _env f =
82-
match f.fd_fun.f_body.fb_ast with
83-
| head :: _ ->
84-
if is_recursion_in_fun head (snd f.fd_fun.f_name) then
85-
Lints_errors.unconditional_recursion f.fd_fun.f_span
86-
else
87-
()
88-
| [] -> ()
116+
let f_properties : fun_prop =
117+
{ fun_name = snd f.fd_fun.f_name; pos = f.fd_fun.f_span }
118+
in
119+
let is_f_or_m : is_fun_or_method = Is_fun f_properties in
120+
check_unconditional_recursion f.fd_fun.f_body.fb_ast is_f_or_m
89121

90122
method! at_class_ _env c =
91123
List.iter c.c_methods ~f:(fun c_method ->
92-
check_unconditional_recursion c_method (snd c.c_name))
124+
let m_properties : method_prop =
125+
{
126+
method_name = snd c_method.m_name;
127+
class_name = snd c.c_name;
128+
pos = c_method.m_span;
129+
}
130+
in
131+
let is_f_or_m : is_fun_or_method = Is_method m_properties in
132+
check_unconditional_recursion c_method.m_body.fb_ast is_f_or_m)
93133
end

‎hphp/hack/test/lint/recursion_no_base_case.php

Lines changed: 160 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,15 @@
11
<?hh
2+
// @format
23

34
function foo1(): void {
4-
foo1(); // stack overflow, produces lint
5+
foo1(); //should produce lint
56
}
67
function foo2(): void {
7-
foo2(); // stack overflow, produces lint
8+
foo2(); //should produce lint
89
$x = 1;
910
}
1011
function foo3(): void {
11-
$x = 1; //should not produce lint
12+
$x = 1; //should produce lint
1213
foo3();
1314
}
1415
function foo4(int $x): void {
@@ -21,6 +22,102 @@ function foo5(): void {
2122
await foo6(); //should produce lint
2223
}
2324

25+
function test_throw_lint(): Error {
26+
throw test_throw_lint(); // should produce lint
27+
}
28+
29+
//Testing for conditionals
30+
31+
function test_if_return(): int {
32+
$x1 = 2;
33+
if ($x1 == 2) {
34+
return $x1;
35+
}
36+
return test_if_return(); //should not produce lint
37+
}
38+
function test_if_void(): void {
39+
$x1 = 2;
40+
if ($x1 == 2) {
41+
return;
42+
}
43+
test_if_void(); //should not produce lint
44+
}
45+
46+
function test_multiple_base_cases_lint(int $x): int {
47+
if ($x == 1) {
48+
return test_multiple_base_cases_lint($x);
49+
} else if ($x == 2) {
50+
return test_multiple_base_cases_lint(1);
51+
}
52+
return test_multiple_base_cases_lint($x); //should produce lint
53+
}
54+
55+
function test_multiple_base_cases_no_lint(int $x): int {
56+
if ($x == 1) {
57+
return 1;
58+
} else if ($x == 2) {
59+
return test_multiple_base_cases_no_lint(1);
60+
}
61+
return test_multiple_base_cases_no_lint($x); //should not produce lint
62+
}
63+
64+
function test_multiple_base_cases_no_lint_throw(int $x): int {
65+
if ($x == 1) {
66+
throw new Error('error');
67+
} else if ($x == 2) {
68+
return test_multiple_base_cases_no_lint_throw(1);
69+
}
70+
return test_multiple_base_cases_no_lint_throw($x); //should not produce lint
71+
}
72+
73+
function test_multiple_base_cases_lint_throw(int $x): Error {
74+
if ($x == 1) {
75+
throw test_multiple_base_cases_lint_throw($x);
76+
} else if ($x == 2) {
77+
return test_multiple_base_cases_lint_throw(1);
78+
}
79+
return test_multiple_base_cases_lint_throw($x); //should produce lint
80+
}
81+
82+
function test_return_in_else_no_lint(int $x): int {
83+
if ($x == 1) {
84+
return test_return_in_else_no_lint(1);
85+
;
86+
} else if ($x == 2) {
87+
return 2;
88+
}
89+
return test_return_in_else_no_lint($x); //should not produce lint
90+
}
91+
92+
function test_nested_ifs_lint(int $x): int {
93+
if ($x == 1) {
94+
if ($x == 2) {
95+
return test_nested_ifs_lint($x);
96+
}
97+
} else if ($x == 2) {
98+
return test_nested_ifs_lint(1);
99+
}
100+
return test_nested_ifs_lint($x); //should produce lint
101+
}
102+
103+
function test_nested_ifs_no_lint(int $x): int {
104+
if ($x == 1) {
105+
if ($x == 2) {
106+
return $x;
107+
}
108+
} else if ($x == 2) {
109+
return test_nested_ifs_no_lint(1);
110+
}
111+
return test_nested_ifs_no_lint($x); //should not produce lint
112+
}
113+
114+
function test_no_return_no_lint(int $i): void {
115+
if ($i > 0) {
116+
echo "$i";
117+
test_no_return_no_lint($i - 1); //should not produce lint
118+
}
119+
}
120+
24121
class Foo {
25122
public static function bar(): void {
26123
Foo::bar(); //should produce lint
@@ -29,10 +126,10 @@ public static function bar2(): void {
29126
static::bar2(); //should produce lint
30127
}
31128
public function bar4(): void {
32-
$this->bar4(); //should produce lint
129+
$this->bar4(); //should produce lint
33130
}
34131
public static function bar5(int $x): void {
35-
Foo::bar5(123); //should produce lint
132+
Foo::bar5($x); //should produce lint
36133
}
37134
public static function returnBar(): int {
38135
return Foo::returnBar(); //should produce lint
@@ -44,13 +141,68 @@ public static function returnBar2(): int {
44141
public static function returnBar3(): int {
45142
return self::returnBar3(); //should produce lint
46143
}
47-
48144
public function returnBar4(): int {
49-
return $this->returnBar4(); //should produce lint
145+
return $this->returnBar4(); //should produce lint
50146
}
51147

52148
public async function returnAwaitExpr(): Awaitable<void> {
53-
return await $this->returnAwaitExpr(); //should produce lint
149+
return await $this->returnAwaitExpr(); //should produce lint
150+
}
151+
public function returnBar5(): int {
152+
$x1 = 2;
153+
if ($x1 == 2) {
154+
return $x1;
155+
}
156+
return $this->returnBar5(); //should not produce lint
157+
}
158+
public function testMultipleBaseCasesLint(int $x): int {
159+
if ($x == 1) {
160+
return $this->testMultipleBaseCasesLint($x);
161+
} else if ($x == 2) {
162+
return $this->testMultipleBaseCasesLint(1);
163+
}
164+
return $this->testMultipleBaseCasesLint($x); //should produce lint
165+
}
166+
167+
public function testMultipleBaseCasesNoLint(int $x): int {
168+
if ($x == 1) {
169+
return 1;
170+
} else if ($x == 2) {
171+
return $this->testMultipleBaseCasesNoLint(1);
172+
}
173+
return $this->testMultipleBaseCasesNoLint($x); //should not produce lint
174+
}
175+
176+
public function testReturnInElseNoLint(int $x): int {
177+
if ($x == 1) {
178+
return $this->testReturnInElseNoLint(1);
179+
;
180+
} else if ($x == 2) {
181+
return 2;
182+
}
183+
return $this->testReturnInElseNoLint($x); //should not produce lint
184+
}
185+
186+
public function testNestedIfsLint(int $x): int {
187+
if ($x == 1) {
188+
if ($x == 2) {
189+
return $this->testNestedIfsLint($x);
190+
}
191+
} else if ($x == 2) {
192+
return $this->testNestedIfsLint(1);
193+
}
194+
return $this->testNestedIfsLint($x); //should produce lint
195+
}
196+
197+
public function testNestedIfsNoLint(int $x): int {
198+
if ($x == 1) {
199+
if ($x == 2) {
200+
return $x;
201+
}
202+
} else if ($x == 2) {
203+
return $this->testNestedIfsNoLint(1);
204+
}
205+
return $this->testNestedIfsNoLint($x); //should not produce lint
54206
}
55207
}
56208

0 commit comments

Comments
 (0)