Rollup merge of rust-lang#126538 - Zalathar:graph, r=nnethercote

jieyouxu · web-flow · commit d92aa567b984 · 2024-06-17T04:53:58.000+01:00
coverage: Several small improvements to graph code

This PR combines a few small improvements to coverage graph handling code:
- Remove some low-value implementation tests that were getting in the way of other changes.
- Clean up `pub` visibility.
- Flatten some code using let-else.
- Prefer `.copied()` over `.cloned()`.

`@rustbot` label +A-code-coverage
diff --git a/compiler/rustc_mir_transform/src/coverage/counters.rs b/compiler/rustc_mir_transform/src/coverage/counters.rs
@@ -168,11 +168,6 @@ impl CoverageCounters {
         self.counter_increment_sites.len()
     }
 
-    #[cfg(test)]
-    pub(super) fn num_expressions(&self) -> usize {
-        self.expressions.len()
-    }
-
     fn set_bcb_counter(&mut self, bcb: BasicCoverageBlock, counter_kind: BcbCounter) -> BcbCounter {
         if let Some(replaced) = self.bcb_counters[bcb].replace(counter_kind) {
             bug!(
diff --git a/compiler/rustc_mir_transform/src/coverage/graph.rs b/compiler/rustc_mir_transform/src/coverage/graph.rs
@@ -14,16 +14,16 @@ use std::ops::{Index, IndexMut};
 /// A coverage-specific simplification of the MIR control flow graph (CFG). The `CoverageGraph`s
 /// nodes are `BasicCoverageBlock`s, which encompass one or more MIR `BasicBlock`s.
 #[derive(Debug)]
-pub(super) struct CoverageGraph {
+pub(crate) struct CoverageGraph {
     bcbs: IndexVec<BasicCoverageBlock, BasicCoverageBlockData>,
     bb_to_bcb: IndexVec<BasicBlock, Option<BasicCoverageBlock>>,
-    pub successors: IndexVec<BasicCoverageBlock, Vec<BasicCoverageBlock>>,
-    pub predecessors: IndexVec<BasicCoverageBlock, Vec<BasicCoverageBlock>>,
+    pub(crate) successors: IndexVec<BasicCoverageBlock, Vec<BasicCoverageBlock>>,
+    pub(crate) predecessors: IndexVec<BasicCoverageBlock, Vec<BasicCoverageBlock>>,
     dominators: Option<Dominators<BasicCoverageBlock>>,
 }
 
 impl CoverageGraph {
-    pub fn from_mir(mir_body: &mir::Body<'_>) -> Self {
+    pub(crate) fn from_mir(mir_body: &mir::Body<'_>) -> Self {
         let (bcbs, bb_to_bcb) = Self::compute_basic_coverage_blocks(mir_body);
 
         // Pre-transform MIR `BasicBlock` successors and predecessors into the BasicCoverageBlock
@@ -135,24 +135,28 @@ impl CoverageGraph {
     }
 
     #[inline(always)]
-    pub fn iter_enumerated(
+    pub(crate) fn iter_enumerated(
         &self,
     ) -> impl Iterator<Item = (BasicCoverageBlock, &BasicCoverageBlockData)> {
         self.bcbs.iter_enumerated()
     }
 
     #[inline(always)]
-    pub fn bcb_from_bb(&self, bb: BasicBlock) -> Option<BasicCoverageBlock> {
+    pub(crate) fn bcb_from_bb(&self, bb: BasicBlock) -> Option<BasicCoverageBlock> {
         if bb.index() < self.bb_to_bcb.len() { self.bb_to_bcb[bb] } else { None }
     }
 
     #[inline(always)]
-    pub fn dominates(&self, dom: BasicCoverageBlock, node: BasicCoverageBlock) -> bool {
+    pub(crate) fn dominates(&self, dom: BasicCoverageBlock, node: BasicCoverageBlock) -> bool {
         self.dominators.as_ref().unwrap().dominates(dom, node)
     }
 
     #[inline(always)]
-    pub fn cmp_in_dominator_order(&self, a: BasicCoverageBlock, b: BasicCoverageBlock) -> Ordering {
+    pub(crate) fn cmp_in_dominator_order(
+        &self,
+        a: BasicCoverageBlock,
+        b: BasicCoverageBlock,
+    ) -> Ordering {
         self.dominators.as_ref().unwrap().cmp_in_dominator_order(a, b)
     }
 
@@ -166,7 +170,7 @@ impl CoverageGraph {
     ///
     /// FIXME: That assumption might not be true for [`TerminatorKind::Yield`]?
     #[inline(always)]
-    pub(super) fn bcb_has_multiple_in_edges(&self, bcb: BasicCoverageBlock) -> bool {
+    pub(crate) fn bcb_has_multiple_in_edges(&self, bcb: BasicCoverageBlock) -> bool {
         // Even though bcb0 conceptually has an extra virtual in-edge due to
         // being the entry point, we've already asserted that it has no _other_
         // in-edges, so there's no possibility of it having _multiple_ in-edges.
@@ -212,7 +216,7 @@ impl graph::StartNode for CoverageGraph {
 impl graph::Successors for CoverageGraph {
     #[inline]
     fn successors(&self, node: Self::Node) -> impl Iterator<Item = Self::Node> {
-        self.successors[node].iter().cloned()
+        self.successors[node].iter().copied()
     }
 }
 
@@ -227,7 +231,7 @@ rustc_index::newtype_index! {
     /// A node in the control-flow graph of CoverageGraph.
     #[orderable]
     #[debug_format = "bcb{}"]
-    pub(super) struct BasicCoverageBlock {
+    pub(crate) struct BasicCoverageBlock {
         const START_BCB = 0;
     }
 }
@@ -259,23 +263,23 @@ rustc_index::newtype_index! {
 /// queries (`dominates()`, `predecessors`, `successors`, etc.) have branch (control flow)
 /// significance.
 #[derive(Debug, Clone)]
-pub(super) struct BasicCoverageBlockData {
-    pub basic_blocks: Vec<BasicBlock>,
+pub(crate) struct BasicCoverageBlockData {
+    pub(crate) basic_blocks: Vec<BasicBlock>,
 }
 
 impl BasicCoverageBlockData {
-    pub fn from(basic_blocks: Vec<BasicBlock>) -> Self {
+    fn from(basic_blocks: Vec<BasicBlock>) -> Self {
         assert!(basic_blocks.len() > 0);
         Self { basic_blocks }
     }
 
     #[inline(always)]
-    pub fn leader_bb(&self) -> BasicBlock {
+    pub(crate) fn leader_bb(&self) -> BasicBlock {
         self.basic_blocks[0]
     }
 
     #[inline(always)]
-    pub fn last_bb(&self) -> BasicBlock {
+    pub(crate) fn last_bb(&self) -> BasicBlock {
         *self.basic_blocks.last().unwrap()
     }
 }
@@ -364,7 +368,7 @@ fn bcb_filtered_successors<'a, 'tcx>(terminator: &'a Terminator<'tcx>) -> Covera
 /// CoverageGraph outside all loops. This supports traversing the BCB CFG in a way that
 /// ensures a loop is completely traversed before processing Blocks after the end of the loop.
 #[derive(Debug)]
-pub(super) struct TraversalContext {
+struct TraversalContext {
     /// BCB with one or more incoming loop backedges, indicating which loop
     /// this context is for.
     ///
@@ -375,7 +379,7 @@ pub(super) struct TraversalContext {
     worklist: VecDeque<BasicCoverageBlock>,
 }
 
-pub(super) struct TraverseCoverageGraphWithLoops<'a> {
+pub(crate) struct TraverseCoverageGraphWithLoops<'a> {
     basic_coverage_blocks: &'a CoverageGraph,
 
     backedges: IndexVec<BasicCoverageBlock, Vec<BasicCoverageBlock>>,
@@ -384,7 +388,7 @@ pub(super) struct TraverseCoverageGraphWithLoops<'a> {
 }
 
 impl<'a> TraverseCoverageGraphWithLoops<'a> {
-    pub(super) fn new(basic_coverage_blocks: &'a CoverageGraph) -> Self {
+    pub(crate) fn new(basic_coverage_blocks: &'a CoverageGraph) -> Self {
         let backedges = find_loop_backedges(basic_coverage_blocks);
 
         let worklist = VecDeque::from([basic_coverage_blocks.start_node()]);
@@ -400,47 +404,46 @@ impl<'a> TraverseCoverageGraphWithLoops<'a> {
 
     /// For each loop on the loop context stack (top-down), yields a list of BCBs
     /// within that loop that have an outgoing edge back to the loop header.
-    pub(super) fn reloop_bcbs_per_loop(&self) -> impl Iterator<Item = &[BasicCoverageBlock]> {
+    pub(crate) fn reloop_bcbs_per_loop(&self) -> impl Iterator<Item = &[BasicCoverageBlock]> {
         self.context_stack
             .iter()
             .rev()
             .filter_map(|context| context.loop_header)
             .map(|header_bcb| self.backedges[header_bcb].as_slice())
     }
 
-    pub(super) fn next(&mut self) -> Option<BasicCoverageBlock> {
+    pub(crate) fn next(&mut self) -> Option<BasicCoverageBlock> {
         debug!(
             "TraverseCoverageGraphWithLoops::next - context_stack: {:?}",
             self.context_stack.iter().rev().collect::<Vec<_>>()
         );
 
         while let Some(context) = self.context_stack.last_mut() {
-            if let Some(bcb) = context.worklist.pop_front() {
-                if !self.visited.insert(bcb) {
-                    debug!("Already visited: {bcb:?}");
-                    continue;
-                }
-                debug!("Visiting {bcb:?}");
-
-                if self.backedges[bcb].len() > 0 {
-                    debug!("{bcb:?} is a loop header! Start a new TraversalContext...");
-                    self.context_stack.push(TraversalContext {
-                        loop_header: Some(bcb),
-                        worklist: VecDeque::new(),
-                    });
-                }
-                self.add_successors_to_worklists(bcb);
-                return Some(bcb);
-            } else {
-                // Strip contexts with empty worklists from the top of the stack
+            let Some(bcb) = context.worklist.pop_front() else {
+                // This stack level is exhausted; pop it and try the next one.
                 self.context_stack.pop();
+                continue;
+            };
+
+            if !self.visited.insert(bcb) {
+                debug!("Already visited: {bcb:?}");
+                continue;
+            }
+            debug!("Visiting {bcb:?}");
+
+            if self.backedges[bcb].len() > 0 {
+                debug!("{bcb:?} is a loop header! Start a new TraversalContext...");
+                self.context_stack
+                    .push(TraversalContext { loop_header: Some(bcb), worklist: VecDeque::new() });
             }
+            self.add_successors_to_worklists(bcb);
+            return Some(bcb);
         }
 
         None
     }
 
-    pub fn add_successors_to_worklists(&mut self, bcb: BasicCoverageBlock) {
+    fn add_successors_to_worklists(&mut self, bcb: BasicCoverageBlock) {
         let successors = &self.basic_coverage_blocks.successors[bcb];
         debug!("{:?} has {} successors:", bcb, successors.len());
 
@@ -494,19 +497,19 @@ impl<'a> TraverseCoverageGraphWithLoops<'a> {
         }
     }
 
-    pub fn is_complete(&self) -> bool {
+    pub(crate) fn is_complete(&self) -> bool {
         self.visited.count() == self.visited.domain_size()
     }
 
-    pub fn unvisited(&self) -> Vec<BasicCoverageBlock> {
+    pub(crate) fn unvisited(&self) -> Vec<BasicCoverageBlock> {
         let mut unvisited_set: BitSet<BasicCoverageBlock> =
             BitSet::new_filled(self.visited.domain_size());
         unvisited_set.subtract(&self.visited);
         unvisited_set.iter().collect::<Vec<_>>()
     }
 }
 
-pub(super) fn find_loop_backedges(
+fn find_loop_backedges(
     basic_coverage_blocks: &CoverageGraph,
 ) -> IndexVec<BasicCoverageBlock, Vec<BasicCoverageBlock>> {
     let num_bcbs = basic_coverage_blocks.num_nodes();
diff --git a/compiler/rustc_mir_transform/src/coverage/tests.rs b/compiler/rustc_mir_transform/src/coverage/tests.rs
@@ -24,7 +24,6 @@
 //! globals is comparatively simpler. The easiest way is to wrap the test in a closure argument
 //! to: `rustc_span::create_default_session_globals_then(|| { test_here(); })`.
 
-use super::counters;
 use super::graph::{self, BasicCoverageBlock};
 
 use itertools::Itertools;
@@ -551,108 +550,3 @@ fn test_covgraph_switchint_loop_then_inner_loop_else_break() {
     assert_successors(&basic_coverage_blocks, bcb(5), &[bcb(1)]);
     assert_successors(&basic_coverage_blocks, bcb(6), &[bcb(4)]);
 }
-
-#[test]
-fn test_find_loop_backedges_none() {
-    let mir_body = goto_switchint();
-    let basic_coverage_blocks = graph::CoverageGraph::from_mir(&mir_body);
-    if false {
-        eprintln!(
-            "basic_coverage_blocks = {:?}",
-            basic_coverage_blocks.iter_enumerated().collect::<Vec<_>>()
-        );
-        eprintln!("successors = {:?}", basic_coverage_blocks.successors);
-    }
-    let backedges = graph::find_loop_backedges(&basic_coverage_blocks);
-    assert_eq!(
-        backedges.iter_enumerated().map(|(_bcb, backedges)| backedges.len()).sum::<usize>(),
-        0,
-        "backedges: {:?}",
-        backedges
-    );
-}
-
-#[test]
-fn test_find_loop_backedges_one() {
-    let mir_body = switchint_then_loop_else_return();
-    let basic_coverage_blocks = graph::CoverageGraph::from_mir(&mir_body);
-    let backedges = graph::find_loop_backedges(&basic_coverage_blocks);
-    assert_eq!(
-        backedges.iter_enumerated().map(|(_bcb, backedges)| backedges.len()).sum::<usize>(),
-        1,
-        "backedges: {:?}",
-        backedges
-    );
-
-    assert_eq!(backedges[bcb(1)], &[bcb(3)]);
-}
-
-#[test]
-fn test_find_loop_backedges_two() {
-    let mir_body = switchint_loop_then_inner_loop_else_break();
-    let basic_coverage_blocks = graph::CoverageGraph::from_mir(&mir_body);
-    let backedges = graph::find_loop_backedges(&basic_coverage_blocks);
-    assert_eq!(
-        backedges.iter_enumerated().map(|(_bcb, backedges)| backedges.len()).sum::<usize>(),
-        2,
-        "backedges: {:?}",
-        backedges
-    );
-
-    assert_eq!(backedges[bcb(1)], &[bcb(5)]);
-    assert_eq!(backedges[bcb(4)], &[bcb(6)]);
-}
-
-#[test]
-fn test_traverse_coverage_with_loops() {
-    let mir_body = switchint_loop_then_inner_loop_else_break();
-    let basic_coverage_blocks = graph::CoverageGraph::from_mir(&mir_body);
-    let mut traversed_in_order = Vec::new();
-    let mut traversal = graph::TraverseCoverageGraphWithLoops::new(&basic_coverage_blocks);
-    while let Some(bcb) = traversal.next() {
-        traversed_in_order.push(bcb);
-    }
-
-    // bcb0 is visited first. Then bcb1 starts the first loop, and all remaining nodes, *except*
-    // bcb6 are inside the first loop.
-    assert_eq!(
-        *traversed_in_order.last().expect("should have elements"),
-        bcb(6),
-        "bcb6 should not be visited until all nodes inside the first loop have been visited"
-    );
-}
-
-#[test]
-fn test_make_bcb_counters() {
-    rustc_span::create_default_session_globals_then(|| {
-        let mir_body = goto_switchint();
-        let basic_coverage_blocks = graph::CoverageGraph::from_mir(&mir_body);
-        // Historically this test would use `spans` internals to set up fake
-        // coverage spans for BCBs 1 and 2. Now we skip that step and just tell
-        // BCB counter construction that those BCBs have spans.
-        let bcb_has_coverage_spans = |bcb: BasicCoverageBlock| (1..=2).contains(&bcb.as_usize());
-        let coverage_counters = counters::CoverageCounters::make_bcb_counters(
-            &basic_coverage_blocks,
-            bcb_has_coverage_spans,
-        );
-        assert_eq!(coverage_counters.num_expressions(), 0);
-
-        assert_eq!(
-            0, // bcb1 has a `Counter` with id = 0
-            match coverage_counters.bcb_counter(bcb(1)).expect("should have a counter") {
-                counters::BcbCounter::Counter { id, .. } => id,
-                _ => panic!("expected a Counter"),
-            }
-            .as_u32()
-        );
-
-        assert_eq!(
-            1, // bcb2 has a `Counter` with id = 1
-            match coverage_counters.bcb_counter(bcb(2)).expect("should have a counter") {
-                counters::BcbCounter::Counter { id, .. } => id,
-                _ => panic!("expected a Counter"),
-            }
-            .as_u32()
-        );
-    });
-}
