feat(types,hir,mir,codegen): Optional[int] flow-sensitive narrowing (D-199, Part 2 of #747) - #780
feat(types,hir,mir,codegen): Optional[int] flow-sensitive narrowing (D-199, Part 2 of #747)#780rotnov wants to merge 20 commits into
Conversation
Adds the MIR and codegen halves of flow-sensitive `Optional[int]`
narrowing, on top of the already-merged checker layer:
- pycc_hir::definitely_terminates: extracted as a shared predicate
(used by both pycc_types::narrow and pycc_mir) since pycc_mir cannot
depend on pycc_types.
- pycc_mir: MirExpr::OptionalUnwrap (read-side counterpart of
OptionalWrap), a $narrowed:{name} scope sentinel threaded through
lower_stmt, and the early-return continuation shape
(apply_post_if_narrowing / lower_stmt_sequence) mirroring the
checker's own mechanism.
- pycc_mir: lower_scoped_body isolates narrowing state around every
*nested* body (if/while/for/try/except/finally/match arms) via a
snapshot/restore of the $narrowed: sentinel subset, since MIR's
scopes stack is one shared mutable per-function frame rather than
per-branch Environment clones like the checker's overlay design --
without this, narrowing established by a nested early-return guard
would leak past its own enclosing body. Caught via end-to-end
execution of the extended fixture, not by a unit test alone; a
dedicated regression test now covers it directly.
- pycc_codegen: OptionalUnwrap emission (extract_value on the
Optional struct's payload field) plus the two separate refcount
classification predicates in bigint_rc.rs (retain_if_int_duplicate's
own inline match is the one that actually emits the retain call,
distinct from int_value_is_a_duplicate_reference).
- tests/fixtures/pep_0604_union.py extended with narrowed-use rungs;
verified byte-for-byte against local CPython 3.14.6 output and
against the compiled pycc binary.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…narrowing Documents the flow-sensitive Optional narrowing design (overlay-based checker state, shared pycc_hir recognizer, MIR scope-sentinel isolation) and flips the narrowing row in the conformance-breadth manifest to proven without promoting its subset marker.
Adds direct unit tests for branches that real type-checked programs cannot reach but that the checker/MIR still code defensively: - narrow.rs's narrowing_target on a wholly unbound name - stmt.rs's is-None test on a non-Optional name (narrows neither branch) - lib.rs's early-return guard on a non-Optional name, and a nested scoped body entered while already narrowed - hir::definitely_terminates's full &&-chain truth table
crates/pycc_codegen/src/bigint_rc.rs's retain_if_int_duplicate has a distinct compiled instantiation (shared by issue_382_exceptions, slice1_codegen_depth, and the pycc binary) whose own test traffic never duplicated a narrowed Optional[int] bigint payload into a second binding -- the one path that actually reaches its OptionalUnwrap arm. tests/fixtures/pep_0604_union.py's own narrowing coverage runs only through tests/conformance.rs's CPython byte-for-byte oracle comparison, which this environment skips whenever python3.14 isn't on PATH, so it never contributed to that instantiation's counters here. Adds tests/issue_769_optional_narrowing.rs: an oracle-independent, self-contained integration test (same build-and-run pattern as tests/issue_770_optional_reassignment.rs) that builds and runs a bigint-payload narrowed duplicate-binding scenario directly, plus a smallint mirror case and an absent-optional case. Also extends tests/fixtures/pep_0604_union.py with the same bigint duplicate-binding scenario so the oracle-backed conformance comparison exercises it too when the pinned CPython oracle is available. Full `cargo llvm-cov --workspace --fail-under-lines 100 --fail-under-regions 100` now reports 0 missed lines/regions/functions across 42836 regions.
Records the implementation, the D-014 coverage-gap diagnosis and fix, and the D-068 dispatch-capability gap (Skill(ievo:deep-review) refuses model invocation in this dispatched-subagent context, matching the identical lesson already logged for #763/PR #770) that leaves review and merge for a session that can actually run /ievo:deep-review.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Finding 1 (blocker, soundness): join_if_branches/join_loop_body/ join_match_branches (pycc_types) and lower_scoped_body's join call sites (pycc_mir) discarded a narrowing kill from a reassignment inside a nested branch, letting a stale Optional narrowing survive past that branch's own close (e.g. `if x is not None: if flag: x = None; print(x + 1)` was incorrectly accepted). Both crates now reconcile sibling-branch narrowing states via a sound conservative intersection (a name stays narrowed only if every supplied branch narrows it to the exact same type): pycc_types gains narrow::join_narrowed, pycc_mir gains its independent lib.rs::join_narrowed twin, and lower_scoped_body now returns each nested body's own ending narrowed state so If/While/ForRange/ForList in stmt.rs can join branches and re-apply the reconciled result; match/try call sites are unchanged (deliberately out of scope -- MIR only lowers HIR the now-sound checker already accepted, and the checker's own join fix for match/try is a provable subset of "unconditionally keep pre-branch state"). Verified end-to-end: `pycc check` correctly rejects the blocker repro with T0021. Finding 2 (warning, completeness): the checker's if/while fast-path helpers (check_if_branches_in_place, check_while_body_in_place, and their _in_function twins) bypassed narrowing-aware statement processing, spuriously rejecting a nested early-return guard inside an unrelated outer if. They now route through narrow::check_stmt_sequence / check_stmt_sequence_in_function like the slow path already did. Verified end-to-end: `pycc run` on the reported shape now builds and produces the expected output. Regression tests: two new pycc_mir::tests::narrow cases proving a reassignment inside a nested if/while kills narrowing past the nested body's own close, and seven new pycc_types checker-level tests covering both findings plus join-reconciliation edge cases (mixed-type sibling narrowing, partial narrowing across branches). Also fixes two pre-existing clippy::ptr_arg errors in apply_post_if_narrowing/restore_narrowing (crates/pycc_mir/src/lib.rs), confirmed via live CI logs to already exist on the pushed branch before this pass and to be blocking build-test-coverage/native-build-test/ ci-gate regardless of the review findings; narrowed from &mut Vec<...> to &mut [...] since neither function calls a Vec-specific method. cargo test --workspace: 0 failures. cargo llvm-cov --workspace --fail-under-lines 100 --fail-under-regions 100: 100.00% lines / 100.00% regions (42894 regions, 27848 lines, 0 missed). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
docs/ROADMAP.md's and docs/PYTHON_STANDARDS.md's #769 additions (from d3ae62f) pushed the llms.txt non-optional-expansion aggregate (scripts/check-site.sh, issue #207) from 260922 bytes -- already only 1222 bytes under the 262144-byte (256 KiB) ceiling on main -- to 264533 bytes, a genuine PR-caused regression confirmed via live CI logs on the already-pushed branch (the "build" job's "Validate website" step). Trimmed both entries to the essential facts (mechanism summary, D-199 pointer, evidence citation) without dropping any distinct claim; local `GITHUB_PAGES=true sh scripts/check-site.sh` now passes at 261976 bytes. The remaining 168-byte margin is thin -- worth a follow-up (either a policy to keep future ROADMAP.md feature-landing paragraphs terser, or revisiting the 256 KiB ceiling) but out of scope for this review-response pass. `ruby scripts/check_roadmap_evidence.rb` still passes after the trim. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
D-068 review response (98e8c41, b7d0340)Addressed the pinned Finding 1 (blocker, soundness) — fixed
def f(x: int | None, flag: bool) -> None:
if x is not None:
if flag:
x = None
print(x + 1) # unsound: x could be None herewas previously accepted. Both crates now reconcile sibling-branch narrowing via a sound conservative intersection: a name stays narrowed only if every supplied branch narrows it to the exact same type. Design tradeoff: intersection is not maximally precise (it doesn't special-case an unconditionally-terminating branch the way the ordinary Regression tests: End-to-end verification beyond unit tests: Finding 2 (warning, completeness) — fixedThe checker's def f(cond: bool, x: int | None) -> int:
if cond:
if x is None:
return 0
return x
return 0All four now route through Regression tests: 7 new checker-level tests in End-to-end verification: Finding 3 (doc drift) — no change neededChecked Incidental fixes (discovered while verifying this branch would actually pass CI)
Gate results
Commits: 98e8c41 (findings 1 & 2 + clippy fix), b7d0340 (llms.txt budget trim). Not merging — this pass fixed the previously-reviewed findings; a fresh |
…dies (D-068 re-review of #780, third round) Two soundness blockers found in a third D-068 pinned-reviewer round against #780: (1) a `while`/`for` loop body read a name narrowed on loop entry even though the loop can re-run and a later iteration's read may actually execute after an earlier iteration's own kill of that name; (2) an `except` handler was checked against the pre-`try` narrowed state even though the handler is only ever entered after some (possibly partial) prefix of the `try` body already ran, including any kill that prefix performed. Both stem from the narrowing design's single left-to-right source-order pass silently assuming execution order always matches source order -- true for straight-line code and `if`/`else`, false for any re-enterable construct. Fixed uniformly via `pycc_hir::killed_names`, a new shared primitive that recursively collects every name a body reassigns anywhere within it, and a new `apply_kill_prescan` in both `pycc_types::narrow` and `pycc_mir` (crate- local, since `pycc_mir` cannot depend on `pycc_types`) that drops those names from the narrowing overlay for the *entire* body before the normal pass runs -- wired into every `while`/`for` loop-body and loop-test call site (module scope, function scope, and both enum-loop helpers) and into `check_try_stmt`'s handler-body checking in both crates. `pycc_mir::expr.rs` has exactly one `narrowed_ty` caller, reading the same `scopes` state the prescan mutates, so MIR eligibility tracks the checker automatically. `crates/pycc_types/src/exception.rs` also gains a `check_stmt_sequence_shared` helper (replacing the old per-statement `check_stmt_shared`) as the vehicle needed to route the handler-body prescan through a sequence-aware call -- this bundles in the first half of the separate warning-finding fix (routing `try`'s four body loops through `narrow::check_stmt_sequence[_in_function]`), since the two changes are interleaved in the same function and cannot be cleanly split; the second half (the `check_match` case-body routing, plus the warning-finding regression tests) lands as a separate commit. Both repros verified empirically against `cargo run --bin pycc` on the pre-fix branch tip: `check` wrongly exited 0 and `run` executed rejected-in- principle `int | None` arithmetic without a diagnostic; after this fix both are rejected with T0021. New tests: 3 checker-level (two rejection repros plus a completeness guard proving the prescan does not over-drop narrowing a loop body never kills), 14 direct `pycc_hir::killed_names` unit tests pinning every `HirStmt` match arm, and 5 MIR-level tests (loop-body read, loop-test read, except-handler read, plus the MIR completeness guard). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…sequence (D-068 re-review of #780, third round warning finding) `check_match`'s per-case body loop called `check_stmt`/`check_stmt_in_function` directly per statement, bypassing `narrow::check_stmt_sequence[_in_function]` and therefore never running `apply_post_if_narrowing` -- so a nested early-return guard inside a `match` case body never narrowed the rest of that same case body, the identical fast-path-bypass defect an earlier D-068 round already fixed for the `if`/`while` fast-path helpers, just never routed through `match` in the first place. The `try`/`except`/`else`/ `finally` half of this same finding was already fixed in the previous commit (bundled there because it was inseparable from that commit's handler-prescan soundness fix in the same function). New tests: five regression tests proving a nested early-return guard now narrows the rest of its enclosing `match` case, `try` body, `except` handler body, and `else` body, plus a sanity check that a `finally` body's own (unrelated) narrowing behavior is unaffected by the routing change (`finally` cannot contain a `return`, so this specific fix's own narrowing-propagation behavior is not itself observable there). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…sequence D-199's Consequences section claimed "a narrowing fact never survives a reassignment of the narrowed name" without qualification -- true only for the constructs D-199 itself covered (straight-line bodies and if/else joins), never true in general once while/for loops and try/except handlers entered scope, per the two counterexamples this round's soundness fix addresses. D-199 is left unedited per this repository's decision-log convention (never hand-edit an accepted entry); this new entry narrows its claim and records the kill-prescan design, alternatives considered, and consequences. Regenerated docs/decisions/README.md via scripts/generate_decisions_index.py. Verified D-201 is the next free decision number: origin/main tops out at D-198, and D-199/D-200 are already claimed by this PR (#780) and PR #786 respectively. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Each of three D-068 review rounds against #780 found a new instance of the same execution-order-vs-source-order unsoundness class in Optional[T] narrowing (loop re-entry, then except-from-mid-try) because each round fixed only the flagged construct instead of stating and auditing the general invariant up front. This round's D-201 states the invariant explicitly and applies it uniformly via killed_names; the retrospective entry records the lesson for future flow-sensitive analysis work. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
D-068 re-review round 3: kill-prescan for re-enterable narrowed bodiesThis round fixes a soundness defect class in flow-sensitive Repros verified empirically (before/after)
Both confirmed as real Also fixed (warning-severity, not soundness): Approach taken and whyApproach 1 (kill-prescan) over Approach 2 (scope-down fallback), per the task's explicit preference. Before checking/lowering a re-enterable body, recursively collect every name that body reassigns anywhere within it (new Implemented identically in both MIR/checker consistency: grepped every caller of Files and functions changed
Tests added
Gates
Decision logNew entry D-201 narrows D-199's Consequences claim ("a narrowing fact never survives a reassignment of the narrowed name" — true only for the constructs D-199 covered, not in general once loops/try-except are in scope). D-199 itself is left unedited per this repo's decision-log convention. RetrospectiveAdded a 2026-08-25 entry to Still needed before mergePer D-068, another fresh pinned-reviewer ( |
D-068 re-review round 4 of #780 flagged the "Narrowing & flow typing" section as stale: it still described only D-199's original if/else scope and never mentioned that D-201's kill-prescan (round 3) extended narrowing survival into while/for loop bodies, try/except handlers, and match case bodies, subject to the prescan's conservative whole-body kill rule. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
D-068 re-review round 4 of #780 traced the actual reason a try's finally body needs no separate kill-prescan: it is checked against `joined`, an Environment folded from body_env (the full try-body walk, including dead code after a raise) through join_loop_body, then intersected with each handler's and the orelse's end-state via join_if_branches -- and it is narrow::join_narrowed's strict intersection, not "not a narrowing-sensitive path", that actually excludes any name killed on any of those paths. D-201 is not yet an accepted decision at the time of this correction -- it lands in this same pull request -- so its own justification text is corrected in place per this repository's append-only rule, which applies only to already-accepted entries. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
D-068 re-review round 4: doc-completion follow-upA fourth pinned-reviewer pass against this PR's kill-prescan soundness fix (round 3, Warnings fixed
Optional item: D-201's
|
Resolves conflicts against #774 (PEP 572 walrus operator), which merged concurrently: both PRs added new MirExpr match arms (OptionalUnwrap vs NamedExpr) at the same sites in bigint_rc.rs and exception.rs, and both touched the If/While lowering arms in pycc_mir/stmt.rs and pycc_types/lib.rs (kill-prescan narrowing vs walrus pre-binding) -- merged to preserve both features. docs/AGENT_RETROSPECTIVE.md and docs/ROADMAP.md conflicts were concurrent doc appends, kept both entries. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…769 overlap collect_named_expr_bindings's match became non-exhaustive after merging main's NamedExpr work in with OptionalUnwrap's kill-prescan work; add the missing arm. pycc_types/src/tests.rs required corresponding fixture updates from the same overlap. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… walrus kills (D-068 review of #780/#774's interaction) Two blocker findings from a D-068 pinned-reviewer round examining #780's Optional[T] narrowing (D-199/D-201) merged against #774's PEP 572 walrus support: 1. pycc_mir::expr::pre_bind_named_expr_targets bound a walrus target via bind_variable but never called kill_narrowing, so `(x := None)` inside a narrowed `if` branch left the stale `$narrowed:{name}` sentinel in place -- a read right after it kept unconditionally lowering to MirExpr::OptionalUnwrap for a value the walrus had just overwritten. Fixed by adding the same kill_narrowing call HirStmt::Assign's own arm already makes. 2. pycc_hir::collect_killed_names (the shared D-201 kill-prescan primitive) put a bare ExprStmt in a no-op arm and never inspected If/While's own test expression for an embedded NamedExpr, so a walrus-only kill inside a re-enterable while body was invisible to the prescan -- unsoundly leaving a pre-kill read narrowed on every iteration after the first. Fixed with a new collect_named_expr_targets_in_expr walker, wired into the ExprStmt/ If-test/While-test arms. Both are exercised by targeted regression tests plus a coverage-pinning test hitting every HirExpr arm of the new walker; a full check_source integration test confirms the checker now rejects the loop-reentry shape with T0021, matching the existing plain-assignment kill's behavior. D-201 is corrected in place (not yet merged into main, so the append-only rule for accepted decisions does not yet apply) to document both fixes. Confirmed via `cargo llvm-cov` before/after that the pre-existing coverage gaps in pycc_mir::expr.rs (super().method(args) lowering) and pycc_types::lib.rs (unroll_enum_loops_in_stmts recursion) already existed at the c304ae9 base commit and are unrelated to this change; flagged separately for tracked follow-up issues rather than fixed here. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
CI-mechanics-only fix round for this PR: - Trim docs/ROADMAP.md prose (the new #774 walrus milestone entry and several verbose sentences in the Language surface/Type system/CLI/ Diagnostics rows) without dropping any distinct fact or citation, to restore the site/llms-txt-context-manifest.json aggregate budget from 263090 to a passing byte count after merging main's #774/#150 docs. - Update site/status/index.html's "not yet" section: Optional[T] parsing and is/is not None comparisons already landed (D-197), and this PR's #769 adds top-level is/is not None narrowing of a checked Optional binding (D-199), so the page no longer claims Optional and narrowing have no grammar surface at all. Satisfies scripts/check_status_page_freshness.rb, which requires a site/status/index.html or site/index.html update whenever docs/ROADMAP.md gains a new feature-landing paragraph (here, #769's). - Update tests/fixtures/pages-performance-manifest.json's source_artifact_sha256 pin for site/status/index.html to match its new content. No compiler logic changed; crates/* untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Outstanding gate before merge (D-068): the pinned local reviewer
(
ievo:deep-review) has not been run against this branch.Skill(skill: "ievo:deep-review")refuses model invocation(
disable-model-invocation: true; "reserved for explicit user invocation")and this dispatched session has no
Task/Agent-style tool to routearound that block — the identical capability gap already logged for
#763/PR #770 (
docs/AGENT_RETROSPECTIVE.md, "2026-08-24 — A dispatchedsubagent cannot satisfy D-068's local-reviewer dispatch requirement"). A
session that can invoke
/ievo:deep-reviewneeds to run it against thefull committed range and resolve any actionable findings before this
merges.
Summary
Implements flow-sensitive
Optional[int]narrowing on a top-levelif name is None:/if name is not None:test — Part 2 of #747,following on from #763/PR #770 (Part 1). Fixes #769.
pycc_hir::optional_none_test/pycc_hir::definitely_terminates: ashared, environment-independent narrowing-test recognizer and a strict
terminator predicate, consumed directly by
pycc_mir(which cannotdepend on
pycc_types) and re-exported thinly bypycc_types::narrow.pycc_types: overlay-based narrowing state onEnvironment(clone/discard join semantics), applied both to
ifbodies and to theearly-return-narrows-the-continuation shape (a guard clause whose body
definitely terminates narrows every read after it).
pycc_mir: a$narrowed:{name}scope-sentinel on the existingscopesstack, plus
narrowing_snapshot/restore_narrowing/lower_scoped_bodyto recreate the checker's clone-and-discard semantics on MIR's single
shared scope frame. Deviation from the original dispatched design,
made and flagged per this task's own "resolve autonomously, flag the
deviation" instruction: without this, narrowing established inside a
nested body (e.g. a
whileloop inside a narrowedif) could leak pastthat body's own close on MIR's shared-frame model, which the checker's
own clone-and-discard
Environmentnever allows. Covered directly bya_nested_scoped_body_entered_while_already_narrowed_still_sees_the_narrowingin
crates/pycc_mir/src/tests/narrow.rs.MirExpr::OptionalUnwrap(read-side counterpart ofOptionalWrap),lowered to a single borrowed
build_extract_valuein codegen — noretain at the unwrap site itself.
bigint_rc.rs's existingretain_if_int_duplicate/int_value_is_a_duplicate_referenceduplicate-reference classification gained an
OptionalUnwraparm so abigint payload duplicated out of a narrowed binding into a second name
is still correctly refcounted.
docs/decisions/D-199-optional-t-flow-sensitive-narrowing-part2.md,docs/TYPE_SYSTEM.md,docs/ROADMAP.md, andtests/fixtures/conformance-breadth-manifest.jsonupdated (themanifest's narrowing row flips to
proven, its existing◐subsetmarker left unpromoted).
Deliberately out of scope (documented in D-199/TYPE_SYSTEM.md/the
manifest): compound conditions (
and/or), narrowing toNoneitself,raiseas an additional terminator alongsidereturn, and any test morecomplex than a top-level
is/is not Nonecomparison.D-014 coverage gate
cargo llvm-cov --workspace --fail-under-lines 100 --fail-under-regions 100initially failed at 1 missed region
(
crates/pycc_codegen/src/bigint_rc.rs:281, the newOptionalUnwraparm).Root cause: Cargo compiles
pycc_codegenunder several distinct metadatahashes depending on which package links it, and the one uncovered
instantiation (shared by
issue_382_exceptions,slice1_codegen_depth,and the
pyccbinary) is only reached when a bigint-valued narrowedOptional[int]read is duplicated into a second binding — the fixtureextension alone didn't close it locally because that scenario in
pep_0604_union.pyonly reaches this instantiation throughtests/conformance.rs's CPython-oracle comparison, which is skippedwithout a pinned
python3.14onPATH. Fixed withtests/issue_769_optional_narrowing.rs, an oracle-independentbuild-and-run integration test (bigint duplicate-binding, smallint
mirror, absent-optional cases), plus the same scenario added to
pep_0604_union.pyfor when the oracle is available. Final coverage: 0missed lines/regions/functions across 42836 regions.
Testing
cargo test -p pycc_hir --lib: 648 passed, 0 failed.cargo llvm-cov --workspace --fail-under-lines 100 --fail-under-regions 100:exit 0, 0 missed lines/regions/functions.
cargo test --workspace: 60test result: okblocks, 0 failures.ruby scripts/check_roadmap_evidence.rb: passed.python3 scripts/generate_decisions_index.py docs/decisions docs/decisions/README.md --check: up to date.See
docs/sessions/2026-08-25-02-issue-769-optional-narrowing-part2.mdfor the full session record.
🤖 Generated with Claude Code