Auto merge of #37749 - keeperofdakeys:should-panic, r=alexcrichton

Improvements to the #[should_panic] feature

Add more error checking for the `#[should_panic]` attribute, and print the expected panic string when it does not match.

Fixes https://github.com/rust-lang/rust/issues/29000

Eg:
```running 3 tests
test test2 ... ok
test test1 ... FAILED
: Panic did not include expected string 'foo'
test test3 ... FAILED

failures:

---- test1 stdout ----
	thread 'test1' panicked at 'bar', test.rs:7
note: Run with `RUST_BACKTRACE=1` for a backtrace.

---- test3 stdout ----
	thread 'test3' panicked at 'bar', test.rs:18

```

5 files changed, 151 insertions, 20 deletions

diff --git a/src/libsyntax/test.rs b/src/libsyntax/test.rs
index 618878c1f79..59a7e75d125 100644
--- a/src/libsyntax/test.rs
+++ b/src/libsyntax/test.rs
@@ -132,7 +132,7 @@ impl<'a> fold::Folder for TestHarnessGenerator<'a> {
                         path: self.cx.path.clone(),
                         bench: is_bench_fn(&self.cx, &i),
                         ignore: is_ignored(&i),
-                        should_panic: should_panic(&i)
+                        should_panic: should_panic(&i, &self.cx)
                     };
                     self.cx.testfns.push(test);
                     self.tests.push(i.ident);
@@ -395,14 +395,44 @@ fn is_ignored(i: &ast::Item) -> bool {
     i.attrs.iter().any(|attr| attr.check_name("ignore"))
 }
 
-fn should_panic(i: &ast::Item) -> ShouldPanic {
+fn should_panic(i: &ast::Item, cx: &TestCtxt) -> ShouldPanic {
     match i.attrs.iter().find(|attr| attr.check_name("should_panic")) {
         Some(attr) => {
-            let msg = attr.meta_item_list()
-                .and_then(|list| list.iter().find(|mi| mi.check_name("expected")))
-                .and_then(|li| li.meta_item())
-                .and_then(|mi| mi.value_str());
-            ShouldPanic::Yes(msg)
+            let sd = cx.span_diagnostic;
+            if attr.is_value_str() {
+                sd.struct_span_warn(
+                    attr.span(),
+                    "attribute must be of the form: \
+                     `#[should_panic]` or \
+                     `#[should_panic(expected = \"error message\")]`"
+                ).note("Errors in this attribute were erroneously allowed \
+                        and will become a hard error in a future release.")
+                .emit();
+                return ShouldPanic::Yes(None);
+            }
+            match attr.meta_item_list() {
+                // Handle #[should_panic]
+                None => ShouldPanic::Yes(None),
+                // Handle #[should_panic(expected = "foo")]
+                Some(list) => {
+                    let msg = list.iter()
+                        .find(|mi| mi.check_name("expected"))
+                        .and_then(|mi| mi.meta_item())
+                        .and_then(|mi| mi.value_str());
+                    if list.len() != 1 || msg.is_none() {
+                        sd.struct_span_warn(
+                            attr.span(),
+                            "argument must be of the form: \
+                             `expected = \"error message\"`"
+                        ).note("Errors in this attribute were erroneously \
+                                allowed and will become a hard error in a \
+                                future release.").emit();
+                        ShouldPanic::Yes(None)
+                    } else {
+                        ShouldPanic::Yes(msg)
+                    }
+                },
+            }
         }
         None => ShouldPanic::No,
     }
diff --git a/src/libtest/lib.rs b/src/libtest/lib.rs
index 95ae6eb2efe..8749a64e5fd 100644
--- a/src/libtest/lib.rs
+++ b/src/libtest/lib.rs
@@ -75,9 +75,9 @@ const TEST_WARN_TIMEOUT_S: u64 = 60;
 // to be used by rustc to compile tests in libtest
 pub mod test {
     pub use {Bencher, TestName, TestResult, TestDesc, TestDescAndFn, TestOpts, TrFailed,
-             TrIgnored, TrOk, Metric, MetricMap, StaticTestFn, StaticTestName, DynTestName,
-             DynTestFn, run_test, test_main, test_main_static, filter_tests, parse_opts,
-             StaticBenchFn, ShouldPanic};
+             TrFailedMsg, TrIgnored, TrOk, Metric, MetricMap, StaticTestFn, StaticTestName,
+             DynTestName, DynTestFn, run_test, test_main, test_main_static, filter_tests,
+             parse_opts, StaticBenchFn, ShouldPanic};
 }
 
 pub mod stats;
@@ -473,6 +473,7 @@ pub struct BenchSamples {
 pub enum TestResult {
     TrOk,
     TrFailed,
+    TrFailedMsg(String),
     TrIgnored,
     TrMetrics(MetricMap),
     TrBench(BenchSamples),
@@ -611,7 +612,7 @@ impl<T: Write> ConsoleTestState<T> {
     pub fn write_result(&mut self, result: &TestResult) -> io::Result<()> {
         match *result {
             TrOk => self.write_ok(),
-            TrFailed => self.write_failed(),
+            TrFailed | TrFailedMsg(_) => self.write_failed(),
             TrIgnored => self.write_ignored(),
             TrMetrics(ref mm) => {
                 self.write_metric()?;
@@ -638,6 +639,7 @@ impl<T: Write> ConsoleTestState<T> {
                                 match *result {
                                     TrOk => "ok".to_owned(),
                                     TrFailed => "failed".to_owned(),
+                                    TrFailedMsg(ref msg) => format!("failed: {}", msg),
                                     TrIgnored => "ignored".to_owned(),
                                     TrMetrics(ref mm) => mm.fmt_metrics(),
                                     TrBench(ref bs) => fmt_bench_samples(bs),
@@ -773,6 +775,14 @@ pub fn run_tests_console(opts: &TestOpts, tests: Vec<TestDescAndFn>) -> io::Resu
                         st.failed += 1;
                         st.failures.push((test, stdout));
                     }
+                    TrFailedMsg(msg) => {
+                        st.failed += 1;
+                        let mut stdout = stdout;
+                        stdout.extend_from_slice(
+                            format!("note: {}", msg).as_bytes()
+                        );
+                        st.failures.push((test, stdout));
+                    }
                 }
                 Ok(())
             }
@@ -1270,12 +1280,16 @@ fn calc_result(desc: &TestDesc, task_result: Result<(), Box<Any + Send>>) -> Tes
     match (&desc.should_panic, task_result) {
         (&ShouldPanic::No, Ok(())) |
         (&ShouldPanic::Yes, Err(_)) => TrOk,
-        (&ShouldPanic::YesWithMessage(msg), Err(ref err))
+        (&ShouldPanic::YesWithMessage(msg), Err(ref err)) =>
             if err.downcast_ref::<String>()
-               .map(|e| &**e)
-               .or_else(|| err.downcast_ref::<&'static str>().map(|e| *e))
-               .map(|e| e.contains(msg))
-               .unwrap_or(false) => TrOk,
+                  .map(|e| &**e)
+                  .or_else(|| err.downcast_ref::<&'static str>().map(|e| *e))
+                  .map(|e| e.contains(msg))
+                  .unwrap_or(false) {
+                TrOk
+            } else {
+                TrFailedMsg(format!("Panic did not include expected string '{}'", msg))
+            },
         _ => TrFailed,
     }
 }
@@ -1482,8 +1496,9 @@ pub mod bench {
 
 #[cfg(test)]
 mod tests {
-    use test::{TrFailed, TrIgnored, TrOk, filter_tests, parse_opts, TestDesc, TestDescAndFn,
-               TestOpts, run_test, MetricMap, StaticTestName, DynTestName, DynTestFn, ShouldPanic};
+    use test::{TrFailed, TrFailedMsg, TrIgnored, TrOk, filter_tests, parse_opts, TestDesc,
+               TestDescAndFn, TestOpts, run_test, MetricMap, StaticTestName, DynTestName,
+               DynTestFn, ShouldPanic};
     use std::sync::mpsc::channel;
 
     #[test]
@@ -1565,18 +1580,20 @@ mod tests {
         fn f() {
             panic!("an error message");
         }
+        let expected = "foobar";
+        let failed_msg = "Panic did not include expected string";
         let desc = TestDescAndFn {
             desc: TestDesc {
                 name: StaticTestName("whatever"),
                 ignore: false,
-                should_panic: ShouldPanic::YesWithMessage("foobar"),
+                should_panic: ShouldPanic::YesWithMessage(expected),
             },
             testfn: DynTestFn(Box::new(move |()| f())),
         };
         let (tx, rx) = channel();
         run_test(&TestOpts::new(), false, desc, tx);
         let (_, res, _) = rx.recv().unwrap();
-        assert!(res == TrFailed);
+        assert!(res == TrFailedMsg(format!("{} '{}'", failed_msg, expected)));
     }
 
     #[test]
diff --git a/src/test/run-fail/test-should-panic-bad-message.rs b/src/test/run-fail/test-should-panic-bad-message.rs
new file mode 100644
index 00000000000..7186672b404
--- /dev/null
+++ b/src/test/run-fail/test-should-panic-bad-message.rs
@@ -0,0 +1,19 @@
+// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// compile-flags: --test
+
+// error-pattern:panicked at 'bar'
+// check-stdout
+#[test]
+#[should_panic(expected = "foo")]
+pub fn test_bar() {
+    panic!("bar")
+}
diff --git a/src/test/run-fail/test-should-panic-no-message.rs b/src/test/run-fail/test-should-panic-no-message.rs
new file mode 100644
index 00000000000..50dc2aed8e9
--- /dev/null
+++ b/src/test/run-fail/test-should-panic-no-message.rs
@@ -0,0 +1,19 @@
+// Copyright 2014 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// compile-flags: --test
+
+// error-pattern:panicked at 'explicit panic'
+// check-stdout
+#[test]
+#[should_panic(expected = "foo")]
+pub fn test_explicit() {
+    panic!()
+}
diff --git a/src/test/run-pass/test-should-panic-attr.rs b/src/test/run-pass/test-should-panic-attr.rs
new file mode 100644
index 00000000000..2d068872a4d
--- /dev/null
+++ b/src/test/run-pass/test-should-panic-attr.rs
@@ -0,0 +1,46 @@
+// Copyright 2016 The Rust Project Developers. See the COPYRIGHT
+// file at the top-level directory of this distribution and at
+// http://rust-lang.org/COPYRIGHT.
+//
+// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
+// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+
+// compile-flags: --test
+
+#[test]
+#[should_panic = "foo"]
+//~^ WARN: attribute must be of the form:
+fn test1() {
+    panic!();
+}
+
+#[test]
+#[should_panic(expected)]
+//~^ WARN: argument must be of the form:
+fn test2() {
+    panic!();
+}
+
+#[test]
+#[should_panic(expect)]
+//~^ WARN: argument must be of the form:
+fn test3() {
+    panic!();
+}
+
+#[test]
+#[should_panic(expected(foo, bar))]
+//~^ WARN: argument must be of the form:
+fn test4() {
+    panic!();
+}
+
+#[test]
+#[should_panic(expected = "foo", bar)]
+//~^ WARN: argument must be of the form:
+fn test5() {
+    panic!();
+}