feat: Improve "type annotations needed" errors (noir-lang#5830)

# Description

## Problem\*

Resolves a common complaint that the "type annotations needed" error is
applied too broadly.

## Summary\*

I've completely removed the old error and split it into several cases:
- Cannot call a method (please add a type annotation for the object
type)
- Cannot access a struct field (please add a type annotation for the
object type)
- Couldn't determine type of generic arg (please add a type annotation)

Errors that were added but shouldn't occur in practice:
- Cannot find a trait impl (multiple matching impls)
- Cyclic type constructed
- Couldn't evaluate array length to concrete size
- Unspecified type

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

---------

Co-authored-by: Michael J Klein <[email protected]>

compiler/noirc_frontend/src/elaborator/statements.rs

@@ -419,7 +419,7 @@ impl<'context> Elaborator<'context> {
         // If we get here the type has no field named 'access.rhs'.
         // Now we specialize the error message based on whether we know the object type in question yet.
         if let Type::TypeVariable(..) = &lhs_type {
-            self.push_err(TypeCheckError::TypeAnnotationsNeeded { span });
+            self.push_err(TypeCheckError::TypeAnnotationsNeededForFieldAccess { span });
         } else if lhs_type != Type::Error {
             self.push_err(TypeCheckError::AccessUnknownMember {
                 lhs_type,

compiler/noirc_frontend/src/elaborator/types.rs

@@ -32,8 +32,8 @@ use crate::{
         SecondaryAttribute, Signedness, UnaryOp, UnresolvedType, UnresolvedTypeData,
     },
     node_interner::{
-        DefinitionKind, DependencyId, ExprId, FuncId, GlobalId, TraitId, TraitImplKind,
-        TraitMethodId,
+        DefinitionKind, DependencyId, ExprId, FuncId, GlobalId, ImplSearchErrorKind, TraitId,
+        TraitImplKind, TraitMethodId,
     },
     Generics, Kind, ResolvedGeneric, Type, TypeBinding, TypeBindings, TypeVariable,
     TypeVariableKind,
@@ -122,7 +122,7 @@ impl<'context> Elaborator<'context> {
             Unit => Type::Unit,
             Unspecified => {
                 let span = typ.span;
-                self.push_err(TypeCheckError::TypeAnnotationsNeeded { span });
+                self.push_err(TypeCheckError::UnspecifiedType { span });
                 Type::Error
             }
             Error => Type::Error,
@@ -482,7 +482,7 @@ impl<'context> Elaborator<'context> {
         match self.interner.lookup_trait_implementation(&object_type, trait_id, &ordered, &named) {
             Ok(impl_kind) => self.get_associated_type_from_trait_impl(path, impl_kind),
             Err(constraints) => {
-                self.push_trait_constraint_error(constraints, span);
+                self.push_trait_constraint_error(&object_type, constraints, span);
                 Type::Error
             }
         }
@@ -1332,7 +1332,7 @@ impl<'context> Elaborator<'context> {
 
             // The type variable must be unbound at this point since follow_bindings was called
             Type::TypeVariable(_, TypeVariableKind::Normal) => {
-                self.push_err(TypeCheckError::TypeAnnotationsNeeded { span });
+                self.push_err(TypeCheckError::TypeAnnotationsNeededForMethodCall { span });
                 None
             }
 
@@ -1596,16 +1596,34 @@ impl<'context> Elaborator<'context> {
             Ok(impl_kind) => {
                 self.interner.select_impl_for_expression(function_ident_id, impl_kind);
             }
-            Err(constraints) => self.push_trait_constraint_error(constraints, span),
+            Err(error) => self.push_trait_constraint_error(object_type, error, span),
         }
     }
 
-    fn push_trait_constraint_error(&mut self, constraints: Vec<TraitConstraint>, span: Span) {
-        if constraints.is_empty() {
-            self.push_err(TypeCheckError::TypeAnnotationsNeeded { span });
-        } else if let Some(error) = NoMatchingImplFoundError::new(self.interner, constraints, span)
-        {
-            self.push_err(TypeCheckError::NoMatchingImplFound(error));
+    fn push_trait_constraint_error(
+        &mut self,
+        object_type: &Type,
+        error: ImplSearchErrorKind,
+        span: Span,
+    ) {
+        match error {
+            ImplSearchErrorKind::TypeAnnotationsNeededOnObjectType => {
+                self.push_err(TypeCheckError::TypeAnnotationsNeededForMethodCall { span });
+            }
+            ImplSearchErrorKind::Nested(constraints) => {
+                if let Some(error) = NoMatchingImplFoundError::new(self.interner, constraints, span)
+                {
+                    self.push_err(TypeCheckError::NoMatchingImplFound(error));
+                }
+            }
+            ImplSearchErrorKind::MultipleMatching(candidates) => {
+                let object_type = object_type.clone();
+                self.push_err(TypeCheckError::MultipleMatchingImpls {
+                    object_type,
+                    span,
+                    candidates,
+                });
+            }
         }
     }
 

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

@@ -188,11 +188,19 @@ pub enum InterpreterError {
     FunctionAlreadyResolved {
         location: Location,
     },
+    MultipleMatchingImpls {
+        object_type: Type,
+        candidates: Vec<String>,
+        location: Location,
+    },
 
     Unimplemented {
         item: String,
         location: Location,
     },
+    TypeAnnotationsNeededForMethodCall {
+        location: Location,
+    },
 
     // These cases are not errors, they are just used to prevent us from running more code
     // until the loop can be resumed properly. These cases will never be displayed to users.
@@ -257,8 +265,10 @@ impl InterpreterError {
             | InterpreterError::ContinueNotInLoop { location, .. }
             | InterpreterError::TraitDefinitionMustBeAPath { location }
             | InterpreterError::FailedToResolveTraitDefinition { location }
-            | InterpreterError::FailedToResolveTraitBound { location, .. } => *location,
-            InterpreterError::FunctionAlreadyResolved { location, .. } => *location,
+            | InterpreterError::FailedToResolveTraitBound { location, .. }
+            | InterpreterError::FunctionAlreadyResolved { location, .. }
+            | InterpreterError::MultipleMatchingImpls { location, .. }
+            | InterpreterError::TypeAnnotationsNeededForMethodCall { location } => *location,
 
             InterpreterError::FailedToParseMacro { error, file, .. } => {
                 Location::new(error.span(), *file)
@@ -527,6 +537,26 @@ impl<'a> From<&'a InterpreterError> for CustomDiagnostic {
                         .to_string();
                 CustomDiagnostic::simple_error(msg, secondary, location.span)
             }
+            InterpreterError::MultipleMatchingImpls { object_type, candidates, location } => {
+                let message = format!("Multiple trait impls match the object type `{object_type}`");
+                let secondary = "Ambiguous impl".to_string();
+                let mut error = CustomDiagnostic::simple_error(message, secondary, location.span);
+                for (i, candidate) in candidates.iter().enumerate() {
+                    error.add_note(format!("Candidate {}: `{candidate}`", i + 1));
+                }
+                error
+            }
+            InterpreterError::TypeAnnotationsNeededForMethodCall { location } => {
+                let mut error = CustomDiagnostic::simple_error(
+                    "Object type is unknown in method call".to_string(),
+                    "Type must be known by this point to know which method to call".to_string(),
+                    location.span,
+                );
+                let message =
+                    "Try adding a type annotation for the object type before this method call";
+                error.add_note(message.to_string());
+                error
+            }
         }
     }
 }

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

@@ -104,8 +104,12 @@ pub enum TypeCheckError {
         second_type: String,
         second_index: usize,
     },
-    #[error("Cannot infer type of expression, type annotations needed before this point")]
-    TypeAnnotationsNeeded { span: Span },
+    #[error("Object type is unknown in method call")]
+    TypeAnnotationsNeededForMethodCall { span: Span },
+    #[error("Object type is unknown in field access")]
+    TypeAnnotationsNeededForFieldAccess { span: Span },
+    #[error("Multiple trait impls may apply to this object type")]
+    MultipleMatchingImpls { object_type: Type, candidates: Vec<String>, span: Span },
     #[error("use of deprecated function {name}")]
     CallDeprecated { name: String, note: Option<String>, span: Span },
     #[error("{0}")]
@@ -166,6 +170,10 @@ pub enum TypeCheckError {
     NoSuchNamedTypeArg { name: Ident, item: String },
     #[error("`{item}` is missing the associated type `{name}`")]
     MissingNamedTypeArg { name: Rc<String>, item: String, span: Span },
+    #[error("Internal compiler error: type unspecified for value")]
+    UnspecifiedType { span: Span },
+    #[error("Binding `{typ}` here to the `_` inside would create a cyclic type")]
+    CyclicType { typ: Type, span: Span },
 }
 
 #[derive(Debug, Clone, PartialEq, Eq)]
@@ -286,11 +294,33 @@ impl<'a> From<&'a TypeCheckError> for Diagnostic {
                 format!("return type is {typ}"),
                 *span,
             ),
-            TypeCheckError::TypeAnnotationsNeeded { span } => Diagnostic::simple_error(
-                "Expression type is ambiguous".to_string(),
-                "Type must be known at this point".to_string(),
-                *span,
-            ),
+            TypeCheckError::TypeAnnotationsNeededForMethodCall { span } => {
+                let mut error = Diagnostic::simple_error(
+                    "Object type is unknown in method call".to_string(),
+                    "Type must be known by this point to know which method to call".to_string(),
+                    *span,
+                );
+                error.add_note("Try adding a type annotation for the object type before this method call".to_string());
+                error
+            },
+            TypeCheckError::TypeAnnotationsNeededForFieldAccess { span } => {
+                let mut error = Diagnostic::simple_error(
+                    "Object type is unknown in field access".to_string(),
+                    "Type must be known by this point".to_string(),
+                    *span,
+                );
+                error.add_note("Try adding a type annotation for the object type before this expression".to_string());
+                error
+            },
+            TypeCheckError::MultipleMatchingImpls { object_type, candidates, span } => {
+                let message = format!("Multiple trait impls match the object type `{object_type}`");
+                let secondary = "Ambiguous impl".to_string();
+                let mut error = Diagnostic::simple_error(message, secondary, *span);
+                for (i, candidate) in candidates.iter().enumerate() {
+                    error.add_note(format!("Candidate {}: `{candidate}`", i + 1));
+                }
+                error
+            },
             TypeCheckError::ResolverError(error) => error.into(),
             TypeCheckError::TypeMismatchWithSource { expected, actual, span, source } => {
                 let message = match source {
@@ -388,6 +418,12 @@ impl<'a> From<&'a TypeCheckError> for Diagnostic {
             TypeCheckError::UnsafeFn { span } => {
                 Diagnostic::simple_warning(error.to_string(), String::new(), *span)
             }
+            TypeCheckError::UnspecifiedType { span } => {
+                Diagnostic::simple_error(error.to_string(), String::new(), *span)
+            }
+            TypeCheckError::CyclicType { typ: _, span } => {
+                Diagnostic::simple_error(error.to_string(), "Cyclic types have unlimited size and are prohibited in Noir".into(), *span)
+            }
         }
     }
 }

compiler/noirc_frontend/src/hir_def/types.rs

@@ -544,7 +544,7 @@ impl TypeVariable {
         };
 
         if binding.occurs(id) {
-            Err(TypeCheckError::TypeAnnotationsNeeded { span })
+            Err(TypeCheckError::CyclicType { span, typ: binding })
         } else {
             *self.1.borrow_mut() = TypeBinding::Bound(binding);
             Ok(())

compiler/noirc_frontend/src/monomorphization/errors.rs

@@ -1,21 +1,21 @@
 use noirc_errors::{CustomDiagnostic, FileDiagnostic, Location};
 
-use crate::hir::comptime::InterpreterError;
+use crate::{hir::comptime::InterpreterError, Type};
 
 #[derive(Debug)]
 pub enum MonomorphizationError {
-    UnknownArrayLength { location: Location },
-    TypeAnnotationsNeeded { location: Location },
+    UnknownArrayLength { length: Type, location: Location },
+    NoDefaultType { location: Location },
     InternalError { message: &'static str, location: Location },
     InterpreterError(InterpreterError),
 }
 
 impl MonomorphizationError {
     fn location(&self) -> Location {
         match self {
-            MonomorphizationError::UnknownArrayLength { location }
+            MonomorphizationError::UnknownArrayLength { location, .. }
             | MonomorphizationError::InternalError { location, .. }
-            | MonomorphizationError::TypeAnnotationsNeeded { location } => *location,
+            | MonomorphizationError::NoDefaultType { location, .. } => *location,
             MonomorphizationError::InterpreterError(error) => error.get_location(),
         }
     }
@@ -32,16 +32,20 @@ impl From<MonomorphizationError> for FileDiagnostic {
 
 impl MonomorphizationError {
     fn into_diagnostic(self) -> CustomDiagnostic {
-        let message = match self {
-            MonomorphizationError::UnknownArrayLength { .. } => {
-                "Length of generic array could not be determined."
+        let message = match &self {
+            MonomorphizationError::UnknownArrayLength { length, .. } => {
+                format!("ICE: Could not determine array length `{length}`")
             }
-            MonomorphizationError::TypeAnnotationsNeeded { .. } => "Type annotations needed",
-            MonomorphizationError::InterpreterError(error) => return (&error).into(),
-            MonomorphizationError::InternalError { message, .. } => message,
+            MonomorphizationError::NoDefaultType { location } => {
+                let message = "Type annotation needed".into();
+                let secondary = "Could not determine type of generic argument".into();
+                return CustomDiagnostic::simple_error(message, secondary, location.span);
+            }
+            MonomorphizationError::InterpreterError(error) => return error.into(),
+            MonomorphizationError::InternalError { message, .. } => message.to_string(),
         };
 
         let location = self.location();
-        CustomDiagnostic::simple_error(message.into(), String::new(), location.span)
+        CustomDiagnostic::simple_error(message, String::new(), location.span)
     }
 }

compiler/noirc_frontend/src/monomorphization/mod.rs

@@ -11,7 +11,7 @@
 use crate::ast::{FunctionKind, IntegerBitSize, Signedness, UnaryOp, Visibility};
 use crate::hir::comptime::InterpreterError;
 use crate::hir::type_check::NoMatchingImplFoundError;
-use crate::node_interner::ExprId;
+use crate::node_interner::{ExprId, ImplSearchErrorKind};
 use crate::{
     debug::DebugInstrumenter,
     hir_def::{
@@ -569,7 +569,7 @@ impl<'interner> Monomorphizer<'interner> {
 
         let length = length.evaluate_to_u32().ok_or_else(|| {
             let location = self.interner.expr_location(&array);
-            MonomorphizationError::UnknownArrayLength { location }
+            MonomorphizationError::UnknownArrayLength { location, length }
         })?;
 
         let contents = try_vecmap(0..length, |_| self.expr(repeated_element))?;
@@ -936,7 +936,10 @@ impl<'interner> Monomorphizer<'interner> {
                 let element = Box::new(Self::convert_type(element.as_ref(), location)?);
                 let length = match length.evaluate_to_u32() {
                     Some(length) => length,
-                    None => return Err(MonomorphizationError::TypeAnnotationsNeeded { location }),
+                    None => {
+                        let length = length.as_ref().clone();
+                        return Err(MonomorphizationError::UnknownArrayLength { location, length });
+                    }
                 };
                 ast::Type::Array(length, element)
             }
@@ -969,7 +972,7 @@ impl<'interner> Monomorphizer<'interner> {
                 // and within a larger generic type.
                 let default = match kind.default_type() {
                     Some(typ) => typ,
-                    None => return Err(MonomorphizationError::TypeAnnotationsNeeded { location }),
+                    None => return Err(MonomorphizationError::NoDefaultType { location }),
                 };
 
                 let monomorphized_default = Self::convert_type(&default, location)?;
@@ -1074,7 +1077,7 @@ impl<'interner> Monomorphizer<'interner> {
                 // and within a larger generic type.
                 let default = match kind.default_type() {
                     Some(typ) => typ,
-                    None => return Err(MonomorphizationError::TypeAnnotationsNeeded { location }),
+                    None => return Err(MonomorphizationError::NoDefaultType { location }),
                 };
 
                 Self::check_type(&default, location)
@@ -1946,6 +1949,7 @@ pub fn resolve_trait_method(
     let impl_id = match trait_impl {
         TraitImplKind::Normal(impl_id) => impl_id,
         TraitImplKind::Assumed { object_type, trait_generics } => {
+            let location = interner.expr_location(&expr_id);
             match interner.lookup_trait_implementation(
                 &object_type,
                 method.trait_id,
@@ -1954,21 +1958,28 @@ pub fn resolve_trait_method(
             ) {
                 Ok(TraitImplKind::Normal(impl_id)) => impl_id,
                 Ok(TraitImplKind::Assumed { .. }) => {
-                    let location = interner.expr_location(&expr_id);
                     return Err(InterpreterError::NoImpl { location });
                 }
-                Err(constraints) => {
-                    let location = interner.expr_location(&expr_id);
+                Err(ImplSearchErrorKind::TypeAnnotationsNeededOnObjectType) => {
+                    return Err(InterpreterError::TypeAnnotationsNeededForMethodCall { location });
+                }
+                Err(ImplSearchErrorKind::Nested(constraints)) => {
                     if let Some(error) =
                         NoMatchingImplFoundError::new(interner, constraints, location.span)
                     {
                         let file = location.file;
                         return Err(InterpreterError::NoMatchingImplFound { error, file });
                     } else {
-                        let location = interner.expr_location(&expr_id);
                         return Err(InterpreterError::NoImpl { location });
                     }
                 }
+                Err(ImplSearchErrorKind::MultipleMatching(candidates)) => {
+                    return Err(InterpreterError::MultipleMatchingImpls {
+                        object_type,
+                        location,
+                        candidates,
+                    });
+                }
             }
         }
     };