Allow the `unsafe` qualifier on struct fields

[bstrie]

No description provided.

[bstrie]

cc @cmr
cc @eddyb

[cmr]

+1

cc @Tobba

[huonw]

Should initialising an unsafe field be unsafe too?

[bstrie]

@huonw I do think it's worth having a discussion over what actions, exactly, should be restricted, rather than just tacitly accepting a blanket restriction on all accesses. As for your question itself, I'm not sure, though off the top of my head I see no reason to make it so.

Similarly I personally see no reason why reading an unsafe field ought to require an unsafe block, but the straw poll on IRC seemed to favor the "make everything require unsafe" approach (with notable dissent from @eddyb).

[cmr]

Initialization seems like it shouldn't be unsafe.

On Mon, May 19, 2014 at 12:40 AM, Ben Striegel notifications@github.comwrote:

@huonw https://github.com/huonw I do think it's worth having a
discussion over what actions, exactly, should be restricted, rather than
just tacitly accepting a blanket restriction on all accesses. As for your
question itself, I'm not sure, though off the top of my head I see no
reason to make it so.

Similarly I personally see no reason why reading an unsafe field ought to
require an unsafe block, but the straw poll on IRC seemed to favor the
"make everything require unsafe" approach (with notable dissent from
@eddyb https://github.com/eddyb).

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/80#issuecomment-43473669
.

http://octayn.net/

[huonw]

I would think the only reason to use an unsafe field is if there is a nontrivial invariant between the field and something else which cannot be easily enforced directly in types (e.g. the capacity field of a Vec needs to be the space stored by the pointer field, and similarly the length field needs to be the number of valid elements in the vector).

Hence, in my mind, anything that sets a value for that field should be unsafe (as it's possible to violate the invariant at that point). E.g.

Vec {
    capacity: 0,
    length: 10,
    pointer: ptr::null()
}

I agree that reading the field doesn't necessarily need to be unsafe (since, in almost all cases, reading can't change the invariants... although maybe it can for "memory-mapped" hardware, where reading/writing from specific places in memory has external side-effects).

---

However, I wonder if most of the advantages of this can be covered by something like (i.e. avoid extending the language):

// unfortunately, this name overlaps with `core::ty::Unsafe`
struct Unsafe<T> {
    value: T
}
impl<T> Unsafe<T> {
     unsafe fn new(value: T) -> Unsafe<T> { 
          Unsafe { value: T }
     }
     fn move(self) -> T { self.value }

     unsafe fn get_mut<'a>(&'a mut self) -> &'a mut T { &mut self.value }
}

impl<T> Deref<T> for Unsafe<T> {
    fn deref<'a>(&'a self) -> &'a T { &self.value }
}

(Using my scheme of initialisation being unsafe, and @bstrie's of reading being safe.)

[Thiez]

This doesn't seem very useful to me. Within a module I would expect the authors to know what they're doing, and the unit-tests to save them when they do not. For other users, you could simply introduce getters and setters, and functions/methods can already be marked unsafe.

[Valloric]

Agreed with @Thiez. This doesn't seem like it needs language-level support.

[lilyball]

I agree with @Thiez as well. Not only that, but having the unsafe marker would affect the implementation of the struct (that knows how to properly maintain safety), requiring it to start using unsafe all over the place for code that is not in fact unsafe.

As a side note, I believe this would also prevent deriving any traits for the struct.

[huonw]

@kballard I would think it's very rare that types with unsafe fields can have any traits correctly implemented via #[deriving]. (E.g. Vec and HashMap certainly can't.)

[lilyball]

@huonw path::posix::Path is a good example. It looks like this:

#[deriving(Clone)]
pub struct Path {
    repr: Vec<u8>,
    sepidx: Option<uint>
}

It would be a good candidate for unsafe fields, because it's perfectly fine to read these fields, but they're private because updating them needs to obey certain invariants. As such, the type can derive Clone just fine. And it could theoretically define Eq as well, although I chose to implement it manually because the sepidx field should be skipped for that. If we come up with a syntax for skipping fields during deriving, as suggested in #14268, then Eq, Hash, and possibly other traits could be reasonably derived for Path as well.

That's not to say I want to make the Path fields public. I don't think there's any real utility gained from that. I'm just using this as a demonstration of a type whose fields can be compatible with derived traits, yet don't want to allow clients to modify them because they must obey invariants.

[nmsmith]

I don't think it should be necessary to wrap a struct that has (what would be) unsafe fields. If a type has unsafe fields, they should be marked as so. Forgetting the implications on mutation, at the very least it makes your code self-documenting. Such fields are inherently unsafe to play with.

As a bonus that hasn't been mentioned yet, marking fields as unsafe means that (correctly-written) unsafe blocks can have (barring other loopholes) truly safe interfaces, even within a module - Vec's len would not be able to be changed outside of unsafe blocks and unsafe functions. That's important, because it isn't safe to change a vector's length independent of its other fields. We're protecting the library implementers (who are only human) as much as the users.

[nmsmith]

I also want to draw attention to a function from Vec:

pub unsafe fn from_raw_parts(length: uint, capacity: uint,
                             ptr: *mut T) -> Vec<T> {
    Vec { len: length, cap: capacity, ptr: ptr }
}

Note that the function is manually annotated as unsafe. To the compiler, there are no unsafe operations being performed here, and so it would allow the implementer to leave this as a safe function. However, the implementer has correctly noted that constructing a new vector in this way is not a safe thing to do.

If we have unsafe fields, then the compiler will reject this function unless it is labelled as unsafe. It will catch the mistake the implementer could make here. And all programmers are human, so this is definitely a mistake worth catching.

There are probably more subtle examples where a function without any unsafe blocks is manipulating fields in an unsafe way.

[pnkfelix]

Meta note on RFC writing style: I don't understand how @bstrie expects the opening example from the Detailed Design to be re-written after this feature is added. Is just idx unsafe, or both idx and data ? I assume the latter. But it should be explicit.

Basically, if you are going to drive presentation via an example, please finish each example, and no switching midway.

[brson]

Per https://github.com/rust-lang/rust/wiki/Meeting-weekly-2014-06-17#rfc-pr-80-unsafe-fields-httpsgithubcomrust-langrfcspull80-, let's not do this right now. While there is some agreement that unsafe fields are desirable, the Unsafe type largely covers this use case and not many want to add this to the set of pending 1.0 language changes.

[pnkfelix]

We probably should have marked this as "postponed", since the phrase used when we closed it was "let's not do this right now" :)

So, adding postponed label.

[ticki]

It seems like the right time to reopen this.