Skip to content
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

coverage: Discard spans that fill the entire function body #121135

Merged
merged 2 commits into from
Feb 17, 2024

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Feb 15, 2024

While debugging some other coverage changes, I discovered a frustrating inconsistency that occurs in functions containing closures, if they end with an implicit () return instead of an explicit trailing-expression.

This turns out to have been caused by the corresponding node in MIR having a span that covers the entire function body. When preparing coverage spans, any span that fills the whole body tends to cause more harm than good, so this PR detects and discards those spans.

(This isn't the first time whole-body spans have caused problems; we also eliminated some of them in #118525.)

@rustbot
Copy link
Collaborator

rustbot commented Feb 15, 2024

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot 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 Feb 15, 2024
@rustbot
Copy link
Collaborator

rustbot commented Feb 15, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

@rustbot rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Feb 15, 2024
When we try to extract coverage-relevant spans from MIR, sometimes we see MIR
statements/terminators whose spans cover the entire function body. Those spans
tend to be unhelpful for coverage purposes, because they often represent
compiler-inserted code, e.g. the implicit return value of `()`.
Comment on lines +137 to +138
// Discard any spans that fill the entire body, because they tend
// to represent compiler-inserted code, e.g. implicitly returning `()`.
Copy link
Member

Choose a reason for hiding this comment

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

For some cases like fn foo() { }, I wonder if this heuristic will trigger incorrectly? Since all the changes in the tests are for the better, I'm not super worried about that.

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 thought about adding a special-case to detect functions with a non-empty body span and no extracted spans, in order to explicitly add a span that covers the whole body. But currently it doesn't seem to be necessary, so I haven't bothered.

For fn foo() {}, I believe the way it currently works is that we generate a span for the signature, and there's another zero-width span right at the end of the body (representing the return), and the span refiner then merges those together into something that covers the whole body.

(None of this is ideal, but sadly that's the reality of trying to heuristically extract coverage spans from MIR. Right now I'm happy to be able to make any progress in simplifying the span refiner into something a bit more maintainable.)

@wesleywiser
Copy link
Member

Thanks @Zalathar!

@bors r+

@bors
Copy link
Contributor

bors commented Feb 16, 2024

📌 Commit cd9021e has been approved by wesleywiser

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 16, 2024
@fmease fmease assigned wesleywiser and unassigned fmease Feb 16, 2024
Nadrieril added a commit to Nadrieril/rust that referenced this pull request Feb 17, 2024
…leywiser

coverage: Discard spans that fill the entire function body

While debugging some other coverage changes, I discovered a frustrating inconsistency that occurs in functions containing closures, if they end with an implicit `()` return instead of an explicit trailing-expression.

This turns out to have been caused by the corresponding node in MIR having a span that covers the entire function body. When preparing coverage spans, any span that fills the whole body tends to cause more harm than good, so this PR detects and discards those spans.

(This isn't the first time whole-body spans have caused problems; we also eliminated some of them in rust-lang#118525.)
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2024
Rollup of 8 pull requests

Successful merges:

 - rust-lang#119032 (Use a hardcoded constant instead of calling OpenProcessToken.)
 - rust-lang#120932 (const_mut_refs: allow mutable pointers to statics)
 - rust-lang#121059 (Add and use a simple extension trait derive macro in the compiler)
 - rust-lang#121135 (coverage: Discard spans that fill the entire function body)
 - rust-lang#121187 (Add examples to document the return type of quickselect functions)
 - rust-lang#121191 (Add myself to review rotation (and a rustbot ping))
 - rust-lang#121192 (Give some intrinsics fallback bodies)
 - rust-lang#121197 (Ensure `./configure` works when `configure.py` path contains spaces)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit c7701ba into rust-lang:master Feb 17, 2024
11 checks passed
@rustbot rustbot added this to the 1.78.0 milestone Feb 17, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Feb 17, 2024
Rollup merge of rust-lang#121135 - Zalathar:no-whole-body-span, r=wesleywiser

coverage: Discard spans that fill the entire function body

While debugging some other coverage changes, I discovered a frustrating inconsistency that occurs in functions containing closures, if they end with an implicit `()` return instead of an explicit trailing-expression.

This turns out to have been caused by the corresponding node in MIR having a span that covers the entire function body. When preparing coverage spans, any span that fills the whole body tends to cause more harm than good, so this PR detects and discards those spans.

(This isn't the first time whole-body spans have caused problems; we also eliminated some of them in rust-lang#118525.)
@Zalathar Zalathar deleted the no-whole-body-span branch February 17, 2024 13:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants