coverage: Count await when the Future is immediately ready #130013

jonathan-conder · 2024-09-06T02:48:51Z

Currently await is only counted towards coverage if the containing
function is suspended and resumed at least once. This is because it
expands to code which contains a branch on the discriminant of Poll.

By treating it like a branching macro (e.g. assert!), these
implementation details will be hidden from the coverage results.

I added a test to ensure the fix works in simple cases, but the heuristic of picking only the first await-related covspan might be unreliable. I plan on testing more thoroughly with a real codebase over the next couple of weeks.

closes #98712

rustbot · 2024-09-06T02:48:58Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @saethlin (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

@rustbot author: the review is finished, PR author should check the comments and take action accordingly
@rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

rustbot · 2024-09-06T02:48:59Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in coverage tests.

cc @Zalathar

Some changes occurred in coverage instrumentation.

cc @Zalathar

saethlin · 2024-09-06T02:53:22Z

Hm. r? Zalathar

compiler/rustc_mir_transform/src/coverage/spans.rs

compiler/rustc_mir_transform/src/coverage/spans/from_mir.rs

tests/coverage/async.rs

Zalathar · 2024-09-06T04:31:48Z

I added a test to ensure the fix works in simple cases, but the heuristic of picking only the first await-related covspan might be unreliable. I plan on testing more thoroughly with a real codebase over the next couple of weeks.

The code for extracting coverage spans from MIR is already a big pile of unreliable heuristics, so I can't complain too much here. 😅

Currently `await` is only counted towards coverage if the containing function is suspended and resumed at least once. A future commit will fix this and update the test to reflect the new behavior.

Currently `await` is only counted towards coverage if the containing function is suspended and resumed at least once. This is because it expands to code which contains a branch on the discriminant of `Poll`. By treating it like a branching macro (e.g. `assert!`), these implementation details will be hidden from the coverage results.

jonathan-conder · 2024-09-06T05:04:09Z

Ok I think I've addressed everything. If it turns out separate seen_* variables are needed, we can go back to dbe6851.

Zalathar · 2024-09-06T05:23:16Z

Looks good. I have a few more tiny suggestions for the test file headers, but I'm happy to approve this as-is.

(Also, thanks for having a nice commit history and a separate PR for the status-quo test; it really makes my life easier!)

@bors r+

bors · 2024-09-06T05:23:19Z

📌 Commit 25d1830 has been approved by Zalathar

It is now in the queue for this repository.

tests/coverage/await_ready.rs

Zalathar · 2024-09-06T05:49:58Z

This is now in rollup, so better not to modify it any further. The remaining test tweaks can be done in #130017, or some other PR.

jonathan-conder · 2024-09-06T06:16:41Z

Ok. I already rebased those changes so I pushed them to a different branch, here is the diff if you want to add it to your PR: https://github.com/jonathan-conder/rust/compare/await_coverage..await_coverage_tweaks

Thanks that was a really quick turnaround!

…iaskrgr Rollup of 6 pull requests Successful merges: - rust-lang#129021 (Check WF of source type's signature on fn pointer cast) - rust-lang#129781 (Make `./x.py <cmd> compiler/<crate>` aware of the crate's features) - rust-lang#129963 (Inaccurate `{Path,OsStr}::to_string_lossy()` documentation) - rust-lang#129969 (Make `Ty::boxed_ty` return an `Option`) - rust-lang#129995 (Remove wasm32-wasip2's tier 2 status from release notes) - rust-lang#130013 (coverage: Count await when the Future is immediately ready ) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#130013 - jonathan-conder:await_coverage, r=Zalathar coverage: Count await when the Future is immediately ready Currently `await` is only counted towards coverage if the containing function is suspended and resumed at least once. This is because it expands to code which contains a branch on the discriminant of `Poll`. By treating it like a branching macro (e.g. `assert!`), these implementation details will be hidden from the coverage results. I added a test to ensure the fix works in simple cases, but the heuristic of picking only the first await-related covspan might be unreliable. I plan on testing more thoroughly with a real codebase over the next couple of weeks. closes rust-lang#98712

rustbot assigned saethlin Sep 6, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Sep 6, 2024

rustbot assigned Zalathar and unassigned saethlin Sep 6, 2024