diff --git a/.noir-sync-commit b/.noir-sync-commit index cd2a6b9708e..0816d70a9fd 100644 --- a/.noir-sync-commit +++ b/.noir-sync-commit @@ -1 +1,2 @@ -f03f8aed34393f7b0f6f68a189ce6c6192f6af6e +1252b5fcc7ed56bb55e95745b83be6e556805397 + diff --git a/barretenberg/cpp/src/barretenberg/dsl/acir_format/ec_operations.cpp b/barretenberg/cpp/src/barretenberg/dsl/acir_format/ec_operations.cpp index 8a89b1db7c0..86bde0342fe 100644 --- a/barretenberg/cpp/src/barretenberg/dsl/acir_format/ec_operations.cpp +++ b/barretenberg/cpp/src/barretenberg/dsl/acir_format/ec_operations.cpp @@ -35,10 +35,10 @@ void create_ec_add_constraint(Builder& builder, const EcAdd& input, bool has_val cycle_group_ct input2_point(x2, y2, infinite2); // Addition cycle_group_ct result = input1_point + input2_point; - - auto x_normalized = result.x.normalize(); - auto y_normalized = result.y.normalize(); - auto infinite = result.is_point_at_infinity().normalize(); + cycle_group_ct standard_result = result.get_standard_form(); + auto x_normalized = standard_result.x.normalize(); + auto y_normalized = standard_result.y.normalize(); + auto infinite = standard_result.is_point_at_infinity().normalize(); builder.assert_equal(x_normalized.witness_index, input.result_x); builder.assert_equal(y_normalized.witness_index, input.result_y); builder.assert_equal(infinite.witness_index, input.result_infinite); diff --git a/barretenberg/cpp/src/barretenberg/dsl/acir_format/multi_scalar_mul.cpp b/barretenberg/cpp/src/barretenberg/dsl/acir_format/multi_scalar_mul.cpp index 0f77ba48916..928eacb75f9 100644 --- a/barretenberg/cpp/src/barretenberg/dsl/acir_format/multi_scalar_mul.cpp +++ b/barretenberg/cpp/src/barretenberg/dsl/acir_format/multi_scalar_mul.cpp @@ -36,7 +36,7 @@ template void create_multi_scalar_mul_constraint(Builder& bui } // Call batch_mul to multiply the points and scalars and sum the results - auto output_point = cycle_group_ct::batch_mul(points, scalars); + auto output_point = cycle_group_ct::batch_mul(points, scalars).get_standard_form(); // Add the constraints builder.assert_equal(output_point.x.get_witness_index(), input.out_point_x); diff --git a/noir/noir-repo/.github/workflows/test-rust-workspace-msrv.yml b/noir/noir-repo/.github/workflows/test-rust-workspace-msrv.yml index ccd431ea879..ae016169830 100644 --- a/noir/noir-repo/.github/workflows/test-rust-workspace-msrv.yml +++ b/noir/noir-repo/.github/workflows/test-rust-workspace-msrv.yml @@ -111,7 +111,7 @@ jobs: fi env: # We treat any cancelled, skipped or failing jobs as a failure for the workflow as a whole. - FAIL: ${{ contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled') || contains(needs.*.result, 'skipped') }} + FAIL: ${{ contains(needs.*.result, 'failure') || contains(needs.*.result, 'skipped') }} - name: Checkout if: ${{ failure() }} diff --git a/noir/noir-repo/acvm-repo/brillig_vm/src/memory.rs b/noir/noir-repo/acvm-repo/brillig_vm/src/memory.rs index 4092cd06ae0..6bd9df49a51 100644 --- a/noir/noir-repo/acvm-repo/brillig_vm/src/memory.rs +++ b/noir/noir-repo/acvm-repo/brillig_vm/src/memory.rs @@ -294,6 +294,13 @@ impl Memory { } pub fn read_slice(&self, addr: MemoryAddress, len: usize) -> &[MemoryValue] { + // Allows to read a slice of uninitialized memory if the length is zero. + // Ideally we'd be able to read uninitialized memory in general (as read does) + // but that's not possible if we want to return a slice instead of owned data. + if len == 0 { + return &[]; + } + &self.inner[addr.to_usize()..(addr.to_usize() + len)] } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs index d38601bfc1b..80a63f223e7 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa.rs @@ -58,8 +58,9 @@ pub(crate) fn optimize_into_acir( let ssa = SsaBuilder::new(program, print_passes, force_brillig_output, print_timings)? .run_pass(Ssa::defunctionalize, "After Defunctionalization:") .run_pass(Ssa::remove_paired_rc, "After Removing Paired rc_inc & rc_decs:") - .run_pass(Ssa::inline_functions, "After Inlining:") + .run_pass(Ssa::separate_runtime, "After Runtime Separation:") .run_pass(Ssa::resolve_is_unconstrained, "After Resolving IsUnconstrained:") + .run_pass(Ssa::inline_functions, "After Inlining:") // Run mem2reg with the CFG separated into blocks .run_pass(Ssa::mem2reg, "After Mem2Reg:") .run_pass(Ssa::as_slice_optimization, "After `as_slice` optimization") diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs index 4a0f9f798ff..93dd47afe68 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/acir_variable.rs @@ -255,7 +255,7 @@ impl AcirContext { } /// Converts an [`AcirVar`] to a [`Witness`] - fn var_to_witness(&mut self, var: AcirVar) -> Result { + pub(crate) fn var_to_witness(&mut self, var: AcirVar) -> Result { let expression = self.var_to_expression(var)?; let witness = if let Some(constant) = expression.to_const() { // Check if a witness has been assigned this value already, if so reuse it. @@ -979,6 +979,7 @@ impl AcirContext { let max_power_of_two = self.add_constant( FieldElement::from(2_i128).pow(&FieldElement::from(bit_size as i128 - 1)), ); + let zero = self.add_constant(FieldElement::zero()); let one = self.add_constant(FieldElement::one()); // Get the sign bit of rhs by computing rhs / max_power_of_two @@ -998,10 +999,19 @@ impl AcirContext { // Unsigned to signed: derive q and r from q1,r1 and the signs of lhs and rhs // Quotient sign is lhs sign * rhs sign, whose resulting sign bit is the XOR of the sign bits let q_sign = self.xor_var(lhs_leading, rhs_leading, AcirType::unsigned(1))?; - let quotient = self.two_complement(q1, q_sign, bit_size)?; let remainder = self.two_complement(r1, lhs_leading, bit_size)?; + // Issue #5129 - When q1 is zero and quotient sign is -1, we compute -0=2^{bit_size}, + // which is not valid because we do not wrap integer operations + // Similar case can happen with the remainder. + let q_is_0 = self.eq_var(q1, zero)?; + let q_is_not_0 = self.not_var(q_is_0, AcirType::unsigned(1))?; + let quotient = self.mul_var(quotient, q_is_not_0)?; + let r_is_0 = self.eq_var(r1, zero)?; + let r_is_not_0 = self.not_var(r_is_0, AcirType::unsigned(1))?; + let remainder = self.mul_var(remainder, r_is_not_0)?; + Ok((quotient, remainder)) } @@ -1017,15 +1027,6 @@ impl AcirContext { Ok(remainder) } - /// Converts the `AcirVar` to a `Witness` if it hasn't been already, and appends it to the - /// `GeneratedAcir`'s return witnesses. - pub(crate) fn return_var(&mut self, acir_var: AcirVar) -> Result<(), InternalError> { - let return_var = self.get_or_create_witness_var(acir_var)?; - let witness = self.var_to_witness(return_var)?; - self.acir_ir.push_return_witness(witness); - Ok(()) - } - /// Constrains the `AcirVar` variable to be of type `NumericType`. pub(crate) fn range_constrain_var( &mut self, @@ -1528,9 +1529,11 @@ impl AcirContext { pub(crate) fn finish( mut self, inputs: Vec, + return_values: Vec, warnings: Vec, ) -> GeneratedAcir { self.acir_ir.input_witnesses = inputs; + self.acir_ir.return_witnesses = return_values; self.acir_ir.warnings = warnings; self.acir_ir } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs index 6c79c0a228d..9a09e7c06ee 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/acir_ir/generated_acir.rs @@ -45,9 +45,6 @@ pub(crate) struct GeneratedAcir { opcodes: Vec>, /// All witness indices that comprise the final return value of the program - /// - /// Note: This may contain repeated indices, which is necessary for later mapping into the - /// abi's return type. pub(crate) return_witnesses: Vec, /// All witness indices which are inputs to the main function @@ -164,11 +161,6 @@ impl GeneratedAcir { fresh_witness } - - /// Adds a witness index to the program's return witnesses. - pub(crate) fn push_return_witness(&mut self, witness: Witness) { - self.return_witnesses.push(witness); - } } impl GeneratedAcir { diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs index 13677506d0b..f3195688773 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs @@ -36,11 +36,8 @@ use acvm::acir::circuit::brillig::BrilligBytecode; use acvm::acir::circuit::{AssertionPayload, ErrorSelector, OpcodeLocation}; use acvm::acir::native_types::Witness; use acvm::acir::BlackBoxFunc; -use acvm::{ - acir::AcirField, - acir::{circuit::opcodes::BlockId, native_types::Expression}, - FieldElement, -}; +use acvm::{acir::circuit::opcodes::BlockId, acir::AcirField, FieldElement}; + use fxhash::FxHashMap as HashMap; use im::Vector; use iter_extended::{try_vecmap, vecmap}; @@ -330,38 +327,10 @@ impl Ssa { bytecode: brillig.byte_code, }); - let runtime_types = self.functions.values().map(|function| function.runtime()); - for (acir, runtime_type) in acirs.iter_mut().zip(runtime_types) { - if matches!(runtime_type, RuntimeType::Acir(_)) { - generate_distinct_return_witnesses(acir); - } - } - Ok((acirs, brillig, self.error_selector_to_type)) } } -fn generate_distinct_return_witnesses(acir: &mut GeneratedAcir) { - // Create a witness for each return witness we have to guarantee that the return witnesses match the standard - // layout for serializing those types as if they were being passed as inputs. - // - // This is required for recursion as otherwise in situations where we cannot make use of the program's ABI - // (e.g. for `std::verify_proof` or the solidity verifier), we need extra knowledge about the program we're - // working with rather than following the standard ABI encoding rules. - // - // TODO: We're being conservative here by generating a new witness for every expression. - // This means that we're likely to get a number of constraints which are just renumbering witnesses. - // This can be tackled by: - // - Tracking the last assigned public input witness and only renumbering a witness if it is below this value. - // - Modifying existing constraints to rearrange their outputs so they are suitable - // - See: https://github.com/noir-lang/noir/pull/4467 - let distinct_return_witness = vecmap(acir.return_witnesses.clone(), |return_witness| { - acir.create_witness_for_expression(&Expression::from(return_witness)) - }); - - acir.return_witnesses = distinct_return_witness; -} - impl<'a> Context<'a> { fn new(shared_context: &'a mut SharedContext) -> Context<'a> { let mut acir_context = AcirContext::default(); @@ -422,6 +391,25 @@ impl<'a> Context<'a> { let dfg = &main_func.dfg; let entry_block = &dfg[main_func.entry_block()]; let input_witness = self.convert_ssa_block_params(entry_block.parameters(), dfg)?; + let num_return_witnesses = + self.get_num_return_witnesses(entry_block.unwrap_terminator(), dfg); + + // Create a witness for each return witness we have to guarantee that the return witnesses match the standard + // layout for serializing those types as if they were being passed as inputs. + // + // This is required for recursion as otherwise in situations where we cannot make use of the program's ABI + // (e.g. for `std::verify_proof` or the solidity verifier), we need extra knowledge about the program we're + // working with rather than following the standard ABI encoding rules. + // + // We allocate these witnesses now before performing ACIR gen for the rest of the program as the location of + // the function's return values can then be determined through knowledge of its ABI alone. + let return_witness_vars = + vecmap(0..num_return_witnesses, |_| self.acir_context.add_variable()); + + let return_witnesses = vecmap(&return_witness_vars, |return_var| { + let expr = self.acir_context.var_to_expression(*return_var).unwrap(); + expr.to_witness().expect("return vars should be witnesses") + }); self.data_bus = dfg.data_bus.to_owned(); let mut warnings = Vec::new(); @@ -429,8 +417,19 @@ impl<'a> Context<'a> { warnings.extend(self.convert_ssa_instruction(*instruction_id, dfg, ssa, brillig)?); } - warnings.extend(self.convert_ssa_return(entry_block.unwrap_terminator(), dfg)?); - Ok(self.acir_context.finish(input_witness, warnings)) + let (return_vars, return_warnings) = + self.convert_ssa_return(entry_block.unwrap_terminator(), dfg)?; + + // TODO: This is a naive method of assigning the return values to their witnesses as + // we're likely to get a number of constraints which are asserting one witness to be equal to another. + // + // We should search through the program and relabel these witnesses so we can remove this constraint. + for (witness_var, return_var) in return_witness_vars.iter().zip(return_vars) { + self.acir_context.assert_eq_var(*witness_var, return_var, None)?; + } + + warnings.extend(return_warnings); + Ok(self.acir_context.finish(input_witness, return_witnesses, warnings)) } fn convert_brillig_main( @@ -468,17 +467,13 @@ impl<'a> Context<'a> { )?; self.shared_context.insert_generated_brillig(main_func.id(), arguments, 0, code); - let output_vars: Vec<_> = output_values + let return_witnesses: Vec = output_values .iter() .flat_map(|value| value.clone().flatten()) - .map(|value| value.0) - .collect(); - - for acir_var in output_vars { - self.acir_context.return_var(acir_var)?; - } + .map(|(value, _)| self.acir_context.var_to_witness(value)) + .collect::>()?; - let generated_acir = self.acir_context.finish(witness_inputs, Vec::new()); + let generated_acir = self.acir_context.finish(witness_inputs, return_witnesses, Vec::new()); assert_eq!( generated_acir.opcodes().len(), @@ -1724,12 +1719,39 @@ impl<'a> Context<'a> { self.define_result(dfg, instruction, AcirValue::Var(result, typ)); } + /// Converts an SSA terminator's return values into their ACIR representations + fn get_num_return_witnesses( + &mut self, + terminator: &TerminatorInstruction, + dfg: &DataFlowGraph, + ) -> usize { + let return_values = match terminator { + TerminatorInstruction::Return { return_values, .. } => return_values, + // TODO(https://github.com/noir-lang/noir/issues/4616): Enable recursion on foldable/non-inlined ACIR functions + _ => unreachable!("ICE: Program must have a singular return"), + }; + + return_values.iter().fold(0, |acc, value_id| { + let is_databus = self + .data_bus + .return_data + .map_or(false, |return_databus| dfg[*value_id] == dfg[return_databus]); + + if is_databus { + // We do not return value for the data bus. + acc + } else { + acc + dfg.type_of_value(*value_id).flattened_size() + } + }) + } + /// Converts an SSA terminator's return values into their ACIR representations fn convert_ssa_return( &mut self, terminator: &TerminatorInstruction, dfg: &DataFlowGraph, - ) -> Result, RuntimeError> { + ) -> Result<(Vec, Vec), RuntimeError> { let (return_values, call_stack) = match terminator { TerminatorInstruction::Return { return_values, call_stack } => { (return_values, call_stack.clone()) @@ -1739,6 +1761,7 @@ impl<'a> Context<'a> { }; let mut has_constant_return = false; + let mut return_vars: Vec = Vec::new(); for value_id in return_values { let is_databus = self .data_bus @@ -1759,7 +1782,7 @@ impl<'a> Context<'a> { dfg, )?; } else { - self.acir_context.return_var(acir_var)?; + return_vars.push(acir_var); } } } @@ -1770,7 +1793,7 @@ impl<'a> Context<'a> { Vec::new() }; - Ok(warnings) + Ok((return_vars, warnings)) } /// Gets the cached `AcirVar` that was converted from the corresponding `ValueId`. If it does @@ -3079,8 +3102,8 @@ mod test { check_call_opcode( &func_with_nested_call_opcodes[1], 2, - vec![Witness(2), Witness(1)], - vec![Witness(3)], + vec![Witness(3), Witness(1)], + vec![Witness(4)], ); } @@ -3100,13 +3123,13 @@ mod test { for (expected_input, input) in expected_inputs.iter().zip(inputs) { assert_eq!( expected_input, input, - "Expected witness {expected_input:?} but got {input:?}" + "Expected input witness {expected_input:?} but got {input:?}" ); } for (expected_output, output) in expected_outputs.iter().zip(outputs) { assert_eq!( expected_output, output, - "Expected witness {expected_output:?} but got {output:?}" + "Expected output witness {expected_output:?} but got {output:?}" ); } } 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 545827df1c5..f763ae52d50 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 @@ -22,7 +22,7 @@ use noirc_errors::Location; /// its blocks, instructions, and values. This struct is largely responsible for /// owning most data in a function and handing out Ids to this data that can be /// shared without worrying about ownership. -#[derive(Debug, Default)] +#[derive(Debug, Default, Clone)] pub(crate) struct DataFlowGraph { /// All of the instructions in a function instructions: DenseMap, 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 a49e02b0380..c44824b464b 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 @@ -64,6 +64,13 @@ impl Function { Self { name, id, entry_block, dfg, runtime: RuntimeType::Acir(InlineType::default()) } } + /// Creates a new function as a clone of the one passed in with the passed in id. + pub(crate) fn clone_with_id(id: FunctionId, another: &Function) -> Self { + let dfg = another.dfg.clone(); + let entry_block = another.entry_block; + Self { name: another.name.clone(), id, entry_block, dfg, runtime: another.runtime } + } + /// The name of the function. /// Used exclusively for debugging purposes. pub(crate) fn name(&self) -> &str { 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 5110140bfcc..7517b54aac3 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 @@ -123,7 +123,12 @@ impl Intrinsic { | Intrinsic::IsUnconstrained => false, // Some black box functions have side-effects - Intrinsic::BlackBox(func) => matches!(func, BlackBoxFunc::RecursiveAggregation), + Intrinsic::BlackBox(func) => matches!( + func, + BlackBoxFunc::RecursiveAggregation + | BlackBoxFunc::MultiScalarMul + | BlackBoxFunc::EmbeddedCurveAdd + ), } } @@ -340,6 +345,9 @@ impl Instruction { // Some `Intrinsic`s have side effects so we must check what kind of `Call` this is. Call { func, .. } => match dfg[*func] { + // Explicitly allows removal of unused ec operations, even if they can fail + Value::Intrinsic(Intrinsic::BlackBox(BlackBoxFunc::MultiScalarMul)) + | Value::Intrinsic(Intrinsic::BlackBox(BlackBoxFunc::EmbeddedCurveAdd)) => true, Value::Intrinsic(intrinsic) => !intrinsic.has_side_effects(), // All foreign functions are treated as having side effects. diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/map.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/map.rs index b6055973f1c..3c3feabc390 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/map.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/map.rs @@ -115,7 +115,7 @@ impl std::fmt::Display for Id { /// access to indices is provided. Since IDs must be stable and correspond /// to indices in the internal Vec, operations that would change element /// ordering like pop, remove, swap_remove, etc, are not possible. -#[derive(Debug)] +#[derive(Debug, Clone)] pub(crate) struct DenseMap { storage: Vec, } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs index 690c0244f62..e07f947db8d 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/flatten_cfg.rs @@ -134,7 +134,7 @@ use fxhash::FxHashMap as HashMap; use std::collections::{BTreeMap, HashSet}; -use acvm::{acir::AcirField, FieldElement}; +use acvm::{acir::AcirField, acir::BlackBoxFunc, FieldElement}; use iter_extended::vecmap; use crate::ssa::{ @@ -769,7 +769,38 @@ impl<'f> Context<'f> { Instruction::Call { func, arguments } } + //Issue #5045: We set curve points to infinity if condition is false + Value::Intrinsic(Intrinsic::BlackBox(BlackBoxFunc::EmbeddedCurveAdd)) => { + arguments[2] = self.var_or_one(arguments[2], condition, call_stack.clone()); + arguments[5] = self.var_or_one(arguments[5], condition, call_stack.clone()); + Instruction::Call { func, arguments } + } + Value::Intrinsic(Intrinsic::BlackBox(BlackBoxFunc::MultiScalarMul)) => { + let mut array_with_predicate = im::Vector::new(); + let array_typ; + if let Value::Array { array, typ } = + &self.inserter.function.dfg[arguments[0]] + { + array_typ = typ.clone(); + for (i, value) in array.clone().iter().enumerate() { + if i % 3 == 2 { + array_with_predicate.push_back(self.var_or_one( + *value, + condition, + call_stack.clone(), + )); + } else { + array_with_predicate.push_back(*value); + } + } + } else { + unreachable!(); + } + arguments[0] = + self.inserter.function.dfg.make_array(array_with_predicate, array_typ); + Instruction::Call { func, arguments } + } _ => Instruction::Call { func, arguments }, }, other => other, @@ -779,6 +810,20 @@ impl<'f> Context<'f> { } } + // Computes: if condition { var } else { 1 } + fn var_or_one(&mut self, var: ValueId, condition: ValueId, call_stack: CallStack) -> ValueId { + let field = self.insert_instruction( + Instruction::binary(BinaryOp::Mul, var, condition), + call_stack.clone(), + ); + let not_condition = + self.insert_instruction(Instruction::Not(condition), call_stack.clone()); + self.insert_instruction( + Instruction::binary(BinaryOp::Add, field, not_condition), + call_stack, + ) + } + fn undo_stores_in_then_branch(&mut self, store_values: &HashMap) { for (address, store) in store_values { let address = *address; 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 1293671da50..e2a7f51d0a0 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 @@ -27,7 +27,7 @@ const RECURSION_LIMIT: u32 = 1000; impl Ssa { /// Inline all functions within the IR. /// - /// In the case of recursive functions, this will attempt + /// In the case of recursive Acir functions, this will attempt /// to recursively inline until the RECURSION_LIMIT is reached. /// /// Functions are recursively inlined into main until either we finish @@ -41,6 +41,8 @@ impl Ssa { /// There are some attributes that allow inlining a function at a different step of codegen. /// Currently this is just `InlineType::NoPredicates` for which we have a flag indicating /// whether treating that inline functions. The default is to treat these functions as entry points. + /// + /// 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) -> Ssa { Self::inline_functions_inner(self, true) @@ -52,12 +54,17 @@ impl Ssa { } fn inline_functions_inner(mut self, no_predicates_is_entry_point: bool) -> Ssa { + let recursive_functions = find_all_recursive_functions(&self); self.functions = btree_map( - get_entry_point_functions(&self, no_predicates_is_entry_point), + get_functions_to_inline_into(&self, no_predicates_is_entry_point), |entry_point| { - let new_function = - InlineContext::new(&self, entry_point, no_predicates_is_entry_point) - .inline_all(&self); + let new_function = InlineContext::new( + &self, + entry_point, + no_predicates_is_entry_point, + recursive_functions.clone(), + ) + .inline_all(&self); (entry_point, new_function) }, ); @@ -80,6 +87,8 @@ struct InlineContext { entry_point: FunctionId, no_predicates_is_entry_point: bool, + // We keep track of the recursive functions in the SSA to avoid inlining them in a brillig context. + recursive_functions: BTreeSet, } /// The per-function inlining context contains information that is only valid for one function. @@ -113,28 +122,101 @@ struct PerFunctionContext<'function> { inlining_entry: bool, } -/// The entry point functions are each function we should inline into - and each function that -/// should be left in the final program. -/// This is the `main` function, any Acir functions with a [fold inline type][InlineType::Fold], -/// and any brillig functions used. -fn get_entry_point_functions( +/// Utility function to find out the direct calls of a function. +fn called_functions(func: &Function) -> BTreeSet { + let mut called_function_ids = BTreeSet::default(); + for block_id in func.reachable_blocks() { + for instruction_id in func.dfg[block_id].instructions() { + let Instruction::Call { func: called_value_id, .. } = &func.dfg[*instruction_id] else { + continue; + }; + + if let Value::Function(function_id) = func.dfg[*called_value_id] { + called_function_ids.insert(function_id); + } + } + } + + called_function_ids +} + +// Recursively explore the SSA to find the functions that end up calling themselves +fn find_recursive_functions( + ssa: &Ssa, + current_function: FunctionId, + mut explored_functions: im::HashSet, + recursive_functions: &mut BTreeSet, +) { + if explored_functions.contains(¤t_function) { + recursive_functions.insert(current_function); + return; + } + + let called_functions = called_functions(&ssa.functions[¤t_function]); + + explored_functions.insert(current_function); + + for called_function in called_functions { + find_recursive_functions( + ssa, + called_function, + explored_functions.clone(), + recursive_functions, + ); + } +} + +fn find_all_recursive_functions(ssa: &Ssa) -> BTreeSet { + let mut recursive_functions = BTreeSet::default(); + find_recursive_functions(ssa, ssa.main_id, im::HashSet::default(), &mut recursive_functions); + recursive_functions +} + +/// The functions we should inline into (and that should be left in the final program) are: +/// - main +/// - Any Brillig function called from Acir +/// - Any Brillig recursive function (Acir recursive functions will be inlined into the main function) +/// - Any Acir functions with a [fold inline type][InlineType::Fold], +fn get_functions_to_inline_into( ssa: &Ssa, no_predicates_is_entry_point: bool, ) -> BTreeSet { - let functions = ssa.functions.iter(); - let mut entry_points = functions - .filter(|(_, function)| { - // If we have not already finished the flattening pass, functions marked - // to not have predicates should be marked as entry points. - let no_predicates_is_entry_point = - no_predicates_is_entry_point && function.is_no_predicates(); - function.runtime().is_entry_point() || no_predicates_is_entry_point + let mut brillig_entry_points = BTreeSet::default(); + let mut acir_entry_points = BTreeSet::default(); + + for (func_id, function) in ssa.functions.iter() { + if function.runtime() == RuntimeType::Brillig { + continue; + } + + // If we have not already finished the flattening pass, functions marked + // to not have predicates should be marked as entry points. + let no_predicates_is_entry_point = + no_predicates_is_entry_point && function.is_no_predicates(); + if function.runtime().is_entry_point() || no_predicates_is_entry_point { + acir_entry_points.insert(*func_id); + } + + for called_function_id in called_functions(function) { + if ssa.functions[&called_function_id].runtime() == RuntimeType::Brillig { + brillig_entry_points.insert(called_function_id); + } + } + } + + let brillig_recursive_functions: BTreeSet<_> = find_all_recursive_functions(ssa) + .into_iter() + .filter(|recursive_function_id| { + let function = &ssa.functions[&recursive_function_id]; + function.runtime() == RuntimeType::Brillig }) - .map(|(id, _)| *id) - .collect::>(); + .collect(); - entry_points.insert(ssa.main_id); - entry_points + std::iter::once(ssa.main_id) + .chain(acir_entry_points) + .chain(brillig_entry_points) + .chain(brillig_recursive_functions) + .collect() } impl InlineContext { @@ -147,6 +229,7 @@ impl InlineContext { ssa: &Ssa, entry_point: FunctionId, no_predicates_is_entry_point: bool, + recursive_functions: BTreeSet, ) -> InlineContext { let source = &ssa.functions[&entry_point]; let mut builder = FunctionBuilder::new(source.name().to_owned(), entry_point); @@ -157,6 +240,7 @@ impl InlineContext { entry_point, call_stack: CallStack::new(), no_predicates_is_entry_point, + recursive_functions, } } @@ -391,18 +475,10 @@ impl<'function> PerFunctionContext<'function> { match &self.source_function.dfg[*id] { Instruction::Call { func, arguments } => match self.get_function(*func) { Some(func_id) => { - let function = &ssa.functions[&func_id]; - // If we have not already finished the flattening pass, functions marked - // to not have predicates should be marked as entry points unless we are inlining into brillig. - let entry_point = &ssa.functions[&self.context.entry_point]; - let no_predicates_is_entry_point = - self.context.no_predicates_is_entry_point - && function.is_no_predicates() - && !matches!(entry_point.runtime(), RuntimeType::Brillig); - if function.runtime().is_entry_point() || no_predicates_is_entry_point { - self.push_instruction(*id); - } else { + if self.should_inline_call(ssa, func_id) { self.inline_function(ssa, *id, func_id, arguments); + } else { + self.push_instruction(*id); } } None => self.push_instruction(*id), @@ -412,6 +488,24 @@ 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 marked as entry points. + let no_predicates_is_entry_point = + self.context.no_predicates_is_entry_point && function.is_no_predicates(); + !inline_type.is_entry_point() && !no_predicates_is_entry_point + } else { + // If the called function is brillig, we inline only if it's into brillig and the function is not recursive + ssa.functions[&self.context.entry_point].runtime() == RuntimeType::Brillig + && !self.context.recursive_functions.contains(&called_func_id) + } + } + /// Inline a function call and remember the inlined return values in the values map fn inline_function( &mut self, 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 f6c3f022bfc..4e5fa262696 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 @@ -18,5 +18,6 @@ mod remove_bit_shifts; mod remove_enable_side_effects; mod remove_if_else; mod resolve_is_unconstrained; +mod runtime_separation; mod simplify_cfg; mod unrolling; diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/runtime_separation.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/runtime_separation.rs new file mode 100644 index 00000000000..c0c9c0a1372 --- /dev/null +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/runtime_separation.rs @@ -0,0 +1,348 @@ +use std::collections::BTreeSet; + +use fxhash::{FxHashMap as HashMap, FxHashSet as HashSet}; + +use crate::ssa::{ + ir::{ + function::{Function, FunctionId, RuntimeType}, + instruction::Instruction, + value::{Value, ValueId}, + }, + ssa_gen::Ssa, +}; + +impl Ssa { + /// This optimization step separates the runtime of the functions in the SSA. + /// After this step, all functions with runtime `Acir` will be converted to Acir and + /// the functions with runtime `Brillig` will be converted to Brillig. + /// It does so by cloning all ACIR functions called from a Brillig context + /// and changing the runtime of the cloned functions to Brillig. + /// This pass needs to run after functions as values have been resolved (defunctionalization). + #[tracing::instrument(level = "trace", skip(self))] + pub(crate) fn separate_runtime(mut self) -> Self { + RuntimeSeparatorContext::separate_runtime(&mut self); + self + } +} + +#[derive(Debug, Default)] +struct RuntimeSeparatorContext { + // Original functions to clone to brillig + acir_functions_called_from_brillig: BTreeSet, + // Tracks the original => cloned version + mapped_functions: HashMap, +} + +impl RuntimeSeparatorContext { + pub(crate) fn separate_runtime(ssa: &mut Ssa) { + let mut runtime_separator = RuntimeSeparatorContext::default(); + + // We first collect all the acir functions called from a brillig context by exploring the SSA recursively + let mut processed_functions = HashSet::default(); + runtime_separator.collect_acir_functions_called_from_brillig( + ssa, + ssa.main_id, + false, + &mut processed_functions, + ); + + // Now we clone the relevant acir functions and change their runtime to brillig + runtime_separator.convert_acir_functions_called_from_brillig_to_brillig(ssa); + + // Now we update any calls within a brillig context to the mapped functions + runtime_separator.replace_calls_to_mapped_functions(ssa); + + // Some functions might be unreachable now (for example an acir function only called from brillig) + prune_unreachable_functions(ssa); + } + + fn collect_acir_functions_called_from_brillig( + &mut self, + ssa: &Ssa, + current_func_id: FunctionId, + mut within_brillig: bool, + processed_functions: &mut HashSet<(/* within_brillig */ bool, FunctionId)>, + ) { + // Processed functions needs the within brillig flag, since it is possible to call the same function from both brillig and acir + if processed_functions.contains(&(within_brillig, current_func_id)) { + return; + } + processed_functions.insert((within_brillig, current_func_id)); + + let func = &ssa.functions[¤t_func_id]; + if func.runtime() == RuntimeType::Brillig { + within_brillig = true; + } + + let called_functions = called_functions(func); + + if within_brillig { + for called_func_id in called_functions.iter() { + let called_func = &ssa.functions[&called_func_id]; + if matches!(called_func.runtime(), RuntimeType::Acir(_)) { + self.acir_functions_called_from_brillig.insert(*called_func_id); + } + } + } + + for called_func_id in called_functions.into_iter() { + self.collect_acir_functions_called_from_brillig( + ssa, + called_func_id, + within_brillig, + processed_functions, + ); + } + } + + fn convert_acir_functions_called_from_brillig_to_brillig(&mut self, ssa: &mut Ssa) { + for acir_func_id in self.acir_functions_called_from_brillig.iter() { + let cloned_id = ssa.clone_fn(*acir_func_id); + let new_func = + ssa.functions.get_mut(&cloned_id).expect("Cloned function should exist in SSA"); + new_func.set_runtime(RuntimeType::Brillig); + self.mapped_functions.insert(*acir_func_id, cloned_id); + } + } + + fn replace_calls_to_mapped_functions(&self, ssa: &mut Ssa) { + for (_function_id, func) in ssa.functions.iter_mut() { + if func.runtime() == RuntimeType::Brillig { + for called_func_value_id in called_functions_values(func).iter() { + let Value::Function(called_func_id) = &func.dfg[*called_func_value_id] else { + unreachable!("Value should be a function") + }; + if let Some(mapped_func_id) = self.mapped_functions.get(called_func_id) { + let mapped_value_id = func.dfg.import_function(*mapped_func_id); + func.dfg.set_value_from_id(*called_func_value_id, mapped_value_id); + } + } + } + } + } +} + +// We only consider direct calls to functions since functions as values should have been resolved +fn called_functions_values(func: &Function) -> BTreeSet { + let mut called_function_ids = BTreeSet::default(); + for block_id in func.reachable_blocks() { + for instruction_id in func.dfg[block_id].instructions() { + let Instruction::Call { func: called_value_id, .. } = &func.dfg[*instruction_id] else { + continue; + }; + + if let Value::Function(_) = func.dfg[*called_value_id] { + called_function_ids.insert(*called_value_id); + } + } + } + + called_function_ids +} + +fn called_functions(func: &Function) -> BTreeSet { + called_functions_values(func) + .into_iter() + .map(|value_id| { + let Value::Function(func_id) = func.dfg[value_id] else { + unreachable!("Value should be a function") + }; + func_id + }) + .collect() +} + +fn collect_reachable_functions( + ssa: &Ssa, + current_func_id: FunctionId, + reachable_functions: &mut HashSet, +) { + if reachable_functions.contains(¤t_func_id) { + return; + } + reachable_functions.insert(current_func_id); + + let func = &ssa.functions[¤t_func_id]; + let called_functions = called_functions(func); + + for called_func_id in called_functions.iter() { + collect_reachable_functions(ssa, *called_func_id, reachable_functions); + } +} + +fn prune_unreachable_functions(ssa: &mut Ssa) { + let mut reachable_functions = HashSet::default(); + collect_reachable_functions(ssa, ssa.main_id, &mut reachable_functions); + + ssa.functions.retain(|id, _value| reachable_functions.contains(id)); +} + +#[cfg(test)] +mod test { + use std::collections::BTreeSet; + + use noirc_frontend::monomorphization::ast::InlineType; + + use crate::ssa::{ + function_builder::FunctionBuilder, + ir::{ + function::{Function, FunctionId, RuntimeType}, + map::Id, + types::Type, + }, + opt::runtime_separation::called_functions, + ssa_gen::Ssa, + }; + + #[test] + fn basic_runtime_separation() { + // brillig fn foo { + // b0(): + // v0 = call bar() + // return v0 + // } + // acir fn bar { + // b0(): + // return 72 + // } + let foo_id = Id::test_new(0); + let mut builder = FunctionBuilder::new("foo".into(), foo_id); + builder.current_function.set_runtime(RuntimeType::Brillig); + + let bar_id = Id::test_new(1); + let bar = builder.import_function(bar_id); + let results = builder.insert_call(bar, Vec::new(), vec![Type::field()]).to_vec(); + builder.terminate_with_return(results); + + builder.new_function("bar".into(), bar_id, InlineType::default()); + let expected_return = 72u128; + let seventy_two = builder.field_constant(expected_return); + builder.terminate_with_return(vec![seventy_two]); + + let ssa = builder.finish(); + assert_eq!(ssa.functions.len(), 2); + + // Expected result + // brillig fn foo { + // b0(): + // v0 = call bar() + // return v0 + // } + // brillig fn bar { + // b0(): + // return 72 + // } + let separated = ssa.separate_runtime(); + + // The original bar function must have been pruned + assert_eq!(separated.functions.len(), 2); + + // All functions should be brillig now + for func in separated.functions.values() { + assert_eq!(func.runtime(), RuntimeType::Brillig); + } + } + + fn find_func_by_name<'ssa>( + ssa: &'ssa Ssa, + funcs: &BTreeSet, + name: &str, + ) -> &'ssa Function { + funcs + .iter() + .find_map(|id| { + let func = ssa.functions.get(id).unwrap(); + if func.name() == name { + Some(func) + } else { + None + } + }) + .unwrap() + } + + #[test] + fn same_function_shared_acir_brillig() { + // acir fn foo { + // b0(): + // v0 = call bar() + // v1 = call baz() + // return v0, v1 + // } + // brillig fn bar { + // b0(): + // v0 = call baz() + // return v0 + // } + // acir fn baz { + // b0(): + // return 72 + // } + let foo_id = Id::test_new(0); + let mut builder = FunctionBuilder::new("foo".into(), foo_id); + + let bar_id = Id::test_new(1); + let baz_id = Id::test_new(2); + let bar = builder.import_function(bar_id); + let baz = builder.import_function(baz_id); + let v0 = builder.insert_call(bar, Vec::new(), vec![Type::field()]).to_vec(); + let v1 = builder.insert_call(baz, Vec::new(), vec![Type::field()]).to_vec(); + builder.terminate_with_return(vec![v0[0], v1[0]]); + + builder.new_brillig_function("bar".into(), bar_id); + let baz = builder.import_function(baz_id); + let v0 = builder.insert_call(baz, Vec::new(), vec![Type::field()]).to_vec(); + builder.terminate_with_return(v0); + + builder.new_function("baz".into(), baz_id, InlineType::default()); + let expected_return = 72u128; + let seventy_two = builder.field_constant(expected_return); + builder.terminate_with_return(vec![seventy_two]); + + let ssa = builder.finish(); + assert_eq!(ssa.functions.len(), 3); + + // Expected result + // acir fn foo { + // b0(): + // v0 = call bar() + // v1 = call baz() <- baz_acir + // return v0, v1 + // } + // brillig fn bar { + // b0(): + // v0 = call baz() <- baz_brillig + // return v0 + // } + // acir fn baz { + // b0(): + // return 72 + // } + // brillig fn baz { + // b0(): + // return 72 + // } + let separated = ssa.separate_runtime(); + + // The original baz function must have been duplicated + assert_eq!(separated.functions.len(), 4); + + let main_function = separated.functions.get(&separated.main_id).unwrap(); + assert_eq!(main_function.runtime(), RuntimeType::Acir(InlineType::Inline)); + + let main_calls = called_functions(main_function); + assert_eq!(main_calls.len(), 2); + + let bar = find_func_by_name(&separated, &main_calls, "bar"); + let baz_acir = find_func_by_name(&separated, &main_calls, "baz"); + + assert_eq!(baz_acir.runtime(), RuntimeType::Acir(InlineType::Inline)); + assert_eq!(bar.runtime(), RuntimeType::Brillig); + + let bar_calls = called_functions(bar); + assert_eq!(bar_calls.len(), 1); + + let baz_brillig = find_func_by_name(&separated, &bar_calls, "baz"); + assert_eq!(baz_brillig.runtime(), RuntimeType::Brillig); + } +} diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs index 21178c55c73..7a77aa76101 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/program.rs @@ -80,6 +80,14 @@ impl Ssa { self.functions.insert(new_id, function); new_id } + + /// Clones an already existing function with a fresh id + pub(crate) fn clone_fn(&mut self, existing_function_id: FunctionId) -> FunctionId { + let new_id = self.next_id.next(); + let function = Function::clone_with_id(new_id, &self.functions[&existing_function_id]); + self.functions.insert(new_id, function); + new_id + } } impl Display for Ssa { diff --git a/noir/noir-repo/compiler/noirc_frontend/src/ast/mod.rs b/noir/noir-repo/compiler/noirc_frontend/src/ast/mod.rs index 090a41fa7d9..a9d9f232b64 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/ast/mod.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/ast/mod.rs @@ -92,7 +92,7 @@ pub enum UnresolvedTypeData { Integer(Signedness, IntegerBitSize), // u32 = Integer(unsigned, ThirtyTwo) Bool, Expression(UnresolvedTypeExpression), - String(Option), + String(UnresolvedTypeExpression), FormatString(UnresolvedTypeExpression, Box), Unit, @@ -191,10 +191,7 @@ impl std::fmt::Display for UnresolvedTypeData { } Expression(expression) => expression.fmt(f), Bool => write!(f, "bool"), - String(len) => match len { - None => write!(f, "str<_>"), - Some(len) => write!(f, "str<{len}>"), - }, + String(len) => write!(f, "str<{len}>"), FormatString(len, elements) => write!(f, "fmt<{len}, {elements}"), Function(args, ret, env) => { let args = vecmap(args, ToString::to_string).join(", "); diff --git a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/expressions.rs b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/expressions.rs index 2ad1598c6a2..8acd1867074 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/expressions.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/expressions.rs @@ -348,6 +348,8 @@ impl<'context> Elaborator<'context> { let func_type = self.type_check_variable(function_name, function_id, turbofish_generics); + self.interner.push_expr_type(function_id, func_type.clone()); + // Type check the new call now that it has been changed from a method call // to a function call. This way we avoid duplicating code. let typ = self.type_check_call(&function_call, func_type, function_args, span); 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 10ebd657494..14bee5b97e0 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/mod.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/mod.rs @@ -15,7 +15,7 @@ use crate::{ }, resolution::{errors::ResolverError, path_resolver::PathResolver, resolver::LambdaContext}, scope::ScopeForest as GenericScopeForest, - type_check::TypeCheckError, + type_check::{check_trait_impl_method_matches_declaration, TypeCheckError}, }, hir_def::{expr::HirIdent, function::Parameters, traits::TraitConstraint}, macros_api::{ @@ -273,7 +273,7 @@ impl<'context> Elaborator<'context> { self.trait_id = None; } - fn elaborate_function(&mut self, mut function: NoirFunction, id: FuncId) { + fn elaborate_function(&mut self, function: NoirFunction, id: FuncId) { self.current_function = Some(id); // Without this, impl methods can accidentally be placed in contracts. See #3254 @@ -303,7 +303,6 @@ impl<'context> Elaborator<'context> { } self.generics = func_meta.all_generics.clone(); - self.desugar_impl_trait_args(&mut function, id); self.declare_numeric_generics(&func_meta.parameters, func_meta.return_type()); self.add_trait_constraints_to_scope(&func_meta); @@ -373,49 +372,31 @@ impl<'context> Elaborator<'context> { } /// This turns function parameters of the form: - /// fn foo(x: impl Bar) + /// `fn foo(x: impl Bar)` /// /// into - /// fn foo(x: T0_impl_Bar) where T0_impl_Bar: Bar - fn desugar_impl_trait_args(&mut self, func: &mut NoirFunction, func_id: FuncId) { - let mut impl_trait_generics = HashSet::default(); - let mut counter: usize = 0; - for parameter in func.def.parameters.iter_mut() { - if let UnresolvedTypeData::TraitAsType(path, args) = ¶meter.typ.typ { - let mut new_generic_ident: Ident = - format!("T{}_impl_{}", func_id, path.as_string()).into(); - let mut new_generic_path = Path::from_ident(new_generic_ident.clone()); - while impl_trait_generics.contains(&new_generic_ident) - || self.lookup_generic_or_global_type(&new_generic_path).is_some() - { - new_generic_ident = - format!("T{}_impl_{}_{}", func_id, path.as_string(), counter).into(); - new_generic_path = Path::from_ident(new_generic_ident.clone()); - counter += 1; - } - impl_trait_generics.insert(new_generic_ident.clone()); - - let is_synthesized = true; - let new_generic_type_data = - UnresolvedTypeData::Named(new_generic_path, vec![], is_synthesized); - let new_generic_type = - UnresolvedType { typ: new_generic_type_data.clone(), span: None }; - let new_trait_bound = TraitBound { - trait_path: path.clone(), - trait_id: None, - trait_generics: args.to_vec(), - }; - let new_trait_constraint = UnresolvedTraitConstraint { - typ: new_generic_type, - trait_bound: new_trait_bound, - }; - - parameter.typ.typ = new_generic_type_data; - func.def.generics.push(new_generic_ident); - func.def.where_clause.push(new_trait_constraint); - } + /// `fn foo(x: T0_impl_Bar) where T0_impl_Bar: Bar` + /// although the fresh type variable is not named internally. + fn desugar_impl_trait_arg( + &mut self, + trait_path: Path, + trait_generics: Vec, + generics: &mut Vec, + trait_constraints: &mut Vec, + ) -> Type { + let new_generic_id = self.interner.next_type_variable_id(); + let new_generic = TypeVariable::unbound(new_generic_id); + generics.push(new_generic.clone()); + + let name = format!("impl {trait_path}"); + let generic_type = Type::NamedGeneric(new_generic, Rc::new(name)); + let trait_bound = TraitBound { trait_path, trait_id: None, trait_generics }; + + if let Some(new_constraint) = self.resolve_trait_bound(&trait_bound, generic_type.clone()) { + trait_constraints.push(new_constraint); } - self.add_generics(&impl_trait_generics.into_iter().collect()); + + generic_type } /// Add the given generics to scope. @@ -491,11 +472,14 @@ impl<'context> Elaborator<'context> { constraint: &UnresolvedTraitConstraint, ) -> Option { let typ = self.resolve_type(constraint.typ.clone()); - let trait_generics = - vecmap(&constraint.trait_bound.trait_generics, |typ| self.resolve_type(typ.clone())); + self.resolve_trait_bound(&constraint.trait_bound, typ) + } + + fn resolve_trait_bound(&mut self, bound: &TraitBound, typ: Type) -> Option { + let trait_generics = vecmap(&bound.trait_generics, |typ| self.resolve_type(typ.clone())); - let span = constraint.trait_bound.trait_path.span(); - let the_trait = self.lookup_trait_or_error(constraint.trait_bound.trait_path.clone())?; + let span = bound.trait_path.span(); + let the_trait = self.lookup_trait_or_error(bound.trait_path.clone())?; let trait_id = the_trait.id; let expected_generics = the_trait.generics.len(); @@ -561,6 +545,8 @@ impl<'context> Elaborator<'context> { self.add_generics(&func.def.generics); + let mut trait_constraints = self.resolve_trait_constraints(&func.def.where_clause); + let mut generics = vecmap(&self.generics, |(_, typevar, _)| typevar.clone()); let mut parameters = Vec::new(); let mut parameter_types = Vec::new(); @@ -575,7 +561,14 @@ impl<'context> Elaborator<'context> { } let type_span = typ.span.unwrap_or_else(|| pattern.span()); - let typ = self.resolve_type_inner(typ, &mut generics); + + let typ = match typ.typ { + UnresolvedTypeData::TraitAsType(path, args) => { + self.desugar_impl_trait_arg(path, args, &mut generics, &mut trait_constraints) + } + _ => self.resolve_type_inner(typ), + }; + self.check_if_type_is_valid_for_program_input( &typ, is_entry_point, @@ -660,7 +653,7 @@ impl<'context> Elaborator<'context> { return_type: func.def.return_type.clone(), return_visibility: func.def.return_visibility, has_body: !func.def.body.is_empty(), - trait_constraints: self.resolve_trait_constraints(&func.def.where_clause), + trait_constraints, is_entry_point, is_trait_function, has_inline_attribute, @@ -858,6 +851,12 @@ impl<'context> Elaborator<'context> { self.generics = trait_impl.resolved_generics; self.current_trait_impl = trait_impl.impl_id; + for (module, function, _) in &trait_impl.methods.functions { + self.local_module = *module; + let errors = check_trait_impl_method_matches_declaration(self.interner, *function); + self.errors.extend(errors.into_iter().map(|error| (error.into(), self.file))); + } + self.elaborate_functions(trait_impl.methods); self.self_type = None; @@ -884,12 +883,15 @@ impl<'context> Elaborator<'context> { fn collect_trait_impl(&mut self, trait_impl: &mut UnresolvedTraitImpl) { self.local_module = trait_impl.module_id; self.file = trait_impl.file_id; + self.current_trait_impl = trait_impl.impl_id; + trait_impl.trait_id = self.resolve_trait_by_path(trait_impl.trait_path.clone()); let self_type = trait_impl.methods.self_type.clone(); let self_type = self_type.expect("Expected struct type to be set before collect_trait_impl"); + self.self_type = Some(self_type.clone()); let self_type_span = trait_impl.object_type.span; if matches!(self_type, Type::MutableReference(_)) { @@ -899,10 +901,10 @@ impl<'context> Elaborator<'context> { assert!(trait_impl.trait_id.is_some()); if let Some(trait_id) = trait_impl.trait_id { + self.generics = trait_impl.resolved_generics.clone(); self.collect_trait_impl_methods(trait_id, trait_impl); let span = trait_impl.object_type.span.expect("All trait self types should have spans"); - self.generics = trait_impl.resolved_generics.clone(); self.declare_methods_on_struct(true, &mut trait_impl.methods, span); let methods = trait_impl.methods.function_ids(); @@ -952,6 +954,8 @@ impl<'context> Elaborator<'context> { } self.generics.clear(); + self.current_trait_impl = None; + self.self_type = None; } fn get_module_mut(&mut self, module: ModuleId) -> &mut ModuleData { @@ -1066,6 +1070,7 @@ impl<'context> Elaborator<'context> { let module = self.module_id(); let location = Location::new(default_impl.def.span, trait_impl.file_id); self.interner.push_function(func_id, &default_impl.def, module, location); + self.define_function_meta(&mut default_impl_clone, func_id, false); func_ids_in_trait.insert(func_id); ordered_methods.push(( method.default_impl_module_id, @@ -1147,6 +1152,7 @@ impl<'context> Elaborator<'context> { self.current_item = Some(DependencyId::Alias(alias_id)); let typ = self.resolve_type(alias.type_alias_def.typ); self.interner.set_type_alias(alias_id, typ, generics); + self.generics.clear(); } fn collect_struct_definitions(&mut self, structs: BTreeMap) { @@ -1211,8 +1217,6 @@ impl<'context> Elaborator<'context> { let global_id = global.global_id; self.current_item = Some(DependencyId::Global(global_id)); - - let definition_kind = DefinitionKind::Global(global_id); let let_stmt = global.stmt_def; if !self.in_contract @@ -1227,11 +1231,12 @@ impl<'context> Elaborator<'context> { self.push_err(ResolverError::MutableGlobal { span }); } - let (let_statement, _typ) = self.elaborate_let(let_stmt); + self.elaborate_global_let(let_stmt, global_id); - let statement_id = self.interner.get_global(global_id).let_statement; - self.interner.get_global_definition_mut(global_id).kind = definition_kind.clone(); - self.interner.replace_statement(statement_id, let_statement); + // Avoid defaulting the types of globals here since they may be used in any function. + // Otherwise we may prematurely default to a Field inside the next function if this + // global was unused there, even if it is consistently used as a u8 everywhere else. + self.type_variables.clear(); } fn define_function_metas( diff --git a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/patterns.rs b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/patterns.rs index b6d6cb2cf2d..12b0b7b5e64 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/patterns.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/patterns.rs @@ -79,7 +79,7 @@ impl<'context> Elaborator<'context> { HirPattern::Mutable(Box::new(pattern), location) } Pattern::Tuple(fields, span) => { - let field_types = match expected_type { + let field_types = match expected_type.follow_bindings() { Type::Tuple(fields) => fields, Type::Error => Vec::new(), expected_type => { 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 a7a2df4041e..40de9a3983a 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/statements.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/statements.rs @@ -15,7 +15,7 @@ use crate::{ macros_api::{ ForLoopStatement, ForRange, HirStatement, LetStatement, Statement, StatementKind, }, - node_interner::{DefinitionId, DefinitionKind, StmtId}, + node_interner::{DefinitionId, DefinitionKind, GlobalId, StmtId}, Type, }; @@ -24,7 +24,7 @@ use super::Elaborator; impl<'context> Elaborator<'context> { fn elaborate_statement_value(&mut self, statement: Statement) -> (HirStatement, Type) { match statement.kind { - StatementKind::Let(let_stmt) => self.elaborate_let(let_stmt), + StatementKind::Let(let_stmt) => self.elaborate_local_let(let_stmt), StatementKind::Constrain(constrain) => self.elaborate_constrain(constrain), StatementKind::Assign(assign) => self.elaborate_assign(assign), StatementKind::For(for_stmt) => self.elaborate_for(for_stmt), @@ -51,7 +51,35 @@ impl<'context> Elaborator<'context> { (id, typ) } - pub(super) fn elaborate_let(&mut self, let_stmt: LetStatement) -> (HirStatement, Type) { + pub(super) fn elaborate_local_let(&mut self, let_stmt: LetStatement) -> (HirStatement, Type) { + let (statement, typ, _) = self.elaborate_let(let_stmt); + (statement, typ) + } + + /// Elaborates a global let statement. Compared to the local version, this + /// version fixes up the result to use the given DefinitionId rather than + /// the fresh one defined by the let pattern. + pub(super) fn elaborate_global_let(&mut self, let_stmt: LetStatement, global_id: GlobalId) { + let (let_statement, _typ, new_ids) = self.elaborate_let(let_stmt); + let statement_id = self.interner.get_global(global_id).let_statement; + + // To apply the changes from the fresh id created in elaborate_let to this global + // we need to change the definition kind and update the type. + assert_eq!(new_ids.len(), 1, "Globals should only define 1 value"); + let new_id = new_ids[0].id; + + self.interner.definition_mut(new_id).kind = DefinitionKind::Global(global_id); + + let definition_id = self.interner.get_global(global_id).definition_id; + let definition_type = self.interner.definition_type(new_id); + self.interner.push_definition_type(definition_id, definition_type); + + self.interner.replace_statement(statement_id, let_statement); + } + + /// Elaborate a local or global let statement. In addition to the HirLetStatement and unit + /// type, this also returns each HirIdent defined by this let statement. + fn elaborate_let(&mut self, let_stmt: LetStatement) -> (HirStatement, Type, Vec) { let expr_span = let_stmt.expression.span; let (expression, expr_type) = self.elaborate_expression(let_stmt.expression); let definition = DefinitionKind::Local(Some(expression)); @@ -77,14 +105,18 @@ impl<'context> Elaborator<'context> { expr_type }; - let let_ = HirLetStatement { - pattern: self.elaborate_pattern(let_stmt.pattern, r#type.clone(), definition), - r#type, - expression, - attributes: let_stmt.attributes, - comptime: let_stmt.comptime, - }; - (HirStatement::Let(let_), Type::Unit) + let mut created_ids = Vec::new(); + let pattern = self.elaborate_pattern_and_store_ids( + let_stmt.pattern, + r#type.clone(), + definition, + &mut created_ids, + ); + + let attributes = let_stmt.attributes; + let comptime = let_stmt.comptime; + let let_ = HirLetStatement { pattern, r#type, expression, attributes, comptime }; + (HirStatement::Let(let_), Type::Unit, created_ids) } pub(super) fn elaborate_constrain(&mut self, stmt: ConstrainStatement) -> (HirStatement, Type) { diff --git a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/types.rs b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/types.rs index 059ff857df8..955b4af327a 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/elaborator/types.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/elaborator/types.rs @@ -39,59 +39,52 @@ impl<'context> Elaborator<'context> { /// Translates an UnresolvedType to a Type pub(super) fn resolve_type(&mut self, typ: UnresolvedType) -> Type { let span = typ.span; - let resolved_type = self.resolve_type_inner(typ, &mut vec![]); + let resolved_type = self.resolve_type_inner(typ); if resolved_type.is_nested_slice() { self.push_err(ResolverError::NestedSlices { span: span.unwrap() }); } - resolved_type } /// Translates an UnresolvedType into a Type and appends any /// freshly created TypeVariables created to new_variables. - pub fn resolve_type_inner( - &mut self, - typ: UnresolvedType, - new_variables: &mut Generics, - ) -> Type { + pub fn resolve_type_inner(&mut self, typ: UnresolvedType) -> Type { use crate::ast::UnresolvedTypeData::*; let resolved_type = match typ.typ { FieldElement => Type::FieldElement, Array(size, elem) => { - let elem = Box::new(self.resolve_type_inner(*elem, new_variables)); - let size = self.resolve_array_size(Some(size), new_variables); + let elem = Box::new(self.resolve_type_inner(*elem)); + let size = self.convert_expression_type(size); Type::Array(Box::new(size), elem) } Slice(elem) => { - let elem = Box::new(self.resolve_type_inner(*elem, new_variables)); + let elem = Box::new(self.resolve_type_inner(*elem)); Type::Slice(elem) } Expression(expr) => self.convert_expression_type(expr), Integer(sign, bits) => Type::Integer(sign, bits), Bool => Type::Bool, String(size) => { - let resolved_size = self.resolve_array_size(size, new_variables); + let resolved_size = self.convert_expression_type(size); Type::String(Box::new(resolved_size)) } FormatString(size, fields) => { let resolved_size = self.convert_expression_type(size); - let fields = self.resolve_type_inner(*fields, new_variables); + let fields = self.resolve_type_inner(*fields); Type::FmtString(Box::new(resolved_size), Box::new(fields)) } Code => Type::Code, Unit => Type::Unit, Unspecified => Type::Error, Error => Type::Error, - Named(path, args, _) => self.resolve_named_type(path, args, new_variables), - TraitAsType(path, args) => self.resolve_trait_as_type(path, args, new_variables), + Named(path, args, _) => self.resolve_named_type(path, args), + TraitAsType(path, args) => self.resolve_trait_as_type(path, args), - Tuple(fields) => { - Type::Tuple(vecmap(fields, |field| self.resolve_type_inner(field, new_variables))) - } + Tuple(fields) => Type::Tuple(vecmap(fields, |field| self.resolve_type_inner(field))), Function(args, ret, env) => { - let args = vecmap(args, |arg| self.resolve_type_inner(arg, new_variables)); - let ret = Box::new(self.resolve_type_inner(*ret, new_variables)); + let args = vecmap(args, |arg| self.resolve_type_inner(arg)); + let ret = Box::new(self.resolve_type_inner(*ret)); // expect() here is valid, because the only places we don't have a span are omitted types // e.g. a function without return type implicitly has a spanless UnresolvedType::Unit return type @@ -99,7 +92,7 @@ impl<'context> Elaborator<'context> { let env_span = env.span.expect("Unexpected missing span for closure environment type"); - let env = Box::new(self.resolve_type_inner(*env, new_variables)); + let env = Box::new(self.resolve_type_inner(*env)); match *env { Type::Unit | Type::Tuple(_) | Type::NamedGeneric(_, _) => { @@ -115,9 +108,9 @@ impl<'context> Elaborator<'context> { } } MutableReference(element) => { - Type::MutableReference(Box::new(self.resolve_type_inner(*element, new_variables))) + Type::MutableReference(Box::new(self.resolve_type_inner(*element))) } - Parenthesized(typ) => self.resolve_type_inner(*typ, new_variables), + Parenthesized(typ) => self.resolve_type_inner(*typ), }; if let Type::Struct(_, _) = resolved_type { @@ -136,12 +129,7 @@ impl<'context> Elaborator<'context> { self.generics.iter().find(|(name, _, _)| name.as_ref() == target_name) } - fn resolve_named_type( - &mut self, - path: Path, - args: Vec, - new_variables: &mut Generics, - ) -> Type { + fn resolve_named_type(&mut self, path: Path, args: Vec) -> Type { if args.is_empty() { if let Some(typ) = self.lookup_generic_or_global_type(&path) { return typ; @@ -164,7 +152,7 @@ impl<'context> Elaborator<'context> { } let span = path.span(); - let mut args = vecmap(args, |arg| self.resolve_type_inner(arg, new_variables)); + let mut args = vecmap(args, |arg| self.resolve_type_inner(arg)); if let Some(type_alias) = self.lookup_type_alias(path.clone()) { let type_alias = type_alias.borrow(); @@ -230,13 +218,8 @@ impl<'context> Elaborator<'context> { } } - fn resolve_trait_as_type( - &mut self, - path: Path, - args: Vec, - new_variables: &mut Generics, - ) -> Type { - let args = vecmap(args, |arg| self.resolve_type_inner(arg, new_variables)); + fn resolve_trait_as_type(&mut self, path: Path, args: Vec) -> Type { + let args = vecmap(args, |arg| self.resolve_type_inner(arg)); if let Some(t) = self.lookup_trait_or_error(path) { Type::TraitAsType(t.id, Rc::new(t.name.to_string()), args) @@ -286,26 +269,6 @@ impl<'context> Elaborator<'context> { } } - fn resolve_array_size( - &mut self, - length: Option, - new_variables: &mut Generics, - ) -> Type { - match length { - None => { - let id = self.interner.next_type_variable_id(); - let typevar = TypeVariable::unbound(id); - new_variables.push(typevar.clone()); - - // 'Named'Generic is a bit of a misnomer here, we want a type variable that - // wont be bound over but this one has no name since we do not currently - // require users to explicitly be generic over array lengths. - Type::NamedGeneric(typevar, Rc::new("".into())) - } - Some(length) => self.convert_expression_type(length), - } - } - pub(super) fn convert_expression_type(&mut self, length: UnresolvedTypeExpression) -> Type { match length { UnresolvedTypeExpression::Variable(path) => { @@ -622,7 +585,7 @@ impl<'context> Elaborator<'context> { pub(super) fn resolve_inferred_type(&mut self, typ: UnresolvedType) -> Type { match &typ.typ { UnresolvedTypeData::Unspecified => self.interner.next_type_variable(), - _ => self.resolve_type_inner(typ, &mut vec![]), + _ => self.resolve_type(typ), } } diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/hir_to_ast.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/hir_to_ast.rs deleted file mode 100644 index e0fdd91adb4..00000000000 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/hir_to_ast.rs +++ /dev/null @@ -1,369 +0,0 @@ -use iter_extended::vecmap; -use noirc_errors::{Span, Spanned}; - -use crate::ast::{ - ArrayLiteral, AssignStatement, BlockExpression, CallExpression, CastExpression, ConstrainKind, - ConstructorExpression, ExpressionKind, ForLoopStatement, ForRange, Ident, IfExpression, - IndexExpression, InfixExpression, LValue, Lambda, LetStatement, Literal, - MemberAccessExpression, MethodCallExpression, Path, Pattern, PrefixExpression, UnresolvedType, - UnresolvedTypeData, UnresolvedTypeExpression, -}; -use crate::ast::{ConstrainStatement, Expression, Statement, StatementKind}; -use crate::hir_def::expr::{HirArrayLiteral, HirBlockExpression, HirExpression, HirIdent}; -use crate::hir_def::stmt::{HirLValue, HirPattern, HirStatement}; -use crate::hir_def::types::Type; -use crate::macros_api::HirLiteral; -use crate::node_interner::{ExprId, NodeInterner, StmtId}; - -// TODO: -// - Full path for idents & types -// - Assert/AssertEq information lost -// - The type name span is lost in constructor patterns & expressions -// - All type spans are lost -// - Type::TypeVariable has no equivalent in the Ast - -impl StmtId { - #[allow(unused)] - fn to_ast(self, interner: &NodeInterner) -> Statement { - let statement = interner.statement(&self); - let span = interner.statement_span(self); - - let kind = match statement { - HirStatement::Let(let_stmt) => { - let pattern = let_stmt.pattern.into_ast(interner); - let r#type = interner.id_type(let_stmt.expression).to_ast(); - let expression = let_stmt.expression.to_ast(interner); - StatementKind::Let(LetStatement { - pattern, - r#type, - expression, - comptime: false, - attributes: Vec::new(), - }) - } - HirStatement::Constrain(constrain) => { - let expr = constrain.0.to_ast(interner); - let message = constrain.2.map(|message| message.to_ast(interner)); - - // TODO: Find difference in usage between Assert & AssertEq - StatementKind::Constrain(ConstrainStatement(expr, message, ConstrainKind::Assert)) - } - HirStatement::Assign(assign) => StatementKind::Assign(AssignStatement { - lvalue: assign.lvalue.into_ast(interner), - expression: assign.expression.to_ast(interner), - }), - HirStatement::For(for_stmt) => StatementKind::For(ForLoopStatement { - identifier: for_stmt.identifier.to_ast(interner), - range: ForRange::Range( - for_stmt.start_range.to_ast(interner), - for_stmt.end_range.to_ast(interner), - ), - block: for_stmt.block.to_ast(interner), - span, - }), - HirStatement::Break => StatementKind::Break, - HirStatement::Continue => StatementKind::Continue, - HirStatement::Expression(expr) => StatementKind::Expression(expr.to_ast(interner)), - HirStatement::Semi(expr) => StatementKind::Semi(expr.to_ast(interner)), - HirStatement::Error => StatementKind::Error, - HirStatement::Comptime(statement) => { - StatementKind::Comptime(Box::new(statement.to_ast(interner))) - } - }; - - Statement { kind, span } - } -} - -impl ExprId { - #[allow(unused)] - fn to_ast(self, interner: &NodeInterner) -> Expression { - let expression = interner.expression(&self); - let span = interner.expr_span(&self); - - let kind = match expression { - HirExpression::Ident(ident, generics) => { - let path = Path::from_ident(ident.to_ast(interner)); - ExpressionKind::Variable( - path, - generics.map(|option| option.iter().map(|generic| generic.to_ast()).collect()), - ) - } - HirExpression::Literal(HirLiteral::Array(array)) => { - let array = array.into_ast(interner, span); - ExpressionKind::Literal(Literal::Array(array)) - } - HirExpression::Literal(HirLiteral::Slice(array)) => { - let array = array.into_ast(interner, span); - ExpressionKind::Literal(Literal::Slice(array)) - } - HirExpression::Literal(HirLiteral::Bool(value)) => { - ExpressionKind::Literal(Literal::Bool(value)) - } - HirExpression::Literal(HirLiteral::Integer(value, sign)) => { - ExpressionKind::Literal(Literal::Integer(value, sign)) - } - HirExpression::Literal(HirLiteral::Str(string)) => { - ExpressionKind::Literal(Literal::Str(string)) - } - HirExpression::Literal(HirLiteral::FmtStr(string, _exprs)) => { - // TODO: Is throwing away the exprs here valid? - ExpressionKind::Literal(Literal::FmtStr(string)) - } - HirExpression::Literal(HirLiteral::Unit) => ExpressionKind::Literal(Literal::Unit), - HirExpression::Block(expr) => ExpressionKind::Block(expr.into_ast(interner)), - HirExpression::Prefix(prefix) => ExpressionKind::Prefix(Box::new(PrefixExpression { - operator: prefix.operator, - rhs: prefix.rhs.to_ast(interner), - })), - HirExpression::Infix(infix) => ExpressionKind::Infix(Box::new(InfixExpression { - lhs: infix.lhs.to_ast(interner), - operator: Spanned::from(infix.operator.location.span, infix.operator.kind), - rhs: infix.rhs.to_ast(interner), - })), - HirExpression::Index(index) => ExpressionKind::Index(Box::new(IndexExpression { - collection: index.collection.to_ast(interner), - index: index.index.to_ast(interner), - })), - HirExpression::Constructor(constructor) => { - let type_name = constructor.r#type.borrow().name.to_string(); - let type_name = Path::from_single(type_name, span); - let fields = - vecmap(constructor.fields, |(name, expr)| (name, expr.to_ast(interner))); - - ExpressionKind::Constructor(Box::new(ConstructorExpression { type_name, fields })) - } - HirExpression::MemberAccess(access) => { - ExpressionKind::MemberAccess(Box::new(MemberAccessExpression { - lhs: access.lhs.to_ast(interner), - rhs: access.rhs, - })) - } - HirExpression::Call(call) => { - let func = Box::new(call.func.to_ast(interner)); - let arguments = vecmap(call.arguments, |arg| arg.to_ast(interner)); - ExpressionKind::Call(Box::new(CallExpression { func, arguments })) - } - HirExpression::MethodCall(method_call) => { - ExpressionKind::MethodCall(Box::new(MethodCallExpression { - object: method_call.object.to_ast(interner), - method_name: method_call.method, - arguments: vecmap(method_call.arguments, |arg| arg.to_ast(interner)), - generics: method_call - .generics - .map(|option| option.iter().map(|generic| generic.to_ast()).collect()), - })) - } - HirExpression::Cast(cast) => { - let lhs = cast.lhs.to_ast(interner); - let r#type = cast.r#type.to_ast(); - ExpressionKind::Cast(Box::new(CastExpression { lhs, r#type })) - } - HirExpression::If(if_expr) => ExpressionKind::If(Box::new(IfExpression { - condition: if_expr.condition.to_ast(interner), - consequence: if_expr.consequence.to_ast(interner), - alternative: if_expr.alternative.map(|expr| expr.to_ast(interner)), - })), - HirExpression::Tuple(fields) => { - ExpressionKind::Tuple(vecmap(fields, |field| field.to_ast(interner))) - } - HirExpression::Lambda(lambda) => { - let parameters = vecmap(lambda.parameters, |(pattern, typ)| { - (pattern.into_ast(interner), typ.to_ast()) - }); - let return_type = lambda.return_type.to_ast(); - let body = lambda.body.to_ast(interner); - ExpressionKind::Lambda(Box::new(Lambda { parameters, return_type, body })) - } - HirExpression::Error => ExpressionKind::Error, - HirExpression::Comptime(block) => ExpressionKind::Comptime(block.into_ast(interner)), - HirExpression::Quote(block) => ExpressionKind::Quote(block), - - // A macro was evaluated here! - HirExpression::Unquote(block) => ExpressionKind::Block(block), - }; - - Expression::new(kind, span) - } -} - -impl HirPattern { - fn into_ast(self, interner: &NodeInterner) -> Pattern { - match self { - HirPattern::Identifier(ident) => Pattern::Identifier(ident.to_ast(interner)), - HirPattern::Mutable(pattern, location) => { - let pattern = Box::new(pattern.into_ast(interner)); - Pattern::Mutable(pattern, location.span, false) - } - HirPattern::Tuple(patterns, location) => { - let patterns = vecmap(patterns, |pattern| pattern.into_ast(interner)); - Pattern::Tuple(patterns, location.span) - } - HirPattern::Struct(typ, patterns, location) => { - let patterns = - vecmap(patterns, |(name, pattern)| (name, pattern.into_ast(interner))); - let name = match typ.follow_bindings() { - Type::Struct(struct_def, _) => { - let struct_def = struct_def.borrow(); - struct_def.name.0.contents.clone() - } - // This pass shouldn't error so if the type isn't a struct we just get a string - // representation of any other type and use that. We're relying on name - // resolution to fail later when this Ast is re-converted to Hir. - other => other.to_string(), - }; - // The name span is lost here - let path = Path::from_single(name, location.span); - Pattern::Struct(path, patterns, location.span) - } - } - } -} - -impl HirIdent { - fn to_ast(&self, interner: &NodeInterner) -> Ident { - let name = interner.definition_name(self.id).to_owned(); - Ident(Spanned::from(self.location.span, name)) - } -} - -impl Type { - fn to_ast(&self) -> UnresolvedType { - let typ = match self { - Type::FieldElement => UnresolvedTypeData::FieldElement, - Type::Array(length, element) => { - let length = length.to_type_expression(); - let element = Box::new(element.to_ast()); - UnresolvedTypeData::Array(length, element) - } - Type::Slice(element) => { - let element = Box::new(element.to_ast()); - UnresolvedTypeData::Slice(element) - } - Type::Integer(sign, bit_size) => UnresolvedTypeData::Integer(*sign, *bit_size), - Type::Bool => UnresolvedTypeData::Bool, - Type::String(length) => { - let length = length.to_type_expression(); - UnresolvedTypeData::String(Some(length)) - } - Type::FmtString(length, element) => { - let length = length.to_type_expression(); - let element = Box::new(element.to_ast()); - UnresolvedTypeData::FormatString(length, element) - } - Type::Unit => UnresolvedTypeData::Unit, - Type::Tuple(fields) => { - let fields = vecmap(fields, |field| field.to_ast()); - UnresolvedTypeData::Tuple(fields) - } - Type::Struct(def, generics) => { - let struct_def = def.borrow(); - let generics = vecmap(generics, |generic| generic.to_ast()); - let name = Path::from_ident(struct_def.name.clone()); - UnresolvedTypeData::Named(name, generics, false) - } - Type::Alias(type_def, generics) => { - // Keep the alias name instead of expanding this in case the - // alias' definition was changed - let type_def = type_def.borrow(); - let generics = vecmap(generics, |generic| generic.to_ast()); - let name = Path::from_ident(type_def.name.clone()); - UnresolvedTypeData::Named(name, generics, false) - } - Type::TypeVariable(_, _) => todo!("Convert Type::TypeVariable Hir -> Ast"), - Type::TraitAsType(_, name, generics) => { - let generics = vecmap(generics, |generic| generic.to_ast()); - let name = Path::from_single(name.as_ref().clone(), Span::default()); - UnresolvedTypeData::TraitAsType(name, generics) - } - Type::NamedGeneric(_, name) => { - let name = Path::from_single(name.as_ref().clone(), Span::default()); - UnresolvedTypeData::TraitAsType(name, Vec::new()) - } - Type::Function(args, ret, env) => { - let args = vecmap(args, |arg| arg.to_ast()); - let ret = Box::new(ret.to_ast()); - let env = Box::new(env.to_ast()); - UnresolvedTypeData::Function(args, ret, env) - } - Type::MutableReference(element) => { - let element = Box::new(element.to_ast()); - UnresolvedTypeData::MutableReference(element) - } - // Type::Forall is only for generic functions which don't store a type - // in their Ast so they don't need to call to_ast for their Forall type. - // Since there is no UnresolvedTypeData equivalent for Type::Forall, we use - // this to ignore this case since it shouldn't be needed anyway. - Type::Forall(_, typ) => return typ.to_ast(), - Type::Constant(_) => panic!("Type::Constant where a type was expected: {self:?}"), - Type::Code => UnresolvedTypeData::Code, - Type::Error => UnresolvedTypeData::Error, - }; - - UnresolvedType { typ, span: None } - } - - fn to_type_expression(&self) -> UnresolvedTypeExpression { - let span = Span::default(); - - match self.follow_bindings() { - Type::Constant(length) => UnresolvedTypeExpression::Constant(length, span), - Type::NamedGeneric(_, name) => { - let path = Path::from_single(name.as_ref().clone(), span); - UnresolvedTypeExpression::Variable(path) - } - // TODO: This should be turned into a proper error. - other => panic!("Cannot represent {other:?} as type expression"), - } - } -} - -impl HirLValue { - fn into_ast(self, interner: &NodeInterner) -> LValue { - match self { - HirLValue::Ident(ident, _) => LValue::Ident(ident.to_ast(interner)), - HirLValue::MemberAccess { object, field_name, field_index: _, typ: _, location } => { - let object = Box::new(object.into_ast(interner)); - LValue::MemberAccess { object, field_name, span: location.span } - } - HirLValue::Index { array, index, typ: _, location } => { - let array = Box::new(array.into_ast(interner)); - let index = index.to_ast(interner); - LValue::Index { array, index, span: location.span } - } - HirLValue::Dereference { lvalue, element_type: _, location } => { - let lvalue = Box::new(lvalue.into_ast(interner)); - LValue::Dereference(lvalue, location.span) - } - } - } -} - -impl HirArrayLiteral { - fn into_ast(self, interner: &NodeInterner, span: Span) -> ArrayLiteral { - match self { - HirArrayLiteral::Standard(elements) => { - ArrayLiteral::Standard(vecmap(elements, |element| element.to_ast(interner))) - } - HirArrayLiteral::Repeated { repeated_element, length } => { - let repeated_element = Box::new(repeated_element.to_ast(interner)); - let length = match length { - Type::Constant(length) => { - let literal = Literal::Integer((length as u128).into(), false); - let kind = ExpressionKind::Literal(literal); - Box::new(Expression::new(kind, span)) - } - other => panic!("Cannot convert non-constant type for repeated array literal from Hir -> Ast: {other:?}"), - }; - ArrayLiteral::Repeated { repeated_element, length } - } - } - } -} - -impl HirBlockExpression { - fn into_ast(self, interner: &NodeInterner) -> BlockExpression { - let statements = vecmap(self.statements, |statement| statement.to_ast(interner)); - BlockExpression { statements } - } -} diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/mod.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/mod.rs index 0c8efae63ee..ab984d2f2be 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/mod.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/comptime/mod.rs @@ -1,5 +1,4 @@ mod errors; -mod hir_to_ast; mod interpreter; mod scan; mod tests; diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs index 5f1ef6477af..5c196324b7d 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs @@ -741,12 +741,10 @@ fn should_check_siblings_for_module(module_path: &Path, parent_path: &Path) -> b } cfg_if::cfg_if! { - if #[cfg(feature = "bn254")] { - pub const CHOSEN_FIELD: &str = "bn254"; - } else if #[cfg(feature = "bls12_381")] { + if #[cfg(feature = "bls12_381")] { pub const CHOSEN_FIELD: &str = "bls12_381"; } else { - compile_error!("please specify a field to compile with"); + pub const CHOSEN_FIELD: &str = "bn254"; } } diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/resolver.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/resolver.rs index fd77312e4f2..35ba964c499 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/resolver.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/resolution/resolver.rs @@ -538,45 +538,43 @@ impl<'a> Resolver<'a> { /// Translates an UnresolvedType into a Type and appends any /// freshly created TypeVariables created to new_variables. - fn resolve_type_inner(&mut self, typ: UnresolvedType, new_variables: &mut Generics) -> Type { + fn resolve_type_inner(&mut self, typ: UnresolvedType) -> Type { use crate::ast::UnresolvedTypeData::*; let resolved_type = match typ.typ { FieldElement => Type::FieldElement, Array(size, elem) => { - let elem = Box::new(self.resolve_type_inner(*elem, new_variables)); - let size = self.resolve_array_size(Some(size), new_variables); + let elem = Box::new(self.resolve_type_inner(*elem)); + let size = self.convert_expression_type(size); Type::Array(Box::new(size), elem) } Slice(elem) => { - let elem = Box::new(self.resolve_type_inner(*elem, new_variables)); + let elem = Box::new(self.resolve_type_inner(*elem)); Type::Slice(elem) } Expression(expr) => self.convert_expression_type(expr), Integer(sign, bits) => Type::Integer(sign, bits), Bool => Type::Bool, String(size) => { - let resolved_size = self.resolve_array_size(size, new_variables); + let resolved_size = self.convert_expression_type(size); Type::String(Box::new(resolved_size)) } FormatString(size, fields) => { let resolved_size = self.convert_expression_type(size); - let fields = self.resolve_type_inner(*fields, new_variables); + let fields = self.resolve_type_inner(*fields); Type::FmtString(Box::new(resolved_size), Box::new(fields)) } Code => Type::Code, Unit => Type::Unit, Unspecified => Type::Error, Error => Type::Error, - Named(path, args, _) => self.resolve_named_type(path, args, new_variables), - TraitAsType(path, args) => self.resolve_trait_as_type(path, args, new_variables), + Named(path, args, _) => self.resolve_named_type(path, args), + TraitAsType(path, args) => self.resolve_trait_as_type(path, args), - Tuple(fields) => { - Type::Tuple(vecmap(fields, |field| self.resolve_type_inner(field, new_variables))) - } + Tuple(fields) => Type::Tuple(vecmap(fields, |field| self.resolve_type_inner(field))), Function(args, ret, env) => { - let args = vecmap(args, |arg| self.resolve_type_inner(arg, new_variables)); - let ret = Box::new(self.resolve_type_inner(*ret, new_variables)); + let args = vecmap(args, |arg| self.resolve_type_inner(arg)); + let ret = Box::new(self.resolve_type_inner(*ret)); // expect() here is valid, because the only places we don't have a span are omitted types // e.g. a function without return type implicitly has a spanless UnresolvedType::Unit return type @@ -584,7 +582,7 @@ impl<'a> Resolver<'a> { let env_span = env.span.expect("Unexpected missing span for closure environment type"); - let env = Box::new(self.resolve_type_inner(*env, new_variables)); + let env = Box::new(self.resolve_type_inner(*env)); match *env { Type::Unit | Type::Tuple(_) | Type::NamedGeneric(_, _) => { @@ -600,9 +598,9 @@ impl<'a> Resolver<'a> { } } MutableReference(element) => { - Type::MutableReference(Box::new(self.resolve_type_inner(*element, new_variables))) + Type::MutableReference(Box::new(self.resolve_type_inner(*element))) } - Parenthesized(typ) => self.resolve_type_inner(*typ, new_variables), + Parenthesized(typ) => self.resolve_type_inner(*typ), }; if let Type::Struct(_, _) = resolved_type { @@ -621,12 +619,7 @@ impl<'a> Resolver<'a> { self.generics.iter().find(|(name, _, _)| name.as_ref() == target_name) } - fn resolve_named_type( - &mut self, - path: Path, - args: Vec, - new_variables: &mut Generics, - ) -> Type { + fn resolve_named_type(&mut self, path: Path, args: Vec) -> Type { if args.is_empty() { if let Some(typ) = self.lookup_generic_or_global_type(&path) { return typ; @@ -649,7 +642,7 @@ impl<'a> Resolver<'a> { } let span = path.span(); - let mut args = vecmap(args, |arg| self.resolve_type_inner(arg, new_variables)); + let mut args = vecmap(args, |arg| self.resolve_type_inner(arg)); if let Some(type_alias) = self.lookup_type_alias(path.clone()) { let type_alias = type_alias.borrow(); @@ -715,13 +708,8 @@ impl<'a> Resolver<'a> { } } - fn resolve_trait_as_type( - &mut self, - path: Path, - args: Vec, - new_variables: &mut Generics, - ) -> Type { - let args = vecmap(args, |arg| self.resolve_type_inner(arg, new_variables)); + fn resolve_trait_as_type(&mut self, path: Path, args: Vec) -> Type { + let args = vecmap(args, |arg| self.resolve_type_inner(arg)); if let Some(t) = self.lookup_trait_or_error(path) { Type::TraitAsType(t.id, Rc::new(t.name.to_string()), args) @@ -774,26 +762,6 @@ impl<'a> Resolver<'a> { } } - fn resolve_array_size( - &mut self, - length: Option, - new_variables: &mut Generics, - ) -> Type { - match length { - None => { - let id = self.interner.next_type_variable_id(); - let typevar = TypeVariable::unbound(id); - new_variables.push(typevar.clone()); - - // 'Named'Generic is a bit of a misnomer here, we want a type variable that - // wont be bound over but this one has no name since we do not currently - // require users to explicitly be generic over array lengths. - Type::NamedGeneric(typevar, Rc::new("".into())) - } - Some(length) => self.convert_expression_type(length), - } - } - fn convert_expression_type(&mut self, length: UnresolvedTypeExpression) -> Type { match length { UnresolvedTypeExpression::Variable(path) => { @@ -846,11 +814,10 @@ impl<'a> Resolver<'a> { /// Translates an UnresolvedType to a Type pub fn resolve_type(&mut self, typ: UnresolvedType) -> Type { let span = typ.span; - let resolved_type = self.resolve_type_inner(typ, &mut vec![]); + let resolved_type = self.resolve_type_inner(typ); if resolved_type.is_nested_slice() { self.errors.push(ResolverError::NestedSlices { span: span.unwrap() }); } - resolved_type } @@ -891,7 +858,7 @@ impl<'a> Resolver<'a> { fn resolve_inferred_type(&mut self, typ: UnresolvedType) -> Type { match &typ.typ { UnresolvedTypeData::Unspecified => self.interner.next_type_variable(), - _ => self.resolve_type_inner(typ, &mut vec![]), + _ => self.resolve_type(typ), } } @@ -1019,7 +986,7 @@ impl<'a> Resolver<'a> { // indicate we should code generate in the same way. Thus, we unify the attributes into one flag here. let has_inline_attribute = has_no_predicates_attribute || should_fold; - let mut generics = vecmap(&self.generics, |(_, typevar, _)| typevar.clone()); + let generics = vecmap(&self.generics, |(_, typevar, _)| typevar.clone()); let mut parameters = vec![]; let mut parameter_types = vec![]; @@ -1032,7 +999,7 @@ impl<'a> Resolver<'a> { } let pattern = self.resolve_pattern(pattern, DefinitionKind::Local(None)); - let typ = self.resolve_type_inner(typ, &mut generics); + let typ = self.resolve_type_inner(typ); parameters.push((pattern, typ.clone(), visibility)); parameter_types.push(typ); diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/type_check/expr.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/type_check/expr.rs index 336c1cedbb6..6504ead178f 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/type_check/expr.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/type_check/expr.rs @@ -2,6 +2,7 @@ use iter_extended::vecmap; use noirc_errors::Span; use crate::ast::{BinaryOpKind, IntegerBitSize, UnaryOp}; +use crate::hir_def::expr::HirCallExpression; use crate::macros_api::Signedness; use crate::{ hir::{resolution::resolver::verify_mutable_reference, type_check::errors::Source}, @@ -176,16 +177,6 @@ impl<'interner> TypeChecker<'interner> { } HirExpression::Index(index_expr) => self.check_index_expression(expr_id, index_expr), HirExpression::Call(call_expr) => { - // Need to setup these flags here as `self` is borrowed mutably to type check the rest of the call expression - // These flags are later used to type check calls to unconstrained functions from constrained functions - let current_func = self.current_function; - let func_mod = current_func.map(|func| self.interner.function_modifiers(&func)); - let is_current_func_constrained = - func_mod.map_or(true, |func_mod| !func_mod.is_unconstrained); - let is_unconstrained_call = self.is_unconstrained_call(&call_expr.func); - - self.check_if_deprecated(&call_expr.func); - let function = self.check_expression(&call_expr.func); let args = vecmap(&call_expr.arguments, |arg| { @@ -193,39 +184,13 @@ impl<'interner> TypeChecker<'interner> { (typ, *arg, self.interner.expr_span(arg)) }); - // Check that we are not passing a mutable reference from a constrained runtime to an unconstrained runtime - if is_current_func_constrained && is_unconstrained_call { - for (typ, _, _) in args.iter() { - if matches!(&typ.follow_bindings(), Type::MutableReference(_)) { - self.errors.push(TypeCheckError::ConstrainedReferenceToUnconstrained { - span: self.interner.expr_span(expr_id), - }); - return Type::Error; - } - } - } - let span = self.interner.expr_span(expr_id); - let return_type = self.bind_function_type(function, args, span); - - // Check that we are not passing a slice from an unconstrained runtime to a constrained runtime - if is_current_func_constrained && is_unconstrained_call { - if return_type.contains_slice() { - self.errors.push(TypeCheckError::UnconstrainedSliceReturnToConstrained { - span: self.interner.expr_span(expr_id), - }); - return Type::Error; - } else if matches!(&return_type.follow_bindings(), Type::MutableReference(_)) { - self.errors.push(TypeCheckError::UnconstrainedReferenceToConstrained { - span: self.interner.expr_span(expr_id), - }); - return Type::Error; - } - }; - - return_type + self.check_call(&call_expr, function, args, span) } HirExpression::MethodCall(mut method_call) => { + let method_call_span = self.interner.expr_span(expr_id); + let object = method_call.object; + let object_span = self.interner.expr_span(&method_call.object); let mut object_type = self.check_expression(&method_call.object).follow_bindings(); let method_name = method_call.method.0.contents.as_str(); match self.lookup_method(&object_type, method_name, expr_id) { @@ -259,19 +224,42 @@ impl<'interner> TypeChecker<'interner> { ); } + // These arguments will be given to the desugared function call. + // Compared to the method arguments, they also contain the object. + let mut function_args = Vec::with_capacity(method_call.arguments.len() + 1); + + function_args.push((object_type.clone(), object, object_span)); + + for arg in method_call.arguments.iter() { + let span = self.interner.expr_span(arg); + let typ = self.check_expression(arg); + function_args.push((typ, *arg, span)); + } + // TODO: update object_type here? - let (_, function_call) = method_call.into_function_call( + let ((function_id, _), function_call) = method_call.into_function_call( &method_ref, object_type, location, self.interner, ); - self.interner.replace_expr(expr_id, HirExpression::Call(function_call)); + let func_type = self.check_expression(&function_id); // Type check the new call now that it has been changed from a method call // to a function call. This way we avoid duplicating code. - self.check_expression(expr_id) + // We call `check_call` rather than `check_expression` directly as we want to avoid + // resolving the object type again once it is part of the arguments. + let typ = self.check_call( + &function_call, + func_type, + function_args, + method_call_span, + ); + + self.interner.replace_expr(expr_id, HirExpression::Call(function_call)); + + typ } None => Type::Error, } @@ -333,6 +321,45 @@ impl<'interner> TypeChecker<'interner> { typ } + fn check_call( + &mut self, + call: &HirCallExpression, + func_type: Type, + args: Vec<(Type, ExprId, Span)>, + span: Span, + ) -> Type { + // Need to setup these flags here as `self` is borrowed mutably to type check the rest of the call expression + // These flags are later used to type check calls to unconstrained functions from constrained functions + let func_mod = self.current_function.map(|func| self.interner.function_modifiers(&func)); + let is_current_func_constrained = + func_mod.map_or(true, |func_mod| !func_mod.is_unconstrained); + + let is_unconstrained_call = self.is_unconstrained_call(&call.func); + self.check_if_deprecated(&call.func); + + // Check that we are not passing a mutable reference from a constrained runtime to an unconstrained runtime + if is_current_func_constrained && is_unconstrained_call { + for (typ, _, _) in args.iter() { + if matches!(&typ.follow_bindings(), Type::MutableReference(_)) { + self.errors.push(TypeCheckError::ConstrainedReferenceToUnconstrained { span }); + } + } + } + + let return_type = self.bind_function_type(func_type, args, span); + + // Check that we are not passing a slice from an unconstrained runtime to a constrained runtime + if is_current_func_constrained && is_unconstrained_call { + if return_type.contains_slice() { + self.errors.push(TypeCheckError::UnconstrainedSliceReturnToConstrained { span }); + } else if matches!(&return_type.follow_bindings(), Type::MutableReference(_)) { + self.errors.push(TypeCheckError::UnconstrainedReferenceToConstrained { span }); + } + }; + + return_type + } + fn check_block(&mut self, block: HirBlockExpression) -> Type { let mut block_type = Type::Unit; @@ -416,9 +443,8 @@ impl<'interner> TypeChecker<'interner> { // Push any trait constraints required by this definition to the context // to be checked later when the type of this variable is further constrained. if let Some(definition) = self.interner.try_definition(ident.id) { - if let DefinitionKind::Function(function) = definition.kind { - let function = self.interner.function_meta(&function); - + if let DefinitionKind::Function(func_id) = definition.kind { + let function = self.interner.function_meta(&func_id); for mut constraint in function.trait_constraints.clone() { constraint.apply_bindings(&bindings); self.trait_constraints.push((constraint, *expr_id)); diff --git a/noir/noir-repo/compiler/noirc_frontend/src/hir/type_check/mod.rs b/noir/noir-repo/compiler/noirc_frontend/src/hir/type_check/mod.rs index de2e575b4e2..65a3186b004 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/hir/type_check/mod.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/hir/type_check/mod.rs @@ -237,7 +237,13 @@ pub(crate) fn check_trait_impl_method_matches_declaration( let impl_ = meta.trait_impl.expect("Trait impl function should have a corresponding trait impl"); - let impl_ = interner.get_trait_implementation(impl_); + + // If the trait implementation is not defined in the interner then there was a previous + // error in resolving the trait path and there is likely no trait for this impl. + let Some(impl_) = interner.try_get_trait_implementation(impl_) else { + return errors; + }; + let impl_ = impl_.borrow(); let trait_info = interner.get_trait(impl_.trait_id); 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 60cc2580bb8..e28a0a64ad0 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/node_interner.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/node_interner.rs @@ -1145,6 +1145,10 @@ impl NodeInterner { } } + pub fn try_get_trait_implementation(&self, id: TraitImplId) -> Option> { + self.trait_implementations.get(&id).cloned() + } + pub fn get_trait_implementation(&self, id: TraitImplId) -> Shared { self.trait_implementations[&id].clone() } diff --git a/noir/noir-repo/compiler/noirc_frontend/src/parser/parser.rs b/noir/noir-repo/compiler/noirc_frontend/src/parser/parser.rs index 890ab795e00..702ea79af9d 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/parser/parser.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/parser/parser.rs @@ -745,9 +745,7 @@ fn bool_type() -> impl NoirParser { fn string_type() -> impl NoirParser { keyword(Keyword::String) - .ignore_then( - type_expression().delimited_by(just(Token::Less), just(Token::Greater)).or_not(), - ) + .ignore_then(type_expression().delimited_by(just(Token::Less), just(Token::Greater))) .map_with_span(|expr, span| UnresolvedTypeData::String(expr).with_span(span)) } diff --git a/noir/noir-repo/compiler/noirc_frontend/src/tests.rs b/noir/noir-repo/compiler/noirc_frontend/src/tests.rs index bd81752c046..99215c8f173 100644 --- a/noir/noir-repo/compiler/noirc_frontend/src/tests.rs +++ b/noir/noir-repo/compiler/noirc_frontend/src/tests.rs @@ -494,7 +494,10 @@ fn check_trait_wrong_parameter_type() { }"; let errors = get_program_errors(src); assert!(!has_parser_error(&errors)); - assert!(errors.len() == 2, "Expected 2 errors, got: {:?}", errors); + + // This is a duplicate error in the name resolver & type checker. + // In the elaborator there is no duplicate and only 1 error is issued + assert!(errors.len() <= 2, "Expected 1 or 2 errors, got: {:?}", errors); for (err, _file_id) in errors { match &err { @@ -593,22 +596,20 @@ fn check_impl_struct_not_trait() { bar: Field, array: [Field; 2], } - + struct Default { x: Field, z: Field, } - // Default is struct not a trait + // Default is a struct not a trait impl Default for Foo { fn default(x: Field, y: Field) -> Self { Self { bar: x, array: [x,y] } } } - fn main() { - } - + fn main() {} "; let errors = get_program_errors(src); assert!(!has_parser_error(&errors)); diff --git a/noir/noir-repo/test_programs/compile_failure/regression_5065_failure/Nargo.toml b/noir/noir-repo/test_programs/compile_failure/regression_5065_failure/Nargo.toml new file mode 100644 index 00000000000..c3dda827d3c --- /dev/null +++ b/noir/noir-repo/test_programs/compile_failure/regression_5065_failure/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "regression_5065_failure" +type = "bin" +authors = [""] +compiler_version = ">=0.30.0" + +[dependencies] \ No newline at end of file diff --git a/noir/noir-repo/test_programs/compile_failure/regression_5065_failure/src/main.nr b/noir/noir-repo/test_programs/compile_failure/regression_5065_failure/src/main.nr new file mode 100644 index 00000000000..ead1754209d --- /dev/null +++ b/noir/noir-repo/test_programs/compile_failure/regression_5065_failure/src/main.nr @@ -0,0 +1,40 @@ +struct Wrapper { + _value: T, +} + +impl Wrapper { + fn new(value: T) -> Self { + Self { _value: value } + } + + fn unwrap(self) -> T { + self._value + } +} + +trait MyTrait { + fn new() -> Self; +} + +struct MyType {} + +impl MyTrait for MyType { + fn new() -> Self { + MyType {} + } +} + +fn foo() -> T where T: MyTrait { + MyTrait::new() +} + +struct BadType {} + +// Check that we get "No matching impl found for `BadType: MyTrait`" +fn concise_regression() -> BadType { + Wrapper::new(foo()).unwrap() +} + +fn main() { + let _ = concise_regression(); +} diff --git a/noir/noir-repo/test_programs/compile_success_empty/regression_5065/Nargo.toml b/noir/noir-repo/test_programs/compile_success_empty/regression_5065/Nargo.toml new file mode 100644 index 00000000000..b1cb9d9ba96 --- /dev/null +++ b/noir/noir-repo/test_programs/compile_success_empty/regression_5065/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "regression_5065" +type = "bin" +authors = [""] +compiler_version = ">=0.30.0" + +[dependencies] \ No newline at end of file diff --git a/noir/noir-repo/test_programs/compile_success_empty/regression_5065/src/main.nr b/noir/noir-repo/test_programs/compile_success_empty/regression_5065/src/main.nr new file mode 100644 index 00000000000..36b0570165f --- /dev/null +++ b/noir/noir-repo/test_programs/compile_success_empty/regression_5065/src/main.nr @@ -0,0 +1,45 @@ +struct Wrapper { + _value: T, +} + +impl Wrapper { + fn new_wrapper(value: T) -> Self { + Self { _value: value } + } + + fn unwrap(self) -> T { + self._value + } +} + +trait MyTrait { + fn new() -> Self; +} + +struct MyType {} + +impl MyTrait for MyType { + fn new() -> Self { + MyType {} + } +} + +fn foo() -> T where T: MyTrait { + MyTrait::new() +} + +// fn verbose_but_compiles() -> MyType { +// let a = Wrapper::new_wrapper(foo()); +// a.unwrap() +// } + +// Check that are able to infer the return type of the call to `foo` +fn concise_regression() -> MyType { + Wrapper::new_wrapper(foo()).unwrap() + // Wrapper::unwrap(Wrapper::new_wrapper(foo())) +} + +fn main() { + // let _ = verbose_but_compiles(); + let _ = concise_regression(); +} diff --git a/noir/noir-repo/test_programs/execution_success/brillig_bit_shifts_runtime/Nargo.toml b/noir/noir-repo/test_programs/execution_success/acir_inside_brillig_recursion/Nargo.toml similarity index 58% rename from noir/noir-repo/test_programs/execution_success/brillig_bit_shifts_runtime/Nargo.toml rename to noir/noir-repo/test_programs/execution_success/acir_inside_brillig_recursion/Nargo.toml index ed8200d8a95..462532bb484 100644 --- a/noir/noir-repo/test_programs/execution_success/brillig_bit_shifts_runtime/Nargo.toml +++ b/noir/noir-repo/test_programs/execution_success/acir_inside_brillig_recursion/Nargo.toml @@ -1,5 +1,5 @@ [package] -name = "brillig_bit_shifts_runtime" +name = "acir_inside_brillig_recursion" type = "bin" authors = [""] diff --git a/noir/noir-repo/test_programs/execution_success/acir_inside_brillig_recursion/Prover.toml b/noir/noir-repo/test_programs/execution_success/acir_inside_brillig_recursion/Prover.toml new file mode 100644 index 00000000000..8b137891791 --- /dev/null +++ b/noir/noir-repo/test_programs/execution_success/acir_inside_brillig_recursion/Prover.toml @@ -0,0 +1 @@ + diff --git a/noir/noir-repo/test_programs/execution_success/acir_inside_brillig_recursion/src/main.nr b/noir/noir-repo/test_programs/execution_success/acir_inside_brillig_recursion/src/main.nr new file mode 100644 index 00000000000..92f8524a771 --- /dev/null +++ b/noir/noir-repo/test_programs/execution_success/acir_inside_brillig_recursion/src/main.nr @@ -0,0 +1,15 @@ +fn main() { + assert_eq(fibonacci(3), fibonacci_hint(3)); +} + +unconstrained fn fibonacci_hint(x: u32) -> u32 { + fibonacci(x) +} + +fn fibonacci(x: u32) -> u32 { + if x <= 1 { + x + } else { + fibonacci(x - 1) + fibonacci(x - 2) + } +} diff --git a/noir/noir-repo/test_programs/execution_success/brillig_array_eq/Prover.toml b/noir/noir-repo/test_programs/execution_success/brillig_array_eq/Prover.toml deleted file mode 100644 index ecfed7de213..00000000000 --- a/noir/noir-repo/test_programs/execution_success/brillig_array_eq/Prover.toml +++ /dev/null @@ -1,2 +0,0 @@ -a = [77,75,108,209,54,16,50,202,155,210,174,185,217,0,170,77,69,217,234,216,10,201,66,51,116,196,81,167,37,77,7,102] -b = [77,75,108,209,54,16,50,202,155,210,174,185,217,0,170,77,69,217,234,216,10,201,66,51,116,196,81,167,37,77,7,102] diff --git a/noir/noir-repo/test_programs/execution_success/brillig_array_eq/src/main.nr b/noir/noir-repo/test_programs/execution_success/brillig_array_eq/src/main.nr deleted file mode 100644 index 90f631dbed8..00000000000 --- a/noir/noir-repo/test_programs/execution_success/brillig_array_eq/src/main.nr +++ /dev/null @@ -1,4 +0,0 @@ -// Simple example of checking where two arrays are equal -unconstrained fn main(a: [Field; 32], b: [Field; 32]) { - assert(a == b); -} diff --git a/noir/noir-repo/test_programs/execution_success/brillig_bit_shifts_runtime/Prover.toml b/noir/noir-repo/test_programs/execution_success/brillig_bit_shifts_runtime/Prover.toml deleted file mode 100644 index 98d8630792e..00000000000 --- a/noir/noir-repo/test_programs/execution_success/brillig_bit_shifts_runtime/Prover.toml +++ /dev/null @@ -1,2 +0,0 @@ -x = 64 -y = 1 \ No newline at end of file diff --git a/noir/noir-repo/test_programs/execution_success/brillig_bit_shifts_runtime/src/main.nr b/noir/noir-repo/test_programs/execution_success/brillig_bit_shifts_runtime/src/main.nr deleted file mode 100644 index cdeaad7514c..00000000000 --- a/noir/noir-repo/test_programs/execution_success/brillig_bit_shifts_runtime/src/main.nr +++ /dev/null @@ -1,20 +0,0 @@ -unconstrained fn main(x: u64, y: u8) { - // runtime shifts on compile-time known values - assert(64 as u32 << y == 128); - assert(64 as u32 >> y == 32); - // runtime shifts on runtime values - assert(x << y == 128); - assert(x >> y == 32); - - // Bit-shift with signed integers - let mut a :i8 = y as i8; - let mut b: i8 = x as i8; - assert(b << 1 == -128); - assert(b >> 2 == 16); - assert(b >> y == 32); - a = -a; - assert(a << 7 == -128); - assert(a << y == -2); - - assert(x >> (x as u8) == 0); -} diff --git a/noir/noir-repo/test_programs/execution_success/brillig_embedded_curve/Nargo.toml b/noir/noir-repo/test_programs/execution_success/brillig_embedded_curve/Nargo.toml deleted file mode 100644 index b92e11d6383..00000000000 --- a/noir/noir-repo/test_programs/execution_success/brillig_embedded_curve/Nargo.toml +++ /dev/null @@ -1,6 +0,0 @@ -[package] -name = "brillig_embedded_curve" -type = "bin" -authors = [""] - -[dependencies] diff --git a/noir/noir-repo/test_programs/execution_success/brillig_embedded_curve/Prover.toml b/noir/noir-repo/test_programs/execution_success/brillig_embedded_curve/Prover.toml deleted file mode 100644 index 7113b9cd038..00000000000 --- a/noir/noir-repo/test_programs/execution_success/brillig_embedded_curve/Prover.toml +++ /dev/null @@ -1,3 +0,0 @@ -priv_key = "1" -pub_x = "0x0000000000000000000000000000000000000000000000000000000000000001" -pub_y = "0x0000000000000002cf135e7506a45d632d270d45f1181294833fc48d823f272c" \ No newline at end of file diff --git a/noir/noir-repo/test_programs/execution_success/brillig_embedded_curve/src/main.nr b/noir/noir-repo/test_programs/execution_success/brillig_embedded_curve/src/main.nr deleted file mode 100644 index 89a699448dc..00000000000 --- a/noir/noir-repo/test_programs/execution_success/brillig_embedded_curve/src/main.nr +++ /dev/null @@ -1,24 +0,0 @@ -use dep::std; - -unconstrained fn main(priv_key: Field, pub_x: pub Field, pub_y: pub Field) { - let g1_y = 17631683881184975370165255887551781615748388533673675138860; - let g1 = std::embedded_curve_ops::EmbeddedCurvePoint { x: 1, y: g1_y, is_infinite: false }; - let scalar = std::embedded_curve_ops::EmbeddedCurveScalar { lo: priv_key, hi: 0 }; - // Test that multi_scalar_mul correctly derives the public key - let res = std::embedded_curve_ops::multi_scalar_mul([g1], [scalar]); - assert(res[0] == pub_x); - assert(res[1] == pub_y); - - // Test that double function calling embedded_curve_add works as expected - let pub_point = std::embedded_curve_ops::EmbeddedCurvePoint { x: pub_x, y: pub_y, is_infinite: false }; - let res = pub_point.double(); - let double = g1.add(g1); - - assert(double.x == res.x); - - // Test calling multi_scalar_mul with multiple points and scalars - let res = std::embedded_curve_ops::multi_scalar_mul([g1, g1], [scalar, scalar]); - - // The results should be double the g1 point because the scalars are 1 and we pass in g1 twice - assert(double.x == res[0]); -} diff --git a/noir/noir-repo/test_programs/execution_success/brillig_schnorr/Prover.toml b/noir/noir-repo/test_programs/execution_success/brillig_schnorr/Prover.toml deleted file mode 100644 index 2faf2018e07..00000000000 --- a/noir/noir-repo/test_programs/execution_success/brillig_schnorr/Prover.toml +++ /dev/null @@ -1,70 +0,0 @@ -message = [0, 1, 2, 3, 4, 5, 6, 7, 8, 9] -message_field = "0x010203040506070809" -pub_key_x = "0x04b260954662e97f00cab9adb773a259097f7a274b83b113532bce27fa3fb96a" -pub_key_y = "0x2fd51571db6c08666b0edfbfbc57d432068bccd0110a39b166ab243da0037197" -signature = [ - 1, - 13, - 119, - 112, - 212, - 39, - 233, - 41, - 84, - 235, - 255, - 93, - 245, - 172, - 186, - 83, - 157, - 253, - 76, - 77, - 33, - 128, - 178, - 15, - 214, - 67, - 105, - 107, - 177, - 234, - 77, - 48, - 27, - 237, - 155, - 84, - 39, - 84, - 247, - 27, - 22, - 8, - 176, - 230, - 24, - 115, - 145, - 220, - 254, - 122, - 135, - 179, - 171, - 4, - 214, - 202, - 64, - 199, - 19, - 84, - 239, - 138, - 124, - 12, -] diff --git a/noir/noir-repo/test_programs/execution_success/brillig_schnorr/src/main.nr b/noir/noir-repo/test_programs/execution_success/brillig_schnorr/src/main.nr deleted file mode 100644 index 03c635b4f6f..00000000000 --- a/noir/noir-repo/test_programs/execution_success/brillig_schnorr/src/main.nr +++ /dev/null @@ -1,25 +0,0 @@ -use dep::std; -// Note: If main has any unsized types, then the verifier will never be able -// to figure out the circuit instance -unconstrained fn main( - message: [u8; 10], - message_field: Field, - pub_key_x: Field, - pub_key_y: Field, - signature: [u8; 64] -) { - // Regression for issue #2421 - // We want to make sure that we can accurately verify a signature whose message is a slice vs. an array - let message_field_bytes = message_field.to_be_bytes(10); - for i in 0..10 { - assert(message[i] == message_field_bytes[i]); - } - // Is there ever a situation where someone would want - // to ensure that a signature was invalid? - // Check that passing a slice as the message is valid - let valid_signature = std::schnorr::verify_signature_slice(pub_key_x, pub_key_y, signature, message_field_bytes); - assert(valid_signature); - // Check that passing an array as the message is valid - let valid_signature = std::schnorr::verify_signature(pub_key_x, pub_key_y, signature, message); - assert(valid_signature); -} diff --git a/noir/noir-repo/test_programs/execution_success/brillig_signed_div/Nargo.toml b/noir/noir-repo/test_programs/execution_success/brillig_signed_div/Nargo.toml deleted file mode 100644 index 4bb9c5ecc1c..00000000000 --- a/noir/noir-repo/test_programs/execution_success/brillig_signed_div/Nargo.toml +++ /dev/null @@ -1,6 +0,0 @@ -[package] -name = "brillig_signed_div" -type = "bin" -authors = [""] - -[dependencies] diff --git a/noir/noir-repo/test_programs/execution_success/brillig_to_be_bytes/Nargo.toml b/noir/noir-repo/test_programs/execution_success/brillig_to_be_bytes/Nargo.toml deleted file mode 100644 index df6c818c90f..00000000000 --- a/noir/noir-repo/test_programs/execution_success/brillig_to_be_bytes/Nargo.toml +++ /dev/null @@ -1,6 +0,0 @@ -[package] -name = "brillig_to_be_bytes" -type = "bin" -authors = [""] - -[dependencies] diff --git a/noir/noir-repo/test_programs/execution_success/brillig_to_be_bytes/Prover.toml b/noir/noir-repo/test_programs/execution_success/brillig_to_be_bytes/Prover.toml deleted file mode 100644 index 07fe857ac7c..00000000000 --- a/noir/noir-repo/test_programs/execution_success/brillig_to_be_bytes/Prover.toml +++ /dev/null @@ -1 +0,0 @@ -x = "2040124" diff --git a/noir/noir-repo/test_programs/execution_success/brillig_to_be_bytes/src/main.nr b/noir/noir-repo/test_programs/execution_success/brillig_to_be_bytes/src/main.nr deleted file mode 100644 index 9d78411f060..00000000000 --- a/noir/noir-repo/test_programs/execution_success/brillig_to_be_bytes/src/main.nr +++ /dev/null @@ -1,12 +0,0 @@ -unconstrained fn main(x: Field) -> pub [u8; 31] { - // The result of this byte array will be big-endian - let byte_array = x.to_be_bytes(31); - let mut bytes = [0; 31]; - for i in 0..31 { - bytes[i] = byte_array[i]; - } - assert(bytes[30] == 60); - assert(bytes[29] == 33); - assert(bytes[28] == 31); - bytes -} diff --git a/noir/noir-repo/test_programs/execution_success/brillig_to_bytes_integration/Nargo.toml b/noir/noir-repo/test_programs/execution_success/brillig_to_bytes_integration/Nargo.toml deleted file mode 100644 index 991f3d1e46c..00000000000 --- a/noir/noir-repo/test_programs/execution_success/brillig_to_bytes_integration/Nargo.toml +++ /dev/null @@ -1,6 +0,0 @@ -[package] -name = "brillig_to_bytes_integration" -type = "bin" -authors = [""] - -[dependencies] diff --git a/noir/noir-repo/test_programs/execution_success/brillig_to_bytes_integration/Prover.toml b/noir/noir-repo/test_programs/execution_success/brillig_to_bytes_integration/Prover.toml deleted file mode 100644 index 23f7acea449..00000000000 --- a/noir/noir-repo/test_programs/execution_success/brillig_to_bytes_integration/Prover.toml +++ /dev/null @@ -1,2 +0,0 @@ -x = "2040124" -_y = "0x2000000000000000000000000000000000000000000000000000000000000000" diff --git a/noir/noir-repo/test_programs/execution_success/brillig_to_bytes_integration/src/main.nr b/noir/noir-repo/test_programs/execution_success/brillig_to_bytes_integration/src/main.nr deleted file mode 100644 index e8e5b9db9ca..00000000000 --- a/noir/noir-repo/test_programs/execution_success/brillig_to_bytes_integration/src/main.nr +++ /dev/null @@ -1,27 +0,0 @@ -use dep::std; - -unconstrained fn main(x: Field, _y: Field) { - // The result of this byte array will be big-endian - let y: Field = 2040124; - let be_byte_array = y.to_be_bytes(31); - // The result of this byte array will be little-endian - let le_byte_array = x.to_le_bytes(31); - - assert(le_byte_array[0] == 60); - assert(le_byte_array[0] == be_byte_array[30]); - assert(le_byte_array[1] == be_byte_array[29]); - assert(le_byte_array[2] == be_byte_array[28]); - - let z = 0 - 1; - let p_bytes = std::field::modulus_le_bytes(); - let z_bytes = z.to_le_bytes(32); - assert(p_bytes[10] == z_bytes[10]); - assert(p_bytes[0] == z_bytes[0] as u8 + 1 as u8); - - let p_bits = std::field::modulus_le_bits(); - let z_bits = z.to_le_bits(std::field::modulus_num_bits() as u32); - assert(z_bits[0] == 0); - assert(p_bits[100] == z_bits[100]); - - _y.to_le_bits(std::field::modulus_num_bits() as u32); -} diff --git a/noir/noir-repo/test_programs/execution_success/brillig_to_le_bytes/Nargo.toml b/noir/noir-repo/test_programs/execution_success/brillig_to_le_bytes/Nargo.toml deleted file mode 100644 index c2ce8ad01b5..00000000000 --- a/noir/noir-repo/test_programs/execution_success/brillig_to_le_bytes/Nargo.toml +++ /dev/null @@ -1,6 +0,0 @@ -[package] -name = "brillig_to_le_bytes" -type = "bin" -authors = [""] - -[dependencies] diff --git a/noir/noir-repo/test_programs/execution_success/brillig_to_le_bytes/Prover.toml b/noir/noir-repo/test_programs/execution_success/brillig_to_le_bytes/Prover.toml deleted file mode 100644 index 07fe857ac7c..00000000000 --- a/noir/noir-repo/test_programs/execution_success/brillig_to_le_bytes/Prover.toml +++ /dev/null @@ -1 +0,0 @@ -x = "2040124" diff --git a/noir/noir-repo/test_programs/execution_success/brillig_to_le_bytes/src/main.nr b/noir/noir-repo/test_programs/execution_success/brillig_to_le_bytes/src/main.nr deleted file mode 100644 index 77d292cf01b..00000000000 --- a/noir/noir-repo/test_programs/execution_success/brillig_to_le_bytes/src/main.nr +++ /dev/null @@ -1,10 +0,0 @@ -unconstrained fn main(x: Field) -> pub [u8; 31] { - // The result of this byte array will be little-endian - let byte_array = x.to_le_bytes(31); - assert(byte_array.len() == 31); - let mut bytes = [0; 31]; - for i in 0..31 { - bytes[i] = byte_array[i]; - } - bytes -} diff --git a/noir/noir-repo/test_programs/execution_success/brillig_top_level/Nargo.toml b/noir/noir-repo/test_programs/execution_success/brillig_top_level/Nargo.toml deleted file mode 100644 index f74a2a82964..00000000000 --- a/noir/noir-repo/test_programs/execution_success/brillig_top_level/Nargo.toml +++ /dev/null @@ -1,6 +0,0 @@ -[package] -name = "brillig_top_level" -type = "bin" -authors = [""] - -[dependencies] diff --git a/noir/noir-repo/test_programs/execution_success/brillig_top_level/Prover.toml b/noir/noir-repo/test_programs/execution_success/brillig_top_level/Prover.toml deleted file mode 100644 index a0150a0e562..00000000000 --- a/noir/noir-repo/test_programs/execution_success/brillig_top_level/Prover.toml +++ /dev/null @@ -1,2 +0,0 @@ -x = "1" -array = ["4", "5", "6"] \ No newline at end of file diff --git a/noir/noir-repo/test_programs/execution_success/brillig_top_level/src/main.nr b/noir/noir-repo/test_programs/execution_success/brillig_top_level/src/main.nr deleted file mode 100644 index 6dfd98b2c3e..00000000000 --- a/noir/noir-repo/test_programs/execution_success/brillig_top_level/src/main.nr +++ /dev/null @@ -1,6 +0,0 @@ -// Tests a very simple program. -// -// The feature being tested is brillig as the entry point. -unconstrained fn main(array: [Field; 3], x: pub Field) -> pub [Field; 2] { - [array[x], array[x + 1]] -} diff --git a/noir/noir-repo/test_programs/execution_success/brillig_to_bits/Nargo.toml b/noir/noir-repo/test_programs/execution_success/empty/Nargo.toml similarity index 67% rename from noir/noir-repo/test_programs/execution_success/brillig_to_bits/Nargo.toml rename to noir/noir-repo/test_programs/execution_success/empty/Nargo.toml index a18b769550d..168911f2b2b 100644 --- a/noir/noir-repo/test_programs/execution_success/brillig_to_bits/Nargo.toml +++ b/noir/noir-repo/test_programs/execution_success/empty/Nargo.toml @@ -1,5 +1,5 @@ [package] -name = "brillig_to_bits" +name = "empty" type = "bin" authors = [""] [dependencies] diff --git a/noir/noir-repo/test_programs/execution_success/empty/src/main.nr b/noir/noir-repo/test_programs/execution_success/empty/src/main.nr new file mode 100644 index 00000000000..f328e4d9d04 --- /dev/null +++ b/noir/noir-repo/test_programs/execution_success/empty/src/main.nr @@ -0,0 +1 @@ +fn main() {} diff --git a/noir/noir-repo/test_programs/execution_success/regression_5045/Nargo.toml b/noir/noir-repo/test_programs/execution_success/regression_5045/Nargo.toml new file mode 100644 index 00000000000..8f56d392fec --- /dev/null +++ b/noir/noir-repo/test_programs/execution_success/regression_5045/Nargo.toml @@ -0,0 +1,7 @@ +[package] +name = "regression_5045" +version = "0.1.0" +type = "bin" +authors = [""] + +[dependencies] \ No newline at end of file diff --git a/noir/noir-repo/test_programs/execution_success/regression_5045/Prover.toml b/noir/noir-repo/test_programs/execution_success/regression_5045/Prover.toml new file mode 100644 index 00000000000..5444a86ec82 --- /dev/null +++ b/noir/noir-repo/test_programs/execution_success/regression_5045/Prover.toml @@ -0,0 +1 @@ +is_active = false \ No newline at end of file diff --git a/noir/noir-repo/test_programs/execution_success/regression_5045/src/main.nr b/noir/noir-repo/test_programs/execution_success/regression_5045/src/main.nr new file mode 100644 index 00000000000..015fb1b2555 --- /dev/null +++ b/noir/noir-repo/test_programs/execution_success/regression_5045/src/main.nr @@ -0,0 +1,20 @@ +use dep::std::embedded_curve_ops::EmbeddedCurvePoint; +use dep::std::embedded_curve_ops::EmbeddedCurveScalar; + +fn main(is_active: bool) { + let a = EmbeddedCurvePoint { + x: 0x1d8eb4378a3bde41e0b6a9a8dcbd21b7ff9c51bdd6ca13ce989abbbf90df3666, + y: 0x06075b63354f2504f9cddba0b94ed0cef35fc88615e69ec1f853b51eb79a24a0, + is_infinite: false + }; + + if is_active { + let bad = EmbeddedCurvePoint { x: 0, y: 5, is_infinite: false }; + let d = bad.double(); + let e = dep::std::embedded_curve_ops::multi_scalar_mul( + [a, bad], + [EmbeddedCurveScalar { lo: 1, hi: 0 }, EmbeddedCurveScalar { lo: 1, hi: 0 }] + ); + assert(e[0] != d.x); + } +} diff --git a/noir/noir-repo/test_programs/execution_success/brillig_schnorr/Nargo.toml b/noir/noir-repo/test_programs/execution_success/signed_cmp/Nargo.toml similarity index 68% rename from noir/noir-repo/test_programs/execution_success/brillig_schnorr/Nargo.toml rename to noir/noir-repo/test_programs/execution_success/signed_cmp/Nargo.toml index 1b598abbf74..642a0924678 100644 --- a/noir/noir-repo/test_programs/execution_success/brillig_schnorr/Nargo.toml +++ b/noir/noir-repo/test_programs/execution_success/signed_cmp/Nargo.toml @@ -1,5 +1,5 @@ [package] -name = "brillig_schnorr" +name = "signed_cmp" type = "bin" authors = [""] diff --git a/noir/noir-repo/test_programs/execution_success/brillig_signed_cmp/Prover.toml b/noir/noir-repo/test_programs/execution_success/signed_cmp/Prover.toml similarity index 100% rename from noir/noir-repo/test_programs/execution_success/brillig_signed_cmp/Prover.toml rename to noir/noir-repo/test_programs/execution_success/signed_cmp/Prover.toml diff --git a/noir/noir-repo/test_programs/execution_success/brillig_signed_cmp/src/main.nr b/noir/noir-repo/test_programs/execution_success/signed_cmp/src/main.nr similarity index 84% rename from noir/noir-repo/test_programs/execution_success/brillig_signed_cmp/src/main.nr rename to noir/noir-repo/test_programs/execution_success/signed_cmp/src/main.nr index 3e3ea0f4b0f..85746ada8f4 100644 --- a/noir/noir-repo/test_programs/execution_success/brillig_signed_cmp/src/main.nr +++ b/noir/noir-repo/test_programs/execution_success/signed_cmp/src/main.nr @@ -1,4 +1,4 @@ -unconstrained fn main(minus_one: i8) { +fn main(minus_one: i8) { assert(minus_one < 0); assert(0 < minus_one as u8); assert(0 > minus_one); diff --git a/noir/noir-repo/test_programs/execution_success/brillig_array_eq/Nargo.toml b/noir/noir-repo/test_programs/execution_success/signed_div/Nargo.toml similarity index 67% rename from noir/noir-repo/test_programs/execution_success/brillig_array_eq/Nargo.toml rename to noir/noir-repo/test_programs/execution_success/signed_div/Nargo.toml index 62ce392f96b..5ce137d8c0c 100644 --- a/noir/noir-repo/test_programs/execution_success/brillig_array_eq/Nargo.toml +++ b/noir/noir-repo/test_programs/execution_success/signed_div/Nargo.toml @@ -1,5 +1,5 @@ [package] -name = "brillig_array_eq" +name = "signed_div" type = "bin" authors = [""] diff --git a/noir/noir-repo/test_programs/execution_success/brillig_signed_div/Prover.toml b/noir/noir-repo/test_programs/execution_success/signed_div/Prover.toml similarity index 100% rename from noir/noir-repo/test_programs/execution_success/brillig_signed_div/Prover.toml rename to noir/noir-repo/test_programs/execution_success/signed_div/Prover.toml diff --git a/noir/noir-repo/test_programs/execution_success/brillig_signed_div/src/main.nr b/noir/noir-repo/test_programs/execution_success/signed_div/src/main.nr similarity index 76% rename from noir/noir-repo/test_programs/execution_success/brillig_signed_div/src/main.nr rename to noir/noir-repo/test_programs/execution_success/signed_div/src/main.nr index bc3f5c3bdc0..04383a459bd 100644 --- a/noir/noir-repo/test_programs/execution_success/brillig_signed_div/src/main.nr +++ b/noir/noir-repo/test_programs/execution_success/signed_div/src/main.nr @@ -4,7 +4,7 @@ struct SignedDivOp { result: i8, } -unconstrained fn main(ops: [SignedDivOp; 15]) { +fn main(ops: [SignedDivOp; 15]) { for i in 0..15 { assert_eq(ops[i].lhs / ops[i].rhs, ops[i].result); } diff --git a/noir/noir-repo/test_programs/execution_success/brillig_wrapping/Nargo.toml b/noir/noir-repo/test_programs/execution_success/to_bits/Nargo.toml similarity index 66% rename from noir/noir-repo/test_programs/execution_success/brillig_wrapping/Nargo.toml rename to noir/noir-repo/test_programs/execution_success/to_bits/Nargo.toml index a52246ba908..ef47f3b4ba1 100644 --- a/noir/noir-repo/test_programs/execution_success/brillig_wrapping/Nargo.toml +++ b/noir/noir-repo/test_programs/execution_success/to_bits/Nargo.toml @@ -1,6 +1,5 @@ [package] -name = "brillig_wrapping" +name = "to_bits" type = "bin" authors = [""] - [dependencies] diff --git a/noir/noir-repo/test_programs/execution_success/brillig_to_bits/src/main.nr b/noir/noir-repo/test_programs/execution_success/to_bits/src/main.nr similarity index 94% rename from noir/noir-repo/test_programs/execution_success/brillig_to_bits/src/main.nr rename to noir/noir-repo/test_programs/execution_success/to_bits/src/main.nr index 7ff3d2467b5..18f65f0bd66 100644 --- a/noir/noir-repo/test_programs/execution_success/brillig_to_bits/src/main.nr +++ b/noir/noir-repo/test_programs/execution_success/to_bits/src/main.nr @@ -1,6 +1,6 @@ use dep::std; -unconstrained fn main() { +fn main() { let field = 1000; let be_bits = field.to_be_bits(16); let le_bits = field.to_le_bits(16); diff --git a/noir/noir-repo/test_programs/execution_success/unconstrained_empty/Nargo.toml b/noir/noir-repo/test_programs/execution_success/unconstrained_empty/Nargo.toml deleted file mode 100644 index 48d0f5938d8..00000000000 --- a/noir/noir-repo/test_programs/execution_success/unconstrained_empty/Nargo.toml +++ /dev/null @@ -1,5 +0,0 @@ -[package] -name = "unconstrained_empty" -type = "bin" -authors = [""] -[dependencies] diff --git a/noir/noir-repo/test_programs/execution_success/unconstrained_empty/src/main.nr b/noir/noir-repo/test_programs/execution_success/unconstrained_empty/src/main.nr deleted file mode 100644 index 5e5fb297236..00000000000 --- a/noir/noir-repo/test_programs/execution_success/unconstrained_empty/src/main.nr +++ /dev/null @@ -1 +0,0 @@ -unconstrained fn main() {} diff --git a/noir/noir-repo/test_programs/execution_success/brillig_signed_cmp/Nargo.toml b/noir/noir-repo/test_programs/execution_success/wrapping_operations/Nargo.toml similarity index 65% rename from noir/noir-repo/test_programs/execution_success/brillig_signed_cmp/Nargo.toml rename to noir/noir-repo/test_programs/execution_success/wrapping_operations/Nargo.toml index 3f485df4a82..3a28b3461ec 100644 --- a/noir/noir-repo/test_programs/execution_success/brillig_signed_cmp/Nargo.toml +++ b/noir/noir-repo/test_programs/execution_success/wrapping_operations/Nargo.toml @@ -1,5 +1,5 @@ [package] -name = "brillig_signed_cmp" +name = "wrapping_operations" type = "bin" authors = [""] diff --git a/noir/noir-repo/test_programs/execution_success/brillig_wrapping/Prover.toml b/noir/noir-repo/test_programs/execution_success/wrapping_operations/Prover.toml similarity index 100% rename from noir/noir-repo/test_programs/execution_success/brillig_wrapping/Prover.toml rename to noir/noir-repo/test_programs/execution_success/wrapping_operations/Prover.toml diff --git a/noir/noir-repo/test_programs/execution_success/brillig_wrapping/src/main.nr b/noir/noir-repo/test_programs/execution_success/wrapping_operations/src/main.nr similarity index 79% rename from noir/noir-repo/test_programs/execution_success/brillig_wrapping/src/main.nr rename to noir/noir-repo/test_programs/execution_success/wrapping_operations/src/main.nr index 4153a466057..85fd65b193c 100644 --- a/noir/noir-repo/test_programs/execution_success/brillig_wrapping/src/main.nr +++ b/noir/noir-repo/test_programs/execution_success/wrapping_operations/src/main.nr @@ -1,6 +1,6 @@ use dep::std; -unconstrained fn main(x: u8, y: u8) { +fn main(x: u8, y: u8) { assert(std::wrapping_sub(x, 1) == y); assert(std::wrapping_add(y, 1) == x); assert(std::wrapping_mul(y, y) == 1); diff --git a/noir/noir-repo/tooling/debugger/ignored-tests.txt b/noir/noir-repo/tooling/debugger/ignored-tests.txt index a6d3c9a3a94..a9193896589 100644 --- a/noir/noir-repo/tooling/debugger/ignored-tests.txt +++ b/noir/noir-repo/tooling/debugger/ignored-tests.txt @@ -1,28 +1,17 @@ -array_dynamic_blackbox_input bigint -bit_shifts_comptime brillig_references brillig_to_bytes_integration debug_logs -double_verify_nested_proof -double_verify_proof -double_verify_proof_recursive -modulus -references -scalar_mul -signed_comparison -to_bytes_integration +fold_after_inlined_calls fold_basic fold_basic_nested_call fold_call_witness_condition -fold_after_inlined_calls -fold_numeric_generic_poseidon -no_predicates_basic -no_predicates_numeric_generic_poseidon -regression_4709 +fold_complex_outputs fold_distinct_return fold_fibonacci -fold_complex_outputs -slice_init_with_complex_type -hashmap -is_unconstrained \ No newline at end of file +fold_numeric_generic_poseidon +is_unconstrained +modulus +references +regression_4709 +to_bytes_integration \ No newline at end of file diff --git a/noir/noir-repo/tooling/nargo_cli/build.rs b/noir/noir-repo/tooling/nargo_cli/build.rs index 74042cf4e40..53c3966cb4c 100644 --- a/noir/noir-repo/tooling/nargo_cli/build.rs +++ b/noir/noir-repo/tooling/nargo_cli/build.rs @@ -38,6 +38,26 @@ fn main() { generate_compile_failure_tests(&mut test_file, &test_dir); } +/// Some tests are explicitly ignored in brillig due to them failing. +/// These should be fixed and removed from this list. +const IGNORED_BRILLIG_TESTS: [&str; 11] = [ + // Takes a very long time to execute as large loops do not get simplified. + &"regression_4709", + // bit sizes for bigint operation doesn't match up. + &"bigint", + // ICE due to looking for function which doesn't exist. + &"fold_after_inlined_calls", + &"fold_basic", + &"fold_basic_nested_call", + &"fold_call_witness_condition", + &"fold_complex_outputs", + &"fold_distinct_return", + &"fold_fibonacci", + &"fold_numeric_generic_poseidon", + // Expected to fail as test asserts on which runtime it is in. + &"is_unconstrained", +]; + fn generate_execution_success_tests(test_file: &mut File, test_data_dir: &Path) { let test_sub_dir = "execution_success"; let test_data_dir = test_data_dir.join(test_sub_dir); @@ -55,6 +75,9 @@ fn generate_execution_success_tests(test_file: &mut File, test_data_dir: &Path) }; let test_dir = &test_dir.path(); + let brillig_ignored = + if IGNORED_BRILLIG_TESTS.contains(&test_name.as_str()) { "\n#[ignore]" } else { "" }; + write!( test_file, r#" @@ -66,6 +89,17 @@ fn execution_success_{test_name}() {{ cmd.arg("--program-dir").arg(test_program_dir); cmd.arg("execute").arg("--force"); + cmd.assert().success(); +}} + +#[test]{brillig_ignored} +fn execution_success_{test_name}_brillig() {{ + let test_program_dir = PathBuf::from("{test_dir}"); + + let mut cmd = Command::cargo_bin("nargo").unwrap(); + cmd.arg("--program-dir").arg(test_program_dir); + cmd.arg("execute").arg("--force").arg("--force-brillig"); + cmd.assert().success(); }} "#, diff --git a/noir/noir-repo/tooling/nargo_cli/tests/stdlib-tests.rs b/noir/noir-repo/tooling/nargo_cli/tests/stdlib-tests.rs index 70a9354f50a..0bb967e7502 100644 --- a/noir/noir-repo/tooling/nargo_cli/tests/stdlib-tests.rs +++ b/noir/noir-repo/tooling/nargo_cli/tests/stdlib-tests.rs @@ -10,7 +10,8 @@ use nargo::{ parse_all, prepare_package, }; -fn run_stdlib_tests(use_elaborator: bool) { +#[test] +fn run_stdlib_tests() { let mut file_manager = file_manager_with_stdlib(&PathBuf::from(".")); file_manager.add_file_with_source_canonical_path(&PathBuf::from("main.nr"), "".to_owned()); let parsed_files = parse_all(&file_manager); @@ -29,7 +30,7 @@ fn run_stdlib_tests(use_elaborator: bool) { let (mut context, dummy_crate_id) = prepare_package(&file_manager, &parsed_files, &dummy_package); - let result = check_crate(&mut context, dummy_crate_id, true, false, use_elaborator); + let result = check_crate(&mut context, dummy_crate_id, true, false, false); report_errors(result, &context.file_manager, true, false) .expect("Error encountered while compiling standard library"); @@ -59,15 +60,3 @@ fn run_stdlib_tests(use_elaborator: bool) { assert!(!test_report.is_empty(), "Could not find any tests within the stdlib"); assert!(test_report.iter().all(|(_, status)| !status.failed())); } - -#[test] -fn stdlib_noir_tests() { - run_stdlib_tests(false) -} - -// Once this no longer panics we can use the elaborator by default and remove the old passes -#[test] -#[should_panic] -fn stdlib_elaborator_tests() { - run_stdlib_tests(true) -} diff --git a/yarn-project/end-to-end/src/benchmarks/bench_tx_size_fees.test.ts b/yarn-project/end-to-end/src/benchmarks/bench_tx_size_fees.test.ts index 0031b69fb03..7ac0ac75212 100644 --- a/yarn-project/end-to-end/src/benchmarks/bench_tx_size_fees.test.ts +++ b/yarn-project/end-to-end/src/benchmarks/bench_tx_size_fees.test.ts @@ -72,7 +72,7 @@ describe('benchmarks/tx_size_fees', () => { // non-rev: 1 nullifiers, overhead; rev: 2 note hashes, 1 nullifier, 1168 B enc note logs, 0 B enc logs,0 B unenc logs, teardown // L2: // non-rev: 0; rev: 0 - 200072320n, + 200062330n, ], [ 'private fee', @@ -81,7 +81,7 @@ describe('benchmarks/tx_size_fees', () => { // non-rev: 3 nullifiers, overhead; rev: 2 note hashes, 1168 B enc note logs, 0 B enc logs, 0 B unenc logs, teardown // L2: // non-rev: 0; rev: 0 - 200036352n, + 200032492n, ], ] as const)( 'sends a tx with a fee with %s payment method', diff --git a/yarn-project/end-to-end/src/e2e_fees/private_payments.test.ts b/yarn-project/end-to-end/src/e2e_fees/private_payments.test.ts index 15769dfe4a3..c977d7a186b 100644 --- a/yarn-project/end-to-end/src/e2e_fees/private_payments.test.ts +++ b/yarn-project/end-to-end/src/e2e_fees/private_payments.test.ts @@ -124,12 +124,12 @@ describe('e2e_fees private_payment', () => { * tx overhead of 512 DA gas * for a total of 21632 DA gas (without gas used during public execution) * public execution uses N gas - * for a total of 200036352 gas + * for a total of 200032492n gas * * The default teardown gas allocation at present is * 100_000_000 for both DA and L2 gas. * - * That produces a grand total of 200036352. + * That produces a grand total of 200032492n. * * This will change because we are presently squashing notes/nullifiers across non/revertible during * private execution, but we shouldn't. @@ -137,7 +137,7 @@ describe('e2e_fees private_payment', () => { * TODO(6583): update this comment properly now that public execution consumes gas */ - expect(tx.transactionFee).toEqual(200036352n); + expect(tx.transactionFee).toEqual(200032492n); await expect(t.getCoinbaseBalance()).resolves.toEqual(InitialSequencerL1Gas + tx.transactionFee!); const [feeAmount, refundAmount] = getFeeAndRefund(tx);