diff --git a/.noir-sync-commit b/.noir-sync-commit index b8333111c299..14513b410b03 100644 --- a/.noir-sync-commit +++ b/.noir-sync-commit @@ -1 +1 @@ -60c770f5f2594eea31ac75c852980edefa40d9eb +075c3d32481314d900cbdea0d277de83444747ab diff --git a/noir/noir-repo/acvm-repo/acir_field/src/field_element.rs b/noir/noir-repo/acvm-repo/acir_field/src/field_element.rs index 2323f008dbeb..47ceb9031116 100644 --- a/noir/noir-repo/acvm-repo/acir_field/src/field_element.rs +++ b/noir/noir-repo/acvm-repo/acir_field/src/field_element.rs @@ -8,7 +8,7 @@ use std::ops::{Add, AddAssign, Div, Mul, Neg, Sub, SubAssign}; use crate::AcirField; // XXX: Switch out for a trait and proper implementations -// This implementation is in-efficient, can definitely remove hex usage and Iterator instances for trivial functionality +// This implementation is inefficient, can definitely remove hex usage and Iterator instances for trivial functionality #[derive(Default, Clone, Copy, Eq, PartialOrd, Ord)] pub struct FieldElement(F); @@ -33,46 +33,6 @@ impl std::fmt::Display for FieldElement { write!(f, "-")?; } - // Number of bits needed to represent the smaller representation - let num_bits = smaller_repr.bits(); - - // Check if the number represents a power of 2 - if smaller_repr.count_ones() == 1 { - let mut bit_index = 0; - for i in 0..num_bits { - if smaller_repr.bit(i) { - bit_index = i; - break; - } - } - return match bit_index { - 0 => write!(f, "1"), - 1 => write!(f, "2"), - 2 => write!(f, "4"), - 3 => write!(f, "8"), - _ => write!(f, "2{}", superscript(bit_index)), - }; - } - - // Check if number is a multiple of a power of 2. - // This is used because when computing the quotient - // we usually have numbers in the form 2^t * q + r - // We focus on 2^64, 2^32, 2^16, 2^8, 2^4 because - // they are common. We could extend this to a more - // general factorization strategy, but we pay in terms of CPU time - let mul_sign = "×"; - for power in [64, 32, 16, 8, 4] { - let power_of_two = BigUint::from(2_u128).pow(power); - if &smaller_repr % &power_of_two == BigUint::zero() { - return write!( - f, - "2{}{}{}", - superscript(power as u64), - mul_sign, - smaller_repr / &power_of_two, - ); - } - } write!(f, "{smaller_repr}") } } @@ -409,35 +369,6 @@ impl SubAssign for FieldElement { } } -// For pretty printing powers -fn superscript(n: u64) -> String { - if n == 0 { - "⁰".to_owned() - } else if n == 1 { - "¹".to_owned() - } else if n == 2 { - "²".to_owned() - } else if n == 3 { - "³".to_owned() - } else if n == 4 { - "⁴".to_owned() - } else if n == 5 { - "⁵".to_owned() - } else if n == 6 { - "⁶".to_owned() - } else if n == 7 { - "⁷".to_owned() - } else if n == 8 { - "⁸".to_owned() - } else if n == 9 { - "⁹".to_owned() - } else if n >= 10 { - superscript(n / 10) + &superscript(n % 10) - } else { - panic!("{}", n.to_string() + " can't be converted to superscript."); - } -} - #[cfg(test)] mod tests { use super::{AcirField, FieldElement}; diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_black_box.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_black_box.rs index 55f89dcb30fd..3685c9540f3f 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_black_box.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_black_box.rs @@ -8,7 +8,7 @@ use acvm::{ use crate::brillig::brillig_ir::{ brillig_variable::BrilligVariable, debug_show::DebugToString, registers::RegisterAllocator, - BrilligBinaryOp, BrilligContext, + BrilligContext, }; /// Transforms SSA's black box function calls into the corresponding brillig instructions @@ -395,19 +395,8 @@ pub(crate) fn convert_black_box_call BrilligBlock<'block> { match output_register { // Returned vectors need to emit some bytecode to format the result as a BrilligVector ValueOrArray::HeapVector(heap_vector) => { - // Update the stack pointer so that we do not overwrite - // dynamic memory returned from other external calls - // Single values and allocation of fixed sized arrays has already been handled - // inside of `allocate_external_call_result` - let total_size = self.brillig_context.allocate_register(); - self.brillig_context.codegen_usize_op( - heap_vector.size, - total_size, - BrilligBinaryOp::Add, - 2, // RC and Length + self.brillig_context.initialize_externally_returned_vector( + output_variable.extract_vector(), + *heap_vector, ); - - self.brillig_context - .increase_free_memory_pointer_instruction(total_size); - let brillig_vector = output_variable.extract_vector(); - let size_pointer = self.brillig_context.allocate_register(); - - self.brillig_context.codegen_usize_op( - brillig_vector.pointer, - size_pointer, - BrilligBinaryOp::Add, - 1_usize, // Slices are [RC, Size, ...items] - ); - self.brillig_context - .store_instruction(size_pointer, heap_vector.size); - self.brillig_context.deallocate_register(size_pointer); - self.brillig_context.deallocate_register(total_size); - // Update the dynamic slice length maintained in SSA if let ValueOrArray::MemoryAddress(len_index) = output_values[i - 1] { @@ -515,8 +491,11 @@ impl<'block> BrilligBlock<'block> { element_size, ); - self.brillig_context - .codegen_initialize_vector(destination_vector, source_size_register); + self.brillig_context.codegen_initialize_vector( + destination_vector, + source_size_register, + None, + ); // Items let vector_items_pointer = @@ -1575,7 +1554,7 @@ impl<'block> BrilligBlock<'block> { let size = self .brillig_context .make_usize_constant_instruction(array.len().into()); - self.brillig_context.codegen_initialize_vector(vector, size); + self.brillig_context.codegen_initialize_vector(vector, size, None); self.brillig_context.deallocate_single_addr(size); } _ => unreachable!( @@ -1821,11 +1800,6 @@ impl<'block> BrilligBlock<'block> { // The stack pointer will then be updated by the caller of this method // once the external call is resolved and the array size is known self.brillig_context.load_free_memory_pointer_instruction(vector.pointer); - self.brillig_context.indirect_const_instruction( - vector.pointer, - BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, - 1_usize.into(), - ); variable } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs index 76e35395dd6e..26c7151bf072 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_slice_ops.rs @@ -79,17 +79,15 @@ impl<'block> BrilligBlock<'block> { source_vector: BrilligVector, removed_items: &[BrilligVariable], ) { - let read_pointer = self.brillig_context.allocate_register(); - self.brillig_context.call_vector_pop_procedure( + let read_pointer = self.brillig_context.codegen_make_vector_items_pointer(source_vector); + self.read_variables(read_pointer, removed_items); + self.brillig_context.deallocate_register(read_pointer); + + self.brillig_context.call_vector_pop_front_procedure( source_vector, target_vector, - read_pointer, removed_items.len(), - false, ); - - self.read_variables(read_pointer, removed_items); - self.brillig_context.deallocate_register(read_pointer); } pub(crate) fn slice_pop_back_operation( @@ -99,12 +97,11 @@ impl<'block> BrilligBlock<'block> { removed_items: &[BrilligVariable], ) { let read_pointer = self.brillig_context.allocate_register(); - self.brillig_context.call_vector_pop_procedure( + self.brillig_context.call_vector_pop_back_procedure( source_vector, target_vector, read_pointer, removed_items.len(), - true, ); self.read_variables(read_pointer, removed_items); @@ -213,7 +210,7 @@ mod tests { push_back: bool, array: Vec, item_to_push: FieldElement, - mut expected_return: Vec, + expected_return: Vec, ) { let arguments = vec![ BrilligParameter::Slice( @@ -223,11 +220,15 @@ mod tests { BrilligParameter::SingleAddr(BRILLIG_MEMORY_ADDRESSING_BIT_SIZE), ]; let result_length = array.len() + 1; + assert_eq!(result_length, expected_return.len()); + let result_length_with_metadata = result_length + 2; // Leading length and capacity + + // Entry points don't support returning slices, so we implicitly cast the vector to an array + // With the metadata at the start. let returns = vec![BrilligParameter::Array( vec![BrilligParameter::SingleAddr(BRILLIG_MEMORY_ADDRESSING_BIT_SIZE)], - result_length + 1, // Leading length since the we return a vector + result_length_with_metadata, )]; - expected_return.insert(0, FieldElement::from(result_length)); let (_, mut function_context, mut context) = create_test_environment(); @@ -262,14 +263,17 @@ mod tests { let bytecode = create_entry_point_bytecode(context, arguments, returns).byte_code; let (vm, return_data_offset, return_data_size) = create_and_run_vm(array.into_iter().chain(vec![item_to_push]).collect(), &bytecode); - assert_eq!(return_data_size, expected_return.len()); - assert_eq!( - vm.get_memory()[return_data_offset..(return_data_offset + expected_return.len())] - .iter() - .map(|mem_val| mem_val.to_field()) - .collect::>(), - expected_return - ); + assert_eq!(return_data_size, result_length_with_metadata); + let mut returned_vector: Vec = vm.get_memory() + [return_data_offset..(return_data_offset + result_length_with_metadata)] + .iter() + .map(|mem_val| mem_val.to_field()) + .collect(); + let returned_size = returned_vector.remove(0); + assert_eq!(returned_size, result_length.into()); + let _returned_capacity = returned_vector.remove(0); + + assert_eq!(returned_vector, expected_return); } test_case_push( @@ -321,7 +325,7 @@ mod tests { fn test_case_pop( pop_back: bool, array: Vec, - mut expected_return_array: Vec, + expected_return_array: Vec, expected_return_item: FieldElement, ) { let arguments = vec![BrilligParameter::Slice( @@ -329,15 +333,18 @@ mod tests { array.len(), )]; let result_length = array.len() - 1; + assert_eq!(result_length, expected_return_array.len()); + let result_length_with_metadata = result_length + 2; // Leading length and capacity + // Entry points don't support returning slices, so we implicitly cast the vector to an array + // With the metadata at the start. let returns = vec![ + BrilligParameter::SingleAddr(BRILLIG_MEMORY_ADDRESSING_BIT_SIZE), BrilligParameter::Array( vec![BrilligParameter::SingleAddr(BRILLIG_MEMORY_ADDRESSING_BIT_SIZE)], - result_length + 1, + result_length_with_metadata, ), - BrilligParameter::SingleAddr(BRILLIG_MEMORY_ADDRESSING_BIT_SIZE), ]; - expected_return_array.insert(0, FieldElement::from(result_length)); let (_, mut function_context, mut context) = create_test_environment(); @@ -367,22 +374,28 @@ mod tests { ); } - context.codegen_return(&[target_vector.pointer, removed_item.address]); + context.codegen_return(&[removed_item.address, target_vector.pointer]); let bytecode = create_entry_point_bytecode(context, arguments, returns).byte_code; - let expected_return: Vec<_> = - expected_return_array.into_iter().chain(vec![expected_return_item]).collect(); + let (vm, return_data_offset, return_data_size) = create_and_run_vm(array.clone(), &bytecode); - assert_eq!(return_data_size, expected_return.len()); - - assert_eq!( - vm.get_memory()[return_data_offset..(return_data_offset + expected_return.len())] - .iter() - .map(|mem_val| mem_val.to_field()) - .collect::>(), - expected_return - ); + // vector + removed item + assert_eq!(return_data_size, result_length_with_metadata + 1); + + let mut return_data: Vec = vm.get_memory() + [return_data_offset..(return_data_offset + return_data_size)] + .iter() + .map(|mem_val| mem_val.to_field()) + .collect(); + let returned_item = return_data.remove(0); + assert_eq!(returned_item, expected_return_item); + + let returned_size = return_data.remove(0); + assert_eq!(returned_size, result_length.into()); + let _returned_capacity = return_data.remove(0); + + assert_eq!(return_data, expected_return_array); } test_case_pop( @@ -414,7 +427,7 @@ mod tests { array: Vec, item: FieldElement, index: FieldElement, - mut expected_return: Vec, + expected_return: Vec, ) { let arguments = vec![ BrilligParameter::Slice( @@ -425,11 +438,15 @@ mod tests { BrilligParameter::SingleAddr(BRILLIG_MEMORY_ADDRESSING_BIT_SIZE), ]; let result_length = array.len() + 1; + assert_eq!(result_length, expected_return.len()); + let result_length_with_metadata = result_length + 2; // Leading length and capacity + + // Entry points don't support returning slices, so we implicitly cast the vector to an array + // With the metadata at the start. let returns = vec![BrilligParameter::Array( vec![BrilligParameter::SingleAddr(BRILLIG_MEMORY_ADDRESSING_BIT_SIZE)], - result_length + 1, + result_length_with_metadata, )]; - expected_return.insert(0, FieldElement::from(result_length)); let (_, mut function_context, mut context) = create_test_environment(); @@ -461,15 +478,18 @@ mod tests { let bytecode = create_entry_point_bytecode(context, arguments, returns).byte_code; let (vm, return_data_offset, return_data_size) = create_and_run_vm(calldata, &bytecode); - assert_eq!(return_data_size, expected_return.len()); - - assert_eq!( - vm.get_memory()[return_data_offset..(return_data_offset + expected_return.len())] - .iter() - .map(|mem_val| mem_val.to_field()) - .collect::>(), - expected_return - ); + assert_eq!(return_data_size, result_length_with_metadata); + + let mut returned_vector: Vec = vm.get_memory() + [return_data_offset..(return_data_offset + result_length_with_metadata)] + .iter() + .map(|mem_val| mem_val.to_field()) + .collect(); + let returned_size = returned_vector.remove(0); + assert_eq!(returned_size, result_length.into()); + let _returned_capacity = returned_vector.remove(0); + + assert_eq!(returned_vector, expected_return); } test_case_insert( @@ -546,7 +566,7 @@ mod tests { fn test_case_remove( array: Vec, index: FieldElement, - mut expected_array: Vec, + expected_array: Vec, expected_removed_item: FieldElement, ) { let arguments = vec![ @@ -557,15 +577,16 @@ mod tests { BrilligParameter::SingleAddr(BRILLIG_MEMORY_ADDRESSING_BIT_SIZE), ]; let result_length = array.len() - 1; + assert_eq!(result_length, expected_array.len()); + let result_length_with_metadata = result_length + 2; // Leading length and capacity let returns = vec![ + BrilligParameter::SingleAddr(BRILLIG_MEMORY_ADDRESSING_BIT_SIZE), BrilligParameter::Array( vec![BrilligParameter::SingleAddr(BRILLIG_MEMORY_ADDRESSING_BIT_SIZE)], - result_length + 1, + result_length_with_metadata, ), - BrilligParameter::SingleAddr(BRILLIG_MEMORY_ADDRESSING_BIT_SIZE), ]; - expected_array.insert(0, FieldElement::from(result_length)); let (_, mut function_context, mut context) = create_test_environment(); @@ -592,24 +613,29 @@ mod tests { &[BrilligVariable::SingleAddr(removed_item)], ); - context.codegen_return(&[target_vector.pointer, removed_item.address]); + context.codegen_return(&[removed_item.address, target_vector.pointer]); let calldata: Vec<_> = array.into_iter().chain(vec![index]).collect(); let bytecode = create_entry_point_bytecode(context, arguments, returns).byte_code; let (vm, return_data_offset, return_data_size) = create_and_run_vm(calldata, &bytecode); - let expected_return: Vec<_> = - expected_array.into_iter().chain(vec![expected_removed_item]).collect(); - assert_eq!(return_data_size, expected_return.len()); + // vector + removed item + assert_eq!(return_data_size, result_length_with_metadata + 1); - assert_eq!( - vm.get_memory()[return_data_offset..(return_data_offset + expected_return.len())] - .iter() - .map(|mem_val| mem_val.to_field()) - .collect::>(), - expected_return - ); + let mut return_data: Vec = vm.get_memory() + [return_data_offset..(return_data_offset + return_data_size)] + .iter() + .map(|mem_val| mem_val.to_field()) + .collect(); + let returned_item = return_data.remove(0); + assert_eq!(returned_item, expected_removed_item); + + let returned_size = return_data.remove(0); + assert_eq!(returned_size, result_length.into()); + let _returned_capacity = return_data.remove(0); + + assert_eq!(return_data, expected_array); } test_case_remove( diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_control_flow.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_control_flow.rs index 6935ebb0f530..540d957d5be9 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_control_flow.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_control_flow.rs @@ -12,6 +12,41 @@ use super::{ }; impl BrilligContext { + pub(crate) fn codegen_generic_iteration( + &mut self, + make_iterator: impl FnOnce(&mut BrilligContext) -> T, + update_iterator: impl FnOnce(&mut BrilligContext, &T), + make_finish_condition: impl FnOnce(&mut BrilligContext, &T) -> SingleAddrVariable, + on_iteration: impl FnOnce(&mut BrilligContext, &T), + clean_iterator: impl FnOnce(&mut BrilligContext, T), + ) { + let iterator = make_iterator(self); + + let (loop_section, loop_label) = self.reserve_next_section_label(); + self.enter_section(loop_section); + + // Loop body + let should_end = make_finish_condition(self, &iterator); + + let (exit_loop_section, exit_loop_label) = self.reserve_next_section_label(); + + self.jump_if_instruction(should_end.address, exit_loop_label); + + // Call the on iteration function + on_iteration(self, &iterator); + + // Update iterator + update_iterator(self, &iterator); + self.jump_instruction(loop_label); + + // Exit the loop + self.enter_section(exit_loop_section); + + // Deallocate our temporary registers + self.deallocate_single_addr(should_end); + clean_iterator(self, iterator); + } + /// This codegen will issue a loop for (let iterator_register = loop_start; i < loop_bound; i += step) /// The body of the loop should be issued by the caller in the on_iteration closure. pub(crate) fn codegen_for_loop( diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_memory.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_memory.rs index 0199d9537a68..a34034bb5505 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_memory.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_memory.rs @@ -93,16 +93,125 @@ impl BrilligContext< ); } else { let value_register = self.allocate_register(); + let end_source_pointer = self.allocate_register(); + self.memory_op_instruction( + source_pointer, + num_elements_variable.address, + end_source_pointer, + BrilligBinaryOp::Add, + ); - self.codegen_loop(num_elements_variable.address, |ctx, iterator| { - ctx.codegen_load_with_offset(source_pointer, iterator, value_register); - ctx.codegen_store_with_offset(destination_pointer, iterator, value_register); - }); - + self.codegen_generic_iteration( + |brillig_context| { + let source_iterator = brillig_context.allocate_register(); + let target_iterator = brillig_context.allocate_register(); + + brillig_context.mov_instruction(source_iterator, source_pointer); + brillig_context.mov_instruction(target_iterator, destination_pointer); + + (source_iterator, target_iterator) + }, + |brillig_context, &(source_iterator, target_iterator)| { + brillig_context.codegen_usize_op_in_place( + source_iterator, + BrilligBinaryOp::Add, + 1, + ); + brillig_context.codegen_usize_op_in_place( + target_iterator, + BrilligBinaryOp::Add, + 1, + ); + }, + |brillig_context, &(source_iterator, _)| { + // We have finished when the source/target pointer is less than the source/target start + let finish_condition = + SingleAddrVariable::new(brillig_context.allocate_register(), 1); + brillig_context.memory_op_instruction( + source_iterator, + end_source_pointer, + finish_condition.address, + BrilligBinaryOp::Equals, + ); + finish_condition + }, + |brillig_context, &(source_iterator, target_iterator)| { + brillig_context.load_instruction(value_register, source_iterator); + brillig_context.store_instruction(target_iterator, value_register); + }, + |brillig_context, (source_iterator, target_iterator)| { + brillig_context.deallocate_register(source_iterator); + brillig_context.deallocate_register(target_iterator); + }, + ); self.deallocate_register(value_register); + self.deallocate_register(end_source_pointer); } } + /// Copies num_elements_variable from the source pointer to the target pointer, starting from the end + pub(crate) fn codegen_mem_copy_from_the_end( + &mut self, + source_start: MemoryAddress, + target_start: MemoryAddress, + num_elements_variable: SingleAddrVariable, + ) { + self.codegen_generic_iteration( + |brillig_context| { + // Create the pointer to the last item for both source and target + let num_items_minus_one = brillig_context.allocate_register(); + brillig_context.codegen_usize_op( + num_elements_variable.address, + num_items_minus_one, + BrilligBinaryOp::Sub, + 1, + ); + let target_pointer = brillig_context.allocate_register(); + brillig_context.memory_op_instruction( + target_start, + num_items_minus_one, + target_pointer, + BrilligBinaryOp::Add, + ); + let source_pointer = brillig_context.allocate_register(); + brillig_context.memory_op_instruction( + source_start, + num_items_minus_one, + source_pointer, + BrilligBinaryOp::Add, + ); + brillig_context.deallocate_register(num_items_minus_one); + (source_pointer, target_pointer) + }, + |brillig_context, &(source_pointer, target_pointer)| { + brillig_context.codegen_usize_op_in_place(source_pointer, BrilligBinaryOp::Sub, 1); + brillig_context.codegen_usize_op_in_place(target_pointer, BrilligBinaryOp::Sub, 1); + }, + |brillig_context, &(source_pointer, _)| { + // We have finished when the source/target pointer is less than the source/target start + let finish_condition = + SingleAddrVariable::new(brillig_context.allocate_register(), 1); + brillig_context.memory_op_instruction( + source_pointer, + source_start, + finish_condition.address, + BrilligBinaryOp::LessThan, + ); + finish_condition + }, + |brillig_context, &(source_pointer, target_pointer)| { + let value_register = brillig_context.allocate_register(); + brillig_context.load_instruction(value_register, source_pointer); + brillig_context.store_instruction(target_pointer, value_register); + brillig_context.deallocate_register(value_register); + }, + |brillig_context, (source_pointer, target_pointer)| { + brillig_context.deallocate_register(source_pointer); + brillig_context.deallocate_register(target_pointer); + }, + ); + } + /// This instruction will reverse the order of the `size` elements pointed by `pointer`. pub(crate) fn codegen_array_reverse( &mut self, @@ -170,7 +279,7 @@ impl BrilligContext< self.codegen_usize_op(vector.pointer, current_pointer, BrilligBinaryOp::Add, 1); self.load_instruction(heap_vector.size, current_pointer); // Now prepare the pointer to the items - self.codegen_usize_op(current_pointer, heap_vector.pointer, BrilligBinaryOp::Add, 1); + self.codegen_usize_op(current_pointer, heap_vector.pointer, BrilligBinaryOp::Add, 2); self.deallocate_register(current_pointer); heap_vector @@ -201,16 +310,73 @@ impl BrilligContext< result } + pub(crate) fn codegen_update_vector_length( + &mut self, + vector: BrilligVector, + new_length: SingleAddrVariable, + ) { + let write_pointer = self.allocate_register(); + self.codegen_usize_op(vector.pointer, write_pointer, BrilligBinaryOp::Add, 1); + self.store_instruction(write_pointer, new_length.address); + self.deallocate_register(write_pointer); + } + + /// Returns a variable holding the capacity of a given vector + pub(crate) fn codegen_make_vector_capacity( + &mut self, + vector: BrilligVector, + ) -> SingleAddrVariable { + let result = SingleAddrVariable::new_usize(self.allocate_register()); + self.codegen_usize_op(vector.pointer, result.address, BrilligBinaryOp::Add, 2); + self.load_instruction(result.address, result.address); + result + } + + /// Writes a pointer to the items of a given vector + pub(crate) fn codegen_vector_items_pointer( + &mut self, + vector: BrilligVector, + result: MemoryAddress, + ) { + self.codegen_usize_op(vector.pointer, result, BrilligBinaryOp::Add, 3); + } + /// Returns a pointer to the items of a given vector pub(crate) fn codegen_make_vector_items_pointer( &mut self, vector: BrilligVector, ) -> MemoryAddress { let result = self.allocate_register(); - self.codegen_usize_op(vector.pointer, result, BrilligBinaryOp::Add, 2); + self.codegen_vector_items_pointer(vector, result); result } + /// Reads the metadata of a vector and stores it in the given variables + pub(crate) fn codegen_read_vector_metadata( + &mut self, + vector: BrilligVector, + rc: SingleAddrVariable, + size: SingleAddrVariable, + capacity: SingleAddrVariable, + items_pointer: SingleAddrVariable, + ) { + assert!(rc.bit_size == BRILLIG_MEMORY_ADDRESSING_BIT_SIZE); + assert!(size.bit_size == BRILLIG_MEMORY_ADDRESSING_BIT_SIZE); + assert!(capacity.bit_size == BRILLIG_MEMORY_ADDRESSING_BIT_SIZE); + assert!(items_pointer.bit_size == BRILLIG_MEMORY_ADDRESSING_BIT_SIZE); + + self.load_instruction(rc.address, vector.pointer); + + let read_pointer = self.allocate_register(); + self.codegen_usize_op(vector.pointer, read_pointer, BrilligBinaryOp::Add, 1); + self.load_instruction(size.address, read_pointer); + self.codegen_usize_op_in_place(read_pointer, BrilligBinaryOp::Add, 1); + self.load_instruction(capacity.address, read_pointer); + self.codegen_usize_op(read_pointer, items_pointer.address, BrilligBinaryOp::Add, 1); + + self.deallocate_register(read_pointer); + } + /// Returns a variable holding the length of a given array pub(crate) fn codegen_make_array_length(&mut self, array: BrilligArray) -> SingleAddrVariable { let result = SingleAddrVariable::new_usize(self.allocate_register()); @@ -262,28 +428,88 @@ impl BrilligContext< ); } - /// Initializes a vector, allocating memory to store its representation and initializing the reference counter and size. + pub(crate) fn codegen_initialize_vector_metadata( + &mut self, + vector: BrilligVector, + size: SingleAddrVariable, + capacity: Option, + ) { + // Write RC + self.indirect_const_instruction( + vector.pointer, + BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, + 1_usize.into(), + ); + + // Write size + let write_pointer = self.allocate_register(); + self.codegen_usize_op(vector.pointer, write_pointer, BrilligBinaryOp::Add, 1); + self.store_instruction(write_pointer, size.address); + + // Write capacity + self.codegen_usize_op_in_place(write_pointer, BrilligBinaryOp::Add, 1); + self.store_instruction(write_pointer, capacity.unwrap_or(size).address); + + self.deallocate_register(write_pointer); + } + + /// Initializes a vector, allocating memory to store its representation and initializing the reference counter, size and capacity pub(crate) fn codegen_initialize_vector( &mut self, vector: BrilligVector, size: SingleAddrVariable, + capacity: Option, // Defaults to size if None ) { let allocation_size = self.allocate_register(); - self.codegen_usize_op(size.address, allocation_size, BrilligBinaryOp::Add, 2); + // Allocation size = capacity + 3 (rc, size, capacity) + self.codegen_usize_op( + capacity.unwrap_or(size).address, + allocation_size, + BrilligBinaryOp::Add, + 3, + ); self.codegen_allocate_mem(vector.pointer, allocation_size); self.deallocate_register(allocation_size); - // Write RC + self.codegen_initialize_vector_metadata(vector, size, capacity); + } + + /// We don't know the length of a vector returned externally before the call + /// so we pass the free memory pointer and then use this function to allocate + /// after the fact when we know the length. + pub(crate) fn initialize_externally_returned_vector( + &mut self, + vector: BrilligVector, + resulting_heap_vector: HeapVector, + ) { + let total_size = self.allocate_register(); + self.codegen_usize_op( + resulting_heap_vector.size, + total_size, + BrilligBinaryOp::Add, + 3, // Rc, length and capacity + ); + + self.increase_free_memory_pointer_instruction(total_size); + let write_pointer = self.allocate_register(); + + // Vectors are [RC, Size, Capacity, ...items] + // Initialize RC self.indirect_const_instruction( vector.pointer, BRILLIG_MEMORY_ADDRESSING_BIT_SIZE, 1_usize.into(), ); - // Write size - let len_write_pointer = self.allocate_register(); - self.codegen_usize_op(vector.pointer, len_write_pointer, BrilligBinaryOp::Add, 1); - self.store_instruction(len_write_pointer, size.address); - self.deallocate_register(len_write_pointer); + // Initialize size + self.codegen_usize_op(vector.pointer, write_pointer, BrilligBinaryOp::Add, 1_usize); + self.store_instruction(write_pointer, resulting_heap_vector.size); + + // Initialize capacity + self.codegen_usize_op_in_place(write_pointer, BrilligBinaryOp::Add, 1_usize); + self.store_instruction(write_pointer, resulting_heap_vector.size); + + self.deallocate_register(write_pointer); + self.deallocate_register(total_size); } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs index 75d91716c23e..b78dcb09d9a2 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/entry_point.rs @@ -196,7 +196,7 @@ impl BrilligContext { let deflattened_items_pointer = if is_vector { let vector = BrilligVector { pointer: deflattened_array_pointer }; - self.codegen_initialize_vector(vector, deflattened_size_variable); + self.codegen_initialize_vector(vector, deflattened_size_variable, None); self.codegen_make_vector_items_pointer(vector) } else { diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs index 0ee6fe494350..1c3d1f4d0ad1 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/mod.rs @@ -5,7 +5,8 @@ mod mem_copy; mod prepare_vector_insert; mod prepare_vector_push; mod vector_copy; -mod vector_pop; +mod vector_pop_back; +mod vector_pop_front; mod vector_remove; use array_copy::compile_array_copy_procedure; @@ -15,7 +16,8 @@ use mem_copy::compile_mem_copy_procedure; use prepare_vector_insert::compile_prepare_vector_insert_procedure; use prepare_vector_push::compile_prepare_vector_push_procedure; use vector_copy::compile_vector_copy_procedure; -use vector_pop::compile_vector_pop_procedure; +use vector_pop_back::compile_vector_pop_back_procedure; +use vector_pop_front::compile_vector_pop_front_procedure; use vector_remove::compile_vector_remove_procedure; use crate::brillig::brillig_ir::AcirField; @@ -36,7 +38,8 @@ pub(crate) enum ProcedureId { VectorCopy, MemCopy, PrepareVectorPush(bool), - VectorPop(bool), + VectorPopFront, + VectorPopBack, PrepareVectorInsert, VectorRemove, CheckMaxStackDepth, @@ -56,8 +59,11 @@ pub(crate) fn compile_procedure( ProcedureId::PrepareVectorPush(push_back) => { compile_prepare_vector_push_procedure(&mut brillig_context, push_back); } - ProcedureId::VectorPop(pop_back) => { - compile_vector_pop_procedure(&mut brillig_context, pop_back); + ProcedureId::VectorPopFront => { + compile_vector_pop_front_procedure(&mut brillig_context); + } + ProcedureId::VectorPopBack => { + compile_vector_pop_back_procedure(&mut brillig_context); } ProcedureId::PrepareVectorInsert => { compile_prepare_vector_insert_procedure(&mut brillig_context); diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/prepare_vector_insert.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/prepare_vector_insert.rs index 8dbbf80782cb..1c1a738509cb 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/prepare_vector_insert.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/prepare_vector_insert.rs @@ -2,7 +2,7 @@ use std::vec; use acvm::{acir::brillig::MemoryAddress, AcirField}; -use super::ProcedureId; +use super::{prepare_vector_push::reallocate_vector_for_insertion, ProcedureId}; use crate::brillig::brillig_ir::{ brillig_variable::{BrilligVector, SingleAddrVariable}, debug_show::DebugToString, @@ -58,9 +58,19 @@ pub(super) fn compile_prepare_vector_insert_procedure( + brillig_context: &mut BrilligContext, + source_vector: BrilligVector, + source_rc: SingleAddrVariable, + source_capacity: SingleAddrVariable, + target_vector: BrilligVector, + target_size: SingleAddrVariable, +) { + let does_capacity_fit = SingleAddrVariable::new(brillig_context.allocate_register(), 1); + brillig_context.memory_op_instruction( + target_size.address, + source_capacity.address, + does_capacity_fit.address, + BrilligBinaryOp::LessThanEquals, + ); + + let is_rc_one = SingleAddrVariable::new(brillig_context.allocate_register(), 1); + brillig_context.codegen_usize_op( + source_rc.address, + is_rc_one.address, + BrilligBinaryOp::Equals, + 1, + ); + + // Reallocate target vector for insertion + brillig_context.codegen_branch( + does_capacity_fit.address, + |brillig_context, does_capacity_fit| { + if does_capacity_fit { + brillig_context.codegen_branch(is_rc_one.address, |brillig_context, is_rc_one| { + if is_rc_one { + // We can insert in place, so we can just move the source pointer to the destination pointer and update the length + brillig_context + .mov_instruction(target_vector.pointer, source_vector.pointer); + brillig_context.codegen_update_vector_length(target_vector, target_size); + } else { + brillig_context.codegen_initialize_vector( + target_vector, + target_size, + Some(source_capacity), + ); + } + }); + } else { + let double_size = + SingleAddrVariable::new_usize(brillig_context.allocate_register()); + brillig_context.codegen_usize_op( + target_size.address, + double_size.address, + BrilligBinaryOp::Mul, + 2_usize, + ); + brillig_context.codegen_initialize_vector( + target_vector, + target_size, + Some(double_size), + ); + brillig_context.deallocate_single_addr(double_size); + } + }, + ); + + brillig_context.deallocate_single_addr(is_rc_one); + brillig_context.deallocate_single_addr(does_capacity_fit); +} diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_copy.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_copy.rs index 7695e840c0b3..6d2f9c4afb4f 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_copy.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_copy.rs @@ -53,8 +53,8 @@ pub(super) fn compile_vector_copy_procedure( let result_vector = BrilligVector { pointer: new_vector_pointer_return }; // Allocate the memory for the new vec - let allocation_size = ctx.codegen_make_vector_length(source_vector); - ctx.codegen_usize_op_in_place(allocation_size.address, BrilligBinaryOp::Add, 2_usize); + let allocation_size = ctx.codegen_make_vector_capacity(source_vector); + ctx.codegen_usize_op_in_place(allocation_size.address, BrilligBinaryOp::Add, 3_usize); // Capacity plus 3 (rc, len, cap) ctx.codegen_allocate_mem(result_vector.pointer, allocation_size.address); ctx.codegen_mem_copy(source_vector.pointer, result_vector.pointer, allocation_size); diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_pop.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_pop_back.rs similarity index 60% rename from noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_pop.rs rename to noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_pop_back.rs index 8fcfebb2360e..bfc9d5128526 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_pop.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_pop_back.rs @@ -11,14 +11,13 @@ use crate::brillig::brillig_ir::{ }; impl BrilligContext { - /// Pops items from the vector, returning the new vector and the pointer to the popped items in read_pointer. - pub(crate) fn call_vector_pop_procedure( + /// Pops items from the back of a vector, returning the new vector and the pointer to the popped items in read_pointer. + pub(crate) fn call_vector_pop_back_procedure( &mut self, source_vector: BrilligVector, destination_vector: BrilligVector, read_pointer: MemoryAddress, item_pop_count: usize, - back: bool, ) { let source_vector_pointer_arg = MemoryAddress::direct(ScratchSpace::start()); let item_pop_count_arg = MemoryAddress::direct(ScratchSpace::start() + 1); @@ -28,16 +27,15 @@ impl BrilligContext< self.mov_instruction(source_vector_pointer_arg, source_vector.pointer); self.usize_const_instruction(item_pop_count_arg, item_pop_count.into()); - self.add_procedure_call_instruction(ProcedureId::VectorPop(back)); + self.add_procedure_call_instruction(ProcedureId::VectorPopBack); self.mov_instruction(destination_vector.pointer, new_vector_pointer_return); self.mov_instruction(read_pointer, read_pointer_return); } } -pub(super) fn compile_vector_pop_procedure( +pub(super) fn compile_vector_pop_back_procedure( brillig_context: &mut BrilligContext, - pop_back: bool, ) { let source_vector_pointer_arg = MemoryAddress::direct(ScratchSpace::start()); let item_pop_count_arg = MemoryAddress::direct(ScratchSpace::start() + 1); @@ -65,49 +63,47 @@ pub(super) fn compile_vector_pop_procedure( BrilligBinaryOp::Sub, ); - brillig_context.codegen_initialize_vector(target_vector, target_size); + let rc = brillig_context.allocate_register(); + brillig_context.load_instruction(rc, source_vector.pointer); + let is_rc_one = brillig_context.allocate_register(); + brillig_context.codegen_usize_op(rc, is_rc_one, BrilligBinaryOp::Equals, 1_usize); - // Now we offset the source pointer by removed_items.len() let source_vector_items_pointer = brillig_context.codegen_make_vector_items_pointer(source_vector); - let target_vector_items_pointer = - brillig_context.codegen_make_vector_items_pointer(target_vector); - - if pop_back { - // Now we copy the source vector starting at index 0 into the target vector - brillig_context.codegen_mem_copy( - source_vector_items_pointer, - target_vector_items_pointer, - target_size, - ); - brillig_context.memory_op_instruction( - source_vector_items_pointer, - target_size.address, - read_pointer_return, - BrilligBinaryOp::Add, - ); - } else { - let source_copy_pointer = brillig_context.allocate_register(); - brillig_context.memory_op_instruction( - source_vector_items_pointer, - item_pop_count_arg, - source_copy_pointer, - BrilligBinaryOp::Add, - ); - - // Now we copy the source vector starting at index removed_items.len() into the target vector - brillig_context.codegen_mem_copy( - source_copy_pointer, - target_vector_items_pointer, - target_size, - ); - brillig_context.mov_instruction(read_pointer_return, source_vector_items_pointer); - - brillig_context.deallocate_register(source_copy_pointer); - } + + brillig_context.codegen_branch(is_rc_one, |brillig_context, is_rc_one| { + if is_rc_one { + // We can reuse the source vector updating its length + brillig_context.mov_instruction(target_vector.pointer, source_vector.pointer); + brillig_context.codegen_update_vector_length(target_vector, target_size); + } else { + // We need to clone the source vector + brillig_context.codegen_initialize_vector(target_vector, target_size, None); + + let target_vector_items_pointer = + brillig_context.codegen_make_vector_items_pointer(target_vector); + + // Now we copy the source vector starting at index 0 into the target vector but with the reduced length + brillig_context.codegen_mem_copy( + source_vector_items_pointer, + target_vector_items_pointer, + target_size, + ); + brillig_context.deallocate_register(target_vector_items_pointer); + } + }); + + brillig_context.memory_op_instruction( + source_vector_items_pointer, + target_size.address, + read_pointer_return, + BrilligBinaryOp::Add, + ); + + brillig_context.deallocate_register(rc); + brillig_context.deallocate_register(is_rc_one); + brillig_context.deallocate_register(source_vector_items_pointer); brillig_context.deallocate_single_addr(source_size); brillig_context.deallocate_single_addr(target_size); - brillig_context.deallocate_register(source_vector_items_pointer); - brillig_context.deallocate_register(target_vector_items_pointer); } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_pop_front.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_pop_front.rs new file mode 100644 index 000000000000..49123ca2f500 --- /dev/null +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_pop_front.rs @@ -0,0 +1,130 @@ +use std::vec; + +use acvm::{acir::brillig::MemoryAddress, AcirField}; + +use super::ProcedureId; +use crate::brillig::brillig_ir::{ + brillig_variable::{BrilligVector, SingleAddrVariable}, + debug_show::DebugToString, + registers::{RegisterAllocator, ScratchSpace}, + BrilligBinaryOp, BrilligContext, +}; + +impl BrilligContext { + /// Pops items from the front of a vector, returning the new vector + pub(crate) fn call_vector_pop_front_procedure( + &mut self, + source_vector: BrilligVector, + destination_vector: BrilligVector, + item_pop_count: usize, + ) { + let source_vector_pointer_arg = MemoryAddress::direct(ScratchSpace::start()); + let item_pop_count_arg = MemoryAddress::direct(ScratchSpace::start() + 1); + let new_vector_pointer_return = MemoryAddress::direct(ScratchSpace::start() + 2); + + self.mov_instruction(source_vector_pointer_arg, source_vector.pointer); + self.usize_const_instruction(item_pop_count_arg, item_pop_count.into()); + + self.add_procedure_call_instruction(ProcedureId::VectorPopFront); + + self.mov_instruction(destination_vector.pointer, new_vector_pointer_return); + } +} + +pub(super) fn compile_vector_pop_front_procedure( + brillig_context: &mut BrilligContext, +) { + let source_vector_pointer_arg = MemoryAddress::direct(ScratchSpace::start()); + let item_pop_count_arg = MemoryAddress::direct(ScratchSpace::start() + 1); + let new_vector_pointer_return = MemoryAddress::direct(ScratchSpace::start() + 2); + + brillig_context.set_allocated_registers(vec![ + source_vector_pointer_arg, + item_pop_count_arg, + new_vector_pointer_return, + ]); + + let source_vector = BrilligVector { pointer: source_vector_pointer_arg }; + let target_vector = BrilligVector { pointer: new_vector_pointer_return }; + + let source_rc = SingleAddrVariable::new_usize(brillig_context.allocate_register()); + let source_size = SingleAddrVariable::new_usize(brillig_context.allocate_register()); + let source_capacity = SingleAddrVariable::new_usize(brillig_context.allocate_register()); + let source_items_pointer = SingleAddrVariable::new_usize(brillig_context.allocate_register()); + brillig_context.codegen_read_vector_metadata( + source_vector, + source_rc, + source_size, + source_capacity, + source_items_pointer, + ); + + // target_size = source_size - item_pop_count + let target_size = SingleAddrVariable::new_usize(brillig_context.allocate_register()); + brillig_context.memory_op_instruction( + source_size.address, + item_pop_count_arg, + target_size.address, + BrilligBinaryOp::Sub, + ); + + let is_rc_one = brillig_context.allocate_register(); + brillig_context.codegen_usize_op( + source_rc.address, + is_rc_one, + BrilligBinaryOp::Equals, + 1_usize, + ); + + brillig_context.codegen_branch(is_rc_one, |brillig_context, is_rc_one| { + if is_rc_one { + // We reuse the source vector, moving the metadata to the right (decreasing capacity) + brillig_context.memory_op_instruction( + source_vector.pointer, + item_pop_count_arg, + target_vector.pointer, + BrilligBinaryOp::Add, + ); + brillig_context.memory_op_instruction( + source_capacity.address, + item_pop_count_arg, + source_capacity.address, + BrilligBinaryOp::Sub, + ); + brillig_context.codegen_initialize_vector_metadata( + target_vector, + target_size, + Some(source_capacity), + ); + } else { + brillig_context.codegen_initialize_vector(target_vector, target_size, None); + + let target_vector_items_pointer = + brillig_context.codegen_make_vector_items_pointer(target_vector); + + let source_copy_pointer = brillig_context.allocate_register(); + brillig_context.memory_op_instruction( + source_items_pointer.address, + item_pop_count_arg, + source_copy_pointer, + BrilligBinaryOp::Add, + ); + // Now we copy the source vector starting at index removed_items.len() into the target vector + brillig_context.codegen_mem_copy( + source_copy_pointer, + target_vector_items_pointer, + target_size, + ); + + brillig_context.deallocate_register(source_copy_pointer); + brillig_context.deallocate_register(target_vector_items_pointer); + } + }); + + brillig_context.deallocate_register(is_rc_one); + brillig_context.deallocate_single_addr(target_size); + brillig_context.deallocate_single_addr(source_rc); + brillig_context.deallocate_single_addr(source_size); + brillig_context.deallocate_single_addr(source_capacity); + brillig_context.deallocate_single_addr(source_items_pointer); +} diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_remove.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_remove.rs index b7b54f970fa5..7abc43286ee7 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_remove.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/procedures/vector_remove.rs @@ -53,7 +53,7 @@ pub(super) fn compile_vector_remove_procedure( let target_vector = BrilligVector { pointer: new_vector_pointer_return }; let index = SingleAddrVariable::new_usize(index_arg); - // First we need to allocate the target vector decrementing the size by removed_items.len() + // Reallocate if necessary let source_size = brillig_context.codegen_make_vector_length(source_vector); let target_size = SingleAddrVariable::new_usize(brillig_context.allocate_register()); @@ -64,19 +64,36 @@ pub(super) fn compile_vector_remove_procedure( BrilligBinaryOp::Sub, ); - brillig_context.codegen_initialize_vector(target_vector, target_size); + let rc = brillig_context.allocate_register(); + brillig_context.load_instruction(rc, source_vector.pointer); + let is_rc_one = brillig_context.allocate_register(); + brillig_context.codegen_usize_op(rc, is_rc_one, BrilligBinaryOp::Equals, 1_usize); - // Copy the elements to the left of the index let source_vector_items_pointer = brillig_context.codegen_make_vector_items_pointer(source_vector); - let target_vector_items_pointer = - brillig_context.codegen_make_vector_items_pointer(target_vector); - brillig_context.codegen_mem_copy( - source_vector_items_pointer, - target_vector_items_pointer, - index, - ); + let target_vector_items_pointer = brillig_context.allocate_register(); + + brillig_context.codegen_branch(is_rc_one, |brillig_context, is_rc_one| { + if is_rc_one { + brillig_context.mov_instruction(target_vector.pointer, source_vector.pointer); + brillig_context.codegen_update_vector_length(target_vector, target_size); + brillig_context + .codegen_vector_items_pointer(target_vector, target_vector_items_pointer); + } else { + brillig_context.codegen_initialize_vector(target_vector, target_size, None); + + // Copy the elements to the left of the index + brillig_context + .codegen_vector_items_pointer(target_vector, target_vector_items_pointer); + + brillig_context.codegen_mem_copy( + source_vector_items_pointer, + target_vector_items_pointer, + index, + ); + } + }); // Compute the source pointer after the removed items let source_pointer_after_index = brillig_context.allocate_register(); @@ -124,6 +141,8 @@ pub(super) fn compile_vector_remove_procedure( SingleAddrVariable::new_usize(item_count), ); + brillig_context.deallocate_register(rc); + brillig_context.deallocate_register(is_rc_one); brillig_context.deallocate_register(source_pointer_after_index); brillig_context.deallocate_register(target_pointer_at_index); brillig_context.deallocate_register(item_count); diff --git a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/expressions.rs b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/expressions.rs index 31a518ca97fb..6dfd295db6f8 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -8,7 +8,7 @@ use crate::{ ast::{ ArrayLiteral, BlockExpression, CallExpression, CastExpression, ConstructorExpression, Expression, ExpressionKind, Ident, IfExpression, IndexExpression, InfixExpression, - ItemVisibility, Lambda, Literal, MemberAccessExpression, MethodCallExpression, + ItemVisibility, Lambda, Literal, MemberAccessExpression, MethodCallExpression, Path, PrefixExpression, StatementKind, UnaryOp, UnresolvedTypeData, UnresolvedTypeExpression, }, hir::{ @@ -21,7 +21,7 @@ use crate::{ hir_def::{ expr::{ HirArrayLiteral, HirBinaryOp, HirBlockExpression, HirCallExpression, HirCastExpression, - HirConstructorExpression, HirExpression, HirIfExpression, HirIndexExpression, + HirConstructorExpression, HirExpression, HirIdent, HirIfExpression, HirIndexExpression, HirInfixExpression, HirLambda, HirLiteral, HirMemberAccess, HirMethodCallExpression, HirPrefixExpression, }, @@ -247,27 +247,35 @@ impl<'context> Elaborator<'context> { let scope_tree = self.scopes.current_scope_tree(); let variable = scope_tree.find(ident_name); - if let Some((old_value, _)) = variable { + + let hir_ident = if let Some((old_value, _)) = variable { old_value.num_times_used += 1; - let ident = HirExpression::Ident(old_value.ident.clone(), None); - let expr_id = self.interner.push_expr(ident); - self.interner.push_expr_location(expr_id, call_expr_span, self.file); - let ident = old_value.ident.clone(); - let typ = self.type_check_variable(ident, expr_id, None); - self.interner.push_expr_type(expr_id, typ.clone()); - capture_types.push(typ); - fmt_str_idents.push(expr_id); + old_value.ident.clone() + } else if let Ok((definition_id, _)) = + self.lookup_global(Path::from_single(ident_name.to_string(), call_expr_span)) + { + HirIdent::non_trait_method(definition_id, Location::new(call_expr_span, self.file)) } else if ident_name.parse::().is_ok() { self.push_err(ResolverError::NumericConstantInFormatString { name: ident_name.to_owned(), span: call_expr_span, }); + continue; } else { self.push_err(ResolverError::VariableNotDeclared { name: ident_name.to_owned(), span: call_expr_span, }); - } + continue; + }; + + let hir_expr = HirExpression::Ident(hir_ident.clone(), None); + let expr_id = self.interner.push_expr(hir_expr); + self.interner.push_expr_location(expr_id, call_expr_span, self.file); + let typ = self.type_check_variable(hir_ident, expr_id, None); + self.interner.push_expr_type(expr_id, typ.clone()); + capture_types.push(typ); + fmt_str_idents.push(expr_id); } let len = Type::Constant(str.len().into(), Kind::u32()); @@ -528,9 +536,6 @@ impl<'context> Elaborator<'context> { last_segment.generics = Some(generics.ordered_args); } - let exclude_last_segment = true; - self.check_unsupported_turbofish_usage(&path, exclude_last_segment); - let last_segment = path.last_segment(); let is_self_type = last_segment.ident.is_self_type_name(); diff --git a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/mod.rs b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/mod.rs index 9f56b72f4e9b..2a723286d8be 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/mod.rs @@ -4,7 +4,8 @@ use std::{ }; use crate::{ - ast::ItemVisibility, hir_def::traits::ResolvedTraitBound, StructField, StructType, TypeBindings, + ast::ItemVisibility, hir::resolution::import::PathResolutionItem, + hir_def::traits::ResolvedTraitBound, StructField, StructType, TypeBindings, }; use crate::{ ast::{ @@ -20,7 +21,7 @@ use crate::{ }, def_collector::{dc_crate::CollectedItems, errors::DefCollectorErrorKind}, def_map::{DefMaps, ModuleData}, - def_map::{LocalModuleId, ModuleDefId, ModuleId, MAIN_FUNCTION}, + def_map::{LocalModuleId, ModuleId, MAIN_FUNCTION}, resolution::errors::ResolverError, resolution::import::PathResolution, scope::ScopeForest as GenericScopeForest, @@ -667,11 +668,11 @@ impl<'context> Elaborator<'context> { pub fn resolve_module_by_path(&mut self, path: Path) -> Option { match self.resolve_path(path.clone()) { - Ok(PathResolution { module_def_id: ModuleDefId::ModuleId(module_id), error }) => { - if error.is_some() { - None - } else { + Ok(PathResolution { item: PathResolutionItem::Module(module_id), errors }) => { + if errors.is_empty() { Some(module_id) + } else { + None } } _ => None, @@ -680,8 +681,8 @@ impl<'context> Elaborator<'context> { fn resolve_trait_by_path(&mut self, path: Path) -> Option { let error = match self.resolve_path(path.clone()) { - Ok(PathResolution { module_def_id: ModuleDefId::TraitId(trait_id), error }) => { - if let Some(error) = error { + Ok(PathResolution { item: PathResolutionItem::Trait(trait_id), errors }) => { + for error in errors { self.push_err(error); } return Some(trait_id); diff --git a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/patterns.rs b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/patterns.rs index d55011f98a1c..b45f455633b3 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -9,7 +9,7 @@ use crate::{ }, hir::{ def_collector::dc_crate::CompilationError, - resolution::errors::ResolverError, + resolution::{errors::ResolverError, import::PathResolutionItem}, type_check::{Source, TypeCheckError}, }, hir_def::{ @@ -178,9 +178,6 @@ impl<'context> Elaborator<'context> { mutable: Option, new_definitions: &mut Vec, ) -> HirPattern { - let exclude_last_segment = true; - self.check_unsupported_turbofish_usage(&name, exclude_last_segment); - let last_segment = name.last_segment(); let name_span = last_segment.ident.span(); let is_self_type = last_segment.ident.is_self_type_name(); @@ -195,7 +192,7 @@ impl<'context> Elaborator<'context> { }; let (struct_type, generics) = match self.lookup_type_or_error(name) { - Some(Type::Struct(struct_type, generics)) => (struct_type, generics), + Some(Type::Struct(struct_type, struct_generics)) => (struct_type, struct_generics), None => return error_identifier(self), Some(typ) => { let typ = typ.to_string(); @@ -468,54 +465,102 @@ impl<'context> Elaborator<'context> { } pub(super) fn elaborate_variable(&mut self, variable: Path) -> (ExprId, Type) { - let exclude_last_segment = true; - self.check_unsupported_turbofish_usage(&variable, exclude_last_segment); - let unresolved_turbofish = variable.segments.last().unwrap().generics.clone(); let span = variable.span; - let expr = self.resolve_variable(variable); + let (expr, item) = self.resolve_variable(variable); let definition_id = expr.id; + let type_generics = item.map(|item| self.resolve_item_turbofish(item)).unwrap_or_default(); + let definition_kind = self.interner.try_definition(definition_id).map(|definition| definition.kind.clone()); + let mut bindings = TypeBindings::new(); + // Resolve any generics if we the variable we have resolved is a function // and if the turbofish operator was used. - let generics = definition_kind.and_then(|definition_kind| match &definition_kind { - DefinitionKind::Function(function) => { - self.resolve_function_turbofish_generics(function, unresolved_turbofish, span) + let generics = if let Some(DefinitionKind::Function(func_id)) = &definition_kind { + self.resolve_function_turbofish_generics(func_id, unresolved_turbofish, span) + } else { + None + }; + + // If this is a function call on a type that has generics, we need to bind those generic types. + if !type_generics.is_empty() { + if let Some(DefinitionKind::Function(func_id)) = &definition_kind { + // `all_generics` will always have the enclosing type generics first, so we need to bind those + let func_generics = &self.interner.function_meta(func_id).all_generics; + for (type_generic, func_generic) in type_generics.into_iter().zip(func_generics) { + let type_var = &func_generic.type_var; + bindings + .insert(type_var.id(), (type_var.clone(), type_var.kind(), type_generic)); + } } - _ => None, - }); + } let id = self.interner.push_expr(HirExpression::Ident(expr.clone(), generics.clone())); self.interner.push_expr_location(id, span, self.file); - let typ = self.type_check_variable(expr, id, generics); + let typ = self.type_check_variable_with_bindings(expr, id, generics, bindings); self.interner.push_expr_type(id, typ.clone()); (id, typ) } - fn resolve_variable(&mut self, path: Path) -> HirIdent { + /// Solve any generics that are part of the path before the function, for example: + /// + /// foo::Bar::::baz + /// ^^^^^ + /// solve these + fn resolve_item_turbofish(&mut self, item: PathResolutionItem) -> Vec { + match item { + PathResolutionItem::StructFunction(struct_id, Some(generics), _func_id) => { + let struct_type = self.interner.get_struct(struct_id); + let struct_type = struct_type.borrow(); + let struct_generics = struct_type.instantiate(self.interner); + self.resolve_struct_turbofish_generics( + &struct_type, + struct_generics, + Some(generics.generics), + generics.span, + ) + } + PathResolutionItem::TypeAliasFunction(_type_alias_id, Some(generics), _func_id) => { + // TODO: https://github.com/noir-lang/noir/issues/6311 + self.push_err(TypeCheckError::UnsupportedTurbofishUsage { span: generics.span }); + Vec::new() + } + PathResolutionItem::TraitFunction(_trait_id, Some(generics), _func_id) => { + // TODO: https://github.com/noir-lang/noir/issues/6310 + self.push_err(TypeCheckError::UnsupportedTurbofishUsage { span: generics.span }); + Vec::new() + } + _ => Vec::new(), + } + } + + fn resolve_variable(&mut self, path: Path) -> (HirIdent, Option) { if let Some(trait_path_resolution) = self.resolve_trait_generic_path(&path) { - if let Some(error) = trait_path_resolution.error { + for error in trait_path_resolution.errors { self.push_err(error); } - HirIdent { - location: Location::new(path.span, self.file), - id: self.interner.trait_method_id(trait_path_resolution.method.method_id), - impl_kind: ImplKind::TraitMethod(trait_path_resolution.method), - } + ( + HirIdent { + location: Location::new(path.span, self.file), + id: self.interner.trait_method_id(trait_path_resolution.method.method_id), + impl_kind: ImplKind::TraitMethod(trait_path_resolution.method), + }, + None, + ) } else { // If the Path is being used as an Expression, then it is referring to a global from a separate module // Otherwise, then it is referring to an Identifier // This lookup allows support of such statements: let x = foo::bar::SOME_GLOBAL + 10; // If the expression is a singular indent, we search the resolver's current scope as normal. let span = path.span(); - let (hir_ident, var_scope_index) = self.get_ident_from_path(path); + let ((hir_ident, var_scope_index), item) = self.get_ident_from_path(path); if hir_ident.id != DefinitionId::dummy_id() { match self.interner.definition(hir_ident.id).kind { @@ -557,7 +602,7 @@ impl<'context> Elaborator<'context> { } } - hir_ident + (hir_ident, item) } } @@ -567,8 +612,17 @@ impl<'context> Elaborator<'context> { expr_id: ExprId, generics: Option>, ) -> Type { - let mut bindings = TypeBindings::new(); + let bindings = TypeBindings::new(); + self.type_check_variable_with_bindings(ident, expr_id, generics, bindings) + } + pub(super) fn type_check_variable_with_bindings( + &mut self, + ident: HirIdent, + expr_id: ExprId, + generics: Option>, + mut bindings: TypeBindings, + ) -> Type { // 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 @@ -668,24 +722,31 @@ impl<'context> Elaborator<'context> { } } - pub fn get_ident_from_path(&mut self, path: Path) -> (HirIdent, usize) { + pub fn get_ident_from_path( + &mut self, + path: Path, + ) -> ((HirIdent, usize), Option) { let location = Location::new(path.last_ident().span(), self.file); let error = match path.as_ident().map(|ident| self.use_variable(ident)) { - Some(Ok(found)) => return found, + Some(Ok(found)) => return (found, None), // Try to look it up as a global, but still issue the first error if we fail Some(Err(error)) => match self.lookup_global(path) { - Ok(id) => return (HirIdent::non_trait_method(id, location), 0), + Ok((id, item)) => { + return ((HirIdent::non_trait_method(id, location), 0), Some(item)) + } Err(_) => error, }, None => match self.lookup_global(path) { - Ok(id) => return (HirIdent::non_trait_method(id, location), 0), + Ok((id, item)) => { + return ((HirIdent::non_trait_method(id, location), 0), Some(item)) + } Err(error) => error, }, }; self.push_err(error); let id = DefinitionId::dummy_id(); - (HirIdent::non_trait_method(id, location), 0) + ((HirIdent::non_trait_method(id, location), 0), None) } pub(super) fn elaborate_type_path(&mut self, path: TypePath) -> (ExprId, Type) { diff --git a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/scope.rs b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/scope.rs index d38b7a501759..8e033c914be6 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/scope.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/scope.rs @@ -2,14 +2,11 @@ use noirc_errors::{Location, Spanned}; use crate::ast::{Ident, Path, PathKind, ERROR_IDENT}; use crate::hir::def_map::{LocalModuleId, ModuleId}; -use crate::hir::resolution::import::{PathResolution, PathResolutionResult}; +use crate::hir::resolution::import::{PathResolution, PathResolutionItem, PathResolutionResult}; use crate::hir::resolution::path_resolver::{PathResolver, StandardPathResolver}; use crate::hir::scope::{Scope as GenericScope, ScopeTree as GenericScopeTree}; use crate::{ - hir::{ - def_map::{ModuleDefId, TryFromModuleDefId}, - resolution::errors::ResolverError, - }, + hir::resolution::errors::ResolverError, hir_def::{ expr::{HirCapturedVar, HirIdent}, traits::Trait, @@ -26,16 +23,6 @@ type Scope = GenericScope; type ScopeTree = GenericScopeTree; impl<'context> Elaborator<'context> { - pub(super) fn lookup(&mut self, path: Path) -> Result { - let span = path.span(); - let id = self.resolve_path_or_error(path)?; - T::try_from(id).ok_or_else(|| ResolverError::Expected { - expected: T::description(), - got: id.as_str().to_owned(), - span, - }) - } - pub fn module_id(&self) -> ModuleId { assert_ne!(self.local_module, LocalModuleId::dummy_id(), "local_module is unset"); ModuleId { krate: self.crate_id, local_id: self.local_module } @@ -53,14 +40,14 @@ impl<'context> Elaborator<'context> { pub(super) fn resolve_path_or_error( &mut self, path: Path, - ) -> Result { + ) -> Result { let path_resolution = self.resolve_path(path)?; - if let Some(error) = path_resolution.error { + for error in path_resolution.errors { self.push_err(error); } - Ok(path_resolution.module_def_id) + Ok(path_resolution.item) } pub(super) fn resolve_path(&mut self, path: Path) -> PathResolutionResult { @@ -72,8 +59,8 @@ impl<'context> Elaborator<'context> { let struct_type = struct_type.borrow(); if path.segments.len() == 1 { return Ok(PathResolution { - module_def_id: ModuleDefId::TypeId(struct_type.id), - error: None, + item: PathResolutionItem::Struct(struct_type.id), + errors: Vec::new(), }); } @@ -132,8 +119,8 @@ impl<'context> Elaborator<'context> { Err(err) => return Err(err), }; - self.interner.add_module_def_id_reference( - path_resolution.module_def_id, + self.interner.add_path_resolution_kind_reference( + path_resolution.item.clone(), location, is_self_type_name, ); @@ -183,21 +170,24 @@ impl<'context> Elaborator<'context> { } } - pub(super) fn lookup_global(&mut self, path: Path) -> Result { + pub(super) fn lookup_global( + &mut self, + path: Path, + ) -> Result<(DefinitionId, PathResolutionItem), ResolverError> { let span = path.span(); - let id = self.resolve_path_or_error(path)?; + let item = self.resolve_path_or_error(path)?; - if let Some(function) = TryFromModuleDefId::try_from(id) { - return Ok(self.interner.function_definition_id(function)); + if let Some(function) = item.function_id() { + return Ok((self.interner.function_definition_id(function), item)); } - if let Some(global) = TryFromModuleDefId::try_from(id) { + if let PathResolutionItem::Global(global) = item { let global = self.interner.get_global(global); - return Ok(global.definition_id); + return Ok((global.definition_id, item)); } - let expected = "global variable".into(); - let got = "local variable".into(); + let expected = "global variable"; + let got = "local variable"; Err(ResolverError::Expected { span, expected, got }) } @@ -239,10 +229,22 @@ impl<'context> Elaborator<'context> { /// Lookup a given trait by name/path. pub fn lookup_trait_or_error(&mut self, path: Path) -> Option<&mut Trait> { - match self.lookup(path) { - Ok(trait_id) => Some(self.get_trait_mut(trait_id)), - Err(error) => { - self.push_err(error); + let span = path.span(); + match self.resolve_path_or_error(path) { + Ok(item) => { + if let PathResolutionItem::Trait(trait_id) = item { + Some(self.get_trait_mut(trait_id)) + } else { + self.push_err(ResolverError::Expected { + expected: "trait", + got: item.description(), + span, + }); + None + } + } + Err(err) => { + self.push_err(err); None } } @@ -250,10 +252,22 @@ impl<'context> Elaborator<'context> { /// Lookup a given struct type by name. pub fn lookup_struct_or_error(&mut self, path: Path) -> Option> { - match self.lookup(path) { - Ok(struct_id) => Some(self.get_struct(struct_id)), - Err(error) => { - self.push_err(error); + let span = path.span(); + match self.resolve_path_or_error(path) { + Ok(item) => { + if let PathResolutionItem::Struct(struct_id) = item { + Some(self.get_struct(struct_id)) + } else { + self.push_err(ResolverError::Expected { + expected: "type", + got: item.description(), + span, + }); + None + } + } + Err(err) => { + self.push_err(err); None } } @@ -271,20 +285,20 @@ impl<'context> Elaborator<'context> { let span = path.span; match self.resolve_path_or_error(path) { - Ok(ModuleDefId::TypeId(struct_id)) => { + Ok(PathResolutionItem::Struct(struct_id)) => { let struct_type = self.get_struct(struct_id); let generics = struct_type.borrow().instantiate(self.interner); Some(Type::Struct(struct_type, generics)) } - Ok(ModuleDefId::TypeAliasId(alias_id)) => { + Ok(PathResolutionItem::TypeAlias(alias_id)) => { let alias = self.interner.get_type_alias(alias_id); let alias = alias.borrow(); Some(alias.instantiate(self.interner)) } Ok(other) => { self.push_err(ResolverError::Expected { - expected: StructId::description(), - got: other.as_str().to_owned(), + expected: "type", + got: other.description(), span, }); None @@ -297,6 +311,11 @@ impl<'context> Elaborator<'context> { } pub fn lookup_type_alias(&mut self, path: Path) -> Option> { - self.lookup(path).ok().map(|id| self.interner.get_type_alias(id)) + match self.resolve_path_or_error(path) { + Ok(PathResolutionItem::TypeAlias(type_alias_id)) => { + Some(self.interner.get_type_alias(type_alias_id)) + } + _ => None, + } } } diff --git a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/statements.rs b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/statements.rs index 238160e5aa49..757def16a93b 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/statements.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/statements.rs @@ -293,7 +293,8 @@ impl<'context> Elaborator<'context> { let mut mutable = true; let span = ident.span(); let path = Path::from_single(ident.0.contents, span); - let (ident, scope_index) = self.get_ident_from_path(path); + let ((ident, scope_index), _) = self.get_ident_from_path(path); + self.resolve_local_variable(ident.clone(), scope_index); let typ = if ident.id == DefinitionId::dummy_id() { diff --git a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/types.rs b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/types.rs index 8ffbd15bdabf..2879204d3ee9 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/types.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/types.rs @@ -14,8 +14,10 @@ use crate::{ hir::{ comptime::{Interpreter, Value}, def_collector::dc_crate::CompilationError, - def_map::ModuleDefId, - resolution::{errors::ResolverError, import::PathResolutionError}, + resolution::{ + errors::ResolverError, + import::{PathResolutionError, PathResolutionItem}, + }, type_check::{ generics::{Generic, TraitGenerics}, NoMatchingImplFoundError, Source, TypeCheckError, @@ -46,7 +48,7 @@ pub const WILDCARD_TYPE: &str = "_"; pub(super) struct TraitPathResolution { pub(super) method: TraitMethod, - pub(super) error: Option, + pub(super) errors: Vec, } impl<'context> Elaborator<'context> { @@ -404,7 +406,7 @@ impl<'context> Elaborator<'context> { // If we cannot find a local generic of the same name, try to look up a global match self.resolve_path_or_error(path.clone()) { - Ok(ModuleDefId::GlobalId(id)) => { + Ok(PathResolutionItem::Global(id)) => { if let Some(current_item) = self.current_item { self.interner.add_global_dependency(current_item, id); } @@ -551,7 +553,7 @@ impl<'context> Elaborator<'context> { let constraint = the_trait.as_constraint(path.span); return Some(TraitPathResolution { method: TraitMethod { method_id: method, constraint, assumed: true }, - error: None, + errors: Vec::new(), }); } } @@ -564,15 +566,14 @@ impl<'context> Elaborator<'context> { // E.g. `t.method()` with `where T: Foo` in scope will return `(Foo::method, T, vec![Bar])` fn resolve_trait_static_method(&mut self, path: &Path) -> Option { let path_resolution = self.resolve_path(path.clone()).ok()?; - let ModuleDefId::FunctionId(func_id) = path_resolution.module_def_id else { return None }; - + let func_id = path_resolution.item.function_id()?; let meta = self.interner.function_meta(&func_id); let the_trait = self.interner.get_trait(meta.trait_id?); let method = the_trait.find_method(path.last_name())?; let constraint = the_trait.as_constraint(path.span); Some(TraitPathResolution { method: TraitMethod { method_id: method, constraint, assumed: false }, - error: path_resolution.error, + errors: path_resolution.errors, }) } @@ -600,7 +601,7 @@ impl<'context> Elaborator<'context> { if let Some(method) = the_trait.find_method(path.last_name()) { return Some(TraitPathResolution { method: TraitMethod { method_id: method, constraint, assumed: true }, - error: None, + errors: Vec::new(), }); } } @@ -1826,19 +1827,6 @@ impl<'context> Elaborator<'context> { context.trait_constraints.push((constraint, expr_id)); } - pub fn check_unsupported_turbofish_usage(&mut self, path: &Path, exclude_last_segment: bool) { - for (index, segment) in path.segments.iter().enumerate() { - if exclude_last_segment && index == path.segments.len() - 1 { - continue; - } - - if segment.generics.is_some() { - let span = segment.turbofish_span(); - self.push_err(TypeCheckError::UnsupportedTurbofishUsage { span }); - } - } - } - pub fn bind_generics_from_trait_constraint( &self, constraint: &TraitConstraint, diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 658812be3240..20c162fbd3aa 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -385,9 +385,8 @@ impl DefCollector { let current_def_map = context.def_maps.get_mut(&crate_id).unwrap(); let file_id = current_def_map.file_id(module_id); - let has_path_resolution_error = resolved_import.error.is_some(); - - if let Some(error) = resolved_import.error { + let has_path_resolution_error = !resolved_import.errors.is_empty(); + for error in resolved_import.errors { errors.push(( DefCollectorErrorKind::PathResolutionError(error).into(), file_id, @@ -557,7 +556,7 @@ fn inject_prelude( span: Span::default(), }; - if let Ok(PathResolution { module_def_id, error }) = path_resolver::resolve_path( + if let Ok(PathResolution { item, errors }) = path_resolver::resolve_path( &context.def_maps, ModuleId { krate: crate_id, local_id: crate_root }, None, @@ -565,8 +564,8 @@ fn inject_prelude( &mut context.def_interner.usage_tracker, &mut None, ) { - assert!(error.is_none(), "Tried to add private item to prelude"); - let module_id = module_def_id.as_module().expect("std::prelude should be a module"); + assert!(errors.is_empty(), "Tried to add private item to prelude"); + let module_id = item.module_id().expect("std::prelude should be a module"); let prelude = context.module(module_id).scope().names(); for path in prelude { diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_map/module_def.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_map/module_def.rs index a487bda81b3a..a751eacd2dda 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_map/module_def.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_map/module_def.rs @@ -99,79 +99,3 @@ impl From for ModuleDefId { ModuleDefId::TraitId(trait_id) } } - -pub trait TryFromModuleDefId: Sized { - fn try_from(id: ModuleDefId) -> Option; - fn dummy_id() -> Self; - fn description() -> String; -} - -impl TryFromModuleDefId for FuncId { - fn try_from(id: ModuleDefId) -> Option { - id.as_function() - } - - fn dummy_id() -> Self { - FuncId::dummy_id() - } - - fn description() -> String { - "function".to_string() - } -} - -impl TryFromModuleDefId for StructId { - fn try_from(id: ModuleDefId) -> Option { - id.as_type() - } - - fn dummy_id() -> Self { - StructId::dummy_id() - } - - fn description() -> String { - "type".to_string() - } -} - -impl TryFromModuleDefId for TypeAliasId { - fn try_from(id: ModuleDefId) -> Option { - id.as_type_alias() - } - - fn dummy_id() -> Self { - TypeAliasId::dummy_id() - } - - fn description() -> String { - "type alias".to_string() - } -} - -impl TryFromModuleDefId for TraitId { - fn try_from(id: ModuleDefId) -> Option { - id.as_trait() - } - - fn dummy_id() -> Self { - TraitId::dummy_id() - } - - fn description() -> String { - "trait".to_string() - } -} - -impl TryFromModuleDefId for GlobalId { - fn try_from(id: ModuleDefId) -> Option { - id.as_global() - } - - fn dummy_id() -> Self { - GlobalId::dummy_id() - } - - fn description() -> String { - "global".to_string() - } -} diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_map/namespace.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_map/namespace.rs index 6fac6d2b991b..a600d98dd8b6 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_map/namespace.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_map/namespace.rs @@ -13,14 +13,6 @@ impl PerNs { PerNs { types: Some((t, ItemVisibility::Public, false)), values: None } } - pub fn take_types(self) -> Option { - self.types.map(|it| it.0) - } - - pub fn take_values(self) -> Option { - self.values.map(|it| it.0) - } - pub fn iter_defs(self) -> impl Iterator { self.types.map(|it| it.0).into_iter().chain(self.values.map(|it| it.0)) } diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/errors.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/errors.rs index 3c4022b58bbb..e1e60daff600 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/errors.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/errors.rs @@ -38,7 +38,7 @@ pub enum ResolverError { #[error("could not resolve path")] PathResolutionError(#[from] PathResolutionError), #[error("Expected")] - Expected { span: Span, expected: String, got: String }, + Expected { span: Span, expected: &'static str, got: &'static str }, #[error("Duplicate field in constructor")] DuplicateField { field: Ident }, #[error("No such field in struct")] diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/import.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/import.rs index 93039b1ea7f6..58a3a8418017 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/import.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/import.rs @@ -3,12 +3,13 @@ use thiserror::Error; use crate::graph::CrateId; use crate::hir::def_collector::dc_crate::CompilationError; -use crate::node_interner::ReferenceId; + +use crate::node_interner::{FuncId, GlobalId, ReferenceId, StructId, TraitId, TypeAliasId}; use crate::usage_tracker::UsageTracker; use std::collections::BTreeMap; -use crate::ast::{Ident, ItemVisibility, Path, PathKind, PathSegment}; +use crate::ast::{Ident, ItemVisibility, Path, PathKind, PathSegment, UnresolvedType}; use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleDefId, ModuleId, PerNs}; use super::errors::ResolverError; @@ -26,16 +27,80 @@ pub struct ImportDirective { struct NamespaceResolution { module_id: ModuleId, + item: PathResolutionItem, namespace: PerNs, - error: Option, + errors: Vec, } type NamespaceResolutionResult = Result; +#[derive(Debug)] pub struct PathResolution { - pub module_def_id: ModuleDefId, + pub item: PathResolutionItem, + pub errors: Vec, +} - pub error: Option, +/// All possible items that result from resolving a Path. +/// Note that this item doesn't include the last turbofish in a Path, +/// only intermediate ones, if any. +#[derive(Debug, Clone)] +pub enum PathResolutionItem { + Module(ModuleId), + Struct(StructId), + TypeAlias(TypeAliasId), + Trait(TraitId), + Global(GlobalId), + ModuleFunction(FuncId), + StructFunction(StructId, Option, FuncId), + TypeAliasFunction(TypeAliasId, Option, FuncId), + TraitFunction(TraitId, Option, FuncId), +} + +impl PathResolutionItem { + pub fn function_id(&self) -> Option { + match self { + PathResolutionItem::ModuleFunction(func_id) + | PathResolutionItem::StructFunction(_, _, func_id) + | PathResolutionItem::TypeAliasFunction(_, _, func_id) + | PathResolutionItem::TraitFunction(_, _, func_id) => Some(*func_id), + _ => None, + } + } + + pub fn module_id(&self) -> Option { + match self { + Self::Module(module_id) => Some(*module_id), + _ => None, + } + } + + pub fn description(&self) -> &'static str { + match self { + PathResolutionItem::Module(..) => "module", + PathResolutionItem::Struct(..) => "type", + PathResolutionItem::TypeAlias(..) => "type alias", + PathResolutionItem::Trait(..) => "trait", + PathResolutionItem::Global(..) => "global", + PathResolutionItem::ModuleFunction(..) + | PathResolutionItem::StructFunction(..) + | PathResolutionItem::TypeAliasFunction(..) + | PathResolutionItem::TraitFunction(..) => "function", + } + } +} + +#[derive(Debug, Clone)] +pub struct Turbofish { + pub generics: Vec, + pub span: Span, +} + +/// Any item that can appear before the last segment in a path. +#[derive(Debug)] +enum IntermediatePathResolutionItem { + Module(ModuleId), + Struct(StructId, Option), + Trait(TraitId, Option), } pub(crate) type PathResolutionResult = Result; @@ -48,6 +113,8 @@ pub enum PathResolutionError { Private(Ident), #[error("There is no super module")] NoSuper(Span), + #[error("turbofish (`::<_>`) not allowed on {item}")] + TurbofishNotAllowedOnItem { item: String, span: Span }, } #[derive(Debug)] @@ -56,10 +123,12 @@ pub struct ResolvedImport { pub name: Ident, // The symbol which we have resolved to pub resolved_namespace: PerNs, + // The item which we have resolved to + pub item: PathResolutionItem, // The module which we must add the resolved namespace to pub module_scope: LocalModuleId, pub is_prelude: bool, - pub error: Option, + pub errors: Vec, } impl From for CompilationError { @@ -83,6 +152,9 @@ impl<'a> From<&'a PathResolutionError> for CustomDiagnostic { PathResolutionError::NoSuper(span) => { CustomDiagnostic::simple_error(error.to_string(), String::new(), *span) } + PathResolutionError::TurbofishNotAllowedOnItem { item: _, span } => { + CustomDiagnostic::simple_error(error.to_string(), String::new(), *span) + } } } } @@ -95,15 +167,19 @@ pub fn resolve_import( path_references: &mut Option<&mut Vec>, ) -> Result { let module_scope = import_directive.module_id; - let NamespaceResolution { module_id: resolved_module, namespace: resolved_namespace, error } = - resolve_path_to_ns( - import_directive, - crate_id, - crate_id, - def_maps, - usage_tracker, - path_references, - )?; + let NamespaceResolution { + module_id: resolved_module, + item, + namespace: resolved_namespace, + mut errors, + } = resolve_path_to_ns( + import_directive, + crate_id, + crate_id, + def_maps, + usage_tracker, + path_references, + )?; let name = resolve_path_name(import_directive); @@ -113,28 +189,25 @@ pub fn resolve_import( .map(|(_, visibility, _)| visibility) .expect("Found empty namespace"); - let error = error.or_else(|| { - if import_directive.self_type_module_id == Some(resolved_module) - || can_reference_module_id( - def_maps, - crate_id, - import_directive.module_id, - resolved_module, - visibility, - ) - { - None - } else { - Some(PathResolutionError::Private(name.clone())) - } - }); + if !(import_directive.self_type_module_id == Some(resolved_module) + || can_reference_module_id( + def_maps, + crate_id, + import_directive.module_id, + resolved_module, + visibility, + )) + { + errors.push(PathResolutionError::Private(name.clone())); + } Ok(ResolvedImport { name, resolved_namespace, + item, module_scope, is_prelude: import_directive.is_prelude, - error, + errors, }) } @@ -275,13 +348,16 @@ fn resolve_name_in_module( let mut current_mod_id = ModuleId { krate, local_id: starting_mod }; let mut current_mod = &def_map.modules[current_mod_id.local_id.0]; + let mut intermediate_item = IntermediatePathResolutionItem::Module(current_mod_id); + // There is a possibility that the import path is empty // In that case, early return if import_path.is_empty() { return Ok(NamespaceResolution { module_id: current_mod_id, + item: PathResolutionItem::Module(current_mod_id), namespace: PerNs::types(current_mod_id.into()), - error: None, + errors: Vec::new(), }); } @@ -293,25 +369,34 @@ fn resolve_name_in_module( usage_tracker.mark_as_referenced(current_mod_id, first_segment); - let mut warning: Option = None; + let mut errors = Vec::new(); for (index, (last_segment, current_segment)) in import_path.iter().zip(import_path.iter().skip(1)).enumerate() { - let last_segment = &last_segment.ident; - let current_segment = ¤t_segment.ident; + let last_ident = &last_segment.ident; + let current_ident = ¤t_segment.ident; + let last_segment_generics = &last_segment.generics; let (typ, visibility) = match current_ns.types { - None => return Err(PathResolutionError::Unresolved(last_segment.clone())), + None => return Err(PathResolutionError::Unresolved(last_ident.clone())), Some((typ, visibility, _)) => (typ, visibility), }; // In the type namespace, only Mod can be used in a path. - current_mod_id = match typ { + (current_mod_id, intermediate_item) = match typ { ModuleDefId::ModuleId(id) => { if let Some(path_references) = path_references { path_references.push(ReferenceId::Module(id)); } - id + + if last_segment_generics.is_some() { + errors.push(PathResolutionError::TurbofishNotAllowedOnItem { + item: format!("module `{last_ident}`"), + span: last_segment.turbofish_span(), + }); + } + + (id, IntermediatePathResolutionItem::Module(id)) } ModuleDefId::FunctionId(_) => panic!("functions cannot be in the type namespace"), // TODO: If impls are ever implemented, types can be used in a path @@ -319,51 +404,75 @@ fn resolve_name_in_module( if let Some(path_references) = path_references { path_references.push(ReferenceId::Struct(id)); } - id.module_id() + + ( + id.module_id(), + IntermediatePathResolutionItem::Struct( + id, + last_segment_generics.as_ref().map(|generics| Turbofish { + generics: generics.clone(), + span: last_segment.turbofish_span(), + }), + ), + ) } ModuleDefId::TypeAliasId(_) => panic!("type aliases cannot be used in type namespace"), ModuleDefId::TraitId(id) => { if let Some(path_references) = path_references { path_references.push(ReferenceId::Trait(id)); } - id.0 + + ( + id.0, + IntermediatePathResolutionItem::Trait( + id, + last_segment_generics.as_ref().map(|generics| Turbofish { + generics: generics.clone(), + span: last_segment.turbofish_span(), + }), + ), + ) } ModuleDefId::GlobalId(_) => panic!("globals cannot be in the type namespace"), }; - warning = warning.or_else(|| { - // If the path is plain or crate, the first segment will always refer to - // something that's visible from the current module. - if (plain_or_crate && index == 0) - || can_reference_module_id( - def_maps, - importing_crate, - starting_mod, - current_mod_id, - visibility, - ) - { - None - } else { - Some(PathResolutionError::Private(last_segment.clone())) - } - }); + // If the path is plain or crate, the first segment will always refer to + // something that's visible from the current module. + if !((plain_or_crate && index == 0) + || can_reference_module_id( + def_maps, + importing_crate, + starting_mod, + current_mod_id, + visibility, + )) + { + errors.push(PathResolutionError::Private(last_ident.clone())); + } current_mod = &def_maps[¤t_mod_id.krate].modules[current_mod_id.local_id.0]; // Check if namespace - let found_ns = current_mod.find_name(current_segment); + let found_ns = current_mod.find_name(current_ident); if found_ns.is_none() { - return Err(PathResolutionError::Unresolved(current_segment.clone())); + return Err(PathResolutionError::Unresolved(current_ident.clone())); } - usage_tracker.mark_as_referenced(current_mod_id, current_segment); + usage_tracker.mark_as_referenced(current_mod_id, current_ident); current_ns = found_ns; } - Ok(NamespaceResolution { module_id: current_mod_id, namespace: current_ns, error: warning }) + let module_def_id = + current_ns.values.or(current_ns.types).map(|(id, _, _)| id).expect("Found empty namespace"); + + let item = merge_intermediate_path_resolution_item_with_module_def_id( + intermediate_item, + module_def_id, + ); + + Ok(NamespaceResolution { module_id: current_mod_id, item, namespace: current_ns, errors }) } fn resolve_path_name(import_directive: &ImportDirective) -> Ident { @@ -425,3 +534,27 @@ fn resolve_external_dep( path_references, ) } + +fn merge_intermediate_path_resolution_item_with_module_def_id( + intermediate_item: IntermediatePathResolutionItem, + module_def_id: ModuleDefId, +) -> PathResolutionItem { + match module_def_id { + ModuleDefId::ModuleId(module_id) => PathResolutionItem::Module(module_id), + ModuleDefId::TypeId(struct_id) => PathResolutionItem::Struct(struct_id), + ModuleDefId::TypeAliasId(type_alias_id) => PathResolutionItem::TypeAlias(type_alias_id), + ModuleDefId::TraitId(trait_id) => PathResolutionItem::Trait(trait_id), + ModuleDefId::GlobalId(global_id) => PathResolutionItem::Global(global_id), + ModuleDefId::FunctionId(func_id) => match intermediate_item { + IntermediatePathResolutionItem::Module(_) => { + PathResolutionItem::ModuleFunction(func_id) + } + IntermediatePathResolutionItem::Struct(struct_id, generics) => { + PathResolutionItem::StructFunction(struct_id, generics, func_id) + } + IntermediatePathResolutionItem::Trait(trait_id, generics) => { + PathResolutionItem::TraitFunction(trait_id, generics, func_id) + } + }, + } +} diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs index 562366fae778..705820e91019 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/path_resolver.rs @@ -88,9 +88,5 @@ pub fn resolve_path( let resolved_import = resolve_import(module_id.krate, &import, def_maps, usage_tracker, path_references)?; - let namespace = resolved_import.resolved_namespace; - let id = - namespace.values.or(namespace.types).map(|(id, _, _)| id).expect("Found empty namespace"); - - Ok(PathResolution { module_def_id: id, error: resolved_import.error }) + Ok(PathResolution { item: resolved_import.item, errors: resolved_import.errors }) } diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/type_check/errors.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/type_check/errors.rs index 99de6bca4345..3b4ab148ef72 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/type_check/errors.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/type_check/errors.rs @@ -434,11 +434,15 @@ impl<'a> From<&'a TypeCheckError> for Diagnostic { let msg = format!("Expected {expected_count} generic{expected_plural} from this function, but {actual_count} {actual_plural} provided"); Diagnostic::simple_error(msg, "".into(), *span) }, - TypeCheckError::MacroReturningNonExpr { typ, span } => Diagnostic::simple_error( - format!("Expected macro call to return a `Quoted` but found a(n) `{typ}`"), - "Macro calls must return quoted values, otherwise there is no code to insert".into(), - *span, - ), + TypeCheckError::MacroReturningNonExpr { typ, span } => { + let mut error = Diagnostic::simple_error( + format!("Expected macro call to return a `Quoted` but found a(n) `{typ}`"), + "Macro calls must return quoted values, otherwise there is no code to insert.".into(), + *span, + ); + error.add_secondary("Hint: remove the `!` from the end of the function name.".to_string(), *span); + error + }, TypeCheckError::UnsupportedTurbofishUsage { span } => { let msg = "turbofish (`::<_>`) usage at this position isn't supported yet"; Diagnostic::simple_error(msg.to_string(), "".to_string(), *span) diff --git a/noir/noir-repo/compiler/noirc_frontend/src/lexer/token.rs b/noir/noir-repo/compiler/noirc_frontend/src/lexer/token.rs index 8f05832d26d5..daf594459828 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/lexer/token.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/lexer/token.rs @@ -1,4 +1,4 @@ -use acvm::{acir::AcirField, FieldElement}; +use acvm::FieldElement; use noirc_errors::{Position, Span, Spanned}; use std::fmt; @@ -367,7 +367,7 @@ impl fmt::Display for Token { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { match *self { Token::Ident(ref s) => write!(f, "{s}"), - Token::Int(n) => write!(f, "{}", n.to_u128()), + Token::Int(n) => write!(f, "{}", n), Token::Bool(b) => write!(f, "{b}"), Token::Str(ref b) => write!(f, "{b:?}"), Token::FmtStr(ref b) => write!(f, "f{b:?}"), diff --git a/noir/noir-repo/compiler/noirc_frontend/src/locations.rs b/noir/noir-repo/compiler/noirc_frontend/src/locations.rs index 48660142d0a8..4dd699251a6a 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/locations.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/locations.rs @@ -5,7 +5,10 @@ use rustc_hash::FxHashMap as HashMap; use crate::{ ast::{FunctionDefinition, ItemVisibility}, - hir::def_map::{ModuleDefId, ModuleId}, + hir::{ + def_map::{ModuleDefId, ModuleId}, + resolution::import::PathResolutionItem, + }, node_interner::{ DefinitionId, FuncId, GlobalId, NodeInterner, ReferenceId, StructId, TraitId, TypeAliasId, }, @@ -99,6 +102,37 @@ impl NodeInterner { }; } + pub(crate) fn add_path_resolution_kind_reference( + &mut self, + kind: PathResolutionItem, + location: Location, + is_self_type: bool, + ) { + match kind { + PathResolutionItem::Module(module_id) => { + self.add_module_reference(module_id, location); + } + PathResolutionItem::Struct(struct_id) => { + self.add_struct_reference(struct_id, location, is_self_type); + } + PathResolutionItem::TypeAlias(type_alias_id) => { + self.add_alias_reference(type_alias_id, location); + } + PathResolutionItem::Trait(trait_id) => { + self.add_trait_reference(trait_id, location, is_self_type); + } + PathResolutionItem::Global(global_id) => { + self.add_global_reference(global_id, location); + } + PathResolutionItem::ModuleFunction(func_id) + | PathResolutionItem::StructFunction(_, _, func_id) + | PathResolutionItem::TypeAliasFunction(_, _, func_id) + | PathResolutionItem::TraitFunction(_, _, func_id) => { + self.add_function_reference(func_id, location); + } + } + } + pub(crate) fn add_module_reference(&mut self, id: ModuleId, location: Location) { self.add_reference(ReferenceId::Module(id), location, false); } diff --git a/noir/noir-repo/compiler/noirc_frontend/src/tests.rs b/noir/noir-repo/compiler/noirc_frontend/src/tests.rs index 56c4b5e9a120..5ce3ec6686c6 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/tests.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/tests.rs @@ -617,8 +617,8 @@ fn check_trait_impl_for_non_type() { for (err, _file_id) in errors { match &err { CompilationError::ResolverError(ResolverError::Expected { expected, got, .. }) => { - assert_eq!(expected, "type"); - assert_eq!(got, "function"); + assert_eq!(*expected, "type"); + assert_eq!(*got, "function"); } _ => { panic!("No other errors are expected! Found = {:?}", err); diff --git a/noir/noir-repo/compiler/noirc_frontend/src/tests/bound_checks.rs b/noir/noir-repo/compiler/noirc_frontend/src/tests/bound_checks.rs index 271f9d7a1a71..05669bda4111 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/tests/bound_checks.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/tests/bound_checks.rs @@ -14,7 +14,7 @@ fn overflowing_u8() { if let CompilationError::TypeError(error) = &errors[0].0 { assert_eq!( error.to_string(), - "The value `2⁸` cannot fit into `u8` which has range `0..=255`" + "The value `256` cannot fit into `u8` which has range `0..=255`" ); } else { panic!("Expected OverflowingAssignment error, got {:?}", errors[0].0); @@ -52,7 +52,7 @@ fn overflowing_i8() { if let CompilationError::TypeError(error) = &errors[0].0 { assert_eq!( error.to_string(), - "The value `2⁷` cannot fit into `i8` which has range `-128..=127`" + "The value `128` cannot fit into `i8` which has range `-128..=127`" ); } else { panic!("Expected OverflowingAssignment error, got {:?}", errors[0].0); diff --git a/noir/noir-repo/compiler/noirc_frontend/src/tests/turbofish.rs b/noir/noir-repo/compiler/noirc_frontend/src/tests/turbofish.rs index 71e63e878e80..3ded243a2800 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/tests/turbofish.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/tests/turbofish.rs @@ -1,4 +1,8 @@ -use crate::hir::{def_collector::dc_crate::CompilationError, type_check::TypeCheckError}; +use crate::hir::{ + def_collector::dc_crate::CompilationError, + resolution::{errors::ResolverError, import::PathResolutionError}, + type_check::TypeCheckError, +}; use super::{assert_no_errors, get_program_errors}; @@ -100,32 +104,6 @@ fn turbofish_in_constructor() { assert_eq!(expr_typ, "Field"); } -#[test] -fn turbofish_in_middle_of_variable_unsupported_yet() { - let src = r#" - struct Foo { - x: T - } - - impl Foo { - pub fn new(x: T) -> Self { - Foo { x } - } - } - - fn main() { - let _ = Foo::::new(1); - } - "#; - let errors = get_program_errors(src); - assert_eq!(errors.len(), 1); - - assert!(matches!( - errors[0].0, - CompilationError::TypeError(TypeCheckError::UnsupportedTurbofishUsage { .. }), - )); -} - #[test] fn turbofish_in_struct_pattern() { let src = r#" @@ -214,3 +192,80 @@ fn numeric_turbofish() { "#; assert_no_errors(src); } + +#[test] +fn errors_if_turbofish_after_module() { + let src = r#" + mod moo { + pub fn foo() {} + } + + fn main() { + moo::::foo(); + } + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::PathResolutionError( + PathResolutionError::TurbofishNotAllowedOnItem { item, .. }, + )) = &errors[0].0 + else { + panic!("Expected a turbofish not allowed on item error, got {:?}", errors[0].0); + }; + assert_eq!(item, "module `moo`"); +} + +#[test] +fn turbofish_in_type_before_call_does_not_error() { + let src = r#" + struct Foo { + x: T + } + + impl Foo { + fn new(x: T) -> Self { + Foo { x } + } + } + + fn main() { + let _ = Foo::::new(1); + } + "#; + assert_no_errors(src); +} + +#[test] +fn turbofish_in_type_before_call_errors() { + let src = r#" + struct Foo { + x: T + } + + impl Foo { + fn new(x: T) -> Self { + Foo { x } + } + } + + fn main() { + let _ = Foo::::new(true); + } + "#; + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::TypeError(TypeCheckError::TypeMismatch { + expected_typ, + expr_typ, + expr_span: _, + }) = &errors[0].0 + else { + panic!("Expected a type mismatch error, got {:?}", errors[0].0); + }; + + assert_eq!(expected_typ, "i32"); + assert_eq!(expr_typ, "bool"); +} diff --git a/noir/noir-repo/noir_stdlib/src/hash/keccak.nr b/noir/noir-repo/noir_stdlib/src/hash/keccak.nr index 50fbab8a416e..93b366d1ec11 100644 --- a/noir/noir-repo/noir_stdlib/src/hash/keccak.nr +++ b/noir/noir-repo/noir_stdlib/src/hash/keccak.nr @@ -1,4 +1,3 @@ -use crate::collections::vec::Vec; use crate::runtime::is_unconstrained; global BLOCK_SIZE_IN_BYTES: u32 = 136; //(1600 - BITS * 2) / WORD_SIZE; @@ -12,7 +11,10 @@ fn keccakf1600(input: [u64; 25]) -> [u64; 25] {} #[no_predicates] pub(crate) fn keccak256(input: [u8; N], message_size: u32) -> [u8; 32] { assert(N >= message_size); - let mut block_bytes = [0; BLOCK_SIZE_IN_BYTES]; + + // Copy input to block bytes. For that we'll need at least input bytes (N) + // but we want it to be padded to a multiple of BLOCK_SIZE_IN_BYTES. + let mut block_bytes = [0; ((N / BLOCK_SIZE_IN_BYTES) + 1) * BLOCK_SIZE_IN_BYTES]; if is_unconstrained() { for i in 0..message_size { block_bytes[i] = input[i]; @@ -28,7 +30,6 @@ pub(crate) fn keccak256(input: [u8; N], message_size: u32) -> [u8; 3 //1. format_input_lanes let max_blocks = (N + BLOCK_SIZE_IN_BYTES) / BLOCK_SIZE_IN_BYTES; //maximum number of bytes to hash - let max_blocks_length = (BLOCK_SIZE_IN_BYTES * max_blocks); let real_max_blocks = (message_size + BLOCK_SIZE_IN_BYTES) / BLOCK_SIZE_IN_BYTES; let real_blocks_bytes = real_max_blocks * BLOCK_SIZE_IN_BYTES; @@ -36,9 +37,9 @@ pub(crate) fn keccak256(input: [u8; N], message_size: u32) -> [u8; 3 block_bytes[real_blocks_bytes - 1] = 0x80; // populate a vector of 64-bit limbs from our byte array - let num_limbs = max_blocks_length / WORD_SIZE; - let mut sliced_buffer = Vec::new(); - for i in 0..num_limbs { + let mut sliced_buffer = + [0; (((N / BLOCK_SIZE_IN_BYTES) + 1) * BLOCK_SIZE_IN_BYTES) / WORD_SIZE]; + for i in 0..sliced_buffer.len() { let limb_start = WORD_SIZE * i; let mut sliced = 0; @@ -48,7 +49,7 @@ pub(crate) fn keccak256(input: [u8; N], message_size: u32) -> [u8; 3 v *= 256; } - sliced_buffer.push(sliced as u64); + sliced_buffer[i] = sliced as u64; } //2. sponge_absorb @@ -59,11 +60,11 @@ pub(crate) fn keccak256(input: [u8; N], message_size: u32) -> [u8; 3 for i in 0..real_max_blocks { if (i == 0) { for j in 0..LIMBS_PER_BLOCK { - state[j] = sliced_buffer.get(j); + state[j] = sliced_buffer[j]; } } else { for j in 0..LIMBS_PER_BLOCK { - state[j] = state[j] ^ sliced_buffer.get(i * LIMBS_PER_BLOCK + j); + state[j] = state[j] ^ sliced_buffer[i * LIMBS_PER_BLOCK + j]; } } state = keccakf1600(state); @@ -73,13 +74,13 @@ pub(crate) fn keccak256(input: [u8; N], message_size: u32) -> [u8; 3 // We peel out the first block as to avoid a conditional inside of the loop. // Otherwise, a dynamic predicate can cause a blowup in a constrained runtime. for j in 0..LIMBS_PER_BLOCK { - state[j] = sliced_buffer.get(j); + state[j] = sliced_buffer[j]; } state = keccakf1600(state); for i in 1..max_blocks { if i < real_max_blocks { for j in 0..LIMBS_PER_BLOCK { - state[j] = state[j] ^ sliced_buffer.get(i * LIMBS_PER_BLOCK + j); + state[j] = state[j] ^ sliced_buffer[i * LIMBS_PER_BLOCK + j]; } state = keccakf1600(state); } @@ -114,8 +115,7 @@ mod tests { #[test] fn hash_hello_world() { - // "hello world" - let input = [72, 101, 108, 108, 111, 32, 119, 111, 114, 108, 100, 33]; + let input = "Hello world!".as_bytes(); let result = [ 0xec, 0xd0, 0xe1, 0x8, 0xa9, 0x8e, 0x19, 0x2a, 0xf1, 0xd2, 0xc2, 0x50, 0x55, 0xf4, 0xe3, 0xbe, 0xd7, 0x84, 0xb5, 0xc8, 0x77, 0x20, 0x4e, 0x73, 0x21, 0x9a, 0x52, 0x3, 0x25, 0x1f, @@ -137,4 +137,18 @@ mod tests { ]; assert_eq(keccak256(input, 13), result); } + + #[test] + fn hash_longer_than_136_bytes() { + let input = "123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789 123456789" + .as_bytes(); + assert(input.len() > 136); + + let result = [ + 0x1d, 0xca, 0xeb, 0xdf, 0xd9, 0xd6, 0x24, 0x67, 0x1c, 0x18, 0x16, 0xda, 0xd, 0x8a, 0xeb, + 0xa8, 0x75, 0x71, 0x2c, 0xc, 0x89, 0xe0, 0x25, 0x2, 0xe8, 0xb6, 0x5e, 0x16, 0x5, 0x55, + 0xe4, 0x40, + ]; + assert_eq(keccak256(input, input.len()), result); + } } diff --git a/noir/noir-repo/noir_stdlib/src/hash/sha256.nr b/noir/noir-repo/noir_stdlib/src/hash/sha256.nr index 41df099c15d7..d55044907ac4 100644 --- a/noir/noir-repo/noir_stdlib/src/hash/sha256.nr +++ b/noir/noir-repo/noir_stdlib/src/hash/sha256.nr @@ -519,10 +519,10 @@ fn hash_final_block(msg_block: MSG_BLOCK, mut state: STATE) -> HASH { mod tests { use super::{ - attach_len_to_msg_block, build_msg_block, byte_into_item, get_item_byte, lshift8, make_item, + attach_len_to_msg_block, build_msg_block, byte_into_item, get_item_byte, make_item, set_item_byte_then_zeros, set_item_zeros, }; - use super::{INT_BLOCK, INT_BLOCK_SIZE, MSG_BLOCK}; + use super::INT_BLOCK; use super::sha256_var; #[test] diff --git a/noir/noir-repo/test_programs/execution_success/fmtstr_with_global/Nargo.toml b/noir/noir-repo/test_programs/execution_success/fmtstr_with_global/Nargo.toml new file mode 100644 index 000000000000..889683f7410f --- /dev/null +++ b/noir/noir-repo/test_programs/execution_success/fmtstr_with_global/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "fmtstr_with_global" +type = "bin" +authors = [""] +compiler_version = ">=0.32.0" + +[dependencies] diff --git a/noir/noir-repo/test_programs/execution_success/fmtstr_with_global/src/main.nr b/noir/noir-repo/test_programs/execution_success/fmtstr_with_global/src/main.nr new file mode 100644 index 000000000000..8b9c96350153 --- /dev/null +++ b/noir/noir-repo/test_programs/execution_success/fmtstr_with_global/src/main.nr @@ -0,0 +1,5 @@ +global FOO = 1; + +fn main() { + println(f"foo = {FOO}"); +} diff --git a/noir/noir-repo/tooling/debugger/src/repl.rs b/noir/noir-repo/tooling/debugger/src/repl.rs index 012af0e88e87..486e84060f0c 100644 --- a/noir/noir-repo/tooling/debugger/src/repl.rs +++ b/noir/noir-repo/tooling/debugger/src/repl.rs @@ -5,6 +5,8 @@ use acvm::acir::circuit::brillig::{BrilligBytecode, BrilligFunctionId}; use acvm::acir::circuit::{Circuit, Opcode, OpcodeLocation}; use acvm::acir::native_types::{Witness, WitnessMap, WitnessStack}; use acvm::brillig_vm::brillig::Opcode as BrilligOpcode; +use acvm::brillig_vm::MemoryValue; +use acvm::AcirField; use acvm::{BlackBoxFunctionSolver, FieldElement}; use nargo::NargoError; use noirc_driver::CompiledProgram; @@ -369,6 +371,12 @@ impl<'a, B: BlackBoxFunctionSolver> ReplDebugger<'a, B> { }; for (index, value) in memory.iter().enumerate() { + // Zero field is the default value, we omit it when printing memory + if let MemoryValue::Field(field) = value { + if field == &FieldElement::zero() { + continue; + } + } println!("{index} = {}", value); } } diff --git a/noir/noir-repo/tooling/lsp/src/attribute_reference_finder.rs b/noir/noir-repo/tooling/lsp/src/attribute_reference_finder.rs index 39e1385a6e84..22afa086303b 100644 --- a/noir/noir-repo/tooling/lsp/src/attribute_reference_finder.rs +++ b/noir/noir-repo/tooling/lsp/src/attribute_reference_finder.rs @@ -13,7 +13,10 @@ use noirc_frontend::{ graph::CrateId, hir::{ def_map::{CrateDefMap, LocalModuleId, ModuleId}, - resolution::path_resolver::{PathResolver, StandardPathResolver}, + resolution::{ + import::PathResolutionItem, + path_resolver::{PathResolver, StandardPathResolver}, + }, }, node_interner::ReferenceId, parser::{ParsedSubModule, Parser}, @@ -22,8 +25,6 @@ use noirc_frontend::{ ParsedModule, }; -use crate::modules::module_def_id_to_reference_id; - pub(crate) struct AttributeReferenceFinder<'a> { byte_index: usize, /// The module ID in scope. This might change as we traverse the AST @@ -106,6 +107,20 @@ impl<'a> Visitor for AttributeReferenceFinder<'a> { return; }; - self.reference_id = Some(module_def_id_to_reference_id(result.module_def_id)); + self.reference_id = Some(path_resolution_item_to_reference_id(result.item)); + } +} + +fn path_resolution_item_to_reference_id(item: PathResolutionItem) -> ReferenceId { + match item { + PathResolutionItem::Module(module_id) => ReferenceId::Module(module_id), + PathResolutionItem::Struct(struct_id) => ReferenceId::Struct(struct_id), + PathResolutionItem::TypeAlias(type_alias_id) => ReferenceId::Alias(type_alias_id), + PathResolutionItem::Trait(trait_id) => ReferenceId::Trait(trait_id), + PathResolutionItem::Global(global_id) => ReferenceId::Global(global_id), + PathResolutionItem::ModuleFunction(func_id) + | PathResolutionItem::StructFunction(_, _, func_id) + | PathResolutionItem::TypeAliasFunction(_, _, func_id) + | PathResolutionItem::TraitFunction(_, _, func_id) => ReferenceId::Function(func_id), } } diff --git a/noir/noir-repo/tooling/lsp/src/requests/code_action.rs b/noir/noir-repo/tooling/lsp/src/requests/code_action.rs index f3e9130e17db..5c2831be7e95 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/code_action.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/code_action.rs @@ -12,7 +12,10 @@ use lsp_types::{ }; use noirc_errors::Span; use noirc_frontend::{ - ast::{ConstructorExpression, ItemVisibility, NoirTraitImpl, Path, UseTree, Visitor}, + ast::{ + CallExpression, ConstructorExpression, ItemVisibility, MethodCallExpression, NoirTraitImpl, + Path, UseTree, Visitor, + }, graph::CrateId, hir::def_map::{CrateDefMap, LocalModuleId, ModuleId}, node_interner::NodeInterner, @@ -29,6 +32,7 @@ use super::{process_request, to_lsp_location}; mod fill_struct_fields; mod implement_missing_members; mod import_or_qualify; +mod remove_bang_from_call; mod remove_unused_import; mod tests; @@ -250,4 +254,32 @@ impl<'a> Visitor for CodeActionFinder<'a> { true } + + fn visit_call_expression(&mut self, call: &CallExpression, span: Span) -> bool { + if !self.includes_span(span) { + return false; + } + + if call.is_macro_call { + self.remove_bang_from_call(call.func.span); + } + + true + } + + fn visit_method_call_expression( + &mut self, + method_call: &MethodCallExpression, + span: Span, + ) -> bool { + if !self.includes_span(span) { + return false; + } + + if method_call.is_macro_call { + self.remove_bang_from_call(method_call.method_name.span()); + } + + true + } } diff --git a/noir/noir-repo/tooling/lsp/src/requests/code_action/remove_bang_from_call.rs b/noir/noir-repo/tooling/lsp/src/requests/code_action/remove_bang_from_call.rs new file mode 100644 index 000000000000..90f4fef0efd2 --- /dev/null +++ b/noir/noir-repo/tooling/lsp/src/requests/code_action/remove_bang_from_call.rs @@ -0,0 +1,97 @@ +use lsp_types::TextEdit; +use noirc_errors::{Location, Span}; +use noirc_frontend::{node_interner::ReferenceId, QuotedType, Type}; + +use crate::byte_span_to_range; + +use super::CodeActionFinder; + +impl<'a> CodeActionFinder<'a> { + pub(super) fn remove_bang_from_call(&mut self, span: Span) { + // If we can't find the referenced function, there's nothing we can do + let Some(ReferenceId::Function(func_id)) = + self.interner.find_referenced(Location::new(span, self.file)) + else { + return; + }; + + // If the return type is Quoted, all is good + let func_meta = self.interner.function_meta(&func_id); + if let Type::Quoted(QuotedType::Quoted) = func_meta.return_type() { + return; + } + + // The `!` comes right after the name + let byte_span = span.end() as usize..span.end() as usize + 1; + let Some(range) = byte_span_to_range(self.files, self.file, byte_span) else { + return; + }; + + let title = "Remove `!` from call".to_string(); + let text_edit = TextEdit { range, new_text: "".to_string() }; + + let code_action = self.new_quick_fix(title, text_edit); + self.code_actions.push(code_action); + } +} + +#[cfg(test)] +mod tests { + use tokio::test; + + use crate::requests::code_action::tests::assert_code_action; + + #[test] + async fn test_removes_bang_from_call() { + let title = "Remove `!` from call"; + + let src = r#" + fn foo() {} + + fn main() { + fo>|| Option String { let func_meta = args.interner.function_meta(&id); + let func_modifiers = args.interner.function_modifiers(&id); + let func_name_definition_id = args.interner.definition(func_meta.name.id); let mut string = String::new(); let formatted_parent_module = format_parent_module(ReferenceId::Function(id), args, &mut string); - let formatted_parent_struct = if let Some(struct_id) = func_meta.struct_id { + + let formatted_parent_type = if let Some(trait_impl_id) = func_meta.trait_impl { + let trait_impl = args.interner.get_trait_implementation(trait_impl_id); + let trait_impl = trait_impl.borrow(); + let trait_ = args.interner.get_trait(trait_impl.trait_id); + + let generics: Vec<_> = + trait_impl + .trait_generics + .iter() + .filter_map(|generic| { + if let Type::NamedGeneric(_, name) = generic { + Some(name) + } else { + None + } + }) + .collect(); + + string.push('\n'); + string.push_str(" impl"); + if !generics.is_empty() { + string.push('<'); + for (index, generic) in generics.into_iter().enumerate() { + if index > 0 { + string.push_str(", "); + } + string.push_str(generic); + } + string.push('>'); + } + + string.push(' '); + string.push_str(&trait_.name.0.contents); + if !trait_impl.trait_generics.is_empty() { + string.push('<'); + for (index, generic) in trait_impl.trait_generics.iter().enumerate() { + if index > 0 { + string.push_str(", "); + } + string.push_str(&generic.to_string()); + } + string.push('>'); + } + + string.push_str(" for "); + string.push_str(&trait_impl.typ.to_string()); + + true + } else if let Some(trait_id) = func_meta.trait_id { + let trait_ = args.interner.get_trait(trait_id); + string.push('\n'); + string.push_str(" trait "); + string.push_str(&trait_.name.0.contents); + format_generics(&trait_.generics, &mut string); + + true + } else if let Some(struct_id) = func_meta.struct_id { let struct_type = args.interner.get_struct(struct_id); let struct_type = struct_type.borrow(); if formatted_parent_module { string.push_str("::"); } string.push_str(&struct_type.name.0.contents); + string.push('\n'); + string.push_str(" "); + string.push_str("impl"); + + let impl_generics: Vec<_> = func_meta + .all_generics + .iter() + .take(func_meta.all_generics.len() - func_meta.direct_generics.len()) + .cloned() + .collect(); + format_generics(&impl_generics, &mut string); + + string.push(' '); + string.push_str(&struct_type.name.0.contents); + format_generic_names(&impl_generics, &mut string); + true } else { false }; - if formatted_parent_module || formatted_parent_struct { + if formatted_parent_module || formatted_parent_type { string.push('\n'); } string.push_str(" "); + + if func_modifiers.visibility != ItemVisibility::Private { + string.push_str(&func_modifiers.visibility.to_string()); + string.push(' '); + } + if func_modifiers.is_unconstrained { + string.push_str("unconstrained "); + } + if func_modifiers.is_comptime { + string.push_str("comptime "); + } + string.push_str("fn "); string.push_str(&func_name_definition_id.name); format_generics(&func_meta.direct_generics, &mut string); @@ -411,21 +498,55 @@ fn format_local(id: DefinitionId, args: &ProcessRequestCallbackArgs) -> String { string } -/// Some doc comments fn format_generics(generics: &Generics, string: &mut String) { + format_generics_impl( + generics, false, // only show names + string, + ); +} + +fn format_generic_names(generics: &Generics, string: &mut String) { + format_generics_impl( + generics, true, // only show names + string, + ); +} + +fn format_generics_impl(generics: &Generics, only_show_names: bool, string: &mut String) { if generics.is_empty() { return; } string.push('<'); for (index, generic) in generics.iter().enumerate() { - string.push_str(&generic.name); - if index != generics.len() - 1 { + if index > 0 { string.push_str(", "); } + + if only_show_names { + string.push_str(&generic.name); + } else { + match generic.kind() { + noirc_frontend::Kind::Any | noirc_frontend::Kind::Normal => { + string.push_str(&generic.name); + } + noirc_frontend::Kind::IntegerOrField | noirc_frontend::Kind::Integer => { + string.push_str("let "); + string.push_str(&generic.name); + string.push_str(": u32"); + } + noirc_frontend::Kind::Numeric(typ) => { + string.push_str("let "); + string.push_str(&generic.name); + string.push_str(": "); + string.push_str(&typ.to_string()); + } + } + } } string.push('>'); } + fn format_pattern(pattern: &HirPattern, interner: &NodeInterner, string: &mut String) { match pattern { HirPattern::Identifier(ident) => { @@ -647,6 +768,11 @@ mod hover_tests { use tokio::test; async fn assert_hover(directory: &str, file: &str, position: Position, expected_text: &str) { + let hover_text = get_hover_text(directory, file, position).await; + assert_eq!(hover_text, expected_text); + } + + async fn get_hover_text(directory: &str, file: &str, position: Position) -> String { let (mut state, noir_text_document) = test_utils::init_lsp_server(directory).await; // noir_text_document is always `src/main.nr` in the workspace directory, so let's go to the workspace dir @@ -673,7 +799,7 @@ mod hover_tests { panic!("Expected hover contents to be Markup"); }; - assert_eq!(markup.value, expected_text); + markup.value } #[test] @@ -759,7 +885,7 @@ mod hover_tests { "two/src/lib.nr", Position { line: 3, character: 4 }, r#" one - fn function_one()"#, + pub fn function_one()"#, ) .await; } @@ -771,7 +897,7 @@ mod hover_tests { "two/src/lib.nr", Position { line: 2, character: 7 }, r#" two - fn function_two()"#, + pub fn function_two()"#, ) .await; } @@ -783,6 +909,7 @@ mod hover_tests { "two/src/lib.nr", Position { line: 20, character: 6 }, r#" one::subone::SubOneStruct + impl SubOneStruct fn foo(self, x: i32, y: i32) -> Field"#, ) .await; @@ -892,7 +1019,7 @@ mod hover_tests { assert_hover( "workspace", "two/src/lib.nr", - Position { line: 43, character: 4 }, + Position { line: 42, character: 4 }, r#" two mod other"#, ) @@ -904,7 +1031,7 @@ mod hover_tests { assert_hover( "workspace", "two/src/lib.nr", - Position { line: 44, character: 11 }, + Position { line: 43, character: 11 }, r#" two mod other"#, ) @@ -955,8 +1082,32 @@ mod hover_tests { "workspace", "two/src/lib.nr", Position { line: 54, character: 2 }, - " two\n fn attr(_: FunctionDefinition) -> Quoted", + " two\n comptime fn attr(_: FunctionDefinition) -> Quoted", ) .await; } + + #[test] + async fn hover_on_generic_struct_function() { + let hover_text = + get_hover_text("workspace", "two/src/lib.nr", Position { line: 70, character: 11 }) + .await; + assert!(hover_text.starts_with( + " two::Foo + impl Foo + fn new() -> Foo" + )); + } + + #[test] + async fn hover_on_trait_impl_function_call() { + let hover_text = + get_hover_text("workspace", "two/src/lib.nr", Position { line: 83, character: 16 }) + .await; + assert!(hover_text.starts_with( + " two + impl Bar for Foo + pub fn bar_stuff(self)" + )); + } } diff --git a/noir/noir-repo/tooling/lsp/test_programs/workspace/two/src/lib.nr b/noir/noir-repo/tooling/lsp/test_programs/workspace/two/src/lib.nr index c6b516d88ab3..2dec902f3278 100644 --- a/noir/noir-repo/tooling/lsp/test_programs/workspace/two/src/lib.nr +++ b/noir/noir-repo/tooling/lsp/test_programs/workspace/two/src/lib.nr @@ -41,8 +41,8 @@ fn use_impl_method() { } mod other; -use other::another_function; use crate::other::other_function; +use other::another_function; use one::subone::GenericStruct; @@ -57,4 +57,30 @@ pub fn foo() {} comptime fn attr(_: FunctionDefinition) -> Quoted { quote { pub fn hello() {} } -} \ No newline at end of file +} + +struct Foo {} + +impl Foo { + fn new() -> Self { + Foo {} + } +} + +fn new_foo() -> Foo { + Foo::new() +} + +trait Bar { + fn bar_stuff(self); +} + +impl Bar for Foo { + fn bar_stuff(self) {} +} + +fn bar_stuff() { + let foo = Foo::new(); + foo.bar_stuff(); +} + diff --git a/noir/noir-repo/tooling/nargo_fmt/src/formatter/expression.rs b/noir/noir-repo/tooling/nargo_fmt/src/formatter/expression.rs index cd46b09f1906..239b52b7d04b 100644 --- a/noir/noir-repo/tooling/nargo_fmt/src/formatter/expression.rs +++ b/noir/noir-repo/tooling/nargo_fmt/src/formatter/expression.rs @@ -310,6 +310,24 @@ impl<'a, 'b> ChunkFormatter<'a, 'b> { } pub(super) fn format_quote(&mut self) -> ChunkGroup { + // A quote's prefix isn't captured in the token, so let's figure it out which one + // is it by looking at the source code. + let mut quote_source_code = + &self.source[self.token_span.start() as usize..self.token_span.end() as usize]; + + // Remove "quote" and any whitespace following it + quote_source_code = quote_source_code.strip_prefix("quote").unwrap(); + quote_source_code = quote_source_code.trim_start(); + + // The first char is the delimiter + let delimiter_start = quote_source_code.chars().next().unwrap(); + let delimiter_end = match delimiter_start { + '(' => ')', + '{' => '}', + '[' => ']', + _ => panic!("Unexpected delimiter: {}", delimiter_start), + }; + // We use the current token rather than the Tokens we got from `Token::Quote` because // the current token has whitespace and comments in it, while the one we got from // the parser doesn't. @@ -319,11 +337,13 @@ impl<'a, 'b> ChunkFormatter<'a, 'b> { let mut group = ChunkGroup::new(); group.verbatim(self.chunk(|formatter| { - formatter.write("quote {"); + formatter.write("quote"); + formatter.write_space(); + formatter.write(&delimiter_start.to_string()); for token in tokens.0 { formatter.write_source_span(token.to_span()); } - formatter.write("}"); + formatter.write(&delimiter_end.to_string()); })); group } @@ -2045,6 +2065,13 @@ global y = 1; assert_format(src, expected); } + #[test] + fn format_quote_with_bracket_delimiter() { + let src = "global x = quote [ 1 2 3 $four $(five) ];"; + let expected = "global x = quote [ 1 2 3 $four $(five) ];\n"; + assert_format(src, expected); + } + #[test] fn format_lambda_no_parameters() { let src = "global x = | | 1 ;"; diff --git a/noir/noir-repo/tooling/noirc_abi_wasm/test/browser/errors.test.ts b/noir/noir-repo/tooling/noirc_abi_wasm/test/browser/errors.test.ts index 0f75ff64a3ef..cc060cff4d67 100644 --- a/noir/noir-repo/tooling/noirc_abi_wasm/test/browser/errors.test.ts +++ b/noir/noir-repo/tooling/noirc_abi_wasm/test/browser/errors.test.ts @@ -9,7 +9,7 @@ it('errors when an integer input overflows', async () => { const { abi, inputs } = await import('../shared/uint_overflow'); expect(() => abiEncode(abi, inputs)).to.throw( - 'The value passed for parameter `foo` does not match the specified type:\nValue Field(2³⁸) does not fall within range of allowable values for a Integer { sign: Unsigned, width: 32 }', + 'The value passed for parameter `foo` does not match the specified type:\nValue Field(274877906944) does not fall within range of allowable values for a Integer { sign: Unsigned, width: 32 }', ); }); diff --git a/noir/noir-repo/tooling/noirc_abi_wasm/test/node/errors.test.ts b/noir/noir-repo/tooling/noirc_abi_wasm/test/node/errors.test.ts index fba451b4a8c8..491fd5a56713 100644 --- a/noir/noir-repo/tooling/noirc_abi_wasm/test/node/errors.test.ts +++ b/noir/noir-repo/tooling/noirc_abi_wasm/test/node/errors.test.ts @@ -5,7 +5,7 @@ it('errors when an integer input overflows', async () => { const { abi, inputs } = await import('../shared/uint_overflow'); expect(() => abiEncode(abi, inputs)).to.throw( - 'The value passed for parameter `foo` does not match the specified type:\nValue Field(2³⁸) does not fall within range of allowable values for a Integer { sign: Unsigned, width: 32 }', + 'The value passed for parameter `foo` does not match the specified type:\nValue Field(274877906944) does not fall within range of allowable values for a Integer { sign: Unsigned, width: 32 }', ); });