Additional cleanup

This cleans up the refactoring from the previous patch and cleans things
up a bit. Each module has a clear entry point and everything else is
private.

compiler/rustc_typeck/src/check/generator_interior/drop_ranges.rs

@@ -12,39 +12,33 @@
 //! The end result is a data structure that maps the post-order index of each node in the HIR tree
 //! to a set of values that are known to be dropped at that location.
 
-use self::cfg_build::DropRangeVisitor;
+use self::cfg_build::build_control_flow_graph;
-use self::record_consumed_borrow::ExprUseDelegate;
+use self::record_consumed_borrow::find_consumed_and_borrowed;
 use crate::check::FnCtxt;
 use hir::def_id::DefId;
-use hir::{Body, HirId, HirIdMap, Node, intravisit};
+use hir::{Body, HirId, HirIdMap, Node};
 use rustc_hir as hir;
 use rustc_index::bit_set::BitSet;
 use rustc_index::vec::IndexVec;
-use rustc_middle::hir::map::Map;
 use std::collections::BTreeMap;
 use std::fmt::Debug;
-use std::mem::swap;
 
 mod cfg_build;
-mod record_consumed_borrow;
 mod cfg_propagate;
 mod cfg_visualize;
+mod record_consumed_borrow;
 
 pub fn compute_drop_ranges<'a, 'tcx>(
     fcx: &'a FnCtxt<'a, 'tcx>,
     def_id: DefId,
     body: &'tcx Body<'tcx>,
 ) -> DropRanges {
-    let mut expr_use_visitor = ExprUseDelegate::new(fcx.tcx.hir());
+    let consumed_borrowed_places = find_consumed_and_borrowed(fcx, def_id, body);
-    expr_use_visitor.consume_body(fcx, def_id, body);
 
-    let mut drop_range_visitor = DropRangeVisitor::from_uses(
+    let num_exprs = fcx.tcx.region_scope_tree(def_id).body_expr_count(body.id()).unwrap_or(0);
-        expr_use_visitor,
+    let mut drop_ranges =
-        fcx.tcx.region_scope_tree(def_id).body_expr_count(body.id()).unwrap_or(0),
+        build_control_flow_graph(fcx.tcx.hir(), consumed_borrowed_places, body, num_exprs);
-    );
-    intravisit::walk_body(&mut drop_range_visitor, body);
 
-    let mut drop_ranges = drop_range_visitor.into_drop_ranges();
     drop_ranges.propagate_to_fixpoint();
 
     drop_ranges
@@ -105,31 +99,6 @@ impl Debug for DropRanges {
 /// (hir_id, post_order_id) -> bool, where a true value indicates that the value is definitely
 /// dropped at the point of the node identified by post_order_id.
 impl DropRanges {
-    pub fn new(hir_ids: impl Iterator<Item = HirId>, hir: &Map<'_>, num_exprs: usize) -> Self {
-        let mut hir_id_map = HirIdMap::<HirIdIndex>::default();
-        let mut next = <_>::from(0u32);
-        for hir_id in hir_ids {
-            for_each_consumable(hir_id, hir.find(hir_id), |hir_id| {
-                if !hir_id_map.contains_key(&hir_id) {
-                    hir_id_map.insert(hir_id, next);
-                    next = <_>::from(next.index() + 1);
-                }
-            });
-        }
-        debug!("hir_id_map: {:?}", hir_id_map);
-        let num_values = hir_id_map.len();
-        Self {
-            hir_id_map,
-            nodes: IndexVec::from_fn_n(|_| NodeInfo::new(num_values), num_exprs + 1),
-            deferred_edges: <_>::default(),
-            post_order_map: <_>::default(),
-        }
-    }
-
-    fn hidx(&self, hir_id: HirId) -> HirIdIndex {
-        *self.hir_id_map.get(&hir_id).unwrap()
-    }
-
     pub fn is_dropped_at(&mut self, hir_id: HirId, location: usize) -> bool {
         self.hir_id_map
             .get(&hir_id)
@@ -142,13 +111,6 @@ impl DropRanges {
         self.hir_id_map.len()
     }
 
-    /// Adds an entry in the mapping from HirIds to PostOrderIds
-    ///
-    /// Needed so that `add_control_edge_hir_id` can work.
-    pub fn add_node_mapping(&mut self, hir_id: HirId, post_order_id: usize) {
-        self.post_order_map.insert(hir_id, post_order_id);
-    }
-
     /// Returns a reference to the NodeInfo for a node, panicking if it does not exist
     fn expect_node(&self, id: PostOrderId) -> &NodeInfo {
         &self.nodes[id]
@@ -160,48 +122,10 @@ impl DropRanges {
         &mut self.nodes[id]
     }
 
-    pub fn add_control_edge(&mut self, from: usize, to: usize) {
+    fn add_control_edge(&mut self, from: usize, to: usize) {
         trace!("adding control edge from {} to {}", from, to);
         self.node_mut(from.into()).successors.push(to.into());
     }
-
-    /// Like add_control_edge, but uses a hir_id as the target.
-    ///
-    /// This can be used for branches where we do not know the PostOrderId of the target yet,
-    /// such as when handling `break` or `continue`.
-    pub fn add_control_edge_hir_id(&mut self, from: usize, to: HirId) {
-        self.deferred_edges.push((from, to));
-    }
-
-    /// Looks up PostOrderId for any control edges added by HirId and adds a proper edge for them.
-    ///
-    /// Should be called after visiting the HIR but before solving the control flow, otherwise some
-    /// edges will be missed.
-    fn process_deferred_edges(&mut self) {
-        let mut edges = vec![];
-        swap(&mut edges, &mut self.deferred_edges);
-        edges.into_iter().for_each(|(from, to)| {
-            let to = *self.post_order_map.get(&to).expect("Expression ID not found");
-            trace!("Adding deferred edge from {} to {}", from, to);
-            self.add_control_edge(from, to)
-        });
-    }
-
-    pub fn drop_at(&mut self, value: HirId, location: usize) {
-        let value = self.hidx(value);
-        self.node_mut(location.into()).drops.push(value);
-    }
-
-    pub fn reinit_at(&mut self, value: HirId, location: usize) {
-        let value = match self.hir_id_map.get(&value) {
-            Some(value) => *value,
-            // If there's no value, this is never consumed and therefore is never dropped. We can
-            // ignore this.
-            None => return,
-        };
-        self.node_mut(location.into()).reinits.push(value);
-    }
-
 }
 
 #[derive(Debug)]

compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_build.rs

@@ -1,51 +1,57 @@
-use super::{for_each_consumable, record_consumed_borrow::ExprUseDelegate, DropRanges};
+use super::{
+    for_each_consumable, record_consumed_borrow::ConsumedAndBorrowedPlaces, DropRanges, HirIdIndex,
+    NodeInfo,
+};
 use hir::{
     intravisit::{self, NestedVisitorMap, Visitor},
-    Expr, ExprKind, Guard, HirId, HirIdMap, HirIdSet,
+    Body, Expr, ExprKind, Guard, HirId, HirIdMap,
 };
 use rustc_hir as hir;
+use rustc_index::vec::IndexVec;
 use rustc_middle::hir::map::Map;
 
+/// Traverses the body to find the control flow graph and locations for the
+/// relevant places are dropped or reinitialized.
+///
+/// The resulting structure still needs to be iterated to a fixed point, which
+/// can be done with propagate_to_fixpoint in cfg_propagate.
+pub fn build_control_flow_graph<'tcx>(
+    hir: Map<'tcx>,
+    consumed_borrowed_places: ConsumedAndBorrowedPlaces,
+    body: &'tcx Body<'tcx>,
+    num_exprs: usize,
+) -> DropRanges {
+    let mut drop_range_visitor = DropRangeVisitor::new(hir, consumed_borrowed_places, num_exprs);
+    intravisit::walk_body(&mut drop_range_visitor, body);
+    drop_range_visitor.drop_ranges
+}
+
 /// This struct is used to gather the information for `DropRanges` to determine the regions of the
 /// HIR tree for which a value is dropped.
 ///
 /// We are interested in points where a variables is dropped or initialized, and the control flow
 /// of the code. We identify locations in code by their post-order traversal index, so it is
 /// important for this traversal to match that in `RegionResolutionVisitor` and `InteriorVisitor`.
-pub struct DropRangeVisitor<'tcx> {
+struct DropRangeVisitor<'tcx> {
     hir: Map<'tcx>,
-    /// Maps a HirId to a set of HirIds that are dropped by that node.
+    places: ConsumedAndBorrowedPlaces,
-    ///
-    /// See also the more detailed comment on `ExprUseDelegate.consumed_places`.
-    consumed_places: HirIdMap<HirIdSet>,
-    borrowed_places: HirIdSet,
     drop_ranges: DropRanges,
     expr_count: usize,
 }
 
 impl<'tcx> DropRangeVisitor<'tcx> {
-    pub fn from_uses(uses: ExprUseDelegate<'tcx>, num_exprs: usize) -> Self {
+    fn new(hir: Map<'tcx>, places: ConsumedAndBorrowedPlaces, num_exprs: usize) -> Self {
-        debug!("consumed_places: {:?}", uses.consumed_places);
+        debug!("consumed_places: {:?}", places.consumed);
         let drop_ranges = DropRanges::new(
-            uses.consumed_places.iter().flat_map(|(_, places)| places.iter().copied()),
+            places.consumed.iter().flat_map(|(_, places)| places.iter().copied()),
-            &uses.hir,
+            hir,
             num_exprs,
         );
-        Self {
+        Self { hir, places, drop_ranges, expr_count: 0 }
-            hir: uses.hir,
-            consumed_places: uses.consumed_places,
-            borrowed_places: uses.borrowed_places,
-            drop_ranges,
-            expr_count: 0,
-        }
-    }
-
-    pub fn into_drop_ranges(self) -> DropRanges {
-        self.drop_ranges
     }
 
     fn record_drop(&mut self, hir_id: HirId) {
-        if self.borrowed_places.contains(&hir_id) {
+        if self.places.borrowed.contains(&hir_id) {
             debug!("not marking {:?} as dropped because it is borrowed at some point", hir_id);
         } else {
             debug!("marking {:?} as dropped at {}", hir_id, self.expr_count);
@@ -59,7 +65,8 @@ impl<'tcx> DropRangeVisitor<'tcx> {
     fn consume_expr(&mut self, expr: &hir::Expr<'_>) {
         debug!("consuming expr {:?}, count={}", expr.hir_id, self.expr_count);
         let places = self
-            .consumed_places
+            .places
+            .consumed
             .get(&expr.hir_id)
             .map_or(vec![], |places| places.iter().cloned().collect());
         for place in places {
@@ -167,3 +174,60 @@ impl<'tcx> Visitor<'tcx> for DropRangeVisitor<'tcx> {
         self.expr_count += 1;
     }
 }
+
+impl DropRanges {
+    fn new(hir_ids: impl Iterator<Item = HirId>, hir: Map<'_>, num_exprs: usize) -> Self {
+        let mut hir_id_map = HirIdMap::<HirIdIndex>::default();
+        let mut next = <_>::from(0u32);
+        for hir_id in hir_ids {
+            for_each_consumable(hir_id, hir.find(hir_id), |hir_id| {
+                if !hir_id_map.contains_key(&hir_id) {
+                    hir_id_map.insert(hir_id, next);
+                    next = <_>::from(next.index() + 1);
+                }
+            });
+        }
+        debug!("hir_id_map: {:?}", hir_id_map);
+        let num_values = hir_id_map.len();
+        Self {
+            hir_id_map,
+            nodes: IndexVec::from_fn_n(|_| NodeInfo::new(num_values), num_exprs + 1),
+            deferred_edges: <_>::default(),
+            post_order_map: <_>::default(),
+        }
+    }
+
+    fn hidx(&self, hir_id: HirId) -> HirIdIndex {
+        *self.hir_id_map.get(&hir_id).unwrap()
+    }
+
+    /// Adds an entry in the mapping from HirIds to PostOrderIds
+    ///
+    /// Needed so that `add_control_edge_hir_id` can work.
+    fn add_node_mapping(&mut self, hir_id: HirId, post_order_id: usize) {
+        self.post_order_map.insert(hir_id, post_order_id);
+    }
+
+    /// Like add_control_edge, but uses a hir_id as the target.
+    ///
+    /// This can be used for branches where we do not know the PostOrderId of the target yet,
+    /// such as when handling `break` or `continue`.
+    fn add_control_edge_hir_id(&mut self, from: usize, to: HirId) {
+        self.deferred_edges.push((from, to));
+    }
+
+    fn drop_at(&mut self, value: HirId, location: usize) {
+        let value = self.hidx(value);
+        self.node_mut(location.into()).drops.push(value);
+    }
+
+    fn reinit_at(&mut self, value: HirId, location: usize) {
+        let value = match self.hir_id_map.get(&value) {
+            Some(value) => *value,
+            // If there's no value, this is never consumed and therefore is never dropped. We can
+            // ignore this.
+            None => return,
+        };
+        self.node_mut(location.into()).reinits.push(value);
+    }
+}

compiler/rustc_typeck/src/check/generator_interior/drop_ranges/cfg_propagate.rs

@@ -1,8 +1,7 @@
-use std::collections::BTreeMap;
-
-use rustc_index::{bit_set::BitSet, vec::IndexVec};
-
 use super::{DropRanges, PostOrderId};
+use rustc_index::{bit_set::BitSet, vec::IndexVec};
+use std::collections::BTreeMap;
+use std::mem::swap;
 
 impl DropRanges {
     pub fn propagate_to_fixpoint(&mut self) {
@@ -64,4 +63,18 @@ impl DropRanges {
         }
         preds
     }
+
+    /// Looks up PostOrderId for any control edges added by HirId and adds a proper edge for them.
+    ///
+    /// Should be called after visiting the HIR but before solving the control flow, otherwise some
+    /// edges will be missed.
+    fn process_deferred_edges(&mut self) {
+        let mut edges = vec![];
+        swap(&mut edges, &mut self.deferred_edges);
+        edges.into_iter().for_each(|(from, to)| {
+            let to = *self.post_order_map.get(&to).expect("Expression ID not found");
+            trace!("Adding deferred edge from {} to {}", from, to);
+            self.add_control_edge(from, to)
+        });
+    }
 }

compiler/rustc_typeck/src/check/generator_interior/drop_ranges/record_consumed_borrow.rs

@@ -2,7 +2,7 @@ use crate::{
     check::FnCtxt,
     expr_use_visitor::{self, ExprUseVisitor},
 };
-use hir::{HirId, HirIdMap, HirIdSet, Body, def_id::DefId};
+use hir::{def_id::DefId, Body, HirId, HirIdMap, HirIdSet};
 use rustc_hir as hir;
 use rustc_middle::hir::{
     map::Map,
@@ -10,12 +10,17 @@ use rustc_middle::hir::{
 };
 use rustc_middle::ty;
 
-/// Works with ExprUseVisitor to find interesting values for the drop range analysis.
+pub fn find_consumed_and_borrowed<'a, 'tcx>(
-///
+    fcx: &'a FnCtxt<'a, 'tcx>,
-/// Interesting values are those that are either dropped or borrowed. For dropped values, we also
+    def_id: DefId,
-/// record the parent expression, which is the point where the drop actually takes place.
+    body: &'tcx Body<'tcx>,
-pub struct ExprUseDelegate<'tcx> {
+) -> ConsumedAndBorrowedPlaces {
-    pub(super) hir: Map<'tcx>,
+    let mut expr_use_visitor = ExprUseDelegate::new(fcx.tcx.hir());
+    expr_use_visitor.consume_body(fcx, def_id, body);
+    expr_use_visitor.places
+}
+
+pub struct ConsumedAndBorrowedPlaces {
     /// Records the variables/expressions that are dropped by a given expression.
     ///
     /// The key is the hir-id of the expression, and the value is a set or hir-ids for variables
@@ -23,17 +28,32 @@ pub struct ExprUseDelegate<'tcx> {
     ///
     /// Note that this set excludes "partial drops" -- for example, a statement like `drop(x.y)` is
     /// not considered a drop of `x`, although it would be a drop of `x.y`.
-    pub(super) consumed_places: HirIdMap<HirIdSet>,
+    pub consumed: HirIdMap<HirIdSet>,
     /// A set of hir-ids of values or variables that are borrowed at some point within the body.
-    pub(super) borrowed_places: HirIdSet,
+    pub borrowed: HirIdSet,
+}
+
+/// Works with ExprUseVisitor to find interesting values for the drop range analysis.
+///
+/// Interesting values are those that are either dropped or borrowed. For dropped values, we also
+/// record the parent expression, which is the point where the drop actually takes place.
+struct ExprUseDelegate<'tcx> {
+    hir: Map<'tcx>,
+    places: ConsumedAndBorrowedPlaces,
 }
 
 impl<'tcx> ExprUseDelegate<'tcx> {
-    pub fn new(hir: Map<'tcx>) -> Self {
+    fn new(hir: Map<'tcx>) -> Self {
-        Self { hir, consumed_places: <_>::default(), borrowed_places: <_>::default() }
+        Self {
+            hir,
+            places: ConsumedAndBorrowedPlaces {
+                consumed: <_>::default(),
+                borrowed: <_>::default(),
+            },
+        }
     }
 
-    pub fn consume_body(
+    fn consume_body(
         &mut self,
         fcx: &'_ FnCtxt<'_, 'tcx>,
         def_id: DefId,
@@ -51,10 +71,10 @@ impl<'tcx> ExprUseDelegate<'tcx> {
     }
 
     fn mark_consumed(&mut self, consumer: HirId, target: HirId) {
-        if !self.consumed_places.contains_key(&consumer) {
+        if !self.places.consumed.contains_key(&consumer) {
-            self.consumed_places.insert(consumer, <_>::default());
+            self.places.consumed.insert(consumer, <_>::default());
         }
-        self.consumed_places.get_mut(&consumer).map(|places| places.insert(target));
+        self.places.consumed.get_mut(&consumer).map(|places| places.insert(target));
     }
 }
 
@@ -82,7 +102,7 @@ impl<'tcx> expr_use_visitor::Delegate<'tcx> for ExprUseDelegate<'tcx> {
         _diag_expr_id: HirId,
         _bk: rustc_middle::ty::BorrowKind,
     ) {
-        place_hir_id(&place_with_id.place).map(|place| self.borrowed_places.insert(place));
+        place_hir_id(&place_with_id.place).map(|place| self.places.borrowed.insert(place));
     }
 
     fn mutate(