Skip to content

Commit 4bf0685

Browse files
matthewjaspermatthewjasper
committed
Evaluate borrow and struct expressions in into
This fixes some ordering problems around assignment expressions.
1 parent 7320818 commit 4bf0685

File tree

7 files changed

+184
-96
lines changed

7 files changed

+184
-96
lines changed

‎src/librustc_mir/build/expr/as_rvalue.rs

+10-85
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
//! See docs in `build/expr/mod.rs`.
22
3-
use rustc_data_structures::fx::FxHashMap;
43
use rustc_index::vec::Idx;
54

65
use crate::build::expr::category::{Category, RvalueFunc};
@@ -9,11 +8,16 @@ use crate::hair::*;
98
use rustc::middle::region;
109
use rustc::mir::interpret::PanicInfo;
1110
use rustc::mir::*;
12-
use rustc::ty::{self, CanonicalUserTypeAnnotation, Ty, UpvarSubsts};
11+
use rustc::ty::{self, Ty, UpvarSubsts};
1312
use syntax_pos::Span;
1413

1514
impl<'a, 'tcx> Builder<'a, 'tcx> {
16-
/// See comment on `as_local_operand`
15+
/// Returns an rvalue suitable for use until the end of the current
16+
/// scope expression.
17+
///
18+
/// The operand returned from this function will *not be valid* after
19+
/// an ExprKind::Scope is passed, so please do *not* return it from
20+
/// functions to avoid bad miscompiles.
1721
pub fn as_local_rvalue<M>(&mut self, block: BasicBlock, expr: M) -> BlockAnd<Rvalue<'tcx>>
1822
where
1923
M: Mirror<'tcx, Output = Expr<'tcx>>,
@@ -23,7 +27,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
2327
}
2428

2529
/// Compile `expr`, yielding an rvalue.
26-
pub fn as_rvalue<M>(
30+
fn as_rvalue<M>(
2731
&mut self,
2832
block: BasicBlock,
2933
scope: Option<region::Scope>,
@@ -66,16 +70,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
6670
let value_operand = unpack!(block = this.as_operand(block, scope, value));
6771
block.and(Rvalue::Repeat(value_operand, count))
6872
}
69-
ExprKind::Borrow {
70-
borrow_kind,
71-
arg,
72-
} => {
73-
let arg_place = match borrow_kind {
74-
BorrowKind::Shared => unpack!(block = this.as_read_only_place(block, arg)),
75-
_ => unpack!(block = this.as_place(block, arg)),
76-
};
77-
block.and(Rvalue::Ref(this.hir.tcx().lifetimes.re_erased, borrow_kind, arg_place))
78-
}
7973
ExprKind::Binary { op, lhs, rhs } => {
8074
let lhs = unpack!(block = this.as_operand(block, scope, lhs));
8175
let rhs = unpack!(block = this.as_operand(block, scope, rhs));
@@ -256,77 +250,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
256250
};
257251
block.and(Rvalue::Aggregate(result, operands))
258252
}
259-
ExprKind::Adt {
260-
adt_def,
261-
variant_index,
262-
substs,
263-
user_ty,
264-
fields,
265-
base,
266-
} => {
267-
// see (*) above
268-
let is_union = adt_def.is_union();
269-
let active_field_index = if is_union {
270-
Some(fields[0].name.index())
271-
} else {
272-
None
273-
};
274-
275-
// first process the set of fields that were provided
276-
// (evaluating them in order given by user)
277-
let fields_map: FxHashMap<_, _> = fields
278-
.into_iter()
279-
.map(|f| {
280-
(
281-
f.name,
282-
unpack!(block = this.as_operand(block, scope, f.expr)),
283-
)
284-
}).collect();
285-
286-
let field_names = this.hir.all_fields(adt_def, variant_index);
287-
288-
let fields = if let Some(FruInfo { base, field_types }) = base {
289-
let base = unpack!(block = this.as_place(block, base));
290-
291-
// MIR does not natively support FRU, so for each
292-
// base-supplied field, generate an operand that
293-
// reads it from the base.
294-
field_names
295-
.into_iter()
296-
.zip(field_types.into_iter())
297-
.map(|(n, ty)| match fields_map.get(&n) {
298-
Some(v) => v.clone(),
299-
None => this.consume_by_copy_or_move(this.hir.tcx().mk_place_field(
300-
base.clone(),
301-
n,
302-
ty,
303-
)),
304-
})
305-
.collect()
306-
} else {
307-
field_names
308-
.iter()
309-
.filter_map(|n| fields_map.get(n).cloned())
310-
.collect()
311-
};
312-
313-
let inferred_ty = expr.ty;
314-
let user_ty = user_ty.map(|ty| {
315-
this.canonical_user_type_annotations.push(CanonicalUserTypeAnnotation {
316-
span: source_info.span,
317-
user_ty: ty,
318-
inferred_ty,
319-
})
320-
});
321-
let adt = box AggregateKind::Adt(
322-
adt_def,
323-
variant_index,
324-
substs,
325-
user_ty,
326-
active_field_index,
327-
);
328-
block.and(Rvalue::Aggregate(adt, fields))
329-
}
330253
ExprKind::Assign { .. } | ExprKind::AssignOp { .. } => {
331254
block = unpack!(this.stmt_expr(block, expr, None));
332255
block.and(this.unit_rvalue())
@@ -351,6 +274,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
351274
| ExprKind::Match { .. }
352275
| ExprKind::NeverToAny { .. }
353276
| ExprKind::Use { .. }
277+
| ExprKind::Borrow { .. }
278+
| ExprKind::Adt { .. }
354279
| ExprKind::Loop { .. }
355280
| ExprKind::LogicalOp { .. }
356281
| ExprKind::Call { .. }

‎src/librustc_mir/build/expr/category.rs

+2-2
Original file line numberDiff line numberDiff line change
@@ -48,19 +48,19 @@ impl Category {
4848
| ExprKind::Match { .. }
4949
| ExprKind::NeverToAny { .. }
5050
| ExprKind::Use { .. }
51+
| ExprKind::Adt { .. }
52+
| ExprKind::Borrow { .. }
5153
| ExprKind::Call { .. } => Some(Category::Rvalue(RvalueFunc::Into)),
5254

5355
ExprKind::Array { .. }
5456
| ExprKind::Tuple { .. }
55-
| ExprKind::Adt { .. }
5657
| ExprKind::Closure { .. }
5758
| ExprKind::Unary { .. }
5859
| ExprKind::Binary { .. }
5960
| ExprKind::Box { .. }
6061
| ExprKind::Cast { .. }
6162
| ExprKind::Pointer { .. }
6263
| ExprKind::Repeat { .. }
63-
| ExprKind::Borrow { .. }
6464
| ExprKind::Assign { .. }
6565
| ExprKind::AssignOp { .. }
6666
| ExprKind::Yield { .. }

‎src/librustc_mir/build/expr/into.rs

+99-3
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,9 @@ use crate::build::expr::category::{Category, RvalueFunc};
44
use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder};
55
use crate::hair::*;
66
use rustc::mir::*;
7-
use rustc::ty;
7+
use rustc::ty::{self, CanonicalUserTypeAnnotation};
8+
use rustc_data_structures::fx::FxHashMap;
9+
use syntax_pos::symbol::sym;
810

911
use rustc_target::spec::abi::Abi;
1012

@@ -270,6 +272,102 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
270272
ExprKind::Use { source } => {
271273
this.into(destination, block, source)
272274
}
275+
ExprKind::Borrow { arg, borrow_kind } => {
276+
// We don't do this in `as_rvalue` because we use `as_place`
277+
// for borrow expressions, so we cannot create an `RValue` that
278+
// remains valid across user code. `as_rvalue` is usually called
279+
// by this method anyway, so this shouldn't cause too many
280+
// unnecessary temporaries.
281+
let arg_place = match borrow_kind {
282+
BorrowKind::Shared => unpack!(block = this.as_read_only_place(block, arg)),
283+
_ => unpack!(block = this.as_place(block, arg)),
284+
};
285+
let borrow = Rvalue::Ref(
286+
this.hir.tcx().lifetimes.re_erased,
287+
borrow_kind,
288+
arg_place,
289+
);
290+
this.cfg.push_assign(block, source_info, destination, borrow);
291+
block.unit()
292+
}
293+
ExprKind::Adt {
294+
adt_def,
295+
variant_index,
296+
substs,
297+
user_ty,
298+
fields,
299+
base,
300+
} => {
301+
// See the notes for `ExprKind::Array` in `as_rvalue` and for
302+
// `ExprKind::Borrow` above.
303+
let is_union = adt_def.is_union();
304+
let active_field_index = if is_union {
305+
Some(fields[0].name.index())
306+
} else {
307+
None
308+
};
309+
310+
let scope = this.local_scope();
311+
312+
// first process the set of fields that were provided
313+
// (evaluating them in order given by user)
314+
let fields_map: FxHashMap<_, _> = fields
315+
.into_iter()
316+
.map(|f| {
317+
(
318+
f.name,
319+
unpack!(block = this.as_operand(block, scope, f.expr)),
320+
)
321+
}).collect();
322+
323+
let field_names = this.hir.all_fields(adt_def, variant_index);
324+
325+
let fields = if let Some(FruInfo { base, field_types }) = base {
326+
let base = unpack!(block = this.as_place(block, base));
327+
328+
// MIR does not natively support FRU, so for each
329+
// base-supplied field, generate an operand that
330+
// reads it from the base.
331+
field_names
332+
.into_iter()
333+
.zip(field_types.into_iter())
334+
.map(|(n, ty)| match fields_map.get(&n) {
335+
Some(v) => v.clone(),
336+
None => this.consume_by_copy_or_move(
337+
this.hir.tcx().mk_place_field(base.clone(), n, ty),
338+
),
339+
}).collect()
340+
} else {
341+
field_names
342+
.iter()
343+
.filter_map(|n| fields_map.get(n).cloned())
344+
.collect()
345+
};
346+
347+
let inferred_ty = expr.ty;
348+
let user_ty = user_ty.map(|ty| {
349+
this.canonical_user_type_annotations.push(CanonicalUserTypeAnnotation {
350+
span: source_info.span,
351+
user_ty: ty,
352+
inferred_ty,
353+
})
354+
});
355+
let adt = box AggregateKind::Adt(
356+
adt_def,
357+
variant_index,
358+
substs,
359+
user_ty,
360+
active_field_index,
361+
);
362+
this.cfg.push_assign(
363+
block,
364+
source_info,
365+
destination,
366+
Rvalue::Aggregate(adt, fields)
367+
);
368+
block.unit()
369+
}
370+
273371

274372
// These cases don't actually need a destination
275373
ExprKind::Assign { .. }
@@ -324,10 +422,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
324422
| ExprKind::Cast { .. }
325423
| ExprKind::Pointer { .. }
326424
| ExprKind::Repeat { .. }
327-
| ExprKind::Borrow { .. }
328425
| ExprKind::Array { .. }
329426
| ExprKind::Tuple { .. }
330-
| ExprKind::Adt { .. }
331427
| ExprKind::Closure { .. }
332428
| ExprKind::Literal { .. }
333429
| ExprKind::Yield { .. } => {

‎src/test/ui/borrowck/borrowck-init-in-fru.stderr

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
error[E0381]: use of possibly-uninitialized variable: `origin`
2-
--> $DIR/borrowck-init-in-fru.rs:9:5
2+
--> $DIR/borrowck-init-in-fru.rs:9:14
33
|
44
LL | origin = Point { x: 10, ..origin };
5-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ use of possibly-uninitialized `origin.y`
5+
| ^^^^^^^^^^^^^^^^^^^^^^^^^ use of possibly-uninitialized `origin.y`
66

77
error: aborting due to previous error
88

‎src/test/ui/mir/mir_assign_eval_order.rs

+67
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
// Test evaluation order of assignment expressions is right to left.
2+
3+
// run-pass
4+
5+
// We would previously not finish evaluating borrow and FRU expressions before
6+
// starting on the LHS
7+
8+
struct S(i32);
9+
10+
fn evaluate_reborrow_before_assign() {
11+
let mut x = &1;
12+
let y = &mut &2;
13+
let z = &3;
14+
// There's an implicit reborrow of `x` on the right-hand side of the
15+
// assignement. Note that writing an explicit reborrow would not show this
16+
// bug, as now there would be two reborrows on the right-hand side and at
17+
// least one of them would happen before the left-hand side is evaluated.
18+
*{ x = z; &mut *y } = x;
19+
assert_eq!(*x, 3);
20+
assert_eq!(**y, 1); // y should be assigned the original value of `x`.
21+
}
22+
23+
fn evaluate_mut_reborrow_before_assign() {
24+
let mut x = &mut 1;
25+
let y = &mut &mut 2;
26+
let z = &mut 3;
27+
*{ x = z; &mut *y } = x;
28+
assert_eq!(*x, 3);
29+
assert_eq!(**y, 1); // y should be assigned the original value of `x`.
30+
}
31+
32+
// We should evaluate `x[2]` and borrow the value out *before* evaluating the
33+
// LHS and changing its value.
34+
fn evaluate_ref_to_temp_before_assign_slice() {
35+
let mut x = &[S(0), S(1), S(2)][..];
36+
let y = &mut &S(7);
37+
*{ x = &[S(3), S(4), S(5)]; &mut *y } = &x[2];
38+
assert_eq!(2, y.0);
39+
assert_eq!(5, x[2].0);
40+
}
41+
42+
// We should evaluate `x[2]` and copy the value out *before* evaluating the LHS
43+
// and changing its value.
44+
fn evaluate_fru_to_temp_before_assign_slice() {
45+
let mut x = &[S(0), S(1), S(2)][..];
46+
let y = &mut S(7);
47+
*{ x = &[S(3), S(4), S(5)]; &mut *y } = S { ..x[2] };
48+
assert_eq!(2, y.0);
49+
assert_eq!(5, x[2].0);
50+
}
51+
52+
// We should evaluate `*x` and copy the value out *before* evaluating the LHS
53+
// and dropping `x`.
54+
fn evaluate_fru_to_temp_before_assign_box() {
55+
let x = Box::new(S(0));
56+
let y = &mut S(1);
57+
*{ drop(x); &mut *y } = S { ..*x };
58+
assert_eq!(0, y.0);
59+
}
60+
61+
fn main() {
62+
evaluate_reborrow_before_assign();
63+
evaluate_mut_reborrow_before_assign();
64+
evaluate_ref_to_temp_before_assign_slice();
65+
evaluate_fru_to_temp_before_assign_slice();
66+
evaluate_fru_to_temp_before_assign_box();
67+
}

‎src/test/ui/nll/issue-52534-2.stderr

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
error[E0597]: `x` does not live long enough
2-
--> $DIR/issue-52534-2.rs:6:9
2+
--> $DIR/issue-52534-2.rs:6:13
33
|
44
LL | y = &x
5-
| ^^^^^^ borrowed value does not live long enough
5+
| ^^ borrowed value does not live long enough
66
LL |
77
LL | }
88
| - `x` dropped here while still borrowed

‎src/test/ui/span/issue-36537.stderr

+2-2
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,8 @@
11
error[E0597]: `a` does not live long enough
2-
--> $DIR/issue-36537.rs:5:9
2+
--> $DIR/issue-36537.rs:5:13
33
|
44
LL | p = &a;
5-
| ^^^^^^ borrowed value does not live long enough
5+
| ^^ borrowed value does not live long enough
66
...
77
LL | }
88
| - `a` dropped here while still borrowed

0 commit comments

Comments
 (0)