# Unnecessary \`unsafe\` block… **URL:** \ **Category:** language design\ **Created:** [February 15, 2022, 12:52pm UTC](https://internals.rust-lang.org/t/unnecessary-unsafe-block/16144 "2022-02-15T12:52:57Z")\ **Posts on this page:** 20\ **Page:** 1
**Author:** ![steffahn](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/steffahn/32/13288_2.png) [@steffahn](https://internals.rust-lang.org/u/steffahn)\ **Post date:** [February 15, 2022, 12:52pm UTC](https://internals.rust-lang.org/t/unnecessary-unsafe-block/16144/1 "2022-02-15T12:52:57Z")
Hey, I’d be interested in feedback what the best approach here ought to be. Consider some code, e.g. ```rust 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: ```plaintext 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: ```plaintext 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 ```rust 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](https://github.com/rust-lang/rust/pull/93678). ---
**Author:** ![H2CO3](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/h2co3/32/2849_2.png) [@H2CO3](https://internals.rust-lang.org/u/H2CO3)\ **Post date:** [February 15, 2022, 1:54pm UTC](https://internals.rust-lang.org/t/unnecessary-unsafe-block/16144/2 "2022-02-15T13:54:01Z")
> [@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. ---
**Author:** ![ckaran](https://avatars.discourse-cdn.com/v4/letter/c/f475e1/32.png) [@ckaran](https://internals.rust-lang.org/u/ckaran)\ **Post date:** [February 15, 2022, 2:22pm UTC](https://internals.rust-lang.org/t/unnecessary-unsafe-block/16144/3 "2022-02-15T14:22:27Z")
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](https://doc.rust-lang.org/nomicon/safe-unsafe-meaning.html): ```markdown 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. ``` ---
**Author:** ![CAD97](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/cad97/32/3460_2.png) [@CAD97](https://internals.rust-lang.org/u/CAD97)\ **Post date:** [February 15, 2022, 4:01pm UTC](https://internals.rust-lang.org/t/unnecessary-unsafe-block/16144/4 "2022-02-15T16:01:27Z")
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. ---
**Author:** ![chrefr](https://avatars.discourse-cdn.com/v4/letter/c/e480ec/32.png) [@chrefr](https://internals.rust-lang.org/u/chrefr)\ **Post date:** [February 15, 2022, 9:48pm UTC](https://internals.rust-lang.org/t/unnecessary-unsafe-block/16144/5 "2022-02-15T21:48:01Z")
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? ---
**Author:** ![robinm](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/robinm/32/6255_2.png) [@robinm](https://internals.rust-lang.org/u/robinm)\ **Post date:** [February 15, 2022, 11:28pm UTC](https://internals.rust-lang.org/t/unnecessary-unsafe-block/16144/6 "2022-02-15T23:28:06Z")
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. ---
**Author:** ![toc](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/toc/32/6692_2.png) [@toc](https://internals.rust-lang.org/u/toc)\ **Post date:** [February 16, 2022, 2:24am UTC](https://internals.rust-lang.org/t/unnecessary-unsafe-block/16144/7 "2022-02-16T02:24:58Z")
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: ```rust 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(); } ``` ---
**Author:** ![anon32976453](https://avatars.discourse-cdn.com/v4/letter/a/49beb7/32.png) [@anon32976453](https://internals.rust-lang.org/u/anon32976453)\ **Post date:** [February 16, 2022, 7:36am UTC](https://internals.rust-lang.org/t/unnecessary-unsafe-block/16144/8 "2022-02-16T07:36:02Z")
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 `export`s 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. ---
**Author:** ![scottmcm](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/scottmcm/32/2355_2.png) [@scottmcm](https://internals.rust-lang.org/u/scottmcm)\ **Post date:** [February 16, 2022, 7:59am UTC](https://internals.rust-lang.org/t/unnecessary-unsafe-block/16144/9 "2022-02-16T07:59:39Z")
> [@anon32976453](#): > > 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. ---
**Author:** ![H2CO3](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/h2co3/32/2849_2.png) [@H2CO3](https://internals.rust-lang.org/u/H2CO3)\ **Post date:** [February 16, 2022, 8:00am UTC](https://internals.rust-lang.org/t/unnecessary-unsafe-block/16144/10 "2022-02-16T08:00:18Z")
> [@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. ---
**Author:** ![H2CO3](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/h2co3/32/2849_2.png) [@H2CO3](https://internals.rust-lang.org/u/H2CO3)\ **Post date:** [February 16, 2022, 8:09am UTC](https://internals.rust-lang.org/t/unnecessary-unsafe-block/16144/11 "2022-02-16T08:09:39Z")
> [@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. ---
**Author:** ![afetisov](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/afetisov/32/8508_2.png) [@afetisov](https://internals.rust-lang.org/u/afetisov)\ **Post date:** [February 16, 2022, 1:02pm UTC](https://internals.rust-lang.org/t/unnecessary-unsafe-block/16144/12 "2022-02-16T13:02:58Z")
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. ---
**Author:** ![afetisov](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/afetisov/32/8508_2.png) [@afetisov](https://internals.rust-lang.org/u/afetisov)\ **Post date:** [February 16, 2022, 1:10pm UTC](https://internals.rust-lang.org/t/unnecessary-unsafe-block/16144/13 "2022-02-16T13:10:33Z")
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.. ---
**Author:** ![H2CO3](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/h2co3/32/2849_2.png) [@H2CO3](https://internals.rust-lang.org/u/H2CO3)\ **Post date:** [February 16, 2022, 2:23pm UTC](https://internals.rust-lang.org/t/unnecessary-unsafe-block/16144/14 "2022-02-16T14:23:27Z")
> [@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. ---
**Author:** ![afetisov](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/afetisov/32/8508_2.png) [@afetisov](https://internals.rust-lang.org/u/afetisov)\ **Post date:** [February 16, 2022, 2:49pm UTC](https://internals.rust-lang.org/t/unnecessary-unsafe-block/16144/15 "2022-02-16T14:49:13Z")
> [@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): ```rust pub fn drain(&mut self, range: R) -> Drain<'_, T, A> where R: RangeBounds, { 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: ```rust pub fn drain(&mut self, range: R) -> Drain<'_, T, A> where R: RangeBounds, { 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. ---
**Author:** ![afetisov](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/afetisov/32/8508_2.png) [@afetisov](https://internals.rust-lang.org/u/afetisov)\ **Post date:** [February 16, 2022, 3:04pm UTC](https://internals.rust-lang.org/t/unnecessary-unsafe-block/16144/16 "2022-02-16T15:04:59Z")
> [@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. ---
**Author:** ![H2CO3](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/h2co3/32/2849_2.png) [@H2CO3](https://internals.rust-lang.org/u/H2CO3)\ **Post date:** [February 16, 2022, 5:02pm UTC](https://internals.rust-lang.org/t/unnecessary-unsafe-block/16144/17 "2022-02-16T17:02:12Z")
> [@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. ---
**Author:** ![steffahn](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/steffahn/32/13288_2.png) [@steffahn](https://internals.rust-lang.org/u/steffahn)\ **Post date:** [February 16, 2022, 6:50pm UTC](https://internals.rust-lang.org/t/unnecessary-unsafe-block/16144/18 "2022-02-16T18:50:27Z")
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.) ---
**Author:** ![steffahn](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/steffahn/32/13288_2.png) [@steffahn](https://internals.rust-lang.org/u/steffahn)\ **Post date:** [February 16, 2022, 7:16pm UTC](https://internals.rust-lang.org/t/unnecessary-unsafe-block/16144/19 "2022-02-16T19:16:28Z")
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](https://internals.rust-lang.org/t/unsafe-categories/15334/2): > > 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](https://doc.rust-lang.org/stable/reference/types/pointer.html). Reading or writing a [mutable](https://doc.rust-lang.org/stable/reference/items/static-items.html#mutable-statics) or [external](https://doc.rust-lang.org/stable/reference/items/external-blocks.html) static variable. Accessing a field of a [union](https://doc.rust-lang.org/stable/reference/items/unions.html), 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: ```rust 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. ```rust 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. ```rust /// # 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 ---
**Author:** ![CAD97](https://sea2.discourse-cdn.com/flex002/user_avatar/internals.rust-lang.org/cad97/32/3460_2.png) [@CAD97](https://internals.rust-lang.org/u/CAD97)\ **Post date:** [February 16, 2022, 10:11pm UTC](https://internals.rust-lang.org/t/unnecessary-unsafe-block/16144/20 "2022-02-16T22:11:21Z")
> [@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. [Next page](https://internals.rust-lang.org/t/unnecessary-unsafe-block/16144.md?page=2)