Driver: Multiple errors can be reported. #447
Conversation
Burnleydev1
requested review from
ceastlund,
panglesd and
pitag-ha
as code owners
There was a problem hiding this comment.
Thanks @Burnleydev1!
I left some comments, but it looks good!
Before merging this PR, I will write an issue explaining why we need your changes: most of the discussion happened offline, and it is good to have a log of the reason of the change.
test/driver/exception_handling/run.t
Outdated
| [%%ocaml.error "A second located error in a whole file transform"] | ||
| [%%ocaml.error "SHOULD APPEAR LAST"] | ||
| type a = int | ||
| type b = int[@@deriving deriver_raised_exception] |
There was a problem hiding this comment.
Maybe a new impl.ml should be created, without this line.
There was a problem hiding this comment.
Hello @panglesd, please I do not understand how to do this, do I add a new multiple_whole_file.ml and add its exe line to the dune file?
There was a problem hiding this comment.
The file impl.ml is defined at lines 113 and 114 of this file. Such a file is created once per example, overwriting the previous one. For instance an impl.ml file was already created at line 84-86, overwritten at line 113-114.
It would be ideal to overwrite impl.ml one more time to have the file tailored to the example (e.g., without a mention to deriver_raised_exception)
There was a problem hiding this comment.
I noticed that we also need an update to the documentation for this PR!
At least the part on handling errors should be updated to reflect the new behaviour.
There was a problem hiding this comment.
This looks good to me!
I have only one concern regarding documentation. I think the good practice section should only contain the recommended way to deal with errors to avoid confusion. It dives pretty deep into how we handle raised exceptions and I don't think this is useful for ppx authors.
Specifying this behaviour is important but I think it should probably live elsewhere for clarity's sake.
@panglesd what should be done regarding #457? There's a bit of documentation here and I think it will conflict with your work. I'm happy to cut out the documentation changes so we can merge this and discuss how to best document the behaviour elsewhere.
|
@Burnleydev1 do you want to take care of rebasing and fixing the last bits? I'm happy to take care of it if you don't have time for this. |
NathanReb
force-pushed
the
multiple_errors
branch
from
5b918a3 to
1729680
Compare
Signed-off-by: Burnleydev1 <[email protected]>
Signed-off-by: Burnleydev1 <[email protected]>
Signed-off-by: Burnleydev1 <[email protected]>
Signed-off-by: Burnleydev1 <[email protected]>
Signed-off-by: Burnleydev1 <[email protected]>
Signed-off-by: Burnleydev1 <[email protected]>
Signed-off-by: Burnleydev1 <[email protected]>
NathanReb
force-pushed
the
multiple_errors
branch
from
1729680 to
9bb25cd
Compare
Signed-off-by: Nathan Rebours <[email protected]>
NathanReb
force-pushed
the
multiple_errors
branch
from
9bb25cd to
55a1892
Compare
|
I just rebased and dropped the documentation changes. This should be good to go! |
|
Great, thanks @NathanReb! Thanks a lot @Burnleydev1 for your work! |
CHANGES: - Fix `Longident.parse` so it properly handles unparenthesized dotted operators such as `+.` or `*.`. (ocaml-ppx/ppxlib#111, @rgrinberg, @NathanReb) - raising an exception does no longer cancel the whole context free phase(ocaml-ppx/ppxlib#453, @Burnleydev1) - Sort embedded errors that are appended to the AST by location so the compiler reports the one closer to the beginning of the file first. (ocaml-ppx/ppxlib#463, @NathanReb) - Update `Attribute.get` to ignore `loc_ghost`. (ocaml-ppx/ppxlib#460, @ceastlund) - Add API to manipulate attributes that are used as flags (ocaml-ppx/ppxlib#408, @dianaoigo) - Update changelog to use ISO 8061 date format: YYYY-MM-DD. (ocaml-ppx/ppxlib#445, @ceastlund) - Replace `Caml` with `Stdlib`. (ocaml-ppx/ppxlib#427, @ceastlund) - When a transformation raises, the last valid AST is used as input to the upcoming transformations. All such errors are collected and appended as extension nodes to the final AST (ocaml-ppx/ppxlib#447, @Burnleydev1) - Fix a small mistake in the man pages: Embededding errors is done by default with `-as-pp`, not with `-dump-ast` (ocaml-ppx/ppxlib#464, @pitag-ha) - Set appropriate binary mode when writing to `stdout` especially for Windows compatibility. (ocaml-ppx/ppxlib#466, @jonahbeckford)
CHANGES: - Add an optional `embed_errors` argument to `Context_free.map_top_down` that controls how to deal with exceptions thrown by context-free rules. (ocaml-ppx/ppxlib#468, @NathanReb) - Fix `Longident.parse` so it properly handles unparenthesized dotted operators such as `+.` or `*.`. (ocaml-ppx/ppxlib#111, @rgrinberg, @NathanReb) - raising an exception does no longer cancel the whole context free phase(ocaml-ppx/ppxlib#453, @Burnleydev1) - Sort embedded errors that are appended to the AST by location so the compiler reports the one closer to the beginning of the file first. (ocaml-ppx/ppxlib#463, @NathanReb) - Update `Attribute.get` to ignore `loc_ghost`. (ocaml-ppx/ppxlib#460, @ceastlund) - Add API to manipulate attributes that are used as flags (ocaml-ppx/ppxlib#408, @dianaoigo) - Update changelog to use ISO 8061 date format: YYYY-MM-DD. (ocaml-ppx/ppxlib#445, @ceastlund) - Replace `Caml` with `Stdlib`. (ocaml-ppx/ppxlib#427, @ceastlund) - When a transformation raises, the last valid AST is used as input to the upcoming transformations. All such errors are collected and appended as extension nodes to the final AST (ocaml-ppx/ppxlib#447, @Burnleydev1) - Fix a small mistake in the man pages: Embededding errors is done by default with `-as-pp`, not with `-dump-ast` (ocaml-ppx/ppxlib#464, @pitag-ha) - Set appropriate binary mode when writing to `stdout` especially for Windows compatibility. (ocaml-ppx/ppxlib#466, @jonahbeckford)
CHANGES: - Add an optional `embed_errors` argument to `Context_free.map_top_down` that controls how to deal with exceptions thrown by context-free rules. (ocaml-ppx/ppxlib#468, @NathanReb) - Fix `Longident.parse` so it properly handles unparenthesized dotted operators such as `+.` or `*.`. (ocaml-ppx/ppxlib#111, @rgrinberg, @NathanReb) - raising an exception does no longer cancel the whole context free phase(ocaml-ppx/ppxlib#453, @Burnleydev1) - Sort embedded errors that are appended to the AST by location so the compiler reports the one closer to the beginning of the file first. (ocaml-ppx/ppxlib#463, @NathanReb) - Update `Attribute.get` to ignore `loc_ghost`. (ocaml-ppx/ppxlib#460, @ceastlund) - Add API to manipulate attributes that are used as flags (ocaml-ppx/ppxlib#408, @dianaoigo) - Update changelog to use ISO 8061 date format: YYYY-MM-DD. (ocaml-ppx/ppxlib#445, @ceastlund) - Replace `Caml` with `Stdlib`. (ocaml-ppx/ppxlib#427, @ceastlund) - When a transformation raises, the last valid AST is used as input to the upcoming transformations. All such errors are collected and appended as extension nodes to the final AST (ocaml-ppx/ppxlib#447, @Burnleydev1) - Fix a small mistake in the man pages: Embededding errors is done by default with `-as-pp`, not with `-dump-ast` (ocaml-ppx/ppxlib#464, @pitag-ha) - Set appropriate binary mode when writing to `stdout` especially for Windows compatibility. (ocaml-ppx/ppxlib#466, @jonahbeckford)
No description provided.