Skip to content

ppx: upgrade to ppxlib 0.36 #1866

New issue

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

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

Already on GitHub? # to your account

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

anmonteiro
Copy link

  • Upgrades the JSOO ppxes to ppxlib 0.36 with Bump AST to OCaml 5.2.0 ocaml-ppx/ppxlib#514
  • I realize ppx_expect and its transitive deps need to be upgraded first before this change can be considered, but I'm opening this up to unblock some of my PRs in other repos (this builds fine if you don't run the tests)

@smorimoto smorimoto requested a review from hhugo March 11, 2025 11:10
@smorimoto smorimoto added no changelog dependencies Pull requests that update a dependency file labels Mar 11, 2025
@hhugo hhugo force-pushed the anmonteiro/ppxlib-0.36-ast-5.2 branch from 99efd3f to ea844f1 Compare March 18, 2025 14:07
Copy link
Member

@hhugo hhugo left a comment

Choose a reason for hiding this comment

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

Blocked until the CI goes green

@hhugo hhugo added the blocked label Mar 18, 2025
| { pparam_desc = Pparam_val (label, _, _arg); _ } ->
Some (Arg.make ~label ()))
in
params @ create_meth_ty body
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we need to recurse here.

Copy link
Author

Choose a reason for hiding this comment

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

@hhugo the reason why I added recursion was for cases like this:

class x =
  object
    method foo a = fun b -> a + b
  end

due to ocaml/ocaml#12236 preserving both Pexp_function nodes in the AST. I'm not very familiar with how JSOO handles this case, so I trust you to know whether it's unnecessary.

| Pexp_function (params, _, Pfunction_body body) ->
let params =
List.filter_map params ~f:(function
| { pparam_desc = Pparam_newtype _; _ } -> None
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that this is what we want. I'll have to look futher

@hhugo hhugo force-pushed the anmonteiro/ppxlib-0.36-ast-5.2 branch 2 times, most recently from 6fb1a5d to 3f4d8a2 Compare March 24, 2025 22:19
@hhugo hhugo force-pushed the anmonteiro/ppxlib-0.36-ast-5.2 branch from 3f4d8a2 to afbfc00 Compare March 26, 2025 15:07
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
blocked dependencies Pull requests that update a dependency file no changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants