Coverage instruments closure bodies in macros (not the macro body)

richkadel · richkadel · commit cb70221857d7 · 2021-05-06T11:15:39.000-07:00
Fixes: #84884 This solution might be considered a compromise, but I think it is the better choice. The results in the `closure.rs` test correctly resolve all test cases broken as described in #84884. One test pattern (in both `closure_macro.rs` and `closure_macro_async.rs`) was also affected, and removes coverage statistics for the lines inside the closure, because the closure includes a macro. (The coverage remains at the callsite of the macro, so we lose some detail, but there isn't a perfect choice with macros. Often macro implementations are split across the macro and the callsite, and there doesn't appear to be a single "right choice" for which body should be covered. For the current implementation, we can't do both. The callsite is most likely to be the preferred site for coverage. I applied this fix to all `MacroKinds`, not just `Bang`. I'm trying to resolve an issue of lost coverage in a `MacroKind::Attr`-based, function-scoped macro. Instead of only searching for a body_span that is "not a function-like macro" (that is, MacroKind::Bang), I'm expanding this to all `MacroKind`s. Maybe I should expand this to `ExpnKind::Desugaring` and `ExpnKind::AstPass` (or subsets, depending on their sub-kinds) as well, but I'm not sure that's a good idea. I'd like to add a test of the `Attr` macro on functions, but I need time to figure out how to constract a good, simple example without external crate dependencies. For the moment, all tests still work as expected (no change), this new commit shouldn't have a negative affect, and more importantly, I believe it will have a positive effect. I will try to confirm this.
diff --git a/compiler/rustc_mir/src/transform/coverage/mod.rs b/compiler/rustc_mir/src/transform/coverage/mod.rs
@@ -32,7 +32,7 @@ use rustc_middle::mir::{
 use rustc_middle::ty::TyCtxt;
 use rustc_span::def_id::DefId;
 use rustc_span::source_map::SourceMap;
-use rustc_span::{CharPos, Pos, SourceFile, Span, Symbol};
+use rustc_span::{CharPos, ExpnKind, Pos, SourceFile, Span, Symbol};
 
 /// A simple error message wrapper for `coverage::Error`s.
 #[derive(Debug)]
@@ -113,8 +113,29 @@ struct Instrumentor<'a, 'tcx> {
 impl<'a, 'tcx> Instrumentor<'a, 'tcx> {
     fn new(pass_name: &'a str, tcx: TyCtxt<'tcx>, mir_body: &'a mut mir::Body<'tcx>) -> Self {
         let source_map = tcx.sess.source_map();
-        let (some_fn_sig, hir_body) = fn_sig_and_body(tcx, mir_body.source.def_id());
-        let body_span = hir_body.value.span;
+        let def_id = mir_body.source.def_id();
+        let (some_fn_sig, hir_body) = fn_sig_and_body(tcx, def_id);
+
+        let mut body_span = hir_body.value.span;
+
+        if tcx.is_closure(def_id) {
+            // If the MIR function is a closure, and if the closure body span
+            // starts from a macro, but it's content is not in that macro, try
+            // to find a non-macro callsite, and instrument the spans there
+            // instead.
+            loop {
+                let expn_data = body_span.ctxt().outer_expn_data();
+                if expn_data.is_root() {
+                    break;
+                }
+                if let ExpnKind::Macro(..) = expn_data.kind {
+                    body_span = expn_data.call_site;
+                } else {
+                    break;
+                }
+            }
+        }
+
         let source_file = source_map.lookup_source_file(body_span.lo());
         let fn_sig_span = match some_fn_sig.filter(|fn_sig| {
             fn_sig.span.ctxt() == body_span.ctxt()
diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.closure.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.closure.txt
@@ -37,7 +37,7 @@
    37|      0|            countdown = 10;
    38|      0|        }
    39|      0|        "alt string 2".to_owned()
-   40|      1|    };
+   40|      0|    };
    41|      1|    println!(
    42|      1|        "The string or alt: {}"
    43|      1|        ,
@@ -125,36 +125,98 @@
   125|      0|            countdown = 10;
   126|      0|        }
   127|      0|        "closure should be unused".to_owned()
-  128|      1|    };
-  129|      1|
+  128|      0|    };
+  129|       |
   130|      1|    let mut countdown = 10;
   131|      1|    let _short_unused_closure = | _unused_arg: u8 | countdown += 1;
                                                                   ^0
-  132|      1|
-  133|      1|    // Macros can sometimes confuse the coverage results. Compare this next assignment, with an
-  134|      1|    // unused closure that invokes the `println!()` macro, with the closure assignment above, that
-  135|      1|    // does not use a macro. The closure above correctly shows `0` executions.
-  136|      1|    let _short_unused_closure = | _unused_arg: u8 | println!("not called");
-  137|      1|    // The closure assignment above is executed, with a line count of `1`, but the `println!()`
-  138|      1|    // could not have been called, and yet, there is no indication that it wasn't...
-  139|      1|
-  140|      1|    // ...but adding block braces gives the expected result, showing the block was not executed.
+  132|       |
+  133|       |
+  134|      1|    let short_used_covered_closure_macro = | used_arg: u8 | println!("called");
+  135|      1|    let short_used_not_covered_closure_macro = | used_arg: u8 | println!("not called");
+                                                                              ^0
+  136|      1|    let _short_unused_closure_macro = | _unused_arg: u8 | println!("not called");
+                                                                        ^0
+  137|       |
+  138|       |
+  139|       |
+  140|       |
   141|      1|    let _short_unused_closure_block = | _unused_arg: u8 | { println!("not called") };
                                                                         ^0
-  142|      1|
+  142|       |
   143|      1|    let _shortish_unused_closure = | _unused_arg: u8 | {
   144|      0|        println!("not called")
-  145|      1|    };
-  146|      1|
+  145|      0|    };
+  146|       |
   147|      1|    let _as_short_unused_closure = |
   148|       |        _unused_arg: u8
-  149|      1|    | { println!("not called") };
-                    ^0
-  150|      1|
+  149|      0|    | { println!("not called") };
+  150|       |
   151|      1|    let _almost_as_short_unused_closure = |
   152|       |        _unused_arg: u8
-  153|      1|    | { println!("not called") }
-                    ^0
-  154|      1|    ;
-  155|      1|}
+  153|      0|    | { println!("not called") }
+  154|       |    ;
+  155|       |
+  156|       |
+  157|       |
+  158|       |
+  159|       |
+  160|      1|    let _short_unused_closure_line_break_no_block = | _unused_arg: u8 |
+  161|      0|println!("not called")
+  162|       |    ;
+  163|       |
+  164|      1|    let _short_unused_closure_line_break_no_block2 =
+  165|       |        | _unused_arg: u8 |
+  166|      0|            println!(
+  167|      0|                "not called"
+  168|      0|            )
+  169|       |    ;
+  170|       |
+  171|      1|    let short_used_not_covered_closure_line_break_no_block_embedded_branch =
+  172|      1|        | _unused_arg: u8 |
+  173|      0|            println!(
+  174|      0|                "not called: {}",
+  175|      0|                if is_true { "check" } else { "me" }
+  176|      0|            )
+  177|       |    ;
+  178|       |
+  179|      1|    let short_used_not_covered_closure_line_break_block_embedded_branch =
+  180|      1|        | _unused_arg: u8 |
+  181|      0|        {
+  182|      0|            println!(
+  183|      0|                "not called: {}",
+  184|      0|                if is_true { "check" } else { "me" }
+  185|       |            )
+  186|      0|        }
+  187|       |    ;
+  188|       |
+  189|      1|    let short_used_covered_closure_line_break_no_block_embedded_branch =
+  190|      1|        | _unused_arg: u8 |
+  191|      1|            println!(
+  192|      1|                "not called: {}",
+  193|      1|                if is_true { "check" } else { "me" }
+                                                            ^0
+  194|      1|            )
+  195|       |    ;
+  196|       |
+  197|      1|    let short_used_covered_closure_line_break_block_embedded_branch =
+  198|      1|        | _unused_arg: u8 |
+  199|      1|        {
+  200|      1|            println!(
+  201|      1|                "not called: {}",
+  202|      1|                if is_true { "check" } else { "me" }
+                                                            ^0
+  203|       |            )
+  204|      1|        }
+  205|       |    ;
+  206|       |
+  207|      1|    if is_false {
+  208|      0|        short_used_not_covered_closure_macro(0);
+  209|      0|        short_used_not_covered_closure_line_break_no_block_embedded_branch(0);
+  210|      0|        short_used_not_covered_closure_line_break_block_embedded_branch(0);
+  211|      1|    }
+  212|      1|    short_used_covered_closure_macro(0);
+  213|      1|    short_used_covered_closure_line_break_no_block_embedded_branch(0);
+  214|      1|    short_used_covered_closure_line_break_block_embedded_branch(0);
+  215|      1|}
 
diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.closure_macro.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.closure_macro.txt
@@ -14,15 +14,15 @@
    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"))
+   17|       |        $value.or_else(|e| { // FIXME(85000): no coverage in closure macros
+   18|       |            let message = format!($error_message, e);
+   19|       |            if message.len() > 0 {
+   20|       |                println!("{}", message);
+   21|       |                Ok(String::from("ok"))
    22|       |            } else {
-   23|      0|                bail!("error");
+   23|       |                bail!("error");
    24|       |            }
-   25|      0|        })
+   25|       |        })
    26|       |    };
    27|       |}
    28|       |
diff --git a/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.closure_macro_async.txt b/src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.closure_macro_async.txt
@@ -14,15 +14,15 @@
    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"))
+   17|       |        $value.or_else(|e| { // FIXME(85000): no coverage in closure macros
+   18|       |            let message = format!($error_message, e);
+   19|       |            if message.len() > 0 {
+   20|       |                println!("{}", message);
+   21|       |                Ok(String::from("ok"))
    22|       |            } else {
-   23|      0|                bail!("error");
+   23|       |                bail!("error");
    24|       |            }
-   25|      0|        })
+   25|       |        })
    26|       |    };
    27|       |}
    28|       |
diff --git a/src/test/run-make-fulldeps/coverage/closure.rs b/src/test/run-make-fulldeps/coverage/closure.rs
@@ -130,14 +130,14 @@ fn main() {
     let mut countdown = 10;
     let _short_unused_closure = | _unused_arg: u8 | countdown += 1;
 
-    // Macros can sometimes confuse the coverage results. Compare this next assignment, with an
-    // unused closure that invokes the `println!()` macro, with the closure assignment above, that
-    // does not use a macro. The closure above correctly shows `0` executions.
-    let _short_unused_closure = | _unused_arg: u8 | println!("not called");
-    // The closure assignment above is executed, with a line count of `1`, but the `println!()`
-    // could not have been called, and yet, there is no indication that it wasn't...
-
-    // ...but adding block braces gives the expected result, showing the block was not executed.
+
+    let short_used_covered_closure_macro = | used_arg: u8 | println!("called");
+    let short_used_not_covered_closure_macro = | used_arg: u8 | println!("not called");
+    let _short_unused_closure_macro = | _unused_arg: u8 | println!("not called");
+
+
+
+
     let _short_unused_closure_block = | _unused_arg: u8 | { println!("not called") };
 
     let _shortish_unused_closure = | _unused_arg: u8 | {
@@ -152,4 +152,64 @@ fn main() {
         _unused_arg: u8
     | { println!("not called") }
     ;
+
+
+
+
+
+    let _short_unused_closure_line_break_no_block = | _unused_arg: u8 |
+println!("not called")
+    ;
+
+    let _short_unused_closure_line_break_no_block2 =
+        | _unused_arg: u8 |
+            println!(
+                "not called"
+            )
+    ;
+
+    let short_used_not_covered_closure_line_break_no_block_embedded_branch =
+        | _unused_arg: u8 |
+            println!(
+                "not called: {}",
+                if is_true { "check" } else { "me" }
+            )
+    ;
+
+    let short_used_not_covered_closure_line_break_block_embedded_branch =
+        | _unused_arg: u8 |
+        {
+            println!(
+                "not called: {}",
+                if is_true { "check" } else { "me" }
+            )
+        }
+    ;
+
+    let short_used_covered_closure_line_break_no_block_embedded_branch =
+        | _unused_arg: u8 |
+            println!(
+                "not called: {}",
+                if is_true { "check" } else { "me" }
+            )
+    ;
+
+    let short_used_covered_closure_line_break_block_embedded_branch =
+        | _unused_arg: u8 |
+        {
+            println!(
+                "not called: {}",
+                if is_true { "check" } else { "me" }
+            )
+        }
+    ;
+
+    if is_false {
+        short_used_not_covered_closure_macro(0);
+        short_used_not_covered_closure_line_break_no_block_embedded_branch(0);
+        short_used_not_covered_closure_line_break_block_embedded_branch(0);
+    }
+    short_used_covered_closure_macro(0);
+    short_used_covered_closure_line_break_no_block_embedded_branch(0);
+    short_used_covered_closure_line_break_block_embedded_branch(0);
 }
diff --git a/src/test/run-make-fulldeps/coverage/closure_macro.rs b/src/test/run-make-fulldeps/coverage/closure_macro.rs
@@ -14,7 +14,7 @@ macro_rules! bail {
 
 macro_rules! on_error {
     ($value:expr, $error_message:expr) => {
-        $value.or_else(|e| {
+        $value.or_else(|e| { // FIXME(85000): no coverage in closure macros
             let message = format!($error_message, e);
             if message.len() > 0 {
                 println!("{}", message);
diff --git a/src/test/run-make-fulldeps/coverage/closure_macro_async.rs b/src/test/run-make-fulldeps/coverage/closure_macro_async.rs
@@ -14,7 +14,7 @@ macro_rules! bail {
 
 macro_rules! on_error {
     ($value:expr, $error_message:expr) => {
-        $value.or_else(|e| {
+        $value.or_else(|e| { // FIXME(85000): no coverage in closure macros
             let message = format!($error_message, e);
             if message.len() > 0 {
                 println!("{}", message);
