Draft of branch coverage support

[Zalathar]

EDIT: I've closed this and re-opened a non-draft version at #122322.

This is a work-in-progress snapshot of my implementation of branch coverage.

It works, and produces correct results in many cases. However, it sometimes produces confusing or incorrect results for if expressions that contain the !/&&/|| operators, since the THIR-to-MIR lowering of those is non-trivial.

[Zalathar]

@rustbot label +A-code-coverage

[Zalathar]

@rustbot author

[Zalathar]

Two problems here:

• The span doesn't include !.
• The true/false counts are reversed, since they're tracking cond instead of !cond.

[Zalathar]

This all looks correct, which is nice.

[Zalathar]

This currently doesn't get branch coverage at all. I'd like to fix that if possible, but for me it's a lower priority than getting the logical ops correct.

[Zalathar]

cc @Swatinem in case you're curious.

The MIR building stuff will have to change once I figure out how to handle the logical ops better.

[bors]

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

[Zalathar]

Well, this definitely indicates a need to add macro-expanded spans to the list of things I need to figure out.

The existing coverage code has some functions in spans.rs for dealing with this sort of thing, which I can probably look to for inspiration.

[Zalathar]

I tried passing each of the branch spans through function_source_span. That gets rid of the bogus spans, but it also results in a lot of unhelpful noise for assertions.

[Zalathar]

Now that the || bug is fixed, there are no known correctness issues remaining. The ! mappings are non-ideal, but they're not wrong.

So I'm thinking it might make sense for me to polish the current functionality into a review-ready state, so that people can start playing with branch coverage on nightly and start flushing out any lingering correctness issues.

[Zalathar]

I found a way to fix the mappings for !: pass an extra enclosing_not_expr: Option<&Expr<'tcx>> argument to the recursive call.

We also need to propagate it through the ExprKind::Scope case, but the other recursive cases reset it to None.

[Zalathar]

Big rebase!

I had to resolve some non-trivial merge conflicts, and I also changed the MIR builder code to be specific to branch coverage only, since currently there are no concrete plans to use it for recording non-branch coverage info (though this still remains a future possibility).

[bors]

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

[Zalathar]

The main concern I have with this PR at the moment is that the way it handles ! expressions is pretty fragile. Because of things like #111752, the current MIR building code makes it a little awkward to determine which arm of an if is the true-value arm, and which is the false-value arm.

(The current approach tries to keep track of the outermost enclosing !, and then walks back down the THIR tree counting negations until it hits a non-negation node. This avoids major changes to the existing MIR-building code, and gets the job done, but I don't feel good about it.)

My preferred approach would be to rearrange the MIR-building code in a way that naturally satisfies the needs of branch coverage (and beyond), without adversely affecting non-coverage builds. But that's going to require some care, because this code already has some important and subtle responsibilities.

[Zalathar]

Updated for #121370 (//@ directives in tests).

[Zalathar]

I've adjusted how I handle the issue of || producing incorrect counts (diff).

Instead of sneakily creating an extra block when inserting the block marker, I decided to just fix the issue at its source, by making the || lowering create a new block for the LHS and RHS success paths to converge at.

This adds one extra block to the lowering of || even when coverage is disabled, but that extra block is soon removed by the MIR simplifier.

[Zalathar]

I received some questions about this implementation, which I have answered in a Zulip thread: https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/Branch.20coverage.20questions

[bors]

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

[bors]

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

[bors]

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

[Zalathar]

Switched to a different mechanism for handling ! expressions (diff), which is less intrusive to rustc_mir_build::build::matches, and should be a bit easier to maintain/extend.

[Zalathar]

Now that I've resolved my own concerns, I think it might be time to close this and open a new PR for actual review.