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

Make the ppx's driver aware #2079

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

Conversation

rgrinberg
Copy link
Contributor

The current ppx's only act as classical ppx's which make them unusable for all
dune users. The situation is easily fixed by defining the rewriters using dune's
ppx_rewriter kind.

This will create a ppx library that will work when linked into a driver and a
standalone ppx.exe rewriter for use in classical ppx. findlib's tags are used to
distinguish the 2 cases (see the ppx_driver tag).

The only "problem" with this transition is that every ppx needs a valid library
name. We could maintain the old names, but then we'd have to introduce toplevel
packages for them. Instead, we just put these ppx rewriters under the reason
pacakge.

Signed-off-by: Rudi Grinberg rudi.grinberg@gmail.com

The current ppx's only act as classical ppx's which make them unusable for all
dune users. The situation is easily fixed by defining the rewriters using dune's
ppx_rewriter kind.

This will create a ppx library that will work when linked into a driver and a
standalone ppx.exe rewriter for use in classical ppx. findlib's tags are used to
distinguish the 2 cases (see the ppx_driver tag).

The only "problem" with this transition is that every ppx needs a valid library
name. We could maintain the old names, but then we'd have to introduce toplevel
packages for them. Instead, we just put these ppx rewriters under the reason
pacakge.

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg
Copy link
Contributor Author

Btw, this will of course also make all the .merlin hacks unnecessary. dune will know to generate the correct .merlin after this change.

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg
Copy link
Contributor Author

Btw, I suppose I can also get 100% compatibility with master by defining the old exe's as manual drivers. Quite ugly, but possibly worth it. @jordwalke what do you think?

@jaredly
Copy link
Contributor

jaredly commented Jul 19, 2018

Why is it ugly to make the old exe's manual drivers?

Signed-off-by: Rudi Grinberg <rudi.grinberg@gmail.com>
@rgrinberg
Copy link
Contributor Author

I added a commit as an example of how to do it for ppx_react. The rest are done similarly. I guess it's not so ugly, just completely pointless. Those who insist on using classical ppx can turn the rewriter with -package reason.ppx_react like they would for any classical ppx.

@chenglou
Copy link
Member

Hey! Thanks, but we were actually planning to fuse the ppx and refmt (optionally expose it as a flag of refmt). ppx_react is also dead. I'll hold this PR for now, but thanks for looking out for this

@jordwalke
Copy link
Member

jordwalke commented Jul 20, 2018

We might still benefit from making these ppx processors compatible with ppx drivers, even when we fuse the ppx into refmt binary. Fusing means to enable a ppx you just refmt the file but pass a flag to refmt --run-the-built-in-ppxs.

Fusing the ppx into refmt has two benefits:

  1. Makes the download size smaller when people install refmt through a prebuilt global binary.
  2. Avoids having to write out intermediate files to first run refmt and then run the ppxs.

Number one doesn't benefit everyone, but it's still a nice feature to have for those who do just install a global binary. Number two is more interesting. ppx_driver runs a bunch of ppxs in one pass. It's like by fusing refmt and the jsx ppxs, we're making a little ppx driver just for reason. I wish there was a way to run ppx drivers and refmt in a single pass (but note that refmt is not a ppx plugin - it's a parser). Either way we can still fuse.
I'm thinking that even if we fuse them together, there might be no harm in having them also support ppx_drivers. I was doing some local development on the jsx ppx itself and having support for the jsx ppx playing well with ppx drivers would have made things way simpler for my workflow. I would have just disabled the fusing and just used it as a regular ppx driver compatible plugin.

@jordwalke
Copy link
Member

@rgrinberg note that -package reason.ppx_react is not an option for the workflows that use the ppx plugins in the "old ppx mode". It needs to be supplied just like -ppx reactjs_ppx. Is that a problem? Is it possible to make an executable that can also act as an argument to -ppx ?

@rgrinberg
Copy link
Contributor Author

Is that a problem? Is it possible to make an executable that can also act as an argument to -ppx ?

It's not a problem in the sense that it is easily solved by making the standalone shim binaries like I did for ppx_react. I could do it for the rest of the binaries as well.

Also, let's not forget the benefit of making these rewriters available for normal OCaml users who still like the traditional syntax.

Finally, you should consider the value that ppxlib provides in the long term when it's ready. It would make defining simple ppx's such as this far more trivial. Would be nice to keep this option if you don't plan to take it for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants