Nim: double-quote bug in getopt() in parseopt

[greencardamom]

Details of the bug available in the forum:

https://forum.nim-lang.org/t/4080

[timotheecour]

could that be same issue as https://github.com/nim-lang/nimble/issues/514 ?

[andreaferretti]

Can you post an example here as well? It helps not having to go to external sites to see what's wrong

[timotheecour]

ok, re-posting verbatim :

in https://github.com/mratsim/Arraymancer for example here: https://github.com/mratsim/Arraymancer/blob/0605c7fcd34e216623a891f4963d2a4ef98882b1/arraymancer.nimble#L112

change:
--nimcache: "nimcache" to --nimcache: "nimcache withspace" in code below:

proc test(name: string, lang: string = "c") =
  if not dirExists "build":
    mkDir "build"
  if not dirExists "nimcache":
    mkDir "nimcache"
  --run
  --nimcache: "nimcache"
  switch("out", ("./build/" & name))
  setCommand lang, "tests/" & name & ".nim"

and run nimble test: it'll fail with: Error: cannot open 'nimcache2.nim'
which is due to wrong quoting applied to nimcache argument,

it's obviously a contrived example, but it's not contrived with another flag, eg:

  --name: "bob morton"

NOTE: I originally got suspicious after seeing nimble test fail with following error message:
Error: Execution failed with exit code 35584
... Command: "/Users/timothee/.nimble/bin/nim" c --noNimblePath --path:"/Users/timothee/.nimble/pkgs/nimblas-0.2.1" --path:"/Users/timothee/.nimble/pkgs/nimlapack-0.1.1" --path:"/Users/timothee/.nimble/pkgs/nimcuda-0.1.5" --path:"/Users/timothee/.nimble/pkgs/nimcl-0.1.3" --path:"/Users/timothee/.nimble/pkgs/opencl-1.0" --path:"/Users/timothee/.nimble/pkgs/clblast-0.0.1" --path:"/Users/timothee/.nimble/pkgs/opencl-1.0" --path:"/Users/timothee/.nimble/pkgs/stb_image-2.1" "--out:./build/tests_cpu" "--run" "--nimcache:"nimcache"" "tests/tests_cpu.nim"

which showed wrong quoting for "--nimcache:"nimcache"" (should've been "--nimcache:\"nimcache\"" or anything equivalent)

[pb-cdunn]

This is not a bug. parseopt parses one long string, so there will always be corner cases. The way to "fix" this would be to store and iterate over a list of strings, relying on the caller to have quoted each one properly, which is what the shell would pass to the program in Linux.

But if you try that (as I just did), you discover that you are re-writing parseopt2.

In other words, to parse an array of individual strings -- which may embed spaces and quotation marks -- use parseopt2.

In tests/misc/tparseopt.nim,

var argv: seq[string] = @["--left", "--debug:\" foo bar\"", "-l=4", "-r:2"]

produces

...
kind: cmdLongOption     key:val  --  debug:" foo bar"
...

[kaushalmodi]

In other words, to parse an array of individual strings -- which may embed spaces and quotation marks -- use parseopt2.

parseopt2 is deprecated on devel. From what I understand, parseopt is the one-and-only CLI options parsing stdlib on devel. With that only, I reproduced this issue here.

Double quoted arg values with spaces is pretty common (not a corner case).

[pb-cdunn]

Double quoted arg values with spaces is pretty common (not a corner case).

I agree.

parseopt2 is deprecated on devel.

That's a mistake, IMO. If parseopt2 is deprecated, then it should basically replace parseopt.

We should not write code in the option-parser which replicates the behavior of the shell. We should assume that the command-line has already been parsed into tokens, which we can then parse.

On systems where we do not have this functionality in the shell, we should simply not support spaces and quotes inside arguments because it's just too easy to get it wrong. In other words, overload initOptParser to split on whitespace when handed a string, and write parseopt to operate naturally on the sub-strings.

But maybe the thinking is that stdlib should only support functionality that works everywhere. In that case, the answer for you is to use something else for command-line parsing. There are a bunch of alternatives already. I like cligen.

[kaushalmodi]

On systems where we do not have this functionality in the shell, we should simply not support spaces and quotes inside arguments because it's just too easy to get it wrong. In other words, overload initOptParser to split on whitespace when handed a string, and write parseopt to operate naturally on the sub-strings.

But maybe the thinking is that stdlib should only support functionality that works everywhere.

Sorry, I don't know the internals of the parseopt/parseopt2 libraries.

In that case, the answer for you is to use something else for command-line parsing. There are a bunch of alternatives already. I like cligen.

Yes, cligen is awesome. That's what I use.

I was simply reproducing the issue the OP reported in that Nim Forum thread. I am surprised that such a common case is not supported in the stdlib.

[c-blake]

FYI - In the comments for https://github.com/nim-lang/Nim/pull/7297 Araq seemed open to using os.parseCmdLine in a way similar to what @pb-cdunn was just suggesting. That is a more invasive re-write of parseopt than my pull request was at the time which was just to get optional elision of sepchar. I would be happy to contribute my cligen/parseopt3, and have offered in the past. I'm not sure if it would be wanted..Maybe it's considered too featureful, but I did try to be very backward compatible where possible in both API and command syntax. Since that PR it has even grown one more feature based on @timotheecour wanting += syntax (in general <ANY-OP-CHAR>= where users can override opChars).

[dom96]

We've discussed this in https://github.com/nim-lang/Nim/issues/6818.

I'm going to close this as a duplicate.

[kaushalmodi]

@greencardamom This is now fixed on the devel branch. See my little test that shows that spaces and quotes are now handled well: https://ptpb.pw/sYl0/nim.