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

ppx_import + ppxlib based deriver applied in the "wrong" order #1861

Closed
NathanReb opened this issue Feb 21, 2019 · 10 comments
Closed

ppx_import + ppxlib based deriver applied in the "wrong" order #1861

NathanReb opened this issue Feb 21, 2019 · 10 comments

Comments

@NathanReb
Copy link
Collaborator

I recently started working on a new ppxlib based deriver: ppx_factory. As I was trying to use it conjointly with ppx_import I ran into an unsettling error. The error seems to indicate that the deriver is applied before ppx_import's rewriter somehow.

I managed to reproduce it in a minimal example. Consider the following a.ml file:

type t =
  | A of int
  | B of string

the following b.ml file:

type u = [%import: A.t]
[@@deriving factory]

and finally the following dune file:

(library
  (name main)
  (public_name main)
  (preprocess
    (staged_pps
      ppx_factory
      ppx_import
    )
  )
)

Building the library fails with:

$ dune build
    ocamldep .main.objs/b.ml.d (exit 2)
(cd _build/default && /home/nathan/.opam/4.07.1/bin/ocamldep.opt -modules -ppx '.ppx/143b9244e7f4bba18422ab05df463b8c/ppx.exe --as-ppx --cookie '\''library-name="main"'\''' -impl b.ml) > _build/default/.main.objs/b.ml.d
File "b.ml", line 1, characters 0-44:
Error: ppx_factory.factory: cannot derive from abstract type. Has to be a record or variant type.

I double checked my code to make sure the error doesn't come from a bug in ppx_factory but it seems not.

What disturbs me the most is that merlin is happy with that when I open b.ml in my editor and autocompletion even offers me the values generated by ppx_factory.

I'm not sure whether this should be reported here or in ppxlib. I may have misused ppxlib somehow but that strange difference between merlin and dune made me think there was something wrong going on unrelated to my implementation.

Do you have any idea what it might be?

@ejgallego
Copy link
Collaborator

ppx_import uses an order parameter to try to run before derivers:

https://github.com/ocaml-ppx/ppx_import/blob/0f8976954ed9ce51b18d36365001ce4574f81b66/src/ppx_import.ml#L461-L462

Indeed maybe this is not working well with ppxlib. Should we port ppx_import to ppxlib?

@NathanReb
Copy link
Collaborator Author

I just tried with another ppxlib based deriver, ppx_compare and it seems to work just fine so I guess the bug is on my end, I'll have a deeper look!

@ghost
Copy link

ghost commented Feb 21, 2019

Normally it should work because ppxlib integrates itself with the omp driver. This is all horribly complicated but it's supposed to work.

It is definitely surprising that it works with merlin but not for normal compilation though. I'm puzzled as to what is going on.

@ghost
Copy link

ghost commented Feb 21, 2019

Hmm, one thing to consider is that ppx_import behaves differently when called from ocamldep. This is because at this stage the cmi of the other modules don't exist so it cannot call the typer. IIRC, ppx_import generates a dummy type, maybe that dummy type is something that is not understood by ppx_factory?

@NathanReb
Copy link
Collaborator Author

I will have a look at that.

In my original encounter with this issue, the type that was imported was from a separate library, would that change anything with regards to the potential issue you're describing?

@ghost
Copy link

ghost commented Feb 21, 2019

Nope. I just had a look at ppx_import.ml, and indeed when called by ocamldep it replaces type t = [%import A.x] by either just type t or type t = A.x.

@NathanReb
Copy link
Collaborator Author

Hmm, I guess I'm a little confused. What would be the point of using ppx_import in such a case and how am I supposed to get around that so that my deriver is applied to the actual "copied" type definition ?

ppx_compare seems to be handling that just fine but I'm unsure what I'm doing wrong. Should it silently do nothing on abstract types so that it's then re-applied at a later compilation stage?

I guess this is unrelated to dune so feel free to close the issue! I'll eventually open a new issue on ppxlib referencing this one.

@ghost
Copy link

ghost commented Feb 21, 2019

The problem is that ppx_import cannot perform the expansion of [%import A.x] when called from ocamldep. However, it still needs to produce some AST since ocamldep doesn't inspect extension points. In particular, it needs to produce an AST that references A so that ocamldep discover the dependency on A. That's why it generates A.x. However, when later called by ocamlc/ocamlopt, ppx_import will correctly expand [%import A.x] to the right thing.

It appears to work with ppx_compare because [@@deriving compare] is allowed on abstract types. There might be situations in which some valid dependencies will be missed and the behaviour will be unpredictable.

To work around this, you should probably accept abstract types in ppx_factory. Or at least accept them only when called from ocamldep.

@NathanReb
Copy link
Collaborator Author

Ok that makes much more sense now.

Indeed returning an empty list of str_items appeared to solve the issue in my case. I'll figure something out to properly deal with abstract types in such cases.

I feel like this might be a worthy addition to ppxlib's documentation but it's a bit scarse atm. I'd be happy to contribute!

Thanks for your help on this!

@ghost
Copy link

ghost commented Feb 21, 2019

No problem! Documenting this would be nice indeed

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

2 participants