Skip to content

Commit

Permalink
chore(ci): enforce formatting of noir rust code (#4765)
Browse files Browse the repository at this point in the history
Closes #4763
  • Loading branch information
TomAFrench authored Feb 26, 2024
1 parent f3c65ce commit d9a1853
Show file tree
Hide file tree
Showing 5 changed files with 86 additions and 42 deletions.
107 changes: 72 additions & 35 deletions noir/aztec_macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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 {
Expand All @@ -38,11 +38,16 @@ impl MacroProcessor for AztecMacro {
&self,
crate_id: &CrateId,
context: &mut HirContext,
unresolved_traits_impls: &Vec<UnresolvedTraitImpl>,
unresolved_traits_impls: &[UnresolvedTraitImpl],
collected_functions: &mut Vec<UnresolvedFunctions>,
) -> 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(())
}
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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<UnresolvedTraitImpl>,
collected_functions: &mut Vec<UnresolvedFunctions>,
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");
Expand All @@ -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(());
}

Expand All @@ -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);

Expand All @@ -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]
Expand All @@ -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(())
Expand All @@ -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<UnresolvedTraitImpl>,
trait_name: &str
unresolved_traits_impls: &[UnresolvedTraitImpl],
trait_name: &str,
) -> Vec<String> {
let mut struct_typenames: Vec<String> = 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 {
Expand All @@ -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
}
Expand All @@ -1722,11 +1755,11 @@ fn generate_compute_note_hash_and_nullifier(note_types: &Vec<String>) -> NoirFun
}

fn generate_compute_note_hash_and_nullifier_source(note_types: &Vec<String>) -> 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(
Expand All @@ -1737,7 +1770,8 @@ fn generate_compute_note_hash_and_nullifier_source(note_types: &Vec<String>) ->
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.
Expand All @@ -1749,12 +1783,14 @@ fn generate_compute_note_hash_and_nullifier_source(note_types: &Vec<String>) ->
, 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,
Expand All @@ -1765,7 +1801,8 @@ fn generate_compute_note_hash_and_nullifier_source(note_types: &Vec<String>) ->
let note_header = NoteHeader::new(contract_address, nonce, storage_slot);
{}
}}", full_if_statement)
}}",
full_if_statement
)
}
}

}
13 changes: 9 additions & 4 deletions noir/compiler/noirc_frontend/src/hir/def_collector/dc_crate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -258,11 +258,16 @@ impl DefCollector {

// TODO(#4653): generalize this function
for macro_processor in &macro_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);
Expand Down
2 changes: 1 addition & 1 deletion noir/compiler/noirc_frontend/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ pub mod macros_api {
&self,
_crate_id: &CrateId,
_context: &mut HirContext,
_unresolved_traits_impls: &Vec<UnresolvedTraitImpl>,
_unresolved_traits_impls: &[UnresolvedTraitImpl],
_collected_functions: &mut Vec<UnresolvedFunctions>,
) -> Result<(), (MacroError, FileId)>;

Expand Down
2 changes: 1 addition & 1 deletion noir/noirc_macros/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ impl MacroProcessor for AssertMessageMacro {
&self,
_crate_id: &CrateId,
_context: &mut HirContext,
_unresolevd_traits_impls: &Vec<UnresolvedTraitImpl>,
_unresolved_traits_impls: &[UnresolvedTraitImpl],
_collected_functions: &mut Vec<UnresolvedFunctions>,
) -> Result<(), (MacroError, FileId)> {
Ok(())
Expand Down
4 changes: 3 additions & 1 deletion noir/scripts/test_native.sh
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,6 @@ else
export GIT_COMMIT=$(git rev-parse --verify HEAD)
fi

cargo test --workspace --locked --release
cargo fmt --all --check
cargo clippy --workspace --locked --release
cargo test --workspace --locked --release

0 comments on commit d9a1853

Please sign in to comment.