Unsafe fields

[jhpratt]

Fixes #381

Rendered

Tracking:

• Tracking issue for RFC 3458: Unsafe fields rust#132922

[juntyr]

I’m not sure I like that the entire struct expression is now inside an unsafe block (though I’m not sure what a better syntax would be). If the safety invariant requires the entire struct, this makes sense. However, if it is more specific, a larger unsafe block is too broad as it also allows unsafe code usage to initialise the safe fields, which should get their own unsafe blocks. Though perhaps this could just be linted against, e.g. don’t use unsafe expressions in a struct initialiser without putting them inside nested unsafe blocks.

[jhpratt]

The alternative is to do this:

let mut foo = Foo {
    safe_field: 0,
    unsafe_field: unsafe { 1 },
};

That feels worse to me.

Presumably the safety invariant being promised is an invariant within the struct, even if it is not strictly required that that be the case.

Ideally we'd also have partial initialization, such that it would be possible to do

let mut foo = Foo { safe_field: 0 };
unsafe { foo.unsafe_field = 1; }

but I expect that's quite a bit farther away.

[juntyr]

I agree that just unsafe in one field initialiser expression is insufficient as it means something very different. I do like however that it clearly shows which field the unsafety applies to.

It reminds me a bit of unsafe blocks in unsafe functions (#2585). Ideally, the fact that struct init is unsafe would not allow unsafe field init expressions. I doubt that special-casing struct inits to not propagate outer unsafe blocks would be backwards compatible, so a new lint would be the only avenue in this direction.

Some new syntax like this

let mut foo = unsafe Foo {
    safe_field: 0,
    unsafe unsafe_field: 1,
};

would communicate intent better but looks quite unnatural to me.

Perhaps in the future there might be explicit safe blocks to reassert a safe context within an unsafe block, so that it could be written as follows:

let mut foo = unsafe {
    Foo {
        safe_field: safe { /* some more complex expr here */ },
        unsafe_field: safe { /* some more complex expr here */ },
    }
};

which could be encouraged with clippy lints. Though for now just moving safe non-trivial struct field initialisers into variables that are initialised outside the unsafe block.

Overall, I think going with the original syntax of

let mut foo = unsafe {
    Foo {
        safe_field: 2,
        unsafe_field: 1,
    }
};

looks like a good and intuitive solution, though I still think that a lint against using unsafe expressions inside the struct expression without a nested unsafe block would still help.

[jhpratt]

Maybe this?

let mut foo = unsafe Foo {
    safe_field: 0,
    unsafe_field: 1,
};

unsafe would be followed immediately by a struct expression, and doing so would only indicate that unsafe fields may be initialized. It would not introduce an unsafe context.

[juntyr]

I like it - the syntax is still close enough to the unsafe block syntax (except when initialising tuple structs) that it's relatively intuitive what the unsafety applies to, but coupled strongly to the struct name so that it also makes sense why no unsafe context is introduced.

[scottmcm]

I think there's something nice about

let mut foo = unsafe Foo {
    safe_field: 0,
    unsafe_field: 1,
};

but to me that would go with unsafe struct Foo, like how unsafe trait Bar leads to unsafe impl Bar.

Spitballing: if per-field safety is really needed, then maybe

let mut foo = Foo {
    safe_field: 0,
    unsafe unsafe_field: 1,
};

though maybe the answer is the same as for unsafe { unsafe_fn_call(complicated_expr) }: if you don't want the expr in the block, use a let.

After all, there's always field shorthand available, so you can do

let safe_field = unsafe { complex to construct };
let unsafe_field = easy and safe to construct;
unsafe { Foo { safe_field, unsafe_field } }

Perhaps the more important factor would be the workflow for the errors the programmer gets when changing a (internal and thus not semver break) field to unsafe. Anything at the struct level wouldn't give new "hey, you need unsafe here" if you already had another field that was unsafe.

Of course, if the safety is at the struct level (not the field level) then that doesn't come up.

---

Hmm, the "you added an additional requirement to something that's already in an unsafe block and there's no way to help you make sure you handed that" is a pre-existing problem that needs tooling for that everywhere, so maybe it's not something this RFC needs to think about.

If we had unsafe(aligned(a), nonzero(b)) { ... } so that tooling could help ensure that people thought about everything declared in the safety section, then we'd just have unsafe(initialized(ptr, len)) { ... } so that it acknowledged the discharge of the obligation for the type invariant, and the "I need 100 unsafe blocks" problem goes away.

[algesten]

By introducing unsafe fields, Rust can improve the situation where a field that is otherwise safe is used as a safety invariant.

I think the motivation could point out who would benefit from this.

I assume it's library authors a making unsafe fields a "reminder to self" about upholding some invariant as opposed to say expecting a unsafe field in an API surface. I.e we still expect unsafe set_len rather than pub unsafe len, right?

[programmerjake]

this seems closely related to mut(self) fields #3323 which should probably be mentioned.

[jhpratt]

I assume it's library authors a making unsafe fields a "reminder to self" about upholding some invariant as opposed to say expecting a unsafe field in an API surface. I.e we still expect unsafe set_len rather than pub unsafe len, right?

Not necessarily. It is entirely reasonable that Vec.len could be exposed. Whether it actually is exposed is a decision solely for T-libs-api. Likewise with the inner fields of the various nonzero types. I'd rather not tie down who this is intended for, as it truly is intended for everyone. I know I've written a binary that had fields relied upon in unsafe code — there was just no way to make it actually unsafe.

this seems closely related to mut(self) fields #3323 which should probably be mentioned.

Related, sure, but beyond the mention in unresolved questions, I'm not sure how it could be mentioned. Pretty much the only overlap is what's considered a "mutable access", which I didn't feel necessary to be copy-pasted.

[jsgf]

How does this look with functional struct update?

Foo {
// Stuff
.. unsafe { other }
}

?
Or does the whole initializer need to be unsafe?

Edit: or I guess it doesn't need unsafe if the source had been initialized with unsafe.

[jhpratt]

@jsgf Great question. I don't have an immediate answer, though I believe the mechanism currently in the compiler would require the entire initializer to be unsafe.

[mo8it]

I love it! This is much better than getters and setters, both for library authors and users.

typed-builder would have to adjust, but I would love to implement this feature there :)

@idanarye (the main author) Maybe you have some input regarding the builder pattern?

[djc]

The implicit notion that only mutation is unsafe (and reading is not) seems tricky. Do you have reasoning to prove that we'll never need fields that are unsafe to read? Maybe there should be an alternative syntax proposal (like unsafe(mut) or mut(unsafe)) that makes this more obvious/explicit?

[idanarye]

@mo8it I don't want to spam the comments here with a discussion about typed builder, so I've opened a discussion in my repository instead: idanarye/rust-typed-builder#103

[idanarye]

Regarding the RFC itself - I think you are trying to solve a visibility issue with the safety mechanism, which is the wrong tool for the job. You gave Vec as an example, and it looks like you want to grant public read access to its len field (so that you don't have to use len() as a method? Let's ignore the question if that's big enough an improvement to justify such a feature). To do so, you are willing to provide public write access to it as well but make that access unsafe.

But why?

I mean, it's obvious why you don't want to give regular write access to len. But why give any access at all? Even if you require an unsafe block, what good will come from letting external users modify the len field without going through a method that upholds the invariant? As long as we are devising a brand new feature, wouldn't it make more sense to add a feature that gives public read access without any write access at all (other than private access from inside the defining module, of course)?

I'm aware of the set_len method that grants such access, but this is an explicit decision to give such access, with a fully documented method. Not a side-effect of wanting to provide a non-method-call read access to the field.

---

Another thing - conceptually it never makes sense to define only one field as unsafe. The invariant is a property of the struct as a whole. If this is unsafe:

let mut vec = Vec::new();
vec.len = 4; // UNSAFE!!!

The why not this?

let mut vec = Vec::from([1, 2, 3, 4]);
vec.buf = RawVec::new(); // perfectly safe apparently

Yes, buf is not publicly exposed at all, but inside the module len will need unsafe block to modify and buf won't, even though the invariant is about both of them, together, and how they interact with each other.

Whatever the semantics of unsafe fields will be - conceptually it makes more sense to put the unsafe on the struct itself. If there are fields that are not part of the invariant, they should not be part of the struct - because unsafety should be contained as much as possible and not contaminated with unrelated data. The only reason putting unsafe only on len seems to make case in your example is that the goal - as I've said before - is about visibility, not about safety.

[Jules-Bertholet]

One could argue that ptr::addr_of_mut! on an unsafe field need not be unsafe, because writes through the pointer are unsafe.

[jhpratt]

Consider this in conjunction with the examples present in the RFC.

fn make_zero(p: *mut u8) {
    unsafe { *p = 0; }
}

let p = ptr::addr_of_mut!(foo.unsafe_field);
make_zero(p);

Ignoring thread-safety, which can be easily achieved with locks, no single step appears wrong. make_zero does not do anything wrong — assigning zero to an arbitrary *mut u8 is fine. Passing a pointer to the method is naturally okay. Yet it still results in unsoundness, as foo.unsafe_field must be odd.

[Jules-Bertholet]

"assigning zero to an arbitrary *mut u8 is fine" what‽ No it is not fine‽ An arbitrary *mut u8 could be null, dangling, aliased... fn make_zero is unsound.

[jhpratt]

True — I typed that far too quickly and without thinking. Regardless, it's not immediately obvious to me that ptr::addr_of_mut! should be allowed safely.

[Jules-Bertholet]

I am not fully convinced either way, but I fear it would just be confusing to require unsafe for an operation that can't lead to unsoundness, especially as addr_of!(struct.field) as *mut _ would do the same thing with no unsafe.

[jhpratt]

especially as addr_of!(struct.field) as *mut _ would do the same thing

While you can do that, it's undefined behavior to actually mutate the resulting mut pointer. I've just confirmed this with miri.

[Jules-Bertholet]

It's UB under Stacked Borrows, but MIRIFLAGS=-Zmiri-tree-borrows accepts it.

[jhpratt]

That's surprising. I'm not familiar with tree borrows, admittedly.

[CodesInChaos]

ptr::from_mut(&mut struct).wrapping_offset(offset_of!(Struct, field)) should achieve the same thing as addr_of_mut!, and I doubt you'd want to make offset_of unsafe for unsafe fields. And even without offset_of, you could use ptr::from_mut(&mut struct).wrapping_offset(addr_of!(stuct.Field) - ptr::from_ref(&struct)). Both of these should be safe in every borrowing model.

[burdges]

Vec::len should not do this even if this feature exists, because Vec::set_len is better pedagogically.

static muts require unsafe blocks for both writing and reading. An unsafe field would likely be some similar construction, so unsafe for both writing and reading. An UnsafeCell already hits those requirements, but any variants should set their auto-traits.

Ain't clear this proposal handles auto-traits correctly, even if some use case exists. If you need this, then define your own type which provides this. We've the inner-builder or whatever deref polymorphism pattern, which comes up far more in practice, and can simulate this of desired.

pub struct ThingBuilder { ... }

impl ThingBuilder {
    fn build(self) -> Thing {
        ...
        Thing { ..., inner, self }
    }
}

pub struct Thing {
    ... 
    inner: ThingBuilder,
}

impl Deref for Thing {
    type Target = ThingBuilder;
    fn deref(&self) -> ThingBuilder {
        &self.inner
    }
}

// We stop mutating ThingBuilder once we create a Thing, so Thing: !DerefMut,
// but Thing: Deref<Target=ThingBuilder> to make reading & replicating the
// builder config easy.

[jhpratt]

Do you have reasoning to prove that we'll never need fields that are unsafe to read?

How could a field of a struct be unsafe to read?

---

You gave Vec as an example, and it looks like you want to grant public read access to its len field (so that you don't have to use len() as a method?

Within the module it's defined in (as the field is private), it is currently safe to assign any value, despite the fact that it can lead to undefined behavior. Said another way, the current behavior is inherently unsound.

Nothing in the RFC so much as hints at Vec.len being made public, nor is an RFC an appropriate place to make a change like that. It is an example of a field that should be unsafe to avoid unsoundness and nothing more.

Another thing - conceptually it never makes sense to define only one field as unsafe.

I never claimed that was the case.

The why not this?

let mut vec = Vec::from([1, 2, 3, 4]);
vec.buf = RawVec::new(); // perfectly safe apparently

Inclusion of one example does not mean that everything not included is forbidden. There is simply no reason to repeat the same thing for every field. Of course buf would also be unsafe. I used Vec.len as the example because it's a clear, obvious example where its safety invariants are publicly documented.

The invariant is a property of the struct as a whole.

If there are fields that are not part of the invariant, they should not be part of the struct

For Vec, yes, but only because all fields of the strict interact with all other fields.

I have real world code where this is not the case. The flags field is for whether other fields are initialized. Note that some fields have niche value optimization, and as such don't interact with other fields in any way. Are you asserting that month: Option<Month> and other similar fields should be in a separate struct solely because it has niche value optimization? That appears to a logical conclusion as a result of what you've said.

---

static muts require unsafe blocks for both writing and reading. An unsafe field would likely be some similar construction, so unsafe for both writing and reading.

static mut requires unsafe due to inherent data races between threads. Unsafe fields have no such issue.

Ain't clear this proposal handles auto-traits correctly, even if some use case exists.

I don't follow. What problems do you see with handling auto traits? The type of the field is unchanged, so there would be no impact on auto traits.

[scottmcm]

The appears to be missing the https://github.com/rust-lang/rfcs/blob/master/0000-template.md#rationale-and-alternatives section. Please add one, with a bunch of subsections for various "here's why I picked this way over some other way" decisions you made.

For example, that's a great place to talk about unsafe structs vs unsafe fields.

[scottmcm]

(@jhpratt I'd love for you to steal something like the text in this post for the RFC)

I assume it's library authors a making unsafe fields a "reminder to self" about upholding some invariant as opposed to say expecting a unsafe field in an API surface.

A huge incentive for it, to me at least, is helping avoid misunderstandings about the model.

For example, a 2020 PACMPL paper contains the following statement:

To check how prevalent unsafe is used as a documentation feature, our queries gathered data for unsafe traits and unsafe functions with safe implementations.

set_len being an unsafe fn is of course not merely a "documentation feature". It's an absolutely critical part of the soundness of Vec. (And the paper does talk about "invariants that are potentially critical for upholding Rust’s safety guarantees" (emphasis added), so I don't think their analysis is incorrect, but I still find the phrasing curious.)

Thus a huge win of this would be to avoid the (from the same paper)

After all, the compiler does not force developers to declare such functions as unsafe -- in contrast to other unsafe features.

Having the body of set_len do something the compiler recognizes as unsafe is a big help to people understanding the soundness of Vec, and in general to people adding new features inside existing unsafety privacy boundaries.

Especially combined with other accepted work like https://rust-lang.github.io/rfcs/2316-safe-unsafe-trait-methods.html we could start even doing things like clippy lints for "why is this unsafe when it doesn't do anything unsafe? Should one of the types involved be marked unsafe?"

If we don't have to link to tootsie pops as often because changing things that are relied on by unsafe code is itself unsafe, I'd consider that a big win.

---

As another way to look at this, it's weird that when I'm writing a method on by type with a safety invariant that I can do Self { a, b } and it's totally "safe", whereas if I call Self::new_unchecked(a, b) I need an unsafe block and tidy nags me to write a safety comment.

Tidy should nag about a safety comment for the constructor too, so that I'm not disincentivized to use the other, correctly-marked-unsafe function when writing things.

[burdges]

We do not need unsafe per se when maintaining a safety invariant within its defining visibility boundary aka module:

"Because it relies on invariants of a struct field, this unsafe code does more than pollute a whole function: it pollutes a whole module. Generally, the only bullet-proof way to limit the scope of unsafe code is at the module boundary with privacy."

In other words, rust does not have unsafe types because you must enforce invariants at module boundaries anyways. Also various discussions in https://github.com/rust-lang/unsafe-code-guidelines clarify this point.

A method like Vec::set_len must be unsafe due to being public. An extern fn is unsafe because it points outside the module. etc.

Anyways..

If I understand, you want this type:

pub UnsafeInvariant<T>(T);
impl<T> UnsafeInvariant<T> {
    fn new(t: T) -> UnsafeInvariant<T> { UnsafeInvariant(t) }
    unsafe fn get(&self) -> &T { &self.0 }
    unsafe fn get_mut(&mut self) -> &mut T { &mut self.0 }
}

It's similar to UnsafeCell but propagates all auto-traits normally, based upon your comment above.

You still enforce invariants by visibility but UnsafeInvariant could provide the documentation for which you propose unsafe fields. In practice, I suspect you'd be better off like this:

    fn invariant_get(&self) -> &T { &self.0 }
    fn invariant_get_mut(&mut self) -> &mut T { &mut self.0 }

Why? All those unsafe blocks you'll write risk other mistakes, so ideally they should not exist if they merely maintain some invarant. Instead, you want a safe but distinctively named accessor method, which flags that you maintain the invariant.

Anecdotally, this type winds up being much more common:

struct HideMut<T>(T);
impl<T> Deref for HideMut<T> {
    type Target = T;
    fn deref(&self) -> &T { &self.0 }
}
impl<T> HideMut<T> {
    fn new(t: T) -> HideMut<T> { HideMut(t) }
    unsafe fn get_mut(&mut self) -> &mut T { &mut self.0 }
}

And UnsafeCell remains more common than both of course.

In fact, if you wrap the HideMut declaration inside some macro_rules! use_hide_mut then HideMut becomes module local, so the local module can access pub foo: HideMut<Foo> fields freely, but the outside world has only immutable access, even if given a &mut for the containing struct. This is really the common pattern.

We made this a local type for visibility modifiers, so a language level construct helps here. Also conversely, if you do not require visibility modifiers then simple types like UnsafeInvariant suffice, no language change necessary.

I suppose one might imagine pub(positive_visibility) unsafe(negative_visibility) mut(positive_visibility) field: type, except this still cannot capture when mutation becomes unsafe but reading remains safe. Yet, visibility control types like UnsafeInvariant and HideMut work fine.

[scottmcm]

suggestion: include a section talking about how both of the following are reasonable types:

/// This type has a *correctness* invariant that it holds an even number,
/// but it's not something on which `unsafe` code is allowed to rely.
pub struct Even(u32);

impl Even {
    pub fn new(x: u32) -> Option<Self>;
    pub fn new_i_promise(x: u32) -> Self;
    pub fn get_value(&self) -> u32;
}

/// # Safety
///
/// The value of this type must always be an even number.
/// (`unsafe` code is allowed to rely on that fact.)
pub unsafe struct Even(u32);

impl Even {
    pub fn new(x: u32) -> Option<Self>;
    pub unsafe fn new_unchecked(x: u32) -> Self;
    pub fn get_value(&self) -> u32;
}

and it's up to the type author to decide which is appropriate for the expected uses of the type.

[scottmcm]

EDIT: see below; it looks like the thing I was worried about here is probably impossible for other reasons.

I do thing that "safe to read; unsafe to modify" is the 99%+ case, and should certainly be the default, but

How could a field of a struct be unsafe to read?

Well, the field in AtomicPtr is unsafe to read, because it could be a race, for example. https://github.com/rust-lang/rust/blob/7a5814f922f85370e773f2001886b8f57002811c/library/core/src/sync/atomic.rs#L176

Or the value field of a ShardedLock in crossbeam https://docs.rs/crossbeam-utils/0.8.11/src/crossbeam_utils/sync/sharded_lock.rs.html#81

So perhaps some nuance for !Freeze could make sense? I'm not sure what the semver implications of that would be, though.

[Jules-Bertholet]

Well, the field in AtomicPtr is unsafe to read, because it could be a race

I don't think that is correct? The unsafe operation is dereferencing the pointer returned by UnsafeCell::get(), accessing the field can't lead to UB on its own.

[scottmcm]

accessing the field can't lead to UB on its own

Ah, I guess an access can't actually read an UnsafeCell (without ownership) because it's never Copy.

So I think the PlaceMention is always ok for everything, and a read would be unsafe for an UnsafeCell, but you can't actually do a read of an UnsafeCell directly in Rust.

[jhpratt]

We do not need unsafe per se when maintaining a safety invariant within its defining visibility boundary aka module

rust does not have unsafe types because you must enforce invariants at module boundaries anyways

The nomicon describes current behavior; using it as an argument against this RFC is counter to the purpose of the RFC. It's circular reasoning at best.

if they merely maintain some invarant.

The invariants are "merely" there for soundness. If the invariant is violated, the result is undefined behavior. That's far more serious than you make it sound.

Yet, visibility control types like UnsafeInvariant and HideMut work fine.

I have never seen anyone write code like this in practice. The standard library and my own code (in time) is included in this. That is a significant argument in favor of something better.

---

@scottmcm I'll definitely include parts of that into the RFC. Also reading that blog post now — I'd never seen it before.

[burdges]

If the invariant is violated, the result is undefined behavior.

That is a significant argument in favor of something better.

This RFC is not better because memory safety also helps when writing unsafe code.

The unsafe keyword is not simply a marker to tell you where to read more carefully. Its a marker of where safety rules must be violated.

In other words, we always convert regular code invariants into memory safety assurance, but these regular code invariants have exactly the same risks as other regular code, including their own memory safety concerns. This RFC confuses the memory safety consumed in maintaining the regular code invariant with the actually unsafe options the code requires.

In the past, unsafe fn bodies were unsafe blocks, but rust changed this to reduce the unsafe code surface area. This RFC is a mistake because it increases the unsafe code surface area with no benefits, given the same cautions can be maintained in other ways, like by variable naming, etc.

UnsafeInvariant not being used is evidence this feature is not required. UnsafeInvariant would make sense if you wanted to split the regular code invariant across distant visibility boundaries. In practice, unsafe fns always sufficed, or indeed proved more nuanced than UnsafeInvariant.

Anyways..

I think this discussion belongs in https://github.com/rust-lang/unsafe-code-guidelines where at least some people think formally about the unsafe code boundary.

[jhpratt]

In other words, we always convert regular code invariants into memory safety assurance, but these regular code invariants have exactly the same risks as other regular code, including their own memory safety concerns. This RFC confuses the memory safety consumed in maintaining the regular code invariant with the actually unsafe options the code requires.

This is your fundamental misunderstanding.

Other code in Vec relies on the invariants of Vec.len in ways that leads to undefined behavior if the invariants are broken. Fields like Vec.len are not "regular code invariants" — they are tightly coupled to whether the code is sound or not. It is a soundness invariant. You cannot possibly claim otherwise.

given the same cautions can be maintained in other ways, like by variable naming, etc.

Frankly, it's thinking like this that led to the creation of Rust. Thread safety can be maintained if everyone is super careful, but we all know how that works out. Likewise with a million other things. Programmers can not be relied upon to do the right thing. We have to force them to do it by leveraging the compiler wherever possible.

I think this discussion belongs in https://github.com/rust-lang/unsafe-code-guidelines where at least some people think formally about the unsafe code boundary.

I have no idea why you think the discussion belongs there, particularly as you're the one that initiated it here.

[burdges]

Other code in Vec relies on the invariants of Vec.len in ways that leads to undefined behavior if the invariants are broken.

Yes, but this does not make altering Vec.len within the Vec module an unsafe operation. That's not how safety works.

What happens if I've unsafe code which relies upon an invariant between the values in a slice, so your unsafe field is a &[*mut Foo] or &[usize]? I want memory safety around the regular code invariants in how I maintain this slice. Yes, those regular code invariants control memory safety in how the module get used, hence privacy. It's clearly worse if I've many more unsafe blocks merely to access this slice, which now might intermix with some real unsafe code blocks for the *mut Foos or even violate slice invariants.

We often have this "catch your tail" phenomenon in formalalisms.

Frankly, it's thinking like this that led to the creation of Rust

No. There is a formal model from the rustbelt project about how unsafe works, which gets discussed in the unsafe code guidlines repo. We should only expand what falls under unsafe code if the formal model says so. This RFC confuses those really important formal models with mere "read this" markers.

In brief, you don't have a formal conception of when unsafe types should be used. It's unlikely one exists. That makes this change a regression towards the C days.

[jhpratt]

Yes, but this does not make altering Vec.len within the Vec module an unsafe operation. That's not how safety works.

It actually is. If something can result in undefined behavior, it is unsafe. Where that happens is wholly irrelevant. You're using circular logic regarding the UCG, which is by definition written about Rust as it currently is. You seem to be claiming that library undefined behavior doesn't exist, which is bizarre.

I'll leave it at that as there's clearly no convincing you that the mere concept of an unsafe field has merit. I won't be responding further to anything along those lines.

[tgross35]

Could you add another motivating example to the RFC? It seems like the vec.len example isn't super convincing because changing anything in Vec/RawVec breaks the model down and is "unsafe", by definition: that is protected via the private/public module interface. This makes me think that the resolution would be something like unsafe modules or #[unsafe] types rather than fine grained per-field control, so I'm curious to see what motivates this specific solution.

[jhpratt]

@tgross35 I linked this earlier in the thread. Is that sufficient for you? It demonstrates that field-level safety is appropriate, as some of the fields are perfectly safe to set as they have no interaction with any other field. Yet at the same time, all fields are one logical unit that should not be split into separate structs.

[tgross35]

Thank you for the link, I meant specifically to add an example to the RFC document (which perhaps you planned to do anyway).

Even with that example, it does seem to me that it would be more correct to nest flags into a separate type with the other items it has the invariant with. To me, it illustrates the concept better; a field alone is never unsafe but rather it is unsafe in the context of other fields, and it seems like this is a suitable level for abstraction to a separate type. Also:

• If a struct of 12 fields has 8 marked unsafe, it isn't immediately clear how they may be related. Are they all tied together via an invariant? Are 6 tied together and the remaining 2 associated somehow? This becomes immediately clear by separating things that are related into separate types, and I think that behavior should be encouraged
• For the example Parsed struct, if sufficiently many fields are related to flags then it seems like it would make sense to mark the entire struct #[upholds_unsafe_invariant] or something like that. Maybe a few fields aren't related to the invariant, but commenting // SAFETY: not related to the invariant is easy enough to make this clear (and forces you to make the change if they do become related to the invariant for some reason)

Niche optimization was mentioned as well as a reason for not wanting to split off types, but I think this is more applicable to all separations of logic, and not just those with safety concerns. I don't wish to derail this conversation, but the thought has crossed my mind before about how Rust could potentially use a #[flatten] attribute for nested struct fields that tells the compiler to merge the child struct's fields into the parent's and rearrange for best possible size. (yes - possible method duplication, but this would be meant for one-off structs).

I think that investigating something like that may be more widely useful than using niche optimization as a justification for why a single flat struct is the correct solution. (edit: I brought this up for some discussion on Zulip https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/.60.23.5Bflatten.5D.60.20struct.20field.20attribute)

[burdges]

As you're replying by email, I'll un-edit my above comment and paste it here.

There exist alternatives that avoid "unsafe creap" while still doing enforcement in rustc:

Idea 1. Explicit rolls

An unsafe field can only be written from special unsafe(TypeName) { .. } blocks, but these "roll focused" unsafe blocks disallow regular unsafe operations. A regular unsafe { .. } block cannot directly write to unsafe fields.

We'd permit unsafe(TypeName) fn too, even if TypeName is a ZST or lacks unsafe fields, so unsafe fns can be roll focused too, with their rolls again being multuly exclusive to regular unsafe blocks.

In future, we could add unsafe(TypeName1,TypeName2,..,TypeNamen,number) { .. } blocks that permit exactly number regular unsafe operations, and any number of roll focused unsafe operations for TypeName1,TypeName2,..,TypeNamen.

In this, TypeName is the identifier after struct or union, not a fully qualified type. Alternatively, rolls could be specified like unsafe(module) { .. } where module is the module where TypeName gets declared.

In this way, you can mark as much code as you like unsafe for rolls you create, without fearing that regular unsafe fns get invoked more often.

Idea 2. Implicit rolls

If people dislike the verbosity of unsafe(TypeName) then rustc could simply create an "unsafe report" that infers & documents rolls:

module_path::fn_name1 : 
  unsafe block 1 : 2 primitive unsafe operations

module_path::fn_name2 : 
  unsafe block 1 : field writes to TypeName
  unsafe block 2 : std::vec::Vec::set_len

You could lint against unsafe blocks having multiple inferred rolls too.

[RalfJung]

That sounds quite similar to ideas around more explicitly discharging proof obligations, which have been floating around for a while. As I already said in my previous message: this is not a new problem. Calling more than one unsafe fn in an unsafe block happens regularly. I don't think unsafe fields make this so much worse that we have to block them on a more fine-grained unsafety tracking mechanism.

[burdges]

Alright, thanks for clarifying everything.

[nikomatsakis]

I concur with Ralf and Mario that the rule-of-thumb that "any function that is declared unsafe ought to have an unsafe op in its body" is a very reasonable one. As an unsafe code author I rely on unsafe warnings to flag potentially dangerous operations for me -- and indeed to remind me where such operations live in the code when re-reading after an absence.

…

On Fri, Jun 7, 2024, at 3:06 AM, Jeff Burdges wrote: > That "linter" may as well be the programmer themselves, trying to figure out what is unsafe an where, placing SAFETY comments. > Alright, but adding unsafe blocks incurs some cost: You must check that only the desired `unsafe fn`s get invoked. If some type has both `Vec` and an unsafe field, then you've lost any extra warnings when calling `Vec::set_let` while accessing the unsafe field. If the linter is the programmer, then it's more constrained to simply append `_unsafe` to the field name. This avoids "unsafe creep" where more code becomes unsafe, while exposing when unsafe fields get accessed. An alternative that'd avoid "unsafe creap" while still doing enforcement in rustc: An unsafe field can only be written from special `unsafe(TypeName) { .. }` blocks, but these "roll focused" unsafe blocks disallow regular unsafe operations. A regular `unsafe { .. }` block cannot directly write to unsafe fields. We'd permit `unsafe(TypeName) fn` too, even if `TypeName` is a ZST or lacks unsafe fields, so `unsafe fn`s can be roll focused too, with their rolls again being multuly exclusive to regular unsafe blocks. In future, we could add `unsafe(TypeName1,TypeName2,..,TypeNamen,number) { .. }` blocks that permit exactly `number` regular unsafe operations, and any number of roll focused unsafe operations for `TypeName1,TypeName2,..,TypeNamen`. In this, `TypeName` is the identifier after `struct` or `union`, not a fully qualified type. Alternatively, rolls could be specified like `unsafe(module) { .. }` where `module` is the module where `TypeName` gets declared. In this way, you can mark as much code as you like `unsafe` for rolls you create, without fearing that regular `unssafe fn`s get invoked more often. — Reply to this email directly, view it on GitHub <#3458 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABF4ZQHAIMCJT4O3ZX5VGLZGFLXXAVCNFSM6AAAAAA2ILY666VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNJUGIZDIMZQG4>. You are receiving this because you commented.Message ID: ***@***.***>

[ssokolow]

any function that is declared unsafe ought to have an unsafe op in its body

Putting it that way does make it sound more overtly appealing. If nothing else, it should discourage people trying to diverge from the "unsafe = memory-unsafe" definition and abuse it for other purposes.

(EDIT: Sorry to any people following via e-mail for accidentally hitting the wrong Enter key combo prematurely.)

[scottmcm]

I concur with Ralf and Mario that the rule-of-thumb that "any function that is declared unsafe ought to have an unsafe op in its body" is a very reasonable one.

IIRC there's even been academic papers that count "unsafe functions that don't do anything unsafe", despite that being a mostly-meaningless metric today (as demonstrated by Vec::set_len).

So agreed, big fan of an obvious "this is why it's unsafe" you can point at locally inside the unsafe fn.

[CodesInChaos]

Should implementing/deriving Copy for a type which contains unsafe fields be unsafe? While it's technically just a marker trait, it behaves quite similar to an initializer. Implementing Copy for Vec would definitely be unsound.

I think types containing unsafe fields should require unsafe impl Copy for X.

[idanarye]

Copy can already be used to violate safety because you can use it on structs that contain raw pointers. For that reason, BTW, I think Copy should be unsafe in general (although #[derive]ing it is okay because it can only be done in the same module)

[kennytm]

@CodesInChaos There is no Copy without Clone. The RFC specifies

An unsafe field may only be mutated or initialized in an unsafe context. Failure to do so is a compile error.

Meaning the Clone implementation can't be safely derived in the first place.

So you need to implement Clone yourself in order to implement Copy. The canonical implementation is:

impl Clone for X {
    fn clone(&self) -> Self {
        *self
    }
}
impl Copy for X {}

I think copying a new value Self out of a place of type Self should still be considered a kind of initialization and raise the unsafe error by default. Assuming the unsafe {} block acts the same as {} block regarding copying, the canonical implementation should be forced to become instead:

impl Clone for X {
    fn clone(&self) -> Self {
        // SAFETY: we determined the unsafe field is safe to copy.
        unsafe { *self }
    }
}
impl Copy for X {}

But then elsewhere let y = *x; should not be considered unsafe! This created a paradox that the Clone::clone impl does not require the unsafe {} block actually 🤔

It seems the best is just prohibit Copy impl entirely when it contains any unsafe field 🤷 (or require unsafe impl Copy for X even if Copy itself is not an unsafe trait).

[CodesInChaos]

@kennytm I don't think Clone has any relevance, since you could always have a clone implementation that's inconsistent with Copy (e.g. by unconditionally panicking).

Implementing Copy is the relevant point of unsafety, and hence should require the unsafe keyword. Prohibiting it entirely is too restrictive, since most types which don't own heap allocated data can be copied, even if they have unsafe invariants.

@idanarye Copying raw pointers is safe. It's only safety critical if you use them somewhere where they're assumed valid. Since such assumptions come with an safety critical invariant, they should be unsafe fields. So requiring an unsafe impl Copy for types containing unsafe fields should fix this.

[jhpratt]

So…I'm conflicted. Yes, initialization is included, but it is already known that the data contained in the struct is valid. The situation that is motivation for this RFC is when a field has its own invariant and/or relies on another field in the struct. In my head, I view the unsafe bit being unchecked assignment to a field. But Clone and Copy are not that, as the assignment was already checked (when it was previously constructed/assigned). The #[derive(Clone)] macro could thus trivially wrap the the whole expression in unsafe {} when it encounters an unsafe field.

Is there a plausible situation where the safety of a field would rely on external factors to a struct? If not, I'll need to figure out a way to rework the RFC to make Clone/Copy acceptable.

[RalfJung]

it is already known that the data contained in the struct is valid

We're talking about safety invariants here, not validity invariants.
And safety invariants in Rust, in general, express ownership -- just because they hold once, doesn't mean they hold twice. A safety invariant could be "I exclusively own the right to access this region of memory": just because this is the case, doesn't mean you can make a copy and have two values where the safety invariant holds for both of them.

[jhpratt]

That is true. In any situation, I feel that there must be a way to implement Clone and Copy for types containing unsafe fields. What form that takes I'm not certain, as the traits are (of course) safe on their own.

[CodesInChaos]

I think requiring unsafe impl Copy for types with unsafe fields would be enough. You'd have to implement Copy and Clone manually, instead of deriving them (the generated code would fail, because it misses the necessary unsafes). That's a bit verbose (perhaps 10 lines of code), but that's no big deal IMO.

Consider Vec, the fields are all copyable and copying ensures consistency between allocation size, len and capacity. But it doesn't ensure that the allocation is owned by a single vec, and hence needs a custom Clone and a disabled Copy.

[jhpratt]

The problem is that (last I checked) you can't unsafe impl a safe trait. I suppose this could be lifted for types that contain unsafe fields, but then removing an unsafe field is a very unexpected breaking change.

[CodesInChaos]

The problem is that (last I checked) you can't unsafe impl a safe trait.

Yes, that rule would need a special case for Copy on types with unsafe fields.

but then removing an unsafe field is a very unexpected breaking change.

Ibelieve unsafe impl Copy must be in the same crate as the type containing the unsafe fields, due to orphan rules. So you can adjust both at the same time, and it would not be a breaking change.

[jhpratt]

There are more traits that just Copy — this issue applies to all traits.

[CodesInChaos]

There are more traits that just Copy — this issue applies to all traits.

I think it's limited to Copy. Copy is conceptually an unsafe marker trait, but the compiler has some magic that prevents you from implementing it at all for types which contain non copyable fields, making safe to implement. The problem with Copy only arises due to compiler giving it special treatment, so more special treatment to fix that seems appropriate.

The only similar issue I see are unsafe auto-traits (Send, Sync), but I didn't think about those enough to figure out if disabling the auto implementation of those for types with unsafe fields makes sense.

---

edit: Thinking about it a bit more, it applies to traits which meet all three of these conditions:

• are an unsafe trait (which Copy effectively is)
• can be implemented in safe code, via derive, auto-traits (Send, Sync) or special compiler rules (Copy)
• the automatic implementation accepts types containing unsafe fields

[kennytm]

I don't think Clone has any relevance, since you could always have a clone implementation that's inconsistent with Copy (e.g. by unconditionally panicking).

Okay the Copy documentation did not prohibit Clone not performing exactly a bit-copy, but with a Copy type I can't think of any case why you want to implement clone() to be anything other than a bit-copy operation.

Prohibiting it entirely is too restrictive, since most types which don't own heap allocated data can be copied, even if they have unsafe invariants.

You can't impl Copy types which has non-Copy fields already

#[derive(Clone)]
struct MyRange(std::ops::Range<u32>);

impl Copy for MyRange {} 
//~^ error[E0204]: the trait `Copy` cannot be implemented for this type

IMO "prohibiting" while restrictive at least has a precedent. While unsafe impl Copy expands the scope of unsafe impl against E0199 (implementing the trait `Copy` is not unsafe). So maybe this goes into an Unresolved Question.

[idanarye]

@CodesInChaos

Copying raw pointers is safe. It's only safety critical if you use them somewhere where they're assumed valid. Since such assumptions come with an safety critical invariant, they should be unsafe fields. So requiring an unsafe impl Copy for types containing unsafe fields should fix this.

Say I have a MyBox struct - an implementation of Box that uses a raw pointer directly. The fields are private - the module where MyBox is defined is that the only place where you can touch them - and thus the rule is that this module is responsible for maintaining the safety variants of the pointer within (and if it exposes any API that may violate it - it should mark it as unsafe)

Naturally, I should not impl Copy for MyBox because that would break the uniqueness invariant, and more specifically - when I drop one copy and the memory is released I'll still have the other copy with a dangling pointer.

But I can impl Copy for MyBox in a different module of the same crate. That module does not have access to MyBox's private fields, and does not bear the responsibility of maintaining its safety invariant - but it can still violate them without even using the unsafe keyword.

---

This problem I described already applied to current Rust, without unsafe fields. My point here is that this is basically the same problem as implementing Copy for structs with unsafe fields - that the bit copy can violate the safety invariant. We should either live with that (because we already live with that) or find a general solution for it - one that solves the currently possible cases and not just the ones introduced by unsafe fields.

[RalfJung]

But I can impl Copy for MyBox in a different module of the same crate.

Oh wow, I had never realized that... but it makes sense, the trait system cares about coherence which works per-crate, and there's no visibility question arising here.

Is there any chance we can fix impl Copy so that it is only allowed inside modules where all fields of the type are accessible, i.e., where you could have written the obvious Clone impl that copies all fields? That's actually entirely independent of this discussion and of unsafe Copy discussions, so I'll open a new issue.

[CodesInChaos]

I think in a green field design, Copy should have been an unsafe marker trait, with a safe derive.

@kennytm Types which have unsafe invariants but don't own heap allocated data can often can be Copyable. Preventing those from using unsafe fields seems undesirable.

@idanarye One of the main selling points of unsafe fields is that it avoids relying on visibility for safety. It reduces the scope to unsafe code, and code that unsafe code relies on. See withoutboats's post.

[jswrenn]

Hey @rust-lang/lang, I'd like to nominate this RFC for experimental implementation. We've had lots of recent discussion on Zulip culminating in a living design doc and a proposed project goal. At this point, we'd most benefit from upstreaming an experimental implementation that we can all play with and get a feel for.

[joshtriplett]

Speaking for myself: enthusiastic support, I'd love to see this experiment.