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
Access to Clflags from ppx preprocessors #6497
Comments
Comment author: @whitequark I forgot one thing in description; I do not think exposing any other Clflags fields makes any sense. Those two should satisfy any needs a preprocessor may have. |
Comment author: @alainfrisch Indeed, it could make sense to pass more contextual information from the caller (which can be ocamlc, ocamlopt, but also ocaml, ocamldoc, ocamldep, etc). Extra information could be passed in several ways:
Do you have an opinion? Concerning the actual set of information to pass, I'm wondering whether we should define exactly which options are passed, or whether we should pass the entire command-line (ppx would need to reparse it, taking OCAMLPARAMS into account, but we would automatically support all current and future flags). |
Comment author: @whitequark Naturally, I would first reach for changing the binary protocol. It's actually simple to keep the ability to pipe camlp4 to ppx, you just need to match on magic number. I think command-line is not a good idea for the reasons you have listed. I do not have a preference between changing binary protocol and @@@attributes, except for the fact that I really don't like the complexity of the serializer/deserializer associated with @@@attributes (but it could be hidden in the compiler). I think parsing command-line should be done in exactly one place for consistency. Additionally, OCaml tools frequently have differents sets of valid arguments and it is not clear how Ast_mapper should deal with that; it would have to duplicate the option parsers of different tools or give up on "parsing all options" goal. I don't see a point in passing all flags. I think the simplest solution is to change the protocol and magic number, and pass a record with the four flags I've mentioned, but there are certain advantages in passing that record via @@@attributes. |
Comment author: @alainfrisch If we don't pass the full command-line, we should at least send the tool name (either as a concrete sum type or as a string to be more extensible) to support the toplevel, ocamldoc, ocamldep in addition to ocamlc/ocamlopt. I think that @@@attributes are more coherent with how we pass warnings/errors back from ppx to the compiler. Moreover, they would not require to change the magic number if we ever want to pass more information (e.g. from tools such as ocamldoc, or maybe merlin). Do you want to give it a try? |
Comment author: @whitequark I think sending tool name as a string is OK. In @@@attributes there isn't a choice anyway. I think @@@attributes are worth a try. Would you write a patch, or should I? |
Comment author: @whitequark Actually, if tool name is sent, there is no need to send native_code, as it is just [tool_name = "ocamlopt"]. |
Comment author: @alainfrisch
I won't have time to work on it in the next days, so feel free to hack happily and provide a patch if we wish! |
Comment author: @alainfrisch Should we have a single attributes, e.g.: [@@@ocaml.ppx.context {tool_name="ocamlc"; load_path=["...";...;"..."]; for_pack=None; debug=false}] or rather multiple attributes: [@@@ocaml.ppx.tool_name "ocamlc"] I don't have a strong preference. |
Comment author: @whitequark I prefer single attributes. Easier to skip, less noise in debugging output, etc. Where do we export tool_name? Ast_mapper? |
Comment author: @alainfrisch
Yes, this makes it clear that the information is for ppx. I assume you have a internal reference in mind, with an accessor function, and the reference (together with other references in Clflags) would be set by Ast_mapper.apply by parsing the top of the AST to find the discussed attribute? |
Comment author: @whitequark Yes, it is simple and consistent with existing interfaces. |
Comment author: @whitequark What to do if Ast_mapper |
Comment author: @alainfrisch What about having the ppx report an error (or a warning?) in the resulting AST? |
Comment author: @whitequark Do you think replacing the @@@ocaml.ppx.context with @@@ocaml.error is appropriate? |
Comment author: @alainfrisch Probably rather with [%%ocaml.error] (i.e. the extension node recognized by the compiler). |
Comment author: @whitequark Right, I meant [%%ocaml.error]. |
Comment author: @whitequark I just realized another (related) problem, there is no entry point to read Location.input_name from. You can't do it as: let mapper argv = and you can't do it in mapper.structure, because that can be called several times while nested. Use case: I want to keep track of full module path for types in ppx_deriving. It's probably possible by adding more global state, but meh. Also, that'll break if ppx drivers will reuse whatever is returned by the mapper generator function applied to command-line options. Any chance Ast_mapper.mapper will gather a |
Comment author: @alainfrisch It would be possible to arrange so that Location.input_name is set before the function creating the mapper (from command-line arguments) is executed. Concerning the "toplevel_structure", the current API can be used to have a specific behavior at toplevel: let mapper argv = |
Comment author: @whitequark @doligez: I'm currently working on this. Expect a PR in a few days. |
Comment author: @whitequark @alain: how would we output the tool_name field? I'm currently thinking of setting the Ast_mapper.tool_name reference in the corresponding drivers. Don't see a cleaner solution. |
Comment author: @whitequark Test code:
How to build:
How to test:
|
Comment author: @alainfrisch Thanks! I've committed the patch to 4.02 (commit 15062), with some modifications (mostly cosmetics, except for tool_name which is now passed explicitly by the calling tool when it applies ppx processors instead of setting the global reference). |
Comment author: @alainfrisch Commit 15064 fixes the toplevel. It was broken because it received a typedtree containing the extra context attribute when it was expecting a simple Tstr_eval. The attribute is now removed in the same function which adds it (apply_rewriters) and it never flows to the type-checker. |
Comment author: @alainfrisch Also committed to trunk (15068). |
Comment author: @whitequark Excellent, thanks! 15064 would also fix utop (regular toplevel seemed to work for me, but perhaps I am mistaken.) |
…cessors (tool name, some command-line arguments). Corresponds to ocaml#85 Patch by Peter Zotov, with some modifications (mostly, the tool_name is now passed explicitly instead of through a global reference). Also rename Clflags.open_module to Clflags.open_modules and set Clflags.for_package even in bytecode. git-svn-id: http://caml.inria.fr/svn/ocaml/version/4.02@15062 f963ae5c-01c2-4b8c-9fe0-0dff7051ff02
git-svn-id: http://caml.inria.fr/svn/ocaml/version/4.02@15064 f963ae5c-01c2-4b8c-9fe0-0dff7051ff02
Original bug ID: 6497
Reporter: @whitequark
Assigned to: @alainfrisch
Status: closed (set by @xavierleroy on 2016-12-07T10:34:29Z)
Resolution: fixed
Priority: normal
Severity: minor
Version: 4.02.0+beta1 / +rc1
Target version: 4.02.0+dev
Category: ~DO NOT USE (was: OCaml general)
Monitored by: @jmeber
Bug description
I have ran several times into a need to access Clflags, which is obviously impossible, as the ppx preprocessor runs in a different process.
These include:
Getting the name of the current module, which is critical to 2 of 4 of the extensions I currently have. I'm currently using Location.input_name and deriving the module name from the file basename, but that would break with -for-pack.
Getting the list of -I flags. I have several extensions in mind that would do something with Typedtree of other modules, but it would currently be impractical to deploy them.
Getting the state of -g flag, to generate information only useful together with backtraces.
Getting the kind of compiler (ocamlc/ocamlnat), for conditional compilation.
So I think the Clflags.(for_pack, include_dirs, debug, native_code) should be exposed to ppx like Location.input_name currently is. I consider the lack of Clflags.for_pack a bug; it would be great to have it fixed in 4.02.
The text was updated successfully, but these errors were encountered: