Skip to content

Add tests to document current behavior of post-instr in branches #435

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
mbarbin wants to merge 1 commit into from
Closed

Add tests to document current behavior of post-instr in branches #435

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
113 changes: 113 additions & 0 deletions test/instrument/control/if.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,3 +97,116 @@ tail position iff the whole if-expression is in tail position.
else (
___bisect_visit___ 4;
print_endline "bar")

When the if-then-else is in a sequence, the next-expressions are
instrumented as expected.

$ bash ../test.sh <<'EOF'
> let _ =
> if bool_of_string "true" then print_endline "foo" else print_endline "bar";
> print_endline "this";
> print_endline "that";
> EOF
let _ =
if bool_of_string "true" then (
___bisect_visit___ 5;
___bisect_post_visit___ 2 (print_endline "foo"))
else (
___bisect_visit___ 4;
___bisect_post_visit___ 3 (print_endline "bar"));
___bisect_post_visit___ 1 (print_endline "this");
___bisect_post_visit___ 0 (print_endline "that")

Where there's a raise or a failwith guarded by a conditional, the
branch itself is instrumented, but the raising expression is not
post-instrumented...
Copy link
Owner

@aantron aantron Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the incremental value of having this test over the tests in apply/special.t, including

bisect_ppx/test/instrument/apply/special.t

Line 293 in e302656

> let _ = failwith (print_endline "foo"; "bar")
? Is there any need to test this in interaction with if-expressions?

Copy link
Contributor Author

@mbarbin mbarbin Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe there's a need for this test from the standpoint of the current
implementation. I do see its value in terms of future-proofing and documenting.

The if cond then raise pattern is quite common. As it stands, one must be
cautious with how the raising expression is written. It's important to note that
even seemingly innocuous refactoring, which shouldn't affect coverage, could
potentially introduce non-visitable coverage points. This can be quite
surprising.

Consider the following example:

-| if cond then raise Not_found
+| if cond then raise_s [%sexp "Key not found", { key : Key.t}]

This results in the insertion of non-visitable point 1:

-| if cond then raise Not_found
+| if cond then (__bisect_post__visit 1 (raise_s [%sexp "Key not found", { key : Key.t}]))
``


$ bash ../test.sh <<'EOF'
> let _f ~cond1 ~cond2 =
> if cond1 then failwith "this";
> if cond2 then raise Not_found;
> print_endline "this";
> print_endline "that";
> EOF
let _f ~cond1 ~cond2 =
___bisect_visit___ 5;
if cond1 then (
___bisect_visit___ 4;
failwith "this");
___bisect_visit___ 3;
if cond2 then (
___bisect_visit___ 2;
raise Not_found);
___bisect_visit___ 1;
___bisect_post_visit___ 0 (print_endline "this");
print_endline "that"

... however note that this doesn't hold for all raising expressions.
For example, if you use `invalid_arg`, or a raising helper, its
application will be post-instrumented (unless all of its arguments are
labeled !?), resulting in a code location that can never be visited,
by design. These are currently acknowledged limitations.
Copy link
Owner

@aantron aantron Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an acknowledged and inevitable limitation because there is no way for Bisect to know that a specific identifier refers to a function that always or never raises. In fact, it doesn't know that for the identifiers failwith or invalid_arg either, as those can be redefined and shadowed by the user, including by another module or a third-party library that is not even processed by Bisect during compilation of the user's particular project. I don't see that we need a test for this -- the actual behavior is covered by the normal apply tests.

Copy link
Owner

@aantron aantron Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Raises or diverges)

Copy link
Contributor Author

@mbarbin mbarbin Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there is no way for Bisect to know that a specific identifier refers to a
function that always or never raises

I agree. It's fair to say that "a strategy to limit non-visitable points, which
relies partly on identifiers, will inevitably encounter these limitations."
However, if we were to discover an alternative approach (assuming one exists),
we might still see improvements in this test. In other words, the insertion of
post-instrumentation might not be inevitable when considering the range of
possible instrumentation strategies.

Copy link
Contributor Author

@mbarbin mbarbin Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Raises or diverges)

Btw, I didn't understand the part where you said "or diverges".

Copy link
Owner

@aantron aantron Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Raises or diverges)

Btw, I didn't understand the part where you said "or diverges".

Post-instrumentation checks that a function evaluates to a value. The two ways that it doesn't in OCaml are when it raises an exception or when it diverges.

Copy link
Owner

@aantron aantron Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are the two ways that the out-edge of aPexp_apply may not be followed.

Copy link
Contributor Author

@mbarbin mbarbin Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does "diverging" encompass non-termination and potentially the effects in OCaml 5? Something else?

Regarding the effects, I'm optimistic that their influence on the strategy will
be similar to that of exceptions. I hope this won't necessitate too big of an
exploration of the OCaml 5 runtime 🤯. Thank you again for your explanation!

Copy link
Owner

@aantron aantron Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, "diverging" is non-termination. OCaml 5 effects are supposed to always be returned from exactly once, so they should not affect control flow as visible to Bisect, although this is not enforced, and for effects that correspond to async computation the "exactly once" might be later than program termination, so in that sense they behave like exceptions.


$ bash ../test.sh <<'EOF'
> let invalid_n_labeled ~n = invalid_arg (Printf.sprintf "Invalid value of n=%d" n)
>
> let invalid_p_unlabeled p = invalid_arg (Printf.sprintf "Invalid value of p=%d" p)
>
> let _f ~cond1 ~n ~p =
> if cond1 then invalid_arg "this";
> if n < 0 then invalid_n_labeled ~n;
> if p < 0 then invalid_p_unlabeled p;
> print_endline "this";
> print_endline "that";
> EOF
let invalid_n_labeled ~n =
___bisect_visit___ 1;
invalid_arg
(___bisect_post_visit___ 0 (Printf.sprintf "Invalid value of n=%d" n))

let invalid_p_unlabeled p =
___bisect_visit___ 3;
invalid_arg
(___bisect_post_visit___ 2 (Printf.sprintf "Invalid value of p=%d" p))

let _f ~cond1 ~n ~p =
___bisect_visit___ 13;
if cond1 then (
___bisect_visit___ 12;
___bisect_post_visit___ 11 (invalid_arg "this"));
___bisect_visit___ 10;
if n < 0 then (
___bisect_visit___ 9;
invalid_n_labeled ~n);
___bisect_visit___ 8;
if p < 0 then (
___bisect_visit___ 7;
___bisect_post_visit___ 6 (invalid_p_unlabeled p));
___bisect_visit___ 5;
___bisect_post_visit___ 4 (print_endline "this");
print_endline "that"

If you try and disable post-instrumentation with a `coverage off`
directive, the branch pre-instrumentation is removed as well,
resulting in losing the ability to control that the branch itself is
visited.
Copy link
Owner

@aantron aantron Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you move this to attribute.t? That is the place for testing [@coverage off]. It already includes cases for interaction of [@coverage off] with multiple language constructs in case.

Copy link
Contributor Author

@mbarbin mbarbin Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate your suggestion to add tests to attribute.t, which I had
overlooked. I will proceed to incorporate new tests there to demonstrate how to
disable coverage in branches (same PR ok?).

Despite the presence of branch coverage testing in attribute.t, I advocate for
retaining this specific part of the control test here. It serves to illustrate
that this method does not provide a viable workaround for the issue at hand.
This was in response to your comment in #433:

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.

This might come across more as literate testing (akin to a tutorial, perhaps?)
rather than a pure behavior test. I've attempted to construct a small narrative
around the added tests. However, if you have strong reservations, I'm open to
removing this section entirely.


$ bash ../test.sh <<'EOF'
> let invalid_p_unlabeled p = invalid_arg (Printf.sprintf "Invalid value of p=%d" p)
>
> let _f ~p =
> if p < 0 then (invalid_p_unlabeled p [@coverage off]);
> print_endline "this";
> print_endline "that";
> EOF
let invalid_p_unlabeled p =
___bisect_visit___ 1;
invalid_arg
(___bisect_post_visit___ 0 (Printf.sprintf "Invalid value of p=%d" p))

let _f ~p =
___bisect_visit___ 4;
if p < 0 then invalid_p_unlabeled p [@coverage off];
___bisect_visit___ 3;
___bisect_post_visit___ 2 (print_endline "this");
print_endline "that"
133 changes: 133 additions & 0 deletions test/instrument/control/match.t
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,3 +65,136 @@ cases are in tail position iff the match expression is in tail position.
| () ->
___bisect_visit___ 2;
print_endline "bar"

Copy link
Owner

@aantron aantron Feb 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Analogous questions for the match.t changes as for the if.t changes.

When the match is in a sequence, the next-expressions are instrumented
as expected.

$ bash ../test.sh <<'EOF'
> let _ =
> (match print_endline "foo" with () -> print_endline "bar");
> print_endline "this";
> print_endline "that"
> EOF
let _ =
(match print_endline "foo" with
| () ->
___bisect_visit___ 3;
___bisect_post_visit___ 2 (print_endline "bar"));
___bisect_post_visit___ 1 (print_endline "this");
___bisect_post_visit___ 0 (print_endline "that")

Where there's a raise or a failwith in a match branch, the branch
itself is instrumented, but the raising expression is not
post-instrumented...

$ bash ../test.sh <<'EOF'
> let _f ~cond1 ~cond2 =
> (match cond1 with
> | true -> failwith "this"
> | false -> ());
> (match cond2 with
> | true -> raise Not_found
> | false -> ());
> print_endline "this";
> print_endline "that";
> EOF
let _f ~cond1 ~cond2 =
___bisect_visit___ 5;
(match cond1 with
| true ->
___bisect_visit___ 3;
failwith "this"
| false ->
___bisect_visit___ 4;
());
(match cond2 with
| true ->
___bisect_visit___ 1;
raise Not_found
| false ->
___bisect_visit___ 2;
());
___bisect_post_visit___ 0 (print_endline "this");
print_endline "that"

... however note that this doesn't hold for all raising expressions.
For example, if you use `invalid_arg`, or a raising helper, its
application will be post-instrumented (unless all of its arguments are
labeled !?), resulting in a code location that can never be visited,
by design. These are currently acknowledged limitations.

$ bash ../test.sh <<'EOF'
> let invalid_n_labeled ~n = invalid_arg (Printf.sprintf "Invalid value of n=%d" n)
>
> let invalid_p_unlabeled p = invalid_arg (Printf.sprintf "Invalid value of p=%d" p)
>
> let _f ~cond1 ~n ~p =
> (match cond1 with true -> invalid_arg "this" | false -> ());
> (match n < 0 with true -> invalid_n_labeled ~n | false -> ());
> (match p < 0 with true -> invalid_p_unlabeled p | false -> ());
> print_endline "this";
> print_endline "that";
> EOF
let invalid_n_labeled ~n =
___bisect_visit___ 1;
invalid_arg
(___bisect_post_visit___ 0 (Printf.sprintf "Invalid value of n=%d" n))

let invalid_p_unlabeled p =
___bisect_visit___ 3;
invalid_arg
(___bisect_post_visit___ 2 (Printf.sprintf "Invalid value of p=%d" p))

let _f ~cond1 ~n ~p =
___bisect_visit___ 13;
(match cond1 with
| true ->
___bisect_visit___ 11;
___bisect_post_visit___ 10 (invalid_arg "this")
| false ->
___bisect_visit___ 12;
());
(match n < 0 with
| true ->
___bisect_visit___ 8;
invalid_n_labeled ~n
| false ->
___bisect_visit___ 9;
());
(match p < 0 with
| true ->
___bisect_visit___ 6;
___bisect_post_visit___ 5 (invalid_p_unlabeled p)
| false ->
___bisect_visit___ 7;
());
___bisect_post_visit___ 4 (print_endline "this");
print_endline "that"

If you try and disable post-instrumentation with a `coverage off`
directive, the branch pre-instrumentation is removed as well,
resulting in losing the ability to control that the branch itself is
visited.

$ bash ../test.sh <<'EOF'
> let invalid_p_unlabeled p = invalid_arg (Printf.sprintf "Invalid value of p=%d" p)
>
> let _f ~p =
> (match p < 0 with true -> (invalid_p_unlabeled p [@coverage off]) | false -> ());
> print_endline "this";
> print_endline "that";
> EOF
let invalid_p_unlabeled p =
___bisect_visit___ 1;
invalid_arg
(___bisect_post_visit___ 0 (Printf.sprintf "Invalid value of p=%d" p))

let _f ~p =
___bisect_visit___ 4;
(match p < 0 with
| true -> invalid_p_unlabeled p [@coverage off]
| false ->
___bisect_visit___ 3;
());
___bisect_post_visit___ 2 (print_endline "this");
print_endline "that"