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

Allow plugins to declare new arguments #796

Merged
merged 1 commit into from Jan 17, 2017

Conversation

Projects
None yet
8 participants
@lefessan
Contributor

lefessan commented Sep 7, 2016

Now that ocamlc has plugins, such plugins may want to declare their own arguments.

@dsheets

This comment has been minimized.

Show comment
Hide comment
@dsheets

dsheets Sep 7, 2016

Member

Is there a plan to prevent plugin and compiler arguments from interfering? Is there a plan to prevent plugin arguments from two unrelated plugins from interfering with each other?

Member

dsheets commented Sep 7, 2016

Is there a plan to prevent plugin and compiler arguments from interfering? Is there a plan to prevent plugin arguments from two unrelated plugins from interfering with each other?

@lefessan

This comment has been minimized.

Show comment
Hide comment
@lefessan

lefessan Sep 7, 2016

Contributor

I imagine it's more the problem of plugins, not of the compiler. If a plugin wants, it can remove existing arguments, or check if an argument already exists before adding it.

Contributor

lefessan commented Sep 7, 2016

I imagine it's more the problem of plugins, not of the compiler. If a plugin wants, it can remove existing arguments, or check if an argument already exists before adding it.

@dsheets

This comment has been minimized.

Show comment
Hide comment
@dsheets

dsheets Sep 7, 2016

Member

Respectfully, that plan does not appear to anticipate the success of this feature.

Member

dsheets commented Sep 7, 2016

Respectfully, that plan does not appear to anticipate the success of this feature.

@diml

This comment has been minimized.

Show comment
Hide comment
@diml

diml Sep 7, 2016

Member

In ppx_core we have some code to detect clashes of globally named elements and print a helpful error messages pointing to the place where the two things where registered. For instance:

Warning: code transformation custom_printf registered twice.
  - first time was at ppx_custom_printf.ml:337
  - second time is at ppx_fail.ml:19

While these kind of errors are indeed the fault of the plugin, having this kind of safety check helps debugging a lot. The code to get the location of the caller is here and is used this way:

let caller_id = Caller_id.get ~skip:[__FILE__] in
...

Maybe we could do something similar for this feature? We would need to export an add_arg function instead of the reference directly.

Member

diml commented Sep 7, 2016

In ppx_core we have some code to detect clashes of globally named elements and print a helpful error messages pointing to the place where the two things where registered. For instance:

Warning: code transformation custom_printf registered twice.
  - first time was at ppx_custom_printf.ml:337
  - second time is at ppx_fail.ml:19

While these kind of errors are indeed the fault of the plugin, having this kind of safety check helps debugging a lot. The code to get the location of the caller is here and is used this way:

let caller_id = Caller_id.get ~skip:[__FILE__] in
...

Maybe we could do something similar for this feature? We would need to export an add_arg function instead of the reference directly.

@lefessan

This comment has been minimized.

Show comment
Hide comment
@lefessan

lefessan Sep 7, 2016

Contributor

I am not saying we should not provide such helpers, just that this PR only provides the basic mechanism that is needed by the plugins, while another PR later can provide the helpers.

Contributor

lefessan commented Sep 7, 2016

I am not saying we should not provide such helpers, just that this PR only provides the basic mechanism that is needed by the plugins, while another PR later can provide the helpers.

@avsm

This comment has been minimized.

Show comment
Hide comment
@avsm

avsm Sep 7, 2016

Member

I think @diml's point is that the raw ref should not be exposed directly to plugins, and that they should go through the helper functions which guarantee that any extra plugin CLI arguments will be checked for clashes.

Member

avsm commented Sep 7, 2016

I think @diml's point is that the raw ref should not be exposed directly to plugins, and that they should go through the helper functions which guarantee that any extra plugin CLI arguments will be checked for clashes.

@lefessan

This comment has been minimized.

Show comment
Hide comment
@lefessan

lefessan Sep 7, 2016

Contributor

The ref cannot not be exposed directly, because it is used (as a ref) by Arg.parse_dynamic. So, it is possible to provide an helper function, but not to prevent plugins from accessing directly the arguments (unless we wrap Arg.parse_dynamic too). I think it is a lot of code for a very small problem, that did not even appear yet.

Contributor

lefessan commented Sep 7, 2016

The ref cannot not be exposed directly, because it is used (as a ref) by Arg.parse_dynamic. So, it is possible to provide an helper function, but not to prevent plugins from accessing directly the arguments (unless we wrap Arg.parse_dynamic too). I think it is a lot of code for a very small problem, that did not even appear yet.

@diml

This comment has been minimized.

Show comment
Hide comment
@diml

diml Sep 7, 2016

Member

Wrapping Arg.parse_dynamic is a one-liner. Even if we don't implement the check now, it will make the API future-proof

Member

diml commented Sep 7, 2016

Wrapping Arg.parse_dynamic is a one-liner. Even if we don't implement the check now, it will make the API future-proof

@lefessan

This comment has been minimized.

Show comment
Hide comment
@lefessan

lefessan Sep 7, 2016

Contributor

Future proof ? How can we forcast what plugin writers will want to do, and won't be able to do just because it was decided to abstract this value. Why not do the same for all references in Clflags, and write setters and getters for all of them, checking that they are being consistently modified with whatever invariants we decide for them ? I think we are adding limitations without any reason.

Moreover, what should happen if two plugins declare the same option ? Shall we raise an error, preventing the use of the two plugins together even if the option is not actually used ? Shall we instead declare an option that will raise the error only if it is used ? Shall we have different cases if the existing options was declared by the compiler or by another plugin ? Shall we see option handlers as hooks, that could be combined by different plugins (i.e. an option could be declared by several plugins (and the compiler), that would just add some code to be executed when the option is called) ? Sounds like a lot of cases to handle to make the API future-proof.

Contributor

lefessan commented Sep 7, 2016

Future proof ? How can we forcast what plugin writers will want to do, and won't be able to do just because it was decided to abstract this value. Why not do the same for all references in Clflags, and write setters and getters for all of them, checking that they are being consistently modified with whatever invariants we decide for them ? I think we are adding limitations without any reason.

Moreover, what should happen if two plugins declare the same option ? Shall we raise an error, preventing the use of the two plugins together even if the option is not actually used ? Shall we instead declare an option that will raise the error only if it is used ? Shall we have different cases if the existing options was declared by the compiler or by another plugin ? Shall we see option handlers as hooks, that could be combined by different plugins (i.e. an option could be declared by several plugins (and the compiler), that would just add some code to be executed when the option is called) ? Sounds like a lot of cases to handle to make the API future-proof.

@dsheets

This comment has been minimized.

Show comment
Hide comment
@dsheets

dsheets Sep 7, 2016

Member

How about namespacing plugin arguments under something like --x-[pluginshortname]-[pluginarg]?

Member

dsheets commented Sep 7, 2016

How about namespacing plugin arguments under something like --x-[pluginshortname]-[pluginarg]?

@diml

This comment has been minimized.

Show comment
Hide comment
@diml

diml Sep 7, 2016

Member

I like the namespacing idea. Another advantage of adopting a uniform naming convention is that when someone report an error and paste a compilation command, we'll be able to tell immediately which arguments are for plugins and which ones are for the compiler itself

Member

diml commented Sep 7, 2016

I like the namespacing idea. Another advantage of adopting a uniform naming convention is that when someone report an error and paste a compilation command, we'll be able to tell immediately which arguments are for plugins and which ones are for the compiler itself

@lefessan lefessan added this to the 4.04 milestone Sep 14, 2016

@lefessan

This comment has been minimized.

Show comment
Hide comment
@lefessan

lefessan Sep 14, 2016

Contributor

I implemented a check for already defined arguments, using the function Clflags.add_arguments. As a consequence, it discovered that -keep-docs was duplicated in the compiler. I added the PR to Milestone 4.04, as the bug fix might make this (very short) PR relevant for the upcoming release.

Contributor

lefessan commented Sep 14, 2016

I implemented a check for already defined arguments, using the function Clflags.add_arguments. As a consequence, it discovered that -keep-docs was duplicated in the compiler. I added the PR to Milestone 4.04, as the bug fix might make this (very short) PR relevant for the upcoming release.

try
let loc2 = Misc.StringMap.find arg_name !arg_names in
Printf.eprintf
"Warning: plugin argument %s is already defined:\n" arg_name;

This comment has been minimized.

@damiendoligez

damiendoligez Sep 29, 2016

Member

The warning is not in emacs format, so it will get lost in the noise of the build. I'm not sure what can be done about this.

@damiendoligez

damiendoligez Sep 29, 2016

Member

The warning is not in emacs format, so it will get lost in the noise of the build. I'm not sure what can be done about this.

This comment has been minimized.

@lefessan

lefessan Sep 29, 2016

Contributor

Maybe I could print the warning as located at the first line of the plugin ?

@lefessan

lefessan Sep 29, 2016

Contributor

Maybe I could print the warning as located at the first line of the plugin ?

This comment has been minimized.

@damiendoligez

damiendoligez Oct 4, 2016

Member

Do you still have the file name of the plugin's source at this point??

@damiendoligez

damiendoligez Oct 4, 2016

Member

Do you still have the file name of the plugin's source at this point??

This comment has been minimized.

@alainfrisch

alainfrisch Oct 4, 2016

Contributor

I guess this can be extracted from the callstack at registration time. That said, it might be more user friendly to have each plugin provide a symbolic name and use that name in the error message.

@alainfrisch

alainfrisch Oct 4, 2016

Contributor

I guess this can be extracted from the callstack at registration time. That said, it might be more user friendly to have each plugin provide a symbolic name and use that name in the error message.

This comment has been minimized.

@damiendoligez

damiendoligez Oct 5, 2016

Member

I guess this can be extracted from the callstack at registration time.

Please refrain from using java-style reflection tricks, thank you very much. I guess this problem can wait.

@damiendoligez

damiendoligez Oct 5, 2016

Member

I guess this can be extracted from the callstack at registration time.

Please refrain from using java-style reflection tricks, thank you very much. I guess this problem can wait.

This comment has been minimized.

@damiendoligez

damiendoligez Oct 5, 2016

Member

In fact you have loc and loc2 available here. Why not make it a proper warning and use Location.print_warning twice?

@damiendoligez

damiendoligez Oct 5, 2016

Member

In fact you have loc and loc2 available here. Why not make it a proper warning and use Location.print_warning twice?

@Kakadu

This comment has been minimized.

Show comment
Hide comment
@Kakadu

Kakadu Sep 30, 2016

@lefessan Do you have examples of helloworld plugins somewhere?

Kakadu commented Sep 30, 2016

@lefessan Do you have examples of helloworld plugins somewhere?

@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Oct 18, 2016

Member

@lefessan what do you think of namespacing? I like the idea (although I'd drop the x- part).

Member

damiendoligez commented Oct 18, 2016

@lefessan what do you think of namespacing? I like the idea (although I'd drop the x- part).

@bobzhang

This comment has been minimized.

Show comment
Hide comment
@bobzhang

bobzhang Oct 18, 2016

Member

FYI, we do extend the ocaml compiler with more flags in bucklescript, all flags are prefixed with -bs to prevent clash with the future release of ocaml compiler.
Another issue I noticed is that not too many ppx used qualified attributes which will cause more problems in the future

Member

bobzhang commented Oct 18, 2016

FYI, we do extend the ocaml compiler with more flags in bucklescript, all flags are prefixed with -bs to prevent clash with the future release of ocaml compiler.
Another issue I noticed is that not too many ppx used qualified attributes which will cause more problems in the future

@Kakadu

This comment has been minimized.

Show comment
Hide comment
@Kakadu

Kakadu Oct 18, 2016

Folks, how far away are we from full support of compiler plugins which will, for example, allow to implement additional rewriting of typed tree?

Kakadu commented Oct 18, 2016

Folks, how far away are we from full support of compiler plugins which will, for example, allow to implement additional rewriting of typed tree?

@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Oct 19, 2016

Member

We still haven't completely agreed on the design, and I just learned that @lefessan will be unavailable for some time. Since this feature is not important enough to delay the release, I'm removing the 4.04 milestone.

Member

damiendoligez commented Oct 19, 2016

We still haven't completely agreed on the design, and I just learned that @lefessan will be unavailable for some time. Since this feature is not important enough to delay the release, I'm removing the 4.04 milestone.

@damiendoligez damiendoligez modified the milestones: 4.05-or-later, 4.04 Oct 19, 2016

@Kakadu

This comment has been minimized.

Show comment
Hide comment
@Kakadu

Kakadu Oct 19, 2016

@damiendoligez Can you briefly describe current status? I kind of want this feature, so I need to decide will I put some effort and time into it in the nearest future.

Kakadu commented Oct 19, 2016

@damiendoligez Can you briefly describe current status? I kind of want this feature, so I need to decide will I put some effort and time into it in the nearest future.

@damiendoligez

This comment has been minimized.

Show comment
Hide comment
@damiendoligez

damiendoligez Oct 19, 2016

Member

I certainly expect the feature to be included in 4.05, and probably even in 4.04.1 if there is a 4.04.1.
AFAICT we just need to agree on the namespacing thing.

Concerning full support for compiler plugins, I think we're very close but you'll have to wait for Fabrice to come back to get a full answer.

Member

damiendoligez commented Oct 19, 2016

I certainly expect the feature to be included in 4.05, and probably even in 4.04.1 if there is a 4.04.1.
AFAICT we just need to agree on the namespacing thing.

Concerning full support for compiler plugins, I think we're very close but you'll have to wait for Fabrice to come back to get a full answer.

@lefessan

This comment has been minimized.

Show comment
Hide comment
@lefessan

lefessan Dec 22, 2016

Contributor

Here is an updated version. I am not fond of namespacing on arguments, as it may prevent us from adding simple arguments. I would prefer plugin developers to do it by themselves if they want their plugin to follow such a discipline. Anyway, I think we can merge this PR, and wait for a proposal for namespaces, and decide at that point what to do next.

Contributor

lefessan commented Dec 22, 2016

Here is an updated version. I am not fond of namespacing on arguments, as it may prevent us from adding simple arguments. I would prefer plugin developers to do it by themselves if they want their plugin to follow such a discipline. Anyway, I think we can merge this PR, and wait for a proposal for namespaces, and decide at that point what to do next.

@lefessan lefessan merged commit 522471d into ocaml:trunk Jan 17, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lefessan lefessan deleted the lefessan:2016-09-07-parse-dynamic branch Jan 17, 2017

@lefessan

This comment has been minimized.

Show comment
Hide comment
@lefessan

lefessan Jan 17, 2017

Contributor

Since there was no discussion anymore since the last version, I think we reached a satisfying state.
Adding namespaces would be trivial, if really necessary (add an optional namespace argument to Clflags.add_arguments). Thus merged.

Contributor

lefessan commented Jan 17, 2017

Since there was no discussion anymore since the last version, I think we reached a satisfying state.
Adding namespaces would be trivial, if really necessary (add an optional namespace argument to Clflags.add_arguments). Thus merged.

camlspotter pushed a commit to camlspotter/ocaml that referenced this pull request Oct 17, 2017

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