Skip to content

Commit

Permalink
Auto merge of #24021 - pnkfelix:fn-params-outlive-body, r=nikomatsakis
Browse files Browse the repository at this point in the history
Encode more precise scoping rules for function params

Function params outlive everything in the body (incl temporaries).  Thus if we assign them their own `CodeExtent`, the region inference can properly show that it is sound to have temporaries with destructors that reference the parameters (because such temporaries will be dropped before the parameters are dropped).

Fix #23338
  • Loading branch information
bors committed Apr 8, 2015
2 parents 926f38e + 86c5faf commit 9266d59
Show file tree
Hide file tree
Showing 7 changed files with 301 additions and 8 deletions.
13 changes: 13 additions & 0 deletions src/librustc/metadata/tydecode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -369,6 +369,16 @@ fn parse_region_<F>(st: &mut PState, conv: &mut F) -> ty::Region where

fn parse_scope(st: &mut PState) -> region::CodeExtent {
match next(st) {
'P' => {
assert_eq!(next(st), '[');
let fn_id = parse_uint(st) as ast::NodeId;
assert_eq!(next(st), '|');
let body_id = parse_uint(st) as ast::NodeId;
assert_eq!(next(st), ']');
region::CodeExtent::ParameterScope {
fn_id: fn_id, body_id: body_id
}
}
'M' => {
let node_id = parse_uint(st) as ast::NodeId;
region::CodeExtent::Misc(node_id)
Expand All @@ -378,8 +388,11 @@ fn parse_scope(st: &mut PState) -> region::CodeExtent {
region::CodeExtent::DestructionScope(node_id)
}
'B' => {
assert_eq!(next(st), '[');
let node_id = parse_uint(st) as ast::NodeId;
assert_eq!(next(st), '|');
let first_stmt_index = parse_uint(st);
assert_eq!(next(st), ']');
let block_remainder = region::BlockRemainder {
block: node_id, first_statement_index: first_stmt_index,
};
Expand Down
4 changes: 3 additions & 1 deletion src/librustc/metadata/tyencode.rs
Original file line number Diff line number Diff line change
Expand Up @@ -275,9 +275,11 @@ pub fn enc_region(w: &mut Encoder, cx: &ctxt, r: ty::Region) {

fn enc_scope(w: &mut Encoder, _cx: &ctxt, scope: region::CodeExtent) {
match scope {
region::CodeExtent::ParameterScope {
fn_id, body_id } => mywrite!(w, "P[{}|{}]", fn_id, body_id),
region::CodeExtent::Misc(node_id) => mywrite!(w, "M{}", node_id),
region::CodeExtent::Remainder(region::BlockRemainder {
block: b, first_statement_index: i }) => mywrite!(w, "B{}{}", b, i),
block: b, first_statement_index: i }) => mywrite!(w, "B[{}|{}]", b, i),
region::CodeExtent::DestructionScope(node_id) => mywrite!(w, "D{}", node_id),
}
}
Expand Down
41 changes: 34 additions & 7 deletions src/librustc/middle/region.rs
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,15 @@ use syntax::visit::{Visitor, FnKind};
RustcDecodable, Debug, Copy)]
pub enum CodeExtent {
Misc(ast::NodeId),
DestructionScope(ast::NodeId), // extent of destructors for temporaries of node-id

// extent of parameters passed to a function or closure (they
// outlive its body)
ParameterScope { fn_id: ast::NodeId, body_id: ast::NodeId },

// extent of destructors for temporaries of node-id
DestructionScope(ast::NodeId),

// extent of code following a `let id = expr;` binding in a block
Remainder(BlockRemainder)
}

Expand Down Expand Up @@ -153,15 +161,19 @@ impl CodeExtent {
pub fn node_id(&self) -> ast::NodeId {
match *self {
CodeExtent::Misc(node_id) => node_id,

// These cases all return rough approximations to the
// precise extent denoted by `self`.
CodeExtent::Remainder(br) => br.block,
CodeExtent::DestructionScope(node_id) => node_id,
CodeExtent::ParameterScope { fn_id: _, body_id } => body_id,
}
}

/// Maps this scope to a potentially new one according to the
/// NodeId transformer `f_id`.
pub fn map_id<F>(&self, f_id: F) -> CodeExtent where
F: FnOnce(ast::NodeId) -> ast::NodeId,
pub fn map_id<F>(&self, mut f_id: F) -> CodeExtent where
F: FnMut(ast::NodeId) -> ast::NodeId,
{
match *self {
CodeExtent::Misc(node_id) => CodeExtent::Misc(f_id(node_id)),
Expand All @@ -170,6 +182,8 @@ impl CodeExtent {
block: f_id(br.block), first_statement_index: br.first_statement_index }),
CodeExtent::DestructionScope(node_id) =>
CodeExtent::DestructionScope(f_id(node_id)),
CodeExtent::ParameterScope { fn_id, body_id } =>
CodeExtent::ParameterScope { fn_id: f_id(fn_id), body_id: f_id(body_id) },
}
}

Expand All @@ -180,6 +194,7 @@ impl CodeExtent {
match ast_map.find(self.node_id()) {
Some(ast_map::NodeBlock(ref blk)) => {
match *self {
CodeExtent::ParameterScope { .. } |
CodeExtent::Misc(_) |
CodeExtent::DestructionScope(_) => Some(blk.span),

Expand Down Expand Up @@ -277,6 +292,7 @@ enum InnermostDeclaringBlock {
Block(ast::NodeId),
Statement(DeclaringStatementContext),
Match(ast::NodeId),
FnDecl { fn_id: ast::NodeId, body_id: ast::NodeId },
}

impl InnermostDeclaringBlock {
Expand All @@ -285,6 +301,8 @@ impl InnermostDeclaringBlock {
InnermostDeclaringBlock::None => {
return Option::None;
}
InnermostDeclaringBlock::FnDecl { fn_id, body_id } =>
CodeExtent::ParameterScope { fn_id: fn_id, body_id: body_id },
InnermostDeclaringBlock::Block(id) |
InnermostDeclaringBlock::Match(id) => CodeExtent::from_node_id(id),
InnermostDeclaringBlock::Statement(s) => s.to_code_extent(),
Expand Down Expand Up @@ -1198,25 +1216,34 @@ fn resolve_fn(visitor: &mut RegionResolutionVisitor,
body.id,
visitor.cx.parent);

// This scope covers the function body, which includes the
// bindings introduced by let statements as well as temporaries
// created by the fn's tail expression (if any). It does *not*
// include the fn parameters (see below).
let body_scope = CodeExtent::from_node_id(body.id);
visitor.region_maps.mark_as_terminating_scope(body_scope);

let dtor_scope = CodeExtent::DestructionScope(body.id);
visitor.region_maps.record_encl_scope(body_scope, dtor_scope);

record_superlifetime(visitor, dtor_scope, body.span);
let fn_decl_scope = CodeExtent::ParameterScope { fn_id: id, body_id: body.id };
visitor.region_maps.record_encl_scope(dtor_scope, fn_decl_scope);

record_superlifetime(visitor, fn_decl_scope, body.span);

if let Some(root_id) = visitor.cx.root_id {
visitor.region_maps.record_fn_parent(body.id, root_id);
}

let outer_cx = visitor.cx;

// The arguments and `self` are parented to the body of the fn.
// The arguments and `self` are parented to the fn.
visitor.cx = Context {
root_id: Some(body.id),
parent: InnermostEnclosingExpr::Some(body.id),
var_parent: InnermostDeclaringBlock::Block(body.id)
parent: InnermostEnclosingExpr::None,
var_parent: InnermostDeclaringBlock::FnDecl {
fn_id: id, body_id: body.id
},
};
visit::walk_fn_decl(visitor, decl);

Expand Down
5 changes: 5 additions & 0 deletions src/librustc/util/ppaux.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,9 @@ pub fn explain_region_and_span(cx: &ctxt, region: ty::Region)
};
let scope_decorated_tag = match scope {
region::CodeExtent::Misc(_) => tag,
region::CodeExtent::ParameterScope { .. } => {
"scope of parameters for function"
}
region::CodeExtent::DestructionScope(_) => {
new_string = format!("destruction scope surrounding {}", tag);
&*new_string
Expand Down Expand Up @@ -952,6 +955,8 @@ impl<'tcx> Repr<'tcx> for ty::FreeRegion {
impl<'tcx> Repr<'tcx> for region::CodeExtent {
fn repr(&self, _tcx: &ctxt) -> String {
match *self {
region::CodeExtent::ParameterScope { fn_id, body_id } =>
format!("ParameterScope({}, {})", fn_id, body_id),
region::CodeExtent::Misc(node_id) =>
format!("Misc({})", node_id),
region::CodeExtent::DestructionScope(node_id) =>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// This is just checking that we still reject code where temp values
// are borrowing values for longer than they will be around.
//
// Compare to run-pass/issue-23338-params-outlive-temps-of-body.rs

use std::cell::RefCell;

fn foo(x: RefCell<String>) -> String {
let y = x;
y.borrow().clone() //~ ERROR `y` does not live long enough
}

fn foo2(x: RefCell<String>) -> String {
let ret = {
let y = x;
y.borrow().clone() //~ ERROR `y` does not live long enough
};
ret
}

fn main() {
let r = RefCell::new(format!("data"));
assert_eq!(foo(r), "data");
let r = RefCell::new(format!("data"));
assert_eq!(foo2(r), "data");
}
171 changes: 171 additions & 0 deletions src/test/run-pass/issue-23338-ensure-param-drop-order.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,171 @@
// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// ignore-pretty : (#23623) problems when ending with // comments

// This test is ensuring that parameters are indeed dropped after
// temporaries in a fn body.

use std::cell::RefCell;

use self::d::D;

pub fn main() {
let log = RefCell::new(vec![]);
d::println(&format!("created empty log"));
test(&log);

assert_eq!(&log.borrow()[..],
[
// created empty log
// +-- Make D(da_0, 0)
// | +-- Make D(de_1, 1)
// | | calling foo
// | | entered foo
// | | +-- Make D(de_2, 2)
// | | | +-- Make D(da_1, 3)
// | | | | +-- Make D(de_3, 4)
// | | | | | +-- Make D(de_4, 5)
3, // | | | +-- Drop D(da_1, 3)
// | | | | |
4, // | | | +-- Drop D(de_3, 4)
// | | | |
// | | | | eval tail of foo
// | | | +-- Make D(de_5, 6)
// | | | | +-- Make D(de_6, 7)
6, // | | | +-- Drop D(de_5, 6)
// | | | | |
5, // | | | | +-- Drop D(de_4, 5)
// | | | |
2, // | | +-- Drop D(de_2, 2)
// | | |
1, // | +-- Drop D(de_1, 1)
// | |
0, // +-- Drop D(da_0, 0)
// |
// | result D(de_6, 7)
7 // +-- Drop D(de_6, 7)

]);
}

fn test<'a>(log: d::Log<'a>) {
let da = D::new("da", 0, log);
let de = D::new("de", 1, log);
d::println(&format!("calling foo"));
let result = foo(da, de);
d::println(&format!("result {}", result));
}

fn foo<'a>(da0: D<'a>, de1: D<'a>) -> D<'a> {
d::println(&format!("entered foo"));
let de2 = de1.incr(); // creates D(de_2, 2)
let de4 = {
let _da1 = da0.incr(); // creates D(da_1, 3)
de2.incr().incr() // creates D(de_3, 4) and D(de_4, 5)
};
d::println(&format!("eval tail of foo"));
de4.incr().incr() // creates D(de_5, 6) and D(de_6, 7)
}

// This module provides simultaneous printouts of the dynamic extents
// of all of the D values, in addition to logging the order that each
// is dropped.

const PREF_INDENT: u32 = 16;

pub mod d {
#![allow(unused_parens)]
use std::fmt;
use std::mem;
use std::cell::RefCell;

static mut counter: u32 = 0;
static mut trails: u64 = 0;

pub type Log<'a> = &'a RefCell<Vec<u32>>;

pub fn current_width() -> u32 {
unsafe { max_width() - trails.leading_zeros() }
}

pub fn max_width() -> u32 {
unsafe {
(mem::size_of_val(&trails)*8) as u32
}
}

pub fn indent_println(my_trails: u32, s: &str) {
let mut indent: String = String::new();
for i in 0..my_trails {
unsafe {
if trails & (1 << i) != 0 {
indent = indent + "| ";
} else {
indent = indent + " ";
}
}
}
println!("{}{}", indent, s);
}

pub fn println(s: &str) {
indent_println(super::PREF_INDENT, s);
}

fn first_avail() -> u32 {
unsafe {
for i in 0..64 {
if trails & (1 << i) == 0 {
return i;
}
}
}
panic!("exhausted trails");
}

pub struct D<'a> {
name: &'static str, i: u32, uid: u32, trail: u32, log: Log<'a>
}

impl<'a> fmt::Display for D<'a> {
fn fmt(&self, w: &mut fmt::Formatter) -> fmt::Result {
write!(w, "D({}_{}, {})", self.name, self.i, self.uid)
}
}

impl<'a> D<'a> {
pub fn new(name: &'static str, i: u32, log: Log<'a>) -> D<'a> {
unsafe {
let trail = first_avail();
let ctr = counter;
counter += 1;
trails |= (1 << trail);
let ret = D {
name: name, i: i, log: log, uid: ctr, trail: trail
};
indent_println(trail, &format!("+-- Make {}", ret));
ret
}
}
pub fn incr(&self) -> D<'a> {
D::new(self.name, self.i + 1, self.log)
}
}

impl<'a> Drop for D<'a> {
fn drop(&mut self) {
unsafe { trails &= !(1 << self.trail); };
self.log.borrow_mut().push(self.uid);
indent_println(self.trail, &format!("+-- Drop {}", self));
indent_println(::PREF_INDENT, "");
}
}
}
Loading

0 comments on commit 9266d59

Please sign in to comment.