ethereum / fe

Emerging smart contract language for the Ethereum blockchain.
https://fe-lang.org
Other
1.6k stars 180 forks source link

Compiler doesn't check duplicated trait implementation #870

Open Y-Nak opened 1 year ago

Y-Nak commented 1 year ago

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

BKDaugherty commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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