noir-lang / noir

Noir is a domain specific language for zero knowledge proofs
https://noir-lang.org
Apache License 2.0
893 stars 199 forks source link

`[T]::pop_front()` panics when inside non-executed `if` #5462

Closed michaeljklein closed 1 month ago

michaeljklein commented 4 months ago

Aim

Attempted to conditionally pop_front from a slice, depending on whether it's empty:

fn main() {
    let empty_slice: [u8] = &[];

    // Also panics:
    // if empty_slice != &[] {
    if empty_slice.len() != 0 {
        // Works:
        // let _ = empty_slice.pop_back();

        // Panics:
        let _ = empty_slice.pop_front();
    }
}

Expected Behavior

I expected the predicate to prevent pop_front from executing and failing when the slice is empty.

Bug

The application panicked (crashed).
Message:  There are no elements in this slice to be removed
Location: compiler/noirc_evaluator/src/ssa/ir/instruction/call.rs:162

To Reproduce

1. 2. 3. 4.

Project Impact

Nice-to-have

Impact Context

NOTE: this is required to finish the slice control flow test here.

Workaround

None

Workaround Description

No response

Additional Context

No response

Installation Method

Binary (noirup default)

Nargo Version

nargo version = 0.31.0 noirc version = 0.31.0+d44f882be094bf492b1742370fd3896b0c371f59 (git version hash: d44f882be094bf492b1742370fd3896b0c371f59, is dirty: false)

NoirJS Version

No response

Would you like to submit a PR for this Issue?

None

Support Needs

No response

jfecher commented 4 months ago

Looks like pop_back is broken as well but in a different way.

Instead of calling pop_back on its slice internally it repeatedly calls push_front to build a new slice. I believe this is to avoid logic on guessing how many times to pop when the element is a struct type which is represented as multiple SSA values.

Anyway, neither pop_back nor pop_front have checks for if the slice is empty. pop_front leads to the error you found while pop_back will silently "optimize" to an empty array result. Execution will still fail though because it will leave behind a v0 = sub u32 0, u32 1 instruction. When this happens the program just exits with "Constraint Failed" and no explanation.

It looks like there is a length check on SliceRemove and SliceInsert at least.