🧹 Collect and remove TODO's in code

CLAUDE.md

@@ -6,6 +6,7 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co
 - Run `cargo fmt`
 - Run `cargo check` and fix warnings and errors
 - Ensure all tests pass (`cargo test`)
+- **No TODO comments in source code.** All open issues must be tracked in `TODO.md` instead. Do not introduce `// TODO`, `// FIXME`, or `// HACK` comments — add an entry to `TODO.md` or resolve the issue inline before committing.
 
 ## Common Commands
 

TODO.md

@@ -0,0 +1,135 @@
+# TODO
+
+Tracked open issues across the codebase. All items here must be resolved before merging — see `CLAUDE.md`.
+
+---
+
+## ndc_analyser
+
+### `src/analyser.rs`
+
+- **Option type inference** (`analyser.rs:42`): `None` literals default to `Option<Any>`. Needs Hindley-Milner or similar to infer the inner type; consider requiring explicit type annotations in the interim.
+
+- **Boolean type checking in Logical expressions** (`analyser.rs:58–59`): Operands of `&&`/`||` are not checked to be `Bool`. Emit an error or warning when a non-Bool operand is detected.
+
+- **Never type for VariableDeclaration** (`analyser.rs:66`): `VariableDeclaration` currently returns `StaticType::unit()`. It should return a bottom/never type since a declaration is not an expression that yields a value.
+
+- **Conflicting function bindings** (`analyser.rs:141`): Function hoisting always creates a new binding; it should error when a binding already exists in the same scope to catch accidental shadowing.
+
+- **Missing-semicolon warning in if-expression** (`analyser.rs:174`): Emit a diagnostic when an if-expression without a semicolon is used in statement position.
+
+- **Branch type mismatch warning** (`analyser.rs:178`): When the true and false branches of an if-expression have different static types, emit a warning.
+
+- **Empty list type inference** (`analyser.rs:225`): `[]` defaults to `List<Any>`. A better solution is needed — either infer from usage context or require a type annotation.
+
+- **Empty map type inference** (`analyser.rs:260`): Empty map literals default to `Map<Any, unit>` for the same reason as empty lists above.
+
+- **Dynamic binding type accuracy** (`analyser.rs:314`): `Binding::Dynamic` returns `Function { parameters: None, return_type: Any }` which is a lie. Ideally the type captures the union of all candidate signatures.
+
+- **For-loop variable type with untyped sequences** (`analyser.rs:342`): When the sequence has no type parameters the loop variable defaults to `Any`. This should improve once all sequence types carry element type parameters.
+
+- **Function parameter type inference** (`analyser.rs:503`): Parameter types in anonymous/lambda functions always default to `Any`. Requires an HM-like inference pass to do better.
+
+### `src/scope.rs`
+
+- **Variadic branch in `find_function_candidates`** (`scope.rs:99`): The variadic branch inside `find_function_candidates` has a `debug_assert!(false, ...)` and an early return. Determine whether this is truly unreachable and replace with `unreachable!()` or propagate a proper error.
+
+---
+
+## ndc_core
+
+### `src/num.rs`
+
+- **Bitwise NOT of non-integers** (`num.rs:181`): Bitwise negation of floats, rationals, and complex numbers silently produces `NaN`, matching Noulith. Decide whether this is the intended behaviour for Andy C++ or whether it should be an error.
+
+- **Division performance** (`num.rs:323`): `Div<&Number>` always converts both operands to rational before dividing, even for simple integer cases. Add an integer fast-path to avoid the allocation.
+
+- **BigInt rounding not downcasting to small int** (`num.rs:584`): The `round_impl!` macro converts `Rational` results to `Int::BigInt` unconditionally. It should attempt to downcast to `Int::Small` first when the value fits.
+
+---
+
+## ndc_lexer
+
+### `src/lib.rs`
+
+- **`consume()` internal error handling** (`lib.rs:202`): `consume()` calls `.expect()` on an empty iterator, which panics. Decide whether to use `unreachable!()` (if this is guaranteed impossible) or propagate a recoverable error.
+
+### `src/number.rs`
+
+- **Error interception after base-2 literals** (`number.rs:48`): The error-interception block after binary literal lexing may be unnecessary since the language has no numeric suffixes. Evaluate and remove or consolidate.
+
+- **Error interception after hex literals** (`number.rs:85`): Same question as above for hexadecimal literals.
+
+- **Error interception after octal literals** (`number.rs:104`): Same question as above for octal literals.
+
+- **Underscore after decimal point** (`number.rs:130`): `_` separators are silently ignored even after a `.` (e.g. `1._5`). Consider making this a lexer error.
+
+- **`validator_for_radix` performance** (`number.rs:231`): Uses string slicing and `str::contains` for each character. Replace with a direct range/char comparison for clarity and speed.
+
+### `src/string.rs`
+
+- **Unicode escape sequences** (`string.rs:72`): `\uXXXX` Unicode escapes are not yet supported in string literals.
+
+- **Byte escape sequences** (`string.rs:73`): `\xFF` byte escapes are unimplemented. Likely intentional (strings are UTF-8) but should be an explicit decision and documented error.
+
+---
+
+## ndc_parser
+
+### `src/parser.rs`
+
+- **Parser ambiguity: if-expression followed by `[`** (`parser.rs:738`): When an if-expression is followed by `[`, the parser cannot distinguish indexing from a list comprehension and emits a generic error. The error message should explain the ambiguity and suggest adding a semicolon.
+
+- **"Expected an expression" error quality** (`parser.rs:1001`): The fallback error message when no expression is found may not describe the situation accurately. Investigate and improve.
+
+---
+
+## ndc_stdlib
+
+### Documentation (`file.rs`, `index.rs`, `list.rs`, `math.rs`, `string.rs`)
+
+- **Missing documentation for stdlib functions**: Many `NativeFunction` registrations have `documentation: None`. The infrastructure for attaching human-readable documentation to stdlib functions needs to be completed so that `docs` queries return useful output. Affected files: `file.rs` (×2), `index.rs` (×2), `list.rs` (×2), `math.rs` (×14), `string.rs` (×1).
+
+### `src/value.rs`
+
+- **`docs()` helper commented out** (`value.rs:11`): The `docs()` function is stubbed and commented out. Complete the implementation once the `NativeFunction` documentation infrastructure is in place.
+
+---
+
+## ndc_vm
+
+Items from the VM code review (`ndc_vm/REVIEW.md`). Severity: **bug** > **performance** > **modularity** > **readability**.
+
+### Bugs
+
+- **`UnboundedRangeIter` wraps at `i64::MAX`** (`iterator.rs:149`, Minor): `self.current += 1` overflows silently in release builds, producing negative values. Use `checked_add(1)` and return `None` on overflow.
+
+- **`resolve_callee` panics on unexpected `Object` variants** (`vm.rs:766`, Minor): An unrecognised `Object` variant in the callee slot triggers an uncontrolled `panic!`. Return `Err(VmError)` instead, consistent with the surrounding `Result`-returning function.
+
+### Performance
+
+- **`MinHeapIter` clones then sorts** (`iterator.rs:274`, Minor): Clones all elements into a `Vec` then sorts. Draining the cloned `BinaryHeap` via repeated `pop()` yields elements in sorted order without the extra sort pass.
+
+### Modularity
+
+- **`compile_for_*` duplicated scaffolding** (`compiler.rs:748–930`, Minor): `compile_for_block`, `compile_for_list`, and `compile_for_map` each duplicate ~180 lines of loop scaffolding (GetIterator → new_loop_context → IterNext → CloseUpvalue → jump-back → patch breaks → Pop). Extract a shared helper that accepts a closure for the loop body.
+
+- **`VmIterator::deep_copy` has a silent default** (`iterator.rs:34`, Minor): The default implementation returns `None`, so any new iterator type that forgets to override `deep_copy` silently shares mutable state when values pass through `pure fn` memoization. Remove the default and make the method required.
+
+- **`OutputSink` closed to extension** (`vm.rs:20`, Low priority): Adding a third output destination requires touching the enum and every match arm. Low urgency; note for future refactors.
+
+### Idiomatic Rust
+
+- **`SetUpvalue` does not pop** (`vm.rs:307`, Minor): `SetLocal` pops the stack after storing; `SetUpvalue` uses `last()` + clone and leaves the value on the stack. The asymmetry should be documented or reconciled.
+
+- **`VmCallable` uses `RefCell<&mut Vm>`** (`vm.rs:1042`, Minor): Interior mutability via `RefCell<&mut Vm>` can panic on re-entrant borrow. Prefer `call(&mut self, ...)` if the macro codegen allows it.
+
+### Readability
+
+- **`compile_if` no-else unit push is unexplained** (`compiler.rs:564`, Nitpick): The no-else arm pushes `Value::unit()` without a comment, making both branches look structurally equivalent when they are not. Add a brief comment.
+
+- **Dead `else { None }` in `dispatch_call`** (`vm.rs:594`, Nitpick): The `else` path unconditionally sets `memo = None`, making the `let memo = ...` binding misleading. Simplify by passing `None` directly.
+
+- **Uninformative panic messages** (`vm.rs:434`, Nitpick): `"invalid type"` and `"invalid type 2"` provide no diagnostic context. Replace with descriptive messages including the opcode name and index.
+
+- **`max_local` vs `num_locals()`** (`compiler.rs:15`, Nitpick): The backing field and its public accessor use different names for the same concept. Rename one to match the other.

ndc_analyser/src/analyser.rs

@@ -39,7 +39,6 @@ impl Analyser {
                 resolved,
             } => {
                 if ident == "None" {
-                    // TODO: we're going to need something like HM to infer the type of option here, maybe force type annotations?
                     return Ok(StaticType::Option(Box::new(StaticType::Any)));
                 }
                 let binding = self.scope_tree.get_binding_any(ident).ok_or_else(|| {
@@ -55,15 +54,15 @@ impl Analyser {
                 Ok(StaticType::unit())
             }
             Expression::Logical { left, right, .. } => {
-                self.analyse(left)?; // TODO: throw error if type does not match bool?
-                self.analyse(right)?; // TODO: throw error if type does not match bool?
+                self.analyse(left)?;
+                self.analyse(right)?;
                 Ok(StaticType::Bool)
             }
             Expression::Grouping(expr) => self.analyse(expr),
             Expression::VariableDeclaration { l_value, value } => {
                 let typ = self.analyse(value)?;
                 self.resolve_lvalue_declarative(l_value, typ, *span)?;
-                Ok(StaticType::unit()) // TODO: never type here?
+                Ok(StaticType::unit())
             }
             Expression::Assignment { l_value, r_value } => {
                 self.resolve_lvalue(l_value, *span)?;
@@ -138,8 +137,6 @@ impl Analyser {
                 };
 
                 if let Some(slot) = pre_slot {
-                    // TODO: is this correct, for now we just always create a new binding, we could
-                    //       also produce an error if we are generating a conflicting binding
                     self.scope_tree
                         .update_binding_type(slot, function_type.clone());
                     *resolved_name = Some(slot);
@@ -170,13 +167,9 @@ impl Analyser {
                     StaticType::unit()
                 };
 
-                if true_type != StaticType::unit() {
-                    // TODO: Emit warning for not using a semicolon in this if
-                }
+                if true_type != StaticType::unit() {}
 
-                if true_type != false_type {
-                    // TODO maybe show warning?
-                }
+                if true_type != false_type {}
 
                 Ok(true_type.lub(&false_type))
             }
@@ -222,7 +215,6 @@ impl Analyser {
             Expression::List { values } => {
                 let element_type = self.analyse_multiple_expression_with_same_type(values)?;
 
-                // TODO: for now if we encounter an empty list expression we say the list is generic over Any but this clearly is not a good solution
                 Ok(StaticType::List(Box::new(
                     element_type.unwrap_or(StaticType::Any),
                 )))
@@ -257,7 +249,6 @@ impl Analyser {
                     self.analyse(default)?;
                 }
 
-                // TODO: defaulting to Any here is surely going to bite us later
                 Ok(StaticType::Map {
                     key: Box::new(key_type.unwrap_or(StaticType::Any)),
                     value: Box::new(value_type.unwrap_or_else(StaticType::unit)),
@@ -311,7 +302,6 @@ impl Analyser {
             }
             Binding::Resolved(res) => self.scope_tree.get_type(*res).clone(),
 
-            // TODO: are we just going to lie about the type or is this just how truthful we can be
             Binding::Dynamic(_) => StaticType::Function {
                 parameters: None,
                 return_type: Box::new(StaticType::Any),
@@ -339,7 +329,6 @@ impl Analyser {
 
                 self.scope_tree.new_iteration_scope();
 
-                // TODO: when we give type parameters to all instances of sequence we can correctly infer StaticType::Any in this position
                 self.resolve_lvalue_declarative(
                     l_value,
                     sequence_type
@@ -500,8 +489,6 @@ impl Analyser {
         let mut seen_names: Vec<&str> = Vec::new();
 
         for param in parameters {
-            // TODO: big challenge how do we figure out the function parameter types?
-            //       it seems like this is something we need an HM like system for!?
             let resolved_type = StaticType::Any;
             types.push(resolved_type.clone());
             if seen_names.contains(&param.name.as_str()) {

ndc_analyser/src/scope.rs

@@ -96,7 +96,6 @@ impl Scope {
                 let Some(param_types) = parameters else {
                     // If this branch happens then the function we're matching against is variadic meaning it's always a match
                     debug_assert!(false, "we should never be calling find_function_candidates if there were variadic matches");
-                    // TODO: Change to unreachable?
                     return Some(slot);
                 };
 

ndc_core/src/num.rs

@@ -178,7 +178,6 @@ impl Not for Number {
     fn not(self) -> Self::Output {
         match self {
             Self::Int(int) => int.not().into(),
-            // TODO: bitwise negation of all non integer numbers in Noulith result in NAN, is that what we want for our language too?
             _ => f64::NAN.into(),
         }
     }
@@ -320,7 +319,6 @@ impl_binary_operator_all!(Rem, rem, Rem::rem, Rem::rem, Rem::rem, Rem::rem);
 impl Div<&Number> for &Number {
     type Output = Number;
 
-    /// TODO: always converting operands to rational numbers is needlessly slow in some cases
     fn div(self, rhs: &Number) -> Self::Output {
         match (self.to_rational(), rhs.to_rational()) {
             (Some(left), Some(right)) if !right.is_zero() => Number::rational(left / right),
@@ -581,7 +579,6 @@ macro_rules! implement_rounding {
                             Number::Float(f)
                         }
                     }
-                    // TODO: fix bigint -> int
                     Number::Rational(r) => Number::Int(Int::BigInt(r.$method().to_integer())),
                     Number::Complex(c) => Complex::new(c.re.$method(), c.im.$method()).into(),
                 }

ndc_lexer/src/lib.rs

@@ -199,7 +199,6 @@ impl SourceIterator<'_> {
 
     pub fn consume(&mut self, count: usize) {
         for _ in 0..count {
-            // TODO: this is an internal error how should we handle these?
             self.next()
                 .expect("tried to consume but iterator was empty");
         }

ndc_lexer/src/number.rs

@@ -45,8 +45,6 @@ impl NumberLexer for Lexer<'_> {
 
             self.lex_to_buffer(&mut buf, |c| c == '1' || c == '0');
 
-            // TODO do these common error interceptions even make sense considering we don't really have any suffixes we support
-            // maybe we can pull these checks outside of lex number and see if the next token after lexing a number is ascii alpha?
             match self.source.peek() {
                 Some(c) if c.is_ascii_digit() => {
                     self.source.next();
@@ -82,8 +80,6 @@ impl NumberLexer for Lexer<'_> {
 
             self.lex_to_buffer(&mut buf, |c| c.is_ascii_hexdigit());
 
-            // TODO: also intercept common errors here?
-
             return match buf_to_token_with_radix(&buf, 16) {
                 Some(token) => Ok(TokenLocation {
                     token,
@@ -101,8 +97,6 @@ impl NumberLexer for Lexer<'_> {
 
             self.lex_to_buffer(&mut buf, |c| matches!(c, '0'..='7'));
 
-            // TODO: also intercept common errors here?
-
             return match buf_to_token_with_radix(&buf, 8) {
                 Some(token) => Ok(TokenLocation {
                     token,
@@ -127,7 +121,6 @@ impl NumberLexer for Lexer<'_> {
                 }
                 // A `_` inside a number is ignored unless it's after a `.`
                 '_' => {
-                    // TODO: Maybe disallow `_` after `.`
                     self.source.next();
                     // ignore underscore for nice number formatting
                 }
@@ -228,7 +221,6 @@ fn buf_to_token_with_radix(buf: &str, radix: u32) -> Option<Token> {
     }
 }
 
-// TODO: maybe this implementation is a lot slower than it needs to be?
 fn validator_for_radix(radix: usize) -> impl Fn(char) -> bool {
     move |c| "0123456789abcdefghijlkmnopqrstuvwxyz"[0..radix].contains(c.to_ascii_lowercase())
 }

ndc_lexer/src/string.rs

@@ -69,8 +69,6 @@ impl StringLexer for Lexer<'_> {
         // This was guaranteed by the caller, but we could make a nice error?
         assert_eq!(self.source.next(), Some('"'));
 
-        // TODO: support \u8080 type escape sequences
-        // TODO: should we handle bytes like \xFF? Probably not for strings because they aren't valid UTF-8
         let mut buf = String::new();
         #[allow(clippy::while_let_on_iterator)]
         while let Some(next_ch) = self.source.next() {

ndc_parser/src/parser.rs

@@ -735,23 +735,6 @@ impl Parser {
                         _ => {}
                     }
 
-                    // TODO: this error may be triggered in a scenario described below, and it would
-                    //       probably be nice if we could have a special message in a later version
-                    //
-                    // # Error code
-                    //
-                    // if x == y { true } else { false }
-                    // [x for x in 1..10]
-                    //
-                    // In this case we have some kind of expression that could also be a statement
-                    // followed by a list comprehension (the same problem would arise if the next
-                    // statement was a tuple). The list comprehension or tuple will now be interpreted
-                    // as an operand for the previous expression as if we meant to write this:
-                    //
-                    // if x == y { foo } else { bar }[12]
-                    //
-                    // This ambiguity can only be resolved by adding a semicolon to the if expression
-                    // or by not putting a list comprehension or tuple in this position.
                     let end_token =
                         self.require_current_token_matches(&Token::RightSquareBracket)?;
 
@@ -998,8 +981,6 @@ impl Parser {
                 resolved: Binding::None,
             },
             _ => {
-                // TODO: this error might not be the best way to describe what's happening here
-                //       figure out if there is a better way to handle errors here.
                 return Err(Error::text(
                     format!(
                         "Expected an expression but got '{}' instead",