-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add derive(Deref) RFC
#3911
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
base: master
Are you sure you want to change the base?
Add derive(Deref) RFC
#3911
Conversation
5c730be to
71ef5a8
Compare
1aeaa35 to
db0edb1
Compare
db0edb1 to
bd87e29
Compare
text/0000-derive-deref.md
Outdated
| # Unresolved questions | ||
| [unresolved-questions]: #unresolved-questions | ||
|
|
||
| Should we also derive `DerefMut` with the same conditions as described 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.
I often don't want DerefMut on something that implements Deref, so I don't think derive(Deref) should implement DerefMut, however I think having a separate derive(DerefMut) would be awesome.
One thing to consider with derive(DerefMut) is what happens when you implement Deref manually but derive(DerefMut) -- should it error when Deref::Target isn't the expected type? If you don't explicitly check for it, the deref_mut() could end up coercing the return type to fit. e.g.:
#[derive(DerefMut)] // should this error or rely on silent coercion of String to str?
pub struct S(String);
impl Deref for S {
type Target = str;
fn deref(&self) -> &str {
&self.0
}
}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.
You made me realize I formulated my sentence badly. I meant adding a new DerefMut derivable item with the same condition as described for Deref.
And it's exactly for such cases that I didn't want to put DerefMut in this RFC. :)
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 clarified my sentence and also added the problem you mentioned with its example.
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.
it should be noted there's already this kinda incongruity possible with PartialOrd and Ord, and, to a lesser extent, (Partial)Eq. i think we lint in this case but can't remember off the top of my head.
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.
it should be noted there's already this kinda incongruity possible with
PartialOrdandOrd, and, to a lesser extent,(Partial)Eq. i think we lint in this case but can't remember off the top of my head.
yes, except none of those can cause a type error or require type coercion. Deref is different because it has an associated type that DerefMut uses.
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 think one example is enough, no?
bd87e29 to
3206f4c
Compare
|
Lines 133 to 134 in 11ca03e
|
|
@programmerjake Dully noted. |
|
|
||
| Using `#[derive(Deref)]` on unions produces a compilation error. | ||
|
|
||
| # Drawbacks |
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.
One disadvantage of defining #[derive(Deref)] as this RFC does is that it will do the wrong thing for smart pointer types.
#[derive(Deref)]
struct MyPtr1<T>(Box<T>);MyPtr1 will deref to Box<T> rather than T, which is likely harmless but may reveal an implementation detail, increase the number of *s needed, or in the case of derive(DerefMut), allow breaking an invariant.
#[derive(Deref)]
struct MyPtr2<T>(*mut T);MyPtr2 will dereference to a raw pointer which can't be safely dereferenced, whereas the intent would presumably be to dereference to T.
I don’t think that this is a reason not to provide this derive at all, but it is something that I think should be noted explicitly as a hazard. (The #[deref(forward)] attribute proposed below would allow getting the right behavior from MyPtr1, but users would still need to know to add 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.
Adding to that, #[deref(forward)] should raise a compile error when used with raw pointers (or any type unsafe to Deref if we add more).
Or, we could make an unsafe attribute for that case
text/0000-derive-deref.md
Outdated
| # Drawbacks | ||
| [drawbacks]: #drawbacks | ||
|
|
||
| While this does enable more Rust code to "just work", it also means that we should be able to produce high-quality error messages in the compiler, as it is trivial to detect how many fields a struct or an enum variant has. |
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 don’t understand how this paragraph describes a drawback. Did it belong to a different section?
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 think they meant that we need to be careful making the error messages
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.
Yes, I'll reword it to make it more obvious.
text/0000-derive-deref.md
Outdated
| #[derive(derive_more::Deref)] | ||
| struct Point(#[deref] i32, i32); | ||
| ``` | ||
| - You can use the `#[forward]` attribute to use the `Deref` target of the current derefed item. |
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.
| - You can use the `#[forward]` attribute to use the `Deref` target of the current derefed item. | |
| - You can use the `#[deref(forward)]` attribute to use the `Deref` target of the current derefed item. |
text/0000-derive-deref.md
Outdated
| # Future possibilities | ||
| [future-possibilities]: #future-possibilities | ||
|
|
||
| ## `#[forward]` attribute |
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.
Consistency:
| ## `#[forward]` attribute | |
| ## `#[deref(forward)]` attribute |
text/0000-derive-deref.md
Outdated
| #[deref(forward)] | ||
| struct MyBoxedInt(Box<i32>); |
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 think it would be a better syntax if the #[deref(forward)] attribute were placed on the field being dereferenced exactly like #[deref] is. That way, #[deref(..)]’s placement always has a single meaning and forward is a modifier to it rather than something different.
| #[deref(forward)] | |
| struct MyBoxedInt(Box<i32>); | |
| struct MyBoxedInt(#[deref(forward)] Box<i32>); |
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.
Yeah, that also solves all possible ambiguities. Good idea!
text/0000-derive-deref.md
Outdated
| impl derive_more::core::ops::Deref for MyBoxedInt { | ||
| type Target = <Box<i32> as derive_more::core::ops::Deref>::Target; | ||
| #[inline] | ||
| fn deref(&self) -> &Self::Target { | ||
| <Box<i32> as derive_more::core::ops::Deref>::deref(&self.0) |
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.
| impl derive_more::core::ops::Deref for MyBoxedInt { | |
| type Target = <Box<i32> as derive_more::core::ops::Deref>::Target; | |
| #[inline] | |
| fn deref(&self) -> &Self::Target { | |
| <Box<i32> as derive_more::core::ops::Deref>::deref(&self.0) | |
| impl ::core::ops::Deref for MyBoxedInt { | |
| type Target = <Box<i32> as ::core::ops::Deref>::Target; | |
| #[inline] | |
| fn deref(&self) -> &Self::Target { | |
| <Box<i32> as ::core::ops::Deref>::deref(&self.0) |
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.
👍
text/0000-derive-deref.md
Outdated
| type Target = $t; | ||
|
|
||
| fn deref(&self) -> &Self::Target { | ||
| self.0 |
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.
| self.0 | |
| &self.0 |
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.
👍
text/0000-derive-deref.md
Outdated
| type Target = $t; | ||
|
|
||
| fn deref(&self) -> &Self::Target { | ||
| self.$n |
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.
| self.$n | |
| &self.$n |
text/0000-derive-deref.md
Outdated
| type Target = $t; | ||
|
|
||
| fn deref(&self) -> &Self::Target { | ||
| self.$f |
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.
| self.$f | |
| &self.$f |
text/0000-derive-deref.md
Outdated
| # Future possibilities | ||
| [future-possibilities]: #future-possibilities | ||
|
|
||
| ## `#[forward]` attribute |
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.
It could also be handy to allow multiple deref operations:
#[derive(Deref)] // generated Deref::Target = T
pub struct CowArc<'a, T> {
#[deref(forward = 2)] // maybe `#[deref(count = 2)]`?
v: Cow<'a, Arc<T>>,
}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 don't understand what you're suggesting.
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 suppose they mean that #[deref(forward = n)] would deref that field n times
Their example would deref to T rather than Arc or Cow
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. Adding this suggestion then.
text/0000-derive-deref.md
Outdated
| } | ||
| ``` | ||
|
|
||
| In case we want to deref more than once, we could allow a non-null positive integers value: |
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.
| In case we want to deref more than once, we could allow a non-null positive integers value: | |
| In case we want to deref more than once, we could allow a positive integer value: |
null is an unusual way of describing zero. also zero doesn't count as a positive integer.
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 PR introduces discussion about adding another derivable trait:
Deref(follows-up the newly added derivable traitFrom).Rendered