feat(analyser): put the name resolver in front of the analyser 🔗

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

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<RefCell<StructRegistry>>,
     scope_tree: ScopeTree,
     /// Stack of explicit `return` types for each enclosing function scope.
@@ -57,6 +62,7 @@ impl Analyser {
         struct_registry: Rc<RefCell<StructRegistry>>,
     ) -> 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<StaticType, AnalysisError> {
+        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<StaticType, AnalysisError> {
         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<String>,
 }
 
+impl From<NameResolutionError> 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,23 +1538,32 @@ 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)),
         );
     }
 
     #[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)),
         );
     }
 
     #[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)),
         );

ndc_analyser/src/name_resolution.rs

@@ -10,20 +10,22 @@ 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<ScopeId>,
     bindings: Vec<SymbolId>,
     types: Vec<SymbolId>,
     kind: ScopeKind,
 }
 
+#[derive(Debug, Clone)]
 pub(crate) struct ScopeTree {
     current: ScopeId,
     scopes: Vec<Scope>,
     bound_in: AHashMap<SymbolId, ScopeId>,
 }
 
+#[derive(Debug, Clone)]
 pub(crate) struct NameResolver {
     symbols: SymbolTable,
     scopes: ScopeTree,
@@ -57,9 +59,12 @@ pub(crate) struct NameResolver {
     type_names: AHashMap<NodeId, SymbolId>,
     /// What each struct declaration binds, keyed by the declaration's node.
     structs: AHashMap<NodeId, StructSymbols>,
+    /// Errors from statements that failed to resolve. Resolution carries on with
+    /// the next statement, so `a; b;` reports both unknown names.
+    errors: Vec<NameResolutionError>,
 }
 
-#[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 },
@@ -85,25 +90,49 @@ 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<Vec<SymbolId>>);
 
 /// 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,
 }
 
 /// 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,
@@ -292,6 +321,7 @@ impl NameResolver {
             setters: Default::default(),
             type_names: Default::default(),
             structs: Default::default(),
+            errors: Default::default(),
         }
     }
 
@@ -314,10 +344,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<NameResolutionError> {
+        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 +403,7 @@ impl NameResolver {
             Expression::Block { statements } => {
                 self.in_scope(ScopeKind::Block, |resolver| {
                     for statement in statements {
-                        resolver.resolve(statement)?;
+                        resolver.resolve_statement(statement);
                     }
                     Ok(())
                 })?;

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<String> {
+    let mut resolver = NameResolver::new([]);
+    let tokens = Lexer::new(source, SourceId::new(0))
+        .collect::<Result<Vec<_>, _>>()
+        .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());
+}