noir-lang / noir

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

bug: non-numeric type variable Error #2540

Closed ghost closed 7 months ago

ghost commented 1 year ago

Aim

fn hello<N>(array: [u1; N]) {
    for i in 0..N {}
}

fn main() {
    let slice: [u1] = [1, 2];
    hello(slice);
}

Expected Behavior

The hello function should iterate over a valid array of type u1 without panicking or producing runtime errors. In the given example, calling hello with the slice array [1, 2] should execute the loop successfully.

needed for #2473

Bug

The application panicked (crashed).
Message:  Non-numeric type variable used in expression expecting a value: NotConstant
Location: crates/noirc_frontend/src/monomorphization/mod.rs:630

This is a bug. We may have already fixed this in newer versions of Nargo so try searching for similar issues at https://github.com/noir-lang/noir/issues/.
If there isn't an open issue for this bug, consider opening one at https://github.com/noir-lang/noir/issues/new?labels=bug&template=bug_report.yml

To Reproduce

  1. nargo execute

Installation Method

Compiled from source

Nargo Version

No response

Additional Context

No response

Would you like to submit a PR for this Issue?

No

Support Needs

No response

jfecher commented 1 year ago

This is an interesting one. The N in hello takes its value from the length of an array type, but in the case of slices there is no length. Instead the internal value NotConstant used to mark slices is leaked. Not sure what the fix here would be. This may need to be a compiler error of some kind.

jfecher commented 1 year ago

Is this functionality actually required for the linked PR? hello could be rewritten as:

fn hello<N>(array: [u1; N]) {
    for i in 0..array.len() {}
}
ghost commented 1 year ago

@jfecher, doesn't work in this case

error: Could not determine loop bound at compile-time
    ┌─ std/ec/tecurve.nr:343:20
    │
343 │             for i in 0..bits.len() {
    │                    ----------
    │
    = Call stack:
      1. noir/crates/nargo_cli/tests/execution_success/eddsa/src/main.nr:51:17
      2. std/eddsa.nr:68:45
      3. std/ec/tecurve.nr:141:13
      4. std/ec/tecurve.nr:356:13
      5. std/ec/tecurve.nr:343:25
jfecher commented 1 year ago

@f01dab1e looks like a different case in the tecurve test. Using array.len() for the original example passes. I'm assuming you've modified the eddsa test as well since it passes locally for me. The "Could not determine loop bound at compile-time" error is intended for when a value derived from an input to main is used as a loop bound. I'm not sure why that'd be the case for an array length though, unless the array was a slice returned from an unconstrained function.

jfecher commented 8 months ago

Reassigning to @michaeljklein, this is part of #4220

vezenovm commented 7 months ago

@michaeljklein Can we confirm whether we can close this?

michaeljklein commented 7 months ago

@vezenovm yes, adding a regression test here: https://github.com/noir-lang/noir/pull/4602

Noting that there were two issues in the test case:

  1. 2 : u1
    • I've removed this for the purpose of the regression test
  2. Non-numeric type variable used in expression expecting a value
    • NotConstant has been removed with the array/slice separation
vezenovm commented 7 months ago

Noting that there were two issues in the test case:

  1. 2 : u1
  • I've removed this for the purpose of the regression test

We used to support the u1 integer type when this issue was written, so updating the regression that way is the right move.