From e00c3705794657ea8f8faa16bc2325511567e185 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Fri, 2 Aug 2024 08:03:56 -0300 Subject: [PATCH] fix: let a trait impl that relies on another trait work (#5646) # 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) { 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 Two for T where T: One {} impl Two for T where T: Three {} ``` The error: ``` error[E0119]: conflicting implementations of trait `Two` --> src/main.rs:13:1 | 11 | impl Two for T where T: One {} | ------------------------------ first implementation here 12 | 13 | impl 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 Three for T where T: One { fn three(self) { println("One"); } } impl Three for T where T: Two { fn three(self) { println("Two"); } } fn use_one(t: T) where T: One { Three::three(t); } fn use_two(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. --- compiler/noirc_frontend/src/node_interner.rs | 14 ++-- compiler/noirc_frontend/src/tests.rs | 72 ++++++++++++++++++++ 2 files changed, 82 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index d5996e6a1f3..06428fe2b01 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -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) = @@ -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; } } @@ -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![]) diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 8ce430b6e48..df85cf0dda4 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -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 Two for T where T: One { + fn two(self) -> i32 { + self.one() + 1 + } + } + + fn use_it(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 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); +}