From d4fb6dc9b5523fa0f7433ce66ecb813202152f99 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Tue, 10 Mar 2026 19:21:50 -0700 Subject: [PATCH 1/4] engine: make previous fully intrinsic Replace the remaining stdlib-backed PREVIOUS behavior with a single intrinsic two-argument path. Unary PREVIOUS now desugars to PREVIOUS(x, 0), module-backed and expression arguments rewrite through scalar helper auxes, and LTM first-step guards use INITIAL_TIME semantics instead of an implicit previous model. This removes stdlib/previous.stmx and its generated registration, updates dependency classification so PREVIOUS fallback expressions behave like init-only dependencies, and keeps interpreter/VM behavior aligned across direct, arrayed, SELF, module-backed, and nonzero-start cases. --- docs/design/ltm--loops-that-matter.md | 13 +- docs/tech-debt.md | 9 - src/simlin-engine/CLAUDE.md | 6 +- src/simlin-engine/src/ast/expr1.rs | 21 ++- src/simlin-engine/src/ast/expr2.rs | 5 +- src/simlin-engine/src/ast/expr3.rs | 12 +- src/simlin-engine/src/builtins.rs | 17 +- src/simlin-engine/src/builtins_visitor.rs | 141 ++++++++------- src/simlin-engine/src/bytecode.rs | 9 +- src/simlin-engine/src/compiler/codegen.rs | 43 ++--- src/simlin-engine/src/compiler/context.rs | 9 +- src/simlin-engine/src/compiler/expr.rs | 4 +- src/simlin-engine/src/compiler/mod.rs | 8 +- src/simlin-engine/src/compiler/pretty.rs | 2 +- src/simlin-engine/src/db.rs | 25 +-- src/simlin-engine/src/db_ltm.rs | 53 +++--- src/simlin-engine/src/db_ltm_tests.rs | 41 +++-- .../src/db_stdlib_ports_tests.rs | 4 - src/simlin-engine/src/db_tests.rs | 73 ++++---- src/simlin-engine/src/interpreter.rs | 24 +-- src/simlin-engine/src/ltm.rs | 40 +---- src/simlin-engine/src/ltm_augment.rs | 40 ++--- src/simlin-engine/src/model.rs | 91 +++++----- src/simlin-engine/src/patch.rs | 14 +- src/simlin-engine/src/project.rs | 8 +- src/simlin-engine/src/stdlib.gen.rs | 167 +----------------- src/simlin-engine/src/unit_checking_test.rs | 5 +- src/simlin-engine/src/units_check.rs | 21 ++- src/simlin-engine/src/units_infer.rs | 9 +- src/simlin-engine/src/variable.rs | 51 ++++-- src/simlin-engine/src/vm.rs | 22 ++- stdlib/previous.stmx | 61 ------- 32 files changed, 425 insertions(+), 623 deletions(-) delete mode 100644 stdlib/previous.stmx diff --git a/docs/design/ltm--loops-that-matter.md b/docs/design/ltm--loops-that-matter.md index 878ad5a8a..79bdfd373 100644 --- a/docs/design/ltm--loops-that-matter.md +++ b/docs/design/ltm--loops-that-matter.md @@ -242,7 +242,7 @@ For a link from `x` to `z` where `z = f(x, y, ...)`: replacing `x` inside `x_rate`, or corrupting function names like `MAX`). 4. The link score is: ``` - if (TIME = PREVIOUS(TIME)) then 0 + if (TIME = INITIAL_TIME) then 0 else if ((z - PREVIOUS(z)) = 0) OR ((x - PREVIOUS(x)) = 0) then 0 else ABS(SAFEDIV((partial_eq - PREVIOUS(z)), (z - PREVIOUS(z)), 0)) * SIGN(SAFEDIV((partial_eq - PREVIOUS(z)), (x - PREVIOUS(x)), 0)) @@ -267,7 +267,7 @@ The ratio is wrapped in `ABS()` because flow-to-stock polarity is structural: inflows always contribute positively (+1), outflows negatively (-1). The sign is applied outside the absolute value. This equation returns 0 for the first two timesteps (insufficient history for second-order differences), guarded by -`TIME = PREVIOUS(TIME)` and `PREVIOUS(TIME) = PREVIOUS(PREVIOUS(TIME))`. +`TIME = INITIAL_TIME` and `PREVIOUS(TIME, INITIAL_TIME) = INITIAL_TIME`. ### Stock-to-Flow Links @@ -540,11 +540,10 @@ computational interval" strategy. module-internal stocks to loop stock lists) is an implementation-specific extension that enables correct cycle partitioning. -4. **PREVIOUS via stdlib module**: The `PREVIOUS()` function used in link score - equations is implemented as a standard library module (`stdlib/previous.stmx`) - using a stock-and-flow structure, not as a built-in function. This affects - initial-timestep behavior: `TIME = PREVIOUS(TIME)` is used to detect the first - timestep and return 0. +4. **PREVIOUS is intrinsic**: The `PREVIOUS()` function used in link score + equations is compiled as an intrinsic two-argument builtin. Unary syntax is + desugared to `PREVIOUS(x, 0)`. LTM first-timestep behavior is handled + explicitly with `TIME = INITIAL_TIME`. 5. **Relative loop score formula**: The implementation uses `SAFEDIV(loop_score, sum_of_abs_scores, 0)` with explicit division-by-zero diff --git a/docs/tech-debt.md b/docs/tech-debt.md index bced0bb34..fa3886b13 100644 --- a/docs/tech-debt.md +++ b/docs/tech-debt.md @@ -159,15 +159,6 @@ Known debt items consolidated from CLAUDE.md files and codebase analysis. Each e - **Owner**: unassigned - **Last reviewed**: 2026-02-22 -### 20. parse_source_variable Missing module_idents Context - -- **Component**: simlin-engine (src/simlin-engine/src/db.rs) -- **Severity**: low -- **Description**: `parse_source_variable` calls `parse_var` (module_idents = None) rather than `parse_var_with_module_context`. Computing module_idents requires a model-level view (knowing which sibling variables are module-expanded), but salsa tracks individual variables. The consequence: `PREVIOUS(x)` where `x` is a user-written stdlib-call aux (e.g., `x = SMTH1(...)`) will compile to LoadPrev via the salsa-cached path instead of falling through to module expansion. The U+205A composite-identifier check in builtins_visitor.rs catches already-expanded names (the common case after a full compile), so this gap only affects the raw user-facing variable name at incremental-parse time. A fix would require computing a `module_idents` salsa input (set of user-written stdlib-call aux/flow names) at the model level and threading it into per-variable parse. -- **Measure**: Write a test: model with `x = SMTH1(input, 1)` and `y = PREVIOUS(x)`, verify y compiles to module expansion (not LoadPrev) via the salsa incremental path. -- **Owner**: unassigned -- **Last reviewed**: 2026-03-01 - ### 19. Flaky Hypothesis Tests in pysimlin Due to Slow Input Generation - **Component**: pysimlin (src/pysimlin/tests/test_json_types.py) diff --git a/src/simlin-engine/CLAUDE.md b/src/simlin-engine/CLAUDE.md index 134b213c1..4f7551c0c 100644 --- a/src/simlin-engine/CLAUDE.md +++ b/src/simlin-engine/CLAUDE.md @@ -11,7 +11,7 @@ Equation text flows through these stages in order: 1. **`src/lexer/`** - Tokenizer for equation syntax 2. **`src/parser/`** - Recursive descent parser producing `Expr0` AST 3. **`src/ast/`** - AST type system with progressive lowering: `Expr0` (parsed) -> `Expr1` (modules expanded) -> `Expr2` (dimensions resolved) -> `Expr3` (subscripts expanded). `array_view.rs` tracks array dimensions and sparsity. -4. **`src/builtins.rs`** - Builtin function definitions (e.g. `MIN`, `PULSE`, `LOOKUP`, `QUANTUM`, `SSHAPE`, `VECTOR SELECT`, `VECTOR ELM MAP`, `VECTOR SORT ORDER`, `ALLOCATE AVAILABLE`, `NPV`, `MODULO`, `PREVIOUS`, `INIT`). `is_stdlib_module_function()` is the authoritative predicate for deciding whether a function name expands to a stdlib module; shared by `equation_is_stdlib_call()` (pre-scan) and `contains_stdlib_call()` (walk-time). `builtins_visitor.rs` handles implicit module instantiation and routes `PREVIOUS`/`INIT` between opcode compilation and module expansion: 1-arg `PREVIOUS(var)` for simple scalar variables compiles to `LoadPrev`; 2-arg `PREVIOUS(x, init_val)`, nested, module-variable, and expression arguments fall through to module expansion. `INIT(x)` always compiles to `LoadInitial`. Tracks `module_idents` to prevent `PREVIOUS(module_var)` from using `LoadPrev` (modules occupy multiple slots). +4. **`src/builtins.rs`** - Builtin function definitions (e.g. `MIN`, `PULSE`, `LOOKUP`, `QUANTUM`, `SSHAPE`, `VECTOR SELECT`, `VECTOR ELM MAP`, `VECTOR SORT ORDER`, `ALLOCATE AVAILABLE`, `NPV`, `MODULO`, `PREVIOUS`, `INIT`). `is_stdlib_module_function()` is the authoritative predicate for deciding whether a function name expands to a stdlib module; shared by `equation_is_stdlib_call()` (pre-scan) and `contains_stdlib_call()` (walk-time). `builtins_visitor.rs` handles implicit module instantiation 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, AllocateAvailable) during array expansion, hoists them into `AssignTemp` pre-computations, and emits per-element `TempArrayElement` reads - `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 @@ -29,7 +29,7 @@ Equation text flows through these stages in order: - **`src/common.rs`** - Error types (`ErrorCode` with 100+ variants), `Result`, identifier types (`RawIdent`, `Ident`, dimension/element name types), canonicalization - **`src/datamodel.rs`** - Core structures: `Project`, `Model`, `Variable`, `Equation` (including `Arrayed` variant with `default_equation` for EXCEPT semantics), `Dimension` (with `mappings: Vec` replacing the old `maps_to` field), `DimensionMapping`, `DataSource`/`DataSourceKind`, `UnitMap` -- **`src/variable.rs`** - Variable variants (`Stock`, `Flow`, `Aux`, `Module`), `ModuleInput`, `Table` (graphical functions). `classify_dependencies()` is the primary API for extracting dependency categories from an AST in a single walk, returning a `DepClassification` with five sets: `all` (every referenced ident), `init_referenced`, `previous_referenced`, `previous_only` (idents only inside PREVIOUS), and `init_only` (idents only inside INIT/PREVIOUS). The older single-purpose functions (`identifier_set`, `init_referenced_idents`, etc.) remain as thin wrappers. `parse_var_with_module_context` accepts a `module_idents` set so `PREVIOUS(module_var)` falls through to module expansion instead of `LoadPrev`. +- **`src/variable.rs`** - Variable variants (`Stock`, `Flow`, `Aux`, `Module`), `ModuleInput`, `Table` (graphical functions). `classify_dependencies()` is the primary API for extracting dependency categories from an AST in a single walk, returning a `DepClassification` with five sets: `all` (every referenced ident), `init_referenced`, `previous_referenced`, `previous_only` (idents only inside PREVIOUS), and `init_only` (idents only inside INIT/PREVIOUS). The older single-purpose functions (`identifier_set`, `init_referenced_idents`, etc.) remain as thin wrappers. `parse_var_with_module_context` accepts a `module_idents` set so `PREVIOUS(module_var)` rewrites through a scalar helper aux instead of `LoadPrev`. - **`src/dimensions.rs`** - Dimension context and dimension matching for arrays - **`src/model.rs`** - Model compilation stages (`ModelStage0` -> `ModelStage1` -> `ModuleStage2`), dependency resolution, topological sort. `collect_module_idents` pre-scans datamodel variables to identify which names will expand to modules (preventing incorrect `LoadPrev` compilation). `init_referenced_vars` extends the Initials runlist to include variables referenced by `INIT()` calls, ensuring their values are captured in the `initial_values` snapshot. `check_units` is gated behind `cfg(any(test, feature = "testing"))` (production unit checking uses salsa tracked functions). - **`src/project.rs`** - `Project` struct aggregating models. `from_salsa(datamodel, db, source_project, cb)` builds a Project from a pre-synced salsa database (all variable parsing comes from salsa-cached results). `from_datamodel(datamodel)` is a convenience wrapper that creates a local DB and syncs. `From`, `with_ltm()`, and `with_ltm_all_links()` are all gated behind `cfg(any(test, feature = "testing"))` (retained only for tests and the AST interpreter cross-validation path); production code uses `db::compile_project_incremental` with `ltm_enabled`/`ltm_discovery_mode` on `SourceProject`. @@ -42,7 +42,7 @@ The primary compilation path uses salsa tracked functions for fine-grained incre - **`src/db.rs`** - `SimlinDb`, `SourceProject`/`SourceModel`/`SourceVariable` salsa inputs, `compile_project_incremental()` entry point, dependency graph computation, diagnostic accumulation via `CompilationDiagnostic` accumulator. `SourceProject` carries `ltm_enabled` and `ltm_discovery_mode` flags for LTM compilation. `Diagnostic` includes a `severity` field (`Error`/`Warning`) and `DiagnosticError` variants: `Equation`, `Model`, `Unit`, `Assembly`. `VariableDeps` includes `init_referenced_vars` to track variables referenced by `INIT()` calls. Dependency extraction uses two calls to `classify_dependencies()` (one for the dt AST, one for the init AST) instead of separate walker functions. `parse_source_variable_with_module_context` is the sole parse entry point (the non-module-context variant was removed). `variable_relevant_dimensions` provides dimension-granularity invalidation: scalar variables produce an empty dimension set so dimension changes never invalidate their parse results. - **`src/db_analysis.rs`** - Salsa-tracked causal graph analysis: `model_causal_edges`, `model_loop_circuits`, `model_cycle_partitions`, `model_detected_loops`. Produces `DetectedLoop` structs with polarity. -- **`src/db_ltm.rs`** - LTM (Loops That Matter) equation parsing and compilation as salsa tracked functions. Handles link scores, loop scores, relative loop scores, and PREVIOUS module expansion for LTM synthetic variables. +- **`src/db_ltm.rs`** - LTM (Loops That Matter) equation parsing and compilation as salsa tracked functions. Handles link scores, loop scores, relative loop scores, and any implicit helper/module vars synthesized while parsing LTM equations. - **`src/db_diagnostic_tests.rs`** - Verification tests for diagnostic accumulation paths. - **`src/db_differential_tests.rs`** - Differential tests verifying `classify_dependencies()` produces identical results to the old per-category walker functions, plus fragment-phase agreement tests ensuring dt/init ASTs yield consistent dependency classifications. - **`src/db_dimension_invalidation_tests.rs`** - Tests for dimension-granularity salsa invalidation: verifying that dimension changes only re-parse variables that reference those dimensions. diff --git a/src/simlin-engine/src/ast/expr1.rs b/src/simlin-engine/src/ast/expr1.rs index 154bc0561..7d16d4473 100644 --- a/src/simlin-engine/src/ast/expr1.rs +++ b/src/simlin-engine/src/ast/expr1.rs @@ -252,7 +252,19 @@ impl Expr1 { "vector_elm_map" => check_arity!(VectorElmMap, 2), "vector_sort_order" => check_arity!(VectorSortOrder, 2), "allocate_available" => check_arity!(AllocateAvailable, 3), - "previous" => check_arity!(Previous, 1), + "previous" => { + if args.len() == 1 { + let a = args.remove(0); + let zero = Expr1::Const("0".to_string(), 0.0, loc); + BuiltinFn::Previous(Box::new(a), Box::new(zero)) + } else if args.len() == 2 { + let b = args.remove(1); + let a = args.remove(0); + BuiltinFn::Previous(Box::new(a), Box::new(b)) + } else { + return eqn_err!(BadBuiltinArgs, loc.start, loc.end); + } + } "init" => check_arity!(Init, 1), _ => { // TODO: this could be a table reference, array reference, @@ -419,9 +431,10 @@ impl Expr1 { Box::new(b.constify_dimensions(scope)), Box::new(c.constify_dimensions(scope)), ), - BuiltinFn::Previous(a) => { - BuiltinFn::Previous(Box::new(a.constify_dimensions(scope))) - } + BuiltinFn::Previous(a, b) => BuiltinFn::Previous( + Box::new(a.constify_dimensions(scope)), + Box::new(b.constify_dimensions(scope)), + ), BuiltinFn::Init(a) => BuiltinFn::Init(Box::new(a.constify_dimensions(scope))), }; Expr1::App(func, loc) diff --git a/src/simlin-engine/src/ast/expr2.rs b/src/simlin-engine/src/ast/expr2.rs index 4710ac782..69f193455 100644 --- a/src/simlin-engine/src/ast/expr2.rs +++ b/src/simlin-engine/src/ast/expr2.rs @@ -805,7 +805,10 @@ impl Expr2 { ctx.set_allow_dimension_union(prev); result } - Previous(e) => Previous(Box::new(Expr2::from(*e, ctx)?)), + Previous(a, b) => Previous( + Box::new(Expr2::from(*a, ctx)?), + Box::new(Expr2::from(*b, ctx)?), + ), Init(e) => Init(Box::new(Expr2::from(*e, ctx)?)), }; // TODO: Handle array sources for builtin functions that return arrays diff --git a/src/simlin-engine/src/ast/expr3.rs b/src/simlin-engine/src/ast/expr3.rs index 955173d2b..a9181a681 100644 --- a/src/simlin-engine/src/ast/expr3.rs +++ b/src/simlin-engine/src/ast/expr3.rs @@ -189,8 +189,8 @@ impl Expr3 { | Size(e) | Stddev(e) | Sum(e) - | Previous(e) | Init(e) => e.references_a2a_dimension(), + Previous(a, b) => a.references_a2a_dimension() || b.references_a2a_dimension(), Max(a, b) | Min(a, b) => { a.references_a2a_dimension() || b.as_ref().is_some_and(|e| e.references_a2a_dimension()) @@ -965,9 +965,13 @@ impl<'a> Pass1Context<'a> { req_a2a || pp_a2a || avail_a2a, ) } - Previous(e) => { - let (new_e, has_a2a) = self.transform_inner(*e); - (Previous(Box::new(new_e)), has_a2a) + Previous(a, b) => { + let (new_a, a_has_a2a) = self.transform_inner(*a); + let (new_b, b_has_a2a) = self.transform_inner(*b); + ( + Previous(Box::new(new_a), Box::new(new_b)), + a_has_a2a || b_has_a2a, + ) } Init(e) => { let (new_e, has_a2a) = self.transform_inner(*e); diff --git a/src/simlin-engine/src/builtins.rs b/src/simlin-engine/src/builtins.rs index 9e4a360fb..537c46290 100644 --- a/src/simlin-engine/src/builtins.rs +++ b/src/simlin-engine/src/builtins.rs @@ -103,7 +103,7 @@ pub enum BuiltinFn { // ALLOCATE AVAILABLE(request, priority_profile, avail) AllocateAvailable(Box, Box, Box), // builtins replacing stdlib modules - Previous(Box), + Previous(Box, Box), Init(Box), } @@ -153,7 +153,7 @@ impl BuiltinFn { VectorSortOrder(_, _) => "vector_sort_order", AllocateAvailable(_, _, _) => "allocate_available", // builtins replacing stdlib modules - Previous(_) => "previous", + Previous(_, _) => "previous", Init(_) => "init", } } @@ -251,7 +251,7 @@ impl BuiltinFn { AllocateAvailable(a, b, c) => { AllocateAvailable(Box::new(f(*a)?), Box::new(f(*b)?), Box::new(f(*c)?)) } - Previous(a) => Previous(Box::new(f(*a)?)), + Previous(a, b) => Previous(Box::new(f(*a)?), Box::new(f(*b)?)), Init(a) => Init(Box::new(f(*a)?)), }) } @@ -280,7 +280,11 @@ impl BuiltinFn { } Abs(a) | Arccos(a) | Arcsin(a) | Arctan(a) | Cos(a) | Exp(a) | Int(a) | Ln(a) | Log10(a) | Sign(a) | Sin(a) | Sqrt(a) | Tan(a) | Size(a) | Stddev(a) | Sum(a) - | Previous(a) | Init(a) => f(a), + | Init(a) => f(a), + Previous(a, b) => { + f(a); + f(b); + } Inf | Pi | Time | TimeStep | StartTime | FinalTime | IsModuleInput(_, _) => {} Max(a, b) | Min(a, b) => { f(a); @@ -455,8 +459,11 @@ where | BuiltinFn::Size(a) | BuiltinFn::Stddev(a) | BuiltinFn::Sum(a) - | BuiltinFn::Previous(a) | BuiltinFn::Init(a) => cb(BuiltinContents::Expr(a)), + BuiltinFn::Previous(a, b) => { + cb(BuiltinContents::Expr(a)); + cb(BuiltinContents::Expr(b)); + } BuiltinFn::Mean(args) => { args.iter().for_each(|a| cb(BuiltinContents::Expr(a))); } diff --git a/src/simlin-engine/src/builtins_visitor.rs b/src/simlin-engine/src/builtins_visitor.rs index aff9379ef..fd9bbeca1 100644 --- a/src/simlin-engine/src/builtins_visitor.rs +++ b/src/simlin-engine/src/builtins_visitor.rs @@ -19,7 +19,6 @@ fn stdlib_args(name: &str) -> Option<&'static [&'static str]> { &["input", "delay_time", "initial_value"] } "npv" => &["stream", "discount_rate", "initial_value", "factor"], - "previous" => &["input", "initial_value"], _ => { return None; } @@ -34,9 +33,11 @@ fn contains_stdlib_call(expr: &Expr0) -> bool { Const(_, _, _) => false, Var(_, _) => false, App(UntypedBuiltinFn(func, args), _) => { - // INIT is included because it needs per-element temp vars in A2A - // context, though it doesn't create a standalone module. - if crate::builtins::is_stdlib_module_function(func.as_str()) || func.as_str() == "init" + // INIT/PREVIOUS are included because they may need per-element + // temp vars in A2A context, though they don't create standalone + // stdlib modules. + if crate::builtins::is_stdlib_module_function(func.as_str()) + || matches!(func.as_str(), "init" | "previous") { return true; } @@ -122,11 +123,11 @@ fn get_dimension_names(dimensions: &[Dimension]) -> Vec pub struct BuiltinVisitor<'a> { variable_name: &'a str, - /// Modules synthesized during the current walk (e.g., SMOOTH, DELAY, - /// PREVIOUS expansions). These are created using the same + /// Modules synthesized during the current walk (e.g., SMOOTH, DELAY + /// expansions). These are created using the same /// `is_stdlib_module_function` classification rule, extending the base /// set from `collect_module_idents()` at runtime so that nested references - /// (like `PREVIOUS(SMOOTH(...))`) correctly route through module expansion. + /// (like `PREVIOUS(SMOOTH(...))`) correctly synthesize scalar helper args. vars: HashMap, datamodel::Variable>, n: usize, self_allowed: bool, @@ -139,9 +140,8 @@ pub struct BuiltinVisitor<'a> { /// Reference to DimensionsContext for dimension mapping lookups dimensions_ctx: Option<&'a DimensionsContext>, /// Identifiers of Module variables in the parent model. - /// PREVIOUS(module_var) must fall through to module expansion - /// because modules occupy multiple slots and LoadPrev at the - /// base offset reads an internal sub-variable, not the output. + /// PREVIOUS(module_var) must synthesize a scalar temp arg rather than + /// reading a flat slot directly, because modules occupy multiple slots. module_idents: Option<&'a HashSet>>, } @@ -198,7 +198,8 @@ impl<'a> BuiltinVisitor<'a> { /// PREVIOUS/INIT opcode routing only applies to direct scalar variables. /// Module variables and qualified module outputs (`module·output`) must - /// be treated as module-backed so they can route through module expansion. + /// be treated as module-backed so PREVIOUS/INIT can synthesize scalar + /// helper args before compiling to intrinsic opcodes. fn is_module_backed_ident(&self, ident: &RawIdent) -> bool { let canonical = Ident::new(&canonicalize(ident.as_str())); if self.is_known_module_ident(&canonical) { @@ -329,6 +330,37 @@ impl<'a> BuiltinVisitor<'a> { } } + fn make_temp_arg(&mut self, arg: Expr0) -> RawIdent { + let transformed_arg = if self.active_subscript.is_some() { + self.substitute_dimension_refs(arg) + } else { + arg + }; + let subscript_suffix = self.subscript_suffix(); + let id = if subscript_suffix.is_empty() { + format!("$⁚{}⁚{}⁚arg0", self.variable_name, self.n) + } else { + format!( + "$⁚{}⁚{}⁚arg0⁚{}", + self.variable_name, self.n, subscript_suffix + ) + }; + let eqn = print_eqn(&transformed_arg); + let x_var = datamodel::Variable::Aux(datamodel::Aux { + ident: id.clone(), + equation: datamodel::Equation::Scalar(eqn), + documentation: "".to_string(), + units: None, + gf: None, + ai_state: None, + uid: None, + compat: datamodel::Compat::default(), + }); + self.vars.insert(Ident::new(&id), x_var); + self.n += 1; + RawIdent::new_from_str(&id) + } + fn walk_index(&mut self, expr: IndexExpr0) -> Result { use IndexExpr0::*; let result: IndexExpr0 = match expr { @@ -369,23 +401,26 @@ impl<'a> BuiltinVisitor<'a> { let rhs = it.next().unwrap(); return Ok(Op2(BinaryOp::Mod, Box::new(lhs), Box::new(rhs), loc)); } + let args = if func == "previous" && args.len() == 1 { + let mut args = args; + args.push(Const("0".to_string(), 0.0, loc)); + args + } else { + args + }; // PREVIOUS and INIT opcode routing: // - // 1-arg PREVIOUS(var) where var is a simple scalar variable - // reference compiles to the LoadPrev opcode. All other - // PREVIOUS forms fall through to module expansion: - // - 2-arg PREVIOUS(x, init_val) - // - nested PREVIOUS(PREVIOUS(x)) - // - PREVIOUS(expr) - // - PREVIOUS(module_var) -- modules occupy multiple slots, - // so LoadPrev at the base offset reads the wrong value - // - let previous_needs_module = func == "previous" - && (args.len() > 1 - || args.first().is_none_or(|a| match a { - Var(ident, _) => self.is_module_backed_ident(ident), - _ => true, - })); + // PREVIOUS compiles to the intrinsic opcode path for both + // unary and binary syntax. When arg0 is not a direct scalar + // slot (nested PREVIOUS, PREVIOUS(expr), PREVIOUS(module_var), + // module inputs inside implicit models, etc.), rewrite arg0 + // to a synthesized scalar temp variable. + let previous_needs_temp_arg = func == "previous" + && args.len() == 2 + && args.first().is_some_and(|a| match a { + Var(ident, _) => self.is_module_backed_ident(ident), + _ => true, + }); // LoadInitial only supports direct variable offsets. Rewrite // INIT(expr) and INIT(module_var) to INIT($temp_arg), where // $temp_arg captures expr/module_var each timestep and INIT @@ -396,46 +431,24 @@ impl<'a> BuiltinVisitor<'a> { Var(ident, _) => self.is_module_backed_ident(ident), _ => true, }); - if init_needs_temp_arg { - let arg = args.into_iter().next().expect("init arity checked"); - let transformed_arg = if self.active_subscript.is_some() { - self.substitute_dimension_refs(arg) - } else { - arg - }; - let subscript_suffix = self.subscript_suffix(); - let id = if subscript_suffix.is_empty() { - format!("$⁚{}⁚{}⁚arg0", self.variable_name, self.n) - } else { - format!( - "$⁚{}⁚{}⁚arg0⁚{}", - self.variable_name, self.n, subscript_suffix - ) - }; - let eqn = print_eqn(&transformed_arg); - let x_var = datamodel::Variable::Aux(datamodel::Aux { - ident: id.clone(), - equation: datamodel::Equation::Scalar(eqn), - documentation: "".to_string(), - units: None, - gf: None, - ai_state: None, - uid: None, - compat: datamodel::Compat::default(), - }); - self.vars.insert(Ident::new(&id), x_var); - self.n += 1; + if previous_needs_temp_arg { + let mut args = args.into_iter(); + let arg0 = args.next().expect("previous arity checked"); + let fallback = args.next().expect("previous arity checked"); + let id = self.make_temp_arg(arg0); return Ok(App( - UntypedBuiltinFn(func, vec![Var(RawIdent::new_from_str(&id), loc)]), + UntypedBuiltinFn(func, vec![Var(id, loc), fallback]), loc, )); } - if previous_needs_module { - // Fall through to module expansion for 2-arg form, - // complex-argument forms, and module variable arguments. - } else if is_builtin_fn(&func) { + if init_needs_temp_arg { + let arg = args.into_iter().next().expect("init arity checked"); + let id = self.make_temp_arg(arg); + return Ok(App(UntypedBuiltinFn(func, vec![Var(id, loc)]), loc)); + } + if is_builtin_fn(&func) { // Builtins that survive routing stay as builtins (e.g. - // PREVIOUS(var) and INIT(var)) and compile to opcodes. + // PREVIOUS(var, init) and INIT(var)) and compile to opcodes. return Ok(App(UntypedBuiltinFn(func, args), loc)); } @@ -553,10 +566,8 @@ impl<'a> BuiltinVisitor<'a> { /// Expand stdlib function calls (SMTH1, DELAY, etc.) and PREVIOUS/INIT /// builtins into implicit module instances and opcode-backed builtins. /// -/// When `module_idents` is provided, `PREVIOUS(module_var)` falls through -/// to module expansion instead of compiling to LoadPrev, because modules -/// occupy multiple slots and LoadPrev at the base offset reads an internal -/// sub-variable rather than the output. +/// When `module_idents` is provided, `PREVIOUS(module_var)` synthesizes a +/// scalar temp arg instead of reading a flat slot directly. pub fn instantiate_implicit_modules( variable_name: &str, ast: Ast, diff --git a/src/simlin-engine/src/bytecode.rs b/src/simlin-engine/src/bytecode.rs index fb5b4e905..884f82bc7 100644 --- a/src/simlin-engine/src/bytecode.rs +++ b/src/simlin-engine/src/bytecode.rs @@ -560,11 +560,10 @@ pub(crate) enum Opcode { off: VariableOffset, }, - /// Load the previous-timestep value of a variable from the prev_values - /// snapshot. Pushes `prev_values[module_off + off]` (or falls back to - /// `curr[]` during initials). Only simple PREVIOUS(var) compiles to - /// this opcode; nested PREVIOUS, PREVIOUS(TIME), and 2-arg forms use - /// module expansion instead. + /// Resolve PREVIOUS(var, fallback) for a direct scalar variable. + /// Pops the already-evaluated fallback from the stack, then pushes either + /// `prev_values[module_off + off]` or that fallback when `TIME == + /// INITIAL_TIME`. LoadPrev { off: VariableOffset, }, diff --git a/src/simlin-engine/src/compiler/codegen.rs b/src/simlin-engine/src/compiler/codegen.rs index 052347935..5f53c20d7 100644 --- a/src/simlin-engine/src/compiler/codegen.rs +++ b/src/simlin-engine/src/compiler/codegen.rs @@ -693,40 +693,25 @@ impl<'module> Compiler<'module> { return Ok(Some(())); }; - // PREVIOUS(x) and INIT(x) compile to dedicated opcodes that + // PREVIOUS(x, init) and INIT(x) compile to dedicated opcodes that // read from curr[] (previous timestep) or the initial-value // buffer, respectively. Handle them before the general // builtin dispatch because they do not use CallBuiltin. match builtin { - BuiltinFn::Previous(arg) => { - // Only simple PREVIOUS(var) reaches the builtin path; - // the builtins_visitor routes nested PREVIOUS, - // PREVIOUS(TIME), and 2-arg forms through module - // expansion. - // - // At Expr0 level, the arg was a plain Var, but during - // lowering inside a submodule context (e.g. LTM-augmented - // SMOOTH), the variable may resolve to a ModuleInput - // instead of a Var. Module inputs don't have slots in - // the flat curr[] buffer, so we can't use LoadPrev. We - // fall back to LoadModuleInput which reads the current - // value -- the PREVIOUS delay is already applied at the - // call site by the parent module. + BuiltinFn::Previous(arg, fallback) => { + self.walk_expr(fallback)?.unwrap(); match arg.as_ref() { Expr::Var(off, _) => { self.push(Opcode::LoadPrev { off: *off as VariableOffset, }); } - Expr::ModuleInput(off, _) => { - self.push(Opcode::LoadModuleInput { - input: *off as ModuleInputOffset, - }); - } _ => { - // Unexpected argument type -- emit the - // expression normally as a fallback. - self.walk_expr(arg)?; + return sim_err!( + NotSimulatable, + "PREVIOUS requires a variable reference after helper rewriting" + .to_string() + ); } } return Ok(Some(())); @@ -917,7 +902,7 @@ impl<'module> Compiler<'module> { self.push(Opcode::PopView {}); return Ok(Some(())); } - BuiltinFn::Previous(_) | BuiltinFn::Init(_) => { + BuiltinFn::Previous(_, _) | BuiltinFn::Init(_) => { unreachable!( "Previous/Init builtins should be handled before reaching BuiltinId dispatch" ); @@ -974,7 +959,7 @@ impl<'module> Compiler<'module> { // Previous/Init are handled by the early-return path at the top // of walk_builtin (LoadPrev/LoadInitial opcodes). Reaching here // would be a logic error. - BuiltinFn::Previous(_) | BuiltinFn::Init(_) => { + BuiltinFn::Previous(_, _) | BuiltinFn::Init(_) => { unreachable!( "Previous/Init builtins should be handled before reaching BuiltinId dispatch" ); @@ -1366,8 +1351,12 @@ impl<'module> Compiler<'module> { self.collect_iter_source_views_impl(b, views, seen); self.collect_iter_source_views_impl(c, views, seen); } - // Single expression argument (non-array) - Previous(a) | Init(a) => { + // Scalar lag/initial builtins + Previous(a, b) => { + self.collect_iter_source_views_impl(a, views, seen); + self.collect_iter_source_views_impl(b, views, seen); + } + Init(a) => { self.collect_iter_source_views_impl(a, views, seen); } // Constants/no-arg builtins diff --git a/src/simlin-engine/src/compiler/context.rs b/src/simlin-engine/src/compiler/context.rs index a28f8a148..73fd0dede 100644 --- a/src/simlin-engine/src/compiler/context.rs +++ b/src/simlin-engine/src/compiler/context.rs @@ -722,7 +722,9 @@ impl Context<'_> { Box::new(self.lower_pass0(c)), ), // Single expression builtins replacing stdlib modules - Previous(e) => Previous(Box::new(self.lower_pass0(e))), + Previous(a, b) => { + Previous(Box::new(self.lower_pass0(a)), Box::new(self.lower_pass0(b))) + } Init(e) => Init(Box::new(self.lower_pass0(e))), } } @@ -2016,7 +2018,10 @@ impl Context<'_> { Box::new(self.lower_from_expr3(avail)?), ) } - BFn::Previous(a) => BuiltinFn::Previous(Box::new(self.lower_from_expr3(a)?)), + BFn::Previous(a, b) => BuiltinFn::Previous( + Box::new(self.lower_from_expr3(a)?), + Box::new(self.lower_from_expr3(b)?), + ), BFn::Init(a) => BuiltinFn::Init(Box::new(self.lower_from_expr3(a)?)), }) } diff --git a/src/simlin-engine/src/compiler/expr.rs b/src/simlin-engine/src/compiler/expr.rs index 05833a6c4..f8690682d 100644 --- a/src/simlin-engine/src/compiler/expr.rs +++ b/src/simlin-engine/src/compiler/expr.rs @@ -219,7 +219,9 @@ impl Expr { Box::new(b.strip_loc()), Box::new(c.strip_loc()), ), - BuiltinFn::Previous(a) => BuiltinFn::Previous(Box::new(a.strip_loc())), + BuiltinFn::Previous(a, b) => { + BuiltinFn::Previous(Box::new(a.strip_loc()), Box::new(b.strip_loc())) + } BuiltinFn::Init(a) => BuiltinFn::Init(Box::new(a.strip_loc())), }; Expr::App(builtin, loc) diff --git a/src/simlin-engine/src/compiler/mod.rs b/src/simlin-engine/src/compiler/mod.rs index 582e07f15..8601e14b3 100644 --- a/src/simlin-engine/src/compiler/mod.rs +++ b/src/simlin-engine/src/compiler/mod.rs @@ -2176,8 +2176,12 @@ fn extract_temp_sizes_from_builtin(builtin: &BuiltinFn, temp_sizes_map: &mut Has | BuiltinFn::StartTime | BuiltinFn::FinalTime | BuiltinFn::IsModuleInput(_, _) => {} - // Single expression builtins replacing stdlib modules - BuiltinFn::Previous(expr) | BuiltinFn::Init(expr) => { + // Scalar lag/initial builtins + BuiltinFn::Previous(a, b) => { + extract_temp_sizes(a, temp_sizes_map); + extract_temp_sizes(b, temp_sizes_map); + } + BuiltinFn::Init(expr) => { extract_temp_sizes(expr, temp_sizes_map); } } diff --git a/src/simlin-engine/src/compiler/pretty.rs b/src/simlin-engine/src/compiler/pretty.rs index 92c174232..1812af7b3 100644 --- a/src/simlin-engine/src/compiler/pretty.rs +++ b/src/simlin-engine/src/compiler/pretty.rs @@ -222,7 +222,7 @@ pub fn pretty(expr: &Expr) -> String { pretty(c) ) } - BuiltinFn::Previous(a) => format!("previous({})", pretty(a)), + BuiltinFn::Previous(a, b) => format!("previous({}, {})", pretty(a), pretty(b)), BuiltinFn::Init(a) => format!("init({})", pretty(a)), }, Expr::EvalModule(module, model_name, _input_set, args) => { diff --git a/src/simlin-engine/src/db.rs b/src/simlin-engine/src/db.rs index 9447d1791..f2ffb1f12 100644 --- a/src/simlin-engine/src/db.rs +++ b/src/simlin-engine/src/db.rs @@ -1898,13 +1898,6 @@ fn get_stdlib_composite_ports() -> &'static crate::ltm_augment::CompositePortMap let sync = sync_from_datamodel(&db, &dm_project); let mut ports = HashMap::new(); for (model_name, synced_model) in &sync.models { - // PREVIOUS uses a stock internally for its one-step memory, - // but it is infrastructure — not a dynamic smoothing module - // whose inputs should get composite causal links. - if model_name == "stdlib⁚previous" { - continue; - } - let edges_result = model_causal_edges(&db, synced_model.source, sync.project); if edges_result.stocks.is_empty() { continue; @@ -3089,8 +3082,8 @@ pub fn compute_layout( } // Section 3b: Implicit variables generated by LTM equation - // parsing (PREVIOUS module instances). These stdlib modules - // need their own slots in the parent model's layout. + // parsing. Helper auxes and any expanded module vars need their + // own slots in the parent model's layout. let ltm_implicit = model_ltm_implicit_var_info(db, model, project); let mut ltm_im_names: Vec<&String> = ltm_implicit.keys().collect(); ltm_im_names.sort_unstable(); @@ -5097,9 +5090,9 @@ pub fn assemble_module( } } - // Append LTM implicit module fragments to initials and stocks - // runlists. PREVIOUS module instances contain stocks that need - // initialization and stock update phases. + // Append LTM implicit var fragments to the relevant runlists. + // Some implicit vars participate in initials and/or stocks even + // though they are not part of the original model. if is_root && project.ltm_enabled(db) { let ltm_implicit = model_ltm_implicit_var_info(db, model, project); let mut ltm_im_names: Vec<&String> = ltm_implicit.keys().collect(); @@ -5779,14 +5772,14 @@ fn calc_flattened_offsets_incremental( } // Include LTM variables (loop scores, relative loop scores, and their - // implicit PREVIOUS module instances) when LTM is enabled and this is - // the root model. These occupy slots after the implicit variables, + // implicit helper/module vars) when LTM is enabled and this is the + // root model. These occupy slots after the implicit variables, // matching compute_layout's Section 3 ordering. if is_root && project.ltm_enabled(db) { let layout = compute_layout(db, *source_model, project, true); // Enumerate all LTM variable names from the synthetic variables list - // and their implicit PREVIOUS module variables. + // and their implicit helper/module variables. let ltm_vars = if project.ltm_discovery_mode(db) { model_ltm_all_link_synthetic_variables(db, *source_model, project) } else { @@ -5808,7 +5801,7 @@ fn calc_flattened_offsets_incremental( ); } - // Add implicit PREVIOUS module variables from this LTM equation + // Add implicit variables from this LTM equation let parsed = db_ltm::parse_ltm_var_with_ids(db, ltm_var, project, <m_module_idents); for implicit_dm_var in &parsed.implicit_vars { let im_name = canonicalize(implicit_dm_var.get_ident()).into_owned(); diff --git a/src/simlin-engine/src/db_ltm.rs b/src/simlin-engine/src/db_ltm.rs index 91c3864b6..1479d455b 100644 --- a/src/simlin-engine/src/db_ltm.rs +++ b/src/simlin-engine/src/db_ltm.rs @@ -6,7 +6,8 @@ //! //! This module contains the per-equation compilation pipeline for LTM //! synthetic variables: link scores, loop scores, and relative loop -//! scores. It handles PREVIOUS module expansion and produces symbolic +//! scores. It handles intrinsic PREVIOUS helper rewrites plus other +//! implicit variables produced during parsing, and produces symbolic //! bytecodes for assembly by `assemble_module`. use std::collections::{BTreeSet, HashMap, HashSet}; @@ -55,7 +56,7 @@ pub(super) fn ltm_module_idents( /// Creates a transient `datamodel::Variable::Aux`, runs it through /// `parse_var` (which invokes `BuiltinVisitor` and /// `instantiate_implicit_modules`), and returns the parsed variable plus -/// any implicit module variables generated by PREVIOUS expansion. +/// any implicit helper/module variables generated while parsing. pub(super) fn parse_ltm_equation( var_name: &str, equation: &str, @@ -119,10 +120,11 @@ pub(super) fn parse_ltm_var_with_ids( /// Metadata about implicit variables generated by LTM equation parsing. /// -/// Each LTM link-score equation uses PREVIOUS() which creates implicit -/// stdlib module instances. This structure collects the implicit -/// variables across all LTM equations in a model so that `compute_layout` -/// can allocate slots and `assemble_module` can compile them. +/// LTM equations may synthesize helper auxes for intrinsic PREVIOUS/INIT +/// routing and may also expand stdlib module calls such as SMOOTH/DELAY. +/// This structure collects those implicit variables across all LTM +/// equations in a model so that `compute_layout` can allocate slots and +/// `assemble_module` can compile them. #[derive(Clone, Debug, PartialEq, Eq, salsa::Update)] pub struct LtmImplicitVarMeta { /// Canonical name of the LTM variable that created this implicit var @@ -141,10 +143,10 @@ pub struct LtmImplicitVarMeta { /// Cached implicit variable info for all LTM synthetic variables. /// -/// Parses each LTM equation to discover implicit modules (PREVIOUS -/// instances), caching the results. Both `compute_layout` and -/// `assemble_module` read this to allocate slots and compile fragments -/// for PREVIOUS module instances within LTM equations. +/// Parses each LTM equation to discover implicit helper/module variables, +/// caching the results. Both `compute_layout` and `assemble_module` read +/// this to allocate slots and compile fragments for those implicit vars +/// within LTM equations. #[salsa::tracked(returns(ref))] pub fn model_ltm_implicit_var_info( db: &dyn Db, @@ -229,12 +231,11 @@ pub fn model_ltm_implicit_var_info( /// LTM equations are pure scalar aux equations that may reference: /// - Model variables (stocks, flows, auxes) from the parent model /// - Other LTM variables (loop scores referencing link scores) -/// - Implicit module variables (PREVIOUS instances created by module -/// expansion) +/// - Implicit helper/module variables created during parsing /// - Implicit time/dt/initial_time/final_time variables /// -/// Since PREVIOUS still uses module expansion (not the LoadPrev opcode), -/// the parsed equation will contain implicit module variables that need +/// Parsed LTM equations may synthesize helper auxes for PREVIOUS/INIT +/// and may also expand stdlib module calls, so those implicit vars need /// to be handled the same way as in `compile_var_fragment`. #[salsa::tracked(returns(ref))] pub fn compile_ltm_var_fragment( @@ -251,9 +252,9 @@ pub fn compile_ltm_var_fragment( /// Compile an arbitrary LTM equation string to symbolic bytecodes. /// /// Shared implementation used by `compile_ltm_var_fragment` (link scores) -/// and the loop/relative score compilation in `assemble_module`. Handles -/// PREVIOUS module expansion by building a mini-context that includes -/// both model variables and implicit module variables. +/// and the loop/relative score compilation in `assemble_module`. Builds +/// a mini-context that includes both model variables and implicit vars +/// synthesized while parsing the LTM equation. pub(super) fn compile_ltm_equation_fragment( db: &dyn Db, var_name: &str, @@ -496,8 +497,8 @@ pub(super) fn compile_ltm_equation_fragment( continue; } - // Check if this is an implicit module from the LTM equation's own - // PREVIOUS expansion + // Check if this is an implicit var from the LTM equation's own + // parse-time helper/module synthesis. let mut found_in_parsed = false; for implicit_dm_var in &parsed.implicit_vars { if let datamodel::Variable::Module(dm_module) = implicit_dm_var @@ -749,7 +750,7 @@ pub(super) fn compile_ltm_equation_fragment( } } - // Add implicit module vars from PREVIOUS expansion + // Add implicit vars synthesized while parsing the LTM equation for (im_ident, im_var, im_size) in &implicit_module_vars { if !mini_metadata.contains_key(im_ident) { mini_metadata.insert( @@ -787,8 +788,8 @@ pub(super) fn compile_ltm_equation_fragment( let inputs = BTreeSet::new(); let mut module_models = model_module_map(db, model, project).clone(); - // Merge LTM implicit module references (PREVIOUS instances from LTM - // equation expansion) into the module_models map so the compiler + // Merge LTM implicit module references from LTM equation parsing into + // the module_models map so the compiler // context can resolve module_var_name -> sub_model_name lookups. if !implicit_module_refs.is_empty() { let current_model_modules = module_models.entry(model_name_ident.clone()).or_default(); @@ -923,7 +924,7 @@ pub(super) fn compile_ltm_equation_fragment( }) } -/// Compile a single implicit variable (PREVIOUS module instance) from an +/// Compile a single implicit variable from an /// LTM equation to symbolic bytecodes. /// /// This is analogous to `compile_implicit_var_fragment` but for implicit @@ -1407,9 +1408,9 @@ pub(super) fn compile_ltm_implicit_var_fragment( } }; - // PREVIOUS modules participate in initials (stock init) and stocks - // (stock update) phases. The dep_graph won't have LTM implicit vars - // in its runlists since they are not part of the original model. + // LTM implicit vars participate in whichever phases their lowered + // form needs. The dep_graph won't have them in its runlists since + // they are not part of the original model. // We compile all available phases. let initial_bytecodes = match build_var(true) { Ok(var_result) => compile_phase(&var_result.ast), diff --git a/src/simlin-engine/src/db_ltm_tests.rs b/src/simlin-engine/src/db_ltm_tests.rs index 6f782d7d8..7cbbee4c7 100644 --- a/src/simlin-engine/src/db_ltm_tests.rs +++ b/src/simlin-engine/src/db_ltm_tests.rs @@ -6,19 +6,28 @@ use super::compile_ltm_equation_fragment; use crate::datamodel; use crate::db::{SimlinDb, sync_from_datamodel}; -fn phase_has_sym_load_prev(phase: &Option) -> bool { - phase.as_ref().is_some_and(|bc| { - bc.symbolic.code.iter().any(|op| { - matches!( - op, - crate::compiler::symbolic::SymbolicOpcode::SymLoadPrev { .. } - ) +fn phase_sym_load_prev_names( + phase: &Option, +) -> Vec<&str> { + phase + .as_ref() + .map(|bc| { + bc.symbolic + .code + .iter() + .filter_map(|op| match op { + crate::compiler::symbolic::SymbolicOpcode::SymLoadPrev { var } => { + Some(var.name.as_str()) + } + _ => None, + }) + .collect() }) - }) + .unwrap_or_default() } #[test] -fn test_ltm_previous_module_var_uses_module_expansion() { +fn test_ltm_previous_module_var_uses_helper_rewrite() { let project = datamodel::Project { name: "ltm_prev_module_regression".to_string(), sim_specs: datamodel::SimSpecs::default(), @@ -77,16 +86,22 @@ fn test_ltm_previous_module_var_uses_module_expansion() { ) .expect("LTM equation should compile"); + let initial_prev_names = phase_sym_load_prev_names(&fragment.fragment.initial_bytecodes); + let flow_prev_names = phase_sym_load_prev_names(&fragment.fragment.flow_bytecodes); + let stock_prev_names = phase_sym_load_prev_names(&fragment.fragment.stock_bytecodes); + assert!( - !phase_has_sym_load_prev(&fragment.fragment.initial_bytecodes), + initial_prev_names.is_empty(), "initial phase should not use SymLoadPrev for PREVIOUS(module_var)", ); assert!( - !phase_has_sym_load_prev(&fragment.fragment.flow_bytecodes), - "flow phase should not use SymLoadPrev for PREVIOUS(module_var)", + flow_prev_names + .iter() + .all(|name| name.starts_with("$⁚$⁚ltm⁚test_prev_module⁚0⁚arg0")), + "flow phase should use SymLoadPrev only for the synthesized helper arg, got {flow_prev_names:?}", ); assert!( - !phase_has_sym_load_prev(&fragment.fragment.stock_bytecodes), + stock_prev_names.is_empty(), "stock phase should not use SymLoadPrev for PREVIOUS(module_var)", ); } diff --git a/src/simlin-engine/src/db_stdlib_ports_tests.rs b/src/simlin-engine/src/db_stdlib_ports_tests.rs index 47d268680..678d450f9 100644 --- a/src/simlin-engine/src/db_stdlib_ports_tests.rs +++ b/src/simlin-engine/src/db_stdlib_ports_tests.rs @@ -28,10 +28,6 @@ fn test_stdlib_composite_ports_include_dynamic_module_inputs() { "{model_name} should be present in the stdlib composite-port cache" ); } - assert!( - !ports.contains_key(&Ident::new("stdlib⁚previous")), - "PREVIOUS is infrastructure and must not get composite ports" - ); } #[test] diff --git a/src/simlin-engine/src/db_tests.rs b/src/simlin-engine/src/db_tests.rs index 8520d840a..41ac5248f 100644 --- a/src/simlin-engine/src/db_tests.rs +++ b/src/simlin-engine/src/db_tests.rs @@ -113,8 +113,11 @@ fn test_sync_simple_project() { .model_names(&db) .contains(&"main".to_string()) ); - // 1 user model + 7 stdlib models (init stdlib removed) - assert_eq!(result.project.model_names(&db).len(), 8); + // 1 user model + the remaining stdlib models (PREVIOUS/INIT are intrinsic). + assert_eq!( + result.project.model_names(&db).len(), + 1 + crate::stdlib::MODEL_NAMES.len() + ); let sim_specs = result.project.sim_specs(&db); assert_eq!(sim_specs.start, 0.0); @@ -4876,15 +4879,14 @@ fn test_init_opcode_interpreter_vm_parity() { } } -/// "previous" is still in MODEL_NAMES because 2-arg PREVIOUS(x, init_val) -/// still uses module expansion. "init" was removed -- 1-arg INIT now -/// compiles directly to LoadInitial. +/// PREVIOUS and INIT are both intrinsic now and should not appear in the +/// stdlib model registry. #[test] -fn test_previous_still_in_stdlib_model_names() { +fn test_previous_removed_from_stdlib_model_names() { let names = crate::stdlib::MODEL_NAMES; assert!( - names.contains(&"previous"), - "expected 'previous' in MODEL_NAMES (2-arg form still module-expanded)" + !names.contains(&"previous"), + "'previous' should no longer be in MODEL_NAMES" ); assert!( !names.contains(&"init"), @@ -4896,8 +4898,8 @@ fn test_previous_still_in_stdlib_model_names() { /// is recomputed each timestep; PREVIOUS(flow) should return the prior /// timestep's computed flow value. /// -/// Like stocks, the 1-arg PREVIOUS(flow) form returns 0 at t=0 -/// (stdlib module default initial_value). +/// Like stocks, the 1-arg PREVIOUS(flow) form returns its desugared +/// fallback `0` at t=0. #[test] fn test_previous_of_flow_interpreter_vm_parity() { use crate::test_common::TestProject; @@ -4927,9 +4929,8 @@ fn test_previous_of_flow_interpreter_vm_parity() { ); } - // The stdlib PREVIOUS module defaults initial_value=0 for the 1-arg - // form. At t=0, PREVIOUS(growth) returns 0. At subsequent steps it - // returns growth's value from the prior timestep. + // Unary PREVIOUS desugars to PREVIOUS(growth, 0). At t=0 it returns 0, + // and at subsequent steps it returns growth's prior-timestep value. let growth_vals = interp .get("growth") .expect("growth not in interpreter results"); @@ -5010,15 +5011,15 @@ fn test_arrayed_1arg_previous_loadprev_per_element() { ); } - // At t=0, LoadPrev reads from prev_values which is initialized to zeros. + // At t=0, unary PREVIOUS uses its desugared fallback of 0. assert!( (interp_a1[0] - 0.0).abs() < 1e-10, - "prev_val[a1] at t=0 should be 0 (LoadPrev zeros), got {}", + "prev_val[a1] at t=0 should be 0, got {}", interp_a1[0] ); assert!( (interp_a2[0] - 0.0).abs() < 1e-10, - "prev_val[a2] at t=0 should be 0 (LoadPrev zeros), got {}", + "prev_val[a2] at t=0 should be 0, got {}", interp_a2[0] ); @@ -5038,18 +5039,18 @@ fn test_arrayed_1arg_previous_loadprev_per_element() { } } -/// AC3.2: PREVIOUS(arrayed_var, init_val) (2-arg) creates per-element -/// module instances for an arrayed variable. Each element's module uses the -/// shared init_val at t=0 and tracks that element's previous value thereafter. +/// AC3.2: PREVIOUS(arrayed_var, init_val) (2-arg) compiles per element with +/// the explicit fallback. Each element uses the shared init_val at t=0 and +/// tracks that element's previous value thereafter. /// /// Model: DimA = {a1, a2} /// base_val[DimA]: a1 = 10, a2 = 20 /// prev_val[DimA] = PREVIOUS(base_val[DimA], 99) /// -/// At t=0: prev_val[a1] = 99, prev_val[a2] = 99 (init_val from module) +/// At t=0: prev_val[a1] = 99, prev_val[a2] = 99 (explicit fallback) /// At t=1: prev_val[a1] = 10, prev_val[a2] = 20 (prior step values) #[test] -fn test_arrayed_2arg_previous_per_element_modules() { +fn test_arrayed_2arg_previous_per_element() { use crate::test_common::TestProject; let tp = TestProject::new("arrayed_prev_2arg") @@ -5072,8 +5073,7 @@ fn test_arrayed_2arg_previous_per_element_modules() { .get("prev_val[a2]") .expect("prev_val[a2] not in VM results"); - // 2-arg PREVIOUS uses module expansion, so init_val=99 is returned at t=0 - // (not 0 as with the LoadPrev opcode path). + // The explicit fallback is returned at t=0. assert!( (vm_a1[0] - 99.0).abs() < 1e-10, "2-arg PREVIOUS[a1] at t=0 should be init_val=99, got {}", @@ -5689,8 +5689,8 @@ fn test_previous_self_initial_value() { let dep_graph = model_dependency_graph(&db, source_model, sync.project); assert!( dep_graph.runlist_initials.contains(&"switch".to_string()), - "switch must be in the initials runlist so the PREVIOUS module's \ - stock is initialized after switch is computed" + "switch must be in the initials runlist so PREVIOUS fallback helpers \ + are initialized after switch is computed" ); let compiled = compile_project_incremental(&db, sync.project, "main").unwrap(); @@ -5864,7 +5864,7 @@ fn test_previous_returns_zero_at_first_timestep() { } } #[test] -fn test_2arg_previous_uses_module_expansion() { +fn test_2arg_previous_uses_explicit_fallback() { use crate::test_common::TestProject; let tp = TestProject::new("prev_2arg") @@ -5888,7 +5888,7 @@ fn test_2arg_previous_uses_module_expansion() { ); } #[test] -fn test_dependency_graph_includes_previous_module_for_module_backed_var() { +fn test_dependency_graph_includes_previous_helper_for_module_backed_var() { use crate::testutils::{x_aux, x_model}; let project = crate::testutils::x_project( @@ -5897,8 +5897,8 @@ fn test_dependency_graph_includes_previous_module_for_module_backed_var() { "main", vec![ x_aux("x", "TIME", None), - x_aux("delayed", "PREVIOUS(x, 99)", None), - x_aux("prev_delayed", "PREVIOUS(delayed)", None), + x_aux("delayed", "SMTH1(x, 99)", None), + x_aux("prev_delayed", "PREVIOUS(delayed, 123)", None), ], )], ); @@ -5908,15 +5908,15 @@ fn test_dependency_graph_includes_previous_module_for_module_backed_var() { let source_model = sync.models["main"].source; let dep_graph = model_dependency_graph(&db, source_model, sync.project); - let has_previous_module = dep_graph + let has_previous_helper = dep_graph .runlist_initials .iter() .chain(dep_graph.runlist_flows.iter()) .chain(dep_graph.runlist_stocks.iter()) - .any(|name| name.starts_with("$⁚prev_delayed⁚0⁚previous")); + .any(|name| name.starts_with("$⁚prev_delayed⁚0⁚arg0")); assert!( - has_previous_module, - "dependency graph runlists should include the implicit PREVIOUS module for PREVIOUS(module-backed var)" + has_previous_helper, + "dependency graph runlists should include the helper aux for PREVIOUS(module-backed var)" ); } #[test] @@ -5944,10 +5944,9 @@ fn test_previous_of_module_backed_variable_compiles_correctly() { use crate::testutils::{x_aux, x_model}; use crate::vm::Vm; - // PREVIOUS(x) where x = SMTH1(input, 1) must use module expansion, - // not the LoadPrev opcode. LoadPrev is only valid for simple scalar - // variables; module-backed variables like SMTH1 occupy multiple VM - // slots (internal stock + aux), so LoadPrev would read the wrong slot. + // PREVIOUS(x) where x = SMTH1(input, 1) must rewrite through a scalar + // helper aux, not LoadPrev directly against the module-backed variable. + // Module-backed variables like SMTH1 occupy multiple VM slots. let project = datamodel::Project { name: "previous_of_smooth".to_string(), sim_specs: datamodel::SimSpecs { diff --git a/src/simlin-engine/src/interpreter.rs b/src/simlin-engine/src/interpreter.rs index de94e2a6b..4ff47ef53 100644 --- a/src/simlin-engine/src/interpreter.rs +++ b/src/simlin-engine/src/interpreter.rs @@ -1364,29 +1364,15 @@ impl ModuleEvaluator<'_> { f64::NAN } } - BuiltinFn::Previous(arg) => { - // PREVIOUS(x): return the value of x from the previous - // timestep, read from the snapshot taken after stocks - // but before the time advance. - // - // Only simple PREVIOUS(var) reaches here via the - // builtin path. Nested PREVIOUS and 2-arg forms go - // through module expansion in the builtins_visitor. - let prev_vals = self.sim.prev_values.borrow(); - if prev_vals.is_empty() { - // Before the first timestep snapshot, prev_values - // is empty. Return 0.0 to match the 1-arg stdlib - // PREVIOUS default (initial_value=0) and the VM's - // zero-initialized prev_values buffer. - 0.0 + BuiltinFn::Previous(arg, fallback) => { + if crate::float::approx_eq(self.curr[TIME_OFF], self.curr[INITIAL_TIME_OFF]) + { + self.eval(fallback) } else { + let prev_vals = self.sim.prev_values.borrow(); match arg.as_ref() { Expr::Var(off, _) => prev_vals[self.off + *off], _ => { - // Fallback: evaluate the expression normally. - // In practice, only Var reaches here because - // the builtins_visitor only promotes simple - // PREVIOUS(var) to the builtin path. drop(prev_vals); self.eval(arg) } diff --git a/src/simlin-engine/src/ltm.rs b/src/simlin-engine/src/ltm.rs index 8251c8b3c..a377d3db3 100644 --- a/src/simlin-engine/src/ltm.rs +++ b/src/simlin-engine/src/ltm.rs @@ -129,8 +129,6 @@ impl LoopPolarity { #[cfg_attr(feature = "debug-derive", derive(Debug))] #[derive(Clone, Copy, PartialEq, Eq)] pub(crate) enum ModuleLtmRole { - /// PREVIOUS -- LTM infrastructure module, never analyze - Infrastructure, /// Has internal stocks (SMOOTH, DELAY, TREND, user-defined modules with stocks) DynamicModule, /// No internal stocks -- pure passthrough @@ -139,18 +137,12 @@ pub(crate) enum ModuleLtmRole { /// Classify a module model for LTM analysis. /// -/// The PREVIOUS infrastructure module is used BY link score equations -/// and must never be analyzed to avoid infinite recursion. Dynamic modules -/// contain stocks and need composite link scores. Stateless modules are -/// passthroughs. +/// Dynamic modules contain stocks and need composite link scores. +/// Stateless modules are passthroughs. pub(crate) fn classify_module_for_ltm( - model_name: &Ident, + _model_name: &Ident, module_model: &ModelStage1, ) -> ModuleLtmRole { - let name = model_name.as_str(); - if name == "stdlib⁚previous" { - return ModuleLtmRole::Infrastructure; - } if module_model .variables .values() @@ -3254,32 +3246,6 @@ mod tests { ); } - #[test] - fn test_classify_previous_as_infrastructure() { - use crate::test_common::TestProject; - - // PREVIOUS is used by LTM link score equations, so any LTM-augmented - // project will have it. - let project = TestProject::new("test_classify_prev") - .with_sim_time(0.0, 10.0, 1.0) - .stock("level", "50", &["adj"], &[], None) - .flow("adj", "(100 - level) / 5", None) - .compile() - .expect("should compile"); - - let ltm_project = project.with_ltm().expect("should augment with LTM"); - let prev_ident = Ident::new("stdlib⁚previous"); - let prev_model = ltm_project - .models - .get(&prev_ident) - .expect("should have stdlib⁚previous model"); - - assert_eq!( - classify_module_for_ltm(&prev_ident, prev_model), - ModuleLtmRole::Infrastructure - ); - } - #[test] fn test_classify_smth1_as_dynamic() { use crate::test_common::TestProject; diff --git a/src/simlin-engine/src/ltm_augment.rs b/src/simlin-engine/src/ltm_augment.rs index 99398d181..9f3fd4977 100644 --- a/src/simlin-engine/src/ltm_augment.rs +++ b/src/simlin-engine/src/ltm_augment.rs @@ -5,9 +5,9 @@ //! LTM project augmentation - adds synthetic variables for link and loop scores //! //! This module generates synthetic variables for Loops That Matter (LTM) analysis. -//! The generated equations use the PREVIOUS function, which is implemented as a -//! module in stdlib/previous.stmx (not as a builtin function). The PREVIOUS module -//! uses a stock-and-flow structure to store and return the previous timestep's value. +//! The generated equations use the intrinsic two-argument `PREVIOUS(value, initial)` +//! function. First- and second-timestep guards are expressed explicitly with +//! `TIME = INITIAL_TIME` and `PREVIOUS(TIME, INITIAL_TIME) = INITIAL_TIME`. use crate::ast::{Expr0, IndexExpr0, print_eqn}; use crate::builtins::UntypedBuiltinFn; @@ -379,7 +379,7 @@ fn generate_module_link_score_equation( format!( "if \ - (TIME = PREVIOUS(TIME)) \ + (TIME = INITIAL_TIME) \ then 0 \ else if \ (({to_q} - PREVIOUS({to_q})) = 0) OR (({from_q} - PREVIOUS({from_q})) = 0) \ @@ -549,7 +549,7 @@ fn generate_auxiliary_to_auxiliary_equation( // Return 0 at the initial timestep when PREVIOUS values don't exist yet format!( "if \ - (TIME = PREVIOUS(TIME)) \ + (TIME = INITIAL_TIME) \ then 0 \ else if \ (({to_q} - PREVIOUS({to_q})) = 0) OR (({from_q} - PREVIOUS({from_q})) = 0) \ @@ -587,7 +587,7 @@ fn generate_flow_to_stock_equation(flow: &str, stock: &str, stock_var: &Variable // Return 0 for the first two timesteps when we don't have enough history for second-order differences format!( "if \ - (TIME = PREVIOUS(TIME)) OR (PREVIOUS(TIME) = PREVIOUS(PREVIOUS(TIME))) \ + (TIME = INITIAL_TIME) OR (PREVIOUS(TIME, INITIAL_TIME) = INITIAL_TIME) \ then 0 \ else {sign}ABS(SAFEDIV({numerator}, {denominator}, 0))" ) @@ -648,7 +648,7 @@ fn generate_stock_to_flow_equation( // Return 0 at the initial timestep when PREVIOUS values don't exist yet format!( "if \ - (TIME = PREVIOUS(TIME)) \ + (TIME = INITIAL_TIME) \ then 0 \ else if \ ({flow_diff} = 0) OR ({stock_diff} = 0) \ @@ -1080,7 +1080,7 @@ mod tests { // Verify the EXACT equation structure // Returns 0 at initial timestep when PREVIOUS values don't exist let expected = "if \ - (TIME = PREVIOUS(TIME)) \ + (TIME = INITIAL_TIME) \ then 0 \ else if \ ((y - PREVIOUS(y)) = 0) OR ((x - PREVIOUS(x)) = 0) \ @@ -1119,7 +1119,7 @@ mod tests { // ABS wraps the ratio; sign is fixed (+1 for inflows) per corrected 2023 formula // Returns 0 for first two timesteps when insufficient history let expected = "if \ - (TIME = PREVIOUS(TIME)) OR (PREVIOUS(TIME) = PREVIOUS(PREVIOUS(TIME))) \ + (TIME = INITIAL_TIME) OR (PREVIOUS(TIME, INITIAL_TIME) = INITIAL_TIME) \ then 0 \ else ABS(SAFEDIV(\ (PREVIOUS(inflow_rate) - PREVIOUS(PREVIOUS(inflow_rate))), \ @@ -1158,7 +1158,7 @@ mod tests { // ABS wraps the ratio; sign is fixed (-1 for outflows) per corrected 2023 formula // Returns 0 for first two timesteps when insufficient history let expected = "if \ - (TIME = PREVIOUS(TIME)) OR (PREVIOUS(TIME) = PREVIOUS(PREVIOUS(TIME))) \ + (TIME = INITIAL_TIME) OR (PREVIOUS(TIME, INITIAL_TIME) = INITIAL_TIME) \ then 0 \ else -ABS(SAFEDIV(\ (PREVIOUS(outflow_rate) - PREVIOUS(PREVIOUS(outflow_rate))), \ @@ -1325,7 +1325,7 @@ mod tests { // Verify the EXACT equation structure for module-to-variable link // Returns 0 at initial timestep when PREVIOUS values don't exist let expected = "if \ - (TIME = PREVIOUS(TIME)) \ + (TIME = INITIAL_TIME) \ then 0 \ else if \ ((processed - PREVIOUS(processed)) = 0) OR ((smoother - PREVIOUS(smoother)) = 0) \ @@ -1390,7 +1390,7 @@ mod tests { // Verify the EXACT equation structure for variable-to-module link // Returns 0 at initial timestep when PREVIOUS values don't exist let expected = "if \ - (TIME = PREVIOUS(TIME)) \ + (TIME = INITIAL_TIME) \ then 0 \ else if \ ((processor - PREVIOUS(processor)) = 0) OR ((raw_data - PREVIOUS(raw_data)) = 0) \ @@ -1449,7 +1449,7 @@ mod tests { // Verify the EXACT equation structure for module-to-module link // Returns 0 at initial timestep when PREVIOUS values don't exist let expected = "if \ - (TIME = PREVIOUS(TIME)) \ + (TIME = INITIAL_TIME) \ then 0 \ else if \ ((filter_b - PREVIOUS(filter_b)) = 0) OR ((filter_a - PREVIOUS(filter_a)) = 0) \ @@ -1608,7 +1608,7 @@ mod tests { // Sign term uses first-order stock change per LTM paper formula // Returns 0 at initial timestep when PREVIOUS values don't exist let expected = "if \ - (TIME = PREVIOUS(TIME)) \ + (TIME = INITIAL_TIME) \ then 0 \ else if \ ((deaths - PREVIOUS(deaths)) = 0) OR \ @@ -1655,7 +1655,7 @@ mod tests { // Returns 0 at initial timestep when PREVIOUS values don't exist // Non-stock dependencies (like rate) get wrapped in PREVIOUS() let expected = "if \ - (TIME = PREVIOUS(TIME)) \ + (TIME = INITIAL_TIME) \ then 0 \ else if \ ((production - PREVIOUS(production)) = 0) OR \ @@ -1701,7 +1701,7 @@ mod tests { // Returns 0 at initial timestep when PREVIOUS values don't exist // Non-stock dependencies (like drain_time) get wrapped in PREVIOUS() let expected = "if \ - (TIME = PREVIOUS(TIME)) \ + (TIME = INITIAL_TIME) \ then 0 \ else if \ ((drainage - PREVIOUS(drainage)) = 0) OR \ @@ -1744,7 +1744,7 @@ mod tests { // Sign term uses first-order stock change per LTM paper formula // Returns 0 at initial timestep when PREVIOUS values don't exist let expected = "if \ - (TIME = PREVIOUS(TIME)) \ + (TIME = INITIAL_TIME) \ then 0 \ else if \ ((births - PREVIOUS(births)) = 0) OR \ @@ -1786,7 +1786,7 @@ mod tests { // Sign term uses first-order stock change per LTM paper formula // Returns 0 at initial timestep when PREVIOUS values don't exist let expected = "if \ - (TIME = PREVIOUS(TIME)) \ + (TIME = INITIAL_TIME) \ then 0 \ else if \ ((constant_flow - PREVIOUS(constant_flow)) = 0) OR \ @@ -1831,7 +1831,7 @@ mod tests { // wrap the variable `s` in PREVIOUS, not the function name. // (The parse roundtrip lowercases function names — case-insensitive language.) let expected = "if \ - (TIME = PREVIOUS(TIME)) \ + (TIME = INITIAL_TIME) \ then 0 \ else if \ ((y - PREVIOUS(y)) = 0) OR ((max_val - PREVIOUS(max_val)) = 0) \ @@ -1920,7 +1920,7 @@ mod tests { // Sign term uses first-order stock change per LTM paper formula // Returns 0 at initial timestep when PREVIOUS values don't exist let expected = "if \ - (TIME = PREVIOUS(TIME)) \ + (TIME = INITIAL_TIME) \ then 0 \ else if \ ((inflow - PREVIOUS(inflow)) = 0) OR \ diff --git a/src/simlin-engine/src/model.rs b/src/simlin-engine/src/model.rs index 678c247a5..5bcfbf966 100644 --- a/src/simlin-engine/src/model.rs +++ b/src/simlin-engine/src/model.rs @@ -806,9 +806,9 @@ where /// whose equations parse to a top-level stdlib function call (e.g. SMTH1, /// DELAY, etc.) /// -/// This set is needed so that `PREVIOUS(module_var)` falls through to module -/// expansion instead of compiling to LoadPrev, because modules occupy -/// multiple slots and LoadPrev at the base offset reads the wrong value. +/// This set is needed so that `PREVIOUS(module_var)` rewrites through a +/// synthesized scalar helper aux instead of compiling `LoadPrev` directly +/// against a multi-slot module. pub(crate) fn collect_module_idents( variables: &[datamodel::Variable], ) -> HashSet> { @@ -840,8 +840,7 @@ pub(crate) fn collect_module_idents( /// Check if a scalar equation's top-level expression is a stdlib function call. /// /// Uses `is_stdlib_module_function` as the underlying predicate for name -/// matching, with additional PREVIOUS arg-count logic: 1-arg PREVIOUS uses -/// the LoadPrev opcode, while 2+ args expand to a module. +/// matching. /// /// This intentionally re-parses the equation text rather than reusing the /// already-parsed AST. It runs during `collect_module_idents` (called from @@ -860,12 +859,8 @@ pub(crate) fn equation_is_stdlib_call(eqn: &datamodel::Equation) -> bool { match &ast { Expr0::App(crate::builtins::UntypedBuiltinFn(func, args), _) => { let func_lower = func.to_lowercase(); - // PREVIOUS(x) with 1 arg uses LoadPrev; 2+ args expand to a module. - if func_lower == "previous" { - args.len() > 1 - } else { - crate::builtins::is_stdlib_module_function(&func_lower) - } + let _ = args; + crate::builtins::is_stdlib_module_function(&func_lower) } _ => false, } @@ -882,21 +877,20 @@ impl ModelStage0 { ) -> Self { let mut implicit_vars: Vec = Vec::new(); - // Determine which variable names should force PREVIOUS to module - // expansion (rather than the LoadPrev opcode). + // Determine which variable names should force PREVIOUS to synthesize + // a scalar temp arg rather than reading a flat slot directly. // // For user models, only explicit Module variables and stdlib-call - // Aux/Flow variables need module expansion because they occupy + // Aux/Flow variables need temp-arg rewriting because they occupy // multiple slots and LoadPrev at the base offset reads the wrong // sub-variable. // // For implicit (stdlib) models, ALL variable names are included. // Inside a submodule, some variables are module inputs whose values // are passed from the parent via a transient array -- they have no - // persistent slot in prev_values. PREVIOUS(module_input) compiled - // as LoadPrev/LoadModuleInput returns the CURRENT value, not the - // previous one. Module expansion creates a stock that correctly - // tracks the one-step delay. + // persistent slot in prev_values. PREVIOUS(module_input) must first + // capture the current scalar into a temp helper so LoadPrev reads + // that helper's slot on the next step. let module_idents: HashSet> = if implicit { x_model .variables @@ -965,11 +959,10 @@ impl ModelStage0 { ) -> Self { // For implicit (stdlib) models, bypass the salsa cache and use // the direct path with module_idents awareness. This ensures - // PREVIOUS calls inside submodules always expand to modules - // rather than compiling to LoadPrev/LoadModuleInput, which - // returns the current value instead of the previous value for - // module inputs. The performance impact is negligible since - // stdlib models have very few variables. + // PREVIOUS calls inside submodules rewrite through scalar helper + // auxes instead of compiling LoadPrev/LoadModuleInput directly + // against transient module-input slots. The performance impact is + // negligible since stdlib models have very few variables. if implicit { return Self::new(x_model, dimensions, units_ctx, implicit); } @@ -979,9 +972,9 @@ impl ModelStage0 { let mut implicit_vars: Vec = Vec::new(); let mut variable_list: Vec = Vec::new(); - // Collect module identifiers for the PREVIOUS gate. + // Collect module identifiers for the PREVIOUS/INIT helper rewrite. // For user models, only explicit Module variables and stdlib-call - // Aux/Flow variables need module expansion. + // Aux/Flow variables are multi-slot/module-backed. let module_idents: HashSet> = collect_module_idents(&x_model.variables); let mut module_ident_list: Vec = module_idents .iter() @@ -1597,7 +1590,7 @@ fn test_errors() { } #[test] -fn test_new_cached_preserves_previous_module_expansion() { +fn test_new_cached_preserves_previous_helper_rewrite() { let units_ctx = Context::new(&[], &Default::default()).unwrap(); let main_model = x_model( "main", @@ -1636,21 +1629,21 @@ fn test_new_cached_preserves_previous_module_expansion() { false, ); - let has_previous_module = |model: &ModelStage0| { + let has_previous_helper = |model: &ModelStage0| { model .variables .keys() - .any(|ident| ident.as_str().starts_with("$⁚prev_sub⁚0⁚previous")) + .any(|ident| ident.as_str().starts_with("$⁚prev_sub⁚0⁚arg0")) }; assert!( - has_previous_module(&direct), - "direct parse should expand PREVIOUS(sub) into an implicit module" + has_previous_helper(&direct), + "direct parse should synthesize a scalar helper for PREVIOUS(sub)" ); assert_eq!( - has_previous_module(&direct), - has_previous_module(&cached), - "cached parse should preserve PREVIOUS(module_var) module expansion" + has_previous_helper(&direct), + has_previous_helper(&cached), + "cached parse should preserve PREVIOUS(module_var) helper rewriting" ); } @@ -1730,7 +1723,7 @@ fn test_init_expression_interpreter_vm_parity() { } #[test] -fn test_previous_module_input_var_uses_module_expansion() { +fn test_previous_module_input_var_uses_helper_rewrite() { let units_ctx = Context::new(&[], &Default::default()).unwrap(); let module_input = datamodel::Variable::Aux(datamodel::Aux { ident: "input".to_string(), @@ -1754,8 +1747,8 @@ fn test_previous_module_input_var_uses_module_expansion() { parsed .variables .keys() - .any(|ident| ident.as_str().starts_with("$⁚lagged⁚0⁚previous")), - "PREVIOUS(module_input) should be expanded to an implicit previous module" + .any(|ident| ident.as_str().starts_with("$⁚lagged⁚0⁚arg0")), + "PREVIOUS(module_input) should synthesize a scalar helper aux" ); } @@ -1770,8 +1763,8 @@ fn test_model_implicit_var_info_uses_module_context() { "main", vec![ x_aux("x", "TIME", None), - x_aux("delayed", "PREVIOUS(x, 99)", None), - x_aux("prev_delayed", "PREVIOUS(delayed)", None), + x_aux("delayed", "SMTH1(x, 99)", None), + x_aux("prev_delayed", "PREVIOUS(delayed, 123)", None), ], )], source: None, @@ -1784,8 +1777,8 @@ fn test_model_implicit_var_info_uses_module_context() { assert!( implicit_info .keys() - .any(|name| name.starts_with("$⁚prev_delayed⁚0⁚previous")), - "model_implicit_var_info should include implicit vars for PREVIOUS(module-backed var)" + .any(|name| name.starts_with("$⁚prev_delayed⁚0⁚arg0")), + "model_implicit_var_info should include helper auxes for PREVIOUS(module-backed var)" ); } @@ -1800,8 +1793,8 @@ fn test_incremental_compile_previous_of_module_backed_var() { "main", vec![ x_aux("x", "TIME", None), - x_aux("delayed", "PREVIOUS(x, 99)", None), - x_aux("prev_delayed", "PREVIOUS(delayed)", None), + x_aux("delayed", "SMTH1(x, 99)", None), + x_aux("prev_delayed", "PREVIOUS(delayed, 123)", None), ], )], source: None, @@ -1818,7 +1811,7 @@ fn test_incremental_compile_previous_of_module_backed_var() { } #[test] -fn test_collect_module_idents_skips_one_arg_previous() { +fn test_collect_module_idents_skips_intrinsic_previous() { let vars = vec![ x_aux("x", "TIME", None), x_aux("prev_x", "PREVIOUS(x)", None), @@ -1827,16 +1820,16 @@ fn test_collect_module_idents_skips_one_arg_previous() { let ids = collect_module_idents(&vars); assert!( !ids.contains(&Ident::new("prev_x")), - "1-arg PREVIOUS should stay on the opcode path, not module-backed", + "1-arg PREVIOUS should stay on the intrinsic opcode path", ); assert!( - ids.contains(&Ident::new("prev_x_init")), - "2-arg PREVIOUS should remain module-backed", + !ids.contains(&Ident::new("prev_x_init")), + "2-arg PREVIOUS should also stay intrinsic", ); } #[test] -fn test_collect_module_idents_marks_apply_to_all_stdlib_calls() { +fn test_collect_module_idents_skips_apply_to_all_previous() { let vars = vec![ x_aux("x", "TIME", None), datamodel::Variable::Aux(datamodel::Aux { @@ -1855,8 +1848,8 @@ fn test_collect_module_idents_marks_apply_to_all_stdlib_calls() { ]; let ids = collect_module_idents(&vars); assert!( - ids.contains(&Ident::new("prev_x_init")), - "ApplyToAll equations that invoke 2-arg PREVIOUS should be marked module-backed", + !ids.contains(&Ident::new("prev_x_init")), + "ApplyToAll equations that invoke PREVIOUS should stay intrinsic", ); } diff --git a/src/simlin-engine/src/patch.rs b/src/simlin-engine/src/patch.rs index bc5b9ec26..187cde2fb 100644 --- a/src/simlin-engine/src/patch.rs +++ b/src/simlin-engine/src/patch.rs @@ -708,9 +708,10 @@ fn rename_builtin( Box::new(rename_expr(b, old_ident, new_ident)), Box::new(rename_expr(c, old_ident, new_ident)), ), - BuiltinFn::Previous(expr) => { - BuiltinFn::Previous(Box::new(rename_expr(expr, old_ident, new_ident))) - } + BuiltinFn::Previous(a, b) => BuiltinFn::Previous( + Box::new(rename_expr(a, old_ident, new_ident)), + Box::new(rename_expr(b, old_ident, new_ident)), + ), BuiltinFn::Init(expr) => BuiltinFn::Init(Box::new(rename_expr(expr, old_ident, new_ident))), } } @@ -914,9 +915,10 @@ pub(crate) fn builtin_to_untyped(builtin: &BuiltinFn) -> UntypedBuiltinFn "allocate_available".to_string(), vec![expr2_to_expr0(a), expr2_to_expr0(b), expr2_to_expr0(c)], ), - BuiltinFn::Previous(expr) => { - UntypedBuiltinFn("previous".to_string(), vec![expr2_to_expr0(expr)]) - } + BuiltinFn::Previous(a, b) => UntypedBuiltinFn( + "previous".to_string(), + vec![expr2_to_expr0(a), expr2_to_expr0(b)], + ), BuiltinFn::Init(expr) => UntypedBuiltinFn("init".to_string(), vec![expr2_to_expr0(expr)]), } } diff --git a/src/simlin-engine/src/project.rs b/src/simlin-engine/src/project.rs index b49d5c0be..42e14fc95 100644 --- a/src/simlin-engine/src/project.rs +++ b/src/simlin-engine/src/project.rs @@ -152,11 +152,9 @@ impl Project { .is_some_and(|suffix| crate::stdlib::MODEL_NAMES.contains(&suffix)); let model_name = src_model.name(db); let src_vars = src_model.variables(db); - // For stdlib models, ALL variable names must be module idents - // so PREVIOUS(module_input) uses module expansion instead of - // LoadPrev. Inside a submodule, module inputs are passed via a - // transient array with no persistent slot in prev_values; - // LoadPrev would return the current value, not the previous one. + // For stdlib models, ALL variable names must be module idents so + // PREVIOUS(module_input) rewrites through a scalar helper aux + // instead of reading a transient module-input slot directly. let extra_module_idents: Vec = if is_stdlib { src_vars.keys().cloned().collect() } else { diff --git a/src/simlin-engine/src/stdlib.gen.rs b/src/simlin-engine/src/stdlib.gen.rs index 3ac2898ab..5125b0d6b 100644 --- a/src/simlin-engine/src/stdlib.gen.rs +++ b/src/simlin-engine/src/stdlib.gen.rs @@ -5,7 +5,7 @@ // because they reference simulation system variables (TIME, TIME STEP, INITIAL TIME). // Those models are defined in this file and re-emitted verbatim by gen_stdlib.rs. // -// Stdlib SHA256: 3204863937f4a2210e69947f59536cbb14e7e1515b12c2c89e48ffc3f162d11e +// Stdlib SHA256: 7edfc8d7e03eda7f3cbdc9acf9dd6b809c8aaee1c851499ea61935270576aefc #![allow( clippy::approx_constant, @@ -21,16 +21,13 @@ use crate::datamodel::{ SimMethod, SimSpecs, Stock, StockFlow, Variable, View, ViewElement, Visibility, view_element, }; -pub const MODEL_NAMES: [&str; 7] = [ - "delay1", "delay3", "npv", "previous", "smth1", "smth3", "trend", -]; +pub const MODEL_NAMES: [&str; 6] = ["delay1", "delay3", "npv", "smth1", "smth3", "trend"]; pub fn get(name: &str) -> Option { match name { "delay1" => Some(delay1()), "delay3" => Some(delay3()), "npv" => Some(npv()), - "previous" => Some(previous()), "smth1" => Some(smth1()), "smth3" => Some(smth3()), "trend" => Some(trend()), @@ -432,166 +429,6 @@ fn delay3() -> Model { } } -fn previous() -> Model { - Model { - name: "stdlib⁚previous".to_string(), - sim_specs: None, - variables: vec![ - Variable::Flow(Flow { - ident: "draining".to_string(), - equation: Equation::Scalar("output / DT".to_string()), - documentation: "".to_string(), - units: None, - gf: None, - ai_state: None, - uid: None, - compat: Compat::default(), - }), - Variable::Aux(Aux { - ident: "initial_value".to_string(), - equation: Equation::Scalar("0".to_string()), - documentation: "".to_string(), - units: None, - gf: None, - ai_state: None, - uid: None, - compat: Compat::default(), - }), - Variable::Aux(Aux { - ident: "input".to_string(), - equation: Equation::Scalar("0".to_string()), - documentation: "".to_string(), - units: None, - gf: None, - ai_state: None, - uid: None, - compat: Compat::default(), - }), - Variable::Flow(Flow { - ident: "measuring".to_string(), - equation: Equation::Scalar("input / DT".to_string()), - documentation: "".to_string(), - units: None, - gf: None, - ai_state: None, - uid: None, - compat: Compat::default(), - }), - Variable::Stock(Stock { - ident: "output".to_string(), - equation: Equation::Scalar("initial_value".to_string()), - documentation: "".to_string(), - units: None, - inflows: vec!["measuring".to_string()], - outflows: vec!["draining".to_string()], - ai_state: None, - uid: None, - compat: Compat::default(), - }), - ], - views: vec![View::StockFlow(StockFlow { - name: None, - elements: vec![ - ViewElement::Stock(view_element::Stock { - name: "output".to_string(), - uid: 1, - x: 418_f64, - y: 211_f64, - label_side: LabelSide::Top, - }), - ViewElement::Flow(view_element::Flow { - name: "measuring".to_string(), - uid: 2, - x: 351.75_f64, - y: 211_f64, - label_side: LabelSide::Bottom, - points: vec![ - FlowPoint { - x: 308_f64, - y: 211_f64, - attached_to_uid: Some(8), - }, - FlowPoint { - x: 395.5_f64, - y: 211_f64, - attached_to_uid: Some(1), - }, - ], - }), - ViewElement::Flow(view_element::Flow { - name: "draining".to_string(), - uid: 3, - x: 468.75_f64, - y: 210_f64, - label_side: LabelSide::Top, - points: vec![ - FlowPoint { - x: 440.5_f64, - y: 210_f64, - attached_to_uid: Some(1), - }, - FlowPoint { - x: 521_f64, - y: 210_f64, - attached_to_uid: Some(9), - }, - ], - }), - ViewElement::Link(view_element::Link { - uid: 4, - from_uid: 1, - to_uid: 3, - shape: LinkShape::Arc(39.56_f64), - polarity: None, - }), - ViewElement::Aux(view_element::Aux { - name: "initial value".to_string(), - uid: 5, - x: 351.75_f64, - y: 133_f64, - label_side: LabelSide::Bottom, - }), - ViewElement::Aux(view_element::Aux { - name: "input".to_string(), - uid: 6, - x: 293_f64, - y: 133_f64, - label_side: LabelSide::Bottom, - }), - ViewElement::Link(view_element::Link { - uid: 7, - from_uid: 6, - to_uid: 2, - shape: LinkShape::Arc(48.01299999999998_f64), - polarity: None, - }), - ViewElement::Cloud(view_element::Cloud { - uid: 8, - flow_uid: 2, - x: 308_f64, - y: 211_f64, - }), - ViewElement::Cloud(view_element::Cloud { - uid: 9, - flow_uid: 3, - x: 521_f64, - y: 210_f64, - }), - ], - view_box: Rect { - x: 0_f64, - y: 0_f64, - width: 0_f64, - height: 0_f64, - }, - zoom: 1_f64, - use_lettered_polarity: false, - })], - loop_metadata: vec![], - groups: vec![], - } -} - fn smth1() -> Model { Model { name: "stdlib⁚smth1".to_string(), diff --git a/src/simlin-engine/src/unit_checking_test.rs b/src/simlin-engine/src/unit_checking_test.rs index 666e0b56b..da367a471 100644 --- a/src/simlin-engine/src/unit_checking_test.rs +++ b/src/simlin-engine/src/unit_checking_test.rs @@ -367,9 +367,8 @@ mod tests { fn test_previous_basic_functionality() { // Test PREVIOUS function returns exact previous timestep values per XMILE spec // - // NOTE: The stdlib/previous.stmx implementation uses a stock mechanism - // which may cause smoothing when values change between save steps. - // However, for values sampled at save steps, it works correctly. + // PREVIOUS is intrinsic now: it returns the explicit fallback on the + // first timestep, then the last dt snapshot thereafter. let results = TestProject::new("previous_basic") .with_sim_time(0.0, 2.0, 0.5) // Run from t=0 to t=2 with dt=0.5 diff --git a/src/simlin-engine/src/units_check.rs b/src/simlin-engine/src/units_check.rs index 09fb7f7af..f1d17e46e 100644 --- a/src/simlin-engine/src/units_check.rs +++ b/src/simlin-engine/src/units_check.rs @@ -222,8 +222,25 @@ impl UnitEvaluator<'_> { BuiltinFn::VectorElmMap(source, _) => self.check(source), BuiltinFn::VectorSortOrder(_, _) => Ok(Units::Constant), BuiltinFn::AllocateAvailable(req, _, _) => self.check(req), - // Previous(x) and Init(x) preserve the units of their argument - BuiltinFn::Previous(a) | BuiltinFn::Init(a) => self.check(a), + // Previous(x, init) preserves the units of x and requires + // the fallback to be compatible with it. + BuiltinFn::Previous(a, b) => { + let units = self.check(a)?; + let fallback_units = self.check(b)?; + if !fallback_units.equals(&units) { + return Err(ConsistencyError( + ErrorCode::UnitMismatch, + b.get_loc(), + Some(format!( + "PREVIOUS fallback has units '{}' but expected '{}'", + fallback_units.to_unit_map(), + units.to_unit_map() + )), + )); + } + Ok(units) + } + BuiltinFn::Init(a) => self.check(a), } } Expr2::Subscript(base_name, _, _, loc) => { diff --git a/src/simlin-engine/src/units_infer.rs b/src/simlin-engine/src/units_infer.rs index 2138d9817..862ead145 100644 --- a/src/simlin-engine/src/units_infer.rs +++ b/src/simlin-engine/src/units_infer.rs @@ -518,10 +518,13 @@ impl UnitInferer<'_> { BuiltinFn::AllocateAvailable(req, _, _) => { self.gen_constraints(req, prefix, current_var, constraints) } - // Previous(x) and Init(x) preserve the units of their argument - BuiltinFn::Previous(a) | BuiltinFn::Init(a) => { - self.gen_constraints(a, prefix, current_var, constraints) + // Previous(x, init) and Init(x) preserve the units of the + // lagged/current argument; the fallback must be compatible. + BuiltinFn::Previous(a, b) => { + self.gen_constraints(a, prefix, current_var, constraints)?; + self.gen_constraints(b, prefix, current_var, constraints) } + BuiltinFn::Init(a) => self.gen_constraints(a, prefix, current_var, constraints), }, Expr2::Subscript(base_name, _, _, _) => { // A subscripted expression has the same units as the base array diff --git a/src/simlin-engine/src/variable.rs b/src/simlin-engine/src/variable.rs index b63507d4b..ac5a17e57 100644 --- a/src/simlin-engine/src/variable.rs +++ b/src/simlin-engine/src/variable.rs @@ -517,7 +517,8 @@ where /// Like `parse_var` but accepts a set of module variable identifiers from /// the parent model. When provided, `PREVIOUS(module_var)` in equations will -/// fall through to module expansion instead of compiling to LoadPrev. +/// synthesize a scalar helper aux instead of compiling `LoadPrev` directly +/// against a multi-slot module. pub fn parse_var_with_module_context( dimensions: &[datamodel::Dimension], v: &datamodel::Variable, @@ -823,11 +824,14 @@ impl ClassifyVisitor<'_> { }); if !is_dimension { self.all.insert(id.clone()); + if self.in_init && !self.in_previous { + self.init_referenced.insert(id.to_string()); + } } self.record_ident(id.as_str()); } Expr2::App(builtin, _, _) => match builtin { - BuiltinFn::Previous(arg) => { + BuiltinFn::Previous(arg, fallback) => { if let Expr2::Var(ident, _, _) | Expr2::Subscript(ident, _, _, _) = arg.as_ref() { self.previous_referenced.insert(ident.to_string()); @@ -837,12 +841,13 @@ impl ClassifyVisitor<'_> { self.in_previous = true; self.walk(arg); self.in_previous = old; + + let old = self.in_init; + self.in_init = true; + self.walk(fallback); + self.in_init = old; } BuiltinFn::Init(arg) => { - if let Expr2::Var(ident, _, _) | Expr2::Subscript(ident, _, _, _) = arg.as_ref() - { - self.init_referenced.insert(ident.to_string()); - } let old = self.in_init; self.in_init = true; self.walk(arg); @@ -859,6 +864,9 @@ impl ClassifyVisitor<'_> { }, Expr2::Subscript(id, args, _, _) => { self.all.insert(id.clone()); + if self.in_init && !self.in_previous { + self.init_referenced.insert(id.to_string()); + } self.record_ident(id.as_str()); args.iter().for_each(|arg| self.walk_index(arg)); } @@ -1049,6 +1057,7 @@ fn test_classify_dependencies_matrix() { let loc = Loc::new(0, 1); let const_one = Expr2::Const("1".to_string(), 1.0, loc); + let const_zero = Expr2::Const("0".to_string(), 0.0, loc); let module_inputs_with_input: BTreeSet> = [Ident::new("input")].into_iter().collect(); @@ -1166,7 +1175,10 @@ fn test_classify_dependencies_matrix() { ast: Ast::ApplyToAll( vec![dim1.clone()], Expr2::App( - BuiltinFn::Previous(Box::new(Expr2::Var(Ident::new("b"), None, loc))), + BuiltinFn::Previous( + Box::new(Expr2::Var(Ident::new("b"), None, loc)), + Box::new(const_zero.clone()), + ), None, loc, ), @@ -1197,7 +1209,10 @@ fn test_classify_dependencies_matrix() { vec![IndexExpr2::Range( const_one.clone(), Expr2::App( - BuiltinFn::Previous(Box::new(Expr2::Var(Ident::new("lagged"), None, loc))), + BuiltinFn::Previous( + Box::new(Expr2::Var(Ident::new("lagged"), None, loc)), + Box::new(const_zero.clone()), + ), None, loc, ), @@ -1308,7 +1323,10 @@ fn test_classify_dependencies_matrix() { label: "mixed_prev_current_a2a", ast: Ast::ApplyToAll(vec![dim1.clone()], { let prev = Expr2::App( - BuiltinFn::Previous(Box::new(Expr2::Var(Ident::new("b"), None, loc))), + BuiltinFn::Previous( + Box::new(Expr2::Var(Ident::new("b"), None, loc)), + Box::new(const_zero.clone()), + ), None, loc, ); @@ -1349,7 +1367,10 @@ fn test_classify_dependencies_matrix() { Ident::new("arr"), vec![IndexExpr2::Range( Expr2::App( - BuiltinFn::Previous(Box::new(Expr2::Var(Ident::new("b"), None, loc))), + BuiltinFn::Previous( + Box::new(Expr2::Var(Ident::new("b"), None, loc)), + Box::new(const_zero.clone()), + ), None, loc, ), @@ -1399,7 +1420,10 @@ fn test_classify_dependencies_matrix() { label: "both_lagged_a2a", ast: Ast::ApplyToAll(vec![dim1.clone()], { let prev = Expr2::App( - BuiltinFn::Previous(Box::new(Expr2::Var(Ident::new("b"), None, loc))), + BuiltinFn::Previous( + Box::new(Expr2::Var(Ident::new("b"), None, loc)), + Box::new(const_zero.clone()), + ), None, loc, ); @@ -1449,7 +1473,10 @@ fn test_classify_dependencies_matrix() { Ident::new("arr"), vec![IndexExpr2::Range( Expr2::App( - BuiltinFn::Previous(Box::new(Expr2::Var(Ident::new("x"), None, loc))), + BuiltinFn::Previous( + Box::new(Expr2::Var(Ident::new("x"), None, loc)), + Box::new(const_zero.clone()), + ), None, loc, ), diff --git a/src/simlin-engine/src/vm.rs b/src/simlin-engine/src/vm.rs index 10f76a528..25d5837a2 100644 --- a/src/simlin-engine/src/vm.rs +++ b/src/simlin-engine/src/vm.rs @@ -241,9 +241,8 @@ pub struct Vm { // Used by LoadInitial opcode to freeze a variable's initial value. initial_values: Box<[f64]>, // Snapshot of curr[] taken after stocks but before the time advance - // each timestep. LoadPrev reads from this buffer so that - // PREVIOUS(x) returns the value of x from the previous timestep, - // matching the stdlib module-based PREVIOUS behavior. + // each timestep. LoadPrev reads from this buffer after the first + // timestep; when TIME == INITIAL_TIME it returns its fallback instead. prev_values: Box<[f64]>, } @@ -868,8 +867,8 @@ impl Vm { // During initials, LoadInitial falls back to curr[] (which IS the // initial value being computed). The snapshot hasn't been captured yet. initial_values: &self.initial_values, - // During initials, prev_values is zero-initialized so PREVIOUS(x) - // yields the 1-arg default at t=0. + // LoadPrev chooses its fallback when TIME == INITIAL_TIME, so a + // fresh zeroed prev_values buffer is sufficient here. prev_values: &mut self.prev_values, }; @@ -1064,12 +1063,17 @@ impl Vm { Opcode::LoadVar { off } => { stack.push(curr[module_off + *off as usize]); } - // LoadPrev reads from the prev_values snapshot taken after - // stocks but before the time advance each timestep. During - // initials this buffer is zero-initialized. + // LoadPrev pops the caller-provided fallback and uses it when + // TIME == INITIAL_TIME; otherwise it reads from prev_values. Opcode::LoadPrev { off } => { + let fallback = stack.pop(); let abs_off = module_off + *off as usize; - stack.push(prev_values[abs_off]); + let value = if crate::float::approx_eq(curr[TIME_OFF], curr[INITIAL_TIME_OFF]) { + fallback + } else { + prev_values[abs_off] + }; + stack.push(value); } // LoadInitial reads from the initial-value buffer captured at t=0. // During the initials phase, the snapshot hasn't been taken yet, diff --git a/stdlib/previous.stmx b/stdlib/previous.stmx deleted file mode 100644 index 0ea0c6093..000000000 --- a/stdlib/previous.stmx +++ /dev/null @@ -1,61 +0,0 @@ - - -
- previous - Simlin - Simlin -
- - 1 - 13 -
4
- - - - - initial_value - measuring - draining - - - input / DT - - - output / DT - - - 0 - - - 0 - - - - - - - - - - - - - - - - - - - output - draining - - - - - input - measuring - - - - - From 1dd24802be998c13a2fcde0d3345a410d1798e84 Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Tue, 10 Mar 2026 19:56:09 -0700 Subject: [PATCH 2/4] engine: address review feedback on previous intrinsic Add explanatory comments for the interpreter non-Var PREVIOUS fallback invariant and the dual desugaring of unary PREVIOUS in expr1.rs. Clean up the dead `let _ = args` binding in equation_is_stdlib_call and remove the unused model_name parameter from classify_module_for_ltm. --- src/simlin-engine/src/ast/expr1.rs | 3 +++ src/simlin-engine/src/interpreter.rs | 4 ++++ src/simlin-engine/src/ltm.rs | 10 +++------- src/simlin-engine/src/ltm_augment.rs | 4 ++-- src/simlin-engine/src/model.rs | 3 +-- 5 files changed, 13 insertions(+), 11 deletions(-) diff --git a/src/simlin-engine/src/ast/expr1.rs b/src/simlin-engine/src/ast/expr1.rs index 7d16d4473..d5ebd5966 100644 --- a/src/simlin-engine/src/ast/expr1.rs +++ b/src/simlin-engine/src/ast/expr1.rs @@ -252,6 +252,9 @@ impl Expr1 { "vector_elm_map" => check_arity!(VectorElmMap, 2), "vector_sort_order" => check_arity!(VectorSortOrder, 2), "allocate_available" => check_arity!(AllocateAvailable, 3), + // Unary PREVIOUS(x) desugars to PREVIOUS(x, 0). + // builtins_visitor may have already added the fallback + // at Expr0 level, so both 1-arg and 2-arg forms are valid. "previous" => { if args.len() == 1 { let a = args.remove(0); diff --git a/src/simlin-engine/src/interpreter.rs b/src/simlin-engine/src/interpreter.rs index 4ff47ef53..dec670884 100644 --- a/src/simlin-engine/src/interpreter.rs +++ b/src/simlin-engine/src/interpreter.rs @@ -1372,6 +1372,10 @@ impl ModuleEvaluator<'_> { let prev_vals = self.sim.prev_values.borrow(); match arg.as_ref() { Expr::Var(off, _) => prev_vals[self.off + *off], + // builtins_visitor rewrites non-scalar PREVIOUS args + // through helper auxes, so only Var nodes should + // reach here in practice. If one slips through, + // evaluating the current value is the safe fallback. _ => { drop(prev_vals); self.eval(arg) diff --git a/src/simlin-engine/src/ltm.rs b/src/simlin-engine/src/ltm.rs index a377d3db3..89d0599b3 100644 --- a/src/simlin-engine/src/ltm.rs +++ b/src/simlin-engine/src/ltm.rs @@ -139,10 +139,7 @@ pub(crate) enum ModuleLtmRole { /// /// Dynamic modules contain stocks and need composite link scores. /// Stateless modules are passthroughs. -pub(crate) fn classify_module_for_ltm( - _model_name: &Ident, - module_model: &ModelStage1, -) -> ModuleLtmRole { +pub(crate) fn classify_module_for_ltm(module_model: &ModelStage1) -> ModuleLtmRole { if module_model .variables .values() @@ -361,8 +358,7 @@ impl CausalGraph { { // Build internal graph for this module instance if we have the model if let Some(module_model) = project.models.get(model_name) - && classify_module_for_ltm(model_name, module_model) - == ModuleLtmRole::DynamicModule + && classify_module_for_ltm(module_model) == ModuleLtmRole::DynamicModule { // Recursively build graph for the module let module_graph = CausalGraph::from_model(module_model, project)?; @@ -3264,7 +3260,7 @@ mod tests { .expect("should have stdlib⁚smth1 model"); assert_eq!( - classify_module_for_ltm(&smth1_ident, smth1_model), + classify_module_for_ltm(smth1_model), ModuleLtmRole::DynamicModule ); } diff --git a/src/simlin-engine/src/ltm_augment.rs b/src/simlin-engine/src/ltm_augment.rs index 9f3fd4977..a7f9817a1 100644 --- a/src/simlin-engine/src/ltm_augment.rs +++ b/src/simlin-engine/src/ltm_augment.rs @@ -157,7 +157,7 @@ pub(crate) fn compute_composite_ports(project: &Project) -> CompositePortMap { if !model.implicit { continue; } - if classify_module_for_ltm(model_name, model) != ModuleLtmRole::DynamicModule { + if classify_module_for_ltm(model) != ModuleLtmRole::DynamicModule { continue; } let graph = match CausalGraph::from_model(model, project) { @@ -248,7 +248,7 @@ fn generate_ltm_variables_inner( if !model.implicit { continue; } - if classify_module_for_ltm(model_name, model) != ModuleLtmRole::DynamicModule { + if classify_module_for_ltm(model) != ModuleLtmRole::DynamicModule { continue; } let module_vars = generate_module_internal_ltm_variables(model_name, model, project); diff --git a/src/simlin-engine/src/model.rs b/src/simlin-engine/src/model.rs index 5bcfbf966..114decbb8 100644 --- a/src/simlin-engine/src/model.rs +++ b/src/simlin-engine/src/model.rs @@ -857,9 +857,8 @@ pub(crate) fn equation_is_stdlib_call(eqn: &datamodel::Equation) -> bool { return false; }; match &ast { - Expr0::App(crate::builtins::UntypedBuiltinFn(func, args), _) => { + Expr0::App(crate::builtins::UntypedBuiltinFn(func, _args), _) => { let func_lower = func.to_lowercase(); - let _ = args; crate::builtins::is_stdlib_module_function(&func_lower) } _ => false, From 2c79bec81f5827ba51f817ac07881bc218d770ac Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Tue, 10 Mar 2026 20:04:09 -0700 Subject: [PATCH 3/4] engine: fix LoadPrev stack effect and PREVIOUS unit inference Fix two bugs found by codex review: 1. LoadPrev pops a fallback value from the stack before pushing its result, so its stack effect is (1,1) not (0,1). The incorrect metadata made ByteCode::max_stack_depth() overestimate capacity. 2. In unit inference, Previous(a, b) was returning the units of the fallback (b) instead of the lagged argument (a). For unary PREVIOUS(x) desugared to PREVIOUS(x, 0), this returned Constant instead of propagating x's units, breaking inference for variables that depend solely on PREVIOUS for their unit information. --- src/simlin-engine/src/bytecode.rs | 12 +++++- src/simlin-engine/src/units_infer.rs | 64 ++++++++++++++++++++++++++-- 2 files changed, 72 insertions(+), 4 deletions(-) diff --git a/src/simlin-engine/src/bytecode.rs b/src/simlin-engine/src/bytecode.rs index 884f82bc7..f7c6ada38 100644 --- a/src/simlin-engine/src/bytecode.rs +++ b/src/simlin-engine/src/bytecode.rs @@ -894,11 +894,14 @@ impl Opcode { // Constants/variables: push 1 Opcode::LoadConstant { .. } | Opcode::LoadVar { .. } - | Opcode::LoadPrev { .. } | Opcode::LoadInitial { .. } | Opcode::LoadGlobalVar { .. } | Opcode::LoadModuleInput { .. } => (0, 1), + // LoadPrev pops the caller-provided fallback, then pushes + // either the fallback (at t=INITIAL_TIME) or prev_values[off]. + Opcode::LoadPrev { .. } => (1, 1), + // Legacy subscript: PushSubscriptIndex pops an index from the // arithmetic stack and appends it to a separate subscript_index // SmallVec (not the arithmetic stack). Multiple PushSubscriptIndex @@ -1452,6 +1455,13 @@ mod tests { ); } + #[test] + fn test_stack_effect_load_prev_pops_fallback() { + // LoadPrev pops the fallback value from the stack, then + // pushes the result (either the fallback or prev_values[off]). + assert_eq!((Opcode::LoadPrev { off: 0 }).stack_effect(), (1, 1)); + } + #[test] fn test_stack_effect_builtins() { assert_eq!( diff --git a/src/simlin-engine/src/units_infer.rs b/src/simlin-engine/src/units_infer.rs index 862ead145..bd8410777 100644 --- a/src/simlin-engine/src/units_infer.rs +++ b/src/simlin-engine/src/units_infer.rs @@ -518,11 +518,12 @@ impl UnitInferer<'_> { BuiltinFn::AllocateAvailable(req, _, _) => { self.gen_constraints(req, prefix, current_var, constraints) } - // Previous(x, init) and Init(x) preserve the units of the + // Previous(x, fallback) and Init(x) preserve the units of the // lagged/current argument; the fallback must be compatible. BuiltinFn::Previous(a, b) => { - self.gen_constraints(a, prefix, current_var, constraints)?; - self.gen_constraints(b, prefix, current_var, constraints) + let a_units = self.gen_constraints(a, prefix, current_var, constraints)?; + self.gen_constraints(b, prefix, current_var, constraints)?; + Ok(a_units) } BuiltinFn::Init(a) => self.gen_constraints(a, prefix, current_var, constraints), }, @@ -1147,6 +1148,63 @@ pub(crate) fn infer( units.infer(model) } +/// PREVIOUS(x) desugars to PREVIOUS(x, 0). The inferred units should +/// come from x, not the fallback 0 constant. +#[test] +fn test_previous_infers_units_from_lagged_arg() { + let sim_specs = sim_specs_with_units("parsec"); + let units_ctx = Context::new_with_builtins(&[], &sim_specs).unwrap(); + + // position has explicit units "widget". prev_pos has no declared + // units; inference should propagate "widget" from position + // through PREVIOUS(position, 0). + let test_case: &[(crate::datamodel::Variable, &str)] = &[ + (x_aux("position", "10", Some("widget")), "widget"), + (x_aux("prev_pos", "PREVIOUS(position)", None), "widget"), + ]; + + let expected = test_case + .iter() + .map(|(var, units)| (var.get_ident(), *units)) + .collect::>(); + let vars = test_case + .iter() + .map(|(var, _)| var) + .cloned() + .collect::>(); + let model = x_model("main", vars); + let project_datamodel = x_project(sim_specs.clone(), &[model]); + + for _ in 0..64 { + let mut results: UnitResult, UnitMap>> = Ok(Default::default()); + let db = crate::db::SimlinDb::default(); + let sync = crate::db::sync_from_datamodel(&db, &project_datamodel); + let _project = crate::project::Project::from_salsa( + project_datamodel.clone(), + &db, + sync.project, + |models, units_ctx, model| { + results = infer(models, units_ctx, model); + }, + ); + let results = results.unwrap(); + for (ident, expected_units) in expected.iter() { + let expected_units: UnitMap = + crate::units::parse_units(&units_ctx, Some(expected_units)) + .unwrap() + .unwrap(); + if let Some(computed_units) = results.get(&*canonicalize(ident)) { + assert_eq!( + expected_units, *computed_units, + "variable '{ident}': expected {expected_units:?} but got {computed_units:?}" + ); + } else { + panic!("inference results don't contain variable '{ident}'"); + } + } + } +} + #[test] fn test_constraint_generation_consistency() { use crate::common::canonicalize; From 0a30c7935852b47db870eae6256a4e9ef3dd980b Mon Sep 17 00:00:00 2001 From: Bobby Powers Date: Tue, 10 Mar 2026 20:29:54 -0700 Subject: [PATCH 4/4] engine: constrain PREVIOUS fallback units to match lagged arg Add an explicit equality constraint between the lagged argument and fallback in PREVIOUS during unit inference, analogous to Max/Min handling. Without this, equations like PREVIOUS(x, seed) could leave the fallback unconstrained or with inconsistent units even though units_check requires compatibility, leading to spurious diagnostics. Also reflows an awkward comment in db_ltm.rs. --- src/simlin-engine/src/db_ltm.rs | 6 +-- src/simlin-engine/src/units_infer.rs | 65 +++++++++++++++++++++++++++- 2 files changed, 67 insertions(+), 4 deletions(-) diff --git a/src/simlin-engine/src/db_ltm.rs b/src/simlin-engine/src/db_ltm.rs index 1479d455b..ec7836079 100644 --- a/src/simlin-engine/src/db_ltm.rs +++ b/src/simlin-engine/src/db_ltm.rs @@ -788,9 +788,9 @@ pub(super) fn compile_ltm_equation_fragment( let inputs = BTreeSet::new(); let mut module_models = model_module_map(db, model, project).clone(); - // Merge LTM implicit module references from LTM equation parsing into - // the module_models map so the compiler - // context can resolve module_var_name -> sub_model_name lookups. + // Merge LTM implicit module references from LTM equation parsing into the + // module_models map so the compiler context can resolve module_var_name -> + // sub_model_name lookups. if !implicit_module_refs.is_empty() { let current_model_modules = module_models.entry(model_name_ident.clone()).or_default(); for (var_ident, (sub_model_name, _input_set)) in &implicit_module_refs { diff --git a/src/simlin-engine/src/units_infer.rs b/src/simlin-engine/src/units_infer.rs index bd8410777..2b4f78182 100644 --- a/src/simlin-engine/src/units_infer.rs +++ b/src/simlin-engine/src/units_infer.rs @@ -522,7 +522,19 @@ impl UnitInferer<'_> { // lagged/current argument; the fallback must be compatible. BuiltinFn::Previous(a, b) => { let a_units = self.gen_constraints(a, prefix, current_var, constraints)?; - self.gen_constraints(b, prefix, current_var, constraints)?; + let b_units = self.gen_constraints(b, prefix, current_var, constraints)?; + // Constrain fallback to match the lagged argument's units, + // analogous to Max/Min handling. + if let Units::Explicit(ref a_map) = a_units + && let Units::Explicit(b_map) = b_units + { + let loc = a.get_loc().union(&b.get_loc()); + constraints.push(LocatedConstraint::new( + combine(UnitOp::Div, a_map.clone(), b_map), + current_var, + Some(loc), + )); + } Ok(a_units) } BuiltinFn::Init(a) => self.gen_constraints(a, prefix, current_var, constraints), @@ -1205,6 +1217,57 @@ fn test_previous_infers_units_from_lagged_arg() { } } +/// PREVIOUS(x, fallback) should propagate units from x to fallback +/// during inference, so a fallback with incompatible declared units +/// is detected as a mismatch. +#[test] +fn test_previous_constrains_fallback_units() { + let sim_specs = sim_specs_with_units("parsec"); + + // "seed" has wrong units ("wallop" vs "widget"). PREVIOUS(position, seed) + // should fail inference because the fallback is constrained to match + // the lagged argument. + let test_case: &[(crate::datamodel::Variable, &str)] = &[ + (x_aux("position", "10", Some("widget")), "widget"), + (x_aux("seed", "0", Some("wallop")), "wallop"), + ( + x_aux("prev_pos", "PREVIOUS(position, seed)", None), + "widget", + ), + ]; + + let vars = test_case + .iter() + .map(|(var, _unit)| var) + .cloned() + .collect::>(); + let model = x_model("main", vars); + let project_datamodel = x_project(sim_specs, &[model]); + + for _ in 0..64 { + let mut results: UnitResult, UnitMap>> = + Err(UnitError::InferenceError { + code: ErrorCode::UnitMismatch, + sources: vec![], + details: None, + }); + let db = crate::db::SimlinDb::default(); + let sync = crate::db::sync_from_datamodel(&db, &project_datamodel); + let _project = crate::project::Project::from_salsa( + project_datamodel.clone(), + &db, + sync.project, + |models, units_ctx, model| { + results = infer(models, units_ctx, model); + }, + ); + assert!( + results.is_err(), + "PREVIOUS(widget, wallop) should fail unit inference" + ); + } +} + #[test] fn test_constraint_generation_consistency() { use crate::common::canonicalize;