chore: address some frontend tests TODOs

compiler/noirc_frontend/src/ast/expression.rs

@@ -930,6 +930,13 @@ impl FunctionReturnType {
             FunctionReturnType::Ty(typ) => Cow::Borrowed(typ),
         }
     }
+
+    pub fn location(&self) -> Location {
+        match self {
+            FunctionReturnType::Default(location) => *location,
+            FunctionReturnType::Ty(typ) => typ.location,
+        }
+    }
 }
 
 impl Display for FunctionReturnType {

compiler/noirc_frontend/src/elaborator/mod.rs

@@ -1264,9 +1264,13 @@ impl<'context> Elaborator<'context> {
         self.check_parent_traits_are_implemented(&trait_impl);
         self.remove_trait_impl_assumed_trait_implementations(trait_impl.impl_id);
 
-        for (module, function, _) in &trait_impl.methods.functions {
+        for (module, function, noir_function) in &trait_impl.methods.functions {
             self.local_module = *module;
-            let errors = check_trait_impl_method_matches_declaration(self.interner, *function);
+            let errors = check_trait_impl_method_matches_declaration(
+                self.interner,
+                *function,
+                noir_function,
+            );
             self.push_errors(errors.into_iter().map(|error| error.into()));
         }
 

compiler/noirc_frontend/src/elaborator/statements.rs

@@ -91,15 +91,19 @@ impl Elaborator<'_> {
         let type_contains_unspecified = let_stmt.r#type.contains_unspecified();
         let annotated_type = self.resolve_inferred_type(let_stmt.r#type);
 
+        let pattern_location = let_stmt.pattern.location();
         let expr_location = let_stmt.expression.location;
         let (expression, expr_type) =
             self.elaborate_expression_with_target_type(let_stmt.expression, Some(&annotated_type));
 
         // Require the top-level of a global's type to be fully-specified
         if type_contains_unspecified && global_id.is_some() {
             let expected_type = annotated_type.clone();
-            let error =
-                ResolverError::UnspecifiedGlobalType { location: expr_location, expected_type };
+            let error = ResolverError::UnspecifiedGlobalType {
+                pattern_location,
+                expr_location,
+                expected_type,
+            };
             self.push_err(error);
         }
 
@@ -202,19 +206,18 @@ impl Elaborator<'_> {
         );
 
         // Check that start range and end range have the same types
-        let range_location = start_location.merge(end_location);
         self.unify(&start_range_type, &end_range_type, || TypeCheckError::TypeMismatch {
             expected_typ: start_range_type.to_string(),
             expr_typ: end_range_type.to_string(),
-            expr_location: range_location,
+            expr_location: end_location,
         });
 
         let expected_type = self.polymorphic_integer();
 
         self.unify(&start_range_type, &expected_type, || TypeCheckError::TypeCannotBeUsed {
             typ: start_range_type.clone(),
             place: "for loop",
-            location: range_location,
+            location: start_location,
         });
 
         self.interner.push_definition_type(identifier.id, start_range_type);

compiler/noirc_frontend/src/elaborator/traits.rs

@@ -273,6 +273,7 @@ impl Elaborator<'_> {
 pub(crate) fn check_trait_impl_method_matches_declaration(
     interner: &mut NodeInterner,
     function: FuncId,
+    noir_function: &NoirFunction,
 ) -> Vec<TypeCheckError> {
     let meta = interner.function_meta(&function);
     let modifiers = interner.function_modifiers(&function);
@@ -349,6 +350,8 @@ pub(crate) fn check_trait_impl_method_matches_declaration(
             definition_type,
             method_name,
             &meta.parameters,
+            &meta.return_type,
+            noir_function,
             meta.name.location,
             &trait_info.name.0.contents,
             &mut errors,
@@ -358,11 +361,14 @@ pub(crate) fn check_trait_impl_method_matches_declaration(
     errors
 }
 
+#[allow(clippy::too_many_arguments)]
 fn check_function_type_matches_expected_type(
     expected: &Type,
     actual: &Type,
     method_name: &str,
     actual_parameters: &Parameters,
+    actual_return_type: &FunctionReturnType,
+    noir_function: &NoirFunction,
     location: Location,
     trait_name: &str,
     errors: &mut Vec<TypeCheckError>,
@@ -381,11 +387,16 @@ fn check_function_type_matches_expected_type(
         if params_a.len() == params_b.len() {
             for (i, (a, b)) in params_a.iter().zip(params_b.iter()).enumerate() {
                 if a.try_unify(b, &mut bindings).is_err() {
+                    let parameter_location = noir_function.def.parameters.get(i);
+                    let parameter_location = parameter_location.map(|param| param.typ.location);
+                    let parameter_location =
+                        parameter_location.unwrap_or_else(|| actual_parameters.0[i].0.location());
+
                     errors.push(TypeCheckError::TraitMethodParameterTypeMismatch {
                         method_name: method_name.to_string(),
                         expected_typ: a.to_string(),
                         actual_typ: b.to_string(),
-                        parameter_location: actual_parameters.0[i].0.location(),
+                        parameter_location,
                         parameter_index: i + 1,
                     });
                 }
@@ -395,7 +406,7 @@ fn check_function_type_matches_expected_type(
                 errors.push(TypeCheckError::TypeMismatch {
                     expected_typ: ret_a.to_string(),
                     expr_typ: ret_b.to_string(),
-                    expr_location: location,
+                    expr_location: actual_return_type.location(),
                 });
             }
         } else {

compiler/noirc_frontend/src/elaborator/types.rs

@@ -174,8 +174,8 @@ impl Elaborator<'_> {
 
         if !kind.unifies(&resolved_type.kind()) {
             let expected_typ_err = CompilationError::TypeError(TypeCheckError::TypeKindMismatch {
-                expected_kind: kind.to_string(),
-                expr_kind: resolved_type.kind().to_string(),
+                expected_kind: kind.clone(),
+                expr_kind: resolved_type.kind(),
                 expr_location: location,
             });
             self.push_err(expected_typ_err);
@@ -523,8 +523,8 @@ impl Elaborator<'_> {
                     (Type::Constant(lhs, lhs_kind), Type::Constant(rhs, rhs_kind)) => {
                         if !lhs_kind.unifies(&rhs_kind) {
                             self.push_err(TypeCheckError::TypeKindMismatch {
-                                expected_kind: lhs_kind.to_string(),
-                                expr_kind: rhs_kind.to_string(),
+                                expected_kind: lhs_kind,
+                                expr_kind: rhs_kind,
                                 expr_location: location,
                             });
                             return Type::Error;
@@ -557,8 +557,8 @@ impl Elaborator<'_> {
     fn check_kind(&mut self, typ: Type, expected_kind: &Kind, location: Location) -> Type {
         if !typ.kind().unifies(expected_kind) {
             self.push_err(TypeCheckError::TypeKindMismatch {
-                expected_kind: expected_kind.to_string(),
-                expr_kind: typ.kind().to_string(),
+                expected_kind: expected_kind.clone(),
+                expr_kind: typ.kind(),
                 expr_location: location,
             });
             return Type::Error;

compiler/noirc_frontend/src/hir/def_collector/errors.rs

@@ -77,7 +77,7 @@ pub enum DefCollectorErrorKind {
 impl DefCollectorErrorKind {
     pub fn location(&self) -> Location {
         match self {
-            DefCollectorErrorKind::Duplicate { first_def: ident, .. }
+            DefCollectorErrorKind::Duplicate { second_def: ident, .. }
             | DefCollectorErrorKind::UnresolvedModuleDecl { mod_name: ident, .. }
             | DefCollectorErrorKind::CannotReexportItemWithLessVisibility {
                 item_name: ident,
@@ -160,10 +160,10 @@ impl<'a> From<&'a DefCollectorErrorKind> for Diagnostic {
                     let second_location = second_def.0.location();
                     let mut diag = Diagnostic::simple_error(
                         primary_message,
-                        format!("First {} found here", &typ),
-                        first_location,
+                        format!("Second {} found here", &typ),
+                        second_location,
                     );
-                    diag.add_secondary(format!("Second {} found here", &typ), second_location);
+                    diag.add_secondary(format!("First {} found here", &typ), first_location);
                     diag
                 }
             }

compiler/noirc_frontend/src/hir/resolution/errors.rs

@@ -109,7 +109,11 @@ pub enum ResolverError {
     #[error("Only `comptime` globals can be mutable")]
     MutableGlobal { location: Location },
     #[error("Globals must have a specified type")]
-    UnspecifiedGlobalType { location: Location, expected_type: Type },
+    UnspecifiedGlobalType {
+        pattern_location: Location,
+        expr_location: Location,
+        expected_type: Type,
+    },
     #[error("Global failed to evaluate")]
     UnevaluatedGlobalType { location: Location },
     #[error("Globals used in a type position must be non-negative")]
@@ -168,7 +172,7 @@ pub enum ResolverError {
     AttributeFunctionIsNotAPath { function: String, location: Location },
     #[error("Attribute function `{name}` is not in scope")]
     AttributeFunctionNotInScope { name: String, location: Location },
-    #[error("The trait `{missing_trait}` is not implemented for `{type_missing_trait}")]
+    #[error("The trait `{missing_trait}` is not implemented for `{type_missing_trait}`")]
     TraitNotImplemented {
         impl_trait: String,
         missing_trait: String,
@@ -197,7 +201,7 @@ pub enum ResolverError {
 impl ResolverError {
     pub fn location(&self) -> Location {
         match self {
-            ResolverError::DuplicateDefinition { first_location: location, .. }
+            ResolverError::DuplicateDefinition { second_location: location, .. }
             | ResolverError::UnconditionalRecursion { location, .. }
             | ResolverError::PathIsNotIdent { location }
             | ResolverError::Expected { location, .. }
@@ -233,7 +237,7 @@ impl ResolverError {
             | ResolverError::WhileInConstrainedFn { location }
             | ResolverError::JumpOutsideLoop { location, .. }
             | ResolverError::MutableGlobal { location }
-            | ResolverError::UnspecifiedGlobalType { location, .. }
+            | ResolverError::UnspecifiedGlobalType { pattern_location: location, .. }
             | ResolverError::UnevaluatedGlobalType { location }
             | ResolverError::NegativeGlobalType { location, .. }
             | ResolverError::NonIntegralGlobalType { location, .. }
@@ -292,10 +296,10 @@ impl<'a> From<&'a ResolverError> for Diagnostic {
             ResolverError::DuplicateDefinition { name, first_location, second_location} => {
                 let mut diag = Diagnostic::simple_error(
                     format!("duplicate definitions of {name} found"),
-                    "first definition found here".to_string(),
-                    *first_location,
+                    "second definition found here".to_string(),
+                    *second_location,
                 );
-                diag.add_secondary("second definition found here".to_string(), *second_location);
+                diag.add_secondary("first definition found here".to_string(), *first_location);
                 diag
             }
             ResolverError::UnusedVariable { ident } => {
@@ -573,12 +577,14 @@ impl<'a> From<&'a ResolverError> for Diagnostic {
                     *location,
                 )
             },
-            ResolverError::UnspecifiedGlobalType { location, expected_type } => {
-                Diagnostic::simple_error(
+            ResolverError::UnspecifiedGlobalType { pattern_location, expr_location, expected_type } => {
+                let mut diagnostic = Diagnostic::simple_error(
                     "Globals must have a specified type".to_string(),
-                    format!("Inferred type is `{expected_type}`"),
-                    *location,
-                )
+                    String::new(),
+                    *pattern_location,
+                );
+                diagnostic.add_secondary(format!("Inferred type is `{expected_type}`"), *expr_location);
+                diagnostic
             },
             ResolverError::UnevaluatedGlobalType { location } => {
                 Diagnostic::simple_error(
@@ -763,9 +769,9 @@ impl<'a> From<&'a ResolverError> for Diagnostic {
             ResolverError::TraitNotImplemented { impl_trait, missing_trait: the_trait, type_missing_trait: typ, location, missing_trait_location} => {
                 let mut diagnostic = Diagnostic::simple_error(
                     format!("The trait bound `{typ}: {the_trait}` is not satisfied"), 
-                    format!("The trait `{the_trait}` is not implemented for `{typ}")
+                    format!("The trait `{the_trait}` is not implemented for `{typ}`")
                     , *location);
-                diagnostic.add_secondary(format!("required by this bound in `{impl_trait}"), *missing_trait_location);
+                diagnostic.add_secondary(format!("required by this bound in `{impl_trait}`"), *missing_trait_location);
                 diagnostic
             },
             ResolverError::LoopNotYetSupported { location  } => {

compiler/noirc_frontend/src/hir/type_check/errors.rs

@@ -63,7 +63,7 @@ pub enum TypeCheckError {
     #[error("Expected type {expected} is not the same as {actual}")]
     TypeMismatchWithSource { expected: Type, actual: Type, location: Location, source: Source },
     #[error("Expected type {expected_kind:?} is not the same as {expr_kind:?}")]
-    TypeKindMismatch { expected_kind: String, expr_kind: String, expr_location: Location },
+    TypeKindMismatch { expected_kind: Kind, expr_kind: Kind, expr_location: Location },
     #[error("Evaluating {to} resulted in {to_value}, but {from_value} was expected")]
     TypeCanonicalizationMismatch {
         to: Type,
@@ -369,11 +369,37 @@ impl<'a> From<&'a TypeCheckError> for Diagnostic {
                 )
             }
             TypeCheckError::TypeKindMismatch { expected_kind, expr_kind, expr_location } => {
-                Diagnostic::simple_error(
-                    format!("Expected kind {expected_kind}, found kind {expr_kind}"),
-                    String::new(),
-                    *expr_location,
-                )
+                // Try to improve the error message for some kind combinations
+                match (expected_kind, expr_kind) {
+                    (Kind::Normal, Kind::Numeric(_)) => {
+                        Diagnostic::simple_error(
+                            "Expected type, found numeric generic".into(),
+                            "not a type".into(),
+                            *expr_location,
+                        )
+                    }
+                    (Kind::Numeric(typ), Kind::Normal) => {
+                        Diagnostic::simple_error(
+                            "Type provided when a numeric generic was expected".into(),
+                            format!("the numeric generic is not of type `{typ}`"),
+                            *expr_location,
+                        )
+                    }
+                    (Kind::Numeric(expected_type), Kind::Numeric(found_type)) => {
+                        Diagnostic::simple_error(
+                            format!("The numeric generic is not of type `{expected_type}`"),
+                            format!("expected `{expected_type}`, found `{found_type}`"),
+                            *expr_location,
+                        )
+                    }
+                    _ => {
+                        Diagnostic::simple_error(
+                            format!("Expected kind {expected_kind}, found kind {expr_kind}"),
+                            String::new(),
+                            *expr_location,
+                        )
+                    }
+                }
             }
             TypeCheckError::TypeCanonicalizationMismatch { to, from, to_value, from_value, location } => {
                 Diagnostic::simple_error(

compiler/noirc_frontend/src/hir_def/types.rs

@@ -858,8 +858,8 @@
     ) -> Result<(), TypeCheckError> {
         if !binding.kind().unifies(kind) {
             return Err(TypeCheckError::TypeKindMismatch {
-                expected_kind: format!("{}", kind),
-                expr_kind: format!("{}", binding.kind()),
+                expected_kind: kind.clone(),
+                expr_kind: binding.kind(),
                 expr_location: location,
             });
         }
@@ -2144,8 +2144,8 @@
                     kind.ensure_value_fits(x, location)
                 } else {
                     Err(TypeCheckError::TypeKindMismatch {
-                        expected_kind: format!("{}", constant_kind),
-                        expr_kind: format!("{}", kind),
+                        expected_kind: constant_kind,
+                        expr_kind: kind.clone(),
                         expr_location: location,
                     })
                 }
@@ -2166,8 +2166,8 @@
                     op.function(lhs_value, rhs_value, &infix_kind, location)
                 } else {
                     Err(TypeCheckError::TypeKindMismatch {
-                        expected_kind: format!("{}", kind),
-                        expr_kind: format!("{}", infix_kind),
+                        expected_kind: kind.clone(),
+                        expr_kind: infix_kind,
                         expr_location: location,
                     })
                 }
@@ -2346,7 +2346,7 @@
         }
 
         let recur_on_binding = |id, replacement: &Type| {
             // Prevent recuring forever if there's a `T := T` binding
             if replacement.type_variable_id() == Some(id) {
                 replacement.clone()
             } else {
@@ -2432,7 +2432,7 @@
                 Type::Tuple(fields)
             }
             Type::Forall(typevars, typ) => {
                 // Trying to substitute_helper a variable de, substitute_bound_typevarsfined within a nested Forall
                 // is usually impossible and indicative of an error in the type checker somewhere.
                 for var in typevars {
                     assert!(!type_bindings.contains_key(&var.id()));
@@ -2598,7 +2598,7 @@
         }
     }
 
     /// Follow bindings if this is a type variable or generic to the first non-typevariable
     /// type. Unlike `follow_bindings`, this won't recursively follow any bindings on any
     /// fields or arguments of this type.
     pub fn follow_bindings_shallow(&self) -> Cow<Type> {
@@ -2623,7 +2623,7 @@
 
     /// Replace any `Type::NamedGeneric` in this type with a `Type::TypeVariable`
     /// using to the same inner `TypeVariable`. This is used during monomorphization
     /// to bind to named generics since they are unbindable during type checking.
     pub fn replace_named_generics_with_type_variables(&mut self) {
         match self {
             Type::FieldElement
@@ -3096,7 +3096,7 @@
                 len.hash(state);
                 env.hash(state);
             }
             Type::Tuple(elems) => elems.hash(state),
             Type::DataType(def, args) => {
                 def.hash(state);
                 args.hash(state);

compiler/noirc_frontend/src/node_interner.rs

@@ -216,12 +216,12 @@
     interned_statement_kinds: noirc_arena::Arena<StatementKind>,
 
     // Interned `UnresolvedTypeData`s during comptime code.
     interned_unresolved_type_datas: noirc_arena::Arena<UnresolvedTypeData>,
 
     // Interned `Pattern`s during comptime code.
     interned_patterns: noirc_arena::Arena<Pattern>,
 
     /// Determins whether to run in LSP mode. In LSP mode references are tracked.
     pub(crate) lsp_mode: bool,
 
     /// Store the location of the references in the graph.
@@ -697,7 +697,7 @@
             quoted_types: Default::default(),
             interned_expression_kinds: Default::default(),
             interned_statement_kinds: Default::default(),
             interned_unresolved_type_datas: Default::default(),
             interned_patterns: Default::default(),
             lsp_mode: false,
             location_indices: LocationIndices::default(),
@@ -765,7 +765,7 @@
             id: type_id,
             name: unresolved_trait.trait_def.name.clone(),
             crate_id: unresolved_trait.crate_id,
-            location: unresolved_trait.trait_def.location,
+            location: unresolved_trait.trait_def.name.location(),
             generics,
             visibility: ItemVisibility::Private,
             self_type_typevar: TypeVariable::unbound(self.next_type_variable_id(), Kind::Normal),
@@ -2180,7 +2180,7 @@
         &mut self,
         typ: UnresolvedTypeData,
     ) -> InternedUnresolvedTypeData {
         InternedUnresolvedTypeData(self.interned_unresolved_type_datas.insert(typ))
     }
 
     pub fn get_unresolved_type_data(&self, id: InternedUnresolvedTypeData) -> &UnresolvedTypeData {

compiler/noirc_frontend/src/parser/parser/function.rs

@@ -109,10 +109,24 @@ impl Parser<'_> {
             let visibility = self.parse_visibility();
             (FunctionReturnType::Ty(self.parse_type_or_error()), visibility)
         } else {
-            (
-                FunctionReturnType::Default(self.location_at_previous_token_end()),
-                Visibility::Private,
-            )
+            // This will return the span between `)` and `{`
+            //
+            // fn foo() { }
+            //        ^^^
+            let mut location = self.previous_token_location.merge(self.current_token_location);
+
+            // Here we change it to this (if there's space)
+            //
+            // fn foo() { }
+            //         ^
+            if location.span.end() - location.span.start() >= 3 {
+                location = Location::new(
+                    Span::from(location.span.start() + 1..location.span.end() - 1),
+                    location.file,
+                );
+            }
+
+            (FunctionReturnType::Default(location), Visibility::Private)
         };
 
         let where_clause = self.parse_where_clause();