From 6c0ea05c0f8f30676439bc7cf7144edb16b07b8a Mon Sep 17 00:00:00 2001 From: Tom French Date: Wed, 10 Jul 2024 10:47:09 +0100 Subject: [PATCH 1/8] fix: remove compile-time error for invalid indices --- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 57 +++++++------------ 1 file changed, 22 insertions(+), 35 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index cfcc7a9a997..49fbffa2261 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -1085,45 +1085,32 @@ impl<'a> Context<'a> { } }; - let side_effects_always_enabled = - self.acir_context.is_constant_one(&self.current_side_effects_enabled_var); - let index_out_of_bounds = index >= array_size; - - // Note that the value of `side_effects_always_enabled` doesn't affect the value which we return here for valid - // indices, just whether we return an error for invalid indices at compile time or defer until execution. - match (side_effects_always_enabled, index_out_of_bounds) { - (true, false) => { - let value = match store_value { - Some(store_value) => AcirValue::Array(array.update(index, store_value)), - None => array[index].clone(), - }; + if index >= array_size { + return Ok(false); + } + + if let Some(store_value) = store_value { + // Note that the value of `side_effects_always_enabled` doesn't affect the value which we return here for + // valid indices, just whether we return an error for invalid indices at compile time or defer until execution. + let side_effects_always_enabled = + self.acir_context.is_constant_one(&self.current_side_effects_enabled_var); + if side_effects_always_enabled { + // If we know that this write will always occur then we can perform it at compile time. + let value = AcirValue::Array(array.update(index, store_value)); self.define_result(dfg, instruction, value); Ok(true) + } else { + // If a predicate is applied however we must wait until runtime. + Ok(false) } - (false, false) => { - if store_value.is_none() { - // If there is a predicate and the index is not out of range, we can optimistically perform the - // read at compile time as if the predicate is true. - // - // This is as if the predicate is false, any side-effects will be disabled so the value returned - // will not affect the rest of execution. - self.define_result(dfg, instruction, array[index].clone()); - Ok(true) - } else { - // We do not do this for a array writes however. - Ok(false) - } - } - - // Report the error if side effects are enabled. - (true, true) => { - let call_stack = self.acir_context.get_call_stack(); - Err(RuntimeError::IndexOutOfBounds { index, array_size, call_stack }) - } - // Index is out of bounds but predicate may result in this array operation being skipped - // so we don't return an error now. - (false, true) => Ok(false), + } else { + // If the index is not out of range, we can optimistically perform the read at compile time + // as if the predicate were true. This is as if the predicate were to resolve to false then + // the result should not affect the rest of circuit execution. + let value = array[index].clone(); + self.define_result(dfg, instruction, value); + Ok(true) } } From 90d89e1ae01b96c12380acf8c05ed0f4b4b72023 Mon Sep 17 00:00:00 2001 From: Tom French Date: Wed, 10 Jul 2024 10:49:38 +0100 Subject: [PATCH 2/8] . --- compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 49fbffa2261..585afc27919 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -1090,8 +1090,6 @@ impl<'a> Context<'a> { } if let Some(store_value) = store_value { - // Note that the value of `side_effects_always_enabled` doesn't affect the value which we return here for - // valid indices, just whether we return an error for invalid indices at compile time or defer until execution. let side_effects_always_enabled = self.acir_context.is_constant_one(&self.current_side_effects_enabled_var); From a9e6e283c8f3f960a17a2c7c0e0c581dd9fd5261 Mon Sep 17 00:00:00 2001 From: Tom French Date: Wed, 10 Jul 2024 10:51:37 +0100 Subject: [PATCH 3/8] . --- compiler/noirc_evaluator/src/errors.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/compiler/noirc_evaluator/src/errors.rs b/compiler/noirc_evaluator/src/errors.rs index 2b328898257..2c7ec0f8e1a 100644 --- a/compiler/noirc_evaluator/src/errors.rs +++ b/compiler/noirc_evaluator/src/errors.rs @@ -19,8 +19,6 @@ use serde::{Deserialize, Serialize}; pub enum RuntimeError { #[error(transparent)] InternalError(#[from] InternalError), - #[error("Index out of bounds, array has size {array_size}, but index was {index}")] - IndexOutOfBounds { index: usize, array_size: usize, call_stack: CallStack }, #[error("Range constraint of {num_bits} bits is too large for the Field size")] InvalidRangeConstraint { num_bits: u32, call_stack: CallStack }, #[error("The value `{value:?}` cannot fit into `{typ}` which has range `{range}`")] @@ -145,7 +143,6 @@ impl RuntimeError { | InternalError::UndeclaredAcirVar { call_stack } | InternalError::Unexpected { call_stack, .. }, ) - | RuntimeError::IndexOutOfBounds { call_stack, .. } | RuntimeError::InvalidRangeConstraint { call_stack, .. } | RuntimeError::TypeConversion { call_stack, .. } | RuntimeError::UnInitialized { call_stack, .. } From 4f9fc1613fc4c9493585e6b25c8f00eb46fe7fd3 Mon Sep 17 00:00:00 2001 From: Tom French Date: Wed, 10 Jul 2024 11:04:49 +0100 Subject: [PATCH 4/8] chore: add test for invalid index errors --- noir_stdlib/src/collections/bounded_vec.nr | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/noir_stdlib/src/collections/bounded_vec.nr b/noir_stdlib/src/collections/bounded_vec.nr index c218ecd2348..f9ac3bee92a 100644 --- a/noir_stdlib/src/collections/bounded_vec.nr +++ b/noir_stdlib/src/collections/bounded_vec.nr @@ -14,7 +14,7 @@ impl BoundedVec { /// Get an element from the vector at the given index. /// Panics if the given index points beyond the end of the vector (`self.len()`). pub fn get(self, index: u32) -> T { - assert(index < self.len); + assert(index < self.len, "Attempted to read past end of BoundedVec"); self.get_unchecked(index) } @@ -173,6 +173,18 @@ mod bounded_vec_tests { assert(bounded_vec1 != bounded_vec2); } + mod get { + use crate::collections::bounded_vec::BoundedVec; + + #[test(should_fail_with = "Attempted to read past end of BoundedVec")] + fn panics_when_reading_elements_past_end_of_vec() { + let mut vec: BoundedVec = BoundedVec::new(); + + println(vec.get(0)); + } + } + + mod set { use crate::collections::bounded_vec::BoundedVec; From e06daa397244eeaa4816615d0867598e8622b02b Mon Sep 17 00:00:00 2001 From: Tom French Date: Wed, 10 Jul 2024 11:06:53 +0100 Subject: [PATCH 5/8] . --- noir_stdlib/src/collections/bounded_vec.nr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/noir_stdlib/src/collections/bounded_vec.nr b/noir_stdlib/src/collections/bounded_vec.nr index f9ac3bee92a..e93ef6e794d 100644 --- a/noir_stdlib/src/collections/bounded_vec.nr +++ b/noir_stdlib/src/collections/bounded_vec.nr @@ -178,7 +178,7 @@ mod bounded_vec_tests { #[test(should_fail_with = "Attempted to read past end of BoundedVec")] fn panics_when_reading_elements_past_end_of_vec() { - let mut vec: BoundedVec = BoundedVec::new(); + let vec: BoundedVec = BoundedVec::new(); println(vec.get(0)); } From 43bf911f7f972744079b8e85145a24754a943a4d Mon Sep 17 00:00:00 2001 From: Tom French Date: Wed, 10 Jul 2024 11:09:17 +0100 Subject: [PATCH 6/8] . --- noir_stdlib/src/collections/bounded_vec.nr | 43 +++++++++++----------- 1 file changed, 22 insertions(+), 21 deletions(-) diff --git a/noir_stdlib/src/collections/bounded_vec.nr b/noir_stdlib/src/collections/bounded_vec.nr index e93ef6e794d..5ea12ef0719 100644 --- a/noir_stdlib/src/collections/bounded_vec.nr +++ b/noir_stdlib/src/collections/bounded_vec.nr @@ -152,26 +152,6 @@ impl From<[T; Len]> for BoundedVec } mod bounded_vec_tests { - // TODO: Allow imports from "super" - use crate::collections::bounded_vec::BoundedVec; - - #[test] - fn empty_equality() { - let mut bounded_vec1: BoundedVec = BoundedVec::new(); - let mut bounded_vec2: BoundedVec = BoundedVec::new(); - - assert_eq(bounded_vec1, bounded_vec2); - } - - #[test] - fn inequality() { - let mut bounded_vec1: BoundedVec = BoundedVec::new(); - let mut bounded_vec2: BoundedVec = BoundedVec::new(); - bounded_vec1.push(1); - bounded_vec2.push(2); - - assert(bounded_vec1 != bounded_vec2); - } mod get { use crate::collections::bounded_vec::BoundedVec; @@ -184,7 +164,6 @@ mod bounded_vec_tests { } } - mod set { use crate::collections::bounded_vec::BoundedVec; @@ -307,4 +286,26 @@ mod bounded_vec_tests { assert_eq(bounded_vec.storage()[1], 2); } } + + mod trait_eq { + use crate::collections::bounded_vec::BoundedVec; + + #[test] + fn empty_equality() { + let mut bounded_vec1: BoundedVec = BoundedVec::new(); + let mut bounded_vec2: BoundedVec = BoundedVec::new(); + + assert_eq(bounded_vec1, bounded_vec2); + } + + #[test] + fn inequality() { + let mut bounded_vec1: BoundedVec = BoundedVec::new(); + let mut bounded_vec2: BoundedVec = BoundedVec::new(); + bounded_vec1.push(1); + bounded_vec2.push(2); + + assert(bounded_vec1 != bounded_vec2); + } + } } From 93ae3612ff5b7a79498f99bb512ea1230a5e2b23 Mon Sep 17 00:00:00 2001 From: Tom French Date: Wed, 10 Jul 2024 11:22:12 +0100 Subject: [PATCH 7/8] . --- noir_stdlib/src/collections/bounded_vec.nr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/noir_stdlib/src/collections/bounded_vec.nr b/noir_stdlib/src/collections/bounded_vec.nr index 5ea12ef0719..79985b5eef4 100644 --- a/noir_stdlib/src/collections/bounded_vec.nr +++ b/noir_stdlib/src/collections/bounded_vec.nr @@ -160,7 +160,7 @@ mod bounded_vec_tests { fn panics_when_reading_elements_past_end_of_vec() { let vec: BoundedVec = BoundedVec::new(); - println(vec.get(0)); + crate::println(vec.get(0)); } } From 1be06d9c8761f89c6015cda335dcb2641363293d Mon Sep 17 00:00:00 2001 From: Tom French Date: Wed, 10 Jul 2024 12:00:31 +0100 Subject: [PATCH 8/8] . --- noir_stdlib/src/collections/bounded_vec.nr | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/noir_stdlib/src/collections/bounded_vec.nr b/noir_stdlib/src/collections/bounded_vec.nr index 79985b5eef4..a56d6f60390 100644 --- a/noir_stdlib/src/collections/bounded_vec.nr +++ b/noir_stdlib/src/collections/bounded_vec.nr @@ -159,7 +159,7 @@ mod bounded_vec_tests { #[test(should_fail_with = "Attempted to read past end of BoundedVec")] fn panics_when_reading_elements_past_end_of_vec() { let vec: BoundedVec = BoundedVec::new(); - + crate::println(vec.get(0)); } }