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

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.

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

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