crystal
crystal copied to clipboard
crystal-lang
`crystal spec` command eats argument values instead of forwarding
The compiler command crystal spec inspects its arguments and forwards all arguments it does not understand to the spec executable which can then interpret them as runtime arguments.
A problem arises when the spec command interprets an option value (for example foobar in --tag foobar) as a path to a spec file or folder. This happens if the argument is a valid path.
https://github.com/crystal-lang/crystal/blob/e2903dbbc42e5668b9a1f950869b0871efe592c3/src/compiler/crystal/command/spec.cr#L40-L45
The following example demonstrates how that can lead to very unexpected and suprising behaviour:
$ mkdir foobar
$ crystal spec --tag foobar
Unhandled exception: Missing option: --tag (OptionParser::MissingOption)
from src/option_parser.cr:123:45 in '->'
from src/primitives.cr:266:3 in 'parse'
from src/spec.cr:114:3 in '__crystal_main'
from src/crystal/main.cr:110:5 in 'main_user_code'
from src/crystal/main.cr:96:7 in 'main'
from src/crystal/main.cr:119:3 in 'main'
from __libc_start_main
from _start
from ???
The spec command is unaware of the semantics of --tag. The argument foobar matches an existing file path, so it is interpreted as a source location and not forwarded to the spec runner. The executable is then invoked as spec_executable --tag which triggers an error.
This is really bad user experience. The command clearly passes a value to the --tag option, so the error complaining about a missing option value doesn't make sense from a user's perspective.
The root of the problem is that the compiler command does not understand the semantics of its arguments and just passes them on. But it can't know if --foo bar is a value-less argument --foo plus an independent argument bar or an option argument --foo with value bar.
To fix this properly, the spec command would need to know about the arguments of the spec runner. We already have a small dependency there since #10046 but this is only used for showing runtime options in the help message.
I'm not sure it would be a good idea to introduce semantical dependencies though, because that would greatly hinder the ability to use different spec runners than the one distributed with the respective compiler release. That includes introducing changes to the CLI interface, custom enhancements or alternative spec implementations.
An alternative solution would be to clearly separate runtime options from file arguments in the command line syntax. This could be done by disallowing file paths after any runtime option. Thus crystal spec --tag foobar spec would be an error. I don't think this would be very user friendly as well, but at least it would give a clear error message. Perhaps the practical impact would even be minimal.
A simple workaround is obviously to use the assignment syntax (--tag=foobar) which keeps the option name and value in a single argument. But at this time, that's not even shown in the command documentation.
Technically, I think this is a superior solution, actually. The assignment syntax also works for options with optional values, which is a similar problem (#5338). Allowing only assignment syntax for all CLI options would trivially avoid such issues.
This is certainly not a very urgent issue. The chance of collisions is typically pretty low. And it's easy to work around. But the error is surprising and hard to understand if you're not familiar with the implementation details.
/cc https://github.com/crystal-lang/crystal-book/pull/535
Another example for the same bug is with the --location option:
$ crystal spec --location spec/foo_spec.cr:4
location -l must be file:line
Through the mechanism described above, the compiler's spec command consumes the arguments incorrectly as it interprets the value of the --location option as a stand-alone argument.
So this results in filenames = ["spec/foo_spec.cr:4"] and options = ["--location"].
As a result of that, the compiler constructs arguments for the spec runner that look like this: ["--location", "--location", "spec/foo_spec.cr:4"]. [^1]
As a result, the spec runner tries to parse the second flag as value of the first flag, which results in the given error.
This error does not depend on chance (as with the --tag) example given above but it occurs every the time you use --location with crystal spec per documentation.
The compiler command should probably recognize --location options instead of just forwarding them to the spec runner. Otherwise crystal spec --location spec/foo_spec.cr:4 would not restrict the source to spec/foo_spec.cr and build the entire spec suite, only to filter at runtime for a specific spec in that file.
[^1]: It's not exactly this, due to some technicalities that are irrelevant for this issue. The following code makes it actually be ["--location", "-l", "4"] but it has the same meaning and effect. https://github.com/crystal-lang/crystal/blob/bd37b40bedb262ace8a2cb039707fa0dc2132a59/src/compiler/crystal/command/spec.cr#L73-L78
I think we should make the spec ignore any filename arguments after the first unknown flag.
So this code: https://github.com/crystal-lang/crystal/blob/bd37b40bedb262ace8a2cb039707fa0dc2132a59/src/compiler/crystal/command/spec.cr#L40-L45
should become something like this:
pos = options.index(&.starts_with?("-")) || options.size
filenames = options[...pos]
options.reject! { |option| filenames.includes?(option) }
Yeah, that's pretty similar. But here we don't actually know what is a valid flag for the spec runner so we must treat every argument that starts with a dash as a flag.
Maybe we can remove the spec command? Or deprecate it...
It only exists so you can pass a filename with a line number, like crystal spec file.cr:22.
Instead, we could standarize that. So if you pass a file to crystal and it has :n at the end, we treat that as a command line argument of some sort. Just some random thought...
I don't think that really changes much, because we'd still need to handle that case with a line number while recognizing compiler options and forwarding other options to the program. And that's exactly the spec command 🤷