diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 3708a3709..8272f8b8a 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -1022,9 +1022,13 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for Methods { ["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], 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]) @@ -2381,6 +2385,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) { @@ -2398,15 +2403,13 @@ fn lint_search_is_some<'a, 'tcx>( 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 { - match &closure_arg.pat.node { - hir::PatKind::Ref(..) => Some(search_snippet.replacen('&', "", 1)), - hir::PatKind::Binding(_, _, expr, _) => { - let closure_arg_snip = snippet(cx, expr.span, ".."); - Some(search_snippet.replace(&format!("*{}", closure_arg_snip), &closure_arg_snip)) - } - _ => None, + 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 @@ -2416,14 +2419,12 @@ fn lint_search_is_some<'a, 'tcx>( span_lint_and_sugg( cx, SEARCH_IS_SOME, - expr.span, + method_span.with_hi(expr.span.hi()), &msg, "try this", format!( "any({})", - any_search_snippet - .as_ref() - .map_or(&*search_snippet, String::as_str) + any_search_snippet.as_ref().map_or(&*search_snippet, String::as_str) ), Applicability::MachineApplicable, ); diff --git a/clippy_lints/src/utils/mod.rs b/clippy_lints/src/utils/mod.rs index 9d88e020f..2bb09d8e0 100644 --- a/clippy_lints/src/utils/mod.rs +++ b/clippy_lints/src/utils/mod.rs @@ -338,7 +338,7 @@ 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`. +/// `expr`. method/span lists are sorted with the most recent call first. pub fn method_calls(expr: &Expr, max_depth: usize) -> (Vec, Vec<&[Expr]>, Vec) { let mut method_names = Vec::with_capacity(max_depth); let mut arg_lists = Vec::with_capacity(max_depth); diff --git a/tests/ui/methods.fixed b/tests/ui/methods.fixed deleted file mode 100644 index f86da92ba..000000000 --- a/tests/ui/methods.fixed +++ /dev/null @@ -1,306 +0,0 @@ -// aux-build:option_helpers.rs -// compile-flags: --edition 2018 -// run-rustfix - -#![warn(clippy::all, clippy::pedantic, clippy::option_unwrap_used)] -#![allow( - clippy::blacklisted_name, - unused, - clippy::print_stdout, - clippy::non_ascii_literal, - clippy::new_without_default, - clippy::missing_docs_in_private_items, - clippy::needless_pass_by_value, - clippy::default_trait_access, - clippy::use_self, - clippy::useless_format, - clippy::wrong_self_convention -)] - -#[macro_use] -extern crate option_helpers; - -use std::collections::BTreeMap; -use std::collections::HashMap; -use std::collections::HashSet; -use std::collections::VecDeque; -use std::iter::FromIterator; -use std::ops::Mul; -use std::rc::{self, Rc}; -use std::sync::{self, Arc}; - -use option_helpers::IteratorFalsePositives; - -pub struct T; - -impl T { - pub fn add(self, other: T) -> T { - self - } - - // no error, not public interface - pub(crate) fn drop(&mut self) {} - - // no error, private function - fn neg(self) -> Self { - self - } - - // no error, private function - fn eq(&self, other: T) -> bool { - true - } - - // No error; self is a ref. - fn sub(&self, other: T) -> &T { - self - } - - // No error; different number of arguments. - fn div(self) -> T { - self - } - - // No error; wrong return type. - fn rem(self, other: T) {} - - // Fine - fn into_u32(self) -> u32 { - 0 - } - - fn into_u16(&self) -> u16 { - 0 - } - - fn to_something(self) -> u32 { - 0 - } - - fn new(self) -> Self { - unimplemented!(); - } -} - -struct Lt<'a> { - foo: &'a u32, -} - -impl<'a> Lt<'a> { - // The lifetime is different, but that’s irrelevant; see issue #734. - #[allow(clippy::needless_lifetimes)] - pub fn new<'b>(s: &'b str) -> Lt<'b> { - unimplemented!() - } -} - -struct Lt2<'a> { - foo: &'a u32, -} - -impl<'a> Lt2<'a> { - // The lifetime is different, but that’s irrelevant; see issue #734. - pub fn new(s: &str) -> Lt2 { - unimplemented!() - } -} - -struct Lt3<'a> { - foo: &'a u32, -} - -impl<'a> Lt3<'a> { - // The lifetime is different, but that’s irrelevant; see issue #734. - pub fn new() -> Lt3<'static> { - unimplemented!() - } -} - -#[derive(Clone, Copy)] -struct U; - -impl U { - fn new() -> Self { - U - } - // Ok because `U` is `Copy`. - fn to_something(self) -> u32 { - 0 - } -} - -struct V { - _dummy: T, -} - -impl V { - fn new() -> Option> { - None - } -} - -struct AsyncNew; - -impl AsyncNew { - async fn new() -> Option { - None - } -} - -struct BadNew; - -impl BadNew { - fn new() -> i32 { - 0 - } -} - -impl Mul for T { - type Output = T; - // No error, obviously. - fn mul(self, other: T) -> T { - self - } -} - -/// Checks implementation of the following lints: -/// * `OPTION_MAP_UNWRAP_OR` -/// * `OPTION_MAP_UNWRAP_OR_ELSE` -#[rustfmt::skip] -fn option_methods() { - let opt = Some(1); - - // Check `OPTION_MAP_UNWRAP_OR`. - // Single line case. - let _ = opt.map(|x| x + 1) - // Should lint even though this call is on a separate line. - .unwrap_or(0); - // Multi-line cases. - let _ = opt.map(|x| { - x + 1 - } - ).unwrap_or(0); - let _ = opt.map(|x| x + 1) - .unwrap_or({ - 0 - }); - // Single line `map(f).unwrap_or(None)` case. - let _ = opt.map(|x| Some(x + 1)).unwrap_or(None); - // Multi-line `map(f).unwrap_or(None)` cases. - let _ = opt.map(|x| { - Some(x + 1) - } - ).unwrap_or(None); - let _ = opt - .map(|x| Some(x + 1)) - .unwrap_or(None); - // macro case - let _ = opt_map!(opt, |x| x + 1).unwrap_or(0); // should not lint - - // Should not lint if not copyable - let id: String = "identifier".to_string(); - let _ = Some("prefix").map(|p| format!("{}.{}", p, id)).unwrap_or(id); - // ...but DO lint if the `unwrap_or` argument is not used in the `map` - let id: String = "identifier".to_string(); - let _ = Some("prefix").map(|p| format!("{}.", p)).unwrap_or(id); - - // Check OPTION_MAP_UNWRAP_OR_ELSE - // single line case - let _ = opt.map(|x| x + 1) - // Should lint even though this call is on a separate line. - .unwrap_or_else(|| 0); - // Multi-line cases. - let _ = opt.map(|x| { - x + 1 - } - ).unwrap_or_else(|| 0); - let _ = opt.map(|x| x + 1) - .unwrap_or_else(|| - 0 - ); - // Macro case. - // Should not lint. - let _ = opt_map!(opt, |x| x + 1).unwrap_or_else(|| 0); - - // Issue #4144 - { - let mut frequencies = HashMap::new(); - let word = "foo"; - - frequencies - .get_mut(word) - .map(|count| { - *count += 1; - }) - .unwrap_or_else(|| { - frequencies.insert(word.to_owned(), 1); - }); - } -} - -/// Checks implementation of `FILTER_NEXT` lint. -#[rustfmt::skip] -fn filter_next() { - let v = vec![3, 2, 1, 0, -1, -2, -3]; - - // Single-line case. - let _ = v.iter().filter(|&x| *x < 0).next(); - - // Multi-line case. - let _ = v.iter().filter(|&x| { - *x < 0 - } - ).next(); - - // Check that hat we don't lint if the caller is not an `Iterator`. - let foo = IteratorFalsePositives { foo: 0 }; - let _ = foo.filter().next(); -} - -/// Checks implementation of `SEARCH_IS_SOME` lint. -#[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 _ = any(|x| *x < 0); - let _ = any(|x| **y == x); // one dereference less - let _ = any(|x| x == 0); - - // Check `find().is_some()`, multi-line case. - let _ = v.iter().find(|&x| { - *x < 0 - } - ).is_some(); - - // Check `position().is_some()`, single-line case. - let _ = any(|&x| x < 0); - - // Check `position().is_some()`, multi-line case. - let _ = v.iter().position(|&x| { - x < 0 - } - ).is_some(); - - // Check `rposition().is_some()`, single-line case. - let _ = any(|&x| x < 0); - - // Check `rposition().is_some()`, multi-line case. - let _ = v.iter().rposition(|&x| { - x < 0 - } - ).is_some(); - - // Check that we don't lint if the caller is not an `Iterator`. - let foo = IteratorFalsePositives { foo: 0 }; - let _ = foo.find().is_some(); - let _ = foo.position().is_some(); - let _ = foo.rposition().is_some(); -} - -#[allow(clippy::similar_names)] -fn main() { - let opt = Some(0); - let _ = opt.unwrap(); -} diff --git a/tests/ui/methods.rs b/tests/ui/methods.rs index 930b76a68..8f53b8cec 100644 --- a/tests/ui/methods.rs +++ b/tests/ui/methods.rs @@ -1,6 +1,5 @@ // aux-build:option_helpers.rs // compile-flags: --edition 2018 -// run-rustfix #![warn(clippy::all, clippy::pedantic, clippy::option_unwrap_used)] #![allow( @@ -267,6 +266,7 @@ fn search_is_some() { 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 c35a74463..b30371fa5 100644 --- a/tests/ui/methods.stderr +++ b/tests/ui/methods.stderr @@ -1,5 +1,5 @@ error: defining a method called `add` on this type; consider implementing the `std::ops::Add` trait or choosing a less ambiguous name - --> $DIR/methods.rs:37:5 + --> $DIR/methods.rs:36:5 | LL | / pub fn add(self, other: T) -> T { LL | | self @@ -9,7 +9,7 @@ LL | | } = note: `-D clippy::should-implement-trait` implied by `-D warnings` error: methods called `new` usually return `Self` - --> $DIR/methods.rs:153:5 + --> $DIR/methods.rs:152:5 | LL | / fn new() -> i32 { LL | | 0 @@ -19,7 +19,7 @@ LL | | } = note: `-D clippy::new-ret-no-self` implied by `-D warnings` error: called `map(f).unwrap_or(a)` on an Option value. This can be done more directly by calling `map_or(a, f)` instead - --> $DIR/methods.rs:175:13 + --> $DIR/methods.rs:174:13 | LL | let _ = opt.map(|x| x + 1) | _____________^ @@ -31,7 +31,7 @@ LL | | .unwrap_or(0); = note: replace `map(|x| x + 1).unwrap_or(0)` with `map_or(0, |x| x + 1)` error: called `map(f).unwrap_or(a)` on an Option value. This can be done more directly by calling `map_or(a, f)` instead - --> $DIR/methods.rs:179:13 + --> $DIR/methods.rs:178:13 | LL | let _ = opt.map(|x| { | _____________^ @@ -41,7 +41,7 @@ LL | | ).unwrap_or(0); | |____________________________^ error: called `map(f).unwrap_or(a)` on an Option value. This can be done more directly by calling `map_or(a, f)` instead - --> $DIR/methods.rs:183:13 + --> $DIR/methods.rs:182:13 | LL | let _ = opt.map(|x| x + 1) | _____________^ @@ -51,7 +51,7 @@ LL | | }); | |__________________^ error: called `map(f).unwrap_or(None)` on an Option value. This can be done more directly by calling `and_then(f)` instead - --> $DIR/methods.rs:188:13 + --> $DIR/methods.rs:187:13 | LL | let _ = opt.map(|x| Some(x + 1)).unwrap_or(None); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -59,7 +59,7 @@ LL | let _ = opt.map(|x| Some(x + 1)).unwrap_or(None); = note: replace `map(|x| Some(x + 1)).unwrap_or(None)` with `and_then(|x| Some(x + 1))` error: called `map(f).unwrap_or(None)` on an Option value. This can be done more directly by calling `and_then(f)` instead - --> $DIR/methods.rs:190:13 + --> $DIR/methods.rs:189:13 | LL | let _ = opt.map(|x| { | _____________^ @@ -69,7 +69,7 @@ LL | | ).unwrap_or(None); | |_____________________^ error: called `map(f).unwrap_or(None)` on an Option value. This can be done more directly by calling `and_then(f)` instead - --> $DIR/methods.rs:194:13 + --> $DIR/methods.rs:193:13 | LL | let _ = opt | _____________^ @@ -80,7 +80,7 @@ LL | | .unwrap_or(None); = note: replace `map(|x| Some(x + 1)).unwrap_or(None)` with `and_then(|x| Some(x + 1))` error: called `map(f).unwrap_or(a)` on an Option value. This can be done more directly by calling `map_or(a, f)` instead - --> $DIR/methods.rs:205:13 + --> $DIR/methods.rs:204:13 | LL | let _ = Some("prefix").map(|p| format!("{}.", p)).unwrap_or(id); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -88,7 +88,7 @@ LL | let _ = Some("prefix").map(|p| format!("{}.", p)).unwrap_or(id); = note: replace `map(|p| format!("{}.", p)).unwrap_or(id)` with `map_or(id, |p| format!("{}.", p))` error: called `map(f).unwrap_or_else(g)` on an Option value. This can be done more directly by calling `map_or_else(g, f)` instead - --> $DIR/methods.rs:209:13 + --> $DIR/methods.rs:208:13 | LL | let _ = opt.map(|x| x + 1) | _____________^ @@ -100,7 +100,7 @@ LL | | .unwrap_or_else(|| 0); = note: replace `map(|x| x + 1).unwrap_or_else(|| 0)` with `map_or_else(|| 0, |x| x + 1)` error: called `map(f).unwrap_or_else(g)` on an Option value. This can be done more directly by calling `map_or_else(g, f)` instead - --> $DIR/methods.rs:213:13 + --> $DIR/methods.rs:212:13 | LL | let _ = opt.map(|x| { | _____________^ @@ -110,7 +110,7 @@ LL | | ).unwrap_or_else(|| 0); | |____________________________________^ error: called `map(f).unwrap_or_else(g)` on an Option value. This can be done more directly by calling `map_or_else(g, f)` instead - --> $DIR/methods.rs:217:13 + --> $DIR/methods.rs:216:13 | LL | let _ = opt.map(|x| x + 1) | _____________^ @@ -120,7 +120,7 @@ LL | | ); | |_________________^ error: called `filter(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find(p)` instead. - --> $DIR/methods.rs:247:13 + --> $DIR/methods.rs:246:13 | LL | let _ = v.iter().filter(|&x| *x < 0).next(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ @@ -129,7 +129,7 @@ LL | let _ = v.iter().filter(|&x| *x < 0).next(); = note: replace `filter(|&x| *x < 0).next()` with `find(|&x| *x < 0)` error: called `filter(p).next()` on an `Iterator`. This is more succinctly expressed by calling `.find(p)` instead. - --> $DIR/methods.rs:250:13 + --> $DIR/methods.rs:249:13 | LL | let _ = v.iter().filter(|&x| { | _____________^ @@ -139,24 +139,30 @@ LL | | ).next(); | |___________________________^ error: called `is_some()` after searching an `Iterator` with find. This is more succinctly expressed by calling `any()`. - --> $DIR/methods.rs:267:13 + --> $DIR/methods.rs:266:22 | LL | let _ = v.iter().find(|&x| *x < 0).is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| *x < 0)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| *x < 0)` | = note: `-D clippy::search-is-some` implied by `-D warnings` 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)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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:269:13 + --> $DIR/methods.rs:268:20 | LL | let _ = (0..1).find(|x| *x == 0).is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| x == 0)` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ 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 @@ -169,10 +175,10 @@ LL | | ).is_some(); | |______________________________^ error: called `is_some()` after searching an `Iterator` with position. This is more succinctly expressed by calling `any()`. - --> $DIR/methods.rs:278:13 + --> $DIR/methods.rs:278:22 | LL | let _ = v.iter().position(|&x| x < 0).is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `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:281:13 @@ -185,10 +191,10 @@ LL | | ).is_some(); | |______________________________^ error: called `is_some()` after searching an `Iterator` with rposition. This is more succinctly expressed by calling `any()`. - --> $DIR/methods.rs:287:13 + --> $DIR/methods.rs:287:22 | LL | let _ = v.iter().rposition(|&x| x < 0).is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `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:290:13 @@ -208,5 +214,5 @@ LL | let _ = opt.unwrap(); | = note: `-D clippy::option-unwrap-used` implied by `-D warnings` -error: aborting due to 23 previous errors +error: aborting due to 24 previous errors