Repository navigation
Python operator-realization parity - #691
Conversation
|
Review finding: This diff looks Python-only, not Python/Go reconcile. The changed files are:
I don’t see any Go-side change here, so the PR title / lane framing is broader than the actual patch. More importantly, shrinking
but I don’t see a new boundary/integration receipt proving those programs execute under So I’d want one of two follow-ups before treating this as lane progress:
As written, the risk is that we re-open the Python determinism matrix without a precise proof that the admitted rows are semantically green. |
|
Direction for next revision: This should now be treated as a narrower Python parity PR, not broad Python/Go reconcile. Recommended shape for the next revision:
Concretely, the three fixtures reopened by the current exclusion shrink are:
Please make the proof for those rows explicit. Right now the diff looks directionally right, but the receipt is still too indirect for a parity claim. |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
8ea65c8f· Trigger:schedule - Thinking:
334s wall
BLOCKING (1)
Root Cause
src/v3/spec/python.dagOrderedRing.div was admitted as a bare infix Python carrier without a semantic-equivalence receipt → fail closed for Python Int division until a faithful realization exists, or route it through a declared helper tested on negative operands.
Non-blocking — Strengths
src/v3/compiler/tests/boundary/m1_4_emit_python_test.rsThe new CPython fixture test is a useful receipt for shrinking PYTHON_EMIT_EXCLUDE because it exercises the newly admitted list programs end to end.
| } | ||
|
|
||
| data python_int_eq: OperatorRealization = { | ||
| language: python_language |
There was a problem hiding this comment.
BLOCKING: carrier "//" realizes OrderedRing.div with Python floor division, which diverges from the Rust/Go Int division realizations for negative operands and violates DB-8 cross-target deterministic emission.
There was a problem hiding this comment.
Verified against current head 8b4c4719d: the unsafe Python division realization has been removed. src/v3/spec/python.dag no longer contains python_int_div, OrderedRing.div, or carrier "//", and the regenerated bootstrap snapshots were updated in the same commit. This PR now intentionally omits Python OrderedRing.div until a faithful helper-based realization exists.
|
Addressing the blocking review from commit Current code has no Validation after the fix:
|
|
Review metadata
Diff is narrowly scoped: adds Python Findings: none against the pinned docs.
Exploratory observation (non-ask): the per-ID-type Verdict: APPROVE — narrow, well-justified parity patch; the int-division omission shows good fail-closed instinct, and the CPython execution test gives the new realizations real teeth. |
# Conflicts: # src/v3/compiler/src/bootstrap_generated.rs # src/v3/compiler/src/bootstrap_generated_without_parse_surface.rs # src/v3/compiler/tests/integration/common/determinism_fixtures.rs
|
Review metadata
APPROVE — Diff is narrow and clean. The new Python operator rows stay on the existing shared |
|
Review metadata
ReviewThe actual diff between
Exploratory observation (non-blocking)
VerdictAPPROVE — diff is a 49-line test addition that exercises emitted Python against the canonical Rust-expected stdout for three existing fixtures. Clean, narrowly scoped, no invariant or coding/testing-discipline violations. |
|
Review metadata
Findings
Verdict |
|
Review metadata
Diff is a 55-line, test-only addition to
All helpers ( Verdict: APPROVE — narrowly scoped test additions that reuse the canonical fixture registry and execute emitted Python under CPython, matching existing patterns in the file. No invariants touched, no substrate changes, nothing to flag. |
|
Review metadata
APPROVE Diff is narrowly scoped to boundary tests, and the new checks reuse the existing |
|
Review metadata
Verdict: APPROVE — diff is small and test-only (55-line addition to |
|
Review metadata
Findings
Verdict |
Summary
Narrow Python parity follow-up for emitted program fixtures. This keeps Go out of scope: Go already had the relevant operator realizations, and the broader Python/Go
Behavior::Loopcapability landed separately in #692.This PR adds Python
OperatorRealizationrows for integer operators whose Python infix carrier preserves the Rust/Go-visible semantics needed by the reopened fixtures: multiplication, inequality, and integer comparisons.OrderedRing.divis intentionally not added here because Python//floors negative operands while Rust/Go integer division truncates toward zero; a faithful Python division realization needs a helper path rather than a bare infix carrier.The generated bootstrap snapshots are refreshed from
src/v3/spec/python.dag.Fixture Rows Reopened
The Python determinism matrix now admits these previously excluded fixtures:
list_map_then_fold_twelvelist_filter_then_fold_sevennested_list_builtins_inside_lambda_sixThe boundary receipt
emitted_python_admitted_list_fixtures_execute_under_cpythonexecutes those emitted Python programs underpython3and checks stdout against the Rust-canonical expected values (12,7, and6).Verification
cargo test -p v3-compiler --test integration emitted_python_admitted_list_fixtures_execute_under_cpython -- --nocapturecargo test -p v3-compiler --test determinism_test -- --nocapturecargo test -p v3-compiler --test integration bootstrap -- --nocapture