Repository navigation
CG: unify TLC-2 dual authority, mark transport/config complete - #338
Conversation
SimpleMethodSpec co-locates template + wraps_result on one type (same pattern as HigherOrderMethodSpec). Both rust_method_templates() and rust_method_wraps_result() now derive from rust_simple_method_specs — single authority, no parallel maps. Transport/config centralization marked complete. Remaining per-backend rendering is inherent language differences addressed by 3→1 homomorphism. Flag Go op_children filter inconsistency (Rust/Python don't filter). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ChatGPT ReviewBased on the uploaded diff, this is a good refactor in principle, but I would not rubber-stamp it as-is. The core refactor is directionally right. Moving from two parallel Rust method tables to one My main concern is the stage0 implementation of that refactor. In the generated Rust, the old cached So my review summary would be: request changes for the caching regression, even though the authority unification itself is good. On roadmap alignment, I’d split the claims: TLC-2 is plausibly complete in the narrow sense used by this PR. If the claim is “the last local parallel authority for simple Rust method template wrapping is gone,” then yes, the diff does that. But TLC-2 is not obviously complete in the broad architectural sense implied by the label “runtime bridge signature derivation.” This PR unifies one Rust extdep sub-authority; it does not fully solve the broader “same type/coercion authority as emission” problem across the lane. I would not object to the checkbox if you intend the narrow reading, but I would avoid overselling it in prose. Related method/coercion modeling work is still explicitly open in the roadmap ( I do not think “Transport/config complete” is justified by this diff. The only transport/config code change here is a new comment in On compositional modeling quality, the A couple smaller code-review notes: The new tests are useful, but they only prove parity between the list and the derived maps. They do not prove that the actual consumer behavior stayed correct. I’d add one direct consumer-level test around The Go I don’t see a new invariant violation in the authority refactor itself. The invariant problem I do see is the likely stage0 performance regression from uncached repeated derivation. And I’d push back on the transport/config completion claim. Everything else looks reasonable. |
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 1, MODELING: 2+/2-, ROADMAP: 1✓/1!)
INVARIANTS — Violations (1)
ROOT CAUSE ANALYSIS
src/v2/05_emit_go.dagUpstream, there is no shared structural authority defining which service children are emitted as operations (gap in shared emit traversal modeling), so Go applies an ad-hoc backend filter while other emitters use fullitem.children; this drops zero-arg ops in Go and creates backend-inconsistent emitted APIs; fix it by defining one shared predicate/list in a common emitter authority (e.g., via sharedservice-shape dispatch in05_emitor04_resolve) and remove backend-local filtering (MODELING M8/M3).
MODELING — Strengths
dsl/extdeps/languages/rust/emit.dagSimpleMethodSpeccentralizes template + wrapping intent in one record and derives both maps from that single authority, which is a clean compositional correction of the prior parallel-map duplication.src/v2/languages.dagSwitchingmethod_templatestorust_method_templates()correctly treats LanguageSpec as a consumer of derived extdep facts.
MODELING — Improvements
dsl/extdeps/languages/rust/emit.dagmethod_nameremains string-keyed; if method names are constrained, model them as a structural enum/coproduct rather than strings and keep this authority reused across language extdeps for clearer algebraic dispatch (MODELING M4).src/v2/languages.dagMirror this constructor-style import pattern across all backends so adding languages cannot accidentally pass data constants instead of canonical derived accessors.
ROADMAP — Verified
- TLC-2: Runtime bridge signature derivation: The PR’s code changes add and consume
SimpleMethodSpec/derived maps plus tests, directly matching the claimed single-authority consolidation.
ROADMAP — Incomplete
- Transport/config: The roadmap marks this lane complete, but this diff still leaves backend divergence in operation-set traversal and no emitted proof that the 3→1 service emission model is fully enforced.
The PR mostly aligns with the single-authority direction and closes the runtime wrap-result duplication, but it still leaves a backend-service traversal inconsistency unresolved.
| @@ -1234,7 +1234,9 @@ fn emit_go_service_def( | |||
| ) -> String { | |||
| let safe_name = sanitize_service_name(name: authored_name(env: env, node: item)) | |||
| let transport = service_fallback_transport(item: item) | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Replace 30 redundant ExprData-matching wrapper functions (15 per backend) with thin delegates that call shared helpers or typed handlers directly via accessor functions. New shared helpers in 05_emit.dag: - emit_expr_var_shared: target-parameterized var emission - emit_expr_field_access_shared: service-receiver check + delegate - extract_string_interp_parts: StringPart extraction from children New accessors in 00_core.dag: - expr_field_access_summary: extract FieldSummary from ExprFieldAccess - expr_method_call_semantics: extract MethodSemantics from ExprMethodCall Net: 159 lines removed from .dag source (-237/+78), 371 lines removed from stage0 (-532/+161). No Rust emission changes (bootstrap safe). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Remove Go-only params filter on service operation children. Rust and Python use item.children unfiltered — Go now matches. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Share cast/index/slice handlers via target-parameterized functions: - emit_typed_cast_shared: type(expr) rendering - emit_typed_index_shared: string/map/list dispatch via IndexingSemantics - emit_typed_slice_shared: string/list dispatch via IndexingSemantics Delete dead code: - emit_go_typed_bin_op (emit_default_bin_op used directly) - emit_go_typed_cast, emit_go_typed_index, emit_go_typed_slice - emit_py_typed_cast, emit_py_typed_index, emit_py_typed_slice Net: 47 lines removed from .dag source (-102/+55). No Rust emission changes (bootstrap safe). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 0, MODELING: 3+/2-, ROADMAP: 0✓/0!)
MODELING — Strengths
src/v2/05_emit.dagPromotes compositional homomorphism by centralizing repeatedExprVar/FieldAccess/Call/Index/Slice/Castlowering into shared helpers, matching theemit_shared_exprauthority-driven pattern from the shared expression contract.dsl/extdeps/languages/rust/emit.dagSimpleMethodSpeclifts method render/wraps_resultmetadata into one layer-2 authority, and folds this once intorust_method_templates()andrust_method_wraps_result(), which is strong M7 single-authority modeling.src/v2/languages.dagBindingmethod_templatestorust_method_templates()keeps LanguageSpec as a consumer of extdep facts instead of owning backend method templates, preserving layer separation.
MODELING — Improvements
src/v2/05_emit.dagCould further improve structural safety by moving the remaining variant-specific mismatch behavior into a single shared dispatch guard so all backends fail uniformly instead of relying on call-site correctness.dsl/extdeps/languages/rust/emit.dagA sharedMethodTemplateSpecpattern betweenSimpleMethodSpecand existingHigherOrderMethodSpecwould reduce schema drift and make future method-family extensions more regular at L2.
This PR materially improves single-authority modeling and removes a prior Go service-traversal bug, with no new invariant violations introduced.
ChatGPT ReviewVerdict: good direction, but I’d request changes before merge. The core TLC-2 refactor is real. Replacing the parallel Rust simple-method tables with one The main thing I would block on is a stage0 performance/cost-model regression. In INVARIANTS So my main requested change is: keep On invariant review more broadly:
For
My one modeling nit is operational: if the dominant downstream operation is “lookup by method name,” the authoritative authored shape can still be a list, but the runtime/emitter shape wants a cached keyed index. Right now the list is good source data but not a good direct lookup representation. Roadmap alignment:
The other concrete code-review concern is
to:
That is only safe if the post-dissolution model now guarantees that every service child is an operation node. If that invariant is true, I want a direct test proving it. If it is not true, this will mis-emit non-operation children as Go methods. This is the other source-level issue I’d want resolved before merge. A few smaller notes:
So: the refactor is directionally right, the |
Phase 1-3 complete: -206 .dag lines, -481 stage0 lines. Remaining phases deferred — Go/Python depth/indent asymmetry (Go: depth:0 + prefix, Python: depth:depth) blocks clean parameterization of TCO, method dispatch, and item handlers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… blocker detail - Tests: 294→316 pass, 41→44 ignored - Hand-maintained files: 9→2 (7 eliminated in PR #316) - Expand depth/indent asymmetry blocker with resolution options - Record PR #338 review feedback (MethodTemplateSpec, method name enum) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 0, MODELING: 2+/2-, ROADMAP: 2✓/1!)
MODELING — Strengths
dsl/extdeps/languages/rust/emit.dagSimpleMethodSpec+ derived helper functions (rust_method_templates(),rust_method_wraps_result()) tighten authority by making method template/wrapping metadata single-source within Rust extdep instead of parallel ad-hoc maps.src/v2/05_emit.dagThe new shared expression helpers (emit_expr_*_shared,emit_typed_*_shared,extract_string_interp_parts) remove duplicated ExprData handling and keep target emitters focused on rendering, which improves compositionality and backend parity.
MODELING — Improvements
dsl/extdeps/languages/rust/emit.dagmethod_nameis still a string key, so this schema remains string-identity-based; define a shared method-template spec enum/family (including HigherOrderMethodSpec) to model dispatch structurally rather than by proxy names (M4/M7 direction).src/v2/05_emit.dagemit_typed_index_sharedandemit_typed_slice_sharedstill require backend-specific recursion-depth threading from callers, so algebraic style facts are not yet fully normalized at the shared layer until depth policy is made an explicit shared strategy.
ROADMAP — Verified
- CG-2 TLC-2 Runtime bridge signature derivation:
src/v2/05_emit_rust.daganddsl/extdeps/languages/rust/emit.dagnow consumert_function_registryandSimpleMethodSpec-derived wraps in one pipeline, matching the roadmap claim that method wrapping is now from a single authority. - CG-3 Transport/config: The PR updates service traversal and transport/type-template sharing points (
service_fallback_transport,compute_service_fields, and corresponding emit paths), and roadmap text now marks this lane complete in this scope.
ROADMAP — Incomplete
- CG-3 3 backends → 1 parameterized homomorphism: Roadmap documents phases 1-3 only; code still has Go depth-asymmetry workarounds (
depth = 0in shared cast/index/slice paths), so full homomorphism is still blocked as stated.
This diff appears to close the previously flagged service traversal divergence and is mostly aligned with CG goals, with remaining gap explicitly limited to documented cross-backend depth/style asymmetry rather than new invariant violations.
ChatGPT ReviewI’d request changes before merge. The core refactor is directionally right. In INVARIANTS My main blocker is the generated-Rust shape of that refactor. In the uploaded diff, INVARIANTS INVARIANTS On invariant review more broadly: I do not see a new duplicate-authority violation in the Pasted markdown For Roadmap-wise, I buy the narrow TLC-2 claim in this PR: the local Rust simple-method dual authority is gone. I would keep that wording narrow, though. The broader algebra/bridge lane is still not fully done; the checked-in roadmap still carries open work around I do not think the That Go change is my second concrete code-review concern. If any non-operation child can still appear under a service node, The new tests in A few smaller notes:
So my final take is: good source-model cleanup, good shared-emitter progress, but not merge-ready as-is. The stage0 lookup caching regression is the real blocker, the Go |
Model block formatting as data per target language: - block_open/close: braces (Go/Rust) vs colon (Python) - else_clause, match_keyword, case_keyword, arm_separator - stmt_terminator: ";" (Rust) vs "" (Go/Python) - significant_whitespace: true (Python) vs false (Go/Rust) Python's whitespace is a language-spec correctness fact. Go/Rust's whitespace is a readability correctness fact. Both modeled as data on LanguageSpec — the emitter reads, never decides. Add Style Emission (SE) exploratory direction to ROADMAP. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
ChatGPT review in progress... (view conversation) |
- PR #337: stage0 100% generated — zero hand-maintained files. main.rs and compiler_tests.rs both emitter-produced. - PR #338: TLC-2 complete (SimpleMethodSpec unifies dual authority), transport/config complete (inherent per-language differences only). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Go emitter now threads depth:depth for sub-expression recursion, matching Rust and Python's eager depth-threading convention. Previously Go used depth:0 + lazy prefix wrapping via wrap_result. Changes (all in 05_emit_go.dag): - emit_go_typed_expr: wrap_result → identity, recurse depth:0 → depth:depth - 9 simple bridge functions: remove make_indent wrapping - 4 compound handlers: remove first-line indent (keep closing brace indent) - 8 functions gain depth:Int parameter (field_access, typed_call, method_call, algebra_method_call, plain_method_call, first_arg, record_lit, string_interp + interp_segment) - 35 depth:0 sites → depth:depth (except test gen + top-level data_def) - 2 containers (block_stmts, init_block_stmts) add per-statement indent Motivation: Go's indentation is a readability/style concern, not language-spec correctness (unlike Python's significant whitespace). Both are modeled as data on BlockSyntax. Aligning the depth strategy unblocks homomorphism Phases 4-6. Output change: Go emitted code has cosmetic whitespace differences only. Remains valid, readable Go. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
SimpleMethodSpectype co-locates template +wraps_resultflag for simple method templates (same pattern asHigherOrderMethodSpec). Bothrust_method_templates()andrust_method_wraps_result()now derive fromrust_simple_method_specs— eliminates paralleldatamaps as dual authority.[x]— remaining per-backend rendering is inherent language differences addressed by the 3→1 homomorphism.op_childrenfilter inconsistency inemit_go_service_def(Rust/Python don't filter by params).method_wraps_result_derived_from_specs,method_templates_derived_from_specs.Test plan
cargo test -p v2-compiler-tests— 316 pass, 0 failcargo clippy --all-targets -- -D warnings— clean./scripts/check-stage0-freshness.sh— fresh🤖 Generated with Claude Code