From 3f9266d50a5628da6021ed390ce4f1bb47a1952f Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Fri, 11 Oct 2024 20:27:31 +0100 Subject: [PATCH 1/3] Reject invalid expression with in CLI parser --- acvm-repo/acvm/src/compiler/mod.rs | 2 +- .../acvm/src/compiler/transformers/csat.rs | 9 +++++++-- acvm-repo/acvm/src/compiler/transformers/mod.rs | 1 + compiler/noirc_driver/src/lib.rs | 7 ++++++- tooling/nargo_cli/benches/criterion.rs | 2 ++ tooling/nargo_cli/src/cli/mod.rs | 17 +++++++++++++++++ 6 files changed, 34 insertions(+), 4 deletions(-) diff --git a/acvm-repo/acvm/src/compiler/mod.rs b/acvm-repo/acvm/src/compiler/mod.rs index 5ece3d19a6e..92e03cc90c2 100644 --- a/acvm-repo/acvm/src/compiler/mod.rs +++ b/acvm-repo/acvm/src/compiler/mod.rs @@ -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. diff --git a/acvm-repo/acvm/src/compiler/transformers/csat.rs b/acvm-repo/acvm/src/compiler/transformers/csat.rs index f258e0a8818..bdd6998835a 100644 --- a/acvm-repo/acvm/src/compiler/transformers/csat.rs +++ b/acvm-repo/acvm/src/compiler/transformers/csat.rs @@ -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. /// @@ -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() } } diff --git a/acvm-repo/acvm/src/compiler/transformers/mod.rs b/acvm-repo/acvm/src/compiler/transformers/mod.rs index 4fd8ba7883f..4e29681cbed 100644 --- a/acvm-repo/acvm/src/compiler/transformers/mod.rs +++ b/acvm-repo/acvm/src/compiler/transformers/mod.rs @@ -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}; diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 2f0122524eb..300c8292086 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -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; @@ -134,7 +135,11 @@ pub fn parse_expression_width(input: &str) -> Result Ok(ExpressionWidth::Unbounded), - _ => Ok(ExpressionWidth::Bounded { width }), + w if w >= MIN_EXPRESSION_WIDTH => Ok(ExpressionWidth::Bounded { width }), + _ => Err(Error::new( + ErrorKind::InvalidInput, + format!("minimum value is {MIN_EXPRESSION_WIDTH}"), + )), } } diff --git a/tooling/nargo_cli/benches/criterion.rs b/tooling/nargo_cli/benches/criterion.rs index 949f7add393..87683f39fe2 100644 --- a/tooling/nargo_cli/benches/criterion.rs +++ b/tooling/nargo_cli/benches/criterion.rs @@ -23,6 +23,8 @@ fn compile_program(test_program_dir: &Path) { cmd.arg("--program-dir").arg(test_program_dir); cmd.arg("compile"); cmd.arg("--force"); + cmd.arg("--expression-width"); + cmd.arg("1"); cmd.assert().success(); } diff --git a/tooling/nargo_cli/src/cli/mod.rs b/tooling/nargo_cli/src/cli/mod.rs index 10ec38ad1d5..2e8c5360181 100644 --- a/tooling/nargo_cli/src/cli/mod.rs +++ b/tooling/nargo_cli/src/cli/mod.rs @@ -112,3 +112,20 @@ pub(crate) fn start_cli() -> eyre::Result<()> { println!("{markdown}"); Ok(()) } + +#[cfg(test)] +mod tests { + use clap::Parser; + /// Test that parsing an invalid option doesn't + #[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())); + } +} From b34365f683717b10b4d22fbde651166563b65c88 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Fri, 11 Oct 2024 20:41:57 +0100 Subject: [PATCH 2/3] Change error message to include 0 as an option --- compiler/noirc_driver/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/compiler/noirc_driver/src/lib.rs b/compiler/noirc_driver/src/lib.rs index 300c8292086..f7270a6e4ed 100644 --- a/compiler/noirc_driver/src/lib.rs +++ b/compiler/noirc_driver/src/lib.rs @@ -138,7 +138,7 @@ pub fn parse_expression_width(input: &str) -> Result= MIN_EXPRESSION_WIDTH => Ok(ExpressionWidth::Bounded { width }), _ => Err(Error::new( ErrorKind::InvalidInput, - format!("minimum value is {MIN_EXPRESSION_WIDTH}"), + format!("has to be 0 or at least {MIN_EXPRESSION_WIDTH}"), )), } } From f3cc5112b2dc4638920f3dc6908ca8f21e7d21b8 Mon Sep 17 00:00:00 2001 From: Akosh Farkash Date: Thu, 17 Oct 2024 18:01:09 +0100 Subject: [PATCH 3/3] Cleanup --- tooling/nargo_cli/benches/criterion.rs | 2 -- tooling/nargo_cli/src/cli/mod.rs | 1 - 2 files changed, 3 deletions(-) diff --git a/tooling/nargo_cli/benches/criterion.rs b/tooling/nargo_cli/benches/criterion.rs index 87683f39fe2..949f7add393 100644 --- a/tooling/nargo_cli/benches/criterion.rs +++ b/tooling/nargo_cli/benches/criterion.rs @@ -23,8 +23,6 @@ fn compile_program(test_program_dir: &Path) { cmd.arg("--program-dir").arg(test_program_dir); cmd.arg("compile"); cmd.arg("--force"); - cmd.arg("--expression-width"); - cmd.arg("1"); cmd.assert().success(); } diff --git a/tooling/nargo_cli/src/cli/mod.rs b/tooling/nargo_cli/src/cli/mod.rs index 2e8c5360181..9108815f05d 100644 --- a/tooling/nargo_cli/src/cli/mod.rs +++ b/tooling/nargo_cli/src/cli/mod.rs @@ -116,7 +116,6 @@ pub(crate) fn start_cli() -> eyre::Result<()> { #[cfg(test)] mod tests { use clap::Parser; - /// Test that parsing an invalid option doesn't #[test] fn test_parse_invalid_expression_width() { let cmd = "nargo --program-dir . compile --expression-width 1";