Evaluate borrow and struct expressions in into

This fixes some ordering problems around assignment expressions.

Author: matthewjasper

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 crate::hair::*;
 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<M>(&mut self, block: BasicBlock, expr: M) -> BlockAnd<Rvalue<'tcx>>
     where
         M: Mirror<'tcx, Output = Expr<'tcx>>,
@@ -23,7 +27,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
     }
 
     /// Compile `expr`, yielding an rvalue.
-    pub fn as_rvalue<M>(
+    fn as_rvalue<M>(
         &mut self,
         block: BasicBlock,
         scope: Option<region::Scope>,
@@ -66,16 +70,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 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,72 +250,6 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
                 };
                 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(base.clone().field(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())
@@ -346,6 +274,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             | ExprKind::Match { .. }
             | ExprKind::NeverToAny { .. }
             | ExprKind::Use { .. }
+            | ExprKind::Borrow { .. }
+            | ExprKind::Adt { .. }
             | ExprKind::Loop { .. }
             | ExprKind::LogicalOp { .. }
             | ExprKind::Call { .. }

src/librustc_mir/build/expr/category.rs

@@ -48,19 +48,19 @@ impl Category {
             | 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 { .. }
             | ExprKind::Box { .. }
             | ExprKind::Cast { .. }
             | ExprKind::Pointer { .. }
             | ExprKind::Repeat { .. }
-            | ExprKind::Borrow { .. }
             | ExprKind::Assign { .. }
             | ExprKind::AssignOp { .. }
             | ExprKind::Yield { .. }

src/librustc_mir/build/expr/into.rs

@@ -4,7 +4,9 @@ use crate::build::expr::category::{Category, RvalueFunc};
 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,100 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             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(base.clone().field(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 +420,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
             | ExprKind::Cast { .. }
             | ExprKind::Pointer { .. }
             | ExprKind::Repeat { .. }
-            | ExprKind::Borrow { .. }
             | ExprKind::Array { .. }
             | ExprKind::Tuple { .. }
-            | ExprKind::Adt { .. }
             | ExprKind::Closure { .. }
             | ExprKind::Literal { .. }
             | ExprKind::Yield { .. } => {

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
 

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();
+}

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

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