lint(unsafe_code): exclude unsafe declarations from lint coverage

[wyfo]

Unsafe declarations of traits, functions, methods, or extern blocks cannot cause undefined behavior by themselves — only unsafe blocks or implementations of unsafe traits can.

The unsafe_code lint previously applied to these declarations, preventing, for example, declarations of unsafe function with safe body. See #108926.

This change removes all such unsafe declarations from unsafe_code coverage.

However, to maintain soundness, unsafe functions with bodies are only allowed when unsafe_op_in_unsafe_fn is set to forbid. This ensures unsafe operations inside such functions require an unsafe block. Declarations of unsafe functions without bodies (e.g., in traits) are always allowed.

Closes #108926

[rustbot]

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

[RalfJung]

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.

[wyfo]

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.

[rustbot]

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.

[rustbot]

⚠️ 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

[petrochenkov]

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?

[nikomatsakis]

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

[Nadrieril]

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.

[nikomatsakis]

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.

[joshtriplett]

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.

[wyfo]

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.

[traviscross]

@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.)

[Nadrieril]

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...

[traviscross]

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();
}

[ia0]

Note that unsafe_code does not always lint when the unsafe keyword is used, 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 (if it were in a contravariant position, then there would most probably be an unsafe block in the function definition). 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)] // this function's incorrectness has soundness consequences
pub fn with_copy(op: impl FnOnce(unsafe fn(*const u8, *mut u8, usize))) {
    op(std::ptr::copy_nonoverlapping); // incorrect, using std::ptr::copy would be correct
}

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
}

Or more simply when defining a type alias which contains the unsafe keyword like type Foo = unsafe fn().

Actually thinking about this, the same should hold for any type that needs an unsafe block to use, like raw pointers. So the treatment of the unsafe function pointer types should be similar to the one of raw pointer types, which is currently coherent.

[wyfo]

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.

You bring an interesting point here, and I think you're right. Once RFC 3245/#100706 lands, user will be able to keep the unsafe keyword only if he needs the unsafe contract, i.e. for unsafe operations in the method body.
Waiting for it, there is again a workaround (both safe and unsafe methods, the latter with a default body calling the former), that I'm already using.

About splitting the lints, I don't see any use case where it would be useful. If a user allows unsafe API declaration, that's necessarily to use it with unsafe assertions in the same crate. In the same way, if a user allows unsafe assertions, which is the real unsafe thing, then I don't see why he would prevent himself to declare unsafe API. Appart of the didactic value, I think it should require a concrete use case to start working on it.

So, now I'm convinced that implementing an unsafe method belongs indeed to "creating an unsafe API", as long as RFC 3245 is concerned, I think I went in the wrong direction and will close the PR if nobody disagree with it.

[ia0]

From #148651 (comment):

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.

With that logic, shouldn't pub static mut also lint (even though it doesn't have the unsafe keyword)? If pub unsafe fn should lint because you can only use it meaningfully (i.e. not pass it around as a function pointer, but really call it) with an unsafe block, then I'm failing to see how pub static mut is different (could be historical and some would argue it should be pub unsafe static mut).

I think the "do nothing" option is insufficient. We should at least fix the lint documentation (if not its behavior, as this PR is doing). Right now, the lint documentation says:

The unsafe_code lint catches usage of unsafe code and other potentially unsound constructs like no_mangle, export_name, and link_section.

with the following explanation:

This lint is intended to restrict the usage of unsafe blocks and other constructs (including, but not limited to no_mangle, link_section and export_name attributes) wrong usage of which causes undefined behavior.

The way I (and apparently other users) understand those sentences, is that the lint is about code that may cause undefined behavior (so not where the undefined behavior would happen, but where the blame would be assigned). This notion of "unsafe code" (which is the lint name) is actually mentioned by the reference under undefined.soundness:

It is the programmer's responsibility when writing unsafe code to ensure that any safe code interacting with the unsafe code cannot trigger these behaviors. unsafe code that satisfies this property for any safe client is called sound; if unsafe code can be misused by safe code to exhibit undefined behavior, it is unsound.

Sadly it doesn't really specify what such unsafe code really is. The fact that the word "unsafe" is rendered as unsafe could hint that the unsafe keyword is present. But is the keyword just an over-approximation or an exact characterization? And should this notion of unsafe code match between the reference and the lint?

Something else that should probably be mentioned in the documentation (which seems to be a general assumption) is that this lint is meant to be used at crate-level with the forbid level. This seems to be the main justification for accepting false positives, by saying that they would be hidden by a true positive in almost all realistic cases.

I can understand that changing the semantics of the lint is not worth the benefit of having less false positives or being more didactic/consistent/unsurprising, but in that case I would expect the documentation of the lint to more accurately describe its intent, behavior, and expected usage. This is not only useful for users, but also for maintainers as the language evolves and decision needs to be made on whether a new language construct should lint or not.