Implement traits, impls and generic functions with trait bounds

[cburgdorf]

Abstract

We want to have traits as well as generic functions with trait bounds. One of the main motivations is to properly implement emit on the Context struct as pub fn emit<T: Event>(val: T) { ... }

How was it fixed?

Working example below:

https://github.com/ethereum/fe/blob/f97afea7937202e32b40de5f866b960b60d84c0a/crates/test-files/fixtures/features/generic_functions.fe#L1-L42

Some notes:

• Generic functions a currently monomorphized at MIR lowering
• Because trait functions are usually (and currently only) defined without body the signature part was ripped out of FunctionId and into FunctionSigId to allow traits to reuse a bunch of code that doesn't care about the body anyway.
• There are certain limitations that can potentially be improved in the future e.g.
  • support traits for non-struct types
  • allow trait methods to define a body as a default implementation
  • support generic functions in traits (e.g. trait PartialEq { pub fn eq<T>(self, other: T); })
  • support multiple bounds
  • support unbounded generics
  • support trait associated functions (e.g. SomeTrait::some_fun()

[cburgdorf]

I just changed this back to draft because I will update it soon™ with all the remaining parts to cover impl blocks. Just need a few more cleanups and bits here and there.

[Y-Nak]

I left some comments.

Also, the below code causes ICE.

trait Computable {
  fn compute(self) -> u256;
}

struct Mac {
  pub fn compute(self) -> u256 {
     return 0
  }
}

impl Computable for Mac {
  fn compute(self) -> u256 {
    return 1
  }
}

contract Bar {
  pub fn foo() -> u256 {
    let mac: Mac = Mac()
    return mac.compute()
  }
  
  pub fn foo2() -> u256 {
    let mac: Mac = Mac()
    return foo_computable(val: mac)
  }
}

fn foo_computable<T: Computable>(val: T) -> u256 {
  return val.compute()
}

thread 'main' panicked at 'should be lowered in `lower_types_in_functions`', crates/mir/src/lower/types.rs:25:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

I haven't looked into this yet, but it'd be better to address this in this PR.

[Y-Nak]

I'd like to discuss whether traits should be regarded as a type. For our use case, it feels be better to treat trait as an Item.

[cburgdorf]

Ah, yes. That's a good point. I initially made it a type because I started by ignoring generics to just support something like:

pub fn bar(val: SomeTrait) {
}

But then later I noticed that it would be better to add generics first and only support

pub fn bar<T: SomeTrait(val: T) {
}

I don't know if we ever want to support the first example (or maybe val: impl SomeTrait as rust does) or just leave it to generics entirely. My current thinking is that we should not support the first example and leave it to generics to work with traits and hence could remove Trait as a type and add it as its own category of Item.

[cburgdorf]

Also, the below code causes ICE.

Thanks, this code should have been rejected because generics should not be supported yet outside of struct functions. Fixed it on my local machine.

[Y-Nak]

Ok, I misunderstood the cause of ICE. I wrote the test case to check what would happen when method name conflicts happen, and it caused ICE, so I wrongly suspected that function name conflict was the cause of the ICE.

After modifying the test case, it causes ICE because of name conflicts this time.

trait Computable1 {
    fn compute(self) -> u256;
}

trait Computable2 {
    fn compute(self) -> u256;
}


struct Mac {
}

impl Computable1 for Mac {
    fn compute(self) -> u256 {
        return 1
    }
}

impl Computable2 for Mac {
    fn compute(self) -> u256 {
        return 2
    }
}

contract Bar {
    pub fn foo() -> u256 {
        let mac: Mac = Mac()
        let mys: MyStruct = MyStruct()
        return mys.foo_computable1(val: mac)
    }
    
    pub fn foo2() -> u256 {
        let mac: Mac = Mac()
        let mys: MyStruct = MyStruct()
        return mys.foo_computable2(val: mac)
    }
}

struct MyStruct {
    pub fn foo_computable1<T: Computable1>(self, val: T) -> u256 {
        return val.compute()
    }

    pub fn foo_computable2<T: Computable2>(self, val: T) -> u256 {
        return val.compute()
    }
}

↓

Error: DeclarationError: Function name foo$Mac$compute already taken in this scope.
  --> input.yul:51:13:
   |
51 |             function foo$Mac$compute() -> $$ret {
   |             ^ (Relevant source part starts here and spans across multiple lines).


thread 'main' panicked at 'Yul compilation failed with the above errors', crates/driver/src/lib.rs:192:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

You've hit an internal compiler error. This is a bug in the Fe compiler.
Fe is still under heavy development, and isn't yet ready for production use.

If you would, please report this bug at the following URL:
  https://github.com/ethereum/fe/issues/new

So we need to mangle the function name to fix the bug, I think it would be better to implement the mangling in codegen, not in mir.

[cburgdorf]

Oh, I see. I believe this is because here we are considering the name of the class which for the impl is the class of the receiver. If instead we would have the impl here instead then we could consider a combination of trait name and receiver name which should make it unique.

https://github.com/ethereum/fe/blob/9ff971ebd1a49ff927c50c6cbc829ba3b3862ca8/crates/mir/src/db/queries/function.rs#L84-L86

think it would be better to implement the mangling in codegen, not in mir

Do you suggest to fix it here?

https://github.com/ethereum/fe/blob/9ff971ebd1a49ff927c50c6cbc829ba3b3862ca8/crates/codegen/src/db/queries/function.rs#L19-L24

Wouldn't that mean that we let name_with_class return an faulty name? Why wouldn't we want to fix it earlier?

Follow up question, we could fix that by making Impl a variant of Class but you suggested to get rid of Class altogether. If we do that should Impl also become an Item and should we consider trait name + receiver name as the name of the impl?

[Y-Nak]

Oh, I see. I believe this is because here we are considering the name of the class which for the impl is the class of the receiver.

Partially yes, this is one of the concrete examples. But I didn't come up with a concrete example when I reviewed it, I just felt that it may reduce too much of the amount of information that should be kept.

Wouldn't that mean that we let name_with_class return an faulty name? Why wouldn't we want to fix it earlier?

Yes, it would work perfectly in this case. I just thought that name mangling should be performed in the codegen phase because the mangled name is never used from another compilation pass. But yes, fixing the class perfectly works in this case.

If we do that should Impl also become an Item and should we consider trait name + receiver name as the name of the impl?

I think the "name" of the function would differ depending on the context. What context is in your head?

Currently, the "name" of the function is used in two contexts,

1. Printing MIR for debugging,
2. symbol_name in codegen
   For 1., the name would be <MyStruct as MyTrait>::method_name like Rust's mir.
   For 2., the name would be MyTrait::MyStruct::method_name or even method_name_{impl_id} would be sufficient.

Now, I'm feeling that defining separate two functions for the two purposes and removing name_with_class would be the best option, instead of tweaking name_with_class.

[cburgdorf]

Thanks again for the feedback @Y-Nak
I renamed name_with_class to debug_name and made it format the impl function as <MyStruct as MyTrait>::method_name per your suggestion. I also tweaked symbol_name directly so that MyTrait$MyStruct$method_name is used there.

I didn't feel like adding the removal of Class to this PR so instead I added Class::Impl for now as an interim solution.

I think the only thing left from your suggestions is the removal of Trait from TypeDef which I started with but then ran into issues. Since I'm not yet entirely sure if we eventually want to support using traits in places for types maybe we can leave that as is for now and revisit later as we extend and tune the trait support?

[Y-Nak]

Great work @cburgdorf!

we can leave that as is for now and revisit later as we extend and tune the trait support?

👍