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

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

Merged
merged 1 commit into from
Jan 17, 2024

Conversation

NathanReb
Copy link
Collaborator

This may seem like an unnecessary feature since the whole point of embedding errors is to allow reporting them all at once. This works with merlin but not with the compiler which will only report the first one it encounters.

You'll note I kept the List.rev that was added in #447 so that errors with the same loc will appear in the preserved transformation order. That shouldn't impact a lot of real life cases though.

This will help fixing #453 's tests as well.

Note that the cookies and checks error will still be appended before the transformation errors so they won't be displayed by the compiler if there is one transformation error, even though they may be located closer to the beginning of the file.
I'm happy to simply collect all such errors together, sort them and then append them to AST as one common batch of errors if you think this is okay and won't break anything.

@NathanReb NathanReb force-pushed the sort-embedded-errors branch 2 times, most recently from 0f527d2 to a8a8caf Compare January 11, 2024 13:21
Copy link
Member

@pitag-ha pitag-ha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That does sound good to me, but I'm a bit out of context: Why do the errors get sorted badly in the first place? I'm expecting them to be added to the list of errors in the order in which they're encountered when traversing the AST. That might be a different order than the one you're suggesting, but it would make it coherent with what happens when there's no ppx transformation.

CHANGES.md Outdated Show resolved Hide resolved
@panglesd
Copy link
Collaborator

I think the reasoning was that errors that happen in an earlier stage of the transformation process should be reported first. Since the OCaml compiler shows only the first error in the AST, that was a way to do that.
If that is not the case, that's not intended!

However, I think that's a good idea to sort the errors by location. It makes it easier to have the right order, and is another natural order for the errors. So, I'm in favor of this PR!

This may seem like an unnecessary feature since the whole point
of embedding errors is to allow reporting them all at once. This works
with merlin but not with the compiler which will only report the first
one it encounters.

Signed-off-by: Nathan Rebours <[email protected]>
@NathanReb
Copy link
Collaborator Author

#447 introduced a List.rev on the collected errors. That's because errors were being appended to a list as they were collected, meaning it was in reverse order, first error collected was last in the list. As it allowed multiple passes to take place even though previous transformation may have failed, it made sense to try and get the errors in the correct order, or at least, in the same order as transformations were being applied.

In #453, we allow context-free transformations to raise without canceling all subsequent context-free transformations. It seems that the above mentioned List.rev messes with how errors are collected and accumulated in the context-free pass as it's been swapping the order of those errors which were previously correctly ordered.

At this point it's hard to predict how errors will be ordered and so I decided to sort them by locations because that's likely the most meaningful order. Does that sound good to you?

@NathanReb NathanReb merged commit 98fb4d1 into ocaml-ppx:main Jan 17, 2024
5 checks passed
@NathanReb NathanReb deleted the sort-embedded-errors branch January 17, 2024 15:07
NathanReb added a commit to NathanReb/opam-repository that referenced this pull request Feb 1, 2024
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)
NathanReb added a commit to NathanReb/opam-repository that referenced this pull request Feb 5, 2024
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)
nberth pushed a commit to nberth/opam-repository that referenced this pull request Jun 18, 2024
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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants