Coverage position misplacement in module

[vch9]

I just found this in our test coverage.

Shouldn't the coverage position be on the next line ?

[aantron]

No, there shouldn't be a coverage point on any line here. Bisect only instruments the expression language (not the module language, and certainly not comments).

I have two most likely initial candidate explanations:

1. The source file and the .coverage files are somehow badly out of sync, for example the .coverage files come from a different branch than the sources.
2. There is a false location generated for some expression, which just happens to actually be on the line of the comment, rather than where the expression actually is.

[aantron]

Are you able to reduce the source file to smaller examples that still exhibit this behavior? Does it occur outside of your overall build?

The content of the comment makes me suspect that this is a module that might be relatively easily taken out of your overall project.

[vch9]

I can't have my hands on a computer this week-end but the file (and module) is relatively small yes. I did not manage to reproduce the issue on a minimal example (in a few minutes tbh), but will do very soon.

Also an information I did not mention: the code is preprocessed by ppx_inline_test before bisect_ppx, could the generated code mess up positions ?

[aantron]

Also an information I did not mention: the code is preprocessed by ppx_inline_test before bisect_ppx, could the generated code mess up positions ?

Yes, that's very possible. I don't know what the case is here yet of course, but PPXs have often emitted bad locations in past experience.

[vch9]

Here is a minimal example: https://github.com/vch9/bisect_ppx-issue-387

It is most certainly the generated code by ppx_inline_test. The ppx produces just before the module the line:

let () = Ppx_inline_test_lib.Runtime.set_lib_and_partition "trie" ""
module Suffix = struct
  ...
end

This is I believe the only difference of coverage's location when the preprocessing is enabled or not in this example.

[aantron]

Could inspect the AST to get the exact location generated by ppx_inline_test? You should be able to run something like ocamlc -c -dparsetree the_preprocessed_source_file.pp.ml.

I think the "correct" behavior for PPXs in most cases is to give generated code a "ghost" location, unless it corresponds to some user-written code in a very obvious way. Bisect_ppx knows not to instrument expressions that have ghost locations:

bisect_ppx/src/ppx/instrument.ml

Lines 185 to 192 in e65ee7f

	and expression_should_not_be_instrumented ~point_loc:loc ~use_loc_of =
	let e =
	match use_loc_of with
	| Some e -> e
	| None -> e
	in
	Location.(loc.loc_ghost) ||
	Coverage_attributes.has_off_attribute e.pexp_attributes

If it's true that this generated code has a non-ghost location, it might not be deliberate, and this might be a mistake in ppx_inline_test.

[vch9]

The generated code does not have a ghost location:

 structure_item (src/trie.ml[4,201+0]..[4,201+0])
    Pstr_value Nonrec
    [
      <def>
        pattern (src/trie.ml[4,201+0]..[4,201+0])
          Ppat_construct "()" (src/trie.ml[4,201+0]..[4,201+0])
          None
        expression (src/trie.ml[4,201+0]..[4,201+0])
          Pexp_apply
          expression (src/trie.ml[4,201+0]..[4,201+0])
            Pexp_ident "Ppx_inline_test_lib.Runtime.set_lib_and_partition" (src/trie.ml[4,201+0]..[4,201+0])
          [
            <arg>
            Nolabel
              expression (src/trie.ml[4,201+0]..[4,201+0])
                Pexp_constant PConst_string("trie",None)
            <arg>
            Nolabel
              expression (src/trie.ml[4,201+0]..[4,201+0])
                Pexp_constant PConst_string("",None)
          ]
    ]

The generated code then is instrumented:

let () =
  ___bisect_post_visit___ 0
    (Ppx_inline_test_lib.Runtime.set_lib_and_partition "trie" "")

I will make an issue on ppx_inline_test for this

[aantron]

Looks like this issue is being handled in janestreet/ppx_inline_test#33, so closing it here.