Fix expect_fun_call lint suggestions

This commit corrects some bad suggestions produced by the
`expect_fun_call` lint and enables `rust-fix` checking on the tests.

Addresses #3630

clippy_lints/src/methods/mod.rs

@@ -1148,28 +1148,6 @@ fn lint_or_fun_call(cx: &LateContext<'_, '_>, expr: &hir::Expr, method_span: Spa
 
 /// 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]) {
-    fn extract_format_args(arg: &hir::Expr) -> Option<(&hir::Expr, &hir::Expr)> {
-        let arg = match &arg.node {
-            hir::ExprKind::AddrOf(_, expr) => expr,
-            hir::ExprKind::MethodCall(method_name, _, args)
-                if method_name.ident.name == "as_str" || method_name.ident.name == "as_ref" =>
-            {
-                &args[0]
-            },
-            _ => arg,
-        };
-
-        if let hir::ExprKind::Call(ref inner_fun, ref inner_args) = arg.node {
-            if is_expn_of(inner_fun.span, "format").is_some() && inner_args.len() == 1 {
-                if let hir::ExprKind::Call(_, format_args) = &inner_args[0].node {
-                    return Some((&format_args[0], &format_args[1]));
-                }
-            }
-        }
-
-        None
-    }
-
     fn generate_format_arg_snippet(
         cx: &LateContext<'_, '_>,
         a: &hir::Expr,
@@ -1189,14 +1167,6 @@ fn lint_expect_fun_call(cx: &LateContext<'_, '_>, expr: &hir::Expr, method_span:
         unreachable!()
     }
 
-    fn check_general_case(
-        cx: &LateContext<'_, '_>,
-        name: &str,
-        method_span: Span,
-        self_expr: &hir::Expr,
-        arg: &hir::Expr,
-        span: Span,
-    ) {
     fn is_call(node: &hir::ExprKind) -> bool {
         match node {
             hir::ExprKind::AddrOf(_, expr) => {
@@ -1212,30 +1182,54 @@ fn lint_expect_fun_call(cx: &LateContext<'_, '_>, expr: &hir::Expr, method_span:
         }
     }
 
-        if name != "expect" {
+    if args.len() != 2 || name != "expect" || !is_call(&args[1].node) {
         return;
     }
 
-        let self_type = cx.tables.expr_ty(self_expr);
-        let known_types = &[&paths::OPTION, &paths::RESULT];
-
-        // if not a known type, return early
-        if known_types.iter().all(|&k| !match_type(cx, self_type, k)) {
-            return;
-        }
-
-        if !is_call(&arg.node) {
-            return;
-        }
-
-        let closure = if match_type(cx, self_type, &paths::OPTION) {
+    let receiver_type = cx.tables.expr_ty(&args[0]);
+    let closure_args = if match_type(cx, receiver_type, &paths::OPTION) {
         "||"
-        } else {
+    } else if match_type(cx, receiver_type, &paths::RESULT) {
         "|_|"
+    } else {
+        return;
     };
-        let span_replace_word = method_span.with_hi(span.hi());
 
-        if let Some((fmt_spec, fmt_args)) = extract_format_args(arg) {
+    // Strip off `&`, `as_ref()` and `as_str()` until we're left with either a `String` or `&str`
+    // which we call `arg_root`.
+    let mut arg_root = &args[1];
+    loop {
+        arg_root = match &arg_root.node {
+            hir::ExprKind::AddrOf(_, expr) => expr,
+            hir::ExprKind::MethodCall(method_name, _, call_args) => {
+                if call_args.len() == 1
+                    && (method_name.ident.name == "as_str" || method_name.ident.name == "as_ref")
+                    && {
+                        let arg_type = cx.tables.expr_ty(&call_args[0]);
+                        let base_type = walk_ptrs_ty(arg_type);
+                        base_type.sty == ty::Str || match_type(cx, base_type, &paths::STRING)
+                    }
+                {
+                    &call_args[0]
+                } else {
+                    break;
+                }
+            },
+            _ => break,
+        };
+    }
+
+    let span_replace_word = method_span.with_hi(expr.span.hi());
+
+    let mut applicability = Applicability::MachineApplicable;
+
+    //Special handling for `format!` as arg_root
+    if let hir::ExprKind::Call(ref inner_fun, ref inner_args) = arg_root.node {
+        if is_expn_of(inner_fun.span, "format").is_some() && inner_args.len() == 1 {
+            if let hir::ExprKind::Call(_, format_args) = &inner_args[0].node {
+                let fmt_spec = &format_args[0];
+                let fmt_args = &format_args[1];
+
                 let mut applicability = Applicability::MachineApplicable;
                 let mut args = vec![snippet(cx, fmt_spec.span, "..").into_owned()];
 
@@ -1249,15 +1243,29 @@ fn lint_expect_fun_call(cx: &LateContext<'_, '_>, expr: &hir::Expr, method_span:
                     span_replace_word,
                     &format!("use of `{}` followed by a function call", name),
                     "try this",
-                format!("unwrap_or_else({} panic!({}))", closure, sugg),
+                    format!("unwrap_or_else({} panic!({}))", closure_args, sugg),
                     applicability,
                 );
 
                 return;
             }
+        }
+    }
 
-        let mut applicability = Applicability::MachineApplicable;
-        let sugg: Cow<'_, _> = snippet_with_applicability(cx, arg.span, "..", &mut applicability);
+    // If root_arg is `&'static str` or `String` we can use it directly in the `panic!` call otherwise
+    // we must use `to_string` to convert it.
+    let mut arg_root_snippet: Cow<'_, _> = snippet_with_applicability(cx, arg_root.span, "..", &mut applicability);
+    let arg_root_ty = cx.tables.expr_ty(arg_root);
+    let mut requires_conv = !match_type(cx, arg_root_ty, &paths::STRING);
+    if let ty::Ref(ty::ReStatic, ty, ..) = arg_root_ty.sty {
+        if ty.sty == ty::Str {
+            requires_conv = false;
+        }
+    };
+
+    if requires_conv {
+        arg_root_snippet.to_mut().push_str(".to_string()");
+    }
 
     span_lint_and_sugg(
         cx,
@@ -1265,17 +1273,9 @@ fn lint_expect_fun_call(cx: &LateContext<'_, '_>, expr: &hir::Expr, method_span:
         span_replace_word,
         &format!("use of `{}` followed by a function call", name),
         "try this",
-            format!("unwrap_or_else({} {{ let msg = {}; panic!(msg) }}))", closure, sugg),
+        format!("unwrap_or_else({} {{ panic!({}) }})", closure_args, arg_root_snippet),
         applicability,
     );
-    }
-
-    if args.len() == 2 {
-        match args[1].node {
-            hir::ExprKind::Lit(_) => {},
-            _ => check_general_case(cx, name, method_span, &args[0], &args[1], expr.span),
-        }
-    }
 }
 
 /// Checks for the `CLONE_ON_COPY` lint.

tests/ui/expect_fun_call.fixed

@@ -0,0 +1,84 @@
+// run-rustfix
+
+#![warn(clippy::expect_fun_call)]
+
+/// Checks implementation of the `EXPECT_FUN_CALL` lint
+
+fn main() {
+    struct Foo;
+
+    impl Foo {
+        fn new() -> Self {
+            Foo
+        }
+
+        fn expect(&self, msg: &str) {
+            panic!("{}", msg)
+        }
+    }
+
+    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.unwrap_or_else(|| panic!("Error {}: fake error", error_code));
+
+    let with_none_and_as_str: Option<i32> = None;
+    with_none_and_as_str.unwrap_or_else(|| panic!("Error {}: fake error", error_code));
+
+    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.unwrap_or_else(|_| panic!("Error {}: fake error", error_code));
+
+    let with_err_and_as_str: Result<(), ()> = Err(());
+    with_err_and_as_str.unwrap_or_else(|_| panic!("Error {}: fake error", error_code));
+
+    let with_dummy_type = Foo::new();
+    with_dummy_type.expect("another test string");
+
+    let with_dummy_type_and_format = Foo::new();
+    with_dummy_type_and_format.expect(&format!("Error {}: fake error", error_code));
+
+    let with_dummy_type_and_as_str = Foo::new();
+    with_dummy_type_and_as_str.expect(format!("Error {}: fake error", error_code).as_str());
+
+    //Issue #2937
+    Some("foo").unwrap_or_else(|| panic!("{} {}", 1, 2));
+
+    //Issue #2979 - this should not lint
+    {
+        let msg = "bar";
+        Some("foo").expect(msg);
+    }
+
+    {
+        fn get_string() -> String {
+            "foo".to_string()
+        }
+
+        fn get_static_str() -> &'static str {
+            "foo"
+        }
+
+        fn get_non_static_str(_: &u32) -> &str {
+            "foo"
+        }
+
+        Some("foo").unwrap_or_else(|| { panic!(get_string()) });
+        Some("foo").unwrap_or_else(|| { panic!(get_string()) });
+        Some("foo").unwrap_or_else(|| { panic!(get_string()) });
+
+        Some("foo").unwrap_or_else(|| { panic!(get_static_str()) });
+        Some("foo").unwrap_or_else(|| { panic!(get_non_static_str(&0).to_string()) });
+    }
+}

tests/ui/expect_fun_call.rs

@@ -1,9 +1,10 @@
+// run-rustfix
+
 #![warn(clippy::expect_fun_call)]
-#![allow(clippy::useless_format)]
 
 /// Checks implementation of the `EXPECT_FUN_CALL` lint
 
-fn expect_fun_call() {
+fn main() {
     struct Foo;
 
     impl Foo {
@@ -51,14 +52,33 @@ fn expect_fun_call() {
     let with_dummy_type_and_as_str = Foo::new();
     with_dummy_type_and_as_str.expect(format!("Error {}: fake error", error_code).as_str());
 
+    //Issue #2937
+    Some("foo").expect(format!("{} {}", 1, 2).as_ref());
+
     //Issue #2979 - this should not lint
+    {
         let msg = "bar";
         Some("foo").expect(msg);
+    }
 
-    Some("foo").expect({ &format!("error") });
-    Some("foo").expect(format!("error").as_ref());
+    {
+        fn get_string() -> String {
+            "foo".to_string()
+        }
 
-    Some("foo").expect(format!("{} {}", 1, 2).as_ref());
+        fn get_static_str() -> &'static str {
+            "foo"
+        }
+
+        fn get_non_static_str(_: &u32) -> &str {
+            "foo"
+        }
+
+        Some("foo").expect(&get_string());
+        Some("foo").expect(get_string().as_ref());
+        Some("foo").expect(get_string().as_str());
+
+        Some("foo").expect(get_static_str());
+        Some("foo").expect(get_non_static_str(&0));
+    }
 }
-
-fn main() {}

tests/ui/expect_fun_call.stderr

@@ -1,5 +1,5 @@
 error: use of `expect` followed by a function call
-  --> $DIR/expect_fun_call.rs:27:26
+  --> $DIR/expect_fun_call.rs:28:26
    |
 LL |     with_none_and_format.expect(&format!("Error {}: fake error", error_code));
    |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| panic!("Error {}: fake error", error_code))`
@@ -7,40 +7,58 @@ LL |     with_none_and_format.expect(&format!("Error {}: fake error", error_code
    = note: `-D clippy::expect-fun-call` implied by `-D warnings`
 
 error: use of `expect` followed by a function call
-  --> $DIR/expect_fun_call.rs:30:26
+  --> $DIR/expect_fun_call.rs:31:26
    |
 LL |     with_none_and_as_str.expect(format!("Error {}: fake error", error_code).as_str());
    |                          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| panic!("Error {}: fake error", error_code))`
 
 error: use of `expect` followed by a function call
-  --> $DIR/expect_fun_call.rs:40:25
+  --> $DIR/expect_fun_call.rs:41:25
    |
 LL |     with_err_and_format.expect(&format!("Error {}: fake error", error_code));
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|_| panic!("Error {}: fake error", error_code))`
 
 error: use of `expect` followed by a function call
-  --> $DIR/expect_fun_call.rs:43:25
+  --> $DIR/expect_fun_call.rs:44:25
    |
 LL |     with_err_and_as_str.expect(format!("Error {}: fake error", error_code).as_str());
    |                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|_| panic!("Error {}: fake error", error_code))`
 
 error: use of `expect` followed by a function call
-  --> $DIR/expect_fun_call.rs:58:17
-   |
-LL |     Some("foo").expect({ &format!("error") });
-   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| { let msg = { &format!("error") }; panic!(msg) }))`
-
-error: use of `expect` followed by a function call
-  --> $DIR/expect_fun_call.rs:59:17
-   |
-LL |     Some("foo").expect(format!("error").as_ref());
-   |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| panic!("error"))`
-
-error: use of `expect` followed by a function call
-  --> $DIR/expect_fun_call.rs:61:17
+  --> $DIR/expect_fun_call.rs:56:17
    |
 LL |     Some("foo").expect(format!("{} {}", 1, 2).as_ref());
    |                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| panic!("{} {}", 1, 2))`
 
-error: aborting due to 7 previous errors
+error: use of `expect` followed by a function call
+  --> $DIR/expect_fun_call.rs:77:21
+   |
+LL |         Some("foo").expect(&get_string());
+   |                     ^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| { panic!(get_string()) })`
+
+error: use of `expect` followed by a function call
+  --> $DIR/expect_fun_call.rs:78:21
+   |
+LL |         Some("foo").expect(get_string().as_ref());
+   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| { panic!(get_string()) })`
+
+error: use of `expect` followed by a function call
+  --> $DIR/expect_fun_call.rs:79:21
+   |
+LL |         Some("foo").expect(get_string().as_str());
+   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| { panic!(get_string()) })`
+
+error: use of `expect` followed by a function call
+  --> $DIR/expect_fun_call.rs:81:21
+   |
+LL |         Some("foo").expect(get_static_str());
+   |                     ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| { panic!(get_static_str()) })`
+
+error: use of `expect` followed by a function call
+  --> $DIR/expect_fun_call.rs:82:21
+   |
+LL |         Some("foo").expect(get_non_static_str(&0));
+   |                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| { panic!(get_non_static_str(&0).to_string()) })`
+
+error: aborting due to 10 previous errors