(DRAFT) Proof-of-concept for instrumenting the RHS of lazy logical operators

[Zalathar]

I was unsatisfied with some of the MIR building changes proposed by #124402, so I spent some time trying to find a way to achieve the same result in a nicer way.

This PR shows what I was able to come up with. The coverage-side code is a bit more involved, but I think that's a fair tradeoff for the benefit of having a lower impact on the main MIR-building code.

---

In this PR I've currently preserved the original bless commit from #124402, to demonstrate that I was able to get identical coverage reports, and equivalent mappings.

r? @ghost
@rustbot label +A-code-coverage

[Zalathar]

The other advantage of this approach is that it isn't specifically tied to lazy logical operators; it should also be applicable to any other standalone boolean condition that we want to instrument.

[Zalathar]

Now, one obstacle I foresee when expanding this approach to MC/DC coverage is that it might require some extra effort on the coverage side to make sure that these standalone conditions are correctly grouped (or not grouped) as part of a decision.

I think that will require some extra care and attention (and testing), but I don't think it will be a major problem overall.

[Zalathar]

Note that in addition to being less obtrusive on a source-code level, another advantage that this approach has over #122402 is that it's easier to be confident that we haven't changed the lowering or control-flow in a way that would result in language bugs (like things unexpectedly not borrow-checking when coverage is enabled).

(I suspect that this currently isn't an actual problem, but it's a risk that will grow over time as the code continues to evolve and change.)

[RenjiSann]

This indeed is a much less instrusive implementation.
Thank you for working this out !

I will make another PR to make sure MCDC does take in account the rhs, because it is not the case for now, as showed by this example:

    1|       |use std::hint::black_box;
    2|       |
    3|      4|fn bar(b: bool) {
    4|      4|    if b {
  ------------------
  |  Branch (4:8): [True: 3, False: 1]
  ------------------
    5|      3|        black_box("yes");
    6|      3|    } else {
    7|      1|        black_box("no");
    8|      1|    }
    9|      4|}
   10|       |
   11|      4|fn foo(a: bool, b: bool, c: bool) {
   12|      4|    bar(a && b || c);
  ------------------
  |  Branch (12:9): [True: 3, False: 1]
  |  Branch (12:14): [True: 1, False: 2]
  |  Branch (12:19): [True: 2, False: 1]
  ------------------
  |---> MC/DC Decision Region (12:9) to (12:15)
  |
  |  Number of Conditions: 2
  |     Condition C1 --> (12:9)
  |     Condition C2 --> (12:14)
  |
  |  Executed MC/DC Test Vectors:
  |
  |     C1, C2    Result
  |  1 { F,  -  = F      }
  |  2 { T,  F  = F      }
  |  3 { T,  T  = T      }
  |
  |  C1-Pair: covered: (1,3)
  |  C2-Pair: covered: (2,3)
  |  MC/DC Coverage for Decision: 100.00%
  |
  ------------------
   13|      4|}
   14|       |
   15|      1|fn main() {
   16|      1|    foo(true, false, false);
   17|      1|    foo(true, true, true);
   18|      1|    foo(true, false, true);
   19|      1|    foo(false, true, true);
   20|      1|}

[bors]

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

[RenjiSann]

Hi !
Can we rebase and merge this, or shall we wait for #124278 first ?

[Zalathar]

I’ve been busy with other things recently, but I’ll try to take a look at this again and figure out what makes sense.

[Zalathar]

@RenjiSann I was thinking about asking you to take over, but in the end I decided it would be easiest to just do a little bit of polish and get this ready.

If you're happy with my revisions, I might close this and open a fresh PR with the exact same changes.

I think it's fine for this and #124278 to proceed concurrently. In the worst-case scenario where this does cause problems for MC/DC, we can just revert it aggressively (since the whole point is to be a stepping-stone towards MC/DC).

[RenjiSann]

Sounds good to me! I will create a new PR as well to account for the changes for MCDC.

[Zalathar]

Closing this to create a fresh non-draft PR instead.

[Zalathar]

Fresh PR created as #125756.