From 89846cfbc4961c5258d91b5973f027be80885a20 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Tue, 21 May 2024 19:29:31 +0100 Subject: [PATCH] fix(frontend): Call trait method with mut self from generic definition (#5041) # Description ## Problem\* Resolves Not sure if there is another issue for this I will have to look again, but I found this bug when trying to implement https://github.com/noir-lang/noir/issues/4514 ## Summary\* When type checking a `MethodCall` from the HIR now we also add a mutable reference to the object type when we have a `HirMethodReference::TraitMethodId`. We were previously only doing this for a `HirMethodReference::FuncId`. For example that meant this program would fail with the following errors laid out in the comments: ```rust fn hash_simple_array(input: [Field; 2]) -> Field where H: Hasher + Default { // Check that we can a trait method to instead a trait implementation let mut hasher: H = H::default(); // Regression that the object is converted to a mutable reference type `&mut _`. // Otherwise will see `Expected type &mut _, found type H`. // Then we need to make sure to also auto dereference later in the type checking process // when searching for a matching impl or else we will get `No matching impl found for `&mut H: Hasher` hasher.write(input[0]); hasher.write(input[1]); hasher.finish() } ``` I also had to add auto dereferencing when verifying the trait constraints later inside of `type_check_func`. So first we add a mutable reference to the method call's object type to avoid a mismatched type error, and then we later dereference it to find the appropriate trait implementation. ## Additional Context ## Documentation\* Check one: - [ ] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [ ] I have tested the changes locally. - [ ] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: Jake Fecher Co-authored-by: Jake Fecher Co-authored-by: Tom French --- .../noirc_frontend/src/hir/type_check/expr.rs | 32 +++++--- .../noirc_frontend/src/hir/type_check/mod.rs | 9 ++- noir_stdlib/src/lib.nr | 2 +- .../trait_method_mut_self/Nargo.toml | 7 ++ .../trait_method_mut_self/Prover.toml | 2 + .../trait_method_mut_self/src/main.nr | 74 +++++++++++++++++++ 6 files changed, 113 insertions(+), 13 deletions(-) create mode 100644 test_programs/execution_success/trait_method_mut_self/Nargo.toml create mode 100644 test_programs/execution_success/trait_method_mut_self/Prover.toml create mode 100644 test_programs/execution_success/trait_method_mut_self/src/main.nr diff --git a/compiler/noirc_frontend/src/hir/type_check/expr.rs b/compiler/noirc_frontend/src/hir/type_check/expr.rs index c4f80591bf..abff466e1d 100644 --- a/compiler/noirc_frontend/src/hir/type_check/expr.rs +++ b/compiler/noirc_frontend/src/hir/type_check/expr.rs @@ -236,17 +236,27 @@ impl<'interner> TypeChecker<'interner> { // Automatically add `&mut` if the method expects a mutable reference and // the object is not already one. - if let HirMethodReference::FuncId(func_id) = &method_ref { - if *func_id != FuncId::dummy_id() { - let function_type = - self.interner.function_meta(func_id).typ.clone(); - - self.try_add_mutable_reference_to_object( - &mut method_call, - &function_type, - &mut object_type, - ); + let func_id = match &method_ref { + HirMethodReference::FuncId(func_id) => *func_id, + HirMethodReference::TraitMethodId(method_id, _) => { + let id = self.interner.trait_method_id(*method_id); + let definition = self.interner.definition(id); + let DefinitionKind::Function(func_id) = definition.kind else { + unreachable!( + "Expected trait function to be a DefinitionKind::Function" + ) + }; + func_id } + }; + + if func_id != FuncId::dummy_id() { + let function_type = self.interner.function_meta(&func_id).typ.clone(); + self.try_add_mutable_reference_to_object( + &mut method_call, + &function_type, + &mut object_type, + ); } // TODO: update object_type here? @@ -567,7 +577,7 @@ impl<'interner> TypeChecker<'interner> { /// Insert as many dereference operations as necessary to automatically dereference a method /// call object to its base value type T. - fn insert_auto_dereferences(&mut self, object: ExprId, typ: Type) -> (ExprId, Type) { + pub(crate) fn insert_auto_dereferences(&mut self, object: ExprId, typ: Type) -> (ExprId, Type) { if let Type::MutableReference(element) = typ { let location = self.interner.id_location(object); diff --git a/compiler/noirc_frontend/src/hir/type_check/mod.rs b/compiler/noirc_frontend/src/hir/type_check/mod.rs index 9335b8297f..b2a76828c8 100644 --- a/compiler/noirc_frontend/src/hir/type_check/mod.rs +++ b/compiler/noirc_frontend/src/hir/type_check/mod.rs @@ -141,8 +141,15 @@ pub fn type_check_func(interner: &mut NodeInterner, func_id: FuncId) -> Vec(x: T, y: T) -> T { } #[builtin(as_witness)] -pub fn as_witness(x: Field) {} \ No newline at end of file +pub fn as_witness(x: Field) {} diff --git a/test_programs/execution_success/trait_method_mut_self/Nargo.toml b/test_programs/execution_success/trait_method_mut_self/Nargo.toml new file mode 100644 index 0000000000..d2fe9e8e13 --- /dev/null +++ b/test_programs/execution_success/trait_method_mut_self/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "trait_method_mut_self" +type = "bin" +authors = [""] +compiler_version = ">=0.29.0" + +[dependencies] \ No newline at end of file diff --git a/test_programs/execution_success/trait_method_mut_self/Prover.toml b/test_programs/execution_success/trait_method_mut_self/Prover.toml new file mode 100644 index 0000000000..f28f2f8cc4 --- /dev/null +++ b/test_programs/execution_success/trait_method_mut_self/Prover.toml @@ -0,0 +1,2 @@ +x = "5" +y = "10" diff --git a/test_programs/execution_success/trait_method_mut_self/src/main.nr b/test_programs/execution_success/trait_method_mut_self/src/main.nr new file mode 100644 index 0000000000..fa47fd5d88 --- /dev/null +++ b/test_programs/execution_success/trait_method_mut_self/src/main.nr @@ -0,0 +1,74 @@ +use dep::std::hash::Hasher; +use dep::std::hash::poseidon2::Poseidon2Hasher; + +fn main(x: Field, y: pub Field) { + let mut a_mut_ref = AType { x }; + + pass_trait_by_value(a_mut_ref, y); + assert(a_mut_ref.x == x); + + pass_trait_by_value_impl_param(a_mut_ref, y); + assert(a_mut_ref.x == x); + + pass_trait_by_mut_ref(&mut a_mut_ref, y); + assert(a_mut_ref.x == y); + + let mut hasher = Poseidon2Hasher::default(); + hasher.write(x); + hasher.write(y); + let expected_hash = hasher.finish(); + // Check that we get the same result when using the hasher in a + // method that purely uses trait methods without a supplied implementation. + assert(hash_simple_array::([x, y]) == expected_hash); +} + +trait SomeTrait { + fn set_value(&mut self, new_value: Field) -> (); + + fn get_value(self) -> Field; +} + +struct AType { + x: Field +} + +impl SomeTrait for AType { + fn set_value(&mut self, new_value: Field) -> () { + self.x = new_value; + } + + fn get_value(self) -> Field { + self.x + } +} + +fn pass_trait_by_value_impl_param(mut a_mut_ref: impl SomeTrait, value: Field) { + // We auto add a mutable reference to the object type if the method call expects a mutable self + a_mut_ref.set_value(value); + assert(a_mut_ref.get_value() == value); +} + +fn pass_trait_by_value(mut a_mut_ref: T, value: Field) where T: SomeTrait { + // We auto add a mutable reference to the object type if the method call expects a mutable self + a_mut_ref.set_value(value); + assert(a_mut_ref.get_value() == value); +} + +fn pass_trait_by_mut_ref(a_mut_ref: &mut T, value: Field) where T: SomeTrait { + // We auto add a mutable reference to the object type if the method call expects a mutable self + a_mut_ref.set_value(value); +} + +fn hash_simple_array(input: [Field; 2]) -> Field where H: Hasher + Default { + // Check that we can call a trait method instead of a trait implementation + // TODO: Need to remove the need for this type annotation + // TODO: Curently, without the annotation we will get `Expression type is ambiguous` when trying to use the `hasher` + let mut hasher: H = H::default(); + // Regression that the object is converted to a mutable reference type `&mut _`. + // Otherwise will see `Expected type &mut _, found type H`. + // Then we need to make sure to also auto dereference later in the type checking process + // when searching for a matching impl or else we will get `No matching impl found for `&mut H: Hasher` + hasher.write(input[0]); + hasher.write(input[1]); + hasher.finish() +}