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: Support match statements in branch coverage #130744

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

ranger-ross
Copy link
Contributor

@ranger-ross ranger-ross commented Sep 23, 2024

This PR builds on the work original done in #124154. (Also see #124118)

In addition, to supporting match arms this PR also attempts to support or-patterns in match patterns by tracking subbranches.
The current implementation handles the basics but nested or-patterns+match guards still have some issues. I figured I would open a PR with what I have so far to get feedback and to see if I am even headed in the right direction.

r? Zalathar


This is my first non-trivial PR for the compiler so apologies if I am doing something wildly incorrect.

@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 Sep 23, 2024
@rust-log-analyzer

This comment has been minimized.

@ranger-ross
Copy link
Contributor Author

@rustbot label +A-code-coverage

@rustbot rustbot added the A-code-coverage Area: Source-based code coverage (-Cinstrument-coverage) label Sep 23, 2024
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@ranger-ross ranger-ross marked this pull request as ready for review September 29, 2024 10:25
@rustbot
Copy link
Collaborator

rustbot commented Sep 29, 2024

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in coverage instrumentation.

cc @Zalathar

Some changes occurred in match lowering

cc @Nadrieril

Some changes occurred in coverage instrumentation.

cc @Zalathar

Some changes occurred in coverage instrumentation.

cc @Zalathar

Some changes occurred in coverage instrumentation.

cc @Zalathar

Some changes occurred in coverage tests.

cc @Zalathar

@ranger-ross
Copy link
Contributor Author

Note: There is still a bug with nested or-patterns + match guards being used together.

ie. Branch coverage does not appear to work correctly on this code:

let a = 1;
let b = 2;
match black_box((a, b)) {
    (1, 2 | 3) | (2, 4) if b > 1 => {}
    _ => {}
}

tbh, I am not super sure what the correct behavior for coverage for nested or-patterns.

@@ -1840,7 +1871,10 @@ impl<'a, 'tcx> Builder<'a, 'tcx> {
});
for candidate in candidates_to_expand.iter_mut() {
if !candidate.subcandidates.is_empty() {
self.merge_trivial_subcandidates(candidate);
// FIXME: Support merging trival candidates in branch coverage instrumentation
Copy link
Member

Choose a reason for hiding this comment

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

This means that turning on branch coverage can cause exponential blowups in MIR size, this is unfortunate. Are you sure tracking or-patterns is worth it? (I'm not expert here, just curious)

@Zalathar
Copy link
Contributor

Fair warning that it may take me a while to get around to properly looking at this.

(I also have some general concerns about expanding branch coverage with the rest of the coverage code in its present state, though I haven't looked at this PR closely enough to know whether those concerns apply here.)

@ranger-ross
Copy link
Contributor Author

No worries, and yeah I was also not in love with the implementation 😅

After opening this PR, I am starting question what the correct behavior is.

I think an argument can be made that a pattern should be considered a single branch. (Either it matches or it doesn't)

But on the other hand I can see value in knowing if the rhs of an or-pattern was never matched.

@Nadrieril
Copy link
Member

I'd say if you want to be sure all the parts of your pattern are useful, you can use MC/DC (when we manage to implement it)

@ranger-ross
Copy link
Contributor Author

ranger-ross commented Oct 5, 2024

I'd say if you want to be sure all the parts of your pattern are useful, you can use MC/DC (when we manage to implement it)

Thats true, I am starting to lean towards patterns being considered a single branch in branch coverage.
Like you mentioned, if someone really needs in depth pattern coverage they can just use MC/DC.

@bors
Copy link
Contributor

bors commented Oct 7, 2024

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

@sassman
Copy link

sassman commented Nov 5, 2024

Is there actually a (Pre-) RFC for the whole branch coverage feature, just to understand the expected and defined behavior better?

I'm asking because I found this issue taiki-e/cargo-llvm-cov#394 that indeed shows an interesting corner case for branch coverage and generics where it's a bit unintuitive what is the right thing to expect to see.

@ranger-ross
Copy link
Contributor Author

Is there actually a (Pre-) RFC for the whole branch coverage feature, just to understand the expected and defined behavior better?

I did a search for "coverage" in the rust-lang/rfcs repo and found very little about coverage. (and nothing about branch coverage)

After a bit of searching, the origin of the branch coverage feature was merged in #122322 .
We probably need to more formally define the expected behavior. As you mentioned, the correct behavior is sometimes not obvious.

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-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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants