Skip to content

Commit

Permalink
chore: Move check_method_signatures to type checking phase (#4516)
Browse files Browse the repository at this point in the history 
# Description

## Problem\*

Resolves #3583

## Summary\*

Resolves an old TODO by moving this function to the type checking phase.
I've rewritten the function since it was binding certain type variables
then unbound them afterward. I've changed that to substitute new
variables and only use `try_unify` to avoid accidentally binding
anything.

## Additional Context



## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[Exceptional Case]** 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.
  • Loading branch information
@jfecher
jfecher authored Mar 11, 2024
1 parent 810d195 commit 8d66677
Show file tree
Hide file tree
Showing 9 changed files with 239 additions and 205 deletions.
176 changes: 22 additions & 154 deletions compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,13 +5,14 @@ use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleId};
use crate::hir::resolution::errors::ResolverError;

use crate::hir::resolution::import::{resolve_import, ImportDirective};
use crate::hir::resolution::resolver::Resolver;
use crate::hir::resolution::{
collect_impls, collect_trait_impls, path_resolver, resolve_free_functions, resolve_globals,
resolve_impls, resolve_structs, resolve_trait_by_path, resolve_trait_impls, resolve_traits,
resolve_type_aliases,
};
use crate::hir::type_check::{type_check_func, TypeCheckError, TypeChecker};
use crate::hir::type_check::{
check_trait_impl_method_matches_declaration, type_check_func, TypeCheckError, TypeChecker,
};
use crate::hir::Context;

use crate::macros_api::{MacroError, MacroProcessor};
Expand All @@ -20,8 +21,7 @@ use crate::node_interner::{FuncId, GlobalId, NodeInterner, StructId, TraitId, Ty
use crate::parser::{ParserError, SortedModule};
use crate::{
ExpressionKind, Ident, LetStatement, Literal, NoirFunction, NoirStruct, NoirTrait,
NoirTypeAlias, Path, PathKind, Type, TypeBindings, UnresolvedGenerics,
UnresolvedTraitConstraint, UnresolvedType,
NoirTypeAlias, Path, PathKind, UnresolvedGenerics, UnresolvedTraitConstraint, UnresolvedType,
};
use fm::FileId;
use iter_extended::vecmap;
Expand Down Expand Up @@ -368,12 +368,12 @@ impl DefCollector {
&mut errors,
));

functions.extend(resolve_trait_impls(
let impl_functions = resolve_trait_impls(
context,
def_collector.collected_traits_impls,
crate_id,
&mut errors,
));
);

for macro_processor in macro_processors {
macro_processor.process_typed_ast(&crate_id, context).unwrap_or_else(
Expand All @@ -387,6 +387,8 @@ impl DefCollector {

errors.extend(type_check_globals(&mut context.def_interner, resolved_globals.globals));
errors.extend(type_check_functions(&mut context.def_interner, functions));
errors.extend(type_check_trait_impl_signatures(&mut context.def_interner, &impl_functions));
errors.extend(type_check_functions(&mut context.def_interner, impl_functions));
errors
}
}
Expand Down Expand Up @@ -468,158 +470,24 @@ fn type_check_functions(
.into_iter()
.flat_map(|(file, func)| {
type_check_func(interner, func)
.iter()
.cloned()
.into_iter()
.map(|e| (e.into(), file))
.collect::<Vec<_>>()
})
.collect()
}

// TODO(vitkov): Move this out of here and into type_check
#[allow(clippy::too_many_arguments)]
pub(crate) fn check_methods_signatures(
resolver: &mut Resolver,
impl_methods: &[(FileId, FuncId)],
trait_id: TraitId,
trait_name_span: Span,
// These are the generics on the trait itself from the impl.
// E.g. in `impl Foo<A, B> for Bar<B, C>`, this is `vec![A, B]`.
trait_generics: Vec<UnresolvedType>,
trait_impl_generic_count: usize,
file_id: FileId,
errors: &mut Vec<(CompilationError, FileId)>,
) {
let self_type = resolver.get_self_type().expect("trait impl must have a Self type").clone();
let trait_generics = vecmap(trait_generics, |typ| resolver.resolve_type(typ));

// Temporarily bind the trait's Self type to self_type so we can type check
let the_trait = resolver.interner.get_trait_mut(trait_id);
the_trait.self_type_typevar.bind(self_type);

if trait_generics.len() != the_trait.generics.len() {
let error = DefCollectorErrorKind::MismatchGenericCount {
actual_generic_count: trait_generics.len(),
expected_generic_count: the_trait.generics.len(),
// Preferring to use 'here' over a more precise term like 'this reference'
// to try to make the error easier to understand for newer users.
location: "here it",
origin: the_trait.name.to_string(),
span: trait_name_span,
};
errors.push((error.into(), file_id));
}

// We also need to bind the traits generics to the trait's generics on the impl
for (generic, binding) in the_trait.generics.iter().zip(trait_generics) {
generic.bind(binding);
}

// Temporarily take the trait's methods so we can use both them and a mutable reference
// to the interner within the loop.
let trait_methods = std::mem::take(&mut the_trait.methods);

for (file_id, func_id) in impl_methods {
let func_name = resolver.interner.function_name(func_id).to_owned();

// This is None in the case where the impl block has a method that's not part of the trait.
// If that's the case, a `MethodNotInTrait` error has already been thrown, and we can ignore
// the impl method, since there's nothing in the trait to match its signature against.
if let Some(trait_method) =
trait_methods.iter().find(|method| method.name.0.contents == func_name)
{
let impl_method = resolver.interner.function_meta(func_id);

let impl_method_generic_count =
impl_method.typ.generic_count() - trait_impl_generic_count;

// We subtract 1 here to account for the implicit generic `Self` type that is on all
// traits (and thus trait methods) but is not required (or allowed) for users to specify.
let the_trait = resolver.interner.get_trait(trait_id);
let trait_method_generic_count =
trait_method.generics().len() - 1 - the_trait.generics.len();

if impl_method_generic_count != trait_method_generic_count {
let trait_name = resolver.interner.get_trait(trait_id).name.clone();

let error = DefCollectorErrorKind::MismatchGenericCount {
actual_generic_count: impl_method_generic_count,
expected_generic_count: trait_method_generic_count,
origin: format!("{}::{}", trait_name, func_name),
location: "this method",
span: impl_method.location.span,
};
errors.push((error.into(), *file_id));
}

// This instantiation is technically not needed. We could bind each generic in the
// trait function to the impl's corresponding generic but to do so we'd have to rely
// on the trait function's generics being first in the generic list, since the same
// list also contains the generic `Self` variable, and any generics on the trait itself.
//
// Instantiating the impl method's generics here instead is a bit less precise but
// doesn't rely on any orderings that may be changed.
let impl_function_type = impl_method.typ.instantiate(resolver.interner).0;

let mut bindings = TypeBindings::new();
let mut typecheck_errors = Vec::new();

if let Type::Function(impl_params, impl_return, _) = impl_function_type.as_monotype() {
if trait_method.arguments().len() != impl_params.len() {
let error = DefCollectorErrorKind::MismatchTraitImplementationNumParameters {
actual_num_parameters: impl_method.parameters.0.len(),
expected_num_parameters: trait_method.arguments().len(),
trait_name: resolver.interner.get_trait(trait_id).name.to_string(),
method_name: func_name.to_string(),
span: impl_method.location.span,
};
errors.push((error.into(), *file_id));
}

// Check the parameters of the impl method against the parameters of the trait method
let args = trait_method.arguments().iter();
let args_and_params = args.zip(impl_params).zip(&impl_method.parameters.0);

for (parameter_index, ((expected, actual), (hir_pattern, _, _))) in
args_and_params.enumerate()
{
if expected.try_unify(actual, &mut bindings).is_err() {
typecheck_errors.push(TypeCheckError::TraitMethodParameterTypeMismatch {
method_name: func_name.to_string(),
expected_typ: expected.to_string(),
actual_typ: actual.to_string(),
parameter_span: hir_pattern.span(),
parameter_index: parameter_index + 1,
});
}
}

if trait_method.return_type().try_unify(impl_return, &mut bindings).is_err() {
let impl_method = resolver.interner.function_meta(func_id);
let ret_type_span = impl_method.return_type.get_type().span;
let expr_span = ret_type_span.expect("return type must always have a span");

let expected_typ = trait_method.return_type().to_string();
let expr_typ = impl_method.return_type().to_string();
let error = TypeCheckError::TypeMismatch { expr_typ, expected_typ, expr_span };
typecheck_errors.push(error);
}
} else {
unreachable!(
"impl_function_type is not a function type, it is: {impl_function_type}"
);
}

errors.extend(typecheck_errors.iter().cloned().map(|e| (e.into(), *file_id)));
}
}

// Now unbind `Self` and the trait's generics
let the_trait = resolver.interner.get_trait_mut(trait_id);
the_trait.set_methods(trait_methods);
the_trait.self_type_typevar.unbind(the_trait.self_type_typevar_id);

for generic in &the_trait.generics {
generic.unbind(generic.id());
}
fn type_check_trait_impl_signatures(
interner: &mut NodeInterner,
file_func_ids: &[(FileId, FuncId)],
) -> Vec<(CompilationError, fm::FileId)> {
file_func_ids
.iter()
.flat_map(|(file, func)| {
check_trait_impl_method_matches_declaration(interner, *func)
.into_iter()
.map(|e| (e.into(), *file))
.collect::<Vec<_>>()
})
.collect()
}
20 changes: 0 additions & 20 deletions compiler/noirc_frontend/src/hir/def_collector/errors.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,14 +41,6 @@ pub enum DefCollectorErrorKind {
OverlappingImplNote { span: Span },
#[error("Cannot `impl` a type defined outside the current crate")]
ForeignImpl { span: Span, type_name: String },
#[error("Mismatched number of parameters in trait implementation")]
MismatchTraitImplementationNumParameters {
actual_num_parameters: usize,
expected_num_parameters: usize,
trait_name: String,
method_name: String,
span: Span,
},
#[error("Mismatched number of generics in {location}")]
MismatchGenericCount {
actual_generic_count: usize,
Expand Down Expand Up @@ -176,18 +168,6 @@ impl From<DefCollectorErrorKind> for Diagnostic {
"".to_string(),
trait_path.span(),
),
DefCollectorErrorKind::MismatchTraitImplementationNumParameters {
expected_num_parameters,
actual_num_parameters,
trait_name,
method_name,
span,
} => {
let plural = if expected_num_parameters == 1 { "" } else { "s" };
let primary_message = format!(
"`{trait_name}::{method_name}` expects {expected_num_parameters} parameter{plural}, but this method has {actual_num_parameters}");
Diagnostic::simple_error(primary_message, "".to_string(), span)
}
DefCollectorErrorKind::MismatchGenericCount {
actual_generic_count,
expected_generic_count,
Expand Down
10 changes: 9 additions & 1 deletion compiler/noirc_frontend/src/hir/resolution/resolver.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,6 +217,7 @@ impl<'a> Resolver<'a> {
pub fn resolve_trait_function(
&mut self,
name: &Ident,
generics: &UnresolvedGenerics,
parameters: &[(Ident, UnresolvedType)],
return_type: &FunctionReturnType,
where_clause: &[UnresolvedTraitConstraint],
Expand All @@ -237,7 +238,7 @@ impl<'a> Resolver<'a> {
is_internal: false,
is_unconstrained: false,
visibility: FunctionVisibility::Public, // Trait functions are always public
generics: Vec::new(), // self.generics should already be set
generics: generics.clone(),
parameters: vecmap(parameters, |(name, typ)| Param {
visibility: Visibility::Private,
pattern: Pattern::Identifier(name.clone()),
Expand Down Expand Up @@ -975,11 +976,18 @@ impl<'a> Resolver<'a> {
self.handle_function_type(&func_id);
self.handle_is_function_internal(&func_id);

let direct_generics = func.def.generics.iter();
let direct_generics = direct_generics
.filter_map(|generic| self.find_generic(&generic.0.contents))
.map(|(name, typevar, _span)| (name.clone(), typevar.clone()))
.collect();

FuncMeta {
name: name_ident,
kind: func.kind,
location,
typ,
direct_generics,
trait_impl: self.current_trait_impl,
parameters: parameters.into(),
return_type: func.def.return_type.clone(),
Expand Down
17 changes: 3 additions & 14 deletions compiler/noirc_frontend/src/hir/resolution/traits.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,7 @@ use crate::{
graph::CrateId,
hir::{
def_collector::{
dc_crate::{
check_methods_signatures, CompilationError, UnresolvedTrait, UnresolvedTraitImpl,
},
dc_crate::{CompilationError, UnresolvedTrait, UnresolvedTraitImpl},
errors::{DefCollectorErrorKind, DuplicateType},
},
def_map::{CrateDefMap, ModuleDefId, ModuleId},
Expand Down Expand Up @@ -131,6 +129,7 @@ fn resolve_trait_methods(
let func_id = unresolved_trait.method_ids[&name.0.contents];
let (_, func_meta) = resolver.resolve_trait_function(
name,
generics,
parameters,
return_type,
where_clause,
Expand Down Expand Up @@ -365,6 +364,7 @@ pub(crate) fn resolve_trait_by_path(
Err(_) => Err(DefCollectorErrorKind::TraitNotFound { trait_path: path }),
}
}

pub(crate) fn resolve_trait_impls(
context: &mut Context,
traits: Vec<UnresolvedTraitImpl>,
Expand Down Expand Up @@ -424,17 +424,6 @@ pub(crate) fn resolve_trait_impls(
new_resolver.set_self_type(Some(self_type.clone()));

if let Some(trait_id) = maybe_trait_id {
check_methods_signatures(
&mut new_resolver,
&impl_methods,
trait_id,
trait_impl.trait_path.span(),
trait_impl.trait_generics,
trait_impl.generics.len(),
trait_impl.file_id,
errors,
);

let where_clause = trait_impl
.where_clause
.into_iter()
Expand Down
Loading

0 comments on commit 8d66677

Please sign in to comment.