From f8d87432d7d9b427ae39a34c168faed3f131b6be Mon Sep 17 00:00:00 2001 From: Tim Fennis Date: Tue, 13 Oct 2026 16:02:24 +0200 Subject: [PATCH 1/3] =?UTF-8?q?feat(analyser):=20keep=20resolving=20names?= =?UTF-8?q?=20after=20an=20error=20=F0=9F=A9=B9?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The resolver records an error per failing statement and carries on with the next one, at the top level and inside blocks, so `a; b;` reports both unknown names like the current analyser does. A failing statement still stops at its first error. Co-Authored-By: Claude Opus 5.5 --- ndc_analyser/src/name_resolution.rs | 21 ++++++- .../src/name_resolution/resolver_tests.rs | 61 ++++++++++++++++++- 2 files changed, 78 insertions(+), 4 deletions(-) diff --git a/ndc_analyser/src/name_resolution.rs b/ndc_analyser/src/name_resolution.rs index aa91b3bd..74f1de86 100644 --- a/ndc_analyser/src/name_resolution.rs +++ b/ndc_analyser/src/name_resolution.rs @@ -57,6 +57,9 @@ pub(crate) struct NameResolver { type_names: AHashMap, /// What each struct declaration binds, keyed by the declaration's node. structs: AHashMap, + /// Errors from statements that failed to resolve. Resolution carries on with + /// the next statement, so `a; b;` reports both unknown names. + errors: Vec, } #[derive(Debug, thiserror::Error)] @@ -292,6 +295,7 @@ impl NameResolver { setters: Default::default(), type_names: Default::default(), structs: Default::default(), + errors: Default::default(), } } @@ -314,10 +318,23 @@ impl NameResolver { self.captured.insert(symbol); } + /// Resolves `statement`, recording its error instead of returning it so the + /// statements after it still resolve. A failed statement stops at its first error. + pub(crate) fn resolve_statement(&mut self, statement: &ExpressionLocation) { + if let Err(error) = self.resolve(statement) { + self.errors.push(error); + } + } + + /// The errors recorded so far, leaving none behind. + pub(crate) fn take_errors(&mut self) -> Vec { + std::mem::take(&mut self.errors) + } + /// Resolve lexical names: values, accessors, and the type names in annotations. /// Choosing overloads, checking types and validating control flow belong to /// later passes. - pub(crate) fn resolve(&mut self, loc: &ExpressionLocation) -> Result<(), NameResolutionError> { + fn resolve(&mut self, loc: &ExpressionLocation) -> Result<(), NameResolutionError> { match &loc.expression { Expression::BoolLiteral(_) | Expression::NoneLiteral @@ -360,7 +377,7 @@ impl NameResolver { Expression::Block { statements } => { self.in_scope(ScopeKind::Block, |resolver| { for statement in statements { - resolver.resolve(statement)?; + resolver.resolve_statement(statement); } Ok(()) })?; diff --git a/ndc_analyser/src/name_resolution/resolver_tests.rs b/ndc_analyser/src/name_resolution/resolver_tests.rs index 113a1510..627af79a 100644 --- a/ndc_analyser/src/name_resolution/resolver_tests.rs +++ b/ndc_analyser/src/name_resolution/resolver_tests.rs @@ -12,9 +12,13 @@ fn resolve_source(resolver: &mut NameResolver, source: &str) -> Result<(), NameR .expect("source lexes"); let expressions = Parser::from_tokens(tokens).parse().expect("source parses"); for expression in &expressions { - resolver.resolve(expression)?; + resolver.resolve_statement(expression); + } + // Most tests care about the first error; the recovery tests read them all. + match resolver.take_errors().into_iter().next() { + Some(error) => Err(error), + None => Ok(()), } - Ok(()) } /// Declared symbols in declaration order; the symbol table hands out ids sequentially. @@ -1152,3 +1156,56 @@ fn struct_bindings_are_captured_like_functions() { [p.constructor, p.accessors[0].0] ); } + +/// Every error resolving `source` reports, in order. +fn all_errors(source: &str) -> Vec { + let mut resolver = NameResolver::new([]); + let tokens = Lexer::new(source, SourceId::new(0)) + .collect::, _>>() + .expect("source lexes"); + for expression in &Parser::from_tokens(tokens).parse().expect("source parses") { + resolver.resolve_statement(expression); + } + resolver + .take_errors() + .iter() + .map(ToString::to_string) + .collect() +} + +#[test] +fn errors_are_reported_for_every_failing_statement() { + let expected = [ + "Identifier a has not previously been declared", + "Identifier b has not previously been declared", + ]; + // At the top level, in a block and in a function body. + for source in ["a; b;", "{ a; b; }", "fn f() { a; b; }"] { + assert_eq!(all_errors(source), expected, "{source}"); + } + assert_eq!( + all_errors("let x: Unknwn = 1; undefined_variable;"), + [ + "unknown type `Unknwn`", + "Identifier undefined_variable has not previously been declared" + ] + ); +} + +#[test] +fn a_failing_statement_stops_at_its_first_error() { + assert_eq!( + all_errors("(a, b);"), + ["Identifier a has not previously been declared"] + ); +} + +#[test] +fn statements_after_an_error_still_resolve() { + let mut resolver = NameResolver::new([]); + resolve_source(&mut resolver, "missing; let x = 1; x;").unwrap_err(); + + let [x] = declared(&resolver); + assert_eq!(uses(&resolver, x), 1); + assert!(resolver.take_errors().is_empty()); +} From f7a8e4bd3961acd260bf22756bb11bf96de97223 Mon Sep 17 00:00:00 2001 From: Tim Fennis Date: Tue, 13 Oct 2026 16:05:17 +0200 Subject: [PATCH 2/3] =?UTF-8?q?feat(analyser):=20resolve=20names=20before?= =?UTF-8?q?=20analysis=20=F0=9F=94=97?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The analyser runs the name resolver on each top-level statement first, with the registered natives as builtins, and reports its errors as analysis errors. A statement with unresolved names isn't analysed further, which would only repeat its errors. The resolver's state is part of the analyser checkpoint, so REPL lines that fail roll it back. `fn` after a `let` of the same name in one scope is now an error, as decided for the new resolver, so the functional test that relied on it adding an overload now expects that error. The analyser's unit tests for index assignment declare the `[]` and `[]=` natives they use. Co-Authored-By: Claude Opus 5.5 --- ndc_analyser/src/analyser.rs | 68 +++++++++++++++++-- ndc_analyser/src/name_resolution.rs | 36 ++++++++-- ndc_analyser/src/scope.rs | 8 +++ ndc_analyser/src/symbols.rs | 4 +- .../034_fn_after_let_adds_overload.ndc | 7 -- .../034_fn_after_let_is_an_error.ndc | 6 ++ 6 files changed, 109 insertions(+), 20 deletions(-) delete mode 100644 tests/functional/programs/005_functions/034_fn_after_let_adds_overload.ndc create mode 100644 tests/functional/programs/005_functions/034_fn_after_let_is_an_error.ndc diff --git a/ndc_analyser/src/analyser.rs b/ndc_analyser/src/analyser.rs index 60b74680..d2dd536a 100644 --- a/ndc_analyser/src/analyser.rs +++ b/ndc_analyser/src/analyser.rs @@ -1,3 +1,4 @@ +use crate::name_resolution::{NameResolutionError, NameResolver}; use crate::scope::{CallKind, ResolvedCall, ScopeTree, TypeBinding, TypeDef}; use itertools::{Itertools, izip}; use ndc_core::static_type::StaticTypeConstructionError; @@ -33,12 +34,16 @@ pub struct AnalysisResult { /// discards every binding and struct declared since. #[derive(Debug, Clone)] pub struct Checkpoint { + resolver: NameResolver, scope_tree: ScopeTree, struct_count: usize, } #[derive(Debug)] pub struct Analyser { + /// Resolves names before analysis. It runs alongside the scope tree below + /// until the type pass reads names from it instead. + resolver: NameResolver, struct_registry: Rc>, scope_tree: ScopeTree, /// Stack of explicit `return` types for each enclosing function scope. @@ -57,6 +62,7 @@ impl Analyser { struct_registry: Rc>, ) -> Self { Self { + resolver: NameResolver::new(scope_tree.native_names()), scope_tree, struct_registry, return_type_stack: Vec::new(), @@ -67,12 +73,14 @@ impl Analyser { pub fn checkpoint(&self) -> Checkpoint { Checkpoint { + resolver: self.resolver.clone(), scope_tree: self.scope_tree.clone(), struct_count: self.struct_registry.borrow().len(), } } pub fn restore(&mut self, checkpoint: Checkpoint) { + self.resolver = checkpoint.resolver; self.scope_tree = checkpoint.scope_tree; self.struct_registry .borrow_mut() @@ -152,9 +160,25 @@ impl Analyser { !self.errors.is_empty() } + /// Analyses a top-level statement. Names resolve first; a statement with + /// unresolved names isn't analysed further, which would only repeat its errors. pub fn analyse( &mut self, expr_loc: &mut ExpressionLocation, + ) -> Result { + self.resolver.resolve_statement(expr_loc); + let name_errors = self.resolver.take_errors(); + if !name_errors.is_empty() { + self.errors + .extend(name_errors.into_iter().map(AnalysisError::from)); + return Ok(StaticType::Any); + } + self.analyse_expr(expr_loc) + } + + fn analyse_expr( + &mut self, + expr_loc: &mut ExpressionLocation, ) -> Result { let typ = self.analyse_inner(expr_loc)?; self.result.expr_types.insert(expr_loc.id, typ.clone()); @@ -164,7 +188,7 @@ impl Analyser { /// Like [`analyse`], but on error emits the error and returns `Any` /// so that analysis can continue. fn analyse_or_any(&mut self, expr_loc: &mut ExpressionLocation) -> StaticType { - match self.analyse(expr_loc) { + match self.analyse_expr(expr_loc) { Ok(t) => t, Err(e) => { self.emit(e); @@ -298,7 +322,7 @@ impl Analyser { self.analyse_or_any(right); Ok(StaticType::Bool) } - Expression::Grouping(expr) => self.analyse(expr), + Expression::Grouping(expr) => self.analyse_expr(expr), Expression::Cast { value, annotation, @@ -1273,6 +1297,16 @@ pub struct AnalysisError { help_text: Option, } +impl From for AnalysisError { + fn from(error: NameResolutionError) -> Self { + Self { + text: error.to_string(), + span: error.span(), + help_text: None, + } + } +} + impl AnalysisError { pub fn span(&self) -> Span { self.span @@ -1476,6 +1510,19 @@ mod tests { analyse_last_type_with_globals(source, vec![]) } + /// `[]` and `[]=`, which index reads and writes go through. Name resolution + /// rejects index access without them, as it would any other unknown function. + fn index_natives() -> Vec<(String, StaticType)> { + let any_function = StaticType::Function { + parameters: None, + return_type: Box::new(StaticType::Any), + }; + vec![ + ("[]".to_string(), any_function.clone()), + ("[]=".to_string(), any_function), + ] + } + fn assert_analysis_error(source: &str, globals: Vec<(String, StaticType)>, expected: &str) { let (_, result) = analyse_with_globals(source, globals); assert!( @@ -1491,7 +1538,10 @@ mod tests { #[test] fn inferred_list_index_assignment_widens_element_type() { assert_eq!( - analyse_last_type("let values = [1]; values[0] = \"two\"; values"), + analyse_last_type_with_globals( + "let values = [1]; values[0] = \"two\"; values", + index_natives() + ), StaticType::List(Box::new(StaticType::Any)), ); } @@ -1499,7 +1549,10 @@ mod tests { #[test] fn inferred_list_slice_assignment_widens_element_type() { assert_eq!( - analyse_last_type("let values = [1]; values[0..1] = [\"two\"]; values"), + analyse_last_type_with_globals( + "let values = [1]; values[0..1] = [\"two\"]; values", + index_natives() + ), StaticType::List(Box::new(StaticType::Any)), ); } @@ -1507,7 +1560,10 @@ mod tests { #[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"), + analyse_last_type_with_globals( + "let values = %{\"one\": 1}; values[\"two\"] = \"two\"; values", + index_natives() + ), StaticType::Map { key: Box::new(StaticType::String), value: Box::new(StaticType::Any), @@ -1524,7 +1580,7 @@ mod tests { assert_eq!( analyse_last_type_with_globals( "let values = [1]; values[0] += 0.5; values", - vec![("+".to_string(), add)], + [index_natives(), vec![("+".to_string(), add)]].concat(), ), StaticType::List(Box::new(StaticType::Any)), ); diff --git a/ndc_analyser/src/name_resolution.rs b/ndc_analyser/src/name_resolution.rs index 74f1de86..16421e1f 100644 --- a/ndc_analyser/src/name_resolution.rs +++ b/ndc_analyser/src/name_resolution.rs @@ -10,7 +10,7 @@ use ndc_parser::{ #[derive(Debug, Eq, PartialEq, Copy, Clone, Hash)] pub(crate) struct ScopeId(usize); -#[derive(Debug)] +#[derive(Debug, Clone)] pub(crate) struct Scope { parent: Option, bindings: Vec, @@ -18,12 +18,14 @@ pub(crate) struct Scope { kind: ScopeKind, } +#[derive(Debug, Clone)] pub(crate) struct ScopeTree { current: ScopeId, scopes: Vec, bound_in: AHashMap, } +#[derive(Debug, Clone)] pub(crate) struct NameResolver { symbols: SymbolTable, scopes: ScopeTree, @@ -62,7 +64,7 @@ pub(crate) struct NameResolver { errors: Vec, } -#[derive(Debug, thiserror::Error)] +#[derive(Debug, Clone, thiserror::Error)] pub(crate) enum NameResolutionError { #[error("Identifier {name} has not previously been declared")] UnknownName { name: String, span: Span }, @@ -88,17 +90,37 @@ pub(crate) enum NameResolutionError { IllegalStructRedefinition { name: String, span: Span }, } +impl NameResolutionError { + pub(crate) fn span(&self) -> Span { + match self { + Self::UnknownName { span, .. } + | Self::DuplicateParameter { span, .. } + | Self::IllegalFunctionRedefinition { span, .. } + | Self::SequenceTargetNotAllowed { span } + | Self::UnknownType { span, .. } + | Self::StructShadowsBuiltinType { span, .. } + | Self::StructRedefinition { span, .. } + | Self::DuplicateField { span, .. } + | Self::IllegalStructRedefinition { span, .. } => *span, + } + } +} + /// Same-named symbols visible at a use, nearest scope first, newest first /// within a scope. For `x` in `let x = 1; { let x = 2; x }`: `[[x2], [x1]]`. /// Identifiers always have at least one candidate, augmented operators may /// not. -#[derive(Debug)] +#[derive(Debug, Clone)] pub(crate) struct Candidates(Vec>); /// The two operations an augmented assignment can use. For `xs ++= ys`: /// `in_place` holds `++=` (mutates `xs`), `operator` holds `++` (builds a new value /// that is written back). Either may be empty; the type pass picks. -#[derive(Debug)] +#[derive(Debug, Clone)] +#[cfg_attr( + not(test), + expect(dead_code, reason = "read by the type pass, which is not written yet") +)] pub(crate) struct AugmentedCandidates { in_place: Candidates, operator: Candidates, @@ -106,7 +128,11 @@ pub(crate) struct AugmentedCandidates { /// The symbols a struct declaration binds. For `struct Point { x: Int }`: the /// type `Point`, the constructor `Point`, and the getter `x` with the setter `x=`. -#[derive(Debug)] +#[derive(Debug, Clone)] +#[cfg_attr( + not(test), + expect(dead_code, reason = "read by the type pass, which is not written yet") +)] pub(crate) struct StructSymbols { /// `Point` in annotations such as `let p: Point`. type_symbol: SymbolId, diff --git a/ndc_analyser/src/scope.rs b/ndc_analyser/src/scope.rs index 4e57e299..87edf3c0 100644 --- a/ndc_analyser/src/scope.rs +++ b/ndc_analyser/src/scope.rs @@ -422,6 +422,14 @@ impl ScopeTree { } } + /// The names of the native bindings, in registration order. + pub(crate) fn native_names(&self) -> impl Iterator { + self.global_scope + .identifiers + .iter() + .map(|binding| binding.name.as_str()) + } + pub(crate) fn get_type(&self, res: ResolvedVar) -> &StaticType { match res { ResolvedVar::Local { slot } => self.find_type_by_slot(self.current_scope_idx, slot), diff --git a/ndc_analyser/src/symbols.rs b/ndc_analyser/src/symbols.rs index ab0f4494..15471156 100644 --- a/ndc_analyser/src/symbols.rs +++ b/ndc_analyser/src/symbols.rs @@ -1,13 +1,13 @@ #[derive(Clone, Copy, Eq, PartialEq, Ord, PartialOrd, Hash, Debug)] pub(crate) struct SymbolId(usize); -#[derive(Debug)] +#[derive(Debug, Clone)] struct Symbol { name: String, kind: BindingKind, } -#[derive(Debug, Default)] +#[derive(Debug, Default, Clone)] pub(crate) struct SymbolTable { symbols: Vec, } diff --git a/tests/functional/programs/005_functions/034_fn_after_let_adds_overload.ndc b/tests/functional/programs/005_functions/034_fn_after_let_adds_overload.ndc deleted file mode 100644 index c0d0996f..00000000 --- a/tests/functional/programs/005_functions/034_fn_after_let_adds_overload.ndc +++ /dev/null @@ -1,7 +0,0 @@ -// A fn declaration after a let binding should add to the scope as a new -// overload entry (different arity), not replace the let binding. -let foo = fn(a) => "let:" ++ a; -fn foo(a, b) { "fn:" ++ a ++ "," ++ b } - -assert_eq(foo("x"), "let:x"); -assert_eq(foo("x", "y"), "fn:x,y"); diff --git a/tests/functional/programs/005_functions/034_fn_after_let_is_an_error.ndc b/tests/functional/programs/005_functions/034_fn_after_let_is_an_error.ndc new file mode 100644 index 00000000..70fe8a3c --- /dev/null +++ b/tests/functional/programs/005_functions/034_fn_after_let_is_an_error.ndc @@ -0,0 +1,6 @@ +// A scope holds a name either as `let` bindings or as functions, never both. +// A `fn` after a `let` of the same name in the same scope is an error rather +// than a new overload. In a nested scope it would shadow the `let` instead. +// expect-error: Illegal redefinition of variable foo as a function in the same scope +let foo = fn(a) => "let:" ++ a; +fn foo(a, b) { "fn:" ++ a ++ "," ++ b } From 39d6c025d7eaca364ea6991148900fbdf1a2d1ff Mon Sep 17 00:00:00 2001 From: Tim Fennis Date: Tue, 13 Oct 2026 16:05:32 +0200 Subject: [PATCH 3/3] =?UTF-8?q?docs(manual):=20document=20fn=20after=20let?= =?UTF-8?q?=20in=20the=20same=20scope=20=F0=9F=93=96?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude Opus 5.5 --- manual/src/reference/types/function.md | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/manual/src/reference/types/function.md b/manual/src/reference/types/function.md index 5e429033..2b5964f0 100644 --- a/manual/src/reference/types/function.md +++ b/manual/src/reference/types/function.md @@ -164,6 +164,18 @@ let foo = fn(a) => "let"; foo("x"); // "let": both fn overloads are shadowed ``` +The reverse order is an error within one scope: a scope holds a name either as `let` bindings or as functions. In a nested scope, the `fn` shadows the outer `let` as usual. The same rule applies to a struct's constructor. + +```ndc +let foo = fn(a) => "let"; +fn foo(a, b) { "fn" } // ERROR: Illegal redefinition of variable foo as a function in the same scope + +let bar = 1; +{ + fn bar(a) { a } // fine: a different scope +} +``` + A non-function `let` binding shadows the name for value access, but function calls still resolve to the underlying function: ```ndc