Fix VM slot collision in nested list/map comprehensions

The accumulator slot for an outer list comprehension was assigned
post-analysis as `max_outer_loop_var_slot + 1`.  Inner loop variables
were allocated from the same scope offset and therefore received the
same slot number (e.g. outer acc = 3, inner `_` = 3).  At runtime the
inner `SetLocal` silently overwrote the outer accumulator list, causing
a panic "ListPush expects a list".

Fix: reserve the accumulator slot in the scope tree *before* analysing
the body expression via a new `ScopeTree::reserve_anonymous_slot()`.
This bumps the scope's offset so any nested loop variables receive
strictly higher slot numbers.  The old post-hoc calculation in
`Expression::For` is removed.

Adds regression test bug0008.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

Author: timfennis

ndc_interpreter/src/semantic/analyser.rs

@@ -3,8 +3,7 @@ use crate::semantic::ScopeTree;
 use itertools::Itertools;
 use ndc_lexer::Span;
 use ndc_parser::{
-    Binding, Expression, ExpressionLocation, ForBody, ForIteration, Lvalue, ResolvedVar,
-    TypeSignature,
+    Binding, Expression, ExpressionLocation, ForBody, ForIteration, Lvalue, TypeSignature,
 };
 use std::fmt::Debug;
 
@@ -193,20 +192,6 @@ impl Analyser {
             }
             Expression::For { iterations, body } => {
                 let return_type = self.resolve_for_iterations(iterations, body, *span)?;
-                // Assign the VM accumulator slot for list comprehensions. We do this after
-                // resolution so we can derive the slot from the resolved loop-variable slots
-                // without touching the scope tree (the interpreter doesn't need this slot).
-                let max_loop_slot = iterations.iter().filter_map(iteration_local_slot).max();
-                let acc_slot = Some(max_loop_slot.map_or(0, |s| s + 1));
-                match body.as_mut() {
-                    ForBody::List {
-                        accumulator_slot, ..
-                    } => *accumulator_slot = acc_slot,
-                    ForBody::Map {
-                        accumulator_slot, ..
-                    } => *accumulator_slot = acc_slot,
-                    ForBody::Block(_) => {}
-                }
                 Ok(return_type)
             }
             Expression::Call {
@@ -379,13 +364,25 @@ impl Analyser {
                     self.analyse(block)?;
                     StaticType::unit()
                 }
-                ForBody::List { expr, .. } => StaticType::List(Box::new(self.analyse(expr)?)),
+                ForBody::List {
+                    expr,
+                    accumulator_slot,
+                    ..
+                } => {
+                    // Reserve the accumulator slot BEFORE analysing the body so
+                    // that nested for-comprehensions receive strictly higher slot
+                    // numbers and cannot collide with this accumulator.
+                    *accumulator_slot = Some(self.scope_tree.reserve_anonymous_slot());
+                    StaticType::List(Box::new(self.analyse(expr)?))
+                }
                 ForBody::Map {
                     key,
                     value,
                     default,
+                    accumulator_slot,
                     ..
                 } => {
+                    *accumulator_slot = Some(self.scope_tree.reserve_anonymous_slot());
                     let key_type = self.analyse(key)?;
                     let value_type = if let Some(value) = value {
                         self.analyse(value)?
@@ -580,27 +577,6 @@ impl Analyser {
     }
 }
 
-/// Returns the highest local slot index used by the loop variable of `it`.
-/// Used to find the highest-numbered loop variable slot when assigning the comprehension
-/// accumulator slot (which must sit above all loop variable slots).
-fn iteration_local_slot(it: &ForIteration) -> Option<usize> {
-    let ForIteration::Iteration { l_value, .. } = it else {
-        return None;
-    };
-    max_lvalue_slot(l_value)
-}
-
-fn max_lvalue_slot(lv: &Lvalue) -> Option<usize> {
-    match lv {
-        Lvalue::Identifier {
-            resolved: Some(ResolvedVar::Local { slot }),
-            ..
-        } => Some(*slot),
-        Lvalue::Sequence(seq) => seq.iter().filter_map(max_lvalue_slot).max(),
-        _ => None,
-    }
-}
-
 #[derive(thiserror::Error, Debug)]
 #[error("{text}")]
 pub struct AnalysisError {

ndc_interpreter/src/semantic/scope.rs

@@ -448,6 +448,14 @@ impl ScopeTree {
         }
     }
 
+    /// Reserve a slot in the current scope without creating a named binding.
+    /// Used to allocate the list/map accumulator before analysing the body of a
+    /// for-comprehension, so that any nested comprehensions receive strictly
+    /// higher slot numbers and cannot collide with this accumulator.
+    pub(crate) fn reserve_anonymous_slot(&mut self) -> usize {
+        self.scopes[self.current_scope_idx].allocate("\x00".to_string(), StaticType::Any)
+    }
+
     pub(crate) fn update_binding_type(&mut self, var: ResolvedVar, new_type: StaticType) {
         match var {
             ResolvedVar::Local { slot } => {

tests/programs/900_bugs/bug0008_nested_list_comp_acc_slot.ndc

@@ -0,0 +1,20 @@
+// BUG#0008: The VM accumulator slot for an outer list comprehension was
+// computed post-analysis as max_outer_loop_var_slot + 1.  Nested loop
+// variables were assigned slots from the same scope offset, so they
+// received the *same* number (e.g. outer acc = slot 3, inner _ = slot 3).
+// At runtime the inner SetLocal overwrote the outer list, causing a panic
+// "ListPush expects a list".
+
+let height = 3;
+let width = 3;
+let grid = [[ 0 for _ in 0..width] for _ in 0..height];
+
+assert_eq(grid, [[0,0,0],[0,0,0],[0,0,0]]);
+
+// Also verify that a flat list comprehension with a guard (two iterations
+// plus a filter) isn't broken by the same class of slot collision.
+let found = [(y, x) for y in 0..height, x in 0..width, if y == x];
+assert_eq(found, [(0,0),(1,1),(2,2)]);
+
+print("ok");
+// expect-output: ok