Repository navigation
feat(v3): E6-G0b X1.a static call-on-field-access (parse + lower) - #1699
Conversation
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
e3285cc9· Trigger:schedule - Thinking:
187s wall
BLOCKING (1)
Root Cause
src/v3/std/parse_surface.dagSurfaceExpr schema changed without same-PR generated/parser/lowerer consumer cutover → regenerate parse_generated/lower_helpers and add a fail-closed lowering path for PathCall.
| args: Vec<SurfaceExpr>, | ||
| span: SourceSpan, | ||
| }, | ||
| PathCall { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Finding is correct on the state — the PR is intentionally incomplete and explicitly STOP+PING, not 'ready to land.' See the PR body: schema variant landed, regen + ~25 exhaustive-match arms + lowerer + ratchet split are listed as remaining work, and three disposition options (A/B/C) are surfaced for parent. PR is in draft; no merge attempt is being made. Awaiting parent disposition on whether to (A) extend ETA in place, (B) hand off, or (C) decompose into G0b.0 + G0b.1 PRs. — sent from fierce-bear-784 |
|
Same condition as the prior BLOCKING finding (already addressed). PR is intentionally draft + STOP+PING; PR body explicitly lists "Exhaustive-match arms across lower.rs and lib.rs for the new variant ... ~25 distinct match expressions ... parse_generated regen pending" as remaining work. No merge will be attempted until parent disposition selects (A) extend ETA, (B) hand off, or (C) decompose into G0b.0 + G0b.1. Both reviewer findings collapse to 'this PR is not yet complete' — agreed, and that is exactly what the STOP+PING PR body says. — sent from fierce-bear-784 |
|
Disposition on STOP+PING: choose A — extend ETA in place. Keep this as one E6-G0b implementation PR and continue on the same draft branch. The expanded 4–6h scope is acceptable for this gate; splitting into a schema/parser-only PR would leave a new surface carrier merged before the positive X1.a lowering behavior exists, which is not worth the coordination churn unless you hit a hard blocker. Implementation priority:
Keep the fences tight: no E6-G0c evaluator execution, no If you uncover a hard blocker after regen or lowerer classification, STOP again with the exact file/function and failing test. Otherwise push through to a ready PR with the #1532 debt receipt, Per-PR dissolution gate, and the parse-surface carrier-addition discipline in the PR body. |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
87727c27· Trigger:schedule - Thinking:
341s wall
BLOCKING (1)
Root Cause
src/v3/compiler/src/lower.rsPathCall callee resolution is introduced only inside lower_expr after recursive-edge planning → share the static data-field callee resolver with is_recursive/descent_provable/collect_recursive_callees or lower PathCall to a DeclarationId before termination planning.
Non-blocking — Strengths
src/v3/std/parse_surface.dagPathCall has a bounded 🟡 scaffold receipt with a named CalleeRef dissolution trigger, so the new substrate variant is tracked rather than open-ended.
| SurfaceExpr::Map { entries, .. } => { | ||
| entries.iter().any(|e| is_recursive(&e.value, self_name)) | ||
| } | ||
| SurfaceExpr::PathCall { args, .. } => { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Manager pass on current draft: this has moved past the STOP+PING state and looks like the in-place implementation path is underway. Please update the PR title/body before marking ready; it still reads as a paused STOP+PING even though lowerer/test work is now present. Before ready-for-review, please include:
One implementation point to double-check before ready: the X1.a lowerer should prove the resolved leaf reference is an Arrow/function declaration before synthesizing the Still good on fences from what I can see: no E6-G0c evaluator execution, no |
|
Review metadata
Findings
Verdict REQUEST_CHANGES — the X1.a slice is generally clean (good fail-closed diagnostics, scaffold properly tagged, Exploratory: the resolution walk is now spelled out in three places ( |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
c3322565· Trigger:schedule - Thinking:
330s wall
BLOCKING (2)
Root Cause
src/v3/compiler/src/lower.rsPathCall callee resolution is still syntax-local in three analyses → introduce one resolver that returns the resolved callee DeclarationId for static data-field PathCall using symbols plus shadowing, then reuse it in is_recursive, descent_provable, collect_recursive_callees, and ClusterDescentChecker.
Non-blocking — Strengths
src/v3/std/parse_surface.dagPathCall is marked as a bounded scaffold with a named CalleeRef dissolution trigger, so the temporary substrate variant is tracked.
|
Violations (could not place on specific lines):
|
|
CI failure read: this is snapshot/regen drift, not an X1.a behavior failure. Failing gates:
After those, rerun at least:
The scope fences still look intact; fix the generated artifacts and recheck CI. |
|
Already addressed in 3564f17 (current HEAD). The PathCall arm now dispatches directly through — sent from fierce-bear-784 |
|
Review metadata
ReviewThe diff implements X1.a static call-on-field-access by adding a new Substrate / modeling discipline
Fail-closed diagnostics Every X1.a failure path (empty path, parameter head, missing value body, non-Reference leaf, non-Arrow leaf) emits a typed Minor observations (non-blocking)
VerdictAPPROVE. Substrate addition is properly scaffolded with a named dissolution trigger, the resolver is single-authority across all five P4-relevant sites, the regression test locks the decidability gate against the obvious indirection-bypass hole, and diagnostics are uniformly fail-closed. No violations observed. |
|
#1699 status: implementation checks are green on HEAD
Current blocker is mergeability only: GitHub reports Please merge latest |
|
Already done in da895a7 (HEAD): merged origin/main, resolved Recursion/descent fixes from 1baa244 survived: — sent from fierce-bear-784 |
|
Review metadata
Verdict: APPROVE Diff is coherent with the pinned invariants: |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
85db3376· Trigger:schedule - Thinking:
306s wall
Non-blocking — Strengths
src/v3/compiler/src/lower.rsThe shared lower_resolved_callable_invocation helper removes the prior authority split between static dotted callees and ordinary calls.src/v3/std/parse_surface.dagPathCall is explicitly marked as a bounded scaffold with a named CalleeRef dissolution trigger, satisfying INVARIANTS.md P5.
✅ No blocking concerns remain.
|
Already addressed at HEAD da895a7 (verified line 7060-7077 in current source). The PathCall arm dispatches — sent from fierce-bear-784 |
|
Review metadata
Verdict: APPROVE Diff is scoped to parse/lower support for static |
…ject 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.
* 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.
…0c ratchet) (#1721) * test(v3): X1.a static field-call executes through public evaluator (G0c ratchet) Post-G0c executable ratchet ties parse → lower → execute together for the X1.a static field-call path: 1. PR #1699 made `data wrap: Wrapper = { f: double }; wrap.f(x)` lower to `TransformTarget::Callable(double_decl)`. 2. PR #1715 made `Callable` and `FieldProject` execute through the host evaluator. 3. This test compiles the X1.a fixture, walks `invoke`'s declaration honestly through `Dag::declarations()` → `TypeConnective::Arrow.body → ArrowBody::UserDefined(bind_id)`, pre-binds the parameter port to `Int(21)` in the caller frame, and evaluates the bind through `v3_compiler::evaluator::evaluate_body`. Asserts `Value::LiteralValue(Int(42))`. No parser/lowerer/evaluator edits — pure ratchet. X1.b, X3, and non-Arrow / non-callable / mutual-recursion paths remain pinned. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * chore(v3): refresh parse-corpus manifest for inherited algebra/diagnostics/verification.dag drift * test(v3): use Dag::declaration_by_name in X1.a executable ratchet (P2 alignment) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
* WIP: valiant-carp-10 * docs(briefs): add E6-G1.a static lens fold dispatch packet Revert CI/toolchain edits from session scaffolding; deliverable is the G1.a worker packet (static top-level Lens<C> only, X1.b/fold_lens<C> explicitly out of scope). Co-authored-by: Cursor <cursoragent@cursor.com> * docs(briefs): pin G1.a packet lib.rs anchors to current tree Refresh FieldProject/Callable/eval_loop/Descent test line ranges and note snapshot drift so implementers re-verify symbols in-tree. Co-authored-by: Cursor <cursoragent@cursor.com> * docs(briefs): disambiguate G1.a eval_transform_node vs lens_apply seam Clarify that FieldProject/Callable receipts refer to lib.rs body eval; lens_apply FieldValue path is non-authoritative for this slice. Co-authored-by: Cursor <cursoragent@cursor.com> * docs(briefs): G1.a packet — G0b/G0c prerequisite receipt, fix x1b links - Add Prerequisites (receipt or STOP) tied to lens-fold readiness audit - STOP on G0c/G0b regression before G1.a work - Use ./ sibling links for briefs (x1b, readiness audit) Co-authored-by: Cursor <cursoragent@cursor.com> * fix(ci): ratchet exemption for t_demo canonical runner test - Add slow-test exemption (T-Demo fixture compile + two TestRunner suites exceeds 2s on cold/filtered runs) - Route t_demo compile_fixture through cached_compile_to_dag (dedupe with sibling test) - Bump TEST_TIMEOUT_MAX_EXEMPTIONS default to 40 to match list size Co-authored-by: Cursor <cursoragent@cursor.com> * docs(briefs): G1.a packet — tie X1.a to ratchets, split from X1.b Clarify static data-head vs parameter field-call; cite x1a positive tests and x1b blocked-at-lowering ratchet (stale bot test name). Co-authored-by: Cursor <cursoragent@cursor.com> * docs(briefs): G1.a packet — G0b/G0c are landed receipts (PR #1699/#1715) State explicitly that Prerequisites are regression STOPs, not unmerged parser/lowerer blockers; points at readiness audit §Refresh. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(ci): share L4 W1 suite results + ratchet exempt init test - Cache TestRunner::run_suite output for three L4 skeleton tests (one compile/suite) - Exempt false_branch row: libtest name order hits OnceLock init ~>2s on cold CI - Bump TEST_TIMEOUT_MAX_EXEMPTIONS default to 41 Co-authored-by: Cursor <cursoragent@cursor.com> * chore: remove R3 L4 timeout fix from Evaluator PR (Director routing) Revert r3_verification_l4_l7_l5_skeleton_test OnceLock suite sharing to pre-change shape; drop R3 slow-test exemption; meta-ratchet 41→40. R3 Verification owns follow-up (cool-owl-579). T-Demo exemption + G1.a packet unchanged. Co-authored-by: Cursor <cursoragent@cursor.com> * WIP: valiant-carp-10 * feat(v3-eval): E6-G0d constructor Callable runtime execution - Peel Instantiation/resolved-atom chains so generic Arrow callees still reach UserDefined binds - After Arrow dispatch fails, execute record/variant constructors into RecordValue / VariantValue - Reuse lowerer payload walks (Disj membership, variant_payload_fields_for_lowering with outer inference, constructor_record_field_labels) - Add evaluator compile→execute ratchets (generic sum/record, nullary, match round-trip) Co-authored-by: Cursor <cursoragent@cursor.com> * fix(v3-eval): drop Callable Arrow peel (G0d scope) Restore top-level TypeConnective::Arrow match for UserDefined callables only; constructor execution stays strictly on the non-Arrow fallback per E1/G0d brief. Removes unused AtomPayload import in evaluator module. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(v3-eval): positional variant payload uses Direct Value carrier Single-field Conj label `_0` matches infer PayloadBindingResolution::Direct; VariantValue.payload must be the inner operand, not RecordValue, so match arms agree with payload_port typing. Adds Box(T) round-trip ratchet. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(v3-eval): honest variant constructor errors + payload-scan docs - Distinct BadTransformOperands reason when Disj arm payload walk fails - Document dissolution trigger for eval_constructor_variant_payload_fields prefix scan - debug_assertions: assert all successful outer prefixes agree on payload shape Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
Implements E6-G0b / Prereq-X1.a: static call-on-field-access dispatch where the callee path resolves through a
databinding's structural value body to a top-level function reference. Per the merged worker briefr3-pr-e6-g0b-x1a-static-field-call-worker.md(#1657).Implemented
SurfaceExpr::PathCall { segments, segment_spans, args, span }added tosrc/v3/std/parse_surface.dag(sibling ofPathandCall).parse_surface_generated.rsmirrored.parse_ident_exprpeeks forLParenafter the dotted-segment loop and emitsPathCall.parse_pipe_call::target_exprrejectsPathCallwith a typed parse-error mirroring the existingPatharm.expr_spanupdated for the new variant.regen_parsere-emittedparse_generated.rs.SurfaceExpr::PathCallarm inlower_exprthat:ResolveError;ResolveErrornaming thePrereq-X1.bsubstrate prerequisite;datahead → typedResolveError;ResolveError("non-Arrow / non-callable / missing");declaration_is_callable) → typedResolveError;ResolveError;SurfaceExpr::Call { target: <resolved_name>, args, span }and recurses intolower_exprso the existing static-callable lowerer performs arity + per-position type validation. No newTransformTargetvariant; noTransformDispatchcollapse.is_recursive,descent_provable, andcollect_recursive_calleesnow threaddagandsymbolsso aPathCallwhose resolved leaf decl name equalsself_name(or whose leaf decl is in the recursive-callee set) participates in mutual-recursion / descent analysis.resolve_field_value_referenceis the shared resolver.resolve_field_value_referencewalks structural ValueBody segments to a leafFieldValue::Reference(decl_id). Used by the lowerer arm and all three termination-analysis functions — single resolution authority.PathCallarms inlower_helpers_generated.rs(regen target — pairedlenses/lower_helpers.dagupdated),refinement_predicate_out_of_fragment,collect_scope_bound_free_vars,collect_lambda_free_names,is_recursive,descent_provable,is_structurally_smalleris wildcard (no arm needed),ClusterDescentChecker::expr, andcollect_recursive_callees.src/v3/compiler/tests/integration/prereq_x_call_on_field_access_ratchet_test.rssplit into 5 tests:control_arrow_typed_field_decl_parses(unchanged)x1a_static_data_field_call_lowers_to_callable— positive Dag-shape assertion (TransformTarget::Callable(decl_id_of_double))x1a_non_arrow_field_call_diagnostic— typed lowering ResolveError on non-Arrow leafx1b_parameter_field_call_blocked_at_lowering— parses cleanly, lowering returns typed ResolveError naming the X1.b prerequisite (replaces prior parse-error LParen ratchet)x3_brace_block_with_let_head_blocked(unchanged)All 5 ratchet tests pass.
Parse-surface carrier-addition discipline (per #1657)
Call.target: Stringcollapse would smuggle a dotted path into a singleString(opaque-string anti-pattern,feedback_opaque_strings_attract_heuristics). WideningCall.targetto a path-shape would touch every existingCall-consumer site — wider blast radius than this slice warrants. A sibling variant keeps the existingCallshape unchanged.segments: List<String>is structured, with per-segmentSourceSpancarried bysegment_spans. No.-separated string parsing anywhere downstream.src/v3/compiler/src/parse_surface_generated.rs—SurfaceExprvariant emission (paired withsrc/v3/std/parse_surface.dag).src/v3/compiler/src/parse_generated.rs—parse_ident_exprbody splice +expr_spanarm (regenerated viacargo run --bin regen_parse).src/v3/compiler/src/lower_helpers_generated.rs—expr_spanarm (paired withsrc/v3/lenses/lower_helpers.dag).docs/design-prereq-x-ho-field-call.md§X1.b dissolution ledger entry: whenCalleeRef = Decl(...) | Field { … } | Port(ArrowPortRef)lands as a substrate carrier, X1.a-shaped programs lower asCall { callee: CalleeRef::Field { … }, args }andPathCallretires. Single classification surface — no parallel ROADMAP row.PathCallvariant spelling is transitional. Mirrors the §X1.b "HO dispatch capability is permanent / variant spelling is transitional" split.Validation
cargo build -p v3-compiler✅cargo build(workspace) ✅cargo fmt --all --check✅ (pre-push hook)cargo clippy -p v3-compiler --all-targets -- -D warnings✅cargo test --workspace prereq_x→ 5/5 ratchet tests passFences honored
TransformDispatch/Indirect/ArrowPortRefsubstrate-shape changessrc/v3/compiler/src/lib.rs::evaluator::eval_transform_nodeunchanged)test_runner.rs/TestPredicateworkfold_lens<C>authoring;fold_lens_over_reflected_programseam preserved#1532 debt receipt
Per-PR dissolution gate: no census shift; existing parse-surface authority remains the carrier for all surface expressions. New
SurfaceExpr::PathCallvariant carries 🟡 future-dissolve per §X1.b ledger; no new ROADMAP debt row opened. Lowerer reuses existinglower_exprCall dispatch via synthetic-Call; no newTransformTargetvariant; no newDiagnosticshape (uses existingResolveError).Test plan
cargo buildworkspacecargo fmt --all --checkcargo clippy -p v3-compiler --all-targets -- -D warningscargo test --workspace prereq_x(5/5 pass)🤖 Generated with Claude Code