rfcs icon indicating copy to clipboard operation
rfcs copied to clipboard

Improve boilerplate for Trait impl

Open prasannavl opened this issue 5 years ago • 13 comments

Currently impl for a struct and traits look like this:

struct Services {};

impl Services {

}

impl Clone for Services {
    fn clone(&self) -> Services {
        Services {}
    }
}

Now if we add lifetimes, and/or generics, the syntax gets out of hand rather quickly.

struct Services<'a, T> { wrap_drive: &'a WarpDrive<T> };

impl<'a, T> Services<'a, T> {

}

impl<'a, T> Clone for Services<'a, T> {
    fn clone(&self) -> Services<'a, T> {
        Services {
          // ..
         }
    }
}

What I'm proposing is to be able to write this instead:

struct Services<'a, T> { wrap_drive: &'a WarpDrive<T> };

impl<'a, T> Services<'a, T> {
    fn beam_all_services() { }
    
    // ..snip..snip
    
    impl Clone for Self {
        fn clone(&self) -> Services<'a, T> {
            Services { 
               // ... 
            }
        }
    }
}

Advantages:

  • Reduce boiler plate for impl when it's an in-module struct.
  • Encourage in-module blocks to be cleanly encapsulated into the parent impl block, allowing for easier source comprehension and maintenance.
  • While the above is a trivial case, code bases like actix and Hyper which makes heavy use of generics can be made much more readable removing redundant type information from each impl blocks.
  • This is all entirely optional. So, no breaking changes.

Disadvantages:

  • impls are no longer just confined to the top-level, but also be one level nested.
  • Compiler complexity.

prasannavl avatar Apr 02 '19 08:04 prasannavl

I believe a large part of this is covered by https://github.com/rust-lang/rfcs/pull/2089.

Ixrec avatar Apr 02 '19 09:04 Ixrec

@lxrec, AFAIK #2089 deals only with implied bounds? This doesn't have much to do with them, and hence will likely not have the same issues as the unresolved questions on the RFC as well.

Since, everything here is limited in the block the scope, it's equivalent to the compiler simply inheriting the traits from the parent block. This is a simpler and tightly scoped approach that includes removing boilerplate with generic parameters and lifetimes as well.

#2089 will still require <T>s and <'a>s, unless I'm mistaken?

prasannavl avatar Apr 02 '19 09:04 prasannavl

A priori, I'd worry removing too many <..>s might harm explicitness and create confusion.

I think #2089 covers all significant use cases if implied bounds come from type aliases. I'm unsure if implied bounds coming from type aliases creates other problems though, but if not then the type alias solutions sounds far more powerful and less strange than this bespoke syntax.

I also think this strays into parameterized module land since

mod impl_services<'a,T> {
    impl Clone for Services<'a, T> { ... }
}

Again parameterized modules are way more powerful, so if we want them eventually then we should favor them over this syntax.

Also, any issues with parameterized modules might indicate issues here too, meaning that discussion must be revisited carefully. I suppose implied bounds coming from type aliases might fit with the 2019 priorities, but parameterized modules sound pretty far afield.

burdges avatar Apr 03 '19 09:04 burdges

A priori, I'd worry removing too many <..>s might harm explicitness and create confusion.

This proposal only removes them inside a type impl. And the type becomes the place to define, which I think is appropriate. I'd think that's concise and to the point.

I'm rather convinced to take a view that differs from yours here as any reasonably complex project is just filled with <>, ', and Ts. Most of them are really just noise to provide sufficient info to the compiler, not to human beings.

Again parameterized modules are way more powerful, so if we want them eventually then we should favor them over this syntax.

This, I personally am not a fan of - I think this does what you say - Harm explicitness and create confusion.

prasannavl avatar Apr 03 '19 10:04 prasannavl

the syntax gets out of hand rather quickly.

Citation needed. Or, I just don't see how specifying the type constraints for an impl counts as "syntax getting out of hand".

Furthermore, the proposed syntax unconditionally increases indentation by one level – and makes trait methods stand on a different indent level than inherent methods. This is at best ugly, and makes method definition syntax inconsistent.

The third problem with this feature is that it's severely non-orthogonal: I'd very much not like two very similar pieces of syntax for the exact same semantic purpose.

H2CO3 avatar May 02 '19 12:05 H2CO3

What about just making it implicit?

struct Services<'a, T: SomeTrait> { wrap_drive: &'a WarpDrive<T> };

impl Services<__> {

}

impl Clone for Services<__> {
    fn clone(&self) -> Services<__> {
        Services {
          // ..
         }
    }
}

It's because rust could determine the types, and I would definitely don't want to change every occurrence of the 'Services' whenever it receives a new type. When I want to be more specific, I could do it. E.g.:

impl Clone for Services<_, Something> {
    fn clone(&self) -> Services<_> {
        Services {
          // ..
         }
    }
}

There would be two new language feature:

<__> would mean infer all types automatically <_, _> would encourage us to keep the number of type arguments, but those could be auto inferred too.

Usually, we want to write testable code.

Currently, I can write easily with dynamic dispatch because those do not require template arguments.

pub struct Game {
    map: Box<dyn Map>,
    character: Box<dyn Character>,
    food: Box<dyn Food>,
    /// .... some other members
}

impl Game {
   // ...
}

Whereas in static dispatch case:

pub struct Game<
    TSomething: Something,
    TMap: Map<TSomething>,
    TCharacter: Character<TSomething>,
    TFood: Food<TSomething>,
> {
    map: TMap,
    character: TCharacter,
    food: TFood,
}


impl<
    TSomething: Something,
    TMap: Map<TSomething>,
    TCharacter: Character<TSomething>,
    TFood: Food<TSomething>,
>  Game<TSomething, TMap, TCharacter, TFood> {
   // ...
}

So I would make this instead:

pub struct Game<
    TSomething: Something,
    TMap: Map<TSomething>,
    TCharacter: Character<TSomething>,
    TFood: Food<TSomething>,
> {
    map: TMap,
    character: TCharacter,
    food: TFood,
}


impl Game<__> {
   // ...
}

fxdave avatar Nov 21 '20 22:11 fxdave

It looks like you are trying to put the trait bounds on the type definition and infer them from there in the impl block. That's not great – because, for several reasons (e.g. interoperability and only constraining types as much as necessary, and allowing for finer granurality of methods), the trait bounds should be added on impl blocks themselves, and not on the type definition.

Besides, if you need to repeat the trait bounds, you can just copy and paste. You aren't forced to power-type everything twice.

H2CO3 avatar Nov 21 '20 22:11 H2CO3

It looks like you are trying to put the trait bounds on the type definition and infer them from there

That's exactly what I want, but please let's focus on this proposal.

interoperability

Interoperability is not decreasing. Imagine it as a preprocessor that includes the types before the build. What is not specified remains in the template.

only constraining types as much as necessary

This would constrain all types very well defined. For this reasoning, I would ask you, why do you allow template arguments then? That allows you to only constraining types as much as necessary. Why not just strictly add the concrete types?

the trait bounds should be added on impl blocks themselves, and not on the type definition.

Why?

Besides, if you need to repeat the trait bounds, you can just copy and paste. You aren't forced to power-type everything twice.

Code duplication is generally a bad idea. Software constantly changes, and if it's hard to do, I would call them legacy.

fxdave avatar Nov 21 '20 23:11 fxdave

Why?

You can look it up on URLO. That's the entire point that I was making w.r.t. interoperability, and that you have missed, apparently.

Edit: Official API guidelines, the related issue, a Reddit thread discussing the same

H2CO3 avatar Nov 21 '20 23:11 H2CO3

It'd become easier to teach that if clippy warned against bounds on data structures that violate those rules (or perhaps some superset like non-associated type bound on a Copy type`).

burdges avatar Nov 22 '20 00:11 burdges

Edit: Official API guidelines, the related issue, a Reddit thread discussing the same

I have just read these and some others about the topic. I think you reject a feature for conventional reasons.

Let's just have a glimpse of other languages:

class UserController {
    repo: UserRepositoryInterface;
    construct(repo: UserRepositoryInterface) {
         this.repo = repo;
    }

    index(): Promise<User[]>  {
        return this.repo.getAll();
     }
}

// test:
class FakeUserRepository implements UserRepositoryInterface {
    async getAll(): Promise<User[]> {
         return [new User("Foo")];
    }
}

let c = new UserController(new FakeUserRepository);
assert.equal(c.index().length, 1);
assert.equal(c.index()[0].name, "Foo");

Simple and straightforward to use. So the point here is I don't want the actual types. I just need the functionality, which is possible by dynamic dispatch. And the same functionality can be implemented by static dispatch. So I think static dispatch should be as easy as the dynamic approach.

What about this one?:

struct Game {
    map: impl Map,
    character: impl Character,
    food: impl Food,
}

impl Game {
 // ...
}

In this case, everything is solved.

Inferring the types:

Game {
    map: RectMap::new(100, 100),
    character: Snake::new(),
    food: Apple::new(),
}

EDIT: When the only reason to make it abstract is to test it, I don't need template arguments.

fxdave avatar Nov 22 '20 02:11 fxdave

The "other language" also does it with dynamic dispatch. Then so can you. It is not the case that dynamic dispatch makes the types go away. In the case of dyn Trait, the dyn Trait is the type. It is a distinct type from the one it is created from, it is a distinct entity from the trait it is referring to, it can be used in type checking and won't unify with different concrete types, etc.

Your example with impl Trait-typed struct fields effectively proposes global type inference, which is highly problematic for another, different set of reasons, and Rust's design explicitly avoids it, and this fact has been confirmed several times by the community and the team when it came up in the past. So that part of the proposal doesn't fit with the direction of the language, either.

H2CO3 avatar Nov 22 '20 07:11 H2CO3

I'm running into this annoyance. The boilerplate starts getting out of hand when you have a custom type (which takes a type parameter) and start implementing multiple traits for it where you just want to reuse the same bounds for the type parameter. Perhaps something like associated type declarations on inherent impls could help this situation? Or a way to express bounds on an inherent impl associated type? If any of that is even possible or makes sense. Idk. In the meantime, I suppose it's possible to define a macro for the type parameter/bound boilerplate.

tmillr avatar Mar 06 '24 21:03 tmillr