Port the 3 ppx's to `ppxlib` #271

pitag-ha · 2020-06-26T10:31:04Z

Before, the 2 ppx rewriters tyxml-ppx and tyxml-jsx as well as the internal .ml-file generator in /syntax/ were based on omp: the former two on the omp driver and all of them on Ast_408. None of those will exist in omp 2.0.0 anymore. For more information on that omp change, see https://discuss.ocaml.org/t/ocaml-migrate-parsetree-2-0-0/5991 by @jeremiedimino.

This PR ports those 3 ppx's from the direct use of omp and tools like ppx_tools_versioned to ppxlib. It also adds a couple of small additions to the tyxml-ppx-tests. With this change, compiler-versions >= 4.04 are supported; before it was >= 4.02.

Closes ocaml-ppx/ppxlib#146.

@NathanReb , would you mind having a look at this?

Drup

The code looks okay-ish. I'll wait for ppxlib to be thinned down (is that done already ?) and closer to release before merging. I must admit, I'm still not on board with the whole "let's freeze the parsetree version, and dedicate someone to patch everyone's libraries when we upgrade", but let's assume I accept it for now.

I would like to keep 4.03, is that very difficult ?

Drup · 2020-06-29T21:42:13Z

syntax/reflect/reflect.ml


 let read_sig filename =
-  Location.input_name := filename ;


I suppose you have checked that the error locations are still good ?

In ppxlib, that part of the Location is handled differently (in particular, Location.input_name doesn't exist anymore). I haven't tested the error locations manually. I'll ask the rest of the ppxlib team about this.

Thanks for pointing me to that part of the code! I think that that particular line is not necessary, but there was another problem I hadn't noticed. I've fixed that now.

To test that everything is correct now, Ive provoked an exception by introducing an error into one of the .mli-files the .ml-files get generated from with this PPX. The location gets reported correctly now (including the .mli-filename). The locations inside the generated file will get picked up by the compiler when parsing it. Does that sound convincing?

Yes, that should be pretty good. In any case I realized it's on the reflect, which is not user-facing.

Drup · 2020-06-29T21:43:01Z

test/test_ppx.ml

+module Dummy_html = struct
+  include HtmlWrapped
+  let p = HtmlWrapped.a
+end


That test is very weird, it breaks the underlying assumption of the ppx. What is the objective here ?

I think this is just testing a feature of the ppx that was previously untested. I remember discussing it with @pitag-ha back then.

Could you be more specific ? Because that's a missfeature. The ppx use reflection to mirror the compile-time rules to the typing rules. If you rename elements like that, you break that underlying assumption.

If you replace the function, you are supposed to replace them by something with the same type.

Ok so just summarize, you don't have a problem with the test per-se but you'd prefer if it redefined a sensible p function?

Or to rephrase you'd prefer if it illustrated a proper use of of the [%html.<module name> ...] feature.

I'm not sure I understand what you think is a missfeature here.

Ah, if your goal is to test replacing the module (importantly, this module is supposed to respect the same signature), then sure, but we already do that.

About the objective of the test: yes, it's to test replacing the module. I've tried to make that clear with the commit message "Add tyxml-ppx tests: (...) open modules", but maybe the term "replace" would have been better here than "open". Do you want me to change that?

About respecting the module signature: Are you referring to respecting the signature of HtmlWrapped? If so, I think I do that, because p is a value in HtmlWrapped of the same type as a. So my intention was to shadow the definition of p with another definition of the same type.

Which are the tests you're referring to that already test replacing the module? Before writing this test, my changes already passed the tests without taking the replacing module feature into account. That's why I added it: I just wanted to make sure I wouldn't break that feature when integrating it with ppxlib. I added the test to the PR, because I thought it might also be helpful for others. But if you don't agree, I can get rid of it.

In test_ppx we use both Html and HtmlWrapped, so the fact that we can substitute the module is already tested, but it might only test in one direction. I'm curious how your changes made the test pass while breaking that feature.

p and a do not have the same types, so that substitution isn't valid. For something simple that covers the same need, I would prefer let a ~a (elt : Html_types.a_content_fun list) = Html.a ~a (txt "a link : " :: elt), which is always correct, and of the same type.

I'm curious how your changes made the test pass while breaking that feature.

I first ported the rest without implementing that feature by always passing None to get_modname, which (when implementing the feature) is only done if there's no module in the extension point name. I was expecting the tests to fail, but they passed.

For something simple that covers the same need, I would prefer let a ~a (elt : Html_types.a_content_fun list) = Html.a ~a (txt "a link : " :: elt)

That gives me the type error

Error: This expression has type string but an expression was expected of type string wrap

for "a link : ". Do you know how to solve it? Replacing it by Xml.W.return "a link : " gives the error

This expression has type Html_types.flow5_without_interactive list but an expression was expected of type ([> Html_types.txt ] as 'a) elt list

I pushed a fix for the test so that it behaves more to my linking, while still testing what you suggested.

Drup · 2020-06-29T21:43:35Z

ppx/dune

 (flags (:standard
         -safe-string
-         -open Migrate_parsetree
-         -open Ast_408
-         -open Ppx_tools_408


You could also open Ppxlib here ...

Ok, I've done that now. I've done the same in syntax/dune, syntax/reflect/dune and jsx/dune, since I have the impression that you prefer that style (and in those files, before Migrate_parsetree was opened). Is that correct?

pitag-ha · 2020-07-23T09:49:29Z

What are you referring to concretely when asking if ppxlib is already thinned down? Do you mean if the base dependency is already dropped? If so: yes, in master that's already done.

I don't know how much work it would be to make ppxlib compatible with 4.03, but I imagine it would be quite a bit of work. I'll ask the rest of the team. What's the reason why you'd like to keep that specific version?

Your overall feedback "the code looks okay-ish" has given me the impression that you're not too happy with the overall OCaml code quality. If you point me to concrete parts that you find worth improving, I'm happy to improve them and happy to learn.

Drup · 2020-07-23T11:39:39Z

What are you referring to concretely when asking if ppxlib is already thinned down? Do you mean if the base dependency is already dropped? If so: yes, in master that's already done.

That is indeed what I meant.

I don't know how much work it would be to make ppxlib compatible with 4.03, but I imagine it would be quite a bit of work. I'll ask the rest of the team. What's the reason why you'd like to keep that specific version?

I prefer to keep compatibility when I can, it's not a very strong requirement for such old versions.

Your overall feedback "the code looks okay-ish" has given me the impression that you're not too happy with the overall OCaml code quality. If you point me to concrete parts that you find worth improving, I'm happy to improve them and happy to learn.

The code isn't bad at all, just not necessarily written according to my style, and since I'm the most likely to maintain it, I prefer if it fits the way I like. I'll simply rewrite bits to my preferences. I'll take a more thorough look once we merge this when the new ppxlib get released.

avsm · 2021-04-15T14:42:15Z

@Drup would you have time to look at this soon to get it merged and released? I'm seeing a number of problems in opam-repository with universe splits from tyxml and the new ppxlib, and this PR would resolve them.

Drup · 2021-04-15T15:07:58Z

Arg, thanks for reminding me. I have a deadline early next week, I'll work on this after that.

Drup

Ok, I have read everything, and made various changes to my liking, which I have pushed here. I do like the fact that it's finally using proper objects to implement open recursion instead of the previous clumsy encoding.

If one of the ppxlib people is willing, I wouldn't mind a "counter-review" to check that what I did is indeed correct wrt ppxlib. Otherwise, I'll merge everything.

Thanks @pitag-ha for doing the first version and @aantron and @avsm for pinging me.

Drup · 2021-04-21T12:58:57Z

jsx/tyxml_jsx.ml

+      | _ -> super#expression e
+
+  method! expression =
+    self#expr_mapper c


Is there any reason to have two functions like that, instead of renaming expr_mapper into expression ?

In the end, I moved everything to a nice object-based visitor, it seems to work as expected.

Drup · 2021-04-21T14:28:13Z

syntax/reflect/reflect.ml


 let read_sig filename =
-  Location.input_name := filename ;


Yes, that should be pretty good. In any case I realized it's on the reflect, which is not user-facing.

Drup · 2021-04-21T14:37:55Z

test/test_ppx.ml

+module Dummy_html = struct
+  include HtmlWrapped
+  let p = HtmlWrapped.a
+end


I pushed a fix for the test so that it behaves more to my linking, while still testing what you suggested.

Drup · 2021-04-21T14:59:27Z

I almost forgot: is it now possible to have expect tests with the ppx ? I tried previously (there are various stuff commented in the test directory), but it was much more difficult than it should.

Before, the 2 ppx rewriters `tyxml-ppx` and `tyxml-jsx` as well as the internal `.ml`-file generator in /syntax/ were based on `omp`: the former two on the `omp` driver and all of them on `Ast_408`. None of those will exist in `omp 2.0.0` anymore. This commit ports those 3 ppx's from the direct use of `omp` and tools like `ppx_tools_versioned` to `ppxlib`.

Drup · 2021-04-22T09:06:17Z

(I'm merging now, as this will allow to upgrade my switches to recent ppxlib&co and move forward, but I still welcome reviews/answers, especially regarding the expect tests with a ppx)

@pitag-ha

CHANGES: * Move all the PPXs to ppxlib (ocsigen/tyxml#271, Initial code by Sonja @pitag-ha Heinze) * Add the `translate` attribute (ocsigen/tyxml#281 by Javier @jchavarri Chávarri) * Update allowed `inputmode`s (ocsigen/tyxml#279 by Joel @joelburget Burget) * Add the `picture` element (ocsigen/tyxml#263 by Stéphane @slegrand45 Legrand)

CHANGES: * Move all the PPXs to ppxlib (ocsigen/tyxml#271, Initial code by Sonja Heinze) * Add the `translate` attribute (ocsigen/tyxml#281 by Javier Chávarri) * Update allowed `inputmode`s (ocsigen/tyxml#279 by Joel Burget) * Add the `picture` element (ocsigen/tyxml#263 by Stéphane Legrand)

@pitag-ha

CHANGES: * Move all the PPXs to ppxlib (ocsigen/tyxml#271, Initial code by Sonja @pitag-ha Heinze) * Add the `translate` attribute (ocsigen/tyxml#281 by Javier @jchavarri Chávarri) * Update allowed `inputmode`s (ocsigen/tyxml#279 by Joel @joelburget Burget) * Add the `picture` element (ocsigen/tyxml#263 by Stéphane @slegrand45 Legrand)

@pitag-ha

CHANGES: * Move all the PPXs to ppxlib (ocsigen/tyxml#271, Initial code by Sonja @pitag-ha Heinze) * Add the `translate` attribute (ocsigen/tyxml#281 by Javier Chávarri) * Update allowed `inputmode`s (ocsigen/tyxml#279 by Joel Burget) * Add the `picture` element (ocsigen/tyxml#263 by Stéphane Legrand)

CHANGES: * Move all the PPXs to ppxlib (ocsigen/tyxml#271, Initial code by Sonja Heinze) * Add the `translate` attribute (ocsigen/tyxml#281 by Javier Chávarri) * Update allowed `inputmode`s (ocsigen/tyxml#279 by Joel Burget) * Add the `picture` element (ocsigen/tyxml#263 by Stéphane Legrand)

pitag-ha · 2021-04-23T15:49:27Z

is it now possible to have expect tests with the ppx ?

What are you referring to? Do you want to test tyxml-ppx with expect tests? Is something in ppxlib making that difficult?

Drup · 2021-04-23T16:35:14Z

What are you referring to? Do you want to test tyxml-ppx with expect tests? Is something in ppxlib making that difficult?

Yes, I would like to, to test failures and locations in particular. It used to be quite tricky to do with dune. There is an attempt in the test directory that I commented out because it was too unstable. Is it easier now ?

pitag-ha · 2021-04-23T17:59:04Z

Could you use dune cram tests for that? In ppxlib we use cram tests to test failures and locations (see e.g. here). Or do you need to do something more specific?

pitag-ha force-pushed the ppxlib branch 2 times, most recently from 3464071 to ba11978 Compare June 26, 2020 15:59

Drup reviewed Jul 22, 2020

View reviewed changes

pitag-ha force-pushed the ppxlib branch 2 times, most recently from 6651c97 to 18bfbd2 Compare July 31, 2020 15:45

Drup force-pushed the ppxlib branch 2 times, most recently from bc4e81f to 3a11e37 Compare April 21, 2021 14:52

Drup approved these changes Apr 21, 2021

View reviewed changes

pitag-ha and others added 6 commits April 21, 2021 18:21

Add tyxml-ppx tests: namespace and open modules

b69a15a

Fix the dummy_module test to respect Tyxml's invariants.

f290b9a

Actually use objects in the JSX.

7146fff

Simplify some ppxlib code.

b55debf

Cleanup/use objects in reflect.

03d637f

Drup force-pushed the ppxlib branch from 3a11e37 to 03d637f Compare April 21, 2021 16:32

Drup added 2 commits April 21, 2021 18:51

Fix for recent ppxlib versions.

244226c

Stop testing 4.03

9c99d53

Drup merged commit 6c9dd5b into ocsigen:master Apr 22, 2021

Drup mentioned this pull request Apr 22, 2021

[new release] tyxml-ppx, tyxml-syntax, tyxml and tyxml-jsx (4.5.0) ocaml/opam-repository#18548

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port the 3 ppx's to `ppxlib` #271

Port the 3 ppx's to `ppxlib` #271

pitag-ha commented Jun 26, 2020 •

edited

Loading

Drup left a comment •

edited

Loading

Drup Jun 29, 2020

pitag-ha Jul 23, 2020

pitag-ha Jul 31, 2020 •

edited

Loading

Drup Apr 21, 2021

Drup Jun 29, 2020

NathanReb Jul 22, 2020

Drup Jul 22, 2020

NathanReb Jul 22, 2020

NathanReb Jul 22, 2020

Drup Jul 22, 2020

pitag-ha Jul 23, 2020

Drup Jul 23, 2020

pitag-ha Jul 27, 2020 •

edited

Loading

Drup Apr 21, 2021

Drup Jun 29, 2020

pitag-ha Jul 31, 2020

pitag-ha commented Jul 23, 2020

Drup commented Jul 23, 2020

avsm commented Apr 15, 2021

Drup commented Apr 15, 2021

Drup left a comment •

edited

Loading

Drup Apr 21, 2021

Drup Apr 21, 2021

Drup Apr 21, 2021

Drup Apr 21, 2021

Drup commented Apr 21, 2021

Drup commented Apr 22, 2021 •

edited

Loading

pitag-ha commented Apr 23, 2021

Drup commented Apr 23, 2021

pitag-ha commented Apr 23, 2021

Port the 3 ppx's to ppxlib #271

Port the 3 ppx's to ppxlib #271

Conversation

pitag-ha commented Jun 26, 2020 • edited Loading

Drup left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitag-ha Jul 31, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitag-ha Jul 27, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pitag-ha commented Jul 23, 2020

Drup commented Jul 23, 2020

avsm commented Apr 15, 2021

Drup commented Apr 15, 2021

Drup left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Drup commented Apr 21, 2021

Drup commented Apr 22, 2021 • edited Loading

pitag-ha commented Apr 23, 2021

Drup commented Apr 23, 2021

pitag-ha commented Apr 23, 2021

Port the 3 ppx's to `ppxlib` #271

Port the 3 ppx's to `ppxlib` #271

pitag-ha commented Jun 26, 2020 •

edited

Loading

Drup left a comment •

edited

Loading

pitag-ha Jul 31, 2020 •

edited

Loading

pitag-ha Jul 27, 2020 •

edited

Loading

Drup left a comment •

edited

Loading

Drup commented Apr 22, 2021 •

edited

Loading