`unsafe_code` lint fires for `unsafe fn` with safe body

[chbaker0]

#![forbid(unsafe_op_in_unsafe_fn)]
#![forbid(unsafe_code)]

trait Foo {
    unsafe fn dangerous();
}

struct SafeImpl;

impl Foo for SafeImpl {
    unsafe fn dangerous() {
        println!("this is safe!");
    }
}

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2021&gist=3f3fbdb3494c6770487823f284c303f9

With forbid(unsafe_op_in_unsafe_fn), an unsafe fn's body in principle no longer has an implicit unsafe block. So, unsafe_code should not trigger on an unsafe fn alone: there is not necessarily any unsafe code inside!

This has practical effect. Consider one crate defining interfaces with unsafe methods (e.g. representing an OS API). Another crate wants to create mock implementations of these interfaces that are 100% safe. While any crate consuming these mocks will necessarily contain unsafe {...} blocks, it is nice for the test support crate to declare it itself has only safe code.

[chbaker0]

@rustbot label +A-diagnostics

[mu001999]

What about unsafe traits? They are also forbidden by lint unsafe_code.

[chbaker0]

Yes, unsafe trait items should be allowed too IMO, it's only unsafe impl that's actually unsafe.

Both unsafe trait and unsafe fn say "users must follow my contract", while unsafe { ... } blocks and unsafe impl say "I'm following the contracts defined by the interfaces I'm using". Only the latter are asserting things the compiler can't check.

[Ezrashaw]

@rustbot claim

[RalfJung]

I agree, the current behavior is quite confusing (but at least it is consistent between unsafe fn and unsafe trait).

[ia0]

I agree there seems to be a general confusion regarding unsafe, so I decided to write down my mental model to help education in that regard (the main idea being to make the proof type explicit as it would be in dependent type programming languages). This general confusion could be decomposed in the following core confusions:

• Confusion between the unsafe keyword in types and in code. In types, the unsafe keyword represents properties (things to prove). In code, the unsafe keyword represents proofs (proving the properties of the unsafe keyword in the associated type). This confusion is accentuated by the fact that it's the same unsafe keyword in both cases (versus say using unsafeprop in types and unsafeproof in code, e.g. unsafeprop trait and unsafeproof impl).
• Confusion between pre- and post-conditions, or more generally between requirements and guarantees. Requirements are things to prove. Guarantees are things you can rely on to prove requirements. This confusion is accentuated by the fact that there is currently no simple way to have post-conditions (there's only unsafe traits and it's quite a heavy encoding) and by the fact that "pre" and "post" may be interpreted as the code "before" and "after" the function call while it is actually about the properties to provide and receive from the function call. In particular, pre-conditions may refer to code after the function call (like Pin::get_unchecked_mut or String::as_mut_vec) and post-conditions may refer to data before the function call.

I am not recommending Rust to change anything. I am just trying to lift this general confusion regarding unsafe through educational materials. Whether Rust should change anything (and how) is up to the community as a whole. In particular, lifting the confusion should be enough to clarify issues like this one. Once the first core confusion is lifted it should be obvious that unsafe fns and unsafe traits are not unsafe code and should not lint. Only unsafe blocks and unsafe impls should lint.

[wyfo]

I was myself facing the same issue a few months ago. And because it arose in another project last week, I decided to open #148651 to exclude unsafe declarations from the lint coverage, as they cannot cause UB by themselves.

But because of the legacy behavior of unsafe functions, my PR adds the condition of having unsafe_op_in_unsafe_fn set to forbid to allow unsafe function with a body (method without body, as well as traits are always allowed).

EDIT: I don't know how I missed the discussion #108975 🤦‍♂️ I don't think after reading it that my proposal goes in the right direction.
However, the previous PR comments contains interesting points that should be repeated here:

• declaring unsafe traits often comes with using them with unsafe code, so even if counter-intuitive for the majority, relaxing the restriction is not a priority
• the real problem faced by users, including me, is for trait's unsafe methods implementation. However, Tracking Issue for refined trait implementations #100706 may provide a better solution to the problem.

[wyfo]

About #100706 as a possible solution, I think it might take quite a bit of time to be implemented and stabilized. On the other hand, "fixing" the lint according to the consensus about declaration of unsafe precondition vs discharging of unsafe obligations would surely be quicker and may unblock users encountering the issue.

On the library author side, my hack is to declare unsafe method with a default body calling a _safe-suffixed method to be implemented. For example

pub trait WriteBytesSlice: Sized {
    fn size(&self) -> usize;
    /// # Safety
    ///
    /// `slice.len()` must be equal to result of a previously called [`Self::size`]
    unsafe fn write(self, slice: &mut[u8]) {
        self.write_safe(slice);
    }
    /// Allows to provide a safe implementation of [`Self::write`]
    fn write_safe(self, slice: &mut [u8]);
}

So, AFAIC, I can wait #100706 thanks to this hack.