From 12c5f01a523433680b17f2a8290a1df25e9ca57f Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Sat, 25 Jul 2026 10:20:31 -0700 Subject: [PATCH 1/8] doc: fix discovered issues rather than filing them The "Tracking Discovered Issues" section told an agent that finding a problem mid-task obliged it to FILE the problem. That defers the cost without reducing it, and it spends the one moment when the context needed to fix the thing is already loaded. Filing becomes the exception, and both exceptions are explicit conversations rather than silent decisions: a fix too large to fold in (say so, with a cost estimate, and sequence it) and a fix that is not the agent's to make. The track-issue agent survives as the mechanism for those. The address-feedback skill's deferred-feedback step said the same thing about review comments, so it now defaults to fixing and points at the root policy. --- .claude/skills/address-feedback/SKILL.md | 2 +- CLAUDE.md | 13 ++++++++++--- 2 files changed, 11 insertions(+), 4 deletions(-) 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 From 6893927cee5d423f2992bb172f4487e89a9ca272 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Sat, 25 Jul 2026 10:25:49 -0700 Subject: [PATCH 2/8] engine: cache the two pre-layout model stages (#966) check_model_units is tracked per model, but its body rebuilt a ModelStage0 for every model in the project and lowered all of them to ModelStage1 before picking the one it came for. Collecting whole-project diagnostics therefore cost M x (whole-project reconstruction), and unit checking runs on the interactive path: editor error panels, MCP read_model/edit_model, and simlin_project_get_errors. The two stages become salsa-tracked returns(ref) queries in a new db::stages submodule, and check_model_units reads them. Cross-module unit inference still needs the full models map, so the read pattern stays O(M^2) -- but the BUILDS are now one per model per revision. A 12-model fixture builds 12 of each where the deleted code built 36, pinned by execution counters rather than memo pointer identity: salsa backdates a re-executed query whose value compares equal and can keep the memo address, so a pointer-equal memo does not prove the body did not run. The salsa-native Stage0 construction existed in three places that had silently diverged on three fields: whether a stdlib-prefixed name with an unknown suffix counts as implicit, whether a stdlib model contributes all its variable names as extra module idents, and whether duplicate-canonical-ident errors are recorded. This takes Project::from_salsa's behaviour in all three cases -- the more careful one each time -- and each was verified inert for unit checking rather than assumed: `implicit` is never read on the unit path, no stdlib body calls PREVIOUS/INIT so the module-ident set cannot change a parse, and check_model_units never reads either stage's `errors`. Forcing the old empty module-ident set red-lights exactly one test and nothing else across the suite. is_stdlib and the module-ident set are derived TOGETHER as a Stage0Context, so model_stage0 cannot obtain one without the other and the wiring has a single place to go wrong -- the earlier shape let the call site silently drop the extra idents with the whole suite staying green. The strict predicate also replaces check_model_units' bare-prefix skip gate, so a stdlib-prefixed model with an unknown suffix (a user model, not a generic template) is now unit checked. build_stdlib_models names every model from stdlib::MODEL_NAMES, so no real template can fall through the strict rule; asserted end to end. The tradeoff is memory: returns(ref) retains one Stage0 and one Stage1 per model per revision -- roughly two lowered copies of every equation in the project, name-keyed and pre-layout -- where the old code paid CPU instead. The per-variable mini ModelStage0 built during fragment lowering is deliberately NOT migrated: it is scoped to one variable's dependencies, and pointing it at a project-wide cached stage would add a project-wide dependency edge to every fragment compile. Fixes #966 Fixes #988 Fixes #989 --- src/simlin-engine/src/common.rs | 5 +- src/simlin-engine/src/db.rs | 12 + src/simlin-engine/src/db/diagnostic.rs | 6 + src/simlin-engine/src/db/stages.rs | 361 ++++++++++++++ src/simlin-engine/src/db/stages_tests.rs | 572 +++++++++++++++++++++++ src/simlin-engine/src/db/units.rs | 177 +++---- 6 files changed, 1029 insertions(+), 104 deletions(-) create mode 100644 src/simlin-engine/src/db/stages.rs create mode 100644 src/simlin-engine/src/db/stages_tests.rs diff --git a/src/simlin-engine/src/common.rs b/src/simlin-engine/src/common.rs index dbe8f14e5..0e62ceada 100644 --- a/src/simlin-engine/src/common.rs +++ b/src/simlin-engine/src/common.rs @@ -973,8 +973,9 @@ 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. +/// the gate here -- while the salsa-driven paths (`db::stages::model_stage0` +/// and, until it reads that query, `Project::from_salsa`) feed the memoized +/// `db::model_duplicate_variables` groups directly. #[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..523eebcbf 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,15 @@ 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_stage0, model_stage1, project_models_stage0, source_model_is_stdlib, +}; +// Test-only: the stage-query execution counters, so `stages_tests` can prove +// each model's stages are BUILT at most once per revision (GH #966) -- a claim +// pointer equality of a `returns(ref)` memo cannot support. +#[cfg(test)] +pub(crate) use stages::{StageExecutions, reset_stage_executions, stage_executions}; mod units; mod var_fragment; @@ -1378,6 +1388,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..53d103de8 100644 --- a/src/simlin-engine/src/db/diagnostic.rs +++ b/src/simlin-engine/src/db/diagnostic.rs @@ -24,7 +24,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. diff --git a/src/simlin-engine/src/db/stages.rs b/src/simlin-engine/src/db/stages.rs new file mode 100644 index 000000000..f3fca7b12 --- /dev/null +++ b/src/simlin-engine/src/db/stages.rs @@ -0,0 +1,361 @@ +// 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. +//! +//! This is where 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 box exists to delete. One other copy +//! survives for now: `Project::from_salsa` still builds its own stages inline, +//! and migrating it to read these queries is the follow-on commit. Where the +//! two bodies used to disagree (the stdlib `implicit` test, the stdlib +//! module-ident set, and whether duplicate-canonical-ident model errors are +//! recorded) this one takes `from_salsa`'s behaviour, so that migration is a +//! deletion rather than a merge. +//! +//! **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::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}; + +// Test-only per-thread execution counters for the two stage queries. +// +// 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. +// +// 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_stage_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) }; +} + +/// How many times each stage query body has run on this thread. +#[cfg(test)] +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +pub(crate) struct StageExecutions { + pub(crate) stage0: usize, + pub(crate) stage1: usize, +} + +/// Zero both counters (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_stage_executions() { + STAGE0_EXECUTIONS.with(|c| c.set(0)); + STAGE1_EXECUTIONS.with(|c| c.set(0)); +} + +/// Read the current counters (test-only). +#[cfg(test)] +pub(crate) fn stage_executions() -> StageExecutions { + StageExecutions { + stage0: STAGE0_EXECUTIONS.with(|c| c.get()), + stage1: STAGE1_EXECUTIONS.with(|c| c.get()), + } +} + +/// Record one entry into a stage query's body (test-only). +#[cfg(test)] +fn note_execution(counter: &'static std::thread::LocalKey>) { + counter.with(|c| c.set(c.get() + 1)); +} + +/// 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 `Project::from_salsa` has +/// always used and the rule the datamodel-driven `ModelStage0::new` uses for an +/// implicit model: one rule means the two paths hit the SAME +/// `ModuleIdentContext` and share 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), + ), + } +} + +/// Every project model's Stage0, keyed by canonical model name: 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. +pub(crate) fn project_models_stage0( + db: &dyn Db, + project: SourceProject, +) -> HashMap, &ModelStage0> { + project + .models(db) + .values() + .map(|src_model| { + let s0 = model_stage0(db, *src_model, project); + (s0.ident.clone(), s0) + }) + .collect() +} + +/// 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 carries the WHOLE-PROJECT Stage0 map because +/// `ModelStage1::new` resolves a `module·output` reference's dimensions by +/// following the module to its target model's variables. Only models this one +/// instantiates can be followed, so the map is wider than it needs to be and +/// every model's lowered stage currently depends on every other model's parse +/// results; narrowing it to the module-reachable closure is the follow-on to +/// GH #966, deliberately left out of this commit so the caching change is +/// behavior-preserving on its own. +#[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 mut models_s0 = project_models_stage0(db, project); + + // Make sure the lowering scope can see THIS model's own variables. + // + // Every caller today reaches this query through a `project.models(db)` + // entry, so the map already holds the identical memo and this insert is + // inert -- a pointer write of a value that is already there. It is here for + // the case that is not visible from the call sites: salsa handles are + // REUSED across syncs (`PersistentSyncState` threads the prior + // `SourceModel`s into the next sync), so a handle can outlive its place in + // the project's canonical-name map -- after the model is renamed, or + // deleted while something still holds it. + // + // What that would cost, concretely: 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. + models_s0.insert(model_s0.ident.clone(), model_s0); + + 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..e5d2904a4 --- /dev/null +++ b/src/simlin-engine/src/db/stages_tests.rs @@ -0,0 +1,572 @@ +// 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 constructors build -- the datamodel-driven +//! `ModelStage0::new_cached` for Stage0, and `Project::from_salsa`'s own +//! lowering for Stage1. +//! 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. +//! 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. + +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]) +} + +/// The same shape plus a dimension and an apply-to-all variable, so Stage0's +/// dimension resolution and Stage1's array lowering are exercised. +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 main = project + .models + .iter_mut() + .find(|m| m.name == "main") + .expect("fixture has a main model"); + main.variables + .push(datamodel::Variable::Aux(datamodel::Aux { + ident: "pop".to_string(), + equation: datamodel::Equation::ApplyToAll( + vec!["Region".to_string()], + "driver * 2".to_string(), + ), + documentation: String::new(), + units: None, + gf: None, + ai_state: None, + uid: None, + compat: datamodel::Compat::default(), + })); + main.variables.push(x_aux("total_pop", "SUM(pop[*])", None)); + 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_cached` builds for the same model. +/// +/// `new_cached` is an independently written constructor (it derives the +/// module-ident set and the duplicate-ident errors from the `datamodel::Model` +/// rather than from the salsa inputs), so this is a real cross-check and not a +/// restatement of the query body. It is only a valid oracle for a macro-free +/// project: `new_cached` builds a single-model `MacroRegistry`, while the query +/// reads the project-wide one. +#[test] +fn cached_stage0_equals_datamodel_driven_constructor() { + for project in [three_model_project(), arrayed_module_project()] { + 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 ["main", "sub_a", "sub_b"] { + let source = sync.models[name].source; + let oracle = ModelStage0::new_cached( + &db, + source, + sync.project, + dm_model(&project, name), + dims, + units_ctx, + false, + ); + assert!( + *model_stage0(&db, source, sync.project) == oracle, + "cached model_stage0 for `{name}` must equal the datamodel-driven build" + ); + } + } +} + +/// The cached `model_stage1`'s lowered variables must equal the ones +/// `Project::from_salsa` produces for the same model. +/// +/// Only the fields `from_salsa` leaves alone are compared: it post-processes +/// each `ModelStage1` after construction (`model_deps.take()`, then +/// `set_dependencies`, which fills `instantiations` and extends `errors`), so +/// those three fields legitimately differ. `variables` -- the lowering output +/// this query exists to cache -- is the part that must match exactly. +#[test] +fn cached_stage1_lowering_equals_project_from_salsa() { + for project in [three_model_project(), arrayed_module_project()] { + let db = SimlinDb::default(); + let sync = sync_from_datamodel(&db, &project); + let oracle_project = crate::project::Project::from(project.clone()); + + for name in ["main", "sub_a", "sub_b"] { + let ident: Ident = Ident::new(name); + let cached: &ModelStage1 = model_stage1(&db, sync.models[name].source, sync.project); + let oracle = &oracle_project.models[&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!( + cached.variables == oracle.variables, + "cached model_stage1 lowering for `{name}` must equal Project::from_salsa's" + ); + } + } +} + +// ── 2. the three Stage0 semantics decisions ───────────────────────────── + +/// A model is implicit/stdlib only when the `stdlib⁚` prefix is followed by a +/// name that is actually in `stdlib::MODEL_NAMES`. +/// +/// `db/units.rs`'s deleted `build_model_s0` used the bare prefix; the unified +/// query takes `Project::from_salsa`'s stricter rule. Both are inert for unit +/// checking today (nothing on the units path reads `implicit`), so this pins +/// the decision at the only place it is observable. +#[test] +fn model_is_stdlib_requires_a_known_stdlib_suffix() { + assert!(crate::db::stages::model_is_stdlib("stdlib\u{205A}smth1")); + assert!(crate::db::stages::model_is_stdlib("stdlib\u{205A}trend")); + // Prefix present, suffix names no stdlib model: a user model, not a + // generic template. + assert!(!crate::db::stages::model_is_stdlib("stdlib\u{205A}bogus")); + assert!(!crate::db::stages::model_is_stdlib("main")); + assert!(!crate::db::stages::model_is_stdlib("smth1")); +} + +/// Every model `db::sync` splices in satisfies the strict predicate, so +/// tightening the rule cannot orphan a real stdlib template. +/// +/// `build_stdlib_models` walks `stdlib::MODEL_NAMES` and names each model +/// `format!("stdlib\u{{205A}}{name}")`, so a spliced model's suffix is in +/// `MODEL_NAMES` by construction. This asserts that end-to-end over a synced +/// project rather than trusting the construction, since `source_model_is_stdlib` +/// now gates the unit-check skip too (GH #988). +#[test] +fn every_spliced_stdlib_model_satisfies_the_strict_predicate() { + let db = SimlinDb::default(); + let project = x_project( + sim_specs_with_units("month"), + &[x_model("main", vec![x_aux("x", "1", None)])], + ); + let sync = sync_from_datamodel(&db, &project); + + let mut seen = 0usize; + for (canonical, source) in sync.project.models(&db) { + let prefixed = canonical.starts_with("stdlib\u{205A}"); + assert_eq!( + prefixed, + crate::db::source_model_is_stdlib(&db, *source), + "spliced model '{canonical}' must be classified by the strict predicate" + ); + seen += usize::from(prefixed); + } + assert_eq!( + seen, + crate::stdlib::MODEL_NAMES.len(), + "every stdlib model should be spliced into a synced project" + ); +} + +/// The DISPLAY name is canonicalized before the predicate sees it: the +/// project's model map is canonically keyed, so an imported `Stdlib⁚Smth1` is +/// the same model as `stdlib⁚smth1` and must classify the same way. +#[test] +fn source_model_is_stdlib_canonicalizes_the_display_name() { + let db = SimlinDb::default(); + let project = x_project( + sim_specs_with_units("month"), + &[ + x_model("main", vec![x_aux("x", "1", None)]), + x_model("Stdlib\u{205A}Smth1", vec![x_aux("input", "1", None)]), + ], + ); + let sync = sync_from_datamodel(&db, &project); + let shadow = sync.models["stdlib\u{205A}smth1"].source; + assert_eq!( + shadow.name(&db), + "Stdlib\u{205A}Smth1", + "display name is raw" + ); + assert!( + crate::db::source_model_is_stdlib(&db, shadow), + "the predicate must canonicalize before testing the prefix and suffix" + ); +} + +/// A stdlib model's variable parses treat EVERY variable name as +/// module-backed; a user model's extra set is empty. +/// +/// Inert today (no stdlib body calls `PREVIOUS`/`INIT`, the only consumer of +/// the module-ident set), but load-bearing for cache sharing: the same rule in +/// both construction paths means one `ModuleIdentContext` and one set of +/// per-variable parse memos. +#[test] +fn stdlib_models_add_every_variable_name_to_the_module_ident_set() { + let db = SimlinDb::default(); + let project = three_model_project(); + let sync = sync_from_datamodel(&db, &project); + + let smth1 = sync.models["stdlib\u{205A}smth1"].source; + let mut stdlib_extra = crate::db::stages::extra_module_idents(&db, smth1, true); + stdlib_extra.sort(); + let mut expected: Vec = smth1.variables(&db).keys().cloned().collect(); + expected.sort(); + assert_eq!( + stdlib_extra, expected, + "a stdlib model contributes all of its variable names" + ); + assert!( + !expected.is_empty(), + "the fixture stdlib model has variables" + ); + + let main = sync.models["main"].source; + assert!( + crate::db::stages::extra_module_idents(&db, main, false).is_empty(), + "a user model contributes no extra module idents" + ); +} + +/// `model_stage0` actually PASSES the stdlib extra idents on -- pinned on the +/// interned `ModuleIdentContext` identity, not on a parse result. +/// +/// This is the wiring the previous version of these tests could not see. The +/// rule is inert in every stage VALUE (no stdlib body calls `PREVIOUS`/`INIT`), +/// so reverting the query's call site to `vec![]` left the whole engine suite +/// green while quietly minting a SECOND set of per-variable parse memos under a +/// different key -- losing the cache sharing with `Project::from_salsa` that is +/// the entire stated reason for the rule. `Stage0Context` is the one place that +/// wiring can now go wrong, and interned contexts compare by identity, so the +/// difference is directly observable even though the parse is not. +#[test] +fn model_stage0_parses_a_stdlib_model_under_the_extended_module_context() { + let db = SimlinDb::default(); + let project = three_model_project(); + let sync = sync_from_datamodel(&db, &project); + + let smth1 = sync.models["stdlib\u{205A}smth1"].source; + let stdlib_ctx = crate::db::stages::model_stage0_context(&db, smth1, sync.project); + assert!(stdlib_ctx.is_stdlib); + assert_ne!( + stdlib_ctx.module_idents, + model_module_ident_context(&db, smth1, sync.project, vec![]), + "a stdlib model must be parsed under an EXTENDED module-ident context, \ + not the bare one a user model gets" + ); + + // A user model adds nothing, so it must land on exactly the bare context -- + // that shared identity is what keeps one set of parse memos. + let main = sync.models["main"].source; + let main_ctx = crate::db::stages::model_stage0_context(&db, main, sync.project); + assert!(!main_ctx.is_stdlib); + assert_eq!( + main_ctx.module_idents, + model_module_ident_context(&db, main, sync.project, vec![]), + "a user model's stage context must be the bare module-ident context" + ); + + // And the query reads its `implicit` flag from that same derivation, so the + // context cannot be derived correctly while the query uses another one. + assert_eq!( + model_stage0(&db, smth1, sync.project).implicit, + stdlib_ctx.is_stdlib + ); + assert_eq!( + model_stage0(&db, main, sync.project).implicit, + main_ctx.is_stdlib + ); +} + +/// A genuine stdlib model's cached Stage0 equals the datamodel-driven +/// constructor's IMPLICIT build -- which independently applies both of the +/// decisions above (`implicit: true`, and every variable name in the +/// module-ident set). +/// +/// `npv` is the fixture rather than the more representative `smth1` because +/// `ModelStage0`'s derived `PartialEq` compares the parsed `f64` constants, and +/// every SMOOTH/DELAY/TREND template declares `initial_value = NAN`: `NaN != +/// NaN` makes those stages unequal even to a bit-identical rebuild. `npv` +/// carries no NaN literal, so equality is meaningful there. +#[test] +fn cached_stdlib_stage0_equals_implicit_datamodel_build() { + let db = SimlinDb::default(); + // Every stdlib model is spliced into every synced project, referenced or + // not, so a bare `main` is enough to reach one. + let main = x_model("main", vec![x_aux("input", "1", None)]); + let project = x_project(sim_specs_with_units("month"), &[main]); + let sync = sync_from_datamodel(&db, &project); + + let source = sync.models["stdlib\u{205A}npv"].source; + let cached = model_stage0(&db, source, sync.project); + assert!(cached.implicit, "a stdlib model's Stage0 is implicit"); + assert!( + cached.variables.len() > 1, + "the fixture stdlib model has a body to compare" + ); + + let stdlib_dm = crate::stdlib::get("npv").expect("npv is a stdlib model"); + let oracle = ModelStage0::new_cached( + &db, + source, + sync.project, + &stdlib_dm, + project_datamodel_dims(&db, sync.project), + project_units_context(&db, sync.project), + true, + ); + assert!( + *cached == oracle, + "a stdlib model's cached Stage0 must equal the implicit datamodel-driven build" + ); +} + +/// Two variables whose names canonicalize identically collapse last-wins on +/// the canonical-keyed `variables` map, so Stage0 records the collision as a +/// model-level `DuplicateVariable` error (GH #885/#891). +/// +/// `db/units.rs`'s deleted builder set `errors: None`; the unified query takes +/// `Project::from_salsa`'s recording behaviour. Nothing on the units path reads +/// `ModelStage0::errors`, so the change adds no unit diagnostic -- asserted +/// below so a future reader of the field cannot start emitting one silently. +#[test] +fn stage0_records_duplicate_canonical_ident_errors() { + let db = SimlinDb::default(); + let model = x_model( + "main", + vec![x_aux("net flow", "1", None), x_aux("net_flow", "2", None)], + ); + let project = x_project(sim_specs_with_units("month"), &[model]); + let sync = sync_from_datamodel(&db, &project); + let source = sync.models["main"].source; + + let errors = model_stage0(&db, source, sync.project) + .errors + .as_ref() + .expect("duplicate canonical idents must record a model-level error"); + let dup = errors + .iter() + .find(|e| e.code == ErrorCode::DuplicateVariable) + .unwrap_or_else(|| panic!("expected a DuplicateVariable error, got: {errors:?}")); + let msg = dup.details.as_deref().unwrap_or(""); + assert!( + msg.contains("'net flow'") && msg.contains("'net_flow'"), + "message should name both colliding spellings, got: {msg}" + ); + // The lowered stage carries it forward, and the unit pass still ignores it. + assert!(model_stage1(&db, source, sync.project).errors.is_some()); + let unit_diagnostics = unit_pass_diagnostics(&db, source, sync.project); + assert!( + !unit_diagnostics + .iter() + .any(|cd| matches!(&cd.0.error, DiagnosticError::Model(e) + if e.code == ErrorCode::DuplicateVariable)), + "the unit pass must not start reporting Stage0's model errors: {unit_diagnostics:?}" + ); +} + +// ── 3. the GH #966 cost claim ─────────────────────────────────────────── + +/// Collecting whole-project diagnostics BUILDS each model's Stage0 and Stage1 +/// exactly once, not once per (checked model, project model) pair. +/// +/// What the execution counts prove: the two query BODIES ran `models.len()` +/// times each across a whole-project diagnostic collection that runs the unit +/// pass on three models. Before this change the same collection constructed +/// `3 x models.len()` of each. The second half re-reads every model's stages +/// once per model -- the exact pre-#966 access pattern -- and shows the build +/// count does not move, so the linearity is a property of the cache and not of +/// the order `collect_all_diagnostics` happens to walk in. +/// +/// What they do NOT prove: anything about wall-clock time; anything about a +/// LATER revision (no incrementality claim is made here); and nothing about the +/// number of memo READS, which is still quadratic in the model count until the +/// lowering scope is narrowed to the module-reachable closure. +/// +/// What makes the measurement sound: `reset_stage_executions()` immediately +/// before the measured region. The counters are thread-local, which keeps a +/// PARALLEL run's other tests off this thread, but that is not on its own +/// enough -- under `--test-threads=1` libtest runs every test on this same +/// thread, so an earlier test's counts would otherwise still be sitting there. +/// The `SimlinDb` is local to this test, so no memo predates the reset either. +#[test] +fn whole_project_diagnostics_build_each_models_stages_once() { + let db = SimlinDb::default(); + let project = three_model_project(); + let sync = sync_from_datamodel(&db, &project); + let n_models = sync.project.models(&db).len(); + // Three user models plus the spliced stdlib set: the pre-#966 cost was + // 3 x n_models of each stage, so n_models and 3 x n_models are far apart. + assert!( + n_models > 3, + "fixture should have several models: {n_models}" + ); + + reset_stage_executions(); + let diagnostics = collect_all_diagnostics(&db, sync.project); + let after_collect = stage_executions(); + assert_eq!( + after_collect, + StageExecutions { + stage0: n_models, + stage1: n_models, + }, + "each model's stages must be built exactly once per revision, got {after_collect:?} \ + for {n_models} models (diagnostics: {diagnostics:?})" + ); + + // The pre-#966 access pattern, made explicit: every model's stages, once + // per model. n_models^2 reads, still n_models builds. + let sources: Vec = sync.project.models(&db).values().copied().collect(); + for _target in &sources { + for m in &sources { + let _ = model_stage1(&db, *m, sync.project); + let _ = model_stage0(&db, *m, sync.project); + } + } + assert_eq!( + stage_executions(), + after_collect, + "re-reading every model's stages must not rebuild any of them" + ); +} + +// ── 4. diagnostics did not move ───────────────────────────────────────── + +/// A project whose `main` model has a genuine dimensional inconsistency. +fn unit_mismatch_project() -> datamodel::Project { + let main = x_model( + "main", + vec![ + x_aux("a", "1", Some("widget")), + x_aux("b", "2", Some("gadget")), + x_aux("c", "a + b", Some("widget")), + ], + ); + x_project(sim_specs_with_units("month"), &[main]) +} + +/// A unit warning still reaches BOTH harvest points after the stage +/// construction moved out of `check_model_units`: the direct +/// `check_model_units::accumulated` drain (what the conveyor parameter tests +/// use) and `collect_all_diagnostics` (via `model_all_diagnostics`). +#[test] +fn unit_warning_reaches_both_harvest_points() { + let db = SimlinDb::default(); + let project = unit_mismatch_project(); + let sync = sync_from_datamodel(&db, &project); + let source = sync.models["main"].source; + + let direct = unit_pass_diagnostics(&db, source, sync.project); + assert!( + direct + .iter() + .any(|cd| matches!(&cd.0.error, DiagnosticError::Unit(_))), + "check_model_units::accumulated must still carry the unit warning: {direct:?}" + ); + + let all = collect_all_diagnostics(&db, sync.project); + assert!( + all.iter().any(|d| d.model == "main" + && d.severity == DiagnosticSeverity::Warning + && matches!(&d.error, DiagnosticError::Unit(_))), + "collect_all_diagnostics must still carry the unit warning: {all:?}" + ); +} + +/// GH #988: a model carrying the stdlib PREFIX but an unknown suffix is a user +/// model, so the unit pass checks it instead of skipping it. +/// +/// The skip gate used to accept the bare prefix, which meant an imported model +/// under the stdlib namespace silently lost unit checking while the stage query +/// -- using the strict rule -- staged it as an ordinary user model. Both now +/// call `source_model_is_stdlib`; a real stdlib model is still skipped. +#[test] +fn a_stdlib_prefixed_model_with_an_unknown_suffix_is_unit_checked() { + let db = SimlinDb::default(); + let namespaced = x_model( + "stdlib\u{205A}bogus", + vec![ + x_aux("a", "1", Some("widget")), + x_aux("b", "2", Some("gadget")), + x_aux("c", "a + b", Some("widget")), + ], + ); + let project = x_project( + sim_specs_with_units("month"), + &[x_model("main", vec![x_aux("x", "1", None)]), namespaced], + ); + let sync = sync_from_datamodel(&db, &project); + + let source = sync.models["stdlib\u{205A}bogus"].source; + let direct = unit_pass_diagnostics(&db, source, sync.project); + assert!( + direct + .iter() + .any(|cd| matches!(&cd.0.error, DiagnosticError::Unit(_))), + "a prefix-only model must be unit-checked, not skipped: {direct:?}" + ); + + // A genuine stdlib model is still skipped: it is a generic template whose + // formal parameters are unitless until instantiated. + let real = sync.models["stdlib\u{205A}smth1"].source; + assert!( + unit_pass_diagnostics(&db, real, sync.project).is_empty(), + "a real stdlib model must still be skipped by the unit pass" + ); +} diff --git a/src/simlin-engine/src/db/units.rs b/src/simlin-engine/src/db/units.rs index 34a11df24..48dfec88c 100644 --- a/src/simlin-engine/src/db/units.rs +++ b/src/simlin-engine/src/db/units.rs @@ -4,12 +4,22 @@ // pattern: Imperative Shell // -// Salsa-tracked unit-checking orchestration: it reads salsa inputs, rebuilds -// the temporary ModelStage0/ModelStage1 representations, runs the pure unit +// Salsa-tracked unit-checking orchestration: it reads the cached +// ModelStage0/ModelStage1 representations (`db::stages`), runs the pure unit // inference (`units_infer`) and consistency checking (`units_check`) cores, // and accumulates the resulting diagnostics. The dimensional-analysis logic // itself is the Functional Core in `units.rs`/`units_infer.rs`/`units_check.rs`; // this module only wires it into the salsa graph. +// +// It constructs no model stage of its own -- `db::stages` owns that -- with ONE +// deliberate carve-out: `check_conveyor_param_units` clones the cached Stage0, +// inserts its synthetic ``/``/``/leak-fraction auxes +// into the clone, and calls `ModelStage1::new` on the result. That +// `ModelStage1::new` call is NOT an unmigrated construction site and must not be +// replaced with a read of `db::stages::model_stage1`: the whole point is that +// the augmented stage is a throwaway. Those auxes exist only to be unit-checked, +// and adding them to a cached stage would feed their constraints into every +// other reader of that memo -- inference and the ordinary unit check included. //! Per-model unit inference and checking as a salsa-tracked query. //! @@ -35,9 +45,9 @@ use crate::common::{Canonical, Ident}; use crate::datamodel; use crate::db::{ CompilationDiagnostic, Db, Diagnostic, DiagnosticError, DiagnosticSeverity, SourceModel, - SourceProject, SourceVariable, model_module_ident_context, - parse_source_variable_with_module_context, project_datamodel_dims, project_dimensions_context, - project_units_context, + SourceProject, SourceVariable, model_stage0, model_stage1, project_datamodel_dims, + project_dimensions_context, project_models_stage0, project_units_context, + source_model_is_stdlib, }; /// Collect the identifiers that must share units because they sit in the @@ -153,23 +163,37 @@ fn init_value_equivalence_group( /// Per-model tracked function that performs unit inference and checking, /// accumulating unit warnings/errors through the salsa accumulator. /// -/// Builds temporary ModelStage0/ModelStage1 representations from the -/// salsa-cached parsed variables, then runs the same unit inference and -/// checking pipeline as the old `run_default_model_checks` callback. -/// Unit mismatches are accumulated as DiagnosticSeverity::Warning to -/// match the old-path behavior where unit issues don't block simulation. +/// Reads each project model's salsa-cached `ModelStage1` (`db::stages`), then +/// runs the same unit inference and checking pipeline as the old +/// `run_default_model_checks` callback. Unit mismatches are accumulated as +/// DiagnosticSeverity::Warning to match the old-path behavior where unit issues +/// don't block simulation. +/// +/// This function used to BUILD both stages for every project model on every +/// call, so collecting a project's diagnostics was quadratic in the model count +/// (GH #966). Reading the cached queries keeps the same whole-project view -- +/// cross-module inference constraints resolve submodel variable types, and +/// stdlib models stay in the map because user models reference them as modules +/// -- while each model's stages are built once per revision. /// /// Stdlib (implicit) models are skipped because they are generic /// templates that only make sense when instantiated with specific inputs. #[salsa::tracked] pub fn check_model_units(db: &dyn Db, model: SourceModel, project: SourceProject) { use crate::common::{ErrorCode, ErrorKind}; - use crate::model::{ModelStage0, ModelStage1, ScopeStage0, VariableStage0}; + use crate::model::ModelStage1; // Skip stdlib models -- they are generic and unit checking doesn't - // apply until instantiated with concrete inputs. Stdlib model names - // start with the "stdlib\u{205A}" prefix (two-dot punctuation separator). - if model.name(db).starts_with("stdlib\u{205A}") { + // apply until instantiated with concrete inputs. + // + // The test is `db::stages`', shared rather than re-spelled here (GH #988). + // This gate used to accept the bare `stdlib\u{205A}` prefix while the stage + // query additionally required the suffix to name a real stdlib model, so + // the two disagreed about an imported `stdlib\u{205A}` model: it + // was skipped here but staged as a user model there. The strict rule is the + // right one for both -- such a model is a user model, and this gate exists + // to skip generic templates, which it is not -- so it is now unit-checked. + if source_model_is_stdlib(db, model) { return; } @@ -184,73 +208,22 @@ pub fn check_model_units(db: &dyn Db, model: SourceModel, project: SourceProject let model_name = model.name(db).clone(); let units_ctx = project_units_context(db, project); - let dm_dims = project_datamodel_dims(db, project); - let dim_context = project_dimensions_context(db, project); - // Helper: build a ModelStage0 from a SourceModel's parsed variables. - let build_model_s0 = |src_model: &SourceModel, is_stdlib: bool| -> ModelStage0 { - let src_vars = src_model.variables(db); - let module_ctx = model_module_ident_context(db, *src_model, project, vec![]); - let mut var_list: Vec = Vec::new(); - let mut implicit_dm: Vec = Vec::new(); - for (_name, svar) in src_vars.iter() { - let parsed = parse_source_variable_with_module_context(db, *svar, project, module_ctx); - var_list.push(parsed.variable.clone()); - implicit_dm.extend(parsed.implicit_vars.iter().cloned()); - } - // Parse implicit vars (SMOOTH/DELAY expansion). - let mut dummy: Vec = Vec::new(); - var_list.extend(implicit_dm.into_iter().map(|dm_var| { - crate::variable::parse_var(dm_dims, &dm_var, &mut dummy, units_ctx, |mi| { - Ok(Some(mi.clone())) - }) - })); - let variables: HashMap, VariableStage0> = var_list - .into_iter() - .map(|v| (Ident::new(v.ident()), v)) - .collect(); - ModelStage0 { - ident: Ident::new(src_model.name(db)), - display_name: src_model.name(db).clone(), - variables, - errors: None, - implicit: is_stdlib, - is_macro: src_model.macro_spec(db).is_some(), - macro_params: crate::model::macro_param_idents(src_model.macro_spec(db).as_ref()), - } - }; - - // Build ModelStage0 for all project models so that cross-module unit + // Read every project model's lowered stage so that cross-module unit // inference constraints (module inputs/outputs) can resolve submodel // variable types. Stdlib models are included in the map because user // models may reference them as modules. - let project_models = project.models(db); - let mut all_s0: Vec = Vec::new(); - for (name, src_model) in project_models.iter() { - let is_stdlib = name.starts_with("stdlib\u{205A}"); - all_s0.push(build_model_s0(src_model, is_stdlib)); - } - - let models_s0: HashMap, &ModelStage0> = - all_s0.iter().map(|m| (m.ident.clone(), m)).collect(); - - // Lower all ModelStage0 -> ModelStage1. - let all_s1: Vec = all_s0 - .iter() - .map(|ms0| { - let scope = ScopeStage0 { - models: &models_s0, - dimensions: dim_context, - model_name: ms0.ident.as_str(), - }; - ModelStage1::new(&scope, ms0) + let models_s1: HashMap, &ModelStage1> = project + .models(db) + .values() + .map(|src_model| { + let s1 = model_stage1(db, *src_model, project); + (s1.name.clone(), s1) }) .collect(); - let models_s1: HashMap, &ModelStage1> = - all_s1.iter().map(|m| (m.name.clone(), m)).collect(); - - // Find the target model in the lowered map. + // Find the target model in the lowered map. A `SourceModel` that is not the + // project's entry under its own canonical name has nothing to check here. let target_ident = Ident::::new(&model_name); let target_model = match models_s1.get(&target_ident) { Some(m) => *m, @@ -452,20 +425,15 @@ pub fn check_model_units(db: &dyn Db, model: SourceModel, project: SourceProject // here with the datamodel + lowering machinery already in hand. Runs after // the ordinary unit check so a conveyor model's ordinary variables are still // checked exactly as before. - if let Some(target_ms0) = all_s0.iter().find(|m| m.ident == target_ident) { - check_conveyor_param_units( - db, - model, - &model_name, - target_model, - target_ms0, - &models_s0, - dim_context, - dm_dims, - units_ctx, - &inferred_units, - ); - } + check_conveyor_param_units( + db, + model, + project, + &model_name, + target_model, + units_ctx, + &inferred_units, + ); } /// Unit-check a model's conveyor block parameters (docs/design/conveyors.md @@ -498,19 +466,17 @@ pub fn check_model_units(db: &dyn Db, model: SourceModel, project: SourceProject /// "unknown units are skipped" rule elsewhere in unit checking. Likewise a /// parameter whose expression reads a variable with unknown units is skipped /// (a `DoesNotExist` verdict), never reported as a mismatch. -// The salsa keys, the two model stages, and the four project-wide contexts are all -// already resolved by the single caller (`check_model_units`); re-deriving any of -// them here would re-run a salsa query per conveyor stock. -#[allow(clippy::too_many_arguments)] +// The lowered target model, its model name and the inferred-units map are all +// already resolved by the single caller (`check_model_units`), so they are +// passed rather than re-derived. The Stage0 scope and the dimension queries are +// read here instead, ONCE, and only past the early return below: a model with no +// conveyor -- almost every model -- then pays nothing for them at all. fn check_conveyor_param_units( db: &dyn Db, model: SourceModel, + project: SourceProject, model_name: &str, target_model: &crate::model::ModelStage1, - target_ms0: &crate::model::ModelStage0, - models_s0: &HashMap, &crate::model::ModelStage0>, - dim_context: &crate::dimensions::DimensionsContext, - dm_dims: &[datamodel::Dimension], units_ctx: &crate::units::Context, inferred_units: &HashMap, crate::datamodel::UnitMap>, ) { @@ -662,10 +628,13 @@ fn check_conveyor_param_units( } // Lower every synthesized parameter aux in the target model's context. We - // build a throwaway augmented ModelStage1 rather than perturbing the real - // `target_model` (which drives inference and the ordinary unit check): the - // synthetic auxes must not add constraints to the model under analysis. - let mut aug_ms0 = target_ms0.clone(); + // build a throwaway augmented ModelStage1 from a CLONE of the cached Stage0 + // rather than perturbing either the real `target_model` (which drives + // inference and the ordinary unit check) or the cached stage itself: the + // synthetic auxes must not add constraints to the model under analysis, and + // must never reach another reader of the memo. + let dm_dims = project_datamodel_dims(db, project); + let mut aug_ms0 = model_stage0(db, model, project).clone(); for dm_var in &synth_dm_vars { let mut dummy: Vec = Vec::new(); let vs0 = crate::variable::parse_var(dm_dims, dm_var, &mut dummy, units_ctx, |mi| { @@ -673,9 +642,13 @@ fn check_conveyor_param_units( }); aug_ms0.variables.insert(Ident::new(vs0.ident()), vs0); } + // The scope holds each model's UNAUGMENTED Stage0 -- including the target's, + // so the synthetic auxes stay invisible to dimension resolution exactly as + // they were before the stages were cached. + let models_s0 = project_models_stage0(db, project); let scope = crate::model::ScopeStage0 { - models: models_s0, - dimensions: dim_context, + models: &models_s0, + dimensions: project_dimensions_context(db, project), model_name: aug_ms0.ident.as_str(), }; let aug_s1 = crate::model::ModelStage1::new(&scope, &aug_ms0); From bbe3d756e0b0d2cb80eab17ad5e65aa4734a4c86 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Sat, 25 Jul 2026 10:26:30 -0700 Subject: [PATCH 3/8] engine: take project-level diagnostics off the accumulator Two project-level facts -- the macro-registry build error and the unit definition errors -- were accumulated from inside the bodies of project_macro_registry and project_units_context. Both had the same two defects, and the second one is a silent drop. They were reported once per MODEL that happened to reach the query: 15 copies of DuplicateMacroName in a 15-model project, for a fact that belongs to no model. Every existing test asserted with .any(), so N copies and one copy were indistinguishable and the suite could not see it. Worse, they VANISHED after any unrelated edit. Measured on a 13-model fixture: 13 reports before an unrelated input bump, zero after -- through simlin_project_get_errors, the CLI, both MCP surfaces, and simlin-serve. A project with a broken macro set simply stopped being reported. The cause is that salsa's accumulated() walk prunes a dependency subtree whose root memo reports no accumulated inputs, so an error accumulated by a query that some unrelated subtree happens to reach is not a diagnostic anyone can rely on. Both errors become memoized VALUES that collect_all_diagnostics emits once, the way emit_duplicate_variable_diagnostics already handles duplicate idents. project_units_context_result carries the context and its definition errors together so the two cannot come from separate builds, and Project::from_salsa reads that value instead of draining the accumulator -- it had the same vanishing bug in Project.errors. project_units_context stays a TRACKED PROJECTION over that result rather than becoming a plain accessor, which is load bearing. A malformed unit equation is dropped from the context entirely, so editing one bad equation into a different bad equation changes the error list while the context stays byte-identical. As a plain accessor every one of its ~30 callers would then be invalidated -- every keystroke while typing a unit equation re-parsing every variable in the project. As a tracked projection salsa backdates the equal context and readers keep exactly the dependency width they had. Adding no_eq to the projection fails the regression test even though the returned context is still equal, which isolates value-equality backdating as the mechanism. The module-reference cycle stays per-model on purpose (GH #806): a diagnostic naming the model that reaches the cycle, not one project-level row, so an unrelated draft cycle elsewhere cannot hide a valid model's errors. Every production surface that reports project errors was traced by call chain and still receives both: libsimlin (get_errors, the three apply_patch passes), simlin-cli, simlin-mcp-core read_model/edit_model, simlin-serve, the wasm/TypeScript path, pysimlin, compile_project_incremental's hard fail, and Project::from_salsa. collect_model_diagnostics has no production caller. --- src/libsimlin/src/patch.rs | 9 + src/simlin-engine/CLAUDE.md | 10 +- src/simlin-engine/src/db.rs | 11 +- src/simlin-engine/src/db/diagnostic.rs | 83 +++++- src/simlin-engine/src/db/diagnostic_tests.rs | 249 ++++++++++++++++++ .../src/db/dimension_invalidation_tests.rs | 95 ++++++- src/simlin-engine/src/db/macro_registry.rs | 52 ++-- src/simlin-engine/src/db/query.rs | 95 +++++-- src/simlin-engine/src/project.rs | 48 ++-- 9 files changed, 555 insertions(+), 97 deletions(-) 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..0adce719a 100644 --- a/src/simlin-engine/CLAUDE.md +++ b/src/simlin-engine/CLAUDE.md @@ -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`, 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 where they are built from salsa inputs; `Project::from_salsa` still carries an inline copy that the #966 follow-on deletes, and where the two bodies used to disagree this one takes `from_salsa`'s behaviour so that deletion is not a merge. `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 every model's Stage0. 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 this path and `Project::from_salsa` hit the same interned context and share 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. The Stage1 lowering scope is still WHOLE-PROJECT, so every model's lowered stage depends on every other model's parses -- narrowing it to the module-reachable closure is the follow-on to #966. Test-only thread-local execution counters (`reset_stage_executions`/`stage_executions`) 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 independently-written constructors (`ModelStage0::new_cached` for Stage0, `Project::from_salsa`'s own lowering for Stage1's `variables`), the three Stage0 semantics decisions above, the GH #966 cost claim (a whole-project `collect_all_diagnostics` BUILDS each model's Stage0/Stage1 exactly once, and re-reading every model's stages once per model -- the pre-#966 access pattern -- rebuilds none), 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). Note the stdlib oracle uses `npv`, not `smth1`: `ModelStage0`'s derived `PartialEq` compares parsed `f64` constants and every SMOOTH/DELAY/TREND template declares `initial_value = NAN`, so those stages are unequal even to a bit-identical rebuild. - **`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. 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) 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 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. diff --git a/src/simlin-engine/src/db.rs b/src/simlin-engine/src/db.rs index 523eebcbf..8dc0a4b6f 100644 --- a/src/simlin-engine/src/db.rs +++ b/src/simlin-engine/src/db.rs @@ -78,12 +78,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; @@ -1172,8 +1172,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 { diff --git a/src/simlin-engine/src/db/diagnostic.rs b/src/simlin-engine/src/db/diagnostic.rs index 53d103de8..2d3ccffcd 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, @@ -858,10 +886,53 @@ 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(); + + // 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: String::new(), + variable: Some(unit_name.clone()), + error: DiagnosticError::Unit(UnitError::DefinitionError(eq_err.clone(), None)), + severity: DiagnosticSeverity::Error, + }); + } + } + + // 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 (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 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..e4459c076 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_stage_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!( + stage_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_stage_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!( + stage_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..5a6621b1b 100644 --- a/src/simlin-engine/src/db/macro_registry.rs +++ b/src/simlin-engine/src/db/macro_registry.rs @@ -38,13 +38,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. @@ -177,14 +172,26 @@ 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. +/// datamodel `Vec`. When the build fails the error is returned in +/// `build_error`, 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. +/// +/// 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 +202,8 @@ 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); return MacroRegistryResult { registry: crate::module_functions::MacroRegistry::default(), build_error: Some((code, message)), diff --git a/src/simlin-engine/src/db/query.rs b/src/simlin-engine/src/db/query.rs index 380d447f9..497000e5b 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. diff --git a/src/simlin-engine/src/project.rs b/src/simlin-engine/src/project.rs index 6e83037b3..ebd230f03 100644 --- a/src/simlin-engine/src/project.rs +++ b/src/simlin-engine/src/project.rs @@ -21,9 +21,10 @@ pub struct Project { pub models: HashMap, Arc>, #[allow(dead_code)] model_order: Vec>, - /// Project-level errors. With the `from_salsa` construction path, - /// unit definition errors are recovered from the salsa accumulator - /// in `project_units_context` so callers can still inspect them. + /// Project-level errors. With the `from_salsa` construction path, unit + /// definition errors are read off `UnitsContextResult::definition_errors` + /// -- the same memoized derivation `collect_all_diagnostics` reports from + /// -- so callers can inspect them here and get the same set. pub errors: Vec, /// Cached dimension context for subdimension lookups pub dimensions_ctx: DimensionsContext, @@ -72,31 +73,34 @@ impl Project { { use crate::common::{ErrorCode, ErrorKind, topo_sort}; use crate::db::{ - CompilationDiagnostic, DiagnosticError, model_module_ident_context, - parse_source_variable_with_module_context, project_datamodel_dims, - project_dimensions_context, project_units_context, + model_module_ident_context, parse_source_variable_with_module_context, + project_datamodel_dims, project_dimensions_context, project_units_context_result, }; use crate::model::{ModelStage0, VariableStage0, enumerate_modules}; - let units_ctx = project_units_context(db, source_project); + let units_result = project_units_context_result(db, source_project); + let units_ctx = &units_result.ctx; - // Recover unit definition errors from the salsa accumulator so - // callers that inspect Project.errors (e.g. tests) still see them. - let project_errors: Vec = - project_units_context::accumulated::(db, source_project) - .into_iter() - .filter_map(|cd| match &cd.0.error { - DiagnosticError::Unit(unit_err) => { - let name = cd.0.variable.as_deref().unwrap_or("unknown"); - Some(Error { - kind: ErrorKind::Model, - code: ErrorCode::UnitDefinitionErrors, - details: Some(format!("{name}: {unit_err}")), - }) + // Unit definition errors come off the memoized result, so callers that + // inspect `Project.errors` (e.g. tests) see the same set + // `collect_all_diagnostics` reports -- and see it deterministically. + // They used to be recovered by draining the salsa accumulator, which + // returned them once per reachable model and returned NOTHING at all + // once an unrelated revision bump let the DFS prune the subtree. + let project_errors: Vec = units_result + .definition_errors + .iter() + .flat_map(|(name, eq_errors)| { + eq_errors.iter().map(move |eq_err| { + let unit_err = crate::common::UnitError::DefinitionError(eq_err.clone(), None); + Error { + kind: ErrorKind::Model, + code: ErrorCode::UnitDefinitionErrors, + details: Some(format!("{name}: {unit_err}")), } - _ => None, }) - .collect(); + }) + .collect(); let dm_dims = project_datamodel_dims(db, source_project); // Read the project-global dimension context from the salsa-cached query // rather than rebuilding it here (it is canonicalized once per project). From f3599a42ed58f4625b48a66cae71c028ebe1094f Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Sat, 25 Jul 2026 11:52:32 -0700 Subject: [PATCH 4/8] engine: one salsa-native construction of the model stages The salsa-native ModelStage0 build existed in three places. #966 made db::stages the cached one; this deletes the other two: Project::from_salsa's inline whole-project build (which parsed every variable, re-parsed the SMOOTH/DELAY implicit vars, and lowered the whole project a second time) and the test-only ModelStage0::new_cached. from_salsa now reads model_stage1 and clones. The clone is mandatory rather than stylistic: set_dependencies takes the errors, fills instantiations, and pushes equation errors onto the variables themselves, so it needs an owned value the cache must not see. Equality with the deleted build was measured, not argued. The inline build was temporarily restored as an oracle and compared field by field against the queries across three fixtures and every model of each -- including the stdlib templates -- then removed; the review reproduced it independently over 36 model-instances. The two builds agree because they were already computing the same thing from the same inputs: the map key from_salsa tested for `implicit` IS canonicalize(display_name), both read the same variables map, and model_module_ident_context sorts and dedups so list order cannot matter. The whole-project scope map in model_stage1 turned out to have NO test coverage: replacing it with an empty map left all 5299 tests green, while every arrayed cross-module reference silently lowered as if it were scalar -- `get_dimensions` returns None for a model missing from the scope, and a None dimension is not an error. That matters because narrowing this map is the next box, so it would have been narrowed with nothing watching. Three fixtures now constrain it: an arrayed read through one module hop (catches an empty map), a two-level chain `main -> sub_a -> sub_c` reducing over the grandchild's arrayed output (catches a direct-targets-only closure, which the one-hop fixture cannot see), and a tripwire asserting no synced stdlib Stage0 is arrayed or holds a module instance -- which is WHY a stdlib-omitting closure is safe today, and which fails the moment that stops being true. ModelStage0::new gained a new_in_project sibling that builds its MacroRegistry from all project models rather than from the single model, so an oracle cannot silently disagree with the query about whether a sibling-defined macro call expands. `new` is the single-model wrapper, so its eight callers are unchanged. Project::from_salsa was also nondeterministic run to run -- model order and each ModuleStage2's initials runlist flipped between two builds in one process, because topo_sort was seeded from HashMap keys and the initials runlist from a HashSet. This is the Project-path analogue of GH #595, fixed the same way, by sorting both seeds. Verified test-only before touching an ordering: from_salsa is the only non-test caller of set_dependencies, and the only reader of ModelStage1::instantiations is compiler::Module::new, whose single caller is a cfg(test) helper. Pinned by a 64-run determinism test; 8 runs was measurably flaky. from_salsa itself is test-only (plus one engine integration test that feeds ltm_finding::discover_loops); the plan calls it the live-oracle monolith, and its rustdoc now says so, since "is this production code" was not answerable from the call sites. --- src/simlin-engine/CLAUDE.md | 8 +- src/simlin-engine/src/common.rs | 20 +- src/simlin-engine/src/db/stages.rs | 75 ++- src/simlin-engine/src/db/stages_tests.rs | 573 +++++++++++++++++++++-- src/simlin-engine/src/model.rs | 235 ++++------ src/simlin-engine/src/project.rs | 248 ++++++---- 6 files changed, 833 insertions(+), 326 deletions(-) diff --git a/src/simlin-engine/CLAUDE.md b/src/simlin-engine/CLAUDE.md index 0adce719a..89993ff5d 100644 --- a/src/simlin-engine/CLAUDE.md +++ b/src/simlin-engine/CLAUDE.md @@ -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. +- **`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 @@ -104,8 +104,8 @@ The primary compilation path uses salsa tracked functions for fine-grained incre - **`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_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`, 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 where they are built from salsa inputs; `Project::from_salsa` still carries an inline copy that the #966 follow-on deletes, and where the two bodies used to disagree this one takes `from_salsa`'s behaviour so that deletion is not a merge. `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 every model's Stage0. 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 this path and `Project::from_salsa` hit the same interned context and share 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. The Stage1 lowering scope is still WHOLE-PROJECT, so every model's lowered stage depends on every other model's parses -- narrowing it to the module-reachable closure is the follow-on to #966. Test-only thread-local execution counters (`reset_stage_executions`/`stage_executions`) 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 independently-written constructors (`ModelStage0::new_cached` for Stage0, `Project::from_salsa`'s own lowering for Stage1's `variables`), the three Stage0 semantics decisions above, the GH #966 cost claim (a whole-project `collect_all_diagnostics` BUILDS each model's Stage0/Stage1 exactly once, and re-reading every model's stages once per model -- the pre-#966 access pattern -- rebuilds none), 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). Note the stdlib oracle uses `npv`, not `smth1`: `ModelStage0`'s derived `PartialEq` compares parsed `f64` constants and every SMOOTH/DELAY/TREND template declares `initial_value = NAN`, so those stages are unequal even to a bit-identical rebuild. +- **`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 every model's Stage0. 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. The Stage1 lowering scope is still WHOLE-PROJECT, so every model's lowered stage depends on every other model's parses -- narrowing it to the module-reachable closure is the follow-on to #966, and three fixtures in `stages_tests` are what can see that 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 is currently harmless, 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). Test-only thread-local execution counters (`reset_stage_executions`/`stage_executions`) 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 exactly once, and re-reading every model's stages once per model -- the pre-#966 access pattern -- 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). 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 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. 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. diff --git a/src/simlin-engine/src/common.rs b/src/simlin-engine/src/common.rs index 0e62ceada..9529c4be1 100644 --- a/src/simlin-engine/src/common.rs +++ b/src/simlin-engine/src/common.rs @@ -944,11 +944,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,10 +972,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 paths (`db::stages::model_stage0` -/// and, until it reads that query, `Project::from_salsa`) feed 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/stages.rs b/src/simlin-engine/src/db/stages.rs index f3fca7b12..f2afea6a8 100644 --- a/src/simlin-engine/src/db/stages.rs +++ b/src/simlin-engine/src/db/stages.rs @@ -20,16 +20,42 @@ //! the model count (GH #966). Caching them here as `returns(ref)` queries makes //! each model's stages one memoized value that consumers READ. //! -//! This is where 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 box exists to delete. One other copy -//! survives for now: `Project::from_salsa` still builds its own stages inline, -//! and migrating it to read these queries is the follow-on commit. Where the -//! two bodies used to disagree (the stdlib `implicit` test, the stdlib -//! module-ident set, and whether duplicate-canonical-ident model errors are -//! recorded) this one takes `from_salsa`'s behaviour, so that migration is a -//! deletion rather than a merge. +//! 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 @@ -168,11 +194,13 @@ pub(crate) fn source_model_is_stdlib(db: &dyn Db, model: SourceModel) -> bool { /// 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 `Project::from_salsa` has -/// always used and the rule the datamodel-driven `ModelStage0::new` uses for an -/// implicit model: one rule means the two paths hit the SAME -/// `ModuleIdentContext` and share one `parse_source_variable_with_module_context` -/// cache entry, instead of minting a second set of parses under a second key. +/// 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. @@ -321,6 +349,23 @@ pub(crate) fn model_stage0(db: &dyn Db, model: SourceModel, project: SourceProje /// results; narrowing it to the module-reachable closure is the follow-on to /// GH #966, deliberately left out of this commit so the caching change is /// behavior-preserving on its own. +/// +/// Anyone doing that narrowing: three fixtures in `db::stages_tests` are what +/// can see it going wrong, and they separate the mistakes it 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 is currently harmless, and +/// that test asserts the precise reason (no stdlib template is arrayed or +/// instantiates a module). It fails the moment that stops being true, which +/// is the moment such a narrowing would start mis-lowering silently. #[salsa::tracked(returns(ref))] pub(crate) fn model_stage1(db: &dyn Db, model: SourceModel, project: SourceProject) -> ModelStage1 { #[cfg(test)] diff --git a/src/simlin-engine/src/db/stages_tests.rs b/src/simlin-engine/src/db/stages_tests.rs index e5d2904a4..8c83f1f3e 100644 --- a/src/simlin-engine/src/db/stages_tests.rs +++ b/src/simlin-engine/src/db/stages_tests.rs @@ -7,14 +7,45 @@ //! Three claims are pinned here: //! //! 1. **Value**: the cached `model_stage0` / `model_stage1` equal what the -//! independently-written constructors build -- the datamodel-driven -//! `ModelStage0::new_cached` for Stage0, and `Project::from_salsa`'s own -//! lowering for Stage1. +//! 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. +//! 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}; @@ -47,34 +78,167 @@ fn three_model_project() -> datamodel::Project { x_project(sim_specs_with_units("month"), &[main, sub_a, sub_b]) } -/// The same shape plus a dimension and an apply-to-all variable, so Stage0's +/// 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(datamodel::Variable::Aux(datamodel::Aux { - ident: "pop".to_string(), - equation: datamodel::Equation::ApplyToAll( - vec!["Region".to_string()], - "driver * 2".to_string(), - ), - documentation: String::new(), - units: None, - gf: None, - ai_state: None, - uid: None, - compat: datamodel::Compat::default(), - })); + .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 } @@ -101,28 +265,41 @@ fn unit_pass_diagnostics( // ── 1. value oracles ──────────────────────────────────────────────────── /// The cached `model_stage0` must equal what the datamodel-driven -/// `ModelStage0::new_cached` builds for the same model. +/// `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. /// -/// `new_cached` is an independently written constructor (it derives the -/// module-ident set and the duplicate-ident errors from the `datamodel::Model` -/// rather than from the salsa inputs), so this is a real cross-check and not a -/// restatement of the query body. It is only a valid oracle for a macro-free -/// project: `new_cached` builds a single-model `MacroRegistry`, while the query -/// reads the project-wide one. +/// 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 project in [three_model_project(), arrayed_module_project()] { + 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 ["main", "sub_a", "sub_b"] { - let source = sync.models[name].source; - let oracle = ModelStage0::new_cached( - &db, - source, - 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, @@ -130,45 +307,292 @@ fn cached_stage0_equals_datamodel_driven_constructor() { ); assert!( *model_stage0(&db, source, sync.project) == oracle, - "cached model_stage0 for `{name}` must equal the datamodel-driven build" + "cached model_stage0 for `{name}` in fixture `{fixture}` must equal the \ + datamodel-driven build" ); } } } -/// The cached `model_stage1`'s lowered variables must equal the ones -/// `Project::from_salsa` produces for the same model. +/// 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. /// -/// Only the fields `from_salsa` leaves alone are compared: it post-processes -/// each `ModelStage1` after construction (`model_deps.take()`, then -/// `set_dependencies`, which fills `instantiations` and extends `errors`), so -/// those three fields legitimately differ. `variables` -- the lowering output -/// this query exists to cache -- is the part that must match exactly. +/// 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_project_from_salsa() { - for project in [three_model_project(), arrayed_module_project()] { +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 oracle_project = crate::project::Project::from(project.clone()); + 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 ["main", "sub_a", "sub_b"] { + for name in names { let ident: Ident = Ident::new(name); - let cached: &ModelStage1 = model_stage1(&db, sync.models[name].source, sync.project); - let oracle = &oracle_project.models[&ident]; + 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}` must equal Project::from_salsa's" + "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 does the scope narrowing (the follow-on to GH #966). +/// Four plausible narrowings were probed against the full workspace; "whole +/// project MINUS every `stdlib⁚` model" came back green, and the reason is not +/// missing coverage. The scope map 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. +/// +/// One route DOES reach a stdlib entry, and it is deliberately out of scope +/// here: `resolve_relative`'s TERMINAL lookup. A module-input `src` spelled +/// `stdlib⁚