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

feat: Implement generic traits #4000

Merged
merged 15 commits into from
Jan 16, 2024
Merged

feat: Implement generic traits #4000

merged 15 commits into from
Jan 16, 2024

Conversation

jfecher
Copy link
Contributor

@jfecher jfecher commented Jan 10, 2024

Description

Problem*

Resolves #3471
Resolves #3474

Summary*

Implements support for generics on the trait directly. E.g. trait Into<T> { ... }

Additional Context

The old trait_generics test has been renamed to trait_impl_generics - I think this is more accurate. There is a new test in trait_generics now which tests the generic traits added by this PR.

I've discovered two new bugs in writing this PR, which are commented in the trait_generics test. I'll make issues for them now.

Documentation*

Check one:

  • No documentation needed.
  • Documentation included in this PR.
  • [Exceptional Case] Documentation to be submitted in a separate PR.

PR Checklist*

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt on default settings.

@github-actions github-actions bot added the documentation Improvements or additions to documentation label Jan 10, 2024
@jfecher
Copy link
Contributor Author

jfecher commented Jan 10, 2024

Issue/PR #4000 🎉

@jfecher
Copy link
Contributor Author

jfecher commented Jan 10, 2024

Ah, forgot to update some code I commented out. PR is not done yet, apologies.

Copy link
Contributor

github-actions bot commented Jan 10, 2024

@Thunkar
Copy link
Contributor

Thunkar commented Jan 10, 2024

Just checked to avoid duplicating work. This fixes #3474 !

False alarm, will keep working on it

Yup, it is fixed

@jfecher
Copy link
Contributor Author

jfecher commented Jan 10, 2024

@Thunkar I didn't finish rewriting the check for the parameters / return type of a trait method yet at the time. I tested again after fixing the trait_impl_generics test and #3474 is fixed now in this PR

@jfecher
Copy link
Contributor Author

jfecher commented Jan 10, 2024

@kevaundray @TomAFrench @vezenovm, this PR is ready to review again

@sirasistant
Copy link
Contributor

sirasistant commented Jan 11, 2024

One question, this fails on me with the code in this PR:

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

global DATA_LEN = 2;

struct Data {
    a: Field,
    b: Field,
}

impl Serializable<DATA_LEN> for Data {
    fn serialize(self) -> [Field; DATA_LEN] {
        [self.a, self.b]
    }
}

fn sum<T, N>(data: T) -> Field where T: Serializable<N> {
    let serialized = data.serialize();
    serialized.fold(0, |acc, elem| acc + elem)
}

fn main(data: Data) -> pub Field {
    sum(data)
}

It generates invalid SSA:

Initial SSA:
acir fn main f0 {
  b0(v0: Field, v1: Field):
    v3 = call f1(v0, v1)
    return v3
}
acir fn sum f1 {
  b0(v0: Field, v1: Field):
    v3, v4 = call f2(v0, v1) 
    inc_rc v4
    inc_rc v4
    v8 = call f3(v3, v4, Field 0, f4)
    return v8
}
acir fn serialize f2 {
  b0(v0: Field, v1: Field):
    return [v0, v1]
}
acir fn fold f3 {
  b0(v0: Field, v1: [Field], v2: Field, v4: function):
    v3 = allocate
    store v2 at v3
    inc_rc v1
    inc_rc v1
    jmp b1(Field 0)
  b1(v5: Field):
    v8 = lt v5, v0
    jmpif v8 then: b2, else: b3
  b2():
    v10 = cast v0 as u64
    v11 = lt v5, v10
    constrain v11 == u1 1 'Index out of bounds'
    v13 = array_get v1, index v5
    v14 = load v3
    v15 = call v4(v14, v13)
    store v15 at v3
    v16 = add v5, Field 1
    jmp b1(v16)
  b3():
    v17 = load v3
    return v17
}
acir fn lambda f4 {
  b0(v0: Field, v1: Field):
    v2 = add v0, v1
    return v2
}

It thinks that serialize returns a slice instead of a fixed length array
Crashes with

The application panicked (crashed).
Message:  assertion failed: `(left == right)`
  left: `2`,
 right: `1`
Location: compiler/noirc_evaluator/src/ssa/opt/inlining.rs:420

I think it fails to inline serialize because the return values in serialize (one return value, an array) don't match with the return values at the callsite (two return values, one length and one slice)

@jfecher
Copy link
Contributor Author

jfecher commented Jan 16, 2024

Thanks for the test @sirasistant, it turned out to be a very good one since it highlighted a lot of requirements I hadn't yet done. I had thought I was missing something since the original PR was quite small, and indeed I was missing a lot. I also thought the Into<T> test was sufficient, but your example highlights the need to test more. I added your example as a test, and added a variant of it where the trait method is called with a static method which gave me some additional issues while testing that are resolved now.

This PR should be ready for review now.

@kevaundray kevaundray added this pull request to the merge queue Jan 16, 2024
Merged via the queue into master with commit 916fd15 Jan 16, 2024
30 checks passed
@kevaundray kevaundray deleted the jf/trait-generics branch January 16, 2024 19:46
github-merge-queue bot pushed a commit that referenced this pull request Jan 22, 2024
🤖 I have created a release *beep* *boop*
---


<details><summary>0.23.0</summary>

## [0.23.0](v0.22.0...v0.23.0)
(2024-01-22)


### ⚠ BREAKING CHANGES

* Ban nested slices
([#4018](#4018))
* Breaking changes from aztec-packages
([#3955](#3955))
* Rename Arithmetic opcode to AssertZero
([#3840](#3840))
* remove circuit methods from noir_wasm
([#3869](#3869))

### Features

* Add `assert_max_bit_size` method to `Field`
([#4016](#4016))
([bc9a44f](bc9a44f))
* Add `noir-compiler` checks to `aztec_macros`
([#4031](#4031))
([420a5c7](420a5c7))
* Add a `--force` flag to force a full recompile
([#4054](#4054))
([27a8e68](27a8e68))
* Add dependency resolver for `noir_wasm` and implement `FileManager`
for consistency with native interface
([#3891](#3891))
([c29c7d7](c29c7d7))
* Add foreign call support to `noir_codegen` functions
([#3933](#3933))
([e5e52a8](e5e52a8))
* Add MVP `nargo export` command
([#3870](#3870))
([fbb51ed](fbb51ed))
* Add support for codegenning multiple functions which use the same
structs in their interface
([#3868](#3868))
([1dcfcc5](1dcfcc5))
* Added efficient field comparisons for bn254
([#4042](#4042))
([1f9cad0](1f9cad0))
* Assert maximum bit size when creating a U128 from an integer
([#4024](#4024))
([8f9c7e4](8f9c7e4))
* Avoid unnecessary range checks by inspecting instructions for casts
([#4039](#4039))
([378c18e](378c18e))
* Breaking changes from aztec-packages
([#3955](#3955))
([5be049e](5be049e))
* Bubble up `Instruction::Constrain`s to be applied as early as
possible. ([#4065](#4065))
([66f5cdd](66f5cdd))
* Cached LSP parsing
([#4083](#4083))
([b4f724e](b4f724e))
* Comparison for signed integers
([#3873](#3873))
([bcbd49b](bcbd49b))
* Decompose `Instruction::Cast` to have an explicit truncation
instruction ([#3946](#3946))
([35f18ef](35f18ef))
* Decompose `Instruction::Constrain` into multiple more basic
constraints ([#3892](#3892))
([51cf9d3](51cf9d3))
* Docker testing flow
([#3895](#3895))
([179c90d](179c90d))
* Extract parsing to its own pass and do it in parallel
([#4063](#4063))
([569cbbc](569cbbc))
* Implement `Eq` trait on curve points
([#3944](#3944))
([abf751a](abf751a))
* Implement DAP protocol in Nargo
([#3627](#3627))
([13834d4](13834d4))
* Implement generic traits
([#4000](#4000))
([916fd15](916fd15))
* Implement Operator Overloading
([#3931](#3931))
([4b16090](4b16090))
* **lsp:** Cache definitions for goto requests
([#3930](#3930))
([4a2140f](4a2140f))
* **lsp:** Goto global
([#4043](#4043))
([15237b3](15237b3))
* **lsp:** Goto struct member inside Impl method
([#3918](#3918))
([99c2c5a](99c2c5a))
* **lsp:** Goto trait from trait impl
([#3956](#3956))
([eb566e2](eb566e2))
* **lsp:** Goto trait method declaration
([#3991](#3991))
([eb79166](eb79166))
* **lsp:** Goto type alias
([#4061](#4061))
([dc83385](dc83385))
* **lsp:** Goto type definition
([#4029](#4029))
([8bb4ddf](8bb4ddf))
* **lsp:** Re-add code lens feature with improved performance
([#3829](#3829))
([8f5cd6c](8f5cd6c))
* Optimize array ops for arrays of structs
([#4027](#4027))
([c9ec0d8](c9ec0d8))
* Optimize logic gate ACIR-gen
([#3897](#3897))
([926460a](926460a))
* Prefer `AcirContext`-native methods for performing logic operations
([#3898](#3898))
([0ec39b8](0ec39b8))
* Remove range constraints from witnesses which are constrained to be
constants ([#3928](#3928))
([afe9c7a](afe9c7a))
* Remove truncation from brillig casts
([#3997](#3997))
([857ff97](857ff97))
* Remove truncations which can be seen to be noops using type
information ([#3953](#3953))
([cc3c2c2](cc3c2c2))
* Remove unnecessary predicate from `Lt` instruction
([#3922](#3922))
([a63433f](a63433f))
* Simplify chains of casts to be all in terms of the original `ValueId`
([#3984](#3984))
([2384d3e](2384d3e))
* Simplify multiplications by `0` or `1` in ACIR gen
([#3924](#3924))
([e58844d](e58844d))
* Support for u128
([#3913](#3913))
([b4911dc](b4911dc))
* Support printing more types
([#4071](#4071))
([f5c4632](f5c4632))
* Sync `aztec-packages`
([#4011](#4011))
([fee2452](fee2452))
* Sync commits from `aztec-packages`
([#4068](#4068))
([7a8f3a3](7a8f3a3))
* Use singleton `WasmBlackBoxFunctionSolver` in `noir_js`
([#3966](#3966))
([10b28de](10b28de))


### Bug Fixes

* Acir gen doesn't panic on unsupported BB function
([#3866](#3866))
([34fd978](34fd978))
* Allow abi encoding arrays of structs from JS
([#3867](#3867))
([9b713f8](9b713f8))
* Allow abi encoding tuples from JS
([#3894](#3894))
([f7fa181](f7fa181))
* Allow ast when macro errors
([#4005](#4005))
([efccec3](efccec3))
* Allow lsp to run inside of a docker container
([#3876](#3876))
([2529977](2529977))
* Bit-shifts for signed integers
([#3890](#3890))
([6ddd98a](6ddd98a))
* Checks for cyclic dependencies
([#3699](#3699))
([642011a](642011a))
* **debugger:** Crash when stepping through locations spanning multiple
lines ([#3920](#3920))
([223e860](223e860))
* Don't fail if no tests and the user didn't provide a pattern
([#3864](#3864))
([decbd0f](decbd0f))
* Fix advisory issue in cargo-deny
([#4077](#4077))
([19baea0](19baea0))
* Fixing dark mode background on the CTA button
([#3882](#3882))
([57eae42](57eae42))
* Fixup exports from `noir_wasm`
([#4022](#4022))
([358cdd2](358cdd2))
* Handle multiple imports in the same file
([#3903](#3903))
([219423e](219423e))
* Hoist constraints on inputs to top of program
([#4076](#4076))
([447aa34](447aa34))
* Implement missing codegen for `BlackBoxFunc::EcdsaSecp256r1` in
brillig ([#3943](#3943))
([2c5eceb](2c5eceb))
* Improve `nargo test` output
([#3973](#3973))
([3ab5ff4](3ab5ff4))
* Make `constant_to_radix` emit a slice instead of an array
([#4049](#4049))
([5cdb1d0](5cdb1d0))
* Operator overloading & static trait method references resolving to
generic impls ([#3967](#3967))
([f1de8fa](f1de8fa))
* Preserve brillig entrypoint functions without arguments
([#3951](#3951))
([1111465](1111465))
* Prevent `Instruction::Constrain`s for non-primitive types
([#3916](#3916))
([467948f](467948f))
* Remove panic for adding an invalid crate name in wasm compiler
([#3977](#3977))
([7a1baa5](7a1baa5))
* Return error rather instead of panicking on invalid circuit
([#3976](#3976))
([67201bf](67201bf))
* Search all levels of struct nesting before codegenning primitive types
([#3970](#3970))
([13ae014](13ae014))
* Update generics docs to mention we have traits now
([#3980](#3980))
([c2acdf1](c2acdf1))


### Miscellaneous Chores

* Ban nested slices
([#4018](#4018))
([f8a1fb7](f8a1fb7))
* Remove circuit methods from noir_wasm
([#3869](#3869))
([12d884e](12d884e))
* Rename Arithmetic opcode to AssertZero
([#3840](#3840))
([836f171](836f171))
</details>

<details><summary>0.39.0</summary>

## [0.39.0](v0.38.0...v0.39.0)
(2024-01-22)


### ⚠ BREAKING CHANGES

* Breaking changes from aztec-packages
([#3955](#3955))
* Rename Arithmetic opcode to AssertZero
([#3840](#3840))
* Remove unused methods on ACIR opcodes
([#3841](#3841))
* Remove partial backend feature
([#3805](#3805))

### Features

* Aztec-packages
([#3754](#3754))
([c043265](c043265))
* Breaking changes from aztec-packages
([#3955](#3955))
([5be049e](5be049e))
* Remove range constraints from witnesses which are constrained to be
constants ([#3928](#3928))
([afe9c7a](afe9c7a))
* Speed up transformation of debug messages
([#3815](#3815))
([2a8af1e](2a8af1e))
* Sync `aztec-packages`
([#4011](#4011))
([fee2452](fee2452))
* Sync commits from `aztec-packages`
([#4068](#4068))
([7a8f3a3](7a8f3a3))


### Bug Fixes

* Deserialize odd length hex literals
([#3747](#3747))
([4000fb2](4000fb2))
* Return error rather instead of panicking on invalid circuit
([#3976](#3976))
([67201bf](67201bf))


### Miscellaneous Chores

* Remove partial backend feature
([#3805](#3805))
([0383100](0383100))
* Remove unused methods on ACIR opcodes
([#3841](#3841))
([9e5d0e8](9e5d0e8))
* Rename Arithmetic opcode to AssertZero
([#3840](#3840))
([836f171](836f171))
</details>

---
This PR was generated with [Release
Please](https://github.com/googleapis/release-please). See
[documentation](https://github.com/googleapis/release-please#release-please).

---------

Co-authored-by: TomAFrench <[email protected]>
github-merge-queue bot pushed a commit that referenced this pull request Jan 26, 2024
# Description

## Problem\*

Resolves #4124
Resolves #4095

## Summary\*

We were never applying trait constraints from method calls before. These
have been handled for other identifiers since #4000, but not for method
calls which desugar to a function identifier that is called, then type
checked with its own special function. I've fixed this by removing the
special function and recursively type checking the function call they
desugar to instead. This way we have less code duplication and only need
to fix things in one spot in the future.

## Additional Context

It is a good day when you get to fix a bug by removing code.

This is a draft currently because I still need:
- [x] To add `&mut` implicitly where applicable to the function calls
that are now checked recursively
- [x] To add the test case I'm using locally

## Documentation\*

Check one:
- [x] No documentation needed.
- [ ] Documentation included in this PR.
- [ ] **[Exceptional Case]** 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.
Thunkar added a commit to AztecProtocol/aztec-packages that referenced this pull request Jan 30, 2024
Closes: #3756,
#2838


Taking advantage of @jfecher's fantastic work in
noir-lang/noir#4000, implemented `Serialize<N>`
and `Deserialize<N>`. Together with `NoteInterface`, they make possible
getting rid of all the serialization interfaces, which greatly simplify
how the storage is handled and opens the door to further improvements.
~~Still some clutter to go, the lengths are still needed in some
places.~~


![Brace yourself](https://i.imgflip.com/8crkve.jpg)

I'm so sorry.

---------

Co-authored-by: sirasistant <[email protected]>
AztecBot pushed a commit to AztecProtocol/aztec-nr that referenced this pull request Jan 31, 2024
Closes: AztecProtocol/aztec-packages#3756,
AztecProtocol/aztec-packages#2838


Taking advantage of @jfecher's fantastic work in
noir-lang/noir#4000, implemented `Serialize<N>`
and `Deserialize<N>`. Together with `NoteInterface`, they make possible
getting rid of all the serialization interfaces, which greatly simplify
how the storage is handled and opens the door to further improvements.
~~Still some clutter to go, the lengths are still needed in some
places.~~


![Brace yourself](https://i.imgflip.com/8crkve.jpg)

I'm so sorry.

---------

Co-authored-by: sirasistant <[email protected]>
@Savio-Sou Savio-Sou mentioned this pull request Feb 15, 2024
michaelelliot pushed a commit to Swoir/noir_rs that referenced this pull request Feb 28, 2024
…ocol#4135)

Closes: AztecProtocol#3756,
AztecProtocol#2838


Taking advantage of @jfecher's fantastic work in
noir-lang/noir#4000, implemented `Serialize<N>`
and `Deserialize<N>`. Together with `NoteInterface`, they make possible
getting rid of all the serialization interfaces, which greatly simplify
how the storage is handled and opens the door to further improvements.
~~Still some clutter to go, the lengths are still needed in some
places.~~


![Brace yourself](https://i.imgflip.com/8crkve.jpg)

I'm so sorry.

---------

Co-authored-by: sirasistant <[email protected]>
superstar0402 added a commit to superstar0402/aztec-nr that referenced this pull request Aug 16, 2024
Closes: AztecProtocol/aztec-packages#3756,
AztecProtocol/aztec-packages#2838


Taking advantage of @jfecher's fantastic work in
noir-lang/noir#4000, implemented `Serialize<N>`
and `Deserialize<N>`. Together with `NoteInterface`, they make possible
getting rid of all the serialization interfaces, which greatly simplify
how the storage is handled and opens the door to further improvements.
~~Still some clutter to go, the lengths are still needed in some
places.~~


![Brace yourself](https://i.imgflip.com/8crkve.jpg)

I'm so sorry.

---------

Co-authored-by: sirasistant <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Traits should allow methods with generic return types Support generic traits
4 participants