chore: move unbound impl generics to their functions

[asterite]

In Rust, a code like this:

trait Serialize<U> {}

struct PublicMutable<T>
{
    x: T,
}

impl<T, U> PublicMutable<T>
where
    T: Serialize<U>,
{
    fn serialize() {}
}

fails to compile and gives this error:

error[E0207]: the type parameter `U` is not constrained by the impl trait, self type, or predicates
 --> src/main.rs:8:9
  |
8 | impl<T, U> PublicMutable<T>
  |         ^ unconstrained type parameter

I'm not exactly sure of the technicalities here, there's an explanation here, but the solution in Rust is to move the constrains to the relevant functions.

Noir will eventually do the same thing (see noir-lang/noir#6388 ) so this PR fixes the code that will eventually stop to compile.

[nventuro]

I don't understand the underlying issue enough to comment on whether or not this is an issue for us, e.g. if we happen to be relying on this for some reason. I also don't understand what the problem is so as to be able to identify it and prevent it in the future. Can you expand on this?

[asterite]

Sure!

TL;DR: if this PR gets merged on Noir's side, Aztec-Packages will stop compiling. This will make it compile again.

Noir tends to work as similar to how Rust does. We have this issue on the Noir repository which essentially says that in Rust, this code fails to compile:

struct Foo {}

impl<T> Foo { }

There's a generic impl over T, but T isn't used in one of Foo's generic arguments.

In Noir, that currently doesn't give an error. But, because we want Noir to work as similar to Rust as possible, we'll make that an error.

If that happens, the code in Aztec-Packages will fail to compile. So, this PR anticipates that Noir change to fix this code. The way to fix it is to move those generic types (and bounds, like where T: ...) to the functions that need it.

For Aztec-Packages this should be a no-op except that the code is structured differently.

[nventuro]

I find it very strange that the where clase in the impl is not considered as 'being in use', given that the only change is that where will be in the fns instead of the entire impl.

[asterite]

I find it very strange that the where clase in the impl is not considered as 'being in use', given that the only change is that where will be in the fns instead of the entire impl.

To be honest, me too. But it works like that in Rust and after researching a bit I think it does make sense and so it should work the same way in Noir.

There are a couple of ways to try to convince ourselves that this makes sense :-)

For example, this code in Noir (and Rust too):

fn foo<T>() -> Field {
    1
}

fn main() {
    let _ = foo();
}

There's a type parameter T but what is it inferred to in the call? There's no way to infer it so we get this error:

error: Type annotation needed
  ┌─ src/main.nr:6:13
  │
6 │     let _ = foo();
  │             --- Could not determine the type of the generic argument `T` declared on the function `foo`

So, in general, if a type parameter cannot be deduced it will give an error. And, if it's unused, it will definitely give an error (maybe both Noir and Rust should give an error on the function definition instead of the call, but that's not the case right now).

What if we have this instead?

struct Foo {}

impl<T> Foo {
    fn foo() -> Field {
        1
    }
}

fn main() {
    let _ = Foo::foo();
}

What's T in that impl when we call Foo::foo()? It can't be inferred, so it leads to an error. Actually, if you try that right now in Noir, it gives this error:

error: Type annotation needed
   ┌─ src/main.nr:10:18
   │
10 │     let _ = Foo::foo();
   │                  --- Could not determine type of generic argument

However, we have a bug in the Noir compiler where if T is a numeric generic (let T: u32) then it won't error in this case. But it's a bug! Once it's fixed it will lead to the same error. The solution here too is to move the T to the functions that use it.

So, another way to see this is: there's a bug that doesn't trigger this error. Once it's fixed, the code in Aztec-Packages will fail to compile anyway, it's just that by requiring that T appears in Foo or instead be moved into relevant functions it fixes the issue for every function in that impl, and also matches Rust's behavior.

[TomAFrench]

I find it very strange that the where clase in the impl is not considered as 'being in use', given that the only change is that where will be in the fns instead of the entire impl.

The explanation which makes sense for me is that for any type T, it can only implement a trait once.

impl<T, U> PublicMutable<T>
where
    T: Serialize<U>,
{
    fn serialize() {}
}

This code implies a whole family of trait implementations for PublicMutable on T which breaks this property.

[TomAFrench]

LGTM