diff --git a/.noir-sync-commit b/.noir-sync-commit index e9d72241d77..d571609ea98 100644 --- a/.noir-sync-commit +++ b/.noir-sync-commit @@ -1 +1 @@ -164d29e4d1960d16fdeafe2cc8ea8144a769f7b2 +877b806ee02cb640472c6bb2b1ed7bc76b861a9b diff --git a/noir/bb-version b/noir/bb-version index 316ba4bd9e6..2f4c74eb2dd 100644 --- a/noir/bb-version +++ b/noir/bb-version @@ -1 +1 @@ -0.55.0 +0.56.0 diff --git a/noir/noir-repo/.github/workflows/gates_report_brillig.yml b/noir/noir-repo/.github/workflows/gates_report_brillig.yml index 4e12a6fcbca..e7ec30923f0 100644 --- a/noir/noir-repo/.github/workflows/gates_report_brillig.yml +++ b/noir/noir-repo/.github/workflows/gates_report_brillig.yml @@ -74,7 +74,7 @@ jobs: - name: Compare Brillig bytecode size reports id: brillig_bytecode_diff - uses: noir-lang/noir-gates-diff@3fb844067b25d1b59727ea600b614503b33503f4 + uses: noir-lang/noir-gates-diff@d88f7523b013b9edd3f31c5cfddaef87a3fe1b48 with: report: gates_report_brillig.json header: | diff --git a/noir/noir-repo/.github/workflows/mirror-external_libs.yml b/noir/noir-repo/.github/workflows/mirror-external_libs.yml deleted file mode 100644 index e577ac0ed92..00000000000 --- a/noir/noir-repo/.github/workflows/mirror-external_libs.yml +++ /dev/null @@ -1,8 +0,0 @@ -name: Mirror Repositories -on: - workflow_dispatch: {} -jobs: - lint: - runs-on: ubuntu-latest - steps: - - run: echo Dummy workflow TODO \ No newline at end of file diff --git a/noir/noir-repo/.github/workflows/test-js-packages.yml b/noir/noir-repo/.github/workflows/test-js-packages.yml index e45a482cc59..ff894f061ee 100644 --- a/noir/noir-repo/.github/workflows/test-js-packages.yml +++ b/noir/noir-repo/.github/workflows/test-js-packages.yml @@ -519,16 +519,12 @@ jobs: fail-fast: false matrix: project: - # Disabled as these are currently failing with many visibility errors - # - { repo: AztecProtocol/aztec-nr, path: ./ } - # - { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-contracts } - # Disabled as aztec-packages requires a setup-step in order to generate a `Nargo.toml` - #- { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits } + # Disabled as these are currently failing with many visibility errors + - { repo: AztecProtocol/aztec-nr, path: ./ } + - { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-contracts } + # Disabled as aztec-packages requires a setup-step in order to generate a `Nargo.toml` + #- { repo: AztecProtocol/aztec-packages, path: ./noir-projects/noir-protocol-circuits } - { repo: zac-williamson/noir-edwards, path: ./, ref: 037e44b2ee8557c51f6aef9bb9d63ea9e32722d1 } - # TODO: Enable these once they're passing against master again. - # - { repo: zac-williamson/noir-bignum, path: ./, ref: 030c2acce1e6b97c44a3bbbf3429ed96f20d72d3 } - # - { repo: vlayer-xyz/monorepo, path: ./, ref: ee46af88c025863872234eb05d890e1e447907cb } - # - { repo: hashcloak/noir-bigint, path: ./, ref: 940ddba3a5201b508e7b37a2ef643551afcf5ed8 } name: Check external repo - ${{ matrix.project.repo }} steps: - name: Checkout diff --git a/noir/noir-repo/Cargo.lock b/noir/noir-repo/Cargo.lock index 6a469bd67f4..cdc1d75a421 100644 --- a/noir/noir-repo/Cargo.lock +++ b/noir/noir-repo/Cargo.lock @@ -2753,6 +2753,7 @@ version = "0.34.0" dependencies = [ "acvm", "async-lsp", + "chumsky", "codespan-lsp", "convert_case 0.6.0", "fm", diff --git a/noir/noir-repo/acvm-repo/bn254_blackbox_solver/src/lib.rs b/noir/noir-repo/acvm-repo/bn254_blackbox_solver/src/lib.rs index 43ee6a9ddd2..952c4498d84 100644 --- a/noir/noir-repo/acvm-repo/bn254_blackbox_solver/src/lib.rs +++ b/noir/noir-repo/acvm-repo/bn254_blackbox_solver/src/lib.rs @@ -13,7 +13,10 @@ mod schnorr; use ark_ec::AffineRepr; pub use embedded_curve_ops::{embedded_curve_add, multi_scalar_mul}; pub use generator::generators::derive_generators; -pub use poseidon2::{field_from_hex, poseidon2_permutation, Poseidon2Config, POSEIDON2_CONFIG}; +pub use poseidon2::{ + field_from_hex, poseidon2_permutation, poseidon_hash, Poseidon2Config, Poseidon2Sponge, + POSEIDON2_CONFIG, +}; // Temporary hack, this ensure that we always use a bn254 field here // without polluting the feature flags of the `acir_field` crate. diff --git a/noir/noir-repo/acvm-repo/bn254_blackbox_solver/src/poseidon2.rs b/noir/noir-repo/acvm-repo/bn254_blackbox_solver/src/poseidon2.rs index dd3e8b725c2..64823e37029 100644 --- a/noir/noir-repo/acvm-repo/bn254_blackbox_solver/src/poseidon2.rs +++ b/noir/noir-repo/acvm-repo/bn254_blackbox_solver/src/poseidon2.rs @@ -543,6 +543,75 @@ impl<'a> Poseidon2<'a> { } } +/// Performs a poseidon hash with a sponge construction equivalent to the one in poseidon2.nr +pub fn poseidon_hash(inputs: &[FieldElement]) -> Result { + let two_pow_64 = 18446744073709551616_u128.into(); + let iv = FieldElement::from(inputs.len()) * two_pow_64; + let mut sponge = Poseidon2Sponge::new(iv, 3); + for input in inputs.iter() { + sponge.absorb(*input)?; + } + sponge.squeeze() +} + +pub struct Poseidon2Sponge<'a> { + rate: usize, + poseidon: Poseidon2<'a>, + squeezed: bool, + cache: Vec, + state: Vec, +} + +impl<'a> Poseidon2Sponge<'a> { + pub fn new(iv: FieldElement, rate: usize) -> Poseidon2Sponge<'a> { + let mut result = Poseidon2Sponge { + cache: Vec::with_capacity(rate), + state: vec![FieldElement::zero(); rate + 1], + squeezed: false, + rate, + poseidon: Poseidon2::new(), + }; + result.state[rate] = iv; + result + } + + fn perform_duplex(&mut self) -> Result<(), BlackBoxResolutionError> { + // zero-pad the cache + for _ in self.cache.len()..self.rate { + self.cache.push(FieldElement::zero()); + } + // add the cache into sponge state + for i in 0..self.rate { + self.state[i] += self.cache[i]; + } + self.state = self.poseidon.permutation(&self.state, 4)?; + Ok(()) + } + + pub fn absorb(&mut self, input: FieldElement) -> Result<(), BlackBoxResolutionError> { + assert!(!self.squeezed); + if self.cache.len() == self.rate { + // If we're absorbing, and the cache is full, apply the sponge permutation to compress the cache + self.perform_duplex()?; + self.cache = vec![input]; + } else { + // If we're absorbing, and the cache is not full, add the input into the cache + self.cache.push(input); + } + Ok(()) + } + + pub fn squeeze(&mut self) -> Result { + assert!(!self.squeezed); + // If we're in absorb mode, apply sponge permutation to compress the cache. + self.perform_duplex()?; + self.squeezed = true; + + // Pop one item off the top of the permutation and return it. + Ok(self.state[0]) + } +} + #[cfg(test)] mod test { use acir::AcirField; @@ -562,4 +631,19 @@ mod test { ]; assert_eq!(result, expected_result); } + + #[test] + fn hash_smoke_test() { + let fields = [ + FieldElement::from(1u128), + FieldElement::from(2u128), + FieldElement::from(3u128), + FieldElement::from(4u128), + ]; + let result = super::poseidon_hash(&fields).expect("should hash successfully"); + assert_eq!( + result, + field_from_hex("130bf204a32cac1f0ace56c78b731aa3809f06df2731ebcf6b3464a15788b1b9"), + ); + } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 8929122f515..e764c81b023 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -553,14 +553,7 @@ impl<'block> BrilligBlock<'block> { let results = dfg.instruction_results(instruction_id); let source = self.convert_ssa_single_addr_value(arguments[0], dfg); - - let radix: u32 = dfg - .get_numeric_constant(arguments[1]) - .expect("Radix should be known") - .try_to_u64() - .expect("Radix should fit in u64") - .try_into() - .expect("Radix should be u32"); + let radix = self.convert_ssa_single_addr_value(arguments[1], dfg); let target_array = self .variables @@ -595,13 +588,17 @@ impl<'block> BrilligBlock<'block> { ) .extract_array(); + let two = self.brillig_context.make_usize_constant_instruction(2_usize.into()); + self.brillig_context.codegen_to_radix( source, target_array, - 2, + two, matches!(endianness, Endian::Big), true, ); + + self.brillig_context.deallocate_single_addr(two); } // `Intrinsic::AsWitness` is used to provide hints to acir-gen on optimal expression splitting. diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs index cf484fa5038..f9ded224b33 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/constant_allocation.rs @@ -1,7 +1,7 @@ //! This module analyzes the usage of constants in a given function and decides an allocation point for them. //! The allocation point will be the common dominator of all the places where the constant is used. //! By allocating in the common dominator, we can cache the constants for all subsequent uses. -use fxhash::FxHashMap as HashMap; +use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; use crate::ssa::ir::{ basic_block::BasicBlockId, @@ -26,20 +26,23 @@ pub(crate) struct ConstantAllocation { constant_usage: HashMap>>, allocation_points: HashMap>>, dominator_tree: DominatorTree, + blocks_within_loops: HashSet, } impl ConstantAllocation { pub(crate) fn from_function(func: &Function) -> Self { let cfg = ControlFlowGraph::with_function(func); let post_order = PostOrder::with_function(func); - let dominator_tree = DominatorTree::with_cfg_and_post_order(&cfg, &post_order); + let mut dominator_tree = DominatorTree::with_cfg_and_post_order(&cfg, &post_order); + let blocks_within_loops = find_all_blocks_within_loops(func, &cfg, &mut dominator_tree); let mut instance = ConstantAllocation { constant_usage: HashMap::default(), allocation_points: HashMap::default(), dominator_tree, + blocks_within_loops, }; instance.collect_constant_usage(func); - instance.decide_allocation_points(); + instance.decide_allocation_points(func); instance } @@ -95,16 +98,16 @@ impl ConstantAllocation { } } - fn decide_allocation_points(&mut self) { + fn decide_allocation_points(&mut self, func: &Function) { for (constant_id, usage_in_blocks) in self.constant_usage.iter() { let block_ids: Vec<_> = usage_in_blocks.iter().map(|(block_id, _)| *block_id).collect(); - let common_dominator = self.common_dominator(&block_ids); + let allocation_point = self.decide_allocation_point(*constant_id, &block_ids, func); - // If the common dominator is one of the places where it's used, we take the first usage in the common dominator. - // Otherwise, we allocate it at the terminator of the common dominator. + // If the allocation point is one of the places where it's used, we take the first usage in the allocation point. + // Otherwise, we allocate it at the terminator of the allocation point. let location = if let Some(locations_in_common_dominator) = - usage_in_blocks.get(&common_dominator) + usage_in_blocks.get(&allocation_point) { *locations_in_common_dominator .first() @@ -114,7 +117,7 @@ impl ConstantAllocation { }; self.allocation_points - .entry(common_dominator) + .entry(allocation_point) .or_default() .entry(location) .or_default() @@ -122,21 +125,97 @@ impl ConstantAllocation { } } - fn common_dominator(&self, block_ids: &[BasicBlockId]) -> BasicBlockId { - if block_ids.len() == 1 { - return block_ids[0]; - } - - let mut common_dominator = block_ids[0]; + fn decide_allocation_point( + &self, + constant_id: ValueId, + blocks_where_is_used: &[BasicBlockId], + func: &Function, + ) -> BasicBlockId { + // Find the common dominator of all the blocks where the constant is used. + let common_dominator = if blocks_where_is_used.len() == 1 { + blocks_where_is_used[0] + } else { + let mut common_dominator = blocks_where_is_used[0]; + + for block_id in blocks_where_is_used.iter().skip(1) { + common_dominator = + self.dominator_tree.common_dominator(common_dominator, *block_id); + } - for block_id in block_ids.iter().skip(1) { - common_dominator = self.dominator_tree.common_dominator(common_dominator, *block_id); + common_dominator + }; + // If the value only contains constants, it's safe to hoist outside of any loop + if func.dfg.is_constant(constant_id) { + self.exit_loops(common_dominator) + } else { + common_dominator } + } - common_dominator + /// Returns the nearest dominator that is outside of any loop. + fn exit_loops(&self, block: BasicBlockId) -> BasicBlockId { + let mut current_block = block; + while self.blocks_within_loops.contains(¤t_block) { + current_block = self + .dominator_tree + .immediate_dominator(current_block) + .expect("No dominator found when trying to allocate a constant outside of a loop"); + } + current_block } } pub(crate) fn is_constant_value(id: ValueId, dfg: &DataFlowGraph) -> bool { matches!(&dfg[dfg.resolve(id)], Value::NumericConstant { .. } | Value::Array { .. }) } + +/// For a given function, finds all the blocks that are within loops +fn find_all_blocks_within_loops( + func: &Function, + cfg: &ControlFlowGraph, + dominator_tree: &mut DominatorTree, +) -> HashSet { + let mut blocks_in_loops = HashSet::default(); + for block_id in func.reachable_blocks() { + let block = &func.dfg[block_id]; + let successors = block.successors(); + for successor_id in successors { + if dominator_tree.dominates(successor_id, block_id) { + blocks_in_loops.extend(find_blocks_in_loop(successor_id, block_id, cfg)); + } + } + } + + blocks_in_loops +} + +/// Return each block that is in a loop starting in the given header block. +/// Expects back_edge_start -> header to be the back edge of the loop. +fn find_blocks_in_loop( + header: BasicBlockId, + back_edge_start: BasicBlockId, + cfg: &ControlFlowGraph, +) -> HashSet { + let mut blocks = HashSet::default(); + blocks.insert(header); + + let mut insert = |block, stack: &mut Vec| { + if !blocks.contains(&block) { + blocks.insert(block); + stack.push(block); + } + }; + + // Starting from the back edge of the loop, each predecessor of this block until + // the header is within the loop. + let mut stack = vec![]; + insert(back_edge_start, &mut stack); + + while let Some(block) = stack.pop() { + for predecessor in cfg.predecessors(block) { + insert(predecessor, &mut stack); + } + } + + blocks +} diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs index 73e88cee676..92595292bf0 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/variable_liveness.rs @@ -570,28 +570,34 @@ mod test { let liveness = VariableLiveness::from_function(func, &constants); assert!(liveness.get_live_in(&func.entry_block()).is_empty()); - assert_eq!(liveness.get_live_in(&b1), &FxHashSet::from_iter([v0, v1, v3, v4].into_iter())); + assert_eq!( + liveness.get_live_in(&b1), + &FxHashSet::from_iter([v0, v1, v3, v4, twenty_seven, one].into_iter()) + ); assert_eq!(liveness.get_live_in(&b3), &FxHashSet::from_iter([v3].into_iter())); - assert_eq!(liveness.get_live_in(&b2), &FxHashSet::from_iter([v0, v1, v3, v4].into_iter())); + assert_eq!( + liveness.get_live_in(&b2), + &FxHashSet::from_iter([v0, v1, v3, v4, twenty_seven, one].into_iter()) + ); assert_eq!( liveness.get_live_in(&b4), - &FxHashSet::from_iter([v0, v1, v3, v4, v6, v7].into_iter()) + &FxHashSet::from_iter([v0, v1, v3, v4, v6, v7, twenty_seven, one].into_iter()) ); assert_eq!( liveness.get_live_in(&b6), - &FxHashSet::from_iter([v0, v1, v3, v4, one].into_iter()) + &FxHashSet::from_iter([v0, v1, v3, v4, twenty_seven, one].into_iter()) ); assert_eq!( liveness.get_live_in(&b5), - &FxHashSet::from_iter([v0, v1, v3, v4, v6, v7, one].into_iter()) + &FxHashSet::from_iter([v0, v1, v3, v4, v6, v7, twenty_seven, one].into_iter()) ); assert_eq!( liveness.get_live_in(&b7), - &FxHashSet::from_iter([v0, v1, v3, v4, v6, v7, one].into_iter()) + &FxHashSet::from_iter([v0, v1, v3, v4, v6, v7, twenty_seven, one].into_iter()) ); assert_eq!( liveness.get_live_in(&b8), - &FxHashSet::from_iter([v0, v1, v3, v4, v6, v7, one].into_iter()) + &FxHashSet::from_iter([v0, v1, v3, v4, v6, v7, twenty_seven, one].into_iter()) ); let block_3 = &func.dfg[b3]; diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_intrinsic.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_intrinsic.rs index c9c31267d7b..5f4781788f5 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_intrinsic.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_ir/codegen_intrinsic.rs @@ -68,21 +68,20 @@ impl BrilligContext< &mut self, source_field: SingleAddrVariable, target_array: BrilligArray, - radix: u32, + radix: SingleAddrVariable, big_endian: bool, output_bits: bool, // If true will generate bit limbs, if false will generate byte limbs ) { assert!(source_field.bit_size == F::max_num_bits()); + assert!(radix.bit_size == 32); self.codegen_initialize_array(target_array); let heap_array = self.codegen_brillig_array_to_heap_array(target_array); - let radix_var = self.make_constant_instruction(F::from(radix as u128), 32); - self.black_box_op_instruction(BlackBoxOp::ToRadix { input: source_field.address, - radix: radix_var.address, + radix: radix.address, output: heap_array, output_bits, }); @@ -93,6 +92,5 @@ impl BrilligContext< self.deallocate_single_addr(items_len); } self.deallocate_register(heap_array.pointer); - self.deallocate_register(radix_var.address); } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs index 9221a925aa8..991ff22c902 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/function_inserter.rs @@ -5,7 +5,7 @@ use crate::ssa::ir::types::Type; use super::{ basic_block::BasicBlockId, dfg::{CallStack, InsertInstructionResult}, - function::{Function, RuntimeType}, + function::Function, instruction::{Instruction, InstructionId}, value::ValueId, }; @@ -46,14 +46,7 @@ impl<'f> FunctionInserter<'f> { if let Some(fetched_value) = self.const_arrays.get(&(new_array.clone(), typ.clone())) { - // Arrays in ACIR are immutable, but in Brillig arrays are copy-on-write - // so for function's with a Brillig runtime we make sure to check that value - // in our constants array map matches the resolved array value id. - if matches!(self.function.runtime(), RuntimeType::Acir(_)) { - return *fetched_value; - } else if *fetched_value == value { - return value; - } + return *fetched_value; }; let new_array_clone = new_array.clone(); diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/array_set.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/array_set.rs index 491a17adb66..6d48b8c0d67 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/array_set.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/array_set.rs @@ -2,12 +2,13 @@ use crate::ssa::{ ir::{ basic_block::BasicBlockId, dfg::DataFlowGraph, - instruction::{Instruction, InstructionId}, + instruction::{Instruction, InstructionId, TerminatorInstruction}, types::Type::{Array, Slice}, + value::ValueId, }, ssa_gen::Ssa, }; -use fxhash::{FxHashMap as HashMap, FxHashSet}; +use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; impl Ssa { /// Map arrays with the last instruction that uses it @@ -16,28 +17,42 @@ impl Ssa { #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn array_set_optimization(mut self) -> Self { for func in self.functions.values_mut() { - let mut reachable_blocks = func.reachable_blocks(); - let block = if !func.runtime().is_entry_point() { + let reachable_blocks = func.reachable_blocks(); + + if !func.runtime().is_entry_point() { assert_eq!(reachable_blocks.len(), 1, "Expected there to be 1 block remaining in Acir function for array_set optimization"); - reachable_blocks.pop_first().unwrap() - } else { - // We only apply the array set optimization in the return block of Brillig functions - func.find_last_block() - }; + } + let mut array_to_last_use = HashMap::default(); + let mut instructions_to_update = HashSet::default(); + let mut arrays_from_load = HashSet::default(); - let instructions_to_update = analyze_last_uses(&func.dfg, block); - make_mutable(&mut func.dfg, block, instructions_to_update); + for block in reachable_blocks.iter() { + analyze_last_uses( + &func.dfg, + *block, + &mut array_to_last_use, + &mut instructions_to_update, + &mut arrays_from_load, + ); + } + for block in reachable_blocks { + make_mutable(&mut func.dfg, block, &instructions_to_update); + } } self } } -/// Returns the set of ArraySet instructions that can be made mutable +/// Builds the set of ArraySet instructions that can be made mutable /// because their input value is unused elsewhere afterward. -fn analyze_last_uses(dfg: &DataFlowGraph, block_id: BasicBlockId) -> FxHashSet { +fn analyze_last_uses( + dfg: &DataFlowGraph, + block_id: BasicBlockId, + array_to_last_use: &mut HashMap, + instructions_that_can_be_made_mutable: &mut HashSet, + arrays_from_load: &mut HashSet, +) { let block = &dfg[block_id]; - let mut array_to_last_use = HashMap::default(); - let mut instructions_that_can_be_made_mutable = FxHashSet::default(); for instruction_id in block.instructions() { match &dfg[*instruction_id] { @@ -54,7 +69,22 @@ fn analyze_last_uses(dfg: &DataFlowGraph, block_id: BasicBlockId) -> FxHashSet { for argument in arguments { @@ -68,18 +98,22 @@ fn analyze_last_uses(dfg: &DataFlowGraph, block_id: BasicBlockId) -> FxHashSet { + let result = dfg.instruction_results(*instruction_id)[0]; + if matches!(dfg.type_of_value(result), Array { .. } | Slice { .. }) { + arrays_from_load.insert(result); + } + } _ => (), } } - - instructions_that_can_be_made_mutable } /// Make each ArraySet instruction in `instructions_to_update` mutable. fn make_mutable( dfg: &mut DataFlowGraph, block_id: BasicBlockId, - instructions_to_update: FxHashSet, + instructions_to_update: &HashSet, ) { if instructions_to_update.is_empty() { return; @@ -105,3 +139,129 @@ fn make_mutable( *dfg[block_id].instructions_mut() = instructions; } + +#[cfg(test)] +mod tests { + use std::sync::Arc; + + use im::vector; + + use crate::ssa::{ + function_builder::FunctionBuilder, + ir::{ + function::RuntimeType, + instruction::{BinaryOp, Instruction}, + map::Id, + types::Type, + }, + }; + + #[test] + fn array_set_in_loop_with_conditional_clone() { + // We want to make sure that we do not mark a single array set mutable which is loaded + // from and cloned in a loop. If the array is inadvertently marked mutable, and is cloned in a previous iteration + // of the loop, its clone will also be altered. + // + // acir(inline) fn main f0 { + // b0(): + // v2 = allocate + // store [Field 0, Field 0, Field 0, Field 0, Field 0] at v2 + // v3 = allocate + // store [Field 0, Field 0, Field 0, Field 0, Field 0] at v3 + // jmp b1(u32 0) + // b1(v5: u32): + // v7 = lt v5, u32 5 + // jmpif v7 then: b3, else: b2 + // b3(): + // v8 = eq v5, u32 5 + // jmpif v8 then: b4, else: b5 + // b4(): + // v9 = load v2 + // store v9 at v3 + // jmp b5() + // b5(): + // v10 = load v2 + // v12 = array_set v10, index v5, value Field 20 + // store v12 at v2 + // v14 = add v5, u32 1 + // jmp b1(v14) + // b2(): + // return + // } + let main_id = Id::test_new(0); + let mut builder = FunctionBuilder::new("main".into(), main_id); + builder.set_runtime(RuntimeType::Brillig); + + let array_type = Type::Array(Arc::new(vec![Type::field()]), 5); + let zero = builder.field_constant(0u128); + let array_constant = + builder.array_constant(vector![zero, zero, zero, zero, zero], array_type.clone()); + + let v2 = builder.insert_allocate(array_type.clone()); + + builder.insert_store(v2, array_constant); + + let v3 = builder.insert_allocate(array_type.clone()); + builder.insert_store(v3, array_constant); + + let b1 = builder.insert_block(); + let zero_u32 = builder.numeric_constant(0u128, Type::unsigned(32)); + builder.terminate_with_jmp(b1, vec![zero_u32]); + + // Loop header + builder.switch_to_block(b1); + let v5 = builder.add_block_parameter(b1, Type::unsigned(32)); + let five = builder.numeric_constant(5u128, Type::unsigned(32)); + let v7 = builder.insert_binary(v5, BinaryOp::Lt, five); + + let b2 = builder.insert_block(); + let b3 = builder.insert_block(); + let b4 = builder.insert_block(); + let b5 = builder.insert_block(); + builder.terminate_with_jmpif(v7, b3, b2); + + // Loop body + // b3 is the if statement conditional + builder.switch_to_block(b3); + let two = builder.numeric_constant(5u128, Type::unsigned(32)); + let v8 = builder.insert_binary(v5, BinaryOp::Eq, two); + builder.terminate_with_jmpif(v8, b4, b5); + + // b4 is the rest of the loop after the if statement + builder.switch_to_block(b4); + let v9 = builder.insert_load(v2, array_type.clone()); + builder.insert_store(v3, v9); + builder.terminate_with_jmp(b5, vec![]); + + builder.switch_to_block(b5); + let v10 = builder.insert_load(v2, array_type.clone()); + let twenty = builder.field_constant(20u128); + let v12 = builder.insert_array_set(v10, v5, twenty); + builder.insert_store(v2, v12); + let one = builder.numeric_constant(1u128, Type::unsigned(32)); + let v14 = builder.insert_binary(v5, BinaryOp::Add, one); + builder.terminate_with_jmp(b1, vec![v14]); + + builder.switch_to_block(b2); + builder.terminate_with_return(vec![]); + + let ssa = builder.finish(); + // We expect the same result as above + let ssa = ssa.array_set_optimization(); + + let main = ssa.main(); + assert_eq!(main.reachable_blocks().len(), 6); + + let array_set_instructions = main.dfg[b5] + .instructions() + .iter() + .filter(|instruction| matches!(&main.dfg[**instruction], Instruction::ArraySet { .. })) + .collect::>(); + + assert_eq!(array_set_instructions.len(), 1); + if let Instruction::ArraySet { mutable, .. } = &main.dfg[*array_set_instructions[0]] { + // The single array set should not be marked mutable + assert!(!mutable); + } + } +} diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/die.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/die.rs index 7356998ceb8..02737f5645b 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -1,7 +1,6 @@ //! Dead Instruction Elimination (DIE) pass: Removes any instruction without side-effects for //! which the results are unused. -use std::collections::HashSet; - +use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; use im::Vector; use noirc_errors::Location; @@ -18,6 +17,8 @@ use crate::ssa::{ ssa_gen::{Ssa, SSA_WORD_SIZE}, }; +use super::rc::{pop_rc_for, RcInstruction}; + impl Ssa { /// Performs Dead Instruction Elimination (DIE) to remove any instructions with /// unused results. @@ -105,6 +106,16 @@ impl Context { let instructions_len = block.instructions().len(); + // We can track IncrementRc instructions per block to determine whether they are useless. + // IncrementRc and DecrementRc instructions are normally side effectual instructions, but we remove + // them if their value is not used anywhere in the function. However, even when their value is used, their existence + // is pointless logic if there is no array set between the increment and the decrement of the reference counter. + // We track per block whether an IncrementRc instruction has a paired DecrementRc instruction + // with the same value but no array set in between. + // If we see an inc/dec RC pair within a block we can safely remove both instructions. + let mut inc_rcs: HashMap> = HashMap::default(); + let mut inc_rcs_to_remove = HashSet::default(); + // Indexes of instructions that might be out of bounds. // We'll remove those, but before that we'll insert bounds checks for them. let mut possible_index_out_of_bounds_indexes = Vec::new(); @@ -131,6 +142,17 @@ impl Context { }); } } + + self.track_inc_rcs_to_remove( + *instruction_id, + function, + &mut inc_rcs, + &mut inc_rcs_to_remove, + ); + } + + for id in inc_rcs_to_remove { + self.instructions_to_remove.insert(id); } // If there are some instructions that might trigger an out of bounds error, @@ -155,6 +177,45 @@ impl Context { false } + fn track_inc_rcs_to_remove( + &self, + instruction_id: InstructionId, + function: &Function, + inc_rcs: &mut HashMap>, + inc_rcs_to_remove: &mut HashSet, + ) { + let instruction = &function.dfg[instruction_id]; + // DIE loops over a block in reverse order, so we insert an RC instruction for possible removal + // when we see a DecrementRc and check whether it was possibly mutated when we see an IncrementRc. + match instruction { + Instruction::IncrementRc { value } => { + if let Some(inc_rc) = pop_rc_for(*value, function, inc_rcs) { + if !inc_rc.possibly_mutated { + inc_rcs_to_remove.insert(inc_rc.id); + inc_rcs_to_remove.insert(instruction_id); + } + } + } + Instruction::DecrementRc { value } => { + let typ = function.dfg.type_of_value(*value); + + // We assume arrays aren't mutated until we find an array_set + let inc_rc = + RcInstruction { id: instruction_id, array: *value, possibly_mutated: false }; + inc_rcs.entry(typ).or_default().push(inc_rc); + } + Instruction::ArraySet { array, .. } => { + let typ = function.dfg.type_of_value(*array); + if let Some(inc_rcs) = inc_rcs.get_mut(&typ) { + for inc_rc in inc_rcs { + inc_rc.possibly_mutated = true; + } + } + } + _ => {} + } + } + /// Returns true if an instruction can be removed. /// /// An instruction can be removed as long as it has no side-effects, and none of its result @@ -509,10 +570,12 @@ fn apply_side_effects( #[cfg(test)] mod test { + use std::sync::Arc; + use crate::ssa::{ function_builder::FunctionBuilder, ir::{ - instruction::{BinaryOp, Intrinsic}, + instruction::{BinaryOp, Instruction, Intrinsic}, map::Id, types::Type, }, @@ -642,4 +705,78 @@ mod test { assert_eq!(main.dfg[main.entry_block()].instructions().len(), 1); } + + #[test] + fn remove_useless_paired_rcs_even_when_used() { + // acir(inline) fn main f0 { + // b0(v0: [Field; 2]): + // inc_rc v0 + // v2 = array_get v0, index u32 0 + // dec_rc v0 + // return v2 + // } + let main_id = Id::test_new(0); + + // Compiling main + let mut builder = FunctionBuilder::new("main".into(), main_id); + let v0 = builder.add_parameter(Type::Array(Arc::new(vec![Type::field()]), 2)); + builder.increment_array_reference_count(v0); + let zero = builder.numeric_constant(0u128, Type::unsigned(32)); + let v1 = builder.insert_array_get(v0, zero, Type::field()); + builder.decrement_array_reference_count(v0); + builder.terminate_with_return(vec![v1]); + + let ssa = builder.finish(); + let main = ssa.main(); + + // The instruction count never includes the terminator instruction + assert_eq!(main.dfg[main.entry_block()].instructions().len(), 3); + + // Expected output: + // + // acir(inline) fn main f0 { + // b0(v0: [Field; 2]): + // v2 = array_get v0, index u32 0 + // return v2 + // } + let ssa = ssa.dead_instruction_elimination(); + let main = ssa.main(); + + let instructions = main.dfg[main.entry_block()].instructions(); + assert_eq!(instructions.len(), 1); + assert!(matches!(&main.dfg[instructions[0]], Instruction::ArrayGet { .. })); + } + + #[test] + fn keep_paired_rcs_with_array_set() { + // acir(inline) fn main f0 { + // b0(v0: [Field; 2]): + // inc_rc v0 + // v2 = array_set v0, index u32 0, value u32 0 + // dec_rc v0 + // return v2 + // } + let main_id = Id::test_new(0); + + // Compiling main + let mut builder = FunctionBuilder::new("main".into(), main_id); + let v0 = builder.add_parameter(Type::Array(Arc::new(vec![Type::field()]), 2)); + builder.increment_array_reference_count(v0); + let zero = builder.numeric_constant(0u128, Type::unsigned(32)); + let v1 = builder.insert_array_set(v0, zero, zero); + builder.decrement_array_reference_count(v0); + builder.terminate_with_return(vec![v1]); + + let ssa = builder.finish(); + let main = ssa.main(); + + // The instruction count never includes the terminator instruction + assert_eq!(main.dfg[main.entry_block()].instructions().len(), 3); + + // We expect the output to be unchanged + let ssa = ssa.dead_instruction_elimination(); + let main = ssa.main(); + + assert_eq!(main.dfg[main.entry_block()].instructions().len(), 3); + } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/rc.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/rc.rs index 1750f2d80a5..06025fd9e8b 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/rc.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/rc.rs @@ -1,4 +1,4 @@ -use std::collections::{HashMap, HashSet}; +use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; use crate::ssa::{ ir::{ @@ -35,13 +35,13 @@ struct Context { // // The type of the array being operated on is recorded. // If an array_set to that array type is encountered, that is also recorded. - inc_rcs: HashMap>, + inc_rcs: HashMap>, } -struct IncRc { - id: InstructionId, - array: ValueId, - possibly_mutated: bool, +pub(crate) struct RcInstruction { + pub(crate) id: InstructionId, + pub(crate) array: ValueId, + pub(crate) possibly_mutated: bool, } /// This function is very simplistic for now. It takes advantage of the fact that dec_rc @@ -80,7 +80,8 @@ impl Context { let typ = function.dfg.type_of_value(*value); // We assume arrays aren't mutated until we find an array_set - let inc_rc = IncRc { id: *instruction, array: *value, possibly_mutated: false }; + let inc_rc = + RcInstruction { id: *instruction, array: *value, possibly_mutated: false }; self.inc_rcs.entry(typ).or_default().push(inc_rc); } } @@ -107,11 +108,11 @@ impl Context { /// is not possibly mutated, then we can remove them both. Returns each such pair. fn find_rcs_to_remove(&mut self, function: &Function) -> HashSet { let last_block = function.find_last_block(); - let mut to_remove = HashSet::new(); + let mut to_remove = HashSet::default(); for instruction in function.dfg[last_block].instructions() { if let Instruction::DecrementRc { value } = &function.dfg[*instruction] { - if let Some(inc_rc) = self.pop_rc_for(*value, function) { + if let Some(inc_rc) = pop_rc_for(*value, function, &mut self.inc_rcs) { if !inc_rc.possibly_mutated { to_remove.insert(inc_rc.id); to_remove.insert(*instruction); @@ -122,16 +123,20 @@ impl Context { to_remove } +} - /// Finds and pops the IncRc for the given array value if possible. - fn pop_rc_for(&mut self, value: ValueId, function: &Function) -> Option { - let typ = function.dfg.type_of_value(value); +/// Finds and pops the IncRc for the given array value if possible. +pub(crate) fn pop_rc_for( + value: ValueId, + function: &Function, + inc_rcs: &mut HashMap>, +) -> Option { + let typ = function.dfg.type_of_value(value); - let rcs = self.inc_rcs.get_mut(&typ)?; - let position = rcs.iter().position(|inc_rc| inc_rc.array == value)?; + let rcs = inc_rcs.get_mut(&typ)?; + let position = rcs.iter().position(|inc_rc| inc_rc.array == value)?; - Some(rcs.remove(position)) - } + Some(rcs.remove(position)) } fn remove_instructions(to_remove: HashSet, function: &mut Function) { diff --git a/noir/noir-repo/compiler/noirc_frontend/src/ast/statement.rs b/noir/noir-repo/compiler/noirc_frontend/src/ast/statement.rs index 299c42b85c6..38cf2cb1d80 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/ast/statement.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/ast/statement.rs @@ -141,13 +141,14 @@ impl StatementKind { pattern: Pattern, r#type: UnresolvedType, expression: Expression, + attributes: Vec, ) -> StatementKind { StatementKind::Let(LetStatement { pattern, r#type, expression, comptime: false, - attributes: vec![], + attributes, }) } @@ -814,6 +815,7 @@ impl ForRange { Pattern::Identifier(array_ident.clone()), UnresolvedTypeData::Unspecified.with_span(Default::default()), array, + vec![], ), span: array_span, }; @@ -858,6 +860,7 @@ impl ForRange { Pattern::Identifier(identifier), UnresolvedTypeData::Unspecified.with_span(Default::default()), Expression::new(loop_element, array_span), + vec![], ), span: array_span, }; diff --git a/noir/noir-repo/compiler/noirc_frontend/src/ast/type_alias.rs b/noir/noir-repo/compiler/noirc_frontend/src/ast/type_alias.rs index 3228765170e..b279d86f19e 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/ast/type_alias.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/ast/type_alias.rs @@ -1,4 +1,4 @@ -use super::{Ident, UnresolvedGenerics, UnresolvedType}; +use super::{Ident, ItemVisibility, UnresolvedGenerics, UnresolvedType}; use iter_extended::vecmap; use noirc_errors::Span; use std::fmt::Display; @@ -9,6 +9,7 @@ pub struct NoirTypeAlias { pub name: Ident, pub generics: UnresolvedGenerics, pub typ: UnresolvedType, + pub visibility: ItemVisibility, pub span: Span, } @@ -17,9 +18,10 @@ impl NoirTypeAlias { name: Ident, generics: UnresolvedGenerics, typ: UnresolvedType, + visibility: ItemVisibility, span: Span, ) -> NoirTypeAlias { - NoirTypeAlias { name, generics, typ, span } + NoirTypeAlias { name, generics, typ, visibility, span } } } diff --git a/noir/noir-repo/compiler/noirc_frontend/src/ast/visitor.rs b/noir/noir-repo/compiler/noirc_frontend/src/ast/visitor.rs index 80442d29398..ed4d17cadd8 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/ast/visitor.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/ast/visitor.rs @@ -32,6 +32,7 @@ pub enum AttributeTarget { Struct, Trait, Function, + Let, } /// Implements the [Visitor pattern](https://en.wikipedia.org/wiki/Visitor_pattern) for Noir's AST. @@ -499,7 +500,7 @@ impl Item { noir_trait_impl.accept(self.span, visitor); } ItemKind::Impl(type_impl) => type_impl.accept(self.span, visitor), - ItemKind::Global(let_statement) => { + ItemKind::Global(let_statement, _visibility) => { if visitor.visit_global(let_statement, self.span) { let_statement.accept(visitor); } @@ -1097,6 +1098,10 @@ impl Statement { impl LetStatement { pub fn accept(&self, visitor: &mut impl Visitor) { + for attribute in &self.attributes { + attribute.accept(AttributeTarget::Let, visitor); + } + if visitor.visit_let_statement(self) { self.accept_children(visitor); } diff --git a/noir/noir-repo/compiler/noirc_frontend/src/debug/mod.rs b/noir/noir-repo/compiler/noirc_frontend/src/debug/mod.rs index ed9265536f9..66de265f869 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/debug/mod.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/debug/mod.rs @@ -146,6 +146,7 @@ impl DebugInstrumenter { ast::Pattern::Identifier(ident("__debug_expr", ret_expr.span)), ast::UnresolvedTypeData::Unspecified.with_span(Default::default()), ret_expr.clone(), + vec![], ), span: ret_expr.span, }; @@ -249,6 +250,7 @@ impl DebugInstrumenter { }), span: let_stmt.expression.span, }, + vec![], ), span: *span, } @@ -274,6 +276,7 @@ impl DebugInstrumenter { ast::Pattern::Identifier(ident("__debug_expr", assign_stmt.expression.span)), ast::UnresolvedTypeData::Unspecified.with_span(Default::default()), assign_stmt.expression.clone(), + vec![], ); let expression_span = assign_stmt.expression.span; let new_assign_stmt = match &assign_stmt.lvalue { diff --git a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/comptime.rs b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/comptime.rs index ca441758322..560be895628 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/comptime.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/comptime.rs @@ -439,11 +439,12 @@ impl<'context> Elaborator<'context> { resolved_trait_generics: Vec::new(), }); } - TopLevelStatementKind::Global(global) => { + TopLevelStatementKind::Global(global, visibility) => { let (global, error) = dc_mod::collect_global( self.interner, self.def_maps.get_mut(&self.crate_id).unwrap(), Documented::new(global, item.doc_comments), + visibility, self.file, self.local_module, self.crate_id, 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 a3b71f3e211..46a22bb232f 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -535,6 +535,8 @@ impl<'context> Elaborator<'context> { } }; + self.mark_struct_as_constructed(r#type.clone()); + let turbofish_span = last_segment.turbofish_span(); let struct_generics = self.resolve_struct_turbofish_generics( @@ -564,6 +566,12 @@ impl<'context> Elaborator<'context> { (expr, Type::Struct(struct_type, generics)) } + pub(super) fn mark_struct_as_constructed(&mut self, struct_type: Shared) { + let struct_type = struct_type.borrow(); + let parent_module_id = struct_type.id.parent_module_id(self.def_maps); + self.interner.usage_tracker.mark_as_used(parent_module_id, &struct_type.name); + } + /// Resolve all the fields of a struct constructor expression. /// Ensures all fields are present, none are repeated, and all /// are part of the struct. @@ -790,7 +798,7 @@ impl<'context> Elaborator<'context> { let parameter = DefinitionKind::Local(None); let typ = self.resolve_inferred_type(typ); arg_types.push(typ.clone()); - (self.elaborate_pattern(pattern, typ.clone(), parameter), typ) + (self.elaborate_pattern(pattern, typ.clone(), parameter, true), typ) }); let return_type = self.resolve_inferred_type(lambda.return_type); 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 f9016b1ca65..c5e457c405b 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/mod.rs @@ -95,7 +95,7 @@ pub struct Elaborator<'context> { pub(crate) interner: &'context mut NodeInterner, - def_maps: &'context mut DefMaps, + pub(crate) def_maps: &'context mut DefMaps, pub(crate) file: FileId, @@ -362,8 +362,12 @@ impl<'context> Elaborator<'context> { if let Kind::Numeric(typ) = &generic.kind { let definition = DefinitionKind::GenericType(generic.type_var.clone()); let ident = Ident::new(generic.name.to_string(), generic.span); - let hir_ident = - self.add_variable_decl_inner(ident, false, false, false, definition); + let hir_ident = self.add_variable_decl( + ident, false, // mutable + false, // allow_shadowing + false, // warn_if_unused + definition, + ); self.interner.push_definition_type(hir_ident.id, *typ.clone()); } } @@ -755,11 +759,16 @@ impl<'context> Elaborator<'context> { type_span, ); + if is_entry_point { + self.mark_parameter_type_as_used(&typ); + } + let pattern = self.elaborate_pattern_and_store_ids( pattern, typ.clone(), DefinitionKind::Local(None), &mut parameter_idents, + true, // warn_if_unused None, ); @@ -832,6 +841,57 @@ impl<'context> Elaborator<'context> { self.current_item = None; } + fn mark_parameter_type_as_used(&mut self, typ: &Type) { + match typ { + Type::Array(_n, typ) => self.mark_parameter_type_as_used(typ), + Type::Slice(typ) => self.mark_parameter_type_as_used(typ), + Type::Tuple(types) => { + for typ in types { + self.mark_parameter_type_as_used(typ); + } + } + Type::Struct(struct_type, generics) => { + self.mark_struct_as_constructed(struct_type.clone()); + for generic in generics { + self.mark_parameter_type_as_used(generic); + } + } + Type::Alias(alias_type, generics) => { + self.mark_parameter_type_as_used(&alias_type.borrow().get_type(generics)); + } + Type::MutableReference(typ) => { + self.mark_parameter_type_as_used(typ); + } + Type::InfixExpr(left, _op, right) => { + self.mark_parameter_type_as_used(left); + self.mark_parameter_type_as_used(right); + } + Type::FieldElement + | Type::Integer(..) + | Type::Bool + | Type::String(_) + | Type::FmtString(_, _) + | Type::Unit + | Type::Quoted(..) + | Type::Constant(..) + | Type::TraitAsType(..) + | Type::TypeVariable(..) + | Type::NamedGeneric(..) + | Type::Function(..) + | Type::Forall(..) + | Type::Error => (), + } + + if let Type::Alias(alias_type, generics) = typ { + self.mark_parameter_type_as_used(&alias_type.borrow().get_type(generics)); + return; + } + + if let Type::Struct(struct_type, _generics) = typ { + self.mark_struct_as_constructed(struct_type.clone()); + } + } + fn run_function_lints(&mut self, func: &FuncMeta, modifiers: &FunctionModifiers) { self.run_lint(|_| lints::inlining_attributes(func, modifiers).map(Into::into)); self.run_lint(|_| lints::missing_pub(func, modifiers).map(Into::into)); @@ -1126,13 +1186,100 @@ impl<'context> Elaborator<'context> { self.file = alias.file_id; self.local_module = alias.module_id; + let name = &alias.type_alias_def.name; + let visibility = alias.type_alias_def.visibility; + let span = alias.type_alias_def.typ.span; + let generics = self.add_generics(&alias.type_alias_def.generics); self.current_item = Some(DependencyId::Alias(alias_id)); let typ = self.resolve_type(alias.type_alias_def.typ); + + if visibility != ItemVisibility::Private { + self.check_aliased_type_is_not_more_private(name, visibility, &typ, span); + } + self.interner.set_type_alias(alias_id, typ, generics); self.generics.clear(); } + fn check_aliased_type_is_not_more_private( + &mut self, + name: &Ident, + visibility: ItemVisibility, + typ: &Type, + span: Span, + ) { + match typ { + Type::Struct(struct_type, generics) => { + let struct_type = struct_type.borrow(); + let struct_module_id = struct_type.id.module_id(); + + // We only check this in types in the same crate. If it's in a different crate + // then it's either accessible (all good) or it's not, in which case a different + // error will happen somewhere else, but no need to error again here. + if struct_module_id.krate == self.crate_id { + // Find the struct in the parent module so we can know its visibility + let parent_module_id = struct_type.id.parent_module_id(self.def_maps); + let parent_module_data = self.get_module(parent_module_id); + let per_ns = parent_module_data.find_name(&struct_type.name); + if let Some((_, aliased_visibility, _)) = per_ns.types { + if aliased_visibility < visibility { + self.push_err(ResolverError::TypeIsMorePrivateThenItem { + typ: struct_type.name.to_string(), + item: name.to_string(), + span, + }); + } + } + } + + for generic in generics { + self.check_aliased_type_is_not_more_private(name, visibility, generic, span); + } + } + Type::Tuple(types) => { + for typ in types { + self.check_aliased_type_is_not_more_private(name, visibility, typ, span); + } + } + Type::Alias(alias_type, generics) => { + self.check_aliased_type_is_not_more_private( + name, + visibility, + &alias_type.borrow().get_type(generics), + span, + ); + } + Type::Function(args, return_type, env, _) => { + for arg in args { + self.check_aliased_type_is_not_more_private(name, visibility, arg, span); + } + self.check_aliased_type_is_not_more_private(name, visibility, return_type, span); + self.check_aliased_type_is_not_more_private(name, visibility, env, span); + } + Type::MutableReference(typ) | Type::Array(_, typ) | Type::Slice(typ) => { + self.check_aliased_type_is_not_more_private(name, visibility, typ, span); + } + Type::InfixExpr(left, _op, right) => { + self.check_aliased_type_is_not_more_private(name, visibility, left, span); + self.check_aliased_type_is_not_more_private(name, visibility, right, span); + } + Type::FieldElement + | Type::Integer(..) + | Type::Bool + | Type::String(..) + | Type::FmtString(..) + | Type::Unit + | Type::Quoted(..) + | Type::TypeVariable(..) + | Type::Forall(..) + | Type::TraitAsType(..) + | Type::Constant(..) + | Type::NamedGeneric(..) + | Type::Error => (), + } + } + fn collect_struct_definitions(&mut self, structs: &BTreeMap) { // This is necessary to avoid cloning the entire struct map // when adding checks after each struct field is resolved. 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 7afa3215566..6ed59a61e4e 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -26,6 +26,7 @@ impl<'context> Elaborator<'context> { pattern: Pattern, expected_type: Type, definition_kind: DefinitionKind, + warn_if_unused: bool, ) -> HirPattern { self.elaborate_pattern_mut( pattern, @@ -33,6 +34,7 @@ impl<'context> Elaborator<'context> { definition_kind, None, &mut Vec::new(), + warn_if_unused, None, ) } @@ -45,6 +47,7 @@ impl<'context> Elaborator<'context> { expected_type: Type, definition_kind: DefinitionKind, created_ids: &mut Vec, + warn_if_unused: bool, global_id: Option, ) -> HirPattern { self.elaborate_pattern_mut( @@ -53,10 +56,12 @@ impl<'context> Elaborator<'context> { definition_kind, None, created_ids, + warn_if_unused, global_id, ) } + #[allow(clippy::too_many_arguments)] fn elaborate_pattern_mut( &mut self, pattern: Pattern, @@ -64,6 +69,7 @@ impl<'context> Elaborator<'context> { definition: DefinitionKind, mutable: Option, new_definitions: &mut Vec, + warn_if_unused: bool, global_id: Option, ) -> HirPattern { match pattern { @@ -80,7 +86,13 @@ impl<'context> Elaborator<'context> { let location = Location::new(name.span(), self.file); HirIdent::non_trait_method(id, location) } else { - self.add_variable_decl(name, mutable.is_some(), true, definition) + self.add_variable_decl( + name, + mutable.is_some(), + true, // allow_shadowing + warn_if_unused, + definition, + ) }; self.interner.push_definition_type(ident.id, expected_type); new_definitions.push(ident.clone()); @@ -97,6 +109,7 @@ impl<'context> Elaborator<'context> { definition, Some(span), new_definitions, + warn_if_unused, global_id, ); let location = Location::new(span, self.file); @@ -128,6 +141,7 @@ impl<'context> Elaborator<'context> { definition.clone(), mutable, new_definitions, + warn_if_unused, global_id, ) }); @@ -151,6 +165,7 @@ impl<'context> Elaborator<'context> { definition, mutable, new_definitions, + warn_if_unused, global_id, ) } @@ -180,7 +195,7 @@ impl<'context> Elaborator<'context> { // shadowing here lets us avoid further errors if we define ERROR_IDENT // multiple times. let name = ERROR_IDENT.into(); - let identifier = this.add_variable_decl(name, false, true, definition.clone()); + let identifier = this.add_variable_decl(name, false, true, true, definition.clone()); HirPattern::Identifier(identifier) }; @@ -263,6 +278,7 @@ impl<'context> Elaborator<'context> { definition.clone(), mutable, new_definitions, + true, // warn_if_unused None, ); @@ -295,16 +311,6 @@ impl<'context> Elaborator<'context> { } pub(super) fn add_variable_decl( - &mut self, - name: Ident, - mutable: bool, - allow_shadowing: bool, - definition: DefinitionKind, - ) -> HirIdent { - self.add_variable_decl_inner(name, mutable, allow_shadowing, true, definition) - } - - pub fn add_variable_decl_inner( &mut self, name: Ident, mutable: bool, 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 2d46c4c6341..55b641ca3d4 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/statements.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/statements.rs @@ -98,12 +98,16 @@ impl<'context> Elaborator<'context> { } } + let warn_if_unused = + !let_stmt.attributes.iter().any(|attr| attr.is_allow_unused_variables()); + let r#type = annotated_type; let pattern = self.elaborate_pattern_and_store_ids( let_stmt.pattern, r#type.clone(), definition, &mut Vec::new(), + warn_if_unused, global_id, ); @@ -215,7 +219,12 @@ impl<'context> Elaborator<'context> { // TODO: For loop variables are currently mutable by default since we haven't // yet implemented syntax for them to be optionally mutable. let kind = DefinitionKind::Local(None); - let identifier = self.add_variable_decl(identifier, false, true, kind); + let identifier = self.add_variable_decl( + identifier, false, // mutable + true, // allow_shadowing + true, // warn_if_unused + kind, + ); // Check that start range and end range have the same types let range_span = start_span.merge(end_span); diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/hir_to_display_ast.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/hir_to_display_ast.rs index 972826f5b7c..4344d19829a 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/hir_to_display_ast.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/hir_to_display_ast.rs @@ -29,7 +29,7 @@ impl HirStatement { let pattern = let_stmt.pattern.to_display_ast(interner); let r#type = interner.id_type(let_stmt.expression).to_display_ast(); let expression = let_stmt.expression.to_display_ast(interner); - StatementKind::new_let(pattern, r#type, expression) + StatementKind::new_let(pattern, r#type, expression, let_stmt.attributes.clone()) } HirStatement::Constrain(constrain) => { let expr = constrain.0.to_display_ast(interner); diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index dc6a2cec975..59fc186173a 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -30,7 +30,6 @@ use crate::{ InterpreterError, Value, }, def_collector::dc_crate::CollectedItems, - def_map::ModuleId, }, hir_def::function::FunctionBody, macros_api::{HirExpression, HirLiteral, Ident, ModuleDefId, NodeInterner, Signedness}, @@ -505,14 +504,7 @@ fn struct_def_module( ) -> IResult { let self_argument = check_one_argument(arguments, location)?; let struct_id = get_struct(self_argument)?; - let struct_module_id = struct_id.module_id(); - - // A struct's module is its own module. To get the module where its defined we need - // to look for its parent. - let module_data = interpreter.elaborator.get_module(struct_module_id); - let parent_local_id = module_data.parent.expect("Expected struct module parent to exist"); - let parent = ModuleId { krate: struct_module_id.krate, local_id: parent_local_id }; - + let parent = struct_id.parent_module_id(interpreter.elaborator.def_maps); Ok(Value::ModuleDefinition(parent)) } @@ -2338,6 +2330,7 @@ fn function_def_set_parameters( parameter_type.clone(), DefinitionKind::Local(None), &mut parameter_idents, + true, // warn_if_unused None, ) }); 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 d365e5807c2..faf72e86fb4 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 @@ -498,7 +498,7 @@ impl DefCollector { let ident = ident.clone(); let error = CompilationError::ResolverError(ResolverError::UnusedItem { ident, - item_type: unused_item.item_type(), + item: *unused_item, }); (error, module.location.file) }) diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index f50a0608fab..162d39e0adb 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -139,15 +139,16 @@ impl<'a> ModCollector<'a> { fn collect_globals( &mut self, context: &mut Context, - globals: Vec>, + globals: Vec<(Documented, ItemVisibility)>, crate_id: CrateId, ) -> Vec<(CompilationError, fm::FileId)> { let mut errors = vec![]; - for global in globals { + for (global, visibility) in globals { let (global, error) = collect_global( &mut context.def_interner, &mut self.def_collector.def_map, global, + visibility, self.file_id, self.module_id, crate_id, @@ -306,6 +307,7 @@ impl<'a> ModCollector<'a> { let doc_comments = type_alias.doc_comments; let type_alias = type_alias.item; let name = type_alias.name.clone(); + let visibility = type_alias.visibility; // And store the TypeId -> TypeAlias mapping somewhere it is reachable let unresolved = UnresolvedTypeAlias { @@ -327,8 +329,19 @@ impl<'a> ModCollector<'a> { context.def_interner.set_doc_comments(ReferenceId::Alias(type_alias_id), doc_comments); // Add the type alias to scope so its path can be looked up later - let result = self.def_collector.def_map.modules[self.module_id.0] - .declare_type_alias(name.clone(), type_alias_id); + let result = self.def_collector.def_map.modules[self.module_id.0].declare_type_alias( + name.clone(), + visibility, + type_alias_id, + ); + + let parent_module_id = ModuleId { krate, local_id: self.module_id }; + context.def_interner.usage_tracker.add_unused_item( + parent_module_id, + name.clone(), + UnusedItem::TypeAlias(type_alias_id), + visibility, + ); if let Err((first_def, second_def)) = result { let err = DefCollectorErrorKind::Duplicate { @@ -503,7 +516,7 @@ impl<'a> ModCollector<'a> { if let Err((first_def, second_def)) = self.def_collector.def_map.modules [trait_id.0.local_id.0] - .declare_global(name.clone(), global_id) + .declare_global(name.clone(), ItemVisibility::Public, global_id) { let error = DefCollectorErrorKind::Duplicate { typ: DuplicateType::TraitAssociatedConst, @@ -526,7 +539,11 @@ impl<'a> ModCollector<'a> { TraitItem::Type { name } => { if let Err((first_def, second_def)) = self.def_collector.def_map.modules [trait_id.0.local_id.0] - .declare_type_alias(name.clone(), TypeAliasId::dummy_id()) + .declare_type_alias( + name.clone(), + ItemVisibility::Public, + TypeAliasId::dummy_id(), + ) { let error = DefCollectorErrorKind::Duplicate { typ: DuplicateType::TraitAssociatedType, @@ -1144,6 +1161,7 @@ pub(crate) fn collect_global( interner: &mut NodeInterner, def_map: &mut CrateDefMap, global: Documented, + visibility: ItemVisibility, file_id: FileId, module_id: LocalModuleId, crate_id: CrateId, @@ -1164,7 +1182,15 @@ pub(crate) fn collect_global( ); // Add the statement to the scope so its path can be looked up later - let result = def_map.modules[module_id.0].declare_global(name, global_id); + let result = def_map.modules[module_id.0].declare_global(name.clone(), visibility, global_id); + + let parent_module_id = ModuleId { krate: crate_id, local_id: module_id }; + interner.usage_tracker.add_unused_item( + parent_module_id, + name, + UnusedItem::Global(global_id), + visibility, + ); let error = result.err().map(|(first_def, second_def)| { let err = diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_map/mod.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_map/mod.rs index d810e95218c..9afe897a167 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_map/mod.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_map/mod.rs @@ -42,12 +42,16 @@ impl ModuleId { pub fn dummy_id() -> ModuleId { ModuleId { krate: CrateId::dummy_id(), local_id: LocalModuleId::dummy_id() } } -} -impl ModuleId { pub fn module(self, def_maps: &DefMaps) -> &ModuleData { &def_maps[&self.krate].modules()[self.local_id.0] } + + /// Returns this module's parent, if there's any. + pub fn parent(self, def_maps: &DefMaps) -> Option { + let module_data = &def_maps[&self.krate].modules()[self.local_id.0]; + module_data.parent.map(|local_id| ModuleId { krate: self.krate, local_id }) + } } pub type DefMaps = BTreeMap; diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_map/module_data.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_map/module_data.rs index 645d8650c7e..149b1977925 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_map/module_data.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_map/module_data.rs @@ -96,8 +96,13 @@ impl ModuleData { self.definitions.remove_definition(name); } - pub fn declare_global(&mut self, name: Ident, id: GlobalId) -> Result<(), (Ident, Ident)> { - self.declare(name, ItemVisibility::Public, id.into(), None) + pub fn declare_global( + &mut self, + name: Ident, + visibility: ItemVisibility, + id: GlobalId, + ) -> Result<(), (Ident, Ident)> { + self.declare(name, visibility, id.into(), None) } pub fn declare_struct( @@ -112,9 +117,10 @@ impl ModuleData { pub fn declare_type_alias( &mut self, name: Ident, + visibility: ItemVisibility, id: TypeAliasId, ) -> Result<(), (Ident, Ident)> { - self.declare(name, ItemVisibility::Public, id.into(), None) + self.declare(name, visibility, id.into(), None) } pub fn declare_trait( 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 1e7f29527e2..f3c61a7fbe2 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 @@ -2,7 +2,10 @@ pub use noirc_errors::Span; use noirc_errors::{CustomDiagnostic as Diagnostic, FileDiagnostic}; use thiserror::Error; -use crate::{ast::Ident, hir::comptime::InterpreterError, parser::ParserError, Type}; +use crate::{ + ast::Ident, hir::comptime::InterpreterError, parser::ParserError, usage_tracker::UnusedItem, + Type, +}; use super::import::PathResolutionError; @@ -20,8 +23,8 @@ pub enum ResolverError { DuplicateDefinition { name: String, first_span: Span, second_span: Span }, #[error("Unused variable")] UnusedVariable { ident: Ident }, - #[error("Unused {item_type}")] - UnusedItem { ident: Ident, item_type: &'static str }, + #[error("Unused {}", item.item_type())] + UnusedItem { ident: Ident, item: UnusedItem }, #[error("Could not find variable in this scope")] VariableNotDeclared { name: String, span: Span }, #[error("path is not an identifier")] @@ -130,6 +133,8 @@ pub enum ResolverError { MutatingComptimeInNonComptimeContext { name: String, span: Span }, #[error("Failed to parse `{statement}` as an expression")] InvalidInternedStatementInExpr { statement: String, span: Span }, + #[error("Type `{typ}` is more private than item `{item}`")] + TypeIsMorePrivateThenItem { typ: String, item: String, span: Span }, } impl ResolverError { @@ -164,14 +169,24 @@ impl<'a> From<&'a ResolverError> for Diagnostic { diagnostic.unnecessary = true; diagnostic } - ResolverError::UnusedItem { ident, item_type } => { + ResolverError::UnusedItem { ident, item} => { let name = &ident.0.contents; + let item_type = item.item_type(); - let mut diagnostic = Diagnostic::simple_warning( - format!("unused {item_type} {name}"), - format!("unused {item_type}"), - ident.span(), - ); + let mut diagnostic = + if let UnusedItem::Struct(..) = item { + Diagnostic::simple_warning( + format!("{item_type} `{name}` is never constructed"), + format!("{item_type} is never constructed"), + ident.span(), + ) + } else { + Diagnostic::simple_warning( + format!("unused {item_type} {name}"), + format!("unused {item_type}"), + ident.span(), + ) + }; diagnostic.unnecessary = true; diagnostic } @@ -529,6 +544,13 @@ impl<'a> From<&'a ResolverError> for Diagnostic { *span, ) }, + ResolverError::TypeIsMorePrivateThenItem { typ, item, span } => { + Diagnostic::simple_warning( + format!("Type `{typ}` is more private than item `{item}`"), + String::new(), + *span, + ) + }, } } } 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 938da0a879f..73a1b1ccb7c 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 @@ -289,7 +289,7 @@ fn resolve_name_in_module( return Err(PathResolutionError::Unresolved(first_segment.clone())); } - usage_tracker.mark_as_used(current_mod_id, first_segment); + usage_tracker.mark_as_referenced(current_mod_id, first_segment); let mut warning: Option = None; for (index, (last_segment, current_segment)) in @@ -356,7 +356,7 @@ fn resolve_name_in_module( return Err(PathResolutionError::Unresolved(current_segment.clone())); } - usage_tracker.mark_as_used(current_mod_id, current_segment); + usage_tracker.mark_as_referenced(current_mod_id, current_segment); current_ns = found_ns; } 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 f7e0a85c79c..97faea4b445 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/lexer/token.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/lexer/token.rs @@ -804,6 +804,7 @@ impl Attribute { } ["varargs"] => Attribute::Secondary(SecondaryAttribute::Varargs), ["use_callers_scope"] => Attribute::Secondary(SecondaryAttribute::UseCallersScope), + ["allow", tag] => Attribute::Secondary(SecondaryAttribute::Allow(tag.to_string())), tokens => { tokens.iter().try_for_each(|token| validate(token))?; Attribute::Secondary(SecondaryAttribute::Custom(CustomAttribute { @@ -925,6 +926,9 @@ pub enum SecondaryAttribute { /// within the scope of the calling function/module rather than this one. /// This affects functions such as `Expression::resolve` or `Quoted::as_type`. UseCallersScope, + + /// Allow chosen warnings to happen so they are silenced. + Allow(String), } impl SecondaryAttribute { @@ -948,6 +952,14 @@ impl SecondaryAttribute { SecondaryAttribute::Abi(_) => Some("abi".to_string()), SecondaryAttribute::Varargs => Some("varargs".to_string()), SecondaryAttribute::UseCallersScope => Some("use_callers_scope".to_string()), + SecondaryAttribute::Allow(_) => Some("allow".to_string()), + } + } + + pub(crate) fn is_allow_unused_variables(&self) -> bool { + match self { + SecondaryAttribute::Allow(string) => string == "unused_variables", + _ => false, } } } @@ -966,6 +978,7 @@ impl fmt::Display for SecondaryAttribute { SecondaryAttribute::Abi(ref k) => write!(f, "#[abi({k})]"), SecondaryAttribute::Varargs => write!(f, "#[varargs]"), SecondaryAttribute::UseCallersScope => write!(f, "#[use_callers_scope]"), + SecondaryAttribute::Allow(ref k) => write!(f, "#[allow(#{k})]"), } } } @@ -1011,7 +1024,9 @@ impl AsRef for SecondaryAttribute { SecondaryAttribute::Deprecated(Some(string)) => string, SecondaryAttribute::Deprecated(None) => "", SecondaryAttribute::Custom(attribute) => &attribute.contents, - SecondaryAttribute::Field(string) | SecondaryAttribute::Abi(string) => string, + SecondaryAttribute::Field(string) + | SecondaryAttribute::Abi(string) + | SecondaryAttribute::Allow(string) => string, SecondaryAttribute::ContractLibraryMethod => "", SecondaryAttribute::Export => "", SecondaryAttribute::Varargs => "", diff --git a/noir/noir-repo/compiler/noirc_frontend/src/node_interner.rs b/noir/noir-repo/compiler/noirc_frontend/src/node_interner.rs index 75178df319d..a95282a1ec9 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/node_interner.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/node_interner.rs @@ -23,6 +23,7 @@ use crate::graph::CrateId; use crate::hir::comptime; use crate::hir::def_collector::dc_crate::CompilationError; use crate::hir::def_collector::dc_crate::{UnresolvedStruct, UnresolvedTrait, UnresolvedTypeAlias}; +use crate::hir::def_map::DefMaps; use crate::hir::def_map::{LocalModuleId, ModuleId}; use crate::hir::type_check::generics::TraitGenerics; use crate::hir_def::traits::NamedType; @@ -489,6 +490,11 @@ impl StructId { pub fn local_module_id(self) -> LocalModuleId { self.0.local_id } + + /// Returns the module where this struct is defined. + pub fn parent_module_id(self, def_maps: &DefMaps) -> ModuleId { + self.module_id().parent(def_maps).expect("Expected struct module parent to exist") + } } #[derive(Debug, Eq, PartialEq, Hash, Copy, Clone, PartialOrd, Ord)] @@ -676,7 +682,7 @@ impl Default for NodeInterner { auto_import_names: HashMap::default(), comptime_scopes: vec![HashMap::default()], trait_impl_associated_types: HashMap::default(), - usage_tracker: UsageTracker::new(), + usage_tracker: UsageTracker::default(), doc_comments: HashMap::default(), } } diff --git a/noir/noir-repo/compiler/noirc_frontend/src/parser/mod.rs b/noir/noir-repo/compiler/noirc_frontend/src/parser/mod.rs index 968af82a8b3..1b6d573560d 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/parser/mod.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/parser/mod.rs @@ -47,7 +47,7 @@ pub enum TopLevelStatementKind { Impl(TypeImpl), TypeAlias(NoirTypeAlias), SubModule(ParsedSubModule), - Global(LetStatement), + Global(LetStatement, ItemVisibility), InnerAttribute(SecondaryAttribute), Error, } @@ -64,7 +64,7 @@ impl TopLevelStatementKind { TopLevelStatementKind::Impl(i) => Some(ItemKind::Impl(i)), TopLevelStatementKind::TypeAlias(t) => Some(ItemKind::TypeAlias(t)), TopLevelStatementKind::SubModule(s) => Some(ItemKind::Submodules(s)), - TopLevelStatementKind::Global(c) => Some(ItemKind::Global(c)), + TopLevelStatementKind::Global(c, visibility) => Some(ItemKind::Global(c, visibility)), TopLevelStatementKind::InnerAttribute(a) => Some(ItemKind::InnerAttribute(a)), TopLevelStatementKind::Error => None, } @@ -249,7 +249,7 @@ pub struct SortedModule { pub trait_impls: Vec, pub impls: Vec, pub type_aliases: Vec>, - pub globals: Vec>, + pub globals: Vec<(Documented, ItemVisibility)>, /// Module declarations like `mod foo;` pub module_decls: Vec>, @@ -271,7 +271,7 @@ impl std::fmt::Display for SortedModule { write!(f, "{import}")?; } - for global_const in &self.globals { + for (global_const, _visibility) in &self.globals { write!(f, "{global_const}")?; } @@ -321,7 +321,9 @@ impl ParsedModule { ItemKind::TypeAlias(type_alias) => { module.push_type_alias(type_alias, item.doc_comments); } - ItemKind::Global(global) => module.push_global(global, item.doc_comments), + ItemKind::Global(global, visibility) => { + module.push_global(global, visibility, item.doc_comments); + } ItemKind::ModuleDecl(mod_name) => { module.push_module_decl(mod_name, item.doc_comments); } @@ -354,7 +356,7 @@ pub enum ItemKind { TraitImpl(NoirTraitImpl), Impl(TypeImpl), TypeAlias(NoirTypeAlias), - Global(LetStatement), + Global(LetStatement, ItemVisibility), ModuleDecl(ModuleDeclaration), Submodules(ParsedSubModule), InnerAttribute(SecondaryAttribute), @@ -438,8 +440,13 @@ impl SortedModule { self.submodules.push(Documented::new(submodule, doc_comments)); } - fn push_global(&mut self, global: LetStatement, doc_comments: Vec) { - self.globals.push(Documented::new(global, doc_comments)); + fn push_global( + &mut self, + global: LetStatement, + visibility: ItemVisibility, + doc_comments: Vec, + ) { + self.globals.push((Documented::new(global, doc_comments), visibility)); } } @@ -526,11 +533,10 @@ impl std::fmt::Display for TopLevelStatementKind { TopLevelStatementKind::Function(fun) => fun.fmt(f), TopLevelStatementKind::Module(m) => m.fmt(f), TopLevelStatementKind::Import(tree, visibility) => { - if visibility == &ItemVisibility::Private { - write!(f, "use {tree}") - } else { - write!(f, "{visibility} use {tree}") + if visibility != &ItemVisibility::Private { + write!(f, "{visibility} ")?; } + write!(f, "use {tree}") } TopLevelStatementKind::Trait(t) => t.fmt(f), TopLevelStatementKind::TraitImpl(i) => i.fmt(f), @@ -538,7 +544,12 @@ impl std::fmt::Display for TopLevelStatementKind { TopLevelStatementKind::Impl(i) => i.fmt(f), TopLevelStatementKind::TypeAlias(t) => t.fmt(f), TopLevelStatementKind::SubModule(s) => s.fmt(f), - TopLevelStatementKind::Global(c) => c.fmt(f), + TopLevelStatementKind::Global(c, visibility) => { + if visibility != &ItemVisibility::Private { + write!(f, "{visibility} ")?; + } + c.fmt(f) + } TopLevelStatementKind::InnerAttribute(a) => write!(f, "#![{}]", a), TopLevelStatementKind::Error => write!(f, "error"), } diff --git a/noir/noir-repo/compiler/noirc_frontend/src/parser/parser.rs b/noir/noir-repo/compiler/noirc_frontend/src/parser/parser.rs index b007653062b..d902f0a3b5a 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/parser/parser.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/parser/parser.rs @@ -220,6 +220,7 @@ fn implementation() -> impl NoirParser { /// global_declaration: 'global' ident global_type_annotation '=' literal fn global_declaration() -> impl NoirParser { let p = attributes::attributes() + .then(item_visibility()) .then(maybe_comp_time()) .then(spanned(keyword(Keyword::Mut)).or_not()) .then_ignore(keyword(Keyword::Global).labelled(ParsingRuleLabel::Global)) @@ -229,7 +230,9 @@ fn global_declaration() -> impl NoirParser { let p = then_commit_ignore(p, just(Token::Assign)); let p = then_commit(p, expression()); p.validate( - |(((((attributes, comptime), mutable), mut pattern), r#type), expression), span, emit| { + |((((((attributes, visibility), comptime), mutable), mut pattern), r#type), expression), + span, + emit| { let global_attributes = attributes::validate_secondary_attributes(attributes, span, emit); @@ -239,10 +242,19 @@ fn global_declaration() -> impl NoirParser { let span = mut_span.merge(pattern.span()); pattern = Pattern::Mutable(Box::new(pattern), span, false); } - LetStatement { pattern, r#type, comptime, expression, attributes: global_attributes } + ( + LetStatement { + pattern, + r#type, + comptime, + expression, + attributes: global_attributes, + }, + visibility, + ) }, ) - .map(TopLevelStatementKind::Global) + .map(|(let_statement, visibility)| TopLevelStatementKind::Global(let_statement, visibility)) } /// submodule: 'mod' ident '{' module '}' @@ -290,14 +302,21 @@ fn contract( fn type_alias_definition() -> impl NoirParser { use self::Keyword::Type; - let p = ignore_then_commit(keyword(Type), ident()); - let p = then_commit(p, function::generics()); - let p = then_commit_ignore(p, just(Token::Assign)); - let p = then_commit(p, parse_type()); - - p.map_with_span(|((name, generics), typ), span| { - TopLevelStatementKind::TypeAlias(NoirTypeAlias { name, generics, typ, span }) - }) + item_visibility() + .then_ignore(keyword(Type)) + .then(ident()) + .then(function::generics()) + .then_ignore(just(Token::Assign)) + .then(parse_type()) + .map_with_span(|(((visibility, name), generics), typ), span| { + TopLevelStatementKind::TypeAlias(NoirTypeAlias { + name, + generics, + typ, + visibility, + span, + }) + }) } fn self_parameter() -> impl NoirParser { @@ -550,8 +569,12 @@ fn declaration<'a, P>(expr_parser: P) -> impl NoirParser + 'a where P: ExprParser + 'a, { - let_statement(expr_parser) - .map(|((pattern, typ), expr)| StatementKind::new_let(pattern, typ, expr)) + attributes().then(let_statement(expr_parser)).validate( + |(attributes, ((pattern, typ), expr)), span, emit| { + let attributes = attributes::validate_secondary_attributes(attributes, span, emit); + StatementKind::new_let(pattern, typ, expr, attributes) + }, + ) } pub fn pattern() -> impl NoirParser { diff --git a/noir/noir-repo/compiler/noirc_frontend/src/tests.rs b/noir/noir-repo/compiler/noirc_frontend/src/tests.rs index cb291902ae2..9b55e689eed 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/tests.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/tests.rs @@ -195,8 +195,10 @@ fn check_trait_implementation_duplicate_method() { x + 2 * y } } - - fn main() {}"; + + fn main() { + let _ = Foo { bar: 1, array: [2, 3] }; // silence Foo never constructed warning + }"; let errors = get_program_errors(src); assert!(!has_parser_error(&errors)); @@ -237,6 +239,7 @@ fn check_trait_wrong_method_return_type() { } fn main() { + let _ = Foo {}; // silence Foo never constructed warning } "; let errors = get_program_errors(src); @@ -279,6 +282,7 @@ fn check_trait_wrong_method_return_type2() { } fn main() { + let _ = Foo { bar: 1, array: [2, 3] }; // silence Foo never constructed warning }"; let errors = get_program_errors(src); assert!(!has_parser_error(&errors)); @@ -401,6 +405,7 @@ fn check_trait_wrong_method_name() { } fn main() { + let _ = Foo { bar: 1, array: [2, 3] }; // silence Foo never constructed warning }"; let compilation_errors = get_program_errors(src); assert!(!has_parser_error(&compilation_errors)); @@ -639,8 +644,10 @@ fn check_impl_struct_not_trait() { Self { bar: x, array: [x,y] } } } - - fn main() {} + + fn main() { + let _ = Default { x: 1, z: 1 }; // silence Default never constructed warning + } "; let errors = get_program_errors(src); assert!(!has_parser_error(&errors)); @@ -719,6 +726,7 @@ fn check_trait_duplicate_implementation() { impl Default for Foo { } fn main() { + let _ = Foo { bar: 1 }; // silence Foo never constructed warning } "; let errors = get_program_errors(src); @@ -757,6 +765,7 @@ fn check_trait_duplicate_implementation_with_alias() { } fn main() { + let _ = MyStruct {}; // silence MyStruct never constructed warning } "; let errors = get_program_errors(src); @@ -1367,7 +1376,9 @@ fn ban_mutable_globals() { // Mutable globals are only allowed in a comptime context let src = r#" mut global FOO: Field = 0; - fn main() {} + fn main() { + let _ = FOO; // silence FOO never used warning + } "#; assert_eq!(get_program_errors(src).len(), 1); } @@ -1549,7 +1560,9 @@ fn struct_numeric_generic_in_function() { inner: u64 } - pub fn bar() { } + pub fn bar() { + let _ = Foo { inner: 1 }; // silence Foo never constructed warning + } "#; let errors = get_program_errors(src); assert_eq!(errors.len(), 1); @@ -1724,7 +1737,7 @@ fn numeric_generic_used_in_nested_type_fails() { a: Field, b: Bar, } - struct Bar { + pub struct Bar { inner: N } "#; @@ -1879,6 +1892,10 @@ fn constant_used_with_numeric_generic() { [self.value] } } + + fn main() { + let _ = ValueNote { value: 1 }; // silence ValueNote never constructed warning + } "#; assert_no_errors(src); } @@ -1985,6 +2002,10 @@ fn numeric_generics_value_kind_mismatch_u32_u64() { self.len += 1; } } + + fn main() { + let _ = BoundedVec { storage: [1], len: 1 }; // silence never constructed warning + } "#; let errors = get_program_errors(src); assert_eq!(errors.len(), 1); @@ -2060,6 +2081,10 @@ fn impl_stricter_than_trait_no_trait_method_constraints() { process_array(serialize_thing(self)) } } + + fn main() { + let _ = MyType { a: 1, b: 1 }; // silence MyType never constructed warning + } "#; let errors = get_program_errors(src); @@ -2150,6 +2175,10 @@ fn impl_stricter_than_trait_different_object_generics() { fn tuple_bad() where (Option, Option): MyTrait { } } + + fn main() { + let _ = OtherOption { inner: Option { inner: 1 } }; // silence unused warnings + } "#; let errors = get_program_errors(src); @@ -2211,6 +2240,10 @@ fn impl_stricter_than_trait_different_trait() { // types are the same. fn bar() where Option: OtherDefault {} } + + fn main() { + let _ = Option { inner: 1 }; // silence Option never constructed warning + } "#; let errors = get_program_errors(src); @@ -2248,6 +2281,10 @@ fn trait_impl_where_clause_stricter_pass() { fn bad_foo() where A: OtherTrait { } } + + fn main() { + let _ = Option { inner: 1 }; // silence Option never constructed warning + } "#; let errors = get_program_errors(src); @@ -2333,6 +2370,10 @@ fn impl_not_found_for_inner_impl() { process_array(serialize_thing(self)) } } + + fn main() { + let _ = MyType { a: 1, b: 1 }; // silence MyType never constructed warning + } "#; let errors = get_program_errors(src); @@ -2639,7 +2680,9 @@ fn incorrect_generic_count_on_struct_impl() { let src = r#" struct Foo {} impl Foo {} - fn main() {} + fn main() { + let _ = Foo {}; // silence Foo never constructed warning + } "#; let errors = get_program_errors(src); @@ -2659,9 +2702,11 @@ fn incorrect_generic_count_on_struct_impl() { #[test] fn incorrect_generic_count_on_type_alias() { let src = r#" - struct Foo {} - type Bar = Foo; - fn main() {} + pub struct Foo {} + pub type Bar = Foo; + fn main() { + let _ = Foo {}; // silence Foo never constructed warning + } "#; let errors = get_program_errors(src); @@ -2693,7 +2738,9 @@ fn uses_self_type_for_struct_function_call() { } } - fn main() {} + fn main() { + let _ = S {}; // silence S never constructed warning + } "#; assert_no_errors(src); } @@ -2743,7 +2790,9 @@ fn uses_self_type_in_trait_where_clause() { } - fn main() {} + fn main() { + let _ = Bar {}; // silence Bar never constructed warning + } "#; let errors = get_program_errors(src); @@ -2903,6 +2952,8 @@ fn as_trait_path_syntax_resolves_outside_impl() { // AsTraitPath syntax is a bit silly when associated types // are explicitly specified let _: i64 = 1 as >::Assoc; + + let _ = Bar {}; // silence Bar never constructed warning } "#; @@ -2934,6 +2985,8 @@ fn as_trait_path_syntax_no_impl() { fn main() { let _: i64 = 1 as >::Assoc; + + let _ = Bar {}; // silence Bar never constructed warning } "#; @@ -3047,7 +3100,9 @@ fn errors_once_on_unused_import_that_is_not_accessible() { struct Foo {} } use moo::Foo; - fn main() {} + fn main() { + let _ = Foo {}; + } "#; let errors = get_program_errors(src); @@ -3088,3 +3143,60 @@ fn trait_unconstrained_methods_typechecked_correctly() { println!("{errors:?}"); assert_eq!(errors.len(), 0); } + +#[test] +fn errors_if_type_alias_aliases_more_private_type() { + let src = r#" + struct Foo {} + pub type Bar = Foo; + + pub fn no_unused_warnings(_b: Bar) { + let _ = Foo {}; + } + + fn main() {} + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::TypeIsMorePrivateThenItem { + typ, item, .. + }) = &errors[0].0 + else { + panic!("Expected an unused item error"); + }; + + assert_eq!(typ, "Foo"); + assert_eq!(item, "Bar"); +} + +#[test] +fn errors_if_type_alias_aliases_more_private_type_in_generic() { + let src = r#" + pub struct Generic { value: T } + + struct Foo {} + pub type Bar = Generic; + + pub fn no_unused_warnings(_b: Bar) { + let _ = Foo {}; + let _ = Generic { value: 1 }; + } + + fn main() {} + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::TypeIsMorePrivateThenItem { + typ, item, .. + }) = &errors[0].0 + else { + panic!("Expected an unused item error"); + }; + + assert_eq!(typ, "Foo"); + assert_eq!(item, "Bar"); +} diff --git a/noir/noir-repo/compiler/noirc_frontend/src/tests/unused_items.rs b/noir/noir-repo/compiler/noirc_frontend/src/tests/unused_items.rs index b49414d8b03..a4d379b6358 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/tests/unused_items.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/tests/unused_items.rs @@ -1,4 +1,7 @@ -use crate::hir::{def_collector::dc_crate::CompilationError, resolution::errors::ResolverError}; +use crate::{ + hir::{def_collector::dc_crate::CompilationError, resolution::errors::ResolverError}, + tests::assert_no_errors, +}; use super::get_program_errors; @@ -28,14 +31,13 @@ fn errors_on_unused_private_import() { let errors = get_program_errors(src); assert_eq!(errors.len(), 1); - let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item_type }) = - &errors[0].0 + let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item }) = &errors[0].0 else { panic!("Expected an unused item error"); }; assert_eq!(ident.to_string(), "bar"); - assert_eq!(*item_type, "import"); + assert_eq!(item.item_type(), "import"); } #[test] @@ -64,14 +66,13 @@ fn errors_on_unused_pub_crate_import() { let errors = get_program_errors(src); assert_eq!(errors.len(), 1); - let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item_type }) = - &errors[0].0 + let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item }) = &errors[0].0 else { panic!("Expected an unused item error"); }; assert_eq!(ident.to_string(), "bar"); - assert_eq!(*item_type, "import"); + assert_eq!(item.item_type(), "import"); } #[test] @@ -96,14 +97,13 @@ fn errors_on_unused_function() { let errors = get_program_errors(src); assert_eq!(errors.len(), 1); - let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item_type }) = - &errors[0].0 + let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item }) = &errors[0].0 else { panic!("Expected an unused item error"); }; assert_eq!(ident.to_string(), "foo"); - assert_eq!(*item_type, "function"); + assert_eq!(item.item_type(), "function"); } #[test] @@ -120,14 +120,13 @@ fn errors_on_unused_struct() { let errors = get_program_errors(src); assert_eq!(errors.len(), 1); - let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item_type }) = - &errors[0].0 + let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item }) = &errors[0].0 else { panic!("Expected an unused item error"); }; assert_eq!(ident.to_string(), "Foo"); - assert_eq!(*item_type, "struct"); + assert_eq!(item.item_type(), "struct"); } #[test] @@ -148,12 +147,118 @@ fn errors_on_unused_trait() { let errors = get_program_errors(src); assert_eq!(errors.len(), 1); - let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item_type }) = - &errors[0].0 + let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item }) = &errors[0].0 else { panic!("Expected an unused item error"); }; assert_eq!(ident.to_string(), "Foo"); - assert_eq!(*item_type, "trait"); + assert_eq!(item.item_type(), "trait"); +} + +#[test] +fn silences_unused_variable_warning() { + let src = r#" + fn main() { + #[allow(unused_variables)] + let x = 1; + } + "#; + assert_no_errors(src); +} + +#[test] +fn errors_on_unused_type_alias() { + let src = r#" + type Foo = Field; + type Bar = Field; + pub fn bar(_: Bar) {} + fn main() {} + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item }) = &errors[0].0 + else { + panic!("Expected an unused item error"); + }; + + assert_eq!(ident.to_string(), "Foo"); + assert_eq!(item.item_type(), "type alias"); +} + +#[test] +fn errors_if_type_alias_aliases_more_private_type() { + let src = r#" + struct Foo {} + pub type Bar = Foo; + pub fn no_unused_warnings(_b: Bar) { + let _ = Foo {}; + } + fn main() {} + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::TypeIsMorePrivateThenItem { + typ, item, .. + }) = &errors[0].0 + else { + panic!("Expected an unused item error"); + }; + + assert_eq!(typ, "Foo"); + assert_eq!(item, "Bar"); +} + +#[test] +fn errors_if_type_alias_aliases_more_private_type_in_generic() { + let src = r#" + pub struct Generic { value: T } + struct Foo {} + pub type Bar = Generic; + pub fn no_unused_warnings(_b: Bar) { + let _ = Foo {}; + let _ = Generic { value: 1 }; + } + fn main() {} + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::TypeIsMorePrivateThenItem { + typ, item, .. + }) = &errors[0].0 + else { + panic!("Expected an unused item error"); + }; + + assert_eq!(typ, "Foo"); + assert_eq!(item, "Bar"); +} + +#[test] +fn warns_on_unused_global() { + let src = r#" + global foo = 1; + global bar = 1; + + fn main() { + let _ = bar; + } + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item }) = &errors[0].0 + else { + panic!("Expected an unused item warning"); + }; + + assert_eq!(ident.to_string(), "foo"); + assert_eq!(item.item_type(), "global"); } diff --git a/noir/noir-repo/compiler/noirc_frontend/src/usage_tracker.rs b/noir/noir-repo/compiler/noirc_frontend/src/usage_tracker.rs index b6f41dc72f2..275ca1f964b 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/usage_tracker.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/usage_tracker.rs @@ -4,15 +4,17 @@ use crate::{ ast::{Ident, ItemVisibility}, hir::def_map::ModuleId, macros_api::StructId, - node_interner::{FuncId, TraitId}, + node_interner::{FuncId, GlobalId, TraitId, TypeAliasId}, }; -#[derive(Debug)] +#[derive(Debug, Copy, Clone, PartialEq, Eq)] pub enum UnusedItem { Import, Function(FuncId), Struct(StructId), Trait(TraitId), + TypeAlias(TypeAliasId), + Global(GlobalId), } impl UnusedItem { @@ -22,20 +24,18 @@ impl UnusedItem { UnusedItem::Function(_) => "function", UnusedItem::Struct(_) => "struct", UnusedItem::Trait(_) => "trait", + UnusedItem::TypeAlias(_) => "type alias", + UnusedItem::Global(_) => "global", } } } -#[derive(Debug)] +#[derive(Debug, Default)] pub struct UsageTracker { unused_items: HashMap>, } impl UsageTracker { - pub(crate) fn new() -> Self { - Self { unused_items: HashMap::new() } - } - pub(crate) fn add_unused_item( &mut self, module_id: ModuleId, @@ -49,8 +49,29 @@ impl UsageTracker { } } + /// Marks an item as being referenced. This doesn't always makes the item as used. For example + /// if a struct is referenced it will still be considered unused unless it's constructed somewhere. + pub(crate) fn mark_as_referenced(&mut self, current_mod_id: ModuleId, name: &Ident) { + let Some(items) = self.unused_items.get_mut(¤t_mod_id) else { + return; + }; + + let Some(unused_item) = items.get(name) else { + return; + }; + + if let UnusedItem::Struct(_) = unused_item { + return; + } + + items.remove(name); + } + + /// Marks an item as being used. pub(crate) fn mark_as_used(&mut self, current_mod_id: ModuleId, name: &Ident) { - self.unused_items.entry(current_mod_id).or_default().remove(name); + if let Some(items) = self.unused_items.get_mut(¤t_mod_id) { + items.remove(name); + }; } pub(crate) fn unused_items(&self) -> &HashMap> { diff --git a/noir/noir-repo/docs/docs/noir/concepts/data_types/index.md b/noir/noir-repo/docs/docs/noir/concepts/data_types/index.md index 3eadb2dc8a4..11f51e2b65a 100644 --- a/noir/noir-repo/docs/docs/noir/concepts/data_types/index.md +++ b/noir/noir-repo/docs/docs/noir/concepts/data_types/index.md @@ -105,6 +105,14 @@ type Bad2 = Bad1; // ^^^^^^^^^^^ 'Bad2' recursively depends on itself: Bad2 -> Bad1 -> Bad2 ``` +By default, like functions, type aliases are private to the module the exist in. You can use `pub` +to make the type alias public or `pub(crate)` to make it public to just its crate: + +```rust +// This type alias is now public +pub type Id = u8; +``` + ## Wildcard Type Noir can usually infer the type of the variable from the context, so specifying the type of a variable is only required when it cannot be inferred. However, specifying a complex type can be tedious, especially when it has multiple generic arguments. Often some of the generic types can be inferred from the context, and Noir only needs a hint to properly infer the other types. We can partially specify a variable's type by using `_` as a marker, indicating where we still want the compiler to infer the type. diff --git a/noir/noir-repo/docs/docs/noir/concepts/globals.md b/noir/noir-repo/docs/docs/noir/concepts/globals.md index 97a92a86e72..1145c55dfc7 100644 --- a/noir/noir-repo/docs/docs/noir/concepts/globals.md +++ b/noir/noir-repo/docs/docs/noir/concepts/globals.md @@ -70,3 +70,13 @@ fn foo() -> [Field; 100] { ... } This is usually fine since Noir will generally optimize any function call that does not refer to a program input into a constant. It should be kept in mind however, if the called function performs side-effects like `println`, as these will still occur on each use. + +### Visibility + +By default, like functions, globals are private to the module the exist in. You can use `pub` +to make the global public or `pub(crate)` to make it public to just its crate: + +```rust +// This global is now public +pub global N = 5; +``` \ No newline at end of file diff --git a/noir/noir-repo/noir_stdlib/src/ec/consts/te.nr b/noir/noir-repo/noir_stdlib/src/ec/consts/te.nr index 349e518cdb2..e82302eadee 100644 --- a/noir/noir-repo/noir_stdlib/src/ec/consts/te.nr +++ b/noir/noir-repo/noir_stdlib/src/ec/consts/te.nr @@ -8,6 +8,7 @@ pub struct BabyJubjub { } #[field(bn254)] +#[deprecated = "It's recommmended to use the external noir-edwards library (https://github.com/noir-lang/noir-edwards)"] pub fn baby_jubjub() -> BabyJubjub { BabyJubjub { // Baby Jubjub (ERC-2494) parameters in affine representation diff --git a/noir/noir-repo/noir_stdlib/src/ec/tecurve.nr b/noir/noir-repo/noir_stdlib/src/ec/tecurve.nr index b306873806d..f9cdf83aab9 100644 --- a/noir/noir-repo/noir_stdlib/src/ec/tecurve.nr +++ b/noir/noir-repo/noir_stdlib/src/ec/tecurve.nr @@ -27,6 +27,7 @@ mod affine { impl Point { // Point constructor + #[deprecated = "It's recommmended to use the external noir-edwards library (https://github.com/noir-lang/noir-edwards)"] pub fn new(x: Field, y: Field) -> Self { Self { x, y } } diff --git a/noir/noir-repo/noir_stdlib/src/field/bn254.nr b/noir/noir-repo/noir_stdlib/src/field/bn254.nr index 0aa5ca0717b..8ff62062d5c 100644 --- a/noir/noir-repo/noir_stdlib/src/field/bn254.nr +++ b/noir/noir-repo/noir_stdlib/src/field/bn254.nr @@ -4,7 +4,7 @@ use crate::runtime::is_unconstrained; global PLO: Field = 53438638232309528389504892708671455233; global PHI: Field = 64323764613183177041862057485226039389; -global TWO_POW_128: Field = 0x100000000000000000000000000000000; +pub(crate) global TWO_POW_128: Field = 0x100000000000000000000000000000000; // Decomposes a single field into two 16 byte fields. fn compute_decomposition(x: Field) -> (Field, Field) { diff --git a/noir/noir-repo/noir_stdlib/src/hash/keccak.nr b/noir/noir-repo/noir_stdlib/src/hash/keccak.nr index 5346ff9fae6..37eb4dfe8a6 100644 --- a/noir/noir-repo/noir_stdlib/src/hash/keccak.nr +++ b/noir/noir-repo/noir_stdlib/src/hash/keccak.nr @@ -99,7 +99,7 @@ pub(crate) fn keccak256(input: [u8; N], message_size: u32) -> [u8; 3 } mod tests { - use crate::hash::keccak::keccak256; + use super::keccak256; #[test] fn smoke_test() { diff --git a/noir/noir-repo/noir_stdlib/src/hash/mimc.nr b/noir/noir-repo/noir_stdlib/src/hash/mimc.nr index 8c43536d18b..1bcc9e3fcc7 100644 --- a/noir/noir-repo/noir_stdlib/src/hash/mimc.nr +++ b/noir/noir-repo/noir_stdlib/src/hash/mimc.nr @@ -6,6 +6,7 @@ use crate::default::Default; // You must use constants generated for the native field // Rounds number should be ~ log(p)/log(exp) // For 254 bit primes, exponent 7 and 91 rounds seems to be recommended +#[deprecated = "It's recommmended to use the external MiMC library (https://github.com/noir-lang/mimc)"] fn mimc(x: Field, k: Field, constants: [Field; N], exp: Field) -> Field { //round 0 let mut t = x + k; @@ -116,6 +117,7 @@ global MIMC_BN254_CONSTANTS: [Field; MIMC_BN254_ROUNDS] = [ //mimc implementation with hardcoded parameters for BN254 curve. #[field(bn254)] +#[deprecated = "It's recommmended to use the external MiMC library (https://github.com/noir-lang/mimc)"] pub fn mimc_bn254(array: [Field; N]) -> Field { let exponent = 7; let mut r = 0; diff --git a/noir/noir-repo/noir_stdlib/src/hash/mod.nr b/noir/noir-repo/noir_stdlib/src/hash/mod.nr index 93bce3c20e1..c69c3f9c49e 100644 --- a/noir/noir-repo/noir_stdlib/src/hash/mod.nr +++ b/noir/noir-repo/noir_stdlib/src/hash/mod.nr @@ -36,7 +36,7 @@ pub fn pedersen_hash_with_separator(input: [Field; N], separator: u3 __pedersen_hash_with_separator(input, separator) } -fn pedersen_commitment_with_separator(input: [Field; N], separator: u32) -> EmbeddedCurvePoint { +pub fn pedersen_commitment_with_separator(input: [Field; N], separator: u32) -> EmbeddedCurvePoint { let value = __pedersen_commitment_with_separator(input, separator); if (value[0] == 0) & (value[1] == 0) { EmbeddedCurvePoint { x: 0, y: 0, is_infinite: true } @@ -88,7 +88,7 @@ fn __pedersen_hash_with_separator(input: [Field; N], separator: u32) fn __pedersen_commitment_with_separator(input: [Field; N], separator: u32) -> [Field; 2] {} #[field(bn254)] -fn derive_generators(domain_separator_bytes: [u8; M], starting_index: u32) -> [EmbeddedCurvePoint; N] { +pub fn derive_generators(domain_separator_bytes: [u8; M], starting_index: u32) -> [EmbeddedCurvePoint; N] { crate::assert_constant(domain_separator_bytes); // TODO(https://github.com/noir-lang/noir/issues/5672): Add back assert_constant on starting_index __derive_generators(domain_separator_bytes, starting_index) @@ -102,10 +102,10 @@ fn __derive_generators( ) -> [EmbeddedCurvePoint; N] {} #[field(bn254)] - // Same as from_field but: - // does not assert the limbs are 128 bits - // does not assert the decomposition does not overflow the EmbeddedCurveScalar - fn from_field_unsafe(scalar: Field) -> EmbeddedCurveScalar { +// Same as from_field but: +// does not assert the limbs are 128 bits +// does not assert the decomposition does not overflow the EmbeddedCurveScalar +fn from_field_unsafe(scalar: Field) -> EmbeddedCurveScalar { let (xlo, xhi) = unsafe { crate::field::bn254::decompose_hint(scalar) }; @@ -419,4 +419,3 @@ fn assert_pedersen() { } ); } - diff --git a/noir/noir-repo/noir_stdlib/src/hash/sha256.nr b/noir/noir-repo/noir_stdlib/src/hash/sha256.nr index 27eb673e035..413c26d6f6b 100644 --- a/noir/noir-repo/noir_stdlib/src/hash/sha256.nr +++ b/noir/noir-repo/noir_stdlib/src/hash/sha256.nr @@ -8,7 +8,7 @@ use crate::runtime::is_unconstrained; pub fn sha256(input: [u8; N]) -> [u8; 32] // docs:end:sha256 { - crate::sha256::digest(input) + digest(input) } #[foreign(sha256_compression)] @@ -35,39 +35,41 @@ fn msg_u8_to_u32(msg: [u8; 64]) -> [u32; 16] { msg32 } -unconstrained fn build_msg_block_iter(msg: [u8; N], message_size: u64, msg_start: u32) -> ([u8; 64], u64) { +unconstrained fn build_msg_block_iter(msg: [u8; N], message_size: u32, msg_start: u32) -> ([u8; 64], u32) { let mut msg_block: [u8; BLOCK_SIZE] = [0; BLOCK_SIZE]; - let mut msg_byte_ptr: u64 = 0; // Message byte pointer - let mut msg_end = msg_start + BLOCK_SIZE; - if msg_end > N { - msg_end = N; - } - for k in msg_start..msg_end { - if k as u64 < message_size { - msg_block[msg_byte_ptr] = msg[k]; - msg_byte_ptr = msg_byte_ptr + 1; + // We insert `BLOCK_SIZE` bytes (or up to the end of the message) + let block_input = if msg_start + BLOCK_SIZE > message_size { + if message_size < msg_start { + // This function is sometimes called with `msg_start` past the end of the message. + // In this case we return an empty block and zero pointer to signal that the result should be ignored. + 0 + } else { + message_size - msg_start } + } else { + BLOCK_SIZE + }; + for k in 0..block_input { + msg_block[k] = msg[msg_start + k]; } - (msg_block, msg_byte_ptr) + (msg_block, block_input) } // Verify the block we are compressing was appropriately constructed fn verify_msg_block( msg: [u8; N], - message_size: u64, + message_size: u32, msg_block: [u8; 64], msg_start: u32 -) -> u64 { - let mut msg_byte_ptr: u64 = 0; // Message byte pointer +) -> u32 { + let mut msg_byte_ptr: u32 = 0; // Message byte pointer let mut msg_end = msg_start + BLOCK_SIZE; - let mut extra_bytes = 0; if msg_end > N { msg_end = N; - extra_bytes = msg_end - N; } for k in msg_start..msg_end { - if k as u64 < message_size { + if k < message_size { assert_eq(msg_block[msg_byte_ptr], msg[k]); msg_byte_ptr = msg_byte_ptr + 1; } @@ -81,6 +83,7 @@ global ZERO = 0; // Variable size SHA-256 hash pub fn sha256_var(msg: [u8; N], message_size: u64) -> [u8; 32] { + let message_size = message_size as u32; let num_blocks = N / BLOCK_SIZE; let mut msg_block: [u8; BLOCK_SIZE] = [0; BLOCK_SIZE]; let mut h: [u32; 8] = [1779033703, 3144134277, 1013904242, 2773480762, 1359893119, 2600822924, 528734635, 1541459225]; // Intermediate hash, starting with the canonical initial value @@ -91,23 +94,23 @@ pub fn sha256_var(msg: [u8; N], message_size: u64) -> [u8; 32] { let (new_msg_block, new_msg_byte_ptr) = unsafe { build_msg_block_iter(msg, message_size, msg_start) }; - if msg_start as u64 < message_size { + if msg_start < message_size { msg_block = new_msg_block; } if !is_unconstrained() { // Verify the block we are compressing was appropriately constructed let new_msg_byte_ptr = verify_msg_block(msg, message_size, msg_block, msg_start); - if msg_start as u64 < message_size { + if msg_start < message_size { msg_byte_ptr = new_msg_byte_ptr; } - } else if msg_start as u64 < message_size { + } else if msg_start < message_size { msg_byte_ptr = new_msg_byte_ptr; } // If the block is filled, compress it. // An un-filled block is handled after this loop. - if msg_byte_ptr == 64 { + if msg_byte_ptr == BLOCK_SIZE { h = sha256_compression(msg_u8_to_u32(msg_block), h); } } @@ -122,21 +125,21 @@ pub fn sha256_var(msg: [u8; N], message_size: u64) -> [u8; 32] { build_msg_block_iter(msg, message_size, msg_start) }; - if msg_start as u64 < message_size { + if msg_start < message_size { msg_block = new_msg_block; } if !is_unconstrained() { let new_msg_byte_ptr = verify_msg_block(msg, message_size, msg_block, msg_start); - if msg_start as u64 < message_size { + if msg_start < message_size { msg_byte_ptr = new_msg_byte_ptr; } - } else if msg_start as u64 < message_size { + } else if msg_start < message_size { msg_byte_ptr = new_msg_byte_ptr; } } - if msg_byte_ptr == BLOCK_SIZE as u64 { + if msg_byte_ptr == BLOCK_SIZE { msg_byte_ptr = 0; } @@ -160,14 +163,14 @@ pub fn sha256_var(msg: [u8; N], message_size: u64) -> [u8; 32] { } if !crate::runtime::is_unconstrained() { - for i in 0..64 { + for i in 0..BLOCK_SIZE { assert_eq(msg_block[i], last_block[i]); } // If i >= 57, there aren't enough bits in the current message block to accomplish this, so // the 1 and 0s fill up the current block, which we then compress accordingly. // Not enough bits (64) to store length. Fill up with zeros. - for _i in 57..64 { + for _i in 57..BLOCK_SIZE { if msg_byte_ptr <= 63 & msg_byte_ptr >= 57 { assert_eq(msg_block[msg_byte_ptr], zero); msg_byte_ptr += 1; @@ -204,42 +207,40 @@ pub fn sha256_var(msg: [u8; N], message_size: u64) -> [u8; 32] { hash_final_block(msg_block, h) } -unconstrained fn pad_msg_block(mut msg_block: [u8; 64], mut msg_byte_ptr: u64) -> ([u8; 64], u64) { +unconstrained fn pad_msg_block( + mut msg_block: [u8; 64], + mut msg_byte_ptr: u32 +) -> ([u8; BLOCK_SIZE], u32) { // If i >= 57, there aren't enough bits in the current message block to accomplish this, so // the 1 and 0s fill up the current block, which we then compress accordingly. if msg_byte_ptr >= 57 { // Not enough bits (64) to store length. Fill up with zeros. - if msg_byte_ptr < 64 { - for _ in 57..64 { - if msg_byte_ptr <= 63 { - msg_block[msg_byte_ptr] = 0; - msg_byte_ptr += 1; - } - } + for i in msg_byte_ptr..BLOCK_SIZE { + msg_block[i] = 0; } + (msg_block, BLOCK_SIZE) + } else { + (msg_block, msg_byte_ptr) } - (msg_block, msg_byte_ptr) } -unconstrained fn attach_len_to_msg_block(mut msg_block: [u8; 64], mut msg_byte_ptr: u64, message_size: u64) -> [u8; 64] { +unconstrained fn attach_len_to_msg_block(mut msg_block: [u8; BLOCK_SIZE], msg_byte_ptr: u32, message_size: u32) -> [u8; BLOCK_SIZE] { + // We assume that `msg_byte_ptr` is less than 57 because if not then it is reset to zero before calling this function. + // In any case, fill blocks up with zeros until the last 64 (i.e. until msg_byte_ptr = 56). + + for i in msg_byte_ptr..56 { + msg_block[i] = 0; + } + let len = 8 * message_size; let len_bytes: [u8; 8] = (len as Field).to_be_bytes(); - for _i in 0..64 { - // In any case, fill blocks up with zeros until the last 64 (i.e. until msg_byte_ptr = 56). - if msg_byte_ptr < 56 { - msg_block[msg_byte_ptr] = 0; - msg_byte_ptr = msg_byte_ptr + 1; - } else if msg_byte_ptr < 64 { - for j in 0..8 { - msg_block[msg_byte_ptr + j] = len_bytes[j]; - } - msg_byte_ptr += 8; - } + for i in 0..8 { + msg_block[56 + i] = len_bytes[i]; } msg_block } -fn hash_final_block(msg_block: [u8; 64], mut state: [u32; 8]) -> [u8; 32] { +fn hash_final_block(msg_block: [u8; BLOCK_SIZE], mut state: [u32; 8]) -> [u8; 32] { let mut out_h: [u8; 32] = [0; 32]; // Digest as sequence of bytes // Hash final padded block @@ -255,3 +256,83 @@ fn hash_final_block(msg_block: [u8; 64], mut state: [u32; 8]) -> [u8; 32] { out_h } + +mod tests { + use super::sha256_var; + + #[test] + fn smoke_test() { + let input = [0xbd]; + let result = [ + 0x68, 0x32, 0x57, 0x20, 0xaa, 0xbd, 0x7c, 0x82, 0xf3, 0x0f, 0x55, 0x4b, 0x31, 0x3d, 0x05, 0x70, 0xc9, 0x5a, 0xcc, 0xbb, 0x7d, 0xc4, 0xb5, 0xaa, 0xe1, 0x12, 0x04, 0xc0, 0x8f, 0xfe, 0x73, 0x2b + ]; + assert_eq(sha256_var(input, input.len() as u64), result); + } + + #[test] + fn msg_just_over_block() { + let input = [ + 102, 114, 111, 109, 58, 114, 117, 110, 110, 105, 101, 114, 46, 108, 101, 97, 103, 117, 101, 115, 46, 48, 106, 64, 105, 99, 108, 111, 117, 100, 46, 99, 111, 109, 13, 10, 99, 111, 110, 116, 101, 110, 116, 45, 116, 121, 112, 101, 58, 116, 101, 120, 116, 47, 112, 108, 97, 105, 110, 59, 32, 99, 104, 97, 114, 115, 101, 116 + ]; + let result = [ + 91, 122, 146, 93, 52, 109, 133, 148, 171, 61, 156, 70, 189, 238, 153, 7, 222, 184, 94, 24, 65, 114, 192, 244, 207, 199, 87, 232, 192, 224, 171, 207 + ]; + assert_eq(sha256_var(input, input.len() as u64), result); + } + + #[test] + fn msg_multiple_over_block() { + let input = [ + 102, 114, 111, 109, 58, 114, 117, 110, 110, 105, 101, 114, 46, 108, 101, 97, 103, 117, 101, 115, 46, 48, 106, 64, 105, 99, 108, 111, 117, 100, 46, 99, 111, 109, 13, 10, 99, 111, 110, 116, 101, 110, 116, 45, 116, 121, 112, 101, 58, 116, 101, 120, 116, 47, 112, 108, 97, 105, 110, 59, 32, 99, 104, 97, 114, 115, 101, 116, 61, 117, 115, 45, 97, 115, 99, 105, 105, 13, 10, 109, 105, 109, 101, 45, 118, 101, 114, 115, 105, 111, 110, 58, 49, 46, 48, 32, 40, 77, 97, 99, 32, 79, 83, 32, 88, 32, 77, 97, 105, 108, 32, 49, 54, 46, 48, 32, 92, 40, 51, 55, 51, 49, 46, 53, 48, 48, 46, 50, 51, 49, 92, 41, 41, 13, 10, 115, 117, 98, 106, 101, 99, 116, 58, 72, 101, 108, 108, 111, 13, 10, 109, 101, 115, 115, 97, 103, 101, 45, 105, 100, 58, 60, 56, 70, 56, 49, 57, 68, 51, 50, 45, 66, 54, 65, 67, 45, 52, 56, 57, 68, 45, 57, 55, 55, 70, 45, 52, 51, 56, 66, 66, 67, 52, 67, 65, 66, 50, 55, 64, 109, 101, 46, 99, 111, 109, 62, 13, 10, 100, 97, 116, 101, 58, 83, 97, 116, 44, 32, 50, 54, 32, 65, 117, 103, 32, 50, 48, 50, 51, 32, 49, 50, 58, 50, 53, 58, 50, 50, 32, 43, 48, 52, 48, 48, 13, 10, 116, 111, 58, 122, 107, 101, 119, 116, 101, 115, 116, 64, 103, 109, 97, 105, 108, 46, 99, 111, 109, 13, 10, 100, 107, 105, 109, 45, 115, 105, 103, 110, 97, 116, 117, 114, 101, 58, 118, 61, 49, 59, 32, 97, 61, 114, 115, 97, 45, 115, 104, 97, 50, 53, 54, 59, 32, 99, 61, 114, 101, 108, 97, 120, 101, 100, 47, 114, 101, 108, 97, 120, 101, 100, 59, 32, 100, 61, 105, 99, 108, 111, 117, 100, 46, 99, 111, 109, 59, 32, 115, 61, 49, 97, 49, 104, 97, 105, 59, 32, 116, 61, 49, 54, 57, 51, 48, 51, 56, 51, 51, 55, 59, 32, 98, 104, 61, 55, 120, 81, 77, 68, 117, 111, 86, 86, 85, 52, 109, 48, 87, 48, 87, 82, 86, 83, 114, 86, 88, 77, 101, 71, 83, 73, 65, 83, 115, 110, 117, 99, 75, 57, 100, 74, 115, 114, 99, 43, 118, 85, 61, 59, 32, 104, 61, 102, 114, 111, 109, 58, 67, 111, 110, 116, 101, 110, 116, 45, 84, 121, 112, 101, 58, 77, 105, 109, 101, 45, 86, 101, 114, 115, 105, 111, 110, 58, 83, 117, 98, 106, 101, 99 + ]; + let result = [ + 116, 90, 151, 31, 78, 22, 138, 180, 211, 189, 69, 76, 227, 200, 155, 29, 59, 123, 154, 60, 47, 153, 203, 129, 157, 251, 48, 2, 79, 11, 65, 47 + ]; + assert_eq(sha256_var(input, input.len() as u64), result); + } + + #[test] + fn msg_just_under_block() { + let input = [ + 102, 114, 111, 109, 58, 114, 117, 110, 110, 105, 101, 114, 46, 108, 101, 97, 103, 117, 101, 115, 46, 48, 106, 64, 105, 99, 108, 111, 117, 100, 46, 99, 111, 109, 13, 10, 99, 111, 110, 116, 101, 110, 116, 45, 116, 121, 112, 101, 58, 116, 101, 120, 116, 47, 112, 108, 97, 105, 110, 59 + ]; + let result = [ + 143, 140, 76, 173, 222, 123, 102, 68, 70, 149, 207, 43, 39, 61, 34, 79, 216, 252, 213, 165, 74, 16, 110, 74, 29, 64, 138, 167, 30, 1, 9, 119 + ]; + assert_eq(sha256_var(input, input.len() as u64), result); + } + + #[test] + fn msg_big_not_block_multiple() { + let input = [ + 102, 114, 111, 109, 58, 114, 117, 110, 110, 105, 101, 114, 46, 108, 101, 97, 103, 117, 101, 115, 46, 48, 106, 64, 105, 99, 108, 111, 117, 100, 46, 99, 111, 109, 13, 10, 99, 111, 110, 116, 101, 110, 116, 45, 116, 121, 112, 101, 58, 116, 101, 120, 116, 47, 112, 108, 97, 105, 110, 59, 32, 99, 104, 97, 114, 115, 101, 116, 61, 117, 115, 45, 97, 115, 99, 105, 105, 13, 10, 109, 105, 109, 101, 45, 118, 101, 114, 115, 105, 111, 110, 58, 49, 46, 48, 32, 40, 77, 97, 99, 32, 79, 83, 32, 88, 32, 77, 97, 105, 108, 32, 49, 54, 46, 48, 32, 92, 40, 51, 55, 51, 49, 46, 53, 48, 48, 46, 50, 51, 49, 92, 41, 41, 13, 10, 115, 117, 98, 106, 101, 99, 116, 58, 72, 101, 108, 108, 111, 13, 10, 109, 101, 115, 115, 97, 103, 101, 45, 105, 100, 58, 60, 56, 70, 56, 49, 57, 68, 51, 50, 45, 66, 54, 65, 67, 45, 52, 56, 57, 68, 45, 57, 55, 55, 70, 45, 52, 51, 56, 66, 66, 67, 52, 67, 65, 66, 50, 55, 64, 109, 101, 46, 99, 111, 109, 62, 13, 10, 100, 97, 116, 101, 58, 83, 97, 116, 44, 32, 50, 54, 32, 65, 117, 103, 32, 50, 48, 50, 51, 32, 49, 50, 58, 50, 53, 58, 50, 50, 32, 43, 48, 52, 48, 48, 13, 10, 116, 111, 58, 122, 107, 101, 119, 116, 101, 115, 116, 64, 103, 109, 97, 105, 108, 46, 99, 111, 109, 13, 10, 100, 107, 105, 109, 45, 115, 105, 103, 110, 97, 116, 117, 114, 101, 58, 118, 61, 49, 59, 32, 97, 61, 114, 115, 97, 45, 115, 104, 97, 50, 53, 54, 59, 32, 99, 61, 114, 101, 108, 97, 120, 101, 100, 47, 114, 101, 108, 97, 120, 101, 100, 59, 32, 100, 61, 105, 99, 108, 111, 117, 100, 46, 99, 111, 109, 59, 32, 115, 61, 49, 97, 49, 104, 97, 105, 59, 32, 116, 61, 49, 54, 57, 51, 48, 51, 56, 51, 51, 55, 59, 32, 98, 104, 61, 55, 120, 81, 77, 68, 117, 111, 86, 86, 85, 52, 109, 48, 87, 48, 87, 82, 86, 83, 114, 86, 88, 77, 101, 71, 83, 73, 65, 83, 115, 110, 117, 99, 75, 57, 100, 74, 115, 114, 99, 43, 118, 85, 61, 59, 32, 104, 61, 102, 114, 111, 109, 58, 67, 111, 110, 116, 101, 110, 116, 45, 84, 121, 112, 101, 58, 77, 105, 109, 101, 45, 86, 101, 114, 115, 105, 111, 110, 58, 83, 117, 98, 106, 101, 99, 116, 58, 77, 101, 115, 115, 97, 103, 101, 45, 73, 100, 58, 68, 97, 116, 101, 58, 116, 111, 59, 32, 98, 61 + ]; + let result = [ + 112, 144, 73, 182, 208, 98, 9, 238, 54, 229, 61, 145, 222, 17, 72, 62, 148, 222, 186, 55, 192, 82, 220, 35, 66, 47, 193, 200, 22, 38, 26, 186 + ]; + assert_eq(sha256_var(input, input.len() as u64), result); + } + + #[test] + fn msg_big_with_padding() { + let input = [ + 48, 130, 1, 37, 2, 1, 0, 48, 11, 6, 9, 96, 134, 72, 1, 101, 3, 4, 2, 1, 48, 130, 1, 17, 48, 37, 2, 1, 1, 4, 32, 176, 223, 31, 133, 108, 84, 158, 102, 70, 11, 165, 175, 196, 12, 201, 130, 25, 131, 46, 125, 156, 194, 28, 23, 55, 133, 157, 164, 135, 136, 220, 78, 48, 37, 2, 1, 2, 4, 32, 190, 82, 180, 235, 222, 33, 79, 50, 152, 136, 142, 35, 116, 224, 6, 242, 156, 141, 128, 248, 10, 61, 98, 86, 248, 45, 207, 210, 90, 232, 175, 38, 48, 37, 2, 1, 3, 4, 32, 0, 194, 104, 108, 237, 246, 97, 230, 116, 198, 69, 110, 26, 87, 17, 89, 110, 199, 108, 250, 36, 21, 39, 87, 110, 102, 250, 213, 174, 131, 171, 174, 48, 37, 2, 1, 11, 4, 32, 136, 155, 87, 144, 111, 15, 152, 127, 85, 25, 154, 81, 20, 58, 51, 75, 193, 116, 234, 0, 60, 30, 29, 30, 183, 141, 72, 247, 255, 203, 100, 124, 48, 37, 2, 1, 12, 4, 32, 41, 234, 106, 78, 31, 11, 114, 137, 237, 17, 92, 71, 134, 47, 62, 78, 189, 233, 201, 214, 53, 4, 47, 189, 201, 133, 6, 121, 34, 131, 64, 142, 48, 37, 2, 1, 13, 4, 32, 91, 222, 210, 193, 62, 222, 104, 82, 36, 41, 138, 253, 70, 15, 148, 208, 156, 45, 105, 171, 241, 195, 185, 43, 217, 162, 146, 201, 222, 89, 238, 38, 48, 37, 2, 1, 14, 4, 32, 76, 123, 216, 13, 51, 227, 72, 245, 59, 193, 238, 166, 103, 49, 23, 164, 171, 188, 194, 197, 156, 187, 249, 28, 198, 95, 69, 15, 182, 56, 54, 38, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 + ]; + let result = [ + 32, 85, 108, 174, 127, 112, 178, 182, 8, 43, 134, 123, 192, 211, 131, 66, 184, 240, 212, 181, 240, 180, 106, 195, 24, 117, 54, 129, 19, 10, 250, 53 + ]; + let message_size = 297; + assert_eq(sha256_var(input, message_size), result); + } + + #[test] + fn msg_big_no_padding() { + let input = [ + 48, 130, 1, 37, 2, 1, 0, 48, 11, 6, 9, 96, 134, 72, 1, 101, 3, 4, 2, 1, 48, 130, 1, 17, 48, 37, 2, 1, 1, 4, 32, 176, 223, 31, 133, 108, 84, 158, 102, 70, 11, 165, 175, 196, 12, 201, 130, 25, 131, 46, 125, 156, 194, 28, 23, 55, 133, 157, 164, 135, 136, 220, 78, 48, 37, 2, 1, 2, 4, 32, 190, 82, 180, 235, 222, 33, 79, 50, 152, 136, 142, 35, 116, 224, 6, 242, 156, 141, 128, 248, 10, 61, 98, 86, 248, 45, 207, 210, 90, 232, 175, 38, 48, 37, 2, 1, 3, 4, 32, 0, 194, 104, 108, 237, 246, 97, 230, 116, 198, 69, 110, 26, 87, 17, 89, 110, 199, 108, 250, 36, 21, 39, 87, 110, 102, 250, 213, 174, 131, 171, 174, 48, 37, 2, 1, 11, 4, 32, 136, 155, 87, 144, 111, 15, 152, 127, 85, 25, 154, 81, 20, 58, 51, 75, 193, 116, 234, 0, 60, 30, 29, 30, 183, 141, 72, 247, 255, 203, 100, 124, 48, 37, 2, 1, 12, 4, 32, 41, 234, 106, 78, 31, 11, 114, 137, 237, 17, 92, 71, 134, 47, 62, 78, 189, 233, 201, 214, 53, 4, 47, 189, 201, 133, 6, 121, 34, 131, 64, 142, 48, 37, 2, 1, 13, 4, 32, 91, 222, 210, 193, 62, 222, 104, 82, 36, 41, 138, 253, 70, 15, 148, 208, 156, 45, 105, 171, 241, 195, 185, 43, 217, 162, 146, 201, 222, 89, 238, 38, 48, 37, 2, 1, 14, 4, 32, 76, 123, 216, 13, 51, 227, 72, 245, 59, 193, 238, 166, 103, 49, 23, 164, 171, 188, 194, 197, 156, 187, 249, 28, 198, 95, 69, 15, 182, 56, 54, 38 + ]; + let result = [ + 32, 85, 108, 174, 127, 112, 178, 182, 8, 43, 134, 123, 192, 211, 131, 66, 184, 240, 212, 181, 240, 180, 106, 195, 24, 117, 54, 129, 19, 10, 250, 53 + ]; + assert_eq(sha256_var(input, input.len() as u64), result); + } +} diff --git a/noir/noir-repo/noir_stdlib/src/meta/mod.nr b/noir/noir-repo/noir_stdlib/src/meta/mod.nr index 1d787ebcdc1..0dc2e5fa714 100644 --- a/noir/noir-repo/noir_stdlib/src/meta/mod.nr +++ b/noir/noir-repo/noir_stdlib/src/meta/mod.nr @@ -36,7 +36,7 @@ use crate::hash::poseidon2::Poseidon2Hasher; // A derive function is one that given a struct definition can // create us a quoted trait impl from it. -type DeriveFunction = fn(StructDefinition) -> Quoted; +pub type DeriveFunction = fn(StructDefinition) -> Quoted; // We'll keep a global HANDLERS map to keep track of the derive handler for each trait comptime mut global HANDLERS: UHashMap> = UHashMap::default(); @@ -243,11 +243,15 @@ mod tests { } // docs:end:big-derive-usage-example + impl DoNothing for Bar { + fn do_nothing(_: Self) {} + } + // This function is just to remove unused warnings fn remove_unused_warnings() { - let _: Bar = crate::panic::panic(f""); - let _: MyStruct = crate::panic::panic(f""); - let _: MyOtherStruct = crate::panic::panic(f""); + let _: Bar = Bar { x: 1, y: [2, 3] }; + let _: MyStruct = MyStruct { my_field: 1 }; + let _: MyOtherStruct = MyOtherStruct { my_other_field: 2 }; let _ = derive_do_nothing(crate::panic::panic(f"")); let _ = derive_do_nothing_alt(crate::panic::panic(f"")); remove_unused_warnings(); diff --git a/noir/noir-repo/noir_stdlib/src/schnorr.nr b/noir/noir-repo/noir_stdlib/src/schnorr.nr index 336041fec19..e24aabf3cda 100644 --- a/noir/noir-repo/noir_stdlib/src/schnorr.nr +++ b/noir/noir-repo/noir_stdlib/src/schnorr.nr @@ -1,4 +1,3 @@ -use crate::collections::vec::Vec; use crate::embedded_curve_ops::{EmbeddedCurvePoint, EmbeddedCurveScalar}; #[foreign(schnorr_verify)] @@ -23,7 +22,11 @@ pub fn verify_signature_slice( // docs:end:schnorr_verify_slice {} -pub fn verify_signature_noir(public_key: EmbeddedCurvePoint, signature: [u8; 64], message: [u8; N]) -> bool { +pub fn verify_signature_noir( + public_key: EmbeddedCurvePoint, + signature: [u8; 64], + message: [u8; N] +) -> bool { //scalar lo/hi from bytes let sig_s = EmbeddedCurveScalar::from_bytes(signature, 0); let sig_e = EmbeddedCurveScalar::from_bytes(signature, 32); @@ -42,7 +45,11 @@ pub fn verify_signature_noir(public_key: EmbeddedCurvePoint, signatu is_ok } -pub fn assert_valid_signature(public_key: EmbeddedCurvePoint, signature: [u8; 64], message: [u8; N]) { +pub fn assert_valid_signature( + public_key: EmbeddedCurvePoint, + signature: [u8; 64], + message: [u8; N] +) { //scalar lo/hi from bytes let sig_s = EmbeddedCurveScalar::from_bytes(signature, 0); let sig_e = EmbeddedCurveScalar::from_bytes(signature, 32); diff --git a/noir/noir-repo/noir_stdlib/src/sha256.nr b/noir/noir-repo/noir_stdlib/src/sha256.nr index ce217f7a689..7679e517317 100644 --- a/noir/noir-repo/noir_stdlib/src/sha256.nr +++ b/noir/noir-repo/noir_stdlib/src/sha256.nr @@ -1,2 +1,11 @@ // This file is kept for backwards compatibility. -pub use crate::hash::sha256::{digest, sha256_var}; + +#[deprecated = "replace with std::hash::sha256::digest"] +pub fn digest(msg: [u8; N]) -> [u8; 32] { + crate::hash::sha256::digest(msg) +} + +#[deprecated = "replace with std::hash::sha256::sha256_var"] +pub fn sha256_var(msg: [u8; N], message_size: u64) -> [u8; 32] { + crate::hash::sha256::sha256_var(msg, message_size) +} diff --git a/noir/noir-repo/noir_stdlib/src/sha512.nr b/noir/noir-repo/noir_stdlib/src/sha512.nr index b474e27b416..0a8a5bf4760 100644 --- a/noir/noir-repo/noir_stdlib/src/sha512.nr +++ b/noir/noir-repo/noir_stdlib/src/sha512.nr @@ -1,2 +1,6 @@ // This file is kept for backwards compatibility. -pub use crate::hash::sha512::digest; + +#[deprecated = "replace with std::hash::sha512::digest"] +pub fn digest(msg: [u8; N]) -> [u8; 64] { + crate::hash::sha512::digest(msg) +} diff --git a/noir/noir-repo/scripts/install_bb.sh b/noir/noir-repo/scripts/install_bb.sh index d60c73c0976..c94a1b7dff0 100755 --- a/noir/noir-repo/scripts/install_bb.sh +++ b/noir/noir-repo/scripts/install_bb.sh @@ -1,6 +1,6 @@ #!/bin/bash -VERSION="0.55.0" +VERSION="0.56.0" BBUP_PATH=~/.bb/bbup diff --git a/noir/noir-repo/test_programs/compile_success_empty/field_or_integer_static_trait_method/src/main.nr b/noir/noir-repo/test_programs/compile_success_empty/field_or_integer_static_trait_method/src/main.nr index f5a54c82ce5..64eb564cda1 100644 --- a/noir/noir-repo/test_programs/compile_success_empty/field_or_integer_static_trait_method/src/main.nr +++ b/noir/noir-repo/test_programs/compile_success_empty/field_or_integer_static_trait_method/src/main.nr @@ -23,3 +23,10 @@ fn main() { let value: Field = Field::read(data); assert_eq(value, 10); } + +#[attr] +pub fn foo() {} + +comptime fn attr(_: FunctionDefinition) -> Quoted { + quote { pub fn hello() {} } +} diff --git a/noir/noir-repo/tooling/lsp/Cargo.toml b/noir/noir-repo/tooling/lsp/Cargo.toml index c15895d801f..209f2afe4a4 100644 --- a/noir/noir-repo/tooling/lsp/Cargo.toml +++ b/noir/noir-repo/tooling/lsp/Cargo.toml @@ -14,6 +14,7 @@ workspace = true [dependencies] acvm.workspace = true +chumsky.workspace = true codespan-lsp.workspace = true lsp-types.workspace = true nargo.workspace = true diff --git a/noir/noir-repo/tooling/lsp/src/attribute_reference_finder.rs b/noir/noir-repo/tooling/lsp/src/attribute_reference_finder.rs new file mode 100644 index 00000000000..f08c8073a79 --- /dev/null +++ b/noir/noir-repo/tooling/lsp/src/attribute_reference_finder.rs @@ -0,0 +1,115 @@ +/// If the cursor is on an custom attribute, this struct will try to resolve its +/// underlying function and return a ReferenceId to it. +/// This is needed in hover and go-to-definition because when an annotation generates +/// code, that code ends up residing in the attribute definition (it ends up having the +/// attribute's span) so using the usual graph to locate what points to that location +/// will give not only the attribute function but also any type generated by it. +use std::collections::BTreeMap; + +use chumsky::Parser; +use fm::FileId; +use noirc_errors::Span; +use noirc_frontend::{ + ast::{AttributeTarget, Visitor}, + graph::CrateId, + hir::{ + def_map::{CrateDefMap, LocalModuleId, ModuleId}, + resolution::path_resolver::{PathResolver, StandardPathResolver}, + }, + lexer::Lexer, + node_interner::ReferenceId, + parser::{path_no_turbofish, ParsedSubModule}, + token::CustomAttribute, + usage_tracker::UsageTracker, + 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 + /// if we are analyzing something inside an inline module declaration. + module_id: ModuleId, + def_maps: &'a BTreeMap, + reference_id: Option, +} + +impl<'a> AttributeReferenceFinder<'a> { + #[allow(clippy::too_many_arguments)] + pub(crate) fn new( + file: FileId, + byte_index: usize, + krate: CrateId, + def_maps: &'a BTreeMap, + ) -> Self { + // Find the module the current file belongs to + let def_map = &def_maps[&krate]; + let local_id = if let Some((module_index, _)) = + def_map.modules().iter().find(|(_, module_data)| module_data.location.file == file) + { + LocalModuleId(module_index) + } else { + def_map.root() + }; + let module_id = ModuleId { krate, local_id }; + Self { byte_index, module_id, def_maps, reference_id: None } + } + + pub(crate) fn find(&mut self, parsed_module: &ParsedModule) -> Option { + parsed_module.accept(self); + + self.reference_id + } + + fn includes_span(&self, span: Span) -> bool { + span.start() as usize <= self.byte_index && self.byte_index <= span.end() as usize + } +} + +impl<'a> Visitor for AttributeReferenceFinder<'a> { + fn visit_parsed_submodule(&mut self, parsed_sub_module: &ParsedSubModule, _span: Span) -> bool { + // Switch `self.module_id` to the submodule + let previous_module_id = self.module_id; + + let def_map = &self.def_maps[&self.module_id.krate]; + if let Some(module_data) = def_map.modules().get(self.module_id.local_id.0) { + if let Some(child_module) = module_data.children.get(&parsed_sub_module.name) { + self.module_id = ModuleId { krate: self.module_id.krate, local_id: *child_module }; + } + } + + parsed_sub_module.accept_children(self); + + // Restore the old module before continuing + self.module_id = previous_module_id; + + false + } + + fn visit_custom_attribute(&mut self, attribute: &CustomAttribute, _target: AttributeTarget) { + if !self.includes_span(attribute.contents_span) { + return; + } + + let name = match attribute.contents.split_once('(') { + Some((left, _right)) => left.to_string(), + None => attribute.contents.to_string(), + }; + let (tokens, _) = Lexer::lex(&name); + + let parser = path_no_turbofish(); + let Ok(path) = parser.parse(tokens) else { + return; + }; + + let resolver = StandardPathResolver::new(self.module_id); + let mut usage_tracker = UsageTracker::default(); + let Ok(result) = resolver.resolve(self.def_maps, path, &mut usage_tracker, &mut None) + else { + return; + }; + + self.reference_id = Some(module_def_id_to_reference_id(result.module_def_id)); + } +} diff --git a/noir/noir-repo/tooling/lsp/src/lib.rs b/noir/noir-repo/tooling/lsp/src/lib.rs index 39d4c3faa61..771f67e1fa2 100644 --- a/noir/noir-repo/tooling/lsp/src/lib.rs +++ b/noir/noir-repo/tooling/lsp/src/lib.rs @@ -62,6 +62,7 @@ use serde_json::Value as JsonValue; use thiserror::Error; use tower::Service; +mod attribute_reference_finder; mod modules; mod notifications; mod requests; diff --git a/noir/noir-repo/tooling/lsp/src/modules.rs b/noir/noir-repo/tooling/lsp/src/modules.rs index c8f7430c997..f1eff3b5a7d 100644 --- a/noir/noir-repo/tooling/lsp/src/modules.rs +++ b/noir/noir-repo/tooling/lsp/src/modules.rs @@ -18,15 +18,6 @@ pub(crate) fn get_parent_module( interner.reference_module(reference_id).copied() } -pub(crate) fn get_parent_module_id( - def_maps: &BTreeMap, - module_id: ModuleId, -) -> Option { - let crate_def_map = &def_maps[&module_id.krate]; - let module_data = &crate_def_map.modules()[module_id.local_id.0]; - module_data.parent.map(|parent| ModuleId { krate: module_id.krate, local_id: parent }) -} - pub(crate) fn module_def_id_to_reference_id(module_def_id: ModuleDefId) -> ReferenceId { match module_def_id { ModuleDefId::ModuleId(id) => ReferenceId::Module(id), diff --git a/noir/noir-repo/tooling/lsp/src/requests/code_action/import_or_qualify.rs b/noir/noir-repo/tooling/lsp/src/requests/code_action/import_or_qualify.rs index 03a953bde85..0d97ccde2ed 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/code_action/import_or_qualify.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/code_action/import_or_qualify.rs @@ -7,7 +7,7 @@ use noirc_frontend::{ use crate::{ byte_span_to_range, - modules::{get_parent_module_id, relative_module_full_path, relative_module_id_path}, + modules::{relative_module_full_path, relative_module_id_path}, }; use super::CodeActionFinder; @@ -28,7 +28,7 @@ impl<'a> CodeActionFinder<'a> { return; } - let current_module_parent_id = get_parent_module_id(self.def_maps, self.module_id); + let current_module_parent_id = self.module_id.parent(self.def_maps); // The Path doesn't resolve to anything so it means it's an error and maybe we // can suggest an import or to fully-qualify the path. diff --git a/noir/noir-repo/tooling/lsp/src/requests/completion/auto_import.rs b/noir/noir-repo/tooling/lsp/src/requests/completion/auto_import.rs index 2713ae252bf..20b126a248d 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/completion/auto_import.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/completion/auto_import.rs @@ -1,7 +1,7 @@ use lsp_types::{Position, Range, TextEdit}; use noirc_frontend::macros_api::ModuleDefId; -use crate::modules::{get_parent_module_id, relative_module_full_path, relative_module_id_path}; +use crate::modules::{relative_module_full_path, relative_module_id_path}; use super::{ kinds::{FunctionCompletionKind, FunctionKind, RequestedItems}, @@ -17,7 +17,7 @@ impl<'a> NodeFinder<'a> { requested_items: RequestedItems, function_completion_kind: FunctionCompletionKind, ) { - let current_module_parent_id = get_parent_module_id(self.def_maps, self.module_id); + let current_module_parent_id = self.module_id.parent(self.def_maps); for (name, entries) in self.interner.get_auto_import_names() { if !name_matches(name, prefix) { diff --git a/noir/noir-repo/tooling/lsp/src/requests/completion/builtins.rs b/noir/noir-repo/tooling/lsp/src/requests/completion/builtins.rs index 6812ebc135b..cf2af4036f7 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/completion/builtins.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/completion/builtins.rs @@ -116,6 +116,15 @@ impl<'a> NodeFinder<'a> { )); } } + AttributeTarget::Let => { + if name_matches("allow", prefix) || name_matches("unused_variables", prefix) { + self.completion_items.push(simple_completion_item( + "allow(unused_variables)", + CompletionItemKind::METHOD, + None, + )); + } + } } } } diff --git a/noir/noir-repo/tooling/lsp/src/requests/completion/completion_items.rs b/noir/noir-repo/tooling/lsp/src/requests/completion/completion_items.rs index c0155096dc8..809988c34a5 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/completion/completion_items.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/completion/completion_items.rs @@ -51,6 +51,10 @@ impl<'a> NodeFinder<'a> { AttributeTarget::Struct => Some(Type::Quoted(QuotedType::StructDefinition)), AttributeTarget::Trait => Some(Type::Quoted(QuotedType::TraitDefinition)), AttributeTarget::Function => Some(Type::Quoted(QuotedType::FunctionDefinition)), + AttributeTarget::Let => { + // No item can be suggested for a let statement attribute + return Vec::new(); + } } } else { None diff --git a/noir/noir-repo/tooling/lsp/src/requests/completion/tests.rs b/noir/noir-repo/tooling/lsp/src/requests/completion/tests.rs index 45eb79bd1c2..68cb3870f63 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/completion/tests.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/completion/tests.rs @@ -1942,6 +1942,26 @@ mod completion_tests { .await; } + #[test] + async fn test_suggests_built_in_let_attribute() { + let src = r#" + fn foo() { + #[allo>|<] + let x = 1; + } + "#; + + assert_completion_excluding_auto_import( + src, + vec![simple_completion_item( + "allow(unused_variables)", + CompletionItemKind::METHOD, + None, + )], + ) + .await; + } + #[test] async fn test_suggests_function_attribute() { let src = r#" diff --git a/noir/noir-repo/tooling/lsp/src/requests/goto_definition.rs b/noir/noir-repo/tooling/lsp/src/requests/goto_definition.rs index 6538e64dc90..a2443ea165d 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/goto_definition.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/goto_definition.rs @@ -1,8 +1,11 @@ use std::future::{self, Future}; +use crate::attribute_reference_finder::AttributeReferenceFinder; +use crate::utils; use crate::{types::GotoDefinitionResult, LspState}; use async_lsp::ResponseError; +use fm::PathString; use lsp_types::request::GotoTypeDefinitionParams; use lsp_types::{GotoDefinitionParams, GotoDefinitionResponse}; @@ -29,21 +32,42 @@ fn on_goto_definition_inner( params: GotoDefinitionParams, return_type_location_instead: bool, ) -> Result { + let uri = params.text_document_position_params.text_document.uri.clone(); + let position = params.text_document_position_params.position; process_request(state, params.text_document_position_params, |args| { - args.interner - .get_definition_location_from(args.location, return_type_location_instead) - .or_else(|| { - args.interner - .reference_at_location(args.location) - .map(|reference| args.interner.reference_location(reference)) - }) - .and_then(|found_location| { - let file_id = found_location.file; - let definition_position = - to_lsp_location(args.files, file_id, found_location.span)?; - let response = GotoDefinitionResponse::from(definition_position).to_owned(); - Some(response) + let path = PathString::from_path(uri.to_file_path().unwrap()); + let reference_id = args.files.get_file_id(&path).and_then(|file_id| { + utils::position_to_byte_index(args.files, file_id, &position).and_then(|byte_index| { + let file = args.files.get_file(file_id).unwrap(); + let source = file.source(); + let (parsed_module, _errors) = noirc_frontend::parse_program(source); + + let mut finder = AttributeReferenceFinder::new( + file_id, + byte_index, + args.crate_id, + args.def_maps, + ); + finder.find(&parsed_module) }) + }); + let location = if let Some(reference_id) = reference_id { + Some(args.interner.reference_location(reference_id)) + } else { + args.interner + .get_definition_location_from(args.location, return_type_location_instead) + .or_else(|| { + args.interner + .reference_at_location(args.location) + .map(|reference| args.interner.reference_location(reference)) + }) + }; + location.and_then(|found_location| { + let file_id = found_location.file; + let definition_position = to_lsp_location(args.files, file_id, found_location.span)?; + let response = GotoDefinitionResponse::from(definition_position).to_owned(); + Some(response) + }) }) } @@ -249,4 +273,18 @@ mod goto_definition_tests { ) .await; } + + #[test] + async fn goto_attribute_function() { + expect_goto( + "go_to_definition", + Position { line: 31, character: 3 }, // "attr" + "src/main.nr", + Range { + start: Position { line: 34, character: 12 }, + end: Position { line: 34, character: 16 }, + }, + ) + .await; + } } diff --git a/noir/noir-repo/tooling/lsp/src/requests/hover.rs b/noir/noir-repo/tooling/lsp/src/requests/hover.rs index 46d2a5cfc8f..2628c9b2ab6 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/hover.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/hover.rs @@ -1,7 +1,7 @@ use std::future::{self, Future}; use async_lsp::ResponseError; -use fm::FileMap; +use fm::{FileMap, PathString}; use lsp_types::{Hover, HoverContents, HoverParams, MarkupContent, MarkupKind}; use noirc_frontend::{ ast::Visibility, @@ -15,7 +15,10 @@ use noirc_frontend::{ Generics, Shared, StructType, Type, TypeAlias, TypeBinding, TypeVariable, }; -use crate::{modules::module_full_path, LspState}; +use crate::{ + attribute_reference_finder::AttributeReferenceFinder, modules::module_full_path, utils, + LspState, +}; use super::{process_request, to_lsp_location, ProcessRequestCallbackArgs}; @@ -23,18 +26,41 @@ pub(crate) fn on_hover_request( state: &mut LspState, params: HoverParams, ) -> impl Future, ResponseError>> { + let uri = params.text_document_position_params.text_document.uri.clone(); + let position = params.text_document_position_params.position; let result = process_request(state, params.text_document_position_params, |args| { - args.interner.reference_at_location(args.location).and_then(|reference| { - let location = args.interner.reference_location(reference); - let lsp_location = to_lsp_location(args.files, location.file, location.span); - format_reference(reference, &args).map(|formatted| Hover { - range: lsp_location.map(|location| location.range), - contents: HoverContents::Markup(MarkupContent { - kind: MarkupKind::Markdown, - value: formatted, - }), + let path = PathString::from_path(uri.to_file_path().unwrap()); + args.files + .get_file_id(&path) + .and_then(|file_id| { + utils::position_to_byte_index(args.files, file_id, &position).and_then( + |byte_index| { + let file = args.files.get_file(file_id).unwrap(); + let source = file.source(); + let (parsed_module, _errors) = noirc_frontend::parse_program(source); + + let mut finder = AttributeReferenceFinder::new( + file_id, + byte_index, + args.crate_id, + args.def_maps, + ); + finder.find(&parsed_module) + }, + ) + }) + .or_else(|| args.interner.reference_at_location(args.location)) + .and_then(|reference| { + let location = args.interner.reference_location(reference); + let lsp_location = to_lsp_location(args.files, location.file, location.span); + format_reference(reference, &args).map(|formatted| Hover { + range: lsp_location.map(|location| location.range), + contents: HoverContents::Markup(MarkupContent { + kind: MarkupKind::Markdown, + value: formatted, + }), + }) }) - }) }); future::ready(result) @@ -918,4 +944,15 @@ mod hover_tests { ) .await; } + + #[test] + async fn hover_on_attribute_function() { + assert_hover( + "workspace", + "two/src/lib.nr", + Position { line: 54, character: 2 }, + " two\n fn attr(_: FunctionDefinition) -> Quoted", + ) + .await; + } } diff --git a/noir/noir-repo/tooling/lsp/src/trait_impl_method_stub_generator.rs b/noir/noir-repo/tooling/lsp/src/trait_impl_method_stub_generator.rs index ae12bac4c06..56d2e5e1ea1 100644 --- a/noir/noir-repo/tooling/lsp/src/trait_impl_method_stub_generator.rs +++ b/noir/noir-repo/tooling/lsp/src/trait_impl_method_stub_generator.rs @@ -197,12 +197,7 @@ impl<'a> TraitImplMethodStubGenerator<'a> { } } - let module_id = struct_type.id.module_id(); - let module_data = &self.def_maps[&module_id.krate].modules()[module_id.local_id.0]; - let parent_module_local_id = module_data.parent.unwrap(); - let parent_module_id = - ModuleId { krate: module_id.krate, local_id: parent_module_local_id }; - + let parent_module_id = struct_type.id.parent_module_id(self.def_maps); let current_module_parent_id = current_module_data .parent .map(|parent| ModuleId { krate: self.module_id.krate, local_id: parent }); diff --git a/noir/noir-repo/tooling/lsp/test_programs/go_to_definition/src/main.nr b/noir/noir-repo/tooling/lsp/test_programs/go_to_definition/src/main.nr index 4550324c182..cb04fe04eaa 100644 --- a/noir/noir-repo/tooling/lsp/test_programs/go_to_definition/src/main.nr +++ b/noir/noir-repo/tooling/lsp/test_programs/go_to_definition/src/main.nr @@ -28,3 +28,10 @@ trait Trait { } use dependency::something; + +#[attr] +pub fn foo() {} + +comptime fn attr(_: FunctionDefinition) -> Quoted { + quote { pub fn hello() {} } +} \ No newline at end of file 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 3f0f0f117b7..c6b516d88ab 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 @@ -51,3 +51,10 @@ use std::collections::bounded_vec::BoundedVec; fn instantiate_generic() { let x: BoundedVec = BoundedVec::new(); } + +#[attr] +pub fn foo() {} + +comptime fn attr(_: FunctionDefinition) -> Quoted { + quote { pub fn hello() {} } +} \ No newline at end of file diff --git a/noir/noir-repo/tooling/nargo_fmt/src/visitor/item.rs b/noir/noir-repo/tooling/nargo_fmt/src/visitor/item.rs index 7d5e1886072..b2a689919bd 100644 --- a/noir/noir-repo/tooling/nargo_fmt/src/visitor/item.rs +++ b/noir/noir-repo/tooling/nargo_fmt/src/visitor/item.rs @@ -276,7 +276,7 @@ impl super::FmtVisitor<'_> { ItemKind::Struct(_) | ItemKind::Trait(_) | ItemKind::TypeAlias(_) - | ItemKind::Global(_) + | ItemKind::Global(..) | ItemKind::ModuleDecl(_) | ItemKind::InnerAttribute(_) => { self.push_rewrite(self.slice(span).to_string(), span); diff --git a/noir/noir-repo/tooling/noir_js/src/index.ts b/noir/noir-repo/tooling/noir_js/src/index.ts index 653c51a2292..f3016efd032 100644 --- a/noir/noir-repo/tooling/noir_js/src/index.ts +++ b/noir/noir-repo/tooling/noir_js/src/index.ts @@ -2,14 +2,7 @@ import * as acvm from '@noir-lang/acvm_js'; import * as abi from '@noir-lang/noirc_abi'; import { CompiledCircuit } from '@noir-lang/types'; -export { - ecdsa_secp256r1_verify, - ecdsa_secp256k1_verify, - keccak256, - blake2s256, - xor, - and, -} from '@noir-lang/acvm_js'; +export { ecdsa_secp256r1_verify, ecdsa_secp256k1_verify, keccak256, blake2s256, xor, and } from '@noir-lang/acvm_js'; export { InputMap } from '@noir-lang/noirc_abi'; export { WitnessMap, ForeignCallHandler, ForeignCallInput, ForeignCallOutput } from '@noir-lang/acvm_js'; diff --git a/yarn-project/yarn.lock b/yarn-project/yarn.lock index 677a033c70b..5275032f798 100644 --- a/yarn-project/yarn.lock +++ b/yarn-project/yarn.lock @@ -3380,12 +3380,12 @@ __metadata: "@noir-lang/noir_js@file:../noir/packages/noir_js::locator=%40aztec%2Faztec3-packages%40workspace%3A.": version: 0.34.0 - resolution: "@noir-lang/noir_js@file:../noir/packages/noir_js#../noir/packages/noir_js::hash=999f32&locator=%40aztec%2Faztec3-packages%40workspace%3A." + resolution: "@noir-lang/noir_js@file:../noir/packages/noir_js#../noir/packages/noir_js::hash=aa1d0b&locator=%40aztec%2Faztec3-packages%40workspace%3A." dependencies: "@noir-lang/acvm_js": 0.50.0 "@noir-lang/noirc_abi": 0.34.0 "@noir-lang/types": 0.34.0 - checksum: 72009b3553a8c2652270b86b02bf5215456adb3c66ebb08a3b66185d20b708e86cf3300c3f57641d2e9e9f10d340bbf5e2f9fb1f89507900ccdd9b0e42c4349a + checksum: 4d5102053ee8ef7e1258221275796493eeaef14664e5de238ce293151c90d0f9aaa19df1c0beb94b3a3911694e50bbe9f71a93497cdfc69cf0e5eb7ee6af3142 languageName: node linkType: hard