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

Use the Lwt PPX inside Lwt #912

Open
MisterDA opened this issue Dec 6, 2021 · 5 comments
Open

Use the Lwt PPX inside Lwt #912

MisterDA opened this issue Dec 6, 2021 · 5 comments

Comments

@MisterDA
Copy link
Contributor

MisterDA commented Dec 6, 2021

Hi all,
I've been using Lwt intensively for quite some time now, and one of my pain points is the exception handling support, namely the erase of the backtraces which makes locating the origin of the exception extremely painful (and I love to complain about it). Would it make sense to convert the code of Lwt itself to using the ppx? That way backtraces would be preserved at least inside Lwt, and it could also promote the use of the ppx.

Considering that the issue tracking explicit backtrace handling (#720) hasn't seen any progress in the past couple of years, I'm thinking it could be a good temporary (for the next 2 years) tradeoff.

I have conflicted view about the ppx: it some cases it hurts readability (but come on, everything's better than >>= fun ->) but it's also closer to the direct-style. I would have liked to switch to binding operators too, but they don't offer the full feature set the ppx has. The killer feature being of course the improvement in usability.

I'd be willing to write the patch to transition, maybe in a couple of weeks. Would you be interested in it? I'd like to have an idea first, as not to waste time on sed scripts or refactoring ppx'es. Would there be any technical drawbacks to switching to the ppx?

Thanks!

@smorimoto
Copy link
Member

It sounds very nice to me. How about you? @raphael-proust

@raphael-proust
Copy link
Collaborator

I tend to prefer not using ppx but that's partly a personal preference. In fact, I have only three (hand-wave) rational (/hand-wave) and non-minor concerns:

  • It widens the area of expertise required from maintainers and contributors. True not everyone needs to understand ppx or even be able to modify it, but someone has to. Someones plural ideally so we can have a PR author and a PR reviewer. I'm not familiar enough with ppxes to be one of them at present.

    Who in amongst the maintainer has the background for working on ppxes? I suspect there will be sufficient numbers, but I'd rather check explicitly.

  • I remember some significant update issues with the release of newer OCaml version. Something about delays in ppxes being releasable because ppx lib had to be updated to account for new AST construct. Possibly some changes in ppxlib's or some other related tool's interface.

    How does that work in practice? When a new version of OCaml is released, what's our upgrade path? Are we able to release Lwt immediately following an OCaml release without waiting on some ppx-specific dependency? Are we able to not release Lwt immediately following an OCaml release without breaking some mutual compatibility with other ppx-using packages?

  • Is it ok to add the dependency cone of ppxlib to Lwt? In terms of co-installability, do we lose anything? Anything significant? I don't think that's an issue, but currently the Lwt dependencies are very few. In fact, it's just some shims for the core library (result, seq, ocplib-endian's bytes) and unix+threads for unix. We are adding a significant number of dependencies with the ppx.

These are my three concerns. They are not necessarily blocking, but I'd like to hear opinions on them.

@pitag-ha
Copy link

Hey, thanks for this discussion! To give my perspective (as a ppxlib maintainer) on the concerns raised:

True not everyone needs to understand ppx or even be able to modify it, but someone has to. Someones plural ideally

I agree here. To write and maintain a PPX in a central project like Lwt it's important to have some expertise, also because ppxlib is lacking some documentation on good standards and more advanced features (although we'll try to find time to improve our documentation). In case that helps: feel free to ping me whenever you have a PPX question/doubt.

I remember some significant update issues with the release of newer OCaml version.

I think you might be referring to the situation before the "Astlib + upgrade PPXs plan" (which we started with about a year ago). Is that possible? If you're referring to something more recent, I'd be very interested if you could give some more detail. The current situation is the following (there's more info on my discuss post from July):

We're collaborating with the OCaml release team and the opam team to make sure that the PPX ecosystem is always compatible with the newest release; e.g. 4.14 hasn't even been beta-released yet, but we're already releasing ppxlib.0.24.0 which is 4.14 compatible (PR submitted to opam-repository). Furthermore, with Astlib, soon ppxlib will always be compatible with OCaml trunk and hence any release. Our plan is to have that working from OCaml 5.1 on.

We still need to bump the ppxlib AST to the newest version frequently. I think that's what you have in mind with

Something about delays in ppxes being releasable because ppx lib had to be updated to account for new AST construct. Possibly some changes in ppxlib's or some other related tool's interface.

And it's true: that used to be a problem for all AST bumps before 4.10. The reason is the following: bumping the AST means breaking the part of the ppxlib API that low-level constructs or destructs the AST nodes that were extended in the new version. Hence, bumping the AST usually means breaking about a handful of PPXs. So what we've been doing when bumping the AST since the 4.10 bump is the following: We create a workspace with all PPXs (that are on opam and use dune) and send a patch PR to that handful of PPXs we break. I know that sounds crazy, but it's working well. There's also a discuss post from February with more detail on that.

When a new version of OCaml is released, what's our upgrade path?

Most of the time, you won't have to do anything. In the case that the new OCaml version touches an AST node that your PPX manipulates with ppxlib's low-level API, the opam team will add an upper ppxlib bound on LWT and we (the ppxlib team) will send you a few-lines patch PR that you can merge to be compatible again with the latest ppxlib version. It's not a big deal if that takes some months, since also the pen-ultimate ppxlib version is compatible with the latest compiler; it's just doesn't support its newest features.

Are we able to release Lwt immediately following an OCaml release without waiting on some ppx-specific dependency?

Yes.

Are we able to not release Lwt immediately following an OCaml release without breaking some mutual compatibility with other ppx-using packages?

Also yes (as explained in more detail two paragraphs above.)

Is it ok to add the dependency cone of ppxlib to Lwt?

Ppxlib only depends on ocaml, dune, ocaml-compiler-libs, stdlib-shims, sexplib0 and ppx_derivers, so I'd say yes (you might be remembering the times when ppxlib had more dependencies such as base and stdio, but we've dropped those).

@dinosaure
Copy link
Member

dinosaure commented Dec 10, 2021

Ppxlib only depends on ocaml, dune, ocaml-compiler-libs, stdlib-shims, sexplib0 and ppx_derivers, so I'd say yes (you might be remembering the times when ppxlib had more dependencies such as base and stdio, but we've dropped those).

I would like say to that sexplib0 and ppx_derivers is not one the ownership of the lwt team. For a project like lwt (as dune) less dependencies the project has, better the release cycle is.

Then, if the let%lwt really matters (even if I really that it's a subjective point of view), let*/let+ from OCaml exists and can be used instead of a lwt_ppx. If it's about the backtrace, I'm really sure that we should be able to provide a let* operator which use Lwt.backtrace_bind with some extra information such as __LINE__ or __FUNCTION__.

@raphael-proust
Copy link
Collaborator

I have converted Lwt_list, by hand, to the form that it would have with the ppx. I've pushed the experiment on https://github.com/raphael-proust/lwt/tree/experiment-with-backtrace-bind

It's clearly less readable. And unlike the ppx, the location might be somewhat inexact by reporting the reraise function. Still, it's not completely unreadable and the location should be accurate enough. Maybe we can increase the readability with first-class modules? Instead of let module Reraise = struct … end in Lwt.backtrace_bind (…) … we could write Lwt.backtrace_bind (module struct … end) …? Not sure it would work as well. I need to experiment with an actual backtrace.

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

No branches or pull requests

5 participants