coverage: Hoist the splitting of visible macro invocations

Author: Zalathar

compiler/rustc_mir_transform/src/coverage/spans.rs

@@ -215,7 +215,6 @@ impl<'a> CoverageSpansGenerator<'a> {
             // span-processing steps don't make sense yet.
             if self.some_prev.is_none() {
                 debug!("  initial span");
-                self.maybe_push_macro_name_span();
                 continue;
             }
 
@@ -227,15 +226,13 @@ impl<'a> CoverageSpansGenerator<'a> {
                 debug!("  same bcb (and neither is a closure), merge with prev={prev:?}");
                 let prev = self.take_prev();
                 self.curr_mut().merge_from(&prev);
-                self.maybe_push_macro_name_span();
             // Note that curr.span may now differ from curr_original_span
             } 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();
                 self.refined_spans.push(prev);
-                self.maybe_push_macro_name_span();
             } else if prev.is_closure {
                 // drop any equal or overlapping span (`curr`) and keep `prev` to test again in the
                 // next iter
@@ -251,7 +248,6 @@ impl<'a> CoverageSpansGenerator<'a> {
                 self.update_pending_dups();
             } else {
                 self.cutoff_prev_at_overlapping_curr();
-                self.maybe_push_macro_name_span();
             }
         }
 
@@ -286,36 +282,6 @@ impl<'a> CoverageSpansGenerator<'a> {
         self.refined_spans
     }
 
-    /// If `curr` is part of a new macro expansion, carve out and push a separate
-    /// span that ends just after the macro name and its subsequent `!`.
-    fn maybe_push_macro_name_span(&mut self) {
-        let curr = self.curr();
-
-        let Some(visible_macro) = curr.visible_macro else { return };
-
-        // The split point is relative to `curr_original_span`,
-        // because `curr.span` may have been merged with preceding spans.
-        let split_point_after_macro_bang = self.curr_original_span.lo()
-            + BytePos(visible_macro.as_str().len() as u32)
-            + BytePos(1); // add 1 for the `!`
-        debug_assert!(split_point_after_macro_bang <= curr.span.hi());
-        if split_point_after_macro_bang > curr.span.hi() {
-            // Something is wrong with the macro name span;
-            // return now to avoid emitting malformed mappings (e.g. #117788).
-            return;
-        }
-
-        let mut macro_name_cov = curr.clone();
-        macro_name_cov.span = macro_name_cov.span.with_hi(split_point_after_macro_bang);
-        self.curr_mut().span = curr.span.with_lo(split_point_after_macro_bang);
-
-        debug!(
-            "  and curr starts a new macro expansion, so add a new span just for \
-            the macro `{visible_macro}!`, new span={macro_name_cov:?}",
-        );
-        self.refined_spans.push(macro_name_cov);
-    }
-
     #[track_caller]
     fn curr(&self) -> &CoverageSpan {
         self.some_curr.as_ref().unwrap_or_else(|| bug!("some_curr is None (curr)"))

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

@@ -38,6 +38,7 @@ pub(super) fn mir_to_initial_sorted_coverage_spans(
 
     initial_spans.sort_by(|a, b| basic_coverage_blocks.cmp_in_dominator_order(a.bcb, b.bcb));
     remove_unwanted_macro_spans(&mut initial_spans);
+    split_visible_macro_spans(&mut initial_spans);
 
     initial_spans.sort_by(|a, b| {
         // First sort by span start.
@@ -79,6 +80,41 @@ fn remove_unwanted_macro_spans(initial_spans: &mut Vec<CoverageSpan>) {
     });
 }
 
+/// When a span corresponds to a macro invocation that is visible from the
+/// function body, split it into two parts. The first part covers just the
+/// macro name plus `!`, and the second part covers the rest of the macro
+/// invocation. This seems to give better results for code that uses macros.
+fn split_visible_macro_spans(initial_spans: &mut Vec<CoverageSpan>) {
+    let mut extra_spans = vec![];
+
+    initial_spans.retain(|covspan| {
+        if covspan.is_closure {
+            return true;
+        }
+
+        let Some(visible_macro) = covspan.visible_macro else { return true };
+
+        let split_len = visible_macro.as_str().len() as u32 + 1;
+        let (before, after) = covspan.span.split_at(split_len);
+        if !covspan.span.contains(before) || !covspan.span.contains(after) {
+            // Something is unexpectedly wrong with the split point.
+            // The debug assertion in `split_at` will have already caught this,
+            // but in release builds it's safer to do nothing and maybe get a
+            // bug report for unexpected coverage, rather than risk an ICE.
+            return true;
+        }
+
+        assert!(!covspan.is_closure);
+        extra_spans.push(CoverageSpan::new(before, covspan.visible_macro, covspan.bcb, false));
+        extra_spans.push(CoverageSpan::new(after, covspan.visible_macro, covspan.bcb, false));
+        false // Discard the original covspan that we just split.
+    });
+
+    // The newly-split spans are added at the end, so any previous sorting
+    // is not preserved.
+    initial_spans.extend(extra_spans);
+}
+
 // Generate a set of `CoverageSpan`s from the filtered set of `Statement`s and `Terminator`s of
 // the `BasicBlock`(s) in the given `BasicCoverageBlockData`. One `CoverageSpan` is generated
 // for each `Statement` and `Terminator`. (Note that subsequent stages of coverage analysis will

tests/coverage/closure.cov-map

@@ -81,21 +81,18 @@ Number of file 0 mappings: 1
 - Code(Zero) at (prev + 171, 13) to (start + 2, 14)
 
 Function name: closure::main::{closure#14}
-Raw bytes (36): 0x[01, 01, 03, 05, 0a, 01, 05, 01, 05, 05, 03, b2, 01, 0d, 00, 15, 01, 01, 11, 01, 1b, 05, 01, 1e, 00, 25, 0a, 00, 2f, 00, 33, 03, 01, 0d, 00, 0e]
+Raw bytes (29): 0x[01, 01, 02, 01, 05, 05, 02, 04, 01, b2, 01, 0d, 02, 1b, 05, 02, 1e, 00, 25, 02, 00, 2f, 00, 33, 07, 01, 0d, 00, 0e]
 Number of files: 1
 - file 0 => global file 1
-Number of expressions: 3
-- expression 0 operands: lhs = Counter(1), rhs = Expression(2, Sub)
-- expression 1 operands: lhs = Counter(0), rhs = Counter(1)
-- expression 2 operands: lhs = Counter(0), rhs = Counter(1)
-Number of file 0 mappings: 5
-- Code(Expression(0, Add)) at (prev + 178, 13) to (start + 0, 21)
-    = (c1 + (c0 - c1))
-- Code(Counter(0)) at (prev + 1, 17) to (start + 1, 27)
-- Code(Counter(1)) at (prev + 1, 30) to (start + 0, 37)
-- Code(Expression(2, Sub)) at (prev + 0, 47) to (start + 0, 51)
+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 + 178, 13) to (start + 2, 27)
+- Code(Counter(1)) at (prev + 2, 30) to (start + 0, 37)
+- Code(Expression(0, Sub)) at (prev + 0, 47) to (start + 0, 51)
     = (c0 - c1)
-- Code(Expression(0, Add)) at (prev + 1, 13) to (start + 0, 14)
+- Code(Expression(1, Add)) at (prev + 1, 13) to (start + 0, 14)
     = (c1 + (c0 - c1))
 
 Function name: closure::main::{closure#15}
@@ -118,21 +115,18 @@ Number of file 0 mappings: 6
     = (c1 + (c0 - c1))
 
 Function name: closure::main::{closure#16}
-Raw bytes (36): 0x[01, 01, 03, 05, 0a, 01, 05, 01, 05, 05, 03, c4, 01, 0d, 00, 15, 01, 01, 11, 01, 1b, 05, 01, 1e, 00, 25, 0a, 00, 2f, 00, 33, 03, 01, 0d, 00, 0e]
+Raw bytes (29): 0x[01, 01, 02, 01, 05, 05, 02, 04, 01, c4, 01, 0d, 02, 1b, 05, 02, 1e, 00, 25, 02, 00, 2f, 00, 33, 07, 01, 0d, 00, 0e]
 Number of files: 1
 - file 0 => global file 1
-Number of expressions: 3
-- expression 0 operands: lhs = Counter(1), rhs = Expression(2, Sub)
-- expression 1 operands: lhs = Counter(0), rhs = Counter(1)
-- expression 2 operands: lhs = Counter(0), rhs = Counter(1)
-Number of file 0 mappings: 5
-- Code(Expression(0, Add)) at (prev + 196, 13) to (start + 0, 21)
-    = (c1 + (c0 - c1))
-- Code(Counter(0)) at (prev + 1, 17) to (start + 1, 27)
-- Code(Counter(1)) at (prev + 1, 30) to (start + 0, 37)
-- Code(Expression(2, Sub)) at (prev + 0, 47) to (start + 0, 51)
+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 + 196, 13) to (start + 2, 27)
+- Code(Counter(1)) at (prev + 2, 30) to (start + 0, 37)
+- Code(Expression(0, Sub)) at (prev + 0, 47) to (start + 0, 51)
     = (c0 - c1)
-- Code(Expression(0, Add)) at (prev + 1, 13) to (start + 0, 14)
+- Code(Expression(1, Add)) at (prev + 1, 13) to (start + 0, 14)
     = (c1 + (c0 - c1))
 
 Function name: closure::main::{closure#17}