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: Treat each match arm as a "branch" for branch coverage #124154

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Zalathar
Copy link
Contributor

@Zalathar Zalathar commented Apr 19, 2024

This PR adds branch coverage instrumentation for individual arms in match expressions.

It seems to work well in most cases. However, the current implementation cannot handle match expressions containing or-patterns |, and therefore ignores such match expressions entirely.

(More specifically, it ignores a match expression if any of its Candidates has pre_binding_block = None.)


Right now I'm not sure whether this should be merged as-is (to provide a useful subset of functionality, and avoid bit-rot), or should be left as a draft until the or-pattern limitation can be lifted (could potentially require major rework).

@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. A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) labels Apr 19, 2024
Copy link
Contributor

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

lgtm, the added tests really speak for themselves 🎉
left a small style nit

compiler/rustc_mir_transform/src/coverage/mod.rs Outdated Show resolved Hide resolved
@Zalathar
Copy link
Contributor Author

Some small style updates (diff).

@Zalathar
Copy link
Contributor Author

I noticed that I accidentally included my let-else test as well, but instead of removing it, I decided to just add my if-let test as well. Both of these test show that the respective features don't support branch coverage yet.

(diff)

@Zalathar
Copy link
Contributor Author

Discard match expressions early if they have <2 arms (diff).

@Zalathar Zalathar force-pushed the branch-arms branch 2 times, most recently from 59c2385 to 356427f Compare April 20, 2024 02:30
@Zalathar
Copy link
Contributor Author

After investigating some more, I realised that my scheme for making the false-branch expressions isn't accounting for the actual control flow around match guards.

For matches with no guards, the false-count for any arm is just the sum of the execution counts of all subsequent arms.

But for an arm with a guard, we also need to subtract the number of times that the match succeeded but the guard failed. We're currently failing to do that, which is why we get inflated counts, which then mess up all earlier arms too.

@Zalathar
Copy link
Contributor Author

For now I've updated the implementation to skip any match expressions that have guards, so that at least we aren't producing wrong branch-coverage numbers for them (diff).

@Zalathar
Copy link
Contributor Author

More style updates (diff).

@Zalathar
Copy link
Contributor Author

It looks like the point where we know an arm's pre-binding block and its guard-succeeded block is just after the call to bind_pattern in lower_match_arms.

Is there a good way to forward this information to BranchInfoBuilder?

@bors
Copy link
Contributor

bors commented Apr 20, 2024

☔ The latest upstream changes (presumably #124194) made this pull request unmergeable. Please resolve the merge conflicts.

@Zalathar
Copy link
Contributor Author

Rebasing this on top of #123409 turned out to be very difficult (due to how the branch and MC/DC code have become interdependent), so I've filed #124217 to help relieve that tension, separately from the branch coverage changes that will be built on top of it.

@Zalathar Zalathar force-pushed the branch-arms branch 4 times, most recently from d0dfcd8 to bc042ac Compare April 21, 2024 09:02
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Apr 23, 2024
Rollup merge of rust-lang#124217 - Zalathar:pre-branch, r=oli-obk

coverage: Prepare for improved branch coverage

When trying to rebase my new branch coverage work (including rust-lang#124154) on top of the introduction of MC/DC coverage (rust-lang#123409), I found it a lot harder than anticipated. With the benefit of hindsight, the branch coverage code and MC/DC code have become more interdependent than I'm happy with.

This PR therefore disentangles them a bit, so that it will be easier for both areas of code to evolve independently without interference.

---

This PR also includes a few extra branch coverage tests that I had sitting around from my current branch coverage work. They mostly just demonstrate that certain language constructs listed in rust-lang#124118 currently don't have branch coverage support.

``@rustbot`` label +A-code-coverage
LL| |
LL| 1|fn never_taken() {
LL| 1| match black_box(false) {
LL| 1| _ if black_box(false) => {}
Copy link
Contributor

Choose a reason for hiding this comment

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

I am wondering why the pattern _ and the guard black_box(false) are in the same code region, while there are 2 branch expressions.

I would have expected the guard to have its own counter displayed underneath

   LL|      1|        _ if black_box(false) => {}
                           ^1                  ^0

@bors
Copy link
Contributor

bors commented Apr 30, 2024

☔ The latest upstream changes (presumably #124399) made this pull request unmergeable. Please resolve the merge conflicts.

@Zalathar
Copy link
Contributor Author

Brushed some of the cobwebs off this PR.

I still don't want to proceed without a plan for how to handle or-patterns, but at least this implementation is now a more up-to-date reference.

Within the `InstrumentCoverage` pass, we now represent branches as a list of
arms, instead of a true/false pair, until we prepare the final table of
mappings to be attached to the MIR body.

(We then flatten the list into two-way branches by treating each arm as a
branch between its success block, and the total of all later arms.)

Currently all of the branches produced by MIR building are still two-way, but
this is a step towards allowing many-way branches.
@bors
Copy link
Contributor

bors commented Jun 30, 2024

☔ The latest upstream changes (presumably #127174) made this pull request unmergeable. Please resolve the merge conflicts.

@Nadrieril
Copy link
Member

#127159 should make your life easier.

@Zalathar
Copy link
Contributor Author

Zalathar commented Jul 1, 2024

I also want to add a link here to https://rust-lang.zulipchat.com/#narrow/stream/213817-t-lang/topic/Details.20of.20match.20guard.20semantics/near/448161106, from which I learned that the interaction between or-patterns and guards is more complex than I had thought, because the guard might need to be evaluated multiple times per match in the general case.

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 15, 2024
coverage: Restrict `ExpressionUsed` simplification to `Code` mappings

In the future, branch and MC/DC mappings might have expressions that don't correspond to any single point in the control-flow graph. That makes it trickier to keep track of which expressions should expect an `ExpressionUsed` node.

We therefore sidestep that complexity by only performing `ExpressionUsed` simplification for expressions associated directly with ordinary `Code` mappings.

(This simplification step is inherited from the original coverage implementation, which only supported `Code` mappings anyway, so there's no particular reason to extend it to other kinds of mappings unless we specifically choose to.)

Relevant to:
- rust-lang#124154
- rust-lang#126677
- rust-lang#124278

`@rustbot` label +A-code-coverage
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 15, 2024
coverage: Restrict `ExpressionUsed` simplification to `Code` mappings

In the future, branch and MC/DC mappings might have expressions that don't correspond to any single point in the control-flow graph. That makes it trickier to keep track of which expressions should expect an `ExpressionUsed` node.

We therefore sidestep that complexity by only performing `ExpressionUsed` simplification for expressions associated directly with ordinary `Code` mappings.

(This simplification step is inherited from the original coverage implementation, which only supported `Code` mappings anyway, so there's no particular reason to extend it to other kinds of mappings unless we specifically choose to.)

Relevant to:
- rust-lang#124154
- rust-lang#126677
- rust-lang#124278

``@rustbot`` label +A-code-coverage
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 15, 2024
coverage: Restrict `ExpressionUsed` simplification to `Code` mappings

In the future, branch and MC/DC mappings might have expressions that don't correspond to any single point in the control-flow graph. That makes it trickier to keep track of which expressions should expect an `ExpressionUsed` node.

We therefore sidestep that complexity by only performing `ExpressionUsed` simplification for expressions associated directly with ordinary `Code` mappings.

(This simplification step is inherited from the original coverage implementation, which only supported `Code` mappings anyway, so there's no particular reason to extend it to other kinds of mappings unless we specifically choose to.)

Relevant to:
- rust-lang#124154
- rust-lang#126677
- rust-lang#124278

```@rustbot``` label +A-code-coverage
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 15, 2024
Rollup merge of rust-lang#127758 - Zalathar:expression-used, r=oli-obk

coverage: Restrict `ExpressionUsed` simplification to `Code` mappings

In the future, branch and MC/DC mappings might have expressions that don't correspond to any single point in the control-flow graph. That makes it trickier to keep track of which expressions should expect an `ExpressionUsed` node.

We therefore sidestep that complexity by only performing `ExpressionUsed` simplification for expressions associated directly with ordinary `Code` mappings.

(This simplification step is inherited from the original coverage implementation, which only supported `Code` mappings anyway, so there's no particular reason to extend it to other kinds of mappings unless we specifically choose to.)

Relevant to:
- rust-lang#124154
- rust-lang#126677
- rust-lang#124278

```@rustbot``` label +A-code-coverage
@Dylan-DPC
Copy link
Member

@Zalathar any updates on this? thanks

@Dylan-DPC Dylan-DPC 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 Oct 20, 2024
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-author Status: This is awaiting some action (such as code changes or more information) from the author. 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.

8 participants