diff --git a/docs/design-plans/2026-03-07-assert-compiles-migration.md b/docs/design-plans/2026-03-07-assert-compiles-migration.md new file mode 100644 index 00000000..d11b110f --- /dev/null +++ b/docs/design-plans/2026-03-07-assert-compiles-migration.md @@ -0,0 +1,115 @@ +# Assert-Compiles Migration Design + +## Summary + +Simlin's simulation engine has two compilation paths: a legacy monolithic path (`compile()`) and the current incremental path (`compile_incremental()`) built on salsa for fine-grained caching. Twenty-six tests still exercise the monolithic path through the `assert_compiles()` helper, keeping the old code alive. This plan eliminates that technical debt by migrating every test to `assert_compiles_incremental()` and then deleting the dead monolithic helpers entirely. + +Twenty-three of the 26 tests already pass on the incremental path and need only a mechanical one-line change. The remaining three expose two feature gaps in the incremental compiler: (1) the `@N` position syntax (e.g., `arr[@1]`) fails when the left-hand side is scalar because the subscript lowering pipeline does not resolve positional indices to concrete element names outside of an apply-to-all context, and (2) the `MEAN` builtin rejects dynamic-range subscripts that other array-reduce builtins like `SUM` and `STDDEV` already handle, due to an overly narrow pattern match in the codegen layer. The plan fixes both gaps with targeted changes -- resolving `@N` to named elements early in the AST lowering chain, and extracting a shared `emit_array_reduce` helper that all six array-reduce builtins use -- then switches the remaining tests and removes the old code. + +## Definition of Done + +All 26 tests currently using `assert_compiles()` are switched to `assert_compiles_incremental()` and pass. This requires: (1) switching the 23 already-working tests, (2) implementing incremental support for `@N` position syntax and `MEAN` with dynamic ranges so the remaining 3 tests pass, and (3) deleting `assert_compiles()` and any code that becomes dead after its removal (but keeping the AST interpreter and its supporting infrastructure). + +## Acceptance Criteria + +### assert-compiles-migration.AC1: @N position syntax works on incremental path +- **assert-compiles-migration.AC1.1 Success:** `arr[@1]` in scalar context compiles and selects the first dimension element +- **assert-compiles-migration.AC1.2 Success:** `cube[@1, *, @3]` with mixed position/wildcard compiles on incremental path +- **assert-compiles-migration.AC1.3 Failure:** `arr[@0]` produces a compile error (1-based, 0 is invalid) +- **assert-compiles-migration.AC1.4 Failure:** `arr[@N]` where N exceeds dimension size produces a compile error + +### assert-compiles-migration.AC2: MEAN with dynamic ranges works on incremental path +- **assert-compiles-migration.AC2.1 Success:** `MEAN(data[start_idx:end_idx])` with variable bounds compiles and simulates correctly +- **assert-compiles-migration.AC2.2 Success:** Existing SUM, SIZE, STDDEV, VMIN, VMAX behavior unchanged after refactoring to shared helper + +### assert-compiles-migration.AC3: assert_compiles fully removed +- **assert-compiles-migration.AC3.1 Success:** All 26 formerly-monolithic tests pass with `assert_compiles_incremental()` +- **assert-compiles-migration.AC3.2 Success:** `assert_compiles()` method deleted from `test_common.rs` +- **assert-compiles-migration.AC3.3 Success:** `compile()` method deleted from `test_common.rs` (if no other callers) +- **assert-compiles-migration.AC3.4 Success:** `cargo test -p simlin-engine` passes with zero regressions + +## Glossary + +- **Incremental compilation / salsa**: The engine's current compilation strategy, where each variable is compiled independently and results are cached using salsa (a Rust framework for demand-driven incremental computation). Only variables whose inputs change are recompiled. +- **Monolithic compilation**: The older compilation path that compiles an entire project in one pass without caching. It is the code being removed by this migration. +- **AST lowering chain (Expr0 -> Expr1 -> Expr2 -> Expr3 -> Expr)**: The sequence of intermediate expression representations that an equation passes through during compilation. Each stage resolves more syntactic sugar and subscript information until the final `Expr` form is ready for code generation. +- **@N position syntax / DimPosition**: A subscript notation (e.g., `@1`, `@3`) that refers to the Nth element of a dimension by numeric position rather than by name. `DimPosition` is the corresponding AST node. +- **A2A (apply-to-all)**: A system dynamics concept where a single equation defines all elements of an arrayed variable. In the compiler, A2A context is tracked via `active_dimension`. +- **Scalar context**: The opposite of A2A context -- when the left-hand side of an equation is a plain (non-arrayed) variable, so there is no active dimension to iterate over. +- **Array-reduce builtins**: Built-in functions (SUM, SIZE, STDDEV, VMIN, VMAX, MEAN) that collapse an array argument into a single scalar value. +- **`walk_expr_as_view` / `PopView`**: Internal codegen methods that set up and tear down a "view" -- a runtime slice of an array -- so that an array opcode can operate over the selected elements. +- **`ViewRangeDynamic`**: A VM opcode that constructs an array view whose bounds are determined at runtime (from variable values) rather than at compile time. +- **`lower_index_expr3`**: The function in `context.rs` responsible for resolving subscript expressions during the Expr3 lowering stage, including dimension positions and dynamic subscripts. +- **`codegen.rs`**: The compiler module that translates the final AST (`Expr`) into stack-based bytecode instructions for the VM. + +## Architecture + +The migration has three independent workstreams that converge on deleting `assert_compiles()`: + +1. **Trivial test switch (23 tests):** `builtins_visitor.rs` (20), `db_tests.rs` (2), `db_prev_init_tests.rs` (1) -- change `assert_compiles()` to `assert_compiles_incremental()` with no compiler changes. + +2. **@N position syntax in scalar context:** When `arr[@1]` appears with a scalar LHS (no active A2A dimension), `lower_index_expr3` in `context.rs` currently returns `ArrayReferenceNeedsExplicitSubscripts`. The fix resolves `DimPosition(n)` to a concrete element name early in the lowering pipeline: look up the referenced dimension's element list and replace `@N` with `elements[n-1]`. No VM changes or new opcodes needed. A2A context (`matrix[@2, @1]`) already works via the existing dynamic subscript path. + +3. **MEAN with dynamic ranges + array-reduce abstraction:** The `BuiltinFn::Mean` handler in `codegen.rs` gates the array path on `matches!(arg, Expr::StaticSubscript | Expr::TempArray)`, missing `Expr::Subscript` with `Range` indices. SUM, SIZE, and STDDEV work because they call `walk_expr_as_view` unconditionally. The fix introduces a shared `emit_array_reduce` helper and a robust `is_array_expr` check, eliminating the 6x copy-paste of `walk_expr_as_view + ArrayOp + PopView` across SUM, SIZE, STDDEV, VMIN, VMAX, and MEAN. + +4. **Cleanup:** Delete `assert_compiles()` and its sole dependency `compile()` from `test_common.rs`. Verify no other callers exist before removing. + +## Existing Patterns + +The incremental compilation pipeline (`compile_var_fragment` in `db.rs`) compiles one variable at a time through the AST lowering chain: `Expr0 -> Expr1 -> Expr2 -> Expr3 -> Expr`. Subscript resolution happens in `context.rs` via `normalize_subscripts3` (static path producing `IndexOp`) and `lower_index_expr3` (dynamic fallback). The `@N` fix follows the existing pattern of resolving indices to concrete element names during lowering. + +The array-reduce builtins (SUM, SIZE, STDDEV, VMIN, VMAX, MEAN) all share the same terminal pattern: `walk_expr_as_view(arg)` then emit an `Array*` opcode then `PopView`. This pattern is currently duplicated 6 times with no shared helper. The refactoring extracts this into a single method, which is a new pattern but one that follows the codebase's general preference for reducing duplication. + +The `ViewRangeDynamic` opcode already exists in the VM and handles dynamic range bounds. The MEAN fix leverages this existing infrastructure. + +## Implementation Phases + + +### Phase 1: @N Position Syntax in Scalar Context + +**Goal:** Make `arr[@N]` work on the incremental compilation path when the LHS is a scalar variable. + +**Components:** +- `src/simlin-engine/src/compiler/context.rs` -- modify `lower_index_expr3` to resolve `IndexExpr3::DimPosition(pos)` to `IndexExpr3::Named(elements[pos-1])` when `active_dimension.is_none()`, with a compile error for out-of-range positions +- `src/simlin-engine/src/array_tests.rs` -- switch `dimension_position_single` (line 342) and `dimension_position_and_wildcard` (line 1366) from `assert_compiles()` to `assert_compiles_incremental()` + +**Dependencies:** None + +**Done when:** `dimension_position_single` and `dimension_position_and_wildcard` pass with `assert_compiles_incremental()`. Covers `assert-compiles-migration.AC1.*`. + + + +### Phase 2: Array-Reduce Abstraction and MEAN Dynamic Ranges + +**Goal:** Unify the array-reduce codegen pattern across all builtins and fix MEAN with dynamic range arguments. + +**Components:** +- `src/simlin-engine/src/compiler/codegen.rs` -- extract `emit_array_reduce(arg, opcode)` helper; refactor SUM, SIZE, STDDEV, VMIN, VMAX, MEAN handlers to use it; fix MEAN's array detection to include `Expr::Subscript` with `Range` indices +- `src/simlin-engine/src/array_tests.rs` -- switch `mean_with_dynamic_range` (line 2023) from `assert_compiles()` to `assert_compiles_incremental()` + +**Dependencies:** None (independent of Phase 1) + +**Done when:** `mean_with_dynamic_range` passes with `assert_compiles_incremental()`. All existing array-reduce tests continue to pass. Covers `assert-compiles-migration.AC2.*`. + + + +### Phase 3: Bulk Test Migration and Cleanup + +**Goal:** Switch all remaining 23 tests and delete `assert_compiles()`. + +**Components:** +- `src/simlin-engine/src/builtins_visitor.rs` -- switch 20 `assert_compiles()` calls to `assert_compiles_incremental()` +- `src/simlin-engine/src/db_tests.rs` -- switch 2 `assert_compiles()` calls to `assert_compiles_incremental()` +- `src/simlin-engine/src/db_prev_init_tests.rs` -- switch 1 `assert_compiles()` call to `assert_compiles_incremental()` +- `src/simlin-engine/src/test_common.rs` -- delete `assert_compiles()` method and `compile()` method (if no other callers remain) + +**Dependencies:** Phases 1 and 2 (all tests must pass on incremental path first) + +**Done when:** Zero calls to `assert_compiles()` remain in the codebase. `assert_compiles` and `compile()` are deleted from `test_common.rs`. All engine tests pass. Covers `assert-compiles-migration.AC3.*`. + + +## Additional Considerations + +**Out-of-range @N:** If a user writes `arr[@5]` on a 3-element dimension, the compiler should emit a clear error at compile time rather than silently producing wrong results. This is a new error path that needs a test. + +**Other builtins with dynamic ranges:** The array-reduce abstraction in Phase 2 should be verified against all builtins that accept array arguments, not just MEAN. If any other builtin has the same narrow `is_array` check, fix it in the same pass. diff --git a/docs/implementation-plans/2026-03-08-assert-compiles-migration/phase_01.md b/docs/implementation-plans/2026-03-08-assert-compiles-migration/phase_01.md new file mode 100644 index 00000000..4762efe5 --- /dev/null +++ b/docs/implementation-plans/2026-03-08-assert-compiles-migration/phase_01.md @@ -0,0 +1,125 @@ +# Assert-Compiles Migration -- Phase 1: @N Position Syntax in Scalar Context + +**Goal:** Make `arr[@N]` work on the incremental compilation path when the LHS is a scalar variable, and when mixed with wildcards. + +**Architecture:** Modify `lower_index_expr3` in `context.rs` to resolve `DimPosition(pos)` to a concrete 1-based offset when in scalar context (no active A2A dimension), rather than returning `ArrayReferenceNeedsExplicitSubscripts`. For mixed contexts (DimPosition + wildcard, like `cube[@1, *, @3]`), the same resolution applies when the active subscript at position `pos` doesn't match the target dimension. + +**Tech Stack:** Rust (simlin-engine) + +**Scope:** Phase 1 of 3 from design plan + +**Codebase verified:** 2026-03-08 + +**Reference files for executor:** +- `/home/bpowers/src/simlin/src/simlin-engine/CLAUDE.md` -- engine architecture and module map +- `/home/bpowers/src/simlin/CLAUDE.md` -- project-wide development standards + +--- + +## Acceptance Criteria Coverage + +This phase implements and tests: + +### assert-compiles-migration.AC1: @N position syntax works on incremental path +- **assert-compiles-migration.AC1.1 Success:** `arr[@1]` in scalar context compiles and selects the first dimension element +- **assert-compiles-migration.AC1.2 Success:** `cube[@1, *, @3]` with mixed position/wildcard compiles on incremental path +- **assert-compiles-migration.AC1.3 Failure:** `arr[@0]` produces a compile error (1-based, 0 is invalid) +- **assert-compiles-migration.AC1.4 Failure:** `arr[@N]` where N exceeds dimension size produces a compile error + +--- + +## Codebase Verification Findings + +- **Confirmed:** `lower_index_expr3` at `src/simlin-engine/src/compiler/context.rs:2062-2070` handles `IndexExpr3::DimPosition(pos, dim_loc)` starting at line 2131. When `self.active_dimension.is_none()`, it returns `sim_err!(ArrayReferenceNeedsExplicitSubscripts, ...)` at line 2134. This is the fix site. +- **Discrepancy:** Design says "resolve to `IndexExpr3::Named(elements[pos-1])`" but `IndexExpr3::Named` does not exist. The correct approach: resolve to `SubscriptIndex::Single(Expr::Const((pos) as f64, loc))` (1-based offset), consistent with how the A2A path resolves at line 2148. +- **Confirmed:** `dimension_position_single` test at `array_tests.rs:334` (inside `mod dimension_position_tests` starting at line 330). `dimension_position_and_wildcard` test at `array_tests.rs:1357`. +- **Confirmed:** `Dimension::Named(_, named_dim)` has `named_dim.elements: Vec` and `Dimension::Indexed(_, size)` has `size: u32`. `Dimension::len()` exists at `dimensions.rs:108` and returns the element count for both variants. +- **Confirmed:** For out-of-range errors, existing code uses `ErrorCode::MismatchedDimensions` (consistent with A2A path at line 2142). No dedicated `ArrayIndexOutOfBounds` variant. +- **Gap:** No `assert_compile_error_incremental()` method exists on `TestProject`. For error tests (AC1.3, AC1.4), call `compile_incremental()` and verify the error message contains the expected context (e.g., the variable name) to avoid false positives from unrelated compilation errors. + +--- + + + + +### Task 1: Modify `lower_index_expr3` to resolve scalar @N + +**Verifies:** assert-compiles-migration.AC1.1, assert-compiles-migration.AC1.2 + +**Files:** +- Modify: `src/simlin-engine/src/compiler/context.rs:2131-2161` (the `DimPosition` match arm in `lower_index_expr3`) + +**Implementation:** + +Replace the `IndexExpr3::DimPosition(pos, dim_loc)` match arm (lines 2131-2161) with logic that handles three cases: + +1. **Scalar context** (`self.active_dimension.is_none()`): Resolve `@N` to a concrete 1-based element offset in `dims[i]`. Check bounds: `pos == 0` or `pos > dim_size` produces `MismatchedDimensions`. Otherwise return `SubscriptIndex::Single(Expr::Const(*pos as f64, *dim_loc))`. + +2. **A2A context, within active subscript range, subscript matches dimension** (existing behavior): `active_subscripts[pos-1]` resolved via `dim.get_offset(subscript)` returns the concrete offset. This is the existing dimension-reordering path (e.g., `matrix[@2, @1]`). + +3. **A2A context, but subscript doesn't match dimension or pos out of range** (new fallback for mixed cases like `cube[@1, *, @3]`): Fall back to the same concrete element offset as the scalar case. + +Bounds check (applies to scalar context and A2A fallback): +```rust +let pos_val = *pos as usize; +if pos_val == 0 || pos_val > dims[i].len() { + return sim_err!(MismatchedDimensions, id.as_str().to_string()); +} +``` + +**Verification:** +Run: `cargo test -p simlin-engine dimension_position` +Expected: Existing `dimension_position_reorder` and `dimension_position_3d` tests still pass (they use the A2A path unchanged). + +**Commit:** `engine: resolve @N position syntax to concrete offset in scalar context` + + + +### Task 2: Switch existing tests to `assert_compiles_incremental()` + +**Verifies:** assert-compiles-migration.AC1.1, assert-compiles-migration.AC1.2 + +**Files:** +- Modify: `src/simlin-engine/src/array_tests.rs:334` (`dimension_position_single` test) +- Modify: `src/simlin-engine/src/array_tests.rs:1357` (`dimension_position_and_wildcard` test) + +**Implementation:** + +In `dimension_position_single`: change `project.assert_compiles()` to `project.assert_compiles_incremental()`. Remove the comment about @N not being supported on the incremental path. Keep `assert_sim_builds()` and `assert_scalar_result()` calls unchanged (they use the AST interpreter for cross-validation). + +In `dimension_position_and_wildcard`: change `.assert_compiles()` to `.assert_compiles_incremental()`. Remove the comment about @N not being supported. + +**Verification:** +Run: `cargo test -p simlin-engine dimension_position_single` +Run: `cargo test -p simlin-engine dimension_position_and_wildcard` +Expected: Both pass. + +**Commit:** `engine: switch @N position tests to incremental path` + + + +### Task 3: Add error tests for out-of-range @N + +**Verifies:** assert-compiles-migration.AC1.3, assert-compiles-migration.AC1.4 + +**Files:** +- Modify: `src/simlin-engine/src/array_tests.rs` (add new tests in `dimension_position_tests` module) + +**Implementation:** + +Add two new tests in the `dimension_position_tests` module that verify the incremental compiler rejects invalid @N positions: + +- `dimension_position_zero_is_error`: Build a `TestProject` with an indexed dimension (size 3), an array, and a scalar aux using `arr[@0]`. Assert `project.compile_incremental()` returns `Err` and the error message contains the variable name (`first_elem` or similar) to confirm the error is specifically about the @0 reference, not an unrelated compilation issue. + +- `dimension_position_out_of_range_is_error`: Build a `TestProject` with an indexed dimension (size 3), an array, and a scalar aux using `arr[@5]`. Assert `project.compile_incremental()` returns `Err` and the error message contains the variable name to confirm it is specifically about the out-of-range @N. + +Follow the existing `TestProject` builder pattern from tests like `dimension_position_single`. + +**Verification:** +Run: `cargo test -p simlin-engine dimension_position` +Expected: All dimension_position tests pass (existing + new). + +**Commit:** `engine: add error tests for out-of-range @N position syntax` + + + diff --git a/docs/implementation-plans/2026-03-08-assert-compiles-migration/phase_02.md b/docs/implementation-plans/2026-03-08-assert-compiles-migration/phase_02.md new file mode 100644 index 00000000..926bf35f --- /dev/null +++ b/docs/implementation-plans/2026-03-08-assert-compiles-migration/phase_02.md @@ -0,0 +1,185 @@ +# Assert-Compiles Migration -- Phase 2: Array-Reduce Abstraction and MEAN Dynamic Ranges + +**Goal:** Unify the array-reduce codegen pattern across all builtins and fix MEAN with dynamic range arguments. + +**Architecture:** Extract a shared `emit_array_reduce(&mut self, arg: &Expr, opcode: Opcode) -> Result>` helper in `codegen.rs` that encapsulates the `walk_expr_as_view(arg) + push(opcode) + push(PopView) + return Ok(Some(()))` pattern. Refactor SUM, SIZE, STDDEV, and the 1-arg paths of MIN/MAX to use it. For MEAN, restructure the handler: when `args.len() == 1`, call `emit_array_reduce` directly (like SUM/SIZE/STDDEV do), which routes through `walk_expr_as_view` and correctly handles `Expr::Subscript` with `Range` indices. The multi-argument scalar MEAN path remains for `args.len() > 1`. + +**Tech Stack:** Rust (simlin-engine) + +**Scope:** Phase 2 of 3 from design plan + +**Codebase verified:** 2026-03-08 + +**Reference files for executor:** +- `/home/bpowers/src/simlin/src/simlin-engine/CLAUDE.md` -- engine architecture and module map +- `/home/bpowers/src/simlin/CLAUDE.md` -- project-wide development standards + +--- + +## Acceptance Criteria Coverage + +This phase implements and tests: + +### assert-compiles-migration.AC2: MEAN with dynamic ranges works on incremental path +- **assert-compiles-migration.AC2.1 Success:** `MEAN(data[start_idx:end_idx])` with variable bounds compiles and simulates correctly +- **assert-compiles-migration.AC2.2 Success:** Existing SUM, SIZE, STDDEV, VMIN, VMAX behavior unchanged after refactoring to shared helper + +--- + +## Codebase Verification Findings + +- **Confirmed:** `BuiltinFn::Mean(args)` handler at `codegen.rs:865-897`. The `is_array` check at line 871 matches only `Expr::StaticSubscript` and `Expr::TempArray`, missing `Expr::Subscript` with `Range` indices. This is the bug. +- **Confirmed:** SUM (line 915), SIZE (line 901), STDDEV (line 908) all call `walk_expr_as_view(arg)` unconditionally with no `is_array` guard. MIN/MAX (lines 794-823) use `Option>` arity dispatch, calling `walk_expr_as_view` for 1-arg form. +- **Confirmed:** The `walk_expr_as_view` -> ArrayOp -> PopView pattern is copy-pasted 6 times (SUM, SIZE, STDDEV, MIN 1-arg, MAX 1-arg, MEAN array path). +- **Confirmed:** `walk_expr_as_view` at line 281 handles `Expr::Subscript` with `SubscriptIndex::Range` via `ViewRangeDynamic` opcode. The plumbing works -- SUM/SIZE/STDDEV already use it for dynamic ranges. +- **Confirmed:** `mean_with_dynamic_range` test at `array_tests.rs:2010-2026` uses `assert_compiles()`. Adjacent `size_with_dynamic_range` (line 2028) and `stddev_with_dynamic_range` (line 2045) use `assert_compiles_incremental()`, confirming those builtins already work. +- **Confirmed:** `BuiltinFn::Mean` takes `Vec` (variadic). `Sum`/`Size`/`Stddev` take `Box`. `Min`/`Max` take `(Box, Option>)`. + +--- + + + + +### Task 1: Extract `emit_array_reduce` helper + +**Verifies:** assert-compiles-migration.AC2.2 (no behavior change for existing builtins) + +**Files:** +- Modify: `src/simlin-engine/src/compiler/codegen.rs` + +**Implementation:** + +Add a private method to the codegen struct: + +```rust +/// Emit the array-reduce pattern: push view, emit reduction opcode, pop view. +/// Used by SUM, SIZE, STDDEV, MIN (1-arg), MAX (1-arg), and MEAN (1-arg). +fn emit_array_reduce(&mut self, arg: &Expr, opcode: Opcode) -> Result> { + self.walk_expr_as_view(arg)?; + self.push(opcode); + self.push(Opcode::PopView {}); + Ok(Some(())) +} +``` + +Then refactor each builtin handler to use it: + +**SUM** (currently lines 915-921): +```rust +BuiltinFn::Sum(arg) => { + return self.emit_array_reduce(arg, Opcode::ArraySum {}); +} +``` + +**SIZE** (currently lines 901-907): +```rust +BuiltinFn::Size(arg) => { + return self.emit_array_reduce(arg, Opcode::ArraySize {}); +} +``` + +**STDDEV** (currently lines 908-914): +```rust +BuiltinFn::Stddev(arg) => { + return self.emit_array_reduce(arg, Opcode::ArrayStddev {}); +} +``` + +**MAX** 1-arg path (currently lines 794-806): +```rust +BuiltinFn::Max(a, b) => { + if let Some(b) = b { + self.walk_expr(a)?.unwrap(); + self.walk_expr(b)?.unwrap(); + let id = self.curr_code.intern_literal(0.0); + self.push(Opcode::LoadConstant { id }); + } else { + return self.emit_array_reduce(a, Opcode::ArrayMax {}); + } +} +``` + +**MIN** 1-arg path (currently lines 809-823): +```rust +BuiltinFn::Min(a, b) => { + if let Some(b) = b { + self.walk_expr(a)?.unwrap(); + self.walk_expr(b)?.unwrap(); + let id = self.curr_code.intern_literal(0.0); + self.push(Opcode::LoadConstant { id }); + } else { + return self.emit_array_reduce(a, Opcode::ArrayMin {}); + } +} +``` + +**Verification:** +Run: `cargo test -p simlin-engine` +Expected: All existing tests pass (no behavior change). + +**Commit:** `engine: extract emit_array_reduce helper for array-reduce builtins` + + + +### Task 2: Fix MEAN to use `emit_array_reduce` for single-arg form + +**Verifies:** assert-compiles-migration.AC2.1, assert-compiles-migration.AC2.2 + +**Files:** +- Modify: `src/simlin-engine/src/compiler/codegen.rs:865-897` (the `BuiltinFn::Mean` handler) + +**Implementation:** + +Replace the MEAN handler. When `args.len() == 1`, call `emit_array_reduce` directly (like SUM/SIZE/STDDEV), eliminating the `is_array` check entirely. The multi-arg scalar path remains for `args.len() > 1`: + +```rust +BuiltinFn::Mean(args) => { + if args.len() == 1 { + return self.emit_array_reduce(&args[0], Opcode::ArrayMean {}); + } + + // Multi-argument scalar mean: (arg1 + arg2 + ... + argN) / N + let id = self.curr_code.intern_literal(0.0); + self.push(Opcode::LoadConstant { id }); + + for arg in args.iter() { + self.walk_expr(arg)?.unwrap(); + self.push(Opcode::Op2 { op: Op2::Add }); + } + + let id = self.curr_code.intern_literal(args.len() as f64); + self.push(Opcode::LoadConstant { id }); + self.push(Opcode::Op2 { op: Op2::Div }); + return Ok(Some(())); +} +``` + +This removes the `is_array` check entirely for single-arg MEAN, routing all single-arg cases through `walk_expr_as_view`. Since `walk_expr_as_view` handles `StaticSubscript`, `TempArray`, `Var`, AND `Subscript` (with `Range`), all array forms now work including dynamic ranges. + +**Verification:** +Run: `cargo test -p simlin-engine` +Expected: All existing tests pass, including all MEAN-related tests. + +**Commit:** `engine: fix MEAN to handle dynamic range subscripts via emit_array_reduce` + + + +### Task 3: Switch `mean_with_dynamic_range` test to incremental path + +**Verifies:** assert-compiles-migration.AC2.1 + +**Files:** +- Modify: `src/simlin-engine/src/array_tests.rs:2010-2026` (`mean_with_dynamic_range` test) + +**Implementation:** + +Change `project.assert_compiles()` to `project.assert_compiles_incremental()`. Remove the comment "MEAN with dynamic ranges is not yet supported on the incremental path". Keep `assert_sim_builds()` and `assert_scalar_result()` calls unchanged. + +**Verification:** +Run: `cargo test -p simlin-engine mean_with_dynamic_range` +Expected: Test passes. + +**Commit:** `engine: switch mean_with_dynamic_range test to incremental path` + + + diff --git a/docs/implementation-plans/2026-03-08-assert-compiles-migration/phase_03.md b/docs/implementation-plans/2026-03-08-assert-compiles-migration/phase_03.md new file mode 100644 index 00000000..25e1a554 --- /dev/null +++ b/docs/implementation-plans/2026-03-08-assert-compiles-migration/phase_03.md @@ -0,0 +1,126 @@ +# Assert-Compiles Migration -- Phase 3: Bulk Test Migration and Cleanup + +**Goal:** Switch all remaining 23 tests from `assert_compiles()` to `assert_compiles_incremental()`, then delete `assert_compiles()`. + +**Architecture:** Mechanical replacement of `assert_compiles()` with `assert_compiles_incremental()` across 3 files (20 + 2 + 1 calls), followed by deleting the `assert_compiles()` method from `test_common.rs`. The `compile()` method is NOT deleted because it has other active callers (`assert_compile_error_impl` and `tests/simulate_ltm.rs`). + +**Tech Stack:** Rust (simlin-engine) + +**Scope:** Phase 3 of 3 from design plan + +**Codebase verified:** 2026-03-08 + +**Reference files for executor:** +- `/home/bpowers/src/simlin/src/simlin-engine/CLAUDE.md` -- engine architecture and module map +- `/home/bpowers/src/simlin/CLAUDE.md` -- project-wide development standards + +--- + +## Acceptance Criteria Coverage + +This phase implements and tests: + +### assert-compiles-migration.AC3: assert_compiles fully removed +- **assert-compiles-migration.AC3.1 Success:** All 26 formerly-monolithic tests pass with `assert_compiles_incremental()` +- **assert-compiles-migration.AC3.2 Success:** `assert_compiles()` method deleted from `test_common.rs` +- **assert-compiles-migration.AC3.3 Success:** `compile()` method deleted from `test_common.rs` (if no other callers) +- **assert-compiles-migration.AC3.4 Success:** `cargo test -p simlin-engine` passes with zero regressions + +--- + +## Codebase Verification Findings + +- **Confirmed:** 20 `assert_compiles()` calls in `builtins_visitor.rs` at lines: 741, 756, 779, 803, 827, 846, 860, 874, 887, 901, 928, 942, 965, 982, 1004, 1027, 1053, 1074, 1086, 1100. +- **Confirmed:** 2 calls in `db_tests.rs` at lines 4974 and 5062. +- **Confirmed:** 1 call in `db_prev_init_tests.rs` at line 81. +- **Important: `compile()` method (lines 387-433) cannot be deleted.** It has other active callers: + - `assert_compile_error_impl()` at line 603 (called by `assert_compile_error()` which has active callers throughout `array_tests.rs` and `builtins_visitor.rs`) + - `tests/simulate_ltm.rs` at 10 call sites (lines 582, 609, 656, 685, 742, 783, 824, 888, 933, 1003) +- **AC3.3 clarification:** `compile()` has other callers, so it stays. AC3.3 is satisfied with the note "other callers remain." + +--- + + +### Task 1: Switch `builtins_visitor.rs` tests to incremental path + +**Verifies:** assert-compiles-migration.AC3.1 + +**Files:** +- Modify: `src/simlin-engine/src/builtins_visitor.rs` + +**Implementation:** + +Replace `assert_compiles()` with `assert_compiles_incremental()` in all 20 test functions listed below. This is a mechanical find-and-replace within the `#[cfg(test)]` module. Remove any comments about the monolithic path or incremental support gaps if present. + +Test functions (line numbers for the `assert_compiles()` call): +- `test_arrayed_delay1_basic` (741) +- `test_arrayed_delay1_mixed_args` (756) +- `test_arrayed_delay1_numerical_values` (779) +- `test_arrayed_delay1_all_arrayed` (803) +- `test_arrayed_delay1_different_element_values` (827) +- `test_arrayed_delay3` (846) +- `test_arrayed_delayn_order1` (860) +- `test_arrayed_delayn_order3` (874) +- `test_arrayed_smooth1` (887) +- `test_arrayed_smthn_order1` (901) +- `test_arrayed_delay1_indexed_dimension` (928) +- `test_arrayed_delay_in_expression` (942) +- `test_arrayed_per_element_delay1` (965) +- `test_arrayed_per_element_mixed_stdlib` (982) +- `test_arrayed_per_element_delay1_with_subscripted_inputs` (1004) +- `test_npv_basic` (1027) +- `test_npv_with_discount` (1053) +- `test_modulo_function` (1074) +- `test_nested_init_does_not_rewrite_generated_arg_helpers` (1086) +- `test_delay_alias` (1100) + +**Verification:** +Run: `cargo test -p simlin-engine builtins_visitor` +Expected: All 20 tests pass. + +**Commit:** `engine: switch builtins_visitor tests to incremental path` + + + +### Task 2: Switch `db_tests.rs` and `db_prev_init_tests.rs` tests to incremental path + +**Verifies:** assert-compiles-migration.AC3.1 + +**Files:** +- Modify: `src/simlin-engine/src/db_tests.rs:4974` (`test_arrayed_1arg_previous_loadprev_per_element`) +- Modify: `src/simlin-engine/src/db_tests.rs:5062` (`test_arrayed_2arg_previous_per_element_modules`) +- Modify: `src/simlin-engine/src/db_prev_init_tests.rs:81` (`test_nested_previous_does_not_create_false_cycle_via_helper_deps`) + +**Implementation:** + +Replace `assert_compiles()` with `assert_compiles_incremental()` in all 3 test functions. Remove any comments about the monolithic path if present. + +**Verification:** +Run: `cargo test -p simlin-engine test_arrayed_1arg_previous` +Run: `cargo test -p simlin-engine test_arrayed_2arg_previous` +Run: `cargo test -p simlin-engine test_nested_previous_does_not_create_false_cycle` +Expected: All 3 pass. + +**Commit:** `engine: switch db_tests and db_prev_init_tests to incremental path` + + + +### Task 3: Delete `assert_compiles()` method from `test_common.rs` + +**Verifies:** assert-compiles-migration.AC3.2, assert-compiles-migration.AC3.4 + +**Files:** +- Modify: `src/simlin-engine/src/test_common.rs:532-545` (delete `assert_compiles()` method) + +**Implementation:** + +Delete the `assert_compiles()` method (lines 532-545). Do NOT delete `compile()` (lines 387-433) -- it has other active callers (`assert_compile_error_impl` and `tests/simulate_ltm.rs`). + +Verify there are zero remaining calls to `assert_compiles()` (not `assert_compiles_incremental`) in the engine codebase. The grep should return zero results. + +**Verification:** +Run: `cargo test -p simlin-engine` +Expected: All engine tests pass. No compilation errors. + +**Commit:** `engine: delete assert_compiles() method` + diff --git a/docs/implementation-plans/2026-03-08-assert-compiles-migration/test-requirements.md b/docs/implementation-plans/2026-03-08-assert-compiles-migration/test-requirements.md new file mode 100644 index 00000000..40733312 --- /dev/null +++ b/docs/implementation-plans/2026-03-08-assert-compiles-migration/test-requirements.md @@ -0,0 +1,67 @@ +# Test Requirements: Assert-Compiles Migration + +This document maps every acceptance criterion from the [design plan](../../design-plans/2026-03-07-assert-compiles-migration.md) to either an automated test or a documented human verification step. + +--- + +## Automated Tests + +### AC1: @N position syntax works on incremental path + +| Criterion | Test Type | Test Name | Test File | Phase/Task | +|-----------|-----------|-----------|-----------|------------| +| AC1.1 `arr[@1]` scalar context | Unit (compile + simulate) | `dimension_position_single` | `src/simlin-engine/src/array_tests.rs` | P1/T2 | +| AC1.2 `cube[@1, *, @3]` mixed | Unit (compile) | `dimension_position_and_wildcard` | `src/simlin-engine/src/array_tests.rs` | P1/T2 | +| AC1.3 `arr[@0]` error | Unit (compile error) | `dimension_position_zero_is_error` (new) | `src/simlin-engine/src/array_tests.rs` | P1/T3 | +| AC1.4 `arr[@N]` out-of-range error | Unit (compile error) | `dimension_position_out_of_range_is_error` (new) | `src/simlin-engine/src/array_tests.rs` | P1/T3 | + +Regression guard: existing `dimension_position_reorder` and `dimension_position_3d` confirm A2A path is unaffected. + +### AC2: MEAN with dynamic ranges works on incremental path + +| Criterion | Test Type | Test Name | Test File | Phase/Task | +|-----------|-----------|-----------|-----------|------------| +| AC2.1 MEAN with variable bounds | Unit (compile + simulate) | `mean_with_dynamic_range` | `src/simlin-engine/src/array_tests.rs` | P2/T3 | +| AC2.2 Existing builtins unchanged | Regression | All existing array-reduce tests | `src/simlin-engine/src/array_tests.rs` | P2/T1 | + +AC2.2 rationale: `emit_array_reduce` is a mechanical extraction of duplicated code. Each builtin already has test coverage. P2/T1 is a separate commit from P2/T2 so `cargo test` at the boundary gates regressions. + +### AC3: assert_compiles fully removed + +| Criterion | Test Type | Test Name(s) | Phase/Task | +|-----------|-----------|-------------|------------| +| AC3.1 All 26 tests pass incremental | Unit (compile) | 20 in `builtins_visitor`, 2 in `db_tests`, 1 in `db_prev_init_tests`, plus 3 from P1/P2 | P3/T1-T2 | +| AC3.2 `assert_compiles()` deleted | Build (compilation) | N/A -- absence verified by successful build | P3/T3 | +| AC3.3 `compile()` retained | Human verification | See below | P3/T3 | +| AC3.4 Zero regressions | Full suite | `cargo test -p simlin-engine` | P3/T3 | + +--- + +## Human Verification + +### AC3.3: `compile()` method retention + +**Criterion:** `compile()` method deleted from `test_common.rs` (if no other callers). + +**Decision:** `compile()` is NOT deleted -- it has active callers: +1. `assert_compile_error_impl()` at `test_common.rs:603` +2. `tests/simulate_ltm.rs` -- 10 call sites + +**Verification:** After deleting `assert_compiles()`, grep for `.compile()` in engine test files to confirm callers remain. If they do (expected), `compile()` stays and AC3.3 is satisfied. + +--- + +## Traceability Matrix + +| AC | Automated Test(s) | Human | Phase | +|----|-------------------|-------|-------| +| AC1.1 | `dimension_position_single` | -- | P1 | +| AC1.2 | `dimension_position_and_wildcard` | -- | P1 | +| AC1.3 | `dimension_position_zero_is_error` (new) | -- | P1 | +| AC1.4 | `dimension_position_out_of_range_is_error` (new) | -- | P1 | +| AC2.1 | `mean_with_dynamic_range` | -- | P2 | +| AC2.2 | Existing array-reduce suite (regression) | -- | P2 | +| AC3.1 | 26 switched tests | -- | P1+P2+P3 | +| AC3.2 | Build succeeds after deletion | -- | P3 | +| AC3.3 | -- | Verify `compile()` callers | P3 | +| AC3.4 | `cargo test -p simlin-engine` | -- | P3 | diff --git a/docs/test-plans/2026-03-08-assert-compiles-migration.md b/docs/test-plans/2026-03-08-assert-compiles-migration.md new file mode 100644 index 00000000..d1dab3a2 --- /dev/null +++ b/docs/test-plans/2026-03-08-assert-compiles-migration.md @@ -0,0 +1,60 @@ +# Test Plan: Assert-Compiles Migration + +## Prerequisites +- Engine builds cleanly: `cargo build -p simlin-engine` +- All engine tests pass: `cargo test -p simlin-engine` +- Full integration suite passes: `cargo test --features file_io --test simulate` + +## Phase 1: @N Position Syntax + +| Step | Action | Expected | +|------|--------|----------| +| 1.1 | Run `cargo test -p simlin-engine dimension_position_single -- --exact` | Test passes, confirming `arr[@1]` resolves to first element (10.0) in scalar context | +| 1.2 | Run `cargo test -p simlin-engine dimension_position_and_wildcard -- --exact` | Test passes, confirming `cube[@1, *, @3]` compiles on incremental path | +| 1.3 | Run `cargo test -p simlin-engine dimension_position_zero_is_error -- --exact` | Test passes, confirming `@0` is rejected with an error referencing the variable name | +| 1.4 | Run `cargo test -p simlin-engine dimension_position_out_of_range_is_error -- --exact` | Test passes, confirming `@5` on a size-3 dimension is rejected | +| 1.5 | Run `cargo test -p simlin-engine dimension_position_reorder -- --exact` | Regression guard passes, confirming A2A `matrix[@2, @1]` transpose still works | +| 1.6 | Run `cargo test -p simlin-engine dimension_position_3d -- --exact` | Regression guard passes, confirming 3D A2A `cube[@3, @2, @1]` reorder still works | + +## Phase 2: MEAN with Dynamic Ranges + +| Step | Action | Expected | +|------|--------|----------| +| 2.1 | Run `cargo test -p simlin-engine mean_with_dynamic_range -- --exact` | Test passes, result = 25.0 (correct MEAN of 2-element subrange) | +| 2.2 | Run `cargo test -p simlin-engine sum_with_dynamic_range -- --exact` | Regression guard: SUM with dynamic range still works | +| 2.3 | Run `cargo test -p simlin-engine size_with_dynamic_range -- --exact` | Regression guard: SIZE with dynamic range returns actual range size | +| 2.4 | Run `cargo test -p simlin-engine stddev_with_dynamic_range -- --exact` | Regression guard: STDDEV with dynamic range uses correct count | +| 2.5 | Run `cargo test -p simlin-engine size_with_empty_dynamic_range -- --exact` | Regression guard: SIZE with reversed range returns 0 | + +## Phase 3: assert_compiles Removal + +| Step | Action | Expected | +|------|--------|----------| +| 3.1 | Run `cargo test -p simlin-engine` (full engine test suite) | All tests pass with zero failures | +| 3.2 | Search for old method: `grep -rn 'fn assert_compiles\b' src/simlin-engine/ \| grep -v incremental` | Zero matches | +| 3.3 | Search for old call sites: `grep -rn '\.assert_compiles(' src/simlin-engine/ \| grep -v incremental` | Zero matches | +| 3.4 | Verify `compile()` has callers: `grep -rn '\.compile()' src/simlin-engine/src/test_common.rs src/simlin-engine/tests/simulate_ltm.rs` | Active callers in `test_common.rs:588` and 10 sites in `simulate_ltm.rs` | + +## End-to-End Regression Suite + +| Step | Action | Expected | +|------|--------|----------| +| E1 | Run `cargo test -p simlin-engine` | All engine tests pass | +| E2 | Run `cargo test --features file_io --test simulate` | All simulation integration tests pass | +| E3 | Run `cargo test --features file_io --test simulate_ltm` | All LTM simulation tests pass | +| E4 | Run `cargo build -p simlin-engine --release` | Release build succeeds | + +## Traceability + +| Acceptance Criterion | Automated Test | Manual Step | +|----------------------|----------------|-------------| +| AC1.1 `arr[@1]` scalar context | `dimension_position_single` | 1.1 | +| AC1.2 `cube[@1, *, @3]` mixed | `dimension_position_and_wildcard` | 1.2 | +| AC1.3 `arr[@0]` error | `dimension_position_zero_is_error` | 1.3 | +| AC1.4 `arr[@N]` out-of-range error | `dimension_position_out_of_range_is_error` | 1.4 | +| AC2.1 MEAN with variable bounds | `mean_with_dynamic_range` | 2.1 | +| AC2.2 Existing builtins unchanged | Existing array-reduce suite (50+ tests) | 2.2-2.5 | +| AC3.1 All 26 tests pass incremental | 20 builtins_visitor + 2 db_tests + 1 db_prev_init + 3 new | 3.1 | +| AC3.2 `assert_compiles()` deleted | Build succeeds | 3.2, 3.3 | +| AC3.3 `compile()` retained | -- | 3.4 | +| AC3.4 Zero regressions | `cargo test -p simlin-engine` | E1-E4 | diff --git a/src/simlin-engine/CLAUDE.md b/src/simlin-engine/CLAUDE.md index 323b2216..134b213c 100644 --- a/src/simlin-engine/CLAUDE.md +++ b/src/simlin-engine/CLAUDE.md @@ -14,9 +14,9 @@ Equation text flows through these stages in order: 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). 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 + - `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 - `expr.rs` - Expression compilation - - `codegen.rs` - Bytecode emission; routes array-producing builtins through dedicated opcodes instead of element-wise iteration + - `codegen.rs` - Bytecode emission; routes array-producing builtins through dedicated opcodes instead of element-wise iteration. `emit_array_reduce()` is the shared helper for single-argument array builtins (SUM, SIZE, STDDEV, MIN, MAX, MEAN): pushes view, emits reduction opcode, pops view - `dimensions.rs` - Dimension checking/inference - `subscript.rs` - Array subscript expansion and iteration - `pretty.rs` - Debug pretty-printing diff --git a/src/simlin-engine/src/array_tests.rs b/src/simlin-engine/src/array_tests.rs index 8256352d..00deb239 100644 --- a/src/simlin-engine/src/array_tests.rs +++ b/src/simlin-engine/src/array_tests.rs @@ -338,8 +338,7 @@ mod dimension_position_tests { .array_with_ranges("arr[Items]", vec![("1", "10"), ("2", "20"), ("3", "30")]) .scalar_aux("first_elem", "arr[@1]"); // Should get first element = 10 - // @N position syntax is not yet supported on the incremental path - project.assert_compiles(); + project.assert_compiles_incremental(); project.assert_sim_builds(); project.assert_scalar_result("first_elem", 10.0); } @@ -435,6 +434,45 @@ mod dimension_position_tests { // Should get A=1 slice with C,B ordering: [111, 121, 131, 112, 122, 132] project.assert_interpreter_result("slice", &[111.0, 121.0, 131.0, 112.0, 122.0, 132.0]); } + + #[test] + fn dimension_position_zero_is_error() { + // @0 is invalid (positions are 1-based) + let project = TestProject::new("dim_pos_zero") + .indexed_dimension("Items", 3) + .array_with_ranges("arr[Items]", vec![("1", "10"), ("2", "20"), ("3", "30")]) + .scalar_aux("first_elem", "arr[@0]"); + + let result = project.compile_incremental(); + assert!(result.is_err(), "@0 should be rejected as out-of-range"); + let err = result.unwrap_err(); + let details = format!("{err:?}"); + assert!( + details.contains("first_elem"), + "error should reference the variable name, got: {details}" + ); + } + + #[test] + fn dimension_position_out_of_range_is_error() { + // @5 exceeds dimension size 3 + let project = TestProject::new("dim_pos_oob") + .indexed_dimension("Items", 3) + .array_with_ranges("arr[Items]", vec![("1", "10"), ("2", "20"), ("3", "30")]) + .scalar_aux("first_elem", "arr[@5]"); + + let result = project.compile_incremental(); + assert!( + result.is_err(), + "@5 should be rejected for size-3 dimension" + ); + let err = result.unwrap_err(); + let details = format!("{err:?}"); + assert!( + details.contains("first_elem"), + "error should reference the variable name, got: {details}" + ); + } } #[cfg(test)] @@ -1356,14 +1394,33 @@ mod combined_operations_tests { #[test] fn dimension_position_and_wildcard() { // Combine dimension position with wildcard - // @N position syntax is not yet supported on the incremental path TestProject::new("combined_dimpos_wildcard") .indexed_dimension("X", 2) .indexed_dimension("Y", 3) .indexed_dimension("Z", 4) .array_aux("cube[X,Y,Z]", "X * 100 + Y * 10 + Z") - .array_aux("slice[Z,Y]", "cube[@1, *, @3]") // Fix X=0, reorder Y and Z - .assert_compiles(); + .array_aux("slice[Z,Y]", "cube[@1, *, @3]") // Fix X=1, reorder Y and Z + .assert_compiles_incremental(); + } + + #[test] + fn dimension_position_and_wildcard_indexed_values() { + // Verify @N with wildcard produces correct values on indexed dimensions. + // This catches a bug where the A2A iteration subscript from the LHS + // dimension (Y) is incorrectly used as the RHS dimension (X) subscript + // when both are indexed and their numeric ranges overlap. + let project = TestProject::new("dimpos_wildcard_values") + .indexed_dimension("X", 2) + .indexed_dimension("Y", 3) + .array_aux("matrix[X, Y]", "X * 10 + Y") + // row[Y] = matrix[@1, *] should select first row: [11, 12, 13] + .array_aux("row[Y]", "matrix[@1, *]") + // Check second element of row: matrix[1, 2] = 12 + .scalar_aux("check", "row[@2]"); + + project.assert_compiles_incremental(); + project.assert_sim_builds(); + project.assert_scalar_result("check", 12.0); } #[test] @@ -2019,12 +2076,40 @@ mod structural_lowering_tests { // NOT (20 + 30) / 5 = 10 (which would be wrong if using full array size) .scalar_aux("result", "MEAN(data[start_idx:end_idx])"); - // MEAN with dynamic ranges is not yet supported on the incremental path - project.assert_compiles(); + project.assert_compiles_incremental(); project.assert_sim_builds(); project.assert_scalar_result("result", 25.0); } + #[test] + fn mean_of_scalar_expression() { + // MEAN(scalar_expr) where the argument is a composite scalar expression + // (not an array) should compile and return the expression's value. + // Regression: routing all single-arg MEAN through emit_array_reduce + // broke this because walk_expr_as_view can't handle Expr::Op2. + let project = TestProject::new("mean_scalar_expr") + .scalar_aux("x", "5") + .scalar_aux("result", "MEAN(x + 1)"); + + project.assert_compiles_incremental(); + project.assert_sim_builds(); + project.assert_scalar_result("result", 6.0); + } + + #[test] + fn mean_of_scalar_var() { + // MEAN(scalar_var) where the argument is a bare scalar Var hits the + // Expr::Var arm and goes through emit_array_reduce/walk_expr_as_view. + // Verify the VM's ArrayMean opcode handles a 1-element view correctly. + let project = TestProject::new("mean_scalar_var") + .scalar_aux("x", "7") + .scalar_aux("result", "MEAN(x)"); + + project.assert_compiles_incremental(); + project.assert_sim_builds(); + project.assert_scalar_result("result", 7.0); + } + #[test] fn size_with_dynamic_range() { // Test SIZE with dynamic range - must return actual range size diff --git a/src/simlin-engine/src/builtins_visitor.rs b/src/simlin-engine/src/builtins_visitor.rs index f5c34648..aff9379e 100644 --- a/src/simlin-engine/src/builtins_visitor.rs +++ b/src/simlin-engine/src/builtins_visitor.rs @@ -738,7 +738,7 @@ mod tests { .aux("init", "0", None) .array_aux("d[SubA]", "DELAY1(input[SubA], delay_time, init)"); - project.assert_compiles(); + project.assert_compiles_incremental(); project.assert_sim_builds(); } @@ -753,7 +753,7 @@ mod tests { .aux("init_scalar", "0", None) .array_aux("d[DimA]", "DELAY1(input_a[DimA], delay, init_scalar)"); - project.assert_compiles(); + project.assert_compiles_incremental(); project.assert_sim_builds(); } @@ -776,7 +776,7 @@ mod tests { .aux("init", "0", None) .array_aux("d[DimA]", "DELAY1(input_a[DimA], delay, init)"); - project.assert_compiles(); + project.assert_compiles_incremental(); project.assert_sim_builds(); // Get results for 2 timesteps (0 and 1) @@ -800,7 +800,7 @@ mod tests { "DELAY1(input_a[DimA], delay_a[DimA], init_a[DimA])", ); - project.assert_compiles(); + project.assert_compiles_incremental(); project.assert_sim_builds(); } @@ -824,7 +824,7 @@ mod tests { "DELAY1(input_a[DimA], delay_a[DimA], init_a[DimA])", ); - project.assert_compiles(); + project.assert_compiles_incremental(); project.assert_sim_builds(); // At step 1: output = stock/delay @@ -843,7 +843,7 @@ mod tests { .array_const("delay_a[DimA]", 1.0) .array_aux("d[DimA]", "DELAY3(input, delay_a[DimA])"); - project.assert_compiles(); + project.assert_compiles_incremental(); project.assert_sim_builds(); } @@ -857,7 +857,7 @@ mod tests { .aux("init", "0", None) .array_aux("d[DimA]", "DELAYN(input_a[DimA], delay_time, 1, init)"); - project.assert_compiles(); + project.assert_compiles_incremental(); project.assert_sim_builds(); } @@ -871,7 +871,7 @@ mod tests { .aux("init", "0", None) .array_aux("d[DimA]", "DELAYN(input_a[DimA], delay_time, 3, init)"); - project.assert_compiles(); + project.assert_compiles_incremental(); project.assert_sim_builds(); } @@ -884,7 +884,7 @@ mod tests { .aux("smooth_time", "1", None) .array_aux("s[DimA]", "SMTH1(input_a[DimA], smooth_time)"); - project.assert_compiles(); + project.assert_compiles_incremental(); project.assert_sim_builds(); } @@ -898,7 +898,7 @@ mod tests { .aux("init", "0", None) .array_aux("s[DimA]", "SMTHN(input_a[DimA], smooth_time, 1, init)"); - project.assert_compiles(); + project.assert_compiles_incremental(); project.assert_sim_builds(); } @@ -925,7 +925,7 @@ mod tests { .aux("init", "0", None) .array_aux("d[Idx]", "DELAY1(input[Idx], delay_time, init)"); - project.assert_compiles(); + project.assert_compiles_incremental(); project.assert_sim_builds(); } @@ -939,7 +939,7 @@ mod tests { .array_const("delay_a[DimA]", 1.0) .array_aux("d[DimA]", "k * DELAY3(input, delay_a[DimA])"); - project.assert_compiles(); + project.assert_compiles_incremental(); project.assert_sim_builds(); } @@ -962,7 +962,7 @@ mod tests { ], ); - project.assert_compiles(); + project.assert_compiles_incremental(); project.assert_sim_builds(); } @@ -979,7 +979,7 @@ mod tests { vec![("A1", "DELAY1(input1, delay_time, init)"), ("A2", "42")], ); - project.assert_compiles(); + project.assert_compiles_incremental(); project.assert_sim_builds(); } @@ -1001,7 +1001,7 @@ mod tests { ], ); - project.assert_compiles(); + project.assert_compiles_incremental(); project.assert_sim_builds(); } @@ -1024,7 +1024,7 @@ mod tests { None, ); - project.assert_compiles(); + project.assert_compiles_incremental(); project.assert_sim_builds(); // output = stock + inflow * DT // t=0: stock=0, inflow=10*1*(1+0)^0=10, output = 0 + 10*1 = 10 @@ -1050,7 +1050,7 @@ mod tests { None, ); - project.assert_compiles(); + project.assert_compiles_incremental(); project.assert_sim_builds(); let results = project.run_interpreter().unwrap(); let vals = results.get("result").unwrap(); @@ -1071,7 +1071,7 @@ mod tests { .aux("b", "3", None) .aux("result", "MODULO(a, b)", None); - project.assert_compiles(); + project.assert_compiles_incremental(); project.assert_sim_builds(); project.assert_interpreter_result("result", &[1.0, 1.0]); } @@ -1083,7 +1083,7 @@ mod tests { .aux("x", "1", None) .aux("result", "INIT(INIT(x + 1))", None); - project.assert_compiles(); + project.assert_compiles_incremental(); project.assert_sim_builds(); project.assert_interpreter_result("result", &[2.0, 2.0]); } @@ -1097,7 +1097,7 @@ mod tests { .aux("init", "0", None) .aux("result", "DELAY(input, delay_time, init)", None); - project.assert_compiles(); + project.assert_compiles_incremental(); project.assert_sim_builds(); } } diff --git a/src/simlin-engine/src/compiler/codegen.rs b/src/simlin-engine/src/compiler/codegen.rs index 7607831c..05234793 100644 --- a/src/simlin-engine/src/compiler/codegen.rs +++ b/src/simlin-engine/src/compiler/codegen.rs @@ -360,6 +360,15 @@ impl<'module> Compiler<'module> { } } + /// Emit the array-reduce pattern: push view, emit reduction opcode, pop view. + /// Used by SUM, SIZE, STDDEV, MIN (1-arg), MAX (1-arg), and MEAN (1-arg). + fn emit_array_reduce(&mut self, arg: &Expr, opcode: Opcode) -> Result> { + self.walk_expr_as_view(arg)?; + self.push(opcode); + self.push(Opcode::PopView {}); + Ok(Some(())) + } + fn walk(&mut self, exprs: &[Expr]) -> Result { for expr in exprs.iter() { self.walk_expr(expr)?; @@ -793,32 +802,22 @@ impl<'module> Compiler<'module> { } BuiltinFn::Max(a, b) => { if let Some(b) = b { - // Two-argument scalar max self.walk_expr(a)?.unwrap(); self.walk_expr(b)?.unwrap(); let id = self.curr_code.intern_literal(0.0); self.push(Opcode::LoadConstant { id }); } else { - // Single-argument array max - self.walk_expr_as_view(a)?; - self.push(Opcode::ArrayMax {}); - self.push(Opcode::PopView {}); - return Ok(Some(())); + return self.emit_array_reduce(a, Opcode::ArrayMax {}); } } BuiltinFn::Min(a, b) => { if let Some(b) = b { - // Two-argument scalar min self.walk_expr(a)?.unwrap(); self.walk_expr(b)?.unwrap(); let id = self.curr_code.intern_literal(0.0); self.push(Opcode::LoadConstant { id }); } else { - // Single-argument array min - self.walk_expr_as_view(a)?; - self.push(Opcode::ArrayMin {}); - self.push(Opcode::PopView {}); - return Ok(Some(())); + return self.emit_array_reduce(a, Opcode::ArrayMin {}); } } BuiltinFn::Quantum(a, b) => { @@ -863,21 +862,22 @@ impl<'module> Compiler<'module> { self.walk_expr(c)?.unwrap(); } BuiltinFn::Mean(args) => { - // Check if this is a single array argument (array mean) - // vs multiple scalar arguments (variadic mean) if args.len() == 1 { - // Check if the argument is an array expression - let arg = &args[0]; - let is_array = matches!( - arg, - Expr::StaticSubscript(_, _, _) | Expr::TempArray(_, _, _) - ); - if is_array { - // Array mean - use ArrayMean opcode - self.walk_expr_as_view(arg)?; - self.push(Opcode::ArrayMean {}); - self.push(Opcode::PopView {}); - return Ok(Some(())); + // MEAN is variadic (Vec), unlike other array-reduce + // builtins which take Box. Single-arg MEAN can receive + // scalar expressions (Op2, etc.) that walk_expr_as_view + // can't handle, so we match on expression type first. + match &args[0] { + Expr::StaticSubscript(..) + | Expr::TempArray(..) + | Expr::Var(..) + | Expr::Subscript(..) => { + return self.emit_array_reduce(&args[0], Opcode::ArrayMean {}); + } + _ => { + self.walk_expr(&args[0])?.unwrap(); + return Ok(Some(())); + } } } @@ -899,25 +899,13 @@ impl<'module> Compiler<'module> { return sim_err!(TodoArrayBuiltin, "RANK not yet supported".to_owned()); } BuiltinFn::Size(arg) => { - // SIZE returns the number of elements in an array - self.walk_expr_as_view(arg)?; - self.push(Opcode::ArraySize {}); - self.push(Opcode::PopView {}); - return Ok(Some(())); + return self.emit_array_reduce(arg, Opcode::ArraySize {}); } BuiltinFn::Stddev(arg) => { - // STDDEV computes standard deviation of array elements - self.walk_expr_as_view(arg)?; - self.push(Opcode::ArrayStddev {}); - self.push(Opcode::PopView {}); - return Ok(Some(())); + return self.emit_array_reduce(arg, Opcode::ArrayStddev {}); } BuiltinFn::Sum(arg) => { - // SUM computes the sum of array elements - self.walk_expr_as_view(arg)?; - self.push(Opcode::ArraySum {}); - self.push(Opcode::PopView {}); - return Ok(Some(())); + return self.emit_array_reduce(arg, Opcode::ArraySum {}); } BuiltinFn::VectorSelect(sel, expr, max_val, action, _err) => { self.walk_expr_as_view(sel)?; diff --git a/src/simlin-engine/src/compiler/context.rs b/src/simlin-engine/src/compiler/context.rs index 0e9e1d5c..a28f8a14 100644 --- a/src/simlin-engine/src/compiler/context.rs +++ b/src/simlin-engine/src/compiler/context.rs @@ -1223,7 +1223,27 @@ impl Context<'_> { active_dimension: self.active_dimension.as_deref(), }; - if let Some(operations) = normalize_subscripts3(indices, &config) { + if let Some(mut operations) = normalize_subscripts3(indices, &config) { + // In scalar context (no active A2A dimension and not + // inside an array-reducing builtin like SUM), resolve + // DimPosition(@N) to a concrete Single element offset. + // DimPosition normally preserves the dimension for A2A + // iteration, but in scalar context @N selects element N. + if self.active_dimension.is_none() && !self.preserve_wildcards_for_iteration { + for (i, op) in operations.iter_mut().enumerate() { + if let IndexOp::DimPosition(pos) = op { + let pos_1based = *pos + 1; + // pos_1based == 0 is defensive: normalize_subscripts3 + // already rejects @0, but we guard here too. + if pos_1based == 0 || pos_1based > dims[i].len() { + return sim_err!(MismatchedDimensions, id.as_str().to_string()); + } + // Convert to 0-based Single index + *op = IndexOp::Single(pos_1based - 1); + } + } + } + // Build a unified view for any combination of static operations let orig_dims: Vec = dims.iter().map(|d| d.len()).collect(); @@ -2129,35 +2149,57 @@ impl Context<'_> { } IndexExpr3::DimPosition(pos, dim_loc) => { - // @1, @2, etc. in dynamic context + let pos_val = *pos as usize; + + // Scalar context: no active A2A dimension, resolve @N directly + // to a concrete 1-based element offset in the target dimension. if self.active_dimension.is_none() { - return sim_err!( - ArrayReferenceNeedsExplicitSubscripts, - id.as_str().to_string() - ); - } - let active_subscripts = self.active_subscript.as_ref().unwrap(); - let pos = (*pos as usize).saturating_sub(1); - if pos >= active_subscripts.len() { - return sim_err!(MismatchedDimensions, id.as_str().to_string()); + if pos_val == 0 || pos_val > dims[i].len() { + return sim_err!(MismatchedDimensions, id.as_str().to_string()); + } + return Ok(SubscriptIndex::Single(Expr::Const( + pos_val as f64, + *dim_loc, + ))); } - let subscript = &active_subscripts[pos]; + // A2A context: try to resolve @N via the active subscript at + // this position (dimension-reordering path, e.g. matrix[@2, @1]). + // For named dimensions, element names are unique across dimensions, + // so get_offset reliably distinguishes elements — this also handles + // subdimension cases (e.g. selected[SubRegion] = data[@1]). + // For indexed dimensions, numeric element names overlap across + // unrelated dimensions (e.g. "2" is valid in both X and Y), so + // get_offset alone can't discriminate the mixed-wildcard case + // (row[Y] = matrix[@1, *]); we require an exact dimension match. + let active_subscripts = self.active_subscript.as_ref().unwrap(); + let active_dims = self.active_dimension.as_ref().unwrap(); let dim = &dims[i]; + let pos_0 = pos_val.saturating_sub(1); + if pos_0 < active_subscripts.len() { + let subscript = &active_subscripts[pos_0]; + let allow_binding = match dim { + Dimension::Named(..) => true, + Dimension::Indexed(..) => active_dims.iter().any(|ad| ad == dim), + }; + if allow_binding && let Some(offset) = dim.get_offset(subscript) { + return Ok(SubscriptIndex::Single(Expr::Const( + (offset + 1) as f64, + *dim_loc, + ))); + } + } - if let Some(offset) = dim.get_offset(subscript) { - Ok(SubscriptIndex::Single(Expr::Const( - (offset + 1) as f64, - *dim_loc, - ))) - } else if let Ok(idx_val) = subscript.as_str().parse::() { - Ok(SubscriptIndex::Single(Expr::Const( - idx_val as f64, - *dim_loc, - ))) - } else { - sim_err!(MismatchedDimensions, id.as_str().to_string()) + // A2A fallback for mixed cases (e.g. cube[@1, *, @3]) where + // the active subscript doesn't match the target dimension. + // Resolve to a concrete 1-based offset, same as scalar context. + if pos_val == 0 || pos_val > dims[i].len() { + return sim_err!(MismatchedDimensions, id.as_str().to_string()); } + Ok(SubscriptIndex::Single(Expr::Const( + pos_val as f64, + *dim_loc, + ))) } IndexExpr3::Expr(e) => { diff --git a/src/simlin-engine/src/compiler/subscript.rs b/src/simlin-engine/src/compiler/subscript.rs index cb087280..c03560ba 100644 --- a/src/simlin-engine/src/compiler/subscript.rs +++ b/src/simlin-engine/src/compiler/subscript.rs @@ -171,9 +171,12 @@ pub(crate) fn normalize_subscripts3( } IndexExpr3::DimPosition(pos, _) => { - // @1 is position 0, @2 is position 1, etc. - let dim_idx = (*pos as usize).saturating_sub(1); - IndexOp::DimPosition(dim_idx) + // @N is 1-based: @1 -> position 0, @2 -> position 1, etc. + // @0 is invalid; bail out so the caller reports an error. + if *pos == 0 { + return None; + } + IndexOp::DimPosition(*pos as usize - 1) } IndexExpr3::Expr(expr) => { diff --git a/src/simlin-engine/src/db_prev_init_tests.rs b/src/simlin-engine/src/db_prev_init_tests.rs index e6ccdcac..bb930d89 100644 --- a/src/simlin-engine/src/db_prev_init_tests.rs +++ b/src/simlin-engine/src/db_prev_init_tests.rs @@ -78,7 +78,7 @@ fn test_nested_previous_does_not_create_false_cycle_via_helper_deps() { .aux("x", "z + 1", None) .aux("z", "PREVIOUS(PREVIOUS(x))", None); - tp.assert_compiles(); + tp.assert_compiles_incremental(); tp.assert_sim_builds(); let vm = tp.run_vm().expect("VM should run"); diff --git a/src/simlin-engine/src/db_tests.rs b/src/simlin-engine/src/db_tests.rs index d117af11..8520d840 100644 --- a/src/simlin-engine/src/db_tests.rs +++ b/src/simlin-engine/src/db_tests.rs @@ -4971,7 +4971,7 @@ fn test_arrayed_1arg_previous_loadprev_per_element() { .array_aux("prev_val[DimA]", "PREVIOUS(base_val[DimA])"); // Verify compilation and simulation both succeed - tp.assert_compiles(); + tp.assert_compiles_incremental(); tp.assert_sim_builds(); let interp = tp @@ -5059,7 +5059,7 @@ fn test_arrayed_2arg_previous_per_element_modules() { .array_aux("prev_val[DimA]", "PREVIOUS(base_val[DimA], 99)"); // Verify compilation and simulation both succeed - tp.assert_compiles(); + tp.assert_compiles_incremental(); tp.assert_sim_builds(); let vm = tp.run_vm().expect("VM should run successfully"); diff --git a/src/simlin-engine/src/test_common.rs b/src/simlin-engine/src/test_common.rs index 0d33cbfc..94e3ba0a 100644 --- a/src/simlin-engine/src/test_common.rs +++ b/src/simlin-engine/src/test_common.rs @@ -529,21 +529,6 @@ impl TestProject { Ok(output) } - /// Test that compilation succeeds - pub fn assert_compiles(&self) { - match self.compile() { - Ok(_) => {} - Err(errors) => { - let error_msg = errors - .iter() - .map(|(loc, code)| format!("{loc}: {code:?}")) - .collect::>() - .join(", "); - panic!("Compilation failed with errors: {error_msg}"); - } - } - } - /// Test that compilation fails with specific error pub fn assert_compile_error(&self, expected_error: ErrorCode) { self.assert_compile_error_impl(expected_error)