How to style-check comments in all packages in opam (a.k.a. "odoc lint")

[aantron]

Some thoughts about a separate tool for this purpose, perhaps a shell or OCaml script in this repo, an improved version of the current corpus.sh.

TL;DR for @avsm, @rizo: I'm leaning towards a script that works on one package at a time, to run in opam's CI. It will:

1. Run over the package's .cmti files naturally generated by its own build system.
2. Blindly run over the package's .mli and .mld files, trying to compile them individually.
3. Filter out duplicate errors.
4. Show the result and succeed/fail according to what we think about the severity of having doc syntax errors.

The purpose of (1) is to use the build system's knowledge about project layout and preprocessing. The purpose of (2) is to stupidly try to cover anything that wasn't included by (1), and cover .mld files.

Note that we can do (1) and (2) whether the package build succeeds or not. (1) can still produce some .cmti files, and (2) will work for some .mli files.

Thought process starting from the last time I did a mass style check (more like whether-odoc-is-reasonable check), using corpus.sh:

1. corpus.sh wants as large an opam switch as possible.

   The reason for this is that opam files already contain each package's own build instructions, and we want to build as many .cmtis that way as we can.

   This can be done with a command like

   opam install odoc --criteria='+count(solution)'
   

   It then goes over all the .cmtis and runs odoc compile on each one.

   This covers, to some extent, both packages that use Dune, and packages that don't, as long as they pass -bin-annot. In my experience, using a "typical" package as the "center" of the switch (odoc in the command above) installs about 75% of the packages.

The rest of the thoughts are about covering packages that aren't installed in (1), or files that aren't compiled.

2. List all packages, and repeat (1) with "centers" that are outside already-installed switches.

   These consistent switches centered around different packages might have large intersections of packages that can be installed in both. We may want to install only each package and its dependencies in each next switch.

   There is also the complication that packages installed in each switch might not be at their latest version, so we probably need to check that.

   Also, some packages will not be buildable. This whole process needs to be done on a dedicated (virtual) machine or in a container, because some packages have system dependencies that need to be installed first.

   (recommended) A yet other way to do this is to rely on opam's CI, or a variant of it, and either take .cmti files from it, or just the generated error messages. We likely don't actually need to ever run this new corpus.sh outside opam CI anyway, and it's probably fine to run the script on one package at a time.

After solving (2), we should be checking as many .cmti files as we can get automatically compiled by each package's own build system.

3. Keep the sources of each package. Also, if it is possible that opam ever doesn't download the sources of any package after (2), we should try to download them separately.

   Go through the sources, find .mld files, and run odoc compile on them.

   Find all .mli files in sources, subtract .mli files that we already have .cmti files for. Try to compile these .mli files using separate ocamlc commands, and then run odoc compile on the resulting .cmtis.

   This should cover all .mld files, and all .mlis that either were not used during successful builds in (1) or (2), or .mlis in projects where the builds failed, or could not be started due to constraints, except those .mlis that require preprocessing. We probably can't do anything intelligent about the latter.

   (not recommended by me) We should be able to find the source file of each .cmti by loading it using compiler-libs, and checking locations. At least Dune seems to put correct locations into the files, even in case of preprocessing (though this will need to be checked). However, even if the source file names are wrong, the only harm is that we will effectively try to process the corresponding .mli file twice, as long as no .cmti ever claims that the source file is a real .mli from which it was not compiled (thus shadowing it).

   (recommended) Alternatively, we can just blindly try to compile all .mlis in (3), and use postprocessing to remove duplicate errors.

We may want a mode that works on the master branch of each repo, as that might be more useful for knowing what still needs to be fixed (rather than only released). If going with doing this per-package, then the same script that will work in opam's CI should also work from the root directory of a repo checkout.

EDIT: This was prompted by #226.

[aantron]

There is the complication that if odoc is built from sources to run the style check on a package, odoc's own dependencies might be inconsistent with the package. That is probably solved by installing the odoc binary in the container/virtual machine/wherever this is done. For local/dev checks, most people can still build odoc from source using a normal opam install, because conflicts between odoc and actively developed packages are probably rare.

[dbuenzli]

I don't understand what is wrong with simply using odig here. odig will also do the actual job of correctly sorting out the dependencies across packages to get the right includes directories when you odoc compile them and it will test what people really want to distribute rather than make assumptions about the source tree of packages.

I could add a flag to odig odoc to ask to stop after compile if you desire so.

[aantron]

odig may well be the right answer for this. Does odig (in some way) effectively try to compile .mli files that may be found in a source tree, but are not part of a build for some reason (missing optional dependencies, OS incompatibility, etc.)?

[dbuenzli]

Does odig (in some way) effectively try to compile .mli files that may be found in a source tree, but are not part of a build for some reason (missing optional dependencies, OS incompatibility, etc.)?

No odig works on package installs, i.e. what is in installed in your opam var lib according to the package conventions which are agnostic to build systems (but automatically implemented by systems like dune and topkg). Consult odig help packaging for those and more specifically the section about odoc. These will be extended to support mld files in the next version aswell, see b0-system/odig#31.

Regarding the actual question I don't really understand why you want to peek in package source trees. If you want to minimize false positives and churn, I think the best is to simply excercice odoc on what people expect it will be used on which is package installs. I'm sure the sample you'll get will be large enough to get confidence in the work that is being done.

That said I'm not even sure it's useful to reach for maximality you could simply "bless" a few dozens package as being testing package based on their popularity and/or documentation language usage. I mean I don't expect the parser to change that much or will it ?

[aantron]

I think the motivation for @asvm and @rizo wasn't so much to test odoc on packages, but to test packages against odoc when packages are released, or during their development.

My original motivation for corpus.sh specifically was indeed to check odoc against packages, though, and for that a few test packages do suffice. In fact, I don't see a big motivation for doing almost anything for this purpose, except some ad-hoc testing as the remaining bugs in the parser and/or loader are finished up.

[dbuenzli]

I think the motivation for @asvm and @rizo wasn't so much to test odoc on packages, but to test packages against odoc when packages are released, or during their development.

I don't understand. Shouldn't this simply be done by generating the docs with odoc via the build system of your package ?

[aantron]

I'm not sure either. I'll let @avsm and/or @rizo comment.

It looks like another goal is to produce some output that can be used in Merlin, but that wouldn't be the script I suggested at the start of this issue.

[rizo]

My work on odoc lint was motivated by two reasons: 1) the ability to consistently and periodically test odoc's markup parser on a large package set (literally the whole OPAM repository) and 2) to prepare the work needed to implement merlin integration via a ppx.

After the discussion in #226 I no longer think that 1 is achievable via the mli approach and 2 is complicated for the same reason (other ppxs can interfere with the interfaces). I have started working on an alternative approach for the merlin integration. Regarding 1, I think odig should serve us well for that – after all we should worry about the docs for installable packages people actually use.

[rizo]

Slightly unrelated but something I thought about while working on lint: is it possible/useful to report unresolved reference during compilation? I'm not sure how that would work in dune/merlin without something like #27 though.

[lpw25]

is it possible/useful to report unresolved reference during compilation?

This can't work in general because you are allowed forward references to modules that are compiled later than the current file. Even for backward references it requires the .odoc files to exist for the file's dependencies. That would mean making compilation depend on doc generation which I think is probably not a good idea, and I doubt the dune devs would be keen on making such a change to the build rules.

[github-actions]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.