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..d5ebd5966 100644 --- a/src/simlin-engine/src/ast/expr1.rs +++ b/src/simlin-engine/src/ast/expr1.rs @@ -252,7 +252,22 @@ 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), + // 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); + 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 +434,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..f7c6ada38 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, }, @@ -895,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 @@ -1453,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/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..ec7836079 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,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 (PREVIOUS instances from LTM - // equation expansion) 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 { @@ -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..dec670884 100644 --- a/src/simlin-engine/src/interpreter.rs +++ b/src/simlin-engine/src/interpreter.rs @@ -1364,29 +1364,19 @@ 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], + // 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. _ => { - // 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..89d0599b3 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,9 @@ 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. -pub(crate) fn classify_module_for_ltm( - model_name: &Ident, - module_model: &ModelStage1, -) -> ModuleLtmRole { - let name = model_name.as_str(); - if name == "stdlib⁚previous" { - return ModuleLtmRole::Infrastructure; - } +/// Dynamic modules contain stocks and need composite link scores. +/// Stateless modules are passthroughs. +pub(crate) fn classify_module_for_ltm(module_model: &ModelStage1) -> ModuleLtmRole { if module_model .variables .values() @@ -369,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)?; @@ -3254,32 +3242,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; @@ -3298,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 99398d181..a7f9817a1 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; @@ -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); @@ -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..114decbb8 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 @@ -858,14 +857,9 @@ 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(); - // 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) - } + crate::builtins::is_stdlib_module_function(&func_lower) } _ => false, } @@ -882,21 +876,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 +958,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 +971,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 +1589,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 +1628,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 +1722,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 +1746,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 +1762,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 +1776,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 +1792,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 +1810,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 +1819,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 +1847,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..2b4f78182 100644 --- a/src/simlin-engine/src/units_infer.rs +++ b/src/simlin-engine/src/units_infer.rs @@ -518,10 +518,26 @@ 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, fallback) and Init(x) preserve the units of the + // lagged/current argument; the fallback must be compatible. + BuiltinFn::Previous(a, b) => { + let a_units = self.gen_constraints(a, 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), }, Expr2::Subscript(base_name, _, _, _) => { // A subscripted expression has the same units as the base array @@ -1144,6 +1160,114 @@ 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}'"); + } + } + } +} + +/// 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; 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 - - - - -