Small cleanups in ndc_analyser: unreachable!, stale comments, docs

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

Author: timfennis

ndc_analyser/REVIEW.md

@@ -27,29 +27,3 @@
   collects exact matches, loose matches, and all-by-name simultaneously would improve compile-time
   performance for programs with many overloads.
 
-## Small: Cleanup
-
-- **`debug_assert!(false)` → `unreachable!`** (`src/scope.rs` ~line 98)
-  A variadic function match should be impossible at this call-site. Replace with `unreachable!`
-  once confident the invariant holds.
-
-- **`new_iteration_scope` identical to `new_block_scope`** (`src/scope.rs` ~line 46)
-  Both constructors produce identical `Scope` values. Merge them, or add a comment explaining why
-  the distinction exists for future work (e.g., break/continue scoping).
-
-- **Anonymous slot uses magic string `"\x00"`** (`src/scope.rs` ~line 456)
-  `reserve_anonymous_slot` uses `"\x00"` as a sentinel name. An `Option<String>` name field would
-  be more explicit, though this works since the lexer never produces null bytes.
-
-- **Stale comment on `Return` analysis** (`src/analyser.rs` ~line 255)
-  Remove the "Actually it doesn't seem to make it any easier" comment.
-
-- **Commented-out debug println** (`src/analyser.rs` ~line 287)
-  Remove `// println!("resolve fn {name} {}", ...)`.
-
-- **Unnecessary clone of `StaticType::Any`** (`src/analyser.rs` ~line 490)
-  `resolved_type.clone()` on a unit variant — just push `StaticType::Any` directly.
-
-- **Missing comment on `from_global_scope` dual-root design** (`src/scope.rs` ~line 145)
-  `global_scope` is separate from `scopes[0]` — add a comment explaining why there are two root
-  scopes (one for natives, one for user top-level code).

ndc_analyser/src/analyser.rs

@@ -236,8 +236,6 @@ impl Analyser {
                     value: Box::new(value_type.unwrap_or_else(StaticType::unit)),
                 })
             }
-            // Return evaluates to the type of the expression it returns, which makes type checking easier!
-            // Actually it doesn't seem to make it any easier
             Expression::Return { value } => self.analyse(value),
             Expression::RangeInclusive { start, end }
             | Expression::RangeExclusive { start, end } => {
@@ -268,8 +266,6 @@ impl Analyser {
             return self.analyse(ident);
         };
 
-        // println!("resolve fn {name} {}", argument_types.iter().join(", "));
-
         let binding = self
             .scope_tree
             .resolve_function_binding(name, argument_types);
@@ -448,15 +444,14 @@ impl Analyser {
         let mut seen_names: Vec<&str> = Vec::new();
 
         for param in parameters {
-            let resolved_type = StaticType::Any;
-            types.push(resolved_type.clone());
+            types.push(StaticType::Any);
             if seen_names.contains(&param.name.as_str()) {
                 return Err(AnalysisError::parameter_redefined(&param.name, span));
             }
             seen_names.push(&param.name);
 
             self.scope_tree
-                .create_local_binding(param.name.clone(), resolved_type);
+                .create_local_binding(param.name.clone(), StaticType::Any);
         }
 
         Ok(types)

ndc_analyser/src/scope.rs

@@ -43,19 +43,14 @@ impl Scope {
         }
     }
 
+    /// Identical to `new_block_scope` today — kept as a separate constructor so that
+    /// iteration-specific behaviour (e.g. break/continue scoping) can be added later.
     pub(crate) fn new_iteration_scope(
         parent_idx: Option<usize>,
         base_offset: usize,
         function_scope_idx: usize,
     ) -> Self {
-        Self {
-            parent_idx,
-            creates_environment: false,
-            base_offset,
-            function_scope_idx,
-            identifiers: Vec::default(),
-            upvalues: Vec::default(),
-        }
+        Self::new_block_scope(parent_idx, base_offset, function_scope_idx)
     }
 
     pub(crate) fn find_slot_by_name(&self, find_ident: &str) -> Option<usize> {
@@ -94,9 +89,7 @@ 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");
-                    return Some(slot);
+                    unreachable!("find_function_candidates should never be called when there are variadic matches");
                 };
 
                 let is_good = param_types.len() == find_types.len()
@@ -142,6 +135,13 @@ pub struct ScopeTree {
 }
 
 impl ScopeTree {
+    /// Build a `ScopeTree` seeded with pre-registered global bindings (native functions etc.).
+    ///
+    /// Two root scopes exist by design: `global_scope` holds native/built-in bindings that are
+    /// always accessible, while `scopes[0]` is the user's top-level function scope where
+    /// user-defined declarations land. This separation keeps native bindings out of the
+    /// mutable scope chain so they can be searched as a fallback without interfering with
+    /// user-level shadowing.
     pub fn from_global_scope(global_scope_map: Vec<(String, StaticType)>) -> Self {
         let mut global_scope = Scope::new_function_scope(None, 0);
         global_scope.identifiers = global_scope_map;
@@ -406,6 +406,9 @@ impl ScopeTree {
     /// 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(), StaticType::Any)
     }