Skip to content

Commit

Permalink
Rollup merge of #84897 - richkadel:cover-closure-macros, r=tmandry
Browse files Browse the repository at this point in the history 
Coverage instruments closure bodies in macros (not the macro body)

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.

r? `@tmandry`
cc: `@wesleywiser`
  • Loading branch information
@JohnTitor
JohnTitor committed May 7, 2021
2 parents b088318 + cb70221 commit 343a094
Show file tree
Hide file tree
Showing 7 changed files with 192 additions and 49 deletions.
27 changes: 24 additions & 3 deletions compiler/rustc_mir/src/transform/coverage/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)]
Expand Down Expand Up @@ -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()
Expand Down
106 changes: 84 additions & 22 deletions src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.closure.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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| ,
Expand Down Expand Up @@ -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|}

14 changes: 7 additions & 7 deletions src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.closure_macro.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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| |
Expand Down
14 changes: 7 additions & 7 deletions src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.closure_macro_async.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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| |
Expand Down
76 changes: 68 additions & 8 deletions src/test/run-make-fulldeps/coverage/closure.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 | {
Expand All @@ -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);
}
2 changes: 1 addition & 1 deletion src/test/run-make-fulldeps/coverage/closure_macro.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
2 changes: 1 addition & 1 deletion src/test/run-make-fulldeps/coverage/closure_macro_async.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down

0 comments on commit 343a094

Please sign in to comment.