Allowing multiple errors to be reported in one pass of the context_free phase #453

Burnleydev1 · 2023-08-03T13:59:28Z

No description provided.

panglesd

Making good progress!

I reviewed only the "Catching extenders' exceptions". Except for minor comments, it looks good!

The embed_errors value is not passed from the driver yet, which is probably why the tests do not work yet...

src/context_free.ml

panglesd · 2023-08-04T12:58:14Z

src/context_free.ml

+      (try
+         E.For_context.convert_res ts ~ctxt ext
+         |> With_errors.of_result ~default:None
+       with exn when embed_errors -> (None, [ exn_to_error exn ]))


I think it makes sense to catch the exception in convert_res rather than here. One would not expect that a *_res function which returns a result type raises a located exception (the res suffix is for "result", and the result type is often used to have "more explicit errors". Here it would be a combination of both...)

That would also factor some of the code here!

I guess one problem here is we only catch when embed_errors is true. To preserve that behaviour we'd need to propagate the embed_errors argument to convert_res which I'm not sure is a good idea.

It's also unclear to me what would be the best way to deal with an exception from the actual rewriting function here. The errors returned by convert are Ast_pattern parsing errors which are slightly different from the ones we're catching here.

src/context_free.ml

test/driver/exception_handling/run.t

panglesd

Nice progress on the "constant" rewriting!

I left some small comments.

src/context_free.ml

test/driver/exception_handling/run.t

CHANGES.md

panglesd

Nice improvements! Only the "special function" rewriting is missing.

I left some comments.

src/context_free.ml

panglesd

A small batch of comments!

src/context_free.ml

test/driver/exception_handling/special_functions.ml

Burnleydev1 · 2023-08-22T11:07:56Z

Thank you @panglesd, I have implemented the changes requested.

NathanReb

This looks good, great change!

Do we really need to tie this to embed_errors? Are there good reasons not to do this? It's already on by default when used through dune if I'm not mistaken. I guess this could break tests that rely on the output of the driver. Do you have any other uses cases in mind?

CHANGES.md

NathanReb · 2024-01-11T10:23:11Z

The tests are broken it seems. The error extension points are printed backward and some locations were off when running the driver without -embed-errors.

I think this comes from a change in #447. I suggest we sort errors by location before inserting them in a separate PR so that the order of errors is a bit more reliable than what it is right now.

…ee transformation. Signed-off-by: Burnleydev1 <[email protected]>

Signed-off-by: Burnleydev1 <[email protected]>

…ontext_free transformation. Signed-off-by: Burnleydev1 <[email protected]>

Signed-off-by: Burnleydev1 <[email protected]>

… errors Signed-off-by: Burnleydev1 <[email protected]>

Signed-off-by: Burnleydev1 <[email protected]>

Signed-off-by: Nathan Rebours <[email protected]>

NathanReb · 2024-01-30T11:47:31Z

This should be good to go except for a CI failure with tests on 4.05 and 4.04, I'm looking into it.

We discussed a slightly different approach regarding context free transformations throwing exceptions where we would inject the error node directly in the AST in place of the node that needed to be rewritten instead of leaving the original node and inserting the error at the beginning of the AST. I still believe this would be a cleaner solution but at the moment I think the solution we have is good enough to be merged. We can still improve this later on.

Signed-off-by: Nathan Rebours <[email protected]>

NathanReb · 2024-01-30T12:06:28Z

Should be good now!

panglesd

Thanks @NathanReb for the final work on this PR! I believe it is ready to be merged.

About the other approach of replacing extension nodes by error nodes in case of exceptions: I agree this is a very valid approach, and would definitely accept a PR with the changes. I would also think the current situation is good enough.

For reference, here are the pros and cons of each approach, as discussed in a dev meeting (quoting from the notes):

[Main advantages of current approach:] Coherence with error collecting from other phases, less potential for later coming whole-file transformations to mess up embedded errors. We all agree that the latter shouldn't be taken too much into consideration.
[Main advantages of replacing the extension node:] Coherence with what we ask PPX authors to do (embed the errors themselves) and no duplication of error messages in case of extension nodes, where the compiler will also emit an error if the node doesn't get replaced.

@rgrinberg

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)

This extra argument that was added in ocaml-ppx#453 caused breakage in `ocsigen-i18n` that was using `Context_free.map_top_down`. Although it's undocumented, this function is still part of the public API. Adding an extra optional argument is not strictly speaking non-breaking but is good enough of an effort considering the status of this function and in particular, the fact that it only has a single public external user for which it prevents breakage. Signed-off-by: Nathan Rebours <[email protected]>

@NathanReb

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)

@NathanReb

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)

Burnleydev1 marked this pull request as ready for review August 4, 2023 12:41

Burnleydev1 requested review from ceastlund, panglesd and pitag-ha as code owners August 4, 2023 12:41

panglesd reviewed Aug 4, 2023

View reviewed changes

panglesd reviewed Aug 9, 2023

View reviewed changes

src/context_free.ml Outdated Show resolved Hide resolved

src/context_free.ml Outdated Show resolved Hide resolved

test/driver/exception_handling/run.t Outdated Show resolved Hide resolved

CHANGES.md Outdated Show resolved Hide resolved

panglesd reviewed Aug 13, 2023

View reviewed changes

src/context_free.ml Outdated Show resolved Hide resolved

src/context_free.ml Outdated Show resolved Hide resolved

panglesd reviewed Aug 22, 2023

View reviewed changes

panglesd mentioned this pull request Oct 24, 2023

Update documentation with new behaviour wrt exceptions #457

Merged

panglesd mentioned this pull request Jan 8, 2024

Driver: Multiple errors can be reported. #447

Merged

NathanReb reviewed Jan 10, 2024

View reviewed changes

CHANGES.md Outdated Show resolved Hide resolved

NathanReb force-pushed the context-free branch from a83582c to df9cc61 Compare January 11, 2024 10:15

NathanReb mentioned this pull request Jan 11, 2024

Sort errors by location before appending them to the AST #463

Merged

Burnleydev1 added 15 commits January 17, 2024 16:22

Allowing multiple errors to be reported in one pass of the context_fr…

c82fedf

…ee transformation. Signed-off-by: Burnleydev1 <[email protected]>

minor updates

637bef7

Signed-off-by: Burnleydev1 <[email protected]>

add changlog

5e45c05

Signed-off-by: Burnleydev1 <[email protected]>

Added -embed-error flag

f513df0

Signed-off-by: Burnleydev1 <[email protected]>

improving errors in the context-free phase: Deriver

263f8b6

Signed-off-by: Burnleydev1 <[email protected]>

proress: Allowing multiple errors to be reported in one pass of the c…

1a97108

…ontext_free transformation. Signed-off-by: Burnleydev1 <[email protected]>

remove unused changes.

5ad150d

Signed-off-by: Burnleydev1 <[email protected]>

In progress: constant_type and special_functions can report multiple…

ad2fece

… errors Signed-off-by: Burnleydev1 <[email protected]>

In progress: implemented requested changes.

cabaf58

Signed-off-by: Burnleydev1 <[email protected]>

In progress: implemented requested changes.

32c0dd6

Signed-off-by: Burnleydev1 <[email protected]>

In progress: implemented requested changes.

a7ba230

Signed-off-by: Burnleydev1 <[email protected]>

minor changes

1f06a4d

Signed-off-by: Burnleydev1 <[email protected]>

create fix value for embed_error in map_node and attrs.

8609b4b

Signed-off-by: Burnleydev1 <[email protected]>

Added some test for special functions

becbc48

Signed-off-by: Burnleydev1 <[email protected]>

improvements on special functions

b1855e9

Signed-off-by: Burnleydev1 <[email protected]>

Burnleydev1 and others added 4 commits January 17, 2024 16:22

applying requested changes.

8561870

Signed-off-by: Burnleydev1 <[email protected]>

Fix failing test

30c9913

Signed-off-by: Nathan Rebours <[email protected]>

Remove unwanted empty line at the end of CHANGES.md

5e09949

Signed-off-by: Nathan Rebours <[email protected]>

Factor out exn_to_error

d73ae80

Signed-off-by: Nathan Rebours <[email protected]>

NathanReb force-pushed the context-free branch from df9cc61 to d73ae80 Compare January 30, 2024 11:10

Polish context-free exception test

c3f3c7a

Signed-off-by: Nathan Rebours <[email protected]>

NathanReb force-pushed the context-free branch from adefd8e to c3f3c7a Compare January 30, 2024 12:06

NathanReb requested a review from panglesd January 30, 2024 12:06

panglesd approved these changes Jan 31, 2024

View reviewed changes

NathanReb merged commit edc51a4 into ocaml-ppx:main Jan 31, 2024
5 checks passed

NathanReb mentioned this pull request Feb 1, 2024

[new release] ppxlib (0.32.0) ocaml/opam-repository#25152

Closed

NathanReb mentioned this pull request Feb 2, 2024

Make Context_free.map_top_down's embed_errors arg optional #468

Merged

NathanReb mentioned this pull request Feb 5, 2024

[new release] ppxlib (0.32.0) ocaml/opam-repository#25179

Merged

Burnleydev1 deleted the context-free branch June 9, 2024 09:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allowing multiple errors to be reported in one pass of the context_free phase #453

Allowing multiple errors to be reported in one pass of the context_free phase #453

Burnleydev1 commented Aug 3, 2023

panglesd left a comment

panglesd Aug 4, 2023

NathanReb Jan 11, 2024

NathanReb Jan 11, 2024

panglesd left a comment

panglesd left a comment

panglesd left a comment

Burnleydev1 commented Aug 22, 2023

NathanReb left a comment

NathanReb commented Jan 11, 2024

NathanReb commented Jan 30, 2024

NathanReb commented Jan 30, 2024

panglesd left a comment

Allowing multiple errors to be reported in one pass of the context_free phase #453

Allowing multiple errors to be reported in one pass of the context_free phase #453

Conversation

Burnleydev1 commented Aug 3, 2023

panglesd left a comment

Choose a reason for hiding this comment

panglesd Aug 4, 2023

Choose a reason for hiding this comment

NathanReb Jan 11, 2024

Choose a reason for hiding this comment

NathanReb Jan 11, 2024

Choose a reason for hiding this comment

panglesd left a comment

Choose a reason for hiding this comment

panglesd left a comment

Choose a reason for hiding this comment

panglesd left a comment

Choose a reason for hiding this comment

Burnleydev1 commented Aug 22, 2023

NathanReb left a comment

Choose a reason for hiding this comment

NathanReb commented Jan 11, 2024

NathanReb commented Jan 30, 2024

NathanReb commented Jan 30, 2024

panglesd left a comment

Choose a reason for hiding this comment