Skip to content

Allow Drop types in const's too, with #![feature(drop_types_in_const)]. #44212

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

Merged
merged 1 commit into from
Sep 9, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions src/librustc_mir/transform/qualify_consts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ impl<'a, 'tcx> Qualifier<'a, 'tcx, 'tcx> {

/// Check for NEEDS_DROP (from an ADT or const fn call) and
/// error, unless we're in a function, or the feature-gate
/// for globals with destructors is enabled.
/// for constant with destructors is enabled.
fn deny_drop(&self) {
self.deny_drop_with_feature_gate_override(true);
}
Expand All @@ -213,9 +213,9 @@ impl<'a, 'tcx> Qualifier<'a, 'tcx, 'tcx> {
return;
}

// Static and const fn's allow destructors, but they're feature-gated.
let msg = if allow_gate && self.mode != Mode::Const {
// Feature-gate for globals with destructors is enabled.
// Constants allow destructors, but they're feature-gated.
let msg = if allow_gate {
// Feature-gate for constant with destructors is enabled.
if self.tcx.sess.features.borrow().drop_types_in_const {
return;
}
Expand All @@ -235,11 +235,13 @@ impl<'a, 'tcx> Qualifier<'a, 'tcx, 'tcx> {
let mut err =
struct_span_err!(self.tcx.sess, self.span, E0493, "{}", msg);

if allow_gate && self.mode != Mode::Const {
if allow_gate {
help!(&mut err,
"in Nightly builds, add `#![feature(drop_types_in_const)]` \
to the crate attributes to enable");
} else {
// FIXME(eddyb) this looks up `self.mir.return_ty`.
// We probably want the actual return type here, if at all.
self.find_drop_implementation_method_span()
.map(|span| err.span_label(span, "destructor defined here"));

Expand Down
2 changes: 1 addition & 1 deletion src/test/compile-fail/issue-17718-const-destructors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,6 @@ impl Drop for A {
}

const FOO: A = A;
//~^ ERROR: constants are not allowed to have destructors
//~^ ERROR: destructors in constants are an unstable feature

fn main() {}
11 changes: 9 additions & 2 deletions src/test/compile-fail/static-drop-scope.rs
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,18 @@ impl Drop for WithDtor {
fn drop(&mut self) {}
}

static FOO: Option<&'static WithDtor> = Some(&WithDtor);
static PROMOTION_FAIL_S: Option<&'static WithDtor> = Some(&WithDtor);
//~^ ERROR statics are not allowed to have destructors
//~| ERROR borrowed value does not live long enoug

static BAR: i32 = (WithDtor, 0).1;
const PROMOTION_FAIL_C: Option<&'static WithDtor> = Some(&WithDtor);
//~^ ERROR constants are not allowed to have destructors
//~| ERROR borrowed value does not live long enoug

static EARLY_DROP_S: i32 = (WithDtor, 0).1;
//~^ ERROR statics are not allowed to have destructors

const EARLY_DROP_C: i32 = (WithDtor, 0).1;
//~^ ERROR constants are not allowed to have destructors
Copy link
Contributor

Choose a reason for hiding this comment

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

This error message is confusing since constants are allowed to have destructors. Instead, they're not allowed to require destruction upon use. The error message should be changed to clarify the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe "not allowed to execute destructors"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Please leave a comment on #33156 instead, to do so when stabilizing the feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

done

Copy link
Member Author

@eddyb eddyb Aug 31, 2017

Choose a reason for hiding this comment

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

I was directing that at @SergioBenitez FWIW, github didn't update, but thanks for updating the issue description!


fn main () {}
22 changes: 20 additions & 2 deletions src/test/run-pass/issue-34053.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,32 @@

#![feature(drop_types_in_const)]

use std::sync::atomic::{AtomicUsize, ATOMIC_USIZE_INIT, Ordering};

static DROP_COUNTER: AtomicUsize = ATOMIC_USIZE_INIT;

struct A(i32);

impl Drop for A {
fn drop(&mut self) {}
fn drop(&mut self) {
// update global drop count
DROP_COUNTER.fetch_add(1, Ordering::SeqCst);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we need a test that demonstrates the runtime semantics of constants with DROP. In particular, the destructor does execute when you (e.g.) access BAR or A::BAZ, right?

I want some test that shows when the destructor runs and tests that it did indeed execute (i.e., by observing some effect it had on a global counter).

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, right, this was the odd thing about the RFC amendment.


static FOO: A = A(123);
const BAR: A = A(456);

impl A {
const BAZ: A = A(789);
}

fn main() {
println!("{}", &FOO.0);
assert_eq!(DROP_COUNTER.load(Ordering::SeqCst), 0);
assert_eq!(&FOO.0, &123);
assert_eq!(DROP_COUNTER.load(Ordering::SeqCst), 0);
assert_eq!(BAR.0, 456);
assert_eq!(DROP_COUNTER.load(Ordering::SeqCst), 1);
assert_eq!(A::BAZ.0, 789);
assert_eq!(DROP_COUNTER.load(Ordering::SeqCst), 2);
}
4 changes: 3 additions & 1 deletion src/test/ui/span/E0493.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#![feature(drop_types_in_const)]

struct Foo {
a: u32
}
Expand All @@ -24,7 +26,7 @@ impl Drop for Bar {
fn drop(&mut self) {}
}

const F : Foo = Foo { a : 0 };
const F : Foo = (Foo { a : 0 }, Foo { a : 1 }).1;

fn main() {
}
8 changes: 4 additions & 4 deletions src/test/ui/span/E0493.stderr
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
error[E0493]: constants are not allowed to have destructors
--> $DIR/E0493.rs:27:17
--> $DIR/E0493.rs:29:17
|
16 | fn drop(&mut self) {}
18 | fn drop(&mut self) {}
| --------------------- destructor defined here
...
27 | const F : Foo = Foo { a : 0 };
| ^^^^^^^^^^^^^ constants cannot have destructors
29 | const F : Foo = (Foo { a : 0 }, Foo { a : 1 }).1;
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ constants cannot have destructors

error: aborting due to previous error