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

feat: Sync from noir #10710

Merged
merged 3 commits into from
Dec 13, 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
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
Loading