From fb927dab600f92a901011c128add0b850e3180e3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20D=C3=A4schle?= Date: Thu, 9 Jul 2020 22:38:43 +0200 Subject: [PATCH] checker: check all branches for return (#5763) --- vlib/v/checker/checker.v | 82 +++++++++++++++++-- .../checker/tests/return_missing_comp_if.out | 7 ++ .../v/checker/tests/return_missing_comp_if.vv | 9 ++ .../tests/return_missing_comp_if_nested.out | 7 ++ .../tests/return_missing_comp_if_nested.vv | 17 ++++ .../checker/tests/return_missing_if_match.out | 7 ++ .../checker/tests/return_missing_if_match.vv | 13 +++ .../checker/tests/return_missing_match_if.out | 7 ++ .../checker/tests/return_missing_match_if.vv | 15 ++++ .../v/checker/tests/return_missing_nested.out | 7 ++ vlib/v/checker/tests/return_missing_nested.vv | 11 +++ .../v/checker/tests/return_missing_simple.out | 7 ++ vlib/v/checker/tests/return_missing_simple.vv | 7 ++ .../checker/tests/return_working_comp_if.out | 0 .../v/checker/tests/return_working_comp_if.vv | 9 ++ .../tests/return_working_comp_if_nested.out | 0 .../tests/return_working_comp_if_nested.vv | 17 ++++ .../checker/tests/return_working_if_match.out | 0 .../checker/tests/return_working_if_match.vv | 13 +++ .../checker/tests/return_working_match_if.out | 0 .../checker/tests/return_working_match_if.vv | 15 ++++ .../v/checker/tests/return_working_nested.out | 0 vlib/v/checker/tests/return_working_nested.vv | 12 +++ .../v/checker/tests/return_working_simple.out | 0 vlib/v/checker/tests/return_working_simple.vv | 9 ++ .../v/checker/tests/return_working_two_if.out | 0 vlib/v/checker/tests/return_working_two_if.vv | 17 ++++ .../v/checker/tests/return_working_unsafe.out | 0 vlib/v/checker/tests/return_working_unsafe.vv | 7 ++ vlib/v/parser/parser.v | 2 + 30 files changed, 292 insertions(+), 5 deletions(-) create mode 100644 vlib/v/checker/tests/return_missing_comp_if.out create mode 100644 vlib/v/checker/tests/return_missing_comp_if.vv create mode 100644 vlib/v/checker/tests/return_missing_comp_if_nested.out create mode 100644 vlib/v/checker/tests/return_missing_comp_if_nested.vv create mode 100644 vlib/v/checker/tests/return_missing_if_match.out create mode 100644 vlib/v/checker/tests/return_missing_if_match.vv create mode 100644 vlib/v/checker/tests/return_missing_match_if.out create mode 100644 vlib/v/checker/tests/return_missing_match_if.vv create mode 100644 vlib/v/checker/tests/return_missing_nested.out create mode 100644 vlib/v/checker/tests/return_missing_nested.vv create mode 100644 vlib/v/checker/tests/return_missing_simple.out create mode 100644 vlib/v/checker/tests/return_missing_simple.vv create mode 100644 vlib/v/checker/tests/return_working_comp_if.out create mode 100644 vlib/v/checker/tests/return_working_comp_if.vv create mode 100644 vlib/v/checker/tests/return_working_comp_if_nested.out create mode 100644 vlib/v/checker/tests/return_working_comp_if_nested.vv create mode 100644 vlib/v/checker/tests/return_working_if_match.out create mode 100644 vlib/v/checker/tests/return_working_if_match.vv create mode 100644 vlib/v/checker/tests/return_working_match_if.out create mode 100644 vlib/v/checker/tests/return_working_match_if.vv create mode 100644 vlib/v/checker/tests/return_working_nested.out create mode 100644 vlib/v/checker/tests/return_working_nested.vv create mode 100644 vlib/v/checker/tests/return_working_simple.out create mode 100644 vlib/v/checker/tests/return_working_simple.vv create mode 100644 vlib/v/checker/tests/return_working_two_if.out create mode 100644 vlib/v/checker/tests/return_working_two_if.vv create mode 100644 vlib/v/checker/tests/return_working_unsafe.out create mode 100644 vlib/v/checker/tests/return_working_unsafe.vv diff --git a/vlib/v/checker/checker.v b/vlib/v/checker/checker.v index 8486ef4ff1..54ecac93ab 100644 --- a/vlib/v/checker/checker.v +++ b/vlib/v/checker/checker.v @@ -964,9 +964,6 @@ pub fn (mut c Checker) call_method(mut call_expr ast.CallExpr) table.Type { } pub fn (mut c Checker) call_fn(mut call_expr ast.CallExpr) table.Type { - if call_expr.name == 'panic' { - c.returns = true - } fn_name := call_expr.name if fn_name == 'main' { c.error('the `main` function cannot be called in the program', call_expr.pos) @@ -1843,6 +1840,11 @@ fn (mut c Checker) stmt(node ast.Stmt) { if node.has_else { c.stmts(node.else_stmts) } + mut stmts := node.stmts.clone() + stmts << node.else_stmts + if has_return := c.has_return(stmts) { + c.returns = has_return + } } ast.ConstDecl { mut field_names := []string{} @@ -2006,7 +2008,7 @@ fn (mut c Checker) stmt(node ast.Stmt) { c.check_valid_snake_case(node.name, 'module name', node.pos) } ast.Return { - c.returns = true + // c.returns = true c.return_stmt(mut node) c.scope_returns = true } @@ -2466,6 +2468,8 @@ pub fn (mut c Checker) match_expr(mut node ast.MatchExpr) table.Type { c.match_exprs(mut node, cond_type_sym) c.expected_type = cond_type mut ret_type := table.void_type + mut require_return := false + mut branch_without_return := false for branch in node.branches { for expr in branch.exprs { c.expected_type = cond_type @@ -2502,6 +2506,19 @@ pub fn (mut c Checker) match_expr(mut node ast.MatchExpr) table.Type { } } } + if has_return := c.has_return(branch.stmts) { + if has_return { + require_return = true + } else { + branch_without_return = true + } + } + } + if require_return && branch_without_return { + c.returns = false + } else { + // if inner if branch has not covered all branches but this one + c.returns = true } // if ret_type != table.void_type { // node.is_expr = c.expected_type != table.void_type @@ -2602,6 +2619,8 @@ pub fn (mut c Checker) if_expr(mut node ast.IfExpr) table.Type { } former_expected_type := c.expected_type node.typ = table.void_type + mut require_return := false + mut branch_without_return := false for i, branch in node.branches { if branch.cond is ast.ParExpr { c.error('unnecessary `()` in an if condition. use `if expr {` instead of `if (expr) {`.', @@ -2694,6 +2713,19 @@ pub fn (mut c Checker) if_expr(mut node ast.IfExpr) table.Type { branch.pos) } } + if has_return := c.has_return(branch.stmts) { + if has_return { + require_return = true + } else { + branch_without_return = true + } + } + } + if require_return && (!node.has_else || branch_without_return) { + c.returns = false + } else { + // if inner if branch has not covered all branches but this one + c.returns = true } // if only untyped literals were given default to int/f64 if node.typ == table.any_int_type { @@ -2710,6 +2742,19 @@ pub fn (mut c Checker) if_expr(mut node ast.IfExpr) table.Type { return table.bool_type } +fn (c Checker) has_return(stmts []ast.Stmt) ?bool { + // complexity means either more match or ifs + exprs := stmts.filter(it is ast.ExprStmt).map(it as ast.ExprStmt) + contains_comp_if := stmts.filter(it is ast.CompIf).len > 0 + contains_if_match := exprs.filter(it.expr is ast.IfExpr || it.expr is ast.MatchExpr).len > 0 + contains_complexity := contains_comp_if || contains_if_match + // if the inner complexity covers all paths with returns there is no need for further checks + if !contains_complexity || !c.returns { + return has_top_return(stmts) + } + return none +} + pub fn (mut c Checker) postfix_expr(node ast.PostfixExpr) table.Type { typ := c.expr(node.expr) typ_sym := c.table.get_type_symbol(typ) @@ -2999,6 +3044,7 @@ fn (c &Checker) fetch_and_verify_orm_fields(info table.Struct, pos token.Positio } fn (mut c Checker) fn_decl(mut node ast.FnDecl) { + c.returns = true if node.is_generic && c.cur_generic_type == 0 { // need the cur_generic_type check to avoid inf. recursion // loop thru each generic type and generate a function for gen_type in c.table.fn_gen_types[node.name] { @@ -3075,10 +3121,36 @@ fn (mut c Checker) fn_decl(mut node ast.FnDecl) { } c.stmts(node.stmts) + returns := c.returns || has_top_return(node.stmts) if node.language == .v && !node.no_body && - node.return_type != table.void_type && !c.returns && + node.return_type != table.void_type && !returns && node.name !in ['panic', 'exit'] { c.error('missing return at end of function `$node.name`', node.pos) } c.returns = false } + +fn has_top_return(stmts []ast.Stmt) bool { + if stmts.filter(it is ast.Return).len > 0 { + return true + } + mut has_unsafe_return := false + for _, stmt in stmts { + if stmt is ast.UnsafeStmt { + for ustmt in stmt.stmts { + if ustmt is ast.Return { + has_unsafe_return = true + } + } + } + } + if has_unsafe_return { + return true + } + exprs := stmts.filter(it is ast.ExprStmt).map(it as ast.ExprStmt) + has_panic_exit := exprs.filter(it.expr is ast.CallExpr).map(it.expr as ast.CallExpr).filter(it.name == 'panic' || it.name == 'exit').len > 0 + if has_panic_exit { + return true + } + return false +} diff --git a/vlib/v/checker/tests/return_missing_comp_if.out b/vlib/v/checker/tests/return_missing_comp_if.out new file mode 100644 index 0000000000..e1005d0850 --- /dev/null +++ b/vlib/v/checker/tests/return_missing_comp_if.out @@ -0,0 +1,7 @@ +vlib/v/checker/tests/return_missing_comp_if.v:3:1: error: missing return at end of function `foo` + 1 | fn main() {} + 2 | + 3 | fn foo() string { + | ~~~~~~~~~~~~~~~ + 4 | $if windows { + 5 | \ No newline at end of file diff --git a/vlib/v/checker/tests/return_missing_comp_if.vv b/vlib/v/checker/tests/return_missing_comp_if.vv new file mode 100644 index 0000000000..e6e6d79178 --- /dev/null +++ b/vlib/v/checker/tests/return_missing_comp_if.vv @@ -0,0 +1,9 @@ +fn main() {} + +fn foo() string { + $if windows { + + } $else { + + } +} diff --git a/vlib/v/checker/tests/return_missing_comp_if_nested.out b/vlib/v/checker/tests/return_missing_comp_if_nested.out new file mode 100644 index 0000000000..329ba665bf --- /dev/null +++ b/vlib/v/checker/tests/return_missing_comp_if_nested.out @@ -0,0 +1,7 @@ +vlib/v/checker/tests/return_missing_comp_if_nested.v:3:1: error: missing return at end of function `foo` + 1 | fn main() {} + 2 | + 3 | fn foo() string { + | ~~~~~~~~~~~~~~~ + 4 | $if windows { + 5 | if true { \ No newline at end of file diff --git a/vlib/v/checker/tests/return_missing_comp_if_nested.vv b/vlib/v/checker/tests/return_missing_comp_if_nested.vv new file mode 100644 index 0000000000..08fada9e8f --- /dev/null +++ b/vlib/v/checker/tests/return_missing_comp_if_nested.vv @@ -0,0 +1,17 @@ +fn main() {} + +fn foo() string { + $if windows { + if true { + + } else { + return '' + } + } $else { + if true { + + } else { + return '' + } + } +} diff --git a/vlib/v/checker/tests/return_missing_if_match.out b/vlib/v/checker/tests/return_missing_if_match.out new file mode 100644 index 0000000000..a4bd1ab9d3 --- /dev/null +++ b/vlib/v/checker/tests/return_missing_if_match.out @@ -0,0 +1,7 @@ +vlib/v/checker/tests/return_missing_if_match.v:3:1: error: missing return at end of function `foo` + 1 | fn main() {} + 2 | + 3 | fn foo() string { + | ~~~~~~~~~~~~~~~ + 4 | if true { + 5 | match 1 { \ No newline at end of file diff --git a/vlib/v/checker/tests/return_missing_if_match.vv b/vlib/v/checker/tests/return_missing_if_match.vv new file mode 100644 index 0000000000..88ab70c0a7 --- /dev/null +++ b/vlib/v/checker/tests/return_missing_if_match.vv @@ -0,0 +1,13 @@ +fn main() {} + +fn foo() string { + if true { + match 1 { + 1 { return '' } + 2 { } + else { return '' } + } + } else { + return '' + } +} diff --git a/vlib/v/checker/tests/return_missing_match_if.out b/vlib/v/checker/tests/return_missing_match_if.out new file mode 100644 index 0000000000..ab92b4883b --- /dev/null +++ b/vlib/v/checker/tests/return_missing_match_if.out @@ -0,0 +1,7 @@ +vlib/v/checker/tests/return_missing_match_if.v:3:1: error: missing return at end of function `foo` + 1 | fn main() {} + 2 | + 3 | fn foo() string { + | ~~~~~~~~~~~~~~~ + 4 | match 1 { + 5 | 1 { return '' } \ No newline at end of file diff --git a/vlib/v/checker/tests/return_missing_match_if.vv b/vlib/v/checker/tests/return_missing_match_if.vv new file mode 100644 index 0000000000..4ff9345c70 --- /dev/null +++ b/vlib/v/checker/tests/return_missing_match_if.vv @@ -0,0 +1,15 @@ +fn main() {} + +fn foo() string { + match 1 { + 1 { return '' } + 2 { + if true { + + } else { + return '' + } + } + else { return '' } + } +} diff --git a/vlib/v/checker/tests/return_missing_nested.out b/vlib/v/checker/tests/return_missing_nested.out new file mode 100644 index 0000000000..dd9544977d --- /dev/null +++ b/vlib/v/checker/tests/return_missing_nested.out @@ -0,0 +1,7 @@ +vlib/v/checker/tests/return_missing_nested.v:3:1: error: missing return at end of function `foo` + 1 | fn main() {} + 2 | + 3 | fn foo() string { + | ~~~~~~~~~~~~~~~ + 4 | if true { + 5 | return '' \ No newline at end of file diff --git a/vlib/v/checker/tests/return_missing_nested.vv b/vlib/v/checker/tests/return_missing_nested.vv new file mode 100644 index 0000000000..41b4c7fc1c --- /dev/null +++ b/vlib/v/checker/tests/return_missing_nested.vv @@ -0,0 +1,11 @@ +fn main() {} + +fn foo() string { + if true { + return '' + } else { + if true { + return '' + } + } +} diff --git a/vlib/v/checker/tests/return_missing_simple.out b/vlib/v/checker/tests/return_missing_simple.out new file mode 100644 index 0000000000..2e7a4eda93 --- /dev/null +++ b/vlib/v/checker/tests/return_missing_simple.out @@ -0,0 +1,7 @@ +vlib/v/checker/tests/return_missing_simple.v:3:1: error: missing return at end of function `foo` + 1 | fn main() {} + 2 | + 3 | fn foo() string { + | ~~~~~~~~~~~~~~~ + 4 | if true { + 5 | return '' \ No newline at end of file diff --git a/vlib/v/checker/tests/return_missing_simple.vv b/vlib/v/checker/tests/return_missing_simple.vv new file mode 100644 index 0000000000..ed200feddf --- /dev/null +++ b/vlib/v/checker/tests/return_missing_simple.vv @@ -0,0 +1,7 @@ +fn main() {} + +fn foo() string { + if true { + return '' + } else {} +} diff --git a/vlib/v/checker/tests/return_working_comp_if.out b/vlib/v/checker/tests/return_working_comp_if.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/vlib/v/checker/tests/return_working_comp_if.vv b/vlib/v/checker/tests/return_working_comp_if.vv new file mode 100644 index 0000000000..d6fffe5915 --- /dev/null +++ b/vlib/v/checker/tests/return_working_comp_if.vv @@ -0,0 +1,9 @@ +fn main() {} + +fn foo() string { + $if windows { + return '' + } $else { + return '' + } +} diff --git a/vlib/v/checker/tests/return_working_comp_if_nested.out b/vlib/v/checker/tests/return_working_comp_if_nested.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/vlib/v/checker/tests/return_working_comp_if_nested.vv b/vlib/v/checker/tests/return_working_comp_if_nested.vv new file mode 100644 index 0000000000..c76e981e36 --- /dev/null +++ b/vlib/v/checker/tests/return_working_comp_if_nested.vv @@ -0,0 +1,17 @@ +fn main() {} + +fn foo() string { + $if windows { + if true { + return '' + } else { + return '' + } + } $else { + if true { + return '' + } else { + return '' + } + } +} diff --git a/vlib/v/checker/tests/return_working_if_match.out b/vlib/v/checker/tests/return_working_if_match.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/vlib/v/checker/tests/return_working_if_match.vv b/vlib/v/checker/tests/return_working_if_match.vv new file mode 100644 index 0000000000..5e8d1bf9c8 --- /dev/null +++ b/vlib/v/checker/tests/return_working_if_match.vv @@ -0,0 +1,13 @@ +fn main() {} + +fn foo() string { + if true { + match 1 { + 1 { return '' } + 2 { return '' } + else { return '' } + } + } else { + return '' + } +} diff --git a/vlib/v/checker/tests/return_working_match_if.out b/vlib/v/checker/tests/return_working_match_if.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/vlib/v/checker/tests/return_working_match_if.vv b/vlib/v/checker/tests/return_working_match_if.vv new file mode 100644 index 0000000000..2fbbcdf2b8 --- /dev/null +++ b/vlib/v/checker/tests/return_working_match_if.vv @@ -0,0 +1,15 @@ +fn main() {} + +fn foo() string { + match 1 { + 1 { return '' } + 2 { + if true { + return '' + } else { + return '' + } + } + else { return '' } + } +} diff --git a/vlib/v/checker/tests/return_working_nested.out b/vlib/v/checker/tests/return_working_nested.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/vlib/v/checker/tests/return_working_nested.vv b/vlib/v/checker/tests/return_working_nested.vv new file mode 100644 index 0000000000..1d72a020fd --- /dev/null +++ b/vlib/v/checker/tests/return_working_nested.vv @@ -0,0 +1,12 @@ +fn main() {} + +fn foo() string { + if true { + return '' + } else { + if true { + return '' + } + return '' + } +} diff --git a/vlib/v/checker/tests/return_working_simple.out b/vlib/v/checker/tests/return_working_simple.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/vlib/v/checker/tests/return_working_simple.vv b/vlib/v/checker/tests/return_working_simple.vv new file mode 100644 index 0000000000..271e3f6254 --- /dev/null +++ b/vlib/v/checker/tests/return_working_simple.vv @@ -0,0 +1,9 @@ +fn main() {} + +fn foo() string { + if true { + return '' + } else { + return '' + } +} diff --git a/vlib/v/checker/tests/return_working_two_if.out b/vlib/v/checker/tests/return_working_two_if.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/vlib/v/checker/tests/return_working_two_if.vv b/vlib/v/checker/tests/return_working_two_if.vv new file mode 100644 index 0000000000..a95825fd41 --- /dev/null +++ b/vlib/v/checker/tests/return_working_two_if.vv @@ -0,0 +1,17 @@ +fn main() {} + +fn foo() string { + if true { + if true { + return '' + } + + if true { + return '' + } else { + return '' + } + } else { + return '' + } +} diff --git a/vlib/v/checker/tests/return_working_unsafe.out b/vlib/v/checker/tests/return_working_unsafe.out new file mode 100644 index 0000000000..e69de29bb2 diff --git a/vlib/v/checker/tests/return_working_unsafe.vv b/vlib/v/checker/tests/return_working_unsafe.vv new file mode 100644 index 0000000000..777b297818 --- /dev/null +++ b/vlib/v/checker/tests/return_working_unsafe.vv @@ -0,0 +1,7 @@ +fn main() {} + +fn foo() string { + unsafe { + return '' + } +} diff --git a/vlib/v/parser/parser.v b/vlib/v/parser/parser.v index 62790ca2cc..c30fe1e745 100644 --- a/vlib/v/parser/parser.v +++ b/vlib/v/parser/parser.v @@ -551,6 +551,7 @@ pub fn (mut p Parser) stmt(is_top_level bool) ast.Stmt { expr: p.vweb() } } + return ast.Stmt{} } .key_continue, .key_break { tok := p.tok @@ -601,6 +602,7 @@ pub fn (mut p Parser) stmt(is_top_level bool) ast.Stmt { .key_const { p.error_with_pos('const can only be defined at the top level (outside of functions)', p.tok.position()) + return ast.Stmt{} } // literals, 'if', etc. in here else {