Extending the unsafe keyword

language design
1

I had this in my head for quite some time and would like to hear some feedback to it. My proposal can easily be combined with this one: "Split keyword unsafe into unsafe / checked"

In the 2024 edition, it seems that a linter change will be included:

unsafe fn map_physical_region<T>(&self, physical_address: usize, size: usize) -> PhysicalMapping<Self, T> {
        PhysicalMapping::new(
            physical_address,
            self.as_ptr(physical_address),
            size,
            size,
            *self,
        )
    }

Now emits this warning:

warning: call to unsafe function `acpi_rs::PhysicalMapping::<H, T>::new` is unsafe and requires unsafe block (error E0133)
  --> rose-kernel/src/acpi/acpi_mapping_handler.rs:30:9
   |
30 | /         PhysicalMapping::new(
31 | |             physical_address,
32 | |             self.as_ptr(physical_address),
33 | |             size,
34 | |             size,
35 | |             *self,
36 | |         )
   | |_________^ call to unsafe function
   |
   = note: for more information, see issue #71668 <https://github.com/rust-lang/rust/issues/71668>
   = note: consult the function's documentation for information on how to avoid undefined behavior
note: an unsafe function restricts its caller, but its body is safe by default
  --> rose-kernel/src/acpi/acpi_mapping_handler.rs:29:5
   |
29 |     unsafe fn map_physical_region<T>(&self, physical_address: usize, size: usize) -> PhysicalMapping<Self, T> {
   |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   = note: `#[warn(unsafe_op_in_unsafe_fn)]` on by default

I personally like this change, since it prevents oversight of using unsafe functions, but it also has a downside: It introduces more nesting and lines.

Another thing thats is slightly annoying sometimes is this syntax:

let Some(x) = (unsafe { get_x() }) else {
    return;
}

The braces around the unsafe block are redundant to the reader, but necessary for it to compile, because else doesn't like being put after curly braces when not used as an if / else.

My proposal would be, to add another way to use unsafe:

let Some(x) = unsafe get_x() else {
    return;
}

In this case, unsafe is not a block, but a modifier, allowing the following expression or statement to be unsafe. Another example with a statement:

unsafe for page in list_pages_unsafe() {
    map_page_unsafe(page);
}

In this case, the entirety of the for statement, both the iterator expression and body would allow for unsafe things to happen.

The only purpose of this change is to avoid pointless nesting blocks for one liners, but especially in low level code, where unsafe is used frequently, this would be a huge improvement. Alternatively, one could just write a simple macro in the form of:

let Some(x) = do_unsafe!(get_x()) else {
    return;
}

But for such a simple thing, I believe this is a needless abstraction, and might lead to stupid human errors like:

  • Tom realized that the logger is always initialized when the do_unsafe macro is called, and puts some logging for reasons only he is smart enough to grasp.
  • This makes it into production because Rick reviewing it doesn't see an issue with it. It works after all.
  • A couple weeks later, an unsafe step is added, that runs parallel to the logger initialization. It's an edge case so it doesn't break most of the time.
  • Under the read world workload, the logger takes a lot longer to initialize. It now breaks in production but is rarely reproducible in testing.
  • You got yourself a 80 hour workweek in order to fix it.

Sure it's a dumb idea to add side effects to random stuff but in reality we all are stupid sometimes. So I'd like to avoid using a macro.

1 Like
2

Splitting the introduction keyword from the elimination keyword has come up many times. I think it'd be a good change -- I tend to call it hold_my_beer for bikeshed avoidance -- but it's hard to weigh the churn of it.

I've never been fond of this. To me, the critical thing is that unsafe doesn't just affect a value (like ? does, say) but that it affects everything in the lexical block. The most recent public release just leaned into that, in fact, with the stabilization of THIR unsafeck. And when something affects a block, I think braces on it are appropriate.

See the following post from the last time this came up for more about what I think is a better way to address this motivation:

6 Likes
3

Agreed, if it affects a lexical block it should have the braces, and I'd like to keep the current behavior. My intention is to add a second use case, where it does not affect an entire block, but the exact expression or statement it prefixes.

1 Like
4

Personally I wrap unsafe { } not just around the specific expression that requires it, but around all code upon which its safety depends (which can often be the entire function body), as this will alert any future coders to the dangers that lie in modifying anything therein.

2 Likes
5

It's hard to distinguish the bounds of how far unsafe goes without braces. If you write unsafe a.b.c().d(), is that unsafe { a.b }.c().d() or unsafe { a.b.c() }.d() or unsafe {a.b.c().d() }? If you write unsafe e1 + e2 is that unsafe { e1 } + e2 or unsafe { e1 + e2 }?

If we were to introduce something like that, we'd need a rule for how far it extends, and that seems more complex and less immediately obvious than requiring the braces.

With braces, the reader of the code can very obviously see how far unsafe goes.

Personally, the way I'd love to write the function you started the thread with is:

unsafe fn map_physical_region<T>(&self, physical_address: usize, size: usize) -> PhysicalMapping<Self, T>
unsafe {
    PhysicalMapping::new(
        physical_address,
        self.as_ptr(physical_address),
        size,
        size,
        *self,
    )
}
6 Likes
6

I have in the past proposed that the unsafe keyword only applies to a single function call, and method calls use a different syntax:

So you would write your example as a.b.unsafe c().d().

This wouldn't work with operators, only function calls and method calls.

In unsafe f(g()).h() it would only apply to f, not g or h. Note that there is currently no way to write this without introducing an extra variable or unnecessarily including g() in the unsafe block.

If anybody mistakenly writes it thinking it applies to more than the f call there will be no great harm done: they will just get a compile error.

In this example, assuming self.as_ptr is a safe function, it is unnecessarily included in the unsafe block. So there is more to analyze regarding safety than if the unsafe keyword only applied to the function call.

2 Likes
7

I don't think such precise unsafe is needed.

Very often unsafety depends on a larger context, not just a single expression. For example, safety of a call to an unsafe function typically depends on what arguments are passed to it, so all the code that computes and checks these arguments can cause unsafety too. IMHO it's better to judge unsafe blocks as a whole, instead of focusing only on tiny specifics.

If a block has a mix of unsafe code, and safe code that is really unrelated to the unsafety and gets in the way of reviewing actually unsafe code, then perhaps it would be better to reorganize such code than to pepper it with microunsafe keywords.

The syntactic noise of unsafe {} is IMHO a feature. It should stand out to be easy to spot. It should add friction at the point of use to nudge developers to make safe abstractions instead of using a little bit of unsafe everywhere else.

6 Likes
8

This argument doesn't work because it's already not enough to study code that's inside the unsafe block, changing code outside an unsafe block can already make an unsafe block unsound.

So in the case where only one function call is unsafe, wrapping more code in an unsafe block than necessary doesn't help anything.

If you apply unsafe to a single function call rather than a block, the benefit is that you know where to look for the prerequisites that have to be satisfied: the documentation of that one function.

5 Likes
9

Very very relevant point. I have personally witnessed both

  • a case where a macro author has unintentionally wrapped up some caller-provided expr metavariable inside of an unsafe block[1] that was there for a surrounding unsafe function whose preconditions were enforced by this safe macro. The expression was usually a closure passed to this function. The refactor to move the closure before this function call worked, but I feel somewhat bad about it in the general case, because you fully give up the enhanced type inference magic that closures directly passed to functions gain. Also for such a refactor, using match instead of let is generally reasonable as to not change the scope of temporary lifetimes, which is another very subtle usability point and easy to get wrong
  • an instance of API design that could not use the most straightforward approach of marking some unsafe function as unsafe directly due to strong usability concerns, as the API was supposed to receive a closure callback and act as some sort of “scope” callback API. The passed closure is thus containing potentially a large block of code, and you don't want to mark all that code as unsafe, too. It took us multiple … iterations to come up with a satisfying alternative API that avoids this problem, and all because you cannot simply mark a function call as unsafe without wrapping all the function arguments in the same unsafe block

  1. I'm linking the part of the PR that fixes this.

    The PR also addresses lints, which have similar scoping problems, though harder to solve neatly at a language level. E.g. see the if false trickery to avoid needing to wrap the whole user expression into an #[allow(unused_labels)] lint. For lints, perhaps some way to save and restore a lint level inside of a smaller sub-scope of a larger #[allow(…)] scope would be useful. ↩︎

3 Likes
10

This feels like an argument for a way to flag code inside an unsafe block as safe code. Downside is coming up with a sensible way to nest not_unsafe and unsafe blocks such that there's no obfuscated behaviour to care about; my gut feeling would be to say that the nearest block defines whether unsafe powers are in action or not, so that in the following nesting pattern:

unsafe {
    // 1
    unsafe {
        // 2
        not_unsafe {
            // 3
            not_unsafe {
                // 4
                unsafe {
                    // 5
                }
                // 6
            }
            // 7
        }
        // 8
    }
    // 9
}

You can use unsafe powers at points 1, 2, 5, 8 and 9, since the immediate enclosing block is unsafe, but not at points 3, 4, 6 and 7, since the immediate enclosing block is not_unsafe. I can see some argument for counting the number of unsafe and not_unsafe (which would add unsafe powers to points 3 and 7), but I'd argue that this is likely to surprise developers.

This would then let your PyO3 API look like:

unsafe { py_local_threads(|| not_unsafe { awesome_ferris() }) }

where the presence of not_unsafe stops awesome_ferris() having unsafe powers, because it's in a not_unsafe block, not an unsafe block.

Note that this is just a bikeshed opportunity - I'm not saying that this syntax is good (it very obviously isn't from naming onwards), merely that you're talking about cases where you want to "surrender" unsafe powers while still being inside an unsafe block.

2 Likes
11

Personally, I'd like to see a .unsafe, similar to a .await. postfixes tend to be better than prefix keywords.

And if we stick with the async/.await similarity we can see that unsafe/.unsafe gives the unsafe keyword 2 different behaviors...

2 Likes
12

Adding support for unsafe hygiene would fix that, right? That is the unsafeness of an expression would be determined by if the source of the expression is wrapped inside an unsafe block or not rather than if the final location after macro expansion is wrapped in an unsafe block or not. Just like identifiers are resolved relative to their source scope rather than post-expansion scope.

5 Likes
13

Postfix is bad for unsafe because it requires you to go back and reevaluate the prior expression with new context. await and ? operate on a value, so a consistently left-to-right control flow makes sense for them. unsafe operates on and changes the interpretation of a scope, so should be said before the scope it controls.

Consider

{
    // potentially long block of code
}.if condition_holds();

Some languages do have this "retroactive" control flow (e.g. return $expr if $test), but Rust doesn't. Even with postfix macros, most discussion has settled into that the macro should capture the receiver as a place evaluated before the macro, and not be able to muck with control flow outside of the macro call itself.

7 Likes
14

Another idea might be, that using braces instead of curly braces was allowed. In this case, there is no unsafe block, but only on expression allowed. this would still fix curly brace issues the likes of

let Some(x) = unsafe(whatever_fn()) else {

}

while keeping the keyword prefixed.

I personally don't love this for a simple reason: In a program like a game engine, where you have 98% Safe code and 2 % unsafe code, it makes sense to warn with noise. This excerpt is from a micro controller OS kernel. There is about 80% unsafe code and 20% logging. I don't need noise to know it is potentially unsafe. Using a Box is potentially unsafe here. But when every method that could be one or two lines requires a nested unsafe block, the code gets longer and one nesting level deeper for no reason. (No, rustfmt does not leave the second unsafe aligned with the first, and it would make no sense to do so, because, in fact, it is a block.)

15

But this can be easily emulated with a macro, and things that can be a simple macro don't tend to get added to the language.

16

You can easily emulate let else with a macro. You could emulate async await with a macro. You can emulate Err(e)? with a macro. Thats not an argument seeing how powerful the macro system is.

1 Like
17

None of those are easy, the latter two I'm pretty sure aren't even possible (assuming try block support for ?). Making a macro for unsafe!(foo()) is trivial.

2 Likes
18

No, rustfmt does not leave the second unsafe aligned with the first, and it would make no sense to do so, because, in fact, it is a block.

Read the example I gave again: it only has one set of braces. And this would need both language and rustfmt support.

19

Going off on a tangent here with a fleeting thought: maybe it should be the module that is declared unsafe, since that is the boundary at which safety can be encapsulated (via field/method visibility)?

20

Scope of the unsafe block is a balance between false positives and false negatives — you don't want too much safe or unrelated code in the unsafe block, because it distracts from the important things to review. But you also don't want to miss code or inputs that can cause unsafety, but are outside of the unsafe block.

It's a balance that may not be perfect, because theoretically there's no upper bound to how large the scope of unsafe can be. For example, when calling a non-reentrant C function (one that uses local static buffers) the invariant is that there must be no other caller of such function, which could be broken anywhere in the program or even libraries the program links to.

In the other extreme, when only a single function call is marked unsafe, then all of its inputs and potential prerequisites are outside of the scope of unsafe. Then it's not clear how far to review the code around it. How far do I need to track the inputs to the function? Where the invariants have been checked?

If you mark a whole module as unsafe, it may be technically correct, but it's not helpful. It moves the trade-off too much towards false positives. The role of unsafe is to help code reviews focus on the dangerous parts. When a whole module is marked unsafe, then this focus is lost, and you lose the advantage that safe/unsafe split has over reviewing C or C++ code.

Taking Vec.len for example. Any code in the entire module could in theory break len and cause UB. However, majority of code implementing Vec does not change that field. There's code that is unsafe for other reasons. There's plenty of code that is safe. So putting a whole-module unsafe due to the len field has a very low signal-to-noise ratio — it doesn't even highlight uses of len, and casts a doubt on code unrelated to len.

So in my experience the middle ground of large-ish blocks of unsafe {} works best.

When calling an FFI function, an unsafe block can include checks of function's arguments, so the reviewer doesn't have to search larger scope and trace where the inputs come from and where they have been checked before.

When manipulating something like Vec.len, the unsafe block can be designed so that the length is known to be correct before and after the block, and inside the block there's all manipulation and computation of the new length. If there's only unsafe vec.set_len(new_len);, then it's not clear at which point vec.len became outdated, and where the code that must be panic-safe starts.

3 Likes