noir-lang / noir

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

Nargo fails to detect missing trait bounds #5210

Closed nventuro closed 1 week ago

nventuro commented 1 month ago

Aim

In certain scenarios the compiler's front end does not realize that it's missing trait founds, and fails with ICE's later in the compilation pipeline. This is annoying because these errors have no context, the LSP doesn't see them, etc.

Expected Behavior

Given the following code:

trait Serialize<N> {
    fn serialize(self) -> [Field; N];
}

trait ToField {
    fn to_field(self) -> Field;
}

fn process_array<N>(array: [Field; N]) -> Field {
    array[0]
}

fn serialize_thing<T, N>(thing: T) -> [Field; N] where T: Serialize<N> {
    thing.serialize()
}

struct MyType<T> {
    a: T,
    b: T,
}

impl<T> Serialize<2> for MyType<T> {
    fn serialize(self) -> [Field; 2] where T: ToField {
        [ self.a.to_field(), self.b.to_field() ]
    }
}

impl<T> MyType<T> {
    fn do_thing_with_serialization<N>(self) -> Field {
        process_array(self.serialize())
    }

    fn do_thing_with_serialization_with_extra_steps<N>(self) -> Field {
        process_array(serialize_thing(self))
    }
}

Compilation fails with

30 │         process_array(self.serialize())
   │                       ---------------- No impl for `T: ToField`

This is expected: we can only serialize MyType if we ask that T can be converted into a field - otherwise the serialization trait is not implemented. However, by calling .serialize() thorugh serialize_thing in do_thing_with_serialization_with_extra_steps, this requirement is seemingly lifted - we can perform the same callstack without having to require the ToField impl. Seems great!

Unfortunately, if we actually try to call this function we get the following nasty ICE. showing that the true issue was the frontend not checking for this trait:

The application panicked (crashed).
Message:  internal error: entered unreachable code: Failed to find trait impl during monomorphization. The failed constraint(s) are:
  Field: ToField
Location: compiler/noirc_frontend/src/monomorphization/mod.rs:1082

This is doubly annoying when writing library code because it only shows up when using the library, not when building it.

nventuro commented 1 month ago

Interestingly do_thing_with_serialization_with_extra_steps works fine if I add an impl:

impl ToField for Field {
    fn to_field(self) -> Field {
        self
    }
}

I'd want to be able to use the with_extra_steps approach, as it doesn't force me to pollute the entire callstack with T: ToField. It seems like this issue might be two separate issues?

jfecher commented 1 month ago

I'd want to be able to use the with_extra_steps approach, as it doesn't force me to pollute the entire callstack with T: ToField. It seems like this issue might be two separate issues?

The trait constraint is required everywhere in the call stack until T can be resolved to a concrete type. It's just a consequence of the way we do trait dispatch where it is able to find the impl anyway once it exists - it would still panic if it did not. Without the trait constraint there would be two issues:

So the only issue here is that the compiler does not error for the missing constraint on do_thing_with_serialization_with_extra_steps.

nventuro commented 1 month ago

I see, that makes perfect sense. It seems we've been abusing this bug a little bit, so some aztec-nr code will break due to missing where clauses once this is fixed.

This makes me think that I'd also be good to get https://github.com/noir-lang/noir/issues/4508 done - we typically end up having to annonate all methods of an impl with the where clause as soon as we require it in a single place.