Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(ci): enforce formatting of noir rust code #4765

Merged
merged 2 commits into from
Feb 26, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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
Loading