Repository navigation
Phase 3+4: unify TCO and service rendering across Python/Go - #407
Conversation
ChatGPT ReviewI’d request changes before merge. Directionally, this PR is moving the codebase the right way: the thesis explicitly prefers collapsing parallel mechanisms into one shared mechanism, and the roadmap says Track 13 Phase 3–4 is exactly shared TCO/block plus shared service/transport unification. So the intent is thesis-aligned. The problems are in two places where the new shared layer still breaks “zero bugs by construction” discipline instead of fully enforcing it. chatgpt-review-50986074-fe83-45… chatgpt-review-6628f7e9-7ace-4f…
chatgpt-review-ac91d315-265a-4a…
chatgpt-review-ac91d315-265a-4a…
chatgpt-review-6628f7e9-7ace-4f…
chatgpt-review-d9d8edba-efec-4e… I do not see an obvious control-flow bug in the TCO extraction itself. The chatgpt-review-8d6cafa8-a460-48… ROADMAP.md is not modified in this diff, so there is no roadmap-text drift to flag. Directionally, the work matches Track 13 Phase 3–4; it just needs to land in a way that preserves the stronger fail-closed and single-authority rules the thesis requires. chatgpt-review-6628f7e9-7ace-4f… References: PR diff, std-library.dag, THESIS.md, INVARIANTS.md, ROADMAP.md |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e03356ddc
ℹ️ 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".
| let use_tco = is_tco_eligible(name: name, body: body, registry: registry, source_index: si) | ||
| if use_tco { | ||
| let body_str = emit_go_typed_tco_body(texpr: body, fn_name: name, params: params, registry: registry, scope: body_scope, depth: 1) | ||
| let body_str = emit_unified_tco_body( |
There was a problem hiding this comment.
This module now calls emit_unified_tco_body, emit_unified_service_def, and emit_unified_transport_dispatch, but those symbols are not added to the import v2.compiler.emit { ... } list. Because this file uses explicit named imports, compiling the .dag sources for bootstrap/stage1 will hit unresolved-name diagnostics at these call sites instead of generating code.
Useful? React with 👍 / 👎.
| let use_tco = is_tco_eligible(name: name, body: body, registry: registry, source_index: si) | ||
| if use_tco { | ||
| let body_str = emit_py_typed_tco_body(texpr: body, fn_name: name, params: params, registry: registry, scope: body_scope, depth: depth + 1) | ||
| let body_str = emit_unified_tco_body( |
There was a problem hiding this comment.
This file was switched to emit_unified_tco_body/service unification calls, but the new shared symbols are not included in import v2.compiler.emit { ... }. With named-import semantics, that leaves these references out of scope and causes compile-time unresolved-name failures when the .dag source is bootstrapped.
Useful? React with 👍 / 👎.
Phase 3: TCO unification - Create emit_unified_tco_expr + emit_unified_tco_body in shared emitter - Delete 16 per-language TCO wrapper functions (8 Python + 8 Go) - Keep only language-specific match/pattern renderers as callbacks - Rust TCO untouched (deferred to Phase 6, ownership complexity) Phase 4: Service/transport orchestration unification - Create emit_unified_service_def, emit_unified_operation_method, emit_unified_transport_dispatch in shared emitter - Add ServiceSyntax type to LanguageSpec (self_param, receiver, return wrapping, class vs top-level methods) - Delete emit_py_operation_method, emit_py_transport_call, emit_go_operation_method, emit_go_transport_call - Rewrite emit_py_service_def and emit_go_service_def as thin adapters wiring callbacks to unified orchestration - Transport implementations (REST/shell/file/local) stay per-language Net: 20 per-language functions deleted, 5 shared functions added. Per-language emitter files shrink; shared emitter grows as authority. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ChatGPT ReviewI’d request changes before merge. Directionally, this PR is moving in the right thesis direction. The thesis explicitly prefers collapsing parallel mechanisms into one shared mechanism when they are structurally the same, and Track 13 says Phase 3–4 is exactly shared TCO/block plus shared service/transport unification. So the goal is aligned with “zero bugs by construction.” chatgpt-review-f8c0a255-91f1-42… chatgpt-review-6628f7e9-7ace-4f… What blocks merge for me is that the new shared layer still leaves one fail-open path, one real Python codegen bug, and one modeling choice that looks like a second partial authority rather than a clean extension of the existing language-spec model.
In the new shared dispatcher, the final chatgpt-review-3f0853c3-5aa9-42… The root-cause fix is to pass a closed transport kind or lowered transport variant into shared emit and exhaustively match it.
This is not just a modeling concern; it is a real codegen bug from the refactor. A minimal fix is to indent each rendered method before joining, or join on
The new That clashes with the modeling/invariant guidance:
chatgpt-review-3f0853c3-5aa9-42… chatgpt-review-3439aebf-71c3-46… The reference pattern in the std library is already better.
The only test change in the diff is chatgpt-review-6628f7e9-7ace-4f… chatgpt-review-d9d8edba-efec-4e… For this PR I would expect at least:
What looks good: the TCO extraction itself seems mechanically faithful. Lifting the orchestration into shared helpers while leaving backend-specific match/block rendering in Go and Python is the right shape for Phase 3. Also, the diff does include regenerated stage0 mirrors, so I do not see a stage0-freshness omission here. ROADMAP.md is not modified in this PR, so there is no roadmap-text drift to flag. Directionally, though, this is aligned with Track 13’s stated next steps: Phase 3 is TCO/block unification and Phase 4 is service/transport unification. My objection is not to the phase choice; it is that the implementation still leaves one unsound fallback and one non-structural service model, which cuts against the thesis requirement that emitted code be the mechanical consequence of validated facts. chatgpt-review-6628f7e9-7ace-4f… chatgpt-review-3439aebf-71c3-46… References: PR diff, std-library.dag, THESIS.md, INVARIANTS.md, MODELING.md, ROADMAP.md |
…sures The .dag compiler generates Rust closures without type annotations. When closures are stored in `let` bindings or passed through callback chains, Rust's type inference fails (E0282). Also fixes Rc<dyn Fn> vs impl Fn mismatches (E0277) when callbacks pass through callbacks. Fixes: - Add missing imports: authored_name in 05_emit.dag, unified TCO + service functions in Python/Go emitter imports - Restructure TCO match callbacks: match functions reconstruct recurse_expr locally (call emit_py_typed_expr / emit_go_typed_expr directly) instead of receiving it as a callback-through-callback - Add unified_tco_recurse named helper to avoid let-binding closure type inference failure - Add emit_py_typed_tco_body / emit_go_typed_tco_body as named wiring functions (avoids let-binding closures at call sites) - Add emit_py_transport_body / emit_go_transport_body to avoid deeply nested closure chains in service rendering Stage0 regenerated and fixed-point verified. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
853e679 to
1b5368b
Compare
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
cursor · composer-2 · 1b5368b8
Review (INVARIANTS: 3, MODELING: 2+/2-, ROADMAP: 0✓/0!)
INVARIANTS — Violations (3)
ROOT CAUSE ANALYSIS
src/v2/05_emit.dagTypeEnv’s field name and carrier type were not migrated insrc/v2/04_env.dag(and call sites that buildTypeEnv) before emit started emittingsource_index→ literals and accessors refer to a non-existent field → the compiler cannot prove the graph from env construction to emission sound; fix by atomically renaming and retypingTypeEnv(singleNewlineIndex?vs per-file map is a modeling choice) and updatingauthored_name/every env builder in one change set, aligned with M4 (avoid string-keyed maps as identity proxies only after Node/span authority is explicit).src/v2/00_core.dagUpstreamauthored_name_atand derived*_atAPIs still assume a file-keyedMap<String, NewlineIndex>→ emit was retargeted to a single optional index without updating those definitions → every downstream_atcall is ill-typed; fix by either (a) implementingauthored_name_at(source_index: NewlineIndex?, node: Node)(and threading module identity from Node/source graph) or (b) keepingMap<String, NewlineIndex>inTypeEnvand passingenv.source_indiceseverywhere, then deleting the half-migration.src/v2/05_emit.dagDispatch was extracted without threading the same inputs as the old per-targetemit_*_transport_callfunctions → either registry was needed for parity and dropped, or the parameter is speculative; removeregistryfrom the unified signature and all call sites, or pass it intorender_rest/render_shellif those backends still need item lookup—preserve single-authority data flow per INVARIANTS “no duplicate representations.”
MODELING — Strengths
src/v2/languages.dagServiceSyntaxextendsLanguageSpecwith the same pattern asRecordLitSyntax/TcoSyntax, parameterizing receiver and error-return shape so Python/Go share orchestration instead of parallel method templates—matches THESIS concept unification and M4 (emit reads declared language facts).src/v2/05_emit.dagemit_unified_tco_expr/emit_unified_service_defcentralize TCO and service control flow for Python/Go with callbacks for patterns and transports—reduces “no parallel implementations” drift between backends.
MODELING — Improvements
src/v2/languages.dagrust_specanddag_specnow carryservice_syntaxstrings that05_emit_rust.dagdoes not consume (Rust service emission remains backend-specific per comments), risking a second authoritative description of Rust service signatures; either drive Rust fromservice_syntaxor omit Rust/dag entries until consumed soLanguageSpecstays a single authority.src/v2/05_emit.dagCollapsingMap<String, NewlineIndex>toNewlineIndex?removes string keys (M4 direction) but must be modeled as an explicit “compilation unit source context” fact carried withTypeEnv/InferScopeso cross-moduleident_spanrecovery is either impossible by construction or still sound; document that tradeoff as a type field, not only comments inorder_typed_call_args.
ROADMAP — Unclaimed
- M4 / shared emission direction: This PR materially shares TCO and service orchestration across Python/Go and adds
ServiceSyntaxtoLanguageSpec, advancing the mechanical-emission story, butROADMAP.mdis not updated to record that partial step toward spec-driven emission.
Strong intent—unifying Python/Go TCO and service emission behind LanguageSpec aligns with “zero bugs by construction” and anti-parallel-implementation—but the PR must land atomically with the core/source-index API migration or it breaks the build and splits authority between specs and Rust.
| fn empty_emit_scope() -> InferScope { | ||
| InferScope { | ||
| type_env: TypeEnv { bindings: empty_map(), recursive_types: [], recursive_type_set: empty_map(), inductive_fields: empty_map(), source_indices: empty_map() }, | ||
| type_env: TypeEnv { bindings: empty_map(), recursive_types: [], recursive_type_set: empty_map(), inductive_fields: empty_map(), source_index: none }, |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| ExprError { kind: _, message: message } => | ||
| emit_error_expr(message: message, target: target) | ||
| ExprVar { binding_kind: _ } => emit_ident(name: expr_var_name_at(texpr: expr, source_indices: source_indices), target: target) | ||
| ExprVar { binding_kind: _ } => emit_ident(name: expr_var_name_at(texpr: expr, source_index: source_index), target: target) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Violations (could not place on specific lines):
|
…urce_indices Review feedback from PR #407: 1. ServiceSyntax → structural coproducts (not string fragments): - ServiceMethodStrategy = SelfInParams | ExternalReceiver - ServiceReturnStrategy = ArrowReturn | ErrorTupleReturn Emitter dispatches structurally (like VisibilitySpec pattern). 2. Transport dispatch fail-closed: - Explicit is_local_transport check before local fallback - Unrecognized transport → emit_error_expr (not silent local) 3. source_index → source_indices alignment: - Bulk rename to match main's Map<String, NewlineIndex> API - Fixes 268 stale parameter references across 3 emitter files Stage0 regenerated, fixed-point verified, ratchet passes (≤421). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Review (INVARIANTS: 3, MODELING: 2+/2-, ROADMAP: 0✓/0!) INVARIANTS — Violations (3) ROOT CAUSE ANALYSIS
MODELING — Strengths
MODELING — Improvements
ROADMAP — Unclaimed
Strong intent—unifying Python/Go TCO and service emission behind |
ChatGPT ReviewI’d request changes before merge. Directionally, this is the right move. The thesis explicitly says parallel mechanisms should collapse when they are really the same mechanism, and Track 13 calls out Phase 3 as TCO/block unification and Phase 4 as service/transport unification. So the goal is thesis-aligned. chatgpt-review-f8c0a255-91f1-42… chatgpt-review-3439aebf-71c3-46… What blocks merge for me is:
In the diff, some new/shared call sites switch to singular forms like
This may be inherited behavior, but this PR now centralizes it into the shared path, so the blast radius is larger. I’d treat this as a real blocker. The fix is to indent each rendered method before joining, or have the operation-method helper return already class-indented output for significant-whitespace targets.
Adding The issue is that those facts are only consumed for Python and Go right now. Rust still keeps backend-specific service emission, while chatgpt-review-cb197540-fedc-40…
One thing this PR gets right: the new shared dispatcher now fails closed on an unknown transport instead of silently falling through, which is better than the old pattern. But it still classifies transport inside emit by calling I would not block merge on that by itself, because it is clearly moving in the right direction. But it does mean this PR does not fully satisfy Track 13’s “single emitter reads data, never decides” end state yet. The invariants are explicit that when emit still has to branch on proxy facts, the upstream boundary is still insufficient. chatgpt-review-3439aebf-71c3-46… chatgpt-review-cb197540-fedc-40…
The diff only strengthens chatgpt-review-6628f7e9-7ace-4f… The minimum set I’d want:
A smaller note: the local What looks good:
ROADMAP.md is not modified in this diff, so there is no roadmap-text drift to flag. Directionally, though, this PR is clearly aligned with the next stated Track 13 steps. My merge recommendation is “not yet” because of the incomplete References: PR diff, std-library.dag, THESIS.md, INVARIANTS.md, MODELING.md, ROADMAP.md |
1. Remove unused registry param from emit_unified_transport_dispatch and its callers (emit_py_transport_body, emit_go_transport_body). Dead surface on the causal path from registry to transport emission. 2. Fix TypeEnv construction: source_indices: none → empty_map(). TypeEnv declares source_indices: Map<String, NewlineIndex>, not optional. 3. Align remaining source_index → source_indices mismatches from rebase. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · 56fc38b2
Review (INVARIANTS: 4, MODELING: 1+/1-, ROADMAP: 0✓/1!)
INVARIANTS — Violations (4)
ROOT CAUSE ANALYSIS
src/v2/04_env.dagTypeEnv still defines one authority, source_indices, but the refactor renamed only some emitter consumers to source_index; that boundary split propagates into backend wrappers as mixed field reads and helper calls. Fix it upstream by keeping one canonical field or accessor on TypeEnv and updating all emit consumers atomically, which is the M2 single-authority fix instead of more downstream patches.src/v2/00_core.dagExprFieldAccess already carries structural authority through FieldSummary and FieldAccessStyle, but this emit path ignores it and re-derives call-vs-field from binding heuristics; that gap propagates into stage0 as ordinary value receivers being rewritten as thunks. Fix it upstream by having both simple and typed field emission consume one receiver-kind authority from the IR, which is the M8 and No parallel implementations fix.src/v2/05_emit.dagThe shared service path models nested blocks as raw String payloads from render_transport_body and render_service_fields instead of compositional block fragments, so indentation is patched downstream one line at a time and Python loses structure. Fix it upstream by returning List or a BlockEmitState-like fragment and letting the shared emitter assemble indentation from BlockSyntax, which is the M1 and Track 13 compositional fix.
MODELING — Strengths
src/v2/languages.dagServiceMethodStrategy and ServiceReturnStrategy turn receiver placement and result shape into closed LanguageSpec data, which composes with VisibilitySpec and ItemKeywords and moves backend decisions out of per-language emitter code.
MODELING — Improvements
src/v2/languages.dagSelfInParams { self_param: String } and ErrorTupleReturn { error_type: String } still store rendered tokens like &self and error; model receiver mode and result carrier shape structurally so Track 13 Phase 5-6 borrow work composes instead of hiding in strings.
ROADMAP — Incomplete
- Track 13 Phase 3-4: The diff clearly centralizes TCO and service/transport orchestration, but the mixed source_index boundary and string-based Python block rendering mean Phase 3-4 are not mechanically correct end-to-end yet.
ROADMAP — Unclaimed
- Track 13 Phase 3-4: This PR is the roadmap's stated next work for Track 13, but ROADMAP.md still says Phase 3-4 are only ready / Next.
The PR moves Track 13 in the right direction, but it still violates single-authority boundaries and relies on downstream string and heuristic emission patches, so it does not yet meet the thesis's zero-bugs-by-construction bar.
| let ret_type = emit_node_type(n: inferred, target: Go, source_indices: scope.type_env.source_indices) | ||
| let body_scope = build_params_scope(scope: scope, params: params) | ||
| let si = scope.type_env.source_indices | ||
| let si = scope.type_env.source_index |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| let ret_str = emit_py_inferred(inferred: inferred, source_indices: scope.type_env.source_indices) | ||
| let body_scope = build_params_scope(scope: scope, params: params) | ||
| let si = scope.type_env.source_indices | ||
| let si = scope.type_env.source_index |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| } else { | ||
| v2_rt::concat(v2_rt::concat(emit_simple_expr(&b, &target, &source_indices), ".".to_string()), emit_ident(f.clone(), target.clone())) | ||
| { | ||
| let base_is_fn = match (*b.expr_data.clone()).clone() { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| let eff_transport = effective_operation_transport(op_node: op_node, fallback: fallback_transport) | ||
| let body = render_transport_body(eff_transport, op_text, si, method_depth + 1) | ||
| let sig = concat(spec.items.async_prefix, spec.items.func_keyword, " ", receiver_str, method_name, | ||
| "(", all_params, ")", ret_str) |
There was a problem hiding this comment.
Invariant violation: For significant-whitespace targets, emit_unified_operation_method pastes a multi-line transport body behind a single make_indent(...), so Python service methods lose indentation after the first newline and emitted code becomes structurally invalid.
|
Review (INVARIANTS: 4, MODELING: 1+/1-, ROADMAP: 0✓/1!) INVARIANTS — Violations (4) ROOT CAUSE ANALYSIS
MODELING — Strengths
MODELING — Improvements
ROADMAP — Incomplete
ROADMAP — Unclaimed
The PR moves Track 13 in the right direction, but it still violates single-authority boundaries and relies on downstream string and heuristic emission patches, so it does not yet meet the thesis's zero-bugs-by-construction bar. |
ChatGPT ReviewI reviewed the PR diff against the thesis and the attached docs. The direction is right: Track 13 explicitly wants shared TCO and shared service/transport traversal, and the thesis wants emission to be mechanical translation from declared target facts rather than per-backend logic. The new
In this diff, That is a straight source-of-truth violation. The roadmap says stage0 is a derived artifact from Under the thesis, once stage0 and What I’d do instead: land the source-side authorities first, regenerate stage0, and let the generated Rust reflect them. For this PR specifically, that means:
The shared and backend emitters now use This is exactly the kind of broken causal link the thesis is trying to make impossible: not a modeling disagreement, just a refactor introducing an inconsistent boundary name. chatgpt-review-92e85b0f-6149-4f…
But
So the shared emitter is still deciding target syntax instead of just reading it. That cuts against both the roadmap goal that codegen decisions become spec-referenced lookups and the invariant that emission should translate annotated structure rather than branch on target-specific syntax in code. I would treat this as “good refactor shape, incomplete factoring.” The next step is to push one level farther:
Right now the PR has moved the classification into
The stage0 Rust emitter now special-cases optional record fields in request bodies: required fields go into the initial JSON object, optional fields are only inserted when the value is Under the invariants, this is a textbook Root-Cause Depth issue: if the emitter needs to know whether Related concern, lower confidence but still worth flagging: the new
A couple of positives:
Net: good directional refactor, not merge-ready. My merge blockers are:
If those are fixed, the underlying Phase 3/4 extraction looks worth landing. |
…trap Fix remaining source_index → source_indices field accesses in emitter files (TypeEnv.source_index → .source_indices, empty_map() for TypeEnv construction). Bootstrap stage0 with new ServiceMethodStrategy and ServiceReturnStrategy coproduct type imports. Also fixes: go_source_extension missing () call in generated languages.rs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ChatGPT ReviewI reviewed the PR diff against the thesis and attached docs. Overall: the direction is right. This PR removes parallel Go/Python implementations by extracting shared TCO and shared service/transport rendering into chatgpt-review-4f2b7385-3463-4d… I would still push back on one design issue before merge, and call out two follow-up concerns.
But the PR then uses I would fix that in this PR by making the no-self form explicit, not encoded as an empty string.
The new shared layer is a good extraction, but
That means the PR centralizes backend logic, but it does not yet meet the stronger invariant that emission is a pure translation from chatgpt-review-4f2b7385-3463-4d… I would not call this a functional bug in the narrow sense, because it preserves the old Go behavior. But it is important architectural truth-in-labeling: this PR is a good Phase 3/4 consolidation, not full thesis completion for Track 13.
But because the old Go/Python helpers treated the catch-all as local, this is also a behavior change. That is good only if every intended local path is already structurally represented as a real local transport. The repo docs already call out transport/service modeling as an area where heuristics still need to become structural facts. chatgpt-review-ba1c1f33-cd59-4f… chatgpt-review-31be40db-1433-47… The problem is that the only test change I see is At minimum I’d want one representative emitted-output test per backend covering:
I do not see chatgpt-review-ba1c1f33-cd59-4f… Net: good refactor, right direction, not quite at the thesis bar yet. My recommendation:
|
1. Add TODO comments on hardcoded Go receiver syntax and error tuple format in emit_unified_operation_method — tracked as Phase 5 items for receiver_template and return_template on LanguageSpec. 2. Document the M5 codegen gap: the .dag compiler does not generate `use` statements for new coproduct types used in match arms. The bootstrap imports for ServiceMethodStrategy/ServiceReturnStrategy are manually added with a comment explaining why. 3. Fix go_source_extension() missing parens in generated languages.rs. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · 78dc26ca
Review (INVARIANTS: 3, MODELING: 1+/1-, ROADMAP: 0✓/2!)
INVARIANTS — Violations (3)
ROOT CAUSE ANALYSIS
src/v2/languages.dagUpstream gap: the compiler-local LanguageSpec still lacks a single structural authority for service method headers and returns, so 05_emit.dag introduces ServiceMethodStrategy and ServiceReturnStrategy and then finishes the job with hardcoded Go syntax; project the existing std.languages function, error, and type syntax into v2.compiler.languages, or add one ServiceDefinitionSyntax in std/, so shared emission only instantiates declared templates (M1, M9).src/v2/05_emit_rust.dagUpstream gap: .dag-to-Rust import derivation is still losing type dependencies needed by generated match-arm code, which forces a downstream hand-edit in src/v2/stage0/src/v2_compiler_emit.rs; fix import emission at the generator, regenerate, and keep stage0 free of manual bootstrap exceptions.src/v2/05_emit.dagUpstream gap: the refactor abstracted transport rendering behind callbacks but preserved the old backend-shaped signature, so a global registry bag still propagates through shared service orchestration and obscures the real dependency set; remove registry from emit_unified_operation_method and emit_unified_service_def, and let backend closures capture it only if a transport renderer actually needs it.
MODELING — Strengths
src/v2/languages.dagServiceMethodStrategy and ServiceReturnStrategy model the current backend split as closed coproducts instead of open string flags, which is the right M4 shape for a finite strategy set.
MODELING — Improvements
src/v2/languages.dagThese new compiler-local types are only a partial proxy for function, error, and service syntax already grounded in dsl/std/languages.dag; a more compositional model would project that existing authority, or add one ServiceDefinitionSyntax in std/, so 05_emit.dag composes templates instead of branching on bespoke service enums (M1, M9, layer alignment).
ROADMAP — Incomplete
- Track 13 Phase 4: The PR clearly advances service and transport unification, but hardcoded pointer-receiver and error-tuple syntax in 05_emit.dag mean Phase 4 still falls short of Track 13's LanguageSpec-driven end state.
- Bootstrap: The manual bootstrap imports added in src/v2/stage0/src/v2_compiler_emit.rs contradict ROADMAP.md's claim that stage0 content is generated from .dag source except for compiler_tests.rs.
ROADMAP — Unclaimed
- Track 13 Phase 3: The diff introduces emit_unified_tco_expr and emit_unified_tco_body and moves Python and Go through the shared non-pattern TCO path, but ROADMAP.md still says Phase 3 is only next work.
The PR moves real Python and Go TCO and service logic into shared paths, but it still breaks the thesis at two root points: shared service emission is not yet pure LanguageSpec translation, and stage0 bootstrap is still being repaired downstream by hand.
| let method_depth = match spec.service_method { | ||
| SelfInParams { self_param: _ } => 1 | ||
| ExternalReceiver { var_name: _ } => 0 | ||
| } |
There was a problem hiding this comment.
Invariant violation: emit_unified_operation_method hardcodes pointer-receiver and error-tuple syntax in shared emit code, violating Emission is translation, not decision-making and No duplicate representations.
| @@ -33,7 +33,9 @@ pub use crate::v2_compiler_infer::{InferScope, build_params_scope, extend_scope} | |||
| pub use crate::v2_compiler_infer_emit_info::{TypeSummary, EmitGraphInfo}; | |||
| pub use crate::v2_compiler_artifact::{RenderTarget}; | |||
| use crate::v2_compiler_artifact::RenderTarget::{Rust, Python, Go, Dag}; | |||
There was a problem hiding this comment.
Invariant violation: The bootstrap-only manual imports in a generated stage0 file violate No parallel implementations; stage0 should compile because the generator emitted the right imports, not because the artifact was patched.
| target: RenderTarget, | ||
| registry: Map<String, ItemInfo>, | ||
| env: TypeEnv, | ||
| render_transport_body: fn(Node, String, Map<String, NewlineIndex>, Int) -> String |
There was a problem hiding this comment.
Invariant violation: emit_unified_operation_method keeps registry in the shared service API even though the new shared path never reads it, violating Minimal information per interface.
|
Review (INVARIANTS: 3, MODELING: 1+/1-, ROADMAP: 0✓/2!) INVARIANTS — Violations (3) ROOT CAUSE ANALYSIS
MODELING — Strengths
MODELING — Improvements
ROADMAP — Incomplete
ROADMAP — Unclaimed
The PR moves real Python and Go TCO and service logic into shared paths, but it still breaks the thesis at two root points: shared service emission is not yet pure LanguageSpec translation, and stage0 bootstrap is still being repaired downstream by hand. |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · 8636e09e
Review (INVARIANTS: 4, MODELING: 1+/1-, ROADMAP: 1✓/1!)
INVARIANTS — Violations (4)
ROOT CAUSE ANALYSIS
src/v2/05_emit.dagMissing upstream service-call boundary fact: the new shared transport contract drops op_node/params before rendering begins -> local transport renderers only know a name and must fabricate zero-arg calls -> pass the operation node or rendered argument list through render_transport_body/emit_unified_transport_dispatch so local transport emission translates the declared signature mechanically instead of guessing (MODELING.md M8/M9).src/v2/05_emit.dagMissing upstream service-signature authority: LanguageSpec now distinguishes SelfInParams vs ExternalReceiver and ArrowReturn vs ErrorTupleReturn, but it still does not declare how those forms render -> 05_emit.dag fills the gap with target-specific string assembly -> move receiver and service-return rendering into declared LanguageSpec/std.languages syntax data and have emit_unified_operation_method apply templates mechanically (MODELING.md M7/M9).src/v2/stage0/src/v2_compiler_emit.rsMissing upstream import derivation in the .dag->Rust generator for coproduct variants used in generated match arms -> the generated Rust artifact needs hand-maintained use lines to compile -> teach the generator to derive those imports from the referenced type definitions, then regenerate and delete the manual patch so stage0 remains a pure consequence of source (MODELING.md M7/M9).
MODELING — Strengths
src/v2/languages.dagServiceMethodStrategy models receiver placement as a closed coproduct in LanguageSpec, which composes better than branching on target names in the emitters.
MODELING — Improvements
src/v2/languages.dagServiceReturnStrategy is too thin to be the authority: per M9 it should compose from existing function/error language facts or a richer service-signature product, because the current model still leaves 05_emit.dag hardcoding receiver and Go tuple syntax.
ROADMAP — Verified
- Track 13 Phase 3: The diff adds emit_unified_tco_expr and emit_unified_tco_body and rewires Go/Python TCO entry points to use them, so TCO recursion did move into the shared emitter.
ROADMAP — Incomplete
- Track 13 Phase 4: Service/transport orchestration moved into 05_emit.dag, but shared service emission is not yet mechanically complete because receiver syntax, Go error-tuple syntax, and local-transport argument forwarding are still missing from the upstream language contract.
ROADMAP — Unclaimed
- Track 13 progress: This PR materially advances Track 13 beyond Phase 3-4 ready, but ROADMAP.md was not updated to record the landed Phase 3 work and the remaining Phase 4 gaps.
The PR moves Track 13 forward, but it still violates zero-bugs-by-construction where the shared emitter makes target decisions from missing upstream facts instead of translating a complete declared model.
| target: RenderTarget, | ||
| registry: Map<String, ItemInfo>, | ||
| env: TypeEnv, | ||
| render_transport_body: fn(Node, String, Map<String, NewlineIndex>, Int) -> String |
There was a problem hiding this comment.
Invariant violation: emit_unified_operation_method hands transport renderers only (transport, op_name, source_indices, depth), so local transport emission cannot forward the operation's declared parameters, violating Explicit boundary contracts.
| // rendering template so the shared emitter reads, never decides. | ||
| let receiver_str = match spec.service_method { | ||
| SelfInParams { self_param: _ } => "" | ||
| ExternalReceiver { var_name: v } => concat("(", v, " *", service_name, ") ") |
There was a problem hiding this comment.
Invariant violation: The shared emitter hardcodes Go receiver syntax as "(" + var + " *" + service_name + ")" instead of reading declared language data, violating Emission is translation, not decision-making.
| let ret_type = emit_node_type(n: resolved_type(n: op_node), target: target, source_indices: si) | ||
| let ret_str = match spec.service_return { | ||
| ArrowReturn => concat(spec.items.return_arrow, ret_type) | ||
| ErrorTupleReturn { error_type: et } => concat(" (", ret_type, ", ", et, ")") |
There was a problem hiding this comment.
Invariant violation: The shared emitter hardcodes Go's "(T, error)" return form instead of deriving it from the language contract, violating Emission is translation, not decision-making.
| pub use crate::v2_compiler_artifact::{RenderTarget}; | ||
| use crate::v2_compiler_artifact::RenderTarget::{Rust, Python, Go, Dag}; | ||
| pub use crate::v2_compiler_languages::{LanguageSpec, TestConventions, ServiceFieldTemplates, BlockSyntax, TcoSyntax, ExpressionSemantics, IfValueForm, VisibilitySpec, NamingCase, ReservedWordStrategy, StringInterpSyntax, InterpStyle, EscapePair, RecordLitSyntax, ImportRule, language_spec_for_target, is_string_like, test_conventions_for_target, target_keyword, binop_symbol, wrap_shared_type, TestNameStyle, ImportTrigger}; | ||
| // Bootstrap: ServiceMethodStrategy + ServiceReturnStrategy imports are added |
There was a problem hiding this comment.
Invariant violation: The generated stage0 file now advertises manual bootstrap imports for coproduct variants, violating No parallel implementations.
|
Review (INVARIANTS: 4, MODELING: 1+/1-, ROADMAP: 1✓/1!) INVARIANTS — Violations (4) ROOT CAUSE ANALYSIS
MODELING — Strengths
MODELING — Improvements
ROADMAP — Verified
ROADMAP — Incomplete
ROADMAP — Unclaimed
The PR moves Track 13 forward, but it still violates zero-bugs-by-construction where the shared emitter makes target decisions from missing upstream facts instead of translating a complete declared model. |
ChatGPT ReviewI reviewed PR #407 diff against the thesis first. Against the project thesis, the direction is correct: this PR removes parallel Go/Python mechanisms and pushes more emission behavior into shared paths and My verdict: good direction, but I would request changes before merge. What this PR gets rightThis is clearly aligned with Track 13’s next steps. The roadmap says Track 13 Phase 3 is TCO/block unification and Phase 4 is service/transport, with the target being a single emitter that reads chatgpt-review-04d8bd3b-04d4-45… The new I also like that transport dispatch is now more fail-closed. The shared transport helper no longer silently defaults everything unknown to “local”; it checks local transport explicitly and emits an error for unrecognized transport kinds. That is much closer to the invariants’ “no fallbacks that fabricate” bar. chatgpt-review-04d8bd3b-04d4-45… Main issue 1:
|
Move ServiceMethodStrategy/ServiceReturnStrategy dispatch into accessor functions in languages.dag (service_self_param, service_receiver_str, service_method_depth, service_methods_inside_class, service_return_str). The emitter calls accessors instead of importing + pattern-matching the coproduct types directly. This eliminates the stage0 bootstrap import gap — no manual stage0 edits needed, freshness check passes cleanly. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The .dag compiler generates data-constant references without call syntax in struct literals (go_source_extension vs go_source_extension()), causing a Rust type mismatch. Inline the string literal ".go" directly in go_spec's scaffold to avoid the codegen gap. Stage0 regenerated, fixed-point verified. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ChatGPT ReviewI reviewed this PR against the thesis first, not just against local style. The bar here is “mechanical translation from validated intent,” not merely “less duplicated backend code.” The thesis explicitly collapses coercion into emission and says target specs should determine the output; the roadmap and invariants sharpen that into “the emitter reads specs and never decides.” chatgpt-review-045e5713-4361-4e… chatgpt-review-9604e653-fff7-4a… chatgpt-review-ad86e4b5-e7cc-4d… Net: the TCO half is directionally good. The service half is not thesis-clean yet. I would request changes before merge. 1) Invariant violationsA.
|
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · 128de513
Review (INVARIANTS: 2, MODELING: 1+/1-, ROADMAP: 0✓/0!)
INVARIANTS — Violations (2)
ROOT CAUSE ANALYSIS
src/v2/languages.dagThe upstream gap is that dsl/std/languages.dag still has no first-class service-definition syntax authority, so this PR adds compiler-local helpers that reconstruct receiver and return forms from strings; that missing lower-layer fact propagates into emit_unified_operation_method, which stays "shared" only by calling compiler logic instead of reading extdep data. Add a service-definition syntax model in dsl/std/languages.dag (or extend FunctionSyntax/TypeDefinitionSyntax/ErrorHandlingSyntax), populate it in language extdeps, and let 05_emit.dag do lookup-only translation per M1/M9 and the layer model.src/v2/languages.dagThe upstream gap is still the bootstrap/import path for extdep data constants, so the PR patches over the missing authority by copying the extension into go_spec; that duplicate then propagates through scaffold_for_target() into every Go path computation. Fix import emission so extdep data constants remain single-authority, then delete the inline literal per M2/M7.
MODELING — Strengths
src/v2/languages.dagServiceMethodStrategy uses a closed coproduct instead of target-name branching, which is the right M4/M9 shape for method-form variation.
MODELING — Improvements
src/v2/languages.dagdag_spec currently uses SelfInParams { self_param: "" }, which shows the model is missing a NoReceiver case; add that variant and move service-definition syntax into dsl/std/languages.dag/extdeps so absence is structural, not a string sentinel.
ROADMAP — Unclaimed
- Track 13 Phase 3-4: The diff clearly advances the planned TCO and service/transport unification, but ROADMAP.md still says Phase 3-4 are only "ready/next" instead of reflecting this as in-progress or partial work with remaining blockers.
The PR moves Phase 3/4 work toward a single emitter, but it still violates the zero-bugs-by-construction thesis where service-definition syntax and Go scaffold facts are reconstructed in compiler code instead of flowing from single-authority language data.
| fn service_receiver_str(spec: LanguageSpec, service_name: String) -> String { | ||
| match spec.service_method { | ||
| SelfInParams { self_param: _ } => "" | ||
| ExternalReceiver { var_name: v } => concat("(", v, " *", service_name, ") ") |
There was a problem hiding this comment.
Invariant violation: service_receiver_str/service_return_str still encode Go receiver and (T, error) syntax with string concatenation in compiler code rather than declared language data, violating Emission is translation, not decision-making and No duplicate representations.
| manifest_file: "go.mod", | ||
| module_init_file: null, | ||
| source_file_extension: go_source_extension, | ||
| source_file_extension: ".go", |
There was a problem hiding this comment.
Invariant violation: go_spec.scaffold.source_file_extension is re-hardcoded as ".go" even though extdeps.languages.go.emit.go_source_extension already defines that fact, violating No duplicate representations.
|
Review (INVARIANTS: 2, MODELING: 1+/1-, ROADMAP: 0✓/0!) INVARIANTS — Violations (2) ROOT CAUSE ANALYSIS
MODELING — Strengths
MODELING — Improvements
ROADMAP — Unclaimed
The PR moves Phase 3/4 work toward a single emitter, but it still violates the zero-bugs-by-construction thesis where service-definition syntax and Go scaffold facts are reconstructed in compiler code instead of flowing from single-authority language data. |
1 participant
Summary
emit_unified_tco_expr+emit_unified_tco_bodyreplace 16 per-language wrapper functions. Match/pattern renderers stay as language-specific callbacks.emit_unified_service_def,emit_unified_operation_method,emit_unified_transport_dispatchreplace per-language dispatch and method rendering. NewServiceSyntaxtype on LanguageSpec parameterizes method signatures (self_param, receiver, error return wrapping). Transport implementations (REST/shell/file/local) stay per-language.ServiceSyntax(6 fields) added to LanguageSpec.Test plan
cargo test --workspace --exclude v2-compiler-tests— 20 passedcargo test -p v2-compiler-tests— 401 passedcargo clippy --all-targets -- -D warnings— cleancargo test -p v2-compiler-tests strict_compile_diagnostic_count -- --ignored— 0 diagnostics (ratchet stable)🤖 Generated with Claude Code