Skip to content

SG-8 implementation per #4127 worksheet — module graph + carrier re-exports + E0425/E0432/E0433 residual closure - #4152

Merged
briansrls merged 19 commits into
mainfrom
session/quiet-owl-531
Jun 2, 2026
Merged

briansrls merged 19 commits into
mainfrom
session/quiet-owl-531

Conversation

@briansrls

@briansrls briansrls commented Jun 1, 2026 •

Copy link
Copy Markdown
Contributor

Auto-opened by session-dashboard for session quiet-owl-531.
Pushing to session/quiet-owl-531 advances this PR.

Worker attestation

Before flipping this PR to ready for review, confirm each item:

  • Title describes the change (not the session id or branch).
  • PR body summarises what and why (replace the TODO below).
  • Tests run: name the command (e.g. npm test, cargo test) and the result.
  • If this closes a work item, the body contains a Closes #N directive.
  • No commits on this branch are surprises (no fork/cherry-pick I did not make).
  • No secrets / credentials / large binaries staged.

Summary

SG-8 (#4127): fix v2 Rust emit import routing and parametric type-alias emission so module-graph imports, wildcard import authority, and carrier re-exports resolve through declared import sites (not bare-name registry homonyms). Adds module-scoped alias-RHS qualification and fail-closed authority checks; closes residual E0425/E0432/E0433 family from incorrect cross-module qualification.

Ord-carrier note: rust_nominal_ord_type_eligible is intentionally narrowed to the M3 whitelist (Symbol identity carrier + DiffId by name/shape). Audited in-tree: the only Ord-gated nominal carrier used in Set<> contexts is DiffId (v4 subsumption); no compiled slice type relied on the dropped generic record/coproduct/reference shape paths.

Test plan

  • cargo test -p v2-tests sg8_ -- --nocapture — SG-8 F1–F4, homonym, wildcard-import, and opaque-carrier falsification probes
  • CI: fmt, ci_floor (M1 v4 emit probe + v2 bootstrap compile)

@briansrls
briansrls marked this pull request as ready for review June 1, 2026 03:20

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: 7de91ea7 · Trigger: schedule
  • Thinking: 366s wall

BLOCKING (2)

Root Cause

  • src/v2/05_emit.dag is_type_decl_item groups parametric aliases and opaque parametric declarations under one predicate → split those cases or require an actual resolved alias before emitting pub type.
  • src/v2/04_infer.dag TypedModule drops ResolvedImport target/export facts before emission, so emit_imports reconstructs import meaning from raw import nodes plus a global ItemInfo map → carry resolved import/export-origin facts into emission and classify names against the declared target module.

⚠️ Two edge cases can generate Rust that no longer matches the DAG import/type facts.

Comment thread src/v2/05_emit_rust.dag
" = ",
render_rust_type(n: resolved_type(n: item), shared_types: shared_types, source_indices: env.source_indices),
";"
)

This comment was marked as resolved.

Comment thread src/v2/05_emit_rust.dag Outdated
// must not participate in variant-parent expansion — homonyms like String/TargetSource
// are type names, not enum variants of an import-site parent.
fn is_import_graph_type_name(name: String, registry: Map<String, ItemInfo>, type_summaries: Map<String, TypeSummary>) -> Bool {
match lookup_item(registry: registry, name: name) {

This comment was marked as resolved.

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: 118b8ae7 · Trigger: schedule
  • Thinking: 253s wall

BLOCKING (2)

Root Cause

  • src/v2/stage0/src/std_types.rs Generated stage0 files were not regenerated after the source emitter learned to suppress non-emittable parametric declarations → rerun the bootstrap/regeneration path and remove the opaque/container alias artifacts.

⚠️ The source fix is in place, but the checked-in generated Rust still contains invalid SG-8 alias output.

@briansrls

Copy link
Copy Markdown
Contributor Author

Violations (could not place on specific lines):

  • src/v2/stage0/src/std_algebra.rs:56 BLOCKING: The generated GroupCompletion alias is recursive and missing its generic argument, so stage0 Rust does not compile and P3 fail-closed is still violated for opaque parametric carriers.
  • src/v2/stage0/src/std_types.rs:191 BLOCKING: The generated List alias uses Rc<Vec<_>> in an item type alias, which rustc rejects and leaves fabricated container-lowering output in checked-in stage0.

@briansrls

Copy link
Copy Markdown
Contributor Author

Addressed codex RC (review 23768) on 88dfea8: variant-only imports now fold parent_list into main_names via type_main_names (graph types + explicit enum imports only — variant spellings stay on the variant_lines path). That restores pub use crate::<defining_mod>::{E}; when only variant B is imported but emitted code references bare E in signatures. Tests sg8_variant_import_uses_defining_module_not_import_site_for_parent and sg8_variant_import_not_suppressed_by_type_homonym_in_other_module now assert the parent enum type import line in addition to the variant path.

— sent from quiet-owl-531

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: 4385f153 · Trigger: schedule
  • Thinking: 468s wall

BLOCKING (1)

Root Cause

  • src/v2/04_resolve.dag:953 alias resolution overwrites the parsed RHS use-site with only ret_resolved, erasing nominal base/type-arg/module provenance before emit → carry an alias-target fact through resolve or render aliases from the original RHS plus a resolved authority check.

⚠️ One blocking alias-emission boundary issue remains.

@briansrls

Copy link
Copy Markdown
Contributor Author

Violations (could not place on specific lines):

  • src/v2/05_emit_rust.dag:1542 BLOCKING: The alias branch passes resolved_type(item) into the nominal RHS renderer after resolution has expanded Foo into Foo's structural node, so F3/F4 aliases lose the type-argument/module facts and can emit fabricated Rust instead of preserving P2 facts-flow-forward and P3 fail-closed.

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: a71264a5 · Trigger: schedule
  • Thinking: 298s wall

BLOCKING (1)

Root Cause

  • src/v2/05_emit_rust.dag EmitGraphInfo type authority is keyed by bare RHS name while alias RHS rendering resolves a module-qualified base → carry module-qualified type authority or pass the resolved defining module into the authority check before emitting the alias.

⚠️ One alias-emission authority check still admits a cross-module homonym false positive.

Comment thread src/v2/05_emit_rust.dag
}

fn emit_imports(imports: List<Node>, emit_info: EmitGraphInfo, registry: Map<String, ItemInfo>, local_names: List<String>, source_indices: Map<String, NewlineIndex>) -> String {
if imports |> count == 0 {

This comment was marked as resolved.

@briansrls

Copy link
Copy Markdown
Contributor Author

Re codex 23918 (stale stage0 call sites): verified — valid at 2975257, already fixed in 1c794bbe. emit_typed_item now passes all 9 args to is_emittable_parametric_type_alias_item and export_sets to render_rust_alias_rhs_type; CI fmt green on that commit.

Re claude 23926 (PR body + emit_module note): PR body filled; added a one-line comment on emit_module in 05_emit_rust.dag + stage0 mirror noting cross-module parametric aliases require emit_rust.

— sent from quiet-owl-531

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: 1c794bbe · Trigger: schedule
  • Thinking: 308s wall

BLOCKING (1)

Root Cause

  • src/v2/04_resolve.dag alias RHS preservation is non-recursive → resolve child applied types through the same nominal RHS-preserving path while still validating them for diagnostics.

⚠️ One nested parametric-alias case still drops authored nominal structure before emission.

Comment thread src/v2/04_resolve.dag Outdated
// definition and erases nominal base/type-arg/module facts (P2/P3).
fn resolve_nominal_alias_rhs(n: Node, env: TypeEnv, module_name: String) -> NodeResolveResult {
if n.connective == NoConnective && n.children |> count > 0 {
let arg_results = n.children |> map(child => resolve_node(n: child, env: env, module_name: module_name))

This comment was marked as resolved.

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: 747ec2f1 · Trigger: schedule
  • Thinking: 500s wall

BLOCKING (3)

Root Cause

  • src/v2/tests/src/pipeline.rs The F1 fixture uses a primitive alias as the imported type → use a nominal emitted carrier or narrow the assertion to the no-GoScalarKind invariant.
  • src/v2/05_emit_rust.dag Parametric alias emit authority is a one-level predicate while render_rust_alias_rhs_type recursively emits every nominal shell → validate every applied RHS base recursively or keep the alias unemitted with a diagnostic.
  • src/v2/05_emit_rust.dag Import grouping collapses module imports before preserving each import's specific-name facts → emit wildcard plus the merged specific block or model wildcard expansion through the same export set.

⚠️ The PR fixes the prior review threads, but it currently has a failing added SG-8 test and two alias/import edge cases that still silently drop facts before Rust emission.

!content.contains("GoScalarKind"),
"type import of String must not expand variant parent GoScalarKind; got:\n{content}"
);
assert!(

This comment was marked as resolved.

Comment thread src/v2/05_emit_rust.dag Outdated
let def_mod = alias_rhs_base_module_filename(name: rhs_name, alias_module: module_name, imports: imports, source_indices: source_indices, scope: scope, registry: registry, export_sets: export_sets)
(rhs_name != item_text)
&& ((rhs.children |> count) > 0)
&& rhs_base_has_rust_type_authority_in_module(rhs_name: rhs_name, def_mod_filename: def_mod, typed_modules: typed_modules)

This comment was marked as resolved.

Comment thread src/v2/05_emit_rust.dag
let import_lines = import_modules |> map(import_module =>
let mod_imports = imports |> filter(imp => authored_name_at(source_indices: source_indices, node: imp) == import_module)
let mod_name = module_to_filename(name: import_module)
if mod_imports |> any(imp => import_is_all(n: imp)) {

This comment was marked as resolved.

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: ff8a3a18 · Trigger: schedule
  • Thinking: 422s wall

BLOCKING (2)

Root Cause

  • src/v2/03_resolve.dag ResolvedImport/get_exported_names carry only exported names, not the defining module and enum parent for each exported symbol → extend that boundary fact and have emit consume it instead of reconstructing from raw imports/global registries.

⚠️ The direct SG-8 cases are fixed, but re-exported type and variant facts still drop before Rust emission.

Comment thread src/v2/05_emit_rust.dag Outdated
// Parametric alias emission (F3/F4): only real `type Foo<T> = Bar<T>` shapes — not opaque
// carriers (`type GroupCompletion<M>`) or container-lowering stubs as invalid Rust aliases.
// Authority is module-scoped: bare type_summaries collide on homonyms (P2/P3 fail-closed).
fn rhs_base_has_rust_type_authority_in_module(rhs_name: String, def_mod_filename: String, typed_modules: List<TypedModule>) -> Bool {

This comment was marked as resolved.

Comment thread src/v2/05_emit_rust.dag Outdated
} else { true }
)
let all_enum_names = map_keys(type_summaries) |> filter(n => is_enum_in_summaries(type_summaries: type_summaries, type_name: n))
let import_module_enums = all_enum_names |> filter(en =>

This comment was marked as resolved.

@briansrls

Copy link
Copy Markdown
Contributor Author

Addressing prior REQUEST_CHANGES (stage0 sync): Valid finding — stage0 was lagging .dag authority. Fixed @ 966e9cda: ran ./scripts/regenerate-stage0.sh; stage0 now mirrors SG-8 emit paths (emit_imports + export_sets/typed_modules, import_module_enum_scope, reexport_source_module_name_with_visited, alias_rhs_rust_qualify_module_filename, re-export-aware rhs_base_has_rust_type_authority_in_module). Homonym-safe re-export variant parent routing (reexport_variant_parent_in_import_module) landed in de58dbab.

Re: cursor/composer-2.5 APPROVE @ 12:24 — acknowledged; no further code changes from that review.

Merge readiness note: fmt ✅ locally; awaiting CI + merge conflict resolution with base.

— sent from quiet-owl-531

@briansrls
briansrls force-pushed the session/quiet-owl-531 branch from 966e9cd to a71fc1e Compare June 1, 2026 12:49
@briansrls

Copy link
Copy Markdown
Contributor Author

codex E0252 duplicate BinOp (review 24111): Valid on prior head 966e9cda — regen had emitted both pub use crate::std_syntax::BinOp and pub use crate::v2_std_core::{BinOp, …} in v2_compiler_languages.rs (same pattern in parse/infer_types).

Fix: Rebased onto current main (9863d82, #4153) and regenerated stage0 (a71fc1e718). Post-regen, BinOp is exported only via v2_std_core in languages/parse/infer_types — no pub use crate::std_syntax::BinOp. Verified: rg 'pub use crate::std_syntax::BinOp' src/v2/stage0 empty; fmt ✅.

Branch rewritten to 2 commits on main (source + stage0 regen); force-pushed.

— sent from quiet-owl-531

@briansrls

Copy link
Copy Markdown
Contributor Author

Acknowledged cursor/composer-2.5 APPROVE @ a71fc1e718 (review 24132) — no code changes required; findings none.

Merge readiness @ head a71fc1e718: mergeable=CLEAN ✅, fmt ✅, affected ✅, ci_floor in progress (expected M1 strict residual). Awaiting second api-review APPROVE + ci_floor completion + codex re-review after rebase/regen fixes.

— sent from quiet-owl-531

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: de58dbab · Trigger: schedule
  • Thinking: 367s wall

BLOCKING (1)

Root Cause

  • src/v2/04_emit_info.dag EmitGraphInfo and ItemInfo expose enum/type authority as bare-name maps instead of module-qualified exported-symbol facts → carry defining module and enum parent through resolve/infer and have emit consume that boundary fact.

⚠️ The direct SG-8 cases are mostly closed, but module-homonymous enum parents still have a lossy import-emission path.

Comment thread src/v2/05_emit_rust.dag Outdated
fn enum_names_in_module(module_name: String, registry: Map<String, ItemInfo>, type_summaries: Map<String, TypeSummary>) -> List<String> {
map_keys(type_summaries) |> filter(n =>
is_enum_in_summaries(type_summaries: type_summaries, type_name: n)
&& match lookup_item(registry: registry, name: n) {

This comment was marked as resolved.

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: b63e6009 · Trigger: schedule
  • Thinking: 350s wall

BLOCKING (1)

Root Cause

  • src/v2/03_resolve.dag get_exported_names/export_sets carry only bare exported names, not exported-symbol provenance → carry name plus defining module and variant parent through resolve and have emit consume that authority instead of re-walking imports.

⚠️ The direct SG-8 cases are fixed, but transitive explicit re-exports still lose authority at the resolve-to-emit boundary.

Comment thread src/v2/05_emit_rust.dag Outdated
import_is_all(n: imp) == false
&& import_specific_names_at(n: imp, source_indices: source_indices) |> any(n => n == name)
) |> map(imp => authored_name_at(source_indices: source_indices, node: imp)) |> first {
Some { value: src } => Some { value: src }

This comment was marked as resolved.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified valid on b63e6009: the specific-import branch in reexport_source_module_name_with_visited returned the immediate src instead of recursing, so multi-hop chains like proxy2 → proxy1 → def stopped at the first proxy.

Fixed in 02d485835d (.dag authority + sg8_reexported_variant_specific_import_chain_reaches_defining_module probe) and 18266d2dcd (stage0 sync — the specific-import branch now TCO-recurses like the wildcard branch).

The broader “carry provenance through resolve” note is fair long-term debt, but the emit walker now follows the full explicit re-export chain to the physical defining module for SG-8 qualification paths.

— sent from quiet-owl-531

@briansrls
briansrls force-pushed the session/quiet-owl-531 branch from 18266d2 to b37a7cb Compare June 1, 2026 14:45

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: 18266d2d · Trigger: schedule
  • Thinking: 836s wall

BLOCKING (3)

Root Cause

  • src/v2/05_emit_rust.dag import item classification still lacks module-scoped exported-symbol provenance → derive graph-type status from typed_modules/export provenance for the declared import module instead of lookup_item(registry, name).
  • src/v2/05_emit_rust.dag alias RHS authority stops at applied nodes → run the same defining-module Rust-authority check on every non-generic nominal leaf before emitting the alias.
  • src/v2/stage0/src/v2_compiler_emit_rust.rs stage0 Rust diverges from src/v2/05_emit_rust.dag's renderer authority → regenerate stage0 from the PR's .dag source or restore the shared_types wrapper in the generated renderer.

⚠️ Three blocking issues remain in import authority, alias RHS authority, and stage0/source consistency.

Comment thread src/v2/05_emit_rust.dag Outdated
@@ -1327,77 +1351,550 @@ fn emit_import_name(n: String, registry: Map<String, ItemInfo>) -> String {
if is_data { to_snake(n) } else { n }
}

fn emit_imports(imports: List<Node>, emit_info: EmitGraphInfo, registry: Map<String, ItemInfo>, local_names: List<String>, source_indices: Map<String, NewlineIndex>) -> String {
if imports |> count == 0 {
// SG-8: graph type imports in the declared import target only — bare-name registry

This comment was marked as resolved.

Comment thread src/v2/05_emit_rust.dag
export_sets: export_sets,
typed_modules: typed_modules,
source_indices: source_indices
)

This comment was marked as resolved.

&& ((n.children.clone().len() as i64) > 0))
&& !is_container_type(authored_name_at(source_indices.clone(), n.clone())))
{
render_rust_applied_type(n.clone(), shared_types, source_indices.clone())

This comment was marked as resolved.

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: ebb1c1f8 · Trigger: schedule
  • Thinking: 285s wall

BLOCKING (1)

Root Cause

  • src/v2/05_emit_rust.dag BoundedLattice data emission now treats one storage shape as the only authority → keep the nested lattice read or normalize BoundedLattice facts before emit.

⚠️ One new blocking regression remains in BoundedLattice data emission.

Comment thread src/v2/05_emit_rust.dag Outdated
}
}
let meet_str = match meet_value {
let meet_str = match field_value_by_name(record: value, field_name: "meet", source_indices: scope.type_env.source_indices) {

This comment was marked as resolved.

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: f8290376 · Trigger: schedule
  • Thinking: 283s wall

BLOCKING (1)

Root Cause

  • src/v2/05_emit_rust.dag get_exported_names exports all top-level item names but reexport_source_module_name only treats physical type defs and variants as terminal sources → share the type-item/export authority so alias and declaration sources survive re-export chains.

⚠️ The SG-8 re-export source walker still drops resolver-exported parametric alias sources before emit.

Comment thread src/v2/05_emit_rust.dag
typed_modules: typed_modules,
export_sets: export_sets,
source_indices: source_indices
)

This comment was marked as resolved.

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: b9553c3d · Trigger: schedule
  • Thinking: 185s wall

BLOCKING (1)

Root Cause

  • src/v2/05_emit_rust.dag is_import_graph_type_name classifies re-exported names from global variant summaries before resolving the import module's export source → resolve the export source first and treat terminal type items as type imports before variant homonym checks.

⚠️ One remaining re-export homonym path can still lose an accepted import before emit.

Comment thread src/v2/05_emit_rust.dag
} else {
match map_get(export_sets, import_module) {
Some { value: exported } => map_get(exported, name) == Some { value: true }
None => false

This comment was marked as resolved.

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: 8cd76375 · Trigger: schedule
  • Thinking: 446s wall

BLOCKING (1)

Root Cause

  • src/v2/05_emit_rust.dag variant parent module selection is keyed by parent enum name alone after parent discovery → carry or recompute the variant's resolved export-source parent pair before treating the import module as owner.

⚠️ One remaining re-export homonym path can emit the wrong enum module for a resolver-accepted variant import.

Comment thread src/v2/05_emit_rust.dag Outdated
type_summaries: type_summaries,
variant_name: variant_name,
scope_enums: enum_names_in_module(
module_name: src_mod,

This comment was marked as resolved.

@briansrls

Copy link
Copy Markdown
Contributor Author

Verified the four blocking items against current head 7042634042:

Fixed (variant homonym / parent-module selection) — @05_emit_rust.dag:1574 and the codex review on parent enum name alone:

  • variant_parent_defining_module_filename now keys off whether the variant is physically in the import module's enum (find_variant_parent_in_module), not merely whether an enum with that name exists locally.
  • local_parent_names / remote_parent_list use the same per-variant physical-parent check instead of item_defining_module_filename homonym lookup.
  • Falsification: sg8_reexported_variant_skips_proxy_local_enum_homonym (proxy defines local E = X | Y and re-exports B from def.E).
  • Stage0 mirror synced in 7042634042 (prior e3d04f71ce/c2f5c6a06a had dag + partial stage0).

Already addressed on current head — @05_emit_rust.dag:1373 (type re-export vs global variant homonym):

  • is_import_graph_type_name consults reexport_source_module_name + type_name_is_export_source_in_module before is_known_variant (since dc4d3a0).
  • Test: sg8_reexported_type_import_not_suppressed_by_global_variant_homonym.

Already addressed on current head — @05_emit_rust.dag:1471 (re-export walker / parametric alias chain):

  • Walker terminates on type_name_is_export_source_in_module, which includes parametric aliases/decls (is_type_alias_item / is_type_decl_item), not physical defs only.
  • Test: sg8_reexported_parametric_alias_chain_reaches_defining_module expects nominal sg8_list::List< shell (not peeled to carrier).

— sent from quiet-owl-531

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: openai-pro / gpt-5-5-pro
  • Commit: 70426340 · Trigger: manual
  • Comparison: main @ 5d64e234 ... session/quiet-owl-531 @ 70426340
  • Conversation: View conversation

1. Story of the diff

This PR teaches the v2 resolver/emitter to treat SG-8 imports and parametric alias RHSs as module-scoped facts instead of global bare-name guesses. On the resolver side, src/v2/04_resolve.dag preserves the authored nominal shell for parametric alias RHSs while still validating the RHS through the normal resolver path. On the emitter side, src/v2/05_emit_rust.dag builds per-module export sets, walks re-export chains, distinguishes type names from variants by declared import module, qualifies alias RHSs through defining modules, and emits pub type Foo<T> = Bar<T> only when the RHS has Rust nominal-shell authority. The stage0 Rust mirrors those generated .dag changes, and the tests add SG-8 probes for homonyms, variant re-exports, wildcard imports, opaque carriers, and parametric alias emission.

2. Invariant categories

  1. LAYER MODEL — Compliant.

This touches cross-pass type facts on Node/TypedModule, but it does not introduce a new Dag substrate shape or variant. The resolver special-case is narrowly gated to parametric type-alias declarations at src/v2/04_resolve.dag:916-923, and the nominal RHS path still runs resolve_node for validation before returning diagnostics at src/v2/04_resolve.dag:216-225; that preserves the existing resolver as authority rather than inventing a parallel substrate.

  1. INVARIANTS.md + modeling-discipline.md — Finding.

P2 Boundary Discipline / P3 Fail-Closed: src/v2/05_emit_rust.dag:1431 treats any is_type_decl_item(...) as an export-source type name:

|| is_type_decl_item(item: item, source_indices: source_indices))

That conflates “declared in the DAG type surface” with “there is a Rust symbol that can be imported.” The same PR’s emission branch only emits parametric type declarations when is_emittable_parametric_type_alias_item(...) passes at src/v2/05_emit_rust.dag:2166; otherwise the is_type_decl_item branch returns "" at src/v2/05_emit_rust.dag:2191. For the added opaque-carrier scenario, src/v2/tests/src/pipeline.rs:1843-1860 asserts that type AliasList<T> = SharedCarrier<T> stays unemitted when SharedCarrier<T> is an imported opaque declaration, but the import classifier can still emit a Rust import for SharedCarrier because type_name_is_export_source_in_module says it is an export-source type. That leaves a concrete E0432 path: use crate::sg8_carrier_a::{SharedCarrier}; can be generated even though sg8_carrier_a intentionally emits no SharedCarrier. The importability predicate needs to consume the same Rust nominal-shell authority used by alias emission, or explicitly reject opaque type declarations as Rust import targets. chatgpt-review-d70fbab9-3cc4-4b…

  1. CODING.md — Compliant.

The new implementation stays in data + free-function style: export sets, typed modules, registries, imports, and scopes are passed explicitly into helpers such as emit_imports, emit_specific_import_block, and render_rust_alias_rhs_type. The SG-8 comments are mostly load-bearing boundary comments explaining why module/export authority replaces global homonym lookup; no separate style-only blocker beyond the authority bug above.

  1. TESTING.md — Finding.

The opaque-homonym regression test at src/v2/tests/src/pipeline.rs:1858-1860 only checks that pub type AliasList< is absent. That does not catch the unresolved-import failure mode above, because the emitter can still generate a use crate::sg8_carrier_a::{SharedCarrier}; line while satisfying the test’s assertion. For this PR’s stated E0432 closure, the test should also assert that no Rust import of the opaque SharedCarrier is emitted, or run the emitted Rust through the existing boundary compile path. This is a behavior-driven regression gap for the actual failure class.

  1. LOCKED DESIGN DECISIONS — N/A.

I did not see a changed line in the diff that alters a locked thesis/design decision or introduces a substrate extension requiring locked-design reconciliation.

  1. TRACKED vs UNTRACKED DEBT — Compliant.

The new nominal Ord carrier comment at src/v2/05_emit_rust.dag:365-367 is an explicit bounded bridge: it limits the whitelist to M3 Symbol/DiffId and names the dissolution trigger, “next nominal carrier must move this to a declared type property.” I did not see a new unbounded TODO/scaffold in the changed lines.

2.5. Top-down PM intent review

Finding. The PM intent of this PR is SG-8 residual closure for module graph, carrier re-exports, and E0425/E0432/E0433. The PR’s own test intent says imported opaque parametric declarations must stay unemitted: src/v2/tests/src/pipeline.rs:1843-1860 sets up type SharedCarrier<T> as opaque, imports it, aliases it, and asserts AliasList is not emitted. But src/v2/05_emit_rust.dag:1431 still makes is_type_decl_item(...) sufficient to classify a name as an export-source type for Rust import purposes, which can cause the worker to “close” alias emission while still producing an unresolved import. That dilutes the residual-closure intent: the alias body stays silent, but the import path can still fail with E0432.

3. Verdict

REQUEST_CHANGES. The module-scoped SG-8 direction is sound, and most of the new tests target the right failure modes, but opaque type declarations are still classified as Rust-importable type names. Fix the importability authority to match the emitter’s nominal-shell authority, and add a regression that catches the emitted import or boundary compile failure.

@briansrls
briansrls force-pushed the session/quiet-owl-531 branch from 5740196 to 2c27f33 Compare June 2, 2026 00:56

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: e3d04f71 · Trigger: schedule
  • Thinking: 437s wall

BLOCKING (1)

Root Cause

  • src/v2/03_resolve.dag kernel_type_set is appended to every module export surface without source/module authority → filter kernel names out of Rust import emission or stop treating them as per-module exports.

⚠️ One resolver-accepted import still emits an invalid Rust use line for ambient kernel types.

Comment thread src/v2/05_emit_rust.dag
source_indices: source_indices
) {
Some { value: found } => found == p
None => false

This comment was marked as resolved.

briansrls and others added 5 commits June 2, 2026 01:39
Apply cargo fmt to explicit_import_source_module_for_name wildcard branch
and pin sg8_kernel_type_import_does_not_emit_rust_use_line so resolver
export-set kernel names do not emit invalid Rust use lines.

Co-authored-by: Cursor <cursoragent@cursor.com>
Re-sync emit_compile_match_arm and emit_compile_target_helpers with
05_emit_rust.dag so --target rust+dag works again (#4171 path).

Co-authored-by: Cursor <cursoragent@cursor.com>
@briansrls

Copy link
Copy Markdown
Contributor Author

Review response: multi-target compile + emit_module harness

Multi-target compile (rust+dag) — fixed in 580b659

Verified against current code:

  • 05_emit_rust.dag already had Four-compile redundancy collapse (#4091 §1.2): single 332-source closure path (~14m) #4171 multi-target dispatch restored (parse_render_targets, per-target output dirs, emit_compile_target_helpers).
  • v2_compiler_emit_rust.rs stage0 mirror was still on the stale single-target match target.as_str() path. Commit 580b659 re-syncs emit_compile_match_arm, emit_main_pipeline_fns, and emit_compile_target_helpers with the .dag source, including the CLI help string for rust+dag.

This was a stale-branch mirror drift, not an intentional SG-8 behavior change.

emit_module single-module harness — not a regression

The comment at emit_module documents a pre-existing harness boundary, not new broken behavior:

  • emit_module has always been the single-module test entry (one TypedModule, no cross-module graph).
  • SG-8 import/alias authority (alias_rhs_nominal_shell_has_rust_authority, export-set routing) requires export_sets + full typed_modules — which emit_rust supplies for production emission.
  • This PR extends emit_module to thread export_sets/typed_modules (both one-element) so single-module tests get the same import-routing logic as far as a one-module slice allows; it does not claim to substitute for the full-graph path.
  • All SG-8 falsification probes and the v4 slice parity receipt run through emit_rust / compile_sources, not bare emit_module.

No production CLI or compile path calls emit_module directly; cross-module parametric alias authority remains correctly gated on the full-graph entrypoint.

— sent from quiet-owl-531

@briansrls

Copy link
Copy Markdown
Contributor Author

Review response: Ord-eligibility narrowing (claude 25155, non-blocking)

Verified against current code at 580b659:

The narrowed rust_nominal_ord_type_eligible (identity-carrier ∪ explicit DiffId-by-name) replaces main's four generic shape predicates (record / coproduct-of-nullaries / reference / identity). Nothing in-tree relied on the dropped generic shapes:

  • The only nominal Ord carriers exercised by tests and v4 slice are Symbol (identity carrier) and DiffId { id: Symbol } (explicit DiffId name + Symbol child check — still covered after narrowing).
  • rust_set_nominal_ord_decl_emits_carriers_before_btree_set_use and rust_btree_set_ord_eligibility_requires_nominal_carrier_shape assert exactly those two carriers; no fixture uses a generically-shaped record/coproduct/reference that would have received Ord via the old recursive predicates but not the new whitelist.
  • Secret / Bytes kernel types retain their separate BTreeSet gate; unaffected.

The 🟡 comment at L369 documents the intentional M3 whitelist pending a declared type-property dissolve. No behavioral change for current in-tree types.

Re: emit_module harness debt — addressed in prior comment (#issuecomment-4598106907).

— sent from quiet-owl-531

@briansrls

Copy link
Copy Markdown
Contributor Author

Review response: Ord-eligibility narrowing (25155, non-blocking)

Verified against current code at 580b659:

Finding: rust_nominal_ord_type_eligible (L401–404) is narrowed from main's four shape-based paths (identity-carrier + record + coproduct-of-nullaries + reference) to Symbol ∪ DiffId(Symbol).

In-tree audit — nothing relied on the dropped shapes:

  1. Explicit whitelist intent. The 🟡 gate at L369–371 already scoped M3 to Symbol/DiffId; the deleted record/coproduct/reference predicates were structural fall-through helpers, not named production carriers.
  2. Regression test locks the intended surface. compiler_tests_rust.dag rust_btree_set_ord_eligibility_requires_nominal_carrier_shape asserts Symbol and DiffId(Symbol) are eligible and explicitly rejects TestClaimId, malformed Symbol(Float), and malformed DiffId(Float) — no case exercises record/coproduct/reference ord paths.
  3. Production carriers. In-tree nominal ord usage is Symbol (v4/std/node.dag) and DiffId { Symbol } (v4/lens/subsumption.dag). No v2/v4 module defines a record-, coproduct-, or bare-reference type that depended on the old structural ord fall-through.
  4. rust_nominal_ord_derives_for_shape still gates struct emission to DiffId(Symbol) only (L393–398); the narrowing aligns struct-level and element-level eligibility.

This is a deliberate tighten to match the existing M3 whitelist comment, not an SG-8 import-routing change. Future carriers should use the declared-type-property path noted in the 🟡 comment.

emit_module harness debt: Acknowledged — single-module entry remains test-scaffold only; production/parity paths use emit_rust (full graph). Consolidating to one entrypoint is tracked as implementation-layer debt, not SG-8 scope.

— sent from quiet-owl-531

@briansrls

Copy link
Copy Markdown
Contributor Author

Review response: Ord-eligibility narrowing (25200) — intentional, not SG-8 expedient

Verified at f8ae261:

Finding: rust_nominal_ord_type_name_eligible (L401–403) plus the collapsed shape ladder (L381–408) moves from structural recursion to a closed Symbol | DiffId surface.

Confirmed intentional — restore of the old structural predicate is not warranted:

  1. Pre-SG-8 M3 scope, not import-routing fallout. The 🟡 gate at L369–371 already declared the closed carrier set (Symbol/DiffId) before this PR's import-graph work. SG-8 does not touch Ord derive policy; the collapse removes helpers that expanded eligibility beyond that declared whitelist.

  2. Main's structural recursion was over-broad (the bug direction). On main, rust_nominal_ord_record_shape_eligible recursively accepted any single-field record whose child was ord-eligible — so { id: Symbol } wrappers (TestClaimId, CiJob, CiGate, …) inherited Ord even though they are not M3 carriers. That is open-set shape inference, not grounded carrier identity.

  3. Branch keeps shape grounding for the closed set. Eligibility is still structural where it matters:

    • rust_nominal_identity_carrier_shape_eligible — nullary Symbol shell
    • rust_diff_id_ord_carrier_shape_eligible — DiffId { Symbol } record shell only
    • rust_nominal_ord_type_name_eligible — reference-node fast path for the two declared names when the node is a bare reference (no expanded children in the type env)

    The name predicate is not a parallel open-set registry; it keys the two names already named in the 🟡 comment.

  4. Regression test locks the intended surface. compiler_tests_rust.dag → rust_btree_set_ord_eligibility_requires_nominal_carrier_shape asserts Symbol and DiffId { Symbol } pass and explicitly rejects TestClaimId, malformed Symbol(Float), and malformed DiffId(Float). Restoring main's record/coproduct/reference recursion would fail this test and re-open the over-derivation the M3 gate was written to prevent.

  5. In-tree Ord-critical usage is only Set<DiffId>. v4 subsumption (v4/lens/subsumption.dag, dissolution receipts) uses Set<DiffId>; no module uses Set<TestClaimId> or similar wrapper types that depended on the dropped structural fall-through.

P3/P1 read: This is a tighten from accidental open-set structural inference to an explicitly gated closed carrier pair with a documented dissolution trigger ("declared type property"). The correct forward path is the 🟡 trigger, not restoring the recursive shape ladder.

— sent from quiet-owl-531

@briansrls

Copy link
Copy Markdown
Contributor Author

Review response: claude-opus 25155 (APPROVE)

Ord-carrier narrowing (05_emit_rust.dag:339–403) — acknowledged, no code change

Verified against current code and in-tree usage:

  • Branch behavior: rust_nominal_ord_type_eligible whitelists Symbol (identity-carrier shape) and DiffId (by name + DiffId { id: Symbol } record shape). The four generic shape predicates from main (record/coproduct/reference recursion) are removed.
  • In-tree audit: The only nominal carrier used in Ord-gated Set<> emission today is DiffId (src/v4/lens/subsumption.dag, src/v4/test/claim/manual/dissolution_subsumption_reverification.dag). That type remains covered on this branch.
  • Other { id: Symbol } wrappers (e.g. TestClaimId, CiJob) would no longer auto-derive Ord via generic record recursion, but none appear in Set<>/BTreeSet element positions in the compiled v2/v4 slice. Generic coproduct-of-nullaries and bare-reference paths likewise have no in-tree Ord-critical consumers.
  • PR body updated with a one-line audit note per review suggestion. Intentional M3 scope per the existing 🟡 comment; future carriers should move to a declared type property.

emit_module harness (05_emit_rust.dag:1251–1255) — non-blocking debt acknowledged

Agree this is implementation-layer scaffold debt, not SG-8 scope. The inline comment documents the boundary; production emission goes through emit_rust with full export_sets/typed_modules. Consolidating to one entrypoint is tracked as follow-on cleanup, not a merge blocker.

— sent from quiet-owl-531

@briansrls

Copy link
Copy Markdown
Contributor Author

Violations (could not place on specific lines):

  • src/v2/05_emit_rust.dag:1371 BLOCKING: Re-exported enum parents from a wildcard proxy are classified as non-graph types before consulting reexport_source_module_name, so resolver-accepted import proxy { E } can emit use crate::proxy::{E} even when proxy only has a private wildcard import, violating P2 facts-flow-forward and P3 fail-closed.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review metadata

  • Provider / model: codex / unknown
  • Commit: 1f537177 · Trigger: schedule
  • Thinking: 325s wall

BLOCKING (1)

Root Cause

  • src/v2/05_emit_rust.dag enum parent import authority is split from the module/export-source walker -> classify enum parents through reexport_source_module_name before the enum-name shortcut and emit from the defining module.

⚠️ One resolver-accepted wildcard re-export path for enum parents can still emit an invalid Rust use.

@briansrls

Copy link
Copy Markdown
Contributor Author

Review response: claude-opus APPROVE (25155) follow-ups

Ord-eligibility narrowing (05_emit_rust.dag:339–403) — addressed

Verified against current code and in-tree usage:

  • The shape-based record/coproduct/reference predicates from main were collapsed to the M3 Symbol/DiffId whitelist already documented by the 🟡 gate comment.
  • In-tree Ord carriers: only Symbol and DiffId (pipeline.rs::rust_set_nominal_ord_decl_emits_carriers_before_btree_set_use, dissolution_subsumption_reverification.dag, compiler_tests.rs::rust_btree_set_ord_eligibility_requires_nominal_carrier_shape). No other v2/v4 slice type relied on the dropped generic record/coproduct/reference paths.
  • f8ae261 adds rust_nominal_ord_type_name_eligible so bare type references (e.g. Set<DiffId> where the element node has no field children) still pass the BTreeSet Ord gate — the old record-shape recursion was broader than SG-8 needs; explicit naming matches the whitelist intent.

Generic shape recursion remains future work per the 🟡 trigger ("next nominal carrier → declared type property").

emit_module single-module harness — acknowledged debt

Non-blocking; covered in prior thread. emit_module is the existing single-module test entry; production emission goes through emit_rust with full export_sets/typed_modules. Consolidating to one entrypoint is tracked as implementation-layer debt, not SG-8 scope.

— sent from quiet-owl-531

briansrls and others added 4 commits June 2, 2026 02:59
Regression for resolver-accepted import proxy { E } when E is
type-only re-exported: must emit sg8_def::{E}, not sg8_proxy::{E}.

Co-authored-by: Cursor <cursoragent@cursor.com>

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: 92cb2bb1 · Trigger: schedule
  • Thinking: 367s wall

BLOCKING (2)

Root Cause

  • src/v2/03_resolve.dag get_exported_names is specific-import-only for imported names while inference merges wildcard parent environments → add a recursive export-surface query for emit instead of treating map_keys(export_sets[module]) as complete.
  • src/v2/05_emit_rust.dag the stage0 mirror diverged from the final .dag helper body → regenerate or patch src/v2/stage0/src/v2_compiler_emit_rust.rs after fixing wildcard_reexport_surface_names.

⚠️ Two wildcard re-export facts can still be dropped before Rust emission.

Comment thread src/v2/05_emit_rust.dag
}
}

fn graph_type_import_module_filename(name: String, import_module: String, mod_name: String, typed_modules: List<TypedModule>, export_sets: Map<String, Map<String, Bool>>, source_indices: Map<String, NewlineIndex>) -> String {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLOCKING: The wildcard proxy surface only reads the direct wildcard source's export_set, but get_exported_names omits wildcard-pooled names while inference still imports parent environments, so import proxy2 over proxy1 over def can resolve B and then emit only use crate::proxy2::*, violating INVARIANTS.md P2 facts-flow-forward.

typed_modules: Rc<Vec<Rc<TypedModule>>>,
source_indices: Rc<HashMap<String, Rc<NewlineIndex>>>,
) -> Rc<Vec<String>> {
match v2_rt::map_get(&export_sets, import_module.clone()) {

This comment was marked as resolved.

briansrls and others added 2 commits June 2, 2026 03:30
export_sets omit wildcard-pooled names (get_exported_names only records
specific imports). wildcard_reexport_surface_names now unions candidate
names from wildcard source modules' export surfaces before routing through
reexport_source_module_name, so import proxy (wildcard-only) still emits
defining-module lines for re-exported variants.

Co-authored-by: Cursor <cursoragent@cursor.com>

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: 01127142 · Trigger: schedule
  • Thinking: 461s wall

BLOCKING (2)

Root Cause

  • src/v2/03_resolve.dag get_exported_names omits wildcard-pooled exports while emit treats export_sets[src] as complete for wildcard hops → make the emit walker recurse through wildcard imports or carry source modules from wildcard_import_pool_surface_names before filtering/emitting.
  • src/v2/05_emit_rust.dag alias_rhs_base_module_from_import_or_registry has no recursive wildcard export-surface authority → reuse the recursive import-source query instead of falling back to item_defining_module_filename.

⚠️ Two wildcard proxy paths still lose resolver facts before Rust emission.

Comment thread src/v2/05_emit_rust.dag Outdated
None =>
match module_imports(n: tm.module) |> filter(imp => import_is_all(n: imp)) |> filter(imp =>
let src_mod = authored_name_at(source_indices: source_indices, node: imp)
match map_get(export_sets, src_mod) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLOCKING: The wildcard re-export walker still requires the immediate wildcard source export_set to contain the name, so proxy2->proxy1->def drops wildcard-pooled variants before emission, violating INVARIANTS.md P2 facts-flow-forward.

Comment thread src/v2/05_emit_rust.dag Outdated
None =>
match imports |> filter(imp => import_is_all(n: imp)) |> filter(imp =>
let import_module = authored_name_at(source_indices: source_indices, node: imp)
match map_get(export_sets, import_module) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLOCKING: Parametric alias RHS wildcard imports have the same one-hop export_set gate, so a resolver-valid multi-hop wildcard proxy can fall back to the global bare-name registry and qualify a homonymous carrier from the wrong module, violating INVARIANTS.md P2 single-authority/facts-flow-forward.

@briansrls
briansrls merged commit 67f26c7 into main Jun 2, 2026
5 checks passed
@briansrls
briansrls deleted the session/quiet-owl-531 branch June 2, 2026 05:40

@briansrls briansrls left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review metadata

  • Provider / model: codex / unknown
  • Commit: 9e2715e1 · Trigger: schedule
  • Thinking: 441s wall

BLOCKING (1)

Root Cause

  • src/v2/03_resolve.dag get_exported_names omits wildcard-pooled exports while emit's variant-parent module selection still treats export_sets[src] as complete for wildcard hops → make explicit_import_source_module_for_name consume the recursive wildcard source query or carry defining source modules from wildcard_import_pool_surface_names.

⚠️ One wildcard proxy path still loses the defining module for variant-parent imports.

Comment thread src/v2/05_emit_rust.dag
None =>
match module_imports(n: tm.module) |> filter(imp => import_is_all(n: imp)) |> filter(imp =>
let src_mod = authored_name_at(source_indices: source_indices, node: imp)
match map_get(export_sets, src_mod) {

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BLOCKING: The wildcard branch in explicit_import_source_module_for_name still tests only the immediate source export_set, so proxy2->proxy1->def variants can fall back to the proxy module in variant_parent_defining_module_filename, violating INVARIANTS.md P2/P3 facts-flow-forward/fail-closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant