coverage: Remove the final span-merge pass, and rename `is_closure` to `is_hole`#121433

Zalathar · 2024-02-22T04:47:23Z

This is a combination of semi-related changes that all touch the coverage span-refinement code.

The first significant change is to remove the final span-merging pass that occurs after the main span-refinement code has run. This step used to be essential for getting good coverage mappings, but after other recent changes to span refinement (e.g. #121135 and #121261) it doesn't seem to have much effect any more, so we can simplify the code by removing it.

The other big change is to take code that refers to “closure spans”, and rename it to refer to “hole spans” instead. When checking for the is_closure flag, we don't specifically care whether the span represents a closure; we just want to know whether it's a span that should be carved out of other spans and then discarded.

(Closure spans are currently the only kind of hole span, but in the future might want to add other kinds of hole spans representing nested items or inactive #[cfg(..)] regions.)

@rustbot label +A-code-coverage

rustbot · 2024-02-22T04:47:30Z

r? @pnkfelix

rustbot has assigned @pnkfelix.
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 · 2024-02-22T04:47:32Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Zalathar · 2024-02-22T04:49:25Z

The final merge pass was introduced in #118695, replacing the previous practice of performing a merge on every entry added to the list of refined covspans.

This step used to be essential, but after other recent changes to span refinement it doesn't appear to be particularly necessary. Removing the final merge step will make it easier to simplify or replace the span refinement code.

Now that we don't perform a final merge pass on refined spans, there's no need to create refined spans for closure/hole spans, so we can just discard them immediately.

When refining covspans, we don't specifically care which ones represent closures; we just want to know which ones represent "holes" that should be carved out of other spans and then discarded. (Closures are currently the only source of hole spans, but in the future we might want to also create hole spans for nested items and inactive `#[cfg(..)]` regions.)

Zalathar · 2024-02-23T03:26:19Z

Hmm, thinking about this some more, whatever I replace the span refiner with might want to only have a post-merging step. Perhaps I should experiment with that a bit more before advancing down this particular path.

Zalathar · 2024-02-23T07:02:06Z

@rustbot author

Zalathar · 2024-02-23T08:53:13Z

Since I'm having second thoughts about moving the final span-merge pass, I'll extract the other changes into a new PR.

coverage: Rename `is_closure` to `is_hole` Extracted from rust-lang#121433, since I was having second thoughts about some of the other changes bundled in that PR, but these changes are still fine. --- When refining covspans, we don't specifically care which ones represent closures; we just want to know which ones represent "holes" that should be carved out of other spans and then discarded. (Closures are currently the only source of hole spans, but in the future we might want to also create hole spans for nested items and inactive `#[cfg(..)]` regions.) `@rustbot` label +A-code-coverage

coverage: Rename `is_closure` to `is_hole` Extracted from rust-lang#121433, since I was having second thoughts about some of the other changes bundled in that PR, but these changes are still fine. --- When refining covspans, we don't specifically care which ones represent closures; we just want to know which ones represent "holes" that should be carved out of other spans and then discarded. (Closures are currently the only source of hole spans, but in the future we might want to also create hole spans for nested items and inactive `#[cfg(..)]` regions.) ``@rustbot`` label +A-code-coverage

Rollup merge of rust-lang#121492 - Zalathar:hole, r=fmease coverage: Rename `is_closure` to `is_hole` Extracted from rust-lang#121433, since I was having second thoughts about some of the other changes bundled in that PR, but these changes are still fine. --- When refining covspans, we don't specifically care which ones represent closures; we just want to know which ones represent "holes" that should be carved out of other spans and then discarded. (Closures are currently the only source of hole spans, but in the future we might want to also create hole spans for nested items and inactive `#[cfg(..)]` regions.) ``@rustbot`` label +A-code-coverage

rustbot assigned pnkfelix Feb 22, 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 Feb 22, 2024

rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Feb 22, 2024

Zalathar added 4 commits February 23, 2024 11:03

coverage: Remove some lingering references to pending_dups

4e922eb

coverage: Closure spans don't need to become refined spans

f380bd7

Now that we don't perform a final merge pass on refined spans, there's no need to create refined spans for closure/hole spans, so we can just discard them immediately.

Zalathar force-pushed the hole branch from 5f1d880 to 303c333 Compare February 23, 2024 00:07

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 23, 2024

Zalathar closed this Feb 23, 2024

Zalathar mentioned this pull request Feb 23, 2024

coverage: Rename is_closure to is_hole #121492

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

coverage: Remove the final span-merge pass, and rename `is_closure` to `is_hole`#121433