perf(d5): solve MOS by SCC-condensed summary fixpoint, drop the depth cap#151
Conversation
… cap
The MOS solver resolved each method's summary by a memo-less recursive
descent over the call graph, capped at depth 3. The cap existed only to
dodge the exponential that re-descent hits on diamond-shaped call graphs
(A forwards a param down two paths that reconverge on a shared callee),
and it bought that safety with two costs: false `unknown`s on forward
chains deeper than the cap, and a less-precise `may` on recursion (the
descent broke each cycle at a spurious `no`, which a later join turned a
provable `must` into `may`).
Replace it with the textbook interprocedural summary fixpoint:
* condense the call graph into strongly-connected components (iterative
Tarjan, emitted bottom-up = reverse-topological);
* process components bottom-up — a callee's summary is final before any
caller reads it, so each method is resolved once and reused (the
context-insensitive summary IS the memoization);
* within a recursive component, seed each edge at the lattice bottom
(⊥, "no evidence yet") and iterate the four-point transfer lattice to
its least fixpoint, mapping any residual ⊥ to `no` at finalization;
* resolve forward-returns by a memoized, cycle-safe chase.
This removes the exponential without the cap, and is strictly more precise
on recursion: a method that disposes on its base case and recurses
otherwise now resolves to `must` (every terminating path disposes), and
mutual recursion grounded by a dispose deep in the cycle carries that
`must` across the whole SCC. Mutual recursion that never disposes still
settles at `no` and still terminates.
The only residual `unknown` is an extern (unsummarized) forward boundary;
`solve_with_log` now surfaces exactly those (sorted, deterministic) in
place of the old cap log — no silent truncation.
Public API unchanged in shape: solve() / solve_with_log() keep their
return types; the now-meaningless `cap` keyword is dropped (the sole
caller, the ownir bridge, never passed it). Tests that asserted the old
depth-broken behaviour (chain past cap -> unknown; self-recursion -> may)
are updated to the precise fixpoint results, plus new diamond, deep-chain,
extern-log, and grounded-mutual-recursion cases.
Full suite green (ownership 40/40, ownir bridge 194/194, mypy --strict,
ruff). No existing bridge/corpus diagnostic shifts.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe ownership solver now computes summaries with SCC-based fixpoint iteration instead of a depth cap, resolves returns through cycle-safe forwarding, and updates tests for deep chains, extern boundaries, and recursive convergence. ChangesOwnership solver fixpoint
🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://cdn.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 32600629be
ℹ️ 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".
| elif r.callee in visiting: | ||
| v = "unknown" # return-forward cycle: no ground to stand on | ||
| else: | ||
| inner = resolve_return(r.callee, visiting | {key}) |
There was a problem hiding this comment.
For a long but acyclic chain of ReturnSkeleton("forward") wrappers, this still recurses once per hop after the depth cap was removed, so around Python's default recursion limit solve_with_log() raises RecursionError instead of producing a summary. The bridge calls solve(...) for all functions and catches exceptions by dropping to an empty MOS, so a deep wrapper chain can disable interprocedural ownership summaries for the whole input rather than just resolving that return chain.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in e2bb4ca. resolve_return was the one part of the solver still using Python recursion (the SCC pass and param fixpoint are iterative), so with the cap gone a deep acyclic forward-return chain hit the recursion limit, and the bridge's except Exception: mos = {} would have turned that into an empty MOS for the whole input. Since a return has at most one forward target, resolution is a linked-list walk — now done iteratively (chase to terminal/extern/cycle, then propagate back up with the same aliasOf-through-forward degrade). Behaviour is identical to the recursive version; it just no longer grows the stack. Added N=3000 deep-chain regression tests (past the default recursion limit) for both the return and param chains.
Generated by Claude Code
The SCC pass and the param fixpoint are iterative, but resolve_return was
left recursive — one Python frame per forward-return hop. With the depth
cap gone, a long but acyclic chain of `ReturnSkeleton("forward")` wrappers
recurses to Python's default limit and raises RecursionError. The bridge
catches a solve() exception by dropping to an EMPTY MOS, so one deep
wrapper chain would disable interprocedural ownership summaries for the
entire input rather than just that one return chain. (Codex P2.)
A return has at most one forward target, so resolution is a linked-list
walk: chase the forward edges to a terminal / extern / cycle, then
propagate the value back up applying the aliasOf-through-forward degrade
at each hop. Behaviour is identical to the recursive version (same memo,
same cycle -> unknown, same aliasOf rule); it just no longer grows the
call stack.
Adds deep-chain (N=3000, past the recursion limit) regression tests for
both the forward-return and forward-param chains.
ownership 42/42, ownir bridge 194/194, mypy --strict, ruff — all green.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
![coderabbitai[bot]](https://cdn.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_ownership.py (1)
137-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a forward-return extern log assertion.
This new coverage only exercises unresolved logging for parameter forwarding.
solve_with_log()now has a separateresolve_return()path that also logs extern boundaries, so a regression there would currently slip through.🧪 Suggested test addition
_, log2 = solve_with_log([_m("Caller", _p(0, PathAction("forward", "Extern", 0)))]) expect(any("Extern#0" in e for e in log2), "extern forward is logged, not silent") + + s3, log3 = solve_with_log([ + _m("Caller", ret=ReturnSkeleton("forward", callee="Extern")), + ]) + expect(s3["Caller"].returns == "unknown", "forward-return to extern -> unknown") + expect(any("return Extern" in e for e in log3), "extern forward-return is logged")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_ownership.py` around lines 137 - 140, Add a test assertion in test_ownership around solve_with_log to cover the resolve_return() path for extern boundaries, not just PathAction("forward"). Use the existing logging helpers and symbols like solve_with_log, resolve_return, and Extern#0 to verify that a forward-return extern is logged when resolution fails, so regressions in return-boundary logging are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@ownlang/ownership.py`:
- Around line 241-249: The docstring for solve_with_log overstates that
extern-boundary forwards are the only source of unknown results, but
resolve_return and lookup also produce unknown in other cases. Update the
contract in solve_with_log and related helpers like resolve_return and lookup to
either log the residual unknown-producing cases (unrecognized return kind,
return-forward cycle, aliasOf remap, missing param index) or soften the wording
so it no longer promises complete coverage.
---
Nitpick comments:
In `@tests/test_ownership.py`:
- Around line 137-140: Add a test assertion in test_ownership around
solve_with_log to cover the resolve_return() path for extern boundaries, not
just PathAction("forward"). Use the existing logging helpers and symbols like
solve_with_log, resolve_return, and Extern#0 to verify that a forward-return
extern is logged when resolution fails, so regressions in return-boundary
logging are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3af4708b-83b6-40b1-8fbd-517bb0905b9e
📒 Files selected for processing (2)
ownlang/ownership.pytests/test_ownership.py
…ndary CodeRabbit review: the solve_with_log / module docstrings claimed the extern boundary is the ONLY source of `unknown`. It isn't — resolve_return also yields `unknown` (without logging) for a return-forward cycle, an un-remappable aliasOf, and an unrecognised return kind, and lookup does for a missing param index. Those are intrinsic, precision-safe, deterministic-from-input degradations, not "gave up on something external", so they are intentionally not logged. Soften the wording: the log covers extern (outside-the-analyzed-set) boundaries, not every `unknown`. Also add a forward-return extern-boundary log assertion — the return chase is a separate code path from the param-forward case the existing test covered, so a regression in return-boundary logging would otherwise slip through. Docs/test only — no behaviour change. ownership 44/44, ownir 194/194, mypy --strict, ruff all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
2 participants
Что и зачем
Решатель Method Ownership Summary (
ownlang/ownership.py) считал сводки memo-less рекурсивным спуском по графу вызовов, ограниченным глубиной 3. Cap существовал только чтобы обойти экспоненту, которую такой спуск ловит на diamond-графах (параметр форвардится двумя путями, сходящимися на общем callee), и платил за это двумя потерями точности: ложныеunknownна цепочках глубже cap (= пропущенные течки) и более слабыйmayна рекурсии (спуск рвал цикл фальшивымno, и join превращал доказуемыйmustвmay).Заменяю на каноничный межпроцедурный summary-фикспойнт: свёртка графа в компоненты сильной связности (итеративный Тарьян, выдаются снизу вверх), обработка компонент bottom-up — сводка callee финализируется до прочтения любым caller, поэтому каждый метод резолвится один раз (контекстно-независимая сводка и есть мемоизация); внутри рекурсивной компоненты рёбра сидятся в дне решётки (⊥, «нет улик»), а не в
no, и итерируются до наименьшей неподвижной точки. Экспонента убрана без cap, а точность на рекурсии строго выше. Единственный остаточныйunknown— extern-граница;solve_with_logпечатает их (сортированно, детерминированно) вместо старого cap-лога.Тип изменения
unknown= пропущенные течки; имprecisemayна рекурсии). Также perf: экспонента → линейность.Как проверено
python tests/run_tests.py— полный прогон зелёный (ownership 40/40, ownir bridge 194/194, и остальные сюиты); ни один существующий диагноз не сдвинулсяruff check .иmypy(--strict, 17 файлов) — чистоownlang/ownership.py, покрытtests/test_ownership.py)Тесты, кодировавшие старое депт-брейк-поведение (цепочка за cap → unknown; самоrekursия + базовый dispose → may), обновлены на точные результаты фикспойнта. Добавлены кейсы: diamond, глубокая цепочка без cap, extern-лог, взаимная рекурсия, заземлённая
dispose.Связанные issue
Нет issue для закрытия. Refs: дизайн-нота P-005 D5 (
docs/notes/d5-ownership-transfer.md) — реализует §4 (SCC-фикспойнт) и снимает cap-обусловленные FN. Open question §10 q2 (collision-free signature key для перегрузок) остаётся отдельной задачей.Чеклист
tests/test_ownership.py: 40/40, +3 новых кейса)perf(d5): …)🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
unknownresults on fully traceable paths.Tests