Make accessing Unsafe<T> fields unsafe

[arielb1]

Pretty: https://github.com/arielb1/rfcs/blob/unsafe-unsafe-access/active/0000-unsafe-unsafe-access.md

[lilyball]

This RFC very specifically only makes the fields of the Unsafe item unsafe. Is there any reason why this is better than allowing arbitrary fields to be marked as unsafe, thus requiring the unsafe modifier to access them? This would look like

pub struct Unsafe<T> {
    pub unsafe value: T,
    pub marker1: marker::InvariantType<T>
}

This approach avoids special-casing Unsafe, and also provides this functionality to any other type that might benefit (such as the length/capacity fields on Vec).

[arielb1]

The reason I prefer this to be Unsafe-specific is because it shouldn't be the right thing anywhere else - e.g. if you made the fields of Vec unsafe that would prevent you from modifying an existing Vec, but would not help you much because you could still conjure a bogus Vec out of thin air and swap it into place (of course, if you have an &mut Unsafe<T>, I'm fine with you accessing its fields, and letting people conjure Unsafe<T>s is exactly what I'm trying to do here).

[lilyball]

you could still conjure a bogus Vec out of thin air

Hrm. I suppose you have a point, although anyone doing that would probably know they're doing something very silly anyway.

What if an unsafe field also required unsafe to construct the literal value? That is not something that Unsafe particularly wants, but I don't think there's any harm in requiring unsafe {} to make an Unsafe literal.

[arielb1]

@kballard

The entire point here is to allow initialization without unsafe {}

[lilyball]

@arielb1 Why is that useful? End-users of Rust APIs do not construct Unsafe values by hand. Constructing an Unsafe value from a struct literal is only used to provide the statics like INIT_ATOMIC_UINT that are themselves used to initialize other statics. Initializing another static from INIT_ATOMIC_UINT will not require unsafe {} (as it does not reference the value field), and the definition of INIT_ATOMIC_UINT itself should have no problem using unsafe {}.

[arielb1]

@kballard

Dereferencing Unsafe is semantically special because it can trigger undefined behaviour (while dereferencing any other field can't trigger UB by itself).

[lilyball]

@arielb1 You can't dereference Unsafe. It doesn't implement Deref. So I don't know what you mean by that.

Assuming you mean "accessing a field of", I'm still not sure how this is relevant to the use of unsafe {} when constructing a struct literal for Unsafe.

[arielb1]

@kballard

I meant "taking a reference to a field of". It is the only way you can have undefined behaviour outside of an unsafe block, which is weird.

[lilyball]

@arielb1 Well, taking a reference in this case is largely equivalent to creating a &T or &mut T from a *const T or *mut T. In fact, Unsafe<T> itself is pretty much the same thing as working with *mut T except without the indirection.

[arielb1]

@kballard

Sure. And therefore it should be unsafe (like dereferencing a regular *mut T).

[lilyball]

@arielb1 Yes, accessing the value field should be unsafe. I have never disagreed on that point.

My only point of contention here is whether it makes sense to special-case field access of Unsafe in the compiler, or whether we should just have the more general rule of allowing the unsafe modifier on a field declaration. The only real difference as far as Unsafe is concerned is the latter would presumably require you to use unsafe {} when constructing a struct literal but the former, as you have defined, would not. I argue that the need for using unsafe {} on the struct literal is not a problem.

[arielb1]

@kballard

Accessing fields that aren't the value field of an Unsafe<T> can't cause undefined behaviour (directly), so they shouldn't be able to be marked unsafe.

[nmsmith]

Just noticed one point worth arguing:

The reason I prefer this to be Unsafe-specific is because it shouldn't be the right thing anywhere else - e.g. if you made the fields of Vec unsafe that would prevent you from modifying an existing Vec, but would not help you much because you could still conjure a bogus Vec out of thin air and swap it into place

That's not true if unsafe fields are unsafe to initialize just as they are unsafe to modify. You can (and should) enforce that construction of a Vec take place in an unsafe block as well.

[thestinger]

I definitely don't think this should be specific to Unsafe. The purpose of Unsafe is to mark cases where fields without inherited mutability are mutated. It will not be possible to use this for Vec if it requires Unsafe because it would result in missed optimizations. I do think having unsafe fields is important in order to allow writing types like Vec<T> without using visibility to contain unsafety rather than unsafe blocks.

[nmsmith]

I agree with all of what @thestinger just said. Unsafe fields are valuable in many situations, and hopefully will be implemented as their own thing at some point. But that's a point for discussion here: #80

Perhaps Unsafe could be a motivation for seeing them sooner rather than later. I also discuss the need for them in my own RFC #193

[arielb1]

@thestinger

The issue here is that accessing Unsafe can result in undefined behaviour while accessing other fields can't - i.e. other unsafe fields should probably be readable safely, while here they can't/

Thinking about it more, actual unsafe fields do seem like a good addition (but I think that is less important than this change, and I have no problem with it being delayed past 1.0, etc.) – privacy is subtly different from unsafeness, the former being "don't access this as it may not be here next version", while the latter means "be careful while touching this, you may break something" – logically, Vec<T> makes no attempt to hide its fields, as there's no abstraction problem with careful users accessing them, while an hash table, balanced binary tree, or BufferedReader may prefer to hide its fields, as applications should not depend on the specifics of their implementation.

However, this orthogonality isn't really related to the Unsafe<T> issue, where the issue is that safe reads of a field can cause undefined behaviour by data race.

As a summary, there are 3 different reasons for controlled field access:

Privacy – the field is an implementation detail, to ensure the consequences of changing its semantics are limited.
Unsafety – the field is involved in an invariant, which a violation of could cause major trouble (e.g. undefined behaviour), to limit and mark the code that can violate it.
Unsafe<T> – the field may be borrowed mutably, and accessing it may invoke undefined behaviour by data race.

These seem different enough (well you can say there's a "no data race" invariant, but that is a rather weird one, being breakable by reads).

[thestinger]

@arielb1: I'm fully aware that unsafe and Unsafe<T> are totally distinct cases, please read my comment again. The Unsafe<T> type already makes use of unsafe methods, new language features are not required for it. Your comment is directed at the wrong person.

[arielb1]

@thestinger

You can read the field of an Unsafe<T> without calling an unsafe function (see rust rust-lang/rust#15920), causing UB - and this makes the issue different from e.g. Vec<T>, as merely reading the length, or even the unsafe content pointer, is harmless.

[brson]

Changing the way unsafe fields work on Rust has been discussed a few times, there are several potential designs, and the need is not pressing. With all the other changes in the pipeline, this is not something we want to do now. Thank you.