cargo miri: parse arguments (more) like corresponding cargo commands

[RalfJung]

Currently there is a discrepancy between the way cargo test and cargo miri test handle arguments (and same for run):

cargo test -- arguments
cargo miri test -- -- arguments

This regularly leads to confusion. So maybe we should fix that. There are two sub-parts to this question:

• How to best fix this? Maybe cargo miri test -Zmiri-disable-stacked-borrows -- arguments should work, though that looks odd. Or maybe we follow the RUSTFLAGS env var and add MIRIFLAGS or so.
• How to deal with the backwards incompatibility?

For the latter, would be nice to get the thoughts of some Miri users how they'd feel if we broke their scripts that already use double-double-dash now in order to fix this discrepancy.

[elichai]

Yeah it always takes me a sec to remember I need to pass another -- to cargo-miri when passing arguments so if there's an easy way to unify that then I think it's worth the breaking change.
I don't have much projects running miri in the CI, mostly due to some C bindings (where I locally override it with a shim and then run miri)
and in the ones I do it's ususally something like travis_wait 120 cargo miri test --features="feature1 feature2 feature3"

[RalfJung]

Hm, there was not a lot of feedback for this. I am still wondering it is is worth the breakage to make people write MIRIFLAGS=-Zmiri-disable-validation cargo miri test -- test-args. @rust-lang/miri any opinions?

[oli-obk]

I think we should just leave it as it is. The gains are minimal, and it's not clear if doing the change now will lead to less confusion. If we can improve the error then that could be done, but I think having examples and documentation for it should suffice.

[RalfJung]

If we ever manage to resolve #739, that would probably be easier if our command-line arguments were more like cargo's.

[alecmocatta]

cargo test accepts a few more arguments than cargo check, which currently results in an error if you try to use these arguments with cargo miri:

$ cargo miri test test_name
error: Found argument 'test_name' which wasn't expected, or isn't valid in this context

USAGE:
    cargo check --all --features <FEATURES>... --lib --profile <PROFILE-NAME>

Args I'm aware of are --doc, --no-fast-fail, --no-run, and most importantly TESTNAME. Perhaps a full clap parsing of miri arguments (like the cargo ops do), extracting miri-specific arguments and forwarding the remainder, may ultimately be the best option to handle these?

[RalfJung]

cargo test test_name is really short for cargo test -- test_name. This is not a cargo argument, it is passed to the test runner. --no-fast-fail is AFAIK also a test runner argument. --no-run OTOH is a cargo argument. EDIT: ah no, --no-fast-fail is also a cargo argument. But the test name definitely is not.

Perhaps a full clap parsing of miri arguments (like the cargo ops do), extracting miri-specific arguments and forwarding th

I'd rather avoid duplicating the cargo clap parsing in Miri, that's horrible to maintain. Instead I hope we can just forward all our arguments to cargo, add --target in the right spot, set some env vars so cargo calls back to us for running rustc and running the binaries, and then let cargo do all the heavy lifting.

[alecmocatta]

TESTNAME is explicitly parsed and forwarded, along with the others I mentioned, by cargo test but not by cargo check.

Given cargo miri test is forwarding arguments to cargo check, some parsing of args (like TESTNAME) will be necessary for compatibility with cargo test (if that is desired).

An alternative might be adding a --no-build flag to cargo test and using that instead of cargo check.

[RalfJung]

TESTNAME is explicitly parsed and forwarded, along with the others I mentioned, by cargo test

Oh I see, thanks for pointing that out. I thought this was some kind of magic that detected that I had omitted the --.

The goal is to use cargo test for cargo miri test. I think that is the only way we can avoid having to understand cargo testing -- as your series of bugreports show, there's a lot of stuff going on there that we do not implement currently.