feat!: Allow impls on primitive types

[jfecher]

Related issue(s)

Resolves #736

Description

Allows impls on primitive types and changes several library functions into methods. This PR does not verify these primitive impls are in the stdlib only as I could not find a good method to do so.

Note that this PR is currently expected to break almost all noir code in its changing of library functions into methods.

Test additions / changes

Updates many tests to the new method syntax

Checklist

• I have tested the changes locally.
• I have formatted the changes with Prettier and/or cargo fmt with default settings.
• I have linked this PR to the issue(s) that it resolves.
• I have reviewed the changes on GitHub, line by line.
• I have ensured all changes are covered in the description.
• This PR requires documentation updates when merged.

Additional context

What rule should we go by for which library functions should be methods and which (if any) should be free functions? So far, I've started by only making functions that take an argument of the same type into methods. This excludes nullary functions like std::field::modulus_num_bits for example.

[kevaundray]

Nice! Will look at this when I get back

[kevaundray]

Since this breaks a lot of noir code, we should push a release once this is merged so that we break less newer people's code.

Alternatively, maybe we could slowly deprécate those functions which are now methods?

[jfecher]

I think some discussion on which functions should or should not be methods may also be helpful. Having .pedersen() as a method for example makes sense to me and yet still feels rather awkward.

[jfecher]

Ah, also worth noting: this PR fixes #618 since for each loops now use array.len() internally rather than dep::std::array::len(array). So the problem with the path is no more.

[kevaundray]

I think some discussion on which functions should or should not be methods may also be helpful. Having .pedersen() as a method for example makes sense to me and yet still feels rather awkward.

I think for hash functions it feels more natural to have them as functions instead of methods, because unlike .len() there will be other implementations for a hash function that one may want to use, so theres nothing special about the current hash functions that we have in the stdlib, its just that they are the ones we had in our primary backend.

[kevaundray]

Ah, also worth noting: this PR fixes #618 since for each loops now use array.len() internally rather than dep::std::array::len(array). So the problem with the path is no more.

Nice, should we kee the issue open and make it more general, since this would fix it for array::len but not in general?

[jfecher]

I think for hash functions it feels more natural to have them as functions instead of methods, because unlike .len() there will be other implementations for a hash function that one may want to use, so theres nothing special about the current hash functions that we have in the stdlib, its just that they are the ones we had in our primary backend.

Alright, I'll change these methods back to functions then. Should reduce breakage in user code as a result. What are your thoughts on the std::field functions that were changed to methods?

Nice, should we kee the issue open and make it more general, since this would fix it for array::len but not in general?

I think it is fine to close it. It was an issue because the for-each syntax desugared internally using dep::std::array::len and there was no way to change this. Now that it does not do this, there is no other similar construct using a fixed path like this.

[vezenovm]

Alright, I'll change these methods back to functions then. Should reduce breakage in user code as a result. What are your thoughts on the std::field functions that were changed to methods?

I think std::field functions as methods makes sense as Field is a native compiler type.

let x: Field = 5;
let arr = x.to_le_bytes()

The snippet above feels natural to me as there is one implementation of this method for the Field type.

Hashes feel separate from this and it would be awkward to have something like .pedersen() as you mentioned above. We don't have a native Hash type, so I think keeping a module of different hash functions is good for now. We could bring some type of inheritance structure to the hash functions where all hash functions could implement a Hash trait with methods such as update([u8]) and finalize() -> [u8].

[kevaundray]

Left some comments, I like the change.

[kevaundray]

+1 to Maxim's comment -- Field looks good already. Need to go back over the specific methods that were changed to methods on Field as there might be some that we only want to be on Field, which may break IntOrField variant. Just something that came OTOH so may not be a problem.

An example would be if we had a method named shift_right which would only be for Integers.

[jfecher]

An example would be if we had a method named shift_right which would only be for Integers.

Note that the IntOrField variant only controls which functions are visible to integers or fields. The functions themselves can still say they only accept an int or a field (and indeed some methods in std::field do only accept fields). Using these methods on integers you will get a normal type error like you did before.

[kevaundray]

LGTM -- one nit regarding the if true, apart from that happy to merge

[vezenovm]

LGTM -- one nit regarding the if true, apart from that happy to merge

Did we decide whether we are going to merge this before a release or after? CI on the preprocess PR is almost finished

[kevaundray]

LGTM -- one nit regarding the if true, apart from that happy to merge

Did we decide whether we are going to merge this before a release or after? CI on the preprocess PR is almost finished

Was going to merge your one first as the last non-breaking change and push a release. Then this PR along with the other breaking change from Tom would go into the next release which we can cut tomorrow or early next week.