Skip to content

Lane 1 Stage 1c PR 1: E-5 clean-emission invariant + Rust pilot - #514

Merged
briansrls merged 5 commits into
mainfrom
bright-bat-665
Apr 17, 2026
Merged

briansrls merged 5 commits into
mainfrom
bright-bat-665

Conversation

@briansrls

Copy link
Copy Markdown
Contributor

Summary

  • Declares the E-5 clean-emission invariant in INVARIANTS.md: emitted code must satisfy each target's declared clean-code contract by construction, with no #[allow(...)] / # noqa / pragma suppression. Contract grows by adding typed rules; never by silencing.
  • Lands the CleanEmissionContract type system in src/v3/std/clean_emission.dag — 8 typed rules (expression_wrapping, pattern_bindings, imports, block_return, variable_bindings, match_arm_body, correction_style, post_emit_verifier) per DB-4. Each rule is a closed coproduct with NotApplicable variants where relevant.
  • Populates rust_clean_emission: CleanEmissionContract in spec/rust.dag with the full concrete rule choices per DB-4. Caches the declaration in Dag::TargetSyntaxCache (same pattern as rust_rendering).
  • Pilot dispatch (Rust): emit_rust::render_branch_pattern reads contract.pattern_bindings and, when set to EmitUnderscoreWhenUnused, renders _ instead of the binding name when the arm body does not consume the payload port. Port-liveness is structural (walk path.output's producer→input subgraph); no textual scanning.
  • Checked-in lens snapshots (lens_cost_generated.rs, lens_unused_parameters_generated.rs) regenerated: match arms like Behavior::Value(v) => Vec::new() now emit Behavior::Value(_) => Vec::new(), eliminating real unused_variables warnings the emitter was previously producing.

Scope split (Stage 1c across three PRs)

This is PR 1. Two follow-ups tracked in src/v3/ROADMAP.md active-deferrals:

  • PR 2 — Go + Python pilots (emit_go's {x} := {expr}; blank-identifier path; Python's _v convention). Different render surface from Rust's render_branch_pattern, so separate PR per E-6 same-PR consumer discipline (consumers and spec items land together).
  • PR 3 — wire post_emit_verifier as a hard CI gate (rustc -D warnings, gofmt, py_compile) and strike unused_variables from the test-wrapper #[allow(...)] umbrella.

Stage doc reconciliation (bundled)

docs/phase1-lane2-clean-emission-invariant.md was drifted vs DB-4 (sketched 5 fields, DB-4 carries 8) and still listed E-6/E-7/E-8 as "candidate companion invariants" even though PR #493 landed them on 2026-04-16. This PR reconciles: Scope synced to 8 rules, StructFieldRule removed (publicity concern, not clean-emission), stale section rewritten to point at the landed invariants (+ E-9 from PR #497).

Test plan

  • cargo build -p v3-compiler — compiles.
  • cargo test -p v3-compiler — all green after regenerating lens_{cost,unused_parameters}_generated.rs (via cargo run -p v3-compiler --bin regen_lens_{cost,unused_parameters}).
  • cargo test --workspace --exclude v2-compiler-tests — all green.
  • cargo test -p v2-compiler-tests — all green (415 passed).
  • cargo clippy --all-targets -- -D warnings — clean.
  • cargo fmt --all --check — clean.
  • New test emit_rust_unused_payload_binding_renders_as_underscore asserts BoxedInt::Boxed { _0: _ } => 0 shape.
  • Ignored roundtrip emit_rust_unused_payload_binding_passes_deny_unused compiles the emitted code under #![deny(unused_variables)] — passes.

🤖 Generated with Claude Code

Declares the clean-emission contract type system (8 rules in
std/clean_emission.dag), populates rust_clean_emission in spec/
rust.dag, wires emit_rust to dispatch on PatternBindingRule::
EmitUnderscoreWhenUnused in render_branch_pattern, and adds
INVARIANTS.md E-5. Match-arm payload bindings that the arm body
does not consume now render as `_` by construction, so the emitted
code passes `rustc -D unused_variables` without suppression.

Port-liveness is a structural walk from path.output over
producer→input edges; no textual scanning of rendered body strings.

Scope split documented in the stage doc and src/v3/ROADMAP.md
active-deferrals: PR 2 adds Go + Python pilots; PR 3 wires
post_emit_verifier as a hard CI gate.

Also reconciles the stage doc with DB-4 (5→8 fields, strikes the
stale "candidate companion invariants E-6/E-7/E-8" section — those
already landed in PR #493).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@briansrls

This comment has been minimized.

@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: 82f20c509a

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

],
);
return Ok(format!("{} @ {}", binding.binding_name, inner_pattern));
return Ok(format!("{rendered_binding} @ {inner_pattern}"));

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 Avoid wildcard aliases in multi-field match patterns

When pattern_bindings resolves to underscore for an unused payload, the children.len() != 1 path now emits {rendered_binding} @ {inner_pattern}, which becomes _ @ Enum::Variant { ... } for multi-field variants. Rust's @ pattern requires an identifier on the left side, so this output does not compile. This regresses any arm that binds a whole multi-field payload and then doesn't use it (e.g., Variant(v) => 0 where Variant has multiple fields).

Useful? React with 👍 / 👎.

@briansrls

briansrls commented Apr 17, 2026 •

Copy link
Copy Markdown
Contributor Author

ChatGPT Review

Grounded against THESIS.md’s spec-driven emission claim and the active modeling-discipline checklist. chatgpt-review-a1b4677e-cc7d-4b…

chatgpt-review-77913d96-8e80-42…

Principle audit.

Fail-closed. Mostly good. The new Rust clean-emission contract is loaded through EmitError paths when the declaration is missing or malformed, so the happy path is no longer driven by ad-hoc emitter convention. My one real concern is non-blocking but worth tightening now: parse_pattern_binding_rule still accepts EmitPrefixedUnderscoreWhenUnused and NotApplicablePatternBinding for Rust (src/v3/compiler/src/emit_rust.rs:946-953), and render_payload_binding_name then silently treats them like the normal binding case (src/v3/compiler/src/emit_rust.rs:3497-3507). Since the comment directly above says those states are invalid for Rust, I’d rather reject them in build() and surface MalformedTargetSyntax than rely on tests to catch drift. Right now that last bit is behavioral, not structural.

Illegal states unrepresentable. The overall direction is good: lint policy is now typed substrate data instead of emitter-local conditionals. The same non-blocking gap remains, though: the shared PatternBindingRule still lets a Rust target pick Python-only or not-applicable variants, even though the Rust consumer says they should never happen. That means one illegal target-specific state is still representable and only caught indirectly.

Facts flow forward. Strong. pattern_bindings now flows all the way from src/v3/std/clean_emission.dag to src/v3/spec/rust.dag, through TargetSyntaxCache, into CleanEmissionContractBinding, and finally into render_branch_pattern plus the deny-unused regression test. That is exactly the forward flow I want to see. The other contract fields are authored but not yet consumed, but the diff is honest about that in src/v3/spec/rust.dag:897-903, the stage doc, and the roadmap, so I read those as tracked scaffolds rather than silent drops.

Coproduct dissolution. Acceptable here. The new substrate coproducts in src/v3/std/clean_emission.dag are explicitly classified inline, so this PR is not sneaking in unlabeled substrate enums. I do not see a concrete dissolve-now miss in this diff. The only thing I’d keep watching is whether some of these rule enums stay true target-capability vocabulary or later turn out to be rollout-state scaffolds in disguise, but that is not a concrete defect in this PR.

Single-authority. Good. src/v3/std/clean_emission.dag is the canonical rule vocabulary, src/v3/spec/rust.dag is the Rust selection, and the emitter reads that selection. The Rust-side binding enums are implementation-local parse shims, not a second competing authority.

API-level enforcement. Improved, but not all the way there. The pilot moves behavior under a typed contract, which is the right API boundary. The remaining issue is the same one from fail-closed / illegal-state checks: Rust-invalid PatternBindingRule values are still constructible and then normalized later instead of being rejected at the API boundary. Today the backstop is test coverage; I’d prefer the type/build path to stop it.

Design question. Is CleanEmissionContract supposed to be a truly target-neutral vocabulary that each target validates, or should each target reject impossible rule choices at contract-parse time? What’s at stake is whether this contract becomes enforced substrate fact or advisory metadata: if Rust can legally carry Python-only pattern-binding rules and just “do something reasonable,” future clean-emission rules will tend to copy that shape.

Verdict. APPROVE_WITH_COMMENTS. This is real forward progress: it turns E-5 from prose into substrate data, wires a genuine Rust consumer, fixes the wildcard-payload aliasing edge case, and adds a meaningful end-to-end deny-unused regression. My only substantive ask is to make the Rust-side contract parsing reject impossible PatternBindingRule variants instead of silently treating them as EmitBindingAlways.

LOOP HEALTH: converging — this round converts review feedback into a real substrate type, a real emitter consumer, and a real regression test, and the remaining debt is explicitly tracked instead of being smuggled in.


View conversation

@briansrls

Copy link
Copy Markdown
Contributor Author

codex · gpt-5.4 · 8fa579a5

✅ Review (blocking: 0, non-blocking: 2+/0-)

Non-blocking — Strengths

  • src/v3/std/clean_emission.dag The clean-emission policy now lives as a first-class substrate contract with explicit coproduct receipts, which keeps the rule set in specs instead of emitter-local convention.
  • src/v3/compiler/src/emit_rust.rs The Rust pilot reads rust_clean_emission through typed declaration ids and decides underscore emission via a structural port-liveness walk rather than a post-render text hack.

ROADMAP — Verified

  • Lane 1 Stage 1c PR 1 scope: The diff matches the staged plan: contract type system, rust_clean_emission, cached accessor, Rust consumer, and targeted tests all land in the same PR.
  • Tracked deferrals: The remaining Go/Python pilots and post_emit_verifier gate are named in ROADMAP with concrete scope and thresholds, so the unread contract fields are tracked rather than silent debt.

✅ The prior blocking bug is fixed and I do not see a remaining blocker in the changed lines.

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

codex · gpt-5.4 · 8fa579a5

✅ Review (blocking: 0, non-blocking: 2+/0-)

Non-blocking — Strengths

  • src/v3/std/clean_emission.dag The clean-emission policy now lives as a first-class substrate contract with explicit coproduct receipts, which keeps the rule set in specs instead of emitter-local convention.
  • src/v3/compiler/src/emit_rust.rs The Rust pilot reads rust_clean_emission through typed declaration ids and decides underscore emission via a structural port-liveness walk rather than a post-render text hack.

ROADMAP — Verified

  • Lane 1 Stage 1c PR 1 scope: The diff matches the staged plan: contract type system, rust_clean_emission, cached accessor, Rust consumer, and targeted tests all land in the same PR.
  • Tracked deferrals: The remaining Go/Python pilots and post_emit_verifier gate are named in ROADMAP with concrete scope and thresholds, so the unread contract fields are tracked rather than silent debt.

✅ The prior blocking bug is fixed and I do not see a remaining blocker in the changed lines.

@briansrls

briansrls commented Apr 17, 2026 •

Copy link
Copy Markdown
Contributor Author

Meta-Review (Loop Health)

According to a document from April 17, 2026, the review loop on PR #514 is making real progress, but the marginal value of another same-shaped round on this PR is now low. ⚖️ SHIP_WITH_DEBT. The loop has already converted E-5 from prose into substrate data, wired one real consumer, fixed the only blocking bug raised in the first browser round, and left the remaining debt explicit rather than hidden.

Loop summary.

Visible history shows N = 3 review rounds, K = 1 codex review, L = 2 browser reviews, over ~37 minutes from 17:55 UTC to 18:32 UTC on 2026-04-17. M commits is not recoverable from the attached materials, but the record proves at least 2 reviewed implementation revisions: round 1 blocks on the multi-field _ @ ... path, round 2 explicitly says the wildcard-payload aliasing edge case was fixed, and codex then confirms the prior blocker is gone.

Forward progress evidence.

This loop did not just polish substrate. It created a real consumer path: the warning policy now lives as typed clean-emission contract data instead of emitter folklore, the Rust emitter reads that contract through typed ids / structural liveness, and the round added a meaningful deny-unused regression target. That is exactly the kind of progress the thesis wants: emission becoming a consequence of validated intent rather than ad-hoc emitter interpretation, and a consumer/test now exists to make the rule real instead of ornamental.

The loop also shows real “findings → structure” behavior, not just patching. The first round’s blocker was fixed immediately; the second round downgrades the remaining gap to non-blocking and explicitly frames the result as “typed substrate data + genuine Rust consumer + regression test.” That is forward progress, not churn.

Debt accumulation evidence.

There is still debt. The clean-emission contract is structurally cross-target, but only one family is actually live in this PR: pattern_bindings. The browser review says the other fields are “authored but not yet consumed,” with follow-up work named in the stage doc and roadmap. That is good accounting, but it is still scaffolding, not dissolved structure. So the loop is paying off some debt while also carrying forward a bounded pile of new scaffolds.

More importantly, one class of issue survived multiple rounds without graduating. Both browser reviews call out the same structural hole: Rust still accepts EmitPrefixedUnderscoreWhenUnused and NotApplicablePatternBinding even though the Rust consumer says those states should be impossible. The loop fixed the concrete instance (_ @ ...) but not the broader rule “target-invalid contract variants must be rejected in build().” By the project’s own invariant, that is still a downstream normalization of a missing upstream fact, not the final shape.

Cheating signal.

Low, and mostly honest. The implementer is not hiding compromises. The unread fields are explicitly called out as tracked follow-up work, and codex verifies that the remaining Go/Python pilots and post-emit-verifier gate are named in the roadmap with concrete scope and thresholds. That is “cheating with accounting,” which is acceptable. The only visible “good enough for now” move is the Rust-side acceptance-and-normalization of impossible rule variants. That shortcut is real, but it is small, explicit, and already called out twice in review rather than smuggled through silently.

Meta-verdict — ⚖️ SHIP_WITH_DEBT

This is not a stagnant loop, so not 🔁 PAUSE_AND_REGROUP. It is not substrate failure, so not 🔄 REVERT_AND_RETHINK. It is also not the best use of another round to keep grinding this PR, so not 📈 KEEP_ITERATING. The loop already banked the high-value win: one new structural rule, one real consumer, one real regression, one blocking bug removed. The remaining issue is narrow, explicit, and better handled as a focused follow-up: make Rust reject impossible PatternBindingRule variants at contract-parse time before Go/Python pilots copy the loophole. Merge this PR, carry the tracked debt honestly, and spend the next round on that structural follow-up instead of another pass over the same pilot.


View conversation

@briansrls

briansrls commented Apr 17, 2026 •

Copy link
Copy Markdown
Contributor Author

ChatGPT Review

Grounded against THESIS.md’s “spec IS the implementation” / layer-opacity claims and the active six-principle review checklist. Pasted text

chatgpt-review-77913d96-8e80-42…

Principle audit.

Fail-closed. Good. The earlier Rust-invalid-rule hole looks closed now: CleanEmissionContractBinding::build and parse_pattern_binding_rule fail through EmitError::MissingTargetSyntax / EmitError::MalformedTargetSyntax instead of normalizing bad states in rendering (src/v3/compiler/src/emit_rust.rs:878-963). The new regression test that mutates rust_clean_emission to bad constructors is exactly the right receipt that this is structural, not just comment-level (src/v3/compiler/src/emit_rust.rs:5114-5159), and the roundtrip deny-unused test in m1_3_emit_rust_test.rs:400-440 proves the live path, not just the string shape.

Illegal states unrepresentable. Good for the live Rust slice. The substrate contract remains target-neutral, but Rust now narrows it immediately into the implementation-local PatternBindingRuleBinding::{EmitBindingAlways, EmitUnderscoreWhenUnused}, and the other PatternBindingRule constructors are rejected before rendering (src/v3/compiler/src/emit_rust.rs:497-500, 897-963). That fixes the earlier issue without pushing more target-specific shape into substrate.

Facts flow forward. Strong overall. pattern_bindings now flows from src/v3/std/clean_emission.dag:12-20, 38-42 into src/v3/spec/rust.dag:897-923, through TargetSyntaxCache and Dag::rust_clean_emission_spec() (src/v3/compiler/src/dag.rs:1071-1077, 1320-1329, 1686-1688), and then into render_branch_pattern plus focused tests (src/v3/compiler/tests/m1_3_emit_rust_test.rs:355-440). The remaining contract fields are explicitly staged in the spec comment and roadmap, so I read those as tracked bridges rather than silent drops. My one NON-BLOCKING concern is that the emitter then re-derives the meaning of the constructor by name (src/v3/compiler/src/emit_rust.rs:883-889, 918-960) even though the structural fact is already present as FieldValue::Variant.constructor.

Coproduct dissolution. Acceptable here. The new substrate coproducts in src/v3/std/clean_emission.dag are at least explicitly classified inline, and I do not see a concrete dissolve-now miss in the live Rust pilot itself. No review-stop on this principle.

Single-authority. Mostly good: the vocabulary lives once in src/v3/std/clean_emission.dag, and Rust’s concrete policy lives once in src/v3/spec/rust.dag. My one NON-BLOCKING concern is the consumer side: emit_rust.rs still teaches itself which rule constructor means what by hardcoded std names (src/v3/compiler/src/emit_rust.rs:883-889, 918-960). That is not duplicate source data yet, but it is a second semantic authority channel, and it weakens the thesis’s “spec-driven emission” story. chatgpt-review-df6fb205-f950-4a…

chatgpt-review-e2618b50-c6c8-45…

API-level enforcement. Improved, but this is where the remaining comment lives. The API now prevents Rust-invalid rule choices from reaching rendering, which is the important fix. But it still does not make name-coupled contract decoding impossible: a future contributor can add more named_variant_id(..., "...") parsing and the type system will not stop them. I would rather see bootstrap/cache surface the relevant DeclarationIds, or a generated typed binding, so the emitter can only dispatch on structural identity and never on spellings. That aligns better with “every dependency is a substrate fact.” chatgpt-review-df6fb205-f950-4a…

Design question. Should clean-emission contract decoding be declaration-id driven end-to-end, or is hardcoded constructor-name decoding an acceptable boundary here? What is at stake is whether this new contract becomes a real example of “spec is the implementation,” or whether the remaining seven rules and the Go/Python pilots will copy a parser shape that is still coupled to std spellings and therefore still needs Rust edits when the vocabulary evolves. chatgpt-review-a1b4677e-cc7d-4b…

Verdict. APPROVE_WITH_COMMENTS. The prior concern is fixed, the Rust pilot is real, and the tracked deferrals in spec/rust.dag and ROADMAP.md look honest rather than smuggled. My one ask is to get contract decoding off hardcoded rule names before that pattern gets cloned across the other rule fields and the Go/Python pilots.

LOOP HEALTH: converging — this round fixes the earlier hole, lands a real substrate contract plus a real Rust consumer plus focused regression tests, and the remaining debt is explicitly named in the roadmap instead of being hidden.


View conversation

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

codex · gpt-5.4 · e5337c97

⚠️ Review (blocking: 1, non-blocking: 1+/0-)

BLOCKING (1)

Root Cause

  • src/v3/std/clean_emission.dag Verifier invocation was modeled, but verifier verdict semantics were not -> add an explicit output policy or verifier-result shape before Go/Python consumers land.

Non-blocking — Strengths

  • src/v3/compiler/src/emit_rust.rs The Rust pilot keeps E-5 structural: it parses rust_clean_emission.pattern_bindings up front, rejects Rust-invalid variants fail-closed, and decides underscore emission from port reachability rather than string rewriting.

⚠️ The Rust pilot itself looks clean, but the new PostEmitVerifier substrate type is underspecified for the already-declared Go follow-up and should be fixed before other targets consume it.

Comment thread src/v3/std/clean_emission.dag Outdated
// 🟢 TERMINAL. Declares the external verifier that validates
// emitted source for this target. `syntax_only` distinguishes
// shallow checks (gofmt, py_compile) from full compiles (rustc).
// `expected_exit_code` is explicit because some verifiers use a

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

BLOCKING: PostEmitVerifier cannot represent Go's declared verifier semantics: gofmt -l returns exit 0 for both clean and dirty files, so the contract collapses pass and fail into the same state instead of encoding the verifier result structurally (illegal states unrepresentable / single authority).

@briansrls

Copy link
Copy Markdown
Contributor Author

codex · gpt-5.4 · e5337c97

⚠️ Review (blocking: 1, non-blocking: 1+/0-)

BLOCKING (1)

Root Cause

  • src/v3/std/clean_emission.dag Verifier invocation was modeled, but verifier verdict semantics were not -> add an explicit output policy or verifier-result shape before Go/Python consumers land.

Non-blocking — Strengths

  • src/v3/compiler/src/emit_rust.rs The Rust pilot keeps E-5 structural: it parses rust_clean_emission.pattern_bindings up front, rejects Rust-invalid variants fail-closed, and decides underscore emission from port reachability rather than string rewriting.

⚠️ The Rust pilot itself looks clean, but the new PostEmitVerifier substrate type is underspecified for the already-declared Go follow-up and should be fixed before other targets consume it.

@briansrls

briansrls commented Apr 17, 2026 •

Copy link
Copy Markdown
Contributor Author

Meta-Review (Loop Health)

According to a document from April 17, 2026, this loop is making real forward progress, not merely shifting debt — but only for the Rust pilot slice. Meta-verdict: ⚖️ SHIP_WITH_DEBT. chatgpt-review-00470c06-ad31-44…

Loop summary.

N = 5 recorded review rounds, M ≈ 3 visible reviewed pushes / code states, K = 2 codex reviews, L = 3 browser reviews, over about 3 hours 31 minutes (17:55 → 21:26 UTC on 2026-04-17). The attached material does not expose exact Git commit count, so “~3” is the honest approximation from the review history rather than from PR metadata. chatgpt-review-00470c06-ad31-44…

Forward progress evidence.

This loop accomplished three concrete things.

First, it graduated a recurring complaint into an invariant-shaped contract. The review history explicitly describes E-5 moving from prose into substrate data instead of staying emitter folklore. chatgpt-review-00470c06-ad31-44…

Second, it enabled a real consumer. The Rust emitter now reads rust_clean_emission, drives render_branch_pattern from that contract, and has targeted deny-unused coverage; both browser and codex reviews call out that this is a genuine emitter consumer, not just new substrate with no user. That satisfies the “consumers define correctness” test. chatgpt-review-00470c06-ad31-44…

chatgpt-review-00470c06-ad31-44…

Third, the loop fixed a real blocker structurally. The first browser pass blocked on the invalid _ @ ... multi-field pattern path; later reviews explicitly say that blocker is fixed and that Rust-invalid PatternBindingRule variants are now rejected fail-closed before rendering. That is actual convergence, not comment churn. chatgpt-review-00470c06-ad31-44…

chatgpt-review-00470c06-ad31-44…

Debt accumulation evidence.

The loop also accumulated debt, but it did so openly.

The new clean-emission contract surface is bigger than the live consumer surface. Browser review notes that the other contract fields are “authored but not yet consumed,” with only pattern_bindings live in this PR. That is net new scaffold surface. chatgpt-review-00470c06-ad31-44…

The final codex review adds a more important debt signal: PostEmitVerifier invocation was modeled, but verifier verdict semantics were not. That means the loop moved beyond the Rust pilot into a second substrate fact whose semantics are not yet nailed down. chatgpt-review-00470c06-ad31-44…

There is also a recurring pattern across rounds: the same class keeps reappearing one layer outward. First the loop found the wildcard-alias bug, then Rust-invalid rule variants, then name-coupled constructor decoding, then verifier semantics. That is still forward motion, but it shows the marginal value of another same-PR round is dropping. chatgpt-review-00470c06-ad31-44…

chatgpt-review-00470c06-ad31-44…

chatgpt-review-00470c06-ad31-44…

Cheating signal.

The implementer is documenting compromises, not hiding them. The browser review explicitly says the remaining deferrals in spec/rust.dag and ROADMAP.md look “honest rather than smuggled,” which is exactly what you want from a scoped compromise. chatgpt-review-00470c06-ad31-44…

But the newest fixes are mixed in kind. The Rust slice is structural: typed contract, fail-closed parsing, structural liveness walk, deny-unused tests. The remainder is still “good enough for now”: constructor meaning is still re-derived by name in the consumer, and the verifier contract is modeled ahead of a complete consumer. That is cheating with accounting, not silent cheating — acceptable for a pilot, but it should stop here. chatgpt-review-00470c06-ad31-44…

chatgpt-review-00470c06-ad31-44…

Why this is not PAUSE_AND_REGROUP.

Because the loop is not stagnant. It already did the healthy-loop thing: it converted a repeated issue into a structural contract/invariant and attached a real consumer to it. This is not “same bug, new file” whack-a-mole. chatgpt-review-00470c06-ad31-44…

chatgpt-review-d9d83974-1da0-4a…

Why this is not KEEP_ITERATING.

Because the main uncertainty of this PR is already resolved: the Rust pilot is real. Another round on this same PR is likely to buy adjacent cleanup — verifier semantics, constructor-ID decoding, Go/Python rollout — rather than a new consumer or a new invariant. That is diminishing-return iteration. chatgpt-review-00470c06-ad31-44…

chatgpt-review-00470c06-ad31-44…

Why this is not REVERT_AND_RETHINK.

Because the substrate direction is not wrong. The thesis wants target facts expressed as structural declarations and consumed by emitters, and this PR is doing exactly that for one live slice. The remaining gap is rollout completeness, not a wrong base architecture. chatgpt-review-21b0b362-ab67-4b…

Meta-verdict: ⚖️ SHIP_WITH_DEBT

Ship this PR as the Rust proof-point. The loop has already earned the merge by enabling a real consumer and banking E-5 as a structural contract. Do not keep grinding this same PR until post_emit_verifier and the other seven rules are perfect. Those are already explicit follow-ups; pushing them through more rounds on this branch will mostly shift debt sideways instead of creating new correctness. The bluff has already been called: the consumer is real.


View conversation

@briansrls

Copy link
Copy Markdown
Contributor Author

ChatGPT review in progress... (view conversation)

Check back in ~30 minutes for the full review.

@briansrls
briansrls merged commit 6257624 into main Apr 17, 2026
3 checks passed
briansrls added a commit that referenced this pull request Apr 18, 2026
Wires `go_clean_emission: CleanEmissionContract` into `emit_go` so
the emitter's pattern-binding render path dispatches on the
contract's `pattern_bindings` rule instead of unconditionally
emitting `{name} := v.field;` in every type-switch arm.

Because Go's unused-local is a compile error (not a warning like
Rust's `unused_variables`), this pilot also stress-tests the E-5
contract: the realization of `EmitUnderscoreWhenUnused` on Go is
*elision* (drop the per-arm binding, and drop `switch v :=` when no
arm binds), not rename-to-underscore. The contract held across that
stricter surface without a shape change — Rust↔Go is "same
structural intent, statement-level vs pattern-level site," which is
why the doc split cleanly separates the second target from Python's
genuinely distinct surface.

## Implementation

- `src/v3/spec/go.dag` — `go_clean_emission` populated with all 8
  rule fields (Go-correct defaults: `NoWrapping`,
  `EmitUnderscoreWhenUnused`, `IncludeOnlyReferenced`,
  `ExplicitReturnKeyword`, `EmitLetBindingAlways`,
  `NotApplicableMatchArmBody`, tab-indent `CorrectionStyle`,
  `gofmt -l` + `RequireEmptyStdout` verifier). Seven rules are
  authored-but-unread under the same rollout comment the Rust pilot
  (PR #514) used — consumers wire in as Lane 1d/1e lands.
- `src/v3/compiler/src/dag.rs` — `TargetSyntaxCache::go_clean_emission`
  + `go_clean_emission_spec()` accessor + bootstrap name lookup.
- `src/v3/compiler/src/emit_go.rs` — `CleanEmissionContractBinding`
  (Go-valid slice rejects `EmitPrefixedUnderscoreWhenUnused` and
  `NotApplicablePatternBinding` at parse time, fail-closed);
  `render_sum_branch` elides the per-arm binding prefix when port
  liveness reports the payload isn't consumed; drops `switch v :=`
  to plain `switch any(...).(type)` when no arm binds so `v` never
  becomes the new unused local; `port_is_consumed_from` is a
  structural BFS over producer→input edges mirroring the Rust pilot
  (no textual scan of rendered arm bodies).
- `src/v3/compiler/tests/m1_3_emit_go_test.rs` —
  `emit_go_unused_payload_binding_is_elided` asserts the elision +
  switch-header drop;
  `emit_go_used_payload_binding_is_preserved` proves elision is
  keyed on port liveness per arm, not blanket suppression;
  `emit_go_unused_payload_binding_compiles` (`#[ignore]`) runs
  emitted Go through `go run` when a Go toolchain is available.

## Doc split

PR 1 (#514) deferral in `src/v3/ROADMAP.md:527` bundled Go + Python
as PR 2 with a 1-week yellow flag. That framing hid a structural
distinction:

- Rust and Go share the same contract-level intent — swap the
  unused identifier for the discard form of the target's binding
  site. Rust realizes it inside the pattern (`_` on the slot);
  Go realizes it one level up (elide the statement + drop `v :=`
  when no arm binds). Near-isomorphic at the contract surface.
- Python's pattern-bind doesn't emit the binding as part of the
  pattern at all. Distinct surface — the axis that actually
  validates the contract generalizes beyond "same pattern slot,
  different symbol."

Bundling them obscured that signal. This PR splits:

- PR 2 (this one): Go pilot. Second-target proof.
- PR 3 (deferred, M): Python pilot. True target-agnostic
  validation — if Python needs a contract-SHAPE change rather
  than a new instantiation, that's a structural finding worth
  flagging rather than absorbing silently.

Updates `src/v3/ROADMAP.md:527` (deferral row) and
`docs/phase1-lane2-clean-emission-invariant.md` §Scope to reflect
the three-PR split with the Rust↔Go isomorphism vs
Python-distinct-surface rationale spelled out.

## Sample emitted Go (end-to-end)

Source:

    type BoxedInt = Boxed(Int) | Empty
    fn ignore_payload(b: BoxedInt) -> Int = match b {
      Boxed(value) => 0, Empty => 1
    }

Before this PR, emitted:

    switch v := any(p0).(type) {
      case Boxed: value := v._0; return 0   // Go error: value declared but not used
      case Empty: return 1
    }

After:

    switch any(p0).(type) {
      case Boxed: return 0
      case Empty: return 1
    }

Clean under Go's unused-local rule by construction, not by
`gofmt`-aware post-processing.

## Test plan

- [x] `cargo test -p v3-compiler --test m1_3_emit_go_test` — 3
      passed, 2 ignored (new tests + preserved pilot + Go roundtrip
      gated on toolchain)
- [x] `cargo test --workspace --exclude v2-compiler-tests` — all
      green
- [x] `cargo test -p v2-compiler-tests` — 415 passed
- [x] `cargo clippy --all-targets -- -D warnings` — clean
- [x] `cargo fmt --all --check` — clean
- [x] `ROADMAP.md` + design doc split landed with the PR per E-6
      "same-PR consumer" discipline

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
briansrls added a commit that referenced this pull request Apr 18, 2026
* Lane 1 Stage 1c PR 2: Go pilot for E-5 clean-emission contract

Wires `go_clean_emission: CleanEmissionContract` into `emit_go` so
the emitter's pattern-binding render path dispatches on the
contract's `pattern_bindings` rule instead of unconditionally
emitting `{name} := v.field;` in every type-switch arm.

Because Go's unused-local is a compile error (not a warning like
Rust's `unused_variables`), this pilot also stress-tests the E-5
contract: the realization of `EmitUnderscoreWhenUnused` on Go is
*elision* (drop the per-arm binding, and drop `switch v :=` when no
arm binds), not rename-to-underscore. The contract held across that
stricter surface without a shape change — Rust↔Go is "same
structural intent, statement-level vs pattern-level site," which is
why the doc split cleanly separates the second target from Python's
genuinely distinct surface.

## Implementation

- `src/v3/spec/go.dag` — `go_clean_emission` populated with all 8
  rule fields (Go-correct defaults: `NoWrapping`,
  `EmitUnderscoreWhenUnused`, `IncludeOnlyReferenced`,
  `ExplicitReturnKeyword`, `EmitLetBindingAlways`,
  `NotApplicableMatchArmBody`, tab-indent `CorrectionStyle`,
  `gofmt -l` + `RequireEmptyStdout` verifier). Seven rules are
  authored-but-unread under the same rollout comment the Rust pilot
  (PR #514) used — consumers wire in as Lane 1d/1e lands.
- `src/v3/compiler/src/dag.rs` — `TargetSyntaxCache::go_clean_emission`
  + `go_clean_emission_spec()` accessor + bootstrap name lookup.
- `src/v3/compiler/src/emit_go.rs` — `CleanEmissionContractBinding`
  (Go-valid slice rejects `EmitPrefixedUnderscoreWhenUnused` and
  `NotApplicablePatternBinding` at parse time, fail-closed);
  `render_sum_branch` elides the per-arm binding prefix when port
  liveness reports the payload isn't consumed; drops `switch v :=`
  to plain `switch any(...).(type)` when no arm binds so `v` never
  becomes the new unused local; `port_is_consumed_from` is a
  structural BFS over producer→input edges mirroring the Rust pilot
  (no textual scan of rendered arm bodies).
- `src/v3/compiler/tests/m1_3_emit_go_test.rs` —
  `emit_go_unused_payload_binding_is_elided` asserts the elision +
  switch-header drop;
  `emit_go_used_payload_binding_is_preserved` proves elision is
  keyed on port liveness per arm, not blanket suppression;
  `emit_go_unused_payload_binding_compiles` (`#[ignore]`) runs
  emitted Go through `go run` when a Go toolchain is available.

## Doc split

PR 1 (#514) deferral in `src/v3/ROADMAP.md:527` bundled Go + Python
as PR 2 with a 1-week yellow flag. That framing hid a structural
distinction:

- Rust and Go share the same contract-level intent — swap the
  unused identifier for the discard form of the target's binding
  site. Rust realizes it inside the pattern (`_` on the slot);
  Go realizes it one level up (elide the statement + drop `v :=`
  when no arm binds). Near-isomorphic at the contract surface.
- Python's pattern-bind doesn't emit the binding as part of the
  pattern at all. Distinct surface — the axis that actually
  validates the contract generalizes beyond "same pattern slot,
  different symbol."

Bundling them obscured that signal. This PR splits:

- PR 2 (this one): Go pilot. Second-target proof.
- PR 3 (deferred, M): Python pilot. True target-agnostic
  validation — if Python needs a contract-SHAPE change rather
  than a new instantiation, that's a structural finding worth
  flagging rather than absorbing silently.

Updates `src/v3/ROADMAP.md:527` (deferral row) and
`docs/phase1-lane2-clean-emission-invariant.md` §Scope to reflect
the three-PR split with the Rust↔Go isomorphism vs
Python-distinct-surface rationale spelled out.

## Sample emitted Go (end-to-end)

Source:

    type BoxedInt = Boxed(Int) | Empty
    fn ignore_payload(b: BoxedInt) -> Int = match b {
      Boxed(value) => 0, Empty => 1
    }

Before this PR, emitted:

    switch v := any(p0).(type) {
      case Boxed: value := v._0; return 0   // Go error: value declared but not used
      case Empty: return 1
    }

After:

    switch any(p0).(type) {
      case Boxed: return 0
      case Empty: return 1
    }

Clean under Go's unused-local rule by construction, not by
`gofmt`-aware post-processing.

## Test plan

- [x] `cargo test -p v3-compiler --test m1_3_emit_go_test` — 3
      passed, 2 ignored (new tests + preserved pilot + Go roundtrip
      gated on toolchain)
- [x] `cargo test --workspace --exclude v2-compiler-tests` — all
      green
- [x] `cargo test -p v2-compiler-tests` — 415 passed
- [x] `cargo clippy --all-targets -- -D warnings` — clean
- [x] `cargo fmt --all --check` — clean
- [x] `ROADMAP.md` + design doc split landed with the PR per E-6
      "same-PR consumer" discipline

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* docs: track PR 2.5 (dissolve PatternBindingRule name-keyed recovery)

Insert the ROADMAP deferral row for the PR 2.5 follow-up flagged in
#520 ChatGPT review: `parse_pattern_binding_rule` currently uses
`named_variant_id(dag, "PatternBindingRule", "EmitBindingAlways")`
et al. in both emit_rust.rs and emit_go.rs to recover the variant
`DeclarationId`s before comparing. That's the anti-pattern Q5 of
`feedback_substrate_principle_audit` calls out — typed at the
compare site, name-keyed at the resolution site; three parallel
recovery paths will exist if Python copies the pattern.

Recovery: typed `PatternBindingRuleVariants` cache on `Dag`,
populated at bootstrap end, read by all three emitters.

Marked as prerequisite for the Python pilot so emit_python.rs never
writes the name-lookup path in the first place (dissolve before the
consumer multiplies).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* Align go_clean_emission defaults to DB-4 rule table

Codex review on d0ab9b2 flagged expression_wrapping as a BLOCKING
DB-4 drift. Audit catches variable_bindings too — both authored
from emitter-current-behavior assumptions rather than consulted
from the pinned design table.

- expression_wrapping: NoWrapping → WrapOnlyInOperandPosition
  DB-4 (docs/design-clean-emission-contract.md:79) pins Go here:
  wrap when precedence matters, omit at statement terminus. Go is
  precedence-capable, so NoWrapping was a wrong classification —
  that variant is reserved for precedence-tolerant targets like
  SPICE/English.
- variable_bindings: EmitLetBindingAlways → EmitLetUnderscoreWhenUnused
  DB-4:127 says for Go "compile error on unused — emitter MUST
  reference or use `_ =` assignment." EmitLetBindingAlways would
  emit `{name} := {expr}` unconditionally, which for Go means
  unused locals fail to compile. The rule that realizes DB-4's
  stated behavior is EmitLetUnderscoreWhenUnused (discard form
  when the identifier is not referenced).

Both fields remain authored-but-unread per the existing rollout
comment (consumers wire in as Lane 1d/1e lands); this commit only
moves the AUTHORED values from "aspirational match-current-emitter"
to "DB-4 faithful."

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@briansrls
briansrls deleted the bright-bat-665 branch June 1, 2026 18:40
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.

1 participant