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: Mark mem::forget as a safe function #25187

Merged
merged 1 commit into from
May 8, 2015

Conversation

alexcrichton
Copy link
Member

This commit is an implementation of RFC 1066 where the conclusion was
that leaking a value is a safe operation in Rust code, so updating the signature
of this function follows suit.

Closes #25186

@rust-highfive
Copy link
Collaborator

r? @nikomatsakis

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member Author

r? @aturon

I'm specifically curious on suggestions for improvements to the doc-comment on mem::forget as well.

@rust-highfive rust-highfive assigned aturon and unassigned nikomatsakis May 7, 2015
@alexcrichton alexcrichton added the beta-nominated Nominated for backporting to the compiler in the beta channel. label May 7, 2015
/// The safety of this function implies that when writing `unsafe` code
/// yourself, you cannot write a primitive that relies on a destructor running
/// to preserve memory safety. Unsafe code must be resilient to destructors not
/// running in all circumstances.
Copy link
Member

Choose a reason for hiding this comment

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

This is very confusing. We can't rely on destructors? Of course we can, as long as we don't give away the ownership of the value (so no risk of forget or being passed into an Rc owner, for example).

We rely on stack guards in our code, for example here https://github.com/rust-lang/rust/blob/master/src/libstd/io/mod.rs#L72-L78

Copy link
Member Author

Choose a reason for hiding this comment

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

I've pushed some minor updates to the wording here, but I will echo @aturon in that explicitly specifying this kind of detail is likely best left to a future RFC with a more focused discussion.

@alexcrichton alexcrichton added the beta-accepted Accepted for backporting to the compiler in the beta channel. label May 7, 2015
@@ -235,7 +235,6 @@ extern "rust-intrinsic" {
///
/// `forget` is unsafe because the caller is responsible for
/// ensuring the argument is deallocated already.
Copy link
Contributor

Choose a reason for hiding this comment

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

forget is no longer unsafe.

@tbu-
Copy link
Contributor

tbu- commented May 7, 2015

Might be a breaking change because the calling convention of mem::forget changed. (?)

This commit is an implementation of [RFC 1066][rfc] where the conclusion was
that leaking a value is a safe operation in Rust code, so updating the signature
of this function follows suit.

[rfc]: https://github.com/rust-lang/rfcs/blob/master/text/1066-safe-mem-forget.md

Closes rust-lang#25186
@alexcrichton
Copy link
Member Author

@tbu- strictly speaking this is a breaking change, but due to the current implementation in the compiler it's not a breaking change because taking the function by value (which is when the ABI in the type starts mattering) you get an ICE:

use std::mem;

fn main() {
    let f = mem::forget::<i32>;
}
<anon>:4:9: 4:10 warning: unused variable: `f`, #[warn(unused_variables)] on by default
<anon>:4     let f = mem::forget::<i32>;
                 ^
error: internal compiler error: can't monomorphize a NodeForeignItem(ForeignItem { ident: forget#0, attrs: [Spanned { node: Attribute_ { id: AttrId(810), style: AttrOuter, value: Spanned { node: MetaNameValue("doc", Spanned { node: LitStr("/// Moves a value out of scope without running drop glue.", CookedStr), span: Span { lo: BytePos(124395), hi: BytePos(124452), expn_id: ExpnId(4294967295) } }), span: Span { lo: BytePos(124456), hi: BytePos(124513), expn_id: ExpnId(4294967295) } }, is_sugared_doc: true }, span: Span { lo: BytePos(124456), hi: BytePos(124513), expn_id: ExpnId(4294967295) } }, Spanned { node: Attribute_ { id: AttrId(811), style: AttrOuter, value: Spanned { node: MetaNameValue("doc", Spanned { node: LitStr("///", CookedStr), span: Span { lo: BytePos(124457), hi: BytePos(124460), expn_id: ExpnId(4294967295) } }), span: Span { lo: BytePos(124518), hi: BytePos(124521), expn_id: ExpnId(4294967295) } }, is_sugared_doc: true }, span: Span { lo: BytePos(124518), hi: BytePos(124521), expn_id: ExpnId(4294967295) } }, Spanned { node: Attribute_ { id: AttrId(812), style: AttrOuter, value: Spanned { node: MetaNameValue("doc", Spanned { node: LitStr("/// `forget` is unsafe because the caller is responsible for", CookedStr), span: Span { lo: BytePos(124465), hi: BytePos(124525), expn_id: ExpnId(4294967295) } }), span: Span { lo: BytePos(124526), hi: BytePos(124586), expn_id: ExpnId(4294967295) } }, is_sugared_doc: true }, span: Span { lo: BytePos(124526), hi: BytePos(124586), expn_id: ExpnId(4294967295) } }, Spanned { node: Attribute_ { id: AttrId(813), style: AttrOuter, value: Spanned { node: MetaNameValue("doc", Spanned { node: LitStr("/// ensuring the argument is deallocated already.", CookedStr), span: Span { lo: BytePos(124530), hi: BytePos(124579), expn_id: ExpnId(4294967295) } }), span: Span { lo: BytePos(124591), hi: BytePos(124640), expn_id: ExpnId(4294967295) } }, is_sugared_doc: true }, span: Span { lo: BytePos(124591), hi: BytePos(124640), expn_id: ExpnId(4294967295) } }, Spanned { node: Attribute_ { id: AttrId(814), style: AttrOuter, value: Spanned { node: MetaList("stable", [Spanned { node: MetaNameValue("feature", Spanned { node: LitStr("rust1", CookedStr), span: Span { lo: BytePos(124603), hi: BytePos(124610), expn_id: ExpnId(4294967295) } }), span: Span { lo: BytePos(124654), hi: BytePos(124672), expn_id: ExpnId(4294967295) } }, Spanned { node: MetaNameValue("since", Spanned { node: LitStr("1.0.0", CookedStr), span: Span { lo: BytePos(124620), hi: BytePos(124627), expn_id: ExpnId(4294967295) } }), span: Span { lo: BytePos(124673), hi: BytePos(124689), expn_id: ExpnId(4294967295) } }]), span: Span { lo: BytePos(124647), hi: BytePos(124690), expn_id: ExpnId(4294967295) } }, is_sugared_doc: false }, span: Span { lo: BytePos(124645), hi: BytePos(124690), expn_id: ExpnId(4294967295) } }], node: ForeignItemFn(FnDecl { inputs: [Arg { ty: Ty { id: 16, node: TyPath(None, Path { span: Span { lo: BytePos(124715), hi: BytePos(124716), expn_id: ExpnId(4294967295) }, global: false, segments: [PathSegment { identifier: T#0, parameters: AngleBracketedParameters(AngleBracketedParameterData { lifetimes: [], types: [], bindings: [] }) }] }), span: Span { lo: BytePos(124715), hi: BytePos(124716), expn_id: ExpnId(4294967295) } }, pat: Pat { id: 15, node: PatWild(PatWildSingle), span: Span { lo: BytePos(124712), hi: BytePos(124713), expn_id: ExpnId(4294967295) } }, id: 14 }], output: Return(Ty { id: 17, node: TyTup([]), span: Span { lo: BytePos(124721), hi: BytePos(124723), expn_id: ExpnId(4294967295) } }), variadic: false }, Generics { lifetimes: [], ty_params: [TyParam { ident: T#0, id: 18, bounds: [], default: None, span: Span { lo: BytePos(124648), hi: BytePos(124649), expn_id: ExpnId(4294967295) } }], where_clause: WhereClause { id: 19, predicates: [] } }), id: 13, span: Span { lo: BytePos(124699), hi: BytePos(124724), expn_id: ExpnId(4294967295) }, vis: Public })
note: the compiler unexpectedly panicked. this is a bug.
note: we would appreciate a bug report: https://github.com/rust-lang/rust/blob/master/CONTRIBUTING.md#bug-reports
note: run with `RUST_BACKTRACE=1` for a backtrace
thread 'rustc' panicked at 'Box<Any>', /home/rustbuild/src/rust-buildbot/slave/beta-dist-rustc-linux/build/src/libsyntax/diagnostic.rs:209

@frankmcsherry
Copy link
Contributor

@bluss observes back in rust-lang/rfcs#1066 that std::boxed::into_raw<T>(Box<T>) -> *mut T should be able to have the unsafe label removed as well, for the same reason. If this is true, and now is a fine time to break some things in the name of clarifying unsafe, it might be good to do too. Obviously it isn't necessary to do, but neither was removing unsafe from mem::forget. :)

@bluss
Copy link
Member

bluss commented May 8, 2015

@alexcrichton Thanks for updating the text!

@brson
Copy link
Contributor

brson commented May 8, 2015

@bors r+ p=1

@bors
Copy link
Contributor

bors commented May 8, 2015

📌 Commit dd59b1f has been approved by brson

@alexcrichton
Copy link
Member Author

@bors: p=50 (putting beta-accepted at the top)

@bors
Copy link
Contributor

bors commented May 8, 2015

⌛ Testing commit dd59b1f with merge 7132092...

bors added a commit that referenced this pull request May 8, 2015
This commit is an implementation of [RFC 1066][rfc] where the conclusion was
that leaking a value is a safe operation in Rust code, so updating the signature
of this function follows suit.

[rfc]: https://github.com/rust-lang/rfcs/blob/master/text/1066-safe-mem-forget.md

Closes #25186
@bors bors merged commit dd59b1f into rust-lang:master May 8, 2015
@alexcrichton alexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label May 11, 2015
@RalfJung
Copy link
Member

What's the reason that into_raw still keeps its unsafe? Also see what @frankmcsherry wrote above (which I can't find an answer to, nor has there ever been an answer to similar questions in the RFC PR itself).

@alexcrichton alexcrichton deleted the mem-forget-safe branch July 10, 2015 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
beta-accepted Accepted for backporting to the compiler in the beta channel.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking issue for Alter mem::forget to be safe (RFC 1066)
10 participants