coverage: Remove the final span-merge pass, and rename is_closure to is_hole

compiler/rustc_mir_transform/src/coverage/spans.rs

@@ -90,24 +90,17 @@ pub(super) fn generate_coverage_spans(
 struct CurrCovspan {
     span: Span,
     bcb: BasicCoverageBlock,
-    is_closure: bool,
+    is_hole: bool,
 }
 
 impl CurrCovspan {
-    fn new(span: Span, bcb: BasicCoverageBlock, is_closure: bool) -> Self {
-        Self { span, bcb, is_closure }
+    fn new(span: Span, bcb: BasicCoverageBlock, is_hole: bool) -> Self {
+        Self { span, bcb, is_hole }
     }
 
     fn into_prev(self) -> PrevCovspan {
-        let Self { span, bcb, is_closure } = self;
-        PrevCovspan { span, bcb, merged_spans: vec![span], is_closure }
-    }
-
-    fn into_refined(self) -> RefinedCovspan {
-        // This is only called in cases where `curr` is a closure span that has
-        // been carved out of `prev`.
-        debug_assert!(self.is_closure);
-        self.into_prev().into_refined()
+        let Self { span, bcb, is_hole } = self;
+        PrevCovspan { span, bcb, merged_spans: vec![span], is_hole }
     }
 }
 
@@ -118,12 +111,12 @@ struct PrevCovspan {
     /// List of all the original spans from MIR that have been merged into this
     /// span. Mainly used to precisely skip over gaps when truncating a span.
     merged_spans: Vec<Span>,
-    is_closure: bool,
+    is_hole: bool,
 }
 
 impl PrevCovspan {
     fn is_mergeable(&self, other: &CurrCovspan) -> bool {
-        self.bcb == other.bcb && !self.is_closure && !other.is_closure
+        self.bcb == other.bcb && !self.is_hole && !other.is_hole
     }
 
     fn merge_from(&mut self, other: &CurrCovspan) {
@@ -138,15 +131,15 @@ impl PrevCovspan {
             self.span = self.span.with_hi(max_hi);
         }
 
-        if self.merged_spans.is_empty() { None } else { Some(self.into_refined()) }
+        if self.merged_spans.is_empty() { None } else { self.into_refined() }
     }
 
-    fn refined_copy(&self) -> RefinedCovspan {
-        let &Self { span, bcb, merged_spans: _, is_closure } = self;
-        RefinedCovspan { span, bcb, is_closure }
+    fn refined_copy(&self) -> Option<RefinedCovspan> {
+        let &Self { span, bcb, merged_spans: _, is_hole } = self;
+        (!is_hole).then_some(RefinedCovspan { span, bcb })
     }
 
-    fn into_refined(self) -> RefinedCovspan {
+    fn into_refined(self) -> Option<RefinedCovspan> {
         // Even though we consume self, we can just reuse the copying impl.
         self.refined_copy()
     }
@@ -156,18 +149,6 @@ impl PrevCovspan {
 struct RefinedCovspan {
     span: Span,
     bcb: BasicCoverageBlock,
-    is_closure: bool,
-}
-
-impl RefinedCovspan {
-    fn is_mergeable(&self, other: &Self) -> bool {
-        self.bcb == other.bcb && !self.is_closure && !other.is_closure
-    }
-
-    fn merge_from(&mut self, other: &Self) {
-        debug_assert!(self.is_mergeable(other));
-        self.span = self.span.to(other.span);
-    }
 }
 
 /// Converts the initial set of coverage spans (one per MIR `Statement` or `Terminator`) into a
@@ -182,10 +163,9 @@ struct SpansRefiner {
     /// dominance between the `BasicCoverageBlock`s of equal `Span`s.
     sorted_spans_iter: std::vec::IntoIter<SpanFromMir>,
 
-    /// The current coverage span to compare to its `prev`, to possibly merge, discard, force the
-    /// discard of the `prev` (and or `pending_dups`), or keep both (with `prev` moved to
-    /// `pending_dups`). If `curr` is not discarded or merged, it becomes `prev` for the next
-    /// iteration.
+    /// The current coverage span to compare to its `prev`, to possibly merge, discard,
+    /// or cause `prev` to be modified or discarded.
+    /// If `curr` is not discarded or merged, it becomes `prev` for the next iteration.
     some_curr: Option<CurrCovspan>,
 
     /// The coverage span from a prior iteration; typically assigned from that iteration's `curr`.
@@ -229,24 +209,22 @@ impl SpansRefiner {
             let curr = self.curr();
 
             if prev.is_mergeable(curr) {
-                debug!("  same bcb (and neither is a closure), merge with prev={prev:?}");
+                debug!(?prev, "curr will be merged into prev");
                 let curr = self.take_curr();
                 self.prev_mut().merge_from(&curr);
             } else if prev.span.hi() <= curr.span.lo() {
                 debug!(
                     "  different bcbs and disjoint spans, so keep curr for next iter, and add prev={prev:?}",
                 );
                 let prev = self.take_prev().into_refined();
-                self.refined_spans.push(prev);
-            } else if prev.is_closure {
+                self.refined_spans.extend(prev);
+            } else if prev.is_hole {
                 // drop any equal or overlapping span (`curr`) and keep `prev` to test again in the
                 // next iter
-                debug!(
-                    "  curr overlaps a closure (prev). Drop curr and keep prev for next iter. prev={prev:?}",
-                );
+                debug!(?prev, "prev (a hole) overlaps curr, so discarding curr");
                 self.take_curr(); // Discards curr.
-            } else if curr.is_closure {
-                self.carve_out_span_for_closure();
+            } else if curr.is_hole {
+                self.carve_out_span_for_hole();
             } else {
                 self.cutoff_prev_at_overlapping_curr();
             }
@@ -256,24 +234,9 @@ impl SpansRefiner {
         // so add it to the output as well.
         if let Some(prev) = self.some_prev.take() {
             debug!("    AT END, adding last prev={prev:?}");
-            self.refined_spans.push(prev.into_refined());
+            self.refined_spans.extend(prev.into_refined());
         }
 
-        // Do one last merge pass, to simplify the output.
-        self.refined_spans.dedup_by(|b, a| {
-            if a.is_mergeable(b) {
-                debug!(?a, ?b, "merging list-adjacent refined spans");
-                a.merge_from(b);
-                true
-            } else {
-                false
-            }
-        });
-
-        // Remove spans derived from closures, originally added to ensure the coverage
-        // regions for the current function leave room for the closure's own coverage regions
-        // (injected separately, from the closure's own MIR).
-        self.refined_spans.retain(|covspan| !covspan.is_closure);
         self.refined_spans
     }
 
@@ -316,48 +279,44 @@ impl SpansRefiner {
             {
                 // Skip curr because prev has already advanced beyond the end of curr.
                 // This can only happen if a prior iteration updated `prev` to skip past
-                // a region of code, such as skipping past a closure.
-                debug!(
-                    "  prev.span starts after curr.span, so curr will be dropped (skipping past \
-                    closure?); prev={prev:?}",
-                );
+                // a region of code, such as skipping past a hole.
+                debug!(?prev, "prev.span starts after curr.span, so curr will be dropped");
             } else {
-                self.some_curr = Some(CurrCovspan::new(curr.span, curr.bcb, curr.is_closure));
+                self.some_curr = Some(CurrCovspan::new(curr.span, curr.bcb, curr.is_hole));
                 return true;
             }
         }
         false
     }
 
-    /// If `prev`s span extends left of the closure (`curr`), carve out the closure's span from
-    /// `prev`'s span. (The closure's coverage counters will be injected when processing the
-    /// closure's own MIR.) Add the portion of the span to the left of the closure; and if the span
-    /// extends to the right of the closure, update `prev` to that portion of the span. For any
-    /// `pending_dups`, repeat the same process.
-    fn carve_out_span_for_closure(&mut self) {
+    /// If `prev`s span extends left of the hole (`curr`), carve out the hole's span from
+    /// `prev`'s span. Add the portion of the span to the left of the hole; and if the span
+    /// extends to the right of the hole, update `prev` to that portion of the span.
+    fn carve_out_span_for_hole(&mut self) {
         let prev = self.prev();
         let curr = self.curr();
+        assert!(!prev.is_hole && curr.is_hole);
 
         let left_cutoff = curr.span.lo();
         let right_cutoff = curr.span.hi();
-        let has_pre_closure_span = prev.span.lo() < right_cutoff;
-        let has_post_closure_span = prev.span.hi() > right_cutoff;
-
-        if has_pre_closure_span {
-            let mut pre_closure = self.prev().refined_copy();
-            pre_closure.span = pre_closure.span.with_hi(left_cutoff);
-            debug!("  prev overlaps a closure. Adding span for pre_closure={:?}", pre_closure);
-            self.refined_spans.push(pre_closure);
+        let has_pre_hole_span = prev.span.lo() < right_cutoff;
+        let has_post_hole_span = prev.span.hi() > right_cutoff;
+
+        if has_pre_hole_span {
+            let mut pre_hole = prev.refined_copy().expect("prev is not a hole span");
+            pre_hole.span = pre_hole.span.with_hi(left_cutoff);
+            debug!(?pre_hole, "prev overlaps a hole; adding pre-hole span");
+            self.refined_spans.push(pre_hole);
         }
 
-        if has_post_closure_span {
-            // Mutate `prev.span` to start after the closure (and discard curr).
+        if has_post_hole_span {
+            // Mutate `prev.span` to start after the hole (and discard curr).
             self.prev_mut().span = self.prev().span.with_lo(right_cutoff);
-            debug!("  Mutated prev.span to start after the closure. prev={:?}", self.prev());
+            debug!(prev=?self.prev(), "mutated prev to start after the hole");
 
-            // Prevent this curr from becoming prev.
-            let closure_covspan = self.take_curr().into_refined();
-            self.refined_spans.push(closure_covspan); // since self.prev() was already updated
+            // Discard this curr, since it's a hole span.
+            let curr = self.take_curr();
+            assert!(curr.is_hole);
         }
     }
 

compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs

@@ -52,14 +52,14 @@ pub(super) fn mir_to_initial_sorted_coverage_spans(
             // - Span A extends further left, or
             // - Both have the same start and span A extends further right
             .then_with(|| Ord::cmp(&a.span.hi(), &b.span.hi()).reverse())
-            // If two spans have the same lo & hi, put closure spans first,
-            // as they take precedence over non-closure spans.
-            .then_with(|| Ord::cmp(&a.is_closure, &b.is_closure).reverse())
+            // If two spans have the same lo & hi, put hole spans first,
+            // as they take precedence over non-hole spans.
+            .then_with(|| Ord::cmp(&a.is_hole, &b.is_hole).reverse())
             // After deduplication, we want to keep only the most-dominated BCB.
             .then_with(|| basic_coverage_blocks.cmp_in_dominator_order(a.bcb, b.bcb).reverse())
     });
 
-    // Among covspans with the same span, keep only one. Closure spans take
+    // Among covspans with the same span, keep only one. Hole spans take
     // precedence, otherwise keep the one with the most-dominated BCB.
     // (Ideally we should try to preserve _all_ non-dominating BCBs, but that
     // requires a lot more complexity in the span refiner, for little benefit.)
@@ -78,8 +78,8 @@ pub(super) fn mir_to_initial_sorted_coverage_spans(
 fn remove_unwanted_macro_spans(initial_spans: &mut Vec<SpanFromMir>) {
     let mut seen_macro_spans = FxHashSet::default();
     initial_spans.retain(|covspan| {
-        // Ignore (retain) closure spans and non-macro-expansion spans.
-        if covspan.is_closure || covspan.visible_macro.is_none() {
+        // Ignore (retain) hole spans and non-macro-expansion spans.
+        if covspan.is_hole || covspan.visible_macro.is_none() {
             return true;
         }
 
@@ -96,7 +96,7 @@ fn split_visible_macro_spans(initial_spans: &mut Vec<SpanFromMir>) {
     let mut extra_spans = vec![];
 
     initial_spans.retain(|covspan| {
-        if covspan.is_closure {
+        if covspan.is_hole {
             return true;
         }
 
@@ -112,7 +112,7 @@ fn split_visible_macro_spans(initial_spans: &mut Vec<SpanFromMir>) {
             return true;
         }
 
-        assert!(!covspan.is_closure);
+        assert!(!covspan.is_hole);
         extra_spans.push(SpanFromMir::new(before, covspan.visible_macro, covspan.bcb, false));
         extra_spans.push(SpanFromMir::new(after, covspan.visible_macro, covspan.bcb, false));
         false // Discard the original covspan that we just split.
@@ -336,7 +336,10 @@ pub(super) struct SpanFromMir {
     pub(super) span: Span,
     visible_macro: Option<Symbol>,
     pub(super) bcb: BasicCoverageBlock,
-    pub(super) is_closure: bool,
+    /// If true, this covspan represents a "hole" that should be carved out
+    /// from other spans, e.g. because it represents a closure expression that
+    /// will be instrumented separately as its own function.
+    pub(super) is_hole: bool,
 }
 
 impl SpanFromMir {
@@ -348,8 +351,8 @@ impl SpanFromMir {
         span: Span,
         visible_macro: Option<Symbol>,
         bcb: BasicCoverageBlock,
-        is_closure: bool,
+        is_hole: bool,
     ) -> Self {
-        Self { span, visible_macro, bcb, is_closure }
+        Self { span, visible_macro, bcb, is_hole }
     }
 }

tests/coverage/fn_sig_into_try.cov-map

@@ -7,45 +7,48 @@ Number of file 0 mappings: 1
 - Code(Counter(0)) at (prev + 10, 1) to (start + 5, 2)
 
 Function name: fn_sig_into_try::b
-Raw bytes (28): 0x[01, 01, 02, 01, 00, 00, 02, 04, 01, 11, 01, 03, 0f, 00, 03, 0f, 00, 10, 02, 01, 05, 00, 0c, 07, 01, 01, 00, 02]
+Raw bytes (33): 0x[01, 01, 02, 01, 00, 00, 02, 05, 01, 11, 01, 02, 01, 01, 03, 05, 00, 0f, 00, 00, 0f, 00, 10, 02, 01, 05, 00, 0c, 07, 01, 01, 00, 02]
 Number of files: 1
 - file 0 => global file 1
 Number of expressions: 2
 - expression 0 operands: lhs = Counter(0), rhs = Zero
 - expression 1 operands: lhs = Zero, rhs = Expression(0, Sub)
-Number of file 0 mappings: 4
-- Code(Counter(0)) at (prev + 17, 1) to (start + 3, 15)
-- Code(Zero) at (prev + 3, 15) to (start + 0, 16)
+Number of file 0 mappings: 5
+- Code(Counter(0)) at (prev + 17, 1) to (start + 2, 1)
+- Code(Counter(0)) at (prev + 3, 5) to (start + 0, 15)
+- Code(Zero) at (prev + 0, 15) to (start + 0, 16)
 - Code(Expression(0, Sub)) at (prev + 1, 5) to (start + 0, 12)
     = (c0 - Zero)
 - Code(Expression(1, Add)) at (prev + 1, 1) to (start + 0, 2)
     = (Zero + (c0 - Zero))
 
 Function name: fn_sig_into_try::c
-Raw bytes (28): 0x[01, 01, 02, 01, 00, 00, 02, 04, 01, 18, 01, 03, 17, 00, 03, 17, 00, 18, 02, 01, 05, 00, 0c, 07, 01, 01, 00, 02]
+Raw bytes (33): 0x[01, 01, 02, 01, 00, 00, 02, 05, 01, 18, 01, 02, 01, 01, 03, 0d, 00, 17, 00, 00, 17, 00, 18, 02, 01, 05, 00, 0c, 07, 01, 01, 00, 02]
 Number of files: 1
 - file 0 => global file 1
 Number of expressions: 2
 - expression 0 operands: lhs = Counter(0), rhs = Zero
 - expression 1 operands: lhs = Zero, rhs = Expression(0, Sub)
-Number of file 0 mappings: 4
-- Code(Counter(0)) at (prev + 24, 1) to (start + 3, 23)
-- Code(Zero) at (prev + 3, 23) to (start + 0, 24)
+Number of file 0 mappings: 5
+- Code(Counter(0)) at (prev + 24, 1) to (start + 2, 1)
+- Code(Counter(0)) at (prev + 3, 13) to (start + 0, 23)
+- Code(Zero) at (prev + 0, 23) to (start + 0, 24)
 - Code(Expression(0, Sub)) at (prev + 1, 5) to (start + 0, 12)
     = (c0 - Zero)
 - Code(Expression(1, Add)) at (prev + 1, 1) to (start + 0, 2)
     = (Zero + (c0 - Zero))
 
 Function name: fn_sig_into_try::d
-Raw bytes (28): 0x[01, 01, 02, 01, 00, 00, 02, 04, 01, 1f, 01, 04, 0f, 00, 04, 0f, 00, 10, 02, 01, 05, 00, 0c, 07, 01, 01, 00, 02]
+Raw bytes (33): 0x[01, 01, 02, 01, 00, 00, 02, 05, 01, 1f, 01, 03, 13, 01, 04, 05, 00, 0f, 00, 00, 0f, 00, 10, 02, 01, 05, 00, 0c, 07, 01, 01, 00, 02]
 Number of files: 1
 - file 0 => global file 1
 Number of expressions: 2
 - expression 0 operands: lhs = Counter(0), rhs = Zero
 - expression 1 operands: lhs = Zero, rhs = Expression(0, Sub)
-Number of file 0 mappings: 4
-- Code(Counter(0)) at (prev + 31, 1) to (start + 4, 15)
-- Code(Zero) at (prev + 4, 15) to (start + 0, 16)
+Number of file 0 mappings: 5
+- Code(Counter(0)) at (prev + 31, 1) to (start + 3, 19)
+- Code(Counter(0)) at (prev + 4, 5) to (start + 0, 15)
+- Code(Zero) at (prev + 0, 15) to (start + 0, 16)
 - Code(Expression(0, Sub)) at (prev + 1, 5) to (start + 0, 12)
     = (c0 - Zero)
 - Code(Expression(1, Add)) at (prev + 1, 1) to (start + 0, 2)

tests/coverage/issue-84561.cov-map

@@ -1,13 +1,14 @@
 Function name: <issue_84561::Foo as core::fmt::Debug>::fmt
-Raw bytes (29): 0x[01, 01, 02, 01, 05, 05, 02, 04, 01, 8a, 01, 05, 01, 25, 05, 01, 25, 00, 26, 02, 01, 09, 00, 0f, 07, 01, 05, 00, 06]
+Raw bytes (34): 0x[01, 01, 02, 01, 05, 05, 02, 05, 01, 8a, 01, 05, 00, 44, 01, 01, 09, 00, 25, 05, 00, 25, 00, 26, 02, 01, 09, 00, 0f, 07, 01, 05, 00, 06]
 Number of files: 1
 - file 0 => global file 1
 Number of expressions: 2
 - expression 0 operands: lhs = Counter(0), rhs = Counter(1)
 - expression 1 operands: lhs = Counter(1), rhs = Expression(0, Sub)
-Number of file 0 mappings: 4
-- Code(Counter(0)) at (prev + 138, 5) to (start + 1, 37)
-- Code(Counter(1)) at (prev + 1, 37) to (start + 0, 38)
+Number of file 0 mappings: 5
+- Code(Counter(0)) at (prev + 138, 5) to (start + 0, 68)
+- Code(Counter(0)) at (prev + 1, 9) to (start + 0, 37)
+- Code(Counter(1)) at (prev + 0, 37) to (start + 0, 38)
 - Code(Expression(0, Sub)) at (prev + 1, 9) to (start + 0, 15)
     = (c0 - c1)
 - Code(Expression(1, Add)) at (prev + 1, 5) to (start + 0, 6)