Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Check where clauses when searching for trait impls #3407

Merged
merged 5 commits into from
Nov 2, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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::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 object_type: UnresolvedType,
pub methods: UnresolvedFunctions,
pub generics: UnresolvedGenerics,
pub where_clause: Vec<UnresolvedTraitConstraint>,
}

#[derive(Clone)]
Expand Down Expand Up @@ -919,6 +922,7 @@
def_maps,
functions,
Some(self_type.clone()),
None,
generics,
errors,
);
Expand Down Expand Up @@ -968,13 +972,16 @@
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 @@

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 @@
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 All @@ -1034,7 +1053,7 @@
methods
}

// TODO(vitkov): Move this out of here and into type_check

Check warning on line 1056 in compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (vitkov)
fn check_methods_signatures(
resolver: &mut Resolver,
impl_methods: &Vec<(FileId, FuncId)>,
Expand Down Expand Up @@ -1147,19 +1166,22 @@
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 @@
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 @@
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 Expand Up @@ -383,7 +384,7 @@
let modifiers = FunctionModifiers {
name: name.to_string(),
visibility: crate::FunctionVisibility::Public,
// TODO(Maddiaa): Investigate trait implementations with attributes see: https://github.com/noir-lang/noir/issues/2629

Check warning on line 387 in compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (Maddiaa)
attributes: crate::token::Attributes::empty(),
is_unconstrained: false,
contract_function_type: None,
Expand Down Expand Up @@ -439,7 +440,7 @@
}
}
TraitItem::Type { name } => {
// TODO(nickysn or alexvitkov): implement context.def_interner.push_empty_type_alias and get an id, instead of using TypeAliasId::dummy_id()

Check warning on line 443 in compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (nickysn)

Check warning on line 443 in compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (alexvitkov)
if let Err((first_def, second_def)) = self.def_collector.def_map.modules
[id.0.local_id.0]
.declare_type_alias(name.clone(), TypeAliasId::dummy_id())
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::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,9 +89,13 @@
/// 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

Check warning on line 98 in compiler/noirc_frontend/src/hir/resolution/resolver.rs

View workflow job for this annotation

GitHub Actions / Spellcheck / Spellcheck

Unknown word (overriden)
/// as if they're in the parent module, but should be placed in a child module.
/// Since they should be within a child module, in_contract is manually set to false
/// for these so we can still resolve them in the parent module without them being in a contract.
Expand Down Expand Up @@ -142,6 +147,7 @@
generics: Vec::new(),
errors: Vec::new(),
lambda_stack: Vec::new(),
current_trait_impl: None,
file,
in_contract,
}
Expand All @@ -155,6 +161,10 @@
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 @@
(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 @@
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
Loading