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 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 aa91b3bd..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, @@ -57,9 +59,12 @@ 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)] +#[derive(Debug, Clone, thiserror::Error)] pub(crate) enum NameResolutionError { #[error("Identifier {name} has not previously been declared")] UnknownName { name: String, span: Span }, @@ -85,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, @@ -103,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, @@ -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 { + 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(()) })?; 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()); +} 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 }