-
Notifications
You must be signed in to change notification settings - Fork 12.4k
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
Conversation
r? @cjgillot (rustbot has picked a reviewer for you, use r? to override) |
Some changes occurred to MIR optimizations cc @rust-lang/wg-mir-opt |
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) | ||
} |
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.
This function looks a lot like Span::find_ancestor_inside
. Should it be reused? Or is there a subtle difference?
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 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.
// FIXME(#118525): Consider switching from `original_sp` to `Span::find_ancestor_inside`, | ||
// which is similar but gives slightly different results in some edge cases. |
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 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.
Thanks. |
…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
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
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.
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.
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.
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.
…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.
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.
…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.)
…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.)
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.)
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