Mantis Bug Tracker

View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0006399OCamlOCaml generalpublic2014-05-06 19:312014-05-14 12:24
Reporterwhitequark 
Assigned Tofrisch 
PrioritynormalSeveritymajorReproducibilityhave not tried
StatusclosedResolutionfixed 
PlatformOSOS Version
Product Version 
Target VersionFixed in Version4.02.0+dev 
Summary0006399: No way for a preprocessor to communicate an error properly
DescriptionCurrently, the only thing a ppx preprocessor can do to communicate an error is to write it to stderr; indeed, this is what Ast_mapper.run_main does with any raised exceptions.

I think it is wrong, because it leaves the tooling with having to guess the formatting of the output, and in particular extracting the location.

As a motivating case, I've just tried to add proper ppx support to utop. Not only ppx errors will not be nicely highlighted (because ppx doesn't have access to the source code), but also Ast_mapper.run_main's writes to stderr will be interleaved with the utop output, and essentially will be overwritten with utop's formatting.

This will only be worse for tools like merlin.

I propose that if the preprocessor exits with a success, then the output file contains a serialized AST; if it exits with a failure, then the output file contains serialized Location.error list.

I will provide a patch shortly if this is approved.
TagsNo tags attached.
Attached Files

- Relationships

-  Notes
(0011367)
frisch (developer)
2014-05-06 20:56

This is a very good idea. Please send a patch!

One could also plan for a way for ppx processors to communicate warnings to the compiler, for the same reason.

Note that currently, the format of serialized AST used for -ppx is strictly the same as for -pp (same magic numbers as well). We might need to change that, at least to support communicating warnings in the absence of errors. (Except if we let ppx processors communicate warnings as attributes...)
(0011368)
frisch (developer)
2014-05-06 21:42

Well, actually, we could define a "built-in" [%%error "...."] extension node, on which the compiler reacts by displaying the error message in the payload and the location of the extension node. The ppx processor could then create a Parsetree with only this extension node to be sure it would be reported by the compiler, or insert it in a more "logical" position in the whole Parsetree (which could have the effect that an actual type error "earlier" in the Parsetree would be displayed first).
(0011369)
whitequark (reporter)
2014-05-06 21:45

And do a similar thing with warnings, right?

Thanks, I like this solution much more than what I had in mind.
(0011371)
whitequark (reporter)
2014-05-06 22:42

Should I call them [%%ocaml.error] or [%%error]? I think the former.

Also, I'm thinking it's better to use [%ocaml.error] here. The syntactic extension will then just replace the part of the tree it doesn't like with:

  [%ocaml.error "message" [%ocaml.error "suberror1"] [%ocaml.error "suberror2"] ...]

Similarly, [%ocaml.warning] will work like a wrapper:

  [%ocaml.warning "message" (let foo in bar ...)]

It's really convenient, because you don't have to keep some way of going up the tree to insert the Pstr_extension, you just replace the subtree you can't interpret and maybe even keep its location.
(0011372)
frisch (developer)
2014-05-06 23:35

We should support both [%ocaml.error] and [%error] (built-in attributes such as ocaml. and ocaml.deprecated now supports both the short and the qualified form).

For errors: the compiler should recognize all instances of [%ocaml.error] and of [%%ocaml.error].

> It's really convenient, because you don't have to keep some way of going up the tree to insert the Pstr_extension.

If the ppx wants to do so (e.g. because of a fatal error which is not really associated to a specific part of the Parsetree), it would probably raise an exception which would be captured "at toplevel". We could provide some built-in mechanism in Ast_mapper.


Concerning the insertion of warnings, it would rather make sense to use attributes, not extension nodes, I think. (But @ocaml.warning is already reserved for the built-in attribute which lets the user enable/disable warnings.)
(0011373)
whitequark (reporter)
2014-05-07 00:13

Agreed; Ast_mapper.run_main should intercept errors, as it does now, and replace the output tree with the relevant Pstr_extension.


I agree; however, I couldn't come up with a good name, and so I'll leave the warning attribute to someone more qualified to solve the hardest problem in CS. :D
(0011374)
whitequark (reporter)
2014-05-07 02:33

Your suggestion worked perfectly, see https://github.com/ocaml/ocaml/pull/55 [^]
(0011377)
frisch (developer)
2014-05-07 10:27

Thanks a lot! I've applied your patch, after exposing extension_of_error in ast_mapper.mli (so that it can be used by ppx code to create "local" errors).
(0011391)
frisch (developer)
2014-05-07 19:24
edited on: 2014-05-07 19:25

There is now also a similar protocol to pass warnings to the compiler (all reported as warning 22 for now). See recent commits on trunk. It doesn't work in the toplevel yet (hence I leave this ticket open).

(0011392)
whitequark (reporter)
2014-05-07 19:28

Nice! But why did you add Location.Error? It doesn't seem like it is used anywhere.
(0011393)
frisch (developer)
2014-05-07 21:24

> why did you add Location.Error?

To make it easy for a ppx processor to report a global error. See for instance how sedlex does it: https://github.com/alainfrisch/sedlex/commit/0ebf3130d969f04e61813097e5dc284c120829bd [^]

Without this built-in exception, each ppx processor would need to create its own exception and register a "printer" (exception -> error) with Location.

- Issue History
Date Modified Username Field Change
2014-05-06 19:31 whitequark New Issue
2014-05-06 20:56 frisch Note Added: 0011367
2014-05-06 21:42 frisch Note Added: 0011368
2014-05-06 21:45 whitequark Note Added: 0011369
2014-05-06 22:42 whitequark Note Added: 0011371
2014-05-06 23:35 frisch Note Added: 0011372
2014-05-07 00:13 whitequark Note Added: 0011373
2014-05-07 02:33 whitequark Note Added: 0011374
2014-05-07 10:27 frisch Note Added: 0011377
2014-05-07 19:24 frisch Note Added: 0011391
2014-05-07 19:25 frisch Note Edited: 0011391 View Revisions
2014-05-07 19:28 whitequark Note Added: 0011392
2014-05-07 21:24 frisch Note Added: 0011393
2014-05-14 12:24 frisch Status new => closed
2014-05-14 12:24 frisch Assigned To => frisch
2014-05-14 12:24 frisch Resolution open => fixed
2014-05-14 12:24 frisch Fixed in Version => 4.02.0+dev


Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker