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

fix: "Missing trait impl" error in trait dispatch #3440

Merged
merged 8 commits into from
Nov 9, 2023
Merged
Show file tree
Hide file tree
Changes from 6 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
28 changes: 20 additions & 8 deletions compiler/noirc_frontend/src/hir/resolution/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +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,
TraitImplId, TraitImplKind,
};
use crate::{
hir::{def_map::CrateDefMap, resolution::path_resolver::PathResolver},
Expand Down Expand Up @@ -1207,8 +1207,12 @@ impl<'a> Resolver<'a> {
Literal::Unit => HirLiteral::Unit,
}),
ExpressionKind::Variable(path) => {
if let Some(expr) = self.resolve_trait_generic_path(&path) {
expr
if let Some((hir_expr, object_type)) = self.resolve_trait_generic_path(&path) {
let expr_id = self.interner.push_expr(hir_expr);
self.interner.push_expr_location(expr_id, expr.span, self.file);
self.interner
.select_impl_for_ident(expr_id, TraitImplKind::Assumed { object_type });
return expr_id;
} else {
// If the Path is being used as an Expression, then it is referring to a global from a separate module
// Otherwise, then it is referring to an Identifier
Expand Down Expand Up @@ -1370,6 +1374,8 @@ impl<'a> Resolver<'a> {
ExpressionKind::Parenthesized(sub_expr) => return self.resolve_expression(*sub_expr),
};

// If these lines are ever changed, make sure to change the early return
// in the ExpressionKind::Variable case as well
let expr_id = self.interner.push_expr(hir_expr);
self.interner.push_expr_location(expr_id, expr.span, self.file);
expr_id
Expand Down Expand Up @@ -1576,7 +1582,10 @@ impl<'a> Resolver<'a> {
}

// this resolves Self::some_static_method, inside an impl block (where we don't have a concrete self_type)
fn resolve_trait_static_method_by_self(&mut self, path: &Path) -> Option<HirExpression> {
fn resolve_trait_static_method_by_self(
&mut self,
path: &Path,
) -> Option<(HirExpression, Type)> {
if let Some(trait_id) = self.trait_id {
if path.kind == PathKind::Plain && path.segments.len() == 2 {
let name = &path.segments[0].0.contents;
Expand All @@ -1590,7 +1599,7 @@ impl<'a> Resolver<'a> {
the_trait.self_type_typevar,
crate::TypeVariableKind::Normal,
);
return Some(HirExpression::TraitMethodReference(self_type, method));
return Some((HirExpression::TraitMethodReference(method), self_type));
}
}
}
Expand All @@ -1599,7 +1608,10 @@ impl<'a> Resolver<'a> {
}

// this resolves a static trait method T::trait_method by iterating over the where clause
fn resolve_trait_method_by_named_generic(&mut self, path: &Path) -> Option<HirExpression> {
fn resolve_trait_method_by_named_generic(
&mut self,
path: &Path,
) -> Option<(HirExpression, Type)> {
if path.segments.len() != 2 {
return None;
}
Expand All @@ -1621,15 +1633,15 @@ impl<'a> Resolver<'a> {
the_trait.find_method(path.segments.last().unwrap().clone())
{
let self_type = self.resolve_type(typ.clone());
return Some(HirExpression::TraitMethodReference(self_type, method));
return Some((HirExpression::TraitMethodReference(method), self_type));
}
}
}
}
None
}

fn resolve_trait_generic_path(&mut self, path: &Path) -> Option<HirExpression> {
fn resolve_trait_generic_path(&mut self, path: &Path) -> Option<(HirExpression, Type)> {
self.resolve_trait_static_method_by_self(path)
.or_else(|| self.resolve_trait_method_by_named_generic(path))
}
Expand Down
6 changes: 6 additions & 0 deletions compiler/noirc_frontend/src/hir/type_check/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,8 @@ pub enum TypeCheckError {
},
#[error("No matching impl found")]
NoMatchingImplFound { constraints: Vec<(Type, String)>, span: Span },
#[error("Constraint for `{typ}: {trait_name}` is not needed, another matching impl is already in scope")]
UnneededTraitConstraint { trait_name: String, typ: Type, span: Span },
}

impl TypeCheckError {
Expand Down Expand Up @@ -269,6 +271,10 @@ impl From<TypeCheckError> for Diagnostic {

diagnostic
}
TypeCheckError::UnneededTraitConstraint { trait_name, typ, span } => {
let msg = format!("Constraint for `{typ}: {trait_name}` is not needed, another matching impl is already in scope");
Diagnostic::simple_warning(msg, "Unnecessary trait constraint in where clause".into(), span)
}
}
}
}
97 changes: 50 additions & 47 deletions compiler/noirc_frontend/src/hir/type_check/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ use crate::{
},
types::Type,
},
node_interner::{DefinitionKind, ExprId, FuncId, TraitMethodId},
node_interner::{DefinitionKind, ExprId, FuncId, TraitId, TraitMethodId},
BinaryOpKind, Signedness, TypeBinding, TypeVariableKind, UnaryOp,
};

Expand Down Expand Up @@ -132,7 +132,9 @@ impl<'interner> TypeChecker<'interner> {
HirExpression::Index(index_expr) => self.check_index_expression(expr_id, index_expr),
HirExpression::Call(call_expr) => {
self.check_if_deprecated(&call_expr.func);

let function = self.check_expression(&call_expr.func);

let args = vecmap(&call_expr.arguments, |arg| {
let typ = self.check_expression(arg);
(typ, *arg, self.interner.expr_span(arg))
Expand Down Expand Up @@ -160,21 +162,29 @@ impl<'interner> TypeChecker<'interner> {
// so that the backend doesn't need to worry about methods
let location = method_call.location;

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 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,
&mut args,
);
let trait_id = match &method_ref {
HirMethodReference::FuncId(func_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);
self.try_add_mutable_reference_to_object(
&mut method_call,
&func_meta.typ,
&mut args,
);
}

let meta = self.interner.function_meta(func_id);
meta.trait_impl.map(|impl_id| {
self.interner
.get_trait_implementation(impl_id)
.borrow()
.trait_id
})
}
}
HirMethodReference::TraitMethodId(method) => Some(method.trait_id),
};

let (function_id, function_call) = method_call.into_function_call(
method_ref.clone(),
Expand All @@ -185,29 +195,8 @@ 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,
});
}
}
if let Some(trait_id) = trait_id {
self.verify_trait_constraint(&object_type, trait_id, function_id, span);
}

self.interner.replace_expr(expr_id, function_call);
Expand Down Expand Up @@ -285,7 +274,7 @@ impl<'interner> TypeChecker<'interner> {

Type::Function(params, Box::new(lambda.return_type), Box::new(env_type))
}
HirExpression::TraitMethodReference(_, method) => {
HirExpression::TraitMethodReference(method) => {
let the_trait = self.interner.get_trait(method.trait_id);
let method = &the_trait.methods[method.method_index];

Expand All @@ -305,6 +294,26 @@ impl<'interner> TypeChecker<'interner> {
typ
}

fn verify_trait_constraint(
&mut self,
object_type: &Type,
trait_id: TraitId,
function_ident_id: ExprId,
span: Span,
) {
match self.interner.lookup_trait_implementation(object_type, trait_id) {
Ok(impl_kind) => self.interner.select_impl_for_ident(function_ident_id, impl_kind),
Err(erroring_constraints) => {
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 });
}
}
}

/// Check if the given method type requires a mutable reference to the object type, and check
/// if the given object type is already a mutable reference. If not, add one.
/// This is used to automatically transform a method call: `foo.bar()` into a function
Expand Down Expand Up @@ -512,13 +521,11 @@ impl<'interner> TypeChecker<'interner> {

let func_meta = self.interner.function_meta(&func_id);
let param_len = func_meta.parameters.len();

(func_meta.typ, param_len)
}
HirMethodReference::TraitMethodId(_, method) => {
HirMethodReference::TraitMethodId(method) => {
let the_trait = self.interner.get_trait(method.trait_id);
let method = &the_trait.methods[method.method_index];

(method.get_type(), method.arguments.len())
}
};
Expand All @@ -537,7 +544,6 @@ impl<'interner> TypeChecker<'interner> {

self.interner.store_instantiation_bindings(*function_ident_id, instantiation_bindings);
self.interner.push_expr_type(function_ident_id, function_type.clone());

self.bind_function_type(function_type, arguments, span)
}

Expand Down Expand Up @@ -889,10 +895,7 @@ impl<'interner> TypeChecker<'interner> {
if method.name.0.contents == method_name {
let trait_method =
TraitMethodId { trait_id: constraint.trait_id, method_index };
return Some(HirMethodReference::TraitMethodId(
object_type.clone(),
trait_method,
));
return Some(HirMethodReference::TraitMethodId(trait_method));
}
}
}
Expand Down
25 changes: 24 additions & 1 deletion compiler/noirc_frontend/src/hir/type_check/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,23 +43,46 @@ pub fn type_check_func(interner: &mut NodeInterner, func_id: FuncId) -> Vec<Type
let mut type_checker = TypeChecker::new(interner);
type_checker.current_function = Some(func_id);

let meta = type_checker.interner.function_meta(&func_id);
let mut errors = Vec::new();

// Temporarily add any impls in this function's `where` clause to scope
for constraint in &meta.trait_constraints {
let object = constraint.typ.clone();
let trait_id = constraint.trait_id;

if !type_checker.interner.add_assumed_trait_implementation(object, trait_id) {
let trait_name = type_checker.interner.get_trait(trait_id).name.to_string();
let typ = constraint.typ.clone();
let span = meta.name.location.span;
errors.push(TypeCheckError::UnneededTraitConstraint { trait_name, typ, span });
}
}

// Bind each parameter to its annotated type.
// This is locally obvious, but it must be bound here so that the
// Definition object of the parameter in the NodeInterner is given the correct type.
for param in meta.parameters.into_iter() {
type_checker.bind_pattern(&param.0, param.1);
}

let (function_last_type, delayed_type_check_functions, mut errors) =
let (function_last_type, delayed_type_check_functions, mut body_errors) =
type_checker.check_function_body(function_body_id);

errors.append(&mut body_errors);

// Go through any delayed type checking errors to see if they are resolved, or error otherwise.
for type_check_fn in delayed_type_check_functions {
if let Err(error) = type_check_fn() {
errors.push(error);
}
}

// Now remove all the `where` clause constraints we added
for constraint in &meta.trait_constraints {
interner.remove_assumed_trait_implementations_for_trait(constraint.trait_id);
}

// Check declared return type and actual return type
if !can_ignore_ret {
let (expr_span, empty_function) = function_info(interner, function_body_id);
Expand Down
8 changes: 4 additions & 4 deletions compiler/noirc_frontend/src/hir_def/expr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ pub enum HirExpression {
If(HirIfExpression),
Tuple(Vec<ExprId>),
Lambda(HirLambda),
TraitMethodReference(Type, TraitMethodId),
TraitMethodReference(TraitMethodId),
jfecher marked this conversation as resolved.
Show resolved Hide resolved
Error,
}

Expand Down Expand Up @@ -156,7 +156,7 @@ pub enum HirMethodReference {
/// Or a method can come from a Trait impl block, in which case
/// the actual function called will depend on the instantiated type,
/// which can be only known during monomorphization.
TraitMethodId(Type, TraitMethodId),
TraitMethodId(TraitMethodId),
}

impl HirMethodCallExpression {
Expand All @@ -174,8 +174,8 @@ impl HirMethodCallExpression {
let id = interner.function_definition_id(func_id);
HirExpression::Ident(HirIdent { location, id })
}
HirMethodReference::TraitMethodId(typ, method_id) => {
HirExpression::TraitMethodReference(typ, method_id)
HirMethodReference::TraitMethodId(method_id) => {
HirExpression::TraitMethodReference(method_id)
}
};
let func = interner.push_expr(expr);
Expand Down
1 change: 1 addition & 0 deletions compiler/noirc_frontend/src/hir_def/traits.rs
Original file line number Diff line number Diff line change
Expand Up @@ -151,5 +151,6 @@ impl TraitFunction {
Box::new(self.return_type.clone()),
Box::new(Type::Unit),
)
.generalize()
}
}
Loading