Warn if non-trivial work is done inside .expect

- added tests for common usages of format and as_str arguments to expect
- added tests for usages of Option and Result types
- given performance impact of passing non literal expressions to expect, added to perf group

clippy_lints/src/lib.rs

@@ -590,6 +590,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         methods::OK_EXPECT,
         methods::OPTION_MAP_OR_NONE,
         methods::OR_FUN_CALL,
+        methods::EXPECT_FUN_CALL,
         methods::SEARCH_IS_SOME,
         methods::SHOULD_IMPLEMENT_TRAIT,
         methods::SINGLE_CHAR_PATTERN,
@@ -899,6 +900,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         loops::UNUSED_COLLECT,
         methods::ITER_NTH,
         methods::OR_FUN_CALL,
+        methods::EXPECT_FUN_CALL,
         methods::SINGLE_CHAR_PATTERN,
         misc::CMP_OWNED,
         mutex_atomic::MUTEX_ATOMIC,

clippy_lints/src/methods.rs

@@ -329,6 +329,32 @@ declare_clippy_lint! {
     "using any `*or` method with a function call, which suggests `*or_else`"
 }
 
+/// **What it does:** Checks for calls to `.expect(&format!(...))`, `.expect(foo(..))`,
+/// etc., and suggests to use `unwrap_or_else` instead
+///
+/// **Why is this bad?** The function will always be called.
+///
+/// **Known problems:** If the function has side-effects, not calling it will
+/// change the semantic of the program, but you shouldn't rely on that anyway.
+/// 
+/// **Example:**
+/// ```rust
+/// foo.expect(&format("Err {}: {}", err_code, err_msg))
+/// ```
+/// or
+/// ```rust
+/// foo.expect(format("Err {}: {}", err_code, err_msg).as_str())
+/// ```
+/// this can instead be written:
+/// ```rust
+/// foo.unwrap_or_else(|_| panic!(&format("Err {}: {}", err_code, err_msg)))
+/// ```
+declare_clippy_lint! {
+    pub EXPECT_FUN_CALL,
+    perf,
+    "using any `expect` method with a function call"
+}
+
 /// **What it does:** Checks for usage of `.clone()` on a `Copy` type.
 ///
 /// **Why is this bad?** The only reason `Copy` types implement `Clone` is for
@@ -657,6 +683,7 @@ impl LintPass for Pass {
             RESULT_MAP_UNWRAP_OR_ELSE,
             OPTION_MAP_OR_NONE,
             OR_FUN_CALL,
+            EXPECT_FUN_CALL,
             CHARS_NEXT_CMP,
             CHARS_LAST_CMP,
             CLONE_ON_COPY,
@@ -741,6 +768,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Pass {
                 }
 
                 lint_or_fun_call(cx, expr, *method_span, &method_call.name.as_str(), args);
+                lint_expect_fun_call(cx, expr, *method_span, &method_call.name.as_str(), args);
 
                 let self_ty = cx.tables.expr_ty_adjusted(&args[0]);
                 if args.len() == 1 && method_call.name == "clone" {
@@ -964,6 +992,48 @@ fn lint_or_fun_call(cx: &LateContext, expr: &hir::Expr, method_span: Span, name:
     }
 }
 
+/// Checks for the `EXPECT_FUN_CALL` lint.
+fn lint_expect_fun_call(cx: &LateContext, expr: &hir::Expr, method_span: Span, name: &str, args: &[hir::Expr]) {
+    #[allow(too_many_arguments)]
+    fn check_general_case(
+        cx: &LateContext,
+        name: &str,
+        method_span: Span,
+        arg: &hir::Expr,
+        span: Span,
+    ) {
+        if name != "expect" {
+            return;
+        }
+
+        // don't lint for constant values
+        let owner_def = cx.tcx.hir.get_parent_did(arg.id);
+        let promotable = cx.tcx.rvalue_promotable_map(owner_def).contains(&arg.hir_id.local_id);
+        if promotable {
+            return;
+        }
+
+        let sugg: Cow<_> = snippet(cx, arg.span, "..");
+        let span_replace_word = method_span.with_hi(span.hi());
+        
+        span_lint_and_sugg(
+            cx,
+            EXPECT_FUN_CALL,
+            span_replace_word,
+            &format!("use of `{}` followed by a function call", name),
+            "try this",
+            format!("unwrap_or_else(|_| panic!({}))", sugg),
+        );
+    }
+
+    if args.len() == 2 {
+        match args[1].node {
+            hir::ExprLit(_) => {},
+            _ => check_general_case(cx, name, method_span, &args[1], expr.span),
+        }
+    }
+}
+
 /// Checks for the `CLONE_ON_COPY` lint.
 fn lint_clone_on_copy(cx: &LateContext, expr: &hir::Expr, arg: &hir::Expr, arg_ty: Ty) {
     let ty = cx.tables.expr_ty(expr);

tests/ui/methods.rs

@@ -342,6 +342,35 @@ fn or_fun_call() {
     let _ = stringy.unwrap_or("".to_owned());
 }
 
+/// Checks implementation of the `EXPECT_FUN_CALL` lint
+fn expect_fun_call() {
+    let with_some = Some("value");
+    with_some.expect("error");
+
+    let with_none: Option<i32> = None;
+    with_none.expect("error");
+
+    let error_code = 123_i32;
+    let with_none_and_format: Option<i32> = None;
+    with_none_and_format.expect(&format!("Error {}: fake error", error_code));
+
+    let with_none_and_as_str: Option<i32> = None;
+    with_none_and_as_str.expect(format!("Error {}: fake error", error_code).as_str());
+
+    let with_ok: Result<(), ()> = Ok(());
+    with_ok.expect("error");
+
+    let with_err: Result<(), ()> = Err(());
+    with_err.expect("error");
+
+    let error_code = 123_i32;
+    let with_err_and_format: Result<(), ()> = Err(());
+    with_err_and_format.expect(&format!("Error {}: fake error", error_code));
+
+    let with_err_and_as_str: Result<(), ()> = Err(());
+    with_err_and_as_str.expect(format!("Error {}: fake error", error_code).as_str());
+}
+
 /// Checks implementation of `ITER_NTH` lint
 fn iter_nth() {
     let mut some_vec = vec![0, 1, 2, 3];

tests/ui/methods.stderr

@@ -423,83 +423,109 @@ error: use of `unwrap_or` followed by a function call
 342 |     let _ = stringy.unwrap_or("".to_owned());
     |                     ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| "".to_owned())`
 
-error: called `.iter().nth()` on a Vec. Calling `.get()` is both faster and more readable
-   --> $DIR/methods.rs:353:23
+error: use of `expect` followed by a function call
+   --> $DIR/methods.rs:355:26
     |
-353 |         let bad_vec = some_vec.iter().nth(3);
+355 |     with_none_and_format.expect(&format!("Error {}: fake error", error_code));
+    |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|_| panic!(&format!("Error {}: fake error", error_code)))`
+    |
+    = note: `-D expect-fun-call` implied by `-D warnings`
+
+error: use of `expect` followed by a function call
+   --> $DIR/methods.rs:358:26
+    |
+358 |     with_none_and_as_str.expect(format!("Error {}: fake error", error_code).as_str());
+    |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|_| panic!(format!("Error {}: fake error", error_code).as_str()))`
+
+error: use of `expect` followed by a function call
+   --> $DIR/methods.rs:368:25
+    |
+368 |     with_err_and_format.expect(&format!("Error {}: fake error", error_code));
+    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|_| panic!(&format!("Error {}: fake error", error_code)))`
+
+error: use of `expect` followed by a function call
+   --> $DIR/methods.rs:371:25
+    |
+371 |     with_err_and_as_str.expect(format!("Error {}: fake error", error_code).as_str());
+    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|_| panic!(format!("Error {}: fake error", error_code).as_str()))`
+
+error: called `.iter().nth()` on a Vec. Calling `.get()` is both faster and more readable
+   --> $DIR/methods.rs:382:23
+    |
+382 |         let bad_vec = some_vec.iter().nth(3);
     |                       ^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: `-D iter-nth` implied by `-D warnings`
 
 error: called `.iter().nth()` on a slice. Calling `.get()` is both faster and more readable
-   --> $DIR/methods.rs:354:26
+   --> $DIR/methods.rs:383:26
     |
-354 |         let bad_slice = &some_vec[..].iter().nth(3);
+383 |         let bad_slice = &some_vec[..].iter().nth(3);
     |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: called `.iter().nth()` on a slice. Calling `.get()` is both faster and more readable
-   --> $DIR/methods.rs:355:31
+   --> $DIR/methods.rs:384:31
     |
-355 |         let bad_boxed_slice = boxed_slice.iter().nth(3);
+384 |         let bad_boxed_slice = boxed_slice.iter().nth(3);
     |                               ^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: called `.iter().nth()` on a VecDeque. Calling `.get()` is both faster and more readable
-   --> $DIR/methods.rs:356:29
+   --> $DIR/methods.rs:385:29
     |
-356 |         let bad_vec_deque = some_vec_deque.iter().nth(3);
+385 |         let bad_vec_deque = some_vec_deque.iter().nth(3);
     |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: called `.iter_mut().nth()` on a Vec. Calling `.get_mut()` is both faster and more readable
-   --> $DIR/methods.rs:361:23
+   --> $DIR/methods.rs:390:23
     |
-361 |         let bad_vec = some_vec.iter_mut().nth(3);
+390 |         let bad_vec = some_vec.iter_mut().nth(3);
     |                       ^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: called `.iter_mut().nth()` on a slice. Calling `.get_mut()` is both faster and more readable
-   --> $DIR/methods.rs:364:26
+   --> $DIR/methods.rs:393:26
     |
-364 |         let bad_slice = &some_vec[..].iter_mut().nth(3);
+393 |         let bad_slice = &some_vec[..].iter_mut().nth(3);
     |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: called `.iter_mut().nth()` on a VecDeque. Calling `.get_mut()` is both faster and more readable
-   --> $DIR/methods.rs:367:29
+   --> $DIR/methods.rs:396:29
     |
-367 |         let bad_vec_deque = some_vec_deque.iter_mut().nth(3);
+396 |         let bad_vec_deque = some_vec_deque.iter_mut().nth(3);
     |                             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: called `skip(x).next()` on an iterator. This is more succinctly expressed by calling `nth(x)`
-   --> $DIR/methods.rs:379:13
+   --> $DIR/methods.rs:408:13
     |
-379 |     let _ = some_vec.iter().skip(42).next();
+408 |     let _ = some_vec.iter().skip(42).next();
     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     |
     = note: `-D iter-skip-next` implied by `-D warnings`
 
 error: called `skip(x).next()` on an iterator. This is more succinctly expressed by calling `nth(x)`
-   --> $DIR/methods.rs:380:13
+   --> $DIR/methods.rs:409:13
     |
-380 |     let _ = some_vec.iter().cycle().skip(42).next();
+409 |     let _ = some_vec.iter().cycle().skip(42).next();
     |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: called `skip(x).next()` on an iterator. This is more succinctly expressed by calling `nth(x)`
-   --> $DIR/methods.rs:381:13
+   --> $DIR/methods.rs:410:13
     |
-381 |     let _ = (1..10).skip(10).next();
+410 |     let _ = (1..10).skip(10).next();
     |             ^^^^^^^^^^^^^^^^^^^^^^^
 
 error: called `skip(x).next()` on an iterator. This is more succinctly expressed by calling `nth(x)`
-   --> $DIR/methods.rs:382:14
+   --> $DIR/methods.rs:411:14
     |
-382 |     let _ = &some_vec[..].iter().skip(3).next();
+411 |     let _ = &some_vec[..].iter().skip(3).next();
     |              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
 
 error: used unwrap() on an Option value. If you don't want to handle the None case gracefully, consider using expect() to provide a better panic message
-   --> $DIR/methods.rs:391:13
+   --> $DIR/methods.rs:420:13
     |
-391 |     let _ = opt.unwrap();
+420 |     let _ = opt.unwrap();
     |             ^^^^^^^^^^^^
     |
     = note: `-D option-unwrap-used` implied by `-D warnings`
 
-error: aborting due to 66 previous errors
+error: aborting due to 70 previous errors