diff --git a/vlib/v/tests/ownership_recursive_return_param_alias_test.v b/vlib/v/tests/ownership_recursive_return_param_alias_test.v new file mode 100644 index 00000000000000..9820019b664d1d --- /dev/null +++ b/vlib/v/tests/ownership_recursive_return_param_alias_test.v @@ -0,0 +1,58 @@ +// vtest vflags: -d ownership +// Recursive calls prepend the argument projection to the callee's returned parameter path +// (`.left.name`, `.left.right.name`, ...). Return alias inference used to grow those paths +// forever, so compiling this file with the ownership checker never finished. + +struct RecursiveAliasNode { + name string + left &RecursiveAliasNode = unsafe { nil } + right &RecursiveAliasNode = unsafe { nil } +} + +fn recursive_alias_outermost(n &RecursiveAliasNode, go_left bool) string { + if go_left && !isnil(n.left) { + return recursive_alias_outermost(n.left, go_left) + } + if !go_left && !isnil(n.right) { + return recursive_alias_outermost(n.right, go_left) + } + return n.name +} + +fn recursive_alias_zigzag_left(n &RecursiveAliasNode) string { + if isnil(n.left) { + return n.name + } + return recursive_alias_zigzag_right(n.left) +} + +fn recursive_alias_zigzag_right(n &RecursiveAliasNode) string { + if isnil(n.right) { + return n.name + } + return recursive_alias_zigzag_left(n.right) +} + +fn test_recursive_return_param_aliases_reach_fixed_point() { + leaf_a := &RecursiveAliasNode{ + name: 'a' + } + leaf_b := &RecursiveAliasNode{ + name: 'b' + } + mid := &RecursiveAliasNode{ + name: 'mid' + left: leaf_a + right: leaf_b + } + root := &RecursiveAliasNode{ + name: 'root' + left: mid + right: leaf_a + } + assert recursive_alias_outermost(root, true) == 'a' + assert recursive_alias_outermost(root, false) == 'a' + assert recursive_alias_outermost(mid, false) == 'b' + assert recursive_alias_zigzag_left(root) == 'b' + assert recursive_alias_zigzag_right(root) == 'a' +} diff --git a/vlib/v/transform/array.v b/vlib/v/transform/array.v index 46e2325bbc35d5..8ce8dbb9aad2b5 100644 --- a/vlib/v/transform/array.v +++ b/vlib/v/transform/array.v @@ -3808,6 +3808,21 @@ fn (mut t Transformer) array_map_call_result_path_origin_is_external(source type if root !in origins { return true } + if source.source_is_prefix { + // The result aliases some storage at or below the source path, but not a known one, + // so keep it external when the path covering that storage, or any path below it, is. + prefix_path := source_path + source.source_suffix + if origins[array_map_local_pointer_path(prefix_path, root, origins)] { + return true + } + for path, external in origins { + if external && !path.starts_with(array_map_local_pointer_pointee_prefix) + && array_map_local_path_is_possible_projection(path, prefix_path) { + return true + } + } + return false + } effective_path := source_path + source.source_suffix + relative_suffix return origins[array_map_local_pointer_path(effective_path, root, origins)] } diff --git a/vlib/v/types/checker.v b/vlib/v/types/checker.v index c7c1303545abdc..c4f328a5a2e70e 100644 --- a/vlib/v/types/checker.v +++ b/vlib/v/types/checker.v @@ -245,6 +245,9 @@ pub: arg_id flat.NodeId source_suffix string target_suffix string + // source_is_prefix reports that the result aliases storage at or below `source_suffix` + // of the argument, but not a known exact path (a path widened through recursion). + source_is_prefix bool } // LocalBinding represents local binding data used by types. diff --git a/vlib/v/types/checker_ownership_alias.v b/vlib/v/types/checker_ownership_alias.v new file mode 100644 index 00000000000000..e20a19c53458e0 --- /dev/null +++ b/vlib/v/types/checker_ownership_alias.v @@ -0,0 +1,63 @@ +module types + +// This file is compiled with and without `-d ownership`, so that its pure helpers can be +// unit tested by the regular compiler. A `-d ownership` build selects the ownership +// compiler, which would also run ownership analysis over the whole `types` module. + +const ownership_unknown_pointer_index_alias = '' + +// ownership_alias_chain_borrows_indexed_storage reports whether `rhs_name` reaches indexed +// storage (or an unknown index alias) through the recorded pointer index aliases. +fn ownership_alias_chain_borrows_indexed_storage(aliases map[string]string, rhs_name string) bool { + // Follow the complete alias chain (`arr -> val -> t[k]`). Cycles represent unresolved + // alias state, so treat them conservatively as borrowed storage. + mut cur := rhs_name + mut seen := map[string]bool{} + for { + if seen[cur] { + return true + } + seen[cur] = true + source := aliases[cur] or { return false } + if source == ownership_unknown_pointer_index_alias { + return true + } + if source.contains('[') { + return true + } + cur = source + } + return false +} + +// ownership_return_param_call_source composes, in function `caller`, a callee's returned +// parameter path behind the projection of the argument passed for that parameter: when the +// callee returns `.name` of its parameter, `callee(p.next)` returns `.next.name` of `p`. It also +// returns whether the result is a prefix source, which aliases storage at or below the path +// rather than that exact path, and the functions that composed the path (`callee_via` lists +// those of the callee's path). +// +// Around a call cycle the argument projection is prepended again every round (`.next.name`, +// `.next.next.name`, ...), so the return fixed point would never converge, and it would grow +// exponentially for a function that recurses through several fields. A cycle shows as a +// callee path that `caller` already composed; such a call yields a prefix source at the +// argument projection. Every other composition extends `via` by a function that is not in it +// yet, which bounds the exact paths, and keeps acyclic call chains exact. +fn ownership_return_param_call_source(caller string, arg_suffix string, callee_suffix string, callee_is_prefix bool, callee_via []string) (string, bool, []string) { + if caller in callee_via { + return arg_suffix, true, [caller] + } + return arg_suffix + callee_suffix, callee_is_prefix, ownership_return_param_via(callee_via, + caller) +} + +// ownership_return_param_via returns `via` extended by `caller`, unless it already lists it. +fn ownership_return_param_via(via []string, caller string) []string { + if caller in via { + return via + } + mut out := []string{cap: via.len + 1} + out << via + out << caller + return out +} diff --git a/vlib/v/types/checker_ownership_alias_test.v b/vlib/v/types/checker_ownership_alias_test.v index fb2272599cc739..6e36a0c02ff4f1 100644 --- a/vlib/v/types/checker_ownership_alias_test.v +++ b/vlib/v/types/checker_ownership_alias_test.v @@ -1,4 +1,3 @@ -// vtest vflags: -d ownership module types fn test_long_pointer_index_alias_chain_is_borrowed() { @@ -22,3 +21,117 @@ fn test_long_pointer_index_alias_chain_is_borrowed() { } assert ownership_alias_chain_borrows_indexed_storage(cycle, 'left') } + +fn test_return_param_call_source_keeps_acyclic_chains_exact() { + // `inner(p)` returns `p.left.name`, and `outer(p)` returns `inner(p.left)`: the repeated + // `.left` projection is not a cycle. + mut suffix, mut is_prefix, mut via := ownership_return_param_call_source('outer', '.left', + '.left.name', false, ['inner']) + assert suffix == '.left.left.name' + assert !is_prefix + assert via == ['inner', 'outer'] + // Depth alone never widens a path. + suffix, is_prefix, via = ownership_return_param_call_source('a', '.a', '.b.c.d.e.f', false, + ['b', 'c', 'd', 'e']) + assert suffix == '.a.b.c.d.e.f' + assert !is_prefix + // A prefix source stays a prefix source when it is composed further. + suffix, is_prefix, via = ownership_return_param_call_source('a', '.a', '.b', true, ['b']) + assert suffix == '.a.b' + assert is_prefix + assert via == ['b', 'a'] +} + +fn test_return_param_call_source_widens_call_cycles_to_prefixes() { + // Direct recursion: the callee's own path lists the caller. + mut suffix, mut is_prefix, mut via := ownership_return_param_call_source('last', '.left', + '.name', false, ['last']) + assert suffix == '.left' + assert is_prefix + assert via == ['last'] + // Mutual recursion: `zig` composed the path that `zag` returns to it. + suffix, is_prefix, via = ownership_return_param_call_source('zig', '[0]', '.alt.name', false, + ['zag', 'zig', 'zag']) + assert suffix == '[0]' + assert is_prefix + assert via == ['zig'] +} + +fn test_return_param_via_lists_each_function_once() { + assert ownership_return_param_via([], 'a') == ['a'] + assert ownership_return_param_via(['a'], 'b') == ['a', 'b'] + assert ownership_return_param_via(['a', 'b'], 'a') == ['a', 'b'] +} + +struct ReturnSourceCallSite { + caller string + callee string + arg_suffix string +} + +struct ReturnSource { + suffix string + is_prefix bool + via []string +} + +// return_sources_fixed_point runs the return alias fixed point of `sites` from base sources that +// return `.name` of the parameter, and returns the rounds it took with the sources per function. +fn return_sources_fixed_point(sites []ReturnSourceCallSite, fns []string) (int, map[string]map[string]ReturnSource) { + mut sources := map[string]map[string]ReturnSource{} + for f in fns { + sources[f] = { + '.name': ReturnSource{'.name', false, [f]} + } + } + mut rounds := 0 + for changed := true; changed; rounds++ { + assert rounds < 20 + changed = false + for site in sites { + for _, callee_source in sources[site.callee].clone() { + suffix, is_prefix, via := ownership_return_param_call_source(site.caller, + site.arg_suffix, callee_source.suffix, callee_source.is_prefix, callee_source.via) + key := suffix + if is_prefix { '*' } else { '' } + if key !in sources[site.caller] { + sources[site.caller][key] = ReturnSource{suffix, is_prefix, via} + changed = true + } + } + } + } + return rounds, sources +} + +// Mutual recursion through two fields each: `a(p)` returns `b(p.left)`, `b(p.right)` or `p.name`, +// and `b(p)` returns `a(p.left)`, `a(p.right)` or `p.name`. Without widening, the returned paths +// of the return fixed point grow forever. +fn test_return_param_call_sources_reach_fixed_point_for_mutual_recursion() { + rounds, sources := return_sources_fixed_point([ + ReturnSourceCallSite{'a', 'b', '.left'}, + ReturnSourceCallSite{'a', 'b', '.right'}, + ReturnSourceCallSite{'b', 'a', '.left'}, + ReturnSourceCallSite{'b', 'a', '.right'}, + ], ['a', 'b']) + assert rounds < 10 + assert sources['a'].len < 16 + assert sources['b'].len < 16 + // The paths that did not go around the cycle stay exact. + assert !sources['a']['.left.name'].is_prefix + assert !sources['a']['.right.name'].is_prefix + assert sources['a']['.left*'].is_prefix +} + +// `outer(p)` returns `inner(p.left)`, `inner(p)` returns `leaf(p.left)`, and `leaf(p)` returns +// `leaf(p.next)` or `p.name`: only the recursion of `leaf` is widened. +fn test_return_param_call_sources_widen_only_the_cycle() { + _, sources := return_sources_fixed_point([ + ReturnSourceCallSite{'outer', 'inner', '.left'}, + ReturnSourceCallSite{'inner', 'leaf', '.left'}, + ReturnSourceCallSite{'leaf', 'leaf', '.next'}, + ], ['leaf']) + assert sources['leaf'].keys().sorted() == ['.name', '.next*'] + assert sources['inner'].keys().sorted() == ['.left.name', '.left.next*'] + assert sources['outer'].keys().sorted() == ['.left.left.name', '.left.left.next*'] + assert sources['outer']['.left.left.name'].via == ['leaf', 'inner', 'outer'] +} diff --git a/vlib/v/types/checker_ownership_d_ownership.v b/vlib/v/types/checker_ownership_d_ownership.v index 1a489e52659471..8b085ca8730175 100644 --- a/vlib/v/types/checker_ownership_d_ownership.v +++ b/vlib/v/types/checker_ownership_d_ownership.v @@ -4,8 +4,6 @@ import time import v.flat import v.gen.c.naming -const ownership_unknown_pointer_index_alias = '' - enum OwnershipBorrowedProjectionAction { not_borrowed clone_value @@ -68,6 +66,13 @@ struct OwnershipReturnParamDescendant { slot_idx int source_suffix string target_suffix string + // source_is_prefix marks a source widened at a call cycle: the result aliases storage at + // or below `source_suffix`, but not a known exact path. See + // `ownership_return_param_call_source`. + source_is_prefix bool + // via lists the functions that composed `source_suffix`, starting with the one whose + // return expression named it, so that composing it again in one of them detects a cycle. + via []string } struct OwnershipReturnParamArg { @@ -543,6 +548,7 @@ fn ownership_return_param_desc_in(values []OwnershipReturnParamDescendant, needl // graphs instead of growing paths such as `.next.next...` without bound. fn ownership_return_param_desc_subsumes(existing OwnershipReturnParamDescendant, candidate OwnershipReturnParamDescendant) bool { return existing.param_idx == candidate.param_idx && existing.slot_idx == candidate.slot_idx + && (existing.source_is_prefix || !candidate.source_is_prefix) && ownership_storage_suffix_contains(existing.source_suffix, candidate.source_suffix) && ownership_storage_suffix_contains(existing.target_suffix, candidate.target_suffix) } @@ -3048,11 +3054,13 @@ fn (mut tc TypeChecker) ownership_prescan_add_return_param_descendant_from_expr( } for pi, pname in param_names { if source_name == pname { - tc.ownership_add_fn_return_param_descendant(fn_name, pi, slot_idx, '', target_suffix) + tc.ownership_add_fn_return_param_descendant(fn_name, pi, slot_idx, '', target_suffix, + false, [fn_name]) return true } if ownership_storage_key_is_descendant(source_name, pname) { - tc.ownership_add_fn_return_param_descendant(fn_name, pi, slot_idx, source_name[pname.len..], target_suffix) + tc.ownership_add_fn_return_param_descendant(fn_name, pi, slot_idx, source_name[pname.len..], + target_suffix, false, [fn_name]) return true } } @@ -3263,7 +3271,8 @@ fn (mut tc TypeChecker) ownership_prescan_return_param_sources(fn_name string, e continue } if ownership_storage_key_is_descendant(name, pname) { - tc.ownership_add_fn_return_param_descendant(fn_name, pi, slot_idx, name[pname.len..], '') + tc.ownership_add_fn_return_param_descendant(fn_name, pi, slot_idx, name[pname.len..], + '', false, [fn_name]) } } return @@ -3383,12 +3392,18 @@ fn (mut tc TypeChecker) ownership_prescan_add_return_param_descendant_from_call_ } for pi, pname in param_names { if arg_name == pname { - tc.ownership_add_fn_return_param_descendant(fn_name, pi, slot_idx, source.source_suffix, callee_desc.target_suffix) + // Passing the parameter itself does not grow the path, so it cannot diverge. + tc.ownership_add_fn_return_param_descendant(fn_name, pi, slot_idx, source.source_suffix, + callee_desc.target_suffix, callee_desc.source_is_prefix, ownership_return_param_via(callee_desc.via, + fn_name)) return } if ownership_storage_key_is_descendant(arg_name, pname) { - source_suffix := arg_name[pname.len..] + source.source_suffix - tc.ownership_add_fn_return_param_descendant(fn_name, pi, slot_idx, source_suffix, callee_desc.target_suffix) + source_suffix, source_is_prefix, via := ownership_return_param_call_source(fn_name, + arg_name[pname.len..], source.source_suffix, callee_desc.source_is_prefix, + callee_desc.via) + tc.ownership_add_fn_return_param_descendant(fn_name, pi, slot_idx, source_suffix, callee_desc.target_suffix, + source_is_prefix, via) return } } @@ -3474,9 +3489,9 @@ fn (mut tc TypeChecker) ownership_add_fn_return_descendant(fn_name string, slot_ st.ownership_fn_return_descs[fn_name] = descs } -fn (mut tc TypeChecker) ownership_add_fn_return_param_descendant(fn_name string, param_idx int, slot_idx int, source_suffix string, target_suffix string) { +fn (mut tc TypeChecker) ownership_add_fn_return_param_descendant(fn_name string, param_idx int, slot_idx int, source_suffix string, target_suffix string, source_is_prefix bool, via []string) { if fn_name == '' || param_idx < 0 || slot_idx < 0 - || (source_suffix == '' && target_suffix == '') { + || (source_suffix == '' && target_suffix == '' && !source_is_prefix) { return } mut st := tc.ownership_state() @@ -3484,10 +3499,12 @@ fn (mut tc TypeChecker) ownership_add_fn_return_param_descendant(fn_name string, []OwnershipReturnParamDescendant{} } candidate := OwnershipReturnParamDescendant{ - param_idx: param_idx - slot_idx: slot_idx - source_suffix: source_suffix - target_suffix: target_suffix + param_idx: param_idx + slot_idx: slot_idx + source_suffix: source_suffix + target_suffix: target_suffix + source_is_prefix: source_is_prefix + via: via } for desc in descs { if ownership_return_param_desc_subsumes(desc, candidate) { @@ -9770,6 +9787,12 @@ fn (mut tc TypeChecker) ownership_mark_from_return_param_descendant(target_name if target_name == '' || desc.param_idx < 0 { return false } + // A prefix source does not say which storage below `source_suffix` the result aliases, so + // it cannot hand the ownership of an exact source to the target. Leaving the target + // unowned is conservative: at worst the returned storage leaks, it is never dropped twice. + if desc.source_is_prefix { + return false + } source := tc.ownership_call_arg_for_return_param_source(call_id, node, desc.param_idx, desc.source_suffix) or { return false } arg_id := source.arg_id source_suffix := source.source_suffix @@ -10719,9 +10742,10 @@ pub fn (mut tc TypeChecker) ownership_call_result_sources(id flat.NodeId) []Owne source := tc.ownership_call_arg_for_return_param_source_info(node, info, desc.param_idx, desc.source_suffix) or { continue } slot_prefix := if is_multi_return { '[${desc.slot_idx}]' } else { '' } candidate := OwnershipCallResultSource{ - arg_id: source.arg_id - source_suffix: source.source_suffix - target_suffix: slot_prefix + desc.target_suffix + arg_id: source.arg_id + source_suffix: source.source_suffix + target_suffix: slot_prefix + desc.target_suffix + source_is_prefix: desc.source_is_prefix } if candidate !in result { result << candidate @@ -10849,28 +10873,6 @@ fn (tc &TypeChecker) ownership_rhs_borrows_indexed_storage(rhs_id flat.NodeId) b return ownership_alias_chain_borrows_indexed_storage(tc.ownership.pointer_index_aliases, rhs_name) } -fn ownership_alias_chain_borrows_indexed_storage(aliases map[string]string, rhs_name string) bool { - // Follow the complete alias chain (`arr -> val -> t[k]`). Cycles represent unresolved - // alias state, so treat them conservatively as borrowed storage. - mut cur := rhs_name - mut seen := map[string]bool{} - for { - if seen[cur] { - return true - } - seen[cur] = true - source := aliases[cur] or { return false } - if source == ownership_unknown_pointer_index_alias { - return true - } - if source.contains('[') { - return true - } - cur = source - } - return false -} - // ownership_rhs_may_borrow_storage reports whether a map assignment reads through an indexed // alias. Lowering clones the RHS whether that source slot is the same as or distinct from the // destination slot.