Add derive(Deref) RFC

[GuillaumeGomez]

This PR introduces discussion about adding another derivable trait: Deref (follows-up the newly added derivable trait From).

Rendered

[programmerjake]

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
    }
}

[GuillaumeGomez]

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

[GuillaumeGomez]

I clarified my sentence and also added the problem you mentioned with its example.

[soqb]

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.

[programmerjake]

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.

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.

[GuillaumeGomez]

I think one example is enough, no?

[programmerjake]

rfcs/README.md

Lines 133 to 134 in 11ca03e

	**Specifically, do not squash or rebase commits after they are visible on
	the pull request.**

[GuillaumeGomez]

@programmerjake Dully noted.

[kpreid]

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

[ionicmc-rs]

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

[madsmtm]

Can we lint against the MyPtr1 case? Maybe a rule like "if the deref'd type itself implements Deref"? Or would that be overzealous?

Would be nice to see discussion of that in the RFC text, this derive is somewhat special in that it's the first to implement a trait with an associated type.

[oxalica]

I have a concern that this would encourage users to abuse Deref for non-pointer types, since many other std derives have a sense of "users should just derive everything possible as long as it compiles", ie. Default, Hash, Clone and etc. But the derive(Deref) in this RFC performs almost no check.
The example here is already an anti-pattern that impl Deref for a non-smart-pointer type which does not have a "dereference" semantic at all. Thus the Deref impl breaks the semantics of API that takes an impl Deref bound (from std and/or other ecosystem libraries), eg. what does Pin<TcpPort> even mean?

There is a related derive(CoercePointee) which only works for smart-pointer types. I think we should at least let derive(Deref) require the similar kind of restriction as derive(CoercePointee) does. For example, requiring it to be a struct with a single field of (nested) std pointer types. So #[derive(Deref)] struct MyBox<T>(Pin<Box<T>>); compiles, but #[derive(Deref)] struct TcpPort(u16); definitely should not.

[programmerjake]

I think at the very least, derive(Deref) should work for struct S<T>(Ptr<T>) for any Ptr<T> that implements Deref, e.g. triomphe::Arc

[GuillaumeGomez]

Quoting your anti-pattern link docs:

In general, deref traits should not be implemented if:

1. the deref implementations could fail unexpectedly; or
2. the type has methods that are likely to collide with methods on the target type; or
3. committing to deref coercion as part of the public API is not desirable.

I don't see anything in this example that would match any of these three rules.

[clarfonthey]

I definitely also had a bit of a knee-jerk reaction to seeing the example of a TCP port, but in the case of transparent newtypes, it does feel fine to implement Deref and not DerefMut.

I guess that in this particular case, perhaps we could lint against the specific case of publicly exported types which implement Deref but aren't smart pointers? So, in this case, it's fine for a private tagged newtype to implement Deref because there's no API commitment, but we should warn people for public types because of that commitment.

For example, to me, it makes the most sense that a TCP port type would not implement Deref and potentially have various methods that allow constructing TCP socket addresses and/or opening connections, and just a get method to get the raw integer. It seems like a fair commitment to an API to say that you will never do this. But for private code that just wants to do this temporarily, I guess it's fine, since it's serving a distinct purpose in that case.

[proski]

Let's keep Deref for things that "point to" something or "refer to something" in the semantic sense (I'm not talking about the implementation). TCP port doesn't refer to a number, it wraps a number to prevent operations that don't normally make sense for port numbers (e.g. adding port numbers or using a port number as an index). From/To is the correct semantic here, not Ref/Deref.

[GuillaumeGomez]

Newtypes are good for two things:

1. Add new methods to a foreign type
2. Add extra information to a primitive (like, it's not a number, it's a TCP port)

I don't see how any of that prevents it from implementing Deref.

[ionicmc-rs]

TCP port doesn't refer to a number, it wraps

For example, you also dont add TCP ports, or mask them, this example in specific wasn't well fit

[GuillaumeGomez]

Whether it refers to a number of wrap it, it comes to the same thing in the end. I want to be able to deref to it. Newtypes are exactly meant for that.