C-DEREF contradicts idiomatic API in the standard library

[matklad]

C-DEREF unambiguously says that Deref is only for smart pointers. However I've noticed (via this tweet) that there's another case where I reach out for Deref. It's not immediately clear to me who is wrong, the API guidelines or my code, so I am raising this issue to figure this out!

EDIT: https://github.com/rust-lang/api-guidelines/issues/249#issuecomment-904446836 gives much better examples

I often implement Deref for newtype struct pattern -- when the struct has a single field, and exists to enforce some static invariant. A good example here is MainfistPath type from rust-analyzer, which is a wrapper around Path which guarantees that parent is not-None. I implement Deref here because ManifestPath is a Path, and I want to get all the methods for free.

Another case is somewhat more obscure, and relates to this code. There, I have a hashbrown hash map of Nodes, but I use the raw API to supply my own, custom Hash. The code currently has a bug where, in one place, default hash impl is used, because Node: Hash. I want to do the following:

struct NoHash<T>(T);
// impl<T> !Hash for NoHash<T> {}

impl<T> Deref for NoHash<T> {}

Again, the reasoning here is that I wrap the type as is, and it would be convenient to get access to all the methods for free.

I suggest focusing on the first case, at it seems more representative to me:

// Current implementation:
pub struct ManifestPath { file: AbsPathBuf }

impl ops::Deref for ManifestPath {
  type Target = AbsPath;

  fn deref(&self) -> &Self::Target { 
    &*self.file 
  }
}

impl ManifestPath {
  // Shadow `parent` from `Deref`.
  pub fn parent(&self) -> &AbsPath {
    self.file.parent().unwrap()
  }
}

// Implementation endorses(?) by the guidelines:

pub struct ManifestPath { file: AbsPathBuf }

impl ManifestPath {
  pub fn file(&self) -> AbsPath { 
    &*self.file 
  }
  pub fn dir(&self) -> &AbsPath {
    self.file.parent().unwrap()
  }
}

Some specific questions:

• Does the "current version" violate the guideline? To me, it seems that it clearly does, as ManifestPath is not a smart pointer, but my definition of smart pointer might be wrong.
• What are specific problems caused by the Deref impl? I personally don't see any (not to say that there aren't any)
• What's the best guideline-compliant way to write ManifestPath?

Quick googling showed one post wich says you can implement Deref for newtypes (cc @JWorthe): https://www.worthe-it.co.za/blog/2020-10-31-newtype-pattern-in-rust.html

[Ixrec]

My impression was that this newtype Deref pattern was mostly a hackaround for the lack of a proper "delegation" feature (or https://crates.io/crates/ambassador). Unfortunately I've forgotten most of the details since the last time this was thoroughly discussed, so I might be completely off-base here for state-of-the-art Rust, but: I believe the main limitations are that this does nothing for delegation use cases beyond a simple newtype, and even with a simple newtype it can quickly lead to ambiguity and messy error messages if there's a method name collision somewhere or just too many refs and derefs happening in close proximity (though, again, this may be out of date; I don't recall any concrete examples). I wouldn't be surprised if it's the least bad option for some newtypes, but IIRC it was one of those solutions that "works until it doesn't" and usually has a more robust alternative.

[CAD97]

FWIW, the ship has long since sailed on "only smart pointers deref".

Stable std counterexamples:

• https://doc.rust-lang.org/stable/std/mem/struct.ManuallyDrop.html
• https://doc.rust-lang.org/stable/std/panic/struct.AssertUnwindSafe.html

Unstable std:

• https://doc.rust-lang.org/stable/std/lazy/struct.Lazy.html

Big name library counterexamples:

• https://docs.rs/lazy_static/1.4.0/lazy_static/macro.lazy_static.html
• https://docs.rs/once_cell/1.8.0/once_cell/sync/struct.Lazy.html
• https://docs.rs/shrinkwraprs/0.3.0/shrinkwraprs/

If your newtype is transparent (that is, not repr, but that &Newtype -> &Wrapped is fine) and is primarily for the fact it's a new type, not to introduce any sort of restriction, abstraction, or encapsulation, then it (imho) absolutely makes sense to impl Deref.

However, I should note that this doesn't override the "Deref XOR inherent methods" choice. The "go ahead and Deref" is on the grounds that you don't want/need to add any new methods, and that any required/desired additional functionality is fine with being accessed as Newtype::function.

Or IOW, don't struct Newtype(pub T) and .0 or .inner() everywhere, when that's just extra unnecessary line noise; you already satisfy the prior above point, and if you're .0/.innering a lot then respecting the latter above point will still reduce the typing (as in type system) overhead.

If it were up to me, we should strike C-DEREF (to avoid confusion/misleading advice) or reword it to be more about "don't introduce methods that could clash" (or IOW act like a smart pointer, even if it's into the exact same memory location) than "be a smart pointer".

(And of course, guidelines are meant to be bent when they don't apply as directly. I think the important thing is to realize when you impl Deref that you're saying that any place you can use &Target you should be able to use &Wrapper. Not quite "is-a"; perhaps "as-a". Either way, you're running up against the same class of issues that you get from misusing inheritance.)

[Freax13]

[...] or reword it to be more about "don't introduce methods that could clash" [...] than "be a smart pointer".

This sounds similar to C-SMART-PTR.

[matklad]

AssertUnwindSafe and ManuallyDrop examples convince me that the current guideline as stated is clearly wrong/misleading, and that we need to do something about this.

[CAD97]

Minor note: ManifestPath isn't in violation of C-DEREF (but it is in violation of C-SMART-PTR)!

This is because it wraps AbsPathBuf but derefs to AbsPath. It is thus still a smart pointer to AbsPath, as AbsPathBuf is, as it Derefs to the heap path rather than the inline buf. (However, I still would support ManifestPath being an unsized transparent wrapper of AbsPath which impls Deref<AbsPath>)

In any case, both C-DEREF and C-SMART-PTR I think are aimed much more strongly at generic for<T> Deref<T> for MyType<T> than they are at a specific Deref<KnownType> for MyType, which has a much less open-ended implication (though still open-ended in that KnownType can evolve semver-compatibly separately from MyType).

I think I'm personally fine with ManifestPath deliberately shadowing AbsPath here, as it is specifically to provide the same semantics, just enhanced with the added information that it has (by wrapping AbsPathBuf and controlling mutation). As such, I (personally) would restrict C-SMART-PTR to be roughly the two part

• Items which Deref to a generic type do not add inherent methods
• Items which Deref to a concrete type only shadow inherent methods with equivalent functionality

For example, consider a type which implements Deref<Path>, perhaps Utf8Path. Utf8Path should be able to be used as-a Path, so it implements Deref<Path>. Additionally, .to_str() can return &str directly, rather than Option<&str>, due to the additional type state guarantees provided by the wrapper. As such, it may implement .to_str(). so long as the implementation agrees with .deref().to_str() on what .to_str() means. However, Utf8Path may not implement .try_is_file(), because that method is not defined by Path, so it may conflict with a future definition of .try_is_file().

[CAD97]

Also, I'd just like to note that C-DEREF says that Deref should only be used for the purpose of smart pointers, but does not actually define what a smart pointer is. A reading could also interpret the guidance not as "only for smart pointers," but rather "only for the purpose of types which specifically want the implicit derefs and interaction with method resolution, which was designed with smart pointers in mind."

I think C-DEREF can stick around in a more clear fashion, which doesn't just say "only for smart pointers" but instead acknowledges the implicit meanings of Deref and specifies that types should implement Deref only if they want all of those behaviors, and give the as-a rule as a guiding principle.

[CAD97]

I've PRd #251 as a smaller rewording which should be fairly unobjectionable.

[Freax13]

The book also encourages implementing Deref for newtypes: https://doc.rust-lang.org/book/ch19-03-advanced-traits.html?highlight=Deref#using-the-newtype-pattern-to-implement-external-traits-on-external-types