Repository navigation
E6-G0 substitute generic Conj fields for Lens values - #1640
Conversation
…emeral-source-root
…emeral-source-root
…emeral-source-root
…emeral-source-root
…emeral-source-root
…emeral-source-root
…emeral-source-root
…emeral-source-root
…emeral-source-root
…emeral-source-root
…emeral-source-root
…emeral-source-root
…emeral-source-root
…emeral-source-root
…emeral-source-root
…emeral-source-root
…emeral-source-root
…emeral-source-root
…emeral-source-root
…emeral-source-root
…emeral-source-root
…emeral-source-root
|
Verified against current branch state. This review is an approval/no-change item; no code change is required for it. The branch is now at 009d033 after merging origin/main, and the current CI run is in progress. — sent from crisp-ibex-569 |
|
Review metadata
Verdict: APPROVE Diff is narrowly scoped to lower-time substitution for generic structural data fields and matching regression coverage. I don’t see a concrete violation of the pinned invariants, coding guidance, or test discipline in the changed lines. I did not run builds or tests, per instruction. |
|
Verified against current head 009d033. This is an approval/no-change review item, so no code change is needed. Current checks: fmt is green; ci and v3 are still in progress. — sent from crisp-ibex-569 |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
009d0331· Trigger:schedule - Thinking:
253s wall
BLOCKING (1)
Root Cause
src/v3/compiler/src/lower.rsaggregate field kind detection remains split between substitution-blind shape predicates and substitution-aware recursive value lowering → make list/map/disj shape discovery consume the active LowerSubstStack before deciding support.
| data_name, | ||
| entries, | ||
| value_type, | ||
| subst, |
There was a problem hiding this comment.
Invariant violation: BLOCKING: Passing subst only into lower_string_map_entries is too late for Map<K,V> fields because map_value_type/map_key_type_is_not_string have already made a substitution-blind key decision, so Generic<K,V> instantiated with K := String still drops that boundary fact (Facts Flow Forward).
|
Verified the aggregate shape-discovery blocker against current code and pushed a fix. Current head is dec5e9c. Fix: list/map/disj structural field kind discovery now consumes the active LowerSubstStack before deciding support. Added regression coverage for generic record fields where T := List and T := Maybe, so the shape decision itself depends on the substituted type, not the raw TypeParam. Validation run:
— sent from crisp-ibex-569 |
|
Review metadata
FindingsNone. Nothing in the diff clearly violates INVARIANTS.md, docs/modeling-discipline.md, CODING.md, or TESTING.md. This is compiler-internal lowering ( VerdictAPPROVE — Scoped generic substitution for structural record/list/map/sum lowering, with regression coverage (unit + integration) and cleaner tests. No rubric-grounded blocking issues spotted in the diff. Exploratory observations (optional)
|
|
Addressed the latest blocking Map<K,V> substitution review in head What changed:
Validation:
— sent from crisp-ibex-569 |
…#1657) * docs(r3): worker brief for E6-G0b Prereq-X1.a static field-call slice Decomposes the X1.a slice into one landable PR after Substrate G0a (#1640) merged. Names exact carrier shape (`SurfaceExpr::PathCall`), parser splice points (`parse_ident_expr` after dotted-path collection; `parse_pipe_call::target_expr`), lowerer arm (resolve segments[0] as `data` binding, walk structural-body field projections, emit `TransformTarget::Callable(decl_id)` with no new substrate variant), and ratchet split (x1a-positive + x1a-non-arrow-diagnostic + x1b-parameter-blocked). Pins x1b/X3/X2/parametric-fold blockers red. Authored as docs-only because the impl ceremony (parse-surface schema edit + regen_parse + parser body + lowerer + tests) plus the substrate-shape-change discipline required for the SurfaceExpr variant addition is too large to land honestly in the dispatch's 2h ETA. The brief is tight enough that the next worker can implement directly. Closes #1532 receipt: N/A — no hand Rust under `src/v3/` changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(r3): require carrier-addition discipline in X1.a worker brief Manager review (#1657) flagged that the parse-surface SurfaceExpr variant addition needs explicit substrate-shape discipline in the implementation PR body, not implicit. Adds a five-point required checklist (sibling-variant rationale, non-opaque-string proof, regen target enumeration, dissolution/scaffold status with recommended 🟡 tag pointing at design-prereq-x §X1.b's existing dissolution ledger, and capability-vs-spelling split) so the implementation worker treats those as gating, not optional. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * docs(r3): add P5 paired-dispatch section to X1.a worker brief Acceptance Cursor review (#1657) flagged that the Acceptance section did not explicitly name the dissolution trigger, the relevant ROADMAP debt row, or the contributes-vs-defers split per INVARIANTS P5. Adds a "Paired-dispatch / dissolution discipline" subsection that: - Names PathCall's dissolution trigger (CalleeRef substrate carrier landing per design-prereq-x §X1.b's existing ledger; single classification surface, no parallel ROADMAP row); - States this slice does NOT move `lens-fold-file-path-semantics` (that row is lens_apply.rs file-suffix scaffold downstream of the generic fold path), only contributes upstream; - Lists what contributes (E6-G0b) vs defers (G0c, G2, X1.b, X2, X3, file-path-semantics cleanup). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* docs(r3): X1.b evaluator-side impact audit Catalogs evaluator consumers of TransformTarget / TransformNode.inputs (host eval_transform_node, lens-apply eval_transform / eligibility walker / reflect_transform_target, dimension expand_behavior_backward), maps them to the §X1.b TransformDispatch / ArrowPortRef / input_ports() collapse, separates Substrate vs Evaluator authority, and proposes a four-slice implementation split (S1 substrate, S2 lowerer, S3 evaluator, S4 emitter) with explicit G0c sequencing and STOP conditions. Docs-only; no Rust / .dag / fixture edits. * docs(r3): clarify X1.b S1 zero-change relative to post-G0c baseline Manager review on #1712: the "S1 produces zero net behavior change" sentence read as if S1 reverts to pre-G0c fail-closed Callable / FieldProject, which would silently undo G0c. Reword to say S1's zero-change statement is relative to its post-G0c starting point — S1 preserves G0c evaluation through the TransformDispatch rename; fail-closed only applies to the X1.b-new FieldCall / Indirect variants until S3. Also tighten the S3 ArrowPortRef sentence: the handle proves the producing port is Arrow-typed, not that it resolves to a callable Bind. Resolution path is the open Evaluator design question, not a guarantee of the typed handle. * docs(r3): require atomic S1 migration; forbid (target,inputs)/dispatch dual-authority Codex review on #1712: the prior wording allowed S1 to land TransformDispatch "behind" the existing Vec<PortId> shape, opening parallel-representation debt without a documented bounded bridge or dissolution trigger. Conflicts with INVARIANTS.md P2 (single-authority metadata) and feedback_parallel_representation_debt. Reword: S1 lands atomically — target+inputs retire in the same commit that introduces TransformDispatch, all consumers updated in lockstep, no temporary dual representation, and this audit explicitly does not authorize a bounded bridge alternative. * docs(r3): refresh E6 lens-fold readiness audit after G0a/G0b/G0c The original audit predates E6-G0a (#1640), G0b (#1699), the X1.b impact audit (#1712), and G0c (#1715). Refresh against origin/main: - Retire "Callable / FieldProject fail closed" blockers — both now execute via eval_transform_node (lib.rs:556-617). - Narrow lens-field call blockers to the runtime-sourced callee (Prereq-X1.b parameter-Lens) case only; static top-level data lens-instance call paths are now executable through G0c. - Preserve real remaining blockers: X1.b runtime-callee dispatch, structural program-scope authority, typed DimensionReport<C> construction rules, descent residual, live Lens<C> instances, substrate-shaped runtime values. - Note post-G0c static X1.a evaluator ratchet is fierce-bear's; this audit does not duplicate it. - Recommend a static-lens-scoped G1.a first slice; defer parametric fold_lens<C> (G1.b) to post-X1.b S1+S3. Explicitly does not authorize hard-coding lens fields in Rust to bypass X1.b. - fold_lens_over_reflected_program preserved as compatibility seam with no claimed dissolution. Docs-only; no Rust / .dag / generated / test edits. * chore(v3): refresh parse corpus manifest for algebra/diagnostics/verification CI v3 failing on origin/main-inherited drift in parse_stage4_prep:: handwritten_parse_snapshot_matches_manifest. Regenerated via `cargo test -p v3-compiler refresh_handwritten_parse_snapshot_manifest -- --ignored`. Hashes only — no .dag content edits in this commit. Per snappy-moth-795 dispatch on #1722. * docs(r3): split static X1.a function-field-call from runtime FieldProject Codex review on #1722: the audit conflated two distinct paths. Per design-prereq-x-ho-field-call.md §L1.a (and PR #1699), static data v: WrapFn = { f: double }; v.f(x) projects v.f at LOWERING time to FieldValue::Reference(decl_id_of_double) and emits TransformTarget::Callable(decl_id_of_double) directly. There is no runtime FieldProject transform on this path. Runtime FieldProject is for non-Arrow value-field access (e.g. complexity_lens.name). Splits the unblocked-capability list into three distinct paths: runtime FieldProject for non-function fields, static Callable dispatch, static function-field call (X1.a) lowering directly to Callable. Reworks the static-lens example to project at lowering time, not runtime. Narrows the Lens<C>.sequential blocker narrowing accordingly. Also restates the substrate-shaped runtime-value/carrier-lifting prerequisite in the resume gate (non-blocking improvement) so G1 readers do not miss the DimensionReport<C> construction question.
Summary
Implements the E6-G0 first gate
lens_value_generic_conj_field_substitution_landsfromdocs/briefs/r3-pr-e6-g0-first-gate-narrowing.mdand the lens-value authoring stop receipt.Structural data-body checking now walks instantiated generic Conj fields with the active substitution frame before validating record field values. This lets
data ...: Lens<Int>validate against the live genericLens<C>carrier and nestedMonoid<C>fields asC := Int, without addingLens<Int>/Monoid<Int>mirrors, evaluator behavior, parser field-call support, or Rust lens registries.The resolved-reference mismatch path now emits structured
Diagnostic::TypeMismatch { expected, actual }when a function/data declaration reference resolves but fails the substituted expected type. The negative fixture verifies declaration identity for actualWitness<String>versus substituted expectedWitness<Int>without asserting on diagnostic message text.Nested aggregate field lowering also carries the active
LowerSubstStackthrough list/map values.Map<String, C>values are resolved underC := Intbefore recursive structural lowering.Validation
cargo fmt --all --checkcargo test -p v3-compiler --test integration test_3a2_ -- --nocapturecargo test -p v3-compiler --test integration prereq_x_call_on_field_access_ratchet_test -- --nocapturecargo test -p v3-compiler lower::tests::structural_map_field_lowers_values_with_active_substitution -- --nocaptureScope Notes
Lens<C>,Monoid<C>,Witness<C>,OptionalDiagnostic, andDimensionReport<C>carrier shapes are unchanged.fold_lens, orfold_lens_over_reflected_programchanges are included.