Skip to content

Commit

Permalink
feat: add set and set_unchecked methods to Vec and BoundedVec (
Browse files Browse the repository at this point in the history
…#5241)

# Description

## Problem\*

Resolves <!-- Link to GitHub Issue -->

## Summary\*

This PR exposes `set` and `set_unchecked` methods to `BoundedVec` which
allow writing to individual indices in the array without having to
access `self.storage` directly.

I've also documented the expected behaviour for the get and set
operations.


## Additional Context



## Documentation\*

Check one:
- [ ] No documentation needed.
- [x] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
TomAFrench authored Jun 13, 2024
1 parent a2816db commit 1849389
Show file tree
Hide file tree
Showing 6 changed files with 263 additions and 6 deletions.
40 changes: 38 additions & 2 deletions docs/docs/noir/standard_library/containers/boundedvec.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ but for now make sure to use type annotations when using bounded vectors. Otherw
### get

```rust
pub fn get(mut self: Self, index: u64) -> T {
pub fn get(self, index: u64) -> T {
```

Retrieves an element from the vector at the given index, starting from zero.
Expand All @@ -80,7 +80,7 @@ fn foo<N>(v: BoundedVec<u32, N>) {
### get_unchecked

```rust
pub fn get_unchecked(mut self: Self, index: u64) -> T {
pub fn get_unchecked(self, index: u64) -> T {
```

Retrieves an element from the vector at the given index, starting from zero, without
Expand All @@ -93,6 +93,42 @@ Example:

#include_code get_unchecked_example test_programs/noir_test_success/bounded_vec/src/main.nr rust

### set

```rust
pub fn set(&mut self: Self, index: u64, value: T) {
```

Writes an element to the vector at the given index, starting from zero.

If the given index is equal to or greater than the length of the vector, this will issue a constraint failure.

Example:

```rust
fn foo<N>(v: BoundedVec<u32, N>) {
let first = v.get(0);
assert(first != 42);
v.set(0, 42);
let new_first = v.get(0);
assert(new_first == 42);
}
```

### set_unchecked

```rust
pub fn set_unchecked(&mut self: Self, index: u64, value: T) -> T {
```

Writes an element to the vector at the given index, starting from zero, without performing a bounds check.

Since this function does not perform a bounds check on length before accessing the element, it is unsafe! Use at your own risk!

Example:

#include_code set_unchecked_example test_programs/noir_test_success/bounded_vec/src/main.nr rust


### push

Expand Down
19 changes: 19 additions & 0 deletions docs/docs/noir/standard_library/containers/vec.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,25 @@ let vector: Vec<Field> = Vec::from_slice(&[10, 20, 30]);
assert(vector.get(1) == 20);
```

### set

```rust
pub fn set(&mut self: Self, index: u64, value: T) {
```

Writes an element to the vector at the given index, starting from zero.

Panics if the index points beyond the vector's end.

Example:

```rust
let vector: Vec<Field> = Vec::from_slice(&[10, 20, 30]);
assert(vector.get(1) == 20);
vector.set(1, 42);
assert(vector.get(1) == 42);
```

### push

Adds a new element to the vector's end, returning a new vector with a length one greater than the original unmodified vector.
Expand Down
59 changes: 56 additions & 3 deletions noir_stdlib/src/collections/bounded_vec.nr
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,35 @@ impl<T, MaxLen> BoundedVec<T, MaxLen> {
BoundedVec { storage: [zeroed; MaxLen], len: 0 }
}

pub fn get(mut self: Self, index: u32) -> T {
/// Get an element from the vector at the given index.
/// Panics if the given index points beyond the end of the vector (`self.len()`).
pub fn get(self, index: u32) -> T {
assert(index < self.len);
self.storage[index]
self.get_unchecked(index)
}

pub fn get_unchecked(mut self: Self, index: u32) -> T {
/// Get an element from the vector at the given index.
/// Responds with undefined data for `index` where `self.len < index < self.max_len()`.
pub fn get_unchecked(self, index: u32) -> T {
self.storage[index]
}

/// Write an element to the vector at the given index.
/// Panics if the given index points beyond the end of the vector (`self.len()`).
pub fn set(&mut self, index: u32, value: T) {
assert(index < self.len, "Attempted to write past end of BoundedVec");
self.set_unchecked(index, value)
}

/// Write an element to the vector at the given index.
/// Does not check whether the passed `index` is a valid index within the vector.
///
/// Silently writes past the end of the vector for `index` where `self.len < index < self.max_len()`
/// Panics if the given index points beyond the maximum length of the vector (`self.max_len()`).
pub fn set_unchecked(&mut self, index: u32, value: T) {
self.storage[index] = value;
}

pub fn push(&mut self, elem: T) {
assert(self.len < MaxLen, "push out of bounds");

Expand Down Expand Up @@ -142,6 +162,39 @@ mod bounded_vec_tests {
assert(bounded_vec1 != bounded_vec2);
}

mod set {
use crate::collections::bounded_vec::BoundedVec;

#[test]
fn set_updates_values_properly() {
let mut vec = BoundedVec::from_array([0, 0, 0, 0, 0]);

vec.set(0, 42);
assert_eq(vec.storage, [42, 0, 0, 0, 0]);

vec.set(1, 43);
assert_eq(vec.storage, [42, 43, 0, 0, 0]);

vec.set(2, 44);
assert_eq(vec.storage, [42, 43, 44, 0, 0]);

vec.set(1, 10);
assert_eq(vec.storage, [42, 10, 44, 0, 0]);

vec.set(0, 0);
assert_eq(vec.storage, [0, 10, 44, 0, 0]);
}

#[test(should_fail_with = "Attempted to write past end of BoundedVec")]
fn panics_when_writing_elements_past_end_of_vec() {
let mut vec: BoundedVec<Field, 5> = BoundedVec::new();
vec.set(0, 42);

// Need to use println to avoid DIE removing the write operation.
crate::println(vec.get(0));
}
}

mod from_array {
use crate::collections::bounded_vec::BoundedVec;

Expand Down
39 changes: 39 additions & 0 deletions noir_stdlib/src/collections/vec.nr
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,12 @@ impl<T> Vec<T> {
self.slice[index]
}

/// Write an element to the vector at the given index.
/// Panics if the given index points beyond the end of the vector (`self.len()`).
pub fn set(&mut self, index: u32, value: T) {
self.slice[index] = value;
}

/// Push a new element to the end of the vector, returning a
/// new vector with a length one greater than the
/// original unmodified vector.
Expand Down Expand Up @@ -57,3 +63,36 @@ impl<T> Vec<T> {
self.slice.len()
}
}

mod tests {
use crate::collections::vec::Vec;

#[test]
fn set_updates_values_properly() {
let mut vec = Vec { slice: &[0, 0, 0, 0, 0] };

vec.set(0, 42);
assert_eq(vec.slice, &[42, 0, 0, 0, 0]);

vec.set(1, 43);
assert_eq(vec.slice, &[42, 43, 0, 0, 0]);

vec.set(2, 44);
assert_eq(vec.slice, &[42, 43, 44, 0, 0]);

vec.set(1, 10);
assert_eq(vec.slice, &[42, 10, 44, 0, 0]);

vec.set(0, 0);
assert_eq(vec.slice, &[0, 10, 44, 0, 0]);
}

#[test(should_fail)]
fn panics_when_writing_elements_past_end_of_vec() {
let mut vec = Vec::new();
vec.set(0, 42);

// Need to use println to avoid DIE removing the write operation.
crate::println(vec.get(0));
}
}
27 changes: 27 additions & 0 deletions test_programs/noir_test_success/bounded_vec/src/main.nr
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,33 @@ fn sum_of_first_three<N>(v: BoundedVec<u32, N>) -> u32 {
}
// docs:end:get_unchecked_example

#[test(should_fail)]
// docs:start:set_unchecked_example
fn set_unchecked_example() {
let mut vec: BoundedVec<u32, 5> = BoundedVec::new();
vec.extend_from_array([1, 2]);

// Here we're safely writing within the valid range of `vec`
// `vec` now has the value [42, 2]
vec.set_unchecked(0, 42);

// We can then safely read this value back out of `vec`.
// Notice that we use the checked version of `get` which would prevent reading unsafe values.
assert_eq(vec.get(0), 42);

// We've now written past the end of `vec`.
// As this index is still within the maximum potential length of `v`,
// it won't cause a constraint failure.
vec.set_unchecked(2, 42);
println(vec);

// This will write past the end of the maximum potential length of `vec`,
// it will then trigger a constraint failure.
vec.set_unchecked(5, 42);
println(vec);
}
// docs:end:set_unchecked_example

#[test(should_fail_with = "push out of bounds")]
fn push_docs_example() {
// docs:start:bounded-vec-push-example
Expand Down
85 changes: 84 additions & 1 deletion tooling/nargo_cli/tests/stdlib-tests.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use std::io::Write;
use std::{collections::BTreeMap, path::PathBuf};

use acvm::blackbox_solver::StubbedBlackBoxSolver;
use fm::FileManager;
use noirc_driver::{check_crate, file_manager_with_stdlib, CompileOptions};
use noirc_frontend::hir::FunctionNameMatch;

Expand All @@ -9,6 +11,7 @@ use nargo::{
package::{Package, PackageType},
parse_all, prepare_package,
};
use termcolor::{Color, ColorChoice, ColorSpec, StandardStream, WriteColor};

#[test]
fn run_stdlib_tests() {
Expand All @@ -23,7 +26,7 @@ fn run_stdlib_tests() {
root_dir: PathBuf::from("."),
package_type: PackageType::Binary,
entry_path: PathBuf::from("main.nr"),
name: "dummy".parse().unwrap(),
name: "stdlib".parse().unwrap(),
dependencies: BTreeMap::new(),
};

Expand Down Expand Up @@ -58,5 +61,85 @@ fn run_stdlib_tests() {
.collect();

assert!(!test_report.is_empty(), "Could not find any tests within the stdlib");
display_test_report(&file_manager, &dummy_package, &CompileOptions::default(), &test_report);
assert!(test_report.iter().all(|(_, status)| !status.failed()));
}

// This code is copied from `src/cli/test_cmd.rs`.
// This should be abstracted into a proper test runner at some point.
fn display_test_report(
file_manager: &FileManager,
package: &Package,
compile_options: &CompileOptions,
test_report: &[(String, TestStatus)],
) {
let writer = StandardStream::stderr(ColorChoice::Always);
let mut writer = writer.lock();

for (test_name, test_status) in test_report {
write!(writer, "[{}] Testing {test_name}... ", package.name)
.expect("Failed to write to stderr");
writer.flush().expect("Failed to flush writer");

match &test_status {
TestStatus::Pass { .. } => {
writer
.set_color(ColorSpec::new().set_fg(Some(Color::Green)))
.expect("Failed to set color");
writeln!(writer, "ok").expect("Failed to write to stderr");
}
TestStatus::Fail { message, error_diagnostic } => {
writer
.set_color(ColorSpec::new().set_fg(Some(Color::Red)))
.expect("Failed to set color");
writeln!(writer, "FAIL\n{message}\n").expect("Failed to write to stderr");
if let Some(diag) = error_diagnostic {
noirc_errors::reporter::report_all(
file_manager.as_file_map(),
&[diag.clone()],
compile_options.deny_warnings,
compile_options.silence_warnings,
);
}
}
TestStatus::CompileError(err) => {
noirc_errors::reporter::report_all(
file_manager.as_file_map(),
&[err.clone()],
compile_options.deny_warnings,
compile_options.silence_warnings,
);
}
}
writer.reset().expect("Failed to reset writer");
}

write!(writer, "[{}] ", package.name).expect("Failed to write to stderr");

let count_all = test_report.len();
let count_failed = test_report.iter().filter(|(_, status)| status.failed()).count();
let plural = if count_all == 1 { "" } else { "s" };
if count_failed == 0 {
writer.set_color(ColorSpec::new().set_fg(Some(Color::Green))).expect("Failed to set color");
write!(writer, "{count_all} test{plural} passed").expect("Failed to write to stderr");
writer.reset().expect("Failed to reset writer");
writeln!(writer).expect("Failed to write to stderr");
} else {
let count_passed = count_all - count_failed;
let plural_failed = if count_failed == 1 { "" } else { "s" };
let plural_passed = if count_passed == 1 { "" } else { "s" };

if count_passed != 0 {
writer
.set_color(ColorSpec::new().set_fg(Some(Color::Green)))
.expect("Failed to set color");
write!(writer, "{count_passed} test{plural_passed} passed, ",)
.expect("Failed to write to stderr");
}

writer.set_color(ColorSpec::new().set_fg(Some(Color::Red))).expect("Failed to set color");
writeln!(writer, "{count_failed} test{plural_failed} failed")
.expect("Failed to write to stderr");
writer.reset().expect("Failed to reset writer");
}
}

0 comments on commit 1849389

Please sign in to comment.