types: make ownership return-alias inference terminate on recursive fns - #28937
medvednikov wants to merge 3 commits into
Conversation
… terminates Return alias inference composes a callee's returned parameter path behind the argument projection (`f(p.next)` with `.name` gives `.next.name`). Through recursion that projection is prepended again every round (`.next.next.name`, ...). Subsumption only merges paths that extend a shallower one, so the fixed point never converged; recursion through several fields (for example `ownership_default_clone_missing_method_inner` over base/elem/key/value types) grew the path set exponentially, so every `-d ownership` build of `v.types` spun forever. Widen such paths to the argument projection, which already covers everything below it: always for direct recursion, and for mutual recursion once a composed path gets deeper than 4 projections. checker_ownership_alias_test.v used `-d ownership` only to see a pure helper. That define now selects the ownership compiler, which rejects the `types` module itself with thousands of move errors, so move the helper and its sentinel const into an always compiled file and drop the vflags line. Fixes #28923
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc7dd4728f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The previous commit widened a recursive return-param alias path to the
argument projection (`.next.name` to `.next`) and stored it as an ordinary
exact source. Consumers look sources up exactly, so a call site could take
ownership from the parent storage instead: in
`x := deepest(Holder{value: 'v', next: [Holder{value: s}]})` the `string`
result was marked as an owned `Holder`. The depth limit also truncated
acyclic call chains deeper than four projections.
Widened sources now carry `source_is_prefix`: the result aliases storage at
or below the path, but not a known one.
- A path is widened only for a direct recursive call, or when the callee's
returned path already contains the argument projection (a call cycle).
Acyclic chains keep their exact paths at any depth.
- Composing a prefix keeps it a prefix; an exact source never subsumes a
prefix one.
- Call-site ownership marking does not transfer ownership from a prefix
source, so the target stays unowned (at worst a leak, never a wrongly
typed or double drop).
- `ownership_call_result_sources` reports the flag, and the array map
lowering treats such a result as external when any path below the
source is.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e544101094
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The previous commit treated a composed return-param path as a call cycle when the callee's returned path already contained the argument projection. Acyclic chains can repeat a projection: when `inner(p)` returns `p.left.name` and `outer(p)` returns `inner(p.left)`, the exact `.left.left.name` was widened to the prefix `.left`, so `outer`'s result no longer received the ownership of the returned storage. Each return-param descendant now records `via`, the functions that composed its path. Composing a path in a function that is already in its `via` means that the path went around a call cycle, and only then is it widened to a prefix source at the argument projection. Direct recursion is the one-function case. Every other composition extends `via` by a new function, so exact paths stay bounded by simple call chains, and acyclic chains stay exact.
Summary
vlib/v/types/checker_ownership_alias_test.v(// vtest vflags: -d ownership) never finished compiling, so./v test vlib/v/types/hung. The time went into ownership return-alias inference over thev.typessources, not into the test itself.Return-alias inference composes a callee's returned parameter path behind the argument projection. With
f(p.next)and a callee that returns.name, the path becomes.next.name. Through recursion that projection is prepended again every fixed-point round (.next.next.name, ...). Subsumption only merges a path that extends a shallower one, so the set never converged.ownership_default_clone_missing_method_innerrecurses through four fields (base_type,elem_type,key_type,value_type), so the path set grew exponentially. A plain linked-list function (return f(n.next)/return n.name) also looped.Such paths are now widened to an explicit prefix source (
OwnershipReturnParamDescendant.source_is_prefix): the result aliases storage at or below the argument projection, but not a known exact path.via, the functions that composed its path. A path is widened only when it is composed again in a function already in itsvia, i.e. when it went around a call cycle; direct recursion is the one-function case. Every other composition extendsviaby a new function, so exact paths stay bounded by simple call chains, and acyclic chains stay exact at any depth, even when they repeat a projection (outer(p)returninginner(p.left), whereinner(p)returnsp.left.name, stays.left.left.name).ownership_call_result_sourcesreports the flag. Thearray.maplowering treats such a result as external when the covering path, or any path below it, is.checker_ownership_alias_test.vonly needed-d ownershipto see one pure helper,ownership_alias_chain_borrows_indexed_storage. With the hang gone, that define selects the ownership compiler, which rejects thetypesmodule itself with ~5.6k move errors. So the helper and its sentinel const move into an always compiledchecker_ownership_alias.v, together with the new path-composition helpers, and the test drops itsvflagsline.Fixes #28923
Related: #28678 describes the same hang.
module typestests built with-d ownershipnow finish, but they then stop at the move errors above, so that issue is left open.Validation
All runs use
V_MACOS_V3_NO_FALLBACK=1.vlib/v/tests/ownership_recursive_return_param_alias_test.vcovers direct and mutual recursion under-d ownership. It passes, and still hangs on master after 60s.checker_ownership_alias_test.v:outer/innerchain;-d ownershipbuild of amodule typestest finishes in ~20s (4 return fixed-point rounds); it never finished on master.-ownershipchecks of small programs:outer/innerchain that returns an owned string from a nested literal argument still reportsuse of moved valuefor the result, with typestring;stringresult as an ownedHolder../v -silent test vlib/v/types/: 31/31 passed. It never finished on master../v -silent test vlib/v/transform/: 17/17 passed./v -silent test vlib/v/gen/c/: 27/27 passed (checked before the provenance commit, which does not touch cgen or the transform)./v -silent vlib/v/compiler_errors_test.v: 1721 passed, 0 failedvlibtests whosevflagsuse ownership or autofree, excluding the valgrind ones and including the new test: 15/15 passedvlib/v/compiler_tests/ownership/ownership_test.vbuildsvlib/v/v.vwith-d ownership, which doesn't build on master or here. A copy run against ownership compilers built from master and from this branch fails the same 5 of 28 test fns on both. This was checked on the first commit, not re-run after the follow-up.