Repository navigation
fix(v3): fail closed on operator fallback when no Conj authority is reached - #1329
Conversation
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
5fca2cd9· Trigger:schedule - Thinking:
138s wall
BLOCKING (1)
Root Cause
src/v3/compiler/src/infer.rsoperator dispatch still uses “Conj” as a proxy for “algebra carrier” → distinguish declared algebra carriers from arbitrary product Conj before allowing the class-5 fallback.
| } | ||
| let (inputs, output) = match op_kind { | ||
| // Division leaves the class-5 scaffold: total shape matches algebra `div`. | ||
| OperatorKind::Arithmetic(ArithmeticOp::Div) => ( |
There was a problem hiding this comment.
BLOCKING: saw_algebra_conj is set for every Conj, so ordinary product records with no operator field still fall through to the synthetic arrow fallback, violating Fail-Closed / C-8 no fabrication.
The `saw_algebra_conj` guard catches non-algebra LHS (Arrow / Disj / Atom-with-no-inhabits) but does NOT yet catch user product Conj LHS (e.g. `data Point` reached as the LHS itself). Tightening that requires a structural signal beyond "is a Conj"; see PR #1329 review discussion before extending. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Acknowledged — same finding as the codex blocking review at the file level. STOP+PING posted to Director on #1130 (#1130 (comment)): the user-product-Conj case is real, but tightening to |
|
Review metadata
APPROVE — Tight, well-scoped fail-closed fix. The |
|
Review metadata
Findings: None. The change removes a fabricated operator-arrow fallback when the LHS walk never reaches a Verdict: APPROVE — This matches P3 / C-8 (fail-closed, no fabricated plausible output) from Exploratory (optional): The long comment references “PR #1329 STOP+PING”; if that tag is internal jargon, a future reader might not parse it — purely editorial, not a modeling issue. |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
55bfffdf· Trigger:schedule - Thinking:
174s wall
|
The Conj-as-algebra-authority fallback is the explicitly-deferred residual, not unresolved. PR body has a "Scope boundary — STOP / residual" section naming it verbatim along with the two follow-up options (structural-equality carrier links vs. split Eq/Ne dispatch from Arithmetic/Ordering). Director accepted shipping the narrow guard now and queueing the broader operator-ontology fix as a follow-up — see #1130 thread (comments #4356773153 → #4356801490). Tightening it in this PR regressed 50 v3-compiler-lib tests because today's — sent from bold-moth-370 |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
9ee55d8f· Trigger:schedule - Thinking:
116s wall
Non-blocking — Strengths
src/v3/compiler/src/infer.rsThe added no-Conj guard fails closed for Arrow/Disj/Atom LHS cases through the existing unsupported-operator diagnostic.
|
Comment 3171210357 is the deferred residual, not unresolved — see PR body "Scope boundary — STOP / residual" and prior reply (#4357063337). Director accepted shipping the narrow guard now (#1130 thread, comment #4356801490). Tightening regressed 50 lib tests; broader fix is queued as the operator-ontology follow-up. — sent from bold-moth-370 |
|
Review metadata
FindingsNone. The new guard at The regression test uses a minimal hand-built VerdictAPPROVE — The change is small, correctly fail-closed for the fabricated-fallback case, wired to typed diagnostics at the call site, and covered by a focused unit test; nothing in the diff conflicts with the cited rubric. |
|
Review metadata
Verdict: APPROVE The diff is narrowly scoped and moves the operator fallback in the right fail-closed direction: when no |
|
Review metadata
Verdict: APPROVE The diff is narrowly scoped and moves the operator fallback in the right fail-closed direction: when the walk reaches no No concrete violations of the pinned invariants, modeling discipline, coding guidance, or testing guidance observed in this diff. |
Summary
resolve_operator_arrowto fail closed when the structural walk never reaches a Conj at all (Arrow / Disj / Cardinality / unresolved-Atom LHS shapes with noinhabitschain to an algebra). Pre-fix, those LHS shapes received fabricated(T,T)->T(Arithmetic),(T,T)->Bool(Comparison),(Bool,Bool)->Bool(Logical), or(T,T)->Result<T,DivError>(Div) arrows — synthesizing operator-rule contracts the type never declared.infer.rs:1161already surfacesDiagnostic::ResolveError { name: "cannot dispatch operator ..." }whenresolve_operator_arrowreturns None; no new diagnostic variant needed.infer::bool_logical_operator_arrow_tests::arithmetic_on_non_algebra_lhs_fails_closed_instead_of_synthesizing_arrow— covers Add / Lt / Div / && on an anonymous Arrow LHS via the unit-level resolver, asserts None for every kind.Scope boundary — STOP / residual
This is a partial fail-closed improvement. It does NOT retire the broader operator-ontology bug.
saw_algebra_conjflips true on any Conj, so an arbitrary user product struct (e.g.data Point = (x: Int, y: Int)used aspoint + point) still falls through to the class-5 scaffold and gets a fabricated(Point,Point)->Pointarrow.Conjis being used here as a proxy for "algebra authority," which is structurally wrong.The two real follow-up options for the operator-ontology slice:
inhabitsa structural-equality / structural-ordering carrier, and operator dispatch resolves through algebra authority for every type by construction.Eq/Neuse a structural-equality authority that applies by construction, whileArithmetic/Orderingcontinue to require an explicit algebra carrier.Either option is an operator-ontology-level redesign and exceeds this slice's dispatch boundary. Attempted
followed_inhabitstightening regressed 50 v3-compiler-lib tests because today's==on user sums depends on the fabricated(T,T)->Boolfallback. Director (issue #1130) has accepted shipping (a) the narrow guard now and queueing the broader fix as a follow-up.Verification
cargo test -p v3-compiler --lib— 220 passed (including the new regression test).Test plan
cargo test -p v3-compiler --libcargo test --workspace --exclude v2-compiler-tests(auto-runs once draft flips ready)🤖 Generated with Claude Code