From eb6fc0f3658bf126ed38d7aec7ee3f44ee0533b5 Mon Sep 17 00:00:00 2001 From: jfecher Date: Tue, 23 Jan 2024 09:13:01 -0800 Subject: [PATCH] fix: apply generic arguments from trait constraints before instantiating identifiers (#4121) # Description ## Problem\* Resolves #4088 ## Summary\* We were calling `instantiate` on identifiers before applying any trait constraints. So if we have a constraint like `Foo` (referring to the trait `trait Foo { ... }`, and an identifier `foo : forall T. fn(T)`, we need to apply the `T = Field` constraint before instantiating `foo` to replace `T` with a fresh type variable. ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** 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: Maxim Vezenov --- .../src/hir/resolution/resolver.rs | 5 +- .../noirc_frontend/src/hir/type_check/expr.rs | 110 ++++++++++-------- .../regression_4088/Nargo.toml | 5 + .../regression_4088/Prover.toml | 2 + .../regression_4088/src/main.nr | 27 +++++ 5 files changed, 101 insertions(+), 48 deletions(-) create mode 100644 test_programs/execution_success/regression_4088/Nargo.toml create mode 100644 test_programs/execution_success/regression_4088/Prover.toml create mode 100644 test_programs/execution_success/regression_4088/src/main.nr diff --git a/compiler/noirc_frontend/src/hir/resolution/resolver.rs b/compiler/noirc_frontend/src/hir/resolution/resolver.rs index 492e96a471..f3e3b22103 100644 --- a/compiler/noirc_frontend/src/hir/resolution/resolver.rs +++ b/compiler/noirc_frontend/src/hir/resolution/resolver.rs @@ -1746,7 +1746,8 @@ impl<'a> Resolver<'a> { // This resolves a static trait method T::trait_method by iterating over the where clause // - // Returns the trait method, object type, and the trait generics. + // Returns the trait method, trait constraint, and whether the impl is assumed from a where + // clause. This is always true since this helper searches where clauses for a generic constraint. // E.g. `t.method()` with `where T: Foo` in scope will return `(Foo::method, T, vec![Bar])` fn resolve_trait_method_by_named_generic( &mut self, @@ -1789,7 +1790,7 @@ impl<'a> Resolver<'a> { // Try to resolve the given trait method path. // - // Returns the trait method, object type, and the trait generics. + // Returns the trait method, trait constraint, and whether the impl is assumed to exist by a where clause or not // E.g. `t.method()` with `where T: Foo` in scope will return `(Foo::method, T, vec![Bar])` fn resolve_trait_generic_path( &mut self, diff --git a/compiler/noirc_frontend/src/hir/type_check/expr.rs b/compiler/noirc_frontend/src/hir/type_check/expr.rs index 58cf4e7b28..22baa9f0da 100644 --- a/compiler/noirc_frontend/src/hir/type_check/expr.rs +++ b/compiler/noirc_frontend/src/hir/type_check/expr.rs @@ -5,8 +5,8 @@ use crate::{ hir::{resolution::resolver::verify_mutable_reference, type_check::errors::Source}, hir_def::{ expr::{ - self, HirArrayLiteral, HirBinaryOp, HirExpression, HirLiteral, HirMethodCallExpression, - HirMethodReference, HirPrefixExpression, ImplKind, + self, HirArrayLiteral, HirBinaryOp, HirExpression, HirIdent, HirLiteral, + HirMethodCallExpression, HirMethodReference, HirPrefixExpression, ImplKind, }, types::Type, }, @@ -46,50 +46,7 @@ impl<'interner> TypeChecker<'interner> { /// function `foo` to refer to. pub(crate) fn check_expression(&mut self, expr_id: &ExprId) -> Type { let typ = match self.interner.expression(expr_id) { - HirExpression::Ident(ident) => { - // An identifiers type may be forall-quantified in the case of generic functions. - // E.g. `fn foo(t: T, field: Field) -> T` has type `forall T. fn(T, Field) -> T`. - // We must instantiate identifiers at every call site to replace this T with a new type - // variable to handle generic functions. - let t = self.interner.id_type_substitute_trait_as_type(ident.id); - - // This instantiate's a trait's generics as well which need to be set - // when the constraint below is later solved for when the function is - // finished. How to link the two? - let (typ, bindings) = t.instantiate(self.interner); - - // Push any trait constraints required by this definition to the context - // to be checked later when the type of this variable is further constrained. - 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.trait_constraints.push((constraint, *expr_id)); - } - } - } - - if let ImplKind::TraitMethod(_, mut constraint, assumed) = ident.impl_kind { - constraint.apply_bindings(&bindings); - if assumed { - let trait_impl = TraitImplKind::Assumed { - object_type: constraint.typ, - trait_generics: constraint.trait_generics, - }; - self.interner.select_impl_for_expression(*expr_id, trait_impl); - } else { - // Currently only one impl can be selected per expr_id, so this - // constraint needs to be pushed after any other constraints so - // that monomorphization can resolve this trait method to the correct impl. - self.trait_constraints.push((constraint, *expr_id)); - } - } - - self.interner.store_instantiation_bindings(*expr_id, bindings); - typ - } + HirExpression::Ident(ident) => self.check_ident(ident, expr_id), HirExpression::Literal(literal) => { match literal { HirLiteral::Array(HirArrayLiteral::Standard(arr)) => { @@ -341,6 +298,67 @@ impl<'interner> TypeChecker<'interner> { typ } + /// Returns the type of the given identifier + fn check_ident(&mut self, ident: HirIdent, expr_id: &ExprId) -> Type { + let mut bindings = TypeBindings::new(); + + // Add type bindings from any constraints that were used. + // We need to do this first since otherwise instantiating the type below + // will replace each trait generic with a fresh type variable, rather than + // the type used in the trait constraint (if it exists). See #4088. + if let ImplKind::TraitMethod(_, constraint, _) = &ident.impl_kind { + let the_trait = self.interner.get_trait(constraint.trait_id); + assert_eq!(the_trait.generics.len(), constraint.trait_generics.len()); + + for (param, arg) in the_trait.generics.iter().zip(&constraint.trait_generics) { + bindings.insert(param.id(), (param.clone(), arg.clone())); + } + } + + // An identifiers type may be forall-quantified in the case of generic functions. + // E.g. `fn foo(t: T, field: Field) -> T` has type `forall T. fn(T, Field) -> T`. + // We must instantiate identifiers at every call site to replace this T with a new type + // variable to handle generic functions. + let t = self.interner.id_type_substitute_trait_as_type(ident.id); + + // This instantiates a trait's generics as well which need to be set + // when the constraint below is later solved for when the function is + // finished. How to link the two? + let (typ, bindings) = t.instantiate_with_bindings(bindings, self.interner); + + // Push any trait constraints required by this definition to the context + // to be checked later when the type of this variable is further constrained. + 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.trait_constraints.push((constraint, *expr_id)); + } + } + } + + if let ImplKind::TraitMethod(_, mut constraint, assumed) = ident.impl_kind { + constraint.apply_bindings(&bindings); + if assumed { + let trait_impl = TraitImplKind::Assumed { + object_type: constraint.typ, + trait_generics: constraint.trait_generics, + }; + self.interner.select_impl_for_expression(*expr_id, trait_impl); + } else { + // Currently only one impl can be selected per expr_id, so this + // constraint needs to be pushed after any other constraints so + // that monomorphization can resolve this trait method to the correct impl. + self.trait_constraints.push((constraint, *expr_id)); + } + } + + self.interner.store_instantiation_bindings(*expr_id, bindings); + typ + } + pub fn verify_trait_constraint( &mut self, object_type: &Type, diff --git a/test_programs/execution_success/regression_4088/Nargo.toml b/test_programs/execution_success/regression_4088/Nargo.toml new file mode 100644 index 0000000000..a5e7832b73 --- /dev/null +++ b/test_programs/execution_success/regression_4088/Nargo.toml @@ -0,0 +1,5 @@ +[package] +name = "regression_4088" +type = "bin" +authors = [""] +[dependencies] diff --git a/test_programs/execution_success/regression_4088/Prover.toml b/test_programs/execution_success/regression_4088/Prover.toml new file mode 100644 index 0000000000..839e31e7e4 --- /dev/null +++ b/test_programs/execution_success/regression_4088/Prover.toml @@ -0,0 +1,2 @@ +[note] +value = 0 diff --git a/test_programs/execution_success/regression_4088/src/main.nr b/test_programs/execution_success/regression_4088/src/main.nr new file mode 100644 index 0000000000..9e4d7892fc --- /dev/null +++ b/test_programs/execution_success/regression_4088/src/main.nr @@ -0,0 +1,27 @@ +trait Serialize { + fn serialize(self) -> [Field; N]; +} + +struct ValueNote { + value: Field, +} + +impl Serialize<1> for ValueNote { + fn serialize(self) -> [Field; 1] { + [self.value] + } +} + +fn check(serialized_note: [Field; N]) { + assert(serialized_note[0] == 0); +} + +fn oopsie(note: Note) where Note: Serialize { + let serialized_note = Note::serialize(note); + + check(serialized_note) +} + +fn main(mut note: ValueNote) { + oopsie(note); +}