From 7e139de3499478cf573d2a7ad480f434cb898d9f Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 7 Feb 2024 02:32:55 +0000 Subject: [PATCH] fix: Type check ACIR mutable reference passed to brillig (#4281) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description ## Problem\* Resolves #4245 ## Summary\* This adds a type check for call expressions to check the arguments of a call to an unconstrained function from a constrained function. As we have two different runtime environments this makes type checking a bit more complicated. As a followup to this PR we could try and catch the error added here (https://github.com/noir-lang/noir/pull/4280) sooner. However, if the unconstrained function uses generics we must still fall back to the "type check" in SSA/ACIR gen from PR #4280. This PR now errors for the snippet in issue with the below using `nargo check`: Screenshot 2024-02-06 at 3 36 27 PM ## Additional Context ## Documentation\* Check one: - [ ] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[Exceptional Case]** 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. --- .../src/hir/type_check/errors.rs | 7 +++- .../noirc_frontend/src/hir/type_check/expr.rs | 34 +++++++++++++++++++ .../brillig_mut_ref_from_acir/Nargo.toml | 7 ++++ .../brillig_mut_ref_from_acir/src/main.nr | 8 +++++ 4 files changed, 55 insertions(+), 1 deletion(-) create mode 100644 test_programs/compile_failure/brillig_mut_ref_from_acir/Nargo.toml create mode 100644 test_programs/compile_failure/brillig_mut_ref_from_acir/src/main.nr diff --git a/compiler/noirc_frontend/src/hir/type_check/errors.rs b/compiler/noirc_frontend/src/hir/type_check/errors.rs index 267dbd6b5be..b7e26d5c77a 100644 --- a/compiler/noirc_frontend/src/hir/type_check/errors.rs +++ b/compiler/noirc_frontend/src/hir/type_check/errors.rs @@ -115,6 +115,10 @@ pub enum TypeCheckError { NoMatchingImplFound { constraints: Vec<(Type, String)>, span: Span }, #[error("Constraint for `{typ}: {trait_name}` is not needed, another matching impl is already in scope")] UnneededTraitConstraint { trait_name: String, typ: Type, span: Span }, + #[error( + "Cannot pass a mutable reference from a constrained runtime to an unconstrained runtime" + )] + ConstrainedReferenceToUnconstrained { span: Span }, } impl TypeCheckError { @@ -202,7 +206,8 @@ impl From for Diagnostic { | TypeCheckError::AmbiguousBitWidth { span, .. } | TypeCheckError::IntegerAndFieldBinaryOperation { span } | TypeCheckError::OverflowingAssignment { span, .. } - | TypeCheckError::FieldModulo { span } => { + | TypeCheckError::FieldModulo { span } + | TypeCheckError::ConstrainedReferenceToUnconstrained { span } => { Diagnostic::simple_error(error.to_string(), String::new(), span) } TypeCheckError::PublicReturnType { typ, span } => Diagnostic::simple_error( diff --git a/compiler/noirc_frontend/src/hir/type_check/expr.rs b/compiler/noirc_frontend/src/hir/type_check/expr.rs index 5885707a9b7..4bd7779587d 100644 --- a/compiler/noirc_frontend/src/hir/type_check/expr.rs +++ b/compiler/noirc_frontend/src/hir/type_check/expr.rs @@ -36,6 +36,18 @@ impl<'interner> TypeChecker<'interner> { } } + fn is_unconstrained_call(&self, expr: &ExprId) -> bool { + if let HirExpression::Ident(expr::HirIdent { id, .. }) = self.interner.expression(expr) { + if let Some(DefinitionKind::Function(func_id)) = + self.interner.try_definition(id).map(|def| &def.kind) + { + let modifiers = self.interner.function_modifiers(func_id); + return modifiers.is_unconstrained; + } + } + false + } + /// Infers a type for a given expression, and return this type. /// As a side-effect, this function will also remember this type in the NodeInterner /// for the given expr_id key. @@ -139,6 +151,15 @@ impl<'interner> TypeChecker<'interner> { } HirExpression::Index(index_expr) => self.check_index_expression(expr_id, index_expr), HirExpression::Call(call_expr) => { + // Need to setup these flags here as `self` is borrowed mutably to type check the rest of the call expression + // These flags are later used to type check calls to unconstrained functions from constrained functions + let current_func = self + .current_function + .expect("Can only have call expression inside of a function body"); + let func_mod = self.interner.function_modifiers(¤t_func); + let is_current_func_constrained = !func_mod.is_unconstrained; + let is_unconstrained_call = self.is_unconstrained_call(&call_expr.func); + self.check_if_deprecated(&call_expr.func); let function = self.check_expression(&call_expr.func); @@ -147,6 +168,19 @@ impl<'interner> TypeChecker<'interner> { let typ = self.check_expression(arg); (typ, *arg, self.interner.expr_span(arg)) }); + + for (typ, _, _) in args.iter() { + if is_current_func_constrained + && is_unconstrained_call + && matches!(&typ, Type::MutableReference(_)) + { + self.errors.push(TypeCheckError::ConstrainedReferenceToUnconstrained { + span: self.interner.expr_span(expr_id), + }); + return Type::Error; + } + } + let span = self.interner.expr_span(expr_id); self.bind_function_type(function, args, span) } diff --git a/test_programs/compile_failure/brillig_mut_ref_from_acir/Nargo.toml b/test_programs/compile_failure/brillig_mut_ref_from_acir/Nargo.toml new file mode 100644 index 00000000000..a20ee09714c --- /dev/null +++ b/test_programs/compile_failure/brillig_mut_ref_from_acir/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "brillig_mut_ref_from_acir" +type = "bin" +authors = [""] +compiler_version = ">=0.23.0" + +[dependencies] \ No newline at end of file diff --git a/test_programs/compile_failure/brillig_mut_ref_from_acir/src/main.nr b/test_programs/compile_failure/brillig_mut_ref_from_acir/src/main.nr new file mode 100644 index 00000000000..cf3279cac0d --- /dev/null +++ b/test_programs/compile_failure/brillig_mut_ref_from_acir/src/main.nr @@ -0,0 +1,8 @@ +unconstrained fn mut_ref_identity(value: &mut Field) -> Field { + *value +} + +fn main(mut x: Field, y: pub Field) { + let returned_x = mut_ref_identity(&mut x); + assert(returned_x == x); +} \ No newline at end of file