Crystal: OptionParser should allow flag with optional value

[straight-shoota]

In #4499 it has come up, that OptionParser does not allow for a flag wich could either be assigned a value (--flag=value) or not be used without value (--flag). In the references PR it happend that --color a.cr was misinterpreted as flag --color with value a.cr, whereas it is meant to be a value-less flag --color and an unnamed argument a.cr.

Currently, the format --flag [value] allows a flag value to be missing, but this only works if it is the last argument or the next argument is also a flag. It would also match an unnamed argument. There should be a way to prevent that.

Now, obviously, this can't work with an empty space as separater between flag an value as this would always be ambigous, but it would work if it was enforced to be an equals sign: --foo arg would be an value-less flag, --flag=value would be a flag with value value.

[MakeNowJust]

this only works if it is the last argument or the next argument is also a flag.

It depends options handler order. For example:

require "option_parser"

def parse(args)
  p [:parse, args]
  OptionParser.parse(args) do |opts|
    opts.on("-a [val]", "") do |val|
      p [:a, val]
    end

    opts.on("-b [val]", "") do |val|
      p [:b, val]
    end

    opts.unknown_args do |args|
      p [:unknown_args, args]
    end
  end
  puts
end

parse(%w(-a -b b))
# [:parse, ["-a", "-b", "b"]
# [:a, "-b"]
# [:unknown_args, ["b"]]

parse(%w(-b -a a))
# [:parse, ["-b", "-a", "a"]
# [:a, "a"]
# [:unknown_args, []]
# NOTE: why "-b" is not called?

[straight-shoota]

Wow, this looks like there are some serious issues with optional argument values.

[urde-graven]

This is patched in commit 0c306cad23f906d87a82ca667c862d3afde9011d, which fixes 4 issues - see new specs in this commit against crystal main branch:

Failures:

  1) OptionParser optional option doesn't get separated value if specified with merged flag
     Failure/Error: flag.should eq(expect_value)

       Expected: ""
            got: "123"

     # spec/std/option_parser_spec.cr:15

  2) OptionParser optional option doesn't get separated value that looks like flag if specified with separated flag
     Failure/Error: flag.should eq(expect_value)

       Expected: ""
            got: "-g -h"

     # spec/std/option_parser_spec.cr:15

  3) OptionParser doesn't get value shifted in position because of removing another flag
     Failure/Error: flag.should eq(expect_value)

       Expected: ""
            got: "123"

     # spec/std/option_parser_spec.cr:15

  4) OptionParser should take short-flag's missing information about option from long flag
     Failure/Error: captured_z.should eq ""

       Expected: ""
            got: "14"

Three fixed problems are mentioned here - flag matched as optional value, separate argument matched when optional value is defined together with flag, and shifted argument matched as value because arguments were removed by previous matches. I also added improvement requested for example in #6632, because removing of consumed arguments is now additional work, but this is evidently done poorly to maintain backward compatibility - it should probably work like in Ruby's OptionParser, eg #parse should be non-destructive and #parse! should consume flags and their values:

OptionParser.parse!
(from ruby core)
------------------------------------------------------------------------
  parse!(argv = default_argv, into: nil)

------------------------------------------------------------------------
Same as #parse, but removes switches destructively. Non-option arguments remain in argv.

But I would agree with https://github.com/crystal-lang/crystal/pull/4809#issuecomment-375487285, that optionparser needs a rewrite and this could be part of such work

[RX14]

For now we should probably move OptionParser to be internal to the compiler if nobody wants to fix it. I certainly look at how much code it takes to implement it and i'm worried. It's implemented in an efficient way (callbacks) but I don't think that's probably the best way to implement it.

[urde-graven]

Something should be done, because these bugs are not minor. Else I would not try to push them upstream - I have not good experience with trying it, I almost always maintain my own fork. And I'm also working on more interesting things now - possibility of building shared libraries in crystal, generating .h files for them and so on. If you want, I can cut down these fixes to minimum, so it will be simpler to apply. Also I can remove new optional argument for non-destructive parsing. This patch should make parsing much faster for huge amount of arguments, but problem is overall algorithm, which scans all non-consumed arguments again and again for each handler instead of going sequentially through arguments and match them against all handlers.

[urde-graven]

What is your decision @RX14?

[RX14]

I'd like an opinion from the other core team members - I'm too busy with exams to rewrite OptionParser, as much as I would like to.

[urde-graven]

This is understandable, I also almost started to work on it. But I also know that that would be pointless work. Do you want to rewrite it, rather than apply this fast fix? Or you just don't like something about it? Are you afraid of new bugs?

[asterite]

Does this really need a rewrite? It seems to be just a bug with OptionParser...

[urde-graven]

I'd think that minimally mentioned algorithm needs rework, which I can do. I didn't started to rewrite it to minimize changes. I meant, that new interface of whole OptionParser would by pointless work from me, because you have probably another ideas. My idea is, that OptionParser in stdlib should be suited for most common simple programs, and also allow to serve as basic building block for more advanced "CLI Builders". So for example it should be also able to stop parsing on first unknown argument (I found this request two times here) and non-destructive parsing.

[RX14]

@asterite to me, just looking at OptionParser, it seems like a lot of complex code to do a "simple" thing. I might just be blinded by my preconceptions of how easy arguments parsing is.

[urde-graven]

Have you already decided? I hope I don't push you too much to stress you.

[urde-graven]

So are you usually trying to similarly discourage new contributors? Or should I take it as a special discriminatory honor? Or you just don't care?

[asterite]

@urde-graven If you have a patch that fixes some issue in OptionParser, please open a PR. If it behaves like Ruby's OptionParser then it will likely be accepted.

[urde-graven]

@asterite Only patch already mentioned in https://github.com/crystal-lang/crystal/issues/5338#issuecomment-452672895. I have not worked on it further nor opened PR due to the discussion here. If It's ok, I'll open it.

[asterite]

@urde-graven If it works like Ruby's OptionParser, then yes, please open a PR. Thank you!

[asterite]

@straight-shoota I'm sorry that it took me so long, but could you explain the issue with some example code and command line invocation?

[urde-graven]

@asterite

1. The first problem ("separate argument matched when optional value is defined together with flag") is explained also in https://github.com/crystal-lang/crystal/pull/4499#issuecomment-348510730. Example:

require "option_parser"

def parse(args)
  p [:parse, args]
  OptionParser.parse(args) do |opts|
    opts.on("-c[always|never|auto]", "--color=[always|never|auto]", "") do |val|
      p [:color, val]
    end
    opts.unknown_args do |args|
      p [:unknown_args, args]
    end
  end
  puts
end

parse(%w(--color a.cr))
# old OptionParser:
# [:parse, ["--color", "a.cr"]]
# [:color, "a.cr"]
# [:unknown_args, []]
# with patch:
# [:parse, ["--color", "a.cr"]]
# [:color, ""]
# [:unknown_args, ["a.cr"]]

parse(%w(-c a.cr))
# old OptionParser:
# [:parse, ["-c", "a.cr"]]
# [:color, "a.cr"]
# [:unknown_args, []]
# with patch:
# [:parse, ["-c", "a.cr"]]
# [:color, ""]
# [:unknown_args, ["a.cr"]]

1. Example for "flag matched as optional value" is in https://github.com/crystal-lang/crystal/issues/5338#issuecomment-348524535 (and the patch also fixes problem with missing call of -b, which I overlooked before). Output of the provided example with patched OptionParser:

[:parse, ["-a", "-b", "b"]]
[:a, ""]
[:b, "b"]
[:unknown_args, []]

[:parse, ["-b", "-a", "a"]]
[:a, "a"]
[:b, ""]
[:unknown_args, []]

1. Example for the third problem ("shifted argument matched as value because arguments were removed by previous matches") follows:

require "option_parser"

def parse(args)
  p [:parse, args]
  OptionParser.parse(args) do |opts|
    opts.on("-a [val]", "") do |val|
      p [:a, val]
    end
    opts.on("-b [val]", "") do |val|
      p [:b, val]
    end
    opts.unknown_args do |args|
      p [:unknown_args, args]
    end
  end
  puts
end

parse(%w(-b -a a c)
# old OptionParser:
# [:parse, ["-b", "-a", "a", "c"]]
# [:a, "a"]
# [:b, "c"]
# [:unknown_args, []]
# with patch:
# [:parse, ["-b", "-a", "a", "c"]]
# [:a, "a"]
# [:b, ""]
# [:unknown_args, ["c"]]

4) "OptionParser should take short-flag's missing information about option from long flag" - from spec:

args = ["-f", "12", "-g", "13", "-h", "14"]
captured_f = captured_g = captured_z = nil
parser = OptionParser.parse(args) do |opts|
  opts.on("-f", "--flag X", "some flag") { |flag_value| captured_f = flag_value }
  opts.on("-g", "--gflag=Y", "another flag") { |flag_value| captured_g = flag_value }
  opts.on("-h", "--hopt=[Z]", "optional flag") { |flag_value| captured_z = flag_value }
end
captured_f.should eq "12"
captured_g.should eq "13"
captured_z.should eq ""
args.should eq ["14"]
parser.to_s.should contain "   -f, --flag X"

[asterite]

@urde-graven Thank you!

I think all the existing problems in OptionParser come from the fact that I don't (didn't?) understand how option parsing work in general. I thought --flag value was valid syntax. But it seems these two are only valid:

• -xvalue: when -x is defined (single letter) then the value comes right after the first letter
• --some=value: when --some is defined, the value must be given with right after the name followed by = and the value

With that we can easily allow optional values:

• for -x[value]: nothing comes after x
• for --some=[value]: nothing comes after some

Is what I'm saying correct?

Is there any program out there where you can say --flag value?

[straight-shoota]

There is no standard concept for option parsing. There are different styles employed by different programs.

Is there any program out there where you can say --flag value?

Yes, of course. Loads of them. I'd say this is actually more common than --flag=value. And there is nothing wrong with that notation per se. It can only be ambiguous when the value is optional.

[asterite]

Cool, then. Feel free to continue forward with this, then (I won't have any opinion at all because I don't know about this subject).

[urde-graven]

https://www.gnu.org/software/libc/manual/html_node/Argument-Syntax.html