ppxlib

ppxlib copied to clipboard

Hi, I faced an issue that isn't a huge deal but rather something that I would like to report.

Ppxlib.File_path.get_default_path doesn't follow the recommendation from Location: https://v2.ocaml.org/releases/4.05/htmlman/libref/Location.html

Note on the use of Lexing.position in this module. If pos_fname = "", then use !input_name instead.

One of the current workarounds is to use compiler-libs.common and use Location.input_name as a fallback.

Would it make sense to have it inside ppxlib?

Good catch, thanks!

I do think it makes sense to have it inside ppxlib.

Can you open a PR, or should I go ahead?

I'm happy to contribute to it, give me a little bit of time and should be simple to add.

Thanks!

One of the current workarounds is to use compiler-libs.common and use Location.input_name as a fallback.

This is definitely something we should avoid. Thanks for opening the issue, @davesnx!

I have a couple of questions: I'm curious why it's recommended to use !input_name when pos_fname = "". Or, more concretely, what are the situations in which pos_fname = "", but !input_name=<something interesting>. Can you give an example in which this has bitten you? Also, have you seen the difference in Expansion_context between input_name on one hand and the file_name component of code_path on the other hand? Would using input_name solve the problem in your concrete situation?

I ported a ppx from omp to ppxlib and this trick was implemented there. Haven't struggled with the issue myself. Repo: https://github.com/ml-in-barcelona/react-jsx-ppx

Also, have you seen the difference in Expansion_context between input_name on one hand and the file_name component of code_path on the other hand No I haven't.

I will try to "force" the issue to appear and report back here.

Even though I opened the PR, seems that it's something that isn't safe to add since there's a chance that this issue rarely (or never) happens. Commit comes from https://github.com/ocaml/ocaml/commit/fc701113aa68d5f787333b7f1d0f9e5d50dafc7f

Hey @davesnx, have you found out more about when the issue arises? I'm just asking because I've just gone through the open PRs and have seen your PR about this again.

Since the PPX in question is a whole-file transformation, you can use Ast_traverse.map_with_context (instead of Ast_traverse.map) in case you want to try using the Expansion_context as mentioned above.

It sounds good to me to make a change as proposed if that's needed/helpful.

I can't repro the case where this is true, I will close the issue and the PR.