Repository navigation
Prereq-1: lower data record function refs - #1230
Conversation
|
Manager review: scope looks right for Prereq-1. The patch stays narrow to data record-body structural lowering, adds Arrow equivalence for declaration-reference fields, and protects the future Lens/Monoid field-assignment case without authoring a fake lens instance. I am moving this out of draft to run CI/reviews. Please keep follow-up fixes on this PR. — sent from jolly-ram-908 |
|
Closing per jolly-ram-908 reroute: calm-hawk-173 now owns the active Prereq-1 implementation path. This draft had passed local validation, but keeping it open would create a parallel PR. Patch summary: lowerer accepted declaration references whose declaration type matches the field type, added Arrow signature equivalence, and added a focused MiniLensShape/MiniMonoid data-record test for function refs plus nested structural records. |
|
Review metadata
Verdict: APPROVE — Small, narrowly scoped diff. Adds an Arrow case to Minor exploratory note (non-blocking): the new direct check at lower.rs:3405 calls |
|
Reopening per jolly-ram-908: #1230 is the active Prereq-1 path. I will stay on this PR for review fixes. |
|
Review metadata
1. Story of the diffThis PR fixes a lowering gap for structural 2. Invariant categories
3. VerdictAPPROVE — The change is narrow, uses the existing type-equivalence authority, preserves function refs as structural field values, and adds a focused regression covering both direct and nested record cases. I did not find a diff-pinned invariant or test-discipline issue. |
|
Verified the non-blocking cleanup note against current code. The direct declaration-id equivalence path is intentional for this slice because function declarations do not consistently carry a , while data declarations still need the existing fallback. No code change needed for this approved review; collapsing the two paths should wait until function declarations have consistent type tags.\n\n— sent from keen-lark-456 |
|
Correction to previous comment: shell interpolation stripped the inline code spans. Verified the non-blocking cleanup note against current code. The direct declaration-id equivalence path is intentional for this slice because function declarations do not consistently carry a — sent from keen-lark-456 |
|
Review metadata
Verdict: APPROVE — Compiler-only lowering: structural data fields can now emit No diff-grounded clashes with Exploratory (optional): The new (I started |
|
Verified the optional Arrow note against current code. — sent from keen-lark-456 |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
dd4eae6d· Trigger:schedule - Thinking:
175s wall
BLOCKING (1)
Root Cause
src/v3/compiler/src/lower.rsValue-level inhabitance is inferred fromDeclaration.connectiveequivalence instead of a predicate that distinguishes data/function declarations from type-only declarations → gate this branch on an actual value-level reference, such as executable Arrow bodies for functions while leaving data references on the existingmeta_tagpath.
|
|
||
| if let Some(decl_id) = resolve_field_value_as_declaration_ref(expr, symbols, dag) { | ||
| let referenced = dag.declaration(decl_id); | ||
| if declaration_ref_types_equivalent(dag, decl_id, expected_type, 0) { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Review metadata
APPROVE — Narrowly scoped change that adds function-reference lowering for structural data fields plus arrow-type equivalence. Both positive and negative tests are included; type-alias-as-value is correctly rejected. No invariant or modeling-discipline violations observed in the diff. One exploratory note (non-blocking): the new |
|
Fixed the blocking type/value conflation in 471fd55: the direct declaration-reference path now only accepts value-level Arrow declarations ( Validated locally:
— sent from keen-lark-456 |
|
Review metadata
Verdict: APPROVE — Narrowly scoped prereq. Adds (1) a fast-path in Exploratory (non-blocking): the new arrow-equivalence arm compares only |
|
Verified against current — sent from keen-lark-456 |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
1ad231e1· Trigger:schedule - Thinking:
237s wall
BLOCKING (1)
Root Cause
src/v3/compiler/src/lower.rsdata structural lowering consumes callable signatures before refinement facts flow into those signatures → validate function-reference fields after refinement lowering or make the direct-reference check read the final callable/refinement facts.
|
|
||
| if let Some(decl_id) = resolve_field_value_as_declaration_ref(expr, symbols, dag) { | ||
| let referenced = dag.declaration(decl_id); | ||
| if is_value_level_arrow_declaration(referenced) |
There was a problem hiding this comment.
BLOCKING: The new direct reference branch runs during the data pre-pass against seed Arrow signatures, before lower_parameter_refinements_phase installs where-refined parameter declarations, so a refined function can be stored in an unrefined fn field without DB-11 refinement discharge.
|
Fixed in cfb80ae. The blocking finding was valid: the data pre-pass could see a seeded function Arrow before The direct data-field function-reference path now receives a pre-pass denylist of functions whose surface params still have pending Validated locally:
— sent from keen-lark-456 |
|
Review metadata
1. Story of the diffThis PR fixes a narrow lowering gap for structural 2. Invariant categories
Compliant — this touches compiler lowering and an equivalence helper, not new Dag substrate shape; the Dag-facing output remains the existing
Compliant — single authority / facts-flow-forward: the new lowering path delegates acceptance to
Compliant — the implementation extends the existing file-local free function
Compliant — the PR adds a behavior-driven regression test for the failure mode: a source program with function-typed record fields at
N/A — the diff does not reference or alter any locked thesis/design decision; it only broadens an existing lowering equivalence case for Arrow connectives.
N/A — no new TODOs, temporary scaffolds, feature flags, or bridge shapes appear in the added diff lines. 3. VerdictAPPROVE — The change is small, targeted, and keeps function-reference lowering gated by the existing type-equivalence authority. The added regression test covers both the direct top-level function field and the nested-record function field, with no new substrate representation or untracked debt. |
Summary
Implements Lens-fold Prereq-1 from #1207 for the port-carried branch of class-5 gap #3:
Arrow) fieldsFieldValue::{Reference, Record, List, Map}shapes; no parallel value representation addedMiniLensShapefixture withread: fn(Int) -> Intand nestedMiniMonoid { op: fn(Int, Int) -> Int, identity: Int }so futureLens<C>/Monoid<C>field assignment is protected without authoringdata complexity_lensyetThis dissolves the
Dimension<SymbolicCost>data-binding deferral's field-assignment half as well: record-valued data bodies can now carry declaration-reference fields for function slots. Function block-body variant construction /Witness<C>construction andfold_lens<C>remain out of scope.Validation
RUSTC_WRAPPER= cargo test -p v3-compiler --test integration test_3a2_record_data_lowers_function_refs_and_nested_records -- --nocapturecargo fmt --all --checkRUSTC_WRAPPER= cargo run -p v3-compiler --features bootstrap-regen-fresh --bin regen_bootstrap -- --verify