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 Drop types in statics/const functions #1440

Merged
merged 7 commits into from
Apr 22, 2016

Conversation

thepowersgang
Copy link
Contributor

This addresses #913 and rust-lang/rust#30667

rendered

# Motivation
[motivation]: #motivation

Most collection types do not allocate any memory when constructed empty. With the change to make leaking safe, the restriction on `static` items with destructors
Copy link
Member

Choose a reason for hiding this comment

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

You mean some. @gankro, I believe, explicitly expressed the intent to specify that we only will have allocation less new when it doesn’t hurt performance/implementation complexity or something like that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point I guess. I had forgotten about the BTree collections (which, looking at the source, do allocate on 'new'). That said, there's still a lot of them that don't allocate until data is added (Vec/String, HashMap, LinkedList)

@mahkoh
Copy link
Contributor

mahkoh commented Jan 1, 2016

The RFC does not address the case of objects in thread local storage.

@thepowersgang
Copy link
Contributor Author

To clarify for those who have jumped to conclusions -

This RFC does not change the current stance on life-after-main. It fully accepts that Drop types placed in statics WILL leak.

…destructors will never run, acknowledge `.dtors` as alternative
@eddyb
Copy link
Member

eddyb commented Jan 1, 2016

The confusion around destruction order is not the main problem, but the actual safety of the safety (accessing globals after they've been dropped).

Thread locals in libstd solve this by having a "destructor is running" flag, set before going into the destructor, and by making access indirect (using closures) such that "destructor is not running" checks can be done before using the value.

Sadly, such a scheme doesn't map directly to multi-threaded globals, as you need locks instead of mere flags.

However, static variables also run in the issue of calling Drop::drop with a mutable reference to a an immutably shared global. If there is no interior mutability, it's even stored in read-only memory.

Therefore, a proper solution arises: a DropShared trait, implemented by Mutex and RwLock, which can run the inner value's destructor thanks to interior mutability, and already have a poison flag to indicate that the inner value may be in an invalid state, preventing further accesses.

Is it worth it? Doubtful, given how globals would be dropped just before the process is destroyed.
But it might be useful for other concurrent contraptions (arenas shared across threads?).

cc @aturon

@mahkoh
Copy link
Contributor

mahkoh commented Jan 1, 2016

It's odd that const items cannot contain the return values of const functions in all cases. (At least in one case proposed by this RFC.) On the one hand this makes const fn a superset of const. But one the other hand const functions cannot express everything that can appear on the right hand side of a static, e.g., they cannot refer to other statics which the rhs of a static can. I assumed that this restriction was in place precisely to allow const functions to be always used as the rhs of a const item. But if this is not the case (no longer the case with this RFC), then why can't const functions refer to statics? (This is necessary to create a linked list with a sentinel node as the rv of a const fn.)

@eddyb
Copy link
Member

eddyb commented Jan 1, 2016

@mahkoh That's because it's impossible to check that property from the type, i.e. the body of a const fn is not technically part of the public API (this may change in the future, not sure if necessary).

Whereas we already compute "had UnsafeCell" differently for const fn than for const expressions: The former is based on the type, the latter is based on the value and allows &None::<Cell<i32>> as there is no UnsafeCell value that could ever be accessed.

Therefore, computing "has UnsafeCell<D> where D requires drop" is a trivial addition, and does not require inspecting the body of the const fn either.

Allowing const fn to return references to statics would require inspecting the returned value, not just the return type.

OTOH, the restriction on const items I suggested in rust-lang/rust#30667 is to prevent logical construction of a value with a destructor from a constant path, i.e. {CONST;} calling a destructor.

I felt that const fn being permitted to return such values would be less confusing or controversial, but the const item restriction is merely a conservative choice and not actually required for safety, AFAIK.

@arielb1
Copy link
Contributor

arielb1 commented Jan 1, 2016

@eddyb

What exactly is meant by the body of a const fn not being a part of the public API? I am quite sure that it is a part of it in basically the same way the body of a regular fn is part of the public API - code that can call it can depend on its behaviour (but unlike non-const fns, problems here can cause compilation failures in addition to runtime failures).

1 for having unrestricted statics with destructors (as I said several times, it is perfectly legal to exit with exit(3), which will skip all cleanup). I am not sure of consts with destructors - it may be unobvious when the destructors are called.

@Ericson2314
Copy link
Contributor

We don't require const items to be Copy, right? If we indeed don't, it should already be clear that using a const item is more flexible/different than a move of a normal value—a normal move would be disallowed. I don't think allowing Drop const items would add more confusion then.

@arielb1
Copy link
Contributor

arielb1 commented Jan 3, 2016

@Ericson2314

The problem is that you can take the address of a constant, in which case it is very unobvious when the destructor is run.

@eddyb
Copy link
Member

eddyb commented Jan 3, 2016

@arielb1 Oh, I forgot about taking a reference to a constant value, that's something we can prevent relatively easily (we already allow &0 and disallow &AtomicUsize::new(0) based solely on the fact that the latter contains UnsafeCell).

@Ericson2314
Copy link
Contributor

Mm it's a gotcha sure, but follows from const items' rvalue semantics. Maybe a lint would suffice?

@retep998
Copy link
Member

retep998 commented Jan 4, 2016

I'm definitely in favor of this RFC. While it is recommended to avoid statics, sometimes you just need statics, and the limitation on no Drop types was rather restrictive in the face of leaking being considered safe.

@nrc nrc added the T-lang Relevant to the language team, which will review and decide on the RFC. label Jan 4, 2016
@nikomatsakis nikomatsakis self-assigned this Jan 7, 2016
@nikomatsakis
Copy link
Contributor

Discussed in the @rust-lang/lang meeting. Everyone felt in favor of this in principle, though none of us have had time to read the RFC in detail (it's not clear to me whether there are grey areas left uncovered). One question was whether we want to guarantee dtors won't run or leave it unspecified.

@retep998
Copy link
Member

It should be backwards compatible if we start with it unspecified, since we can later change to a guarantee in either direction, right?

@aturon
Copy link
Member

aturon commented Mar 12, 2016

@retep998 Yeah, I made a similar point during the meeting. But it'd probably be a good idea to at least have a good sense of the overall contour here and what our long-term plans may be.

@glaebhoerl
Copy link
Contributor

I thought "no life outside of main()" is an explicit design goal of Rust, which would be the argument for guaranteeing they don't run.

@eddyb
Copy link
Member

eddyb commented Mar 12, 2016

Note that #[link_section = "..."] static lets you place a static instructing the C runtime to run a destructor, so a crate could just provide this for a few platforms (and use the same machinery TLS has to making it safe, i.e. disallowing access while the destructor runs).

Thinking about it more, I think we can make #[thread_local] static safe to access and don't guarantee destructors, the standard library providing that on top - so cases where you just want a Cell<usize> counter would be better served.

@arielb1
Copy link
Contributor

arielb1 commented Apr 21, 2016

If STATIC has an initializer, then (&STATIC).0 will work atm in old trans, even if it shouldn't.

Uh, no, it doesn't, at least not reliably:

struct Foo(u64, u64);

struct Unit(Foo);

static FOO: Unit = Unit((&BAR).0);
static BAR: Unit = Unit(Foo(0,0));
fn main() {}

@arielb1
Copy link
Contributor

arielb1 commented Apr 21, 2016

@eddyb

I believe all of the work is in actually checking constants and static initializers and interactions around borrowing & rvalue promotions.

We do rvalue promotion purely as an optimization today, don't we (except for empty arrays which are not affected by this)? I don't think we actually specified anything here.

@arielb1
Copy link
Contributor

arielb1 commented Apr 21, 2016

I'll also note that while leaking dtors is indeed allowed in safe code, it's also rare today -- and almost always a bug. It would be good to flesh out the lint side of this story -- I could imagine warning by default on any dtor leaked by a static, with the ability to apply an attribute to a type that flags it as "leaks are harmless", or something like that.

This is just as much of a leak as exiting by calling exit(3) or by an angry user with a task manager. I don't feel like this is a big thing.

@eddyb
Copy link
Member

eddyb commented Apr 21, 2016

@arielb1 Right, by "has an initializer" I meant that "the static being referenced was translated before this one".

The interactions with rvalue promotions are that, if we don't want to kill it, we have to ensure it preserves semantics.

@arielb1
Copy link
Contributor

arielb1 commented Apr 21, 2016

@aturon

Again: the invariant Drop statics break is that there is no accessible data with destructors after main returns. This can't really be a safety invariant - after main returns, your program is not running anymore (and because Rust is not a total language, it is very easy to prevent main from ever possibly returning).

It is somewhat useful if you want to use valgrind to prove the absence of inaccessible data (mem::forget-style leaks), but that does not seem to be that interesting, especially because valgrind already separates accessible from inaccessible data.

@arielb1
Copy link
Contributor

arielb1 commented Apr 21, 2016

@eddyb

The interactions with rvalue promotions are that, if we don't want to kill it, we have to ensure it preserves semantics.

I still don't see what rvalue promotions have to do with this. Rvalue promotions are supposed to not change the semantics of code that compiles without them in any case. Given that promoted rvalues are neither statics nor constants, I don't see how this RFC applies.

@eddyb
Copy link
Member

eddyb commented Apr 21, 2016

@arielb1 Either this RFC or the rvalue promotions one has to specify how the two features will interact, before this RFC is implemented.

@arielb1
Copy link
Contributor

arielb1 commented Apr 21, 2016

I don't see how these 2 features interact. This affects consts and statics. Rvalue promotion affects rvalues. The code might be shared, but rvalues are neither consts nor statics.

They might be using a similar code-path, but they are clearly distinct. In any case, the rvalue promotions RFC was not accepted yet.

@Kimundi
Copy link
Member

Kimundi commented Apr 22, 2016

@arielb1: I think the argument here is "If a rvalue can be promoted it could also usually be written as a const", which amounts to the let x = &possible_const_rvalue example and wether you'd get a drop with that or not.

In my opinion it seems clear that promotions should not change the drop semantic inside functions, and I'd be happy to add wording in that regard to #1414. :)

@nikomatsakis
Copy link
Contributor

Huzzah! The @rust-lang/lang team has decided to accept this RFC.

In our discussion, the feeling was the semantics here are clearly underspecified, but that this is somewhat true of constants in general, and that exploring the problem in practice was the best way to uncover complications. Naturally we should be careful with stability here to avoid unintentionally stabilizing unsupportable semantics. The core thrust of this RFC, though, that statics can have types that would require destructing, but which never get run, seems unproblematic.

@nikomatsakis
Copy link
Contributor

Tracking issue: rust-lang/rust#33156

If you'd like to keep following the development of this feature, please subscribe to that issue, thanks! :)

nikomatsakis added a commit to nikomatsakis/rfcs that referenced this pull request Apr 22, 2016
@nikomatsakis nikomatsakis merged commit ccd4a78 into rust-lang:master Apr 22, 2016
@Kimundi
Copy link
Member

Kimundi commented Apr 23, 2016

Yay! 😄

@SimonSapin
Copy link
Contributor

From the RFC:

Continue to prevent const items from holding types with destructors.

What is the reason for this? I was hoping to use cont items of types with destructors to work around macro hygiene. (Having a macro expand to a path to a const item rather than a struct literal, and thus being able to make the struct’s fields private.)

@eddyb
Copy link
Member

eddyb commented Nov 25, 2016

I believe this was trying to minimize the scope of the RFC and potential confusion: while it would be perfectly safe to duplicate a constant, each use would run the destructor once which may be unexpected.
That said, if someone wants to create a RFC for extending this feature to const, I'd be on-board.

@SergioBenitez
Copy link
Contributor

I've created a PR (#1817) to amend the RFC to support drop types in const items.

aturon added a commit that referenced this pull request Jul 25, 2017
Amend #1440: allow `const` items to contain drop types.
@Centril Centril added A-drop Proposals relating to the Drop trait or drop semantics A-static Proposals relating to static items. A-const-eval Proposals relating to compile time evaluation (CTFE). labels Nov 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Proposals relating to compile time evaluation (CTFE). A-drop Proposals relating to the Drop trait or drop semantics A-static Proposals relating to static items. final-comment-period Will be merged/postponed/closed in ~10 calendar days unless new substational objections are raised. T-lang Relevant to the language team, which will review and decide on the RFC.
Projects
None yet
Development

Successfully merging this pull request may close these issues.