Bad ppx annotations are not reported

[yminsky]

Expected Behavior

Inside of Jane Street's walls, when I put in a useless annotation, I get a compilation error. In Dune, I do not.

In addition to getting errors on unused annotations, I'll get spelling-fix recommendations. Given that the PPX is capable of doing this, I'm assuming the issue is with the build rules, which is why I'm reporting it here.

Reproduction

It's easy to reproduce. Create a minimal dune file:

(library
 (name foo)
 (preprocess (pps ppx_jane))
 (libraries base))

And create a single file, dune.ml:

open Base

type z =
  { x : int [@sexp.drop_if Fn.const true]
  ; y : float
  }
[@@deriving sexp_of]

And it will build without error, despite the fact that the correct annotation is [@sexp_drop_if Fn.const true], i.e., the period after sexp should be an underscore.

Specifications

[yminsky@viserion ~]$ dune --version
2.9.1
[yminsky@viserion ~]$ ocamlc --version
4.13.1
[yminsky@viserion ~]$ cat /etc/os-release 
NAME="Fedora Linux"
VERSION="35 (Workstation Edition)"

[ghost]

These checks are actually disabled in ppxlib, and explicitly enabled in Jane Street internal copy of ppxlib. All ppx rewriters in use must be based on ppxlib for these checks to work. In the past, a lot of open source ppx rewriters were not based on ppxlib, which is why we disabled them in the public version of ppxlib.

Now that most ppx rewriters are ppxlib-based, we should probably enable them by default. /cc @pitag-ha @emillon.

[rgrinberg]

If the check is enabled on the ppxlib side, will that not break a bunch of old (and possibly not so old) projects? Seems a bit aggressive to me.

What about treating ppxlib the same way as we treat ocaml's warnings? Basically freeze the set based on the dune project version.

[ghost]

I think that's good idea.

[pitag-ha]

What about treating ppxlib the same way as we treat ocaml's warnings? Basically freeze the set based on the dune project version.

That sounds interesting! Would you mind giving me some context about this idea? How are ocaml's warnings treated in dune / what does it mean to freeze a set (set of what?) based on the dune project version? Do you mean to call the ppxlib driver either with or without certain flags depending on the dune version?

[ghost]

That sounds interesting! Would you mind giving me some context about this idea? How are ocaml's warnings treated in dune / what does it mean to freeze a set (set of what?) based on the dune project version?

For instance, let's say that the default -w argument Dune passes to ocamlc is -w @1..10@20..60 and a new version of OCaml introduces a new warning 61. Let's say everyone agrees it is a good warning to have by default. If we wanted to enable it by default in Dune 3.0, then having (lang dune 2.9) would make Dune still pass the old -w @1..10@20..60 argument to ocamlc. But having (lang dune 3.0) would make Dune pass -w @1..10@20..61 to ocamlc.

This way existing projects are completely unaffected and will continue building exactly the same as they did before. When someone decides to upgrade their (lang dune ...) version, only then will they get new build errors that they need to fix.

Do you mean to call the ppxlib driver either with or without certain flags depending on the dune version?

I'll let @rgrinberg comment if he add another idea in mind, but that's indeed how we could do it. Another way, that would be easier from Dune's point of view would be to always pass the Dune lang version to ppxlib and let it decides what it means.

[rgrinberg]

Jeremie's description is what I had in mind. In particular, setting individual warnings in dune. Passing the dune-project version to ppxlib also sounds like an interesting approach.

[pitag-ha]

I see, thanks for explaining! That sounds like a good idea also to me.

[ghost]

Let's go for the version where Dune passes -check -check-on-extensions to ppxlib, at least it will work with all existing versions of ppxlib. And to recognise ppxlib, let's just match on the library name. This will make our life simpler.

Could one of you write the PR? @pitag-ha if you want to give it a shot we can give you some pointers.

[pitag-ha]

@pitag-ha if you want to give it a shot

Sure, sounds good. That way I also get a bit more familiar with how dune calls ppxlib.

we can give you some pointers.

That would be great (I haven't worked on dune yet)!

[emillon]

Just chiming in, this plan sounds good.

[pitag-ha]

By the way, I have another question in the same context. Why does dune pass the -null flag to a PPX linter used via the lint stanza (there's more information on this ppxlib issue)? Does that also have historic reasons?

[ghost]

Great. I'm not working today and have other obligations, but I'll do a write up on Monday, unless Rudi beats me to it.

By the way, I have another question in the same context. Why does dune pass the -null flag to a PPX linter used via the lint stanza (there's more information on this ppxlib issue)? Does that also have historic reasons?

Because the linting step shouldn't produce an output AST. It's should only produce errors (if any). BTW, It's ppxlib who is telling Dune to use this flag, see this line in the dune file of ppxlib.

[pitag-ha]

Because the linting step shouldn't produce an output AST. It's should only produce errors (if any). BTW, It's ppxlib who is telling Dune to use this flag, see this line in the dune file of ppxlib.

Oh ok, that makes a lot of sense. Thanks! It's not really that related in the end. I hope it hasn't been too distracting!

[ghost]

No problem, it was just a two small comments detour. I haven't forgotten about this BTW, just pretty busy at the moment. I wanted to give an overview of the handling of preprocessing in Dune, as this part is a bit complex. I started writing something up, but in the meantime if you want to start looking everything happens in src/dune_rules/preprocessing.ml. In particular, this is the part where we construct the command line for invoking the ppx driver.

[pitag-ha]

BTW, I haven't forgotten about this either. I'll get to it hopefully later this week (or, if not, beginning of next week).

[pitag-ha]

Hi again, I've now started looking into implementing this and have made the following observation: When passing the PPX driver to merlin, dune doesn't know if the driver comes from ppxlib or not. So if we enable the checks via flags without changing much code, there will be a difference in behavior between dune (checks enabled) and merlin (checks not enabled). Another option would be to enable the checks via a ppxlib driver function when creating the driver. Enabling the checks by creation would have the upside that we guarantee consistency among different driver calls. It would have the downside that dune build --verbose, which shows the driver invocation including the flags, wouldn't reveal that the checks have been enabled.

So what do you think? I'm happy to implement either of these options: via ppxlib function (very simple); via flags with inconsistency between dune and merlin (very simple); via flags with consistency between dune and merlin but without guarantee of general consistency (probably a bit more complex).

In case you want to know my opinion: I might be missing some implications, but with the implications I've seen I think I'd go for the first option, since it seems better long-term. Of course, it would be nice to understand the new behavior when using --verbose, but to be fair folks can also have a look at dune's changelog.

[ghost]

The ppxlib function method seems like a good idea to me. It seems to me that keeping the behaviours of the tools consistent is the most important here. As long as the difference in behaviour between <3.0 and >=3.0 is documented and announced enough, it should all work fine.

BTW, while such a change wouldn't be visible via dune build --verbose, it would be visible via dune rules. So it would be possible for a user to debug this from the outside.

[pitag-ha]

Oh cool, I wasn't aware of dune rules. So I'll go for the ppxlib function method. Thanks!