Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Arithmetic on numeric generics #4784

Closed
nventuro opened this issue Apr 11, 2024 · 2 comments · Fixed by #5625 or #5950
Closed

Arithmetic on numeric generics #4784

nventuro opened this issue Apr 11, 2024 · 2 comments · Fixed by #5625 or #5950
Assignees
Labels
aztec.nr Helpful for development of Aztec.nr the smart contract framework enhancement New feature or request

Comments

@nventuro
Copy link
Contributor

nventuro commented Apr 11, 2024

Problem

There's multiple design patterns that result in the need to declare generic array lenghts that are calculated on some other generic value.

Examples include:

impl<T, const N: usize> [T; N] {
    fn split_first(self) -> (T, [T; N - 1]) {
        ...
    }
}

or

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

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

impl<T, N> Serialize<N * 2> for Foo<T> {
    fn serialize(self) -> [Field; N * 2] where T: Serialize<N> {
      ...
    }
}

Happy Case

I'd want to be able to both have numeric generics without having to resort to the workaround from #4633, and to be able to perform basic arithmetic on them. My current use case is to do addition to both other generics and constants (e.g. N + N + 1), but it seems multiplication and subtraction are common patterns as well.

Project Impact

Blocker

Impact Context

Lack of this feature blocks multiple things, such as flatten as requested by @LogvinovLeon here, or AztecProtocol/aztec-packages#5492 which is required for non-trivial aztec-nr data structures.

This is also related to #4633, since many of the above fall into scenarios where the current workaround to force a generic value to be numeric (i.e. make it an array length) don't work.

Workaround

None

@nventuro nventuro added the enhancement New feature or request label Apr 11, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Apr 11, 2024
@Savio-Sou Savio-Sou added the aztec.nr Helpful for development of Aztec.nr the smart contract framework label Apr 12, 2024
@TomAFrench TomAFrench moved this from 📋 Backlog to 🏗 In progress in Noir May 3, 2024
@Thunkar
Copy link
Contributor

Thunkar commented May 8, 2024

Would be awesome to also have the modulo operation (mainly for AES)

pub fn compute_ciphertext<N>(
       self,
       secret: GrumpkinPrivateKey,
       point: GrumpkinPoint
   ) -> [u8; N + 16 - N % 16] where Note: NoteInterface<N> {
...

@jfecher
Copy link
Contributor

jfecher commented Jun 3, 2024

The Noir team has been thinking about this issue a lot and I have a few thoughts here as well.

  • It seems like slices could be usable for Serialize here now. We've avoided using them in the past but they should be bug free now that nested slices are not allowed. Since nested slices shouldn't be needed here this could be an option, although there could also be some performance implications with appending slices.
  • Another option could be changing the Serialize interface so that it accepts a large enough buffer and the generic is no longer needed:
trait Serialize {
    fn serialize<N>(self, state: &mut SerializeState<N>);
}

struct SerializeState<N> {
    buffer: [Field; N],
    index: u64,
}

impl<T, M> Serialize for [T; M] where T: Serialize {
    fn serialize<N>(self, state: &mut SerializeState<N>) {
        for i in 0 .. M {
            self[i].serialize(state);
        }
    }
}

impl Serialize for Field {
    fn serialize<N>(self, state: &mut SerializeState<N>) {
        state.buffer[state.index] = self;
        state.index += 1;
    }
}

struct Foo { x: [u64; 3], y: Field }

impl Serialize for Foo {
    fn serialize<N>(self, state: &mut SerializeState<N>) {
        self.x.serialize(state);
        self.y.serialize(state);
    }
}

The caveat of course being that main would need to supply a buffer that is large enough:

fn serialize_helper(x: impl Serialize, buffer: [Field; N]) -> [Field; N]) {
    let mut state = SerializeState { buffer, index: 0 };
    x.serialize(&mut state);
    state.buffer
}

fn main(foo: Foo) {
    let serialized = serialize_helper(foo, [0; 10]);
}

Once we have metaprogramming we'd be able to write a function which can give us the size of a type:

// Assuming we have
comptime fn size_of_type(typ: Type) -> u64 { ... }

// We could write a macro to make a large enough array
comptime fn make_buffer(typ: Type) -> Expr {
    let mut vec = Vec::new();
    for i in 0 .. size_of_type(typ) {
        vec.push(0);
    }
    x.into_array() // Returns a code snippet of an array of zeroes
}

/// This needs to be a comptime function now so that we can get the type
/// of x without it returning an opaque generic type like `T`
comptime fn serialize_helper(x: TypedExpr, buffer: [Field; N]) -> Expr {
    let buffer = make_buffer!(x.get_type());

    quote {
        let mut state = SerializeState { buffer: $buffer, index: 0 };
        x.serialize(&mut state);
        state.buffer
    }
}

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

The downside of the approach is that you'd still need to pass in the buffer length explicitly if serialize_helper is operating on a generic argument, e.g:

fn bar<T>(foo: T) where T: Serialize {
    // Error here, size_of_type(foo) is unknown
    let serialized = serialize_helper(foo);
}

This could possibly be resolved by a T: Sized constraint which returns a compile-time size however. Another caveat is that the size of each type here is separated from the Serialize function so we may not want a [bool; 4] to be 4 fields large if we could pack them into one field.

github-merge-queue bot pushed a commit that referenced this issue Aug 2, 2024
# Description

## Problem\*

Resolves #4784

## Summary\*

Adds a limited form of arithmetic operators on numeric generic types in
Noir. This includes only 5 operations: +, -, *, /, and %.

Going forward I'm going to distinguish between two definitions for
clearer communication between devs & users:
- Numeric generics: generics that use numeric type variables like `fn
foo<let N: u32>()`
- Arithmetic generics: when arithmetic operators are used on numeric
generics in a type position: `fn foo<let N: u32>() -> [Field; N + 1]`

## Additional Context

It is explicitly a _non-goal_ to always correctly return when two
_arithmetic generics_ are equal. For example, in our current type
checking we do not check whether applying the distributive law would
make two types equal. So you'd get a type error for trying to use `2*(N
+ 1)` where `2*N + 2` was expected. Users should generally consider the
type equality here to be largely structural and try to match that as
best as possible. ~~The only check performed besides simplification of
constant terms is commutativity for `+` and `*`. Even then, the check is
only one level deep. So `N * M * 3` is not equal to `3 * N * M`.~~
Update: we canonicalize commutative operations now, so we sort this and
it now type checks.

## Documentation\*

Check one:
- [ ] No documentation needed.
- [ ] Documentation included in this PR.
- [x] **[For Experimental Features]** Documentation to be submitted in a
separate PR.
- I'd like to have this in for some time before we start advertising it.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.

---------

Co-authored-by: Maxim Vezenov <[email protected]>
@github-project-automation github-project-automation bot moved this from 🏗 In progress to ✅ Done in Noir Aug 2, 2024
@jfecher jfecher mentioned this issue Sep 5, 2024
5 tasks
github-merge-queue bot pushed a commit that referenced this issue Sep 6, 2024
# Description

## Problem\*

Resolves #4784 for users who
haven't discovered the secret `--arithmetic-generics` flag

## Summary\*

Enables the `--arithmetic-generics` flag by default and provides
documentation for arithmetic generics.

## Additional Context

I'm open to discussion on whether we should test this more before
releasing but since the additional type checks and overflow checks that
were originally blocking it are implemented I figured I'd make this PR.

## Documentation\*

Check one:
- [ ] No documentation needed.
- [x] Documentation included in this PR.
- [ ] **[For Experimental Features]** Documentation to be submitted in a
separate PR.

# PR Checklist\*

- [x] I have tested the changes locally.
- [x] I have formatted the changes with [Prettier](https://prettier.io/)
and/or `cargo fmt` on default settings.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aztec.nr Helpful for development of Aztec.nr the smart contract framework enhancement New feature or request
Projects
Archived in project
5 participants