Skip to content

Commit 9266d59

Browse files
committed
Auto merge of #24021 - pnkfelix:fn-params-outlive-body, r=nikomatsakis
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
2 parents 926f38e + 86c5faf commit 9266d59

7 files changed

+301
-8
lines changed

src/librustc/metadata/tydecode.rs

+13
Original file line numberDiff line numberDiff line change
@@ -369,6 +369,16 @@ fn parse_region_<F>(st: &mut PState, conv: &mut F) -> ty::Region where
369369

370370
fn parse_scope(st: &mut PState) -> region::CodeExtent {
371371
match next(st) {
372+
'P' => {
373+
assert_eq!(next(st), '[');
374+
let fn_id = parse_uint(st) as ast::NodeId;
375+
assert_eq!(next(st), '|');
376+
let body_id = parse_uint(st) as ast::NodeId;
377+
assert_eq!(next(st), ']');
378+
region::CodeExtent::ParameterScope {
379+
fn_id: fn_id, body_id: body_id
380+
}
381+
}
372382
'M' => {
373383
let node_id = parse_uint(st) as ast::NodeId;
374384
region::CodeExtent::Misc(node_id)
@@ -378,8 +388,11 @@ fn parse_scope(st: &mut PState) -> region::CodeExtent {
378388
region::CodeExtent::DestructionScope(node_id)
379389
}
380390
'B' => {
391+
assert_eq!(next(st), '[');
381392
let node_id = parse_uint(st) as ast::NodeId;
393+
assert_eq!(next(st), '|');
382394
let first_stmt_index = parse_uint(st);
395+
assert_eq!(next(st), ']');
383396
let block_remainder = region::BlockRemainder {
384397
block: node_id, first_statement_index: first_stmt_index,
385398
};

src/librustc/metadata/tyencode.rs

+3-1
Original file line numberDiff line numberDiff line change
@@ -275,9 +275,11 @@ pub fn enc_region(w: &mut Encoder, cx: &ctxt, r: ty::Region) {
275275

276276
fn enc_scope(w: &mut Encoder, _cx: &ctxt, scope: region::CodeExtent) {
277277
match scope {
278+
region::CodeExtent::ParameterScope {
279+
fn_id, body_id } => mywrite!(w, "P[{}|{}]", fn_id, body_id),
278280
region::CodeExtent::Misc(node_id) => mywrite!(w, "M{}", node_id),
279281
region::CodeExtent::Remainder(region::BlockRemainder {
280-
block: b, first_statement_index: i }) => mywrite!(w, "B{}{}", b, i),
282+
block: b, first_statement_index: i }) => mywrite!(w, "B[{}|{}]", b, i),
281283
region::CodeExtent::DestructionScope(node_id) => mywrite!(w, "D{}", node_id),
282284
}
283285
}

src/librustc/middle/region.rs

+34-7
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,15 @@ use syntax::visit::{Visitor, FnKind};
9595
RustcDecodable, Debug, Copy)]
9696
pub enum CodeExtent {
9797
Misc(ast::NodeId),
98-
DestructionScope(ast::NodeId), // extent of destructors for temporaries of node-id
98+
99+
// extent of parameters passed to a function or closure (they
100+
// outlive its body)
101+
ParameterScope { fn_id: ast::NodeId, body_id: ast::NodeId },
102+
103+
// extent of destructors for temporaries of node-id
104+
DestructionScope(ast::NodeId),
105+
106+
// extent of code following a `let id = expr;` binding in a block
99107
Remainder(BlockRemainder)
100108
}
101109

@@ -153,15 +161,19 @@ impl CodeExtent {
153161
pub fn node_id(&self) -> ast::NodeId {
154162
match *self {
155163
CodeExtent::Misc(node_id) => node_id,
164+
165+
// These cases all return rough approximations to the
166+
// precise extent denoted by `self`.
156167
CodeExtent::Remainder(br) => br.block,
157168
CodeExtent::DestructionScope(node_id) => node_id,
169+
CodeExtent::ParameterScope { fn_id: _, body_id } => body_id,
158170
}
159171
}
160172

161173
/// Maps this scope to a potentially new one according to the
162174
/// NodeId transformer `f_id`.
163-
pub fn map_id<F>(&self, f_id: F) -> CodeExtent where
164-
F: FnOnce(ast::NodeId) -> ast::NodeId,
175+
pub fn map_id<F>(&self, mut f_id: F) -> CodeExtent where
176+
F: FnMut(ast::NodeId) -> ast::NodeId,
165177
{
166178
match *self {
167179
CodeExtent::Misc(node_id) => CodeExtent::Misc(f_id(node_id)),
@@ -170,6 +182,8 @@ impl CodeExtent {
170182
block: f_id(br.block), first_statement_index: br.first_statement_index }),
171183
CodeExtent::DestructionScope(node_id) =>
172184
CodeExtent::DestructionScope(f_id(node_id)),
185+
CodeExtent::ParameterScope { fn_id, body_id } =>
186+
CodeExtent::ParameterScope { fn_id: f_id(fn_id), body_id: f_id(body_id) },
173187
}
174188
}
175189

@@ -180,6 +194,7 @@ impl CodeExtent {
180194
match ast_map.find(self.node_id()) {
181195
Some(ast_map::NodeBlock(ref blk)) => {
182196
match *self {
197+
CodeExtent::ParameterScope { .. } |
183198
CodeExtent::Misc(_) |
184199
CodeExtent::DestructionScope(_) => Some(blk.span),
185200

@@ -277,6 +292,7 @@ enum InnermostDeclaringBlock {
277292
Block(ast::NodeId),
278293
Statement(DeclaringStatementContext),
279294
Match(ast::NodeId),
295+
FnDecl { fn_id: ast::NodeId, body_id: ast::NodeId },
280296
}
281297

282298
impl InnermostDeclaringBlock {
@@ -285,6 +301,8 @@ impl InnermostDeclaringBlock {
285301
InnermostDeclaringBlock::None => {
286302
return Option::None;
287303
}
304+
InnermostDeclaringBlock::FnDecl { fn_id, body_id } =>
305+
CodeExtent::ParameterScope { fn_id: fn_id, body_id: body_id },
288306
InnermostDeclaringBlock::Block(id) |
289307
InnermostDeclaringBlock::Match(id) => CodeExtent::from_node_id(id),
290308
InnermostDeclaringBlock::Statement(s) => s.to_code_extent(),
@@ -1198,25 +1216,34 @@ fn resolve_fn(visitor: &mut RegionResolutionVisitor,
11981216
body.id,
11991217
visitor.cx.parent);
12001218

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

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

1207-
record_superlifetime(visitor, dtor_scope, body.span);
1229+
let fn_decl_scope = CodeExtent::ParameterScope { fn_id: id, body_id: body.id };
1230+
visitor.region_maps.record_encl_scope(dtor_scope, fn_decl_scope);
1231+
1232+
record_superlifetime(visitor, fn_decl_scope, body.span);
12081233

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

12131238
let outer_cx = visitor.cx;
12141239

1215-
// The arguments and `self` are parented to the body of the fn.
1240+
// The arguments and `self` are parented to the fn.
12161241
visitor.cx = Context {
12171242
root_id: Some(body.id),
1218-
parent: InnermostEnclosingExpr::Some(body.id),
1219-
var_parent: InnermostDeclaringBlock::Block(body.id)
1243+
parent: InnermostEnclosingExpr::None,
1244+
var_parent: InnermostDeclaringBlock::FnDecl {
1245+
fn_id: id, body_id: body.id
1246+
},
12201247
};
12211248
visit::walk_fn_decl(visitor, decl);
12221249

src/librustc/util/ppaux.rs

+5
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,9 @@ pub fn explain_region_and_span(cx: &ctxt, region: ty::Region)
113113
};
114114
let scope_decorated_tag = match scope {
115115
region::CodeExtent::Misc(_) => tag,
116+
region::CodeExtent::ParameterScope { .. } => {
117+
"scope of parameters for function"
118+
}
116119
region::CodeExtent::DestructionScope(_) => {
117120
new_string = format!("destruction scope surrounding {}", tag);
118121
&*new_string
@@ -952,6 +955,8 @@ impl<'tcx> Repr<'tcx> for ty::FreeRegion {
952955
impl<'tcx> Repr<'tcx> for region::CodeExtent {
953956
fn repr(&self, _tcx: &ctxt) -> String {
954957
match *self {
958+
region::CodeExtent::ParameterScope { fn_id, body_id } =>
959+
format!("ParameterScope({}, {})", fn_id, body_id),
955960
region::CodeExtent::Misc(node_id) =>
956961
format!("Misc({})", node_id),
957962
region::CodeExtent::DestructionScope(node_id) =>
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
2+
// file at the top-level directory of this distribution and at
3+
// http://rust-lang.org/COPYRIGHT.
4+
//
5+
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
6+
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
7+
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
8+
// option. This file may not be copied, modified, or distributed
9+
// except according to those terms.
10+
11+
// This is just checking that we still reject code where temp values
12+
// are borrowing values for longer than they will be around.
13+
//
14+
// Compare to run-pass/issue-23338-params-outlive-temps-of-body.rs
15+
16+
use std::cell::RefCell;
17+
18+
fn foo(x: RefCell<String>) -> String {
19+
let y = x;
20+
y.borrow().clone() //~ ERROR `y` does not live long enough
21+
}
22+
23+
fn foo2(x: RefCell<String>) -> String {
24+
let ret = {
25+
let y = x;
26+
y.borrow().clone() //~ ERROR `y` does not live long enough
27+
};
28+
ret
29+
}
30+
31+
fn main() {
32+
let r = RefCell::new(format!("data"));
33+
assert_eq!(foo(r), "data");
34+
let r = RefCell::new(format!("data"));
35+
assert_eq!(foo2(r), "data");
36+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,171 @@
1+
// Copyright 2015 The Rust Project Developers. See the COPYRIGHT
2+
// file at the top-level directory of this distribution and at
3+
// http://rust-lang.org/COPYRIGHT.
4+
//
5+
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
6+
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
7+
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
8+
// option. This file may not be copied, modified, or distributed
9+
// except according to those terms.
10+
11+
// ignore-pretty : (#23623) problems when ending with // comments
12+
13+
// This test is ensuring that parameters are indeed dropped after
14+
// temporaries in a fn body.
15+
16+
use std::cell::RefCell;
17+
18+
use self::d::D;
19+
20+
pub fn main() {
21+
let log = RefCell::new(vec![]);
22+
d::println(&format!("created empty log"));
23+
test(&log);
24+
25+
assert_eq!(&log.borrow()[..],
26+
[
27+
// created empty log
28+
// +-- Make D(da_0, 0)
29+
// | +-- Make D(de_1, 1)
30+
// | | calling foo
31+
// | | entered foo
32+
// | | +-- Make D(de_2, 2)
33+
// | | | +-- Make D(da_1, 3)
34+
// | | | | +-- Make D(de_3, 4)
35+
// | | | | | +-- Make D(de_4, 5)
36+
3, // | | | +-- Drop D(da_1, 3)
37+
// | | | | |
38+
4, // | | | +-- Drop D(de_3, 4)
39+
// | | | |
40+
// | | | | eval tail of foo
41+
// | | | +-- Make D(de_5, 6)
42+
// | | | | +-- Make D(de_6, 7)
43+
6, // | | | +-- Drop D(de_5, 6)
44+
// | | | | |
45+
5, // | | | | +-- Drop D(de_4, 5)
46+
// | | | |
47+
2, // | | +-- Drop D(de_2, 2)
48+
// | | |
49+
1, // | +-- Drop D(de_1, 1)
50+
// | |
51+
0, // +-- Drop D(da_0, 0)
52+
// |
53+
// | result D(de_6, 7)
54+
7 // +-- Drop D(de_6, 7)
55+
56+
]);
57+
}
58+
59+
fn test<'a>(log: d::Log<'a>) {
60+
let da = D::new("da", 0, log);
61+
let de = D::new("de", 1, log);
62+
d::println(&format!("calling foo"));
63+
let result = foo(da, de);
64+
d::println(&format!("result {}", result));
65+
}
66+
67+
fn foo<'a>(da0: D<'a>, de1: D<'a>) -> D<'a> {
68+
d::println(&format!("entered foo"));
69+
let de2 = de1.incr(); // creates D(de_2, 2)
70+
let de4 = {
71+
let _da1 = da0.incr(); // creates D(da_1, 3)
72+
de2.incr().incr() // creates D(de_3, 4) and D(de_4, 5)
73+
};
74+
d::println(&format!("eval tail of foo"));
75+
de4.incr().incr() // creates D(de_5, 6) and D(de_6, 7)
76+
}
77+
78+
// This module provides simultaneous printouts of the dynamic extents
79+
// of all of the D values, in addition to logging the order that each
80+
// is dropped.
81+
82+
const PREF_INDENT: u32 = 16;
83+
84+
pub mod d {
85+
#![allow(unused_parens)]
86+
use std::fmt;
87+
use std::mem;
88+
use std::cell::RefCell;
89+
90+
static mut counter: u32 = 0;
91+
static mut trails: u64 = 0;
92+
93+
pub type Log<'a> = &'a RefCell<Vec<u32>>;
94+
95+
pub fn current_width() -> u32 {
96+
unsafe { max_width() - trails.leading_zeros() }
97+
}
98+
99+
pub fn max_width() -> u32 {
100+
unsafe {
101+
(mem::size_of_val(&trails)*8) as u32
102+
}
103+
}
104+
105+
pub fn indent_println(my_trails: u32, s: &str) {
106+
let mut indent: String = String::new();
107+
for i in 0..my_trails {
108+
unsafe {
109+
if trails & (1 << i) != 0 {
110+
indent = indent + "| ";
111+
} else {
112+
indent = indent + " ";
113+
}
114+
}
115+
}
116+
println!("{}{}", indent, s);
117+
}
118+
119+
pub fn println(s: &str) {
120+
indent_println(super::PREF_INDENT, s);
121+
}
122+
123+
fn first_avail() -> u32 {
124+
unsafe {
125+
for i in 0..64 {
126+
if trails & (1 << i) == 0 {
127+
return i;
128+
}
129+
}
130+
}
131+
panic!("exhausted trails");
132+
}
133+
134+
pub struct D<'a> {
135+
name: &'static str, i: u32, uid: u32, trail: u32, log: Log<'a>
136+
}
137+
138+
impl<'a> fmt::Display for D<'a> {
139+
fn fmt(&self, w: &mut fmt::Formatter) -> fmt::Result {
140+
write!(w, "D({}_{}, {})", self.name, self.i, self.uid)
141+
}
142+
}
143+
144+
impl<'a> D<'a> {
145+
pub fn new(name: &'static str, i: u32, log: Log<'a>) -> D<'a> {
146+
unsafe {
147+
let trail = first_avail();
148+
let ctr = counter;
149+
counter += 1;
150+
trails |= (1 << trail);
151+
let ret = D {
152+
name: name, i: i, log: log, uid: ctr, trail: trail
153+
};
154+
indent_println(trail, &format!("+-- Make {}", ret));
155+
ret
156+
}
157+
}
158+
pub fn incr(&self) -> D<'a> {
159+
D::new(self.name, self.i + 1, self.log)
160+
}
161+
}
162+
163+
impl<'a> Drop for D<'a> {
164+
fn drop(&mut self) {
165+
unsafe { trails &= !(1 << self.trail); };
166+
self.log.borrow_mut().push(self.uid);
167+
indent_println(self.trail, &format!("+-- Drop {}", self));
168+
indent_println(::PREF_INDENT, "");
169+
}
170+
}
171+
}

0 commit comments

Comments
 (0)