Skip to content

Commit

Permalink
feat: Check where clauses when searching for trait impls (#3407)
Browse files Browse the repository at this point in the history
  • Loading branch information
jfecher authored and TomAFrench committed Nov 14, 2023
1 parent 2f45b29 commit 78735ba
Show file tree
Hide file tree
Showing 15 changed files with 254 additions and 65 deletions.
35 changes: 29 additions & 6 deletions compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,15 @@ use crate::hir::resolution::{
use crate::hir::type_check::{type_check_func, TypeCheckError, TypeChecker};
use crate::hir::Context;
use crate::hir_def::traits::{Trait, TraitConstant, TraitFunction, TraitImpl, TraitType};
use crate::node_interner::{FuncId, NodeInterner, StmtId, StructId, TraitId, TypeAliasId};
use crate::node_interner::{
FuncId, NodeInterner, StmtId, StructId, TraitId, TraitImplId, TypeAliasId,
};

use crate::parser::{ParserError, SortedModule};
use crate::{
ExpressionKind, Generics, Ident, LetStatement, Literal, NoirFunction, NoirStruct, NoirTrait,
NoirTypeAlias, Path, Shared, StructType, TraitItem, Type, TypeBinding, TypeVariableKind,
UnresolvedGenerics, UnresolvedType,
UnresolvedGenerics, UnresolvedTraitConstraint, UnresolvedType,
};
use fm::FileId;
use iter_extended::vecmap;
Expand Down Expand Up @@ -91,6 +93,7 @@ pub struct UnresolvedTraitImpl {
pub object_type: UnresolvedType,
pub methods: UnresolvedFunctions,
pub generics: UnresolvedGenerics,
pub where_clause: Vec<UnresolvedTraitConstraint>,
}

#[derive(Clone)]
Expand Down Expand Up @@ -919,6 +922,7 @@ fn resolve_impls(
def_maps,
functions,
Some(self_type.clone()),
None,
generics,
errors,
);
Expand Down Expand Up @@ -968,13 +972,16 @@ fn resolve_trait_impls(
let self_type = resolver.resolve_type(unresolved_type.clone());
let generics = resolver.get_generics().to_vec();

let impl_id = interner.next_trait_impl_id();

let mut impl_methods = resolve_function_set(
interner,
crate_id,
&context.def_maps,
trait_impl.methods.clone(),
Some(self_type.clone()),
generics,
Some(impl_id),
generics.clone(),
errors,
);

Expand All @@ -993,6 +1000,8 @@ fn resolve_trait_impls(

let mut new_resolver =
Resolver::new(interner, &path_resolver, &context.def_maps, trait_impl.file_id);

new_resolver.set_generics(generics);
new_resolver.set_self_type(Some(self_type.clone()));

if let Some(trait_id) = maybe_trait_id {
Expand All @@ -1004,17 +1013,27 @@ fn resolve_trait_impls(
errors,
);

let where_clause = trait_impl
.where_clause
.into_iter()
.flat_map(|item| new_resolver.resolve_trait_constraint(item))
.collect();

let resolved_trait_impl = Shared::new(TraitImpl {
ident: trait_impl.trait_path.last_segment().clone(),
typ: self_type.clone(),
trait_id,
file: trait_impl.file_id,
where_clause,
methods: vecmap(&impl_methods, |(_, func_id)| *func_id),
});

if let Some((prev_span, prev_file)) =
interner.add_trait_implementation(self_type.clone(), trait_id, resolved_trait_impl)
{
if let Err((prev_span, prev_file)) = interner.add_trait_implementation(
self_type.clone(),
trait_id,
impl_id,
resolved_trait_impl,
) {
let error = DefCollectorErrorKind::OverlappingImpl {
typ: self_type.clone(),
span: self_type_span.unwrap_or_else(|| trait_impl.trait_path.span()),
Expand Down Expand Up @@ -1147,19 +1166,22 @@ fn resolve_free_functions(
def_maps,
unresolved_functions,
self_type.clone(),
None,
vec![], // no impl generics
errors,
)
})
.collect()
}

#[allow(clippy::too_many_arguments)]
fn resolve_function_set(
interner: &mut NodeInterner,
crate_id: CrateId,
def_maps: &BTreeMap<CrateId, CrateDefMap>,
mut unresolved_functions: UnresolvedFunctions,
self_type: Option<Type>,
trait_impl_id: Option<TraitImplId>,
impl_generics: Vec<(Rc<String>, Shared<TypeBinding>, Span)>,
errors: &mut Vec<(CompilationError, FileId)>,
) -> Vec<(FileId, FuncId)> {
Expand All @@ -1180,6 +1202,7 @@ fn resolve_function_set(
resolver.set_generics(impl_generics.clone());
resolver.set_self_type(self_type.clone());
resolver.set_trait_id(unresolved_functions.trait_id);
resolver.set_trait_impl_id(trait_impl_id);

// Without this, impl methods can accidentally be placed in contracts. See #3254
if self_type.is_some() {
Expand Down
1 change: 1 addition & 0 deletions compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,7 @@ impl<'a> ModCollector<'a> {
methods: unresolved_functions,
object_type: trait_impl.object_type,
generics: trait_impl.impl_generics,
where_clause: trait_impl.where_clause,
trait_id: None, // will be filled later
};

Expand Down
20 changes: 20 additions & 0 deletions compiler/noirc_frontend/src/hir/resolution/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ use crate::hir::def_map::{LocalModuleId, ModuleDefId, TryFromModuleDefId, MAIN_F
use crate::hir_def::stmt::{HirAssignStatement, HirForStatement, HirLValue, HirPattern};
use crate::node_interner::{
DefinitionId, DefinitionKind, ExprId, FuncId, NodeInterner, StmtId, StructId, TraitId,
TraitImplId,
};
use crate::{
hir::{def_map::CrateDefMap, resolution::path_resolver::PathResolver},
Expand Down Expand Up @@ -88,6 +89,10 @@ pub struct Resolver<'a> {
/// Set to the current type if we're resolving an impl
self_type: Option<Type>,

/// If we're currently resolving methods within a trait impl, this will be set
/// to the corresponding trait impl ID.
current_trait_impl: Option<TraitImplId>,

/// True if the current module is a contract.
/// This is usually determined by self.path_resolver.module_id(), but it can
/// be overriden for impls. Impls are an odd case since the methods within resolve
Expand Down Expand Up @@ -142,6 +147,7 @@ impl<'a> Resolver<'a> {
generics: Vec::new(),
errors: Vec::new(),
lambda_stack: Vec::new(),
current_trait_impl: None,
file,
in_contract,
}
Expand All @@ -155,6 +161,10 @@ impl<'a> Resolver<'a> {
self.trait_id = trait_id;
}

pub fn set_trait_impl_id(&mut self, impl_id: Option<TraitImplId>) {
self.current_trait_impl = impl_id;
}

pub fn get_self_type(&mut self) -> Option<&Type> {
self.self_type.as_ref()
}
Expand Down Expand Up @@ -357,6 +367,15 @@ impl<'a> Resolver<'a> {
(hir_func, func_meta)
}

pub fn resolve_trait_constraint(
&mut self,
constraint: UnresolvedTraitConstraint,
) -> Option<TraitConstraint> {
let typ = self.resolve_type(constraint.typ);
let trait_id = self.lookup_trait_or_error(constraint.trait_bound.trait_path)?.id;
Some(TraitConstraint { typ, trait_id })
}

/// Translates an UnresolvedType into a Type and appends any
/// freshly created TypeVariables created to new_variables.
fn resolve_type_inner(&mut self, typ: UnresolvedType, new_variables: &mut Generics) -> Type {
Expand Down Expand Up @@ -809,6 +828,7 @@ impl<'a> Resolver<'a> {
kind: func.kind,
location,
typ,
trait_impl: self.current_trait_impl,
parameters: parameters.into(),
return_type: func.def.return_type.clone(),
return_visibility: func.def.return_visibility,
Expand Down
23 changes: 18 additions & 5 deletions compiler/noirc_frontend/src/hir/type_check/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@ pub enum TypeCheckError {
parameter_span: Span,
parameter_index: usize,
},
#[error("No matching impl found")]
NoMatchingImplFound { constraints: Vec<(Type, String)>, span: Span },
}

impl TypeCheckError {
Expand Down Expand Up @@ -250,11 +252,22 @@ impl From<TypeCheckError> for Diagnostic {
Diagnostic::simple_warning(primary_message, secondary_message, span)
}
TypeCheckError::UnusedResultError { expr_type, expr_span } => {
Diagnostic::simple_warning(
format!("Unused expression result of type {expr_type}"),
String::new(),
expr_span,
)
let msg = format!("Unused expression result of type {expr_type}");
Diagnostic::simple_warning(msg, String::new(), expr_span)
}
TypeCheckError::NoMatchingImplFound { constraints, span } => {
assert!(!constraints.is_empty());
let msg = format!("No matching impl found for `{}: {}`", constraints[0].0, constraints[0].1);
let mut diagnostic = Diagnostic::from_message(&msg);

diagnostic.add_secondary(format!("No impl for `{}: {}`", constraints[0].0, constraints[0].1), span);

// These must be notes since secondaries are unordered
for (typ, trait_name) in &constraints[1..] {
diagnostic.add_note(format!("Required by `{typ}: {trait_name}`"));
}

diagnostic
}
}
}
Expand Down
36 changes: 32 additions & 4 deletions compiler/noirc_frontend/src/hir/type_check/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -146,7 +146,7 @@ impl<'interner> TypeChecker<'interner> {
match self.lookup_method(&object_type, method_name, expr_id) {
Some(method_ref) => {
let mut args = vec![(
object_type,
object_type.clone(),
method_call.object,
self.interner.expr_span(&method_call.object),
)];
Expand All @@ -160,11 +160,14 @@ impl<'interner> TypeChecker<'interner> {
// so that the backend doesn't need to worry about methods
let location = method_call.location;

if let HirMethodReference::FuncId(func_id) = method_ref {
let mut func_id = None;
if let HirMethodReference::FuncId(id) = method_ref {
func_id = Some(id);

// Automatically add `&mut` if the method expects a mutable reference and
// the object is not already one.
if func_id != FuncId::dummy_id() {
let func_meta = self.interner.function_meta(&func_id);
if id != FuncId::dummy_id() {
let func_meta = self.interner.function_meta(&id);
self.try_add_mutable_reference_to_object(
&mut method_call,
&func_meta.typ,
Expand All @@ -182,6 +185,31 @@ impl<'interner> TypeChecker<'interner> {
let span = self.interner.expr_span(expr_id);
let ret = self.check_method_call(&function_id, method_ref, args, span);

if let Some(func_id) = func_id {
let meta = self.interner.function_meta(&func_id);

if let Some(impl_id) = meta.trait_impl {
let trait_impl = self.interner.get_trait_implementation(impl_id);

let result = self.interner.lookup_trait_implementation(
&object_type,
trait_impl.borrow().trait_id,
);

if let Err(erroring_constraints) = result {
let constraints = vecmap(erroring_constraints, |constraint| {
let r#trait = self.interner.get_trait(constraint.trait_id);
(constraint.typ, r#trait.name.to_string())
});

self.errors.push(TypeCheckError::NoMatchingImplFound {
constraints,
span,
});
}
}
}

self.interner.replace_expr(expr_id, function_call);
ret
}
Expand Down
8 changes: 3 additions & 5 deletions compiler/noirc_frontend/src/hir/type_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,10 @@ pub fn type_check_func(interner: &mut NodeInterner, func_id: FuncId) -> Vec<Type
let (expr_span, empty_function) = function_info(interner, function_body_id);
let func_span = interner.expr_span(function_body_id); // XXX: We could be more specific and return the span of the last stmt, however stmts do not have spans yet
if let Type::TraitAsType(t) = &declared_return_type {
if interner
.lookup_trait_implementation(function_last_type.follow_bindings(), t.id)
.is_none()
{
if interner.lookup_trait_implementation(&function_last_type, t.id).is_err() {
let error = TypeCheckError::TypeMismatchWithSource {
expected: declared_return_type.clone(),
actual: function_last_type.clone(),
actual: function_last_type,
span: func_span,
source: Source::Return(meta.return_type, expr_span),
};
Expand Down Expand Up @@ -287,6 +284,7 @@ mod test {
return_visibility: Visibility::Private,
return_distinctness: Distinctness::DuplicationAllowed,
has_body: true,
trait_impl: None,
return_type: FunctionReturnType::Default(Span::default()),
trait_constraints: Vec::new(),
};
Expand Down
5 changes: 4 additions & 1 deletion compiler/noirc_frontend/src/hir_def/function.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use noirc_errors::{Location, Span};
use super::expr::{HirBlockExpression, HirExpression, HirIdent};
use super::stmt::HirPattern;
use super::traits::TraitConstraint;
use crate::node_interner::{ExprId, NodeInterner};
use crate::node_interner::{ExprId, NodeInterner, TraitImplId};
use crate::FunctionKind;
use crate::{Distinctness, FunctionReturnType, Type, Visibility};

Expand Down Expand Up @@ -114,6 +114,9 @@ pub struct FuncMeta {
pub has_body: bool,

pub trait_constraints: Vec<TraitConstraint>,

/// The trait impl this function belongs to, if any
pub trait_impl: Option<TraitImplId>,
}

impl FuncMeta {
Expand Down
12 changes: 12 additions & 0 deletions compiler/noirc_frontend/src/hir_def/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,12 @@ pub struct TraitImpl {
pub trait_id: TraitId,
pub file: FileId,
pub methods: Vec<FuncId>, // methods[i] is the implementation of trait.methods[i] for Type typ

/// The where clause, if present, contains each trait requirement which must
/// be satisfied for this impl to be selected. E.g. in `impl Eq for [T] where T: Eq`,
/// `where_clause` would contain the one `T: Eq` constraint. If there is no where clause,
/// this Vec is empty.
pub where_clause: Vec<TraitConstraint>,
}

#[derive(Debug, Clone)]
Expand All @@ -76,6 +82,12 @@ pub struct TraitConstraint {
// pub trait_generics: Generics, TODO
}

impl TraitConstraint {
pub fn new(typ: Type, trait_id: TraitId) -> Self {
Self { typ, trait_id }
}
}

impl std::hash::Hash for Trait {
fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
self.id.hash(state);
Expand Down
7 changes: 5 additions & 2 deletions compiler/noirc_frontend/src/hir_def/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1085,10 +1085,13 @@ impl Type {
}

/// Replace each NamedGeneric (and TypeVariable) in this type with a fresh type variable
pub(crate) fn instantiate_named_generics(&self, interner: &NodeInterner) -> Type {
pub(crate) fn instantiate_named_generics(
&self,
interner: &NodeInterner,
) -> (Type, TypeBindings) {
let mut substitutions = HashMap::new();
self.find_all_unbound_type_variables(interner, &mut substitutions);
self.substitute(&substitutions)
(self.substitute(&substitutions), substitutions)
}

/// For each unbound type variable in the current type, add a type binding to the given list
Expand Down
2 changes: 1 addition & 1 deletion compiler/noirc_frontend/src/monomorphization/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -820,7 +820,7 @@ impl<'interner> Monomorphizer<'interner> {

let trait_impl = self
.interner
.lookup_trait_implementation(self_type.follow_bindings(), method.trait_id)
.lookup_trait_implementation(&self_type, method.trait_id)
.expect("ICE: missing trait impl - should be caught during type checking");

let hir_func_id = trait_impl.borrow().methods[method.method_index];
Expand Down
Loading

0 comments on commit 78735ba

Please sign in to comment.