New API: summary/meta-issue

[aantron]

The Bisect "library" was originally included in the installation only so that instrumented programs could be linked with the runtime. It's an internal interface. However, it has made Bisect somewhat "hackable" (ocveralls, bisect-summary). That's a good thing :)

To avoid problems like in #108, and more that are on the way, I think we should try to design a good replacement interface. "Good" means both easy to use, and abstract enough to withstand serious changes in Bisect_ppx.

Proposal

The interface I can suggest now is something quite similar to @lindig's proposal. This would replace Bisect.Common:

(* module Bisect.Coverage *)

type location
(* Abstract, see below. *)

val location_to_offset : location -> int
val location_to_line : location -> int

type point =
  {location : location;
   hits     : int}

type file =
  {points      : point array;
   line_count  : int;
   source_code : string option}

type t = (string, file) Hashtbl.t

val read : ?source_code_directory:string -> ~out_files:string list -> coverage

At a file system level, there would be only .out files. They would be self-contained. They would include everything now in .out files, .cmp files (#102), and the source code (#103). So, a tool would only need to find a collection of them, and pass their names to read. This should make both programmatic use and use of bisect-ppx-report easier – especially in contexts where there isn't access to the build directories.

Details

• Lines are included in locations, so that it's not necessary to walk over the source to convert offsets to lines. Also, the source may be missing.

• Type location is abstract, because we might need to switch to location ranges to address a potential future problem:

  There may be some challenges, when an expression has the same location as some relevant subexpression.

  I'd also like the freedom to decide between using Location.t, and thus depending on compiler-libs, or not.

• A digest can be easily computed from source_code. We can include the digest, so that ocveralls can work even if the source is missing, but I'm wondering if it's not better for ocveralls to simply fail in that case. @lindig, what was your reason for including the digest?

• line_count is there for the same reason as lines in locations, so that you can know how many lines to report as unvisited without scanning the source, especially if it is missing.

• The source can be missing if we provide the -no-source-code option:

  We could offer a preprocessor flag such as -no-source-code for users worried about accidentally leaking intellectual property in an instrumented binary.

  I'm not really sure if it's even necessary to provide it. It would simplify the interface if we knew we don't have to worry about this. However, if someone does use -no-source-code, the ?source_code_directory option to read would be a fallback to look for source files in the file system.

• If we proceed with this, Bisect_ppx's library will become incompatible with Bisect's. That will complicate ocveralls, if it wants to be compatible with both. @sagotch, what do you think?

Work

• .cmp (Eliminate .cmp files #102).
• Sources (Suggestion: inline source into instrumented files #103). Abandoned. See Problem using with Esy/Reason/Dune #187 (comment).
• Provide the new interface.
• Hide (what's left of) Bisect.Common or discourage its use, e.g. by renaming it to Internal. Bisect.Common was renamed to Bisect_common as part of BuckleScript build, NPM packaging, Js_of_ocaml support #206. This isn't quite hiding, but it's enough to disrupt any current users.
• Avoid use of Marshal for .out files (see documentation for file format #114). Done as part of BuckleScript build, NPM packaging, Js_of_ocaml support #206, see 923f088.

[lindig]

My motivation for including the MD5 digest was that you could check that runtime data fits to the source file that you are looking at. Maybe do this:

type source = Digest of string | Code of string
type file = 
    { …
    ; source: source
    }

[aantron]

Ah, right, good point. If you generate the coverage data on a remote system, and bring it back to a development system with sources available, then the digest is kind of needed. I guess we should just always include it, then.

That it's useful also for coveralls is then a happy coincidence :)

[lindig]

If you include both source code and the digest there is a risk of inconsistency between the two. That is why I suggested to use either one. Admittedly, this would be a small problem and I'd be fine with it.

[aantron]

How would the inconsistency arise? The digest would be computed from the source code.

[lindig]

Of course, there won't be an inconsistency if you always do it this way. But if things go wrong, you could end up with an inconsistency. It would be safer to let the user compute the digest from the source code string and to be sure this is correct rather than relying on the stored value. Calling Digest.string should be easy enough.

[aantron]

I think it would be natural for Bisect_ppx to compute the digest from the same source code string that it includes in the output – I think that's required for Bisect_ppx to be correct, since it already, AFAIR, reads the source for other purposes during instrumentation.

Perhaps we just need to clearly document this relation between the source and digest as part of the API, so that the user doesn't have to worry about inconsistency between them.