Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RFC: Allow safe access to some static mut vars #177

Closed
wants to merge 1 commit into from

Conversation

alexcrichton
Copy link
Member

@alexcrichton
Copy link
Member Author

cc rust-lang/rust#14862

@lilyball
Copy link
Contributor

👎

As described in great detail in the referenced issue, this is not even remotely safe. It's also a complete mis-use of the Share kind. The following line from the RFC is incorrect:

The original reason for restricting access to a static mut was to prevent data races between tasks, but from the definition of Share it can be seen that safely allowing shared borrows of static mut variables can never lead to data races.

The definition of Share does not in any way mean that borrows of static mut variables cannot lead to data races. The only reason that Share is able to guarantee that &T is thread-safe is because of the property that a &T can never alias with a &mut T (for the same underlying value). This property is explicitly broken for shared mut, and the fact that this property does not hold is why shared mut is inherently unsafe and must remain so.

The counterargument is that since a &mut borrow still requires unsafe, the code that does the writing can ensure safety. But that argument does not pass muster. The mere ability for a &mut borrow to be taken (even though it requires unsafe) without borrowck being aware makes the act of taking a & borrow inherently unsafe. The related argument that a static mut can have its privacy restricted such that all the code that can access it does so safely also is not valid. Not only do we have no precedence for defining safety in any way relating to access control, but even with a private static mut, the unsafety of taking a &mut borrow cannot be contained within an unsafe {} block.

The general rule for unsafe is that an unsafe {} block must contain all unsafety within it, and not leak any of the unsafety outside. If the unsafe {} block enables code that is considered safe to violate memory safety, then the unsafe {} block did not contain the unsafety, and the code is, by definition, buggy.

Given this definition, since the mere ability to take a &mut borrow causes the & borrows to be unsafe, this means that the mere existence of a &mut borrow on a static mut requires the containing unsafe {} block to expand until it covers the entire scope of code that has visibility to the static mut. At that point, all code outside the unsafe {} block cannot take a & borrow on the static mut, and therefore the unsafety is contained. But besides the impracticality of this, this also means that you cannot in fact take a & borrow on a static mut except within an unsafe {} block, thus making the entire point of this RFC (to allow such borrows without unsafe {}) entirely moot.


The only situation in which this RFC actually affects things is when the static mut contains some Unsafe<T> data. As long as you ensure you never take a &mut borrow of the static mut, then you can safely take & borrows without unsafe {}, which means you can then safely call the methods that use the Unsafe<T> for internal mutability. That is in fact the primary motivation of this RFC. However, the fact that a single &mut borrow instantly makes all & borrows unsafe makes this behavior too dangerous to allow, as it will invariably result in Rust code that is unsafe in ways the code author is not aware of. Furthermore, it makes it impossible to have a public static mut of a Share type. Arguably, static muts shouldn't be public, but that's not something a language feature should cause to be a fundamentally unsafe operation.

I believe that there are alternative approaches that we can use to solve the static safe Unsafe<T> issue without opening this serious safety hole.

@erickt
Copy link

erickt commented Jul 23, 2014

@kballard: do you have an alternative you prefer?

I also think we should also consider putting static mut behind a feature flag. It feels kind of wart-y, and since CTFE may someday alleviate our need for the Once pattern, which would also cut down on some usages of static mut.

@alexcrichton
Copy link
Member Author

While CTFE will allow for nicer initializers, I don't think it will help with requiring unsafe or not to access the static variable.

@huonw
Copy link
Member

huonw commented Jul 23, 2014

Not only do we have no precedence for defining safety in any way relating to access control

The safe functions in the Vec API can only be considered safe because the fields are not externally accessible.


Requiring unsafe access to these types unnecessarily introduces `unsafe` blocks
in otherwise safe programs, and have the risk of leading to further unsafe
behaviors if it's unclear what exactly in the block is unsafe.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Has there been an RFC or discussion on other ways to achieve this? We only have three sources of unsafety, could we mark up the unsafe {} with which we are intending on using?

unsafe<RawDeref + MutStatic + UnsafeFn> { }

Sorry it is a little off topic, I'm just not 100% across all the previous discussions and this seems like a similar solution.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This has come up in the past before, although there haven't been any concrete proposals in the area, more of just ideas. For now though, the RFC is written with the assumption that this does not exist (but it would be quite nice!)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, once rust-lang/rust#15701 is implemented it could be a lint, interpreting things like #[behaviour(raw_deref, mut_static)] unsafe { ... }.

@alexcrichton
Copy link
Member Author

this is not even remotely safe

I would imagine that the large portion of safety that most Rust users consider is only safe code considered as a whole. Can you concretely explain how you believe this proposal is unsafe assuming there is no unsafe code?

@erickt
Copy link

erickt commented Jul 23, 2014

@alexcrichton: that's why I said "some" usages of static mut :)

Two thoughts off the top of my head without thinking about it too deeply:

  • How much extra complexity would static const add? If it's not that much it might be nice to avoid the unsafety @kballard is referring to.
  • How about the opposite, where we get rid of static mut and replace it with this hypothetical static const. Could static const foo: Unsafe<T> = ... somehow cover those use cases?

@alexcrichton
Copy link
Member Author

How much extra complexity would static const add?

I view the addition of a new kind of static to the language as more than it's worth given the problem that this RFC is trying to solve. The naming would also have to be quite different because otherwise we'll have a system where static cannot change but static const can (seems backwards?)

How about the opposite, where we get rid of static mut and replace it with this hypothetical static const

This is certainly plausible! I personally believe that static mut is quite useful out of just Share types, and it seems odd to hamstring the entire language for what may be a common use case in some applications. I think there were more persuasive arguments in the meeting, but I can't recall them right now.

@bharrisau
Copy link

Is there a potential that the static mut is uninitialized?

If initialization isn't a problem, I don't see why reading a static mut FOO: &T would be an issue if T is Share. If I understand things correctly, there is a guarantee that there is no &mut T in existence if there is an &T?

Storing static mut FOO: T doesn't guarantee that, as some unsafe code can take an &mut T and there is no way to know that there are no &T out there.

@alexcrichton
Copy link
Member Author

Is there a potential that the static mut is uninitialized?

Thankfully no, all static variables are required to have initializers, and initializers are always just bit patterns so there are no statics which are ever uninitialized at any time.

If I understand things correctly, there is a guarantee that there is no &mut T in existence if there is an &T?

The borrow checker cannot guarantee this because it doesn't have total knowledge, but this is why unsafe is required to get a &mut borrow.

@arielb1
Copy link
Contributor

arielb1 commented Jul 23, 2014

One issue here is because statics are used in 2 contexts here - there are static VALUES (which are, for example, useful in initializers) and static VARIABLES (assignables, by Harper's nomelecture) which have an identity and can potentially be modified.

Static values are values. changing them makes as much sense as executing 1+1 = 3. Note that sometimes (e.g. mutex static initializers) they need to contain an Unsafe. Being values, they still can't be mutated.

Static assignables are accessible through the program. It seems that a decent way of handling them is to make them behave like a &'static T parameter passed to all functions in the program. Of course given multithreading they need to be Share.

@arielb1
Copy link
Contributor

arielb1 commented Jul 23, 2014

Thinking about it more, the initializers-can't-be-priv problem may be annoying:

If one wants a manually-synchronized static (i.e. a static variable which can't be proven synchronized by the type-system), then with my proposal it would need to be wrapped withing a Share wrapper. Because of the initializer-can't-be-priv issue, you can't have an externally-safe wrapper in a library.

For example

struct MyDSWrapper {
  // intentionally private
  inner: Unsafe<ManuallySynchronizedDS>
}

impl share for MyDSWrapper;

static mut svar: MyDSWrapper = MyDSWrapper
    { inner: ManuallySynchronizedDS { ... }};

pub fn work() {
  let x = external_lock();
  do_something_with(&mut unsafe { *svar.get() });
}

However, an inline wrapper wouldn't be much more dangerous than most other uses of unsafe (i.e. uncareful safe code in the same module can trigger undefined behaviour). I don't think this is a big problem but some don't like it. However, this isn't particularly related to static muts but it is rather the way Rust handles unsafe.

@alexcrichton
Copy link
Member Author

One issue here is because statics are used in 2 contexts here

Rust takes the same approach with static and static mut

Because of the initializer-can't-be-priv issue, you can't have an externally-safe wrapper in a library.

If you take a look at sync::mutex and std::sync::atomics you'll see that there are a good number of static variables which contain Unsafe. Due to placing them in read-only memory, the compiler makes it illegal to take the address of a static with an Unsafe inside of it.

@lilyball
Copy link
Contributor

@huonw

The safe functions in the Vec API can only be considered safe because the fields are not externally accessible.

But the unsafety is limited to unsafe code. The only actual possibility for memory unsafety requires dereferencing the *mut pointer in Vec, which can only be done by unsafe code. Furthermore, the fact that the unsafe code relies on controlling access to the non-unsafe integral fields is not a consequence of a language feature but instead a decision made by the library author.

Furthermore, I think there's a fundamental difference between arguing that access to a struct field can produce unsafety vs static mut producing "safe" unsafety. A struct field that is used to maintain invariants in unsafe code is expected to always be private. A static mut has no such privacy exception; in fact, if this RFC passes, things like Once and the various atomics will be fully expected to be placed in static mut code outside of the library author's hands. And that's dangerous. The library author working on Vec can be assured that they know the invariants and can maintain safety, but expecting clients of an API to use static mut pretty much guarantees someone will screw something up, because we can't expect all Rust users to properly understand how to handle unsafety. This is why we generally try to provide safe APIs whenever possible.

@erickt

do you have an alternative you prefer?

Yes. Make static that contains data that is both Unsafe and Share live in non-constant storage. This allows the Unsafe to remain mutable but doesn't mark the whole static as mutable, thus avoiding the issue of &mut borrows and remaining safe.

The issue here is that this then allows e.g. atomics::INIT_ATOMIC_UINT to be modified, which isn't so great when it's used later. Strictly speaking, I'm not sure if that's a big deal, because it should be copied to all statics that use it as an initializer before it can be modified, and anyone using it in non-static initializers could just be using AtomicUint::new(), but it does seem rather unfortunate. Of course, CTFE would let us remove the static entirely and have everyone use AtomicUint::new(). In the meantime, I would suggest we could have static const which restores the constraint we have today on static, e.g. that you cannot borrow the value if it contains Unsafe. This does mean having 3 static types, but this new one (static const) would really only ever be used by a few library APIs and would not generally affect users of Rust, so I think it's a fair trade.

@lilyball
Copy link
Contributor

@alexcrichton

I would imagine that the large portion of safety that most Rust users consider is only safe code considered as a whole. Can you concretely explain how you believe this proposal is unsafe assuming there is no unsafe code?

You cannot assume there is no unsafe code. That's the problem. You, the library author, do not have control over the static mut that the client must use in order to use your API (e.g. for a Once, or an atomic).

Maybe we should just get rid of static mut entirely. Or rather, adjust it so that all it does is a) store the value in mutable memory, and b) allow Unsafe + Share values to be borrowed. No taking a &mut borrow of the static itself. Anyone who needs the current behavior can then just use Unsafe<T>, e.g. static mut FOO: uint would now be static mut FOO: Unsafe<uint>.

@nikomatsakis
Copy link
Contributor

Regarding safety, I am of the opinion that this proposal does not introduce any new risks for unsafe code. It is always true that unsafe operations may interfere with safe operations (that is, indeed, why we call them unsafe).

The rules that govern static mut are that when one writes to a static mut, one must ensure that this read is ordered with respect to all reads of that static mut. As @kballard notes, these reads may occur in safe code or in unsafe code. In practice, this probably means that privacy must be used to restrict the set of code where the static mut can be accessed. This is par for the course with unsafe code.

The fact that the writes must be ordered with respect to "safe" reads is not unusual and occurs in many situations. For example, transmuting a pointer can easily violate Rust's aliasing rules, which would render otherwise safe code unsafe. The public field in Unsafe is another example. Basically, unsafe code requires controlling scope, which generally requires privacy.

@arielb1
Copy link
Contributor

arielb1 commented Jul 23, 2014

Well the unsafety problem is slightly different from the Vec problem - actually it is the already-present problem with Unsafe (when one can take the value while the Unsafe is borrowed by unsafe code) - but this problem has nothing to do with statics.

From some point of view, it can be said that this is different from the Vec problem - because in the Vec problem the undefined behaviour happens within unsafe code, while here the problem is that safe code can see aliasing & and an &mut. I would prefer that this problem is fixed (mostly because it would make the spurious write semantics simpler) but it (still) has nothing to do with statics.

@nikomatsakis
Copy link
Contributor

That said, I think there is an argument for using const (not static const) to introduce constants. It's just that the argument isn't about safety but rather creating a better analogy between static and let.

Conceivably we could have three kinds of declarations:

const foo: T = ...;
static foo: T = ...;
static mut foo: T = ...;

A const decl would declare a constant, which would mean:

  • No significant address, value automatically inlined into this crate and other crates
  • Deeply immutable
  • It is an rvalue when accessed (hence taking the address copies it onto the stack first)

And then having static (and static mut) be static variables, directly analogous to stack variables declared with let:

  • Significant address, value is not inlined
  • May contain interior mutability
  • All accesses to static mut can be unsafe.

Under this scheme, the #[inline] annotation could not be applied to a static or const. Things like INIT_ATOMIC_UINT would be declared as a const. static would only be used to either prohibit inlining the value or to permit mutation.

The downside is that there are three things, but in a sense those three things exist today, it's just that we use #[inline(never)] to distinguish one of them. Moreover, there would be very few use cases for static mut -- basically just interfacing with C code that uses global variables.

Worth thinking about.

@arielb1
Copy link
Contributor

arielb1 commented Jul 23, 2014

What's the difference in your proposal between static and static mut?

@lilyball
Copy link
Contributor

@nikomatsakis

I like const. This is effectively isomorphic to adding static const, but by ditching the static it does seem semantically like a bit of a different thing, and as you pointed out this includes fixing the weirdness like #[inline(never)]. Presumably it would also fix the need for the attribute to allow/disallow significant address.

This could also be coupled with my earlier suggestion to remove static mut entirely and use Unsafe<T> instead as necessary. This would leave us with just const + static. This does have one significant difference from keeping static mut, which is that Unsafe has a public value field so you could actually bypass the unsafe block and mutate the value directly, but that's an issue that we already have with Unsafe that we've just been ignoring. The existence of Unsafe might be a good reason to resurrect the RFC (#80) requesting the ability to mark struct fields as unsafe. CTFE would also solve this problem (as we could just mark the field as private), but that's a long way off.

@arielb1

The difference is static allows types with interior mutability to retain their mutability, but the static itself is not mutable. Thus any type that does not contain interior mutability is actually deeply immutable.

@arielb1
Copy link
Contributor

arielb1 commented Jul 23, 2014

@kballard

In this case static mut is useless (because it can be emulated by static and a Share wrapper around Unsafe<T> as demonstrated in my previous post).

I'm not sure why unsafe fields would fix this - last time I read it was that the concept of an unsafe field is a field that is unsafe to access or initialize (in the same way a private field is encapsulation-breaking - and therefore dangerous - to access or initialize). What we want here is a field that is safe to initialize but dangerous (potentially UB-invoking) to access.

@lilyball
Copy link
Contributor

@arielb1

Not sure what you mean by a Share wrapper. We already have library constructs that provide various synchronization mechanisms, thus adding Share. You don't need to define a new one. But you're right, static mut is useless in this proposal, because static mut FOO: T is equivalent to static FOO: Unsafe<T>, as I described in my comment. Granted, there is the one difference of the public value field, and that public value field is what unsafe fields would fix (the unsafe field thing is because with Unsafe<T>, you can "safely" access the value field and introduce a data race with an unsafe writer; you're not supposed to do this, you're supposed to use the unsafe get() method instead.

@arielb1
Copy link
Contributor

arielb1 commented Jul 23, 2014

@kballard

By "Share wrapper" I meant a wrapper around Unsafe that is Share. And I think unsafe fields are orthogonal to public-initialize-private-access fields. (For example, a public-initialize-private-access field would be rather bad on Vec).

@glaebhoerl
Copy link
Contributor

@nikomatsakis Under the three-way distinction you propose, would static (not const, not mut) things still be considered to be "known at compile time"? If we gain non-type generic parameters (functions parameterized over a compile-time uint, etc.), would it be allowed to pass a static as such an argument, or only consts? Similarly, would it be permitted to use statics in patterns, or only consts?

(Premature bikeshedding: one small issue with the particular naming suggested here is that it would make *const pointers confusing again, because the two consts would have nothing to do with each other. And in general, this meaning of const wouldn't match the intuitions of people coming from C/C++, who might try to interpret it as the complement of mut, which it is not.)

IINM, it also wouldn't be possible to take an &'static reference to a const, only to a static. This would be another use case for the latter. (And suggests that 'static and const should have different names, which under this proposed naming, is the case.)

@arielb1
Copy link
Contributor

arielb1 commented Jul 23, 2014

@glaebhoerl

I think that it would be preferable to let only consts be "compile-time known" - you can always declare const my_const : T = some_static (again, because statics containing an Unsafe are mutable and therefore are a bad idea in pattern matching etc.)

@lilyball
Copy link
Contributor

I think there's three big reasons why I view static mut as different than, say, a Vec's fields.

The first is that a Vecs fields are private, and have neither need nor expectation of being public. But a static mut, as modified by this RFC, is expected to be public. Or at least, out of control of the library author. Libraries would not vend public static muts themselves, but library APIs would require that the client create and use a static mut themselves. Notably, Once needs this, and atomics will often want to be static muts as well. Yes, the client will have to use an unsafe block to cause a problem, but my worry is that we would be promoting an API design that increases the risk of clients writing buggy, unsafe code.

The second is that the unsafety with Vec fields does not center around safe manipulations of the fields, but around the invariants assumed by the unsafe code. These invariants are not part of the language, but are created by the library author. And the invariants are all wrapped up in a single container type, that encapsulates all the logic for maintaining them. On the other hand, with static mut, the invariant is that any and all access to the static mut is potentially unsafe and requires some form of external synchronization, and this invariant is assumed by the language itself. This is why, today, any code touching it must be marked unsafe. By allowing some form of access to be considered safe, that breaks the assumption of external synchronization. This is simply not appropriate for a language-level feature in a memory-safe language.

Note here, again, that the only reason the Share kind is able to have the meaning it does is because it is able to assume that any mechanism by which a &T can be shared between threads requires appropriate synchronization already, which will prevent the existence of an aliasing &mut T. But the static mut cannot guarantee that a &T will not alias with a &mut T, because it cannot guarantee the external synchronization.

This is why I very strongly favor the suggestions that allow for Unsafe<T> + Share to work without allowing &mut, e.g. my original "let's just let that work in static" and @nikomatsakis newer const proposal.

The third reason is that, with Vec fields, the safety issue is, again, the invariants assumed by the unsafe code. But with static mut as modified by this proposal, the safety issue is that perfectly safe code can unwittingly produce undefined behavior without ever invoking unsafe code (some other thread does need to use unsafe code at some point for this, but the thread that crashes never does). And this isn't just "oops I broke invariants assumed by the library", it's "oops I constructed a language-level value (a &T) that produces UB".

Ultimately, it just feels like library-provided invariants are things that are never expected to be checked by the compiler, whereas &T being safe is basically the most sacred of safety guarantees provided by the language. To the best of my knowledge, there is no way to ever construct a &T that is unsafe without using an unsafe block at some point. The two guarantees provided by &T are

  1. It will never outlive the referenced T, and
  2. It will never alias with a &mut T (more accurately, you are denied access to the &T while an aliasing &mut T exists)

The first one isn't important to this discussion, but the second is. Borrowck enforces it normally, so the only way to violate it is with unsafe. If a &T can be constructed without unsafe then I believe you cannot construct an aliasing &mut T without your code actually being explicitly wrong (either invoking UB via transmuting a &T, or deliberately breaking borrowck by transmuting away the lifetime from a &mut T). If you are able to construct a potentially-aliasing &mut T with code that is not definitively wrong, then you are using some construct that will also require unsafe to construct the potentially-aliasing &T (e.g. a *mut T). And that's just for local data. For shared data, constructing a &T that may possibly alias with another task's &mut T definitely requires unsafe to construct the &T (because, ultimately, any way of sharing potentially-mutable data between tasks requires unsafe).

The problem is, this RFC very explicitly breaks this otherwise-inviolate guarantee. It provides a mechanism to produce a &T that may very well alias with another task's &mut T without ever using unsafe. That's a consequence of the fact that static, by definition, provides a way of sharing data without unsafe. At its core, that is why static mut must require unsafe for all accesses, because I can write a task that never once uses unsafe and still violates memory safety. This comes back to the assumption made by Share, which is that &T is threadsafe if and only if it can assume that the &T does not alias with a &mut T. This is why mechanisms that can guarantee that assumption can be used to share &Ts to various tasks. Since static mut cannot itself guarantee that assumption, it is simply not correct to remove the need for unsafe.

@lilyball
Copy link
Contributor

Incidentally, I retract my suggestion to remove static mut and use static FOO: Unsafe<T> instead. Besides that making the issue of the public value field on Unsafe<T> into a much more serious issue than it is today (by reintroducing the same issues I have above about this RFC's proposal), it's also not compatible with what I believe is the planned change of opt-in kinds. With opt-in kinds, Unsafe<T> would no longer implicitly be Share, thus disallowing it from being used in a static. We could of course create an UnsafeShare type that does explicitly have the Share kind, but that's a bad idea because we would still need the public value field. Better to just keep static mut.

@arielb1
Copy link
Contributor

arielb1 commented Jul 23, 2014

The Unsafe reference problem would still exist if we have static mut. I think the best way to fix the static mut problem would be to add public-initialize-private-access fields and use them for Unsafe. Up to the URP, the situation with statics and with vectors is identical (you shouldn't be able to create a vector with a bogus *T pointer).

By the way, could we talk on IRC?

@glaebhoerl
Copy link
Contributor

@arielb1

I think that it would be preferable to let only consts be "compile-time known" - you can always declare const my_const : T = some_static

If statics aren't compile-time known, only consts, then this is precisely what you couldn't do. Maybe you meant to write the reverse (static MY_STATIC: T = SOME_CONST;).

(again, because statics containing an Unsafe are mutable and therefore are a bad idea in pattern matching etc.)

I don't think there would necessarily be a problem with allowing static ints in patterns but not Unsafes. I think (hope) that's what we already do today. I think both options (statics are compile-time known vs. they aren't) are reasonable, it's just something that would have to be given thought and decided.

@arielb1
Copy link
Contributor

arielb1 commented Jul 23, 2014

@kballard

You can already write an &T that can alias with a &mut T created by otherwise-legal unsafe code (rust-lang/rust/#15920)

My current viewpoint is:

  1. The public field in Unsafe (issue 15920) is problematic. Fix it. This would probably require "public initialization, private access" fields.
  2. There should be 2 types of statics - value statics (const) and variable statics (static). Value statics should be compile-time constants (i.e. rvalues, taking a reference of them creates a temporary), and variable statics should be variables - up to ABI they should behave like members of a struct that an &'static reference to each static is passed as an "implicit argument" to all methods (e.g., in a classic RISC, you can say that %gp is an "implicit argument" that is a pointer to a structure containing all statics). Because of this, they are required to be Share. Of course a static can be placed into rodata iff it contains no Unsafe<T> substructure.
  3. static mut is unnecessary, being the same as UnsafeShare<T> and should not be included (I'm not particularly in favour of adding syntactic sugar for unsafe code).
  4. Then there's also the address_insignificant issue. I would say leaving it as an attribute would be fine (address-insignificance would be legal in the struct-of-pointers interpretation I proposed in (2)). For sanity, the compiler should complain if someone tries to create an address insignificant variable containing an Unsafe and maybe in more cases (that depends on the language semantics on object partial aliasing).

@lilyball
Copy link
Contributor

@arielb1 Regarding Unsafe<T>'s public value field, yes, it's an issue, and I would love to see it fixed. It's not as big an issue as the proposed change to static mut because Unsafe<T> is expected to only ever be used by carefully-written library code that very tightly controls access to the Unsafe<T>. It obviously allows for constructing a &T that aliases with a &mut T, but doing this requires using unsafe code within that task, and also requires you to very intentionally produce a &T from an Unsafe<T> using a mechanism that you're told you shouldn't be doing (i.e. accessing the value field directly). It's problematic, but it's at least obviously problematic and regular users of Rust will not be expected to ever use Unsafe<T> themselves.

static mut with this proposed change, on the other hand, is not obviously problematic to most people. This very debate proves that point. And the entire point of making this change is to allow regular users of Rust to be able to use static mut with various library-provided types, which means we do actually expect regular users to be using this functionality. I think that means we need to meet a much higher safety bar, to make it more difficult for a regular user to write unsafe code.

FWIW, with the const proposal, you could still stuff an Unsafe<T> in a static and thus introduce the same issue I've been complaining about. But, along the lines of the public value field, that would at least be something we would not be encouraging users to do (and in fact would be telling them they're not supposed to do this). Heck, that's why the type is called Unsafe, to tell people that the very usage of this type should be considered an unsafe thing and needs to be handled carefully. And once we have opt-in kinds the problem goes away, as Unsafe<T> will no longer be Share at that point.

If we can fix the Unsafe value field issue, then I would be right back to thinking that static mut is unnecessary. Granted, with opt-in kinds, we'd either have to go ahead and make Unsafe<T> be Share anyway (which seems quite odd) or have an explicit separate UnsafeShare<T> type (which seems a shame; maybe call it StaticMut or something).

@nikomatsakis
Copy link
Contributor

@kballard I don't know why you say that a static mut is "expected" to be public. Indeed, I expect just the opposite.

@nikomatsakis
Copy link
Contributor

On Wed, Jul 23, 2014 at 09:08:55AM -0700, Gábor Lehel wrote:

@nikomatsakis Under the three-way distinction you propose, would static (not const, not mut) things still be considered to be "known at compile time"?

No.

If we gain non-type generic parameters (functions parameterized over a compile-time uint, etc.), would it be allowed to pass a static as such an argument, or only consts?

No.

Similarly, would it be permitted to use statics in patterns, or only consts?

Only consts.

IINM it also wouldn't be possible to take an &'static reference to a const, only to a static. This would be another use case for the latter. (And is an argument in favor of using different names for them.)

Yes.

@lilyball
Copy link
Contributor

@nikomatsakis

I don't know why you say that a static mut is "expected" to be public. Indeed, I expect just the opposite.

Maybe that's the confusion here. Go look at std::sync::Once. By far the most common use for this requires stuffing it into a static mut, and then using an unsafe {} block to invoke what is actually a safe method. Similarly, if you're using std::sync::atomics or sync::mutex to protect global resources, you're going to be using static mut to hold them. These are the cases that motivate this RFC to begin with; the whole reason of allowing you to bypass the unsafe {} is because clients of these APIs are currently required to use static mut and unsafe {} in order to use what are actually safe APIs.

@nikomatsakis
Copy link
Contributor

On Thu, Jul 24, 2014 at 10:22:38AM -0700, Kevin Ballard wrote:

These are the cases that motivate this RFC to begin with; the whole
reason of allowing you to bypass the unsafe {} is because clients
of these APIs are currently required to use static mut and unsafe {} in order to use what are actually safe APIs.

@kballard I guess I wasn't precise enough. I expect any static muts
that are expected to be used unsafely to be private. Static
variables that will only be used safely can be public, yes. And yes,
unsafe code can cause problems there, but that's nothing new. If I
return a &T to someone, they can unsafely transmute it to a *mut T
and then modify it willy nilly even though they are not supposed
to.

@lilyball
Copy link
Contributor

Ultimately, I still feel like this is a violation of Share. That kind is defined such that passing &T between tasks is thread-safe, not such that constructing a &T from a T is thread-safe. Existing data-sharing types like RWLock need to provide the guarantee that the construction of &T is safe. And of course construction of a &'static T is safe from a static because it is not mutable. But a static mut cannot make the guarantee that the construction of &T is safe, because there is no synchronization going on. Anyone using a static mut has to provide that guarantee themselves before constructing the &T, which is why construction of &T requires unsafe {}.

I do get the argument that mutation is still unsafe and therefore the guarantee can be provided on the mutation side. But I just don't consider that persuasive. Generally, the approach Rust takes is to require unsafe on both ends of the potentially-unsafe behavior, not just the mutation end.

Also, the more we talk about this, the more I'm convinced that requiring users to use static mut in order to use otherwise-safe APIs (e.g. Once, StaticMutex) is a bad idea. The user doesn't want the value itself to be mutable, they just want to be able to use a value that safely provides interior mutability. This alone I think is a good enough reason to switch to the const + static approach outlined earlier.

@alexcrichton
Copy link
Member Author

Closing in favor of #246, which is now accepted.

@alexcrichton alexcrichton deleted the safe-static-mut branch October 2, 2014 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants