Repository navigation
feat: add Node.ident with parser assignment and emitter defaults - #408
Conversation
ChatGPT ReviewReviewed against THESIS.md, INVARIANTS.md, MODELING.md, ROADMAP.md, std-library.dag, and the PR diff. Grounding this in the project thesis: gunbc is supposed to validate intent first and only then emit as a mechanical consequence. “If it compiles, the intent is sound” is the bar, so silent backend repair is the wrong failure mode. chatgpt-review-5bbb8377-1bc7-44… chatgpt-review-59965226-a710-4a…
In the Concretely, this can turn an incomplete record literal into a valid-looking Rust program with behavior the user never declared. It also makes semantics depend on the backend: this fallback exists only in Rust emit, so incomplete record literals can now drift across targets instead of being rejected uniformly. That cuts directly against the “single declaration → all targets” thesis and against the existing record-completeness direction, which is a missing-field diagnostic, not implicit backend defaults. chatgpt-review-5bbb8377-1bc7-44… chatgpt-review-59965226-a710-4a…
The new helper: hardcodes If implicit defaults are truly intended language semantics, they should be declared once in the structural model and consumed by every backend. If they are not intended semantics, this helper should not exist.
Directionally, adding On the source side, parse only assigns non-zero ids to module and import nodes ( That is a boundary-sufficiency problem more than a solution. If this field is meant to be authoritative, it needs preservation on structural rewrites and broader assignment than module/import only. If it is only scaffolding, it should stay non-authoritative until that propagation lands. chatgpt-review-729f3429-67c6-4d… chatgpt-review-59965226-a710-4a…
The new parser test only checks that module/import ids are non-zero and round-trip through the parser intern table. It does not test uniqueness across authored nodes, preservation through parse → resolve → infer → emit, or any downstream consumer that relies on Separately, the new Rust missing-field defaulting behavior has no behavioral test establishing that this is intended language semantics. That matters because this PR changes observable program behavior, not just an implementation detail. The testing invariants here explicitly prefer behavioral/public-contract tests over local implementation checks; this change needs that level of coverage if it’s intentional. chatgpt-review-59965226-a710-4a… chatgpt-review-73937e64-e8a4-4d… Compositional-modeling note:
I would block on finding 1. Finding 3 is also important if the intent is for |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbcfadaa36
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| Some(s) => { | ||
| let fs = s.field_summaries.clone(); | ||
| let ftm = s.field_type_map.clone(); | ||
| let missing = Rc::new({ let mut __result = Vec::new(); for k in Rc::new(v2_rt::map_keys(&fs)).iter().cloned() { if !emit_map_has(provided_set.clone(), k.clone()) { __result.push(k); } } __result }); |
There was a problem hiding this comment.
This default-filling path derives missing from whatever lookup_emit_type_summary(tn) returns, then emits those fields unconditionally when they are defaultable. type_summaries is keyed by bare type name (add_emit_item_summary inserts by summary.name), so two modules can legally collide on names like Config; in that case, emitting module A can pick module B’s summary and inject foreign fields (e.g., Int/Bool/String defaults), producing invalid Rust even when A’s literal already provides all of its real fields.
Useful? React with 👍 / 👎.
| pub fn parse(tokens: &Rc<Vec<Rc<Token>>>, source_index: Option<Rc<NewlineIndex>>) -> Rc<ParseResult> { | ||
| { | ||
| let state = Rc::new(ParserState { | ||
| let pre_interned = pre_intern_tokens(tokens.clone(), empty_intern_table()); |
There was a problem hiding this comment.
Pre-interning every token here adds a full intern pass before parsing each module. In this codebase, intern returns new persistent tables (v2_std_core::intern via rc_list_push/rc_map_insert), so each new unique token grows copied structures; doing this for all tokens (including punctuation/literals) can make parse time and memory grow super-linearly on large inputs. Before this change, only a small set of names were interned during parse, so this introduces a substantial performance regression risk.
Useful? React with 👍 / 👎.
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
cursor · composer-2 · dbcfadaa
Review (INVARIANTS: 4, MODELING: 6+/6-, ROADMAP: 0✓/0!)
INVARIANTS — Violations (4)
ROOT CAUSE ANALYSIS
src/v2/05_emit_rust.dagPer-field defaults and target zero inhabitants are not declared as data on the typed IR or LanguageSpec and are not proven at infer/reconcile; summary registry type strings flow to emit → symptom is if-chains and invented Rust initializers; upstream fix is carry explicit default Expr nodes or a single kernel/LanguageSpec zero table and fail-closed when literals omit required fields (MODELING.md import-from-authority; THESIS.md causal completeness for record completeness).src/v2/stage0Emission policy was extended in both trees; until regeneration is the sole write path with CI enforcing empty diff, any edit doubles drift risk; upstream fix is land the .dag change and regenerate stage0 so one definition drives both (INVARIANTS.md structural prevention for stage0).src/v2/05_emit_rust.dagDefaults and Rust zero inhabitants are not declared as structured facts on bindings or LanguageSpec and are not resolved before emit; summary type-name strings force downstream guessing → invented initializers; upstream fix is attach resolved default Expr or a single kernel/LanguageSpec zero table at the infer/reconcile boundary and emit only those facts fail-closed (MODELING.md import-from-authority; THESIS.md record completeness as causal fact not emit heuristic).src/v2/stage0Bootstrap policy was duplicated in .dag and .rs; drift is inherent while two sites exist; upstream fix is regenerate stage0 from .dag under CI diff ratchet so one source defines emission behavior (INVARIANTS.md stage0 structural prevention).
MODELING — Strengths
src/v2/00_core.dagNode.ident hooks module/import names into the existing InternTable and pre_intern_tokens aligns token text with later intern calls.src/v2/02_parse.dagPre-interning tokens before ParserState initialization makes module and import intern ids consistent with token stream text.src/v2/05_emit_rust.dagThe all_defaultable guard avoids emitting defaults when any missing field lacks a known zero or optional status, mitigating wrong output from ambiguous cross-module type-name summaries (comment in + lines).src/v2/00_core.dagNode.ident plus pre_intern_tokens ties parser-visible text to InternTable ids before parse consumes the table, composing with existing intern machinery.src/v2/02_parse.dagparse seeds intern_table from pre_intern_tokens so module_path and import path intern results align with token-interned strings.src/v2/05_emit_rust.dagall_defaultable gates default emission so mixed missing fields without known zeros do not silently compile, reducing false positives from registry ambiguity (per +line comment).
MODELING — Improvements
src/v2/00_core.dagParser and helpers still leave ident at 0 for most nodes, so ident cannot yet serve as a universal stable NodeId without widening assignment or a distinguished optional/sentinel model (MODELING.md M4: reduce string/int identity proxies in favor of structural edges once consumers exist).src/v2/02_parse.dagThe new core import lists intern_find_or_empty but no added code uses it—drop the symbol to satisfy minimal-import discipline and avoid speculative coupling.src/v2/05_emit_rust.dagDrive zero/optional emission from shared std/languages data keyed by structural type identity rather than Rust type-name strings so Rust stays one LanguageSpec projection (THESIS.md target-language spec = transport spec; extdeps fidelity).src/v2/00_core.dagMost synthesized nodes still use ident 0, so ident is not yet a reliable graph-local NodeId; extend assignment at construction or model absence structurally so downstream does not treat 0 as collision-prone proxy (MODELING.md M4 direction).src/v2/02_parse.dagDrop unused intern_find_or_empty from the expanded import list (or use it) to keep dependencies minimal and avoid speculative coupling.src/v2/05_emit_rust.dagReplace rust_zero_value string switches with data in std/languages.dag (or kernel inhabitant tables) parameterized by LanguageSpec so Rust emission stays one projection, not ad hoc type-name matches (THESIS.md coercion=emission unification; INVARIANTS open-set prevention).
PR advances stable intern ids on module/import nodes and pragmatic Rust struct literal emission, but leaves two thesis-relevant gaps: emit still guesses defaults from strings instead of carried IR facts, and stage0 duplicates that policy until regen-only workflow holds.
| @@ -3550,7 +3550,43 @@ fn emit_typed_record_lit(type_name: String?, fields: List<Node>, parent_enum: St | |||
| } else { val_str } | |||
There was a problem hiding this comment.
Invariant violation: emit_typed_record_lit now completes missing struct fields using EmitTypeSummary plus rust_zero_value string matches on type names, conflicting with INVARIANTS.md “No case enumeration for open sets,” “Heuristics indicate lost structure,” and “No fallbacks that fabricate” (silent completion of literals without IR witnesses).
| @@ -3626,7 +3627,43 @@ let field_val = if needs_wrap.clone() { | |||
| }; | |||
There was a problem hiding this comment.
Invariant violation: The same missing-field synthesis and rust_zero_value policy are hand-maintained in stage0 alongside 05_emit_rust.dag, violating INVARIANTS.md “No duplicate representations” / “No parallel implementations” for bootstrap-generated Rust versus the .dag source of truth.
| @@ -3550,7 +3550,43 @@ fn emit_typed_record_lit(type_name: String?, fields: List<Node>, parent_enum: St | |||
| } else { val_str } | |||
There was a problem hiding this comment.
Invariant violation: emit_typed_record_lit now completes missing struct fields using EmitTypeSummary plus rust_zero_value branches on string type names, conflicting with INVARIANTS.md “No case enumeration for open sets,” “Heuristics indicate lost structure,” and “No fallbacks that fabricate” when literals lack explicit field witnesses.
| @@ -3626,7 +3627,43 @@ let field_val = if needs_wrap.clone() { | |||
| }; | |||
There was a problem hiding this comment.
Invariant violation: The same default-filling block and rust_zero_value live in hand-edited stage0 in parallel with 05_emit_rust.dag, violating INVARIANTS.md “No duplicate representations” / “No parallel implementations” until regeneration is the only write path.
|
Review (INVARIANTS: 4, MODELING: 6+/6-, ROADMAP: 0✓/0!) INVARIANTS — Violations (4) ROOT CAUSE ANALYSIS
MODELING — Strengths
MODELING — Improvements
PR advances stable intern ids on module/import nodes and pragmatic Rust struct literal emission, but leaves two thesis-relevant gaps: emit still guesses defaults from strings instead of carried IR facts, and stage0 duplicates that policy until regen-only workflow holds. |
…nsumer test Three changes in one self-contained PR: 1. **Emitter default-value support**: rust_zero_value + all_defaultable guard in emit_typed_record_lit. New struct fields with known zero values (Int→0, Bool→false, String→"") are auto-emitted without touching every .dag construction site. Solves the bootstrap freshness problem permanently. 2. **Node.ident: Int field**: Added to Node alongside name. Parser pre-interns all tokens, then assigns real idents to module and import nodes via intern()/with. Transforms (map_children, with_optional/required_cardinality) preserve ident through rewrites. 3. **Consumer test**: module_node_ident_is_populated_by_parser proves the round-trip: parser assigns ident, intern_str recovers the original name. Stage0 regenerated and fixed-point verified (pass1 == pass2). 402 tests pass, ratchet at 0 diagnostics. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
dbcfada to
4225cf3
Compare
|
Re: P1 (summary name collisions) — already addressed by the Re: P2 (pre-interning perf) — valid observation. |
|
ChatGPT review in progress... (view conversation) |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
codex · gpt-5.4 · 4225cf3e
Review (INVARIANTS: 2, MODELING: 1+/2-, ROADMAP: 0✓/1!)
INVARIANTS — Violations (2)
ROOT CAUSE ANALYSIS
src/v2/00_core.dagMissing declaration-driven symbol authority → the parser reconstructs ids from raw token text,Nodestores that id alongsidename, and the rest of the graph is forced to fake absence with0; upstream fix is Track 3’s single-identity model: create symbols only when constructing named nodes, represent absence explicitly (Int?or a dedicated symbol type), thread that one authority throughTypeBindingand downstream consumers, and derive display text from spans/source rather than a parallelname + identpair (M1, M5, M8, M9).
MODELING — Strengths
src/v2/00_core.dagThe change keeps recursion onNodeitself and preserves the new field through existing structural rewrites, so it does not introduce a second recursive semantic authority.
MODELING — Improvements
src/v2/00_core.dagidentis not a faithful domain fact yet because it is just a parser-localInt; a more compositional model would make identity a single declaration-driven symbol authority with explicit absence until all producers can populate it, consistent with M1/M5/M9 and ROADMAP Track 3.src/v2/02_parse.dagIdentity creation belongs in parse, but it should be attached to authored names/module paths as nodes are constructed rather than by pre-interning every token lexeme, so the model composes from structure instead of lexer-order artifacts (M8/M9).
ROADMAP — Incomplete
- M2: Node.name deleted: The code adds
Node.identbut does not removeNode.name, fix the documentedauthored_name_atfallback blocker, or wire identity throughTypeBinding, so the milestone remains incomplete.
ROADMAP — Unclaimed
- Track 3: Structural identity / Node.name deletion: The diff advances the pending “InternTable as identity consumer” step by threading ids into module/import nodes, but ROADMAP.md still describes that step as deferred.
The PR moves identity work upstream into parsing, but the current Node.ident design is still duplicated, fabricated, and order-dependent, so it does not yet meet the thesis’s zero-bugs-by-construction bar.
|
|
||
| type Node { | ||
| name: String | ||
| ident: Int |
There was a problem hiding this comment.
Invariant violation: Making Node.ident a total Int adds a second authority for authored identity and leaves no structural way to represent “no identity,” violating INVARIANTS.md “No duplicate representations” and “No fallbacks that fabricate.”
| ) | ||
| } | ||
|
|
||
| fn pre_intern_tokens(tokens: List<Token>, table: InternTable) -> InternTable { |
There was a problem hiding this comment.
Invariant violation: pre_intern_tokens derives identity from every token lexeme, so ids shift with unrelated earlier syntax and literals instead of declaration structure, violating INVARIANTS.md “Root-Cause Depth Invariant” and “No duplicate representations.”
|
Review (INVARIANTS: 2, MODELING: 1+/2-, ROADMAP: 0✓/1!) INVARIANTS — Violations (2) ROOT CAUSE ANALYSIS
MODELING — Strengths
MODELING — Improvements
ROADMAP — Incomplete
ROADMAP — Unclaimed
The PR moves identity work upstream into parsing, but the current |
…am ctrl coupling Per loyal-swift-270 review at gunbc#846 #issuecomment-4384265438. Substantive cross-program engagement: 3 RED + 4 YELLOW + 4 GREEN findings, mapped to PM's 7 focus areas. §10.2.7 Research PM section added with full canvas absorption. §10.3 grew with 7 new escalations (1 of 7 deferred per Y-3 minor scope): Q-Class-6-Substrate-Extension-Lens (R-1) — 3 ctrl-side substrate proposals (SchemaPreservation/ResourceCap/TotalResult) don't fit Classes 1-5. PM-side routing to Substrate Mgr committed per ctrl#408 §3. Per Brian no-post-R3-deferral: include as Class 6 with closure-criterion. OPEN — substantial scope-determination (sixth substrate-gap class adds ~1 lane equivalent or scope-cession). Q-Timeline-Risk-Alternates (R-2) — §6 8-12 week estimate is commitment not derivation. Add risk-weighted "+X weeks if..." branches for C1-gap budget (2-6 wks/gap) + thesis-edit scope + demo-authoring buffer. RATIFIED-by- default — PM updates §9.2 in subsequent commit. Q-Plan-vs-Structure-Drift-Discipline (R-3) — §3 lane status replicates r3-structure.md data; drift risk if updated unevenly. Per Research PM recommendation (a): §3 → pointer-with-delta. RATIFIED-by-default — PM refactors §3 OR canonical-ledger-landing absorbs. Q-WEDGE-A (Y-1) — WEDGE-CORE-CLAIM Part A (ctrl#444) build-orchestration as free consequence. Per R3-as-consequence-layer-of-thesis: include as thesis-edit gate. OPEN — Director ratification. Q-Tier4-Inclusion (Y-1) — Tier 4 "out of scope, declared" per ctrl#1608. Convergent across LLVM/K8s/Discord/PyTorch viability portfolio. OPEN — Director ratification of inclusion-or-explicit-out-of-scope. Q-Lens-Self-Application-Stronger-Demo (Y-2) — current demo cashes "lens applies to CI workflow"; stronger shape ties lens output to emission gate (CI workflow → TestClaim → fails → gunbc emission refuses). Demonstrates 4 gates simultaneously + WEDGE-CORE-CLAIM Part B opportunistically. RATIFIED-by-default — Verification Mgr authors per gunbc#1276 cadence. Q-Director-PM-Redundancy (Y-4) — 30-min Director loop + weekly PM compile have no fallback for unavailability >2 days. Add deputy mechanisms. OPEN — Director / Brian ratification. Y-3 (Class boundary overlap Class 3∩4 + Class 5∩T-FixedPoint) deferred to canonical-ledger landing — minor scope-clarification. Cross-program coupling now first-class in plan: ctrl#444 / ctrl#1608 §6d / ctrl#369 / ctrl#370 / ctrl#404-#408 all referenced. Plan no longer treats ctrl side as silent dependency. GREEN affirmations: §1.6 demo principle + §6 dep DAG + §10 RATIFIED-default + §7 PR-authoring contract — Research-PM-validated as load-bearing. 7 of 8 reviewers complete (Verification ✓ Director ✓ Grounding ✓ Substrate ✓ Debt-Paydown ✓ Evaluator ✓ Research PM ✓). Pending: PB. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 participant
Summary
rust_zero_value+all_defaultableguard inemit_typed_record_lit. New struct fields with known zero values (Int→0, Bool→false, String→"") are auto-emitted, solving the bootstrap freshness problem permanently.intern()/with. Transforms (map_children,with_optional_cardinality,with_required_cardinality) preserve ident through rewrites.module_node_ident_is_populated_by_parserproves the round-trip: parser assigns ident →intern_strrecovers the original name.Test plan
🤖 Generated with Claude Code