diff --git a/Cargo.lock b/Cargo.lock index ee1694b26..de6d8942e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3802,6 +3802,7 @@ dependencies = [ "simlin", "simlin-engine", "stringreader", + "tempfile", ] [[package]] diff --git a/docs/implementation-plans/2026-05-19-clearn-residual/phase_01.md b/docs/implementation-plans/2026-05-19-clearn-residual/phase_01.md new file mode 100644 index 000000000..4828f4cae --- /dev/null +++ b/docs/implementation-plans/2026-05-19-clearn-residual/phase_01.md @@ -0,0 +1,176 @@ +# C-LEARN Residual — Phase 1: Lookup-only variable saved-value semantics (#590) + +**Goal:** A standalone graphical-function ("lookup-only") variable — one whose entire equation is an inline table with no functional argument — produces the same saved series as genuine Vensim, uniformly across scalar, arrayed (`Equation::Arrayed`), and apply-to-all (A2A) shapes, resolving the inconsistent `gf(0)`-vs-literal-`0` lowering. + +**Architecture:** Today the compiler lowers a lookup-only variable inconsistently: the scalar path wraps it as `LOOKUP(self, 0)` → a constant `gf(0)`, while the arrayed and A2A hoisting paths emit a literal `0` and never consult the attached tables. The fix is confined to `compiler::Var::new` (`src/simlin-engine/src/compiler/mod.rs`): detect lookup-only variables and lower all three shapes uniformly to the empirically-determined Vensim rule (leading hypothesis: `gf(Time)`), reusing the per-element table layout (`variable.rs::build_tables`/`reorder_arrayed_element_tables`) and the existing `Lookup` opcode. No new datamodel field, no protobuf change, no new VM/codegen primitive. Because the production salsa-incremental compile path (`db_var_fragment.rs::lower_var_fragment`) reuses `compiler::Var::new`, the fix applies to both the legacy and incremental paths automatically. + +**Tech Stack:** Rust (`simlin-engine` crate). Fixtures build `datamodel::Project` directly (the `per_element_gf_tests.rs` pattern), because `TestProject` cannot attach graphical functions. + +**Scope:** Phase 1 of 5 from `docs/design-plans/2026-05-19-clearn-residual.md`. + +**Codebase verified:** 2026-05-20 (branch `clearn-residual`, off `main`@`2ed93950`). + +--- + +## Acceptance Criteria Coverage + +This phase implements and tests: + +### clearn-residual.AC1: Lookup-only (graphical-function) variables produce Vensim-matching saved series +- **clearn-residual.AC1.1 Success:** A scalar lookup-only variable (equation is an inline graphical function, no functional argument) produces the saved series determined to match genuine Vensim (per the Phase 1 determination), not a constant `gf(0)`. +- **clearn-residual.AC1.2 Success:** An arrayed (`Equation::Arrayed`) lookup-only variable produces the correct per-element saved series for every element, not literal `0`. +- **clearn-residual.AC1.3 Success:** An apply-to-all lookup-only variable produces the correct saved series for every element. +- **clearn-residual.AC1.4 Success (no regression):** A graphical-function variable that is applied with an argument elsewhere (`var(idx)` → `LOOKUP(var, idx)`) still produces correct applied values. +- **clearn-residual.AC1.5 Edge:** A lookup-only variable whose declared element order is non-alphabetical maps each element to its own table (no positional mis-map; consistent with the `per_element_gf_tests.rs` invariant). +- **clearn-residual.AC1.6 Failure/robustness:** Out-of-range lookup-index semantics for the scalar `Lookup` opcode are unchanged by the fix (the fix changes which expression is fed to the table, not the table's clamp/NaN behavior). + +--- + +## Verified ground truth (read before starting) + +Confirmed by investigation on 2026-05-20. Trust these over the design's line numbers. + +- **The compiler never sees the `0+0` sentinel string as a signal.** `LOOKUP_SENTINEL = "0+0"` (`src/simlin-engine/src/mdl/mod.rs:41`) is MDL-format-internal — written by the importer and read only by the MDL writer's round-trip check `is_lookup_only_equation` (`src/simlin-engine/src/mdl/writer.rs:831-834`). It is NOT a stable datamodel contract: an XMILE-sourced lookup-only variable carries an empty/absent equation + a `gf`, not `"0+0"`. **Detection must treat "equation is empty-trimmed OR equals `LOOKUP_SENTINEL`" together with "has a graphical function (`tables` non-empty)".** +- **Datamodel distinction** (set at MDL import, `src/simlin-engine/src/mdl/convert/variables.rs`): + - lookup-only (`MdlEquation::Lookup`, :666-670): `equation = LOOKUP_SENTINEL`, `gf = Some(..)`. + - WITH LOOKUP (`MdlEquation::WithLookup`, :671-675): `equation = `, `gf = Some(..)`. + - empty/no-data (`MdlEquation::EmptyRhs`, :676): `equation = LOOKUP_SENTINEL`, `gf = None`. + So lookup-only ⇔ `gf.is_some()` (i.e. compiler `tables` non-empty) AND equation is empty-or-sentinel; WITH LOOKUP has `tables` non-empty but a real input equation and must keep `gf(input)`. +- **`is_table_only` is a dead end.** The compiler-side `Variable::Var.is_table_only` (`src/simlin-engine/src/variable.rs:104`) is hard-coded `false` everywhere and is NOT present in the datamodel, the protobuf (`src/simlin-engine/src/project_io.proto` `message Aux`/`message Flow`), serde, or json. Do NOT route detection through it (would require a protobuf schema change). Detect via `tables` + equation form instead. +- **Scalar lowering** (`src/simlin-engine/src/compiler/mod.rs`): the `Variable::Var { tables, .. }` arm at `:770`; scalar sub-arm `:781-801`. For any scalar Var with non-empty `tables`, it wraps `Expr::App(BuiltinFn::Lookup(Box::new(Expr::Var(off, loc)), Box::new(main_expr), loc), loc)` where `main_expr` is the lowered equation. For lookup-only, `main_expr` is the lowered `"0+0"` = constant `0` → `gf(0)`. For WITH LOOKUP, `main_expr` is the real input → `gf(input)`. +- **Arrayed lowering** (`expand_arrayed_with_hoisting`, `compiler/mod.rs:1605`): the non-array-producing else-branch (`:1650-1683`) emits `Expr::AssignCurr(off + i, main_expr)` with `main_expr` = constant `0` for lookup-only — no `LOOKUP` wrap, tables ignored. +- **A2A lowering** (`expand_a2a_with_hoisting`, `compiler/mod.rs:1692`): the fallback per-element loop (`:1719-1736`) likewise emits `Expr::AssignCurr(off + i, main_expr=0)` — no `LOOKUP` wrap, tables ignored. +- **Table layout (already correct)**: `variable.rs::build_tables` (`:369-428`) produces `tables.len() == n` for arrayed-with-per-element-gfs (laid out in row-major declared order via `reorder_arrayed_element_tables`, `:346-358`) and `tables.len() == 1` for A2A or arrayed-without-per-element-gfs (single variable-level table). The per-element index `i` in the lowering loops is the same row-major `SubscriptIterator` index used by `build_tables`, so element `i`'s table is at `tables[i]` → `graphical_functions[base_gf + i]`. +- **The `Lookup` opcode** (`src/simlin-engine/src/vm.rs:1507-1528`) pops `lookup_index` then `element_offset`, indexes `graphical_functions[base_gf + element_offset]`, and pushes `NaN` if `element_offset < 0 || element_offset >= table_count`. `codegen.rs::extract_table_info` (`:593-610`) computes `elem_off = off − base_off` from the `Expr::Var(off)` fed to `Lookup`, and `table_count` (`codegen.rs:730-735`) = the table var's table count. +- **Current sim time as an `Expr`**: `Expr::App(BuiltinFn::Time, loc)`. `codegen.rs:836-850` lowers `BuiltinFn::Time` to `Opcode::LoadGlobalVar { off: TIME_OFF }` with `TIME_OFF = 0` (`vm.rs:83`). Precedent: `MdlEquation::Implicit` (`mdl/convert/variables.rs:677-680`) already produces a `Time`-indexed lookup (`equation = "TIME"`, `gf = Some(..)`). +- **`TestProject` cannot attach graphical functions** (`src/simlin-engine/src/test_common.rs:20`; every builder sets `gf: None`). Build `datamodel::Project` directly. The template is `arrayed_gf_project` in `src/simlin-engine/src/per_element_gf_tests.rs:66-131` (a `#[cfg(test)]` module in `src/`, NOT `tests/`); `ramp_gf(base, slope)` (`:33-44`) builds a 2-point identifying table; the file also shows how to compile, run, and assert per-element series, and how to inspect `compiled.modules.get(&compiled.root).context.graphical_functions` / opcodes. +- **No existing test exercises arrayed/A2A lookup-only *simulation* behavior** (greenfield). `per_element_gf_tests.rs` covers explicit `LOOKUP(g[Dim], x)` consumers — a different shape — and is the layout/no-regression reference. + +--- + + +### Task 1: Determine the standalone lookup-only saved-value semantics (spike) + +**Verifies:** none directly — produces the rule that AC1.1/1.2/1.3 encode. This task decides the `index_expr` used by Tasks 3-4. + +**Files:** +- Investigate (temporary instrumentation, reverted before commit): `src/simlin-engine/tests/simulate.rs` (`run_clearn_vs_vdf`, `:1669-1697`). +- Reference: `docs/reference/xmile-v1.0.html` (graphical-function / lookup input semantics). +- Output: a short written determination recorded as a doc comment in the new test module created in Task 3 (and summarized in the commit message). + +**Implementation:** +Decide empirically what genuine Vensim saves for a standalone lookup-only variable, choosing among: +- **Candidate A — `gf(Time)`** (leading hypothesis; `index_expr = Expr::App(BuiltinFn::Time, loc)`). Plausible because C-LEARN's `INITIAL TIME = 1850` and these tables are year-indexed historical data. +- **Candidate B — `gf(0)`** (current scalar behavior; `index_expr = Expr::Const(0.0, loc)`). +- **Candidate C — literal `0`** (current arrayed/A2A behavior). + +Method: temporarily instrument `run_clearn_vs_vdf` to print, for a few representative scalar lookup-only bases that are NOT suspected VDF-decode artifacts (`global_emissions_from_graph_lookup`, `historical_gdp_lookup`), the engine series under each candidate vs the `Ref.vdf` reference series at several time steps. Run via `cargo test -p simlin-engine --features file_io --release --test simulate -- --ignored run_clearn_vs_vdf` (or a temporary `#[ignore]` probe test calling the helper). Cross-check the conclusion against the XMILE spec's description of a graphical function whose input is unspecified. + +Avoid the bases the design flags as suspected decode artifacts (`rs_*`, `oc,_bc,_and_bio_aerosol_forcings`, `other_forcings_*`) for the determination — Phase 4 confirms those reference columns separately. Note: the probe base `historical_gdp_lookup` (a lookup-only variable, #590 cluster) is DISTINCT from `historical_gdp` (the #591-c1 `:NA:`-arithmetic boundary base handled in Phase 4); probe the `_lookup` one only. + +**Verification:** +The determination is documented with evidence (the candidate-vs-reference comparison) and a chosen `index_expr`. Revert all temporary instrumentation; `git status` is clean of probe code before moving on. If the determination contradicts `gf(Time)`, update Tasks 3-4 expectations and `index_expr` accordingly and note it — do not proceed on an unconfirmed rule. + +**Commit:** none (spike). The conclusion is committed as part of Task 3's doc comment. + + + + + +### Task 2: Pure lookup-only detection helper + unit tests + +**Verifies:** clearn-residual.AC1.4 (the helper is what keeps WITH LOOKUP on `gf(input)` while routing lookup-only to the determined rule). + +**Files:** +- Modify: `src/simlin-engine/src/compiler/mod.rs` (add a small private helper `fn is_lookup_only(...) -> bool`). +- Test: `src/simlin-engine/src/compiler/mod.rs` (or the nearest `#[cfg(test)]` module) — unit tests for the helper. + +**Implementation:** +Add a pure helper that, given the variable's equation form and whether it has tables, returns whether it is lookup-only. The signal: has a graphical function (`!tables.is_empty()`) AND the equation is empty-trimmed or equals `LOOKUP_SENTINEL` (mirror `mdl::writer::is_lookup_only_equation`'s "empty or sentinel" rule so XMILE-sourced and MDL-sourced lookup-only variables are both detected). It must return `false` for a WITH-LOOKUP variable (has tables but a real input equation) and for an ordinary aux (no tables). Keep it functional/pure (input: equation string or `Ast`/`eqn` view + `tables` emptiness; output: bool) so it is unit-testable without compiling a model. + +**Testing:** unit tests cover: +- lookup-only with sentinel equation + tables → `true`. +- lookup-only with empty equation + tables → `true` (XMILE-form). +- WITH LOOKUP (real input equation, e.g. `"some_input"`) + tables → `false` (AC1.4 distinction). +- ordinary aux (real equation, no tables) → `false`. +- sentinel equation but no tables (empty-RHS aux) → `false`. + +**Verification:** +Run: `cargo test -p simlin-engine --lib is_lookup_only` +Expected: all pass. + +**Commit:** `engine: add pure lookup-only detection helper` + + + +### Task 3: Generality fixtures for standalone + applied lookup-only (RED) + +**Verifies:** clearn-residual.AC1.1, clearn-residual.AC1.2, clearn-residual.AC1.3, clearn-residual.AC1.4, clearn-residual.AC1.5 + +**Files:** +- Test: a NEW `#[cfg(test)]` module `src/simlin-engine/src/lookup_only_tests.rs`, registered in `lib.rs` alongside `per_element_gf_tests` (do NOT extend `per_element_gf_tests.rs` — a dedicated module keeps the `cargo test --lib lookup_only` filter in Task 4 reliable). Every test function name in it MUST contain the substring `lookup_only` (e.g. `scalar_lookup_only_evaluates_at_time`, `arrayed_lookup_only_non_sorted_order`) so the verification filter cannot silently match nothing. Build `datamodel::Project` directly (mirror `arrayed_gf_project` / `ramp_gf` from `per_element_gf_tests.rs`). Record the Task 1 determination as a module doc comment. + +**Implementation:** +Build small, C-LEARN-independent inline-GF models and assert the determined standalone-lookup series. Use tables whose values are distinct and identifying so a wrong index (`0`) or a zeroed series is unambiguously detectable. Pick a sim window aligned to the table's x-domain (e.g. tables keyed on `x = [2000, 2001, 2002]`, sim `INITIAL TIME = 2000`, `FINAL TIME = 2002`, `dt = 1`), so `gf(Time)` yields the table's y-values step by step. + +Fixtures (each asserts the user-facing variable's series directly): +- **Scalar lookup-only** (AC1.1): one aux `g`, `equation = LOOKUP_SENTINEL`, `gf = Some(table x→y)`. Assert `g` series equals the determined rule (`gf(Time)` → the y-values `[y@2000, y@2001, y@2002]`), NOT a constant `gf(0)`. +- **Arrayed lookup-only** (AC1.2, AC1.5): `g[Dim]` with `Equation::Arrayed`, each element's equation `LOOKUP_SENTINEL` + its own per-element `gf`. Use a **non-alphabetical declared order** (e.g. `Dim = [Z, A, M]`) with each element's table carrying an element-identifying value, and assert each element's series equals its OWN table at `Time` (no positional mis-map). +- **A2A lookup-only** (AC1.3): `g[Dim]` with `Equation::ApplyToAll`, `equation = LOOKUP_SENTINEL`, one variable-level `gf`. Assert every element's series equals the single shared table at `Time`. +- **Applied-lookup no-regression** (AC1.4): in the same or a sibling model, a consumer `out = LOOKUP(g, idx)` (scalar) and/or `out[Dim] = LOOKUP(g[Dim], idx)` referencing a lookup-only `g`, with `idx` a real input distinct from `Time`. Assert `out` equals `gf(idx)` (the applied value), proving the standalone fix does not perturb the applied path. + +**Testing:** the assertions above. Before Task 4, these FAIL: the arrayed/A2A standalone cases produce `0` and the scalar standalone case produces a constant `gf(0)`. + +**Verification:** +Run: `cargo test -p simlin-engine --lib lookup_only` +Expected: **FAILS** (RED) for the standalone cases (arrayed/A2A = 0, scalar = constant gf(0)); the applied-lookup case may already pass. + +**Commit:** `engine: add failing generality fixtures for lookup-only saved values` + + + +### Task 4: Uniform lookup-only lowering (GREEN) + +**Verifies:** clearn-residual.AC1.1, clearn-residual.AC1.2, clearn-residual.AC1.3, clearn-residual.AC1.4, clearn-residual.AC1.5, clearn-residual.AC1.6 + +**Files:** +- Modify: `src/simlin-engine/src/compiler/mod.rs`: + - Scalar arm `:781-801` (the `Ast::Scalar` case under `Variable::Var`). + - `expand_arrayed_with_hoisting` non-array-producing else-branch `:1650-1683`. + - `expand_a2a_with_hoisting` fallback per-element loop `:1719-1736`. + +**Implementation:** +Using the Task 2 helper and the Task 1 `index_expr` (e.g. `Expr::App(BuiltinFn::Time, loc)`), lower lookup-only variables uniformly: +- **Scalar:** when the variable is lookup-only, feed `index_expr` to the existing `LOOKUP(Expr::Var(off, loc), index_expr)` wrap instead of the lowered (constant-0) equation. WITH LOOKUP (non-lookup-only) keeps `LOOKUP(self, input)` unchanged. +- **Arrayed (per-element tables, `tables.len() == n`):** for lookup-only element `i`, replace `AssignCurr(off + i, 0)` with `AssignCurr(off + i, Expr::App(BuiltinFn::Lookup(Box::new(Expr::Var(off + i, loc)), Box::new(index_expr.clone()), loc), loc))`. `extract_table_info` computes `elem_off = i`, the VM reads `graphical_functions[base_gf + i]` — element `i`'s own table. +- **A2A (single shared table, `tables.len() == 1`):** for lookup-only, every element must read the one shared table, so wrap the BASE offset, not `off + i`: `AssignCurr(off + i, Expr::App(BuiltinFn::Lookup(Box::new(Expr::Var(off, loc)), Box::new(index_expr.clone()), loc), loc))` → `elem_off = 0`, `table_count = 1`. (Using `off + i` here would make `element_offset >= table_count` for `i > 0` and the VM would push `NaN`.) + +Detection of lookup-only inside these branches uses the Task 2 helper (tables non-empty + equation empty/sentinel). Add a concise comment at each site explaining the A2A-vs-arrayed offset distinction (it is non-obvious and bounds-check-critical). Reuse `build_tables`/`reorder_arrayed_element_tables` and the `Lookup` opcode; add no new VM/codegen primitive. + +**Testing:** +- Task 3 fixtures pass (GREEN): scalar/arrayed/A2A standalone produce the determined series; non-alphabetical order maps each element to its own table; applied-lookup unchanged. +- AC1.6: the scalar `Lookup` opcode's out-of-range clamp/NaN behavior is unchanged — the fix only changes which expression is fed as the index. Verify by (a) the existing lookup tests still passing (`cargo test -p simlin-engine` lookup/per-element GF tests), and (b) if not already covered, a small assertion that a lookup whose index falls outside the table x-range clamps to the endpoint value (low/high) as before. + +**Verification:** +Run: `cargo test -p simlin-engine --lib lookup_only is_lookup_only` +Expected: all pass (GREEN). +Run: `cargo test -p simlin-engine --lib per_element_gf` and `cargo test -p simlin-engine` (default suite) +Expected: green (no regression to the per-element GF layout/applied-lookup tests). + +**Commit:** `engine: lower lookup-only variables uniformly to determined saved-value semantics` + + + + +--- + +## Phase completion criteria + +- The determined semantics is documented (Task 1) with evidence and a chosen `index_expr`. +- Tasks 2-4 committed; the generality fixtures pass (RED before Task 4, GREEN after) for scalar, arrayed, and A2A standalone lookup-only, plus the applied-lookup no-regression and non-alphabetical-order cases. +- `cargo test -p simlin-engine` (default, non-ignored) is green, including `per_element_gf_tests`. +- **Ignored C-LEARN gate note:** This phase makes the engine produce the determined-correct value uniformly. Whether each #590 base (`historical_gdp_lookup`, `historical_forestry_lookup`, the arrayed `rs_*`, the scalar `*_from_graph_lookup`/`*_forcings`) then reconciles against `Ref.vdf` — versus being a VDF-decode artifact whose reference column is mis-decoded — is determined in Phase 4. Do NOT prune `EXPECTED_VDF_RESIDUAL` here. Running the `--ignored` `clearn_residual_exactness` after this phase will report any reconciled bases as `shrank`; that is expected and is closed in Phase 4. Optionally record the observed `shrank` set in the commit body. + +## No special-casing (hard constraint) + +No change keys on a C-LEARN variable name, the C-LEARN `.mdl`/`.vdf` path, or the residual list. Detection is the general "has-gf + empty/sentinel-equation" rule; the index is the general determined rule; the fixtures are small models independent of C-LEARN. The only C-LEARN contact is the Task 1 read-only measurement against `Ref.vdf`, which produces a documented number, not a code branch. diff --git a/docs/implementation-plans/2026-05-19-clearn-residual/phase_02.md b/docs/implementation-plans/2026-05-19-clearn-residual/phase_02.md new file mode 100644 index 000000000..963f105b3 --- /dev/null +++ b/docs/implementation-plans/2026-05-19-clearn-residual/phase_02.md @@ -0,0 +1,186 @@ +# C-LEARN Residual — Phase 2: Stop import-time linearization of shadowed macros + +**Goal:** A user-defined macro that shares a builtin's name (here `RAMP FROM TO`) is resolved by the compile-time macro path instead of being silently replaced by an import-time linear-only rewrite, so the macro's exponential branch runs when the model selects it. + +**Architecture:** The MDL→datamodel expression formatter (`xmile_compat.rs`) currently has a `format_call_ctx` arm that rewrites any 4+-arg `RAMP FROM TO(...)` into a hardcoded linear `(xfrom) + RAMP(slope, tstart, tend)` string at import time, discarding the 5th arg (`islinear`) and the entire macro body, *before* compile-time macro resolution runs. We delete that arm so the call survives import as `RAMP_FROM_TO(...)` and resolves through the already-correct "macro-shadows-everything" precedence in `builtins_visitor.rs`. No new builtin, opcode, or codegen primitive is added. + +**Tech Stack:** Rust (`simlin-engine` crate). Tests use the in-crate inline-MDL macro harness (`macro_expansion_tests.rs`: `mdl()` + `run_mdl_var()`) and the existing file-fixture path (`tests/simulate.rs::simulate_mdl_path`). + +**Scope:** Phase 2 of 5 from `docs/design-plans/2026-05-19-clearn-residual.md`. + +**Codebase verified:** 2026-05-20 (branch `clearn-residual`, off `main`@`2ed93950`). + +--- + +## Acceptance Criteria Coverage + +This phase implements and tests: + +### clearn-residual.AC2: User macros sharing a builtin name resolve via the macro path +- **clearn-residual.AC2.1 Success:** A model defining `:MACRO: RAMP FROM TO(...)` invoked with the exponential selector (`islinear = 0`) produces the exponential trajectory from the macro body, not a linear ramp. +- **clearn-residual.AC2.2 Success (no regression):** The same invocation with the linear selector (`islinear = 1`) produces the linear trajectory (existing `simulates_macro_clearn_ramp_from_to_mdl` behavior preserved). +- **clearn-residual.AC2.3 Success:** After import, `RAMP FROM TO(...)` is a resolvable call (not a pre-linearized `RAMP(...)` string), so compile-time macro resolution applies. +- **clearn-residual.AC2.4 Edge:** Nonpositive endpoints exercise the macro's `linear` selector (forced linear when `xfrom>0 :AND: xto>0` is false) per the macro definition. +- **clearn-residual.AC2.5 Failure/guard:** No `xmile_compat.rs` formatter special-case rewrites a name a model defines as a macro ahead of macro resolution (verified for the audited builtin-named macros, e.g. `SSHAPE`). + +--- + +## Verified ground truth (read before starting) + +These were confirmed by investigation on 2026-05-20. Trust them over the design's line numbers. + +- **The load-bearing bug** is the `"ramp from to"` arm in `XmileFormatter::format_call_ctx` at `src/simlin-engine/src/mdl/xmile_compat.rs:422-434`. It fires for `args.len() >= 4` (so it catches the C-LEARN 5-arg call), returns the linear string, and never reads `args[4]` (`islinear`). Deleting this arm is the entire fix. +- **The name mapping `"ramp from to" => "RAMP_FROM_TO"` at `xmile_compat.rs:555`** is *redundant* with the catch-all `_ => canonical.to_uppercase().replace(' ', "_")` (same file, ~line 569): both produce `RAMP_FROM_TO`. Keep line 555 (it documents intent and is the active formatting path once the arm is gone); changing it alone changes nothing. Do **not** remove it as part of the fix — its presence is harmless and self-documenting. +- **`RAMP FROM TO` is NOT a native builtin** (no `BuiltinFn::RampFromTo`; confirmed absent from `builtins.rs`/`builtins_visitor.rs`). After the arm is removed, the *only* resolution is the user macro. **No test model uses `RAMP FROM TO` without also defining the macro** (verified by grep over `test/`; every occurrence — the `macro_clearn_ramp_from_to` fixture, C-LEARN itself at line 67, and the inline `macro_expansion_tests.rs` test — co-defines the macro). So removing the arm orphans nothing. +- **The macro-shadows-everything precedence already handles `RAMP FROM TO`**: `builtins_visitor.rs:626-638` resolves an in-model macro named like a builtin before alias/modulo/builtin handling. Proven by the passing `macro_expansion_tests.rs` test `ac5_4_macro_shadows_ramp_from_to_builtin` (the 2-arg case, which today escapes the formatter arm because `args.len() < 4`). +- **The #554 self-call exception is irrelevant here**: `is_enclosing_macro_renamed_builtin_self_call` (`builtins_visitor.rs:254-261`) only fires for `init`/`previous`/stdlib-module names; `is_renamed_builtin_macro_collision("ramp_from_to")` is `false`. +- **SSHAPE is NOT a formatter-rewrite hazard.** There is *no* `sshape` arm in `format_call_ctx`. `sshape` appears only as a name-preserving rename in `format_function_name` (`xmile_compat.rs:556`), and `SSHAPE` is a genuine 3-arg builtin (`BuiltinFn::Sshape`). A same-named macro shadows it cleanly via the normal path (proven by the existing `macro_expansion_tests.rs` test `ac5_4_macro_shadows_sshape_builtin`). The design's "audit SSHAPE" instruction is based on an incorrect assumption; the audit (Task 4) records this. +- **The existing `simulates_macro_clearn_ramp_from_to_mdl` (`tests/simulate.rs:3181`) does NOT discriminate the bug from the fix.** Its fixture uses `is linear = 1` with positive endpoints, so the linear rewrite and the correct macro resolution produce the identical series `[2,2,4,6,8,10,10]`. It passes today *with the macro bypassed* and will still pass after the fix. It is a no-regression guard only — **not** proof the macro path runs. Task 1 adds the discriminating test. +- **The unit test `test_ramp_from_to_transforms_args` (`xmile_compat.rs:2031-2054`) encodes the buggy linear output**: it asserts the formatter output `contains("RAMP")` and `!starts_with("RAMP_FROM_TO")`. Task 2 inverts it. +- **Inline MDL test harness** (in-crate, fast, NOT `file_io`-gated): `src/simlin-engine/src/macro_expansion_tests.rs` provides `fn mdl(body: &str) -> String` (wraps body with `{UTF-8}` header + a `CONTROL_TAIL` control section) and `fn run_mdl_var(source: &str, var: &str) -> Vec` (compiles via salsa, runs the VM, returns the variable's saved series). Read `CONTROL_TAIL` (top of that file) to learn the `INITIAL TIME`/`FINAL TIME`/`TIME STEP`/`SAVEPER` of the harness so you can align the ramp window and the asserted step. The 2-arg shadow test at `macro_expansion_tests.rs:463` is the closest existing template. + +--- + + + + +### Task 1: Add a discriminating exponential-branch regression test (RED) + +**Verifies:** clearn-residual.AC2.1, clearn-residual.AC2.2, clearn-residual.AC2.4 + +**Files:** +- Test: `src/simlin-engine/src/macro_expansion_tests.rs` (add a new `#[test]` near the existing `ac5_4_macro_shadows_ramp_from_to_builtin` at line 463) — in-crate unit test, no `file_io`. + +**Implementation:** +Write a test that defines a 5-parameter `RAMP FROM TO` macro whose body genuinely branches on `islinear`, with an exponential (here: clearly non-linear) branch that the import-time linear rewrite cannot reproduce. Use `mdl()` + `run_mdl_var()`. First read `CONTROL_TAIL` and pick `tstart`/`tend`/the asserted step so the asserted step lies strictly inside the ramp window. + +Fixture body (the macro mirrors C-LEARN's `linear` selector so AC2.4 is exercised; the exp branch uses double slope so it is provably distinct from the linear branch mid-ramp): + +``` +:MACRO: RAMP FROM TO(xfrom, xto, tstart, tend, islinear) +RAMP FROM TO = IF THEN ELSE(linear = 1, linear ramp, exp ramp) + ~ dmnl + ~ | +linear = IF THEN ELSE(xfrom > 0 :AND: xto > 0, islinear, 1) + ~ dmnl + ~ | +slope = (xto - xfrom) / (tend - tstart) + ~ dmnl + ~ | +linear ramp = xfrom + RAMP(slope, tstart, tend) + ~ dmnl + ~ | +exp ramp = xfrom + 2 * RAMP(slope, tstart, tend) + ~ dmnl + ~ | +:END OF MACRO: +``` + +Calling models (three separate `run_mdl_var` invocations or three vars in one model): +- `y_exp = RAMP FROM TO(10, 110, , , 0)` — `islinear = 0`, positive endpoints. Expected: the **exp** branch (`xfrom + 2*slope*(t-tstart)`), NOT the linear branch. +- `y_lin = RAMP FROM TO(10, 110, , , 1)` — `islinear = 1`, positive endpoints. Expected: the **linear** branch. +- `y_force = RAMP FROM TO(-10, 110, , , 0)` — `islinear = 0` but `xfrom <= 0`, so `linear` is forced to `1`. Expected: the **linear** branch despite `islinear = 0`. + +**Testing:** assertions (compute exact values from the chosen window; example with `tstart=1, tend=11, slope=10`, asserted at a saved step where `Time=6` → `RAMP(10,1,11)=50`): +- AC2.1: `y_exp` at the mid step `== 10 + 2*50 == 110` (exp), and is NOT equal to the linear value `10 + 50 == 60` (a margin assertion `>= 50` apart, or exact `== 110`). +- AC2.2: `y_lin` at the mid step `== 60` (linear). +- AC2.4: `y_force` at the mid step `== (-10) + 50 == 40` (linear branch, because `linear` forced to 1), and is NOT the exp value `-10 + 100 == 90`. + +Note: with the current (buggy) formatter, all three reduce to the linear rewrite, so `y_exp` would be `60` and `y_force` would be `40` — the `y_exp` assertion FAILS, giving a true RED. + +**Verification:** +Run: `cargo test -p simlin-engine --lib ` +Expected: **FAILS** (RED) — `y_exp` is the linear value `60`, not the exp value `110`, because the formatter linearized the call before macro resolution. + +**Commit:** `engine: add failing exponential-branch test for shadowed RAMP FROM TO macro` + + + +### Task 2: Invert the formatter unit test to expect macro survival (RED) + +**Verifies:** clearn-residual.AC2.3 + +**Files:** +- Test: `src/simlin-engine/src/mdl/xmile_compat.rs:2031-2054` (rewrite `test_ramp_from_to_transforms_args`). + +**Implementation:** +The existing test asserts the formatter linearizes (`result.contains("RAMP")` and `!result.starts_with("RAMP_FROM_TO")`). Invert it so it asserts the call SURVIVES as a resolvable macro call: format the same 4-arg `ramp from to` call and assert the result IS the `RAMP_FROM_TO(...)` form (so compile-time macro resolution can apply) and is NOT a pre-linearized `RAMP(...)` rewrite. Rename the test to reflect the new intent (e.g. `test_ramp_from_to_survives_as_macro_call`). Keep the arg-count and arg-content checks meaningful (the four args appear in order in the emitted call). + +**Testing:** +- AC2.3: assert the formatted output starts with `RAMP_FROM_TO(` (or canonicalizes to `ramp_from_to`), contains the four argument expressions, and does NOT contain a top-level `RAMP(` linear rewrite. + +**Verification:** +Run: `cargo test -p simlin-engine --lib test_ramp_from_to` +Expected: **FAILS** (RED) — the current formatter still emits the linear `RAMP(...)`, so the inverted assertion fails. + +**Commit:** `engine: rewrite RAMP FROM TO formatter test to expect macro survival` + + + +### Task 3: Remove the import-time linearization arm (GREEN) + +**Verifies:** clearn-residual.AC2.1, clearn-residual.AC2.2, clearn-residual.AC2.3, clearn-residual.AC2.4 + +**Files:** +- Modify: `src/simlin-engine/src/mdl/xmile_compat.rs:422-434` (delete the entire `"ramp from to" => { ... }` arm from `format_call_ctx`). + +**Implementation:** +Delete the `"ramp from to"` arm at `xmile_compat.rs:422-434`. After deletion, a `ramp from to` call falls through to the default call-formatting path, which calls `format_function_name("ramp from to")` → `"RAMP_FROM_TO"` (line 555, or equivalently the catch-all), producing `RAMP_FROM_TO(arg0, arg1, arg2, arg3, arg4)`. At compile time, `builtins_visitor.rs` canonicalizes this to `ramp_from_to`, `resolve_macro` finds the in-model macro, and `expand_module_function` expands it (the macro-shadows-everything path at `builtins_visitor.rs:626-638`). Do NOT touch line 555. Do NOT add any new builtin/opcode. + +**Testing:** +- Tasks 1 and 2 tests now pass (GREEN). +- No regression: `simulates_macro_clearn_ramp_from_to_mdl` (`tests/simulate.rs:3181`) still passes (its `islinear=1` fixture yields the same linear series via the macro path). + +**Verification:** +Run: `cargo test -p simlin-engine --lib test_ramp_from_to ` +Expected: both pass (GREEN). +Run: `cargo test -p simlin-engine --features file_io --test simulate simulates_macro_clearn_ramp_from_to_mdl` +Expected: passes (no regression). +Run: `cargo test -p simlin-engine --lib macro` +Expected: all macro-expansion tests pass (e.g. `ac5_4_macro_shadows_ramp_from_to_builtin`, `ac5_4_macro_shadows_sshape_builtin`). + +**Commit:** `engine: stop linearizing shadowed RAMP FROM TO macro at import` + + + +### Task 4: Record the formatter special-case audit and verify the shadow guard + +**Verifies:** clearn-residual.AC2.5 + +**Files:** +- Modify: `src/simlin-engine/src/mdl/xmile_compat.rs` (add a doc comment above `format_call_ctx`'s `match` documenting the audit and the shadowing hazard). +- Test (verify, add only if a gap exists): `src/simlin-engine/src/macro_expansion_tests.rs`. + +**Implementation:** +Add a concise doc comment above the `match canonical.as_str()` in `format_call_ctx` recording the audit conclusion: arms that *restructure* a builtin-named call into a different expression at import time pre-empt any same-named user `:MACRO:`. The `ramp from to` arm was the one C-LEARN tripped (now removed). Document that other restructuring arms (notably the multi-word `sample if true`, plus `pulse`/`pulse train`/`modulo`/`zidz`/`xidz`/`get data between times`/etc.) carry the *same* latent hazard but are not currently shadowed by any in-repo model; if a future model defines a macro with one of those names, the arm must be guarded the same way. Explicitly note that **`SSHAPE` is name-preserving** (handled only by `format_function_name`, not restructured here), so it shadows correctly via `builtins_visitor.rs:626-638` with no formatter change needed. + +Then confirm the guard already exists: `ac5_4_macro_shadows_sshape_builtin` and `ac5_4_macro_shadows_ramp_from_to_builtin` in `macro_expansion_tests.rs` prove a same-named macro shadows the builtin. If (and only if) one is missing, add a minimal equivalent. Do not duplicate existing coverage. + +This task changes no runtime behavior; it is documentation + a coverage check (the design's AC2.5 is a guard, not new functionality). + +**Testing:** +- AC2.5: the existing `ac5_4_macro_shadows_*` tests pass, demonstrating macro-shadows precedence for both a multi-word name (`RAMP FROM TO`) and a single-word builtin (`SSHAPE`). The audit doc is present. + +**Verification:** +Run: `cargo test -p simlin-engine --lib ac5_4_macro_shadows` +Expected: both shadow tests pass. +Run: `cargo build -p simlin-engine` (doc comment compiles). + +**Commit:** `engine: document formatter macro-shadowing audit (RAMP FROM TO, SSHAPE)` + + + + +--- + +## Phase completion criteria + +- Tasks 1-4 committed; the discriminating exponential-branch test passes (was RED before Task 3, GREEN after). +- `cargo test -p simlin-engine` (default, non-ignored) is green, including the inverted formatter test and all macro-expansion tests. +- `simulates_macro_clearn_ramp_from_to_mdl` still passes (no regression). +- The formatter audit is recorded as a doc comment. +- **Ignored C-LEARN gate note:** This phase is expected to make the `im_3_emissions`, `im_3_emissions_vs_rs`, `im_3_ff_co2`, `relative_emissions_to_equity`, and `relative_emissions_to_equity_target` bases reconcile against `Ref.vdf`. Do NOT prune `EXPECTED_VDF_RESIDUAL` here — Phase 4 re-measures after Phases 1-3 and reconciles the gate. Running `cargo test -p simlin-engine --features file_io --release --test simulate -- --ignored clearn_residual_exactness` after this phase will report those bases as `shrank`; that is expected and is closed in Phase 4. Optionally record the observed `shrank` set in the commit body as evidence the fix landed. + +## No special-casing (hard constraint) + +No change in this phase keys on a C-LEARN variable name, the C-LEARN `.mdl`/`.vdf` path, or the residual list. The fix is the deletion of one general formatter arm; the generality test uses a small inline macro independent of C-LEARN. diff --git a/docs/implementation-plans/2026-05-19-clearn-residual/phase_03.md b/docs/implementation-plans/2026-05-19-clearn-residual/phase_03.md new file mode 100644 index 000000000..21c2e14a4 --- /dev/null +++ b/docs/implementation-plans/2026-05-19-clearn-residual/phase_03.md @@ -0,0 +1,220 @@ +# C-LEARN Residual — Phase 3: Correct user-macro INITIAL recurrences (#591-c1) + +**Goal:** An element-wise `INITIAL` recurrence expressed through a trivial passthrough macro (`:MACRO: INIT(x) = INITIAL(x)`) produces correct values at every saved step, matching the proven bare-`INITIAL` opcode path — no drop to `0` at t≥1 and no spurious `:NA:`. + +**Architecture:** The importer renames Vensim `INITIAL`/`active initial`/`reinitial` to `INIT` (`xmile_compat.rs:545-547`). C-LEARN also defines `:MACRO: INIT(x) = INITIAL(x)` (which, after the rename, reads `INIT = INIT(x)`). Under macro-shadows-everything precedence (`builtins_visitor.rs:626-638`), every `INITIAL` call therefore collides with the macro and expands to a per-element synthetic module whose value is mis-ordered/mis-propagated. The fix collapses a *genuine passthrough macro* (single parameter, single primary-output body that is exactly `out = BUILTIN(param)` where `BUILTIN` is a renamed-builtin self-call) directly to the proven opcode (`LoadInitial`) at the call site, bypassing `expand_module_function`. This generalizes the existing #554 self-call exception from inside the macro body to the call site. + +**Key constraint discovered in investigation:** the macro body AST is **not** reachable at the call site — `MacroRegistry`/`ModuleFunctionDescriptor` deliberately store only name/ports/output, and the body is parsed transiently in `MacroRegistry::build` and discarded. So the collapse requires computing a body-derived "passthrough" classification in `MacroRegistry::build` (where each body is already parsed) and threading it onto `ModuleFunctionDescriptor`; the call site then reads that flag. This is a registry/descriptor change plus a call-site branch, not a pure call-site change. + +**Tech Stack:** Rust (`simlin-engine` crate). Tests use the inline-MDL harness (`tests/simulate.rs::run_inline_mdl` + `element_series`) and a small file fixture mirroring the proven `helper_recurrence.mdl`. + +**Scope:** Phase 3 of 5 from `docs/design-plans/2026-05-19-clearn-residual.md`. + +**Codebase verified:** 2026-05-20 (branch `clearn-residual`, off `main`@`2ed93950`). + +--- + +## Acceptance Criteria Coverage + +This phase implements and tests: + +### clearn-residual.AC3: User-macro `INITIAL` recurrences produce correct multi-step values +- **clearn-residual.AC3.1 Success:** A model with `:MACRO: INIT(x) = INITIAL(x)` and a scalar `INITIAL`-captured value holds that value constant across all saved steps (matching the opcode path). +- **clearn-residual.AC3.2 Success:** An element-wise `INITIAL` recurrence routed through the passthrough macro produces correct per-element values at t0 and at every subsequent step (no drop to `0` at t≥1, no spurious `:NA:`). +- **clearn-residual.AC3.3 Success (no regression):** A bare `INITIAL(expr)` with no user macro still compiles to `LoadInitial` and behaves as today (`helper_recurrence.mdl` stays green). +- **clearn-residual.AC3.4 Edge:** The passthrough collapse fires only for a genuine `out = BUILTIN(param)` body; a non-passthrough macro that merely shares a builtin name still expands as a macro (not mis-collapsed to the opcode). +- **clearn-residual.AC3.5 Success (downstream):** A `SAMPLE UNTIL` fed by a corrected `INITIAL` value samples until the correct time without any dedicated `SAMPLE UNTIL` change. + +--- + +## Verified ground truth (read before starting) + +Confirmed by investigation on 2026-05-20. + +- **INITIAL→INIT rename:** `src/simlin-engine/src/mdl/xmile_compat.rs:545-547` (`"active initial"`/`"initial"`/`"reinitial"` → `"INIT"`). +- **Macro expansion → synthetic module:** `expand_module_function` (`src/simlin-engine/src/builtins_visitor.rs:480-579`). Receives only a `ModuleFunctionDescriptor` (model_name, parameter_ports, primary_output, additional_outputs, is_macro) — **NOT the body AST**. +- **#554 self-call exception (the pattern to generalize):** predicate `is_enclosing_macro_renamed_builtin_self_call` at `builtins_visitor.rs:254-261`; its use gates the macro-resolution branch at `builtins_visitor.rs:623-638`: + ```rust + let is_renamed_builtin_self_call = self.is_enclosing_macro_renamed_builtin_self_call(&func); + ... + if !is_renamed_builtin_self_call + && let Some(descriptor) = self.macro_registry.resolve_macro(&func) { + let descriptor = descriptor.clone(); + return self.expand_module_function(&descriptor, &func, args, loc); + } + // falls through to alias/MODULO/PREVIOUS/INIT intrinsic routing + ``` + When the branch is skipped, control reaches the `init`/`previous` intrinsic routing (~`builtins_visitor.rs:655-697`), which emits `LoadInitial`/`LoadPrev` (with `make_temp_arg` hoisting at `:413-440` for expression args). +- **`MacroRegistry`** (`src/simlin-engine/src/module_functions.rs:209-214`): `macros: HashMap`; `resolve_macro` at `:283-286`. **`ModuleFunctionDescriptor`** at `module_functions.rs:45-60`. **`MacroRegistry::build`** (`:230-280`) calls `check_for_recursion` (`:296-363`) which parses each body via `Expr0::new(formula, LexerType::Equation)` and walks it (`collect_called_macros`, `:419-470`) — so the body AST IS available at build time and the classification can be computed there. +- **The collision predicate** `is_renamed_builtin_macro_collision` (`module_functions.rs:180-182`) is `true` for `init`/`previous` and stdlib-module names — exactly the renamed-builtins. `is_renamed_builtin_macro_collision("init")` is `true`. +- **`LoadInitial`** (`src/simlin-engine/src/vm.rs:1352-1363`): in `StepPart::Initials` reads `curr[abs_off]` (so the referenced var must be ordered earlier in the Initials runlist); in flows reads the t=0 snapshot `initial_values[abs_off]` (snapshot taken at `vm.rs:1151-1155`). +- **Runlist/deps**: `model.rs` (`init_referenced_vars:122-129`, `module_output_deps:203-248`, `all_deps:301-486`) is the **legacy** path; the **production** path is salsa: `VariableDeps.init_referenced_vars` at `db.rs:854-857`/`944`, module deps at `db.rs:881-896`. Any contingent secondary fix must target the salsa path (and likely `model.rs` for parity). +- **Proven reference test:** `helper_recurrence_mdl_synthetic_helper_in_scc_simulates` at `tests/simulate.rs:2377` (fixture `test/sdeverywhere/models/helper_recurrence/helper_recurrence.mdl`), exercising `ecc[tNext] = INITIAL(ecc[tPrev]*2)` over a subrange — the **bare-INITIAL, no-macro** path. Expected `ecc[t1]=1, ecc[t2]=2, ecc[t3]=4` constant across saved steps. This is what the macro-routed version must equal. +- **Test-helper caveat:** `ensure_results` skips implicit module vars (`tests/test_helpers.rs:81-87`; `is_implicit_module_var` matches the `$\u{205A}` prefix at `:29-31`). Fixtures must assert the USER-FACING variable (e.g. `ecc[t1]`), not the implicit `$⁚...` helper. +- **Inline harness:** `tests/simulate.rs::run_inline_mdl` (`:3032-3040`) → `Results`; `element_series(&r, "ecc[t1]")` (`:1831-1842`); `macro_test_value_at` (`:3014-3028`). Inline `:MACRO:` example: `simulates_macro_independent_invocation_state` (`:3305-3339`). `simulate_mdl_path(path)` (`:763`) for file fixtures with a sibling reference. +- **Feasibility (confirmed):** the call-site collapse is sound and the safer design option. The strict structural match (single param; single primary-output body equation that is exactly `App(BUILTIN, [Var(the_sole_param)])`; the call name canonicalizes to a renamed-builtin collision name) cannot misfire on a non-passthrough macro that merely shares a name. NO NA-arithmetic change is made (`float.rs::NA` is left untouched). + +--- + + +### Task 1: Add failing INIT-macro recurrence + scalar fixtures (RED) + +**Verifies:** clearn-residual.AC3.1, clearn-residual.AC3.2 + +**Files:** +- Test (scalar, inline): `src/simlin-engine/tests/simulate.rs` (new `#[test]` using `run_inline_mdl` near the other inline `:MACRO:` tests, e.g. by `simulates_macro_independent_invocation_state`). +- Fixture (element-wise): copy `test/sdeverywhere/models/helper_recurrence/helper_recurrence.mdl` to a new fixture directory (e.g. `test/test-models/tests/macro_init_recurrence/`) and PREPEND a `:MACRO: INIT(x) = INITIAL(x)` block; copy the sibling reference output (the `helper_recurrence` expected `.dat`/`output.tab`) into the new directory. The recurrence text is unchanged — its existing `INITIAL(...)` is renamed to `INIT(...)` at import and now collides with the new macro. +- Test (element-wise): `src/simlin-engine/tests/simulate.rs` — a `#[test]` calling `simulate_mdl_path("../../test/test-models/tests/macro_init_recurrence/.mdl")` (file_io). First confirm how `simulate_mdl_path` locates the reference for the existing fixtures and follow that convention. + +**Implementation:** +- **Scalar (AC3.1):** inline MDL defining `:MACRO: INIT(x) = INITIAL(x)` and a scalar capture, e.g. `captured = INIT(growing)` with `growing = Time`. `INITIAL(Time)` captures `INITIAL TIME` at t0, so `captured` must be constant `== INITIAL TIME` across all saved steps. Assert the series is flat at `INITIAL TIME` via `macro_test_value_at`/`element_series`. +- **Element-wise (AC3.2):** the `macro_init_recurrence` fixture is `helper_recurrence.mdl` + the `:MACRO: INIT` block. Expected output is identical to `helper_recurrence` (`ecc[t1]=1, ecc[t2]=2, ecc[t3]=4` constant across saved steps). Assert via the same reference comparison the existing fixture uses (which skips implicit `$⁚` vars per `ensure_results`). + +**Testing:** +- AC3.1: scalar `captured` series is constant at `INITIAL TIME` for every saved step. +- AC3.2: per-element `ecc[t1]/[t2]/[t3]` equal `1/2/4` at every saved step (no drop to `0` at t≥1, no `:NA:`). + +**Verification:** +Run: `cargo test -p simlin-engine --features file_io --test simulate ` +Expected: **FAILS** (RED) — the INIT-macro collision routes through the buggy synthetic module: the recurrence drops to `0`/`:NA:` at t≥1 and/or the scalar capture is not held constant. + +**Commit:** `engine: add failing INIT passthrough-macro recurrence fixtures` + + + + + +### Task 2: Pure passthrough-classification function + unit tests + +**Verifies:** clearn-residual.AC3.4 + +**Files:** +- Modify: `src/simlin-engine/src/module_functions.rs` (add a pure classifier, e.g. `fn classify_passthrough(macro_name, parameter_ports, primary_output_body_ast) -> Option` and a `PassthroughBuiltin` type, or a `bool` if a single target suffices — `init` is the only live target, but keep the structural test general over `is_renamed_builtin_macro_collision`). +- Test: `src/simlin-engine/src/module_functions.rs` `#[cfg(test)]` (unit tests). + +**Implementation:** +A pure function that returns `Some(passthrough target)` iff ALL of: +- the macro has exactly one parameter (`parameter_ports.len() == 1`); +- the primary-output body equation AST is exactly `App(UntypedBuiltinFn(call, [arg]), _)` (a single call with a single argument); +- `arg` is exactly `Var(the sole parameter)` (the bare parameter, not an expression like `param*2`); +- `canonicalize(call) == canonicalize(macro_name)` (a self-call — the form the importer's rename produces, e.g. `INIT = INIT(x)`); and +- `is_renamed_builtin_macro_collision(canonicalize(call))` is `true` (so the fall-through to the existing intrinsic routing is valid and the target is a real opcode-backed builtin). +Otherwise `None`. Keep it purely structural over the parsed AST (functional core), with no registry/IO access, so it is unit-testable in isolation. + +**Testing:** unit tests: +- `INIT = INIT(x)` (single param `x`) → `Some(init)` (positive). +- `INIT = INIT(x) + 1` (Op2 body) → `None` (AC3.4 negative). +- `INIT = INIT(x * 2)` (arg not the bare param) → `None` (AC3.4 negative). +- two-parameter macro whose body is `F(a)` → `None` (arity). +- a macro with the body `ABS(x)` where `abs` is NOT a renamed-builtin collision → `None` (not opcode-backed via this path). +- a multi-output macro (additional outputs present) → `None`. + +**Verification:** +Run: `cargo test -p simlin-engine --lib classify_passthrough` +Expected: all pass. + +**Commit:** `engine: add pure passthrough-macro classifier` + + + +### Task 3: Thread the passthrough classification onto the descriptor + +**Verifies:** clearn-residual.AC3.4 (registry-level wiring of the classifier) + +**Files:** +- Modify: `src/simlin-engine/src/module_functions.rs` — add a field to `ModuleFunctionDescriptor` (`:45-60`), e.g. `passthrough: Option`; populate it in `MacroRegistry::build` (`:230-280`) by parsing the primary-output body equation (reuse the `Expr0::new` parse already performed by `check_for_recursion`, or parse the primary-output equation once) and calling the Task 2 classifier. Non-macro descriptors and non-passthrough macros get `None`. +- Test: `src/simlin-engine/src/module_functions.rs` `#[cfg(test)]` — build a `MacroRegistry` from a small project containing `:MACRO: INIT(x) = INITIAL(x)` and assert its descriptor's `passthrough == Some(init)`; build one with `:MACRO: INIT(x) = INITIAL(x) + 1` and assert `passthrough == None`. + +**Implementation:** +Compute the classification once at registry-build time (the only place the body AST is available), store it on the descriptor, and keep `salsa::Update`/`Clone`/`Eq` derivations intact (`ModuleFunctionDescriptor` and `MacroRegistry` derive these). Do not change `expand_module_function`'s signature. Ensure the new field participates in equality so salsa invalidation is correct. + +**Testing:** +- The INIT passthrough macro is classified `Some`; the `INITIAL(x)+1` near-miss is `None` (registry-level AC3.4). + +**Verification:** +Run: `cargo test -p simlin-engine --lib macro_registry passthrough` +Expected: pass. +Run: `cargo build -p simlin-engine` (salsa derives compile). + +**Commit:** `engine: classify passthrough macros at registry build time` + + + +### Task 4: Call-site passthrough collapse (GREEN) + +**Verifies:** clearn-residual.AC3.1, clearn-residual.AC3.2, clearn-residual.AC3.3, clearn-residual.AC3.5 + +**Files:** +- Modify: `src/simlin-engine/src/builtins_visitor.rs:623-638` (the macro-resolution branch in `walk`). + +**Implementation:** +At the call site, when `resolve_macro(&func)` returns a descriptor whose `passthrough.is_some()` and the arity matches, SKIP `expand_module_function` and fall through to the existing intrinsic routing (which, because the passthrough is a self-call, sees `func` canonicalizing to the renamed builtin — e.g. `init` — and emits `LoadInitial` with the existing `make_temp_arg` hoisting for the expression argument). Concretely, restructure so the descriptor is obtained first and the expansion is guarded: +```rust +let is_renamed_builtin_self_call = self.is_enclosing_macro_renamed_builtin_self_call(&func); +let descriptor = if is_renamed_builtin_self_call { None } else { self.macro_registry.resolve_macro(&func) }; +if let Some(descriptor) = descriptor { + if descriptor.passthrough.is_none() { + let descriptor = descriptor.clone(); + return self.expand_module_function(&descriptor, &func, args, loc); + } + // genuine passthrough macro: fall through to the renamed-builtin intrinsic + // routing (func canonicalizes to the opcode-backed builtin), exactly as the + // #554 self-call exception does inside a macro body. +} +``` +Add a comment explaining the self-call invariant that makes the fall-through valid (the classifier guarantees `canonicalize(call) == canonicalize(macro_name)`, so `func` routes to the right intrinsic). Make NO NA-arithmetic change. + +**Testing:** +- Task 1 fixtures pass (GREEN): scalar capture constant; element-wise recurrence = `1/2/4` across all steps. +- AC3.3: `helper_recurrence_mdl_synthetic_helper_in_scc_simulates` (the bare-INITIAL, no-macro path) still passes unchanged. +- All macro-expansion tests pass (the #554 in-body collapse and the Phase 2 RAMP FROM TO behavior are unaffected — RAMP FROM TO is not a passthrough, so `passthrough == None` and it still expands as a module). +- AC3.5: observe (informational) that, after this collapse, the `--ignored` C-LEARN `SAMPLE UNTIL`-fed bases (`last_set_target_year`, `last_active_target_year`, `time_from_target_to_ultimate_target`, `target_emissions_for_rate`, `ultimate_target_value_from_rate`, `depth_at_bottom`, `emissions_with_stopped_growth`) move toward reconciliation — final reconciliation/attribution is Phase 4. No dedicated `SAMPLE UNTIL` change is made. + +**Verification:** +Run: `cargo test -p simlin-engine --features file_io --test simulate helper_recurrence` +Expected: Task 1 tests GREEN; `helper_recurrence` still green. +Run: `cargo test -p simlin-engine --lib macro` and `cargo test -p simlin-engine` (default suite) +Expected: green. +Observe: `cargo test -p simlin-engine --features file_io --release --test simulate -- --ignored clearn_residual_exactness` reports the #591-c1 bases as `shrank` (records which reconciled — feeds Task 5's decision and Phase 4). + +**Commit:** `engine: collapse trivial passthrough macros to the builtin opcode at the call site` + + + + + +### Task 5: (Contingent) Fix residual synthetic-module ordering/propagation — only if needed + +**Verifies:** clearn-residual.AC3.2 (only if a non-passthrough INITIAL-macro module remains divergent) + +**Entry criteria (do this task ONLY if true):** After Task 4, the `--ignored` `clearn_residual_exactness` still shows one or more of the #591-c1 bases (`last_set_target_year` … `emissions_with_stopped_growth`) failing, AND root-causing shows the cause is a *non-passthrough* macro-module output (an INITIAL-derived module not collapsible by Task 4, or the Phase 2 `RAMP FROM TO` module) whose value is mis-ordered in the Initials runlist or mis-propagated in the flows phase. If, instead, Task 4 reconciled these bases, SKIP this task and record "not needed — passthrough collapse sufficed" with the observed evidence. + +**Files (if performed):** +- `src/simlin-engine/src/db.rs` (salsa, production): `VariableDeps.init_referenced_vars` (`:854-857`/`:944`), module-output deps (`:881-896`). +- `src/simlin-engine/src/vm.rs`: Initials vs flows `LoadInitial` (`:1352-1363`). +- For parity: `src/simlin-engine/src/model.rs` (`module_output_deps:203-248`, `all_deps:301-486`). + +**Implementation (if performed):** Use the systematic-debugging skill — reproduce on a minimal non-passthrough INITIAL-macro-module fixture (build it; do not rely on C-LEARN), confirm the root cause is Initials topological ordering or flows-phase module-output propagation, fix it generally in the salsa path (with `model.rs` parity), and add the minimal fixture as a regression test asserting the user-facing series. Do not key on any C-LEARN name. + +**Verification (if performed):** +Run: the new minimal fixture test + `cargo test -p simlin-engine`. +Expected: green; the previously-divergent base reconciles in the `--ignored` gate. + +**If skipped:** record the skip rationale (with the Task 4 `--ignored` evidence) in the Phase 3 completion notes / commit body. No code change. + +**Commit (if performed):** `engine: fix synthetic-module INITIAL ordering/propagation` + + +--- + +## Phase completion criteria + +- Tasks 1-4 committed; the scalar and element-wise INIT-macro fixtures pass (RED before Task 4, GREEN after); `helper_recurrence` stays green (AC3.3); the passthrough classifier rejects near-misses (AC3.4). +- Task 5 either performed (with its minimal regression fixture green) or explicitly skipped with recorded evidence that the Task 4 collapse sufficed. +- `cargo test -p simlin-engine` (default, non-ignored) is green. +- NO NA-arithmetic change was made (`float.rs::NA` untouched). +- **Ignored C-LEARN gate note:** Do NOT prune `EXPECTED_VDF_RESIDUAL` here; Phase 4 re-measures and reconciles. The `--ignored` `clearn_residual_exactness` will report the reconciled #591-c1 bases as `shrank` — expected, closed in Phase 4. AC3.5 (`SAMPLE UNTIL` downstream) is verified there: those bases reconcile downstream of the INIT fix with no dedicated `SAMPLE UNTIL` change. + +## No special-casing (hard constraint) + +No change keys on a C-LEARN variable name, the C-LEARN `.mdl`/`.vdf` path, or the residual list. The passthrough classifier is a general structural rule; the fixtures are small models (a `helper_recurrence` variant and an inline scalar capture) independent of C-LEARN. diff --git a/docs/implementation-plans/2026-05-19-clearn-residual/phase_04.md b/docs/implementation-plans/2026-05-19-clearn-residual/phase_04.md new file mode 100644 index 000000000..9009435ee --- /dev/null +++ b/docs/implementation-plans/2026-05-19-clearn-residual/phase_04.md @@ -0,0 +1,155 @@ +# C-LEARN Residual — Phase 4: Residual attribution and gate tightening + +**Goal:** Every remaining divergent C-LEARN base (after Phases 1-3) is honestly classified and either fixed or documented with a sourced reason; `EXPECTED_VDF_RESIDUAL` shrinks to exactly the genuine remainder, and both `--ignored` C-LEARN gates pass. + +**Architecture:** This phase is measurement-driven. It re-runs the C-LEARN-vs-`Ref.vdf` comparison after the Phase 1-3 engine fixes, classifies each still-divergent base into a precise category, fixes the tractable reference-side (VDF-reader decode) bugs and any remaining engine-genuine divergences, and then tightens the carve-out (`EXPECTED_VDF_RESIDUAL`) to exactly the live remainder — enforced by the `clearn_residual_exactness` guard. The comparator's tolerances and floors are NOT loosened; the carve-out remains a documented, exact exclusion. + +**Tech Stack:** Rust (`simlin-engine` crate): the VDF reader (`vdf.rs`, `vdf/record_results.rs`), the comparator + gate (`tests/simulate.rs`), and `gh` for issue updates. + +**Scope:** Phase 4 of 5 from `docs/design-plans/2026-05-19-clearn-residual.md`. **Depends on Phases 1-3** (must re-measure after the engine fixes). + +**Codebase verified:** 2026-05-20 (branch `clearn-residual`, off `main`@`2ed93950`). Both `--ignored` gates pass today with exactly 32 residual bases (verified by a live run). + +--- + +## Acceptance Criteria Coverage + +This phase implements and tests: + +### clearn-residual.AC4: The residual is honestly attributed and the gate is tightened +- **clearn-residual.AC4.1 Success:** After Phases 1-3, `EXPECTED_VDF_RESIDUAL` is reduced to the proven remainder and `simulates_clearn` passes (`--ignored`). +- **clearn-residual.AC4.2 Success:** `clearn_residual_exactness` passes — the live failing set equals the reduced `EXPECTED_VDF_RESIDUAL` exactly (no `grew` / no `shrank`). +- **clearn-residual.AC4.3 Success:** Every still-excluded base carries a one-line sourced reason (engine-genuine-tracked / VDF-decode-artifact / benign-near-zero / NaN-vs-`:NA:` / boundary), and #590 / #591 are updated to reflect the final disposition. +- **clearn-residual.AC4.4 Failure/guard:** The comparator's 1% tolerance, per-series floor, and matched-variable floor are unchanged for every non-excluded variable (the carve-out remains a documented exclusion, never a tolerance loosening; the gate cannot pass vacuously). +- **clearn-residual.AC4.5 Edge:** A NaN-vs-`:NA:` series (e.g. `slr_inches_from_2000`) is handled by the documented NaN-skip mechanism and does not enter the failure set. + +--- + +## Verified ground truth (read before starting) + +Confirmed by investigation on 2026-05-20 (line numbers accurate; both gates ran green live). + +- **`EXPECTED_VDF_RESIDUAL`**: `src/simlin-engine/tests/simulate.rs:1563-1622`, exactly **32 base strings** today, grouped by GitHub-issue cluster comments (#590: 17 bases; #591-c1: 8 bases incl. `historical_gdp`; #591-c2: 7 bases incl. `diffusion_flux`, `co2eq_gap_closing_percentage`; #591-c3: 0 bases). The five design category labels (engine-genuine-tracked / VDF-decode-artifact / benign-near-zero / NaN-vs-`:NA:` / boundary) do NOT exist in code yet — **this phase creates that taxonomy.** +- **`simulates_clearn`**: `tests/simulate.rs:1652-1661` (`#[test] #[ignore]`); calls `run_clearn_vs_vdf` + `ensure_vdf_results_excluding(.., EXPECTED_VDF_RESIDUAL)`. Run: `cargo test -p simlin-engine --features file_io --release --test simulate -- --ignored simulates_clearn`. +- **`clearn_residual_exactness`**: `tests/simulate.rs:1720-1766` (`#[test] #[ignore]`). Computes the live failing set (no exclusion), collapses to base names via `vdf_ident_base_name`, and asserts `grew.is_empty() && shrank.is_empty()` vs `EXPECTED_VDF_RESIDUAL`. This is the lockstep guard: when a Phase 1-3 fix reconciles a base, it appears in `shrank` and this test fails until the base is pruned. +- **Comparator (do NOT loosen — AC4.4)**: `classify_vdf_ident` (`:236-304`), `ensure_vdf_results_excluding` (`:349-454`). Constants: `VDF_RTOL = 0.01` (`:164`, the 1% tol), `K_ATOL = 1e-4` (`:176`, per-series abs floor `atol = K_ATOL*peak`), `MIN_MATCHED_FRACTION = 0.10` (`:144`), `MIN_MATCHED_ABSOLUTE = 10` (`:150`), `MAX_NAN_SKIPPED_FRACTION = 0.10` (`:160`). The matched floor is checked AFTER exclusion (so the gate can't pass vacuously). These five constants and the floor logic must remain unchanged. +- **NaN-skip (AC4.5)**: `classify_vdf_ident:264-268` skips any cell where either side is NaN (counts `nan_skipped`, never `failures`). This is why `slr_inches_from_2000` (Vensim literal NaN vs Simlin `:NA:` sentinel) is NOT in the list (it appears only in comments at `:1616-1621`). `float.rs::NA = -6.490371073168535e32 = -2^109` (`float.rs:24`), finite, distinct from NaN. +- **VDF-reader decode-bug candidate sites** (`src/simlin-engine/src/vdf/record_results.rs`, `vdf.rs`): (a) smallest-`start` name-collision pick (`record_results.rs:532-537`); (b) post-canonicalization HashMap overwrite (`record_results.rs:627-632` + `vdf.rs:1973-1974`); (c) owner-vs-descriptor `f[10]`-highest fallback for `Ref.vdf`'s abbreviated `rs_*` descriptors (`record_results.rs:346-354`). The `is_lookupish_name` doc (`vdf.rs:95-97`) explicitly names `Ref.vdf` as the one corpus fixture where the lexical descriptor test fails — corroborating the `rs_*` cluster as a likely reference-side artifact. There are existing non-ignored VDF unit tests (`tests/vdf_alias_decoder.rs`, `vdf/signatures.rs` tests) to extend. +- **`run_clearn_vs_vdf`**: `tests/simulate.rs:1669-1697` (loads `../../test/xmutil_test_models/C-LEARN v77 for Vensim.mdl` + `Ref.vdf`; uses `open_vensim` null provider). `vdf_ident_base_name` strips a trailing `[elem,…]` subscript. +- **Issues**: #590 (data/graph-lookup `0+0` zeroing) and #591 (residual divergence) are OPEN. Code references at `tests/simulate.rs` lines 185, 341-342, 408, 1554, 1564/1585/1600/1616/1645/1658/1714/1762. + +--- + + +### Task 1: Re-measure after Phases 1-3 and classify every remaining divergent base + +**Verifies:** none directly — produces the classification that AC4.3 records and that drives Tasks 2-4. + +**Files:** +- Investigate: `tests/simulate.rs` (`clearn_residual_exactness` already prints the live `grew`/`shrank`; optionally add temporary instrumentation to `run_clearn_vs_vdf`/`classify_vdf_ident` to dump, per still-failing base, the failing cells, magnitudes, NaN-skip counts, and `:NA:`-reconciled counts). Revert instrumentation before committing. +- Output: a classification table recorded in the Phase 4 commit body and as the restructured `EXPECTED_VDF_RESIDUAL` comments (Task 4). + +**Implementation:** +Run `clearn_residual_exactness` (`--ignored`, release). It will FAIL with a `shrank` list (the bases Phases 1-3 reconciled) — that is expected. Capture the live failing set. For EACH still-failing base, assign exactly one category with sourced evidence: +- **engine-genuine** — the engine's value is wrong on a meaningful magnitude (a real bug to fix in Task 3). +- **VDF-decode-artifact** — the engine value is correct but the `Ref.vdf` reference column is mis-decoded (trace to one of the three candidate sites; e.g. the abbreviated `rs_*` descriptors). Fix in Task 2 if tractable. +- **benign-near-zero** — divergence only on near-zero magnitudes (cross-simulator noise), e.g. `diffusion_flux`, `co2eq_gap_closing_percentage`. +- **NaN-vs-`:NA:`** — representation mismatch, NaN-skipped by the comparator (e.g. `slr_inches_from_2000`); should NOT be in the failure set at all. +- **boundary** — matches except at a `:NA:`-arithmetic boundary cell, e.g. `historical_gdp`. + +Use the systematic-debugging skill; root-cause-confirm each classification (this codebase has a history of wrong guesses). Avoid assuming the design's pre-classification — verify each against the live data. + +**Verification:** +A complete classification table: every live-failing base has exactly one category and a one-line sourced reason (no "unknown"). Temporary instrumentation reverted (`git status` clean of probe code). + +**Commit:** none (investigation; the table lands in Task 4's comments and commit body). + + + +### Task 2: Fix tractable VDF-reader decode bugs (contingent) + +**Verifies:** clearn-residual.AC4.1, clearn-residual.AC4.2 (by removing reference-side artifacts from the failure set) + +**Entry criteria:** Task 1 classified one or more bases as VDF-decode-artifact AND the root cause is tractable at one of the three candidate sites. If no decode artifact is tractable, document why (precise attribution) and SKIP to Task 4 (those bases stay excluded with a `VDF-decode-artifact` reason). + +**Files (if performed):** +- `src/simlin-engine/src/vdf/record_results.rs` (candidate sites at `:346-354`, `:518-538`, `:622-632`) and/or `src/simlin-engine/src/vdf.rs` (`:1967-2005` build_results; `is_lookupish_name` `:85-101`). +- Test: `src/simlin-engine/tests/vdf_alias_decoder.rs` or `src/simlin-engine/src/vdf/signatures.rs` `#[cfg(test)]` — a non-ignored unit test reproducing the mis-decode on a small synthetic record set (NOT keyed on C-LEARN names). + +**Implementation (if performed):** Fix the identified decode bug generally (e.g. correct the owner-vs-descriptor disambiguation so `Ref.vdf`'s abbreviated `rs_*` descriptors are not misclassified, or the name-collision/canonicalization-overwrite that drops a column). Follow the format spec `docs/design/vdf.md`. Add a focused regression test at the reader level that pins the corrected decode on a minimal synthetic input, independent of C-LEARN. + +**Verification (if performed):** +Run: `cargo test -p simlin-engine vdf` (the non-ignored VDF reader tests, including the new one). +Expected: green; the artifact base(s) now decode correctly (will be removed from the live failure set, observed in Task 4). + +**If skipped:** record the precise attribution for each decode-artifact base (which candidate site, why intractable now) for Task 4's comment + a tracked follow-up issue. + +**Commit (if performed):** `engine: fix VDF reader decode of ` + + + +### Task 3: Fix any remaining engine-genuine divergences (contingent) + +**Verifies:** clearn-residual.AC4.1, clearn-residual.AC4.2 + +**Entry criteria:** Task 1 classified one or more bases as engine-genuine (the engine value is wrong on a meaningful magnitude) AND the cause is in scope (not NA-arithmetic, which is confirmed correct and out of scope). If none, SKIP to Task 4. + +**Files (if performed):** determined by root-cause (engine compiler/VM/importer). Add a general regression fixture independent of C-LEARN. + +**Implementation (if performed):** Use the systematic-debugging skill. Reproduce on a minimal general fixture, fix the root cause generally, add the fixture as a regression test. Do NOT touch NA-arithmetic (`float.rs::NA` and `NA+NA == -2^110` are correct and explicitly out of scope). Do NOT key on any C-LEARN name. + +**Verification (if performed):** +Run: the new fixture test + `cargo test -p simlin-engine`. +Expected: green; the base reconciles in the `--ignored` gate (observed in Task 4). + +**If skipped:** record why no engine-genuine divergence remains (or why an identified one is out of scope, e.g. tracked as separate tech debt). + +**Commit (if performed):** `engine: ` + + + +### Task 4: Tighten the gate to the proven remainder and attribute every base + +**Verifies:** clearn-residual.AC4.1, clearn-residual.AC4.2, clearn-residual.AC4.3, clearn-residual.AC4.4, clearn-residual.AC4.5 + +**Files:** +- Modify: `src/simlin-engine/tests/simulate.rs:1563-1622` (`EXPECTED_VDF_RESIDUAL`) — prune to exactly the live remainder; restructure the comments into the five-category taxonomy with a one-line sourced reason per base. +- Modify (only if the guard logic/messages need it): `tests/simulate.rs:1720-1766` (`clearn_residual_exactness`) — keep the grew/shrank assertion; update only the doc/messages, not the comparator. +- Add: a non-ignored guard test asserting AC4.4 invariants (see below) if one does not already exist. +- Update: GitHub issues #590 / #591 via `gh`. + +**Implementation:** +1. Run `clearn_residual_exactness` (`--ignored`) to get the exact live failing set after Tasks 1-3. Set `EXPECTED_VDF_RESIDUAL` to that set exactly (prune the reconciled bases). +2. Reorganize the list comments under the five category headings, each base carrying a one-line sourced reason (AC4.3). Bases reconciled by Phases 1-3 / Tasks 2-3 are removed entirely (not re-labeled). +3. Confirm `slr_inches_from_2000`-style NaN-vs-`:NA:` series are NOT in the list (NaN-skipped) — AC4.5. Add a small non-ignored unit test of `classify_vdf_ident` proving a synthetic NaN-vs-`:NA:` series produces `nan_skipped` cells and zero `failures` (so it never enters the failure set), independent of C-LEARN. +4. AC4.4 guard: do not change `VDF_RTOL`, `K_ATOL`, `MIN_MATCHED_FRACTION`, `MIN_MATCHED_ABSOLUTE`, `MAX_NAN_SKIPPED_FRACTION`, or the floor logic. Add (or confirm) a non-ignored unit test that pins these constants and that a matched-variable count below the floor still panics (the gate cannot pass vacuously). +5. Update #590 / #591 with the final disposition of each base (which were fixed in which phase; which remain and why, by category). Use `gh issue comment`/`gh issue close` as appropriate; if `gh` is unavailable in the execution environment, write the intended issue update text into the commit body and flag it for the human to post. + +**Testing:** +- AC4.1: `simulates_clearn` (`--ignored`) passes with the reduced list. +- AC4.2: `clearn_residual_exactness` (`--ignored`) passes (no `grew`, no `shrank`). +- AC4.3: every remaining base has a one-line sourced category reason; issues updated. +- AC4.4: the comparator-constants/floor guard test passes; a diff shows no tolerance/floor change. +- AC4.5: the NaN-vs-`:NA:` `classify_vdf_ident` unit test passes. + +**Verification:** +Run: `cargo test -p simlin-engine --features file_io --release --test simulate -- --ignored simulates_clearn clearn_residual_exactness` +Expected: both pass. +Run: `cargo test -p simlin-engine` (default suite, including the new AC4.4/AC4.5 guard tests) +Expected: green. + +**Commit:** `engine: tighten C-LEARN VDF residual carve-out to the proven remainder` + + +--- + +## Phase completion criteria + +- `simulates_clearn` and `clearn_residual_exactness` both pass (`--ignored`, release) with the reduced `EXPECTED_VDF_RESIDUAL`. +- Every still-excluded base carries a one-line sourced reason under one of the five categories (no "unknown"); #590 / #591 updated/closed with the final disposition. +- The comparator's tolerance/floors are demonstrably unchanged (AC4.4 guard test green); the NaN-vs-`:NA:` skip is tested (AC4.5). +- `cargo test -p simlin-engine` (default, non-ignored) is green, including the new VDF reader and guard tests. + +## No special-casing (hard constraint) + +The carve-out is the ONLY place C-LEARN base names appear (it is the documented measurement exclusion, which this phase shrinks — never a tolerance loosening). All engine/reader fixes (Tasks 2-3) ship with general regression tests on synthetic inputs, independent of C-LEARN names/paths. No fix branches on a C-LEARN variable name or the residual list. diff --git a/docs/implementation-plans/2026-05-19-clearn-residual/phase_05.md b/docs/implementation-plans/2026-05-19-clearn-residual/phase_05.md new file mode 100644 index 000000000..3b46c7d45 --- /dev/null +++ b/docs/implementation-plans/2026-05-19-clearn-residual/phase_05.md @@ -0,0 +1,133 @@ +# C-LEARN Residual — Phase 5: Native external-data provider wiring (forward-looking; cuttable) + +**Goal:** Native (filesystem-capable) targets resolve Vensim `GET DIRECT *` external-data functions, so large Vensim models that depend on external CSV data import and simulate with real values via the CLI. + +**Architecture:** The entire data-provider stack already exists and is wired end-to-end through `open_vensim_with_data` (including a libsimlin FFI and ~7 passing engine tests). The only gap is `simlin-cli::open_model`, which calls the plain `open_vensim(&contents)` and drops the model's parent directory, so a `GET DIRECT` model fails to open from the CLI. The fix builds a `FilesystemDataProvider` rooted at the model file's parent directory and passes it via `open_vensim_with_data`. This is a ~5-line change at one site plus a fixture and tests. + +**Scope:** Phase 5 of 5 from `docs/design-plans/2026-05-19-clearn-residual.md`. **Forward-looking and cleanly cuttable: C-LEARN needs NO external data (all its lookup data is inline), so this phase does not affect residual closure (Phases 1-4) and may be dropped without consequence.** It serves other large Vensim models. + +**Tech Stack:** Rust (`simlin-cli`, `simlin-engine`). `simlin-cli` already enables the `file_io` feature. + +**Codebase verified:** 2026-05-20 (branch `clearn-residual`, off `main`@`2ed93950`). + +--- + +## Acceptance Criteria Coverage + +This phase implements and tests: + +### clearn-residual.AC5: Native targets resolve external-data functions (forward-looking) +- **clearn-residual.AC5.1 Success:** On a native (`file_io`) build, a model using `GET DIRECT DATA` (or `GET DIRECT LOOKUPS`/`CONSTANTS`) imported via the CLI resolves values from a companion file located relative to the model path. +- **clearn-residual.AC5.2 Success:** The resolved external data drives simulation (the fixture asserts a downstream value that reflects the external data, not a zeroed series). +- **clearn-residual.AC5.3 Edge:** A missing or unreadable data file produces a clear diagnostic rather than a silent `0+0` zeroing. +- **clearn-residual.AC5.4 Constraint:** WASM / libsimlin builds remain on the null provider (behavior unchanged); a tracked follow-up issue captures their data-supply API. + +--- + +## Verified ground truth (read before starting) + +Confirmed by investigation on 2026-05-20. + +- **`DataProvider` trait**: `src/simlin-engine/src/data_provider/mod.rs:16-75` (NOT feature-gated; `NullDataProvider` too). **`FilesystemDataProvider`**: `src/simlin-engine/src/data_provider/csv_provider.rs:16-547` (`#[cfg(feature = "file_io")]`); `FilesystemDataProvider::new(base_dir: impl Into)` takes the root directory and sandboxes paths (rejects absolute/`..`, `canonicalize`s — a missing file returns an error, not a zero). +- **ONLY `GET DIRECT *` is wired**: `is_get_direct_ref` (`mdl/convert/external_data.rs:448-451`) and `parse_get_direct` (`:63-136`) recognize only `GET DIRECT DATA|CONSTANTS|LOOKUPS|SUBSCRIPT`. `GET XLS`, `GET DATA`, `GET 123`, `GET VDF` are normalized but NOT resolved (they pass through and fail compilation). **The fixture must use `GET DIRECT *` only.** +- **CSV always (`file_io`); Excel needs `ext_data` (calamine), which `simlin-cli` does NOT enable** (`simlin-cli/Cargo.toml:16` enables only `file_io`). **The fixture must be CSV-only** (a `.xls*` file would error with a "requires the 'ext_data' feature" message). +- **`open_vensim_with_data`**: `src/simlin-engine/src/compat.rs:63-74`. `open_vensim(contents)` is literally `open_vensim_with_data(contents, None)`. `open_vensim_with_data` is NOT feature-gated (only `FilesystemDataProvider` is). +- **The change site**: `src/simlin-cli/src/main.rs::open_model` (`:176-216`), `InputFormat::Vensim` arm calls `open_vensim(&contents)` at `:187`. `file_path` is computed at `:178-182`; `simlin-cli/Cargo.toml:16` already enables `file_io`. **Guard the stdin case**: `file_path` falls back to `/dev/stdin` (parent `/dev`), so only build a `FilesystemDataProvider` when the input is a real file; otherwise pass `None`. +- **No-provider behavior today is a HARD ERROR** (not a silent zero): `try_resolve_data_expr` (`external_data.rs:471-490`) returns `Err("external data file '...' referenced but no DataProvider configured")` → `open_vensim` returns `Err` → the CLI `die!`s. So the CLI currently CANNOT open a `GET DIRECT` model. (The design's AC5.3 "silent 0+0 zeroing" framing is inaccurate for `GET DIRECT *`; the genuine requirement is that a missing/unreadable *file* yields a CLEAR file-level diagnostic, which `FilesystemDataProvider` provides.) +- **Ready template**: `tests/simulate.rs::simulate_mdl_path_with_data` (`:786-812`, `#[cfg(feature = "file_io")]`) does exactly `FilesystemDataProvider::new(mdl_abs.parent()) → open_vensim_with_data(&contents, Some(&provider))` — the CLI change mirrors this. Inline-tempfile examples: `simulates_get_direct_data_scalar_csv` (`:2785`), `simulates_get_direct_constants_scalar_csv` (`:2848`), `simulates_get_direct_lookups_scalar_csv` (`:2888`). +- **Existing CSV fixtures** (passing, reusable): `test/sdeverywhere/models/directconst/` (model + `data/*.csv`, relative-subdir resolution), `test/sdeverywhere/models/directdata/` (`*.csv` + `.dat` reference), `directlookups/`, `directsubs/`. (Avoid `directdata`'s `.xlsx` path; use the CSV references.) +- **libsimlin already has the data FFI**: `simlin_project_open_vensim_with_data` (`src/libsimlin/src/project.rs:577`) builds a `FilesystemDataProvider` from a `data_dir` under `file_io`. So the AC5.4 follow-up is about WASM *callers* supplying data (the data-supply surface), not new FFI. Other native callers using the plain `open_vensim` (out of scope here): `pysimlin`, `simlin-serve` (`parse.rs:59`), `simlin-mcp-core` (`open.rs:129`). + +--- + + + + +### Task 1: Add a CLI external-data fixture test (RED) + +**Verifies:** clearn-residual.AC5.1, clearn-residual.AC5.2 + +**Harness decision (resolve the fork before writing the test):** Use the **extracted-testable-function** approach as the primary, because it does not depend on the CLI's stdout format and gives a precise assertion. In **Task 1** you extract `open_model`'s `InputFormat::Vensim` arm into a small function `open_vensim_model(path: &Path, contents: &str) -> Result` that keeps the CURRENT null-provider behavior (`open_vensim(contents)`); in **Task 2** you change only that function's body to build the provider from the path's parent and call `open_vensim_with_data`. The Task-1 test calls `open_vensim_model` on the fixture, compiles + runs the returned project (mirror `tests/simulate.rs`'s compile/run helpers), and asserts the data-driven variable's series. Because the extraction lands in Task 1 with current behavior, the test **compiles and fails on behavior** (`open_vensim_model` returns `Err("... no DataProvider configured")`) until Task 2 wires the provider — a behavior-RED, not a compile error. (A `std::process::Command` CLI integration test is an acceptable alternative ONLY if extraction is undesirable; do not do both.) + +**Fixture decision:** Use the existing `test/sdeverywhere/models/directconst/directconst.mdl` (+ its `data/*.csv`) as the primary fixture. Its `GET DIRECT CONSTANTS` references use the relative subdir `data/a.csv` (verified), so a `FilesystemDataProvider` rooted at the model directory resolves them. First confirm at kickoff that opening it needs no sdeverywhere spec file (the `*_spec.json` files are for the sdeverywhere toolchain, not the MDL importer). **Fallback (only if directconst needs a spec or an absolute path):** add a tiny purpose-built fixture under `test/test-models/` — a minimal `.mdl` with one `GET DIRECT CONSTANTS('vals.csv', ',', 'A', 'B')` reference + a `vals.csv`, plus a downstream variable that multiplies the constant — fully controlled and unambiguous. + +**Files:** +- Modify: `src/simlin-cli/src/main.rs` — extract the `InputFormat::Vensim` arm's body into a small function `fn open_vensim_model(path: &Path, contents: &str) -> Result` that, IN THIS TASK, keeps the CURRENT behavior (`open_vensim(contents)`, null provider); have `open_model` call it. This makes the RED a behavior failure, not a compile error, and gives Task 2 a single body to change. +- Fixture: `test/sdeverywhere/models/directconst/` (reuse; primary) or a tiny new fixture under `test/test-models/` (fallback per the decision above). +- Test: `src/simlin-cli/tests/external_data.rs` (new integration test) calling the extracted `open_vensim_model`. + +**Implementation:** +Extract `open_vensim_model` (current null-provider behavior) and write the RED test that calls it on the fixture, compiles + runs the returned project (mirror `tests/simulate.rs`'s compile/run helpers), and asserts the data-driven variable equals the CSV-derived value (tie the expected number to the actual CSV contents), NOT zero/NaN. + +**Testing:** +- AC5.1/AC5.2: the data-driven variable's value matches the external CSV (not zeroed). + +**Verification:** +Run: `cargo test -p simlin-cli external_data` +Expected: **FAILS** (RED) — `open_vensim_model` still uses the null provider, so it returns `Err("... no DataProvider configured")` (the test compiles and fails on behavior). + +**Commit:** `cli: extract open_vensim_model and add failing GET DIRECT test` + + + +### Task 2: Wire FilesystemDataProvider into the CLI (GREEN) + +**Verifies:** clearn-residual.AC5.1, clearn-residual.AC5.2, clearn-residual.AC5.3 + +**Files:** +- Modify: `src/simlin-cli/src/main.rs` — imports (`:25`, add `open_vensim_with_data` and `FilesystemDataProvider`) and the body of the `open_vensim_model` function extracted in Task 1. + +**Implementation:** +Change `open_vensim_model`'s body: when the input is a real file (not stdin), build `FilesystemDataProvider::new()` and call `open_vensim_with_data(contents, Some(&provider))`; otherwise (stdin) keep `open_vensim(contents)` (equivalently `open_vensim_with_data(contents, None)`). Mirror `simulate_mdl_path_with_data` (`tests/simulate.rs:786-812`). Guard the stdin/`/dev/stdin` case so its parent (`/dev`) is never used as a data root. + +**Testing:** +- Task 1's test passes (GREEN): the CLI resolves the external CSV and simulates the data-driven value. +- AC5.3: add a test where the model references a NONEXISTENT CSV; assert the CLI emits a clear file-level diagnostic (the `FilesystemDataProvider` canonicalize/`not found` error surfaced through the CLI's error path), NOT a silent zero or a generic message. + +**Verification:** +Run: `cargo test -p simlin-cli` +Expected: Task 1 test GREEN; the missing-file test asserts a clear diagnostic. +Run: `cargo build -p simlin-cli` and a manual `simlin-cli` invocation on the fixture (optional smoke). + +**Commit:** `cli: resolve GET DIRECT external data via filesystem provider` + + + +### Task 3: Confirm WASM/libsimlin unchanged and file the follow-up + +**Verifies:** clearn-residual.AC5.4 + +**Files:** +- Verify only (no code change): WASM/libsimlin default callers still use the null provider (the plain `open_vensim`/`simlin_project_open_vensim` path). Confirm no behavior change there. +- File a tracked follow-up issue. + +**Implementation:** +Confirm that this phase touched only `simlin-cli` (and a fixture/test): the WASM and libsimlin default paths are unchanged (libsimlin already exposes `simlin_project_open_vensim_with_data` for the data case; the no-data FFI and WASM remain on the null provider). Then file a tracked follow-up issue capturing the data-supply API for WASM callers (the FFI surface for supplying external data to browser/WASM consumers — the right design is its own question, explicitly out of scope here). Use the `track-issue` agent (the repo's convention) so duplicates are checked against GitHub issues and `docs/tech-debt.md`. + +**Testing:** +- AC5.4: a grep/confirmation that no WASM/libsimlin default-path behavior changed; the follow-up issue exists. + +**Verification:** +Run: `cargo test -p simlin-engine` and `cargo build` for the WASM target per repo conventions (or confirm the libsimlin/WASM open paths are untouched by this phase's diff). +Expected: unchanged behavior; follow-up issue filed. + +**Commit:** `cli: note WASM/libsimlin data-supply follow-up` (or no commit if only an issue is filed — record the issue link in the phase notes). + + + + +--- + +## Phase completion criteria + +- The CLI fixture imports and simulates with data resolved from the filesystem on a native build (AC5.1/AC5.2); a missing data file yields a clear diagnostic (AC5.3). +- WASM/libsimlin default paths remain on the null provider (AC5.4); the follow-up issue for the WASM/libsimlin data-supply API is filed. +- `cargo test -p simlin-cli` and `cargo test -p simlin-engine` are green. + +## Cuttability + +This phase is independent of Phases 1-4 and of C-LEARN residual closure. If time/scope requires, it can be dropped entirely without affecting the C-LEARN gate or any AC1-AC4 deliverable. + +## No special-casing (hard constraint) + +The fixture is a small `GET DIRECT *` CSV model independent of C-LEARN; the wiring is a general CLI change (any model with `GET DIRECT *` benefits). No code branches on a C-LEARN name/path. diff --git a/docs/implementation-plans/2026-05-19-clearn-residual/test-requirements.md b/docs/implementation-plans/2026-05-19-clearn-residual/test-requirements.md new file mode 100644 index 000000000..6faaa13a6 --- /dev/null +++ b/docs/implementation-plans/2026-05-19-clearn-residual/test-requirements.md @@ -0,0 +1,187 @@ +# C-LEARN Residual — Test Requirements + +Maps every acceptance criterion in +[`docs/design-plans/2026-05-19-clearn-residual.md`](/docs/design-plans/2026-05-19-clearn-residual.md) +to its verification: an automated test (with phase/task, type, file path, and run +command) or human verification (with justification + approach). Used by the +test-analyst during execution to validate coverage. + +## Conventions + +- **Default suite** = `cargo test -p simlin-engine` (the non-ignored, capped suite + the pre-commit hook runs). New per-phase generality fixtures live here and must + stay tiny (root `CLAUDE.md` 3-minute cap). +- **`file_io`-gated** = the test is compiled only under `--features file_io` + (file-fixture path, CSV providers, the C-LEARN gate). +- **`#[ignore]` runtime class** = `#[test] #[ignore]`, NOT part of the default + suite; run explicitly via `-- --ignored`, release build (C-LEARN is ~53k lines, + ~5s just to parse). The two C-LEARN gates are the only such tests here. +- **C-LEARN gate run command** (AC4.1/AC4.2; informational observation in + Phases 1-3): + `cargo test -p simlin-engine --features file_io --release --test simulate -- --ignored simulates_clearn clearn_residual_exactness` +- All engine-fix generality fixtures are C-LEARN-independent by design constraint + (no fix may key on a C-LEARN name/path/residual-list); the only C-LEARN contact + is read-only measurement in the two gates. + +--- + +## clearn-residual.AC1 — Lookup-only variables produce Vensim-matching saved series (Phase 1) + +| AC | Criterion (abridged) | Verification | Phase/Task | Type | File / run | +|----|----------------------|--------------|------------|------|------------| +| **AC1.1** | Scalar lookup-only var produces the Vensim-determined series, not constant `gf(0)` | **Automated** (rule correctness). The Vensim *determination* itself is a documented spike (P1 T1) — a human/process artifact (written conclusion + chosen `index_expr`); the resulting rule's correctness is automated by the `scalar_lookup_only_*` fixture. | P1 T1 (spike, determines rule) → P1 T3 (RED) / T4 (GREEN) | unit (in-crate) | `src/simlin-engine/src/lookup_only_tests.rs` (fn names contain `lookup_only`); `cargo test -p simlin-engine --lib lookup_only` | +| **AC1.2** | Arrayed (`Equation::Arrayed`) lookup-only produces correct per-element series, not literal `0` | **Automated** | P1 T3/T4 | unit | `src/simlin-engine/src/lookup_only_tests.rs`; `cargo test -p simlin-engine --lib lookup_only` | +| **AC1.3** | Apply-to-all lookup-only produces correct series for every element | **Automated** | P1 T3/T4 | unit | `src/simlin-engine/src/lookup_only_tests.rs`; same command | +| **AC1.4** | Applied lookup `var(idx)`→`LOOKUP(var,idx)` still correct (no regression) | **Automated** — applied-lookup fixture in T3 + the pure detection helper (T2) keeping WITH-LOOKUP on `gf(input)` | P1 T2 (helper unit tests) + T3/T4 (applied fixture) | unit | `src/simlin-engine/src/lookup_only_tests.rs` (applied case) + `src/simlin-engine/src/compiler/mod.rs` `#[cfg(test)]` (`is_lookup_only`); `cargo test -p simlin-engine --lib lookup_only is_lookup_only` | +| **AC1.5** | Non-alphabetical declared element order maps each element to its own table | **Automated** — arrayed fixture uses a non-alphabetical order (e.g. `Dim=[Z,A,M]`) with element-identifying tables | P1 T3/T4 | unit | `src/simlin-engine/src/lookup_only_tests.rs` (`arrayed_lookup_only_non_sorted_order`); same command | +| **AC1.6** | Out-of-range scalar `Lookup` clamp/NaN behavior unchanged by the fix | **Automated** — (a) existing per-element-GF / lookup tests stay green; (b) a clamp assertion (added in T4 if not already covered) | P1 T4 | unit | `src/simlin-engine/src/per_element_gf_tests.rs` (no regression) + clamp assertion; `cargo test -p simlin-engine --lib per_element_gf` and default suite | + +Notes: `TestProject` cannot attach graphical functions, so fixtures build +`datamodel::Project` directly (mirror `per_element_gf_tests.rs::arrayed_gf_project`/`ramp_gf`). +None of these are `file_io`-gated or ignored. Pruning `EXPECTED_VDF_RESIDUAL` for +#590 bases happens in Phase 4, not here (the `--ignored` exactness guard will report +`shrank` after this phase — expected). + +--- + +## clearn-residual.AC2 — User macros sharing a builtin name resolve via the macro path (Phase 2) + +| AC | Criterion (abridged) | Verification | Phase/Task | Type | File / run | +|----|----------------------|--------------|------------|------|------------| +| **AC2.1** | `:MACRO: RAMP FROM TO(...)` with `islinear=0` runs the exponential branch, not a linear ramp | **Automated** | P2 T1 (RED) → T3 (GREEN) | unit (in-crate inline-MDL harness, no `file_io`) | `src/simlin-engine/src/macro_expansion_tests.rs` (new `#[test]` near `ac5_4_macro_shadows_ramp_from_to_builtin`); `cargo test -p simlin-engine --lib ` | +| **AC2.2** | Same call with `islinear=1` runs the linear branch (no regression) | **Automated** — new test's `y_lin` assertion *and* the preserved file fixture | P2 T1 + T3 | unit + integration (`file_io`) | `macro_expansion_tests.rs` (`y_lin`) + `src/simlin-engine/tests/simulate.rs::simulates_macro_clearn_ramp_from_to_mdl`; `cargo test -p simlin-engine --features file_io --test simulate simulates_macro_clearn_ramp_from_to_mdl` | +| **AC2.3** | After import `RAMP FROM TO(...)` survives as a resolvable call (not a pre-linearized `RAMP(...)` string) | **Automated** — inverted formatter unit test | P2 T2 (RED, inverts) → T3 (GREEN) | unit | `src/simlin-engine/src/mdl/xmile_compat.rs` (`test_ramp_from_to_survives_as_macro_call`, formerly `test_ramp_from_to_transforms_args`); `cargo test -p simlin-engine --lib test_ramp_from_to` | +| **AC2.4** | Nonpositive endpoints force the macro's `linear` selector | **Automated** — `y_force` assertion in the T1 test | P2 T1 → T3 | unit | `src/simlin-engine/src/macro_expansion_tests.rs` (`y_force`); same command as AC2.1 | +| **AC2.5** | No `xmile_compat.rs` formatter special-case rewrites a model-defined macro name ahead of macro resolution (incl. audited `SSHAPE`) | **Mostly automated** — guard tests prove macro-shadows precedence; the *audit conclusion* itself (which formatter arms carry the latent hazard) is a human-reviewed doc comment | P2 T4 | unit (existing guards) + doc-comment audit (human) | `src/simlin-engine/src/macro_expansion_tests.rs` (`ac5_4_macro_shadows_ramp_from_to_builtin`, `ac5_4_macro_shadows_sshape_builtin`); `cargo test -p simlin-engine --lib ac5_4_macro_shadows`. Audit prose in `xmile_compat.rs` above `format_call_ctx`. | + +Notes: the existing `simulates_macro_clearn_ramp_from_to_mdl` does NOT discriminate +bug from fix (its `islinear=1`/positive-endpoint series is identical either way); +the discriminating proof is the new T1 exponential-branch test. SSHAPE was confirmed +*not* a formatter-rewrite hazard (name-preserving via `format_function_name`) — T4 +records this rather than guarding it. `im_3_*`/`relative_emissions_*` reconciliation +is observed via the `--ignored` gate but pruned in Phase 4. + +--- + +## clearn-residual.AC3 — User-macro `INITIAL` recurrences produce correct multi-step values (Phase 3) + +| AC | Criterion (abridged) | Verification | Phase/Task | Type | File / run | +|----|----------------------|--------------|------------|------|------------| +| **AC3.1** | `:MACRO: INIT(x)=INITIAL(x)` + scalar capture holds constant across all saved steps | **Automated** | P3 T1 (RED) → T4 (GREEN) | integration (`file_io`, inline-MDL via `run_inline_mdl`) | `src/simlin-engine/tests/simulate.rs` (new inline `#[test]` near `simulates_macro_independent_invocation_state`); `cargo test -p simlin-engine --features file_io --test simulate ` | +| **AC3.2** | Element-wise `INITIAL` recurrence via passthrough macro correct at t0 and every step (no drop to `0`, no spurious `:NA:`) | **Automated** — primarily via T4 collapse; contingent T5 only if a non-passthrough module remains divergent | P3 T1 (RED) → T4 (GREEN); P3 T5 (contingent) | integration (`file_io`, file fixture) | fixture `test/test-models/tests/macro_init_recurrence/` (= `helper_recurrence.mdl` + prepended `:MACRO: INIT`); test in `src/simlin-engine/tests/simulate.rs` via `simulate_mdl_path`; `cargo test -p simlin-engine --features file_io --test simulate ` | +| **AC3.3** | Bare `INITIAL(expr)` (no user macro) still compiles to `LoadInitial` and behaves as today | **Automated** — preserved reference fixture | P3 T4 (no-regression) | integration (`file_io`) | `src/simlin-engine/tests/simulate.rs::helper_recurrence_mdl_synthetic_helper_in_scc_simulates`; `cargo test -p simlin-engine --features file_io --test simulate helper_recurrence` | +| **AC3.4** | Passthrough collapse fires only for genuine `out=BUILTIN(param)`; a name-sharing non-passthrough macro still expands as a macro | **Automated** — pure classifier unit tests + registry-level wiring tests | P3 T2 (classifier unit) + T3 (registry wiring) | unit | `src/simlin-engine/src/module_functions.rs` `#[cfg(test)]` (`classify_passthrough` positives/negatives; registry `passthrough==Some/None`); `cargo test -p simlin-engine --lib classify_passthrough` and `cargo test -p simlin-engine --lib macro_registry passthrough` | +| **AC3.5** | `SAMPLE UNTIL` fed by a corrected `INITIAL` samples until the correct time with no dedicated `SAMPLE UNTIL` change | **Automated, but indirectly** — resolves downstream of AC3.2; the C-LEARN `SAMPLE UNTIL`-fed bases reconcile in the Phase 4 `--ignored` gate (no dedicated `SAMPLE UNTIL` test/fixture is created). Phase 3 only *observes* movement (informational). | P3 T4 (observe) → P4 T4 (`simulates_clearn`/exactness confirm) | integration, `#[ignore]` runtime class | `src/simlin-engine/tests/simulate.rs` (`simulates_clearn`, `clearn_residual_exactness`); C-LEARN gate run command | + +Notes: `ensure_results` skips implicit `$⁚` module vars, so fixtures assert the +user-facing series directly. NO NA-arithmetic change is made (out of scope). Task 5 +is contingent — if performed it adds a minimal non-passthrough INITIAL-macro-module +regression fixture (default suite); if skipped, the skip is recorded with `--ignored` +evidence (a human/process artifact, not a test). + +--- + +## clearn-residual.AC4 — Residual honestly attributed and gate tightened (Phase 4) + +| AC | Criterion (abridged) | Verification | Phase/Task | Type | File / run | +|----|----------------------|--------------|------------|------|------------| +| **AC4.1** | After P1-3, `EXPECTED_VDF_RESIDUAL` reduced to the proven remainder and `simulates_clearn` passes | **Automated** | P4 T4 (depends on T1 classify, contingent T2/T3 fixes) | integration, **`file_io` + `#[ignore]` runtime class, release** | `src/simlin-engine/tests/simulate.rs::simulates_clearn`; `cargo test -p simlin-engine --features file_io --release --test simulate -- --ignored simulates_clearn` | +| **AC4.2** | `clearn_residual_exactness` passes — live failing set equals `EXPECTED_VDF_RESIDUAL` exactly (no `grew`/`shrank`) | **Automated** | P4 T4 | integration, **`file_io` + `#[ignore]` runtime class, release** | `src/simlin-engine/tests/simulate.rs::clearn_residual_exactness`; `... -- --ignored clearn_residual_exactness` | +| **AC4.3** | Every still-excluded base carries a sourced one-line reason (5-category taxonomy); #590/#591 updated | **Partly automated, partly human.** Automated: the exactness guard (AC4.2) proves the list is exactly the live remainder. Human: the prose category reasons in `EXPECTED_VDF_RESIDUAL` comments and the GitHub #590/#591 issue updates (`gh issue comment`/`close`) are reviewed, not asserted by a test. | P4 T1 (classify, human) + T4 (comments + `gh`) | guard (automated) + doc/issue review (human) | guard: `clearn_residual_exactness` (above). Reasons: `src/simlin-engine/tests/simulate.rs:~1563-1622` comments. Issues: #590, #591 (`gh`). | +| **AC4.4** | Comparator 1% tol / per-series floor / matched-variable floor unchanged; gate cannot pass vacuously | **Automated** — a non-ignored guard test pinning the five constants (`VDF_RTOL`, `K_ATOL`, `MIN_MATCHED_FRACTION`, `MIN_MATCHED_ABSOLUTE`, `MAX_NAN_SKIPPED_FRACTION`) + a sub-floor-panics assertion | P4 T4 (item 4) | unit (default suite) | `src/simlin-engine/tests/simulate.rs` (guard test pinning constants + below-floor panic); `cargo test -p simlin-engine` | +| **AC4.5** | NaN-vs-`:NA:` series (e.g. `slr_inches_from_2000`) handled by NaN-skip; never enters the failure set | **Automated** — `classify_vdf_ident` unit test on a synthetic NaN-vs-`:NA:` series (asserts `nan_skipped` cells, zero `failures`), C-LEARN-independent | P4 T4 (item 3) | unit (default suite) | `src/simlin-engine/tests/simulate.rs` (`classify_vdf_ident` synthetic-series unit test); `cargo test -p simlin-engine` | + +Notes: Task 1 (classification) and the contingent Task 2 (VDF-reader decode fix) / +Task 3 (engine-genuine fix) feed AC4.1/AC4.2 by removing reference-side artifacts and +real divergences from the live set; if performed, each ships a non-ignored synthetic +regression test (`cargo test -p simlin-engine vdf` for T2). The two C-LEARN gates are +intentionally NOT in the default `cargo test`. + +--- + +## clearn-residual.AC5 — Native targets resolve external-data functions (Phase 5 — cuttable) + +> **Cuttability:** Phase 5 is forward-looking and independent of C-LEARN residual +> closure (C-LEARN needs no external data). If the phase is cut, AC5.1–AC5.4 are +> not delivered and are not gating on the residual work. + +| AC | Criterion (abridged) | Verification | Phase/Task | Type | File / run | +|----|----------------------|--------------|------------|------|------------| +| **AC5.1** | Native (`file_io`) build resolves `GET DIRECT DATA`/`LOOKUPS`/`CONSTANTS` from a companion file relative to the model | **Automated** | P5 T1 (RED, extract `open_vensim_model`) → T2 (GREEN, wire provider) | integration (`file_io`) | `src/simlin-cli/tests/external_data.rs` (calls extracted `open_vensim_model`); fixture `test/sdeverywhere/models/directconst/` (primary) or tiny new `test/test-models/` CSV fixture; `cargo test -p simlin-cli external_data` | +| **AC5.2** | Resolved external data drives simulation (downstream value reflects the data, not a zeroed series) | **Automated** — same test asserts the CSV-derived downstream value | P5 T1 → T2 | integration (`file_io`) | `src/simlin-cli/tests/external_data.rs`; `cargo test -p simlin-cli external_data` | +| **AC5.3** | Missing/unreadable data file produces a clear diagnostic, not a silent `0+0` zeroing | **Automated** — missing-CSV test asserts a clear file-level diagnostic via the `FilesystemDataProvider` error path | P5 T2 | integration (`file_io`) | `src/simlin-cli/tests/external_data.rs` (missing-file case); `cargo test -p simlin-cli` | +| **AC5.4** | WASM/libsimlin remain on the null provider (unchanged); a tracked follow-up issue captures their data-supply API | **Partly automated, partly human/process.** Automated: WASM/libsimlin default-open paths untouched (diff/build confirm). Human/process: filing the tracked follow-up issue via the `track-issue` agent. | P5 T3 | build/diff confirm (automated) + issue filing (human) | `cargo test -p simlin-engine` + WASM build per repo conventions; follow-up issue filed via `track-issue` agent | + +--- + +## clearn-residual.AC6 — Cross-cutting correctness and hygiene + +| AC | Criterion (abridged) | Verification | Type | File / run | +|----|----------------------|--------------|------|------------| +| **AC6.1** | No change keys on a C-LEARN variable name, the C-LEARN `.mdl`/`.vdf` path, or the residual list | **Human verification** (grep-based review of the change set). Optionally scriptable as a grep check, but judgment is required (the `EXPECTED_VDF_RESIDUAL` carve-out legitimately contains base names; the reviewer must confirm it is the *only* C-LEARN contact and no code *branches* on those names). | grep review (human) | `git diff main...HEAD` reviewed for C-LEARN names/paths/residual-list branches; carve-out is the sole permitted contact | +| **AC6.2** | Each engine fix (Phases 1-3) ships ≥1 generality-proving test independent of C-LEARN | **Automated** — satisfied by the per-phase fixtures | unit/integration | P1: `lookup_only_tests.rs` + `is_lookup_only`; P2: `macro_expansion_tests.rs` exponential-branch + inverted formatter test; P3: `classify_passthrough` + scalar/element INIT fixtures. All run in the default suite (P3 element fixture under `file_io`). | +| **AC6.3** | `cargo test --workspace` green within the 3-min cap AND the pre-commit hook passes | **Automated** — enforced by the pre-commit hook / CI (Rust fmt/clippy/tests, TS lint/types, WASM build, TS tests, Python bindings) | suite / hook | pre-commit hook (`scripts/pre-commit`); `cargo test --workspace`. (The two C-LEARN `#[ignore]` gates are excluded from this capped run by design.) | +| **AC6.4** | New end-to-end C-LEARN tests stay `#[ignore]` (runtime class) and run via `--ignored` | **Human/structural check** — confirm `simulates_clearn` / `clearn_residual_exactness` retain `#[test] #[ignore]` and are absent from the default suite; optionally grep for the attribute. Not a behavioral assertion. | structural review (human) | `src/simlin-engine/tests/simulate.rs` (`#[ignore]` retained on both gates); confirm they don't run under bare `cargo test` | + +--- + +## Automated coverage summary + +All **29** sub-criteria (AC1: 6, AC2: 5, AC3: 5, AC4: 5, AC5: 4, AC6: 4) bucketed +exactly once: + +- **Automated, default suite (`cargo test -p simlin-engine`) — 18:** AC1.1, AC1.2, + AC1.3, AC1.4, AC1.5, AC1.6, AC2.1, AC2.3, AC2.4, AC3.1, AC3.2, AC3.3, AC3.4, AC4.4, + AC4.5, AC6.2, AC6.3, plus AC5.1, AC5.2, AC5.3 *if Phase 5 ships* (Phase 5 is + cuttable; AC2.2 also has a `file_io` integration leg). These run unignored under + the pre-commit hook / CI cap. (AC3.2's element fixture and AC2.2's regression + fixture require `--features file_io`; they are still part of the unignored suite.) +- **Automated, `#[ignore]` runtime class (release, `--ignored`) — 2:** AC4.1 + (`simulates_clearn`), AC4.2 (`clearn_residual_exactness`). Run only via + `cargo test -p simlin-engine --features file_io --release --test simulate -- --ignored ...`; + intentionally NOT in the default `cargo test`/pre-commit run (per AC6.3, AC6.4). +- **Hybrid (automated test/guard + a human/process component) — 4:** AC2.5 (shadow + guards automated; audit conclusion is a reviewed doc comment), AC3.5 (resolves + downstream — confirmed by the automated `--ignored` gate, no dedicated test), + AC4.3 (exactness guard automated; the per-base category prose and #590/#591 issue + updates are human-reviewed), AC5.4 (WASM/libsimlin-unchanged is build/diff-confirmable; + filing the follow-up issue via the `track-issue` agent is process). +- **Primarily human/process — 2:** AC6.1 (grep/review the change set for C-LEARN + special-casing), AC6.4 (`#[ignore]` retention / not-in-default-suite structural + check). + +Process artifacts feeding the automated rules (not themselves ACs): AC1.1's +Vensim-semantics *determination* (Phase 1 Task 1 spike) is a written conclusion whose +rule is then validated by the `lookup_only` fixtures; Phase 3 Task 5's skip rationale +and Phase 4 Task 1's classification table are human-recorded inputs to automated +guards. Phase 5's three contingent/automated ACs (AC5.1–AC5.3) drop if the cuttable +phase is not shipped. + +## Human verification checklist + +1. **AC1.1 (spike conclusion)** — Confirm Phase 1 Task 1's empirical determination of + the standalone-lookup saved-value rule is recorded (evidence + chosen `index_expr`) + as the `lookup_only_tests.rs` module doc comment, and that temporary instrumentation + was reverted. (Rule correctness is then automated.) +2. **AC2.5 (formatter audit)** — Read the doc comment above `format_call_ctx`'s match: + it must record which restructuring arms carry the macro-shadowing hazard and note + that `SSHAPE` is name-preserving (no guard needed). Confirm no new formatter arm + rewrites a name a model could define as a macro. +3. **AC3.5 (downstream)** — Confirm no dedicated `SAMPLE UNTIL` code/test was added and + that the `SAMPLE UNTIL`-fed bases reconcile via the Phase 4 `--ignored` gate. +4. **AC3 Task 5 (contingent)** — Confirm Task 5 was either performed (with a minimal + non-passthrough INITIAL-macro regression fixture green) or explicitly skipped with + recorded `--ignored` evidence that the call-site collapse sufficed. +5. **AC4.3 (attribution + issues)** — Confirm every still-excluded base in + `EXPECTED_VDF_RESIDUAL` has a one-line sourced reason under one of the five + categories (no "unknown"), and that #590/#591 were updated/closed with the final + per-base disposition. +6. **AC5.4 (WASM/libsimlin + follow-up)** — Confirm the WASM/libsimlin default-open + paths are untouched by the Phase 5 diff and that the data-supply follow-up issue + was filed via the `track-issue` agent. (Skip if Phase 5 is cut.) +7. **AC6.1 (no special-casing)** — Grep/review the full change set: no code branches on + a C-LEARN variable name, the C-LEARN `.mdl`/`.vdf` path, or the residual list; the + `EXPECTED_VDF_RESIDUAL` carve-out is the only place C-LEARN base names appear. +8. **AC6.4 (`#[ignore]` retention)** — Confirm `simulates_clearn` and + `clearn_residual_exactness` retain `#[test] #[ignore]` and do not run under bare + `cargo test`. diff --git a/docs/tech-debt.md b/docs/tech-debt.md index 6d80d4f2b..dbf881e2d 100644 --- a/docs/tech-debt.md +++ b/docs/tech-debt.md @@ -597,3 +597,14 @@ Known debt items consolidated from CLAUDE.md files and codebase analysis. Each e - **Suspected fix**: Commit the implementation plan at the **start** of execution — the `starting-an-implementation-plan` / `executing-an-implementation-plan` step should commit `docs/implementation-plans//` before the first task subagent runs — rather than rescuing it at the `finishing-a-development-branch` step. Optionally add a guard in the execute step that refuses to start if the plan directory is untracked. - **Owner**: unassigned - **Last reviewed**: 2026-05-15 + +### 64. Doubled `ImportError{generic: ...}` wrapping in user-facing CLI error messages + +- **Component**: simlin-engine (`src/simlin-engine/src/common.rs:239-252` `impl fmt::Display for Error`), simlin-cli (`src/simlin-cli/src/main.rs:217` `die!`) +- **Severity**: low +- **Description**: The CLI surfaces engine import/parse errors with a verbose, doubled debug-style wrapper. For a model whose `GET DIRECT *` data file is missing, the message is `model '...' error: ImportError{generic: Failed to parse MDL: import error: ImportError{generic: cannot resolve data file 'does_not_exist.csv': No such file or directory (os error 2)}}`. The `ImportError{generic: ...}` wrapper appears **twice** and the inner OS error is nested several layers deep. Root cause: `Error`'s `Display` renders `write!(f, "{}{{{}: {}}}", kind, self.code, details)` (-> `ImportError{generic:
}`); when an inner `Error` is stringified into an outer error's `details` (MDL import wrapping a data-resolution `Error`), the prefix nests verbatim and double-wraps. The CLI then prints the whole thing via `die!("model '{}' error: {}", &file_path, err)`. The essential signal (file name + OS cause) is present and correct, so this is cosmetic/developer-experience polish, not a correctness bug. **Pre-existing** (present before the Phase 5 C-LEARN-residual work; the `die!` formatting is unchanged at base SHA `55d62b74`). Distinct from #581 (assembly diagnostics silently discarded), #437 (CLI debug truncation), and #598 (WASM data-supply API). +- **Measure**: Run `simlin-cli` against an MDL model with an unresolvable `GET DIRECT *` reference (e.g. a sibling `does_not_exist.csv`) and inspect the `model '...' error:` line for the doubled `ImportError{generic:` prefix. +- **Suspected fix**: Flatten the `Error` `Display` so a nested import error does not re-emit the `ImportError{generic: ...}` prefix (e.g. detect/strip the wrapper when the `details` string is itself a rendered `Error`), or have the CLI walk and render a cleaner error chain rather than printing the raw nested `Display`. +- **Owner**: unassigned +- **Discovered**: C-LEARN residual Phase 5 code review (the AC5.3 missing-data-file diagnostic path exercises it) +- **Last reviewed**: 2026-05-20 diff --git a/docs/test-plans/2026-05-19-clearn-residual.md b/docs/test-plans/2026-05-19-clearn-residual.md new file mode 100644 index 000000000..729ade404 --- /dev/null +++ b/docs/test-plans/2026-05-19-clearn-residual.md @@ -0,0 +1,198 @@ +# C-LEARN Residual — Human Test Plan + +Generated by the test-analyst after Phase 1 coverage validation **PASSED** for the +five-phase C-LEARN residual work (`docs/implementation-plans/2026-05-19-clearn-residual/`, +branch `clearn-residual`, range `eed5f11a..f542fcee`). + +Every acceptance criterion in +[`test-requirements.md`](/docs/implementation-plans/2026-05-19-clearn-residual/test-requirements.md) +has automated coverage that was confirmed to verify real behavior (not vacuous). +This plan adds human-executable verification for the hybrid/human-mapped criteria +and the cross-phase end-to-end behaviors that benefit from a person's judgment. + +The work is a set of **general** Vensim import/simulation primitives — none keys on +a C-LEARN name, path, or the residual list. The single C-LEARN contact is the +read-only `EXPECTED_VDF_RESIDUAL` carve-out, exercised only by the two `#[ignore]`d +release gates. Keep that framing in mind while verifying: a reviewer's job here is +largely to confirm the *generality* and the *honesty of the residual attribution*, +not to re-derive numeric series (the automated gates do that). + +--- + +## Prerequisites + +- Toolchain per `./scripts/dev-init.sh` (run it once; idempotent). +- `gh` CLI authenticated (for the AC4.3 / AC5.4 issue-disposition checks). +- Working tree at `f542fcee` (or merged equivalent), clean. +- These commands must be green before starting (the analyst confirmed each): + + ```bash + # Default engine suite (capped, no C-LEARN gates) — 3550 lib tests + 96 simulate + cargo test -p simlin-engine + cargo test -p simlin-engine --features file_io --test simulate + + # CLI suite (Phase 5 external-data tests) + cargo test -p simlin-cli + + # The two release-only C-LEARN gates (NOT in the default suite; ~5s parse) + cargo test -p simlin-engine --features file_io --release --test simulate -- \ + --ignored simulates_clearn clearn_residual_exactness + ``` + +--- + +## Phase 1: Lookup-only `gf(Time)` lowering (#590 → reframed as #597 reader) + +| Step | Action | Expected | +|------|--------|----------| +| 1.1 | Open `src/simlin-engine/src/lookup_only_tests.rs`. Read the module doc comment ("Phase 1 determination: the standalone index is `gf(Time)`"). | The spike conclusion is recorded as prose with evidence: applied consumers byte-match `gf(Time)` against `Ref.vdf`, and the importer's `MdlEquation::Implicit` arm already lowers an unspecified-input gf to `equation = "TIME"`. The chosen `index_expr` is `Expr::App(BuiltinFn::Time, …)`. **AC1.1 human leg.** | +| 1.2 | `git diff eed5f11a..f542fcee -- src/simlin-engine/src/compiler/mod.rs` and grep for `eprintln!`/`dbg!`/`println!`. | No temporary instrumentation remains (the analyst confirmed empty). The determination is permanent doc, not live probes. | +| 1.3 | Read `lookup_only_index_expr` (compiler/mod.rs ~line 1188) and `var_is_lookup_only` (~1158). | A single centralized `gf(Time)` index used by scalar / A2A / arrayed sites; `var_is_lookup_only` requires `has_tables` AND empty-or-sentinel equation. Confirm WITH LOOKUP (real input) is excluded. | +| 1.4 | `cargo test -p simlin-engine --lib lookup_only` then `cargo test -p simlin-engine --lib per_element_gf`. | 12 + 16 tests pass. The arrayed fixture uses declared order `Z, A, M` (non-alphabetical) with element-identifying triples, so a positional mis-map would visibly swap series. AC1.6 no-regression holds. | + +**End-to-end (Phase 1):** the `arrayed_lookup_only_non_sorted_order_*` and +`a2a_lookup_only_shares_one_table_at_time` tests are the layout-hazard proof — a +`Shared`-layout element reading `off + i` would push the VM `Lookup` out of range +and return NaN. Confirm both shapes return the per-year y-values, not NaN/0. + +--- + +## Phase 2: Shadowed `RAMP FROM TO` macro survives import + +| Step | Action | Expected | +|------|--------|----------| +| 2.1 | Open `src/simlin-engine/src/mdl/xmile_compat.rs`, read the doc comment above `format_call_ctx`'s `match` (the macro-shadowing audit, ~line 260). | **AC2.5 human leg.** It records: which restructuring arms carry the latent shadow hazard (`sample if true`, `pulse`, `modulo`, `zidz`, `xidz`, `get data between times`, …); that the `ramp from to` arm was *removed*; and that `SSHAPE` is name-preserving (handled only by `format_function_name`, no guard needed). | +| 2.2 | Scan the `match canonical.as_str()` arms in `format_call_ctx` for any arm that rewrites a builtin name a model could define as a `:MACRO:`. | No `"ramp from to" =>` restructuring arm exists. The only `"ramp from to"` reference is the name-preserving `=> "RAMP_FROM_TO"` in `format_function_name` (~line 568). No *new* restructuring arm was introduced. | +| 2.3 | `cargo test -p simlin-engine --lib ramp_from_to` and `cargo test -p simlin-engine --lib ac5_4_macro_shadows`. | `test_ramp_from_to_survives_as_macro_call`, `macro_ramp_from_to_runs_selected_branch`, and both `ac5_4_macro_shadows_{ramp_from_to,sshape}_builtin` pass. The exponential-branch test uses *double slope* so the macro result (110) is provably distinct from the import-time linearization (60). | +| 2.4 | `cargo test -p simlin-engine --features file_io --test simulate simulates_macro_clearn_ramp_from_to_mdl`. | The file fixture (`test/test-models/tests/macro_clearn_ramp_from_to/`) runs the `islinear=1` branch end-to-end and matches `output.tab`. | + +--- + +## Phase 3: Passthrough-`INIT` macro collapse + +| Step | Action | Expected | +|------|--------|----------| +| 3.1 | `cargo test -p simlin-engine --lib classify_passthrough` and `cargo test -p simlin-engine --lib passthrough`. | The pure classifier passes on the positive (`init(x)`) and on every negative: Op2 body (`init(x)+1`), expression arg (`init(x*2)`), two-param arity, non-collision builtin (`abs`), multi-output, non-self-call. Registry-build wiring (`build_classifies_init_passthrough_macro_as_some`, `build_does_not_classify_near_miss_init_macro`) confirms the descriptor's `passthrough` field. **AC3.4.** | +| 3.2 | `cargo test -p simlin-engine --features file_io --test simulate simulates_passthrough_init_macro`. | Both `_scalar_capture_is_constant` (AC3.1: `captured = INIT(growing)` pinned at INITIAL TIME=2 while `growing` rises 2..6) and `_element_recurrence` (AC3.2: `ecc[t1..t3] = 1,2,4` via the macro path) pass. | +| 3.3 | `cargo test -p simlin-engine --features file_io --test simulate helper_recurrence`. | The bare-`INITIAL` reference fixture (AC3.3, no user macro) still compiles to `LoadInitial` and simulates `1,2,4`. This is the side-by-side control for AC3.2. | +| 3.4 | **AC3.5 (downstream — manual judgment).** `git diff eed5f11a..f542fcee` and search for any new `SAMPLE UNTIL` code path or dedicated `SAMPLE UNTIL` test/fixture. | None added. The `SAMPLE UNTIL`-fed bases reconcile *only* through the corrected `INITIAL` upstream, confirmed by the `simulates_clearn` gate passing. No dedicated change was the right call. | +| 3.5 | **AC3 Task 5 (contingent — manual).** Check `phase_03.md` and the commit bodies for whether Task 5 (non-passthrough INITIAL-macro regression fixture) was performed or skipped-with-evidence. | Either a minimal non-passthrough fixture is green in the default suite, OR the skip is recorded with `--ignored` evidence that the call-site collapse sufficed. Confirm the recorded disposition matches what's in the tree (no orphan/half-built fixture). | + +--- + +## Phase 4: Residual attribution and gate tightening + +| Step | Action | Expected | +|------|--------|----------| +| 4.1 | Open `src/simlin-engine/tests/simulate.rs`, read `EXPECTED_VDF_RESIDUAL` (~line 1746) and its header doc. | **AC4.3 human leg.** Exactly 21 base variables, each with a one-line sourced reason under one of five categories: engine-genuine-tracked (0), VDF-decode-artifact (17, #597), benign-near-zero (2, #591), NaN-vs-`:NA:` (0 by construction), boundary `:NA:`-arithmetic (2, #591). No reason is "unknown". | +| 4.2 | Verify the category counts add up: 17 + 2 + 2 = 21, and the empty categories (engine-genuine, NaN-vs-:NA:) carry an explanation for *why* they're empty. | The init-runlist nondeterminism (the sole engine-genuine divergence) is documented as fixed in `e24b0080`; the NaN-vs-`:NA:` series are documented as comparator-skipped, never failing. | +| 4.3 | `cargo test -p simlin-engine --features file_io --release --test simulate -- --ignored clearn_residual_exactness`. | Passes. The live failing set equals `EXPECTED_VDF_RESIDUAL` **exactly** — no `grew` (regression) or `shrank` (a fix to prune). This is the machine guarantee behind the prose in step 4.1. | +| 4.4 | `cargo test -p simlin-engine --features file_io --release --test simulate -- --ignored simulates_clearn`. | Passes. Hard 1% tolerance holds for every non-excluded variable; the matched floor is checked *after* exclusion (the doc cites ~3360 matched vs a 348 floor, ~9.7x). | +| 4.5 | `cargo test -p simlin-engine --features file_io --test simulate vdf_comparator_constants_and_floor_are_pinned`. | **AC4.4.** The five constants are pinned exactly (`VDF_RTOL=0.01`, `K_ATOL=1e-4`, `MIN_MATCHED_FRACTION=0.10`, `MIN_MATCHED_ABSOLUTE=10`, `MAX_NAN_SKIPPED_FRACTION=0.10`) and a below-floor comparison (1 of 16 matched, all in-tolerance) **panics** — the gate cannot pass vacuously. | +| 4.6 | `cargo test -p simlin-engine --features file_io --test simulate classify_vdf_ident` and `... ensure_vdf_results_rejects_vacuous`. | **AC4.5.** A synthetic NaN-vs-`:NA:` series with a finite tail is NaN-skipped (3 skipped, 3 compared, 0 failures, not all-NaN) so it never enters the residual; an entirely-NaN series *would*. The 7 vacuous-comparison scenarios (below-floor, all-NaN core, excessive NaN fraction, spurious `:NA:`, near-zero benign vs meaningful) each get the right panic/pass outcome. | +| 4.7 | `gh issue view 590`, `591`, `597`, `595`. | **AC4.3 issues.** All exist and reflect the final disposition: #597 (arrayed lookup-only VDF reader mis-decode) supersedes #590's "engine 0+0" framing; #591 holds the benign-near-zero + `:NA:`-arithmetic remainder; #595 holds the deeper init-ordering soundness gap (its nondeterminism half fixed by `e24b0080`). Confirm the comments match the taxonomy. | + +--- + +## Phase 5: CLI external-data resolution (shipped, not cut) + +> Note: the implementation tests live in an inline `#[cfg(test)] mod open_vensim_model_tests` +> in `src/simlin-cli/src/main.rs`, not the `tests/external_data.rs` path the +> requirements predicted. This is an equivalent location run by the default CLI suite. + +| Step | Action | Expected | +|------|--------|----------| +| 5.1 | `cargo test -p simlin-cli`. | 3 tests pass: `resolves_get_direct_constants_from_filesystem` (AC5.1/AC5.2), `missing_data_file_yields_clear_diagnostic` (AC5.3), `stdin_get_direct_yields_null_provider_error`. | +| 5.2 | **Manual CLI run (AC5.1/AC5.2 end-to-end).** From the repo root: `cargo run -p simlin-cli -- simulate test/sdeverywhere/models/directconst/directconst.mdl` and look at the `a` column. | `a` resolves to `2050` (cell B2 of `data/a.csv`), not `0`/NaN. The `GET DIRECT CONSTANTS('data/a.csv', ',', 'B2')` reference resolved relative to the model file. | +| 5.3 | **Manual diagnostic (AC5.3).** Create a temp `.mdl` with `a = GET DIRECT CONSTANTS('nope.csv', ',', 'B2')` and `simulate` it. | Errors with a message naming `nope.csv` and "cannot resolve data file" — a clear file-level diagnostic, not a silent `0`. | +| 5.4 | **Manual stdin (AC5.4 + design intent).** `cat test/sdeverywhere/models/directconst/directconst.mdl \| cargo run -p simlin-cli -- simulate`. | Surfaces the engine's "no DataProvider configured" error. The provider is keyed on the path *argument*, so a `< file` redirection (where `/dev/stdin` is a regular file) does NOT wrongly root a provider at `/dev`. | +| 5.5 | **AC5.4 (WASM/libsimlin untouched + follow-up).** `git diff --stat eed5f11a..f542fcee -- src/libsimlin src/engine/core src/simlin-mcp src/simlin-serve` and `gh issue view 598`. | Zero changes to libsimlin/engine-core/mcp/serve (they remain on the null provider). Issue **#598** ("WASM/libsimlin: data-supply API for GET DIRECT external data") is filed, capturing the deferred data-supply API. | + +--- + +## End-to-End: full C-LEARN reconciliation + +**Purpose:** validate that the five primitives *together* let the real 53k-line +hero model compile via the incremental path, run to FINAL TIME, and match +genuine Vensim (`Ref.vdf`) within the 1% cross-simulator tolerance on the matched +floor — with the residual honestly banked. + +Steps: +1. `cargo test -p simlin-engine --features file_io --release --test simulate -- --ignored simulates_clearn clearn_residual_exactness` +2. Confirm both pass and the run prints the matched-ident count and live-residual + size (the exactness test logs `N idents matched; M live residual bases vs M excluded`). +3. Sanity: the two numbers (live residual vs excluded) are equal — that equality + is the whole point of the exactness guard. + +This is the only place C-LEARN is touched, and only read-only. + +--- + +## End-to-End: generality — no C-LEARN special-casing + +**Purpose:** AC6.1 / AC6.2 — confirm every engine fix is a general primitive with +≥1 C-LEARN-independent test, and no code branches on a hero-model name/path/residual. + +Steps: +1. `grep -rln "last_set_target_year\|co2eq_gap_closing\|historical_forestry_lookup\|ozone_precursor_forcings" --include="*.rs" src/` + → only `src/simlin-engine/tests/simulate.rs` (the `EXPECTED_VDF_RESIDUAL` carve-out). +2. `git diff eed5f11a..f542fcee | grep -iE "^\+" | grep -iE "c-learn|clearn|Ref\.vdf"` + → matches appear only in comments and the carve-out, never as a code *branch* on + a name. Confirm by eye that no `if name == "…"` / `match` arm keys on a residual + variable. +3. Confirm each phase's generality fixture is C-LEARN-free: + - P1 `lookup_only_tests.rs` (synthetic year-indexed tables) + `is_lookup_only` units + - P2 `macro_ramp_from_to_runs_selected_branch` (inline MDL) + `test_ramp_from_to_survives_as_macro_call` + - P3 `classify_passthrough` units + `simulates_passthrough_init_macro_*` (synthetic fixtures) + - P5 `directconst` (an sdeverywhere fixture, not C-LEARN) + +--- + +## Human Verification Required + +| Criterion | Why Manual | Steps | +|-----------|------------|-------| +| **AC1.1** (spike conclusion) | The empirical determination of the `gf(Time)` rule is a written conclusion, not a behavioral assert (the rule's *correctness* is then automated). | Step 1.1–1.2: confirm the determination + evidence is in the `lookup_only_tests.rs` module doc and instrumentation was reverted. | +| **AC2.5** (formatter audit) | Identifying *which* restructuring arms carry the latent macro-shadow hazard, and that `SSHAPE` is name-preserving, is a code-reading conclusion. | Step 2.1–2.2: read the `format_call_ctx` audit comment; confirm no new restructuring arm shadows a macro-definable name. | +| **AC3.5** (downstream) | Confirming the *absence* of a dedicated `SAMPLE UNTIL` change, and that the fix is purely upstream, needs judgment. | Step 3.4: diff for `SAMPLE UNTIL`; confirm reconciliation rides the `simulates_clearn` gate. | +| **AC3 Task 5** (contingent) | Whether the contingent task ran or was skipped-with-evidence is a process disposition. | Step 3.5: cross-check `phase_03.md` / commit bodies against the tree. | +| **AC4.3** (attribution + issues) | The per-base category prose and the #590/#591/#597/#595 issue updates are reviewed, not asserted by a test (the exactness guard only proves the *list membership*). | Step 4.1–4.2, 4.7: read every reason; confirm no "unknown"; verify the four issues' dispositions. | +| **AC5.4** (WASM/libsimlin + follow-up) | "Untouched" is diff-confirmable but the *intent* (deliberate null-provider hold) + filing #598 is process. | Step 5.5: confirm zero diff to those crates and that #598 exists. | +| **AC6.1** (no special-casing) | The carve-out legitimately contains base names; a human must confirm it is the *only* contact and nothing *branches* on them. | E2E "generality" steps 1–3. | +| **AC6.4** (`#[ignore]` retention) | Structural check that the two gates retain `#[test] #[ignore]` and are absent from the default suite. | `cargo test -p simlin-engine --features file_io --test simulate -- --list --ignored \| grep clearn` lists both; running them by name without `--ignored` reports "0 passed; 1 ignored". | + +--- + +## Traceability + +| Acceptance Criterion | Automated Test | Manual Step | +|----------------------|----------------|-------------| +| AC1.1 scalar `gf(Time)` | `lookup_only_tests::scalar_lookup_only_evaluates_at_time` | 1.1 (spike doc) | +| AC1.2 arrayed per-element | `lookup_only_tests::arrayed_lookup_only_non_sorted_order_*`, `…_variable_level_gf_*` | 1.4 | +| AC1.3 apply-to-all shared table | `lookup_only_tests::a2a_lookup_only_shares_one_table_at_time` | 1.4 | +| AC1.4 applied lookup no-regression | `compiler::is_lookup_only_tests::*` + `lookup_only_tests::applied_lookup_only_*` | 1.3 | +| AC1.5 non-alphabetical order | `lookup_only_tests::arrayed_lookup_only_non_sorted_order_*` | 1.4 | +| AC1.6 OOB clamp unchanged | `per_element_gf_tests::*` (16) | 1.4 | +| AC2.1 exp branch runs | `macro_expansion_tests::macro_ramp_from_to_runs_selected_branch` (`y_exp`) | 2.3 | +| AC2.2 linear branch no-regression | `…runs_selected_branch` (`y_lin`) + `simulates_macro_clearn_ramp_from_to_mdl` | 2.3, 2.4 | +| AC2.3 call survives import | `xmile_compat::tests::test_ramp_from_to_survives_as_macro_call` | 2.3 | +| AC2.4 nonpositive forces linear | `…runs_selected_branch` (`y_force`) | 2.3 | +| AC2.5 no formatter shadow | `ac5_4_macro_shadows_{ramp_from_to,sshape}_builtin` | 2.1, 2.2 | +| AC3.1 scalar capture constant | `simulates_passthrough_init_macro_scalar_capture_is_constant` | 3.2 | +| AC3.2 element recurrence | `simulates_passthrough_init_macro_element_recurrence` | 3.2 | +| AC3.3 bare INITIAL reference | `helper_recurrence_mdl_synthetic_helper_in_scc_simulates` | 3.3 | +| AC3.4 passthrough classifier | `module_functions::tests::classify_passthrough_*` + `build_*passthrough*` | 3.1 | +| AC3.5 SAMPLE UNTIL downstream | `simulates_clearn` (gate) | 3.4 | +| AC4.1 `simulates_clearn` passes | `simulates_clearn` (`#[ignore]`, release) | 4.4 | +| AC4.2 residual exactness | `clearn_residual_exactness` (`#[ignore]`, release) | 4.3 | +| AC4.3 sourced attribution | `clearn_residual_exactness` (membership guard) | 4.1, 4.2, 4.7 | +| AC4.4 constants/floor pinned | `vdf_comparator_constants_and_floor_are_pinned` | 4.5 | +| AC4.5 NaN-vs-`:NA:` skip | `classify_vdf_ident_nan_vs_na_skips_without_failing` | 4.6 | +| AC5.1 native resolves GET DIRECT | `open_vensim_model_tests::resolves_get_direct_constants_from_filesystem` | 5.1, 5.2 | +| AC5.2 data drives sim | same test (asserts `a == 2050`) | 5.2 | +| AC5.3 missing file diagnostic | `open_vensim_model_tests::missing_data_file_yields_clear_diagnostic` | 5.3 | +| AC5.4 WASM/libsimlin unchanged + follow-up | diff (untouched) + issue #598 | 5.5 | +| AC6.1 no C-LEARN special-casing | grep review | E2E generality | +| AC6.2 per-phase generality test | P1/P2/P3/P5 fixtures (above) | E2E generality | +| AC6.3 workspace green + hook | `cargo test --workspace` / pre-commit | Prerequisites | +| AC6.4 `#[ignore]` retention | `--list --ignored` structural check | AC6.4 row above | diff --git a/src/simlin-cli/CLAUDE.md b/src/simlin-cli/CLAUDE.md index 1bd4c2ca7..66dc6b73e 100644 --- a/src/simlin-cli/CLAUDE.md +++ b/src/simlin-cli/CLAUDE.md @@ -2,6 +2,8 @@ CLI tool for simulating and converting SD models. Primarily used for testing and debugging. +**Last updated: 2026-05-20** + For global development standards, see the root [CLAUDE.md](/CLAUDE.md). For build/test/lint commands, see [docs/dev/commands.md](/docs/dev/commands.md). @@ -26,3 +28,7 @@ Uses [clap](https://docs.rs/clap) derive API. Each subcommand declares exactly t Commands that read model files (`simulate`, `convert`, `equations`, `debug`) share `InputArgs` via `#[command(flatten)]`: - Positional `PATH` (optional for `simulate`, reads stdin) - `--format ` -- auto-detected from file extension when omitted (`.mdl` -> vensim, `.pb`/`.bin` -> protobuf, `.txt` -> systems, everything else -> xmile). Systems format output shows only non-infinite stocks in declaration order. + +## External data resolution (Vensim `GET DIRECT *`) + +A Vensim model opened from a **named path** resolves its `GET DIRECT *` (DATA, CONSTANTS, LOOKUPS, SUBSCRIPT) references against a `FilesystemDataProvider` rooted at the *model file's parent directory* (a bare filename roots at `.`), matching Vensim's relative-to-model resolution (`open_vensim_model` in `main.rs`). A model read from **stdin** (a pipe or `< file`) gets the null provider and any external-data reference surfaces the engine's "no DataProvider configured" error -- the provider is keyed on the path *argument* (the user's intent), NOT on an `is_file()` check of the `stdin` device sentinel, which under a `< model.mdl` redirection resolves to a regular file and would wrongly root a provider at the device's parent directory. diff --git a/src/simlin-cli/Cargo.toml b/src/simlin-cli/Cargo.toml index 5216d9118..575c4b1d1 100644 --- a/src/simlin-cli/Cargo.toml +++ b/src/simlin-cli/Cargo.toml @@ -19,3 +19,6 @@ simlin-engine = { version = "0.1", path = "../simlin-engine", features = ["file_ # a direct dep + local `#[global_allocator]` so there is exactly one global # allocator in this artifact even under `cargo clippy --all-features`. simlin = { version = "0.1", path = "../libsimlin", features = ["mimalloc"] } + +[dev-dependencies] +tempfile = "3" diff --git a/src/simlin-cli/src/main.rs b/src/simlin-cli/src/main.rs index de9f88260..7d26396e6 100644 --- a/src/simlin-cli/src/main.rs +++ b/src/simlin-cli/src/main.rs @@ -12,7 +12,7 @@ use std::fs::File; use std::io::{BufRead, BufReader, Write}; -use std::path::PathBuf; +use std::path::{Path, PathBuf}; use std::result::Result as StdResult; use clap::{Args, Parser, Subcommand, ValueEnum}; @@ -21,6 +21,7 @@ use simlin::errors::{ FormattedError, FormattedErrorKind, FormattedErrors, format_diagnostic, format_simulation_error, }; use simlin_engine::common::ErrorKind; +use simlin_engine::data_provider::FilesystemDataProvider; use simlin_engine::datamodel::Project as DatamodelProject; use simlin_engine::db::{ PersistentSyncState, SimlinDb, SourceProject, collect_all_diagnostics, @@ -30,7 +31,9 @@ use simlin_engine::db::{ }; use simlin_engine::prost::Message; use simlin_engine::{Error, ErrorCode, Result, Results, Vm, datamodel, project_io, serde}; -use simlin_engine::{load_csv, load_dat, open_vensim, open_xmile, to_mdl, to_xmile}; +use simlin_engine::{ + load_csv, load_dat, open_vensim, open_vensim_with_data, open_xmile, to_mdl, to_xmile, +}; mod gen_stdlib; mod vdf_dump; @@ -192,7 +195,10 @@ fn open_model(input: &InputArgs) -> (DatamodelProject, Option) { let (result, visible) = match format { InputFormat::Vensim => { let contents = std::fs::read_to_string(&file_path).unwrap(); - (open_vensim(&contents), None) + // `input.path` is the user's intent: `Some` for a named model file + // (anchors the external-data root), `None` for stdin (pipe or + // `< file`) where no data root can be inferred. + (open_vensim_model(input.path.as_deref(), &contents), None) } InputFormat::Protobuf => { let file = File::open(&file_path).unwrap(); @@ -223,6 +229,42 @@ fn open_model(input: &InputArgs) -> (DatamodelProject, Option) { } } +/// Parse a Vensim MDL model, resolving any `GET DIRECT *` external-data +/// references against the filesystem. +/// +/// `path` carries the user's intent, not filesystem metadata: +/// +/// - `Some(model_path)` -- the user named a model file on the command line. +/// Its parent directory roots a [`FilesystemDataProvider`], so a relative +/// reference like `GET DIRECT CONSTANTS('data/a.csv', ...)` resolves to a +/// sibling of the model file (matching Vensim's relative-to-model +/// resolution). A bare filename resolves companions against the CWD. +/// - `None` -- the model was read from stdin (whether a pipe or a `< file` +/// redirection). There is no model path to anchor a data root, so no +/// provider is built and any external-data reference remains unresolved; +/// the engine reports a clear "no DataProvider configured" error. +/// +/// Keying on the path argument rather than `is_file()` of stdin's +/// `/dev/stdin` sentinel matters: under `simlin simulate < model.mdl`, +/// `/dev/stdin` resolves to a regular file, so an `is_file()` check would +/// wrongly build a provider rooted at `/dev`. +fn open_vensim_model(path: Option<&Path>, contents: &str) -> Result { + match path { + Some(model_path) => { + // A bare filename (e.g. `model.mdl`) has an empty parent; its data + // companions live in the current directory, so use "." there. + let base_dir = match model_path.parent() { + Some(p) if !p.as_os_str().is_empty() => p, + _ => Path::new("."), + }; + let provider = FilesystemDataProvider::new(base_dir); + open_vensim_with_data(contents, Some(&provider)) + } + // stdin (pipe or `< file`): no model path to anchor a data root. + None => open_vensim(contents), + } +} + fn open_binary(reader: &mut dyn BufRead) -> Result { let mut contents_buf: Vec = vec![]; reader.read_until(0, &mut contents_buf).map_err(|_err| { @@ -703,3 +745,117 @@ fn main() { } } } + +#[cfg(test)] +mod open_vensim_model_tests { + use super::*; + use simlin_engine::common::{Canonical, Ident}; + + /// Compile and run a datamodel project to completion, returning the + /// first row of results. Mirrors the CLI's non-LTM `run_simulation` + /// path so the test exercises the same incremental-salsa pipeline the + /// `simulate` subcommand uses. + fn run_to_first_row(project: &DatamodelProject) -> Results { + let mut db = SimlinDb::default(); + let sync_state = sync_from_datamodel_incremental(&mut db, project, None); + run_simulation(&db, sync_state.project, "main") + .unwrap_or_else(|e| panic!("simulation failed: {e}")) + } + + fn scalar_value(results: &Results, name: &str) -> f64 { + let ident = Ident::::new(name); + let off = results.offsets[&ident]; + results.iter().next().unwrap()[off] + } + + /// AC5.1/AC5.2: a `GET DIRECT CONSTANTS` model opened through the CLI's + /// `open_vensim_model` resolves its companion CSV (relative to the model + /// file) and the resolved value drives simulation. + /// + /// `directconst.mdl`'s `a = GET DIRECT CONSTANTS('data/a.csv', ',', 'B2')` + /// reads cell B2 of `data/a.csv` (`a,\n,2050`), i.e. `2050`. The expected + /// `directconst.dat` confirms `a -> 2050`. The assertion is tied to the + /// actual CSV contents, not to zero/NaN. + #[test] + fn resolves_get_direct_constants_from_filesystem() { + let path = Path::new("../../test/sdeverywhere/models/directconst/directconst.mdl"); + let contents = std::fs::read_to_string(path) + .unwrap_or_else(|e| panic!("failed to read fixture {}: {e}", path.display())); + + let project = open_vensim_model(Some(path), &contents) + .unwrap_or_else(|e| panic!("open_vensim_model failed: {e}")); + + let results = run_to_first_row(&project); + let a = scalar_value(&results, "a"); + assert!( + (a - 2050.0).abs() < 1e-6, + "a must resolve to the CSV value 2050 (data/a.csv cell B2), got {a}" + ); + } + + /// AC5.3: a model referencing a missing data file produces a clear, + /// file-level diagnostic (the `FilesystemDataProvider` "cannot resolve + /// data file" error surfaced through `open_vensim_model`), NOT a silent + /// zero or a generic message. The model file is written into a real + /// (existing) temp directory so the base-directory canonicalize + /// succeeds and the failure is specifically the missing CSV. + #[test] + fn missing_data_file_yields_clear_diagnostic() { + use std::io::Write; + + let dir = tempfile::tempdir().unwrap(); + let mdl_path = dir.path().join("missing.mdl"); + let mut f = std::fs::File::create(&mdl_path).unwrap(); + write!( + f, + "{{UTF-8}}\n\ + a = GET DIRECT CONSTANTS('does_not_exist.csv', ',', 'B2') ~~|\n\ + INITIAL TIME = 0 ~~|\n\ + FINAL TIME = 1 ~~|\n\ + TIME STEP = 1 ~~|\n\ + SAVEPER = TIME STEP ~~|\n" + ) + .unwrap(); + let contents = std::fs::read_to_string(&mdl_path).unwrap(); + + let err = open_vensim_model(Some(&mdl_path), &contents) + .expect_err("opening a model with a missing data file must error, not silently zero"); + let msg = format!("{err}"); + assert!( + msg.contains("does_not_exist.csv"), + "diagnostic must name the offending file; got: {msg}" + ); + assert!( + msg.contains("cannot resolve data file"), + "diagnostic must be the file-level 'cannot resolve data file' error, \ + not a generic message; got: {msg}" + ); + } + + /// Reading a `GET DIRECT` model from stdin (`path == None`) must NOT build + /// a `FilesystemDataProvider`: there is no model path to anchor a data + /// root, whether stdin is a pipe or a `< file` redirection. The null + /// provider surfaces the engine's "no DataProvider configured" error + /// rather than silently resolving companions against some unrelated + /// directory (the `/dev`-rooted provider an `is_file()` check on the + /// `/dev/stdin` sentinel would have wrongly built under `< file`). + #[test] + fn stdin_get_direct_yields_null_provider_error() { + // Reuse the real `GET DIRECT CONSTANTS` fixture's contents, but feed + // it through the stdin branch (no path argument). + let path = Path::new("../../test/sdeverywhere/models/directconst/directconst.mdl"); + let contents = std::fs::read_to_string(path) + .unwrap_or_else(|e| panic!("failed to read fixture {}: {e}", path.display())); + + let err = open_vensim_model(None, &contents).expect_err( + "stdin (no path) must not build a data provider, so a GET DIRECT \ + reference must surface the null-provider error", + ); + let msg = format!("{err}"); + assert!( + msg.contains("no DataProvider configured"), + "stdin must use the null provider (no FilesystemDataProvider built); \ + expected the 'no DataProvider configured' error, got: {msg}" + ); + } +} diff --git a/src/simlin-engine/CLAUDE.md b/src/simlin-engine/CLAUDE.md index a10dfb62e..e7671c88e 100644 --- a/src/simlin-engine/CLAUDE.md +++ b/src/simlin-engine/CLAUDE.md @@ -2,7 +2,7 @@ Core simulation engine for system dynamics models. Compiles, type-checks, unit-checks, and simulates SD models. See the root `CLAUDE.md` for full development guidelines; this file maps where functionality lives. -**Last updated: 2026-05-19 (Element-level cycle resolution + genuine-Vensim VECTOR ELM MAP/SORT ORDER + `:NA:` sentinel, the work that made C-LEARN compile via the incremental path, run to FINAL TIME, and match genuine Vensim (`Ref.vdf`) within the 1% cross-simulator tolerance on the matched floor. The whole-variable `model_dependency_graph` cycle gate now refines a recurrence SCC to an element-acyclic verdict over a cross-member-comparable symbolic `SymVarRef` element graph (`db_dep_graph.rs`: `resolve_recurrence_sccs`/`refine_scc_to_element_verdict`/`symbolic_phase_element_order`, GH #575); a resolved SCC's per-element symbolic segments are interleaved into one combined fragment along the SCC's `element_order` and injected at `assemble_module` (`db.rs`: `combine_scc_fragment`/`var_phase_symbolic_fragment_prod`). Per-variable lowering moved to a new sibling module `db_var_fragment.rs` (`lower_var_fragment`). `crate::float::NA` is the finite Vensim `:NA:` sentinel (`-2^109`, NOT IEEE NaN); both `:NA:` paths route to it. New top-level VM-adjacent modules `vm_vector_sort_order.rs` (arrayed VECTOR SORT ORDER, per-iterated-slice 0-based ranks, #585) and `vm_vector_elm_map.rs` (base+full-source, OOB→NaN, no modulo). New `Opcode::LookupArray` (per-element arrayed-GF apply → array view, #580); `src/compiler/symbolic.rs` gains cross-fragment GF de-duplication (`GfDedup`, #582) and `TempStrategy { Recycle, Sum }`. Per-element graphical-function tables now lay out by element-name → declared dimension index (`variable.rs::reorder_arrayed_element_tables`, `db.rs::extract_tables_from_source_var`), not `Equation::Arrayed` Vec position. New tests: `db_dep_graph_tests.rs`, `db_combined_fragment_tests.rs`, `per_element_gf_tests.rs`; `tests/simulate.rs` `simulates_clearn` is un-stubbed (`#[ignore]` for runtime only) with a hardened `ensure_vdf_results` comparator + `EXPECTED_VDF_RESIDUAL` carve-out.) Earlier: Vensim macro support, Phases 1-7 complete. `:MACRO:`/`` definitions import as macro-marked `datamodel::Model`s (`Model.macro_spec: Option`, persisted through protobuf/JSON/schema); single-output macros inline through `BuiltinVisitor`, multi-output (`:`-list) ones materialize at import. New top-level modules: `module_functions.rs` (the unified `ModuleFunctionDescriptor`/`MacroRegistry` resolver+validator for stdlib functions *and* macros, the shared `is_renamed_*` collision predicates) and `db_macro_registry.rs` (the `project_macro_registry` salsa query + sync-time `macro_registry_build_error`); `SourceProject` gains `macro_registry_build_error`, `SourceModel` gains `macro_spec`; `ErrorCode::DuplicateMacroName`; new `tests/metasd_macros.rs` (gated on `file_io`). LTM arrays hardening, Phases 1-8 complete. Phase 7: #502 per-element graphical-function static link polarity -- when an arrayed source feeds an arrayed graphical-function target, `lookup_table_polarity` folds the per-element `tables` list on `Variable::Var` into one link polarity, falling back to `Unknown` for the multi-dim case; #492 the GF strict-monotonicity check uses a y-range-relative epsilon (`max(EPSILON, range_rel * (y_max - y_min))`) so numeric-import noise no longer flips a monotone lookup table to `Unknown`. Phase 6: #483 analytic STDDEV ceteris-paribus partial -- `generate_nonlinear_partial` builds the unrolled population-variance `sqrt` formula for STDDEV (divisor `N`, matching `vm.rs::Opcode::ArrayStddev`) instead of the delta-ratio stand-in; RANK keeps the delta-ratio (an order statistic, unreachable via real models since it returns an array) with a documented justification, pinned by `test_generate_rank_keeps_delta_ratio`. Phase 5: #515 budgeted cross-element-through-aggregate loop recovery -- `recover_cross_agg_loops` drops the old `MAX_AGG_PETALS = 8` hard drop for a deterministic petal priority + a threaded `agg_loop_budget` loop-count budget (`MAX_CROSS_AGG_LOOPS = 256`, `#[cfg(test)]`-overridable via `AggLoopBudgetGuard`; `MAX_AGG_PETALS` survives as a soft per-agg petal cap), surfaces truncation on `LtmVariablesResult.agg_recovery_truncated` + a `Warning`, and enumerates each disjoint petal subset's distinct *cyclic orderings* (`cyclic_orderings(m)` -- (m-1)!/2 for m≥3, mirror reversals skipped, via Heap's algorithm) instead of one ordering per subset. Phase 4 (2026-05-12): #514 sliced-reducer hoisting -- `AggNode.read_slice`, read-slice-driven element graph / link scores, dynamic-index carve-out reclassified as `DynamicIndex`; arrayed synthetic aggs route through agg-half link-score emitters with subscripted agg names and a subscripted Δsource denominator in the diagonal case (strict-prefix broadcast over-subscribes -- GH #528); mapped-dimension sliced reducers stay conservative; a scalar feeder of a hoisted reducer emits a bare element-graph node. Phases 1-3: the `model_ltm_reference_sites` classification IR (`db_ltm_ir.rs`), the consolidated `reducer_kind`/`ReducerKind` table in `ltm_agg.rs`, element-level A2A `Loop::stocks` + per-slot `loop_partitions` (#487), iterated-dimension subscripts ⇒ `Bare` (#511), disjoint-dim arrayed→arrayed per-source-element link scores + the unscoreable-edge `Warning` (#510).)** +**Last updated: 2026-05-20 (C-LEARN residual closure (#590/#591), as five general Vensim import/simulation primitives, not model-specific patches: (1) a standalone lookup-only variable -- a graphical-function holder with no functional input -- lowers uniformly to `gf(Time)` across scalar/A2A/arrayed shapes (`src/compiler/mod.rs`: `var_is_lookup_only`/`is_lookup_only`/`lookup_only_index_expr` + `LookupOnlyLayout { PerElement, Shared }`); (2) a genuine passthrough macro `:MACRO: INIT(x) = INITIAL(x)` collapses to the builtin opcode at the call site (`module_functions.rs`: `classify_passthrough` + a `passthrough: Option` field on `ModuleFunctionDescriptor`, classified at `MacroRegistry::build`), instead of expanding a buggy per-element synthetic module; (3) the import-time XMILE formatter no longer linearizes a shadowed `RAMP FROM TO` macro -- `xmile_compat.rs::format_call_ctx` carries a macro-shadowing audit and the `ramp from to` restructuring arm was removed so the call survives as `RAMP_FROM_TO(...)` and resolves through the macro path; (4) the simulation **initials runlist is now deterministic** (`db_dep_graph.rs` sorts the init set before `topo_sort_str`, GH #595) and the dt stock-submodel-output chain-break applies to ALL readers (parity with the legacy `model.rs::module_output_deps` gate); (5) the VDF reader re-binds a **standalone** graphical-function descriptor to its forward-link output OT (`record_results.rs::standalone_descriptor_rebinds`). New `#[cfg(test)] lookup_only_tests.rs`; the C-LEARN `EXPECTED_VDF_RESIDUAL` carve-out (`tests/simulate.rs`) is now an exact, taxonomy-attributed remainder pinned by `clearn_residual_exactness`. simlin-cli now resolves `GET DIRECT *` external data via a `FilesystemDataProvider` -- see its CLAUDE.md.) Earlier: Element-level cycle resolution + genuine-Vensim VECTOR ELM MAP/SORT ORDER + `:NA:` sentinel, the work that made C-LEARN compile via the incremental path, run to FINAL TIME, and match genuine Vensim (`Ref.vdf`) within the 1% cross-simulator tolerance on the matched floor. The whole-variable `model_dependency_graph` cycle gate now refines a recurrence SCC to an element-acyclic verdict over a cross-member-comparable symbolic `SymVarRef` element graph (`db_dep_graph.rs`: `resolve_recurrence_sccs`/`refine_scc_to_element_verdict`/`symbolic_phase_element_order`, GH #575); a resolved SCC's per-element symbolic segments are interleaved into one combined fragment along the SCC's `element_order` and injected at `assemble_module` (`db.rs`: `combine_scc_fragment`/`var_phase_symbolic_fragment_prod`). Per-variable lowering moved to a new sibling module `db_var_fragment.rs` (`lower_var_fragment`). `crate::float::NA` is the finite Vensim `:NA:` sentinel (`-2^109`, NOT IEEE NaN); both `:NA:` paths route to it. New top-level VM-adjacent modules `vm_vector_sort_order.rs` (arrayed VECTOR SORT ORDER, per-iterated-slice 0-based ranks, #585) and `vm_vector_elm_map.rs` (base+full-source, OOB→NaN, no modulo). New `Opcode::LookupArray` (per-element arrayed-GF apply → array view, #580); `src/compiler/symbolic.rs` gains cross-fragment GF de-duplication (`GfDedup`, #582) and `TempStrategy { Recycle, Sum }`. Per-element graphical-function tables now lay out by element-name → declared dimension index (`variable.rs::reorder_arrayed_element_tables`, `db.rs::extract_tables_from_source_var`), not `Equation::Arrayed` Vec position. New tests: `db_dep_graph_tests.rs`, `db_combined_fragment_tests.rs`, `per_element_gf_tests.rs`; `tests/simulate.rs` `simulates_clearn` is un-stubbed (`#[ignore]` for runtime only) with a hardened `ensure_vdf_results` comparator + `EXPECTED_VDF_RESIDUAL` carve-out.) Earlier: Vensim macro support, Phases 1-7 complete. `:MACRO:`/`` definitions import as macro-marked `datamodel::Model`s (`Model.macro_spec: Option`, persisted through protobuf/JSON/schema); single-output macros inline through `BuiltinVisitor`, multi-output (`:`-list) ones materialize at import. New top-level modules: `module_functions.rs` (the unified `ModuleFunctionDescriptor`/`MacroRegistry` resolver+validator for stdlib functions *and* macros, the shared `is_renamed_*` collision predicates) and `db_macro_registry.rs` (the `project_macro_registry` salsa query + sync-time `macro_registry_build_error`); `SourceProject` gains `macro_registry_build_error`, `SourceModel` gains `macro_spec`; `ErrorCode::DuplicateMacroName`; new `tests/metasd_macros.rs` (gated on `file_io`). LTM arrays hardening, Phases 1-8 complete. Phase 7: #502 per-element graphical-function static link polarity -- when an arrayed source feeds an arrayed graphical-function target, `lookup_table_polarity` folds the per-element `tables` list on `Variable::Var` into one link polarity, falling back to `Unknown` for the multi-dim case; #492 the GF strict-monotonicity check uses a y-range-relative epsilon (`max(EPSILON, range_rel * (y_max - y_min))`) so numeric-import noise no longer flips a monotone lookup table to `Unknown`. Phase 6: #483 analytic STDDEV ceteris-paribus partial -- `generate_nonlinear_partial` builds the unrolled population-variance `sqrt` formula for STDDEV (divisor `N`, matching `vm.rs::Opcode::ArrayStddev`) instead of the delta-ratio stand-in; RANK keeps the delta-ratio (an order statistic, unreachable via real models since it returns an array) with a documented justification, pinned by `test_generate_rank_keeps_delta_ratio`. Phase 5: #515 budgeted cross-element-through-aggregate loop recovery -- `recover_cross_agg_loops` drops the old `MAX_AGG_PETALS = 8` hard drop for a deterministic petal priority + a threaded `agg_loop_budget` loop-count budget (`MAX_CROSS_AGG_LOOPS = 256`, `#[cfg(test)]`-overridable via `AggLoopBudgetGuard`; `MAX_AGG_PETALS` survives as a soft per-agg petal cap), surfaces truncation on `LtmVariablesResult.agg_recovery_truncated` + a `Warning`, and enumerates each disjoint petal subset's distinct *cyclic orderings* (`cyclic_orderings(m)` -- (m-1)!/2 for m≥3, mirror reversals skipped, via Heap's algorithm) instead of one ordering per subset. Phase 4 (2026-05-12): #514 sliced-reducer hoisting -- `AggNode.read_slice`, read-slice-driven element graph / link scores, dynamic-index carve-out reclassified as `DynamicIndex`; arrayed synthetic aggs route through agg-half link-score emitters with subscripted agg names and a subscripted Δsource denominator in the diagonal case (strict-prefix broadcast over-subscribes -- GH #528); mapped-dimension sliced reducers stay conservative; a scalar feeder of a hoisted reducer emits a bare element-graph node. Phases 1-3: the `model_ltm_reference_sites` classification IR (`db_ltm_ir.rs`), the consolidated `reducer_kind`/`ReducerKind` table in `ltm_agg.rs`, element-level A2A `Loop::stocks` + per-slot `loop_partitions` (#487), iterated-dimension subscripts ⇒ `Bare` (#511), disjoint-dim arrayed→arrayed per-source-element link scores + the unscoreable-edge `Warning` (#510).)** **Maintenance note**: Keep this file up to date when adding, removing, or reorganizing modules. @@ -13,9 +13,9 @@ Equation text flows through these stages in order: 1. **`src/lexer/`** - Tokenizer for equation syntax 2. **`src/parser/`** - Recursive descent parser producing `Expr0` AST 3. **`src/ast/`** - AST type system with progressive lowering: `Expr0` (parsed) -> `Expr1` (modules expanded) -> `Expr2` (dimensions resolved) -> `Expr3` (subscripts expanded). `array_view.rs` tracks array dimensions and sparsity. `Expr2Context` trait includes `has_mapping_to()` for cross-dimension mapping lookups during `find_matching_dimension`. -4. **`src/builtins.rs`** - Builtin function definitions (e.g. `MIN`, `PULSE`, `LOOKUP`, `QUANTUM`, `SSHAPE`, `VECTOR SELECT`, `VECTOR ELM MAP`, `VECTOR SORT ORDER`, `VECTOR RANK`, `ALLOCATE AVAILABLE`, `ALLOCATE BY PRIORITY`, `NPV`, `MODULO`, `PREVIOUS`, `INIT`). `is_stdlib_module_function()` is the authoritative predicate for deciding whether a function name expands to a stdlib module; the **module-function** generalization that also resolves project macros lives in **`src/module_functions.rs`** -- the pure (Functional-Core) `ModuleFunctionDescriptor`/`MacroRegistry` resolver+validator unifying stdlib functions and macros (`stdlib_descriptor`, `MacroRegistry::build`/`resolve_macro`, the shared `is_renamed_opcode_intrinsic`/`is_renamed_stdlib_module_builtin`/`is_renamed_builtin_macro_collision` precedence predicates). `equation_is_module_call()` (pre-scan, renamed from `equation_is_stdlib_call`) and `contains_module_call()` (walk-time, renamed from `contains_stdlib_call`) both consult the `MacroRegistry`. `builtins_visitor.rs` handles implicit module instantiation, single-output macro inlining (an arrayed macro invocation enters the per-element path), and PREVIOUS/INIT helper rewriting: unary `PREVIOUS(x)` desugars to `PREVIOUS(x, 0)`, direct scalar args compile to `LoadPrev`, and module-backed or expression args are first rewritten through synthesized scalar helper auxes. `INIT(x)` compiles to `LoadInitial`, using the same helper rewrite when needed. Tracks `module_idents` so `PREVIOUS(module_var)` never reads a multi-slot module directly. +4. **`src/builtins.rs`** - Builtin function definitions (e.g. `MIN`, `PULSE`, `LOOKUP`, `QUANTUM`, `SSHAPE`, `VECTOR SELECT`, `VECTOR ELM MAP`, `VECTOR SORT ORDER`, `VECTOR RANK`, `ALLOCATE AVAILABLE`, `ALLOCATE BY PRIORITY`, `NPV`, `MODULO`, `PREVIOUS`, `INIT`). `is_stdlib_module_function()` is the authoritative predicate for deciding whether a function name expands to a stdlib module; the **module-function** generalization that also resolves project macros lives in **`src/module_functions.rs`** -- the pure (Functional-Core) `ModuleFunctionDescriptor`/`MacroRegistry` resolver+validator unifying stdlib functions and macros (`stdlib_descriptor`, `MacroRegistry::build`/`resolve_macro`, the shared `is_renamed_opcode_intrinsic`/`is_renamed_stdlib_module_builtin`/`is_renamed_builtin_macro_collision` precedence predicates). A **genuine passthrough macro** -- a single-parameter, single-output macro whose body is exactly `out = BUILTIN(param)` self-calling its own renamed-builtin-collision name (`:MACRO: INIT(x) = INITIAL(x)`, stored after the importer's `INITIAL`->`INIT` rename as `init = init(x)`) -- is classified once at `MacroRegistry::build` time (`classify_passthrough`, the only place the body AST is available) into a `passthrough: Option` on the descriptor; `builtins_visitor.rs` reads it to *collapse the call to the builtin opcode* (`init`->`LoadInitial`) instead of expanding the buggy per-element synthetic module (#591), falling through to the same renamed-builtin intrinsic routing the #554 self-call exception takes. Strict criteria (no additional outputs, bare-parameter argument, self-call, renamed-builtin-collision name) keep it from misfiring on a near-miss like `INIT = INIT(x) + 1`. `equation_is_module_call()` (pre-scan, renamed from `equation_is_stdlib_call`) and `contains_module_call()` (walk-time, renamed from `contains_stdlib_call`) both consult the `MacroRegistry` (a passthrough caller is still classified module-backed here -- benign, since it collapses to a flat-slot variable). `builtins_visitor.rs` handles implicit module instantiation, single-output macro inlining (an arrayed macro invocation enters the per-element path), and PREVIOUS/INIT helper rewriting: unary `PREVIOUS(x)` desugars to `PREVIOUS(x, 0)`, direct scalar args compile to `LoadPrev`, and module-backed or expression args are first rewritten through synthesized scalar helper auxes. `INIT(x)` compiles to `LoadInitial`, using the same helper rewrite when needed. Tracks `module_idents` so `PREVIOUS(module_var)` never reads a multi-slot module directly. 5. **`src/compiler/`** - Multi-pass compilation to bytecode: - - `mod.rs` - Orchestration; includes A2A hoisting logic that detects array-producing builtins (VectorElmMap, VectorSortOrder, Rank, AllocateAvailable, AllocateByPriority) during array expansion, hoists them into `AssignTemp` pre-computations, and emits per-element `TempArrayElement` reads + - `mod.rs` - Orchestration; includes A2A hoisting logic that detects array-producing builtins (VectorElmMap, VectorSortOrder, Rank, AllocateAvailable, AllocateByPriority) during array expansion, hoists them into `AssignTemp` pre-computations, and emits per-element `TempArrayElement` reads. Also lowers a **standalone lookup-only variable** -- a graphical-function holder whose equation is empty (XMILE) or the MDL lookup sentinel (`var_is_lookup_only`/`is_lookup_only`, mirroring `mdl::writer::is_lookup_only_equation`'s "empty or sentinel" rule) -- uniformly to `gf(Time)` across the scalar, A2A, and arrayed shapes (`lookup_only_index_expr`; the index is the current simulation time, #590). WITH LOOKUP (`var = WITH LOOKUP(input, table)`: tables present *and* a real input) keeps its `LOOKUP(self, input)` lowering and is NOT lookup-only. An arrayed lookup-only variable's `LookupOnlyLayout` (`PerElement` when `tables.len() == n` -- each element wraps `off + i`; `Shared` when `tables.len() == 1` -- every element wraps the base `off`) is derived from the table count `variable.rs::build_tables` produced, so the LOOKUP offset matches the layout and `element_offset` stays in range - `context.rs` - Symbol tables and variable metadata; `lower_preserving_dimensions()` skips Pass 1 dimension resolution to keep full array views for array-producing builtins. Handles `@N` position syntax resolution: in scalar context (no active A2A dimension, not inside an array-reducing builtin), `DimPosition(@N)` resolves to a concrete element offset; inside array-reducing builtins (`preserve_wildcards_for_iteration`), dimension views are preserved for iteration. Two wildcard-preservation contexts: `with_preserved_wildcards()` for reducers (SUM, MEAN, etc.) where `ActiveDimRef` resolves to a concrete offset, and `with_vector_builtin_wildcards()` for array-producing builtins (VectorSortOrder, VectorElmMap, etc.) where `ActiveDimRef` is promoted to `Wildcard` to preserve the full array view - `expr.rs` - Expression compilation - `codegen.rs` - Bytecode emission; routes array-producing builtins through dedicated opcodes instead of element-wise iteration. `emit_array_reduce()` is the shared helper for single-argument array builtins (SUM, SIZE, STDDEV, MIN, MAX, MEAN): pushes view, emits reduction opcode, pops view @@ -50,7 +50,7 @@ The primary compilation path uses salsa tracked functions for fine-grained incre - **`src/db_ltm_ir.rs`** (a top-level module, sibling of `db` -- not a `db.rs` submodule, 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 `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` carries the GH #511 iterated-dimension fix -- a subscript whose indices are *exactly* the target equation's iterated dimensions, in the position matching the source's declared dimension order (each index `d_i` either named the same as the source's `i`-th dim or a dimension that maps to it -- the AC3.5 mapped-dimension case), classifies as `Bare` (a same-element-on-shared-dims reference: `row_sum[Region]` inside `growth[Region,Age]` reads the same `Region` element of `row_sum`, which `emit_edges_for_reference` then projects via `expand_same_element`). 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`, and a not-hoistable reducer's argument -- a *dynamic index* (`SUM(pop[idx,*])`, `idx` non-literal) or a *mapped*-dimension sliced reducer (`SUM(matrix[State,*])` over `matrix[Region,D2]` with a `State→Region` mapping; `enumerate_agg_nodes` declines the remapped axis -- tracked tech debt). For the dynamic-index case `model_ltm_reference_sites` reclassifies that `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. (`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 `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 top-level module, sibling of `db` -- 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`, so `macro_registry_build_error` runs `MacroRegistry::build` over the datamodel `Vec` at sync time and stores its typed `(ErrorCode, message)` on the `SourceProject` input; the query reads it back and surfaces it 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_dep_graph.rs`** (a top-level module, sibling of `db` -- 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). The thin `#[salsa::tracked]` wrappers `crate::db::model_dependency_graph` / `model_dependency_graph_with_inputs` stay in `db.rs` (the `ModelDepGraphResult` salsa types do) and delegate straight here. Also holds the single shared **dt-phase cycle relation** `dt_walk_successors(var_info, name) -> Vec<&str>` (Stock/Module/absent ⇒ `[]`; otherwise `dt_deps` filtered to known, non-stock targets, module-targets kept -- exactly the successor set `compute_inner` iterates for dt-phase cycle detection), the **init-phase** analogue `init_walk_successors` (a stock is NOT an init sink), 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 each 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 `dt_walk_successors` 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.rs`** (a top-level module, sibling of `db` -- 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 thin `#[salsa::tracked]` wrappers `crate::db::model_dependency_graph` / `model_dependency_graph_with_inputs` stay in `db.rs` (the `ModelDepGraphResult` salsa types do) and delegate straight here. Also holds the single shared **dt-phase cycle relation** `dt_walk_successors(var_info, name) -> Vec<&str>` (Stock/Module/absent ⇒ `[]`; otherwise `dt_deps` filtered to known, non-stock targets, module-targets kept -- exactly the successor set `compute_inner` iterates for dt-phase cycle detection), the **init-phase** analogue `init_walk_successors` (a stock is NOT an init sink), 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 each 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 `dt_walk_successors` 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`/`db_tests.rs` under the per-file line cap: the `dt_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_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 top-level module, sibling of `db` -- 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. - **`src/db_combined_fragment_tests.rs`** - Structural tests for `combine_scc_fragment` (segment ordering along `element_order`, write-`SymVarRef` identity preservation, single trailing `Ret`, per-member resource renumbering with no cross-member collision, merged side-channels). Numeric correctness is the end-to-end job of the `ref.mdl`/`interleaved.mdl` simulation tests. In its own file to keep `db.rs`/`db_tests.rs` under the per-file line cap. @@ -75,9 +75,9 @@ The primary compilation path uses salsa tracked functions for fine-grained incre - convert/ subdir - Multi-pass AST to datamodel conversion (includes `external_data.rs` for GET DIRECT resolution via `DataProvider`, `macros.rs` for turning each `:MACRO:` block into a macro-marked `Model` via `Model::new_macro`, and `multi_output.rs` for materializing `:`-list multi-output macro invocations at import). The pipeline is reusable for macro sub-models. - view/ subdir - Sketch/diagram parsing (`elements.rs`, `processing.rs`, `types.rs`, `mod.rs`) and datamodel conversion (`convert.rs`). Captures font specs and element dimensions/bits during parsing for roundtrip fidelity. - `writer.rs` - MDL output: variable equations (with original casing, native LOOKUP syntax, backslash continuations), `:MACRO:`/`:END OF MACRO:` blocks for macro-marked models (reconstructing multi-output invocations), sketch sections (view splitting on Group boundaries, element ordering) - - `xmile_compat.rs` - Expression formatting for XMILE output + - `xmile_compat.rs` - Expression formatting for XMILE output. Carries a documented **macro-shadowing audit** (`format_call_ctx`): arms that *restructure* a builtin-named call into a different expression at import time (`MODULO(x,y)`->`(x) MOD (y)`, `SAMPLE IF TRUE`, etc.) run BEFORE compile-time macro resolution, so they silently pre-empt a same-named `:MACRO:` -- the opposite of Vensim semantics, where a project macro shadows the like-named builtin. The `ramp from to` arm (the one C-LEARN tripped) was removed so `RAMP FROM TO(...)` survives import as `RAMP_FROM_TO(...)` and resolves through the macro path; the other restructuring arms carry the same latent hazard but are not currently shadowed by any in-repo model. Name-preserving renames (e.g. `SSHAPE`, a genuine builtin not restructured here) are safe and shadow cleanly - `settings.rs` - Integration settings parser -- **`src/vdf.rs`** - Vensim VDF (binary data file) parser. Parses all structural elements (sections, records, name/slot/offset tables, data blocks) and extracts results via `to_results_via_records()`: each section-1 record's `f[2]` is a direct key into the section-2 name table and its `f[11]` is the variable's OT-block start; graphical-function descriptor records are peeled off so the remaining owner spans form a clean OT partition. Submodules: `record_results.rs` (the record-to-OT mapping pipeline), `section3.rs` (array-shape directory), `section5_dims.rs` (dimension/element recovery), `signatures.rs` (the two `#`-signature stdlib-call encodings). See `docs/design/vdf.md` for the format specification. +- **`src/vdf.rs`** - Vensim VDF (binary data file) parser. Parses all structural elements (sections, records, name/slot/offset tables, data blocks) and extracts results via `to_results_via_records()`: each section-1 record's `f[2]` is a direct key into the section-2 name table and its `f[11]` is the variable's OT-block start; graphical-function descriptor records are peeled off so the remaining owner spans form a clean OT partition. Submodules: `record_results.rs` (the record-to-OT mapping pipeline; besides peeling *overlapping* descriptors, `standalone_descriptor_rebinds` re-binds a **standalone** lookup-only descriptor -- a graphical-function variable Vensim saves *only* as a descriptor, with no separate consumer-owner record, so it would otherwise decode at its spurious `f[11]`-as-OT-start stock ghost slot -- to its forward-link evaluated-output OT `lookup_record[f[11]].word[10]`, gated on the ghost slot carrying the stock class code and the forward link being a valid owner data OT; scalar only, arrayed standalone lookups are deferred), `section3.rs` (array-shape directory), `section5_dims.rs` (dimension/element recovery), `signatures.rs` (the two `#`-signature stdlib-call encodings). See `docs/design/vdf.md` for the format specification. - **`src/systems/`** - Systems format (`.txt`) parser, translator, and writer. A line-oriented notation for stock-and-flow models with implicit flow typing (Rate, Conversion, Leak). Pipeline: `lexer.rs` -> `parser.rs` -> `ast.rs` (IR) -> `translate.rs` (to `datamodel::Project`). Each systems flow becomes a stdlib module instance (`systems_rate`, `systems_leak`, or `systems_conversion`), with chained `available`/`remaining` wiring for multi-outflow stocks. `writer.rs` reconstructs `.txt` format from a translated `datamodel::Project` by inspecting module structure. Public API: `systems::parse()`, `systems::translate::translate()`, `systems::project_to_systems()` - **`src/json.rs`** - JSON serialization matching Go `sd` package schema - **`src/json_sdai.rs`** - JSON schema for AI metadata augmentation. `generate_relationships()` derives `Relationship` entries from pre-computed equation dependency polarities (via `compute_link_polarities`), filtering out stock-flow structural edges and sorting deterministically @@ -128,6 +128,7 @@ The primary compilation path uses salsa tracked functions for fine-grained incre - **`src/test_common.rs`**, **`src/testutils.rs`** - Helpers and fixtures (e.g. `x_model`, `x_stock`, `x_flow`). `TestProject` is the primary builder for test models: chainable methods like `.aux()`, `.stock()`, `.flow()`, `.array_aux()`, `.array_stock()`, `.array_flow()`, `.named_dimension()`, and `.build_datamodel()` / `.build_sim_with_ltm()`. `assert_compiles_incremental()` verifies the model compiles without errors via the salsa path. - **`src/array_tests.rs`** - Array-specific tests (feature-gated) - **`src/per_element_gf_tests.rs`** - Per-element graphical-function (arrayed GF) layout invariant: a per-element GF table must land at the element's *declared dimension index* (row-major), not its `Equation::Arrayed` `elems` Vec position. Uses deliberately NON-alphabetical declared orders (where position != dim-index) so a positional mis-map is observable; covers the scalar `Lookup` path, the `LookupArray` (VECTOR SELECT) path, the MDL-importer-sort twin, sparse + multi-dim, the salsa dependency-table path (`extract_tables_from_source_var`), and a compile-time/perf structural guard that the reorder is materialized at compile time and the VM opcode carries no element name. The LTM-polarity twin lives in the `ltm` tests module (`test_per_element_gf_link_polarity_fixed_index_non_sorted_order`). +- **`src/lookup_only_tests.rs`** - End-to-end tests for the standalone-lookup-only `gf(Time)` lowering (#590): scalar, A2A, and arrayed (both `LookupOnlyLayout::Shared` and `PerElement`) holders evaluate their table at the current simulation time, while WITH LOOKUP keeps feeding its real input. The pure `is_lookup_only`/`var_is_lookup_only` predicate units live inline in `src/compiler/mod.rs`. - **`src/json_proptest.rs`**, **`src/json_sdai_proptest.rs`** - Property-based tests - **`src/unit_checking_test.rs`** - Unit checking regression tests - **`src/test_sir_xmile.rs`** - SIR epidemiology model integration tests diff --git a/src/simlin-engine/src/builtins_visitor.rs b/src/simlin-engine/src/builtins_visitor.rs index 754f63363..54f54baa9 100644 --- a/src/simlin-engine/src/builtins_visitor.rs +++ b/src/simlin-engine/src/builtins_visitor.rs @@ -630,8 +630,40 @@ impl<'a> BuiltinVisitor<'a> { // `RAMP FROM TO` therefore expands as the macro even though // it parsed as `CallKind::Builtin`. `func` is the raw call // name (resolve_macro canonicalizes internally). - if !is_renamed_builtin_self_call - && let Some(descriptor) = self.macro_registry.resolve_macro(&func) + // + // The #554 self-call exception (`is_renamed_builtin_self_call`) + // suppresses resolution for a renamed-builtin call inside the + // like-named macro's own body, so it routes to the intrinsic + // rather than recursing into the macro. + let descriptor = if is_renamed_builtin_self_call { + None + } else { + self.macro_registry.resolve_macro(&func) + }; + + // #591-c1: a *genuine passthrough* macro + // (`:MACRO: INIT(x) = INITIAL(x)`, stored after the importer's + // INITIAL -> INIT rename as `init = init(x)`) is NOT expanded + // into a per-element synthetic module (which mis-orders / + // mis-propagates its value). Only a NON-passthrough resolved + // descriptor expands here; a passthrough descriptor leaves + // `func`/`args` untouched and falls through to the + // renamed-builtin intrinsic routing below -- exactly as the + // #554 self-call exception does inside a macro body, here + // generalized from the macro body to the call site. + // + // The fall-through is sound because of the self-call invariant + // the classifier guarantees: `passthrough.is_some()` implies + // `canonicalize(call) == canonicalize(macro_name)` AND + // `is_renamed_builtin_macro_collision(canonicalize(call))` + // (`classify_passthrough`). So `func` here canonicalizes to the + // opcode-backed builtin (e.g. `init`) and routes to the right + // intrinsic below -- `init` -> `LoadInitial`, with the existing + // `make_temp_arg` hoisting for an expression argument + // (`init_needs_temp_arg`). The macro body did no work beyond the + // bare call, so collapsing to the opcode loses nothing. + if let Some(descriptor) = descriptor + && descriptor.passthrough.is_none() { let descriptor = descriptor.clone(); return self.expand_module_function(&descriptor, &func, args, loc); diff --git a/src/simlin-engine/src/compiler/mod.rs b/src/simlin-engine/src/compiler/mod.rs index 3920d8aa9..c5c9f25bf 100644 --- a/src/simlin-engine/src/compiler/mod.rs +++ b/src/simlin-engine/src/compiler/mod.rs @@ -720,8 +720,10 @@ impl Var { exprs.push(Expr::AssignCurr(off, Box::new(main_expr))); exprs } + // Stocks never carry graphical functions, so they + // are never lookup-only (`None`). Ast::ApplyToAll(dims, ast) => { - expand_a2a_with_hoisting(ctx, dims, ast, off)? + expand_a2a_with_hoisting(ctx, dims, ast, off, false)? } Ast::Arrayed( dims, @@ -735,6 +737,7 @@ impl Var { default_ast.as_ref(), *apply_default_for_missing, off, + None, )?, } } else { @@ -767,7 +770,7 @@ impl Var { } } } - Variable::Var { tables, .. } => { + Variable::Var { tables, eqn, .. } => { let off = ctx.get_base_offset(&Ident::new(var.ident()))?; let ast = if ctx.is_initial { var.init_ast() @@ -777,6 +780,12 @@ impl Var { if ast.is_none() { return sim_err!(EmptyEquation, var.ident().to_string()); } + // A standalone lookup-only variable (a graphical-function + // holder with no functional input) is lowered uniformly to + // `gf(Time)` across all three shapes below. WITH LOOKUP + // (tables + a real input) keeps its `LOOKUP(self, input)` + // lowering; ordinary auxes are untouched. + let lookup_only = var_is_lookup_only(eqn.as_ref(), !tables.is_empty()); match ast.as_ref().unwrap() { Ast::Scalar(ast) => { let mut exprs = ctx.lower(ast)?; @@ -785,10 +794,19 @@ impl Var { hoist_nested_array_builtins_in_scalar(main_expr, &mut exprs); let main_expr = if !tables.is_empty() { let loc = main_expr.get_loc(); + // For lookup-only, evaluate the table at the + // determined index (Time) instead of the + // lowered (constant-0) sentinel equation; WITH + // LOOKUP feeds the user's input expression. + let index = if lookup_only { + lookup_only_index_expr(loc) + } else { + main_expr + }; Expr::App( BuiltinFn::Lookup( Box::new(Expr::Var(off, loc)), - Box::new(main_expr), + Box::new(index), loc, ), loc, @@ -800,9 +818,27 @@ impl Var { exprs } Ast::ApplyToAll(dims, ast) => { - expand_a2a_with_hoisting(ctx, dims, ast, off)? + expand_a2a_with_hoisting(ctx, dims, ast, off, lookup_only)? } Ast::Arrayed(dims, elements, default_ast, apply_default_for_missing) => { + // A lookup-only arrayed variable's table layout is + // decided by `variable.rs::build_tables`: per-element + // tables (`tables.len() == n`) ONLY when at least one + // element carries its own gf, otherwise a single + // shared variable-level table (`tables.len() == 1`, + // the same shape A2A always has). The expansion needs + // that distinction to wrap the right LOOKUP offset + // (element-own `off + i` vs shared base `off`), so + // derive it from the actual table count here. + let lookup_only_layout = lookup_only.then(|| { + let element_count: usize = + dims.iter().map(Dimension::len).product(); + if tables.len() == element_count { + LookupOnlyLayout::PerElement + } else { + LookupOnlyLayout::Shared + } + }); expand_arrayed_with_hoisting( ctx, dims, @@ -810,6 +846,7 @@ impl Var { default_ast.as_ref(), *apply_default_for_missing, off, + lookup_only_layout, )? } } @@ -1083,6 +1120,122 @@ fn contains_array_producing_builtin(expr: &Expr) -> bool { } } +/// Pure predicate: a "lookup-only" variable is a standalone graphical-function +/// holder -- it carries a graphical function (`has_tables`) and its equation is +/// empty or the MDL lookup sentinel (i.e. there is no functional input +/// expression to feed the table). +/// +/// Mirrors `mdl::writer::is_lookup_only_equation`'s "empty or sentinel" rule so +/// that BOTH an XMILE-sourced lookup-only variable (empty/absent equation + +/// `gf`) and an MDL-sourced one (`equation == LOOKUP_SENTINEL` + `gf`) are +/// detected -- the `LOOKUP_SENTINEL` string is MDL-format-internal and is NOT a +/// stable datamodel contract, so the empty-equation arm is load-bearing, not +/// redundant. +/// +/// Returns `false` for WITH LOOKUP (`var = WITH LOOKUP(input, table)`: tables +/// present but a *real* input equation -- it must keep its `LOOKUP(self, +/// input)` lowering) and for an ordinary aux (a real equation, no tables). The +/// `has_tables` guard also keeps an empty-RHS aux (no gf) off this path. +fn is_lookup_only(equation: &str, has_tables: bool) -> bool { + has_tables && is_empty_or_sentinel(equation) +} + +/// An equation string with no functional content: empty/whitespace-only (the +/// XMILE lookup-only form) or exactly the MDL lookup sentinel (the MDL form). +/// The trimmed comparison mirrors `mdl::writer::is_lookup_only_equation`. +fn is_empty_or_sentinel(equation: &str) -> bool { + let trimmed = equation.trim(); + trimmed.is_empty() || trimmed == crate::mdl::LOOKUP_SENTINEL +} + +/// Whether a whole variable is lookup-only, given its source `datamodel::Equation` +/// and whether it carries graphical functions (`has_tables`). A scalar/A2A +/// variable is lookup-only when its single equation is empty/sentinel; an +/// arrayed (`Equation::Arrayed`) variable is lookup-only when EVERY element +/// equation (and the EXCEPT default, if any) is empty/sentinel -- i.e. it is a +/// pure per-element table holder. Returns `false` when the variable has no +/// equation or no tables. +fn var_is_lookup_only(eqn: Option<&crate::datamodel::Equation>, has_tables: bool) -> bool { + use crate::datamodel::Equation; + if !has_tables { + return false; + } + match eqn { + // Scalar / A2A: one equation string -- the pure `is_lookup_only` atom. + Some(Equation::Scalar(s)) | Some(Equation::ApplyToAll(_, s)) => { + is_lookup_only(s, has_tables) + } + // Arrayed: a pure per-element table holder iff EVERY element equation + // (and the EXCEPT default, if any) is empty/sentinel. `has_tables` is + // already true here, so the per-element check is just the string + // predicate `is_empty_or_sentinel` that the atom is built on. + Some(Equation::Arrayed(_, elements, default, _)) => { + !elements.is_empty() + && elements.iter().all(|(_, e, _, _)| is_empty_or_sentinel(e)) + && default.as_deref().map(is_empty_or_sentinel).unwrap_or(true) + } + None => false, + } +} + +/// The index expression a standalone lookup-only variable's table is evaluated +/// at. Determined empirically (Phase 1, #590) to be the current simulation time +/// -- `gf(Time)` -- matching genuine Vensim for the year-indexed historical +/// lookup tables that motivate this, and consistent with the importer's +/// `MdlEquation::Implicit` arm (`mdl/convert/variables.rs`), which already +/// lowers an unspecified-input gf to `equation = "TIME"`. Centralized here so +/// the scalar, arrayed, and A2A lowering sites stay in lock-step. +fn lookup_only_index_expr(loc: Loc) -> Expr { + Expr::App(BuiltinFn::Time, loc) +} + +#[cfg(test)] +mod is_lookup_only_tests { + use super::*; + + #[test] + fn is_lookup_only_sentinel_equation_with_tables_is_true() { + // MDL-sourced lookup-only: `g()` imports as equation = + // LOOKUP_SENTINEL ("0+0") + a graphical function. + assert!(is_lookup_only(crate::mdl::LOOKUP_SENTINEL, true)); + } + + #[test] + fn is_lookup_only_empty_equation_with_tables_is_true() { + // XMILE-sourced lookup-only: a variable with a `` but no equation + // carries an empty/absent equation rather than the MDL sentinel. + assert!(is_lookup_only("", true)); + // Whitespace-only is equivalent to empty (matches the `.trim()` in + // `mdl::writer::is_lookup_only_equation`). + assert!(is_lookup_only(" ", true)); + } + + #[test] + fn is_lookup_only_with_lookup_real_input_is_false() { + // WITH LOOKUP (`g = WITH LOOKUP(some_input,
)`): tables present + // but a real input expression. Must NOT be treated as lookup-only -- + // it keeps `LOOKUP(self, input)` so the index is the user's input + // (this is the AC1.4 distinction). + assert!(!is_lookup_only("some_input", true)); + } + + #[test] + fn is_lookup_only_ordinary_aux_no_tables_is_false() { + // An ordinary aux: a real equation and no graphical function. + assert!(!is_lookup_only("3 * x + 1", false)); + } + + #[test] + fn is_lookup_only_sentinel_equation_without_tables_is_false() { + // An empty-RHS aux (MdlEquation::EmptyRhs): the sentinel equation but + // NO graphical function. `has_tables == false` keeps it off the + // lookup-only path even though its equation text matches the sentinel. + assert!(!is_lookup_only(crate::mdl::LOOKUP_SENTINEL, false)); + // The empty-equation, no-tables case is likewise not lookup-only. + assert!(!is_lookup_only("", false)); + } +} + /// Test-only wrapper exposing the production recursive /// array-producing-builtin predicate (`contains_array_producing_builtin`, /// which delegates to the private `is_array_producing_builtin`): true iff @@ -1595,6 +1748,24 @@ fn array_view_from_dims(dims: &[Dimension]) -> ArrayView { ArrayView::contiguous_with_names(dim_sizes, dim_names) } +/// The graphical-function table layout of a lookup-only arrayed variable, which +/// determines the LOOKUP offset its expansion must wrap. The two variants mirror +/// the two layouts `variable.rs::build_tables` can produce for an arrayed +/// variable, so that an invalid "lookup-only but neither layout" state is +/// unrepresentable (the caller derives the variant from `tables.len()`). +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum LookupOnlyLayout { + /// One table per element (`tables.len() == n`), laid out by row-major + /// declared dimension index. Produced ONLY when at least one element carries + /// its OWN gf. Element `i` reads its own table, so wrap `off + i`. + PerElement, + /// A single variable-level table shared by every element (`tables.len() == + /// 1`). Produced when the arrayed variable has empty/sentinel element + /// equations and one variable-level gf -- the same shape A2A always has. Every + /// element reads that one table, so wrap the BASE offset `off`. + Shared, +} + /// Handle the Arrayed expansion, detecting array-producing builtins in /// per-element expressions and hoisting them into AssignTemp pre-computations. /// @@ -1602,6 +1773,11 @@ fn array_view_from_dims(dims: &[Dimension]) -> ArrayView { /// like VectorElmMap, VectorSortOrder, or AllocateAvailable, the builtin must /// be evaluated once for the whole array and stored in temp. Each element then /// reads its result via TempArrayElement. +/// +/// `lookup_only` is `Some(layout)` iff the variable is a standalone +/// graphical-function holder (every element equation empty/sentinel); the layout +/// selects the LOOKUP offset (see [`LookupOnlyLayout`]). It is `None` for an +/// ordinary arrayed variable. fn expand_arrayed_with_hoisting( ctx: &Context, dims: &[Dimension], @@ -1609,9 +1785,53 @@ fn expand_arrayed_with_hoisting( default_ast: Option<&crate::ast::Expr2>, apply_default_for_missing: bool, off: usize, + lookup_only: Option, ) -> Result> { let active_dims = Arc::<[Dimension]>::from(dims.to_vec()); + // A lookup-only arrayed variable is a pure table holder: every element + // evaluates a table at the determined index (Time). WHICH table it reads + // depends on the layout `variable.rs::build_tables` chose, so the LOOKUP + // offset must match it: + // + // * `LookupOnlyLayout::PerElement` (`tables.len() == n`): element `i` reads + // its own table, so wrap `off + i`. `codegen::extract_table_info` derives + // `elem_off = off+i - base_off = i` and `table_count = n`, so `i` is always + // in range. + // * `LookupOnlyLayout::Shared` (`tables.len() == 1`): every element reads the + // one shared variable-level table, so wrap the BASE offset `off`, exactly + // like `expand_a2a_with_hoisting`: `elem_off = off - base_off = 0` and + // `table_count = 1`, so element_offset 0 is in range for all elements. + // Wrapping `off + i` here would make `element_offset == i >= 1 == + // table_count` for i > 0, and the VM's `Lookup` opcode would push NaN. + // + // (A lookup-only equation is a constant sentinel, never an array-producing + // builtin, so this never needs the hoisting path below.) + if let Some(layout) = lookup_only { + let exprs: Vec = SubscriptIterator::new(dims) + .enumerate() + .map(|(i, _)| { + let loc = Loc::default(); + let table_off = match layout { + LookupOnlyLayout::PerElement => off + i, + LookupOnlyLayout::Shared => off, + }; + Expr::AssignCurr( + off + i, + Box::new(Expr::App( + BuiltinFn::Lookup( + Box::new(Expr::Var(table_off, loc)), + Box::new(lookup_only_index_expr(loc)), + loc, + ), + loc, + )), + ) + }) + .collect(); + return Ok(exprs); + } + // Scan ALL subscript combinations to find any equation that needs hoisting. // The first element alone may be a constant override while later elements // use a default (or explicit equation) containing array-producing builtins. @@ -1694,9 +1914,42 @@ fn expand_a2a_with_hoisting( dims: &[Dimension], ast: &crate::ast::Expr2, off: usize, + lookup_only: bool, ) -> Result> { - // Lower once using element 0's subscripts to detect the expression shape. let active_dims = Arc::<[Dimension]>::from(dims.to_vec()); + + // An apply-to-all lookup-only variable has ONE variable-level table + // (`variable.rs::build_tables` falls through to the single-gf branch, so + // `tables.len() == 1`) shared by every element. Every element must read + // that one table, so the LOOKUP wraps the BASE offset `off`, NOT `off + i`: + // `codegen::extract_table_info` derives `elem_off = off - base_off = 0` and + // `table_count == 1`, so element_offset 0 is in range for all elements. + // Wrapping `off + i` here would make `element_offset == i >= 1 == + // table_count` for i > 0, and the VM's `Lookup` opcode would push NaN. This + // is the deliberate inverse of the arrayed per-element case in + // `expand_arrayed_with_hoisting` (per-element tables, wrap `off + i`). + if lookup_only { + let exprs: Vec = SubscriptIterator::new(dims) + .enumerate() + .map(|(i, _)| { + let loc = Loc::default(); + Expr::AssignCurr( + off + i, + Box::new(Expr::App( + BuiltinFn::Lookup( + Box::new(Expr::Var(off, loc)), + Box::new(lookup_only_index_expr(loc)), + loc, + ), + loc, + )), + ) + }) + .collect(); + return Ok(exprs); + } + + // Lower once using element 0's subscripts to detect the expression shape. let first_subscripts: Vec = SubscriptIterator::new(dims).next().unwrap_or_default(); let first_ctx = ctx.with_active_subscripts(active_dims.clone(), &first_subscripts); let mut first_exprs = first_ctx.lower(ast)?; diff --git a/src/simlin-engine/src/db_dep_graph.rs b/src/simlin-engine/src/db_dep_graph.rs index 821db164e..cf857397a 100644 --- a/src/simlin-engine/src/db_dep_graph.rs +++ b/src/simlin-engine/src/db_dep_graph.rs @@ -42,7 +42,7 @@ use crate::common::{Canonical, Ident}; use crate::db::{ CompilationDiagnostic, Db, Diagnostic, DiagnosticError, DiagnosticSeverity, ModelDepGraphResult, ResolvedScc, SccPhase, SourceModel, SourceProject, SourceVariableKind, - model_module_ident_context, variable_direct_dependencies_with_context, + VariableDeps, model_module_ident_context, variable_direct_dependencies_with_context, variable_direct_dependencies_with_context_and_inputs, }; @@ -212,59 +212,115 @@ pub(crate) fn build_var_info( let project_models = project.models(db); - for (name, source_var) in source_vars.iter() { - let deps = if module_input_names.is_empty() { - variable_direct_dependencies_with_context( - db, - *source_var, - project, - module_ident_context, - ) - } else { - variable_direct_dependencies_with_context_and_inputs( - db, - *source_var, - project, - module_ident_context, - module_input_names.clone(), - ) - }; + // Per-variable deps, computed once and reused (salsa-cached). A pre-pass + // both seeds the instance->model map below and is reused in the main loop. + let var_deps: Vec<(&String, &VariableDeps)> = source_vars + .iter() + .map(|(name, source_var)| { + let deps = if module_input_names.is_empty() { + variable_direct_dependencies_with_context( + db, + *source_var, + project, + module_ident_context, + ) + } else { + variable_direct_dependencies_with_context_and_inputs( + db, + *source_var, + project, + module_ident_context, + module_input_names.clone(), + ) + }; + (name, deps) + }) + .collect(); + + // Map each module-INSTANCE name to its model name, so a `instance·subvar` + // dependency can be resolved to the right submodel. A module instance name + // is NOT itself a key in `project_models` (keyed by MODEL name). Both + // declared module variables (`source_vars`) AND synthesized/implicit module + // instances (e.g. a SMOOTH's `$⁚..⁚smth1⁚`, which live only in a + // variable's `implicit_vars`) must be covered, since the + // stock-output-reading consumer references the implicit instance's output. + let mut module_instance_model: HashMap = source_vars + .iter() + .filter(|(_, sv)| sv.kind(db) == SourceVariableKind::Module) + .map(|(n, sv)| (n.clone(), canonicalize(sv.model_name(db)).into_owned())) + .collect(); + for (_, deps) in &var_deps { + for implicit in &deps.implicit_vars { + if implicit.is_module + && let Some(model_name) = &implicit.model_name + { + module_instance_model + .insert(implicit.name.clone(), canonicalize(model_name).into_owned()); + } + } + } + + for (name, deps) in &var_deps { let init_only_dt = deps.dt_init_only_referenced_vars.clone(); let lagged_dt_previous = deps.dt_previous_referenced_vars.clone(); let lagged_initial_previous = deps.initial_previous_referenced_vars.clone(); - let kind = source_var.kind(db); - let mut dt_deps = if kind == SourceVariableKind::Module { - deps.dt_deps - .iter() - .filter(|dep| { - let effective = dep.strip_prefix('\u{00B7}').unwrap_or(dep); - if let Some(dot_pos) = effective.find('\u{00B7}') { - let module_name = &effective[..dot_pos]; - let var_name = &effective[dot_pos + '\u{00B7}'.len_utf8()..]; - let sub_canonical = canonicalize(module_name); - if let Some(sub_model) = project_models.get(sub_canonical.as_ref()) { - let sub_vars = sub_model.variables(db); - if let Some(sub_var) = sub_vars.get(var_name) { - return sub_var.kind(db) != SourceVariableKind::Stock; - } - } - true - } else { - true + let kind = source_vars[name.as_str()].kind(db); + // A `submodel·subvar` dependency whose `subvar` is a Stock is read + // from the PRIOR timestep in the dt phase (a stock breaks the + // dependency chain), so it must NOT impose a same-step ordering edge. + // This mirrors the legacy `model.rs::module_output_deps` gate + // (`if ctx.is_initial || !output_var.is_stock()` -- the dt-phase case + // omits the module dependency for a stock output). It applies to EVERY + // reader, not just module variables: a NON-module variable that reads + // a stock submodel output (e.g. `v = SMOOTH(...)·output`, the SMOOTH + // output being an INTEG stock) must likewise drop the dt edge. + // Otherwise `normalize_deps` collapses `submodel·output` to the bare + // `submodel` module name and the reader gains a spurious `reader -> + // module` dt edge; combined with the module being a sink in the cycle + // relation (`dt_walk_successors`) but carrying its input src as a + // direct dep in `dt_dependencies`, this forms an ordering cycle invisible + // to cycle detection that `topo_sort_str` breaks arbitrarily -- sometimes + // emitting the module BEFORE its input, so the module reads a stale input + // each flows step (C-LEARN's `emissions_with_stopped_growth` drop-to-0, + // #591-c1). The init phase keeps the edge (stocks do not break the chain + // there), so only `dt_deps` is filtered. + let keep_dt_dep = |dep: &str| -> bool { + let effective = dep.strip_prefix('\u{00B7}').unwrap_or(dep); + if let Some(dot_pos) = effective.find('\u{00B7}') { + let module_name = &effective[..dot_pos]; + let var_name = &effective[dot_pos + '\u{00B7}'.len_utf8()..]; + // Resolve `module_name` to a submodel: it is either a module + // INSTANCE (the common case -- a synthesized stdlib/macro + // instance, keyed by its own ident) or, for a nested-module + // reference, already a MODEL name. Try the instance map first, + // then fall back to a direct model-name lookup. + let sub_canonical = canonicalize(module_name); + let sub_model = module_instance_model + .get(module_name) + .and_then(|m| project_models.get(m.as_str())) + .or_else(|| project_models.get(sub_canonical.as_ref())); + if let Some(sub_model) = sub_model { + let sub_vars = sub_model.variables(db); + if let Some(sub_var) = sub_vars.get(var_name) { + return sub_var.kind(db) != SourceVariableKind::Stock; } - }) - .cloned() - .collect() - } else { - deps.dt_deps.clone() + } + } + true }; + let mut dt_deps: BTreeSet = deps + .dt_deps + .iter() + .filter(|d| keep_dt_dep(d)) + .cloned() + .collect(); dt_deps.retain(|dep| !init_only_dt.contains(dep)); dt_deps.retain(|dep| !lagged_dt_previous.contains(dep)); let mut initial_deps = deps.initial_deps.clone(); initial_deps.retain(|dep| !lagged_initial_previous.contains(dep)); var_info.insert( - name.clone(), + (*name).clone(), VarInfo { is_stock: kind == SourceVariableKind::Stock, is_module: kind == SourceVariableKind::Module, @@ -280,7 +336,14 @@ pub(crate) fn build_var_info( // ensures that if the deps + implicit vars haven't changed, this // function is cached. for implicit in &deps.implicit_vars { - let mut dt_deps = implicit.dt_deps.clone(); + // Same stock-submodel-output dt chain-break as above (an implicit + // var can also read a stock submodel output). + let mut dt_deps: BTreeSet = implicit + .dt_deps + .iter() + .filter(|d| keep_dt_dep(d)) + .cloned() + .collect(); dt_deps.retain(|dep| !implicit.dt_init_only_referenced_vars.contains(dep)); dt_deps.retain(|dep| !implicit.dt_previous_referenced_vars.contains(dep)); let mut initial_deps = implicit.initial_deps.clone(); @@ -2244,7 +2307,20 @@ pub(crate) fn model_dependency_graph_impl( } init_set.extend(additional); } - let init_list: Vec<&String> = init_set.into_iter().collect(); + // `init_set` is a `HashSet`, so its iteration order is a function of + // the per-process HashMap RandomState seed, not of the model. Sort + // before handing it to `topo_sort_str`, which emits `names` in visit + // order and breaks ties (variables with no ordering dependency between + // them -- independent constants, stocks, `INITIAL()`/`PREVIOUS()` + // helpers) by exactly that order. Without this sort the initials + // runlist is HashMap-iteration-order dependent: two compiles of the + // SAME model produce different init orderings and therefore different + // initial values for any unordered pair (GH #595). The Flows and + // Stocks runlists already filter the pre-sorted `var_names`, so they + // are deterministic by construction; this matches that contract for + // the initials phase. + let mut init_list: Vec<&String> = init_set.into_iter().collect(); + init_list.sort_unstable(); topo_sort_str( init_list, &initial_dependencies, diff --git a/src/simlin-engine/src/db_dep_graph_tests.rs b/src/simlin-engine/src/db_dep_graph_tests.rs index 990b61e63..c30e26f54 100644 --- a/src/simlin-engine/src/db_dep_graph_tests.rs +++ b/src/simlin-engine/src/db_dep_graph_tests.rs @@ -3032,3 +3032,196 @@ fn resolve_dt_self_loop_subsumed_by_multi_scc_resolves_no_duplicate() { scc_map corruption)" ); } + +// ── Initials runlist ordering determinism (GH #595) ───────────────────── +// +// The Initials runlist is built by `topo_sort_str(init_list, ..)` where +// `init_list` is the set of init-phase variables (stocks, modules, +// INIT()-referenced vars, the empty-dt/non-empty-init `INITIAL()`-backed +// vars, plus their transitive init deps). `topo_sort_str` emits names in +// the *visit order of its `names` argument*, breaking ties (variables with +// no ordering dependency between them) by that argument's order. If +// `init_list` is materialized from a `HashSet` in iteration order, the +// runlist becomes HashMap-RandomState dependent: two compiles of the SAME +// model produce DIFFERENT init orderings, so a `PREVIOUS()`/`INITIAL()` +// variable can be evaluated before or after an unrelated init helper, +// yielding nondeterministic initial values (the Flows and Stocks runlists +// filter the pre-sorted `var_names`, so they were already deterministic). +// +// The fix: `init_list` must be a deterministic function of the model +// alone. The flows/stocks runlists achieve this by filtering the sorted +// `var_names`; the initials runlist must likewise sort its names before +// `topo_sort_str`. These tests pin that property without relying on +// probability: every fresh `SimlinDb` (fresh per-HashMap RandomState +// seeds) must produce a BYTE-IDENTICAL `runlist_initials`, and the order +// must equal the topological sort with a stable (sorted-name) tie-break. + +/// A model whose Initials runlist contains many variables with NO ordering +/// dependency between them: independent constant-init stocks, an +/// `INITIAL()`-backed aux that pins several constants into the init runlist, +/// and a `PREVIOUS()` aux (whose lagged dep is stripped from both phases, so +/// it too is unordered relative to its input). With this many unordered init +/// nodes a HashMap-iteration-order-dependent `init_list` essentially never +/// repeats the same order across fresh databases. +fn init_runlist_determinism_fixture() -> datamodel::Project { + single_model_project(vec![ + aux_var("zeta", "1"), + aux_var("yankee", "2"), + aux_var("xray", "3"), + aux_var("whiskey", "4"), + aux_var("victor", "5"), + aux_var("uniform", "6"), + // `INITIAL()` pins each referenced constant into the initials runlist + // (an `INIT()`-referenced var) with no ordering edge between them. + aux_var( + "init_sum", + "INIT(zeta) + INIT(yankee) + INIT(xray) + INIT(whiskey) + INIT(victor) + INIT(uniform)", + ), + // A PREVIOUS aux: its lagged input dep is stripped from dt AND init + // deps, so `prev_alpha` is unordered relative to `victor` in the init + // runlist -- the exact symptom shape of C-LEARN's + // `previous_emissions_for_rate`. + aux_var("prev_alpha", "PREVIOUS(victor, 0)"), + datamodel::Variable::Stock(datamodel::Stock { + ident: "stock_bravo".to_string(), + equation: datamodel::Equation::Scalar("10".to_string()), + documentation: String::new(), + units: None, + inflows: vec![], + outflows: vec![], + ai_state: None, + uid: None, + compat: datamodel::Compat::default(), + }), + datamodel::Variable::Stock(datamodel::Stock { + ident: "stock_charlie".to_string(), + equation: datamodel::Equation::Scalar("20".to_string()), + documentation: String::new(), + units: None, + inflows: vec![], + outflows: vec![], + ai_state: None, + uid: None, + compat: datamodel::Compat::default(), + }), + datamodel::Variable::Stock(datamodel::Stock { + ident: "stock_delta".to_string(), + equation: datamodel::Equation::Scalar("30".to_string()), + documentation: String::new(), + units: None, + inflows: vec![], + outflows: vec![], + ai_state: None, + uid: None, + compat: datamodel::Compat::default(), + }), + ]) +} + +/// Build `runlist_initials` for `init_runlist_determinism_fixture` from a +/// freshly-seeded database. +fn init_runlist_once(project: &datamodel::Project) -> Vec { + let db = SimlinDb::default(); + let result = sync_from_datamodel(&db, project); + let model = result.models["main"].source; + let dep = crate::db::model_dependency_graph(&db, model, result.project); + dep.runlist_initials.clone() +} + +#[test] +fn initials_runlist_is_deterministic_across_fresh_databases() { + // Each `SimlinDb::default()` gets fresh per-HashMap RandomState seeds. A + // HashMap-iteration-order-dependent `init_list` would yield different + // runlist orders across these builds; a deterministic build yields one. + let project = init_runlist_determinism_fixture(); + let baseline = init_runlist_once(&project); + // Sanity: the fixture must actually populate the initials runlist with + // many independent nodes, else the test could pass vacuously. + assert!( + baseline.len() >= 8, + "fixture must put many independent vars in the initials runlist \ + (got {}): {baseline:?}", + baseline.len() + ); + for i in 0..32 { + let again = init_runlist_once(&project); + assert_eq!( + baseline, again, + "runlist_initials must be a deterministic function of the model \ + (fresh-database build {i} diverged -- HashMap-iteration-order \ + dependence reintroduced in the initials runlist build)" + ); + } +} + +#[test] +fn initials_runlist_is_sorted_topological_order() { + // Pin the deterministic order directly (not just self-consistency): the + // initials runlist must equal a stable topological sort with a + // sorted-name tie-break. We reconstruct that reference order from the + // engine's own `initial_dependencies` so the assertion tracks the real + // dependency edges rather than a hard-coded list, and confirm the engine + // emits exactly it. A HashMap-order-dependent build would (almost surely) + // disagree with this canonical order. + let project = init_runlist_determinism_fixture(); + + let db = SimlinDb::default(); + let result = sync_from_datamodel(&db, &project); + let model = result.models["main"].source; + let dep = crate::db::model_dependency_graph(&db, model, result.project); + let actual = dep.runlist_initials.clone(); + + // Reference: visit the runlist's members in sorted order, emitting each + // member's (sorted) init deps that are themselves in the runlist before + // the member -- a stable Kahn-style topological order with an alphabetical + // tie-break. This mirrors `topo_sort_str` fed a *sorted* `names` list. + let member_set: std::collections::BTreeSet = actual.iter().cloned().collect(); + let mut expected: Vec = Vec::new(); + let mut emitted: std::collections::BTreeSet = std::collections::BTreeSet::new(); + // ASSUMES this fixture's initials runlist is acyclic (no *resolved* init + // SCC among the members): a plain DFS post-order matches `topo_sort_str` + // only on a DAG. A resolved init SCC would be emitted SCC-contiguous at the + // SCC's slot by `topo_sort_str`'s condensation, and this reference + // reconstruction would diverge -- such a fixture needs SCC-aware expected + // order, not this helper. + fn emit( + name: &str, + deps: &std::collections::HashMap>, + members: &std::collections::BTreeSet, + emitted: &mut std::collections::BTreeSet, + out: &mut Vec, + ) { + if emitted.contains(name) { + return; + } + emitted.insert(name.to_string()); + if let Some(ds) = deps.get(name) { + for d in ds.iter() { + if members.contains(d.as_str()) { + emit(d, deps, members, emitted, out); + } + } + } + if members.contains(name) { + out.push(name.to_string()); + } + } + let mut sorted_members: Vec<&String> = member_set.iter().collect(); + sorted_members.sort(); + for m in sorted_members { + emit( + m, + &dep.initial_dependencies, + &member_set, + &mut emitted, + &mut expected, + ); + } + + assert_eq!( + actual, expected, + "runlist_initials must be the stable (sorted-name tie-break) \ + topological order; a divergence means the initials runlist build \ + is not sorting its candidate set before topo_sort_str" + ); +} diff --git a/src/simlin-engine/src/lib.rs b/src/simlin-engine/src/lib.rs index 25478b323..54488e235 100644 --- a/src/simlin-engine/src/lib.rs +++ b/src/simlin-engine/src/lib.rs @@ -63,6 +63,8 @@ pub mod json_sdai; mod json_sdai_proptest; pub mod layout; mod lexer; +#[cfg(test)] +mod lookup_only_tests; pub mod ltm; pub mod ltm_agg; pub mod ltm_augment; diff --git a/src/simlin-engine/src/lookup_only_tests.rs b/src/simlin-engine/src/lookup_only_tests.rs new file mode 100644 index 000000000..9b848d570 --- /dev/null +++ b/src/simlin-engine/src/lookup_only_tests.rs @@ -0,0 +1,429 @@ +// 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 +// This is a test orchestrator: it builds datamodel projects (data), drives the +// salsa incremental compile + the bytecode VM (I/O-ish orchestration), and +// asserts the resulting saved series. The pure logic under test lives in +// `compiler::Var::new` / `compiler::is_lookup_only`. + +//! Standalone graphical-function ("lookup-only") variable saved-value +//! semantics (#590). +//! +//! A *lookup-only* variable is one whose entire equation is an inline +//! graphical function with no functional argument (e.g. Vensim `g( (x,y)... )`, +//! imported as `equation = LOOKUP_SENTINEL` + a `gf`). These tests pin that +//! such a variable produces the same saved series across the scalar, arrayed +//! (`Equation::Arrayed`), and apply-to-all shapes, and that the fix does not +//! perturb a *applied* lookup (`out = LOOKUP(g, idx)`). +//! +//! ## Phase 1 determination: the standalone index is `gf(Time)` +//! +//! Genuine Vensim evaluates a standalone lookup-only variable's table at the +//! current simulation time -- `gf(Time)` -- NOT at a constant `0` (the engine's +//! prior scalar behavior) and NOT as a literal `0` (the engine's prior +//! arrayed/A2A behavior). This was confirmed empirically: applied consumers of +//! C-LEARN's lookup-only tables byte-match `gf(Time)` against `Ref.vdf` (the +//! table y-values stepped by year, with clamp-to-endpoint outside the +//! x-domain), and the codebase's own `MdlEquation::Implicit` arm +//! (`mdl/convert/variables.rs`) already lowers an unspecified-input gf to +//! `equation = "TIME"`. So `index_expr = Expr::App(BuiltinFn::Time, loc)`. +//! +//! Each fixture below keys its table(s) on `x = [2000, 2001, 2002]` and runs +//! the sim over `INITIAL TIME = 2000 .. FINAL TIME = 2002` at `dt = 1`, so +//! `gf(Time)` yields the table's y-values step by step: `[y@2000, y@2001, +//! y@2002]`. The y-values are chosen distinct and element-identifying so a +//! wrong index (a constant `gf(0)` clamps below the x-domain to `y@2000`, +//! giving a flat `[y@2000, y@2000, y@2000]`) or a zeroed series (literal `0`) +//! is unambiguously detectable. + +use crate::datamodel; +use crate::db::{SimlinDb, compile_project_incremental, sync_from_datamodel_incremental}; +use crate::mdl::LOOKUP_SENTINEL; +use crate::vm::Vm; + +/// A 3-point continuous graphical function keyed on `x = [2000, 2001, 2002]`. +/// Evaluated at integer years 2000..=2002 it returns `y2000`, `y2001`, `y2002` +/// exactly (an exact x-match in `vm::lookup`), and clamps to `y2000` / `y2002` +/// outside the x-domain. +fn year_gf(y2000: f64, y2001: f64, y2002: f64) -> datamodel::GraphicalFunction { + let ys = vec![y2000, y2001, y2002]; + let y_min = ys.iter().cloned().fold(f64::INFINITY, f64::min); + let y_max = ys.iter().cloned().fold(f64::NEG_INFINITY, f64::max); + datamodel::GraphicalFunction { + kind: datamodel::GraphicalFunctionKind::Continuous, + x_points: Some(vec![2000.0, 2001.0, 2002.0]), + y_points: ys, + x_scale: datamodel::GraphicalFunctionScale { + min: 2000.0, + max: 2002.0, + }, + y_scale: datamodel::GraphicalFunctionScale { + min: y_min, + max: y_max, + }, + } +} + +/// Sim window aligned to the `year_gf` x-domain: three save steps at +/// `Time = 2000, 2001, 2002` (dt = 1), so `gf(Time)` walks the table's +/// y-values one per step. +fn year_specs() -> datamodel::SimSpecs { + datamodel::SimSpecs { + start: 2000.0, + stop: 2002.0, + dt: datamodel::Dt::Dt(1.0), + save_step: None, + sim_method: datamodel::SimMethod::Euler, + time_units: None, + } +} + +fn aux_lookup_only(ident: &str, gf: datamodel::GraphicalFunction) -> datamodel::Variable { + datamodel::Variable::Aux(datamodel::Aux { + ident: ident.to_string(), + // Standalone lookup-only: the sentinel equation (no functional input) + // plus a variable-level graphical function. + equation: datamodel::Equation::Scalar(LOOKUP_SENTINEL.to_string()), + documentation: String::new(), + units: None, + gf: Some(gf), + ai_state: None, + uid: None, + compat: datamodel::Compat::default(), + }) +} + +fn project_with( + name: &str, + dimensions: Vec, + variables: Vec, +) -> datamodel::Project { + datamodel::Project { + name: name.to_string(), + sim_specs: year_specs(), + dimensions, + units: vec![], + models: vec![datamodel::Model { + name: "main".to_string(), + sim_specs: None, + variables, + views: vec![], + loop_metadata: vec![], + groups: vec![], + macro_spec: None, + }], + source: None, + ai_information: None, + } +} + +/// Compile + run a project via the incremental salsa path and return every +/// variable's full saved series keyed by (canonicalized) name -- including +/// arrayed per-element keys `name[elem]`. +fn run_series(project: &datamodel::Project) -> std::collections::HashMap> { + let mut db = SimlinDb::default(); + let sync = sync_from_datamodel_incremental(&mut db, project, None); + let compiled = compile_project_incremental(&db, sync.project, "main") + .unwrap_or_else(|e| panic!("lookup-only project should compile: {e:?}")); + let mut vm = Vm::new(compiled).expect("VM creation should succeed"); + vm.run_to_end().expect("VM run should succeed"); + let results = vm.into_results(); + crate::test_common::collect_results(&results) +} + +fn series_of<'a>(series: &'a std::collections::HashMap>, key: &str) -> &'a [f64] { + series + .get(key) + .unwrap_or_else(|| panic!("missing series for {key}; have {:?}", series.keys())) + .as_slice() +} + +fn assert_series_eq(actual: &[f64], expected: &[f64], what: &str) { + assert_eq!( + actual.len(), + expected.len(), + "{what}: step count mismatch (got {actual:?}, want {expected:?})" + ); + for (i, (a, e)) in actual.iter().zip(expected.iter()).enumerate() { + assert!( + (a - e).abs() < 1e-9, + "{what}: step {i} got {a}, want {e} (full got {actual:?}, want {expected:?})" + ); + } +} + +/// AC1.1: a SCALAR lookup-only variable is evaluated at `gf(Time)`, producing +/// the per-year table values `[y@2000, y@2001, y@2002]` -- NOT a constant +/// `gf(0)` (which clamps below the x-domain to a flat `[y@2000, y@2000, +/// y@2000]`). +#[test] +fn scalar_lookup_only_evaluates_at_time() { + let project = project_with( + "scalar_lookup_only", + vec![], + vec![aux_lookup_only("g", year_gf(11.0, 22.0, 33.0))], + ); + + let series = run_series(&project); + assert_series_eq( + series_of(&series, "g"), + &[11.0, 22.0, 33.0], + "scalar lookup-only g must evaluate at gf(Time), not a constant gf(0) \ + (which would be a flat [11, 11, 11])", + ); +} + +/// AC1.2 + AC1.5: an ARRAYED (`Equation::Arrayed`) lookup-only variable, with a +/// NON-alphabetical declared element order, evaluates each element's OWN table +/// at `gf(Time)` (per-element series), NOT a literal `0`. The `elems` Vec is +/// given in alphabetical order (the order the MDL importer's sort produces) +/// while the dimension is declared `Z, A, M`, so a positional table mis-map +/// would swap the per-element series -- pinning the per-element-GF layout +/// invariant for the standalone case too. +#[test] +fn arrayed_lookup_only_non_sorted_order_evaluates_own_table_at_time() { + // Declared order Z, A, M (NOT alphabetical). Each element's table carries + // an element-identifying triple: + // Z -> [100, 200, 300], A -> [1000, 2000, 3000], M -> [10, 20, 30]. + // `Equation::Arrayed` element tuples: (element name, equation, EXCEPT + // default, per-element gf). `ElementName` is a `String` alias. + let arrayed_elements: Vec<( + String, + String, + Option, + Option, + )> = vec![ + // elems Vec in ALPHABETICAL order (A, M, Z), each lookup-only. + ( + "A".to_string(), + LOOKUP_SENTINEL.to_string(), + None, + Some(year_gf(1000.0, 2000.0, 3000.0)), + ), + ( + "M".to_string(), + LOOKUP_SENTINEL.to_string(), + None, + Some(year_gf(10.0, 20.0, 30.0)), + ), + ( + "Z".to_string(), + LOOKUP_SENTINEL.to_string(), + None, + Some(year_gf(100.0, 200.0, 300.0)), + ), + ]; + + let g = datamodel::Variable::Aux(datamodel::Aux { + ident: "g".to_string(), + equation: datamodel::Equation::Arrayed( + vec!["Dim".to_string()], + arrayed_elements, + None, + false, + ), + documentation: String::new(), + units: None, + gf: None, + ai_state: None, + uid: None, + compat: datamodel::Compat::default(), + }); + + let project = project_with( + "arrayed_lookup_only", + vec![datamodel::Dimension::named( + "Dim".to_string(), + vec!["Z".to_string(), "A".to_string(), "M".to_string()], + )], + vec![g], + ); + + let series = run_series(&project); + // Each element reads its OWN table (declared index Z=0, A=1, M=2) at + // gf(Time). A literal-0 lowering would give [0, 0, 0]; a positional + // mis-map would swap Z and A's series. + assert_series_eq( + series_of(&series, "g[z]"), + &[100.0, 200.0, 300.0], + "arrayed lookup-only element Z (declared index 0) at gf(Time)", + ); + assert_series_eq( + series_of(&series, "g[a]"), + &[1000.0, 2000.0, 3000.0], + "arrayed lookup-only element A (declared index 1) at gf(Time)", + ); + assert_series_eq( + series_of(&series, "g[m]"), + &[10.0, 20.0, 30.0], + "arrayed lookup-only element M (declared index 2) at gf(Time)", + ); +} + +/// AC1.3: an apply-to-all lookup-only variable (one variable-level table shared +/// by every element) evaluates that single shared table at `gf(Time)` for every +/// element, NOT a literal `0`. Critically, every element must read the BASE +/// table offset (table_count == 1); a per-element `off + i` would push the VM's +/// Lookup bounds check out of range for i > 0 and return NaN. +#[test] +fn a2a_lookup_only_shares_one_table_at_time() { + let g = datamodel::Variable::Aux(datamodel::Aux { + ident: "g".to_string(), + equation: datamodel::Equation::ApplyToAll( + vec!["Dim".to_string()], + LOOKUP_SENTINEL.to_string(), + ), + documentation: String::new(), + units: None, + // One variable-level GF, shared by every element. + gf: Some(year_gf(7.0, 8.0, 9.0)), + ai_state: None, + uid: None, + compat: datamodel::Compat::default(), + }); + + let project = project_with( + "a2a_lookup_only", + vec![datamodel::Dimension::named( + "Dim".to_string(), + vec!["P".to_string(), "Q".to_string(), "R".to_string()], + )], + vec![g], + ); + + let series = run_series(&project); + for elem in ["p", "q", "r"] { + assert_series_eq( + series_of(&series, &format!("g[{elem}]")), + &[7.0, 8.0, 9.0], + &format!( + "A2A lookup-only element {elem} must read the single shared table at \ + gf(Time) (a literal 0 -> [0,0,0]; an off+i bounds overflow -> NaN)" + ), + ); + } +} + +/// AC1.2 (layout robustness): an ARRAYED (`Equation::Arrayed`) lookup-only +/// variable that carries a SINGLE *variable-level* gf (no per-element gfs) must +/// have every element read that one shared table at `gf(Time)`. +/// +/// This is the symmetric twin of the A2A bounds hazard. `variable.rs::build_tables` +/// only produces a per-element table layout (`tables.len() == n`) when at least +/// one element carries its OWN gf; an `Equation::Arrayed` with empty/sentinel +/// element equations and a single *variable-level* gf falls through to the +/// shared-table branch (`tables.len() == 1`), exactly like A2A. So the arrayed +/// lookup-only lowering must wrap the BASE offset (`elem_off = 0`, +/// `table_count = 1`) here, NOT `off + i` -- a per-element `off + i` would make +/// `element_offset == i >= 1 == table_count` for i > 0 and the VM's `Lookup` +/// opcode would push NaN for every element after the first. +/// +/// The MDL/XMILE importers never emit this exact shape (they attach per-element +/// gfs to an arrayed lookup), but `datamodel::Project` is a public compile input +/// (protobuf/JSON/serde/MCP/pysimlin/libsimlin all preserve it verbatim), so a +/// directly-constructed project can reach it and must compile correctly. +#[test] +fn arrayed_lookup_only_variable_level_gf_shares_one_table_at_time() { + // `Equation::Arrayed` with empty/sentinel element equations and NO + // per-element gfs, plus a single variable-level gf shared by every element. + let arrayed_elements: Vec<( + String, + String, + Option, + Option, + )> = vec![ + ("P".to_string(), LOOKUP_SENTINEL.to_string(), None, None), + ("Q".to_string(), LOOKUP_SENTINEL.to_string(), None, None), + ("R".to_string(), LOOKUP_SENTINEL.to_string(), None, None), + ]; + + let g = datamodel::Variable::Aux(datamodel::Aux { + ident: "g".to_string(), + equation: datamodel::Equation::Arrayed( + vec!["Dim".to_string()], + arrayed_elements, + None, + false, + ), + documentation: String::new(), + units: None, + // One variable-level GF, shared by every element (tables.len() == 1). + gf: Some(year_gf(7.0, 8.0, 9.0)), + ai_state: None, + uid: None, + compat: datamodel::Compat::default(), + }); + + let project = project_with( + "arrayed_lookup_only_variable_level_gf", + vec![datamodel::Dimension::named( + "Dim".to_string(), + vec!["P".to_string(), "Q".to_string(), "R".to_string()], + )], + vec![g], + ); + + let series = run_series(&project); + for elem in ["p", "q", "r"] { + assert_series_eq( + series_of(&series, &format!("g[{elem}]")), + &[7.0, 8.0, 9.0], + &format!( + "arrayed lookup-only element {elem} with a single variable-level gf must \ + read the one shared table at gf(Time) (an off+i bounds overflow -> NaN \ + for every element after the first)" + ), + ); + } +} + +/// AC1.4 (no regression): a graphical-function variable that is *applied* with +/// an argument elsewhere (`out = LOOKUP(g, idx)` where `idx` is a real input +/// distinct from Time) still produces the applied value `gf(idx)`. The +/// standalone-lookup fix only changes which expression `g` itself is evaluated +/// at; it must not perturb the applied path. We assert BOTH halves on one +/// model: `out` follows `idx` (constant 2001 -> a flat `gf(2001) = 22`), while +/// the standalone `g` follows `gf(Time)` (`[11, 22, 33]`). +#[test] +fn applied_lookup_only_uses_argument_not_time_no_regression() { + let g = aux_lookup_only("g", year_gf(11.0, 22.0, 33.0)); + // `idx` is a real input distinct from Time: a constant 2001. So + // out = LOOKUP(g, 2001) = gf(2001) = 22 at every step. + let idx = datamodel::Variable::Aux(datamodel::Aux { + ident: "idx".to_string(), + equation: datamodel::Equation::Scalar("2001".to_string()), + documentation: String::new(), + units: None, + gf: None, + ai_state: None, + uid: None, + compat: datamodel::Compat::default(), + }); + let out = datamodel::Variable::Aux(datamodel::Aux { + ident: "out".to_string(), + equation: datamodel::Equation::Scalar("LOOKUP(g, idx)".to_string()), + documentation: String::new(), + units: None, + gf: None, + ai_state: None, + uid: None, + compat: datamodel::Compat::default(), + }); + + let project = project_with("applied_lookup_only", vec![], vec![g, idx, out]); + + let series = run_series(&project); + assert_series_eq( + series_of(&series, "out"), + &[22.0, 22.0, 22.0], + "applied LOOKUP(g, idx) must follow idx (gf(2001) = 22), not Time -- \ + the standalone fix must not perturb the applied path", + ); + assert_series_eq( + series_of(&series, "g"), + &[11.0, 22.0, 33.0], + "standalone g still evaluates at gf(Time) even when applied elsewhere", + ); +} diff --git a/src/simlin-engine/src/macro_expansion_tests.rs b/src/simlin-engine/src/macro_expansion_tests.rs index 97cc97098..9f24abf20 100644 --- a/src/simlin-engine/src/macro_expansion_tests.rs +++ b/src/simlin-engine/src/macro_expansion_tests.rs @@ -483,6 +483,107 @@ y= } } +/// clearn-residual.AC2.1/AC2.2/AC2.4: a 5-parameter `RAMP FROM TO` macro that +/// branches on its `islinear` selector must run the branch the caller selects. +/// This is the discriminating test the existing fixtures lack: the macro's +/// `exp` branch (here `xfrom + 2*RAMP(...)`, deliberately *double* slope so it +/// is provably distinct from the linear branch mid-ramp) cannot be reproduced +/// by the import-time linear rewrite, so with the buggy formatter all three +/// invocations collapse to the linear value and `y_exp` fails. +/// +/// Harness control (`CONTROL_TAIL`): INITIAL TIME = 0, FINAL TIME = 2, +/// TIME STEP = SAVEPER = 1, so the saved steps are Time = {0, 1, 2}. The ramp +/// window `tstart = 0 .. tend = 2` puts the single interior saved step at +/// Time = 1, where `RAMP(slope, 0, 2) == slope * (1 - 0) == slope`. +/// +/// Expected values at Time = 1 (series index 1): +/// - `y_exp = RAMP FROM TO(10, 110, 0, 2, 0)`: slope = (110-10)/(2-0) = 50; +/// both endpoints positive so `linear = islinear = 0` -> exp branch: +/// `10 + 2*50 = 110` (linear branch would be `10 + 50 = 60`). +/// - `y_lin = RAMP FROM TO(10, 110, 0, 2, 1)`: `linear = 1` -> linear branch: +/// `10 + 50 = 60`. +/// - `y_force = RAMP FROM TO(-10, 110, 0, 2, 0)`: slope = (110-(-10))/(2-0) = 60; +/// `xfrom <= 0` forces `linear = 1` despite `islinear = 0` -> linear branch: +/// `-10 + 60 = 50` (exp branch would be `-10 + 2*60 = 110`). +#[test] +fn macro_ramp_from_to_runs_selected_branch() { + let source = mdl(r#":MACRO: RAMP FROM TO(xfrom, xto, tstart, tend, islinear) +RAMP FROM TO = IF THEN ELSE(linear = 1, linear ramp, exp ramp) + ~ dmnl + ~ | +linear = IF THEN ELSE(xfrom > 0 :AND: xto > 0, islinear, 1) + ~ dmnl + ~ | +slope = (xto - xfrom) / (tend - tstart) + ~ dmnl + ~ | +linear ramp = xfrom + RAMP(slope, tstart, tend) + ~ dmnl + ~ | +exp ramp = xfrom + 2 * RAMP(slope, tstart, tend) + ~ dmnl + ~ | +:END OF MACRO: +y_exp= + RAMP FROM TO(10, 110, 0, 2, 0) + ~ + ~ | + +y_lin= + RAMP FROM TO(10, 110, 0, 2, 1) + ~ + ~ | + +y_force= + RAMP FROM TO(-10, 110, 0, 2, 0) + ~ + ~ | +"#); + + // Time = 1 is series index 1 (saved steps Time = 0, 1, 2). + const MID: usize = 1; + + let y_exp = run_mdl_var(&source, "y_exp"); + let y_lin = run_mdl_var(&source, "y_lin"); + let y_force = run_mdl_var(&source, "y_force"); + + // AC2.1: islinear = 0 with positive endpoints runs the exp branch (110), + // which is provably distinct from the linear branch (60). The import-time + // linearizer cannot produce 110, so this assertion is the true RED. + assert!( + (y_exp[MID] - 110.0).abs() < 1e-9, + "y_exp at Time=1 expected 110 (exp branch: xfrom + 2*slope), got {}", + y_exp[MID] + ); + assert!( + (y_exp[MID] - 60.0).abs() > 1e-6, + "y_exp at Time=1 must NOT equal the linear value 60; got {} -- the \ + macro's exp branch did not run (formatter linearized the call)", + y_exp[MID] + ); + + // AC2.2: islinear = 1 runs the linear branch (60), the no-regression value. + assert!( + (y_lin[MID] - 60.0).abs() < 1e-9, + "y_lin at Time=1 expected 60 (linear branch: xfrom + slope), got {}", + y_lin[MID] + ); + + // AC2.4: nonpositive endpoint forces linear = 1 despite islinear = 0, so + // the linear branch (50) runs, not the exp branch (110). + assert!( + (y_force[MID] - 50.0).abs() < 1e-9, + "y_force at Time=1 expected 50 (forced-linear branch: xfrom + slope), got {}", + y_force[MID] + ); + assert!( + (y_force[MID] - 110.0).abs() > 1e-6, + "y_force at Time=1 must NOT equal the exp value 110; got {} -- the \ + forced-linear selector did not take effect", + y_force[MID] + ); +} + /// macros.AC5.6: a call to a name that is neither a macro, a stdlib /// function, nor a builtin must fail the compile with `UnknownBuiltin`. #[test] diff --git a/src/simlin-engine/src/mdl/xmile_compat.rs b/src/simlin-engine/src/mdl/xmile_compat.rs index abb0ec7d5..cbdf48a8b 100644 --- a/src/simlin-engine/src/mdl/xmile_compat.rs +++ b/src/simlin-engine/src/mdl/xmile_compat.rs @@ -257,7 +257,33 @@ impl XmileFormatter { ) -> String { let canonical = to_lower_space(name); - // Handle special function transformations + // Macro-shadowing audit (clearn-residual.AC2.5). + // + // Some arms below *restructure* a builtin-named call into a different + // expression at import time (e.g. `MODULO(x, y)` -> `(x) MOD (y)`, + // `SAMPLE IF TRUE(...)` -> a nested form). That rewrite happens BEFORE + // compile-time macro resolution, so if a model defines a `:MACRO:` with + // the same name, the rewrite silently pre-empts it: the macro body and + // any selector arguments are discarded and the macro never runs. Vensim + // semantics are the opposite -- a project macro shadows the like-named + // builtin (resolved in `builtins_visitor.rs` before alias/modulo/ + // builtin handling), so the macro must reach that resolver intact. + // + // The `ramp from to` arm carried exactly this hazard and was the one + // C-LEARN tripped; it has been removed so a `RAMP FROM TO(...)` call + // survives import as `RAMP_FROM_TO(...)` and resolves through the macro + // path. The other restructuring arms here (notably the multi-word + // `sample if true`, plus `pulse`/`pulse train`/`modulo`/`zidz`/`xidz`/ + // `get data between times`/etc.) carry the SAME latent hazard but are + // not currently shadowed by any in-repo model. If a future model + // defines a macro with one of those names, that arm must be guarded the + // same way (let the call fall through so macro resolution can apply). + // + // Name-preserving renames are SAFE and need no change: `SSHAPE` is + // handled only by `format_function_name` (it is a genuine 3-arg + // builtin, not restructured here), so a same-named macro shadows it + // cleanly. The `ac5_4_macro_shadows_*` tests pin both the multi-word + // (`RAMP FROM TO`) and single-word (`SSHAPE`) shadowing cases. match canonical.as_str() { "a function of" => { // xmutil emits literal NAN, not NAN(args) @@ -419,19 +445,6 @@ impl XmileFormatter { ); } } - "ramp from to" => { - // RAMP FROM TO(y_start, y_end, t_start, t_end) -> - // y_start + RAMP((y_end - y_start) / (t_end - t_start), t_start, t_end) - if args.len() >= 4 { - let y_start = self.format_expr_ctx(&args[0], ctx); - let y_end = self.format_expr_ctx(&args[1], ctx); - let t_start = self.format_expr_ctx(&args[2], ctx); - let t_end = self.format_expr_ctx(&args[3], ctx); - return format!( - "({y_start}) + RAMP((({y_end}) - ({y_start})) / (({t_end}) - ({t_start})), {t_start}, {t_end})" - ); - } - } "modulo" => { // MODULO(x, y) -> (x) MOD (y) if args.len() >= 2 { @@ -2029,7 +2042,14 @@ mod tests { } #[test] - fn test_ramp_from_to_transforms_args() { + fn test_ramp_from_to_survives_as_macro_call() { + // `RAMP FROM TO` is not a native builtin: a model that uses it must + // define a same-named `:MACRO:`, which the compile-time macro-shadows- + // everything precedence (`builtins_visitor.rs`) resolves. The formatter + // must therefore leave the call intact as `RAMP_FROM_TO(...)` so that + // resolution can apply -- it must NOT rewrite it into a hardcoded + // linear `(xfrom) + RAMP(slope, tstart, tend)` at import time (which + // would discard the macro body and its `islinear` selector). let formatter = XmileFormatter::new(); let expr = Expr::App( Cow::Borrowed("ramp from to"), @@ -2045,12 +2065,29 @@ mod tests { loc(), ); let result = formatter.format_expr(&expr); - assert!(result.contains("RAMP"), "should contain RAMP: {result}"); + + // The call survives as a resolvable `RAMP_FROM_TO(...)` invocation + // (canonicalizes to `ramp_from_to` at compile time). + assert!( + result.starts_with("RAMP_FROM_TO("), + "expected the call to survive as RAMP_FROM_TO(...): {result}" + ); + + // The four argument expressions appear, in order, inside the call. + let mut search_from = 0usize; + for arg in ["0", "100", "5", "15"] { + let rel = result[search_from..] + .find(arg) + .unwrap_or_else(|| panic!("arg {arg:?} missing or out of order in: {result}")); + search_from += rel + arg.len(); + } + + // No top-level linear rewrite: the linearized form contained a bare + // `RAMP(` call (and a `+`). `RAMP_FROM_TO(` does not match `RAMP(`. assert!( - !result.starts_with("RAMP_FROM_TO"), - "should not be bare RAMP_FROM_TO: {result}" + !result.contains("RAMP("), + "must not be linearized into a bare RAMP(...) call: {result}" ); - assert!(result.contains("100") && result.contains("0"), "{result}"); } #[test] diff --git a/src/simlin-engine/src/model.rs b/src/simlin-engine/src/model.rs index 9dd0f8915..b4250b751 100644 --- a/src/simlin-engine/src/model.rs +++ b/src/simlin-engine/src/model.rs @@ -857,8 +857,19 @@ pub(crate) fn equation_is_module_call( match &ast { Expr0::App(crate::builtins::UntypedBuiltinFn(func, _args), _) => { let func_lower = func.to_lowercase(); - crate::builtins::is_stdlib_module_function(&func_lower) - || macro_registry.resolve_macro(func).is_some() + // Any resolvable project macro counts as a module call here, + // including a genuine passthrough macro (`:MACRO: INIT(x) = + // INITIAL(x)`) that `builtins_visitor::walk` later collapses to a + // scalar opcode (#591-c1). Pre-classifying the passthrough caller + // as module-backed is benign: the only downstream consumer of this + // result is `is_module_backed_ident`, which gates whether a + // *referencing* `PREVIOUS`/`INIT` synthesizes a scalar temp arg + // (`builtins_visitor.rs`). Since a passthrough caller collapses to + // a plain flat-slot variable, that temp-arg copy is value-identical + // to reading the slot directly -- so the classification does not + // change any observable result either way. + let is_module_macro = macro_registry.resolve_macro(func).is_some(); + crate::builtins::is_stdlib_module_function(&func_lower) || is_module_macro } _ => false, } diff --git a/src/simlin-engine/src/module_functions.rs b/src/simlin-engine/src/module_functions.rs index d824ac94f..af64d0877 100644 --- a/src/simlin-engine/src/module_functions.rs +++ b/src/simlin-engine/src/module_functions.rs @@ -57,6 +57,18 @@ pub(crate) struct ModuleFunctionDescriptor { /// `parameter_ports.len()`); false for stdlib functions, which permit /// fewer arguments than ports (trailing ports are optional). pub is_macro: bool, + /// `Some` iff this is a *genuine passthrough* macro -- a single-parameter, + /// single-output macro whose primary-output body is exactly + /// `out = BUILTIN(param)` with `BUILTIN` canonicalizing to the macro's own + /// renamed-builtin-collision name (`:MACRO: INIT(x) = INITIAL(x)` -> + /// `init = init(x)`). The call site reads this to collapse the macro + /// directly to its opcode (`LoadInitial`) instead of expanding the buggy + /// per-element synthetic module. Always `None` for stdlib descriptors and + /// for non-passthrough macros. Classified once at [`MacroRegistry::build`] + /// time (the only place the body AST is available) via + /// [`classify_passthrough`], and participates in `PartialEq`/`Eq` so salsa + /// invalidation stays correct. + pub passthrough: Option, } /// The single source of truth for stdlib input-port names and order. Each @@ -181,6 +193,140 @@ pub(crate) fn is_renamed_builtin_macro_collision(canonical: &str) -> bool { is_renamed_opcode_intrinsic(canonical) || is_renamed_stdlib_module_builtin(canonical) } +/// A *genuine passthrough* macro classification: a single-parameter macro whose +/// primary-output body is exactly `out = BUILTIN(param)`, where `BUILTIN` +/// canonicalizes to the same renamed-builtin-collision name as the macro itself +/// (the self-call shape the MDL importer's `INITIAL` -> `INIT` rename produces: +/// `:MACRO: INIT(x) = INITIAL(x)` stored as `init = init(x)`). +/// +/// When present, the call site collapses the macro directly to its proven +/// opcode (for `init`, `LoadInitial`) by *skipping* `expand_module_function` +/// and falling through to the existing renamed-builtin intrinsic routing -- the +/// same fall-through the #554 self-call exception takes inside a macro body, +/// here generalized to the call site. This avoids the buggy per-element +/// synthetic module the macro would otherwise expand into. +// +// `salsa::Update` so it can ride on `ModuleFunctionDescriptor` (which is held +// by the salsa-tracked `project_macro_registry` query); a pure data marker, not +// a side effect, mirroring the descriptor's own derivation rationale. +#[cfg_attr(feature = "debug-derive", derive(Debug))] +#[derive(Clone, PartialEq, Eq, salsa::Update)] +pub(crate) struct PassthroughBuiltin { + /// The canonical name of the renamed builtin the macro collapses to (e.g. + /// `"init"`). This equals `canonicalize(macro_name)` and satisfies + /// [`is_renamed_builtin_macro_collision`], so the call-site fall-through + /// routes the call to the correct opcode/intrinsic. + pub canonical_builtin: String, +} + +/// Pure structural classifier (Functional Core, no registry/IO access): +/// decide whether a macro is a *genuine passthrough* of a renamed builtin -- +/// `Some(PassthroughBuiltin)` iff ALL of: +/// +/// 1. the macro has exactly one parameter (`parameter_ports.len() == 1`); +/// 2. the macro has no additional outputs (a multi-output `:`-list macro +/// delivers more than the primary output, so it cannot collapse to one +/// opcode); +/// 3. the primary-output body AST is exactly `App(BUILTIN, [arg])` -- a single +/// call with a single argument; +/// 4. `arg` is exactly `Var(the sole parameter)` (the bare parameter, NOT an +/// expression like `param * 2`, which would do work the collapse drops); +/// 5. `canonicalize(call) == canonicalize(macro_name)` (a self-call -- the +/// form the importer's `INITIAL` -> `INIT` rename produces); and +/// 6. `is_renamed_builtin_macro_collision(canonicalize(call))` is `true`, so +/// the call-site fall-through to the existing intrinsic routing lands on a +/// real opcode-backed builtin (`init`/`previous`) or stdlib module rather +/// than `UnknownBuiltin`. +/// +/// Otherwise `None`. The strictness of (3)-(6) guarantees the collapse cannot +/// misfire on a non-passthrough macro that merely shares a builtin name (e.g. +/// `INIT = INIT(x) + 1`, or `INIT = INIT(x * 2)`): such a macro keeps expanding +/// as a module. +pub(crate) fn classify_passthrough( + macro_name: &str, + parameter_ports: &[String], + additional_outputs: &[String], + primary_output_body_ast: &Expr0, +) -> Option { + // (1) exactly one parameter; (2) no additional outputs. + if parameter_ports.len() != 1 || !additional_outputs.is_empty() { + return None; + } + let sole_param = ¶meter_ports[0]; + + // (3) the body is exactly a single one-argument call. + let Expr0::App(UntypedBuiltinFn(call, args), _) = primary_output_body_ast else { + return None; + }; + let [arg] = args.as_slice() else { + return None; + }; + + // (4) the single argument is the bare sole parameter (canonical match, so a + // case/whitespace variant of the formal parameter still counts). + let Expr0::Var(arg_ident, _) = arg else { + return None; + }; + if canonicalize(arg_ident.as_str()) != canonicalize(sole_param) { + return None; + } + + // (5) the call is a self-call: its canonical name equals the macro's. + let call_canonical = canonicalize(call); + if call_canonical != canonicalize(macro_name) { + return None; + } + + // (6) the (self-)call name is a renamed-builtin collision, so the call-site + // fall-through routes it to a real opcode/intrinsic. + if !is_renamed_builtin_macro_collision(call_canonical.as_ref()) { + return None; + } + + Some(PassthroughBuiltin { + canonical_builtin: call_canonical.into_owned(), + }) +} + +/// Bridge from a datamodel macro `Model`/`MacroSpec` to the pure +/// [`classify_passthrough`]: locate the primary-output body variable, parse its +/// (single, scalar) equation, and classify. Returns `None` (not a passthrough) +/// when the primary output is missing, has no equation, is an arrayed +/// multi-formula body (a passthrough's `out = BUILTIN(param)` is a single scalar +/// formula), or fails to parse. +/// +/// This is the only place each macro body equation is parsed for +/// classification, so the (transient) body AST never needs to escape registry +/// build. Kept structural over the parsed AST -- no IO -- so the registry stays +/// a Functional Core. +fn classify_macro_passthrough( + model: &datamodel::Model, + spec: &datamodel::MacroSpec, +) -> Option { + let primary_canonical = canonicalize(&spec.primary_output); + let primary_var = model + .variables + .iter() + .find(|v| canonicalize(v.get_ident()) == primary_canonical)?; + let equation = primary_var.get_equation()?; + // A genuine passthrough body is a single scalar formula. An arrayed body + // yields multiple per-element formulas (which `Equation::source_text` + // `\n`-joins into something that does not reparse as one expression), so it + // can never be the bare `out = BUILTIN(param)` shape -- treat it as a + // non-passthrough rather than guessing at one element. + let formulas = equation_formulas(equation); + let [formula] = formulas.as_slice() else { + return None; + }; + let ast = Expr0::new(formula, LexerType::Equation).ok()??; + classify_passthrough( + &model.name, + &spec.parameters, + &spec.additional_outputs, + &ast, + ) +} + /// Build a [`ModuleFunctionDescriptor`] for a stdlib module-function. /// /// Called *after* `rewrite_alias_module_call` has normalized aliases, so @@ -200,6 +346,8 @@ pub(crate) fn stdlib_descriptor(name: &str) -> Option primary_output: "output".to_string(), additional_outputs: vec![], is_macro: false, + // Stdlib functions are never passthrough macros. + passthrough: None, }) } @@ -242,6 +390,14 @@ impl MacroRegistry { format!("duplicate macro definition: {}", canonical) ); } + // Classify a genuine passthrough macro (single-param `out = + // BUILTIN(param)` self-call of a renamed builtin) once here, where + // the body is parseable; the call site reads this off the + // descriptor to collapse it to the opcode rather than expanding the + // buggy per-element synthetic module (#591-c1). A non-passthrough + // macro -- including Phase 2's RAMP FROM TO, which is NOT a + // passthrough -- gets `None` and still expands as a module. + let passthrough = classify_macro_passthrough(model, spec); macros.insert( canonical.clone(), ModuleFunctionDescriptor { @@ -250,6 +406,7 @@ impl MacroRegistry { primary_output: spec.primary_output.clone(), additional_outputs: spec.additional_outputs.clone(), is_macro: true, + passthrough, }, ); } @@ -1120,4 +1277,222 @@ mod tests { ); assert_eq!(err.code, crate::common::ErrorCode::CircularDependency); } + + // --- MacroRegistry::build threads the passthrough classification ---------- + // + // The pure `classify_passthrough` is computed once at registry-build time + // (the only place each macro body is parsed) and stored on the descriptor, + // so the call site can read it without re-parsing the (discarded) body AST. + + #[test] + fn build_classifies_init_passthrough_macro_as_some() { + // The #591-c1 shape: `:MACRO: INIT(x) = INITIAL(x)` stored as the + // datamodel macro body `init = init(x)` after the importer rename. + let models = vec![plain_model("main"), macro_model("init", &["x"], "init(x)")]; + let registry = MacroRegistry::build(&models).expect("the INIT passthrough macro builds"); + let d = registry + .resolve_macro("init") + .expect("the INIT macro resolves"); + assert_eq!( + d.passthrough, + Some(PassthroughBuiltin { + canonical_builtin: "init".to_string() + }), + "a genuine `INIT = INIT(x)` passthrough must be classified at build time" + ); + } + + #[test] + fn build_does_not_classify_near_miss_init_macro() { + // `:MACRO: INIT(x) = INITIAL(x) + 1` is NOT a bare passthrough -- the + // `+ 1` is real work the opcode collapse would drop -- so the descriptor + // must record `passthrough == None` and the macro keeps expanding. + let models = vec![ + plain_model("main"), + macro_model("init", &["x"], "init(x) + 1"), + ]; + let registry = + MacroRegistry::build(&models).expect("the near-miss INIT macro still builds"); + let d = registry + .resolve_macro("init") + .expect("the INIT macro resolves"); + assert_eq!( + d.passthrough, None, + "INIT = INIT(x) + 1 is a near-miss and must NOT be classified as a passthrough" + ); + } + + #[test] + fn build_leaves_non_passthrough_macro_descriptor_passthrough_none() { + // An ordinary macro (not a renamed-builtin self-call at all) must carry + // `passthrough == None`. + let models = vec![ + plain_model("main"), + macro_model("mymacro", &["a", "b"], "a * b"), + ]; + let registry = MacroRegistry::build(&models).expect("ordinary macro project builds"); + let d = registry.resolve_macro("mymacro").expect("mymacro resolves"); + assert_eq!( + d.passthrough, None, + "a non-passthrough macro must have passthrough == None" + ); + } + + #[test] + fn stdlib_descriptor_passthrough_is_none() { + // Stdlib (non-macro) descriptors are never passthroughs. + let d = stdlib_descriptor("smth1").expect("smth1 is a stdlib module-function"); + assert_eq!( + d.passthrough, None, + "a stdlib descriptor is not a passthrough macro" + ); + } + + // --- classify_passthrough: the pure structural passthrough classifier --- + // + // A `:MACRO: INIT(x) = INITIAL(x)` collides (after the MDL importer renames + // `INITIAL` -> `INIT`) with the opcode-backed `init` intrinsic, so its + // datamodel body is `init = init(x)`. Such a *genuine passthrough* macro + // (single param; body exactly `out = BUILTIN(param)` where `BUILTIN` + // canonicalizes to the same renamed-builtin collision name) is collapsed at + // the call site directly to the opcode (LoadInitial), bypassing the buggy + // per-element synthetic module. `classify_passthrough` is the pure + // structural rule that decides this; it must NOT misfire on a non-passthrough + // macro that merely shares a builtin name. + + /// Parse a macro body equation into the `Expr0` AST the classifier expects. + fn body_ast(equation: &str) -> Expr0 { + Expr0::new(equation, LexerType::Equation) + .expect("body equation must parse") + .expect("body equation must not be empty") + } + + #[test] + fn classify_passthrough_init_self_call_is_some_init() { + // The exact #591-c1 shape: `INIT = INIT(x)` (single param `x`), the + // datamodel form of `:MACRO: INIT(x) = INITIAL(x)` after the importer + // rename. `init` is an opcode-backed renamed-builtin collision, so the + // call-site fall-through to the `init`->LoadInitial intrinsic routing is + // valid -- classify as a passthrough targeting `init`. + let result = classify_passthrough("init", &["x".to_string()], &[], &body_ast("init(x)")); + assert_eq!( + result, + Some(PassthroughBuiltin { + canonical_builtin: "init".to_string() + }), + "INIT = INIT(x) is a genuine passthrough to the `init` opcode" + ); + } + + #[test] + fn classify_passthrough_op2_body_is_none() { + // `INIT = INIT(x) + 1` is NOT a bare passthrough: the body is an Op2, + // not a single call, so collapsing it to the opcode would drop the + // `+ 1` (AC3.4 negative). + let result = + classify_passthrough("init", &["x".to_string()], &[], &body_ast("init(x) + 1")); + assert_eq!( + result, None, + "INIT = INIT(x) + 1 is an Op2 body, not a bare passthrough" + ); + } + + #[test] + fn classify_passthrough_arg_not_bare_param_is_none() { + // `INIT = INIT(x * 2)`: the call's argument is an expression, not the + // bare parameter, so the macro does real work the opcode collapse would + // discard (AC3.4 negative). + let result = + classify_passthrough("init", &["x".to_string()], &[], &body_ast("init(x * 2)")); + assert_eq!( + result, None, + "INIT = INIT(x * 2) has an expression argument, not the bare param" + ); + } + + #[test] + fn classify_passthrough_two_param_body_is_none() { + // A two-parameter macro fails the single-parameter arity gate even when + // its body is a single one-argument call. + let result = classify_passthrough( + "f", + &["a".to_string(), "b".to_string()], + &[], + &body_ast("f(a)"), + ); + assert_eq!( + result, None, + "a two-parameter macro cannot be a single-arg passthrough" + ); + } + + #[test] + fn classify_passthrough_non_collision_builtin_is_none() { + // `ABS = ABS(x)`: a single-param, bare-arg self-call, but `abs` is NOT a + // renamed-builtin collision (no dedicated opcode/stdlib-module routing + // reachable by the call-site fall-through), so the passthrough collapse + // would have nowhere valid to land -- must be None. + let result = classify_passthrough("abs", &["x".to_string()], &[], &body_ast("abs(x)")); + assert_eq!( + result, None, + "`abs` is not a renamed-builtin collision, so it is not opcode-backed \ + via the call-site fall-through" + ); + } + + #[test] + fn classify_passthrough_multi_output_macro_is_none() { + // A multi-output macro (additional outputs present from Vensim's + // `:`-list syntax) is NOT collapsible: its call site receives more than + // the primary output, so it must keep expanding as a module even if its + // primary-output body looks like a bare self-call. + let result = classify_passthrough( + "init", + &["x".to_string()], + &["secondary".to_string()], + &body_ast("init(x)"), + ); + assert_eq!( + result, None, + "a multi-output macro must not collapse to a single opcode" + ); + } + + #[test] + fn classify_passthrough_different_call_name_is_none() { + // The call must be a *self*-call (canonicalize(call) == + // canonicalize(macro_name)) -- the form the importer's rename produces. + // `INIT = PREVIOUS(x, 0)` is not a self-call (the macro is `init`, the + // call is `previous`), so it is not the renamed-builtin self-collapse + // case. NOTE: `previous(x, 0)` is a TWO-arg call, so this case is + // actually rejected at the single-argument gate (3) before gate (5) is + // reached; `classify_passthrough_single_arg_non_self_call_name_is_none` + // below exercises gate (5) in isolation. + let result = + classify_passthrough("init", &["x".to_string()], &[], &body_ast("previous(x, 0)")); + assert_eq!( + result, None, + "a call to a different builtin name than the macro is not a self-call \ + passthrough" + ); + } + + #[test] + fn classify_passthrough_single_arg_non_self_call_name_is_none() { + // Gate (5) -- the self-call-name check -- in isolation. A single-arg + // `previous(x)` body inside an `init`-named macro passes the arity gate + // (1/2), the single-call/single-arg gate (3), and the bare-arg gate (4), + // so the ONLY thing that can reject it is gate (5): + // canonicalize("previous") != canonicalize("init"). (`previous(x)` is one + // arg at the `Expr0` level; the unary->`previous(x, 0)` desugar happens + // later in `builtins_visitor`, not at parse time.) Without gate (5) this + // would mis-collapse a non-self-call macro onto the wrong opcode. + let result = + classify_passthrough("init", &["x".to_string()], &[], &body_ast("previous(x)")); + assert_eq!( + result, None, + "a single-arg call to a different builtin name than the macro must be \ + rejected at the self-call-name gate, not collapsed" + ); + } } diff --git a/src/simlin-engine/src/vdf/record_results.rs b/src/simlin-engine/src/vdf/record_results.rs index 324fcc35f..d6d735c2b 100644 --- a/src/simlin-engine/src/vdf/record_results.rs +++ b/src/simlin-engine/src/vdf/record_results.rs @@ -24,8 +24,8 @@ use std::collections::{HashMap, HashSet}; use super::{ - SYSTEM_NAMES, VdfFile, VdfRecord, VdfSection3Directory, VdfSection3DirectoryEntry, - is_lookupish_name, is_owner_ot_class_code, + SYSTEM_NAMES, VDF_SECTION6_OT_CODE_STOCK, VdfFile, VdfRecord, VdfSection3Directory, + VdfSection3DirectoryEntry, is_lookupish_name, is_owner_ot_class_code, }; use crate::common::{Canonical, Ident}; @@ -181,6 +181,16 @@ pub(super) fn decoded_record_spans( /// Result of identifying graphical-function descriptor records. /// +/// `descriptor_indices` are the records (by `rec_idx`) that must NOT be +/// emitted at their `f[11]`-as-OT-start slot. Two sub-cases: +/// - **Overlapping descriptors** are dropped entirely: their consuming owner +/// record exists separately in the same OT component and carries the series. +/// - **Standalone descriptors** (a lookup-only variable Vensim saves *only* as +/// a descriptor, no separate consumer-owner record) are re-bound: they +/// appear additionally in `rebinds` mapping `rec_idx -> forward-link OT` +/// (`lookup_record[f[11]].word[10]`), and the caller emits them there +/// instead of dropping them. +/// /// `used_f10_fallback` records when the descriptor peeling step had to /// resort to the highest-`f[10]` tie-break because the lexical /// lookup-def-name test was ambiguous (`Ref.vdf` is the canonical case). @@ -189,6 +199,9 @@ pub(super) fn decoded_record_spans( #[derive(Clone, Debug, Default)] pub(super) struct DescriptorIdentification { pub(super) descriptor_indices: HashSet, + /// `rec_idx -> forward-link OT` for standalone descriptors re-bound to + /// their evaluated-output OT. A subset of `descriptor_indices`. + pub(super) rebinds: HashMap, #[allow(dead_code)] pub(super) used_f10_fallback: bool, } @@ -357,12 +370,382 @@ pub(super) fn identify_descriptor_records( } } + // Standalone (non-overlapping) descriptors: a lookup-only variable Vensim + // saves only as a descriptor record. The overlap path above never sees it + // (it collides with nothing), so it would otherwise decode at its spurious + // `f[11]`-as-OT-start ghost slot. Recognise it and re-bind to its + // forward-link evaluated-output OT. + let lookup_word10: Vec = vdf + .section6_lookup_records() + .map(|recs| recs.iter().map(|r| r.ot_index()).collect()) + .unwrap_or_default(); + let class_codes = vdf.section6_ot_class_codes().unwrap_or_default(); + let f11_by_span: Vec = spans + .iter() + .map(|s| vdf.records[s.rec_idx].fields[11]) + .collect(); + let rebinds = standalone_descriptor_rebinds( + spans, + &f11_by_span, + &overlapping, + n_lookups, + &lookup_word10, + &class_codes, + vdf.offset_table_count, + ); + // A re-bound standalone descriptor is also a descriptor: it must not be + // emitted at its `f[11]`-as-OT-start slot. + descriptor_indices.extend(rebinds.keys().copied()); + DescriptorIdentification { descriptor_indices, + rebinds, used_f10_fallback, } } +/// Identify *standalone* (non-overlapping) graphical-function descriptor +/// records and compute their forward-link re-bind OT. +/// +/// `identify_descriptor_records` only peels descriptors that sit in an +/// overlapping OT component, because a descriptor that collides with a real +/// owner is recognised by the collision. A lookup-only variable that Vensim +/// saves *only* as a descriptor record (no separate consumer-owner record) +/// does not overlap anything, so it slips through as an owner and decodes at +/// its `f[11]`-as-OT-start slot -- a ghost stock slot holding `0`/garbage (see +/// `docs/design/vdf.md`, "Descriptor pruning"). Its real series is the +/// forward-linked evaluated-output OT `lookup_record[f[11]].word[10]`. +/// +/// This pure function (functional core) recognises such a record and returns +/// its `rec_idx -> forward OT` re-bind, gated to avoid disturbing legitimate +/// owners: +/// - the span is NOT in `overlapping` (the connected-component peeling path +/// owns the overlapping case); +/// - its `f[11]` (`f11_by_span[i]`) is a valid section-6 lookup-record index +/// (`< n_lookups`) -- the structural pre-condition for the forward link; +/// - its `f[11]`-as-OT-start slot (`span.start`) carries the **stock** class +/// code (`0x08`). A graphical-function/lookup variable is never a stock, so +/// landing on a stock slot is the spurious-owner telltale. A legitimate +/// scalar owner whose `f[11]` is coincidentally `< n_lookups` carries a +/// non-stock data code (`0x11` dynamic etc.) and is left untouched; +/// - the forward link `lookup_record[f[11]].word[10]` is a valid data OT +/// (`1 <= ot < ot_count` with an owner class code -- never Time/0). +/// +/// When the forward link is not a valid data OT (e.g. it points at Time/0, the +/// "no saved consumer" case), the record is NOT re-bound: it has no recoverable +/// series, and re-binding to Time would be worse than leaving it. Such records +/// remain a genuine residual. +fn standalone_descriptor_rebinds( + spans: &[DecodedRecordSpan], + f11_by_span: &[u32], + overlapping: &HashSet, + n_lookups: usize, + lookup_word10: &[usize], + class_codes: &[u8], + ot_count: usize, +) -> HashMap { + let mut rebinds = HashMap::new(); + for (i, span) in spans.iter().enumerate() { + if overlapping.contains(&i) { + continue; + } + // Scalar only. An arrayed descriptor re-bound to a single forward OT + // would scalarize and lose its element columns; the arrayed lookup-only + // case needs element-order info the VDF does not store on disk and is + // deferred (see `docs/design/vdf.md` and the C-LEARN residual plan). + if span.length() != 1 { + continue; + } + let f11 = match f11_by_span.get(i) { + Some(&v) => v as usize, + None => continue, + }; + // f[11] must be a valid section-6 lookup-record index. + if f11 >= n_lookups { + continue; + } + // The f[11]-as-OT-start slot must be a STOCK slot -- the spurious-owner + // telltale. (`span.start` is exactly the `f[11]`-as-OT-start.) + if class_codes.get(span.start).copied() != Some(VDF_SECTION6_OT_CODE_STOCK) { + continue; + } + // Resolve the forward link and require it be a valid data OT. + let fwd = match lookup_word10.get(f11) { + Some(&v) => v, + None => continue, + }; + if fwd == 0 || fwd >= ot_count { + continue; + } + let fwd_is_owner = class_codes + .get(fwd) + .copied() + .map(is_owner_ot_class_code) + .unwrap_or(false); + if !fwd_is_owner { + continue; + } + rebinds.insert(span.rec_idx, fwd); + } + rebinds +} + +#[cfg(test)] +mod standalone_descriptor_tests { + use super::*; + // `VDF_SECTION6_OT_CODE_STOCK` arrives via `use super::*`; the dynamic and + // Time codes are pulled in directly for the synthetic OT class arrays. + use crate::vdf::{VDF_SECTION6_OT_CODE_DYNAMIC, VDF_SECTION6_OT_CODE_TIME}; + + fn span(rec_idx: usize, name: &str, start: usize) -> DecodedRecordSpan { + // Scalar span (length 1). + span_with_len(rec_idx, name, start, 1) + } + + fn span_with_len(rec_idx: usize, name: &str, start: usize, len: usize) -> DecodedRecordSpan { + DecodedRecordSpan { + rec_idx, + name: name.to_string(), + start, + end: start + len, + sort_key: 0, + } + } + + /// A standalone graphical-function descriptor whose `f[11]` is a valid + /// section-6 lookup-record index and whose `f[11]`-as-OT-start lands on a + /// STOCK (0x08) ghost slot holding the wrong value must be re-bound to its + /// forward link `lookup_record[f[11]].word[10]`, not emitted at the ghost + /// slot. Reproduces the `Ref.vdf` standalone-lookup-only mis-decode on a + /// minimal synthetic record set (NOT keyed on any C-LEARN name). + #[test] + fn standalone_lookup_descriptor_rebinds_to_forward_link() { + // OT layout (class codes): 0=Time, 1=dynamic owner (the real GF output + // the descriptor must resolve to), 2=stock-coded GHOST slot the + // descriptor's f[11]-as-OT-start spuriously lands on. + let class_codes = [ + VDF_SECTION6_OT_CODE_TIME, // OT 0: Time + VDF_SECTION6_OT_CODE_DYNAMIC, // OT 1: real evaluated-output (forward link) + VDF_SECTION6_OT_CODE_STOCK, // OT 2: ghost stock slot + ]; + let ot_count = class_codes.len(); + + // Two lookup records. The descriptor's f[11] == 1 indexes lookup + // record[1], whose word[10] (evaluated-output OT) == 1. + let lookup_word10 = [9usize, 1usize]; + let n_lookups = lookup_word10.len(); + + // One standalone descriptor span: its f[11] == 1 (a valid lookup + // index), but as an OT-start it lands on OT 2 (the stock ghost). It is + // NOT in any overlap component. + let spans = [span(0, "Some Forcing graph", 2)]; + let f11_by_span = [1u32]; + let overlapping: HashSet = HashSet::new(); + + let rebinds = standalone_descriptor_rebinds( + &spans, + &f11_by_span, + &overlapping, + n_lookups, + &lookup_word10, + &class_codes, + ot_count, + ); + + // The descriptor (rec_idx 0) must be re-bound to forward OT 1, NOT left + // at its ghost f[11]-as-OT-start slot (OT 2). + assert_eq!(rebinds.get(&0).copied(), Some(1)); + } + + /// A legitimate scalar owner whose data lives at its `f[11]`-as-OT-start + /// slot (a DYNAMIC 0x11 slot) must NOT be re-bound, even if `f[11]` happens + /// to be a valid lookup index. This guards the two `Ref.vdf` + /// `*_conc_change_at_impact_year` owners (class 0x11) the fix must preserve. + #[test] + fn legit_dynamic_owner_with_small_f11_is_not_rebound() { + let class_codes = [ + VDF_SECTION6_OT_CODE_TIME, + VDF_SECTION6_OT_CODE_DYNAMIC, // OT 1: the owner's real data + ]; + let ot_count = class_codes.len(); + let lookup_word10 = [9usize, 9usize]; + let n_lookups = lookup_word10.len(); + // f[11] == 1 is both the owner's OT start (dynamic, holds its data) AND + // coincidentally a valid lookup index. It must stay an owner. + let spans = [span(0, "Some Concentration", 1)]; + let f11_by_span = [1u32]; + let overlapping: HashSet = HashSet::new(); + + let rebinds = standalone_descriptor_rebinds( + &spans, + &f11_by_span, + &overlapping, + n_lookups, + &lookup_word10, + &class_codes, + ot_count, + ); + assert!( + rebinds.is_empty(), + "a dynamic-coded owner must not be re-bound: {rebinds:?}" + ); + } + + /// Pins the STOCK-slot guard as the *sole* load-bearing reason a legitimate + /// dynamic owner is left untouched. + /// + /// `legit_dynamic_owner_with_small_f11_is_not_rebound` (above) also exercises + /// a dynamic owner, but there the forward link `lookup_word10[f11] == 9` is + /// out of OT range, so the "valid forward data OT" guard rejects the re-bind + /// *first* and the STOCK-slot guard never decides. This case constructs a + /// span where every *other* precondition passes -- it is non-overlapping, + /// scalar, its `f[11]` is a valid lookup index, and its forward link resolves + /// to a valid in-range owner OT -- so the ONLY condition standing between the + /// owner and a (wrong) re-bind is the STOCK-slot requirement: its + /// `f[11]`-as-OT-start lands on a DYNAMIC (0x11) slot, not a STOCK (0x08) + /// ghost. Removing or broadening that guard would flip this test to a + /// non-empty re-bind (verified by mutation), so it enforces the docstring + /// promise to preserve legitimate 0x11 owners. + #[test] + fn legit_dynamic_owner_blocked_only_by_stock_slot_guard() { + // OT layout: 0=Time, 1=the dynamic owner's own data slot (where its + // f[11]-as-OT-start lands -- DYNAMIC, NOT stock), 2=a second dynamic + // owner that the forward link resolves to (a valid in-range data OT). + let class_codes = [ + VDF_SECTION6_OT_CODE_TIME, + VDF_SECTION6_OT_CODE_DYNAMIC, // OT 1: owner's f[11]-as-OT-start slot + VDF_SECTION6_OT_CODE_DYNAMIC, // OT 2: a valid forward-link data OT + ]; + let ot_count = class_codes.len(); + // f[11] == 1 indexes lookup record[1], whose word[10] == 2 -- an + // in-range owner OT, so the forward-data-OT guards (1 <= fwd < ot_count + // and fwd is an owner class) BOTH pass. Only the STOCK-slot guard at + // span.start (OT 1, DYNAMIC) can reject. + let lookup_word10 = [9usize, 2usize]; + let n_lookups = lookup_word10.len(); + let spans = [span(0, "Some Dynamic Owner", 1)]; + let f11_by_span = [1u32]; + let overlapping: HashSet = HashSet::new(); + + let rebinds = standalone_descriptor_rebinds( + &spans, + &f11_by_span, + &overlapping, + n_lookups, + &lookup_word10, + &class_codes, + ot_count, + ); + assert!( + rebinds.is_empty(), + "a dynamic owner whose only disqualifier is the non-stock slot must \ + not be re-bound (the STOCK-slot guard is the sole gate here): {rebinds:?}" + ); + } + + /// A standalone descriptor whose forward link `word[10]` points at Time + /// (OT 0) has no valid evaluated-output OT; it must NOT be re-bound (Time is + /// never a data owner). Guards the `Ref Global Emissions ... LOOKUP` case. + #[test] + fn standalone_descriptor_with_time_forward_link_is_not_rebound() { + let class_codes = [ + VDF_SECTION6_OT_CODE_TIME, + VDF_SECTION6_OT_CODE_STOCK, // OT 1: ghost stock slot + ]; + let ot_count = class_codes.len(); + // lookup record[1].word[10] == 0 -> Time, an invalid evaluated output. + let lookup_word10 = [9usize, 0usize]; + let n_lookups = lookup_word10.len(); + let spans = [span(0, "Ref graph LOOKUP", 1)]; + let f11_by_span = [1u32]; + let overlapping: HashSet = HashSet::new(); + + let rebinds = standalone_descriptor_rebinds( + &spans, + &f11_by_span, + &overlapping, + n_lookups, + &lookup_word10, + &class_codes, + ot_count, + ); + assert!( + rebinds.is_empty(), + "a Time forward-link must not be re-bound: {rebinds:?}" + ); + } + + /// An OVERLAPPING descriptor (already handled by the connected-component + /// peeling path) must NOT be re-bound by the standalone path -- it is the + /// existing path's responsibility to drop it in favor of its colliding + /// consumer owner. + #[test] + fn overlapping_descriptor_is_left_to_the_component_path() { + let class_codes = [VDF_SECTION6_OT_CODE_TIME, VDF_SECTION6_OT_CODE_STOCK]; + let ot_count = class_codes.len(); + let lookup_word10 = [9usize, 1usize]; + let n_lookups = lookup_word10.len(); + let spans = [span(0, "Overlapping graph", 1)]; + let f11_by_span = [1u32]; + // Mark span 0 as overlapping. + let mut overlapping: HashSet = HashSet::new(); + overlapping.insert(0); + + let rebinds = standalone_descriptor_rebinds( + &spans, + &f11_by_span, + &overlapping, + n_lookups, + &lookup_word10, + &class_codes, + ot_count, + ); + assert!( + rebinds.is_empty(), + "an overlapping descriptor must be left to the component path: {rebinds:?}" + ); + } + + /// An ARRAYED descriptor (span length > 1) must NOT be re-bound by the + /// scalar standalone path: re-binding it to a single forward OT would + /// scalarize and lose its element columns. The arrayed lookup-only case is + /// deferred (it needs element-order info the VDF format does not store on + /// disk). This guards the deferred C-LEARN `rs_*` / `historical_*_lookup` + /// arrayed descriptors against accidental scalarization. + #[test] + fn arrayed_standalone_descriptor_is_not_rebound() { + // OT layout: 0=Time, 1=dynamic (forward link), 2..5 = stock ghost span. + let class_codes = [ + VDF_SECTION6_OT_CODE_TIME, + VDF_SECTION6_OT_CODE_DYNAMIC, + VDF_SECTION6_OT_CODE_STOCK, + VDF_SECTION6_OT_CODE_STOCK, + VDF_SECTION6_OT_CODE_STOCK, + ]; + let ot_count = class_codes.len(); + let lookup_word10 = [9usize, 1usize]; + let n_lookups = lookup_word10.len(); + // A 3-element arrayed descriptor (start 2, len 3), f[11] == 1. + let spans = [span_with_len(0, "RS arrayed graph", 2, 3)]; + let f11_by_span = [1u32]; + let overlapping: HashSet = HashSet::new(); + + let rebinds = standalone_descriptor_rebinds( + &spans, + &f11_by_span, + &overlapping, + n_lookups, + &lookup_word10, + &class_codes, + ot_count, + ); + assert!( + rebinds.is_empty(), + "an arrayed descriptor must not be scalar-re-bound: {rebinds:?}" + ); + } +} + /// A reconstructed result-emission candidate. A `RecordResultCandidate` /// covers exactly one OT-aligned span and binds it to one or more section-1 /// records. Multiple records can collapse onto the same span when several @@ -576,9 +959,57 @@ pub(super) fn build_record_result_columns( } } + // Emit standalone-descriptor re-binds last, each as one scalar column at + // its forward-link evaluated-output OT. These deliberately do NOT consult + // `claimed_ot`: the forward OT is frequently a *consumer* OT already owned + // by another variable (Vensim stores one evaluated series consumed under + // several names), and the lookup-only variable legitimately shares it. The + // names are distinct, so this adds a distinct column rather than a + // duplicate. Determinism: iterate in `rec_idx` order so a name that maps to + // multiple descriptor records resolves to the lowest `rec_idx` consistently. + emit_descriptor_rebinds(&spans, &desc_id.rebinds, &mut ordered); + ordered } +/// Emit re-bound standalone descriptors as scalar columns at their forward OT. +/// +/// One column per distinct canonical name (lowest `rec_idx` wins on the rare +/// duplicate-name case), pushed onto `ordered`. Skips a name already present +/// in `ordered` so a re-bind never shadows a real owner column of the same +/// canonical identity. +fn emit_descriptor_rebinds( + spans: &[DecodedRecordSpan], + rebinds: &HashMap, + ordered: &mut Vec<(Ident, usize)>, +) { + let span_by_rec: HashMap = + spans.iter().map(|s| (s.rec_idx, s)).collect(); + let already: HashSet> = ordered.iter().map(|(id, _)| id.clone()).collect(); + + let mut entries: Vec<(usize, usize)> = rebinds + .iter() + .map(|(&rec_idx, &fwd_ot)| (rec_idx, fwd_ot)) + .collect(); + entries.sort_unstable(); + + let mut emitted: HashSet> = HashSet::new(); + for (rec_idx, fwd_ot) in entries { + let Some(span) = span_by_rec.get(&rec_idx) else { + continue; + }; + let key = if span.name.starts_with('#') { + Ident::::from_str_unchecked(&span.name) + } else { + Ident::::new(&span.name) + }; + if already.contains(&key) || !emitted.insert(key.clone()) { + continue; + } + ordered.push((key, fwd_ot)); + } +} + /// Append one owner span's columns to `ordered`, marking the OT slots in /// `claimed_ot`. The span has already been validated as an owner record /// (post descriptor identification). Element labels are resolved via diff --git a/src/simlin-engine/src/vm.rs b/src/simlin-engine/src/vm.rs index 58bb75386..dfe2ece23 100644 --- a/src/simlin-engine/src/vm.rs +++ b/src/simlin-engine/src/vm.rs @@ -3098,6 +3098,23 @@ mod lookup_tests { assert_eq!(0.5, lookup(&table, 0.5)); assert_eq!(1.5, lookup(&table, 1.5)); } + + /// AC1.6 regression pin (#590): the regular (Interpolate-mode) `lookup` + /// clamps an out-of-range index to the endpoint y-value and returns NaN for + /// a NaN index. The lookup-only saved-value fix only changes *which* + /// expression is fed to a standalone graphical-function variable's table + /// (the index), never the table's clamp/NaN behavior -- this pins that the + /// clamp the fix relies on is unchanged. + #[test] + fn test_regular_lookup_outside_range_clamps_to_endpoints() { + let table = test_table(); // x in [0, 2], y = x + // Below the first x: clamp to the first y. + assert_eq!(0.0, lookup(&table, -5.0)); + // Above the last x: clamp to the last y. + assert_eq!(2.0, lookup(&table, 5.0)); + // NaN index: NaN result. + assert!(lookup(&table, f64::NAN).is_nan()); + } } #[cfg(test)] diff --git a/src/simlin-engine/tests/simulate.rs b/src/simlin-engine/tests/simulate.rs index c0335ccb3..7f950e1bc 100644 --- a/src/simlin-engine/tests/simulate.rs +++ b/src/simlin-engine/tests/simulate.rs @@ -503,6 +503,184 @@ fn synthetic_results(columns: &[(&str, Vec)]) -> Results { } } +/// AC4.5: a NaN-vs-`:NA:` series (Vensim writes literal IEEE NaN where Simlin +/// writes the finite `:NA:` sentinel `-2^109`, e.g. C-LEARN's +/// `slr_inches_from_2000`) must be handled by the comparator's NaN-skip path +/// and must NOT enter the failure set, so it never needs an +/// `EXPECTED_VDF_RESIDUAL` entry. This pins that contract on a SYNTHETIC series +/// (no C-LEARN parse, no C-LEARN name), so it is independent of the hero model: +/// - every cell whose VDF side is IEEE NaN is counted `nan_skipped`, never +/// `failures` (the `:NA:`-sentinel SIM value is irrelevant -- the NaN guard +/// fires first); +/// - the late finite steps that DO match within tolerance are `compared` with +/// zero `failures`; +/// - because at least one finite step is compared, the series is NOT `all_nan`. +/// +/// Together (`failures == 0 && !all_nan`) means `clearn_residual_exactness`'s +/// membership predicate (`failures > 0 || all_nan`) is false, so such a series +/// is never added to the residual set. (An ENTIRELY-NaN core series is a +/// different, separately-guarded degeneracy -- see the all-NaN assertions in +/// `ensure_vdf_results_excluding` and scenario (2) of the vacuous-comparison +/// test; the realistic NaN-vs-`:NA:` case has finite tail steps and is partial.) +#[test] +fn classify_vdf_ident_nan_vs_na_skips_without_failing() { + let na = simlin_engine::float::NA; + // `float::NA` is the finite Vensim `:NA:` sentinel, NOT IEEE NaN. + assert!(na.is_finite(), "the :NA: sentinel must be finite, not NaN"); + assert!( + (na - (-(2.0_f64).powi(109))).abs() < 1e18, + ":NA: sentinel is -2^109" + ); + + // Model `slr_inches_from_2000`: early steps are NaN on the VDF side (Vensim + // literal IEEE NaN) while SIM carries the finite `:NA:` sentinel; the late + // steps are finite and match within the 1% tolerance. + let vdf = synthetic_results(&[( + "synthetic_nan_na_series", + vec![f64::NAN, f64::NAN, f64::NAN, 2.5, 5.0, 7.5], + )]); + let sim = synthetic_results(&[( + "synthetic_nan_na_series", + // Where VDF is NaN, SIM is the finite `:NA:` sentinel; where VDF is + // finite, SIM matches it (within 1%). + vec![na, na, na, 2.5, 5.0, 7.5], + )]); + let vdf_off = vdf.offsets[&simlin_engine::common::Ident::new("synthetic_nan_na_series")]; + let sim_off = sim.offsets[&simlin_engine::common::Ident::new("synthetic_nan_na_series")]; + + let stats = classify_vdf_ident(&vdf, &sim, vdf_off, sim_off); + + // The three NaN-on-VDF-side cells are skipped (the `:NA:` SIM value never + // reaches the tolerance check); the three finite matching cells compare clean. + assert_eq!( + stats.nan_skipped, 3, + "the NaN-on-VDF cells must be NaN-skipped" + ); + assert_eq!(stats.compared, 3, "the finite tail cells must be compared"); + assert_eq!( + stats.failures, 0, + "a NaN-vs-:NA: series with matching finite tail must produce NO failures" + ); + assert!( + !stats.all_nan, + "a partial-NaN series (finite tail) is NOT all-NaN" + ); + // No spurious `:NA:` reconciliation: the SIM `:NA:` cells were NaN-skipped + // (VDF NaN), so they never reach the `:NA:`-sentinel reconciliation branch. + assert_eq!(stats.na_reconciled, 0); + + // The membership predicate `clearn_residual_exactness` uses to build the + // live residual set: this series is excluded from it, so it never needs an + // `EXPECTED_VDF_RESIDUAL` carve-out (AC4.5). The exclusion holds *because* + // the series has a finite tail (like `slr_inches_from_2000`); the test name + // names only this partial-NaN / finite-tail case, NOT the general + // NaN-vs-:NA: contract. + let enters_residual = stats.failures > 0 || stats.all_nan; + assert!( + !enters_residual, + "a partial-NaN (finite-tail) NaN-vs-:NA: series must NOT enter the residual failure set" + ); + + // Boundary made executable: an ENTIRELY-NaN VDF series (Vensim literal NaN + // at every step) is the separately-guarded exception. With no finite cell + // to compare, `all_nan` is set and the SAME membership predicate flips true, + // so such a series WOULD enter the residual set (it is handled by the + // all-NaN guards in `ensure_vdf_results_excluding`, not by this finite-tail + // skip path). This pins exactly why the test's scope is the finite-tail case. + let all_nan_vdf = synthetic_results(&[( + "synthetic_all_nan_series", + vec![f64::NAN, f64::NAN, f64::NAN, f64::NAN, f64::NAN, f64::NAN], + )]); + let all_na_sim = + synthetic_results(&[("synthetic_all_nan_series", vec![na, na, na, na, na, na])]); + let all_nan_off = + all_nan_vdf.offsets[&simlin_engine::common::Ident::new("synthetic_all_nan_series")]; + let all_na_sim_off = + all_na_sim.offsets[&simlin_engine::common::Ident::new("synthetic_all_nan_series")]; + let all_nan_stats = classify_vdf_ident(&all_nan_vdf, &all_na_sim, all_nan_off, all_na_sim_off); + assert!( + all_nan_stats.all_nan, + "an entirely-NaN VDF series must be flagged all_nan (the separately-guarded exception)" + ); + assert!( + all_nan_stats.failures > 0 || all_nan_stats.all_nan, + "an all-NaN series WOULD enter the residual set -- it is NOT covered by the finite-tail skip" + ); +} + +/// AC4.4 guard: the C-LEARN VDF carve-out (`EXPECTED_VDF_RESIDUAL`) is a +/// documented membership EXCLUSION, never a tolerance loosening. This pins the +/// five comparator constants to their exact values AND proves the matched-variable +/// floor still PANICS when too few variables match (so the gate -- which checks +/// the floor AFTER exclusion -- can never pass vacuously by carving the matched +/// set below the floor). If a future change loosens any constant or weakens the +/// floor, this test fails before any C-LEARN gate is even run. Uses synthetic +/// inputs only (no C-LEARN parse, no C-LEARN name). +#[test] +fn vdf_comparator_constants_and_floor_are_pinned() { + // 1. Pin the five comparator constants (AC4.4): a drift in any of these is a + // silent tolerance/floor change that must be a deliberate, reviewed edit. + assert_eq!(VDF_RTOL, 0.01, "the 1% cross-simulator tolerance is fixed"); + assert_eq!( + K_ATOL, 1e-4, + "the per-series absolute-floor coefficient is fixed" + ); + assert_eq!( + MIN_MATCHED_FRACTION, 0.10, + "the matched-fraction floor is fixed" + ); + assert_eq!( + MIN_MATCHED_ABSOLUTE, 10, + "the absolute matched floor is fixed" + ); + assert_eq!( + MAX_NAN_SKIPPED_FRACTION, 0.10, + "the global NaN-skipped ceiling is fixed" + ); + + // 2. The floor formula itself is `max(absolute, fraction * count)`. + assert_eq!( + min_matched(16), + 10, + "16 vars: absolute floor (10) dominates" + ); + assert_eq!( + min_matched(5000), + 500, + "5000 vars: fractional floor (10%) dominates" + ); + + // 3. The gate cannot pass vacuously: a comparison whose matched count is + // below the floor must PANIC even though every matched cell is in + // tolerance (the legacy `failures == 0` would pass it). 16 reference + // variables (floor = 10) with only ONE shared, in-tolerance ident. + let names: &[&str] = &[ + "v00", "v01", "v02", "v03", "v04", "v05", "v06", "v07", "v08", "v09", "v10", "v11", "v12", + "v13", "v14", "v15", + ]; + let expected: Vec<(&str, Vec)> = names.iter().map(|n| (*n, vec![1.0, 1.0, 1.0])).collect(); + let sim_below: Vec<(&str, Vec)> = vec![("v00", vec![1.0, 1.0, 1.0])]; + let e = synthetic_results(&expected); + let a = synthetic_results(&sim_below); + let prev_hook = std::panic::take_hook(); + std::panic::set_hook(Box::new(|_| {})); + let below = std::panic::catch_unwind(|| ensure_vdf_results(&e, &a)); + // A control: enough matched vars, all in tolerance, must NOT panic -- proving + // the panic above is the FLOOR firing, not an unrelated failure. + let a_ok = synthetic_results(&expected); + let e_ok = synthetic_results(&expected); + let ok = std::panic::catch_unwind(|| ensure_vdf_results(&e_ok, &a_ok)); + std::panic::set_hook(prev_hook); + assert!( + below.is_err(), + "a below-floor comparison (1 of 16 matched) must PANIC, not pass vacuously" + ); + assert!( + ok.is_ok(), + "an above-floor, in-tolerance comparison must pass (control)" + ); +} + /// AC8.2: the hardened `ensure_vdf_results` must FAIL (panic), rather than /// vacuously pass, on a near-empty or degenerate comparison, while the /// user-directed comparator reconciles the legitimate `:NA:`-sentinel and @@ -1539,19 +1717,24 @@ fn simulates_wrld3_03() { /// Known-residual C-LEARN base-variable names excluded from the /// `simulates_clearn` VDF gate. C-LEARN compiles via the incremental path, /// runs to FINAL TIME, and matches `Ref.vdf` within the 1% cross-simulator -/// tolerance on ~94.7% of matched idents (3298 of 3482; equivalently ~96.3% -/// measured per-cell — the per-cell rate the plan's `test-requirements.md` -/// quotes) after the per-element-GF fix (`61573545`) and the `:NA:`-aware + -/// near-zero-robust comparator (`8775ae97`). The user-directed decision was to BANK that match and TRACK the -/// genuine residual rather than chase it to within 1%. +/// tolerance on the overwhelming majority of its 3482 matched idents; the +/// short, fully-attributed remainder below (21 base variables) is the proven +/// genuine residual after Phases 1-3 of the C-LEARN-residual plan. Phases 1-3 +/// reconciled the bulk that earlier sat here: the inline-data graph-lookup +/// `0+0` zeroing (#590, fixed Phase 1 -- lookup-only lowers to `gf(Time)`), +/// RAMP FROM TO import linearization (Phase 2), the passthrough-INIT + +/// dt-runlist ordering (Phase 3), and the init-runlist nondeterminism +/// (`e24b0080`). The user-directed decision was to BANK that match and TRACK +/// the residual rather than chase the rest to within 1%. /// /// This is a TRANSPARENT, documented, tracked carve-out, NOT a tolerance /// loosening: the hard 1% comparison stays unconditional for every NON-excluded -/// variable, and the matched floor is checked AFTER exclusion (3298 matched vs a -/// 348 floor, ~9.5x -- the exclusion cannot create a vacuous pass). Each base -/// below was enumerated by running C-LEARN through the exact `classify_vdf_ident` -/// comparator logic and collecting the bases with >=1 failing cell; every base -/// maps to a tracked cluster in GH #590 or #591. +/// variable, and the matched floor is checked AFTER exclusion (3360 matched vs a +/// 348 floor, ~9.7x -- the exclusion cannot create a vacuous pass; pinned by +/// `vdf_comparator_constants_and_floor_are_pinned`). Each base below carries a +/// one-line sourced reason under one of five categories +/// (engine-genuine-tracked / VDF-decode-artifact / benign-near-zero / +/// NaN-vs-`:NA:` / boundary); none is "unknown". /// /// This set is GUARDED for exactness by the committed `clearn_residual_exactness` /// regression test (run it to re-derive/verify after an engine change: @@ -1561,64 +1744,63 @@ fn simulates_wrld3_03() { /// (with the symmetric difference) if the residual grew (a regression) or shrank /// (a fix that should prune the exclusion). const EXPECTED_VDF_RESIDUAL: &[&str] = &[ - // -- GH #590: data / graph-lookup variables import as a literal `0+0` - // (or per-element `0+0 | 0+0 | ...`) stub instead of their lookup/data - // values, so the whole variable is zeroed (relative error 1.0). The - // dominant residual cluster. - "global_emissions_from_graph_lookup", - "ref_global_emissions_from_graph_lookup", - "historical_forestry_lookup", - "historical_gdp_lookup", - "oc,_bc,_and_bio_aerosol_forcings", - "other_forcings_composite_plus_rcp85", - "other_forcings_smooth_plus_rcp85", - "ozone_precursor_forcings", - "rs_ch4", - "rs_hfc125", - "rs_hfc134a", - "rs_hfc143a", - "rs_hfc152a", - "rs_hfc227ea", - "rs_hfc23", - "rs_hfc245ca", - "rs_hfc32", - // -- GH #591 cluster 1: SAMPLE UNTIL / INIT-of-`:NA:` / target-policy - // (COP dimension). A `:NA:`-arithmetic edge and/or SAMPLE UNTIL - // semantics where Vensim performs NA-arithmetic (e.g. `-2*NA` = - // -1.298e33) while Simlin yields a bare `:NA:` sentinel (-6.49e32) or 0. - // `historical_gdp` (the bare twin of the #590 `_lookup`) is here, not - // #590: its non-`:NA:` cells match exactly; only its `-2*NA` cells - // diverge (vdf -1.298e33 vs sim -6.49e32 sentinel). - "last_set_target_year", - "last_active_target_year", - "time_from_target_to_ultimate_target", - "target_emissions_for_rate", - "ultimate_target_value_from_rate", - "depth_at_bottom", - "emissions_with_stopped_growth", - "historical_gdp", - // -- GH #591 cluster 2: genuine numeric tail (meaningful-value divergence, - // not near-zero noise) on the emissions and ocean-diffusion chains. The - // `im_3_*` / `relative_emissions_*` group diverges by rel ~0.247 on - // substantial values; `co2eq_gap_closing_percentage` is a near-zero-ratio - // percentage (large rel, tiny absolute ~1.4e-3); `diffusion_flux` is the - // ocean-diffusion physical variable feeding the SLR sub-model, diverging - // on small magnitudes (vdf 0 vs sim ~1e-8, and ~0.5 rel on ~1e-5 cells) - // -- a member discovered by the exhaustive measurement beyond #591's - // representative list. - "im_3_emissions", - "im_3_emissions_vs_rs", - "im_3_ff_co2", - "relative_emissions_to_equity", - "relative_emissions_to_equity_target", - "co2eq_gap_closing_percentage", - "diffusion_flux", - // -- GH #591 cluster 3 (other-NaN: Vensim writes literal IEEE NaN while - // Simlin writes the `:NA:` sentinel, e.g. `slr_inches_from_2000`) needs - // NO entry here: the comparator NaN-skips those cells (NaN on either - // side is skipped, not failed) and the late finite steps match within - // tolerance, so no `slr_inches_from_2000`-style base reaches the failure - // set. It is tracked in #591 for the representation gap, not excluded. + // ===== engine-genuine-tracked (0): NONE ===== + // The only engine-genuine divergence -- the init-runlist nondeterminism + // (which permuted `depth_at_bottom`'s per-layer init values) -- was fixed in + // `e24b0080` (deterministic init runlist). `clearn_residual_exactness` now + // runs deterministically, and no base below is the engine producing a wrong + // value on a meaningful magnitude. (#595 tracks the remaining deeper init- + // ordering soundness gap; its nondeterminism half is fixed by `e24b0080`.) + + // ===== VDF-decode-artifact (17): engine output proven CORRECT; the ===== + // `Ref.vdf` REFERENCE column is mis-decoded by our reader. For every base + // here the engine's `gf(Time)` matches the model tables and applied + // consumers match `Ref.vdf` exactly; the divergence is entirely in the + // reference-side decode of standalone graphical-function ("lookup-only") + // descriptor columns. The SCALAR half was fixed in `d69754bc`; the ARRAYED + // element-order half and the 4 scalar no-distinct-column cases are tracked + // in #597 (which supersedes #590's "engine 0+0" framing -- the engine is + // correct, the reader is not). + // + // -- Arrayed descriptor block, element-order mis-decode (#597) -- + "historical_forestry_lookup", // VDF col mis-decoded; engine gf(Time) correct. + "historical_gdp_lookup", // e.g. [g77_india] vdf 2.6e-4 vs engine 1.43e5 (correct). + "rs_ch4", // VDF descriptor col decodes to all-zero ghost; engine correct. + "rs_co2_ff", // arrayed lookup-only; VDF col mis-decoded, engine correct. + "rs_gdp_in_trillions", // arrayed lookup-only; VDF col mis-decoded, engine correct. + "rs_hfc125", // arrayed lookup-only; VDF col mis-decoded, engine correct. + "rs_hfc134a", // arrayed lookup-only; VDF col mis-decoded, engine correct. + "rs_hfc143a", // arrayed lookup-only; VDF col mis-decoded, engine correct. + "rs_hfc152a", // arrayed lookup-only; VDF col mis-decoded, engine correct. + "rs_hfc227ea", // arrayed lookup-only; VDF col mis-decoded, engine correct. + "rs_hfc23", // arrayed lookup-only; VDF col mis-decoded, engine correct. + "rs_hfc245ca", // arrayed lookup-only; VDF col mis-decoded, engine correct. + "rs_hfc32", // arrayed lookup-only; VDF col mis-decoded, engine correct. + // -- Scalar descriptor with NO distinct saved VDF column (#597) -- + "ref_global_emissions_from_graph_lookup", // descriptor forward-links to Time; no distinct col. + "ozone_precursor_forcings", // forward-links to a shared "other forcings" OT (>1% off). + "oc,_bc,_and_bio_aerosol_forcings", // forward-links to the same shared OT; no distinct col. + "other_forcings_smooth_plus_rcp85", // shared-OT forward link; only 131/251 cells match. + // ===== benign-near-zero (2): divergence ONLY on near-zero magnitudes ===== + // (cross-simulator f32/integration noise), never on a meaningful value. + // Tracked in #591 cluster 2. + "co2eq_gap_closing_percentage", // ratio of near-equal small values; peak ~1.1e-2 (vdf -6.8e-4 vs sim 6.9e-4). + "diffusion_flux", // ~2% on a small early transient (~7% of cells, vdf 0 vs sim ~5e-10); matches late. + // ===== NaN-vs-:NA: (0): NONE in this list (by construction) ===== + // Where Vensim writes literal IEEE NaN and Simlin writes the finite `:NA:` + // sentinel (`-2^109`, e.g. `slr_inches_from_2000`), the comparator NaN-skips + // those cells and the finite tail matches within tolerance, so the series is + // neither all-NaN nor has failing cells and never reaches the failure set. + // Pinned independently by `classify_vdf_ident_nan_vs_na_skips_without_failing`. + // Tracked in #591 for the representation gap, not excluded here. + + // ===== boundary (2): match everywhere except :NA:-arithmetic cells ===== + // Vensim computes `-2*NA = -1.298e33` while Simlin keeps the bare `:NA:` + // sentinel `-6.49e32` (`crate::float::NA`); NA-arithmetic is confirmed + // CORRECT and explicitly out of scope (see `float::NA`). The genuine + // documented remainder, tracked in #591 cluster 1. + "historical_gdp", // non-:NA: cells match exactly (e.g. [oecd_us] 6.667e4); only -2*NA cells diverge. + "last_set_target_year", // INIT(VECTOR SELECT(...)); every cell is -2*NA (vdf -1.298e33 vs sim -6.49e32 sentinel). ]; // FULL end-to-end C-LEARN simulation against `Ref.vdf`. Un-stubbed (no longer a @@ -1641,22 +1823,26 @@ const EXPECTED_VDF_RESIDUAL: &[&str] = &[ // `DoesNotExist` blockers (incl. the `"goal 1.5 for temperature"` quoted- // period ident, #559) are likewise cleared. // -// What remains is a genuine NUMERIC residual (~3.7% of cells), explicitly -// excluded via `EXPECTED_VDF_RESIDUAL` and tracked in #590 (the dominant `0+0` -// data/graph-lookup-import cluster) and #591 (SAMPLE UNTIL / INIT-`:NA:` / -// NA-arithmetic, the `im_3_*` numeric tail, the ocean-diffusion tail, and the -// NaN-vs-`:NA:` representation gap). The exclusion is a transparent, documented, -// tracked carve-out -- NOT a tolerance loosening; the hard 1% gate holds for -// every non-excluded variable and the matched floor is checked after exclusion. -// Run with: cargo test --release -- --ignored simulates_clearn +// What remains is a short, fully-attributed residual of 21 base variables +// (Phases 1-3 reconciled the rest), explicitly excluded via +// `EXPECTED_VDF_RESIDUAL` under a five-category taxonomy: 17 VDF-reader +// decode-artifacts where the engine is PROVEN correct (#597, supersedes #590's +// "engine 0+0" framing -- the #590 engine bug is fixed), 2 benign-near-zero, +// and 2 `:NA:`-arithmetic boundary cells (#591); the engine-genuine and +// NaN-vs-`:NA:` categories are empty (the sole engine-genuine divergence, init- +// runlist nondeterminism, was fixed in `e24b0080`). The exclusion is a +// transparent, documented, tracked carve-out -- NOT a tolerance loosening; the +// hard 1% gate holds for every non-excluded variable and the matched floor is +// checked after exclusion. Run with: cargo test --release -- --ignored simulates_clearn #[test] #[ignore] fn simulates_clearn() { let (vdf_results, results) = run_clearn_vs_vdf(); // Hard 1% gate for every NON-excluded variable; the documented, tracked - // EXPECTED_VDF_RESIDUAL bases (#590 / #591) are skipped. The matched floor - // is enforced AFTER exclusion, so this cannot vacuously pass. + // EXPECTED_VDF_RESIDUAL bases (#597 reader-artifact / #591 residual) are + // skipped. The matched floor is enforced AFTER exclusion, so this cannot + // vacuously pass. ensure_vdf_results_excluding(&vdf_results, &results, EXPECTED_VDF_RESIDUAL); } @@ -2498,6 +2684,33 @@ fn helper_recurrence_mdl_synthetic_helper_in_scc_simulates() { simulate_mdl_path(path); } +/// clearn-residual.AC3.2: an element-wise `INITIAL` recurrence routed through a +/// trivial passthrough macro (`:MACRO: INIT(x) = INITIAL(x)`) produces the same +/// correct per-element values as the proven bare-`INITIAL` opcode path +/// (`helper_recurrence_mdl_synthetic_helper_in_scc_simulates` above). +/// +/// The `macro_init_recurrence` fixture is `helper_recurrence.mdl` with the +/// `:MACRO: INIT(x) = INITIAL(x)` block PREPENDED. The recurrence text +/// (`ecc[tNext] = INITIAL(ecc[tPrev] * 2)`) is unchanged: its `INITIAL(...)` is +/// renamed to `INIT(...)` at import and now collides with the macro, so the +/// recurrence routes through the macro instead of the `LoadInitial` opcode. +/// +/// Expected (identical to `helper_recurrence`): `ecc[t1]=1, ecc[t2]=2, +/// ecc[t3]=4`, constant across both saved steps (INITIAL TIME=0, FINAL TIME=1, +/// TIME STEP=1 => 2 steps). `simulate_mdl_path` compares against the sibling +/// `macro_init_recurrence.dat`, which asserts the user-facing `ecc[..]` series +/// (`ensure_results` skips the implicit `$\u{205A}` module vars). +/// +/// RED before the Phase 3 Task 4 call-site collapse: the INIT-macro collision +/// routes the recurrence through the buggy per-element synthetic module, so the +/// recurrence drops to 0/`:NA:` at t>=1 rather than holding 1/2/4. +#[test] +fn simulates_passthrough_init_macro_element_recurrence() { + simulate_mdl_path( + "../../test/test-models/tests/macro_init_recurrence/macro_init_recurrence.mdl", + ); +} + /// element-cycle-resolution.AC2.5 (Phase 2 Task 9, the transitioned /// inter-variable-cycle assertion). `ref.mdl` and `interleaved.mdl` are /// INTER-variable element-acyclic recurrence SCCs (`ce`<->`ecc` / @@ -3338,6 +3551,82 @@ TIME STEP = 1 ~~| } } +/// clearn-residual.AC3.1: a scalar `INITIAL` capture routed through a trivial +/// passthrough macro (`:MACRO: INIT(x) = INITIAL(x)`) holds its value constant +/// across every saved step. +/// +/// The MDL importer renames Vensim `INITIAL` -> `INIT`, so `captured = +/// INIT(growing)` -- written against the `INIT` macro -- and the macro body +/// `INIT = INITIAL(x)` (stored as `init = init(x)`) collide. `INITIAL(growing)` +/// where `growing = Time` captures `INITIAL TIME` at t0, so `captured` MUST be +/// the constant `INITIAL TIME` (here 2) at every saved step. +/// +/// Control window: INITIAL TIME=2, FINAL TIME=6, TIME STEP=1, SAVEPER=1 => 5 +/// saved steps (t=2,3,4,5,6). `growing` rises 2,3,4,5,6 while `captured` stays +/// pinned at 2 -- the AC3.1 user-facing invariant (a held-constant INITIAL +/// capture, distinguishable from a value that drifts with `growing`). +/// +/// This test asserts only the AC3.1 invariant. It is NOT a value RED->GREEN +/// discriminator for the Task 4 call-site collapse: the pre-collapse +/// synthetic-module path already produces the constant `[2,2,2,2,2]` for a +/// scalar *non-recurrence* INITIAL-capture (the per-element ordering bug the +/// collapse fixes does not affect this scalar case). The genuine value +/// RED->GREEN discriminator is the element-wise +/// `simulates_passthrough_init_macro_element_recurrence` (AC3.2), where the +/// synthetic-module routing produces wrong per-element values (drops to +/// 0/`:NA:` at t>=1) before the collapse. +#[test] +fn simulates_passthrough_init_macro_scalar_capture_is_constant() { + // The importer renames Vensim INITIAL to INIT, so BOTH the macro body + // (`INIT = INITIAL(x)` -> `init = init(x)`) AND the caller's + // `captured = INITIAL(growing)` (-> `init(growing)`) collide with the + // like-named renamed builtin -- the #591-c1 shape. The caller must write + // the genuine Vensim builtin `INITIAL(...)` (which the importer recognizes + // and renames), NOT the post-rename `INIT(...)`: a single-argument + // `INIT(arg)` written directly would hit the MDL importer's + // 1-arg-call->LOOKUP heuristic (GH #553) and never reach macro resolution. + let mdl = "\ +{UTF-8} +:MACRO: INIT(x) +INIT = INITIAL(x) + ~ passthrough macro + ~ collides with renamed builtin + | +:END OF MACRO: +growing = Time ~~| +captured = INITIAL(growing) ~~| +INITIAL TIME = 2 ~~| +FINAL TIME = 6 ~~| +SAVEPER = 1 ~~| +TIME STEP = 1 ~~| +"; + let results = run_inline_mdl(mdl); + + // INITIAL TIME = 2; growing = Time; captured = INITIAL(growing) frozen at t0. + let expected_growing = [2.0, 3.0, 4.0, 5.0, 6.0]; + let captured = element_series(&results, "captured"); + assert_eq!( + captured.len(), + expected_growing.len(), + "expected 5 saved steps (t=2..6), got {}", + captured.len() + ); + for (step, &g) in expected_growing.iter().enumerate() { + let grew = macro_test_value_at(&results, "growing", step); + assert!( + (grew - g).abs() < 1e-9, + "sanity: growing must equal Time ({g}) at step {step}, got {grew}" + ); + // The capture is INITIAL(growing) = INITIAL TIME = 2 at EVERY step. + assert!( + (captured[step] - 2.0).abs() < 1e-9, + "captured = INIT(growing) must be held constant at INITIAL TIME (2) \ + for every saved step; at step {step} (Time={g}) got {}", + captured[step] + ); + } +} + /// macros.AC2.4: a macro invoked with an expression-valued argument /// (`y = M(a + b, t)`) -- the argument is evaluated in the caller's context. /// @@ -4403,3 +4692,92 @@ fn compiles_and_runs_clearn_structural() { all_nan.len() ); } + +/// Regression for the synthetic-module flows-phase ordering bug behind +/// C-LEARN's `emissions_with_stopped_growth` (#591-c1): an arrayed +/// `SAMPLE IF TRUE(cond, SMOOTH(input, dt), init)` whose SMOOTH `input` has a +/// CURRENT-value (dt-phase) dependency back on the variable itself. +/// +/// The desugar is `captured[e] = IF cond THEN smth1·output ELSE PREVIOUS(self, +/// init)`. With the always-true condition the SMOOTH branch is taken every +/// step; smoothing a constant yields that constant, so every element must hold +/// its per-element constant (`base[e]`) at every saved step. +/// +/// The flows-phase cycle `captured -> smth1·output -> smth1(module) -> input -> +/// captured` is hidden from CYCLE DETECTION because a module is a sink in the +/// cycle relation (`dt_walk_successors`), so the model compiles (no +/// `CircularDependency`). But `captured` reads the SMOOTH *stock* output, which +/// in the dt phase is a prior-timestep read and must NOT impose a same-step +/// ordering on the module: before the fix, the salsa runlist +/// (`db_dep_graph::build_var_info` / `topo_sort_str`) kept a `captured -> +/// smth1` dt edge anyway (the `·output` stock dep was not chain-broken for a +/// NON-module reader), creating a false ordering cycle that `topo_sort_str` +/// broke by emitting the SMOOTH module BEFORE its `input` for SOME elements +/// (the broken element is HashMap-iteration-order-dependent, hence the bug was +/// also nondeterministic). Those elements then read a stale (0) input each +/// flows step and decayed to 0 at t>=1 while holding the correct value at t0 -- +/// exactly the C-LEARN `emissions_with_stopped_growth[cop_developing_b]` +/// symptom (vdf constant, sim drops to 0 at t>=1). The fix chain-breaks a stock +/// submodel-output dep for every reader in the dt phase (mirroring the legacy +/// `model.rs::module_output_deps` `!output_var.is_stock()` gate), so no false +/// ordering cycle forms and every element is ordered input-before-module. +/// +/// The `input` is a separate `feedback[cop] = ACTIVE INITIAL(captured*0 + base, +/// base)`, exactly C-LEARN's shape (its `CO2 FF emissions = ACTIVE INITIAL(..., +/// RS CO2 FF(...))`): the dt (active) branch carries the feedback on `captured` +/// that triggers the flows-phase bug, while the init branch's deps are only +/// `base` -- so there is NO init-time algebraic cycle (C-LEARN's emissions has +/// none either, which is why its t0 always matched). `captured*0` keeps the +/// numeric value `base` while preserving the structural dt dependency. `apply` +/// gates the SMOOTH input through an `IF THEN ELSE` so a hoisted per-element +/// `arg0` helper is synthesized (the exact C-LEARN shape, where the mis-ordered +/// node was the `arg0` helper feeding the module). Seven elements (a COP-like +/// dimension) make the order-dependent break observable. +#[test] +fn synthetic_module_feedback_input_ordered_before_module() { + let mdl = "\ +{UTF-8} +cop: e0, e1, e2, e3, e4, e5, e6 ~~| +apply= 2 ~~| +base[cop]= 100,200,300,400,500,600,700 ~~| +feedback[cop]= ACTIVE INITIAL(captured[cop]*0 + base[cop], base[cop]) ~~| +captured[cop]= SAMPLE IF TRUE(Time <= 100, SMOOTH(IF THEN ELSE(apply=1, 0, feedback[cop]), TIME STEP), 0) ~~| +INITIAL TIME = 0 ~~| +FINAL TIME = 4 ~~| +SAVEPER = 1 ~~| +TIME STEP = 1 ~~| + +\\\\\\---/// Sketch information - do not modify anything except names +V300 Do not put anything below this section - it will be ignored +*View 1 +$192-192-192,0,Times New Roman|12||0-0-0|0-0-0|0-0-255|-1--1--1|-1--1--1|72,72,100,0 +10,1,base,150,165,28,8,8,3,0,0,0,0,0,0 +10,2,captured,150,200,28,8,8,3,0,0,0,0,0,0 +///---\\\\\\ +:L<%^E!@ +1:Current.vdf +"; + let results = run_inline_mdl(mdl); + // Each element must hold its per-element constant base[e] across every + // saved step (smoothing a constant). A mis-ordered module reads a stale 0 + // input and decays to 0 at t>=1 (correct only at t0). + let elems = ["e0", "e1", "e2", "e3", "e4", "e5", "e6"]; + let expected = [100.0, 200.0, 300.0, 400.0, 500.0, 600.0, 700.0]; + for (e, want) in elems.iter().zip(expected.iter()) { + let series = element_series(&results, &format!("captured[{e}]")); + assert_eq!( + series.len(), + results.step_count, + "captured[{e}] should have one value per saved step" + ); + for (step, &got) in series.iter().enumerate() { + assert!( + (got - want).abs() < 1e-9, + "captured[{e}] must hold the smoothed constant {want} at EVERY \ + saved step (the SMOOTH input must be ordered before its module \ + each flows step); at step {step} got {got}, full series \ + {series:?}" + ); + } + } +} diff --git a/test/test-models/tests/macro_init_recurrence/macro_init_recurrence.dat b/test/test-models/tests/macro_init_recurrence/macro_init_recurrence.dat new file mode 100644 index 000000000..1687013ea --- /dev/null +++ b/test/test-models/tests/macro_init_recurrence/macro_init_recurrence.dat @@ -0,0 +1,17 @@ +ecc[t1] +0 1 +ecc[t2] +0 2 +1 2 +ecc[t3] +0 4 +1 4 +FINAL TIME +0 1 +INITIAL TIME +0 0 +SAVEPER +0 1 +1 1 +TIME STEP +0 1 diff --git a/test/test-models/tests/macro_init_recurrence/macro_init_recurrence.mdl b/test/test-models/tests/macro_init_recurrence/macro_init_recurrence.mdl new file mode 100644 index 000000000..df2d905a6 --- /dev/null +++ b/test/test-models/tests/macro_init_recurrence/macro_init_recurrence.mdl @@ -0,0 +1,52 @@ +{UTF-8} +:MACRO: INIT(x) +INIT = INITIAL(x) + ~ passthrough macro + ~ collides with renamed builtin + | +:END OF MACRO: + +Target: (t1-t3) + ~ + ~ | + +tNext: (t2-t3) -> tPrev + ~ + ~ | + +tPrev: (t1-t2) -> tNext + ~ + ~ | + +ecc[t1] = 1 ~~| + +ecc[tNext] = INITIAL(ecc[tPrev] * 2) ~~| + +FINAL TIME = 1 + ~ Month + ~ The final time for the simulation. + | + +INITIAL TIME = 0 + ~ Month + ~ The initial time for the simulation. + | + +SAVEPER = TIME STEP + ~ Month [0,?] + ~ The frequency with which output is stored. + | + +TIME STEP = 1 + ~ Month [0,?] + ~ The time step for the simulation. + | + +\\\---/// Sketch information - do not modify anything except names +V300 Do not put anything below this section - it will be ignored +*View 1 +$192-192-192,0,Times New Roman|12||0-0-0|0-0-0|0-0-255|-1--1--1|-1--1--1|96,96,100,0 +///---\\\ +:L<%^E!@ +1:helperrec.vdf64 +9:helperrec