diff --git a/.noir-sync-commit b/.noir-sync-commit index c4b6d336e04a..b63a49f65c0d 100644 --- a/.noir-sync-commit +++ b/.noir-sync-commit @@ -1 +1 @@ -b88db67a4fa92f861329105fb732a7b1309620fe +eb975ab28fb056cf92859377c02f2bb1a608eda3 diff --git a/noir/noir-repo/compiler/fm/src/file_map.rs b/noir/noir-repo/compiler/fm/src/file_map.rs index ba552fe51561..857c7460fb9d 100644 --- a/noir/noir-repo/compiler/fm/src/file_map.rs +++ b/noir/noir-repo/compiler/fm/src/file_map.rs @@ -80,6 +80,19 @@ impl FileMap { pub fn all_file_ids(&self) -> impl Iterator { self.name_to_id.values() } + + pub fn get_name(&self, file_id: FileId) -> Result { + 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 { @@ -97,16 +110,7 @@ impl<'a> Files<'a> for FileMap { type Source = &'a str; fn name(&self, file_id: Self::FileId) -> Result { - 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 { diff --git a/noir/noir-repo/compiler/noirc_errors/src/reporter.rs b/noir/noir-repo/compiler/noirc_errors/src/reporter.rs index f029b4e6de8b..e57775d9a7f8 100644 --- a/noir/noir-repo/compiler/noirc_errors/src/reporter.rs +++ b/noir/noir-repo/compiler/noirc_errors/src/reporter.rs @@ -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 { diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/acir/mod.rs b/noir/noir-repo/compiler/noirc_evaluator/src/acir/mod.rs index f83a2095f13a..769d0d80cc4a 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/acir/mod.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/acir/mod.rs @@ -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!( diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs index 56511127da8c..d2bf7e5bdca3 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/brillig/brillig_gen/brillig_block.rs @@ -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 } diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/map.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/map.rs index 23f5380f0303..0fb02f19b14e 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/map.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/ir/map.rs @@ -4,7 +4,7 @@ use std::{ collections::BTreeMap, hash::Hash, str::FromStr, - sync::atomic::{AtomicUsize, Ordering}, + sync::atomic::{AtomicU32, Ordering}, }; use thiserror::Error; @@ -18,7 +18,7 @@ use thiserror::Error; /// another map where it will likely be invalid. #[derive(Serialize, Deserialize)] pub(crate) struct Id { - 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, @@ -26,14 +26,15 @@ pub(crate) struct Id { impl Id { /// 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 } @@ -43,7 +44,7 @@ impl Id { /// 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) } } @@ -187,7 +188,7 @@ impl DenseMap { /// Adds an element to the map. /// Returns the identifier/reference to that element. pub(crate) fn insert(&mut self, element: T) -> Id { - let id = Id::new(self.storage.len()); + let id = Id::new(self.storage.len().try_into().unwrap()); self.storage.push(element); id } @@ -195,7 +196,7 @@ impl DenseMap { /// 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) -> Id { - let id = Id::new(self.storage.len()); + let id = Id::new(self.storage.len().try_into().unwrap()); self.storage.push(f(id)); id } @@ -204,7 +205,7 @@ impl DenseMap { /// /// The id-element pairs are ordered by the numeric values of the ids. pub(crate) fn iter(&self) -> impl ExactSizeIterator, &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()) } } @@ -219,13 +220,13 @@ impl std::ops::Index> for DenseMap { type Output = T; fn index(&self, id: Id) -> &Self::Output { - &self.storage[id.index] + &self.storage[id.index as usize] } } impl std::ops::IndexMut> for DenseMap { fn index_mut(&mut self, id: Id) -> &mut Self::Output { - &mut self.storage[id.index] + &mut self.storage[id.index as usize] } } @@ -253,7 +254,7 @@ impl SparseMap { /// Adds an element to the map. /// Returns the identifier/reference to that element. pub(crate) fn insert(&mut self, element: T) -> Id { - let id = Id::new(self.storage.len()); + let id = Id::new(self.storage.len().try_into().unwrap()); self.storage.insert(id, element); id } @@ -261,7 +262,7 @@ impl SparseMap { /// 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) -> Id { - let id = Id::new(self.storage.len()); + let id = Id::new(self.storage.len().try_into().unwrap()); self.storage.insert(id, f(id)); id } @@ -365,7 +366,7 @@ impl std::ops::Index<&K> for TwoWayMap { /// This type wraps an AtomicUsize so it can safely be used across threads. #[derive(Debug, Serialize, Deserialize)] pub(crate) struct AtomicCounter { - next: AtomicUsize, + next: AtomicU32, _marker: std::marker::PhantomData, } @@ -373,7 +374,7 @@ impl AtomicCounter { /// Create a new counter starting after the given Id. /// Use AtomicCounter::default() to start at zero. pub(crate) fn starting_after(id: Id) -> 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 diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/defunctionalize.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/defunctionalize.rs index ded1f52d529a..7d7798fd30ac 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/defunctionalize.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/defunctionalize.rs @@ -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 diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs index 142447c83a55..7ef793a350b6 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/opt/unrolling.rs @@ -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); diff --git a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs index b0003aa5f0fc..7c7e977c6ce5 100644 --- a/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs +++ b/noir/noir-repo/compiler/noirc_evaluator/src/ssa/parser/into_ssa.rs @@ -57,7 +57,7 @@ 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); @@ -65,7 +65,7 @@ impl Translator { 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); diff --git a/noir/noir-repo/tooling/nargo_cli/src/cli/test_cmd.rs b/noir/noir-repo/tooling/nargo_cli/src/cli/test_cmd.rs index 75e71818594a..1fd4ed2d8733 100644 --- a/noir/noir-repo/tooling/nargo_cli/src/cli/test_cmd.rs +++ b/noir/noir-repo/tooling/nargo_cli/src/cli/test_cmd.rs @@ -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, @@ -71,6 +71,8 @@ enum Format { Pretty, /// Display one character per test Terse, + /// Output a JSON Lines document + Json, } impl Format { @@ -78,6 +80,7 @@ impl Format { match self { Format::Pretty => Box::new(PrettyFormatter), Format::Terse => Box::new(TerseFormatter), + Format::Json => Box::new(JsonFormatter), } } } @@ -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"), } } } @@ -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| { @@ -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), @@ -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; } @@ -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 @@ -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, @@ -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)); - } -} diff --git a/noir/noir-repo/tooling/nargo_cli/src/cli/test_cmd/formatters.rs b/noir/noir-repo/tooling/nargo_cli/src/cli/test_cmd/formatters.rs index 2a791930f60b..1b9b2d503788 100644 --- a/noir/noir-repo/tooling/nargo_cli/src/cli/test_cmd/formatters.rs +++ b/noir/noir-repo/tooling/nargo_cli/src/cli/test_cmd/formatters.rs @@ -2,15 +2,47 @@ use std::{io::Write, panic::RefUnwindSafe, time::Duration}; use fm::FileManager; use nargo::ops::TestStatus; +use noirc_errors::{reporter::stack_trace, FileDiagnostic}; +use serde_json::{json, Map}; use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, StandardStreamLock, WriteColor}; use super::TestResult; +/// A formatter for showing test results. +/// +/// The order of events is: +/// 1. Compilation of all packages happen (in parallel). There's no formatter method for this. +/// 2. If compilation is successful, one `package_start_async` for each package. +/// 3. For each test, one `test_start_async` event +/// (there's no `test_start_sync` event because it would happen right before `test_end_sync`) +/// 4. For each package, sequentially: +/// a. A `package_start_sync` event +/// b. One `test_end` event for each test +/// a. A `package_end` event +/// +/// The reason we have some `sync` and `async` events is that formatters that show output +/// to humans rely on the `sync` events to show a more predictable output (package by package), +/// and formatters that output to a machine-readable format (like JSON) rely on the `async` +/// events to show things as soon as they happen, regardless of a package ordering. pub(super) trait Formatter: Send + Sync + RefUnwindSafe { - fn package_start(&self, package_name: &str, test_count: usize) -> std::io::Result<()>; + fn package_start_async(&self, package_name: &str, test_count: usize) -> std::io::Result<()>; + + fn package_start_sync(&self, package_name: &str, test_count: usize) -> std::io::Result<()>; + + fn test_start_async(&self, name: &str, package_name: &str) -> std::io::Result<()>; + + #[allow(clippy::too_many_arguments)] + fn test_end_async( + &self, + test_result: &TestResult, + file_manager: &FileManager, + show_output: bool, + deny_warnings: bool, + silence_warnings: bool, + ) -> std::io::Result<()>; #[allow(clippy::too_many_arguments)] - fn test_end( + fn test_end_sync( &self, test_result: &TestResult, current_test_count: usize, @@ -35,11 +67,30 @@ pub(super) trait Formatter: Send + Sync + RefUnwindSafe { pub(super) struct PrettyFormatter; impl Formatter for PrettyFormatter { - fn package_start(&self, package_name: &str, test_count: usize) -> std::io::Result<()> { + fn package_start_async(&self, _package_name: &str, _test_count: usize) -> std::io::Result<()> { + Ok(()) + } + + fn package_start_sync(&self, package_name: &str, test_count: usize) -> std::io::Result<()> { package_start(package_name, test_count) } - fn test_end( + fn test_start_async(&self, _name: &str, _package_name: &str) -> std::io::Result<()> { + Ok(()) + } + + fn test_end_async( + &self, + _test_result: &TestResult, + _file_manager: &FileManager, + _show_output: bool, + _deny_warnings: bool, + _silence_warnings: bool, + ) -> std::io::Result<()> { + Ok(()) + } + + fn test_end_sync( &self, test_result: &TestResult, _current_test_count: usize, @@ -173,11 +224,30 @@ impl Formatter for PrettyFormatter { pub(super) struct TerseFormatter; impl Formatter for TerseFormatter { - fn package_start(&self, package_name: &str, test_count: usize) -> std::io::Result<()> { + fn package_start_async(&self, _package_name: &str, _test_count: usize) -> std::io::Result<()> { + Ok(()) + } + + fn package_start_sync(&self, package_name: &str, test_count: usize) -> std::io::Result<()> { package_start(package_name, test_count) } - fn test_end( + fn test_start_async(&self, _name: &str, _package_name: &str) -> std::io::Result<()> { + Ok(()) + } + + fn test_end_async( + &self, + _test_result: &TestResult, + _file_manager: &FileManager, + _show_output: bool, + _deny_warnings: bool, + _silence_warnings: bool, + ) -> std::io::Result<()> { + Ok(()) + } + + fn test_end_sync( &self, test_result: &TestResult, current_test_count: usize, @@ -317,8 +387,153 @@ impl Formatter for TerseFormatter { } } +pub(super) struct JsonFormatter; + +impl Formatter for JsonFormatter { + fn package_start_async(&self, package_name: &str, test_count: usize) -> std::io::Result<()> { + let json = json!({"type": "suite", "event": "started", "name": package_name, "test_count": test_count}); + println!("{json}"); + Ok(()) + } + + fn package_start_sync(&self, _package_name: &str, _test_count: usize) -> std::io::Result<()> { + Ok(()) + } + + fn test_start_async(&self, name: &str, package_name: &str) -> std::io::Result<()> { + let json = json!({"type": "test", "event": "started", "name": name, "suite": package_name}); + println!("{json}"); + Ok(()) + } + + fn test_end_async( + &self, + test_result: &TestResult, + file_manager: &FileManager, + show_output: bool, + _deny_warnings: bool, + silence_warnings: bool, + ) -> std::io::Result<()> { + let mut json = Map::new(); + json.insert("type".to_string(), json!("test")); + json.insert("name".to_string(), json!(&test_result.name)); + json.insert("exec_time".to_string(), json!(test_result.time_to_run.as_secs_f64())); + + let mut stdout = String::new(); + if show_output && !test_result.output.is_empty() { + stdout.push_str(test_result.output.trim()); + } + + match &test_result.status { + TestStatus::Pass => { + json.insert("event".to_string(), json!("ok")); + } + TestStatus::Fail { message, error_diagnostic } => { + json.insert("event".to_string(), json!("failed")); + + if !stdout.is_empty() { + stdout.push('\n'); + } + stdout.push_str(message.trim()); + + if let Some(diagnostic) = error_diagnostic { + if !(diagnostic.diagnostic.is_warning() && silence_warnings) { + stdout.push('\n'); + stdout.push_str(&diagnostic_to_string(diagnostic, file_manager)); + } + } + } + TestStatus::Skipped => { + json.insert("event".to_string(), json!("ignored")); + } + TestStatus::CompileError(diagnostic) => { + json.insert("event".to_string(), json!("failed")); + + if !(diagnostic.diagnostic.is_warning() && silence_warnings) { + if !stdout.is_empty() { + stdout.push('\n'); + } + stdout.push_str(&diagnostic_to_string(diagnostic, file_manager)); + } + } + } + + if !stdout.is_empty() { + json.insert("stdout".to_string(), json!(stdout)); + } + + let json = json!(json); + println!("{json}"); + + Ok(()) + } + + fn test_end_sync( + &self, + _test_result: &TestResult, + _current_test_count: usize, + _total_test_count: usize, + _file_manager: &FileManager, + _show_output: bool, + _deny_warnings: bool, + _silence_warnings: bool, + ) -> std::io::Result<()> { + Ok(()) + } + + fn package_end( + &self, + _package_name: &str, + test_results: &[TestResult], + _file_manager: &FileManager, + _show_output: bool, + _deny_warnings: bool, + _silence_warnings: bool, + ) -> std::io::Result<()> { + let mut passed = 0; + let mut failed = 0; + let mut ignored = 0; + for test_result in test_results { + match &test_result.status { + TestStatus::Pass => passed += 1, + TestStatus::Fail { .. } | TestStatus::CompileError(..) => failed += 1, + TestStatus::Skipped => ignored += 1, + } + } + let event = if failed == 0 { "ok" } else { "failed" }; + let json = json!({"type": "suite", "event": event, "passed": passed, "failed": failed, "ignored": ignored}); + println!("{json}"); + Ok(()) + } +} + fn package_start(package_name: &str, test_count: usize) -> std::io::Result<()> { let plural = if test_count == 1 { "" } else { "s" }; println!("[{package_name}] Running {test_count} test function{plural}"); Ok(()) } + +fn diagnostic_to_string(file_diagnostic: &FileDiagnostic, file_manager: &FileManager) -> String { + let file_map = file_manager.as_file_map(); + + let custom_diagnostic = &file_diagnostic.diagnostic; + let mut message = String::new(); + message.push_str(custom_diagnostic.message.trim()); + + for note in &custom_diagnostic.notes { + message.push('\n'); + message.push_str(note.trim()); + } + + if let Ok(name) = file_map.get_name(file_diagnostic.file_id) { + message.push('\n'); + message.push_str(&format!("at {name}")); + } + + if !custom_diagnostic.call_stack.is_empty() { + message.push('\n'); + message.push_str(&stack_trace(file_map, &custom_diagnostic.call_stack)); + } + + message +}