feat(d5): merge same-name overloads in MOS + sync design note#152
Conversation
The extractor names a call's callee `{Type}.{Method}` with no parameter
signature, so two overloads (e.g. C.M(Stream) / C.M(string)) collide on the
same MOS key and a forward to that name cannot pick an overload. The bridge
previously dropped every non-unique name from the skeleton set, so an
overloaded method got NO interprocedural summary at all — a forward to it
stayed `unknown` (a missed leak) and an overloaded factory was never seen as
fresh (note's open question §10 q2).
Merge same-name overloads into ONE conservative summary instead of dropping
(`_merge_skeletons`): a lattice join at (name, parameter-index) granularity —
a parameter transfers `must` only when EVERY overload carrying that index
consumes it (an overload that merely keeps the index contributes a `borrow`
path, so the join can never fabricate a `must`), and the return is the shared
kind only when all overloads agree (else `unknown`, never a fabricated
`fresh`). Sound and precision-safe; strictly better than dropping. Arg-type /
per-arity disambiguation would need call-site type info the fact stream does
not carry — documented as the residual.
Two bridge regression tests: agreeing overloads resolve a forward to consume
(caller OWN002, previously missed); disagreeing overloads join to `may` so the
caller stays silent (no fabricated must). ownir 196/196, mypy --strict, ruff.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
The note's §4 described a worklist + depth-cap-3 pass; the shipped solver is an SCC-condensed summary fixpoint with no cap (the condensation removed both the exponential the cap guarded and the cap-induced false `unknown`s). Rewrite §4 to the real algorithm (iterative Tarjan, context-insensitive bottom-up resolution, ⊥-seeded least fixpoint for params, iterative cycle-safe return chase, extern- only logged residual). Update the D5.0 test list, the §5 cap bullet, and the factory-overload line to match, and mark §10 q2 (overload signature key) as partially addressed by the conservative same-name merge. Docs only. 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 PR updates overload handling for first-party ownership summaries, adds regression tests for agreeing and disagreeing overload cases, and revises the D5 ownership-transfer note to match the SCC-based solver and no-depth-cap behavior. ChangesOverload-aware ownership summaries
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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://avatars.githubusercontent.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: 947217d61b
ℹ️ 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".
| by_key.setdefault(key, []).append(MethodSkeleton(key, tuple(params), ret)) | ||
| # one skeleton per name: solve() keys by name (a forward names its callee with no | ||
| # signature), so same-name overloads are joined into a single conservative summary. | ||
| return [_merge_skeletons(group) for group in by_key.values()] |
There was a problem hiding this comment.
Merging same-name overloads makes mos.get(callee) non-null, so _lower_flow now emits direct Calls to overloaded names, but the core signature table still stores only one function per name and overwrites duplicates with whichever overload appears last. When overloads disagree (for example one borrow-only overload and one consuming overload) and the consuming definition is last, acquire s; C.N(s); release s is reported as OWN002 even though the merged MOS for that parameter is may and should stay silent for an ambiguous overload name; direct calls need to use the merged effects or remain unlowered until overload resolution is available.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in b595a52 — good catch, this was a real regression the merge introduced (reproduced: pre-merge the name was absent from the MOS so no Call was emitted and the case stayed silent; post-merge the last-wins core signature consumed s, and it was order-dependent on which overload was defined last). Rather than leave it unlowered, a direct call to an overloaded name now routes each arg through the merged MOS contract via the $consume/$borrow channel — same resolution as a forward to that name (must→consume, no→borrow, may/unknown→plain). So disagreeing overloads join to may → plain → silent, while agreeing-consume overloads still apply consume (a genuine use/double-dispose is still caught). Added 4 regression tests (direct disagreeing → silent, direct agreeing → OWN002, and the return-side factory both ways).
Generated by Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
docs/notes/d5-ownership-transfer.md (1)
471-483: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSync the
MethodSkeleton.keycontract with this note.This section now correctly describes the current name-only merge as a partial answer, but
ownlang/ownership.py:112-117still documentsMethodSkeleton.keyas a stable, collision-free signature key. Updating that code-side comment too would avoid two different contracts for the same field.🤖 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 `@docs/notes/d5-ownership-transfer.md` around lines 471 - 483, Update the documentation for MethodSkeleton.key in ownlang/ownership.py so it matches the current note: it should no longer claim to be a stable, collision-free signature key. Keep the comment consistent with the name-only merge behavior described by _merge_skeletons in ownir.py, and clarify that overloads are currently merged conservatively rather than reconstructed via a full signature key.tests/test_ownir.py (1)
1344-1382: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a return-side overload regression.
These cases pin the parameter-transfer join, but
_merge_returns()is also new and this PR explicitly restores overloaded-factoryfreshresolution. A small agreeing/disagreeing factory-overload case here would lock that path down too.🤖 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_ownir.py` around lines 1344 - 1382, Add a regression test for return-side overload merging in the same test area as the existing parameter-transfer overload cases. Use the existing check_facts harness to cover both an agreeing and a disagreeing overloaded factory/return path so `_merge_returns()` is exercised, and verify that the overloaded `fresh` resolution behaves correctly when calls target the shared name. Reference the nearby `ov_fwd`, `ov_use`, and overload-name patterns to keep the new case aligned with the current overload join tests.
🤖 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.
Nitpick comments:
In `@docs/notes/d5-ownership-transfer.md`:
- Around line 471-483: Update the documentation for MethodSkeleton.key in
ownlang/ownership.py so it matches the current note: it should no longer claim
to be a stable, collision-free signature key. Keep the comment consistent with
the name-only merge behavior described by _merge_skeletons in ownir.py, and
clarify that overloads are currently merged conservatively rather than
reconstructed via a full signature key.
In `@tests/test_ownir.py`:
- Around line 1344-1382: Add a regression test for return-side overload merging
in the same test area as the existing parameter-transfer overload cases. Use the
existing check_facts harness to cover both an agreeing and a disagreeing
overloaded factory/return path so `_merge_returns()` is exercised, and verify
that the overloaded `fresh` resolution behaves correctly when calls target the
shared name. Reference the nearby `ov_fwd`, `ov_use`, and overload-name patterns
to keep the new case aligned with the current overload join tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b5c1b414-543b-4d12-b367-139188044dcc
📒 Files selected for processing (3)
docs/notes/d5-ownership-transfer.mdownlang/ownir.pytests/test_ownir.py
…t last-wins Merging same-name overloads (prev commit) put overloaded names into the MOS, which made the call-site lowering emit a resolvable core `Call` for them. But the core's signature table keeps only ONE same-name FnDecl (last-wins), so `lower_call` mis-applied whichever overload was defined last: with disagreeing overloads (one consuming, one borrow-only) and the consuming one last, `acquire s; C.N(s); release s` was a false OWN002 — and order-dependent. (Codex P2; this was a regression introduced by the merge — pre-merge the name was absent from the MOS so no `Call` was emitted and the case stayed silent.) Route a DIRECT call to an overloaded name through the merged MOS contract via the `$consume`/`$borrow` channel instead of the last-wins signature — exactly as a FORWARD to that name resolves (`must`→consume, `no`→borrow, `may`/`unknown`→plain). Disagreeing overloads join to `may` → plain → silent; agreeing-consume overloads still apply `consume` (a real use/double-dispose is still caught). Four bridge regression tests: direct disagreeing (silent), direct agreeing-consume (OWN002), and return-side overloaded factory — agreeing-fresh leaks (OWN001), disagreeing makes no fresh claim (silent). Also sync the `MethodSkeleton.key` docstring to the name-only / merge reality (CodeRabbit). ownir 200/200, ownership 44/44, mypy --strict, ruff — all green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019846YSZ35c7CdkWQ1qX5gm
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/ownir.py`:
- Line 884: Normalize overload handling in the merged MOS contract path by using
the same canonical method key that first_party() produces instead of raw
spellings. Update the overload detection and mos.get(callee) lookup in ownir.py
so qualified/direct calls to canonical-equivalent first-party overloads still
hit the merged contract branch, or introduce a raw-to-canonical alias map used
by both the Counter-based overload set and summary lookup. Add a regression
around the overloaded consume flow to cover qualified overload calls.
🪄 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: 7e96b8df-4b5d-44da-94ac-488bd814b342
📒 Files selected for processing (3)
ownlang/ownership.pyownlang/ownir.pytests/test_ownir.py
✅ Files skipped from review due to trivial changes (1)
- ownlang/ownership.py
The overload merge keyed `overloaded` and the call-site summary lookup on RAW method spellings, while `first_party` canonicalizes (strips `global::`). So a `global::`-qualified direct call to an overloaded method missed the merged-contract channel branch and fell through to the raw `mos.get` (also unqualified) → no effect emitted → the merged consume handoff was silently dropped (a missed double-discharge / use-after-consume). (CodeRabbit, Major.) Build `overloaded` from canonical names and resolve the channel branch's summary with the same bare-key fallback `_callee_returns_fresh` uses. The non-overloaded direct-`Call` path stays on the raw `summ` lookup deliberately — it names the callee in the emitted Call, which must match the core signature table (a qualified name there would raise OWN040); the channel emits only `$consume`/`$borrow` sink externs, so a qualified callee is safe through it. Regression: a `global::`-qualified call to agreeing-consume overloads now applies the merged consume (OWN002 on a later release), where raw-key matching stayed silent. ownir 201/201, ownership 44/44, 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
Что и зачем
Follow-up к #151. Экстрактор именует callee как
{Type}.{Method}без сигнатуры параметров, поэтому одноимённые перегрузки коллидируют по ключу MOS, и forward к такому имени не может выбрать перегрузку. Раньше мост дропал любое неуникальное имя целиком — перегруженный метод не получал межпроцедурной сводки вообще: forward к нему оставалсяunknown(пропущенная течка), а перегруженная фабрика никогда не считаласьfresh(open question §10 q2 дизайн-ноты).Вместо дропа — консервативный merge одноимённых перегрузок в одну сводку (
_merge_skeletons): join по решётке на гранулярности (имя, индекс параметра). Параметр получаетmustтолько если каждая перегрузка с этим индексом его потребляет (перегрузка, которая просто держит индекс, вноситborrow-путь → join никогда не выдумываетmust); return — общий вид только при согласии всех перегрузок, иначеunknown(никакого сфабрикованногоfresh). Звуково и precision-safe, строго лучше дропа. Плюс синхронизация дизайн-нотыd5-ownership-transfer.mdпод реально отгруженный SCC-фикспойнт-решатель (§4 описывал worklist + cap-3, которых в коде нет).Тип изменения
Как проверено
python tests/run_tests.py— зелёный (ownir bridge 196/196 с 2 новыми overload-тестами, ownership 44/44, остальные сюиты)ruff check .иmypy(--strict, 17 файлов) — чистоownlang/ownir.py, покрытыtests/test_ownir.py)Два регресс-теста: согласные перегрузки резолвят forward в consume (caller OWN002 — раньше пропускалось); несогласные перегрузки join'ятся в
may, и caller остаётся молчаливым (никакого ложного must).Связанные issue
Нет issue для закрытия. Refs: #151 (вводит SCC-фикспойнт-решатель), дизайн-нота P-005 D5 §10 q2 (сигнатурный ключ перегрузок — теперь частично закрыт консервативным merge'ем; остаток — разрешение по типу/арности требует инфы с call-site, которой в fact-потоке нет).
Чеклист
tests/test_ownir.py: +2 overload-кейса)feat(d5):,docs(d5):)🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
global::).