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.

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?

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?

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.