Skip to content

Commit

Permalink
feat: Sync from noir (#10710)
Browse files Browse the repository at this point in the history
Automated pull of development from the
[noir](https://github.com/noir-lang/noir) programming language, a
dependency of Aztec.
BEGIN_COMMIT_OVERRIDE
feat: add `nargo test --format json`
(noir-lang/noir#6796)
chore: Change Id to use a u32
(noir-lang/noir#6807)
END_COMMIT_OVERRIDE

---------

Co-authored-by: TomAFrench <tom@tomfren.ch>
  • Loading branch information
AztecBot and TomAFrench authored Dec 13, 2024
1 parent 9c9a2dc commit d74d0fc
Show file tree
Hide file tree
Showing 11 changed files with 287 additions and 71 deletions.
2 changes: 1 addition & 1 deletion .noir-sync-commit
Original file line number Diff line number Diff line change
@@ -1 +1 @@
b88db67a4fa92f861329105fb732a7b1309620fe
eb975ab28fb056cf92859377c02f2bb1a608eda3
24 changes: 14 additions & 10 deletions noir/noir-repo/compiler/fm/src/file_map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,19 @@ impl FileMap {
pub fn all_file_ids(&self) -> impl Iterator<Item = &FileId> {
self.name_to_id.values()
}

pub fn get_name(&self, file_id: FileId) -> Result<PathString, Error> {
let name = self.files.get(file_id.as_usize())?.name().clone();

// See if we can make the file name a bit shorter/easier to read if it starts with the current directory
if let Some(current_dir) = &self.current_dir {
if let Ok(name_without_prefix) = name.0.strip_prefix(current_dir) {
return Ok(PathString::from_path(name_without_prefix.to_path_buf()));
}
}

Ok(name)
}
}
impl Default for FileMap {
fn default() -> Self {
Expand All @@ -97,16 +110,7 @@ impl<'a> Files<'a> for FileMap {
type Source = &'a str;

fn name(&self, file_id: Self::FileId) -> Result<Self::Name, Error> {
let name = self.files.get(file_id.as_usize())?.name().clone();

// See if we can make the file name a bit shorter/easier to read if it starts with the current directory
if let Some(current_dir) = &self.current_dir {
if let Ok(name_without_prefix) = name.0.strip_prefix(current_dir) {
return Ok(PathString::from_path(name_without_prefix.to_path_buf()));
}
}

Ok(name)
self.get_name(file_id)
}

fn source(&'a self, file_id: Self::FileId) -> Result<Self::Source, Error> {
Expand Down
2 changes: 1 addition & 1 deletion noir/noir-repo/compiler/noirc_errors/src/reporter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,7 @@ fn convert_diagnostic(
diagnostic.with_message(&cd.message).with_labels(secondary_labels).with_notes(notes)
}

fn stack_trace<'files>(
pub fn stack_trace<'files>(
files: &'files impl Files<'files, FileId = fm::FileId>,
call_stack: &[Location],
) -> String {
Expand Down
2 changes: 1 addition & 1 deletion noir/noir-repo/compiler/noirc_evaluator/src/acir/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1880,7 +1880,7 @@ impl<'a> Context<'a> {
// This conversion is for debugging support only, to allow the
// debugging instrumentation code to work. Taking the reference
// of a function in ACIR is useless.
let id = self.acir_context.add_constant(function_id.to_usize());
let id = self.acir_context.add_constant(function_id.to_u32());
AcirValue::Var(id, AcirType::field())
}
Value::ForeignFunction(_) => unimplemented!(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1614,7 +1614,7 @@ impl<'block> BrilligBlock<'block> {

self.brillig_context.const_instruction(
new_variable.extract_single_addr(),
value_id.to_usize().into(),
value_id.to_u32().into(),
);
new_variable
}
Expand Down
33 changes: 17 additions & 16 deletions noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/map.rs
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use std::{
collections::BTreeMap,
hash::Hash,
str::FromStr,
sync::atomic::{AtomicUsize, Ordering},
sync::atomic::{AtomicU32, Ordering},
};
use thiserror::Error;

Expand All @@ -18,22 +18,23 @@ use thiserror::Error;
/// another map where it will likely be invalid.
#[derive(Serialize, Deserialize)]
pub(crate) struct Id<T> {
index: usize,
index: u32,
// If we do not skip this field it will simply serialize as `"_marker":null` which is useless extra data
#[serde(skip)]
_marker: std::marker::PhantomData<T>,
}

impl<T> Id<T> {
/// Constructs a new Id for the given index.
/// This constructor is deliberately private to prevent
/// constructing invalid IDs.
pub(crate) fn new(index: usize) -> Self {
///
/// This is private so that we can guarantee ids created from this function
/// point to valid T values in their external maps.
fn new(index: u32) -> Self {
Self { index, _marker: std::marker::PhantomData }
}

/// Returns the underlying index of this Id.
pub(crate) fn to_usize(self) -> usize {
pub(crate) fn to_u32(self) -> u32 {
self.index
}

Expand All @@ -43,7 +44,7 @@ impl<T> Id<T> {
/// as unlike DenseMap::push and SparseMap::push, the Ids created
/// here are likely invalid for any particularly map.
#[cfg(test)]
pub(crate) fn test_new(index: usize) -> Self {
pub(crate) fn test_new(index: u32) -> Self {
Self::new(index)
}
}
Expand Down Expand Up @@ -187,15 +188,15 @@ impl<T> DenseMap<T> {
/// Adds an element to the map.
/// Returns the identifier/reference to that element.
pub(crate) fn insert(&mut self, element: T) -> Id<T> {
let id = Id::new(self.storage.len());
let id = Id::new(self.storage.len().try_into().unwrap());
self.storage.push(element);
id
}

/// Given the Id of the element being created, adds the element
/// returned by the given function to the map
pub(crate) fn insert_with_id(&mut self, f: impl FnOnce(Id<T>) -> T) -> Id<T> {
let id = Id::new(self.storage.len());
let id = Id::new(self.storage.len().try_into().unwrap());
self.storage.push(f(id));
id
}
Expand All @@ -204,7 +205,7 @@ impl<T> DenseMap<T> {
///
/// The id-element pairs are ordered by the numeric values of the ids.
pub(crate) fn iter(&self) -> impl ExactSizeIterator<Item = (Id<T>, &T)> {
let ids_iter = (0..self.storage.len()).map(|idx| Id::new(idx));
let ids_iter = (0..self.storage.len() as u32).map(|idx| Id::new(idx));
ids_iter.zip(self.storage.iter())
}
}
Expand All @@ -219,13 +220,13 @@ impl<T> std::ops::Index<Id<T>> for DenseMap<T> {
type Output = T;

fn index(&self, id: Id<T>) -> &Self::Output {
&self.storage[id.index]
&self.storage[id.index as usize]
}
}

impl<T> std::ops::IndexMut<Id<T>> for DenseMap<T> {
fn index_mut(&mut self, id: Id<T>) -> &mut Self::Output {
&mut self.storage[id.index]
&mut self.storage[id.index as usize]
}
}

Expand Down Expand Up @@ -253,15 +254,15 @@ impl<T> SparseMap<T> {
/// Adds an element to the map.
/// Returns the identifier/reference to that element.
pub(crate) fn insert(&mut self, element: T) -> Id<T> {
let id = Id::new(self.storage.len());
let id = Id::new(self.storage.len().try_into().unwrap());
self.storage.insert(id, element);
id
}

/// Given the Id of the element being created, adds the element
/// returned by the given function to the map
pub(crate) fn insert_with_id(&mut self, f: impl FnOnce(Id<T>) -> T) -> Id<T> {
let id = Id::new(self.storage.len());
let id = Id::new(self.storage.len().try_into().unwrap());
self.storage.insert(id, f(id));
id
}
Expand Down Expand Up @@ -365,15 +366,15 @@ impl<K: Eq + Hash, V> std::ops::Index<&K> for TwoWayMap<K, V> {
/// This type wraps an AtomicUsize so it can safely be used across threads.
#[derive(Debug, Serialize, Deserialize)]
pub(crate) struct AtomicCounter<T> {
next: AtomicUsize,
next: AtomicU32,
_marker: std::marker::PhantomData<T>,
}

impl<T> AtomicCounter<T> {
/// Create a new counter starting after the given Id.
/// Use AtomicCounter::default() to start at zero.
pub(crate) fn starting_after(id: Id<T>) -> Self {
Self { next: AtomicUsize::new(id.index + 1), _marker: Default::default() }
Self { next: AtomicU32::new(id.index + 1), _marker: Default::default() }
}

/// Return the next fresh id
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ fn create_apply_functions(
}

fn function_id_to_field(function_id: FunctionId) -> FieldElement {
(function_id.to_usize() as u128).into()
(function_id.to_u32() as u128).into()
}

/// Creates an apply function for the given signature and variants
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1146,7 +1146,7 @@ mod tests {

let refs = loop0.find_pre_header_reference_values(function, &loops.cfg).unwrap();
assert_eq!(refs.len(), 1);
assert!(refs.contains(&ValueId::new(2)));
assert!(refs.contains(&ValueId::test_new(2)));

let (loads, stores) = loop0.count_loads_and_stores(function, &refs);
assert_eq!(loads, 1);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,15 +57,15 @@ impl Translator {
// A FunctionBuilder must be created with a main Function, so here wer remove it
// from the parsed SSA to avoid adding it twice later on.
let main_function = parsed_ssa.functions.remove(0);
let main_id = FunctionId::new(0);
let main_id = FunctionId::test_new(0);
let mut builder = FunctionBuilder::new(main_function.external_name.clone(), main_id);
builder.set_runtime(main_function.runtime_type);

// Map function names to their IDs so calls can be resolved
let mut function_id_counter = 1;
let mut functions = HashMap::new();
for function in &parsed_ssa.functions {
let function_id = FunctionId::new(function_id_counter);
let function_id = FunctionId::test_new(function_id_counter);
function_id_counter += 1;

functions.insert(function.internal_name.clone(), function_id);
Expand Down
58 changes: 27 additions & 31 deletions noir/noir-repo/tooling/nargo_cli/src/cli/test_cmd.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use acvm::{BlackBoxFunctionSolver, FieldElement};
use bn254_blackbox_solver::Bn254BlackBoxSolver;
use clap::Args;
use fm::FileManager;
use formatters::{Formatter, PrettyFormatter, TerseFormatter};
use formatters::{Formatter, JsonFormatter, PrettyFormatter, TerseFormatter};
use nargo::{
insert_all_files_for_workspace_into_file_manager, ops::TestStatus, package::Package, parse_all,
prepare_package, workspace::Workspace, PrintOutput,
Expand Down Expand Up @@ -71,13 +71,16 @@ enum Format {
Pretty,
/// Display one character per test
Terse,
/// Output a JSON Lines document
Json,
}

impl Format {
fn formatter(&self) -> Box<dyn Formatter> {
match self {
Format::Pretty => Box::new(PrettyFormatter),
Format::Terse => Box::new(TerseFormatter),
Format::Json => Box::new(JsonFormatter),
}
}
}
Expand All @@ -87,6 +90,7 @@ impl Display for Format {
match self {
Format::Pretty => write!(f, "pretty"),
Format::Terse => write!(f, "terse"),
Format::Json => write!(f, "json"),
}
}
}
Expand Down Expand Up @@ -211,6 +215,12 @@ impl<'a> TestRunner<'a> {
) -> bool {
let mut all_passed = true;

for (package_name, total_test_count) in test_count_per_package {
self.formatter
.package_start_async(package_name, *total_test_count)
.expect("Could not display package start");
}

let (sender, receiver) = mpsc::channel();
let iter = &Mutex::new(tests.into_iter());
thread::scope(|scope| {
Expand All @@ -228,6 +238,10 @@ impl<'a> TestRunner<'a> {
break;
};

self.formatter
.test_start_async(&test.name, &test.package_name)
.expect("Could not display test start");

let time_before_test = std::time::Instant::now();
let (status, output) = match catch_unwind(test.runner) {
Ok((status, output)) => (status, output),
Expand Down Expand Up @@ -255,6 +269,16 @@ impl<'a> TestRunner<'a> {
time_to_run,
};

self.formatter
.test_end_async(
&test_result,
self.file_manager,
self.args.show_output,
self.args.compile_options.deny_warnings,
self.args.compile_options.silence_warnings,
)
.expect("Could not display test start");

if thread_sender.send(test_result).is_err() {
break;
}
Expand All @@ -275,7 +299,7 @@ impl<'a> TestRunner<'a> {
let total_test_count = *total_test_count;

self.formatter
.package_start(package_name, total_test_count)
.package_start_sync(package_name, total_test_count)
.expect("Could not display package start");

// Check if we have buffered test results for this package
Expand Down Expand Up @@ -485,7 +509,7 @@ impl<'a> TestRunner<'a> {
current_test_count: usize,
total_test_count: usize,
) -> std::io::Result<()> {
self.formatter.test_end(
self.formatter.test_end_sync(
test_result,
current_test_count,
total_test_count,
Expand All @@ -496,31 +520,3 @@ impl<'a> TestRunner<'a> {
)
}
}

#[cfg(test)]
mod tests {
use std::io::Write;
use std::{thread, time::Duration};
use termcolor::{ColorChoice, StandardStream};

#[test]
fn test_stderr_lock() {
for i in 0..4 {
thread::spawn(move || {
let mut writer = StandardStream::stderr(ColorChoice::Always);
//let mut writer = writer.lock();

let mut show = |msg| {
thread::sleep(Duration::from_millis(10));
//println!("{i} {msg}");
writeln!(writer, "{i} {msg}").unwrap();
};

show("a");
show("b");
show("c");
});
}
thread::sleep(Duration::from_millis(100));
}
}
Loading

0 comments on commit d74d0fc

Please sign in to comment.