Skip to content

Commit

Permalink
fix: Reject invalid expression with in CLI parser (#6287)
Browse files Browse the repository at this point in the history
# Description

## Problem\*

Resolves #5560

## Summary\*

Rejects an invalid `--expression-value` in the CLI, which would later
cause a panic in `acvm::compiler::transformers::csat`.

An example error message looks like:

```text
error: invalid value '1' for '--expression-width <EXPRESSION_WIDTH>': minimum value is 3
```

## Additional Context

The issue suggests that
[CSatTransformer::new](https://github.com/noir-lang/noir/blob/ae87d287ab1fae0f999dfd0d1166fbddb927ba97/acvm-repo/acvm/src/compiler/transformers/csat.rs#L24-L27)
should return a `Result` explaining the problem it has with `width`,
rather than doing an `assert!` (without any message) and crashing the
program. I agree, however looking at the chain of calls, none of the
half-dozen functions we go through to reach this use `Result`, and the
`CSatTransformer` is the only option we have. This suggests to me that
this limitation is perhaps supposed to be well-known to the user, ie.
it's not the case that one transformer has X limit and another has Y
limit.

For this reason I added a `pub const MIN_EXPRESSION_WIDTH: usize = 3;`
to the `acvm::compiler` module and using that as a common value for the
assertion as well as the validation in the CLI. Should the assumption of
a single global value change, removing that will force us to update the
validation logic as well.

That said if you prefer going the `Result` route I'm not against it, it
just seemed like an overkill for this single use case.
 
## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] 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.
- [ ] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
  • Loading branch information
aakoshh authored Oct 17, 2024
1 parent ae87d28 commit 052aee8
Show file tree
Hide file tree
Showing 5 changed files with 31 additions and 4 deletions.
2 changes: 1 addition & 1 deletion acvm-repo/acvm/src/compiler/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,8 @@ mod transformers;

pub use optimizers::optimize;
use optimizers::optimize_internal;
pub use transformers::transform;
use transformers::transform_internal;
pub use transformers::{transform, MIN_EXPRESSION_WIDTH};

/// This module moves and decomposes acir opcodes. The transformation map allows consumers of this module to map
/// metadata they had about the opcodes to the new opcode structure generated after the transformation.
Expand Down
9 changes: 7 additions & 2 deletions acvm-repo/acvm/src/compiler/transformers/csat.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@ use acir::{
};
use indexmap::IndexMap;

/// Minimum width accepted by the `CSatTransformer`.
pub const MIN_EXPRESSION_WIDTH: usize = 3;

/// A transformer which processes any [`Expression`]s to break them up such that they
/// fit within the [`ProofSystemCompiler`][crate::ProofSystemCompiler]'s width.
///
Expand All @@ -22,9 +25,11 @@ pub(crate) struct CSatTransformer {
}

impl CSatTransformer {
// Configure the width for the optimizer
/// Create an optimizer with a given width.
///
/// Panics if `width` is less than `MIN_EXPRESSION_WIDTH`.
pub(crate) fn new(width: usize) -> CSatTransformer {
assert!(width > 2);
assert!(width >= MIN_EXPRESSION_WIDTH, "width has to be at least {MIN_EXPRESSION_WIDTH}");

CSatTransformer { width, solvable_witness: HashSet::new() }
}
Expand Down
1 change: 1 addition & 0 deletions acvm-repo/acvm/src/compiler/transformers/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ use indexmap::IndexMap;
mod csat;

pub(crate) use csat::CSatTransformer;
pub use csat::MIN_EXPRESSION_WIDTH;

use super::{transform_assert_messages, AcirTransformationMap};

Expand Down
7 changes: 6 additions & 1 deletion compiler/noirc_driver/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

use abi_gen::{abi_type_from_hir_type, value_from_hir_expression};
use acvm::acir::circuit::ExpressionWidth;
use acvm::compiler::MIN_EXPRESSION_WIDTH;
use clap::Args;
use fm::{FileId, FileManager};
use iter_extended::vecmap;
Expand Down Expand Up @@ -134,7 +135,11 @@ pub fn parse_expression_width(input: &str) -> Result<ExpressionWidth, std::io::E

match width {
0 => Ok(ExpressionWidth::Unbounded),
_ => Ok(ExpressionWidth::Bounded { width }),
w if w >= MIN_EXPRESSION_WIDTH => Ok(ExpressionWidth::Bounded { width }),
_ => Err(Error::new(
ErrorKind::InvalidInput,
format!("has to be 0 or at least {MIN_EXPRESSION_WIDTH}"),
)),
}
}

Expand Down
16 changes: 16 additions & 0 deletions tooling/nargo_cli/src/cli/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,3 +112,19 @@ pub(crate) fn start_cli() -> eyre::Result<()> {
println!("{markdown}");
Ok(())
}

#[cfg(test)]
mod tests {
use clap::Parser;
#[test]
fn test_parse_invalid_expression_width() {
let cmd = "nargo --program-dir . compile --expression-width 1";
let res = super::NargoCli::try_parse_from(cmd.split_ascii_whitespace());

let err = res.expect_err("should fail because of invalid width");
assert!(err.to_string().contains("expression-width"));
assert!(err
.to_string()
.contains(acvm::compiler::MIN_EXPRESSION_WIDTH.to_string().as_str()));
}
}

0 comments on commit 052aee8

Please sign in to comment.