Fix tracing::instrument async block coverage#158276
Open
xd009642 wants to merge 1 commit into
Open
Conversation
Collaborator
|
Some changes occurred in coverage instrumentation. cc @Zalathar Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
Collaborator
|
r? @nnethercote rustbot has assigned @nnethercote. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
Attempt to tighten up const-eval based eligibility checks to prevent us
from discarding too many regions.
Using tracing::instrument code such as:
```rust
#[tracing::instrument]
pub async fn fetch(id: u64) -> Result<String, Error> {
let row = db_get(id).await?;
Ok(row.name)
}
```
Often results in functions with no coverage counters.
Using the following MRE:
```rust
use tracing::instrument;
pub async fn plain_async_branch(value: u8) -> &'static str {
if value.is_multiple_of(2) {
"plain even"
} else {
"plain odd"
}
}
pub fn instrumented_sync_branch(value: u8) -> &'static str {
if value.is_multiple_of(2) {
"sync even"
} else {
"sync odd"
}
}
pub async fn instrumented_async_branch(value: u8) -> &'static str {
if value.is_multiple_of(2) {
"async even"
} else {
"async odd"
}
}
mod tests {
use super::*;
#[tokio::test]
async fn plain_async_branch_reports_coverage() {
assert_eq!(plain_async_branch(2).await, "plain even");
assert_eq!(plain_async_branch(3).await, "plain odd");
}
#[test]
fn instrumented_sync_branch_reports_coverage() {
assert_eq!(instrumented_sync_branch(2), "sync even");
assert_eq!(instrumented_sync_branch(3), "sync odd");
}
#[tokio::test]
async fn instrumented_async_branch_reports_coverage() {
assert_eq!(instrumented_async_branch(2).await, "async even");
assert_eq!(instrumented_async_branch(3).await, "async odd");
}
}
```
We find that `instrumented_async_branch` is always uncovered. An
investigation lead me to `is_eligible_for_coverage` in
`rustc_mir_transform` as being the cause of filtering it out.
After that I ended up taking the very scientific approach of searching const in
https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/struct.TyCtxt.html
and seeing the note for constness picking `is_const_fn ||
is_const_trait_impl`. (I did also try `hir_body_const_context` but that
seemed to not work for me as things that can potentially be const folded
show up?).
Now with this change I see that the region isn't marked as ineligible
but somewhere further along in processing it gets removed. This last
point is why this PR is still in it's draft state as there still seems
to be something afoot.
6ccf282 to
e741c23
Compare
Collaborator
|
A job failed! Check out the build log: (web) (plain enhanced) (plain) Click to see the possible cause of the failure (guessed by this bot) |
Contributor
|
This is a small and simple change and it seems reasonable. But I will defer to someone who knows a lot more about this code than I do. r? @Zalathar |
Collaborator
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #131119 (or tries to...)
Using tracing::instrument code such as:
Often results in functions with no coverage counters.
Using the following MRE:
We find that
instrumented_async_branchis always uncovered. An investigation lead me tois_eligible_for_coverageinrustc_mir_transformas being the cause of filtering it out.After that I ended up taking the very scientific approach of searching const in https://doc.rust-lang.org/nightly/nightly-rustc/rustc_middle/ty/struct.TyCtxt.html and seeing the note for constness picking
is_const_fn || is_const_trait_impl. (I did also tryhir_body_const_contextbut that seemed to not work for me as things that can potentially be const folded show up?).Now with this change I see that the region isn't marked as ineligible but somewhere further along in processing it gets removed. This last point is why this PR is still in it's
draft state as there still seems to be something afootremoved draft state as I need some input or guidance 😅 .