[fine] Many test improvements, error improvements

- Check for more error conditions
- Move to more definitive error assertions
- Simpler error messages in some cases
- Test more conditions more thoroughly, revisit old tests

fine/build.rs

@@ -37,8 +37,13 @@ fn generate_test_for_file(path: PathBuf) -> String {
         };
 
         let line = line.trim();
-        if line == "@ignore" {
+        if let Some(line) = line.strip_prefix("@ignore") {
-            disabled = quote! { #[ignore] };
+            let reason = line.trim();
+            assert_ne!(
+                reason, "",
+                "You need to provide at least some description for ignoring in {display_path}"
+            );
+            disabled = quote! { #[ignore = #reason] };
         } else if line == "@concrete:" {
             let mut concrete = String::new();
             while let Some(line) = lines.next() {

fine/src/compiler.rs

@@ -308,7 +308,6 @@ fn compile_literal(c: &mut Compiler, t: TreeRef, tr: &Tree) -> CR {
             Instruction::PushFalse
         }),
         Type::String => {
-            // TODO: Interpret string here make good!
             let mut result = String::new();
             let mut input = tok.as_str().chars();
             while let Some(ch) = input.next() {
@@ -514,7 +513,6 @@ fn compile_identifier_expression(c: &mut Compiler, t: TreeRef, tree: &Tree) -> O
 }
 
 fn compile_load_declaration(c: &mut Compiler, t: TreeRef, declaration: &Declaration) -> CR {
-    // TODO: Load function declaration. :P
     let instruction = match declaration {
         Declaration::Variable {
             location, index, ..
@@ -555,6 +553,8 @@ fn compile_load_declaration(c: &mut Compiler, t: TreeRef, declaration: &Declarat
             Instruction::LoadFunction(index)
         }
         Declaration::ExternFunction { id, .. } => Instruction::LoadExternFunction(id.id()),
+
+        // There is no universe where it's possible to use a class as a variable.
         Declaration::Class { .. } => Instruction::Panic,
     };
 

fine/src/parser.rs

@@ -510,7 +510,6 @@ fn file(p: &mut CParser) {
     let m = p.start();
     while !p.eof() {
         match p.peek() {
-            TokenKind::Fun => function(p),
             TokenKind::Class => class(p),
             _ => statement(p),
         }
@@ -660,6 +659,7 @@ fn block(p: &mut CParser) {
 
 fn statement(p: &mut CParser) {
     match p.peek() {
+        TokenKind::Fun => function(p),
         TokenKind::LeftBrace => block(p),
         TokenKind::Let => statement_let(p),
         TokenKind::Return => statement_return(p),
@@ -988,11 +988,14 @@ mod tests {
     #[test]
     fn tree_ref_size() {
         // What's the point of doing all that work if the tree ref isn't nice
-        // and "small"?
+        // and "small"? TreeRef is pervasive throughout the system: we use
+        // them to key function definitions and the type checker and use them
+        // to link classes to their definitions, etc. It's important that an
+        // Option<TreeRef> be *extremely* cheap to manipulate.
         //
-        // TODO: This is a dumb optimization because tokens are
+        // TODO: This optimization isn't as good as it might be because tokens are
         //       huge so Child is huge no matter what we do. If we retain
-        //       tokens out of line then we can re-visit this optimization.
+        //       tokens out of line then we can take full advantage of this.
         assert_eq!(4, std::mem::size_of::<Option<TreeRef>>());
     }
 }

fine/src/semantics.rs

@@ -86,9 +86,6 @@ pub enum Type {
     // chain assignments, and so we flow the type of the assignment through.)
     Assignment(Box<Type>),
 
-    // This is until generics are working
-    MagicPrintGarbage,
-
     // An potentially-bound type variable.
     // We need to ... like ... unify these things if possible.
     TypeVariable(TreeRef),
@@ -131,7 +128,6 @@ impl fmt::Display for Type {
             F64 => write!(f, "f64"),
             String => write!(f, "string"),
             Bool => write!(f, "bool"),
-            MagicPrintGarbage => write!(f, "MagicPrintGarbage"),
             Function(args, ret) => {
                 write!(f, "fun (")?;
                 let mut first = true;
@@ -224,8 +220,8 @@ impl Environment {
         }
     }
 
-    pub fn insert(&mut self, token: &Token, t: Type) {
+    pub fn insert(&mut self, token: &Token, t: Type) -> Option<Declaration> {
-        self.declarations.insert(
+        let result = self.declarations.insert(
             token.as_str().into(),
             Declaration::Variable {
                 declaration_type: t,
@@ -234,6 +230,7 @@ impl Environment {
             },
         );
         self.next_index += 1;
+        result
     }
 
     pub fn bind(&self, token: &Token) -> Option<&Declaration> {
@@ -387,14 +384,7 @@ impl<'a> Semantics<'a> {
             set_logical_parents(&mut logical_parents, tree, root, None);
         }
 
-        let mut root_environment = Environment::new(None, Location::Module);
+        let root_environment = Environment::new(None, Location::Module);
-        root_environment.declarations.insert(
-            "print".into(),
-            Declaration::ExternFunction {
-                declaration_type: Type::MagicPrintGarbage,
-                id: ExternalFunctionId(0),
-            },
-        );
 
         let mut semantics = Semantics {
             syntax_tree: tree,
@@ -526,23 +516,6 @@ impl<'a> Semantics<'a> {
 
             TreeKind::ForStatement => self.environment_of_for(parent, tree),
 
-            // TODO: Blocks should introduce a local environment if required.
-            //       Test with a type error in a block statement and a
-            //       binding outside. You will need a new assertion type and
-            //       possibly a compile/run to ensure it works.
-            //
-            //  let x = 7;
-            //  {
-            //    let x = 23;
-            //  }
-            //  print(x); // 7
-            //
-            //  {
-            //     let y = 12; // check: `y` is local not global!
-            //  }
-            //  print(y); // error, cannot find 'y'
-
-            // TODO: MORE Things that introduce an environment!
             _ => parent,
         };
 
@@ -558,18 +531,23 @@ impl<'a> Semantics<'a> {
                 Child::Tree(t) => {
                     let ct = &self.syntax_tree[*t];
                     if ct.kind == TreeKind::FunctionDecl {
-                        // TODO: Should I have accessors for function decls?
                         let Some(name) = ct.nth_token(1) else {
                             continue;
                         };
 
-                        environment.declarations.insert(
+                        let existing = environment.declarations.insert(
                             name.as_str().into(),
                             Declaration::Function {
                                 declaration_type: self.type_of(*t),
                                 declaration: *t,
                             },
                         );
+                        if existing.is_some() {
+                            self.report_error_tree(
+                                ct,
+                                format!("duplicate definition of function '{name}'"),
+                            );
+                        }
                     }
                 }
                 _ => {}
@@ -585,35 +563,42 @@ impl<'a> Semantics<'a> {
             match child {
                 Child::Tree(t) => {
                     let ct = &self.syntax_tree[*t];
-                    match ct.kind {
+                    let binding = match ct.kind {
                         TreeKind::FunctionDecl => {
-                            // TODO: Should I have accessors for function decls?
                             let Some(name) = ct.nth_token(1) else {
                                 continue;
                             };
 
-                            environment.declarations.insert(
+                            let declaration = Declaration::Function {
-                                name.as_str().into(),
+                                declaration_type: self.type_of(*t),
-                                Declaration::Function {
+                                declaration: *t,
-                                    declaration_type: self.type_of(*t),
+                            };
-                                    declaration: *t,
+                            Some(("function", name, declaration))
-                                },
-                            );
                         }
                         TreeKind::ClassDecl => {
                             let Some(name) = ct.nth_token(1) else {
                                 continue;
                             };
 
-                            environment.declarations.insert(
+                            let declaration = Declaration::Class {
-                                name.as_str().into(),
+                                declaration_type: self.type_of(*t),
-                                Declaration::Class {
+                                declaration: *t,
-                                    declaration_type: self.type_of(*t),
+                            };
-                                    declaration: *t,
+                            Some(("class", name, declaration))
-                                },
+                        }
+                        _ => None,
+                    };
+
+                    if let Some((what, name, declaration)) = binding {
+                        let existing = environment
+                            .declarations
+                            .insert(name.as_str().into(), declaration);
+                        if existing.is_some() {
+                            self.report_error_tree(
+                                ct,
+                                format!("duplicate definition of {what} '{name}'"),
                             );
                         }
-                        _ => {}
                     }
                 }
                 _ => {}
@@ -667,7 +652,12 @@ impl<'a> Semantics<'a> {
             };
 
             let declaration_type = self.type_of(*ct);
-            environment.insert(param_name, declaration_type);
+            if environment.insert(param_name, declaration_type).is_some() {
+                self.report_error_tree(
+                    param,
+                    format!("duplicate definition of parameter '{param_name}'"),
+                );
+            }
         }
 
         EnvironmentRef::new(environment)
@@ -948,7 +938,7 @@ impl<'a> Semantics<'a> {
         } else {
             self.report_error(
                 op.start,
-                format!("cannot assign a value of type `{right_type}` to type `{left_type}`"),
+                format!("cannot assign a value of type '{right_type}' to type '{left_type}'"),
             );
             Some(Type::Error)
         }
@@ -1106,7 +1096,7 @@ impl<'a> Semantics<'a> {
                     if !self.type_compat(&then_type, &else_type) {
                         self.report_error_tree(
                             tree,
-                            format!("the type of the `then` branch ({then_type}) must match the type of the `else` branch ({else_type})"),
+                            format!("the type of the 'then' branch ('{then_type}') must match the type of the 'else' branch ('{else_type}')"),
                         );
                         Some(Type::Error)
                     } else {
@@ -1172,18 +1162,6 @@ impl<'a> Semantics<'a> {
 
                 Some(*ret.clone())
             }
-            Type::MagicPrintGarbage => {
-                if arg_types.len() > 1 {
-                    self.report_error_tree(tree, "print takes a single argument");
-                    Some(Type::Error)
-                } else if arg_types.len() == 0 {
-                    Some(Type::Nothing)
-                } else {
-                    let mut arg_types = arg_types;
-                    let (_, t) = arg_types.pop().unwrap();
-                    Some(t)
-                }
-            }
             _ => {
                 self.report_error_tree_ref(f_ref, format!("expected a function type, got: {f}"));
                 Some(Type::Error)
@@ -1266,8 +1244,7 @@ impl<'a> Semantics<'a> {
                     declaration_type, ..
                 } => declaration_type.clone(),
                 Declaration::Class { .. } => {
-                    // TODO: Test this case
+                    self.report_error_tree(tree, format!("{id} is a class, not a value (did you mean to create a new instance with 'new'?)"));
-                    self.report_error_tree(tree, format!("{id} is a class, not a value (did you mean to create a new instance with `new`?)"));
                     Type::Error
                 }
             });
@@ -1396,7 +1373,7 @@ impl<'a> Semantics<'a> {
             Declaration::Class { .. } => {
                 self.report_error_tree(
                     tree,
-                    format!("`{id}` is a class, and cannot be the value of a field"),
+                    format!("'{id}' is a class, and cannot be the value of a field"),
                 );
                 Some(Type::Error)
             }
@@ -1500,7 +1477,9 @@ pub fn check(s: &Semantics) {
             TreeKind::Error => {} // already reported
             TreeKind::File => {}
             TreeKind::FunctionDecl => check_function_decl(s, t, tree),
-            TreeKind::ParamList => {}
+            TreeKind::ParamList => {
+                let _ = s.environment_of(t);
+            }
             TreeKind::Parameter => {
                 let _ = s.type_of(t);
             }
@@ -1546,7 +1525,7 @@ pub fn check(s: &Semantics) {
             }
             TreeKind::ForStatement => check_for_statement(s, t),
 
-            TreeKind::ClassDecl => {}
+            TreeKind::ClassDecl => check_class_declaration(s, tree),
             TreeKind::FieldDecl => {}
             TreeKind::FieldList => {}
             TreeKind::NewObjectExpression => check_new_object_expression(s, tree),
@@ -1582,7 +1561,7 @@ fn check_function_decl(s: &Semantics, t: TreeRef, tree: &Tree) {
                     (start, end_pos)
                 });
 
-            s.report_error_span(start, end, format!("the body of this function yields a value of type `{body_type}`, but callers expect this function to produce a `{return_type}`"));
+            s.report_error_span(start, end, format!("the body of this function yields a value of type '{body_type}', but callers expect this function to produce a '{return_type}'"));
         }
     }
 }
@@ -1617,7 +1596,7 @@ fn check_return_statement(s: &Semantics, tree: &Tree) {
             };
 
             if !s.type_compat(&expected_type, &actual_type) {
-                s.report_error_tree(tree, format!("callers of this function expect a value of type `{expected_type}` but this statement returns a value of type `{actual_type}`"));
+                s.report_error_tree(tree, format!("callers of this function expect a value of type '{expected_type}' but this statement returns a value of type '{actual_type}'"));
             }
         }
         Type::Error => (),
@@ -1634,11 +1613,6 @@ fn check_for_statement(s: &Semantics, t: TreeRef) {
     let _ = s.environment_of(t);
 }
 
-// TODO: TEST: Check mutual recursion with function calls
-// TODO: TEST: Missing fields
-// TODO: TEST: Extra fields
-// TODO: TEST: Existing and type mismatch
-
 fn check_new_object_expression(s: &Semantics, tree: &Tree) {
     let Some(type_expression) = tree.nth_tree(1) else {
         return;
@@ -1700,6 +1674,22 @@ fn check_new_object_expression(s: &Semantics, tree: &Tree) {
     }
 }
 
+fn check_class_declaration(s: &Semantics, tree: &Tree) {
+    let mut fields = HashMap::new();
+    for field in tree.children_of_kind(s.syntax_tree, TreeKind::FieldDecl) {
+        let f = &s.syntax_tree[field];
+        let Some(name) = f.nth_token(0) else {
+            continue;
+        };
+        match fields.insert(name.as_str(), field) {
+            Some(_) => {
+                s.report_error_tree(f, format!("duplicate definition of field '{name}'"));
+            }
+            None => {}
+        }
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;

fine/src/vm.rs

@@ -13,7 +13,8 @@ pub enum VMErrorCode {
     #[error("internal error: stack type mismatch: {0:?} is not {1:?}")]
     StackTypeMismatch(StackValue, Type),
 
-    // TODO: This one is *not* like the others!
+    // TODO: This one is *not* like the others! Distinguish between internal
+    //       errors and user errors?
     #[error("divide by zero")]
     DivideByZero,
 

fine/tests/example_tests.rs

@@ -298,7 +298,7 @@ fn assert_errors(tree: &SyntaxTree, lines: &Lines, expected_errors: Vec<&str>) {
         None,
         expected_errors,
         errors,
-        "expected no errors"
+        "expected error messages to match"
     );
 }
 

fine/tests/expression/errors/assignment_errors.fine

@@ -14,7 +14,7 @@ fun wrong() {
 }
 
 // @expect-errors:
-// | 7:4: cannot assign a value of type `string` to type `f64`
+// | 7:4: cannot assign a value of type 'string' to type 'f64'
-// | 8:4: cannot assign a value of type `f64` to type `string`
+// | 8:4: cannot assign a value of type 'f64' to type 'string'
-// | 11:4: cannot assign a value of type `f64` to type `string`
+// | 11:4: cannot assign a value of type 'f64' to type 'string'
 // | 13:2: cannot assign a new value to a function declaration

fine/tests/expression/errors/class_as_a_variable.fine

@@ -0,0 +1,8 @@
+class Foo {}
+
+fun test() -> f64 {
+  Foo + 23
+}
+
+// @expect-errors:
+// | 4:2: Foo is a class, not a value (did you mean to create a new instance with 'new'?)

fine/tests/expression/errors/class_duplicate_fields.fine

@@ -3,6 +3,5 @@ class Foo {
   x: f64;
 }
 
-// @ignore
 // @expect-errors:
-// asdfadsf
+// | 3:2: duplicate definition of field 'x'

fine/tests/expression/errors/duplicate_arguments.fine

@@ -1,5 +1,4 @@
 fun something(x: f64, x: f64) {}
 
-// @ignore
 // @expect-errors:
-// asdfadsf
+// | 1:22: duplicate definition of parameter 'x'

fine/tests/expression/errors/duplicates.fine

@@ -0,0 +1,16 @@
+fun nested() {
+  fun foo() {}
+  fun foo() {}
+
+  ;
+}
+
+fun nested() {}
+
+class Bar {}
+class Bar {}
+
+// @expect-errors:
+// | 3:2: duplicate definition of function 'foo'
+// | 8:0: duplicate definition of function 'nested'
+// | 11:0: duplicate definition of class 'Bar'

fine/tests/expression/errors/if_mismatched_arms.fine

@@ -1,2 +1,4 @@
 if true { "blarg" } else { 23 }
-// @type-error: 0 the type of the `then` branch (string) must match the type of the `else` branch (f64)
+
+// @expect-errors:
+// | 1:0: the type of the 'then' branch ('string') must match the type of the 'else' branch ('f64')

fine/tests/expression/errors/if_requires_else.fine

@@ -1,2 +1,4 @@
 if (if false { true }) { 32 } else { 23 }
-// @type-error: 4 the type of the `then` branch (bool) must match the type of the `else` branch (())
+
+// @expect-errors:
+// | 1:4: the type of the 'then' branch ('bool') must match the type of the 'else' branch ('()')

fine/tests/expression/errors/locals_in_globals.fine

@@ -0,0 +1,12 @@
+{
+  // This is a block-local declaration; it should *not* appear in the global
+  // environment.
+  let y = 23;
+}
+
+fun foo() -> f64 {
+  y + 3
+}
+
+// @expect-errors:
+// | 8:2: cannot find value y here

fine/tests/expression/errors/return_statement_mismatch.fine

@@ -5,4 +5,5 @@ fun test() -> f64 {
   23.0
 }
 
-// @check-error: callers of this function expect a value of type `f64` but this statement returns a value of type `string`
+// @expect-errors:
+// | 3:4: callers of this function expect a value of type 'f64' but this statement returns a value of type 'string'

fine/tests/expression/errors/return_type_mismatch.fine

@@ -2,4 +2,4 @@ fun test() -> bool {
   32
 }
 
-// @check-error: the body of this function yields a value of type `f64`, but callers expect this function to produce a `bool`
+// @check-error: the body of this function yields a value of type 'f64', but callers expect this function to produce a 'bool'

fine/tests/expression/generic_function.fine

@@ -6,6 +6,6 @@ fun test() {
   generic_add(10, 10)
 }
 
-// @ignore
+// @ignore Feature is undesigned, this is tentative garbage
 // @no-errors
 // @eval: 20

fine/tests/expression/lists.fine

@@ -11,5 +11,6 @@ fun test() -> f64 {
   sum(val)
 }
 
+// @ignore WIP
 // @no-errors
 // @type: 88 list<f64>

fine/tests/expression/variable.fine

@@ -1,10 +1,15 @@
 let x = 23;
 let y = x * 2;
-let z = print(y);
+let z = y;
 z;
 
+fun test() -> f64 {
+  x + y
+}
+
 // @no-errors
-// @type: 41 f64
+// @type: 38 f64
+// @eval: Float(69.0)
 // @concrete:
 // | File
 // |   LetStatement
@@ -29,25 +34,38 @@ z;
 // |     Let:'"let"'
 // |     Identifier:'"z"'
 // |     Equal:'"="'
-// |     CallExpression
+// |     Identifier
-// |       Identifier
+// |       Identifier:'"y"'
-// |         Identifier:'"print"'
-// |       ArgumentList
-// |         LeftParen:'"("'
-// |         Argument
-// |           Identifier
-// |             Identifier:'"y"'
-// |         RightParen:'")"'
 // |     Semicolon:'";"'
 // |   ExpressionStatement
 // |     Identifier
 // |       Identifier:'"z"'
 // |     Semicolon:'";"'
+// |   FunctionDecl
+// |     Fun:'"fun"'
+// |     Identifier:'"test"'
+// |     ParamList
+// |       LeftParen:'"("'
+// |       RightParen:'")"'
+// |     ReturnType
+// |       Arrow:'"->"'
+// |       TypeExpression
+// |         Identifier:'"f64"'
+// |     Block
+// |       LeftBrace:'"{"'
+// |       ExpressionStatement
+// |         BinaryExpression
+// |           Identifier
+// |             Identifier:'"x"'
+// |           Plus:'"+"'
+// |           Identifier
+// |             Identifier:'"y"'
+// |       RightBrace:'"}"'
 // |
 // @compiles-to:
 // | function << module >> (0 args, 0 locals):
 // |   strings (0):
-// |   code (14):
+// |   code (12):
 // |     0: PushFloat(23.0)
 // |     1: StoreModule(0)
 // |     2: LoadModule(0)
@@ -55,11 +73,16 @@ z;
 // |     4: FloatMultiply
 // |     5: StoreModule(1)
 // |     6: LoadModule(1)
-// |     7: LoadExternFunction(0)
+// |     7: StoreModule(2)
-// |     8: Call(1)
+// |     8: LoadModule(2)
-// |     9: StoreModule(2)
+// |     9: Discard
-// |     10: LoadModule(2)
+// |     10: PushNothing
-// |     11: Discard
+// |     11: Return
-// |     12: PushNothing
+// | function test (0 args, 0 locals):
-// |     13: Return
+// |   strings (0):
+// |   code (4):
+// |     0: LoadModule(0)
+// |     1: LoadModule(1)
+// |     2: FloatAdd
+// |     3: Return
 // |

fine/tests/expression/worst_fib.fine

@@ -4,10 +4,16 @@ fun worst_fib(n: f64) -> f64 {
   } else if n == 1 {
     1
   } else {
-    worst_fib(n-2) + worst_fib(n-1)
+    worst_fib(n-2) + delegate_worst_fib(n-1)
   }
 }
 
+// NOTE: This nonsense exists to make sure mutual recursion works, in
+//       addition to direct recursion.
+fun delegate_worst_fib(n: f64) -> f64 {
+  worst_fib(n)
+}
+
 fun test() -> f64 {
   worst_fib(10)
 }