Repository navigation
fix(v3): parse bare single-variant sum RHS - #1286
Conversation
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
a26c2824· Trigger:schedule - Thinking:
332s wall
Non-blocking — Strengths
src/v3/compiler/parse_parser_body.txtThe parser-local single-variant sum disambiguation keeps TypeSum vs TypeAlias explicit and flows into the existing SurfaceItem::TypeSum lowering path.
✅ Code-only parser change; I found no blocking thesis or invariant concerns.
|
Codex schedule review ( The No commit from this item. — sent from smart-boar-25 |
|
Review metadata
FindingsNone. Nothing in the diff clearly breaches INVARIANTS.md, docs/modeling-discipline.md, CODING.md, or TESTING.md. This is implementation-layer parsing (not substrate modeling): tighter sum-vs-alias lookahead, a small helper, and parse regression tests that target the parser’s public surface ( VerdictAPPROVE — The change is narrow: Exploratory observations (optional)
|
|
Review metadata
Build passing doesn't change the verdict — it confirms the drift. |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
fd2cc50f· Trigger:schedule - Thinking:
258s wall
BLOCKING (2)
Root Cause
src/v3/compiler/parse_parser_body.txtthe parser template and generator changed but the generated snapshot was not updated → regenerate parse_generated.rs from the new authority or remove the authority change.src/v3/compiler/parse_parser_body.txtthe pass-1 alias-reference fact is introduced without a parse decision that consumes it → either make rhs_is_sum consult the set or delete the two-pass scaffold.
| out.push_str("use crate::tokenize::{Token, TokenKind};\n"); | ||
| out.push_str("use crate::operators::{LogicalOp, OperatorKind};\n\n"); | ||
| out.push_str("use crate::operators::{LogicalOp, OperatorKind};\n"); | ||
| out.push_str("use std::collections::HashSet;\n\n"); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| /// `None` during pass 1 (bare-RHS sum disambiguation disabled). `Some` during | ||
| /// pass 2: a bare `type T = U` is [`SurfaceItem::TypeSum`] iff `U` is **not** | ||
| /// in this set (prelude + imports + module `type`/`data` LHS names). | ||
| bare_rhs_alias_reference_idents: Option<HashSet<String>>, |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Review metadata
Findings
VerdictAPPROVE_WITH_COMMENTS — the fix unblocks |
|
Review metadata
FindingsNone. Nothing in the diff clearly breaks INVARIANTS.md, docs/modeling-discipline.md, CODING.md, or TESTING.md. The change is implementation (surface parse / generated parser), not substrate on the Dag. Ambiguity is handled with an explicit two-pass pass and a documented, conservative prelude identifier list plus module LHS/import names ( VerdictAPPROVE — Scoped parser fix, mirrored regen output, corpus manifest refresh, and focused integration tests; no rubric violation tied to a specific diff hunk. Exploratory observations (optional)Maintaining |
…iant-sum-parser # Conflicts: # src/v3/compiler/src/bootstrap_generated.rs # src/v3/compiler/src/bootstrap_generated_without_parse_surface.rs
|
Review metadata
Findings
Verdict: REQUEST_CHANGES. The parser fix is directionally right and the added focused tests pass, but the generic-binder collection needs to be scoped to the declaration being disambiguated rather than stored as a module-wide alias authority. |
briansrls
left a comment
There was a problem hiding this comment.
Review metadata
- Provider / model:
codex/unknown - Commit:
bdafd366· Trigger:schedule - Thinking:
381s wall
BLOCKING (1)
Root Cause
src/v3/compiler/parse_parser_body.txtthe bare-RHS disambiguator collapses declaration-local generic binders into module-level parser state → make the type-parameter exception local to the declaration being parsed or carry an explicit scoped ambiguity fact into pass 2
| } => { | ||
| out.insert(name.clone()); | ||
| for param in type_params { | ||
| out.insert(param.clone()); |
There was a problem hiding this comment.
BLOCKING: Type parameters from one declaration are inserted into a module-global alias set, so a later bare single-variant sum whose constructor matches any earlier generic parameter is misparsed as TypeAlias; this loses the scoped binder fact and violates Facts Flow Forward.
|
Verified against current head |
|
Verified against current head |
|
Review metadata
Findings
Verdict: REQUEST_CHANGES The implementation covers record payload and plain bare nullary sums, but the first pass can still fail a valid bare nullary sum when |
…/tests - Add go_native_int TypeRealization (carrier "int") and TargetIntegerTypeInhabitance row go_integer_inhabit_int_platform_dependent with unit PlatformDependentFact per design-emission-model Q1 (Track B; no UInt128/u128 widening). - Crate docs: clarify ScratchIntExamples ceiling vs match_bound Track B predicate, #1133/#1286 program-bound gate, Pattern D STOP+PING on opportunistic width rows. - fold.rs: Track B module notes; match_bound unit tests; allow(dead_code) on BoundDeclarationView::PlatformDependent until LanguageSpec supplies program facts. - Regenerate bootstrap snapshots + SG-2 parse manifest for go.dag churn. Co-authored-by: Cursor <cursoragent@cursor.com>
…on-fold receipts (#1913) * docs(s7): Phase 1 BoundDeclarationView/match_bound inventory (fold.rs) Co-authored-by: Cursor <cursoragent@cursor.com> * docs(s7): clarify inventory authority, Track A/B gates, Pattern D Co-authored-by: Cursor <cursoragent@cursor.com> * docs(s7): auditable merge-base receipt for S7 worker brief (blob SHA) Co-authored-by: Cursor <cursoragent@cursor.com> * docs(s7): Q1/P1 gate — PlatformDependent ≠ wider StaticBoundFact Co-authored-by: Cursor <cursoragent@cursor.com> * docs(s7): inventory — PlatformDependent arm is DiffersKind placeholder until Track B Co-authored-by: Cursor <cursoragent@cursor.com> * feat(std): ReferenceModel<T> phantom carrier (S7 PR-F Phase 2) Co-authored-by: Cursor <cursoragent@cursor.com> * test(v3): refresh SG-2 parse corpus manifest for types.dag +ReferenceModel Co-authored-by: Cursor <cursoragent@cursor.com> * WIP: loyal-wolf-828 * WIP: loyal-wolf-828 * fix(v3): refresh SG-2 parse manifest after authority drift Update fnv/debug_bytes for dsl/std/types.dag (ReferenceModel commentary) and src/v3/std/emit_model.dag so handwritten_parse_snapshot_matches_manifest matches the committed corpus. Co-authored-by: Cursor <cursoragent@cursor.com> * WIP: loyal-wolf-828 * fix(v3): extend TargetIntegerInhabitanceBound coproduct expectations for CI substrate_coproducts_match_runtime_carriers compares lowered sum variants to emit_model; PlatformDependentFact is unit like BoundUnspecified. Co-authored-by: Cursor <cursoragent@cursor.com> * feat(s7): Go native int PlatformDependentFact row + Phase 1 fold docs/tests - Add go_native_int TypeRealization (carrier "int") and TargetIntegerTypeInhabitance row go_integer_inhabit_int_platform_dependent with unit PlatformDependentFact per design-emission-model Q1 (Track B; no UInt128/u128 widening). - Crate docs: clarify ScratchIntExamples ceiling vs match_bound Track B predicate, #1133/#1286 program-bound gate, Pattern D STOP+PING on opportunistic width rows. - fold.rs: Track B module notes; match_bound unit tests; allow(dead_code) on BoundDeclarationView::PlatformDependent until LanguageSpec supplies program facts. - Regenerate bootstrap snapshots + SG-2 parse manifest for go.dag churn. Co-authored-by: Cursor <cursoragent@cursor.com> * test(v3): assert Go PlatformDependentFact inhabitance row in bootstrap Structural Slice B receipt: Track B native Go `int` row must stay populated. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(v3): satisfy clippy manual_contains in inhabitance test Use Vec::contains for the Go native int row check so cargo clippy -p v3-compiler --all-targets -- -D warnings passes in CI. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(v3/go): drop duplicate go_native_int TypeRealization for Int go_int is the single Go TypeRealization for substrate Int; the platform-dependent inhabitance row must reference it so the Go emitter's duplicate-realization map stays single-authority. Removes go_native_int and points go_integer_inhabit_int_platform_dependent at go_int; regen bootstrap + parse manifest. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
…bitance (#1980) (#2279) * feat(v3): retire coercion-fold ScratchIntExamples; SelectedTargetInhabitance Remove LanguageSpecProjection::ScratchIntExamples and IntScratchExample. fold_program_to_target now only handles Undeclared and DeclaredIntegerIntents; selected rows return SelectedTargetInhabitance { type_realization: DeclarationId } instead of the lane-local primitive enum. Rewrite crate tests to exercise design-doc scenarios through declared integer intents. Update emit_model.dag comments and grounding Coercion-Fold stratum_b assertions. Run full-bootstrap grounding tests on a larger stack thread to avoid overflow on default test stacks (emission_diagnostic_lockstep and stratum_b). Co-authored-by: Brian Searls <briansrls@users.noreply.github.com> * fix(v3/coercion-fold): gate type_realization on TypeRealization meta-tag Follow-up on PR review: after selecting an inhabitance row, require declaration_opt + meta_tag == dag.type_realization_meta() before returning SelectedTargetInhabitance so malformed substrate rows fail closed under UnderRefined("target_integer_type_realization") (or TypeRealization_meta). Add regression tests for rust_u32 pass, UInt32 rejection, bogus id rejection. Co-authored-by: Brian Searls <briansrls@users.noreply.github.com> * docs(v3): note TypeRealization meta gate for inhabitance type_realization TargetIntegerTypeInhabitance.type_realization stays DeclarationRef in emit_model; document that coercion-fold validates TypeRealization meta-tag at selection time (already enforced in validate_type_realization_reference). Co-authored-by: Brian Searls <briansrls@users.noreply.github.com> * chore(v3): regen bootstrap snapshots after emit_model.dag edits Comments on TargetIntegerTypeInhabitance shifted parse spans; committed snapshots drifted bootstrap verify (CI ci job). Co-authored-by: Brian Searls <briansrls@users.noreply.github.com> * refactor(v3/tests): opt-in DeclarationId witness helper for coercion-fold Add compiler feature declaration-id-raw-test-witness with DeclarationId::declaration_id_raw_for_testing; enable from v3-grounding-coercion-fold dev-dependencies and replace unsafe transmute in row-count gate tests. Non-production opt-in avoids widening the default ctor surface. Co-authored-by: Brian Searls <briansrls@users.noreply.github.com> * chore(v3): refresh parse_corpus_manifest for emit_model.dag edits SG-2 handwritten_parse_snapshot_matches_manifest ratchet: emit_model comments shifted snapshot hash (fnv1a64). Co-authored-by: Brian Searls <briansrls@users.noreply.github.com> * refactor(v3/coercion-fold): seal SelectedTargetInhabitance + doc yellow scope - Private type_realization field with accessor; pub(crate) from_validated_type_realization only after the TypeRealization gate (addresses APPROVE_WITH_COMMENTS carrier proof concern). - LanguageSpecProjection / residual YELLOW: name #1980 scratch retirement, Undeclared, and IntegerTargetIntent + BoundDeclaration dissolution (#1133 / #1286). - Tests compare SelectedTargetInhabitance via type_realization() only. Co-authored-by: Brian Searls <briansrls@users.noreply.github.com> * test(v3/coercion-fold): note deferred Examples 1/5 vs declared projection APPROVE exploratory: no Rust consumers of ScratchIntExamples; Examples 1 and 5 axes need #1133 / #1286 facts IntegerTargetIntent cannot omit today. Co-authored-by: Brian Searls <briansrls@users.noreply.github.com> * fix(v3/coercion-fold): align TypeRealization payload with inhabitance row validate_type_realization_matches_inhabitance_row now parses substrate TypeRealization.language/target and requires they match row.language and row.kernel_integer (plus meta tag), blocking cross-language or wrong-kernel references on malformed rows (E-6 / FAIL-CLOSED). Expand gate tests: language mismatch, kernel vs realization target mismatch. Co-authored-by: Brian Searls <briansrls@users.noreply.github.com> * refactor(v3/coercion-fold): unify row gate + SelectedTarget mint Respond to exploratory review (opus): selected_target_after_row_type_realization_gate returns SelectedTargetInhabitance directly so validation and construction cannot split; from_validated_type_realization stays a single callee. Co-authored-by: Brian Searls <briansrls@users.noreply.github.com> * refactor(v3): dedupe UnderRefined axis for type-realization gate Co-authored-by: Brian Searls <briansrls@users.noreply.github.com> --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Brian Searls <briansrls@users.noreply.github.com>
|
Review metadata
1. Story of the diffThis PR teaches the v3 parser to recognize single-variant sum RHS forms that do not contain a top-level 2. Invariant categories
N/A — this diff is parser implementation / generated-parser maintenance only; it does not add substrate types, Dag fields, Dag mutation APIs, or new substrate variants.
Finding — BLOCKING, P2 Boundary Discipline / facts flow forward + P3 Fail-Closed.
Compliant, with the invariant bug above. The new helper is a free function over explicit inputs (
Finding — BLOCKING regression gap tied to the same parser path. The new alias-preservation regression covers imported names and sibling declarations (
N/A — the diff does not reference or alter a locked thesis/design decision; it changes parser disambiguation and matching tests only.
N/A — no new TODO, staged scaffold, compatibility shim, or temporary migration surface is introduced in the diff. The hard-coded prelude alias list is a parser disambiguation table rather than a marked bridge, so I would not classify it as tracked/untracked debt from this diff alone. 3. VerdictREQUEST_CHANGES. The intended single-variant sum support is directionally sound, and the tests cover the motivating cases, but the two-pass alias-reference authority currently omits local type parameters and will misparse generic aliases such as |
|
Review metadata
1. Story of the diffThis PR changes the v3 parser’s 2. Invariant categories1. LAYER MODEL — substrate vs implementationCompliant — the hand-authored change is parser implementation, and the substrate-looking updates are generated snapshot receipts of the parse fix: e.g. 2. INVARIANTS.md + modeling-discipline.mdFinding — BLOCKING, Boundary Discipline / facts flow forward. 3. CODING.mdFinding — BLOCKING, clear interfaces / precise carriers. 4. TESTING.mdFinding — BLOCKING, regression coverage for the fix. The added alias-preservation fixture covers the positive generic-alias case with 5. LOCKED DESIGN DECISIONSN/A — the diff does not reference or alter a locked thesis/design decision; the substrate snapshot changes are generated consequences of the parser fix rather than an explicit design divergence. 6. TRACKED vs UNTRACKED DEBTFinding — NON-BLOCKING, scaffold tracking. 3. VerdictREQUEST_CHANGES — the main parser approach is sensible and well-covered for the headline behavior, but the module-wide alias set currently turns local generic binders into global alias authorities. That is a parser correctness regression in the new disambiguation path, and it should be fixed with a scoped carrier plus the missing negative test before merge. |
Summary
type InputEvaluationOrder = LeftFirstwithout requiring the old parenthesized workaround.type Id<T> = Twithout leaking those binders into unrelated declarations.Verification
cargo run -p v3-compiler --bin regen_parsecargo run -p v3-compiler --features bootstrap-regen-fresh --bin regen_bootstrapcargo run -p v3-compiler --features bootstrap-regen-fresh --bin regen_bootstrap -- --verifycargo fmt --all --checkcargo clippy --workspace -- -D warningscargo test -p v3-compiler --test integration m2_substrate_inhabitance_test::parse_bare_rhs_alias_reference_stays_type_alias -- --nocapturecargo test -p v3-compiler --test integration m2_substrate_inhabitance_test -- --nocapturecargo test -p v3-compiler --test integration sg2_parse_authority_test::parse_generated_module_matches_checked_in_snapshot -- --nocapturecargo test -p v3-compiler refresh_handwritten_parse_snapshot_manifest -- --ignoredcargo test -p v3-compiler --test integration parse_stage4_prep::handwritten_parse_snapshot_matches_manifest -- --nocapture