From ea84e19c108c731e907a890b5bf063cc97c211ac Mon Sep 17 00:00:00 2001 From: Tim Fennis Date: Wed, 19 Aug 2026 08:51:43 +0200 Subject: [PATCH 1/5] =?UTF-8?q?fix(compiler):=20unify=20augmented=20assign?= =?UTF-8?q?ment=20lowering=20=F0=9F=94=81?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ndc_analyser/src/analyser.rs | 116 ++++++-- ndc_parser/src/expression.rs | 14 +- ndc_parser/src/lib.rs | 4 +- ndc_parser/src/parser.rs | 6 +- ndc_stdlib/src/list.rs | 2 +- ndc_stdlib/src/string.rs | 2 +- ndc_vm/src/compiler.rs | 269 +++++++++--------- tests/compiler/tests/compiler.rs | 50 ++++ .../004_basic/055_unified_op_assign.ndc | 62 ++++ ...incompatible_specialized_op_assignment.ndc | 3 + 10 files changed, 357 insertions(+), 171 deletions(-) create mode 100644 tests/functional/programs/004_basic/055_unified_op_assign.ndc create mode 100644 tests/functional/programs/004_basic/065_incompatible_specialized_op_assignment.ndc diff --git a/ndc_analyser/src/analyser.rs b/ndc_analyser/src/analyser.rs index 3f41ac16..49de2eaf 100644 --- a/ndc_analyser/src/analyser.rs +++ b/ndc_analyser/src/analyser.rs @@ -6,8 +6,8 @@ use itertools::{Itertools, izip}; use ndc_core::{StaticType, TypeSignature}; use ndc_lexer::Span; use ndc_parser::{ - Binding, Candidate, Expression, ExpressionLocation, ForBody, ForIteration, FunctionParameter, - Lvalue, NodeId, + AugmentedAssignmentPlan, Binding, Candidate, Expression, ExpressionLocation, ForBody, + ForIteration, FunctionParameter, Lvalue, NodeId, }; /// Side table holding semantic information keyed by AST node identity. @@ -195,18 +195,15 @@ impl Analyser { l_value, r_value, operation, - resolved_assign_operation, - resolved_operation, + plan, } => { let left_type = self.resolve_single_lvalue(l_value, *span)?; let right_type = self.analyse_or_any(r_value); - let arg_types = vec![left_type, right_type]; + let arg_types = vec![left_type.clone(), right_type.clone()]; - // Resolve both `op=` and `op` so we can widen the lvalue - // by the result type of whichever one actually fires. let ResolvedCall { binding: assign_binding, - .. + return_type: assign_return, } = self.scope_tree.resolve_call( &format!("{operation}="), &arg_types, @@ -218,24 +215,67 @@ impl Analyser { } = self .scope_tree .resolve_call(operation, &arg_types, CallKind::Operator); + let has_op_binding = !matches!(op_binding, Binding::None); + let assign_is_eligible = Self::augmented_rhs_is_compatible(&left_type, &right_type); - *resolved_assign_operation = assign_binding; - *resolved_operation = op_binding; + let writeback_type = match assign_binding { + Binding::Resolved(candidate) if assign_is_eligible => { + *plan = AugmentedAssignmentPlan::Resolved(Binding::Resolved(candidate)); + None + } + Binding::Dynamic(mut assign_candidates) if assign_is_eligible => { + match op_binding.clone() { + Binding::Resolved(candidate) => { + if !assign_candidates.contains(&candidate) { + assign_candidates.push(candidate); + } + } + Binding::Dynamic(op_candidates) => { + for candidate in op_candidates { + if !assign_candidates.contains(&candidate) { + assign_candidates.push(candidate); + } + } + } + Binding::None => {} + } - // Either form satisfies the call: `op=` mutates in place; - // `op` falls back through `a = a op b`. Only error when both - // are missing — e.g. `Map -= Map` is fine via `-=` even when - // `-` itself has no Map overload. - if matches!(resolved_assign_operation, Binding::None) - && matches!(resolved_operation, Binding::None) - { - self.emit(AnalysisError::function_not_found( - operation, &arg_types, *span, - )); - } + let result_type = if has_op_binding { + assign_return.lub(&op_return) + } else { + assign_return + }; + *plan = + AugmentedAssignmentPlan::Resolved(Binding::Dynamic(assign_candidates)); + Some(result_type) + } + Binding::Resolved(_) | Binding::Dynamic(_) => { + // A specialized mutation exists, but using it would + // change the concrete left type. Reject it here rather + // than falling through to an ordinary operation whose + // erased return type could widen the same target. + self.emit(AnalysisError::mismatched_types( + &right_type, + &left_type, + *span, + )); + *plan = AugmentedAssignmentPlan::Unresolved; + None + } + _ if has_op_binding => { + *plan = AugmentedAssignmentPlan::Resolved(op_binding); + Some(op_return) + } + _ => { + self.emit(AnalysisError::function_not_found( + operation, &arg_types, *span, + )); + *plan = AugmentedAssignmentPlan::Unresolved; + None + } + }; - if !matches!(resolved_operation, Binding::None) { - let result_type = op_return; + if let Some(result_type) = writeback_type { match l_value { Lvalue::Identifier { resolved: Some(target), @@ -640,7 +680,11 @@ impl Analyser { let type_of_index_target = self.analyse_or_any(value); let get_args = [type_of_index_target.clone(), index_type.clone()]; - let set_args = [type_of_index_target.clone(), index_type, StaticType::Any]; + let set_args = [ + type_of_index_target.clone(), + index_type.clone(), + StaticType::Any, + ]; // Indexing isn't operator-form for vec purposes: there's no // natural broadcast story for `(list_a, list_b)[i]`. @@ -655,6 +699,13 @@ impl Analyser { .binding, ); + let range_type = StaticType::Iterator(Box::new(StaticType::Int)); + if index_type.is_subtype(&range_type) + && let StaticType::List(element) = &type_of_index_target + { + return Ok(StaticType::List(element.clone())); + } + if let Some(t) = type_of_index_target.index_element_type() { Ok(t) } else { @@ -684,6 +735,23 @@ impl Analyser { } } + /// Specialized `op=` implementations preserve the concrete left type. + /// A tuple left-hand side represents vector dispatch, so a scalar right + /// operand must be compatible with every concrete tuple element. + fn augmented_rhs_is_compatible(left_type: &StaticType, right_type: &StaticType) -> bool { + match (left_type, right_type) { + (StaticType::Tuple(left), StaticType::Tuple(right)) => { + left.len() == right.len() + && right + .iter() + .zip(left) + .all(|(right, left)| right.is_subtype(left)) + } + (StaticType::Tuple(left), right) => left.iter().all(|left| right.is_subtype(left)), + (left, right) => right.is_subtype(left), + } + } + /// Resolve expressions as arguments to a function and return the function arity fn resolve_parameters_declarative( &mut self, diff --git a/ndc_parser/src/expression.rs b/ndc_parser/src/expression.rs index 912bd76e..8380d42b 100644 --- a/ndc_parser/src/expression.rs +++ b/ndc_parser/src/expression.rs @@ -26,6 +26,17 @@ pub enum Binding { Dynamic(Vec), // figure it out at runtime } +/// The operation selected by the analyser for an augmented assignment. +/// +/// Every selected operation returns the updated left value internally. The +/// compiler writes that value back through the assignment target, while the +/// augmented-assignment expression itself evaluates to unit. +#[derive(Debug, Eq, PartialEq, Clone)] +pub enum AugmentedAssignmentPlan { + Unresolved, + Resolved(Binding), +} + #[derive(Debug, Eq, PartialEq, Clone, Copy)] pub enum ResolvedVar { Local { slot: usize }, @@ -110,8 +121,7 @@ pub enum Expression { l_value: Lvalue, r_value: Box, operation: String, - resolved_assign_operation: Binding, - resolved_operation: Binding, + plan: AugmentedAssignmentPlan, }, FunctionDeclaration { name: Option, diff --git a/ndc_parser/src/lib.rs b/ndc_parser/src/lib.rs index 198b87f4..98cff6dc 100644 --- a/ndc_parser/src/lib.rs +++ b/ndc_parser/src/lib.rs @@ -3,8 +3,8 @@ mod operator; mod parser; pub use expression::{ - Binding, Candidate, CaptureSource, Expression, ExpressionLocation, ForBody, ForIteration, - FunctionParameter, Lvalue, NodeId, ResolvedVar, + AugmentedAssignmentPlan, Binding, Candidate, CaptureSource, Expression, ExpressionLocation, + ForBody, ForIteration, FunctionParameter, Lvalue, NodeId, ResolvedVar, }; pub use operator::{BinaryOperator, LogicalOperator, UnaryOperator}; pub use parser::Error; diff --git a/ndc_parser/src/parser.rs b/ndc_parser/src/parser.rs index 8a298f80..b7c05700 100644 --- a/ndc_parser/src/parser.rs +++ b/ndc_parser/src/parser.rs @@ -2,7 +2,8 @@ use std::fmt::Write; use crate::expression::Expression; use crate::expression::{ - Binding, ExpressionLocation, ForBody, ForIteration, FunctionParameter, Lvalue, NodeId, + AugmentedAssignmentPlan, Binding, ExpressionLocation, ForBody, ForIteration, FunctionParameter, + Lvalue, NodeId, }; use crate::operator::{BinaryOperator, LogicalOperator, UnaryOperator}; use ndc_core::{Parameter, StaticType, TypeSignature}; @@ -376,8 +377,7 @@ impl Parser { .expect("guaranteed to produce an lvalue"), r_value: Box::new(expression), operation: operation_identifier, - resolved_assign_operation: Binding::None, - resolved_operation: Binding::None, + plan: AugmentedAssignmentPlan::Unresolved, }; Ok(op_assign.to_location(start.merge(end))) diff --git a/ndc_stdlib/src/list.rs b/ndc_stdlib/src/list.rs index 0c45b0e2..f4be2b66 100644 --- a/ndc_stdlib/src/list.rs +++ b/ndc_stdlib/src/list.rs @@ -325,7 +325,7 @@ pub mod ops { StaticType::List(Box::new(StaticType::Any)), StaticType::List(Box::new(StaticType::Any)), ]), - return_type: Box::new(StaticType::Tuple(vec![])), + return_type: Box::new(StaticType::List(Box::new(StaticType::Any))), }, func: NativeFunc::Simple(Box::new(|args| { let [left, right] = args else { diff --git a/ndc_stdlib/src/string.rs b/ndc_stdlib/src/string.rs index 6e2c061c..dcf28f4c 100644 --- a/ndc_stdlib/src/string.rs +++ b/ndc_stdlib/src/string.rs @@ -36,7 +36,7 @@ mod inner { } /// Appends the right string to the left string in place. - #[function(name = "++=")] + #[function(name = "++=", return_type = String)] pub fn op_list_concat(left: &mut StringRepr, right: &mut StringRepr) -> Value { if Rc::ptr_eq(left, right) { let new = right.borrow().repeat(2).clone(); diff --git a/ndc_vm/src/compiler.rs b/ndc_vm/src/compiler.rs index dd85a17f..661b4bb5 100644 --- a/ndc_vm/src/compiler.rs +++ b/ndc_vm/src/compiler.rs @@ -4,8 +4,8 @@ use crate::{Object, Value}; use ndc_core::{StaticType, TypeSignature}; use ndc_lexer::Span; use ndc_parser::{ - Binding, Candidate, CaptureSource, Expression, ExpressionLocation, ForBody, ForIteration, - FunctionParameter, LogicalOperator, Lvalue, ResolvedVar, + AugmentedAssignmentPlan, Binding, Candidate, CaptureSource, Expression, ExpressionLocation, + ForBody, ForIteration, FunctionParameter, LogicalOperator, Lvalue, ResolvedVar, }; use std::rc::Rc; @@ -240,124 +240,24 @@ impl Compiler { Expression::OpAssignment { l_value, r_value, - resolved_assign_operation, - resolved_operation, + plan, .. } => { - match l_value { - Lvalue::Identifier { - resolved, - span: lv_span, - .. - } => { - let var = resolved.expect("lvalue must be resolved"); - match Self::op_assign_strategy(&resolved_assign_operation) { - OpAssignStrategy::InPlaceScalar => { - // `|=`, `&=`, `++=` over a List/Map/String: - // the in-place op mutates the value's Rc and - // returns unit (or the lhs). The slot already - // points at the shared Rc, so just discard. - self.compile_binding(resolved_assign_operation, span)?; - self.emit_get_var(var, lv_span); - self.compile_expr(*r_value)?; - self.ir.write(OpCode::Call(2), span); - self.ir.write(OpCode::Pop, span); - } - OpAssignStrategy::DynamicMerge => { - // `op=` exists but either dispatches to - // multiple candidates at runtime, or resolved - // to a single vec candidate that produces a - // fresh tuple. Either way the result must be - // stored back via SetVar — `op=` returns lhs - // when it mutates in place, or a fresh value - // when vec'd. - let (opcode, callee_binding): (OpCode, Binding) = - match resolved_assign_operation { - Binding::Resolved(Candidate::Vec(_)) => { - (OpCode::CallVec(2), resolved_assign_operation) - } - Binding::Dynamic(assign_candidates) => { - // Merge with `op` candidates so the runtime - // dispatcher can fall back to `a op b` shape - // for arg types `op=` doesn't accept. - let mut merged = assign_candidates; - match resolved_operation { - Binding::Dynamic(c) => merged.extend(c), - Binding::Resolved(c) => merged.push(c), - Binding::None => {} - } - (OpCode::Call(2), Binding::Dynamic(merged)) - } - _ => unreachable!( - "DynamicMerge fires only for Resolved(Vec) or Dynamic op=" - ), - }; - self.compile_binding(callee_binding, span)?; - self.emit_get_var(var, lv_span); - self.compile_expr(*r_value)?; - self.ir.write(opcode, span); - self.emit_set_var(var, lv_span); - } - OpAssignStrategy::FallbackToOp => { - // No `op=` overload: lower to `lhs = lhs op rhs`. - // Vec-resolved `op` (e.g. `a += (3, 4)` on - // `Tuple`) goes through `CallVec` - // for the speed-up; everything else is `Call`. - let opcode = Self::call_opcode_for(&resolved_operation, 2); - self.compile_binding(resolved_operation, span)?; - self.emit_get_var(var, lv_span); - self.compile_expr(*r_value)?; - self.ir.write(opcode, span); - self.emit_set_var(var, lv_span); - } - } + let target = PreparedAssignmentTarget::prepare(self, l_value, span)?; + let binding = match plan { + AugmentedAssignmentPlan::Resolved(binding) => binding, + AugmentedAssignmentPlan::Unresolved => { + return Err(CompileError::unresolved_binding(span)); } - Lvalue::Index { - value, - index, - resolved_get, - resolved_set, - } => { - let container_span = value.span; - let index_span = index.span; - - let tmp_container = self.num_locals; - let tmp_index = self.num_locals + 1; - self.num_locals += 2; - - self.compile_expr(*value)?; - self.ir - .write(OpCode::SetLocal(tmp_container), container_span); - self.compile_expr(*index)?; - self.ir.write(OpCode::SetLocal(tmp_index), index_span); - - self.compile_binding( - resolved_set.expect("[]= must be resolved"), - container_span.merge(index_span), - )?; - self.ir - .write(OpCode::GetLocal(tmp_container), container_span); - self.ir.write(OpCode::GetLocal(tmp_index), index_span); - - let op_opcode = Self::call_opcode_for(&resolved_operation, 2); - self.compile_binding(resolved_operation, span)?; - self.compile_binding( - resolved_get.expect("[] must be resolved"), - index_span, - )?; - self.ir - .write(OpCode::GetLocal(tmp_container), container_span); - self.ir.write(OpCode::GetLocal(tmp_index), index_span); - self.ir.write(OpCode::Call(2), span); // [](container, index) → current_value - self.compile_expr(*r_value)?; - self.ir.write(op_opcode, span); // op(current_value, r_value) → new_value - self.ir.write(OpCode::Call(3), span); // []=(container, index, new_value) - self.ir.write(OpCode::Pop, span); // discard []= result; common code below pushes unit - } - Lvalue::Sequence(_) => { - return Err(CompileError::lvalue_required_to_be_single_identifier(span)); - } - } + }; + + let opcode = Self::call_opcode_for(&binding, 2); + self.compile_binding(binding, span)?; + target.emit_read(self)?; + self.compile_expr(*r_value)?; + self.ir.write(opcode, span); + target.emit_store(self)?; + let idx = self.ir.add_constant(Value::unit()); self.ir.write(OpCode::Constant(idx), span); } @@ -981,31 +881,124 @@ struct LoopContext { break_instructions: Vec, } -/// Which lowering shape an `op=` site takes. -enum OpAssignStrategy { - /// A scalar `op=` overload exists and was resolved exactly. The op - /// mutates the value's Rc in place; the result is discarded. - InPlaceScalar, - /// `op=` resolved to multiple candidates. Merge with `op` candidates and - /// dispatch at runtime; store the result back. - DynamicMerge, - /// No `op=` overload — lower to `lhs = lhs op rhs`. - FallbackToOp, +/// An assignment location whose side-effecting components have already been +/// evaluated and cached. This lets augmented assignment use one lowering for +/// every target without evaluating an index expression more than once. +enum PreparedAssignmentTarget { + Variable { + variable: ResolvedVar, + span: Span, + }, + Index { + cached_container: usize, + cached_index: usize, + container_span: Span, + index_span: Span, + getter: Binding, + setter: Binding, + }, } -impl Compiler { - fn op_assign_strategy(op_assign: &Binding) -> OpAssignStrategy { - match op_assign { - Binding::Resolved(Candidate::Scalar(_)) => OpAssignStrategy::InPlaceScalar, - // A vec-resolved op= would produce a fresh tuple result that must - // be stored back; the merge path handles that correctly via - // SetVar after the call. (In practice the stdlib has no such - // overload, but this keeps the contract uniform.) - Binding::Resolved(Candidate::Vec(_)) | Binding::Dynamic(_) => { - OpAssignStrategy::DynamicMerge +impl PreparedAssignmentTarget { + fn prepare(compiler: &mut Compiler, l_value: Lvalue, span: Span) -> Result { + match l_value { + Lvalue::Identifier { resolved, span, .. } => Ok(Self::Variable { + variable: resolved.expect("lvalue must be resolved"), + span, + }), + Lvalue::Index { + value, + index, + resolved_get, + resolved_set, + } => { + let container_span = value.span; + let index_span = index.span; + let cached_container = compiler.num_locals; + let cached_index = compiler.num_locals + 1; + compiler.num_locals += 2; + + compiler.compile_expr(*value)?; + compiler + .ir + .write(OpCode::SetLocal(cached_container), container_span); + compiler.compile_expr(*index)?; + compiler + .ir + .write(OpCode::SetLocal(cached_index), index_span); + + Ok(Self::Index { + cached_container, + cached_index, + container_span, + index_span, + getter: resolved_get.expect("[] must be resolved"), + setter: resolved_set.expect("[]= must be resolved"), + }) + } + Lvalue::Sequence(_) => Err(CompileError::lvalue_required_to_be_single_identifier(span)), + } + } + + fn emit_read(&self, compiler: &mut Compiler) -> Result<(), CompileError> { + match self { + Self::Variable { variable, span } => compiler.emit_get_var(*variable, *span), + Self::Index { + cached_container, + cached_index, + container_span, + index_span, + getter, + .. + } => { + compiler.compile_binding(getter.clone(), *index_span)?; + compiler + .ir + .write(OpCode::GetLocal(*cached_container), *container_span); + compiler + .ir + .write(OpCode::GetLocal(*cached_index), *index_span); + compiler + .ir + .write(OpCode::Call(2), container_span.merge(*index_span)); } - Binding::None => OpAssignStrategy::FallbackToOp, } + Ok(()) + } + + fn emit_store(&self, compiler: &mut Compiler) -> Result<(), CompileError> { + match self { + Self::Variable { variable, span } => compiler.emit_set_var(*variable, *span), + Self::Index { + cached_container, + cached_index, + container_span, + index_span, + setter, + .. + } => { + let cached_value = compiler.num_locals; + compiler.num_locals += 1; + let target_span = container_span.merge(*index_span); + + compiler + .ir + .write(OpCode::SetLocal(cached_value), target_span); + compiler.compile_binding(setter.clone(), target_span)?; + compiler + .ir + .write(OpCode::GetLocal(*cached_container), *container_span); + compiler + .ir + .write(OpCode::GetLocal(*cached_index), *index_span); + compiler + .ir + .write(OpCode::GetLocal(cached_value), target_span); + compiler.ir.write(OpCode::Call(3), target_span); + compiler.ir.write(OpCode::Pop, target_span); + } + } + Ok(()) } } diff --git a/tests/compiler/tests/compiler.rs b/tests/compiler/tests/compiler.rs index 3426d22d..b959fb1b 100644 --- a/tests/compiler/tests/compiler.rs +++ b/tests/compiler/tests/compiler.rs @@ -42,6 +42,16 @@ fn compile_with_stdlib(input: &str) -> Vec { .to_vec() } +fn compile_with_stdlib_unoptimized(input: &str) -> Vec { + let mut interp = ndc_interpreter::Interpreter::capturing(); + interp.configure(ndc_stdlib::register); + interp + .compile_str_unoptimized(input) + .expect("compile failed") + .opcodes() + .to_vec() +} + // if true { 1 } // // 0: Constant(0) push `true` @@ -325,6 +335,46 @@ fn test_assignment() { ); } +#[test] +fn test_augmented_assignment_always_writes_back() { + let variable = compile_with_stdlib_unoptimized("let value = [1]; value ++= [2];"); + assert!( + variable + .windows(4) + .any(|ops| matches!(ops, [Call(2), SetLocal(0), Constant(_), Pop])), + "specialized variable augmentation must write back its result and push unit: {variable:?}", + ); + + let index = compile_with_stdlib_unoptimized( + "let value = [1]; let values = [value]; values[0] ++= [2];", + ); + assert!( + index + .windows(3) + .any(|ops| matches!(ops, [Call(3), Pop, Constant(_)])), + "specialized indexed augmentation must write back through []= and push unit: {index:?}", + ); +} + +#[test] +fn test_augmented_assignment_writeback_uses_target_store_shape() { + let variable = compile_with_stdlib_unoptimized("let value = 1; value += 2;"); + assert!( + variable + .windows(4) + .any(|ops| matches!(ops, [Call(2), SetLocal(0), Constant(_), Pop])), + "writeback variable augmentation must store the operation result and push unit: {variable:?}", + ); + + let index = compile_with_stdlib_unoptimized("let values = [1]; values[0] += 2;"); + assert!( + index + .windows(3) + .any(|ops| matches!(ops, [Call(3), Pop, Constant(_)])), + "writeback index augmentation must discard the setter result before pushing unit: {index:?}", + ); +} + // { let a = 3; a } // // Declaration stores 3 into pre-allocated slot 0. diff --git a/tests/functional/programs/004_basic/055_unified_op_assign.ndc b/tests/functional/programs/004_basic/055_unified_op_assign.ndc new file mode 100644 index 00000000..2c311607 --- /dev/null +++ b/tests/functional/programs/004_basic/055_unified_op_assign.ndc @@ -0,0 +1,62 @@ +// Exact indexed op= must mutate the value in place and preserve aliases. +let aliased = [1]; +let nested = [aliased]; +nested[0] ++= [2, 3]; +assert_eq(aliased, [1, 2, 3]); +assert_eq(nested[0], [1, 2, 3]); + +// Map -= Map exists without an ordinary Map - Map overload. Indexed +// augmentation must therefore use -= directly. +let set = %{1, 2, 3}; +let sets = [set]; +sets[0] -= %{2}; +assert_eq(set, %{1, 3}); +assert_eq(sets[0], %{1, 3}); + +// An Any-typed indexed value resolves dynamically. op= candidates have +// priority over ordinary op candidates, and their returned lhs is written back. +let dynamic_alias = [10]; +let dynamic_values: List = [dynamic_alias]; +dynamic_values[0] ++= [20]; +assert_eq(dynamic_alias, [10, 20]); +assert_eq(dynamic_values[0], [10, 20]); + +// Location components, the current-value read, and the rhs execute once in +// source order. The source expression evaluates to unit. +let events = []; +let ordered_values = [1]; +fn target() { + events ++= ["target"]; + ordered_values +} +fn location() { + events ++= ["index"]; + 0 +} +fn replacement() { + events ++= ["rhs"]; + 2 +} +assert_eq((target()[location()] += replacement()), ()); +assert_eq(ordered_values, [3]); +assert_eq(events, ["target", "index", "rhs"]); + +// Getters may return detached values. The updated result must still be stored +// through the target even when the specialized operator mutates that result. +let text = "ab"; +text[0] ++= "x"; +assert_eq(text, "axb"); + +let sliced = [1, 2]; +sliced[0..1] ++= [3]; +assert_eq(sliced, [1, 3, 2]); + +// Compatible specialized mutation preserves the fixed container type. Any on +// the target remains the explicit escape hatch for heterogeneous contents. +let typed_values: List = [1]; +typed_values ++= [2]; +assert_eq(typed_values, [1, 2]); + +let mixed_values: List = [1]; +mixed_values ++= ["two"]; +assert_eq(mixed_values, [1, "two"]); diff --git a/tests/functional/programs/004_basic/065_incompatible_specialized_op_assignment.ndc b/tests/functional/programs/004_basic/065_incompatible_specialized_op_assignment.ndc new file mode 100644 index 00000000..cd551592 --- /dev/null +++ b/tests/functional/programs/004_basic/065_incompatible_specialized_op_assignment.ndc @@ -0,0 +1,3 @@ +// expect-error: mismatched types: found List but expected List +let values = [1]; +values ++= ["two"]; From e7017e17b3252d465b4a4bede68b7b33b088db67 Mon Sep 17 00:00:00 2001 From: Tim Fennis Date: Wed, 19 Aug 2026 08:56:48 +0200 Subject: [PATCH 2/5] =?UTF-8?q?fix(analyser):=20validate=20indexed=20assig?= =?UTF-8?q?nment=20types=20=F0=9F=9B=A1=EF=B8=8F?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ndc_analyser/src/analyser.rs | 249 ++++++++++++++---- ndc_core/src/static_type.rs | 4 + .../056_annotated_list_index_assignment.ndc | 3 + .../057_annotated_map_index_assignment.ndc | 3 + ...058_annotated_list_index_op_assignment.ndc | 3 + .../059_annotated_map_index_op_assignment.ndc | 3 + .../060_inferred_index_assignment_widens.ndc | 7 + .../061_non_widenable_index_assignment.ndc | 3 + .../062_non_widenable_index_op_assignment.ndc | 3 + 9 files changed, 220 insertions(+), 58 deletions(-) create mode 100644 tests/functional/programs/004_basic/056_annotated_list_index_assignment.ndc create mode 100644 tests/functional/programs/004_basic/057_annotated_map_index_assignment.ndc create mode 100644 tests/functional/programs/004_basic/058_annotated_list_index_op_assignment.ndc create mode 100644 tests/functional/programs/004_basic/059_annotated_map_index_op_assignment.ndc create mode 100644 tests/functional/programs/004_basic/060_inferred_index_assignment_widens.ndc create mode 100644 tests/functional/programs/004_basic/061_non_widenable_index_assignment.ndc create mode 100644 tests/functional/programs/004_basic/062_non_widenable_index_op_assignment.ndc diff --git a/ndc_analyser/src/analyser.rs b/ndc_analyser/src/analyser.rs index 49de2eaf..b55941dc 100644 --- a/ndc_analyser/src/analyser.rs +++ b/ndc_analyser/src/analyser.rs @@ -169,25 +169,7 @@ impl Analyser { Expression::Assignment { l_value, r_value } => { let old_type = self.resolve_lvalue_or_any(l_value, *span); let new_type = self.analyse_or_any(r_value); - - if let Lvalue::Identifier { - resolved: Some(target), - .. - } = l_value - { - let widened = old_type.lub(&new_type); - if widened != old_type - && let Err(annotated_type) = - self.scope_tree.update_binding_type(*target, widened) - && !new_type.is_subtype(&annotated_type) - { - self.emit(AnalysisError::mismatched_types( - &new_type, - &annotated_type, - *span, - )); - } - } + self.validate_lvalue_write(l_value, &old_type, &new_type, *span); Ok(StaticType::unit()) } @@ -276,45 +258,7 @@ impl Analyser { }; if let Some(result_type) = writeback_type { - match l_value { - Lvalue::Identifier { - resolved: Some(target), - .. - } => { - let widened = arg_types[0].lub(&result_type); - if widened != arg_types[0] - && let Err(annotated_type) = - self.scope_tree.update_binding_type(*target, widened) - && !result_type.is_subtype(&annotated_type) - { - self.emit(AnalysisError::mismatched_types( - &result_type, - &annotated_type, - *span, - )); - } - } - Lvalue::Index { value, .. } => { - if let Expression::Identifier { - resolved: Binding::Resolved(Candidate::Scalar(target)), - .. - } = &value.expression - { - let container_type = self.scope_tree.get_type(*target).clone(); - if let Some(elem_type) = container_type.index_element_type() { - let widened_elem = elem_type.lub(&result_type); - if widened_elem != elem_type { - let new_container = - container_type.with_element_type(widened_elem); - let _ = self - .scope_tree - .update_binding_type(*target, new_container); - } - } - } - } - _ => {} - } + self.validate_lvalue_write(l_value, &left_type, &result_type, *span); } Ok(StaticType::unit()) @@ -752,6 +696,66 @@ impl Analyser { } } + /// Validate a value that will be stored through an lvalue, widening an + /// inferred binding when the location has a stable variable to update. + fn validate_lvalue_write( + &mut self, + lvalue: &Lvalue, + stored_type: &StaticType, + value_type: &StaticType, + span: Span, + ) { + match lvalue { + Lvalue::Identifier { + resolved: Some(target), + .. + } => { + let widened = stored_type.lub(value_type); + if widened != *stored_type + && let Err(annotated_type) = + self.scope_tree.update_binding_type(*target, widened) + && !value_type.is_subtype(&annotated_type) + { + self.emit(AnalysisError::mismatched_types( + value_type, + &annotated_type, + span, + )); + } + } + Lvalue::Index { value, .. } => { + if value_type.is_subtype(stored_type) { + return; + } + + if let Expression::Identifier { + resolved: Binding::Resolved(Candidate::Scalar(target)), + .. + } = &value.expression + { + let container_type = self.scope_tree.get_type(*target).clone(); + let widened_container = + container_type.with_element_type(stored_type.lub(value_type)); + if widened_container != container_type + && self + .scope_tree + .update_binding_type(*target, widened_container) + .is_ok() + { + return; + } + } + + self.emit(AnalysisError::mismatched_types( + value_type, + stored_type, + span, + )); + } + Lvalue::Identifier { resolved: None, .. } | Lvalue::Sequence(_) => {} + } + } + /// Resolve expressions as arguments to a function and return the function arity fn resolve_parameters_declarative( &mut self, @@ -996,3 +1000,132 @@ impl AnalysisError { } } } + +#[cfg(test)] +mod tests { + use super::*; + use ndc_lexer::{Lexer, SourceId}; + use ndc_parser::Parser; + + fn analyse_with_globals( + source: &str, + globals: Vec<(String, StaticType)>, + ) -> (StaticType, AnalysisResult) { + let tokens = Lexer::new(source, SourceId::SYNTHETIC) + .collect::, _>>() + .expect("lex failed"); + let mut expressions = Parser::from_tokens(tokens).parse().expect("parse failed"); + let mut analyser = Analyser::from_scope_tree(ScopeTree::from_global_scope(globals)); + let mut last_type = StaticType::unit(); + for expression in &mut expressions { + last_type = analyser.analyse(expression).expect("analysis failed"); + } + (last_type, analyser.take_result()) + } + + fn analyse_last_type_with_globals( + source: &str, + globals: Vec<(String, StaticType)>, + ) -> StaticType { + let (last_type, result) = analyse_with_globals(source, globals); + assert!( + result.errors.is_empty(), + "analysis errors: {:?}", + result.errors + ); + last_type + } + + fn analyse_last_type(source: &str) -> StaticType { + analyse_last_type_with_globals(source, vec![]) + } + + fn assert_analysis_error(source: &str, globals: Vec<(String, StaticType)>, expected: &str) { + let (_, result) = analyse_with_globals(source, globals); + assert!( + result + .errors + .iter() + .any(|error| error.to_string().contains(expected)), + "expected an error containing {expected:?}, got {:?}", + result.errors, + ); + } + + #[test] + fn inferred_list_index_assignment_widens_element_type() { + assert_eq!( + analyse_last_type("let values = [1]; values[0] = \"two\"; values"), + StaticType::List(Box::new(StaticType::Any)), + ); + } + + #[test] + fn inferred_map_index_assignment_widens_value_and_preserves_key_type() { + assert_eq!( + analyse_last_type("let values = %{\"one\": 1}; values[\"two\"] = \"two\"; values"), + StaticType::Map { + key: Box::new(StaticType::String), + value: Box::new(StaticType::Any), + }, + ); + } + + #[test] + fn inferred_index_augmented_assignment_widens_element_type() { + let add = StaticType::Function { + parameters: Some(vec![StaticType::Int, StaticType::Float]), + return_type: Box::new(StaticType::Number), + }; + assert_eq!( + analyse_last_type_with_globals( + "let values = [1]; values[0] += 0.5; values", + vec![("+".to_string(), add)], + ), + StaticType::List(Box::new(StaticType::Number)), + ); + } + + #[test] + fn compatible_specialized_assignment_preserves_left_type() { + let list_any = StaticType::List(Box::new(StaticType::Any)); + let append = StaticType::Function { + parameters: Some(vec![list_any.clone(), list_any.clone()]), + return_type: Box::new(list_any), + }; + + assert_eq!( + analyse_last_type_with_globals( + "let values = [1]; values ++= [2]; values", + vec![("++=".to_string(), append)], + ), + StaticType::List(Box::new(StaticType::Int)), + ); + } + + #[test] + fn incompatible_specialized_assignment_is_rejected() { + let list_any = StaticType::List(Box::new(StaticType::Any)); + let concat = StaticType::Function { + parameters: Some(vec![list_any.clone(), list_any.clone()]), + return_type: Box::new(list_any), + }; + + assert_analysis_error( + "let values = [1]; values ++= [\"two\"];", + vec![ + ("++=".to_string(), concat.clone()), + ("++".to_string(), concat.clone()), + ], + "mismatched types: found List but expected List", + ); + assert_analysis_error( + "let values: List = [1]; values ++= [\"two\"];", + vec![ + ("++=".to_string(), concat.clone()), + ("++".to_string(), concat), + ], + "mismatched types: found List but expected List", + ); + } +} diff --git a/ndc_core/src/static_type.rs b/ndc_core/src/static_type.rs index 8fc82ab2..b9308062 100644 --- a/ndc_core/src/static_type.rs +++ b/ndc_core/src/static_type.rs @@ -584,6 +584,10 @@ impl StaticType { Self::MinHeap(_) => Self::MinHeap(Box::new(new_elem)), Self::MaxHeap(_) => Self::MaxHeap(Box::new(new_elem)), Self::Deque(_) => Self::Deque(Box::new(new_elem)), + Self::Map { key, .. } => Self::Map { + key: key.clone(), + value: Box::new(new_elem), + }, _ => self.clone(), } } diff --git a/tests/functional/programs/004_basic/056_annotated_list_index_assignment.ndc b/tests/functional/programs/004_basic/056_annotated_list_index_assignment.ndc new file mode 100644 index 00000000..915fcd7a --- /dev/null +++ b/tests/functional/programs/004_basic/056_annotated_list_index_assignment.ndc @@ -0,0 +1,3 @@ +// expect-error: mismatched types: found String but expected Int +let values: List = [1]; +values[0] = "two"; diff --git a/tests/functional/programs/004_basic/057_annotated_map_index_assignment.ndc b/tests/functional/programs/004_basic/057_annotated_map_index_assignment.ndc new file mode 100644 index 00000000..8051cbf4 --- /dev/null +++ b/tests/functional/programs/004_basic/057_annotated_map_index_assignment.ndc @@ -0,0 +1,3 @@ +// expect-error: mismatched types: found String but expected Int +let values: Map = %{"one": 1}; +values["one"] = "two"; diff --git a/tests/functional/programs/004_basic/058_annotated_list_index_op_assignment.ndc b/tests/functional/programs/004_basic/058_annotated_list_index_op_assignment.ndc new file mode 100644 index 00000000..c0bf7d58 --- /dev/null +++ b/tests/functional/programs/004_basic/058_annotated_list_index_op_assignment.ndc @@ -0,0 +1,3 @@ +// expect-error: mismatched types: found Number but expected Int +let values: List = [1]; +values[0] += 0.5; diff --git a/tests/functional/programs/004_basic/059_annotated_map_index_op_assignment.ndc b/tests/functional/programs/004_basic/059_annotated_map_index_op_assignment.ndc new file mode 100644 index 00000000..26554cd8 --- /dev/null +++ b/tests/functional/programs/004_basic/059_annotated_map_index_op_assignment.ndc @@ -0,0 +1,3 @@ +// expect-error: mismatched types: found Number but expected Int +let values: Map = %{"one": 1}; +values["one"] += 0.5; diff --git a/tests/functional/programs/004_basic/060_inferred_index_assignment_widens.ndc b/tests/functional/programs/004_basic/060_inferred_index_assignment_widens.ndc new file mode 100644 index 00000000..164c0be4 --- /dev/null +++ b/tests/functional/programs/004_basic/060_inferred_index_assignment_widens.ndc @@ -0,0 +1,7 @@ +let values = [1]; +values[0] = "two"; +assert_eq(values[0], "two"); + +let mapped = %{"one": 1}; +mapped["two"] = "two"; +assert_eq(mapped["two"], "two"); diff --git a/tests/functional/programs/004_basic/061_non_widenable_index_assignment.ndc b/tests/functional/programs/004_basic/061_non_widenable_index_assignment.ndc new file mode 100644 index 00000000..595a6b76 --- /dev/null +++ b/tests/functional/programs/004_basic/061_non_widenable_index_assignment.ndc @@ -0,0 +1,3 @@ +// expect-error: mismatched types: found String but expected Int +fn values() -> List => [1]; +values()[0] = "two"; diff --git a/tests/functional/programs/004_basic/062_non_widenable_index_op_assignment.ndc b/tests/functional/programs/004_basic/062_non_widenable_index_op_assignment.ndc new file mode 100644 index 00000000..df42dd15 --- /dev/null +++ b/tests/functional/programs/004_basic/062_non_widenable_index_op_assignment.ndc @@ -0,0 +1,3 @@ +// expect-error: mismatched types: found Number but expected Int +fn values() -> List => [1]; +values()[0] += 0.5; From 8b3b22572d98099b599ee8713582936d0fe2a7c8 Mon Sep 17 00:00:00 2001 From: Tim Fennis Date: Wed, 19 Aug 2026 09:56:12 +0200 Subject: [PATCH 3/5] =?UTF-8?q?fix(compiler):=20keep=20temporaries=20above?= =?UTF-8?q?=20source=20locals=20=F0=9F=A7=B1?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ndc_analyser/src/analyser.rs | 15 +- ndc_analyser/src/scope.rs | 12 - ndc_parser/src/expression.rs | 2 - ndc_parser/src/parser.rs | 2 - ndc_vm/src/compiler.rs | 279 +++++++++++++++--- tests/compiler/tests/compiler.rs | 17 ++ .../004_basic/055_unified_op_assign.ndc | 28 ++ 7 files changed, 286 insertions(+), 69 deletions(-) diff --git a/ndc_analyser/src/analyser.rs b/ndc_analyser/src/analyser.rs index b55941dc..4232d064 100644 --- a/ndc_analyser/src/analyser.rs +++ b/ndc_analyser/src/analyser.rs @@ -538,25 +538,12 @@ impl Analyser { self.analyse_or_any(block); StaticType::unit() } - 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_or_any(expr))) - } + ForBody::List { expr } => StaticType::List(Box::new(self.analyse_or_any(expr))), ForBody::Map { key, value, default, - accumulator_slot, - .. } => { - *accumulator_slot = Some(self.scope_tree.reserve_anonymous_slot()); let key_type = self.analyse_or_any(key); let value_type = if let Some(value) = value { self.analyse_or_any(value) diff --git a/ndc_analyser/src/scope.rs b/ndc_analyser/src/scope.rs index 288fd585..6c2ec46e 100644 --- a/ndc_analyser/src/scope.rs +++ b/ndc_analyser/src/scope.rs @@ -868,18 +868,6 @@ impl ScopeTree { self.scopes[self.current_scope_idx].has_function_with_arity(name, arity) } - /// 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. - /// - /// Uses `"\x00"` as a sentinel name that can never collide with user identifiers - /// since the lexer never produces null bytes. - pub(crate) fn reserve_anonymous_slot(&mut self) -> usize { - self.scopes[self.current_scope_idx] - .allocate("\x00".to_string(), TypeBinding::Inferred(StaticType::Any)) - } - /// Try to update a binding's type. Returns `Err` with the annotated type /// if the binding has an explicit type annotation and cannot be widened. pub(crate) fn update_binding_type( diff --git a/ndc_parser/src/expression.rs b/ndc_parser/src/expression.rs index 8380d42b..30ff2f9b 100644 --- a/ndc_parser/src/expression.rs +++ b/ndc_parser/src/expression.rs @@ -202,13 +202,11 @@ pub enum ForBody { Block(ExpressionLocation), List { expr: ExpressionLocation, - accumulator_slot: Option, }, Map { key: ExpressionLocation, value: Option, default: Option>, - accumulator_slot: Option, }, } diff --git a/ndc_parser/src/parser.rs b/ndc_parser/src/parser.rs index b7c05700..13d3d492 100644 --- a/ndc_parser/src/parser.rs +++ b/ndc_parser/src/parser.rs @@ -850,7 +850,6 @@ impl Parser { Some(Token::For) => { let result = ForBody::List { expr: expr.simplify(), - accumulator_slot: None, }; self.for_comprehension(left_square_bracket_span, result, &Token::RightSquareBracket) } @@ -1321,7 +1320,6 @@ impl Parser { key: key_expr, value: value_expr, default, - accumulator_slot: None, }, &Token::RightCurlyBracket, ); diff --git a/ndc_vm/src/compiler.rs b/ndc_vm/src/compiler.rs index 661b4bb5..159a5475 100644 --- a/ndc_vm/src/compiler.rs +++ b/ndc_vm/src/compiler.rs @@ -35,9 +35,7 @@ impl Compiler { expressions: impl Iterator, ) -> Result { let mut compiler = Self::default(); - for expr_loc in expressions { - compiler.compile_expr(expr_loc)?; - } + compiler.compile_batch(expressions.collect())?; Ok(compiler.finish()?.0) } @@ -51,9 +49,7 @@ impl Compiler { optimize: false, ..Default::default() }; - for expr_loc in expressions { - compiler.compile_expr(expr_loc)?; - } + compiler.compile_batch(expressions.collect())?; Ok(compiler.finish()?.0) } @@ -72,9 +68,7 @@ impl Compiler { optimize: false, ..Default::default() }; - for expr_loc in expressions { - compiler.compile_expr(expr_loc)?; - } + compiler.compile_batch(expressions.collect())?; compiler.finish() } @@ -90,12 +84,28 @@ impl Compiler { new_expressions: impl Iterator, ) -> Result<(CompiledFunction, Self), CompileError> { let mut compiler = self; // checkpoint has no trailing Halt - for expr_loc in new_expressions { - compiler.compile_expr(expr_loc)?; - } + compiler.compile_batch(new_expressions.collect())?; compiler.finish() } + /// Reserve every analyser-assigned source slot before allocating hidden + /// compiler temporaries. The analyser resolves an entire function before + /// bytecode emission, so source declarations compiled later may otherwise + /// collide with a temporary allocated earlier. + fn compile_batch(&mut self, expressions: Vec) -> Result<(), CompileError> { + self.num_locals = self.num_locals.max(source_local_count(&expressions)); + for expression in expressions { + self.compile_expr(expression)?; + } + Ok(()) + } + + fn allocate_temp(&mut self) -> usize { + let slot = self.num_locals; + self.num_locals += 1; + slot + } + /// The instruction index where the trailing `Halt` was written. /// When resuming, this is the `ip` to start from in the new function. pub fn halt_ip(&self) -> usize { @@ -424,8 +434,7 @@ impl Compiler { // 4. Call []= function // 5. Pop the return value - let tmp_value = self.num_locals; - self.num_locals += 1; + let tmp_value = self.allocate_temp(); self.ir.write(OpCode::SetLocal(tmp_value), span); self.compile_binding(resolved_set.expect("[]= must be resolved"), span)?; @@ -659,8 +668,9 @@ impl Compiler { }, return_type: Box::new(return_type.clone()), }; + let function_source_locals = source_local_count(std::slice::from_ref(&body)); let mut fn_compiler = Self { - num_locals: num_params, + num_locals: num_params.max(function_source_locals), allow_return: true, optimize: self.optimize, ..Default::default() @@ -732,13 +742,8 @@ impl Compiler { })?; Ok(()) } - ForBody::List { - expr, - accumulator_slot, - } => { - let tmp_list = accumulator_slot - .ok_or_else(|| CompileError::unresolved_accumulator_slot(span))?; - self.num_locals = self.num_locals.max(tmp_list + 1); + ForBody::List { expr } => { + let tmp_list = self.allocate_temp(); self.ir.write(OpCode::MakeList(0), span); self.ir.write(OpCode::SetLocal(tmp_list), span); self.compile_for_iterations(iterations, span, &mut |this| { @@ -753,11 +758,8 @@ impl Compiler { key, value, default, - accumulator_slot, } => { - let tmp_map = accumulator_slot - .ok_or_else(|| CompileError::unresolved_accumulator_slot(span))?; - self.num_locals = self.num_locals.max(tmp_map + 1); + let tmp_map = self.allocate_temp(); let has_default = default.is_some(); if let Some(default) = default { self.compile_expr(*default)?; @@ -914,9 +916,8 @@ impl PreparedAssignmentTarget { } => { let container_span = value.span; let index_span = index.span; - let cached_container = compiler.num_locals; - let cached_index = compiler.num_locals + 1; - compiler.num_locals += 2; + let cached_container = compiler.allocate_temp(); + let cached_index = compiler.allocate_temp(); compiler.compile_expr(*value)?; compiler @@ -977,8 +978,7 @@ impl PreparedAssignmentTarget { setter, .. } => { - let cached_value = compiler.num_locals; - compiler.num_locals += 1; + let cached_value = compiler.allocate_temp(); let target_span = container_span.merge(*index_span); compiler @@ -1002,6 +1002,215 @@ impl PreparedAssignmentTarget { } } +/// Number of analyser-assigned local slots in the current function. +/// +/// Nested function bodies use their own slot namespace and are deliberately +/// skipped; their compiler performs the same scan when that body is lowered. +fn source_local_count(expressions: &[ExpressionLocation]) -> usize { + expressions + .iter() + .filter_map(max_source_local_in_expression) + .max() + .map_or(0, |slot| slot + 1) +} + +fn max_source_local_in_expression(location: &ExpressionLocation) -> Option { + let expression = &location.expression; + match expression { + Expression::BoolLiteral(_) + | Expression::StringLiteral(_) + | Expression::Int64Literal(_) + | Expression::Float64Literal(_) + | Expression::BigIntLiteral(_) + | Expression::ComplexLiteral(_) + | Expression::Break + | Expression::Continue => None, + Expression::Identifier { resolved, .. } => max_source_local_in_binding(resolved), + Expression::Statement(inner) | Expression::Grouping(inner) => { + max_source_local_in_expression(inner) + } + Expression::Logical { left, right, .. } => [ + max_source_local_in_expression(left), + max_source_local_in_expression(right), + ] + .into_iter() + .flatten() + .max(), + Expression::VariableDeclaration { l_value, value, .. } + | Expression::Assignment { + l_value, + r_value: value, + } => [ + max_source_local_in_lvalue(l_value), + max_source_local_in_expression(value), + ] + .into_iter() + .flatten() + .max(), + Expression::OpAssignment { + l_value, + r_value, + plan, + .. + } => { + let operation = match plan { + AugmentedAssignmentPlan::Unresolved => None, + AugmentedAssignmentPlan::Resolved(binding) => max_source_local_in_binding(binding), + }; + [ + max_source_local_in_lvalue(l_value), + max_source_local_in_expression(r_value), + operation, + ] + .into_iter() + .flatten() + .max() + } + Expression::FunctionDeclaration { + resolved_name, + captures, + .. + } => resolved_name + .iter() + .filter_map(max_source_local_in_var) + .chain(captures.iter().filter_map(|capture| match capture { + CaptureSource::Local(slot) => Some(*slot), + CaptureSource::Upvalue(_) => None, + })) + .max(), + Expression::Block { statements } => statements + .iter() + .filter_map(max_source_local_in_expression) + .max(), + Expression::If { + condition, + on_true, + on_false, + } => [ + max_source_local_in_expression(condition), + max_source_local_in_expression(on_true), + on_false.as_deref().and_then(max_source_local_in_expression), + ] + .into_iter() + .flatten() + .max(), + Expression::While { + expression, + loop_body, + } => [ + max_source_local_in_expression(expression), + max_source_local_in_expression(loop_body), + ] + .into_iter() + .flatten() + .max(), + Expression::For { iterations, body } => iterations + .iter() + .filter_map(|iteration| match iteration { + ForIteration::Iteration { l_value, sequence } => [ + max_source_local_in_lvalue(l_value), + max_source_local_in_expression(sequence), + ] + .into_iter() + .flatten() + .max(), + ForIteration::Guard(guard) => max_source_local_in_expression(guard), + }) + .chain(max_source_local_in_for_body(body)) + .max(), + Expression::Call { + function, + arguments, + } + | Expression::OperatorCall { + function, + arguments, + } => std::iter::once(max_source_local_in_expression(function)) + .chain(arguments.iter().map(max_source_local_in_expression)) + .flatten() + .max(), + Expression::Tuple { values } | Expression::List { values } => values + .iter() + .filter_map(max_source_local_in_expression) + .max(), + Expression::Map { values, default } => values + .iter() + .flat_map(|(key, value)| { + std::iter::once(max_source_local_in_expression(key)) + .chain(value.as_ref().map(max_source_local_in_expression)) + }) + .chain(default.as_deref().map(max_source_local_in_expression)) + .flatten() + .max(), + Expression::Return { value } => max_source_local_in_expression(value), + Expression::RangeInclusive { start, end } | Expression::RangeExclusive { start, end } => { + start + .as_deref() + .map(max_source_local_in_expression) + .into_iter() + .chain(end.as_deref().map(max_source_local_in_expression)) + .flatten() + .max() + } + } +} + +fn max_source_local_in_for_body(body: &ForBody) -> Option { + match body { + ForBody::Block(block) | ForBody::List { expr: block } => { + max_source_local_in_expression(block) + } + ForBody::Map { + key, + value, + default, + } => std::iter::once(max_source_local_in_expression(key)) + .chain(value.as_ref().map(max_source_local_in_expression)) + .chain(default.as_deref().map(max_source_local_in_expression)) + .flatten() + .max(), + } +} + +fn max_source_local_in_lvalue(lvalue: &Lvalue) -> Option { + match lvalue { + Lvalue::Identifier { resolved, .. } => resolved.as_ref().and_then(max_source_local_in_var), + Lvalue::Index { + value, + index, + resolved_set, + resolved_get, + } => [ + max_source_local_in_expression(value), + max_source_local_in_expression(index), + resolved_set.as_ref().and_then(max_source_local_in_binding), + resolved_get.as_ref().and_then(max_source_local_in_binding), + ] + .into_iter() + .flatten() + .max(), + Lvalue::Sequence(lvalues) => lvalues.iter().filter_map(max_source_local_in_lvalue).max(), + } +} + +fn max_source_local_in_binding(binding: &Binding) -> Option { + match binding { + Binding::None => None, + Binding::Resolved(candidate) => max_source_local_in_var(&candidate.var()), + Binding::Dynamic(candidates) => candidates + .iter() + .filter_map(|candidate| max_source_local_in_var(&candidate.var())) + .max(), + } +} + +fn max_source_local_in_var(variable: &ResolvedVar) -> Option { + match variable { + ResolvedVar::Local { slot } => Some(*slot), + ResolvedVar::Upvalue { .. } | ResolvedVar::Global { .. } => None, + } +} + /// Returns the minimum local slot referenced by an lvalue, used to determine /// which upvalues to close at the end of a loop iteration. fn min_lvalue_slot(lv: &Lvalue) -> Option { @@ -1076,14 +1285,6 @@ impl CompileError { } } - fn unresolved_accumulator_slot(span: Span) -> Self { - Self { - text: "accumulator slot was not assigned by the analyser; this is an internal error" - .to_string(), - span, - } - } - pub fn span(&self) -> Span { self.span } diff --git a/tests/compiler/tests/compiler.rs b/tests/compiler/tests/compiler.rs index b959fb1b..ef39c0f5 100644 --- a/tests/compiler/tests/compiler.rs +++ b/tests/compiler/tests/compiler.rs @@ -375,6 +375,23 @@ fn test_augmented_assignment_writeback_uses_target_store_shape() { ); } +#[test] +fn test_augmented_assignment_temporaries_follow_source_locals() { + let ops = + compile_with_stdlib_unoptimized("let values = [1]; values[0] += { let delta = 2; delta };"); + + assert!( + ops.iter().any(|op| matches!(op, SetLocal(1))), + "the rhs block local should retain analyser-assigned slot 1: {ops:?}", + ); + assert!( + ops.iter().any(|op| matches!(op, SetLocal(2))) + && ops.iter().any(|op| matches!(op, SetLocal(3))) + && ops.iter().any(|op| matches!(op, SetLocal(4))), + "prepared-target temporaries must be allocated after both source locals: {ops:?}", + ); +} + // { let a = 3; a } // // Declaration stores 3 into pre-allocated slot 0. diff --git a/tests/functional/programs/004_basic/055_unified_op_assign.ndc b/tests/functional/programs/004_basic/055_unified_op_assign.ndc index 2c311607..2933e52c 100644 --- a/tests/functional/programs/004_basic/055_unified_op_assign.ndc +++ b/tests/functional/programs/004_basic/055_unified_op_assign.ndc @@ -60,3 +60,31 @@ assert_eq(typed_values, [1, 2]); let mixed_values: List = [1]; mixed_values ++= ["two"]; assert_eq(mixed_values, [1, "two"]); + +// Compiler temporaries must live beyond every analyser-assigned local whose +// lifetime overlaps them. Block-local slots are deliberately reused across +// the target, index, and rhs expressions here. +let block_values = [1]; +({ + let target_alias = block_values; + target_alias +})[{ + let block_index = 0; + block_index +}] += { + let delta = 2; + delta +}; +assert_eq(block_values, [3]); + +// Nested functions have an independent local-slot namespace and need their +// own source-local high-water mark before compiler temporaries are allocated. +fn update_in_function() { + let values = [1]; + values[0] += { + let delta = 2; + delta + }; + values +} +assert_eq(update_in_function(), [3]); From e0ee34f18b2f198a16524bec61f478f94955c186 Mon Sep 17 00:00:00 2001 From: Tim Fennis Date: Thu, 20 Aug 2026 17:10:44 +0200 Subject: [PATCH 4/5] fix: fixed pr comments --- ndc_analyser/src/analyser.rs | 42 ++++++++++++++++-- tests/compiler/tests/compiler.rs | 43 +++++++++++++++++++ .../060_inferred_index_assignment_widens.ndc | 8 +++- 3 files changed, 88 insertions(+), 5 deletions(-) diff --git a/ndc_analyser/src/analyser.rs b/ndc_analyser/src/analyser.rs index 4232d064..1496a8f4 100644 --- a/ndc_analyser/src/analyser.rs +++ b/ndc_analyser/src/analyser.rs @@ -710,19 +710,47 @@ impl Analyser { )); } } - Lvalue::Index { value, .. } => { + Lvalue::Index { value, index, .. } => { if value_type.is_subtype(stored_type) { return; } + let range_type = StaticType::Iterator(Box::new(StaticType::Int)); + let is_slice = self + .result + .expr_types + .get(&index.id) + .is_some_and(|index_type| index_type.is_subtype(&range_type)); + let (stored_element_type, value_element_type) = if is_slice { + let Some(stored_element_type) = stored_type.index_element_type() else { + self.emit(AnalysisError::mismatched_types( + value_type, + stored_type, + span, + )); + return; + }; + let Some(value_element_type) = value_type.sequence_element_type() else { + self.emit(AnalysisError::mismatched_types( + value_type, + stored_type, + span, + )); + return; + }; + (stored_element_type, value_element_type) + } else { + (stored_type.clone(), value_type.clone()) + }; + if let Expression::Identifier { resolved: Binding::Resolved(Candidate::Scalar(target)), .. } = &value.expression { let container_type = self.scope_tree.get_type(*target).clone(); - let widened_container = - container_type.with_element_type(stored_type.lub(value_type)); + let widened_container = container_type + .with_element_type(stored_element_type.lub(&value_element_type)); if widened_container != container_type && self .scope_tree @@ -1047,6 +1075,14 @@ mod tests { ); } + #[test] + fn inferred_list_slice_assignment_widens_element_type() { + assert_eq!( + analyse_last_type("let values = [1]; values[0..1] = [\"two\"]; values"), + StaticType::List(Box::new(StaticType::Any)), + ); + } + #[test] fn inferred_map_index_assignment_widens_value_and_preserves_key_type() { assert_eq!( diff --git a/tests/compiler/tests/compiler.rs b/tests/compiler/tests/compiler.rs index ef39c0f5..d10097b6 100644 --- a/tests/compiler/tests/compiler.rs +++ b/tests/compiler/tests/compiler.rs @@ -335,6 +335,20 @@ fn test_assignment() { ); } +// let value = [1]; +// value ++= [2]; +// +// A specialized `++=` mutates and returns the left value. Even though the +// mutation is visible through aliases, SetLocal must write the returned value +// back before the compiler pushes unit for the assignment expression: +// +// Call(2), SetLocal(0), Constant(_), Pop +// +// The indexed form prepares the container and index in temporary locals, +// calls `++=`, then passes its result to `[]=`. The setter's unit result is +// discarded before the assignment's own unit is pushed: +// +// Call(3), Pop, Constant(_) #[test] fn test_augmented_assignment_always_writes_back() { let variable = compile_with_stdlib_unoptimized("let value = [1]; value ++= [2];"); @@ -356,6 +370,21 @@ fn test_augmented_assignment_always_writes_back() { ); } +// let value = 1; +// value += 2; +// +// Ordinary `+` produces a replacement value, so variable augmentation stores +// the result and then produces unit: +// +// Call(2), SetLocal(0), Constant(_), Pop +// +// let values = [1]; +// values[0] += 2; +// +// Indexed augmentation instead sends the replacement through `[]=`. Its unit +// result is popped before the assignment expression's unit is produced: +// +// Call(3), Pop, Constant(_) #[test] fn test_augmented_assignment_writeback_uses_target_store_shape() { let variable = compile_with_stdlib_unoptimized("let value = 1; value += 2;"); @@ -375,6 +404,20 @@ fn test_augmented_assignment_writeback_uses_target_store_shape() { ); } +// let values = [1]; +// values[0] += { let delta = 2; delta }; +// +// Source locals are assigned first: +// +// slot 0: values +// slot 1: delta +// +// Preparing an indexed assignment then reserves non-overlapping compiler +// temporaries after the source-local high-water mark: +// +// slot 2: cached container +// slot 3: cached index +// slot 4: operation result passed to `[]=` #[test] fn test_augmented_assignment_temporaries_follow_source_locals() { let ops = diff --git a/tests/functional/programs/004_basic/060_inferred_index_assignment_widens.ndc b/tests/functional/programs/004_basic/060_inferred_index_assignment_widens.ndc index 164c0be4..121916e2 100644 --- a/tests/functional/programs/004_basic/060_inferred_index_assignment_widens.ndc +++ b/tests/functional/programs/004_basic/060_inferred_index_assignment_widens.ndc @@ -1,7 +1,11 @@ let values = [1]; values[0] = "two"; -assert_eq(values[0], "two"); +// The concatenation requires values[0] to have widened from Int to Any so +// that operator dispatch is deferred until runtime. +assert_eq(values[0] ++ "!", "two!"); let mapped = %{"one": 1}; mapped["two"] = "two"; -assert_eq(mapped["two"], "two"); +// Map value inference must widen for the same reason while preserving the +// String key type. +assert_eq(mapped["two"] ++ "!", "two!"); From 3da4d2ea279de8f4245f5cda2280955a2aa6469a Mon Sep 17 00:00:00 2001 From: Tim Fennis Date: Thu, 20 Aug 2026 17:40:15 +0200 Subject: [PATCH 5/5] fix: keep track of num locals in analyser (for now) --- ndc_analyser/src/analyser.rs | 77 ++++++++- ndc_analyser/src/scope.rs | 25 ++- ndc_interpreter/src/lib.rs | 32 ++-- ndc_parser/src/expression.rs | 18 ++ ndc_parser/src/lib.rs | 2 +- ndc_vm/src/compiler.rs | 269 +++++------------------------- tests/compiler/tests/compiler.rs | 21 ++- tests/compiler/tests/optimizer.rs | 4 +- tests/functional/tests/repl.rs | 12 ++ tests/proptest/tests/panic.rs | 2 +- 10 files changed, 216 insertions(+), 246 deletions(-) diff --git a/ndc_analyser/src/analyser.rs b/ndc_analyser/src/analyser.rs index 1496a8f4..c6387fa8 100644 --- a/ndc_analyser/src/analyser.rs +++ b/ndc_analyser/src/analyser.rs @@ -7,7 +7,7 @@ use ndc_core::{StaticType, TypeSignature}; use ndc_lexer::Span; use ndc_parser::{ AugmentedAssignmentPlan, Binding, Candidate, Expression, ExpressionLocation, ForBody, - ForIteration, FunctionParameter, Lvalue, NodeId, + ForIteration, FunctionParameter, Lvalue, NodeId, SourceLocalCounts, }; /// Side table holding semantic information keyed by AST node identity. @@ -19,6 +19,9 @@ pub struct AnalysisResult { /// Inferred return types for functions without explicit annotations. /// Keyed by the FunctionDeclaration's `NodeId`. pub inferred_return_types: HashMap, + /// Analyser-assigned source-local high-water marks consumed by the compiler + /// before it allocates hidden temporaries. + pub source_local_counts: SourceLocalCounts, /// Errors accumulated during analysis. Non-empty when the analyser /// encountered problems but was able to continue with fallback types. pub errors: Vec, @@ -86,6 +89,13 @@ impl Analyser { ) -> Result { let typ = self.analyse_inner(expr_loc)?; self.result.expr_types.insert(expr_loc.id, typ.clone()); + if self.scope_tree.current_function_is_top_level() { + self.result.source_local_counts.top_level = self + .result + .source_local_counts + .top_level + .max(self.scope_tree.current_function_local_count()); + } Ok(typ) } @@ -313,6 +323,10 @@ impl Analyser { let implicit_return = self.analyse_or_any(body); let explicit_return = self.return_type_stack.pop().unwrap(); *captures = self.scope_tree.current_scope_captures(); + self.result + .source_local_counts + .functions + .insert(*id, self.scope_tree.current_function_local_count()); self.scope_tree.destroy_scope(); // Combine explicit `return` types with the block's implicit return type. @@ -1075,6 +1089,67 @@ mod tests { ); } + #[test] + fn source_local_counts_are_recorded_per_function_frame() { + let (_, result) = analyse_with_globals( + r#" + let top = 0; + { let scoped = 1; scoped }; + fn outer(arg) { + let local = arg; + fn inner(inner_arg) { + let nested = inner_arg; + nested + }; + local + }; + "#, + vec![], + ); + + assert!( + result.errors.is_empty(), + "analysis errors: {:?}", + result.errors + ); + assert_eq!(result.source_local_counts.top_level, 2); + let mut function_counts: Vec<_> = result + .source_local_counts + .functions + .values() + .copied() + .collect(); + function_counts.sort_unstable(); + assert_eq!(function_counts, [2, 3]); + } + + #[test] + fn top_level_source_local_count_is_monotonic_across_batches() { + let mut analyser = Analyser::from_scope_tree(ScopeTree::from_global_scope(vec![])); + + for (source, expected) in [ + ("{ let first = 1; let second = 2; second };", 2), + ("let root = 0;", 2), + ("let next = 1; let last = 2;", 3), + ] { + let tokens = Lexer::new(source, SourceId::SYNTHETIC) + .collect::, _>>() + .expect("lex failed"); + let mut expressions = Parser::from_tokens(tokens).parse().expect("parse failed"); + for expression in &mut expressions { + analyser.analyse(expression).expect("analysis failed"); + } + + let result = analyser.take_result(); + assert!( + result.errors.is_empty(), + "analysis errors: {:?}", + result.errors + ); + assert_eq!(result.source_local_counts.top_level, expected); + } + } + #[test] fn inferred_list_slice_assignment_widens_element_type() { assert_eq!( diff --git a/ndc_analyser/src/scope.rs b/ndc_analyser/src/scope.rs index 6c2ec46e..8c09e47a 100644 --- a/ndc_analyser/src/scope.rs +++ b/ndc_analyser/src/scope.rs @@ -130,6 +130,9 @@ pub(crate) struct Scope { creates_environment: bool, // Only true for function scopes and for-loop iterations base_offset: usize, function_scope_idx: usize, + /// Source-local high-water mark for this function frame. Only meaningful + /// on the function scope itself; child scopes update their owning frame. + local_count: usize, identifiers: Vec, upvalues: Vec<(String, CaptureSource)>, } @@ -145,6 +148,7 @@ impl Scope { creates_environment: true, base_offset: 0, function_scope_idx, + local_count: 0, identifiers: Vec::default(), upvalues: Vec::default(), } @@ -160,6 +164,7 @@ impl Scope { creates_environment: false, base_offset, function_scope_idx, + local_count: 0, identifiers: Vec::default(), upvalues: Vec::default(), } @@ -479,6 +484,18 @@ impl ScopeTree { .collect() } + /// Highest source-local slot used by the current function frame, plus one. + /// Child scopes update this value as bindings are allocated, so destroyed + /// block and iteration scopes remain represented in the high-water mark. + pub(crate) fn current_function_local_count(&self) -> usize { + let function_scope_idx = self.scopes[self.current_scope_idx].function_scope_idx; + self.scopes[function_scope_idx].local_count + } + + pub(crate) fn current_function_is_top_level(&self) -> bool { + self.scopes[self.current_scope_idx].function_scope_idx == 0 + } + // When the Analyser encounters an identifier as the rhs of an expression during resolution it // will use this method to lookup if that identifier has already been seen. pub(crate) fn get_binding_any(&mut self, ident: &str) -> Option { @@ -857,9 +874,11 @@ impl ScopeTree { ident: String, binding: TypeBinding, ) -> ResolvedVar { - ResolvedVar::Local { - slot: self.scopes[self.current_scope_idx].allocate(ident, binding), - } + let function_scope_idx = self.scopes[self.current_scope_idx].function_scope_idx; + let slot = self.scopes[self.current_scope_idx].allocate(ident, binding); + self.scopes[function_scope_idx].local_count = + self.scopes[function_scope_idx].local_count.max(slot + 1); + ResolvedVar::Local { slot } } /// Check whether the current scope already has a `fn` declaration with diff --git a/ndc_interpreter/src/lib.rs b/ndc_interpreter/src/lib.rs index cc7dfda1..06078345 100644 --- a/ndc_interpreter/src/lib.rs +++ b/ndc_interpreter/src/lib.rs @@ -1,7 +1,7 @@ use ndc_analyser::{Analyser, ScopeTree}; use ndc_core::FunctionRegistry; use ndc_lexer::{Lexer, SourceDb, SourceId, TokenLocation}; -use ndc_parser::ExpressionLocation; +use ndc_parser::{ExpressionLocation, SourceLocalCounts}; use ndc_vm::compiler::Compiler; use ndc_vm::value::CompiledFunction; use ndc_vm::{OutputSink, Vm}; @@ -121,8 +121,11 @@ impl Interpreter { pub fn compile_str(&mut self, input: &str) -> Result { let source_id = self.source_db.add("", input); - let (expressions, _) = self.parse_and_analyse(input, source_id)?; - Ok(Compiler::compile(expressions.into_iter())?) + let (expressions, analysis, _) = self.parse_and_analyse(input, source_id)?; + Ok(Compiler::compile( + expressions.into_iter(), + analysis.source_local_counts, + )?) } /// Like [`Self::compile_str`] but skips the peephole optimizer. @@ -132,8 +135,11 @@ impl Interpreter { input: &str, ) -> Result { let source_id = self.source_db.add("", input); - let (expressions, _) = self.parse_and_analyse(input, source_id)?; - Ok(Compiler::compile_unoptimized(expressions.into_iter())?) + let (expressions, analysis, _) = self.parse_and_analyse(input, source_id)?; + Ok(Compiler::compile_unoptimized( + expressions.into_iter(), + analysis.source_local_counts, + )?) } pub fn disassemble_str(&mut self, input: &str) -> Result { @@ -171,8 +177,9 @@ impl Interpreter { // fails — otherwise the analyser would remember declarations from a line // that never actually ran. let analyser_checkpoint = self.analyser.checkpoint(); - let (expressions, mut timings) = self.parse_and_analyse(input, source_id)?; - let vm_result = self.interpret_vm(input, expressions.into_iter()); + let (expressions, analysis, mut timings) = self.parse_and_analyse(input, source_id)?; + let vm_result = + self.interpret_vm(input, expressions.into_iter(), analysis.source_local_counts); if vm_result.is_err() { self.analyser.restore(analyser_checkpoint); } @@ -186,7 +193,7 @@ impl Interpreter { &mut self, input: &str, source_id: SourceId, - ) -> Result<(Vec, ExecutionTimings), InterpreterError> { + ) -> Result<(Vec, AnalysisResult, ExecutionTimings), InterpreterError> { let mut timings = ExecutionTimings::default(); let tokens = measure(&mut timings, Phase::Lexing, || { @@ -203,6 +210,7 @@ impl Interpreter { // Hard errors (structural issues) still abort immediately. if let Err(e) = self.analyser.analyse(e) { self.analyser.restore(checkpoint.clone()); + self.analyser.take_result(); return Err(InterpreterError::Resolver { causes: vec![e] }); } } @@ -218,7 +226,8 @@ impl Interpreter { }); } - Ok((expressions, timings)) + let analysis = self.analyser.take_result(); + Ok((expressions, analysis, timings)) } fn interpret_vm( @@ -226,6 +235,7 @@ impl Interpreter { #[cfg(feature = "trace")] input: &str, #[cfg(not(feature = "trace"))] _input: &str, expressions: impl Iterator, + source_local_counts: SourceLocalCounts, ) -> Result<(Value, ExecutionTimings), InterpreterError> { use ndc_vm::{Function as VmFunction, Object as VmObject, Value as VmValue}; @@ -248,7 +258,7 @@ impl Interpreter { OutputSink::Stdout }; let (code, checkpoint) = measure(&mut timings, Phase::Compiling, || { - Compiler::compile_resumable(expressions) + Compiler::compile_resumable(expressions, source_local_counts) })?; let mut vm = Vm::new(code, globals).with_output(output); #[cfg(feature = "trace")] @@ -271,7 +281,7 @@ impl Interpreter { // find their locals on the stack. let old_checkpoint = checkpoint.clone(); let (code, new_checkpoint) = measure(&mut timings, Phase::Compiling, || { - checkpoint.resume(expressions) + checkpoint.resume(expressions, source_local_counts) })?; vm.resume_from_halt(code, globals, resume_ip, prev_num_locals); #[cfg(feature = "trace")] diff --git a/ndc_parser/src/expression.rs b/ndc_parser/src/expression.rs index 30ff2f9b..c46f0b2a 100644 --- a/ndc_parser/src/expression.rs +++ b/ndc_parser/src/expression.rs @@ -4,6 +4,7 @@ use ndc_core::{StaticType, TypeSignature}; use ndc_lexer::Span; use num::BigInt; use num::complex::Complex64; +use std::collections::HashMap; use std::sync::atomic::{AtomicU32, Ordering}; /// Unique identity for an AST node. Used as a key in side tables (e.g. the @@ -19,6 +20,23 @@ impl NodeId { } } +/// Analyser-assigned source-local high-water marks for each compilation frame. +/// +/// The top-level count remains monotonic across REPL analysis batches. Nested +/// functions use independent slot namespaces and are keyed by their declaration +/// node so the compiler can reserve their source slots before hidden temporaries. +/// +/// This is transitional metadata while the compiler consumes the resolved AST +/// directly. When HIR is introduced, lowering should represent source locals +/// and generated temporaries as logical local IDs. A frame-layout pass can then +/// assign slots and store `num_locals` directly on each HIR module or function, +/// replacing this top-level count and `NodeId` side table. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub struct SourceLocalCounts { + pub top_level: usize, + pub functions: HashMap, +} + #[derive(Debug, Eq, PartialEq, Clone)] pub enum Binding { None, diff --git a/ndc_parser/src/lib.rs b/ndc_parser/src/lib.rs index 98cff6dc..31dd04ed 100644 --- a/ndc_parser/src/lib.rs +++ b/ndc_parser/src/lib.rs @@ -4,7 +4,7 @@ mod parser; pub use expression::{ AugmentedAssignmentPlan, Binding, Candidate, CaptureSource, Expression, ExpressionLocation, - ForBody, ForIteration, FunctionParameter, Lvalue, NodeId, ResolvedVar, + ForBody, ForIteration, FunctionParameter, Lvalue, NodeId, ResolvedVar, SourceLocalCounts, }; pub use operator::{BinaryOperator, LogicalOperator, UnaryOperator}; pub use parser::Error; diff --git a/ndc_vm/src/compiler.rs b/ndc_vm/src/compiler.rs index 159a5475..ad05c27e 100644 --- a/ndc_vm/src/compiler.rs +++ b/ndc_vm/src/compiler.rs @@ -5,7 +5,8 @@ use ndc_core::{StaticType, TypeSignature}; use ndc_lexer::Span; use ndc_parser::{ AugmentedAssignmentPlan, Binding, Candidate, CaptureSource, Expression, ExpressionLocation, - ForBody, ForIteration, FunctionParameter, LogicalOperator, Lvalue, ResolvedVar, + ForBody, ForIteration, FunctionParameter, LogicalOperator, Lvalue, NodeId, ResolvedVar, + SourceLocalCounts, }; use std::rc::Rc; @@ -16,6 +17,7 @@ pub struct Compiler { loop_stack: Vec, allow_return: bool, optimize: bool, + source_local_counts: Rc, } impl Default for Compiler { @@ -26,6 +28,7 @@ impl Default for Compiler { loop_stack: Vec::new(), allow_return: false, optimize: true, + source_local_counts: Rc::new(SourceLocalCounts::default()), } } } @@ -33,8 +36,9 @@ impl Default for Compiler { impl Compiler { pub fn compile( expressions: impl Iterator, + source_local_counts: SourceLocalCounts, ) -> Result { - let mut compiler = Self::default(); + let mut compiler = Self::with_source_local_counts(true, source_local_counts); compiler.compile_batch(expressions.collect())?; Ok(compiler.finish()?.0) } @@ -44,11 +48,9 @@ impl Compiler { /// debugging tools (e.g. a future `--no-optimize` disassembler flag). pub fn compile_unoptimized( expressions: impl Iterator, + source_local_counts: SourceLocalCounts, ) -> Result { - let mut compiler = Self { - optimize: false, - ..Default::default() - }; + let mut compiler = Self::with_source_local_counts(false, source_local_counts); compiler.compile_batch(expressions.collect())?; Ok(compiler.finish()?.0) } @@ -63,11 +65,9 @@ impl Compiler { /// the emitted chunk, and shifting instructions invalidates that. pub fn compile_resumable( expressions: impl Iterator, + source_local_counts: SourceLocalCounts, ) -> Result<(CompiledFunction, Self), CompileError> { - let mut compiler = Self { - optimize: false, - ..Default::default() - }; + let mut compiler = Self::with_source_local_counts(false, source_local_counts); compiler.compile_batch(expressions.collect())?; compiler.finish() } @@ -82,18 +82,25 @@ impl Compiler { pub fn resume( self, new_expressions: impl Iterator, + source_local_counts: SourceLocalCounts, ) -> Result<(CompiledFunction, Self), CompileError> { let mut compiler = self; // checkpoint has no trailing Halt + compiler.num_locals = compiler.num_locals.max(source_local_counts.top_level); + compiler.source_local_counts = Rc::new(source_local_counts); compiler.compile_batch(new_expressions.collect())?; compiler.finish() } - /// Reserve every analyser-assigned source slot before allocating hidden - /// compiler temporaries. The analyser resolves an entire function before - /// bytecode emission, so source declarations compiled later may otherwise - /// collide with a temporary allocated earlier. + fn with_source_local_counts(optimize: bool, source_local_counts: SourceLocalCounts) -> Self { + Self { + num_locals: source_local_counts.top_level, + optimize, + source_local_counts: Rc::new(source_local_counts), + ..Default::default() + } + } + fn compile_batch(&mut self, expressions: Vec) -> Result<(), CompileError> { - self.num_locals = self.num_locals.max(source_local_count(&expressions)); for expression in expressions { self.compile_expr(expression)?; } @@ -142,7 +149,9 @@ impl Compiler { expression_location: ExpressionLocation, ) -> Result<(), CompileError> { let ExpressionLocation { - expression, span, .. + expression, + span, + id, } = expression_location; match expression { Expression::BoolLiteral(b) => { @@ -283,6 +292,7 @@ impl Compiler { } => { let type_signature = FunctionParameter::from_params(¶meters); self.compile_function_decl( + id, name, resolved_name, *body, @@ -645,6 +655,7 @@ impl Compiler { #[allow(clippy::too_many_arguments)] fn compile_function_decl( &mut self, + node_id: NodeId, name: Option, resolved_name: Option, body: ExpressionLocation, @@ -668,11 +679,17 @@ impl Compiler { }, return_type: Box::new(return_type.clone()), }; - let function_source_locals = source_local_count(std::slice::from_ref(&body)); + let function_source_locals = self + .source_local_counts + .functions + .get(&node_id) + .copied() + .ok_or_else(|| CompileError::missing_source_local_count(node_id, span))?; let mut fn_compiler = Self { num_locals: num_params.max(function_source_locals), allow_return: true, optimize: self.optimize, + source_local_counts: Rc::clone(&self.source_local_counts), ..Default::default() }; fn_compiler.compile_expr(body)?; @@ -1002,215 +1019,6 @@ impl PreparedAssignmentTarget { } } -/// Number of analyser-assigned local slots in the current function. -/// -/// Nested function bodies use their own slot namespace and are deliberately -/// skipped; their compiler performs the same scan when that body is lowered. -fn source_local_count(expressions: &[ExpressionLocation]) -> usize { - expressions - .iter() - .filter_map(max_source_local_in_expression) - .max() - .map_or(0, |slot| slot + 1) -} - -fn max_source_local_in_expression(location: &ExpressionLocation) -> Option { - let expression = &location.expression; - match expression { - Expression::BoolLiteral(_) - | Expression::StringLiteral(_) - | Expression::Int64Literal(_) - | Expression::Float64Literal(_) - | Expression::BigIntLiteral(_) - | Expression::ComplexLiteral(_) - | Expression::Break - | Expression::Continue => None, - Expression::Identifier { resolved, .. } => max_source_local_in_binding(resolved), - Expression::Statement(inner) | Expression::Grouping(inner) => { - max_source_local_in_expression(inner) - } - Expression::Logical { left, right, .. } => [ - max_source_local_in_expression(left), - max_source_local_in_expression(right), - ] - .into_iter() - .flatten() - .max(), - Expression::VariableDeclaration { l_value, value, .. } - | Expression::Assignment { - l_value, - r_value: value, - } => [ - max_source_local_in_lvalue(l_value), - max_source_local_in_expression(value), - ] - .into_iter() - .flatten() - .max(), - Expression::OpAssignment { - l_value, - r_value, - plan, - .. - } => { - let operation = match plan { - AugmentedAssignmentPlan::Unresolved => None, - AugmentedAssignmentPlan::Resolved(binding) => max_source_local_in_binding(binding), - }; - [ - max_source_local_in_lvalue(l_value), - max_source_local_in_expression(r_value), - operation, - ] - .into_iter() - .flatten() - .max() - } - Expression::FunctionDeclaration { - resolved_name, - captures, - .. - } => resolved_name - .iter() - .filter_map(max_source_local_in_var) - .chain(captures.iter().filter_map(|capture| match capture { - CaptureSource::Local(slot) => Some(*slot), - CaptureSource::Upvalue(_) => None, - })) - .max(), - Expression::Block { statements } => statements - .iter() - .filter_map(max_source_local_in_expression) - .max(), - Expression::If { - condition, - on_true, - on_false, - } => [ - max_source_local_in_expression(condition), - max_source_local_in_expression(on_true), - on_false.as_deref().and_then(max_source_local_in_expression), - ] - .into_iter() - .flatten() - .max(), - Expression::While { - expression, - loop_body, - } => [ - max_source_local_in_expression(expression), - max_source_local_in_expression(loop_body), - ] - .into_iter() - .flatten() - .max(), - Expression::For { iterations, body } => iterations - .iter() - .filter_map(|iteration| match iteration { - ForIteration::Iteration { l_value, sequence } => [ - max_source_local_in_lvalue(l_value), - max_source_local_in_expression(sequence), - ] - .into_iter() - .flatten() - .max(), - ForIteration::Guard(guard) => max_source_local_in_expression(guard), - }) - .chain(max_source_local_in_for_body(body)) - .max(), - Expression::Call { - function, - arguments, - } - | Expression::OperatorCall { - function, - arguments, - } => std::iter::once(max_source_local_in_expression(function)) - .chain(arguments.iter().map(max_source_local_in_expression)) - .flatten() - .max(), - Expression::Tuple { values } | Expression::List { values } => values - .iter() - .filter_map(max_source_local_in_expression) - .max(), - Expression::Map { values, default } => values - .iter() - .flat_map(|(key, value)| { - std::iter::once(max_source_local_in_expression(key)) - .chain(value.as_ref().map(max_source_local_in_expression)) - }) - .chain(default.as_deref().map(max_source_local_in_expression)) - .flatten() - .max(), - Expression::Return { value } => max_source_local_in_expression(value), - Expression::RangeInclusive { start, end } | Expression::RangeExclusive { start, end } => { - start - .as_deref() - .map(max_source_local_in_expression) - .into_iter() - .chain(end.as_deref().map(max_source_local_in_expression)) - .flatten() - .max() - } - } -} - -fn max_source_local_in_for_body(body: &ForBody) -> Option { - match body { - ForBody::Block(block) | ForBody::List { expr: block } => { - max_source_local_in_expression(block) - } - ForBody::Map { - key, - value, - default, - } => std::iter::once(max_source_local_in_expression(key)) - .chain(value.as_ref().map(max_source_local_in_expression)) - .chain(default.as_deref().map(max_source_local_in_expression)) - .flatten() - .max(), - } -} - -fn max_source_local_in_lvalue(lvalue: &Lvalue) -> Option { - match lvalue { - Lvalue::Identifier { resolved, .. } => resolved.as_ref().and_then(max_source_local_in_var), - Lvalue::Index { - value, - index, - resolved_set, - resolved_get, - } => [ - max_source_local_in_expression(value), - max_source_local_in_expression(index), - resolved_set.as_ref().and_then(max_source_local_in_binding), - resolved_get.as_ref().and_then(max_source_local_in_binding), - ] - .into_iter() - .flatten() - .max(), - Lvalue::Sequence(lvalues) => lvalues.iter().filter_map(max_source_local_in_lvalue).max(), - } -} - -fn max_source_local_in_binding(binding: &Binding) -> Option { - match binding { - Binding::None => None, - Binding::Resolved(candidate) => max_source_local_in_var(&candidate.var()), - Binding::Dynamic(candidates) => candidates - .iter() - .filter_map(|candidate| max_source_local_in_var(&candidate.var())) - .max(), - } -} - -fn max_source_local_in_var(variable: &ResolvedVar) -> Option { - match variable { - ResolvedVar::Local { slot } => Some(*slot), - ResolvedVar::Upvalue { .. } | ResolvedVar::Global { .. } => None, - } -} - /// Returns the minimum local slot referenced by an lvalue, used to determine /// which upvalues to close at the end of a loop iteration. fn min_lvalue_slot(lv: &Lvalue) -> Option { @@ -1251,6 +1059,15 @@ pub struct CompileError { } impl CompileError { + fn missing_source_local_count(node_id: NodeId, span: Span) -> Self { + Self { + text: format!( + "missing source-local count for function node {node_id:?}; analysed AST and metadata must be compiled together" + ), + span, + } + } + fn unresolved_binding(span: Span) -> Self { Self { text: "encountered unresolved binding during compilation, this is probably an internal error".to_string(), diff --git a/tests/compiler/tests/compiler.rs b/tests/compiler/tests/compiler.rs index d10097b6..2fd86acc 100644 --- a/tests/compiler/tests/compiler.rs +++ b/tests/compiler/tests/compiler.rs @@ -14,7 +14,7 @@ fn compile(input: &str) -> Vec { .collect::, _>>() .expect("lex failed"); let expressions = Parser::from_tokens(tokens).parse().expect("parse failed"); - Compiler::compile_unoptimized(expressions.into_iter()) + Compiler::compile_unoptimized(expressions.into_iter(), Default::default()) .expect("compile failed") .opcodes() .to_vec() @@ -435,6 +435,25 @@ fn test_augmented_assignment_temporaries_follow_source_locals() { ); } +// Resolved function AST and its analyser metadata form one compilation unit. +// Omitting the per-function frame size must fail instead of silently reserving +// only parameter slots and allowing hidden temporaries to overlap body locals. +#[test] +fn test_missing_function_source_local_count_is_rejected() { + let mut interp = ndc_interpreter::Interpreter::capturing(); + let (expressions, _) = interp + .analyse_str("fn f() { let local = 1; local }") + .expect("analysis failed"); + + let Err(error) = Compiler::compile(expressions.into_iter(), Default::default()) else { + panic!("compilation should reject missing source-local metadata"); + }; + assert!( + error.to_string().contains("missing source-local count"), + "unexpected compile error: {error}", + ); +} + // { let a = 3; a } // // Declaration stores 3 into pre-allocated slot 0. diff --git a/tests/compiler/tests/optimizer.rs b/tests/compiler/tests/optimizer.rs index 0470a90a..27144cc7 100644 --- a/tests/compiler/tests/optimizer.rs +++ b/tests/compiler/tests/optimizer.rs @@ -13,14 +13,14 @@ fn parse(input: &str) -> Vec { } fn unoptimized(input: &str) -> Vec { - Compiler::compile_unoptimized(parse(input).into_iter()) + Compiler::compile_unoptimized(parse(input).into_iter(), Default::default()) .expect("compile failed") .opcodes() .to_vec() } fn optimized(input: &str) -> Vec { - Compiler::compile(parse(input).into_iter()) + Compiler::compile(parse(input).into_iter(), Default::default()) .expect("compile failed") .opcodes() .to_vec() diff --git a/tests/functional/tests/repl.rs b/tests/functional/tests/repl.rs index 1d5c0c89..32ac1320 100644 --- a/tests/functional/tests/repl.rs +++ b/tests/functional/tests/repl.rs @@ -62,6 +62,18 @@ fn multiple_variables_persist() { assert_eq!(out.trim(), "12"); } +#[test] +fn resumed_compilation_reserves_new_source_locals_before_temporaries() { + // The second batch assigns `delta` slot 1. Indexed augmentation must move + // its hidden temporaries beyond the analyser's updated high-water mark. + let out = repl_output(&[ + "let values = [1];", + "values[0] += { let delta = 2; delta };", + "print(values)", + ]); + assert_eq!(out.trim(), "[3]"); +} + #[test] fn function_defined_on_earlier_line_is_callable() { let out = repl_output(&["fn double(x) => x * 2", "print(double(7))"]); diff --git a/tests/proptest/tests/panic.rs b/tests/proptest/tests/panic.rs index 6f0f9725..6db9cffc 100644 --- a/tests/proptest/tests/panic.rs +++ b/tests/proptest/tests/panic.rs @@ -247,7 +247,7 @@ fn run_pipeline(tokens: Vec) { return; } - let compiled = match Compiler::compile(expressions.into_iter()) { + let compiled = match Compiler::compile(expressions.into_iter(), Default::default()) { Ok(c) => c, Err(_) => return, };