Give match arms a drop/region scope

Also give arms the correct lint scope in MIR.

Author: matthewjasper

src/librustc/cfg/construct.rs

@@ -419,7 +419,7 @@ impl<'a, 'tcx> CFGBuilder<'a, 'tcx> {
         for arm in arms {
             // Add an exit node for when we've visited all the
             // patterns and the guard (if there is one) in the arm.
-            let arm_exit = self.add_dummy_node(&[]);
+            let bindings_exit = self.add_dummy_node(&[]);
 
             for pat in &arm.pats {
                 // Visit the pattern, coming from the discriminant exit
@@ -453,14 +453,16 @@ impl<'a, 'tcx> CFGBuilder<'a, 'tcx> {
 
                 // Add an edge from the exit of this pattern to the
                 // exit of the arm
-                self.add_contained_edge(pat_exit, arm_exit);
+                self.add_contained_edge(pat_exit, bindings_exit);
             }
 
             // Visit the body of this arm
-            let body_exit = self.expr(&arm.body, arm_exit);
+            let body_exit = self.expr(&arm.body, bindings_exit);
+
+            let arm_exit = self.add_ast_node(arm.hir_id.local_id, &[body_exit]);
 
             // Link the body to the exit of the expression
-            self.add_contained_edge(body_exit, expr_exit);
+            self.add_contained_edge(arm_exit, expr_exit);
         }
 
         expr_exit

src/librustc/middle/region.rs

@@ -119,18 +119,18 @@ impl fmt::Debug for Scope {
 pub enum ScopeData {
     Node,
 
-    // Scope of the call-site for a function or closure
-    // (outlives the arguments as well as the body).
+    /// Scope of the call-site for a function or closure
+    /// (outlives the arguments as well as the body).
     CallSite,
 
-    // Scope of arguments passed to a function or closure
-    // (they outlive its body).
+    /// Scope of arguments passed to a function or closure
+    /// (they outlive its body).
     Arguments,
 
-    // Scope of destructors for temporaries of node-id.
+    /// Scope of destructors for temporaries of node-id.
     Destruction,
 
-    // Scope following a `let id = expr;` binding in a block.
+    /// Scope following a `let id = expr;` binding in a block.
     Remainder(FirstStatementIndex)
 }
 
@@ -152,11 +152,11 @@ newtype_index! {
     ///
     /// * The subscope with `first_statement_index == 1` is scope of `c`,
     ///   and thus does not include EXPR_2, but covers the `...`.
-    pub struct FirstStatementIndex { .. }
+    pub struct FirstStatementIndex {
+        derive [HashStable]
+    }
 }
 
-impl_stable_hash_for!(struct crate::middle::region::FirstStatementIndex { private });
-
 // compilation error if size of `ScopeData` is not the same as a `u32`
 static_assert_size!(ScopeData, 4);
 
@@ -814,13 +814,25 @@ fn resolve_block<'a, 'tcx>(visitor: &mut RegionResolutionVisitor<'a, 'tcx>, blk:
 }
 
 fn resolve_arm<'a, 'tcx>(visitor: &mut RegionResolutionVisitor<'a, 'tcx>, arm: &'tcx hir::Arm) {
+    let prev_cx = visitor.cx;
+
+    visitor.enter_scope(
+        Scope {
+            id: arm.hir_id.local_id,
+            data: ScopeData::Node,
+        }
+    );
+    visitor.cx.var_parent = visitor.cx.parent;
+
     visitor.terminating_scopes.insert(arm.body.hir_id.local_id);
 
     if let Some(hir::Guard::If(ref expr)) = arm.guard {
         visitor.terminating_scopes.insert(expr.hir_id.local_id);
     }
 
     intravisit::walk_arm(visitor, arm);
+
+    visitor.cx = prev_cx;
 }
 
 fn resolve_pat<'a, 'tcx>(visitor: &mut RegionResolutionVisitor<'a, 'tcx>, pat: &'tcx hir::Pat) {
@@ -893,10 +905,6 @@ fn resolve_expr<'a, 'tcx>(visitor: &mut RegionResolutionVisitor<'a, 'tcx>, expr:
                 terminating(body.hir_id.local_id);
             }
 
-            hir::ExprKind::Match(..) => {
-                visitor.cx.var_parent = visitor.cx.parent;
-            }
-
             hir::ExprKind::DropTemps(ref expr) => {
                 // `DropTemps(expr)` does not denote a conditional scope.
                 // Rather, we want to achieve the same behavior as `{ let _t = expr; _t }`.

src/librustc_mir/build/matches/mod.rs

@@ -12,6 +12,7 @@ use crate::build::{GuardFrame, GuardFrameLocal, LocalsForNode};
 use crate::hair::{self, *};
 use rustc::hir::HirId;
 use rustc::mir::*;
+use rustc::middle::region;
 use rustc::ty::{self, CanonicalUserTypeAnnotation, Ty};
 use rustc::ty::layout::VariantIdx;
 use rustc_data_structures::bit_set::BitSet;
@@ -251,45 +252,47 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
 
         // Step 5. Create everything else: the guards and the arms.
 
-        let outer_source_info = self.source_info(span);
         let arm_end_blocks: Vec<_> = arm_candidates.into_iter().map(|(arm, candidates)| {
-            let mut arm_block = self.cfg.start_new_block();
-
-            let body = self.hir.mirror(arm.body.clone());
-            let scope = self.declare_bindings(
-                None,
-                body.span,
-                &arm.patterns[0],
-                ArmHasGuard(arm.guard.is_some()),
-                Some((Some(&scrutinee_place), scrutinee_span)),
-            );
-
-            if let Some(source_scope) = scope {
-                this.source_scope = source_scope;
-            }
-
-            for candidate in candidates {
-                self.bind_and_guard_matched_candidate(
-                    candidate,
-                    arm.guard.clone(),
-                    arm_block,
-                    &fake_borrow_temps,
-                    scrutinee_span,
+            let arm_source_info = self.source_info(arm.span);
+            let region_scope = (arm.scope, arm_source_info);
+            self.in_scope(region_scope, arm.lint_level, |this| {
+                let arm_block = this.cfg.start_new_block();
+
+                let body = this.hir.mirror(arm.body.clone());
+                let scope = this.declare_bindings(
+                    None,
+                    arm.span,
+                    &arm.patterns[0],
+                    ArmHasGuard(arm.guard.is_some()),
+                    Some((Some(&scrutinee_place), scrutinee_span)),
                 );
-            }
 
+                if let Some(source_scope) = scope {
+                    this.source_scope = source_scope;
+                }
 
-            unpack!(arm_block = self.into(destination, arm_block, body));
+                for candidate in candidates {
+                    this.clear_top_scope(arm.scope);
+                    this.bind_and_guard_matched_candidate(
+                        candidate,
+                        arm.guard.clone(),
+                        arm_block,
+                        &fake_borrow_temps,
+                        scrutinee_span,
+                        region_scope,
+                    );
+                }
 
-            arm_block
+                this.into(destination, arm_block, body)
+            })
         }).collect();
 
         // all the arm blocks will rejoin here
         let end_block = self.cfg.start_new_block();
 
         for arm_block in arm_end_blocks {
             self.cfg.terminate(
-                arm_block,
+                unpack!(arm_block),
                 outer_source_info,
                 TerminatorKind::Goto { target: end_block },
             );
@@ -502,7 +505,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
                     visibility_scope =
                         Some(this.new_source_scope(scope_span, LintLevel::Inherited, None));
                 }
-                let source_info = SourceInfo { span, this.source_scope };
+                let source_info = SourceInfo { span, scope: this.source_scope };
                 let visibility_scope = visibility_scope.unwrap();
                 this.declare_binding(
                     source_info,
@@ -1315,6 +1318,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
         arm_block: BasicBlock,
         fake_borrows: &Vec<(&Place<'tcx>, Local)>,
         scrutinee_span: Span,
+        region_scope: (region::Scope, SourceInfo),
     ) {
         debug!("bind_and_guard_matched_candidate(candidate={:?})", candidate);
 
@@ -1497,17 +1501,40 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
             //
             // and that is clearly not correct.
             let post_guard_block = self.cfg.start_new_block();
+            let otherwise_post_guard_block = self.cfg.start_new_block();
             self.cfg.terminate(
                 block,
                 source_info,
                 TerminatorKind::if_(
                     self.hir.tcx(),
-                    cond,
+                    cond.clone(),
                     post_guard_block,
-                    candidate.otherwise_block.unwrap()
+                    otherwise_post_guard_block,
                 ),
             );
 
+            self.exit_scope(
+                source_info.span,
+                region_scope,
+                otherwise_post_guard_block,
+                candidate.otherwise_block.unwrap(),
+            );
+
+            if let Operand::Copy(cond_place) | Operand::Move(cond_place) = cond {
+                if let Place::Base(PlaceBase::Local(cond_temp)) = cond_place {
+                    // We will call `clear_top_scope` if there's another guard. So
+                    // we have to drop this variable now or it will be "storage
+                    // leaked".
+                    self.pop_variable(
+                        post_guard_block,
+                        region_scope.0,
+                        cond_temp
+                    );
+                } else {
+                    bug!("Expected as_local_operand to produce a temporary");
+                }
+            }
+
             let by_value_bindings = candidate.bindings.iter().filter(|binding| {
                 if let BindingMode::ByValue = binding.binding_mode { true } else { false }
             });

src/librustc_mir/build/scope.rs

@@ -19,13 +19,18 @@ paragraph). This is because region scopes are tied to
 them. Eventually, when we shift to non-lexical lifetimes, there should
 be no need to remember this mapping.
 
-There is one additional wrinkle, actually, that I wanted to hide from
-you but duty compels me to mention. In the course of building
-matches, it sometimes happen that certain code (namely guards) gets
-executed multiple times. This means that the scope lexical scope may
-in fact correspond to multiple, disjoint SEME regions. So in fact our
+### Not so SEME Regions
+
+In the course of building matches, it sometimes happens that certain code
+(namely guards) gets executed multiple times. This means that the scope lexical
+scope may in fact correspond to multiple, disjoint SEME regions. So in fact our
 mapping is from one scope to a vector of SEME regions.
 
+Also in matches, the scopes assigned to arms are not even SEME regions! Each
+arm has a single region with one entry for each pattern. We manually
+manipulate the scheduled drops in this scope to avoid dropping things multiple
+times, although drop elaboration would clean this up for value drops.
+
 ### Drops
 
 The primary purpose for scopes is to insert drops: while building
@@ -731,7 +736,7 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
             // Note that this code iterates scopes from the inner-most to the outer-most,
             // invalidating caches of each scope visited. This way bare minimum of the
             // caches gets invalidated. i.e., if a new drop is added into the middle scope, the
-            // cache of outer scpoe stays intact.
+            // cache of outer scope stays intact.
             scope.invalidate_cache(!needs_drop, this_scope);
             if this_scope {
                 if let DropKind::Value { .. } = drop_kind {
@@ -873,6 +878,73 @@ impl<'a, 'gcx, 'tcx> Builder<'a, 'gcx, 'tcx> {
 
         success_block
     }
+
+    // `match` arm scopes
+    // ==================
+    /// Unschedules any drops in the top scope.
+    ///
+    /// This is only needed for `match` arm scopes, because they have one
+    /// entrance per pattern, but only one exit.
+    pub fn clear_top_scope(&mut self, region_scope: region::Scope) {
+        let top_scope = self.scopes.last_mut().unwrap();
+
+        assert_eq!(top_scope.region_scope, region_scope);
+
+        top_scope.drops.clear();
+        top_scope.invalidate_cache(false, true);
+    }
+
+    /// Drops the single variable provided
+    ///
+    /// * The scope must be the top scope.
+    /// * The variable must be in that scope.
+    /// * The variable must be at the top of that scope: it's the next thing
+    ///   scheduled to drop.
+    /// * The drop must be of DropKind::Storage.
+    ///
+    /// This is used for the boolean holding the result of the match guard. We
+    /// do this because:
+    ///
+    /// * The boolean is different for each pattern
+    /// * There is only one exit for the arm scope
+    /// * The guard expression scope is too short, it ends just before the
+    ///   boolean is tested.
+    pub fn pop_variable(
+        &mut self,
+        block: BasicBlock,
+        region_scope: region::Scope,
+        variable: Local,
+    ) {
+        let top_scope = self.scopes.last_mut().unwrap();
+
+        assert_eq!(top_scope.region_scope, region_scope);
+
+        let top_drop_data = top_scope.drops.pop().unwrap();
+
+        match top_drop_data.kind {
+            DropKind::Value { .. } => {
+                bug!("Should not be calling pop_top_variable on non-copy type!")
+            }
+            DropKind::Storage => {
+                // Drop the storage for both value and storage drops.
+                // Only temps and vars need their storage dead.
+                match top_drop_data.location {
+                    Place::Base(PlaceBase::Local(index)) => {
+                        let source_info = top_scope.source_info(top_drop_data.span);
+                        assert_eq!(index, variable);
+                        self.cfg.push(block, Statement {
+                            source_info,
+                            kind: StatementKind::StorageDead(index)
+                        });
+                    }
+                    _ => unreachable!(),
+                }
+            }
+        }
+
+        top_scope.invalidate_cache(true, true);
+    }
+
 }
 
 /// Builds drops for pop_scope and exit_scope.

src/librustc_mir/hair/cx/expr.rs

@@ -879,8 +879,12 @@ fn convert_arm<'a, 'gcx, 'tcx>(cx: &mut Cx<'a, 'gcx, 'tcx>, arm: &'tcx hir::Arm)
                 _ => None,
             },
         body: arm.body.to_ref(),
-        // BUG: fix this
-        lint_level: LintLevel::Inherited,
+        lint_level: LintLevel::Explicit(arm.hir_id),
+        scope: region::Scope {
+            id: arm.hir_id.local_id,
+            data: region::ScopeData::Node
+        },
+        span: arm.span,
     }
 }