Improve reporting of exceptions coming from rewriters

[pitag-ha]

Some context

There are three different ways in which PPX rewriters might report their errors (ordered from best-practice to worst-practice):

1. The rewriter can convert their error into an exception extension node and embed that node into the AST. That's the ideal way of error reporting for the following reason: the AST stays as complete as possible and only the concrete flawed node(s) contain(s) an exception. As a consequence, both merlin and the compiler can still read the whole AST returned by ppxlib and merlin reports all errors in it.
2. The rewriter can raise a located error. In that case, the behavior of the ppxlib driver depends on the flags set: if the embed-errors-flag isn't set, the raised exception is propagated to the ppxlib driver. If it's set (which, I think, is the case when dune or merlin run it), the driver catches that exception, turns it into an exception extension node itself and returns a one-node AST with just that one extension node, while the rest of the AST is lost and merlin can only report that one error. Still, this option is better than option 3. for two reasons:
   • at least the error is located, so merlin and the compiler can point to the right place in the source code when reporting it
   • the driver exists successfully instead of raising
3. The rewriter might raise any kind of exception, not a located error. That's the worst case scenario since in that case the ppxlib driver just raises a fatal error without pointing to where the error comes from.

Suggestion for improvement

In cases 2. and 3., it would be nice to extend the error message to also point to the PPX rewriter that's throwing those exceptions and pointing out that the user could file an issue in that PPX rewriter. That would improve the situation in two ways:

• the devs of the rewriter get notified that it would be good to improve their error reporting
• the end-user would understand where the problem lies instead of blaming (and possibly filing issues at) merlin.

So I suggest to do the following two things (while also improving our documentation on how to report errors with option 1. as part of a different issue):

• [x] Write tests to verify the current behaviour I've described in Some context. Done by @panglesd in #289.
• [ ] Improve our error reporting in the case of 2. and 3. the way just described.

edit note: I've just edited the issue description to correct it after @panglesd has found out that option 2. is the same for context-free rules (i.e. extension node rewriters or derivers) and whole-file transormations (i.e. a transformation registered via the impl or intf argument or similar of Driver.register_transformation).

[pitag-ha]

cc @panglesd who might be interested in implementing part(s) of this issue as part of his on-boarding at Tarides (not related to Outreachy) :)

[panglesd]

I submitted a first PR #289 for the first task: writing tests to verify the current behaviour. I found out that actually, there is no distinction between 2.a) and 2.b). In both cases, the whole AST of the file is thrown away and replaced by a one-node error AST. @pitag-ha should I open a new issue about this?

[pitag-ha]

I submitted a first PR #289 for the first task: writing tests to verify the current behaviour.

Thanks, the tests are great! I'll check the first check box of the issue.

I found out that actually, there is no distinction between 2.a) and 2.b). In both cases, the whole AST of the file is thrown away and replaced by a one-node error AST.

Also thanks! That's both very good to know and unfortunate that that's the case. I'll adapt the issue description.

@pitag-ha should I open a new issue about this?

You mean an issue to implement the situation I originally described in 2.a) (that for context-free rules, a raised located error in the rewriter is caught and embedded as an exception extension node into the rest of the AST)? If so: Yes, that would be great!