diff --git a/compiler/noirc_evaluator/src/ssa/opt/array_set.rs b/compiler/noirc_evaluator/src/ssa/opt/array_set.rs index e7f69f87da8..865a1e31eb3 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/array_set.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/array_set.rs @@ -28,17 +28,21 @@ impl Ssa { impl Function { pub(crate) fn array_set_optimization(&mut self) { + if matches!(self.runtime(), RuntimeType::Brillig(_)) { + // Brillig is supposed to use refcounting to decide whether to mutate an array; + // array mutation was only meant for ACIR. We could use it with Brillig as well, + // but then some of the optimizations that we can do in ACIR around shared + // references have to be skipped, which makes it more cumbersome. + return; + } + let reachable_blocks = self.reachable_blocks(); if !self.runtime().is_entry_point() { assert_eq!(reachable_blocks.len(), 1, "Expected there to be 1 block remaining in Acir function for array_set optimization"); } - let mut context = Context::new( - &self.dfg, - self.parameters(), - matches!(self.runtime(), RuntimeType::Brillig(_)), - ); + let mut context = Context::new(&self.dfg); for block in reachable_blocks.iter() { context.analyze_last_uses(*block); @@ -53,8 +57,6 @@ impl Function { struct Context<'f> { dfg: &'f DataFlowGraph, - function_parameters: &'f [ValueId], - is_brillig_runtime: bool, array_to_last_use: HashMap, instructions_that_can_be_made_mutable: HashSet, // Mapping of an array that comes from a load and whether the address @@ -64,15 +66,9 @@ struct Context<'f> { } impl<'f> Context<'f> { - fn new( - dfg: &'f DataFlowGraph, - function_parameters: &'f [ValueId], - is_brillig_runtime: bool, - ) -> Self { + fn new(dfg: &'f DataFlowGraph) -> Self { Context { dfg, - function_parameters, - is_brillig_runtime, array_to_last_use: HashMap::default(), instructions_that_can_be_made_mutable: HashSet::default(), arrays_from_load: HashMap::default(), @@ -94,21 +90,12 @@ impl<'f> Context<'f> { self.instructions_that_can_be_made_mutable.remove(&existing); } } - Instruction::ArraySet { array, value, .. } => { + Instruction::ArraySet { array, .. } => { let array = self.dfg.resolve(*array); if let Some(existing) = self.array_to_last_use.insert(array, *instruction_id) { self.instructions_that_can_be_made_mutable.remove(&existing); } - if self.is_brillig_runtime { - let value = self.dfg.resolve(*value); - - if let Some(existing) = self.inner_nested_arrays.get(&value) { - self.instructions_that_can_be_made_mutable.remove(existing); - } - let result = self.dfg.instruction_results(*instruction_id)[0]; - self.inner_nested_arrays.insert(result, *instruction_id); - } // If the array we are setting does not come from a load we can safely mark it mutable. // If the array comes from a load we may potentially being mutating an array at a reference @@ -128,17 +115,13 @@ impl<'f> Context<'f> { } }); - // We cannot safely mutate slices that are inputs to the function, as they might be shared with the caller. - // NB checking the block parameters is not enough, as we might have jumped into a parameterless blocks inside the function. - let is_function_param = self.function_parameters.contains(&array); - let can_mutate = if let Some(is_from_param) = self.arrays_from_load.get(&array) { // If the array was loaded from a reference parameter, we cannot // safely mark that array mutable as it may be shared by another value. !is_from_param && is_return_block } else { - !is_array_in_terminator && !is_function_param + !is_array_in_terminator }; if can_mutate { diff --git a/compiler/noirc_evaluator/src/ssa/opt/mod.rs b/compiler/noirc_evaluator/src/ssa/opt/mod.rs index 5576b494570..098f62bceba 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mod.rs @@ -44,7 +44,7 @@ pub(crate) fn assert_normalized_ssa_equals(mut ssa: super::Ssa, expected: &str) let expected = trim_leading_whitespace_from_lines(expected); if ssa != expected { - println!("Got:\n~~~\n{}\n~~~\nExpected:\n~~~\n{}\n~~~", ssa, expected); - similar_asserts::assert_eq!(ssa, expected); + println!("Expected:\n~~~\n{expected}\n~~~\nGot:\n~~~\n{ssa}\n~~~"); + similar_asserts::assert_eq!(expected, ssa); } } diff --git a/compiler/noirc_evaluator/src/ssa/parser/tests.rs b/compiler/noirc_evaluator/src/ssa/parser/tests.rs index 3ed6be57b5e..9205353151e 100644 --- a/compiler/noirc_evaluator/src/ssa/parser/tests.rs +++ b/compiler/noirc_evaluator/src/ssa/parser/tests.rs @@ -425,3 +425,14 @@ fn test_slice() { "; assert_ssa_roundtrip(src); } + +#[test] +fn test_negative() { + let src = " + acir(inline) fn main f0 { + b0(): + return Field -1 + } + "; + assert_ssa_roundtrip(src); +} diff --git a/compiler/noirc_frontend/src/elaborator/comptime.rs b/compiler/noirc_frontend/src/elaborator/comptime.rs index 279adc331ea..a27e2bf0163 100644 --- a/compiler/noirc_frontend/src/elaborator/comptime.rs +++ b/compiler/noirc_frontend/src/elaborator/comptime.rs @@ -452,7 +452,14 @@ impl<'context> Elaborator<'context> { } ItemKind::Impl(r#impl) => { let module = self.module_id(); - dc_mod::collect_impl(self.interner, generated_items, r#impl, self.file, module); + dc_mod::collect_impl( + self.interner, + generated_items, + r#impl, + self.file, + module, + &mut self.errors, + ); } ItemKind::ModuleDecl(_) diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index 8a6c46ca50c..80c1ee217c2 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -42,7 +42,7 @@ use crate::{ }; use self::builtin_helpers::{eq_item, get_array, get_ctstring, get_str, get_u8, hash_item, lex}; -use super::Interpreter; +use super::{foreign, Interpreter}; pub(crate) mod builtin_helpers; @@ -57,6 +57,7 @@ impl<'local, 'context> Interpreter<'local, 'context> { let interner = &mut self.elaborator.interner; let call_stack = &self.elaborator.interpreter_call_stack; match name { + "apply_range_constraint" => foreign::apply_range_constraint(arguments, location), "array_as_str_unchecked" => array_as_str_unchecked(interner, arguments, location), "array_len" => array_len(interner, arguments, location), "assert_constant" => Ok(Value::Bool(true)), diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/foreign.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/foreign.rs index d1ab6a1dabd..3de72969cab 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/foreign.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/foreign.rs @@ -1,4 +1,6 @@ -use acvm::blackbox_solver::BlackBoxFunctionSolver; +use acvm::{ + acir::BlackBoxFunc, blackbox_solver::BlackBoxFunctionSolver, AcirField, BlackBoxResolutionError, +}; use bn254_blackbox_solver::Bn254BlackBoxSolver; use im::Vector; use iter_extended::try_vecmap; @@ -29,6 +31,28 @@ pub(super) fn call_foreign( } } +pub(super) fn apply_range_constraint( + arguments: Vec<(Value, Location)>, + location: Location, +) -> IResult { + let (value, num_bits) = check_two_arguments(arguments, location)?; + + let input = get_field(value)?; + let num_bits = get_u32(num_bits)?; + + if input.num_bits() < num_bits { + Ok(Value::Unit) + } else { + Err(InterpreterError::BlackBoxError( + BlackBoxResolutionError::Failed( + BlackBoxFunc::RANGE, + "value exceeds range check bounds".to_owned(), + ), + location, + )) + } +} + // poseidon2_permutation(_input: [Field; N], _state_length: u32) -> [Field; N] fn poseidon2_permutation( interner: &mut NodeInterner, diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index a373441b4e0..bae57daae15 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -97,9 +97,9 @@ pub fn collect_defs( errors.extend(collector.collect_functions(context, ast.functions, crate_id)); - collector.collect_trait_impls(context, ast.trait_impls, crate_id); + errors.extend(collector.collect_trait_impls(context, ast.trait_impls, crate_id)); - collector.collect_impls(context, ast.impls, crate_id); + errors.extend(collector.collect_impls(context, ast.impls, crate_id)); collector.collect_attributes( ast.inner_attributes, @@ -163,7 +163,13 @@ impl<'a> ModCollector<'a> { errors } - fn collect_impls(&mut self, context: &mut Context, impls: Vec, krate: CrateId) { + fn collect_impls( + &mut self, + context: &mut Context, + impls: Vec, + krate: CrateId, + ) -> Vec<(CompilationError, FileId)> { + let mut errors = Vec::new(); let module_id = ModuleId { krate, local_id: self.module_id }; for r#impl in impls { @@ -173,8 +179,11 @@ impl<'a> ModCollector<'a> { r#impl, self.file_id, module_id, + &mut errors, ); } + + errors } fn collect_trait_impls( @@ -182,7 +191,9 @@ impl<'a> ModCollector<'a> { context: &mut Context, impls: Vec, krate: CrateId, - ) { + ) -> Vec<(CompilationError, FileId)> { + let mut errors = Vec::new(); + for mut trait_impl in impls { let trait_name = trait_impl.trait_name.clone(); @@ -198,6 +209,13 @@ impl<'a> ModCollector<'a> { let module = ModuleId { krate, local_id: self.module_id }; for (_, func_id, noir_function) in &mut unresolved_functions.functions { + if noir_function.def.attributes.is_test_function() { + let error = DefCollectorErrorKind::TestOnAssociatedFunction { + span: noir_function.name_ident().span(), + }; + errors.push((error.into(), self.file_id)); + } + let location = Location::new(noir_function.def.span, self.file_id); context.def_interner.push_function(*func_id, &noir_function.def, module, location); } @@ -224,6 +242,8 @@ impl<'a> ModCollector<'a> { self.def_collector.items.trait_impls.push(unresolved_trait_impl); } + + errors } fn collect_functions( @@ -1051,6 +1071,7 @@ pub fn collect_impl( r#impl: TypeImpl, file_id: FileId, module_id: ModuleId, + errors: &mut Vec<(CompilationError, FileId)>, ) { let mut unresolved_functions = UnresolvedFunctions { file_id, functions: Vec::new(), trait_id: None, self_type: None }; @@ -1058,6 +1079,15 @@ pub fn collect_impl( for (method, _) in r#impl.methods { let doc_comments = method.doc_comments; let mut method = method.item; + + if method.def.attributes.is_test_function() { + let error = DefCollectorErrorKind::TestOnAssociatedFunction { + span: method.name_ident().span(), + }; + errors.push((error.into(), file_id)); + continue; + } + let func_id = interner.push_empty_fn(); method.def.where_clause.extend(r#impl.where_clause.clone()); let location = Location::new(method.span(), file_id); diff --git a/compiler/noirc_frontend/src/hir/def_collector/errors.rs b/compiler/noirc_frontend/src/hir/def_collector/errors.rs index d72f493092d..c08b4ff2062 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/errors.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/errors.rs @@ -82,6 +82,8 @@ pub enum DefCollectorErrorKind { }, #[error("{0}")] UnsupportedNumericGenericType(#[from] UnsupportedNumericGenericType), + #[error("The `#[test]` attribute may only be used on a non-associated function")] + TestOnAssociatedFunction { span: Span }, } impl DefCollectorErrorKind { @@ -291,6 +293,12 @@ impl<'a> From<&'a DefCollectorErrorKind> for Diagnostic { diag } DefCollectorErrorKind::UnsupportedNumericGenericType(err) => err.into(), + DefCollectorErrorKind::TestOnAssociatedFunction { span } => Diagnostic::simple_error( + "The `#[test]` attribute is disallowed on `impl` methods".into(), + String::new(), + *span, + ), + } } } diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index b709436cccc..23f134cc2a3 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -3692,3 +3692,51 @@ fn allows_struct_with_generic_infix_type_as_main_input_3() { "#; assert_no_errors(src); } + +#[test] +fn disallows_test_attribute_on_impl_method() { + let src = r#" + pub struct Foo {} + impl Foo { + #[test] + fn foo() {} + } + + fn main() {} + "#; + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + assert!(matches!( + errors[0].0, + CompilationError::DefinitionError(DefCollectorErrorKind::TestOnAssociatedFunction { + span: _ + }) + )); +} + +#[test] +fn disallows_test_attribute_on_trait_impl_method() { + let src = r#" + pub trait Trait { + fn foo() {} + } + + pub struct Foo {} + impl Trait for Foo { + #[test] + fn foo() {} + } + + fn main() {} + "#; + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + assert!(matches!( + errors[0].0, + CompilationError::DefinitionError(DefCollectorErrorKind::TestOnAssociatedFunction { + span: _ + }) + )); +} diff --git a/test_programs/compile_failure/comptime_apply_failing_range_constraint/Nargo.toml b/test_programs/compile_failure/comptime_apply_failing_range_constraint/Nargo.toml new file mode 100644 index 00000000000..e14cfeae5d6 --- /dev/null +++ b/test_programs/compile_failure/comptime_apply_failing_range_constraint/Nargo.toml @@ -0,0 +1,6 @@ +[package] +name = "comptime_apply_failing_range_constraint" +type = "bin" +authors = [""] + +[dependencies] diff --git a/test_programs/compile_failure/comptime_apply_failing_range_constraint/src/main.nr b/test_programs/compile_failure/comptime_apply_failing_range_constraint/src/main.nr new file mode 100644 index 00000000000..752f13a2e51 --- /dev/null +++ b/test_programs/compile_failure/comptime_apply_failing_range_constraint/src/main.nr @@ -0,0 +1,5 @@ +fn main() { + comptime { + 256.assert_max_bit_size::<8>() + } +} diff --git a/test_programs/compile_success_empty/comptime_apply_range_constraint/Nargo.toml b/test_programs/compile_success_empty/comptime_apply_range_constraint/Nargo.toml new file mode 100644 index 00000000000..84d16060440 --- /dev/null +++ b/test_programs/compile_success_empty/comptime_apply_range_constraint/Nargo.toml @@ -0,0 +1,6 @@ +[package] +name = "comptime_apply_range_constraint" +type = "bin" +authors = [""] + +[dependencies] diff --git a/test_programs/compile_success_empty/comptime_apply_range_constraint/src/main.nr b/test_programs/compile_success_empty/comptime_apply_range_constraint/src/main.nr new file mode 100644 index 00000000000..ff5e0ba9511 --- /dev/null +++ b/test_programs/compile_success_empty/comptime_apply_range_constraint/src/main.nr @@ -0,0 +1,5 @@ +fn main() { + comptime { + 2.assert_max_bit_size::<8>() + } +}