diff --git a/src/librustc_mir/build/expr/as_rvalue.rs b/src/librustc_mir/build/expr/as_rvalue.rs index 4f1ac8e51dc..f9b77a4b5dd 100644 --- a/src/librustc_mir/build/expr/as_rvalue.rs +++ b/src/librustc_mir/build/expr/as_rvalue.rs @@ -1,6 +1,5 @@ //! See docs in `build/expr/mod.rs`. -use rustc_data_structures::fx::FxHashMap; use rustc_index::vec::Idx; use crate::build::expr::category::{Category, RvalueFunc}; @@ -9,11 +8,16 @@ use rustc::middle::region; use rustc::mir::interpret::PanicInfo; use rustc::mir::*; -use rustc::ty::{self, CanonicalUserTypeAnnotation, Ty, UpvarSubsts}; +use rustc::ty::{self, Ty, UpvarSubsts}; use syntax_pos::Span; impl<'a, 'tcx> Builder<'a, 'tcx> { - /// See comment on `as_local_operand` + /// Returns an rvalue suitable for use until the end of the current + /// scope expression. + /// + /// The operand returned from this function will *not be valid* after + /// an ExprKind::Scope is passed, so please do *not* return it from + /// functions to avoid bad miscompiles. pub fn as_local_rvalue(&mut self, block: BasicBlock, expr: M) -> BlockAnd> where M: Mirror<'tcx, Output = Expr<'tcx>>, @@ -23,7 +27,7 @@ pub fn as_local_rvalue(&mut self, block: BasicBlock, expr: M) -> BlockAnd( + fn as_rvalue( &mut self, block: BasicBlock, scope: Option, @@ -66,16 +70,6 @@ fn expr_as_rvalue( let value_operand = unpack!(block = this.as_operand(block, scope, value)); block.and(Rvalue::Repeat(value_operand, count)) } - ExprKind::Borrow { - borrow_kind, - arg, - } => { - let arg_place = match borrow_kind { - BorrowKind::Shared => unpack!(block = this.as_read_only_place(block, arg)), - _ => unpack!(block = this.as_place(block, arg)), - }; - block.and(Rvalue::Ref(this.hir.tcx().lifetimes.re_erased, borrow_kind, arg_place)) - } ExprKind::Binary { op, lhs, rhs } => { let lhs = unpack!(block = this.as_operand(block, scope, lhs)); let rhs = unpack!(block = this.as_operand(block, scope, rhs)); @@ -256,77 +250,6 @@ fn expr_as_rvalue( }; block.and(Rvalue::Aggregate(result, operands)) } - ExprKind::Adt { - adt_def, - variant_index, - substs, - user_ty, - fields, - base, - } => { - // see (*) above - let is_union = adt_def.is_union(); - let active_field_index = if is_union { - Some(fields[0].name.index()) - } else { - None - }; - - // first process the set of fields that were provided - // (evaluating them in order given by user) - let fields_map: FxHashMap<_, _> = fields - .into_iter() - .map(|f| { - ( - f.name, - unpack!(block = this.as_operand(block, scope, f.expr)), - ) - }).collect(); - - let field_names = this.hir.all_fields(adt_def, variant_index); - - let fields = if let Some(FruInfo { base, field_types }) = base { - let base = unpack!(block = this.as_place(block, base)); - - // MIR does not natively support FRU, so for each - // base-supplied field, generate an operand that - // reads it from the base. - field_names - .into_iter() - .zip(field_types.into_iter()) - .map(|(n, ty)| match fields_map.get(&n) { - Some(v) => v.clone(), - None => this.consume_by_copy_or_move(this.hir.tcx().mk_place_field( - base.clone(), - n, - ty, - )), - }) - .collect() - } else { - field_names - .iter() - .filter_map(|n| fields_map.get(n).cloned()) - .collect() - }; - - let inferred_ty = expr.ty; - let user_ty = user_ty.map(|ty| { - this.canonical_user_type_annotations.push(CanonicalUserTypeAnnotation { - span: source_info.span, - user_ty: ty, - inferred_ty, - }) - }); - let adt = box AggregateKind::Adt( - adt_def, - variant_index, - substs, - user_ty, - active_field_index, - ); - block.and(Rvalue::Aggregate(adt, fields)) - } ExprKind::Assign { .. } | ExprKind::AssignOp { .. } => { block = unpack!(this.stmt_expr(block, expr, None)); block.and(this.unit_rvalue()) @@ -351,6 +274,8 @@ fn expr_as_rvalue( | ExprKind::Match { .. } | ExprKind::NeverToAny { .. } | ExprKind::Use { .. } + | ExprKind::Borrow { .. } + | ExprKind::Adt { .. } | ExprKind::Loop { .. } | ExprKind::LogicalOp { .. } | ExprKind::Call { .. } diff --git a/src/librustc_mir/build/expr/category.rs b/src/librustc_mir/build/expr/category.rs index f679a00035d..ae5289986e7 100644 --- a/src/librustc_mir/build/expr/category.rs +++ b/src/librustc_mir/build/expr/category.rs @@ -48,11 +48,12 @@ pub fn of(ek: &ExprKind<'_>) -> Option { | ExprKind::Match { .. } | ExprKind::NeverToAny { .. } | ExprKind::Use { .. } + | ExprKind::Adt { .. } + | ExprKind::Borrow { .. } | ExprKind::Call { .. } => Some(Category::Rvalue(RvalueFunc::Into)), ExprKind::Array { .. } | ExprKind::Tuple { .. } - | ExprKind::Adt { .. } | ExprKind::Closure { .. } | ExprKind::Unary { .. } | ExprKind::Binary { .. } @@ -60,7 +61,6 @@ pub fn of(ek: &ExprKind<'_>) -> Option { | ExprKind::Cast { .. } | ExprKind::Pointer { .. } | ExprKind::Repeat { .. } - | ExprKind::Borrow { .. } | ExprKind::Assign { .. } | ExprKind::AssignOp { .. } | ExprKind::Yield { .. } diff --git a/src/librustc_mir/build/expr/into.rs b/src/librustc_mir/build/expr/into.rs index c9e5fca4d75..404ca3204e6 100644 --- a/src/librustc_mir/build/expr/into.rs +++ b/src/librustc_mir/build/expr/into.rs @@ -4,7 +4,9 @@ use crate::build::{BlockAnd, BlockAndExtension, BlockFrame, Builder}; use crate::hair::*; use rustc::mir::*; -use rustc::ty; +use rustc::ty::{self, CanonicalUserTypeAnnotation}; +use rustc_data_structures::fx::FxHashMap; +use syntax_pos::symbol::sym; use rustc_target::spec::abi::Abi; @@ -270,6 +272,102 @@ pub fn into_expr( ExprKind::Use { source } => { this.into(destination, block, source) } + ExprKind::Borrow { arg, borrow_kind } => { + // We don't do this in `as_rvalue` because we use `as_place` + // for borrow expressions, so we cannot create an `RValue` that + // remains valid across user code. `as_rvalue` is usually called + // by this method anyway, so this shouldn't cause too many + // unnecessary temporaries. + let arg_place = match borrow_kind { + BorrowKind::Shared => unpack!(block = this.as_read_only_place(block, arg)), + _ => unpack!(block = this.as_place(block, arg)), + }; + let borrow = Rvalue::Ref( + this.hir.tcx().lifetimes.re_erased, + borrow_kind, + arg_place, + ); + this.cfg.push_assign(block, source_info, destination, borrow); + block.unit() + } + ExprKind::Adt { + adt_def, + variant_index, + substs, + user_ty, + fields, + base, + } => { + // See the notes for `ExprKind::Array` in `as_rvalue` and for + // `ExprKind::Borrow` above. + let is_union = adt_def.is_union(); + let active_field_index = if is_union { + Some(fields[0].name.index()) + } else { + None + }; + + let scope = this.local_scope(); + + // first process the set of fields that were provided + // (evaluating them in order given by user) + let fields_map: FxHashMap<_, _> = fields + .into_iter() + .map(|f| { + ( + f.name, + unpack!(block = this.as_operand(block, scope, f.expr)), + ) + }).collect(); + + let field_names = this.hir.all_fields(adt_def, variant_index); + + let fields = if let Some(FruInfo { base, field_types }) = base { + let base = unpack!(block = this.as_place(block, base)); + + // MIR does not natively support FRU, so for each + // base-supplied field, generate an operand that + // reads it from the base. + field_names + .into_iter() + .zip(field_types.into_iter()) + .map(|(n, ty)| match fields_map.get(&n) { + Some(v) => v.clone(), + None => this.consume_by_copy_or_move( + this.hir.tcx().mk_place_field(base.clone(), n, ty), + ), + }).collect() + } else { + field_names + .iter() + .filter_map(|n| fields_map.get(n).cloned()) + .collect() + }; + + let inferred_ty = expr.ty; + let user_ty = user_ty.map(|ty| { + this.canonical_user_type_annotations.push(CanonicalUserTypeAnnotation { + span: source_info.span, + user_ty: ty, + inferred_ty, + }) + }); + let adt = box AggregateKind::Adt( + adt_def, + variant_index, + substs, + user_ty, + active_field_index, + ); + this.cfg.push_assign( + block, + source_info, + destination, + Rvalue::Aggregate(adt, fields) + ); + block.unit() + } + // These cases don't actually need a destination ExprKind::Assign { .. } @@ -324,10 +422,8 @@ pub fn into_expr( | ExprKind::Cast { .. } | ExprKind::Pointer { .. } | ExprKind::Repeat { .. } - | ExprKind::Borrow { .. } | ExprKind::Array { .. } | ExprKind::Tuple { .. } - | ExprKind::Adt { .. } | ExprKind::Closure { .. } | ExprKind::Literal { .. } | ExprKind::Yield { .. } => { diff --git a/src/test/ui/borrowck/borrowck-init-in-fru.stderr b/src/test/ui/borrowck/borrowck-init-in-fru.stderr index a4c042d1c12..f01afe1466a 100644 --- a/src/test/ui/borrowck/borrowck-init-in-fru.stderr +++ b/src/test/ui/borrowck/borrowck-init-in-fru.stderr @@ -1,8 +1,8 @@ error[E0381]: use of possibly-uninitialized variable: `origin` - --> $DIR/borrowck-init-in-fru.rs:9:5 + --> $DIR/borrowck-init-in-fru.rs:9:14 | LL | origin = Point { x: 10, ..origin }; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ use of possibly-uninitialized `origin.y` + | ^^^^^^^^^^^^^^^^^^^^^^^^^ use of possibly-uninitialized `origin.y` error: aborting due to previous error diff --git a/src/test/ui/mir/mir_assign_eval_order.rs b/src/test/ui/mir/mir_assign_eval_order.rs new file mode 100644 index 00000000000..1594421b0b1 --- /dev/null +++ b/src/test/ui/mir/mir_assign_eval_order.rs @@ -0,0 +1,67 @@ +// Test evaluation order of assignment expressions is right to left. + +// run-pass + +// We would previously not finish evaluating borrow and FRU expressions before +// starting on the LHS + +struct S(i32); + +fn evaluate_reborrow_before_assign() { + let mut x = &1; + let y = &mut &2; + let z = &3; + // There's an implicit reborrow of `x` on the right-hand side of the + // assignement. Note that writing an explicit reborrow would not show this + // bug, as now there would be two reborrows on the right-hand side and at + // least one of them would happen before the left-hand side is evaluated. + *{ x = z; &mut *y } = x; + assert_eq!(*x, 3); + assert_eq!(**y, 1); // y should be assigned the original value of `x`. +} + +fn evaluate_mut_reborrow_before_assign() { + let mut x = &mut 1; + let y = &mut &mut 2; + let z = &mut 3; + *{ x = z; &mut *y } = x; + assert_eq!(*x, 3); + assert_eq!(**y, 1); // y should be assigned the original value of `x`. +} + +// We should evaluate `x[2]` and borrow the value out *before* evaluating the +// LHS and changing its value. +fn evaluate_ref_to_temp_before_assign_slice() { + let mut x = &[S(0), S(1), S(2)][..]; + let y = &mut &S(7); + *{ x = &[S(3), S(4), S(5)]; &mut *y } = &x[2]; + assert_eq!(2, y.0); + assert_eq!(5, x[2].0); +} + +// We should evaluate `x[2]` and copy the value out *before* evaluating the LHS +// and changing its value. +fn evaluate_fru_to_temp_before_assign_slice() { + let mut x = &[S(0), S(1), S(2)][..]; + let y = &mut S(7); + *{ x = &[S(3), S(4), S(5)]; &mut *y } = S { ..x[2] }; + assert_eq!(2, y.0); + assert_eq!(5, x[2].0); +} + +// We should evaluate `*x` and copy the value out *before* evaluating the LHS +// and dropping `x`. +fn evaluate_fru_to_temp_before_assign_box() { + let x = Box::new(S(0)); + let y = &mut S(1); + *{ drop(x); &mut *y } = S { ..*x }; + assert_eq!(0, y.0); +} + +fn main() { + evaluate_reborrow_before_assign(); + evaluate_mut_reborrow_before_assign(); + evaluate_ref_to_temp_before_assign_slice(); + evaluate_fru_to_temp_before_assign_slice(); + evaluate_fru_to_temp_before_assign_box(); +} diff --git a/src/test/ui/nll/issue-52534-2.stderr b/src/test/ui/nll/issue-52534-2.stderr index dd8a87f7e29..cef4aba0240 100644 --- a/src/test/ui/nll/issue-52534-2.stderr +++ b/src/test/ui/nll/issue-52534-2.stderr @@ -1,8 +1,8 @@ error[E0597]: `x` does not live long enough - --> $DIR/issue-52534-2.rs:6:9 + --> $DIR/issue-52534-2.rs:6:13 | LL | y = &x - | ^^^^^^ borrowed value does not live long enough + | ^^ borrowed value does not live long enough LL | LL | } | - `x` dropped here while still borrowed diff --git a/src/test/ui/span/issue-36537.stderr b/src/test/ui/span/issue-36537.stderr index edb804e850e..0939584380a 100644 --- a/src/test/ui/span/issue-36537.stderr +++ b/src/test/ui/span/issue-36537.stderr @@ -1,8 +1,8 @@ error[E0597]: `a` does not live long enough - --> $DIR/issue-36537.rs:5:9 + --> $DIR/issue-36537.rs:5:13 | LL | p = &a; - | ^^^^^^ borrowed value does not live long enough + | ^^ borrowed value does not live long enough ... LL | } | - `a` dropped here while still borrowed