Auto merge of #3690 - mikerite:fix-3630-expect-fun-call, r=phansch

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

Author: bors

clippy_lints/src/methods/mod.rs

@@ -1148,26 +1148,46 @@ 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]));
-                }
-            }
+    // Strip `&`, `as_ref()` and `as_str()` off `arg` until we're left with either a `String` or
+    // `&str`
+    fn get_arg_root<'a>(cx: &LateContext<'_, '_>, arg: &'a hir::Expr) -> &'a hir::Expr {
+        let mut arg_root = arg;
+        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,
+            };
         }
+        arg_root
+    }
 
-        None
+    // Only `&'static str` or `String` can be used directly in the `panic!`. Other types should be
+    // converted to string.
+    fn requires_to_string(cx: &LateContext<'_, '_>, arg: &hir::Expr) -> bool {
+        let arg_ty = cx.tables.expr_ty(arg);
+        if match_type(cx, arg_ty, &paths::STRING) {
+            return false;
+        }
+        if let ty::Ref(ty::ReStatic, ty, ..) = arg_ty.sty {
+            if ty.sty == ty::Str {
+                return false;
+            }
+        };
+        true
     }
 
     fn generate_format_arg_snippet(
@@ -1189,93 +1209,82 @@ 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) => {
-                    is_call(&expr.node)
-                },
-                hir::ExprKind::Call(..)
-                | hir::ExprKind::MethodCall(..)
-                // These variants are debatable or require further examination
-                | hir::ExprKind::If(..)
-                | hir::ExprKind::Match(..)
-                | hir::ExprKind::Block{ .. } => true,
-                _ => false,
-            }
-        }
-
-        if name != "expect" {
-            return;
+    fn is_call(node: &hir::ExprKind) -> bool {
+        match node {
+            hir::ExprKind::AddrOf(_, expr) => {
+                is_call(&expr.node)
+            },
+            hir::ExprKind::Call(..)
+            | hir::ExprKind::MethodCall(..)
+            // These variants are debatable or require further examination
+            | hir::ExprKind::If(..)
+            | hir::ExprKind::Match(..)
+            | hir::ExprKind::Block{ .. } => true,
+            _ => false,
         }
+    }
 
-        let self_type = cx.tables.expr_ty(self_expr);
-        let known_types = &[&paths::OPTION, &paths::RESULT];
+    if args.len() != 2 || name != "expect" || !is_call(&args[1].node) {
+        return;
+    }
 
-        // if not a known type, return early
-        if known_types.iter().all(|&k| !match_type(cx, self_type, k)) {
-            return;
-        }
+    let receiver_type = cx.tables.expr_ty(&args[0]);
+    let closure_args = if match_type(cx, receiver_type, &paths::OPTION) {
+        "||"
+    } else if match_type(cx, receiver_type, &paths::RESULT) {
+        "|_|"
+    } else {
+        return;
+    };
 
-        if !is_call(&arg.node) {
-            return;
-        }
+    let arg_root = get_arg_root(cx, &args[1]);
 
-        let closure = if match_type(cx, self_type, &paths::OPTION) {
-            "||"
-        } else {
-            "|_|"
-        };
-        let span_replace_word = method_span.with_hi(span.hi());
+    let span_replace_word = method_span.with_hi(expr.span.hi());
 
-        if let Some((fmt_spec, fmt_args)) = extract_format_args(arg) {
-            let mut applicability = Applicability::MachineApplicable;
-            let mut args = vec![snippet(cx, fmt_spec.span, "..").into_owned()];
+    let mut applicability = Applicability::MachineApplicable;
 
-            args.extend(generate_format_arg_snippet(cx, fmt_args, &mut applicability));
+    //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 sugg = args.join(", ");
+                let mut args = vec![snippet(cx, fmt_spec.span, "..").into_owned()];
 
-            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!({}))", closure, sugg),
-                applicability,
-            );
+                args.extend(generate_format_arg_snippet(cx, fmt_args, &mut applicability));
 
-            return;
-        }
+                let sugg = args.join(", ");
 
-        let mut applicability = Applicability::MachineApplicable;
-        let sugg: Cow<'_, _> = snippet_with_applicability(cx, arg.span, "..", &mut applicability);
+                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!({}))", closure_args, sugg),
+                    applicability,
+                );
 
-        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({} {{ let msg = {}; panic!(msg) }}))", closure, sugg),
-            applicability,
-        );
+                return;
+            }
+        }
     }
 
-    if args.len() == 2 {
-        match args[1].node {
-            hir::ExprKind::Lit(_) => {},
-            _ => check_general_case(cx, name, method_span, &args[0], &args[1], expr.span),
-        }
+    let mut arg_root_snippet: Cow<'_, _> = snippet_with_applicability(cx, arg_root.span, "..", &mut applicability);
+    if requires_to_string(cx, arg_root) {
+        arg_root_snippet.to_mut().push_str(".to_string()");
     }
+
+    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!({}) }})", closure_args, arg_root_snippet),
+        applicability,
+    );
 }
 
 /// 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);
+    {
+        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"
+        }
 
-fn main() {}
+        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));
+    }
+}