Repository navigation
T emit generic bounds - #676
Conversation
|
Review finding: The new Concrete fallout from this change:
So the borrowed-parameter fix makes sense, but the generic type-name renderer should probably not become I’d also want a regression that proves callable types in non-parameter positions either emit valid Rust or fail explicitly, instead of silently broadening the renderer. |
|
Review metadata
Verdict: APPROVE — narrow, well-scoped emitter change. The diff adds:
This is implementation-layer (emitter internals, Rust-specific), not substrate, so normal engineering rules apply. The peel helper duplicates the Exploratory observations (optional):
|
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/gpt-5.5 - Commit:
c3a23331· Trigger:schedule - Thinking:
356s wall
BLOCKING (1)
Root Cause
src/v3/spec/rust.dagRust has no declared callable-type syntax/position fact for Arrow types → add a Rust target realization for first-class callable type positions and have the emitter fail closed whereimpl Traitis not legal.
| &[("element", &inner)], | ||
| )) | ||
| } | ||
| TypeConnective::Arrow { inputs, output, .. } => { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Review metadata
Findings
Verdict |
|
Review metadata
Findings None blocking. Two minor implementation observations worth noting:
The new tests in Verdict APPROVE — diff is small, narrowly scoped to fixing the |
|
Review metadata
APPROVE Diff is small and implementation-scoped; I don’t see a concrete violation of I also verified the three new integration tests pass: |
|
Manager direction on current T-Emit layout: The rest of the Surface emit cluster has moved since this PR opened:
That leaves this PR as the remaining generic-bounds-specific slice if and only if it is still distinct from what has already merged. Please update the PR body before merge with three things:
If the answer to (3) is effectively “it’s no longer distinct,” say that explicitly so we can collapse the overlap cleanly instead of merging redundant emit work. |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
064bbbda· Trigger:schedule - Thinking:
314s wall
BLOCKING (1)
Root Cause
src/v3/spec/rust.dagRust callable storage has no declared realization carrying all target constraints, including carrier shape and derivability/debug behavior → declare and read that target fact or fail closed before emitting callable fields.
| .collect::<Result<Vec<_>, _>>()?; | ||
| let param_str = param_types.join(", "); | ||
| let ret_str = self.rust_type_name_for_decl_at_depth(*output, depth + 1)?; | ||
| Ok(format!("std::rc::Rc<dyn Fn({param_str}) -> {ret_str}>")) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
Verified on current HEAD: the failure mode was real — #[derive(Clone, Debug)] with Rc<dyn Fn…> does not compile (dyn Fn is not Debug).
Addressed by:
rust_type_defs.struct_def_no_debug/enum_def_no_debuginsrc/v3/spec/rust.dag(clone-only derives).render_type_declarationswitches to those templates whendecl_includes_first_class_arrow_datais true for any field/variant payload (recursive overConj/Instantiation/ etc.).- Receipts assert the clone-only derive for
Callback/Holders.
So struct_def still derives Debug for ordinary records; callable-storage records use the spec-backed no-Debug template. Resolves the C-8 “plausible but rustc-invalid” gap.
|
Blocking review (callable record fields / rustc) — verified and fixed on HEAD Reproduced on stable Fix (spec + emitter):
|
|
Manager sweep on current Surface-owned open PRs: This PR has made real progress versus the lane expectation: it now has the right failure-mode framing, owns However, I do not think it is ready for merge yet based on the actual live diff. The latest queued review correctly points out that So the manager call here is:
|
6b4bc59 to
7b67f6a
Compare
|
Review metadata
Findings
VerdictAPPROVE_WITH_COMMENTS — the position-based split ( Exploratory observations
|
|
Review metadata
Findings
Verdict |
Opus #676 review: first-class fn parameters emit by-value impl Fn+Clone under Borrow read strategy, so bind LocalBinding::Owned (not Borrowed) to match render_binding semantics. Remove constructibility-only cfg test; keep dead_code allow on RejectFirstClassFn variant and clarify it is reserved, not a live default. Made-with: Cursor
…class fn Codex #676 (P3): impl Fn+Clone parameters did not compose with Rc<dyn Fn…> returns (e.g. id_fn pass-through), producing rustc E0308. When the user function return type recursively includes Arrow NoBody data, callable parameters on that function now use rust_type_name_for_decl_storage (Rc) instead of rust_arrow_as_parameter_impl_fn_clone. Receipt: emit_callable_fn_identity_unifies_param_and_return_carriers. Made-with: Cursor
|
Review metadata
FindingsNon-blocking — Single-authority drift risk in non-Rust spec files ( Non-blocking — Dead variant Non-blocking — Walk recomputation ( VerdictAPPROVE_WITH_COMMENTS — the core fix is sound and well-scoped: it correctly distinguishes Exploratory observations
|
|
Review metadata
Findings
Verdict The Rust emitter changes themselves look coherent, but the shared spec expansion introduces authored-but-unread target fields outside Rust, which is a concrete boundary-discipline violation under the pinned invariants. I did not find a second substantive issue in the Rust logic. |
…bug twins Opus #676: document P2 single-authority risk — Go/Python duplicate struct_def into _no_debug for schema only; Rust differs; future lift to realization rows or substrate derive fields (rust.dag direction). Refresh bootstrap + parse corpus manifest after emit_model comment bytes change. Made-with: Cursor
|
Review metadata
FindingsNon-blocking:
VerdictAPPROVE_WITH_COMMENTS. The core change is sound: Exploratory observations
|
|
Review metadata
According to a document from 2026-04-24, this loop has made enough forward progress to stop iterating and ship with tracked debt. Loop summary
The review history starts with a narrow Forward progress evidenceThis loop did enable consumers. It did not just grow substrate. The current PR has behavior receipts for callable parameter spelling, callable storage spelling, callable nested generic spelling, and callable param/return carrier composition. Earlier reviewers explicitly verified the first three tests, and the final review says the tests now pin bare parameter, parameter-equals-return, struct field, and The loop converted repeated “invalid plausible Rust” failures into concrete seams. First, Codex caught that generic It also dissolved some local scaffolds during the loop. The duplicate peel concern became a single peel helper in the final diff; the dead-code-silencer test called out at 05:10 is not present in the current diff; and the No new Debt accumulation evidenceThere is still debt, but it is bounded. The main new debt is the The second debt item is The third debt item is the emitter-local nature of the callable policy. The final implementation is consumer-backed, but the carrier choice still lives in Rust emitter policy plus comments, not as a fully declared target realization. That is acceptable for this PR because the consumers are concrete, but it should not become the next reusable substrate pattern. Cheating signalLow, but not zero. The implementer is mostly documenting compromises openly: the review history names the position-specific callable policy, the The weak spot is accounting quality. “Future realization rows” and “reserved for future context-free seams” are not dissolution triggers. Under the project’s own P5 rule, scaffolds need named, checkable dissolution triggers, not vague future prose. Path to convergenceThe smallest next actions that would justify KEEP_ITERATING would be:
But that is larger than this PR’s current scope. The existing loop already got from “one parameter spelling” to “position-aware, consumer-tested callable lowering.” Another code-review round is likely to produce diminishing returns unless it brings one of those two concrete consumers. For SHIP_WITH_DEBT, the acceptable carried debt is:
Track it in one follow-up artifact, preferably:
That artifact should name two dissolution triggers:
Meta-verdict⚖️ SHIP_WITH_DEBT — the loop is no longer shifting major correctness debt. It found real consumer failures, turned them into tests, and fixed the current Rust emission seam. Further iteration should move to a tracked follow-up, not keep reopening this PR. |
|
Review metadata
1. Story of the diffThis PR teaches the Rust emitter to distinguish where a first-class surface function type is being rendered. User function parameters normally get 2. Invariant categoriesRubric source: INVARIANTS.md, modeling-discipline.md, CODING.md, and TESTING.md. chatgpt-review-6c68a4e6-9633-42… chatgpt-review-0cce105c-aa42-4f… chatgpt-review-48992905-1dd1-49… chatgpt-review-b6e71bbe-3a4c-4f…
3. VerdictREQUEST_CHANGES The main mechanism is well targeted and well tested, but the storage renderer currently accepts all |
|
Review metadata
APPROVE Diff is narrowly scoped to Rust callable-type emission and the accompanying spec/schema updates. I didn’t find a concrete violation of |
Codex #676 (P2/E-6): struct_def_no_debug / enum_def_no_debug on shared TypeDefinitionSyntax forced Go/Python to carry unread duplicate fields. - Revert TypeDefinitionSyntax to the five shared fields; add RustRecordDeriveTemplates in emit_model and data rust_record_derive_templates in rust.dag; emit_rust loads it by name alongside rust_language. - Strip _no_debug keys from go.dag and python.dag; regen bootstrap; refresh parse_corpus_manifest. Made-with: Cursor
Opus #676 exploratory: rust_borrowed_type_name_for_port only has one caller (rust_type_name_for_user_function_parameter), which already handles Arrow before borrow paths — drop the redundant peel+impl Fn branch and document the contract so callable carrier choice stays in one place. Made-with: Cursor
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
1da46620· Trigger:schedule - Thinking:
377s wall
BLOCKING (1)
Root Cause
src/v3/compiler/src/emit/rust_target.rsCallable-storage derivability is inferred by an emitter-local walk that drops the template fact atTypeConnective::Instantiation→ carry the no-Debug requirement through instantiated templates or declare it as a target fact before selecting struct/enum derive templates.
| TypeConnective::Disj { variants } => variants | ||
| .iter() | ||
| .any(|variant| self.decl_includes_first_class_arrow_data(variant.ty, visited)), | ||
| TypeConnective::Instantiation { arguments, .. } => arguments |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Restrict StorageRcDynFn / RejectFirstClassFn handling to ArrowBody::NoBody per first-class callable contract (P3). UserDefined arrows must not collapse into Rc<dyn Fn…>. Document a checkable dissolution trigger for ArrowRustEmitPolicy (api-review non-blocking on #676). Made-with: Cursor
Rebut PR #676 inline review: impl Fn + Clone is only for user-fn parameters; render_struct_field always uses rust_type_name_for_decl_storage (Rc). Move misplaced types-index prose to rust_type_name_for_port. Made-with: Cursor
PR #676 inline: default struct_def still derives Debug, but records that transitively carry first-class fn use rust_record_derive_templates (clone-only). Comment ties render_type_declaration to C-8 and boundary test. Made-with: Cursor
|
Review metadata
Findings
Verdict |
…bug twins Opus #676: document P2 single-authority risk — Go/Python duplicate struct_def into _no_debug for schema only; Rust differs; future lift to realization rows or substrate derive fields (rust.dag direction). Refresh bootstrap + parse corpus manifest after emit_model comment bytes change. Made-with: Cursor
Codex #676 (P2/E-6): struct_def_no_debug / enum_def_no_debug on shared TypeDefinitionSyntax forced Go/Python to carry unread duplicate fields. - Revert TypeDefinitionSyntax to the five shared fields; add RustRecordDeriveTemplates in emit_model and data rust_record_derive_templates in rust.dag; emit_rust loads it by name alongside rust_language. - Strip _no_debug keys from go.dag and python.dag; regen bootstrap; refresh parse_corpus_manifest. Made-with: Cursor
Opus #676 exploratory: rust_borrowed_type_name_for_port only has one caller (rust_type_name_for_user_function_parameter), which already handles Arrow before borrow paths — drop the redundant peel+impl Fn branch and document the contract so callable carrier choice stays in one place. Made-with: Cursor
Restrict StorageRcDynFn / RejectFirstClassFn handling to ArrowBody::NoBody per first-class callable contract (P3). UserDefined arrows must not collapse into Rc<dyn Fn…>. Document a checkable dissolution trigger for ArrowRustEmitPolicy (api-review non-blocking on #676). Made-with: Cursor
Rebut PR #676 inline review: impl Fn + Clone is only for user-fn parameters; render_struct_field always uses rust_type_name_for_decl_storage (Rc). Move misplaced types-index prose to rust_type_name_for_port. Made-with: Cursor
PR #676 inline: default struct_def still derives Debug, but records that transitively carry first-class fn use rust_record_derive_templates (clone-only). Comment ties render_type_declaration to C-8 and boundary test. Made-with: Cursor
Codex #676: callable detection for record #[derive(Debug)] omission must see through zero-arity Instantiation heads, but the same walk is used for user-fn return-type composition — following template there makes plain Int pick up stdlib fn refs and wrongly forces Rc on callable params. Split DeclFirstClassArrowWalk: AppliedTypeArguments (args only) vs RecordDeriveOmitDebug (args + template). Made-with: Cursor
Codex #676: remove declaration_by_name side channel for rust_record_derive_templates; add LanguageSpec.record_derive_templates in emit_model.dag and reference it from rust/go/python_language. emit_rust parses templates from rust_language like other syntax bundles. Go/Python carry stub RustRecordDeriveTemplates data for inhabitance. Regenerate bootstrap and parse corpus manifest. Made-with: Cursor
- Regenerate bootstrap + lens/variant/infer emit snapshots so PB-1 and SG lens drift tests match runtime regen_bootstrap/emit_rust_module. - Do not use bare name for named first-class fn aliases in rust_type_name_for_decl_with_policy (storage path expands to Rc<dyn Fn…>; P3). - Add boundary test for type F = fn-> field through alias; adjust lens structural_resolution assertions now that regen struct helpers omit Debug derive. Made-with: Cursor
…676) Track named dissolution: per-target derive lists in substrate replace the shared-model Rust type and Go/Python stub rows; regen bootstrap for span drift from comment lines. Made-with: Cursor
- Document why return-type walk uses AppliedTypeArguments (Int substrate vs full template) and point to THESIS target-realization / rust.dag. - Clarify RustRecordDeriveTemplates is the declared LanguageSpec hook, not an undeclared extension; P2 stubs remain in TODO. - Regen bootstrap + parse manifest for emit_model line drift. Made-with: Cursor
Return/derive walk now recurses non-List Instantiation templates with an Int/Bool/String short-circuit; re-run regen_lens so checked-in #[derive] lines match emit_rust_module. Made-with: Cursor
Clarify at render site that return detection uses the same\ndecl_includes_first_class_arrow_data as storage/derive paths,\nnot a separate args-only pass — addresses review concern about\ntemplate-hidden fn vs C-8 Rc param alignment. Made-with: Cursor
The PartialFunction (Map<K,V> template) record embeds algebra\nArrow+NoBody operation fields, not first-class user fn data.\nRecursing that template in decl_includes_first_class_arrow_data\nfalse-positived and over-forced Rc for callable params (C-8, PR #676).\n\n- Cache partial_function template on Dag like list_template\n- Skip that template in Instantiation branch + unit test Made-with: Cursor
rust_type_name_for_decl_with_policy returned the alias name when\npeel did not reach a top-level Arrow. Named L = List<fn…> never\npeels that way but still carries first-class fn; skip the early\nreturn when decl_includes_first_class_arrow_data (C-8, PR #676).\n\nAdd emit_callable_field_types_expand_list_fn_named_alias. Made-with: Cursor
Claude review on #676: gating the named-alias path on\ndecl_includes_first_class_arrow_data can leave named records that\nhold fn (via alias) with no Conj/Disj match arm, hitting\nMissingTypeRealization. Emitted user struct/enum names are valid\nRust in param slots; return the name. Test:\nemit_callable_struct_with_fn_field_names_ok_in_param_slot. Made-with: Cursor
Named user records/sums with callable fields (Callback) must return\nthe declared Rust name for nested field types (Wrapper.cb) before\nthe List<fn>/alias decl_includes check — P2 name authority (codex #676).\n\nTest: emit_callable_nested_named_record_field_uses_type_name. Made-with: Cursor
Reconcile with main (e.g. r1 surface manager #739); regen only. Made-with: Cursor
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
5427838c· Trigger:schedule - Thinking:
262s wall
✅ The PR-scoped mixed code/spec diff fixes the prior callable carrier gaps and I found no new blocking concerns.
|
Review metadata
Verdict: APPROVE The diff looks clean against the pinned invariants: first-class Rust Verified with: |
What this fixes (generic-bound / callable surface)
Failure mode: First-class
fn(A) -> Blowering in Rust type-name emission was position-wrong for the stage0 / rustc contract from PR #650:fnparameter position, the emitter must spellimpl Fn(A) -> B + Clone. Emitting a borrowed callable (&impl Fn…) is ill-formed Rust and breaks the synthesizedClonestory for callable parameters.List/Vectype args, and similar),impl Traitis invalid — those sites must usestd::rc::Rc<dyn Fn(…) -> …>, notimpl Fn + Clone.This PR implements that split (parameter vs storage vs fail-closed where
impl Fnis illegal) and documents it insrc/v3/spec/rust.dag.Post-mortem:
docs/postmortems/pr-650-emitter-callable-clone-bound.md.Receipt owned here
emit_generic_bounds_survive—src/v3/compiler/tests/boundary/m1_3_emit_rust_test.rsPins the Rust signature line for
fn twice(f: fn(Int) -> Int) -> Int: expectsimpl Fn(i64) -> i64 + Cloneon the parameter and forbids&impl Fn.Companion receipts on the same seam (same PR / same review thread):
emit_callable_field_types_use_rc_dyn_fn_storageemit_callable_list_element_types_use_rc_dyn_fn_in_vecStill additive after #681, #692, #694
This slice remains distinct from what those PRs landed:
Behavior::Loopon other targets; does not implement Rust’s position-sensitivefn→impl Fn/Rc<dyn Fntype spelling.emit_rust_fixtures_rustc_greenis the broad rustc-green baseline over fixtures; this PR owns the narrow, behavior-named regression on the generic-bound / callable-parameter line (emit_generic_bounds_survive) plus field/Veccarrier checks. The baseline gate does not subsume that seam-specific receipt.So: not redundant — merge this if you want the explicit
emit_generic_bounds_survive(and related) pins on HEAD; otherwise we would be relying only on indirect coverage from the general rustc gate.Opened from session-dashboard for session
vivid-cat-794.