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

Insert caught exception in place of the rewritten or generated item #472

Merged
merged 3 commits into from
Feb 13, 2024

Conversation

NathanReb
Copy link
Collaborator

This should wrap up the work on #448 !

When a context free rule raises, the exception was caught and turned into an error extension node prepended to the whole AST. This changes this behaviour to instead insert the error extension where the generated code would be, had the rule not raised.

When a context free rule raises, the exception was caught and turned
into an error extension node prepended to the whole AST.
This changes this behaviour to instead insert the error extension where
the generated code would be, had the rule not raised.

Signed-off-by: Nathan Rebours <[email protected]>
@NathanReb NathanReb force-pushed the insert-context-free-exception branch from 17a7fd5 to 667c002 Compare February 9, 2024 17:58
@NathanReb
Copy link
Collaborator Author

Fixed the build failure and enabled this behaviour for special functions and constants as well!

Copy link
Collaborator

@panglesd panglesd left a comment

Choose a reason for hiding this comment

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

Thanks! It will be great to be sure not to have an "unrewritten extension node" error displayed in a list of errors. And it's a very natural place to put the error in.

| EC.Signature_item -> Ast_builder.Default.psig_extension ~loc extension []
| EC.Structure_item -> Ast_builder.Default.pstr_extension ~loc extension []
| EC.Ppx_import ->
(* Insert the error in the type decl manifest *)
Copy link
Collaborator

Choose a reason for hiding this comment

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

We've had the same idea 🙂

This function could be in extension.ml, in my opinion, but I'm not even sure and that might be bikeshedding: feel free to ignore the suggestion...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I moved it in context_free because putting it in extension meant to make it public and I'm not sure we want to.

It is also only used for errors and is therefore very specific.

@NathanReb NathanReb merged commit fe4a387 into ocaml-ppx:main Feb 13, 2024
5 checks passed
@NathanReb NathanReb deleted the insert-context-free-exception branch February 13, 2024 13:24
NathanReb added a commit to NathanReb/opam-repository that referenced this pull request Apr 23, 2024
CHANGES:

- Add support for OCaml 5.2

- Insert errors from caught located exceptions in place of the code that
  should have been generated by context-free rules. (ocaml-ppx/ppxlib#472, @NathanReb)
NathanReb added a commit to NathanReb/opam-repository that referenced this pull request Apr 23, 2024
CHANGES:

- Add support for OCaml 5.2

- Insert errors from caught located exceptions in place of the code that
  should have been generated by context-free rules. (ocaml-ppx/ppxlib#472, @NathanReb)
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.

2 participants