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

Expand option #778

Merged
merged 10 commits into from Oct 7, 2016

Conversation

Projects
None yet
4 participants
@bschommer
Contributor

bschommer commented Aug 25, 2016

This PR adds a new spec for the Arg parser: Expand of (string -> string array) which takes a function as arguments and parses the returned string array recursively with the same set of specs.

This implements the approach mentioned by @xavierleroy in #748.

bschommer added some commits Aug 12, 2016

Added expand spec type.
Expand expecets a function that takes a string and returns an
array of string which then are parsed inplace of the string
argument.
Added doc and tests.
Comment string for the argumnet is added as well as a test for
the new spec.
@alainfrisch

This comment has been minimized.

Show comment
Hide comment
@alainfrisch

alainfrisch Aug 25, 2016

Contributor

It's actually a bit more general than @xavierleroy suggestion in #748, since with this PR the user is responsible for expanding the argument (which could amount to reading a response file, or something else e.g. some environment variable).

Contributor

alainfrisch commented Aug 25, 2016

It's actually a bit more general than @xavierleroy suggestion in #748, since with this PR the user is responsible for expanding the argument (which could amount to reading a response file, or something else e.g. some environment variable).

@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Aug 25, 2016

Contributor

However it does not enforce the user to choose some predefined response file style.

Contributor

bschommer commented Aug 25, 2016

However it does not enforce the user to choose some predefined response file style.

@gasche gasche added the approved label Sep 2, 2016

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Sep 2, 2016

Member

I personally like this way to separate the two aspects of the discussed feature and thing this one could be merged as soon as we want to -- but of course I won't consider MPR#7050 solved before we have at least one easy-to-use parsing function.

Member

gasche commented Sep 2, 2016

I personally like this way to separate the two aspects of the discussed feature and thing this one could be merged as soon as we want to -- but of course I won't consider MPR#7050 solved before we have at least one easy-to-use parsing function.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Sep 2, 2016

Member

I think Expand should be a bit more carefully specified, as there are some slightly non-trivial semantics coming from the bracketing of buildup/teardown parts of the parse_argv_dynamic recursive call. For example, if -count <int> is an integer-taking action, it is not possible for the expanded arguments to end on -count with the expected count coming after that in the non-expanded argument list -- I'm not saying we have to support that. The implemented behaviour could arguably be understood from the documentation "[...] and recursively parse the returned array" (which is more a description of the implementation than a behavioral specification), but it's not clear from the name "Expand" (that to me suggests an expand-first-and-interpret after, as opposed to interpret-subarray-and-then-continue).

Member

gasche commented Sep 2, 2016

I think Expand should be a bit more carefully specified, as there are some slightly non-trivial semantics coming from the bracketing of buildup/teardown parts of the parse_argv_dynamic recursive call. For example, if -count <int> is an integer-taking action, it is not possible for the expanded arguments to end on -count with the expected count coming after that in the non-expanded argument list -- I'm not saying we have to support that. The implemented behaviour could arguably be understood from the documentation "[...] and recursively parse the returned array" (which is more a description of the implementation than a behavioral specification), but it's not clear from the name "Expand" (that to me suggests an expand-first-and-interpret after, as opposed to interpret-subarray-and-then-continue).

@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Sep 3, 2016

Contributor

You are right, there are some complications concerning the documentation and also the implementation.
Another problem is what happens to the count? The current implementation uses a new count for the recursive call since when the argument is parsed we don't parse arguments of the original array. What one could additionally implement is that either the function also returns an optional counter or add a an additional Expand_with_counter that contains this as argument.
Also is it allowed to modify the spec list in the recursively call and if further recursion is allowed/wanted.

Contributor

bschommer commented Sep 3, 2016

You are right, there are some complications concerning the documentation and also the implementation.
Another problem is what happens to the count? The current implementation uses a new count for the recursive call since when the argument is parsed we don't parse arguments of the original array. What one could additionally implement is that either the function also returns an optional counter or add a an additional Expand_with_counter that contains this as argument.
Also is it allowed to modify the spec list in the recursively call and if further recursion is allowed/wanted.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Sep 3, 2016

Member

Given the heavy skew of the current API towards heavily imperative left-to-right processing, I think that the following specification could be the simplest one:

if the remaining arguments to process are of the form ["-foo"; "arg"] @ rest where -foo is registered as Expand f, then the arguments f "arg" @ rest are processed.

This immediately suggests an implementation that actually concatenates the argument arrays, and makes no validity check on f "arg" as an argument subsequence. This is a very weak specification, but it feels "canonical" in a sense -- its a stable point in the design space. This dictates the way the current counter should evolve (namely, the same counter should be used after concatenation is performed).

Regarding recursion, it seems natural to me that nested expansions should be permitted and I think that it matters in practice: the idea should be that if a given command-line is too long, it is always possible to stuff it in a file and use -args <file> instead, and this guarantee would be broken by forbidding expansions inside expansions. Of course, that raises an issue of non-termination; given that expansion functions may have side-effects a simple cycle-detection strategy would not work, so the best option I think is to set a hard limit on the limit of successive expansions.

(I'll temporarily remove the "approved tag" during the design discussion to avoid confusion.)

Member

gasche commented Sep 3, 2016

Given the heavy skew of the current API towards heavily imperative left-to-right processing, I think that the following specification could be the simplest one:

if the remaining arguments to process are of the form ["-foo"; "arg"] @ rest where -foo is registered as Expand f, then the arguments f "arg" @ rest are processed.

This immediately suggests an implementation that actually concatenates the argument arrays, and makes no validity check on f "arg" as an argument subsequence. This is a very weak specification, but it feels "canonical" in a sense -- its a stable point in the design space. This dictates the way the current counter should evolve (namely, the same counter should be used after concatenation is performed).

Regarding recursion, it seems natural to me that nested expansions should be permitted and I think that it matters in practice: the idea should be that if a given command-line is too long, it is always possible to stuff it in a file and use -args <file> instead, and this guarantee would be broken by forbidding expansions inside expansions. Of course, that raises an issue of non-termination; given that expansion functions may have side-effects a simple cycle-detection strategy would not work, so the best option I think is to set a hard limit on the limit of successive expansions.

(I'll temporarily remove the "approved tag" during the design discussion to avoid confusion.)

@gasche gasche removed the approved label Sep 3, 2016

@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Sep 5, 2016

Contributor

This immediately suggests an implementation that actually concatenates the argument arrays, and makes no validity check on f "arg" as an argument subsequence. This is a very weak specification, but it feels "canonical" in a sense -- its a stable point in the design space. This dictates the way the current counter should evolve (namely, the same counter should be used after concatenation is performed).

Makes sense. I will update the PR accordingly.

Regarding recursion, it seems natural to me that nested expansions should be permitted and I think that it matters in practice: the idea should be that if a given command-line is too long, it is always possible to stuff it in a file and use -args instead, and this guarantee would be broken by forbidding expansions inside expansions. Of course, that raises an issue of non-termination; given that expansion functions may have side-effects a simple cycle-detection strategy would not work, so the best option I think is to set a hard limit on the limit of successive expansions.

I would let the user decide on the recursion for himself.

Contributor

bschommer commented Sep 5, 2016

This immediately suggests an implementation that actually concatenates the argument arrays, and makes no validity check on f "arg" as an argument subsequence. This is a very weak specification, but it feels "canonical" in a sense -- its a stable point in the design space. This dictates the way the current counter should evolve (namely, the same counter should be used after concatenation is performed).

Makes sense. I will update the PR accordingly.

Regarding recursion, it seems natural to me that nested expansions should be permitted and I think that it matters in practice: the idea should be that if a given command-line is too long, it is always possible to stuff it in a file and use -args instead, and this guarantee would be broken by forbidding expansions inside expansions. Of course, that raises an issue of non-termination; given that expansion functions may have side-effects a simple cycle-detection strategy would not work, so the best option I think is to set a hard limit on the limit of successive expansions.

I would let the user decide on the recursion for himself.

bschommer added some commits Sep 5, 2016

Update recursive call to use original counter.
Instead of introducing a new counter the recursive call takes the
original counter. Furthermore the documentation is updated to
reflect the actually semantics of Expand.
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Sep 5, 2016

Member

I would let the user decide on the recursion for himself.

Well there is a realistic risk that someone would put --args foo whithin foo, and just looping silently is not an excellent user interface I think. But in the interest of having this feature progress, we might defer that discussion until someone actually hits the issue and is brave enough to report it?

Member

gasche commented Sep 5, 2016

I would let the user decide on the recursion for himself.

Well there is a realistic risk that someone would put --args foo whithin foo, and just looping silently is not an excellent user interface I think. But in the interest of having this feature progress, we might defer that discussion until someone actually hits the issue and is brave enough to report it?

@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Sep 5, 2016

Contributor

How about an adding an additional bool to expand that states if recursion is allowed?

Contributor

bschommer commented Sep 5, 2016

How about an adding an additional bool to expand that states if recursion is allowed?

@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Sep 5, 2016

Contributor

I just recognized that when the current variable is used for the recursive call the whole function needs to be changed, since the current variable is used to get the arguments.

Contributor

bschommer commented Sep 5, 2016

I just recognized that when the current variable is used for the recursive call the whole function needs to be changed, since the current variable is used to get the arguments.

Actually concatenate arrays.
Reusing the current pointer only works if the arrays are concatenated.
@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Sep 5, 2016

Contributor

The arrays now are concatenated and the recursion is removed.

Contributor

bschommer commented Sep 5, 2016

The arrays now are concatenated and the recursion is removed.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Sep 5, 2016

Member

How about an adding an additional bool to expand that states if recursion is allowed?

No, I feel that this is not a meaningful choice to give to the users (because why would they not allow it?). But not having any termination-checking behavior is probably fine for a first PR.

Thanks for the new implementation that is about what I would expect. Note that it should be possible to implement it without actual array concatenation (which has a quadratic behavior) by keeping a stack (a list) of (currently processed array, offset of the first element to substract when indexing in it) pairs, but I think implementing that is probably premature optimization and a simpler, easier-to-understand concatenating implementation is fine.

I would replace the !l accesses with just Array.length !argv, which I suspect would make the code clearer and remove one way to get it wrong.

Member

gasche commented Sep 5, 2016

How about an adding an additional bool to expand that states if recursion is allowed?

No, I feel that this is not a meaningful choice to give to the users (because why would they not allow it?). But not having any termination-checking behavior is probably fine for a first PR.

Thanks for the new implementation that is about what I would expect. Note that it should be possible to implement it without actual array concatenation (which has a quadratic behavior) by keeping a stack (a list) of (currently processed array, offset of the first element to substract when indexing in it) pairs, but I think implementing that is probably premature optimization and a simpler, easier-to-understand concatenating implementation is fine.

I would replace the !l accesses with just Array.length !argv, which I suspect would make the code clearer and remove one way to get it wrong.

@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Sep 5, 2016

Contributor

My first implementation try did use a list, however the possibility to have arguments of the form -foo=bar made this far more complicated. I will replace l with the length.

Contributor

bschommer commented Sep 5, 2016

My first implementation try did use a list, however the possibility to have arguments of the form -foo=bar made this far more complicated. I will replace l with the length.

Replace !l with Array.length.
This avoids forgetting to update the lenght of after the Array is
updated.
@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Sep 5, 2016

Member

No, I was thinking of a list of (array, offset) pairs, where offset is the value of !counter at the time the array was pushed in by an Expand node. The code to get the next array element would not be !argv.(!current) anymore, but something like (untested)

let rec elem idx = function
| [] -> assert false
| (argv, offset) :: rest ->
  if idx < offset then elem idx rest
  else argv.(idx - offset)
in
elem !current !argv_stack

but, again, I think this is premature optimization (also I'm not completely sure how reliably that works if/when user code modifies the current reference).

Member

gasche commented Sep 5, 2016

No, I was thinking of a list of (array, offset) pairs, where offset is the value of !counter at the time the array was pushed in by an Expand node. The code to get the next array element would not be !argv.(!current) anymore, but something like (untested)

let rec elem idx = function
| [] -> assert false
| (argv, offset) :: rest ->
  if idx < offset then elem idx rest
  else argv.(idx - offset)
in
elem !current !argv_stack

but, again, I think this is premature optimization (also I'm not completely sure how reliably that works if/when user code modifies the current reference).

@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Sep 6, 2016

Contributor

Okay. However I think the concatenation is not that bad since ideally it should not happen to often.

Contributor

bschommer commented Sep 6, 2016

Okay. However I think the concatenation is not that bad since ideally it should not happen to often.

@diml

This comment has been minimized.

Show comment
Hide comment
@diml

diml Sep 9, 2016

Member

BTW, at some point we might want to allow the user to get access to the expanded array. There are programs such as the toplevel or ocamldebug that are using the current reference as an index in Sys.argv and for this reason they can't use Expand arguments. To use Expand arguments in such programs we would need to pass argv as a reference as well

Member

diml commented Sep 9, 2016

BTW, at some point we might want to allow the user to get access to the expanded array. There are programs such as the toplevel or ocamldebug that are using the current reference as an index in Sys.argv and for this reason they can't use Expand arguments. To use Expand arguments in such programs we would need to pass argv as a reference as well

@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Sep 12, 2016

Contributor

Adding such a function should not be that complicated and makes sense. Any suggestions for a name?

Contributor

bschommer commented Sep 12, 2016

Adding such a function should not be that complicated and makes sense. Any suggestions for a name?

Added parse_and_expand_argv_dynamic.
The new function takes a reference to the argv array and modifies
it if an Expand spec is encountered. Also added tests for this.
@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Sep 13, 2016

Contributor

I added a function parse_and_expand_argv_dynamic which takes the argv as reference arguments and modifies it.

Contributor

bschommer commented Sep 13, 2016

I added a function parse_and_expand_argv_dynamic which takes the argv as reference arguments and modifies it.

@diml

This comment has been minimized.

Show comment
Hide comment
@diml

diml Sep 15, 2016

Member

Thanks, I reviewed the changes and they look good to me. @gasche are you happy as well with the current patch?

One thing I was wondering is whether parse_and_exnapdn_argv_dynamic should be the only one allowed to treat expand arguments. With the other ones the user might end up with a current reference pointing to an array they don't have access to which can be a source of errors. But maybe that's too inconvenient.

Member

diml commented Sep 15, 2016

Thanks, I reviewed the changes and they look good to me. @gasche are you happy as well with the current patch?

One thing I was wondering is whether parse_and_exnapdn_argv_dynamic should be the only one allowed to treat expand arguments. With the other ones the user might end up with a current reference pointing to an array they don't have access to which can be a source of errors. But maybe that's too inconvenient.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Sep 15, 2016

Member

I would like most users to be able to take advantage of Expand easily, by just adding one such option in their specification list. This use-case suggests that while forbidding expand nodes with parse_argv{,_dynamic} functions is a possible choice, it should definitely be allowed with the more common parse{,_dynamic} functions.

One suggestion I considered is to have a global Arg.argv : string array ref reference that would be modified in line with Arg.current : int ref, such that upon return of one of the current-modifying function we have the invariant that !Arg.current is a valid index into !Arg.argv. However that is very ugly because to get the useful behavior with parse_argv one would need to either set !Arg.argv even on user-passed local current references (ugly) or do a physical equality test to set it only when current == Arg.current (also ugly).

I think I dislike the current, very stateful API of current : int ref parameters and global value in this module, and I'm rather unwilling to add more out-variables and global mutable states. Is there not a better API for the need solved by ?(current : int ref) today that we could use for both "current" and "argv" in the new expand-specific functions?

Member

gasche commented Sep 15, 2016

I would like most users to be able to take advantage of Expand easily, by just adding one such option in their specification list. This use-case suggests that while forbidding expand nodes with parse_argv{,_dynamic} functions is a possible choice, it should definitely be allowed with the more common parse{,_dynamic} functions.

One suggestion I considered is to have a global Arg.argv : string array ref reference that would be modified in line with Arg.current : int ref, such that upon return of one of the current-modifying function we have the invariant that !Arg.current is a valid index into !Arg.argv. However that is very ugly because to get the useful behavior with parse_argv one would need to either set !Arg.argv even on user-passed local current references (ugly) or do a physical equality test to set it only when current == Arg.current (also ugly).

I think I dislike the current, very stateful API of current : int ref parameters and global value in this module, and I'm rather unwilling to add more out-variables and global mutable states. Is there not a better API for the need solved by ?(current : int ref) today that we could use for both "current" and "argv" in the new expand-specific functions?

@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Sep 15, 2016

Contributor

One could use a slightly different approach to handle Expand options by adding a function which takes only care of expanding these options and nothing else. That way one could expand all options before an actual call to any of the parse_argv functions and we would not need worry about the problems from usages of current etc.

Contributor

bschommer commented Sep 15, 2016

One could use a slightly different approach to handle Expand options by adding a function which takes only care of expanding these options and nothing else. That way one could expand all options before an actual call to any of the parse_argv functions and we would not need worry about the problems from usages of current etc.

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Sep 15, 2016

Member

That doesn't really work if the expansion depends on state modified by the processing of previous options (for example one could think of -I dir/ options to add a directory to the set of included directories, and -args foo option that would look for a file named foo in the current directory or any included directory). Having this seems natural given the general semantics of Args processing (in particular with the _dynamic variants where the specification themselves can be modified by argument processing). That said, it does simplify the API quite a bit, as you suggest.

Member

gasche commented Sep 15, 2016

That doesn't really work if the expansion depends on state modified by the processing of previous options (for example one could think of -I dir/ options to add a directory to the set of included directories, and -args foo option that would look for a file named foo in the current directory or any included directory). Having this seems natural given the general semantics of Args processing (in particular with the _dynamic variants where the specification themselves can be modified by argument processing). That said, it does simplify the API quite a bit, as you suggest.

@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Sep 15, 2016

Contributor

Then one probably wants to forbid the usage of Expand in every function except for parse_and_expand_argv_dynamic. Otherwise someone the whole usage of current breaks if Expand is used.

Contributor

bschommer commented Sep 15, 2016

Then one probably wants to forbid the usage of Expand in every function except for parse_and_expand_argv_dynamic. Otherwise someone the whole usage of current breaks if Expand is used.

@diml

This comment has been minimized.

Show comment
Hide comment
@diml

diml Sep 15, 2016

Member

About a better API for ?(current : int ref), I can think of two different usages of current:

  1. to skip some arguments while parsing. For instance using this one could implement an option that takes two parameters (I think, I didn't check that it works)
  2. to parse the remaining of the command line in a different way once a certain argument is reached

(1) doesn't seem worth supporting. (2) is what is more commonly used, by the toplevel or ocamldebug for instance.

For (2), we can have an API without out-variables:

val parse_blah
  :  int * string array
  -> (key * spec * doc) list ref
  -> anon_fun
  -> usage_msg
  -> int * string array

one issue is how to tell Arg to stop parsing. In the toplevel, the run_script functions is called directly inside the argument handler, so we never leave Arg.parse. ocamldebug raise a specific exception to stop the parsing. Both are inspecting Arg.current.

To use the toplevel method with an API without out-variables, we would need to pass current and the updated argv to every handler in Arg.spec. This is a big API change.

We could use the ocamldebug method and have a special exception to stop parsing. It's not super nice but we already have a few special exceptions (Arg.Help and Arg.Bad) and this doesn't require to change the types.

That said, the whole API of Arg rely on mutating global states. I'm wondering if it's worth trying to change this particular case. At least adding Arg.argv is in line with the rest of the current API

Member

diml commented Sep 15, 2016

About a better API for ?(current : int ref), I can think of two different usages of current:

  1. to skip some arguments while parsing. For instance using this one could implement an option that takes two parameters (I think, I didn't check that it works)
  2. to parse the remaining of the command line in a different way once a certain argument is reached

(1) doesn't seem worth supporting. (2) is what is more commonly used, by the toplevel or ocamldebug for instance.

For (2), we can have an API without out-variables:

val parse_blah
  :  int * string array
  -> (key * spec * doc) list ref
  -> anon_fun
  -> usage_msg
  -> int * string array

one issue is how to tell Arg to stop parsing. In the toplevel, the run_script functions is called directly inside the argument handler, so we never leave Arg.parse. ocamldebug raise a specific exception to stop the parsing. Both are inspecting Arg.current.

To use the toplevel method with an API without out-variables, we would need to pass current and the updated argv to every handler in Arg.spec. This is a big API change.

We could use the ocamldebug method and have a special exception to stop parsing. It's not super nice but we already have a few special exceptions (Arg.Help and Arg.Bad) and this doesn't require to change the types.

That said, the whole API of Arg rely on mutating global states. I'm wondering if it's worth trying to change this particular case. At least adding Arg.argv is in line with the rest of the current API

@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Sep 16, 2016

Contributor

My suggestions:

  • For the Expand option/this PR forbid the usage of Expand in all functions except for the parse_and_expand_argv_dynamic which also modifies the array to avoid problems if someone used current in combination with Expand.

  • For a new API without out-variables I would add a function:

    val parse_next_arg: int * string array
    -> (key * spec * doc) list ref
    -> anon_fun 
    -> usage_msg 
    -> int * string array

    which parses only one argument starting at the given index returns the (potentially) modified array and the index of the next argument. This would avoid the usage of current and any other global state. Also this would make it easier to abort at a certain index etc.

Contributor

bschommer commented Sep 16, 2016

My suggestions:

  • For the Expand option/this PR forbid the usage of Expand in all functions except for the parse_and_expand_argv_dynamic which also modifies the array to avoid problems if someone used current in combination with Expand.

  • For a new API without out-variables I would add a function:

    val parse_next_arg: int * string array
    -> (key * spec * doc) list ref
    -> anon_fun 
    -> usage_msg 
    -> int * string array

    which parses only one argument starting at the given index returns the (potentially) modified array and the index of the next argument. This would avoid the usage of current and any other global state. Also this would make it easier to abort at a certain index etc.

@diml

This comment has been minimized.

Show comment
Hide comment
@diml

diml Sep 30, 2016

Member

Sorry for the delay, yes these suggestions look right to me. For parse_next_arg I suppose that we don't even need to use a reference for the spec, given that we'll be calling at most one callback

Member

diml commented Sep 30, 2016

Sorry for the delay, yes these suggestions look right to me. For parse_next_arg I suppose that we don't even need to use a reference for the spec, given that we'll be calling at most one callback

Disallow Expand in several functions.
Since Expand modifies the array and if the array is not passed as
reference the usage of Expand would break lead to problems when
current is used.
@bschommer

This comment has been minimized.

Show comment
Hide comment
@bschommer

bschommer Oct 5, 2016

Contributor

I just noticed a bug in the current implementation of Arg. The following program:

open Arg

let _ =
   let test1 = ref 0
  and test2 = ref false in
  let test_int i = test1 := i
  and test_bool b = test2 := b
  and f_anon _ = () in
  let spec = ["-test", Tuple [Int test_int;Bool test_bool], "Blub"] in
  let args = [|"prog";"-test=10";"true"|] in
  parse_argv args spec f_anon "blub";
  assert (!test1 = 10);
  assert (!test2)

Fails with:

Fatal error: exception Arg.Bad("prog: wrong argument '10'; option '-test=10' expects a boolean.
blub
  -test Blub
  -help  Display this list of options
  --help  Display this list of options
")

which is due to the fact that follow ref is never unset.

Contributor

bschommer commented Oct 5, 2016

I just noticed a bug in the current implementation of Arg. The following program:

open Arg

let _ =
   let test1 = ref 0
  and test2 = ref false in
  let test_int i = test1 := i
  and test_bool b = test2 := b
  and f_anon _ = () in
  let spec = ["-test", Tuple [Int test_int;Bool test_bool], "Blub"] in
  let args = [|"prog";"-test=10";"true"|] in
  parse_argv args spec f_anon "blub";
  assert (!test1 = 10);
  assert (!test2)

Fails with:

Fatal error: exception Arg.Bad("prog: wrong argument '10'; option '-test=10' expects a boolean.
blub
  -test Blub
  -help  Display this list of options
  --help  Display this list of options
")

which is due to the fact that follow ref is never unset.

Show outdated Hide outdated stdlib/arg.ml
@@ -122,12 +129,11 @@ let float_of_string_opt x =
try Some (float_of_string x)
with Failure _ -> None
let parse_argv_dynamic ?(current=current) argv speclist anonfun errmsg =
let l = Array.length argv in
let parse_and_expand_argv_dynamic_aux expand current argv speclist anonfun errmsg =

This comment has been minimized.

@diml

diml Oct 7, 2016

Member

I find expand not very descriptive, maybe expand_allowed?

@diml

diml Oct 7, 2016

Member

I find expand not very descriptive, maybe expand_allowed?

This comment has been minimized.

@bschommer

bschommer Oct 7, 2016

Contributor

It is now named expand_allowed.

@bschommer

bschommer Oct 7, 2016

Contributor

It is now named expand_allowed.

Show outdated Hide outdated stdlib/arg.ml
consume_arg ();
done;
| Expand f ->
if not expand then
raise (Stop (Message ("Expand is not allowed")));

This comment has been minimized.

@diml

diml Oct 7, 2016

Member

This is in fact a misuse of the API so it should raise Invalid_argument. I would put some informative message to help the programmer debug the problem: "Arg.Expand is only allowed with Arg.parse_and_expand_argv_dynamic"

@diml

diml Oct 7, 2016

Member

This is in fact a misuse of the API so it should raise Invalid_argument. I would put some informative message to help the programmer debug the problem: "Arg.Expand is only allowed with Arg.parse_and_expand_argv_dynamic"

This comment has been minimized.

@bschommer

bschommer Oct 7, 2016

Contributor

Is replaced.

@bschommer

bschommer Oct 7, 2016

Contributor

Is replaced.

Show outdated Hide outdated stdlib/arg.mli
@@ -125,6 +132,13 @@ val parse_argv_dynamic : ?current:int ref -> string array ->
See {!Arg.parse_dynamic}.
*)
val parse_and_expand_argv_dynamic : ?current:int ref -> string array ref ->

This comment has been minimized.

@diml

diml Oct 7, 2016

Member

Having ?current default to Arg.current doesn't seem right. We might as well make current a mandatory argument for this function

@diml

diml Oct 7, 2016

Member

Having ?current default to Arg.current doesn't seem right. We might as well make current a mandatory argument for this function

This comment has been minimized.

@bschommer

bschommer Oct 7, 2016

Contributor

Current is now mandatory.

@bschommer

bschommer Oct 7, 2016

Contributor

Current is now mandatory.

@diml diml self-assigned this Oct 7, 2016

Make current mandatory and raise Invalid_arg.
For the parse_and_expand_argv_dynamic function a current reference
is now mandatory. As well as Invalid_arg is raised if Expand is
used in any other function then parse_and_expand_argv_dynamic.
@diml

diml approved these changes Oct 7, 2016

@diml

This comment has been minimized.

Show comment
Hide comment
@diml

diml Oct 7, 2016

Member

The current state looks good to me. I'm planning to merge this PR unless someone disagrees (/cc @gasche )

Member

diml commented Oct 7, 2016

The current state looks good to me. I'm planning to merge this PR unless someone disagrees (/cc @gasche )

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Oct 7, 2016

Member

I haven't followed the last iterations but I trust @diml's opinion.

Member

gasche commented Oct 7, 2016

I haven't followed the last iterations but I trust @diml's opinion.

@diml diml merged commit 64ef11a into ocaml:trunk Oct 7, 2016

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@diml

This comment has been minimized.

Show comment
Hide comment
@diml

diml Oct 7, 2016

Member

Alright, it's now merged. @bschommer thanks for your contribution

Member

diml commented Oct 7, 2016

Alright, it's now merged. @bschommer thanks for your contribution

@gasche

This comment has been minimized.

Show comment
Hide comment
@gasche

gasche Oct 7, 2016

Member

(Also, thanks everyone for the work!)

Member

gasche commented Oct 7, 2016

(Also, thanks everyone for the work!)

@bschommer bschommer deleted the bschommer:expand-option branch Oct 10, 2016

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

add Arg.Expand (#778)
Add the `Arg.Expand` constructor to `Arg.spec`. This new specification allows the user to expand the `argv` array being parsed, for instance to implement responsefile support.

A new function `Arg.parse_and_expand_argv_dynamic` is added. It takes both `current` and `argv` as references and as mandatory arguments. This function allows the user to access the `argv` array after expansion.

To avoid confusion regarding the `?current` argument of the various parsing functions as well as with `Arg.current`, `Arg.Expand` is only allowed with the new function.

Tests for this PR are added to the testsuite.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment