Unify compiler architecture and harden semantic contracts - #132
Merged
Conversation
Keep parsed call syntax immutable while publishing generated default arguments as typechecker evidence. Split staged bindings and typechecker facts into explicit generation-owned results, remove SemanticInfo, and simplify interface and match evidence without semantic rediscovery. Preserve reset, diagnostics, flow, ownership, lowering, LSP, and fingerprint behavior. Result constructors initialize required phase maps; BaseExprType, ExpandedDefaultBinding, MatchCases, CallArgumentsOrSource, and RebuildTypedASTIndex centralize cross-phase invariants.
Replace path-based Module.Key and semantic DefiningModuleKey with one comparable moduleid.ID of origin, namespace, dependency, and import path. Identity is now logical rather than positional: it survives filesystem relocation, file path is a secondary index, and graph and diagnostics boundaries consume the length-framed ID.String() encoding so component delimiters cannot collide. ModuleKeyFor, ModuleByKey, and loader-side identity backfill are deleted; NewModuleForFile and prelude.ModuleID are the only identity derivations. ID.Valid is the single identity predicate and requires both origin and import path. That invariant keeps ctx.modules keys distinct, since an identity carrying only an origin would collapse every local module onto one entry. No IsZero counterpart exists: a partially populated identity is invalid, not empty. Split mixed Module.ConstValues into constantresult.Result, which keeps authoritative post-typecheck ModuleValues physically separate from the mutable evaluator QueryCache. Foreign constants resolve through the defining module and are read from the owner rather than copied into the consumer, and fingerprints and MIR consume only authoritative values. Both slices land together because they interleave in project/modules.go and pipeline/pipeline.go; splitting them by hunk would produce intermediate commits that do not compile. Preserve diagnostics, import resolution, symbol provenance, reset lifetime, fingerprints, and LSP behavior. Validated with gofmt, go vet, go test -count=1 ./..., focused race suites, a fresh compiler bundle, and the bundled-binary x_test suite.
Importing the prelude explicitly crashed the compiler. prelude.ModuleID hardcoded import path "prelude/global", but the prelude file resolves to <coreRoot>/src/global.peep, whose canonical ImportPathForFile result is "global". An explicit `import "core:global"` therefore reached AddModule with the same file under a second identity. ModuleID now takes a context and derives the import path from the resolved prelude file, so the auto-loaded prelude and an explicit import agree. The hardcoded mismatch predates canonical module identity; it became a crash because AddModule gained a one-identity-per-file invariant. That conflict is reachable from user source and from two library roots sharing a directory, so AddModule now emits ErrAmbiguousImport and keeps the first registration instead of panicking. Identity conflicts are user-facing errors, not impossible states, and in the LSP the panic killed the server process for the whole session. AddModule releases the registry lock before reporting so the diagnostic bag is never taken under it. No x_test fixture used a core: import, which is why the full suite stayed green. Add import_prelude_identity covering an explicit prelude import, and a direct AddModule conflict-diagnostic test. Validated with gofmt, go vet, go test -count=1 ./..., focused race suites, a fresh compiler bundle, and the bundled-binary x_test suite. The failing input exits 0 on origin/main, panicked on the previous commit, and exits 0 again after this change.
An exported function default that references an imported constant lost the constant's value from its semantic export fingerprint. Foreign constants are published only in the owning module, by design, so reading the consumer's ModuleValues yielded nothing and the fact degraded to an empty value. The exported surface then stopped changing when the imported constant changed. Add CompilerContext.PublishedConstant, which resolves a constant symbol through its defining identity and returns the owner's published value. It returns the value alone: constant evaluation never publishes nil, so nil is the absent case and a separate found flag would be redundant. Query cache entries stay excluded, since only published values are stable enough for cross-module reads. Constant evaluation now calls it for its foreign branch instead of repeating the owner lookup, so one join serves both. SemanticExportFingerprint takes a context because resolving imported values requires the module registry; a nil context still reads local values so callers without a registry keep working. Validated with gofmt, go vet, go test -count=1 ./..., focused race suites, a fresh compiler bundle, and the bundled-binary x_test suite. The added fingerprint test fails against the previous consumer-only lookup.
Global symbols are injected into every module's scope, so naming the prelude in an import list or reaching a global symbol through `global::` adds nothing. Both are style observations, not defects, so they report as info (S prefix) and leave compilation unchanged. S0004 reports an explicit prelude import at the import declaration. It is emitted where imports resolve, by comparing the resolved identity against prelude.ModuleID, so it cannot fire for an ordinary library. S0005 reports a redundant qualifier by comparing the resolved symbol against the same symbol in global scope. Keying it on symbol identity rather than name keeps a same-named unrelated export quiet, and needs no prelude dependency in the resolver: the note states exactly what is true, that this symbol is already in scope. It also fires alongside the existing unexported-symbol error, where dropping the qualifier is the actual fix rather than exporting the symbol. Every prelude symbol is currently lowercase, so a qualified global symbol cannot both resolve and be exported today. Cover the valid case with a pipeline test that builds a real temporary library root containing an exported global, plus a companion test proving an ordinary library qualifier stays quiet. Add source fixtures for the explicit import and for the qualified private symbol. Validated with gofmt, go vet, go test -count=1 ./..., focused race suites, a fresh compiler bundle, and the bundled-binary x_test suite.
Structural traversal prevents a forgotten child walk. Nothing prevented a forgotten semantic decision: adding a node kind and never teaching a phase about it. These tests parse the real sources, enumerate every type implementing stmtNode and exprNode, and require each dispatch site to either handle a kind or record a decision about it. This is the weakest of the mechanisms the roadmap permits. It lists compile-time visitor interfaces first, mechanically checked dispatch tables second, and completeness tests third, and warns only against visitor base types carrying no-op defaults. A test reports an omission on the next test run; an unsatisfied interface fails the build. Later work should promote this to the compile tier and keep these tests as the supplement the roadmap intends. Scope is the first experiment only: source AST statement and expression kinds, across manually listed dispatch functions. Declarations, type syntax, HIR, MIR and backend families are not covered. Module-level declaration handling is partitioned across separate binding, function and type-declaration passes per phase, so its contract needs a different shape than a flat per-kind table. Widening the scan past stmt.go was necessary: every declaration also implements stmtNode, so ast.Stmt has nineteen implementors rather than ten, and a stmt.go-only scan silently checked a subset. Test-only, no production code changes. Verified by adding a synthetic statement kind and a synthetic expression kind, each reported by every relevant site, with the tree restored afterward.
Two defects shared one root: the registry mutated its file index before validating the registration it was about to reject. AddModule deleted the previous file-index entry for an identity whose path changed, then checked whether the new path already belonged to another identity. A rejected registration therefore left the module reachable by ID but no longer by file. Under the earlier panic the process died and the inconsistency was unobservable; making the conflict recoverable exposed it. Both directions are now validated before any index changes. An identity claiming a second file was also silently relocated rather than diagnosed. Extensions compare case-insensitively while the import path keeps the file's own case, so foo.peep and foo.PEEP reduce to one logical identity while remaining distinct files on a case-sensitive filesystem. Import resolution reused whichever module loaded first, attaching graph edges, diagnostics, constants and symbols to the wrong file. Registration and import resolution now report ambiguity instead of choosing arbitrarily. Add coverage for the relocation-conflict path, which previous tests missed because they only exercised two identities sharing one file.
Artifact paths were built from origin and import path only, so identities differing solely by namespace or dependency wrote the same files and the survivor depended on map iteration order. Core json and vendor json both produced _gen/core/json.ll. Namespace and dependency now occupy their own segments. Empty components render as a placeholder rather than collapsing, because dropping them would reintroduce the same ambiguity from the other direction: namespace "a" with import path "b/c" would otherwise share a path with no namespace and import path "a/b/c". Paths are only written into a staging tree and never parsed back, so the shape change is contained.
Instruction emission was a chain of type assertions with no final branch, so an unrecognized instruction emitted nothing and silently dropped program behavior. The terminator switch had no default either, which is worse: the block was written without a terminator, producing malformed LLVM IR rather than a missing operation. Both now classify every node and panic on an unhandled one. Reaching either default means MIR carried a node the backend never learned to emit, which is a compiler bug rather than invalid source, so an immediate internal failure is the correct policy. Note that mir.Instr and mir.Terminator are structural interfaces with no marker method, so no closed set exists to check mechanically. These defaults are the available guard until those families are sealed.
The shared declaration reason was factually wrong. parseStmt really does build FnDecl, StructDecl, InterfaceDecl, EnumDecl and TypeAliasDecl nodes inside a block; the resolver reports them as unsupported statements and HIR lowers them to hir.Invalid. CFG through ownership are not error-gated, so those nodes do reach the sites that skip them. They are skipped because a declaration evaluates no expression at a CFG site, not because the parser refuses them. The earlier reason was asserted from an observed P0004 without checking which phase emitted it. Adopt the roadmap's four-way vocabulary. Kinds without a case are now classified traverse, ignore or reject with a reason, rather than carrying an unlabelled excuse. Whether a case body handles or rejects is still not distinguishable from source shape; separating those needs the compile-time visitor interfaces the roadmap prefers. Verify the else-position reason before reusing it: parseIfStmt produces only a block or an else-if, so lowerElse rejecting anything else is sound. Delete declaredStatementKinds and declaredExpressionKinds, which forwarded to declaredKinds without adding behavior.
Child traversal is handwritten, and interface satisfaction cannot see inside a valid method. Adding a node-bearing field and forgetting to visit it compiled, passed the node-dispatch contract, and silently hid the field from every ast.Inspect consumer. This was the remaining gap the framework direction names explicitly. Two checks read the AST package. The first requires each forEachChild to mention every node-bearing field of its own type. The second covers sub-structures such as Param and StructLitField, which carry nodes but have no traversal of their own because a helper or inline loop expands them; every node-bearing field of those must appear in some traversal context. Node-bearing is transitive, so a field typed []Param counts even though Param is not itself a node. A composite field the classifier cannot judge is reported rather than assumed inert, so an unfamiliar shape fails loudly instead of being skipped. Sub-structures held by no node, such as the Module root aggregate, are excluded because consumers walk them directly rather than through ast.Inspect. Expansion evidence is matched by field name across traversal contexts, so a field sharing a name with a traversed field of another type can read as covered. Removing that needs go/types resolution. The check still catches a newly added field whose name appears in no traversal at all. Verified against both shapes: adding ForStmt.Else without updating forEachChild, and adding Param.Constraint with no expansion. Each is reported with the tree restored afterward.
Identity was assembled separately in module construction, import resolution and the prelude, each pairing an origin and namespace with a derived import path. The parts stayed similar by convention only, which is how the prelude came to register under an import path no import could reproduce. CompilerContext.IdentityForFile now owns that assembly and every producer calls it. It is not a forwarding wrapper: it couples the origin and namespace a caller chose with the import path derived from the file, and returns an error when no identity exists. Exactly one moduleid.ID literal remains in production code, inside that function.
Attribute arguments are real AST expressions but were classified as inert metadata, so no forEachChild reached them and the contract could not see the gap. Attribute and Attributed now own traversals, and the five declarations embedding Attributed chain into them, making attribute args reachable through ast.Inspect and ast.Index. The child-traversal contract is rewritten with owner-qualified coverage: sub-structure fields must be expanded by a traversal context that handles that specific sub-structure (helpers found structurally by their visit parameter, liveness required), and embedded node-bearing composites must own a traversal and be chained by type name. The dispatch contract now only counts type switches whose operand is the function's statement or expression input, so unrelated nested switches cannot fake coverage; the ast package alias is resolved from file imports; ImportDecl/BadDecl reasons state their real reachability; RangeExpr gains a contextual decision; and both contracts share one AST package loader. Proven by mutation: adding an unvisited ForStmt field fails the traversal contract, and removing a dispatch case fails the phase-decision contract.
Two same-identity modules loading from different files could race past the registry conflict check: the loader deduplicated by ID before either reached AddModule, so the losing file was dropped without a diagnostic and the winner depended on scheduling. The scheduled map now records the queued file path; a dedupe hit with a different non-empty path routes through AddModule so the registry emits its ambiguous-import diagnostic, and resolveImports does the same for an already-registered module whose file differs. Conflict policy stays centralized in the registry. AddModule also clears the stale file index when a same-ID registration replaces a file-backed module with a pathless one, keeping the ID and file indexes consistent in both directions.
The dump path encoding was lossy: empty and "_" namespaces collided, invalid components silently became "_", and ":" collapsed to "/" so import paths a:b and a/b wrote the same artifacts. Components are now encoded with one injective, path-safe scheme: a safe byte set passes through, everything else is percent-encoded, leading dots are escaped so no dot-segment survives, and a literal "_" is escaped to reserve the empty-component marker. Distinct canonical identities can no longer share an artifact path and no component can escape the stage directory.
Unknown MIR instructions and terminators previously reached LLVM emission, where unrecognized instruction kinds were silently skipped and terminator kinds fell through without emitting a terminator, producing malformed IR instead of failing. The emitter now panics on unclassified nodes; regression tests pin both failure paths with a test-only instruction/terminator implementation.
The determinism check compared first.String() with itself on the same receiver, which can never fail for a pure function and tested nothing. The collision check stays as the real property.
Ownership capability was scattered across three predicates with no single consume point, and several types had undeclared character: none was move-on-use, FuncType and TypeParameterType were ambiguous, and the backend re-derived drop obligations from the IR table. Add OwnershipCapabilityOf as the classification new code consumes: a copy class (implicit / explicit / never) plus a drop flag, composed from the established predicates so existing behavior is exact. NoneType joins the implicit-copy set per the structural rule (it contains nothing). IsNoCopyType is unexported as the CopyNever half of the derivation, and its cycle guard now pops on exit like the other walkers. The backend drop walker stays for now: it drives structural per-field drop decomposition over the IR table, which is a lowering concern rather than a policy re-derivation; its drift risk is covered by the end-to-end drop fixtures. The design record and maintainer decisions D1-D7 live in docs/compiler-framework/ownership-vocabulary.md.
Captures why the framework work started (the for-loop exposing that adding one construct required coordinating scattered phase logic), the executable-safety goal (adding a node or field should mechanically expose every phase that must change), the workstreams, before/after examples, and the anti-goals that keep the framework from becoming abstraction tax.
Ownership re-derived every consumption decision from AST shapes and hardcoded per-node rules: call arguments were matched against parameter types again, binding and return initializers were hardcoded consumes, match arms re-derived carrier moves from binding types, and alloc special-cased its first argument. The typechecker had already made each of those decisions while checking the call, then threw the answer away. The typechecker now publishes the classification: UseKind (read, copy, move) lives in typeinfo beside OwnershipCapability as the per-use counterpart of the per-type capability, and typecheckresult carries ValueUses keyed by the used expression plus CarrierUse on each match arm. Ownership consumes the published kinds through publishedUse, which falls back to the capability derivation for diagnostics-continued paths where publication is incomplete; the ownership validator will enforce presence for error-free programs. The deleted re-derivations: per-argument capability matching, matchArmMovesCarrier (both call sites), and the alloc argument hardcode. Behavior is preserved end to end; new publication regressions cover copyable, owned, and reference parameters, alloc operands, and match carrier use, and the use-after-move fixture still fails through the published evidence.
The registry detected identity conflicts but reported them with no location, so the reader got a message with nothing to click and editors had no line to attach it to. Import resolution previously labelled the offending import. AddModule now returns the diagnostic it recorded, following the AddError chaining pattern, and import resolution labels it with the import site. The registry keeps sole ownership of the conflict policy; only the caller knows which source site triggered the registration.
ModuleForFile returned success with a zero module identity when identity derivation failed. Every downstream path discards a module with an invalid identity, so callers ran the pipeline on a module that could never register and produced no diagnostic. NewModuleForFile already fails this case by returning nil. ModuleForFile now reports failure, and Load surfaces it as an error instead of dropping the prelude silently.
The unclassified-terminator panic sat inside a nil guard, so it caught unknown terminator kinds only. A block reaching emission with no terminator skipped the switch entirely and emitted an unterminated basic block with no compiler-side signal, which is what the guard's own comment claimed to prevent. An unterminated block is an impossible MIR state: lowerCFGTerminator is exhaustive and panics on unhandled CFG terminators, so it never publishes one. Emission now panics on it, naming the block.
The doc comment kept its opening line from the version that translated between two use-kind vocabularies. That translation is gone, so the line described behavior the function no longer has.
Ownership reads the published use kind for alloc arguments instead of deriving them, and its fallback for a missing entry is a read. Publication happened after the operand type was resolved, so an operand producing no type exited first and the consuming use was lost: the operand stayed live where it used to be moved. The use kinds come from the intrinsic's own semantics and need no types, so they are published right after the arity gate, which already guarantees the argument count they index.
hir.Return.Cleanup and hir.Assign.DropTarget were drop channels nothing produced. No production code ever wrote either one, so folding and MIR lowering carried them, MIR ORed the dead assign flag with the plan, and two tests asserted only that the empty channels survived a fold. Both are deleted, and MIR reads CleanupPlan.BeforeAssign alone. The drops that remain are not competing policy: a source-level free is the programmer's own drop, and MIR temporary drops destroy temporaries MIR materializes, which have no source symbol to plan against. Return and scope exit stay separate channels, now documented with the reason they cannot merge as lowering stands: site drops emit before the terminator runs, while a return must compute its value before unwinding the scopes it leaves. A new regression pins that ordering through the plan, replacing the test that only proved the dead channel was carried.
Ownership published a cleanup plan and consumed use kinds with nothing checking that the two agreed with the artifacts they describe. A stale plan key, a use kind on an untyped node, or a call argument the typechecker never classified all stayed silent, and the ownership analyzer's capability fallback quietly absorbed the last of those. ownershipresult.Validate now checks plan and evidence shape: every plan key names a real CFG site, typed expression, or block scope; every published use kind belongs to a typed expression and is legal for that type's capability; and every argument of a resolved call carries a classification. The pipeline runs it after each ownership pass and reports failure as ICE0002, skipped when the module already has errors so broken source never reports a compiler bug. Reports are sorted and truncated, because plans are maps and an unsorted report would name different problems on different runs. The analyzer's fallback stays: it is reachable only where the typechecker exited before publishing, which means the source is already diagnosed. Per-path single-drop checking stays out, because that is the dataflow ownership already performs and a validator must not re-derive it. Both choices are recorded in the design doc.
CleanupPlan.MatchCarrierMoves was write-only. Ownership recorded a moved match carrier there, but MIR lowering reads the other seven plan fields and never this one, so no drop or suppression depended on it. The effect it appeared to describe is already produced by removing the carrier from the live set two lines above the write. It was the third dead drop channel, after hir.Return.Cleanup and hir.Assign.DropTarget. Keeping it was worse than inert: the phase-boundary validator checked it, which presented dead evidence to the next reader as though lowering depended on it. The two ownership tests that read the map as a proxy now assert the observable instead: leaving the arm that consumes the carrier must not drop it, and leaving the arm that does not must. Removing the live-set deletion fails those assertions, which the map assertion did not require.
Section 4 listed "delete backend typeNeedsDrop" as part of slice 1. The walker is still at backend/llvm/drop_emit.go:303 with ten call sites, so G2, G3 and decision D2 remain open, and the doc read as though owned interface drop policy had already moved into the source language. Section 2.1 claimed every capability query derives from OwnershipCapabilityOf. The dependency runs the other way: the capability composes NeedsDrop, IsImplicitCopyType and noCopyType, which keeps the consolidation behavior-preserving but leaves the single-walker half of D6 outstanding. Both sections now carry an "As implemented" note, matching the one section 3 already carries for slice 4, and the status line no longer calls a document whose decisions are settled a proposal.
The module-key concept was deleted when moduleid.ID became the canonical identity, but internal/diagnostics kept naming its grouping parameter and field moduleKey. It now receives moduleid.ID.String(), so the old name pointed at a type that no longer exists. Rename to moduleScope throughout the bag and its one LSP caller, and record on the field why the package takes an opaque string: diagnostics must not depend on how module identity is spelled, and only ever compares and sorts the value. Pure rename; no behavior change.
Backport the small set of post-1.23 convenience APIs used by tests, CLI, LSP, project loading, and registry code without changing compiler behavior.
Evaluate nested indexes and slice operands once in source order across reads, writes, and borrows. Preserve assignment RHS ordering and temporary projection identities. Share placeOperands to centralize evaluation-order invariants across consumers, as permitted by the shared domain logic helper rule. Add producer and executable source regressions.
Validate complete CFG adjacency and exact site edges, reject incorrect expression identities, and preserve invalid-source recovery. Expose a detached complete edge snapshot for validator consumers. Restore facade-only empty-ID filtering without rejecting generic zero nodes. Share normalization between graph algorithms to preserve caller input. Benchmark insertion and retain current storage pending production profiling.
Reuse recorded operand types so assignment checks retain flow payload evidence and correct HIR projections. Centralize existing type lookup instead of replaying expression typing. Track holder-relative loan paths and replace only overwritten field loans, preserving siblings, copied holders, and partial-write liveness. The field replacement helper protects slot identity and reuses resolved storage evidence. Check mutation against carrier storage while preserving typed backend invariants. Add state, semantic, HIR, and executable positive/negative regressions for replacement, copies, optional clearing, mutable references, joins, and loops.
Return directly from required leaf traversal methods and remove the redundant semantic Type assertion. Retain typed-nil reflection behavior with explicit regression coverage; preserve recursive fingerprints and ownership capabilities.
Document bounded ownership provenance, lexical usage, effect validation limits, CFG index lifecycle, and measured performance tradeoffs. Mark the original task historical without erasing its contents. Validation: full Go tests, vet, focused race tests, bundle, and full executable fixtures passed sequentially. Go baseline remains 1.23.2; validation used installed go1.26.7-X:nodwarf5.
itsfuad
added
documentation
There was a problem hiding this comment.
🟡 Changes recommended
New HIR/MIR validators can still panic on typed-nil interface values, undermining their goal of reporting malformed artifacts deterministically.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Consolidates compiler “kernel” mechanisms (structural traversal, graph/worklist adjacency, place projection grammar, module identity, and published semantic evidence) while preserving explicit phase boundaries, plus adds/updates fixtures and validators to harden semantic contracts and recovery guarantees.
Changes:
- Introduce canonical kernels:
moduleid.ID,graph.Directedadjacency,graph.Worklist,place.Project/Decompose,typeinfo.ForEachChild, and exhaustiveeffect.Visitor. - Move/rename semantic evidence stores (bindings/typechecking) and update pipeline/LSP/import/export fingerprinting to use canonical module identity.
- Add artifact validators and extensive
x_testfixtures for operand evaluation order, cleanup paths, and reference-field rebind behavior.
File summaries
| File | Description |
|---|---|
| x_test/runtime_variant_reference_rebind/src/main.peep | Runtime fixture: ref-field rebind |
| x_test/runtime_variant_reference_rebind/peeper.toml | Fixture config |
| x_test/runtime_projection_operand_order/src/main.peep | Runtime fixture: projection eval order |
| x_test/runtime_projection_operand_order/peeper.toml | Fixture config |
| x_test/runtime_cleanup_paths/src/main.peep | Runtime fixture: cleanup execution |
| x_test/runtime_cleanup_paths/peeper.toml | Fixture stdout expectations |
| x_test/negative_variant_reference_rebind/src/main.peep | Negative borrow/rebind cases |
| x_test/negative_variant_reference_rebind/peeper.toml | Negative diagnostic expectations |
| x_test/negative_projection_uninitialized/src/main.peep | Negative uninitialized index uses |
| x_test/negative_projection_uninitialized/peeper.toml | Diagnostic expectations |
| x_test/negative_projection_borrow_conflict/src/main.peep | Negative borrow/mut conflict in index |
| x_test/negative_projection_borrow_conflict/peeper.toml | Diagnostic expectations |
| x_test/negative_global_qualifier/src/main.peep | Negative redundant qualifier case |
| x_test/negative_global_qualifier/peeper.toml | Diagnostic expectations |
| x_test/negative_default_parameter_arity/peeper.toml | Stabilize failure output excludes |
| x_test/import_prelude_identity/src/main.peep | Prelude identity fixture |
| x_test/import_prelude_identity/peeper.toml | Fixture config |
| x_test/import_default_parameters/src/main.peep | Add nested default call coverage |
| x_test/import_default_parameters/src/external.peep | Nested default parameter definitions |
| x_test/fixtures_test.go | Fix exit-code extraction with errors.As |
| README.md | Add “learning compiler” links |
| pkg/registry/download.go | Replace SplitSeq usage |
| internal/semantics/usage/usage.go | Prelude-aware module ID check; bindings rename |
| internal/semantics/usage/usage_test.go | Update module construction to moduleid.ID |
| internal/semantics/typeinfo/types.go | Seal Type with child + ownership methods |
| internal/semantics/typeinfo/types_test.go | Update tests to OwnershipCapabilityOf; ReturnOriginSources args |
| internal/semantics/typeinfo/syntax.go | ReturnOriginSources takes explicit args slice |
| internal/semantics/typeinfo/structure.go | Canonical semantic type child traversal |
| internal/semantics/typeinfo/structure_test.go | Coverage for structure + typed-nil behavior |
| internal/semantics/typeinfo/relations.go | Use ForEachChild for containment walkers |
| internal/semantics/typeinfo/capability_walk_test.go | Golden capability matrix regression |
| internal/semantics/typechecker/typechecker.go | Initialize module.Typechecking; use bindings methods |
| internal/semantics/typechecker/for_in_test.go | Evidence reads via typecheckresult plans |
| internal/semantics/typechecker/flow_test.go | moduleid.ID; rebuild typed AST index |
| internal/semantics/typechecker/check_fn.go | Bindings-backed symbol lookup; copy capability query |
| internal/semantics/typechecker/assignability.go | Store typechecking evidence; bindings method sets |
| internal/semantics/symbols/symbol.go | Symbol.Type uses typeinfo.Type; DefiningModule is moduleid.ID |
| internal/semantics/resolver/resolver.go | Create bindingresult; global qualifier info diagnostic |
| internal/semantics/resolver/resolver_test.go | Update to bindings + moduleid.ID |
| internal/semantics/place/addressable.go | Canonical place projection decomposition API |
| internal/semantics/ownershipresult/result.go | Clarify cleanup plan semantics; remove carrier-move map |
| internal/semantics/ownership/ownership_test.go | Add effects build; update evidence sources |
| internal/semantics/flowresult/result.go | Embed typecheck CaseTest; remove duplicated match evidence |
| internal/semantics/effect/visitor.go | Exhaustive semantic op visitor contract |
| internal/semantics/effect/visitor_test.go | Visitor dispatch coverage |
| internal/semantics/constantresult/result.go | Separate module constants from query cache |
| internal/semantics/collector/collector.go | Bindings method sets; module ID in symbols |
| internal/semantics/bindingresult/result.go | New bindings result container |
| internal/semantics/binder/type_decl_cycles.go | Type decl graph IDs include moduleid.ID |
| internal/semantics/binder/binder.go | OperationFunctions via bindings |
| internal/semantics/binder/binder_test.go | moduleid.ID updates |
| internal/project/type_lookup.go | Import lookup by module ID |
| internal/project/imports.go | ResolvedImport carries moduleid.ID; SplitSeq removals |
| internal/project/imports_test.go | Assert resolved ID identity |
| internal/project/generic_types.go | Track owner module by moduleid.ID |
| internal/project/export_fingerprint.go | Fingerprint depends on ctx + published constants; bindings method sets |
| internal/project/context.go | Modules/fileIndex keyed by moduleid.ID |
| internal/project/context_test.go | Diagnostics grouping uses ID string |
| internal/prelude/prelude.go | Derive canonical prelude module ID |
| internal/pipeline/loader.go | Schedule modules by moduleid.ID; prelude redundant import info |
| internal/phase/phase.go | Add Effects phase |
| internal/moduleid/identity.go | Canonical module identity encoding |
| internal/moduleid/identity_test.go | Identity framing collision tests |
| internal/lsp/workspace.go | Local import filter uses ID origin; workspace path normalization |
| internal/lsp/workspace_test.go | Regression: imported default provenance after reset |
| internal/lsp/state.go | Diagnostic scheduling uses WaitGroup |
| internal/lsp/server_test.go | Prelude identity assertion via prelude.ModuleID |
| internal/lsp/hover.go | Bindings/typechecking usage; import ID rendering |
| internal/lsp/cursor.go | Bindings-aware symbol resolution |
| internal/lsp/completion.go | Bindings/typechecking aware completion; import via ID |
| internal/ir/mir/validate.go | New MIR topology/identity validator |
| internal/ir/mir/validate_test.go | Validator positive/negative coverage |
| internal/ir/mir/module_lower.go | Return cleanup from plan; assign drop from plan only |
| internal/ir/mir/module_lower_test.go | Ensure return value computed before planned drops |
| internal/ir/mir/model.go | Seal Instr/Terminator sets with markers |
| internal/ir/mir/model_membership_test.go | Compile-time membership assertions |
| internal/ir/hir/validate.go | New HIR shape validator |
| internal/ir/hir/validate_test.go | Validator coverage |
| internal/ir/hir/model.go | Remove Return cleanup + Assign DropTarget |
| internal/ir/hir/lower/variant_rebind_test.go | Lowering tests for index assign + variant rebind |
| internal/ir/hir/lower/lower_interface.go | Interface impl evidence from typechecking |
| internal/ir/hir/fold/fold.go | Fold updated for HIR model changes |
| internal/ir/hir/fold/fold_test.go | Remove return cleanup test; update assign folding |
| internal/ir/cfg/model.go | Canonical Directed adjacency for sites/blocks; new BlockLoopExit |
| internal/ir/cfg/cfg_test.go | Update tests for Directed edges + loop exit origin |
| internal/ir/cfg/build.go | Build edges into Directed graphs |
| internal/ir/cfg/analyze.go | Use block-edge predecessors; structuredControl for loop exit |
| internal/graph/worklist.go | New FIFO dedup/reschedule worklist |
| internal/graph/worklist_test.go | Worklist behavior test |
| internal/graph/graph_test.go | Algorithms ignore empty IDs test |
| internal/graph/directed_test.go | Directed adjacency + algorithms coverage/bench |
| internal/frontend/lexer/lexer_test.go | Replace b.Loop with b.N loop |
| internal/frontend/ast/meta.go | Attribute/Attributed child traversal |
| internal/frontend/ast/decl.go | Decls traverse attributes via forEachChild |
| internal/diagnostics/codes.go | Add ICE + redundant import/qualifier info codes |
| internal/diagnostics/bag.go | Rename moduleKey→moduleScope (opaque string) |
| internal/contracts/type_dispatch_test.go | New semantic type kind dispatch contract |
| internal/backend/llvm/emitter.go | Exhaustive MIR instruction/terminator emission; panic on malformed MIR |
| internal/backend/llvm/emitter_test.go | Add malformed MIR panic test; SplitSeq removal |
| internal/backend/llvm/drop_emit.go | Clarify runtime drop walk vs source policy |
| go.mod | Set Go baseline to 1.23.2 |
| docs/compiler-framework/type-capabilities.md | Rationale for capability consolidation |
| docs/compiler-framework/README.md | Framework architecture overview |
| COMPILER_FRAMEWORK_REPORT.md | Final migration report |
| CONTRIBUTING.md | Point contributors to architecture docs |
| COMPILER_GUIDELINES.md | Record canonical kernels/extension rules |
| cmd/dump.go | Artifact paths include full module identity components |
| cmd/dispatch.go | Replace errors.AsType with errors.As |
| cmd/cli/test_helpers_test.go | Add chdir helper for Go 1.23 |
| cmd/cli/list_test.go | Use chdirForTest helper |
| cmd/cli/init_test.go | Use chdirForTest helper |
| cmd/cli/get_test.go | Use chdirForTest helper |
| cmd/build.go | Error messages use module.ID.ImportPath |
Review details
- Files reviewed: 128/160 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+55
to
+58
| if stmt == nil { | ||
| return append(problems, fmt.Sprintf("function %s holds a nil statement", fn)) | ||
| } | ||
| switch node := stmt.(type) { |
Member
Author
There was a problem hiding this comment.
Fixed in 7c0b397: every interface slot (HIR statements including block children and optional else, MIR terminators and instructions) is now guarded through the canonical typed-nil probe (pkg/typednil.IsNil), so a typed nil is reported as malformed instead of panicking the validator. Regression tests cover each former panic path.
Comment on lines
+75
to
+82
| if block.Term == nil { | ||
| // Emission would otherwise fall off the end of a block. | ||
| problems = append(problems, fmt.Sprintf("function %s block b%d has no terminator", fn.Name, block.ID)) | ||
| continue | ||
| } | ||
| problems = append(problems, validateTransfers(fn, block, blocks)...) | ||
| } | ||
| return problems |
Member
Author
There was a problem hiding this comment.
Fixed in 7c0b397: every interface slot (HIR statements including block children and optional else, MIR terminators and instructions) is now guarded through the canonical typed-nil probe (pkg/typednil.IsNil), so a typed nil is reported as malformed instead of panicking the validator. Regression tests cover each former panic path.
Comment on lines
+49
to
+52
| // validateStmt walks through the canonical child traversal rather than a switch | ||
| // of its own, so a new statement kind is covered here the moment it declares its | ||
| // children. What it adds is the checks a traversal cannot make: an empty slot is | ||
| // invisible to a walk that skips nils. |
Member
Author
There was a problem hiding this comment.
Fixed in 7c0b397: every interface slot (HIR statements including block children and optional else, MIR terminators and instructions) is now guarded through the canonical typed-nil probe (pkg/typednil.IsNil), so a typed nil is reported as malformed instead of panicking the validator. Regression tests cover each former panic path.
itsfuad
commented
Sep 6, 2026
Member
Author
There was a problem hiding this comment.
Can be superseeded by new Go version builtin test utilities
| } | ||
| if status, ok := errors.AsType[programExitStatus](err); ok { | ||
| var status programExitStatus | ||
| if errors.As(err, &status) { |
Member
Author
There was a problem hiding this comment.
Can be superseeded by new Go version But its okay to be here like that
| "fmt" | ||
|
|
||
| "compiler/internal/frontend/ast" | ||
| graphcore "compiler/internal/graph" |
Member
Author
There was a problem hiding this comment.
better not to use alias. Use if only option. Otherwise rename param or variable instead for clash
| package cfg | ||
|
|
||
| import ( | ||
| graphcore "compiler/internal/graph" |
Member
Author
There was a problem hiding this comment.
better not to use alias. Use if only option. Otherwise rename param or variable instead for clash
Typed nils pass plain nil comparisons because the interface value itself is non-nil, so the HIR statement validator and the MIR terminator/instruction validators dereferenced nil concrete pointers and panicked instead of reporting the malformed artifact. Guard every interface slot through one typed-nil probe, fix the HIR validator comment that claimed traversal-based coverage the switch does not provide, and cover the panic paths plus the optional-slot case with regression tests.
The interface nil trap was probed by three private copies: ast.IsNilNode, typeinfo.isNilType, and the ir.WithOrigin guard added with the validator hardening. Detection policy now lives once in pkg/typednil.IsNil, every call site uses it directly, the duplicated helpers are deleted without wrappers, and reflect imports drop out of the ir, ast, and typeinfo packages. A policy change is now a one-function edit.
There was a problem hiding this comment.
🔵 Needs a closer look
Scope spans many core compiler phases/contracts and identity/validation semantics, so changes need final human architectural review despite tests.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
internal/ir/hir/validate.go:83
- Loop validation builds a map just to iterate optional blocks, but
roleis never used and_ = roleis a no-op. This adds noise and can mislead readers into thinking loop parts are labeled in diagnostics when they are not. Prefer direct nil checks for Init/Bindings/Next so intent stays clear.
- Files reviewed: 131/166 changed files
- Comments generated: 0 new
- Review effort level: Lite
Validation never labels loop init, bindings, or next in diagnostics, so iterating a role-keyed map and discarding the key suggested reporting that does not exist. Iterate the optional blocks directly.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Intent
Consolidate structural traversal, graph/worklist kernels, places, and published semantic effects while preserving explicit compiler phase boundaries. Keep semantic evidence canonical without introducing a compiler-wide pass framework.
Scope
This PR includes the full unmerged architectural migration and subsequent qualitative-review fixes (160 changed files relative to main), not only the latest nine commits. main is an ancestor of this branch; no open overlapping PR was found.
Review fixes
Validation
Passed sequentially on the submitted source:
Full executable fixture suite passed in 9.188s. Focused semantic, ownership, HIR/MIR and backend regressions passed during implementation. Original provenance audit repros now pass; negative replacement fixture requires six T0037 diagnostics. Formatting and edited-Go diagnostics clean.
Review-fix round (7c0b397 + 58a737e)
Re-ran after the typed-nil hardening (7c0b397) and the pkg/typednil centralization (58a737e):
Regression proof for the validator fix: reverting only the guards reproduces
invalid memory addresspanics in the new tests; with the guards, both validators report the malformed artifact instead. The typed-nil detection policy now lives in one place,pkg/typednil.IsNil, replacing ast.IsNilNode, typeinfo.isNilType, and the ad-hoc ir guard.Go baseline remains 1.23.2. Actual local validation used go1.26.7-X:nodwarf5, not the baseline compiler. Signature-only history rewrite preserved the exact source tree; nine rewritten commits verified by GitHub.
Boundaries and follow-up
Review plan
Draft for architectural and correctness review. Inspect producer evaluation order, field-loan replacement and sibling preservation, flow-to-HIR payload evidence, artifact validators, then documentation and compatibility. Do not merge until review findings and required checks are resolved.