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).

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

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.

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).

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.

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.)

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.

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?

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

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.

The arrays now are concatenated and the recursion is removed.

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.

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.

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).

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

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

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

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

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.

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?

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.

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.

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.

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

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.

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

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.

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

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

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

(Also, thanks everyone for the work!)