Repository navigation
M4-L2: intern table + migrate n.name reads to authored_name_at - #367
Conversation
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 3, MODELING: 1+/1-, ROADMAP: 0✓/0!)
INVARIANTS — Violations (3)
ROOT CAUSE ANALYSIS
src/v2/compile.dagThe upstream gap is that structural identifier authority is still not carried onNodeitself, so this PR adds a second String-keyed identity table insrc/v2/00_core.dagand pipes it throughParserState,ParseResult, andFrontendResultas speculative metadata; because resolve/infer/emit still decide names fromauthored_name_atornode.name, the new table is an unused parallel representation. Fix it at the producer: either make the parser attach the stable identifier structurally to nodes and have downstream maps consume that authority end-to-end, or delete the table until an end-to-end consumer lands in the same change, per M4/M9 and the boundary-table rule in INVARIANTS.md.src/v2/00_core.dagThe upstream gap is that absence is not modeled structurally at the intern boundary: instead of preserving theNonecase fromintern_find, the helper erases it into a valid-looking ID, so any future consumer would silently treat an unknown identifier as the empty-name authority. The fix is upstream and compositional: keep lookup miss asInt?or a diagnostic-bearing result, and only collapse to a concrete ID where the producer can prove the name exists.src/v2/04_infer.dagThe upstream gap is that imported definitions still do not carry their own authoritative structural identity through the boundary, so infer re-derives names at the consumer; that works only for local nodes, and for parent-module nodes the importer's newline index points at the wrong file, causing authored names to collapse back to the stale bridge field. Fix the upstream authority flow: imported items and signatures should carry their producer-side identity forward from resolve/type-env, or at minimum this merge must read imported nodes withtyped_parent.type_env.source_indexinstead of the importer's index, in line with M1/M4/M9.
MODELING — Strengths
src/v2/03_resolve.dagResolving module/import/export names throughauthored_name_atmoves resolve closer to the source-authoritative path instead of scattering freshnode.namereads.
MODELING — Improvements
src/v2/00_core.dagInternTablemodels identity asMap<String, Int>, which is still a string-proxy layer under M4; make identifier authority structural onNodeand let downstream maps key by that structure, rather than introducing a second string-indexed identity model.
ROADMAP — Unclaimed
- M4 Lane 2 / D6 progress: This PR clearly advances the M4-L2
authored_name_atmigration and adds new identity plumbing, butROADMAP.mdwas not updated to record the additional D6 progress or the new intern-table detour.
The PR makes real M4-L2 progress, but it also adds an unused parallel intern-table authority, a new fail-open intern lookup, and an importer-scoped authored-name path that still breaks identity across module boundaries.
|
|
||
| // Shared frontend path: tokenize -> parse -> resolve. | ||
| // If parse fails, graph is None and diagnostics contains only parse diagnostics. | ||
| fn merge_intern_tables(tables: List<InternTable>) -> InternTable { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| } | ||
|
|
||
| // Reverse lookup with fail-closed default (returns 0 = empty if not found). | ||
| fn intern_find_or_empty(table: InternTable, s: String) -> Int { |
There was a problem hiding this comment.
Invariant violation: intern_find_or_empty fabricates ID 0 on miss, conflating “not interned” with the reserved anonymous-name ID and violating INVARIANTS.md No fallbacks that fabricate and MODELING.md M5.
| // Parent sigs always have concrete return types (inferred: Some). | ||
| let next_func_sigs = fold(typed_parent.func_env.signatures |> map_values, init: func_sigs, f: (acc, rsig) => | ||
| let sig_key = rsig.name | ||
| let sig_key = authored_name_at(source_index: env.source_index, node: rsig) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Review (INVARIANTS: 3, MODELING: 1+/1-, ROADMAP: 0✓/0!) INVARIANTS — Violations (3) ROOT CAUSE ANALYSIS
MODELING — Strengths
MODELING — Improvements
ROADMAP — Unclaimed
The PR makes real M4-L2 progress, but it also adds an unused parallel intern-table authority, a new fail-open intern lookup, and an importer-scoped authored-name path that still breaks identity across module boundaries. |
![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: e6967e84b1
ℹ️ 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".
| // Build a module index for O(1) lookup during import resolution. | ||
| let module_index = modules |> fold(init: empty_map(), f: (acc, m) => | ||
| map_insert(acc, m.name, m) | ||
| map_insert(acc, authored_name_at(source_index: map_get(source_indices, m.span.file), node: m), m) |
There was a problem hiding this comment.
Keying resolver maps with authored_name_at(...) here breaks module/import identity because parser-created module/import nodes do not carry identifier spans for the full dotted path (they currently use start_span from module/import keywords in parse_module/parse_import), so authored lookup can return keyword text rather than the declared path. In this commit that means module_index/imports_by_name keys can collapse and import lookups fail, yielding duplicate-module and unresolved-import diagnostics in normal multi-file compilation when source_indices is provided.
Useful? React with 👍 / 👎.
ChatGPT ReviewI found 3 issues worth flagging.
This looks like the most serious problem in the PR. The diff migrates a lot of chatgpt-review-9a8a81e0-139e-41… chatgpt-review-9a8a81e0-139e-41… 209 Why this matters: this PR changes resolver-ish code and compile serialization to depend on authored-name recovery in exactly the area the roadmap still treats as unfinished structural work. The roadmap’s D6 section still lists remaining This violates the “root-cause depth” / “boundary sufficiency” direction in the invariants: the fix should surface the right fact at the producer, not re-read text at the consumer. chatgpt-review-b517efb9-70d5-46… chatgpt-review-b517efb9-70d5-46… Suggested fix: don’t switch module/import identity sites to
The parser now interns identifiers and the pipeline threads/merges an It is also hard to justify this on performance grounds yet. The PR simultaneously adds many more So this is weak on both modeling and composition:
Suggested fix: either revert
Adding chatgpt-review-b517efb9-70d5-46… Suggested fix: delete A few lower-level notes:
Pasted markdown
chatgpt-review-90992fcb-34c1-40… Overall: I would block on item 1, and probably item 2 unless the intent is explicitly “scaffolding only” and you want to keep the PR narrowly mechanical. The deepest issue is that the producer boundary still does not carry correct structural identity for module/import names, so this PR moves more consumers onto a not-yet-sound source. |
ChatGPT ReviewI found 3 issues worth flagging.
This looks like the most serious problem in the PR. The diff migrates a lot of chatgpt-review-9a8a81e0-139e-41… chatgpt-review-9a8a81e0-139e-41… 209 Why this matters: this PR changes resolver-ish code and compile serialization to depend on authored-name recovery in exactly the area the roadmap still treats as unfinished structural work. The roadmap’s D6 section still lists remaining This violates the “root-cause depth” / “boundary sufficiency” direction in the invariants: the fix should surface the right fact at the producer, not re-read text at the consumer. chatgpt-review-b517efb9-70d5-46… chatgpt-review-b517efb9-70d5-46… Suggested fix: don’t switch module/import identity sites to
The parser now interns identifiers and the pipeline threads/merges an It is also hard to justify this on performance grounds yet. The PR simultaneously adds many more So this is weak on both modeling and composition:
Suggested fix: either revert
Adding chatgpt-review-b517efb9-70d5-46… Suggested fix: delete A few lower-level notes:
Pasted markdown
chatgpt-review-90992fcb-34c1-40… Overall: I would block on item 1, and probably item 2 unless the intent is explicitly “scaffolding only” and you want to keep the PR narrowly mechanical. The deepest issue is that the producer boundary still does not carry correct structural identity for module/import names, so this PR moves more consumers onto a not-yet-sound source. |
ChatGPT ReviewI reviewed the uploaded PR diff.
I see 3 blocking issues and 2 lower-level concerns.
In the diff, module/import identity sites in normalize/resolve/compile serialization are migrated from stored names to This is the exact shape the invariants warn about: a consumer-side re-read compensating for missing producer structure. The fix should be upstream — add a real module-name / import-path span (or equivalent structural authority) and then migrate these call sites atomically. INVARIANTS.md MODELING.md chatgpt-review-a91c1d5e-2ef0-4f… chatgpt-review-826d6293-8543-48…
I found multiple accessor invocations with
The PR adds
Directionally, I get the goal: stop leaning on stringly Relative to the modeling guidance and the “facts flow forward” invariant, this is weak: the structure is being threaded, but not yet used to eliminate the duplicated representation it was introduced to replace. INVARIANTS.md MODELING.md chatgpt-review-a91c1d5e-2ef0-4f… chatgpt-review-826d6293-8543-48…
Returning What I do think is good here: the accessor split itself is a compositional improvement. Replacing blanket I did not see test additions in this PR. Given the scope, I’d want at least:
Net: I would block on (1) and (2). (3) also looks very likely blocking. |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 3, MODELING: 2+/1-, ROADMAP: 0✓/1!)
INVARIANTS — Violations (3)
ROOT CAUSE ANALYSIS
src/v2/02_parse.dagThe upstream gap is that identity was bolted on as a new positional constructor parameter instead of being modeled once as a parser name product carrying text, symbol id, and span; that API change propagates downstream into stale parse helpers that still only have{name, span}, producing the constructor mismatch symptom. The upstream fix is to make parser name capture the single authority and have node builders consume that structural record so the M4 identity change lands once rather than across dozens of ad hoc call sites.src/v2/00_core.dagThe upstream gap is that symbol lookup is still modeled as a totalInt -> Stringeven though invalid ids are representable, so a missing fact is converted into a valid-looking empty identifier; that fabricated emptiness then propagates anywhere the caller treats""as anonymous. The upstream fix is to make intern lookup partial or diagnostic-bearing, reserve0only for explicitly anonymous nodes, and force consumers to handle missing symbol ids structurally per MODELING.md M5/M9.
MODELING — Strengths
src/v2/03_resolve.dagThreadingsource_indicesinto module and import resolution improves fidelity because authored names now come from source spans at the resolution boundary instead of being re-derived later.src/v2/00_core.dagThe PR keeps recursion anchored inNodeand does not introduce a second recursive semantic type, which stays aligned with the bounded-kernel part of MODELING.md M9.
MODELING — Improvements
src/v2/00_core.dagNode.ident: IntandInternTableare still a parallel identity lane besidenameandident_span; make identity a first-classSymbolId/parser-name product consumed structurally by builders, or keep source-derived names as the only authority until M4 deletesNode.name.
ROADMAP — Incomplete
- M4 Lane 2: Node.name deletion: The lane is still incomplete because
Node.nameremains a fallback semantic authority and the new intern-id path is not yet the single structural identity source.
ROADMAP — Unclaimed
- M4 Lane 2: Node.name deletion: This PR advances D6 by threading authored-name access through resolve and normalize, but ROADMAP.md was not updated to record that incremental progress.
The PR moves authored-name fidelity forward, but it still leaves the M4 identity model split, introduces a new fabrication fallback in interning, and partially migrates core constructor interfaces.
|
|
||
| // Param constructor: name carries param name, child[0] is type_expr, child[1] is default_value if present. | ||
| fn make_param_node(name: String, type_expr: Node, default_value: Node?, span: SourceSpan, name_span: SourceSpan) -> Node { | ||
| fn make_param_node(name: String, ident: Int, type_expr: Node, default_value: Node?, span: SourceSpan, name_span: SourceSpan) -> Node { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
|
||
| // Field constructor: extends Param with cardinality and from_key. | ||
| fn make_field_node(name: String, type_expr: Node, cardinality: Cardinality, default_value: Node?, from_key: String?, span: SourceSpan, name_span: SourceSpan) -> Node { | ||
| fn make_field_node(name: String, ident: Int, type_expr: Node, cardinality: Cardinality, default_value: Node?, from_key: String?, span: SourceSpan, name_span: SourceSpan) -> Node { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| } | ||
|
|
||
| // O(1) lookup: intern ID → string. | ||
| fn intern_str(table: InternTable, id: Int) -> String { |
There was a problem hiding this comment.
Invariant violation: intern_str returns "" for an invalid intern id, conflating unknown and anonymous identifiers and violating INVARIANTS.md No fallbacks that fabricate.
|
Review (INVARIANTS: 3, MODELING: 2+/1-, ROADMAP: 0✓/1!) INVARIANTS — Violations (3) ROOT CAUSE ANALYSIS
MODELING — Strengths
MODELING — Improvements
ROADMAP — Incomplete
ROADMAP — Unclaimed
The PR moves authored-name fidelity forward, but it still leaves the M4 identity model split, introduces a new fabrication fallback in interning, and partially migrates core constructor interfaces. |
ChatGPT ReviewI’d block this PR on 3 root-cause issues.
In the diff, chatgpt-review-5f8b7dbb-1ccb-48… chatgpt-review-fb9afbc0-74bf-42…
chatgpt-review-fb9afbc0-74bf-42… chatgpt-review-5f8b7dbb-1ccb-48…
Each parse starts from chatgpt-review-5f8b7dbb-1ccb-48… chatgpt-review-fb9afbc0-74bf-42… A couple of smaller review notes:
chatgpt-review-fb9afbc0-74bf-42…
The fastest way to make this safe would be:
I’d also want two regressions before merge: one for |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 2, MODELING: 1+/1-, ROADMAP: 0✓/0!)
INVARIANTS — Violations (2)
ROOT CAUSE ANALYSIS
src/v2/compile.dagThe upstream gap is thatidentwas introduced as a semantic authority without a globally coherent allocation model: parse assigns IDs per file,merge_intern_tablesrebuilds a fresh table atfront_end_sources, andauthored_name/import-service resolution later decode those old IDs as if they were global, which then propagates into wrong module, service, and type names; the upstream fix is to make symbol identity a single frontend authority by interning against one shared table during parse or by structurally remapping everyNode.identwhen tables are merged, in line with MODELING.md M2/M4/M9.src/v2/04_types.dagThe upstream gap is the same unstable identity model: many synthesized type nodes in this PR still carryident: 0, while parsed nodes carry file-local IDs, andnode_type_compatible/node_type_equals_corenow propagate that non-authoritative slot into semantic comparison; the upstream fix is to ground type identity in one canonical authority only, either a globally consistent symbol reference or a canonical authored-name lookup after resolution, rather than treating local intern slots as the algebra of type equality, per MODELING.md M2 and M9.
MODELING — Strengths
src/v2/03_resolve.dagRouting module/import/export naming throughauthored_name_atcomposes well with the existing accessor boundary and removes more directn.nameauthority from resolve-time logic.
MODELING — Improvements
src/v2/00_core.dagInternTable/InternResultcurrently model identity as a per-fileIntslot plus a later merge, but a more compositional and faithful model would make symbol identity a single parser-owned authority or explicit symbol reference carried through the graph, rather than reconstructing authority from merged lookup tables after the fact.
ROADMAP — Unclaimed
- M4 Lane 2 D6 open item “Migrate remaining
n.namereads”: This diff advances that item in03_normalize.dag,03_resolve.dag,04_access.dag, and04_service.dag, but ROADMAP.md was not updated to reflect the new progress.
The PR makes real M4-L2 progress on replacing raw n.name reads, but the new intern-ID path is not globally coherent and currently breaks semantic identity at both name lookup and type comparison.
|
|
||
| // Shared frontend path: tokenize -> parse -> resolve. | ||
| // If parse fails, graph is None and diagnostics contains only parse diagnostics. | ||
| fn merge_intern_tables(tables: List<InternTable>) -> InternTable { |
There was a problem hiding this comment.
Invariant violation: merge_intern_tables unions per-file intern tables without rewriting existing Node.ident values, so downstream name recovery reads stale local IDs against unrelated global slots and violates INVARIANTS.md Single-authority metadata / No duplicate representations.
| let right_is_container = node_is_element_collection(n: right, source_index: source_index) | ||
| if left_is_container && right_is_container { | ||
| if left.name != right.name { false } | ||
| if left.ident != right.ident { false } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Review (INVARIANTS: 2, MODELING: 1+/1-, ROADMAP: 0✓/0!) INVARIANTS — Violations (2) ROOT CAUSE ANALYSIS
MODELING — Strengths
MODELING — Improvements
ROADMAP — Unclaimed
The PR makes real M4-L2 progress on replacing raw |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 2, MODELING: 1+/2-, ROADMAP: 0✓/0!)
INVARIANTS — Violations (2)
ROOT CAUSE ANALYSIS
src/v2/04_env.dagTypeEnv is built in multiple places but only some receivefrontend.intern_table; the fold heuristic compensates instead of threading the sameInternTablefromfront_end_sourcesthrough everyTypeEnvandmerge_envs; upstream fix: pass the merged table into all env constructions and assert (or structurally enforce) one shared authority, removing the max-length pick.src/v2/04_service.dagNaming for operation nodes lacks one compositional extractor (e.g.node_symbol_name(env, node)using a single InternTable + optional source_index policy from00_core); downstream import merge and localanalyze_itemthen diverge; upstream fix: one shared helper used by both paths (aligned with M5/M9: one fact, one accessor).
MODELING — Strengths
src/v2/00_core.dagThreading parse-time intern IDs intoNode.identwithauthored_name_atfast-path is a clear step toward M4 structural identity and O(1) name comparisons when paired with a single global table.
MODELING — Improvements
src/v2/00_core.dagInternResult { table, id }and extended parse result shapes repeat state-threading; a single compositional “state + value” pattern (or reusing the existing result structs style) would reduce ad-hoc(table, id)pairs per MODELING.md M9 and the known duplicate-parse-result antipattern.src/v2/compile.dagmerge_intern_tablesre-interns string rows without remappingNode.identto the merged numbering; the compositional fix is either a remap pass over the graph after merge or a globally content-stable intern (e.g. canonical merge order + deterministic IDs) soidentstays the single authority—see M4 direction and root-cause depth invariant.
The PR advances M4 interning and authored_name_at migration but still relies on a non-authoritative intern merge without ID remap, fabricating intern lookups, and a few heuristic or split naming paths that undermine single-authority identity across the pipeline.
| ) | ||
| TypeEnv { bindings: merged_bindings, recursive_types: merged_recursive, recursive_type_set: merged_recursive_set, inductive_fields: merged_inductive_fields, source_index: source_index } | ||
| let intern_table = fold(envs, init: InternTable { strings: [], index: empty_map() }, f: (acc, env) => | ||
| if env.intern_table.strings |> count > acc.strings |> count { env.intern_table } else { acc } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| fn service_op_entry(child: Node) -> OpEntry { | ||
| OpEntry { name: child.name, outputs: inferred_to_outputs(inferred: child.inferred, span: child.span), params: child.params } | ||
| fn service_op_entry(child: Node, source_index: NewlineIndex?) -> OpEntry { | ||
| OpEntry { name: authored_name_at(source_index: source_index, node: child), outputs: inferred_to_outputs(inferred: child.inferred, span: child.span), params: child.params } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Review (INVARIANTS: 2, MODELING: 1+/2-, ROADMAP: 0✓/0!) INVARIANTS — Violations (2) ROOT CAUSE ANALYSIS
MODELING — Strengths
MODELING — Improvements
The PR advances M4 interning and |
ChatGPT ReviewI’d block this PR on three correctness issues.
The parser now interns names into a per-file chatgpt-review-5f8b7dbb-1ccb-48… chatgpt-review-fb9afbc0-74bf-42…
I still see raw chatgpt-review-fb9afbc0-74bf-42… chatgpt-review-fb9afbc0-74bf-42…
In chatgpt-review-fb9afbc0-74bf-42… On the
chatgpt-review-fb9afbc0-74bf-42… A few smaller review notes:
The two tests I’d want before merge are:
|
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 4, MODELING: 1+/1-, ROADMAP: 0✓/0!)
INVARIANTS — Violations (4)
ROOT CAUSE ANALYSIS
src/v2/00_core.dagUpstream gap: M4-L2 still has no single model for “how a node’s authored identity is rendered,” so parse stores bothNode.nameandNode.identand this fast path chooses the copied string as authority again; that duplication then hides stale-intern-table bugs downstream because callers see a plausible name even whenidentno longer points at the same fact. Fix upstream by making one authority explicit: authored names come fromSourceSpanwhen source-backed, synthetic names come fromInternTablewhen parser-synthesized, andNode.namebecomes derived/deletable instead of authoritative (MODELING.md M4/M9, ROADMAP M4-L2).src/v2/04_types.dagUpstream gap: the interned-identity migration is being threaded function-by-function instead of through one shared identity/environment abstraction, so the refactor is compensating at every helper boundary and the signature rewrite itself drifted out of coherence; that malformed boundary then propagates to every caller that needs type equality/compatibility. Fix upstream by introducing one canonical identity carrier for type utilities (for example aTypeEnv/lookup context or a single accessor layer) and have all type-comparison helpers consume that boundary once rather than duplicating rawsource_indexandintern_tableparameters across the API surface (MODELING.md M9, INVARIANTS.md Clear interfaces).
MODELING — Strengths
src/v2/00_core.dagCapturing identifier strings once at parse time inInternTablemoves name recovery closer to the parser authority boundary instead of repeatedly slicing source text downstream.
MODELING — Improvements
src/v2/00_core.dagInternResultis a bespoke state-threading pair and the surrounding API still keepsNode.nameas parallel authority; a more compositional model is one generic state/result carrier plus one name authority (SourceSpanfor authored text,InternTablefor synthetic text) so M4-L2 can actually deleteNode.name.
ROADMAP — Unclaimed
- M4 Lane 2 D6 open item “Migrate remaining n.name reads”: This PR clearly advances that item by replacing several resolve/normalize/service reads with intern-based lookup, but ROADMAP.md was not updated and the migration is still partial.
The PR makes real progress on parser-owned identifier authority, but it still leaves duplicate naming authority in authored_name_at and introduces malformed type-identity interfaces, so the M4-L2 move is not yet sound.
| None => node.name | ||
| // Fast path: if ident is wired, name is authoritative (set during parse). | ||
| // Avoids expensive source_text_at derivation for all interned nodes. | ||
| if node.ident != 0 { node.name } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| } | ||
|
|
||
| fn node_type_compatible(left: Node, right: Node, source_index: NewlineIndex?) -> Bool { | ||
| fn node_type_compatible(left: Node, right: Node, source_index: NewlineIndex?, intern_table: InternTable, intern_table: intern_table) -> Bool { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| } | ||
|
|
||
| fn node_type_equals(left: Node, right: Node, source_index: NewlineIndex?) -> Bool { | ||
| fn node_type_equals(left: Node, right: Node, source_index: NewlineIndex?, intern_table: InternTable, intern_table: intern_table) -> Bool { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| } | ||
|
|
||
| fn node_type_equals_core(left: Node, right: Node, source_index: NewlineIndex?) -> Bool { | ||
| fn node_type_equals_core(left: Node, right: Node, source_index: NewlineIndex?, intern_table: InternTable, intern_table: intern_table) -> Bool { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Review (INVARIANTS: 4, MODELING: 1+/1-, ROADMAP: 0✓/0!) INVARIANTS — Violations (4) ROOT CAUSE ANALYSIS
MODELING — Strengths
MODELING — Improvements
ROADMAP — Unclaimed
The PR makes real progress on parser-owned identifier authority, but it still leaves duplicate naming authority in |
ChatGPT ReviewI wouldn’t merge this yet. The core problem is that the PR starts treating
chatgpt-review-fb9afbc0-74bf-42…
chatgpt-review-09854b37-9778-48…
On the chatgpt-review-bc715706-48b0-4d…
chatgpt-review-09854b37-9778-48… A few smaller notes:
Before this merges, I’d want at minimum:
So my review is: request changes / block. The PR is moving the compiler onto a new identity mechanism before that mechanism is globally stable. |
ChatGPT ReviewI’d block this PR.
In
In
This is not just “transitional debt.” The diff contains logic that is plainly wrong:
The intent is good: make identifier handling explicit. The actual model is not. Identity is now split across:
This PR touches parse, resolve, infer, emit, compile, complexity, and ownership, but the diff adds no targeted tests. At minimum, I’d expect:
Even though The fix order I’d recommend is:
|
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 7, MODELING: 1+/1-, ROADMAP: 0✓/1!)
INVARIANTS — Violations (7)
ROOT CAUSE ANALYSIS
src/v2/00_core.dagDeletingNode.nameremoved the only ubiquitous semantic-name authority, but the change did not introduce a single replacement boundary overnode.ident+InternTable; infer item metadata, call-graph construction, emit summaries, workflow emission, and complexity analysis therefore patch the gap with"", which propagates downstream as collapsed registries, lost SCC edges, and disabled proofs. The upstream fix is one canonical semantic-identity API at the typed boundary, or betterInternId/Node-keyed maps instead ofMap<String, X>identity proxies, so phases consume the same authority and only render strings at the edge, aligned with MODELING.md M1/M4/M8/M9.src/v2/00_core.dagThe parser still acceptsfrom_keyandmake_field_nodestill receives it, but the migration keeps that fact in an anonymous property node with no surviving identity and then drops it infield_node_from_key, so downstream emit falls through to default or empty JSON paths. The upstream fix is to modelfrom_keyas an explicit field fact, such asfrom_key: InternId?or a dedicated structural child/summary, and have emit consume that authority directly rather than reconstructing it, per MODELING.md M1/M8/M9.
MODELING — Strengths
src/v2/00_core.dagIntroducingInternTablemakes the split between authored rendering and semantic identity explicit, which is the right compositional direction for D6 if the same authority is consumed end-to-end.
MODELING — Improvements
src/v2/00_core.dagInternTablecurrently lands as rawInt/Stringplumbing; make identity a first-class boundary type or node-edge key so downstream maps stop reintroducing string proxies and""sentinels, matching MODELING.md M4/M9 and ROADMAP.md’s “resolve uses structural identity” goal.
ROADMAP — Incomplete
- M4 Lane 2 acceptance: The diff deletes
Node.name, but the remaining+lines replace structural identity with""across infer, sigs, complexity, and emit, so the ROADMAP acceptance “resolve uses structural identity” is not yet satisfied.
ROADMAP — Unclaimed
- M4 Lane 2 / D6 progress: PR #367 clearly advances the lane by adding
InternTableplumbing and migrating constructors, butROADMAP.mdstill stops at PR #362 and does not record this attempt or its current blockers.
The PR moves Node.name deletion forward, but it currently replaces too many remaining semantic identities with "", so it violates core identity invariants and is not ready to land.
| @@ -3803,7 +3373,7 @@ fn build_item_info(item: Node, source_index: NewlineIndex?) -> ItemInfo { | |||
| match kind { | |||
| FuncItem => | |||
| ItemInfo { | |||
| name: item.name, | |||
| name: "", | |||
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| flat_map(items, item => | ||
| if item.params |> count > 0 && item.body != none { | ||
| collect_calls_in_expr(caller: item.name, texpr: item.body.value, local_func_set: local_func_set, source_index: source_index) | ||
| collect_calls_in_expr(caller: "", texpr: item.body.value, local_func_set: local_func_set, source_index: source_index) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| match child.inferred { | ||
| Some { value: Resolved { node: ft } } => | ||
| let resolved_name = normalize_access_type_node(n: ft).name | ||
| let resolved_name = "" |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
|
||
| fn to_workflow_func(item: Node, module_name: String, registry: Map<String, ItemInfo>, source_index: NewlineIndex?) -> WorkflowFunc { | ||
| let svc_names = match lookup_item(registry: registry, name: item.name) { | ||
| let svc_names = match lookup_item(registry: registry, name: "") { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| let idx = pair.first | ||
| let param = pair.second | ||
| if param_node_type_expr(n: param).name == "ParserState" { | ||
| if "" == "ParserState" { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| None => | ||
| let pname = param_node_name_at(n: p, source_index: si) | ||
| let type_name = param_node_type_expr(n: p).name | ||
| let type_name = "" |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| } else { none } | ||
| } | ||
|
|
||
| fn field_node_from_key(n: Node) -> String? { |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Review (INVARIANTS: 7, MODELING: 1+/1-, ROADMAP: 0✓/1!) INVARIANTS — Violations (7) ROOT CAUSE ANALYSIS
MODELING — Strengths
MODELING — Improvements
ROADMAP — Incomplete
ROADMAP — Unclaimed
The PR moves |
ChatGPT ReviewBlocking.
chatgpt-review-2bad5044-c64c-4b… The main blocker is the new intern-table story. Each file now parses with its own local intern table, then chatgpt-review-984ac44d-888b-43… The second blocker is that the migration away from That leads straight to the third blocker: the PR replaces missing facts with fabricated empty strings all over the place. chatgpt-review-984ac44d-888b-43… There is also at least one outright fact deletion: Beyond the architectural issues, there are already concrete behavior regressions in the diff:
Those are not “wrapper migration” nits. They are obviously wrong leaf behavior caused by the broken upstream identity migration. On compositional modeling quality, I only see one clearly positive move: centralizing transport config key strings into constants is a good single-authority cleanup. But even there the change is only partial; transport kind still ends up string-dispatched rather than fully structural. More broadly, the PR makes composition worse by splitting identifier authority across three channels:
That is a downgrade from a compositional-modeling perspective. A good M4-L2 change would leave one authoritative carrier for identity at each boundary, not a side table plus source recovery plus fake defaults. Testing is also not where it needs to be. This diff touches parse, resolve, infer, emit, ownership, complexity, and compile, but adds no focused regression coverage. At minimum I would expect:
So my recommendation is: do not merge this as-is. The right fix order is:
|
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 7, MODELING: 1+/2-, ROADMAP: 0✓/1!)
INVARIANTS — Violations (7)
ROOT CAUSE ANALYSIS
src/v2/00_core.dagM4 removed the old textual authority before a replacement accessor boundary existed, so functions like param_node_name, field_init_node_name, expr_var_name, field_access_field, and expr_call_func encode absence as "". That fabricated sentinel then propagates into lookups, dispatch, and emit decisions downstream. Replace these APIs with ident plus InternTable or _at(NewlineIndex?) boundaries and delete the fail-open string accessors entirely (M4/M5/M8).src/v2/00_core.dagKernel types lost their textual authority before gaining structural authorities of their own, so downstream code now reads anonymous built-ins through ident and intern_str and can no longer distinguish them reliably. Define kernel types as canonical declaration nodes or canonical interned ids once, then derive these constants from that authority instead of hand-building anonymous leaves (M4/M9).src/v2/04_types.dagThe upstream gap is that M4 removed Node.name without first introducing one authoritative constructor for synthetic named nodes, so callers now re-encode names ad hoc as missing idents, string pseudo-idents, or ident 0. Add a single synthetic named-node builder that takes a real ident or an explicit error and reuse it everywhere types/infer/emit synthesize named Nodes (M4/M5/M8).src/v2/04_types.dagAlgebraTypeTemplate still expects carrier identity to be recoverable as a string; after M4 removed name authority, SameAsReceiver needs a structural receiver witness, not an empty-string sentinel. Carry the receiver declaration edge or ident through template instantiation and reuse std/algebra.dag composition directly instead of reconstructing carrier identity from names (M8/M9).src/v2/05_emit_go.dagThe module-identity boundary was not replaced before Node.name removal, so the language emitters compensate with anonymous sentinels instead of receiving one structural module authority. Thread module.ident plus InternTable into shared module-emission helpers and derive filenames and import paths from that single source, with LanguageSpec handling only syntax rendering.
MODELING — Strengths
src/v2/00_core.dagInternTable places authored-identifier capture at the parser boundary, which is the right authority point for M4 even though later id-rewriting is still incomplete.
MODELING — Improvements
src/v2/02_parse.dagNameResult still extends the bespoke parser result family instead of reusing one generic parser carrier, so the diff continues the M6 value/state/error type proliferation.src/v2/04_types.dagSameAsReceiver should carry a structural receiver witness rather than a carrier string, so container and callable synthesis composes from std/algebra.dag without ad-hoc name recovery.
ROADMAP — Incomplete
- M4 Lane 2: Node.name deletion: The PR advances intern-table plumbing, but D6 and B4 are still incomplete because fabricated "" accessors, anonymous kernel types, and synthetic nodes without ident leave structural identity unsound.
ROADMAP — Unclaimed
- M4 Lane 2 intern-table plumbing: ParserState, ParseResult, FrontendResult, and multiple infer, type, and emit boundaries now carry InternTable, but ROADMAP.md was not updated to record that partial M4-L2 progress.
The PR moves identity plumbing forward, but it still deletes textual authority before structural authorities exist, so the new code introduces fresh fabrication and synthetic-node identity violations that make the M4-L2 migration unsafe.
| } | ||
|
|
||
| fn param_node_name(n: Node) -> String { n.name } | ||
| fn param_node_name(n: Node) -> String { "" } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| data int_type: Node = Node { name: "Int", span: make_span(start: 0, end: 0), ident_span: Some { value: make_span(start: 0, end: 0) }, children: [], connective: NoConnective, params: [], inferred: none, return_cardinality: Required, uses: [], body: none, transport: none, properties: [], type_annotation: none, is_self_recursive: false, has_non_tail_self_call: false, match_pattern: none, expr_data: NoExprData } | ||
| data float_type: Node = Node { name: "Float", span: make_span(start: 0, end: 0), ident_span: Some { value: make_span(start: 0, end: 0) }, children: [], connective: NoConnective, params: [], inferred: none, return_cardinality: Required, uses: [], body: none, transport: none, properties: [], type_annotation: none, is_self_recursive: false, has_non_tail_self_call: false, match_pattern: none, expr_data: NoExprData } | ||
| data none_type: Node = Node { name: "None", span: make_span(start: 0, end: 0), ident_span: Some { value: make_span(start: 0, end: 0) }, children: [], connective: NoConnective, params: [], inferred: none, return_cardinality: Required, uses: [], body: none, transport: none, properties: [], type_annotation: none, is_self_recursive: false, has_non_tail_self_call: false, match_pattern: none, expr_data: NoExprData } | ||
| data unit_type: Node = Node { span: make_span(start: 0, end: 0), ident_span: Some { value: make_span(start: 0, end: 0) }, children: [], connective: Conj, params: [], inferred: none, return_cardinality: Required, uses: [], body: none, transport: none, properties: [], type_annotation: none, is_self_recursive: false, has_non_tail_self_call: false, match_pattern: none, expr_data: NoExprData } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| let param_name = container_param_name_required(kind_name: kind_name, index: 0) | ||
| Node { name: kind_name, span: make_span(start: 0, end: 0), ident_span: default_ident_span(name: kind_name, span: make_span(start: 0, end: 0)), children: [ | ||
| Node { name: param_name, span: make_span(start: 0, end: 0), ident_span: default_ident_span(name: param_name, span: make_span(start: 0, end: 0)), children: [], connective: NoConnective, params: [], inferred: Some { value: Resolved { node: element } }, return_cardinality: Required, uses: [], body: none, transport: none, properties: [], type_annotation: none, is_self_recursive: false, has_non_tail_self_call: false, match_pattern: none, expr_data: NoExprData } | ||
| Node { span: make_span(start: 0, end: 0), ident_span: default_ident_span(ident: kind_name, span: make_span(start: 0, end: 0)), children: [ |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| // unbound type variables in the algebra declaration, NOT concrete types. | ||
| fn nominal_type_ref(name: String) -> Node { | ||
| leaf_node_with_span(name: name, span: make_span(start: 0, end: 0)) | ||
| leaf_node_with_span(ident: 0, span: make_span(start: 0, end: 0)) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| AlgebraTypeVariable { id: var_id } => type_variable_node(id: var_id) | ||
| ContainerOf { source: src, element: inner } => | ||
| let kind_name = match src { SameAsReceiver => base.name Named { name: n } => n } | ||
| let kind_name = match src { SameAsReceiver => "" Named { name: n } => n } |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| let m = typed_module.module | ||
| let scope = module_emit_scope(typed_module: typed_module) | ||
| let pkg_name = go_package_name(module_name: m.name) | ||
| let pkg_name = go_package_name(module_name: "") |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| |> map(item => emit_py_typed_item(item: item, registry: registry, scope: scope)) | ||
| |> join(separator: "\n\n\n") | ||
| let filename = module_to_filename(name: m.name) | ||
| let filename = module_to_filename(name: "") |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
Review (INVARIANTS: 7, MODELING: 1+/2-, ROADMAP: 0✓/1!) INVARIANTS — Violations (7) ROOT CAUSE ANALYSIS
MODELING — Strengths
MODELING — Improvements
ROADMAP — Incomplete
ROADMAP — Unclaimed
The PR moves identity plumbing forward, but it still deletes textual authority before structural authorities exist, so the new code introduces fresh fabrication and synthetic-node identity violations that make the M4-L2 migration unsafe. |
ChatGPT ReviewBlocking.
The biggest blocker is the new intern-table design. Each file now parses with its own There is a second, even more concrete producer-side problem: many named nodes stop carrying identity at all. In The third blocker is fail-open fabrication.
chatgpt-review-2fca4d93-d04d-48… That fabrication is already causing obvious downstream behavior regressions in this diff.
On compositional modeling quality, the intent is good but the resulting model is worse than the pre-change one. Right now identifier authority is split across four mechanisms:
That is multi-authority identity, not compositional structure. Existing std-library patterns go the other way: optional lookup instead of sentinel fabrication ( chatgpt-review-984ac44d-888b-43… A few more concrete review notes:
The tests I’d consider minimum before merge:
Recommendation: don’t merge as-is. The fix order should be:
|
ChatGPT ReviewI reviewed the uploaded PR diff against the attached invariants/modeling docs and existing patterns.
chatgpt-review-390128df-e8ed-4d… The main issues I see:
That means IDs are still file-local when the downstream stages start decoding them against the merged/global table. Two files can both have This is the deepest/root-cause issue in the PR: it introduces a lossy boundary/fact table instead of a single authoritative identity flow, which the invariants explicitly call out as invalid. chatgpt-review-390128df-e8ed-4d… INVARIANTS
In So
The new helper returns Those keys are synthetic compiler vocabulary, not necessarily strings that appeared in parsed source. If they weren’t interned earlier, they silently collapse to the anonymous/empty sentinel. That violates the “no fallbacks that fabricate” rule and also makes later property lookup ambiguous. The docs are pretty direct that silent defaults and lossy fact tables are not acceptable steady state. chatgpt-review-390128df-e8ed-4d… INVARIANTS
I found multiple cases in the emitters where a prior This is not harmless debt. Those empty strings change emitted behavior:
A number of raw Depending on how strict the DAG compiler is here, that’s either:
I saw helper signatures change to require
This should not be in the change. It is unrelated to M4-L2 and violates the repo’s own branch-scoping guidance for review queue work. chatgpt-review-390128df-e8ed-4d… On compositional modeling quality for the The direction is reasonable: moving identity toward parser-owned interned IDs could be a good M4-L2 step. The problem is that it does not land as a single end-to-end authority yet. Right now the PR creates two identity representations:
without an equivalence-preserving transition between them. That is exactly the kind of lossy boundary the invariants warn about. The docs explicitly say new semantic boundaries must land end-to-end and lossy fact tables are already invariant violations. chatgpt-review-390128df-e8ed-4d… I’d summarize the root cause like this:
Tests I’d want before merge:
Net: I would not merge this as-is. The intern-table idea is promising, but the current diff looks structurally incomplete and introduces at least one serious correctness regression. |
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 12, MODELING: 1+/1-, ROADMAP: 0✓/0!)
INVARIANTS — Violations (12)
ROOT CAUSE ANALYSIS
src/v2/compile.dagResolvedGraph already carries the item node, but the analysis boundary drops that authority when it constructs FuncEntry and OwnershipProof inputs; the missing upstream identity then propagates into func_index keys, SCC membership, self-call checks, and proof labels. Fix upstream by carrying the item node or a stable symbol reference through the complexity and ownership boundaries and deriving printable names only at diagnostics, per MODELING.md M4 and M5.src/v2/02_parse.dagThe parser captures identifier ids through expect_ident and expect_name elsewhere, but the import-names path drops that upstream fact and emits anonymous leaves; resolve_import then treats child.ident as the authority for export-membership checks, so specific imports collapse to "". Fix upstream by interning dotted import names at parse time and threading that ident through the import child nodes, or by keeping resolve on authored source text until stable symbol refs exist, per MODELING.md M4 and M5.src/v2/04_items.dagVariant constructor identity is still modeled as a Map<String, TypeBinding> keyed by names; after Node.name deletion the code patches that missing upstream fact with "", so every constructor aliases to one entry downstream in infer. Fix upstream by keying variant locals with a structural child reference or stable symbol reference carried from the type declaration, and render names only at diagnostics, per MODELING.md M4 and M5.src/v2/04_patterns.dagPattern analysis still depends on string recovery to resolve the scrutinee type and enumerate variants; after deleting Node.name the missing upstream fact is patched with "", which then propagates into both lookup_type and the exhaustiveness set comparison. Fix upstream by carrying the resolved type node and variant child references through the pattern boundary and computing exhaustiveness structurally from those nodes, per MODELING.md M4 and M5.src/v2/complexity.dagHigher-order method cost analysis needs a stable semantic slot for the callback argument, but that upstream fact is absent and this helper compensates with a fabricated string match on ""; the gap then propagates into wrong callback summaries. Fix upstream by recording callback position or name on MethodSemantics, or by threading the existing interned arg identity into this helper, so complexity reads a structural fact instead of guessing, per MODELING.md M4, M5, and M9.src/v2/05_emit_rust.dagAfter Node.name deletion the emit boundary still does not carry structural import, method, field, and type facts, and the backends compensate by substituting ""; that missing upstream authority propagates into wrong imports, wrong method dispatch, and anonymous extraction code across targets. Fix upstream by enriching reconcile and emit data with resolved item, type, method, and resource references and letting LanguageSpec supply only syntax, which aligns with MODELING.md M4 and M9 and INVARIANTS.md Emission is translation, not decision-making.src/v2/compile.dagArtifactPlan still models inter-artifact edges by string names, but this diff deletes the populated names without replacing them with structural artifact references, so validation can only compare fabricated empties. Fix upstream by carrying artifact identity structurally on the plan boundary, or by preserving the real artifact name until that reference exists, per MODELING.md M4 and M5.
MODELING — Strengths
src/v2/00_core.dagInternTable sits at the compiler-core boundary and models parser-authoritative symbol capture explicitly, which is more faithful than treating source-text recovery as semantic authority later.
MODELING — Improvements
src/v2/04_types.dagSynthesized type builders still compose from String kind names plus intern_find_or_empty; make them consume declaration nodes or bound type refs so container and nominal identity emerge from the existing type environment, which would be more compositional under MODELING.md M4 and M9.
ROADMAP — Unclaimed
- M4 Lane 2: Node.name deletion: This PR materially advances D6 by deleting Node.name and threading parser interning through parse, resolve, normalize, and reconcile, but ROADMAP.md was not updated to record the cutover or the remaining empty-string identity regressions.
The PR makes real M4-L2 progress, but it still erases semantic identity in analysis, pattern matching, artifact validation, and emission, so the cutover is not yet invariant-safe.
| m.items |> filter(item => item.body != None) |> map(item => | ||
| FuncEntry { | ||
| name: item.name, | ||
| name: "", |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| typed.modules |> flat_map(m => | ||
| m.items |> filter(item => item.body != None) |> map(item => | ||
| analyze_ownership(func_name: item.name, params: item.params, body: item.body.value, si: m.type_env.source_index) | ||
| analyze_ownership(func_name: "", params: item.params, body: item.body.value, si: m.type_env.source_index) |
There was a problem hiding this comment.
Invariant violation: extract_ownership_proofs calls analyze_ownership with func_name "", erasing function identity at the ownership boundary and violating INVARIANTS.md No fallbacks that fabricate and Single-authority metadata.
| @@ -1115,7 +1128,7 @@ fn parse_import_names_acc(tokens: List<Token>, state: ParserState, acc: List<Nod | |||
| let name_span = current_span(tokens: tokens, state: s) | |||
| let r = parse_dotted_ident(tokens: tokens, state: s) | |||
| if has_err(err: r.err) { return NamesResult { names: [], state: r.state, err: r.err } } | |||
| let name_node = parsed_name_leaf(name: r.name, span: name_span) | |||
| let name_node = parsed_name_leaf(ident: 0, span: name_span) | |||
There was a problem hiding this comment.
Invariant violation: parse_import_names_acc constructs every specific-import name leaf with ident 0, so resolve later sees every requested export as anonymous and violates INVARIANTS.md Single-authority metadata and No fallbacks that fabricate.
| fold(item.children, init: acc, f: (vacc, child) => | ||
| map_insert(vacc, child.name, TypeBinding { | ||
| name: child.name, | ||
| map_insert(vacc, "", TypeBinding { |
There was a problem hiding this comment.
Invariant violation: variant_locals_from_items inserts every variant local under the key "", collapsing distinct constructors in env bindings and violating INVARIANTS.md No duplicate representations and MODELING.md M4.
| scrutinee_type | ||
| } else { | ||
| match lookup_type(env: env, name: scrutinee_type.name) { | ||
| match lookup_type(env: env, name: "") { |
There was a problem hiding this comment.
Invariant violation: check_match_exhaustiveness resolves every leaf scrutinee with lookup_type(env, name: ""), so named sum types stop resolving structurally and violate INVARIANTS.md Correctness by construction and No fallbacks that fabricate.
| let std_imports = collect_go_std_imports(has_services: has_services, has_types: has_types, has_functions: has_functions) | ||
| let pkg_imports = imports |> map(imp => | ||
| let mod_name = module_to_filename(name: imp.name) | ||
| let mod_name = module_to_filename(name: "") |
There was a problem hiding this comment.
Invariant violation: emit_go_imports derives every imported module filename from "", so generated Go imports lose module identity and violate INVARIANTS.md No fallbacks that fabricate and Single-authority metadata.
| } else { | ||
| let import_lines = imports |> map(imp => | ||
| let mod_name = module_to_filename(name: imp.name) | ||
| let mod_name = module_to_filename(name: "") |
There was a problem hiding this comment.
Invariant violation: emit_py_imports does the same module_to_filename(name: "") collapse, so Python imports lose module identity under the same invariants.
| // BRIDGE: reads "" until Lane C provides per-method | ||
| // rendering from language extdeps keyed by the definition node. | ||
| let method_name = method_def.name | ||
| let method_name = "" |
There was a problem hiding this comment.
Invariant violation: emit_typed_method_call zeroes method_name before algebra-method dispatch, so Rust rendering stops following the reconciled method authority and violates INVARIANTS.md Emission is translation, not decision-making and No fallbacks that fabricate.
| let children = rt.children | ||
| let extract_lines = children |> map(ch => | ||
| let field_name = emit_ident(name: ch.name, target: Rust) | ||
| let field_name = emit_ident(name: "", target: Rust) |
There was a problem hiding this comment.
Invariant violation: emit_from_key_extraction fabricates empty field and type names for every extracted output, violating INVARIANTS.md No duplicate representations and Correctness by construction.
|
|
||
| fn validate_boundaries(plan: ArtifactPlan) -> List<ErrorNode> { | ||
| let names = plan.artifacts |> map(a => a.name) | ||
| let names = plan.artifacts |> map(a => "") |
There was a problem hiding this comment.
Invariant violation: validate_boundaries rewrites every artifact name to "", so boundary validation no longer checks the actual artifact graph and violates INVARIANTS.md No duplicate representations and Explicit boundary contracts.
|
Review (INVARIANTS: 12, MODELING: 1+/1-, ROADMAP: 0✓/0!) INVARIANTS — Violations (12) ROOT CAUSE ANALYSIS
MODELING — Strengths
MODELING — Improvements
ROADMAP — Unclaimed
The PR makes real M4-L2 progress, but it still erases semantic identity in analysis, pattern matching, artifact validation, and emission, so the cutover is not yet invariant-safe. |
ChatGPT ReviewI reviewed the diff against the attached invariants/modeling docs and the current M4/D6 roadmap state.
The biggest issues I see:
The parser now starts each file with its own That means two files can both have
That is a direct regression in both correctness and diagnostics. It also likely hits lambda parameter identity for any path that relies on
I found many call sites where a previous semantic name read was not migrated to
The new helper returns The attached invariants are explicit that silent defaults, fabrication fallbacks, and placeholder emissions are violations, and that the fix belongs upstream where the fact originates, not in a downstream compensating fallback. chatgpt-review-22570431-7651-42… chatgpt-review-6b362761-6993-4a…
The helper now takes with no
That matches the attached roadmap’s D6 note that accessor migration was only a phase, not the endpoint: resolve still needed structural identity, constructor work remained, and
Since
This should be dropped. It is unrelated review-tool state, not part of M4-L2. On compositional modeling quality for the The direction is right: moving identity upstream and killing direct
without a sound transition between them. That is exactly the kind of upstream fact loss / downstream compensation the attached invariants and roadmap call out as the root cause class. chatgpt-review-6b362761-6993-4a… chatgpt-review-f4628320-ee17-47… The same concern shows up in transport/config modeling. Existing direction says emit should consume the transport/config facts it already has, directly, instead of rediscovering them. This PR still leans on synthetic property keys plus fallback lookup, which is weaker than a first-class structural carrier. chatgpt-review-6b362761-6993-4a… Net assessment:
Tests I’d want before merge:
|
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 9, MODELING: 2+/2-, ROADMAP: 0✓/0!)
INVARIANTS — Violations (9)
ROOT CAUSE ANALYSIS
src/v2/02_parse.dagUpstream gap: after deleting Node.name, the parser still lacks single-authority constructors for named declarations and members. Chain: expect_ident now produces name_id, but raw Node literals for ops, types, and block items either cannot accept ident or ignore it, so items are born anonymous and every later intern_str(item.ident) lookup in infer, complexity, ownership, and emit collapses to empty strings. Upstream fix: add core constructors for every named item/member that require ident plus ident_span and replace raw Node literals with those authorities, per MODELING.md M1/M2/M9.src/v2/00_core.dagUpstream gap: there is no core copy/update API that preserves ident by default when a Node is rewritten. Chain: wrappers in 00_core and rewrites in resolve manually rebuild records, forget ident, and downstream intern-table consumers then see anonymous nodes even when parse assigned an identity. Upstream fix: introduce identity-preserving copy/update helpers in 00_core and route all structural rewrites through them, which matches MODELING.md M1/M2/M9 and keeps Node as the sole recursive authority.src/v2/02_parse.dagUpstream gap: exit entries are still modeled as ad hoc field-init strings instead of a structured exit-entry declaration. Chain: after Node.name deletion the parser has no authority surface for exit-key or output-type identity, so it stringifies, then drops both facts into ident 0 placeholders, and downstream exit typing/emission cannot recover them. Upstream fix: model exit/response entries as structured nodes with an interned status key and a direct Node edge to the output type, leaving string rendering to emit, per MODELING.md M1/M8/M9.src/v2/compile.dagUpstream gap: ownership and Rust emit still consume raw String identities, but the PR does not replace that boundary with structural item/type authority. Chain: compile and emit helpers compensate with empty strings, which then poison ownership proof names, service-module indices, and extdep type lookups. Upstream fix: carry canonical item/type identity across these boundaries as ident plus intern_table or a structural ItemRef/TypeCheckpoint surface, and derive display names once from that authority instead of fabricating blanks, consistent with MODELING.md M1/M7/M9.
MODELING — Strengths
src/v2/00_core.dagMoving semantic identity from Node.name into Node.ident plus an explicit InternTable is the right compositional direction: authored text becomes a recoverable view instead of core authority.src/v2/02_parse.dagExtending NameResult with name_id captures identity at token consumption time, which is more faithful than re-deriving names later from spans or strings.
MODELING — Improvements
src/v2/00_core.dagInternResult is a bespoke state-threading product; if interning is parser state, reuse the shared parser state/result carrier or add a generic State/Result form in std per MODELING.md M6/M9 instead of another ad hoc type.src/v2/02_parse.dagnode_to_name_str and exit-entry synthesis still model types as rendered names rather than structural edges; a more compositional model would keep direct Node references and interned keys until emit, aligning with MODELING.md M1/M8/M9.
ROADMAP — Unclaimed
- M4 Lane 2: Node.name deletion (D6): The diff clearly attempts the open D6 cutover across parse, resolve, compile, and emit, but ROADMAP.md was not updated to record that this Phase 2 work is in flight or that the identity boundary remains incomplete.
The PR fixes several mechanical review issues, but named-node construction and identity-preserving rewrites are still unsound, so empty-string and ident-0 collapses continue to leak through parse, resolve, ownership, and Rust emit.
| @@ -1285,12 +1299,12 @@ fn outputs_to_inferred(outputs: List<Node>, span: SourceSpan, source_index: Newl | |||
| // construct Node directly using these constructors. | |||
| // ========================================================================= | |||
|
|
|||
| fn make_operation_node(name: String, ident_span: SourceSpan?, inputs: List<Node>, outputs: List<Node>, response_props: List<Node>, mock_props: List<Node>, exit_props: List<Node>, modifier_props: List<Node>, transport: Node?, span: SourceSpan, source_index: NewlineIndex?) -> Node { | |||
| fn make_operation_node(ident_span: SourceSpan?, inputs: List<Node>, outputs: List<Node>, response_props: List<Node>, mock_props: List<Node>, exit_props: List<Node>, modifier_props: List<Node>, transport: Node?, span: SourceSpan, source_index: NewlineIndex?) -> Node { | |||
There was a problem hiding this comment.
Invariant violation: make_operation_node and make_capability_node have no ident parameter, so named service/resource members are constructed anonymous, violating INVARIANTS.md Engineering Standards / Clear interfaces and Single-authority metadata.
| if has_err(err: r.err) { return ItemResult { item: named_dummy, state: r.state, err: r.err } } | ||
| let s = skip_newlines(tokens: tokens, state: r.state) | ||
| let r2 = expect(tokens: tokens, state: s, expected: ExpectRBrace) | ||
| if has_err(err: r2.err) { return ItemResult { item: named_dummy, state: r2.state, err: r2.err } } | ||
| let type_children = r.fields |> map(f => field_to_child_node(field: f, source_index: state.source_index)) | ||
| let item = Node { name: name, span: start_span, ident_span: Some { value: name_span }, children: type_children, connective: Conj, params: type_params, inferred: none, return_cardinality: Required, uses: [], body: none, transport: none, properties: [], type_annotation: none, is_self_recursive: false, has_non_tail_self_call: false, match_pattern: none, expr_data: NoExprData } | ||
| let item = Node { span: start_span, ident_span: Some { value: name_span }, children: type_children, connective: Conj, params: type_params, inferred: none, return_cardinality: Required, uses: [], body: none, transport: none, properties: [], type_annotation: none, is_self_recursive: false, has_non_tail_self_call: false, match_pattern: none, expr_data: NoExprData } |
There was a problem hiding this comment.
Invariant violation: parse_type_after_kw builds declared type Nodes without the parsed name_id, and parse_block_item_after_kw drops name_id entirely, so named items lose identity at the parser boundary, violating INVARIANTS.md Single-authority metadata / Correctness by construction.
|
|
||
| fn with_optional_cardinality(n: Node) -> Node { | ||
| Node { name: n.name, span: n.span, ident_span: n.ident_span, children: n.children, connective: n.connective, params: n.params, inferred: n.inferred, return_cardinality: CardOptional, uses: n.uses, body: n.body, transport: n.transport, properties: n.properties, type_annotation: n.type_annotation, is_self_recursive: n.is_self_recursive, has_non_tail_self_call: n.has_non_tail_self_call, match_pattern: n.match_pattern, expr_data: n.expr_data } | ||
| Node { span: n.span, ident_span: n.ident_span, children: n.children, connective: n.connective, params: n.params, inferred: n.inferred, return_cardinality: CardOptional, uses: n.uses, body: n.body, transport: n.transport, properties: n.properties, type_annotation: n.type_annotation, is_self_recursive: n.is_self_recursive, has_non_tail_self_call: n.has_non_tail_self_call, match_pattern: n.match_pattern, expr_data: n.expr_data } |
There was a problem hiding this comment.
Invariant violation: with_optional_cardinality and with_required_cardinality rebuild Nodes without copying ident, so cardinality wrapping erases the only semantic identity authority, violating INVARIANTS.md No duplicate representations / Single-authority metadata.
| let substituted_args = child.children |> map(arg => | ||
| substitute_type_slots(n: arg, slot_bindings: slot_bindings, decl_name: decl_name, source_index: source_index) | ||
| ) | ||
| Node { |
There was a problem hiding this comment.
Invariant violation: substitute_type_slots reconstructs substituted child and parent Nodes without ident, so resolve-time rewriting destroys semantic identity instead of preserving it, violating INVARIANTS.md Correctness by construction / Single-authority metadata.
| @@ -2844,7 +2865,7 @@ fn parse_exit_entries_acc(tokens: List<Token>, state: ParserState, acc: List<Nod | |||
| let code_str = status_expr_to_str(expr: code, source_index: state.source_index) | |||
| let type_name = node_to_name_str(n: r3.type_expr) | |||
| let prop_name = concat("exit_", code_str) | |||
| let entry = make_field_init_node(name: prop_name, value: make_named_expr_node(name: type_name, expr_data: ExprVar { binding_kind: none }, children: [], inferred: none, span: r3.type_expr.span, name_span: r3.type_expr.span), span: r3.type_expr.span, name_span: no_span()) | |||
| let entry = make_field_init_node(ident: 0, value: make_named_expr_node(ident: 0, expr_data: ExprVar { binding_kind: none }, children: [], inferred: none, span: r3.type_expr.span, name_span: r3.type_expr.span), span: r3.type_expr.span, name_span: no_span()) | |||
There was a problem hiding this comment.
Invariant violation: parse_exit_entries_acc computes prop_name and type_name but emits an anonymous field init whose key and value ident are both 0, violating INVARIANTS.md No fallbacks that fabricate / No duplicate representations.
| typed.modules |> flat_map(m => | ||
| m.items |> filter(item => item.body != None) |> map(item => | ||
| analyze_ownership(func_name: item.name, params: item.params, body: item.body.value, si: m.type_env.source_index) | ||
| analyze_ownership(func_name: "", params: item.params, body: item.body.value, si: m.type_env.source_index) |
There was a problem hiding this comment.
Invariant violation: extract_ownership_proofs passes func_name as an empty string into analyze_ownership, collapsing proof identity and violating INVARIANTS.md Single-authority metadata / Correctness by construction.
| fn is_rust_value_type(n: Node) -> Bool { | ||
| let normed = normalize_access_type_node(n: n) | ||
| match is_copy(target: Rust, dag_name: normed.name) { | ||
| match is_copy(target: Rust, dag_name: "") { |
There was a problem hiding this comment.
Invariant violation: is_rust_value_type and is_rust_string_like query coercion/language tables with an empty type name, fabricating unknown-type decisions at the emit boundary and violating INVARIANTS.md No fallbacks that fabricate.
| let proofs = modules |> flat_map(m => | ||
| m.items |> filter(item => item.body != None) |> map(item => | ||
| OwnershipProofEntry { name: item.name, proof: analyze_ownership(func_name: item.name, params: item.params, body: item.body.value, si: m.type_env.source_index) } | ||
| OwnershipProofEntry { name: "", proof: analyze_ownership(func_name: "", params: item.params, body: item.body.value, si: m.type_env.source_index) } |
There was a problem hiding this comment.
Invariant violation: build_ownership_results indexes every OwnershipProofEntry under the empty string, so ownership and fold-eligibility maps overwrite one another, violating INVARIANTS.md No duplicate representations / Single-authority metadata.
| let mod_filename = module_to_filename(name: intern_str(table: tm.type_env.intern_table, id: tm.module.ident)) | ||
| svc_items |> fold(init: acc, f: (a, svc) => | ||
| map_insert(a, svc.name, mod_filename) | ||
| map_insert(a, "", mod_filename) |
There was a problem hiding this comment.
Invariant violation: svc_module_map inserts every service under the empty-string key, collapsing cross-module service import planning and violating INVARIANTS.md No duplicate representations / Single-authority metadata.
|
Review (INVARIANTS: 9, MODELING: 2+/2-, ROADMAP: 0✓/0!) INVARIANTS — Violations (9) ROOT CAUSE ANALYSIS
MODELING — Strengths
MODELING — Improvements
ROADMAP — Unclaimed
The PR fixes several mechanical review issues, but named-node construction and identity-preserving rewrites are still unsound, so empty-string and ident-0 collapses continue to leak through parse, resolve, ownership, and Rust emit. |
ChatGPT ReviewI reviewed the uploaded PR diff itself and used the attached invariants/modeling docs plus
The main issues I see:
Parsing now starts each file with its own
Helpers like
chatgpt-review-390128df-e8ed-4d… On compositional modeling quality for the There is some good direction here. Centralizing a few transport/config keys into shared constants is closer to the existing std-library pattern where one helper/data-table is the single authority and everyone reads from it. But the identity work is not compositional yet. Right now the PR creates overlapping authorities:
That is not one end-to-end fact. Existing std-library patterns work because producers and consumers agree on one authority; this diff introduces a new fact layer without making the producer/consumer contract canonical. So my overall assessment is:
I would not merge this as-is. The fix should be structural and upstream: either parse all sources against one authoritative intern table from the start, or add an explicit remap pass that rewrites every parsed Before merge, I’d want at least:
|
ChatGPT ReviewI reviewed the uploaded PR diff against INVARIANTS.md, MODELING.md, ROADMAP.md, and std-library.dag. I’d block this PR on two correctness issues. 1) Parser-local intern ids are merged globally without remapping existing nodesIn the diff, That means two files can both assign local id This is exactly the kind of boundary problem the invariants call out: a fact table must be an exact derivation and must land end-to-end as the authority, otherwise it is a lossy parallel representation. The fix belongs upstream at the producer boundary, not as another fallback in resolve or emit. chatgpt-review-43b953e2-5282-48… chatgpt-review-43b953e2-5282-48… The root-cause fix is one of:
Anything else is downstream compensation. 2)
|
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 3, MODELING: 2+/2-, ROADMAP: 0✓/1!)
INVARIANTS — Violations (3)
ROOT CAUSE ANALYSIS
src/v2/00_core.dagCardinality is an orthogonal wrapper fact, but core models it by reconstructing a fresh anonymousNodeinstead of structurally updating the same one, sonode_type_equalsandnode_type_compatiblecompare erased ids after optional unwrap and can collapse distinct optional types; fix this upstream with one identity-preserving cardinality-update helper and route every optional-type path through it (MODELING.md M1/M2).src/v2/02_parse.dagParse now interns token-level names but never lifts that identity through composed-name parsing or a single named-item constructor, soparsed_name_leafand the prefix-driven declaration builders keep falling back toident: 0;resolve_import,build_type_env, and service/item registries then callintern_stron anonymous ids and collapse distinct names to""; fix this at the parse boundary by interning composed dotted names once and routing every authored declaration through amake_named_item_node(name, ident, ...)constructor that requiresname_id(MODELING.md M2/M5/M6).
MODELING — Strengths
src/v2/00_core.dagInternTablelives at the compiler-core identity boundary, which is the right layer for M4-L2 and more faithful than late source-text recovery.src/v2/02_parse.dagAddingname_idtoItemPrefixResultputs declaration identity at the parse boundary where the authored name is first observed.
MODELING — Improvements
src/v2/02_parse.dagNameResult,ParseResult, and the other parser records still duplicate the same value+state+err/state-thread shape; per MODELING.md M6/A1 this should collapse into one genericParseResult<T>.src/v2/00_core.dagInternTableis a bijection/partial-function pair in disguise, but it still coexists withNode.nameas a second authority; the more compositional M9 end-state is to ground identity in thePartialFunction/Map vocabulary ofdsl/std/algebra.dagand delete the dual-write.
ROADMAP — Incomplete
- M4 Lane 2: Node.name deletion: The diff advances identity plumbing but does not satisfy the lane's open work because
authored_name_atstill treatsNode.nameas authority and many named parser nodes still carryident: 0.
ROADMAP — Unclaimed
- M4 Lane 2: Node.name deletion: The PR clearly advances D6 wiring through parse, resolve, typecheck, and emit, but ROADMAP.md was not updated to record that incremental progress or revise the remaining debt.
The PR moves M4-L2 plumbing forward, but parser-side identity still collapses many authored names to ident: 0 and the new cardinality helpers erase existing identity, so the intern-based path is not yet safe to merge.
|
|
||
| fn with_optional_cardinality(n: Node) -> Node { | ||
| Node { name: n.name, span: n.span, ident_span: n.ident_span, children: n.children, connective: n.connective, params: n.params, inferred: n.inferred, return_cardinality: CardOptional, uses: n.uses, body: n.body, transport: n.transport, properties: n.properties, type_annotation: n.type_annotation, is_self_recursive: n.is_self_recursive, has_non_tail_self_call: n.has_non_tail_self_call, match_pattern: n.match_pattern, expr_data: n.expr_data } | ||
| Node { name: n.name, ident: 0, span: n.span, ident_span: n.ident_span, children: n.children, connective: n.connective, params: n.params, inferred: n.inferred, return_cardinality: CardOptional, uses: n.uses, body: n.body, transport: n.transport, properties: n.properties, type_annotation: n.type_annotation, is_self_recursive: n.is_self_recursive, has_non_tail_self_call: n.has_non_tail_self_call, match_pattern: n.match_pattern, expr_data: n.expr_data } |
There was a problem hiding this comment.
Invariant violation: with_optional_cardinality and with_required_cardinality overwrite an existing Node.ident with 0, so optional-type unwraps erase semantic identity and violate INVARIANTS.md Single-authority metadata and Correctness by construction.
| @@ -1104,7 +1117,7 @@ fn parse_import_names(tokens: List<Token>, state: ParserState) -> NamesResult { | |||
| // or distinguishing parsed names from synthetic ones), only this | |||
| // function changes — not every lambda/import call site. | |||
| fn parsed_name_leaf(name: String, span: SourceSpan) -> Node { | |||
| leaf_node_with_span(name: name, span: span) | |||
| leaf_node_with_span(name: name, ident: 0, span: span) | |||
There was a problem hiding this comment.
Invariant violation: parsed_name_leaf still hardcodes ident: 0 for specific import names, so the new intern-based export validation path can only see fabricated empty names, violating INVARIANTS.md No fallbacks that fabricate and Single-authority metadata.
| @@ -1384,46 +1400,47 @@ fn parse_type_body_from_prefix(tokens: List<Token>, prefix: ItemPrefixResult, st | |||
| let r2 = expect(tokens: tokens, state: s, expected: ExpectRBrace) | |||
| if has_err(err: r2.err) { return ItemResult { item: named_dummy, state: r2.state, err: r2.err } } | |||
| let type_children = r.fields |> map(f => field_to_child_node(field: f, source_index: prefix.state.source_index)) | |||
| let item = Node { name: name, span: start_span, ident_span: Some { value: name_span }, children: type_children, connective: Conj, params: type_params, inferred: none, return_cardinality: Required, uses: [], body: none, transport: none, properties: [], type_annotation: none, is_self_recursive: false, has_non_tail_self_call: false, match_pattern: none, expr_data: NoExprData } | |||
| let item = Node { name: name, ident: 0, span: start_span, ident_span: Some { value: name_span }, children: type_children, connective: Conj, params: type_params, inferred: none, return_cardinality: Required, uses: [], body: none, transport: none, properties: [], type_annotation: none, is_self_recursive: false, has_non_tail_self_call: false, match_pattern: none, expr_data: NoExprData } | |||
There was a problem hiding this comment.
Invariant violation: parse_type_body_from_prefix captures prefix.name_id but still constructs the named item with ident: 0, and the same drop happens in other named item builders, violating INVARIANTS.md Single-authority metadata and Engineering Standards / Clear interfaces.
|
Review (INVARIANTS: 3, MODELING: 2+/2-, ROADMAP: 0✓/1!) INVARIANTS — Violations (3) ROOT CAUSE ANALYSIS
MODELING — Strengths
MODELING — Improvements
ROADMAP — Incomplete
ROADMAP — Unclaimed
The PR moves M4-L2 plumbing forward, but parser-side identity still collapses many authored names to |
InternTable provides a parser-owned identifier authority that maps strings to sequential integer IDs. Phase 1 adds the type, constructor, lookup functions in 00_core.dag, wires intern_table through ParserState and ParseResult in 02_parse.dag, and merges tables in compile.dag. No Node schema changes — Phase 2 (Node.ident + signature threading) follows after this merges. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
ChatGPT ReviewI reviewed the uploaded PR diff against INVARIANTS.md, MODELING.md, ROADMAP.md, and std-library.dag. I’d block this PR on two correctness issues. 1) Parser-local intern ids are merged globally without remapping existing nodesIn the diff, That means two files can both assign local id This is exactly the kind of boundary problem the invariants call out: a fact table must be an exact derivation and must land end-to-end as the authority, otherwise it is a lossy parallel representation. The fix belongs upstream at the producer boundary, not as another fallback in resolve or emit. chatgpt-review-43b953e2-5282-48… chatgpt-review-43b953e2-5282-48… The root-cause fix is one of:
Anything else is downstream compensation. 2)
|
briansrls
left a comment
briansrls
left a comment
There was a problem hiding this comment.
Review (INVARIANTS: 2, MODELING: 2+/2-, ROADMAP: 1✓/1!)
INVARIANTS — Violations (2)
ROOT CAUSE ANALYSIS
src/v2/00_core.dagThe upstream gap is that M4-L2 still has no single IR-level identifier authority: Node construction still treats authored text as the real identity, so 00_core compensates with an ad hoc interner and 02_parse dual-writes the same fact; that duplication then propagates downstream into merge-only tables and stale-ID hazards. Fix it at the source by choosing one authority at construction time, either ident_span plus source_text_at only or a first-class Identifier or Node.ident with an explicit absent case, and if an interner remains model it once as a grounded partial-bijection or state abstraction rather than parallel fields and bespoke carriers (M2, M6, M7, M9).
MODELING — Strengths
src/v2/02_parse.dagCarrying intern_table on ParserState keeps the new state explicit and pure, which composes with the parser’s existing state-threading style.src/v2/compile.dagmerge_intern_tables is expressed as nested folds over finite tables, so the aggregation itself stays within the bounded iteration model from std/iteration.dag.
MODELING — Improvements
src/v2/00_core.dagInternTable is a bidirectional partial-function concept in disguise, so a more compositional model would ground it once in std as a single-authority symbol or bimap abstraction and derive size or reverse lookup instead of storing three authorities side by side.src/v2/00_core.dagInternResult is another bespoke state-carrier result type, and M6 or M9 points toward a shared State or Result pattern for parser and interning flows rather than growing the result family in core.
ROADMAP — Verified
- M4 Lane 2 dual-write interning scaffold: The diff does add parser-owned interning state, returns it from ParseResult, and threads a merged table through FrontendResult without any current compilation consumer.
ROADMAP — Incomplete
- M4 Lane 2 Node.name deletion: The current diff does not deliver the claimed n.name to authored_name_at migration or parser-wide identifier interning via expect_ident or expect_name; it only interns the module name in parse_module.
ROADMAP — Unclaimed
- M4 Lane 2 intermediate interner phase: ROADMAP.md does not list this new interning side-table step or its exit criteria before Node.name deletion, so the milestone direction changed without a roadmap update.
This revision drops many previously flagged regressions out of the diff, but the remaining interner scaffold still duplicates naming authority at the parser and core boundary and does not yet substantiate the PR’s claimed M4-L2 progress.
|
|
||
| type InternTable { | ||
| strings: List<String> | ||
| index: Map<String, Int> |
There was a problem hiding this comment.
Invariant violation: InternTable stores the same identifier relation in parallel as strings, index, and next_id, violating INVARIANTS.md No duplicate representations and Single-authority metadata.
| let s = r.state | ||
|
|
||
| // Intern the module name for future identity lookup | ||
| let mod_ir = intern(table: s.intern_table, s: mod_name) |
There was a problem hiding this comment.
Invariant violation: parse_module interns mod_name into ParserState.intern_table and then also writes the same identity into module_node(name: mod_name, ...), violating INVARIANTS.md No duplicate representations and Single-authority metadata.
|
Review (INVARIANTS: 2, MODELING: 2+/2-, ROADMAP: 1✓/1!) INVARIANTS — Violations (2) ROOT CAUSE ANALYSIS
MODELING — Strengths
MODELING — Improvements
ROADMAP — Verified
ROADMAP — Incomplete
ROADMAP — Unclaimed
This revision drops many previously flagged regressions out of the diff, but the remaining interner scaffold still duplicates naming authority at the parser and core boundary and does not yet substantiate the PR’s claimed M4-L2 progress. |
The emitter generates Rust use-statements from explicit .dag import lists, not usage analysis. Add InternTable, InternResult, empty_intern_table, and intern to the import blocks in 02_parse.dag and compile.dag so the freshness check passes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
ChatGPT review in progress... (view conversation) |
1 participant
Summary
Node.namereads toauthored_name_atacross 7 logic-layer files (04_env, 04_resolve, 04_types, 04_access, 04_service, 04_lookup, 04_infer)source_indexthrough03_resolve.dag(~22 module-level reads) and03_normalize.dag(2 reads)InternTabletype +intern/intern_str/intern_findfunctions to00_core.dagexpect_ident/expect_name(dual-write —Node.namestill set)ParseResult/FrontendResultcarry merged intern table through pipelineTest plan
authored_name_atfalls back tonode.name, so zero behavioral changes🤖 Generated with Claude Code