From 7918e5e63341a8bc8dbbc905ec7dd4356fe1efe4 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 6 Feb 2024 20:31:49 +0000 Subject: [PATCH 1/5] type check ACIR mutable reference passed to brillig --- .../src/hir/type_check/errors.rs | 7 ++- .../noirc_frontend/src/hir/type_check/expr.rs | 38 +++++++++++++ .../brillig_mut_ref_from_acir/Nargo.toml | 7 +++ .../brillig_mut_ref_from_acir/src/main.nr | 56 +++++++++++++++++++ 4 files changed, 107 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..877dd72833c 100644 --- a/compiler/noirc_frontend/src/hir/type_check/expr.rs +++ b/compiler/noirc_frontend/src/hir/type_check/expr.rs @@ -36,6 +36,22 @@ impl<'interner> TypeChecker<'interner> { } } + fn is_unconstrained_call( + &self, + expr: &ExprId, + // arg_types: impl Iterator + ) -> 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 +155,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_unconstrained_call = self.is_unconstrained_call(&call_expr.func); + let is_current_func_constrained = !func_mod.is_unconstrained; + self.check_if_deprecated(&call_expr.func); let function = self.check_expression(&call_expr.func); @@ -147,6 +172,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..4dd38c90132 --- /dev/null +++ b/test_programs/compile_failure/brillig_mut_ref_from_acir/src/main.nr @@ -0,0 +1,56 @@ +use dep::std::field::bn254::assert_gt; + +struct ListItem { + key: Field, + previous: Field, + next: Field, +} + +impl ListItem { + fn default() -> ListItem { + ListItem { + key: 0, + previous: 0, + next: 0, + } + } +} + +struct Map { + entries: [ListItem; Size], + size: Field, + is_empty: bool, + first_index: Field, + last_index: Field, +} + +unconstrained fn find_previous_key_location_crash(map: &mut Map, key: Field) -> (Field) { + let mut found_index: Field = 0; + found_index +} + +impl Map { + fn default() -> Map { + Map{ + entries: [ListItem::default(); Size], // todo fix + size: 0, + is_empty: true, + first_index: 0, + last_index: 0, + } + } + + + fn insert_crashy(&mut self, key: Field) { + let previous_index = find_previous_key_location_crash(self, key); + self.entries[previous_index].key = key; + } + +} + +fn main(x: Field, y: pub Field) { + let mut test_list: Map<10> = Map::default(); + test_list.insert_crashy(x); + test_list.insert_crashy(x + y); + test_list.insert_crashy(x + y * y); +} \ No newline at end of file From 439d7b127e67600a199186b7f767e54107a6ab84 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 6 Feb 2024 20:37:57 +0000 Subject: [PATCH 2/5] cleanup --- compiler/noirc_frontend/src/hir/type_check/expr.rs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/type_check/expr.rs b/compiler/noirc_frontend/src/hir/type_check/expr.rs index 877dd72833c..4bd7779587d 100644 --- a/compiler/noirc_frontend/src/hir/type_check/expr.rs +++ b/compiler/noirc_frontend/src/hir/type_check/expr.rs @@ -36,11 +36,7 @@ impl<'interner> TypeChecker<'interner> { } } - fn is_unconstrained_call( - &self, - expr: &ExprId, - // arg_types: impl Iterator - ) -> bool { + 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) @@ -161,8 +157,8 @@ impl<'interner> TypeChecker<'interner> { .current_function .expect("Can only have call expression inside of a function body"); let func_mod = self.interner.function_modifiers(¤t_func); - let is_unconstrained_call = self.is_unconstrained_call(&call_expr.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); From e6c643825ac797fce02dc06852e59c05fc09a680 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 6 Feb 2024 20:52:34 +0000 Subject: [PATCH 3/5] type check call expression return type for unconstrained to constrained --- compiler/noirc_evaluator/src/errors.rs | 11 ----------- .../noirc_frontend/src/hir/type_check/errors.rs | 5 ++++- .../noirc_frontend/src/hir/type_check/expr.rs | 16 +++++++++++++++- compiler/noirc_frontend/src/hir_def/types.rs | 2 +- 4 files changed, 20 insertions(+), 14 deletions(-) diff --git a/compiler/noirc_evaluator/src/errors.rs b/compiler/noirc_evaluator/src/errors.rs index f6f404204f5..9aabe3bc749 100644 --- a/compiler/noirc_evaluator/src/errors.rs +++ b/compiler/noirc_evaluator/src/errors.rs @@ -174,17 +174,6 @@ impl RuntimeError { location.span, ) } - RuntimeError::UnconstrainedSliceReturnToConstrained { .. } => { - let primary_message = self.to_string(); - let location = - self.call_stack().back().expect("Expected RuntimeError to have a location"); - - Diagnostic::simple_error( - primary_message, - "If attempting to return a `Vec` type, `Vec` contains a slice internally.".to_string(), - location.span, - ) - } _ => { let message = self.to_string(); let location = diff --git a/compiler/noirc_frontend/src/hir/type_check/errors.rs b/compiler/noirc_frontend/src/hir/type_check/errors.rs index b7e26d5c77a..acc673662e5 100644 --- a/compiler/noirc_frontend/src/hir/type_check/errors.rs +++ b/compiler/noirc_frontend/src/hir/type_check/errors.rs @@ -115,6 +115,8 @@ 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("Slices cannot be returned from an unconstrained runtime to a constrained runtime")] + UnconstrainedSliceReturnToConstrained { span: Span }, #[error( "Cannot pass a mutable reference from a constrained runtime to an unconstrained runtime" )] @@ -207,7 +209,8 @@ impl From for Diagnostic { | TypeCheckError::IntegerAndFieldBinaryOperation { span } | TypeCheckError::OverflowingAssignment { span, .. } | TypeCheckError::FieldModulo { span } - | TypeCheckError::ConstrainedReferenceToUnconstrained { span } => { + | TypeCheckError::ConstrainedReferenceToUnconstrained { span } + | TypeCheckError::UnconstrainedSliceReturnToConstrained { 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 4bd7779587d..998abeedcec 100644 --- a/compiler/noirc_frontend/src/hir/type_check/expr.rs +++ b/compiler/noirc_frontend/src/hir/type_check/expr.rs @@ -169,6 +169,7 @@ impl<'interner> TypeChecker<'interner> { (typ, *arg, self.interner.expr_span(arg)) }); + // Check that we are not passing a mutable reference from a constrained runtime to an unconstrained runtime for (typ, _, _) in args.iter() { if is_current_func_constrained && is_unconstrained_call @@ -182,7 +183,20 @@ impl<'interner> TypeChecker<'interner> { } let span = self.interner.expr_span(expr_id); - self.bind_function_type(function, args, span) + let return_type = self.bind_function_type(function, args, span); + + // Check that we are not passing a slice from an unconstrained runtime to a constrained runtime + if is_current_func_constrained + && is_unconstrained_call + && return_type.contains_slice() + { + self.errors.push(TypeCheckError::UnconstrainedSliceReturnToConstrained { + span: self.interner.expr_span(expr_id), + }); + return Type::Error; + } + + return_type } HirExpression::MethodCall(mut method_call) => { let mut object_type = self.check_expression(&method_call.object).follow_bindings(); diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index 0ba4cb2da65..00e24de279b 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/compiler/noirc_frontend/src/hir_def/types.rs @@ -157,7 +157,7 @@ impl Type { } } - fn contains_slice(&self) -> bool { + pub(crate) fn contains_slice(&self) -> bool { match self { Type::Array(size, _) => matches!(size.as_ref(), Type::NotConstant), Type::Struct(struct_typ, generics) => { From 1b2c2d74b15ef3cb8362aa9d9fc520efc20cd39e Mon Sep 17 00:00:00 2001 From: vezenovm Date: Tue, 6 Feb 2024 20:56:51 +0000 Subject: [PATCH 4/5] switch err listing order --- compiler/noirc_frontend/src/hir/type_check/errors.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/type_check/errors.rs b/compiler/noirc_frontend/src/hir/type_check/errors.rs index acc673662e5..3967d7642f7 100644 --- a/compiler/noirc_frontend/src/hir/type_check/errors.rs +++ b/compiler/noirc_frontend/src/hir/type_check/errors.rs @@ -115,12 +115,12 @@ 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("Slices cannot be returned from an unconstrained runtime to a constrained runtime")] - UnconstrainedSliceReturnToConstrained { span: Span }, #[error( "Cannot pass a mutable reference from a constrained runtime to an unconstrained runtime" )] ConstrainedReferenceToUnconstrained { span: Span }, + #[error("Slices cannot be returned from an unconstrained runtime to a constrained runtime")] + UnconstrainedSliceReturnToConstrained { span: Span }, } impl TypeCheckError { From 4fab63020038b6b7ec195c56e76085a7971f7e59 Mon Sep 17 00:00:00 2001 From: vezenovm Date: Wed, 7 Feb 2024 02:11:26 +0000 Subject: [PATCH 5/5] shorten brillig_mut_ref_from_acir test --- .../brillig_mut_ref_from_acir/src/main.nr | 58 ++----------------- 1 file changed, 5 insertions(+), 53 deletions(-) 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 index 4dd38c90132..cf3279cac0d 100644 --- 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 @@ -1,56 +1,8 @@ -use dep::std::field::bn254::assert_gt; - -struct ListItem { - key: Field, - previous: Field, - next: Field, -} - -impl ListItem { - fn default() -> ListItem { - ListItem { - key: 0, - previous: 0, - next: 0, - } - } -} - -struct Map { - entries: [ListItem; Size], - size: Field, - is_empty: bool, - first_index: Field, - last_index: Field, -} - -unconstrained fn find_previous_key_location_crash(map: &mut Map, key: Field) -> (Field) { - let mut found_index: Field = 0; - found_index -} - -impl Map { - fn default() -> Map { - Map{ - entries: [ListItem::default(); Size], // todo fix - size: 0, - is_empty: true, - first_index: 0, - last_index: 0, - } - } - - - fn insert_crashy(&mut self, key: Field) { - let previous_index = find_previous_key_location_crash(self, key); - self.entries[previous_index].key = key; - } - +unconstrained fn mut_ref_identity(value: &mut Field) -> Field { + *value } -fn main(x: Field, y: pub Field) { - let mut test_list: Map<10> = Map::default(); - test_list.insert_crashy(x); - test_list.insert_crashy(x + y); - test_list.insert_crashy(x + y * y); +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