Auto merge of #84582 - richkadel:issue-84561, r=tmandry

Vastly improves coverage spans for macros

Fixes: #84561

This resolves problems where macros like `trace!(...)` would show zero coverage if tracing was disabled, and `assert_eq!(...)` would show zero coverage if the assertion did not fail, because only one coverage span was generated, for the branch.

This PR started with an idea that I could just drop branching blocks with same span as expanded macro. (See the fixed issue for more details.)

That did help, but it didn't resolve everything.

I also needed to add a span specifically for the macro name (plus `!`) to ensure the macro gets coverage even if it's internal expansion adds conditional branching blocks that are retained, and would otherwise drop the outer span. Now that outer span is _only_ the `(argument, list)`, which can safely be dropped now), because the macro name has its own span.

While testing, I also noticed the spanview debug output can cause an ICE on a function with no body. The
workaround for this is included in this PR (separate commit).

r? `@tmandry`
cc? `@wesleywiser`

Author: bors

compiler/rustc_mir/src/transform/coverage/spans.rs

@@ -1,6 +1,10 @@
 //! This crate hosts a selection of "unit tests" for components of the `InstrumentCoverage` MIR
 //! pass.
 //!
+//! ```shell
+//! ./x.py test --keep-stage 1 compiler/rustc_mir --test-args '--show-output coverage'
+//! ```
+//!
 //! The tests construct a few "mock" objects, as needed, to support the `InstrumentCoverage`
 //! functions and algorithms. Mocked objects include instances of `mir::Body`; including
 //! `Terminator`s of various `kind`s, and `Span` objects. Some functions used by or used on
@@ -679,10 +683,15 @@ fn test_make_bcb_counters() {
         let mut basic_coverage_blocks = graph::CoverageGraph::from_mir(&mir_body);
         let mut coverage_spans = Vec::new();
         for (bcb, data) in basic_coverage_blocks.iter_enumerated() {
-            if let Some(span) =
+            if let Some((span, expn_span)) =
                 spans::filtered_terminator_span(data.terminator(&mir_body), body_span)
             {
-                coverage_spans.push(spans::CoverageSpan::for_terminator(span, bcb, data.last_bb()));
+                coverage_spans.push(spans::CoverageSpan::for_terminator(
+                    span,
+                    expn_span,
+                    bcb,
+                    data.last_bb(),
+                ));
             }
         }
         let mut coverage_counters = counters::CoverageCounters::new(0);

compiler/rustc_mir/src/transform/coverage/tests.rs

@@ -99,7 +99,11 @@ where
     W: Write,
 {
     let def_id = body.source.def_id();
-    let body_span = hir_body(tcx, def_id).value.span;
+    let hir_body = hir_body(tcx, def_id);
+    if hir_body.is_none() {
+        return Ok(());
+    }
+    let body_span = hir_body.unwrap().value.span;
     let mut span_viewables = Vec::new();
     for (bb, data) in body.basic_blocks().iter_enumerated() {
         match spanview {
@@ -664,19 +668,16 @@ fn fn_span<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId) -> Span {
     let hir_id =
         tcx.hir().local_def_id_to_hir_id(def_id.as_local().expect("expected DefId is local"));
     let fn_decl_span = tcx.hir().span(hir_id);
-    let body_span = hir_body(tcx, def_id).value.span;
-    if fn_decl_span.ctxt() == body_span.ctxt() {
-        fn_decl_span.to(body_span)
+    if let Some(body_span) = hir_body(tcx, def_id).map(|hir_body| hir_body.value.span) {
+        if fn_decl_span.ctxt() == body_span.ctxt() { fn_decl_span.to(body_span) } else { body_span }
     } else {
-        // This probably occurs for functions defined via macros
-        body_span
+        fn_decl_span
     }
 }
 
-fn hir_body<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId) -> &'tcx rustc_hir::Body<'tcx> {
+fn hir_body<'tcx>(tcx: TyCtxt<'tcx>, def_id: DefId) -> Option<&'tcx rustc_hir::Body<'tcx>> {
     let hir_node = tcx.hir().get_if_local(def_id).expect("expected DefId is local");
-    let fn_body_id = hir::map::associated_body(hir_node).expect("HIR node is a function with body");
-    tcx.hir().body(fn_body_id)
+    hir::map::associated_body(hir_node).map(|fn_body_id| tcx.hir().body(fn_body_id))
 }
 
 fn escape_html(s: &str) -> String {

compiler/rustc_mir/src/util/spanview.rs

@@ -0,0 +1,42 @@
+    1|       |// compile-flags: --edition=2018
+    2|       |#![feature(no_coverage)]
+    3|       |
+    4|       |macro_rules! bail {
+    5|       |    ($msg:literal $(,)?) => {
+    6|       |        if $msg.len() > 0 {
+    7|       |            println!("no msg");
+    8|       |        } else {
+    9|       |            println!($msg);
+   10|       |        }
+   11|       |        return Err(String::from($msg));
+   12|       |    };
+   13|       |}
+   14|       |
+   15|       |macro_rules! on_error {
+   16|       |    ($value:expr, $error_message:expr) => {
+   17|      0|        $value.or_else(|e| {
+   18|      0|            let message = format!($error_message, e);
+   19|      0|            if message.len() > 0 {
+   20|      0|                println!("{}", message);
+   21|      0|                Ok(String::from("ok"))
+   22|       |            } else {
+   23|      0|                bail!("error");
+   24|       |            }
+   25|      0|        })
+   26|       |    };
+   27|       |}
+   28|       |
+   29|      1|fn load_configuration_files() -> Result<String, String> {
+   30|      1|    Ok(String::from("config"))
+   31|      1|}
+   32|       |
+   33|      1|pub fn main() -> Result<(), String> {
+   34|      1|    println!("Starting service");
+   35|      1|    let config = on_error!(load_configuration_files(), "Error loading configs: {}")?;
+                                                                                                 ^0
+   36|       |
+   37|      1|    let startup_delay_duration = String::from("arg");
+   38|      1|    let _ = (config, startup_delay_duration);
+   39|      1|    Ok(())
+   40|      1|}
+

src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.closure_macro.txt

@@ -0,0 +1,83 @@
+    1|       |// compile-flags: --edition=2018
+    2|       |#![feature(no_coverage)]
+    3|       |
+    4|       |macro_rules! bail {
+    5|       |    ($msg:literal $(,)?) => {
+    6|       |        if $msg.len() > 0 {
+    7|       |            println!("no msg");
+    8|       |        } else {
+    9|       |            println!($msg);
+   10|       |        }
+   11|       |        return Err(String::from($msg));
+   12|       |    };
+   13|       |}
+   14|       |
+   15|       |macro_rules! on_error {
+   16|       |    ($value:expr, $error_message:expr) => {
+   17|      0|        $value.or_else(|e| {
+   18|      0|            let message = format!($error_message, e);
+   19|      0|            if message.len() > 0 {
+   20|      0|                println!("{}", message);
+   21|      0|                Ok(String::from("ok"))
+   22|       |            } else {
+   23|      0|                bail!("error");
+   24|       |            }
+   25|      0|        })
+   26|       |    };
+   27|       |}
+   28|       |
+   29|      1|fn load_configuration_files() -> Result<String, String> {
+   30|      1|    Ok(String::from("config"))
+   31|      1|}
+   32|       |
+   33|      1|pub async fn test() -> Result<(), String> {
+   34|      1|    println!("Starting service");
+   35|      1|    let config = on_error!(load_configuration_files(), "Error loading configs: {}")?;
+                                                                                                 ^0
+   36|       |
+   37|      1|    let startup_delay_duration = String::from("arg");
+   38|      1|    let _ = (config, startup_delay_duration);
+   39|      1|    Ok(())
+   40|      1|}
+   41|       |
+   42|       |#[no_coverage]
+   43|       |fn main() {
+   44|       |    executor::block_on(test());
+   45|       |}
+   46|       |
+   47|       |mod executor {
+   48|       |    use core::{
+   49|       |        future::Future,
+   50|       |        pin::Pin,
+   51|       |        task::{Context, Poll, RawWaker, RawWakerVTable, Waker},
+   52|       |    };
+   53|       |
+   54|       |    #[no_coverage]
+   55|       |    pub fn block_on<F: Future>(mut future: F) -> F::Output {
+   56|       |        let mut future = unsafe { Pin::new_unchecked(&mut future) };
+   57|       |        use std::hint::unreachable_unchecked;
+   58|       |        static VTABLE: RawWakerVTable = RawWakerVTable::new(
+   59|       |
+   60|       |            #[no_coverage]
+   61|       |            |_| unsafe { unreachable_unchecked() }, // clone
+   62|       |
+   63|       |            #[no_coverage]
+   64|       |            |_| unsafe { unreachable_unchecked() }, // wake
+   65|       |
+   66|       |            #[no_coverage]
+   67|       |            |_| unsafe { unreachable_unchecked() }, // wake_by_ref
+   68|       |
+   69|       |            #[no_coverage]
+   70|       |            |_| (),
+   71|       |        );
+   72|       |        let waker = unsafe { Waker::from_raw(RawWaker::new(core::ptr::null(), &VTABLE)) };
+   73|       |        let mut context = Context::from_waker(&waker);
+   74|       |
+   75|       |        loop {
+   76|       |            if let Poll::Ready(val) = future.as_mut().poll(&mut context) {
+   77|       |                break val;
+   78|       |            }
+   79|       |        }
+   80|       |    }
+   81|       |}
+

src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.closure_macro_async.txt

@@ -1,9 +1,9 @@
     1|       |#![allow(unused_assignments, unused_variables, dead_code)]
     2|       |
     3|      1|fn main() {
-    4|       |    // Initialize test constants in a way that cannot be determined at compile time, to ensure
-    5|       |    // rustc and LLVM cannot optimize out statements (or coverage counters) downstream from
-    6|       |    // dependent conditions.
+    4|      1|    // Initialize test constants in a way that cannot be determined at compile time, to ensure
+    5|      1|    // rustc and LLVM cannot optimize out statements (or coverage counters) downstream from
+    6|      1|    // dependent conditions.
     7|      1|    let is_true = std::env::args().len() == 1;
     8|      1|
     9|      1|    let mut countdown = 0;

src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.inner_items.txt

@@ -12,9 +12,9 @@
     5|       |
     6|      1|fn main() {
     7|      1|    let bar = Foo(1);
-    8|      0|    assert_eq!(bar, Foo(1));
+    8|      1|    assert_eq!(bar, Foo(1));
     9|      1|    let baz = Foo(0);
-   10|      0|    assert_ne!(baz, Foo(1));
+   10|      1|    assert_ne!(baz, Foo(1));
    11|      1|    println!("{:?}", Foo(1));
    12|      1|    println!("{:?}", bar);
    13|      1|    println!("{:?}", baz);