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

ACIR gen errors with some kinds of known failing constraints, changing expected errors in tests #5464

Closed
sirasistant opened this issue Jul 10, 2024 · 1 comment · Fixed by #5466
Labels
bug Something isn't working

Comments

@sirasistant
Copy link
Contributor

Aim

Writing a test of a function that guards out of bounds accesses with a custom message, snippet:

    for i in 0..N {
        if i < collapsed.len() {
            let input_index = collapsed_to_input_index_mapping.get_unchecked(i);
            assert(input_index < N, "Out of bounds index hint"); // instead of here

            assert_eq(collapsed.get_unchecked(i), input[input_index].unwrap(), "Wrong collapsed vec content"); // erroring here

Expected Behavior

It should fail in assert(input_index < N, "Out of bounds index hint");

Bug

instead fails in input[input_index].
This is the SSA:

After Array Set Optimizations:
acir(inline) fn verify_collapse_hints_out_of_bounds_index_hint f0 {
  b0():
    constrain u1 0 == u1 1 'Out of bounds index hint'
    v66 = array_get [u1 1, Field 7, u1 0, Field 0, u1 1, Field 3], index u32 10
    v67 = array_get [u1 1, Field 7, u1 0, Field 0, u1 1, Field 3], index u32 11
    constrain v66 == u1 1
    constrain Field 3 == v67 'Wrong collapsed vec content'
    return 
}

As you can see it should fail in the known to fail constraint (it's known to fail at compile time because this is a test)
Instead it fails in the array get with index 10, because ACIR generation evaluates array gets and errors directly, but apparently doesn't do so with assertions

To Reproduce

Project Impact

None

Impact Context

No response

Workaround

None

Workaround Description

No response

Additional Context

No response

Installation Method

None

Nargo Version

No response

NoirJS Version

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

@sirasistant sirasistant added the bug Something isn't working label Jul 10, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Jul 10, 2024
@sirasistant
Copy link
Contributor Author

Minimal repro:

fn main(inputs: [Field; 4], index_hint: u32) {
    assert(index_hint < inputs.len(), "Out of bounds index hint");
    let hinted_value = inputs[index_hint];
    assert_eq(hinted_value, 27);
}

#[test(should_fail_with="Out of bounds index hint")]
fn test_bounds_check() {
    main([1, 2, 3, 4], 4);
}

github-merge-queue bot pushed a commit that referenced this issue Jul 10, 2024
# Description

## Problem\*

Resolves #5464 

## Summary\*

Similarly to how we removed compile-time assertion failures in order to
have correct test error messages, we must do the same for invalid
indices.

## Additional Context



## 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.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Jul 10, 2024
nventuro added a commit to AztecProtocol/aztec-packages that referenced this issue Sep 25, 2024
This restores a test that was disabled by
noir-lang/noir#5464 (or rather the failure
reason was removed), and replaces usage of `assert(f.is_some());
f.unwrap()` with the `expect` function.
AztecBot pushed a commit to AztecProtocol/aztec-nr that referenced this issue Sep 26, 2024
This restores a test that was disabled by
noir-lang/noir#5464 (or rather the failure
reason was removed), and replaces usage of `assert(f.is_some());
f.unwrap()` with the `expect` function.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
Archived in project
1 participant