Add a driver mode with no output if there is no rewriting to do

[NathanReb]

Context

I've been working on allowing dune to use ppx in the past few months. If you want detailed context you can take a look at the original issue, the draft PR or this PR that makes use of it to derive Dyn.t converters using a ppx in dune.

In short, since dune cannot have a build dependency on ppxlib we're going to include a preprocessed (.ml or .mli, not the marshalled AST) version of the source files that use any ppx in a specific folder of dune's source tree. These files will be used by the bootstrap program in place of the original files to build dune while in development, we will simply use ppx as we would in any other project and just keep that extra folder up to date.

This works well but at the moment, using a ppx in a library in dune means we're going to keep a copy of every single file composing it, even if they do not require to be preprocessed. What we would like is to instead only have a preprocessed copy if it's necessary.

We don't want to use the per module ppx specification as this requires a lot of configuration and maintenance over the time.

Feature

The most convenient way would be to have an option to tell the ppxlib driver not to produce an output file if it knows there is no rewriting to be done.

There are cases where the driver wouldn't be able to tell accurately, for instance with ppx that aren't written as context free rules but that's not the majority and likely something we won't use in dune.

I'm thinking of a fairly simple feature, no comparison between ASTs, just keeping track of context free rule applications. I.e. if there is no whole AST transformation registered and if none of the context free rule were applied, the driver would not produce an output.

Let me know what you think about adding such a feature.

I would of course take care of implementing it.

[pitag-ha]

Sounds good to me! Having logic inside the ppxlib driver to know whether it has actually done any transformation or not can end up being useful in a lot of contexts.

One detail about the implementation approach you have in mind:

if there is no whole AST transformation registered and if none of the context free rule were applied, the driver would not produce an output.

There's a case in which this can end up producing a confusing situation: If there's an attribute (e.g. a deriver) annotation in the source code that's not rewritten by the driver, then the driver won't produce an output, so the driver won't add an "attribute ignored" warning. The compiler just drops attributes. So instead of understanding that the deriver/attribute has been ignored, the user will see confusing error messages. In other words: Adding "attribute ignored" warnings is an important feature of the driver that would be silenced by this.

To throw in a possible alternative without having given it much thought: Instead of not producing an output when no transformation happens, you could not produce an output when no transformation is expected, i.e. when the source code doesn't contain extension nodes and doesn't contain attributes, up to several details such as reserved attribute namespaces.

[NathanReb]

That's a good point.

We should definitely report those errors somehow, either by preserving the output in such cases (considering error nodes injection as rewriting and therefore requiring an output) or by emitting an error or warning on stderr.
I think I like the latter a little bit better, it is a bit more direct than delaying the error reporting to whoever is going to consume the preprocessed file. On the other hand that means that the error reporting is going to differ a lot with or without the flag.

We'd only need to do this for unexpanded [@@deriving ...]. Unexpanded extension nodes will trigger an error from the compiler which is already what happens now.

What do you think about this behaviour?

[pitag-ha]

Sorry for the delay in replying.

Emitting errors about untouched attributes on stderr instead of embedding them in the AST is a very good point!

I think the current situation we have is: When the embed_errors=true/false logic is set to true, then all errors are embedded into the AST. If it's set to false, then most errors, e.g. parse errors, are emitted on stderr. However, errors about unused attributes are still embedded into the AST. Do you confirm that? If so, that's a good catch and I agree that we'd benefit from making it coherent.

Btw, I assume you're planning to call the driver with embed-errors set to false in general in your new workflow for the dune repo, right?

[NathanReb]

Yes, it's right that in this dune workflow for ppx we'd have no use for embedding errors as we don't want to commit those errors obviously so we'd have no use for such files and those errors would only be reported when bootstrapping dune again, which doesn't happen that often in development so it would be easy to miss those errors when updating the ppx/ folder until opening a PR and having the CI report it which is not a very good workflow.

In this case, we wouldn't care about the performance because the driver wouldn't be called when building dune so having a version that reports all errors properly before exit 1-ing without writing a file would be ideal.