diff options
| -rw-r--r-- | clippy_lints/src/methods/mod.rs | 84 | ||||
| -rw-r--r-- | clippy_lints/src/utils/internal_lints.rs | 6 | ||||
| -rw-r--r-- | clippy_lints/src/utils/mod.rs | 10 | ||||
| -rw-r--r-- | tests/ui/methods.rs | 4 | ||||
| -rw-r--r-- | tests/ui/methods.stderr | 45 | ||||
| -rw-r--r-- | tests/ui/outer_expn_data.stderr | 4 |
6 files changed, 94 insertions, 59 deletions
diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index ea208953a40..f20b5c711b1 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -1033,7 +1033,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods { return; } - let (method_names, arg_lists) = method_calls(expr, 2); + let (method_names, arg_lists, method_spans) = method_calls(expr, 2); let method_names: Vec<LocalInternedString> = method_names.iter().map(|s| s.as_str()).collect(); let method_names: Vec<&str> = method_names.iter().map(std::convert::AsRef::as_ref).collect(); @@ -1053,11 +1053,15 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods { ["map", "find"] => lint_find_map(cx, expr, arg_lists[1], arg_lists[0]), ["flat_map", "filter"] => lint_filter_flat_map(cx, expr, arg_lists[1], arg_lists[0]), ["flat_map", "filter_map"] => lint_filter_map_flat_map(cx, expr, arg_lists[1], arg_lists[0]), - ["flat_map", ..] => lint_flat_map_identity(cx, expr, arg_lists[0]), + ["flat_map", ..] => lint_flat_map_identity(cx, expr, arg_lists[0], method_spans[0]), ["flatten", "map"] => lint_map_flatten(cx, expr, arg_lists[1]), - ["is_some", "find"] => lint_search_is_some(cx, expr, "find", arg_lists[1], arg_lists[0]), - ["is_some", "position"] => lint_search_is_some(cx, expr, "position", arg_lists[1], arg_lists[0]), - ["is_some", "rposition"] => lint_search_is_some(cx, expr, "rposition", arg_lists[1], arg_lists[0]), + ["is_some", "find"] => lint_search_is_some(cx, expr, "find", arg_lists[1], arg_lists[0], method_spans[1]), + ["is_some", "position"] => { + lint_search_is_some(cx, expr, "position", arg_lists[1], arg_lists[0], method_spans[1]) + }, + ["is_some", "rposition"] => { + lint_search_is_some(cx, expr, "rposition", arg_lists[1], arg_lists[0], method_spans[1]) + }, ["extend", ..] => lint_extend(cx, expr, arg_lists[0]), ["as_ptr", "unwrap"] | ["as_ptr", "expect"] => { lint_cstring_as_ptr(cx, expr, &arg_lists[1][0], &arg_lists[0][0]) @@ -1068,7 +1072,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods { ["collect", "cloned"] => lint_iter_cloned_collect(cx, expr, arg_lists[1]), ["as_ref"] => lint_asref(cx, expr, "as_ref", arg_lists[0]), ["as_mut"] => lint_asref(cx, expr, "as_mut", arg_lists[0]), - ["fold", ..] => lint_unnecessary_fold(cx, expr, arg_lists[0]), + ["fold", ..] => lint_unnecessary_fold(cx, expr, arg_lists[0], method_spans[0]), ["filter_map", ..] => unnecessary_filter_map::lint(cx, expr, arg_lists[0]), ["count", "map"] => lint_suspicious_map(cx, expr), ["assume_init"] => lint_maybe_uninit(cx, &arg_lists[0][0], expr), @@ -1746,11 +1750,12 @@ fn lint_iter_cloned_collect<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, expr: &hir::Ex } } -fn lint_unnecessary_fold(cx: &LateContext<'_, '_>, expr: &hir::Expr, fold_args: &[hir::Expr]) { +fn lint_unnecessary_fold(cx: &LateContext<'_, '_>, expr: &hir::Expr, fold_args: &[hir::Expr], fold_span: Span) { fn check_fold_with_op( cx: &LateContext<'_, '_>, expr: &hir::Expr, fold_args: &[hir::Expr], + fold_span: Span, op: hir::BinOpKind, replacement_method_name: &str, replacement_has_args: bool, @@ -1772,8 +1777,6 @@ fn lint_unnecessary_fold(cx: &LateContext<'_, '_>, expr: &hir::Expr, fold_args: if match_var(&*left_expr, first_arg_ident); if replacement_has_args || match_var(&*right_expr, second_arg_ident); - if let hir::ExprKind::MethodCall(_, span, _) = &expr.node; - then { let mut applicability = Applicability::MachineApplicable; let sugg = if replacement_has_args { @@ -1793,7 +1796,7 @@ fn lint_unnecessary_fold(cx: &LateContext<'_, '_>, expr: &hir::Expr, fold_args: span_lint_and_sugg( cx, UNNECESSARY_FOLD, - span.with_hi(expr.span.hi()), + fold_span.with_hi(expr.span.hi()), // TODO #2371 don't suggest e.g., .any(|x| f(x)) if we can suggest .any(f) "this `.fold` can be written more succinctly using another method", "try", @@ -1817,10 +1820,18 @@ fn lint_unnecessary_fold(cx: &LateContext<'_, '_>, expr: &hir::Expr, fold_args: // Check if the first argument to .fold is a suitable literal if let hir::ExprKind::Lit(ref lit) = fold_args[1].node { match lit.node { - ast::LitKind::Bool(false) => check_fold_with_op(cx, expr, fold_args, hir::BinOpKind::Or, "any", true), - ast::LitKind::Bool(true) => check_fold_with_op(cx, expr, fold_args, hir::BinOpKind::And, "all", true), - ast::LitKind::Int(0, _) => check_fold_with_op(cx, expr, fold_args, hir::BinOpKind::Add, "sum", false), - ast::LitKind::Int(1, _) => check_fold_with_op(cx, expr, fold_args, hir::BinOpKind::Mul, "product", false), + ast::LitKind::Bool(false) => { + check_fold_with_op(cx, expr, fold_args, fold_span, hir::BinOpKind::Or, "any", true) + }, + ast::LitKind::Bool(true) => { + check_fold_with_op(cx, expr, fold_args, fold_span, hir::BinOpKind::And, "all", true) + }, + ast::LitKind::Int(0, _) => { + check_fold_with_op(cx, expr, fold_args, fold_span, hir::BinOpKind::Add, "sum", false) + }, + ast::LitKind::Int(1, _) => { + check_fold_with_op(cx, expr, fold_args, fold_span, hir::BinOpKind::Mul, "product", false) + }, _ => (), } } @@ -2357,22 +2368,21 @@ fn lint_flat_map_identity<'a, 'tcx>( cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr, flat_map_args: &'tcx [hir::Expr], + flat_map_span: Span, ) { if match_trait_method(cx, expr, &paths::ITERATOR) { let arg_node = &flat_map_args[1].node; let apply_lint = |message: &str| { - if let hir::ExprKind::MethodCall(_, span, _) = &expr.node { - span_lint_and_sugg( - cx, - FLAT_MAP_IDENTITY, - span.with_hi(expr.span.hi()), - message, - "try", - "flatten()".to_string(), - Applicability::MachineApplicable, - ); - } + span_lint_and_sugg( + cx, + FLAT_MAP_IDENTITY, + flat_map_span.with_hi(expr.span.hi()), + message, + "try", + "flatten()".to_string(), + Applicability::MachineApplicable, + ); }; if_chain! { @@ -2409,6 +2419,7 @@ fn lint_search_is_some<'a, 'tcx>( search_method: &str, search_args: &'tcx [hir::Expr], is_some_args: &'tcx [hir::Expr], + method_span: Span, ) { // lint if caller of search is an Iterator if match_trait_method(cx, &is_some_args[0], &paths::ITERATOR) { @@ -2420,31 +2431,36 @@ fn lint_search_is_some<'a, 'tcx>( let search_snippet = snippet(cx, search_args[1].span, ".."); if search_snippet.lines().count() <= 1 { // suggest `any(|x| ..)` instead of `any(|&x| ..)` for `find(|&x| ..).is_some()` + // suggest `any(|..| *..)` instead of `any(|..| **..)` for `find(|..| **..).is_some()` let any_search_snippet = if_chain! { if search_method == "find"; if let hir::ExprKind::Closure(_, _, body_id, ..) = search_args[1].node; let closure_body = cx.tcx.hir().body(body_id); if let Some(closure_arg) = closure_body.params.get(0); - if let hir::PatKind::Ref(..) = closure_arg.pat.node; then { - Some(search_snippet.replacen('&', "", 1)) + if let hir::PatKind::Ref(..) = closure_arg.pat.node { + Some(search_snippet.replacen('&', "", 1)) + } else if let Some(name) = get_arg_name(&closure_arg.pat) { + Some(search_snippet.replace(&format!("*{}", name), &name.as_str())) + } else { + None + } } else { None } }; // add note if not multi-line - span_note_and_lint( + span_lint_and_sugg( cx, SEARCH_IS_SOME, - expr.span, + method_span.with_hi(expr.span.hi()), &msg, - expr.span, - &format!( - "replace `{0}({1}).is_some()` with `any({2})`", - search_method, - search_snippet, + "try this", + format!( + "any({})", any_search_snippet.as_ref().map_or(&*search_snippet, String::as_str) ), + Applicability::MachineApplicable, ); } else { span_lint(cx, SEARCH_IS_SOME, expr.span, &msg); diff --git a/clippy_lints/src/utils/internal_lints.rs b/clippy_lints/src/utils/internal_lints.rs index 855b1e6eff3..48400593b9d 100644 --- a/clippy_lints/src/utils/internal_lints.rs +++ b/clippy_lints/src/utils/internal_lints.rs @@ -280,7 +280,7 @@ impl_lint_pass!(OuterExpnDataPass => [OUTER_EXPN_EXPN_DATA]); impl<'a, 'tcx> LateLintPass<'a, 'tcx> for OuterExpnDataPass { fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx hir::Expr) { - let (method_names, arg_lists) = method_calls(expr, 2); + let (method_names, arg_lists, spans) = method_calls(expr, 2); let method_names: Vec<LocalInternedString> = method_names.iter().map(|s| s.as_str()).collect(); let method_names: Vec<&str> = method_names.iter().map(std::convert::AsRef::as_ref).collect(); if_chain! { @@ -294,10 +294,10 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for OuterExpnDataPass { span_lint_and_sugg( cx, OUTER_EXPN_EXPN_DATA, - expr.span.trim_start(self_arg.span).unwrap_or(expr.span), + spans[1].with_hi(expr.span.hi()), "usage of `outer_expn().expn_data()`", "try", - ".outer_expn_data()".to_string(), + "outer_expn_data()".to_string(), Applicability::MachineApplicable, ); } diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 1aff61e3186..bbf9572f8fb 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -342,26 +342,28 @@ pub fn resolve_node(cx: &LateContext<'_, '_>, qpath: &QPath, id: HirId) -> Res { } /// Returns the method names and argument list of nested method call expressions that make up -/// `expr`. -pub fn method_calls(expr: &Expr, max_depth: usize) -> (Vec<Symbol>, Vec<&[Expr]>) { +/// `expr`. method/span lists are sorted with the most recent call first. +pub fn method_calls(expr: &Expr, max_depth: usize) -> (Vec<Symbol>, Vec<&[Expr]>, Vec<Span>) { let mut method_names = Vec::with_capacity(max_depth); let mut arg_lists = Vec::with_capacity(max_depth); + let mut spans = Vec::with_capacity(max_depth); let mut current = expr; for _ in 0..max_depth { - if let ExprKind::MethodCall(path, _, args) = ¤t.node { + if let ExprKind::MethodCall(path, span, args) = ¤t.node { if args.iter().any(|e| e.span.from_expansion()) { break; } method_names.push(path.ident.name); arg_lists.push(&**args); + spans.push(*span); current = &args[0]; } else { break; } } - (method_names, arg_lists) + (method_names, arg_lists, spans) } /// Matches an `Expr` against a chain of methods, and return the matched `Expr`s. diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs index 53f9f82485d..8f53b8cecbd 100644 --- a/tests/ui/methods.rs +++ b/tests/ui/methods.rs @@ -260,9 +260,13 @@ fn filter_next() { #[rustfmt::skip] fn search_is_some() { let v = vec![3, 2, 1, 0, -1, -2, -3]; + let y = &&42; // Check `find().is_some()`, single-line case. let _ = v.iter().find(|&x| *x < 0).is_some(); + let _ = (0..1).find(|x| **y == *x).is_some(); // one dereference less + let _ = (0..1).find(|x| *x == 0).is_some(); + let _ = v.iter().find(|x| **x == 0).is_some(); // Check `find().is_some()`, multi-line case. let _ = v.iter().find(|&x| { diff --git a/tests/ui/methods.stderr b/tests/ui/methods.stderr index 504bb5e8253..b30371fa541 100644 --- a/tests/ui/methods.stderr +++ b/tests/ui/methods.stderr @@ -139,16 +139,33 @@ LL | | ).next(); | |___________________________^ error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`. - --> $DIR/methods.rs:265:13 + --> $DIR/methods.rs:266:22 | LL | let _ = v.iter().find(|&x| *x < 0).is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| *x < 0)` | = note: `-D clippy::search-is-some` implied by `-D warnings` - = note: replace `find(|&x| *x < 0).is_some()` with `any(|x| *x < 0)` error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`. - --> $DIR/methods.rs:268:13 + --> $DIR/methods.rs:267:20 + | +LL | let _ = (0..1).find(|x| **y == *x).is_some(); // one dereference less + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| **y == x)` + +error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`. + --> $DIR/methods.rs:268:20 + | +LL | let _ = (0..1).find(|x| *x == 0).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| x == 0)` + +error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`. + --> $DIR/methods.rs:269:22 + | +LL | let _ = v.iter().find(|x| **x == 0).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| *x == 0)` + +error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`. + --> $DIR/methods.rs:272:13 | LL | let _ = v.iter().find(|&x| { | _____________^ @@ -158,15 +175,13 @@ LL | | ).is_some(); | |______________________________^ error: called `is_some()` after searching an `Iterator` with position. This is more succinctly expressed by calling `any()`. - --> $DIR/methods.rs:274:13 + --> $DIR/methods.rs:278:22 | LL | let _ = v.iter().position(|&x| x < 0).is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = note: replace `position(|&x| x < 0).is_some()` with `any(|&x| x < 0)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|&x| x < 0)` error: called `is_some()` after searching an `Iterator` with position. This is more succinctly expressed by calling `any()`. - --> $DIR/methods.rs:277:13 + --> $DIR/methods.rs:281:13 | LL | let _ = v.iter().position(|&x| { | _____________^ @@ -176,15 +191,13 @@ LL | | ).is_some(); | |______________________________^ error: called `is_some()` after searching an `Iterator` with rposition. This is more succinctly expressed by calling `any()`. - --> $DIR/methods.rs:283:13 + --> $DIR/methods.rs:287:22 | LL | let _ = v.iter().rposition(|&x| x < 0).is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | - = note: replace `rposition(|&x| x < 0).is_some()` with `any(|&x| x < 0)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|&x| x < 0)` error: called `is_some()` after searching an `Iterator` with rposition. This is more succinctly expressed by calling `any()`. - --> $DIR/methods.rs:286:13 + --> $DIR/methods.rs:290:13 | LL | let _ = v.iter().rposition(|&x| { | _____________^ @@ -194,12 +207,12 @@ LL | | ).is_some(); | |______________________________^ 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:301:13 + --> $DIR/methods.rs:305:13 | LL | let _ = opt.unwrap(); | ^^^^^^^^^^^^ | = note: `-D clippy::option-unwrap-used` implied by `-D warnings` -error: aborting due to 21 previous errors +error: aborting due to 24 previous errors diff --git a/tests/ui/outer_expn_data.stderr b/tests/ui/outer_expn_data.stderr index 6803b3feb8e..15fb0ad2ede 100644 --- a/tests/ui/outer_expn_data.stderr +++ b/tests/ui/outer_expn_data.stderr @@ -1,8 +1,8 @@ error: usage of `outer_expn().expn_data()` - --> $DIR/outer_expn_data.rs:21:33 + --> $DIR/outer_expn_data.rs:21:34 | LL | let _ = expr.span.ctxt().outer_expn().expn_data(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `.outer_expn_data()` + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `outer_expn_data()` | note: lint level defined here --> $DIR/outer_expn_data.rs:3:9 |