Private unsafe fields are a poorly motivated feature

[tczajka]

I have read a long discussion about unsafe fields and I find it poorly motivated for private fields. I came here to express why.

I think the feature makes sense for public fields.

The main motivation in the RFC is:

Consequently, Rust programmers will be able to use the unsafe keyword to denote when a named field carries a library safety invariant

And then it provides examples of private fields with "library safety invariants".

But what is a "library safety invariant"? Is it different from just any other "library invariant"?

Suppose I write a non-zero type:

pub struct NonZero {
    /// This is never zero.
    val: u32
}

impl NonZero {
    pub fn new(val: u32) -> Self {
        assert_ne!(val, 0);
        Self { val }
    }
    
    pub fn get(self) -> u32 {
        self.val
    }
}

So val being non-zero is an invariant maintained by my module.

Is val being non-zero a "library safety invariant"? Should the field be marked unsafe? Presumably not because I don't have any unsafe code in my library that relies on it.

Let's say I add such code:

impl NonZero {
    pub fn crazy(self) -> u32 {
        // Safety: self.val is never 0
        unsafe { 100u32.checked_div(self.val).unwrap_unchecked() }
    }
}

According to the RFC, val should now become an unsafe field because mistakenly setting it to 0 would cause undefined behavior, i.e. a memory safety issue.

But this is inconsistent!

Suppose I don't add crazy, and thus keep val a "safe" field.

A user of my module can do the same exact thing that I did:

fn user_crazy(x: NonZero) -> u32 {
    // Safety: x is never 0
    unsafe { 100u32.checked_div(x.get()).unwrap_unchecked() }
}

If any user of my module does that, mistakenly setting val to 0 still causes undefined behavior, i.e. a memory safety issue.

So as the author of my module, I can't know which of my invariants are important for memory safety. To be consistent, I would need to make all structs with any invariants have unsafe fields. But that's obviously ridiculous -- so many structs maintain some sort of invariants.

I think this feature tries to do something impossible. It tries to use unsafe to indicate places in code where bugs might cause UB. But by design, that's basically impossible. unsafe code often relies on correctness of surrounding safe code.

The only thing that can reliably protect invariants is privacy boundaries. And thus it seems to me the way invariants can interact with unsafe is to have unsafe mark APIs with requirements across privacy boundaries.

So an unsafe field might make sense when a field is public. It doesn't really make sense for private fields.

Later in the RFC it explicitly says that correctness invariants are not considered safety invariants:

The unsafe modifier should only be used on fields with safety invariants, not merely correctness invariants.

But again, given that some code may rely on correctness of your module for safety, this distinction doesn't make sense to me.

[RalfJung]

@tczajka

So as the author of my module, I can't know which of my invariants are important for memory safety.

You seem to have misunderstood the RFC.

A field should be unsafe if it is important for soundness of the module itself. Whether other modules rely on functional behavior of your module is irrelevant here and not your concern.

Personally I don't think we should allow public unsafe fields, I would prefer if all soundness-critical fields must be private. But I am apparently in the minority with that position .

[tczajka]

@RalfJung

A field should be unsafe if it is important for soundness of the module itself. Whether other modules rely on functional behavior of your module is irrelevant here and not your concern.

What if the unsafe code (user_crazy) is in a different module, but same crate? Should the field (private to the module) be unsafe then?

[scottmcm]

The point of the RFC, as I see it, is to solve the Vec::set_len problem.

There's lots of people, even academic papers, who make the mistake of "well there was no unsafe in the body so the function didn't need to be unsafe", which is just wrong.

But by making the len field in Vec be unsafe, you eliminate that confusion.

Thus I think it's great, even for private fields.

[tczajka]

@scottmcm

The point of the RFC, as I see it, is to solve the Vec::set_len problem.

There's lots of people, even academic papers, who make the mistake of "well there was no unsafe in the body so the function didn't need to be unsafe", which is just wrong.

The RFC doesn't solve it for this example:

/// This is guaranteed to never be zero.
#[derive(Copy, Clone)]
pub struct NonZero {
    val: u32
}

impl NonZero {
    pub fn new(val: u32) -> Self {
        assert_ne!(val, 0);
        Self { val }
    }
    
    pub fn get(self) -> u32 {
        self.val
    }
    
    pub unsafe fn set_unchecked(&mut self, val: u32) {
        self.val = val;
    }
}

set_unchecked has to be unsafe and uses no unsafe code internally.

Or should val be an unsafe field here, even though the module doesn't use it for memory safety?

[ia0]

@scottmcm

There's lots of people, even academic papers, who make the mistake of "well there was no unsafe in the body so the function didn't need to be unsafe", which is just wrong.

Exactly, and there's another related argument: unsafe reviews. Those are manual human reviews, and as most humans they want to do as little as possible. So being able to audit foreign code for soundness without having to reverse the safety invariants is an advantage.

So this is mostly a matter of taste.

I personally like unsafe fields, because if you don't like them, you simply don't use them. They don't hurt. While at the same time, they fill a theoretical hole that some may occasionally use.

[ia0]

@tczajka

The RFC doesn't solve it for this example:

That's an interesting example. With the robust keyword of the unsafe mental model, you could write:

impl NonZero {
    /// Returns the underlying non-zero value.
    ///
    /// # Robustness
    ///
    /// The returned value is non-zero.
    pub robust fn get(self) -> u32 {
        self.val
    }
}

The Robustness section is the dual of the Safety section. It provides safety guarantees instead of requiring safety conditions. Also note that it differs from Correctness because it only guarantees non-zero, it doesn't guarantee that the same value is returned as the one it was built.

Note that you can simulate that with an unsafe trait:

/// API for NonZero::get()
///
/// # Safety
///
/// The `get()` method must return a non-zero value.
unsafe trait NonZeroGet: Sealed {
    fn get(self) -> u32;
}

[scottmcm]

@tczajka

Or should val be an unsafe field here, even though the module doesn't use it for memory safety?

I would respond that the example is poor.

It should clarify whether "guaranteed" is correctness or soundness, and then either

• make set_unchecked not be unsafe, or
• make val be unsafe, updating the new with a safety comment about why constructing it there is correct

Right now it's unclear to consumers whether they can soundly do std::num::NonZero::new_unchecked(your_nz.get()) or not, so even without this feature that should be clarified.

(Similarly, if set_unchecked is going to be unsafe fn then it should have a # Safety comment to state its preconditions.)

[RalfJung]

@tczajka

Or should val be an unsafe field here, even though the module doesn't use it for memory safety?

As author of the module, you choose the safety invariant of the type. If "not zero" is part of the safety invariant, then the field should be unsafe. If it is just a "logic invariant" (like Ord contract), then it should not.

[tczajka]

@scottmcm

It should clarify whether "guaranteed" is correctness or soundness

I see. I will point out that std::num::NonZero also doesn't seem to explicitly clarify this distinction. It just says:

A value that is known not to equal zero.

and doesn't elaborate on whether it's a correctness or soundness guarantee.

It seems a bit weird to have to clarify this for your guarantees. You then have to state it everywhere, and nobody is really going to want to say what amounts to "I guarantee X but don't trust me for soundness" because it basically says "watch out, I wasn't very careful, this code might be buggy".

You basically want me to state how sure I am of the correctness of the code I'm publishing.

@RalfJung

If it is just a "logic invariant" (like Ord contract), then it should not.

It's different because you don't control who can implement Ord, but only my module implements NonZero. So saying I only provide a guarantee "for purposes of correctness but not for purposes of soundness" would be admitting I'm not sure about my own code. Nobody wants to say that.

[steffahn]

@scottmcm

The point of the RFC, as I see it, is to solve the Vec::set_len problem.

If that’s indeed the primary motivation, I’m wondering why the RCF turned out choosing the “all access is unsafe” option over allowing reads.

[tczajka]

@scottmcm

It should clarify whether "guaranteed" is correctness or soundness, and then either

• make set_unchecked not be unsafe, or
• make val be unsafe, updating the new with a safety comment about why constructing it there is correct

I don't think the first option makes practical sense. Even if there is no risk of undefined behavior in my code or in my users' code (perhaps because they never actually call unsafe APIs), I still want to make sure that my module is responsible for maintaining its own invariants. This is what encapsulation is all about. If it turns out an invariant is violated, I want to know that either the module is buggy, or somebody called some unsafe thing incorrectly. If I made set_unchecked safe, I would lose this benefit of encapsulation. I would now have to grep for calls to set_unchecked to see if somebody else messed with my invariants.

But if I choose the second option for all my structs with invariants, I'd see a proliferation of unsafe fields everywhere, in essentially all my structs that have private fields because most such structs do have some sort of invariants they want to maintain. I don't think that's practical either.

A third option might be "normally keep val safe but the moment you add something like set_unchecked to your module, then change val and all its uses to unsafe". But that's weird and inconsistent too. Just because I add one little rarely used unsafe API that the rest of the code doesn't care about, I don't want to have to change how all the rest of the code is written.

[ryanavella]

@scottmcm

make val be unsafe, updating the new with a safety comment about why constructing it there is correct

This is a lot of churn and raises the MSRV for what is primarily an issue of non-standardized documentation. Private unsafe fields are an implementation detail and so can't be relied on as documentation of soundness.

To give an example, is it sound to write NonZeroU32::new_unchecked(nz.get() as _), where nz is a NonZeroU64? Can you point to me where in the docs it guarantees that for soundness (as opposed to just correctness) purposes a downstream user can rely on .get() producing a non-zero value?

[scottmcm]

@steffahn

I’m wondering why the RCF turned out choosing the “all access is unsafe” option over allowing reads.

Well, that's also the version I want -- see the RFC discussion thread -- but I basically got outvoted.

@ryanavella

This is a lot of churn and raises the MSRV for what is primarily an issue of non-standardized documentation.

TBH, I don't care about your MSRV. Let's get the MSRV-aware resolver, then you can use an old version of my crate until you update.

(Or you can just continue to not do this, like you can make unsound fns all day long if you want. The language won't stop you.)

@tczajka

But if I choose the second option for all my structs with invariants, I'd see a proliferation of unsafe fields everywhere, in essentially all my structs that have private fields because most such structs do have some sort of invariants they want to maintain. I don't think that's practical either.

You can have an invariant that's critical for correctness without it being critical for soundness.

There's lots of code with "this is always Some after ______", for example, but where it never uses unwrap_unchecked, only ever unwrap, because while yeah it's supposed to be like that, it's still worth checking. (Concrete example: https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/mir/struct.BasicBlockData.html#structfield.terminator, which has an invariant, but not one used for soundness ever. It might ICE if it's wrong, but it's not unsound.)

Just like how it's entirely normal to have an invariant that an index is in-bounds that's important for correctness but to still use [] indexing, not get_unchecked, because it's not a safety invariant.

One could argue that the main point of Rust is splitting these two types of invariants for explicitly, rather than having every invariant be probably-soundness-critical like in C.

[Ddystopia]

@tczajka

It seems a bit weird to have to clarify this for your guarantees. You then have to state it everywhere, and nobody is really going to want to say what amounts to "I guarantee X but don't trust me for soundness" because it basically says "watch out, I wasn't very careful, this code might be buggy".

Well, I personally do that. And I believe it isn't biggy. Maintaining guarantees is extremely hard. It is a big responsibility. Just "be good" won't allow you to give a guarantee. It is a totally different way of maintenance. Either I can code as a normal human or I am walking across the module like a ninja and making sure every little thing is correct to not accidentally break it for any cost.

[Ddystopia]

Well, something like NonZero by @tczajka would benefit from reads being unsafe, as get will contain unsafe code.

[steffahn]

I don’t get what argument you’re trying to make, could you elaborate? How does reads being unsafe benefit the use-case of a NonZero type?

[Ddystopia]

You seem to be using unsafe to ensure the logical validity of the program. It undermines the purpose of the unsafe and I believe it is extremely hurmful.

Bugs caused by memory-safety issues (and other "real" unsafe stuff) are "beyond" logic. You can't use conventional logic, because it is undefined behavior, anything can happen. If you happen to write some bytes on the foreign stack then in 15 minutes you may increase the payment of Bob from 5$ to 500$. Those bugs are horrible because you can't reason about them, undefined behavior is scary, and non-local.

Logical invariants are local and logical, you can trace the logic back and fix the issue. If you use unsafe there, you dilute the real stuff by this.

When we say "I don't give a safety guarantee" I don't say "my code is buggy", I say "I am not taking a burden of preventing the inexplicable horrors from happening". If you use a library with unsafe it must be audited regularly. If you won't give those safety guarantees and remain safe, there is no need. Bugs you will potentially cause will be local and logically traceable.

[tczajka]

@scottmcm

You can have an invariant that's critical for correctness without it being critical for soundness.

Let's say I'm publishing a general-purpose prime-number crate. Pure number theory. I don't mess with memory etc. There is no danger of UB anywhere in the crate no matter what. I don't really know how users will use it, I mostly write it for my own number theory experiments where I don't use any unsafe code.

pub struct Prime {
    p: u32
}

It's meant to store prime numbers only. So for instance, prime factorization returns Vec<Prime>, and Prime::new runs a deterministic primality test.

So now assuming the "unsafe field" feature becomes stable, I ask for recommendations:

1. Do you recommend I document it as "prime number (just correctness)" or "prime number (soundness)"?

2. Would you recommend I make p an unsafe field or not?

3. Suppose I add a new_unchecked constructor that just assumes the input is prime so it doesn't run the primality test. Would you recommend that function be marked unsafe or not?

[RustyYato]

@tczajka

I see. I will point out that std::num::NonZero also doesn't seem to explicitly clarify this distinction. It just says:

A value that is known not to equal zero.

and doesn't elaborate on whether it's a correctness or soundness guarantee.

It does make that guarantee,

Layout - NonZero in std::num - Rust

NonZero<T> is guaranteed to have the same layout and bit validity as T with the exception that the all-zero bit pattern is invalid. Option<NonZero<T>> is guaranteed to be compatible with T, including in FFI.