Skip to content

Commit

Permalink
fix(frontend): Error for when impl is stricter than trait (#5343)
Browse files Browse the repository at this point in the history 
# Description

## Problem\*

Resolves #5210 

## Summary\*

For context it is good to start with the linked issue and the
discussion.

In the linked issue discussion, a non-existent trait implementation
would pass type checking and fail during monomorphization. It turns out
the linked issue's error was not that the compiler should error with `No
impl found ...` but rather that the impl has stricter requirements than
the trait. When we move the `where` clause to the trait impl we get the
expected no matching impl error as expected.

But now the following code from the issue will have this error:
<img width="744" alt="Screenshot 2024-06-26 at 4 30 39 PM"
src="https://github.com/noir-lang/noir/assets/43554004/63b3cb45-91d0-4bdd-abc2-e9ba97c541a8">

We will also error if the object type in a trait constraint has
different generics:
```rust
trait MyTrait { }

trait Bar<T> {
    fn bar<U>() where Option<T>: MyTrait;
}

impl<A> Bar<A> for () {
    fn bar<B>() where Option<B>: MyTrait {}
}
```
Will return the following error:
<img width="764" alt="Screenshot 2024-07-10 at 4 57 22 PM"
src="https://github.com/noir-lang/noir/assets/43554004/98489dc8-72aa-40ec-9b82-e56555affebd">



I have also attached a test showing how we get the expected no impl
found error when the where clause is accurately placed on the trait impl
rather than the trait impl method.

## Additional Context

The way we attach trait impl where clauses onto methods did complicate
things a little. We should consider maintaining a clearer separation for
where clauses on impls and where clauses on functions.

## 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: jfecher <jake@aztecprotocol.com>
  • Loading branch information
@vezenovm @jfecher
vezenovm and jfecher authored Jul 18, 2024
1 parent cd16745 commit ece033f
Show file tree
Hide file tree
Showing 14 changed files with 616 additions and 136 deletions.
1 change: 1 addition & 0 deletions compiler/noirc_frontend/src/elaborator/expressions.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -586,6 +586,7 @@ impl<'context> Elaborator<'context> {
typ: operand_type.clone(),
trait_id: trait_id.trait_id,
trait_generics: Vec::new(),
span,
};
self.push_trait_constraint(constraint, expr_id);
self.type_check_operator_method(expr_id, trait_id, operand_type, span);
Expand Down
136 changes: 11 additions & 125 deletions compiler/noirc_frontend/src/elaborator/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,6 @@ use crate::{
UnresolvedStruct, UnresolvedTypeAlias,
},
dc_mod,
errors::DuplicateType,
},
resolution::{errors::ResolverError, path_resolver::PathResolver},
scope::ScopeForest as GenericScopeForest,
Expand Down Expand Up @@ -66,14 +65,15 @@ mod lints;
mod patterns;
mod scope;
mod statements;
mod trait_impls;
mod traits;
pub mod types;
mod unquote;

use fm::FileId;
use iter_extended::vecmap;
use noirc_errors::{Location, Span};
use rustc_hash::{FxHashMap as HashMap, FxHashSet as HashSet};
use rustc_hash::FxHashMap as HashMap;

use self::traits::check_trait_impl_method_matches_declaration;

Expand Down Expand Up @@ -622,7 +622,7 @@ impl<'context> Elaborator<'context> {
});
}

Some(TraitConstraint { typ, trait_id, trait_generics })
Some(TraitConstraint { typ, trait_id, trait_generics, span })
}

/// Extract metadata from a NoirFunction
Expand Down Expand Up @@ -929,7 +929,14 @@ impl<'context> Elaborator<'context> {

if let Some(trait_id) = trait_impl.trait_id {
self.generics = trait_impl.resolved_generics.clone();
self.collect_trait_impl_methods(trait_id, trait_impl);

let where_clause = trait_impl
.where_clause
.iter()
.flat_map(|item| self.resolve_trait_constraint(item))
.collect::<Vec<_>>();

self.collect_trait_impl_methods(trait_id, trait_impl, &where_clause);

let span = trait_impl.object_type.span.expect("All trait self types should have spans");
self.declare_methods_on_struct(true, &mut trait_impl.methods, span);
Expand All @@ -939,12 +946,6 @@ impl<'context> Elaborator<'context> {
self.interner.set_function_trait(*func_id, self_type.clone(), trait_id);
}

let where_clause = trait_impl
.where_clause
.iter()
.flat_map(|item| self.resolve_trait_constraint(item))
.collect();

let trait_generics = trait_impl.resolved_trait_generics.clone();

let resolved_trait_impl = Shared::new(TraitImpl {
Expand Down Expand Up @@ -1075,121 +1076,6 @@ impl<'context> Elaborator<'context> {
}
}

fn collect_trait_impl_methods(
&mut self,
trait_id: TraitId,
trait_impl: &mut UnresolvedTraitImpl,
) {
self.local_module = trait_impl.module_id;
self.file = trait_impl.file_id;

// In this Vec methods[i] corresponds to trait.methods[i]. If the impl has no implementation
// for a particular method, the default implementation will be added at that slot.
let mut ordered_methods = Vec::new();

// check whether the trait implementation is in the same crate as either the trait or the type
self.check_trait_impl_crate_coherence(trait_id, trait_impl);

// set of function ids that have a corresponding method in the trait
let mut func_ids_in_trait = HashSet::default();

// Temporarily take ownership of the trait's methods so we can iterate over them
// while also mutating the interner
let the_trait = self.interner.get_trait_mut(trait_id);
let methods = std::mem::take(&mut the_trait.methods);

for method in &methods {
let overrides: Vec<_> = trait_impl
.methods
.functions
.iter()
.filter(|(_, _, f)| f.name() == method.name.0.contents)
.collect();

if overrides.is_empty() {
if let Some(default_impl) = &method.default_impl {
// copy 'where' clause from unresolved trait impl
let mut default_impl_clone = default_impl.clone();
default_impl_clone.def.where_clause.extend(trait_impl.where_clause.clone());

let func_id = self.interner.push_empty_fn();
let module = self.module_id();
let location = Location::new(default_impl.def.span, trait_impl.file_id);
self.interner.push_function(func_id, &default_impl.def, module, location);
self.define_function_meta(&mut default_impl_clone, func_id, false);
func_ids_in_trait.insert(func_id);
ordered_methods.push((
method.default_impl_module_id,
func_id,
*default_impl_clone,
));
} else {
self.push_err(DefCollectorErrorKind::TraitMissingMethod {
trait_name: self.interner.get_trait(trait_id).name.clone(),
method_name: method.name.clone(),
trait_impl_span: trait_impl
.object_type
.span
.expect("type must have a span"),
});
}
} else {
for (_, func_id, _) in &overrides {
func_ids_in_trait.insert(*func_id);
}

if overrides.len() > 1 {
self.push_err(DefCollectorErrorKind::Duplicate {
typ: DuplicateType::TraitAssociatedFunction,
first_def: overrides[0].2.name_ident().clone(),
second_def: overrides[1].2.name_ident().clone(),
});
}

ordered_methods.push(overrides[0].clone());
}
}

// Restore the methods that were taken before the for loop
let the_trait = self.interner.get_trait_mut(trait_id);
the_trait.set_methods(methods);

// Emit MethodNotInTrait error for methods in the impl block that
// don't have a corresponding method signature defined in the trait
for (_, func_id, func) in &trait_impl.methods.functions {
if !func_ids_in_trait.contains(func_id) {
let trait_name = the_trait.name.clone();
let impl_method = func.name_ident().clone();
let error = DefCollectorErrorKind::MethodNotInTrait { trait_name, impl_method };
self.errors.push((error.into(), self.file));
}
}

trait_impl.methods.functions = ordered_methods;
trait_impl.methods.trait_id = Some(trait_id);
}

fn check_trait_impl_crate_coherence(
&mut self,
trait_id: TraitId,
trait_impl: &UnresolvedTraitImpl,
) {
self.local_module = trait_impl.module_id;
self.file = trait_impl.file_id;

let object_crate = match &trait_impl.resolved_object_type {
Some(Type::Struct(struct_type, _)) => struct_type.borrow().id.krate(),
_ => CrateId::Dummy,
};

let the_trait = self.interner.get_trait(trait_id);
if self.crate_id != the_trait.crate_id && self.crate_id != object_crate {
self.push_err(DefCollectorErrorKind::TraitImplOrphaned {
span: trait_impl.object_type.span.expect("object type must have a span"),
});
}
}

fn define_type_alias(&mut self, alias_id: TypeAliasId, alias: UnresolvedTypeAlias) {
self.file = alias.file_id;
self.local_module = alias.module_id;
Expand Down
1 change: 0 additions & 1 deletion compiler/noirc_frontend/src/elaborator/patterns.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -596,7 +596,6 @@ impl<'context> Elaborator<'context> {
if let Some(definition) = self.interner.try_definition(ident.id) {
if let DefinitionKind::Function(function) = definition.kind {
let function = self.interner.function_meta(&function);

for mut constraint in function.trait_constraints.clone() {
constraint.apply_bindings(&bindings);
self.push_trait_constraint(constraint, expr_id);
Expand Down
Loading

0 comments on commit ece033f

Please sign in to comment.