From 5284b95a064280d2ed70e6fabf6eb863689d3848 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Thu, 24 Jan 2019 06:58:53 +0200 Subject: [PATCH] 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 | 206 ++++++++++++++++---------------- tests/ui/expect_fun_call.fixed | 84 +++++++++++++ tests/ui/expect_fun_call.rs | 42 +++++-- tests/ui/expect_fun_call.stderr | 54 ++++++--- 4 files changed, 254 insertions(+), 132 deletions(-) create mode 100644 tests/ui/expect_fun_call.fixed diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 6c1befe6e..0571f1d92 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/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,93 +1167,115 @@ 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, + 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 args.len() != 2 || name != "expect" || !is_call(&args[1].node) { + 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; + }; + + // 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()]; + + args.extend(generate_format_arg_snippet(cx, fmt_args, &mut applicability)); + + let sugg = args.join(", "); + + 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, + ); + + return; } } - - if name != "expect" { - 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) { - "||" - } else { - "|_|" - }; - let span_replace_word = method_span.with_hi(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()]; - - args.extend(generate_format_arg_snippet(cx, fmt_args, &mut applicability)); - - let sugg = args.join(", "); - - 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, - ); - - return; - } - - 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({} {{ let msg = {}; panic!(msg) }}))", closure, sugg), - 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), + // 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, + 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. diff --git a/tests/ui/expect_fun_call.fixed b/tests/ui/expect_fun_call.fixed new file mode 100644 index 000000000..1f74f6b8c --- /dev/null +++ b/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 = None; + with_none.expect("error"); + + let error_code = 123_i32; + let with_none_and_format: Option = None; + with_none_and_format.unwrap_or_else(|| panic!("Error {}: fake error", error_code)); + + let with_none_and_as_str: Option = 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()) }); + } +} diff --git a/tests/ui/expect_fun_call.rs b/tests/ui/expect_fun_call.rs index 7f0ca0fe8..2d8b4925f 100644 --- a/tests/ui/expect_fun_call.rs +++ b/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 #2979 - this should not lint - let msg = "bar"; - Some("foo").expect(msg); - - Some("foo").expect({ &format!("error") }); - Some("foo").expect(format!("error").as_ref()); - + //Issue #2937 Some("foo").expect(format!("{} {}", 1, 2).as_ref()); -} -fn main() {} + //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").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)); + } +} diff --git a/tests/ui/expect_fun_call.stderr b/tests/ui/expect_fun_call.stderr index a60bd7e4e..900e251d9 100644 --- a/tests/ui/expect_fun_call.stderr +++ b/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