diff --git a/.claude/skills/address-feedback/SKILL.md b/.claude/skills/address-feedback/SKILL.md index f10a48dad..51229941d 100644 --- a/.claude/skills/address-feedback/SKILL.md +++ b/.claude/skills/address-feedback/SKILL.md @@ -101,7 +101,7 @@ Ignore suggestions that: - Would add unnecessary complexity - The reviewer convinced itself weren't actually problems -**Deferred feedback**: Only defer feedback that is genuinely unrelated to this PR's changes -- pre-existing issues in untouched code, future feature requests, or theoretical concerns about code paths this PR does not introduce or modify. For each deferred item, spawn the `track-issue` agent (via the Task tool with `subagent_type: "track-issue"`) with a detailed description. +**Deferred feedback**: The default is to FIX what a review surfaces, including things the review found by accident -- see "Discovered Issues" in the root `CLAUDE.md`. Defer only feedback that is genuinely unrelated to this PR's changes AND too large to fold into it: pre-existing issues in untouched code, future feature requests, or theoretical concerns about code paths this PR does not introduce or modify. Deferring is an explicit call, not a default: say what you are deferring and why. For each deferred item, spawn the `track-issue` agent (via the Task tool with `subagent_type: "track-issue"`) with a detailed description. **CRITICAL**: P0/P1/P2 feedback about code introduced or modified by THIS PR must NEVER be deferred. If a reviewer flags a correctness, data-loss, or behavioral bug in code that this branch touches, fix it in this review cycle. Deferring P1 feedback on your own changes is not acceptable -- it means shipping a known bug. When in doubt about whether feedback is "in scope", err on the side of fixing it. diff --git a/CLAUDE.md b/CLAUDE.md index 170a75e94..7031fc16e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -121,11 +121,18 @@ IMPORTANT: If feedback seems non-actionable, it means you need comments explaini ### libsimlin API Design Keep the FFI surface small and orthogonal. Prefer composable primitives over bulk endpoints. Do NOT add bulk/batch variants to paper over caller-side concurrency issues. -## Tracking Discovered Issues +## Discovered Issues -When you discover something wrong or concerning during your work -- tech debt, design limitations, broken tooling, missing CI checks, unintended consequences of a committed design, deferred review feedback -- it must be explicitly tracked. Never silently drop these observations. +When you discover something wrong or concerning during your work -- tech debt, a latent bug, design limitations, broken tooling, missing CI checks, unintended consequences of a committed design, deferred review feedback -- **fix it as part of the work**. Never silently drop these observations, and never file an issue as a substitute for fixing one. -Spawn the `track-issue` agent (via the Task tool with `subagent_type: "track-issue"`) with a description of the problem. The agent checks for duplicates in GitHub issues and [docs/tech-debt.md](/docs/tech-debt.md), then files the item if it's not already tracked. Using a sub-agent preserves your context on the main task. +Filing defers the cost without reducing it, and the context needed to fix a problem is at its cheapest the moment you find it. Name what you fixed in the commit message or PR body (`Fixes #` when an issue already exists). + +Two things are NOT covered by "fix it", and both are explicit conversations rather than silent decisions: + +- **The fix is too large to fold in** -- it would swamp the branch's review surface, or it belongs to a different subsystem. Say so, with a cost estimate, and sequence it: its own commit, its own PR, or (if that is what the user wants) tracked for later. Do not make that call quietly. +- **The fix is not yours to make** -- it needs a product decision, or access you do not have. Surface it. + +When something genuinely does need tracking, spawn the `track-issue` agent (via the Task tool with `subagent_type: "track-issue"`) with a description of the problem. It checks for duplicates in GitHub issues and [docs/tech-debt.md](/docs/tech-debt.md) and files the item, keeping your context on the main task. ## Generated/Noise Paths diff --git a/docs/design-plans/2026-05-13-macros.md b/docs/design-plans/2026-05-13-macros.md index 492f6f3fa..934889e75 100644 --- a/docs/design-plans/2026-05-13-macros.md +++ b/docs/design-plans/2026-05-13-macros.md @@ -60,6 +60,7 @@ Vensim macros become a first-class, persistent concept in the Simlin engine -- i - **macros.AC5.4 Success:** A macro shadowing a builtin (`SSHAPE`, `RAMP FROM TO`) resolves to the macro; the builtin is not invoked. - **macros.AC5.5 Success:** A macro defined after its first use (`macro_trailing_definition`) still resolves and simulates. - **macros.AC5.6 Failure:** A call to a name that is neither a macro, a stdlib function, nor a builtin reports an "unknown function or macro" error. +- **macros.AC5.7 Failure:** A macro-marked model whose body instantiates a module (`Variable::Module`) is rejected at registry-build time, naming the macro and the offending module variable. A cycle-safety rule rather than a taste rule: `db::project_module_graph` -- the gate every compile / diagnostic / analysis entry point consults so a module cycle surfaces as `CircularDependency` instead of salsa's dependency-graph cycle panic -- records only EXPLICIT module edges, because it must stay parse-free. A macro CALL is an implicit edge, so `mac ->(explicit module)-> u ->(macro call)-> mac` was a cycle the gate reported as absent and every entry point then aborted on (fatal under `panic = "abort"`). Not redundant with AC5.2: that cycle runs through a NON-macro model, which the macro-to-macro call graph cannot express, and the macro set is otherwise valid. The rejection is deliberately broader than the cycle -- an acyclic module inside a macro is rejected too -- and that cost is real rather than zero: the shape is reachable from an ordinary XMILE file (the `` content model is shared with ``, so a `` written inside a `` is passed through unfiltered rather than synthesized), Simlin's own XMILE writer round-trips it, and an acyclic instance compiles and simulates correctly. It is rejected anyway for two reasons: narrowing to only-when-cyclic requires a second reachability analysis inside `MacroRegistry::build` that must agree with `db::project_module_graph`'s, and the back edge it would have to see is the macro CALL -- discoverable only by parsing every model's equations, i.e. the same dependency-list cost that ruled out widening the graph; and a macro is a *template*, so instantiating a sub-model inside one is dubious on its own terms, making this a language rule with a cycle-safety motivation rather than a workaround. How many real models this affects is a judgement, not a measurement -- Stella emits no `` and xmutil emits them only from Vensim `:MACRO:` blocks, which cannot contain modules, so the population is hand-written XMILE or a third-party writer: small, but not empty. The MDL importer cannot produce the shape, though not because "Vensim macros have no modules" -- its multi-output materializer does mint `Variable::Module`; it is simply never run over a macro body. A module *targeting* a macro model (how a multi-output invocation works) is unaffected; only a module *inside* a macro-marked model is rejected. ### macros.AC6: Validation corpus and consumer floor - **macros.AC6.1 Success:** All six `test/test-models/tests/macro_*` fixtures are wired into the active test suite and pass. diff --git a/src/libsimlin/src/lib.rs b/src/libsimlin/src/lib.rs index 8b5ef4cd2..7d2dc1f6f 100644 --- a/src/libsimlin/src/lib.rs +++ b/src/libsimlin/src/lib.rs @@ -308,6 +308,9 @@ impl From for SimlinErrorCode { // wire Generic code; the specific engine code and the message // carry the detail (GH #905). engine::ErrorCode::UnknownElementSubscript => SimlinErrorCode::Generic, + // A macro body instantiating a module likewise collapses to the wire + // Generic code; the engine code and the message carry the detail. + engine::ErrorCode::MacroContainsModule => SimlinErrorCode::Generic, } } } diff --git a/src/libsimlin/src/patch.rs b/src/libsimlin/src/patch.rs index eccc1363b..c707283d9 100644 --- a/src/libsimlin/src/patch.rs +++ b/src/libsimlin/src/patch.rs @@ -404,6 +404,15 @@ pub(crate) fn gather_error_details_with_db( all_errors } +/// The code reported for a rejected patch: the first Error-severity diagnostic, +/// else the VM validation error. +/// +/// "First" is now deterministic. `collect_all_diagnostics` emits the +/// project-level failures (bad unit declarations, an invalid macro set) ahead of +/// the per-model passes, where they used to surface in whatever order the salsa +/// accumulator DFS happened to walk. Accept/reject is unchanged -- the set of +/// Error-severity diagnostics is the same -- but a project carrying both a +/// project-level and a per-model error now reports the project-level code. fn first_error_code( diagnostics: &[engine::db::Diagnostic], sim_error: Option<&engine::Error>, diff --git a/src/simlin-engine/CLAUDE.md b/src/simlin-engine/CLAUDE.md index 78491bc77..b95345be8 100644 --- a/src/simlin-engine/CLAUDE.md +++ b/src/simlin-engine/CLAUDE.md @@ -11,7 +11,7 @@ Equation text flows through these stages in order: 1. **`src/lexer/`** - Tokenizer for equation syntax 2. **`src/parser/`** - Recursive descent parser producing `Expr0` AST 3. **`src/ast/`** - AST type system with progressive lowering: `Expr0` (parsed) -> `Expr1` (modules expanded) -> `Expr2` (dimensions resolved) -> `Expr3` (subscripts expanded). `array_view.rs` tracks array dimensions and sparsity. `Expr2Context` trait includes `has_mapping_to()` for cross-dimension mapping lookups during `find_matching_dimension`. -4. **`src/builtins.rs`** - Builtin function definitions (e.g. `MIN`, `PULSE`, `LOOKUP`, `QUANTUM`, `SSHAPE`, `VECTOR SELECT`, `VECTOR ELM MAP`, `VECTOR SORT ORDER`, `VECTOR RANK`, `ALLOCATE AVAILABLE`, `ALLOCATE BY PRIORITY`, `NPV`, `MODULO`, `PREVIOUS`, `INIT`). `is_stdlib_module_function()` is the authoritative predicate for deciding whether a function name expands to a stdlib module; the **module-function** generalization that also resolves project macros lives in **`src/module_functions.rs`** -- the pure (Functional-Core) `ModuleFunctionDescriptor`/`MacroRegistry` resolver+validator unifying stdlib functions and macros (`stdlib_descriptor`, `MacroRegistry::build`/`resolve_macro`, the shared `is_renamed_opcode_intrinsic`/`is_renamed_stdlib_module_builtin`/`is_renamed_builtin_macro_collision` precedence predicates). A **genuine passthrough macro** -- a single-parameter, single-output macro whose body is exactly `out = BUILTIN(param)` self-calling its own renamed-builtin-collision name (`:MACRO: INIT(x) = INITIAL(x)`, stored after the importer's `INITIAL`->`INIT` rename as `init = init(x)`) -- is classified once at `MacroRegistry::build` time (`classify_passthrough`, the only place the body AST is available) into a `passthrough: Option` on the descriptor; `builtins_visitor.rs` reads it to *collapse the call to the builtin opcode* (`init`->`LoadInitial`) instead of expanding the buggy per-element synthetic module (#591), falling through to the same renamed-builtin intrinsic routing the #554 self-call exception takes. Strict criteria (no additional outputs, bare-parameter argument, self-call, renamed-builtin-collision name) keep it from misfiring on a near-miss like `INIT = INIT(x) + 1`. `equation_is_module_call()` (pre-scan, renamed from `equation_is_stdlib_call`) and `contains_module_call()` (walk-time, renamed from `contains_stdlib_call`) both consult the `MacroRegistry` (a passthrough caller is still classified module-backed here -- benign, since it collapses to a flat-slot variable). `builtins_visitor.rs` handles implicit module instantiation, single-output macro inlining (an arrayed macro invocation enters the per-element path), and PREVIOUS/INIT helper rewriting: unary `PREVIOUS(x)` desugars to `PREVIOUS(x, 0)`, direct scalar args compile to `LoadPrev`, and module-backed or expression args are first rewritten through synthesized scalar helper auxes. `INIT(x)` compiles to `LoadInitial`, using the same helper rewrite when needed. Tracks `module_idents` so `PREVIOUS(module_var)` never reads a multi-slot module directly. +4. **`src/builtins.rs`** - Builtin function definitions (e.g. `MIN`, `PULSE`, `LOOKUP`, `QUANTUM`, `SSHAPE`, `VECTOR SELECT`, `VECTOR ELM MAP`, `VECTOR SORT ORDER`, `VECTOR RANK`, `ALLOCATE AVAILABLE`, `ALLOCATE BY PRIORITY`, `NPV`, `MODULO`, `PREVIOUS`, `INIT`). `is_stdlib_module_function()` is the authoritative predicate for deciding whether a function name expands to a stdlib module; the **module-function** generalization that also resolves project macros lives in **`src/module_functions.rs`** -- the pure (Functional-Core) `ModuleFunctionDescriptor`/`MacroRegistry` resolver+validator unifying stdlib functions and macros (`stdlib_descriptor`, `MacroRegistry::build`/`resolve_macro`, the shared `is_renamed_opcode_intrinsic`/`is_renamed_stdlib_module_builtin`/`is_renamed_builtin_macro_collision` precedence predicates). `MacroRegistry::build` validates in four passes: duplicate macro name and macro/model name collision (macros.AC5.3), macro-to-macro recursion (macros.AC5.2), and -- Pass 4 -- a `Variable::Module` inside a macro-marked model (macros.AC5.7, `ErrorCode::MacroContainsModule`). Pass 4 is a CYCLE-SAFETY rule and is not redundant with Pass 3: `db::project_module_graph` records only EXPLICIT module edges, so a cycle `mac ->(explicit module)-> u ->(macro call, an IMPLICIT edge)-> mac` was invisible to the gate and drove the recursive queries into salsa's dependency-graph cycle panic, while Pass 3's macro-to-macro graph cannot even express the edge through the non-macro `u`. The rejection is deliberately broader than the cycle -- an acyclic module inside a macro is rejected too -- and that cost is REAL, not zero: the shape is reachable from an ordinary XMILE file (the `` content model is shared with ``, so the reader passes a `` through unfiltered), our own XMILE writer round-trips it, and an acyclic instance compiles and SIMULATES correctly today. It is rejected anyway because narrowing to only-when-cyclic needs a second reachability analysis that must agree with `project_module_graph`'s, whose back edge is the macro CALL -- discoverable only by parsing every model's equations, which is the dependency-list cost widening the graph was rejected for; and because a macro is a TEMPLATE, so instantiating a sub-model inside one is dubious on its own terms and reads as a language rule rather than a workaround. The MDL importer cannot produce it, but not because "Vensim macros have no modules": `src/mdl/convert/multi_output.rs` DOES mint a `Variable::Module` for a multi-output `:`-list invocation, and it is unreachable from a macro body only because the scoped body sub-context hard-codes an empty materialization (`src/mdl/convert/macros.rs`). A module *targeting* a macro (how multi-output invocations work) is untouched -- only a module *inside* a macro-marked model is rejected. A **genuine passthrough macro** -- a single-parameter, single-output macro whose body is exactly `out = BUILTIN(param)` self-calling its own renamed-builtin-collision name (`:MACRO: INIT(x) = INITIAL(x)`, stored after the importer's `INITIAL`->`INIT` rename as `init = init(x)`) -- is classified once at `MacroRegistry::build` time (`classify_passthrough`, the only place the body AST is available) into a `passthrough: Option` on the descriptor; `builtins_visitor.rs` reads it to *collapse the call to the builtin opcode* (`init`->`LoadInitial`) instead of expanding the buggy per-element synthetic module (#591), falling through to the same renamed-builtin intrinsic routing the #554 self-call exception takes. Strict criteria (no additional outputs, bare-parameter argument, self-call, renamed-builtin-collision name) keep it from misfiring on a near-miss like `INIT = INIT(x) + 1`. `equation_is_module_call()` (pre-scan, renamed from `equation_is_stdlib_call`) and `contains_module_call()` (walk-time, renamed from `contains_stdlib_call`) both consult the `MacroRegistry` (a passthrough caller is still classified module-backed here -- benign, since it collapses to a flat-slot variable). `builtins_visitor.rs` handles implicit module instantiation, single-output macro inlining (an arrayed macro invocation enters the per-element path), and PREVIOUS/INIT helper rewriting: unary `PREVIOUS(x)` desugars to `PREVIOUS(x, 0)`, direct scalar args compile to `LoadPrev`, and module-backed or expression args are first rewritten through synthesized scalar helper auxes. `INIT(x)` compiles to `LoadInitial`, using the same helper rewrite when needed. Tracks `module_idents` so `PREVIOUS(module_var)` never reads a multi-slot module directly. 5. **`src/compiler/`** - Multi-pass compilation to bytecode: - `mod.rs` - Orchestration; includes A2A hoisting logic that detects array-producing builtins (VectorElmMap, VectorSortOrder, Rank, AllocateAvailable, AllocateByPriority) during array expansion, hoists them into `AssignTemp` pre-computations, and emits per-element `TempArrayElement` reads. Treats a **standalone lookup-only variable** -- a graphical-function holder whose equation is empty or the legacy MDL `"0+0"` sentinel (`crate::variable::var_is_lookup_only`/`is_empty_or_sentinel`, mirroring `mdl::writer::is_lookup_only_equation`'s "empty or sentinel" rule) -- as a non-value-bearing **static table** (`Variable::Var::is_table_only`): it is excluded from every runlist and from the saved output, so it produces NO series of its own (issue #606). Its data is reached only through `LOOKUP(table, x)` call sites (resolved by ident -> `base_gf`); a *bare* reference with no argument is a compile error (`ErrorCode::LookupReferencedWithoutArgument`). WITH LOOKUP (`var = WITH LOOKUP(input, table)`: tables present *and* a real input) is NOT lookup-only -- it is a value-bearing variable that lowers to `LOOKUP(self, input)` for every equation shape (`apply_implicit_with_lookup`): per element for arrayed variables, where a per-element gf applies each element's OWN table and a gf-less element keeps its raw input equation (GH #909) - `context.rs` - Symbol tables and variable metadata; `lower_preserving_dimensions()` skips Pass 1 dimension resolution to keep full array views for array-producing builtins. Handles `@N` position syntax resolution: in scalar context (no active A2A dimension, not inside an array-reducing builtin), `DimPosition(@N)` resolves to a concrete element offset; inside array-reducing builtins (`preserve_wildcards_for_iteration`), dimension views are preserved for iteration. Two wildcard-preservation contexts: `with_preserved_wildcards()` for reducers (SUM, MEAN, etc.) where `ActiveDimRef` resolves to a concrete offset, and `with_vector_builtin_wildcards()` for array-producing builtins (VectorSortOrder, VectorElmMap, etc.) where `ActiveDimRef` is promoted to `Wildcard` to preserve the full array view @@ -92,8 +92,8 @@ Diagnostics (`collect_all_diagnostics`) run on the user's `SourceProject`, so sy - **`src/datamodel.rs`** - Core structures: `Project`, `Model`, `Variable`, `Equation` (including `Arrayed` variant with `default_equation` for EXCEPT semantics and `has_except_default` bool flag), `Dimension` (with `mappings: Vec` replacing the old `maps_to` field, and `parent: Option` for indexed subdimension relationships), `DimensionMapping`, `DataSource`/`DataSourceKind`, `UnitMap`, `MacroSpec` (a macro-marked `Model`'s calling convention: `parameters`/`primary_output`/`additional_outputs`; `Model.macro_spec` is `None` for every ordinary model; `salsa::Update` so it rides directly on the `SourceModel` input). `Model::new_macro(name, params, additional_outputs, body_variables)` is the shared port-synthesis + `MacroSpec`-construction step used by *both* the MDL converter and the XMILE reader: it sets `can_be_module_input` on each formal-parameter body variable (synthesizing a `Flow`/`Aux` placeholder port when absent) so `collect_module_idents` treats the macro as an ordinary sub-model. View element types (`Aux`, `Stock`, `Flow`, `Alias`, `Cloud`) carry an optional `ViewElementCompat` with original Vensim sketch dimensions/bits for MDL roundtrip fidelity. `StockFlow` has an optional `font` string for the Vensim default font spec. - **`src/variable.rs`** - Variable variants (`Stock`, `Flow`, `Aux`, `Module`), `ModuleInput`, `Table` (graphical functions). `classify_dependencies()` is the primary API for extracting dependency categories from an AST in a single walk, returning a `DepClassification` with five sets: `all` (every referenced ident), `init_referenced`, `previous_referenced`, `previous_only` (idents only inside PREVIOUS), and `init_only` (idents only inside INIT/PREVIOUS). `parse_var_with_module_context` accepts a `module_idents` set so `PREVIOUS(module_var)` rewrites through a scalar helper aux instead of `LoadPrev`. Per-element graphical functions: `build_tables` materializes one `Table` per element of an arrayed GF and `reorder_arrayed_element_tables` places each at the element's *flat row-major declared-dimension index*, NOT its `Equation::Arrayed` `elems` Vec position -- because the runtime selects a per-element table by the row-major dimension offset (`vm.rs` `Lookup`/`LookupArray`: `graphical_functions[base_gf + element_offset]`). Elements lacking a GF get an empty placeholder so the index stays aligned. The salsa dependency-table path mirrors this in `db.rs::extract_tables_from_source_var`. - **`src/dimensions.rs`** - `DimensionsContext` for dimension matching, subdimension detection, and element-level mappings. Supports indexed subdimensions via `parent` field (child maps to first N elements of parent). `has_mapping_to()` checks for element-level dimension mappings between two dimensions. `mapped_element_correspondence(iterated_dim, source_dim)` (GH #527) is the reusable element-level correspondence for a mapped dimension pair -- per iterated element, the source element the executed simulation reads (both declaration directions, single-hop only, POSITIONAL mappings only; an explicit element map returns `None` because the executed A2A lowering resolves positionally and ignores it -- GH #753 and the tracked positional-vs-element-map execution inconsistency (GH #756) are the gate for re-enabling; `None` ⇒ callers keep their conservative broadcast, a superset of the true edges). It is what keeps the LTM element graph (`expand_same_element`'s mapped diagonal) and link-score dimensions (`link_score_dimensions` -- whose mapped arm is additionally gated on the edge having a `Bare`-classified site via `model_edge_shapes`, so the arrayed retarget fires exactly when the element graph expands the diagonal rather than the element-mapped `DynamicIndex` cross-product) in lockstep with the classifier (`classify_iterated_dim_shape`, whose mapped arm gates on this same correspondence in BOTH declaration directions since GH #757 -- via `ltm_agg::classify_axis_access` / `iterated_axis_slot_elements` -- so a reverse-declared positional pair classifies `Bare` and gets the diagonal) and -- for positional mappings -- the simulation; the agg machinery's mapped sliced reducers (GH #534) consume it through `ltm_agg::iterated_axis_slot_elements` (the per-source-element preimage inversion), so the same gate governs hoisting and the emitters' slot remap. `SubdimensionRelation` caches parent-child offset mappings for both named (element containment) and indexed (declared parent) dimensions -- **`src/model.rs`** - Model compilation stages (`ModelStage0` -> `ModelStage1` -> `ModuleStage2`), dependency resolution, topological sort. `collect_module_idents` pre-scans datamodel variables to identify which names will expand to modules (preventing incorrect `LoadPrev` compilation). `init_referenced_vars` extends the Initials runlist to include variables referenced by `INIT()` calls, ensuring their values are captured in the `initial_values` snapshot. Unit checking uses salsa tracked functions in `db.rs`. -- **`src/project.rs`** - `Project` struct aggregating models. `from_salsa(datamodel, db, source_project, cb)` builds a Project from a pre-synced salsa database (all variable parsing comes from salsa-cached results). `from_datamodel(datamodel)` is a convenience wrapper that creates a local DB and syncs. Production code uses `db::compile_project_incremental` with `ltm_enabled`/`ltm_discovery_mode` on `SourceProject`. +- **`src/model.rs`** - Model compilation stages (`ModelStage0` -> `ModelStage1` -> `ModuleStage2`), dependency resolution, topological sort. `collect_module_idents` pre-scans datamodel variables to identify which names will expand to modules (preventing incorrect `LoadPrev` compilation). `init_referenced_vars` extends the Initials runlist to include variables referenced by `INIT()` calls, ensuring their values are captured in the `initial_values` snapshot. Unit checking uses salsa tracked functions in `db.rs`. The two `#[cfg(test)]` `ModelStage0` constructors are the salsa-FREE twin of `db::stages::model_stage0`: `new_in_project(project_models, x_model, ..)` builds a stage from a `datamodel::Model` with no database, resolving module-function calls against the whole project's `MacroRegistry` (which is what makes it a faithful oracle for a model that CALLS a macro defined in a sibling model), and `new(x_model, ..)` is the single-model wrapper the many one-model fixtures use. They derive the module-ident set, the macro registry and the duplicate-ident errors along completely different routes than the query, which is exactly why `db::stages_tests` can use them as an oracle. The former salsa-cached `ModelStage0::new_cached` -- a test-only third copy of the query's construction -- was deleted; its coverage now points at the live query. `enumerate_modules_inner` records a model's instantiation BEFORE descending into it, so that a module cycle terminates: the "have I seen this model" test is the recursion guard, so a model still being walked has to count as seen. (A cycle through `main` already terminated, because `enumerate_modules` records `main` up front; one below `main` did not.) This matches its salsa twin `db::assemble::enumerate_module_instances_inner`. Recording early cannot LOSE an instantiation because the insert is unconditional at every module site -- only the recursion is guarded; the set-of-input-sets value is a separate fact, and it is why the ORDER instantiations arrive in is unobservable. +- **`src/project.rs`** - `Project` struct aggregating models. `from_salsa(datamodel, db, source_project, cb)` builds a Project from a pre-synced salsa database: it READS `db::stages::model_stage1` for every project model and clones each memo (the clone is mandatory, not incidental -- `model_deps.take()`, `set_dependencies` and the `model_cb` all mutate, and `set_dependencies` pushes equation errors onto the variables themselves, while a `returns(ref)` memo is shared with every other reader). It used to build its own whole-project `ModelStage0` map and lower it inline, a second salsa-native copy that had silently drifted from `db::stages` on three fields. Its model order and the Initials runlists `set_dependencies` derives are deterministic: both `topo_sort` seeds are sorted first, since `topo_sort` breaks ties by visit order and the seeds came from a `HashMap`'s keys and a `HashSet` respectively (the `Project`-path twin of the GH #595 fix in `db::dep_graph`, safe to change because nothing shipped reads these runlists -- `compiler::Module::new`, the sole reader of `ModelStage1::instantiations`, is called only from `#[cfg(test)] TestProject::build_module`, while production `src/db/assemble.rs` builds `compiler::Module` by struct literal from salsa fragments). `from_datamodel(datamodel)` is a convenience wrapper that creates a local DB and syncs. **Neither is reachable from production**: inside the crate every caller is a test, and the only in-repo user of the public `From` impl is the engine's own `tests/integration/simulate_ltm.rs`, feeding the `ltm_finding::discover_loops` convenience wrapper (the shipped analysis path, libsimlin's `simlin_analyze_discover_loops`, goes through `discover_loops_with_graph` and builds no `Project`). Treat it as a monolith kept honest as a test oracle, not as live code. Production compiles via `db::compile_project_incremental` with `ltm_enabled`/`ltm_discovery_mode` on `SourceProject`. - **`src/results.rs`** - `Results` (variable offsets + timeseries data), `Specs` (time/integration config) - **`src/patch.rs`** - `ModelPatch`/`ProjectPatch` (both `Clone`) for representing and applying model changes. `ModelOperation` variants: `UpsertVariable`, `DeleteVariable`, `RenameVariable`, `UpsertView`, `DeleteView`, `UpdateStockFlows`, `SetLoopName` (names a feedback loop by variable list, writing `LoopMetadata` entries). `ModelPatch` is consumed by `incremental_layout()` to determine what variables were added/removed/renamed @@ -101,18 +101,20 @@ Diagnostics (`collect_all_diagnostics`) run on the user's `SourceProject`, so sy The primary compilation path uses salsa tracked functions for fine-grained incrementality. Key modules: -- **`src/db.rs`** - The module ROOT of the salsa pipeline. After the db-submodule split it holds only the database itself (`SimlinDb` + its `Db` trait, the sync/staged/restore/`current_source_project` methods, the `StdlibModels` cache, the `impl salsa::Database`/`Db`), the `compile_project_incremental()` production entry point, the LTM-glue surface (`LtmSyntheticVar`/`LtmVariablesResult` result types + the `module_link_score_equation` module-link helper (composite reference / ceteris-paribus / signed `black_box_unit_transfer_equation` fallback) called by the per-shape `link_score_equation_text_shaped` (which lives in `src/db/ltm/compile.rs`) -- since Track A the SOLE derivation of a link score's equation text: the former `(from, to)`-keyed `link_score_equation_text` twin was deleted and assembly's standard scalar Bare score now sources the shaped query directly (`compile_ltm_var_fragment`), so the compiled and reported equations are one value and cannot drift; `module_composite_ports` reads the sub-model's `model_ltm_variables` to decide whether a port has a composite, so the same composite reference is used in exhaustive AND discovery mode)/`find_model_output_ports_for_module`/`module_input_pathways_from_edges`/`generate_max_abs_selection` helpers; the last folds a module input port's "strongest pathway" composite through O(1)-sized accumulator helper variables -- inlining the fold into one nested expression doubles the equation text per pathway, which OOMed on macro modules with hundreds of pathways. Since PR #684 a stockless **passthrough** with input→output pathways ALSO emits pathway/composite vars, and since GH #748 the stock-free early return in `model_ltm_variables` fires only when the model is genuinely STATELESS -- no parent-level stocks, no input ports, and no (transitively) stock-carrying module instance (`modules_carry_state`), so a module-only root whose state lives inside a SMOOTH/DELAY or user sub-model is scored instead of silently emitting nothing, and the exhaustive-mode loop-score builder overrides each `x→m` module link with a **per-exit-port pathway selection** (`compute_module_link_overrides` in `src/db/ltm/mod.rs` emits a `$⁚ltm⁚link_score⁚{x}→{m}⁚via⁚{exit}` alias selecting the pathway the loop actually traverses, threaded into `generate_loop_score_variables` as a `(loop_id, link_index)` side-table) because the composite max-abs-selects across ALL pathways and so picks the wrong output port for a multi-output module; `find_model_output_ports` now sorts its result (GH #680) so the parent's pathway recomputation matches the sub-model's emitted `$⁚ltm⁚path⁚{port}⁚{idx}` indices, the residual being that pinned loops pass an empty override map), the `set_project_ltm_enabled`/`set_project_ltm_discovery_mode` setters plus the `LtmEnabledGuard` RAII scope guard (transiently flips `ltm_enabled` and unconditionally restores it on drop; shared by libsimlin's `simlin_project_get_errors` / from-wasm FFIs (GH #466) and `simlin-mcp-core`'s `read_model`/`edit_model` diagnostic passes (GH #662) so the LTM-only advisory harvest behaves identically across every diagnostic-collection surface), and the `#[cfg(test)]` test-module mounts. Everything else moved into per-concern db submodules (`input`/`query`/`sync`/`diagnostic`/`layout`/`var_fragment`/`fragment_compile`/`assemble`/`dep_graph`/`analysis`/`ltm`/...) and is re-exported at the root so the historical `simlin_engine::db::...` surface (and the `use super::*` globs in the root-mounted test modules and `implicit_deps.rs`) keep resolving. `SimlinDb` OWNS its sync state (a private `sync_state: Option` field; the `#[salsa::db]` macro finds `storage` by type, and the field is only mutated via `&mut self` during sync, never during parallel query execution, so no interior mutability is needed), so incrementality is automatic: `db.sync(project) -> SourceProject` threads the prior handles internally (a no-op re-sync stays a salsa cache hit), `db.sync_staged(project) -> (SourceProject, Option)` additionally returns the PRE-staging handles for an exact rollback, `db.restore(project, prev)` re-syncs the prior datamodel with those handles, and `db.current_source_project()` reads the latest handle. A SECOND, `pub(crate)` slot -- `expanded_state: Option`, driven by `db.sync_expanded(expanded_datamodel)` -- holds the conveyor/queue-expanded twin of the project so the special-stock build path is incremental too (see "Conveyors and queues"); it stays `None` until a special-stock model is first built, is never cleared thereafter (salsa cannot reclaim inputs, so clearing would only force a second set to be minted), and its handle is reachable only as `sync_expanded`'s return value, which is what keeps a rolled-back staged patch from poisoning it. `sync_from_datamodel` (fresh `&db` path returning `SyncResult`) and `sync_from_datamodel_incremental` stay public as the internal mechanism the methods wrap and for the read-only test path; `collect_all_diagnostics(db, project: SourceProject)` iterates `project.models(db)`. `SourceProject` carries `ltm_enabled` and `ltm_discovery_mode` flags for LTM compilation, plus `macro_declarations: Vec<(String, Option)>` -- the ordered, pre-dedup list of project models' canonical names + macro specs that the demand-driven `project_macro_registry` query reads to re-derive the macro build error (the canonical-name-keyed `models` map collapses the duplicate/colliding model names the AC5.3 validation needs, so the ordered declaration list supplies them). `SourceModel` carries `macro_spec: Option` so `project_macro_registry` is keyed only on the macro-marked models. `collect_module_idents`/`equation_is_module_call` now consult the `MacroRegistry` so a `y = MYMACRO(...)` caller is pre-classified as module-backed (correct `PREVIOUS`/`INIT` rewrite); a macro-body variable's `enclosing_model` is the macro name so a renamed `init`/`previous` builtin inside a like-named macro resolves to the intrinsic (#554). `Diagnostic` includes a `severity` field (`Error`/`Warning`) and `DiagnosticError` variants: `Equation`, `Model`, `Unit`, `Assembly`. The salsa inputs store the `datamodel::*` types directly on their multi-field structs (`SourceProject.sim_specs`/`dimensions`/`units`, `SourceModel.model_sim_specs`, `SourceVariable.equation`/`gf`/`module_refs`) -- per-field salsa read tracking is what gives fine-grained invalidation, so no mirror-projection types are needed; `datamodel_variable_from_source` re-assembles the kind-tagged `datamodel::Variable` for the parser. `datamodel::Equation::Arrayed` carries `has_except_default` to drive EXCEPT default application. `VariableDeps` includes `init_referenced_vars` to track variables referenced by `INIT()` calls. Dependency extraction uses two calls to `classify_dependencies()` (one for the dt AST, one for the init AST) instead of separate walker functions. `parse_source_variable_with_module_context` is the sole parse entry point (the non-module-context variant was removed). `variable_relevant_dimensions` provides dimension-granularity invalidation: scalar variables produce an empty dimension set so dimension changes never invalidate their parse results. `compile_var_fragment` is the salsa-tracked per-variable fragment compiler; its lowering half lives in the sibling `src/db/var_fragment.rs` (`lower_var_fragment`). For a recurrence SCC that `db::dep_graph::resolve_recurrence_sccs` resolved (element-acyclic), the per-element symbolic segments of every member are interleaved into ONE combined `PerVarBytecodes` along the SCC's `element_order` by `combine_scc_fragment` (the per-element-granular generalization of `compiler::symbolic::concatenate_fragments`), built from each member's exact production symbolic fragment via `var_phase_symbolic_fragment_prod(.., scc.phase)` (loud-safe: an unsourceable member or a segmentation error returns `None`/`Err` and the model keeps its `CircularDependency`). `assemble_module` skips each member's per-variable fragment and injects this combined fragment at the first member's runlist slot (the dt fragment into the flow fragments, the init fragment as one synthetic-ident `SymbolicCompiledInitial`); per-write `SymVarRef` identity is preserved, so variable layout offsets are unchanged and each member stays individually addressable. +- **`src/db.rs`** - The module ROOT of the salsa pipeline. After the db-submodule split it holds only the database itself (`SimlinDb` + its `Db` trait, the sync/staged/restore/`current_source_project` methods, the `StdlibModels` cache, the `impl salsa::Database`/`Db`), the `compile_project_incremental()` production entry point, the LTM-glue surface (`LtmSyntheticVar`/`LtmVariablesResult` result types + the `module_link_score_equation` module-link helper (composite reference / ceteris-paribus / signed `black_box_unit_transfer_equation` fallback) called by the per-shape `link_score_equation_text_shaped` (which lives in `src/db/ltm/compile.rs`) -- since Track A the SOLE derivation of a link score's equation text: the former `(from, to)`-keyed `link_score_equation_text` twin was deleted and assembly's standard scalar Bare score now sources the shaped query directly (`compile_ltm_var_fragment`), so the compiled and reported equations are one value and cannot drift; `module_composite_ports` reads the sub-model's `model_ltm_variables` to decide whether a port has a composite, so the same composite reference is used in exhaustive AND discovery mode)/`find_model_output_ports_for_module`/`module_input_pathways_from_edges`/`generate_max_abs_selection` helpers; the last folds a module input port's "strongest pathway" composite through O(1)-sized accumulator helper variables -- inlining the fold into one nested expression doubles the equation text per pathway, which OOMed on macro modules with hundreds of pathways. Since PR #684 a stockless **passthrough** with input→output pathways ALSO emits pathway/composite vars, and since GH #748 the stock-free early return in `model_ltm_variables` fires only when the model is genuinely STATELESS -- no parent-level stocks, no input ports, and no (transitively) stock-carrying module instance (`modules_carry_state`), so a module-only root whose state lives inside a SMOOTH/DELAY or user sub-model is scored instead of silently emitting nothing, and the exhaustive-mode loop-score builder overrides each `x→m` module link with a **per-exit-port pathway selection** (`compute_module_link_overrides` in `src/db/ltm/mod.rs` emits a `$⁚ltm⁚link_score⁚{x}→{m}⁚via⁚{exit}` alias selecting the pathway the loop actually traverses, threaded into `generate_loop_score_variables` as a `(loop_id, link_index)` side-table) because the composite max-abs-selects across ALL pathways and so picks the wrong output port for a multi-output module; `find_model_output_ports` now sorts its result (GH #680) so the parent's pathway recomputation matches the sub-model's emitted `$⁚ltm⁚path⁚{port}⁚{idx}` indices, the residual being that pinned loops pass an empty override map), the `set_project_ltm_enabled`/`set_project_ltm_discovery_mode` setters plus the `LtmEnabledGuard` RAII scope guard (transiently flips `ltm_enabled` and unconditionally restores it on drop; shared by libsimlin's `simlin_project_get_errors` / from-wasm FFIs (GH #466) and `simlin-mcp-core`'s `read_model`/`edit_model` diagnostic passes (GH #662) so the LTM-only advisory harvest behaves identically across every diagnostic-collection surface), and the `#[cfg(test)]` test-module mounts. Everything else moved into per-concern db submodules (`input`/`query`/`stages`/`sync`/`diagnostic`/`layout`/`var_fragment`/`fragment_compile`/`assemble`/`dep_graph`/`analysis`/`ltm`/...) and is re-exported at the root so the historical `simlin_engine::db::...` surface (and the `use super::*` globs in the root-mounted test modules and `implicit_deps.rs`) keep resolving. `SimlinDb` OWNS its sync state (a private `sync_state: Option` field; the `#[salsa::db]` macro finds `storage` by type, and the field is only mutated via `&mut self` during sync, never during parallel query execution, so no interior mutability is needed), so incrementality is automatic: `db.sync(project) -> SourceProject` threads the prior handles internally (a no-op re-sync stays a salsa cache hit), `db.sync_staged(project) -> (SourceProject, Option)` additionally returns the PRE-staging handles for an exact rollback, `db.restore(project, prev)` re-syncs the prior datamodel with those handles, and `db.current_source_project()` reads the latest handle. A SECOND, `pub(crate)` slot -- `expanded_state: Option`, driven by `db.sync_expanded(expanded_datamodel)` -- holds the conveyor/queue-expanded twin of the project so the special-stock build path is incremental too (see "Conveyors and queues"); it stays `None` until a special-stock model is first built, is never cleared thereafter (salsa cannot reclaim inputs, so clearing would only force a second set to be minted), and its handle is reachable only as `sync_expanded`'s return value, which is what keeps a rolled-back staged patch from poisoning it. `sync_from_datamodel` (fresh `&db` path returning `SyncResult`) and `sync_from_datamodel_incremental` stay public as the internal mechanism the methods wrap and for the read-only test path; `collect_all_diagnostics(db, project: SourceProject)` iterates `project.models(db)`. `SourceProject` carries `ltm_enabled` and `ltm_discovery_mode` flags for LTM compilation, plus `macro_declarations: Vec<(String, Option)>` -- the ordered, pre-dedup list of project models' canonical names + macro specs that the demand-driven `project_macro_registry` query reads to re-derive the macro build error (the canonical-name-keyed `models` map collapses the duplicate/colliding model names the AC5.3 validation needs, so the ordered declaration list supplies them). `SourceModel` carries `macro_spec: Option` so `project_macro_registry` is keyed only on the macro-marked models. `collect_module_idents`/`equation_is_module_call` now consult the `MacroRegistry` so a `y = MYMACRO(...)` caller is pre-classified as module-backed (correct `PREVIOUS`/`INIT` rewrite); a macro-body variable's `enclosing_model` is the macro name so a renamed `init`/`previous` builtin inside a like-named macro resolves to the intrinsic (#554). `Diagnostic` includes a `severity` field (`Error`/`Warning`) and `DiagnosticError` variants: `Equation`, `Model`, `Unit`, `Assembly`. The salsa inputs store the `datamodel::*` types directly on their multi-field structs (`SourceProject.sim_specs`/`dimensions`/`units`, `SourceModel.model_sim_specs`, `SourceVariable.equation`/`gf`/`module_refs`) -- per-field salsa read tracking is what gives fine-grained invalidation, so no mirror-projection types are needed; `datamodel_variable_from_source` re-assembles the kind-tagged `datamodel::Variable` for the parser. `datamodel::Equation::Arrayed` carries `has_except_default` to drive EXCEPT default application. `VariableDeps` includes `init_referenced_vars` to track variables referenced by `INIT()` calls. Dependency extraction uses two calls to `classify_dependencies()` (one for the dt AST, one for the init AST) instead of separate walker functions. `parse_source_variable_with_module_context` is the sole parse entry point (the non-module-context variant was removed). `variable_relevant_dimensions` provides dimension-granularity invalidation: scalar variables produce an empty dimension set so dimension changes never invalidate their parse results. `compile_var_fragment` is the salsa-tracked per-variable fragment compiler; its lowering half lives in the sibling `src/db/var_fragment.rs` (`lower_var_fragment`). For a recurrence SCC that `db::dep_graph::resolve_recurrence_sccs` resolved (element-acyclic), the per-element symbolic segments of every member are interleaved into ONE combined `PerVarBytecodes` along the SCC's `element_order` by `combine_scc_fragment` (the per-element-granular generalization of `compiler::symbolic::concatenate_fragments`), built from each member's exact production symbolic fragment via `var_phase_symbolic_fragment_prod(.., scc.phase)` (loud-safe: an unsourceable member or a segmentation error returns `None`/`Err` and the model keeps its `CircularDependency`). `assemble_module` skips each member's per-variable fragment and injects this combined fragment at the first member's runlist slot (the dt fragment into the flow fragments, the init fragment as one synthetic-ident `SymbolicCompiledInitial`); per-write `SymVarRef` identity is preserved, so variable layout offsets are unchanged and each member stays individually addressable. - **`src/db/input.rs`** (a `db` submodule) - The salsa INPUT layer: the interned key types (`LtmLinkId`/`ModuleIdentContext`/`ModuleInputSet`, the last with its `empty`/`from_canonical_set`/`from_names`/`canonical_input_set` round-trip), the `SourceVariableKind` tag, the three `#[salsa::input]` structs (`SourceProject`/`SourceModel`/`SourceVariable`) that store the synced datamodel field-by-field for fine-grained invalidation, the `PinnedLoopSpec` projection of a non-deleted `LoopMetadata` (a pinned loop's name + canonical variable set, carried on `SourceModel.pinned_loops`; the `LoopMetadata` UIDs are resolved to canonical names at sync time since UIDs never enter the db), the `source_var_is_table_only` lookup-only predicate, `SourceModel.declared_variable_idents` (the ordered, pre-dedup AS-WRITTEN variable-ident list -- the raw data the GH #885 duplicate-canonical-ident check needs, which the canonical-keyed `variables` map collapses; the per-variable analogue of `SourceProject.macro_declarations`), and `datamodel_variable_from_source` (re-assembles the kind-tagged `datamodel::Variable` for the parser). Re-exported at the `db.rs` root. -- **`src/db/query.rs`** (a `db` submodule) - The demand-driven read queries over the inputs: per-variable parsing (`parse_source_variable_with_module_context` + `_impl`), the project-global cached contexts (`project_units_context`, `project_datamodel_dims`, `project_dimensions_context`, `project_converted_dimensions`), the module-ident context (`model_module_ident_context`), the per-variable direct-dependency extraction (`variable_direct_dependencies` + `_impl`, the `VariableDeps` value, the `canonical_module_input_set` helper), the implicit-variable metadata (`ImplicitVarMeta`/`model_implicit_var_info`), the recursive `model_module_map`, and the dimension-read queries (`variable_relevant_dimensions`/`variable_dimensions`/`variable_size`). `variable_relevant_dimensions` gives dimension-granularity invalidation (scalars produce an empty set, so dimension changes never invalidate them). +- **`src/db/query.rs`** (a `db` submodule) - The demand-driven read queries over the inputs: per-variable parsing (`parse_source_variable_with_module_context` + `_impl`), the project-global cached contexts (`project_units_context_result` -- carrying the units `Context` AND the `definition_errors` building it produced, with `project_units_context` a salsa PROJECTION over its `ctx` -- not a plain accessor, since a reader of one would take a dependency on the whole result and rebuild whenever only the errors moved, which is every intermediate state of typing a malformed unit equation; the projection backdates on the equal `Context` and keeps readers' dependencies exactly as narrow as they were. The errors ride the RESULT rather than a salsa accumulator because a bad unit declaration is a project-level fact that an accumulator both reported once per model and lost entirely after an unrelated revision bump -- plus `project_datamodel_dims`, `project_dimensions_context`, `project_converted_dimensions`), the module-ident context (`model_module_ident_context`), the per-variable direct-dependency extraction (`variable_direct_dependencies` + `_impl`, the `VariableDeps` value, the `canonical_module_input_set` helper), the implicit-variable metadata (`ImplicitVarMeta`/`model_implicit_var_info`), the recursive `model_module_map`, the explicit-edges-only module-reference graph (`ModuleReferenceGraph`/`project_module_graph`, the cycle gate every compile / diagnostic / analysis entry point consults first so a module cycle is a `CircularDependency` instead of salsa's dependency-graph cycle panic -- GH #806; its rustdoc carries the argument for why omitting the IMPLICIT edges is sound now that `MacroRegistry::build`'s Pass 4 rejects the one shape that could close a cycle through one, plus the enumerated-not-proved residual that residual rests on), and the dimension-read queries (`variable_relevant_dimensions`/`variable_dimensions`/`variable_size`). `variable_relevant_dimensions` gives dimension-granularity invalidation (scalars produce an empty set, so dimension changes never invalidate them). +- **`src/db/stages.rs`** (a `db` submodule) - The two name-keyed, PRE-LAYOUT model-compilation stages as salsa-cached `returns(ref)` queries. This is the ONLY place in the crate they are built from salsa inputs. `Project::from_salsa` used to carry a second inline copy, silently disagreeing on three fields; this body took `from_salsa`'s behaviour in all three, so retiring that copy was a deletion rather than a merge, and `from_salsa` now clones these memos. Three other `ModelStage0`/`ModelStage1` constructions remain, none of them a whole-model salsa build and each deliberate: the per-variable MINI Stage0 in `db::var_fragment::lower_var_fragment` and `db::ltm::compile` (scoped to one variable's dependencies -- pointing them here would add a project-wide dependency edge to every fragment compile), and `db::units::check_conveyor_param_units`' throwaway augmented `ModelStage1::new` over a CLONE of the cached Stage0 (see that module's header). `model_stage0(db, model, project) -> ModelStage0` parses a model's variables (through the per-variable `parse_source_variable_with_module_context` memos) plus the implicit SMOOTH/DELAY/TREND helpers those parses synthesize; `model_stage1(db, model, project) -> ModelStage1` lowers that Stage0's equations to `Expr2` via `ModelStage1::new` against a `ScopeStage0` holding the project's dimension context and the Stage0 of every model in `model_scope_models(db, model, project)` -- this model plus the transitive closure of the models its module variables instantiate. Both were previously rebuilt from scratch by each consumer -- `db::units::check_model_units` is tracked PER MODEL yet rebuilt both stages for EVERY project model on each call, making whole-project unit diagnostics quadratic in the model count (GH #966); the unit pass now READS these. Two decisions live here rather than at a call site, both currently inert in the stage VALUE and therefore pinned directly by `stages_tests`: `model_is_stdlib` marks a model implicit only when the `stdlib⁚` prefix is followed by a name in `stdlib::MODEL_NAMES` (a bare-prefix model an import produced is a user model, not a template), and `source_model_is_stdlib` is the canonicalizing handle-level wrapper `db::units`' unit-check skip gate now shares (GH #988) -- that gate used to carry its own looser bare-prefix spelling, so an imported `stdlib⁚` model silently lost unit checking while the stage query staged it as an ordinary user model, and `extra_module_idents` adds EVERY variable name of a stdlib model to its `ModuleIdentContext` so `PREVIOUS(module_input)` inside a stdlib body would rewrite through a scalar helper aux (no shipped stdlib body calls PREVIOUS/INIT, so this changes no parse today -- it exists so every path reaching a stdlib model's parse hits the same interned context and shares one set of parse memos instead of minting a second). Stage0 records duplicate-canonical-ident model errors from the memoized `db::model_duplicate_variables` groups (GH #885/#891); nothing on the unit path reads that field. `model_scope_models` is that closure, as an iterative worklist rather than a recursive tracked query -- a module cycle is a project a user can draw, and recursing a tracked query on one is salsa's unrecoverable dependency-graph panic (GH #806). Its edges come from each model's Stage0 `variables`, so they include the IMPLICIT modules builtin and macro expansion synthesized: a macro call targets the macro's own model, an ordinary user model that can be arrayed, which is why `db::project_module_graph` (explicit edges only, deliberately, to stay parse-free) is the wrong source here. A module's own IDENT is an edge too when it names a project model, because `resolve_relative` predates a module ident differing from its target's name and keys each dotted `src` component as a MODEL name; dropping that edge turns a project that lowered cleanly into one reporting `BadModuleInputSrc`. `check_model_units` reads the SAME closure for its inference map (`units_infer` consults its models map only for module targets, recursing along the same edges), so the two halves cannot disagree about what is reachable. The narrowing is what makes an unrelated model's edit invalidate neither a model's lowered stage nor its unit check, pinned by the execution counters in both directions; the whole-project map cost that, and cost the reads too. `model_scope_stage0` resolves the closure to Stage0s and REPAIRS a missing self entry (a handle that outlived its place in the project's model map), because without it `get_dimensions` returns `None` for every reference in the model and every arrayed equation lowers as though it were scalar; the Stage1 map in `check_model_units` deliberately does NOT repair it, since that absence is its signal that the handle is not the project's model. Three fixtures in `stages_tests` are what can see a narrowing go too far, each catching a different mistake: `arrayed_module_project` (`main` reduces over an ARRAYED output of its DIRECT target `sub_a` -- catches a self-only scope; before it existed, emptying the scope map entirely left every test in the crate green), `chain_project` (`main -> sub_a -> sub_c` with `main` reducing over `sub_a.sub_c.out_by_region`, where `sub_c` is reachable only transitively -- the only thing that catches a "self + direct targets" scope, so the closure must be TRANSITIVE), and `omitting_stdlib_models_from_the_lowering_scope_is_inert_today` (dropping the `stdlib⁚*` models wholesale is harmless for LOWERING, and that test asserts the precise reason -- no stdlib template is arrayed or instantiates a module -- so it fails the moment that stops being true; its module half doubles as the tripwire for the stdlib-is-a-SINK premise of `MacroRegistry::build`'s Pass 4 closure argument, so if it ever reds, the abort-class consequence is that `project_module_graph` no longer sees every cycle; the closure does not take that shortcut, and since the inference map shares it, a `stdlib⁚*`-skipping closure is no longer inert end to end -- it reds `unit_checking_test::test_smth1_unit_mismatch_initial`). Test-only thread-local execution counters (`reset_query_executions`/`query_executions`, covering both stage queries AND `check_model_units`, so one reset covers every counter a test in this area reads) let `stages_tests` count query-body entries: pointer equality of a `returns(ref)` memo cannot prove a body did not run, since salsa backdates a re-executed query whose value compares equal without moving the memo. +- **`src/db/stages_tests.rs`** - Tests for the two stage queries: value oracles against the salsa-free `ModelStage0::new_in_project` (Stage0) and `ModelStage1::new` over a whole-project scope of those (Stage1), including `every_shape_project`, the combined fixture carrying every shape the deleted `Project::from_salsa` copy handled -- an implicit stdlib `SMTH1` expansion, an explicit user sub-model, a macro-marked model AND a caller of it, and duplicate canonical idents; the three Stage0 semantics decisions above; the GH #966 cost claim (a whole-project `collect_all_diagnostics` BUILDS each model's Stage0/Stage1 at most once, and only for the models something reaches -- in a fixture instantiating no stdlib template, the spliced stdlib set is never staged at all, which is the scope narrowing measured; re-reading every model's stages once per model, the pre-#966 access pattern, then rebuilds none) plus its companion `project_from_salsa_reads_the_cached_stages` (`from_salsa` demands each model's stages once through the queries and the unit pass afterwards rebuilds none -- the only evidence that distinguishes reading the memo from building an equal second copy, since the deleted build produced EQUAL values by design); and diagnostic reachability across the move (a unit warning still reaches both `check_model_units::accumulated` and `collect_all_diagnostics`, as does `project_macro_registry`'s `DuplicateMacroName`, which `check_model_units` now reaches ONLY two tracked queries deeper); and the scope narrowing itself -- the closure's shape (transitive, implicit/macro edges included, module-ident edge, finite on a cycle) and its INVALIDATION consequences in both directions, since a scope that is too wide only costs incrementality while one that is too narrow answers from a stale memo. Two of those tests demonstrate a CONSEQUENCE rather than a shape, because a name-list assertion cannot show that losing an edge would mis-lower or mis-diagnose anything. `dropping_a_macro_models_scope_edge_changes_the_lowered_value` is the macro-side twin of `arrayed_module_project`: `main` reduces over an ARRAYED macro output, so removing the macro model from the scope changes the lowered VALUE of exactly that caller, with a SCALAR-output control that changes nothing -- which pins the mechanism as dimension resolution through the macro edge rather than sensitivity to the map's contents. Its fixture guard reads the model's STAGE0 (not its scope) on purpose, so that a macro-edge-dropping narrowing reds it on the value rather than on a structural check. `a_two_hop_cross_module_unit_mismatch_is_still_reported` is the unit guard for the TRANSITIVE row of the matrix (the stdlib row already had `unit_checking_test::test_smth1_unit_mismatch_initial`): a widget/gadget contradiction that closes only after `units_infer` walks BOTH module hops, so a direct-targets-only closure drops the grandchild's constraints and the diagnostic silently disappears. Note what a scope mis-lowering can and cannot corrupt: `model_stage1`'s consumers are unit checking and the test-only `Project::from_salsa`, while simulation compiles from the per-variable fragment path's own mini Stage0s and never reads this stage. The two invalidation tests drive `sync_from_datamodel_incremental`, which REUSES the `SourceProject` handle: a value read through an older `SyncResult` after an edit is the POST-edit value, so every read is made against the sync current at that moment, and each has a control step (re-syncing the identical project must re-execute nothing) so a count is attributable to the edit. Every equality oracle here ranges over USER models only: `ModelStage0` derives `PartialEq`, it compares parsed `f64` constants, and every SMOOTH/DELAY/TREND template declares `initial_value = NAN`, so a stdlib stage carrying a NaN does not compare equal even to ITSELF (GH #987/#981). The one stdlib oracle uses `npv`, the single template with no NaN literal. - **`src/db/sync.rs`** (a `db` submodule) - Datamodel -> salsa-input sync: the `SyncResult`/`SyncedModel`/`SyncedVariable` handle maps, the `Clone`-able `PersistentSyncState`/`PersistentModelState`/`PersistentVariableState` snapshots threaded between sync calls (with `to_sync_result`/`to_synced_model`/`from_sync_result`), the one-shot stdlib-input builder (`build_stdlib_models`, feeding the root's `StdlibModels` cache), the fresh (`sync_from_datamodel`) and incremental (`sync_from_datamodel_incremental`) entry points + their per-variable helpers (`source_variable_from_datamodel`, `update_source_variable`), the `macro_declarations_from_datamodel` extractor, `pinned_loops_from_datamodel` (resolves a model's `loop_metadata` UIDs to canonical variable names for `SourceModel.pinned_loops`), and `expand_maps_to_chains` (the `maps_to`/`mappings` reachability closure the parser uses to size a variable's dimension dependency). -- **`src/db/diagnostic.rs`** (a `db` submodule) - Compilation diagnostics: the `CompilationDiagnostic` salsa accumulator, the typed `Diagnostic` value (`severity` field + `DiagnosticError` variants `Equation`/`Model`/`Unit`/`Assembly`), the per-model `model_all_diagnostics` query that triggers every diagnostic source (`compile_var_fragment` per variable, the unit-check pass, and -- when `ltm_enabled` -- `model_ltm_fragment_diagnostics`), the `model_duplicate_variables` query + `emit_duplicate_variable_diagnostics` (GH #885: an Error-severity `DuplicateVariable` diagnostic per group of variables whose names canonicalize to the same ident, derived from the raw `declared_variable_idents` input; `compile_project_incremental` fails hard on the same query, and `queue_compile::build_compiled` runs the identical datamodel-level check before expansion), and the accumulator-drain helpers `collect_model_diagnostics` (one model) / `collect_all_diagnostics` (whole synced project). +- **`src/db/diagnostic.rs`** (a `db` submodule) - Compilation diagnostics: the `CompilationDiagnostic` salsa accumulator, the typed `Diagnostic` value (`severity` field + `DiagnosticError` variants `Equation`/`Model`/`Unit`/`Assembly`), the per-model `model_all_diagnostics` query that triggers every diagnostic source (`compile_var_fragment` per variable, the unit-check pass, and -- when `ltm_enabled` -- `model_ltm_fragment_diagnostics`), the `model_duplicate_variables` query + `emit_duplicate_variable_diagnostics` (GH #885: an Error-severity `DuplicateVariable` diagnostic per group of variables whose names canonicalize to the same ident, derived from the raw `declared_variable_idents` input; `compile_project_incremental` fails hard on the same query, and `queue_compile::build_compiled` runs the identical datamodel-level check before expansion), and the drain helpers `collect_model_diagnostics` (one model) / `collect_all_diagnostics` (whole synced project). Three diagnostics deliberately bypass the accumulator: `collect_all_diagnostics` emits each directly from a memoized derivation, and their row counts DIFFER. The **macro-registry build error** (`project_macro_registry`) is at most one row per project, naming no model or variable. The **unit-definition errors** (`project_units_context_result`) are one row per declaration that failed to parse -- several are normal -- naming no model but carrying the unit's name as `variable`. The **module-reference cycle** (`project_module_graph`) is one row per MODEL that can reach a cycle, carrying that model's name and skipping its per-model passes; that one is deliberately per-model and must stay so, since a model reaching no cycle is still processed normally and an unrelated draft cycle cannot hide a valid model's diagnostics (GH #806) -- collapsing it to a single project-level row would revert that. It is emitted by `module_cycle_diagnostic`, which `collect_model_diagnostics` consults and `collect_all_diagnostics` inherits by delegating to it. That gate used to be inline in the whole-project loop only, so the per-model entry point -- equally `pub` -- drove a cyclic model's passes into `model_module_map` and salsa's dependency-graph cycle panic; one function for the gate is what keeps the two entry points from disagreeing about whether the same project panics. What the first two share is their ORIGIN, not their count: each is derived once per project and each used to be accumulated from inside its deriving query's body, which both over-reported (every model's subtree reaches the query, so the DFS found the one value once per model) and lost the diagnostic entirely once an unrelated revision bump let the DFS prune the subtree. - **`src/db/layout.rs`** (a `db` submodule) - The salsa-tracked per-model *body* layout query `compute_layout` (offsets from 0; the root's `IMPLICIT_VAR_COUNT` shift is applied later at assembly via `VariableLayout::root_shifted`). Lays out explicit variables, implicit (SMOOTH/DELAY/TREND) helpers, and -- when `ltm_enabled` -- the LTM synthetic variables and their implicit helpers. - **`src/db/fragment_compile.rs`** (a `db` submodule) - The *emission half* of per-variable compilation (the *lowering half* is the sibling `src/db/var_fragment.rs`). `compile_var_fragment` is the salsa-tracked per-variable fragment compiler; `compile_implicit_var_fragment`/`compile_implicit_var_phase_bytecodes` do the same for the implicit (SMOOTH/DELAY/TREND) helpers, sharing the `lower_implicit_var` parent->implicit->parse->lower prefix. The compile+symbolize tail (`compile_phase_to_per_var_bytecodes`) lives in `src/db/assemble.rs`. - **`src/db/assemble.rs`** (a `db` submodule) - Module/simulation assembly: the table/metadata extraction helpers (`extract_tables_from_source_var`, `build_module_inputs`, `build_stub_variable`, `build_submodel_metadata`), the per-variable compile+symbolize tail (`compile_phase_to_per_var_bytecodes`, the `VarFragmentResult`/`PerVarOffsetMap` values), the production element-graph source `var_phase_symbolic_fragment_prod` (the layout-independent `SymVarRef` substrate the cycle gate's element-order probe consumes), the resolved recurrence-SCC interleaver (`segment_member_by_element` / `combine_scc_fragment`, the per-element-granular generalization of `concatenate_fragments`), the salsa-tracked `assemble_module` / `assemble_simulation`, module-instance enumeration (`enumerate_module_instances`), and the flattened-offset map builder (`calc_flattened_offsets_incremental`). `assemble_module` injects each resolved SCC's combined per-element fragment at the first member's runlist slot (the dt fragment into flows, the init fragment as one synthetic-ident `SymbolicCompiledInitial`), preserving per-write `SymVarRef` identity so layout offsets are unchanged. `build_submodel_metadata` registers not only a sub-model's explicit/implicit source variables but -- when `ltm_enabled` -- its LTM synthetic vars (and their implicit helpers) at their `compute_layout` offsets, so a parent fragment's cross-module reference to a sub-model LTM var resolves the same way the full flattened-offset assembly does. This is what lets the exhaustive-mode input→macro link score (the composite-reference form `"{module}·$⁚ltm⁚composite⁚{port}"`) compile in the standalone fragment compiler (`compile_ltm_equation_fragment`) instead of stubbing to a constant 0 (GH #548): the composite link score through a SMOOTH/DELAY/TREND/NPV macro is the max-magnitude path score through the macro (LTM ref 6.3/6.4), computed by the sub-model's `$⁚ltm⁚composite⁚{port}` aux. - **`src/db/analysis.rs`** - Salsa-tracked causal graph analysis: `model_causal_edges`, `model_loop_circuits`, `model_cycle_partitions`, `model_detected_loops`. Element-level tracked functions: `model_element_causal_edges` (emits per-reference element edges via `emit_edges_for_reference`, driven by the `db::ltm_ir` classification IR: each reference's access shape -- `Bare`, `FixedIndex(elements)`, `PerElement { axes }` (the GH #525 iterated+literal mix, expanded as the diagonal-with-pinned-axes rows from the shared `read_slice_rows` derivation, broadcast over unshared target dims), `Wildcard`, or `DynamicIndex` -- and its `routing` (`Direct` or `ThroughAgg`) come from `model_ltm_reference_sites`; a `ThroughAgg` reference is routed through a synthetic `$⁚ltm⁚agg⁚{n}` aggregate node by `emit_agg_routed_edges` -- only the rows the reducer's `read_slice` reads: `source[,,] → agg[]` then `agg[] → to[e]` (`agg.result_dims` drives the fan-out, diagonal/broadcast like `Bare`; an `Iterated` axis's slot coordinate is remapped to the corresponding TARGET-dim element via `ltm_agg::iterated_axis_slot_elements` when the axis is a positionally-mapped pair -- GH #534, identity in the literal case), never the all-pairs N×M cross-product; a whole-extent reducer (all-`Reduced`) degenerates to "every source element → scalar agg → every target element". `emit_agg_routed_edges` derives the agg's result-axis dimensions from `AggNode::result_dims` (resolved against `from` ⌢ `to` dims), so it handles a *scalar* feeder of a (possibly arrayed) hoisted reducer (`scale` in `growth[D1] = SUM(matrix[D1,*] * scale)`): `from_dims.is_empty()` ⇒ emit `from → agg[]` (or the bare `from → agg` when the agg is scalar) rather than the malformed `from[]` node the row-layout machinery would mint. `Direct` `DynamicIndex` references (`arr[i+1]`, ranges, and the not-hoistable dynamic-index reducer carve-out `SUM(pop[idx,*])` -- reclassified from `Wildcard` by the IR) still expand to the conservative cross-product. A `Bare` edge between differently-named but MAPPED dimensions (`x[Region] → target[State]` with a `State→Region` mapping) projects the mapping's element-correspondence DIAGONAL via `expand_same_element` + `DimensionsContext::mapped_element_correspondence` -- one source element per target element, not the `Region × State` cross-product (GH #527) -- WHEN a usable correspondence exists (positional mappings only; explicit element maps are declined since execution resolves positionally, see the helper's gate); an unmapped or element-mapped disjoint-named pair keeps the broadcast), `model_element_loop_circuits` (Johnson's algorithm on element graph; legacy path retained for non-LTM consumers), `model_element_cycle_partitions` (stock-to-stock SCCs at element granularity). Tiered enumeration: `model_edge_shapes` (per-`(from, to)` `BTreeSet` projected from the IR's classified sites) and `model_loop_circuits_tiered` (variable-level Johnson + cycle classification + slow-path-subgraph Johnson, keeping synthetic agg nodes in the slow-path subgraph projection so cross-element loops through a hoisted reducer are recovered) replace the full-element Johnson run for LTM compilation. `classify_cycle` labels each variable-level cycle as `PureScalar`, `PureSameElementA2A`, or `CrossElementOrMixed`; pure cycles emit one Loop directly while cross-element / mixed cycles drive the slow-path subgraph (the induced element subgraph over their nodes). `TieredCircuitsResult.slow_path_largest_scc` exposes the cross-element subgraph's largest SCC for the LTM auto-flip gate. Produces `DetectedLoop` structs with polarity (each carrying an optional `name`: `None` for an enumerated loop, `Some(label)` for a modeler-pinned one, plus a `partition: Option` -- a RESULT-SCOPED dense index into `DetectedLoopsResult.partitions` (reusing the discovery surface's `ltm_finding::DiscoveredPartition`), `None` for a loop with no parent-level stock; same stability caveat as `FoundLoop.partition`, GH #685). `model_detected_loops` builds its exhaustive loop set with the SAME machinery the scored surface uses -- `model_loop_circuits_tiered` feeding `db::ltm::build_loops_from_tiered` (under the shared `cross_agg_loop_budget`) plus the shared `recover_agg_hop_polarities` pass -- so the detected and scored loop sets, polarities, and ids are identical BY CONSTRUCTION for every model shape (GH #746; the runtime id join `reclassify_loops_from_results` / pysimlin `get_relative_loop_score` reads `$⁚ltm⁚loop_score⁚{id}` keyed purely on the id, and the previous variable-level-Johnson + per-agg-splice path only bijected for all-scalar cycles -- arrayed cycles silently joined another loop's series). Cross-element loops therefore surface per element instance with element-subscripted variables, mirroring cross-element pins. It resolves cycle partitions over the ELEMENT-level graph (`model_element_cycle_partitions` -- the same granularity the scored `loop_partitions` and the discovery surface use), and `resolve_loop_partitions` mirrors `ltm_finding::attach_partition_metadata` exactly (first-appearance dense remap, `DiscoveredPartition { stocks, loop_count }`) so the two surfaces report partitions in the same shape AND at the same granularity: the partition stock SETS are a usable cross-surface key for scalar and arrayed models alike (an A2A loop's single `partition` index is its first resolving slot's partition; the scored surface's `loop_partitions[id]` carries the full per-slot vector). It appends the model's *pinned* loops (`db::ltm::model_pinned_loops`, deduped against the enumerated set by canonical variable-cycle rotation; a pin that duplicates an enumerated loop transfers its `name` onto the surviving enumerated `DetectedLoop` rather than being discarded), and pinned loops get a partition too. It consults the SHARED `db::ltm::model_ltm_mode` query for the exhaustive-vs-discovery decision -- the SAME gate `model_ltm_variables` uses -- so the two query surfaces never disagree: in discovery mode (whether the user forced `ltm_discovery_mode`, the variable-level SCC exceeded `MAX_LTM_SCC_NODES`, or the slow-path late-flip fired) it returns *only* the pinned loops, so a pinned loop surfaces through `simlin_analyze_get_loops` even in discovery mode (and is backed by the `loop_score` that mode actually emits). Before this unification `model_detected_loops` gated only on the `causal_graph_with_modules` SCC size and ignored both the user flag and the slow-path flip, so a small user-forced-discovery model dropped the pin entirely. `RefShape` and `emit_edges_for_reference` (plus the element-name expansion helpers) live here; the AST walker / agg-routing decision lives in `src/db/ltm_ir.rs`. - **`src/db/ltm_ir.rs`** (a `db` submodule, a child of `db.rs` kept in its own file purely for the per-file line cap; like `ltm_agg`, it builds on `crate::db`) - The single salsa-tracked place a causal edge's access shape *and* aggregate-node routing are decided. `model_ltm_reference_sites(db, model, project) -> LtmReferenceSitesResult` walks each variable's `Expr2` AST once, consults `enumerate_agg_nodes` (the sole "hoistable maximal reducer" decider), and buckets every `Var`/`Subscript` reference by its `(from, to)` causal edge into a `Vec` (`shape` + `target_element` + `routing ∈ {Direct, ThroughAgg{agg}}`); the byte-identical `route_through_agg = !routed_aggs.is_empty() && in_reducer` decision and the `aggs_in_var(to).filter(is_synthetic && reads from)` filter exist here and nowhere else. `model_element_causal_edges`, `model_edge_shapes`, and `model_ltm_variables` are pure readers. Also hosts the AST-walker helpers moved out of `src/db/analysis.rs` (`collect_reference_sites` / `classify_subscript_shape` / `resolve_literal_index` / `collect_reference_shapes`); `classify_subscript_shape` carries the AC1.4 fix -- a subscript whose indices are *all* `Wildcard`/`StarRange` (the reducer-style whole-extent access, e.g. `SUM(x[*:Dim])`) classifies as `Wildcard` to agree with `enumerate_agg_nodes`'s read-slice hoisting test. `classify_iterated_dim_shape` consumes `ltm_agg::classify_axis_access` per axis (T6 of the shape-expressiveness design; a `Reduced` result is post-filtered to `None` -- a non-reducer reference never collapses an axis): an ALL-`Iterated` subscript -- indices that are exactly the target equation's iterated dimensions, position-matched to the source's declared dims by name or by a positional mapping in EITHER declaration direction (GH #511/#527/#757) -- classifies `Bare` (a same-element-on-shared-dims reference projected via `expand_same_element`); a MIXED `Iterated`+`Pinned` subscript (`pop[Region, young]` inside an A2A-over-`Region` equation, GH #525) classifies `RefShape::PerElement { axes }`, whose element edges are the diagonal-with-pinned-axes rows from the shared `read_slice_rows` derivation (never the cross-product whose phantom circuits carried silent confident loop scores) and whose link scores are per-(row, FULL-target-element) scalars `$⁚ltm⁚link_score⁚{from}[{row}]→{to}[{e}]` (the row a function of `e`: project `e` onto the Iterated axes, fill Pinned with literals -- including the BROADCAST case where the Iterated dims are a strict subset of the target's), emitted by `emit_per_element_link_scores`/`generate_per_element_link_equation` and routed per-circuit by the loop builder's `is_per_element_edge` predicate (keyed on the same `model_edge_shapes` IR projection); an all-`Pinned` subscript falls through to `classify_subscript_shape`'s `FixedIndex` (so every existing Bare/FixedIndex link-score name is untouched). A *partially*-iterated subscript that is a *reducer argument* (`SUM(matrix[D1,*])`, `SUM(pop[NYC,*])`, `SUM(matrix3d[D1,NYC,*])`) is hoisted into a synthetic agg by `enumerate_agg_nodes` (its read slice is statically describable), so the reference is `ThroughAgg`-routed and its (`Wildcard`) shape is ignored; the only `Direct` `Wildcard` reducer references remaining are a *whole-RHS* variable-backed reducer's argument (`total = SUM(population[*])`), which keeps `Wildcard`, a DE-HOISTED array-valued reducer's wildcard arg (`RANK(pop[*], 1)` -- GH #771: RANK never sets `in_reducer`, so the site keeps `Wildcard` and takes the conservative cross-product), and a not-hoistable reducer's argument -- a *dynamic index* (`SUM(pop[idx,*])`, `idx` non-literal) or an ELEMENT-mapped sliced reducer the correspondence declines (GH #756; the positionally-mapped case is hoisted since GH #534 -- both declaration directions since GH #757 -- with the `Iterated` axis carrying the (target, source) dim pair). `model_ltm_reference_sites` reclassifies such a not-hoisted `Direct` `Wildcard` `in_reducer` site as `DynamicIndex` (#514) so the `Wildcard`-shape conservative cross-product never fires from a `Direct` site that *could* have been hoisted (the de-hoisted RANK arg is not hoist-eligible, so it deliberately stays `Wildcard`). (`classify_iterated_dim_shape`'s mapped branch -- a *whole-equation*-iterated subscript like `x[State]` inside `target[State] = x[State] * c`, not a sliced reducer argument -- is a separate path and still classifies as `Bare`.) The mapped case needs a `DimensionsContext` (built from `project_datamodel_dims`), so the IR is recomputed when a dimension's mappings change. - **`src/db/ltm_ir_tests.rs`** - Tests for the reference-site IR: the per-AST-site `(shape, in_reducer)` contract (the `ref_site_*` regression guards, ported from `src/db/analysis.rs`) plus the public `ClassifiedSite` contract -- `(shape, target_element, routing)` per site, the AC1.4 `StarRange` consistency, and the AC1.5 SIZE / scalar-source-reducer `Direct` routing, each cross-checked against `enumerate_agg_nodes`. -- **`src/db/macro_registry.rs`** (a `db` submodule, like `db::ltm_ir`, only to keep `db.rs` under the per-file line cap) - The per-project macro-registry salsa query. `project_macro_registry(db, project) -> MacroRegistryResult` wraps the pure `module_functions::MacroRegistry` (resolver) and exposes the build error. Registry-build *validation* (recursion cycle, duplicate macro name, macro/model name collision) can't be re-derived from the canonical-name-keyed `SourceProject.models` (it collapses duplicate / colliding model names), so the query is demand-driven from two salsa inputs: the ordered pre-dedup `SourceProject.macro_declarations` list (canonical name + `macro_spec`, one entry per project model in declaration order) drives Passes 1-2 (duplicate macro name, macro/model collision), and the macro-marked models' body equations -- read from `models` -- drive Pass 3 (recursion cycle). `build_error_from_inputs` reconstructs a `Vec` in declaration order (canonical name + spec + macro bodies; non-macro bodies empty, which Passes 1-2 don't read and Pass 3 skips) and calls the UNCHANGED `MacroRegistry::build` on it, so the produced typed `(ErrorCode, message)` is byte-identical to building over the original datamodel `Vec`. Because the query reads only macro_declarations + macro-marked models' bodies (it skips `macro_spec.is_none()` models' bodies), an ordinary equation edit does NOT invalidate it. It surfaces the build error as a project-level diagnostic so `compile_project_incremental` fails with a clear message. `enclosing_macro_for_var` resolves a macro-body variable's enclosing-macro name (#554, the renamed-intrinsic precedence). +- **`src/db/macro_registry.rs`** (a `db` submodule, like `db::ltm_ir`, only to keep `db.rs` under the per-file line cap) - The per-project macro-registry salsa query. `project_macro_registry(db, project) -> MacroRegistryResult` wraps the pure `module_functions::MacroRegistry` (resolver) and exposes the build error. Registry-build *validation* (recursion cycle, duplicate macro name, macro/model name collision, a module inside a macro body) can't be re-derived from the canonical-name-keyed `SourceProject.models` (it collapses duplicate / colliding model names), so the query is demand-driven from two salsa inputs: the ordered pre-dedup `SourceProject.macro_declarations` list (canonical name + `macro_spec`, one entry per project model in declaration order) drives Passes 1-2 (duplicate macro name, macro/model collision), and the macro-marked models' bodies -- read from `models` -- drive Pass 3 (recursion cycle, from the equations) and Pass 4 (a `Variable::Module` in a macro body, from the variable kinds). `build_error_from_inputs` reconstructs a `Vec` in declaration order (canonical name + spec + macro bodies; non-macro bodies empty, which Passes 1-2 don't read and Passes 3-4 skip) and calls the UNCHANGED `MacroRegistry::build` on it, so the produced typed `(ErrorCode, message)` is byte-identical to building over the original datamodel `Vec`. Because the query reads only macro_declarations + macro-marked models' bodies (it skips `macro_spec.is_none()` models' bodies), an ordinary equation edit does NOT invalidate it. **On a build failure the returned registry is EMPTY, and that is load-bearing for CYCLE SAFETY, not just for error quality**: `collect_all_diagnostics` runs every model's passes after emitting `build_error`, and `analysis::analyze_model` never reads `build_error` at all, so only the empty registry stops a rejected macro's call sites from re-synthesizing the implicit module edge `db::project_module_graph` cannot see (a process abort under `panic=abort`). A "keep a partial registry alongside the error" refactor reopens that hole; the argument and its pinning test are on the query's rustdoc. It does NOT accumulate: `build_error` is a plain memoized value that its two consumers read directly -- `compile_project_incremental` (to fail with a clear message) and `collect_all_diagnostics` (to emit the one project-level `Diagnostic`). Accumulating it from inside the query body was wrong twice over: every model's `model_all_diagnostics` subtree reaches this query through `model_module_ident_context`, so the DFS reported N identical copies for an N-model project, and after an unrelated revision bump the deep-verify path recomputed the pruning flags as `Empty` and the diagnostic vanished from the collection entirely. `enclosing_macro_for_var` resolves a macro-body variable's enclosing-macro name (#554, the renamed-intrinsic precedence). - **`src/db/dep_graph.rs`** (a `db` submodule, like `db::ltm_ir` / `db::macro_registry`, only to keep `db.rs` under the per-file line cap) - Owns the **model dependency-graph cycle gate** and its shared relations. The production gate `model_dependency_graph_impl` lives here (the transitive-closure DFS `compute_transitive`/`compute_inner`; the SCC-aware back-edge break `same_resolved_scc`; the SCC-as-collapsed-node transitive accumulation -- every member of a resolved recurrence SCC ends with the identical member-free union of the SCC's *external* successors so the topo sort never re-sees the intra-SCC cycle; and the SCC-contiguous topological runlist sort `topo_sort_str` so a resolved SCC's members are emitted as one byte-stable contiguous block at the SCC's slot for Phase-2-Task-6 combined-fragment injection). **Deterministic initials runlist**: the init set is a `HashSet`, so `model_dependency_graph_impl` sorts it (`init_list.sort_unstable()`) before `topo_sort_str` -- `topo_sort_str` breaks ties (variables with no ordering edge between them) by visit order, so without the sort the same model compiled twice produced different init orderings and therefore different initial values for any unordered pair (GH #595 tracks the deeper soundness gap; the Flows/Stocks runlists are already deterministic by filtering the pre-sorted `var_names`). **dt stock-submodel-output chain-break**: `build_var_info` filters a `submodel·subvar` dt dependency whose `subvar` is a Stock out of the dt phase (a stock breaks the chain, read from the prior timestep) for **every** reader -- not just module-kind variables -- mirroring the legacy `model.rs::module_output_deps` gate; a non-module reader of a stock submodel output (e.g. `v = SMOOTH(...)·output` where the output is an INTEG stock) would otherwise gain a spurious same-step `reader -> module` edge that `topo_sort_str` breaks arbitrarily, sometimes emitting the module before its input (a stale read each flows step). The init phase keeps the edge (stocks do not break the chain there), so only `dt_deps` are filtered. The dep-graph result types (`SccPhase`, `ResolvedScc`, `ModelDepGraphResult`) and the thin `#[salsa::tracked]` wrapper `crate::db::model_dependency_graph` (keyed on an interned `ModuleInputSet`, empty = the no-inputs case) live here too -- the wrapper delegates straight to `model_dependency_graph_impl` -- and are re-exported at the `db.rs` root. Also holds the single shared **cycle relation** `walk_successors(var_info, name, phase: SccPhase) -> Vec<&Ident>` parameterized by phase: Module/absent ⇒ `[]` in both phases; a Stock ⇒ `[]` in `Dt` only (a stock is a dt sink but NOT an init sink -- the one per-phase difference); otherwise the phase's dep set (`dt_deps` for `Dt`, `initial_deps` for `Initial`) filtered to known targets, with stock-targeted deps dropped in `Dt` only, module-targets kept -- exactly the successor set `compute_inner` iterates per phase. (`SccPhase` is defined here; it already keys `ResolvedScc.phase`/`combine_scc_for_phase`, so the dt/init distinction is the same one this relation makes.) Plus the shared `VarInfo` map builder `build_var_info` (consumed verbatim by `model_dependency_graph_impl` and the `#[cfg(test)]` SCC accessor, so the accessor observes the *exact* `var_info` the engine builds -- never a reconstruction), and the recurrence-SCC element-acyclicity refinement `resolve_recurrence_sccs` / `refine_scc_to_element_verdict` / `symbolic_phase_element_order` (GH #575: the cross-member-comparable symbolic `SymVarRef` element graph; N=1 self-recurrence is just the 1-member case). Defining the relation once and using it in both the gate and the accessor makes the accessor's relation the engine's relation by construction. Also hosts the `#[cfg(test)]` SCC accessor: `dt_cycle_sccs` (uncapped `crate::ltm::scc_components` Tarjan over the `walk_successors(.., SccPhase::Dt)` adjacency → `DtCycleSccs { multi, self_loops }`, sorted/byte-stable), the pure `dt_cycle_sccs_consistency_violation` predicate (functional core), and `dt_cycle_sccs_engine_consistent` (imperative shell -- cross-checks the instrumented SCC set against the engine's real `CircularDependency` flagging on the same compiled model, panics on divergence), plus `array_producing_vars` / `var_noninitial_lowered_exprs` (the set of variables whose engine-lowered non-initial exprs contain an array-producing builtin, over the same `build_var_info` universe). - **`src/db/dep_graph_tests.rs`** - Tests for the dt- *and init*-phase cycle-relation primitives, the `#[cfg(test)]` SCC accessor, the recurrence-SCC element-acyclicity verdict, and the multi-member symbolic builder, in their own file alongside the production code to keep `db.rs`/`src/db/tests.rs` under the per-file line cap: the dt-phase `walk_successors` invariant per node kind (Stock/Module/Aux/absent, BTreeSet-sorted order), `dt_cycle_sccs` integration (clean DAG / two-node cycle / self-loop, each via the consistency-checked accessor), byte-stability, the pure consistency predicate in both divergence directions plus all consistent pairings, and `array_producing_vars` membership over the four positive/negative cases. Phase 2 adds: the init-phase `walk_successors` invariant (a stock is NOT an init sink, modules omitted, unknown-dep filtering, BTreeSet-sorted order) and the init-phase relation/verdict tests (`resolve_init_*` / `init_recurrence_behind_stock_*` / `two_stock_init_*` -- a stock-broken dt chain with a forward init element recurrence resolves init-only, a genuine init element cycle stays `CircularDependency`, and a both-relations aux self-recurrence is not double-resolved as a separate init SCC); the multi-member symbolic-builder / GH #575 regression guards (`resolve_dt_two_member_ref_shaped_scc_resolves_interleaved`, the genuine multi-variable element 2-cycle / scalar 2-cycle staying `Unresolved` via the symbolic builder, the N=1 self-recurrence remaining byte-identical to Phase 1, an unsourceable member ⇒ `Unresolved` with no panic, and the `var_phase_symbolic_fragment_prod` no-panic contract); and the combined-fragment preconditions (`model_dep_graph_*` -- a resolved multi-member SCC survives the dependency graph with `has_cycle == false`, members carry the SCC's external deps, and the SCC's members are emitted as one byte-stable contiguous runlist block even with an interposing external variable, so Task 6's combined-fragment injection lands in correct relative order). - **`src/db/var_fragment.rs`** (a `db` submodule, like `db::dep_graph` / `db::ltm_ir` / `db::macro_registry`, only to keep `db.rs` under the per-file line cap) - The *lowering* half of per-variable compilation. `lower_var_fragment` parses the source variable, lowers its equation, builds the minimal symbol-table/metadata `Context`, and runs `compiler::Var::new` per phase to yield the lowered `Vec`; the bytecode-emission half (`compile_phase`) stays with the salsa-tracked caller `crate::db::compile_var_fragment`. The split exists because the lowered `Vec` (which does not implement `salsa::Update`) is the reuse surface the cycle-gate's element-order probe needs the engine's *own* production lowering of (no reconstruction), and a plain function cannot accumulate salsa diagnostics -- so diagnostics are returned as data (`LoweredVarFragment`, with `Fatal` aborting the variable and a per-phase `Err` dropping only that phase) and replayed by the caller. @@ -164,7 +166,7 @@ The unit subsystem is partial-result throughout: a single bad declaration or one - **`src/units.rs`** - Unit parsing and `UnitMap` representation. `Context::new`/`new_with_builtins` return `(Context, Vec<(unit_name, errors)>)`: the context always holds every *valid* declaration, with conflicting/duplicate ones reported alongside -- never an empty context (which would lose project-wide alias normalization like yr/year and re-create a spurious mismatch flood). - **`src/units_check.rs`** - Dimensional consistency checking across equations. A reference's units are `declared OR inferred`; unknown units are skipped (not an error). RANK is dimensionless (an ordinal index, not the ranked array's units). -- **`src/units_infer.rs`** - Hindley-Milner-style unit inference: constraint generation (`gen_constraints`, total -- returns `Units`, no fallible `Result`) + unification over the free abelian group of units (`unify`/`solve_for`/`substitute`). `infer` returns `InferenceResult { resolved, conflicts }` and keeps solving past a conflict (keeping the first binding -- a contradiction is confined to its connected component, since substitution only flows along shared metavariables), collecting *every* residual contradiction via `find_constraint_mismatches`. A macro body's declared units may name the formal parameters (a polymorphic Vensim idiom, e.g. `~ xfrom`, or a parameter ratio such as xfrom over tstart, inside RAMP FROM TO). `gen_all_constraints` LOWERS each parameter-named unit identifier to that parameter's per-instantiation metavariable (`lower_macro_unit_to_metavars`, gated on `ModelStage1::is_macro`/`macro_params`), so it resolves to the actual argument units at each instantiation instead of leaking the parameter name as a literal base unit -- while genuine base units (`dmnl`) are kept and still checked. This both *resolves* parameter-named units (GH #619 point 1) and *checks* a macro body's declared signature against its equations (point 2), superseding the GH #618 skip-entirely containment (which neither resolved nor checked them). A conflict that involves a synthetic module/macro instantiation is rewritten by `clarify_macro_conflict` into a plain-language diagnostic naming the function and the using variable (parsed from the `$⁚{var}⁚{n}⁚{func}` synthetic name by `synthetic_owner_and_func`) rather than synthetic-name/metavariable text -- end users are modelers, not software developers (this also cleans up stdlib-module conflict messages). The cross-module parameter bindings + per-instantiation prefix already monomorphize the macro body, so the RAMP FROM TO storm GH #618 contained does not return. +- **`src/units_infer.rs`** - Hindley-Milner-style unit inference: constraint generation (`gen_constraints`, total -- returns `Units`, no fallible `Result`) + unification over the free abelian group of units (`unify`/`solve_for`/`substitute`). `infer` returns `InferenceResult { resolved, conflicts }` and keeps solving past a conflict (keeping the first binding -- a contradiction is confined to its connected component, since substitution only flows along shared metavariables), collecting *every* residual contradiction via `find_constraint_mismatches`. A macro body's declared units may name the formal parameters (a polymorphic Vensim idiom, e.g. `~ xfrom`, or a parameter ratio such as xfrom over tstart, inside RAMP FROM TO). `gen_all_constraints` LOWERS each parameter-named unit identifier to that parameter's per-instantiation metavariable (`lower_macro_unit_to_metavars`, gated on `ModelStage1::is_macro`/`macro_params`), so it resolves to the actual argument units at each instantiation instead of leaking the parameter name as a literal base unit -- while genuine base units (`dmnl`) are kept and still checked. This both *resolves* parameter-named units (GH #619 point 1) and *checks* a macro body's declared signature against its equations (point 2), superseding the GH #618 skip-entirely containment (which neither resolved nor checked them). A conflict that involves a synthetic module/macro instantiation is rewritten by `clarify_macro_conflict` into a plain-language diagnostic naming the function and the using variable (parsed from the `$⁚{var}⁚{n}⁚{func}` synthetic name by `synthetic_owner_and_func`) rather than synthetic-name/metavariable text -- end users are modelers, not software developers (this also cleans up stdlib-module conflict messages). The cross-module parameter bindings + per-instantiation prefix already monomorphize the macro body, so the RAMP FROM TO storm GH #618 contained does not return. `gen_all_constraints` recurses through every module instantiation, and the module graph is a graph, not a tree, so it threads an `InstantiationPath` -- the models being walked on the CURRENT path, as a cons list whose entries live exactly as long as their stack frames. An edge back to a model already on the path is DECLINED, body and input constraints together, so a module cycle degrades to a partial result instead of overflowing the stack -- which, unlike a panic, aborts the whole `panic=abort` host process. Dropping the input constraint has a KNOWN COST, deliberately paid: the callee-side metavariable is not necessarily dead, since a parent equation reading `{module}·{var}` emits that same metavariable, so a genuine cross-module dimensional conflict inside a cycle goes UNREPORTED. That is accepted because the project is already rejected as `CircularDependency` and a unit conflict on a model that cannot compile is noise -- and `back_edge_declines_a_real_cross_module_conflict` builds the shape and pins the silence, so the trade is documented rather than silent. The path is deliberately not a visited-ANYWHERE set: in a diamond (`a` instantiates `b` and `c`, both of which instantiate `d`) `d` really is instantiated twice under two prefixes and both instantiations must be constrained; the diamond tests, not the cycle tests, are what pin that. Depth is bounded by the model count but the number of instantiation prefixes is not (`k` models each instantiating the next twice reaches the last `2^k` times, all legal) -- the guard makes the walk finite, not cheap. The cycle gets no unit diagnostic of its own -- `project_module_graph` already reports it as a `CircularDependency`. ## Special features diff --git a/src/simlin-engine/src/common.rs b/src/simlin-engine/src/common.rs index dbe8f14e5..0d46b3adb 100644 --- a/src/simlin-engine/src/common.rs +++ b/src/simlin-engine/src/common.rs @@ -579,6 +579,35 @@ pub enum ErrorCode { /// key and silently DROPS an unmatched one, so a one-character typo /// simulates plausibly-but-wrong with no signal. Warning-level (GH #905). UnknownElementSubscript, + /// A macro-marked model's body instantiates a module (`Variable::Module`). + /// A cycle-safety rule, not a taste rule. `db::project_module_graph` -- the + /// gate every compile, diagnostic, and analysis entry point consults so a + /// module cycle surfaces as [`ErrorCode::CircularDependency`] rather than as + /// salsa's dependency-graph cycle panic -- records only EXPLICIT module + /// edges, because it must stay parse-free. A macro CALL is an implicit edge + /// it cannot see, so a macro whose body holds an explicit module targeting a + /// model that calls that macro closes a cycle the gate reports as absent, and + /// the recursive queries abort on it (fatal under `panic = "abort"`). + /// Rejected at `MacroRegistry::build`, which restores the invariant that + /// every module cycle lies in explicit edges. + /// + /// The cost is real and accepted, not zero. The shape is reachable from an + /// ordinary XMILE file -- the `` content model is shared with + /// `` and the reader passes a `` through unfiltered -- our own + /// XMILE writer round-trips it, and a project containing an ACYCLIC one + /// compiles and simulates correctly today. It is rejected anyway because + /// narrowing to only-when-cyclic would need a second reachability analysis + /// whose back edge (the macro call) is only discoverable by parsing every + /// model's equations, and because a macro is a template that has no business + /// instantiating a sub-model. The MDL importer cannot produce it, though not + /// because "Vensim macros have no modules" -- its multi-output materializer + /// does mint modules; the scoped macro-body context just never runs it. + /// + /// Distinct from `CircularDependency` on purpose -- the rejection covers the + /// acyclic case too, which is not a cycle and must not claim to be one. A + /// module *targeting* a macro model is unaffected; only a module *inside* one + /// is rejected. See `MacroRegistry::build`'s Pass 4 for the full argument. + MacroContainsModule, } impl fmt::Display for ErrorCode { @@ -665,6 +694,7 @@ impl fmt::Display for ErrorCode { StockBothConveyorAndQueue => "stock_both_conveyor_and_queue", ConveyorInitListUnsupported => "conveyor_init_list_unsupported", UnknownElementSubscript => "unknown_element_subscript", + MacroContainsModule => "macro_contains_module", }; write!(f, "{name}") @@ -944,11 +974,11 @@ where /// /// This is the `ModelStage0`-construction twin of the salsa-layer gate /// (`compile_project_incremental` / `emit_duplicate_variable_diagnostics`, -/// GH #885): the monolithic constructors and `Project::from_salsa` collapse -/// variables into a canonical-keyed map last-wins, so callers seed the -/// model's error list with this result instead of silently building a -/// different model than the one declared. The message text is shared via -/// [`duplicate_variable_message`], so every surface reports identically. +/// GH #885): every `ModelStage0` constructor collapses variables into a +/// canonical-keyed map last-wins, so callers seed the model's error list with +/// this result instead of silently building a different model than the one +/// declared. The message text is shared via [`duplicate_variable_message`], so +/// every surface reports identically. pub(crate) fn duplicate_variable_errors_from_groups( model_name: &str, groups: &[(String, Vec)], @@ -972,9 +1002,12 @@ pub(crate) fn duplicate_variable_errors_from_groups( /// [`duplicate_variable_errors_from_groups`] over a raw declared-ident list: /// groups the idents by canonical form first. Used by the datamodel-driven -/// `ModelStage0` constructors -- which are themselves `#[cfg(test)]`, hence -/// the gate here -- while the salsa-driven path (`Project::from_salsa`) feeds -/// the memoized `db::model_duplicate_variables` groups directly. +/// `ModelStage0` constructor -- itself `#[cfg(test)]`, hence the gate here -- +/// while the salsa-driven path (`db::stages::model_stage0`, which every other +/// consumer including `Project::from_salsa` now reads) feeds the memoized +/// `db::model_duplicate_variables` groups directly. The two routes to the same +/// error list are what `model::test_stage0_records_duplicate_variable_error` +/// cross-checks. #[cfg(test)] pub(crate) fn duplicate_variable_errors<'a, I>(model_name: &str, idents: I) -> Option> where diff --git a/src/simlin-engine/src/db.rs b/src/simlin-engine/src/db.rs index 97e178a8f..cb70bdfdb 100644 --- a/src/simlin-engine/src/db.rs +++ b/src/simlin-engine/src/db.rs @@ -34,6 +34,7 @@ use std::collections::BTreeSet; // * `assemble` -- module/simulation assembly + flattened-offset map. // * `dep_graph` -- the dependency-graph cycle gate + its result types. // * `analysis` -- causal-graph analysis tracked functions. +// * `stages` -- the two cached model-compilation stages (Stage0/Stage1). // * `ltm` / `ltm_ir` / `macro_registry` / `units` -- LTM (a `ltm/` directory: // mod/parse/compile/loops/link_scores), the reference-site IR, the macro // registry, and the unit-check pass. @@ -48,6 +49,16 @@ pub(crate) use invariance::model_flows_invariant; // (`model_ltm_reference_sites`) it compares the Expr0 partial builder against. pub(crate) mod ltm_ir; mod macro_registry; +mod stages; +pub(crate) use stages::{ + model_scope_models, model_scope_stage0, model_stage0, model_stage1, source_model_is_stdlib, +}; +// Test-only: the execution counters for the two stage queries and the unit-check +// pass, so `stages_tests` can prove each model's stages are BUILT at most once +// per revision (GH #966), and that an unrelated model's edit re-executes none of +// the three -- claims pointer equality of a `returns(ref)` memo cannot support. +#[cfg(test)] +pub(crate) use stages::{QueryExecutions, query_executions, reset_query_executions}; mod units; mod var_fragment; @@ -68,12 +79,12 @@ pub use input::{ mod query; pub(crate) use query::canonical_module_input_set; pub use query::{ - ImplicitVarMeta, ModuleReferenceGraph, ParsedVariableResult, VariableDeps, + ImplicitVarMeta, ModuleReferenceGraph, ParsedVariableResult, UnitsContextResult, VariableDeps, model_implicit_var_info, model_module_ident_context, model_module_map, parse_source_variable_with_module_context, project_converted_dimensions, project_datamodel_dims, project_dimensions_context, project_module_graph, - project_units_context, variable_dimensions, variable_direct_dependencies, - variable_relevant_dimensions, variable_size, + project_units_context, project_units_context_result, variable_dimensions, + variable_direct_dependencies, variable_relevant_dimensions, variable_size, }; mod sync; @@ -1162,8 +1173,9 @@ pub fn compile_project_incremental( ) -> crate::Result { // An invalid macro set (AC5.2 cycle / AC5.3 duplicate / collision) fails // the project-level compile before per-model processing, uniformly as - // `NotSimulatable` (the build error's own typed code rides the - // diagnostic `project_macro_registry` accumulated -- see that module). + // `NotSimulatable`. The build error's own typed code reaches the diagnostic + // surface separately: `collect_all_diagnostics` reads this same memoized + // `build_error` and emits one project-level `Diagnostic` from it. if let Some((_code, msg)) = &crate::db::macro_registry::project_macro_registry(db, project).build_error { @@ -1378,6 +1390,8 @@ mod module_wiring_tests; #[cfg(test)] mod prev_init_tests; #[cfg(test)] +mod stages_tests; +#[cfg(test)] mod tests; #[cfg(test)] mod vm_verification_tests; diff --git a/src/simlin-engine/src/db/diagnostic.rs b/src/simlin-engine/src/db/diagnostic.rs index 94596da0f..46ee87cb8 100644 --- a/src/simlin-engine/src/db/diagnostic.rs +++ b/src/simlin-engine/src/db/diagnostic.rs @@ -4,17 +4,45 @@ //! Compilation diagnostics: the salsa `CompilationDiagnostic` accumulator, //! the typed `Diagnostic` value (severity + per-model/per-variable context), -//! the per-model triggering query `model_all_diagnostics`, and the -//! accumulator-drain helpers `collect_model_diagnostics` / -//! `collect_all_diagnostics`. +//! the per-model triggering query `model_all_diagnostics`, and the drain +//! helpers `collect_model_diagnostics` / `collect_all_diagnostics`. //! //! `model_all_diagnostics` is the single per-model query that drives every -//! diagnostic source: it triggers `compile_var_fragment` per variable (the -//! emission half lives in `db.rs`), the unit-check pass, and -- when LTM is -//! enabled -- the LTM fragment-diagnostic pass. The two `collect_*` helpers +//! PER-MODEL diagnostic source: it triggers `compile_var_fragment` per variable +//! (the emission half lives in `db.rs`), the unit-check pass, and -- when LTM +//! is enabled -- the LTM fragment-diagnostic pass. The two `collect_*` helpers //! drain the accumulated `CompilationDiagnostic`s for one model or the whole //! synced project. //! +//! Three diagnostics do NOT come from the accumulator. `collect_all_diagnostics` +//! emits each directly from a memoized derivation, and they differ in how many +//! rows each produces -- do not collapse them into one rule: +//! +//! - the **macro-registry build error** (`project_macro_registry`): at most +//! ONE row per project, naming no model and no variable. A project has one +//! macro set, and it is either valid or not. +//! - the **unit-definition errors** (`project_units_context_result`): one row +//! per declaration that failed to parse, so several are normal. They name no +//! model -- a project has one `units` list, belonging to none of them -- but +//! they carry the offending unit's name as `variable`. +//! - the **module-reference cycle** (`project_module_graph`): one row per +//! MODEL that can reach a cycle, carrying that model's name, and that model's +//! per-model passes are skipped. This one is deliberately per-model and must +//! stay that way: a model reaching no cycle is still processed normally, so +//! an unrelated draft cycle elsewhere in the project cannot hide a valid +//! model's diagnostics (GH #806). Reporting it once project-wide would +//! revert that. +//! +//! What the first two have in common is not their row count but their ORIGIN: +//! each is derived once per project, and each used to be accumulated from inside +//! its deriving query's body. That was wrong twice over. Every model's +//! diagnostic subtree reaches those queries, so the DFS found the single +//! accumulated value once per model and reported N copies; and a value +//! accumulated inside a memo body is only discoverable while the pruning flags +//! along the whole path stay `Any`, so after an unrelated revision bump the +//! deep-verify path recomputed them as `Empty` and the diagnostic vanished from +//! the collection entirely. Reading the memoized value sidesteps both. +//! //! `model_all_diagnostics` performs an untracked read so it always //! re-executes: see the in-body comment for why that is load-bearing for //! diagnostic stability across unrelated salsa revision bumps. Without it, @@ -24,7 +52,13 @@ use super::*; use crate::common::{EquationError, Error, UnitError}; +/// `Debug` is derived rather than left off: every caller that drains this +/// accumulator wants to print the result in an assertion message, and without +/// it each one has to map through `.0.clone()` into the printable `Diagnostic` +/// first. The inner `Diagnostic` is already `Debug`, so the derive costs +/// nothing. #[salsa::accumulator] +#[derive(Debug)] pub struct CompilationDiagnostic(pub Diagnostic); /// A single compilation diagnostic emitted by tracked functions. @@ -838,13 +872,54 @@ pub fn model_module_wiring_diagnostics(db: &dyn Db, model: SourceModel, project: } } +/// The `CircularDependency` diagnostic for a model that can REACH a module +/// cycle, or `None` when its reachable subgraph is acyclic. +/// +/// Driving a reaching model's per-model passes would take `compile_var_fragment` +/// into the recursive `model_module_map` query, which salsa turns into an +/// unrecoverable dependency-graph cycle panic (GH #806). Both diagnostic +/// collectors must therefore consult this FIRST and report the cycle instead of +/// running the passes -- and both must scope it to REACHABILITY, so an +/// unrelated draft cycle elsewhere in the project does not suppress a valid +/// model's own diagnostics. +/// +/// It is one function rather than the same six lines in each collector because +/// the two disagreeing is precisely how this went wrong: the whole-project +/// collector had the gate and the per-model one did not, so the identical +/// project panicked or reported cleanly depending on which `pub` entry point +/// the caller happened to pick. +fn module_cycle_diagnostic( + db: &dyn Db, + project: SourceProject, + model_name: &str, +) -> Option { + project_module_graph(db, project) + .cycle_error_from(model_name) + .map(|(code, message)| Diagnostic { + model: model_name.to_string(), + variable: None, + error: DiagnosticError::Model(Error::new( + crate::common::ErrorKind::Model, + code, + Some(message), + )), + severity: DiagnosticSeverity::Error, + }) +} + /// Collect all `CompilationDiagnostic`s accumulated during /// `model_all_diagnostics` for a single model. +/// +/// A model that reaches a module cycle reports that cycle and nothing else; see +/// [`module_cycle_diagnostic`] for why the passes must not run at all. pub fn collect_model_diagnostics( db: &dyn Db, model: SourceModel, project: SourceProject, ) -> Vec { + if let Some(diagnostic) = module_cycle_diagnostic(db, project, model.name(db)) { + return vec![diagnostic]; + } model_all_diagnostics::accumulated::(db, model, project) .into_iter() .map(|cd| cd.0.clone()) @@ -852,30 +927,59 @@ pub fn collect_model_diagnostics( } /// Collect all diagnostics for every model in a synced project. +/// +/// Project-level failures are emitted here, directly from their memoized +/// derivation, rather than accumulated by whichever tracked query happens to +/// derive them. There is exactly one of each per project, so a per-model +/// accumulator drain would report N copies -- and a value accumulated inside a +/// memo body silently disappears once salsa's accumulator DFS prunes the +/// subtree it lives in (see `db::macro_registry`, and the module-level note +/// above `unit_warning_fixture` in `db/diagnostic_tests.rs`). pub fn collect_all_diagnostics(db: &SimlinDb, project: SourceProject) -> Vec { - let graph = project_module_graph(db, project); - let mut all = Vec::new(); - for (name, source_model) in project.models(db) { - // A model that can REACH a module cycle would drive its per-model passes - // (compile_var_fragment recursing through the submodel) into the salsa - // cycle panic. Report the cycle for that model and skip its passes. A - // model that reaches no cycle is processed normally, so a valid model's - // diagnostics are not hidden by an unrelated draft cycle elsewhere - // (GH #806). - if let Some((code, message)) = graph.cycle_error_from(name) { + + // A unit declaration that failed to parse belongs to the project's `units` + // list, not to any model. Read from the memoized derivation rather than + // drained from the accumulator, for the reasons on `UnitsContextResult`. + for (unit_name, eq_errors) in &project_units_context_result(db, project).definition_errors { + for eq_err in eq_errors { all.push(Diagnostic { - model: name.clone(), - variable: None, - error: DiagnosticError::Model(Error::new( - crate::common::ErrorKind::Model, - code, - Some(message), - )), + model: String::new(), + variable: Some(unit_name.clone()), + error: DiagnosticError::Unit(UnitError::DefinitionError(eq_err.clone(), None)), severity: DiagnosticSeverity::Error, }); - continue; } + } + + // An invalid macro set (recursion cycle / duplicate macro name / macro-model + // collision) is a single project-level fact, carried as a memoized value on + // `project_macro_registry` and read here rather than accumulated there. + // Emitted before the per-model passes so the ordering is deterministic. + if let Some((code, message)) = crate::db::macro_registry::project_macro_registry(db, project) + .build_error + .as_ref() + { + all.push(Diagnostic { + model: String::new(), + variable: None, + error: DiagnosticError::Model(Error::new( + crate::common::ErrorKind::Model, + *code, + Some(message.clone()), + )), + severity: DiagnosticSeverity::Error, + }); + } + + for source_model in project.models(db).values() { + // The module-cycle gate lives in `collect_model_diagnostics` (see + // `module_cycle_diagnostic`): a model that reaches a cycle reports the + // cycle instead of running its passes, and a model that reaches none is + // processed normally -- so an unrelated draft cycle elsewhere in the + // project does not hide a valid model's diagnostics (GH #806). This + // loop used to carry its own copy of that gate, which is how the + // per-model entry point came to be missing it. all.extend(collect_model_diagnostics(db, *source_model, project)); } all diff --git a/src/simlin-engine/src/db/diagnostic_tests.rs b/src/simlin-engine/src/db/diagnostic_tests.rs index a275bf0ea..3d0c097ce 100644 --- a/src/simlin-engine/src/db/diagnostic_tests.rs +++ b/src/simlin-engine/src/db/diagnostic_tests.rs @@ -2677,3 +2677,252 @@ fn test_unknown_element_subscript_warns_on_conveyor_init_list() { "the message must name the unmatched subscript" ); } + +// ---- project-level macro-registry build error ---- +// +// An invalid macro set (a recursion cycle, a duplicate macro name, a +// macro/model name collision) is a PROJECT-level failure: exactly one thing is +// wrong with the project, regardless of how many models it holds. It used to be +// accumulated from inside `project_macro_registry`'s body and discovered only by +// whatever accumulator DFS happened to reach that memo, which made it both +// over-reported (once per model, since every model's `model_all_diagnostics` +// subtree reaches the registry through `model_module_ident_context`) and +// FRAGILE (see the pruning hazard documented above `unit_warning_fixture`: after +// an unrelated revision bump the whole subtree is pruned and the diagnostic +// silently vanishes). It is now emitted once, directly, by +// `collect_all_diagnostics` from the memoized `build_error`. + +/// A project with `n_extra` filler models plus two macros that share a name -- +/// an AC5.3 `DuplicateMacroName` registry-build failure. +fn duplicate_macro_project(n_extra: usize) -> datamodel::Project { + use crate::testutils::{sim_specs_with_units, x_aux, x_model, x_project}; + + let macro_model = |body: &str| { + let mut model = x_model("foo", vec![x_aux("foo", body, None), x_aux("a", "0", None)]); + model.macro_spec = Some(datamodel::MacroSpec { + parameters: vec!["a".to_string()], + primary_output: "foo".to_string(), + additional_outputs: vec![], + }); + model + }; + + let mut models = vec![x_model("main", vec![x_aux("x", "1", None)])]; + for i in 0..n_extra { + models.push(x_model(&format!("filler_{i}"), vec![x_aux("y", "2", None)])); + } + models.push(macro_model("a")); + models.push(macro_model("a + 1")); + x_project(sim_specs_with_units("months"), &models) +} + +/// The registry-build diagnostics in a collected set. +fn macro_build_diagnostics(diags: &[Diagnostic]) -> Vec<&Diagnostic> { + diags + .iter() + .filter(|d| { + matches!(&d.error, DiagnosticError::Model(e) + if e.code == crate::common::ErrorCode::DuplicateMacroName) + }) + .collect() +} + +/// One project-level failure produces exactly ONE diagnostic, however many +/// models the project holds. +#[test] +fn macro_registry_build_error_is_reported_exactly_once() { + let db = SimlinDb::default(); + let project = duplicate_macro_project(4); + let sync = sync_from_datamodel(&db, &project); + + let diags = collect_all_diagnostics(&db, sync.project); + let found = macro_build_diagnostics(&diags); + assert_eq!( + found.len(), + 1, + "a project-level macro-registry failure must be reported once, not once per model; \ + got {} copies: {found:?}", + found.len() + ); + let d = found[0]; + assert_eq!(d.severity, DiagnosticSeverity::Error); + assert!( + d.model.is_empty() && d.variable.is_none(), + "the registry failure is project-level, so it names no model or variable: {d:?}" + ); + + // It reaches the collection as a memoized VALUE read, never through the + // accumulator: no model's per-model drain carries it. That is what makes it + // exactly-once AND immune to the accumulator-DFS pruning below. + for (name, source_model) in sync.project.models(&db) { + let per_model = collect_model_diagnostics(&db, *source_model, sync.project); + assert!( + macro_build_diagnostics(&per_model).is_empty(), + "the project-level registry error must not ride any model's accumulator \ + (model '{name}'): {per_model:?}" + ); + } +} + +/// The registry-build diagnostic survives an unrelated salsa revision bump. +/// +/// This is the failure the accumulator-based emission actually had: bumping +/// `pinned_loops` (the same unrelated input +/// `test_diagnostics_stable_across_unrelated_input_change` uses) let salsa +/// validate `model_all_diagnostics` without re-executing it, the deep-verify +/// path recomputed the DFS pruning flag as `Empty`, and the diagnostic +/// disappeared from the next collection. +#[test] +fn macro_registry_build_error_survives_an_unrelated_input_change() { + use crate::db::PinnedLoopSpec; + use salsa::Setter; + + let mut db = SimlinDb::default(); + let project = duplicate_macro_project(2); + let sync = sync_from_datamodel(&db, &project); + + let before = collect_all_diagnostics(&db, sync.project); + assert_eq!( + macro_build_diagnostics(&before).len(), + 1, + "fixture must produce the registry error to begin with: {before:?}" + ); + + let source_model = sync.models["main"].source; + source_model + .set_pinned_loops(&mut db) + .to(vec![PinnedLoopSpec { + name: "dummy_loop".to_string(), + variables: vec![], + description: String::new(), + }]); + + let after = collect_all_diagnostics(&db, sync.project); + assert_eq!( + macro_build_diagnostics(&after).len(), + 1, + "the registry error must survive an unrelated input change: {after:?}" + ); + assert_eq!( + before, after, + "the full diagnostic set must be identical across an unrelated input change" + ); +} + +// ---- project-level unit-definition errors ---- +// +// The same defect, in the same shape, as the macro-registry build error above: +// a project's `units` list belongs to no model, but the errors from parsing it +// were accumulated inside `project_units_context`'s body, so the DFS found them +// once per model and lost them completely after an unrelated revision bump. +// They now ride `UnitsContextResult::definition_errors` and are emitted once by +// `collect_all_diagnostics`. + +/// A project whose unit declarations conflict: two units claim the same alias +/// `gadget` for different primary names. (Identical duplicate declarations are +/// deliberately tolerated -- Vensim MDL footers repeat `22:` lines -- so a +/// plain duplicate would not produce an error here.) +fn conflicting_unit_alias_project() -> datamodel::Project { + use crate::testutils::{sim_specs_with_units, x_aux, x_model, x_project}; + + let mut dm = x_project( + sim_specs_with_units("years"), + &[ + x_model("main", vec![x_aux("x", "1", None)]), + x_model("other", vec![x_aux("y", "2", None)]), + ], + ); + for name in ["widget", "doodad"] { + dm.units.push(datamodel::Unit { + name: name.to_string(), + equation: None, + disabled: false, + aliases: vec!["gadget".to_string()], + }); + } + dm +} + +/// The unit-definition errors in a collected set. +fn unit_definition_diagnostics(diags: &[Diagnostic]) -> Vec<&Diagnostic> { + diags + .iter() + .filter(|d| { + matches!( + &d.error, + DiagnosticError::Unit(crate::common::UnitError::DefinitionError(_, _)) + ) + }) + .collect() +} + +/// One bad unit declaration produces one diagnostic, however many models the +/// project holds -- and it does not ride any model's accumulator. +#[test] +fn unit_definition_errors_are_reported_exactly_once() { + let db = SimlinDb::default(); + let project = conflicting_unit_alias_project(); + let sync = sync_from_datamodel(&db, &project); + + let diags = collect_all_diagnostics(&db, sync.project); + let found = unit_definition_diagnostics(&diags); + assert_eq!( + found.len(), + 1, + "a conflicting unit alias must be reported once, not once per model; \ + got {} copies: {found:?}", + found.len() + ); + assert!( + found[0].model.is_empty(), + "a unit declaration belongs to the project, not a model: {:?}", + found[0] + ); + + for (name, source_model) in sync.project.models(&db) { + let per_model = collect_model_diagnostics(&db, *source_model, sync.project); + assert!( + unit_definition_diagnostics(&per_model).is_empty(), + "the project-level unit error must not ride model '{name}''s accumulator: \ + {per_model:?}" + ); + } +} + +/// The unit-definition error survives an unrelated salsa revision bump. +#[test] +fn unit_definition_errors_survive_an_unrelated_input_change() { + use crate::db::PinnedLoopSpec; + use salsa::Setter; + + let mut db = SimlinDb::default(); + let project = conflicting_unit_alias_project(); + let sync = sync_from_datamodel(&db, &project); + + let before = collect_all_diagnostics(&db, sync.project); + assert_eq!( + unit_definition_diagnostics(&before).len(), + 1, + "fixture must produce the unit definition error to begin with: {before:?}" + ); + + let source_model = sync.models["main"].source; + source_model + .set_pinned_loops(&mut db) + .to(vec![PinnedLoopSpec { + name: "dummy_loop".to_string(), + variables: vec![], + description: String::new(), + }]); + + let after = collect_all_diagnostics(&db, sync.project); + assert_eq!( + unit_definition_diagnostics(&after).len(), + 1, + "the unit definition error must survive an unrelated input change: {after:?}" + ); + assert_eq!( + before, after, + "the full diagnostic set must be identical across an unrelated input change" + ); +} diff --git a/src/simlin-engine/src/db/dimension_invalidation_tests.rs b/src/simlin-engine/src/db/dimension_invalidation_tests.rs index b67e71335..c4836aa76 100644 --- a/src/simlin-engine/src/db/dimension_invalidation_tests.rs +++ b/src/simlin-engine/src/db/dimension_invalidation_tests.rs @@ -2,7 +2,7 @@ // Use of this source code is governed by the Apache License, // Version 2.0, that can be found in the LICENSE file. -//! Tests for dimension-granularity invalidation (AC8). +//! Tests for input-granularity invalidation (AC8). //! //! Verifies that the per-variable dimension filtering in //! `parse_source_variable_impl` correctly narrows salsa's invalidation @@ -10,6 +10,10 @@ //! variables only depend on their own dimensions (plus transitive //! maps_to targets), and unrelated dimension changes do not trigger //! re-compilation. +//! +//! The same question for the project's UNITS input is at the bottom of the +//! file: a reader of `project_units_context` must not be invalidated by a +//! change that moves only the unit-definition ERROR list. use super::*; use crate::datamodel; @@ -475,3 +479,92 @@ mod expand_maps_to_chains_tests { ); } } + +// ---- units-granularity invalidation ---- + +/// A change that alters ONLY the unit-definition errors must not invalidate +/// readers of `project_units_context`. +/// +/// The two halves of `UnitsContextResult` move independently: a unit whose +/// equation fails to parse is left out of the context entirely, so replacing one +/// malformed equation with a different malformed one changes +/// `definition_errors` while `ctx` stays byte-identical. That is not a contrived +/// case -- it is every intermediate state of typing a unit equation in the +/// editor. +/// +/// `project_units_context` is therefore a salsa PROJECTION over the result, not +/// a plain accessor: it re-executes but backdates on the equal `Context`, so its +/// readers are left alone. With a plain accessor every reader takes a dependency +/// on the whole result and rebuilds on each keystroke -- measured here as +/// `model_stage0` rebuilds, since it reads the context and nothing else that +/// this edit touches. +#[test] +fn unit_definition_error_only_change_does_not_invalidate_context_readers() { + use crate::testutils::{sim_specs_with_units, x_aux, x_model, x_project}; + + let project_with_malformed_unit = |eqn: &str| { + let mut dm = x_project( + sim_specs_with_units("month"), + &[x_model("main", vec![x_aux("x", "1", None)])], + ); + dm.units.push(datamodel::Unit { + name: "bad".to_string(), + equation: Some(eqn.to_string()), + disabled: false, + aliases: vec![], + }); + dm + }; + + let mut db = SimlinDb::default(); + let first = project_with_malformed_unit("widget/"); + let state1 = sync_from_datamodel_incremental(&mut db, &first, None); + let sync1 = state1.to_sync_result(); + let _ = model_stage0(&db, sync1.models["main"].source, sync1.project); + + // Control: re-syncing the identical project rebuilds nothing, so a rebuild + // below is attributable to the edit and not to the re-sync itself. + reset_query_executions(); + let state2 = sync_from_datamodel_incremental(&mut db, &first, Some(&state1)); + let sync2 = state2.to_sync_result(); + let _ = model_stage0(&db, sync2.models["main"].source, sync2.project); + assert_eq!( + query_executions().stage0, + 0, + "re-syncing an unchanged project must not rebuild anything" + ); + + // Snapshot BEFORE the edit: incremental sync reuses the same `SourceProject` + // handle and mutates its fields, so `sync2.project` and `sync3.project` are + // the same salsa input -- reading through it after the edit yields the NEW + // value for both. + let ctx_before = project_units_context(&db, sync2.project).clone(); + let errors_before = project_units_context_result(&db, sync2.project) + .definition_errors + .clone(); + + // A DIFFERENT malformed equation for the same unit: both are rejected. + let second = project_with_malformed_unit("widget * "); + reset_query_executions(); + let state3 = sync_from_datamodel_incremental(&mut db, &second, Some(&state2)); + let sync3 = state3.to_sync_result(); + + assert_eq!( + *project_units_context(&db, sync3.project), + ctx_before, + "the fixture must leave the units context unchanged, or it proves nothing" + ); + let errors_after = &project_units_context_result(&db, sync3.project).definition_errors; + assert_ne!( + errors_after, &errors_before, + "the fixture must change the definition errors, or it proves nothing" + ); + + let _ = model_stage0(&db, sync3.models["main"].source, sync3.project); + assert_eq!( + query_executions().stage0, + 0, + "a change to the unit-definition errors alone must not rebuild a reader \ + of the units context; errors are now {errors_after:?}" + ); +} diff --git a/src/simlin-engine/src/db/macro_registry.rs b/src/simlin-engine/src/db/macro_registry.rs index 71d965e41..8c9057f9e 100644 --- a/src/simlin-engine/src/db/macro_registry.rs +++ b/src/simlin-engine/src/db/macro_registry.rs @@ -18,18 +18,19 @@ //! plus a build-error message). //! //! Registry-build *validation* (macros.AC5.2 recursion cycle, macros.AC5.3 -//! duplicate macro name / macro-model name collision) cannot be re-derived -//! from `SourceProject.models`: that map is keyed by canonical model name, -//! so two same-named macros -- or a macro colliding with a model -- collapse -//! into one entry and become indistinguishable post-sync. The query instead -//! derives the build error on demand from two salsa inputs: the ordered, -//! pre-dedup `SourceProject::macro_declarations` list (canonical name + -//! `macro_spec`, one entry per project model in declaration order) supplies -//! the duplicate/collision data Passes 1-2 need, and the macro-marked -//! models' body equations -- read from `models` -- drive Pass 3's recursion -//! check. It surfaces the result as a project-level diagnostic carrying -//! `MacroRegistry::build`'s own `ErrorCode`, and returns it so -//! `compile_project_incremental` can fail with a clear message. +//! duplicate macro name / macro-model name collision, macros.AC5.7 a module +//! inside a macro body) cannot be re-derived from `SourceProject.models`: that +//! map is keyed by canonical model name, so two same-named macros -- or a macro +//! colliding with a model -- collapse into one entry and become +//! indistinguishable post-sync. The query instead derives the build error on +//! demand from two salsa inputs: the ordered, pre-dedup +//! `SourceProject::macro_declarations` list (canonical name + `macro_spec`, one +//! entry per project model in declaration order) supplies the duplicate/collision +//! data Passes 1-2 need, and the macro-marked models' bodies -- read from +//! `models` -- drive Pass 3's recursion check (equations) and Pass 4's +//! module-in-a-macro check (variable kinds). It surfaces the result as a +//! project-level diagnostic carrying `MacroRegistry::build`'s own `ErrorCode`, +//! and returns it so `compile_project_incremental` can fail with a clear message. //! //! This is a submodule of `db` (a child of `db.rs`, like `ltm_ir`) kept in //! its own file purely to keep `db.rs` under the per-file line cap @@ -38,13 +39,8 @@ use std::collections::HashMap; -use salsa::Accumulator; - use crate::datamodel; -use crate::db::{ - CompilationDiagnostic, Db, Diagnostic, DiagnosticError, DiagnosticSeverity, SourceProject, - SourceVariable, datamodel_variable_from_source, -}; +use crate::db::{Db, SourceProject, SourceVariable, datamodel_variable_from_source}; /// The result of building the per-project macro registry: the (possibly /// empty) resolution registry plus the build error, if any. @@ -61,10 +57,12 @@ use crate::db::{ pub(crate) struct MacroRegistryResult { pub registry: crate::module_functions::MacroRegistry, /// `Some((code, message))` when `MacroRegistry::build` rejected the macro - /// set (duplicate name, macro/model collision, recursion cycle). `code` - /// is `MacroRegistry::build`'s own typed `ErrorCode` -- carried through - /// rather than re-derived from the message prose -- so the diagnostic is - /// tagged with the authoritative code. The registry is then empty. + /// set (duplicate name, macro/model collision, recursion cycle, a module + /// inside a macro body). `code` is `MacroRegistry::build`'s own typed + /// `ErrorCode` -- carried through rather than re-derived from the message + /// prose -- so the diagnostic is tagged with the authoritative code. The + /// registry is then empty, which is REQUIRED, not merely tidy: see the + /// cycle-safety note on [`project_macro_registry`]'s failure return. pub build_error: Option<(crate::common::ErrorCode, String)>, } @@ -96,9 +94,14 @@ pub(crate) struct MacroRegistryResult { /// `find_cycle` sorts roots and uses `BTreeSet` successors, so the reported /// cycle path is independent of model iteration order; declaration order /// here only matters for Passes 1-2. +/// - **Pass 4** (macros.AC5.7: a module inside a macro body) reads the +/// macro-marked models' body variable KINDS + a module's `ident`, both of +/// which the reconstruction carries. It sorts the offending idents, so its +/// message is likewise independent of the `HashMap` +/// iteration order the reconstruction below walks. /// /// Non-macro models get an empty body: Passes 1-2 never read non-macro bodies -/// and Pass 3 skips non-macro models, so their absence cannot change the +/// and Passes 3-4 skip non-macro models, so their absence cannot change the /// result. Returns `None` for a valid macro set, including every macro-free /// project (the build short-circuits when no model carries a `macro_spec`). fn build_error_from_inputs( @@ -172,19 +175,58 @@ fn reconstruct_project_models(db: &dyn Db, project: SourceProject) -> Vec`. When the build fails, the error is accumulated as -/// a project-level `Diagnostic` carrying that exact `ErrorCode` (so it -/// surfaces through `collect_all_diagnostics`, mirroring -/// `project_units_context`) and returned in `build_error` (so the plain -/// `compile_project_incremental` entry can fail with a clear message), and -/// the resolution registry is returned empty so the offending macros' callers -/// are not treated as macro calls -- the compile fails with the registry -/// error, not a confusing cascade. +/// macro-model collision, macros.AC5.7 module inside a macro body) is derived on +/// demand by `build_error_from_inputs`, which reconstructs the project's +/// `datamodel::Model` list (in declaration order, from the `macro_declarations` +/// input plus the macro bodies) and runs the UNCHANGED `MacroRegistry::build` on +/// it -- so its typed `(ErrorCode, message)` is byte-identical to building over +/// the original datamodel `Vec`. When the build fails the error is +/// returned in `build_error`, and the resolution registry is returned empty. +/// +/// # The empty registry on failure is load-bearing for CYCLE SAFETY +/// +/// It reads as an error-quality choice -- the offending macros' callers are not +/// treated as macro calls, so the compile fails with the registry error rather +/// than a confusing cascade. It is more than that, and a refactor that "keeps a +/// partial registry alongside the error" would reopen an abort-class hole. +/// +/// `db::project_module_graph` -- the gate that turns a module cycle into a clean +/// `CircularDependency` instead of salsa's dependency-graph cycle panic -- records +/// only EXPLICIT module edges. A macro CALL is an implicit edge it cannot see. +/// `MacroRegistry::build`'s Pass 4 deletes the one shape that can close a cycle +/// through such an edge (an explicit module inside a macro body, +/// `ErrorCode::MacroContainsModule`), and Pass 3 rejects macro-to-macro recursion. +/// Both are REJECTIONS, and two of the three production entry points do not stop +/// at a rejection: `collect_all_diagnostics` emits `build_error` and then runs +/// every model's passes anyway, and `analysis::analyze_model` does not read +/// `build_error` at all. Only `compile_project_incremental` returns early. +/// +/// What keeps those two from walking the very cycle the build just rejected is +/// this empty registry: with no macro registered, no call resolves as a macro, +/// `builtins_visitor::expand_module_function` synthesizes no implicit module, and +/// the edge does not exist. A registry that retained the rejected macro would +/// restore the edge and abort the host process -- on a project whose macros no +/// longer contain any module at all, since the rejected macro would still be +/// resolvable. Pinned by +/// `db::module_cycle_tests::rejecting_a_macro_empties_the_registry_so_no_implicit_edge_is_synthesized`, +/// which asserts the resulting `UnknownBuiltin` at the call site as the visible +/// proof that no implicit edge was synthesized. +/// +/// This query does NOT accumulate a diagnostic. `build_error` is a plain +/// memoized VALUE that its two consumers read directly: +/// `compile_project_incremental` (to fail with a clear message) and +/// `collect_all_diagnostics` (to emit the project-level `Diagnostic`, once). +/// It used to accumulate here, which was wrong twice over. A registry failure +/// is a PROJECT-level fact, but every model's `model_all_diagnostics` subtree +/// reaches this query through `model_module_ident_context`, so the accumulator +/// DFS found the one accumulated value once per model and `collect_all_diagnostics` +/// reported N identical copies. Worse, a value accumulated inside a memo body is +/// only discoverable while the `accumulated_inputs` flags along the whole path +/// stay `Any`: after an unrelated salsa revision bump the deep-verify path +/// recomputed them as `Empty`, the DFS pruned the subtree, and the diagnostic +/// silently vanished from the next collection entirely (pinned by +/// `db::diagnostic_tests::macro_registry_build_error_survives_an_unrelated_input_change`). +/// Reading the memoized value sidesteps the accumulator, and with it both bugs. /// /// For a *valid* project every model name is unique, so rebuilding the /// resolution map from the (deduplicated) `SourceModel`s here is exact. @@ -195,23 +237,16 @@ pub(crate) fn project_macro_registry(db: &dyn Db, project: SourceProject) -> Mac // AC5.2 cycle, `DuplicateMacroName` for an AC5.3 duplicate / collision), // carried through verbatim from `build`'s `Err` -- not re-derived from the // message prose -- so a future reword of the build error message cannot - // silently mis-tag this diagnostic. + // silently mis-tag the diagnostic `collect_all_diagnostics` builds from it. if let Some((code, message)) = build_error_from_inputs(db, project) { - // Surface through `collect_all_diagnostics` (project-level: no - // specific model/variable). Accumulate directly like - // `project_units_context` -- this query is in the call graph of - // `model_all_diagnostics` via `module_ident_context`. - CompilationDiagnostic(Diagnostic { - model: String::new(), - variable: None, - error: DiagnosticError::Model(crate::common::Error::new( - crate::common::ErrorKind::Model, - code, - Some(message.clone()), - )), - severity: DiagnosticSeverity::Error, - }) - .accumulate(db); + // The EMPTY registry here is required for cycle safety, not just for + // error quality: it is what makes a rejected macro's call sites stop + // resolving as macro calls, so no implicit module edge is synthesized and + // the module cycle the gate cannot see does not exist. Two of the three + // entry points run the per-model passes anyway after a build failure, so + // "keep a partial registry alongside the error" reopens a process abort. + // The full argument is on this function's rustdoc; do not weaken this + // without reading it. return MacroRegistryResult { registry: crate::module_functions::MacroRegistry::default(), build_error: Some((code, message)), @@ -471,6 +506,42 @@ mod tests { assert_eq!(surfaced.0, ErrorCode::DuplicateMacroName); } + /// macros.AC5.7: the Pass 4 rejection must survive the salsa reconstruction. + /// + /// Pass 4 reads something the earlier passes do not: a body variable's KIND + /// and a module's `ident`. `reconstruct_project_models` rebuilds macro bodies + /// from `SourceVariable`s, so this is the only fixture that proves a + /// `Variable::Module` body variable actually round-trips through the salsa + /// inputs -- reconstruct it as an `Aux` (or drop it) and the query would + /// surface no error at all while `MacroRegistry::build` over the original + /// models does, which is exactly what `assert_propagates_build_code`'s + /// comparison catches. + #[test] + fn ac5_7_macro_holding_a_module_propagates_through_the_reconstruction() { + let mut mac = macro_model("mac", &["p1"], "p1 * 2"); + mac.variables + .push(Variable::Module(crate::datamodel::Module { + ident: "u_hop".to_string(), + model_name: "u".to_string(), + documentation: String::new(), + units: None, + references: vec![], + compat: crate::datamodel::Compat::default(), + ai_state: None, + uid: None, + })); + let models = vec![plain_model("u"), mac]; + + assert_propagates_build_code(&models); + let surfaced = build_error_via_query(&x_project(Default::default(), &models)).unwrap(); + assert_eq!(surfaced.0, ErrorCode::MacroContainsModule); + assert!( + surfaced.1.contains("mac") && surfaced.1.contains("u_hop"), + "the rejection must name the macro and its module variable: {:?}", + surfaced.1, + ); + } + /// Non-canonical-name byte-identity proof. Every other oracle fixture uses /// an already-canonical name ("main", "foo", "a"), so it would pass whether /// `reconstruct_project_models` carried the raw or the canonical name. This diff --git a/src/simlin-engine/src/db/module_cycle_tests.rs b/src/simlin-engine/src/db/module_cycle_tests.rs index 0a45c8360..b3351b10c 100644 --- a/src/simlin-engine/src/db/module_cycle_tests.rs +++ b/src/simlin-engine/src/db/module_cycle_tests.rs @@ -11,6 +11,14 @@ //! GH #806. Every production entry point -- compile, diagnostic collection, and //! analysis -- must surface the cycle as a clean `CircularDependency` error //! instead of aborting (a WASM panic plus the recursive-mutex cascade). +//! +//! The gate that does that -- `db::project_module_graph` -- records only EXPLICIT +//! module edges, deliberately, so it stays parse-free. The section at the bottom +//! of this file covers the one shape that could close a cycle through an IMPLICIT +//! (macro-call) edge and so evade the gate entirely. It is fixed by REJECTION +//! rather than by a wider gate (`macros.AC5.7` / +//! `ErrorCode::MacroContainsModule`), which is why those tests assert a rejection +//! and assert that the gate is still blind. use crate::analysis::analyze_model; use crate::common::ErrorCode; @@ -131,16 +139,161 @@ fn mutually_recursive_modules_error_without_panicking() { assert!(analysis.analysis_error.is_some()); } +/// The PER-MODEL diagnostic collector needs the same cycle gate the +/// whole-project one has. `collect_all_diagnostics` consults +/// `project_module_graph` before driving each model's passes; +/// `collect_model_diagnostics` -- equally `pub`, and the shape a future caller +/// would reach for to diagnose one model -- did not, so on a cyclic project it +/// drove `model_all_diagnostics` straight into the recursive `model_module_map` +/// query and salsa raised an unrecoverable dependency-graph cycle panic. Milder +/// than a stack overflow (this one unwinds, so a non-`panic=abort` host could +/// catch it) but the same class: the caller-side gate was the only thing +/// holding it, and only on one of the two callers. +#[test] +fn per_model_diagnostics_report_a_cycle_instead_of_panicking() { + use crate::db::collect_model_diagnostics; + + let mut project = TestProject::new("test").build_datamodel(); + project.models = vec![ + model("a", vec![module_var("to_b", "b"), aux_var("x", "1")]), + model("b", vec![module_var("to_a", "a"), aux_var("y", "1")]), + ]; + + let db = SimlinDb::default(); + let sync = sync_from_datamodel(&db, &project); + + let diags = collect_model_diagnostics(&db, sync.models["a"].source, sync.project); + assert!( + has_circular_diagnostic(&diags), + "expected a CircularDependency diagnostic, got {diags:?}" + ); +} + +/// The gate must not swallow a valid model's diagnostics just because some +/// OTHER model in the project is cyclic -- the same reachability scoping +/// `collect_all_diagnostics` uses. `main` here reaches no cycle, so its own +/// passes still run and its own equation error is still reported. +#[test] +fn per_model_diagnostics_survive_an_unrelated_draft_cycle() { + use crate::db::collect_model_diagnostics; + + let mut project = TestProject::new("test").build_datamodel(); + project.models = vec![ + model("main", vec![aux_var("x", "undefined_thing")]), + model("a", vec![module_var("to_b", "b")]), + model("b", vec![module_var("to_a", "a")]), + ]; + + let db = SimlinDb::default(); + let sync = sync_from_datamodel(&db, &project); + + let diags = collect_model_diagnostics(&db, sync.models["main"].source, sync.project); + assert!( + !has_circular_diagnostic(&diags), + "a model that reaches no cycle must not be reported as cyclic: {diags:?}" + ); + assert!( + !diags.is_empty(), + "main's own diagnostics must still be collected despite the unrelated \ + draft cycle, got: {diags:?}" + ); +} + +/// The cycle diagnostic must carry the model's DISPLAY name, not its canonical +/// map key -- and that is load-bearing, not cosmetic. +/// +/// `simlin-mcp-core`'s `read_model` (and `edit_model`) filter the collected +/// diagnostics with `e.model_name.as_ref().is_none_or(|name| name == +/// model_name)`, where `model_name` comes from `resolve_model_name` and is the +/// DATAMODEL spelling (`&m.name`). So a diagnostic reporting a model as +/// `sub_a` when the project spells it `Sub A` does not match and is dropped +/// from what the MCP tool returns -- silently, since the filter has no +/// fallback. Every other diagnostic in the crate already used +/// `model.name(db)`; the module-cycle gate was the sole outlier, so a +/// `CircularDependency` on a non-canonically-named model was the one error that +/// vanished on the way out. +/// +/// Every OTHER fixture in this file uses names that are already canonical +/// (`a`, `b`, `main`), where the two spellings coincide and nothing can +/// detect a regression. This one does not, so "tidying" the gate back to the +/// canonical key reds here. +#[test] +fn cycle_diagnostic_carries_the_display_name_not_the_canonical_key() { + use crate::db::collect_model_diagnostics; + + let mut project = TestProject::new("test").build_datamodel(); + project.models = vec![ + model( + "Sub A", + vec![module_var("to_b", "Sub B"), aux_var("x", "1")], + ), + model( + "Sub B", + vec![module_var("to_a", "Sub A"), aux_var("y", "1")], + ), + ]; + + let db = SimlinDb::default(); + let sync = sync_from_datamodel(&db, &project); + + let circular_models = |diags: &[crate::db::Diagnostic]| -> Vec { + diags + .iter() + .filter(|d| { + matches!(&d.error, DiagnosticError::Model(e) if e.code == ErrorCode::CircularDependency) + }) + .map(|d| d.model.clone()) + .collect() + }; + + // The per-model entry point (the one that gained the gate). + let per_model = collect_model_diagnostics(&db, sync.models["sub_a"].source, sync.project); + assert_eq!( + circular_models(&per_model), + vec!["Sub A".to_string()], + "the per-model collector must report the display spelling" + ); + + // ...and the whole-project one, which reaches the same helper by delegating. + let all = collect_all_diagnostics(&db, sync.project); + let mut names = circular_models(&all); + names.sort(); + assert_eq!( + names, + vec!["Sub A".to_string(), "Sub B".to_string()], + "the whole-project collector must report display spellings too" + ); +} + /// An unused draft cycle (`a <-> b`) that the requested `main` model does not /// reach must NOT block compiling or analyzing `main` -- the recursive queries /// only loop for modules under the requested root. The cycle is still surfaced /// as a diagnostic for the affected models. Regression for the Codex review /// finding on PR #807 (the project-wide guard over-rejected valid roots). +/// +/// `main` carries a dimensional problem of its OWN so that the "does not +/// block" half is actually pinned. With a clean `main` this test could not +/// tell "main's passes ran and found nothing" apart from "main's passes were +/// skipped" -- the exact failure a project-wide reject would produce, and the +/// one the gate's reachability scoping exists to prevent. A unit mismatch is +/// the right canary: it is a Warning, so it flows through `check_model_units` +/// (a pass the gate would skip) without disturbing the compile assertion +/// above. #[test] fn unused_draft_cycle_does_not_block_valid_main() { + use crate::testutils::x_aux; + let mut project = TestProject::new("test").build_datamodel(); project.models = vec![ - model("main", vec![aux_var("x", "1")]), + model( + "main", + vec![ + x_aux("x", "1", Some("widget")), + // widget + time: a genuine dimensional mismatch, reported as a + // Warning by `check_model_units`. + aux_var("bad", "x + TIME"), + ], + ), model("a", vec![module_var("to_b", "b")]), model("b", vec![module_var("to_a", "a")]), ]; @@ -155,13 +308,19 @@ fn unused_draft_cycle_does_not_block_valid_main() { "a valid main must compile despite an unrelated draft cycle" ); - // Diagnostics still surface the draft cycle (for the affected models), - // without hiding main's own (empty) diagnostics. + // Diagnostics still surface the draft cycle (for the affected models)... let diags = collect_all_diagnostics(&db, sp); assert!( has_circular_diagnostic(&diags), "the unrelated draft cycle should still be reported: {diags:?}" ); + // ...and main's own passes still ran, so its own problem is still reported. + assert!( + diags.iter().any(|d| d.model == "main" + && matches!(&d.error, DiagnosticError::Model(e) if e.code == ErrorCode::UnitMismatch)), + "a valid model's own diagnostics must not be hidden by an unrelated \ + draft cycle: {diags:?}" + ); // Analyzing main succeeds (no analysis_error); analyzing a cyclic model degrades. let main_analysis = @@ -178,6 +337,264 @@ fn unused_draft_cycle_does_not_block_valid_main() { ); } +// ── the macro-module cycle hole (macros.AC5.7) ────────────────────────────── + +/// The minimal project that used to abort: a macro-marked model holding an +/// explicit module whose target CALLS that macro. +/// +/// The cycle is `mac ->(explicit module `u_hop`)-> u ->(macro call `mac(input)`)-> +/// mac`. Its closing edge is the macro call, which `project_module_graph` does +/// not record (it reads variable KINDS off the salsa inputs, never a parse +/// result), so `cycle_error_from` reported no cycle from any root and every +/// entry point drove the recursive `model_module_map` straight into salsa's +/// dependency-graph cycle panic -- an abort under `panic=abort`. +/// +/// Every part of this fixture was ablated; only these three switches carry the +/// bug, and all three are necessary: +/// +/// - `mac` carrying a `macro_spec` (as a plain model, `mac(input)` is just an +/// `UnknownBuiltin` and no implicit edge exists); +/// - `mac` holding the explicit module (without it there is no `mac -> u` +/// edge); +/// - `u`'s equation calling `mac(...)` (without it there is no back edge). +/// +/// Three things the original reproducer carried are DECORATION, measured, and +/// dropped here so the regression pins the bug rather than the fixture: a +/// conventional `main` model instantiating `u`; a `+ u_hop·out` term in +/// `scaled` (added to keep the module from being "dead" -- the module VARIABLE +/// is the edge, so reading its output is not needed); and even the `p1` port +/// aux, which the panic does not need. The port aux is nonetheless KEPT: a +/// macro whose declared parameter has no body variable is a malformed fixture, +/// and what this pins is that a WELL-FORMED macro holding a module is rejected. +fn macro_holding_a_module_project() -> datamodel::Project { + use crate::testutils::{sim_specs_with_units, x_aux, x_model, x_module_named, x_project}; + + let mac = datamodel::Model { + name: "mac".to_string(), + sim_specs: None, + variables: vec![ + x_aux("scaled", "p1 * 2", None), + x_aux("p1", "0", None), + x_module_named("u_hop", "u", &[("p1", "u_hop.input")], None), + ], + views: vec![], + loop_metadata: vec![], + groups: vec![], + macro_spec: Some(datamodel::MacroSpec { + parameters: vec!["p1".to_string()], + primary_output: "scaled".to_string(), + additional_outputs: vec![], + }), + }; + let u = x_model( + "u", + vec![x_aux("input", "0", None), x_aux("out", "mac(input)", None)], + ); + x_project(sim_specs_with_units("month"), &[u, mac]) +} + +/// All three production entry points must REJECT [`macro_holding_a_module_project`] +/// rather than abort on it. +/// +/// Before the `MacroRegistry::build` rejection, all three panicked with salsa's +/// `dependency graph cycle` -- but NOT all in the same query, and the difference +/// is worth recording because it is what someone debugging a reopening would +/// need. Measured by disabling Pass 4 and driving each entry point separately: +/// +/// - `collect_all_diagnostics` -> **`model_module_map`** +/// (`model_all_diagnostics -> compile_var_fragment -> model_module_map -> +/// model_module_map -> parse_source_variable_with_module_context -> +/// variable_relevant_dimensions`); +/// - `compile_project_incremental` -> **`compute_layout`** +/// (`assemble_simulation -> assemble_module -> compute_layout -> +/// compute_layout -> variable_size -> variable_dimensions -> ...`); +/// - `analyze_model` -> **`compute_layout`** (same prefix, reaching +/// `variable_size` via `model_ltm_mode`). +/// +/// So BOTH recursive cross-model queries are live abort paths, not just the one +/// the diagnostic path happens to hit. An earlier version of this comment +/// claimed all three panicked in `model_module_map`; that was wrong for two of +/// the three, and would have sent a reader to the wrong query. +/// +/// Because the panic aborted the test, the return values were never observed -- +/// so this test asserts what the entry points do NOW and records the panic as +/// the pre-fix behavior, rather than pretending the old return values were known. +/// +/// The fix is a rejection, NOT a widening of the module graph, so the gate is +/// deliberately still blind here: the first assertion pins that. Widening +/// `project_module_graph` to parse-derived edges would put every variable's +/// parse on every compile's dependency list, which is why the shape is deleted +/// instead (see that query's rustdoc). A future change that intentionally +/// widens the gate has to come here and say so. +#[test] +fn macro_holding_a_module_is_rejected_instead_of_aborting() { + let project = macro_holding_a_module_project(); + + let mut db = SimlinDb::default(); + let sync = sync_from_datamodel(&db, &project); + let sp = sync.project; + + let graph = crate::db::project_module_graph(&db, sp); + for name in ["mac", "u"] { + assert_eq!( + graph.cycle_error_from(name), + None, + "the module graph is still structural and does not see the macro-call \ + back edge; the rejection -- not this gate -- is what makes {name} safe", + ); + } + + // Diagnostics: returns, and names the offending macro actionably. + let diags = collect_all_diagnostics(&db, sp); + let rejection = diags + .iter() + .find(|d| { + matches!(&d.error, DiagnosticError::Model(e) + if e.code == ErrorCode::MacroContainsModule) + }) + .unwrap_or_else(|| panic!("expected a MacroContainsModule diagnostic, got {diags:?}")); + let message = match &rejection.error { + DiagnosticError::Model(e) => e.get_details().unwrap_or_default(), + other => panic!("expected a Model error, got {other:?}"), + }; + assert!( + message.contains("mac") && message.contains("u_hop"), + "the rejection must name the offending macro and its module variable so the \ + modeller can find it: {message:?}", + ); + + // Compile: a clean `Err`, not an abort. + let err = compile_project_incremental(&db, sp, "u") + .expect_err("a macro holding a module must not compile"); + assert!( + err.get_details().unwrap_or_default().contains("mac"), + "the compile error must name the offending macro: {err:?}", + ); + + // Analysis: degrades to an `analysis_error` carrying the SAME actionable + // message. `analyze_model` never reads `build_error` itself -- the text + // arrives through `run_ltm_pipeline`'s compile failure -- so this pins that + // the rejection is not flattened into an opaque downstream error on the one + // entry point that has no direct view of it. + let analysis = analyze_model(&project, &mut db, sp, "u", None) + .expect("analyze_model must not panic on a macro-module cycle"); + let analysis_error = analysis + .analysis_error + .expect("analyzing a model whose macro is rejected must surface an analysis_error"); + assert!( + analysis_error.contains("mac") && analysis_error.contains("u_hop"), + "the analysis error must carry the actionable rejection, not an opaque \ + downstream failure: {analysis_error:?}", + ); +} + +/// `MacroRegistry::build` returning an EMPTY registry on failure is load-bearing +/// for CYCLE SAFETY, not merely for error quality -- and this is the observable +/// that proves it. +/// +/// `collect_all_diagnostics` and `analyze_model` do NOT stop at the macro +/// build error the way `compile_project_incremental` does: they emit / ignore it +/// and then run each model's passes anyway. What keeps those two from walking +/// the cycle is that with an empty registry `u`'s `mac(input)` no longer resolves +/// as a macro call, so builtin expansion synthesizes no implicit module and the +/// `u -> mac` edge does not exist. The `UnknownBuiltin` on `u·out` below is that +/// non-existence, made visible. +/// +/// A future "keep a partial registry alongside the error" refactor would resolve +/// the call again, restore the invisible edge, and reopen the abort -- on a +/// project with no explicit module inside a macro anywhere, since the rejected +/// macro would still be registered. This test reds if that happens, which is why +/// it asserts the cascade rather than filtering it out as noise. +#[test] +fn rejecting_a_macro_empties_the_registry_so_no_implicit_edge_is_synthesized() { + let project = macro_holding_a_module_project(); + + let db = SimlinDb::default(); + let sync = sync_from_datamodel(&db, &project); + + let registry = &crate::db::macro_registry::project_macro_registry(&db, sync.project).registry; + assert!( + registry.resolve_macro("mac").is_none(), + "a rejected macro set must yield an EMPTY registry, so no call is classified \ + module-backed and the implicit edge never exists", + ); + + let diags = collect_all_diagnostics(&db, sync.project); + assert!( + diags.iter().any(|d| d.model == "u" + && d.variable.as_deref() == Some("out") + && matches!(&d.error, DiagnosticError::Equation(e) + if e.code == ErrorCode::UnknownBuiltin)), + "with the registry emptied, `mac(input)` must fall through to UnknownBuiltin \ + -- the visible proof that no implicit module edge was synthesized: {diags:?}", + ); +} + +/// The rejection must not catch a LEGITIMATE macro. A macro-marked model with no +/// module variable still builds, still resolves, and still expands into a +/// working module instance at its call site. +/// +/// This is the negative control for the new pass, driven through the same salsa +/// pipeline as the rejection test above (rather than only `MacroRegistry::build` +/// in isolation) so it covers the whole registry -> expansion -> compile path. +#[test] +fn a_macro_without_a_module_still_builds_and_expands() { + use crate::testutils::{sim_specs_with_units, x_aux, x_model, x_project}; + + let mac = datamodel::Model { + name: "mac".to_string(), + sim_specs: None, + variables: vec![x_aux("scaled", "p1 * 2", None), x_aux("p1", "0", None)], + views: vec![], + loop_metadata: vec![], + groups: vec![], + macro_spec: Some(datamodel::MacroSpec { + parameters: vec!["p1".to_string()], + primary_output: "scaled".to_string(), + additional_outputs: vec![], + }), + }; + let main = x_model( + "main", + vec![x_aux("input", "3", None), x_aux("out", "mac(input)", None)], + ); + let project = x_project(sim_specs_with_units("month"), &[main, mac]); + + let db = SimlinDb::default(); + let sync = sync_from_datamodel(&db, &project); + + assert!( + crate::db::macro_registry::project_macro_registry(&db, sync.project) + .build_error + .is_none(), + "a macro with no module variable must still build", + ); + let diags = collect_all_diagnostics(&db, sync.project); + assert!( + diags.is_empty(), + "a legitimate macro must produce no diagnostics: {diags:?}" + ); + + // ...and it actually expands: `out` reads the macro instance's `scaled`. + let compiled = compile_project_incremental(&db, sync.project, "main") + .expect("a legitimate macro must compile"); + let mut vm = crate::vm::Vm::new(compiled).expect("VM creation must succeed"); + vm.run_to_end().expect("VM run must succeed"); + let results = vm.into_results(); + let collected = crate::test_common::collect_results(&results); + let out = collected + .get("out") + .unwrap_or_else(|| panic!("`out` missing from results: {:?}", collected.keys())); + // The non-empty check is not redundant: `all()` over an empty series is + // vacuously true, so without it a regression that produced no saved steps + // would pass silently. + assert!(!out.is_empty(), "`out` must have at least one saved step"); + assert!( + out.iter().all(|v| *v == 6.0), + "the expanded macro must compute 3 * 2 = 6 at every step, got {out:?}", + ); +} + /// A valid nested-module project (no cycle) must still compile and produce no /// spurious cycle diagnostic -- the guard must not false-positive on legitimate /// acyclic nesting. diff --git a/src/simlin-engine/src/db/query.rs b/src/simlin-engine/src/db/query.rs index 380d447f9..5c228084f 100644 --- a/src/simlin-engine/src/db/query.rs +++ b/src/simlin-engine/src/db/query.rs @@ -4,9 +4,10 @@ //! Demand-driven read queries over the salsa inputs: per-variable parsing //! (`parse_source_variable_with_module_context` and its `_impl`), the -//! project-global cached contexts (`project_units_context`, -//! `project_datamodel_dims`, `project_dimensions_context`, -//! `project_converted_dimensions`), the module-ident context +//! project-global cached contexts (`project_units_context_result` and its +//! `project_units_context` projection, `project_datamodel_dims`, +//! `project_dimensions_context`, `project_converted_dimensions`), the +//! module-ident context //! (`module_ident_context_for_model`/`model_module_ident_context`), the //! per-variable direct-dependency extraction //! (`variable_direct_dependencies` and its `_impl`, plus the @@ -38,43 +39,79 @@ impl std::fmt::Debug for ParsedVariableResult { } } +/// The project's units context together with the definition errors that +/// building it produced. +/// +/// The errors ride the RESULT rather than a salsa accumulator because a bad +/// unit declaration is a PROJECT-level fact -- there is one `units` list per +/// project, belonging to no model. Accumulating it from inside this query's +/// body meant `collect_all_diagnostics` reported one copy per model (every +/// model's diagnostic subtree reaches this query), and that after an unrelated +/// salsa revision bump the accumulator DFS pruned the subtree and the errors +/// vanished from the collection entirely. `collect_all_diagnostics` now reads +/// this field and emits each error once. See `db::macro_registry` for the +/// sibling case and `db/diagnostic_tests.rs` for the regression tests. +#[derive(Clone, PartialEq, salsa::Update)] +pub struct UnitsContextResult { + pub ctx: crate::units::Context, + /// One entry per unit declaration that failed to parse: its name and the + /// equation errors it produced. + pub definition_errors: Vec<(String, Vec)>, +} + /// Cached units context -- computed once per project, reused across all variables. /// Subsumes the per-variable Context::new_with_builtins calls. /// /// Reads the datamodel `Vec` and `SimSpecs` directly off the salsa input /// (the inputs now store the datamodel types, so no per-call conversion is /// needed). -/// -/// Unit definition parsing errors are accumulated as diagnostics so they -/// appear in `collect_all_diagnostics`. #[salsa::tracked(returns(ref))] -pub fn project_units_context(db: &dyn Db, project: SourceProject) -> crate::units::Context { - use salsa::Accumulator; +pub fn project_units_context_result(db: &dyn Db, project: SourceProject) -> UnitsContextResult { let dm_units = project.units(db); let dm_sim_specs = project.sim_specs(db); // Construction is partial: keep the context built from the valid unit - // declarations and surface each conflicting/duplicate declaration as a - // project-level diagnostic, rather than discarding every unit definition on - // the first conflict. An empty context would lose all alias normalization - // project-wide (yr/year, person/people, model-defined equivalences) and - // re-create a spurious unit-mismatch flood -- the context-layer parallel of - // the inference partial-results fix (GH #614). - let (ctx, unit_parse_errors) = crate::units::Context::new_with_builtins(dm_units, dm_sim_specs); - for (unit_name, eq_errors) in &unit_parse_errors { - for eq_err in eq_errors { - CompilationDiagnostic(Diagnostic { - model: String::new(), - variable: Some(unit_name.clone()), - error: DiagnosticError::Unit(crate::common::UnitError::DefinitionError( - eq_err.clone(), - None, - )), - severity: DiagnosticSeverity::Error, - }) - .accumulate(db); - } + // declarations and report each conflicting/duplicate declaration, rather + // than discarding every unit definition on the first conflict. An empty + // context would lose all alias normalization project-wide (yr/year, + // person/people, model-defined equivalences) and re-create a spurious + // unit-mismatch flood -- the context-layer parallel of the inference + // partial-results fix (GH #614). + let (ctx, definition_errors) = crate::units::Context::new_with_builtins(dm_units, dm_sim_specs); + UnitsContextResult { + ctx, + definition_errors, } - ctx +} + +/// The project's units context, for the many callers that want the context and +/// not the definition errors that came with it. +/// +/// This is a salsa PROJECTION over [`project_units_context_result`], not a plain +/// accessor, and the distinction is load-bearing. A caller that reads a plain +/// accessor takes a dependency on the whole `UnitsContextResult`, so it is +/// invalidated whenever EITHER half changes -- and the two halves move +/// independently: a unit whose equation fails to parse is left out of `ctx` +/// entirely, so editing a malformed unit equation (every intermediate state of +/// typing one) changes `definition_errors` while `ctx` stays byte-identical. +/// Every variable parse in the project would rebuild on each keystroke. +/// +/// As a tracked query it re-executes when the result changes but salsa BACKDATES +/// it when the projected `Context` compares equal, so its readers keep exactly +/// the dependency they had when the errors rode the salsa accumulator instead. +/// Pinned by `db::dimension_invalidation_tests:: +/// unit_definition_error_only_change_does_not_invalidate_context_readers`. +/// +/// **What the projection costs.** `Context::new_with_builtins` still runs +/// exactly once, in the query above -- the cost is memory, not CPU. Salsa now +/// RETAINS two `units::Context` copies per project: the one inside +/// `UnitsContextResult` and the one in this query's memo. Each also costs a +/// clone in every revision where the units input changes. A `Context` is the +/// project's unit name/alias/equation maps, so this is small next to the model +/// stages (see `db::stages`' memory note) and flat in the model count -- but it +/// is per project and permanent, so a future field on `Context` is paid twice. +#[salsa::tracked(returns(ref))] +pub fn project_units_context(db: &dyn Db, project: SourceProject) -> crate::units::Context { + project_units_context_result(db, project).ctx.clone() } /// Cached datamodel dimensions -- computed once per project. @@ -697,8 +734,76 @@ impl ModuleReferenceGraph { /// Build the project's explicit module-instance graph (see /// [`ModuleReferenceGraph`]). Reads each model's module variables and their /// target `model_name`s with flat salsa reads (no recursion). Implicit -/// (SMOOTH/DELAY/TREND/stdlib) modules can only reference leaf stdlib models, so -/// they can never close a user cycle and are intentionally omitted. +/// (SMOOTH/DELAY/TREND/stdlib/MACRO-call) modules are omitted, which keeps this +/// query structural: it reads variable KINDS and target names, never a parse +/// result, so it is invalidated by a variable being added, removed or retargeted +/// and not by an equation being typed. Every production entry point consults it, +/// so widening it to parse-derived edges would put every variable's parse on +/// every compile's dependency list. +/// +/// # Why omitting implicit edges is nonetheless SOUND +/// +/// This doc once claimed the omission was free because implicit modules "can only +/// reference leaf stdlib models, so they can never close a user cycle". That was +/// false for a MACRO call, which expands into an implicit module targeting the +/// macro's own model -- an ordinary user model. A macro whose body instantiated a +/// module targeting a model that CALLED that macro closed a cycle this graph did +/// not see, and `model_module_map` (which does follow implicit edges) then hit +/// salsa's unrecoverable dependency-graph cycle panic with this gate reporting no +/// cycle at all. +/// +/// That shape no longer exists: `MacroRegistry::build`'s Pass 4 rejects a +/// `Variable::Module` inside a macro-marked model (`MacroContainsModule`). With +/// it gone, every implicit module edge terminates somewhere that cannot continue +/// an invisible cycle: +/// +/// - The edges are synthesized at ONE site, +/// `builtins_visitor::expand_module_function`, from a +/// `ModuleFunctionDescriptor`; that type has exactly two producers, +/// `module_functions::stdlib_descriptor` (target `stdlib⁚{name}`) and +/// `MacroRegistry::build`'s Pass 1 (target the macro's own model). +/// - A stdlib model is a SINK -- it instantiates no module, explicit or +/// implicit. Test-asserted over the SYNCED Stage0s by +/// `db::stages_tests::omitting_stdlib_models_from_the_lowering_scope_is_inert_today`, +/// which is the tripwire if a template ever gains one. +/// - A macro model's three possible outgoing edges are all handled: an explicit +/// module in its body (Pass 4 rejects it), an implicit macro-to-macro edge +/// (Pass 3 rejects a cycle among those, and on ANY build failure the registry +/// the pipeline receives is EMPTY, so no call resolves as a macro and the edge +/// does not exist -- see `db::macro_registry::project_macro_registry`), or an +/// implicit stdlib edge (to a sink). +/// +/// So every remaining cycle lies entirely in explicit edges, which is exactly what +/// this query records. +/// +/// **RESIDUAL** (enumerated, not proved): the first bullet assumes builtin +/// expansion and macro expansion are the ONLY synthesisers of implicit module +/// vars. That came from reading the code paths -- the single +/// `expand_module_function` synthesis site and the two descriptor producers -- +/// not from any exhaustiveness check. +/// +/// Three things RECORD implicit module vars downstream of that one synthesis +/// site, and only one of them can open a cycle path: +/// +/// - `model_implicit_var_info` (`is_module` + `model_name`) -- read by BOTH +/// recursive queries, which recurse on its module entries. This is the live +/// path, and the one the argument above is about. +/// - `db::ltm::model_ltm_implicit_var_info` (`LtmImplicitVarMeta`, same two +/// fields) -- `compute_layout` reads it but takes `meta.size` verbatim +/// rather than recursing (Section 3b), and `model_module_map` does not read +/// it at all. Inert for cycle safety TODAY; listed because making it +/// recursive is a one-line edit that would silently reopen this. +/// - `db::stages::model_scope_models` -- walks Stage0 `variables`, but is an +/// iterative worklist, not a recursive tracked query, so a cycle terminates. +/// +/// A future implicit-var synthesizer that mints a `Variable::Module` with some +/// other target would be invisible to this gate again, and NO test would go red: +/// the two tripwires that exist cover the stdlib-sink premise and the +/// empty-registry premise, not this one. Anyone adding an implicit-var +/// synthesizer, or making the LTM recorder recursive, has to check it by hand. +/// +/// `db::stages::model_scope_models` walks the same edges plus the implicit ones +/// and is iterative, so it is unaffected either way. #[salsa::tracked(returns(ref))] pub fn project_module_graph(db: &dyn Db, project: SourceProject) -> ModuleReferenceGraph { let models = project.models(db); diff --git a/src/simlin-engine/src/db/stages.rs b/src/simlin-engine/src/db/stages.rs new file mode 100644 index 000000000..42bfe7ee5 --- /dev/null +++ b/src/simlin-engine/src/db/stages.rs @@ -0,0 +1,586 @@ +// Copyright 2026 The Simlin Authors. All rights reserved. +// Use of this source code is governed by the Apache License, +// Version 2.0, that can be found in the LICENSE file. + +// pattern: Imperative Shell +// +// Both queries only read salsa inputs and hand the results to the pure +// model-lowering core (`crate::model`): the parsing, dimension resolution and +// variable lowering they orchestrate all live there. + +//! The two name-keyed, pre-layout model-compilation stages, as salsa queries. +//! +//! `ModelStage0` (each variable parsed to `Expr0`, module references still +//! unresolved) and `ModelStage1` (those variables lowered to `Expr2` against +//! the project's dimension context) are cheap to describe and expensive to +//! build, and every consumer that wanted one used to build its own. In +//! particular `db::units::check_model_units` is tracked PER MODEL yet rebuilt +//! both stages for EVERY project model on each call, so collecting a project's +//! unit diagnostics cost `M x (whole-project Stage0 + Stage1)` -- quadratic in +//! the model count (GH #966). Caching them here as `returns(ref)` queries makes +//! each model's stages one memoized value that consumers READ. +//! +//! Those consumers read a SCOPE, not the project: [`model_scope_models`] is the +//! model plus the models it can reach through module instantiation, and it is +//! the map both `model_stage1` and `check_model_units` build over. With the +//! whole-project map the caching was still quadratic in READS, and worse, every +//! model's lowered stage and unit check depended on every other model's parse +//! results, so one keystroke anywhere invalidated all of them. +//! +//! This is the ONLY place in the crate the two stages are built from salsa +//! inputs. It is its own file, rather than more of `db/query.rs`, so that a +//! second construction site has to arrive as a new file instead of hiding as a +//! few more lines in a grab-bag module -- the drift this exists to delete. +//! `Project::from_salsa` used to carry a second copy (silently disagreeing on +//! three fields: the stdlib `implicit` test, the stdlib module-ident set, and +//! whether duplicate-canonical-ident model errors are recorded); this body took +//! `from_salsa`'s behaviour in all three, so retiring that copy was a deletion +//! rather than a merge, and `from_salsa` now clones these memos. +//! +//! Every other place in the crate that builds either stage, exhaustively -- +//! this file exists to be where that list is right, so keep it complete: +//! +//! PRODUCTION (three, none a whole-model salsa build, each deliberate): +//! +//! - `db::var_fragment::lower_var_fragment` (`ModelStage0` literal) and +//! `db::ltm::compile` (ditto) build a per-variable MINI Stage0 holding only +//! that variable's dependencies, then call `lower_variable` directly rather +//! than `ModelStage1::new`. Pointing those at this query would add a +//! project-wide dependency edge to every fragment compile -- the opposite of +//! the goal. +//! - `db::units::check_conveyor_param_units` calls `ModelStage1::new` on a +//! CLONE of the cached Stage0 augmented with synthetic conveyor-parameter +//! auxes. The augmented stage is a throwaway on purpose; see that module's +//! header. +//! +//! `#[cfg(test)]` (the oracle surface, all database-free): +//! +//! - `ModelStage0::new` / `new_in_project` build from a `datamodel::Model` +//! with no database at all, which these queries cannot do. They are the +//! independent oracle `db::stages_tests` checks this module against. +//! - Four `ModelStage1::new` call sites lower those oracle Stage0s: +//! `model.rs` (the dependency-resolution and `enumerate_modules` tests) and +//! `db::stages_tests::datamodel_driven_stage1s` (the whole-project lowering +//! oracle). They take a `ScopeStage0` the test builds by hand, so they are +//! construction sites for the stage TYPE but never for a cached value. +//! +//! **Memory.** `returns(ref)` means salsa RETAINS one `ModelStage0` and one +//! `ModelStage1` per model for as long as their memos stay valid, where the old +//! code built them transiently and dropped them at the end of each +//! `check_model_units` call. That retention is the point of the change -- it is +//! what makes the stages readable instead of rebuildable -- but it is a real new +//! resident footprint, roughly two lowered copies of every equation in the +//! project (Stage0's `Expr0` plus Stage1's `Expr2`, both name-keyed and +//! pre-layout). On a C-LEARN-scale project that is the dominant term in this +//! module's cost; the old code's cost was CPU instead. Anything added to either +//! stage is paid per model per revision, so keep them to what lowering needs. +//! +//! Like `db::units`, this is a submodule of `db` reached as `crate::db::...`. + +use std::collections::{BTreeMap, BTreeSet, HashMap}; + +use crate::common::{Canonical, Ident}; +use crate::datamodel; +use crate::db::{ + Db, ModuleIdentContext, SourceModel, SourceProject, model_duplicate_variables, + model_module_ident_context, parse_source_variable_with_module_context, project_datamodel_dims, + project_dimensions_context, project_units_context, +}; +use crate::model::{ModelStage0, ModelStage1, ScopeStage0, VariableStage0}; +use crate::variable::Variable; + +// Test-only per-thread execution counters for the two stage queries and the +// unit-check pass that reads them. +// +// Pointer equality of a `returns(ref)` memo does NOT prove a query body did +// not run: salsa backdates a re-executed query whose value compares equal, and +// the memo keeps its address across that backdating. Counting body entries is +// the only evidence that separates "read the memo" from "rebuilt it and found +// it equal" -- and "built at most once per revision" is exactly the GH #966 +// claim, while "an unrelated model's edit does not invalidate this one" is the +// scope-narrowing claim. +// +// `check_model_units` counts here rather than in `db::units` so that one reset +// covers every counter a test in this area reads. A second mechanism next door +// would let a test reset half of them and measure the other half against +// whatever an earlier test left behind. +// +// Thread-local rather than a global atomic so that a PARALLEL test run cannot +// charge one test's queries to another, and so no lock is needed. That alone +// does not isolate a measuring test: under `--test-threads=1` libtest runs +// every test on the same thread, so the counters carry over. The isolation that +// actually holds in both modes is `reset_query_executions()` at the start of +// the measured region -- see its rustdoc. +// +// The other edge of thread-local, and the dangerous one: the bump happens +// INSIDE the query body, on whatever thread salsa ran it. Nothing in this +// subtree executes in parallel today, but if salsa's `par_map` (or any other +// fan-out) is ever adopted for the stage queries, a body that runs on a worker +// thread bumps that thread's counter and the measuring test never sees it. The +// count then comes in UNDER the expected one and the test fails -- except in +// the case that matters, where the #966 regression is a query rebuilding on +// other threads and the test reads a comfortable-looking small number and goes +// green. Anyone parallelizing here must move these to a shared atomic (or scope +// them to the salsa runtime) in the same change, not afterwards. +#[cfg(test)] +thread_local! { + static STAGE0_EXECUTIONS: std::cell::Cell = const { std::cell::Cell::new(0) }; + static STAGE1_EXECUTIONS: std::cell::Cell = const { std::cell::Cell::new(0) }; + static UNIT_CHECK_EXECUTIONS: std::cell::Cell = const { std::cell::Cell::new(0) }; +} + +/// How many times each counted query body has run on this thread. +#[cfg(test)] +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub(crate) struct QueryExecutions { + pub(crate) stage0: usize, + pub(crate) stage1: usize, + pub(crate) unit_check: usize, +} + +/// Zero every counter (test-only). +/// +/// This is what isolates a measuring test, in BOTH libtest modes: the counters +/// are per-thread, and `--test-threads=1` puts every test on one thread, so a +/// test that does not reset would see whatever earlier tests left behind. Call +/// it after building the `SimlinDb` and syncing, so a stage the fixture setup +/// happened to demand is not charged to the measured work. +#[cfg(test)] +pub(crate) fn reset_query_executions() { + STAGE0_EXECUTIONS.with(|c| c.set(0)); + STAGE1_EXECUTIONS.with(|c| c.set(0)); + UNIT_CHECK_EXECUTIONS.with(|c| c.set(0)); +} + +/// Read the current counters (test-only). +#[cfg(test)] +pub(crate) fn query_executions() -> QueryExecutions { + QueryExecutions { + stage0: STAGE0_EXECUTIONS.with(|c| c.get()), + stage1: STAGE1_EXECUTIONS.with(|c| c.get()), + unit_check: UNIT_CHECK_EXECUTIONS.with(|c| c.get()), + } +} + +/// Record one entry into a counted query's body (test-only). +#[cfg(test)] +fn note_execution(counter: &'static std::thread::LocalKey>) { + counter.with(|c| c.set(c.get() + 1)); +} + +/// Record one entry into `db::units::check_model_units` (test-only). +#[cfg(test)] +pub(super) fn note_unit_check_execution() { + note_execution(&UNIT_CHECK_EXECUTIONS); +} + +/// Is `canonical_model_name` one of the stdlib models `db::sync` splices into +/// every project? +/// +/// The `stdlib⁚` prefix alone is NOT sufficient. It uses a punctuation +/// separator that ordinary model creation never produces, but an import can +/// still carry a model whose name has the prefix and a suffix naming no stdlib +/// model; flagging that model implicit would mark a user model as a generic +/// template. Requiring the suffix to be a real stdlib model name keeps the flag +/// on exactly the models the stdlib splice introduced. +/// +/// This is the ONE stdlib test in the engine's diagnostic path: `db::units`'s +/// unit-check skip gate calls [`source_model_is_stdlib`] rather than carrying a +/// second, looser spelling (GH #988). A model with the prefix and an unknown +/// suffix is a user model, so it IS unit-checked -- the gate exists to skip +/// generic templates, and that is not one. +/// +/// `pub(super)` (not private) so `db::stages_tests` can pin the rule directly: +/// nothing on the unit-checking path reads `ModelStage0::implicit`, so a flip +/// back to the bare-prefix test is invisible in the stage VALUE. +pub(super) fn model_is_stdlib(canonical_model_name: &str) -> bool { + canonical_model_name + .strip_prefix("stdlib\u{205A}") + .is_some_and(|suffix| crate::stdlib::MODEL_NAMES.contains(&suffix)) +} + +/// [`model_is_stdlib`] for a salsa model handle. +/// +/// `SourceModel::name` holds the DISPLAY name, so it is canonicalized first -- +/// the project's model map is canonically keyed, and an imported model spelled +/// `Stdlib⁚Smth1` is the same model as `stdlib⁚smth1`. +pub(crate) fn source_model_is_stdlib(db: &dyn Db, model: SourceModel) -> bool { + model_is_stdlib(Ident::::new(model.name(db)).as_str()) +} + +/// The module-ident names a model's variable parses must treat as module-backed +/// ON TOP OF the ones `model_module_ident_context` derives from the model's own +/// module variables and module-call equations. +/// +/// For a stdlib model that is EVERY variable name. Inside a submodule some +/// variables are module inputs whose values arrive through a transient array +/// and have no persistent slot, so `PREVIOUS(module_input)` must first capture +/// the current value into a scalar helper aux rather than compile a `LoadPrev` +/// against a slot that does not exist. +/// +/// No shipped stdlib body calls `PREVIOUS`/`INIT`, so today this changes no +/// parse result. It is kept because it is the rule the datamodel-driven +/// `ModelStage0::new` uses for an implicit model, and the rule +/// `Project::from_salsa` used while it still built its own stages: one rule +/// means every path reaching a stdlib model's parse hits the SAME +/// `ModuleIdentContext` and shares one +/// `parse_source_variable_with_module_context` cache entry, instead of minting +/// a second set of parses under a second key. +/// +/// `pub(super)` for the same reason as [`model_is_stdlib`]: the rule is inert +/// in the stage VALUE today, so `db::stages_tests` pins it here. +pub(super) fn extra_module_idents(db: &dyn Db, model: SourceModel, is_stdlib: bool) -> Vec { + if is_stdlib { + model.variables(db).keys().cloned().collect() + } else { + vec![] + } +} + +/// The two facts [`model_stage0`] derives about a model before it parses +/// anything: whether it is a stdlib template, and the interned module-ident +/// context its variables must be parsed under. +pub(super) struct Stage0Context<'db> { + pub(super) is_stdlib: bool, + pub(super) module_idents: ModuleIdentContext<'db>, +} + +/// Derive [`Stage0Context`] for `model`. +/// +/// The two are derived TOGETHER, in one `pub(super)` function, on purpose. The +/// stdlib rule feeds both `ModelStage0::implicit` and the extra module idents, +/// and when they were derived separately inside the query body the extra-ident +/// half was untestable: reverting it to `vec![]` at the call site changed no +/// stage value and no test failed, because the rule is inert in the parse +/// result today (see [`extra_module_idents`]). `model_stage0` cannot obtain +/// `implicit` without also obtaining the context, so the wiring now has exactly +/// one place to go wrong, and `db::stages_tests` pins that place on the +/// INTERNED context identity rather than on a parse result that cannot differ. +pub(super) fn model_stage0_context<'db>( + db: &'db dyn Db, + model: SourceModel, + project: SourceProject, +) -> Stage0Context<'db> { + let is_stdlib = source_model_is_stdlib(db, model); + Stage0Context { + is_stdlib, + module_idents: model_module_ident_context( + db, + model, + project, + extra_module_idents(db, model, is_stdlib), + ), + } +} + +/// The models `model`'s lowering and unit inference can reach: itself, plus the +/// transitive closure of the models its module variables instantiate. Keyed by +/// canonical model name, valued by the project's handle for that name. +/// +/// This is the SCOPE of both `model_stage1` below and `check_model_units`' +/// inference map. It is narrower than the project on purpose: with the +/// whole-project map, every model's lowered stage and unit check depended on +/// every other model's parse results, so any edit anywhere invalidated all of +/// them (the follow-on GH #966 named and deferred). +/// +/// # Why the closure is the right width +/// +/// The closure is a SUPERSET of what any consumer can consult, not an exact +/// fit: too wide costs incrementality, too narrow is a silent wrong answer, so +/// where the two disagree this errs wide. Two lowering steps read another +/// model's Stage0, and only these two: +/// +/// - `ArrayContext::get_variable` (`ast/mod.rs`) resolves a dotted +/// `module·var` reference by looking the MODULE VARIABLE up in the current +/// model and recursing into the model it names. The `model_name` edges below +/// are exactly the edges that walk can follow, so for THIS consumer the +/// closure is exactly its reach. +/// - `resolve_relative` (`model.rs`), reached from `lower_variable`'s module +/// arm, resolves a module input's `src`. It predates modules being able to +/// carry an ident different from their target model's name and keys each +/// dotted component as a MODEL name (see its `module ident == model name` +/// TODO). That is why a module's own IDENT is an edge here as well when it +/// happens to name a project model -- otherwise a project with both would +/// start reporting a `BadModuleInputSrc` that the whole-project map did not. +/// Those ident edges are what make the closure strictly WIDER than +/// `ArrayContext`'s reach. +/// +/// `units_infer::gen_all_constraints` reads its map at one site +/// (`self.models.get(model_name)`, for module targets), recursing along the +/// target edges and DECLINING a back edge already on its `InstantiationPath`, so +/// it visits a subset of this closure and the same closure covers it. +/// +/// Where the ident edges leave a residual: `resolve_relative` walks components +/// of an arbitrary `src` STRING, so a `src` naming a model that is neither a +/// module target nor a module ident of the model would resolve under a +/// whole-project map and is a `BadModuleInputSrc` here. That is judged +/// malformed -- a well-formed XMILE `connect from` names a module instance in +/// the same model -- and three things support it, none of which is "the corpus +/// is green": +/// +/// - **A corpus run could not have caught it.** The `BadModuleInputSrc` this +/// mints lands on a `ModelStage1` variable's `errors` and reaches no +/// production diagnostic: `db::assemble::build_module_inputs` does not +/// validate `src`, and nothing on the unit path reads `ModelStage1` variable +/// errors. So a green corpus is not evidence here, and citing it would +/// mislead the next person narrowing something. +/// - **The user-facing warning is scope-independent.** The +/// `BadModuleInputSrc` a modeller actually sees comes from +/// `db::diagnostic::model_module_wiring_diagnostics`, which reads the salsa +/// INPUTS (`model.variables`, `project.models`, `svar.module_refs`) and +/// never a lowering scope. Narrowing this map can therefore neither create +/// nor suppress one (`db::module_wiring_tests`). +/// - **Direct measurement.** Every `resolve_module_input` call was +/// instrumented and counted under the narrowed scope: 560 of 560 resolved +/// across the `file_io` integration corpus, and 970 across the lib suite +/// with exactly one failure -- `db::module_wiring_tests::dangling_src_warns`, +/// which deliberately wires a `src` naming no variable at all and whose +/// scope map held both project models. That is a fixture asserting the +/// warning, not a scope miss. +/// +/// The review added a fourth, independently: a dual-lowering oracle inside +/// `model_stage1` that lowered every model under both this scope and the +/// whole-project one and diffed `variables`, across the corpus and the lib +/// suite, finding zero user-model divergence. +/// +/// # Implicit modules are IN, and that is load bearing +/// +/// The edges come from each model's Stage0 `variables`, which holds the +/// SMOOTH/DELAY/TREND and macro-call modules that builtin expansion synthesized +/// alongside the declared ones. `db::project_module_graph` deliberately omits +/// those (it only needs the edges that can close a user cycle), so it is the +/// wrong source here: a macro call expands into a module targeting the macro's +/// own model, which is an ordinary user model that can perfectly well be +/// arrayed. Dropping it would make `get_dimensions` return `None` and lower the +/// reference as a scalar -- compilable, plausible, and wrong. +/// +/// Stdlib templates are in the closure on the same rule, rather than being +/// spliced in wholesale: a model that instantiates none of them does not stage +/// any, and one that instantiates `smth1` gets exactly `stdlib⁚smth1`. +/// `db::stages_tests::omitting_stdlib_models_from_the_lowering_scope_is_inert_today` +/// records why the wholesale alternative would be safe TODAY; this rule does not +/// depend on that staying true. +/// +/// # Cycles +/// +/// The walk is an iterative worklist over a visited set, NOT a recursive tracked +/// query: `a` instantiating `b` and `b` instantiating `a` is a project a user can +/// draw, and a recursive salsa query on that graph is an unrecoverable +/// dependency-graph panic rather than a diagnostic (GH #806). A model inside a +/// cycle yields its full REACHABLE set -- the strongly-connected component it +/// sits in AND everything downstream of that component, since the walk does not +/// stop at the back edge, it only declines to re-visit -- and each member's +/// stage lowers against all of it. +/// +/// # What the walk costs +/// +/// It reads each reachable model's Stage0 and scans its `variables` map, so one +/// execution is O(variables in the closure) and it re-executes whenever any of +/// those Stage0s changes -- i.e. on an ordinary equation edit, for every model +/// that reaches the edited one. The RESULT is unchanged by such an edit, so +/// salsa backdates it and nothing downstream is invalidated by the re-execution +/// itself; the cost is the scan. That is deliberate: a per-model +/// `module targets` projection in between would let the closure survive those +/// edits untouched, but it is a third query to keep in step with these two for a +/// scan of an already-materialized map, and the models whose scope is wide are +/// the models whose Stage1 has to be rebuilt anyway. +#[salsa::tracked(returns(ref))] +pub(crate) fn model_scope_models( + db: &dyn Db, + model: SourceModel, + project: SourceProject, +) -> BTreeMap, SourceModel> { + let project_models = project.models(db); + let root_ident: Ident = Ident::new(model.name(db)); + + let mut scope: BTreeMap, SourceModel> = BTreeMap::new(); + if let Some(src_model) = project_models.get(root_ident.as_str()) { + scope.insert(root_ident.clone(), *src_model); + } + + // The root is walked through the handle the CALLER passed, not through the + // project's entry for its name: a renamed-or-deleted model's handle outlives + // its place in that map (`PersistentSyncState` threads handles across syncs), + // and its own module targets are still what its lowering will consult. + let mut visited: BTreeSet> = [root_ident].into_iter().collect(); + let mut queue: Vec = vec![model]; + while let Some(src_model) = queue.pop() { + for var in model_stage0(db, src_model, project).variables.values() { + let Variable::Module { + ident, model_name, .. + } = var + else { + continue; + }; + for target in [model_name, ident] { + if !visited.insert(target.clone()) { + continue; + } + if let Some(next) = project_models.get(target.as_str()) { + scope.insert(target.clone(), *next); + queue.push(*next); + } + } + } + } + + scope +} + +/// [`model_scope_models`] resolved to Stage0s: the `models` map a +/// [`ScopeStage0`] lowering scope needs. +/// +/// Both readers -- `model_stage1` below and the conveyor parameter check in +/// `db::units`, which lowers its synthetic parameter auxes in the same scope -- +/// need the identical map. Building it in two places would re-introduce, one +/// level up, exactly the duplicated construction this module exists to remove. +/// +/// The self entry is REPAIRED rather than assumed. Every caller reaches this +/// with a handle the project's model map holds under the same name, so the +/// insert below is inert -- a pointer write of a value already there. It is here +/// for the case the call sites cannot show: a handle that outlived its place in +/// that map (renamed, or deleted while something still holds it). With no self +/// entry `ArrayContext::get_model` returns `None`, so `get_dimensions` returns +/// `None` for every reference in the model and every arrayed equation lowers as +/// though it were scalar. That compiles, simulates, and is wrong -- the exact +/// failure class this plan exists to delete, which is why it is repaired rather +/// than detected. A hard `assert!` was considered and rejected: libsimlin builds +/// with panic=abort, so an assert that ever fired would abort the user's process +/// (a WASM tab, an MCP server) over something recoverable. +/// +/// One BEHAVIOUR DELTA rides on that repair, so it is recorded rather than left +/// to read as a pure substitution. `db::units::check_conveyor_param_units` used +/// the un-repaired whole-project map, so it GAINS the self-entry repair by moving +/// here. It is a strict improvement -- that function lowers an `aug_ms0` built +/// from THIS model's Stage0, and it now lowers it in a scope that is guaranteed +/// to contain this model, which is what it always assumed -- but it is a change, +/// reachable only on the same stale-handle path the repair exists for. +pub(crate) fn model_scope_stage0( + db: &dyn Db, + model: SourceModel, + project: SourceProject, +) -> HashMap, &ModelStage0> { + let mut models_s0: HashMap, &ModelStage0> = + model_scope_models(db, model, project) + .values() + .map(|src_model| { + let s0 = model_stage0(db, *src_model, project); + (s0.ident.clone(), s0) + }) + .collect(); + let model_s0 = model_stage0(db, model, project); + models_s0.insert(model_s0.ident.clone(), model_s0); + models_s0 +} + +/// A model's parsed-but-unresolved stage, built from the salsa-cached +/// per-variable parse results. +/// +/// Keyed on `(model, project)`: the project supplies the dimension list, the +/// units context and the macro registry every parse reads, so a model cannot be +/// staged without one. +#[salsa::tracked(returns(ref))] +pub(crate) fn model_stage0(db: &dyn Db, model: SourceModel, project: SourceProject) -> ModelStage0 { + #[cfg(test)] + note_execution(&STAGE0_EXECUTIONS); + + let units_ctx = project_units_context(db, project); + let dm_dims = project_datamodel_dims(db, project); + + let display_name = model.name(db); + let ident: Ident = Ident::new(display_name); + let Stage0Context { + is_stdlib, + module_idents, + } = model_stage0_context(db, model, project); + + let src_vars = model.variables(db); + let mut var_list: Vec = Vec::with_capacity(src_vars.len()); + let mut implicit_dm: Vec = Vec::new(); + for svar in src_vars.values() { + let parsed = parse_source_variable_with_module_context(db, *svar, project, module_idents); + var_list.push(parsed.variable.clone()); + implicit_dm.extend(parsed.implicit_vars.iter().cloned()); + } + + // The implicit variables SMOOTH/DELAY/TREND expansion synthesized have no + // `SourceVariable` of their own, so they are parsed directly here. They are + // plain stocks/flows/auxes and module instances, never module CALLS, so the + // expansion cannot recurse -- assert that rather than silently dropping a + // second generation into the `nested` sink. + let mut nested_implicit: Vec = Vec::new(); + var_list.extend(implicit_dm.into_iter().map(|dm_var| { + crate::variable::parse_var(dm_dims, &dm_var, &mut nested_implicit, units_ctx, |mi| { + Ok(Some(mi.clone())) + }) + })); + debug_assert!( + nested_implicit.is_empty(), + "implicit vars should not produce further implicit vars" + ); + + let variables: HashMap, VariableStage0> = var_list + .into_iter() + .map(|v| (Ident::new(v.ident()), v)) + .collect(); + + ModelStage0 { + ident, + display_name: display_name.clone(), + variables, + // Two declared variables whose names canonicalize identically already + // collapsed last-wins on the canonical-keyed `variables` map above, so + // it cannot detect the twin; the memoized groups derive from the raw + // pre-dedup declared-ident list instead (GH #885/#891). + errors: crate::common::duplicate_variable_errors_from_groups( + display_name, + model_duplicate_variables(db, model), + ), + implicit: is_stdlib, + is_macro: model.macro_spec(db).is_some(), + macro_params: crate::model::macro_param_idents(model.macro_spec(db).as_ref()), + } +} + +/// A model's lowered stage: `model_stage0` with every variable's AST lowered to +/// `Expr2` against the project's dimension context. +/// +/// The lowering scope is [`model_scope_models`] -- this model plus the models it +/// can reach through module instantiation -- because `ModelStage1::new` resolves +/// a `module·output` reference's dimensions by following the module to its +/// target model's variables. Nothing outside that closure is consultable, so a +/// model's lowered stage does not depend on the rest of the project and an +/// unrelated model's edit does not invalidate it. +/// +/// Three fixtures in `db::stages_tests` are what can see this scope going wrong, +/// and they separate the mistakes a narrowing can make: +/// +/// - `arrayed_module_project` -- `main` reduces over an ARRAYED output of its +/// direct module target `sub_a`, so a scope holding only the model itself +/// lowers that reference as a scalar. (Before it existed, emptying this map +/// entirely left every test in the crate green.) +/// - `chain_project` -- `main -> sub_a -> sub_c`, with `main` reducing over +/// `sub_a.sub_c.out_by_region`. `sub_c` is reachable only THROUGH `sub_a`, +/// so a scope of "self + direct module targets" is caught here and nowhere +/// else. The closure must be TRANSITIVE. +/// - `omitting_stdlib_models_from_the_lowering_scope_is_inert_today` -- a +/// narrowing that drops the `stdlib⁚*` models wholesale is currently +/// harmless, and that test asserts the precise reason (no stdlib template is +/// arrayed or instantiates a module). The closure does not take that +/// shortcut -- it keeps the stdlib models a lowering can actually reach -- +/// so the test now guards a road not taken rather than this code. +#[salsa::tracked(returns(ref))] +pub(crate) fn model_stage1(db: &dyn Db, model: SourceModel, project: SourceProject) -> ModelStage1 { + #[cfg(test)] + note_execution(&STAGE1_EXECUTIONS); + + let model_s0 = model_stage0(db, model, project); + let models_s0 = model_scope_stage0(db, model, project); + + let scope = ScopeStage0 { + models: &models_s0, + dimensions: project_dimensions_context(db, project), + model_name: model_s0.ident.as_str(), + }; + ModelStage1::new(&scope, model_s0) +} diff --git a/src/simlin-engine/src/db/stages_tests.rs b/src/simlin-engine/src/db/stages_tests.rs new file mode 100644 index 000000000..14aa3557b --- /dev/null +++ b/src/simlin-engine/src/db/stages_tests.rs @@ -0,0 +1,1688 @@ +// Copyright 2026 The Simlin Authors. All rights reserved. +// Use of this source code is governed by the Apache License, +// Version 2.0, that can be found in the LICENSE file. + +//! Tests for the two cached model-compilation stage queries (`db::stages`). +//! +//! Three claims are pinned here: +//! +//! 1. **Value**: the cached `model_stage0` / `model_stage1` equal what the +//! independently-written, salsa-free `ModelStage0::new_in_project` (plus +//! `ModelStage1::new` over a whole-project scope of those) builds for the +//! same models -- across every model shape, up to and including the +//! combined [`every_shape_project`]. +//! 2. **Cost (GH #966)**: whole-project unit diagnostics build each model's +//! two stages AT MOST ONCE per revision, not once per (model, model) pair +//! -- and `Project::from_salsa` READS those same memos rather than building +//! a second set. +//! 3. **Diagnostics do not move**: every diagnostic that reached a harvest +//! point before the stage construction moved out of `check_model_units` +//! still reaches it. +//! +//! The oracles used to be `ModelStage0::new_cached` (a test-only third copy of +//! the salsa-cached construction) and `Project::from_salsa`'s own inline +//! lowering. Both are gone: the first was deleted, and the second became +//! circular once `from_salsa` started reading these queries. A datamodel-driven +//! constructor that touches no database is the oracle that cannot go circular. +//! +//! # Compare stages with `PartialEq`, never with Debug text +//! +//! `Ast::Arrayed` holds its per-element equations in a `HashMap`, so a stage's +//! Debug rendering is ORDER-UNSTABLE run to run, while the derived `PartialEq` +//! is order-independent. A Debug-text oracle over a fixture with per-element +//! equations therefore reports spurious inequality -- including a stage +//! "differing from itself" -- for a reason that has nothing to do with the code +//! under test. +//! +//! This has cost time twice: once in a throwaway harness written to design the +//! scope-narrowing fixtures, and once in the temporary oracle used to verify +//! that `Project::from_salsa`'s deleted inline build equalled these queries -- +//! which sorted Debug strings and was sound only by the accident that its +//! fixtures had no `Equation::Arrayed`. +//! +//! The rule: compare with `PartialEq`. If you must reach for Debug text because +//! a NaN literal defeats `PartialEq` (GH #987/#981 -- `ModelStage0` compares +//! parsed `f64` constants, and every SMOOTH/DELAY/TREND template declares +//! `initial_value = NAN`, so such a stage is not even equal to itself), then it +//! is sound ONLY for fixtures with no per-element equations, and the test must +//! say so. + +use super::*; +use crate::common::{Canonical, ErrorCode, Ident}; +use crate::datamodel; +use crate::model::{ModelStage0, ModelStage1}; +use crate::testutils::{sim_specs_with_units, x_aux, x_model, x_module, x_project}; + +// ── fixtures ──────────────────────────────────────────────────────────── + +/// A project with three user models: a `main` that instantiates two +/// sub-models, so Stage1 lowering actually walks the scope map. +fn three_model_project() -> datamodel::Project { + let sub_a = x_model( + "sub_a", + vec![x_aux("input", "0", None), x_aux("out", "input * 2", None)], + ); + let sub_b = x_model( + "sub_b", + vec![x_aux("input", "0", None), x_aux("out", "input + 1", None)], + ); + let main = x_model( + "main", + vec![ + x_aux("driver", "5", None), + x_module("sub_a", &[("driver", "sub_a.input")], None), + x_module("sub_b", &[("driver", "sub_b.input")], None), + x_aux("combined", "sub_a.out + sub_b.out", None), + ], + ); + x_project(sim_specs_with_units("month"), &[main, sub_a, sub_b]) +} + +/// An apply-to-all `datamodel::Aux` over one dimension. +fn x_apply_to_all(ident: &str, dim: &str, equation: &str) -> datamodel::Variable { + datamodel::Variable::Aux(datamodel::Aux { + ident: ident.to_string(), + equation: datamodel::Equation::ApplyToAll(vec![dim.to_string()], equation.to_string()), + documentation: String::new(), + units: None, + gf: None, + ai_state: None, + uid: None, + compat: datamodel::Compat::default(), + }) +} + +/// The same shape plus a dimension and apply-to-all variables, so Stage0's +/// dimension resolution and Stage1's array lowering are exercised. +/// +/// `sub_a` gains an ARRAYED output that `main` reduces over, which is the one +/// construct that makes the whole-project scope map load-bearing: +/// `ArrayContext::get_variable` resolves `sub_a·out_by_region`'s dimensions by +/// following the module variable into `sub_a`'s Stage0, so with the other +/// models missing from the scope `get_dimensions` returns `None` and the +/// reference lowers as though it were scalar. That silent mis-lowering is +/// exactly what `model_stage1`'s self-insert comment describes -- and, until +/// this fixture, nothing in the crate's tests could see it: emptying the scope +/// map entirely left the whole engine suite green. +fn arrayed_module_project() -> datamodel::Project { + let mut project = three_model_project(); + project.dimensions = vec![datamodel::Dimension::named( + "Region".to_string(), + vec!["north".to_string(), "south".to_string()], + )]; + let sub_a = project + .models + .iter_mut() + .find(|m| m.name == "sub_a") + .expect("fixture has a sub_a model"); + sub_a + .variables + .push(x_apply_to_all("out_by_region", "Region", "input * 3")); + + let main = project + .models + .iter_mut() + .find(|m| m.name == "main") + .expect("fixture has a main model"); + main.variables + .push(x_apply_to_all("pop", "Region", "driver * 2")); + main.variables.push(x_aux("total_pop", "SUM(pop[*])", None)); + main.variables.push(x_aux( + "sub_region_total", + "SUM(sub_a.out_by_region[*])", + None, + )); + project +} + +/// A TWO-LEVEL module chain, `main -> sub_a -> sub_c`, where `main` makes an +/// arrayed reference that resolves through BOTH hops. +/// +/// This separates the two narrowings of `model_stage1`'s scope map that +/// [`arrayed_module_project`] cannot tell apart. `ArrayContext::get_variable` +/// resolves `sub_a·sub_c·out_by_region` by recursing once per hop, looking up +/// each intermediate model in `scope.models`: `main` (self), then `sub_a` +/// (a DIRECT module target of main), then `sub_c` (reachable only THROUGH +/// `sub_a`, and not a module target of `main` at all). +/// +/// So a scope narrowed to "self + direct module targets" drops `sub_c`, +/// `get_dimensions` returns `None`, and `SUM(...[*])` lowers as though its +/// argument were scalar -- silently, with no error. Only the TRANSITIVE +/// module-reachable closure is correct, and this fixture is what says so. +fn chain_project() -> datamodel::Project { + let sub_c = x_model( + "sub_c", + vec![ + x_aux("input", "0", None), + x_apply_to_all("out_by_region", "Region", "input * 3"), + ], + ); + let sub_a = x_model( + "sub_a", + vec![ + x_aux("input", "0", None), + x_module("sub_c", &[("input", "sub_c.input")], None), + ], + ); + let main = x_model( + "main", + vec![ + x_aux("driver", "5", None), + x_module("sub_a", &[("driver", "sub_a.input")], None), + x_aux("deep_total", "SUM(sub_a.sub_c.out_by_region[*])", None), + ], + ); + let mut project = x_project(sim_specs_with_units("month"), &[main, sub_a, sub_c]); + project.dimensions = vec![datamodel::Dimension::named( + "Region".to_string(), + vec!["north".to_string(), "south".to_string()], + )]; + project +} + +/// Every shape `Project::from_salsa`'s deleted inline Stage0 build had to +/// handle, in one project: +/// +/// - an IMPLICIT stdlib module instance (`SMTH1`), whose expansion synthesizes +/// module and argument variables that have no `SourceVariable` of their own +/// and so are parsed inside the stage rather than by the per-variable memo; +/// - an EXPLICIT user sub-model instance that `main` READS through +/// (`SUM(sub.out_by_region[*])`), so Stage1 lowering genuinely follows a +/// `module·output` reference into another model's Stage0 and depends on +/// the answer -- instantiating `sub` without reading it would exercise +/// only `resolve_module_input`'s self-entry path; +/// - a MACRO-marked model (`is_macro` / `macro_params` non-default) together +/// with a caller of it, which only classifies correctly under the +/// PROJECT-wide macro registry; +/// - two variables whose names canonicalize identically, the one case where +/// Stage0 records a model-level error. +fn every_shape_project() -> datamodel::Project { + let sub = x_model( + "sub", + vec![ + x_aux("input", "0", None), + x_aux("out", "input * 2", None), + x_apply_to_all("out_by_region", "Region", "input * 4"), + ], + ); + let scaled_macro = datamodel::Model { + name: "scaled".to_string(), + sim_specs: None, + variables: vec![ + x_aux("scaled", "p1 * p2", None), + x_aux("p1", "0", None), + x_aux("p2", "0", None), + ], + views: vec![], + loop_metadata: vec![], + groups: vec![], + macro_spec: Some(datamodel::MacroSpec { + parameters: vec!["p1".to_string(), "p2".to_string()], + primary_output: "scaled".to_string(), + additional_outputs: vec![], + }), + }; + let main = x_model( + "main", + vec![ + x_aux("driver", "5", None), + x_aux("smoothed", "SMTH1(driver, 3)", None), + x_module("sub", &[("driver", "sub.input")], None), + x_aux("sub_total", "SUM(sub.out_by_region[*])", None), + x_aux("scaled_driver", "scaled(driver, 2)", None), + x_aux("net flow", "1", None), + x_aux("net_flow", "2", None), + ], + ); + let mut project = x_project(sim_specs_with_units("month"), &[main, sub, scaled_macro]); + project.dimensions = vec![datamodel::Dimension::named( + "Region".to_string(), + vec!["north".to_string(), "south".to_string()], + )]; + project +} + +/// The datamodel model with the given name. +fn dm_model<'a>(project: &'a datamodel::Project, name: &str) -> &'a datamodel::Model { + project + .models + .iter() + .find(|m| m.name == name) + .unwrap_or_else(|| panic!("fixture has no model named {name}")) +} + +/// Everything the unit pass accumulates for one model, drained through the +/// direct `check_model_units::accumulated` harvest point -- the one the +/// conveyor parameter tests in `db::units` use. +fn unit_pass_diagnostics( + db: &SimlinDb, + model: SourceModel, + project: SourceProject, +) -> Vec<&CompilationDiagnostic> { + crate::db::units::check_model_units::accumulated::(db, model, project) +} + +// ── 1. value oracles ──────────────────────────────────────────────────── + +/// The cached `model_stage0` must equal what the datamodel-driven +/// `ModelStage0::new_in_project` builds for the same model. +/// +/// That constructor is the independently written twin: it derives the +/// module-ident set (`collect_module_idents` over the `datamodel::Model`), the +/// macro registry and the duplicate-ident errors (the raw declared-ident list) +/// along completely different routes than the query, which reads interned salsa +/// contexts and memoized groups. So this is a real cross-check, not a +/// restatement of the query body. +/// +/// It replaced `ModelStage0::new_cached` as this oracle when that test-only +/// third copy of the salsa-cached construction was deleted. +#[test] +fn cached_stage0_equals_datamodel_driven_constructor() { + for (fixture, project, names) in [ + ( + "three_model", + three_model_project(), + &["main", "sub_a", "sub_b"][..], + ), + ( + "arrayed_module", + arrayed_module_project(), + &["main", "sub_a", "sub_b"][..], + ), + ("chain", chain_project(), &["main", "sub_a", "sub_c"][..]), + ] { + let db = SimlinDb::default(); + let sync = sync_from_datamodel(&db, &project); + let dims = project_datamodel_dims(&db, sync.project); + let units_ctx = project_units_context(&db, sync.project); + + for name in names { + let source = sync.models[*name].source; + let oracle = ModelStage0::new_in_project( + &project.models, + dm_model(&project, name), + dims, + units_ctx, + false, + ); + assert!( + *model_stage0(&db, source, sync.project) == oracle, + "cached model_stage0 for `{name}` in fixture `{fixture}` must equal the \ + datamodel-driven build" + ); + } + } +} + +/// Every model a synced project holds -- the datamodel's own models plus the +/// stdlib models `db::sync` splices into every project -- staged by the +/// datamodel-driven constructor. +/// +/// This is the whole-project Stage0 map `Project::from_salsa` used to build +/// inline, reconstructed through the salsa-free constructor so it remains an +/// independent oracle now that `from_salsa` reads these queries instead. The +/// stdlib half is here because the scope map holds it, not because any shipped +/// template can change a lowering -- see +/// [`omitting_stdlib_models_from_the_lowering_scope_is_inert_today`]. +/// +/// BOTH halves pass the same `project.models` as the macro-registry source, so +/// one oracle scope cannot contain two models staged under different registries +/// -- the hazard `ModelStage0::new_in_project`'s rustdoc describes, which would +/// otherwise be live here since [`every_shape_project`] declares a macro. A +/// stdlib model neither declares nor calls a macro, so this changes no stdlib +/// stage today; it removes the split, not a bug. +fn datamodel_driven_stage0s( + project: &datamodel::Project, + dims: &[datamodel::Dimension], + units_ctx: &crate::units::Context, +) -> Vec { + let mut all: Vec = project + .models + .iter() + .map(|m| ModelStage0::new_in_project(&project.models, m, dims, units_ctx, false)) + .collect(); + all.extend(crate::stdlib::MODEL_NAMES.iter().map(|name| { + let dm = crate::stdlib::get(name).expect("MODEL_NAMES only names real stdlib models"); + ModelStage0::new_in_project(&project.models, &dm, dims, units_ctx, true) + })); + all +} + +/// Lower [`datamodel_driven_stage0s`] the way `Project::from_salsa` used to: +/// one whole-project `ScopeStage0` per model, keyed by canonical model name. +fn datamodel_driven_stage1s( + all_s0: &[ModelStage0], + dims_ctx: &crate::dimensions::DimensionsContext, +) -> HashMap, ModelStage1> { + let models_s0: HashMap, &ModelStage0> = + all_s0.iter().map(|m| (m.ident.clone(), m)).collect(); + all_s0 + .iter() + .map(|ms0| { + let scope = crate::model::ScopeStage0 { + models: &models_s0, + dimensions: dims_ctx, + model_name: ms0.ident.as_str(), + }; + (ms0.ident.clone(), ModelStage1::new(&scope, ms0)) + }) + .collect() +} + +/// The cached `model_stage1` must equal the datamodel-driven whole-project +/// lowering of the same models. +/// +/// This used to compare against `Project::from_salsa`'s output. That became +/// circular the moment `from_salsa` started reading this query -- it would have +/// compared a memo against a clone of itself and passed unconditionally -- so +/// the oracle moved to the salsa-free constructors, which is what `from_salsa` +/// was standing in for all along. +/// +/// Because the oracle is now a freshly built `ModelStage1` rather than one +/// `from_salsa` has already post-processed, `model_deps` and `errors` are +/// compared too; the old version had to skip them (`model_deps.take()` and +/// `set_dependencies` had already consumed and rewritten them). `instantiations` +/// is `None` on both sides -- neither construction fills it. +#[test] +fn cached_stage1_lowering_equals_datamodel_driven_lowering() { + for (fixture, project, names) in [ + ( + "three_model", + three_model_project(), + &["main", "sub_a", "sub_b"][..], + ), + ( + "arrayed_module", + arrayed_module_project(), + &["main", "sub_a", "sub_b"][..], + ), + ("chain", chain_project(), &["main", "sub_a", "sub_c"][..]), + ] { + let db = SimlinDb::default(); + let sync = sync_from_datamodel(&db, &project); + let all_s0 = datamodel_driven_stage0s( + &project, + project_datamodel_dims(&db, sync.project), + project_units_context(&db, sync.project), + ); + let oracles = + datamodel_driven_stage1s(&all_s0, project_dimensions_context(&db, sync.project)); + + for name in names { + let ident: Ident = Ident::new(name); + let cached: &ModelStage1 = model_stage1(&db, sync.models[*name].source, sync.project); + let oracle = &oracles[&ident]; + + assert_eq!(cached.name, oracle.name); + assert_eq!(cached.display_name, oracle.display_name); + assert_eq!(cached.implicit, oracle.implicit); + assert_eq!(cached.is_macro, oracle.is_macro); + assert_eq!(cached.macro_params, oracle.macro_params); + assert_eq!(cached.model_deps, oracle.model_deps); + assert_eq!(cached.errors, oracle.errors); + assert!(cached.instantiations.is_none() && oracle.instantiations.is_none()); + assert!( + cached.variables == oracle.variables, + "cached model_stage1 lowering for `{name}` in fixture `{fixture}` must equal \ + the datamodel-driven one" + ); + } + } +} + +/// Omitting the stdlib models from `model_stage1`'s LOWERING scope changes no +/// lowering TODAY -- and this is the property that makes that true, asserted +/// directly so it becomes a tripwire rather than an assumption. +/// +/// Written for whoever did the scope narrowing (the follow-on to GH #966). That +/// narrowing has landed and did NOT take the wholesale route: `model_scope_models` +/// keeps the stdlib models a lowering can actually reach, exactly as it keeps +/// any other module target, so this test now guards a road not taken. +/// +/// The reason "whole project MINUS every `stdlib⁚` model" is inert for LOWERING +/// is not missing coverage. The lowering scope has exactly two consumers, and +/// neither can observe a stdlib model: +/// +/// - `ArrayContext::get_variable` follows a `module·output` reference into the +/// target model's Stage0 to read its DIMENSIONS. Every shipped stdlib +/// variable is scalar, so the answer is `None` whether the model is in the +/// map or not. +/// - `resolve_relative` recurses through intermediate models named by a +/// dotted reference. No stdlib model instantiates a module, so none can ever +/// be an intermediate hop. +/// +/// Two routes DO reach a stdlib entry, and both are deliberately out of scope +/// for this assertion: +/// +/// - `resolve_relative`'s TERMINAL lookup. A module-input `src` spelled +/// `stdlib⁚