Skip to content

Commit

Permalink
fix: let a trait impl that relies on another trait work (#5646)
Browse files Browse the repository at this point in the history
# Description

## Problem

Resolves #5195

## Summary

If I'm understanding the code correctly, when you have code like this:

```rust
trait Trait {
  fn trait_func(self);
}

fn foo<T>(t: T) {
  Trait::trait_func(t);
}
```

the compiler will search in all impls of `Trait` for a matching one.
Then:
- if it only finds one, it succeeds
- otherwise it's an error (no matching impl found, or too many found)

If an impl has a where clause, the compiler checks if that where clause
matches. For example:

```rust
trait One {
}

impl Trait for T where T: One {
  // ...
}
```

So here there's an impl for `Trait`, it has a where clause, so the
compiler checks if that `T` is `One`. And here's the thing: if there's
no match, it would immediately error. I think that was the bug.

In this PR, we capture that error but continue seeing if we can find a
matching impl. If we can't find any impl, we use the error of the first
non-matching `where` clause to help users.

But... let me know if I got this wrong. I _think_ I understood the code,
but I'm not sure.

## Additional Context

When trying out different codes with traits and impls I noticed that we
don't error if two trait impls are conflicting. For example Rust gives
an error in this case:

```rust
trait One {}

impl One for i32 {}

trait Three {}

impl Three for i32 {}

trait Two {}

impl<T> Two for T where T: One {}

impl<T> Two for T where T: Three {}
```

The error:

```
error[E0119]: conflicting implementations of trait `Two`
  --> src/main.rs:13:1
   |
11 | impl<T> Two for T where T: One {}
   | ------------------------------ first implementation here
12 |
13 | impl<T> Two for T where T: Three {}
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ conflicting implementation
```

I don't know if it's a requirement to do this before merging this PR.
Even if there are conflicting impls in Noir, the compiler does the right
thing. For example this code:

```rust
trait One {
}

impl One for i32 {
}

trait Two {

}

impl Two for i32 {

}

trait Three {
    fn three(self);
}

impl<T> Three for T where T: One {
    fn three(self) {
        println("One");
    }
}

impl<T> Three for T where T: Two {
    fn three(self) {
        println("Two");
    }
}

fn use_one<T>(t: T) where T: One {
    Three::three(t);
}

fn use_two<T>(t: T) where T: Two {
    Three::three(t);
}

fn main() {
    let x: i32 = 0;
    use_one(x);
    use_two(x);
}
```

prints:

```
One
Two
```

## 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.
  • Loading branch information
asterite authored Aug 2, 2024
1 parent 8122624 commit e00c370
Show file tree
Hide file tree
Showing 2 changed files with 82 additions and 4 deletions.
14 changes: 10 additions & 4 deletions compiler/noirc_frontend/src/node_interner.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1437,6 +1437,8 @@ impl NodeInterner {

let mut matching_impls = Vec::new();

let mut where_clause_errors = Vec::new();

for (existing_object_type2, impl_kind) in impls {
// Bug: We're instantiating only the object type's generics here, not all of the trait's generics like we need to
let (existing_object_type, instantiation_bindings) =
Expand Down Expand Up @@ -1471,14 +1473,17 @@ impl NodeInterner {
let trait_impl = self.get_trait_implementation(*impl_id);
let trait_impl = trait_impl.borrow();

if let Err(mut errors) = self.validate_where_clause(
if let Err(errors) = self.validate_where_clause(
&trait_impl.where_clause,
&mut fresh_bindings,
&instantiation_bindings,
recursion_limit,
) {
errors.push(make_constraint());
return Err(errors);
// Only keep the first errors we get from a failing where clause
if where_clause_errors.is_empty() {
where_clause_errors.extend(errors);
}
continue;
}
}

Expand All @@ -1491,7 +1496,8 @@ impl NodeInterner {
*type_bindings = fresh_bindings;
Ok(impl_)
} else if matching_impls.is_empty() {
Err(vec![make_constraint()])
where_clause_errors.push(make_constraint());
Err(where_clause_errors)
} else {
// multiple matching impls, type annotations needed
Err(vec![])
Expand Down
72 changes: 72 additions & 0 deletions compiler/noirc_frontend/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2846,3 +2846,75 @@ fn error_on_cast_over_type_variable() {
CompilationError::TypeError(TypeCheckError::TypeMismatch { .. })
));
}

#[test]
fn trait_impl_for_a_type_that_implements_another_trait() {
let src = r#"
trait One {
fn one(self) -> i32;
}
impl One for i32 {
fn one(self) -> i32 {
self
}
}
trait Two {
fn two(self) -> i32;
}
impl<T> Two for T where T: One {
fn two(self) -> i32 {
self.one() + 1
}
}
fn use_it<T>(t: T) -> i32 where T: Two {
Two::two(t)
}
fn main() {}
"#;
assert_no_errors(src);
}

#[test]
fn trait_impl_for_a_type_that_implements_another_trait_with_another_impl_used() {
let src = r#"
trait One {
fn one(self) -> i32;
}
impl One for i32 {
fn one(self) -> i32 {
let _ = self;
1
}
}
trait Two {
fn two(self) -> i32;
}
impl<T> Two for T where T: One {
fn two(self) -> i32 {
self.one() + 1
}
}
impl Two for u32 {
fn two(self) -> i32 {
let _ = self;
0
}
}
fn use_it(t: u32) -> i32 {
Two::two(t)
}
fn main() {}
"#;
assert_no_errors(src);
}

0 comments on commit e00c370

Please sign in to comment.