Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
3 of 5 tasks
nikomatsakis opened this issue Apr 29, 2020 · 105 comments
Open
3 of 5 tasks

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

nikomatsakis opened this issue Apr 29, 2020 · 105 comments
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-unsafe-block-in-unsafe-fn RFC #2585 finished-final-comment-period The final comment period is finished for this PR / Issue. S-tracking-impl-incomplete Status: The implementation is incomplete. T-lang Relevant to the language team, which will review and decide on the PR/issue.

Comments

@nikomatsakis
Copy link
Contributor

nikomatsakis commented Apr 29, 2020

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

@nikomatsakis nikomatsakis added T-lang Relevant to the language team, which will review and decide on the PR/issue. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. F-unsafe-block-in-unsafe-fn RFC #2585 labels Apr 29, 2020
@nikomatsakis nikomatsakis changed the title Tracking Issue for XXX Tracking Issue for "unsafe blocks in unsafe fn" (RFC #2585) Apr 29, 2020
@nikomatsakis
Copy link
Contributor Author

cc @RalfJung

@jonas-schievink jonas-schievink added the B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. label Apr 29, 2020
@nikomatsakis
Copy link
Contributor Author

A relevant note towards the implementation:

@LeSeulArtichaut
Copy link
Contributor

I'll try to work on the implementation!

@RalfJung
Copy link
Member

RalfJung commented May 27, 2020

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
Copy link
Contributor

@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
Copy link
Member

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
Copy link
Contributor

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.

RalfJung added a commit to RalfJung/rust that referenced this issue May 29, 2020
…afe-fn, r=nikomatsakis

Implement RFC 2585: unsafe blocks in unsafe fn

Tracking issue: rust-lang#71668
r? @RalfJung cc @nikomatsakis
RalfJung added a commit to RalfJung/rust that referenced this issue May 29, 2020
…afe-fn, r=nikomatsakis

Implement RFC 2585: unsafe blocks in unsafe fn

Tracking issue: rust-lang#71668
r? @RalfJung cc @nikomatsakis
RalfJung added a commit to RalfJung/rust that referenced this issue May 29, 2020
…afe-fn, r=nikomatsakis

Implement RFC 2585: unsafe blocks in unsafe fn

Tracking issue: rust-lang#71668
r? @RalfJung cc @nikomatsakis
RalfJung added a commit to RalfJung/rust that referenced this issue May 29, 2020
…afe-fn, r=nikomatsakis

Implement RFC 2585: unsafe blocks in unsafe fn

Tracking issue: rust-lang#71668
r? @RalfJung cc @nikomatsakis
RalfJung added a commit to RalfJung/rust that referenced this issue May 29, 2020
…afe-fn, r=nikomatsakis

Implement RFC 2585: unsafe blocks in unsafe fn

Tracking issue: rust-lang#71668
r? @RalfJung cc @nikomatsakis
@LeSeulArtichaut
Copy link
Contributor

Step 1 can now be checked since #71862 landed.

@LeSeulArtichaut LeSeulArtichaut removed their assignment May 30, 2020
@withoutboats
Copy link
Contributor

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
Copy link
Contributor

phil-opp commented Jun 5, 2020

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
Copy link
Member

RalfJung commented Jun 5, 2020

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
Copy link
Contributor

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
Copy link
Contributor

phil-opp commented Jun 5, 2020

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
Copy link

awnion commented Sep 1, 2023

(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
Copy link
Member

RalfJung commented Sep 1, 2023

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
Copy link

awnion commented Sep 1, 2023

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
Copy link
Contributor

digama0 commented Sep 1, 2023

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
Copy link

awnion commented Sep 1, 2023

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.

bors added a commit to rust-lang-ci/rust that referenced this issue Sep 14, 2023
…safe_fn, r=RalfJung

Change `unsafe_op_in_unsafe_fn` to be `warn`-by-default from edition 2024

This was previously FCPed: rust-lang#71668 (comment)

There were two blocking requirements:
* Fix the `unused_unsafe` lint, done in rust-lang#100081
* Have `cargo fix` able to fix the lint, done in rust-lang#112017
github-actions bot pushed a commit to rust-lang/miri that referenced this issue Sep 15, 2023
…r=RalfJung

Change `unsafe_op_in_unsafe_fn` to be `warn`-by-default from edition 2024

This was previously FCPed: rust-lang/rust#71668 (comment)

There were two blocking requirements:
* Fix the `unused_unsafe` lint, done in rust-lang/rust#100081
* Have `cargo fix` able to fix the lint, done in rust-lang/rust#112017
@pigeonhands
Copy link

pigeonhands commented Nov 6, 2023

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
Copy link
Contributor

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

@traviscross
Copy link
Contributor

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

@traviscross
Copy link
Contributor

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

@rustbot rustbot added the I-lang-nominated Nominated for discussion during a lang team meeting. label Jan 28, 2024
@traviscross
Copy link
Contributor

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

@rustbot rustbot removed the I-lang-nominated Nominated for discussion during a lang team meeting. label Jan 31, 2024
@tmandry
Copy link
Member

tmandry commented Jan 31, 2024

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

lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
Add MVP suggestion for `unsafe_op_in_unsafe_fn`

Rebase of rust-lang/rust#99827

cc tracking issue rust-lang/rust#71668

No real changes since the original PR, just migrated the new suggestion to use fluent messages and added a couple more testcases, AFAICT from the discussion there were no outstanding changes requested.
lnicola pushed a commit to lnicola/rust-analyzer that referenced this issue Apr 7, 2024
…r=RalfJung

Change `unsafe_op_in_unsafe_fn` to be `warn`-by-default from edition 2024

This was previously FCPed: rust-lang/rust#71668 (comment)

There were two blocking requirements:
* Fix the `unused_unsafe` lint, done in rust-lang/rust#100081
* Have `cargo fix` able to fix the lint, done in rust-lang/rust#112017
@traviscross traviscross added A-edition-2024 Area: The 2024 edition and removed A-edition-2024 Area: The 2024 edition labels Apr 10, 2024
@traviscross
Copy link
Contributor

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

RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
Add MVP suggestion for `unsafe_op_in_unsafe_fn`

Rebase of rust-lang/rust#99827

cc tracking issue rust-lang/rust#71668

No real changes since the original PR, just migrated the new suggestion to use fluent messages and added a couple more testcases, AFAICT from the discussion there were no outstanding changes requested.
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this issue Apr 27, 2024
…r=RalfJung

Change `unsafe_op_in_unsafe_fn` to be `warn`-by-default from edition 2024

This was previously FCPed: rust-lang/rust#71668 (comment)

There were two blocking requirements:
* Fix the `unused_unsafe` lint, done in rust-lang/rust#100081
* Have `cargo fix` able to fix the lint, done in rust-lang/rust#112017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-RFC-approved Blocker: Approved by a merged RFC but not yet implemented. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. F-unsafe-block-in-unsafe-fn RFC #2585 finished-final-comment-period The final comment period is finished for this PR / Issue. S-tracking-impl-incomplete Status: The implementation is incomplete. T-lang Relevant to the language team, which will review and decide on the PR/issue.
Projects
Status: Needs help: Impl
Development

No branches or pull requests