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

Compiler doesn't check duplicated trait implementation #870

Open
Y-Nak opened this issue Apr 17, 2023 · 5 comments
Open

Compiler doesn't check duplicated trait implementation #870

Y-Nak opened this issue Apr 17, 2023 · 5 comments
Labels
v2-resolve This issue will be resolved in fe-v2 implementation

Comments

@Y-Nak
Copy link
Collaborator

Y-Nak commented Apr 17, 2023

The compiler doesn't check duplicated trait implementation for the same type.

Example

// main.fe
use foo::*
impl Trait for Foo { }

// foo.fe
pub trait Trait { }
pub type Foo { }
impl Trait for Foo { }

expected: duplicate `impl` blocks for trait `Trait` for type `Foo`
actual: None

@Y-Nak Y-Nak added the v2-resolve This issue will be resolved in fe-v2 implementation label Apr 17, 2023
@BKDaugherty
Copy link

Hey @Y-Nak! Cool if I pick this up? Got an in-progress changeset I can probably cleanup later today that should resolve this.

~/Dev/my_first_fe_project 
❯ cat src/main.fe
use logic::*
impl Trait for Foo {
}

contract Main {
    pub fn use_bar() -> u256 {
        let x: Foo = Foo()
        return 1
    }
}%

~/Dev/my_first_fe_project 
❯ cat src/logic.fe
pub trait Trait { }
pub struct Foo { }
impl Trait for Foo { }

~/Dev/my_first_fe_project 
❯ fe build . --overwrite
Unable to compile ..
error: duplicate `impl` blocks for trait `Trait` for type `Foo`
  ┌─ ./src/logic.fe:3:1
  │
3 │ impl Trait for Foo { }
  │ ^^^^^^^^^^^^^^^^^^ `` first defined here
  │
  ┌─ ./src/main.fe:2:1
  │
2 │ impl Trait for Foo {
  │ ------------------ `` redefined here

@Y-Nak
Copy link
Collaborator Author

Y-Nak commented Apr 18, 2024

Hey @BKDaugherty, thanks for fixing this problem. But this issue is already fixed in ongoing fe-v2 implementation, which will replace almost all v1 codebase. An issue is related to v2 implementation If it is labeled v2,
I think it'd be fine to merge your effort to v1. any thoughts, @g-r-a-n-t?

@BKDaugherty
Copy link

BKDaugherty commented Apr 18, 2024

Oh awesome, glad it's already fixed! My bad, didn't realize this was already fixed in the fe-v2 implementation! I did base my branch off of fe-v2, and thought I reproduced the error. Is there a different way to invoke the fe-v2 implementation?

I think it'd be fine to merge your effort to v1

And no worries! It didn't take too long, I was mostly just excited to play around with the codebase :)

@Y-Nak
Copy link
Collaborator Author

Y-Nak commented Apr 18, 2024

Is there a different way to invoke the fe-v2 implementation?

yeah, it's complicated since v2 branch still maintains v1 codebase. I guess it'd be nice timing to remove them from the v2 branch(Initially, I thought it'd be cumbersome to resolve conflicts, but probably conflicts will no longer happen since we all are switching to v2). Is it ok to remove them after merging the ty-check branch? @sbillig @g-r-a-n-t.

To invoke the new driver, you need to specify it explicitly.
e.g., cargo run --bin fe-driver2 foo.fe
There are many differences between v1 and v2 in both syntactic and semantic levels, you can find some examples in hir-analysis/test-files/ directory.
NOTE: Please refer to the ty-check branch. It's the latest one(and will hopefully be ready to be merged into the v2 branch soon).

@sbillig
Copy link
Collaborator

sbillig commented Apr 19, 2024

Is it ok to remove them after merging the ty-check branch?
Yeah, definitely.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v2-resolve This issue will be resolved in fe-v2 implementation
Projects
None yet
Development

No branches or pull requests

3 participants