Skip to content

engine: element-cycle dt-recurrence SCC resolution uses no-input build_var_info wiring; Phase 2 must plumb real module_input_names before consuming ResolvedScc.element_order #573

@bpowers

Description

@bpowers

Summary

The new element-level cycle-resolution code (branch clearn-hero-model, Phase 1, commits fe09d9f5..7241d414) identifies dt-recurrence SCCs and sources per-element exprs under no-module-input wiring (build_var_info(db, model, project, &[])), while the real production dependency-graph path (model_dependency_graph_impl) builds with the actual module_input_names. In Phase 1 this is a sound completeness gap (conservative/loud-safe). It becomes a latent correctness hazard in Phase 2, where ResolvedScc.element_order becomes load-bearing and must reflect the real with-inputs wiring.

This was flagged as Minor issue #1 by the Phase 1 code-reviewer, with the explicit recommendation to file a tracking item so Phase 2 addresses it before consuming element_order. Phase 1 itself remains correctly approved -- the gap is conservative in Phase 1 (worst case is a missed resolution that falls back to CircularDependency, never a wrong simulation).

The divergence (entirely within System A / the production salsa path)

  • resolve_dt_recurrence_sccs -- src/simlin-engine/src/db_dep_graph.rs:947 -- calls build_var_info(db, model, project, &[]) (empty module inputs) to build the SCC universe.
  • phase_element_order -- src/simlin-engine/src/db_dep_graph.rs:718-730 -- calls var_phase_lowered_exprs_prod (db_dep_graph.rs:493, doc at :480), which by its own contract uses "the default no-module-input wiring build_var_info(.., &[])".
  • The recorded ResolvedScc.element_order (db_dep_graph.rs:898-900, element_order: dt_order) therefore reflects the no-input lowering's per-element order.

By contrast, the real path:

  • model_dependency_graph_impl -- src/simlin-engine/src/db.rs:1200-1207 -- builds build_var_info(db, model, project, &module_input_names) with the actual module inputs.
  • model_dependency_graph_with_inputs (db.rs:1603-1609) is the non-empty-inputs entry point; the no-input model_dependency_graph (db.rs:~1619) is the other.
  • Dependency edges differ by wiring: variable_direct_dependencies_with_context_and_inputs (db.rs:999-1006) vs variable_direct_dependencies_with_context (db.rs:989). When a sub-model self-recurrent variable's incoming edges are cut by isModuleInput, the no-input wiring and the with-inputs wiring can identify different SCCs and produce a different per-element order.

Why it is sound in Phase 1 (not a correctness bug yet)

model_dependency_graph_impl re-runs the final compute_transitive(false/true, &resolvable) on the real with-inputs var_info, and the .unwrap_or_else(...) clears resolved_sccs / sets has_cycle on any residual genuine cycle. So in Phase 1:

  • Worst case is a missed resolution: a self-recurrent variable inside an input-wired sub-model conservatively falls back to CircularDependency instead of resolving. Loud and safe, never a silently-wrong simulation.
  • ResolvedScc.element_order may carry the no-input lowering's order, but element_order is NOT consumed in Phase 1 -- Phase 1 Task 6 makes no runlist change. So the possibly-wrong order is inert in Phase 1.

Why it matters / when it becomes live

This becomes load-bearing in Phase 2 of the element-cycle-resolution plan (docs/implementation-plans/2026-05-18-element-cycle-resolution/, see phase_02.md), where element_order is consumed by the new combined-fragment lowering. A sub-model self-recurrence under isModuleInput could then carry a WRONG per-element order if the no-input vs with-inputs wiring differs -- a genuine miscompilation, not a conservative fallback.

Required action for Phase 2

Phase 2 must plumb the real module_input_names into resolve_dt_recurrence_sccs and var_phase_lowered_exprs_prod (and phase_element_order) before element_order is consumed by the combined-fragment lowering, so SCC identification and per-element ordering use the same with-inputs wiring as model_dependency_graph_impl. Equivalently: thread &module_input_names through these helpers instead of the hard-coded &[], mirroring model_dependency_graph_impl's build_var_info(db, model, project, &module_input_names).

Components affected

  • src/simlin-engine/src/db_dep_graph.rs -- resolve_dt_recurrence_sccs (:947), phase_element_order (:718-730), var_phase_lowered_exprs_prod (:493, doc :480), ResolvedScc.element_order emission (:898-900)
  • src/simlin-engine/src/db.rs -- model_dependency_graph_impl (:1200-1207, the correct with-inputs build_var_info), model_dependency_graph_with_inputs (:1603-1609), variable_direct_dependencies_with_context{,_and_inputs} (:989 / :999)
  • docs/implementation-plans/2026-05-18-element-cycle-resolution/phase_02.md -- the consumer that makes this load-bearing

Relationship to existing issues (DISTINCT, not duplicates)

Discovery context

Identified during Phase 1 code review of the element-level cycle-resolution work (branch clearn-hero-model, commits fe09d9f5..7241d414), flagged by the Phase 1 code-reviewer as Minor issue #1 with an explicit recommendation to file a tracking issue so Phase 2 addresses it. Phase 1 was approved on the understanding that this gap is conservative/loud-safe in Phase 1 and that the with-inputs plumbing is a Phase 2 obligation gated on element_order consumption.

Metadata

Metadata

Assignees

No one assigned

    Labels

    engineIssues with the rust-based simulation engine

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions