Open mbarbin opened 7 months ago
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_apply
s 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
, ormatch
case is aPexp_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.
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?
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.
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 wherebisect_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
:test/instrumentation/control/match.t
: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^1:
src/status_line/ml
:I examined the instrumented code using
dune describe
:which shows:
From my understanding, this code will never
visit
the point29
, by design. This matches the misses that I see in the report^2.Tangentially, I noticed that there's special handling for the
raise
function, which is identified as atrivial_function
. If the function wasraise
instead ofraise_s
, this would resolve the issue. However, before suggesting thatraise_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
, ormatch
case is aPexp_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.