r? @petrochenkov

rustbot has assigned @petrochenkov.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Unsafe declarations of traits, functions, methods, or extern blocks cannot cause undefined behavior by themselves

At least for extern blocks, that is not correct. The mere presence of an incorrect extern block can cause UB. See #46188.

At least for extern blocks, that is not correct. The mere presence of an incorrect extern block can cause UB. See #46188.

Indeed, my bad, I've rolled back this part.

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

⚠️ Warning ⚠️

• There are issue links (such as #123) in the commit messages of the following commits.
  Please move them to the PR description, to avoid spamming the issues with references to the commit, and so this bot can automatically canonicalize them to avoid issues with subtree.
  • bf272c0

I'll send this to lang team for a vibe check.
Are you willing to reconsider the decision from #108975 (comment) and not report unsafe fn/trait definitions (as opposed to their uses) in unsafe_code lint?

We discussed this in the @rust-lang/lang meeting today. We did not reach a consensus but there were several points made in various directions.

Everyone agreed that there is didactic value to being precise about when/where you are making an unsafe assertion (i.e., writing trust code that may cause UB if wrong) versus creating an unsafe API (and leaving the assertions to consuming crates).

Many felt that, nonetheless, in practice when they used this lint they meant that they would prefer to have neither unsafe assertions nor unsafe APIs.

The counterexample was something like Send or TrustedLen, where you are declaring a trait that brings obligations but not fulfilling them. (Though even there, you are likely to have some unsafe code that takes advantage of that trait as well.)

No option had meeting consensus. The options we considered are:

• Do nothing, document that lint means "no use of the unsafe keyword with either sense"
• Adopt this proposal, modify the lint to fire only on unsafe assertions and not on declarations of unsafe APIs.
• Or, create a lint group, where you have unsafe_code and break it down into unsafe_assertions or unsafe_apis -- but there was no real discussion of those names, I made them up now.

EDIT: added the 'do nothing' option

There's also the "do-nothing" option, where we'd document that the lint means "no unsafe keyword at all", under the hypothesis that the need for separating the two lints is very niche.

The language doesn't currently permit this, afaik, but I think another use case would be:

trait Foo {
    // Allowed to assume P is true
    unsafe fn bar(&self);
}

impl Foo for () {
    fn bar(&self) { 
        // but I don't need or want to actually make use of this unsafe predicate P in *my impls*
    }
}

...it'd be nice to have a real example though. Most of the cases I've thought of still wind up with some unsafe code somewhere.

Elaborating on the use case for #![forbid(unsafe_code)] forbidding the declaration of unsafe APIs: this helps if you know in advance that "if any APIs in this crate are unsafe to call, they're designed wrong". That's a use case for the current behavior of unsafe_code.

That said, 👍 for making it a lint group with separate lints for the two cases, so that you can forbid one but not the other.

Everyone agreed that there is didactic value to being precise about when/where you are making an unsafe assertion (i.e., writing trust code that may cause UB if wrong) versus creating an unsafe API (and leaving the assertions to consuming crates).

Actually, implementing an unsafe trait method is neither an unsafe assertion, nor creating an unsafe API. It's a third category on its own. This is by the way the use case mentioned in the linked issue #108926.
Here is an example from one of my projects:

pub trait WriteSlot {
    fn slot_size(&self) -> usize;
    /// # Safety
    /// `slot` slice's length must be equal to the result of [`Self::slot_size`]
    unsafe fn write_slot(self, slot: &mut [u8]);
}

I would like to enable the user to use unsafe optimizations (skip bounds check, etc.), but he might prefer using safe code instead, and that's perfectly fine. But forbid(unsafe_code) says no. Fortunately, there is a workaround I mentioned in the issue, so can live with the current statu quo.

EDIT: Thinking a bit more about it, implementing an unsafe trait method is quite similar to implementing a trait method whose one argument have a Send bound. The difference is that the unsafeness is embedded in the type system. But similarly, when implementing for example an Executor trait, you may rely on the Send bound for some unsafe blocks, or you may only use safe code, delegating to an internal executor.

@wyfo is right, that is the use case. Let's say we have a crate that is #![forbid(unsafe_code)] but that crate has a dependency with:

// # Upstream
pub trait Tr {
    /// SAFETY: Caller must ensure that `x` is even and non-zero.
    unsafe fn op(x: u8);
}

Then in our crate:

// # Our crate
#![forbid(unsafe_code)]
pub struct OurThing;
impl Tr for OurThing {
    /// SAFETY: This is safe to call with any value.
    unsafe fn op(x: u8) {
        println!("`x` is {x} and we're OK with zero and odd numbers");
    }
}

If we had #100706 ("refined trait impls", RFC 3245), then we'd probably drop the unsafe on the impl, refining it, but that's not necessary for the argument.

The argument is simply that 1) there's no possibility of unsafety, 2) this makes sense for all parties to do.

It makes sense for the upstream to do this so as to leave room for downstreams to be able to unsafely use this invariant in impls. It makes sense for us, as one downstream, to implement the trait (for whatever reason) but in an entirely safe way (not making use of the invariant), consistent with our forbid(unsafe_code) assertion.

(This is similar, of course, to @nikomatsakis' example in #148651 (comment); I highlight only that the inter-crate relationship may be important to the motivation and that the argument might still work without RFC 3245, though it works better with it.)

Oh, I was sure that RFC 3245 was already stable behavior!

Actually, implementing an unsafe trait method is neither an unsafe assertion, nor creating an unsafe API.

Assuming that RFC was stable, then I'd categorize "implementing an unsafe method" as "creating an unsafe API" because you chose to implement it as unsafe fn instead of fn. Without the RFC though, it's indeed a special third thing.

In the meeting I was in favor of changing the lint but I'd find it unfortunate if our reason for changing it was that the lack of RFC 3245 forces our hand...

We could construct the example such that the #![forbid(unsafe_code)] crate is not exposing an (actually safe to call but) unsafe fn method impl but where the impl is still useful, by being used internally in the crate in a safe way, thusly:

// # Upstream
pub trait Tr {
    final fn f() {
        // SAFETY: We ensure that `x` is even and non-zero.
        unsafe { Self::op(2) };
    }
    /// SAFETY: Caller must ensure that `x` is even and non-zero.
    unsafe fn op(x: u8);
}

// # Our crate
#![forbid(unsafe_code)]
struct OurThing; // NOTE: Not `pub`.
impl Tr for OurThing {
    /// SAFETY: This is safe to call with any value.
    unsafe fn op(x: u8) {
        println!("`x` is {x} and we're OK with zero and odd numbers");
    }
}

fn main() {
    // This is safe.
    OurThing::f();
}

Note that unsafe_code does not always lint when the unsafe keyword is used (including when used in an unsound way), contrary to what the "do nothing" option claims. So maybe there should be a fourth option to lint on all usages of the unsafe keyword.

The problem occurs when the unsafe keyword is used in a covariant position which is not the top-level position of a function signature. Here's an example:

/// Abstracts over a copy implementation.
///
/// # Safety
///
/// The operation `op` must call its argument as if it were `ptr::copy()`. In particular, the
/// pointers must be valid (but may overlap).
#[forbid(unsafe_code)] // even though this function is unsound
pub fn with_copy(op: impl FnOnce(unsafe fn(*const u8, *mut u8, usize))) {
    op(std::ptr::copy_nonoverlapping); // using std::ptr::copy is sound
}

fn main() {
    let mut data = *b"hello world";
    // SAFETY: The pointers are valid (and overlap, but that's authorized).
    with_copy(|copy| unsafe { copy(data.as_ptr(), data[4..].as_mut_ptr(), 7) });
    println!("{}", std::str::from_utf8(&data).unwrap());
}

A similar example can be written using the return type rather than an argument of an argument:

#[forbid(unsafe_code)]
fn get_copy() -> unsafe fn(*const u8, *mut u8, usize) {
    std::ptr::copy_nonoverlapping
}