Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

No way for a preprocessor to communicate an error properly #6399

Closed
vicuna opened this Issue May 6, 2014 · 11 comments

Comments

Projects
None yet
2 participants
@vicuna
Copy link
Collaborator

vicuna commented May 6, 2014

Original bug ID: 6399
Reporter: @whitequark
Assigned to: @alainfrisch
Status: closed (set by @alainfrisch on 2014-05-14T10:24:07Z)
Resolution: fixed
Priority: normal
Severity: major
Fixed in version: 4.02.0+dev
Category: ~DO NOT USE (was: OCaml general)
Monitored by: @gasche

Bug description

Currently, 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.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented May 6, 2014

Comment author: @alainfrisch

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...)

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented May 6, 2014

Comment author: @alainfrisch

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).

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented May 6, 2014

Comment author: @whitequark

And do a similar thing with warnings, right?

Thanks, I like this solution much more than what I had in mind.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented May 6, 2014

Comment author: @whitequark

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.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented May 6, 2014

Comment author: @alainfrisch

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.)

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented May 6, 2014

Comment author: @whitequark

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

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented May 7, 2014

Comment author: @whitequark

Your suggestion worked perfectly, see #55

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented May 7, 2014

Comment author: @alainfrisch

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).

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented May 7, 2014

Comment author: @alainfrisch

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).

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented May 7, 2014

Comment author: @whitequark

Nice! But why did you add Location.Error? It doesn't seem like it is used anywhere.

@vicuna

This comment has been minimized.

Copy link
Collaborator Author

vicuna commented May 7, 2014

Comment author: @alainfrisch

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: ocaml-community/sedlex@0ebf313

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

@vicuna vicuna closed this May 14, 2014

@vicuna vicuna added the bug label Mar 20, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.