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

std: deprecate cast::transmute_mut. #13934

Merged
merged 2 commits into from
May 6, 2014
Merged

Conversation

huonw
Copy link
Member

@huonw huonw commented May 4, 2014

Turning a &T into an &mut T is undefined behaviour, and needs to be
done very very carefully. Providing a convenience function for exactly
this task is a bad idea, just tempting people into doing the wrong
thing.

(The right thing is to use types like Cell, RefCell or Unsafe.)

cc #13933

@huonw
Copy link
Member Author

huonw commented May 4, 2014

r? @alexcrichton particularly the design of the second commit.

(Will rebase at some point.)

@alexcrichton
Copy link
Member

It may be nice to have a writeup in the commit message about why this is undefined behavior. For example, this is undefined:

let foo = 3;
unsafe {
    let foo: &mut int = transmute(&foo);
    *foo = 4;
}

but is this also undefined? If not, why?

@alexcrichton
Copy link
Member

It may be nice to have a writeup in the commit message about why this is undefined behavior. For example, this is undefined:

let foo = 3;
unsafe {
    let foo: &mut int = transmute(&foo);
    *foo = 4;
}

but is this also undefined? If not, why?

let foo = 3;
unsafe {
    let foo: &mut int = &mut *(&foo as *int as *mut int);
    *foo = 4;
}

huonw added 2 commits May 5, 2014 18:20
Turning a `&T` into an `&mut T` carries a large risk of undefined
behaviour, and needs to be done very very carefully. Providing a
convenience function for exactly this task is a bad idea, just tempting
people into doing the wrong thing.

The right thing is to use types like `Cell`, `RefCell` or `Unsafe`.

For memory safety, Rust has that guarantee that `&mut` pointers do not
alias with any other pointer, that is, if you have a `&mut T` then that
is the only usable pointer to that `T`. This allows Rust to assume that
writes through a `&mut T` do not affect the values of any other `&` or
`&mut` references. `&` pointers have no guarantees about aliasing or
not, so it's entirely possible for the same pointer to be passed into
both arguments of a function like

    fn foo(x: &int, y: &int) { ... }

Converting either of `x` or `y` to a `&mut` pointer and modifying it
would affect the other value: invalid behaviour.

(Similarly, it's undefined behaviour to modify the value of an immutable
local, like `let x = 1;`.)

At a low-level, the *only* safe way to obtain an `&mut` out of a `&` is
using the `Unsafe` type (there are higher level wrappers around it, like
`Cell`, `RefCell`, `Mutex` etc.). The `Unsafe` type is registered with
the compiler so that it can reason a little about these `&` to `&mut`
casts, but it is still up to the user to ensure that the `&mut`s
obtained out of an `Unsafe` never alias.

(Note that *any* conversion from `&` to `&mut` can be invalid, including
a plain `transmute`, or casting `&T` -> `*T` -> `*mut T` -> `&mut T`.)

[breaking-change]
@@ -60,6 +60,7 @@ pub unsafe fn transmute<L, G>(thing: L) -> G {

/// Coerce an immutable reference to be mutable.
#[inline]
#[deprecated="casting &T to &mut T is undefined behaviour: use Cell<T>, RefCell<T> or Unsafe<T>"]
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we know / have thought about how long deprecated features should stay around? I don't want to start the discussion here but I think, given the fact that some things may be deprecated in the near future, we should start discussing this and make the removal date/time part of the deprecation message.

Copy link
Member Author

Choose a reason for hiding this comment

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

Just a reasonable amount of time to allow people to get help updating, a relatively obscure function like this is probably ok to remove quicker than normal, a more common function might be left longer than normal. In any case it was mentioned briefly in the original email, specifically

we'll likely leave in #[deprecated] functions for about a month or so.

In terms of actual implementation details of removal, I personally think just using occasionally checking for things to remove & consulting the git history for timing is good enough (since we don't have so much code/#[deprecated]-churn that doing it manually with grep will be particularly hard); others may disagree with this though.

Copy link
Contributor

Choose a reason for hiding this comment

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

@huonw all this sounds good to me. I think a month or so is fine for now. We'll likely need a better rule when we get nearer a 1.0 release.

bors added a commit that referenced this pull request May 5, 2014
Turning a `&T` into an `&mut T` is undefined behaviour, and needs to be
done very very carefully. Providing a convenience function for exactly
this task is a bad idea, just tempting people into doing the wrong
thing.

(The right thing is to use types like `Cell`, `RefCell` or `Unsafe`.)

cc #13933
@bors bors closed this May 6, 2014
@bors bors merged commit edd9bad into rust-lang:master May 6, 2014
@huonw huonw deleted the transmute-mut branch June 27, 2014 06:47
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 13, 2023
… r=Veykril

feat: Make unlinked_file diagnostic quickfixes work for inline modules

Finally got myself to fix this, bothered me quite a bit that this never worked
![Code_Qe3WlMvt5Q](https://user-images.githubusercontent.com/3757771/211927799-023e48ee-7cdd-4dd7-8e25-a23eddc7d897.gif)

(Just gotta fix up the indentation still)
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.

4 participants