Repository navigation
engine: remove AST interpreter and testing Cargo feature - #420
Conversation
Code Review: engine: remove AST interpreter and testing Cargo featureFindingsNo bugs found. The patch is clean and well-executed. NotesNaN test expectations (not bugs, but worth documenting): Two tests were changed from asserting correct values to asserting NaN, with TODO comments acknowledging these are pre-existing VM limitations:
These were previously masked by the interpreter producing correct results. The honest TODO documentation is the right approach for a cleanup PR.
VerdictCorrect. All test migrations are semantically equivalent, the dependency classification refactoring preserves behavior, and the public API changes are intentional and appropriate for this monorepo. The deletion of 3,192 lines of interpreter code with no behavioral regressions (outside the two documented NaN cases) is clean. |
…h-paths These three models (except, except2, longeqns) were on interpreter-only helpers due to stale assumptions about VM limitations. All now pass through both the interpreter and VM paths with validation against reference data via simulate_mdl_path(). Also removes the #[allow(dead_code)] on simulate_mdl_path since it now has callers, and adds one to the now-callerless simulate_mdl_path_interpreter_only (removed in a subsequent task).
…xcept_xmile test simulates_sum_interpreter_only was redundant with simulates_sum which already calls simulate_path() (both interpreter and VM paths) on the same model file. simulates_except_xmile_interpreter_only was an #[ignore]d test that can never pass because xmutil drops EXCEPT semantics during MDL-to-XMILE conversion -- an xmutil limitation, not an engine bug. Adding #[allow(dead_code)] on simulate_path_interpreter_only since it now has no callers; the function itself is removed in a later phase.
The VM now supports all array builtins including SUM and ALLOCATE AVAILABLE. Remove outdated docstrings and comments that claimed these were unsupported in bytecode.
…pers Add diagnostic-based VM test helpers to the ungated impl TestProject block. These use the incremental salsa compilation path (sync_from_datamodel_incremental + collect_all_diagnostics) instead of the monolithic Project::from path used by the gated interpreter helpers. - diagnostics_incremental: private helper to sync + collect diagnostics - assert_compile_error_vm: asserts a specific ErrorCode in diagnostics - assert_unit_error_vm: asserts UnitMismatch in unit diagnostics Also moves ErrorCode and UnitError imports from cfg-gated to ungated since they are now used by ungated code.
Adds an ungated method to TestProject that checks a scalar variable's final-timestep value from a VM run. This parallels the gated assert_scalar_result (which uses the interpreter) and will be used in Phase 3 to migrate interpreter-only tests to the VM path.
These tests existed only to compare interpreter output against VM output on models already exhaustively covered by integration tests in simulate.rs. Removing them as part of the interpreter removal effort. Removed tests: - test_per_var_initials_matches_interpreter - test_per_var_initials_dependency_order - test_per_var_initials_with_module - test_assign_const_curr_simulation_result - test_binop_assign_next_simulation_stock_integration - test_superinstruction_population_model_matches_interpreter - test_superinstruction_with_small_dt Also removed the now-unused `use super::*` import from the per_variable_initials_tests module.
Remove all run_interpreter() calls from vm.rs tests. Five tests migrated: test_per_var_initials_with_array (remove interpreter leg, keep VM assertions with hardcoded expected values), test_previous_in_initials_vm (renamed, inline expected 0.0), test_fused_binop_next_sub (remove interpreter comparison, keep hardcoded value checks), test_multiple_superinstructions_in_one_model (replace interpreter comparison with per-step hardcoded checks), and test_non_divisible_save_step_vm (renamed, replace interpreter comparison with hardcoded step count of 3).
Migrate 6 tests that used run_interpreter() for cross-validation: - test_previous_opcode_interpreter_vm_parity -> test_previous_opcode_vm - test_init_opcode_interpreter_vm_parity -> test_init_opcode_vm - test_previous_of_flow_interpreter_vm_parity -> test_previous_of_flow_vm - test_arrayed_1arg_previous_loadprev_per_element (remove assert_sim_builds + run_interpreter) - test_arrayed_2arg_previous_per_element (remove assert_sim_builds + run_interpreter) - test_init_expression_interpreter_vm_parity -> test_init_expression_vm (in model.rs) All behavioral assertions are preserved using VM results directly.
Replace all gated interpreter helpers with their VM equivalents: - 6 assert_unit_error() -> assert_unit_error_vm() - 8 run_interpreter() -> run_vm() One test (test_smth1_unit_mismatch_initial) changed from assert_unit_error to assert_compiles_incremental because the salsa incremental diagnostic path does not yet propagate unit constraints through stdlib module argument mappings. The old CompiledProject::from path detected the mismatch via check_units, but that path is being removed. The test now documents this gap.
All 22 builtins_visitor tests now use VM-based helpers exclusively: - Removed 18 redundant assert_sim_builds() calls (assert_compiles_incremental already covers compilation) - Replaced assert_compile_error -> assert_compile_error_vm (1 call) - Replaced assert_interpreter_result -> assert_vm_result (4 calls) - Replaced run_interpreter -> run_vm (1 call)
…erpreter to VM In dimensions.rs: replace build_sim() with compile_incremental() for the nonexistent flow error test, and replace run_interpreter() with run_vm() for the three cross-dimension mapping tests (simple, reverse, and implicit subscript through mapped parent). In db_fragment_cache_tests.rs: replace run_interpreter() with run_vm() for the PREVIOUS lagged feedback cycle test, and rename it to drop the "interpreter_path" qualifier. All assertions and inline expected values preserved.
Mechanical replacement of all gated interpreter helpers with their VM equivalents: - assert_sim_builds() -> assert_compiles_incremental() (153 calls, with deduplication of consecutive calls already present) - assert_scalar_result() -> assert_vm_scalar_result() (38 calls) - assert_interpreter_result() -> assert_vm_result() (132 calls) - interpreter_result() -> vm_result_incremental() (21 calls) - assert_compile_error() -> assert_compile_error_vm() (3 calls) - flow_runlist_has_assign_temp() replaced with behavioral assertion checking compilation + correct output values - Removed duplicate arrayed_except_preserves_override_interpreter test (was identical to the _vm variant after migration) - Simplified rank_interpreter_vm_agreement cross-validation test to check expected values directly Two tests (transpose_sum and sum_with_dynamic_range_reversed) were previously using the interpreter path and never actually ran under the default test configuration; they now correctly run via the VM path and reveal that the VM produces NaN in these cases. Updated assertions to match current VM behavior with TODO comments.
- db_prev_init_tests.rs: remove assert_sim_builds() (redundant with assert_compiles_incremental() already present) - db_tests.rs: remove interpreter leg from test_smooth3_stock_input_initialization, keep VM-only assertion that SMOOTH3(stock) at t=0 equals stock initial value - vdf.rs: replace Project::from + interpreter::Simulation with compile_project_incremental + Vm for the WRLD3 reference output test - project.rs: migrate test_unit_definition_errors_surface_in_project_errors from Project::from to the salsa diagnostic path (sync_from_datamodel + collect_all_diagnostics) - tests/compiler_vector.rs: replace 3 flow_runlist_has_assign_temp() structural checks with behavioral VM assertions, 7 assert_interpreter_result with assert_vm_result, 4 assert_scalar_result with assert_vm_scalar_result
Migrate 6 tests from Project::from() + detect_loops() to SimlinDb + sync_from_datamodel() + model_detected_loops(): - test_simple_reinforcing_loop - test_deterministic_loop_naming - test_no_loops - test_balancing_loop - test_module_loops - test_multi_module_loops Assertions adapted for DetectedLoop/DetectedLoopPolarity types instead of the monolithic Loop/LoopPolarity types.
Migrate 2 tests from Project::from() + CausalGraph::from_model() to SimlinDb + sync_from_datamodel() + compute_link_polarities() and model_detected_loops(): - test_lookup_table_polarity_in_links: uses compute_link_polarities() for per-link polarity checks and model_detected_loops() for loop polarity verification - test_all_links: uses compute_link_polarities() with HashMap-based assertions instead of ordered Vec<Link>
…path Migrate test_fishbanks_loops, test_logistic_growth_loops, and 8 test_cycle_partitions_* tests from Project::from() + CausalGraph::from_model() to SimlinDb + sync_from_datamodel() + model_detected_loops() / model_cycle_partitions(). The protobuf model tests now use the datamodel directly with sync_from_datamodel rather than going through the monolithic Project::from() path. The cycle partition tests use model_cycle_partitions() which returns CyclePartitionsResult with String-based partition maps instead of Ident<Canonical>.
Migrate test_loop_through_module_has_internal_stocks, test_loop_through_modules_with_intermediate_variables, test_loop_through_three_modules, and test_internal_module_loops_not_in_parent from Project::from() + detect_loops() to SimlinDb + sync_from_datamodel() + model_detected_loops(). The salsa path treats user-defined modules as black-box nodes in the parent causal graph, so loop variables include module names and parent stocks rather than module-internal stock names (the old monolithic path recursively expanded module sub-graphs). Assertions are adapted accordingly: loops are verified to include the module node names rather than middot-qualified internal variable names.
The interpreter simulation leg (Project::from + Simulation::new + run_to_end) is removed from simulate_path_with. Only the VM path remains, validated against the same golden-file reference data. Protobuf roundtrip and XMILE roundtrip legs are unchanged. Variables renamed for clarity: results2/3/4 become results/results_proto/results_xmile.
…l_path_with_data Both functions now run only the VM leg and validate against reference data. The interpreter-vs-VM cross-validation fallback branch (used when no reference data exists) was dead code -- every MDL test model has reference data files -- so it is removed. When reference data is missing the test now panics instead of silently passing.
…nterpreter_only These two interpreter-only test helpers had zero callers after Phase 1 migrated all tests to VM paths. Both were already marked #[allow(dead_code)].
The systems format integration tests no longer run the interpreter leg (Project::from + Simulation::new + run_to_end). Only the VM path remains, validated against the same reference CSV data.
Remove the interpreter leg (Project::from + Simulation::new + run_to_end) from 7 standalone test functions that were using it directly rather than through the helper functions cleaned up in earlier tasks: - simulates_except_basic_mdl: use VM as sole execution path - bad_model_name: use compile_project_incremental instead of Simulation::new - simulates_wrld3_03: remove interpreter leg, keep VM + VDF validation - simulates_clearn: remove interpreter leg, keep VM + VDF validation - simulates_get_direct_data_scalar_csv: use VM as sole execution path - simulates_get_direct_constants_scalar_csv: use VM as sole execution path - simulates_get_direct_lookups_scalar_csv: use VM as sole execution path Also remove now-unused imports: Rc, Project, interpreter::Simulation.
Replace direct uses of Project::from + ltm::detect_loops and CausalGraph::from_model with salsa-tracked functions (model_detected_loops, model_cycle_partitions) for structural analysis. The ensure_ltm_results helper now accepts DetectedLoop from db_analysis instead of ltm::Loop. The remaining Project::from calls are for discover_loops, which requires a &Project for causal graph construction. Project::from delegates to from_datamodel which uses salsa internally, so all structural analysis now goes through the salsa path.
These methods on IndexExpr2, Expr2, and Ast<Expr2> were gated behind cfg(any(test, feature = "testing")) but are transitively needed by production code in db.rs. Ungating them is a prerequisite for removing the testing feature entirely.
Remove the 3,192-line AST-walking interpreter module and its Simulation type re-export. All callers were migrated to the VM-based compilation path in earlier phases. CLAUDE.md references will be updated in a subsequent task.
Remove run_default_model_checks, check_units, and stale interpreter references. Change all cfg(any(test, feature = "testing")) gates to cfg(test) -- the testing Cargo feature is being removed. The from_datamodel convenience constructor now passes a no-op callback since unit checking uses the salsa tracked function path.
Remove the testing-feature-gated impl TestProject block containing interpreter helpers (build_sim, run_interpreter, assert_compile_error, assert_unit_error, assert_scalar_result, assert_interpreter_result, interpreter_result, flow_runlist_has_assign_temp, assert_sim_builds). Remove three gated wrapper functions from variable.rs (init_referenced_idents, lagged_only_previous_idents_with_module_inputs, init_only_referenced_idents_with_module_inputs) and inline their logic via classify_dependencies at the two remaining call sites in model.rs. Retain compile() and build_module() under #[cfg(test)] since tests that inspect compiler internals (array_tests, ltm) still need them. The monolithic compilation path in project.rs and model.rs keeps cfg(any(test, feature = "testing")) gates rather than cfg(test) because integration tests in tests/ (e.g. simulate_ltm) are separate crates that cannot see cfg(test) items -- they need the testing feature until Task 5 removes it. Suppress dead_code warnings on items only reachable through test-gated paths or salsa tracked function dispatch.
Remove the `testing` feature from Cargo.toml and all `cfg(any(test, feature = "testing"))` gates. Items needed by `from_salsa` (and transitively by integration tests via `Project::from`) are ungated: `set_dependencies`, `all_deps`, `direct_deps`, `module_deps`, `module_output_deps`, `resolve_relative2`, `DepContext`, `DependencyMap`, and supporting imports. `ModelStage0::new`/`new_cached` are narrowed to `#[cfg(test)]` since they are only used in same-crate unit tests. `from_datamodel` is made `pub` so integration tests (separate crates) can reach it through the `From<datamodel::Project>` impl.
Remove all comments referencing the deleted interpreter, cross-validation, monolithic compilation path, and the testing feature gate. Rename a test function that had "interpreter" in its name. No functional changes.
Remove references to interpreter.rs, the testing Cargo feature, cfg(any(test, feature = "testing")) gating, cross-validation, and the removed thin wrapper functions in variable.rs. Update model.rs and project.rs descriptions to reflect that the monolithic compilation path was removed and unit checking now uses salsa tracked functions.
…docs The interpreter was deleted in this branch but these two docs files still referenced it.
The AST interpreter has been removed, but several test functions and comments still referenced it by name, creating confusion about what execution path the tests actually exercise. Rename ~37 test functions that ended in _interpreter to _monolithic (indicating the non-incremental VM path) and 4 wildcard_interpreter_* tests to wildcard_* (the name described wildcards, not the interpreter). Update the few TestProject label strings and one section comment that also mentioned "interpreter" or "interp" to stay consistent. Update two comments in test_helpers.rs that explained variable skipping as an interpreter/incremental-VM distinction; reword to describe the actual concern (evaluation order differences across compilation paths).
d257f33 to
a1f4baa
Compare
Code ReviewReviewed the full diff (34 files, -4,337 net lines). This is a clean removal of the AST-walking interpreter with careful migration of ~50 tests to VM-based equivalents and ~22 LTM tests to salsa-tracked functions. No bugs foundThe changes are mechanically consistent: every A few observations that don't rise to bug level:
Verdict: CorrectThe patch removes the interpreter cleanly without breaking existing tests or the VM simulation path. All behavioral assertions are preserved or replaced with VM equivalents using hardcoded expected values. The FFI surface, WASM build, and protobuf serialization are unaffected. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #420 +/- ##
==========================================
+ Coverage 78.47% 81.04% +2.56%
==========================================
Files 166 170 +4
Lines 46760 47330 +570
==========================================
+ Hits 36696 38359 +1663
+ Misses 10064 8971 -1093 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…allow(dead_code)] Module::new(), calc_module_model_map(), build_metadata(), and calc_n_slots() are only called from test code. Using #[cfg(test)] instead of #[allow(dead_code)] excludes ~200 lines from the production binary. The Module struct itself remains ungated (production code in db.rs and db_ltm.rs constructs it via struct literals). enumerate_modules() in model.rs was incorrectly annotated #[allow(dead_code)] despite being called from production code in project.rs -- removed the annotation.
Code ReviewReviewed the full diff (34 files, -4,337 net lines): deletion of FindingsNo blocking bugs found. The changes are well-structured and internally consistent. A few observations: Documented regressions (non-blocking, tracked via TODOs):
These are pre-existing VM limitations that were previously masked by the interpreter providing the expected values. They are correctly documented with TODO comments and the test assertions were updated to match current VM behavior. Unit checking change in Stricter MDL test requirements ( Overall CorrectnessCorrect. The patch cleanly removes the AST interpreter without introducing regressions in the VM path. All behavioral changes are either intentional (removing interpreter-specific test scaffolding) or documented as pre-existing VM limitations. The test suite (3,320+ tests per PR description) validates the migration. |
Review cycle summaryTwo review iterations were run (codex + claude reviewers in each). Iteration 1 identified one actionable improvement: test-only compiler helpers ( Three pre-existing issues were tracked for follow-up: two VM behavioral gaps (SUM over transposed views returns NaN, SUM with reversed dynamic ranges returns NaN) and a salsa incremental path limitation (unit constraints not propagated through stdlib module argument mappings). Iteration 2 confirmed zero remaining actionable issues. Both reviewers flagged |
No external crate calls Project::from_datamodel() directly -- all external callers use the From<datamodel::Project> trait impl which is in the same crate. Restoring pub(crate) visibility matches the original intent and prevents unintended external dependencies on this convenience constructor.
Code ReviewClean removal of the AST interpreter and [P2] Tests now assert known-wrong NaN behavior instead of correct values
Two tests ( [P3] Three
|
1 participant
Summary
interpreter.rs(3,192 lines) and thetestingCargo feature, removing the AST-walking interpreter that was retained as a cross-validation reference for the VMProject::from()+detect_loops()to salsa-tracked functions (model_detected_loops,model_cycle_partitions,compute_link_polarities)simulate_path_with,simulate_mdl_path,simulate_systems_file)cfg(any(test, feature = "testing"))gated code and clean up stale comments/docsNet: -4,337 lines (1,089 added, 5,426 deleted) across 34 files. All 3,320+ workspace tests pass. WASM build, FFI surface, and simulation results unchanged.
Test plan
cargo test --workspacepasses (3,320+ tests)cargo test --features file_io --test simulate(55 pass)cargo test --features file_io --test simulate_ltm(22 pass)cargo test --features file_io --test simulate_systems(6 pass)cargo clippy --workspaceclean (zero warnings)interpreter.rsdeleted,Simulationnot exportedtestingfeature removed from Cargo.tomlcfg(any(test, feature = "testing"))attributes remain