Tracking Issue for "unsafe blocks in unsafe fn" (RFC #2585)

[nikomatsakis]

This is a tracking issue for the RFC "unsafe blocks in unsafe fn" (rust-lang/rfcs#2585).
The lint unsafe_block_in_unsafe_fn is stable, but the RFC proposes some further things we might want to do.

About tracking issues

Tracking issues are used to record the overall progress of implementation.
They are also uses as hubs connecting to other relevant issues, e.g., bugs or open design questions.
A tracking issue is however not meant for large scale discussion, questions, or bug reports about a feature.
Instead, open a dedicated issue for the specific matter and add the relevant feature gate label.

Steps

• Introduce an opt-in lint that, when enabled, causes unsafe blocks in unsafe functions to be considered required, and warns if they are absent when an unsafe operation is performed.
• Stabilization PR in Rust 1.52.0 (Stabilize unsafe_op_in_unsafe_fn lint #79208)
• Include a suggestion with the lint that can insert required unsafe blocks. This could be as simple as adding a block across the entire function, though more granular insertion is probably better. (Add MVP suggestion for unsafe_op_in_unsafe_fn #112017)
• Adjust documentation to describe the (somewhat unusual) effect of the lint, and to describe the possibility that the lint will be enabled default (see instructions on rustc-dev-guide)
• Write a blog post describing the change, perhaps?

Unresolved Questions

• What is the timeline for adding the lint, and cranking up its default level?
• Should the default level depend on the edition?
• Should we ever make this deny-by-default or even a hard error, in a future edition?
• Should we require cargo fix to be able to do something about this warning before making it even warn-by-default? And how precise should it be?

Implementation history

• Implemented in Implement RFC 2585: unsafe blocks in unsafe fn #71862
• Stabilized in Stabilize unsafe_op_in_unsafe_fn lint #79208
• Fixable suggestion added in Add MVP suggestion for unsafe_op_in_unsafe_fn #112017
• Made warn-by-default for 2024 edition in Change unsafe_op_in_unsafe_fn to be warn-by-default from edition 2024 #112038

[nikomatsakis]

cc @RalfJung

[nikomatsakis]

A relevant note towards the implementation:

• Clippy has a helper function for testing whether a lint is enabled.

[LeSeulArtichaut]

I'll try to work on the implementation!

[RalfJung]

If I understand https://boats.gitlab.io/blog/post/ringbahn/ correctly, that API would also benefit greatly from not treating the body of an unsafe fn as unsafe {}: there is a trait with an unsafe fn, and

It’s important to understand what an unsafe methods means: unsafe methods are unsafe to call - meaning that callers must guarantee additional invariants when calling them. However, they are safe to implement (as long as you don’t do anything unsafe inside of them). So when a user implements the Event trait, they are given additional guarantees about how this method will be called that normally they could not assume.

@withoutboats did I understand this correctly?

[withoutboats]

@RalfJung I don't think its a particularly new situation. Basically, the guarantee that you are getting from it being unsafe are the guarantees you need to be able to call these unsafe methods in the SubmissionQueueEvent that is passed as an argument.

In other words, an Event implementation is going to use probably 1 unsafe operation (calling one of these methods), which it can prove is correct based on the invariant that trait requires the caller to maintain. This isn't really different from any other unsafe code.

What I'm trying to say with the blog post is that the Event trait hasn't introduced a new invariant for you to maintain, its introduced a new invariant that I maintain so its easier for you to call that unsafe method you need to call to get any work done.

This is just how unsafe trait methods work in general, so its nothing new. But I agree that unsafe trait methods (where the invariant contract is specified separately from the implementer) are a compelling example of where unsafe blocks in unsafe fns seem beneficial.

[RalfJung]

What I'm trying to say with the blog post is that the Event trait hasn't introduced a new invariant for you to maintain, its introduced a new invariant that I maintain so its easier for you to call that unsafe method you need to call to get any work done.

Is there a realistic safe implementation of that method that ignores the extra invariant provided by the caller, or is the expectation that most/all implementations will have to do something unsafe justified by that invariant?

[withoutboats]

Yea I believe some kinds of events should be safe to prepare because they only send Copy data into the kernel and so they don't need to worry about any kind of memory safety invariant.

[LeSeulArtichaut]

Step 1 can now be checked since #71862 landed.

[withoutboats]

I want to give an experience report. I've been writing a lot of unsafe code recently with ringbahn, and I've been thinking about how this change would have impacted my experience and the correctness of my code. Unfortunately, my thoughts are not optimistic.

My experience makes me think a big downside of this change has been overlooked, and that the upside is not so great. The goals of this change I think would be better served by a separate linting infrastructure.

First, I think this change would dramatically impact user flow when writing new and unfinished unsafe code. During the prototype, experiment, build-out phase of creating new code, I am often unsure where exactly the function boundaries should go, what code will be reusable, what code should be abstracted, what code needs to be inlined, etc. As a result, I am frequently moving code in and out of functions. By marking those functions unsafe as I construct them, the code remains in an unsafe scope right now. But this change would require me to set up new unsafe scopes as I make these sorts of edits, greatly increasing the ceremony. Similar to the arguments I've made in favor of Ok-wrapping, we should minimize the disruption to the fluidity of creating new functions during this phase of editing, and I see this as a major downside.

Second, so far two memory safety bugs have been discovered in ringbahn, and neither of them would have been prevented by this change. In both cases, invariants were not being upheld by safe code (in both cases, interestingly, because of implicit destructors). The bugs this change is designed to catch seem easy to identify and unlikely to occur: accidentally calling an unsafe function that you didn't mean to call. I am doubtful that this would meaningfully improve the quality of unsafe code or the experience of writing it.

The utility of this change seems to be in the later phase of editing, once the code has settled down and is no longer seeing large refactors. At that point, it is ideal for each unsafe operation to be annotated with a comment justifying why the invariants necessary are being upheld. However, this change to the language is a poor match for actually achieving that goal: even if users are required to put unsafe operations in unsafe blocks, nothing actually makes them justify those unsafe operations, or do anything but wrap entire function bodies in unsafe blocks and call it a day (as I often do during prototyping).

For those situations, what would be more appropriate in my opinion is a linter that requires every statement containing an unsafe operation to be annotated with a comment justifying its correctness. This would automatically enforce what is already becoming a standard style for finished unsafe code. It would not require this change at all to complete.

[phil-opp]

even if users are required to put unsafe operations in unsafe blocks, nothing actually makes them justify those unsafe operations, or do anything but wrap entire function bodies in unsafe blocks and call it a day (as I often do during prototyping).

That's no worse than the status quo where all unsafe functions have an implicit unsafe block around their entire body. To me, the biggest advantage of requiring an explicit unsafe block is that it is much easier to make the unsafe scope smaller (just move lines out of the block) compared to creating a second "safe" inner function (which is far from an obvious and requires lots of boilerplate). The problem of annotating each unsafe block seems to be a different problem to me and completely unrelated to this change.

By marking those functions unsafe as I construct them, the code remains in an unsafe scope right now. But this change would require me to set up new unsafe scopes as I make these sorts of edits, greatly increasing the ceremony.

So your problem is that you need to add an extra unsafe block around function bodies when prototyping? Maybe an option to only warn instead of error on missing unsafe blocks would be good solution for this? By enabling this flag during prototyping, you could add the required unsafe blocks later when you decided how to structure your code. This option could be limited to cargo check so that an actual compilation with this flag wouldn't be possible.

Also, IDE support for adding required unsafe blocks automatically might help with this too. When prototyping and refactoring, I often need to change the mutability of variables, which I found quite annoying without IDE support. With rust-analyzer however, I can now simply apply a suggestion, which solves the problem for me completely.

[RalfJung]

During the prototype, experiment, build-out phase of creating new code, I am often unsure where exactly the function boundaries should go, what code will be reusable, what code should be abstracted, what code needs to be inlined, etc. As a result, I am frequently moving code in and out of functions. By marking those functions unsafe as I construct them, the code remains in an unsafe scope right now. But this change would require me to set up new unsafe scopes as I make these sorts of edits, greatly increasing the ceremony. Similar to the arguments I've made in favor of Ok-wrapping, we should minimize the disruption to the fluidity of creating new functions during this phase of editing, and I see this as a major downside.

I am fully buying your argument with Ok-wrapping, but am unconvinced of it in the context of unsafe blocks. I think that is because with Ok-wrapping, even having that added implicitly for the final product seems like a great state of affairs to me, whereas for unsafe it feels like a prototyping-only kludge.

So maybe a better solution to your problem, instead of keeping unsafe fn internally unsafe (with not just the practical but also the conceptual issues that brings) would be the ability to have unsafe blocks outside functions, so you could wrap an entire module in unsafe and just not care about safety for the time being? That seems to better reflect your prototyping intent.

Another alternative would be a more convenient way to re-gain the ability to mark the entire body of a function as unsafe. For sample code, I personally use

fn main() { unsafe {
} }

but rustfmt will turn this into a double-indent. We could adjust rustfmt, or we could adopt @Centril's proposal of defining functions with =, which would permit

fn main() = unsafe {
}

which seems like minimal effort even during prototyping, while at the same time not mixing up the two different roles of "unsafe" that unsafe fn currently fails to distinguish.

[withoutboats]

Some interesting ideas here definitely. The idea of allowing unsafe scopes outside of individual function bodies for prototyping especially appeals to me (I'd really like it as some kind of #![] pragma, but we can get to that). I agree that there may be a high point on the curve where this change plus some other change can avoid the drawback I've described.

However, I note that neither of you responded directly to my second claim that this is not particularly useful for catching bugs. Both posts contain sort of oblique responses, which I want to look at in particular:

To me, the biggest advantage of requiring an explicit unsafe block is that it is much easier to make the unsafe scope smaller (just move lines out of the block) compared to creating a second "safe" inner function (which is far from an obvious and requires lots of boilerplate).

Why do you want an interior safe scope? If its to avoid accidentally performing an unsafe op, as I understand it, linters like the one I've described would easily suffice. To reiterate my previous claim: as we all know, it is not only the unsafe ops that can cause memory safety inside a module with unsafe code; singling out these ops as the unique points at which extra care is needed gives a false sense of security in my mind.

(with not just the practical but also the conceptual issues that brings)

I get the sense from talking to you that this conceptual issue is a really big motivator for you. I agree that it is unclear the way that unsafe annotates two different sides of the safety contract. However, I am not as convinced that this change will make a big impact on ellucidating the distinction, because we are still using the unsafe keyword in both positions. I think the use of one keyword for both roles (which I think we aren't going to change) is the real stopper here, not the fact that in some cases the keyword plays multiple roles.

[phil-opp]

Why do you want an interior safe scope? If its to avoid accidentally performing an unsafe op, as I understand it, linters like the one I've described would easily suffice.

To me the only difference between safe and unsafe functions is that the latter have additional memory safety related requirements for callers. I see no reason to treat the bodies of safe and unsafe functions differently, so it makes sense to me to use the same mechanism for marking unsafe operations.

Of course we could also create a lint that requires an annotation for each unsafe operation, but this should apply to safe and unsafe functions in the same way. If we created such a lint and at some point decided that this lint is enough to mark unsafe operations, I would see no reason why unsafe blocks shouldn't be made optional in bodies of safe functions too.

To reiterate my previous claim: as we all know, it is not only the unsafe ops that can cause memory safety inside a module with unsafe code; singling out these ops as the unique points at which extra care is needed gives a false sense of security in my mind.

Again, there is no difference between safe and unsafe function bodies to me. Yes, usage of unsafe can pollute a whole module, but it is still useful to mark the places where the actual memory safety is caused. Otherwise, we could also make unsafe blocks in safe functions optional as soon as there is some unsafe used somewhere in the same module, which does not sound like a good idea to me.

I note that neither of you responded directly to my second claim that this is not particularly useful for catching bugs.

It's as useful as unsafe blocks in safe functions. Yes, the actual bugs might be outside unsafe blocks, but unsafe blocks are still useful because they mark the places where additional invariants are required. So even if the bug is somewhere else, you have a place where you can start your debugging, e.g. by adding additional assertions or debugger breakpoints.

[awnion]

(NOT A CONTRIBUTION)

TLDR: I'm pro lint #![warn. I'm against a #![deny by default.

Nobody mentioned that unsafe syntax behavior is aligned with async syntax behavior Id est:

unsafe fn u() {
    // whole block is unsafe {...}
}

async fn a() {
    // whole block is async {...}
}

Lint warning I think would be good enough from just a pedagogical perspective. Deny by default would be just annoying at best.

[RalfJung]

async and unsafe are keywords of very different nature, so the fact that they are aligned is actually bad. async is a form of effect; effects need to be propagated outwards. unsafe is not an effect, it is a proof obligation; it actually has two different meanings depending on where it is used. See the keyword reference for more discussion of this topic.

[awnion]

async and unsafe are keywords of very different nature, so the fact that they are aligned is actually bad. async is a form of effect; effects need to be propagated outwards. unsafe is not an effect, it is a proof obligation; it actually has two different meanings depending on where it is used. See the keyword reference for more discussion of this topic.

I agree. I just felt like would be nice to point out this similarity.

Also if you'd ask me I'm against both async fn syntax and unsafe fn.
I'd be good with fn ... -> impl Future<...> only. The same with unsafe. But we have what we have...

[digama0]

I'd be good with fn ... -> impl Future<...> only. The same with unsafe.

What does that mean? As Ralf said, they are very different features so I don't know how to analogize that to get "the same with unsafe" - fn ... -> impl Unsafe<...> makes no sense.

[awnion]

I'd be good with fn ... -> impl Future<...> only. The same with unsafe.

What does that mean? As Ralf said, they are very different features so I don't know how to analogize that to get "the same with unsafe" - fn ... -> impl Unsafe<...> makes no sense.

You're right. Just wanted to share a perspective on how someone might see it.

[pigeonhands]

If unsafe was changed so that the body of the function no longer has a unsafe block around it, a macro/annotation could be added to return the old behavior and avoid the indent issue.

So for example, these two functions would be identical

#[unsafe_body]
unsafe fn foo() { 
    ...
    my_unsafe_function()
}

unsafe fn foo() { 
    unsafe{
        ...
        my_unsafe_function()
    }
}

[traviscross]

Note that landing this in Rust 2024 probably depends on resolving #119823.

[traviscross]

Note that #119823 is now resolved, clearing the way forward for this.

[traviscross]

@rustbot labels +I-lang-nominated

Given the resolution of #119823, let's discuss what else (if anything) needs to happen to land this in Rust 2024.

[traviscross]

@rustbot labels -I-lang-nominated

We discussed this today in triage and agreed that we would like to make this a warn-by-default lint in Rust 2024 with the intent to later make this a warn-by-default lint across all editions.

We will be proposing FCP merge to this effect.

[tmandry]

I've opened #120535 for making this lint warn-by-default in the 2024 edition and proposed FCP merge.

[traviscross]

We'll track the aspect of this related to Rust 2024 separately here:

• Tracking Issue for Rust 2024: Make unsafe_op_in_unsafe_fn warn-by-default #123916