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: Skip spans that can't be un-expanded back to the function body #118525

Merged
merged 1 commit into from
Dec 3, 2023

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Dec 2, 2023

When we extract coverage spans from MIR, we try to "un-expand" them back to spans that are inside the function's body span.

In cases where that doesn't succeed, the current code just swaps in the entire body span instead. But that tends to result in coverage spans that are completely unrelated to the control flow of the affected code, so it's better to just discard those spans.


Extracted from #118305, since this is a general improvement that isn't specific to branch coverage.


@rustbot label +A-code-coverage

@rustbot
Copy link
Collaborator

rustbot commented Dec 2, 2023

r? @cjgillot

(rustbot has picked a reviewer for you, use r? to override)

@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 Dec 2, 2023
@rustbot
Copy link
Collaborator

rustbot commented Dec 2, 2023

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 Dec 2, 2023
use rustc_span::source_map::original_sp;

let original_span = original_sp(span, body_span).with_ctxt(body_span.ctxt());
if body_span.contains(original_span) { original_span } else { body_span }
body_span.contains(original_span).then_some(original_span)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

This function looks a lot like Span::find_ancestor_inside. Should it be reused? Or is there a subtle difference?

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 was mostly just modifying the existing code, and didn't know about Span::find_ancestor_inside as a potential option.

I tried switching over, and the results are mostly the same, with a few small differences in some corner cases.

At a glance the changes seem like they might be improvements, but I'm not confident in that conclusion. What I might do is stick with the status quo (original_sp) for now, but leave a FIXME to remind me to investigate switching over as a separate PR.

When we extract coverage spans from MIR, we try to "un-expand" them back to
spans that are inside the function's body span.

In cases where that doesn't succeed, the current code just swaps in the entire
body span instead. But that tends to result in coverage spans that are
completely unrelated to the control flow of the affected code, so it's better
to just discard those spans.
Comment on lines +191 to +192
// FIXME(#118525): Consider switching from `original_sp` to `Span::find_ancestor_inside`,
// which is similar but gives slightly different results in some edge cases.
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 wasn't comfortable with actually switching to Span::find_ancestor_inside as part of this PR, since I don't quite understand the subtle differences from the status quo.

So instead I'm leaving this note to indicate that it might be worth looking into.

@cjgillot
Copy link
Contributor

cjgillot commented Dec 3, 2023

Thanks.
@bors r+ rollup

@bors
Copy link
Contributor

bors commented Dec 3, 2023

📌 Commit eb2d4cb has been approved by cjgillot

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 Dec 3, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 3, 2023
…iaskrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#117869 ([rustdoc] Add highlighting for comments in items declaration)
 - rust-lang#118525 (coverage: Skip spans that can't be un-expanded back to the function body)
 - rust-lang#118574 (rustc_session: Address all `rustc::potential_query_instability` lints)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 80c94e8 into rust-lang:master Dec 3, 2023
11 checks passed
@rustbot rustbot added this to the 1.76.0 milestone Dec 3, 2023
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 3, 2023
Rollup merge of rust-lang#118525 - Zalathar:skip-spans, r=cjgillot

coverage: Skip spans that can't be un-expanded back to the function body

When we extract coverage spans from MIR, we try to "un-expand" them back to spans that are inside the function's body span.

In cases where that doesn't succeed, the current code just swaps in the entire body span instead. But that tends to result in coverage spans that are completely unrelated to the control flow of the affected code, so it's better to just discard those spans.

---

Extracted from rust-lang#118305, since this is a general improvement that isn't specific to branch coverage.

---

`@rustbot` label +A-code-coverage
@Zalathar Zalathar deleted the skip-spans branch December 3, 2023 23:01
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Dec 9, 2023
coverage: Simplify the heuristic for ignoring `async fn` return spans

The code for extracting coverage spans from MIR has a special heuristic for dealing with `async fn`, so that the function's closing brace does not have a confusing double count.

The code implementing that heuristic is currently mixed in with the code for flushing remaining spans after the main refinement loop, making the refinement code harder to understand.

We can solve that by hoisting the heuristic to an earlier stage, after the spans have been extracted and sorted but before they have been processed by the refinement loop.

The coverage tests verify that the heuristic is still effective, so coverage mappings/reports for `async fn` have not changed.

---

This PR also has the side-effect of fixing the `None some_prev` panic that started appearing after rust-lang#118525.

The old code assumed that `prev` would always be present after the refinement loop. That was only true if the list of collected spans was non-empty, but prior to rust-lang#118525 that didn't seem to come up in practice. After that change, the list of collected spans could be empty in some specific circumstances, leading to panics.

The new code uses an `if let` to inspect `prev`, which correctly does nothing if there is no span present.
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Dec 9, 2023
coverage: Simplify the heuristic for ignoring `async fn` return spans

The code for extracting coverage spans from MIR has a special heuristic for dealing with `async fn`, so that the function's closing brace does not have a confusing double count.

The code implementing that heuristic is currently mixed in with the code for flushing remaining spans after the main refinement loop, making the refinement code harder to understand.

We can solve that by hoisting the heuristic to an earlier stage, after the spans have been extracted and sorted but before they have been processed by the refinement loop.

The coverage tests verify that the heuristic is still effective, so coverage mappings/reports for `async fn` have not changed.

---

This PR also has the side-effect of fixing the `None some_prev` panic that started appearing after rust-lang#118525.

The old code assumed that `prev` would always be present after the refinement loop. That was only true if the list of collected spans was non-empty, but prior to rust-lang#118525 that didn't seem to come up in practice. After that change, the list of collected spans could be empty in some specific circumstances, leading to panics.

The new code uses an `if let` to inspect `prev`, which correctly does nothing if there is no span present.
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Dec 9, 2023
coverage: Simplify the heuristic for ignoring `async fn` return spans

The code for extracting coverage spans from MIR has a special heuristic for dealing with `async fn`, so that the function's closing brace does not have a confusing double count.

The code implementing that heuristic is currently mixed in with the code for flushing remaining spans after the main refinement loop, making the refinement code harder to understand.

We can solve that by hoisting the heuristic to an earlier stage, after the spans have been extracted and sorted but before they have been processed by the refinement loop.

The coverage tests verify that the heuristic is still effective, so coverage mappings/reports for `async fn` have not changed.

---

This PR also has the side-effect of fixing the `None some_prev` panic that started appearing after rust-lang#118525.

The old code assumed that `prev` would always be present after the refinement loop. That was only true if the list of collected spans was non-empty, but prior to rust-lang#118525 that didn't seem to come up in practice. After that change, the list of collected spans could be empty in some specific circumstances, leading to panics.

The new code uses an `if let` to inspect `prev`, which correctly does nothing if there is no span present.
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Dec 9, 2023
Rollup merge of rust-lang#118666 - Zalathar:body-closure, r=cjgillot

coverage: Simplify the heuristic for ignoring `async fn` return spans

The code for extracting coverage spans from MIR has a special heuristic for dealing with `async fn`, so that the function's closing brace does not have a confusing double count.

The code implementing that heuristic is currently mixed in with the code for flushing remaining spans after the main refinement loop, making the refinement code harder to understand.

We can solve that by hoisting the heuristic to an earlier stage, after the spans have been extracted and sorted but before they have been processed by the refinement loop.

The coverage tests verify that the heuristic is still effective, so coverage mappings/reports for `async fn` have not changed.

---

This PR also has the side-effect of fixing the `None some_prev` panic that started appearing after rust-lang#118525.

The old code assumed that `prev` would always be present after the refinement loop. That was only true if the list of collected spans was non-empty, but prior to rust-lang#118525 that didn't seem to come up in practice. After that change, the list of collected spans could be empty in some specific circumstances, leading to panics.

The new code uses an `if let` to inspect `prev`, which correctly does nothing if there is no span present.
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 27, 2023
…nkov

coverage: Unexpand spans with `find_ancestor_inside_same_ctxt`

Back in rust-lang#118525 (comment) it was observed that our `unexpand_into_body_span` now looks very similar to `Span::find_ancestor_inside`.

At the time I tried switching over, but doing so resulted in incorrect coverage mappings (or assertion failures), so I left a `FIXME` comment instead.

After some investigation, I identified the two problems with my original approach:
- I should have been using `find_ancestor_inside_same_ctxt` instead, since we want a span that's inside the body and has the same context as the body.
- For async functions, we were actually using the post-expansion body span, which is why we needed to forcibly set the unexpanded span's context to match the body span. For body spans produced by macro-expansion, we already have special-case code to detect this and use the pre-expansion call site as the body span. By making this code also detect async desugaring, I was able to end up with a body span that works properly with `find_ancestor_inside_same_ctxt`, avoiding the need to forcibly change the span context.
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 28, 2023
coverage: Unexpand spans with `find_ancestor_inside_same_ctxt`

Back in rust-lang#118525 (comment) it was observed that our `unexpand_into_body_span` now looks very similar to `Span::find_ancestor_inside`.

At the time I tried switching over, but doing so resulted in incorrect coverage mappings (or assertion failures), so I left a `FIXME` comment instead.

After some investigation, I identified the two problems with my original approach:
- I should have been using `find_ancestor_inside_same_ctxt` instead, since we want a span that's inside the body and has the same context as the body.
- For async functions, we were actually using the post-expansion body span, which is why we needed to forcibly set the unexpanded span's context to match the body span. For body spans produced by macro-expansion, we already have special-case code to detect this and use the pre-expansion call site as the body span. By making this code also detect async desugaring, I was able to end up with a body span that works properly with `find_ancestor_inside_same_ctxt`, avoiding the need to forcibly change the span context.
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.)
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.)
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.)
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.

5 participants