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 traits - first prototype #2094

Closed

Conversation

yordanmadzhunkov
Copy link
Contributor

@yordanmadzhunkov yordanmadzhunkov commented Jul 31, 2023

Description

Implement traits #527
Add initial prototype implementation of Traits in Noir.

This is very experimental work and needs more feedback, progress and testing before it can be enabled to the users

Problem*

Add trait functionality to Noir language analogous to how traits are implemented in rust

Resolves

Summary*

The goal is to make this simple program compile & execute

trait Default {
    fn default(x: Field, y: Field) -> Self;
}

struct Foo {
    bar: Field,
    array: [Field; 2],
}

impl Default for Foo {
    fn default(x: Field,y: Field) -> Self {
        Self { bar: x, array: [x,y] }
    }
}

fn main(x: Field, y: Field) {
    let first = Foo::default(x,y);
    assert(first.bar == x);
}

The noir compiler emits the following messages. I accept all kind of suggestion how to improve them:

Example 1:

error: duplicate definitions of Default trait found
   ┌─ tests/compile_tests_data/fail/dup_trait_declaration.nr:3:7
   │
 3 │ trait Default {
   │       ------- first trait definition found here
   ·
19 │ trait Default {
   │       ------- second trait definition found here


Example 2:

error: mismatch return type of method with name default that implemetns trait Default
   ┌─ tests/compile_tests_data/fail/trait_wrong_method_return_type.nr:13:8
   │
13 │     fn default(x: Field, y: Field) -> Field {
   │        -------

error: Could not resolve 'default' in path
   ┌─ tests/compile_tests_data/fail/trait_wrong_method_return_type.nr:19:22
   │
19 │     let first = Foo::default(x,y);

Example 3:

error: method with name default_wrong_name is not part of trait Default, therefore it can't be implemented
   ┌─ tests/compile_tests_data/fail/trait_wrong_method_name.nr:14:8
   │
14 │     fn default_wrong_name(x: Field, y: Field) -> Self {
   │        ------------------

error: Could not resolve 'default_wrong_name' in path
   ┌─ tests/compile_tests_data/fail/trait_wrong_method_name.nr:20:22
   │
20 │     let first = Foo::default_wrong_name(x,y);
   │                      ------------------

Example 4

error: Trait Default not found
  ┌─ tests/compile_tests_data/fail/trait_not_exists.nr:9:6
  │
9 │ impl Default for Foo {
  │      -------

error: Could not resolve 'default' in path
   ┌─ tests/compile_tests_data/fail/trait_not_exists.nr:16:22
   │
16 │     let first = Foo::default(x,y);
   │                      -------

Example 5:

error: Mismatch signature [Number of parameters] of method with name `default` that implemetns trait `Default`
   ┌─ tests/compile_tests_data/fail/trait_wrong_parameters_count.nr:13:8
   │
13 │     fn default(x: Field) -> Self {

Example 6:

error: duplicate definitions of default function found
   ┌─ tests/compile_tests_data/fail/dup_trait_implementation.nr:14:8
   │
14 │     fn default(x: Field,y: Field) -> Self {
   │        ------- first function definition found here
   ·
21 │     fn default(x: Field, y: Field) -> Self {
   │        ------- second function definition found here

Example 7:

error: Mismatch signature of method default that implemtns trait Default
   ┌─ tests/compile_tests_data/fail/trait_wrong_parameter.nr:13:26
   │
13 │     fn default(x: Field, y: Foo) -> Self {
   │                          - `y: Field` expected

Example 8:

error: Default is not a trait, therefore it can't be implemented
   ┌─ tests/compile_tests_data/fail/impl_struct_not_trait.nr:14:6
   │
14 │ impl Default for Foo {

Documentation

  • This PR requires documentation updates when merged.

    • I will submit a noir-lang/docs PR.
    • I will request for and support Dev Rel's help in documenting this PR.

Additional Context

PR Checklist*

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

@yordanmadzhunkov yordanmadzhunkov changed the title Yordan/add traits Implement traits #527 Jul 31, 2023
@yordanmadzhunkov yordanmadzhunkov changed the title Implement traits #527 Implement traits #527 - first prototype Jul 31, 2023
@yordanmadzhunkov yordanmadzhunkov changed the title Implement traits #527 - first prototype feat Implement traits Jul 31, 2023
@yordanmadzhunkov yordanmadzhunkov changed the title feat Implement traits feat: Implement traits - first prototype Aug 1, 2023
Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you break this up into smaller PRs next time? This one is too large which makes it take longer to review (apologies for the delay) and contain more TODOs and stubs which prevent it from being merged.

We don't need to add name resolution, type checking, and monomorphization of traits in one PR. We can focus on e.g. one pass at a time and just add the minimum amount of features so that we have a clean base to build upon that can be expanded later.

crates/noirc_frontend/src/parser/errors.rs Outdated Show resolved Hide resolved
Comment on lines 373 to 375
if generics.len() > 0 || where_clause.len() > 0 {
emit(ParserError::with_reason(ParserErrorReason::TraitsAreExperimental, span));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why we wouldn't issue this error if the trait has no generics. Lets keep the traits are experimental error. We should probably change it to a warning now that we're iterating on traits so that we continue through proving.

crates/noirc_frontend/src/parser/parser.rs Outdated Show resolved Hide resolved
.then_ignore(just(Token::Colon))
.then(parse_type())
.then(optional_default_value())
.validate(|((name, typ), default_value), _span, _emit| TraitItem::Constant {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.validate(|((name, typ), default_value), _span, _emit| TraitItem::Constant {
.map(|((name, typ), default_value)| TraitItem::Constant {

@@ -124,6 +129,39 @@ pub struct StructType {
pub span: Span,
}

#[derive(Debug, PartialEq, Eq)]
pub enum TraitItemType {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub enum TraitItemType {
pub enum TraitItem {

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunetly, this conflicts with crates/noirc_frontend/src/ast/traits.rs: pub enum TraitItem

crates/noirc_frontend/src/hir_def/types.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/hir_def/types.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/monomorphization/mod.rs Outdated Show resolved Hide resolved
crates/noirc_frontend/src/node_interner.rs Outdated Show resolved Hide resolved
@yordanmadzhunkov
Copy link
Contributor Author

Can you break this up into smaller PRs next time? This one is too large which makes it take longer to review (apologies for the delay) and contain more TODOs and stubs which prevent it from being merged.

We don't need to add name resolution, type checking, and monomorphization of traits in one PR. We can focus on e.g. one pass at a time and just add the minimum amount of features so that we have a clean base to build upon that can be expanded later.

I also like to submit small PRs. I was not sure what is the correct approach for implementing traits, so I tried to make it work end-to-end in order to validate the correctness. The result is this BIG PR. In the near future, I will aim at smaller changes/PRs

@kevaundray
Copy link
Contributor

Can you break this up into smaller PRs next time? This one is too large which makes it take longer to review (apologies for the delay) and contain more TODOs and stubs which prevent it from being merged.
We don't need to add name resolution, type checking, and monomorphization of traits in one PR. We can focus on e.g. one pass at a time and just add the minimum amount of features so that we have a clean base to build upon that can be expanded later.

I also like to submit small PRs. I was not sure what is the correct approach for implementing traits, so I tried to make it work end-to-end in order to validate the correctness. The result is this BIG PR. In the near future, I will aim at smaller changes/PRs

Since larger PRs take quite a long time to review, I'd suggest that we break this PR down into smaller PRs and open an issue detailing the smaller chunks that are needed for implementing traits.

If this PR is mostly done, then it would be just copying the relevant parts into separate PRs

yordanmadzhunkov pushed a commit to metacraft-labs/noir that referenced this pull request Aug 9, 2023
* Add several parsing traits test
* Expand trait definition in AST to include body
* Add diagnostic error when where clause is not appled on generic type
github-merge-queue bot pushed a commit that referenced this pull request Aug 10, 2023
* feat: Implement traits - parser support #2094

* Add several parsing traits test
* Expand trait definition in AST to include body
* Add diagnostic error when where clause is not appled on generic type

* Update crates/noirc_frontend/src/parser/parser.rs

Co-authored-by: jfecher <jfecher11@gmail.com>

* make parse_all to ignore warnings and use it everywhere

---------

Co-authored-by: Yordan Madzhunkov <ymadzhunkov@gmail.com>
Co-authored-by: jfecher <jfecher11@gmail.com>
TomAFrench added a commit that referenced this pull request Aug 10, 2023
* master: (80 commits)
  fix: properly capture lvalues in closure environments (#2120) (#2257)
  fix: Optimize contracts built by `nargo info` (#2259)
  chore: impl Display for DebugType (#2258)
  chore: update `noir_wasm` build process to match `acvm_js` (#2067)
  feat: Implement traits - parser support #2094 (#2230)
  chore: Refactor DefCollector duplicate errors (#2231)
  chore: Address clippy warnings (#2246)
  feat: Support `contract` package type in `nargo info` command (#2249)
  feat: Add slice append (#2241)
  chore: Bump `async-lsp` to v0.0.5 (#2186)
  chore: Move the remaining `nargo_cli` lib funcs into `nargo` crate (#2225)
  chore: Add test for eddsa (#2237)
  chore: Split `Nargo.toml` operations into separate package (#2224)
  fix(stdlib): correct `tecurve::contains` formula (#1821)
  feat: Remove `comptime` and warn upon usage (#2178)
  fix: Remove last vestige of array of structs to struct of arrays conversion (#2217)
  fix: Add foreign impl error (#2216)
  feat(nargo)!: Replace `--contracts` flag with `contract` package type (#2204)
  feat: Optimize `x < 0` for unsigned `x` to false (#2206)
  fix: Initialize numeric generics' type to a polymorphic integer when used in an expression (#2179)
  ...
TomAFrench added a commit that referenced this pull request Aug 11, 2023
* master: (29 commits)
  feat(nargo): Add support for contracts in `nargo check` (#2267)
  chore(ci): Name wasm job more clearly (#2269)
  chore(ci): Create cache key with consideration to target (#2273)
  chore(ci): Run publish workflow against PRs (#2268)
  chore: Merge in contents of `build-nargo` repository (#2211)
  fix(lsp): Improve dependency resolution in context of `Nargo.toml` (#2226)
  chore: Remove unnecessary duplication in how we test Noir compiler (#2248)
  fix: properly capture lvalues in closure environments (#2120) (#2257)
  fix: Optimize contracts built by `nargo info` (#2259)
  chore: impl Display for DebugType (#2258)
  chore: update `noir_wasm` build process to match `acvm_js` (#2067)
  feat: Implement traits - parser support #2094 (#2230)
  chore: Refactor DefCollector duplicate errors (#2231)
  chore: Address clippy warnings (#2246)
  feat: Support `contract` package type in `nargo info` command (#2249)
  feat: Add slice append (#2241)
  chore: Bump `async-lsp` to v0.0.5 (#2186)
  chore: Move the remaining `nargo_cli` lib funcs into `nargo` crate (#2225)
  chore: Add test for eddsa (#2237)
  chore: Split `Nargo.toml` operations into separate package (#2224)
  ...
TomAFrench added a commit that referenced this pull request Aug 11, 2023
* master:
  feat(nargo): Add support for contracts in `nargo check` (#2267)
  chore(ci): Name wasm job more clearly (#2269)
  chore(ci): Create cache key with consideration to target (#2273)
  chore(ci): Run publish workflow against PRs (#2268)
  chore: Merge in contents of `build-nargo` repository (#2211)
  fix(lsp): Improve dependency resolution in context of `Nargo.toml` (#2226)
  chore: Remove unnecessary duplication in how we test Noir compiler (#2248)
  fix: properly capture lvalues in closure environments (#2120) (#2257)
  fix: Optimize contracts built by `nargo info` (#2259)
  chore: impl Display for DebugType (#2258)
  chore: update `noir_wasm` build process to match `acvm_js` (#2067)
  feat: Implement traits - parser support #2094 (#2230)
  chore: Refactor DefCollector duplicate errors (#2231)
  chore: Address clippy warnings (#2246)
  feat: Support `contract` package type in `nargo info` command (#2249)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants