Question on out-edge instrumentation of if-then-else and match cases

[mbarbin]

I am currently integrating bisect_ppx into a code base to monitor coverage as I
add more tests. During this process, I've noticed some instances where
bisect_ppx reports lines as uncovered, even though I believe they should not
be. I'm hoping to gain a better understanding of the instrumentation strategy
and consider whether a slight modification to the instrumentation heuristic
might be warranted.

The specific scenario I'm focusing on involves the out-edge instrumentation of
if-then-else and match cases, which I understand is handled by the
___bisect_post_visit___ construct.

In the bisect_ppx tests, I found these examples:

test/instrumentation/control/if.t:

  let _ =
    if bool_of_string "true" then (
      ___bisect_visit___ 3;
      ___bisect_post_visit___ 0 (print_endline "foo"))
    else (
      ___bisect_visit___ 2;
      ___bisect_post_visit___ 1 (print_endline "bar"))
  
  let _ =
   fun () ->
    ___bisect_visit___ 6;
    if bool_of_string "true" then (
      ___bisect_visit___ 5;
      print_endline "foo")
    else (
      ___bisect_visit___ 4;
      print_endline "bar")

test/instrumentation/control/match.t:

 let _ =
    match print_endline "foo" with
    | () ->
        ___bisect_visit___ 1;
        ___bisect_post_visit___ 0 (print_endline "bar")
  
  let _ =
   fun () ->
    ___bisect_visit___ 3;
    match print_endline "foo" with
    | () ->
        ___bisect_visit___ 2;
        print_endline "bar"

Both examples include a case where the branches are post-instrumented (shown
first) and a case where they are not (shown second).

Could we discuss examples where post-instrumentation is beneficial in these
scenarios? I'm unsure. When the expression in the case is a Pexp_apply, and
the function raises an exception, I don't believe this should be considered as
uncovered.

The specific case that led me to investigate this is as follows¹:

src/status_line/ml:

let create ~size ~code =
  if code < 0 then raise_s [%sexp "invalid negative code", { size : int; code : int }];
  (* Do more stuff below *)

I examined the instrumented code using dune describe:

$ dune describe pp my_file.ml --instrument-with bisect_ppx

which shows:

let create ~size  ~code  =
  ___bisect_visit___ 31;
  if code < 0
  then
    (___bisect_visit___ 30;
     ___bisect_post_visit___ 29
       (raise_s
          (Ppx_sexp_conv_lib.Sexp.List
             [Ppx_sexp_conv_lib.Conv.sexp_of_string "invalid negative code";
             Ppx_sexp_conv_lib.Sexp.List
               [Ppx_sexp_conv_lib.Sexp.List
                  [Ppx_sexp_conv_lib.Sexp.Atom "size";
                  ((sexp_of_int)[@merlin.hide ]) size];
               Ppx_sexp_conv_lib.Sexp.List
                 [Ppx_sexp_conv_lib.Sexp.Atom "code";
                 ((sexp_of_int)[@merlin.hide ]) code]]])));

From my understanding, this code will never visit the point 29, by design.
This matches the misses that I see in the report².

Tangentially, I noticed that there's special handling for the raise function,
which is identified as a trivial_function. If the function was raise instead
of raise_s, this would resolve the issue. However, before suggesting that
raise_s be added to the trivial list, I'd like to understand the strategy more
thoroughly.

Based on the information I currently have, I'm inclined to think that when an
expression in a then, else, or match case is a Pexp_apply, it is already
pre-instrumented, and thus does not need to be post-instrumented.

I would appreciate your thoughts on this matter. Thank you!

Environment

dune 3.13.1, bisect_ppx 2.8.3, ocaml 5.1.1.

Footnotes

1. https://github.com/mbarbin/catch-the-bunny/tree/main ↩

2. https://coveralls.io/builds/65621176/source?filename=src%2Fstatus_line.ml ↩

[aantron]

Both examples include a case where the branches are post-instrumented (shown
first) and a case where they are not (shown second).

Bisect does not post-instrument tail calls, because that would break tail call optimization, which is critical for many functional prorgams. In the cases shown first, the Pexp_applys are not in tail position, because they are not inside a function.

Based on the information I currently have, I'm inclined to think that when an
expression in a then, else, or match case is a Pexp_apply, it is already
pre-instrumented, and thus does not need to be post-instrumented.

The post-instrumentation shows whether the function completes evaluation normally, so it is separate from the branch instrumentation, which shows whether the branch is entered.

I believe in your case you should suppress instrumentation of the out-edge with [@coverage off], though I don't immediately recall whether the way that is implemented will also cause the branch instrumentation to also be supressed, which would probably not be right for this code.

[mbarbin]

Thank you for the explanation. I now understand how post-instrumentation
contributes to coverage checking when the returned value of an apply is embedded
in larger expressions, such as record or variant creation.

For example:

let f _ = { x = invalid_arg "fail" ; y = 42 }

would be instrumented as something like this:

let f _ = { x = __bisect_post_visit__ 1 (invalid_arg "fail") ; y = 42 }

and your coverage would show a miss for position 1, indicating dead code in your
program. Got it.

However, I'm still unclear about the benefit of this approach in branches
(if-then-else and match).

In line with TDD, I've added new test cases related to these scenarios in #435.
These cases are complex, and I believe it would be helpful to monitor any behavior
changes around them.

I've also tried suppressing instrumentation of the out-edge with [@coverage off]
as you suggested. Indeed, this removes the branch instrumentation entirely. I
added this to the new tests as well.

Looking at the test cases, something doesn't quite feel right to me. Do you feel
the same way?

[aantron]

However, I'm still unclear about the benefit of this approach in branches
(if-then-else and match).

The post-instrumentation of application expressions is orthogonal to whether they are in if expressions. They only interact in that Bisect considers the surrounding if expression in order to propagate information about whether the nested expression is in tail position or not, and in that the point is visually placed on the end of the application expression, rather than on the next expression, because there are two out-edges, one for the then case and one for the else case, whereas for contexts with only one out-edge (such as when the application is the first expression in a sequence), Bisect places the out-edge point visually on the first character of the successor expression.

Looking at the test cases, something doesn't quite feel right to me. Do you feel
the same way?

I'm not sure what you mean, but I've commented in #435. Perhaps you could point out what is wrong by commenting on a specific line in the PR, except, of course, that I agree that it is suboptimal that [@coverage off] in a branch turns off both the branch instrumentation and the out-edge instrumentation of the nested apply expression.