From e1e3d31917b3252d49161f5597c8b873999ba71b Mon Sep 17 00:00:00 2001 From: Alexander Medvednikov Date: Tue, 21 Apr 2026 05:45:45 +0300 Subject: [PATCH] checker: fix !-propagating call to always-erroring function still requires placeholder return (fixes #26895) --- vlib/v/checker/checker.v | 8 +- vlib/v/checker/fn.v | 2 +- vlib/v/checker/if.v | 2 +- vlib/v/checker/match.v | 2 +- vlib/v/checker/return.v | 213 ++++++++++++++++++++++++++++----- vlib/v/tests/error_void_test.v | 16 +++ 6 files changed, 209 insertions(+), 34 deletions(-) diff --git a/vlib/v/checker/checker.v b/vlib/v/checker/checker.v index 235312f09..731b8295e 100644 --- a/vlib/v/checker/checker.v +++ b/vlib/v/checker/checker.v @@ -163,6 +163,8 @@ mut: visible_param_mutation_cache map[string]bool visible_param_mutation_in_progress map[string]bool immutable_alias_analysis_in_progress map[string]bool + always_error_fn_cache map[string]bool + always_error_fn_in_progress map[string]bool v_current_commit_hash string // same as old C.V_CURRENT_COMMIT_HASH assign_stmt_attr string // for `x := [1,2,3] @[freed]` @@ -194,6 +196,8 @@ pub fn new_checker(table &ast.Table, pref_ &pref.Preferences) &Checker { visible_param_mutation_cache: map[string]bool{} visible_param_mutation_in_progress: map[string]bool{} immutable_alias_analysis_in_progress: map[string]bool{} + always_error_fn_cache: map[string]bool{} + always_error_fn_in_progress: map[string]bool{} } checker.checker_transformer.skip_array_transform = true checker.type_resolver = type_resolver.TypeResolver.new(table, checker) @@ -7376,7 +7380,7 @@ fn (mut c Checker) find_obj_definition(obj ast.ScopeObject) !ast.Expr { return expr } -fn (c &Checker) has_return(stmts []ast.Stmt) ?bool { +fn (mut c Checker) has_return(stmts []ast.Stmt) ?bool { // complexity means either more match or ifs mut has_complexity := false for s in stmts { @@ -7389,7 +7393,7 @@ fn (c &Checker) has_return(stmts []ast.Stmt) ?bool { } // if the inner complexity covers all paths with returns there is no need for further checks if !has_complexity || !c.returns { - return has_top_return(stmts) + return c.has_top_return(stmts) } return none } diff --git a/vlib/v/checker/fn.v b/vlib/v/checker/fn.v index 2dba69b5e..a9e047377 100644 --- a/vlib/v/checker/fn.v +++ b/vlib/v/checker/fn.v @@ -977,7 +977,7 @@ fn (mut c Checker) fn_decl(mut node ast.FnDecl) { } } c.stmts(mut node.stmts) - node_has_top_return := has_top_return(node.stmts) + node_has_top_return := c.has_top_return(node.stmts) node.has_return = c.returns || node_has_top_return c.check_noreturn_fn_decl(mut node) if node.language == .v && !node.no_body && node.return_type != ast.void_type && !node.has_return diff --git a/vlib/v/checker/if.v b/vlib/v/checker/if.v index cdb48a41b..4ff196a6a 100644 --- a/vlib/v/checker/if.v +++ b/vlib/v/checker/if.v @@ -606,7 +606,7 @@ fn (mut c Checker) if_expr(mut node ast.IfExpr) ast.Type { c.returns = false } } - if !node.is_comptime && node.branches.len > 0 && has_top_return(node.branches[0].stmts) + if !node.is_comptime && node.branches.len > 0 && c.has_top_return(node.branches[0].stmts) && node.branches[0].scope != unsafe { nil } && node.branches[0].scope.parent != unsafe { nil } { mut continuation_scope := node.branches[0].scope.parent diff --git a/vlib/v/checker/match.v b/vlib/v/checker/match.v index 9e76a8d28..32abe707a 100644 --- a/vlib/v/checker/match.v +++ b/vlib/v/checker/match.v @@ -504,7 +504,7 @@ fn (mut c Checker) match_expr(mut node ast.MatchExpr) ast.Type { } if node.is_comptime { // branches may not have been processed by c.stmts() - if has_top_return(branch.stmts) { + if c.has_top_return(branch.stmts) { nbranches_with_return++ } else { nbranches_without_return++ diff --git a/vlib/v/checker/return.v b/vlib/v/checker/return.v index 8230ed638..4a0e559ab 100644 --- a/vlib/v/checker/return.v +++ b/vlib/v/checker/return.v @@ -385,40 +385,20 @@ fn (mut c Checker) find_unreachable_statements_after_noreturn_calls(stmts []ast. // Note: has_top_return/1 should be called on *already checked* stmts, // which do have their stmt.expr.is_noreturn set properly: -fn has_top_return(stmts []ast.Stmt) bool { +fn (mut c Checker) has_top_return(stmts []ast.Stmt) bool { for stmt in stmts { match stmt { ast.Return { return true } ast.Block { - if has_top_return(stmt.stmts) { + if c.has_top_return(stmt.stmts) { return true } } ast.ExprStmt { - if stmt.expr is ast.CallExpr { - // do not ignore panic() calls on non checked stmts - if stmt.expr.is_noreturn - || (stmt.expr.is_method == false && stmt.expr.name == 'panic') { - return true - } - } else if stmt.expr is ast.ComptimeCall { - if stmt.expr.kind == .compile_error { - return true - } - } else if stmt.expr is ast.LockExpr { - if has_top_return(stmt.expr.stmts) { - return true - } - } else if stmt.expr is ast.IfExpr { - if has_top_return_in_if_expr(stmt.expr) { - return true - } - } else if stmt.expr is ast.MatchExpr { - if has_top_return_in_match_expr(stmt.expr) { - return true - } + if c.expr_never_falls_through(stmt.expr) { + return true } } else {} @@ -427,19 +407,161 @@ fn has_top_return(stmts []ast.Stmt) bool { return false } -fn has_top_return_in_if_expr(if_expr ast.IfExpr) bool { +fn (mut c Checker) expr_never_falls_through(expr ast.Expr) bool { + match expr { + ast.CallExpr { + // do not ignore panic() calls on non checked stmts + return expr.is_noreturn + || (expr.is_method == false && expr.name == 'panic') + || c.call_expr_propagates_always_error(expr) + } + ast.ComptimeCall { + return expr.kind == .compile_error + } + ast.LockExpr { + return c.has_top_return(expr.stmts) + } + ast.IfExpr { + return c.has_top_return_in_if_expr(expr) + } + ast.MatchExpr { + return c.has_top_return_in_match_expr(expr) + } + else {} + } + + return false +} + +fn (mut c Checker) call_expr_propagates_always_error(node ast.CallExpr) bool { + if node.should_be_skipped || node.or_block.kind != .propagate_result { + return false + } + mut func := ast.Fn{} + if node.is_method { + if node.left_type == 0 { + return false + } + left_sym := c.table.sym(c.unwrap_generic(node.left_type)) + if left_sym.kind == .placeholder { + return false + } + func = c.table.find_method(left_sym, node.name) or { return false } + } else { + if node.name == '' { + return false + } + func = c.table.find_fn(node.name) or { return false } + } + return c.fn_always_errors(func) +} + +fn (mut c Checker) fn_always_errors(func ast.Fn) bool { + if func.name == '' || func.source_fn == unsafe { nil } || func.no_body || func.language != .v + || !func.return_type.has_flag(.result) { + return false + } + fkey := func.fkey() + if fkey in c.always_error_fn_cache { + return c.always_error_fn_cache[fkey] + } + if fkey in c.always_error_fn_in_progress { + return false + } + c.always_error_fn_in_progress[fkey] = true + defer { + c.always_error_fn_in_progress.delete(fkey) + } + fn_decl := unsafe { &ast.FnDecl(func.source_fn) } + result := c.stmts_always_error(fn_decl.stmts) + c.always_error_fn_cache[fkey] = result + return result +} + +fn (mut c Checker) stmts_always_error(stmts []ast.Stmt) bool { + for stmt in stmts { + if c.stmt_always_errors(stmt) { + return true + } + } + return false +} + +fn (mut c Checker) stmt_always_errors(stmt ast.Stmt) bool { + match stmt { + ast.Return { + return c.return_stmt_always_errors(stmt) + } + ast.Block { + return c.stmts_always_error(stmt.stmts) + } + ast.ExprStmt { + return c.expr_always_errors(stmt.expr) + } + ast.ForStmt { + return stmt.is_inf && stmt.stmts.len == 0 + } + else {} + } + + return false +} + +fn (mut c Checker) expr_always_errors(expr ast.Expr) bool { + match expr { + ast.CallExpr { + return expr.is_noreturn || c.call_expr_propagates_always_error(expr) + } + ast.ComptimeCall { + return expr.kind == .compile_error + } + ast.IfExpr { + return c.if_expr_always_errors(expr) + } + ast.MatchExpr { + return c.match_expr_always_errors(expr) + } + ast.LockExpr { + return c.stmts_always_error(expr.stmts) + } + else {} + } + + return false +} + +fn (mut c Checker) return_stmt_always_errors(node ast.Return) bool { + if node.types.len == 1 && c.table.unaliased_type(node.types[0]).idx() == ast.error_type_idx { + return true + } + return false +} + +fn (mut c Checker) has_top_return_in_if_expr(if_expr ast.IfExpr) bool { if if_expr.branches.len < 2 || !if_expr.has_else { return false } for branch in if_expr.branches { - if !has_top_return(branch.stmts) { + if !c.has_top_return(branch.stmts) { return false } } return true } -fn has_top_return_in_match_expr(match_expr ast.MatchExpr) bool { +fn (mut c Checker) if_expr_always_errors(if_expr ast.IfExpr) bool { + if if_expr.branches.len < 2 || !if_expr.has_else { + return false + } + for branch in if_expr.branches { + if !c.stmts_always_error(branch.stmts) { + return false + } + } + return true +} + +fn (mut c Checker) has_top_return_in_match_expr(match_expr ast.MatchExpr) bool { if match_expr.branches.len == 0 { return false } @@ -462,7 +584,7 @@ fn has_top_return_in_match_expr(match_expr ast.MatchExpr) bool { } if has_else { for branch in match_expr.branches { - if !has_top_return(branch.stmts) { + if !c.has_top_return(branch.stmts) { return false } } @@ -471,7 +593,40 @@ fn has_top_return_in_match_expr(match_expr ast.MatchExpr) bool { return false } for branch in match_expr.branches { - if !has_top_return(branch.stmts) { + if !c.has_top_return(branch.stmts) { + return false + } + } + return true +} + +fn (mut c Checker) match_expr_always_errors(match_expr ast.MatchExpr) bool { + if match_expr.branches.len == 0 { + return false + } + if match_expr.is_comptime { + mut has_else := false + for branch in match_expr.branches { + if branch.is_else { + has_else = true + break + } + for expr in branch.exprs { + if expr is ast.Ident && expr.name == '\$else' { + has_else = true + break + } + } + if has_else { + break + } + } + if !has_else { + return false + } + } + for branch in match_expr.branches { + if !c.stmts_always_error(branch.stmts) { return false } } diff --git a/vlib/v/tests/error_void_test.v b/vlib/v/tests/error_void_test.v index e63d79562..ea02f12dc 100644 --- a/vlib/v/tests/error_void_test.v +++ b/vlib/v/tests/error_void_test.v @@ -48,3 +48,19 @@ fn test_option_void_with_return() { } assert true } + +fn must_error_for_generic_result() ! { + return error('boom') +} + +fn never_returns_normally[T](value T) !T { + must_error_for_generic_result()! +} + +fn test_generic_result_propagation_from_always_error_fn_does_not_need_placeholder_return() { + never_returns_normally(42) or { + assert err.msg() == 'boom' + return + } + assert false +} -- 2.39.5