docs(P-022): revise per post-merge review — own-bridge, IDE trigger, lint/perf collisions#168
Conversation
…collisions Folds the accepted findings from docs/notes/p022-review-notes.md (PR #167): - own-bridge crate (highest cost): today's ownir.py beyond the schema (~2k lines of verdict-determining logic — facts->core lowering, the interprocedural MOS inference, BCL fresh-factory table, check_facts verdict mapping) had no named home in the DAG. Added the crate, its edges (ir+cfg+analysis+diagnostics -> bridge; only own-cli may depend on it), a migration step 6 with the normative-semantics prerequisite, and the fitness rule. - IDE extension as the recorded Why (Gate B of incremental-computation): reorders perf priorities to latency/incrementality over batch (the 10-100x figures are batch-CLI numbers), makes panic=abort per-binary (LSP needs unwind — salsa cancels via unwinding), promotes error-tolerant parsing to a requirement, and documents the hybrid extension shape (in-process Roslyn extraction + Rust core over the OwnIR seam). salsa open question upgraded from maybe to Gate-B substance. - indexing_slicing deny vs dense-Vec state: pre-resolved — justified module-scoped allow on the arena-indexed state container, deny stays elsewhere. - Cheap residuals: own-ir relabeled fact contract (verdicts live in own-diagnostics), span primitives pinned to the leaf (not own-syntax), byte-offsets-internally row, NonZeroU32 IDs, [profile.release] as TOML, oracle diffs golden snapshots keyed by (corpus hash, py commit) instead of live Python per CI pass, error-text parity fixtures, #[cold] on diagnostic paths, SmallVec measured-not-sprinkled, enum-size ritual.
|
@coderabbitai review This folds the accepted findings of
Generated by Claude Code |
|
Warning Review limit reached
Next review available in: 53 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR revises the P-022 Rust core migration proposal document, expanding sections on migration rationale, crate topology and dependency edges, ID/index design patterns, parser and library requirements, performance/panic-mode guidance, compiler strictness tooling, and the differential oracle and migration strategy for ChangesProposal document revision
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
cd /tmp
rm -rf repo
git clone --depth 50 https://github.com/PhysShell/Own.NET.git repo 2>&1 | tail -5
cd repo
gh pr view 168 --repo PhysShell/Own.NET --json title,body,files -q '.files[].path' 2>&1 || true
echo "---diff stat---"
gh pr diff 168 --repo PhysShell/Own.NET 2>&1 | head -20Length of output: 1857 🏁 Script executed: #!/bin/bash
set -uo pipefail
cd /tmp/repo
echo "=== FULL DIFF ==="
gh pr diff 168 --repo PhysShell/Own.NET > /tmp/pr168.diff 2>&1
wc -l /tmp/pr168.diff
cat /tmp/pr168.diff
echo "=== REVIEW NOTES ==="
find . -iname 'p022-review-notes.md'
cat docs/notes/p022-review-notes.md 2>/dev/null | head -300Length of output: 29557 🏁 Script executed: #!/bin/bash
set -uo pipefail
cd /tmp/repo
fd -i ownir.py
echo "---"
fd -i ownir.py -x wc -l {}
echo "--- look at schema vs beyond-schema split ---"
fd -i ownir.py -x grep -n "^class\|^def" {} 2>/dev/null | head -80Length of output: 3448
1.
|
CodeRabbit cosmetic: '{all of the above}' after the own-bridge block
implied but didn't spell out that own-cli is the one crate allowed to
depend on the bridge — now it does. (The other note, the ~900 vs ~2000
line-count drift vs p022-review-notes.md, resolves in favor of this doc:
the measured to_module..check_facts span is ~1.7-1.9k lines, so ~2k is
the accurate figure; the notes stay untouched as a historical record.)
|
Thanks — both cosmetic notes handled in
The Generated by Claude Code |
![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: 59c7509d83
ℹ️ 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".
| So the VS/VS Code extension hosts the existing C# extractor and talks to the Rust | ||
| core over the **OwnIR seam** (stdio or FFI) — the very seam this proposal freezes. |
There was a problem hiding this comment.
With this wording, the C# editor path may use FFI, but the fitness function later says nothing except own-cli may depend on own-bridge. In the FFI scenario there must be a cdylib/API crate or LSP server that calls the facts→verdict bridge; otherwise the proposal only permits stdio through the CLI. Please either drop FFI here or add that API crate to the allowed-edge exception so the architecture test doesn't forbid the stated extension shape.
Useful? React with 👍 / 👎.
| # own-diagnostics depends only on own-ir/own-syntax spans — never on the solver. | ||
| # own-diagnostics depends only on the span/location leaf — never on the solver. | ||
|
|
||
| own-ir + own-cfg + own-analysis + own-diagnostics ─▶ own-bridge |
There was a problem hiding this comment.
own-bridge is described as doing facts→core-AST lowering, and this document puts the AST in own-syntax while own-cfg only lowers AST→CFG. If the allowed graph omits own-syntax, the bridge cannot construct the AST without an undeclared dependency or a leaky re-export through own-cfg, so the first edge-set test will encode the wrong boundary. Add own-syntax to this wide consumer, or change the bridge to lower directly to CFG.
Useful? React with 👍 / 👎.
| depends on **it** to construct verdicts. Depends only on the span/location leaf | ||
| (`own-ir`) — **not** on `own-syntax`, or the presentation crate drags in the | ||
| parser. Mirrors today's clean split (`evidence.py` is a pure projection). |
There was a problem hiding this comment.
This bullet now makes own-diagnostics depend only on the span leaf in own-ir, but the topology explanation just above still says the same crate depends on own-ir/own-syntax. That leaves two conflicting allowed-edge sets for the presentation crate and could accidentally permit the parser dependency this paragraph is trying to forbid. Update the earlier prose to name only the span leaf.
Useful? React with 👍 / 👎.
| core on every CI pass: commit its outputs as **golden snapshots keyed by | ||
| (corpus hash, Python-core commit)** and diff Rust against the snapshots; regenerate | ||
| only when Python itself changes (already the exception, not the rule). Otherwise CI |
There was a problem hiding this comment.
These snapshots are keyed by (corpus hash, Python-core commit), but this sentence only allows regeneration when Python changes. When a fixture/corpus file is added or edited during the ratchet, the Python commit can be identical while the expected outputs must be regenerated for the new corpus hash; otherwise CI either diffs Rust against stale outputs or cannot cover the new input. Make the regeneration rule trigger on either key changing.
Useful? React with 👍 / 👎.
…r, snapshot keys Four findings, all confirmed doc-consistency defects: - own-syntax added to own-bridge's edge set (P2): the bridge lowers facts to core-AST nodes, and the AST lives in own-syntax — without the edge the first cargo-metadata test would encode a boundary the bridge cannot satisfy (undeclared dep or a leaky re-export through own-cfg). Diagram, crate bullet, and fitness rule all updated. - Stale 'own-ir/own-syntax' in the topology prose (P3): own-diagnostics depends only on the span leaf in own-ir; the earlier paragraph still allowed the parser edge the bullet forbids. Now names only the leaf. - FFI vs only-own-cli (P2): the Why permits stdio OR FFI, but the fitness rule only allowed own-cli to depend on the bridge — which would forbid the stated extension shape. Rule now reads 'only entry-point crates' (own-cli today; future own-lsp / own-capi cdylib), never a core crate. - Snapshot regeneration on either key (P3): (corpus hash, py commit) keyed snapshots regenerate on a corpus/fixture change too, not only a Python change — otherwise CI diffs stale outputs or skips new inputs.
|
@codex all four folded in
Generated by Claude Code |
Summary
Testing
No commit or PR was created because the requested fixes were already present and the working tree had no changes. |
2 participants
Что и зачем
Ревизия P-022 по принятым находкам пост-мёрж ревью (
docs/notes/p022-review-notes.md, PR #167), в порядке цены поздней находки:own-bridge— недостающий крейт (highest cost). ~2k строк verdict-determining логики изownir.pyза пределами схемы (facts→core loweringto_module/_lower_flow, интерпроцедурный MOS-вывод_build_skeletons/_infer_*, BCL fresh-factory таблица,check_facts) не имели дома в DAG. Добавлен крейт, рёбра (ir+cfg+analysis+diagnostics → bridge; зависеть от него может толькоown-cli), шаг миграции 6 с пререквизитом «нормативная семантика вывода до порта» и fitness-правило.incremental-computation.md): перф-приоритеты — latency/incrementality, а не batch (10–100× — batch-CLI цифры);panic=abort— per-binary (LSP нуженunwind: salsa отменяет через unwinding); error-tolerant парсинг — требование; гибридная форма расширения (in-process Roslyn-экстракция + Rust-ядро через OwnIR-seam); salsa из «когда-нибудь» в суть Gate B.indexing_slicing = denyс dense-Vecстейтом — предрешена в доке: justified module-scoped#[allow]на arena-индексированном контейнере, deny везде ещё.own-syntax); byte-offsets внутри + line-index;NonZeroU32-IDs (niche);[profile.release]как TOML; оракул диффит golden-снапшоты (corpus hash + py commit), не живой Python на каждый CI; error-text parity fixtures;#[cold]на diagnostic-путях; SmallVec «measured, not sprinkled»; enum-size ритуал.Тип изменения
Как проверено
python tests/run_tests.pyruff check .иmypyСвязанные issue
Нет issue. Follow-up к #166 (P-022) по ревью из #167 (
p022-review-notes.md).Чеклист
docs:)🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit