From 9aa4c766d8873e5f1604352bb6782095cd624e30 Mon Sep 17 00:00:00 2001 From: Alexander Medvednikov Date: Sun, 24 May 2026 20:39:36 +0300 Subject: [PATCH] =?UTF-8?q?v2:=20Drop/RAII=20codegen=20=E2=80=94=20emit=20?= =?UTF-8?q?drops=20at=20every=20return=20point=20(Phase=201B)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1A only emitted Type__drop(&var) at a fn's natural exit. Any `if err { return }` mid-fn leaked every owned Drop binding declared before the return. Phase 1B closes that hole. Checker: ownership_check_return snapshots (owned_vars ∩ drop_schedule) \ moved_vars at each ReturnStmt visit, appending to per-fn pending_return_drops in source order. Bindings being returned are already in moved_vars by snapshot time and get excluded. Snapshots publish to env.drop_at_returns[publish_key] when leaving the fn. cleanc: Gen preloads drop_at_returns[fn_name] into cur_fn_return_drops at fn entry. The ReturnStmt handler emits Type__drop(&var) calls in LIFO order and advances cur_fn_return_index — the checker's source walk and the codegen's source walk share an identical statement order, so the parallel counter stays aligned. gen_return_if_branch synthesizes ReturnStmts for `return if cond { x } else { y }`. Those represent the SAME source-level return — emitting drops for them again would consume the next snapshot and desync the counter. Guard with suppress_return_drop_emit around the synthesized gen_stmt calls. Tested: 4 new end-to-end tests cover early-return drop, moved-binding exclusion at early return, and multi-arm returns dropping the right bindings on each arm. All 5 v2/types tests pass; plain v2 build is byte-identical (gate is `$if ownership ?`). --- vlib/v2/gen/cleanc/cleanc.v | 57 ++++++---- vlib/v2/gen/cleanc/fn.v | 30 +++++ vlib/v2/gen/cleanc/if.v | 6 + vlib/v2/gen/cleanc/stmt.v | 1 + vlib/v2/types/checker.v | 15 +++ vlib/v2/types/checker_drop_codegen_test.v | 128 ++++++++++++++++++++++ vlib/v2/types/checker_ownership.v | 44 ++++++++ 7 files changed, 259 insertions(+), 22 deletions(-) diff --git a/vlib/v2/gen/cleanc/cleanc.v b/vlib/v2/gen/cleanc/cleanc.v index 775ff2ca2..312f7a27b 100644 --- a/vlib/v2/gen/cleanc/cleanc.v +++ b/vlib/v2/gen/cleanc/cleanc.v @@ -13,28 +13,41 @@ import time pub struct Gen { mut: - files []ast.File - env &types.Environment = unsafe { nil } - pref &pref.Preferences = unsafe { nil } - sb strings.Builder - indent int - cur_fn_scope &types.Scope = unsafe { nil } - cur_fn_name string - cur_fn_c_name string - cur_fn_ret_type string - cur_fn_c_ret_type string - cur_module string - cur_fn_scope_miss_key string - emitted_types map[string]bool - fn_param_is_ptr map[string][]bool - fn_param_types map[string][]string - fn_return_types map[string]string - v_fn_return_types map[string]string - runtime_local_types map[string]string - runtime_decl_types map[string]string - cur_fn_returned_idents map[string]bool - active_generic_types map[string]types.Type - cur_fn_generic_params map[string]string + files []ast.File + env &types.Environment = unsafe { nil } + pref &pref.Preferences = unsafe { nil } + sb strings.Builder + indent int + cur_fn_scope &types.Scope = unsafe { nil } + cur_fn_name string + cur_fn_c_name string + cur_fn_ret_type string + cur_fn_c_ret_type string + cur_module string + cur_fn_scope_miss_key string + // Per-return drop snapshots for the current fn, populated by the + // checker. Indexed positionally by `cur_fn_return_index`, which the + // ReturnStmt handler increments. Reset at every fn entry. Empty for + // fns the checker didn't see (e.g., plain V without `-d ownership`). + cur_fn_return_drops [][]types.DropEntry + cur_fn_return_index int + // Set true while gen_return_if_branch synthesizes ReturnStmts for + // `return if cond { x } else { y }`. The synthesized statements go + // through the same ReturnStmt handler, but they represent the SAME + // source-level return — emitting drops for them again would consume + // the next snapshot (belonging to a later source return) and break + // the parallel counter the checker relies on. + suppress_return_drop_emit bool + emitted_types map[string]bool + fn_param_is_ptr map[string][]bool + fn_param_types map[string][]string + fn_return_types map[string]string + v_fn_return_types map[string]string + runtime_local_types map[string]string + runtime_decl_types map[string]string + cur_fn_returned_idents map[string]bool + active_generic_types map[string]types.Type + cur_fn_generic_params map[string]string // Phase-1 generic monomorphization (V2_TRANSFORMER_MONOMORPH=1): // when true, the transformer has already cloned generic FnDecls per // env.generic_types binding. Cleanc skips its own spec emission paths diff --git a/vlib/v2/gen/cleanc/fn.v b/vlib/v2/gen/cleanc/fn.v index a259f77ef..a47e59881 100644 --- a/vlib/v2/gen/cleanc/fn.v +++ b/vlib/v2/gen/cleanc/fn.v @@ -4820,6 +4820,14 @@ fn (mut g Gen) gen_fn_decl_with_name_ptr(node &ast.FnDecl, fn_name string) { g.cur_fn_c_name = fn_name g.runtime_local_types.clear() g.runtime_decl_types.clear() + // Drop/RAII (Phase 1B): preload per-return drop snapshots for this fn + // so the ReturnStmt handler can emit `Type__drop(&var)` in source order. + g.cur_fn_return_drops = if g.env != unsafe { nil } { + g.env.drop_at_returns[fn_name] or { [][]types.DropEntry{} } + } else { + [][]types.DropEntry{} + } + g.cur_fn_return_index = 0 g.cur_fn_returned_idents = g.collect_returned_idents(node.stmts) g.is_module_ident_cache.clear() g.not_local_var_cache.clear() @@ -5079,6 +5087,28 @@ fn (mut g Gen) emit_scheduled_drops(stmts []ast.Stmt, fn_name string) { } } +// emit_scheduled_drops_at_return writes destructor calls for the bindings +// the checker recorded for the current ReturnStmt (Phase 1B). Snapshots +// are consumed positionally — the counter advances every call so the next +// ReturnStmt picks up the next snapshot. Drops emit in LIFO order +// (declaration-order reverse) for a single coherent scope. Bindings being +// returned (and thus moved out) were excluded by the checker. +fn (mut g Gen) emit_scheduled_drops_at_return() { + if g.suppress_return_drop_emit { + return + } + if g.cur_fn_return_index >= g.cur_fn_return_drops.len { + return + } + entries := g.cur_fn_return_drops[g.cur_fn_return_index] + g.cur_fn_return_index++ + for i := entries.len - 1; i >= 0; i-- { + entry := entries[i] + g.write_indent() + g.sb.writeln('${entry.type_name}__drop(&${entry.var_name});') + } +} + fn cleanc_stmts_end_with_return(stmts []ast.Stmt) bool { for i := stmts.len - 1; i >= 0; i-- { stmt := stmts[i] diff --git a/vlib/v2/gen/cleanc/if.v b/vlib/v2/gen/cleanc/if.v index ecfba8e1e..8d3d86089 100644 --- a/vlib/v2/gen/cleanc/if.v +++ b/vlib/v2/gen/cleanc/if.v @@ -214,7 +214,10 @@ fn (mut g Gen) gen_return_if_branch(stmts []ast.Stmt) { ret_stmt := ast.ReturnStmt{ exprs: ret_exprs } + prev_suppress := g.suppress_return_drop_emit + g.suppress_return_drop_emit = true g.gen_stmt(ret_stmt) + g.suppress_return_drop_emit = prev_suppress } else { g.gen_stmt(stmt) } @@ -262,7 +265,10 @@ fn (mut g Gen) gen_return_if_expr(if_expr ast.IfExpr, emit_indent bool) { else_stmt := ast.ReturnStmt{ exprs: else_exprs } + prev_suppress := g.suppress_return_drop_emit + g.suppress_return_drop_emit = true g.gen_stmt(else_stmt) + g.suppress_return_drop_emit = prev_suppress g.indent-- g.write_indent() g.sb.writeln('}') diff --git a/vlib/v2/gen/cleanc/stmt.v b/vlib/v2/gen/cleanc/stmt.v index 13b41f0ce..f7434070d 100644 --- a/vlib/v2/gen/cleanc/stmt.v +++ b/vlib/v2/gen/cleanc/stmt.v @@ -107,6 +107,7 @@ fn (mut g Gen) gen_stmt(node ast.Stmt) { g.sb.writeln(';') } ast.ReturnStmt { + g.emit_scheduled_drops_at_return() g.write_indent() if g.is_tuple_alias(g.cur_fn_ret_type) { if node.exprs.len == 1 { diff --git a/vlib/v2/types/checker.v b/vlib/v2/types/checker.v index fe4db7574..130ae2b69 100644 --- a/vlib/v2/types/checker.v +++ b/vlib/v2/types/checker.v @@ -38,6 +38,15 @@ pub mut: // `Type__drop(&var)` calls before the fn's closing brace. Only populated // when the checker runs with `-d ownership`; empty under plain V. drop_at_fn_exit map[string][]DropEntry + // Drop-codegen handoff for early returns: per-fn list of per-return-stmt + // drop snapshots, in source order. The cleanc backend walks the fn body + // in the same order and maintains a parallel counter to match each + // ReturnStmt to its snapshot. Each inner []DropEntry is the set of + // bindings that must be dropped just BEFORE the corresponding return. + // Bindings that are themselves being returned (and thus moved out) are + // excluded by the checker — the return moves them. Only populated when + // the checker runs with `-d ownership`; empty under plain V. + drop_at_returns map[string][][]DropEntry } pub fn Environment.new() &Environment { @@ -608,6 +617,11 @@ mut: // values are bound; pruned (via the `moved_vars` view) when they are // moved or returned. Exposed so the transformer / backends can lower it. drop_schedule map[string][]DropEntry + // Per-fn list of per-return drop snapshots collected during checking, + // in source order. Reset at fn entry, published to + // `env.drop_at_returns[publish_key]` at fn exit. Transient — not used + // outside of fn body checking. + pending_return_drops [][]DropEntry // Source positions for `implements Drop` struct declarations, indexed // by struct name. Used to point the "missing drop method" diagnostic at // the struct decl rather than at an unrelated use site. @@ -3917,6 +3931,7 @@ fn (mut c Checker) check_pending_fn_body(pending PendingFnBody) { $if ownership ? { publish_keys := ownership_publish_keys_for(pending.module_name, pending.decl) c.ownership_snapshot_drops_at_fn_exit(pending.decl.name, publish_keys) + c.ownership_publish_pending_return_drops(publish_keys) c.ownership_leave_fn(prev_ownership_fn, prev_owned, prev_owned_types, prev_moved, prev_borrowed) } diff --git a/vlib/v2/types/checker_drop_codegen_test.v b/vlib/v2/types/checker_drop_codegen_test.v index 4a75fd10e..ca7835ae0 100644 --- a/vlib/v2/types/checker_drop_codegen_test.v +++ b/vlib/v2/types/checker_drop_codegen_test.v @@ -148,3 +148,131 @@ fn main() { assert output.contains('got 42'), 'expected user print, got: ${output}' assert !output.contains('dropping'), 'no drop should fire for non-Drop type, got: ${output}' } + +fn test_drop_fires_before_early_return() { + // Phase 1B: early `return` mid-fn must drop every still-owned Drop binding + // declared before it, in LIFO order. Without this, fns that bail out via + // `if err { return }` leak every resource declared on the happy path. + code := " +struct Foo implements Drop { + id int +} + +fn (mut self Foo) drop() { + println('dropping ' + self.id.str()) +} + +fn run() { + a := Foo{id: 1} + b := Foo{id: 2} + _ = a.id + _ = b.id + println('before return') + return +} + +fn main() { + run() + println('after run') +} +" + exit_code, output := run_drop_program(code) + assert exit_code == 0, 'program should exit 0: ${output}' + // Drops must fire BEFORE the caller observes the return — i.e. before + // 'after run' is printed — and in LIFO order (b first, then a). + before_idx := output.index('before return') or { -1 } + b_idx := output.index('dropping 2') or { -1 } + a_idx := output.index('dropping 1') or { -1 } + after_idx := output.index('after run') or { -1 } + assert before_idx >= 0 && b_idx > before_idx && a_idx > b_idx && after_idx > a_idx, 'drops must fire LIFO at early return, before caller resumes, got: ${output}' +} + +fn test_drop_early_return_skips_moved_binding() { + // A binding moved into a fn call before the early return must NOT be + // dropped again at the return — its drop responsibility transferred. + code := " +struct Foo implements Drop, Owned { + id int +} + +fn (mut self Foo) drop() { + println('dropping ' + self.id.str()) +} + +fn take(f Foo) { + println('took ' + f.id.str()) +} + +fn run() { + a := Foo{id: 1} + b := Foo{id: 2} + take(a) + println('before return') + return + _ = b.id +} + +fn main() { + run() +} +" + exit_code, output := run_drop_program(code) + assert exit_code == 0, 'program should exit 0: ${output}' + assert output.contains('took 1'), 'expected take call, got: ${output}' + assert output.contains('dropping 2'), 'b must drop at early return, got: ${output}' + // a was moved into take — must not produce a second drop for id=1. + dropping_1_count := output.count('dropping 1') + assert dropping_1_count == 0, 'moved binding must not drop at early return, got ${dropping_1_count} occurrences in: ${output}' +} + +fn test_drop_multiple_returns_each_drop_independently() { + // Each return point gets its own drop set computed at that point. A fn + // with two return arms should drop the right bindings on each arm. + code := " +struct Foo implements Drop { + id int +} + +fn (mut self Foo) drop() { + println('dropping ' + self.id.str()) +} + +fn run(flag bool) { + a := Foo{id: 1} + _ = a.id + if flag { + b := Foo{id: 2} + _ = b.id + println('A') + return + } + c := Foo{id: 3} + _ = c.id + println('B') +} + +fn main() { + run(true) + println('---') + run(false) +} +" + exit_code, output := run_drop_program(code) + assert exit_code == 0, 'program should exit 0: ${output}' + // Path A (flag=true): drops b then a at the early return. + // Path B (flag=false): drops c then a at natural exit. + sep_idx := output.index('---') or { -1 } + assert sep_idx > 0, 'expected separator, got: ${output}' + head := output[..sep_idx] + tail := output[sep_idx..] + a_head_idx := head.index('dropping 1') or { -1 } + b_head_idx := head.index('dropping 2') or { -1 } + assert b_head_idx >= 0 && a_head_idx > b_head_idx, 'early-return path must drop b then a, got: ${head}' + + a_tail_idx := tail.index('dropping 1') or { -1 } + c_tail_idx := tail.index('dropping 3') or { -1 } + assert c_tail_idx >= 0 && a_tail_idx > c_tail_idx, 'natural-exit path must drop c then a, got: ${tail}' + + // b was never declared on the B path. + assert !tail.contains('dropping 2'), 'b must not appear on B path, got: ${tail}' +} diff --git a/vlib/v2/types/checker_ownership.v b/vlib/v2/types/checker_ownership.v index 28e09ba8a..586466a12 100644 --- a/vlib/v2/types/checker_ownership.v +++ b/vlib/v2/types/checker_ownership.v @@ -253,6 +253,31 @@ fn (mut c Checker) ownership_check_return(stmt ast.ReturnStmt) { c.ownership_fns[c.ownership_cur_fn] = true } } + // Snapshot drops live AT this return. Run after the loop above so any + // binding being returned is already in `moved_vars` and gets excluded. + c.ownership_snapshot_drops_at_return() +} + +// ownership_snapshot_drops_at_return records the set of Drop-typed bindings +// that are still owned at the current return point. Bindings being returned +// (and thus moved out) are excluded — the surrounding loop in +// `ownership_check_return` adds them to `moved_vars` before we run. The +// snapshot is appended to `c.pending_return_drops` in source order; the +// codegen counter consumes it positionally. An empty snapshot is appended +// for returns with no live drops so the codegen counter stays aligned with +// the source-order index of every ReturnStmt. +fn (mut c Checker) ownership_snapshot_drops_at_return() { + scheduled := c.drop_schedule[c.ownership_cur_fn] or { + c.pending_return_drops << []DropEntry{} + return + } + mut live := []DropEntry{cap: scheduled.len} + for entry in scheduled { + if entry.var_name in c.owned_vars && entry.var_name !in c.moved_vars { + live << entry + } + } + c.pending_return_drops << live } // ownership_mark_from_call detects calls that produce owned values: @@ -417,6 +442,7 @@ fn (mut c Checker) ownership_release_borrow(var_name string, borrower string) { // Marks parameters that received owned values from call sites as owned within this scope. fn (mut c Checker) ownership_enter_fn(fn_name string, decl ast.FnDecl) { c.ownership_cur_fn = fn_name + c.pending_return_drops = [][]DropEntry{} // Check if any parameters of this function received owned values at call sites for i, param in decl.typ.params { key := '${fn_name}__param_${i}' @@ -980,6 +1006,24 @@ fn (mut c Checker) ownership_snapshot_drops_at_fn_exit(schedule_key string, publ } } +// ownership_publish_pending_return_drops moves the per-return drop snapshots +// collected for the current fn into `env.drop_at_returns` under each +// publish key. Each entry is appended in source-order so the cleanc backend +// can match them positionally by walking the fn body in the same order. +// Skipped entirely when no returns were seen (all-drops-at-natural-exit +// case), since the fn-exit map already covers it. +fn (mut c Checker) ownership_publish_pending_return_drops(publish_keys []string) { + if c.pending_return_drops.len == 0 || publish_keys.len == 0 { + return + } + for key in publish_keys { + if key.len == 0 { + continue + } + c.env.drop_at_returns[key] = c.pending_return_drops.clone() + } +} + // ownership_publish_keys_for builds the list of C-mangled fn names under // which a fn's drop schedule should be published, so the cleanc backend // can match by whichever convention it picks. Mirrors cleanc's -- 2.39.5