Skip to content

Introduce a Unsafe<T> type to be used as the basis for Cell, RefCell, Atomic, etc and remove Freeze #12577

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

Closed
nikomatsakis opened this issue Feb 26, 2014 · 13 comments · Fixed by #13076

Comments

@nikomatsakis
Copy link
Contributor

UPDATED

We have various safe wrappers that permit aliasable mutability (Cell, RefCell, etc). To be sound, all of them must use markers for invariance and non-freeze. We should factor out these markers and so on into a base type Unsafe<T> that contains the most general (and unsafe) form of interior mutability, which is a function that yields a *mut T:

struct Unsafe<T> {
    priv value: T,
}

impl<T> Unsafe<T> {
    unsafe fn get(&self) -> *mut T { transmute(&self.value) }
}

It will be undefined behavior to transmute an aliasable reference into something mutable through any other means.

Unsafe<T> wlil be integrated into the compiler as follows:

  • Its contents are invariant (variance analysis).
  • If an immutable static item contains Unsafe<T>, you cannot take its address.
  • If a type contains Unsafe<T> (interior), the compiler will have to be careful about optimizing aliasing and so on. This may correspond to some existing LLVM compiler concept like volatile.
  • Remove the Freeze kind (seemingly orthogonal but...not).

This type replaces transmute_mut as the building block for types like Cell. What is better about this is that it ties together the transmute along with the marker types that are required for soundness, so that they cannot be forgotten.

cc @alexcrichton, with whom I was discussing this
cc @brson, because this relates to our discussion on laying out rules for unsafe code

@nikomatsakis
Copy link
Contributor Author

I guess this is an RFC, which I hate having on the issue tracker. Sigh.

@alexcrichton
Copy link
Member

Albeit a small change, consolidating unsafety is one of my favorite things to do and this does a nice job of it. I'm not entirely sure where such a structure would live, perhaps std::cell, it'd be kinda a shame to have std::mut::Mut.

We may also want to make an effort to tuck this away in the docs so we don't jump to this first as well.

@eddyb
Copy link
Member

eddyb commented Feb 27, 2014

I'd like to know how feasible it would be to restrict most transmutes to transmute_ref<T, U>(&T) -> &U and transmute_mut_ref<T, U>(&mut T) -> &mut U and maybe transmute_value<T: Freeze, U: Freeze>(T) -> U (for the remaining cases).

@nikomatsakis
Copy link
Contributor Author

On Thu, Feb 27, 2014 at 02:02:54AM -0800, Eduard Burtescu wrote:

I'd like to know how feasible it would be to restrict most
transmutes to transmute_ref<T, U>(&T) -> &U and
transmute_mut_ref<T, U>(&mut T) -> &mut U and maybe
transmute_value<T: Freeze, U: Freeze>(T) -> U (for the remaining
cases).

A good question.

@brson
Copy link
Contributor

brson commented Mar 1, 2014

+1

@huonw
Copy link
Member

huonw commented Mar 1, 2014

As another datapoint, I just noticed another example of a Cell-like pattern in libstd: local_data::get_mut.

@nikomatsakis
Copy link
Contributor Author

Today we decided to call this type Unsafe<T>. Moreover, this type would be a lang item that is used to inform the rules related to static as well as LLVM optimizations.

@nikomatsakis
Copy link
Contributor Author

I updated the main text.

@nikomatsakis
Copy link
Contributor Author

@flaper87 good thing we waited to start implementing ;)

@thestinger
Copy link
Contributor

The volatile concept isn't really related to this. It means an external force not part of the program may modify the memory. I don't think we have to relax any of the existing optimizations, since we're not doing any type-based alias analysis.

+1 for these changes though

@flaper87 flaper87 removed their assignment Mar 5, 2014
@flaper87
Copy link
Contributor

flaper87 commented Mar 5, 2014

@nikomatsakis indeed, I could probably work on this since I was already assigned to the other 'remove Freeze` bug.

@flaper87 flaper87 self-assigned this Mar 5, 2014
@flaper87 flaper87 mentioned this issue Mar 6, 2014
4 tasks
@brson brson mentioned this issue Mar 6, 2014
@flaper87
Copy link
Contributor

It's not obvious from the bug report whether the static borrow should be forbidden just for immutable static items or from any kind. Just to make sure we're all on the same page, I think this should be applied just on immutable static items. @nikomatsakis If you agree, I'll update the bug description

@nikomatsakis
Copy link
Contributor Author

On Fri, Mar 14, 2014 at 02:53:39PM -0700, Flavio Percoco Premoli wrote:

It's not obvious from the bug report whether the static borrow
should be forbidden just for immutable static items or from any
kind. Just to make sure we're all on the same page, I think this
should be applied just on immutable static items. @nikomatsakis If
you agree, I'll update the bug description.

Yes, I agree.

bors added a commit that referenced this issue Mar 20, 2014
`Share` implies that all *reachable* content is *threadsafe*.

Threadsafe is defined as "exposing no operation that permits a data race if multiple threads have access to a &T pointer simultaneously". (NB: the type system should guarantee that if you have access to memory via a &T pointer, the only other way to gain access to that memory is through another &T pointer)...

Fixes #11781
cc #12577 

What this PR will do
================

- [x] Add Share kind and
- [x]  Replace usages of Freeze with Share in bounds.
- [x] Add Unsafe<T> #12577
- [x] Forbid taking the address of a immutable static item with `Unsafe<T>` interior

What's left to do in a separate PR (after the snapshot)?
===========================================

- Remove `Freeze` completely
bors added a commit that referenced this issue Mar 22, 2014
This PR removes the `Freeze` kind and the `NoFreeze` marker completely.

Fixes #12577

cc @nikomatsakis r?
bors added a commit to rust-lang-ci/rust that referenced this issue Jul 25, 2022
internal: NameRefKind classification is not optional
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 a pull request may close this issue.

7 participants