Skip to content

nightly-2024-10-18: fix: Reject invalid expression with in CLI parser (#6287)

Pre-release
Pre-release
Compare
Choose a tag to compare
@github-actions github-actions released this 18 Oct 02:28
· 95 commits to master since this release
052aee8
# 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.