diff --git a/.noir-sync-commit b/.noir-sync-commit index b64f398c068..3a367e22506 100644 --- a/.noir-sync-commit +++ b/.noir-sync-commit @@ -1 +1 @@ -9471e28ad6f02bf2fae3782c3db68106b615595f +c172880ae47ec4906cda662801bd4b7866c9586b diff --git a/noir/noir-repo/acvm-repo/acvm/src/compiler/optimizers/unused_memory.rs b/noir/noir-repo/acvm-repo/acvm/src/compiler/optimizers/unused_memory.rs index 1963430210f..1325a3b03cd 100644 --- a/noir/noir-repo/acvm-repo/acvm/src/compiler/optimizers/unused_memory.rs +++ b/noir/noir-repo/acvm-repo/acvm/src/compiler/optimizers/unused_memory.rs @@ -1,4 +1,4 @@ -use acir::circuit::{opcodes::BlockId, Circuit, Opcode}; +use acir::circuit::{brillig::BrilligInputs, opcodes::BlockId, Circuit, Opcode}; use std::collections::HashSet; /// `UnusedMemoryOptimizer` will remove initializations of memory blocks which are unused. @@ -29,6 +29,13 @@ impl UnusedMemoryOptimizer { Opcode::MemoryOp { block_id, .. } => { unused_memory_initialization.remove(block_id); } + Opcode::BrilligCall { inputs, .. } => { + for input in inputs { + if let BrilligInputs::MemoryArray(block) = input { + unused_memory_initialization.remove(block); + } + } + } _ => (), } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/acir/acir_variable.rs b/noir/noir-repo/compiler/noirc_evaluator/src/acir/acir_variable.rs index cf6b1fcc7f7..41e2c2dad1e 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/acir/acir_variable.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/acir/acir_variable.rs @@ -541,6 +541,29 @@ impl> AcirContext { Ok(()) } + /// Constrains the `lhs` and `rhs` to be non-equal. + /// + /// This is done by asserting the existence of an inverse for the value `lhs - rhs`. + /// The constraint `(lhs - rhs) * inverse == 1` will only be satisfiable if `lhs` and `rhs` are non-equal. + pub(crate) fn assert_neq_var( + &mut self, + lhs: AcirVar, + rhs: AcirVar, + assert_message: Option>, + ) -> Result<(), RuntimeError> { + let diff_var = self.sub_var(lhs, rhs)?; + + let one = self.add_constant(F::one()); + let _ = self.inv_var(diff_var, one)?; + if let Some(payload) = assert_message { + self.acir_ir + .assertion_payloads + .insert(self.acir_ir.last_acir_opcode_location(), payload); + } + + Ok(()) + } + pub(crate) fn vars_to_expressions_or_memory( &self, values: &[AcirValue], diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/acir/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/acir/mod.rs index a250189d3f1..137d0f3c28e 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/acir/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/acir/mod.rs @@ -723,6 +723,47 @@ impl<'a> Context<'a> { self.acir_context.assert_eq_var(lhs, rhs, assert_payload)?; } + Instruction::ConstrainNotEqual(lhs, rhs, assert_message) => { + let lhs = self.convert_numeric_value(*lhs, dfg)?; + let rhs = self.convert_numeric_value(*rhs, dfg)?; + + let assert_payload = if let Some(error) = assert_message { + match error { + ConstrainError::StaticString(string) => Some( + self.acir_context.generate_assertion_message_payload(string.clone()), + ), + ConstrainError::Dynamic(error_selector, is_string_type, values) => { + if let Some(constant_string) = try_to_extract_string_from_error_payload( + *is_string_type, + values, + dfg, + ) { + Some( + self.acir_context + .generate_assertion_message_payload(constant_string), + ) + } else { + let acir_vars: Vec<_> = values + .iter() + .map(|value| self.convert_value(*value, dfg)) + .collect(); + + let expressions_or_memory = + self.acir_context.vars_to_expressions_or_memory(&acir_vars)?; + + Some(AssertionPayload { + error_selector: error_selector.as_u64(), + payload: expressions_or_memory, + }) + } + } + } + } else { + None + }; + + self.acir_context.assert_neq_var(lhs, rhs, assert_payload)?; + } Instruction::Cast(value_id, _) => { let acir_var = self.convert_numeric_value(*value_id, dfg)?; self.define_result_var(dfg, instruction_id, acir_var); diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index e9bc6b127f7..ec918c51ff1 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -279,6 +279,10 @@ impl<'block> BrilligBlock<'block> { self.brillig_context.deallocate_single_addr(condition); } } + Instruction::ConstrainNotEqual(..) => { + unreachable!("only implemented in ACIR") + } + Instruction::Allocate => { let result_value = dfg.instruction_results(instruction_id)[0]; let pointer = self.variables.define_single_addr_variable( diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs index 45021fa6158..ed515bbe98c 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs @@ -186,6 +186,7 @@ fn optimize_all(builder: SsaBuilder, options: &SsaEvaluatorOptions) -> Result { + Instruction::Constrain(value_id1, value_id2, _) + | Instruction::ConstrainNotEqual(value_id1, value_id2, _) => { self.clear_constrained( &[function.dfg.resolve(*value_id1), function.dfg.resolve(*value_id2)], function, @@ -555,6 +556,7 @@ impl Context { | Instruction::Binary(..) | Instruction::Cast(..) | Instruction::Constrain(..) + | Instruction::ConstrainNotEqual(..) | Instruction::IfElse { .. } | Instruction::Load { .. } | Instruction::Not(..) diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs index 3d7e0b06d5b..154d485b143 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs @@ -19,7 +19,7 @@ use super::{ ir::{ basic_block::BasicBlock, call_stack::{CallStack, CallStackId}, - dfg::InsertInstructionResult, + dfg::{GlobalsGraph, InsertInstructionResult}, function::RuntimeType, instruction::{ConstrainError, InstructionId, Intrinsic}, types::NumericType, @@ -73,6 +73,13 @@ impl FunctionBuilder { self.current_function.set_runtime(runtime); } + pub(crate) fn set_globals(&mut self, globals: Arc) { + for (_, value) in globals.values_iter() { + self.current_function.dfg.make_global(value.get_type().into_owned()); + } + self.current_function.set_globals(globals); + } + /// Finish the current function and create a new function. /// /// A FunctionBuilder can always only work on one function at a time, so care diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/dfg.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/dfg.rs index 6eae291ca47..28242b223ac 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/dfg.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/dfg.rs @@ -1,4 +1,4 @@ -use std::borrow::Cow; +use std::{borrow::Cow, sync::Arc}; use crate::ssa::{function_builder::data_bus::DataBus, ir::instruction::SimplifyResult}; @@ -102,6 +102,31 @@ pub(crate) struct DataFlowGraph { #[serde(skip)] pub(crate) data_bus: DataBus, + + pub(crate) globals: Arc, +} + +/// The GlobalsGraph contains the actual global data. +/// Global data is expected to only be numeric constants or array constants (which are represented by Instruction::MakeArray). +/// The global's data will shared across functions and should be accessible inside of a function's DataFlowGraph. +#[derive(Debug, Clone, Serialize, Deserialize, Default)] +pub(crate) struct GlobalsGraph { + /// Storage for all of the global values + values: DenseMap, + /// All of the instructions in the global value space. + /// These are expected to all be Instruction::MakeArray + instructions: DenseMap, +} + +impl GlobalsGraph { + pub(crate) fn from_dfg(dfg: DataFlowGraph) -> Self { + Self { values: dfg.values, instructions: dfg.instructions } + } + + /// Iterate over every Value in this DFG in no particular order, including unused Values + pub(crate) fn values_iter(&self) -> impl ExactSizeIterator { + self.values.iter() + } } impl DataFlowGraph { @@ -235,6 +260,24 @@ impl DataFlowGraph { block: BasicBlockId, ctrl_typevars: Option>, call_stack: CallStackId, + ) -> InsertInstructionResult { + self.insert_instruction_and_results_if_simplified( + instruction, + block, + ctrl_typevars, + call_stack, + None, + ) + } + + /// Simplifies a potentially existing instruction and inserts it only if it changed. + pub(crate) fn insert_instruction_and_results_if_simplified( + &mut self, + instruction: Instruction, + block: BasicBlockId, + ctrl_typevars: Option>, + call_stack: CallStackId, + existing_id: Option, ) -> InsertInstructionResult { if !self.is_handled_by_runtime(&instruction) { panic!("Attempted to insert instruction not handled by runtime: {instruction:?}"); @@ -251,7 +294,20 @@ impl DataFlowGraph { result @ (SimplifyResult::SimplifiedToInstruction(_) | SimplifyResult::SimplifiedToInstructionMultiple(_) | SimplifyResult::None) => { - let mut instructions = result.instructions().unwrap_or(vec![instruction]); + let instructions = result.instructions(); + if instructions.is_none() { + if let Some(id) = existing_id { + if self[id] == instruction { + // Just (re)insert into the block, no need to redefine. + self.blocks[block].insert_instruction(id); + return InsertInstructionResult::Results( + id, + self.instruction_results(id), + ); + } + } + } + let mut instructions = instructions.unwrap_or(vec![instruction]); assert!(!instructions.is_empty(), "`SimplifyResult::SimplifiedToInstructionMultiple` must not return empty vector"); if instructions.len() > 1 { @@ -511,7 +567,7 @@ impl DataFlowGraph { &self, value: ValueId, ) -> Option<(FieldElement, NumericType)> { - match &self.values[self.resolve(value)] { + match &self[self.resolve(value)] { Value::NumericConstant { constant, typ } => Some((*constant, *typ)), _ => None, } @@ -520,13 +576,15 @@ impl DataFlowGraph { /// Returns the Value::Array associated with this ValueId if it refers to an array constant. /// Otherwise, this returns None. pub(crate) fn get_array_constant(&self, value: ValueId) -> Option<(im::Vector, Type)> { - match &self.values[self.resolve(value)] { - Value::Instruction { instruction, .. } => match &self.instructions[*instruction] { + let value = self.resolve(value); + if let Some(instruction) = self.get_local_or_global_instruction(value) { + match instruction { Instruction::MakeArray { elements, typ } => Some((elements.clone(), typ.clone())), _ => None, - }, + } + } else { // Arrays are shared, so cloning them is cheap - _ => None, + None } } @@ -619,17 +677,23 @@ impl DataFlowGraph { /// True if the given ValueId refers to a (recursively) constant value pub(crate) fn is_constant(&self, argument: ValueId) -> bool { - match &self[self.resolve(argument)] { + let argument = self.resolve(argument); + match &self[argument] { Value::Param { .. } => false, - Value::Instruction { instruction, .. } => match &self[*instruction] { - Instruction::MakeArray { elements, .. } => { - elements.iter().all(|element| self.is_constant(*element)) + Value::Instruction { .. } => { + let Some(instruction) = self.get_local_or_global_instruction(argument) else { + return false; + }; + match &instruction { + Instruction::MakeArray { elements, .. } => { + elements.iter().all(|element| self.is_constant(*element)) + } + _ => false, } - _ => false, - }, - // TODO: Make this true and handle instruction simplifications with globals. - // Currently all globals are inlined as a temporary measure so this is fine to have as false. - Value::Global(_) => false, + } + Value::Global(_) => { + unreachable!("The global value should have been indexed from the global space"); + } _ => true, } } @@ -642,6 +706,29 @@ impl DataFlowGraph { false } } + + pub(crate) fn is_global(&self, value: ValueId) -> bool { + matches!(self.values[value], Value::Global(_)) + } + + /// Uses value information to determine whether an instruction is from + /// this function's DFG or the global space's DFG. + pub(crate) fn get_local_or_global_instruction(&self, value: ValueId) -> Option<&Instruction> { + match &self[value] { + Value::Instruction { instruction, .. } => { + let instruction = if self.is_global(value) { + let instruction = &self.globals[*instruction]; + // We expect to only have MakeArray instructions in the global space + assert!(matches!(instruction, Instruction::MakeArray { .. })); + instruction + } else { + &self[*instruction] + }; + Some(instruction) + } + _ => None, + } + } } impl std::ops::Index for DataFlowGraph { @@ -660,7 +747,11 @@ impl std::ops::IndexMut for DataFlowGraph { impl std::ops::Index for DataFlowGraph { type Output = Value; fn index(&self, id: ValueId) -> &Self::Output { - &self.values[id] + let value = &self.values[id]; + if matches!(value, Value::Global(_)) { + return &self.globals[id]; + } + value } } @@ -678,6 +769,20 @@ impl std::ops::IndexMut for DataFlowGraph { } } +impl std::ops::Index for GlobalsGraph { + type Output = Value; + fn index(&self, id: ValueId) -> &Self::Output { + &self.values[id] + } +} + +impl std::ops::Index for GlobalsGraph { + type Output = Instruction; + fn index(&self, id: InstructionId) -> &Self::Output { + &self.instructions[id] + } +} + // The result of calling DataFlowGraph::insert_instruction can // be a list of results or a single ValueId if the instruction was simplified // to an existing value. diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/function.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/function.rs index a2068d94661..b59b0c18a10 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/function.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/function.rs @@ -1,11 +1,12 @@ use std::collections::BTreeSet; +use std::sync::Arc; use iter_extended::vecmap; use noirc_frontend::monomorphization::ast::InlineType; use serde::{Deserialize, Serialize}; use super::basic_block::BasicBlockId; -use super::dfg::DataFlowGraph; +use super::dfg::{DataFlowGraph, GlobalsGraph}; use super::instruction::TerminatorInstruction; use super::map::Id; use super::types::Type; @@ -112,6 +113,7 @@ impl Function { pub(crate) fn clone_signature(id: FunctionId, another: &Function) -> Self { let mut new_function = Function::new(another.name.clone(), id); new_function.set_runtime(another.runtime()); + new_function.set_globals(another.dfg.globals.clone()); new_function } @@ -136,6 +138,10 @@ impl Function { self.dfg.set_runtime(runtime); } + pub(crate) fn set_globals(&mut self, globals: Arc) { + self.dfg.globals = globals; + } + pub(crate) fn is_no_predicates(&self) -> bool { match self.runtime() { RuntimeType::Acir(inline_type) => matches!(inline_type, InlineType::NoPredicates), diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs index 786c3671d38..171ca30f5f4 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs @@ -256,6 +256,9 @@ pub(crate) enum Instruction { /// Constrains two values to be equal to one another. Constrain(ValueId, ValueId, Option), + /// Constrains two values to not be equal to one another. + ConstrainNotEqual(ValueId, ValueId, Option), + /// Range constrain `value` to `max_bit_size` RangeCheck { value: ValueId, max_bit_size: u32, assert_message: Option }, @@ -364,6 +367,7 @@ impl Instruction { InstructionResultType::Operand(*value) } Instruction::Constrain(..) + | Instruction::ConstrainNotEqual(..) | Instruction::Store { .. } | Instruction::IncrementRc { .. } | Instruction::DecrementRc { .. } @@ -405,7 +409,7 @@ impl Instruction { }, // These can fail. - Constrain(..) | RangeCheck { .. } => true, + Constrain(..) | ConstrainNotEqual(..) | RangeCheck { .. } => true, // This should never be side-effectful MakeArray { .. } | Noop => false, @@ -472,7 +476,7 @@ impl Instruction { }, // We can deduplicate these instructions if we know the predicate is also the same. - Constrain(..) | RangeCheck { .. } => deduplicate_with_predicate, + Constrain(..) | ConstrainNotEqual(..) | RangeCheck { .. } => deduplicate_with_predicate, // Noop instructions can always be deduplicated, although they're more likely to be // removed entirely. @@ -540,6 +544,7 @@ impl Instruction { } Constrain(..) + | ConstrainNotEqual(..) | EnableSideEffectsIf { .. } | IncrementRc { .. } | DecrementRc { .. } @@ -610,6 +615,7 @@ impl Instruction { Instruction::Cast(_, _) | Instruction::Not(_) | Instruction::Truncate { .. } + | Instruction::ConstrainNotEqual(..) | Instruction::Constrain(_, _, _) | Instruction::RangeCheck { .. } | Instruction::Allocate @@ -656,6 +662,22 @@ impl Instruction { }); Instruction::Constrain(lhs, rhs, assert_message) } + Instruction::ConstrainNotEqual(lhs, rhs, assert_message) => { + // Must map the `lhs` and `rhs` first as the value `f` is moved with the closure + let lhs = f(*lhs); + let rhs = f(*rhs); + let assert_message = assert_message.as_ref().map(|error| match error { + ConstrainError::Dynamic(selector, is_string, payload_values) => { + ConstrainError::Dynamic( + *selector, + *is_string, + payload_values.iter().map(|&value| f(value)).collect(), + ) + } + _ => error.clone(), + }); + Instruction::ConstrainNotEqual(lhs, rhs, assert_message) + } Instruction::Call { func, arguments } => Instruction::Call { func: f(*func), arguments: vecmap(arguments.iter().copied(), f), @@ -714,7 +736,8 @@ impl Instruction { Instruction::Truncate { value, bit_size: _, max_bit_size: _ } => { *value = f(*value); } - Instruction::Constrain(lhs, rhs, assert_message) => { + Instruction::Constrain(lhs, rhs, assert_message) + | Instruction::ConstrainNotEqual(lhs, rhs, assert_message) => { *lhs = f(*lhs); *rhs = f(*rhs); if let Some(ConstrainError::Dynamic(_, _, payload_values)) = assert_message { @@ -786,7 +809,8 @@ impl Instruction { | Instruction::Load { address: value } => { f(*value); } - Instruction::Constrain(lhs, rhs, assert_error) => { + Instruction::Constrain(lhs, rhs, assert_error) + | Instruction::ConstrainNotEqual(lhs, rhs, assert_error) => { f(*lhs); f(*rhs); if let Some(ConstrainError::Dynamic(_, _, values)) = assert_error.as_ref() { @@ -878,6 +902,7 @@ impl Instruction { SimplifiedToInstructionMultiple(constraints) } } + Instruction::ConstrainNotEqual(..) => None, Instruction::ArrayGet { array, index } => { if let Some(index) = dfg.get_numeric_constant(*index) { try_optimize_array_get_from_previous_set(dfg, *array, index) @@ -1093,28 +1118,27 @@ fn try_optimize_array_get_from_previous_set( // Arbitrary number of maximum tries just to prevent this optimization from taking too long. let max_tries = 5; for _ in 0..max_tries { - match &dfg[array_id] { - Value::Instruction { instruction, .. } => { - match &dfg[*instruction] { - Instruction::ArraySet { array, index, value, .. } => { - if let Some(constant) = dfg.get_numeric_constant(*index) { - if constant == target_index { - return SimplifyResult::SimplifiedTo(*value); - } - - array_id = *array; // recur - } else { - return SimplifyResult::None; + if let Some(instruction) = dfg.get_local_or_global_instruction(array_id) { + match instruction { + Instruction::ArraySet { array, index, value, .. } => { + if let Some(constant) = dfg.get_numeric_constant(*index) { + if constant == target_index { + return SimplifyResult::SimplifiedTo(*value); } + + array_id = *array; // recur + } else { + return SimplifyResult::None; } - Instruction::MakeArray { elements: array, typ: _ } => { - elements = Some(array.clone()); - break; - } - _ => return SimplifyResult::None, } + Instruction::MakeArray { elements: array, typ: _ } => { + elements = Some(array.clone()); + break; + } + _ => return SimplifyResult::None, } - _ => return SimplifyResult::None, + } else { + return SimplifyResult::None; } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs index 7fe12b83ea9..85f8dcaba48 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs @@ -85,7 +85,13 @@ fn value(dfg: &DataFlowGraph, id: ValueId) -> String { Value::Function(id) => id.to_string(), Value::Intrinsic(intrinsic) => intrinsic.to_string(), Value::ForeignFunction(function) => function.clone(), - Value::Param { .. } | Value::Instruction { .. } => id.to_string(), + Value::Param { .. } | Value::Instruction { .. } => { + if dfg.is_global(id) { + format!("g{}", id.to_u32()) + } else { + id.to_string() + } + } Value::Global(_) => { format!("g{}", id.to_u32()) } @@ -163,13 +169,14 @@ fn display_instruction( write!(f, "{} = ", value_list)?; } - display_instruction_inner(dfg, &dfg[instruction], results, f) + display_instruction_inner(dfg, &dfg[instruction], results, in_global_space, f) } fn display_instruction_inner( dfg: &DataFlowGraph, instruction: &Instruction, results: &[ValueId], + in_global_space: bool, f: &mut Formatter, ) -> Result { let show = |id| value(dfg, id); @@ -192,6 +199,14 @@ fn display_instruction_inner( writeln!(f) } } + Instruction::ConstrainNotEqual(lhs, rhs, error) => { + write!(f, "constrain {} != {}", show(*lhs), show(*rhs))?; + if let Some(error) = error { + display_constrain_error(dfg, error, f) + } else { + writeln!(f) + } + } Instruction::Call { func, arguments } => { let arguments = value_list(dfg, arguments); writeln!(f, "call {}({}){}", show(*func), arguments, result_types(dfg, results)) @@ -272,7 +287,11 @@ fn display_instruction_inner( if i != 0 { write!(f, ", ")?; } - write!(f, "{}", show(*element))?; + let mut value = show(*element); + if in_global_space { + value = value.replace('v', "g"); + } + write!(f, "{}", value)?; } writeln!(f, "] : {typ}") diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs index 5b75055c09a..e8cae7da5b5 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/constant_folding.rs @@ -423,14 +423,18 @@ impl<'brillig> Context<'brillig> { .then(|| vecmap(old_results, |result| dfg.type_of_value(*result))); let call_stack = dfg.get_instruction_call_stack_id(id); - let new_results = - match dfg.insert_instruction_and_results(instruction, block, ctrl_typevars, call_stack) - { - InsertInstructionResult::SimplifiedTo(new_result) => vec![new_result], - InsertInstructionResult::SimplifiedToMultiple(new_results) => new_results, - InsertInstructionResult::Results(_, new_results) => new_results.to_vec(), - InsertInstructionResult::InstructionRemoved => vec![], - }; + let new_results = match dfg.insert_instruction_and_results_if_simplified( + instruction, + block, + ctrl_typevars, + call_stack, + Some(id), + ) { + InsertInstructionResult::SimplifiedTo(new_result) => vec![new_result], + InsertInstructionResult::SimplifiedToMultiple(new_results) => new_results, + InsertInstructionResult::Results(_, new_results) => new_results.to_vec(), + InsertInstructionResult::InstructionRemoved => vec![], + }; // Optimizations while inserting the instruction should not change the number of results. assert_eq!(old_results.len(), new_results.len()); diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/defunctionalize.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/defunctionalize.rs index 7d7798fd30a..186f10c53e6 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/defunctionalize.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/defunctionalize.rs @@ -278,8 +278,10 @@ fn create_apply_function( function_ids: Vec, ) -> FunctionId { assert!(!function_ids.is_empty()); + let globals = ssa.functions[&function_ids[0]].dfg.globals.clone(); ssa.add_fn(|id| { let mut function_builder = FunctionBuilder::new("apply".to_string(), id); + function_builder.set_globals(globals); let target_id = function_builder.add_parameter(Type::field()); let params_ids = vecmap(signature.params, |typ| function_builder.add_parameter(typ)); diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/inlining.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/inlining.rs index b1dd203cfd0..b5cbc90e30d 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/inlining.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/inlining.rs @@ -46,29 +46,48 @@ impl Ssa { /// This step should run after runtime separation, since it relies on the runtime of the called functions being final. #[tracing::instrument(level = "trace", skip(self))] pub(crate) fn inline_functions(self, aggressiveness: i64) -> Ssa { - Self::inline_functions_inner(self, aggressiveness, false) + let inline_sources = get_functions_to_inline_into(&self, false, aggressiveness); + Self::inline_functions_inner(self, &inline_sources, false) } // Run the inlining pass where functions marked with `InlineType::NoPredicates` as not entry points pub(crate) fn inline_functions_with_no_predicates(self, aggressiveness: i64) -> Ssa { - Self::inline_functions_inner(self, aggressiveness, true) + let inline_sources = get_functions_to_inline_into(&self, true, aggressiveness); + Self::inline_functions_inner(self, &inline_sources, true) } fn inline_functions_inner( mut self, - aggressiveness: i64, + inline_sources: &BTreeSet, inline_no_predicates_functions: bool, ) -> Ssa { - let inline_sources = - get_functions_to_inline_into(&self, inline_no_predicates_functions, aggressiveness); - self.functions = btree_map(&inline_sources, |entry_point| { - let new_function = InlineContext::new( - &self, - *entry_point, - inline_no_predicates_functions, - inline_sources.clone(), - ) - .inline_all(&self); + // Note that we clear all functions other than those in `inline_sources`. + // If we decide to do partial inlining then we should change this to preserve those functions which still exist. + self.functions = btree_map(inline_sources, |entry_point| { + let should_inline_call = + |_context: &PerFunctionContext, ssa: &Ssa, called_func_id: FunctionId| -> bool { + let function = &ssa.functions[&called_func_id]; + + match function.runtime() { + RuntimeType::Acir(inline_type) => { + // If the called function is acir, we inline if it's not an entry point + + // If we have not already finished the flattening pass, functions marked + // to not have predicates should be preserved. + let preserve_function = + !inline_no_predicates_functions && function.is_no_predicates(); + !inline_type.is_entry_point() && !preserve_function + } + RuntimeType::Brillig(_) => { + // If the called function is brillig, we inline only if it's into brillig and the function is not recursive + ssa.functions[entry_point].runtime().is_brillig() + && !inline_sources.contains(&called_func_id) + } + } + }; + + let new_function = + InlineContext::new(&self, *entry_point).inline_all(&self, &should_inline_call); (*entry_point, new_function) }); self @@ -88,16 +107,6 @@ struct InlineContext { // The FunctionId of the entry point function we're inlining into in the old, unmodified Ssa. entry_point: FunctionId, - - /// Whether the inlining pass should inline any functions marked with [`InlineType::NoPredicates`] - /// or whether these should be preserved as entrypoint functions. - /// - /// This is done as we delay inlining of functions with the attribute `#[no_predicates]` until after - /// the control flow graph has been flattened. - inline_no_predicates_functions: bool, - - // These are the functions of the program that we shouldn't inline. - functions_not_to_inline: BTreeSet, } /// The per-function inlining context contains information that is only valid for one function. @@ -355,34 +364,27 @@ impl InlineContext { /// The function being inlined into will always be the main function, although it is /// actually a copy that is created in case the original main is still needed from a function /// that could not be inlined calling it. - fn new( - ssa: &Ssa, - entry_point: FunctionId, - inline_no_predicates_functions: bool, - functions_not_to_inline: BTreeSet, - ) -> Self { + fn new(ssa: &Ssa, entry_point: FunctionId) -> Self { let source = &ssa.functions[&entry_point]; let mut builder = FunctionBuilder::new(source.name().to_owned(), entry_point); builder.set_runtime(source.runtime()); - Self { - builder, - recursion_level: 0, - entry_point, - call_stack: CallStackId::root(), - inline_no_predicates_functions, - functions_not_to_inline, - } + builder.current_function.set_globals(source.dfg.globals.clone()); + + Self { builder, recursion_level: 0, entry_point, call_stack: CallStackId::root() } } /// Start inlining the entry point function and all functions reachable from it. - fn inline_all(mut self, ssa: &Ssa) -> Function { + fn inline_all( + mut self, + ssa: &Ssa, + should_inline_call: &impl Fn(&PerFunctionContext, &Ssa, FunctionId) -> bool, + ) -> Function { let entry_point = &ssa.functions[&self.entry_point]; - // let globals = self.globals; let mut context = PerFunctionContext::new(&mut self, entry_point, &ssa.globals); context.inlining_entry = true; - for (_, value) in ssa.globals.dfg.values_iter() { + for (_, value) in entry_point.dfg.globals.values_iter() { context.context.builder.current_function.dfg.make_global(value.get_type().into_owned()); } @@ -399,7 +401,7 @@ impl InlineContext { } context.blocks.insert(context.source_function.entry_block(), entry_block); - context.inline_blocks(ssa); + context.inline_blocks(ssa, should_inline_call); // translate databus values let databus = entry_point.dfg.data_bus.map_values(|t| context.translate_value(t)); @@ -418,6 +420,7 @@ impl InlineContext { ssa: &Ssa, id: FunctionId, arguments: &[ValueId], + should_inline_call: &impl Fn(&PerFunctionContext, &Ssa, FunctionId) -> bool, ) -> Vec { self.recursion_level += 1; @@ -438,7 +441,7 @@ impl InlineContext { let current_block = context.context.builder.current_block(); context.blocks.insert(source_function.entry_block(), current_block); - let return_values = context.inline_blocks(ssa); + let return_values = context.inline_blocks(ssa, should_inline_call); self.recursion_level -= 1; return_values } @@ -476,13 +479,30 @@ impl<'function> PerFunctionContext<'function> { } let new_value = match &self.source_function.dfg[id] { - value @ Value::Instruction { .. } => { + value @ Value::Instruction { instruction, .. } => { + // TODO: Inlining the global into the function is only a temporary measure + // until Brillig gen with globals is working end to end + if self.source_function.dfg.is_global(id) { + let Instruction::MakeArray { elements, typ } = &self.globals.dfg[*instruction] + else { + panic!("Only expect Instruction::MakeArray for a global"); + }; + let elements = elements + .iter() + .map(|element| self.translate_value(*element)) + .collect::>(); + return self.context.builder.insert_make_array(elements, typ.clone()); + } unreachable!("All Value::Instructions should already be known during inlining after creating the original inlined instruction. Unknown value {id} = {value:?}") } value @ Value::Param { .. } => { unreachable!("All Value::Params should already be known from previous calls to translate_block. Unknown value {id} = {value:?}") } Value::NumericConstant { constant, typ } => { + // TODO: Inlining the global into the function is only a temporary measure + // until Brillig gen with globals is working end to end. + // The dfg indexes a global's inner value directly, so we will need to check here + // whether we have a global. self.context.builder.numeric_constant(*constant, *typ) } Value::Function(function) => self.context.builder.import_function(*function), @@ -491,26 +511,7 @@ impl<'function> PerFunctionContext<'function> { self.context.builder.import_foreign_function(function) } Value::Global(_) => { - // TODO: Inlining the global into the function is only a temporary measure - // until Brillig gen with globals is working end to end - match &self.globals.dfg[id] { - Value::Instruction { instruction, .. } => { - let Instruction::MakeArray { elements, typ } = - &self.globals.dfg[*instruction] - else { - panic!("Only expect Instruction::MakeArray for a global"); - }; - let elements = elements - .iter() - .map(|element| self.translate_value(*element)) - .collect::>(); - self.context.builder.insert_make_array(elements, typ.clone()) - } - Value::NumericConstant { constant, typ } => { - self.context.builder.numeric_constant(*constant, *typ) - } - _ => panic!("Expected only an instruction or a numeric constant"), - } + panic!("Expected a global to be resolved to its inner value"); } }; @@ -568,7 +569,11 @@ impl<'function> PerFunctionContext<'function> { } /// Inline all reachable blocks within the source_function into the destination function. - fn inline_blocks(&mut self, ssa: &Ssa) -> Vec { + fn inline_blocks( + &mut self, + ssa: &Ssa, + should_inline_call: &impl Fn(&PerFunctionContext, &Ssa, FunctionId) -> bool, + ) -> Vec { let mut seen_blocks = HashSet::new(); let mut block_queue = VecDeque::new(); block_queue.push_back(self.source_function.entry_block()); @@ -585,7 +590,7 @@ impl<'function> PerFunctionContext<'function> { self.context.builder.switch_to_block(translated_block_id); seen_blocks.insert(source_block_id); - self.inline_block_instructions(ssa, source_block_id); + self.inline_block_instructions(ssa, source_block_id, should_inline_call); if let Some((block, values)) = self.handle_terminator_instruction(source_block_id, &mut block_queue) @@ -630,7 +635,12 @@ impl<'function> PerFunctionContext<'function> { /// Inline each instruction in the given block into the function being inlined into. /// This may recurse if it finds another function to inline if a call instruction is within this block. - fn inline_block_instructions(&mut self, ssa: &Ssa, block_id: BasicBlockId) { + fn inline_block_instructions( + &mut self, + ssa: &Ssa, + block_id: BasicBlockId, + should_inline_call: &impl Fn(&PerFunctionContext, &Ssa, FunctionId) -> bool, + ) { let mut side_effects_enabled: Option = None; let block = &self.source_function.dfg[block_id]; @@ -638,8 +648,8 @@ impl<'function> PerFunctionContext<'function> { match &self.source_function.dfg[*id] { Instruction::Call { func, arguments } => match self.get_function(*func) { Some(func_id) => { - if self.should_inline_call(ssa, func_id) { - self.inline_function(ssa, *id, func_id, arguments); + if should_inline_call(self, ssa, func_id) { + self.inline_function(ssa, *id, func_id, arguments, should_inline_call); // This is only relevant during handling functions with `InlineType::NoPredicates` as these // can pollute the function they're being inlined into with `Instruction::EnabledSideEffects`, @@ -667,24 +677,6 @@ impl<'function> PerFunctionContext<'function> { } } - fn should_inline_call(&self, ssa: &Ssa, called_func_id: FunctionId) -> bool { - let function = &ssa.functions[&called_func_id]; - - if let RuntimeType::Acir(inline_type) = function.runtime() { - // If the called function is acir, we inline if it's not an entry point - - // If we have not already finished the flattening pass, functions marked - // to not have predicates should be preserved. - let preserve_function = - !self.context.inline_no_predicates_functions && function.is_no_predicates(); - !inline_type.is_entry_point() && !preserve_function - } else { - // If the called function is brillig, we inline only if it's into brillig and the function is not recursive - matches!(ssa.functions[&self.context.entry_point].runtime(), RuntimeType::Brillig(_)) - && !self.context.functions_not_to_inline.contains(&called_func_id) - } - } - /// Inline a function call and remember the inlined return values in the values map fn inline_function( &mut self, @@ -692,6 +684,7 @@ impl<'function> PerFunctionContext<'function> { call_id: InstructionId, function: FunctionId, arguments: &[ValueId], + should_inline_call: &impl Fn(&PerFunctionContext, &Ssa, FunctionId) -> bool, ) { let old_results = self.source_function.dfg.instruction_results(call_id); let arguments = vecmap(arguments, |arg| self.translate_value(*arg)); @@ -707,7 +700,8 @@ impl<'function> PerFunctionContext<'function> { .extend_call_stack(self.context.call_stack, &call_stack); self.context.call_stack = new_call_stack; - let new_results = self.context.inline_function(ssa, function, &arguments); + let new_results = + self.context.inline_function(ssa, function, &arguments, should_inline_call); self.context.call_stack = self .context .builder diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/make_constrain_not_equal.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/make_constrain_not_equal.rs new file mode 100644 index 00000000000..21f536eba2d --- /dev/null +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/make_constrain_not_equal.rs @@ -0,0 +1,72 @@ +use acvm::AcirField; + +use crate::ssa::{ + ir::{ + function::Function, + instruction::{Binary, BinaryOp, Instruction}, + value::Value, + }, + ssa_gen::Ssa, +}; + +impl Ssa { + /// A simple SSA pass to go through each [`Instruction::Constrain`], determine whether it's asserting + /// two values are not equal, and if so replace it with a [`Instruction::ConstrainNotEqual`]. + /// + /// Note that this pass must be placed after CFG flattening as the flattening pass cannot + /// handle this instruction. + #[tracing::instrument(level = "trace", skip(self))] + pub(crate) fn make_constrain_not_equal_instructions(mut self) -> Ssa { + for function in self.functions.values_mut() { + function.make_constrain_not_equal(); + } + self + } +} + +impl Function { + pub(crate) fn make_constrain_not_equal(&mut self) { + if !self.runtime().is_acir() { + return; + } + + for block in self.reachable_blocks() { + let instructions = self.dfg[block].instructions().to_vec(); + + for instruction in instructions { + let constrain_ne: Instruction = match &self.dfg[instruction] { + Instruction::Constrain(lhs, rhs, msg) => { + if self + .dfg + .get_numeric_constant(*rhs) + .map_or(false, |constant| constant.is_zero()) + { + if let Value::Instruction { instruction, .. } = + &self.dfg[self.dfg.resolve(*lhs)] + { + if let Instruction::Binary(Binary { + lhs, + rhs, + operator: BinaryOp::Eq, + .. + }) = self.dfg[*instruction] + { + Instruction::ConstrainNotEqual(lhs, rhs, msg.clone()) + } else { + continue; + } + } else { + continue; + } + } else { + continue; + } + } + _ => continue, + }; + + self.dfg[instruction] = constrain_ne; + } + } + } +} diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mod.rs index 1105e15c30e..f97d36f0844 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/mod.rs @@ -14,6 +14,7 @@ pub(crate) mod flatten_cfg; mod hint; mod inlining; mod loop_invariant; +mod make_constrain_not_equal; mod mem2reg; mod normalize_value_ids; mod rc; diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs index 5f21e3816f0..b248f6734a9 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/normalize_value_ids.rs @@ -3,7 +3,6 @@ use std::collections::BTreeMap; use crate::ssa::{ ir::{ basic_block::BasicBlockId, - dfg::DataFlowGraph, function::{Function, FunctionId}, map::SparseMap, post_order::PostOrder, @@ -26,7 +25,7 @@ impl Ssa { let mut context = Context::default(); context.populate_functions(&self.functions); for function in self.functions.values_mut() { - context.normalize_ids(function, &self.globals.dfg); + context.normalize_ids(function); } self.functions = context.functions.into_btree(); } @@ -66,14 +65,14 @@ impl Context { } } - fn normalize_ids(&mut self, old_function: &mut Function, globals: &DataFlowGraph) { + 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]; - for (_, value) in globals.values_iter() { + for (_, value) in old_function.dfg.globals.values_iter() { new_function.dfg.make_global(value.get_type().into_owned()); } @@ -171,6 +170,11 @@ impl IdMaps { old_value: ValueId, ) -> ValueId { let old_value = old_function.dfg.resolve(old_value); + if old_function.dfg.is_global(old_value) { + // Globals are computed at compile-time and thus are expected to be remain normalized + // between SSA passes + return old_value; + } match &old_function.dfg[old_value] { value @ Value::Instruction { instruction, .. } => { *self.values.get(&old_value).unwrap_or_else(|| { @@ -198,9 +202,7 @@ impl IdMaps { Value::Intrinsic(intrinsic) => new_function.dfg.import_intrinsic(*intrinsic), Value::ForeignFunction(name) => new_function.dfg.import_foreign_function(name), Value::Global(_) => { - // Globals are computed at compile-time and thus are expected to be remain normalized - // between SSA passes - old_value + unreachable!("Should have handled the global case already"); }, } } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs index ca9b75643bc..942fe67b5d5 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/remove_enable_side_effects.rs @@ -143,6 +143,7 @@ impl Context { | Not(_) | Truncate { .. } | Constrain(..) + | ConstrainNotEqual(..) | RangeCheck { .. } | IfElse { .. } | IncrementRc { .. } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs index 1e7a9b7e8b1..0b778ef9b78 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs @@ -20,6 +20,7 @@ use crate::ssa::ir::types::{NumericType, Type}; use crate::ssa::ir::value::ValueId; use super::value::{Tree, Value, Values}; +use super::GlobalsGraph; use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; /// The FunctionContext is the main context object for translating a @@ -109,6 +110,7 @@ impl<'a> FunctionContext<'a> { parameters: &Parameters, runtime: RuntimeType, shared_context: &'a SharedContext, + globals: GlobalsGraph, ) -> Self { let function_id = shared_context .pop_next_function_in_queue() @@ -117,10 +119,10 @@ impl<'a> FunctionContext<'a> { let mut builder = FunctionBuilder::new(function_name, function_id); builder.set_runtime(runtime); + builder.set_globals(Arc::new(globals)); let definitions = HashMap::default(); let mut this = Self { definitions, builder, shared_context, loops: Vec::new() }; - this.add_globals(); this.add_parameters_to_scope(parameters); this } @@ -132,23 +134,18 @@ impl<'a> FunctionContext<'a> { /// avoid calling new_function until the previous function is completely finished with ssa-gen. pub(super) fn new_function(&mut self, id: IrFunctionId, func: &ast::Function) { self.definitions.clear(); + + let globals = self.builder.current_function.dfg.globals.clone(); if func.unconstrained { self.builder.new_brillig_function(func.name.clone(), id, func.inline_type); } else { self.builder.new_function(func.name.clone(), id, func.inline_type); } - - self.add_globals(); + self.builder.set_globals(globals); self.add_parameters_to_scope(&func.parameters); } - fn add_globals(&mut self) { - for (_, value) in self.shared_context.globals_context.dfg.values_iter() { - self.builder.current_function.dfg.make_global(value.get_type().into_owned()); - } - } - /// Add each parameter to the current scope, and return the list of parameter types. /// /// The returned parameter type list will be flattened, so any struct parameters will @@ -1087,6 +1084,7 @@ impl SharedContext { &vec![], RuntimeType::Brillig(InlineType::default()), &globals_shared_context, + GlobalsGraph::default(), ); let mut globals = BTreeMap::default(); for (id, global) in program.globals.iter() { diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs index 529c207a4ad..d73a5946b4c 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs @@ -23,6 +23,7 @@ use self::{ value::{Tree, Values}, }; +use super::ir::dfg::GlobalsGraph; use super::ir::instruction::ErrorType; use super::ir::types::NumericType; use super::{ @@ -49,6 +50,8 @@ pub(crate) fn generate_ssa(program: Program) -> Result { let return_location = program.return_location; let context = SharedContext::new(program); + let globals = GlobalsGraph::from_dfg(context.globals_context.dfg.clone()); + let main_id = Program::main_id(); let main = context.program.main(); @@ -60,7 +63,7 @@ pub(crate) fn generate_ssa(program: Program) -> Result { RuntimeType::Acir(main.inline_type) }; let mut function_context = - FunctionContext::new(main.name.clone(), &main.parameters, main_runtime, &context); + FunctionContext::new(main.name.clone(), &main.parameters, main_runtime, &context, globals); // Generate the call_data bus from the relevant parameters. We create it *before* processing the function body let call_data = function_context.builder.call_data_bus(is_databus); diff --git a/noir/noir-repo/compiler/noirc_frontend/src/ast/statement.rs b/noir/noir-repo/compiler/noirc_frontend/src/ast/statement.rs index bd65fc33377..57572e80d1e 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/ast/statement.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/ast/statement.rs @@ -45,6 +45,7 @@ pub enum StatementKind { Expression(Expression), Assign(AssignStatement), For(ForLoopStatement), + Loop(Expression), Break, Continue, /// This statement should be executed at compile-time @@ -106,6 +107,9 @@ impl StatementKind { // A semicolon on a for loop is optional and does nothing StatementKind::For(_) => self, + // A semicolon on a loop is optional and does nothing + StatementKind::Loop(..) => self, + // No semicolon needed for a resolved statement StatementKind::Interned(_) => self, @@ -961,6 +965,7 @@ impl Display for StatementKind { StatementKind::Expression(expression) => expression.fmt(f), StatementKind::Assign(assign) => assign.fmt(f), StatementKind::For(for_loop) => for_loop.fmt(f), + StatementKind::Loop(block) => write!(f, "loop {}", block), StatementKind::Break => write!(f, "break"), StatementKind::Continue => write!(f, "continue"), StatementKind::Comptime(statement) => write!(f, "comptime {}", statement.kind), diff --git a/noir/noir-repo/compiler/noirc_frontend/src/ast/visitor.rs b/noir/noir-repo/compiler/noirc_frontend/src/ast/visitor.rs index 2f60532980a..ec50a982a70 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/ast/visitor.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/ast/visitor.rs @@ -296,6 +296,10 @@ pub trait Visitor { true } + fn visit_loop_statement(&mut self, _: &Expression) -> bool { + true + } + fn visit_comptime_statement(&mut self, _: &Statement) -> bool { true } @@ -1104,6 +1108,11 @@ impl Statement { StatementKind::For(for_loop_statement) => { for_loop_statement.accept(visitor); } + StatementKind::Loop(block) => { + if visitor.visit_loop_statement(block) { + block.accept(visitor); + } + } StatementKind::Comptime(statement) => { if visitor.visit_comptime_statement(statement) { statement.accept(visitor); diff --git a/noir/noir-repo/compiler/noirc_frontend/src/debug/mod.rs b/noir/noir-repo/compiler/noirc_frontend/src/debug/mod.rs index 3b415a7775b..808bbda677c 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/debug/mod.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/debug/mod.rs @@ -605,7 +605,6 @@ pub fn build_debug_crate_file() -> String { pub fn __debug_member_assign_{n}(var_id: u32, value: T, {var_sig}) {{ /// Safety: debug context unsafe {{ - //@safety: debug context __debug_inner_member_assign_{n}(var_id, value, {vars}); }} }} diff --git a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/mod.rs b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/mod.rs index 10e56364ed3..d3dded22ab4 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/mod.rs @@ -3,10 +3,6 @@ use std::{ rc::Rc, }; -use crate::{ - ast::ItemVisibility, graph::CrateGraph, hir_def::traits::ResolvedTraitBound, - node_interner::GlobalValue, usage_tracker::UsageTracker, StructField, StructType, TypeBindings, -}; use crate::{ ast::{ BlockExpression, FunctionKind, GenericTypeArgs, Ident, NoirFunction, NoirStruct, Param, @@ -41,6 +37,14 @@ use crate::{ token::SecondaryAttribute, Shared, Type, TypeVariable, }; +use crate::{ + ast::{ItemVisibility, UnresolvedType}, + graph::CrateGraph, + hir_def::traits::ResolvedTraitBound, + node_interner::GlobalValue, + usage_tracker::UsageTracker, + StructField, StructType, TypeBindings, +}; mod comptime; mod expressions; @@ -766,7 +770,9 @@ impl<'context> Elaborator<'context> { ) -> Vec { where_clause .iter_mut() - .flat_map(|constraint| self.add_missing_named_generics(&mut constraint.trait_bound)) + .flat_map(|constraint| { + self.add_missing_named_generics(&constraint.typ, &mut constraint.trait_bound) + }) .collect() } @@ -782,7 +788,11 @@ impl<'context> Elaborator<'context> { /// /// with a vector of `` returned so that the caller can then modify the function to: /// `fn foo() where T: Foo { ... }` - fn add_missing_named_generics(&mut self, bound: &mut TraitBound) -> Vec { + fn add_missing_named_generics( + &mut self, + object: &UnresolvedType, + bound: &mut TraitBound, + ) -> Vec { let mut added_generics = Vec::new(); let Ok(item) = self.resolve_path_or_error(bound.trait_path.clone()) else { @@ -796,6 +806,8 @@ impl<'context> Elaborator<'context> { let the_trait = self.get_trait_mut(trait_id); if the_trait.associated_types.len() > bound.trait_generics.named_args.len() { + let trait_name = the_trait.name.to_string(); + for associated_type in &the_trait.associated_types.clone() { if !bound .trait_generics @@ -806,10 +818,12 @@ impl<'context> Elaborator<'context> { // This generic isn't contained in the bound's named arguments, // so add it by creating a fresh type variable. let new_generic_id = self.interner.next_type_variable_id(); - let type_var = TypeVariable::unbound(new_generic_id, Kind::Normal); + let kind = associated_type.type_var.kind(); + let type_var = TypeVariable::unbound(new_generic_id, kind); let span = bound.trait_path.span; - let name = associated_type.name.clone(); + let name = format!("<{object} as {trait_name}>::{}", associated_type.name); + let name = Rc::new(name); let typ = Type::NamedGeneric(type_var.clone(), name.clone()); let typ = self.interner.push_quoted_type(typ); let typ = UnresolvedTypeData::Resolved(typ).with_span(span); diff --git a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/statements.rs b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/statements.rs index 8a46d85d563..24653772f9f 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/statements.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/statements.rs @@ -33,6 +33,7 @@ impl<'context> Elaborator<'context> { StatementKind::Constrain(constrain) => self.elaborate_constrain(constrain), StatementKind::Assign(assign) => self.elaborate_assign(assign), StatementKind::For(for_stmt) => self.elaborate_for(for_stmt), + StatementKind::Loop(block) => self.elaborate_loop(block, statement.span), StatementKind::Break => self.elaborate_jump(true, statement.span), StatementKind::Continue => self.elaborate_jump(false, statement.span), StatementKind::Comptime(statement) => self.elaborate_comptime_statement(*statement), @@ -268,6 +269,15 @@ impl<'context> Elaborator<'context> { (statement, Type::Unit) } + pub(super) fn elaborate_loop( + &mut self, + _block: Expression, + span: noirc_errors::Span, + ) -> (HirStatement, Type) { + self.push_err(ResolverError::LoopNotYetSupported { span }); + (HirStatement::Error, Type::Unit) + } + fn elaborate_jump(&mut self, is_break: bool, span: noirc_errors::Span) -> (HirStatement, Type) { let in_constrained_function = self.in_constrained_function(); diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/display.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/display.rs index 29d1448f07e..ccdfdf00e72 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/display.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/display.rs @@ -732,6 +732,9 @@ fn remove_interned_in_statement_kind( block: remove_interned_in_expression(interner, for_loop.block), ..for_loop }), + StatementKind::Loop(block) => { + StatementKind::Loop(remove_interned_in_expression(interner, block)) + } StatementKind::Comptime(statement) => { StatementKind::Comptime(Box::new(remove_interned_in_statement(interner, *statement))) } diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs index ae0da5ddf54..3506b63919c 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/interpreter/builtin.rs @@ -67,7 +67,6 @@ impl<'local, 'context> Interpreter<'local, 'context> { "array_len" => array_len(interner, arguments, location), "array_refcount" => Ok(Value::U32(0)), "assert_constant" => Ok(Value::Bool(true)), - "as_field" => as_field(interner, arguments, location), "as_slice" => as_slice(interner, arguments, location), "ctstring_eq" => ctstring_eq(arguments, location), "ctstring_hash" => ctstring_hash(arguments, location), @@ -120,7 +119,6 @@ impl<'local, 'context> Interpreter<'local, 'context> { "field_less_than" => field_less_than(arguments, location), "fmtstr_as_ctstring" => fmtstr_as_ctstring(interner, arguments, location), "fmtstr_quoted_contents" => fmtstr_quoted_contents(interner, arguments, location), - "from_field" => from_field(interner, arguments, return_type, location), "fresh_type_variable" => fresh_type_variable(interner), "function_def_add_attribute" => function_def_add_attribute(self, arguments, location), "function_def_body" => function_def_body(interner, arguments, location), @@ -187,7 +185,9 @@ impl<'local, 'context> Interpreter<'local, 'context> { "struct_def_fields_as_written" => { struct_def_fields_as_written(interner, arguments, location) } - "struct_def_generics" => struct_def_generics(interner, arguments, location), + "struct_def_generics" => { + struct_def_generics(interner, arguments, return_type, location) + } "struct_def_has_named_attribute" => { struct_def_has_named_attribute(interner, arguments, location) } @@ -297,16 +297,6 @@ fn array_as_str_unchecked( Ok(Value::String(Rc::new(string))) } -// fn as_field(x: T) -> Field {} -fn as_field( - interner: &NodeInterner, - arguments: Vec<(Value, Location)>, - location: Location, -) -> IResult { - let (value, value_location) = check_one_argument(arguments, location)?; - Interpreter::evaluate_cast_one_step(&Type::FieldElement, value_location, value, interner) -} - fn as_slice( interner: &NodeInterner, arguments: Vec<(Value, Location)>, @@ -457,10 +447,11 @@ fn struct_def_as_type( Ok(Value::Type(Type::Struct(struct_def_rc, generics))) } -/// fn generics(self) -> [Type] +/// fn generics(self) -> [(Type, Option)] fn struct_def_generics( interner: &NodeInterner, arguments: Vec<(Value, Location)>, + return_type: Type, location: Location, ) -> IResult { let argument = check_one_argument(arguments, location)?; @@ -468,11 +459,38 @@ fn struct_def_generics( let struct_def = interner.get_struct(struct_id); let struct_def = struct_def.borrow(); - let generics = - struct_def.generics.iter().map(|generic| Value::Type(generic.clone().as_named_generic())); + let expected = Type::Slice(Box::new(Type::Tuple(vec![ + Type::Quoted(QuotedType::Type), + interner.next_type_variable(), // Option + ]))); + + let actual = return_type.clone(); - let typ = Type::Slice(Box::new(Type::Quoted(QuotedType::Type))); - Ok(Value::Slice(generics.collect(), typ)) + let slice_item_type = match return_type { + Type::Slice(item_type) => *item_type, + _ => return Err(InterpreterError::TypeMismatch { expected, actual, location }), + }; + + let option_typ = match &slice_item_type { + Type::Tuple(types) if types.len() == 2 => types[1].clone(), + _ => return Err(InterpreterError::TypeMismatch { expected, actual, location }), + }; + + let generics: IResult<_> = struct_def + .generics + .iter() + .map(|generic| -> IResult { + let generic_as_named = generic.clone().as_named_generic(); + let numeric_type = match generic_as_named.kind() { + Kind::Numeric(numeric_type) => Some(Value::Type(*numeric_type)), + _ => None, + }; + let numeric_type = option(option_typ.clone(), numeric_type, location.span)?; + Ok(Value::Tuple(vec![Value::Type(generic_as_named), numeric_type])) + }) + .collect(); + + Ok(Value::Slice(generics?, slice_item_type)) } fn struct_def_hash(arguments: Vec<(Value, Location)>, location: Location) -> IResult { @@ -2320,17 +2338,6 @@ fn fmtstr_quoted_contents( Ok(Value::Quoted(Rc::new(tokens))) } -// fn from_field(x: Field) -> T {} -fn from_field( - interner: &NodeInterner, - arguments: Vec<(Value, Location)>, - return_type: Type, - location: Location, -) -> IResult { - let (value, value_location) = check_one_argument(arguments, location)?; - Interpreter::evaluate_cast_one_step(&return_type, value_location, value, interner) -} - // fn fresh_type_variable() -> Type fn fresh_type_variable(interner: &NodeInterner) -> IResult { Ok(Value::Type(interner.next_type_variable_with_kind(Kind::Any))) diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index 9081cb1cccd..7f6509b9f16 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -3,7 +3,7 @@ use super::errors::{DefCollectorErrorKind, DuplicateType}; use crate::elaborator::Elaborator; use crate::graph::CrateId; use crate::hir::comptime::InterpreterError; -use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleId}; +use crate::hir::def_map::{CrateDefMap, LocalModuleId, ModuleDefId, ModuleId}; use crate::hir::resolution::errors::ResolverError; use crate::hir::type_check::TypeCheckError; use crate::locations::ReferencesTracker; @@ -412,13 +412,24 @@ impl DefCollector { visibility, ); - if visibility != ItemVisibility::Private { + if context.def_interner.is_in_lsp_mode() + && visibility != ItemVisibility::Private + { context.def_interner.register_name_for_auto_import( name.to_string(), module_def_id, visibility, Some(defining_module), ); + + if let ModuleDefId::TraitId(trait_id) = module_def_id { + context.def_interner.add_trait_reexport( + trait_id, + defining_module, + name.clone(), + visibility, + ); + } } } diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/errors.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/errors.rs index a4b3c1b9c07..e0e09d53311 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/errors.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/errors.rs @@ -170,6 +170,8 @@ pub enum ResolverError { span: Span, missing_trait_location: Location, }, + #[error("`loop` statements are not yet implemented")] + LoopNotYetSupported { span: Span }, } impl ResolverError { @@ -642,6 +644,13 @@ impl<'a> From<&'a ResolverError> for Diagnostic { diagnostic.add_secondary_with_file(format!("required by this bound in `{impl_trait}"), missing_trait_location.span, missing_trait_location.file); diagnostic }, + ResolverError::LoopNotYetSupported { span } => { + Diagnostic::simple_error( + "`loop` statements are not yet implemented".to_string(), + String::new(), + *span) + + } } } } diff --git a/noir/noir-repo/compiler/noirc_frontend/src/lexer/token.rs b/noir/noir-repo/compiler/noirc_frontend/src/lexer/token.rs index 7f0c87d0794..8c136f5e45d 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/lexer/token.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/lexer/token.rs @@ -345,7 +345,6 @@ impl Display for FmtStrFragment { pub enum DocStyle { Outer, Inner, - Safety, } #[derive(Debug, Clone, PartialEq, Eq, Hash, PartialOrd, Ord)] @@ -425,13 +424,11 @@ impl fmt::Display for Token { Token::LineComment(ref s, style) => match style { Some(DocStyle::Inner) => write!(f, "//!{s}"), Some(DocStyle::Outer) => write!(f, "///{s}"), - Some(DocStyle::Safety) => write!(f, "//@safety{s}"), None => write!(f, "//{s}"), }, Token::BlockComment(ref s, style) => match style { Some(DocStyle::Inner) => write!(f, "/*!{s}*/"), Some(DocStyle::Outer) => write!(f, "/**{s}*/"), - Some(DocStyle::Safety) => write!(f, "/*@safety{s}*/"), None => write!(f, "/*{s}*/"), }, Token::Quote(ref stream) => { @@ -1034,6 +1031,7 @@ pub enum Keyword { Impl, In, Let, + Loop, Match, Mod, Module, @@ -1093,6 +1091,7 @@ impl fmt::Display for Keyword { Keyword::Impl => write!(f, "impl"), Keyword::In => write!(f, "in"), Keyword::Let => write!(f, "let"), + Keyword::Loop => write!(f, "loop"), Keyword::Match => write!(f, "match"), Keyword::Mod => write!(f, "mod"), Keyword::Module => write!(f, "Module"), @@ -1155,6 +1154,7 @@ impl Keyword { "impl" => Keyword::Impl, "in" => Keyword::In, "let" => Keyword::Let, + "loop" => Keyword::Loop, "match" => Keyword::Match, "mod" => Keyword::Mod, "Module" => Keyword::Module, diff --git a/noir/noir-repo/compiler/noirc_frontend/src/node_interner.rs b/noir/noir-repo/compiler/noirc_frontend/src/node_interner.rs index 0ec033a399b..ae2cf224cbd 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/node_interner.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/node_interner.rs @@ -267,6 +267,11 @@ pub struct NodeInterner { /// Captures the documentation comments for each module, struct, trait, function, etc. pub(crate) doc_comments: HashMap>, + + /// Only for LSP: a map of trait ID to each module that pub or pub(crate) exports it. + /// In LSP this is used to offer importing the trait via one of these exports if + /// the trait is not visible where it's defined. + trait_reexports: HashMap>, } /// A dependency in the dependency graph may be a type or a definition. @@ -680,6 +685,7 @@ impl Default for NodeInterner { comptime_scopes: vec![HashMap::default()], trait_impl_associated_types: HashMap::default(), doc_comments: HashMap::default(), + trait_reexports: HashMap::default(), } } } @@ -2256,6 +2262,20 @@ impl NodeInterner { _ => None, } } + + pub fn add_trait_reexport( + &mut self, + trait_id: TraitId, + module_id: ModuleId, + name: Ident, + visibility: ItemVisibility, + ) { + self.trait_reexports.entry(trait_id).or_default().push((module_id, name, visibility)); + } + + pub fn get_trait_reexports(&self, trait_id: TraitId) -> &[(ModuleId, Ident, ItemVisibility)] { + self.trait_reexports.get(&trait_id).map_or(&[], |exports| exports) + } } impl Methods { diff --git a/noir/noir-repo/compiler/noirc_frontend/src/parser/parser/statement.rs b/noir/noir-repo/compiler/noirc_frontend/src/parser/parser/statement.rs index 315f3b9f958..465e48e3bad 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/parser/parser/statement.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/parser/parser/statement.rs @@ -92,6 +92,7 @@ impl<'a> Parser<'a> { /// | ConstrainStatement /// | ComptimeStatement /// | ForStatement + /// | LoopStatement /// | IfStatement /// | BlockStatement /// | AssignStatement @@ -156,6 +157,10 @@ impl<'a> Parser<'a> { return Some(StatementKind::For(for_loop)); } + if let Some(block) = self.parse_loop() { + return Some(StatementKind::Loop(block)); + } + if let Some(kind) = self.parse_if_expr() { return Some(StatementKind::Expression(Expression { kind, @@ -287,6 +292,26 @@ impl<'a> Parser<'a> { Some(ForLoopStatement { identifier, range, block, span: self.span_since(start_span) }) } + /// LoopStatement = 'loop' Block + fn parse_loop(&mut self) -> Option { + if !self.eat_keyword(Keyword::Loop) { + return None; + } + + let block_start_span = self.current_token_span; + let block = if let Some(block) = self.parse_block() { + Expression { + kind: ExpressionKind::Block(block), + span: self.span_since(block_start_span), + } + } else { + self.expected_token(Token::LeftBrace); + Expression { kind: ExpressionKind::Error, span: self.span_since(block_start_span) } + }; + + Some(block) + } + /// ForRange /// = ExpressionExceptConstructor /// | ExpressionExceptConstructor '..' ExpressionExceptConstructor @@ -790,4 +815,30 @@ mod tests { assert!(statement.is_none()); assert_eq!(parser.errors.len(), 2); } + + #[test] + fn parses_empty_loop() { + let src = "loop { }"; + let statement = parse_statement_no_errors(src); + let StatementKind::Loop(block) = statement.kind else { + panic!("Expected loop"); + }; + let ExpressionKind::Block(block) = block.kind else { + panic!("Expected block"); + }; + assert!(block.statements.is_empty()); + } + + #[test] + fn parses_loop_with_statements() { + let src = "loop { 1; 2 }"; + let statement = parse_statement_no_errors(src); + let StatementKind::Loop(block) = statement.kind else { + panic!("Expected loop"); + }; + let ExpressionKind::Block(block) = block.kind else { + panic!("Expected block"); + }; + assert_eq!(block.statements.len(), 2); + } } diff --git a/noir/noir-repo/compiler/noirc_frontend/src/tests/references.rs b/noir/noir-repo/compiler/noirc_frontend/src/tests/references.rs index d8ba6a13f28..956b2c5f05e 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/tests/references.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/tests/references.rs @@ -89,7 +89,6 @@ fn constrained_reference_to_unconstrained() { if x == 5 { /// Safety: test context unsafe { - //@safety: test context mut_ref_input(x_ref, y); } } diff --git a/noir/noir-repo/compiler/noirc_frontend/src/tests/traits.rs b/noir/noir-repo/compiler/noirc_frontend/src/tests/traits.rs index 04eb9cda7ab..5e42d8901fe 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/tests/traits.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/tests/traits.rs @@ -1236,3 +1236,75 @@ fn warns_if_trait_is_not_in_scope_for_generic_function_call_and_there_is_only_on assert_eq!(ident.to_string(), "foo"); assert_eq!(trait_name, "private_mod::Foo"); } + +// See https://github.com/noir-lang/noir/issues/6530 +#[test] +fn regression_6530() { + let src = r#" + pub trait From { + fn from(input: T) -> Self; + } + + pub trait Into { + fn into(self) -> T; + } + + impl Into for U + where + T: From, + { + fn into(self) -> T { + T::from(self) + } + } + + struct Foo { + inner: Field, + } + + impl Into for Foo { + fn into(self) -> Field { + self.inner + } + } + + fn main() { + let foo = Foo { inner: 0 }; + + // This works: + let _: Field = Into::::into(foo); + + // This was failing with 'No matching impl': + let _: Field = foo.into(); + } + "#; + let errors = get_program_errors(src); + assert_eq!(errors.len(), 0); +} + +// See https://github.com/noir-lang/noir/issues/7090 +#[test] +#[should_panic] +fn calls_trait_method_using_struct_name_when_multiple_impls_exist() { + let src = r#" + trait From2 { + fn from2(input: T) -> Self; + } + struct U60Repr {} + impl From2<[Field; 3]> for U60Repr { + fn from2(_: [Field; 3]) -> Self { + U60Repr {} + } + } + impl From2 for U60Repr { + fn from2(_: Field) -> Self { + U60Repr {} + } + } + fn main() { + let _ = U60Repr::from2([1, 2, 3]); + let _ = U60Repr::from2(1); + } + "#; + assert_no_errors(src); +} diff --git a/noir/noir-repo/docs/docs/noir/standard_library/meta/struct_def.md b/noir/noir-repo/docs/docs/noir/standard_library/meta/struct_def.md index 8c6e50ba23a..18e722af31e 100644 --- a/noir/noir-repo/docs/docs/noir/standard_library/meta/struct_def.md +++ b/noir/noir-repo/docs/docs/noir/standard_library/meta/struct_def.md @@ -42,23 +42,28 @@ any generics, the generics are also included as-is. #include_code generics noir_stdlib/src/meta/struct_def.nr rust -Returns each generic on this struct. +Returns each generic on this struct. Each generic is represented as a tuple containing the type, +and an optional containing the numeric type if it's a numeric generic. Example: ``` #[example] -struct Foo { - bar: [T; 2], +struct Foo { + bar: [T; K], baz: Baz, } comptime fn example(foo: StructDefinition) { - assert_eq(foo.generics().len(), 2); + assert_eq(foo.generics().len(), 3); // Fails because `T` isn't in scope // let t = quote { T }.as_type(); - // assert_eq(foo.generics()[0], t); + // assert_eq(foo.generics()[0].0, t); + assert(foo.generics()[0].1.is_none()); + + // Last generic is numeric, so we have the numeric type available to us + assert(foo.generics()[2].1.is_some()); } ``` diff --git a/noir/noir-repo/docs/docs/noir/standard_library/meta/trait_impl.md b/noir/noir-repo/docs/docs/noir/standard_library/meta/trait_impl.md index 659c6aad719..9134bf548b0 100644 --- a/noir/noir-repo/docs/docs/noir/standard_library/meta/trait_impl.md +++ b/noir/noir-repo/docs/docs/noir/standard_library/meta/trait_impl.md @@ -25,8 +25,10 @@ comptime { let generics = my_impl.trait_generic_args(); assert_eq(generics.len(), 2); - assert_eq(generics[0], quote { i32 }.as_type()); - assert_eq(generics[1], quote { Field }.as_type()); + assert_eq(generics[0].0, quote { i32 }.as_type()); + assert(generics[0].1.is_none()); + assert_eq(generics[1].0, quote { Field }.as_type()); + assert(generics[1].1.is_none()); } ``` diff --git a/noir/noir-repo/noir_stdlib/src/meta/mod.nr b/noir/noir-repo/noir_stdlib/src/meta/mod.nr index 21c1b14cc96..7644d5e1dd1 100644 --- a/noir/noir-repo/noir_stdlib/src/meta/mod.nr +++ b/noir/noir-repo/noir_stdlib/src/meta/mod.nr @@ -97,8 +97,19 @@ pub comptime fn make_trait_impl( ) -> Quoted { // docs:end:make_trait_impl let typ = s.as_type(); - let impl_generics = s.generics().map(|g| quote { $g }).join(quote {,}); - let where_clause = s.generics().map(|name| quote { $name: $trait_name }).join(quote {,}); + + let mut impl_generics = &[]; + let mut where_clause = &[]; + for g in s.generics() { + let (typ, numeric_type) = g; + impl_generics = impl_generics.push_back(quote { $typ }); + if numeric_type.is_none() { + where_clause = where_clause.push_back(quote { $typ: $trait_name }); + } + } + + let impl_generics = impl_generics.join(quote {, }); + let where_clause = where_clause.join(quote {, }); // `for_each_field(field1) $join_fields_with for_each_field(field2) $join_fields_with ...` let fields = s.fields_as_written().map(|f: (Quoted, Type)| { diff --git a/noir/noir-repo/noir_stdlib/src/meta/struct_def.nr b/noir/noir-repo/noir_stdlib/src/meta/struct_def.nr index 0e64a537f1f..d561e326fbd 100644 --- a/noir/noir-repo/noir_stdlib/src/meta/struct_def.nr +++ b/noir/noir-repo/noir_stdlib/src/meta/struct_def.nr @@ -1,3 +1,5 @@ +use crate::option::Option; + impl StructDefinition { #[builtin(struct_def_add_attribute)] // docs:start:add_attribute @@ -21,10 +23,11 @@ impl StructDefinition { pub comptime fn has_named_attribute(self, name: str) -> bool {} // docs:end:has_named_attribute - /// Return each generic on this struct. + /// Return (type, option) pairs of each generic in this struct definition. + /// If a generic is numeric, the second element of the pair will contain the numeric type. #[builtin(struct_def_generics)] // docs:start:generics - pub comptime fn generics(self) -> [Type] {} + pub comptime fn generics(self) -> [(Type, Option)] {} // docs:end:generics /// Returns (name, type) pairs of each field in this struct. diff --git a/noir/noir-repo/test_programs/compile_success_empty/comptime_struct_definition/src/main.nr b/noir/noir-repo/test_programs/compile_success_empty/comptime_struct_definition/src/main.nr index 0b71d44f016..41ba76171a8 100644 --- a/noir/noir-repo/test_programs/compile_success_empty/comptime_struct_definition/src/main.nr +++ b/noir/noir-repo/test_programs/compile_success_empty/comptime_struct_definition/src/main.nr @@ -39,7 +39,9 @@ mod foo { let generics = s.generics(); assert_eq(generics.len(), 1); - assert_eq(generics[0], new_generic); + let (typ, numeric) = generics[0]; + assert_eq(typ, new_generic); + assert(numeric.is_none()); } // docs:end:add-generic-example } diff --git a/noir/noir-repo/test_programs/compile_success_empty/eq_derivation_with_numeric_generics/Nargo.toml b/noir/noir-repo/test_programs/compile_success_empty/eq_derivation_with_numeric_generics/Nargo.toml new file mode 100644 index 00000000000..14b4d5eff4a --- /dev/null +++ b/noir/noir-repo/test_programs/compile_success_empty/eq_derivation_with_numeric_generics/Nargo.toml @@ -0,0 +1,6 @@ +[package] +name = "eq_derivation_with_numeric_generics" +type = "bin" +authors = [""] + +[dependencies] diff --git a/noir/noir-repo/test_programs/compile_success_empty/eq_derivation_with_numeric_generics/src/main.nr b/noir/noir-repo/test_programs/compile_success_empty/eq_derivation_with_numeric_generics/src/main.nr new file mode 100644 index 00000000000..3307aeb15ad --- /dev/null +++ b/noir/noir-repo/test_programs/compile_success_empty/eq_derivation_with_numeric_generics/src/main.nr @@ -0,0 +1,12 @@ +#[derive(Eq)] +struct Foo { + a: [Field; T], + b: u32, +} + +fn main() { + let foo = Foo { a: [0; 10], b: 27 }; + let bar = Foo { a: [0; 10], b: 28 }; + assert(foo != bar); +} + diff --git a/noir/noir-repo/test_programs/compile_success_empty/macros_in_comptime/src/main.nr b/noir/noir-repo/test_programs/compile_success_empty/macros_in_comptime/src/main.nr index 0572192225c..112ed16c22a 100644 --- a/noir/noir-repo/test_programs/compile_success_empty/macros_in_comptime/src/main.nr +++ b/noir/noir-repo/test_programs/compile_success_empty/macros_in_comptime/src/main.nr @@ -33,8 +33,8 @@ comptime fn foo(x: Field) { break; } - let loop = quote { for _ in 0..0 { break; } }; - unquote!(loop); + let loop_ = quote { for _ in 0..0 { break; } }; + unquote!(loop_); } mod submodule { diff --git a/noir/noir-repo/test_programs/compile_success_empty/serialize/src/main.nr b/noir/noir-repo/test_programs/compile_success_empty/serialize/src/main.nr index 66c79f9fc9d..a11fdf570d0 100644 --- a/noir/noir-repo/test_programs/compile_success_empty/serialize/src/main.nr +++ b/noir/noir-repo/test_programs/compile_success_empty/serialize/src/main.nr @@ -5,13 +5,12 @@ trait Serialize { fn serialize(self) -> [Field; Self::Size]; } -impl Serialize for (A, B) +impl Serialize for (A, B) where - A: Serialize, - B: Serialize, + A: Serialize, + B: Serialize, { - // let Size = ::Size + ::Size; - let Size: u32 = AS + BS; + let Size = ::Size + ::Size; fn serialize(self: Self) -> [Field; Self::Size] { let mut array: [Field; Self::Size] = std::mem::zeroed(); @@ -28,12 +27,11 @@ where } } -impl Serialize for [T; N] +impl Serialize for [T; N] where - T: Serialize, + T: Serialize, { - // let Size = ::Size * N; - let Size: u32 = TS * N; + let Size = ::Size * N; fn serialize(self: Self) -> [Field; Self::Size] { let mut array: [Field; Self::Size] = std::mem::zeroed(); diff --git a/noir/noir-repo/test_programs/compile_success_no_bug/regression_7062/Nargo.toml b/noir/noir-repo/test_programs/compile_success_no_bug/regression_7062/Nargo.toml new file mode 100644 index 00000000000..0e11219ad98 --- /dev/null +++ b/noir/noir-repo/test_programs/compile_success_no_bug/regression_7062/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "regression_7062" +type = "bin" +authors = [""] +compiler_version = ">=0.31.0" + +[dependencies] diff --git a/noir/noir-repo/test_programs/compile_success_no_bug/regression_7062/src/main.nr b/noir/noir-repo/test_programs/compile_success_no_bug/regression_7062/src/main.nr new file mode 100644 index 00000000000..c640062b45b --- /dev/null +++ b/noir/noir-repo/test_programs/compile_success_no_bug/regression_7062/src/main.nr @@ -0,0 +1,8 @@ +fn main(args: [Field; 2]) { + /// Safety: n/a + unsafe { store(args) }; + // Dummy test to remove the 'underconstraint bug' + assert(args[0] + args[1] != 0); +} + +pub unconstrained fn store(_: [Field]) {} \ No newline at end of file diff --git a/noir/noir-repo/test_programs/execution_success/hint_black_box/src/main.nr b/noir/noir-repo/test_programs/execution_success/hint_black_box/src/main.nr index abceadb07ff..ed2dc2d3760 100644 --- a/noir/noir-repo/test_programs/execution_success/hint_black_box/src/main.nr +++ b/noir/noir-repo/test_programs/execution_success/hint_black_box/src/main.nr @@ -2,12 +2,12 @@ use std::hint::black_box; fn main(a: u32, b: u32) { // This version unrolls into a number of additions - assert_eq(loop(5, a), b); + assert_eq(loop_(5, a), b); // This version simplifies into a single `constraint 50 == b` - assert_eq(loop(5, 10), b); + assert_eq(loop_(5, 10), b); // This version should not simplify down to a single constraint, // it should treat 10 as opaque: - assert_eq(loop(5, black_box(10)), b); + assert_eq(loop_(5, black_box(10)), b); // Check array handling. let arr = [a, a, a, a, a]; @@ -54,7 +54,7 @@ fn main(a: u32, b: u32) { //assert_eq(c, b); } -fn loop(n: u32, k: u32) -> u32 { +fn loop_(n: u32, k: u32) -> u32 { let mut sum = 0; for _ in 0..n { sum = sum + k; diff --git a/noir/noir-repo/test_programs/execution_success/loop/src/main.nr b/noir/noir-repo/test_programs/execution_success/loop/src/main.nr index b3be4c4c3ff..3e1f24742ec 100644 --- a/noir/noir-repo/test_programs/execution_success/loop/src/main.nr +++ b/noir/noir-repo/test_programs/execution_success/loop/src/main.nr @@ -2,12 +2,12 @@ // // The features being tested is basic looping. fn main(six_as_u32: u32) { - assert_eq(loop(4), six_as_u32); + assert_eq(loop_excl(4), six_as_u32); assert_eq(loop_incl(3), six_as_u32); assert(plain_loop() == six_as_u32); } -fn loop(x: u32) -> u32 { +fn loop_excl(x: u32) -> u32 { let mut sum = 0; for i in 0..x { sum = sum + i; diff --git a/noir/noir-repo/test_programs/execution_success/loop_invariant_regression/src/main.nr b/noir/noir-repo/test_programs/execution_success/loop_invariant_regression/src/main.nr index c28ce063116..61f8b1bedba 100644 --- a/noir/noir-repo/test_programs/execution_success/loop_invariant_regression/src/main.nr +++ b/noir/noir-repo/test_programs/execution_success/loop_invariant_regression/src/main.nr @@ -1,11 +1,11 @@ // Tests a simple loop where we expect loop invariant instructions // to be hoisted to the loop's pre-header block. fn main(x: u32, y: u32) { - loop(4, x, y); + loop_(4, x, y); array_read_loop(4, x); } -fn loop(upper_bound: u32, x: u32, y: u32) { +fn loop_(upper_bound: u32, x: u32, y: u32) { for _ in 0..upper_bound { let mut z = x * y; z = z * x; diff --git a/noir/noir-repo/tooling/lsp/src/requests/code_action/import_trait.rs b/noir/noir-repo/tooling/lsp/src/requests/code_action/import_trait.rs index 1e731aa563b..dd500c334ab 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/code_action/import_trait.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/code_action/import_trait.rs @@ -1,16 +1,19 @@ +use std::collections::HashSet; + use noirc_errors::Location; use noirc_frontend::{ ast::MethodCallExpression, - hir::{def_map::ModuleDefId, resolution::visibility::trait_member_is_visible}, - hir_def::traits::Trait, - node_interner::ReferenceId, + hir::def_map::ModuleDefId, + node_interner::{ReferenceId, TraitId}, }; use crate::{ - modules::relative_module_full_path, + modules::{relative_module_full_path, relative_module_id_path}, + requests::TraitReexport, use_segment_positions::{ use_completion_item_additional_text_edits, UseCompletionItemAdditionTextEditsRequest, }, + visibility::module_def_id_is_visible, }; use super::CodeActionFinder; @@ -29,16 +32,7 @@ impl<'a> CodeActionFinder<'a> { let trait_impl = self.interner.get_trait_implementation(trait_impl_id); let trait_id = trait_impl.borrow().trait_id; - let trait_ = self.interner.get_trait(trait_id); - - // Check if the trait is currently imported. If so, no need to suggest anything - let module_data = - &self.def_maps[&self.module_id.krate].modules()[self.module_id.local_id.0]; - if !module_data.scope().find_name(&trait_.name).is_none() { - return; - } - - self.push_import_trait_code_action(trait_); + self.import_trait(trait_id); return; } @@ -48,33 +42,86 @@ impl<'a> CodeActionFinder<'a> { return; }; - for (func_id, trait_id) in - self.interner.lookup_trait_methods(&typ, &method_call.method_name.0.contents, true) - { - let visibility = self.interner.function_modifiers(&func_id).visibility; - if !trait_member_is_visible(trait_id, visibility, self.module_id, self.def_maps) { - continue; - } + let trait_methods = + self.interner.lookup_trait_methods(&typ, &method_call.method_name.0.contents, true); + let trait_ids: HashSet<_> = trait_methods.iter().map(|(_, trait_id)| *trait_id).collect(); - let trait_ = self.interner.get_trait(trait_id); - self.push_import_trait_code_action(trait_); + for trait_id in trait_ids { + self.import_trait(trait_id); } } - fn push_import_trait_code_action(&mut self, trait_: &Trait) { - let trait_id = trait_.id; - + fn import_trait(&mut self, trait_id: TraitId) { + // First check if the trait is visible + let trait_ = self.interner.get_trait(trait_id); + let visibility = trait_.visibility; let module_def_id = ModuleDefId::TraitId(trait_id); - let current_module_parent_id = self.module_id.parent(self.def_maps); - let Some(module_full_path) = relative_module_full_path( + let mut trait_reexport = None; + + if !module_def_id_is_visible( module_def_id, self.module_id, - current_module_parent_id, + visibility, + None, self.interner, - ) else { + self.def_maps, + ) { + // If it's not, try to find a visible reexport of the trait + // that is visible from the current module + let Some((visible_module_id, name, _)) = + self.interner.get_trait_reexports(trait_id).iter().find( + |(module_id, _, visibility)| { + module_def_id_is_visible( + module_def_id, + self.module_id, + *visibility, + Some(*module_id), + self.interner, + self.def_maps, + ) + }, + ) + else { + return; + }; + trait_reexport = Some(TraitReexport { module_id: visible_module_id, name }); + } + + let trait_name = if let Some(trait_reexport) = &trait_reexport { + trait_reexport.name + } else { + &trait_.name + }; + + // Check if the trait is currently imported. If yes, no need to suggest anything + let module_data = + &self.def_maps[&self.module_id.krate].modules()[self.module_id.local_id.0]; + if !module_data.scope().find_name(trait_name).is_none() { return; + } + + let module_def_id = ModuleDefId::TraitId(trait_id); + let current_module_parent_id = self.module_id.parent(self.def_maps); + let module_full_path = if let Some(trait_reexport) = &trait_reexport { + relative_module_id_path( + *trait_reexport.module_id, + &self.module_id, + current_module_parent_id, + self.interner, + ) + } else { + let Some(path) = relative_module_full_path( + module_def_id, + self.module_id, + current_module_parent_id, + self.interner, + ) else { + return; + }; + path }; - let full_path = format!("{}::{}", module_full_path, trait_.name); + + let full_path = format!("{}::{}", module_full_path, trait_name); let title = format!("Import {}", full_path); @@ -242,6 +289,118 @@ mod moo { } } +fn main() { + let x: Field = 1; + x.foobar(); +}"#; + + assert_code_action(title, src, expected).await; + } + + #[test] + async fn test_import_trait_in_method_call_when_one_option_but_not_in_scope_via_reexport() { + let title = "Import moo::Bar"; + + let src = r#"mod moo { + mod nested { + pub trait Foo { + fn foobar(self); + } + + impl Foo for Field { + fn foobar(self) {} + } + } + + pub use nested::Foo as Bar; +} + +fn main() { + let x: Field = 1; + x.foo>|| NodeFinder<'a> { } } + let mut trait_reexport = None; + if let Some(trait_id) = trait_id { let modifiers = self.interner.function_modifiers(&func_id); let visibility = modifiers.visibility; - if !trait_member_is_visible(trait_id, visibility, self.module_id, self.def_maps) - { - continue; + let module_def_id = ModuleDefId::TraitId(trait_id); + if !module_def_id_is_visible( + module_def_id, + self.module_id, + visibility, + None, // defining module + self.interner, + self.def_maps, + ) { + // Try to find a visible reexport of the trait + // that is visible from the current module + let Some((visible_module_id, name, _)) = + self.interner.get_trait_reexports(trait_id).iter().find( + |(module_id, _, visibility)| { + module_def_id_is_visible( + module_def_id, + self.module_id, + *visibility, + Some(*module_id), + self.interner, + self.def_maps, + ) + }, + ) + else { + continue; + }; + + trait_reexport = Some(TraitReexport { module_id: visible_module_id, name }); } } @@ -706,7 +734,7 @@ impl<'a> NodeFinder<'a> { function_completion_kind, function_kind, None, // attribute first type - trait_id, + trait_id.map(|id| (id, trait_reexport.as_ref())), self_prefix, ); if !completion_items.is_empty() { diff --git a/noir/noir-repo/tooling/lsp/src/requests/completion/builtins.rs b/noir/noir-repo/tooling/lsp/src/requests/completion/builtins.rs index c0910e9005e..90b8c6301b7 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/completion/builtins.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/completion/builtins.rs @@ -191,6 +191,7 @@ pub(super) fn keyword_builtin_type(keyword: &Keyword) -> Option<&'static str> { | Keyword::Impl | Keyword::In | Keyword::Let + | Keyword::Loop | Keyword::Match | Keyword::Mod | Keyword::Mut @@ -257,6 +258,7 @@ pub(super) fn keyword_builtin_function(keyword: &Keyword) -> Option NodeFinder<'a> { @@ -160,7 +160,7 @@ impl<'a> NodeFinder<'a> { function_completion_kind: FunctionCompletionKind, function_kind: FunctionKind, attribute_first_type: Option<&Type>, - trait_id: Option, + trait_info: Option<(TraitId, Option<&TraitReexport>)>, self_prefix: bool, ) -> Vec { let func_meta = self.interner.function_meta(&func_id); @@ -233,7 +233,7 @@ impl<'a> NodeFinder<'a> { function_completion_kind, function_kind, attribute_first_type, - trait_id, + trait_info, self_prefix, is_macro_call, ) @@ -276,7 +276,7 @@ impl<'a> NodeFinder<'a> { function_completion_kind: FunctionCompletionKind, function_kind: FunctionKind, attribute_first_type: Option<&Type>, - trait_id: Option, + trait_info: Option<(TraitId, Option<&TraitReexport>)>, self_prefix: bool, is_macro_call: bool, ) -> CompletionItem { @@ -348,7 +348,7 @@ impl<'a> NodeFinder<'a> { } }; - self.auto_import_trait_if_trait_method(func_id, trait_id, &mut completion_item); + self.auto_import_trait_if_trait_method(func_id, trait_info, &mut completion_item); self.completion_item_with_doc_comments(ReferenceId::Function(func_id), completion_item) } @@ -356,25 +356,43 @@ impl<'a> NodeFinder<'a> { fn auto_import_trait_if_trait_method( &self, func_id: FuncId, - trait_id: Option, + trait_info: Option<(TraitId, Option<&TraitReexport>)>, completion_item: &mut CompletionItem, ) -> Option<()> { // If this is a trait method, check if the trait is in scope - let trait_ = self.interner.get_trait(trait_id?); + let (trait_id, trait_reexport) = trait_info?; + + let trait_name = if let Some(trait_reexport) = trait_reexport { + trait_reexport.name + } else { + let trait_ = self.interner.get_trait(trait_id); + &trait_.name + }; + let module_data = &self.def_maps[&self.module_id.krate].modules()[self.module_id.local_id.0]; - if !module_data.scope().find_name(&trait_.name).is_none() { + if !module_data.scope().find_name(trait_name).is_none() { return None; } + // If not, automatically import it let current_module_parent_id = self.module_id.parent(self.def_maps); - let module_full_path = relative_module_full_path( - ModuleDefId::FunctionId(func_id), - self.module_id, - current_module_parent_id, - self.interner, - )?; - let full_path = format!("{}::{}", module_full_path, trait_.name); + let module_full_path = if let Some(reexport_data) = trait_reexport { + relative_module_id_path( + *reexport_data.module_id, + &self.module_id, + current_module_parent_id, + self.interner, + ) + } else { + relative_module_full_path( + ModuleDefId::FunctionId(func_id), + self.module_id, + current_module_parent_id, + self.interner, + )? + }; + let full_path = format!("{}::{}", module_full_path, trait_name); let mut label_details = completion_item.label_details.clone().unwrap(); label_details.detail = Some(format!("(use {})", full_path)); completion_item.label_details = Some(label_details); diff --git a/noir/noir-repo/tooling/lsp/src/requests/completion/tests.rs b/noir/noir-repo/tooling/lsp/src/requests/completion/tests.rs index e6cfebddb0c..8ff568e3c26 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/completion/tests.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/completion/tests.rs @@ -3015,6 +3015,61 @@ mod moo { } } +fn main() { + let x: Field = 1; + x.fooba +} + "#; + assert_eq!(new_code, expected); + } + + #[test] + async fn test_suggests_and_imports_trait_method_with_self_using_public_export() { + let src = r#" +mod moo { + mod nested { + pub trait Foo { + fn foobar(self); + } + + impl Foo for Field { + fn foobar(self) {} + } + } + + pub use nested::Foo as Bar; +} + +fn main() { + let x: Field = 1; + x.fooba>|< +} + "#; + let mut items = get_completions(src).await; + assert_eq!(items.len(), 1); + + let item = items.remove(0); + assert_eq!(item.label_details.unwrap().detail, Some("(use moo::Bar)".to_string())); + + let new_code = + apply_text_edits(&src.replace(">|<", ""), &item.additional_text_edits.unwrap()); + + let expected = r#"use moo::Bar; + +mod moo { + mod nested { + pub trait Foo { + fn foobar(self); + } + + impl Foo for Field { + fn foobar(self) {} + } + } + + pub use nested::Foo as Bar; +} + fn main() { let x: Field = 1; x.fooba diff --git a/noir/noir-repo/tooling/lsp/src/requests/mod.rs b/noir/noir-repo/tooling/lsp/src/requests/mod.rs index d81108c2ec5..80f4a167a04 100644 --- a/noir/noir-repo/tooling/lsp/src/requests/mod.rs +++ b/noir/noir-repo/tooling/lsp/src/requests/mod.rs @@ -16,8 +16,9 @@ use lsp_types::{ }; use nargo_fmt::Config; +use noirc_frontend::ast::Ident; use noirc_frontend::graph::CrateId; -use noirc_frontend::hir::def_map::CrateDefMap; +use noirc_frontend::hir::def_map::{CrateDefMap, ModuleId}; use noirc_frontend::parser::ParserError; use noirc_frontend::usage_tracker::UsageTracker; use noirc_frontend::{graph::Dependency, node_interner::NodeInterner}; @@ -619,6 +620,12 @@ pub(crate) fn find_all_references( .unwrap_or_default() } +/// Represents a trait reexported from a given module with a name. +pub(crate) struct TraitReexport<'a> { + pub(super) module_id: &'a ModuleId, + pub(super) name: &'a Ident, +} + #[cfg(test)] mod initialization { use acvm::blackbox_solver::StubbedBlackBoxSolver; diff --git a/noir/noir-repo/tooling/nargo_cli/build.rs b/noir/noir-repo/tooling/nargo_cli/build.rs index 3394d00b3a2..3b1aff88755 100644 --- a/noir/noir-repo/tooling/nargo_cli/build.rs +++ b/noir/noir-repo/tooling/nargo_cli/build.rs @@ -197,8 +197,6 @@ fn test_{test_name}(force_brillig: ForceBrillig, inliner_aggressiveness: Inliner // Set the maximum increase so that part of the optimization is exercised (it might fail). nargo.arg("--max-bytecode-increase-percent"); nargo.arg("50"); - - // Check whether the test case is non-deterministic }} {test_content} diff --git a/noir/noir-repo/tooling/nargo_fmt/src/formatter/comments_and_whitespace.rs b/noir/noir-repo/tooling/nargo_fmt/src/formatter/comments_and_whitespace.rs index 57512d049df..e20eb4291d1 100644 --- a/noir/noir-repo/tooling/nargo_fmt/src/formatter/comments_and_whitespace.rs +++ b/noir/noir-repo/tooling/nargo_fmt/src/formatter/comments_and_whitespace.rs @@ -1,4 +1,4 @@ -use noirc_frontend::token::{DocStyle, Token}; +use noirc_frontend::token::Token; use super::Formatter; @@ -113,8 +113,7 @@ impl<'a> Formatter<'a> { last_was_block_comment = false; } - Token::LineComment(comment, None) - | Token::LineComment(comment, Some(DocStyle::Safety)) => { + Token::LineComment(comment, None) => { if comment.trim() == "noir-fmt:ignore" { ignore_next = true; } @@ -143,8 +142,7 @@ impl<'a> Formatter<'a> { last_was_block_comment = false; self.written_comments_count += 1; } - Token::BlockComment(comment, None) - | Token::BlockComment(comment, Some(DocStyle::Safety)) => { + Token::BlockComment(comment, None) => { if comment.trim() == "noir-fmt:ignore" { ignore_next = true; } diff --git a/noir/noir-repo/tooling/nargo_fmt/src/formatter/statement.rs b/noir/noir-repo/tooling/nargo_fmt/src/formatter/statement.rs index 77c3c2a396b..72d619c5e5f 100644 --- a/noir/noir-repo/tooling/nargo_fmt/src/formatter/statement.rs +++ b/noir/noir-repo/tooling/nargo_fmt/src/formatter/statement.rs @@ -75,6 +75,9 @@ impl<'a, 'b> ChunkFormatter<'a, 'b> { StatementKind::For(for_loop_statement) => { group.group(self.format_for_loop(for_loop_statement)); } + StatementKind::Loop(block) => { + group.group(self.format_loop(block)); + } StatementKind::Break => { group.text(self.chunk(|formatter| { formatter.write_keyword(Keyword::Break); @@ -277,6 +280,34 @@ impl<'a, 'b> ChunkFormatter<'a, 'b> { group } + fn format_loop(&mut self, block: Expression) -> ChunkGroup { + let mut group = ChunkGroup::new(); + + group.text(self.chunk(|formatter| { + formatter.write_keyword(Keyword::Loop); + })); + + group.space(self); + + let ExpressionKind::Block(block) = block.kind else { + panic!("Expected a block expression for loop body"); + }; + + group.group(self.format_block_expression( + block, true, // force multiple lines + )); + + // If there's a trailing semicolon, remove it + group.text(self.chunk(|formatter| { + formatter.skip_whitespace_if_it_is_not_a_newline(); + if formatter.is_at(Token::Semicolon) { + formatter.bump(); + } + })); + + group + } + fn format_comptime_statement(&mut self, statement: Statement) -> ChunkGroup { let mut group = ChunkGroup::new(); @@ -386,7 +417,7 @@ mod tests { #[test] fn format_let_statement_with_unsafe() { - let src = " fn foo() { + let src = " fn foo() { /// Safety: some doc let x = unsafe { 1 } ; } "; let expected = "fn foo() { @@ -399,7 +430,7 @@ mod tests { #[test] fn format_let_statement_with_unsafe_and_comment_before_it() { - let src = " fn foo() { + let src = " fn foo() { // Some comment /// Safety: some doc let x = unsafe { 1 } ; } "; @@ -527,7 +558,7 @@ mod tests { #[test] fn format_unsafe_statement() { - let src = " fn foo() { unsafe { + let src = " fn foo() { unsafe { 1 } } "; let expected = "fn foo() { unsafe { @@ -749,4 +780,27 @@ mod tests { let expected = src; assert_format_with_max_width(src, expected, " let x = 123456;".len()); } + + #[test] + fn format_empty_loop() { + let src = " fn foo() { loop { } } "; + let expected = "fn foo() { + loop {} +} +"; + assert_format(src, expected); + } + + #[test] + fn format_non_empty_loop() { + let src = " fn foo() { loop { 1 ; 2 } } "; + let expected = "fn foo() { + loop { + 1; + 2 + } +} +"; + assert_format(src, expected); + } }