Skip to content

coverage of async function bodies should match non-async #84323

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 20, 2021
Merged

coverage of async function bodies should match non-async #84323

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
3ece606
DRAFT: coverage of async function bodies should match non-async
richkadel Apr 18, 2021
1893721
Fixes the issue with uncovered source in async function bodies
richkadel Apr 18, 2021
5d8d67f
compute fn_sig span from body call_site, and use body ctxt, not root
richkadel Apr 19, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions compiler/rustc_mir/src/transform/coverage/spans.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,8 +246,8 @@ impl<'a, 'tcx> CoverageSpans<'a, 'tcx> {
) -> Vec<CoverageSpan> {
let mut coverage_spans = CoverageSpans {
mir_body,
fn_sig_span,
body_span,
fn_sig_span: fn_sig_span.with_ctxt(SyntaxContext::root()),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look like the SyntaxContext is adjusted in any other places (e.g. in bcb_to_initial_coverage_spans). Is there something special about async desugarings that makes this necessary? What happens if the body of a function is generated from a macro (and therefore has a non-root SyntaxContext)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See function_source_span(), which does adjust the SyntaxtContext for all other spans.

All of the function body spans are extrapolated to their rustc_span::source_map::original_sp.

They are assumed to then be in the context of the body.

The bug, fixed by this PR, was that I assumed the body would always be in the root. Now all contexts are normalized to root.

This allows me to use Span::to(), which, otherwise, doesn't always return the expected result, if the contexts are different (even though they are in the same source).

The end result (in tests and in many practical use cases) provide the evidence that this approach works, except for the bug fixed by this PR, which is now obvious.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm taking a closer look at this though, and trying an alternative that may be relevant to the concern you raised.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just uploaded a modified solution (see the new commit). This still forces all spans (fn_sig, body, and the MIR sources) to the same ctxt, so Span::to() should still work, but it does not force everything to the root ctxt anymore. Instead, the body_span's ctxt is now the common ctxt.

In the previous commit, I normalized the body_span's ctxt to the root, but that would have changed the behavior or original_sp, when using the body_span as the enclosing span. That was the wrong change, and I'm glad to have had the opportunity to review this.

body_span: body_span.with_ctxt(SyntaxContext::root()),
basic_coverage_blocks,
sorted_spans_iter: None,
refined_spans: Vec::with_capacity(basic_coverage_blocks.num_nodes() * 2),
Expand Down
145 changes: 68 additions & 77 deletions src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.async2.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,8 @@
18| |// the println!() and `let` assignment lines in the coverage code region(s), as it does in the
19| |// non-async function above, unless the `println!()` is inside a covered block.
20| 1|async fn async_func() {
21| | println!("async_func was covered");
22| | let b = true;
21| 1| println!("async_func was covered");
22| 1| let b = true;
23| 1| if b {
24| 1| println!("async_func println in block");
25| 1| }
Expand All @@ -30,8 +30,8 @@
29| |// showing coverage, so the entire async closure _appears_ uncovered; but this is not exactly true.
30| |// It's only certain kinds of lines and/or their context that results in missing coverage.
31| 1|async fn async_func_just_println() {
32| | println!("async_func_just_println was covered");
33| |}
32| 1| println!("async_func_just_println was covered");
33| 1|}
34| |
35| 1|fn main() {
36| 1| println!("codecovsample::main");
Expand All @@ -40,85 +40,76 @@
39| 1|
40| 1| executor::block_on(async_func());
41| 1| executor::block_on(async_func_just_println());
42| 1|
43| 1| // let mut future = Box::pin(async_func());
44| 1| // executor::block_on(future.as_mut());
45| 1|
46| 1| // let mut future = Box::pin(async_func());
47| 1| // executor::block_on(future.as_mut());
48| 1|
49| 1| // let mut future = Box::pin(async_func_just_println());
50| 1| // executor::block_on(future.as_mut());
51| 1|}
52| |
53| |mod executor {
54| | use core::{
55| | future::Future,
56| | pin::Pin,
57| | task::{Context, Poll, RawWaker, RawWakerVTable, Waker},
58| | };
59| |
60| 2| pub fn block_on<F: Future>(mut future: F) -> F::Output {
61| 2| let mut future = unsafe { Pin::new_unchecked(&mut future) };
62| 2| use std::hint::unreachable_unchecked;
63| 2| static VTABLE: RawWakerVTable = RawWakerVTable::new(
64| 2| |_| unsafe { unreachable_unchecked() }, // clone
42| 1|}
43| |
44| |mod executor {
45| | use core::{
46| | future::Future,
47| | pin::Pin,
48| | task::{Context, Poll, RawWaker, RawWakerVTable, Waker},
49| | };
50| |
51| 2| pub fn block_on<F: Future>(mut future: F) -> F::Output {
52| 2| let mut future = unsafe { Pin::new_unchecked(&mut future) };
53| 2| use std::hint::unreachable_unchecked;
54| 2| static VTABLE: RawWakerVTable = RawWakerVTable::new(
55| 2| |_| unsafe { unreachable_unchecked() }, // clone
^0
65| 2| |_| unsafe { unreachable_unchecked() }, // wake
56| 2| |_| unsafe { unreachable_unchecked() }, // wake
^0
66| 2| |_| unsafe { unreachable_unchecked() }, // wake_by_ref
57| 2| |_| unsafe { unreachable_unchecked() }, // wake_by_ref
^0
67| 2| |_| (),
68| 2| );
69| 2| let waker = unsafe { Waker::from_raw(RawWaker::new(core::ptr::null(), &VTABLE)) };
70| 2| let mut context = Context::from_waker(&waker);
71| |
72| | loop {
73| 2| if let Poll::Ready(val) = future.as_mut().poll(&mut context) {
74| 2| break val;
75| 0| }
76| | }
77| 2| }
58| 2| |_| (),
59| 2| );
60| 2| let waker = unsafe { Waker::from_raw(RawWaker::new(core::ptr::null(), &VTABLE)) };
61| 2| let mut context = Context::from_waker(&waker);
62| |
63| | loop {
64| 2| if let Poll::Ready(val) = future.as_mut().poll(&mut context) {
65| 2| break val;
66| 0| }
67| | }
68| 2| }
------------------
| async2::executor::block_on::<core::future::from_generator::GenFuture<async2::async_func::{closure#0}>>:
| 60| 1| pub fn block_on<F: Future>(mut future: F) -> F::Output {
| 61| 1| let mut future = unsafe { Pin::new_unchecked(&mut future) };
| 62| 1| use std::hint::unreachable_unchecked;
| 63| 1| static VTABLE: RawWakerVTable = RawWakerVTable::new(
| 64| 1| |_| unsafe { unreachable_unchecked() }, // clone
| 65| 1| |_| unsafe { unreachable_unchecked() }, // wake
| 66| 1| |_| unsafe { unreachable_unchecked() }, // wake_by_ref
| 67| 1| |_| (),
| 68| 1| );
| 69| 1| let waker = unsafe { Waker::from_raw(RawWaker::new(core::ptr::null(), &VTABLE)) };
| 70| 1| let mut context = Context::from_waker(&waker);
| 71| |
| 72| | loop {
| 73| 1| if let Poll::Ready(val) = future.as_mut().poll(&mut context) {
| 74| 1| break val;
| 75| 0| }
| 76| | }
| 77| 1| }
| 51| 1| pub fn block_on<F: Future>(mut future: F) -> F::Output {
| 52| 1| let mut future = unsafe { Pin::new_unchecked(&mut future) };
| 53| 1| use std::hint::unreachable_unchecked;
| 54| 1| static VTABLE: RawWakerVTable = RawWakerVTable::new(
| 55| 1| |_| unsafe { unreachable_unchecked() }, // clone
| 56| 1| |_| unsafe { unreachable_unchecked() }, // wake
| 57| 1| |_| unsafe { unreachable_unchecked() }, // wake_by_ref
| 58| 1| |_| (),
| 59| 1| );
| 60| 1| let waker = unsafe { Waker::from_raw(RawWaker::new(core::ptr::null(), &VTABLE)) };
| 61| 1| let mut context = Context::from_waker(&waker);
| 62| |
| 63| | loop {
| 64| 1| if let Poll::Ready(val) = future.as_mut().poll(&mut context) {
| 65| 1| break val;
| 66| 0| }
| 67| | }
| 68| 1| }
------------------
| async2::executor::block_on::<core::future::from_generator::GenFuture<async2::async_func_just_println::{closure#0}>>:
| 60| 1| pub fn block_on<F: Future>(mut future: F) -> F::Output {
| 61| 1| let mut future = unsafe { Pin::new_unchecked(&mut future) };
| 62| 1| use std::hint::unreachable_unchecked;
| 63| 1| static VTABLE: RawWakerVTable = RawWakerVTable::new(
| 64| 1| |_| unsafe { unreachable_unchecked() }, // clone
| 65| 1| |_| unsafe { unreachable_unchecked() }, // wake
| 66| 1| |_| unsafe { unreachable_unchecked() }, // wake_by_ref
| 67| 1| |_| (),
| 68| 1| );
| 69| 1| let waker = unsafe { Waker::from_raw(RawWaker::new(core::ptr::null(), &VTABLE)) };
| 70| 1| let mut context = Context::from_waker(&waker);
| 71| |
| 72| | loop {
| 73| 1| if let Poll::Ready(val) = future.as_mut().poll(&mut context) {
| 74| 1| break val;
| 75| 0| }
| 76| | }
| 77| 1| }
| 51| 1| pub fn block_on<F: Future>(mut future: F) -> F::Output {
| 52| 1| let mut future = unsafe { Pin::new_unchecked(&mut future) };
| 53| 1| use std::hint::unreachable_unchecked;
| 54| 1| static VTABLE: RawWakerVTable = RawWakerVTable::new(
| 55| 1| |_| unsafe { unreachable_unchecked() }, // clone
| 56| 1| |_| unsafe { unreachable_unchecked() }, // wake
| 57| 1| |_| unsafe { unreachable_unchecked() }, // wake_by_ref
| 58| 1| |_| (),
| 59| 1| );
| 60| 1| let waker = unsafe { Waker::from_raw(RawWaker::new(core::ptr::null(), &VTABLE)) };
| 61| 1| let mut context = Context::from_waker(&waker);
| 62| |
| 63| | loop {
| 64| 1| if let Poll::Ready(val) = future.as_mut().poll(&mut context) {
| 65| 1| break val;
| 66| 0| }
| 67| | }
| 68| 1| }
------------------
78| |}
69| |}

2 changes: 1 addition & 1 deletion src/test/run-make-fulldeps/coverage-reports/expected_show_coverage.partial_eq.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
2| |// structure of this test.
3| |
4| 2|#[derive(Clone, Debug, PartialEq, Eq, PartialOrd, Ord)]
^0 ^0 ^0 ^0 ^1 ^1 ^0^0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this fix also resolves the mystery (by removing them) of the double counters with some derived traits.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I have to take this back. After reviewing the implementation of original_sp(), I believe preemptively forcing the body to the root context would have changed the behavior of original_sp() on other source spans in a lossy way. Dropping the "extra" coverage counters here was a happy side effect of what was probably not a good change, and my new commit no longer has that side effect, but is probably more correct.

^0 ^0 ^0 ^1 ^0
------------------
| Unexecuted instantiation: <partial_eq::Version as core::cmp::PartialEq>::ne
------------------
Expand Down
9 changes: 0 additions & 9 deletions src/test/run-make-fulldeps/coverage/async2.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,15 +39,6 @@ fn main() {

executor::block_on(async_func());
executor::block_on(async_func_just_println());

// let mut future = Box::pin(async_func());
// executor::block_on(future.as_mut());

// let mut future = Box::pin(async_func());
// executor::block_on(future.as_mut());

// let mut future = Box::pin(async_func_just_println());
// executor::block_on(future.as_mut());
}

mod executor {
Expand Down