Rename only_derive_is_allowed_to_implement_this_trait

[joshlf]

Since our custom derives are not intelligent enough to accept all types for which an unsafe trait impl would be sound, users may in some cases need to implement a trait manually. We should rename only_derive_is_allowed_to_implement_this_trait to something that conveys less of "you aren't supposed to do this" and more "this is dangerous and you'd better know what you're doing and be sure that you need to do this and can't just use the derives".

Since some users may already have manual impls, this is unfortunately a breaking change.

[nbdd0121]

Why not make it an unsafe trait?

[joshlf]

Why not make it an unsafe trait?

They're all unsafe; the function exists to discourage users from implementing the traits manually and steering them towards the derive.

[nbdd0121]

Ah, I see. In that case wouldn't the unsafe itself be a strong enough motivation to steer user away from manual impl? bytemuck doesn't use functions like this.

[joshlf]

My experience has been that many programmers aren't very familiar with unsafe, and so they either just say "oh I need to put unsafe here in order to get this to compile" or else have a vague sense that unsafe is dangerous, but think they understand Rust's memory model and think that their type satisfies the requirements to be e.g. FromBytes when it actually doesn't. Even as the author of zerocopy, I've written unsound implementations of FromBytes before, so this absolutely speaks to the subtlety of unsafe rather than to the skill of these programmers. For those reasons, I've found that unsafe on its own isn't always enough to dissuade people from implementing the traits manually.

[nbdd0121]

Rust 1.66+ have this error message which should help to reduce the possibility that someone just sticks unsafe to get it to compile?

error[E0200]: the trait `MyTrait` requires an `unsafe impl` declaration
 --> <source>:5:1
  |
5 | impl MyTrait for Foo {}
  | ^^^^^^^^^^^^^^^^^^^^^^^
  |
  = note: the trait `MyTrait` enforces invariants that the compiler can't check. Review the trait documentation and make sure this implementation upholds those invariants before adding the `unsafe` keyword
help: add `unsafe` to this trait implementation
  |
5 | unsafe impl MyTrait for Foo {}
  | ++++++

It ultimately should be the responsibility of the one who writes unsafe to ensure that the invariant is upheld. I appreciate the effort to prevent misuse.

The use case that I have that needs manually implementation is I want to add #[derive(AsBytes)] to bitflags!, but they don't compose since bitflags! generates an internal struct which is not AsBytes, requiring a manual unsafe impl. But I don't feel like sticking only_derive_is_allowed_to_implement_this_trait into the impl is the correct solution.

[joshlf]

Ah, so you can use this technique to get the derives to work. Bitflags also already supports some external crates, so you could probably put up a PR to add zerocopy support. It's already been discussed in a few places.

[nbdd0121]

Thanks for the pointer! I wasn't aware that this is possible now with bitflags 2.

[joshlf]

np! Definitely let me know if you end up submitting something upstream because I know of other folks who'd love to make use of it. Also I left this comment, which I suspect will be met with "yeah we know, but unfortunately that doesn't work for these reasons", but maybe we'll get lucky and that'll go somewhere.

[nbdd0121]

I tried the linked technique and zerocopy works now, but I don't have the Debug impl generated by bitflags anymore. So I still have to use a manual impl with only_derive_is_allowed_to_implement_this_trait.