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: Traits methods - default and override #2574

Closed
wants to merge 32 commits into from

Conversation

yordanmadzhunkov
Copy link
Contributor

@yordanmadzhunkov yordanmadzhunkov commented Sep 6, 2023

#Description #2568
In this PR we introduce the following progress of implementation of traits:

  • Trait supports Self as type parameter or return type
  • Trait can have an default implementation that will execute if not override by the struct
  • Trait can have an empty implementation which forces the struct to provide one
  • Structs can implement traits and override the default implementation

Remove unnecessary checks:

  • type of trait implementation doesn't have to be struct
  • Remove the premature checks (from def collector), that should be done at a later phase:

Name resolution/Type checking:

  • Trait function parameters (should be done later, because the current code cannot deal correctly with e.g. type aliases)
  • function return type (same reason as the above)
  • Check for duplicate implementations for the same Trait and Type (this check should work even for empty traits!)
  • Other ( see tests in this PR)

Successful examples:

Self should typecheck properly

struct Foo {
    x: Field
}

trait Asd {
    fn asd() -> Self;
}

impl Asd for Foo {
   
    fn asd() -> Self {
        Foo{x: 100}
    }
}

fn main() {
    assert(Foo::asd().x == 100);

trait_default_implementation

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

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) {
    let first = Foo::method2(x);
    assert(first == x);
}

trait_override_implementation

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

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 method2(x: Field) -> Field {	
        x * 3    
    }
}

fn main(x: Field) {
    let first = Foo::method2(x);
    assert(first == 3 * x);
}

PR Checklist*

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

ymadzhunkov and others added 30 commits September 6, 2023 12:06
Because compile_failure tests are only checked for producing any compiler error,
without checking whether the compiler error is what we expect, it is best if
we keep these tests as simple as possible, so that they are only likely to fail
for a single reason.
…ry, where they're not executed

Tests that were added too early and fail for a known reason (not yet implemented
functionality in the compiler) are temporarily moved to the KNOWN_FAILURES
directory, so they're not executed by 'cargo test'. This is due to the fact that
the trait type checking was done way too early (in the def collector). After
realizing this, we had to remove this code, so we can implement it at a later
stage. However, the tests we added now become a chore, because they now fail for
a known reason - in the long run, we don't want to remove them, because they are
valid tests and should eventually pass. However, they hinder compiler
development, because there are several other steps that need to be implemented
in the compiler, before we can make them pass again. On the other hand, we want
to be able to run the other tests, and the output of 'cargo test' becomes ugly,
when there are failing tests. So, basically we want to postpone these tests for
later, but not forget about them. That's why we move them temporarily to a
KNOWN_FAILURES directory.
… duplicate impl of traits

Conclusions from the tests:
1) duplicate checks must happen for the impl itself, not just for the functions
   inside the trait impl
2) the duplicate check requires type resolution, therefore it must be done later
   (currently it's done in the def collector)
3) the test runner must be fixed to handle internal compiler errors (panic!) as
   a test failure for tests in compile_failure (compile_failure tests must
   produce a proper compiler error, not a compiler crash/internal error/panic!).
trait Default {
    fn default(x: Field, y: Field) -> Field;
}

impl Default for Zoo {
    fn default(x: Field, y: Field) -> Field {
        x + y
    }
}

error: Only struct types may implement trait `Default`
  ┌─ /home/yordan/Work/noir/crates/nargo_cli/tests/compile_failure/impl_trait_for_non_struct/src/main.nr:8:6
  │
8 │ impl Default for main {
  │      ------- Only struct types may implement traits
@yordanmadzhunkov
Copy link
Contributor Author

@jfecher I know it's a big PR, but I hope you like it :)

@yordanmadzhunkov yordanmadzhunkov changed the title Sofia/traits feat: Traits methods - default and override Sep 6, 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 split this into several PRs? It looks like this implements a lot of functionality so it makes it more difficult to judge what any change in particular is meant to implement. I think generally having an "other" category on a PR is a good indicator it can be split up.

Trait function parameters (should be done later, because the current code cannot deal correctly with e.g. type aliases)
function return type (same reason as the above)

We should do these type checks in the type checker when these changes are split into another PR. As you mentioned, the check would be more accurate this way, and I think it'd be easier to understand if the type checking was done during the type checking pass.

@yordanmadzhunkov
Copy link
Contributor Author

First part -> #2585

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