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.

Sign up for GitHub

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

Jump to bottom

UnsafeCell allows types with destructors to end up in statics. #30667

Closed
eddyb opened this issue Jan 1, 2016 · 9 comments
Closed

UnsafeCell allows types with destructors to end up in statics. #30667

eddyb opened this issue Jan 1, 2016 · 9 comments
Labels
A-type-system Area: Type system C-bug Category: This is a bug.

Comments

@eddyb
Copy link
Member

eddyb commented Jan 1, 2016

Apparently this has been used by lazy-static for a while now as a "nightly feature":

use std::cell::UnsafeCell;

struct SyncCell(UnsafeCell<Option<$T>>);
unsafe impl Sync for SyncCell {}

static DATA: SyncCell = SyncCell(UnsafeCell::new(None));

This happens to work even when $T is Vec<X> or String, which should not end up in a static, by the current rules.

A potential fix would involve adding another constant qualification flag for "contains UnsafeCell<D> where D has a destructor" and deny that in a static.
I would like to avoid reusing a combination of the existing flags, as that can result in false positives.

Another possible plan (requiring a new RFC) would be to:

  • allow destructors in statics
    • optionally warn about the "potential leak"
  • allow instantiating structures that impl Drop in constant expressions
  • prevent const items from holding values with destructors, but allow const fn to return them
  • disallow constant expressions which would result in the Drop impl getting called, where they not in a constant context

cc @nikomatsakis @thepowersgang
@thepowersgang
Copy link
Contributor

Reference rust-lang/rfcs#913 and draft RFC on allowing destructors in statics - https://github.com/thepowersgang/rust-lang_rfcs/blob/drop-types-in-const/text/0000-drop-types-in-const.md

thepowersgang added a commit to thepowersgang/rust-lang_rfcs that referenced this issue Jan 1, 2016
@thepowersgang
Replace detailed design with @eddyb's comments from rust-lang/rust#30667
d400320
@thepowersgang thepowersgang mentioned this issue Jan 1, 2016
Merged
@steveklabnik steveklabnik added A-libs A-type-system Area: Type system and removed A-libs labels Jan 4, 2016
@nikomatsakis
Copy link
Contributor

Huh, so, I hadn't thought hard about this for a while, but it seems like, to preserve the spirit of the current rules, we need to be more conservative around some kinds of values. But it is kind of a pain. Basically:

  1. The result of a const fn needs to have type-based heuristics applied, whereas normally we try to use value-based rules. I hadn't thought about this before, seems obvious now.
  2. Any expression that creates an UnsafeCell (directly or indirectly) would have to also have type-based heuristics applied.

By "type-based heuristcs", I mean rules that say "if this type potentially reaches something with a dtor, complain". But argh what a pain. It'd be nice if this "something with a dtor" was more cleanly reified as a language-level bound.

@nikomatsakis
Copy link
Contributor

cc @alexcrichton

@alexcrichton
Copy link
Member

This is somewhat intentional (TLS also relies on this ability). The way I rationalize it looks like:

  • The compiler verifies that no expression requires running a destructor. For example Option<T> where T has a destructor is only allowed if None is used.
  • It's unsafe to then mutate any statics
  • As a result, it requires unsafe code to put something which needs a destructor into a static.

Along those lines this seems ok to me (the issue as-is) personally. I'd also agree though that this probably wasn't given any hard though, and I haven't quite grasped how this connects to const fn just yet.

@eddyb
Copy link
Member Author

eddyb commented Jan 11, 2016

@alexcrichton It's not unsafe to mutate e.g. a StaticMutexWithData<X> with a safe API and no destructor of its own (which seems perfectly normal to have, doesn't it?), and X can be Option<Box<T>> initially set to None.

@alexcrichton
Copy link
Member

Ah yeah I basically just mean that unsafe is needed somewhere to put the dtor in a static (in this case it's hidden inside the mutex).

I would personally be fine with types-with-destructors in statics just being specc'd as never having dtors run.

@nikomatsakis
Copy link
Contributor

On Mon, Jan 11, 2016 at 12:24:43AM -0800, Alex Crichton wrote:

Along those lines this seems ok to me (the issue as-is)
personally. I'd also agree though that this probably wasn't given
any hard though, and I haven't quite grasped how this connects to
const fn just yet.

I don't think there is necessarily a problem with const fn per se,
however we have to ensure that whatever "expr" restrictions we impose
on constant expressions are also imposed on the body of a const fn,
which means that it will limit their utility. Maybe it's a kind of red
herring though.

@indiv0 indiv0 mentioned this issue May 14, 2016
Merged
@Mark-Simulacrum
Copy link
Member

Appears to be a part of/subsumed by #33156.

@Mark-Simulacrum Mark-Simulacrum added the C-bug Category: This is a bug. label Jul 24, 2017
@steveklabnik
Copy link
Member

Triage; wow this is an old issue!

I believe that rust-lang/rfcs#1440 makes this fixed. #33156 was merged. So, closing! If we're still missing something here, please let me know!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-type-system Area: Type system C-bug Category: This is a bug.
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@steveklabnik @alexcrichton @eddyb @nikomatsakis @thepowersgang @Mark-Simulacrum