From 945d4cf69f7a2a63f17e82c5927bad75922a6005 Mon Sep 17 00:00:00 2001 From: BO41 Date: Mon, 26 Aug 2019 12:50:15 +0200 Subject: [PATCH] Dereference one less on search_is_some and make it auto-fixable --- clippy_lints/src/methods/mod.rs | 25 ++- tests/ui/methods.fixed | 306 ++++++++++++++++++++++++++++++++ tests/ui/methods.rs | 4 + tests/ui/methods.stderr | 65 ++++--- 4 files changed, 363 insertions(+), 37 deletions(-) create mode 100644 tests/ui/methods.fixed diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index 81a8e69220c1..06126e02ed33 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/clippy_lints/src/methods/mod.rs @@ -2386,6 +2386,7 @@ 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; @@ -2393,24 +2394,32 @@ fn lint_search_is_some<'a, 'tcx>( 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)) + 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, + } } else { None } }; // add note if not multi-line - span_note_and_lint( + span_lint_and_sugg( cx, SEARCH_IS_SOME, expr.span, &msg, - expr.span, - &format!( - "replace `{0}({1}).is_some()` with `any({2})`", - search_method, - search_snippet, - any_search_snippet.as_ref().map_or(&*search_snippet, String::as_str) + "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/tests/ui/methods.fixed b/tests/ui/methods.fixed new file mode 100644 index 000000000000..f86da92bae32 --- /dev/null +++ b/tests/ui/methods.fixed @@ -0,0 +1,306 @@ +// 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 53f9f82485d1..930b76a68677 100644 --- a/tests/ui/methods.rs +++ b/tests/ui/methods.rs @@ -1,5 +1,6 @@ // aux-build:option_helpers.rs // compile-flags: --edition 2018 +// run-rustfix #![warn(clippy::all, clippy::pedantic, clippy::option_unwrap_used)] #![allow( @@ -260,9 +261,12 @@ 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(); // 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 504bb5e8253f..c35a74463ec8 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:36:5 + --> $DIR/methods.rs:37: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:152:5 + --> $DIR/methods.rs:153: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:174:13 + --> $DIR/methods.rs:175: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:178:13 + --> $DIR/methods.rs:179: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:182:13 + --> $DIR/methods.rs:183: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:187:13 + --> $DIR/methods.rs:188: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:189:13 + --> $DIR/methods.rs:190: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:193:13 + --> $DIR/methods.rs:194: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:204:13 + --> $DIR/methods.rs:205: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:208:13 + --> $DIR/methods.rs:209: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:212:13 + --> $DIR/methods.rs:213: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:216:13 + --> $DIR/methods.rs:217: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:246:13 + --> $DIR/methods.rs:247: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:249:13 + --> $DIR/methods.rs:250:13 | LL | let _ = v.iter().filter(|&x| { | _____________^ @@ -139,17 +139,28 @@ 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:267:13 | 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 | +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:269:13 + | +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:272:13 + | LL | let _ = v.iter().find(|&x| { | _____________^ LL | | *x < 0 @@ -158,15 +169,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:13 | 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 +185,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:13 | 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 +201,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 23 previous errors