miri test and #[should_panic]

[memoryruins]

I've been running miri on various crates, including foundational ones on crates.io.
Currently, running cargo +nightly miri test on a test that is expected to panic, e.g.

#[should_panic]
#[test]
fn f() {
    panic!();
}

causes miri to stop the tests early and printing the following (in addition to a sizeable trace)

error[E0080]: constant evaluation error: the evaluated program panicked at 'explicit panic', src/main.rs:9:5
 --> src/main.rs:9:5
  |
9 |     panic!();
  |     ^^^^^^^^^ the evaluated program panicked at 'explicit panic', src/main.rs:9:5
  |
note: inside call to `f` at src/main.rs:8:1
 --> src/main.rs:8:1
  |
8 | / fn f() {
9 | |     panic!();
10| | }
  | |_^

It would be convenient if miri took account of the #[should_panic] attribute, allowing miri to continue on with other tests. It's trivial to add #[cfg(not(miri))], but the tests continuing to run might be expected by new users of miri.

If the current behaviour is expected or if the attribute is out of reach of miri, feel free to close ^^

miri 0.1.0 (3b83466 2019-02-15)
rustc 1.34.0-nightly (a9410cd1a 2019-02-15)
cargo 1.34.0-nightly (865cb7010 2019-02-10)

[oli-obk]

Yea this definitely makes sense to not run these tests until we figure out how to do unwinding. Unfortunately we are not running the tests manually. Instead, rustc generates a boatload of boilerplate that we then execute. It would be very hard and fragile to extract the relevant information to realize we can't possibly run this test and then report to the boilerplate that said test was ignored/failed/succeeded

[RalfJung]

Well, one thing we could try to do is change the libtest harness to not execute should_panic tests when cfg(miri) is set. But that wouldn't actually run the tests, of course, it would just "auto-ignore" them.

[oli-obk]

I think a less miri-specific variant would be to add a flag to ignore should_panic tests. Then we can just enable that by default. But this has to happen inside the test harness, too.

[memoryruins]

Good idea! It should be straightforward to add (at least, appears so after looking at libtest).
Leaving the following notes for myself to try when I'm home.

First, add a field ignore_should_panic: bool (or a better name, since that could be read differently), to TestOpts in libtest, which would default to false
edit: --exclude-should-panic is clearer than --ignore-should-panic
https://github.com/rust-lang/rust/blob/aadbc459bd97a0325897e2ff94999efbec6a499c/src/libtest/lib.rs#L364-L379

Then. check for the allow_unstable and ignore_should_panic, emitting an appropriate error if needed.
https://github.com/rust-lang/rust/blob/aadbc459bd97a0325897e2ff94999efbec6a499c/src/libtest/lib.rs#L561-L566

Set tests to ignore if they match ignore_should_panic in filter_tests.
https://github.com/rust-lang/rust/blob/aadbc459bd97a0325897e2ff94999efbec6a499c/src/libtest/lib.rs#L1368-L1382

Make a test for the configuration.
https://github.com/rust-lang/rust/blob/aadbc459bd97a0325897e2ff94999efbec6a499c/src/libtest/lib.rs#L1850

Add a description to the cli's ui.
https://github.com/rust-lang/rust/blob/aadbc459bd97a0325897e2ff94999efbec6a499c/src/libtest/lib.rs#L407

After that works and is merged into rustc's libtest, pass the flag by default in cargo miri's test command and mention the behavior in the readme.

miri/src/bin/cargo-miri.rs

Line 342 in e9be422

(MiriCommand::Test, "test") => {

[memoryruins]

cargo miri test -- -- --Z unstable-options --exclude-should-panic

has been working out like a charm! Two possible options from here:

1. Pass the new flag by default and describe the behavior in the readme; however, we would need to filter out duplicate flags. Without doing so, if the user passed in --Z unstable-options --include-ignored, it would complain since -Z unstable-options has been passed in once by us and once by the user.

2. A simpler option: describe the current behavior that originally led to this issue's creation in the common problems section under running miri on projects in the readme, and provide a solution of passing in the new opt we added to libtest.

[oli-obk]

As discussed on zulip, rustdoc and miri don't work together anyway, so having some nice should_panic support for doc comments can be done once we work on running miri on rustdoc code examples.

[RalfJung]

Pass the new flag by default and describe the behavior in the readme; however, we would need to filter out duplicate flags.

That seems like a reasonable approach to me. The argument computation code begins here.

[gnzlbg]

I just ran into this. Let me know if there is anything I can do on the libtest side to help here. We wanted to rework the libtest macros (like #[ignore]) in the new roadmap towards a stable libtest (rust-lang/libtest#2), maybe we should include better miri integration as part of the roadmap (like at least #[should_panic] should not make miri fail, e.g., by expanding to #[cfg(not(miri))], but ideally we would just generate different code when compiling under miri that would allow these tests to run, and properly fail.

[RalfJung]

@gnzlbg have you seen #652 ?

[gnzlbg]

Yes. The issue i'm raising is that miri should be able to run #[should_panic] tests and check that they actually do panic. Skipping #[should_panic] is a temporary workaround, not a fix. I can help with the workaround in the libtest side (e.g. to emit a warning on should_panic tests when running them under miri saying that they are skipped), but I'd prefer to work on actually being able to run #[should_panic] tests under miri.

[RalfJung]

Okay. That would be the newly created #658 then.