engine: migrate assert_compiles to incremental path and delete#387
engine: migrate assert_compiles to incremental path and delete#387
Conversation
Catalogs all 26 assert_compiles() call sites, identifies the two incremental compiler gaps (@n position syntax in scalar context, MEAN with dynamic ranges), and designs the migration path including an array-reduce codegen abstraction.
When the LHS is a scalar variable (no active A2A dimension and not inside an array-reducing builtin), resolve DimPosition(@n) to a concrete element offset in two places: 1. In the static subscript path (normalize_subscripts3 result handling), convert DimPosition ops to Single ops so the built view correctly collapses the dimension to a scalar element. 2. In the dynamic subscript path (lower_index_expr3), resolve @n to a 1-based Expr::Const rather than returning ArrayReferenceNeedsExplicitSubscripts. Also adds a fallback for mixed A2A contexts (e.g. cube[@1, *, @3]) where the active subscript doesn't match the target dimension. Bounds checking rejects @0 and out-of-range @n with MismatchedDimensions in both paths. Switches dimension_position_single and dimension_position_and_wildcard tests to use assert_compiles_incremental().
Add two tests in dimension_position_tests verifying that the incremental compiler rejects invalid @n positions: - dimension_position_zero_is_error: @0 is rejected (1-based, 0 is invalid) - dimension_position_out_of_range_is_error: @5 is rejected for a size-3 dimension Also fix normalize_subscripts3 to bail out on @0 rather than silently treating it the same as @1 via saturating_sub.
The pos_1based == 0 condition in the static subscript path is unreachable because normalize_subscripts3 already rejects @0, but we keep the check as defense-in-depth. Add a comment explaining this.
Unify the repeated walk_expr_as_view + push(opcode) + push(PopView) pattern into a single emit_array_reduce helper method. Refactor SUM, SIZE, STDDEV, and the 1-arg paths of MIN/MAX to use it. No behavior change -- this is a pure mechanical refactor.
…duce The MEAN handler had an is_array guard that only matched StaticSubscript and TempArray, missing Expr::Subscript with Range indices. This caused MEAN with dynamic range arguments (e.g. MEAN(data[start:end])) to fall through to the scalar mean path, producing wrong results on the incremental compilation path. Route all single-arg MEAN through emit_array_reduce, which calls walk_expr_as_view and handles all array expression types including dynamic ranges. The multi-argument scalar path remains for len > 1.
Now that MEAN routes through emit_array_reduce (which handles all array expression types including dynamic subscript ranges), this test passes on the incremental compilation path.
Replace all 20 assert_compiles() calls with assert_compiles_incremental() in builtins_visitor.rs test module, migrating these tests from the monolithic compilation path to the salsa-based incremental path.
Replace assert_compiles() with assert_compiles_incremental() in the remaining 3 test functions: test_arrayed_1arg_previous_loadprev_per_element, test_arrayed_2arg_previous_per_element_modules, and test_nested_previous_does_not_create_false_cycle_via_helper_deps.
All callers have been migrated to assert_compiles_incremental(). The compile() method is retained because it has active callers in assert_compile_error_impl() and tests/simulate_ltm.rs.
Document @n position syntax resolution semantics in context.rs (scalar vs A2A vs array-reducing builtin contexts) and the emit_array_reduce helper in codegen.rs.
Code ReviewI reviewed the full diff across all 10 changed files. The changes are well-structured: the No bugs found. Overall correctness: Correct. The patch is free of bugs and blocking issues. Existing code and tests will not break. 🤖 Generated with Claude Code |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 93a02c5f74
ℹ️ 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".
| 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(())); | ||
| } | ||
| return self.emit_array_reduce(&args[0], Opcode::ArrayMean {}); |
There was a problem hiding this comment.
Routing every one-argument MEAN(...) call through emit_array_reduce makes scalar inputs fail compilation, because walk_expr_as_view only accepts array-like nodes (StaticSubscript, TempArray, Var, Subscript) and rejects scalars like constants or compound scalar expressions. This is a regression from the previous behavior where MEAN(x) with a non-array argument used the scalar path and evaluated to x (sum/divide by 1), so models using forms like MEAN(42) or MEAN(a+b) now error on the incremental compiler.
Useful? React with 👍 / 👎.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #387 +/- ##
==========================================
+ Coverage 76.80% 76.87% +0.07%
==========================================
Files 143 143
Lines 36171 36171
==========================================
+ Hits 27781 27808 +27
+ Misses 8390 8363 -27 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Phase files and test requirements for the three-phase migration: P1 @n position syntax, P2 MEAN dynamic ranges, P3 bulk test switch.
Code ReviewSummaryThis PR makes three substantive changes: (1) resolves Analysis
MEAN fix (codegen.rs): The old code only checked for Test migration: Mechanical FindingsNo bugs found. The changes are well-scoped, the Overall Correctness VerdictCorrect. The patch introduces no regressions — existing code paths produce identical results, the new scalar-context |
Two bugs found during code review: 1. In A2A context, @n resolved via active subscripts even when the target dimension wasn't being iterated. For indexed dimensions with overlapping numeric ranges, this caused cross-dimension matching (e.g., row[Y] = matrix[@1, *] used the Y iteration value as an X subscript). Fix: only resolve @n through active subscripts when dims[i] is actually among the active iteration dimensions. 2. Single-arg MEAN unconditionally routed through emit_array_reduce, which calls walk_expr_as_view. That function only handles array-shaped expressions (StaticSubscript, TempArray, Var, Subscript) and errors on composite scalar expressions like Op2. Fix: match on expression type, routing only array forms through emit_array_reduce and evaluating scalar expressions directly.
Code ReviewI've reviewed the full diff across all 14 commits. The changes are well-structured and the code logic is sound. Summary of changes reviewed:
FindingsNo bugs found. The patch is clean. A few observations (not actionable):
Verdict: CorrectThe patch should not break existing code or tests. All behavioral changes are covered by new and existing tests. |
Exercises the Expr::Var arm in the MEAN handler which routes through emit_array_reduce/walk_expr_as_view. Confirms the VM's ArrayMean opcode handles a 1-element view correctly (returns the value itself).
Code ReviewSummaryThis PR migrates all 26 FindingsNo bugs found. The changes are well-structured and thoroughly tested:
Verdict: CorrectThe patch is free of bugs. Test migration is mechanical and safe, the two feature-gap fixes are well-reasoned with good boundary-condition tests ( 🤖 Generated with Claude Code |
For named dimensions, element names are unique across dimensions, so get_offset reliably distinguishes cross-dimension false positives. This correctly handles subdimension cases (e.g. selected[SubRegion] = data[@1] where SubRegion elements are a subset of Region). For indexed dimensions, numeric element names overlap across unrelated dimensions, so we require an exact dimension identity match to prevent the mixed-wildcard bug (row[Y] = matrix[@1, *]). Also adds a clarifying comment on the MEAN handler explaining why it needs expression-type matching unlike other array-reduce builtins.
Code ReviewI reviewed all source code changes in this PR (compiler/context.rs, compiler/codegen.rs, compiler/subscript.rs, array_tests.rs, builtins_visitor.rs, db_tests.rs, db_prev_init_tests.rs, test_common.rs). FindingsNo bugs found. Notes
Overall CorrectnessCorrect. The patch is well-structured, the logic changes are sound, and the test coverage is thorough. Existing code and tests will not break. |
Review cycle summaryFour iterations of automated review (codex + Claude) identified and fixed two genuine bugs, then refined the fix for correctness across dimension types. Bugs fixed:
Both fixes include targeted regression tests written before the implementation (TDD). A third test ( |
1 participant
Summary
@Nposition syntax to concrete element offsets in scalar context on the incremental compilation path, fixingarr[@1]and mixed cases likecube[@1, *, @3]emit_array_reducehelper eliminating 6x duplication of the walk_expr_as_view/ArrayOp/PopView pattern across SUM, SIZE, STDDEV, MIN, MAX, MEANMEAN(data[start:end])) by routing single-arg MEAN throughemit_array_reduceinstead of a narrowis_arrayguardassert_compiles()tests toassert_compiles_incremental()and deleteassert_compiles()from test_common.rsCloses #377
Test plan
dimension_position_zero_is_error,dimension_position_out_of_range_is_errordimension_positiontests pass (7 existing + 2 new)mean_with_dynamic_rangepasses on incremental pathdocs/test-plans/2026-03-08-assert-compiles-migration.md