Skip to content

Commit

Permalink
Auto merge of #43932 - eddyb:const-scoping, r=nikomatsakis
Browse files Browse the repository at this point in the history
Forward-compatibly deny drops in constants if they *could* actually run.

This is part of #40036, specifically the checks for user-defined destructor invocations on locals which *may not* have been moved away, the motivating example being:
```rust
const FOO: i32 = (HasDrop {...}, 0).1;
```
The evaluation of constant MIR will continue to create `'static` slots for more locals than is necessary (if `Storage{Live,Dead}` statements are ignored), but it shouldn't be misusable.

r? @nikomatsakis
  • Loading branch information
bors committed Aug 30, 2017
2 parents 51a54b6 + c76a024 commit 7eeac1b
Show file tree
Hide file tree
Showing 6 changed files with 177 additions and 54 deletions.
76 changes: 50 additions & 26 deletions src/librustc/middle/region.rs
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,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 @@ -358,9 +360,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 @@ -389,7 +393,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 @@ -803,16 +807,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 @@ -872,15 +871,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 @@ -954,7 +960,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 @@ -1004,7 +1010,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 @@ -1077,12 +1083,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 @@ -1100,7 +1101,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 @@ -1120,7 +1144,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
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 @@ -560,22 +581,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 @@ -866,6 +897,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.
let needs_drop = if let Lvalue::Local(local) = *lvalue {
self.local_needs_drop[local]
} 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
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
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

0 comments on commit 7eeac1b

Please sign in to comment.