Auto merge of #46320 - arielb1:always-resume, r=nikomatsakis

Always unwind through a Resume and other fixes

Should fix most of the small MIR borrowck issues.

r? @nikomatsakis

Author: bors

src/librustc/mir/mod.rs

@@ -733,6 +733,10 @@ impl<'tcx> Terminator<'tcx> {
     pub fn successors_mut(&mut self) -> Vec<&mut BasicBlock> {
         self.kind.successors_mut()
     }
+
+    pub fn unwind_mut(&mut self) -> Option<&mut Option<BasicBlock>> {
+        self.kind.unwind_mut()
+    }
 }
 
 impl<'tcx> TerminatorKind<'tcx> {
@@ -811,6 +815,27 @@ impl<'tcx> TerminatorKind<'tcx> {
             }
         }
     }
+
+    pub fn unwind_mut(&mut self) -> Option<&mut Option<BasicBlock>> {
+        match *self {
+            TerminatorKind::Goto { .. } |
+            TerminatorKind::Resume |
+            TerminatorKind::Return |
+            TerminatorKind::Unreachable |
+            TerminatorKind::GeneratorDrop |
+            TerminatorKind::Yield { .. } |
+            TerminatorKind::SwitchInt { .. } |
+            TerminatorKind::FalseEdges { .. } => {
+                None
+            },
+            TerminatorKind::Call { cleanup: ref mut unwind, .. } |
+            TerminatorKind::Assert { cleanup: ref mut unwind, .. } |
+            TerminatorKind::DropAndReplace { ref mut unwind, .. } |
+            TerminatorKind::Drop { ref mut unwind, .. } => {
+                Some(unwind)
+            }
+        }
+    }
 }
 
 impl<'tcx> BasicBlockData<'tcx> {

src/librustc_mir/borrow_check.rs

@@ -384,33 +384,23 @@ impl<'cx, 'gcx, 'tcx> DataflowResultsConsumer<'cx, 'tcx> for MirBorrowckCtxt<'cx
                 // StorageDead, but we don't always emit those (notably on unwind paths),
                 // so this "extra check" serves as a kind of backup.
                 let domain = flow_state.borrows.base_results.operator();
-                for borrow in domain.borrows() {
-                    let root_place = self.prefixes(
-                        &borrow.place,
-                        PrefixSet::All
-                    ).last().unwrap();
-                    match root_place {
-                        Place::Static(_) => {
-                            self.access_place(
-                                ContextKind::StorageDead.new(loc),
-                                (&root_place, self.mir.source_info(borrow.location).span),
-                                (Deep, Write(WriteKind::StorageDeadOrDrop)),
-                                LocalMutationIsAllowed::Yes,
-                                flow_state
-                            );
-                        }
-                        Place::Local(_) => {
-                            self.access_place(
-                                ContextKind::StorageDead.new(loc),
-                                (&root_place, self.mir.source_info(borrow.location).span),
-                                (Shallow(None), Write(WriteKind::StorageDeadOrDrop)),
-                                LocalMutationIsAllowed::Yes,
-                                flow_state
-                            );
-                        }
-                        Place::Projection(_) => ()
+                let data = domain.borrows();
+                flow_state.borrows.with_elems_outgoing(|borrows| for i in borrows {
+                    let borrow = &data[i];
+
+                    if self.place_is_invalidated_at_exit(&borrow.place) {
+                        debug!("borrow conflicts at exit {:?}", borrow);
+                        let borrow_span = self.mir.source_info(borrow.location).span;
+                        // FIXME: should be talking about the region lifetime instead
+                        // of just a span here.
+                        let end_span = domain.opt_region_end_span(&borrow.region);
+
+                        self.report_borrowed_value_does_not_live_long_enough(
+                            ContextKind::StorageDead.new(loc),
+                            (&borrow.place, borrow_span),
+                            end_span)
                     }
-                }
+                });
             }
             TerminatorKind::Goto { target: _ } |
             TerminatorKind::Unreachable |
@@ -594,7 +584,7 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
                                     context, common_prefix, place_span, bk,
                                     &borrow, end_issued_loan_span)
                             }
-                             WriteKind::StorageDeadOrDrop => {
+                            WriteKind::StorageDeadOrDrop => {
                                 let end_span =
                                     flow_state.borrows.base_results.operator().opt_region_end_span(
                                         &borrow.region);
@@ -751,6 +741,50 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
             Operand::Constant(_) => {}
         }
     }
+
+    /// Returns whether a borrow of this place is invalidated when the function
+    /// exits
+    fn place_is_invalidated_at_exit(&self, place: &Place<'tcx>) -> bool {
+        debug!("place_is_invalidated_at_exit({:?})", place);
+        let root_place = self.prefixes(place, PrefixSet::All).last().unwrap();
+
+        // FIXME(nll-rfc#40): do more precise destructor tracking here. For now
+        // we just know that all locals are dropped at function exit (otherwise
+        // we'll have a memory leak) and assume that all statics have a destructor.
+        let (might_be_alive, will_be_dropped) = match root_place {
+            Place::Static(statik) => {
+                // Thread-locals might be dropped after the function exits, but
+                // "true" statics will never be.
+                let is_thread_local = self.tcx.get_attrs(statik.def_id).iter().any(|attr| {
+                    attr.check_name("thread_local")
+                });
+
+                (true, is_thread_local)
+            }
+            Place::Local(_) => {
+                // Locals are always dropped at function exit, and if they
+                // have a destructor it would've been called already.
+                (false, true)
+            }
+            Place::Projection(..) => bug!("root of {:?} is a projection ({:?})?",
+                                           place, root_place)
+        };
+
+        if !will_be_dropped {
+            debug!("place_is_invalidated_at_exit({:?}) - won't be dropped", place);
+            return false;
+        }
+
+        // FIXME: replace this with a proper borrow_conflicts_with_place when
+        // that is merged.
+        let prefix_set = if might_be_alive {
+            PrefixSet::Supporting
+        } else {
+            PrefixSet::Shallow
+        };
+
+        self.prefixes(place, prefix_set).any(|prefix| prefix == root_place)
+    }
 }
 
 impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
@@ -1667,13 +1701,13 @@ impl<'cx, 'gcx, 'tcx> MirBorrowckCtxt<'cx, 'gcx, 'tcx> {
 
     fn report_borrowed_value_does_not_live_long_enough(&mut self,
                                                        _: Context,
-                                                       (place, span): (&Place, Span),
+                                                       (place, span): (&Place<'tcx>, Span),
                                                        end_span: Option<Span>) {
-        let proper_span = match *place {
+        let root_place = self.prefixes(place, PrefixSet::All).last().unwrap();
+        let proper_span = match *root_place {
             Place::Local(local) => self.mir.local_decls[local].source_info.span,
             _ => span
         };
-
         let mut err = self.tcx.path_does_not_live_long_enough(span, "borrowed value", Origin::Mir);
         err.span_label(proper_span, "temporary value created here");
         err.span_label(span, "temporary value dropped here while still borrowed");
@@ -2162,4 +2196,12 @@ impl<BD> FlowInProgress<BD> where BD: BitDenotation {
         let univ = self.base_results.sets().bits_per_block();
         self.curr_state.elems(univ)
     }
+
+    fn with_elems_outgoing<F>(&self, f: F) where F: FnOnce(indexed_set::Elems<BD::Idx>) {
+        let mut curr_state = self.curr_state.clone();
+        curr_state.union(&self.stmt_gen);
+        curr_state.subtract(&self.stmt_kill);
+        let univ = self.base_results.sets().bits_per_block();
+        f(curr_state.elems(univ));
+    }
 }

src/librustc_mir/build/expr/into.rs

@@ -255,7 +255,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                     this.cfg.terminate(block, source_info, TerminatorKind::Call {
                         func: fun,
                         args,
-                        cleanup,
+                        cleanup: Some(cleanup),
                         destination: if diverges {
                             None
                         } else {
@@ -273,7 +273,9 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
             ExprKind::Break { .. } |
             ExprKind::InlineAsm { .. } |
             ExprKind::Return {.. } => {
-                this.stmt_expr(block, expr)
+                unpack!(block = this.stmt_expr(block, expr));
+                this.cfg.push_assign_unit(block, source_info, destination);
+                block.unit()
             }
 
             // these are the cases that are more naturally handled by some other mode

src/librustc_mir/build/matches/test.rs

@@ -315,7 +315,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                         }),
                         args: vec![val, expect],
                         destination: Some((eq_result.clone(), eq_block)),
-                        cleanup,
+                        cleanup: Some(cleanup),
                     });
 
                     // check the result

src/librustc_mir/build/scope.rs

@@ -383,7 +383,9 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
         assert_eq!(scope.region_scope, region_scope.0);
 
         self.cfg.push_end_region(self.hir.tcx(), block, region_scope.1, scope.region_scope);
+        let resume_block = self.resume_block();
         unpack!(block = build_scope_drops(&mut self.cfg,
+                                          resume_block,
                                           &scope,
                                           &self.scopes,
                                           block,
@@ -422,6 +424,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
         }
 
         {
+        let resume_block = self.resume_block();
         let mut rest = &mut self.scopes[(len - scope_count)..];
         while let Some((scope, rest_)) = {rest}.split_last_mut() {
             rest = rest_;
@@ -441,6 +444,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
             self.cfg.push_end_region(self.hir.tcx(), block, region_scope.1, scope.region_scope);
 
             unpack!(block = build_scope_drops(&mut self.cfg,
+                                              resume_block,
                                               scope,
                                               rest,
                                               block,
@@ -468,6 +472,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
         let src_info = self.scopes[0].source_info(self.fn_span);
         let mut block = self.cfg.start_new_block();
         let result = block;
+        let resume_block = self.resume_block();
         let mut rest = &mut self.scopes[..];
 
         while let Some((scope, rest_)) = {rest}.split_last_mut() {
@@ -491,6 +496,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
             self.cfg.push_end_region(self.hir.tcx(), block, src_info, scope.region_scope);
 
             unpack!(block = build_scope_drops(&mut self.cfg,
+                                              resume_block,
                                               scope,
                                               rest,
                                               block,
@@ -701,18 +707,31 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
     /// This path terminates in Resume. Returns the start of the path.
     /// See module comment for more details. None indicates there’s no
     /// cleanup to do at this point.
-    pub fn diverge_cleanup(&mut self) -> Option<BasicBlock> {
+    pub fn diverge_cleanup(&mut self) -> BasicBlock {
         self.diverge_cleanup_gen(false)
     }
 
-    fn diverge_cleanup_gen(&mut self, generator_drop: bool) -> Option<BasicBlock> {
-        if !self.scopes.iter().any(|scope| scope.needs_cleanup) {
-            return None;
+    fn resume_block(&mut self) -> BasicBlock {
+        if let Some(target) = self.cached_resume_block {
+            target
+        } else {
+            let resumeblk = self.cfg.start_new_cleanup_block();
+            self.cfg.terminate(resumeblk,
+                               SourceInfo {
+                                   scope: ARGUMENT_VISIBILITY_SCOPE,
+                                   span: self.fn_span
+                               },
+                               TerminatorKind::Resume);
+            self.cached_resume_block = Some(resumeblk);
+            resumeblk
         }
-        assert!(!self.scopes.is_empty()); // or `any` above would be false
+    }
+
+    fn diverge_cleanup_gen(&mut self, generator_drop: bool) -> BasicBlock {
+        // To start, create the resume terminator.
+        let mut target = self.resume_block();
 
-        let Builder { ref mut cfg, ref mut scopes,
-                      ref mut cached_resume_block, .. } = *self;
+        let Builder { ref mut cfg, ref mut scopes, .. } = *self;
 
         // Build up the drops in **reverse** order. The end result will
         // look like:
@@ -725,23 +744,14 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
         // store caches. If everything is cached, we'll just walk right
         // to left reading the cached results but never created anything.
 
-        // To start, create the resume terminator.
-        let mut target = if let Some(target) = *cached_resume_block {
-            target
-        } else {
-            let resumeblk = cfg.start_new_cleanup_block();
-            cfg.terminate(resumeblk,
-                          scopes[0].source_info(self.fn_span),
-                          TerminatorKind::Resume);
-            *cached_resume_block = Some(resumeblk);
-            resumeblk
-        };
-
-        for scope in scopes.iter_mut() {
-            target = build_diverge_scope(self.hir.tcx(), cfg, scope.region_scope_span,
-                                         scope, target, generator_drop);
+        if scopes.iter().any(|scope| scope.needs_cleanup) {
+            for scope in scopes.iter_mut() {
+                target = build_diverge_scope(self.hir.tcx(), cfg, scope.region_scope_span,
+                                             scope, target, generator_drop);
+            }
         }
-        Some(target)
+
+        target
     }
 
     /// Utility function for *non*-scope code to build their own drops
@@ -760,7 +770,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                            TerminatorKind::Drop {
                                location,
                                target: next_target,
-                               unwind: diverge_target,
+                               unwind: Some(diverge_target),
                            });
         next_target.unit()
     }
@@ -779,7 +789,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                                location,
                                value,
                                target: next_target,
-                               unwind: diverge_target,
+                               unwind: Some(diverge_target),
                            });
         next_target.unit()
     }
@@ -804,7 +814,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                                expected,
                                msg,
                                target: success_block,
-                               cleanup,
+                               cleanup: Some(cleanup),
                            });
 
         success_block
@@ -813,6 +823,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
 
 /// Builds drops for pop_scope and exit_scope.
 fn build_scope_drops<'tcx>(cfg: &mut CFG<'tcx>,
+                           resume_block: BasicBlock,
                            scope: &Scope<'tcx>,
                            earlier_scopes: &[Scope<'tcx>],
                            mut block: BasicBlock,
@@ -868,7 +879,7 @@ fn build_scope_drops<'tcx>(cfg: &mut CFG<'tcx>,
                 cfg.terminate(block, source_info, TerminatorKind::Drop {
                     location: drop_data.location.clone(),
                     target: next,
-                    unwind: on_diverge
+                    unwind: Some(on_diverge.unwrap_or(resume_block))
                 });
                 block = next;
             }

src/librustc_mir/dataflow/impls/borrows.rs

@@ -132,6 +132,10 @@ impl<'a, 'gcx, 'tcx> Borrows<'a, 'gcx, 'tcx> {
         &self.borrows[idx].location
     }
 
+    pub fn nonlexical_regioncx(&self) -> Option<&'a RegionInferenceContext<'tcx>> {
+        self.nonlexical_regioncx
+    }
+
     /// Returns the span for the "end point" given region. This will
     /// return `None` if NLL is enabled, since that concept has no
     /// meaning there.  Otherwise, return region span if it exists and
@@ -208,6 +212,12 @@ impl<'a, 'gcx, 'tcx> BitDenotation for Borrows<'a, 'gcx, 'tcx> {
             mir::StatementKind::Assign(_, ref rhs) => {
                 if let mir::Rvalue::Ref(region, _, ref place) = *rhs {
                     if is_unsafe_place(self.tcx, self.mir, place) { return; }
+                    if let RegionKind::ReEmpty = region {
+                        // If the borrowed value is dead, the region for it
+                        // can be empty. Don't track the borrow in that case.
+                        return
+                    }
+
                     let index = self.location_map.get(&location).unwrap_or_else(|| {
                         panic!("could not find BorrowIndex for location {:?}", location);
                     });