Skip a shared borrow of a immutable local variables

issue rust-lang#53643

Author: mikhail-m1

src/librustc/mir/mod.rs

@@ -252,11 +252,6 @@ impl<'tcx> Mir<'tcx> {
         } else if self.local_decls[local].name.is_some() {
             LocalKind::Var
         } else {
-            debug_assert!(
-                self.local_decls[local].mutability == Mutability::Mut,
-                "temp should be mutable"
-            );
-
             LocalKind::Temp
         }
     }
@@ -784,33 +779,38 @@ impl<'tcx> LocalDecl<'tcx> {
     /// Create a new `LocalDecl` for a temporary.
     #[inline]
     pub fn new_temp(ty: Ty<'tcx>, span: Span) -> Self {
-        LocalDecl {
-            mutability: Mutability::Mut,
-            ty,
-            name: None,
-            source_info: SourceInfo {
-                span,
-                scope: OUTERMOST_SOURCE_SCOPE,
-            },
-            visibility_scope: OUTERMOST_SOURCE_SCOPE,
-            internal: false,
-            is_user_variable: None,
-        }
+        Self::new_local(ty, Mutability::Mut, false, span)
+    }
+
+    /// Create a new immutable `LocalDecl` for a temporary.
+    #[inline]
+    pub fn new_immutable_temp(ty: Ty<'tcx>, span: Span) -> Self {
+        Self::new_local(ty, Mutability::Not, false, span)
     }
 
     /// Create a new `LocalDecl` for a internal temporary.
     #[inline]
     pub fn new_internal(ty: Ty<'tcx>, span: Span) -> Self {
+        Self::new_local(ty, Mutability::Mut, true, span)
+    }
+
+    #[inline]
+    fn new_local(
+        ty: Ty<'tcx>,
+        mutability: Mutability,
+        internal: bool,
+        span: Span,
+    ) -> Self {
         LocalDecl {
-            mutability: Mutability::Mut,
+            mutability,
             ty,
             name: None,
             source_info: SourceInfo {
                 span,
                 scope: OUTERMOST_SOURCE_SCOPE,
             },
             visibility_scope: OUTERMOST_SOURCE_SCOPE,
-            internal: true,
+            internal,
             is_user_variable: None,
         }
     }

src/librustc_mir/borrow_check/borrow_set.rs

@@ -10,12 +10,14 @@
 
 use borrow_check::place_ext::PlaceExt;
 use dataflow::indexes::BorrowIndex;
+use dataflow::move_paths::MoveData;
 use rustc::mir::traversal;
 use rustc::mir::visit::{PlaceContext, Visitor};
-use rustc::mir::{self, Location, Mir, Place};
+use rustc::mir::{self, Location, Mir, Place, Local};
 use rustc::ty::{Region, TyCtxt};
 use rustc::util::nodemap::{FxHashMap, FxHashSet};
 use rustc_data_structures::indexed_vec::IndexVec;
+use rustc_data_structures::bitvec::BitArray;
 use std::fmt;
 use std::hash::Hash;
 use std::ops::Index;
@@ -43,6 +45,8 @@ crate struct BorrowSet<'tcx> {
 
     /// Map from local to all the borrows on that local
     crate local_map: FxHashMap<mir::Local, FxHashSet<BorrowIndex>>,
+
+    crate locals_state_at_exit: LocalsStateAtExit,
 }
 
 impl<'tcx> Index<BorrowIndex> for BorrowSet<'tcx> {
@@ -96,8 +100,52 @@ impl<'tcx> fmt::Display for BorrowData<'tcx> {
     }
 }
 
+crate enum LocalsStateAtExit {
+    AllAreInvalidated,
+    SomeAreInvalidated { has_storage_dead_or_moved: BitArray<Local> }
+}
+
+impl LocalsStateAtExit {
+    fn build(
+        locals_are_invalidated_at_exit: bool,
+        mir: &Mir<'tcx>,
+        move_data: &MoveData<'tcx>
+    ) -> Self {
+        struct HasStorageDead(BitArray<Local>);
+
+        impl<'tcx> Visitor<'tcx> for HasStorageDead {
+            fn visit_local(&mut self, local: &Local, ctx: PlaceContext<'tcx>, _: Location) {
+                if ctx == PlaceContext::StorageDead {
+                    self.0.insert(*local);
+                }
+            }
+        }
+
+        if locals_are_invalidated_at_exit {
+            LocalsStateAtExit::AllAreInvalidated
+        } else {
+            let mut has_storage_dead = HasStorageDead(BitArray::new(mir.local_decls.len()));
+            has_storage_dead.visit_mir(mir);
+            let mut has_storage_dead_or_moved = has_storage_dead.0;
+            for move_out in &move_data.moves {
+                if let Some(index) = move_data.base_local(move_out.path) {
+                    has_storage_dead_or_moved.insert(index);
+
+                }
+            }
+            LocalsStateAtExit::SomeAreInvalidated{ has_storage_dead_or_moved }
+        }
+    }
+}
+
 impl<'tcx> BorrowSet<'tcx> {
-    pub fn build(tcx: TyCtxt<'_, '_, 'tcx>, mir: &Mir<'tcx>) -> Self {
+    pub fn build(
+        tcx: TyCtxt<'_, '_, 'tcx>,
+        mir: &Mir<'tcx>,
+        locals_are_invalidated_at_exit: bool,
+        move_data: &MoveData<'tcx>
+    ) -> Self {
+
         let mut visitor = GatherBorrows {
             tcx,
             mir,
@@ -107,6 +155,8 @@ impl<'tcx> BorrowSet<'tcx> {
             region_map: FxHashMap(),
             local_map: FxHashMap(),
             pending_activations: FxHashMap(),
+            locals_state_at_exit:
+                LocalsStateAtExit::build(locals_are_invalidated_at_exit, mir, move_data),
         };
 
         for (block, block_data) in traversal::preorder(mir) {
@@ -119,6 +169,7 @@ impl<'tcx> BorrowSet<'tcx> {
             activation_map: visitor.activation_map,
             region_map: visitor.region_map,
             local_map: visitor.local_map,
+            locals_state_at_exit: visitor.locals_state_at_exit,
         }
     }
 
@@ -148,6 +199,8 @@ struct GatherBorrows<'a, 'gcx: 'tcx, 'tcx: 'a> {
     /// the borrow. When we find a later use of this activation, we
     /// remove from the map (and add to the "tombstone" set below).
     pending_activations: FxHashMap<mir::Local, BorrowIndex>,
+
+    locals_state_at_exit: LocalsStateAtExit,
 }
 
 impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> {
@@ -159,7 +212,8 @@ impl<'a, 'gcx, 'tcx> Visitor<'tcx> for GatherBorrows<'a, 'gcx, 'tcx> {
         location: mir::Location,
     ) {
         if let mir::Rvalue::Ref(region, kind, ref borrowed_place) = *rvalue {
-            if borrowed_place.ignore_borrow(self.tcx, self.mir) {
+            if borrowed_place.ignore_borrow(
+                self.tcx, self.mir, &self.locals_state_at_exit) {
                 return;
             }
 

src/librustc_mir/borrow_check/mod.rs

@@ -196,7 +196,12 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>(
         |bd, i| DebugFormatted::new(&bd.move_data().inits[i]),
     ));
 
-    let borrow_set = Rc::new(BorrowSet::build(tcx, mir));
+    let locals_are_invalidated_at_exit = match tcx.hir.body_owner_kind(id) {
+            hir::BodyOwnerKind::Const | hir::BodyOwnerKind::Static(_) => false,
+            hir::BodyOwnerKind::Fn => true,
+    };
+    let borrow_set = Rc::new(BorrowSet::build(
+            tcx, mir, locals_are_invalidated_at_exit, &mdpe.move_data));
 
     // If we are in non-lexical mode, compute the non-lexical lifetimes.
     let (regioncx, polonius_output, opt_closure_req) = nll::compute_regions(
@@ -241,10 +246,7 @@ fn do_mir_borrowck<'a, 'gcx, 'tcx>(
         param_env: param_env,
         location_table,
         movable_generator,
-        locals_are_invalidated_at_exit: match tcx.hir.body_owner_kind(id) {
-            hir::BodyOwnerKind::Const | hir::BodyOwnerKind::Static(_) => false,
-            hir::BodyOwnerKind::Fn => true,
-        },
+        locals_are_invalidated_at_exit,
         access_place_error_reported: FxHashSet(),
         reservation_error_reported: FxHashSet(),
         moved_error_reported: FxHashSet(),

src/librustc_mir/borrow_check/place_ext.rs

@@ -10,27 +10,57 @@
 
 use rustc::hir;
 use rustc::mir::ProjectionElem;
-use rustc::mir::{Local, Mir, Place};
+use rustc::mir::{Local, Mir, Place, Mutability};
 use rustc::ty::{self, TyCtxt};
+use borrow_check::borrow_set::LocalsStateAtExit;
 
 /// Extension methods for the `Place` type.
 crate trait PlaceExt<'tcx> {
     /// Returns true if we can safely ignore borrows of this place.
     /// This is true whenever there is no action that the user can do
     /// to the place `self` that would invalidate the borrow. This is true
     /// for borrows of raw pointer dereferents as well as shared references.
-    fn ignore_borrow(&self, tcx: TyCtxt<'_, '_, 'tcx>, mir: &Mir<'tcx>) -> bool;
+    fn ignore_borrow(
+        &self,
+        tcx: TyCtxt<'_, '_, 'tcx>,
+        mir: &Mir<'tcx>,
+        locals_state_at_exit: &LocalsStateAtExit,
+        ) -> bool;
 
     /// If this is a place like `x.f.g`, returns the local
     /// `x`. Returns `None` if this is based in a static.
     fn root_local(&self) -> Option<Local>;
 }
 
 impl<'tcx> PlaceExt<'tcx> for Place<'tcx> {
-    fn ignore_borrow(&self, tcx: TyCtxt<'_, '_, 'tcx>, mir: &Mir<'tcx>) -> bool {
+    fn ignore_borrow(
+        &self,
+        tcx: TyCtxt<'_, '_, 'tcx>,
+        mir: &Mir<'tcx>,
+        locals_state_at_exit: &LocalsStateAtExit,
+    ) -> bool {
         match self {
-            Place::Promoted(_) |
-            Place::Local(_) => false,
+            Place::Promoted(_) => false,
+
+            // If a local variable is immutable, then we only need to track borrows to guard
+            // against two kinds of errors:
+            // * The variable being dropped while still borrowed (e.g., because the fn returns
+            //   a reference to a local variable)
+            // * The variable being moved while still borrowed
+            //
+            // In particular, the variable cannot be mutated -- the "access checks" will fail --
+            // so we don't have to worry about mutation while borrowed.
+            Place::Local(index) => {
+                match locals_state_at_exit {
+                    LocalsStateAtExit::AllAreInvalidated => false,
+                    LocalsStateAtExit::SomeAreInvalidated { has_storage_dead_or_moved } => {
+                        let ignore = !has_storage_dead_or_moved.contains(*index) &&
+                            mir.local_decls[*index].mutability == Mutability::Not;
+                        debug!("ignore_borrow: local {:?} => {:?}", index, ignore);
+                        ignore
+                    }
+                }
+            }
             Place::Static(static_) => {
                 tcx.is_static(static_.def_id) == Some(hir::Mutability::MutMutable)
             }
@@ -39,7 +69,8 @@ impl<'tcx> PlaceExt<'tcx> for Place<'tcx> {
                 | ProjectionElem::Downcast(..)
                 | ProjectionElem::Subslice { .. }
                 | ProjectionElem::ConstantIndex { .. }
-                | ProjectionElem::Index(_) => proj.base.ignore_borrow(tcx, mir),
+                | ProjectionElem::Index(_) => proj.base.ignore_borrow(
+                    tcx, mir, locals_state_at_exit),
 
                 ProjectionElem::Deref => {
                     let ty = proj.base.ty(mir, tcx).to_ty(tcx);
@@ -55,7 +86,7 @@ impl<'tcx> PlaceExt<'tcx> for Place<'tcx> {
                         // borrowed *that* one, leaving the original
                         // path unborrowed.
                         ty::RawPtr(..) | ty::Ref(_, _, hir::MutImmutable) => true,
-                        _ => proj.base.ignore_borrow(tcx, mir),
+                        _ => proj.base.ignore_borrow(tcx, mir, locals_state_at_exit),
                     }
                 }
             },

src/librustc_mir/build/expr/as_operand.rs

@@ -73,7 +73,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
             Category::Place |
             Category::Rvalue(..) => {
                 let operand =
-                    unpack!(block = this.as_temp(block, scope, expr));
+                    unpack!(block = this.as_temp(block, scope, expr, Mutability::Mut));
                 block.and(Operand::Move(Place::Local(operand)))
             }
         }

src/librustc_mir/build/expr/as_place.rs

@@ -28,22 +28,42 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
         where M: Mirror<'tcx, Output=Expr<'tcx>>
     {
         let expr = self.hir.mirror(expr);
-        self.expr_as_place(block, expr)
+        self.expr_as_place(block, expr, Mutability::Mut)
+    }
+
+    /// Compile `expr`, yielding a place that we can move from etc.
+    /// Mutability note: The caller of this method promises only to read from the resulting
+    /// place. The place itself may or may not be mutable:
+    /// * If this expr is a place expr like a.b, then we will return that place.
+    /// * Otherwise, a temporary is created: in that event, it will be an immutable temporary.
+    pub fn as_read_only_place<M>(&mut self,
+                        block: BasicBlock,
+                        expr: M)
+                        -> BlockAnd<Place<'tcx>>
+        where M: Mirror<'tcx, Output=Expr<'tcx>>
+    {
+        let expr = self.hir.mirror(expr);
+        self.expr_as_place(block, expr, Mutability::Not)
     }
 
     fn expr_as_place(&mut self,
                       mut block: BasicBlock,
-                      expr: Expr<'tcx>)
+                      expr: Expr<'tcx>,
+                      mutability: Mutability)
                       -> BlockAnd<Place<'tcx>> {
-        debug!("expr_as_place(block={:?}, expr={:?})", block, expr);
+        debug!("expr_as_place(block={:?}, expr={:?}, mutability={:?})", block, expr, mutability);
 
         let this = self;
         let expr_span = expr.span;
         let source_info = this.source_info(expr_span);
         match expr.kind {
             ExprKind::Scope { region_scope, lint_level, value } => {
                 this.in_scope((region_scope, source_info), lint_level, block, |this| {
-                    this.as_place(block, value)
+                    if mutability == Mutability::Not {
+                        this.as_read_only_place(block, value)
+                    } else {
+                        this.as_place(block, value)
+                    }
                 })
             }
             ExprKind::Field { lhs, name } => {
@@ -63,7 +83,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                 // region_scope=None so place indexes live forever. They are scalars so they
                 // do not need storage annotations, and they are often copied between
                 // places.
-                let idx = unpack!(block = this.as_temp(block, None, index));
+                let idx = unpack!(block = this.as_temp(block, None, index, Mutability::Mut));
 
                 // bounds check:
                 let (len, lt) = (this.temp(usize_ty.clone(), expr_span),
@@ -137,7 +157,8 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                     Some(Category::Place) => false,
                     _ => true,
                 });
-                let temp = unpack!(block = this.as_temp(block, expr.temp_lifetime, expr));
+                let temp = unpack!(
+                    block = this.as_temp(block, expr.temp_lifetime, expr, mutability));
                 block.and(Place::Local(temp))
             }
         }

src/librustc_mir/build/expr/as_rvalue.rs

@@ -63,7 +63,10 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                 block.and(Rvalue::Repeat(value_operand, count))
             }
             ExprKind::Borrow { region, borrow_kind, arg } => {
-                let arg_place = unpack!(block = this.as_place(block, 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(region, borrow_kind, arg_place))
             }
             ExprKind::Binary { op, lhs, rhs } => {