Add suggestions for unsafe impl error codes

[nbarrios1337]

Adds suggestions for users to add unsafe to trait impls that should be unsafe, and remove unsafe from trait impls that do not require unsafe

With the folllowing code:

struct Foo {}

struct Bar {}

trait Safe {}

unsafe trait Unsafe {}

impl Safe for Foo {} // ok

impl Unsafe for Foo {} // E0200

unsafe impl Safe for Bar {} // E0199

unsafe impl Unsafe for Bar {} // ok

// omitted empty main fn

The current rustc output is:

error[E0199]: implementing the trait `Safe` is not unsafe
  --> e0200.rs:13:1
   |
13 | unsafe impl Safe for Bar {} // E0199
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^

error[E0200]: the trait `Unsafe` requires an `unsafe impl` declaration
  --> e0200.rs:11:1
   |
11 | impl Unsafe for Foo {} // E0200
   | ^^^^^^^^^^^^^^^^^^^^^^

error: aborting due to 2 previous errors

Some errors have detailed explanations: E0199, E0200.
For more information about an error, try `rustc --explain E0199`.

With this PR, the future rustc output would be:

error[E0199]: implementing the trait `Safe` is not unsafe
  --> ../../temp/e0200.rs:13:1
   |
13 | unsafe impl Safe for Bar {} // E0199
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
help: remove `unsafe` from this trait implementation
   |
13 - unsafe impl Safe for Bar {} // E0199
13 + impl Safe for Bar {} // E0199
   |

error[E0200]: the trait `Unsafe` requires an `unsafe impl` declaration
  --> ../../temp/e0200.rs:11:1
   |
11 | impl Unsafe for Foo {} // E0200
   | ^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: the trait `Unsafe` 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
   |
11 | unsafe impl Unsafe for Foo {} // E0200
   | ++++++

error: aborting due to 2 previous errors

Some errors have detailed explanations: E0199, E0200.
For more information about an error, try `rustc --explain E0199`.

@rustbot label +T-compiler +A-diagnostics +A-suggestion-diagnostics

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cjgillot (or someone else) soon.

Please see the contribution instructions for more information.

[inquisitivecrystal]

Telling people to remove unnecessary unsafe is probably fine. For telling them to add it, should we add a note to tell them to make sure they're fulfilling the safety invariants of the trait? When rustc gives me a suggestion, I sometimes follow it without too much thought, but unsafe traits document invariants that the compiler can't check, so we shouldn't be telling them to implement one if they're not sure they're fulfilled those invariants.

It might even be worth writing it out explicitly, as in "note: this trait has safety invariants that the compiler can't check, so check the documentation to make sure you're fulfilling them". (I wouldn't suggest going with that exact phrasing: I'm exhausted and not thinking or writing very clearly at the moment.)

[compiler-errors]

I was about to comment approximately the same feedback as @inquisitivecrystal -- these should probably be a bit more careful around adding unsafe, maybe they shouldn't be MachineApplicable and/or maybe they should have some more descriptive wording about the implication of adding unsafe.

Removing unsafe seems fine, though.

[nbarrios1337]

This is what I was worried about (in a good way!); I had focused a bit more on the implementation and less on the implications.

How about: "the trait {} has some invariant that the compiler can't check. Review the trait documentation and make sure this implementation upholds those invariants before adding the unsafe keyword" as the warning?

I'll replace the Applicability with MaybeIncorrect or Unspecified.

[compiler-errors]

maybe a bit stronger wording: "the trait {} enforces some invariant", but otherwise sounds good.

[inquisitivecrystal]

This looks pretty good!

I left some minor suggestions that might help the sentences flow a bit better. You could also go the other direction and make everything plural (change to enforces invariants and then leave those invariants in place). Not sure whether that's better.

[nbarrios1337]

I think it best if it is pluralized; To my knowledge, there's no restriction against an unsafe trait having more than one manually-checked invariant

[cjgillot]

@bors r+

[bors]

📌 Commit 770538e has been approved by cjgillot

It is now in the queue for this repository.