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

Infer function signatures from trait declaration into 'impl's #2063

Closed
wants to merge 39 commits into
base: master
from

Conversation

Projects
None yet
@dobkeratops

dobkeratops commented Jul 13, 2017

RFC: suggestion to allow ommitting type information in function signatures in 'trait impls', i.e. just take this information directly from the trait declaration (see behaviour in haskell for reference)

https://github.com/dobkeratops/rfcs/blob/infer-function-signatures-from-trait-declaration-into-'impl's/text/infer%20function%20signatures%20from%20trait%20declaration%20into%20impls.md

dobkeratops and others added some commits Jul 1, 2014

Rename 0000-ask_the_compiler_placeholder_ident.md to infer function p…
…arameter and return types from the original trait declaration into impl blocks
Rename infer function parameter and return types from the original tr…
…ait declaration into impl blocks to infer function signatures from trait declaration into impls
@Ixrec

This comment has been minimized.

Show comment
Hide comment
@Ixrec

Ixrec Jul 13, 2017

Contributor

I'm not sure what's going on with this PR since it appears to delete the RFC template and add a second rather detailed RFC unrelated to the PR's title, but I assume this was only meant to include the "infer function signatures from trait declaration into impls" file so I'm only responding to that.

Overall, I think I like the idea but I'm not sure how far I'd want to go with it.

Omitting the return type feels iffy to me since it's easy to mistake that for a function that returns nothing, whereas an omitted argument type is obviously not an argument of type ().

I'm also not sure if I'd want these types to be omitted when the impl and the trait are in separate files/modules, though that's probably a matter of style rather than something the compiler should enforce.

This does seem like rules out ever supporting overloaded functions. I don't know if overloading ever has a chance of happening, and I'm not sure that I want it to happen, but I think we should clearly decide that it's never going to happen before adopting something like this. That probably should get discussed in the Alternatives or Unresolved Questions sections.

I think the "implied bounds" idea tackles an even more unnecessary source of verbosity in exactly this sort of code, makes sense for a lot of the same reasons, and doesn't have some of the drawbacks I just mentioned, so my gut feeling is implied bounds ought to come before this. Though we don't appear to have an RFC for that yet.

Contributor

Ixrec commented Jul 13, 2017

I'm not sure what's going on with this PR since it appears to delete the RFC template and add a second rather detailed RFC unrelated to the PR's title, but I assume this was only meant to include the "infer function signatures from trait declaration into impls" file so I'm only responding to that.

Overall, I think I like the idea but I'm not sure how far I'd want to go with it.

Omitting the return type feels iffy to me since it's easy to mistake that for a function that returns nothing, whereas an omitted argument type is obviously not an argument of type ().

I'm also not sure if I'd want these types to be omitted when the impl and the trait are in separate files/modules, though that's probably a matter of style rather than something the compiler should enforce.

This does seem like rules out ever supporting overloaded functions. I don't know if overloading ever has a chance of happening, and I'm not sure that I want it to happen, but I think we should clearly decide that it's never going to happen before adopting something like this. That probably should get discussed in the Alternatives or Unresolved Questions sections.

I think the "implied bounds" idea tackles an even more unnecessary source of verbosity in exactly this sort of code, makes sense for a lot of the same reasons, and doesn't have some of the drawbacks I just mentioned, so my gut feeling is implied bounds ought to come before this. Though we don't appear to have an RFC for that yet.

@eddyb

This comment has been minimized.

Show comment
Hide comment
@eddyb

eddyb Jul 26, 2017

Member

The desugaring is also very straightforward and "syntactic"

If I may be pedantic, the desugaring has to be semantic because of generic parameters in the trait, but indeed it doesn't require inference, as long as associated types are explicitly given.

There's also the possibility of inferring associated types from the signatures of methods, but that would still not be inferring from the body of a method ("global inference").

Both approaches would ideally use the inference engine internally for maximal flexibility, but the scope of the inference would be very small. We use inference to check that an impl matches a required bound, i.e. unifying Vec<u8>: Clone with String: Clone fails but with Vec<_>: Clone it succeeds and the _ placeholder ends up known to be u8.

Either way, the implementation in the compiler should be straight-forward and self-contained.

Member

eddyb commented Jul 26, 2017

The desugaring is also very straightforward and "syntactic"

If I may be pedantic, the desugaring has to be semantic because of generic parameters in the trait, but indeed it doesn't require inference, as long as associated types are explicitly given.

There's also the possibility of inferring associated types from the signatures of methods, but that would still not be inferring from the body of a method ("global inference").

Both approaches would ideally use the inference engine internally for maximal flexibility, but the scope of the inference would be very small. We use inference to check that an impl matches a required bound, i.e. unifying Vec<u8>: Clone with String: Clone fails but with Vec<_>: Clone it succeeds and the _ placeholder ends up known to be u8.

Either way, the implementation in the compiler should be straight-forward and self-contained.

@dobkeratops

This comment has been minimized.

Show comment
Hide comment
@dobkeratops

dobkeratops Jul 29, 2017

Another thing to mention here:-
psychologically after several years of having tried rust on and off I'm still usually writing type argname then correcting it .
Obviously, that's a triviality an IDE could fix; but it's surprising that the 'split signature' of haskell is actually a more relaxing way around this (moving the elsewhere rather than left or right).

Theoretically I definitely prefer rusts idea of putting the type afterward (and the 'type syntax' is so much cleaner), but the 20-year habit of C/C++ is buried too deep.

The point I'm making is this idea would smooth things out for me by more than you might guess from the simple 'screen space saving'/'character saving' alone

dobkeratops commented Jul 29, 2017

Another thing to mention here:-
psychologically after several years of having tried rust on and off I'm still usually writing type argname then correcting it .
Obviously, that's a triviality an IDE could fix; but it's surprising that the 'split signature' of haskell is actually a more relaxing way around this (moving the elsewhere rather than left or right).

Theoretically I definitely prefer rusts idea of putting the type afterward (and the 'type syntax' is so much cleaner), but the 20-year habit of C/C++ is buried too deep.

The point I'm making is this idea would smooth things out for me by more than you might guess from the simple 'screen space saving'/'character saving' alone

@est31

This comment has been minimized.

Show comment
Hide comment
@est31

est31 Aug 21, 2017

Contributor

@dobkeratops could you consider signing off to the MIT/Apache 2.0 licensing terms inside this issue: #2096 ? Thanks!

Contributor

est31 commented Aug 21, 2017

@dobkeratops could you consider signing off to the MIT/Apache 2.0 licensing terms inside this issue: #2096 ? Thanks!

@anowell

This comment has been minimized.

Show comment
Hide comment
@anowell

anowell Aug 30, 2017

As an example usage, I recently had a trait that defined 18 functions (each generating templates for a generic provider).

Here's a small snippet from one of the implementations:

fn frontend_app_conf<'a>(
    plan: &'a Plan<Self::Plan>,
    tfstate: &'a TfState<Self::Tf>,
    secrets: &'a ConfigSecrets
) -> Box<Template + 'a> { 
    Box::new(frontend::aws_app_conf(plan, tfstate, secrets))
}

fn apiserver_deployment_yaml(
    plan: &'a Plan<Self::Plan>,
    confs: &'a [&'a ::stages::AppConf]
) -> Box<Template + 'a> {
    Box::new(apiserver::aws_deployment_yaml(plan, confs))
}

The fact that all those types were fully defined in the trait definition made it feel particularly verbose to rewrite them 18 times per provider, so I finally caved and wrote a macro:

macro_rules! magic_types {
    // These are the valid keywords
    (plan) => { &'a Plan<Self::Plan> };
    (tfstate) => { &'a TfState<Self::Tf> };
    (secrets) => { &'a ConfigSecrets };
    (confs) => { &'a [&'a ::stages::AppConf] };

    // This is the meat of the macro
    ($(fn $src:ident($($arg:ident),*) $body:block)*) => {
        $(
            #[allow(unused_variables)]
            fn $src<'a>( $($arg: magic_types!($arg)),* ) -> Box<Template + 'a> $body
        )*
    };
}

just so I could write this instead:

magic_types! {
    fn frontend_app_conf(plan, tfstate, secrets) {
        Box::new(frontend::aws_app_conf(plan, tfstate, secrets))
    }
    fn apiserver_deployment_yaml(plan, confs) {
        Box::new(apiserver::aws_deployment_yaml(plan, confs))
    }
    // 16 other similar functions
}

In general, I love that all functions/methods are explicitly typed, exactly for the readability argument discussed. This was a case where I found the types and lifetimes started getting in my way of reading the implementations. Maybe this is rare enough that macros like mine, or maybe a slighly more generalized solution are sufficient; I just thought I'd weigh in with an example where the intent of this RFC appealed to me.

anowell commented Aug 30, 2017

As an example usage, I recently had a trait that defined 18 functions (each generating templates for a generic provider).

Here's a small snippet from one of the implementations:

fn frontend_app_conf<'a>(
    plan: &'a Plan<Self::Plan>,
    tfstate: &'a TfState<Self::Tf>,
    secrets: &'a ConfigSecrets
) -> Box<Template + 'a> { 
    Box::new(frontend::aws_app_conf(plan, tfstate, secrets))
}

fn apiserver_deployment_yaml(
    plan: &'a Plan<Self::Plan>,
    confs: &'a [&'a ::stages::AppConf]
) -> Box<Template + 'a> {
    Box::new(apiserver::aws_deployment_yaml(plan, confs))
}

The fact that all those types were fully defined in the trait definition made it feel particularly verbose to rewrite them 18 times per provider, so I finally caved and wrote a macro:

macro_rules! magic_types {
    // These are the valid keywords
    (plan) => { &'a Plan<Self::Plan> };
    (tfstate) => { &'a TfState<Self::Tf> };
    (secrets) => { &'a ConfigSecrets };
    (confs) => { &'a [&'a ::stages::AppConf] };

    // This is the meat of the macro
    ($(fn $src:ident($($arg:ident),*) $body:block)*) => {
        $(
            #[allow(unused_variables)]
            fn $src<'a>( $($arg: magic_types!($arg)),* ) -> Box<Template + 'a> $body
        )*
    };
}

just so I could write this instead:

magic_types! {
    fn frontend_app_conf(plan, tfstate, secrets) {
        Box::new(frontend::aws_app_conf(plan, tfstate, secrets))
    }
    fn apiserver_deployment_yaml(plan, confs) {
        Box::new(apiserver::aws_deployment_yaml(plan, confs))
    }
    // 16 other similar functions
}

In general, I love that all functions/methods are explicitly typed, exactly for the readability argument discussed. This was a case where I found the types and lifetimes started getting in my way of reading the implementations. Maybe this is rare enough that macros like mine, or maybe a slighly more generalized solution are sufficient; I just thought I'd weigh in with an example where the intent of this RFC appealed to me.

@shanavas786

This comment has been minimized.

Show comment
Hide comment
@shanavas786

shanavas786 Aug 30, 2017

I feel this RFC will reduce readability as one need to peek into trait definition to get type info.
It could be better implemented as an RSL feature to autofill types in trait implementation functions.

shanavas786 commented Aug 30, 2017

I feel this RFC will reduce readability as one need to peek into trait definition to get type info.
It could be better implemented as an RSL feature to autofill types in trait implementation functions.

@dobkeratops

This comment has been minimized.

Show comment
Hide comment
@dobkeratops

dobkeratops Sep 3, 2017

I feel this RFC will reduce readability as one need to peek into trait definition to get type info.

but the user would look at the trait definition first and foremost, similarly a trait implementor would always have to refer to the trait. Similarly people might be reading docs where these things can be filled out/hyperlinked.

I would equally argue the 'rust-language-server' would assist with navigating back to the trait declaration, but the declaration is already very easy to find in rust with grep

dobkeratops commented Sep 3, 2017

I feel this RFC will reduce readability as one need to peek into trait definition to get type info.

but the user would look at the trait definition first and foremost, similarly a trait implementor would always have to refer to the trait. Similarly people might be reading docs where these things can be filled out/hyperlinked.

I would equally argue the 'rust-language-server' would assist with navigating back to the trait declaration, but the declaration is already very easy to find in rust with grep

@repax

This comment has been minimized.

Show comment
Hide comment
@repax

repax Sep 3, 2017

It could be better implemented as an RSL feature to autofill types in trait implementation functions.

The RLS can also provide type info when you're hovering an argument/variable.

I'm not yet sure how I feel about this RFC. Perhaps some hands-on experience would be a good way to find out. How about conducting an experiment?

repax commented Sep 3, 2017

It could be better implemented as an RSL feature to autofill types in trait implementation functions.

The RLS can also provide type info when you're hovering an argument/variable.

I'm not yet sure how I feel about this RFC. Perhaps some hands-on experience would be a good way to find out. How about conducting an experiment?

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Sep 6, 2017

Member

@glaebhoerl

please be aware that there is copious amounts of actual experience with this idea, which has been basically positive

(I will note, however, that Haskell in general permits much more type elision than Rust, so it's hard to know how this would carry over.)

@theduke

I personally find repeating the types in impls where they are completely redundant to be one of the very annoying parts of Rust ergonomics.

@petrochenkov

I bet people would love to write impls using the terse form, especially during prototyping or experimenting.

I think the general idea is good and worth implementing and experimenting with.

@repax

I'm not yet sure how I feel about this RFC. Perhaps some hands-on experience would be a good way to find out. How about conducting an experiment?

So, the RFC as written needs a lot of work, and there's very limited time to reach any full consensus prior to the impl period, but I wonder whether we should consider accepting provisinally as an experimental RFC, i.e. that we'd allow a feature like this to land in nightly to see how it feels, but would require a full RFC before any stabilization.

cc @rust-lang/lang, do we think there's enough potential here to open the door to experimentation? I admit that when I first saw this RFC, my kneejerk reaction was to say "no way", but @petrochenkov's point about prototyping in particular caught my attention.

Member

aturon commented Sep 6, 2017

@glaebhoerl

please be aware that there is copious amounts of actual experience with this idea, which has been basically positive

(I will note, however, that Haskell in general permits much more type elision than Rust, so it's hard to know how this would carry over.)

@theduke

I personally find repeating the types in impls where they are completely redundant to be one of the very annoying parts of Rust ergonomics.

@petrochenkov

I bet people would love to write impls using the terse form, especially during prototyping or experimenting.

I think the general idea is good and worth implementing and experimenting with.

@repax

I'm not yet sure how I feel about this RFC. Perhaps some hands-on experience would be a good way to find out. How about conducting an experiment?

So, the RFC as written needs a lot of work, and there's very limited time to reach any full consensus prior to the impl period, but I wonder whether we should consider accepting provisinally as an experimental RFC, i.e. that we'd allow a feature like this to land in nightly to see how it feels, but would require a full RFC before any stabilization.

cc @rust-lang/lang, do we think there's enough potential here to open the door to experimentation? I admit that when I first saw this RFC, my kneejerk reaction was to say "no way", but @petrochenkov's point about prototyping in particular caught my attention.

@aturon aturon added the I-nominated label Sep 6, 2017

@dobkeratops

This comment has been minimized.

Show comment
Hide comment
@dobkeratops

dobkeratops Sep 6, 2017

@aturon thanks for the feedback.. am I reading this right, that there's a chance this could be tried out in 'nightly' (disabled by default), just not available in the stable compiler? If so that would be awesome.

dobkeratops commented Sep 6, 2017

@aturon thanks for the feedback.. am I reading this right, that there's a chance this could be tried out in 'nightly' (disabled by default), just not available in the stable compiler? If so that would be awesome.

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Sep 6, 2017

Member

@dobkeratops Yep, that's exactly what I'm proposing. This would let us get a feel for the idea before writing up and approving a complete RFC.

Member

aturon commented Sep 6, 2017

@dobkeratops Yep, that's exactly what I'm proposing. This would let us get a feel for the idea before writing up and approving a complete RFC.

@nikomatsakis

This comment has been minimized.

Show comment
Hide comment
@nikomatsakis

nikomatsakis Sep 6, 2017

Contributor

I personally would not be opposed to an Experimental RFC here. I've not read the details of this RFC in particular, but I have certainly felt the desire to write "tighter" impls from time to time, that don't repeat information from the trait.

Contributor

nikomatsakis commented Sep 6, 2017

I personally would not be opposed to an Experimental RFC here. I've not read the details of this RFC in particular, but I have certainly felt the desire to write "tighter" impls from time to time, that don't repeat information from the trait.

@aturon aturon removed the I-nominated label Sep 7, 2017

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Sep 7, 2017

Member

The lang team discussed this RFC in today's meeting, and it quickly became clear that there is not even consensus that there is a problem here to be solved, nor on the best approach to tackle it.

For example, it was pointed out that IDEs should be able to auto-generate impls, and if you're looking for maximum productivity and help in prototyping, you should probably be using an IDE anyway.

Others felt that it was worth addressing this issue within Rust itself, but there was a lot of uncertainty over what should be required (e.g. for return types and where clauses), as well as what the best practices should be.

In general, while the team remains open to experimentation here, the general feeling was that there's going to be so much else going on in the compiler during the impl period, and we're uncertain enough of the motivation here (as well as the RFC text), that it's best not to take it as an experiment right now. We'd welcome a more formal eRFC after the impl period, though.

Thanks, @dobkeratops, for initiating this discussion!

@rfcbot fcp postpone

Member

aturon commented Sep 7, 2017

The lang team discussed this RFC in today's meeting, and it quickly became clear that there is not even consensus that there is a problem here to be solved, nor on the best approach to tackle it.

For example, it was pointed out that IDEs should be able to auto-generate impls, and if you're looking for maximum productivity and help in prototyping, you should probably be using an IDE anyway.

Others felt that it was worth addressing this issue within Rust itself, but there was a lot of uncertainty over what should be required (e.g. for return types and where clauses), as well as what the best practices should be.

In general, while the team remains open to experimentation here, the general feeling was that there's going to be so much else going on in the compiler during the impl period, and we're uncertain enough of the motivation here (as well as the RFC text), that it's best not to take it as an experiment right now. We'd welcome a more formal eRFC after the impl period, though.

Thanks, @dobkeratops, for initiating this discussion!

@rfcbot fcp postpone

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Sep 7, 2017

Team member @aturon has proposed to postpone this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot commented Sep 7, 2017

Team member @aturon has proposed to postpone this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@Centril

This comment has been minimized.

Show comment
Hide comment
@Centril

Centril Sep 11, 2017

Contributor

I think the case for this RFC is very similar to the "Implied bounds" RFC in the way it deals with redundant information.

I don't think the argument @aturon mentions regarding IDE's holds up... one could make the case that the IDE should auto-generate the implied bounds (sure, it might be a bit more tricky for the IDE, but still doable).

For the sake of consistency, I voice my support to merge the "idea" behind this RFC.

Contributor

Centril commented Sep 11, 2017

I think the case for this RFC is very similar to the "Implied bounds" RFC in the way it deals with redundant information.

I don't think the argument @aturon mentions regarding IDE's holds up... one could make the case that the IDE should auto-generate the implied bounds (sure, it might be a bit more tricky for the IDE, but still doable).

For the sake of consistency, I voice my support to merge the "idea" behind this RFC.

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Sep 18, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot commented Sep 18, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

1 similar comment
@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Sep 18, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

rfcbot commented Sep 18, 2017

🔔 This is now entering its final comment period, as per the review above. 🔔

@dobkeratops

This comment has been minimized.

Show comment
Hide comment
@dobkeratops

dobkeratops Sep 19, 2017

yes regarding IDE assistance, rather than have the IDE copy/paste, other features like hover showing full types might be more useful.. (e.g you'll want that to show types of locals aswell)

Many IDEs have a a top-bar that shows the prototype of the current function (useful when scrolling into the function body) - perhaps it could show the full prototype there

Given traits sometimes have defaults impls - you might not always be implementing every function - the ability to show the 'current trait' in some side panel or popup might be useful (think how class-based IDEs have 'class-explorers', you could have a 'trait-view', which can show everything, with superior formatting. (see other ideas, intellij-rust/intellij-rust#1545)

dobkeratops commented Sep 19, 2017

yes regarding IDE assistance, rather than have the IDE copy/paste, other features like hover showing full types might be more useful.. (e.g you'll want that to show types of locals aswell)

Many IDEs have a a top-bar that shows the prototype of the current function (useful when scrolling into the function body) - perhaps it could show the full prototype there

Given traits sometimes have defaults impls - you might not always be implementing every function - the ability to show the 'current trait' in some side panel or popup might be useful (think how class-based IDEs have 'class-explorers', you could have a 'trait-view', which can show everything, with superior formatting. (see other ideas, intellij-rust/intellij-rust#1545)

@rfcbot

This comment has been minimized.

Show comment
Hide comment
@rfcbot

rfcbot Sep 28, 2017

The final comment period is now complete.

rfcbot commented Sep 28, 2017

The final comment period is now complete.

@aturon

This comment has been minimized.

Show comment
Hide comment
@aturon

aturon Sep 28, 2017

Member

Closing as postponed. Thanks @dobkeratops -- this is something we should consider revisiting on an experimental basis in the future.

Member

aturon commented Sep 28, 2017

Closing as postponed. Thanks @dobkeratops -- this is something we should consider revisiting on an experimental basis in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment