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

Projects
None yet
@thepowersgang
Copy link
Contributor

commented Jan 1, 2016

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

This comment has been minimized.

Copy link
@nagisa

nagisa Jan 1, 2016

Contributor

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?

This comment has been minimized.

Copy link
@thepowersgang

thepowersgang Jan 1, 2016

Author Contributor

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)

Allowing types with destructors to be directly used in `const` functions and stored in `static`s will remove the need to have
runtime-initialisation for global variables.

# Detailed design

This comment has been minimized.

Copy link
@nagisa

nagisa Jan 1, 2016

Contributor

This section should explore different implementation strategies as well. E.g. do we use .ctors and .dtors sections (that’s what c++ does) which allows for life-before-main (which we’ve been very reluctant about allowing) or do we employ scheme similar to lazy_static! (initialize on first use)?

This comment has been minimized.

Copy link
@thepowersgang

thepowersgang Jan 1, 2016

Author Contributor

It may not have been clear enough here - This is just to use the current const structure (no life before main, no life after it) to do compile-time construction of objects that support it.

# Drawbacks
[drawbacks]: #drawbacks

Destructors do not run on `static` items (by design), so this can lead to unexpected behavior when a type's destructor has effects outside the program (e.g. a RAII temporary folder handle, which deletes the folder on drop). However, this can already happen using the `lazy_static` crate.

This comment has been minimized.

Copy link
@nagisa

nagisa Jan 1, 2016

Contributor

This drawback is not necessary. I.e. we could use .dtors section which would execute all the necessary destructors for statics once we exit from main.

This comment has been minimized.

Copy link
@petrochenkov

petrochenkov Jan 1, 2016

Contributor

Life after main has the same problems as life before main, destruction order in particular. (The same set of problems apply to thread locals.)

@mahkoh

This comment has been minimized.

Copy link
Contributor

commented Jan 1, 2016

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

@thepowersgang

This comment has been minimized.

Copy link
Contributor Author

commented Jan 1, 2016

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.

Clarify collection types with non-allocating constructors, note that …
…destructors will never run, acknowledge `.dtors` as alternative
@eddyb

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Jan 2, 2016

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented Jan 4, 2016

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

@retep998

This comment has been minimized.

Copy link
Member

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.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

commented Mar 10, 2016

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

This comment has been minimized.

Copy link
Member

commented Mar 10, 2016

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

@aturon

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2016

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2016

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

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2016

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

This comment has been minimized.

Copy link
Member

commented Apr 23, 2016

Yay! 😄

@SimonSapin

This comment has been minimized.

Copy link
Contributor

commented Nov 25, 2016

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

commented Dec 13, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.