engine: hoist nested vector builtins for scalar reducers

[bpowers]

Summary

• hoist nested array-producing builtins in scalar equations before assignment
• rewrite scalar nested vector-builtins using AssignTemp + TempArray so reducers consume full arrays
• add VM regression tests for reducer wrappers around vector_elm_map, vector_sort_order, and allocate_available

Root Cause

Scalar lowering (Ast::Scalar) did not run nested array-builtin hoisting. That left expressions like sum(vector_elm_map(...)) as direct builtin apps inside reducer args, and VM codegen cannot push views for those app nodes.

A2A nested-hoisting logic already handled array-value vs scalar-element argument modes, but test coverage only exercised interpreter behavior for reducer-wrapped builtins in array contexts.

Validation

• cargo test -p simlin-engine --test compiler_vector -- --nocapture
• cargo test -p simlin-engine --lib array_tests::vector_elm_map_tests -- --nocapture
• pre-commit hook (fmt, clippy, rust tests, ts checks, pysimlin tests)

Closes #367
Related to #366

[claude]

Code Review

Summary

This PR fixes scalar equation lowering to hoist nested array-producing builtins (like vector_elm_map, vector_sort_order, allocate_available) into AssignTemp + TempArray pairs so that reducers (SUM, MEAN, etc.) in the VM can consume full arrays. Previously, only A2A (apply-to-all) contexts performed this hoisting.

Analysis

Correctness of hoist_nested_array_builtins_in_scalar:

The new function delegates to the existing replace_nested_builtins_for_element with:

• elem_off: 0 and placeholder_view: ArrayView::contiguous(vec![1]) — a dummy 1-element view
• arg_mode: NestedBuiltinArgMode::ArrayValue — ensures nested builtins are replaced with TempArray (full array reference) rather than TempArrayElement

Since ArrayValue mode never calls project_var_index_to_temp, the placeholder view is never used for index projection. If ScalarElement mode were reached for a deeply nested builtin, project_var_index_to_temp(0, placeholder, builtin_view) correctly returns 0 because the placeholder has empty dimension names, so the dimension-matching loop is a no-op. This is semantically correct for scalar context.

Coverage of Ast::Scalar branches:

• Stock init_ast (line 647): hoisting added — correct
• Variable equation (line 713): hoisting added — correct
• Stock non-initial (line 674): no hoisting needed — this branch calls build_stock_update_expr (which computes current + dt * flows) rather than directly lowering the AST

Guard conditions (line 760):

The early return when is_array_producing_builtin(&main_expr) is true correctly avoids hoisting a top-level array-producing builtin (which would be handled by the existing A2A or arrayed code paths). The !contains_array_producing_builtin check correctly skips the transform when there's nothing to hoist.

Tests:

Six new VM regression tests cover vector_elm_map, vector_sort_order, and allocate_available in both scalar and array contexts, complementing the existing interpreter tests.

Findings

No bugs found.

Verdict

Correct. The patch correctly extends the existing A2A nested-hoisting logic to scalar equation contexts. The approach reuses the well-tested replace_nested_builtins_for_element infrastructure with appropriate parameters for the scalar case, and the new tests provide good coverage for all three array-producing builtin types.

[bpowers]

Completed one local automated review iteration with codex after opening this PR, and no additional actionable issues were identified relative to origin/main.

This branch keeps the compiler change focused on one root cause: scalar equations now hoist nested array-producing builtins into AssignTemp + TempArray, so reducer builtins consume full materialized arrays in VM codegen. The PR also adds VM regression coverage for reducer-wrapped vector_elm_map, vector_sort_order, and allocate_available, including the A2A shape tied to #366, so this behavior is guarded against future regressions.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1bc25d24d7

ℹ️ 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]

Restrict scalar hoisting to reducer array arguments

This helper rewrites any nested array-producing builtin in scalar equations using NestedBuiltinArgMode::ArrayValue, including cases where the parent operation expects scalar operands (for example max(vector_elm_map(source[*], offsets[*]), 15)). That produces TempArray nodes in scalar positions, and VM codegen’s scalar BuiltinFn::Max path (src/simlin-engine/src/compiler/codegen.rs, walk_expr(...).unwrap() around lines 794-799) panics because TempArray returns None outside view consumers. I reproduced this with a scalar max(vector_elm_map(...), 15) equation, which now crashes compilation instead of returning a structured compile error.

Useful? React with 👍 / 👎.

[codecov]

Codecov Report

❌ Patch coverage is 94.93671% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.07%. Comparing base (bb3e977) to head (9dfef46).
⚠️ Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/simlin-engine/src/compiler/mod.rs	85.71%	3 Missing ⚠️
src/simlin-engine/tests/compiler_vector.rs	98.27%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #369      +/-   ##
==========================================
+ Coverage   79.04%   79.07%   +0.03%     
==========================================
  Files         142      142              
  Lines       37396    37475      +79     
==========================================
+ Hits        29558    29635      +77     
- Misses       7838     7840       +2     

☔ 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

Reviewed the compiler changes for hoisting nested vector builtins in scalar reducer contexts.

Changes analyzed:

• hoist_nested_array_builtins_in_scalar function and its two call sites (Stock init + Var scalar branches)
• New ScalarContext variant of NestedBuiltinArgMode with scalar_child_mode() propagation
• scalar_child_mode substitution in 7 scalar-argument positions across Rank, VectorSelect, VectorSortOrder, and AllocateAvailable
• 5 new VM regression tests covering all three array-producing builtins

Verification:

• Traced ScalarContext propagation: correctly preserves itself in scalar positions while allowing ArrayValue to take effect inside reducers (Sum, Max(None), Min(None), etc.)
• Confirmed scalar_child_mode() is identity for existing ScalarElement/ArrayValue callers — no behavioral change for pre-existing A2A paths
• The unreachable! arm (line 1076) is safely guarded by the early return at lines 1060-1062
• The other.map(...) catch-all correctly passes ScalarContext through for 2-arg Max/Min, which preserves the compile error for max(vector_elm_map(...), 15) in scalar context (as tested)
• Placeholder view ArrayView::contiguous(vec![1]) is only a fallback in find_expr_array_view(...).unwrap_or(...) — reached only if the builtin's args lack StaticSubscript nodes, which shouldn't occur for well-formed wildcard-subscripted expressions

Findings: None. The patch is correct.

Overall correctness: Correct. Existing code paths are unaffected by the new enum variant, the new scalar hoisting logic is well-scoped, and test coverage validates the key scenarios for all three array-producing builtins.