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

Improve boilerplate for Trait impl #2676

Open
prasannavl opened this issue Apr 2, 2019 · 13 comments
Open

Improve boilerplate for Trait impl #2676

prasannavl opened this issue Apr 2, 2019 · 13 comments
Labels
A-impls Implementations related proposals & ideas A-syntax Syntax related proposals & ideas T-lang Relevant to the language team, which will review and decide on the RFC.

Comments

@prasannavl
Copy link

prasannavl commented Apr 2, 2019

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.
@Ixrec
Copy link
Contributor

Ixrec commented Apr 2, 2019

I believe a large part of this is covered by #2089.

@prasannavl
Copy link
Author

prasannavl commented Apr 2, 2019

@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?

@burdges
Copy link

burdges commented Apr 3, 2019

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.

@prasannavl
Copy link
Author

prasannavl commented Apr 3, 2019

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.

@H2CO3
Copy link

H2CO3 commented May 2, 2019

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.

@jonas-schievink jonas-schievink added A-impls Implementations related proposals & ideas A-syntax Syntax related proposals & ideas T-lang Relevant to the language team, which will review and decide on the RFC. labels Nov 19, 2019
@fxdave
Copy link

fxdave commented Nov 21, 2020

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<__> {
   // ...
}

@H2CO3
Copy link

H2CO3 commented Nov 21, 2020

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.

@fxdave
Copy link

fxdave commented Nov 21, 2020

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.

@H2CO3
Copy link

H2CO3 commented Nov 21, 2020

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

@burdges
Copy link

burdges commented Nov 22, 2020

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`).

@fxdave
Copy link

fxdave commented Nov 22, 2020

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.

@H2CO3
Copy link

H2CO3 commented Nov 22, 2020

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.

@tmillr
Copy link

tmillr commented Mar 6, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-impls Implementations related proposals & ideas A-syntax Syntax related proposals & ideas T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

No branches or pull requests

7 participants