From b9ec1479e47750a535e62a05dbeb42db3bffb5fe Mon Sep 17 00:00:00 2001 From: Delyan Angelov Date: Wed, 8 Apr 2020 02:19:24 +0300 Subject: [PATCH] checker: allow again fallthrough in or{} blocks of option calls without assignment --- vlib/os/inode_test.v | 10 +- vlib/v/checker/checker.v | 95 ++++++++++++------- vlib/v/tests/defer_test.v | 5 +- .../option_if_assign_and_fallthrough_test.v | 24 +++++ 4 files changed, 92 insertions(+), 42 deletions(-) create mode 100644 vlib/v/tests/option_if_assign_and_fallthrough_test.v diff --git a/vlib/os/inode_test.v b/vlib/os/inode_test.v index 04ca6b33f0..7e875486d5 100644 --- a/vlib/os/inode_test.v +++ b/vlib/os/inode_test.v @@ -24,8 +24,9 @@ fn testsuite_end() { fn test_inode_file_type() { filename := './test1.txt' - mut file := os.open_file(filename, 'w', 0o600) - file.close() + if file := os.open_file(filename, 'w', 0o600) { + file.close() + } mode := os.inode(filename) os.rm(filename) assert mode.typ == .regular @@ -33,8 +34,9 @@ fn test_inode_file_type() { fn test_inode_file_owner_permission() { filename := './test2.txt' - mut file := os.open_file(filename, 'w', 0o600) - file.close() + if file := os.open_file(filename, 'w', 0o600) { + file.close() + } mode := os.inode(filename) os.rm(filename) assert mode.owner.read diff --git a/vlib/v/checker/checker.v b/vlib/v/checker/checker.v index c486bb9749..4059a07584 100644 --- a/vlib/v/checker/checker.v +++ b/vlib/v/checker/checker.v @@ -215,6 +215,7 @@ fn (c mut Checker) assign_expr(assign_expr mut ast.AssignExpr) { right_type_sym := c.table.get_type_symbol(right_type) c.error('cannot assign `$right_type_sym.name` to `$left_type_sym.name`', assign_expr.pos) } + c.check_expr_opt_call(assign_expr.val, right_type, true) } pub fn (c mut Checker) call_expr(call_expr mut ast.CallExpr) table.Type { @@ -396,49 +397,72 @@ pub fn (c mut Checker) call_expr(call_expr mut ast.CallExpr) table.Type { } } -pub fn (c mut Checker) check_or_block(call_expr mut ast.CallExpr, ret_type table.Type) { - if call_expr.or_block.is_used { - stmts_len := call_expr.or_block.stmts.len - if stmts_len != 0 { - last_stmt := call_expr.or_block.stmts[stmts_len - 1] - - if c.is_or_block_stmt_valid(last_stmt) { - match last_stmt { - ast.ExprStmt { - type_fits := c.table.check(c.expr(it.expr), ret_type) - is_panic_or_exit := is_expr_panic_or_exit(it.expr) - if !type_fits && !is_panic_or_exit { - type_name := c.table.get_type_symbol(c.expr(it.expr)).name - expected_type_name := c.table.get_type_symbol(ret_type).name - c.error('wrong return type ‘$type_name‘ in or{} block, expected ‘$expected_type_name‘', it.pos) - } - } - ast.BranchStmt { - if !(it.tok.kind in [.key_continue, .key_break]) { - c.error('only break and continue are allowed as a final branch statement in an or{} block', it.tok.position()) - } - } - else {} - } - } else { - expected_type_name := c.table.get_type_symbol(ret_type).name - c.error('last statement in or{} block should return ‘$expected_type_name‘', call_expr.pos) +pub fn (c mut Checker) check_expr_opt_call(x ast.Expr, xtype table.Type, is_return_used bool){ + match x { + ast.CallExpr { + if table.type_is(it.return_type, .optional) { + c.check_or_block(it, xtype, is_return_used) } - } else { - c.error('require a statement in or{} block', call_expr.pos) } + else{} + } +} + +pub fn (c mut Checker) check_or_block(call_expr mut ast.CallExpr, ret_type table.Type, is_ret_used bool) { + if !call_expr.or_block.is_used { + c.error('${call_expr.name}() returns an option, but you missed to add an `or {}` block to it', call_expr.pos) + return + } + stmts_len := call_expr.or_block.stmts.len + if stmts_len == 0 { + if is_ret_used { + // x := f() or {} + c.error('assignment requires a non empty `or {}` block', call_expr.pos) + return + } + // allow `f() or {}` + return + } + last_stmt := call_expr.or_block.stmts[stmts_len - 1] + if is_ret_used { + if !c.is_last_or_block_stmt_valid(last_stmt) { + expected_type_name := c.table.get_type_symbol(ret_type).name + c.error('last statement in the `or {}` block should return ‘$expected_type_name‘', call_expr.pos) + return + } + match last_stmt { + ast.ExprStmt { + type_fits := c.table.check(c.expr(it.expr), ret_type) + is_panic_or_exit := is_expr_panic_or_exit(it.expr) + if type_fits || is_panic_or_exit { + return + } + type_name := c.table.get_type_symbol(c.expr(it.expr)).name + expected_type_name := c.table.get_type_symbol(ret_type).name + c.error('wrong return type `$type_name` in the `or {}` block, expected `$expected_type_name`', it.pos) + return + } + ast.BranchStmt { + if !(it.tok.kind in [.key_continue, .key_break]) { + c.error('only break/continue is allowed as a branch statement in the end of an `or {}` block', it.tok.position()) + return + } + } + else {} + } + return } } fn is_expr_panic_or_exit(expr ast.Expr) bool { match expr { - ast.CallExpr { return it.name == 'panic' || it.name == 'exit' } + ast.CallExpr { return it.name in ['panic','exit'] } else { return false } } } // TODO: merge to check_or_block when v can handle it -pub fn (c mut Checker) is_or_block_stmt_valid(stmt ast.Stmt) bool { +pub fn (c mut Checker) is_last_or_block_stmt_valid(stmt ast.Stmt) bool { return match stmt { ast.Return { true } ast.BranchStmt { true } @@ -555,6 +579,7 @@ pub fn (c mut Checker) assign_stmt(assign_stmt mut ast.AssignStmt) { assign_stmt.right_types << val_type scope.update_var_type(ident.name, val_type) } + c.check_expr_opt_call(assign_stmt.right[0], right_type, true) } else { // `a := 1` | `a,b := 1,2` if assign_stmt.left.len != assign_stmt.right.len { @@ -581,6 +606,7 @@ pub fn (c mut Checker) assign_stmt(assign_stmt mut ast.AssignStmt) { assign_stmt.left[i] = ident assign_stmt.right_types << val_type scope.update_var_type(ident.name, val_type) + c.check_expr_opt_call(assign_stmt.right[i], val_type, true) } } c.expected_type = table.void_type @@ -711,8 +737,9 @@ fn (c mut Checker) stmt(node ast.Stmt) { } } ast.ExprStmt { - c.expr(it.expr) + etype := c.expr(it.expr) c.expected_type = table.void_type + c.check_expr_opt_call(it.expr, etype, false) } ast.FnDecl { // if it.is_method { @@ -851,9 +878,7 @@ pub fn (c mut Checker) expr(node ast.Expr) table.Type { return it.typ } ast.CallExpr { - call_ret_type := c.call_expr(mut it) - c.check_or_block(mut it, call_ret_type) - return call_ret_type + return c.call_expr(mut it) } ast.CharLiteral { return table.byte_type diff --git a/vlib/v/tests/defer_test.v b/vlib/v/tests/defer_test.v index ee2cb21d94..b0e48870d2 100644 --- a/vlib/v/tests/defer_test.v +++ b/vlib/v/tests/defer_test.v @@ -46,8 +46,7 @@ mut: } fn test_defer_early_exit() { - mut sum := Num{ - 0} + mut sum := Num{0} for i in 0 .. 10 { set_num(i, mut sum) } @@ -57,6 +56,6 @@ fn test_defer_early_exit() { fn test_defer_option() { mut ok := Num{0} - set_num_opt(mut ok) + set_num_opt(mut ok) or {} assert ok.val == 1 } diff --git a/vlib/v/tests/option_if_assign_and_fallthrough_test.v b/vlib/v/tests/option_if_assign_and_fallthrough_test.v new file mode 100644 index 0000000000..e37fe77524 --- /dev/null +++ b/vlib/v/tests/option_if_assign_and_fallthrough_test.v @@ -0,0 +1,24 @@ +fn err_call(ok bool) ?int { + if ok { + return 42 + } + return error('Not ok!') +} + +fn test_if_opt() { + if val := err_call(true) { + eprintln(' val should be available here: $val') + assert val == 42 + } + assert true +} + +fn test_opt_with_fall_through() { + mut x := 1 + err_call(false) or { + eprintln(' this *should* be an error: $err') + x++ + assert true + } + assert x == 2 +}