engine: make previous fully intrinsic

[bpowers]

Summary

• Replace the remaining stdlib-backed PREVIOUS behavior with a single intrinsic two-argument path
• Unary PREVIOUS now desugars to PREVIOUS(x, 0), module-backed and expression arguments rewrite through scalar helper auxes
• LTM first-step guards use INITIAL_TIME semantics instead of an implicit previous model
• Removes stdlib/previous.stmx and its generated registration
• Updates dependency classification so PREVIOUS fallback expressions behave like init-only dependencies

Test plan

• All existing engine tests pass (cargo test)
• LTM tests updated and passing
• Interpreter/VM behavior aligned across direct, arrayed, SELF, module-backed, and nonzero-start cases

[claude]

Code Review

Reviewed the full diff (425 additions, 623 deletions across 32 files). This PR makes PREVIOUS fully intrinsic by:

1. Removing stdlib/previous.stmx and its generated model registration
2. Desugaring unary PREVIOUS(x) to PREVIOUS(x, 0) at both the Expr0 (builtins_visitor) and Expr1 lowering stages
3. Replacing module expansion for complex PREVIOUS args with synthesized scalar helper auxes via make_temp_arg()
4. Changing the VM's LoadPrev opcode to pop a caller-provided fallback, returning it when TIME == INITIAL_TIME
5. Updating LTM first-step guards from TIME = PREVIOUS(TIME) to TIME = INITIAL_TIME
6. Updating dependency classification so PREVIOUS fallback expressions are treated as init-only dependencies

No bugs found.

Key observations:

• The classify_dependencies change in variable.rs correctly generalizes init_referenced tracking: instead of the old explicit top-level Var/Subscript insertion for Init, variables encountered during walk with in_init=true are now added to init_referenced. This is more correct for nested expressions like INIT(a + b) and aligns with how PREVIOUS fallback deps are handled.

• The LTM guard change from PREVIOUS(TIME) = PREVIOUS(PREVIOUS(TIME)) to PREVIOUS(TIME, INITIAL_TIME) = INITIAL_TIME is actually a correctness improvement: the old module-based guard could fail to zero the second timestep when INITIAL_TIME != 0 (the module defaulted initial_value=0, causing PREVIOUS(TIME) at t=0 to return 0 instead of INITIAL_TIME).

• The dual desugaring sites (builtins_visitor at Expr0, expr1.rs at Expr1) are not redundant — builtins_visitor handles cases where the full parse pipeline runs, while expr1.rs handles direct lowering paths. Both correctly check arity before desugaring.

• contains_stdlib_call correctly adds "previous" so PREVIOUS equations still route through the builtins_visitor for temp-arg synthesis, even though is_stdlib_module_function("previous") now returns false.

Overall correctness: CORRECT — The patch is well-structured, tests are updated comprehensively, and the behavioral changes (INITIAL_TIME guards, intrinsic fallback) are improvements over the old module-based approach.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: d4fb6dc9b5

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Fix LoadPrev stack effect after adding fallback pop

Opcode::LoadPrev now consumes a fallback value (stack.pop()) before pushing the selected result, but the stack-effect metadata still treats it as (0, 1) in Opcode::stack_effect. That mismatch makes ByteCode::max_stack_depth overestimate stack usage for equations with many PREVIOUS(...) calls and can trigger false "compiled bytecode requires stack depth..." failures even when runtime execution is safe, so models can fail compilation purely due incorrect static stack accounting.

Useful? React with 👍 / 👎.

[codecov]

Codecov Report

❌ Patch coverage is 78.70968% with 33 lines in your changes missing coverage. Please review.
✅ Project coverage is 76.40%. Comparing base (bd6947d) to head (0a30c79).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
src/simlin-engine/src/units_check.rs	18.18%	9 Missing ⚠️
src/simlin-engine/src/compiler/codegen.rs	30.00%	7 Missing ⚠️
src/simlin-engine/src/builtins.rs	44.44%	5 Missing ⚠️
src/simlin-engine/src/patch.rs	0.00%	4 Missing ⚠️
src/simlin-engine/src/builtins_visitor.rs	93.33%	3 Missing ⚠️
src/simlin-engine/src/compiler/expr.rs	0.00%	2 Missing ⚠️
src/simlin-engine/src/ast/expr3.rs	83.33%	1 Missing ⚠️
src/simlin-engine/src/compiler/pretty.rs	0.00%	1 Missing ⚠️
src/simlin-engine/src/variable.rs	88.88%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #389      +/-   ##
==========================================
- Coverage   76.86%   76.40%   -0.46%     
==========================================
  Files         143      143              
  Lines       36171    36233      +62     
==========================================
- Hits        27802    27684     -118     
- Misses       8369     8549     +180     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Code Review

[P2] LoadPrev stack_effect metadata not updated for new pop semantics

File: src/simlin-engine/src/bytecode.rs, lines 895-900

The LoadPrev opcode now pops a fallback value from the stack before pushing its result (see vm.rs:1069), but stack_effect() still reports (0, 1) instead of (1, 1). This means max_stack_depth() over-estimates the stack size by 1 for every PREVIOUS use, since it doesn't account for the pop. The consequence is conservative — the VM stack is over-allocated rather than under-allocated, so there's no safety issue — but the metadata is incorrect and could mislead future code that relies on accurate stack accounting. The fix is changing the LoadPrev arm from the (0, 1) group to (1, 1).

---

Overall Correctness Verdict

Correct. The patch consistently transforms PREVIOUS from a stdlib module into a fully intrinsic two-argument builtin across all paths (parser, AST lowering, builtins visitor, codegen, VM, interpreter, LTM augmentation, unit checking, dependency classification). The stack_effect metadata issue above is non-blocking — it results in slightly over-allocated stacks but no incorrect behavior. Tests are thoroughly updated and the semantic changes (INITIAL_TIME guards in LTM equations) are actually an improvement for non-zero start times.

[claude]

Code Review

I reviewed the full diff across all 32 changed files. This PR converts PREVIOUS from a stdlib module-backed implementation to a fully intrinsic two-argument builtin (PREVIOUS(x, fallback)), with unary syntax desugaring to PREVIOUS(x, 0).

Analysis

VM/Interpreter consistency: Both the VM (vm.rs:1068-1076) and the interpreter (interpreter.rs:1367-1384) use identical semantics: check TIME == INITIAL_TIME via approx_eq, return fallback at the initial timestep, and read from prev_values otherwise. The codegen (codegen.rs:701-716) correctly pushes the fallback before emitting LoadPrev, matching the declared stack effect of (1, 1) in bytecode.rs:903.

Dependency classification (variable.rs): The ClassifyVisitor refactor replaces the explicit init_referenced.insert inside Init with a general in_init flag propagation through Var/Subscript branches. This is functionally equivalent for the common Init(Var(x)) case and is actually more correct for nested expressions. For Previous(arg, fallback), the arg is walked with in_previous = true (preventing addition to init_referenced via the !in_previous guard), while fallback is walked with in_init = true so its variable references are correctly treated as init-time dependencies.

Module routing: equation_is_stdlib_call correctly no longer special-cases PREVIOUS since it's removed from MODEL_NAMES. The collect_module_idents / is_module_backed_ident pipeline still correctly identifies SMTH1/DELAY/etc. as multi-slot modules, ensuring PREVIOUS(module_var) continues to synthesize scalar helper auxes via builtins_visitor.

Unit checking/inference: units_check.rs validates that fallback units match the lagged arg. units_infer.rs returns units from the lagged arg (not the fallback constant). Both are correct. The new test test_previous_infers_units_from_lagged_arg validates this.

LTM augmentation: First-timestep guards now use TIME = INITIAL_TIME instead of the old TIME = PREVIOUS(TIME). Second-timestep guards use PREVIOUS(TIME, INITIAL_TIME) = INITIAL_TIME. Both are consistent with the new intrinsic semantics.

Verdict

No bugs found. The patch is correct. All semantic changes are internally consistent across the VM, interpreter, codegen, dependency classification, unit analysis, and LTM subsystems. The desugaring is applied at multiple pipeline stages (builtins_visitor Expr0, expr1.rs Expr1) to handle both pre-expanded and raw inputs. Tests have been appropriately updated to reflect the new architecture.

[claude]

Code Review

Reviewed the full diff (569 additions, 634 deletions across 32 files). This PR replaces the stdlib-module-based PREVIOUS implementation with a fully intrinsic two-argument path: PREVIOUS(x, fallback) where unary PREVIOUS(x) desugars to PREVIOUS(x, 0).

Summary of changes

• BuiltinFn::Previous goes from 1-arg to 2-arg throughout the AST pipeline (Expr0→Expr1→Expr2→Expr3, compiler, interpreter, VM)
• LoadPrev opcode stack effect changes from (0,1) to (1,1): pops the fallback, pushes prev_values[off] or the fallback when TIME == INITIAL_TIME
• Non-scalar PREVIOUS args (module-backed vars, expressions, nested PREVIOUS) are rewritten through synthesized scalar helper auxes via the extracted make_temp_arg helper
• stdlib/previous.stmx and its generated registration are removed; MODEL_NAMES drops from 7 to 6
• LTM first-step guards switch from TIME = PREVIOUS(TIME) to TIME = INITIAL_TIME
• classify_dependencies correctly walks the fallback with in_init = true, and generalizes init_referenced population to all Var/Subscript nodes when in_init && !in_previous (which is more correct for complex expressions inside INIT)
• Unit checking and inference now validate that the PREVIOUS fallback has compatible units with the lagged argument

Findings

No blocking bugs found. The changes are mechanically consistent across the full pipeline:

1. Stack balance: Codegen emits walk_expr(fallback) (pushes 1) before LoadPrev (pops 1, pushes 1) — net +1, same as before. ✓
2. Behavioral equivalence for unary PREVIOUS: Desugared PREVIOUS(x, 0) returns 0 at t=INITIAL_TIME (matching the old zero-initialized prev_values behavior). ✓
3. LTM two-step guard correctness: PREVIOUS(TIME, INITIAL_TIME) = INITIAL_TIME correctly returns true at both t=0 (fallback) and t=1 (prev_values holds INITIAL_TIME), yielding 0 for the first two timesteps as intended. ✓
4. Temp arg pipeline: make_temp_arg creates an aux with print_eqn(arg) as its equation, so nested constructs (e.g., PREVIOUS(PREVIOUS(x)) or PREVIOUS(TIME)) correctly compile when the temp aux is later parsed through the normal pipeline. ✓
5. ModuleInput removal in codegen: The old ModuleInput fallback in Previous codegen is replaced with a hard error, which is safe since builtins_visitor now rewrites all module-input cases through temp args before codegen.

The init_referenced generalization (capturing all variables inside INIT(expr) rather than just the top-level Var) is a behavior change that adds more variables to the initials runlist, but this is arguably more correct — INIT(a + b) should ensure both a and b are computed during initialization.

Overall correctness: correct — the patch is internally consistent, tests have been appropriately updated, and existing behavior is preserved through the desugaring equivalence.

[bpowers]

Review cycle summary

Three review iterations with both codex and Claude reviewers. The initial commit was clean and well-structured; reviews caught two real bugs and prompted several improvements:

Bugs fixed: LoadPrev stack effect metadata was (0,1) instead of (1,1) since the opcode now pops a fallback. Unit inference for PREVIOUS was returning units from the fallback argument instead of the lagged argument, causing variables using unary PREVIOUS(x) to lose their inferred units. Both fixed with regression tests.

Improvements: Added equality constraint between PREVIOUS's lagged argument and fallback in unit inference (matching Max/Min handling), so incompatible fallback units are caught during inference rather than only at check time. Removed the dead _model_name parameter from classify_module_for_ltm. Added explanatory comments for the interpreter's non-Var fallback invariant and the dual desugaring in expr1.rs.