From d9a1853cc0474050f40ef52b196568b711f7eb07 Mon Sep 17 00:00:00 2001 From: Tom French <15848336+TomAFrench@users.noreply.github.com> Date: Mon, 26 Feb 2024 14:50:46 +0000 Subject: [PATCH] chore(ci): enforce formatting of noir rust code (#4765) Closes #4763 --- noir/aztec_macros/src/lib.rs | 107 ++++++++++++------ .../src/hir/def_collector/dc_crate.rs | 13 ++- noir/compiler/noirc_frontend/src/lib.rs | 2 +- noir/noirc_macros/src/lib.rs | 2 +- noir/scripts/test_native.sh | 4 +- 5 files changed, 86 insertions(+), 42 deletions(-) diff --git a/noir/aztec_macros/src/lib.rs b/noir/aztec_macros/src/lib.rs index a0a63d8a9a8..8f70d5b5e86 100644 --- a/noir/aztec_macros/src/lib.rs +++ b/noir/aztec_macros/src/lib.rs @@ -3,8 +3,10 @@ use std::vec; use convert_case::{Case, Casing}; use iter_extended::vecmap; +use noirc_errors::Location; use noirc_frontend::hir::def_collector::dc_crate::{UnresolvedFunctions, UnresolvedTraitImpl}; use noirc_frontend::hir::def_map::{LocalModuleId, ModuleId}; +use noirc_frontend::macros_api::parse_program; use noirc_frontend::macros_api::FieldElement; use noirc_frontend::macros_api::{ BlockExpression, CallExpression, CastExpression, Distinctness, Expression, ExpressionKind, @@ -20,8 +22,6 @@ use noirc_frontend::macros_api::{MacroError, MacroProcessor}; use noirc_frontend::macros_api::{ModuleDefId, NodeInterner, SortedModule, StructId}; use noirc_frontend::node_interner::{FuncId, TraitId, TraitImplId, TraitImplKind}; use noirc_frontend::Lambda; -use noirc_frontend::macros_api::parse_program; -use noirc_errors::Location; pub struct AztecMacro; impl MacroProcessor for AztecMacro { @@ -38,11 +38,16 @@ impl MacroProcessor for AztecMacro { &self, crate_id: &CrateId, context: &mut HirContext, - unresolved_traits_impls: &Vec, + unresolved_traits_impls: &[UnresolvedTraitImpl], collected_functions: &mut Vec, ) -> Result<(), (MacroError, FileId)> { if has_aztec_dependency(crate_id, context) { - inject_compute_note_hash_and_nullifier(crate_id, context, unresolved_traits_impls, collected_functions) + inject_compute_note_hash_and_nullifier( + crate_id, + context, + unresolved_traits_impls, + collected_functions, + ) } else { Ok(()) } @@ -351,7 +356,10 @@ fn check_for_storage_implementation(module: &SortedModule) -> bool { } // Check if "compute_note_hash_and_nullifier(AztecAddress,Field,Field,Field,[Field; N]) -> [Field; 4]" is defined -fn check_for_compute_note_hash_and_nullifier_definition(functions_data: &Vec<(LocalModuleId, FuncId, NoirFunction)>, module_id: LocalModuleId) -> bool { +fn check_for_compute_note_hash_and_nullifier_definition( + functions_data: &[(LocalModuleId, FuncId, NoirFunction)], + module_id: LocalModuleId, +) -> bool { functions_data.iter().filter(|func_data| func_data.0 == module_id).any(|func_data| { func_data.2.def.name.0.contents == "compute_note_hash_and_nullifier" && func_data.2.def.parameters.len() == 5 @@ -1611,18 +1619,21 @@ fn event_signature(event: &StructType) -> String { fn inject_compute_note_hash_and_nullifier( crate_id: &CrateId, context: &mut HirContext, - unresolved_traits_impls: &Vec, - collected_functions: &mut Vec, + unresolved_traits_impls: &[UnresolvedTraitImpl], + collected_functions: &mut [UnresolvedFunctions], ) -> Result<(), (MacroError, FileId)> { // We first fetch modules in this crate which correspond to contracts, along with their file id. - let contract_module_file_ids: Vec<(LocalModuleId, FileId)> = context.def_map(crate_id).expect("ICE: Missing crate in def_map") - .modules().iter() + let contract_module_file_ids: Vec<(LocalModuleId, FileId)> = context + .def_map(crate_id) + .expect("ICE: Missing crate in def_map") + .modules() + .iter() .filter(|(_, module)| module.is_contract) .map(|(idx, module)| (LocalModuleId(idx), module.location.file)) .collect(); // If the current crate does not contain a contract module we simply skip it. - if contract_module_file_ids.len() == 0 { + if contract_module_file_ids.is_empty() { return Ok(()); } else if contract_module_file_ids.len() != 1 { panic!("Found multiple contracts in the same crate"); @@ -1633,7 +1644,9 @@ fn inject_compute_note_hash_and_nullifier( // If compute_note_hash_and_nullifier is already defined by the user, we skip auto-generation in order to provide an // escape hatch for this mechanism. // TODO(#4647): improve this diagnosis and error messaging. - if collected_functions.iter().any(|coll_funcs_data| check_for_compute_note_hash_and_nullifier_definition(&coll_funcs_data.functions, module_id)) { + if collected_functions.iter().any(|coll_funcs_data| { + check_for_compute_note_hash_and_nullifier_definition(&coll_funcs_data.functions, module_id) + }) { return Ok(()); } @@ -1647,7 +1660,7 @@ fn inject_compute_note_hash_and_nullifier( // And inject the newly created function into the contract. - // TODO(#4373): We don't have a reasonable location for the source code of this autogenerated function, so we simply + // TODO(#4373): We don't have a reasonable location for the source code of this autogenerated function, so we simply // pass an empty span. This function should not produce errors anyway so this should not matter. let location = Location::new(Span::empty(0), file_id); @@ -1656,7 +1669,12 @@ fn inject_compute_note_hash_and_nullifier( // on collected but unresolved functions. let func_id = context.def_interner.push_empty_fn(); - context.def_interner.push_function(func_id, &func.def, ModuleId { krate: *crate_id, local_id: module_id }, location); + context.def_interner.push_function( + func_id, + &func.def, + ModuleId { krate: *crate_id, local_id: module_id }, + location, + ); context.def_map_mut(crate_id).unwrap() .modules_mut()[module_id.0] @@ -1666,8 +1684,10 @@ fn inject_compute_note_hash_and_nullifier( "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." ); - collected_functions.iter_mut() - .find(|fns| fns.file_id == file_id).expect("ICE: no functions found in contract file") + collected_functions + .iter_mut() + .find(|fns| fns.file_id == file_id) + .expect("ICE: no functions found in contract file") .push_fn(module_id, func_id, func.clone()); Ok(()) @@ -1676,15 +1696,15 @@ fn inject_compute_note_hash_and_nullifier( // Fetches the name of all structs that implement trait_name, both in the current crate and all of its dependencies. fn fetch_struct_trait_impls( context: &mut HirContext, - unresolved_traits_impls: &Vec, - trait_name: &str + unresolved_traits_impls: &[UnresolvedTraitImpl], + trait_name: &str, ) -> Vec { let mut struct_typenames: Vec = Vec::new(); - // These structs can be declared in either external crates or the current one. External crates that contain + // These structs can be declared in either external crates or the current one. External crates that contain // dependencies have already been processed and resolved, but are available here via the NodeInterner. Note that // crates on which the current crate does not depend on may not have been processed, and will be ignored. - for trait_impl_id in 0..(&context.def_interner.next_trait_impl_id()).0 { + for trait_impl_id in 0..context.def_interner.next_trait_impl_id().0 { let trait_impl = &context.def_interner.get_trait_implementation(TraitImplId(trait_impl_id)); if trait_impl.borrow().ident.0.contents == *trait_name { @@ -1697,13 +1717,26 @@ fn fetch_struct_trait_impls( } // This crate's traits and impls have not yet been resolved, so we look for impls in unresolved_trait_impls. - struct_typenames.extend(unresolved_traits_impls.iter() - .filter(|trait_impl| - trait_impl.trait_path.segments.last().expect("ICE: empty trait_impl path").0.contents == *trait_name) - .filter_map(|trait_impl| match &trait_impl.object_type.typ { - UnresolvedTypeData::Named(path, _, _) => Some(path.segments.last().unwrap().0.contents.clone()), - _ => None, - })); + struct_typenames.extend( + unresolved_traits_impls + .iter() + .filter(|trait_impl| { + trait_impl + .trait_path + .segments + .last() + .expect("ICE: empty trait_impl path") + .0 + .contents + == *trait_name + }) + .filter_map(|trait_impl| match &trait_impl.object_type.typ { + UnresolvedTypeData::Named(path, _, _) => { + Some(path.segments.last().unwrap().0.contents.clone()) + } + _ => None, + }), + ); struct_typenames } @@ -1722,11 +1755,11 @@ fn generate_compute_note_hash_and_nullifier(note_types: &Vec) -> NoirFun } fn generate_compute_note_hash_and_nullifier_source(note_types: &Vec) -> String { - // TODO(#4649): The serialized_note parameter is a fixed-size array, but we don't know what length it should have. + // TODO(#4649): The serialized_note parameter is a fixed-size array, but we don't know what length it should have. // For now we hardcode it to 20, which is the same as MAX_NOTE_FIELDS_LENGTH. - if note_types.len() == 0 { - // TODO(#4520): Even if the contract does not include any notes, other parts of the stack expect for this + if note_types.is_empty() { + // TODO(#4520): Even if the contract does not include any notes, other parts of the stack expect for this // function to exist, so we include a dummy version. We likely should error out here instead. " unconstrained fn compute_note_hash_and_nullifier( @@ -1737,7 +1770,8 @@ fn generate_compute_note_hash_and_nullifier_source(note_types: &Vec) -> serialized_note: [Field; 20] ) -> pub [Field; 4] { [0, 0, 0, 0] - }".to_string() + }" + .to_string() } else { // For contracts that include notes we do a simple if-else chain comparing note_type_id with the different // get_note_type_id of each of the note types. @@ -1749,12 +1783,14 @@ fn generate_compute_note_hash_and_nullifier_source(note_types: &Vec) -> , note_type)).collect(); // TODO(#4520): error out on the else instead of returning a zero array - let full_if_statement = if_statements.join(" else ") + " + let full_if_statement = if_statements.join(" else ") + + " else { [0, 0, 0, 0] }"; - format!(" + format!( + " unconstrained fn compute_note_hash_and_nullifier( contract_address: AztecAddress, nonce: Field, @@ -1765,7 +1801,8 @@ fn generate_compute_note_hash_and_nullifier_source(note_types: &Vec) -> let note_header = NoteHeader::new(contract_address, nonce, storage_slot); {} - }}", full_if_statement) + }}", + full_if_statement + ) + } } - -} \ No newline at end of file diff --git a/noir/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs b/noir/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs index b10ca0f0769..7f36af5b30e 100644 --- a/noir/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs +++ b/noir/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs @@ -258,11 +258,16 @@ impl DefCollector { // TODO(#4653): generalize this function for macro_processor in ¯o_processors { - macro_processor.process_unresolved_traits_impls(&crate_id, context, &def_collector.collected_traits_impls, &mut def_collector.collected_functions).unwrap_or_else( - |(macro_err, file_id)| { + macro_processor + .process_unresolved_traits_impls( + &crate_id, + context, + &def_collector.collected_traits_impls, + &mut def_collector.collected_functions, + ) + .unwrap_or_else(|(macro_err, file_id)| { errors.push((macro_err.into(), file_id)); - }, - ); + }); } inject_prelude(crate_id, context, crate_root, &mut def_collector.collected_imports); diff --git a/noir/compiler/noirc_frontend/src/lib.rs b/noir/compiler/noirc_frontend/src/lib.rs index 55f5fb1ca40..be007929fc4 100644 --- a/noir/compiler/noirc_frontend/src/lib.rs +++ b/noir/compiler/noirc_frontend/src/lib.rs @@ -81,7 +81,7 @@ pub mod macros_api { &self, _crate_id: &CrateId, _context: &mut HirContext, - _unresolved_traits_impls: &Vec, + _unresolved_traits_impls: &[UnresolvedTraitImpl], _collected_functions: &mut Vec, ) -> Result<(), (MacroError, FileId)>; diff --git a/noir/noirc_macros/src/lib.rs b/noir/noirc_macros/src/lib.rs index 55109dbfba0..9a916843200 100644 --- a/noir/noirc_macros/src/lib.rs +++ b/noir/noirc_macros/src/lib.rs @@ -22,7 +22,7 @@ impl MacroProcessor for AssertMessageMacro { &self, _crate_id: &CrateId, _context: &mut HirContext, - _unresolevd_traits_impls: &Vec, + _unresolved_traits_impls: &[UnresolvedTraitImpl], _collected_functions: &mut Vec, ) -> Result<(), (MacroError, FileId)> { Ok(()) diff --git a/noir/scripts/test_native.sh b/noir/scripts/test_native.sh index bc1c47ecf12..9b9aa0ce4d7 100755 --- a/noir/scripts/test_native.sh +++ b/noir/scripts/test_native.sh @@ -12,4 +12,6 @@ else export GIT_COMMIT=$(git rev-parse --verify HEAD) fi -cargo test --workspace --locked --release \ No newline at end of file +cargo fmt --all --check +cargo clippy --workspace --locked --release +cargo test --workspace --locked --release