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

Explicitly specify generic numerical parameters #4633

Closed
nventuro opened this issue Mar 25, 2024 · 3 comments · Fixed by #5155
Closed

Explicitly specify generic numerical parameters #4633

nventuro opened this issue Mar 25, 2024 · 3 comments · Fixed by #5155
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 Mar 25, 2024

Problem

I want to write functions that are generic over some parameter N, where N is not a type but a numeric value. This is currently supported only if N is used as an array length in a struct definition or function parameter.

Happy Case

With structs

The following code fails to compile with N is not in scope. While perhaps convoluted, it is a real example of a pattern I'm working on in Aztec-nr:

struct MyStruct<N> {
  a: Field,
}

impl<N> MyStruct<N> {
  fn foo(self) -> Field {
    self.a + N
  }
}

This can be currently worked around by adding a dummy array, as suggested by @jfecher:

struct MyStruct<N> {
  a: Field,
  _dummy: [Field, N],
}

This workaround seems good enough for use cases in which the struct has other data, as the only side effect is slightly wonky construction.

Without structs

If the impl functions don't receive self, then the workaround presented above does not work. This prevents implementing patterns of the form range().map().reduce i.e. doing something a number of times, collecting that into an array and then using that array to produce a value. An example of this is public storage reads:

trait Deserialize<N> {
    fn deserialize(fields: [Field; N]) -> Self;
}

#[oracle(storageRead)]
fn storage_read() -> Field {}

struct PublicStorage {}

impl PublicStorage {
    fn read<T, N>() -> T where T: Deserialize<N> {
        let mut fields = [0; N];
        for i in 0..N {
            fields[i] = storage_read();
        }
        T::deserialize(fields)
    }
}

The above does not compile with N is not in scope for the 0..N part. I could not find a way to make it work, except by making N a type parameter for PublicStorage, adding the dummy variable and making read take self (which I'd rather avoid).

This has not been an issue so far because we read storage in an array, but with AztecProtocol/aztec-packages#5153 this is going to change, and it is already like that for historical public storage reads in private, which are done one at a time.

Project Impact

Blocker

@nventuro nventuro added the enhancement New feature or request label Mar 25, 2024
@github-project-automation github-project-automation bot moved this to 📋 Backlog in Noir Mar 25, 2024
@Savio-Sou Savio-Sou added the aztec.nr Helpful for development of Aztec.nr the smart contract framework label Apr 1, 2024
nventuro added a commit to AztecProtocol/aztec-packages that referenced this issue Apr 10, 2024
(Large) part of
#4761.

This is an initial implementation of `SharedMutableStorage`, with some
limitations. I think those are best worked on in follow-up PRs, once we
have the bones working.

The bulk of the SharedMutable pattern is in `ScheduledValueChange`, a
pure Noir struct that has all of the block number related logic.
`SharedMutable` then makes a state variable out of that struct, adding
public storage access both in public and private (via historical reads -
see #5379), and using the new `request_max_block_number` function (from
#5251).

I made an effort to test as much as I could of these in Noir, with
partial success in the case of `SharedMutable` due to lack of certain
features, notably noir-lang/noir#4652. There
is also an end-to-end test that goes through two scheuled value changes,
showing that scheduled values do not affect the current one.

I added some inline docs but didn't include proper docsite pages yet so
that we can discuss the implementation, API, etc., and make e.g.
renamings less troublesome.

### Notable implementation details

I chose to make the delay a type parameter instead of a value mostly
because of two reasons:
- it lets us nicely serialize and deserialize `ScheduledValueChange`
without including this field (which we are not currently interested in
storing)
- it lets us declare a state variable of type `SharedMutable<T, DELAY>`
without having to change the signature of the `new` function, which is
automatically injected by the macro.

Overall I think this is fine, especially since we may later make the
delay mutable (see below), but still worth noting.

Additionally, I created a simple `public_storage` module to get slightly
nicer API and encapsulation. This highlighted a Noir issue
(noir-lang/noir#4633), which currently only
affects public historical reads but will also affect current reads once
we migrate to using the AVM opcodes.

### Future work

- #5491
- #5492 (this
takes care of padding during storage slot allocation)
- #5501
- #5493

---------

Co-authored-by: Jan Beneš <janbenes1234@gmail.com>
AztecBot pushed a commit to AztecProtocol/aztec-nr that referenced this issue Apr 11, 2024
(Large) part of
AztecProtocol/aztec-packages#4761.

This is an initial implementation of `SharedMutableStorage`, with some
limitations. I think those are best worked on in follow-up PRs, once we
have the bones working.

The bulk of the SharedMutable pattern is in `ScheduledValueChange`, a
pure Noir struct that has all of the block number related logic.
`SharedMutable` then makes a state variable out of that struct, adding
public storage access both in public and private (via historical reads -
see #5379), and using the new `request_max_block_number` function (from
#5251).

I made an effort to test as much as I could of these in Noir, with
partial success in the case of `SharedMutable` due to lack of certain
features, notably noir-lang/noir#4652. There
is also an end-to-end test that goes through two scheuled value changes,
showing that scheduled values do not affect the current one.

I added some inline docs but didn't include proper docsite pages yet so
that we can discuss the implementation, API, etc., and make e.g.
renamings less troublesome.

### Notable implementation details

I chose to make the delay a type parameter instead of a value mostly
because of two reasons:
- it lets us nicely serialize and deserialize `ScheduledValueChange`
without including this field (which we are not currently interested in
storing)
- it lets us declare a state variable of type `SharedMutable<T, DELAY>`
without having to change the signature of the `new` function, which is
automatically injected by the macro.

Overall I think this is fine, especially since we may later make the
delay mutable (see below), but still worth noting.

Additionally, I created a simple `public_storage` module to get slightly
nicer API and encapsulation. This highlighted a Noir issue
(noir-lang/noir#4633), which currently only
affects public historical reads but will also affect current reads once
we migrate to using the AVM opcodes.

### Future work

- AztecProtocol/aztec-packages#5491
- AztecProtocol/aztec-packages#5492 (this
takes care of padding during storage slot allocation)
- AztecProtocol/aztec-packages#5501
- AztecProtocol/aztec-packages#5493

---------

Co-authored-by: Jan Beneš <janbenes1234@gmail.com>
@jfecher jfecher changed the title Generic numerical parameters Explicitly specify generic numerical parameters Apr 12, 2024
@TomAFrench TomAFrench changed the title Explicitly specify generic numerical parameters Add support for const generics May 3, 2024
@TomAFrench
Copy link
Member

Solution to this would end up being some form of const generics system.

@jfecher
Copy link
Contributor

jfecher commented May 3, 2024

Solution to this would end up being some form of const generics system.

We already have const generics, we just call them numeric generics. The thing we're lacking here is just a better way to tell the compiler "this type variable must be numeric, you can safely introduce it into the expression scope." Rust does this via fn foo<const N: usize>() I think we can do fn foo<N: usize>() if we wanted - not sure why Rust didn't leave out the const as well. It may be because now it shares the same syntax as a trait constraint. If we want to avoid this as well, we can't 100% follow rust here since we don't have a const keyword. If we want to avoid introducing a const keyword for this then perhaps let works best of the keywords we have right now: fn foo<let N: usize>.

@TomAFrench TomAFrench changed the title Add support for const generics Explicitly specify generic numerical parameters May 3, 2024
@TomAFrench
Copy link
Member

Yeah "system" -> "syntax" is closer to what I was thinking. Apologies for not being clear.

@TomAFrench TomAFrench assigned vezenovm and unassigned michaeljklein May 17, 2024
github-merge-queue bot pushed a commit that referenced this issue Jun 24, 2024
# Description

## Problem\*

Resolves #4633, but only in the elaborator as we will be moving to this
new infrastructure for the frontend soon and resolving edge cases across
both the old resolver and the elaborator was becoming a time sink.

## Summary\*

We now have an `UnresolvedGeneric` type rather than simply representing
generics using idents.
```rust
pub enum UnresolvedGeneric {
    Variable(Ident),
    Numeric { ident: Ident, typ: UnresolvedType },
}
```
We also have a new `ResolvedGeneric` struct when storing generics during
resolution and elaboration. We were previously storing a tuple of three
elements, but now that I need to distinguish when we have a numeric
generic it was simpler to make this a struct.

Some example usage:
```rust
// Used as a field of a struct
struct Foo<let N: u64> {
    inner: [u64; N],
}
fn baz<let N: u64>() -> Foo<N> {
    Foo { inner: [1; N] }
}
// Used in an impl
impl<let N: u64> Foo<N> {
    fn bar(self) -> u64 {
        N * self.inner[0]
    }
}
```
More examples can be found in `numeric_generics` and documentation will
come in a follow-up.

We make sure to error out when an explicit numeric generic is used as a
type.
For example, this code:
```rust
struct Trash<let N: u64> {
    a: Field,
    b: N,
}
fn id_two<let I: Field>(x: I) -> I {
    x
}
``` 
<img width="749" alt="Screenshot 2024-06-04 at 5 42 53 PM"
src="https://github.com/noir-lang/noir/assets/43554004/a515a508-7ea4-4a23-bfd3-6f7bbae28e55">


## Additional Context

We do not ban the old strategy for implicit numeric generics just yet as
a lot of code uses it. Currently this warning is issued:
<img width="965" alt="Screenshot 2024-06-04 at 11 34 19 AM"
src="https://github.com/noir-lang/noir/assets/43554004/60c79337-98b2-4e70-a11d-947f6611fc41">


## Documentation\*

Check one:
- [ ] No documentation needed.
- [ ] Documentation included in this PR.
- [X] **[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.

---------

Co-authored-by: jfecher <jake@aztecprotocol.com>
@github-project-automation github-project-automation bot moved this from 📋 Backlog to ✅ Done in Noir Jun 24, 2024
nventuro added a commit to AztecProtocol/aztec-packages that referenced this issue Jun 27, 2024
Even though noir-lang/noir#4633 is now closed,
we no longer need this function due to how
github.com//pull/7169 (the sole user of the
API) does historical proofs (i.e. it reads a single slot).
AztecBot pushed a commit to AztecProtocol/aztec-nr that referenced this issue Jun 28, 2024
Even though noir-lang/noir#4633 is now closed,
we no longer need this function due to how
github.com/AztecProtocol/aztec-packages/pull/7169 (the sole user of the
API) does historical proofs (i.e. it reads a single slot).
nventuro added a commit to AztecProtocol/aztec-packages that referenced this issue Jul 16, 2024
With noir-lang/noir#4633 merged we can now use
the new `let` syntax to mark parameters as numeric. I'm not entirely
sure why we need to specify it both on the type and the impl, @vezenovm
could you expand on this a little bit?
AztecBot pushed a commit to AztecProtocol/aztec-nr that referenced this issue Jul 17, 2024
With noir-lang/noir#4633 merged we can now use
the new `let` syntax to mark parameters as numeric. I'm not entirely
sure why we need to specify it both on the type and the impl, @vezenovm
could you expand on this a little bit?
superstar0402 added a commit to superstar0402/aztec-nr that referenced this issue Aug 16, 2024
(Large) part of
AztecProtocol/aztec-packages#4761.

This is an initial implementation of `SharedMutableStorage`, with some
limitations. I think those are best worked on in follow-up PRs, once we
have the bones working.

The bulk of the SharedMutable pattern is in `ScheduledValueChange`, a
pure Noir struct that has all of the block number related logic.
`SharedMutable` then makes a state variable out of that struct, adding
public storage access both in public and private (via historical reads -
see #5379), and using the new `request_max_block_number` function (from
#5251).

I made an effort to test as much as I could of these in Noir, with
partial success in the case of `SharedMutable` due to lack of certain
features, notably noir-lang/noir#4652. There
is also an end-to-end test that goes through two scheuled value changes,
showing that scheduled values do not affect the current one.

I added some inline docs but didn't include proper docsite pages yet so
that we can discuss the implementation, API, etc., and make e.g.
renamings less troublesome.

### Notable implementation details

I chose to make the delay a type parameter instead of a value mostly
because of two reasons:
- it lets us nicely serialize and deserialize `ScheduledValueChange`
without including this field (which we are not currently interested in
storing)
- it lets us declare a state variable of type `SharedMutable<T, DELAY>`
without having to change the signature of the `new` function, which is
automatically injected by the macro.

Overall I think this is fine, especially since we may later make the
delay mutable (see below), but still worth noting.

Additionally, I created a simple `public_storage` module to get slightly
nicer API and encapsulation. This highlighted a Noir issue
(noir-lang/noir#4633), which currently only
affects public historical reads but will also affect current reads once
we migrate to using the AVM opcodes.

### Future work

- AztecProtocol/aztec-packages#5491
- AztecProtocol/aztec-packages#5492 (this
takes care of padding during storage slot allocation)
- AztecProtocol/aztec-packages#5501
- AztecProtocol/aztec-packages#5493

---------

Co-authored-by: Jan Beneš <janbenes1234@gmail.com>
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
Status: Done
Development

Successfully merging a pull request may close this issue.

6 participants