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

Forward-compatibly deny drops in constants if they *could* actually run. #43932

Merged
merged 3 commits into from
Aug 30, 2017
Merged

Forward-compatibly deny drops in constants if they *could* actually run. #43932

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b8c05fe
rustc: use rvalue scope semantics for constant initializers.
eddyb Jul 21, 2017
9b61771
rustc_mir: conservatively deny non-noop drops in constant contexts.
eddyb Jul 22, 2017
c76a024
rustc: treat impl associated consts like const items for constness.
eddyb Aug 26, 2017
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
76 changes: 50 additions & 26 deletions src/librustc/middle/region.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,7 +221,9 @@ pub struct RegionMaps {
/// table, the appropriate cleanup scope is the innermost
/// enclosing statement, conditional expression, or repeating
/// block (see `terminating_scopes`).
rvalue_scopes: NodeMap<CodeExtent>,
/// In constants, None is used to indicate that certain expressions
/// escape into 'static and should have no local cleanup scope.
rvalue_scopes: NodeMap<Option<CodeExtent>>,

/// Encodes the hierarchy of fn bodies. Every fn body (including
/// closures) forms its own distinct region hierarchy, rooted in
Expand Down Expand Up @@ -356,9 +358,11 @@ impl<'tcx> RegionMaps {
self.var_map.insert(var, lifetime);
}

fn record_rvalue_scope(&mut self, var: ast::NodeId, lifetime: CodeExtent) {
fn record_rvalue_scope(&mut self, var: ast::NodeId, lifetime: Option<CodeExtent>) {
debug!("record_rvalue_scope(sub={:?}, sup={:?})", var, lifetime);
assert!(var != lifetime.node_id());
if let Some(lifetime) = lifetime {
assert!(var != lifetime.node_id());
}
self.rvalue_scopes.insert(var, lifetime);
}

Expand Down Expand Up @@ -387,7 +391,7 @@ impl<'tcx> RegionMaps {
// check for a designated rvalue scope
if let Some(&s) = self.rvalue_scopes.get(&expr_id) {
debug!("temporary_scope({:?}) = {:?} [custom]", expr_id, s);
return Some(s);
return s;
}

// else, locate the innermost terminating scope
Expand Down Expand Up @@ -801,16 +805,11 @@ fn resolve_expr<'a, 'tcx>(visitor: &mut RegionResolutionVisitor<'a, 'tcx>, expr:
}

fn resolve_local<'a, 'tcx>(visitor: &mut RegionResolutionVisitor<'a, 'tcx>,
local: &'tcx hir::Local) {
debug!("resolve_local(local.id={:?},local.init={:?})",
local.id,local.init.is_some());
pat: Option<&'tcx hir::Pat>,
init: Option<&'tcx hir::Expr>) {
debug!("resolve_local(pat={:?}, init={:?})", pat, init);

// For convenience in trans, associate with the local-id the var
// scope that will be used for any bindings declared in this
// pattern.
let blk_scope = visitor.cx.var_parent;
let blk_scope = blk_scope.expect("locals must be within a block");
visitor.region_maps.record_var_scope(local.id, blk_scope);

// As an exception to the normal rules governing temporary
// lifetimes, initializers in a let have a temporary lifetime
Expand Down Expand Up @@ -870,15 +869,22 @@ fn resolve_local<'a, 'tcx>(visitor: &mut RegionResolutionVisitor<'a, 'tcx>,
//
// FIXME(#6308) -- Note that `[]` patterns work more smoothly post-DST.

if let Some(ref expr) = local.init {
if let Some(expr) = init {
record_rvalue_scope_if_borrow_expr(visitor, &expr, blk_scope);

if is_binding_pat(&local.pat) {
record_rvalue_scope(visitor, &expr, blk_scope);
if let Some(pat) = pat {
if is_binding_pat(pat) {
record_rvalue_scope(visitor, &expr, blk_scope);
}
}
}

intravisit::walk_local(visitor, local);
if let Some(pat) = pat {
visitor.visit_pat(pat);
}
if let Some(expr) = init {
visitor.visit_expr(expr);
}

/// True if `pat` match the `P&` nonterminal:
///
Expand Down Expand Up @@ -952,7 +958,7 @@ fn resolve_local<'a, 'tcx>(visitor: &mut RegionResolutionVisitor<'a, 'tcx>,
fn record_rvalue_scope_if_borrow_expr<'a, 'tcx>(
visitor: &mut RegionResolutionVisitor<'a, 'tcx>,
expr: &hir::Expr,
blk_id: CodeExtent)
blk_id: Option<CodeExtent>)
{
match expr.node {
hir::ExprAddrOf(_, ref subexpr) => {
Expand Down Expand Up @@ -1002,7 +1008,7 @@ fn resolve_local<'a, 'tcx>(visitor: &mut RegionResolutionVisitor<'a, 'tcx>,
/// Note: ET is intended to match "rvalues or lvalues based on rvalues".
fn record_rvalue_scope<'a, 'tcx>(visitor: &mut RegionResolutionVisitor<'a, 'tcx>,
expr: &hir::Expr,
blk_scope: CodeExtent) {
blk_scope: Option<CodeExtent>) {
let mut expr = expr;
loop {
// Note: give all the expressions matching `ET` with the
Expand Down Expand Up @@ -1075,12 +1081,7 @@ impl<'a, 'tcx> Visitor<'tcx> for RegionResolutionVisitor<'a, 'tcx> {

let outer_cx = self.cx;
let outer_ts = mem::replace(&mut self.terminating_scopes, NodeSet());

// Only functions have an outer terminating (drop) scope,
// while temporaries in constant initializers are 'static.
if let MirSource::Fn(_) = MirSource::from_node(self.tcx, owner_id) {
self.terminating_scopes.insert(body_id.node_id);
}
self.terminating_scopes.insert(body_id.node_id);

if let Some(root_id) = self.cx.root_id {
self.region_maps.record_fn_parent(body_id.node_id, root_id);
Expand All @@ -1098,7 +1099,30 @@ impl<'a, 'tcx> Visitor<'tcx> for RegionResolutionVisitor<'a, 'tcx> {

// The body of the every fn is a root scope.
self.cx.parent = self.cx.var_parent;
self.visit_expr(&body.value);
if let MirSource::Fn(_) = MirSource::from_node(self.tcx, owner_id) {
self.visit_expr(&body.value);
} else {
// Only functions have an outer terminating (drop) scope, while
// temporaries in constant initializers may be 'static, but only
// according to rvalue lifetime semantics, using the same
// syntactical rules used for let initializers.
//
// E.g. in `let x = &f();`, the temporary holding the result from
// the `f()` call lives for the entirety of the surrounding block.
//
// Similarly, `const X: ... = &f();` would have the result of `f()`
// live for `'static`, implying (if Drop restrictions on constants
// ever get lifted) that the value *could* have a destructor, but
// it'd get leaked instead of the destructor running during the
// evaluation of `X` (if at all allowed by CTFE).
//
// However, `const Y: ... = g(&f());`, like `let y = g(&f());`,
// would *not* let the `f()` temporary escape into an outer scope
// (i.e. `'static`), which means that after `g` returns, it drops,
// and all the associated destruction scope rules apply.
self.cx.var_parent = None;
resolve_local(self, None, Some(&body.value));
}

// Restore context we had at the start.
self.cx = outer_cx;
Expand All @@ -1118,7 +1142,7 @@ impl<'a, 'tcx> Visitor<'tcx> for RegionResolutionVisitor<'a, 'tcx> {
resolve_expr(self, ex);
}
fn visit_local(&mut self, l: &'tcx Local) {
resolve_local(self, l);
resolve_local(self, Some(&l.pat), l.init.as_ref().map(|e| &**e));
}
}

Expand Down
71 changes: 63 additions & 8 deletions src/librustc_mir/transform/qualify_consts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,7 @@ struct Qualifier<'a, 'gcx: 'a+'tcx, 'tcx: 'a> {
return_qualif: Option<Qualif>,
qualif: Qualif,
const_fn_arg_vars: BitVector,
local_needs_drop: IndexVec<Local, Option<Span>>,
temp_promotion_state: IndexVec<Local, TempState>,
promotion_candidates: Vec<Candidate>
}
Expand All @@ -146,6 +147,7 @@ impl<'a, 'tcx> Qualifier<'a, 'tcx, 'tcx> {
return_qualif: None,
qualif: Qualif::empty(),
const_fn_arg_vars: BitVector::new(mir.local_decls.len()),
local_needs_drop: IndexVec::from_elem(None, &mir.local_decls),
temp_promotion_state: temps,
promotion_candidates: vec![]
}
Expand Down Expand Up @@ -193,16 +195,26 @@ impl<'a, 'tcx> Qualifier<'a, 'tcx, 'tcx> {
self.add(original);
}

/// Check for NEEDS_DROP (from an ADT or const fn call) and
/// error, unless we're in a function.
fn always_deny_drop(&self) {
self.deny_drop_with_feature_gate_override(false);
}

/// 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.
fn deny_drop(&self) {
self.deny_drop_with_feature_gate_override(true);
}

fn deny_drop_with_feature_gate_override(&self, allow_gate: bool) {
if self.mode == Mode::Fn || !self.qualif.intersects(Qualif::NEEDS_DROP) {
return;
}

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

if self.mode != Mode::Const {
if allow_gate && self.mode != Mode::Const {
help!(&mut err,
"in Nightly builds, add `#![feature(drop_types_in_const)]` \
to the crate attributes to enable");
} else {
self.find_drop_implementation_method_span()
.map(|span| err.span_label(span, "destructor defined here"));

err.span_label(self.span, "constants cannot have destructors");
err.span_label(self.span,
format!("{}s cannot have destructors", self.mode));
}

err.emit();
Expand Down Expand Up @@ -314,6 +327,15 @@ impl<'a, 'tcx> Qualifier<'a, 'tcx, 'tcx> {
return;
}

// When initializing a local, record whether the *value* being
// stored in it needs dropping, which it may not, even if its
// type does, e.g. `None::<String>`.
if let Lvalue::Local(local) = *dest {
if qualif.intersects(Qualif::NEEDS_DROP) {
self.local_needs_drop[local] = Some(self.span);
}
}

match *dest {
Lvalue::Local(index) if self.mir.local_kind(index) == LocalKind::Temp => {
debug!("store to temp {:?}", index);
Expand Down Expand Up @@ -360,7 +382,6 @@ impl<'a, 'tcx> Qualifier<'a, 'tcx, 'tcx> {

let target = match mir[bb].terminator().kind {
TerminatorKind::Goto { target } |
// Drops are considered noops.
TerminatorKind::Drop { target, .. } |
TerminatorKind::Assert { target, .. } |
TerminatorKind::Call { destination: Some((_, target)), .. } => {
Expand Down Expand Up @@ -558,22 +579,32 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> {

fn visit_operand(&mut self, operand: &Operand<'tcx>, location: Location) {
match *operand {
Operand::Consume(_) => {
Operand::Consume(ref lvalue) => {
self.nest(|this| {
this.super_operand(operand, location);
this.try_consume();
});

// Mark the consumed locals to indicate later drops are noops.
if let Lvalue::Local(local) = *lvalue {
self.local_needs_drop[local] = None;
}
}
Operand::Constant(ref constant) => {
if let Literal::Item { def_id, substs } = constant.literal {
// Don't peek inside generic (associated) constants.
if substs.types().next().is_some() {
if let Literal::Item { def_id, substs: _ } = constant.literal {
// Don't peek inside trait associated constants.
if self.tcx.trait_of_item(def_id).is_some() {
self.add_type(constant.ty);
} else {
let bits = self.tcx.at(constant.span).mir_const_qualif(def_id);

let qualif = Qualif::from_bits(bits).expect("invalid mir_const_qualif");
self.add(qualif);

// Just in case the type is more specific than
// the definition, e.g. impl associated const
// with type parameters, take it into account.
self.qualif.restrict(constant.ty, self.tcx, self.param_env);
}

// Let `const fn` transitively have destructors,
Expand Down Expand Up @@ -864,6 +895,30 @@ impl<'a, 'tcx> Visitor<'tcx> for Qualifier<'a, 'tcx, 'tcx> {
}
self.assign(dest, location);
}
} else if let TerminatorKind::Drop { location: ref lvalue, .. } = *kind {
self.super_terminator_kind(bb, kind, location);

// Deny *any* live drops anywhere other than functions.
if self.mode != Mode::Fn {
// HACK(eddyb) Emulate a bit of dataflow analysis,
// conservatively, that drop elaboration will do.
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not really "drop elaboration" dataflow - drop elaboration does not know that None has no destructor - but rather "const qual" dataflow.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, it's a combination of the two.

let needs_drop = if let Lvalue::Local(local) = *lvalue {
self.local_needs_drop[local]
Copy link
Contributor

Choose a reason for hiding this comment

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

I presume that we will reject (for constants) things that mutate "MIR locals" once they are assigned?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that is considered an assignment and disallowed as "statement-like".

} else {
None
};

if let Some(span) = needs_drop {
let ty = lvalue.ty(self.mir, self.tcx).to_ty(self.tcx);
self.add_type(ty);

// Use the original assignment span to be more precise.
let old_span = self.span;
self.span = span;
self.always_deny_drop();
self.span = old_span;
}
}
} else {
// Qualify any operands inside other terminators.
self.super_terminator_kind(bb, kind, location);
Expand Down
37 changes: 21 additions & 16 deletions src/librustc_passes/consts.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,19 +87,14 @@ impl<'a, 'gcx> CheckCrateVisitor<'a, 'gcx> {
}
}

// Adds the worst effect out of all the values of one type.
fn add_type(&mut self, ty: Ty<'gcx>) {
if !ty.is_freeze(self.tcx, self.param_env, DUMMY_SP) {
self.promotable = false;
}

if ty.needs_drop(self.tcx, self.param_env) {
self.promotable = false;
}
// Returns true iff all the values of the type are promotable.
fn type_has_only_promotable_values(&mut self, ty: Ty<'gcx>) -> bool {
ty.is_freeze(self.tcx, self.param_env, DUMMY_SP) &&
!ty.needs_drop(self.tcx, self.param_env)
}

fn handle_const_fn_call(&mut self, def_id: DefId, ret_ty: Ty<'gcx>) {
self.add_type(ret_ty);
self.promotable &= self.type_has_only_promotable_values(ret_ty);

self.promotable &= if let Some(fn_id) = self.tcx.hir.as_local_node_id(def_id) {
FnLikeNode::from_node(self.tcx.hir.get(fn_id)).map_or(false, |fn_like| {
Expand Down Expand Up @@ -333,20 +328,30 @@ fn check_expr<'a, 'tcx>(v: &mut CheckCrateVisitor<'a, 'tcx>, e: &hir::Expr, node
match def {
Def::VariantCtor(..) | Def::StructCtor(..) |
Def::Fn(..) | Def::Method(..) => {}
Def::AssociatedConst(_) => v.add_type(node_ty),
Def::Const(did) => {
v.promotable &= if let Some(node_id) = v.tcx.hir.as_local_node_id(did) {
match v.tcx.hir.expect_item(node_id).node {
hir::ItemConst(_, body) => {

Def::Const(did) |
Def::AssociatedConst(did) => {
let promotable = if v.tcx.trait_of_item(did).is_some() {
// Don't peek inside trait associated constants.
false
} else if let Some(node_id) = v.tcx.hir.as_local_node_id(did) {
match v.tcx.hir.maybe_body_owned_by(node_id) {
Some(body) => {
v.visit_nested_body(body);
v.tcx.rvalue_promotable_to_static.borrow()[&body.node_id]
}
_ => false
None => false
}
} else {
v.tcx.const_is_rvalue_promotable_to_static(did)
};

// Just in case the type is more specific than the definition,
// e.g. impl associated const with type parameters, check it.
// Also, trait associated consts are relaxed by this.
v.promotable &= promotable || v.type_has_only_promotable_values(node_ty);
}

_ => {
v.promotable = false;
}
Expand Down
3 changes: 2 additions & 1 deletion src/test/compile-fail/check-static-values-constraints.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,9 @@ static STATIC8: SafeStruct = SafeStruct{field1: SafeEnum::Variant1,
// This example should fail because field1 in the base struct is not safe
static STATIC9: SafeStruct = SafeStruct{field1: SafeEnum::Variant1,
..SafeStruct{field1: SafeEnum::Variant3(WithDtor),
//~^ ERROR destructors in statics are an unstable feature
//~| ERROR statics are not allowed to have destructors
field2: SafeEnum::Variant1}};
//~^^ ERROR destructors in statics are an unstable feature

struct UnsafeStruct;

Expand Down
Loading