From 5284b95a064280d2ed70e6fabf6eb863689d3848 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Thu, 24 Jan 2019 06:58:53 +0200 Subject: [PATCH 1/2] 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 | 186 ++++++++++++++++---------------- tests/ui/expect_fun_call.fixed | 84 +++++++++++++++ tests/ui/expect_fun_call.rs | 38 +++++-- tests/ui/expect_fun_call.stderr | 46 +++++--- 4 files changed, 238 insertions(+), 116 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 6c1befe6e53a..0571f1d92d70 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, - } - } - - 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 not a known type, return early - if known_types.iter().all(|&k| !match_type(cx, self_type, k)) { - return; - } + if args.len() != 2 || name != "expect" || !is_call(&args[1].node) { + return; + } - if !is_call(&arg.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; + }; - let closure = if match_type(cx, self_type, &paths::OPTION) { - "||" - } else { - "|_|" + // 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(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 span_replace_word = method_span.with_hi(expr.span.hi()); - args.extend(generate_format_arg_snippet(cx, fmt_args, &mut applicability)); + let mut applicability = Applicability::MachineApplicable; - let sugg = args.join(", "); + //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]; - 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, - ); + let mut applicability = Applicability::MachineApplicable; + let mut args = vec![snippet(cx, fmt_spec.span, "..").into_owned()]; - return; - } + args.extend(generate_format_arg_snippet(cx, fmt_args, &mut applicability)); - let mut applicability = Applicability::MachineApplicable; - let sugg: Cow<'_, _> = snippet_with_applicability(cx, arg.span, "..", &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({} {{ let msg = {}; panic!(msg) }}))", closure, 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({} panic!({}))", closure_args, 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), + // 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 000000000000..1f74f6b8cf14 --- /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 7f0ca0fe8095..2d8b4925f358 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 #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)); + } +} diff --git a/tests/ui/expect_fun_call.stderr b/tests/ui/expect_fun_call.stderr index a60bd7e4ed3b..900e251d964c 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 + --> $DIR/expect_fun_call.rs:56:17 | -LL | Some("foo").expect({ &format!("error") }); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| { let msg = { &format!("error") }; panic!(msg) }))` +LL | Some("foo").expect(format!("{} {}", 1, 2).as_ref()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| panic!("{} {}", 1, 2))` error: use of `expect` followed by a function call - --> $DIR/expect_fun_call.rs:59:17 + --> $DIR/expect_fun_call.rs:77:21 | -LL | Some("foo").expect(format!("error").as_ref()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| panic!("error"))` +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:61:17 + --> $DIR/expect_fun_call.rs:78:21 | -LL | Some("foo").expect(format!("{} {}", 1, 2).as_ref()); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| panic!("{} {}", 1, 2))` +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 7 previous errors +error: aborting due to 10 previous errors From 18cacbabb43f2fd87e20706be852e62aea282257 Mon Sep 17 00:00:00 2001 From: Michael Wright Date: Sat, 26 Jan 2019 11:55:54 +0200 Subject: [PATCH 2/2] Incorporate review suggestions --- clippy_lints/src/methods/mod.rs | 79 ++++++++++++++++++--------------- 1 file changed, 44 insertions(+), 35 deletions(-) diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 0571f1d92d70..4126b78e676b 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -1148,6 +1148,48 @@ 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]) { + // 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 + } + + // 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( cx: &LateContext<'_, '_>, a: &hir::Expr, @@ -1195,29 +1237,7 @@ fn lint_expect_fun_call(cx: &LateContext<'_, '_>, expr: &hir::Expr, method_span: 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 arg_root = get_arg_root(cx, &args[1]); let span_replace_word = method_span.with_hi(expr.span.hi()); @@ -1230,7 +1250,6 @@ fn lint_expect_fun_call(cx: &LateContext<'_, '_>, expr: &hir::Expr, method_span: 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)); @@ -1252,18 +1271,8 @@ fn lint_expect_fun_call(cx: &LateContext<'_, '_>, expr: &hir::Expr, method_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 { + if requires_to_string(cx, arg_root) { arg_root_snippet.to_mut().push_str(".to_string()"); }