Skip to content

docs(proposal): P-022 Rust core migration — bird's-eye architecture#166

Merged
PhysShell merged 9 commits into
mainfrom
claude/ownnet-execution-surfaces-adr-8kyc0n
Jul 3, 2026
Merged

docs(proposal): P-022 Rust core migration — bird's-eye architecture#166
PhysShell merged 9 commits into
mainfrom
claude/ownnet-execution-surfaces-adr-8kyc0n

Conversation

@PhysShell

@PhysShell PhysShell commented Jul 3, 2026

Copy link
Copy Markdown
Owner

Что и зачем

Design-only proposal (bird's-eye) по переезду Python-ядра на Rust-workspace, где Python-репо остаётся дифференциальным оракулом / golden-тестом. Кода на Rust пока нет — Python авторитетен, пока не сойдётся паритет. Документ отвечает на конкретные вопросы: крейт-топология (enforced acyclic DAG, decoupled по OwnIR-seam), Rust-идиомы против текущей accidental complexity, библиотеки-кандидаты, prior art, Rust architecture-fitness тулинг, и дизайн дифф-оракула поверх уже замороженных evidence/SARIF/JSON контрактов.

Это брейншторм-PR (draft): зову CodeRabbit и Codex специально ради архитектурного фидбека, а не как gate. Особенно интересуют:

  • крейт-граница own-analysis vs own-diagnostics/own-ir — не течёт ли связность;
  • persistent state (imbl/rpds) vs arena+CoW для dataflow — что бенчить;
  • парсер: hand-roll vs chumsky/winnow для golden-parity;
  • дифф-оракул: сравнение по SARIF-контракту через существующий scripts/oracle_compare.py — дыры в подходе?
  • monorepo (rust/ subtree) vs sibling-repo.

Тип изменения

  • docs — документация
  • feat / fix / refactor / chore / test / ci

Как проверено

  • docs-only; кода нет. Проверять нечего — Python-ядро и гейт не затронуты.
  • python tests/run_tests.py
  • ruff check . и mypy

Связанные issue

Нет связанного issue. Follow-up к execution-surfaces ADR (#164, #165) — Rust-переезд как триггер из §8.

Чеклист

  • изменение покрыто тестом/селфтестом (или объяснено, почему нет) — docs-only, тестировать нечего
  • README/docs обновлены — добавлен P-022, обновлён индекс docs/proposals/README.md
  • коммиты в conventional-commit стиле (docs:)

🤖 Generated with Claude Code


Generated by Claude Code

Summary by CodeRabbit

  • Documentation
    • Added a new note outlining a gradual “ratchet” approach to static analysis strictness across the codebase.
    • Added a design proposal describing a Rust-based core migration architecture, migration strategy, and validation approach.
    • Updated the proposals index to include the new Rust core migration entry.

Design-only proposal for porting the Python core to a Rust workspace,
keeping the Python repo as the differential oracle / golden test. Covers:
crate topology as an enforced acyclic DAG (decoupled across the OwnIR
seam), Rust idioms that remove the current accidental complexity
(enum+match vs assert_never, newtype IDs vs id(sym), persistent state vs
deep copy, Lattice/Analysis traits vs the interleaved _Analyzer), library
candidates, prior art to study (rust-analyzer, ruff, rustc_mir_dataflow,
Polonius, ...), Rust architecture-fitness tooling (crate-graph fitness
test, cargo-modules/deny/machete/public-api), and the differential-oracle
design that rides the already-frozen evidence/SARIF/JSON contracts. No
Rust code yet; Python stays authoritative until parity holds.
@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@PhysShell, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 29 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b7e00dd1-f10a-438e-b3dc-e712e0920715

📥 Commits

Reviewing files that changed from the base of the PR and between 3322d54 and 4d3a37c.

📒 Files selected for processing (1)
  • docs/proposals/P-022-rust-core-migration.md
📝 Walkthrough

Walkthrough

This PR adds two documentation files and updates the proposals index. It introduces a cross-stack strictness/fitness policy document (Rust, C#/.NET, Python tooling, ratchet doctrine) and a design proposal (P-022) for migrating the analysis core to Rust, including architecture, differential oracle, and migration strategy.

Changes

Documentation additions

Layer / File(s) Summary
Cross-stack strictness and fitness policy
docs/notes/strictness-and-fitness.md
Documents a "ratchet, not big bang" strictness doctrine, per-stack tooling (Rust lints/cargo-deny, C#/.NET analyzers/architecture tests, Python mypy/ruff/import-linter), cross-stack tools (CodeQL, Semgrep), SOLID-as-semantic framing, and a rollout order.
P-022 scope, seams, and crate topology
docs/proposals/P-022-rust-core-migration.md
Defines migration goals/non-goals, contract-stable seams (OwnIR, diagnostics, output surfaces), and the proposed crate dependency DAG (own-ir, own-syntax, own-cfg, own-analysis, own-diagnostics, own-codegen, own-cli, own-oracle).
Rust idioms, libraries, and performance principles
docs/proposals/P-022-rust-core-migration.md
Lists Rust patterns to reduce accidental complexity, candidate libraries, and performance/correctness principles for the port.
Architectural fitness tooling and lint ratchet
docs/proposals/P-022-rust-core-migration.md
Outlines CI-automatable architectural analyzers, workspace clippy lint config, and concrete allowed dependency-edge fitness rules.
Differential oracle and migration plan
docs/proposals/P-022-rust-core-migration.md
Details the Python-vs-Rust differential oracle, bottom-up migration strategy, open questions, and initial deliverables.
Proposals index update
docs/proposals/README.md
Adds the P-022 entry with draft/exploratory status.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CI
  participant PythonCore
  participant RustCore
  participant Oracle
  CI->>PythonCore: run on corpus input
  CI->>RustCore: run on same corpus input
  PythonCore-->>Oracle: exit status, SARIF/JSON, CFG export
  RustCore-->>Oracle: exit status, SARIF/JSON
  Oracle->>Oracle: compare normalized outputs exactly
  Oracle-->>CI: pass/fail verdict
Loading

Related issues: None specified.

Related PRs: None specified.

Suggested labels: documentation, proposal

Suggested reviewers: None specified.

Poem

A rabbit sketches plans in ink,
Rust crates lined up, link by link.
Strictness ratchets, tighter still,
Old code climbs the fitness hill.
Oracle watches, byte for byte —
Two cores compared, one path to right. 🐇

🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: a draft proposal for P-022 about Rust core migration and its architecture.
Description check ✅ Passed The description follows the template well, covering purpose, change type, verification, related issue, and checklist items.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/ownnet-execution-surfaces-adr-8kyc0n

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Copy link
Copy Markdown
Owner Author

@coderabbitai review
@codex review

This is an architecture brainstorm, not a gate — docs/proposals/P-022-rust-core-migration.md is a design-only, bird's-eye plan for porting the Python core to Rust with the Python repo as a differential oracle. Please push back hard on the architecture, not the prose. Prompts I'd love takes on:

  1. Crate DAG / coupling. Is own-syntax → own-cfg → own-analysis → own-diagnostics → own-codegen → own-cli (with own-ir as a leaf-ish contract) the right decomposition? Where would coupling realistically leak — e.g. does own-diagnostics end up needing analysis internals for evidence, breaking the "verdict layer independent of the solver" invariant?
  2. Dataflow state representation. Persistent maps (imbl/rpds) vs an arena + copy-on-write for State at merges — which wins for a monotone worklist over a finite lattice, and what should we actually benchmark?
  3. Parser. Hand-rolled recursive descent (for golden-parity of error messages) vs chumsky/winnow. Trade-offs for a differential-oracle context?
  4. The oracle itself. Comparing on the SARIF/JSON contract via the existing scripts/oracle_compare.py (the Rust core as "just another SARIF producer") — what are the blind spots? Which divergences would this miss (e.g. things not represented in SARIF), and how would you close them?
  5. Prior art. Anything better than ruff / rust-analyzer / rustc_mir_dataflow / Polonius to steal the analysis-pass + lattice + registry architecture from?
  6. Repo layout. Monorepo (rust/ subtree, easiest for the oracle to run both) vs sibling repo — any reason to prefer the split?

Findings/suggestions welcome as inline comments on the doc. Thanks!


Generated by Claude Code

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
gh pr view 166 --repo PhysShell/Own.NET --json files,title,body -q '.files[].path'
echo "---"
fd -i "P-022" .
echo "---"
fd -i "oracle_compare" .

Length of output: 465


🏁 Script executed:

#!/bin/bash
set -euo pipefail
echo "=== P-022 doc ==="
cat -n docs/proposals/P-022-rust-core-migration.md
echo "=== oracle_compare.py ==="
cat -n scripts/oracle_compare.py
echo "=== proposals README diff context ==="
sed -n '1,60p' docs/proposals/README.md

Length of output: 65507


🏁 Script executed:

#!/bin/bash
set -euo pipefail
fd -i "evidence.py" .
echo "---"
sed -n '1,80p' ownlang/evidence.py 2>/dev/null || true
echo "---diagnostics.py head---"
sed -n '1,60p' ownlang/diagnostics.py 2>/dev/null || true
echo "---grep Evidence usage in analysis---"
rg -n "Evidence" ownlang/*.py | head -50

Length of output: 8541


🏁 Script executed:

#!/bin/bash
set -euo pipefail
fd -i "codegen" ownlang -t f
echo "---"
rg -n "^def emit|^def render|^import|^from" ownlang/codegen.py 2>/dev/null | head -30
echo "---cli---"
rg -n "emit\(|render\(|analyz" ownlang/cli.py 2>/dev/null | head -40

Length of output: 832


🏁 Script executed:

#!/bin/bash
set -euo pipefail
rg -n "^def emit" ownlang/codegen.py
sed -n '1,40p' ownlang/codegen.py | tail -20
echo "---find cli file---"
fd -i "cli" ownlang -t f

Length of output: 471


🏁 Script executed:

#!/bin/bash
set -euo pipefail
grep -n "^def \|^class " ownlang/codegen.py | head -30

Length of output: 1113


@PhysShell Read the doc end to end plus cross-checked the claims against ownlang/analysis.py, ownlang/diagnostics.py, ownlang/evidence.py, and ownlang/codegen.py. This is a genuinely well-argued bird's-eye plan — the strongest part is that the oracle rides on contracts you already froze (SARIF/evidence) rather than internal state. Pushback per prompt:

1. Crate DAG / coupling

The own-analysis → own-diagnostics edge is actually validated by the current code: ownlang/analysis.py (Lines 239–262, 605–673) constructs Evidence — a type owned by ownlang/diagnostics.py — using solver-internal state (st, rid, line lookups). So the dependency correctly points from the solver toward the verdict/data type, not the reverse. Good sign the invariant will hold in Rust too (own-analysis depends on the Evidence/Diagnostic structs from own-diagnostics; own-diagnostics never reaches back into Lattice/worklist internals).

But I think the diagram has one edge that doesn't match reality: own-diagnostics ──▶ own-codegen. In the Python core, codegen.generate(mod: A.Module) (ownlang/codegen.py:792) takes only the AST — it never imports diagnostics.py or consumes Diagnostic/Evidence. Codegen is decided purely from AST-level shape (_fn_has_buffer, _laminar_scopes, _buffer_modes, etc.), not from analysis verdicts. If that stays true in Rust, own-codegen should depend on own-cfg/AST types directly, and the "verdict-independent codegen" fact is worth stating explicitly rather than implied by a diagram edge that suggests codegen is downstream of diagnostics. If Rust codegen does eventually want ownership verdicts (e.g., inserting a generated Dispose() based on the solver's own conclusions rather than re-deriving policy from AST shape), that's a deliberate new coupling worth flagging separately — don't let it sneak in through the diagnostics layer just because the arrow already points that way in the doc.

2. Dataflow state representation

Agree persistent maps are the right default to reach for, but bench against arena+generational-index CoW before committing, because:

  • imbl/rpds win when merges are frequent and sharing is high (wide CFGs, many join points) — structural sharing amortizes well.
  • A dense arena with a "dirty" bitset + copy-on-write clone wins when the analysis is mostly linear (few merges) and state is small per-RID, since persistent-map lookup has a real per-access constant-factor cost that a flat Vec doesn't.
  • Given the doc's own description of the analyses (ownership/lifetime/effect/DI) as fairly procedural with modest branching per function (not the wide, deeply-nested CFGs of a whole-program solver), I'd bet the arena+CoW wins on this workload, and imbl wins only if you graduate to interprocedural/whole-program dataflow later. Benchmark on the corpus's largest real function (not synthetic), and measure both wall-clock and peak RSS — persistent maps often lose on RSS due to node overhead even when they win on clone-time.

3. Parser

Hand-roll is the right call given the explicit goal is golden-parity of error messages, not just successful parses. chumsky/winnow optimize for combinator ergonomics and give you decent error recovery for free, but matching Python's specific error message wording/positions exactly through a combinator library's error model is friction you don't need in a differential-oracle context — you'd fight the library to reproduce quirks rather than benefiting from it. Once golden-parity is achieved and frozen (i.e., you have a byte-for-byte diffable error corpus), you can then evaluate whether a combinator rewrite still holds parity — but do that as a later refactor gated by the same oracle, not up front.

4. The oracle

This is where I'd push hardest. scripts/oracle_compare.py is a great reference but it's built for a different job: cross-tool fuzzy matching against external tools with different line-numbering conventions (near() at Line 204: a.fkey == b.fkey and abs(a.line - b.line) <= tol, default tolerance 3 lines) and coarse severity buckets (leak/use-after/double/other). For a golden Python-vs-Rust comparison where both consume the identical source and OwnIR, you want the strict mode: exact (path, line, code, subject) set equality, not line-window fuzzy matching — reusing near() as-is would silently mask real divergences (e.g., an off-by-one in the Rust CFG lowering would slip through a ±3-line tolerance).

Blind spots the SARIF/JSON contract comparison structurally can't catch:

  • Crashes/panics on valid input — a Rust panic has no SARIF representation; the oracle needs an explicit "did both binaries exit 0" gate before it even diffs output.
  • Determinism/ordering within the same (line, code) — if two diagnostics tie on the sort key, is intra-tie order also pinned, or could it drift silently?
  • Evidence step semantics, not just SARIF shape — the role field ("moved", "acquired" in analysis.py) drives label text; verify the oracle diffs message text too, not just codeFlows locations, since a step landing on the right line with the wrong label is a semantic bug SARIF-location-diffing alone won't catch.
  • Performance/timeout divergence and stderr/warning output (e.g., P-014's OWN050 advisory) — neither is in SARIF and both matter for parity claims.
  • .ownreport.json fields not modeled in SARIF — the doc says these contracts are compared "layer by layer" (Line 201), which is right; just don't let "SARIF-clean" imply "fully parity" in the CI gate's messaging.

5. Prior art

Your list is strong. One I'd add: clippy's lint-pass registry (distinct from rustc's own dataflow) is a closer analogue to your OWN-code registry than ruff's in some ways, since it's a second-party lint layer bolted onto a compiler's MIR, similar to how your analyses sit atop OwnIR facts from external frontends. Also worth a look: prusti-viper's encoding boundary (Rust MIR → Viper IR) as a model for how you'd eventually seam a verification backend (ties back to P-002) onto own-ir without touching own-analysis.

6. Repo layout

Monorepo is right for this phase — the oracle's whole value proposition is "one clone, one CI run, diff both binaries," and a sibling repo adds submodule/pinned-SHA ceremony that only pays off once the Rust core is independently versioned/released. Revisit the split once Rust is authoritative and Python becomes the legacy/reference-only artifact — at that point the coupling direction inverts and a monorepo's blast radius (Rust CI running on every Python-only doc change, etc.) becomes the annoyance instead of the benefit.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 220502241f

ℹ️ 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".

Comment on lines +189 to +191
shape `scripts/oracle_compare.py` already produces for the cross-tool (Infer#/
CodeQL) diff. **The Rust core is just another SARIF producer that the existing
oracle diffs against Python.**

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Replace the cross-tool oracle before using it for parity

scripts/oracle_compare.py is not an exact implementation-parity oracle: it parses SARIF down to (path, line, rule, message, cls), then compare() keeps only leak-class findings and matches by basename within a line tolerance. If the Rust port reuses it here, changes in evidence/codeFlows, subject/resource metadata, warning/advisory levels, non-leak diagnostics, full paths, ordering, or even exit status can still pass as “parity”. For the Rust migration, make this a new exact diff harness over canonical stdout/stderr/status/SARIF, or extend the script with an exact mode, instead of using the cross-tool oracle.

Useful? React with 👍 / 👎.

│ └────────────────────────┐
own-syntax ───┤ │
(lexer/AST) ▼ │
own-cfg ──▶ own-analysis ──▶ own-diagnostics ──▶ own-codegen

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Route codegen around diagnostics

The diagram locks own-codegen behind own-diagnostics, but the crate notes say codegen emits from CFG + analysis results. With the allowed-edge test proposed below, codegen would have no legal access to solver result types unless own-diagnostics re-exports analysis internals or codegen re-runs/duplicates analysis, which breaks the stated “diagnostics knows nothing about solver internals” invariant. Model diagnostics and codegen as sibling consumers of own-cfg/own-analysis instead of chaining codegen through the verdict renderer.

Useful? React with 👍 / 👎.

files, and OwnIR fact files. Plus generated inputs (see below).
- **Run both:** `python -m ownlang check --format sarif <f>` vs
`own-rs check --format sarif <f>` (identical CLI surface + SARIF ⇒ clean diff).
Likewise `cfg` (dump CFG as JSON), `report` (`.ownreport.json`), `emit` (C#).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Add a canonical CFG JSON seam before depending on it

This treats cfg JSON as an existing per-layer contract, but the Python CLI currently prints a human CFG with _print_cfg and has no --format json surface. If the Rust port starts diffing CFGs as written, there is no Python JSON oracle to compare against, and mirroring the text dump would bake in a non-contract debug format. Add and freeze a canonical Python CFG JSON export before the ratchet uses this seam.

Useful? React with 👍 / 👎.

Both reviewers converged; all corrections are factual/design refinements
(no forks):

- Crate DAG: own-codegen is a SIBLING of own-diagnostics (both consume
  own-cfg/own-analysis), not chained through it. Codegen is
  verdict-independent (AST/CFG-driven, matching Python codegen.generate),
  so the old own-diagnostics -> own-codegen edge would have forced
  diagnostics to re-export solver internals. Added a fitness function
  locking codegen !-> diagnostics (and !-> analysis for now). (Codex P2,
  CodeRabbit #1)
- Oracle: do NOT reuse scripts/oracle_compare.py as the parity oracle —
  it is cross-tool fuzzy (leak-only, +-N line tolerance, coarse severity)
  and would mask off-by-one/label/subject/exit-status divergences. Spec a
  new exact harness over status+stdout+stderr+SARIF/JSON, with an
  exit/crash gate first, exact set-equality incl. evidence label text,
  and intra-tie ordering. (Codex P1, CodeRabbit #4)
- CFG seam does not exist yet: python cfg prints human text, not JSON.
  Add+freeze a canonical cfg --format json before the ratchet uses that
  seam; added as migration step 0. (Codex P2)
- State: arena+CoW likely wins this procedural workload; bench largest
  real function, wall-clock + RSS. Prior art: clippy lint-pass registry,
  prusti-viper encoding boundary. Repo-layout revisit trigger. (CodeRabbit
  #2/#3/#5/#6)

Copy link
Copy Markdown
Owner Author

Thanks both — genuinely useful architecture pushback. Folded all of it into e2e0721. Two points converged across both reviewers; one was Codex-only. Summary of what changed in P-022:

Codegen ⊥ diagnostics (Codex P2 @65 + CodeRabbit #1) — redrew the DAG: own-codegen and own-diagnostics are now sibling consumers of own-cfg/own-analysis, not chained. Documented that codegen is verdict-independent (AST/CFG-driven, matching Python codegen.generate(mod)), and added a fitness function locking own-codegen !→ own-diagnostics (and !→ own-analysis for now; a future edge is an explicit, reviewed change to the allowed set).

Oracle is not oracle_compare.py (Codex P1 @191 + CodeRabbit #4) — you're right that it's a cross-tool fuzzy matcher (leak-only, ±N-line near(), coarse severity) and would mask off-by-one / label / subject / exit-status divergences. The doc now specs a new exact harness over canonicalized status + stdout + stderr + SARIF/JSON: exit/crash gate first, strict set-equality on (path, line, code, subject) plus evidence label text, pinned intra-tie ordering, and "SARIF-clean ≠ full parity". oracle_compare.py is referenced only for SARIF-reading plumbing.

CFG-JSON seam doesn't exist yet (Codex P2 @185) — correct, cfg prints human text via _print_cfg, no --format json. Added migration step 0: build + freeze a canonical cfg --format json on the Python side before the ratchet leans on that seam; don't bake the debug dump in as a contract.

Also folded (CodeRabbit): arena+CoW as the likely winner for this procedural workload (bench largest real function, wall-clock and RSS); prior art += clippy lint-pass registry + prusti-viper encoding boundary; parser combinator-rewrite deferred behind frozen error-parity; monorepo revisit-trigger noted.

Still a draft / design doc — no code yet. Further pushback welcome.


Generated by Claude Code

claude added 3 commits July 3, 2026 02:29
Performance: frontend-specific levers in payoff order — intern everything
(u32 symbols), bitset lattice (u8 bitflags for VarState + dense Vec<_> by
RID, not HashSet) which also tips the persistent-vs-arena call toward
arena+CoW, arenas + u32 indices, FxHashMap, rayon across independent
per-function analyses, mimalloc/jemalloc. Micro-opt last, behind a
flamegraph; perf locked by an iai-callgrind instruction-count ratchet in
CI (deterministic). Added the matching library rows.

Strictness: declarative [workspace.lints] block (Rust >= 1.74) —
unsafe_code=forbid, unreachable_pub/rust_2018_idioms deny, clippy
pedantic/nursery as warn with justified allows, unwrap/indexing/
arithmetic_side_effects/panic/dbg/print_stdout denied surgically. Added
cargo-mutants (does the suite/oracle actually catch a broken transfer?),
cargo-semver-checks, cargo-hack --feature-powerset, cargo-udeps, and the
honest 'no NDepend-for-Rust' limit. Plus the ratchet doctrine: baseline
current violations, hold new code to the full bar, tighten half a turn
per iteration — same ratchet as the oracle (correctness) and
iai-callgrind (perf), honouring 'a false positive is worse than a miss'
at the tooling layer.
…pline)

Cross-checked the performance section against the source article
(troubles.md, 'Writing words and reading dwords'). Added the concrete
frontend-relevant levers it teaches that were missing:

- bump/typed arena for the build-once/read-many AST/CFG (bumpalo,
  typed-arena, rustc TypedArena) alongside u32 indices;
- flat backing arrays, never Vec<Vec<_>> (a cache miss per row);
- SmallVec for the many-small collections (evidence steps, loans,
  per-block diagnostics) — inline by the law of small numbers;
- enums / impl Trait / &dyn over Box<dyn>;
- the article's discipline framing we already run as doctrine: optimize
  by cumulative cost under perf (not by looks), skip one-time costs, keep
  the transfer function pure (registers + benchmarkability), algorithms
  first, #[inline] only on tiny cross-crate hot fns (never inline(always)
  by reflex), LTO + panic=abort, micro-opt last behind a flamegraph;
- further-reading credits (the article, ripgrep post-mortem, Fastware).
Capture the tooling posture across all three stacks as a companion to
P-022, centered on the ratchet doctrine (baseline current violations,
hold new code to the full bar, tighten half a turn per iteration) — the
same monotone gate we run for correctness (oracle) and perf
(iai-callgrind), grounded in 'a false positive is worse than a miss'.

Per stack: Rust (cargo-deny/udeps/semver-checks/hack/modules/mutants;
[workspace.lints] lives in P-022; honest 'no NDepend-for-Rust' limit);
C#/.NET WPF (csproj strictness + editorconfig; IDisposableAnalyzers,
VSTHRD, Meziantou, ErrorProne, Sonar, BannedApiAnalyzers; NetArchTest/
ArchUnitNET + NDepend + Stryker.NET); Python ownlang (ruff ALL, xenon,
vulture, import-linter, pyright, Hypothesis). Cross-stack: CodeQL +
Semgrep. Notes the SOLID/SRP semantic limit (proxy metrics only; real
detection = review, human or the CodeRabbit/Codex layer we already run)
and a start order for ~80% of the value in a day. P-022 links to it.

Copy link
Copy Markdown
Owner Author

@codex review

Specifically: please pressure-test and extend the "Performance principles" section of docs/proposals/P-022-rust-core-migration.md with your own suggestions for this workload (a static-analyzer / compiler frontend — parse → CFG → monotone worklist dataflow → diagnostics/SARIF, ported from the Python ownlang/ core, with a differential oracle vs Python as the correctness gate).

Already covered (please add net-new, not repeats):

  • intern everything (u32 symbols), FxHashMap for small-int keys
  • bitset lattice (u8 bitflags for VarState + dense Vec<_> by RID) instead of HashSet
  • bump/typed arena for the immutable AST/CFG + u32 indices; flat backing arrays over Vec<Vec<_>>
  • SmallVec for the many-small collections (evidence steps, loans, per-block diags)
  • enums / impl Trait / &dyn over Box<dyn>
  • rayon across independent per-function analyses; mimalloc/jemalloc
  • discipline: profile by cumulative cost, keep the transfer function pure, #[inline] only tiny cross-crate hot fns, LTO + panic=abort, micro-opt last
  • perf locked by an iai-callgrind instruction-count ratchet in CI

What am I missing? Candidates I'd love your take on: worklist scheduling / RPO ordering to cut fixpoint iterations; representing the worklist itself (bitset-of-blocks vs VecDeque + in-queue flag); avoiding re-allocation across the per-block transfer (scratch buffers / state pooling); whether persistent maps (imbl) ever beat arena+CoW for our merge frequency; serde/SARIF serialization cost on large OwnIR inputs; string handling in diagnostics rendering; and anything about incremental/salsa that changes the data-model choices now. Concrete crate/pattern picks welcome.


Generated by Claude Code

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d2fc4df260

ℹ️ 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".

Comment on lines +190 to +191
under `perf`/a flamegraph, not by what "looks slow"; one-time costs (CLI/OwnIR parsing)
don't matter, the hot path (the transfer function in the worklist) does — so **keep

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 Badge Keep OwnIR and SARIF I/O in the perf budget

For CLI/CI scans over large OwnIR dumps, serde_json parsing and SARIF/report rendering happen on every input and are O(input + findings), so after the transfer loop is optimized they can dominate wall time and allocations. This sentence tells implementers to exclude that path from profiling; add representative large OwnIR/SARIF benches (e.g. from_slice/to_writer with buffered output and borrowed/interned diagnostic strings) rather than only transfer-function microbenches.

Useful? React with 👍 / 👎.

| `assert_never` dispatch updated by hand in cfg/analysis/codegen | `enum` + exhaustive `match` | a missed variant is a **compile error**, not a runtime assert |
| RID = `id(sym)`; handles keyed by object identity | newtype indices `Rid(u32)`, `BlockId(u32)`, `LoanId(u32)` + arena | deterministic, serializable, no identity hacks |
| AST/CFG as object graphs | **arena + indices** (`la-arena`/`id-arena`) | borrow-checker-friendly graphs, cheap clone, cache-friendly |
| `State.copy()` deep-copies dicts every merge | **persistent maps** (`imbl`/`rpds`) — structural sharing | join/clone at merges is O(log n) sharing, not a deep copy |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 Badge Avoid baking in persistent maps as the default state

This table presents imbl/rpds as the replacement for State.copy(), but the same proposal later argues dense bitset/Vec arena+CoW is likely better for this workload. If this row is followed during the port, the solver may get locked into tree-node allocation and O(log n) lookups before the corpus benchmark has compared it with state pooling/scratch-buffer CoW, so make persistent maps an explicit benchmark candidate rather than the default idiom.

Useful? React with 👍 / 👎.

Comment on lines +121 to +123
Direction of the analysis (forward worklist to fixpoint, union at merge, monotone
transfer over a finite lattice) ports directly — it is textbook `rustc_mir_dataflow`
shape.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 Badge Specify RPO-guided worklist scheduling

The solver shape is left as a generic forward worklist, but for monotone intraprocedural analyses with loops, block order determines how many times joins and transfers rerun. Without an RPO/postorder scheduling principle plus a cheap in_queue bitset (or dense bitset-of-blocks for small CFGs), an implementation can burn most of its time revisiting blocks and reallocating states even with the compact lattice described below.

Useful? React with 👍 / 👎.

Comment on lines +324 to +325
"no findings = parity". Diff `stderr` too (e.g. the P-014 OWN050 advisory notes
live there, not in SARIF).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 Badge Don't treat OWN050 as stderr-only

Current ownir SARIF output passes the shown findings into build_sarif (ownlang/__main__.py:320-325), and the test suite pins advisory OWN050 as a SARIF result with level: "note" (tests/test_ownir.py:2251-2263). Saying these notes live in stderr can make the exact oracle compare the wrong stream or double-count human summary text; stderr is only the machine-format summary/chatter, while the advisory finding remains semantic SARIF output.

Useful? React with 👍 / 👎.

…OWN050)

Four P3 suggestions, all confirmed:

- OWN050 is a real SARIF result (level: note, pinned in test_ownir.py),
  not stderr-only — corrected the oracle exit/crash-gate bullet so the
  exact harness compares it on the SARIF seam and doesn't diff the wrong
  stream / double-count the human summary. (Codex @325)
- Solver scheduling: added RPO/reverse-postorder worklist ordering + an
  in_queue bitset (dense bitset-of-blocks for small CFGs) + scratch-state
  reuse as a first-class lever — block order dominates fixpoint
  iteration count even with the compact lattice. (Codex @123)
- I/O in the perf budget: over large OwnIR dumps serde_json parse + SARIF
  render are O(input+findings) per invocation and can dominate once the
  transfer is tight; bench large-OwnIR from_slice / buffered to_writer
  with interned strings, not only transfer microbenches. Softened the
  'one-time costs don't matter' claim. (Codex @191)
- Dataflow-state table row: present persistent maps (imbl/rpds) as a
  benchmark *candidate*, with dense-bitset arena/scratch CoW as the
  leaning default — don't lock the port into tree-node alloc + O(log n)
  before the corpus bench. (Codex @116)

Copy link
Copy Markdown
Owner Author

Thanks @codex — all four folded into b23f5f8:

  • OWN050 is SARIF, not stderr (@325) — corrected. You're right: it's a real result at level: "note" (pinned in test_ownir.py), so the exact oracle compares it on the SARIF seam; stderr is only the machine-format summary/chatter. Fixed the exit/crash-gate bullet so we don't diff the wrong stream or double-count.
  • RPO worklist scheduling (@123) — added as a first-class lever: reverse-postorder visitation to minimize fixpoint iterations, an in_queue bitset (dense bitset-of-blocks for small CFGs) to avoid double-enqueue, and scratch-state reuse across transfers. Good catch that the compact lattice alone doesn't buy this.
  • I/O in the perf budget (@191) — softened "one-time costs don't matter": over large OwnIR dumps serde_json parse + SARIF render are O(input+findings) per run and can dominate once the transfer is tight, so the perf section now calls for representative large-OwnIR from_slice / buffered to_writer benches with interned strings, not only transfer microbenches.
  • Persistent maps ≠ default (@116) — the state row now reads "dense bitset Vec + arena/scratch CoW (leaning default); imbl/rpds (benchmark candidate)", so the port isn't locked into tree-node alloc + O(log n) before the corpus bench.

Still a draft design doc — no code yet.


Generated by Claude Code

@chatgpt-codex-connector

Copy link
Copy Markdown

Summary

Testing

  • git diff --check

Committed changes on the current branch:

  • 3a38d70 docs(P-022): align state idiom with perf guidance

Created PR:

  • docs(P-022): align state idiom with perf guidance

View task →

Codex flagged (and fixed in its sandbox) that the Patterns/idioms table
still presented persistent maps as THE State.copy() replacement, out of
sync with the Libraries-row/perf-section fix. Apply the same wording here
so the doc is internally consistent: dense bitset Vec + arena/scratch CoW
is the leaning default; imbl/rpds a benchmark candidate. (Codex's own
commit 3a38d70 never landed on the branch/GitHub — folded manually.)

Copy link
Copy Markdown
Owner Author

@coderabbitai review

Draft design doc — re-review requested after folding your earlier round + Codex's. Since your last pass, docs/proposals/P-022-rust-core-migration.md gained: a Performance principles section (interning, bitset lattice, arenas/SmallVec, RPO worklist scheduling, rayon, allocator, iai-callgrind ratchet), a compiler-strictness [workspace.lints] + ratchet section, corrections to the crate DAG (codegen ⊥ diagnostics) and the oracle (exact harness, exit/crash gate, OWN050-is-SARIF), and a companion docs/notes/strictness-and-fitness.md (cross-stack tooling policy). Pushback on the perf/strictness reasoning and any remaining internal inconsistencies welcome.


Generated by Claude Code

@coderabbitai

coderabbitai Bot commented Jul 3, 2026

Copy link
Copy Markdown
🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail
gh pr view 166 --repo PhysShell/Own.NET --json commits -q '.commits[].oid' | tail -20
echo "---"
fd -i "P-022" .
echo "---"
fd -i "strictness-and-fitness" .

Length of output: 705


🏁 Script executed:

#!/bin/bash
set -euo pipefail
echo "=== P-022 doc ==="
cat -n docs/proposals/P-022-rust-core-migration.md
echo "=== strictness-and-fitness ==="
cat -n docs/notes/strictness-and-fitness.md

Length of output: 39048


@PhysShell Good round of edits — the codegen/diagnostics sibling fix and the exact-harness oracle spec are both solid now. Two internal inconsistencies survived into this version, both worth a fix before this settles:

1. Crate diagram arrow direction contradicts the fitness functions you just added.

Lines 65–66:

  own-cfg + own-analysis ─┬─▶ own-diagnostics  (Diagnostic/Evidence model, text + SARIF)
                          └─▶ own-codegen      (C# emit — AST/CFG-driven, verdict-independent)

By the convention you use everywhere else in this diagram (Lines 61–63: own-syntax └─▶ own-cfg └─▶ own-analysis, arrow tip = the dependent crate), this reads as "own-diagnostics depends on own-analysis" and "own-codegen depends on own-analysis." But your own fitness functions two sections later say the opposite:

"own-diagnostics and own-ir must not depend on own-analysis" (Line 316)
"own-codegen must not depend on ... own-analysis either" (Line 318–319)

And it contradicts the finding from the first review round that's still true here: Evidence/Diagnostic are types owned by own-diagnostics and constructed by own-analysis using solver-internal state — so the real Cargo edge is own-diagnostics → own-analysis (own-analysis depends on own-diagnostics for the struct defs), not the reverse. Your own migration strategy (step 4 vs step 5, Lines 390–392) is actually consistent with this correct direction — it's only the ASCII diagram that still draws it backward. Suggest redrawing as:

own-syntax └─▶ own-cfg ─┬─▶ own-analysis  (depends on own-diagnostics for Diagnostic/Evidence types)
                        └─▶ own-codegen   (AST/CFG-driven only, no own-analysis, no own-diagnostics)
own-diagnostics ─▶ own-analysis

(or however you want to render it — the point is the arrow needs to originate from own-diagnostics into own-analysis, and own-codegen's only upstream should be own-cfg.)

2. unsafe_code = "forbid" (workspace-wide) vs. the perf section's unsafe get_unchecked lever.

Line 278 sets unsafe_code = "forbid" in [workspace.lints.rust], and you correctly note (Line 297–299) that forbid "cannot be locally overridden" — that's the whole point of choosing it over deny. But the Performance principles section (Line 211) still lists "unsafe get_unchecked / manual bounds-check elision" as a legitimate last-resort lever. If the workspace lint is really forbid everywhere, that lever is dead on arrival for every crate — worth either (a) dropping the unsafe get_unchecked bullet since bounds-check elision via a single early assert! (which you already mention) achieves the same effect without unsafe, or (b) scoping unsafe_code = "forbid" to the crates that have no legitimate need (e.g. own-diagnostics, own-cli) and using deny + a documented, reviewed #[allow(unsafe_code)] for the handful of hot-path crates (own-cfg/own-analysis) where you actually want that door open. Right now the doc has it both ways.

Everything else — the exact oracle harness, OWN050-as-SARIF correction, RPO scheduling, I/O perf caveat, ratchet framing — reads internally consistent this round.

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Two internal inconsistencies CodeRabbit flagged this round:

- Crate-DAG arrows were drawn backward for the verdict layer. The solver
  constructs Diagnostic/Evidence (types owned by own-diagnostics), so the
  real Cargo edge is own-analysis -> own-diagnostics, NOT the reverse —
  which is what the fitness function ('own-diagnostics must not depend on
  own-analysis') and the migration order already imply. Redrew the diagram
  (own-diagnostics upstream of own-analysis; own-codegen depends on
  own-cfg only) and rewrote the prose + crate bullets to match.
- unsafe_code = 'forbid' (workspace-wide) contradicted the perf section
  still listing 'unsafe get_unchecked' as a lever. Dropped it: bounds-check
  elision is done the safe way (one early assert! + LLVM), and a profiled
  hot path that genuinely needs raw indexing scopes that ONE crate from
  forbid -> deny with a documented reviewed #[allow(unsafe_code)], never
  the workspace.

Copy link
Copy Markdown
Owner Author

Both fixed in 60b45f9 — good catches, both were real internal contradictions:

  1. DAG arrow direction. You're right: the solver constructs Diagnostic/Evidence (owned by own-diagnostics), so the real Cargo edge is own-analysis → own-diagnostics, not the reverse — exactly what the fitness function and the migration order already implied. Redrew the diagram (own-diagnostics upstream of own-analysis; own-codegen depends on own-cfg only), added an explicit "arrow = is-depended-on-by" legend, and rewrote the prose + crate bullets so nothing reads backward anymore.

  2. unsafe_code = "forbid" vs unsafe get_unchecked. Dropped the get_unchecked lever — bounds-check elision now stays the safe way (one early assert! + LLVM; Rust's safe iterators already match the C start/end idiom). Added the escape hatch you suggested as option (b): a profiled hot path that genuinely needs raw indexing scopes that one crate from forbiddeny with a documented, reviewed #[allow(unsafe_code)] — never workspace-wide. So the doc no longer has it both ways.

(Note: a re-review pass may still be mid-flight on the prior commit 3322d54; these fixes are in 60b45f9.)


Generated by Claude Code

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@docs/proposals/P-022-rust-core-migration.md`:
- Around line 59-70: The crate-topology block is an unlabeled fenced code block,
which markdownlint flags; update the fence in the proposal doc to use a language
tag such as text so the block remains unchanged but lint-clean. Locate the
fenced topology diagram around the own-ir/own-cli/own-oracle section and adjust
only the opening/closing fence.
🪄 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: e53541d5-5381-4a99-85f3-14ffeb76502a

📥 Commits

Reviewing files that changed from the base of the PR and between 6f633a4 and 3322d54.

📒 Files selected for processing (3)
  • docs/notes/strictness-and-fitness.md
  • docs/proposals/P-022-rust-core-migration.md
  • docs/proposals/README.md

Comment thread docs/proposals/P-022-rust-core-migration.md Outdated
@PhysShell PhysShell marked this pull request as ready for review July 3, 2026 05:04
@PhysShell PhysShell merged commit 65e1989 into main Jul 3, 2026
28 of 29 checks passed

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 4d3a37c19f

ℹ️ 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".

Comment on lines +76 to +79
The non-obvious edge is **`own-analysis → own-diagnostics`**: the solver *constructs*
`Diagnostic`/`Evidence` values (in Python, `analysis.py` imports and builds them from
`diagnostics.py` using solver-internal state), and those types are *owned by*
`own-diagnostics` — so analysis depends on diagnostics, **not the reverse**.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Add the CFG-to-diagnostics edge

This only calls out own-analysis → own-diagnostics, but the current Python pipeline also treats CFG lowering diagnostics as first-class verdicts: check_module appends d1 from build_cfg before analysis (ownlang/__main__.py:67-70), and ownlang/cfg.py emits Diagnostic for undefined names/resources, duplicate definitions, missing returns, etc. If the Rust crate graph omits own-cfg → own-diagnostics, lowering cannot produce those stable OWN030/OWN031/OWN033 results without duplicating or rerouting diagnostic types, which breaks the exact parity contract. Add that edge or explicitly state where CFG diagnostics are converted.

Useful? React with 👍 / 👎.


### Compiler strictness — declarative `[workspace.lints]` (Rust ≥ 1.74)

Lint config lives once in the workspace `Cargo.toml` and every crate inherits it:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Opt each crate into workspace lints

Cargo does not apply [workspace.lints] to every member automatically; per the Cargo/RFC 3389 inheritance model, a member manifest must include [lints] workspace = true before those lint levels are inherited. If the migration follows this block verbatim, a new crate can silently skip all of the deny/forbid settings during cargo clippy --workspace, so the strictness ratchet will not protect new code. Include the per-crate stanza or add a metadata check that fails when it is absent.

Useful? React with 👍 / 👎.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants