From 79df6a36901d50d8cc8d5b59e51078fe74ad1ac2 Mon Sep 17 00:00:00 2001 From: jfecher Date: Wed, 4 Sep 2024 06:30:51 -0500 Subject: [PATCH 01/25] chore: Add pass to normalize Ids in SSA (#5909) # Description ## Problem\* SSA can be difficult to debug manually, particularly for very large files. Even when comparing two sources side by side it can be hard to find exactly where they differ since one optimization difference earlier on can affect where ValueIds start in every function afterward. ## Summary\* This PR adds a pass to normalize ids in an SSA program - restarting from v0 after every SSA pass instead of continuing from the previous end. The goal of this is to be able to take two SSA programs and easily diff them to find out where they start differing. E.g. using this on two files containing the final SSA from https://github.com/noir-lang/noir/issues/5771 in both failing and passing versions, it is clear that they differ in exactly one ValueId in one instruction. ## Additional Context This new pass is only run when `--show-ssa` is specified, and is run before each printout. ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> --- compiler/noirc_evaluator/src/ssa.rs | 4 +- .../noirc_evaluator/src/ssa/acir_gen/mod.rs | 10 +- .../check_for_underconstrained_values.rs | 2 +- .../noirc_evaluator/src/ssa/ir/function.rs | 7 + compiler/noirc_evaluator/src/ssa/ir/map.rs | 10 +- .../noirc_evaluator/src/ssa/opt/inlining.rs | 13 +- compiler/noirc_evaluator/src/ssa/opt/mod.rs | 1 + .../src/ssa/opt/normalize_value_ids.rs | 194 ++++++++++++++++++ 8 files changed, 227 insertions(+), 14 deletions(-) create mode 100644 compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs diff --git a/compiler/noirc_evaluator/src/ssa.rs b/compiler/noirc_evaluator/src/ssa.rs index 57bd76d4f78..ad6645df228 100644 --- a/compiler/noirc_evaluator/src/ssa.rs +++ b/compiler/noirc_evaluator/src/ssa.rs @@ -82,6 +82,7 @@ pub(crate) fn optimize_into_acir( ) -> Result { let ssa_gen_span = span!(Level::TRACE, "ssa_generation"); let ssa_gen_span_guard = ssa_gen_span.enter(); + let mut ssa = SsaBuilder::new( program, options.enable_ssa_logging, @@ -418,8 +419,9 @@ impl SsaBuilder { Ok(self.print(msg)) } - fn print(self, msg: &str) -> Self { + fn print(mut self, msg: &str) -> Self { if self.print_ssa_passes { + self.ssa.normalize_ids(); println!("{msg}\n{}", self.ssa); } self diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index a2b9e46a15a..0360b15d950 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -770,10 +770,12 @@ impl<'a> Context<'a> { .map(|result_id| dfg.type_of_value(*result_id).flattened_size()) .sum(); - let acir_function_id = ssa - .entry_point_to_generated_index - .get(id) - .expect("ICE: should have an associated final index"); + let Some(acir_function_id) = + ssa.entry_point_to_generated_index.get(id) + else { + unreachable!("Expected an associated final index for call to acir function {id} with args {arguments:?}"); + }; + let output_vars = self.acir_context.call_acir_function( AcirFunctionId(*acir_function_id), inputs, diff --git a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs index 26eab290d4b..aa5f4c8df95 100644 --- a/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs +++ b/compiler/noirc_evaluator/src/ssa/checks/check_for_underconstrained_values.rs @@ -244,7 +244,7 @@ impl Context { } }, Value::ForeignFunction(..) => { - panic!("Should not be able to reach foreign function from non-brillig functions"); + panic!("Should not be able to reach foreign function from non-brillig functions, {func_id} in function {}", function.name()); } Value::Array { .. } | Value::Instruction { .. } diff --git a/compiler/noirc_evaluator/src/ssa/ir/function.rs b/compiler/noirc_evaluator/src/ssa/ir/function.rs index bae9f82e4f1..65a616ef612 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/function.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/function.rs @@ -72,6 +72,13 @@ impl Function { Self { name: another.name.clone(), id, entry_block, dfg, runtime: another.runtime } } + /// Takes the signature (function name & runtime) from a function but does not copy the body. + pub(crate) fn clone_signature(id: FunctionId, another: &Function) -> Self { + let mut new_function = Function::new(another.name.clone(), id); + new_function.runtime = another.runtime; + new_function + } + /// The name of the function. /// Used exclusively for debugging purposes. pub(crate) fn name(&self) -> &str { diff --git a/compiler/noirc_evaluator/src/ssa/ir/map.rs b/compiler/noirc_evaluator/src/ssa/ir/map.rs index 769d52e6e65..23f5380f030 100644 --- a/compiler/noirc_evaluator/src/ssa/ir/map.rs +++ b/compiler/noirc_evaluator/src/ssa/ir/map.rs @@ -1,6 +1,7 @@ use fxhash::FxHashMap as HashMap; use serde::{Deserialize, Serialize}; use std::{ + collections::BTreeMap, hash::Hash, str::FromStr, sync::atomic::{AtomicUsize, Ordering}, @@ -240,7 +241,7 @@ impl std::ops::IndexMut> for DenseMap { /// call to .remove(). #[derive(Debug)] pub(crate) struct SparseMap { - storage: HashMap, T>, + storage: BTreeMap, T>, } impl SparseMap { @@ -271,11 +272,16 @@ impl SparseMap { pub(crate) fn remove(&mut self, id: Id) -> Option { self.storage.remove(&id) } + + /// Unwraps the inner storage of this map + pub(crate) fn into_btree(self) -> BTreeMap, T> { + self.storage + } } impl Default for SparseMap { fn default() -> Self { - Self { storage: HashMap::default() } + Self { storage: Default::default() } } } diff --git a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index 1ff593a1531..7843c55da65 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -2,7 +2,7 @@ //! The purpose of this pass is to inline the instructions of each function call //! within the function caller. If all function calls are known, there will only //! be a single function remaining when the pass finishes. -use std::collections::{BTreeSet, HashSet}; +use std::collections::{BTreeSet, HashSet, VecDeque}; use acvm::acir::AcirField; use iter_extended::{btree_map, vecmap}; @@ -372,14 +372,14 @@ impl<'function> PerFunctionContext<'function> { fn translate_block( &mut self, source_block: BasicBlockId, - block_queue: &mut Vec, + block_queue: &mut VecDeque, ) -> BasicBlockId { if let Some(block) = self.blocks.get(&source_block) { return *block; } // The block is not yet inlined, queue it - block_queue.push(source_block); + block_queue.push_back(source_block); // The block is not already present in the function being inlined into so we must create it. // The block's instructions are not copied over as they will be copied later in inlining. @@ -415,13 +415,14 @@ impl<'function> PerFunctionContext<'function> { /// Inline all reachable blocks within the source_function into the destination function. fn inline_blocks(&mut self, ssa: &Ssa) -> Vec { let mut seen_blocks = HashSet::new(); - let mut block_queue = vec![self.source_function.entry_block()]; + let mut block_queue = VecDeque::new(); + block_queue.push_back(self.source_function.entry_block()); // This Vec will contain each block with a Return instruction along with the // returned values of that block. let mut function_returns = vec![]; - while let Some(source_block_id) = block_queue.pop() { + while let Some(source_block_id) = block_queue.pop_front() { if seen_blocks.contains(&source_block_id) { continue; } @@ -609,7 +610,7 @@ impl<'function> PerFunctionContext<'function> { fn handle_terminator_instruction( &mut self, block_id: BasicBlockId, - block_queue: &mut Vec, + block_queue: &mut VecDeque, ) -> Option<(BasicBlockId, Vec)> { match self.source_function.dfg[block_id].unwrap_terminator() { TerminatorInstruction::Jmp { destination, arguments, call_stack } => { diff --git a/compiler/noirc_evaluator/src/ssa/opt/mod.rs b/compiler/noirc_evaluator/src/ssa/opt/mod.rs index 4e5fa262696..bd9d0baff97 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mod.rs @@ -13,6 +13,7 @@ mod die; pub(crate) mod flatten_cfg; mod inlining; mod mem2reg; +mod normalize_value_ids; mod rc; mod remove_bit_shifts; mod remove_enable_side_effects; diff --git a/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs b/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs new file mode 100644 index 00000000000..f11b310494b --- /dev/null +++ b/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs @@ -0,0 +1,194 @@ +use std::collections::BTreeMap; + +use crate::ssa::{ + ir::{ + basic_block::BasicBlockId, + function::{Function, FunctionId}, + map::SparseMap, + post_order::PostOrder, + value::{Value, ValueId}, + }, + ssa_gen::Ssa, +}; +use fxhash::FxHashMap as HashMap; +use iter_extended::vecmap; + +impl Ssa { + /// This is a debugging pass which re-inserts each instruction + /// and block in a fresh DFG context for each function so that ValueIds, + /// BasicBlockIds, and FunctionIds are always identical for the same SSA code. + /// + /// During normal compilation this is often not the case since prior passes + /// may increase the ID counter so that later passes start at different offsets, + /// even if they contain the same SSA code. + pub(crate) fn normalize_ids(&mut self) { + let mut context = Context::default(); + context.populate_functions(&self.functions); + for function in self.functions.values_mut() { + context.normalize_ids(function); + } + self.functions = context.functions.into_btree(); + } +} + +#[derive(Default)] +struct Context { + functions: SparseMap, + + new_ids: IdMaps, +} + +/// Maps from old ids to new ones. +/// Separate from the rest of Context so we can call mutable methods on it +/// while Context gives out mutable references to functions within. +#[derive(Default)] +struct IdMaps { + // Maps old function id -> new function id + function_ids: HashMap, + + // Maps old block id -> new block id + // Cleared in between each function. + blocks: HashMap, + + // Maps old value id -> new value id + // Cleared in between each function. + values: HashMap, +} + +impl Context { + fn populate_functions(&mut self, functions: &BTreeMap) { + for (id, function) in functions { + self.functions.insert_with_id(|new_id| { + self.new_ids.function_ids.insert(*id, new_id); + Function::clone_signature(new_id, function) + }); + } + } + + fn normalize_ids(&mut self, old_function: &mut Function) { + self.new_ids.blocks.clear(); + self.new_ids.values.clear(); + + let new_function_id = self.new_ids.function_ids[&old_function.id()]; + let new_function = &mut self.functions[new_function_id]; + + let mut reachable_blocks = PostOrder::with_function(old_function).into_vec(); + reachable_blocks.reverse(); + + self.new_ids.populate_blocks(&reachable_blocks, old_function, new_function); + + // Map each parameter, instruction, and terminator + for old_block_id in reachable_blocks { + let new_block_id = self.new_ids.blocks[&old_block_id]; + + let old_block = &mut old_function.dfg[old_block_id]; + for old_instruction_id in old_block.take_instructions() { + let instruction = old_function.dfg[old_instruction_id] + .map_values(|value| self.new_ids.map_value(new_function, old_function, value)); + + let call_stack = old_function.dfg.get_call_stack(old_instruction_id); + let old_results = old_function.dfg.instruction_results(old_instruction_id); + + let ctrl_typevars = instruction + .requires_ctrl_typevars() + .then(|| vecmap(old_results, |result| old_function.dfg.type_of_value(*result))); + + let new_results = new_function.dfg.insert_instruction_and_results( + instruction, + new_block_id, + ctrl_typevars, + call_stack, + ); + + assert_eq!(old_results.len(), new_results.len()); + for (old_result, new_result) in old_results.iter().zip(new_results.results().iter()) + { + let old_result = old_function.dfg.resolve(*old_result); + self.new_ids.values.insert(old_result, *new_result); + } + } + + let old_block = &mut old_function.dfg[old_block_id]; + let mut terminator = old_block + .take_terminator() + .map_values(|value| self.new_ids.map_value(new_function, old_function, value)); + terminator.mutate_blocks(|old_block| self.new_ids.blocks[&old_block]); + new_function.dfg.set_block_terminator(new_block_id, terminator); + } + } +} + +impl IdMaps { + fn populate_blocks( + &mut self, + reachable_blocks: &[BasicBlockId], + old_function: &mut Function, + new_function: &mut Function, + ) { + let old_entry = old_function.entry_block(); + self.blocks.insert(old_entry, new_function.entry_block()); + + for old_id in reachable_blocks { + if *old_id != old_entry { + let new_id = new_function.dfg.make_block(); + self.blocks.insert(*old_id, new_id); + } + + let new_id = self.blocks[old_id]; + let old_block = &mut old_function.dfg[*old_id]; + for old_parameter in old_block.take_parameters() { + let old_parameter = old_function.dfg.resolve(old_parameter); + let typ = old_function.dfg.type_of_value(old_parameter); + let new_parameter = new_function.dfg.add_block_parameter(new_id, typ); + self.values.insert(old_parameter, new_parameter); + } + } + } + + fn map_value( + &mut self, + new_function: &mut Function, + old_function: &Function, + old_value: ValueId, + ) -> ValueId { + let old_value = old_function.dfg.resolve(old_value); + match &old_function.dfg[old_value] { + value @ Value::Instruction { instruction, .. } => { + *self.values.get(&old_value).unwrap_or_else(|| { + let instruction = &old_function.dfg[*instruction]; + unreachable!("Unmapped value with id {old_value}: {value:?}\n from instruction: {instruction:?}, SSA: {old_function}") + }) + } + + value @ Value::Param { .. } => { + *self.values.get(&old_value).unwrap_or_else(|| { + unreachable!("Unmapped value with id {old_value}: {value:?}") + }) + } + + Value::Function(id) => { + let new_id = self.function_ids[id]; + new_function.dfg.import_function(new_id) + } + + Value::NumericConstant { constant, typ } => { + new_function.dfg.make_constant(*constant, typ.clone()) + } + Value::Array { array, typ } => { + if let Some(value) = self.values.get(&old_value) { + return *value; + } + + let array = array + .iter() + .map(|value| self.map_value(new_function, old_function, *value)) + .collect(); + let new_value = new_function.dfg.make_array(array, typ.clone()); + self.values.insert(old_value, new_value); + new_value + } + Value::Intrinsic(intrinsic) => new_function.dfg.import_intrinsic(*intrinsic), + Value::ForeignFunction(name) => new_function.dfg.import_foreign_function(name), + } + } +} From 1737b656c861706c38b59bd5ef6cd095687a2898 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Wed, 4 Sep 2024 08:37:31 -0400 Subject: [PATCH 02/25] feat(perf): Remove last store in return block if last load is before that store (#5910) # Description ## Problem\* Working towards #4535 ## Summary\* Small optimization found while working in mem2reg. Now that we have a small per function state we can see which stores/loads that are leftover can be removed. A place we are potentially missing stores that can be removed is in return blocks. If we have a store inside of a return block that is not loaded from inside that block, we can safely remove that store (given it is not a reference param). ## Additional Context ## Documentation\* Check one: - [X] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [X] I have tested the changes locally. - [X] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: Tom French <15848336+TomAFrench@users.noreply.github.com> Co-authored-by: TomAFrench --- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 32 ++++++++++++++++--- 1 file changed, 27 insertions(+), 5 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 9d6582c0db7..3d98f4126cf 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -116,7 +116,7 @@ struct PerFunctionContext<'f> { /// Track a value's last load across all blocks. /// If a value is not used in anymore loads we can remove the last store to that value. - last_loads: HashMap, + last_loads: HashMap, } impl<'f> PerFunctionContext<'f> { @@ -152,9 +152,31 @@ impl<'f> PerFunctionContext<'f> { // This rule does not apply to reference parameters, which we must also check for before removing these stores. for (block_id, block) in self.blocks.iter() { let block_params = self.inserter.function.dfg.block_parameters(*block_id); - for (value, store_instruction) in block.last_stores.iter() { - let is_reference_param = block_params.contains(value); - if self.last_loads.get(value).is_none() && !is_reference_param { + for (store_address, store_instruction) in block.last_stores.iter() { + let is_reference_param = block_params.contains(store_address); + let terminator = self.inserter.function.dfg[*block_id].unwrap_terminator(); + + let is_return = matches!(terminator, TerminatorInstruction::Return { .. }); + let remove_load = if is_return { + // Determine whether the last store is used in the return value + let mut is_return_value = false; + terminator.for_each_value(|return_value| { + is_return_value = return_value == *store_address || is_return_value; + }); + + // If the last load of a store is not part of the block with a return terminator, + // we can safely remove this store. + let last_load_not_in_return = self + .last_loads + .get(store_address) + .map(|(_, last_load_block)| *last_load_block != *block_id) + .unwrap_or(true); + !is_return_value && last_load_not_in_return + } else { + self.last_loads.get(store_address).is_none() + }; + + if remove_load && !is_reference_param { self.instructions_to_remove.insert(*store_instruction); } } @@ -259,7 +281,7 @@ impl<'f> PerFunctionContext<'f> { } else { references.mark_value_used(address, self.inserter.function); - self.last_loads.insert(address, instruction); + self.last_loads.insert(address, (instruction, block_id)); } } Instruction::Store { address, value } => { From 779e013147cecc6c6bd13492cd60c4e26b7a7113 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Wed, 4 Sep 2024 14:17:34 +0100 Subject: [PATCH 03/25] chore: remove equality operation on boolean constraints against constants (#5919) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit … # Description ## Problem\* Resolves ## Summary\* We're currently performing a `x == 1` equality and then constraining the result to be `1` when dealing with boolean values in brillig. This PR updates the codegen to just act on `x` directly. ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../src/brillig/brillig_gen/brillig_block.rs | 44 +++++++++++++++---- 1 file changed, 35 insertions(+), 9 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 26abafe177f..ef5fbce83d4 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -255,16 +255,40 @@ impl<'block> BrilligBlock<'block> { self.convert_ssa_binary(binary, dfg, result_var); } Instruction::Constrain(lhs, rhs, assert_message) => { - let condition = SingleAddrVariable { - address: self.brillig_context.allocate_register(), - bit_size: 1, + let (condition, deallocate) = match ( + dfg.get_numeric_constant_with_type(*lhs), + dfg.get_numeric_constant_with_type(*rhs), + ) { + // If the constraint is of the form `x == u1 1` then we can simply constrain `x` directly + ( + Some((constant, Type::Numeric(NumericType::Unsigned { bit_size: 1 }))), + None, + ) if constant == FieldElement::one() => { + (self.convert_ssa_single_addr_value(*rhs, dfg), false) + } + ( + None, + Some((constant, Type::Numeric(NumericType::Unsigned { bit_size: 1 }))), + ) if constant == FieldElement::one() => { + (self.convert_ssa_single_addr_value(*lhs, dfg), false) + } + + // Otherwise we need to perform the equality explicitly. + _ => { + let condition = SingleAddrVariable { + address: self.brillig_context.allocate_register(), + bit_size: 1, + }; + self.convert_ssa_binary( + &Binary { lhs: *lhs, rhs: *rhs, operator: BinaryOp::Eq }, + dfg, + condition, + ); + + (condition, true) + } }; - self.convert_ssa_binary( - &Binary { lhs: *lhs, rhs: *rhs, operator: BinaryOp::Eq }, - dfg, - condition, - ); match assert_message { Some(ConstrainError::Dynamic(selector, values)) => { let payload_values = @@ -287,7 +311,9 @@ impl<'block> BrilligBlock<'block> { self.brillig_context.codegen_constrain(condition, None); } } - self.brillig_context.deallocate_single_addr(condition); + if deallocate { + self.brillig_context.deallocate_single_addr(condition); + } } Instruction::Allocate => { let result_value = dfg.instruction_results(instruction_id)[0]; From 44cf9a2140bc06b550d4b46966f1637598ac11a7 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 4 Sep 2024 12:15:10 -0300 Subject: [PATCH 04/25] fix: prevent comptime println from crashing LSP (#5918) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description ## Problem Resolves #5904 ## Summary I found out that writing to the error stream when connected to the LSP client actually sends that output to the LSP output window ("Noir Language Server") in our case so I chose to do that for comptime println/print. Advantages: - No more crashing of the LSP server on comptime println - If you know it goes to that output you can actually see the output without having to go to a terminal to run that code, so debugging comptime will be pretty fast now - (if you don't know the output goes there, it's fine too: at least the LSP server doesn't crash 😄) ![lsp-comptime-println](https://github.com/user-attachments/assets/7d3ecaca-00fb-4c15-90a0-9868260ce7f5) ## Additional Context Though the original issue was about using sockets to communicate with the LSP client, I think we won't need that after all if comptime println was the only issue (and I think most LSP servers use stdin/stdout for communication because it's simpler). Also: it would be better if this output showed up in a dedicated output window, say "Noir Language comptime output" or something like that, but only the LSP client can create those windows and write to them... so implementing that would mean somehow communicating this data from the server to the client (maybe a socket?), which is much harder to do... so for now the output will have to show up like regular logs. And a question: when the LSP server starts we have this code: ```rust eprintln!("LSP starting..."); ``` that shows up on the output window... should we remove it? It feel like it was used to debug the server to know that it started, but I don't know how useful it is now. ## Documentation Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../src/hir/comptime/interpreter.rs | 16 +++++++++++++--- tooling/nargo_cli/src/cli/lsp_cmd.rs | 2 -- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index 4980045c68d..d8e62b66eca 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs @@ -1653,10 +1653,20 @@ impl<'local, 'interner> Interpreter<'local, 'interner> { assert_eq!(arguments.len(), 2); let print_newline = arguments[0].0 == Value::Bool(true); - if print_newline { - println!("{}", arguments[1].0.display(self.elaborator.interner)); + let contents = arguments[1].0.display(self.elaborator.interner); + if self.elaborator.interner.is_in_lsp_mode() { + // If we `println!` in LSP it gets mixed with the protocol stream and leads to crashing + // the connection. If we use `eprintln!` not only it doesn't crash, but the output + // appears in the "Noir Language Server" output window in case you want to see it. + if print_newline { + eprintln!("{}", contents); + } else { + eprint!("{}", contents); + } + } else if print_newline { + println!("{}", contents); } else { - print!("{}", arguments[1].0.display(self.elaborator.interner)); + print!("{}", contents); } Ok(Value::Unit) diff --git a/tooling/nargo_cli/src/cli/lsp_cmd.rs b/tooling/nargo_cli/src/cli/lsp_cmd.rs index 9ff7a42e5f5..bfaa913b33a 100644 --- a/tooling/nargo_cli/src/cli/lsp_cmd.rs +++ b/tooling/nargo_cli/src/cli/lsp_cmd.rs @@ -35,8 +35,6 @@ pub(crate) fn run(_args: LspCommand, _config: NargoConfig) -> Result<(), CliErro .service(router) }); - eprintln!("LSP starting..."); - // Prefer truly asynchronous piped stdin/stdout without blocking tasks. #[cfg(unix)] let (stdin, stdout) = ( From d2caa5bb86f944d6d09182482bef6e35ca2213d6 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 4 Sep 2024 14:09:54 -0300 Subject: [PATCH 05/25] feat: LSP will now suggest private items if they are visible (#5923) # Description ## Problem Resolves #5879 ## Summary Uses the existing visibility check instead of just considering private items to never be visible. ## Additional Context None. ## Documentation Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../noirc_frontend/src/elaborator/comptime.rs | 5 +--- .../src/hir/def_collector/dc_mod.rs | 5 +--- tooling/lsp/src/modules.rs | 5 ++-- .../requests/code_action/import_or_qualify.rs | 1 + tooling/lsp/src/requests/completion.rs | 22 ++------------- .../src/requests/completion/auto_import.rs | 1 + tooling/lsp/src/requests/completion/tests.rs | 25 +++++++++++++++++ tooling/lsp/src/visibility.rs | 28 +++++++++++++------ 8 files changed, 54 insertions(+), 38 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/comptime.rs b/compiler/noirc_frontend/src/elaborator/comptime.rs index baa9c0ab371..5678fa8ddee 100644 --- a/compiler/noirc_frontend/src/elaborator/comptime.rs +++ b/compiler/noirc_frontend/src/elaborator/comptime.rs @@ -269,10 +269,7 @@ impl<'context> Elaborator<'context> { let module = self.module_id(); self.interner.push_function(id, &function.def, module, location); - if self.interner.is_in_lsp_mode() - && !function.def.is_test() - && !function.def.is_private() - { + if self.interner.is_in_lsp_mode() && !function.def.is_test() { self.interner.register_function(id, &function.def); } diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 590cdc541ce..1dbd5a1383b 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -239,10 +239,7 @@ impl<'a> ModCollector<'a> { let location = Location::new(function.span(), self.file_id); context.def_interner.push_function(func_id, &function.def, module, location); - if context.def_interner.is_in_lsp_mode() - && !function.def.is_test() - && !function.def.is_private() - { + if context.def_interner.is_in_lsp_mode() && !function.def.is_test() { context.def_interner.register_function(func_id, &function.def); } diff --git a/tooling/lsp/src/modules.rs b/tooling/lsp/src/modules.rs index 54074dbd94c..d78da15a8ff 100644 --- a/tooling/lsp/src/modules.rs +++ b/tooling/lsp/src/modules.rs @@ -47,10 +47,11 @@ pub(crate) fn module_full_path( current_module_id: ModuleId, current_module_parent_id: Option, interner: &NodeInterner, + def_maps: &BTreeMap, ) -> Option { let full_path; if let ModuleDefId::ModuleId(module_id) = module_def_id { - if !is_visible(visibility, current_module_id, module_id) { + if !is_visible(module_id, current_module_id, visibility, def_maps) { return None; } @@ -61,7 +62,7 @@ pub(crate) fn module_full_path( return None; }; - if !is_visible(visibility, current_module_id, parent_module) { + if !is_visible(parent_module, current_module_id, visibility, def_maps) { return None; } diff --git a/tooling/lsp/src/requests/code_action/import_or_qualify.rs b/tooling/lsp/src/requests/code_action/import_or_qualify.rs index d07d117a317..25accc8a008 100644 --- a/tooling/lsp/src/requests/code_action/import_or_qualify.rs +++ b/tooling/lsp/src/requests/code_action/import_or_qualify.rs @@ -52,6 +52,7 @@ impl<'a> CodeActionFinder<'a> { self.module_id, current_module_parent_id, self.interner, + self.def_maps, ) else { continue; }; diff --git a/tooling/lsp/src/requests/completion.rs b/tooling/lsp/src/requests/completion.rs index 987746d37ce..59758f4b972 100644 --- a/tooling/lsp/src/requests/completion.rs +++ b/tooling/lsp/src/requests/completion.rs @@ -22,10 +22,7 @@ use noirc_frontend::{ UnresolvedGenerics, UnresolvedType, UseTree, UseTreeKind, Visitor, }, graph::{CrateId, Dependency}, - hir::{ - def_map::{CrateDefMap, LocalModuleId, ModuleId}, - resolution::import::can_reference_module_id, - }, + hir::def_map::{CrateDefMap, LocalModuleId, ModuleId}, hir_def::traits::Trait, macros_api::{ModuleDefId, NodeInterner}, node_interner::ReferenceId, @@ -34,7 +31,7 @@ use noirc_frontend::{ }; use sort_text::underscore_sort_text; -use crate::{requests::to_lsp_location, utils, LspState}; +use crate::{requests::to_lsp_location, utils, visibility::is_visible, LspState}; use super::process_request; @@ -1263,21 +1260,6 @@ fn module_def_id_from_reference_id(reference_id: ReferenceId) -> Option, -) -> bool { - can_reference_module_id( - def_maps, - current_module_id.krate, - current_module_id.local_id, - target_module_id, - visibility, - ) -} - #[cfg(test)] mod completion_name_matches_tests { use crate::requests::completion::name_matches; diff --git a/tooling/lsp/src/requests/completion/auto_import.rs b/tooling/lsp/src/requests/completion/auto_import.rs index bbd471dfea1..d8823794999 100644 --- a/tooling/lsp/src/requests/completion/auto_import.rs +++ b/tooling/lsp/src/requests/completion/auto_import.rs @@ -53,6 +53,7 @@ impl<'a> NodeFinder<'a> { self.module_id, current_module_parent_id, self.interner, + self.def_maps, ) else { continue; }; diff --git a/tooling/lsp/src/requests/completion/tests.rs b/tooling/lsp/src/requests/completion/tests.rs index d621ca21bb8..ca959f5d5ca 100644 --- a/tooling/lsp/src/requests/completion/tests.rs +++ b/tooling/lsp/src/requests/completion/tests.rs @@ -1863,4 +1863,29 @@ mod completion_tests { Some("(use bar::foobar)".to_string()), ); } + + #[test] + async fn test_auto_import_suggests_private_function_if_visibile() { + let src = r#" + mod foo { + fn qux() { + barba>|< + } + } + + fn barbaz() {} + + fn main() {} + "#; + + let items = get_completions(src).await; + assert_eq!(items.len(), 1); + + let item = &items[0]; + assert_eq!(item.label, "barbaz()"); + assert_eq!( + item.label_details.as_ref().unwrap().detail, + Some("(use super::barbaz)".to_string()), + ); + } } diff --git a/tooling/lsp/src/visibility.rs b/tooling/lsp/src/visibility.rs index aad8b47fbbe..d6e26f7bc48 100644 --- a/tooling/lsp/src/visibility.rs +++ b/tooling/lsp/src/visibility.rs @@ -1,13 +1,25 @@ -use noirc_frontend::{ast::ItemVisibility, hir::def_map::ModuleId}; +use std::collections::BTreeMap; + +use noirc_frontend::{ + ast::ItemVisibility, + graph::CrateId, + hir::{ + def_map::{CrateDefMap, ModuleId}, + resolution::import::can_reference_module_id, + }, +}; pub(super) fn is_visible( + target_module_id: ModuleId, + current_module_id: ModuleId, visibility: ItemVisibility, - current_module: ModuleId, - target_module: ModuleId, + def_maps: &BTreeMap, ) -> bool { - match visibility { - ItemVisibility::Public => true, - ItemVisibility::Private => false, - ItemVisibility::PublicCrate => current_module.krate == target_module.krate, - } + can_reference_module_id( + def_maps, + current_module_id.krate, + current_module_id.local_id, + target_module_id, + visibility, + ) } From 91f693d81edb1913bf56d2c1038441cec5844646 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 4 Sep 2024 14:46:57 -0300 Subject: [PATCH 06/25] feat: check argument count and types on attribute function callback (#5921) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description ## Problem Resolves #5903 ## Summary Now the compiler will check that attribute function callbacks have at least one argument, that that argument's type matches the corresponding type, and that remaining arguments also match the types given. Also previously errors on these callbacks were shown on the function that had the attribute, instead of on the attribute, likely because attributes didn't have a Span attached to them: this PR adds that too. ## Additional Context The error message is still a bit strange because if you have code like this: ```rust #[attr] fn foo() {} fn attr() {} fn main() {} ``` You get this: ``` error: Expected 0 arguments, but 1 was provided ┌─ src/main.nr:1:3 │ 1 │ #[attr] │ ---- Too many arguments ``` which kind of makes sense, because 1 implicit argument was provided but 0 are expected in the callback, but maybe the error should point out that the callback actually needs one argument. Let me know if you think we should improve the error message here... but at least it doesn't error anymore. Oh, I remember why I didn't improve that error message: the error should likely be on the callback function, but it should point out that the error happens because of a given attribute, so we need two different locations for the error, which I think we currently doesn't support. ## Documentation Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- aztec_macros/src/utils/ast_utils.rs | 4 +- compiler/noirc_driver/src/lib.rs | 10 +- compiler/noirc_errors/src/position.rs | 4 + .../noirc_frontend/src/elaborator/comptime.rs | 94 ++++++++++++++----- compiler/noirc_frontend/src/elaborator/mod.rs | 3 +- .../noirc_frontend/src/elaborator/types.rs | 8 +- .../src/hir/comptime/interpreter/builtin.rs | 2 +- .../noirc_frontend/src/hir_def/function.rs | 3 +- compiler/noirc_frontend/src/hir_def/types.rs | 18 +--- compiler/noirc_frontend/src/lexer/lexer.rs | 19 +++- compiler/noirc_frontend/src/lexer/token.rs | 36 +++++-- 11 files changed, 138 insertions(+), 63 deletions(-) diff --git a/aztec_macros/src/utils/ast_utils.rs b/aztec_macros/src/utils/ast_utils.rs index 19372fa5cb5..316aa60da62 100644 --- a/aztec_macros/src/utils/ast_utils.rs +++ b/aztec_macros/src/utils/ast_utils.rs @@ -187,8 +187,8 @@ pub fn check_trait_method_implemented(trait_impl: &NoirTraitImpl, method_name: & /// Checks if an attribute is a custom attribute with a specific name pub fn is_custom_attribute(attr: &SecondaryAttribute, attribute_name: &str) -> bool { - if let SecondaryAttribute::Custom(custom_attr) = attr { - custom_attr.as_str() == attribute_name + if let SecondaryAttribute::Custom(custom_attribute) = attr { + custom_attribute.contents.as_str() == attribute_name } else { false } diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 88918151366..a315e7ed397 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -452,9 +452,13 @@ fn compile_contract_inner( .attributes .secondary .iter() - .filter_map( - |attr| if let SecondaryAttribute::Custom(tag) = attr { Some(tag) } else { None }, - ) + .filter_map(|attr| { + if let SecondaryAttribute::Custom(attribute) = attr { + Some(&attribute.contents) + } else { + None + } + }) .cloned() .collect(); diff --git a/compiler/noirc_errors/src/position.rs b/compiler/noirc_errors/src/position.rs index 1792197eab7..9b031f56ae2 100644 --- a/compiler/noirc_errors/src/position.rs +++ b/compiler/noirc_errors/src/position.rs @@ -103,6 +103,10 @@ impl Span { let other_distance = other.end() - other.start(); self_distance < other_distance } + + pub fn shift_by(&self, offset: u32) -> Span { + Self::from(self.start() + offset..self.end() + offset) + } } impl From for Range { diff --git a/compiler/noirc_frontend/src/elaborator/comptime.rs b/compiler/noirc_frontend/src/elaborator/comptime.rs index 5678fa8ddee..0c1bc82e1ce 100644 --- a/compiler/noirc_frontend/src/elaborator/comptime.rs +++ b/compiler/noirc_frontend/src/elaborator/comptime.rs @@ -24,7 +24,7 @@ use crate::{ }, node_interner::{DefinitionKind, DependencyId, FuncId, TraitId}, parser::{self, TopLevelStatement}, - Type, TypeBindings, + Type, TypeBindings, UnificationError, }; use super::{Elaborator, FunctionContext, ResolverMeta}; @@ -96,10 +96,14 @@ impl<'context> Elaborator<'context> { generated_items: &mut CollectedItems, ) { for attribute in attributes { - if let SecondaryAttribute::Custom(name) = attribute { - if let Err(error) = - self.run_comptime_attribute_on_item(name, item.clone(), span, generated_items) - { + if let SecondaryAttribute::Custom(attribute) = attribute { + if let Err(error) = self.run_comptime_attribute_on_item( + &attribute.contents, + item.clone(), + span, + attribute.contents_span, + generated_items, + ) { self.errors.push(error); } } @@ -111,10 +115,11 @@ impl<'context> Elaborator<'context> { attribute: &str, item: Value, span: Span, + attribute_span: Span, generated_items: &mut CollectedItems, ) -> Result<(), (CompilationError, FileId)> { - let location = Location::new(span, self.file); - let Some((function, arguments)) = Self::parse_attribute(attribute, self.file)? else { + let location = Location::new(attribute_span, self.file); + let Some((function, arguments)) = Self::parse_attribute(attribute, location)? else { // Do not issue an error if the attribute is unknown return Ok(()); }; @@ -141,12 +146,17 @@ impl<'context> Elaborator<'context> { }; let mut interpreter = self.setup_interpreter(); - let mut arguments = - Self::handle_attribute_arguments(&mut interpreter, function, arguments, location) - .map_err(|error| { - let file = error.get_location().file; - (error.into(), file) - })?; + let mut arguments = Self::handle_attribute_arguments( + &mut interpreter, + &item, + function, + arguments, + location, + ) + .map_err(|error| { + let file = error.get_location().file; + (error.into(), file) + })?; arguments.insert(0, (item, location)); @@ -170,33 +180,62 @@ impl<'context> Elaborator<'context> { #[allow(clippy::type_complexity)] pub(crate) fn parse_attribute( annotation: &str, - file: FileId, + location: Location, ) -> Result)>, (CompilationError, FileId)> { let (tokens, mut lexing_errors) = Lexer::lex(annotation); if !lexing_errors.is_empty() { - return Err((lexing_errors.swap_remove(0).into(), file)); + return Err((lexing_errors.swap_remove(0).into(), location.file)); } let expression = parser::expression() .parse(tokens) - .map_err(|mut errors| (errors.swap_remove(0).into(), file))?; + .map_err(|mut errors| (errors.swap_remove(0).into(), location.file))?; - Ok(match expression.kind { - ExpressionKind::Call(call) => Some((*call.func, call.arguments)), - ExpressionKind::Variable(_) => Some((expression, Vec::new())), - _ => None, - }) + let (mut func, mut arguments) = match expression.kind { + ExpressionKind::Call(call) => (*call.func, call.arguments), + ExpressionKind::Variable(_) => (expression, Vec::new()), + _ => return Ok(None), + }; + + func.span = func.span.shift_by(location.span.start()); + + for argument in &mut arguments { + argument.span = argument.span.shift_by(location.span.start()); + } + + Ok(Some((func, arguments))) } fn handle_attribute_arguments( interpreter: &mut Interpreter, + item: &Value, function: FuncId, arguments: Vec, location: Location, ) -> Result, InterpreterError> { let meta = interpreter.elaborator.interner.function_meta(&function); + let mut parameters = vecmap(&meta.parameters.0, |(_, typ, _)| typ.clone()); + if parameters.is_empty() { + return Err(InterpreterError::ArgumentCountMismatch { + expected: 0, + actual: arguments.len() + 1, + location, + }); + } + + let expected_type = item.get_type(); + let expected_type = expected_type.as_ref(); + + if ¶meters[0] != expected_type { + return Err(InterpreterError::TypeMismatch { + expected: parameters[0].clone(), + actual: expected_type.clone(), + location, + }); + } + // Remove the initial parameter for the comptime item since that is not included // in `arguments` at this point. parameters.remove(0); @@ -213,6 +252,7 @@ impl<'context> Elaborator<'context> { let mut varargs = im::Vector::new(); for (i, arg) in arguments.into_iter().enumerate() { + let arg_location = Location::new(arg.span, location.file); let param_type = parameters.get(i).or(varargs_elem_type).unwrap_or(&Type::Error); let mut push_arg = |arg| { @@ -233,9 +273,17 @@ impl<'context> Elaborator<'context> { }?; push_arg(Value::TraitDefinition(trait_id)); } else { - let expr_id = interpreter.elaborator.elaborate_expression(arg).0; + let (expr_id, expr_type) = interpreter.elaborator.elaborate_expression(arg); push_arg(interpreter.evaluate(expr_id)?); - } + + if let Err(UnificationError) = expr_type.unify(param_type) { + return Err(InterpreterError::TypeMismatch { + expected: param_type.clone(), + actual: expr_type, + location: arg_location, + }); + } + }; } if is_varargs { diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index e84ed76050d..44240b72af0 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -28,6 +28,7 @@ use crate::{ node_interner::{ DefinitionKind, DependencyId, ExprId, FuncId, GlobalId, ReferenceId, TraitId, TypeAliasId, }, + token::CustomAtrribute, Shared, Type, TypeVariable, }; use crate::{ @@ -819,7 +820,7 @@ impl<'context> Elaborator<'context> { let attributes = func.secondary_attributes().iter(); let attributes = attributes.filter_map(|secondary_attribute| secondary_attribute.as_custom()); - let attributes = attributes.map(|str| str.to_string()).collect(); + let attributes: Vec = attributes.cloned().collect(); let meta = FuncMeta { name: name_ident, diff --git a/compiler/noirc_frontend/src/elaborator/types.rs b/compiler/noirc_frontend/src/elaborator/types.rs index e41234a5be5..8dccd5f0344 100644 --- a/compiler/noirc_frontend/src/elaborator/types.rs +++ b/compiler/noirc_frontend/src/elaborator/types.rs @@ -36,7 +36,7 @@ use crate::{ TraitImplKind, TraitMethodId, }, Generics, Kind, ResolvedGeneric, Type, TypeBinding, TypeBindings, TypeVariable, - TypeVariableKind, + TypeVariableKind, UnificationError, }; use super::{lints, Elaborator}; @@ -713,9 +713,9 @@ impl<'context> Elaborator<'context> { expected: &Type, make_error: impl FnOnce() -> TypeCheckError, ) { - let mut errors = Vec::new(); - actual.unify(expected, &mut errors, make_error); - self.errors.extend(errors.into_iter().map(|error| (error.into(), self.file))); + if let Err(UnificationError) = actual.unify(expected) { + self.errors.push((make_error().into(), self.file)); + } } /// Wrapper of Type::unify_with_coercions using self.errors diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index 070749e45ba..36e6fd014d5 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -1609,7 +1609,7 @@ fn function_def_has_named_attribute( let name = name.iter().map(|token| token.to_string()).collect::>().join(""); for attribute in attributes { - let parse_result = Elaborator::parse_attribute(attribute, location.file); + let parse_result = Elaborator::parse_attribute(&attribute.contents, location); let Ok(Some((function, _arguments))) = parse_result else { continue; }; diff --git a/compiler/noirc_frontend/src/hir_def/function.rs b/compiler/noirc_frontend/src/hir_def/function.rs index 7fa33746f31..8e3baef1d00 100644 --- a/compiler/noirc_frontend/src/hir_def/function.rs +++ b/compiler/noirc_frontend/src/hir_def/function.rs @@ -10,6 +10,7 @@ use crate::graph::CrateId; use crate::hir::def_map::LocalModuleId; use crate::macros_api::{BlockExpression, StructId}; use crate::node_interner::{ExprId, NodeInterner, TraitId, TraitImplId}; +use crate::token::CustomAtrribute; use crate::{ResolvedGeneric, Type}; /// A Hir function is a block expression with a list of statements. @@ -166,7 +167,7 @@ pub struct FuncMeta { pub self_type: Option, /// Custom attributes attached to this function. - pub custom_attributes: Vec, + pub custom_attributes: Vec, } #[derive(Debug, Clone)] diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index 638003d3fcd..7b3d0d7a205 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/compiler/noirc_frontend/src/hir_def/types.rs @@ -1467,21 +1467,13 @@ impl Type { /// equal to the other type in the process. When comparing types, unification /// (including try_unify) are almost always preferred over Type::eq as unification /// will correctly handle generic types. - pub fn unify( - &self, - expected: &Type, - errors: &mut Vec, - make_error: impl FnOnce() -> TypeCheckError, - ) { + pub fn unify(&self, expected: &Type) -> Result<(), UnificationError> { let mut bindings = TypeBindings::new(); - match self.try_unify(expected, &mut bindings) { - Ok(()) => { - // Commit any type bindings on success - Self::apply_type_bindings(bindings); - } - Err(UnificationError) => errors.push(make_error()), - } + self.try_unify(expected, &mut bindings).map(|()| { + // Commit any type bindings on success + Self::apply_type_bindings(bindings); + }) } /// `try_unify` is a bit of a misnomer since although errors are not committed, diff --git a/compiler/noirc_frontend/src/lexer/lexer.rs b/compiler/noirc_frontend/src/lexer/lexer.rs index 0afcb02caac..7265593238d 100644 --- a/compiler/noirc_frontend/src/lexer/lexer.rs +++ b/compiler/noirc_frontend/src/lexer/lexer.rs @@ -295,8 +295,12 @@ impl<'a> Lexer<'a> { } self.next_char(); + let contents_start = self.position + 1; + let word = self.eat_while(None, |ch| ch != ']'); + let contents_end = self.position; + if !self.peek_char_is(']') { return Err(LexerErrorKind::UnexpectedCharacter { span: Span::single_char(self.position), @@ -308,7 +312,10 @@ impl<'a> Lexer<'a> { let end = self.position; - let attribute = Attribute::lookup_attribute(&word, Span::inclusive(start, end))?; + let span = Span::inclusive(start, end); + let contents_span = Span::inclusive(contents_start, contents_end); + + let attribute = Attribute::lookup_attribute(&word, span, contents_span)?; Ok(attribute.into_span(start, end)) } @@ -682,7 +689,7 @@ mod tests { use iter_extended::vecmap; use super::*; - use crate::token::{FunctionAttribute, SecondaryAttribute, TestScope}; + use crate::token::{CustomAtrribute, FunctionAttribute, SecondaryAttribute, TestScope}; #[test] fn test_single_double_char() { @@ -810,9 +817,11 @@ mod tests { let token = lexer.next_token().unwrap(); assert_eq!( token.token(), - &Token::Attribute(Attribute::Secondary(SecondaryAttribute::Custom( - "custom(hello)".to_string() - ))) + &Token::Attribute(Attribute::Secondary(SecondaryAttribute::Custom(CustomAtrribute { + contents: "custom(hello)".to_string(), + span: Span::from(0..16), + contents_span: Span::from(2..15) + }))) ); } diff --git a/compiler/noirc_frontend/src/lexer/token.rs b/compiler/noirc_frontend/src/lexer/token.rs index 585e22ce92b..1692908187e 100644 --- a/compiler/noirc_frontend/src/lexer/token.rs +++ b/compiler/noirc_frontend/src/lexer/token.rs @@ -697,7 +697,11 @@ impl fmt::Display for Attribute { impl Attribute { /// If the string is a fixed attribute return that, else /// return the custom attribute - pub(crate) fn lookup_attribute(word: &str, span: Span) -> Result { + pub(crate) fn lookup_attribute( + word: &str, + span: Span, + contents_span: Span, + ) -> Result { let word_segments: Vec<&str> = word .split(|c| c == '(' || c == ')') .filter(|string_segment| !string_segment.is_empty()) @@ -770,7 +774,11 @@ impl Attribute { ["varargs"] => Attribute::Secondary(SecondaryAttribute::Varargs), tokens => { tokens.iter().try_for_each(|token| validate(token))?; - Attribute::Secondary(SecondaryAttribute::Custom(word.to_owned())) + Attribute::Secondary(SecondaryAttribute::Custom(CustomAtrribute { + contents: word.to_owned(), + span, + contents_span, + })) } }; @@ -863,17 +871,26 @@ pub enum SecondaryAttribute { ContractLibraryMethod, Export, Field(String), - Custom(String), + Custom(CustomAtrribute), Abi(String), /// A variable-argument comptime function. Varargs, } +#[derive(PartialEq, Eq, Hash, Debug, Clone, PartialOrd, Ord)] +pub struct CustomAtrribute { + pub contents: String, + // The span of the entire attribute, including leading `#[` and trailing `]` + pub span: Span, + // The span for the attribute contents (what's inside `#[...]`) + pub contents_span: Span, +} + impl SecondaryAttribute { - pub(crate) fn as_custom(&self) -> Option<&str> { - if let Self::Custom(str) = self { - Some(str) + pub(crate) fn as_custom(&self) -> Option<&CustomAtrribute> { + if let Self::Custom(attribute) = self { + Some(attribute) } else { None } @@ -887,7 +904,7 @@ impl fmt::Display for SecondaryAttribute { SecondaryAttribute::Deprecated(Some(ref note)) => { write!(f, r#"#[deprecated("{note}")]"#) } - SecondaryAttribute::Custom(ref k) => write!(f, "#[{k}]"), + SecondaryAttribute::Custom(ref attribute) => write!(f, "#[{}]", attribute.contents), SecondaryAttribute::ContractLibraryMethod => write!(f, "#[contract_library_method]"), SecondaryAttribute::Export => write!(f, "#[export]"), SecondaryAttribute::Field(ref k) => write!(f, "#[field({k})]"), @@ -916,9 +933,8 @@ impl AsRef for SecondaryAttribute { match self { SecondaryAttribute::Deprecated(Some(string)) => string, SecondaryAttribute::Deprecated(None) => "", - SecondaryAttribute::Custom(string) - | SecondaryAttribute::Field(string) - | SecondaryAttribute::Abi(string) => string, + SecondaryAttribute::Custom(attribute) => &attribute.contents, + SecondaryAttribute::Field(string) | SecondaryAttribute::Abi(string) => string, SecondaryAttribute::ContractLibraryMethod => "", SecondaryAttribute::Export => "", SecondaryAttribute::Varargs => "", From 70ebb905da23a0541915a8f6883d6f530934be4e Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 4 Sep 2024 14:52:20 -0300 Subject: [PATCH 07/25] feat: unquote some value as tokens, not as unquote markers (#5924) # Description ## Problem Resolves #5916 ## Summary ## Additional Context I think there might be other `Value`s that we could convert into tokens, like `String`, but I didn't want to handle those in this PR (should it be `Str` or `RawStr`? etc.). And it's possible that we only need this special logic for integers anyway. ## Documentation Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../noirc_frontend/src/hir/comptime/value.rs | 37 +++++++++++++++++++ compiler/noirc_frontend/src/tests.rs | 28 ++++++++++++++ 2 files changed, 65 insertions(+) diff --git a/compiler/noirc_frontend/src/hir/comptime/value.rs b/compiler/noirc_frontend/src/hir/comptime/value.rs index c5818c20c57..fdac95a07fe 100644 --- a/compiler/noirc_frontend/src/hir/comptime/value.rs +++ b/compiler/noirc_frontend/src/hir/comptime/value.rs @@ -429,6 +429,9 @@ impl Value { location: Location, ) -> IResult> { let token = match self { + Value::Unit => { + return Ok(vec![Token::LeftParen, Token::RightParen]); + } Value::Quoted(tokens) => return Ok(unwrap_rc(tokens)), Value::Type(typ) => Token::QuotedType(interner.push_quoted_type(typ)), Value::Expr(ExprValue::Expression(expr)) => { @@ -443,6 +446,40 @@ impl Value { Value::UnresolvedType(typ) => { Token::InternedUnresolvedTypeData(interner.push_unresolved_type_data(typ)) } + Value::U1(bool) => Token::Bool(bool), + Value::U8(value) => Token::Int((value as u128).into()), + Value::U16(value) => Token::Int((value as u128).into()), + Value::U32(value) => Token::Int((value as u128).into()), + Value::U64(value) => Token::Int((value as u128).into()), + Value::I8(value) => { + if value < 0 { + return Ok(vec![Token::Minus, Token::Int((-value as u128).into())]); + } else { + Token::Int((value as u128).into()) + } + } + Value::I16(value) => { + if value < 0 { + return Ok(vec![Token::Minus, Token::Int((-value as u128).into())]); + } else { + Token::Int((value as u128).into()) + } + } + Value::I32(value) => { + if value < 0 { + return Ok(vec![Token::Minus, Token::Int((-value as u128).into())]); + } else { + Token::Int((value as u128).into()) + } + } + Value::I64(value) => { + if value < 0 { + return Ok(vec![Token::Minus, Token::Int((-value as u128).into())]); + } else { + Token::Int((value as u128).into()) + } + } + Value::Field(value) => Token::Int(value), other => Token::UnquoteMarker(other.into_hir_expression(interner, location)?), }; Ok(vec![token]) diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index a30907211a3..e7dbe11f0d1 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -3341,3 +3341,31 @@ fn warns_on_re_export_of_item_with_less_visibility() { ) )); } + +#[test] +fn unoquted_integer_as_integer_token() { + let src = r#" + trait Serialize { + fn serialize() {} + } + + #[attr] + fn foobar() {} + + fn attr(_f: FunctionDefinition) -> Quoted { + let serialized_len = 1; + // We are testing that when we unoqute $serialized_len, it's unquoted + // as the token `1` and not as something else that later won't be parsed correctly + // in the context of a generic argument. + quote { + impl Serialize<$serialized_len> for Field { + fn serialize() { } + } + } + } + + fn main() {} + "#; + + assert_no_errors(src); +} From 2ca2e5cf207a2a1f41ca86d877f0288bcbbfd212 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 4 Sep 2024 15:33:17 -0300 Subject: [PATCH 08/25] feat: module attributes (#5888) # Description ## Problem Resolves #5495 ## Summary Pending: - [ ] Decide whether to keep attributes as String or SecondaryAttribute in ModuleData - [ ] Parsing of module attributes is not ideal (it errors on non-secondary attributes, but I think not all secondary attributes are valid for modules... but maybe it's fine because struct attributes work the same way) ## Additional Context ## Documentation\* Check one: - [ ] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [ ] I have tested the changes locally. - [ ] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- aztec_macros/src/utils/parse_utils.rs | 1 + compiler/noirc_frontend/src/ast/statement.rs | 1 + compiler/noirc_frontend/src/ast/visitor.rs | 13 +++- .../noirc_frontend/src/elaborator/comptime.rs | 63 ++++++++++++---- compiler/noirc_frontend/src/elaborator/mod.rs | 7 +- .../src/hir/comptime/interpreter/builtin.rs | 33 +++++++++ .../noirc_frontend/src/hir/comptime/tests.rs | 8 +- .../src/hir/def_collector/dc_crate.rs | 18 +++++ .../src/hir/def_collector/dc_mod.rs | 74 ++++++++++++++++++- .../noirc_frontend/src/hir/def_map/mod.rs | 8 +- .../src/hir/def_map/module_data.rs | 20 ++++- compiler/noirc_frontend/src/lexer/errors.rs | 8 ++ compiler/noirc_frontend/src/lexer/lexer.rs | 38 +++++++++- compiler/noirc_frontend/src/lexer/token.rs | 11 ++- compiler/noirc_frontend/src/parser/mod.rs | 12 ++- compiler/noirc_frontend/src/parser/parser.rs | 56 +++++++++++--- .../src/parser/parser/attributes.rs | 9 +++ compiler/noirc_frontend/src/tests.rs | 23 +++++- .../docs/noir/standard_library/meta/module.md | 6 ++ noir_stdlib/src/meta/module.nr | 5 ++ .../comptime_module/src/main.nr | 45 +++++++++++ .../comptime_module/src/separate_module.nr | 5 ++ tooling/nargo_fmt/src/visitor/item.rs | 8 +- tooling/nargo_fmt/tests/expected/module.nr | 12 +++ tooling/nargo_fmt/tests/input/module.nr | 12 +++ 25 files changed, 454 insertions(+), 42 deletions(-) create mode 100644 test_programs/compile_success_empty/comptime_module/src/separate_module.nr diff --git a/aztec_macros/src/utils/parse_utils.rs b/aztec_macros/src/utils/parse_utils.rs index 6a2a876e682..e7b3e347a96 100644 --- a/aztec_macros/src/utils/parse_utils.rs +++ b/aztec_macros/src/utils/parse_utils.rs @@ -53,6 +53,7 @@ fn empty_item(item: &mut Item) { ItemKind::Import(use_tree, _) => empty_use_tree(use_tree), ItemKind::Struct(noir_struct) => empty_noir_struct(noir_struct), ItemKind::TypeAlias(noir_type_alias) => empty_noir_type_alias(noir_type_alias), + ItemKind::InnerAttribute(_) => (), } } diff --git a/compiler/noirc_frontend/src/ast/statement.rs b/compiler/noirc_frontend/src/ast/statement.rs index 2e14761a1cc..30db8ad63fd 100644 --- a/compiler/noirc_frontend/src/ast/statement.rs +++ b/compiler/noirc_frontend/src/ast/statement.rs @@ -292,6 +292,7 @@ pub trait Recoverable { #[derive(Debug, PartialEq, Eq, Clone)] pub struct ModuleDeclaration { pub ident: Ident, + pub outer_attributes: Vec, } impl std::fmt::Display for ModuleDeclaration { diff --git a/compiler/noirc_frontend/src/ast/visitor.rs b/compiler/noirc_frontend/src/ast/visitor.rs index 3955e50b03e..0aeeed39dd0 100644 --- a/compiler/noirc_frontend/src/ast/visitor.rs +++ b/compiler/noirc_frontend/src/ast/visitor.rs @@ -16,7 +16,7 @@ use crate::{ QuotedTypeId, }, parser::{Item, ItemKind, ParsedSubModule}, - token::Tokens, + token::{SecondaryAttribute, Tokens}, ParsedModule, QuotedType, }; @@ -432,6 +432,8 @@ pub trait Visitor { fn visit_struct_pattern(&mut self, _: &Path, _: &[(Ident, Pattern)], _: Span) -> bool { true } + + fn visit_secondary_attribute(&mut self, _: &SecondaryAttribute, _: Span) {} } impl ParsedModule { @@ -481,6 +483,9 @@ impl Item { ItemKind::ModuleDecl(module_declaration) => { module_declaration.accept(self.span, visitor); } + ItemKind::InnerAttribute(attribute) => { + attribute.accept(self.span, visitor); + } } } } @@ -1289,6 +1294,12 @@ impl Pattern { } } +impl SecondaryAttribute { + pub fn accept(&self, span: Span, visitor: &mut impl Visitor) { + visitor.visit_secondary_attribute(self, span); + } +} + fn visit_expressions(expressions: &[Expression], visitor: &mut impl Visitor) { for expression in expressions { expression.accept(visitor); diff --git a/compiler/noirc_frontend/src/elaborator/comptime.rs b/compiler/noirc_frontend/src/elaborator/comptime.rs index 0c1bc82e1ce..cfc2e34c520 100644 --- a/compiler/noirc_frontend/src/elaborator/comptime.rs +++ b/compiler/noirc_frontend/src/elaborator/comptime.rs @@ -10,11 +10,12 @@ use crate::{ comptime::{Interpreter, InterpreterError, Value}, def_collector::{ dc_crate::{ - CollectedItems, CompilationError, UnresolvedFunctions, UnresolvedStruct, - UnresolvedTrait, UnresolvedTraitImpl, + CollectedItems, CompilationError, ModuleAttribute, UnresolvedFunctions, + UnresolvedStruct, UnresolvedTrait, UnresolvedTraitImpl, }, dc_mod, }, + def_map::ModuleId, resolution::errors::ResolverError, }, hir_def::expr::HirIdent, @@ -96,21 +97,31 @@ impl<'context> Elaborator<'context> { generated_items: &mut CollectedItems, ) { for attribute in attributes { - if let SecondaryAttribute::Custom(attribute) = attribute { - if let Err(error) = self.run_comptime_attribute_on_item( - &attribute.contents, - item.clone(), - span, - attribute.contents_span, - generated_items, - ) { - self.errors.push(error); - } - } + self.run_comptime_attribute_on_item(attribute, &item, span, generated_items); } } fn run_comptime_attribute_on_item( + &mut self, + attribute: &SecondaryAttribute, + item: &Value, + span: Span, + generated_items: &mut CollectedItems, + ) { + if let SecondaryAttribute::Custom(attribute) = attribute { + if let Err(error) = self.run_comptime_attribute_name_on_item( + &attribute.contents, + item.clone(), + span, + attribute.contents_span, + generated_items, + ) { + self.errors.push(error); + } + } + } + + fn run_comptime_attribute_name_on_item( &mut self, attribute: &str, item: Value, @@ -383,7 +394,8 @@ impl<'context> Elaborator<'context> { | TopLevelStatement::Trait(_) | TopLevelStatement::Impl(_) | TopLevelStatement::TypeAlias(_) - | TopLevelStatement::SubModule(_) => { + | TopLevelStatement::SubModule(_) + | TopLevelStatement::InnerAttribute(_) => { let item = item.to_string(); let error = InterpreterError::UnsupportedTopLevelItemUnquote { item, location }; self.errors.push(error.into_compilation_error_pair()); @@ -422,6 +434,7 @@ impl<'context> Elaborator<'context> { traits: &BTreeMap, types: &BTreeMap, functions: &[UnresolvedFunctions], + module_attributes: &[ModuleAttribute], ) -> CollectedItems { let mut generated_items = CollectedItems::default(); @@ -444,9 +457,31 @@ impl<'context> Elaborator<'context> { } self.run_attributes_on_functions(functions, &mut generated_items); + + self.run_attributes_on_modules(module_attributes, &mut generated_items); + generated_items } + fn run_attributes_on_modules( + &mut self, + module_attributes: &[ModuleAttribute], + generated_items: &mut CollectedItems, + ) { + for module_attribute in module_attributes { + let local_id = module_attribute.module_id; + let module_id = ModuleId { krate: self.crate_id, local_id }; + let item = Value::ModuleDefinition(module_id); + let attribute = &module_attribute.attribute; + let span = Span::default(); + + self.local_module = module_attribute.attribute_module_id; + self.file = module_attribute.attribute_file_id; + + self.run_comptime_attribute_on_item(attribute, &item, span, generated_items); + } + } + fn run_attributes_on_functions( &mut self, function_sets: &[UnresolvedFunctions], diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 44240b72af0..161742029f6 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -321,7 +321,12 @@ impl<'context> Elaborator<'context> { // We have to run any comptime attributes on functions before the function is elaborated // since the generated items are checked beforehand as well. - let generated_items = self.run_attributes(&items.traits, &items.types, &items.functions); + let generated_items = self.run_attributes( + &items.traits, + &items.types, + &items.functions, + &items.module_attributes, + ); // After everything is collected, we can elaborate our generated items. // It may be better to inline these within `items` entirely since elaborating them diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index 36e6fd014d5..d1fcc76c55b 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -108,6 +108,7 @@ impl<'local, 'context> Interpreter<'local, 'context> { function_def_set_return_type(self, arguments, location) } "module_functions" => module_functions(self, arguments, location), + "module_has_named_attribute" => module_has_named_attribute(self, arguments, location), "module_is_contract" => module_is_contract(self, arguments, location), "module_name" => module_name(interner, arguments, location), "modulus_be_bits" => modulus_be_bits(interner, arguments, location), @@ -1816,6 +1817,38 @@ fn module_functions( Ok(Value::Slice(func_ids, slice_type)) } +// fn has_named_attribute(self, name: Quoted) -> bool +fn module_has_named_attribute( + interpreter: &Interpreter, + arguments: Vec<(Value, Location)>, + location: Location, +) -> IResult { + let (self_argument, name) = check_two_arguments(arguments, location)?; + let module_id = get_module(self_argument)?; + let module_data = interpreter.elaborator.get_module(module_id); + let name = get_quoted(name)?; + + let name = name.iter().map(|token| token.to_string()).collect::>().join(""); + + let attributes = module_data.outer_attributes.iter().chain(&module_data.inner_attributes); + for attribute in attributes { + let parse_result = Elaborator::parse_attribute(attribute, location); + let Ok(Some((function, _arguments))) = parse_result else { + continue; + }; + + let ExpressionKind::Variable(path) = function.kind else { + continue; + }; + + if path.last_name() == name { + return Ok(Value::Bool(true)); + } + } + + Ok(Value::Bool(false)) +} + // fn is_contract(self) -> bool fn module_is_contract( interpreter: &Interpreter, diff --git a/compiler/noirc_frontend/src/hir/comptime/tests.rs b/compiler/noirc_frontend/src/hir/comptime/tests.rs index 4c1adf9fca0..64b489422a0 100644 --- a/compiler/noirc_frontend/src/hir/comptime/tests.rs +++ b/compiler/noirc_frontend/src/hir/comptime/tests.rs @@ -23,7 +23,13 @@ fn interpret_helper(src: &str) -> Result { let module_id = LocalModuleId(Index::unsafe_zeroed()); let mut modules = noirc_arena::Arena::default(); let location = Location::new(Default::default(), file); - let root = LocalModuleId(modules.insert(ModuleData::new(None, location, false))); + let root = LocalModuleId(modules.insert(ModuleData::new( + None, + location, + Vec::new(), + Vec::new(), + false, + ))); assert_eq!(root, module_id); let file_manager = FileManager::new(&PathBuf::new()); diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 6a6cabe593d..98555375790 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -7,6 +7,7 @@ use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleId}; use crate::hir::resolution::errors::ResolverError; use crate::hir::resolution::path_resolver; use crate::hir::type_check::TypeCheckError; +use crate::token::SecondaryAttribute; use crate::{Generics, Type}; use crate::hir::resolution::import::{resolve_import, ImportDirective, PathResolution}; @@ -111,6 +112,21 @@ pub struct UnresolvedGlobal { pub stmt_def: LetStatement, } +pub struct ModuleAttribute { + // The file in which the module is defined + pub file_id: FileId, + // The module this attribute is attached to + pub module_id: LocalModuleId, + // The file where the attribute exists (it could be the same as `file_id` + // or a different one if it's an inner attribute in a different file) + pub attribute_file_id: FileId, + // The module where the attribute is defined (similar to `attribute_file_id`, + // it could be different than `module_id` for inner attributes) + pub attribute_module_id: LocalModuleId, + pub attribute: SecondaryAttribute, + pub is_inner: bool, +} + /// Given a Crate root, collect all definitions in that crate pub struct DefCollector { pub(crate) def_map: CrateDefMap, @@ -127,6 +143,7 @@ pub struct CollectedItems { pub globals: Vec, pub(crate) impls: ImplMap, pub(crate) trait_impls: Vec, + pub(crate) module_attributes: Vec, } impl CollectedItems { @@ -238,6 +255,7 @@ impl DefCollector { impls: HashMap::default(), globals: vec![], trait_impls: vec![], + module_attributes: vec![], }, } } diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 1dbd5a1383b..520cccf7580 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -17,6 +17,7 @@ use crate::ast::{ use crate::hir::resolution::errors::ResolverError; use crate::macros_api::{Expression, NodeInterner, UnresolvedType, UnresolvedTypeData}; use crate::node_interner::ModuleAttributes; +use crate::token::SecondaryAttribute; use crate::{ graph::CrateId, hir::def_collector::dc_crate::{UnresolvedStruct, UnresolvedTrait}, @@ -26,6 +27,7 @@ use crate::{ }; use crate::{Generics, Kind, ResolvedGeneric, Type, TypeVariable}; +use super::dc_crate::ModuleAttribute; use super::{ dc_crate::{ CompilationError, DefCollector, UnresolvedFunctions, UnresolvedGlobal, UnresolvedTraitImpl, @@ -63,8 +65,10 @@ pub fn collect_defs( for decl in ast.module_decls { errors.extend(collector.parse_module_declaration( context, - &decl, + decl, crate_id, + file_id, + module_id, macro_processors, )); } @@ -72,6 +76,7 @@ pub fn collect_defs( errors.extend(collector.collect_submodules( context, crate_id, + module_id, ast.submodules, file_id, macro_processors, @@ -102,10 +107,40 @@ pub fn collect_defs( collector.collect_impls(context, ast.impls, crate_id); + collector.collect_attributes( + ast.inner_attributes, + file_id, + module_id, + file_id, + module_id, + true, + ); + errors } impl<'a> ModCollector<'a> { + fn collect_attributes( + &mut self, + attributes: Vec, + file_id: FileId, + module_id: LocalModuleId, + attribute_file_id: FileId, + attribute_module_id: LocalModuleId, + is_inner: bool, + ) { + for attribute in attributes { + self.def_collector.items.module_attributes.push(ModuleAttribute { + file_id, + module_id, + attribute_file_id, + attribute_module_id, + attribute, + is_inner, + }); + } + } + fn collect_globals( &mut self, context: &mut Context, @@ -301,6 +336,8 @@ impl<'a> ModCollector<'a> { context, &name, Location::new(name.span(), self.file_id), + Vec::new(), + Vec::new(), false, false, ) { @@ -433,6 +470,8 @@ impl<'a> ModCollector<'a> { context, &name, Location::new(name.span(), self.file_id), + Vec::new(), + Vec::new(), false, false, ) { @@ -616,6 +655,7 @@ impl<'a> ModCollector<'a> { &mut self, context: &mut Context, crate_id: CrateId, + parent_module_id: LocalModuleId, submodules: Vec, file_id: FileId, macro_processors: &[&dyn MacroProcessor], @@ -626,10 +666,21 @@ impl<'a> ModCollector<'a> { context, &submodule.name, Location::new(submodule.name.span(), file_id), + submodule.outer_attributes.clone(), + submodule.contents.inner_attributes.clone(), true, submodule.is_contract, ) { Ok(child) => { + self.collect_attributes( + submodule.outer_attributes, + file_id, + child.local_id, + file_id, + parent_module_id, + false, + ); + errors.extend(collect_defs( self.def_collector, submodule.contents, @@ -654,8 +705,10 @@ impl<'a> ModCollector<'a> { fn parse_module_declaration( &mut self, context: &mut Context, - mod_decl: &ModuleDeclaration, + mod_decl: ModuleDeclaration, crate_id: CrateId, + parent_file_id: FileId, + parent_module_id: LocalModuleId, macro_processors: &[&dyn MacroProcessor], ) -> Vec<(CompilationError, FileId)> { let mut errors: Vec<(CompilationError, FileId)> = vec![]; @@ -717,10 +770,21 @@ impl<'a> ModCollector<'a> { context, &mod_decl.ident, Location::new(Span::empty(0), child_file_id), + mod_decl.outer_attributes.clone(), + ast.inner_attributes.clone(), true, false, ) { Ok(child_mod_id) => { + self.collect_attributes( + mod_decl.outer_attributes, + child_file_id, + child_mod_id.local_id, + parent_file_id, + parent_module_id, + false, + ); + // Track that the "foo" in `mod foo;` points to the module "foo" context.def_interner.add_module_reference(child_mod_id, location); @@ -743,11 +807,14 @@ impl<'a> ModCollector<'a> { /// Add a child module to the current def_map. /// On error this returns None and pushes to `errors` + #[allow(clippy::too_many_arguments)] fn push_child_module( &mut self, context: &mut Context, mod_name: &Ident, mod_location: Location, + outer_attributes: Vec, + inner_attributes: Vec, add_to_parent_scope: bool, is_contract: bool, ) -> Result { @@ -761,7 +828,8 @@ impl<'a> ModCollector<'a> { // Eventually the location put in `ModuleData` is used for codelenses about `contract`s, // so we keep using `location` so that it continues to work as usual. let location = Location::new(mod_name.span(), mod_location.file); - let new_module = ModuleData::new(parent, location, is_contract); + let new_module = + ModuleData::new(parent, location, outer_attributes, inner_attributes, is_contract); let module_id = self.def_collector.def_map.modules.insert(new_module); let modules = &mut self.def_collector.def_map.modules; diff --git a/compiler/noirc_frontend/src/hir/def_map/mod.rs b/compiler/noirc_frontend/src/hir/def_map/mod.rs index 758b4cf6e5c..a1c4d04cb30 100644 --- a/compiler/noirc_frontend/src/hir/def_map/mod.rs +++ b/compiler/noirc_frontend/src/hir/def_map/mod.rs @@ -111,7 +111,13 @@ impl CrateDefMap { // Allocate a default Module for the root, giving it a ModuleId let mut modules: Arena = Arena::default(); let location = Location::new(Default::default(), root_file_id); - let root = modules.insert(ModuleData::new(None, location, false)); + let root = modules.insert(ModuleData::new( + None, + location, + Vec::new(), + ast.inner_attributes.clone(), + false, + )); let def_map = CrateDefMap { root: LocalModuleId(root), diff --git a/compiler/noirc_frontend/src/hir/def_map/module_data.rs b/compiler/noirc_frontend/src/hir/def_map/module_data.rs index f9542094be7..e829df3572c 100644 --- a/compiler/noirc_frontend/src/hir/def_map/module_data.rs +++ b/compiler/noirc_frontend/src/hir/def_map/module_data.rs @@ -5,6 +5,7 @@ use noirc_errors::Location; use super::{ItemScope, LocalModuleId, ModuleDefId, ModuleId, PerNs}; use crate::ast::{Ident, ItemVisibility}; use crate::node_interner::{FuncId, GlobalId, StructId, TraitId, TypeAliasId}; +use crate::token::SecondaryAttribute; /// Contains the actual contents of a module: its parent (if one exists), /// children, and scope with all definitions defined within the scope. @@ -24,10 +25,25 @@ pub struct ModuleData { /// True if this module is a `contract Foo { ... }` module containing contract functions pub is_contract: bool, + + pub outer_attributes: Vec, + pub inner_attributes: Vec, } impl ModuleData { - pub fn new(parent: Option, location: Location, is_contract: bool) -> ModuleData { + pub fn new( + parent: Option, + location: Location, + outer_attributes: Vec, + inner_attributes: Vec, + is_contract: bool, + ) -> ModuleData { + let outer_attributes = outer_attributes.iter().filter_map(|attr| attr.as_custom()); + let outer_attributes = outer_attributes.map(|attr| attr.contents.to_string()).collect(); + + let inner_attributes = inner_attributes.iter().filter_map(|attr| attr.as_custom()); + let inner_attributes = inner_attributes.map(|attr| attr.contents.to_string()).collect(); + ModuleData { parent, children: HashMap::new(), @@ -35,6 +51,8 @@ impl ModuleData { definitions: ItemScope::default(), location, is_contract, + outer_attributes, + inner_attributes, } } diff --git a/compiler/noirc_frontend/src/lexer/errors.rs b/compiler/noirc_frontend/src/lexer/errors.rs index be5180a777b..2440109af15 100644 --- a/compiler/noirc_frontend/src/lexer/errors.rs +++ b/compiler/noirc_frontend/src/lexer/errors.rs @@ -20,6 +20,8 @@ pub enum LexerErrorKind { IntegerLiteralTooLarge { span: Span, limit: String }, #[error("{:?} is not a valid attribute", found)] MalformedFuncAttribute { span: Span, found: String }, + #[error("{:?} is not a valid inner attribute", found)] + InvalidInnerAttribute { span: Span, found: String }, #[error("Logical and used instead of bitwise and")] LogicalAnd { span: Span }, #[error("Unterminated block comment")] @@ -57,6 +59,7 @@ impl LexerErrorKind { LexerErrorKind::InvalidIntegerLiteral { span, .. } => *span, LexerErrorKind::IntegerLiteralTooLarge { span, .. } => *span, LexerErrorKind::MalformedFuncAttribute { span, .. } => *span, + LexerErrorKind::InvalidInnerAttribute { span, .. } => *span, LexerErrorKind::LogicalAnd { span } => *span, LexerErrorKind::UnterminatedBlockComment { span } => *span, LexerErrorKind::UnterminatedStringLiteral { span } => *span, @@ -103,6 +106,11 @@ impl LexerErrorKind { format!(" {found} is not a valid attribute"), *span, ), + LexerErrorKind::InvalidInnerAttribute { span, found } => ( + "Invalid inner attribute".to_string(), + format!(" {found} is not a valid inner attribute"), + *span, + ), LexerErrorKind::LogicalAnd { span } => ( "Noir has no logical-and (&&) operator since short-circuiting is much less efficient when compiling to circuits".to_string(), "Try `&` instead, or use `if` only if you require short-circuiting".to_string(), diff --git a/compiler/noirc_frontend/src/lexer/lexer.rs b/compiler/noirc_frontend/src/lexer/lexer.rs index 7265593238d..b7492396c90 100644 --- a/compiler/noirc_frontend/src/lexer/lexer.rs +++ b/compiler/noirc_frontend/src/lexer/lexer.rs @@ -286,6 +286,13 @@ impl<'a> Lexer<'a> { fn eat_attribute(&mut self) -> SpannedTokenResult { let start = self.position; + let is_inner = if self.peek_char_is('!') { + self.next_char(); + true + } else { + false + }; + if !self.peek_char_is('[') { return Err(LexerErrorKind::UnexpectedCharacter { span: Span::single_char(self.position), @@ -316,8 +323,19 @@ impl<'a> Lexer<'a> { let contents_span = Span::inclusive(contents_start, contents_end); let attribute = Attribute::lookup_attribute(&word, span, contents_span)?; - - Ok(attribute.into_span(start, end)) + if is_inner { + match attribute { + Attribute::Function(attribute) => Err(LexerErrorKind::InvalidInnerAttribute { + span: Span::from(start..end), + found: attribute.to_string(), + }), + Attribute::Secondary(attribute) => { + Ok(Token::InnerAttribute(attribute).into_span(start, end)) + } + } + } else { + Ok(Token::Attribute(attribute).into_span(start, end)) + } } //XXX(low): Can increase performance if we use iterator semantic and utilize some of the methods on String. See below @@ -907,6 +925,22 @@ mod tests { assert_eq!(sub_string, "test(invalid_scope)"); } + #[test] + fn test_inner_attribute() { + let input = r#"#![something]"#; + let mut lexer = Lexer::new(input); + + let token = lexer.next_token().unwrap(); + assert_eq!( + token.token(), + &Token::InnerAttribute(SecondaryAttribute::Custom(CustomAtrribute { + contents: "something".to_string(), + span: Span::from(0..13), + contents_span: Span::from(3..12), + })) + ); + } + #[test] fn test_int_type() { let input = "u16 i16 i108 u104.5"; diff --git a/compiler/noirc_frontend/src/lexer/token.rs b/compiler/noirc_frontend/src/lexer/token.rs index 1692908187e..7b805b5fd8d 100644 --- a/compiler/noirc_frontend/src/lexer/token.rs +++ b/compiler/noirc_frontend/src/lexer/token.rs @@ -27,6 +27,7 @@ pub enum BorrowedToken<'input> { Keyword(Keyword), IntType(IntType), Attribute(Attribute), + InnerAttribute(SecondaryAttribute), LineComment(&'input str, Option), BlockComment(&'input str, Option), Quote(&'input Tokens), @@ -132,6 +133,7 @@ pub enum Token { Keyword(Keyword), IntType(IntType), Attribute(Attribute), + InnerAttribute(SecondaryAttribute), LineComment(String, Option), BlockComment(String, Option), // A `quote { ... }` along with the tokens in its token stream. @@ -244,6 +246,7 @@ pub fn token_to_borrowed_token(token: &Token) -> BorrowedToken<'_> { Token::RawStr(ref b, hashes) => BorrowedToken::RawStr(b, *hashes), Token::Keyword(k) => BorrowedToken::Keyword(*k), Token::Attribute(ref a) => BorrowedToken::Attribute(a.clone()), + Token::InnerAttribute(ref a) => BorrowedToken::InnerAttribute(a.clone()), Token::LineComment(ref s, _style) => BorrowedToken::LineComment(s, *_style), Token::BlockComment(ref s, _style) => BorrowedToken::BlockComment(s, *_style), Token::Quote(stream) => BorrowedToken::Quote(stream), @@ -363,6 +366,7 @@ impl fmt::Display for Token { } Token::Keyword(k) => write!(f, "{k}"), Token::Attribute(ref a) => write!(f, "{a}"), + Token::InnerAttribute(ref a) => write!(f, "#![{a}]"), Token::LineComment(ref s, _style) => write!(f, "//{s}"), Token::BlockComment(ref s, _style) => write!(f, "/*{s}*/"), Token::Quote(ref stream) => { @@ -428,6 +432,7 @@ pub enum TokenKind { Literal, Keyword, Attribute, + InnerAttribute, Quote, QuotedType, InternedExpr, @@ -445,6 +450,7 @@ impl fmt::Display for TokenKind { TokenKind::Literal => write!(f, "literal"), TokenKind::Keyword => write!(f, "keyword"), TokenKind::Attribute => write!(f, "attribute"), + TokenKind::InnerAttribute => write!(f, "inner attribute"), TokenKind::Quote => write!(f, "quote"), TokenKind::QuotedType => write!(f, "quoted type"), TokenKind::InternedExpr => write!(f, "interned expr"), @@ -467,6 +473,7 @@ impl Token { | Token::FmtStr(_) => TokenKind::Literal, Token::Keyword(_) => TokenKind::Keyword, Token::Attribute(_) => TokenKind::Attribute, + Token::InnerAttribute(_) => TokenKind::InnerAttribute, Token::UnquoteMarker(_) => TokenKind::UnquoteMarker, Token::Quote(_) => TokenKind::Quote, Token::QuotedType(_) => TokenKind::QuotedType, @@ -701,7 +708,7 @@ impl Attribute { word: &str, span: Span, contents_span: Span, - ) -> Result { + ) -> Result { let word_segments: Vec<&str> = word .split(|c| c == '(' || c == ')') .filter(|string_segment| !string_segment.is_empty()) @@ -782,7 +789,7 @@ impl Attribute { } }; - Ok(Token::Attribute(attribute)) + Ok(attribute) } } diff --git a/compiler/noirc_frontend/src/parser/mod.rs b/compiler/noirc_frontend/src/parser/mod.rs index c82906b69a2..596d15176bc 100644 --- a/compiler/noirc_frontend/src/parser/mod.rs +++ b/compiler/noirc_frontend/src/parser/mod.rs @@ -16,7 +16,7 @@ use crate::ast::{ NoirFunction, NoirStruct, NoirTrait, NoirTraitImpl, NoirTypeAlias, Recoverable, StatementKind, TypeImpl, UseTree, }; -use crate::token::{Keyword, Token}; +use crate::token::{Keyword, SecondaryAttribute, Token}; use chumsky::prelude::*; use chumsky::primitive::Container; @@ -41,6 +41,7 @@ pub enum TopLevelStatement { TypeAlias(NoirTypeAlias), SubModule(ParsedSubModule), Global(LetStatement), + InnerAttribute(SecondaryAttribute), Error, } @@ -57,6 +58,7 @@ impl TopLevelStatement { TopLevelStatement::TypeAlias(t) => Some(ItemKind::TypeAlias(t)), TopLevelStatement::SubModule(s) => Some(ItemKind::Submodules(s)), TopLevelStatement::Global(c) => Some(ItemKind::Global(c)), + TopLevelStatement::InnerAttribute(a) => Some(ItemKind::InnerAttribute(a)), TopLevelStatement::Error => None, } } @@ -247,6 +249,8 @@ pub struct SortedModule { /// Full submodules as in `mod foo { ... definitions ... }` pub submodules: Vec, + + pub inner_attributes: Vec, } impl std::fmt::Display for SortedModule { @@ -309,6 +313,7 @@ impl ParsedModule { ItemKind::Global(global) => module.push_global(global), ItemKind::ModuleDecl(mod_name) => module.push_module_decl(mod_name), ItemKind::Submodules(submodule) => module.push_submodule(submodule.into_sorted()), + ItemKind::InnerAttribute(attribute) => module.inner_attributes.push(attribute), } } @@ -334,6 +339,7 @@ pub enum ItemKind { Global(LetStatement), ModuleDecl(ModuleDeclaration), Submodules(ParsedSubModule), + InnerAttribute(SecondaryAttribute), } /// A submodule defined via `mod name { contents }` in some larger file. @@ -342,6 +348,7 @@ pub enum ItemKind { pub struct ParsedSubModule { pub name: Ident, pub contents: ParsedModule, + pub outer_attributes: Vec, pub is_contract: bool, } @@ -350,6 +357,7 @@ impl ParsedSubModule { SortedSubModule { name: self.name, contents: self.contents.into_sorted(), + outer_attributes: self.outer_attributes, is_contract: self.is_contract, } } @@ -371,6 +379,7 @@ impl std::fmt::Display for SortedSubModule { pub struct SortedSubModule { pub name: Ident, pub contents: SortedModule, + pub outer_attributes: Vec, pub is_contract: bool, } @@ -512,6 +521,7 @@ impl std::fmt::Display for TopLevelStatement { TopLevelStatement::TypeAlias(t) => t.fmt(f), TopLevelStatement::SubModule(s) => s.fmt(f), TopLevelStatement::Global(c) => c.fmt(f), + TopLevelStatement::InnerAttribute(a) => write!(f, "#![{}]", a), TopLevelStatement::Error => write!(f, "error"), } } diff --git a/compiler/noirc_frontend/src/parser/parser.rs b/compiler/noirc_frontend/src/parser/parser.rs index bead1e69006..2bc7a88c6c5 100644 --- a/compiler/noirc_frontend/src/parser/parser.rs +++ b/compiler/noirc_frontend/src/parser/parser.rs @@ -26,6 +26,7 @@ use self::path::as_trait_path; use self::primitives::{keyword, macro_quote_marker, mutable_reference, variable}; use self::types::{generic_type_args, maybe_comp_time}; +use attributes::{attributes, inner_attribute, validate_secondary_attributes}; pub use types::parse_type; use visibility::visibility_modifier; @@ -91,7 +92,7 @@ pub fn parse_program(source_program: &str) -> (ParsedModule, Vec) { let (module, mut parsing_errors) = program().parse_recovery_verbose(tokens); parsing_errors.extend(lexing_errors.into_iter().map(Into::into)); - let parsed_module = module.unwrap_or(ParsedModule { items: vec![] }); + let parsed_module = module.unwrap_or_default(); if cfg!(feature = "experimental_parser") { for parsed_item in &parsed_module.items { @@ -215,6 +216,7 @@ fn top_level_statement<'a>( module_declaration().then_ignore(force(just(Token::Semicolon))), use_statement().then_ignore(force(just(Token::Semicolon))), global_declaration().then_ignore(force(just(Token::Semicolon))), + inner_attribute().map(TopLevelStatement::InnerAttribute), )) .recover_via(top_level_statement_recovery()) } @@ -287,25 +289,39 @@ fn global_declaration() -> impl NoirParser { /// submodule: 'mod' ident '{' module '}' fn submodule(module_parser: impl NoirParser) -> impl NoirParser { - keyword(Keyword::Mod) - .ignore_then(ident()) + attributes() + .then_ignore(keyword(Keyword::Mod)) + .then(ident()) .then_ignore(just(Token::LeftBrace)) .then(module_parser) .then_ignore(just(Token::RightBrace)) - .map(|(name, contents)| { - TopLevelStatement::SubModule(ParsedSubModule { name, contents, is_contract: false }) + .validate(|((attributes, name), contents), span, emit| { + let attributes = validate_secondary_attributes(attributes, span, emit); + TopLevelStatement::SubModule(ParsedSubModule { + name, + contents, + outer_attributes: attributes, + is_contract: false, + }) }) } /// contract: 'contract' ident '{' module '}' fn contract(module_parser: impl NoirParser) -> impl NoirParser { - keyword(Keyword::Contract) - .ignore_then(ident()) + attributes() + .then_ignore(keyword(Keyword::Contract)) + .then(ident()) .then_ignore(just(Token::LeftBrace)) .then(module_parser) .then_ignore(just(Token::RightBrace)) - .map(|(name, contents)| { - TopLevelStatement::SubModule(ParsedSubModule { name, contents, is_contract: true }) + .validate(|((attributes, name), contents), span, emit| { + let attributes = validate_secondary_attributes(attributes, span, emit); + TopLevelStatement::SubModule(ParsedSubModule { + name, + contents, + outer_attributes: attributes, + is_contract: true, + }) }) } @@ -434,9 +450,12 @@ fn optional_type_annotation<'a>() -> impl NoirParser + 'a { } fn module_declaration() -> impl NoirParser { - keyword(Keyword::Mod) - .ignore_then(ident()) - .map(|ident| TopLevelStatement::Module(ModuleDeclaration { ident })) + attributes().then_ignore(keyword(Keyword::Mod)).then(ident()).validate( + |(attributes, ident), span, emit| { + let attributes = validate_secondary_attributes(attributes, span, emit); + TopLevelStatement::Module(ModuleDeclaration { ident, outer_attributes: attributes }) + }, + ) } fn use_statement() -> impl NoirParser { @@ -1522,9 +1541,22 @@ mod test { #[test] fn parse_module_declaration() { parse_with(module_declaration(), "mod foo").unwrap(); + parse_with(module_declaration(), "#[attr] mod foo").unwrap(); parse_with(module_declaration(), "mod 1").unwrap_err(); } + #[test] + fn parse_submodule_declaration() { + parse_with(submodule(module()), "mod foo {}").unwrap(); + parse_with(submodule(module()), "#[attr] mod foo {}").unwrap(); + } + + #[test] + fn parse_contract() { + parse_with(contract(module()), "contract foo {}").unwrap(); + parse_with(contract(module()), "#[attr] contract foo {}").unwrap(); + } + #[test] fn parse_use() { let valid_use_statements = [ diff --git a/compiler/noirc_frontend/src/parser/parser/attributes.rs b/compiler/noirc_frontend/src/parser/parser/attributes.rs index 47add6f82e0..66d0ca29ca6 100644 --- a/compiler/noirc_frontend/src/parser/parser/attributes.rs +++ b/compiler/noirc_frontend/src/parser/parser/attributes.rs @@ -67,3 +67,12 @@ pub(super) fn validate_secondary_attributes( struct_attributes } + +pub(super) fn inner_attribute() -> impl NoirParser { + token_kind(TokenKind::InnerAttribute).map(|token| match token { + Token::InnerAttribute(attribute) => attribute, + _ => unreachable!( + "Parser should have already errored due to token not being an inner attribute" + ), + }) +} diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index e7dbe11f0d1..c6c8c5d4b4b 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -28,7 +28,8 @@ use crate::hir::def_collector::dc_crate::DefCollector; use crate::hir_def::expr::HirExpression; use crate::hir_def::stmt::HirStatement; use crate::monomorphization::monomorphize; -use crate::parser::ParserErrorReason; +use crate::parser::{ItemKind, ParserErrorReason}; +use crate::token::SecondaryAttribute; use crate::ParsedModule; use crate::{ hir::def_map::{CrateDefMap, LocalModuleId}, @@ -64,10 +65,28 @@ pub(crate) fn get_program(src: &str) -> (ParsedModule, Context, Vec<(Compilation remove_experimental_warnings(&mut errors); if !has_parser_error(&errors) { + let inner_attributes: Vec = program + .items + .iter() + .filter_map(|item| { + if let ItemKind::InnerAttribute(attribute) = &item.kind { + Some(attribute.clone()) + } else { + None + } + }) + .collect(); + // Allocate a default Module for the root, giving it a ModuleId let mut modules: Arena = Arena::default(); let location = Location::new(Default::default(), root_file_id); - let root = modules.insert(ModuleData::new(None, location, false)); + let root = modules.insert(ModuleData::new( + None, + location, + Vec::new(), + inner_attributes.clone(), + false, + )); let def_map = CrateDefMap { root: LocalModuleId(root), diff --git a/docs/docs/noir/standard_library/meta/module.md b/docs/docs/noir/standard_library/meta/module.md index d283f2da8b2..870e366461c 100644 --- a/docs/docs/noir/standard_library/meta/module.md +++ b/docs/docs/noir/standard_library/meta/module.md @@ -20,6 +20,12 @@ Returns the name of the module. Returns each function in the module. +### has_named_attribute + +#include_code has_named_attribute noir_stdlib/src/meta/module.nr rust + +Returns true if this module has a custom attribute with the given name. + ### is_contract #include_code is_contract noir_stdlib/src/meta/module.nr rust diff --git a/noir_stdlib/src/meta/module.nr b/noir_stdlib/src/meta/module.nr index 6ea3ca55fb1..b3f76812b8a 100644 --- a/noir_stdlib/src/meta/module.nr +++ b/noir_stdlib/src/meta/module.nr @@ -1,4 +1,9 @@ impl Module { + #[builtin(module_has_named_attribute)] + // docs:start:has_named_attribute + fn has_named_attribute(self, name: Quoted) -> bool {} + // docs:end:has_named_attribute + #[builtin(module_is_contract)] // docs:start:is_contract fn is_contract(self) -> bool {} diff --git a/test_programs/compile_success_empty/comptime_module/src/main.nr b/test_programs/compile_success_empty/comptime_module/src/main.nr index 8d834381fea..5722d42ca26 100644 --- a/test_programs/compile_success_empty/comptime_module/src/main.nr +++ b/test_programs/compile_success_empty/comptime_module/src/main.nr @@ -1,10 +1,45 @@ +#[outer_attribute] mod foo { + #![some_attribute] fn x() {} fn y() {} } contract bar {} +#[some_attribute] +mod another_module {} + +#[outer_attribute_func] +mod yet_another_module { + #![super::inner_attribute_func] + fn foo() {} +} + +#[outer_attribute_separate_module] +mod separate_module; + +comptime mut global counter = 0; + +fn increment_counter() { + counter += 1; +} + +fn outer_attribute_func(m: Module) { + assert_eq(m.name(), quote { yet_another_module }); + increment_counter(); +} + +fn inner_attribute_func(m: Module) { + assert_eq(m.name(), quote { yet_another_module }); + increment_counter(); +} + +fn outer_attribute_separate_module(m: Module) { + assert_eq(m.name(), quote { separate_module }); + increment_counter(); +} + fn main() { comptime { @@ -15,6 +50,8 @@ fn main() { let bar = quote { bar }.as_module().unwrap(); assert(bar.is_contract()); + let another_module = quote { another_module }.as_module().unwrap(); + // Check Module::functions assert_eq(foo.functions().len(), 2); assert_eq(bar.functions().len(), 0); @@ -22,7 +59,15 @@ fn main() { // Check Module::name assert_eq(foo.name(), quote { foo }); assert_eq(bar.name(), quote { bar }); + + // Check Module::has_named_attribute + assert(foo.has_named_attribute(quote { some_attribute })); + assert(foo.has_named_attribute(quote { outer_attribute })); + assert(!bar.has_named_attribute(quote { some_attribute })); + assert(another_module.has_named_attribute(quote { some_attribute })); } + + assert_eq(counter, 4); } // docs:start:as_module_example diff --git a/test_programs/compile_success_empty/comptime_module/src/separate_module.nr b/test_programs/compile_success_empty/comptime_module/src/separate_module.nr new file mode 100644 index 00000000000..53784101507 --- /dev/null +++ b/test_programs/compile_success_empty/comptime_module/src/separate_module.nr @@ -0,0 +1,5 @@ +#![inner_attribute_separate_module] +fn inner_attribute_separate_module(m: Module) { + assert_eq(m.name(), quote { separate_module }); + super::increment_counter(); +} diff --git a/tooling/nargo_fmt/src/visitor/item.rs b/tooling/nargo_fmt/src/visitor/item.rs index 0e2d07f13d0..9e556e0fcbe 100644 --- a/tooling/nargo_fmt/src/visitor/item.rs +++ b/tooling/nargo_fmt/src/visitor/item.rs @@ -165,6 +165,11 @@ impl super::FmtVisitor<'_> { continue; } + for attribute in module.outer_attributes { + self.push_str(&format!("#[{}]\n", attribute.as_ref())); + self.push_str(&self.indent.to_string()); + } + let name = module.name; let after_brace = self.span_after(span, Token::LeftBrace).start(); self.last_position = after_brace; @@ -227,7 +232,8 @@ impl super::FmtVisitor<'_> { | ItemKind::TraitImpl(_) | ItemKind::TypeAlias(_) | ItemKind::Global(_) - | ItemKind::ModuleDecl(_) => { + | ItemKind::ModuleDecl(_) + | ItemKind::InnerAttribute(_) => { self.push_rewrite(self.slice(span).to_string(), span); self.last_position = span.end(); } diff --git a/tooling/nargo_fmt/tests/expected/module.nr b/tooling/nargo_fmt/tests/expected/module.nr index e419543dbc4..0a051a1b50f 100644 --- a/tooling/nargo_fmt/tests/expected/module.nr +++ b/tooling/nargo_fmt/tests/expected/module.nr @@ -1,3 +1,6 @@ +#![inner] +#![inner2] + mod a { mod b { struct Data { @@ -13,6 +16,8 @@ mod a { Data2 { a } } + #[custom] + #[another_custom] mod tests { #[test] fn test() { @@ -20,4 +25,11 @@ mod a { data2(1); } } + + #[attr] + mod baz; + + mod empty { + #![inner] + } } diff --git a/tooling/nargo_fmt/tests/input/module.nr b/tooling/nargo_fmt/tests/input/module.nr index e419543dbc4..0a051a1b50f 100644 --- a/tooling/nargo_fmt/tests/input/module.nr +++ b/tooling/nargo_fmt/tests/input/module.nr @@ -1,3 +1,6 @@ +#![inner] +#![inner2] + mod a { mod b { struct Data { @@ -13,6 +16,8 @@ mod a { Data2 { a } } + #[custom] + #[another_custom] mod tests { #[test] fn test() { @@ -20,4 +25,11 @@ mod a { data2(1); } } + + #[attr] + mod baz; + + mod empty { + #![inner] + } } From 94e661e7520d80496bdc9da39b9736bafacb96dc Mon Sep 17 00:00:00 2001 From: jfecher Date: Wed, 4 Sep 2024 14:31:29 -0500 Subject: [PATCH 09/25] feat: Allow inserting new structs and into programs from attributes (#5927) # Description ## Problem\* Resolves https://github.com/noir-lang/noir/issues/5913 ## Summary\* Allows inserting new structs and impls into the program from comptime attributes. ## Additional Context Inserting new structs wasn't allowed originally since functions wouldn't be able to refer to them in their signature. This use case isn't needed for aztec currently so I'm adding this feature now and we can look into expanding it later. See https://github.com/noir-lang/noir/issues/5926 ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../noirc_frontend/src/elaborator/comptime.rs | 20 +- .../noirc_frontend/src/hir/comptime/errors.rs | 2 +- .../src/hir/def_collector/dc_mod.rs | 374 ++++++++++-------- compiler/noirc_frontend/src/hir/mod.rs | 4 +- docs/docs/noir/concepts/comptime.md | 2 +- .../unquote_struct/Nargo.toml | 7 + .../unquote_struct/src/main.nr | 28 ++ 7 files changed, 272 insertions(+), 165 deletions(-) create mode 100644 test_programs/compile_success_empty/unquote_struct/Nargo.toml create mode 100644 test_programs/compile_success_empty/unquote_struct/src/main.nr diff --git a/compiler/noirc_frontend/src/elaborator/comptime.rs b/compiler/noirc_frontend/src/elaborator/comptime.rs index cfc2e34c520..e8dbf2ec775 100644 --- a/compiler/noirc_frontend/src/elaborator/comptime.rs +++ b/compiler/noirc_frontend/src/elaborator/comptime.rs @@ -385,14 +385,30 @@ impl<'context> Elaborator<'context> { self.errors.push(error); } } + TopLevelStatement::Struct(struct_def) => { + if let Some((type_id, the_struct)) = dc_mod::collect_struct( + self.interner, + self.def_maps.get_mut(&self.crate_id).unwrap(), + struct_def, + self.file, + self.local_module, + self.crate_id, + &mut self.errors, + ) { + generated_items.types.insert(type_id, the_struct); + } + } + TopLevelStatement::Impl(r#impl) => { + let module = self.module_id(); + dc_mod::collect_impl(self.interner, generated_items, r#impl, self.file, module); + } + // Assume that an error has already been issued TopLevelStatement::Error => (), TopLevelStatement::Module(_) | TopLevelStatement::Import(..) - | TopLevelStatement::Struct(_) | TopLevelStatement::Trait(_) - | TopLevelStatement::Impl(_) | TopLevelStatement::TypeAlias(_) | TopLevelStatement::SubModule(_) | TopLevelStatement::InnerAttribute(_) => { diff --git a/compiler/noirc_frontend/src/hir/comptime/errors.rs b/compiler/noirc_frontend/src/hir/comptime/errors.rs index cfee6bcedac..48efc08f463 100644 --- a/compiler/noirc_frontend/src/hir/comptime/errors.rs +++ b/compiler/noirc_frontend/src/hir/comptime/errors.rs @@ -488,7 +488,7 @@ impl<'a> From<&'a InterpreterError> for CustomDiagnostic { InterpreterError::UnsupportedTopLevelItemUnquote { item, location } => { let msg = "Unsupported statement type to unquote".into(); let secondary = - "Only functions, globals, and trait impls can be unquoted here".into(); + "Only functions, structs, globals, and impls can be unquoted here".into(); let mut error = CustomDiagnostic::simple_error(msg, secondary, location.span); error.add_note(format!("Unquoted item was:\n{item}")); error diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 520cccf7580..d6432b0ca56 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -15,7 +15,7 @@ use crate::ast::{ TypeImpl, }; use crate::hir::resolution::errors::ResolverError; -use crate::macros_api::{Expression, NodeInterner, UnresolvedType, UnresolvedTypeData}; +use crate::macros_api::{Expression, NodeInterner, StructId, UnresolvedType, UnresolvedTypeData}; use crate::node_interner::ModuleAttributes; use crate::token::SecondaryAttribute; use crate::{ @@ -27,6 +27,7 @@ use crate::{ }; use crate::{Generics, Kind, ResolvedGeneric, Type, TypeVariable}; +use super::dc_crate::CollectedItems; use super::dc_crate::ModuleAttribute; use super::{ dc_crate::{ @@ -171,24 +172,13 @@ impl<'a> ModCollector<'a> { let module_id = ModuleId { krate, local_id: self.module_id }; for r#impl in impls { - let mut unresolved_functions = UnresolvedFunctions { - file_id: self.file_id, - functions: Vec::new(), - trait_id: None, - self_type: None, - }; - - for (mut method, _) in r#impl.methods { - let func_id = context.def_interner.push_empty_fn(); - method.def.where_clause.extend(r#impl.where_clause.clone()); - let location = Location::new(method.span(), self.file_id); - context.def_interner.push_function(func_id, &method.def, module_id, location); - unresolved_functions.push_fn(self.module_id, func_id, method); - } - - let key = (r#impl.object_type, self.module_id); - let methods = self.def_collector.items.impls.entry(key).or_default(); - methods.push((r#impl.generics, r#impl.type_span, unresolved_functions)); + collect_impl( + &mut context.def_interner, + &mut self.def_collector.items, + r#impl, + self.file_id, + module_id, + ); } } @@ -315,92 +305,21 @@ impl<'a> ModCollector<'a> { ) -> Vec<(CompilationError, FileId)> { let mut definition_errors = vec![]; for struct_definition in types { - self.check_duplicate_field_names(&struct_definition, &mut definition_errors); - - let name = struct_definition.name.clone(); - - let unresolved = UnresolvedStruct { - file_id: self.file_id, - module_id: self.module_id, - struct_def: struct_definition, - }; - - let resolved_generics = context.resolve_generics( - &unresolved.struct_def.generics, - &mut definition_errors, + if let Some((id, the_struct)) = collect_struct( + &mut context.def_interner, + &mut self.def_collector.def_map, + struct_definition, self.file_id, - ); - - // Create the corresponding module for the struct namespace - let id = match self.push_child_module( - context, - &name, - Location::new(name.span(), self.file_id), - Vec::new(), - Vec::new(), - false, - false, + self.module_id, + krate, + &mut definition_errors, ) { - Ok(module_id) => context.def_interner.new_struct( - &unresolved, - resolved_generics, - krate, - module_id.local_id, - self.file_id, - ), - Err(error) => { - definition_errors.push((error.into(), self.file_id)); - continue; - } - }; - - // Add the struct to scope so its path can be looked up later - let result = self.def_collector.def_map.modules[self.module_id.0] - .declare_struct(name.clone(), id); - - if let Err((first_def, second_def)) = result { - let error = DefCollectorErrorKind::Duplicate { - typ: DuplicateType::TypeDefinition, - first_def, - second_def, - }; - definition_errors.push((error.into(), self.file_id)); - } - - // And store the TypeId -> StructType mapping somewhere it is reachable - self.def_collector.items.types.insert(id, unresolved); - - if context.def_interner.is_in_lsp_mode() { - let parent_module_id = ModuleId { krate, local_id: self.module_id }; - context.def_interner.register_struct(id, name.to_string(), parent_module_id); + self.def_collector.items.types.insert(id, the_struct); } } definition_errors } - fn check_duplicate_field_names( - &self, - struct_definition: &NoirStruct, - definition_errors: &mut Vec<(CompilationError, FileId)>, - ) { - let mut seen_field_names = std::collections::HashSet::new(); - for (field_name, _) in &struct_definition.fields { - if seen_field_names.insert(field_name) { - continue; - } - - let previous_field_name = *seen_field_names.get(field_name).unwrap(); - definition_errors.push(( - DefCollectorErrorKind::DuplicateField { - first_def: previous_field_name.clone(), - second_def: field_name.clone(), - } - .into(), - self.file_id, - )); - } - } - /// Collect any type aliases definitions declared within the ast. /// Returns a vector of errors if any type aliases were already defined. fn collect_type_aliases( @@ -420,7 +339,8 @@ impl<'a> ModCollector<'a> { type_alias_def: type_alias, }; - let resolved_generics = context.resolve_generics( + let resolved_generics = Context::resolve_generics( + &context.def_interner, &unresolved.type_alias_def.generics, &mut errors, self.file_id, @@ -623,8 +543,12 @@ impl<'a> ModCollector<'a> { } } - let resolved_generics = - context.resolve_generics(&trait_definition.generics, &mut errors, self.file_id); + let resolved_generics = Context::resolve_generics( + &context.def_interner, + &trait_definition.generics, + &mut errors, + self.file_id, + ); let unresolved = UnresolvedTrait { file_id: self.file_id, @@ -818,64 +742,17 @@ impl<'a> ModCollector<'a> { add_to_parent_scope: bool, is_contract: bool, ) -> Result { - let parent = Some(self.module_id); - - // Note: the difference between `location` and `mod_location` is: - // - `mod_location` will point to either the token "foo" in `mod foo { ... }` - // if it's an inline module, or the first char of a the file if it's an external module. - // - `location` will always point to the token "foo" in `mod foo` regardless of whether - // it's inline or external. - // Eventually the location put in `ModuleData` is used for codelenses about `contract`s, - // so we keep using `location` so that it continues to work as usual. - let location = Location::new(mod_name.span(), mod_location.file); - let new_module = - ModuleData::new(parent, location, outer_attributes, inner_attributes, is_contract); - let module_id = self.def_collector.def_map.modules.insert(new_module); - - let modules = &mut self.def_collector.def_map.modules; - - // Update the parent module to reference the child - modules[self.module_id.0].children.insert(mod_name.clone(), LocalModuleId(module_id)); - - let mod_id = ModuleId { - krate: self.def_collector.def_map.krate, - local_id: LocalModuleId(module_id), - }; - - // Add this child module into the scope of the parent module as a module definition - // module definitions are definitions which can only exist at the module level. - // ModuleDefinitionIds can be used across crates since they contain the CrateId - // - // We do not want to do this in the case of struct modules (each struct type corresponds - // to a child module containing its methods) since the module name should not shadow - // the struct name. - if add_to_parent_scope { - if let Err((first_def, second_def)) = - modules[self.module_id.0].declare_child_module(mod_name.to_owned(), mod_id) - { - let err = DefCollectorErrorKind::Duplicate { - typ: DuplicateType::Module, - first_def, - second_def, - }; - return Err(err); - } - - context.def_interner.add_module_attributes( - mod_id, - ModuleAttributes { - name: mod_name.0.contents.clone(), - location: mod_location, - parent: Some(self.module_id), - }, - ); - - if context.def_interner.is_in_lsp_mode() { - context.def_interner.register_module(mod_id, mod_name.0.contents.clone()); - } - } - - Ok(mod_id) + push_child_module( + &mut context.def_interner, + &mut self.def_collector.def_map, + self.module_id, + mod_name, + mod_location, + outer_attributes, + inner_attributes, + add_to_parent_scope, + is_contract, + ) } fn resolve_associated_constant_type( @@ -896,6 +773,162 @@ impl<'a> ModCollector<'a> { } } +/// Add a child module to the current def_map. +/// On error this returns None and pushes to `errors` +#[allow(clippy::too_many_arguments)] +fn push_child_module( + interner: &mut NodeInterner, + def_map: &mut CrateDefMap, + parent: LocalModuleId, + mod_name: &Ident, + mod_location: Location, + outer_attributes: Vec, + inner_attributes: Vec, + add_to_parent_scope: bool, + is_contract: bool, +) -> Result { + // Note: the difference between `location` and `mod_location` is: + // - `mod_location` will point to either the token "foo" in `mod foo { ... }` + // if it's an inline module, or the first char of a the file if it's an external module. + // - `location` will always point to the token "foo" in `mod foo` regardless of whether + // it's inline or external. + // Eventually the location put in `ModuleData` is used for codelenses about `contract`s, + // so we keep using `location` so that it continues to work as usual. + let location = Location::new(mod_name.span(), mod_location.file); + let new_module = + ModuleData::new(Some(parent), location, outer_attributes, inner_attributes, is_contract); + + let module_id = def_map.modules.insert(new_module); + let modules = &mut def_map.modules; + + // Update the parent module to reference the child + modules[parent.0].children.insert(mod_name.clone(), LocalModuleId(module_id)); + + let mod_id = ModuleId { krate: def_map.krate, local_id: LocalModuleId(module_id) }; + + // Add this child module into the scope of the parent module as a module definition + // module definitions are definitions which can only exist at the module level. + // ModuleDefinitionIds can be used across crates since they contain the CrateId + // + // We do not want to do this in the case of struct modules (each struct type corresponds + // to a child module containing its methods) since the module name should not shadow + // the struct name. + if add_to_parent_scope { + if let Err((first_def, second_def)) = + modules[parent.0].declare_child_module(mod_name.to_owned(), mod_id) + { + let err = DefCollectorErrorKind::Duplicate { + typ: DuplicateType::Module, + first_def, + second_def, + }; + return Err(err); + } + + interner.add_module_attributes( + mod_id, + ModuleAttributes { + name: mod_name.0.contents.clone(), + location: mod_location, + parent: Some(parent), + }, + ); + + if interner.is_in_lsp_mode() { + interner.register_module(mod_id, mod_name.0.contents.clone()); + } + } + + Ok(mod_id) +} + +pub fn collect_struct( + interner: &mut NodeInterner, + def_map: &mut CrateDefMap, + struct_definition: NoirStruct, + file_id: FileId, + module_id: LocalModuleId, + krate: CrateId, + definition_errors: &mut Vec<(CompilationError, FileId)>, +) -> Option<(StructId, UnresolvedStruct)> { + check_duplicate_field_names(&struct_definition, file_id, definition_errors); + + let name = struct_definition.name.clone(); + + let unresolved = UnresolvedStruct { file_id, module_id, struct_def: struct_definition }; + + let resolved_generics = Context::resolve_generics( + interner, + &unresolved.struct_def.generics, + definition_errors, + file_id, + ); + + // Create the corresponding module for the struct namespace + let location = Location::new(name.span(), file_id); + let id = match push_child_module( + interner, + def_map, + module_id, + &name, + location, + Vec::new(), + Vec::new(), + false, + false, + ) { + Ok(module_id) => { + interner.new_struct(&unresolved, resolved_generics, krate, module_id.local_id, file_id) + } + Err(error) => { + definition_errors.push((error.into(), file_id)); + return None; + } + }; + + // Add the struct to scope so its path can be looked up later + let result = def_map.modules[module_id.0].declare_struct(name.clone(), id); + + if let Err((first_def, second_def)) = result { + let error = DefCollectorErrorKind::Duplicate { + typ: DuplicateType::TypeDefinition, + first_def, + second_def, + }; + definition_errors.push((error.into(), file_id)); + } + + if interner.is_in_lsp_mode() { + let parent_module_id = ModuleId { krate, local_id: module_id }; + interner.register_struct(id, name.to_string(), parent_module_id); + } + + Some((id, unresolved)) +} + +pub fn collect_impl( + interner: &mut NodeInterner, + items: &mut CollectedItems, + r#impl: TypeImpl, + file_id: FileId, + module_id: ModuleId, +) { + let mut unresolved_functions = + UnresolvedFunctions { file_id, functions: Vec::new(), trait_id: None, self_type: None }; + + for (mut method, _) in r#impl.methods { + let func_id = interner.push_empty_fn(); + method.def.where_clause.extend(r#impl.where_clause.clone()); + let location = Location::new(method.span(), file_id); + interner.push_function(func_id, &method.def, module_id, location); + unresolved_functions.push_fn(module_id.local_id, func_id, method); + } + + let key = (r#impl.object_type, module_id.local_id); + let methods = items.impls.entry(key).or_default(); + methods.push((r#impl.generics, r#impl.type_span, unresolved_functions)); +} + fn find_module( file_manager: &FileManager, anchor: FileId, @@ -1054,6 +1087,29 @@ pub(crate) fn collect_global( (global, error) } +fn check_duplicate_field_names( + struct_definition: &NoirStruct, + file: FileId, + definition_errors: &mut Vec<(CompilationError, FileId)>, +) { + let mut seen_field_names = std::collections::HashSet::new(); + for (field_name, _) in &struct_definition.fields { + if seen_field_names.insert(field_name) { + continue; + } + + let previous_field_name = *seen_field_names.get(field_name).unwrap(); + definition_errors.push(( + DefCollectorErrorKind::DuplicateField { + first_def: previous_field_name.clone(), + second_def: field_name.clone(), + } + .into(), + file, + )); + } +} + #[cfg(test)] mod find_module_tests { use super::*; diff --git a/compiler/noirc_frontend/src/hir/mod.rs b/compiler/noirc_frontend/src/hir/mod.rs index e4f000778d1..c631edfa889 100644 --- a/compiler/noirc_frontend/src/hir/mod.rs +++ b/compiler/noirc_frontend/src/hir/mod.rs @@ -272,14 +272,14 @@ impl Context<'_, '_> { /// Each result is returned in a list rather than returned as a single result as to allow /// definition collection to provide an error for each ill-formed numeric generic. pub(crate) fn resolve_generics( - &mut self, + interner: &NodeInterner, generics: &UnresolvedGenerics, errors: &mut Vec<(CompilationError, FileId)>, file_id: FileId, ) -> Generics { vecmap(generics, |generic| { // Map the generic to a fresh type variable - let id = self.def_interner.next_type_variable_id(); + let id = interner.next_type_variable_id(); let type_var = TypeVariable::unbound(id); let ident = generic.ident(); let span = ident.0.span(); diff --git a/docs/docs/noir/concepts/comptime.md b/docs/docs/noir/concepts/comptime.md index ed55a541fbd..ba078c763d0 100644 --- a/docs/docs/noir/concepts/comptime.md +++ b/docs/docs/noir/concepts/comptime.md @@ -183,7 +183,7 @@ comptime fn my_function_annotation(f: FunctionDefinition) { Anything returned from one of these functions will be inserted at top-level along with the original item. Note that expressions are not valid at top-level so you'll get an error trying to return `3` or similar just as if you tried to write a program containing `3; struct Foo {}`. -You can insert other top-level items such as traits, structs, or functions this way though. +You can insert other top-level items such as trait impls, structs, or functions this way though. For example, this is the mechanism used to insert additional trait implementations into the program when deriving a trait impl from a struct: #include_code derive-field-count-example noir_stdlib/src/meta/mod.nr rust diff --git a/test_programs/compile_success_empty/unquote_struct/Nargo.toml b/test_programs/compile_success_empty/unquote_struct/Nargo.toml new file mode 100644 index 00000000000..c40d6a07093 --- /dev/null +++ b/test_programs/compile_success_empty/unquote_struct/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "unquote_struct" +type = "bin" +authors = [""] +compiler_version = ">=0.33.0" + +[dependencies] diff --git a/test_programs/compile_success_empty/unquote_struct/src/main.nr b/test_programs/compile_success_empty/unquote_struct/src/main.nr new file mode 100644 index 00000000000..e90711dd710 --- /dev/null +++ b/test_programs/compile_success_empty/unquote_struct/src/main.nr @@ -0,0 +1,28 @@ +fn main() { + let foo = Foo { x: 4, y: 4 }; + foo.assert_equal(); +} + +#[output_struct] +fn foo(x: Field, y: u32) -> u32 { + x as u32 + y +} + +// Given a function, wrap its parameters in a struct definition +comptime fn output_struct(f: FunctionDefinition) -> Quoted { + let fields = f.parameters().map(|param: (Quoted, Type)| { + let name = param.0; + let typ = param.1; + quote { $name: $typ, } + }).join(quote {}); + + quote { + struct Foo { $fields } + + impl Foo { + fn assert_equal(self) { + assert_eq(self.x as u32, self.y); + } + } + } +} From 34f21c0eadfc8a03f5177d72de7958903de8ac98 Mon Sep 17 00:00:00 2001 From: jfecher Date: Wed, 4 Sep 2024 14:38:06 -0500 Subject: [PATCH 10/25] fix: Support debug comptime flag for attributes (#5929) # Description ## Problem\* Resolves https://github.com/noir-lang/noir/issues/5915 ## Summary\* We were just missing a quick check after running attributes. Now their output will be shown when the `--debug-comptime-in-file` flag is used ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- compiler/noirc_frontend/src/elaborator/comptime.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/compiler/noirc_frontend/src/elaborator/comptime.rs b/compiler/noirc_frontend/src/elaborator/comptime.rs index e8dbf2ec775..cf6679af8e9 100644 --- a/compiler/noirc_frontend/src/elaborator/comptime.rs +++ b/compiler/noirc_frontend/src/elaborator/comptime.rs @@ -175,6 +175,8 @@ impl<'context> Elaborator<'context> { .call_function(function, arguments, TypeBindings::new(), location) .map_err(|error| error.into_compilation_error_pair())?; + self.debug_comptime(location, |interner| value.display(interner).to_string()); + if value != Value::Unit { let items = value .into_top_level_items(location, self.interner) From 2c22fe555dc41fffc623026b4b8c57d44b869cd2 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 4 Sep 2024 17:39:58 -0300 Subject: [PATCH 11/25] fix: collect functions generated by attributes (#5930) # Description ## Problem Resolves #5901 ## Summary ## Additional Context ## Documentation Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: jfecher --- .../noirc_frontend/src/elaborator/comptime.rs | 29 +++---- .../src/hir/def_collector/dc_mod.rs | 75 +++++++++++-------- .../unquote_function/Nargo.toml | 7 ++ .../unquote_function/src/main.nr | 12 +++ .../unquote_struct/src/main.nr | 6 +- 5 files changed, 82 insertions(+), 47 deletions(-) create mode 100644 test_programs/compile_success_empty/unquote_function/Nargo.toml create mode 100644 test_programs/compile_success_empty/unquote_function/src/main.nr diff --git a/compiler/noirc_frontend/src/elaborator/comptime.rs b/compiler/noirc_frontend/src/elaborator/comptime.rs index cf6679af8e9..7da5efd0b5a 100644 --- a/compiler/noirc_frontend/src/elaborator/comptime.rs +++ b/compiler/noirc_frontend/src/elaborator/comptime.rs @@ -326,21 +326,24 @@ impl<'context> Elaborator<'context> { ) { match item { TopLevelStatement::Function(function) => { - let id = self.interner.push_empty_fn(); - let module = self.module_id(); - self.interner.push_function(id, &function.def, module, location); + let module_id = self.module_id(); - if self.interner.is_in_lsp_mode() && !function.def.is_test() { - self.interner.register_function(id, &function.def); + if let Some(id) = dc_mod::collect_function( + self.interner, + self.def_maps.get_mut(&self.crate_id).unwrap(), + &function, + module_id, + self.file, + &mut self.errors, + ) { + let functions = vec![(self.local_module, id, function)]; + generated_items.functions.push(UnresolvedFunctions { + file_id: self.file, + functions, + trait_id: None, + self_type: None, + }); } - - let functions = vec![(self.local_module, id, function)]; - generated_items.functions.push(UnresolvedFunctions { - file_id: self.file, - functions, - trait_id: None, - self_type: None, - }); } TopLevelStatement::TraitImpl(mut trait_impl) => { let (methods, associated_types, associated_constants) = diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index d6432b0ca56..79b55f60b76 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -248,25 +248,16 @@ impl<'a> ModCollector<'a> { let module = ModuleId { krate, local_id: self.module_id }; for function in functions { - // check if optional field attribute is compatible with native field - if let Some(field) = function.attributes().get_field_attribute() { - if !is_native_field(&field) { - continue; - } - } - - let name = function.name_ident().clone(); - let func_id = context.def_interner.push_empty_fn(); - let visibility = function.def.visibility; - - // First create dummy function in the DefInterner - // So that we can get a FuncId - let location = Location::new(function.span(), self.file_id); - context.def_interner.push_function(func_id, &function.def, module, location); - - if context.def_interner.is_in_lsp_mode() && !function.def.is_test() { - context.def_interner.register_function(func_id, &function.def); - } + let Some(func_id) = collect_function( + &mut context.def_interner, + &mut self.def_collector.def_map, + &function, + module, + self.file_id, + &mut errors, + ) else { + continue; + }; // Now link this func_id to a crate level map with the noir function and the module id // Encountering a NoirFunction, we retrieve it's module_data to get the namespace @@ -275,19 +266,6 @@ impl<'a> ModCollector<'a> { // With this method we iterate each function in the Crate and not each module // This may not be great because we have to pull the module_data for each function unresolved_functions.push_fn(self.module_id, func_id, function); - - // Add function to scope/ns of the module - let result = self.def_collector.def_map.modules[self.module_id.0] - .declare_function(name, visibility, func_id); - - if let Err((first_def, second_def)) = result { - let error = DefCollectorErrorKind::Duplicate { - typ: DuplicateType::Function, - first_def, - second_def, - }; - errors.push((error.into(), self.file_id)); - } } self.def_collector.items.functions.push(unresolved_functions); @@ -842,6 +820,39 @@ fn push_child_module( Ok(mod_id) } +pub fn collect_function( + interner: &mut NodeInterner, + def_map: &mut CrateDefMap, + function: &NoirFunction, + module: ModuleId, + file: FileId, + errors: &mut Vec<(CompilationError, FileId)>, +) -> Option { + if let Some(field) = function.attributes().get_field_attribute() { + if !is_native_field(&field) { + return None; + } + } + let name = function.name_ident().clone(); + let func_id = interner.push_empty_fn(); + let visibility = function.def.visibility; + let location = Location::new(function.span(), file); + interner.push_function(func_id, &function.def, module, location); + if interner.is_in_lsp_mode() && !function.def.is_test() { + interner.register_function(func_id, &function.def); + } + let result = def_map.modules[module.local_id.0].declare_function(name, visibility, func_id); + if let Err((first_def, second_def)) = result { + let error = DefCollectorErrorKind::Duplicate { + typ: DuplicateType::Function, + first_def, + second_def, + }; + errors.push((error.into(), file)); + } + Some(func_id) +} + pub fn collect_struct( interner: &mut NodeInterner, def_map: &mut CrateDefMap, diff --git a/test_programs/compile_success_empty/unquote_function/Nargo.toml b/test_programs/compile_success_empty/unquote_function/Nargo.toml new file mode 100644 index 00000000000..aa56a5798df --- /dev/null +++ b/test_programs/compile_success_empty/unquote_function/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "unquote_function" +type = "bin" +authors = [""] +compiler_version = ">=0.33.0" + +[dependencies] diff --git a/test_programs/compile_success_empty/unquote_function/src/main.nr b/test_programs/compile_success_empty/unquote_function/src/main.nr new file mode 100644 index 00000000000..273a091b26d --- /dev/null +++ b/test_programs/compile_success_empty/unquote_function/src/main.nr @@ -0,0 +1,12 @@ +fn main() { + bar(); +} + +#[output_function] +fn foo() {} + +comptime fn output_function(_f: FunctionDefinition) -> Quoted { + quote { + fn bar() {} + } +} diff --git a/test_programs/compile_success_empty/unquote_struct/src/main.nr b/test_programs/compile_success_empty/unquote_struct/src/main.nr index e90711dd710..603440b5c76 100644 --- a/test_programs/compile_success_empty/unquote_struct/src/main.nr +++ b/test_programs/compile_success_empty/unquote_struct/src/main.nr @@ -10,11 +10,13 @@ fn foo(x: Field, y: u32) -> u32 { // Given a function, wrap its parameters in a struct definition comptime fn output_struct(f: FunctionDefinition) -> Quoted { - let fields = f.parameters().map(|param: (Quoted, Type)| { + let fields = f.parameters().map( + |param: (Quoted, Type)| { let name = param.0; let typ = param.1; quote { $name: $typ, } - }).join(quote {}); + } + ).join(quote {}); quote { struct Foo { $fields } From f18e9ca86c025f736af6e515f812e36fbb622930 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 4 Sep 2024 18:06:14 -0300 Subject: [PATCH 12/25] feat: add `fmtstr::contents` (#5928) # Description ## Problem Resolves #5899 Resolves #5914 ## Summary Two things here: 1. When interpolating quotes values inside a format string, we do that without producing the `quote {` and `}` parts, which is likely what a user would expect (similar to unquoting those values). 2. In order to create identifiers (or any piece of code in general) by joining severa quoted values you can use format strings together with the new `fmtstr::contents` method, which returns a `Quoted` value with the string contents (that is, without the leading and trailing double quotes). ## Additional Context I originally thought about a method like `fmtstr::as_identifier` that would try to parse the string contents as an identifier (maybe an `Ident`, or maybe a `Path`), returning `Option` and `None` in case it couldn't be parsed to that. But I think in general it could be more useful to just get the string contents as a `Quoted` value. After all, if it isn't an identifier you'll learn it later on once the value is unquoted or interpolated. ## Documentation Check one: - [ ] No documentation needed. - [x] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../src/hir/comptime/interpreter.rs | 14 +++- .../src/hir/comptime/interpreter/builtin.rs | 25 ++++++- .../interpreter/builtin/builtin_helpers.rs | 14 ++++ .../noirc_frontend/src/hir/comptime/value.rs | 73 ++++++++++++------- docs/docs/noir/standard_library/fmtstr.md | 13 ++++ noir_stdlib/src/meta/format_string.nr | 6 ++ noir_stdlib/src/meta/mod.nr | 1 + .../comptime_fmt_strings/src/main.nr | 14 ++++ tooling/lsp/src/requests/completion/tests.rs | 2 +- 9 files changed, 130 insertions(+), 32 deletions(-) create mode 100644 docs/docs/noir/standard_library/fmtstr.md create mode 100644 noir_stdlib/src/meta/format_string.nr diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs index d8e62b66eca..9f559b7c5e6 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter.rs @@ -586,7 +586,19 @@ impl<'local, 'interner> Interpreter<'local, 'interner> { consuming = false; if let Some(value) = values.pop_front() { - result.push_str(&value.display(self.elaborator.interner).to_string()); + // When interpolating a quoted value inside a format string, we don't include the + // surrounding `quote {` ... `}` as if we are unquoting the quoted value inside the string. + if let Value::Quoted(tokens) = value { + for (index, token) in tokens.iter().enumerate() { + if index > 0 { + result.push(' '); + } + result + .push_str(&token.display(self.elaborator.interner).to_string()); + } + } else { + result.push_str(&value.display(self.elaborator.interner).to_string()); + } } } other if !consuming => { diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index d1fcc76c55b..d2c9e4ffc0c 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -7,9 +7,9 @@ use acvm::{AcirField, FieldElement}; use builtin_helpers::{ block_expression_to_value, check_argument_count, check_function_not_yet_resolved, check_one_argument, check_three_arguments, check_two_arguments, get_expr, get_field, - get_function_def, get_module, get_quoted, get_slice, get_struct, get_trait_constraint, - get_trait_def, get_trait_impl, get_tuple, get_type, get_typed_expr, get_u32, - get_unresolved_type, hir_pattern_to_tokens, mutate_func_meta_type, parse, + get_format_string, get_function_def, get_module, get_quoted, get_slice, get_struct, + get_trait_constraint, get_trait_def, get_trait_impl, get_tuple, get_type, get_typed_expr, + get_u32, get_unresolved_type, hir_pattern_to_tokens, mutate_func_meta_type, parse, replace_func_meta_parameters, replace_func_meta_return_type, }; use chumsky::{prelude::choice, Parser}; @@ -32,6 +32,7 @@ use crate::{ InterpreterError, Value, }, hir_def::function::FunctionBody, + lexer::Lexer, macros_api::{HirExpression, HirLiteral, ModuleDefId, NodeInterner, Signedness}, node_interner::{DefinitionKind, TraitImplKind}, parser::{self}, @@ -95,6 +96,7 @@ impl<'local, 'context> Interpreter<'local, 'context> { "expr_is_continue" => expr_is_continue(interner, arguments, location), "expr_resolve" => expr_resolve(self, arguments, location), "is_unconstrained" => Ok(Value::Bool(true)), + "fmtstr_quoted_contents" => fmtstr_quoted_contents(interner, arguments, location), "function_def_body" => function_def_body(interner, arguments, location), "function_def_has_named_attribute" => { function_def_has_named_attribute(interner, arguments, location) @@ -1576,6 +1578,23 @@ fn unwrap_expr_value(interner: &NodeInterner, mut expr_value: ExprValue) -> Expr expr_value } +// fn quoted_contents(self) -> Quoted +fn fmtstr_quoted_contents( + interner: &NodeInterner, + arguments: Vec<(Value, Location)>, + location: Location, +) -> IResult { + let self_argument = check_one_argument(arguments, location)?; + let (string, _) = get_format_string(interner, self_argument)?; + let (tokens, _) = Lexer::lex(&string); + let mut tokens: Vec<_> = tokens.0.into_iter().map(|token| token.into_token()).collect(); + if let Some(Token::EOF) = tokens.last() { + tokens.pop(); + } + + Ok(Value::Quoted(Rc::new(tokens))) +} + // fn body(self) -> Expr fn function_def_body( interner: &NodeInterner, diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin/builtin_helpers.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin/builtin_helpers.rs index 14a0e177544..ff3da6d253f 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin/builtin_helpers.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin/builtin_helpers.rs @@ -189,6 +189,20 @@ pub(crate) fn get_expr( } } +pub(crate) fn get_format_string( + interner: &NodeInterner, + (value, location): (Value, Location), +) -> IResult<(Rc, Type)> { + match value { + Value::FormatString(value, typ) => Ok((value, typ)), + value => { + let n = Box::new(interner.next_type_variable()); + let e = Box::new(interner.next_type_variable()); + type_mismatch(value, Type::FmtString(n, e), location) + } + } +} + pub(crate) fn get_function_def((value, location): (Value, Location)) -> IResult { match value { Value::FunctionDefinition(id) => Ok(id), diff --git a/compiler/noirc_frontend/src/hir/comptime/value.rs b/compiler/noirc_frontend/src/hir/comptime/value.rs index fdac95a07fe..7d6e4475c7b 100644 --- a/compiler/noirc_frontend/src/hir/comptime/value.rs +++ b/compiler/noirc_frontend/src/hir/comptime/value.rs @@ -605,33 +605,7 @@ impl<'value, 'interner> Display for ValuePrinter<'value, 'interner> { write!(f, "quote {{")?; for token in tokens.iter() { write!(f, " ")?; - - match token { - Token::QuotedType(id) => { - write!(f, "{}", self.interner.get_quoted_type(*id))?; - } - Token::InternedExpr(id) => { - let value = Value::expression(ExpressionKind::Interned(*id)); - value.display(self.interner).fmt(f)?; - } - Token::InternedStatement(id) => { - let value = Value::statement(StatementKind::Interned(*id)); - value.display(self.interner).fmt(f)?; - } - Token::InternedLValue(id) => { - let value = Value::lvalue(LValue::Interned(*id, Span::default())); - value.display(self.interner).fmt(f)?; - } - Token::InternedUnresolvedTypeData(id) => { - let value = Value::UnresolvedType(UnresolvedTypeData::Interned(*id)); - value.display(self.interner).fmt(f)?; - } - Token::UnquoteMarker(id) => { - let value = Value::TypedExpr(TypedExpr::ExprId(*id)); - value.display(self.interner).fmt(f)?; - } - other => write!(f, "{other}")?, - } + token.display(self.interner).fmt(f)?; } write!(f, " }}") } @@ -713,6 +687,51 @@ impl<'value, 'interner> Display for ValuePrinter<'value, 'interner> { } } +impl Token { + pub fn display<'token, 'interner>( + &'token self, + interner: &'interner NodeInterner, + ) -> TokenPrinter<'token, 'interner> { + TokenPrinter { token: self, interner } + } +} + +pub struct TokenPrinter<'token, 'interner> { + token: &'token Token, + interner: &'interner NodeInterner, +} + +impl<'token, 'interner> Display for TokenPrinter<'token, 'interner> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self.token { + Token::QuotedType(id) => { + write!(f, "{}", self.interner.get_quoted_type(*id)) + } + Token::InternedExpr(id) => { + let value = Value::expression(ExpressionKind::Interned(*id)); + value.display(self.interner).fmt(f) + } + Token::InternedStatement(id) => { + let value = Value::statement(StatementKind::Interned(*id)); + value.display(self.interner).fmt(f) + } + Token::InternedLValue(id) => { + let value = Value::lvalue(LValue::Interned(*id, Span::default())); + value.display(self.interner).fmt(f) + } + Token::InternedUnresolvedTypeData(id) => { + let value = Value::UnresolvedType(UnresolvedTypeData::Interned(*id)); + value.display(self.interner).fmt(f) + } + Token::UnquoteMarker(id) => { + let value = Value::TypedExpr(TypedExpr::ExprId(*id)); + value.display(self.interner).fmt(f) + } + other => write!(f, "{other}"), + } + } +} + fn display_trait_constraint(interner: &NodeInterner, trait_constraint: &TraitConstraint) -> String { let trait_ = interner.get_trait(trait_constraint.trait_id); format!("{}: {}{}", trait_constraint.typ, trait_.name, trait_constraint.trait_generics) diff --git a/docs/docs/noir/standard_library/fmtstr.md b/docs/docs/noir/standard_library/fmtstr.md new file mode 100644 index 00000000000..293793e23ff --- /dev/null +++ b/docs/docs/noir/standard_library/fmtstr.md @@ -0,0 +1,13 @@ +--- +title: fmtstr +--- + +`fmtstr` is the type resulting from using format string (`f"..."`). + +## Methods + +### quoted_contents + +#include_code quoted_contents noir_stdlib/src/meta/format_string.nr rust + +Returns the format string contents (that is, without the leading and trailing double quotes) as a `Quoted` value. \ No newline at end of file diff --git a/noir_stdlib/src/meta/format_string.nr b/noir_stdlib/src/meta/format_string.nr new file mode 100644 index 00000000000..44b69719efe --- /dev/null +++ b/noir_stdlib/src/meta/format_string.nr @@ -0,0 +1,6 @@ +impl fmtstr { + #[builtin(fmtstr_quoted_contents)] + // docs:start:quoted_contents + fn quoted_contents(self) -> Quoted {} + // docs:end:quoted_contents +} diff --git a/noir_stdlib/src/meta/mod.nr b/noir_stdlib/src/meta/mod.nr index 24398054467..9fc399ddbf9 100644 --- a/noir_stdlib/src/meta/mod.nr +++ b/noir_stdlib/src/meta/mod.nr @@ -1,4 +1,5 @@ mod expr; +mod format_string; mod function_def; mod module; mod op; diff --git a/test_programs/compile_success_empty/comptime_fmt_strings/src/main.nr b/test_programs/compile_success_empty/comptime_fmt_strings/src/main.nr index 705a1b2ab4e..0e2d459a00f 100644 --- a/test_programs/compile_success_empty/comptime_fmt_strings/src/main.nr +++ b/test_programs/compile_success_empty/comptime_fmt_strings/src/main.nr @@ -12,4 +12,18 @@ fn main() { }; assert_eq(s1, "x is 4, fake interpolation: {y}, y is 5"); assert_eq(s2, "\0\0\0\0"); + + // Mainly test fmtstr::quoted_contents + call!(glue(quote { hello }, quote { world })); +} + +fn glue(x: Quoted, y: Quoted) -> Quoted { + f"{x}_{y}".quoted_contents() } + +fn hello_world() {} + +comptime fn call(x: Quoted) -> Quoted { + quote { $x() } +} + diff --git a/tooling/lsp/src/requests/completion/tests.rs b/tooling/lsp/src/requests/completion/tests.rs index ca959f5d5ca..a7cfa77a73d 100644 --- a/tooling/lsp/src/requests/completion/tests.rs +++ b/tooling/lsp/src/requests/completion/tests.rs @@ -336,7 +336,7 @@ mod completion_tests { fo>|< } "#; - assert_completion(src, vec![module_completion_item("foobar")]).await; + assert_completion_excluding_auto_import(src, vec![module_completion_item("foobar")]).await; } #[test] From af3db4bf2e8f7feba6d06c3095d7cdf17c8dde75 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Wed, 4 Sep 2024 19:03:52 -0300 Subject: [PATCH 13/25] feat: warn on unused functions (#5892) # Description ## Problem Now that we warn on unused imports, doing that for functions too is relatively straight-forward. ## Summary Now unused private or `pub(crate)` functions will be reported as unused. ## Additional Context I'd like to try this on some Aztec-Packages projects, but for that it would be nice to merge #5895 first so I could get all warnings/errors for a package inside VS Code. We can eventually do the same thing with globals, traits, etc., once we track their visibility. I also think we could warn on unused `pub` functions in a "bin" or "contract" package, not sure... but if we decide to do that, it could be a separate PR. ## Documentation Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../src/hir/def_collector/dc_crate.rs | 40 +++++---- .../src/hir/def_collector/dc_mod.rs | 20 ++++- .../src/hir/resolution/errors.rs | 10 +-- compiler/noirc_frontend/src/node_interner.rs | 2 +- compiler/noirc_frontend/src/tests.rs | 84 +++++++++++++------ compiler/noirc_frontend/src/usage_tracker.rs | 52 +++++++++--- 6 files changed, 144 insertions(+), 64 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 98555375790..3cfa0989d7d 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -8,6 +8,7 @@ use crate::hir::resolution::errors::ResolverError; use crate::hir::resolution::path_resolver; use crate::hir::type_check::TypeCheckError; use crate::token::SecondaryAttribute; +use crate::usage_tracker::UnusedItem; use crate::{Generics, Type}; use crate::hir::resolution::import::{resolve_import, ImportDirective, PathResolution}; @@ -271,7 +272,7 @@ impl DefCollector { root_file_id: FileId, debug_comptime_in_file: Option<&str>, enable_arithmetic_generics: bool, - error_on_usage_tracker: bool, + error_on_unused_items: bool, macro_processors: &[&dyn MacroProcessor], ) -> Vec<(CompilationError, FileId)> { let mut errors: Vec<(CompilationError, FileId)> = vec![]; @@ -406,20 +407,14 @@ impl DefCollector { let result = current_def_map.modules[resolved_import.module_scope.0] .import(name.clone(), visibility, module_def_id, is_prelude); - // Empty spans could come from implicitly injected imports, and we don't want to track those - if visibility != ItemVisibility::Public - && name.span().start() < name.span().end() - { - let module_id = ModuleId { - krate: crate_id, - local_id: resolved_import.module_scope, - }; - - context - .def_interner - .usage_tracker - .add_unused_import(module_id, name.clone()); - } + let module_id = + ModuleId { krate: crate_id, local_id: resolved_import.module_scope }; + context.def_interner.usage_tracker.add_unused_item( + module_id, + name.clone(), + UnusedItem::Import, + visibility, + ); if visibility != ItemVisibility::Private { let local_id = resolved_import.module_scope; @@ -494,26 +489,29 @@ impl DefCollector { ); } - if error_on_usage_tracker { - Self::check_usage_tracker(context, crate_id, &mut errors); + if error_on_unused_items { + Self::check_unused_items(context, crate_id, &mut errors); } errors } - fn check_usage_tracker( + fn check_unused_items( context: &Context, crate_id: CrateId, errors: &mut Vec<(CompilationError, FileId)>, ) { - let unused_imports = context.def_interner.usage_tracker.unused_imports().iter(); + let unused_imports = context.def_interner.usage_tracker.unused_items().iter(); let unused_imports = unused_imports.filter(|(module_id, _)| module_id.krate == crate_id); errors.extend(unused_imports.flat_map(|(module_id, usage_tracker)| { let module = &context.def_maps[&crate_id].modules()[module_id.local_id.0]; - usage_tracker.iter().map(|ident| { + usage_tracker.iter().map(|(ident, unused_item)| { let ident = ident.clone(); - let error = CompilationError::ResolverError(ResolverError::UnusedImport { ident }); + let error = CompilationError::ResolverError(ResolverError::UnusedItem { + ident, + item_type: unused_item.item_type(), + }); (error, module.location.file) }) })); diff --git a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 79b55f60b76..6c1b7632a2e 100644 --- a/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -18,6 +18,7 @@ use crate::hir::resolution::errors::ResolverError; use crate::macros_api::{Expression, NodeInterner, StructId, UnresolvedType, UnresolvedTypeData}; use crate::node_interner::ModuleAttributes; use crate::token::SecondaryAttribute; +use crate::usage_tracker::UnusedItem; use crate::{ graph::CrateId, hir::def_collector::dc_crate::{UnresolvedStruct, UnresolvedTrait}, @@ -36,7 +37,7 @@ use super::{ }, errors::{DefCollectorErrorKind, DuplicateType}, }; -use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleData, ModuleId}; +use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleData, ModuleId, MAIN_FUNCTION}; use crate::hir::resolution::import::ImportDirective; use crate::hir::Context; @@ -833,6 +834,16 @@ pub fn collect_function( return None; } } + + let module_data = &mut def_map.modules[module.local_id.0]; + + let is_test = function.def.attributes.is_test_function(); + let is_entry_point_function = if module_data.is_contract { + function.attributes().is_contract_entry_point() + } else { + function.name() == MAIN_FUNCTION + }; + let name = function.name_ident().clone(); let func_id = interner.push_empty_fn(); let visibility = function.def.visibility; @@ -841,6 +852,13 @@ pub fn collect_function( if interner.is_in_lsp_mode() && !function.def.is_test() { interner.register_function(func_id, &function.def); } + + if !is_test && !is_entry_point_function { + let item = UnusedItem::Function(func_id); + interner.usage_tracker.add_unused_item(module, name.clone(), item, visibility); + } + + // Add function to scope/ns of the module let result = def_map.modules[module.local_id.0].declare_function(name, visibility, func_id); if let Err((first_def, second_def)) = result { let error = DefCollectorErrorKind::Duplicate { diff --git a/compiler/noirc_frontend/src/hir/resolution/errors.rs b/compiler/noirc_frontend/src/hir/resolution/errors.rs index c2038c646b5..e74468bdf18 100644 --- a/compiler/noirc_frontend/src/hir/resolution/errors.rs +++ b/compiler/noirc_frontend/src/hir/resolution/errors.rs @@ -20,8 +20,8 @@ pub enum ResolverError { DuplicateDefinition { name: String, first_span: Span, second_span: Span }, #[error("Unused variable")] UnusedVariable { ident: Ident }, - #[error("Unused import")] - UnusedImport { ident: Ident }, + #[error("Unused {item_type}")] + UnusedItem { ident: Ident, item_type: &'static str }, #[error("Could not find variable in this scope")] VariableNotDeclared { name: String, span: Span }, #[error("path is not an identifier")] @@ -158,12 +158,12 @@ impl<'a> From<&'a ResolverError> for Diagnostic { diagnostic.unnecessary = true; diagnostic } - ResolverError::UnusedImport { ident } => { + ResolverError::UnusedItem { ident, item_type } => { let name = &ident.0.contents; let mut diagnostic = Diagnostic::simple_warning( - format!("unused import {name}"), - "unused import ".to_string(), + format!("unused {item_type} {name}"), + format!("unused {item_type}"), ident.span(), ); diagnostic.unnecessary = true; diff --git a/compiler/noirc_frontend/src/node_interner.rs b/compiler/noirc_frontend/src/node_interner.rs index 4a73df6a15f..aa51779d24b 100644 --- a/compiler/noirc_frontend/src/node_interner.rs +++ b/compiler/noirc_frontend/src/node_interner.rs @@ -655,7 +655,7 @@ impl Default for NodeInterner { auto_import_names: HashMap::default(), comptime_scopes: vec![HashMap::default()], trait_impl_associated_types: HashMap::default(), - usage_tracker: UsageTracker::default(), + usage_tracker: UsageTracker::new(), } } } diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index c6c8c5d4b4b..04c4e414858 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -1381,7 +1381,7 @@ fn ban_mutable_globals() { fn deny_inline_attribute_on_unconstrained() { let src = r#" #[no_predicates] - unconstrained fn foo(x: Field, y: Field) { + unconstrained pub fn foo(x: Field, y: Field) { assert(x != y); } "#; @@ -1397,7 +1397,7 @@ fn deny_inline_attribute_on_unconstrained() { fn deny_fold_attribute_on_unconstrained() { let src = r#" #[fold] - unconstrained fn foo(x: Field, y: Field) { + unconstrained pub fn foo(x: Field, y: Field) { assert(x != y); } "#; @@ -1554,7 +1554,7 @@ fn struct_numeric_generic_in_function() { inner: u64 } - fn bar() { } + pub fn bar() { } "#; let errors = get_program_errors(src); assert_eq!(errors.len(), 1); @@ -1586,7 +1586,7 @@ fn struct_numeric_generic_in_struct() { #[test] fn bool_numeric_generic() { let src = r#" - fn read() -> Field { + pub fn read() -> Field { if N { 0 } else { @@ -1605,7 +1605,7 @@ fn bool_numeric_generic() { #[test] fn numeric_generic_binary_operation_type_mismatch() { let src = r#" - fn foo() -> bool { + pub fn foo() -> bool { let mut check: bool = true; check = N; check @@ -1622,7 +1622,7 @@ fn numeric_generic_binary_operation_type_mismatch() { #[test] fn bool_generic_as_loop_bound() { let src = r#" - fn read() { + pub fn read() { let mut fields = [0; N]; for i in 0..N { fields[i] = i + 1; @@ -1652,7 +1652,7 @@ fn bool_generic_as_loop_bound() { #[test] fn numeric_generic_in_function_signature() { let src = r#" - fn foo(arr: [Field; N]) -> [Field; N] { arr } + pub fn foo(arr: [Field; N]) -> [Field; N] { arr } "#; assert_no_errors(src); } @@ -1694,7 +1694,7 @@ fn normal_generic_as_array_length() { #[test] fn numeric_generic_as_param_type() { let src = r#" - fn foo(x: I) -> I { + pub fn foo(x: I) -> I { let _q: I = 5; x } @@ -1833,7 +1833,7 @@ fn numeric_generic_used_in_where_clause() { fn deserialize(fields: [Field; N]) -> Self; } - fn read() -> T where T: Deserialize { + pub fn read() -> T where T: Deserialize { let mut fields: [Field; N] = [0; N]; for i in 0..N { fields[i] = i as Field + 1; @@ -1847,12 +1847,12 @@ fn numeric_generic_used_in_where_clause() { #[test] fn numeric_generic_used_in_turbofish() { let src = r#" - fn double() -> u32 { + pub fn double() -> u32 { // Used as an expression N * 2 } - fn double_numeric_generics_test() { + pub fn double_numeric_generics_test() { // Example usage of a numeric generic arguments. assert(double::<9>() == 18); assert(double::<7 + 8>() == 30); @@ -1888,7 +1888,7 @@ fn normal_generic_used_when_numeric_expected_in_where_clause() { fn deserialize(fields: [Field; N]) -> Self; } - fn read() -> T where T: Deserialize { + pub fn read() -> T where T: Deserialize { T::deserialize([0, 1]) } "#; @@ -1904,7 +1904,7 @@ fn normal_generic_used_when_numeric_expected_in_where_clause() { fn deserialize(fields: [Field; N]) -> Self; } - fn read() -> T where T: Deserialize { + pub fn read() -> T where T: Deserialize { let mut fields: [Field; N] = [0; N]; for i in 0..N { fields[i] = i as Field + 1; @@ -2450,7 +2450,7 @@ fn use_super() { mod foo { use super::some_func; - fn bar() { + pub fn bar() { some_func(); } } @@ -2464,7 +2464,7 @@ fn use_super_in_path() { fn some_func() {} mod foo { - fn func() { + pub fn func() { super::some_func(); } } @@ -2755,7 +2755,7 @@ fn trait_constraint_on_tuple_type() { fn foo(self, x: A) -> bool; } - fn bar(x: (T, U), y: V) -> bool where (T, U): Foo { + pub fn bar(x: (T, U), y: V) -> bool where (T, U): Foo { x.foo(y) } @@ -3091,7 +3091,7 @@ fn trait_impl_for_a_type_that_implements_another_trait() { } } - fn use_it(t: T) -> i32 where T: Two { + pub fn use_it(t: T) -> i32 where T: Two { Two::two(t) } @@ -3131,7 +3131,7 @@ fn trait_impl_for_a_type_that_implements_another_trait_with_another_impl_used() } } - fn use_it(t: u32) -> i32 { + pub fn use_it(t: u32) -> i32 { Two::two(t) } @@ -3243,12 +3243,14 @@ fn errors_on_unused_private_import() { let errors = get_program_errors(src); assert_eq!(errors.len(), 1); - let CompilationError::ResolverError(ResolverError::UnusedImport { ident }) = &errors[0].0 + let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item_type }) = + &errors[0].0 else { - panic!("Expected an unused import error"); + panic!("Expected an unused item error"); }; assert_eq!(ident.to_string(), "bar"); + assert_eq!(*item_type, "import"); } #[test] @@ -3277,12 +3279,14 @@ fn errors_on_unused_pub_crate_import() { let errors = get_program_errors(src); assert_eq!(errors.len(), 1); - let CompilationError::ResolverError(ResolverError::UnusedImport { ident }) = &errors[0].0 + let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item_type }) = + &errors[0].0 else { - panic!("Expected an unused import error"); + panic!("Expected an unused item error"); }; assert_eq!(ident.to_string(), "bar"); + assert_eq!(*item_type, "import"); } #[test] @@ -3295,7 +3299,7 @@ fn warns_on_use_of_private_exported_item() { use bar::baz; - fn qux() { + pub fn qux() { baz(); } } @@ -3369,7 +3373,7 @@ fn unoquted_integer_as_integer_token() { } #[attr] - fn foobar() {} + pub fn foobar() {} fn attr(_f: FunctionDefinition) -> Quoted { let serialized_len = 1; @@ -3388,3 +3392,35 @@ fn unoquted_integer_as_integer_token() { assert_no_errors(src); } + +#[test] +fn errors_on_unused_function() { + let src = r#" + contract some_contract { + // This function is unused, but it's a contract entrypoint + // so it should not produce a warning + fn foo() -> pub Field { + 1 + } + } + + + fn foo() { + bar(); + } + + fn bar() {} + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::ResolverError(ResolverError::UnusedItem { ident, item_type }) = + &errors[0].0 + else { + panic!("Expected an unused item error"); + }; + + assert_eq!(ident.to_string(), "foo"); + assert_eq!(*item_type, "function"); +} diff --git a/compiler/noirc_frontend/src/usage_tracker.rs b/compiler/noirc_frontend/src/usage_tracker.rs index d8b7b271734..836f9824436 100644 --- a/compiler/noirc_frontend/src/usage_tracker.rs +++ b/compiler/noirc_frontend/src/usage_tracker.rs @@ -1,26 +1,54 @@ -use std::collections::HashSet; +use std::collections::HashMap; -use rustc_hash::FxHashMap as HashMap; +use crate::{ + ast::{Ident, ItemVisibility}, + hir::def_map::ModuleId, + node_interner::FuncId, +}; -use crate::{ast::Ident, hir::def_map::ModuleId}; +#[derive(Debug)] +pub enum UnusedItem { + Import, + Function(FuncId), +} + +impl UnusedItem { + pub fn item_type(&self) -> &'static str { + match self { + UnusedItem::Import => "import", + UnusedItem::Function(_) => "function", + } + } +} -#[derive(Debug, Default)] +#[derive(Debug)] pub struct UsageTracker { - /// List of all unused imports in each module. Each time something is imported it's added - /// to the module's set. When it's used, it's removed. At the end of the program only unused imports remain. - unused_imports: HashMap>, + unused_items: HashMap>, } impl UsageTracker { - pub(crate) fn add_unused_import(&mut self, module_id: ModuleId, name: Ident) { - self.unused_imports.entry(module_id).or_default().insert(name); + pub(crate) fn new() -> Self { + Self { unused_items: HashMap::new() } + } + + pub(crate) fn add_unused_item( + &mut self, + module_id: ModuleId, + name: Ident, + item: UnusedItem, + visibility: ItemVisibility, + ) { + // Empty spans could come from implicitly injected imports, and we don't want to track those + if visibility != ItemVisibility::Public && name.span().start() < name.span().end() { + self.unused_items.entry(module_id).or_default().insert(name, item); + } } pub(crate) fn mark_as_used(&mut self, current_mod_id: ModuleId, name: &Ident) { - self.unused_imports.entry(current_mod_id).or_default().remove(name); + self.unused_items.entry(current_mod_id).or_default().remove(name); } - pub(crate) fn unused_imports(&self) -> &HashMap> { - &self.unused_imports + pub(crate) fn unused_items(&self) -> &HashMap> { + &self.unused_items } } From 416b29314d3a32f566a4cfa55921b4e229d9b786 Mon Sep 17 00:00:00 2001 From: guipublic <47281315+guipublic@users.noreply.github.com> Date: Thu, 5 Sep 2024 10:31:09 +0200 Subject: [PATCH 14/25] chore: error on false constraint (#5890) # Description ## Problem\* Report a bug during compilation when a constraint is false ## Summary\* It is done in acir-gen which ensures that all side effects are resolved. ## Additional Context The reported bug does not fail the compilation, because it will mess around compile/run-time failures and also for the Noir tests. We want a consistent handling of assert, whether it fails at compile-time, run-time, or test-time. ## Documentation\* Check one: - [X] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [X] I have tested the changes locally. - [X] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: jfecher --- compiler/noirc_evaluator/src/errors.rs | 3 +++ .../src/ssa/acir_gen/acir_ir/acir_variable.rs | 10 +++++++++- compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs | 1 + 3 files changed, 13 insertions(+), 1 deletion(-) diff --git a/compiler/noirc_evaluator/src/errors.rs b/compiler/noirc_evaluator/src/errors.rs index c4f56d032f9..bcd6865b721 100644 --- a/compiler/noirc_evaluator/src/errors.rs +++ b/compiler/noirc_evaluator/src/errors.rs @@ -88,6 +88,7 @@ impl From for FileDiagnostic { InternalBug::IndependentSubgraph { call_stack } => { ("There is no path from the output of this brillig call to either return values or inputs of the circuit, which creates an independent subgraph. This is quite likely a soundness vulnerability".to_string(),call_stack) } + InternalBug::AssertFailed { call_stack } => ("As a result, the compiled circuit is ensured to fail. Other assertions may also fail during execution".to_string(), call_stack) }; let call_stack = vecmap(call_stack, |location| location); let file_id = call_stack.last().map(|location| location.file).unwrap_or_default(); @@ -111,6 +112,8 @@ pub enum InternalWarning { pub enum InternalBug { #[error("Input to brillig function is in a separate subgraph to output")] IndependentSubgraph { call_stack: CallStack }, + #[error("Assertion is always false")] + AssertFailed { call_stack: CallStack }, } #[derive(Debug, PartialEq, Eq, Clone, Error)] diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs index a0be1ee19cf..d12d49784ec 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs @@ -2,7 +2,7 @@ use super::big_int::BigIntContext; use super::generated_acir::{BrilligStdlibFunc, GeneratedAcir, PLACEHOLDER_BRILLIG_INDEX}; use crate::brillig::brillig_gen::brillig_directive; use crate::brillig::brillig_ir::artifact::GeneratedBrillig; -use crate::errors::{InternalError, RuntimeError, SsaReport}; +use crate::errors::{InternalBug, InternalError, RuntimeError, SsaReport}; use crate::ssa::acir_gen::{AcirDynamicArray, AcirValue}; use crate::ssa::ir::dfg::CallStack; use crate::ssa::ir::types::Type as SsaType; @@ -126,6 +126,8 @@ pub(crate) struct AcirContext { big_int_ctx: BigIntContext, expression_width: ExpressionWidth, + + pub(crate) warnings: Vec, } impl AcirContext { @@ -518,6 +520,12 @@ impl AcirContext { self.mark_variables_equivalent(lhs, rhs)?; return Ok(()); } + if diff_expr.is_const() { + // Constraint is always false + self.warnings.push(SsaReport::Bug(InternalBug::AssertFailed { + call_stack: self.get_call_stack(), + })); + } self.acir_ir.assert_is_zero(diff_expr); if let Some(payload) = assert_message { diff --git a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 0360b15d950..15b44fde65d 100644 --- a/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -453,6 +453,7 @@ impl<'a> Context<'a> { } warnings.extend(return_warnings); + warnings.extend(self.acir_context.warnings.clone()); // Add the warnings from the alter Ssa passes Ok(self.acir_context.finish(input_witness, return_witnesses, warnings)) From 8b60bbc8082513e29f6573e5235e0a33fdd1517b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=81lvaro=20Rodr=C3=ADguez?= Date: Thu, 5 Sep 2024 12:47:20 +0200 Subject: [PATCH 15/25] feat: Only check array bounds in brillig if index is unsafe (#5938) # Description ## Problem\* Optimizes bounds checks ## Summary\* ## Additional Context ## Documentation\* Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../src/brillig/brillig_gen/brillig_block.rs | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index ef5fbce83d4..fe986089686 100644 --- a/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -633,7 +633,11 @@ impl<'block> BrilligBlock<'block> { }; let index_variable = self.convert_ssa_single_addr_value(*index, dfg); - self.validate_array_index(array_variable, index_variable); + + if !dfg.is_safe_index(*index, *array) { + self.validate_array_index(array_variable, index_variable); + } + self.retrieve_variable_from_array( array_pointer, index_variable, @@ -652,7 +656,11 @@ impl<'block> BrilligBlock<'block> { result_ids[0], dfg, ); - self.validate_array_index(source_variable, index_register); + + if !dfg.is_safe_index(*index, *array) { + self.validate_array_index(source_variable, index_register); + } + self.convert_ssa_array_set( source_variable, destination_variable, From 9d2629dd1bb28a8c2ecb4c33d26119da75d626c2 Mon Sep 17 00:00:00 2001 From: jfecher Date: Thu, 5 Sep 2024 06:17:16 -0500 Subject: [PATCH 16/25] feat: Add `StructDefinition::set_fields` (#5931) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description ## Problem\* Resolves https://github.com/noir-lang/noir/issues/5911 ## Summary\* Adds a function to set the fields on a struct type ## Additional Context Hyper-specific error messages can be fun. Here's the error issued when one of the field names isn't a valid identifier: ``` error: Quoted value in index 1 of this slice is not a valid field name ┌─ src/main.nr:25:18 │ 25 │ s.set_fields(fields); │ ------ `quote { foo bar }` is not a valid field name for `set_fields` │ ``` ## Documentation\* Check one: - [ ] No documentation needed. - [x] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: Ary Borenszweig --- .../noirc_frontend/src/hir/comptime/errors.rs | 13 ++++ .../src/hir/comptime/interpreter/builtin.rs | 61 ++++++++++++++++++- compiler/noirc_frontend/src/hir_def/types.rs | 1 - .../noir/standard_library/meta/struct_def.md | 29 +++++++++ noir_stdlib/src/meta/struct_def.nr | 9 +++ .../comptime_type_definition/src/main.nr | 13 ++++ 6 files changed, 124 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/comptime/errors.rs b/compiler/noirc_frontend/src/hir/comptime/errors.rs index 48efc08f463..f6585786eeb 100644 --- a/compiler/noirc_frontend/src/hir/comptime/errors.rs +++ b/compiler/noirc_frontend/src/hir/comptime/errors.rs @@ -202,6 +202,11 @@ pub enum InterpreterError { TypeAnnotationsNeededForMethodCall { location: Location, }, + ExpectedIdentForStructField { + value: String, + index: usize, + location: Location, + }, // These cases are not errors, they are just used to prevent us from running more code // until the loop can be resumed properly. These cases will never be displayed to users. @@ -269,6 +274,7 @@ impl InterpreterError { | InterpreterError::FailedToResolveTraitBound { location, .. } | InterpreterError::FunctionAlreadyResolved { location, .. } | InterpreterError::MultipleMatchingImpls { location, .. } + | InterpreterError::ExpectedIdentForStructField { location, .. } | InterpreterError::TypeAnnotationsNeededForMethodCall { location } => *location, InterpreterError::FailedToParseMacro { error, file, .. } => { @@ -566,6 +572,13 @@ impl<'a> From<&'a InterpreterError> for CustomDiagnostic { error.add_note(message.to_string()); error } + InterpreterError::ExpectedIdentForStructField { value, index, location } => { + let msg = format!( + "Quoted value in index {index} of this slice is not a valid field name" + ); + let secondary = format!("`{value}` is not a valid field name for `set_fields`"); + CustomDiagnostic::simple_error(msg, secondary, location.span) + } } } } diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index d2c9e4ffc0c..65c9c3f018d 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -33,7 +33,7 @@ use crate::{ }, hir_def::function::FunctionBody, lexer::Lexer, - macros_api::{HirExpression, HirLiteral, ModuleDefId, NodeInterner, Signedness}, + macros_api::{HirExpression, HirLiteral, Ident, ModuleDefId, NodeInterner, Signedness}, node_interner::{DefinitionKind, TraitImplKind}, parser::{self}, token::Token, @@ -133,6 +133,7 @@ impl<'local, 'context> Interpreter<'local, 'context> { "struct_def_as_type" => struct_def_as_type(interner, arguments, location), "struct_def_fields" => struct_def_fields(interner, arguments, location), "struct_def_generics" => struct_def_generics(interner, arguments, location), + "struct_def_set_fields" => struct_def_set_fields(interner, arguments, location), "to_le_radix" => to_le_radix(arguments, return_type, location), "trait_constraint_eq" => trait_constraint_eq(interner, arguments, location), "trait_constraint_hash" => trait_constraint_hash(interner, arguments, location), @@ -326,6 +327,64 @@ fn struct_def_fields( Ok(Value::Slice(fields, typ)) } +/// fn set_fields(self, new_fields: [(Quoted, Type)]) {} +/// Returns (name, type) pairs of each field of this StructDefinition +fn struct_def_set_fields( + interner: &mut NodeInterner, + arguments: Vec<(Value, Location)>, + location: Location, +) -> IResult { + let (the_struct, fields) = check_two_arguments(arguments, location)?; + let struct_id = get_struct(the_struct)?; + + let struct_def = interner.get_struct(struct_id); + let mut struct_def = struct_def.borrow_mut(); + + let field_location = fields.1; + let fields = get_slice(interner, fields)?.0; + + let new_fields = fields + .into_iter() + .flat_map(|field_pair| get_tuple(interner, (field_pair, field_location))) + .enumerate() + .map(|(index, mut field_pair)| { + if field_pair.len() == 2 { + let typ = field_pair.pop().unwrap(); + let name_value = field_pair.pop().unwrap(); + + let name_tokens = get_quoted((name_value.clone(), field_location))?; + let typ = get_type((typ, field_location))?; + + match name_tokens.first() { + Some(Token::Ident(name)) if name_tokens.len() == 1 => { + Ok((Ident::new(name.clone(), field_location.span), typ)) + } + _ => { + let value = name_value.display(interner).to_string(); + let location = field_location; + Err(InterpreterError::ExpectedIdentForStructField { + value, + index, + location, + }) + } + } + } else { + let type_var = interner.next_type_variable(); + let expected = Type::Tuple(vec![type_var.clone(), type_var]); + + let actual = + Type::Tuple(vecmap(&field_pair, |value| value.get_type().into_owned())); + + Err(InterpreterError::TypeMismatch { expected, actual, location }) + } + }) + .collect::, _>>()?; + + struct_def.set_fields(new_fields); + Ok(Value::Unit) +} + fn slice_remove( interner: &mut NodeInterner, arguments: Vec<(Value, Location)>, diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index 7b3d0d7a205..113a4fb3888 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/compiler/noirc_frontend/src/hir_def/types.rs @@ -259,7 +259,6 @@ impl StructType { /// created. Therefore, this method is used to set the fields once they /// become known. pub fn set_fields(&mut self, fields: Vec<(Ident, Type)>) { - assert!(self.fields.is_empty()); self.fields = fields; } diff --git a/docs/docs/noir/standard_library/meta/struct_def.md b/docs/docs/noir/standard_library/meta/struct_def.md index ab3ea4e0698..95e377dffd4 100644 --- a/docs/docs/noir/standard_library/meta/struct_def.md +++ b/docs/docs/noir/standard_library/meta/struct_def.md @@ -43,3 +43,32 @@ comptime fn example(foo: StructDefinition) { #include_code fields noir_stdlib/src/meta/struct_def.nr rust Returns each field of this struct as a pair of (field name, field type). + +### set_fields + +#include_code set_fields noir_stdlib/src/meta/struct_def.nr rust + +Sets the fields of this struct to the given fields list where each element +is a pair of the field's name and the field's type. Expects each field name +to be a single identifier. Note that this will override any previous fields +on this struct. If those should be preserved, use `.fields()` to retrieve the +current fields on the struct type and append the new fields from there. + +Example: + +```rust +// Change this struct to: +// struct Foo { +// a: u32, +// b: i8, +// } +#[mangle_fields] +struct Foo { x: Field } + +comptime fn mangle_fields(s: StructDefinition) { + s.set_fields(&[ + (quote { a }, quote { u32 }.as_type()), + (quote { b }, quote { i8 }.as_type()), + ]); +} +``` diff --git a/noir_stdlib/src/meta/struct_def.nr b/noir_stdlib/src/meta/struct_def.nr index 60fdeba21aa..1ca1b6a3925 100644 --- a/noir_stdlib/src/meta/struct_def.nr +++ b/noir_stdlib/src/meta/struct_def.nr @@ -18,4 +18,13 @@ impl StructDefinition { // docs:start:fields fn fields(self) -> [(Quoted, Type)] {} // docs:end:fields + + /// Sets the fields of this struct to the given fields list. + /// All existing fields of the struct will be overridden with the given fields. + /// Each element of the fields list corresponds to the name and type of a field. + /// Each name is expected to be a single identifier. + #[builtin(struct_def_set_fields)] + // docs:start:set_fields + fn set_fields(self, new_fields: [(Quoted, Type)]) {} + // docs:end:set_fields } diff --git a/test_programs/compile_success_empty/comptime_type_definition/src/main.nr b/test_programs/compile_success_empty/comptime_type_definition/src/main.nr index cdfc9bd6b75..aca8d067dde 100644 --- a/test_programs/compile_success_empty/comptime_type_definition/src/main.nr +++ b/test_programs/compile_success_empty/comptime_type_definition/src/main.nr @@ -6,8 +6,21 @@ struct MyType { field2: (B, C), } +#[mutate_struct_fields] +struct I32AndField { + z: i8, +} + comptime fn my_comptime_fn(typ: StructDefinition) { let _ = typ.as_type(); assert_eq(typ.generics().len(), 3); assert_eq(typ.fields().len(), 2); } + +comptime fn mutate_struct_fields(s: StructDefinition) { + let fields = &[ + (quote[x], quote[i32].as_type()), + (quote[y], quote[Field].as_type()) + ]; + s.set_fields(fields); +} From 6a450076be2889c05428ea1285c5c149cfaf4456 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 5 Sep 2024 11:26:36 -0300 Subject: [PATCH 17/25] fix: use element_size() instead of computing it with division (#5939) # Description ## Problem Resolves #5936 ## Summary ## Additional Context I was computing an array's element size in a way that could lead to dividing by zero, but there was already a method to get the element size. ## Documentation Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: jfecher --- compiler/noirc_evaluator/src/ssa/opt/die.rs | 5 ++--- .../execution_failure/empty_composite_array_get/Nargo.toml | 7 +++++++ .../empty_composite_array_get/Prover.toml | 1 + .../empty_composite_array_get/src/main.nr | 5 +++++ 4 files changed, 15 insertions(+), 3 deletions(-) create mode 100644 test_programs/execution_failure/empty_composite_array_get/Nargo.toml create mode 100644 test_programs/execution_failure/empty_composite_array_get/Prover.toml create mode 100644 test_programs/execution_failure/empty_composite_array_get/src/main.nr diff --git a/compiler/noirc_evaluator/src/ssa/opt/die.rs b/compiler/noirc_evaluator/src/ssa/opt/die.rs index b9804062118..095413d7b9a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/die.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/die.rs @@ -377,13 +377,12 @@ fn handle_array_get_group( next_out_of_bounds_index: &mut Option, possible_index_out_of_bounds_indexes: &mut Vec, ) { - let Some(array_length) = function.dfg.try_get_array_length(*array) else { + if function.dfg.try_get_array_length(*array).is_none() { // Nothing to do for slices return; }; - let flattened_size = function.dfg.type_of_value(*array).flattened_size(); - let element_size = flattened_size / array_length; + let element_size = function.dfg.type_of_value(*array).element_size(); if element_size <= 1 { // Not a composite type return; diff --git a/test_programs/execution_failure/empty_composite_array_get/Nargo.toml b/test_programs/execution_failure/empty_composite_array_get/Nargo.toml new file mode 100644 index 00000000000..fa4614c8351 --- /dev/null +++ b/test_programs/execution_failure/empty_composite_array_get/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "empty_composite_array_get" +type = "bin" +authors = [""] +compiler_version = ">=0.32.0" + +[dependencies] \ No newline at end of file diff --git a/test_programs/execution_failure/empty_composite_array_get/Prover.toml b/test_programs/execution_failure/empty_composite_array_get/Prover.toml new file mode 100644 index 00000000000..ff52a8c5bce --- /dev/null +++ b/test_programs/execution_failure/empty_composite_array_get/Prover.toml @@ -0,0 +1 @@ +empty_input = [] \ No newline at end of file diff --git a/test_programs/execution_failure/empty_composite_array_get/src/main.nr b/test_programs/execution_failure/empty_composite_array_get/src/main.nr new file mode 100644 index 00000000000..1ed9fe4a5e0 --- /dev/null +++ b/test_programs/execution_failure/empty_composite_array_get/src/main.nr @@ -0,0 +1,5 @@ +fn main(empty_input: [(Field, Field); 0]) { + let empty_array: [(Field, Field); 0] = []; + let _ = empty_input[0]; + let _ = empty_array[0]; +} From f391af2d61f4a38e02cb92c76fa4c2c148af3833 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Thu, 5 Sep 2024 16:22:56 +0100 Subject: [PATCH 18/25] feat: remove blocks which consist of only a jump to another block (#5889) # Description ## Problem\* Resolves ## Summary\* This PR removes blocks which are empty and just perform an unconditional jump to another block. We instead just update any references to this block with the block which it will eventually jump to. I initially tried applying this optimization universally but ran into troubles where ACIR functions would run into early returns due before branches reached a common block again and so the cfg flattening pass would fail. This optimisation then only applies to brillig functions (although it wouldn't really affect ACIR anyway). ## Additional Context ## Documentation\* Check one: - [ ] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [ ] I have tested the changes locally. - [ ] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../src/ssa/opt/simplify_cfg.rs | 75 ++++++++++++++++++- 1 file changed, 73 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs index 0a3b2a800ca..6887873dbc3 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/simplify_cfg.rs @@ -15,7 +15,9 @@ use acvm::acir::AcirField; use crate::ssa::{ ir::{ - basic_block::BasicBlockId, cfg::ControlFlowGraph, function::Function, + basic_block::BasicBlockId, + cfg::ControlFlowGraph, + function::{Function, RuntimeType}, instruction::TerminatorInstruction, }, ssa_gen::Ssa, @@ -30,7 +32,7 @@ impl Ssa { /// 5. Replacing any jmpifs with constant conditions with jmps. If this causes the block to have /// only 1 successor then (2) also will be applied. /// - /// Currently, 1 and 4 are unimplemented. + /// Currently, 1 is unimplemented. #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn simplify_cfg(mut self) -> Self { for function in self.functions.values_mut() { @@ -72,6 +74,10 @@ fn simplify_function(function: &mut Function) { // optimizations performed after this point on the same block should check if // the inlining here was successful before continuing. try_inline_into_predecessor(function, &mut cfg, block, predecessor); + } else { + drop(predecessors); + + check_for_double_jmp(function, block, &mut cfg); } } } @@ -102,6 +108,71 @@ fn check_for_constant_jmpif( } } +/// Optimize a jmp to a block which immediately jmps elsewhere to just jmp to the second block. +fn check_for_double_jmp(function: &mut Function, block: BasicBlockId, cfg: &mut ControlFlowGraph) { + if matches!(function.runtime(), RuntimeType::Acir(_)) { + // We can't remove double jumps in ACIR functions as this interferes with the `flatten_cfg` pass. + return; + } + + if !function.dfg[block].instructions().is_empty() + || !function.dfg[block].parameters().is_empty() + { + return; + } + + let Some(TerminatorInstruction::Jmp { destination: final_destination, arguments, .. }) = + function.dfg[block].terminator() + else { + return; + }; + + if !arguments.is_empty() { + return; + } + + let final_destination = *final_destination; + + let predecessors: Vec<_> = cfg.predecessors(block).collect(); + for predecessor_block in predecessors { + let terminator_instruction = function.dfg[predecessor_block].take_terminator(); + let redirected_terminator_instruction = match terminator_instruction { + TerminatorInstruction::JmpIf { + condition, + then_destination, + else_destination, + call_stack, + } => { + let then_destination = + if then_destination == block { final_destination } else { then_destination }; + let else_destination = + if else_destination == block { final_destination } else { else_destination }; + TerminatorInstruction::JmpIf { + condition, + then_destination, + else_destination, + call_stack, + } + } + TerminatorInstruction::Jmp { destination, arguments, call_stack } => { + assert_eq!( + destination, block, + "ICE: predecessor block doesn't jump to current block" + ); + assert!(arguments.is_empty(), "ICE: predecessor jmp has arguments"); + TerminatorInstruction::Jmp { destination: final_destination, arguments, call_stack } + } + TerminatorInstruction::Return { .. } => { + unreachable!("ICE: predecessor block should not have return terminator instruction") + } + }; + + function.dfg[predecessor_block].set_terminator(redirected_terminator_instruction); + cfg.recompute_block(function, predecessor_block); + } + cfg.recompute_block(function, block); +} + /// If the given block has block parameters, replace them with the jump arguments from the predecessor. /// /// Currently, if this function is needed, `try_inline_into_predecessor` will also always apply, From b84009ca428a5790acf53a6c027146b706170574 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 5 Sep 2024 11:46:23 -0400 Subject: [PATCH 19/25] feat(perf): Remove known store values that equal the store address in mem2reg (#5935) # Description ## Problem\* Partially resolves #4535 Replaces https://github.com/noir-lang/noir/pull/5865 ## Summary\* When we see a load we mark the address of that load as being a known value of the load result. When we reach a store instuction, if that store value has a known value which is equal to the address of the store we can remove that store. We also check whether the last instruction was an `inc_rc` or a `dec_rc`. If it was we do not remove the store. ## Additional Context ## Documentation\* Check one: - [X] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [X] I have tested the changes locally. - [X] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../noirc_evaluator/src/ssa/opt/mem2reg.rs | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs index 3d98f4126cf..4d3666bea1a 100644 --- a/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs +++ b/compiler/noirc_evaluator/src/ssa/opt/mem2reg.rs @@ -117,6 +117,10 @@ struct PerFunctionContext<'f> { /// Track a value's last load across all blocks. /// If a value is not used in anymore loads we can remove the last store to that value. last_loads: HashMap, + + /// Track whether the last instruction is an inc_rc/dec_rc instruction. + /// If it is we should not remove any known store values. + inside_rc_reload: bool, } impl<'f> PerFunctionContext<'f> { @@ -131,6 +135,7 @@ impl<'f> PerFunctionContext<'f> { blocks: BTreeMap::new(), instructions_to_remove: BTreeSet::new(), last_loads: HashMap::default(), + inside_rc_reload: false, } } @@ -281,6 +286,10 @@ impl<'f> PerFunctionContext<'f> { } else { references.mark_value_used(address, self.inserter.function); + references.expressions.insert(result, Expression::Other(result)); + references.aliases.insert(Expression::Other(result), AliasSet::known(result)); + references.set_known_value(result, address); + self.last_loads.insert(address, (instruction, block_id)); } } @@ -296,6 +305,14 @@ impl<'f> PerFunctionContext<'f> { self.instructions_to_remove.insert(*last_store); } + let known_value = references.get_known_value(value); + if let Some(known_value) = known_value { + let known_value_is_address = known_value == address; + if known_value_is_address && !self.inside_rc_reload { + self.instructions_to_remove.insert(instruction); + } + } + references.set_known_value(address, value); references.last_stores.insert(address, instruction); } @@ -350,6 +367,18 @@ impl<'f> PerFunctionContext<'f> { Instruction::Call { arguments, .. } => self.mark_all_unknown(arguments, references), _ => (), } + + self.track_rc_reload_state(instruction); + } + + fn track_rc_reload_state(&mut self, instruction: InstructionId) { + match &self.inserter.function.dfg[instruction] { + // We just had an increment or decrement to an array's reference counter + Instruction::IncrementRc { .. } | Instruction::DecrementRc { .. } => { + self.inside_rc_reload = true; + } + _ => self.inside_rc_reload = false, + } } fn check_array_aliasing(&self, references: &mut Block, array: ValueId) { From a297ec643eb3b6c0e8bcf62abdc005414283c7c2 Mon Sep 17 00:00:00 2001 From: Gregorio Juliana Date: Thu, 5 Sep 2024 19:10:01 +0200 Subject: [PATCH 20/25] feat: Add `Quoted::tokens` (#5942) # Description ## Problem\* While `fmtstr.quoted_contents` is great for 99% of use cases, sometimes the raw str (including double quotes) is needed. Consider this case (fn signature generation in `aztec.nr`) ```noir pub(crate) comptime fn compute_fn_selector(f: FunctionDefinition) -> Field { let fn_name = f.name(); let args_signatures = f.parameters().map( | (_, typ): (Quoted, Type) | { signature_of_type(typ) } ).join(quote {,}); let signature = f"{fn_name}({args_signatures})".quoted_contents(); // from_signature takes a str, quoted_contents is no good here! let computation_quote = quote { protocol_types::abis::function_selector::FunctionSelector::from_signature($signature).to_field() }; unquote!(computation_quote) } ``` We have a bunch of `Quoted` values obtained during comptime that we have to string (ha) together. The result is then needed to compute a hash. ## Summary\* ~~This just adds a builtin that "resolves" the `fmtstr` and then turns it into a `Token::Str`~~ It's possible to create a string from quoted values by doing: ```noir let signature = f"{name}({args})"; let signature_as_str = unquote!(quote { $signature }); ``` But this includes whitespaces around the tokens. To get rid of them, ended up going with the approach suggested by @jfecher, which certainly complicates user code, but avoids the "eating whitespaces" problem: > Going forward I think we'll need to instead control the formatting of Quoted values more. The compiler itself can never guess what exact formatting users will want of them since spaces are already not present. So I'm leaning towards a Quoted::tokens(self) -> [Quoted] method to return a slice of each individual token in the Quoted value. Then users could iterate over it and apply (or not apply) spacing in between each token as they see fit. Also, the returned values are `str<_>`, since the compiler has no idea of the returned length after formatting. This means values can only be used by unquoting them and not directly. ## Additional Context My first intuition was to do ```noir let signature = f"\"{quotedvalues}\"" ``` But it doesn't work =/ ## Documentation\* Check one: - [ ] No documentation needed. - [x] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: jfecher --- .../src/hir/comptime/interpreter/builtin.rs | 12 ++++++++++++ docs/docs/noir/standard_library/meta/quoted.md | 6 ++++++ noir_stdlib/src/meta/quoted.nr | 13 +++++++++---- 3 files changed, 27 insertions(+), 4 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index 65c9c3f018d..5db7999a6f2 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -123,6 +123,7 @@ impl<'local, 'context> Interpreter<'local, 'context> { "quoted_as_trait_constraint" => quoted_as_trait_constraint(self, arguments, location), "quoted_as_type" => quoted_as_type(self, arguments, location), "quoted_eq" => quoted_eq(arguments, location), + "quoted_tokens" => quoted_tokens(arguments, location), "slice_insert" => slice_insert(interner, arguments, location), "slice_pop_back" => slice_pop_back(interner, arguments, location, call_stack), "slice_pop_front" => slice_pop_front(interner, arguments, location, call_stack), @@ -535,6 +536,17 @@ fn quoted_as_type( Ok(Value::Type(typ)) } +// fn tokens(quoted: Quoted) -> [Quoted] +fn quoted_tokens(arguments: Vec<(Value, Location)>, location: Location) -> IResult { + let argument = check_one_argument(arguments, location)?; + let value = get_quoted(argument)?; + + Ok(Value::Slice( + value.iter().map(|token| Value::Quoted(Rc::new(vec![token.clone()]))).collect(), + Type::Slice(Box::new(Type::Quoted(QuotedType::Quoted))), + )) +} + fn to_le_radix( arguments: Vec<(Value, Location)>, return_type: Type, diff --git a/docs/docs/noir/standard_library/meta/quoted.md b/docs/docs/noir/standard_library/meta/quoted.md index bf79f2e5d9f..ac075d96648 100644 --- a/docs/docs/noir/standard_library/meta/quoted.md +++ b/docs/docs/noir/standard_library/meta/quoted.md @@ -50,6 +50,12 @@ stream doesn't parse to a type or if the type isn't a valid type in scope. #include_code implements_example test_programs/compile_success_empty/comptime_type/src/main.nr rust +### tokens + +#include_code tokens noir_stdlib/src/meta/quoted.nr rust + +Returns a slice of the individual tokens that form this token stream. + ## Trait Implementations ```rust diff --git a/noir_stdlib/src/meta/quoted.nr b/noir_stdlib/src/meta/quoted.nr index 9fd1e9026ba..3fab43359c1 100644 --- a/noir_stdlib/src/meta/quoted.nr +++ b/noir_stdlib/src/meta/quoted.nr @@ -3,24 +3,29 @@ use crate::option::Option; impl Quoted { #[builtin(quoted_as_expr)] -// docs:start:as_expr + // docs:start:as_expr fn as_expr(self) -> Option {} // docs:end:as_expr #[builtin(quoted_as_module)] -// docs:start:as_module + // docs:start:as_module fn as_module(self) -> Option {} // docs:end:as_module #[builtin(quoted_as_trait_constraint)] -// docs:start:as_trait_constraint + // docs:start:as_trait_constraint fn as_trait_constraint(self) -> TraitConstraint {} // docs:end:as_trait_constraint #[builtin(quoted_as_type)] -// docs:start:as_type + // docs:start:as_type fn as_type(self) -> Type {} // docs:end:as_type + + #[builtin(quoted_tokens)] + // docs:start:tokens + fn tokens(self) -> [Quoted] {} + // docs:end:tokens } impl Eq for Quoted { From 89ac6e087debc37dcc729db0b68062418cd64d2e Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 5 Sep 2024 14:22:12 -0300 Subject: [PATCH 21/25] fix: always place module attribute generated items inside module (#5943) # Description ## Problem Resolves https://github.com/noir-lang/noir/issues/5875#issuecomment-2331698684 ## Summary ## Additional Context ## Documentation Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --------- Co-authored-by: jfecher Co-authored-by: Jake Fecher --- .../noirc_frontend/src/elaborator/comptime.rs | 83 +++++++++++++++---- .../comptime_module/src/main.nr | 15 ++-- .../lsp/src/requests/completion/builtins.rs | 2 +- 3 files changed, 79 insertions(+), 21 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/comptime.rs b/compiler/noirc_frontend/src/elaborator/comptime.rs index 7da5efd0b5a..e9650a625e8 100644 --- a/compiler/noirc_frontend/src/elaborator/comptime.rs +++ b/compiler/noirc_frontend/src/elaborator/comptime.rs @@ -15,7 +15,7 @@ use crate::{ }, dc_mod, }, - def_map::ModuleId, + def_map::{LocalModuleId, ModuleId}, resolution::errors::ResolverError, }, hir_def::expr::HirIdent, @@ -30,6 +30,24 @@ use crate::{ use super::{Elaborator, FunctionContext, ResolverMeta}; +#[derive(Debug, Copy, Clone)] +struct AttributeContext { + // The file where generated items should be added + file: FileId, + // The module where generated items should be added + module: LocalModuleId, + // The file where the attribute is located + attribute_file: FileId, + // The module where the attribute is located + attribute_module: LocalModuleId, +} + +impl AttributeContext { + fn new(file: FileId, module: LocalModuleId) -> Self { + Self { file, module, attribute_file: file, attribute_module: module } + } +} + impl<'context> Elaborator<'context> { /// Elaborate an expression from the middle of a comptime scope. /// When this happens we require additional information to know @@ -89,15 +107,22 @@ impl<'context> Elaborator<'context> { } } - pub(super) fn run_comptime_attributes_on_item( + fn run_comptime_attributes_on_item( &mut self, attributes: &[SecondaryAttribute], item: Value, span: Span, + attribute_context: AttributeContext, generated_items: &mut CollectedItems, ) { for attribute in attributes { - self.run_comptime_attribute_on_item(attribute, &item, span, generated_items); + self.run_comptime_attribute_on_item( + attribute, + &item, + span, + attribute_context, + generated_items, + ); } } @@ -106,6 +131,7 @@ impl<'context> Elaborator<'context> { attribute: &SecondaryAttribute, item: &Value, span: Span, + attribute_context: AttributeContext, generated_items: &mut CollectedItems, ) { if let SecondaryAttribute::Custom(attribute) = attribute { @@ -114,6 +140,7 @@ impl<'context> Elaborator<'context> { item.clone(), span, attribute.contents_span, + attribute_context, generated_items, ) { self.errors.push(error); @@ -127,8 +154,12 @@ impl<'context> Elaborator<'context> { item: Value, span: Span, attribute_span: Span, + attribute_context: AttributeContext, generated_items: &mut CollectedItems, ) -> Result<(), (CompilationError, FileId)> { + self.file = attribute_context.attribute_file; + self.local_module = attribute_context.attribute_module; + let location = Location::new(attribute_span, self.file); let Some((function, arguments)) = Self::parse_attribute(attribute, location)? else { // Do not issue an error if the attribute is unknown @@ -156,6 +187,9 @@ impl<'context> Elaborator<'context> { return Err((ResolverError::NonFunctionInAnnotation { span }.into(), self.file)); }; + self.file = attribute_context.file; + self.local_module = attribute_context.module; + let mut interpreter = self.setup_interpreter(); let mut arguments = Self::handle_attribute_arguments( &mut interpreter, @@ -463,18 +497,28 @@ impl<'context> Elaborator<'context> { let attributes = &trait_.trait_def.attributes; let item = Value::TraitDefinition(*trait_id); let span = trait_.trait_def.span; - self.local_module = trait_.module_id; - self.file = trait_.file_id; - self.run_comptime_attributes_on_item(attributes, item, span, &mut generated_items); + let context = AttributeContext::new(trait_.file_id, trait_.module_id); + self.run_comptime_attributes_on_item( + attributes, + item, + span, + context, + &mut generated_items, + ); } for (struct_id, struct_def) in types { let attributes = &struct_def.struct_def.attributes; let item = Value::StructDefinition(*struct_id); let span = struct_def.struct_def.span; - self.local_module = struct_def.module_id; - self.file = struct_def.file_id; - self.run_comptime_attributes_on_item(attributes, item, span, &mut generated_items); + let context = AttributeContext::new(struct_def.file_id, struct_def.module_id); + self.run_comptime_attributes_on_item( + attributes, + item, + span, + context, + &mut generated_items, + ); } self.run_attributes_on_functions(functions, &mut generated_items); @@ -496,10 +540,14 @@ impl<'context> Elaborator<'context> { let attribute = &module_attribute.attribute; let span = Span::default(); - self.local_module = module_attribute.attribute_module_id; - self.file = module_attribute.attribute_file_id; + let context = AttributeContext { + file: module_attribute.file_id, + module: module_attribute.module_id, + attribute_file: module_attribute.attribute_file_id, + attribute_module: module_attribute.attribute_module_id, + }; - self.run_comptime_attribute_on_item(attribute, &item, span, generated_items); + self.run_comptime_attribute_on_item(attribute, &item, span, context, generated_items); } } @@ -509,15 +557,20 @@ impl<'context> Elaborator<'context> { generated_items: &mut CollectedItems, ) { for function_set in function_sets { - self.file = function_set.file_id; self.self_type = function_set.self_type.clone(); for (local_module, function_id, function) in &function_set.functions { - self.local_module = *local_module; + let context = AttributeContext::new(function_set.file_id, *local_module); let attributes = function.secondary_attributes(); let item = Value::FunctionDefinition(*function_id); let span = function.span(); - self.run_comptime_attributes_on_item(attributes, item, span, generated_items); + self.run_comptime_attributes_on_item( + attributes, + item, + span, + context, + generated_items, + ); } } } diff --git a/test_programs/compile_success_empty/comptime_module/src/main.nr b/test_programs/compile_success_empty/comptime_module/src/main.nr index 5722d42ca26..1d1690c4017 100644 --- a/test_programs/compile_success_empty/comptime_module/src/main.nr +++ b/test_programs/compile_success_empty/comptime_module/src/main.nr @@ -1,8 +1,8 @@ #[outer_attribute] mod foo { #![some_attribute] - fn x() {} - fn y() {} + pub fn x() {} + pub fn y() {} } contract bar {} @@ -13,7 +13,7 @@ mod another_module {} #[outer_attribute_func] mod yet_another_module { #![super::inner_attribute_func] - fn foo() {} + pub fn foo() {} } #[outer_attribute_separate_module] @@ -25,14 +25,16 @@ fn increment_counter() { counter += 1; } -fn outer_attribute_func(m: Module) { +fn outer_attribute_func(m: Module) -> Quoted { assert_eq(m.name(), quote { yet_another_module }); increment_counter(); + quote { pub fn generated_outer_function() {} } } -fn inner_attribute_func(m: Module) { +fn inner_attribute_func(m: Module) -> Quoted { assert_eq(m.name(), quote { yet_another_module }); increment_counter(); + quote { pub fn generated_inner_function() {} } } fn outer_attribute_separate_module(m: Module) { @@ -68,6 +70,9 @@ fn main() { } assert_eq(counter, 4); + + yet_another_module::generated_outer_function(); + yet_another_module::generated_inner_function(); } // docs:start:as_module_example diff --git a/tooling/lsp/src/requests/completion/builtins.rs b/tooling/lsp/src/requests/completion/builtins.rs index 54340075b15..f449177a027 100644 --- a/tooling/lsp/src/requests/completion/builtins.rs +++ b/tooling/lsp/src/requests/completion/builtins.rs @@ -97,6 +97,7 @@ pub(super) fn keyword_builtin_type(keyword: &Keyword) -> Option<&'static str> { Keyword::Expr => Some("Expr"), Keyword::Field => Some("Field"), Keyword::FunctionDefinition => Some("FunctionDefinition"), + Keyword::Module => Some("Module"), Keyword::Quoted => Some("Quoted"), Keyword::StructDefinition => Some("StructDefinition"), Keyword::TraitConstraint => Some("TraitConstraint"), @@ -128,7 +129,6 @@ pub(super) fn keyword_builtin_type(keyword: &Keyword) -> Option<&'static str> { | Keyword::In | Keyword::Let | Keyword::Mod - | Keyword::Module | Keyword::Mut | Keyword::Pub | Keyword::Return From c7479c4e55f47f7c652f0e202636b9e590d11f5d Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 5 Sep 2024 14:35:30 -0300 Subject: [PATCH 22/25] feat: add `FunctionDefinition::add_attribute` (#5944) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Description ## Problem Part of #5874 ## Summary This is fun 😄 ![fn-add-attribute](https://github.com/user-attachments/assets/3fa5bf97-b7f1-4c85-8c54-4198ee35efeb) ## Additional Context I'll add similar methods to `StructDefinition` and `Module` in follow-up PRs. ## Documentation\* Check one: - [ ] No documentation needed. - [x] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../noirc_frontend/src/hir/comptime/errors.rs | 10 ++++ .../src/hir/comptime/interpreter/builtin.rs | 56 ++++++++++++++++++- .../standard_library/meta/function_def.md | 8 +++ noir_stdlib/src/meta/function_def.nr | 5 ++ 4 files changed, 77 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/comptime/errors.rs b/compiler/noirc_frontend/src/hir/comptime/errors.rs index f6585786eeb..5d4d814f3ee 100644 --- a/compiler/noirc_frontend/src/hir/comptime/errors.rs +++ b/compiler/noirc_frontend/src/hir/comptime/errors.rs @@ -207,6 +207,10 @@ pub enum InterpreterError { index: usize, location: Location, }, + InvalidAttribute { + attribute: String, + location: Location, + }, // These cases are not errors, they are just used to prevent us from running more code // until the loop can be resumed properly. These cases will never be displayed to users. @@ -276,6 +280,7 @@ impl InterpreterError { | InterpreterError::MultipleMatchingImpls { location, .. } | InterpreterError::ExpectedIdentForStructField { location, .. } | InterpreterError::TypeAnnotationsNeededForMethodCall { location } => *location, + InterpreterError::InvalidAttribute { location, .. } => *location, InterpreterError::FailedToParseMacro { error, file, .. } => { Location::new(error.span(), *file) @@ -579,6 +584,11 @@ impl<'a> From<&'a InterpreterError> for CustomDiagnostic { let secondary = format!("`{value}` is not a valid field name for `set_fields`"); CustomDiagnostic::simple_error(msg, secondary, location.span) } + InterpreterError::InvalidAttribute { attribute, location } => { + let msg = format!("`{attribute}` is not a valid attribute"); + let secondary = "Note that this method expects attribute contents, without the leading `#[` or trailing `]`".to_string(); + CustomDiagnostic::simple_error(msg, secondary, location.span) + } } } } diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index 5db7999a6f2..a1bb6d09cc9 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -12,7 +12,7 @@ use builtin_helpers::{ get_u32, get_unresolved_type, hir_pattern_to_tokens, mutate_func_meta_type, parse, replace_func_meta_parameters, replace_func_meta_return_type, }; -use chumsky::{prelude::choice, Parser}; +use chumsky::{chain::Chain, prelude::choice, Parser}; use im::Vector; use iter_extended::{try_vecmap, vecmap}; use noirc_errors::Location; @@ -36,7 +36,7 @@ use crate::{ macros_api::{HirExpression, HirLiteral, Ident, ModuleDefId, NodeInterner, Signedness}, node_interner::{DefinitionKind, TraitImplKind}, parser::{self}, - token::Token, + token::{Attribute, SecondaryAttribute, Token}, QuotedType, Shared, Type, }; @@ -97,6 +97,7 @@ impl<'local, 'context> Interpreter<'local, 'context> { "expr_resolve" => expr_resolve(self, arguments, location), "is_unconstrained" => Ok(Value::Bool(true)), "fmtstr_quoted_contents" => fmtstr_quoted_contents(interner, arguments, location), + "function_def_add_attribute" => function_def_add_attribute(self, arguments, location), "function_def_body" => function_def_body(interner, arguments, location), "function_def_has_named_attribute" => { function_def_has_named_attribute(interner, arguments, location) @@ -1666,6 +1667,57 @@ fn fmtstr_quoted_contents( Ok(Value::Quoted(Rc::new(tokens))) } +// fn add_attribute(self, attribute: str) +fn function_def_add_attribute( + interpreter: &mut Interpreter, + arguments: Vec<(Value, Location)>, + location: Location, +) -> IResult { + let (self_argument, attribute) = check_two_arguments(arguments, location)?; + let attribute_location = attribute.1; + let attribute = get_str(interpreter.elaborator.interner, attribute)?; + + let mut tokens = Lexer::lex(&format!("#[{}]", attribute)).0 .0; + if let Some(Token::EOF) = tokens.last().map(|token| token.token()) { + tokens.pop(); + } + if tokens.len() != 1 { + return Err(InterpreterError::InvalidAttribute { + attribute: attribute.to_string(), + location: attribute_location, + }); + } + + let token = tokens[0].token(); + let Token::Attribute(attribute) = token else { + return Err(InterpreterError::InvalidAttribute { + attribute: attribute.to_string(), + location: attribute_location, + }); + }; + + let func_id = get_function_def(self_argument)?; + check_function_not_yet_resolved(interpreter, func_id, location)?; + + let function_modifiers = interpreter.elaborator.interner.function_modifiers_mut(&func_id); + + match attribute { + Attribute::Function(attribute) => { + function_modifiers.attributes.function = Some(attribute.clone()); + } + Attribute::Secondary(attribute) => { + function_modifiers.attributes.secondary.push(attribute.clone()); + } + } + + if let Attribute::Secondary(SecondaryAttribute::Custom(attribute)) = attribute { + let func_meta = interpreter.elaborator.interner.function_meta_mut(&func_id); + func_meta.custom_attributes.push(attribute.clone()); + } + + Ok(Value::Unit) +} + // fn body(self) -> Expr fn function_def_body( interner: &NodeInterner, diff --git a/docs/docs/noir/standard_library/meta/function_def.md b/docs/docs/noir/standard_library/meta/function_def.md index 5657e05fff7..3789fd82866 100644 --- a/docs/docs/noir/standard_library/meta/function_def.md +++ b/docs/docs/noir/standard_library/meta/function_def.md @@ -7,6 +7,14 @@ a function definition in the source program. ## Methods +### add_attribute + +#include_code add_attribute noir_stdlib/src/meta/function_def.nr rust + +Adds an attribute to the function. This is only valid +on functions in the current crate which have not yet been resolved. +This means any functions called at compile-time are invalid targets for this method. + ### body #include_code body noir_stdlib/src/meta/function_def.nr rust diff --git a/noir_stdlib/src/meta/function_def.nr b/noir_stdlib/src/meta/function_def.nr index cbbbfb2f901..c1f2c026d57 100644 --- a/noir_stdlib/src/meta/function_def.nr +++ b/noir_stdlib/src/meta/function_def.nr @@ -1,4 +1,9 @@ impl FunctionDefinition { + #[builtin(function_def_add_attribute)] + // docs:start:add_attribute + fn add_attribute(self, attribute: str) {} + // docs:end:add_attribute + #[builtin(function_def_body)] // docs:start:body fn body(self) -> Expr {} From 8beda6beb10a2e42da788bcc9bf2b375055675c6 Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 5 Sep 2024 15:53:24 -0300 Subject: [PATCH 23/25] feat: add `FunctionDef::set_return_visibility` (#5941) # Description ## Problem Resolves #5902 ## Summary ## Additional Context There's a catch: if you pass `quote { hello world }` to the method, it doesn't crash. The reason is that when we parse the visibility, if it's not `pub` or one of the other non-private visibilities, we simply assume it's private (in regular code then would come a type or `{` and that's where it would fail to parse). I wanted to error in this case but couldn't: 1. One idea I had was to check if we get a non-empty tokens stream. If that's the case and we don't get a private visibility, it's an error. Well, that worked, but I didn't know how to easily create a `ParserError` for that. 2. Another idea is to have a specific parser to parse this or EOF... but I tried a choice with `TokenKind::Token(Token::EOF)` and when it was EOF, it said it was expecting EOF (kind of confusing). Any ideas about the best way to do this? We could also capture a follow-up issue for this, because merging this unblocks some stuff. ## Documentation Check one: - [x] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../noirc_frontend/src/elaborator/lints.rs | 87 +++++++++++-------- compiler/noirc_frontend/src/elaborator/mod.rs | 38 ++++---- .../src/hir/comptime/interpreter/builtin.rs | 24 ++++- .../interpreter/builtin/builtin_helpers.rs | 7 ++ compiler/noirc_frontend/src/parser/mod.rs | 3 +- compiler/noirc_frontend/src/parser/parser.rs | 14 +-- .../src/parser/parser/function.rs | 28 +++--- .../src/parser/parser/visibility.rs | 15 +++- .../standard_library/meta/function_def.md | 8 ++ noir_stdlib/src/meta/function_def.nr | 5 ++ .../comptime_function_definition/src/main.nr | 14 ++- 11 files changed, 156 insertions(+), 87 deletions(-) diff --git a/compiler/noirc_frontend/src/elaborator/lints.rs b/compiler/noirc_frontend/src/elaborator/lints.rs index 78df10fa94c..8253921d305 100644 --- a/compiler/noirc_frontend/src/elaborator/lints.rs +++ b/compiler/noirc_frontend/src/elaborator/lints.rs @@ -1,20 +1,19 @@ use crate::{ - ast::FunctionKind, + ast::{FunctionKind, Ident}, graph::CrateId, hir::{ resolution::errors::{PubPosition, ResolverError}, type_check::TypeCheckError, }, - hir_def::expr::HirIdent, + hir_def::{expr::HirIdent, function::FuncMeta}, macros_api::{ - HirExpression, HirLiteral, NodeInterner, NoirFunction, Signedness, UnaryOp, - UnresolvedTypeData, Visibility, + HirExpression, HirLiteral, NodeInterner, NoirFunction, Signedness, UnaryOp, Visibility, }, - node_interner::{DefinitionKind, ExprId, FuncId}, + node_interner::{DefinitionKind, ExprId, FuncId, FunctionModifiers}, Type, }; -use noirc_errors::Span; +use noirc_errors::{Span, Spanned}; pub(super) fn deprecated_function(interner: &NodeInterner, expr: ExprId) -> Option { let HirExpression::Ident(HirIdent { location, id, impl_kind: _ }, _) = @@ -39,16 +38,17 @@ pub(super) fn deprecated_function(interner: &NodeInterner, expr: ExprId) -> Opti /// Inline attributes are only relevant for constrained functions /// as all unconstrained functions are not inlined and so /// associated attributes are disallowed. -pub(super) fn inlining_attributes(func: &NoirFunction) -> Option { - if func.def.is_unconstrained { - let attributes = func.attributes().clone(); - - if attributes.is_no_predicates() { - Some(ResolverError::NoPredicatesAttributeOnUnconstrained { - ident: func.name_ident().clone(), - }) - } else if attributes.is_foldable() { - Some(ResolverError::FoldAttributeOnUnconstrained { ident: func.name_ident().clone() }) +pub(super) fn inlining_attributes( + func: &FuncMeta, + modifiers: &FunctionModifiers, +) -> Option { + if modifiers.is_unconstrained { + if modifiers.attributes.is_no_predicates() { + let ident = func_meta_name_ident(func, modifiers); + Some(ResolverError::NoPredicatesAttributeOnUnconstrained { ident }) + } else if modifiers.attributes.is_foldable() { + let ident = func_meta_name_ident(func, modifiers); + Some(ResolverError::FoldAttributeOnUnconstrained { ident }) } else { None } @@ -59,24 +59,30 @@ pub(super) fn inlining_attributes(func: &NoirFunction) -> Option /// Attempting to define new low level (`#[builtin]` or `#[foreign]`) functions outside of the stdlib is disallowed. pub(super) fn low_level_function_outside_stdlib( - func: &NoirFunction, + func: &FuncMeta, + modifiers: &FunctionModifiers, crate_id: CrateId, ) -> Option { let is_low_level_function = - func.attributes().function.as_ref().map_or(false, |func| func.is_low_level()); + modifiers.attributes.function.as_ref().map_or(false, |func| func.is_low_level()); if !crate_id.is_stdlib() && is_low_level_function { - Some(ResolverError::LowLevelFunctionOutsideOfStdlib { ident: func.name_ident().clone() }) + let ident = func_meta_name_ident(func, modifiers); + Some(ResolverError::LowLevelFunctionOutsideOfStdlib { ident }) } else { None } } /// Oracle definitions (functions with the `#[oracle]` attribute) must be marked as unconstrained. -pub(super) fn oracle_not_marked_unconstrained(func: &NoirFunction) -> Option { +pub(super) fn oracle_not_marked_unconstrained( + func: &FuncMeta, + modifiers: &FunctionModifiers, +) -> Option { let is_oracle_function = - func.attributes().function.as_ref().map_or(false, |func| func.is_oracle()); - if is_oracle_function && !func.def.is_unconstrained { - Some(ResolverError::OracleMarkedAsConstrained { ident: func.name_ident().clone() }) + modifiers.attributes.function.as_ref().map_or(false, |func| func.is_oracle()); + if is_oracle_function && !modifiers.is_unconstrained { + let ident = func_meta_name_ident(func, modifiers); + Some(ResolverError::OracleMarkedAsConstrained { ident }) } else { None } @@ -106,12 +112,13 @@ pub(super) fn oracle_called_from_constrained_function( } /// `pub` is required on return types for entry point functions -pub(super) fn missing_pub(func: &NoirFunction, is_entry_point: bool) -> Option { - if is_entry_point - && func.return_type().typ != UnresolvedTypeData::Unit - && func.def.return_visibility == Visibility::Private +pub(super) fn missing_pub(func: &FuncMeta, modifiers: &FunctionModifiers) -> Option { + if func.is_entry_point + && func.return_type() != &Type::Unit + && func.return_visibility == Visibility::Private { - Some(ResolverError::NecessaryPub { ident: func.name_ident().clone() }) + let ident = func_meta_name_ident(func, modifiers); + Some(ResolverError::NecessaryPub { ident }) } else { None } @@ -119,11 +126,12 @@ pub(super) fn missing_pub(func: &NoirFunction, is_entry_point: bool) -> Option Option { - if !is_entry_point && func.kind == FunctionKind::Recursive { - Some(ResolverError::MisplacedRecursiveAttribute { ident: func.name_ident().clone() }) + if !func.is_entry_point && func.kind == FunctionKind::Recursive { + let ident = func_meta_name_ident(func, modifiers); + Some(ResolverError::MisplacedRecursiveAttribute { ident }) } else { None } @@ -163,14 +171,13 @@ pub(super) fn unconstrained_function_return( /// /// Application of `pub` to other functions is not meaningful and is a mistake. pub(super) fn unnecessary_pub_return( - func: &NoirFunction, + func: &FuncMeta, + modifiers: &FunctionModifiers, is_entry_point: bool, ) -> Option { - if !is_entry_point && func.def.return_visibility == Visibility::Public { - Some(ResolverError::UnnecessaryPub { - ident: func.name_ident().clone(), - position: PubPosition::ReturnType, - }) + if !is_entry_point && func.return_visibility == Visibility::Public { + let ident = func_meta_name_ident(func, modifiers); + Some(ResolverError::UnnecessaryPub { ident, position: PubPosition::ReturnType }) } else { None } @@ -252,3 +259,7 @@ pub(crate) fn overflowing_int( errors } + +fn func_meta_name_ident(func: &FuncMeta, modifiers: &FunctionModifiers) -> Ident { + Ident(Spanned::from(func.name.location.span, modifiers.name.clone())) +} diff --git a/compiler/noirc_frontend/src/elaborator/mod.rs b/compiler/noirc_frontend/src/elaborator/mod.rs index 161742029f6..6b23336b5ea 100644 --- a/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/compiler/noirc_frontend/src/elaborator/mod.rs @@ -26,7 +26,8 @@ use crate::{ SecondaryAttribute, StructId, }, node_interner::{ - DefinitionKind, DependencyId, ExprId, FuncId, GlobalId, ReferenceId, TraitId, TypeAliasId, + DefinitionKind, DependencyId, ExprId, FuncId, FunctionModifiers, GlobalId, ReferenceId, + TraitId, TypeAliasId, }, token::CustomAtrribute, Shared, Type, TypeVariable, @@ -417,6 +418,10 @@ impl<'context> Elaborator<'context> { self.trait_bounds = func_meta.trait_constraints.clone(); self.function_context.push(FunctionContext::default()); + let modifiers = self.interner.function_modifiers(&id).clone(); + + self.run_function_lints(&func_meta, &modifiers); + self.introduce_generics_into_scope(func_meta.all_generics.clone()); // The DefinitionIds for each parameter were already created in define_function_meta @@ -731,20 +736,6 @@ impl<'context> Elaborator<'context> { let is_entry_point = self.is_entry_point_function(func, in_contract); - self.run_lint(|_| lints::inlining_attributes(func).map(Into::into)); - self.run_lint(|_| lints::missing_pub(func, is_entry_point).map(Into::into)); - self.run_lint(|elaborator| { - lints::unnecessary_pub_return(func, elaborator.pub_allowed(func, in_contract)) - .map(Into::into) - }); - self.run_lint(|_| lints::oracle_not_marked_unconstrained(func).map(Into::into)); - self.run_lint(|elaborator| { - lints::low_level_function_outside_stdlib(func, elaborator.crate_id).map(Into::into) - }); - self.run_lint(|_| { - lints::recursive_non_entrypoint_function(func, is_entry_point).map(Into::into) - }); - // Both the #[fold] and #[no_predicates] alter a function's inline type and code generation in similar ways. // In certain cases such as type checking (for which the following flag will be used) both attributes // indicate we should code generate in the same way. Thus, we unify the attributes into one flag here. @@ -858,6 +849,23 @@ impl<'context> Elaborator<'context> { self.current_item = None; } + 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)); + self.run_lint(|_| { + let pub_allowed = func.is_entry_point || modifiers.attributes.is_foldable(); + lints::unnecessary_pub_return(func, modifiers, pub_allowed).map(Into::into) + }); + self.run_lint(|_| lints::oracle_not_marked_unconstrained(func, modifiers).map(Into::into)); + self.run_lint(|elaborator| { + lints::low_level_function_outside_stdlib(func, modifiers, elaborator.crate_id) + .map(Into::into) + }); + self.run_lint(|_| { + lints::recursive_non_entrypoint_function(func, modifiers).map(Into::into) + }); + } + /// Only sized types are valid to be used as main's parameters or the parameters to a contract /// function. If the given type is not sized (e.g. contains a slice or NamedGeneric type), an /// error is issued. diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index a1bb6d09cc9..a1bb99dbb46 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -6,7 +6,7 @@ use std::{ use acvm::{AcirField, FieldElement}; use builtin_helpers::{ block_expression_to_value, check_argument_count, check_function_not_yet_resolved, - check_one_argument, check_three_arguments, check_two_arguments, get_expr, get_field, + check_one_argument, check_three_arguments, check_two_arguments, get_bool, get_expr, get_field, get_format_string, get_function_def, get_module, get_quoted, get_slice, get_struct, get_trait_constraint, get_trait_def, get_trait_impl, get_tuple, get_type, get_typed_expr, get_u32, get_unresolved_type, hir_pattern_to_tokens, mutate_func_meta_type, parse, @@ -110,6 +110,9 @@ impl<'local, 'context> Interpreter<'local, 'context> { "function_def_set_return_type" => { function_def_set_return_type(self, arguments, location) } + "function_def_set_return_public" => { + function_def_set_return_public(self, arguments, location) + } "module_functions" => module_functions(self, arguments, location), "module_has_named_attribute" => module_has_named_attribute(self, arguments, location), "module_is_contract" => module_is_contract(self, arguments, location), @@ -1935,6 +1938,25 @@ fn function_def_set_return_type( Ok(Value::Unit) } +// fn set_return_public(self, public: bool) +fn function_def_set_return_public( + interpreter: &mut Interpreter, + arguments: Vec<(Value, Location)>, + location: Location, +) -> IResult { + let (self_argument, public) = check_two_arguments(arguments, location)?; + + let func_id = get_function_def(self_argument)?; + check_function_not_yet_resolved(interpreter, func_id, location)?; + + let public = get_bool(public)?; + + let func_meta = interpreter.elaborator.interner.function_meta_mut(&func_id); + func_meta.return_visibility = if public { Visibility::Public } else { Visibility::Private }; + + Ok(Value::Unit) +} + // fn functions(self) -> [FunctionDefinition] fn module_functions( interpreter: &Interpreter, diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin/builtin_helpers.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin/builtin_helpers.rs index ff3da6d253f..163b74efbe6 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin/builtin_helpers.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin/builtin_helpers.rs @@ -90,6 +90,13 @@ pub(crate) fn get_array( } } +pub(crate) fn get_bool((value, location): (Value, Location)) -> IResult { + match value { + Value::Bool(value) => Ok(value), + value => type_mismatch(value, Type::Bool, location), + } +} + pub(crate) fn get_slice( interner: &NodeInterner, (value, location): (Value, Location), diff --git a/compiler/noirc_frontend/src/parser/mod.rs b/compiler/noirc_frontend/src/parser/mod.rs index 596d15176bc..2995e90ab01 100644 --- a/compiler/noirc_frontend/src/parser/mod.rs +++ b/compiler/noirc_frontend/src/parser/mod.rs @@ -26,7 +26,8 @@ use noirc_errors::Span; pub use parser::path::path_no_turbofish; pub use parser::traits::trait_bound; pub use parser::{ - block, expression, fresh_statement, lvalue, parse_program, parse_type, pattern, top_level_items, + block, expression, fresh_statement, lvalue, parse_program, parse_type, pattern, + top_level_items, visibility, }; #[derive(Debug, Clone)] diff --git a/compiler/noirc_frontend/src/parser/parser.rs b/compiler/noirc_frontend/src/parser/parser.rs index 2bc7a88c6c5..1aee697aa88 100644 --- a/compiler/noirc_frontend/src/parser/parser.rs +++ b/compiler/noirc_frontend/src/parser/parser.rs @@ -28,7 +28,8 @@ use self::primitives::{keyword, macro_quote_marker, mutable_reference, variable} use self::types::{generic_type_args, maybe_comp_time}; use attributes::{attributes, inner_attribute, validate_secondary_attributes}; pub use types::parse_type; -use visibility::visibility_modifier; +use visibility::item_visibility; +pub use visibility::visibility; use super::{ foldl_with_span, labels::ParsingRuleLabel, parameter_name_recovery, parameter_recovery, @@ -459,7 +460,7 @@ fn module_declaration() -> impl NoirParser { } fn use_statement() -> impl NoirParser { - visibility_modifier() + item_visibility() .then_ignore(keyword(Keyword::Use)) .then(use_tree()) .map(|(visibility, use_tree)| TopLevelStatement::Import(use_tree, visibility)) @@ -737,15 +738,6 @@ fn call_data() -> impl NoirParser { }) } -fn optional_visibility() -> impl NoirParser { - keyword(Keyword::Pub) - .map(|_| Visibility::Public) - .or(call_data()) - .or(keyword(Keyword::ReturnData).map(|_| Visibility::ReturnData)) - .or_not() - .map(|opt| opt.unwrap_or(Visibility::Private)) -} - pub fn expression() -> impl ExprParser { recursive(|expr| { expression_with_precedence( diff --git a/compiler/noirc_frontend/src/parser/parser/function.rs b/compiler/noirc_frontend/src/parser/parser/function.rs index 9328c882e54..05138bfffd9 100644 --- a/compiler/noirc_frontend/src/parser/parser/function.rs +++ b/compiler/noirc_frontend/src/parser/parser/function.rs @@ -1,10 +1,10 @@ use super::{ attributes::{attributes, validate_attributes}, - block, fresh_statement, ident, keyword, maybe_comp_time, nothing, optional_visibility, - parameter_name_recovery, parameter_recovery, parenthesized, parse_type, pattern, + block, fresh_statement, ident, keyword, maybe_comp_time, nothing, parameter_name_recovery, + parameter_recovery, parenthesized, parse_type, pattern, primitives::token_kind, self_parameter, - visibility::visibility_modifier, + visibility::{item_visibility, visibility}, where_clause, NoirParser, }; use crate::token::{Keyword, Token, TokenKind}; @@ -79,13 +79,9 @@ pub(super) fn function_definition(allow_self: bool) -> impl NoirParser impl NoirParser<(bool, ItemVisibility, bool)> { - keyword(Keyword::Unconstrained) - .or_not() - .then(visibility_modifier()) - .then(maybe_comp_time()) - .map(|((unconstrained, visibility), comptime)| { - (unconstrained.is_some(), visibility, comptime) - }) + keyword(Keyword::Unconstrained).or_not().then(item_visibility()).then(maybe_comp_time()).map( + |((unconstrained, visibility), comptime)| (unconstrained.is_some(), visibility, comptime), + ) } pub(super) fn numeric_generic() -> impl NoirParser { @@ -142,14 +138,12 @@ pub(super) fn generics() -> impl NoirParser { pub(super) fn function_return_type() -> impl NoirParser<(Visibility, FunctionReturnType)> { #[allow(deprecated)] - just(Token::Arrow) - .ignore_then(optional_visibility()) - .then(spanned(parse_type())) - .or_not() - .map_with_span(|ret, span| match ret { + just(Token::Arrow).ignore_then(visibility()).then(spanned(parse_type())).or_not().map_with_span( + |ret, span| match ret { Some((visibility, (ty, _))) => (visibility, FunctionReturnType::Ty(ty)), None => (Visibility::Private, FunctionReturnType::Default(span)), - }) + }, + ) } fn function_parameters<'a>(allow_self: bool) -> impl NoirParser> + 'a { @@ -158,7 +152,7 @@ fn function_parameters<'a>(allow_self: bool) -> impl NoirParser> + 'a let full_parameter = pattern() .recover_via(parameter_name_recovery()) .then_ignore(just(Token::Colon)) - .then(optional_visibility()) + .then(visibility()) .then(typ) .map_with_span(|((pattern, visibility), typ), span| Param { visibility, diff --git a/compiler/noirc_frontend/src/parser/parser/visibility.rs b/compiler/noirc_frontend/src/parser/parser/visibility.rs index d9c1abf2123..ea46becc932 100644 --- a/compiler/noirc_frontend/src/parser/parser/visibility.rs +++ b/compiler/noirc_frontend/src/parser/parser/visibility.rs @@ -4,15 +4,15 @@ use chumsky::{ }; use crate::{ - ast::ItemVisibility, + ast::{ItemVisibility, Visibility}, parser::NoirParser, token::{Keyword, Token}, }; -use super::primitives::keyword; +use super::{call_data, primitives::keyword}; /// visibility_modifier: 'pub(crate)'? 'pub'? '' -pub(crate) fn visibility_modifier() -> impl NoirParser { +pub(crate) fn item_visibility() -> impl NoirParser { let is_pub_crate = (keyword(Keyword::Pub) .then_ignore(just(Token::LeftParen)) .then_ignore(keyword(Keyword::Crate)) @@ -25,3 +25,12 @@ pub(crate) fn visibility_modifier() -> impl NoirParser { choice((is_pub_crate, is_pub, is_private)) } + +pub fn visibility() -> impl NoirParser { + keyword(Keyword::Pub) + .map(|_| Visibility::Public) + .or(call_data()) + .or(keyword(Keyword::ReturnData).map(|_| Visibility::ReturnData)) + .or_not() + .map(|opt| opt.unwrap_or(Visibility::Private)) +} diff --git a/docs/docs/noir/standard_library/meta/function_def.md b/docs/docs/noir/standard_library/meta/function_def.md index 3789fd82866..7c7531fb24a 100644 --- a/docs/docs/noir/standard_library/meta/function_def.md +++ b/docs/docs/noir/standard_library/meta/function_def.md @@ -73,3 +73,11 @@ each parameter pattern to be a syntactically valid parameter. Mutates the function's return type to a new type. This is only valid on functions in the current crate which have not yet been resolved. This means any functions called at compile-time are invalid targets for this method. + +### set_return_public + +#include_code set_return_public noir_stdlib/src/meta/function_def.nr rust + +Mutates the function's return visibility to public (if `true` is given) or private (if `false` is given). +This is only valid on functions in the current crate which have not yet been resolved. +This means any functions called at compile-time are invalid targets for this method. \ No newline at end of file diff --git a/noir_stdlib/src/meta/function_def.nr b/noir_stdlib/src/meta/function_def.nr index c1f2c026d57..0bff86ef102 100644 --- a/noir_stdlib/src/meta/function_def.nr +++ b/noir_stdlib/src/meta/function_def.nr @@ -43,4 +43,9 @@ impl FunctionDefinition { // docs:start:set_return_type fn set_return_type(self, return_type: Type) {} // docs:end:set_return_type + + #[builtin(function_def_set_return_public)] + // docs:start:set_return_public + fn set_return_public(self, public: bool) {} + // docs:end:set_return_public } diff --git a/test_programs/compile_success_empty/comptime_function_definition/src/main.nr b/test_programs/compile_success_empty/comptime_function_definition/src/main.nr index 8528bbce153..48651022b31 100644 --- a/test_programs/compile_success_empty/comptime_function_definition/src/main.nr +++ b/test_programs/compile_success_empty/comptime_function_definition/src/main.nr @@ -3,7 +3,7 @@ use std::meta::type_of; struct Foo { x: Field, field: Field } #[function_attr] -fn foo(w: i32, y: Field, Foo { x, field: some_field }: Foo, mut a: bool, (b, c): (i32, i32)) -> i32 { +pub fn foo(w: i32, y: Field, Foo { x, field: some_field }: Foo, mut a: bool, (b, c): (i32, i32)) -> i32 { let _ = (w, y, x, some_field, a, b, c); 1 } @@ -57,3 +57,15 @@ comptime fn mutate_add_one(f: FunctionDefinition) { fn main() { assert_eq(add_one(41), 42); } + +contract some_contract { + // No pub on the return type is an error + #[super::set_pub_return] + pub fn bar() -> Field { + 1 + } +} + +fn set_pub_return(f: FunctionDefinition) { + f.set_return_public(true); +} From 344dd5ea7ed551dcc3fd414d1c5f49f44721c28c Mon Sep 17 00:00:00 2001 From: Ary Borenszweig Date: Thu, 5 Sep 2024 16:16:04 -0300 Subject: [PATCH 24/25] feat: add `StructDefinition::add_attribute` and `has_named_attribute` (#5945) # Description ## Problem Resolves #5874 ## Summary ## Additional Context I originally wanted to test this by adding `abi(...)` to a struct and seeing that it errors if that struct appears in a signature but is outside of a contract, but... function parameters are type-checked before that (I think). So another way to test this is to also have `has_named_attribute`, so I added that too. Side note: maybe adding an attribute to a function/struct should eventually invoke the associated function, if there's any, but we can probably try to do that in a separate PR, and only if really needed. ## Documentation Check one: - [ ] No documentation needed. - [x] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist - [x] I have tested the changes locally. - [x] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- .../src/hir/comptime/interpreter/builtin.rs | 120 ++++++++++++------ .../interpreter/builtin/builtin_helpers.rs | 24 ++++ .../noir/standard_library/meta/struct_def.md | 12 ++ noir_stdlib/src/meta/struct_def.nr | 10 ++ .../attributes_struct/src/main.nr | 13 ++ 5 files changed, 137 insertions(+), 42 deletions(-) diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index a1bb99dbb46..072ac86b974 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -9,8 +9,8 @@ use builtin_helpers::{ check_one_argument, check_three_arguments, check_two_arguments, get_bool, get_expr, get_field, get_format_string, get_function_def, get_module, get_quoted, get_slice, get_struct, get_trait_constraint, get_trait_def, get_trait_impl, get_tuple, get_type, get_typed_expr, - get_u32, get_unresolved_type, hir_pattern_to_tokens, mutate_func_meta_type, parse, - replace_func_meta_parameters, replace_func_meta_return_type, + get_u32, get_unresolved_type, has_named_attribute, hir_pattern_to_tokens, + mutate_func_meta_type, parse, replace_func_meta_parameters, replace_func_meta_return_type, }; use chumsky::{chain::Chain, prelude::choice, Parser}; use im::Vector; @@ -25,7 +25,6 @@ use crate::{ FunctionReturnType, IntegerBitSize, LValue, Literal, Statement, StatementKind, UnaryOp, UnresolvedType, UnresolvedTypeData, Visibility, }, - elaborator::Elaborator, hir::comptime::{ errors::IResult, value::{ExprValue, TypedExpr}, @@ -135,9 +134,13 @@ impl<'local, 'context> Interpreter<'local, 'context> { "slice_push_front" => slice_push_front(interner, arguments, location), "slice_remove" => slice_remove(interner, arguments, location, call_stack), "str_as_bytes" => str_as_bytes(interner, arguments, location), + "struct_def_add_attribute" => struct_def_add_attribute(self, arguments, location), "struct_def_as_type" => struct_def_as_type(interner, arguments, location), "struct_def_fields" => struct_def_fields(interner, arguments, location), "struct_def_generics" => struct_def_generics(interner, arguments, location), + "struct_def_has_named_attribute" => { + struct_def_has_named_attribute(interner, arguments, location) + } "struct_def_set_fields" => struct_def_set_fields(interner, arguments, location), "to_le_radix" => to_le_radix(arguments, return_type, location), "trait_constraint_eq" => trait_constraint_eq(interner, arguments, location), @@ -268,6 +271,50 @@ fn str_as_bytes( Ok(Value::Array(bytes, byte_array_type)) } +// fn add_attribute(self, attribute: str) +fn struct_def_add_attribute( + interpreter: &mut Interpreter, + arguments: Vec<(Value, Location)>, + location: Location, +) -> IResult { + let (self_argument, attribute) = check_two_arguments(arguments, location)?; + let attribute_location = attribute.1; + let attribute = get_str(interpreter.elaborator.interner, attribute)?; + + let mut tokens = Lexer::lex(&format!("#[{}]", attribute)).0 .0; + if let Some(Token::EOF) = tokens.last().map(|token| token.token()) { + tokens.pop(); + } + if tokens.len() != 1 { + return Err(InterpreterError::InvalidAttribute { + attribute: attribute.to_string(), + location: attribute_location, + }); + } + + let token = tokens.into_iter().next().unwrap().into_token(); + let Token::Attribute(attribute) = token else { + return Err(InterpreterError::InvalidAttribute { + attribute: attribute.to_string(), + location: attribute_location, + }); + }; + + let Attribute::Secondary(attribute) = attribute else { + return Err(InterpreterError::InvalidAttribute { + attribute: attribute.to_string(), + location: attribute_location, + }); + }; + + let struct_id = get_struct(self_argument)?; + interpreter.elaborator.interner.update_struct_attributes(struct_id, |attributes| { + attributes.push(attribute.clone()); + }); + + Ok(Value::Unit) +} + /// fn as_type(self) -> Type fn struct_def_as_type( interner: &NodeInterner, @@ -305,6 +352,25 @@ fn struct_def_generics( Ok(Value::Slice(generics.collect(), typ)) } +// fn has_named_attribute(self, name: Quoted) -> bool +fn struct_def_has_named_attribute( + interner: &NodeInterner, + arguments: Vec<(Value, Location)>, + location: Location, +) -> IResult { + let (self_argument, name) = check_two_arguments(arguments, location)?; + let struct_id = get_struct(self_argument)?; + + let name = get_quoted(name)?; + let name = name.iter().map(|token| token.to_string()).collect::>().join(""); + + let attributes = interner.struct_attributes(&struct_id); + let attributes = attributes.iter().filter_map(|attribute| attribute.as_custom()); + let attributes = attributes.map(|attribute| &attribute.contents); + + Ok(Value::Bool(has_named_attribute(&name, attributes, location))) +} + /// fn fields(self) -> [(Quoted, Type)] /// Returns (name, type) pairs of each field of this StructDefinition fn struct_def_fields( @@ -1691,7 +1757,7 @@ fn function_def_add_attribute( }); } - let token = tokens[0].token(); + let token = tokens.into_iter().next().unwrap().into_token(); let Token::Attribute(attribute) = token else { return Err(InterpreterError::InvalidAttribute { attribute: attribute.to_string(), @@ -1704,7 +1770,7 @@ fn function_def_add_attribute( let function_modifiers = interpreter.elaborator.interner.function_modifiers_mut(&func_id); - match attribute { + match &attribute { Attribute::Function(attribute) => { function_modifiers.attributes.function = Some(attribute.clone()); } @@ -1715,7 +1781,7 @@ fn function_def_add_attribute( if let Attribute::Secondary(SecondaryAttribute::Custom(attribute)) = attribute { let func_meta = interpreter.elaborator.interner.function_meta_mut(&func_id); - func_meta.custom_attributes.push(attribute.clone()); + func_meta.custom_attributes.push(attribute); } Ok(Value::Unit) @@ -1745,31 +1811,15 @@ fn function_def_has_named_attribute( ) -> IResult { let (self_argument, name) = check_two_arguments(arguments, location)?; let func_id = get_function_def(self_argument)?; - let name = get_quoted(name)?; let func_meta = interner.function_meta(&func_id); - let attributes = &func_meta.custom_attributes; - if attributes.is_empty() { - return Ok(Value::Bool(false)); - }; + let name = get_quoted(name)?; let name = name.iter().map(|token| token.to_string()).collect::>().join(""); - for attribute in attributes { - let parse_result = Elaborator::parse_attribute(&attribute.contents, location); - let Ok(Some((function, _arguments))) = parse_result else { - continue; - }; - - let ExpressionKind::Variable(path) = function.kind else { - continue; - }; - - if path.last_name() == name { - return Ok(Value::Bool(true)); - } - } + let attributes = &func_meta.custom_attributes; + let attributes = attributes.iter().map(|attribute| &attribute.contents); - Ok(Value::Bool(false)) + Ok(Value::Bool(has_named_attribute(&name, attributes, location))) } // fn name(self) -> Quoted @@ -1990,27 +2040,13 @@ fn module_has_named_attribute( let (self_argument, name) = check_two_arguments(arguments, location)?; let module_id = get_module(self_argument)?; let module_data = interpreter.elaborator.get_module(module_id); - let name = get_quoted(name)?; + let name = get_quoted(name)?; let name = name.iter().map(|token| token.to_string()).collect::>().join(""); let attributes = module_data.outer_attributes.iter().chain(&module_data.inner_attributes); - for attribute in attributes { - let parse_result = Elaborator::parse_attribute(attribute, location); - let Ok(Some((function, _arguments))) = parse_result else { - continue; - }; - - let ExpressionKind::Variable(path) = function.kind else { - continue; - }; - - if path.last_name() == name { - return Ok(Value::Bool(true)); - } - } - Ok(Value::Bool(false)) + Ok(Value::Bool(has_named_attribute(&name, attributes, location))) } // fn is_contract(self) -> bool diff --git a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin/builtin_helpers.rs b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin/builtin_helpers.rs index 163b74efbe6..f90d50807b8 100644 --- a/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin/builtin_helpers.rs +++ b/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin/builtin_helpers.rs @@ -8,6 +8,7 @@ use crate::{ BlockExpression, ExpressionKind, IntegerBitSize, LValue, Signedness, StatementKind, UnresolvedTypeData, }, + elaborator::Elaborator, hir::{ comptime::{ errors::IResult, @@ -444,3 +445,26 @@ pub(super) fn block_expression_to_value(block_expr: BlockExpression) -> Value { Value::Slice(statements, typ) } + +pub(super) fn has_named_attribute<'a>( + name: &'a str, + attributes: impl Iterator, + location: Location, +) -> bool { + for attribute in attributes { + let parse_result = Elaborator::parse_attribute(attribute, location); + let Ok(Some((function, _arguments))) = parse_result else { + continue; + }; + + let ExpressionKind::Variable(path) = function.kind else { + continue; + }; + + if path.last_name() == name { + return true; + } + } + + false +} diff --git a/docs/docs/noir/standard_library/meta/struct_def.md b/docs/docs/noir/standard_library/meta/struct_def.md index 95e377dffd4..5da4a458d88 100644 --- a/docs/docs/noir/standard_library/meta/struct_def.md +++ b/docs/docs/noir/standard_library/meta/struct_def.md @@ -7,6 +7,12 @@ This type corresponds to `struct Name { field1: Type1, ... }` items in the sourc ## Methods +### add_attribute + +#include_code add_attribute noir_stdlib/src/meta/struct_def.nr rust + +Adds an attribute to the struct. + ### as_type #include_code as_type noir_stdlib/src/meta/struct_def.nr rust @@ -44,6 +50,12 @@ comptime fn example(foo: StructDefinition) { Returns each field of this struct as a pair of (field name, field type). +### has_named_attribute + +#include_code has_named_attribute noir_stdlib/src/meta/struct_def.nr rust + +Returns true if this struct has a custom attribute with the given name. + ### set_fields #include_code set_fields noir_stdlib/src/meta/struct_def.nr rust diff --git a/noir_stdlib/src/meta/struct_def.nr b/noir_stdlib/src/meta/struct_def.nr index 1ca1b6a3925..6c0270a8eec 100644 --- a/noir_stdlib/src/meta/struct_def.nr +++ b/noir_stdlib/src/meta/struct_def.nr @@ -1,4 +1,9 @@ impl StructDefinition { + #[builtin(struct_def_add_attribute)] + // docs:start:add_attribute + fn add_attribute(self, attribute: str) {} + // docs:end:add_attribute + /// Return a syntactic version of this struct definition as a type. /// For example, `as_type(quote { type Foo { ... } })` would return `Foo` #[builtin(struct_def_as_type)] @@ -6,6 +11,11 @@ impl StructDefinition { fn as_type(self) -> Type {} // docs:end:as_type + #[builtin(struct_def_has_named_attribute)] + // docs:start:has_named_attribute + fn has_named_attribute(self, name: Quoted) -> bool {} + // docs:end:has_named_attribute + /// Return each generic on this struct. #[builtin(struct_def_generics)] // docs:start:generics diff --git a/test_programs/compile_success_empty/attributes_struct/src/main.nr b/test_programs/compile_success_empty/attributes_struct/src/main.nr index 0bad42aee57..25c9402035c 100644 --- a/test_programs/compile_success_empty/attributes_struct/src/main.nr +++ b/test_programs/compile_success_empty/attributes_struct/src/main.nr @@ -6,3 +6,16 @@ struct SomeStruct { } fn main() {} + +// Check that add_attribute and has_named_attribute work well + +#[add_attribute] +struct Foo { + +} + +fn add_attribute(s: StructDefinition) { + assert(!s.has_named_attribute(quote { foo })); + s.add_attribute("foo"); + assert(s.has_named_attribute(quote { foo })); +} From ce34fbd19702b71426563a589235a2c5a1efb265 Mon Sep 17 00:00:00 2001 From: Maxim Vezenov Date: Thu, 5 Sep 2024 16:39:38 -0400 Subject: [PATCH 25/25] fix(frontend): Ban type vars bound to a reference from passing the unconstrained boundary (#5949) # Description ## Problem\* Resolves Quick fix found while searching for other bugs ## Summary\* I have provided an example test which would previously panic during ACIR gen about all references not being resolved. However, we should have been allowing the code in the first place as it was attempting to pass a mutable reference from the constrained boundary to the unconstrained boundary. ## Additional Context ## Documentation\* Check one: - [X] No documentation needed. - [ ] Documentation included in this PR. - [ ] **[For Experimental Features]** Documentation to be submitted in a separate PR. # PR Checklist\* - [X] I have tested the changes locally. - [X] I have formatted the changes with [Prettier](https://prettier.io/) and/or `cargo fmt` on default settings. --- compiler/noirc_frontend/src/hir_def/types.rs | 10 +++++-- compiler/noirc_frontend/src/tests.rs | 29 ++++++++++++++++++++ 2 files changed, 37 insertions(+), 2 deletions(-) diff --git a/compiler/noirc_frontend/src/hir_def/types.rs b/compiler/noirc_frontend/src/hir_def/types.rs index 113a4fb3888..d47e6522756 100644 --- a/compiler/noirc_frontend/src/hir_def/types.rs +++ b/compiler/noirc_frontend/src/hir_def/types.rs @@ -1099,13 +1099,19 @@ impl Type { | Type::Unit | Type::Constant(_) | Type::Slice(_) - | Type::TypeVariable(_, _) - | Type::NamedGeneric(_, _, _) | Type::Function(_, _, _, _) | Type::FmtString(_, _) | Type::InfixExpr(..) | Type::Error => true, + Type::TypeVariable(type_var, _) | Type::NamedGeneric(type_var, _, _) => { + if let TypeBinding::Bound(typ) = &*type_var.borrow() { + typ.is_valid_for_unconstrained_boundary() + } else { + true + } + } + // Quoted objects only exist at compile-time where the only execution // environment is the interpreter. In this environment, they are valid. Type::Quoted(_) => true, diff --git a/compiler/noirc_frontend/src/tests.rs b/compiler/noirc_frontend/src/tests.rs index 04c4e414858..7160bee4931 100644 --- a/compiler/noirc_frontend/src/tests.rs +++ b/compiler/noirc_frontend/src/tests.rs @@ -3424,3 +3424,32 @@ fn errors_on_unused_function() { assert_eq!(ident.to_string(), "foo"); assert_eq!(*item_type, "function"); } + +#[test] +fn constrained_reference_to_unconstrained() { + let src = r#" + fn main(mut x: u32, y: pub u32) { + let x_ref = &mut x; + if x == 5 { + unsafe { + mut_ref_input(x_ref, y); + } + } + + assert(x == 10); + } + + unconstrained fn mut_ref_input(x: &mut u32, y: u32) { + *x = y; + } + "#; + + let errors = get_program_errors(src); + assert_eq!(errors.len(), 1); + + let CompilationError::TypeError(TypeCheckError::ConstrainedReferenceToUnconstrained { .. }) = + &errors[0].0 + else { + panic!("Expected an error about passing a constrained reference to unconstrained"); + }; +}