@rustbot label +A-code-coverage

@rustbot author

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.

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

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.

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.

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.

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.

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).

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

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.

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

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.

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

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

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

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

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.

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.