From 05c1ccebaff84e978c17abcbd9c68b33d1d96e44 Mon Sep 17 00:00:00 2001 From: Donald Robertson Date: Mon, 28 May 2018 21:00:45 +0200 Subject: [PATCH 01/10] 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 | 2 + clippy_lints/src/methods.rs | 70 +++++++++++++++++++++++++++++++++ tests/ui/methods.rs | 29 ++++++++++++++ tests/ui/methods.stderr | 78 ++++++++++++++++++++++++------------- 4 files changed, 153 insertions(+), 26 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 612cde8a6..70adaa273 100644 --- a/clippy_lints/src/lib.rs +++ b/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, diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 461efb27f..553198ae4 100644 --- a/clippy_lints/src/methods.rs +++ b/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); diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs index 9e2536558..04e3ec13b 100644 --- a/tests/ui/methods.rs +++ b/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 = None; + with_none.expect("error"); + + let error_code = 123_i32; + let with_none_and_format: Option = None; + with_none_and_format.expect(&format!("Error {}: fake error", error_code)); + + let with_none_and_as_str: Option = 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]; diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr index 1dd1ddc3c..2fe374f35 100644 --- a/tests/ui/methods.stderr +++ b/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 From 2b36017bada5f179d3b6d7b57df0559ba5eeb25b Mon Sep 17 00:00:00 2001 From: Donald Robertson Date: Tue, 29 May 2018 09:29:48 +0200 Subject: [PATCH 02/10] Removing unnecessary allow --- clippy_lints/src/methods.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 553198ae4..b3c4bad25 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -994,7 +994,6 @@ 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, From fe8c9d596543955e0513b239669916b55693dff4 Mon Sep 17 00:00:00 2001 From: Donald Robertson Date: Tue, 29 May 2018 09:46:14 +0200 Subject: [PATCH 03/10] Ensuring correct lint message is output for Option and Result type --- clippy_lints/src/methods.rs | 11 +++++++++-- tests/ui/methods.stderr | 4 ++-- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index b3c4bad25..696bd02cf 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -998,6 +998,7 @@ fn lint_expect_fun_call(cx: &LateContext, expr: &hir::Expr, method_span: Span, n cx: &LateContext, name: &str, method_span: Span, + self_expr: &hir::Expr, arg: &hir::Expr, span: Span, ) { @@ -1005,6 +1006,12 @@ fn lint_expect_fun_call(cx: &LateContext, expr: &hir::Expr, method_span: Span, n return; } + let self_ty = cx.tables.expr_ty(self_expr); + let closure = match match_type(cx, self_ty, &paths::OPTION) { + true => "||", + false => "|_|", + }; + // 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); @@ -1021,14 +1028,14 @@ fn lint_expect_fun_call(cx: &LateContext, expr: &hir::Expr, method_span: Span, n span_replace_word, &format!("use of `{}` followed by a function call", name), "try this", - format!("unwrap_or_else(|_| panic!({}))", sugg), + format!("unwrap_or_else({} panic!({}))", closure, sugg), ); } if args.len() == 2 { match args[1].node { hir::ExprLit(_) => {}, - _ => check_general_case(cx, name, method_span, &args[1], expr.span), + _ => check_general_case(cx, name, method_span, &args[0], &args[1], expr.span), } } } diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr index 2fe374f35..f3c608cb7 100644 --- a/tests/ui/methods.stderr +++ b/tests/ui/methods.stderr @@ -427,7 +427,7 @@ error: use of `expect` followed by a function call --> $DIR/methods.rs:355:26 | 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)))` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| panic!(&format!("Error {}: fake error", error_code)))` | = note: `-D expect-fun-call` implied by `-D warnings` @@ -435,7 +435,7 @@ 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()))` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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 From 1ead12c5004cf54c3f0469a19909bf49321926f7 Mon Sep 17 00:00:00 2001 From: Donald Robertson Date: Tue, 29 May 2018 10:20:18 +0200 Subject: [PATCH 04/10] Adding handling and tests for custom type with implemented expect method --- clippy_lints/src/methods.rs | 17 +++++++--- tests/ui/methods.rs | 19 +++++++++++ tests/ui/methods.stderr | 64 ++++++++++++++++++------------------- 3 files changed, 63 insertions(+), 37 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 696bd02cf..38413c5aa 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -1006,11 +1006,13 @@ fn lint_expect_fun_call(cx: &LateContext, expr: &hir::Expr, method_span: Span, n return; } - let self_ty = cx.tables.expr_ty(self_expr); - let closure = match match_type(cx, self_ty, &paths::OPTION) { - 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; + } // don't lint for constant values let owner_def = cx.tcx.hir.get_parent_did(arg.id); @@ -1019,6 +1021,11 @@ fn lint_expect_fun_call(cx: &LateContext, expr: &hir::Expr, method_span: Span, n return; } + let closure = match match_type(cx, self_type, &paths::OPTION) { + true => "||", + false => "|_|", + }; + let sugg: Cow<_> = snippet(cx, arg.span, ".."); let span_replace_word = method_span.with_hi(span.hi()); diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs index 04e3ec13b..b04c008ba 100644 --- a/tests/ui/methods.rs +++ b/tests/ui/methods.rs @@ -344,6 +344,16 @@ fn or_fun_call() { /// Checks implementation of the `EXPECT_FUN_CALL` lint fn expect_fun_call() { + struct Foo; + + impl Foo { + fn new() -> Self { Foo } + + fn expect(&self, msg: &str) { + panic!("{}", msg) + } + } + let with_some = Some("value"); with_some.expect("error"); @@ -369,6 +379,15 @@ fn expect_fun_call() { let with_err_and_as_str: Result<(), ()> = Err(()); with_err_and_as_str.expect(format!("Error {}: fake error", error_code).as_str()); + + 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()); } /// Checks implementation of `ITER_NTH` lint diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr index f3c608cb7..65b5589a9 100644 --- a/tests/ui/methods.stderr +++ b/tests/ui/methods.stderr @@ -424,105 +424,105 @@ error: use of `unwrap_or` followed by a function call | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| "".to_owned())` error: use of `expect` followed by a function call - --> $DIR/methods.rs:355:26 + --> $DIR/methods.rs:365:26 | -355 | with_none_and_format.expect(&format!("Error {}: fake error", error_code)); +365 | 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 + --> $DIR/methods.rs:368:26 | -358 | with_none_and_as_str.expect(format!("Error {}: fake error", error_code).as_str()); +368 | 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 + --> $DIR/methods.rs:378:25 | -368 | with_err_and_format.expect(&format!("Error {}: fake error", error_code)); +378 | 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 + --> $DIR/methods.rs:381:25 | -371 | with_err_and_as_str.expect(format!("Error {}: fake error", error_code).as_str()); +381 | 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 + --> $DIR/methods.rs:401:23 | -382 | let bad_vec = some_vec.iter().nth(3); +401 | 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:383:26 + --> $DIR/methods.rs:402:26 | -383 | let bad_slice = &some_vec[..].iter().nth(3); +402 | 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:384:31 + --> $DIR/methods.rs:403:31 | -384 | let bad_boxed_slice = boxed_slice.iter().nth(3); +403 | 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:385:29 + --> $DIR/methods.rs:404:29 | -385 | let bad_vec_deque = some_vec_deque.iter().nth(3); +404 | 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:390:23 + --> $DIR/methods.rs:409:23 | -390 | let bad_vec = some_vec.iter_mut().nth(3); +409 | 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:393:26 + --> $DIR/methods.rs:412:26 | -393 | let bad_slice = &some_vec[..].iter_mut().nth(3); +412 | 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:396:29 + --> $DIR/methods.rs:415:29 | -396 | let bad_vec_deque = some_vec_deque.iter_mut().nth(3); +415 | 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:408:13 + --> $DIR/methods.rs:427:13 | -408 | let _ = some_vec.iter().skip(42).next(); +427 | 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:409:13 + --> $DIR/methods.rs:428:13 | -409 | let _ = some_vec.iter().cycle().skip(42).next(); +428 | 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:410:13 + --> $DIR/methods.rs:429:13 | -410 | let _ = (1..10).skip(10).next(); +429 | 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:411:14 + --> $DIR/methods.rs:430:14 | -411 | let _ = &some_vec[..].iter().skip(3).next(); +430 | 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:420:13 + --> $DIR/methods.rs:439:13 | -420 | let _ = opt.unwrap(); +439 | let _ = opt.unwrap(); | ^^^^^^^^^^^^ | = note: `-D option-unwrap-used` implied by `-D warnings` From 32404741c67a3262143e1f00f9fb31c86e684444 Mon Sep 17 00:00:00 2001 From: Donald Robertson Date: Mon, 4 Jun 2018 19:19:01 +0100 Subject: [PATCH 05/10] Replacing match block with if block as conditional was boolean --- clippy_lints/src/methods.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 38413c5aa..aad2f0908 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -1021,10 +1021,7 @@ fn lint_expect_fun_call(cx: &LateContext, expr: &hir::Expr, method_span: Span, n return; } - let closure = match match_type(cx, self_type, &paths::OPTION) { - true => "||", - false => "|_|", - }; + let closure = if match_type(cx, self_type, &paths::OPTION) { "||" } else { "|_|" }; let sugg: Cow<_> = snippet(cx, arg.span, ".."); let span_replace_word = method_span.with_hi(span.hi()); From 451fd5feb9d68c0c9a130b5d36ae73bae06c4200 Mon Sep 17 00:00:00 2001 From: Donald Robertson Date: Tue, 5 Jun 2018 21:15:08 +0100 Subject: [PATCH 06/10] Extracting arguments to format to pass directly to panic when appropriate --- clippy_lints/src/methods.rs | 40 ++++++++++++++++++++++++++++++++++++- tests/ui/methods.stderr | 4 ++-- 2 files changed, 41 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index aad2f0908..d9c0f5950 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -1022,9 +1022,47 @@ fn lint_expect_fun_call(cx: &LateContext, expr: &hir::Expr, method_span: Span, n } let closure = if match_type(cx, self_type, &paths::OPTION) { "||" } else { "|_|" }; + let span_replace_word = method_span.with_hi(span.hi()); + + if let hir::ExprAddrOf(_, ref addr_of) = arg.node { + if let hir::ExprCall(ref _inner_fun, ref inner_args) = addr_of.node { + // TODO: check if inner_fun is call to format! + if inner_args.len() == 1 { + if let hir::ExprCall(_, ref format_args) = inner_args[0].node { + let args_len = format_args.len(); + let args: Vec = format_args + .into_iter() + .take(args_len - 1) + .map(|a| { + if let hir::ExprAddrOf(_, ref format_arg) = a.node { + if let hir::ExprMatch(ref format_arg_expr, _, _) = format_arg.node { + if let hir::ExprTup(ref format_arg_expr_tup) = format_arg_expr.node { + return snippet(cx, format_arg_expr_tup[0].span, "..").into_owned(); + } + } + }; + snippet(cx, a.span, "..").into_owned() + }) + .collect(); + + 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), + ); + + return; + } + } + } + } let sugg: Cow<_> = snippet(cx, arg.span, ".."); - let span_replace_word = method_span.with_hi(span.hi()); span_lint_and_sugg( cx, diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr index 65b5589a9..edf081aaa 100644 --- a/tests/ui/methods.stderr +++ b/tests/ui/methods.stderr @@ -427,7 +427,7 @@ error: use of `expect` followed by a function call --> $DIR/methods.rs:365:26 | 365 | with_none_and_format.expect(&format!("Error {}: fake error", error_code)); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| panic!(&format!("Error {}: fake error", error_code)))` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|| panic!("Error {}: fake error", error_code))` | = note: `-D expect-fun-call` implied by `-D warnings` @@ -441,7 +441,7 @@ error: use of `expect` followed by a function call --> $DIR/methods.rs:378:25 | 378 | with_err_and_format.expect(&format!("Error {}: fake error", error_code)); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `unwrap_or_else(|_| panic!(&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/methods.rs:381:25 From e67d2b26635e7c03346a331ba52703a78a1986aa Mon Sep 17 00:00:00 2001 From: Donald Robertson Date: Wed, 6 Jun 2018 16:53:11 +0100 Subject: [PATCH 07/10] Added check to ensure format macro only being handled, refactored extraction and checks to smaller functions. --- clippy_lints/src/methods.rs | 82 +++++++++++++++++++++---------------- 1 file changed, 47 insertions(+), 35 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index d9c0f5950..7d65193b1 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -7,8 +7,8 @@ use std::fmt; use std::iter; use syntax::ast; use syntax::codemap::{Span, BytePos}; -use crate::utils::{get_arg_name, get_trait_def_id, implements_trait, in_external_macro, in_macro, is_copy, is_self, is_self_ty, - iter_input_pats, last_path_segment, match_def_path, match_path, match_qpath, match_trait_method, +use crate::utils::{get_arg_name, get_trait_def_id, implements_trait, in_external_macro, in_macro, is_copy, is_expn_of, is_self, + is_self_ty, iter_input_pats, last_path_segment, match_def_path, match_path, match_qpath, match_trait_method, match_type, method_chain_args, match_var, return_ty, remove_blocks, same_tys, single_segment_path, snippet, span_lint, span_lint_and_sugg, span_lint_and_then, span_note_and_lint, walk_ptrs_ty, walk_ptrs_ty_depth}; use crate::utils::paths; @@ -994,6 +994,34 @@ 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]) { + fn extract_format_args(arg: &hir::Expr) -> Option<&hir::HirVec> { + if let hir::ExprAddrOf(_, ref addr_of) = arg.node { + if let hir::ExprCall(ref inner_fun, ref inner_args) = addr_of.node { + if let Some(_) = is_expn_of(inner_fun.span, "format") { + if inner_args.len() == 1 { + if let hir::ExprCall(_, ref format_args) = inner_args[0].node { + return Some(format_args); + } + } + } + } + } + + None + } + + fn generate_format_arg_snippet(cx: &LateContext, a: &hir::Expr) -> String { + if let hir::ExprAddrOf(_, ref format_arg) = a.node { + if let hir::ExprMatch(ref format_arg_expr, _, _) = format_arg.node { + if let hir::ExprTup(ref format_arg_expr_tup) = format_arg_expr.node { + return snippet(cx, format_arg_expr_tup[0].span, "..").into_owned(); + } + } + }; + + snippet(cx, a.span, "..").into_owned() + } + fn check_general_case( cx: &LateContext, name: &str, @@ -1024,42 +1052,26 @@ fn lint_expect_fun_call(cx: &LateContext, expr: &hir::Expr, method_span: Span, n let closure = if match_type(cx, self_type, &paths::OPTION) { "||" } else { "|_|" }; let span_replace_word = method_span.with_hi(span.hi()); - if let hir::ExprAddrOf(_, ref addr_of) = arg.node { - if let hir::ExprCall(ref _inner_fun, ref inner_args) = addr_of.node { - // TODO: check if inner_fun is call to format! - if inner_args.len() == 1 { - if let hir::ExprCall(_, ref format_args) = inner_args[0].node { - let args_len = format_args.len(); - let args: Vec = format_args - .into_iter() - .take(args_len - 1) - .map(|a| { - if let hir::ExprAddrOf(_, ref format_arg) = a.node { - if let hir::ExprMatch(ref format_arg_expr, _, _) = format_arg.node { - if let hir::ExprTup(ref format_arg_expr_tup) = format_arg_expr.node { - return snippet(cx, format_arg_expr_tup[0].span, "..").into_owned(); - } - } - }; - snippet(cx, a.span, "..").into_owned() - }) - .collect(); + if let Some(format_args) = extract_format_args(arg) { + let args_len = format_args.len(); + let args: Vec = format_args + .into_iter() + .take(args_len - 1) + .map(|a| generate_format_arg_snippet(cx, a)) + .collect(); - let sugg = args.join(", "); + 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), - ); + 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), + ); - return; - } - } - } + return; } let sugg: Cow<_> = snippet(cx, arg.span, ".."); From 9c73f7ff18d413cb014acba7b1786044b4e00c70 Mon Sep 17 00:00:00 2001 From: Donald Robertson Date: Wed, 6 Jun 2018 17:13:31 +0100 Subject: [PATCH 08/10] Amending use of Some with discarded value to use is_some --- clippy_lints/src/methods.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 7d65193b1..9a93354af 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -997,7 +997,7 @@ fn lint_expect_fun_call(cx: &LateContext, expr: &hir::Expr, method_span: Span, n fn extract_format_args(arg: &hir::Expr) -> Option<&hir::HirVec> { if let hir::ExprAddrOf(_, ref addr_of) = arg.node { if let hir::ExprCall(ref inner_fun, ref inner_args) = addr_of.node { - if let Some(_) = is_expn_of(inner_fun.span, "format") { + if is_expn_of(inner_fun.span, "format").is_some() { if inner_args.len() == 1 { if let hir::ExprCall(_, ref format_args) = inner_args[0].node { return Some(format_args); From e70632215e8cb6272599b1d07bfc0712f8a0d70a Mon Sep 17 00:00:00 2001 From: Donald Robertson Date: Wed, 6 Jun 2018 20:38:13 +0100 Subject: [PATCH 09/10] Combining if statements per lint warnings on build --- clippy_lints/src/methods.rs | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 9a93354af..aafc612b5 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -997,11 +997,9 @@ fn lint_expect_fun_call(cx: &LateContext, expr: &hir::Expr, method_span: Span, n fn extract_format_args(arg: &hir::Expr) -> Option<&hir::HirVec> { if let hir::ExprAddrOf(_, ref addr_of) = arg.node { if let hir::ExprCall(ref inner_fun, ref inner_args) = addr_of.node { - if is_expn_of(inner_fun.span, "format").is_some() { - if inner_args.len() == 1 { - if let hir::ExprCall(_, ref format_args) = inner_args[0].node { - return Some(format_args); - } + if is_expn_of(inner_fun.span, "format").is_some() && inner_args.len() == 1 { + if let hir::ExprCall(_, ref format_args) = inner_args[0].node { + return Some(format_args); } } } From c6fb47331a3396a11d4e552f6afb176c7833c1a4 Mon Sep 17 00:00:00 2001 From: Donald Robertson Date: Mon, 11 Jun 2018 14:17:40 +0100 Subject: [PATCH 10/10] Updating docs to reflect more recent changes to expect_fun_call lint --- clippy_lints/src/methods.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index aafc612b5..d2bad6f58 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -347,7 +347,11 @@ declare_clippy_lint! { /// ``` /// this can instead be written: /// ```rust -/// foo.unwrap_or_else(|_| panic!(&format("Err {}: {}", err_code, err_msg))) +/// foo.unwrap_or_else(|_| panic!("Err {}: {}", err_code, err_msg)) +/// ``` +/// or +/// ```rust +/// foo.unwrap_or_else(|_| panic!(format("Err {}: {}", err_code, err_msg).as_str())) /// ``` declare_clippy_lint! { pub EXPECT_FUN_CALL,