-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
RFC: Inherent traits #2309
RFC: Inherent traits #2309
Conversation
# Unresolved questions | ||
[unresolved]: #unresolved-questions | ||
|
||
- Syntax bike-shedding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps #[delegate(Bar)]
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me "delegation" has always been about making a trait impl for some type reuse the code from some other type's impl of the same trait, and the presence of delegation would be completely unobservable to client code. impl to another type. So "delegating to a trait", in a way that changes whether or not clients need an import, feels to me like a category error at best.
Though I'm certainly not a fan of #[include(Trait)]
. That looks so similar to a C/C++ #include
that I had to pause and think to figure out what the RFC was actually proposing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I assume the bar() method signature in the Reference-level Explanation is meant to appear in rustdoc. And presumably, you could replace the usage of the "inherent impl" feature with writing out those methods by hand, without clients being able to observe the change in any way.
If I'm interpreting that right, perhaps the "correct"/least misleading/most guessable-by-newbies syntax would be a macro rather than an attribute?
impl Foo {
inherent_impl!(Bar)
fn foo(&self) { println!("foo::foo"); }
}
Incidentally, even if everyone else prefers an attribute, I'd still prefer that the attribute be placed inside the impl
block rather than on top of it, simply because we're not modifying the impl block as a whole, we're adding to it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me "delegation" has always been about making a trait impl for some type reuse the code from some other type's impl of the same trait
To me too =) But I thought it felt sufficiently similar in spirit that it might work?
I'm not particularly fond of #[include(Bar)]
(but could certainly live with it..).
If I'm interpreting that right, perhaps the "correct"/least misleading/most guessable-by-newbies syntax would be a macro rather than an attribute?
That would have to be a very magic and special macro since they don't have access to the trait definition otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not impl Foo { #![include_trait(Trait)] }
so inside the inherent impl
block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@burdges I think those two (#[
outside and #![
inside) are interchangeable, so that's the same as this proposal (modulo the naming)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Afaik, all inside attributes use #![..]
while all outside attributes use #[..]
but not all inside attributes can be used outside and visa versa, so I'm restated @Ixrec last proposal with a fresh coat of paint.
What if you only want to make a certain subset of methods inherent as in |
I'm not really opposed to that, but I don't know how much extra value there is in it. |
What comes to mind is that you might want to reuse the names for some methods in the trait for other purposes in the inherent impl that are not exactly like the trait impl. I assume that if |
Where does the annotation go? Over the definition of the type? Over the impl of the trait? Over the inherent impl? You propose over the inherent impl, but what if there are multiple inherent impls to which the trait impl applies? Typically if I can call Whether this feature creates new symbols or not is not merely an implementation concern but a language specification concern which this RFC should discuss in more depth. The motivation of the RFC is legitimate, perhaps trying to optimize the usability of trait imports would be more consistent than trying to completely get around the need of importing the trait name. |
If you want to do individual functions too then consider Also, I'm unfamiliar with the rules for inherent |
@leodasvacas
This is covered in the RFC - it goes on any inherent impl. If you were to implement the same inherent method multiple times, either by including the same trait more than once, or by including traits with overlapping method names, then that would be an error.
As you can see, there is no conflict here, the inherent method is preferred:
Please explain how this could be a concern, and I will endeavour to address that in the RFC. |
Thank you for the correction!
Given the definitions: pub struct Foo;
trait Bar { fn bar(self); }
impl Bar for Foo { fn bar(self) {} }
#[include(Bar)]
impl Foo {} Is the expression |
First, 👍 to being able to define methods on traits without ergonomic loss. That would definitely help me remove not-for-syntax-extension macros. I'm surprised to see this syntax attached to an impl block, though. Why not to the type definition, like |
@scottmcm The generic parameters of a type definition may be applied with concrete arguments in at an impl site. The trait may not be impled for the generic impl, but impled for more specific impls, so I think this is strictly more general as proposed in the RFC. |
# Rationale and alternatives | ||
[alternatives]: #alternatives | ||
|
||
- Do nothing: users may choose to workaround the issue by manually performing the expansion if required. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alternative: Allow inherent impls as long as all functions are crate-private. Inherent methods are preferred over trait methods, but warned about. So if another crate adds a trait method, it does not conflict or change behaviour here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inherent methods are preferred over trait methods, but warned about.
I don't think they are warned about (see my playground link above)?
I think making the functions crate-private would undermine the primary use-cases of this RFC. The goal is to expose trait methods as inherent methods on a type, as part of the public API of a crate.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well... one could allow public inherent methods and require importing them via use crate_name::impl::Type;
or something similar
👍 I've thought about this idea exactly as many times as i forgot to |
Right now, if you I kinda dislike overloading the meaning of An attribute inside an inherent An attribute outside the trait
|
cc @sfackler |
```rust | ||
impl Foo { | ||
#[inline] | ||
pub fn bar(&self) { <Self as Bar>::bar(self); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm unhappy with merging items from different trait impls together with each other and inherent impl items (syntactically) for reasons described in #2303 (comment).
impl Bar for Foo { ... }
should still be a separate item, IMO, just marked as "inherent-like" somehow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@petrochenkov I've read your comment a number of times and I can't really make any sense of it - this RFC does not propose allowing implementing traits via inherent methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. I see.
To be honest I read it a couple days before writing this comment and it looks like I misremembered everything.
There was a few previous iterations on this idea, some of them made methods from specially marked trait impls directly available to name resolution without having the trait in scope and without delegation, like this RFC proposes.
(Trait objects currently behave very similarly, see e.g. rust-lang/rust#41221)
I like inherent trait implementation more compared to the "including" trait methods into type Often types do not have their own methods and express their behavior purely through traits. For example if we'll take RustCrypto/hashes I would like to write: struct Sha256 { .. }
// `#[inherent]` could be a keyword instead
// it only can be used if type was defined in the current crate
#[inherent]
impl FixedOutput for Sha256 { .. } Which will allow users to use |
I agree with @newpavlov that it seems better to associate the annotation with the trait impl rather than the inherent impl block. There are a couple of reasons for this. First, the blocks may not have the same "headers". What would this RFC do for a case like the following: impl SomeTrait for MyType<u32> { ... }
impl SomeTrait for MyType<i32> { ... }
#[inclue(SomeTrait)]
impl<T> MyType<T> { ... } There are lots of other bad combinations you can make. For example, you could write Also, if you don't otherwise have inherent items, you need to write an empty block to use this feature. (Not a major point, just worth noting). By contrast, writing If the RFC was changed in this manner, I'd be in favor of moving to FCP. |
One could presumably also desugar something like this: #[inherent_trait(Foo)]
struct Bar<T, U> { ... } to produce impl<T, U> Bar<T, U> {
fn foo(...) where Self : Foo { <Self as Foo>::foo(...) }
} so that the method is always there, it's just only callable for combinations with the trait actually impl'd. (Sortof like how, iiuc, That said, I like the above |
``` | ||
|
||
The method `bar` is now callable on any instance of `Foo`, regardless of whether the `Bar` trait is currently in scope, or even whether | ||
the `Bar` trait is publically visible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Contemplating this for myself, what are the other options?
Some bad ones:
- Inherit
pub
from the trait: gets into all the "what's pub, really" discussions - Put something on the methods: confusing to have
pub
there suddenly, doesn't work for trait methods with default impls - Put something on the trait: no, traits shouldn't ever care about this
A possibility: #[inherent(A)]
puts pub(A)
on them. So just #[inherent]
is pub
, but you can use #[inherent(crate)]
(or maybe #[inherent(in foo::bar)]
?) to limit it if helpful.
But really, in the same crate the trait import isn't a big deal, so that's probably not worth bothering with, and just "it's always pub
" (as the RFC says) is probably the way to go 👍
Firstly, I don't think your example is problematic: the compiler will see that the trait is not implemented for all Secondly, that would unnecessarily limit the feature. Here are some examples that work with this version of the RFC, but wouldn't under your suggestion: #[derive(X)]
struct Foo;
#[include(X)]
impl Foo {} // Crate 1
impl<T> X for T {}
// Crate 2
struct Foo;
#[include(X)]
impl Foo {} struct Foo<T>;
impl<T> X for Foo<T> {}
#[include(X)]
impl Foo<u32> {} The reason it makes sense to put on the inherent impl block is because of how it behaves wrt the coherence rules: the attribute is allowed precisely on those types for which you can provide an inherent impl. |
@Diggsey I don't find those additional uses particularly compelling, to be honest. My worry is that |
Neither version can be done with a Could you give an example of one of those corner cases? There exists a very straightforward implementation: for each member of the trait, define an inherent method with the same name which delegates to the trait. The checking I described previously doesn't even need to be explicit: by trying to delegate to the corresponding trait method, the compiler will already catch cases where it's not possible. |
After some thought @Diggsey example indeed could be quite useful for extension traits. In my case it's a BTW what about integration with rustdoc? Will it be able to distinguish inherent implementations or will it show duplicated methods? |
Nominating for Lang Team discussion. |
cc @rust-lang/lang |
I'd love to move quickly on this; I think it could be really useful for the epoch release, in the sense that I think we'd want to use this in We discussed this briefly in the Lang Team meeting; other team members are going to try to give their thoughts soon. |
A couple questions:
|
- Increased complexity of the language. | ||
|
||
# Rationale and alternatives | ||
[alternatives]: #alternatives |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about inverting the declaration location to something like
#[inherent]
impl Foo for Bar {
...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We discussed this in the lang team meeting-- IMO placing the attribute on the impl "feels" better, but it doesn't seem like it would work with derived traits and generic imps (e.g. impl<T> ToString for T where T: Display { ... }
-- how do I get an inherent to_string
for u32
?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah good point.
|
||
The `include` attribute is not valid on `impl <trait> for <type>` style `impl` blocks. | ||
|
||
# Reference-level explanation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems a bit underspecified for traits with type parameters.
Some thoughts: First off, I really want something like this feature. I however am not keen on the The first one is that we have generally avoided paths in attributes, because of the need to figure out the hygiene story there. (Maybe that concern is somewhat obsolete? I don't think so though.) But it also (to me) just doesn't quite feel like something that an attribute should be used for -- it just feels kind of non-obvious to me, and it doesn't feel "first class" enough for something that is going to be regularly used to define the type's API. The That said, I do find the idea that one might want to have a blanket impl of a trait but still make the trait "inherent" to a struct somewhat compelling. I was wondering about other syntactic possibilities. In the past, I've considered something like I was just now wondering about some kind of item in the inherent impl, like: impl Foo {
use trait Bar; // ?
} but that doesn't quite feel right. For one thing, Have to ponder it. One corner cases to consider (is this specified?)
|
It's worth pointing out that adding a defaulted method to a trait always has the ability to make code stop compiling. I know that @aturon and I have in the past considered that we might want to just stop doing that, at least in libstd, and instead off extension traits that must be explicitly imported. |
I wonder if there's some potential interaction with delegation here. Perhaps something like: struct Bar;
impl Bar {
fn bar(&self) { ... }
}
struct Foo(Bar);
impl MyTrait for Foo {
fn trait_method(&self) { ... }
}
impl Foo {
delegate bar to self.0;
delegate trait_method to trait MyTrait;
} |
I see the use case this is solving, and I've often found this annoying myself. The trait system is already complex enough as is, more so with specialization on the horizon. What would be interesting is: could this be made automatic, without the need for a manual attribute? |
I would say it's another way around, implementing inherent traits will allow users to use most of the types functionality without knowing much about trait hierarchies surrounding those types, by just copy-pasting examples. So we'll move some difficulty from initial stages of learning Rust at the cost of a slightly higher final point of the learning curve.
This approach will be a disaster in my opinion. Not only I think it's not automatically decidable (don't forget about possible generic trait implementations in third party crates), but adds a lot of implicit magic, instead of perfectly explicit attribution of types/impls. Requirement for explicit |
I also thought about something like this in the last time. |
I have no strong opinions on inherent traits, but I'd be somewhat suspicious of pursuing changes to the (To be clear, mine is not an argument against the RFC or Rustdoc; I just worry that by conveniently sidestepping a toolchain issue with a language change we may end up neglecting an opportunity to improve the tool at hand.) |
We discussed this in the @rust-lang/lang meeting. @cramertj will be writing up more, but I wanted to leave one oddball observation I made. Thanks to specialization, it is presently legal to write an "empty impl" that duplicates a blanket impl. So I could write something like: impl<T> Foo for T { fn bar() { } }
impl Foo for u32 { } This is true even though However, this is probably kinda' confusing, and that particular ability of specialization (to write an empty impl like that) is also a bit surprising, so perhaps not the best idea. But worth recording for posterity. =) |
I actually think its nice. I don't think its unclear what this code does: #[inherent] impl ToString for MyType { } The mechanisms that allow it to work are quite surprising, but that doesn't make it hard to learn how to use it. It can make a language feel deeper and more compelling when code you understand how to use has a surprising way that it is implemented, without negatively impacting the learning curve. |
@rfcbot fcp close We discussed this in the lang team meeting today, and everyone agreed it would be nice to provide "inherent trait methods" of some sort. However, we also felt that this feature would probably fall out naturally from a solution to the more general problem of trait and method delegation. We'd like to see another RFC for delegation before we accept a feature like this. |
Team member @cramertj has proposed to close this. The next step is review by the rest of the tagged teams: No concerns currently listed. Once a majority of reviewers approve (and none object), 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. |
True confessions: I kind of like it too. Though I still want delegation to be easy. |
I also like |
I'm signing off to close this due to:
|
(I have no problem with close, but today's discussion crystallized something for me, so I wanted to write it down. Here goes...) I think there's two parts to this that are both interesting:
|
🔔 This is now entering its final comment period, as per the review above. 🔔 |
here's an alternative: macros. pseudocode: macro_rules! inherent ( (ty, trait, tt) => (impl ty { tt } impl trait for ty { tt }) ); use it like: inherent!(PointType, PointTrait, {
fn x(&self) -> i32 {
self.x
}
fn y(&self) -> i32 {
self.y
}
}); |
The final comment period is now complete. |
Closing as the FCP is now complete. |
See #1880
Rendered