Known limitations of branch coverage instrumentation

[Zalathar]

Branch coverage is known to support these branching language constructs:

• if and while
  • Includes if let and let-chains
  • Treats nested || and && as separate branches
  • while desugars to if before MIR building, so we support it as a consequence of supporting if
• Standalone || and && expressions
  • Nested || and && are treated as separate branches
  • The final operand is not a branch, but can be optionally treated as one via #125756
• let-else statements
• Match arm guards
  • These also desugar to if expressions before MIR building
  • This also means that we naturally support if let guards
  • But their presence makes it more complicated to instrument match arms, as described below

Branch coverage is known to not yet support these branching language constructs:

• [ ] Individual arms and or-patterns in match expressions
  • We need to be sure to support (or gracefully not support) all four combinations of:
  • Arms with/without guards
    • The guards themselves are already supported (as indicated above), but their presence also complicates how we instrument the enclosing match arm, and other match arms in the same match
    • Note that the combination of guards and or-patterns results in particularly complex control-flow, because if a guard fails then we keep searching for other or-pattern matches in the same arm
  • Arms with/without or-patterns, including arbitrary nesting

  • 124154

  • This draft does not support or-patterns, and might not be able to support them without substantial changes (requiring a different approach)
  • This draft uses complex counter expressions to work around not being able to instrument all the appropriate points in the control-flow graph directly; this ties back into concerns about or-patterns
• [ ] The try ? operator (which conditionally returns)
  • This is desugared to a match expression, so once we support those, supporting ? should mostly be a matter of expansion-span bookkeeping
• [ ] .await expressions (which implicitly “return” when the enclosing future is dropped)
  • This can probably be excluded from any MVP of branch coverage, unless it turns out to be unexpectedly easy after supporting ?
  • But some users will presumably want this in the long run
• [ ] Any of the above branching constructs that are introduced by macro expansion
  • The current implementation discards any branch span that isn't “directly visible” in the function body

---

Relevant PR links:

• 122322

• 124223

• 124154

(Rewritten on 2024-07-11; see edit history for the old text.)

[Zalathar]

Based on my investigations so far:

• If-let (and friends) and let-else are relatively easy to support. I have implementations of them, though I haven't filed PRs yet because I'm wary of making any changes that will accidentally make the other branching constructs harder to support.
• Match arms are tricky to support at the moment. Conceptually, it makes sense to treat each arm (except the last) as a branch, with the true arm leading to the guard check (if any) followed by the corresponding body, and the false arm leading to the test for the next arm. However, that control-flow structure doesn't necessarily exist in the lowering from THIR to MIR, so we don't always have a suitable place to insert block markers.
  • I have a sketch of a plan for how to deal with this, by inserting block markers in all the true arms, and then using coverage expressions to create synthetic counts for the false arms. However, this will require some non-trivial reshuffling of how the InstrumentCoverage MIR pass currently deals with branch coverage information.
• The try operator desugars to a match expression, so if we support those, then we should be close to supporting try as well.
  • The caveat here is that we currently ignore all branches that come from expansion of any kind, so once we support match arms, we'll need to deal with that limitation as well.
• Await seems complicated enough that I'm inclined to put it in the too-hard basket for now.

[Zalathar]

There's also some discussion at https://github.com/rust-lang/rust/issues/79649#issuecomment-2061486639 about whether the right-hand-side of a lazy boolean expression (&&/||) should be treated as a “branch” condition, even when it's not part of an if condition.

My view is that “branch coverage” should not be in the business of instrumenting things that clearly aren't branches at the language level, at least not by default. There are valid use-cases for that behaviour, but if it exists then it should require some additional level of opt-in beyond just enabling branch coverage.

(If people want to discuss this point further, I would suggest creating a separate issue for that discussion.)

[Lambdaris]

Is there a draft folk for if-let pattern?

I have tried a bit and found a possibly feasible way to insert block markers, see a naive draft.

Because if let would be taken as match guard this method may solve them together (not sure, I just check it could find the true/false blocks for code like if let (En::A(Some(a)) | En::B(a), En::C(b)) = _.

By this then BranchSpan might need to change its true_marker and false_marker to Vec<BlockMarkerId>.

[Zalathar]

I managed to get my match-arm idea working, so I'll try to tie up the loose ends and post it as a PR.

[Zalathar]

124154 shows my current approach to instrumenting match arms for branch coverage.

[Zalathar]

I now have draft PRs for if-let, let-else, and match arms (without or-patterns).

[ZhuUx]

Noticed the match arms pay attention to occasion like comment at first line of make_expression in counters.rs, I think it may be better put a note here rather reviews of the current two prs.

Many redundant expressions like this may be introduced by match arms. The root cause is rustc may generate several test blocks for certain pattern. For example, code (A | B, C | D) will generate CFG like code

if match A {
    if match C {
        // Assume here is BcbCounter 1
    }  else if match D {
        // ...
    }
} else if match B {
    if match C {
        // Assume here is BcbCounter 2
    }  else if match D {
        // ...
    }
}

The true_term of C apparently should be BcbCounter 1 + BcbCounter 2. Pattern (A | B, C | D, E | F) even make terms of E more sophisticated. Actually they are just composition of several BcbCounter::Counter thus there probably many of intermediate expressions can be optimized out.

However, given that Expressions does not have runtime overhead the main side effect of these redundant expressions might only make people who read mir directly distressed. Further if we wanted to minimize the number of expressions, probably do it in llvm is better.

[ZhuUx]

The true_term of C apparently should be BcbCounter 1 + BcbCounter 2. Pattern (A | B, C | D, E | F) even make terms of E more sophisticated. Actually they are just composition of several BcbCounter::Counter thus there probably many of intermediate expressions can be optimized out.

This might not be "apparent". We also can view C as several branches sharing one span. By this view most work could be simplified but users would be confused with many branches in llvm-cov reports.

[Zalathar]

I've rewritten the issue description to give a more up-to-date summary of the status quo.