Unnecessary `unsafe` block…

[steffahn]

Hey, I’d be interested in feedback what the best approach here ought to be. Consider some code, e.g.

fn safe() {
    safe_operation1();
    safe_operation2();

    // SAFETY: Lorem ipsum dolor sit amet, consectetur adipiscing elit,
    // sed do eiusmod tempor incididunt ut labore et dolore magna aliqua.
    let foo = unsafe {
        let x = safe_preparation_operation(MAGIC_SAFEWORD);
        x.prepare_even_more_safely();
        let result1 = unsafe {
            x.this_is_where_the_unsafe_magic_happens()
        };
        gimme_that_result(result1);

        let final_result = unsafe {
            x.this_is_where_the_unsafe_magic_happens_part_two()
        };

        final_result
    };

    /* and now, let's completely ignore and */ drop(foo) /*, yay!! */;

    safe_operation3();
}

every operation is safe except for the two methods “…where_the_unsafe_magic_happens…”. Now obviously there’s more unsafe blocks here than necessary. But it’s not clear what the best approach is for improving the code. What would you change? What kind of warning/explanation would you expect/want the compiler to give you?

---

Some thoughts…

Explaning the redundancy

So there’s two possible ways of explaining here how/why one or two of the unsafe {} blocks are redundant.

1. The inner unsafe blocks are redundant: There already is a containing unsafe block, so you don’t need to add another one for calling any unsafe method. Remove both inner unsafe blocks (only the blocks, not their content) and you’re good!
2. The outer unsafe block is redundant: The outer unsafe block is unused i.e. it doesn’t (directly) contain any actual unsafe code. If the outer block is removed (e.g. turned into an ordinary block) then the code will be fine!

A question of style?

Now, which solution should be taken? It’s not clear cut, here’s a few pros and cons…

Keeping the outer unsafe blocks would group together unsafe operations with some safe operations for setup that might be crucial for the soundness of the overall endeavor. In this example, there’s also a SAFETY comment explaining why the whole thing is sound. Would it be good style to keep a SAFETY comment on a larger safe block that merely contains multiple unsafe operations in their own smaller unsafe blocks? Or do SAFETY comments have to go directly in front of each and every unsafe block?

Would the code perhaps be easier to understand if the inner blocks are kept, acting as a clear indication where exactly within the lengthy outer block the actually unsafe operations are that need some safety conditions to be fulfilled in order to be sound to call?

Keeping the inner unsafe blocks also means less syntactical change to the code to resolve any warnings.

Keeping the outer unsafe block is what you’re going to do if you’re following the warning that rustc currently gives. (I don’t know how strong of an argument the status quo is for anything.)

Warnings, suggestions, etc…

Now let’s consider some possible ways that the compile could respond to this code. Let’s start with the status quo:

warning: unnecessary `unsafe` block
  --> src/lib.rs:10:23
   |
7  |     let foo = unsafe {
   |               ------ because it's nested under this `unsafe` block
...
10 |         let result1 = unsafe {
   |                       ^^^^^^ unnecessary `unsafe` block
   |
   = note: `#[warn(unused_unsafe)]` on by default

warning: unnecessary `unsafe` block
  --> src/lib.rs:15:28
   |
7  |     let foo = unsafe {
   |               ------ because it's nested under this `unsafe` block
...
15 |         let final_result = unsafe {
   |                            ^^^^^^ unnecessary `unsafe` block

And this is how the warning could look like instead, if the compiler only complained about the outer unsafe block being unused:

warning: unnecessary `unsafe` block
 --> src/lib.rs:7:15
  |
7 |     let foo = unsafe {
  |               ^^^^^^ unnecessary `unsafe` block
  |
  = note: `#[warn(unused_unsafe)]` on by default

Then there’s most likely some middle-ground, too. Let’s first point out some properties of these different warnings:

• the first one is longer / there are more warnings
• the second one does not point out all the unsafe blocks involved

Then some logical alternatives / variations include

• the second version could be used but it could get a note pointing out some contained unsafe blocks, e.g. “this unsafe blocks does contain unsafe operations but those operations are located within nested unsafe blocks” (and then give an example of one of the nested unsafe blocks). Pointing out one example is enough: If the user removed one of the nested unsafe block, the outer one becomes used, and further warnings will point out the remaining now redundant inner unsafe blocks.
• the first version could get a note that the containing unsafe block is actually unused and that resolving the warning in this case could involve either removing the unnecessary unsafe block or the containing one.

A word about macros

There’s one particular and not uncommon usage of unsafe blocks that makes, IMO, warning about the outer unsafe block a better choice: If a macro contains some usage of unsafe, and the inner unsafe block in a setting as above comes from such a macro, then that inner unsafe block might not report any warnings at all, either because the macro author (rightfully) placed an #[allow(unused_unsafe)] around it, or because the unused_unsafe warning doesn’t get reported at all for external macros. In order to have code like

unsafe {
    some_safe_macro!("…");
    more_safe_code();
}

not end up producing no warning at all when some_safe_macro uses unsafe {} internally in its expansion (think e.g. the futures::pin_mut macro), there would need to be special-case logic detecting external macros (or maybe even macros in general) that changes the behavior accordingly in order to, ultimately, do report the outer unsafe block as being unused in this case. OTOH, if the outer unsafe block is always the one that’s reported anyways, then no special handling of macros is required.

---

Finding the right warning behavior here is probably a (minor) language design decision about whether more coarse or more fine-grained unsafe blocks are preferred, and to a large degree a compiler diagnostics design question on what the best approach should look like in detail. Ultimately, I can’t decide alone what approach should be taken, though I have a slight preference (in case you couldn’t tell that already). I’d to get as much feedback / as many opinions as possible here in order to get an indicator of what might be preferred by the community, also feel free to point out any aspects in this discussion that I’ve overlooked. This general question came up during review of #93678.

[H2CO3]

@steffahn

Now, which solution should be taken? It’s not clear cut

I find it pretty clear, for one: unsafe scopes should generally be minimized, because including currently-safe operations in an unsafe block can lead to a situation where you change some of them to unsafe, and the compiler won't yell at you to consider whether safety invariants are still being upheld.

So, to me it's obvious that the outer block is redundant.

[ckaran]

I tend to think that @H2CO3 is right on this, that the unsafe blocks should be as minimal as possible, BUT you need to use the outer unsafe block is if somehow the unsafety could 'leak out' of the inner blocks in some manner. And I don't mean in terms of Rust unsafety, I'm thinking in terms of other guarantees that the programmer has somehow provided that are actually logical errors, but which the programmer chooses to mark as unsafe as a warning to others. This last part is summarized from the Nomicon:

You can use `unsafe` to indicate the existence of unchecked contracts on *functions* and *trait declarations* . On functions, `unsafe` means that users of the function must check that function's documentation to ensure they are using it in a way that maintains the contracts the function requires. On trait declarations, `unsafe` means that implementors of the trait must check the trait documentation to ensure their implementation maintains the contracts the trait requires.

[CAD97]

There is a strong potential counterargument to the simple "unsafe scopes should generally be minimized."

Specifically, I try to use unsafe blocks such that they include the scope of the temporarily broken safety variant. When the unsafe block ends, ideally we're back in a safe state where all values are in a safe state again. This is particularly important w.r.t. unwind safety: an unsafe block clues me in to make sure an unwind doesn't cause unsoundness.

As a simple example, you could write let s = unsafe { string.as_bytes_mut() };, but the unsafely is actually theoretically scoped to the existence of the &mut [u8], not just the acquisition of the reference. This isn't the best example, because no safety invariant is actually broken until the reference is written to, but it illustrates the point.

So my rough heuristic for how big an unsafe block should be is "would an unwind be “interesting” due to suspended invariants." Unfortunately, this isn't really one that the compiler can know to decide which block to eliminate.

@H2CO3

including currently-safe operations in an unsafe block can lead to a situation where you change some of them to unsafe

Generally, I expect this to be rare, personally. Specifically, I'd expect that any previously-safe operation would not gain any safety requirements that aren't able to be checked for, because they're effectively currently being checked. As such, I'd personally expect such an operation to gain a new _unchecked variant, and the safe variant to check the requirements up front then delegate to the unchecked implementation.

That's not to say it doesn't happen. But it feels like the safety is a fundamental part of defining what an operation is, that changing the safety changes the operation, and thus should also change the name.

[chrefr]

I think both arguments are valid, and it makes me wonder: perhaps there should be two kinds of unsafe blocks (probably one of them won't be called unsafe), one to mark the unsafety and the other to mark the soundness, that is, why the whole thing is sound, just like we have unsafe_op_in_unsafe_fn because the unsafe modifier on functions used to serve two purposes?

[robinm]

There is a 3rd possibility. If an unsafe bloc contains inner unsafe blocs, it is not possible to call unsafe function in the outer unsafe bloc without putting them in their own unsafe inner bloc. This would also apply to unsafe function (so using a single inner unsafe bloc would require to use inner unsafe blocs for all other unsafe operations). An inner unsafe bloc means “there is a call to an unsafe operation”, while the outer unsafe means “invariant may be brocken in this bloc, and is therefore not unwind safe”. In this mode both outer and inner unsafe bloc are useful.

Such change (making all inner unsafe blocs required) is obviously breaking, and could not be done without an edition, but it's future proof because the current behavior just more permissif (missing inner unsafe blocs would just yield a warning).

In order to make the inner unsafe blocs as small as possible, a postfix .unsafe keyword could be introduced.

[toc]

There have been proposals for scoped unsafe blocks. I agree it's probably unlikely we accidentally introduce extra unsafety inside the larger block, but the whole point of this is being able to communicate as many constraints about the code to the programmer and the compiler. So perhaps a 4th option would be to have it look like:

fn safe() {
    safe_operation1();
    safe_operation2();

    // SAFETY: This is safe because we're going to uphold all the invariants.
    let foo = unsafe(nothing_extra_allowed) {
        let x = safe_preparation_operation(MAGIC_SAFEWORD);
        x.prepare_even_more_safely();
        // SAFETY: This is safe because x has been prepared so safely.
        let result1 = unsafe { // ← required; allows any unsafe operation
            x.this_is_where_the_unsafe_magic_happens()
        };
        gimme_that_result(result1);

        // SAFETY: Our ffi is all good.
        let final_result = unsafe(ffi) { // ← also required
            x.this_is_where_the_unsafe_ffi_happens()
        };

        final_result
    };

    /* and now, let's completely ignore and */ drop(foo) /*, yay!! */;

    safe_operation3();
}

[yigal100]

The core issue imo is that unsafe is a scalar, it lacks a direction:

• "downwards" - contract between "me" (the person writing the code) and the compiler
• "upwards" - contract between "me" and other programmers who'll read/maintain the code (this includes future me)

This break of symmetry makes it unintuitive to reason about. Incidentally, this is the same conceptual issue as the extern in C/C++ and mod in Rust.

It would have been better to have a directional symmetric and local nomenclature for all of the above. I'm unsure though if this can be easily solved in a backwards compatible way without essentially forking the language.

An example for this kind of symmetry would be C++20 modules: a module marks inline which symbols it exports and other modules can then import those. There are other fuck ups in the design (it is C++ after all) but at least this part should get imo a top score on its intuitive HCI design.

[scottmcm]

@yigal100

The core issue imo is that unsafe [...] lacks a direction

Agreed. We've talked about changing the block form to trusted { ... } or something, but of course the churn there is huge.

[H2CO3]

@CAD97

Specifically, I'd expect that any previously-safe operation would not gain any safety requirements that aren't able to be checked for, because they're effectively currently being checked.

I don't find that the slightest bit convincing tbh. Such modifications need not be small! I remember e.g. that rewriting BTreeMap in such a way that an empty map doesn't allocate was a multi-person, hard, long task, because of the amount and complicated usage of unsafe. It may well happen that a safe function is rewritten so that it's now a part of a larger scheme and needs to be verified separately.

@CAD97

and thus should also change the name.

Okay, but that doesn't help and it's not particularly relevant to the argument, either. If you merely change the name of a function, it's trivial to fix compilation errors with a search-and-replace, while still completely evading having to think about the changing safety requirements.

[H2CO3]

@CAD97

There is a strong potential counterargument to the simple " unsafe scopes should generally be minimized."

Specifically, I try to use unsafe blocks such that they include the scope of the temporarily broken safety variant. When the unsafe block ends, ideally we're back in a safe state where all values are in a safe state again.

And to react to this: it's not a counterargument against minimizing unsafe blocks, it's a counterargument against writing long passages of unsafe. Even though I completely agree with the principle of marking every element of an unsafe sequence of operations as unsafe, this doesn't mean that one should not strive to minimize unsafety, whether inside or outside an unsafe block.

To my taste, the code presented in the OP is already too long and too complicated to be shoved into a single giant unsafe block. It wouldn't pass my code review unless it was refactored so that it brought unsafe parts closer together in a structured way (e.g. by introducing an abstraction boundary such as a new type) and allowed the non-unsafe parts to be treated as unconditionally safe.

Thus, it seems to me overall that it is not the mere act of syntactically minimizing or maximizing unsafe blocks that matters, but the appropriate manner and amount of abstraction to be utilized, which in turn, when done properly, tends to minimize syntactic unsafety markers as well.

[afetisov]

Personally I agree with @CAD97 : unsafe blocks should demarcate areas of potentially violated invariants, and they should, ideally, be the smallest such areas. So whether the outer or inner unsafe block is redundant strongly depends on the context which is beyond the capabilities of the compiler.

With regards to the lint, I wouldn't make a choice at the compiler's side because it just doesn't have the information, definitionally. Perhaps neither the outer no inner unsafe block is a correct choice, and some other blocks should be introduced? Thus I would expect the lint to simply say "some of the following unsafe blocks are redundant" and list the pairs of shadowing blocks. Personally I would like the compiler to provide a link to some guidelines about the usage of unsafe blocks, and let the programmer decide.

The minimal possible unsafe blocks enclosing just the exact unsafe expressions have negative value in my eyes. I already have all unsafe calls highlighted by the IDE, I don't need that information duplicated, verbosely. What I do need is semantic information: what are the invariants? What is the safety reasoning?

It is best practice to annotate each unsafe block with a comment about its safety. That is the guideline about proper unsafe scope that I believe in. Can you provide a meaningful reasoning while the specific unsafe block is safe, or do you need to refer to the properties of the enclosing safe code? If the only thing you can write in a safety comment is "see the reasoning in the block above", then both the comment and the block are redundant.

In other words, each unsafe block should be extractable into an unsafe function. If a piece of code doesn't make any sense as a function, probably it doesn't make sense as an unsafe block either.

An extreme case of the above is access to fields which can violate safety, like Vec::set_len. We don't have unsafe fields, so there is technically no unsafe code inside, but the function is still unsafe. Now the same field access could be inlined into some other function. By the reasoning of @H2CO3 it shouldn't be within an unsafe block since there are no unsafe calls inside, but it must uphold complex invariants.

An FFI call can likely be enclosed in a minimal unsafe block since there is seldom anything else to state aside from "I believe that library should be used this way". Something like a method on a Rust-only container may very well have its entirety wrapped in unsafe, if it's doing something very tricky.

[afetisov]

With regards to macros, in an ideal world they would have unsafety hygiene, and would provide specific guarantees about their safety and the safety of their parameters. A user of the macro shouldn't know or care about its inner workings, and whether it uses unsafe blocks inside. This means that if I want to pass a result of an unsafe call into a macro, ideally I should be required to pass it as foo!(unsafe { bar() }) regardless of whether that specific expression is inlined into an unsafe block. On the other hand, some other macro may be able to accept raw unsafe calls, because the macro itself is explicitly unsafe and puts some extra requirements on its parameters.

That is what an ideal world would look like. In the real world we, unfortunately, have no notion of "unsafe macro" or "unsafe macro arguments", and won't have it in the foreseeable future. This means that it is up to the macro authors to write their macros in a way which wouldn't allow unsafety laundering. But in the meantime, I wouldn't want the compiler to get in the way an emit warnings about redundant unsafe which are true only due to accidental implementation details..

[H2CO3]

@afetisov

I already have all unsafe calls highlighted by the IDE

Not everyone uses an IDE all the time. I'm not willing to install/open an IDE to view a diff online for code review, for example. Please don't use "but the IDE does it anyway" as a serious argument, it doesn't work most of the time.

@afetisov

Now the same field access could be inlined into some other function. By the reasoning of @H2CO3 it shouldn't be within an unsafe block since there are no unsafe calls inside, but it must uphold complex invariants.

No, this is grossly misrepresenting my point.

First of all, it doesn't even fall into the category I'm arguing against. Vec::set_len() does not contain an unsafe block, it is not using the unsafety, it is providing the unsafety as part of its interface.

Second, the words you are putting in my mouth imply that I recommend writing unsound code. That is not the case. I am explicitly not suggesting that one should be able to cause UB or unsoundness merely by writing apparently "safe" code that breaks the invariants of unsafe code!

What I am suggesting is that abstraction boundaries should be drawn in such a way that preferably most code that doesn't technically require an unsafe block is indeed sound, and can't cause UB by itself, ie. it can never violate assumptions of unsafe code. This means that the number of times one has to stop and think "the compiler allows me to write this, but is it actually correct?" is minimized. I can hardly see how that would count as a disadvantage.

[afetisov]

@H2CO3

the words you are putting in my mouth imply that I recommend writing unsound code

I did not imply it. I was talking about inner safety boundaries, which do not actually affect the soundness or safety of your API. For example, here is the code of Vec::drain (comments elided for brevity):

pub fn drain<R>(&mut self, range: R) -> Drain<'_, T, A>
    where
        R: RangeBounds<usize>,
{
    let len = self.len();
    let Range { start, end } = slice::range(range, ..len);

    unsafe {
        self.set_len(start);
        let range_slice = slice::from_raw_parts_mut(self.as_mut_ptr().add(start), end - start);
        Drain {
            tail_start: end,
            tail_len: len - end,
            iter: range_slice.iter(),
            vec: NonNull::from(self),
        }
    }
}

Here Vec::set_len is an unsafe function call, but let's say we didn't extract it as a separate function and instead access the len field directly. If I understand your argument correctly, then you would write the function like this:

pub fn drain<R>(&mut self, range: R) -> Drain<'_, T, A>
    where
        R: RangeBounds<usize>,
{
    let len = self.len();
    let Range { start, end } = slice::range(range, ..len);

    self.len = start;
    let range_slice = unsafe { slice::from_raw_parts_mut(self.as_mut_ptr().add(start), end - start) };
    Drain {
        tail_start: end,
        tail_len: len - end,
        iter: range_slice.iter(),
        vec: NonNull::from(self),
    }
}

The soundness of the function is unchanged. Indeed, the function is almost literally the same. However, now the potentially dangerous self.len assignment is outside of the unsafe block, unmarked and can be missed more easily, even though it is critical for the soundness in the presence of leaks.

[afetisov]

@H2CO3

I'm not willing to install/open an IDE to view a diff online for code review, for example. Please don't use "but the IDE does it anyway" as a serious argument, it doesn't work most of the time.

I understand the sentiment, but I don't think all information should be dumped into the syntax of the language, even when it is readily available from the tools. There is plenty of information which would be missing from the diff anyway: inferred types, elided lifetimes, macro expansions, trait resolution. In that list unsafe calls are pretty much the least of your concerns. I don't believe that non-trivial PRs in Rust can be thoroughly reviewed without using proper tools anyway, and any changes to unsafe code certainly require a thorough analysis.

Besides, the world is converging to at least IDE-lite functionality available everywhere. Gitlab and Github already have IDE-like functionality in their code editors, and it's just a matter of time until they start highlighting unsafe calls in Rust.

[H2CO3]

@afetisov

I understand the sentiment, but I don't think all information should be dumped into the syntax of the language, even when it is readily available from the tools. There is plenty of information which would be missing from the diff anyway: inferred types, elided lifetimes

There's a crucial difference between unsafe and inferred types/lifetimes: when a type can't be inferred (e.g. because it's ambiguous), or when some inferred lifetimes would result in memory unsafety, the compiler complains, and the program won't even get to the point of compiling, let alone executing incorrect code. And even with this significantly smaller burden/lower level of responsibility, Rust still chooses – rightly – to require type annotations even in contexts where they could technically be inferred (e.g. function declarations), because they improve code readability.

I'm not sure what you are getting at with "I don't need that information duplicated, verbosely"; if you are proposing that unsafe blocks should not be required but they should be inferred purely because they can, then I strongly disagree with that, and so does the language design itself, pretty much. Basically, unsafe was designed to be redundant: the compiler knows exactly when you have to use it, and as far as I can tell, it could very well insert unsafe blocks where needed (or just not require them). But the very point of unsafe blocks is to explicitly mark when you are using a memory-unsafe construct. Humans are not mechanistic compilers with gigabytes of memory and low-level global type inference algorithms. Seeing easily and immediately where things can go wrong is extremely helpful.

@afetisov

In that list unsafe calls are pretty much the least of your concerns.

I don't understand how that could possibly be the case. Unsafe operations are the ones that need the most scrutiny during code review, not the least.

[steffahn]

If one was to follow the approach to always “include the scope of the temporarily broken safety variant” in the unsafe blocks and aim to ensure that “When the unsafe block ends, ideally we're back in a safe state where all values are in a safe state again” (both quotes from @CAD97 above), since for an unsafe fn, commonly “getting back into a safe state” is up to the caller afterwards, we should arguably always “stay withing the unsafe block” throughout the definition of such an unsafe fn, right? And the other case of an unsafe fn that does leave us in a safe state will instead always have some soundness-critical unchecked precondition that will need to be fullfilled, so AFAICT similar logic would apply. But then… why does rustc support #[deny(unsafe_op_in_unsafe_fn)]? What kind of unsafe fn would not want to have the whole function body be part of a single big unsafe block? (Because making sure that not the whole body is a single big unsafe block is ultimately what changing the unsafe_op_in_unsafe_fn lint level is all about.)

[steffahn]

This is an interesting idea IMO. There’s certainly value in marking blocks as “the code in here in particular is involved in ensuring some safety-pre/post-conditions for unsafe code, pay attention when modifying it”. This kind of annotation can then apply to potentially quite large blocks of code, without obfuscating the detail of where exactly the unsafe operation actually happened.

This kind of block might as well just be an ordinary block with a SAFETY comment though, IDK. Following your proposal of two levels of unsafe, I thought back to this idea of mine:

Unsafe categories

I personally feel like it would be nicer to just add the ability to use the unsafe keyword in a smaller scope than a whole block, namely, on each individual unsafe operation. In terms of syntax, a possibility for each unsafe feature, i.e. each of The following language level features cannot be used in the safe subset of Rust: Dereferencing a raw pointer. Reading or writing a mutable or external static variable. Accessing a field of a union , other than to assign to it. Calling an unsafe fu…

and I think it might combine quite well. If we have

• a feature for unsafe keyword directly on unsafe operations themself e.g. as presented in the linked post above
• a lint that disallows unsafe operations directly within an unsafe block without using another instance of the unsafe keyword on the operation itself
• perhaps also a lint that makes it so that unsafe blocks are still required

this would enforce a coding style similar to the example code you gave above, but the syntax would look like this instead:

fn safe() {
    safe_operation1();
    safe_operation2();

    // SAFETY: This is safe because we're going to uphold all the invariants.
    let foo = unsafe { // ← still doesn’t directly allow unsafe operations
        let x = safe_preparation_operation(MAGIC_SAFEWORD);
        x.prepare_even_more_safely();
        // SAFETY: This is safe because x has been prepared so safely.
        let result1 =  x.unsafe this_is_where_the_unsafe_magic_happens();
        //                 ↖ still required despite the outer `unsafe` block
        gimme_that_result(result1);

        // SAFETY: Our ffi is all good.
        let final_result = x.unsafe this_is_where_the_unsafe_ffi_happens();
        // (admitted, you only know that this is about FFI, when looking at the docs
        // of `this_is_where_the_unsafe_ffi_happens`, but at least it’s very
        // clear where exactly you have to look)

        final_result
    };

    /* and now, let's completely ignore and */ drop(foo) /*, yay!! */;

    safe_operation3();
}

If instead of using unsafe { … }, marking the block here is done with comments alone, it wouldn’t be too bad either IMO. You’d just need a new convention, e.g.

fn safe() {
    safe_operation1();
    safe_operation2();

    // SAFETY-CRITICAL: this block ensures the pre-conditions
    // of two unsafe operations, be careful when modifying it
    let foo = {
        let x = safe_preparation_operation(MAGIC_SAFEWORD);
        x.prepare_even_more_safely();
        // SAFETY: This is safe because x has been prepared so safely.
        let result1 =  x.unsafe this_is_where_the_unsafe_magic_happens();
        gimme_that_result(result1);

        // SAFETY: Our ffi is all good.
        let final_result = x.unsafe this_is_where_the_unsafe_ffi_happens();
        // (admitted, you only know that this is about FFI, when looking at the docs
        // of `this_is_where_the_unsafe_ffi_happens`, but at least it’s very
        // clear where exactly you have to look)

        final_result
    };

    /* and now, let's completely ignore and */ drop(foo) /*, yay!! */;

    safe_operation3();
}

Using some kind of conventional marker in comments like “SAFETY-CRITICAL”, you can mark blocks, or perhaps entire function bodies (probably commonly relevant for unsafe fn implementations) or even entire modules (in particular when we don’t have unsafe fields yet) with this marker and add a note what one must look out for when modifying any code within the relevant block, function, or module.

In particular such a SAFETY-CRITICAL comment would also imply that in the example above, anything that happens outside of the block is not relevant for ensuring the soundness of the unsafe method calls. If e.g. only ensuring the safety conditions of one of multiple unsafe calls within such a block was completely handled within that block, then a SAFETY-CRITICAL would explain which of the operations that is, e.g.

/// # Safety
/// You must ensure XYZ when calling this function
unsafe fn foo() {
    // SAFETY-CRITICAL: in this function body qux is safe to call because
    // of the safety conditions of `foo` above.

    bar();
    // SAFETY-CRITICAL: baz() and f() below together are relevant for g() to be safe to call
    {
        baz();
        // SAFETY: [explain why exactly qux() is safe to call because of XYZ]
        unsafe qux();
        f();
        // SAFETY: [explain why exactly g() is safe to call because the previous calls to baz() and f()]
        unsafe g();
    }
}

so the comments above would indicate that

• care must be taken when changing anything in the definition of foo() to make sure that qux() stays safe to call
• additionally, care must be taken when changing anything inside of the block to make sure that g() stays safe to call

[CAD97]

@steffahn

since for an unsafe fn , commonly “getting back into a safe state” is up to the caller afterwards, we should arguably always “stay withing the unsafe block” throughout the definition of such an unsafe fn, right?

There's two kinds of unsafe fn w.r.t. safety invariants.

• unsafe because it breaks a safety invariant that the caller is responsible for restoring
• unsafe because the caller has to guarantee that it doesn't break invariants

Or more succinctly, whether usage restores the safety invariants or not.

Of the code I personally end up writing, unchecked preconditions far outweigh the "put it in a fragile state" cases. Even Vec::set_len can primarily be used as an unchecked precondition, where you make sure the items are all valid and initialized before calling set_len. I posit that the end goal of the PPYP pattern is that you're never actually in a state where the safety invariants are broken.

Of course, many functions are both, depending on context. Vec::set_len is of course the canonical example here, along with any other safety critical field access. It can either be pre-checked, leaving you in a safe state, or you can set the length over what's currently initialized and then do fixup operations.

Inside of an unsafe fn, I still try to scope unsafe blocks to suspensions of safety variants. It's just that a) we assume the precondition(s) to be true, and b) any unsafe postcondition intentionally violated safety conditions are treated as immaterial, and don't have to be restored to exit an unsafe block.

It's worth noting two more things to what this looks like in practice, though:

• Most unsafe blocks are minimal and contain a single unsafe operation anyway, because they're precondition-unsafe rather than postcondition-unsafe and I've worked to control unsafety; and
• In heavy pointer manipulation code where I'm basically just writing Rust-flavored C code, I typically end up with a monolithic unsafe block anyway, because it's all unsafe. This is kind of the exception that proves the rule here, and that it's always up to discretion, and there's no be-all-end-all rule here.