-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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.
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
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,115 @@ | ||
1| |// compile-flags: --edition=2018 | ||
2| | | ||
3| |use core::{ | ||
4| | future::Future, | ||
5| | marker::Send, | ||
6| | pin::Pin, | ||
7| |}; | ||
8| | | ||
9| 1|fn non_async_func() { | ||
10| 1| println!("non_async_func was covered"); | ||
11| 1| let b = true; | ||
12| 1| if b { | ||
13| 1| println!("non_async_func println in block"); | ||
14| 1| } | ||
15| 1|} | ||
16| | | ||
17| |// FIXME(#83985): The auto-generated closure in an async function is failing to include | ||
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| 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| } | ||
^0 | ||
26| 1|} | ||
27| | | ||
28| |// FIXME(#83985): As above, this async function only has the `println!()` macro call, which is not | ||
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| 1| println!("async_func_just_println was covered"); | ||
33| 1|} | ||
34| | | ||
35| 1|fn main() { | ||
36| 1| println!("codecovsample::main"); | ||
37| 1| | ||
38| 1| non_async_func(); | ||
39| 1| | ||
40| 1| executor::block_on(async_func()); | ||
41| 1| executor::block_on(async_func_just_println()); | ||
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 | ||
56| 2| |_| unsafe { unreachable_unchecked() }, // wake | ||
^0 | ||
57| 2| |_| unsafe { unreachable_unchecked() }, // wake_by_ref | ||
^0 | ||
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}>>: | ||
| 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}>>: | ||
| 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| } | ||
------------------ | ||
69| |} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, I have to take this back. After reviewing the implementation of |
||
^0 ^0 ^0 ^1 ^0 | ||
------------------ | ||
| Unexecuted instantiation: <partial_eq::Version as core::cmp::PartialEq>::ne | ||
------------------ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,6 @@ | ||
#![allow(unused_assignments, dead_code)] | ||
|
||
// compile-flags: --edition=2018 -C opt-level=1 # fix in rustc_mir/monomorphize/partitioning/mod.rs | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This comment is no longer relevant. |
||
// compile-flags: --edition=2018 -C opt-level=1 | ||
|
||
async fn c(x: u8) -> u8 { | ||
if x == 8 { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,69 @@ | ||
// compile-flags: --edition=2018 | ||
|
||
use core::{ | ||
future::Future, | ||
marker::Send, | ||
pin::Pin, | ||
}; | ||
|
||
fn non_async_func() { | ||
println!("non_async_func was covered"); | ||
let b = true; | ||
if b { | ||
println!("non_async_func println in block"); | ||
} | ||
} | ||
|
||
// FIXME(#83985): The auto-generated closure in an async function is failing to include | ||
// the println!() and `let` assignment lines in the coverage code region(s), as it does in the | ||
// non-async function above, unless the `println!()` is inside a covered block. | ||
async fn async_func() { | ||
println!("async_func was covered"); | ||
let b = true; | ||
if b { | ||
println!("async_func println in block"); | ||
} | ||
} | ||
|
||
// FIXME(#83985): As above, this async function only has the `println!()` macro call, which is not | ||
// showing coverage, so the entire async closure _appears_ uncovered; but this is not exactly true. | ||
// It's only certain kinds of lines and/or their context that results in missing coverage. | ||
async fn async_func_just_println() { | ||
println!("async_func_just_println was covered"); | ||
} | ||
|
||
fn main() { | ||
println!("codecovsample::main"); | ||
|
||
non_async_func(); | ||
|
||
executor::block_on(async_func()); | ||
executor::block_on(async_func_just_println()); | ||
} | ||
|
||
mod executor { | ||
use core::{ | ||
future::Future, | ||
pin::Pin, | ||
task::{Context, Poll, RawWaker, RawWakerVTable, Waker}, | ||
}; | ||
|
||
pub fn block_on<F: Future>(mut future: F) -> F::Output { | ||
let mut future = unsafe { Pin::new_unchecked(&mut future) }; | ||
use std::hint::unreachable_unchecked; | ||
static VTABLE: RawWakerVTable = RawWakerVTable::new( | ||
|_| unsafe { unreachable_unchecked() }, // clone | ||
|_| unsafe { unreachable_unchecked() }, // wake | ||
|_| unsafe { unreachable_unchecked() }, // wake_by_ref | ||
|_| (), | ||
); | ||
let waker = unsafe { Waker::from_raw(RawWaker::new(core::ptr::null(), &VTABLE)) }; | ||
let mut context = Context::from_waker(&waker); | ||
|
||
loop { | ||
if let Poll::Ready(val) = future.as_mut().poll(&mut context) { | ||
break val; | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
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. inbcb_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-rootSyntaxContext
)?There was a problem hiding this comment.
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 theSyntaxtContext
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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, thebody_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 ororiginal_sp
, when using thebody_span
as the enclosing span. That was the wrong change, and I'm glad to have had the opportunity to review this.