[1 changes] chore: separate tests for execution failures from compila…

…tion failures (noir-lang/noir#4559)

feat: remove curly braces with fmt  (noir-lang/noir#4529)
fix: Make `nargo` the default binary for cargo run (noir-lang/noir#4554)
fix: Evaluate operators in globals in types (noir-lang/noir#4537)
chore: Add more `Hash` impls to stdlib (noir-lang/noir#4470)

noir/noir-repo/aztec_macros/src/transforms/compute_note_hash_and_nullifier.rs

@@ -7,7 +7,7 @@ use noirc_frontend::{
     },
     macros_api::{FileId, HirContext, MacroError},
     node_interner::FuncId,
-    parse_program, FunctionReturnType, NoirFunction, UnresolvedTypeData,
+    parse_program, FunctionReturnType, ItemVisibility, NoirFunction, UnresolvedTypeData,
 };
 
 use crate::utils::hir_utils::fetch_struct_trait_impls;
@@ -113,7 +113,7 @@ pub fn inject_compute_note_hash_and_nullifier(
     context.def_map_mut(crate_id).unwrap()
         .modules_mut()[module_id.0]
         .declare_function(
-            func.name_ident().clone(), func_id
+            func.name_ident().clone(), ItemVisibility::Public, func_id
         ).expect(
             "Failed to declare the autogenerated compute_note_hash_and_nullifier function, likely due to a duplicate definition. See https://github.com/AztecProtocol/aztec-packages/issues/4647."
         );

noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs

@@ -227,9 +227,7 @@ impl<'block> BrilligBlock<'block> {
                         dfg,
                     );
                 }
-                _ => {
-                    todo!("ICE: Param type not supported")
-                }
+                Type::Function => todo!("ICE: Type::Function Param not supported"),
             }
         }
     }
@@ -661,11 +659,21 @@ impl<'block> BrilligBlock<'block> {
                 let rc_register = match self.convert_ssa_value(*value, dfg) {
                     BrilligVariable::BrilligArray(BrilligArray { rc, .. })
                     | BrilligVariable::BrilligVector(BrilligVector { rc, .. }) => rc,
-                    _ => unreachable!("ICE: increment rc on non-array"),
+                    other => unreachable!("ICE: increment rc on non-array: {other:?}"),
                 };
                 self.brillig_context.usize_op_in_place(rc_register, BinaryIntOp::Add, 1);
             }
-            _ => todo!("ICE: Instruction not supported {instruction:?}"),
+            Instruction::DecrementRc { value } => {
+                let rc_register = match self.convert_ssa_value(*value, dfg) {
+                    BrilligVariable::BrilligArray(BrilligArray { rc, .. })
+                    | BrilligVariable::BrilligVector(BrilligVector { rc, .. }) => rc,
+                    other => unreachable!("ICE: decrement rc on non-array: {other:?}"),
+                };
+                self.brillig_context.usize_op_in_place(rc_register, BinaryIntOp::Sub, 1);
+            }
+            Instruction::EnableSideEffects { .. } => {
+                todo!("enable_side_effects not supported by brillig")
+            }
         };
 
         let dead_variables = self

noir/noir-repo/compiler/noirc_evaluator/src/ssa/acir_gen/mod.rs

@@ -503,7 +503,7 @@ impl Context {
             Instruction::Load { .. } => {
                 unreachable!("Expected all load instructions to be removed before acir_gen")
             }
-            Instruction::IncrementRc { .. } => {
+            Instruction::IncrementRc { .. } | Instruction::DecrementRc { .. } => {
                 // Do nothing. Only Brillig needs to worry about reference counted arrays
             }
             Instruction::RangeCheck { value, max_bit_size, assert_message } => {

noir/noir-repo/compiler/noirc_evaluator/src/ssa/function_builder/mod.rs

@@ -384,6 +384,20 @@ impl FunctionBuilder {
     /// within the given value. If the given value is not an array and does not contain
     /// any arrays, this does nothing.
     pub(crate) fn increment_array_reference_count(&mut self, value: ValueId) {
+        self.update_array_reference_count(value, true);
+    }
+
+    /// Insert instructions to decrement the reference count of any array(s) stored
+    /// within the given value. If the given value is not an array and does not contain
+    /// any arrays, this does nothing.
+    pub(crate) fn decrement_array_reference_count(&mut self, value: ValueId) {
+        self.update_array_reference_count(value, false);
+    }
+
+    /// Increment or decrement the given value's reference count if it is an array.
+    /// If it is not an array, this does nothing. Note that inc_rc and dec_rc instructions
+    /// are ignored outside of unconstrained code.
+    pub(crate) fn update_array_reference_count(&mut self, value: ValueId, increment: bool) {
         match self.type_of_value(value) {
             Type::Numeric(_) => (),
             Type::Function => (),
@@ -396,7 +410,12 @@ impl FunctionBuilder {
             typ @ Type::Array(..) | typ @ Type::Slice(..) => {
                 // If there are nested arrays or slices, we wait until ArrayGet
                 // is issued to increment the count of that array.
-                self.insert_instruction(Instruction::IncrementRc { value }, None);
+                let instruction = if increment {
+                    Instruction::IncrementRc { value }
+                } else {
+                    Instruction::DecrementRc { value }
+                };
+                self.insert_instruction(instruction, None);
 
                 // This is a bit odd, but in brillig the inc_rc instruction operates on
                 // a copy of the array's metadata, so we need to re-store a loaded array

noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/instruction.rs

@@ -194,6 +194,13 @@ pub(crate) enum Instruction {
     /// implemented via reference counting. In ACIR code this is done with im::Vector and these
     /// IncrementRc instructions are ignored.
     IncrementRc { value: ValueId },
+
+    /// An instruction to decrement the reference count of a value.
+    ///
+    /// This currently only has an effect in Brillig code where array sharing and copy on write is
+    /// implemented via reference counting. In ACIR code this is done with im::Vector and these
+    /// DecrementRc instructions are ignored.
+    DecrementRc { value: ValueId },
 }
 
 impl Instruction {
@@ -214,6 +221,7 @@ impl Instruction {
             Instruction::Constrain(..)
             | Instruction::Store { .. }
             | Instruction::IncrementRc { .. }
+            | Instruction::DecrementRc { .. }
             | Instruction::RangeCheck { .. }
             | Instruction::EnableSideEffects { .. } => InstructionResultType::None,
             Instruction::Allocate { .. }
@@ -250,6 +258,7 @@ impl Instruction {
             | Load { .. }
             | Store { .. }
             | IncrementRc { .. }
+            | DecrementRc { .. }
             | RangeCheck { .. } => false,
 
             Call { func, .. } => match dfg[*func] {
@@ -285,6 +294,7 @@ impl Instruction {
             | Store { .. }
             | EnableSideEffects { .. }
             | IncrementRc { .. }
+            | DecrementRc { .. }
             | RangeCheck { .. } => true,
 
             // Some `Intrinsic`s have side effects so we must check what kind of `Call` this is.
@@ -353,6 +363,7 @@ impl Instruction {
                 Instruction::ArraySet { array: f(*array), index: f(*index), value: f(*value) }
             }
             Instruction::IncrementRc { value } => Instruction::IncrementRc { value: f(*value) },
+            Instruction::DecrementRc { value } => Instruction::DecrementRc { value: f(*value) },
             Instruction::RangeCheck { value, max_bit_size, assert_message } => {
                 Instruction::RangeCheck {
                     value: f(*value),
@@ -409,7 +420,9 @@ impl Instruction {
             Instruction::EnableSideEffects { condition } => {
                 f(*condition);
             }
-            Instruction::IncrementRc { value } | Instruction::RangeCheck { value, .. } => {
+            Instruction::IncrementRc { value }
+            | Instruction::DecrementRc { value }
+            | Instruction::RangeCheck { value, .. } => {
                 f(*value);
             }
         }
@@ -554,6 +567,7 @@ impl Instruction {
             Instruction::Load { .. } => None,
             Instruction::Store { .. } => None,
             Instruction::IncrementRc { .. } => None,
+            Instruction::DecrementRc { .. } => None,
             Instruction::RangeCheck { value, max_bit_size, .. } => {
                 if let Some(numeric_constant) = dfg.get_numeric_constant(*value) {
                     if numeric_constant.num_bits() < *max_bit_size {

noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/printer.rs

@@ -188,6 +188,9 @@ fn display_instruction_inner(
         Instruction::IncrementRc { value } => {
             writeln!(f, "inc_rc {}", show(*value))
         }
+        Instruction::DecrementRc { value } => {
+            writeln!(f, "dec_rc {}", show(*value))
+        }
         Instruction::RangeCheck { value, max_bit_size, .. } => {
             writeln!(f, "range_check {} to {} bits", show(*value), *max_bit_size,)
         }

noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/die.rs

@@ -44,7 +44,7 @@ fn dead_instruction_elimination(function: &mut Function) {
         context.remove_unused_instructions_in_block(function, *block);
     }
 
-    context.remove_increment_rc_instructions(&mut function.dfg);
+    context.remove_rc_instructions(&mut function.dfg);
 }
 
 /// Per function context for tracking unused values and which instructions to remove.
@@ -53,10 +53,10 @@ struct Context {
     used_values: HashSet<ValueId>,
     instructions_to_remove: HashSet<InstructionId>,
 
-    /// IncrementRc instructions must be revisited after the main DIE pass since
+    /// IncrementRc & DecrementRc instructions must be revisited after the main DIE pass since
     /// they technically contain side-effects but we still want to remove them if their
     /// `value` parameter is not used elsewhere.
-    increment_rc_instructions: Vec<(InstructionId, BasicBlockId)>,
+    rc_instructions: Vec<(InstructionId, BasicBlockId)>,
 }
 
 impl Context {
@@ -85,8 +85,9 @@ impl Context {
             } else {
                 let instruction = &function.dfg[*instruction_id];
 
-                if let Instruction::IncrementRc { .. } = instruction {
-                    self.increment_rc_instructions.push((*instruction_id, block_id));
+                use Instruction::*;
+                if matches!(instruction, IncrementRc { .. } | DecrementRc { .. }) {
+                    self.rc_instructions.push((*instruction_id, block_id));
                 } else {
                     instruction.for_each_value(|value| {
                         self.mark_used_instruction_results(&function.dfg, value);
@@ -145,16 +146,19 @@ impl Context {
         }
     }
 
-    fn remove_increment_rc_instructions(self, dfg: &mut DataFlowGraph) {
-        for (increment_rc, block) in self.increment_rc_instructions {
-            let value = match &dfg[increment_rc] {
+    fn remove_rc_instructions(self, dfg: &mut DataFlowGraph) {
+        for (rc, block) in self.rc_instructions {
+            let value = match &dfg[rc] {
                 Instruction::IncrementRc { value } => *value,
-                other => unreachable!("Expected IncrementRc instruction, found {other:?}"),
+                Instruction::DecrementRc { value } => *value,
+                other => {
+                    unreachable!("Expected IncrementRc or DecrementRc instruction, found {other:?}")
+                }
             };
 
-            // This could be more efficient if we have to remove multiple IncrementRcs in a single block
+            // This could be more efficient if we have to remove multiple instructions in a single block
             if !self.used_values.contains(&value) {
-                dfg[block].instructions_mut().retain(|instruction| *instruction != increment_rc);
+                dfg[block].instructions_mut().retain(|instruction| *instruction != rc);
             }
         }
     }

noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/context.rs

@@ -10,6 +10,7 @@ use noirc_frontend::{BinaryOpKind, Signedness};
 
 use crate::errors::RuntimeError;
 use crate::ssa::function_builder::FunctionBuilder;
+use crate::ssa::ir::basic_block::BasicBlockId;
 use crate::ssa::ir::dfg::DataFlowGraph;
 use crate::ssa::ir::function::FunctionId as IrFunctionId;
 use crate::ssa::ir::function::{Function, RuntimeType};
@@ -1022,6 +1023,36 @@ impl<'a> FunctionContext<'a> {
             }
         }
     }
+
+    /// Increments the reference count of all parameters. Returns the entry block of the function.
+    ///
+    /// This is done on parameters rather than call arguments so that we can optimize out
+    /// paired inc/dec instructions within brillig functions more easily.
+    pub(crate) fn increment_parameter_rcs(&mut self) -> BasicBlockId {
+        let entry = self.builder.current_function.entry_block();
+        let parameters = self.builder.current_function.dfg.block_parameters(entry).to_vec();
+
+        for parameter in parameters {
+            self.builder.increment_array_reference_count(parameter);
+        }
+
+        entry
+    }
+
+    /// Ends a local scope of a function.
+    /// This will issue DecrementRc instructions for any arrays in the given starting scope
+    /// block's parameters. Arrays that are also used in terminator instructions for the scope are
+    /// ignored.
+    pub(crate) fn end_scope(&mut self, scope: BasicBlockId, terminator_args: &[ValueId]) {
+        let mut dropped_parameters =
+            self.builder.current_function.dfg.block_parameters(scope).to_vec();
+
+        dropped_parameters.retain(|parameter| !terminator_args.contains(parameter));
+
+        for parameter in dropped_parameters {
+            self.builder.decrement_array_reference_count(parameter);
+        }
+    }
 }
 
 /// True if the given operator cannot be encoded directly and needs

noir/noir-repo/compiler/noirc_evaluator/src/ssa/ssa_gen/mod.rs

@@ -121,8 +121,11 @@ impl<'a> FunctionContext<'a> {
     /// Codegen a function's body and set its return value to that of its last parameter.
     /// For functions returning nothing, this will be an empty list.
     fn codegen_function_body(&mut self, body: &Expression) -> Result<(), RuntimeError> {
+        let entry_block = self.increment_parameter_rcs();
         let return_value = self.codegen_expression(body)?;
         let results = return_value.into_value_list(self);
+        self.end_scope(entry_block, &results);
+
         self.builder.terminate_with_return(results);
         Ok(())
     }
@@ -595,10 +598,8 @@ impl<'a> FunctionContext<'a> {
             arguments.append(&mut values);
         }
 
-        // If an array is passed as an argument we increase its reference count
-        for argument in &arguments {
-            self.builder.increment_array_reference_count(*argument);
-        }
+        // Don't need to increment array reference counts when passed in as arguments
+        // since it is done within the function to each parameter already.
 
         self.codegen_intrinsic_call_checks(function, &arguments, call.location);
         Ok(self.insert_call(function, arguments, &call.return_type, call.location))

noir/noir-repo/compiler/noirc_frontend/src/hir/def_collector/dc_mod.rs

@@ -10,8 +10,8 @@ use crate::{
     macros_api::MacroProcessor,
     node_interner::{FunctionModifiers, TraitId, TypeAliasId},
     parser::{SortedModule, SortedSubModule},
-    FunctionDefinition, Ident, LetStatement, ModuleDeclaration, NoirFunction, NoirStruct,
-    NoirTrait, NoirTraitImpl, NoirTypeAlias, TraitImplItem, TraitItem, TypeImpl,
+    FunctionDefinition, Ident, ItemVisibility, LetStatement, ModuleDeclaration, NoirFunction,
+    NoirStruct, NoirTrait, NoirTraitImpl, NoirTypeAlias, TraitImplItem, TraitItem, TypeImpl,
 };
 
 use super::{
@@ -232,6 +232,7 @@ impl<'a> ModCollector<'a> {
 
             let name = function.name_ident().clone();
             let func_id = context.def_interner.push_empty_fn();
+            let visibility = function.def.visibility;
 
             // First create dummy function in the DefInterner
             // So that we can get a FuncId
@@ -248,7 +249,7 @@ impl<'a> ModCollector<'a> {
 
             // Add function to scope/ns of the module
             let result = self.def_collector.def_map.modules[self.module_id.0]
-                .declare_function(name, func_id);
+                .declare_function(name, visibility, func_id);
 
             if let Err((first_def, second_def)) = result {
                 let error = DefCollectorErrorKind::Duplicate {
@@ -407,7 +408,7 @@ impl<'a> ModCollector<'a> {
 
                         let modifiers = FunctionModifiers {
                             name: name.to_string(),
-                            visibility: crate::ItemVisibility::Public,
+                            visibility: ItemVisibility::Public,
                             // TODO(Maddiaa): Investigate trait implementations with attributes see: https://github.com/noir-lang/noir/issues/2629
                             attributes: crate::token::Attributes::empty(),
                             is_unconstrained: false,
@@ -419,7 +420,7 @@ impl<'a> ModCollector<'a> {
                             .push_function_definition(func_id, modifiers, trait_id.0, location);
 
                         match self.def_collector.def_map.modules[trait_id.0.local_id.0]
-                            .declare_function(name.clone(), func_id)
+                            .declare_function(name.clone(), ItemVisibility::Public, func_id)
                         {
                             Ok(()) => {
                                 if let Some(body) = body {

noir/noir-repo/compiler/noirc_frontend/src/hir/def_map/item_scope.rs

@@ -19,10 +19,11 @@ impl ItemScope {
     pub fn add_definition(
         &mut self,
         name: Ident,
+        visibility: ItemVisibility,
         mod_def: ModuleDefId,
         trait_id: Option<TraitId>,
     ) -> Result<(), (Ident, Ident)> {
-        self.add_item_to_namespace(name, mod_def, trait_id, false)?;
+        self.add_item_to_namespace(name, visibility, mod_def, trait_id, false)?;
         self.defs.push(mod_def);
         Ok(())
     }
@@ -33,6 +34,7 @@ impl ItemScope {
     pub fn add_item_to_namespace(
         &mut self,
         name: Ident,
+        visibility: ItemVisibility,
         mod_def: ModuleDefId,
         trait_id: Option<TraitId>,
         is_prelude: bool,
@@ -41,6 +43,11 @@ impl ItemScope {
             if let Entry::Occupied(mut o) = map.entry(name.clone()) {
                 let trait_hashmap = o.get_mut();
                 if let Entry::Occupied(mut n) = trait_hashmap.entry(trait_id) {
+                    // Generally we want to reject having two of the same ident in the same namespace.
+                    // The exception to this is when we're explicitly importing something
+                    // which exists in the Noir stdlib prelude.
+                    //
+                    // In this case we ignore the prelude and favour the explicit import.
                     let is_prelude = std::mem::replace(&mut n.get_mut().2, is_prelude);
                     let old_ident = o.key();
 
@@ -50,12 +57,12 @@ impl ItemScope {
                         Err((old_ident.clone(), name))
                     }
                 } else {
-                    trait_hashmap.insert(trait_id, (mod_def, ItemVisibility::Public, is_prelude));
+                    trait_hashmap.insert(trait_id, (mod_def, visibility, is_prelude));
                     Ok(())
                 }
             } else {
                 let mut trait_hashmap = HashMap::new();
-                trait_hashmap.insert(trait_id, (mod_def, ItemVisibility::Public, is_prelude));
+                trait_hashmap.insert(trait_id, (mod_def, visibility, is_prelude));
                 map.insert(name, trait_hashmap);
                 Ok(())
             }