From f4c75cde5e5226912995f6809e96efd2ac2c2348 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Tue, 13 Jul 2021 11:26:24 +0200 Subject: [PATCH 01/25] Fix `any()` not taking reference in `search_is_some` lint --- clippy_lints/src/methods/search_is_some.rs | 22 ++++++++++++-- tests/ui/search_is_some_fixable.fixed | 35 ++++++++++++++++++++++ tests/ui/search_is_some_fixable.rs | 35 ++++++++++++++++++++++ tests/ui/search_is_some_fixable.stderr | 14 ++++++++- 4 files changed, 103 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/methods/search_is_some.rs b/clippy_lints/src/methods/search_is_some.rs index 0f2e58d8983f..42a0a09f3854 100644 --- a/clippy_lints/src/methods/search_is_some.rs +++ b/clippy_lints/src/methods/search_is_some.rs @@ -45,9 +45,27 @@ pub(super) fn check<'tcx>( then { if let hir::PatKind::Ref(..) = closure_arg.pat.kind { Some(search_snippet.replacen('&', "", 1)) - } else if let PatKind::Binding(_, _, ident, _) = strip_pat_refs(closure_arg.pat).kind { + } else if let PatKind::Binding(annotation, _, ident, _) = strip_pat_refs(closure_arg.pat).kind { let name = &*ident.name.as_str(); - Some(search_snippet.replace(&format!("*{}", name), name)) + let old_search_snippet = search_snippet.clone(); + let search_snippet = search_snippet.replace(&format!("*{}", name), name); + + if_chain! { + // if there is no dereferencing used in closure body + if old_search_snippet == search_snippet; + if annotation == hir::BindingAnnotation::Unannotated; + if let ty::Ref(_, inner_ty, _) = cx.typeck_results().node_type(closure_arg.hir_id).kind(); + if let ty::Ref(..) = inner_ty.kind(); + // put an `&` in the closure body, but skip closure params + if let Some((start, end)) = old_search_snippet.split_once(&name); + + then { + let end = end.replace(name, &format!("&{}", name)); + Some(format!("{}{}{}", start, name, end)) + } else { + Some(search_snippet) + } + } } else { None } diff --git a/tests/ui/search_is_some_fixable.fixed b/tests/ui/search_is_some_fixable.fixed index 62ff16f67f41..029f557ffa62 100644 --- a/tests/ui/search_is_some_fixable.fixed +++ b/tests/ui/search_is_some_fixable.fixed @@ -66,3 +66,38 @@ fn is_none() { let _ = !s1[2..].contains(&s2); let _ = !s1[2..].contains(&s2[2..]); } + +#[allow(clippy::clone_on_copy, clippy::map_clone)] +mod issue7392 { + struct Player { + hand: Vec, + } + fn filter() { + let p = Player { + hand: vec![1, 2, 3, 4, 5], + }; + let filter_hand = vec![5]; + let _ = p + .hand + .iter() + .filter(|c| !filter_hand.iter().any(|cc| c == &cc)) + .map(|c| c.clone()) + .collect::>(); + } + + struct PlayerTuple { + hand: Vec<(usize, char)>, + } + fn filter_tuple() { + let p = PlayerTuple { + hand: vec![(1, 'a'), (2, 'b'), (3, 'c'), (4, 'd'), (5, 'e')], + }; + let filter_hand = vec![5]; + let _ = p + .hand + .iter() + .filter(|(c, _)| !filter_hand.iter().any(|cc| c == cc)) + .map(|c| c.clone()) + .collect::>(); + } +} diff --git a/tests/ui/search_is_some_fixable.rs b/tests/ui/search_is_some_fixable.rs index 8407f7166474..b8f8fe3d3c19 100644 --- a/tests/ui/search_is_some_fixable.rs +++ b/tests/ui/search_is_some_fixable.rs @@ -66,3 +66,38 @@ fn is_none() { let _ = s1[2..].find(&s2).is_none(); let _ = s1[2..].find(&s2[2..]).is_none(); } + +#[allow(clippy::clone_on_copy, clippy::map_clone)] +mod issue7392 { + struct Player { + hand: Vec, + } + fn filter() { + let p = Player { + hand: vec![1, 2, 3, 4, 5], + }; + let filter_hand = vec![5]; + let _ = p + .hand + .iter() + .filter(|c| filter_hand.iter().find(|cc| c == cc).is_none()) + .map(|c| c.clone()) + .collect::>(); + } + + struct PlayerTuple { + hand: Vec<(usize, char)>, + } + fn filter_tuple() { + let p = PlayerTuple { + hand: vec![(1, 'a'), (2, 'b'), (3, 'c'), (4, 'd'), (5, 'e')], + }; + let filter_hand = vec![5]; + let _ = p + .hand + .iter() + .filter(|(c, _)| filter_hand.iter().find(|cc| c == *cc).is_none()) + .map(|c| c.clone()) + .collect::>(); + } +} diff --git a/tests/ui/search_is_some_fixable.stderr b/tests/ui/search_is_some_fixable.stderr index bd1b6955a972..0d92722229c3 100644 --- a/tests/ui/search_is_some_fixable.stderr +++ b/tests/ui/search_is_some_fixable.stderr @@ -180,5 +180,17 @@ error: called `is_none()` after calling `find()` on a string LL | let _ = s1[2..].find(&s2[2..]).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!s1[2..].contains(&s2[2..])` -error: aborting due to 30 previous errors +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable.rs:83:25 + | +LL | .filter(|c| filter_hand.iter().find(|cc| c == cc).is_none()) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!filter_hand.iter().any(|cc| c == &cc)` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable.rs:99:30 + | +LL | .filter(|(c, _)| filter_hand.iter().find(|cc| c == *cc).is_none()) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!filter_hand.iter().any(|cc| c == cc)` + +error: aborting due to 32 previous errors From 5ed93af9c41e552c8d85390e2387d01eb9429174 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Mon, 9 Aug 2021 12:00:35 +0200 Subject: [PATCH 02/25] Use `ExprUseVisitor` and multipart suggestion to avoid iffy `String` replacement --- clippy_lints/src/methods/search_is_some.rs | 186 ++++++++++++++++----- tests/ui/search_is_some.rs | 45 +++++ tests/ui/search_is_some.stderr | 128 +++++++++++++- tests/ui/search_is_some_fixable.fixed | 43 ----- tests/ui/search_is_some_fixable.rs | 43 ----- tests/ui/search_is_some_fixable.stderr | 98 +++-------- 6 files changed, 334 insertions(+), 209 deletions(-) diff --git a/clippy_lints/src/methods/search_is_some.rs b/clippy_lints/src/methods/search_is_some.rs index 42a0a09f3854..71fca601747d 100644 --- a/clippy_lints/src/methods/search_is_some.rs +++ b/clippy_lints/src/methods/search_is_some.rs @@ -1,15 +1,19 @@ -use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg}; +use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg, span_lint_and_then}; use clippy_utils::source::{snippet, snippet_with_applicability}; use clippy_utils::ty::is_type_diagnostic_item; use clippy_utils::{is_trait_method, strip_pat_refs}; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir as hir; -use rustc_hir::PatKind; +use rustc_hir::{self, HirId, HirIdMap, HirIdSet, PatKind}; +use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::LateContext; +use rustc_middle::hir::place::ProjectionKind; +use rustc_middle::mir::FakeReadCause; use rustc_middle::ty; use rustc_span::source_map::Span; use rustc_span::symbol::sym; +use rustc_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId}; use super::SEARCH_IS_SOME; @@ -42,30 +46,42 @@ pub(super) fn check<'tcx>( if let hir::ExprKind::Closure(_, _, body_id, ..) = search_arg.kind; let closure_body = cx.tcx.hir().body(body_id); if let Some(closure_arg) = closure_body.params.get(0); + then { if let hir::PatKind::Ref(..) = closure_arg.pat.kind { - Some(search_snippet.replacen('&', "", 1)) - } else if let PatKind::Binding(annotation, _, ident, _) = strip_pat_refs(closure_arg.pat).kind { - let name = &*ident.name.as_str(); - let old_search_snippet = search_snippet.clone(); - let search_snippet = search_snippet.replace(&format!("*{}", name), name); - - if_chain! { - // if there is no dereferencing used in closure body - if old_search_snippet == search_snippet; - if annotation == hir::BindingAnnotation::Unannotated; - if let ty::Ref(_, inner_ty, _) = cx.typeck_results().node_type(closure_arg.hir_id).kind(); - if let ty::Ref(..) = inner_ty.kind(); - // put an `&` in the closure body, but skip closure params - if let Some((start, end)) = old_search_snippet.split_once(&name); - - then { - let end = end.replace(name, &format!("&{}", name)); - Some(format!("{}{}{}", start, name, end)) - } else { - Some(search_snippet) + Some((search_snippet.replacen('&', "", 1), None)) + } else if let PatKind::Binding(..) = strip_pat_refs(closure_arg.pat).kind { + let mut visitor = DerefDelegate { + cx, + set: HirIdSet::default(), + deref_suggs: HirIdMap::default(), + borrow_suggs: HirIdMap::default() + }; + + let fn_def_id = cx.tcx.hir().local_def_id(search_arg.hir_id); + cx.tcx.infer_ctxt().enter(|infcx| { + ExprUseVisitor::new( + &mut visitor, &infcx, fn_def_id, cx.param_env, cx.typeck_results() + ).consume_body(closure_body); + }); + + let replacements = if visitor.set.is_empty() { + None + } else { + let mut deref_suggs = Vec::new(); + let mut borrow_suggs = Vec::new(); + for node in visitor.set { + let span = cx.tcx.hir().span(node); + if let Some(sugg) = visitor.deref_suggs.get(&node) { + deref_suggs.push((span, sugg.clone())); + } + if let Some(sugg) = visitor.borrow_suggs.get(&node) { + borrow_suggs.push((span, sugg.clone())); + } } - } + Some((deref_suggs, borrow_suggs)) + }; + Some((search_snippet.to_string(), replacements)) } else { None } @@ -74,35 +90,38 @@ pub(super) fn check<'tcx>( } }; // add note if not multi-line - if is_some { - span_lint_and_sugg( - cx, - SEARCH_IS_SOME, + let (closure_snippet, replacements) = any_search_snippet + .as_ref() + .map_or((&*search_snippet, None), |s| (&s.0, s.1.clone())); + let (span, help, sugg) = if is_some { + ( method_span.with_hi(expr.span.hi()), - &msg, "use `any()` instead", - format!( - "any({})", - any_search_snippet.as_ref().map_or(&*search_snippet, String::as_str) - ), - Applicability::MachineApplicable, - ); + format!("any({})", closure_snippet), + ) } else { let iter = snippet(cx, search_recv.span, ".."); - span_lint_and_sugg( - cx, - SEARCH_IS_SOME, + ( expr.span, - &msg, "use `!_.any()` instead", - format!( - "!{}.any({})", - iter, - any_search_snippet.as_ref().map_or(&*search_snippet, String::as_str) - ), - Applicability::MachineApplicable, - ); - } + format!("!{}.any({})", iter, closure_snippet), + ) + }; + + span_lint_and_then(cx, SEARCH_IS_SOME, span, &msg, |db| { + if let Some((deref_suggs, borrow_suggs)) = replacements { + db.span_suggestion(span, help, sugg, Applicability::MaybeIncorrect); + + if !deref_suggs.is_empty() { + db.multipart_suggestion("...and remove deref", deref_suggs, Applicability::MaybeIncorrect); + } + if !borrow_suggs.is_empty() { + db.multipart_suggestion("...and borrow variable", borrow_suggs, Applicability::MaybeIncorrect); + } + } else { + db.span_suggestion(span, help, sugg, Applicability::MachineApplicable); + } + }); } else { let hint = format!( "this is more succinctly expressed by calling `any()`{}", @@ -164,3 +183,78 @@ pub(super) fn check<'tcx>( } } } + +struct DerefDelegate<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + set: HirIdSet, + deref_suggs: HirIdMap, + borrow_suggs: HirIdMap, +} + +impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { + fn consume(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId) { + if let PlaceBase::Local(id) = cmt.place.base { + let map = self.cx.tcx.hir(); + if cmt.place.projections.is_empty() { + self.set.insert(cmt.hir_id); + } else { + let mut replacement_str = map.name(id).to_string(); + let last_deref = cmt + .place + .projections + .iter() + .rposition(|proj| proj.kind == ProjectionKind::Deref); + + if let Some(pos) = last_deref { + let mut projections = cmt.place.projections.clone(); + projections.truncate(pos); + + for item in projections { + if item.kind == ProjectionKind::Deref { + replacement_str = format!("*{}", replacement_str); + } + } + + self.set.insert(cmt.hir_id); + self.deref_suggs.insert(cmt.hir_id, replacement_str); + } + } + } + } + + fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) { + if let PlaceBase::Local(id) = cmt.place.base { + let map = self.cx.tcx.hir(); + if cmt.place.projections.is_empty() { + let replacement_str = format!("&{}", map.name(id).to_string()); + self.set.insert(cmt.hir_id); + self.borrow_suggs.insert(cmt.hir_id, replacement_str); + } else { + let mut replacement_str = map.name(id).to_string(); + let last_deref = cmt + .place + .projections + .iter() + .rposition(|proj| proj.kind == ProjectionKind::Deref); + + if let Some(pos) = last_deref { + let mut projections = cmt.place.projections.clone(); + projections.truncate(pos); + + for item in projections { + if item.kind == ProjectionKind::Deref { + replacement_str = format!("*{}", replacement_str); + } + } + + self.set.insert(cmt.hir_id); + self.deref_suggs.insert(cmt.hir_id, replacement_str); + } + } + } + } + + fn mutate(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {} + + fn fake_read(&mut self, _: rustc_typeck::expr_use_visitor::Place<'tcx>, _: FakeReadCause, _: HirId) {} +} diff --git a/tests/ui/search_is_some.rs b/tests/ui/search_is_some.rs index 72bc6ef35d31..dfc79207ca77 100644 --- a/tests/ui/search_is_some.rs +++ b/tests/ui/search_is_some.rs @@ -36,6 +36,11 @@ fn main() { // check that we don't lint if `find()` is called with // `Pattern` that is not a string let _ = "hello world".find(|c: char| c == 'o' || c == 'l').is_some(); + + // Check `find().is_some()`, single-line case. + 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(); } #[rustfmt::skip] @@ -70,4 +75,44 @@ fn is_none() { // check that we don't lint if `find()` is called with // `Pattern` that is not a string let _ = "hello world".find(|c: char| c == 'o' || c == 'l').is_none(); + + // Check `find().is_none()`, single-line case. + let _ = (0..1).find(|x| **y == *x).is_none(); // one dereference less + let _ = (0..1).find(|x| *x == 0).is_none(); + let _ = v.iter().find(|x| **x == 0).is_none(); +} + +#[allow(clippy::clone_on_copy, clippy::map_clone)] +mod issue7392 { + struct Player { + hand: Vec, + } + fn filter() { + let p = Player { + hand: vec![1, 2, 3, 4, 5], + }; + let filter_hand = vec![5]; + let _ = p + .hand + .iter() + .filter(|c| filter_hand.iter().find(|cc| c == cc).is_none()) + .map(|c| c.clone()) + .collect::>(); + } + + struct PlayerTuple { + hand: Vec<(usize, char)>, + } + fn filter_tuple() { + let p = PlayerTuple { + hand: vec![(1, 'a'), (2, 'b'), (3, 'c'), (4, 'd'), (5, 'e')], + }; + let filter_hand = vec![5]; + let _ = p + .hand + .iter() + .filter(|(c, _)| filter_hand.iter().find(|cc| c == *cc).is_none()) + .map(|c| c.clone()) + .collect::>(); + } } diff --git a/tests/ui/search_is_some.stderr b/tests/ui/search_is_some.stderr index f3c758e451ef..8db936fd9486 100644 --- a/tests/ui/search_is_some.stderr +++ b/tests/ui/search_is_some.stderr @@ -35,8 +35,53 @@ LL | | ).is_some(); | = help: this is more succinctly expressed by calling `any()` +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some.rs:41:20 + | +LL | let _ = (0..1).find(|x| **y == *x).is_some(); // one dereference less + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: use `any()` instead + | +LL | let _ = (0..1).any(|x| **y == *x); // one dereference less + | ^^^^^^^^^^^^^^^^^^ +help: ...and remove deref + | +LL | let _ = (0..1).find(|x| **y == x).is_some(); // one dereference less + | ^ + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some.rs:42:20 + | +LL | let _ = (0..1).find(|x| *x == 0).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: use `any()` instead + | +LL | let _ = (0..1).any(|x| *x == 0); + | ^^^^^^^^^^^^^^^^ +help: ...and remove deref + | +LL | let _ = (0..1).find(|x| x == 0).is_some(); + | ^ + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some.rs:43:22 + | +LL | let _ = v.iter().find(|x| **x == 0).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: use `any()` instead + | +LL | let _ = v.iter().any(|x| **x == 0); + | ^^^^^^^^^^^^^^^^^ +help: ...and remove deref + | +LL | let _ = v.iter().find(|x| *x == 0).is_some(); + | ^^ + error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some.rs:48:13 + --> $DIR/search_is_some.rs:53:13 | LL | let _ = v.iter().find(|&x| { | _____________^ @@ -48,7 +93,7 @@ LL | | ).is_none(); = help: this is more succinctly expressed by calling `any()` with negation error: called `is_none()` after searching an `Iterator` with `position` - --> $DIR/search_is_some.rs:54:13 + --> $DIR/search_is_some.rs:59:13 | LL | let _ = v.iter().position(|&x| { | _____________^ @@ -60,7 +105,7 @@ LL | | ).is_none(); = help: this is more succinctly expressed by calling `any()` with negation error: called `is_none()` after searching an `Iterator` with `rposition` - --> $DIR/search_is_some.rs:60:13 + --> $DIR/search_is_some.rs:65:13 | LL | let _ = v.iter().rposition(|&x| { | _____________^ @@ -71,5 +116,80 @@ LL | | ).is_none(); | = help: this is more succinctly expressed by calling `any()` with negation -error: aborting due to 6 previous errors +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some.rs:80:13 + | +LL | let _ = (0..1).find(|x| **y == *x).is_none(); // one dereference less + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: use `!_.any()` instead + | +LL | let _ = !(0..1).any(|x| **y == *x); // one dereference less + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: ...and remove deref + | +LL | let _ = (0..1).find(|x| **y == x).is_none(); // one dereference less + | ^ + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some.rs:81:13 + | +LL | let _ = (0..1).find(|x| *x == 0).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: use `!_.any()` instead + | +LL | let _ = !(0..1).any(|x| *x == 0); + | ^^^^^^^^^^^^^^^^^^^^^^^^ +help: ...and remove deref + | +LL | let _ = (0..1).find(|x| x == 0).is_none(); + | ^ + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some.rs:82:13 + | +LL | let _ = v.iter().find(|x| **x == 0).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: use `!_.any()` instead + | +LL | let _ = !v.iter().any(|x| **x == 0); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: ...and remove deref + | +LL | let _ = v.iter().find(|x| *x == 0).is_none(); + | ^^ + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some.rs:98:25 + | +LL | .filter(|c| filter_hand.iter().find(|cc| c == cc).is_none()) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: use `!_.any()` instead + | +LL | .filter(|c| !filter_hand.iter().any(|cc| c == cc)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: ...and borrow variable + | +LL | .filter(|c| filter_hand.iter().find(|cc| c == &cc).is_none()) + | ^^^ + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some.rs:114:30 + | +LL | .filter(|(c, _)| filter_hand.iter().find(|cc| c == *cc).is_none()) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: use `!_.any()` instead + | +LL | .filter(|(c, _)| !filter_hand.iter().any(|cc| c == *cc)) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +help: ...and remove deref + | +LL | .filter(|(c, _)| filter_hand.iter().find(|cc| c == cc).is_none()) + | ^^ + +error: aborting due to 14 previous errors diff --git a/tests/ui/search_is_some_fixable.fixed b/tests/ui/search_is_some_fixable.fixed index 029f557ffa62..6994df427ba9 100644 --- a/tests/ui/search_is_some_fixable.fixed +++ b/tests/ui/search_is_some_fixable.fixed @@ -4,13 +4,9 @@ fn main() { let v = vec![3, 2, 1, 0, -1, -2, -3]; - let y = &&42; // Check `find().is_some()`, single-line case. let _ = v.iter().any(|x| *x < 0); - let _ = (0..1).any(|x| **y == x); // one dereference less - let _ = (0..1).any(|x| x == 0); - let _ = v.iter().any(|x| *x == 0); // Check `position().is_some()`, single-line case. let _ = v.iter().any(|&x| x < 0); @@ -36,13 +32,9 @@ fn main() { fn is_none() { let v = vec![3, 2, 1, 0, -1, -2, -3]; - let y = &&42; // Check `find().is_none()`, single-line case. let _ = !v.iter().any(|x| *x < 0); - let _ = !(0..1).any(|x| **y == x); // one dereference less - let _ = !(0..1).any(|x| x == 0); - let _ = !v.iter().any(|x| *x == 0); // Check `position().is_none()`, single-line case. let _ = !v.iter().any(|&x| x < 0); @@ -66,38 +58,3 @@ fn is_none() { let _ = !s1[2..].contains(&s2); let _ = !s1[2..].contains(&s2[2..]); } - -#[allow(clippy::clone_on_copy, clippy::map_clone)] -mod issue7392 { - struct Player { - hand: Vec, - } - fn filter() { - let p = Player { - hand: vec![1, 2, 3, 4, 5], - }; - let filter_hand = vec![5]; - let _ = p - .hand - .iter() - .filter(|c| !filter_hand.iter().any(|cc| c == &cc)) - .map(|c| c.clone()) - .collect::>(); - } - - struct PlayerTuple { - hand: Vec<(usize, char)>, - } - fn filter_tuple() { - let p = PlayerTuple { - hand: vec![(1, 'a'), (2, 'b'), (3, 'c'), (4, 'd'), (5, 'e')], - }; - let filter_hand = vec![5]; - let _ = p - .hand - .iter() - .filter(|(c, _)| !filter_hand.iter().any(|cc| c == cc)) - .map(|c| c.clone()) - .collect::>(); - } -} diff --git a/tests/ui/search_is_some_fixable.rs b/tests/ui/search_is_some_fixable.rs index b8f8fe3d3c19..d4c9d89e7b83 100644 --- a/tests/ui/search_is_some_fixable.rs +++ b/tests/ui/search_is_some_fixable.rs @@ -4,13 +4,9 @@ fn main() { 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 `position().is_some()`, single-line case. let _ = v.iter().position(|&x| x < 0).is_some(); @@ -36,13 +32,9 @@ fn main() { fn is_none() { let v = vec![3, 2, 1, 0, -1, -2, -3]; - let y = &&42; // Check `find().is_none()`, single-line case. let _ = v.iter().find(|&x| *x < 0).is_none(); - let _ = (0..1).find(|x| **y == *x).is_none(); // one dereference less - let _ = (0..1).find(|x| *x == 0).is_none(); - let _ = v.iter().find(|x| **x == 0).is_none(); // Check `position().is_none()`, single-line case. let _ = v.iter().position(|&x| x < 0).is_none(); @@ -66,38 +58,3 @@ fn is_none() { let _ = s1[2..].find(&s2).is_none(); let _ = s1[2..].find(&s2[2..]).is_none(); } - -#[allow(clippy::clone_on_copy, clippy::map_clone)] -mod issue7392 { - struct Player { - hand: Vec, - } - fn filter() { - let p = Player { - hand: vec![1, 2, 3, 4, 5], - }; - let filter_hand = vec![5]; - let _ = p - .hand - .iter() - .filter(|c| filter_hand.iter().find(|cc| c == cc).is_none()) - .map(|c| c.clone()) - .collect::>(); - } - - struct PlayerTuple { - hand: Vec<(usize, char)>, - } - fn filter_tuple() { - let p = PlayerTuple { - hand: vec![(1, 'a'), (2, 'b'), (3, 'c'), (4, 'd'), (5, 'e')], - }; - let filter_hand = vec![5]; - let _ = p - .hand - .iter() - .filter(|(c, _)| filter_hand.iter().find(|cc| c == *cc).is_none()) - .map(|c| c.clone()) - .collect::>(); - } -} diff --git a/tests/ui/search_is_some_fixable.stderr b/tests/ui/search_is_some_fixable.stderr index 0d92722229c3..5e77c9a5bace 100644 --- a/tests/ui/search_is_some_fixable.stderr +++ b/tests/ui/search_is_some_fixable.stderr @@ -1,196 +1,148 @@ error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:10:22 + --> $DIR/search_is_some_fixable.rs:9:22 | LL | let _ = v.iter().find(|&x| *x < 0).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| *x < 0)` | = note: `-D clippy::search-is-some` implied by `-D warnings` -error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:11:20 - | -LL | let _ = (0..1).find(|x| **y == *x).is_some(); // one dereference less - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| **y == x)` - -error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:12:20 - | -LL | let _ = (0..1).find(|x| *x == 0).is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| x == 0)` - -error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:13:22 - | -LL | let _ = v.iter().find(|x| **x == 0).is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| *x == 0)` - error: called `is_some()` after searching an `Iterator` with `position` - --> $DIR/search_is_some_fixable.rs:16:22 + --> $DIR/search_is_some_fixable.rs:12:22 | LL | let _ = v.iter().position(|&x| x < 0).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|&x| x < 0)` error: called `is_some()` after searching an `Iterator` with `rposition` - --> $DIR/search_is_some_fixable.rs:19:22 + --> $DIR/search_is_some_fixable.rs:15:22 | LL | let _ = v.iter().rposition(|&x| x < 0).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|&x| x < 0)` error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:24:27 + --> $DIR/search_is_some_fixable.rs:20:27 | LL | let _ = "hello world".find("world").is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains("world")` error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:25:27 + --> $DIR/search_is_some_fixable.rs:21:27 | LL | let _ = "hello world".find(&s2).is_some(); | ^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains(&s2)` error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:26:27 + --> $DIR/search_is_some_fixable.rs:22:27 | LL | let _ = "hello world".find(&s2[2..]).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains(&s2[2..])` error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:28:16 + --> $DIR/search_is_some_fixable.rs:24:16 | LL | let _ = s1.find("world").is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains("world")` error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:29:16 + --> $DIR/search_is_some_fixable.rs:25:16 | LL | let _ = s1.find(&s2).is_some(); | ^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains(&s2)` error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:30:16 + --> $DIR/search_is_some_fixable.rs:26:16 | LL | let _ = s1.find(&s2[2..]).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains(&s2[2..])` error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:32:21 + --> $DIR/search_is_some_fixable.rs:28:21 | LL | let _ = s1[2..].find("world").is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains("world")` error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:33:21 + --> $DIR/search_is_some_fixable.rs:29:21 | LL | let _ = s1[2..].find(&s2).is_some(); | ^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains(&s2)` error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:34:21 + --> $DIR/search_is_some_fixable.rs:30:21 | LL | let _ = s1[2..].find(&s2[2..]).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains(&s2[2..])` error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:42:13 + --> $DIR/search_is_some_fixable.rs:37:13 | LL | let _ = v.iter().find(|&x| *x < 0).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|x| *x < 0)` -error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:43:13 - | -LL | let _ = (0..1).find(|x| **y == *x).is_none(); // one dereference less - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!(0..1).any(|x| **y == x)` - -error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:44:13 - | -LL | let _ = (0..1).find(|x| *x == 0).is_none(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!(0..1).any(|x| x == 0)` - -error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:45:13 - | -LL | let _ = v.iter().find(|x| **x == 0).is_none(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|x| *x == 0)` - error: called `is_none()` after searching an `Iterator` with `position` - --> $DIR/search_is_some_fixable.rs:48:13 + --> $DIR/search_is_some_fixable.rs:40:13 | LL | let _ = v.iter().position(|&x| x < 0).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|&x| x < 0)` error: called `is_none()` after searching an `Iterator` with `rposition` - --> $DIR/search_is_some_fixable.rs:51:13 + --> $DIR/search_is_some_fixable.rs:43:13 | LL | let _ = v.iter().rposition(|&x| x < 0).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|&x| x < 0)` error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:57:13 + --> $DIR/search_is_some_fixable.rs:49:13 | LL | let _ = "hello world".find("world").is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!"hello world".contains("world")` error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:58:13 + --> $DIR/search_is_some_fixable.rs:50:13 | LL | let _ = "hello world".find(&s2).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!"hello world".contains(&s2)` error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:59:13 + --> $DIR/search_is_some_fixable.rs:51:13 | LL | let _ = "hello world".find(&s2[2..]).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!"hello world".contains(&s2[2..])` error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:61:13 + --> $DIR/search_is_some_fixable.rs:53:13 | LL | let _ = s1.find("world").is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!s1.contains("world")` error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:62:13 + --> $DIR/search_is_some_fixable.rs:54:13 | LL | let _ = s1.find(&s2).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!s1.contains(&s2)` error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:63:13 + --> $DIR/search_is_some_fixable.rs:55:13 | LL | let _ = s1.find(&s2[2..]).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!s1.contains(&s2[2..])` error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:65:13 + --> $DIR/search_is_some_fixable.rs:57:13 | LL | let _ = s1[2..].find("world").is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!s1[2..].contains("world")` error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:66:13 + --> $DIR/search_is_some_fixable.rs:58:13 | LL | let _ = s1[2..].find(&s2).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!s1[2..].contains(&s2)` error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:67:13 + --> $DIR/search_is_some_fixable.rs:59:13 | LL | let _ = s1[2..].find(&s2[2..]).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!s1[2..].contains(&s2[2..])` -error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:83:25 - | -LL | .filter(|c| filter_hand.iter().find(|cc| c == cc).is_none()) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!filter_hand.iter().any(|cc| c == &cc)` - -error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:99:30 - | -LL | .filter(|(c, _)| filter_hand.iter().find(|cc| c == *cc).is_none()) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!filter_hand.iter().any(|cc| c == cc)` - -error: aborting due to 32 previous errors +error: aborting due to 24 previous errors From b38f173aa329d6a5932414b9c76227d790faee76 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Wed, 18 Aug 2021 16:51:34 +0200 Subject: [PATCH 03/25] Build suggestion within visitor + add more tests --- clippy_lints/src/methods/search_is_some.rs | 202 +++++++++++---------- tests/ui/search_is_some.rs | 45 ----- tests/ui/search_is_some.stderr | 128 +------------ tests/ui/search_is_some_fixable.fixed | 54 ++++++ tests/ui/search_is_some_fixable.rs | 57 ++++++ tests/ui/search_is_some_fixable.stderr | 163 ++++++++++++++--- 6 files changed, 355 insertions(+), 294 deletions(-) diff --git a/clippy_lints/src/methods/search_is_some.rs b/clippy_lints/src/methods/search_is_some.rs index 71fca601747d..ee0e6d3a33f4 100644 --- a/clippy_lints/src/methods/search_is_some.rs +++ b/clippy_lints/src/methods/search_is_some.rs @@ -1,17 +1,17 @@ -use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg, span_lint_and_then}; +use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg}; use clippy_utils::source::{snippet, snippet_with_applicability}; use clippy_utils::ty::is_type_diagnostic_item; -use clippy_utils::{is_trait_method, strip_pat_refs}; +use clippy_utils::{get_parent_expr_for_hir, is_trait_method, strip_pat_refs}; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir as hir; -use rustc_hir::{self, HirId, HirIdMap, HirIdSet, PatKind}; +use rustc_hir::{self, Expr, ExprKind, HirId, PatKind}; use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::LateContext; use rustc_middle::hir::place::ProjectionKind; -use rustc_middle::mir::FakeReadCause; +use rustc_middle::mir::{FakeReadCause, Mutability}; use rustc_middle::ty; -use rustc_span::source_map::Span; +use rustc_span::source_map::{BytePos, Span}; use rustc_span::symbol::sym; use rustc_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId}; @@ -46,42 +46,12 @@ pub(super) fn check<'tcx>( if let hir::ExprKind::Closure(_, _, body_id, ..) = search_arg.kind; let closure_body = cx.tcx.hir().body(body_id); if let Some(closure_arg) = closure_body.params.get(0); - then { if let hir::PatKind::Ref(..) = closure_arg.pat.kind { - Some((search_snippet.replacen('&', "", 1), None)) + Some(search_snippet.replacen('&', "", 1)) } else if let PatKind::Binding(..) = strip_pat_refs(closure_arg.pat).kind { - let mut visitor = DerefDelegate { - cx, - set: HirIdSet::default(), - deref_suggs: HirIdMap::default(), - borrow_suggs: HirIdMap::default() - }; - - let fn_def_id = cx.tcx.hir().local_def_id(search_arg.hir_id); - cx.tcx.infer_ctxt().enter(|infcx| { - ExprUseVisitor::new( - &mut visitor, &infcx, fn_def_id, cx.param_env, cx.typeck_results() - ).consume_body(closure_body); - }); - - let replacements = if visitor.set.is_empty() { - None - } else { - let mut deref_suggs = Vec::new(); - let mut borrow_suggs = Vec::new(); - for node in visitor.set { - let span = cx.tcx.hir().span(node); - if let Some(sugg) = visitor.deref_suggs.get(&node) { - deref_suggs.push((span, sugg.clone())); - } - if let Some(sugg) = visitor.borrow_suggs.get(&node) { - borrow_suggs.push((span, sugg.clone())); - } - } - Some((deref_suggs, borrow_suggs)) - }; - Some((search_snippet.to_string(), replacements)) + get_closure_suggestion(cx, search_arg, closure_body) + .or_else(|| Some(search_snippet.to_string())) } else { None } @@ -90,38 +60,35 @@ pub(super) fn check<'tcx>( } }; // add note if not multi-line - let (closure_snippet, replacements) = any_search_snippet - .as_ref() - .map_or((&*search_snippet, None), |s| (&s.0, s.1.clone())); - let (span, help, sugg) = if is_some { - ( + if is_some { + span_lint_and_sugg( + cx, + SEARCH_IS_SOME, method_span.with_hi(expr.span.hi()), + &msg, "use `any()` instead", - format!("any({})", closure_snippet), - ) + format!( + "any({})", + any_search_snippet.as_ref().map_or(&*search_snippet, String::as_str) + ), + Applicability::MachineApplicable, + ); } else { let iter = snippet(cx, search_recv.span, ".."); - ( + span_lint_and_sugg( + cx, + SEARCH_IS_SOME, expr.span, + &msg, "use `!_.any()` instead", - format!("!{}.any({})", iter, closure_snippet), - ) - }; - - span_lint_and_then(cx, SEARCH_IS_SOME, span, &msg, |db| { - if let Some((deref_suggs, borrow_suggs)) = replacements { - db.span_suggestion(span, help, sugg, Applicability::MaybeIncorrect); - - if !deref_suggs.is_empty() { - db.multipart_suggestion("...and remove deref", deref_suggs, Applicability::MaybeIncorrect); - } - if !borrow_suggs.is_empty() { - db.multipart_suggestion("...and borrow variable", borrow_suggs, Applicability::MaybeIncorrect); - } - } else { - db.span_suggestion(span, help, sugg, Applicability::MachineApplicable); - } - }); + format!( + "!{}.any({})", + iter, + any_search_snippet.as_ref().map_or(&*search_snippet, String::as_str) + ), + Applicability::MachineApplicable, + ); + } } else { let hint = format!( "this is more succinctly expressed by calling `any()`{}", @@ -184,53 +151,86 @@ pub(super) fn check<'tcx>( } } +fn get_closure_suggestion<'tcx>( + cx: &LateContext<'_>, + search_arg: &'tcx hir::Expr<'_>, + closure_body: &hir::Body<'_>, +) -> Option { + let mut visitor = DerefDelegate { + cx, + closure_span: search_arg.span, + next_pos: None, + suggestion_start: String::new(), + suggestion_end: String::new(), + }; + + let fn_def_id = cx.tcx.hir().local_def_id(search_arg.hir_id); + cx.tcx.infer_ctxt().enter(|infcx| { + ExprUseVisitor::new(&mut visitor, &infcx, fn_def_id, cx.param_env, cx.typeck_results()) + .consume_body(closure_body); + }); + + if visitor.suggestion_start.is_empty() { + None + } else { + Some(format!("{}{}", visitor.suggestion_start, visitor.suggestion_end)) + } +} + struct DerefDelegate<'a, 'tcx> { cx: &'a LateContext<'tcx>, - set: HirIdSet, - deref_suggs: HirIdMap, - borrow_suggs: HirIdMap, + closure_span: Span, + next_pos: Option, + suggestion_start: String, + suggestion_end: String, } impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { - fn consume(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId) { + fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {} + + fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) { if let PlaceBase::Local(id) = cmt.place.base { let map = self.cx.tcx.hir(); - if cmt.place.projections.is_empty() { - self.set.insert(cmt.hir_id); + let ident_str = map.name(id).to_string(); + let span = map.span(cmt.hir_id); + let start_span = if let Some(next_pos) = self.next_pos { + Span::new(next_pos, span.lo(), span.ctxt()) } else { - let mut replacement_str = map.name(id).to_string(); - let last_deref = cmt - .place - .projections - .iter() - .rposition(|proj| proj.kind == ProjectionKind::Deref); + self.closure_span.until(span) + }; + let start_snip = snippet(self.cx, start_span, ".."); + let end_span = Span::new(span.hi(), self.closure_span.hi(), span.ctxt()); + let end_snip = snippet(self.cx, end_span, ".."); - if let Some(pos) = last_deref { - let mut projections = cmt.place.projections.clone(); - projections.truncate(pos); + if cmt.place.projections.is_empty() { + self.suggestion_start.push_str(&format!("{}&{}", start_snip, ident_str)); + } else { + let parent_expr = get_parent_expr_for_hir(self.cx, cmt.hir_id); + if let Some(Expr { hir_id: _, kind, .. }) = parent_expr { + if let ExprKind::Call(_, args) | ExprKind::MethodCall(_, _, args, _) = kind { + let args_to_handle = args.iter().filter(|arg| arg.hir_id == cmt.hir_id).collect::>(); + if !args_to_handle.is_empty() { + for arg in &args_to_handle { + let arg_ty_kind = self.cx.typeck_results().expr_ty(arg).kind(); + if matches!(arg_ty_kind, ty::Ref(_, _, Mutability::Not)) { + let start_span = if let Some(next_pos) = self.next_pos { + Span::new(next_pos, span.lo(), span.ctxt()) + } else { + self.closure_span.until(span) + }; + let start_snip = snippet(self.cx, start_span, ".."); - for item in projections { - if item.kind == ProjectionKind::Deref { - replacement_str = format!("*{}", replacement_str); + self.suggestion_start.push_str(&format!("{}&{}", start_snip, ident_str)); + self.suggestion_end = end_snip.to_string(); + self.next_pos = Some(span.hi()); + } + } + return; } } - - self.set.insert(cmt.hir_id); - self.deref_suggs.insert(cmt.hir_id, replacement_str); } - } - } - } - fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) { - if let PlaceBase::Local(id) = cmt.place.base { - let map = self.cx.tcx.hir(); - if cmt.place.projections.is_empty() { - let replacement_str = format!("&{}", map.name(id).to_string()); - self.set.insert(cmt.hir_id); - self.borrow_suggs.insert(cmt.hir_id, replacement_str); - } else { - let mut replacement_str = map.name(id).to_string(); + let mut replacement_str = ident_str; let last_deref = cmt .place .projections @@ -246,11 +246,13 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { replacement_str = format!("*{}", replacement_str); } } - - self.set.insert(cmt.hir_id); - self.deref_suggs.insert(cmt.hir_id, replacement_str); } + + self.suggestion_start + .push_str(&format!("{}{}", start_snip, replacement_str)); + self.suggestion_end = end_snip.to_string(); } + self.next_pos = Some(span.hi()); } } diff --git a/tests/ui/search_is_some.rs b/tests/ui/search_is_some.rs index dfc79207ca77..72bc6ef35d31 100644 --- a/tests/ui/search_is_some.rs +++ b/tests/ui/search_is_some.rs @@ -36,11 +36,6 @@ fn main() { // check that we don't lint if `find()` is called with // `Pattern` that is not a string let _ = "hello world".find(|c: char| c == 'o' || c == 'l').is_some(); - - // Check `find().is_some()`, single-line case. - 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(); } #[rustfmt::skip] @@ -75,44 +70,4 @@ fn is_none() { // check that we don't lint if `find()` is called with // `Pattern` that is not a string let _ = "hello world".find(|c: char| c == 'o' || c == 'l').is_none(); - - // Check `find().is_none()`, single-line case. - let _ = (0..1).find(|x| **y == *x).is_none(); // one dereference less - let _ = (0..1).find(|x| *x == 0).is_none(); - let _ = v.iter().find(|x| **x == 0).is_none(); -} - -#[allow(clippy::clone_on_copy, clippy::map_clone)] -mod issue7392 { - struct Player { - hand: Vec, - } - fn filter() { - let p = Player { - hand: vec![1, 2, 3, 4, 5], - }; - let filter_hand = vec![5]; - let _ = p - .hand - .iter() - .filter(|c| filter_hand.iter().find(|cc| c == cc).is_none()) - .map(|c| c.clone()) - .collect::>(); - } - - struct PlayerTuple { - hand: Vec<(usize, char)>, - } - fn filter_tuple() { - let p = PlayerTuple { - hand: vec![(1, 'a'), (2, 'b'), (3, 'c'), (4, 'd'), (5, 'e')], - }; - let filter_hand = vec![5]; - let _ = p - .hand - .iter() - .filter(|(c, _)| filter_hand.iter().find(|cc| c == *cc).is_none()) - .map(|c| c.clone()) - .collect::>(); - } } diff --git a/tests/ui/search_is_some.stderr b/tests/ui/search_is_some.stderr index 8db936fd9486..f3c758e451ef 100644 --- a/tests/ui/search_is_some.stderr +++ b/tests/ui/search_is_some.stderr @@ -35,53 +35,8 @@ LL | | ).is_some(); | = help: this is more succinctly expressed by calling `any()` -error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some.rs:41:20 - | -LL | let _ = (0..1).find(|x| **y == *x).is_some(); // one dereference less - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | -help: use `any()` instead - | -LL | let _ = (0..1).any(|x| **y == *x); // one dereference less - | ^^^^^^^^^^^^^^^^^^ -help: ...and remove deref - | -LL | let _ = (0..1).find(|x| **y == x).is_some(); // one dereference less - | ^ - -error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some.rs:42:20 - | -LL | let _ = (0..1).find(|x| *x == 0).is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | -help: use `any()` instead - | -LL | let _ = (0..1).any(|x| *x == 0); - | ^^^^^^^^^^^^^^^^ -help: ...and remove deref - | -LL | let _ = (0..1).find(|x| x == 0).is_some(); - | ^ - -error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some.rs:43:22 - | -LL | let _ = v.iter().find(|x| **x == 0).is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | -help: use `any()` instead - | -LL | let _ = v.iter().any(|x| **x == 0); - | ^^^^^^^^^^^^^^^^^ -help: ...and remove deref - | -LL | let _ = v.iter().find(|x| *x == 0).is_some(); - | ^^ - error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some.rs:53:13 + --> $DIR/search_is_some.rs:48:13 | LL | let _ = v.iter().find(|&x| { | _____________^ @@ -93,7 +48,7 @@ LL | | ).is_none(); = help: this is more succinctly expressed by calling `any()` with negation error: called `is_none()` after searching an `Iterator` with `position` - --> $DIR/search_is_some.rs:59:13 + --> $DIR/search_is_some.rs:54:13 | LL | let _ = v.iter().position(|&x| { | _____________^ @@ -105,7 +60,7 @@ LL | | ).is_none(); = help: this is more succinctly expressed by calling `any()` with negation error: called `is_none()` after searching an `Iterator` with `rposition` - --> $DIR/search_is_some.rs:65:13 + --> $DIR/search_is_some.rs:60:13 | LL | let _ = v.iter().rposition(|&x| { | _____________^ @@ -116,80 +71,5 @@ LL | | ).is_none(); | = help: this is more succinctly expressed by calling `any()` with negation -error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some.rs:80:13 - | -LL | let _ = (0..1).find(|x| **y == *x).is_none(); // one dereference less - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | -help: use `!_.any()` instead - | -LL | let _ = !(0..1).any(|x| **y == *x); // one dereference less - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ -help: ...and remove deref - | -LL | let _ = (0..1).find(|x| **y == x).is_none(); // one dereference less - | ^ - -error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some.rs:81:13 - | -LL | let _ = (0..1).find(|x| *x == 0).is_none(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | -help: use `!_.any()` instead - | -LL | let _ = !(0..1).any(|x| *x == 0); - | ^^^^^^^^^^^^^^^^^^^^^^^^ -help: ...and remove deref - | -LL | let _ = (0..1).find(|x| x == 0).is_none(); - | ^ - -error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some.rs:82:13 - | -LL | let _ = v.iter().find(|x| **x == 0).is_none(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | -help: use `!_.any()` instead - | -LL | let _ = !v.iter().any(|x| **x == 0); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ -help: ...and remove deref - | -LL | let _ = v.iter().find(|x| *x == 0).is_none(); - | ^^ - -error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some.rs:98:25 - | -LL | .filter(|c| filter_hand.iter().find(|cc| c == cc).is_none()) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | -help: use `!_.any()` instead - | -LL | .filter(|c| !filter_hand.iter().any(|cc| c == cc)) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -help: ...and borrow variable - | -LL | .filter(|c| filter_hand.iter().find(|cc| c == &cc).is_none()) - | ^^^ - -error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some.rs:114:30 - | -LL | .filter(|(c, _)| filter_hand.iter().find(|cc| c == *cc).is_none()) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - | -help: use `!_.any()` instead - | -LL | .filter(|(c, _)| !filter_hand.iter().any(|cc| c == *cc)) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -help: ...and remove deref - | -LL | .filter(|(c, _)| filter_hand.iter().find(|cc| c == cc).is_none()) - | ^^ - -error: aborting due to 14 previous errors +error: aborting due to 6 previous errors diff --git a/tests/ui/search_is_some_fixable.fixed b/tests/ui/search_is_some_fixable.fixed index 6994df427ba9..967b3d1be432 100644 --- a/tests/ui/search_is_some_fixable.fixed +++ b/tests/ui/search_is_some_fixable.fixed @@ -4,9 +4,19 @@ fn main() { let v = vec![3, 2, 1, 0, -1, -2, -3]; + let y = &&42; // Check `find().is_some()`, single-line case. let _ = v.iter().any(|x| *x < 0); + let _ = (0..1).any(|x| **y == x); // one dereference less + let _ = (0..1).any(|x| x == 0); + let _ = v.iter().any(|x| *x == 0); + let _ = (4..5).any(|x| x == 1 || x == 3 || x == 5); + let _ = (1..3).any(|x| [1, 2, 3].contains(&x)); + let _ = (1..3).any(|x| x == 0 || [1, 2, 3].contains(&x)); + let _ = (1..3).any(|x| [1, 2, 3].contains(&x) || x == 0); + let _ = (1..3) + .any(|x| [1, 2, 3].contains(&x) || x == 0 || [4, 5, 6].contains(&x) || x == -1); // Check `position().is_some()`, single-line case. let _ = v.iter().any(|&x| x < 0); @@ -32,9 +42,18 @@ fn main() { fn is_none() { let v = vec![3, 2, 1, 0, -1, -2, -3]; + let y = &&42; // Check `find().is_none()`, single-line case. let _ = !v.iter().any(|x| *x < 0); + let _ = !(0..1).any(|x| **y == x); // one dereference less + let _ = !(0..1).any(|x| x == 0); + let _ = !v.iter().any(|x| *x == 0); + let _ = !(4..5).any(|x| x == 1 || x == 3 || x == 5); + let _ = !(1..3).any(|x| [1, 2, 3].contains(&x)); + let _ = !(1..3).any(|x| x == 0 || [1, 2, 3].contains(&x)); + let _ = !(1..3).any(|x| [1, 2, 3].contains(&x) || x == 0); + let _ = !(1..3).any(|x| [1, 2, 3].contains(&x) || x == 0 || [4, 5, 6].contains(&x) || x == -1); // Check `position().is_none()`, single-line case. let _ = !v.iter().any(|&x| x < 0); @@ -58,3 +77,38 @@ fn is_none() { let _ = !s1[2..].contains(&s2); let _ = !s1[2..].contains(&s2[2..]); } + +#[allow(clippy::clone_on_copy, clippy::map_clone)] +mod issue7392 { + struct Player { + hand: Vec, + } + fn filter() { + let p = Player { + hand: vec![1, 2, 3, 4, 5], + }; + let filter_hand = vec![5]; + let _ = p + .hand + .iter() + .filter(|c| !filter_hand.iter().any(|cc| c == &cc)) + .map(|c| c.clone()) + .collect::>(); + } + + struct PlayerTuple { + hand: Vec<(usize, char)>, + } + fn filter_tuple() { + let p = PlayerTuple { + hand: vec![(1, 'a'), (2, 'b'), (3, 'c'), (4, 'd'), (5, 'e')], + }; + let filter_hand = vec![5]; + let _ = p + .hand + .iter() + .filter(|(c, _)| !filter_hand.iter().any(|cc| c == cc)) + .map(|c| c.clone()) + .collect::>(); + } +} diff --git a/tests/ui/search_is_some_fixable.rs b/tests/ui/search_is_some_fixable.rs index d4c9d89e7b83..e642523a5320 100644 --- a/tests/ui/search_is_some_fixable.rs +++ b/tests/ui/search_is_some_fixable.rs @@ -4,9 +4,20 @@ fn main() { 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(); + let _ = (4..5).find(|x| *x == 1 || *x == 3 || *x == 5).is_some(); + let _ = (1..3).find(|x| [1, 2, 3].contains(x)).is_some(); + let _ = (1..3).find(|x| *x == 0 || [1, 2, 3].contains(x)).is_some(); + let _ = (1..3).find(|x| [1, 2, 3].contains(x) || *x == 0).is_some(); + let _ = (1..3) + .find(|x| [1, 2, 3].contains(x) || *x == 0 || [4, 5, 6].contains(x) || *x == -1) + .is_some(); // Check `position().is_some()`, single-line case. let _ = v.iter().position(|&x| x < 0).is_some(); @@ -32,9 +43,20 @@ fn main() { fn is_none() { let v = vec![3, 2, 1, 0, -1, -2, -3]; + let y = &&42; // Check `find().is_none()`, single-line case. let _ = v.iter().find(|&x| *x < 0).is_none(); + let _ = (0..1).find(|x| **y == *x).is_none(); // one dereference less + let _ = (0..1).find(|x| *x == 0).is_none(); + let _ = v.iter().find(|x| **x == 0).is_none(); + let _ = (4..5).find(|x| *x == 1 || *x == 3 || *x == 5).is_none(); + let _ = (1..3).find(|x| [1, 2, 3].contains(x)).is_none(); + let _ = (1..3).find(|x| *x == 0 || [1, 2, 3].contains(x)).is_none(); + let _ = (1..3).find(|x| [1, 2, 3].contains(x) || *x == 0).is_none(); + let _ = (1..3) + .find(|x| [1, 2, 3].contains(x) || *x == 0 || [4, 5, 6].contains(x) || *x == -1) + .is_none(); // Check `position().is_none()`, single-line case. let _ = v.iter().position(|&x| x < 0).is_none(); @@ -58,3 +80,38 @@ fn is_none() { let _ = s1[2..].find(&s2).is_none(); let _ = s1[2..].find(&s2[2..]).is_none(); } + +#[allow(clippy::clone_on_copy, clippy::map_clone)] +mod issue7392 { + struct Player { + hand: Vec, + } + fn filter() { + let p = Player { + hand: vec![1, 2, 3, 4, 5], + }; + let filter_hand = vec![5]; + let _ = p + .hand + .iter() + .filter(|c| filter_hand.iter().find(|cc| c == cc).is_none()) + .map(|c| c.clone()) + .collect::>(); + } + + struct PlayerTuple { + hand: Vec<(usize, char)>, + } + fn filter_tuple() { + let p = PlayerTuple { + hand: vec![(1, 'a'), (2, 'b'), (3, 'c'), (4, 'd'), (5, 'e')], + }; + let filter_hand = vec![5]; + let _ = p + .hand + .iter() + .filter(|(c, _)| filter_hand.iter().find(|cc| c == *cc).is_none()) + .map(|c| c.clone()) + .collect::>(); + } +} diff --git a/tests/ui/search_is_some_fixable.stderr b/tests/ui/search_is_some_fixable.stderr index 5e77c9a5bace..3bb8cc573ec5 100644 --- a/tests/ui/search_is_some_fixable.stderr +++ b/tests/ui/search_is_some_fixable.stderr @@ -1,148 +1,261 @@ error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:9:22 + --> $DIR/search_is_some_fixable.rs:10:22 | LL | let _ = v.iter().find(|&x| *x < 0).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| *x < 0)` | = note: `-D clippy::search-is-some` implied by `-D warnings` +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable.rs:11:20 + | +LL | let _ = (0..1).find(|x| **y == *x).is_some(); // one dereference less + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| **y == x)` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable.rs:12:20 + | +LL | let _ = (0..1).find(|x| *x == 0).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| x == 0)` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable.rs:13:22 + | +LL | let _ = v.iter().find(|x| **x == 0).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| *x == 0)` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable.rs:14:20 + | +LL | let _ = (4..5).find(|x| *x == 1 || *x == 3 || *x == 5).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| x == 1 || x == 3 || x == 5)` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable.rs:15:20 + | +LL | let _ = (1..3).find(|x| [1, 2, 3].contains(x)).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| [1, 2, 3].contains(&x))` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable.rs:16:20 + | +LL | let _ = (1..3).find(|x| *x == 0 || [1, 2, 3].contains(x)).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| x == 0 || [1, 2, 3].contains(&x))` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable.rs:17:20 + | +LL | let _ = (1..3).find(|x| [1, 2, 3].contains(x) || *x == 0).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| [1, 2, 3].contains(&x) || x == 0)` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable.rs:19:10 + | +LL | .find(|x| [1, 2, 3].contains(x) || *x == 0 || [4, 5, 6].contains(x) || *x == -1) + | __________^ +LL | | .is_some(); + | |__________________^ help: use `any()` instead: `any(|x| [1, 2, 3].contains(&x) || x == 0 || [4, 5, 6].contains(&x) || x == -1)` + error: called `is_some()` after searching an `Iterator` with `position` - --> $DIR/search_is_some_fixable.rs:12:22 + --> $DIR/search_is_some_fixable.rs:23:22 | LL | let _ = v.iter().position(|&x| x < 0).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|&x| x < 0)` error: called `is_some()` after searching an `Iterator` with `rposition` - --> $DIR/search_is_some_fixable.rs:15:22 + --> $DIR/search_is_some_fixable.rs:26:22 | LL | let _ = v.iter().rposition(|&x| x < 0).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|&x| x < 0)` error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:20:27 + --> $DIR/search_is_some_fixable.rs:31:27 | LL | let _ = "hello world".find("world").is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains("world")` error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:21:27 + --> $DIR/search_is_some_fixable.rs:32:27 | LL | let _ = "hello world".find(&s2).is_some(); | ^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains(&s2)` error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:22:27 + --> $DIR/search_is_some_fixable.rs:33:27 | LL | let _ = "hello world".find(&s2[2..]).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains(&s2[2..])` error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:24:16 + --> $DIR/search_is_some_fixable.rs:35:16 | LL | let _ = s1.find("world").is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains("world")` error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:25:16 + --> $DIR/search_is_some_fixable.rs:36:16 | LL | let _ = s1.find(&s2).is_some(); | ^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains(&s2)` error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:26:16 + --> $DIR/search_is_some_fixable.rs:37:16 | LL | let _ = s1.find(&s2[2..]).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains(&s2[2..])` error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:28:21 + --> $DIR/search_is_some_fixable.rs:39:21 | LL | let _ = s1[2..].find("world").is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains("world")` error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:29:21 + --> $DIR/search_is_some_fixable.rs:40:21 | LL | let _ = s1[2..].find(&s2).is_some(); | ^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains(&s2)` error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:30:21 + --> $DIR/search_is_some_fixable.rs:41:21 | LL | let _ = s1[2..].find(&s2[2..]).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains(&s2[2..])` error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:37:13 + --> $DIR/search_is_some_fixable.rs:49:13 | LL | let _ = v.iter().find(|&x| *x < 0).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|x| *x < 0)` +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable.rs:50:13 + | +LL | let _ = (0..1).find(|x| **y == *x).is_none(); // one dereference less + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!(0..1).any(|x| **y == x)` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable.rs:51:13 + | +LL | let _ = (0..1).find(|x| *x == 0).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!(0..1).any(|x| x == 0)` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable.rs:52:13 + | +LL | let _ = v.iter().find(|x| **x == 0).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|x| *x == 0)` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable.rs:53:13 + | +LL | let _ = (4..5).find(|x| *x == 1 || *x == 3 || *x == 5).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!(4..5).any(|x| x == 1 || x == 3 || x == 5)` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable.rs:54:13 + | +LL | let _ = (1..3).find(|x| [1, 2, 3].contains(x)).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!(1..3).any(|x| [1, 2, 3].contains(&x))` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable.rs:55:13 + | +LL | let _ = (1..3).find(|x| *x == 0 || [1, 2, 3].contains(x)).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!(1..3).any(|x| x == 0 || [1, 2, 3].contains(&x))` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable.rs:56:13 + | +LL | let _ = (1..3).find(|x| [1, 2, 3].contains(x) || *x == 0).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!(1..3).any(|x| [1, 2, 3].contains(&x) || x == 0)` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable.rs:57:13 + | +LL | let _ = (1..3) + | _____________^ +LL | | .find(|x| [1, 2, 3].contains(x) || *x == 0 || [4, 5, 6].contains(x) || *x == -1) +LL | | .is_none(); + | |__________________^ help: use `!_.any()` instead: `!(1..3).any(|x| [1, 2, 3].contains(&x) || x == 0 || [4, 5, 6].contains(&x) || x == -1)` + error: called `is_none()` after searching an `Iterator` with `position` - --> $DIR/search_is_some_fixable.rs:40:13 + --> $DIR/search_is_some_fixable.rs:62:13 | LL | let _ = v.iter().position(|&x| x < 0).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|&x| x < 0)` error: called `is_none()` after searching an `Iterator` with `rposition` - --> $DIR/search_is_some_fixable.rs:43:13 + --> $DIR/search_is_some_fixable.rs:65:13 | LL | let _ = v.iter().rposition(|&x| x < 0).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|&x| x < 0)` error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:49:13 + --> $DIR/search_is_some_fixable.rs:71:13 | LL | let _ = "hello world".find("world").is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!"hello world".contains("world")` error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:50:13 + --> $DIR/search_is_some_fixable.rs:72:13 | LL | let _ = "hello world".find(&s2).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!"hello world".contains(&s2)` error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:51:13 + --> $DIR/search_is_some_fixable.rs:73:13 | LL | let _ = "hello world".find(&s2[2..]).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!"hello world".contains(&s2[2..])` error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:53:13 + --> $DIR/search_is_some_fixable.rs:75:13 | LL | let _ = s1.find("world").is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!s1.contains("world")` error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:54:13 + --> $DIR/search_is_some_fixable.rs:76:13 | LL | let _ = s1.find(&s2).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!s1.contains(&s2)` error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:55:13 + --> $DIR/search_is_some_fixable.rs:77:13 | LL | let _ = s1.find(&s2[2..]).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!s1.contains(&s2[2..])` error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:57:13 + --> $DIR/search_is_some_fixable.rs:79:13 | LL | let _ = s1[2..].find("world").is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!s1[2..].contains("world")` error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:58:13 + --> $DIR/search_is_some_fixable.rs:80:13 | LL | let _ = s1[2..].find(&s2).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!s1[2..].contains(&s2)` error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:59:13 + --> $DIR/search_is_some_fixable.rs:81:13 | LL | let _ = s1[2..].find(&s2[2..]).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!s1[2..].contains(&s2[2..])` -error: aborting due to 24 previous errors +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable.rs:97:25 + | +LL | .filter(|c| filter_hand.iter().find(|cc| c == cc).is_none()) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!filter_hand.iter().any(|cc| c == &cc)` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable.rs:113:30 + | +LL | .filter(|(c, _)| filter_hand.iter().find(|cc| c == *cc).is_none()) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!filter_hand.iter().any(|cc| c == cc)` + +error: aborting due to 42 previous errors From ddcbac37ae7c8e5f9487454f800423e651e56868 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Thu, 19 Aug 2021 10:38:40 +0200 Subject: [PATCH 04/25] Split tests (too long for CI) --- tests/ui/search_is_some_fixable.stderr | 261 ------------------ ...ixed => search_is_some_fixable_none.fixed} | 38 --- ...able.rs => search_is_some_fixable_none.rs} | 39 --- tests/ui/search_is_some_fixable_none.stderr | 139 ++++++++++ tests/ui/search_is_some_fixable_some.fixed | 76 +++++ tests/ui/search_is_some_fixable_some.rs | 77 ++++++ tests/ui/search_is_some_fixable_some.stderr | 138 +++++++++ 7 files changed, 430 insertions(+), 338 deletions(-) delete mode 100644 tests/ui/search_is_some_fixable.stderr rename tests/ui/{search_is_some_fixable.fixed => search_is_some_fixable_none.fixed} (62%) rename tests/ui/{search_is_some_fixable.rs => search_is_some_fixable_none.rs} (61%) create mode 100644 tests/ui/search_is_some_fixable_none.stderr create mode 100644 tests/ui/search_is_some_fixable_some.fixed create mode 100644 tests/ui/search_is_some_fixable_some.rs create mode 100644 tests/ui/search_is_some_fixable_some.stderr diff --git a/tests/ui/search_is_some_fixable.stderr b/tests/ui/search_is_some_fixable.stderr deleted file mode 100644 index 3bb8cc573ec5..000000000000 --- a/tests/ui/search_is_some_fixable.stderr +++ /dev/null @@ -1,261 +0,0 @@ -error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:10:22 - | -LL | let _ = v.iter().find(|&x| *x < 0).is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| *x < 0)` - | - = note: `-D clippy::search-is-some` implied by `-D warnings` - -error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:11:20 - | -LL | let _ = (0..1).find(|x| **y == *x).is_some(); // one dereference less - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| **y == x)` - -error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:12:20 - | -LL | let _ = (0..1).find(|x| *x == 0).is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| x == 0)` - -error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:13:22 - | -LL | let _ = v.iter().find(|x| **x == 0).is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| *x == 0)` - -error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:14:20 - | -LL | let _ = (4..5).find(|x| *x == 1 || *x == 3 || *x == 5).is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| x == 1 || x == 3 || x == 5)` - -error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:15:20 - | -LL | let _ = (1..3).find(|x| [1, 2, 3].contains(x)).is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| [1, 2, 3].contains(&x))` - -error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:16:20 - | -LL | let _ = (1..3).find(|x| *x == 0 || [1, 2, 3].contains(x)).is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| x == 0 || [1, 2, 3].contains(&x))` - -error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:17:20 - | -LL | let _ = (1..3).find(|x| [1, 2, 3].contains(x) || *x == 0).is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| [1, 2, 3].contains(&x) || x == 0)` - -error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:19:10 - | -LL | .find(|x| [1, 2, 3].contains(x) || *x == 0 || [4, 5, 6].contains(x) || *x == -1) - | __________^ -LL | | .is_some(); - | |__________________^ help: use `any()` instead: `any(|x| [1, 2, 3].contains(&x) || x == 0 || [4, 5, 6].contains(&x) || x == -1)` - -error: called `is_some()` after searching an `Iterator` with `position` - --> $DIR/search_is_some_fixable.rs:23:22 - | -LL | let _ = v.iter().position(|&x| x < 0).is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|&x| x < 0)` - -error: called `is_some()` after searching an `Iterator` with `rposition` - --> $DIR/search_is_some_fixable.rs:26:22 - | -LL | let _ = v.iter().rposition(|&x| x < 0).is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|&x| x < 0)` - -error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:31:27 - | -LL | let _ = "hello world".find("world").is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains("world")` - -error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:32:27 - | -LL | let _ = "hello world".find(&s2).is_some(); - | ^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains(&s2)` - -error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:33:27 - | -LL | let _ = "hello world".find(&s2[2..]).is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains(&s2[2..])` - -error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:35:16 - | -LL | let _ = s1.find("world").is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains("world")` - -error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:36:16 - | -LL | let _ = s1.find(&s2).is_some(); - | ^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains(&s2)` - -error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:37:16 - | -LL | let _ = s1.find(&s2[2..]).is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains(&s2[2..])` - -error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:39:21 - | -LL | let _ = s1[2..].find("world").is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains("world")` - -error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:40:21 - | -LL | let _ = s1[2..].find(&s2).is_some(); - | ^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains(&s2)` - -error: called `is_some()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:41:21 - | -LL | let _ = s1[2..].find(&s2[2..]).is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains(&s2[2..])` - -error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:49:13 - | -LL | let _ = v.iter().find(|&x| *x < 0).is_none(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|x| *x < 0)` - -error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:50:13 - | -LL | let _ = (0..1).find(|x| **y == *x).is_none(); // one dereference less - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!(0..1).any(|x| **y == x)` - -error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:51:13 - | -LL | let _ = (0..1).find(|x| *x == 0).is_none(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!(0..1).any(|x| x == 0)` - -error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:52:13 - | -LL | let _ = v.iter().find(|x| **x == 0).is_none(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|x| *x == 0)` - -error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:53:13 - | -LL | let _ = (4..5).find(|x| *x == 1 || *x == 3 || *x == 5).is_none(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!(4..5).any(|x| x == 1 || x == 3 || x == 5)` - -error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:54:13 - | -LL | let _ = (1..3).find(|x| [1, 2, 3].contains(x)).is_none(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!(1..3).any(|x| [1, 2, 3].contains(&x))` - -error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:55:13 - | -LL | let _ = (1..3).find(|x| *x == 0 || [1, 2, 3].contains(x)).is_none(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!(1..3).any(|x| x == 0 || [1, 2, 3].contains(&x))` - -error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:56:13 - | -LL | let _ = (1..3).find(|x| [1, 2, 3].contains(x) || *x == 0).is_none(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!(1..3).any(|x| [1, 2, 3].contains(&x) || x == 0)` - -error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:57:13 - | -LL | let _ = (1..3) - | _____________^ -LL | | .find(|x| [1, 2, 3].contains(x) || *x == 0 || [4, 5, 6].contains(x) || *x == -1) -LL | | .is_none(); - | |__________________^ help: use `!_.any()` instead: `!(1..3).any(|x| [1, 2, 3].contains(&x) || x == 0 || [4, 5, 6].contains(&x) || x == -1)` - -error: called `is_none()` after searching an `Iterator` with `position` - --> $DIR/search_is_some_fixable.rs:62:13 - | -LL | let _ = v.iter().position(|&x| x < 0).is_none(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|&x| x < 0)` - -error: called `is_none()` after searching an `Iterator` with `rposition` - --> $DIR/search_is_some_fixable.rs:65:13 - | -LL | let _ = v.iter().rposition(|&x| x < 0).is_none(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|&x| x < 0)` - -error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:71:13 - | -LL | let _ = "hello world".find("world").is_none(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!"hello world".contains("world")` - -error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:72:13 - | -LL | let _ = "hello world".find(&s2).is_none(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!"hello world".contains(&s2)` - -error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:73:13 - | -LL | let _ = "hello world".find(&s2[2..]).is_none(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!"hello world".contains(&s2[2..])` - -error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:75:13 - | -LL | let _ = s1.find("world").is_none(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!s1.contains("world")` - -error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:76:13 - | -LL | let _ = s1.find(&s2).is_none(); - | ^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!s1.contains(&s2)` - -error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:77:13 - | -LL | let _ = s1.find(&s2[2..]).is_none(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!s1.contains(&s2[2..])` - -error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:79:13 - | -LL | let _ = s1[2..].find("world").is_none(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!s1[2..].contains("world")` - -error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:80:13 - | -LL | let _ = s1[2..].find(&s2).is_none(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!s1[2..].contains(&s2)` - -error: called `is_none()` after calling `find()` on a string - --> $DIR/search_is_some_fixable.rs:81:13 - | -LL | let _ = s1[2..].find(&s2[2..]).is_none(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!s1[2..].contains(&s2[2..])` - -error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:97:25 - | -LL | .filter(|c| filter_hand.iter().find(|cc| c == cc).is_none()) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!filter_hand.iter().any(|cc| c == &cc)` - -error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable.rs:113:30 - | -LL | .filter(|(c, _)| filter_hand.iter().find(|cc| c == *cc).is_none()) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!filter_hand.iter().any(|cc| c == cc)` - -error: aborting due to 42 previous errors - diff --git a/tests/ui/search_is_some_fixable.fixed b/tests/ui/search_is_some_fixable_none.fixed similarity index 62% rename from tests/ui/search_is_some_fixable.fixed rename to tests/ui/search_is_some_fixable_none.fixed index 967b3d1be432..5d9953f9b3c5 100644 --- a/tests/ui/search_is_some_fixable.fixed +++ b/tests/ui/search_is_some_fixable_none.fixed @@ -6,44 +6,6 @@ fn main() { let v = vec![3, 2, 1, 0, -1, -2, -3]; let y = &&42; - // Check `find().is_some()`, single-line case. - let _ = v.iter().any(|x| *x < 0); - let _ = (0..1).any(|x| **y == x); // one dereference less - let _ = (0..1).any(|x| x == 0); - let _ = v.iter().any(|x| *x == 0); - let _ = (4..5).any(|x| x == 1 || x == 3 || x == 5); - let _ = (1..3).any(|x| [1, 2, 3].contains(&x)); - let _ = (1..3).any(|x| x == 0 || [1, 2, 3].contains(&x)); - let _ = (1..3).any(|x| [1, 2, 3].contains(&x) || x == 0); - let _ = (1..3) - .any(|x| [1, 2, 3].contains(&x) || x == 0 || [4, 5, 6].contains(&x) || x == -1); - - // Check `position().is_some()`, single-line case. - let _ = v.iter().any(|&x| x < 0); - - // Check `rposition().is_some()`, single-line case. - let _ = v.iter().any(|&x| x < 0); - - let s1 = String::from("hello world"); - let s2 = String::from("world"); - // caller of `find()` is a `&`static str` - let _ = "hello world".contains("world"); - let _ = "hello world".contains(&s2); - let _ = "hello world".contains(&s2[2..]); - // caller of `find()` is a `String` - let _ = s1.contains("world"); - let _ = s1.contains(&s2); - let _ = s1.contains(&s2[2..]); - // caller of `find()` is slice of `String` - let _ = s1[2..].contains("world"); - let _ = s1[2..].contains(&s2); - let _ = s1[2..].contains(&s2[2..]); -} - -fn is_none() { - let v = vec![3, 2, 1, 0, -1, -2, -3]; - let y = &&42; - // Check `find().is_none()`, single-line case. let _ = !v.iter().any(|x| *x < 0); let _ = !(0..1).any(|x| **y == x); // one dereference less diff --git a/tests/ui/search_is_some_fixable.rs b/tests/ui/search_is_some_fixable_none.rs similarity index 61% rename from tests/ui/search_is_some_fixable.rs rename to tests/ui/search_is_some_fixable_none.rs index e642523a5320..2a8aadfd0e06 100644 --- a/tests/ui/search_is_some_fixable.rs +++ b/tests/ui/search_is_some_fixable_none.rs @@ -6,45 +6,6 @@ fn main() { 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(); - let _ = (4..5).find(|x| *x == 1 || *x == 3 || *x == 5).is_some(); - let _ = (1..3).find(|x| [1, 2, 3].contains(x)).is_some(); - let _ = (1..3).find(|x| *x == 0 || [1, 2, 3].contains(x)).is_some(); - let _ = (1..3).find(|x| [1, 2, 3].contains(x) || *x == 0).is_some(); - let _ = (1..3) - .find(|x| [1, 2, 3].contains(x) || *x == 0 || [4, 5, 6].contains(x) || *x == -1) - .is_some(); - - // Check `position().is_some()`, single-line case. - let _ = v.iter().position(|&x| x < 0).is_some(); - - // Check `rposition().is_some()`, single-line case. - let _ = v.iter().rposition(|&x| x < 0).is_some(); - - let s1 = String::from("hello world"); - let s2 = String::from("world"); - // caller of `find()` is a `&`static str` - let _ = "hello world".find("world").is_some(); - let _ = "hello world".find(&s2).is_some(); - let _ = "hello world".find(&s2[2..]).is_some(); - // caller of `find()` is a `String` - let _ = s1.find("world").is_some(); - let _ = s1.find(&s2).is_some(); - let _ = s1.find(&s2[2..]).is_some(); - // caller of `find()` is slice of `String` - let _ = s1[2..].find("world").is_some(); - let _ = s1[2..].find(&s2).is_some(); - let _ = s1[2..].find(&s2[2..]).is_some(); -} - -fn is_none() { - let v = vec![3, 2, 1, 0, -1, -2, -3]; - let y = &&42; - // Check `find().is_none()`, single-line case. let _ = v.iter().find(|&x| *x < 0).is_none(); let _ = (0..1).find(|x| **y == *x).is_none(); // one dereference less diff --git a/tests/ui/search_is_some_fixable_none.stderr b/tests/ui/search_is_some_fixable_none.stderr new file mode 100644 index 000000000000..34768feb6ddf --- /dev/null +++ b/tests/ui/search_is_some_fixable_none.stderr @@ -0,0 +1,139 @@ +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:10:13 + | +LL | let _ = v.iter().find(|&x| *x < 0).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|x| *x < 0)` + | + = note: `-D clippy::search-is-some` implied by `-D warnings` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:11:13 + | +LL | let _ = (0..1).find(|x| **y == *x).is_none(); // one dereference less + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!(0..1).any(|x| **y == x)` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:12:13 + | +LL | let _ = (0..1).find(|x| *x == 0).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!(0..1).any(|x| x == 0)` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:13:13 + | +LL | let _ = v.iter().find(|x| **x == 0).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|x| *x == 0)` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:14:13 + | +LL | let _ = (4..5).find(|x| *x == 1 || *x == 3 || *x == 5).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!(4..5).any(|x| x == 1 || x == 3 || x == 5)` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:15:13 + | +LL | let _ = (1..3).find(|x| [1, 2, 3].contains(x)).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!(1..3).any(|x| [1, 2, 3].contains(&x))` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:16:13 + | +LL | let _ = (1..3).find(|x| *x == 0 || [1, 2, 3].contains(x)).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!(1..3).any(|x| x == 0 || [1, 2, 3].contains(&x))` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:17:13 + | +LL | let _ = (1..3).find(|x| [1, 2, 3].contains(x) || *x == 0).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!(1..3).any(|x| [1, 2, 3].contains(&x) || x == 0)` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:18:13 + | +LL | let _ = (1..3) + | _____________^ +LL | | .find(|x| [1, 2, 3].contains(x) || *x == 0 || [4, 5, 6].contains(x) || *x == -1) +LL | | .is_none(); + | |__________________^ help: use `!_.any()` instead: `!(1..3).any(|x| [1, 2, 3].contains(&x) || x == 0 || [4, 5, 6].contains(&x) || x == -1)` + +error: called `is_none()` after searching an `Iterator` with `position` + --> $DIR/search_is_some_fixable_none.rs:23:13 + | +LL | let _ = v.iter().position(|&x| x < 0).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|&x| x < 0)` + +error: called `is_none()` after searching an `Iterator` with `rposition` + --> $DIR/search_is_some_fixable_none.rs:26:13 + | +LL | let _ = v.iter().rposition(|&x| x < 0).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|&x| x < 0)` + +error: called `is_none()` after calling `find()` on a string + --> $DIR/search_is_some_fixable_none.rs:32:13 + | +LL | let _ = "hello world".find("world").is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!"hello world".contains("world")` + +error: called `is_none()` after calling `find()` on a string + --> $DIR/search_is_some_fixable_none.rs:33:13 + | +LL | let _ = "hello world".find(&s2).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!"hello world".contains(&s2)` + +error: called `is_none()` after calling `find()` on a string + --> $DIR/search_is_some_fixable_none.rs:34:13 + | +LL | let _ = "hello world".find(&s2[2..]).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!"hello world".contains(&s2[2..])` + +error: called `is_none()` after calling `find()` on a string + --> $DIR/search_is_some_fixable_none.rs:36:13 + | +LL | let _ = s1.find("world").is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!s1.contains("world")` + +error: called `is_none()` after calling `find()` on a string + --> $DIR/search_is_some_fixable_none.rs:37:13 + | +LL | let _ = s1.find(&s2).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!s1.contains(&s2)` + +error: called `is_none()` after calling `find()` on a string + --> $DIR/search_is_some_fixable_none.rs:38:13 + | +LL | let _ = s1.find(&s2[2..]).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!s1.contains(&s2[2..])` + +error: called `is_none()` after calling `find()` on a string + --> $DIR/search_is_some_fixable_none.rs:40:13 + | +LL | let _ = s1[2..].find("world").is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!s1[2..].contains("world")` + +error: called `is_none()` after calling `find()` on a string + --> $DIR/search_is_some_fixable_none.rs:41:13 + | +LL | let _ = s1[2..].find(&s2).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!s1[2..].contains(&s2)` + +error: called `is_none()` after calling `find()` on a string + --> $DIR/search_is_some_fixable_none.rs:42:13 + | +LL | let _ = s1[2..].find(&s2[2..]).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.contains()` instead: `!s1[2..].contains(&s2[2..])` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:58:25 + | +LL | .filter(|c| filter_hand.iter().find(|cc| c == cc).is_none()) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!filter_hand.iter().any(|cc| c == &cc)` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:74:30 + | +LL | .filter(|(c, _)| filter_hand.iter().find(|cc| c == *cc).is_none()) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!filter_hand.iter().any(|cc| c == cc)` + +error: aborting due to 22 previous errors + diff --git a/tests/ui/search_is_some_fixable_some.fixed b/tests/ui/search_is_some_fixable_some.fixed new file mode 100644 index 000000000000..94a4c7b3639c --- /dev/null +++ b/tests/ui/search_is_some_fixable_some.fixed @@ -0,0 +1,76 @@ +// run-rustfix +#![allow(dead_code)] +#![warn(clippy::search_is_some)] + +fn main() { + let v = vec![3, 2, 1, 0, -1, -2, -3]; + let y = &&42; + + // Check `find().is_some()`, single-line case. + let _ = v.iter().any(|x| *x < 0); + let _ = (0..1).any(|x| **y == x); // one dereference less + let _ = (0..1).any(|x| x == 0); + let _ = v.iter().any(|x| *x == 0); + let _ = (4..5).any(|x| x == 1 || x == 3 || x == 5); + let _ = (1..3).any(|x| [1, 2, 3].contains(&x)); + let _ = (1..3).any(|x| x == 0 || [1, 2, 3].contains(&x)); + let _ = (1..3).any(|x| [1, 2, 3].contains(&x) || x == 0); + let _ = (1..3) + .any(|x| [1, 2, 3].contains(&x) || x == 0 || [4, 5, 6].contains(&x) || x == -1); + + // Check `position().is_some()`, single-line case. + let _ = v.iter().any(|&x| x < 0); + + // Check `rposition().is_some()`, single-line case. + let _ = v.iter().any(|&x| x < 0); + + let s1 = String::from("hello world"); + let s2 = String::from("world"); + // caller of `find()` is a `&`static str` + let _ = "hello world".contains("world"); + let _ = "hello world".contains(&s2); + let _ = "hello world".contains(&s2[2..]); + // caller of `find()` is a `String` + let _ = s1.contains("world"); + let _ = s1.contains(&s2); + let _ = s1.contains(&s2[2..]); + // caller of `find()` is slice of `String` + let _ = s1[2..].contains("world"); + let _ = s1[2..].contains(&s2); + let _ = s1[2..].contains(&s2[2..]); +} + +#[allow(clippy::clone_on_copy, clippy::map_clone)] +mod issue7392 { + struct Player { + hand: Vec, + } + fn filter() { + let p = Player { + hand: vec![1, 2, 3, 4, 5], + }; + let filter_hand = vec![5]; + let _ = p + .hand + .iter() + .filter(|c| filter_hand.iter().any(|cc| c == &cc)) + .map(|c| c.clone()) + .collect::>(); + } + + struct PlayerTuple { + hand: Vec<(usize, char)>, + } + fn filter_tuple() { + let p = PlayerTuple { + hand: vec![(1, 'a'), (2, 'b'), (3, 'c'), (4, 'd'), (5, 'e')], + }; + let filter_hand = vec![5]; + let _ = p + .hand + .iter() + .filter(|(c, _)| filter_hand.iter().any(|cc| c == cc)) + .map(|c| c.clone()) + .collect::>(); + } +} diff --git a/tests/ui/search_is_some_fixable_some.rs b/tests/ui/search_is_some_fixable_some.rs new file mode 100644 index 000000000000..8887b0327e22 --- /dev/null +++ b/tests/ui/search_is_some_fixable_some.rs @@ -0,0 +1,77 @@ +// run-rustfix +#![allow(dead_code)] +#![warn(clippy::search_is_some)] + +fn main() { + 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(); + let _ = (4..5).find(|x| *x == 1 || *x == 3 || *x == 5).is_some(); + let _ = (1..3).find(|x| [1, 2, 3].contains(x)).is_some(); + let _ = (1..3).find(|x| *x == 0 || [1, 2, 3].contains(x)).is_some(); + let _ = (1..3).find(|x| [1, 2, 3].contains(x) || *x == 0).is_some(); + let _ = (1..3) + .find(|x| [1, 2, 3].contains(x) || *x == 0 || [4, 5, 6].contains(x) || *x == -1) + .is_some(); + + // Check `position().is_some()`, single-line case. + let _ = v.iter().position(|&x| x < 0).is_some(); + + // Check `rposition().is_some()`, single-line case. + let _ = v.iter().rposition(|&x| x < 0).is_some(); + + let s1 = String::from("hello world"); + let s2 = String::from("world"); + // caller of `find()` is a `&`static str` + let _ = "hello world".find("world").is_some(); + let _ = "hello world".find(&s2).is_some(); + let _ = "hello world".find(&s2[2..]).is_some(); + // caller of `find()` is a `String` + let _ = s1.find("world").is_some(); + let _ = s1.find(&s2).is_some(); + let _ = s1.find(&s2[2..]).is_some(); + // caller of `find()` is slice of `String` + let _ = s1[2..].find("world").is_some(); + let _ = s1[2..].find(&s2).is_some(); + let _ = s1[2..].find(&s2[2..]).is_some(); +} + +#[allow(clippy::clone_on_copy, clippy::map_clone)] +mod issue7392 { + struct Player { + hand: Vec, + } + fn filter() { + let p = Player { + hand: vec![1, 2, 3, 4, 5], + }; + let filter_hand = vec![5]; + let _ = p + .hand + .iter() + .filter(|c| filter_hand.iter().find(|cc| c == cc).is_some()) + .map(|c| c.clone()) + .collect::>(); + } + + struct PlayerTuple { + hand: Vec<(usize, char)>, + } + fn filter_tuple() { + let p = PlayerTuple { + hand: vec![(1, 'a'), (2, 'b'), (3, 'c'), (4, 'd'), (5, 'e')], + }; + let filter_hand = vec![5]; + let _ = p + .hand + .iter() + .filter(|(c, _)| filter_hand.iter().find(|cc| c == *cc).is_some()) + .map(|c| c.clone()) + .collect::>(); + } +} diff --git a/tests/ui/search_is_some_fixable_some.stderr b/tests/ui/search_is_some_fixable_some.stderr new file mode 100644 index 000000000000..90615a85ca3d --- /dev/null +++ b/tests/ui/search_is_some_fixable_some.stderr @@ -0,0 +1,138 @@ +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:10:22 + | +LL | let _ = v.iter().find(|&x| *x < 0).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| *x < 0)` + | + = note: `-D clippy::search-is-some` implied by `-D warnings` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:11:20 + | +LL | let _ = (0..1).find(|x| **y == *x).is_some(); // one dereference less + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| **y == x)` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:12:20 + | +LL | let _ = (0..1).find(|x| *x == 0).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| x == 0)` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:13:22 + | +LL | let _ = v.iter().find(|x| **x == 0).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| *x == 0)` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:14:20 + | +LL | let _ = (4..5).find(|x| *x == 1 || *x == 3 || *x == 5).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| x == 1 || x == 3 || x == 5)` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:15:20 + | +LL | let _ = (1..3).find(|x| [1, 2, 3].contains(x)).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| [1, 2, 3].contains(&x))` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:16:20 + | +LL | let _ = (1..3).find(|x| *x == 0 || [1, 2, 3].contains(x)).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| x == 0 || [1, 2, 3].contains(&x))` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:17:20 + | +LL | let _ = (1..3).find(|x| [1, 2, 3].contains(x) || *x == 0).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| [1, 2, 3].contains(&x) || x == 0)` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:19:10 + | +LL | .find(|x| [1, 2, 3].contains(x) || *x == 0 || [4, 5, 6].contains(x) || *x == -1) + | __________^ +LL | | .is_some(); + | |__________________^ help: use `any()` instead: `any(|x| [1, 2, 3].contains(&x) || x == 0 || [4, 5, 6].contains(&x) || x == -1)` + +error: called `is_some()` after searching an `Iterator` with `position` + --> $DIR/search_is_some_fixable_some.rs:23:22 + | +LL | let _ = v.iter().position(|&x| x < 0).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|&x| x < 0)` + +error: called `is_some()` after searching an `Iterator` with `rposition` + --> $DIR/search_is_some_fixable_some.rs:26:22 + | +LL | let _ = v.iter().rposition(|&x| x < 0).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|&x| x < 0)` + +error: called `is_some()` after calling `find()` on a string + --> $DIR/search_is_some_fixable_some.rs:31:27 + | +LL | let _ = "hello world".find("world").is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains("world")` + +error: called `is_some()` after calling `find()` on a string + --> $DIR/search_is_some_fixable_some.rs:32:27 + | +LL | let _ = "hello world".find(&s2).is_some(); + | ^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains(&s2)` + +error: called `is_some()` after calling `find()` on a string + --> $DIR/search_is_some_fixable_some.rs:33:27 + | +LL | let _ = "hello world".find(&s2[2..]).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains(&s2[2..])` + +error: called `is_some()` after calling `find()` on a string + --> $DIR/search_is_some_fixable_some.rs:35:16 + | +LL | let _ = s1.find("world").is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains("world")` + +error: called `is_some()` after calling `find()` on a string + --> $DIR/search_is_some_fixable_some.rs:36:16 + | +LL | let _ = s1.find(&s2).is_some(); + | ^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains(&s2)` + +error: called `is_some()` after calling `find()` on a string + --> $DIR/search_is_some_fixable_some.rs:37:16 + | +LL | let _ = s1.find(&s2[2..]).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains(&s2[2..])` + +error: called `is_some()` after calling `find()` on a string + --> $DIR/search_is_some_fixable_some.rs:39:21 + | +LL | let _ = s1[2..].find("world").is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains("world")` + +error: called `is_some()` after calling `find()` on a string + --> $DIR/search_is_some_fixable_some.rs:40:21 + | +LL | let _ = s1[2..].find(&s2).is_some(); + | ^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains(&s2)` + +error: called `is_some()` after calling `find()` on a string + --> $DIR/search_is_some_fixable_some.rs:41:21 + | +LL | let _ = s1[2..].find(&s2[2..]).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: use `contains()` instead: `contains(&s2[2..])` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:57:44 + | +LL | .filter(|c| filter_hand.iter().find(|cc| c == cc).is_some()) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|cc| c == &cc)` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:73:49 + | +LL | .filter(|(c, _)| filter_hand.iter().find(|cc| c == *cc).is_some()) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|cc| c == cc)` + +error: aborting due to 22 previous errors + From 6dca4f261d01584aac507379649458fbaf04d4c8 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Thu, 19 Aug 2021 11:50:05 +0200 Subject: [PATCH 05/25] Add some doc for `search_is_some` lint --- clippy_lints/src/methods/search_is_some.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/clippy_lints/src/methods/search_is_some.rs b/clippy_lints/src/methods/search_is_some.rs index ee0e6d3a33f4..b0bfe8d80e0d 100644 --- a/clippy_lints/src/methods/search_is_some.rs +++ b/clippy_lints/src/methods/search_is_some.rs @@ -50,6 +50,8 @@ pub(super) fn check<'tcx>( if let hir::PatKind::Ref(..) = closure_arg.pat.kind { Some(search_snippet.replacen('&', "", 1)) } else if let PatKind::Binding(..) = strip_pat_refs(closure_arg.pat).kind { + // `find()` provides a reference to the item, but `any` does not, + // so we should fix item usages for suggestion get_closure_suggestion(cx, search_arg, closure_body) .or_else(|| Some(search_snippet.to_string())) } else { @@ -151,6 +153,9 @@ pub(super) fn check<'tcx>( } } +// Build suggestion gradually by handling closure arg specific usages, +// such as explicit deref and borrowing cases. +// Returns `None` if no such use cases have been triggered in closure body fn get_closure_suggestion<'tcx>( cx: &LateContext<'_>, search_arg: &'tcx hir::Expr<'_>, @@ -203,8 +208,12 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { let end_snip = snippet(self.cx, end_span, ".."); if cmt.place.projections.is_empty() { + // handle item without any projection, that needs an explicit borrowing + // i.e.: suggest `&x` instead of `x` self.suggestion_start.push_str(&format!("{}&{}", start_snip, ident_str)); } else { + // cases where a parent call is using the item + // i.e.: suggest `.contains(&x)` for `.find(|x| [1, 2, 3].contains(x)).is_none()` let parent_expr = get_parent_expr_for_hir(self.cx, cmt.hir_id); if let Some(Expr { hir_id: _, kind, .. }) = parent_expr { if let ExprKind::Call(_, args) | ExprKind::MethodCall(_, _, args, _) = kind { @@ -230,6 +239,8 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { } } + // handle item projections by removing one explicit deref + // i.e.: suggest `*x` instead of `**x` let mut replacement_str = ident_str; let last_deref = cmt .place From e24aba2c1a8663c2ddebdae740689f8a7bdcadb7 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Tue, 7 Sep 2021 11:06:50 +0200 Subject: [PATCH 06/25] Use applicability for snippets --- clippy_lints/src/methods/search_is_some.rs | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/methods/search_is_some.rs b/clippy_lints/src/methods/search_is_some.rs index b0bfe8d80e0d..ec09bfd743d0 100644 --- a/clippy_lints/src/methods/search_is_some.rs +++ b/clippy_lints/src/methods/search_is_some.rs @@ -167,6 +167,7 @@ fn get_closure_suggestion<'tcx>( next_pos: None, suggestion_start: String::new(), suggestion_end: String::new(), + applicability: Applicability::MachineApplicable, }; let fn_def_id = cx.tcx.hir().local_def_id(search_arg.hir_id); @@ -188,6 +189,7 @@ struct DerefDelegate<'a, 'tcx> { next_pos: Option, suggestion_start: String, suggestion_end: String, + applicability: Applicability, } impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { @@ -203,9 +205,9 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { } else { self.closure_span.until(span) }; - let start_snip = snippet(self.cx, start_span, ".."); + let start_snip = snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability); let end_span = Span::new(span.hi(), self.closure_span.hi(), span.ctxt()); - let end_snip = snippet(self.cx, end_span, ".."); + let end_snip = snippet_with_applicability(self.cx, end_span, "..", &mut self.applicability); if cmt.place.projections.is_empty() { // handle item without any projection, that needs an explicit borrowing @@ -227,11 +229,14 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { } else { self.closure_span.until(span) }; - let start_snip = snippet(self.cx, start_span, ".."); + let start_snip = + snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability); self.suggestion_start.push_str(&format!("{}&{}", start_snip, ident_str)); self.suggestion_end = end_snip.to_string(); self.next_pos = Some(span.hi()); + } else { + self.applicability = Applicability::Unspecified; } } return; From 9ab4b673eb41cca04a626797bb130bda4a14bd1b Mon Sep 17 00:00:00 2001 From: ThibsG Date: Tue, 7 Sep 2021 11:18:27 +0200 Subject: [PATCH 07/25] Simplifying `next_pos` init --- clippy_lints/src/methods/search_is_some.rs | 20 ++++++-------------- 1 file changed, 6 insertions(+), 14 deletions(-) diff --git a/clippy_lints/src/methods/search_is_some.rs b/clippy_lints/src/methods/search_is_some.rs index ec09bfd743d0..b38b8e44f1c6 100644 --- a/clippy_lints/src/methods/search_is_some.rs +++ b/clippy_lints/src/methods/search_is_some.rs @@ -164,7 +164,7 @@ fn get_closure_suggestion<'tcx>( let mut visitor = DerefDelegate { cx, closure_span: search_arg.span, - next_pos: None, + next_pos: search_arg.span.lo(), suggestion_start: String::new(), suggestion_end: String::new(), applicability: Applicability::MachineApplicable, @@ -186,7 +186,7 @@ fn get_closure_suggestion<'tcx>( struct DerefDelegate<'a, 'tcx> { cx: &'a LateContext<'tcx>, closure_span: Span, - next_pos: Option, + next_pos: BytePos, suggestion_start: String, suggestion_end: String, applicability: Applicability, @@ -200,11 +200,7 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { let map = self.cx.tcx.hir(); let ident_str = map.name(id).to_string(); let span = map.span(cmt.hir_id); - let start_span = if let Some(next_pos) = self.next_pos { - Span::new(next_pos, span.lo(), span.ctxt()) - } else { - self.closure_span.until(span) - }; + let start_span = Span::new(self.next_pos, span.lo(), span.ctxt()); let start_snip = snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability); let end_span = Span::new(span.hi(), self.closure_span.hi(), span.ctxt()); let end_snip = snippet_with_applicability(self.cx, end_span, "..", &mut self.applicability); @@ -224,17 +220,13 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { for arg in &args_to_handle { let arg_ty_kind = self.cx.typeck_results().expr_ty(arg).kind(); if matches!(arg_ty_kind, ty::Ref(_, _, Mutability::Not)) { - let start_span = if let Some(next_pos) = self.next_pos { - Span::new(next_pos, span.lo(), span.ctxt()) - } else { - self.closure_span.until(span) - }; + let start_span = Span::new(self.next_pos, span.lo(), span.ctxt()); let start_snip = snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability); self.suggestion_start.push_str(&format!("{}&{}", start_snip, ident_str)); self.suggestion_end = end_snip.to_string(); - self.next_pos = Some(span.hi()); + self.next_pos = span.hi(); } else { self.applicability = Applicability::Unspecified; } @@ -268,7 +260,7 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { .push_str(&format!("{}{}", start_snip, replacement_str)); self.suggestion_end = end_snip.to_string(); } - self.next_pos = Some(span.hi()); + self.next_pos = span.hi(); } } From d0dd797709540f460ee6c9ac26f1231335781b40 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Tue, 7 Sep 2021 11:59:53 +0200 Subject: [PATCH 08/25] Build end of suggestion only once at the end of the process --- clippy_lints/src/methods/search_is_some.rs | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/methods/search_is_some.rs b/clippy_lints/src/methods/search_is_some.rs index b38b8e44f1c6..e793c50ac9d1 100644 --- a/clippy_lints/src/methods/search_is_some.rs +++ b/clippy_lints/src/methods/search_is_some.rs @@ -166,7 +166,6 @@ fn get_closure_suggestion<'tcx>( closure_span: search_arg.span, next_pos: search_arg.span.lo(), suggestion_start: String::new(), - suggestion_end: String::new(), applicability: Applicability::MachineApplicable, }; @@ -179,7 +178,7 @@ fn get_closure_suggestion<'tcx>( if visitor.suggestion_start.is_empty() { None } else { - Some(format!("{}{}", visitor.suggestion_start, visitor.suggestion_end)) + Some(visitor.finish()) } } @@ -188,10 +187,17 @@ struct DerefDelegate<'a, 'tcx> { closure_span: Span, next_pos: BytePos, suggestion_start: String, - suggestion_end: String, applicability: Applicability, } +impl DerefDelegate<'_, 'tcx> { + pub fn finish(&mut self) -> String { + let end_span = Span::new(self.next_pos, self.closure_span.hi(), self.closure_span.ctxt()); + let end_snip = snippet_with_applicability(self.cx, end_span, "..", &mut self.applicability); + format!("{}{}", self.suggestion_start, end_snip) + } +} + impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {} @@ -202,8 +208,6 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { let span = map.span(cmt.hir_id); let start_span = Span::new(self.next_pos, span.lo(), span.ctxt()); let start_snip = snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability); - let end_span = Span::new(span.hi(), self.closure_span.hi(), span.ctxt()); - let end_snip = snippet_with_applicability(self.cx, end_span, "..", &mut self.applicability); if cmt.place.projections.is_empty() { // handle item without any projection, that needs an explicit borrowing @@ -225,7 +229,6 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability); self.suggestion_start.push_str(&format!("{}&{}", start_snip, ident_str)); - self.suggestion_end = end_snip.to_string(); self.next_pos = span.hi(); } else { self.applicability = Applicability::Unspecified; @@ -258,7 +261,6 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { self.suggestion_start .push_str(&format!("{}{}", start_snip, replacement_str)); - self.suggestion_end = end_snip.to_string(); } self.next_pos = span.hi(); } From 97783a8cb9b154b17abbf0084fe8a16d490cf801 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Tue, 7 Sep 2021 12:31:14 +0200 Subject: [PATCH 09/25] Return a struct and add applicability --- clippy_lints/src/methods/search_is_some.rs | 25 ++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/clippy_lints/src/methods/search_is_some.rs b/clippy_lints/src/methods/search_is_some.rs index e793c50ac9d1..a12061994600 100644 --- a/clippy_lints/src/methods/search_is_some.rs +++ b/clippy_lints/src/methods/search_is_some.rs @@ -41,6 +41,7 @@ pub(super) fn check<'tcx>( 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 mut applicability = Applicability::MachineApplicable; let any_search_snippet = if_chain! { if search_method == "find"; if let hir::ExprKind::Closure(_, _, body_id, ..) = search_arg.kind; @@ -52,8 +53,12 @@ pub(super) fn check<'tcx>( } else if let PatKind::Binding(..) = strip_pat_refs(closure_arg.pat).kind { // `find()` provides a reference to the item, but `any` does not, // so we should fix item usages for suggestion - get_closure_suggestion(cx, search_arg, closure_body) - .or_else(|| Some(search_snippet.to_string())) + if let Some(closure_sugg) = get_closure_suggestion(cx, search_arg, closure_body) { + applicability = closure_sugg.applicability; + Some(closure_sugg.suggestion) + } else { + Some(search_snippet.to_string()) + } } else { None } @@ -73,7 +78,7 @@ pub(super) fn check<'tcx>( "any({})", any_search_snippet.as_ref().map_or(&*search_snippet, String::as_str) ), - Applicability::MachineApplicable, + applicability, ); } else { let iter = snippet(cx, search_recv.span, ".."); @@ -88,7 +93,7 @@ pub(super) fn check<'tcx>( iter, any_search_snippet.as_ref().map_or(&*search_snippet, String::as_str) ), - Applicability::MachineApplicable, + applicability, ); } } else { @@ -153,6 +158,11 @@ pub(super) fn check<'tcx>( } } +struct ClosureSugg { + applicability: Applicability, + suggestion: String, +} + // Build suggestion gradually by handling closure arg specific usages, // such as explicit deref and borrowing cases. // Returns `None` if no such use cases have been triggered in closure body @@ -160,7 +170,7 @@ fn get_closure_suggestion<'tcx>( cx: &LateContext<'_>, search_arg: &'tcx hir::Expr<'_>, closure_body: &hir::Body<'_>, -) -> Option { +) -> Option { let mut visitor = DerefDelegate { cx, closure_span: search_arg.span, @@ -178,7 +188,10 @@ fn get_closure_suggestion<'tcx>( if visitor.suggestion_start.is_empty() { None } else { - Some(visitor.finish()) + Some(ClosureSugg { + applicability: visitor.applicability, + suggestion: visitor.finish(), + }) } } From 91dd9c46de11b9abeb391d5b19707c5e7ddfef98 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Sun, 12 Sep 2021 11:31:11 +0200 Subject: [PATCH 10/25] Handle other projection kinds --- clippy_lints/src/methods/search_is_some.rs | 80 +++++++++++++++++---- tests/ui/search_is_some_fixable_none.fixed | 24 +++++++ tests/ui/search_is_some_fixable_none.rs | 26 +++++++ tests/ui/search_is_some_fixable_none.stderr | 36 +++++++++- tests/ui/search_is_some_fixable_some.fixed | 25 +++++++ tests/ui/search_is_some_fixable_some.rs | 26 +++++++ tests/ui/search_is_some_fixable_some.stderr | 28 +++++++- 7 files changed, 231 insertions(+), 14 deletions(-) diff --git a/clippy_lints/src/methods/search_is_some.rs b/clippy_lints/src/methods/search_is_some.rs index a12061994600..d09baf50e497 100644 --- a/clippy_lints/src/methods/search_is_some.rs +++ b/clippy_lints/src/methods/search_is_some.rs @@ -220,7 +220,7 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { let ident_str = map.name(id).to_string(); let span = map.span(cmt.hir_id); let start_span = Span::new(self.next_pos, span.lo(), span.ctxt()); - let start_snip = snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability); + let mut start_snip = snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability); if cmt.place.projections.is_empty() { // handle item without any projection, that needs an explicit borrowing @@ -255,19 +255,75 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { // handle item projections by removing one explicit deref // i.e.: suggest `*x` instead of `**x` let mut replacement_str = ident_str; - let last_deref = cmt - .place - .projections - .iter() - .rposition(|proj| proj.kind == ProjectionKind::Deref); - if let Some(pos) = last_deref { - let mut projections = cmt.place.projections.clone(); - projections.truncate(pos); + // handle index projection first + let index_handled = cmt.place.projections.iter().any(|proj| match proj.kind { + // Index projection like `|x| foo[x]` + // the index is dropped so we can't get it to build the suggestion, + // so the span is set-up again to get more code, using `span.hi()` (i.e.: `foo[x]`) + // instead of `span.lo()` (i.e.: `foo`) + ProjectionKind::Index => { + let start_span = Span::new(self.next_pos, span.hi(), span.ctxt()); + start_snip = snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability); + replacement_str.clear(); + true + }, + _ => false, + }); + + // looking for projections other that need to be handled differently + let other_projections_handled = cmt.place.projections.iter().enumerate().any(|(i, proj)| { + match proj.kind { + // Field projection like `|v| v.foo` + ProjectionKind::Field(idx, variant) => match cmt.place.ty_before_projection(i).kind() { + ty::Adt(def, ..) => { + replacement_str = format!( + "{}.{}", + replacement_str, + def.variants[variant].fields[idx as usize].ident.name.as_str() + ); + true + }, + ty::Tuple(_) => { + replacement_str = format!("{}.{}", replacement_str, idx); + true + }, + _ => false, + }, + ProjectionKind::Index => false, /* handled previously */ + // note: unable to capture `Subslice` kind in tests + ProjectionKind::Subslice => false, + ProjectionKind::Deref => { + // explicit deref for arrays should be avoided in the suggestion + // i.e.: `|sub| *sub[1..4].len() == 3` is not expected + match cmt.place.ty_before_projection(i).kind() { + // dereferencing an array (i.e.: `|sub| sub[1..4].len() == 3`) + ty::Ref(_, inner, _) => match inner.kind() { + ty::Ref(_, innermost, _) if innermost.is_array() => true, + _ => false, + }, + _ => false, + } + }, + } + }); - for item in projections { - if item.kind == ProjectionKind::Deref { - replacement_str = format!("*{}", replacement_str); + // handle `ProjectionKind::Deref` if no special case detected + if !index_handled && !other_projections_handled { + let last_deref = cmt + .place + .projections + .iter() + .rposition(|proj| proj.kind == ProjectionKind::Deref); + + if let Some(pos) = last_deref { + let mut projections = cmt.place.projections.clone(); + projections.truncate(pos); + + for item in projections { + if item.kind == ProjectionKind::Deref { + replacement_str = format!("*{}", replacement_str); + } } } } diff --git a/tests/ui/search_is_some_fixable_none.fixed b/tests/ui/search_is_some_fixable_none.fixed index 5d9953f9b3c5..222731fd212a 100644 --- a/tests/ui/search_is_some_fixable_none.fixed +++ b/tests/ui/search_is_some_fixable_none.fixed @@ -73,4 +73,28 @@ mod issue7392 { .map(|c| c.clone()) .collect::>(); } + + fn field_projection() { + struct Foo { + foo: i32, + bar: u32, + } + let vfoo = vec![Foo { foo: 1, bar: 2 }]; + let _ = !vfoo.iter().any(|v| v.foo == 1 && v.bar == 2); + + let vfoo = vec![(42, Foo { foo: 1, bar: 2 })]; + let _ = !vfoo + .iter().any(|(i, v)| *i == 42 && v.foo == 1 && v.bar == 2); + } + + fn index_projection() { + let vfoo = vec![[0, 1, 2, 3]]; + let _ = !vfoo.iter().any(|a| a[0] == 42); + } + + #[allow(clippy::match_like_matches_macro)] + fn slice_projection() { + let vfoo = vec![[0, 1, 2, 3, 0, 1, 2, 3]]; + let _ = !vfoo.iter().any(|sub| sub[1..4].len() == 3); + } } diff --git a/tests/ui/search_is_some_fixable_none.rs b/tests/ui/search_is_some_fixable_none.rs index 2a8aadfd0e06..43ac1a076678 100644 --- a/tests/ui/search_is_some_fixable_none.rs +++ b/tests/ui/search_is_some_fixable_none.rs @@ -75,4 +75,30 @@ mod issue7392 { .map(|c| c.clone()) .collect::>(); } + + fn field_projection() { + struct Foo { + foo: i32, + bar: u32, + } + let vfoo = vec![Foo { foo: 1, bar: 2 }]; + let _ = vfoo.iter().find(|v| v.foo == 1 && v.bar == 2).is_none(); + + let vfoo = vec![(42, Foo { foo: 1, bar: 2 })]; + let _ = vfoo + .iter() + .find(|(i, v)| *i == 42 && v.foo == 1 && v.bar == 2) + .is_none(); + } + + fn index_projection() { + let vfoo = vec![[0, 1, 2, 3]]; + let _ = vfoo.iter().find(|a| a[0] == 42).is_none(); + } + + #[allow(clippy::match_like_matches_macro)] + fn slice_projection() { + let vfoo = vec![[0, 1, 2, 3, 0, 1, 2, 3]]; + let _ = vfoo.iter().find(|sub| sub[1..4].len() == 3).is_none(); + } } diff --git a/tests/ui/search_is_some_fixable_none.stderr b/tests/ui/search_is_some_fixable_none.stderr index 34768feb6ddf..fa344046aeba 100644 --- a/tests/ui/search_is_some_fixable_none.stderr +++ b/tests/ui/search_is_some_fixable_none.stderr @@ -135,5 +135,39 @@ error: called `is_none()` after searching an `Iterator` with `find` LL | .filter(|(c, _)| filter_hand.iter().find(|cc| c == *cc).is_none()) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!filter_hand.iter().any(|cc| c == cc)` -error: aborting due to 22 previous errors +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:85:17 + | +LL | let _ = vfoo.iter().find(|v| v.foo == 1 && v.bar == 2).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!vfoo.iter().any(|v| v.foo == 1 && v.bar == 2)` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:88:17 + | +LL | let _ = vfoo + | _________________^ +LL | | .iter() +LL | | .find(|(i, v)| *i == 42 && v.foo == 1 && v.bar == 2) +LL | | .is_none(); + | |______________________^ + | +help: use `!_.any()` instead + | +LL ~ let _ = !vfoo +LL ~ .iter().any(|(i, v)| *i == 42 && v.foo == 1 && v.bar == 2); + | + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:96:17 + | +LL | let _ = vfoo.iter().find(|a| a[0] == 42).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!vfoo.iter().any(|a| a[0] == 42)` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:102:17 + | +LL | let _ = vfoo.iter().find(|sub| sub[1..4].len() == 3).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!vfoo.iter().any(|sub| sub[1..4].len() == 3)` + +error: aborting due to 26 previous errors diff --git a/tests/ui/search_is_some_fixable_some.fixed b/tests/ui/search_is_some_fixable_some.fixed index 94a4c7b3639c..ae9478a42e9f 100644 --- a/tests/ui/search_is_some_fixable_some.fixed +++ b/tests/ui/search_is_some_fixable_some.fixed @@ -73,4 +73,29 @@ mod issue7392 { .map(|c| c.clone()) .collect::>(); } + + fn field_projection() { + struct Foo { + foo: i32, + bar: u32, + } + let vfoo = vec![Foo { foo: 1, bar: 2 }]; + let _ = vfoo.iter().any(|v| v.foo == 1 && v.bar == 2); + + let vfoo = vec![(42, Foo { foo: 1, bar: 2 })]; + let _ = vfoo + .iter() + .any(|(i, v)| *i == 42 && v.foo == 1 && v.bar == 2); + } + + fn index_projection() { + let vfoo = vec![[0, 1, 2, 3]]; + let _ = vfoo.iter().any(|a| a[0] == 42); + } + + #[allow(clippy::match_like_matches_macro)] + fn slice_projection() { + let vfoo = vec![[0, 1, 2, 3, 0, 1, 2, 3]]; + let _ = vfoo.iter().any(|sub| sub[1..4].len() == 3); + } } diff --git a/tests/ui/search_is_some_fixable_some.rs b/tests/ui/search_is_some_fixable_some.rs index 8887b0327e22..98c3e7c1a685 100644 --- a/tests/ui/search_is_some_fixable_some.rs +++ b/tests/ui/search_is_some_fixable_some.rs @@ -74,4 +74,30 @@ mod issue7392 { .map(|c| c.clone()) .collect::>(); } + + fn field_projection() { + struct Foo { + foo: i32, + bar: u32, + } + let vfoo = vec![Foo { foo: 1, bar: 2 }]; + let _ = vfoo.iter().find(|v| v.foo == 1 && v.bar == 2).is_some(); + + let vfoo = vec![(42, Foo { foo: 1, bar: 2 })]; + let _ = vfoo + .iter() + .find(|(i, v)| *i == 42 && v.foo == 1 && v.bar == 2) + .is_some(); + } + + fn index_projection() { + let vfoo = vec![[0, 1, 2, 3]]; + let _ = vfoo.iter().find(|a| a[0] == 42).is_some(); + } + + #[allow(clippy::match_like_matches_macro)] + fn slice_projection() { + let vfoo = vec![[0, 1, 2, 3, 0, 1, 2, 3]]; + let _ = vfoo.iter().find(|sub| sub[1..4].len() == 3).is_some(); + } } diff --git a/tests/ui/search_is_some_fixable_some.stderr b/tests/ui/search_is_some_fixable_some.stderr index 90615a85ca3d..4161b5eec0a7 100644 --- a/tests/ui/search_is_some_fixable_some.stderr +++ b/tests/ui/search_is_some_fixable_some.stderr @@ -134,5 +134,31 @@ error: called `is_some()` after searching an `Iterator` with `find` LL | .filter(|(c, _)| filter_hand.iter().find(|cc| c == *cc).is_some()) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|cc| c == cc)` -error: aborting due to 22 previous errors +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:84:29 + | +LL | let _ = vfoo.iter().find(|v| v.foo == 1 && v.bar == 2).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|v| v.foo == 1 && v.bar == 2)` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:89:14 + | +LL | .find(|(i, v)| *i == 42 && v.foo == 1 && v.bar == 2) + | ______________^ +LL | | .is_some(); + | |______________________^ help: use `any()` instead: `any(|(i, v)| *i == 42 && v.foo == 1 && v.bar == 2)` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:95:29 + | +LL | let _ = vfoo.iter().find(|a| a[0] == 42).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|a| a[0] == 42)` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:101:29 + | +LL | let _ = vfoo.iter().find(|sub| sub[1..4].len() == 3).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|sub| sub[1..4].len() == 3)` + +error: aborting due to 26 previous errors From 788c9ccc938ac0384fd06a071806502121af1a5a Mon Sep 17 00:00:00 2001 From: ThibsG Date: Sun, 12 Sep 2021 11:40:14 +0200 Subject: [PATCH 11/25] Applying refactoring for simplified code --- clippy_lints/src/methods/search_is_some.rs | 47 ++++++++++------------ 1 file changed, 22 insertions(+), 25 deletions(-) diff --git a/clippy_lints/src/methods/search_is_some.rs b/clippy_lints/src/methods/search_is_some.rs index d09baf50e497..a5e6a8df03f3 100644 --- a/clippy_lints/src/methods/search_is_some.rs +++ b/clippy_lints/src/methods/search_is_some.rs @@ -5,7 +5,7 @@ use clippy_utils::{get_parent_expr_for_hir, is_trait_method, strip_pat_refs}; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir as hir; -use rustc_hir::{self, Expr, ExprKind, HirId, PatKind}; +use rustc_hir::{self, ExprKind, HirId, PatKind}; use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::LateContext; use rustc_middle::hir::place::ProjectionKind; @@ -229,26 +229,23 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { } else { // cases where a parent call is using the item // i.e.: suggest `.contains(&x)` for `.find(|x| [1, 2, 3].contains(x)).is_none()` - let parent_expr = get_parent_expr_for_hir(self.cx, cmt.hir_id); - if let Some(Expr { hir_id: _, kind, .. }) = parent_expr { - if let ExprKind::Call(_, args) | ExprKind::MethodCall(_, _, args, _) = kind { - let args_to_handle = args.iter().filter(|arg| arg.hir_id == cmt.hir_id).collect::>(); - if !args_to_handle.is_empty() { - for arg in &args_to_handle { - let arg_ty_kind = self.cx.typeck_results().expr_ty(arg).kind(); - if matches!(arg_ty_kind, ty::Ref(_, _, Mutability::Not)) { - let start_span = Span::new(self.next_pos, span.lo(), span.ctxt()); - let start_snip = - snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability); + if let Some(parent_expr) = get_parent_expr_for_hir(self.cx, cmt.hir_id) { + if let ExprKind::Call(..) | ExprKind::MethodCall(..) = parent_expr.kind { + let expr = self.cx.tcx.hir().expect_expr(cmt.hir_id); + let arg_ty_kind = self.cx.typeck_results().expr_ty(expr).kind(); - self.suggestion_start.push_str(&format!("{}&{}", start_snip, ident_str)); - self.next_pos = span.hi(); - } else { - self.applicability = Applicability::Unspecified; - } - } - return; + // Note: this should always be true, as `find` only gives us a reference which are not mutable + if matches!(arg_ty_kind, ty::Ref(_, _, Mutability::Not)) { + let start_span = Span::new(self.next_pos, span.lo(), span.ctxt()); + let start_snip = + snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability); + + self.suggestion_start.push_str(&format!("{}&{}", start_snip, ident_str)); + self.next_pos = span.hi(); + } else { + self.applicability = Applicability::Unspecified; } + return; } } @@ -290,17 +287,17 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { }, _ => false, }, - ProjectionKind::Index => false, /* handled previously */ - // note: unable to capture `Subslice` kind in tests - ProjectionKind::Subslice => false, + // handled previously + ProjectionKind::Index | + // note: unable to trigger `Subslice` kind in tests + ProjectionKind::Subslice => false, ProjectionKind::Deref => { // explicit deref for arrays should be avoided in the suggestion // i.e.: `|sub| *sub[1..4].len() == 3` is not expected match cmt.place.ty_before_projection(i).kind() { // dereferencing an array (i.e.: `|sub| sub[1..4].len() == 3`) - ty::Ref(_, inner, _) => match inner.kind() { - ty::Ref(_, innermost, _) if innermost.is_array() => true, - _ => false, + ty::Ref(_, inner, _) => { + matches!(inner.kind(), ty::Ref(_, innermost, _) if innermost.is_array()) }, _ => false, } From ac45a83ad5df284a4680f0c3c90d86f7a177e41f Mon Sep 17 00:00:00 2001 From: ThibsG Date: Tue, 14 Sep 2021 14:53:28 +0200 Subject: [PATCH 12/25] Handle multiple reference levels into binding type and add more tests --- clippy_lints/src/methods/search_is_some.rs | 29 +++++++++++++++++---- tests/ui/search_is_some_fixable_none.fixed | 11 ++++++++ tests/ui/search_is_some_fixable_none.rs | 11 ++++++++ tests/ui/search_is_some_fixable_none.stderr | 14 +++++++++- tests/ui/search_is_some_fixable_some.fixed | 11 ++++++++ tests/ui/search_is_some_fixable_some.rs | 11 ++++++++ tests/ui/search_is_some_fixable_some.stderr | 14 +++++++++- 7 files changed, 94 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/methods/search_is_some.rs b/clippy_lints/src/methods/search_is_some.rs index a5e6a8df03f3..dca4d84f6b1f 100644 --- a/clippy_lints/src/methods/search_is_some.rs +++ b/clippy_lints/src/methods/search_is_some.rs @@ -50,12 +50,26 @@ pub(super) fn check<'tcx>( then { if let hir::PatKind::Ref(..) = closure_arg.pat.kind { Some(search_snippet.replacen('&', "", 1)) - } else if let PatKind::Binding(..) = strip_pat_refs(closure_arg.pat).kind { + } else if let PatKind::Binding(_, binding_id, _, _) = strip_pat_refs(closure_arg.pat).kind { + // this binding is composed of at least two levels of references, so we need to remove one + let binding_type = cx.typeck_results().node_type(binding_id); + let innermost_is_ref = if let ty::Ref(_, inner,_) = binding_type.kind() { + matches!(inner.kind(), ty::Ref(_, innermost, _) if innermost.is_ref()) + } else { + false + }; + // `find()` provides a reference to the item, but `any` does not, // so we should fix item usages for suggestion if let Some(closure_sugg) = get_closure_suggestion(cx, search_arg, closure_body) { applicability = closure_sugg.applicability; - Some(closure_sugg.suggestion) + if innermost_is_ref { + Some(closure_sugg.suggestion.replacen('&', "", 1)) + } else { + Some(closure_sugg.suggestion) + } + } else if innermost_is_ref { + Some(search_snippet.replacen('&', "", 1)) } else { Some(search_snippet.to_string()) } @@ -230,7 +244,7 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { // cases where a parent call is using the item // i.e.: suggest `.contains(&x)` for `.find(|x| [1, 2, 3].contains(x)).is_none()` if let Some(parent_expr) = get_parent_expr_for_hir(self.cx, cmt.hir_id) { - if let ExprKind::Call(..) | ExprKind::MethodCall(..) = parent_expr.kind { + if let ExprKind::Call(_, call_args) | ExprKind::MethodCall(_, _, call_args, _) = parent_expr.kind { let expr = self.cx.tcx.hir().expect_expr(cmt.hir_id); let arg_ty_kind = self.cx.typeck_results().expr_ty(expr).kind(); @@ -239,8 +253,13 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { let start_span = Span::new(self.next_pos, span.lo(), span.ctxt()); let start_snip = snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability); - - self.suggestion_start.push_str(&format!("{}&{}", start_snip, ident_str)); + // do not suggest ampersand if the ident is the method caller + let ident_sugg = if !call_args.is_empty() && call_args[0].hir_id == cmt.hir_id { + format!("{}{}", start_snip, ident_str) + } else { + format!("{}&{}", start_snip, ident_str) + }; + self.suggestion_start.push_str(&ident_sugg); self.next_pos = span.hi(); } else { self.applicability = Applicability::Unspecified; diff --git a/tests/ui/search_is_some_fixable_none.fixed b/tests/ui/search_is_some_fixable_none.fixed index 222731fd212a..811bb1e31769 100644 --- a/tests/ui/search_is_some_fixable_none.fixed +++ b/tests/ui/search_is_some_fixable_none.fixed @@ -97,4 +97,15 @@ mod issue7392 { let vfoo = vec![[0, 1, 2, 3, 0, 1, 2, 3]]; let _ = !vfoo.iter().any(|sub| sub[1..4].len() == 3); } + + fn please(x: &u32) -> bool { + *x == 9 + } + + fn more_projections() { + let x = 19; + let ppx: &u32 = &x; + let _ = ![ppx].iter().any(|ppp_x: &&u32| please(ppp_x)); + let _ = ![String::from("Hey hey")].iter().any(|s| s.len() == 2); + } } diff --git a/tests/ui/search_is_some_fixable_none.rs b/tests/ui/search_is_some_fixable_none.rs index 43ac1a076678..c6fbb5e2d261 100644 --- a/tests/ui/search_is_some_fixable_none.rs +++ b/tests/ui/search_is_some_fixable_none.rs @@ -101,4 +101,15 @@ mod issue7392 { let vfoo = vec![[0, 1, 2, 3, 0, 1, 2, 3]]; let _ = vfoo.iter().find(|sub| sub[1..4].len() == 3).is_none(); } + + fn please(x: &u32) -> bool { + *x == 9 + } + + fn more_projections() { + let x = 19; + let ppx: &u32 = &x; + let _ = [ppx].iter().find(|ppp_x: &&&u32| please(**ppp_x)).is_none(); + let _ = [String::from("Hey hey")].iter().find(|s| s.len() == 2).is_none(); + } } diff --git a/tests/ui/search_is_some_fixable_none.stderr b/tests/ui/search_is_some_fixable_none.stderr index fa344046aeba..b89ddabee554 100644 --- a/tests/ui/search_is_some_fixable_none.stderr +++ b/tests/ui/search_is_some_fixable_none.stderr @@ -169,5 +169,17 @@ error: called `is_none()` after searching an `Iterator` with `find` LL | let _ = vfoo.iter().find(|sub| sub[1..4].len() == 3).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!vfoo.iter().any(|sub| sub[1..4].len() == 3)` -error: aborting due to 26 previous errors +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:112:17 + | +LL | let _ = [ppx].iter().find(|ppp_x: &&&u32| please(**ppp_x)).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `![ppx].iter().any(|ppp_x: &&u32| please(ppp_x))` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:113:17 + | +LL | let _ = [String::from("Hey hey")].iter().find(|s| s.len() == 2).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `![String::from("Hey hey")].iter().any(|s| s.len() == 2)` + +error: aborting due to 28 previous errors diff --git a/tests/ui/search_is_some_fixable_some.fixed b/tests/ui/search_is_some_fixable_some.fixed index ae9478a42e9f..76b7f3d05d5a 100644 --- a/tests/ui/search_is_some_fixable_some.fixed +++ b/tests/ui/search_is_some_fixable_some.fixed @@ -98,4 +98,15 @@ mod issue7392 { let vfoo = vec![[0, 1, 2, 3, 0, 1, 2, 3]]; let _ = vfoo.iter().any(|sub| sub[1..4].len() == 3); } + + fn please(x: &u32) -> bool { + *x == 9 + } + + fn more_projections() { + let x = 19; + let ppx: &u32 = &x; + let _ = [ppx].iter().any(|ppp_x: &&u32| please(ppp_x)); + let _ = [String::from("Hey hey")].iter().any(|s| s.len() == 2); + } } diff --git a/tests/ui/search_is_some_fixable_some.rs b/tests/ui/search_is_some_fixable_some.rs index 98c3e7c1a685..364250e6ed81 100644 --- a/tests/ui/search_is_some_fixable_some.rs +++ b/tests/ui/search_is_some_fixable_some.rs @@ -100,4 +100,15 @@ mod issue7392 { let vfoo = vec![[0, 1, 2, 3, 0, 1, 2, 3]]; let _ = vfoo.iter().find(|sub| sub[1..4].len() == 3).is_some(); } + + fn please(x: &u32) -> bool { + *x == 9 + } + + fn more_projections() { + let x = 19; + let ppx: &u32 = &x; + let _ = [ppx].iter().find(|ppp_x: &&&u32| please(**ppp_x)).is_some(); + let _ = [String::from("Hey hey")].iter().find(|s| s.len() == 2).is_some(); + } } diff --git a/tests/ui/search_is_some_fixable_some.stderr b/tests/ui/search_is_some_fixable_some.stderr index 4161b5eec0a7..183d99497ac1 100644 --- a/tests/ui/search_is_some_fixable_some.stderr +++ b/tests/ui/search_is_some_fixable_some.stderr @@ -160,5 +160,17 @@ error: called `is_some()` after searching an `Iterator` with `find` LL | let _ = vfoo.iter().find(|sub| sub[1..4].len() == 3).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|sub| sub[1..4].len() == 3)` -error: aborting due to 26 previous errors +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:111:30 + | +LL | let _ = [ppx].iter().find(|ppp_x: &&&u32| please(**ppp_x)).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|ppp_x: &&u32| please(ppp_x))` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:112:50 + | +LL | let _ = [String::from("Hey hey")].iter().find(|s| s.len() == 2).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|s| s.len() == 2)` + +error: aborting due to 28 previous errors From 6d1ccbf46682b99f0301488bfffff403e824b4d7 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Sun, 19 Sep 2021 08:08:54 +0200 Subject: [PATCH 13/25] Correct suggestion when dereferencing enough, calling a function --- clippy_lints/src/methods/search_is_some.rs | 3 +-- tests/ui/search_is_some_fixable_none.fixed | 7 +++++++ tests/ui/search_is_some_fixable_none.rs | 7 +++++++ tests/ui/search_is_some_fixable_none.stderr | 12 +++++++++--- tests/ui/search_is_some_fixable_some.fixed | 7 +++++++ tests/ui/search_is_some_fixable_some.rs | 7 +++++++ tests/ui/search_is_some_fixable_some.stderr | 12 +++++++++--- 7 files changed, 47 insertions(+), 8 deletions(-) diff --git a/clippy_lints/src/methods/search_is_some.rs b/clippy_lints/src/methods/search_is_some.rs index dca4d84f6b1f..98ad7f3866fd 100644 --- a/clippy_lints/src/methods/search_is_some.rs +++ b/clippy_lints/src/methods/search_is_some.rs @@ -248,7 +248,6 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { let expr = self.cx.tcx.hir().expect_expr(cmt.hir_id); let arg_ty_kind = self.cx.typeck_results().expr_ty(expr).kind(); - // Note: this should always be true, as `find` only gives us a reference which are not mutable if matches!(arg_ty_kind, ty::Ref(_, _, Mutability::Not)) { let start_span = Span::new(self.next_pos, span.lo(), span.ctxt()); let start_snip = @@ -261,10 +260,10 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { }; self.suggestion_start.push_str(&ident_sugg); self.next_pos = span.hi(); + return; } else { self.applicability = Applicability::Unspecified; } - return; } } diff --git a/tests/ui/search_is_some_fixable_none.fixed b/tests/ui/search_is_some_fixable_none.fixed index 811bb1e31769..c7c0b7660154 100644 --- a/tests/ui/search_is_some_fixable_none.fixed +++ b/tests/ui/search_is_some_fixable_none.fixed @@ -102,10 +102,17 @@ mod issue7392 { *x == 9 } + fn simple_fn(x: u32) -> bool { + x == 78 + } + fn more_projections() { let x = 19; let ppx: &u32 = &x; let _ = ![ppx].iter().any(|ppp_x: &&u32| please(ppp_x)); let _ = ![String::from("Hey hey")].iter().any(|s| s.len() == 2); + + let v = vec![3, 2, 1, 0]; + let _ = !v.iter().any(|x| simple_fn(*x)); } } diff --git a/tests/ui/search_is_some_fixable_none.rs b/tests/ui/search_is_some_fixable_none.rs index c6fbb5e2d261..6b41da4f2f86 100644 --- a/tests/ui/search_is_some_fixable_none.rs +++ b/tests/ui/search_is_some_fixable_none.rs @@ -106,10 +106,17 @@ mod issue7392 { *x == 9 } + fn simple_fn(x: u32) -> bool { + x == 78 + } + fn more_projections() { let x = 19; let ppx: &u32 = &x; let _ = [ppx].iter().find(|ppp_x: &&&u32| please(**ppp_x)).is_none(); let _ = [String::from("Hey hey")].iter().find(|s| s.len() == 2).is_none(); + + let v = vec![3, 2, 1, 0]; + let _ = v.iter().find(|x| simple_fn(**x)).is_none(); } } diff --git a/tests/ui/search_is_some_fixable_none.stderr b/tests/ui/search_is_some_fixable_none.stderr index b89ddabee554..4313a2d526c7 100644 --- a/tests/ui/search_is_some_fixable_none.stderr +++ b/tests/ui/search_is_some_fixable_none.stderr @@ -170,16 +170,22 @@ LL | let _ = vfoo.iter().find(|sub| sub[1..4].len() == 3).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!vfoo.iter().any(|sub| sub[1..4].len() == 3)` error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable_none.rs:112:17 + --> $DIR/search_is_some_fixable_none.rs:116:17 | LL | let _ = [ppx].iter().find(|ppp_x: &&&u32| please(**ppp_x)).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `![ppx].iter().any(|ppp_x: &&u32| please(ppp_x))` error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable_none.rs:113:17 + --> $DIR/search_is_some_fixable_none.rs:117:17 | LL | let _ = [String::from("Hey hey")].iter().find(|s| s.len() == 2).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `![String::from("Hey hey")].iter().any(|s| s.len() == 2)` -error: aborting due to 28 previous errors +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:120:17 + | +LL | let _ = v.iter().find(|x| simple_fn(**x)).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|x| simple_fn(*x))` + +error: aborting due to 29 previous errors diff --git a/tests/ui/search_is_some_fixable_some.fixed b/tests/ui/search_is_some_fixable_some.fixed index 76b7f3d05d5a..833168cf2422 100644 --- a/tests/ui/search_is_some_fixable_some.fixed +++ b/tests/ui/search_is_some_fixable_some.fixed @@ -103,10 +103,17 @@ mod issue7392 { *x == 9 } + fn simple_fn(x: u32) -> bool { + x == 78 + } + fn more_projections() { let x = 19; let ppx: &u32 = &x; let _ = [ppx].iter().any(|ppp_x: &&u32| please(ppp_x)); let _ = [String::from("Hey hey")].iter().any(|s| s.len() == 2); + + let v = vec![3, 2, 1, 0]; + let _ = v.iter().any(|x| simple_fn(*x)); } } diff --git a/tests/ui/search_is_some_fixable_some.rs b/tests/ui/search_is_some_fixable_some.rs index 364250e6ed81..f9fb241de3f6 100644 --- a/tests/ui/search_is_some_fixable_some.rs +++ b/tests/ui/search_is_some_fixable_some.rs @@ -105,10 +105,17 @@ mod issue7392 { *x == 9 } + fn simple_fn(x: u32) -> bool { + x == 78 + } + fn more_projections() { let x = 19; let ppx: &u32 = &x; let _ = [ppx].iter().find(|ppp_x: &&&u32| please(**ppp_x)).is_some(); let _ = [String::from("Hey hey")].iter().find(|s| s.len() == 2).is_some(); + + let v = vec![3, 2, 1, 0]; + let _ = v.iter().find(|x| simple_fn(**x)).is_some(); } } diff --git a/tests/ui/search_is_some_fixable_some.stderr b/tests/ui/search_is_some_fixable_some.stderr index 183d99497ac1..01b13cea2cd7 100644 --- a/tests/ui/search_is_some_fixable_some.stderr +++ b/tests/ui/search_is_some_fixable_some.stderr @@ -161,16 +161,22 @@ LL | let _ = vfoo.iter().find(|sub| sub[1..4].len() == 3).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|sub| sub[1..4].len() == 3)` error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable_some.rs:111:30 + --> $DIR/search_is_some_fixable_some.rs:115:30 | LL | let _ = [ppx].iter().find(|ppp_x: &&&u32| please(**ppp_x)).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|ppp_x: &&u32| please(ppp_x))` error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable_some.rs:112:50 + --> $DIR/search_is_some_fixable_some.rs:116:50 | LL | let _ = [String::from("Hey hey")].iter().find(|s| s.len() == 2).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|s| s.len() == 2)` -error: aborting due to 28 previous errors +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:119:26 + | +LL | let _ = v.iter().find(|x| simple_fn(**x)).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| simple_fn(*x))` + +error: aborting due to 29 previous errors From 7a55407cc37ddb1bf1d3d95e52db7b6402259966 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Tue, 21 Sep 2021 14:54:19 +0200 Subject: [PATCH 14/25] Fix suggestions when call functions involved taking by ref --- clippy_lints/src/methods/search_is_some.rs | 93 +++++++++++++-------- tests/ui/search_is_some_fixable_none.fixed | 50 ++++++++++- tests/ui/search_is_some_fixable_none.rs | 52 +++++++++++- tests/ui/search_is_some_fixable_none.stderr | 46 ++++++++-- tests/ui/search_is_some_fixable_some.fixed | 51 ++++++++++- tests/ui/search_is_some_fixable_some.rs | 52 +++++++++++- tests/ui/search_is_some_fixable_some.stderr | 38 +++++++-- 7 files changed, 325 insertions(+), 57 deletions(-) diff --git a/clippy_lints/src/methods/search_is_some.rs b/clippy_lints/src/methods/search_is_some.rs index 98ad7f3866fd..25a2c48e2699 100644 --- a/clippy_lints/src/methods/search_is_some.rs +++ b/clippy_lints/src/methods/search_is_some.rs @@ -1,3 +1,5 @@ +use std::iter; + use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg}; use clippy_utils::source::{snippet, snippet_with_applicability}; use clippy_utils::ty::is_type_diagnostic_item; @@ -223,6 +225,26 @@ impl DerefDelegate<'_, 'tcx> { let end_snip = snippet_with_applicability(self.cx, end_span, "..", &mut self.applicability); format!("{}{}", self.suggestion_start, end_snip) } + + fn func_takes_arg_by_ref(&self, parent_expr: &'tcx hir::Expr<'_>, cmt_hir_id: HirId) -> bool { + if_chain! { + if let ExprKind::Call(func, call_args) = parent_expr.kind; + let typ = self.cx.typeck_results().expr_ty(func); + if let ty::FnDef(..) = typ.kind(); + + then { + let mut takes_by_ref = false; + for (arg, ty) in iter::zip(call_args, typ.fn_sig(self.cx.tcx).skip_binder().inputs()) { + if arg.hir_id == cmt_hir_id { + takes_by_ref = matches!(ty.kind(), ty::Ref(_, inner, _) if inner.is_ref()); + } + } + takes_by_ref + } else { + false + } + } + } } impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { @@ -252,18 +274,23 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { let start_span = Span::new(self.next_pos, span.lo(), span.ctxt()); let start_snip = snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability); + + // suggest ampersand if call function is taking args by ref + let takes_arg_by_ref = self.func_takes_arg_by_ref(parent_expr, cmt.hir_id); + // do not suggest ampersand if the ident is the method caller - let ident_sugg = if !call_args.is_empty() && call_args[0].hir_id == cmt.hir_id { - format!("{}{}", start_snip, ident_str) - } else { - format!("{}&{}", start_snip, ident_str) - }; + let ident_sugg = + if !call_args.is_empty() && call_args[0].hir_id == cmt.hir_id && !takes_arg_by_ref { + format!("{}{}", start_snip, ident_str) + } else { + format!("{}&{}", start_snip, ident_str) + }; self.suggestion_start.push_str(&ident_sugg); self.next_pos = span.hi(); return; - } else { - self.applicability = Applicability::Unspecified; } + + self.applicability = Applicability::Unspecified; } } @@ -271,23 +298,8 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { // i.e.: suggest `*x` instead of `**x` let mut replacement_str = ident_str; - // handle index projection first - let index_handled = cmt.place.projections.iter().any(|proj| match proj.kind { - // Index projection like `|x| foo[x]` - // the index is dropped so we can't get it to build the suggestion, - // so the span is set-up again to get more code, using `span.hi()` (i.e.: `foo[x]`) - // instead of `span.lo()` (i.e.: `foo`) - ProjectionKind::Index => { - let start_span = Span::new(self.next_pos, span.hi(), span.ctxt()); - start_snip = snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability); - replacement_str.clear(); - true - }, - _ => false, - }); - - // looking for projections other that need to be handled differently - let other_projections_handled = cmt.place.projections.iter().enumerate().any(|(i, proj)| { + let mut projections_handled = false; + cmt.place.projections.iter().enumerate().for_each(|(i, proj)| { match proj.kind { // Field projection like `|v| v.foo` ProjectionKind::Field(idx, variant) => match cmt.place.ty_before_projection(i).kind() { @@ -297,34 +309,41 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { replacement_str, def.variants[variant].fields[idx as usize].ident.name.as_str() ); - true + projections_handled = true; }, ty::Tuple(_) => { replacement_str = format!("{}.{}", replacement_str, idx); - true + projections_handled = true; }, - _ => false, + _ => (), + }, + // Index projection like `|x| foo[x]` + // the index is dropped so we can't get it to build the suggestion, + // so the span is set-up again to get more code, using `span.hi()` (i.e.: `foo[x]`) + // instead of `span.lo()` (i.e.: `foo`) + ProjectionKind::Index => { + let start_span = Span::new(self.next_pos, span.hi(), span.ctxt()); + start_snip = snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability); + replacement_str.clear(); + projections_handled = true; }, - // handled previously - ProjectionKind::Index | - // note: unable to trigger `Subslice` kind in tests - ProjectionKind::Subslice => false, + // note: unable to trigger `Subslice` kind in tests + ProjectionKind::Subslice => (), ProjectionKind::Deref => { // explicit deref for arrays should be avoided in the suggestion // i.e.: `|sub| *sub[1..4].len() == 3` is not expected - match cmt.place.ty_before_projection(i).kind() { + if let ty::Ref(_, inner, _) = cmt.place.ty_before_projection(i).kind() { // dereferencing an array (i.e.: `|sub| sub[1..4].len() == 3`) - ty::Ref(_, inner, _) => { - matches!(inner.kind(), ty::Ref(_, innermost, _) if innermost.is_array()) - }, - _ => false, + if matches!(inner.kind(), ty::Ref(_, innermost, _) if innermost.is_array()) { + projections_handled = true; + } } }, } }); // handle `ProjectionKind::Deref` if no special case detected - if !index_handled && !other_projections_handled { + if !projections_handled { let last_deref = cmt .place .projections diff --git a/tests/ui/search_is_some_fixable_none.fixed b/tests/ui/search_is_some_fixable_none.fixed index c7c0b7660154..6b8a614bbdaa 100644 --- a/tests/ui/search_is_some_fixable_none.fixed +++ b/tests/ui/search_is_some_fixable_none.fixed @@ -102,10 +102,14 @@ mod issue7392 { *x == 9 } - fn simple_fn(x: u32) -> bool { + fn deref_enough(x: u32) -> bool { x == 78 } + fn arg_no_deref(x: &&u32) -> bool { + **x == 78 + } + fn more_projections() { let x = 19; let ppx: &u32 = &x; @@ -113,6 +117,48 @@ mod issue7392 { let _ = ![String::from("Hey hey")].iter().any(|s| s.len() == 2); let v = vec![3, 2, 1, 0]; - let _ = !v.iter().any(|x| simple_fn(*x)); + let _ = !v.iter().any(|x| deref_enough(*x)); + + #[allow(clippy::redundant_closure)] + let _ = !v.iter().any(|x| arg_no_deref(&x)); + } + + fn field_index_projection() { + struct FooDouble { + bar: Vec>, + } + struct Foo { + bar: Vec, + } + struct FooOuter { + inner: Foo, + inner_double: FooDouble, + } + let vfoo = vec![FooOuter { + inner: Foo { bar: vec![0, 1, 2, 3] }, + inner_double: FooDouble { + bar: vec![vec![0, 1, 2, 3]], + }, + }]; + let _ = !vfoo + .iter().any(|v| v.inner_double.bar[0][0] == 2 && v.inner.bar[0] == 2); + } + + fn index_field_projection() { + struct Foo { + bar: i32, + } + struct FooOuter { + inner: Vec, + } + let vfoo = vec![FooOuter { + inner: vec![Foo { bar: 0 }], + }]; + let _ = !vfoo.iter().any(|v| v.inner[0].bar == 2); + } + + fn double_deref_index_projection() { + let vfoo = vec![&&[0, 1, 2, 3]]; + let _ = !vfoo.iter().any(|x| (**x)[0] == 9); } } diff --git a/tests/ui/search_is_some_fixable_none.rs b/tests/ui/search_is_some_fixable_none.rs index 6b41da4f2f86..acaae650921f 100644 --- a/tests/ui/search_is_some_fixable_none.rs +++ b/tests/ui/search_is_some_fixable_none.rs @@ -106,10 +106,14 @@ mod issue7392 { *x == 9 } - fn simple_fn(x: u32) -> bool { + fn deref_enough(x: u32) -> bool { x == 78 } + fn arg_no_deref(x: &&u32) -> bool { + **x == 78 + } + fn more_projections() { let x = 19; let ppx: &u32 = &x; @@ -117,6 +121,50 @@ mod issue7392 { let _ = [String::from("Hey hey")].iter().find(|s| s.len() == 2).is_none(); let v = vec![3, 2, 1, 0]; - let _ = v.iter().find(|x| simple_fn(**x)).is_none(); + let _ = v.iter().find(|x| deref_enough(**x)).is_none(); + + #[allow(clippy::redundant_closure)] + let _ = v.iter().find(|x| arg_no_deref(x)).is_none(); + } + + fn field_index_projection() { + struct FooDouble { + bar: Vec>, + } + struct Foo { + bar: Vec, + } + struct FooOuter { + inner: Foo, + inner_double: FooDouble, + } + let vfoo = vec![FooOuter { + inner: Foo { bar: vec![0, 1, 2, 3] }, + inner_double: FooDouble { + bar: vec![vec![0, 1, 2, 3]], + }, + }]; + let _ = vfoo + .iter() + .find(|v| v.inner_double.bar[0][0] == 2 && v.inner.bar[0] == 2) + .is_none(); + } + + fn index_field_projection() { + struct Foo { + bar: i32, + } + struct FooOuter { + inner: Vec, + } + let vfoo = vec![FooOuter { + inner: vec![Foo { bar: 0 }], + }]; + let _ = vfoo.iter().find(|v| v.inner[0].bar == 2).is_none(); + } + + fn double_deref_index_projection() { + let vfoo = vec![&&[0, 1, 2, 3]]; + let _ = vfoo.iter().find(|x| (**x)[0] == 9).is_none(); } } diff --git a/tests/ui/search_is_some_fixable_none.stderr b/tests/ui/search_is_some_fixable_none.stderr index 4313a2d526c7..64e81c6b94a2 100644 --- a/tests/ui/search_is_some_fixable_none.stderr +++ b/tests/ui/search_is_some_fixable_none.stderr @@ -170,22 +170,56 @@ LL | let _ = vfoo.iter().find(|sub| sub[1..4].len() == 3).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!vfoo.iter().any(|sub| sub[1..4].len() == 3)` error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable_none.rs:116:17 + --> $DIR/search_is_some_fixable_none.rs:120:17 | LL | let _ = [ppx].iter().find(|ppp_x: &&&u32| please(**ppp_x)).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `![ppx].iter().any(|ppp_x: &&u32| please(ppp_x))` error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable_none.rs:117:17 + --> $DIR/search_is_some_fixable_none.rs:121:17 | LL | let _ = [String::from("Hey hey")].iter().find(|s| s.len() == 2).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `![String::from("Hey hey")].iter().any(|s| s.len() == 2)` error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable_none.rs:120:17 + --> $DIR/search_is_some_fixable_none.rs:124:17 + | +LL | let _ = v.iter().find(|x| deref_enough(**x)).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|x| deref_enough(*x))` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:127:17 + | +LL | let _ = v.iter().find(|x| arg_no_deref(x)).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|x| arg_no_deref(&x))` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:147:17 + | +LL | let _ = vfoo + | _________________^ +LL | | .iter() +LL | | .find(|v| v.inner_double.bar[0][0] == 2 && v.inner.bar[0] == 2) +LL | | .is_none(); + | |______________________^ + | +help: use `!_.any()` instead + | +LL ~ let _ = !vfoo +LL ~ .iter().any(|v| v.inner_double.bar[0][0] == 2 && v.inner.bar[0] == 2); + | + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:163:17 + | +LL | let _ = vfoo.iter().find(|v| v.inner[0].bar == 2).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!vfoo.iter().any(|v| v.inner[0].bar == 2)` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:168:17 | -LL | let _ = v.iter().find(|x| simple_fn(**x)).is_none(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|x| simple_fn(*x))` +LL | let _ = vfoo.iter().find(|x| (**x)[0] == 9).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!vfoo.iter().any(|x| (**x)[0] == 9)` -error: aborting due to 29 previous errors +error: aborting due to 33 previous errors diff --git a/tests/ui/search_is_some_fixable_some.fixed b/tests/ui/search_is_some_fixable_some.fixed index 833168cf2422..7eb07d639a56 100644 --- a/tests/ui/search_is_some_fixable_some.fixed +++ b/tests/ui/search_is_some_fixable_some.fixed @@ -103,10 +103,14 @@ mod issue7392 { *x == 9 } - fn simple_fn(x: u32) -> bool { + fn deref_enough(x: u32) -> bool { x == 78 } + fn arg_no_deref(x: &&u32) -> bool { + **x == 78 + } + fn more_projections() { let x = 19; let ppx: &u32 = &x; @@ -114,6 +118,49 @@ mod issue7392 { let _ = [String::from("Hey hey")].iter().any(|s| s.len() == 2); let v = vec![3, 2, 1, 0]; - let _ = v.iter().any(|x| simple_fn(*x)); + let _ = v.iter().any(|x| deref_enough(*x)); + + #[allow(clippy::redundant_closure)] + let _ = v.iter().any(|x| arg_no_deref(&x)); + } + + fn field_index_projection() { + struct FooDouble { + bar: Vec>, + } + struct Foo { + bar: Vec, + } + struct FooOuter { + inner: Foo, + inner_double: FooDouble, + } + let vfoo = vec![FooOuter { + inner: Foo { bar: vec![0, 1, 2, 3] }, + inner_double: FooDouble { + bar: vec![vec![0, 1, 2, 3]], + }, + }]; + let _ = vfoo + .iter() + .any(|v| v.inner_double.bar[0][0] == 2 && v.inner.bar[0] == 2); + } + + fn index_field_projection() { + struct Foo { + bar: i32, + } + struct FooOuter { + inner: Vec, + } + let vfoo = vec![FooOuter { + inner: vec![Foo { bar: 0 }], + }]; + let _ = vfoo.iter().any(|v| v.inner[0].bar == 2); + } + + fn double_deref_index_projection() { + let vfoo = vec![&&[0, 1, 2, 3]]; + let _ = vfoo.iter().any(|x| (**x)[0] == 9); } } diff --git a/tests/ui/search_is_some_fixable_some.rs b/tests/ui/search_is_some_fixable_some.rs index f9fb241de3f6..b9ebd399a350 100644 --- a/tests/ui/search_is_some_fixable_some.rs +++ b/tests/ui/search_is_some_fixable_some.rs @@ -105,10 +105,14 @@ mod issue7392 { *x == 9 } - fn simple_fn(x: u32) -> bool { + fn deref_enough(x: u32) -> bool { x == 78 } + fn arg_no_deref(x: &&u32) -> bool { + **x == 78 + } + fn more_projections() { let x = 19; let ppx: &u32 = &x; @@ -116,6 +120,50 @@ mod issue7392 { let _ = [String::from("Hey hey")].iter().find(|s| s.len() == 2).is_some(); let v = vec![3, 2, 1, 0]; - let _ = v.iter().find(|x| simple_fn(**x)).is_some(); + let _ = v.iter().find(|x| deref_enough(**x)).is_some(); + + #[allow(clippy::redundant_closure)] + let _ = v.iter().find(|x| arg_no_deref(x)).is_some(); + } + + fn field_index_projection() { + struct FooDouble { + bar: Vec>, + } + struct Foo { + bar: Vec, + } + struct FooOuter { + inner: Foo, + inner_double: FooDouble, + } + let vfoo = vec![FooOuter { + inner: Foo { bar: vec![0, 1, 2, 3] }, + inner_double: FooDouble { + bar: vec![vec![0, 1, 2, 3]], + }, + }]; + let _ = vfoo + .iter() + .find(|v| v.inner_double.bar[0][0] == 2 && v.inner.bar[0] == 2) + .is_some(); + } + + fn index_field_projection() { + struct Foo { + bar: i32, + } + struct FooOuter { + inner: Vec, + } + let vfoo = vec![FooOuter { + inner: vec![Foo { bar: 0 }], + }]; + let _ = vfoo.iter().find(|v| v.inner[0].bar == 2).is_some(); + } + + fn double_deref_index_projection() { + let vfoo = vec![&&[0, 1, 2, 3]]; + let _ = vfoo.iter().find(|x| (**x)[0] == 9).is_some(); } } diff --git a/tests/ui/search_is_some_fixable_some.stderr b/tests/ui/search_is_some_fixable_some.stderr index 01b13cea2cd7..b62cf9c47029 100644 --- a/tests/ui/search_is_some_fixable_some.stderr +++ b/tests/ui/search_is_some_fixable_some.stderr @@ -161,22 +161,48 @@ LL | let _ = vfoo.iter().find(|sub| sub[1..4].len() == 3).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|sub| sub[1..4].len() == 3)` error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable_some.rs:115:30 + --> $DIR/search_is_some_fixable_some.rs:119:30 | LL | let _ = [ppx].iter().find(|ppp_x: &&&u32| please(**ppp_x)).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|ppp_x: &&u32| please(ppp_x))` error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable_some.rs:116:50 + --> $DIR/search_is_some_fixable_some.rs:120:50 | LL | let _ = [String::from("Hey hey")].iter().find(|s| s.len() == 2).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|s| s.len() == 2)` error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable_some.rs:119:26 + --> $DIR/search_is_some_fixable_some.rs:123:26 | -LL | let _ = v.iter().find(|x| simple_fn(**x)).is_some(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| simple_fn(*x))` +LL | let _ = v.iter().find(|x| deref_enough(**x)).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| deref_enough(*x))` -error: aborting due to 29 previous errors +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:126:26 + | +LL | let _ = v.iter().find(|x| arg_no_deref(x)).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| arg_no_deref(&x))` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:148:14 + | +LL | .find(|v| v.inner_double.bar[0][0] == 2 && v.inner.bar[0] == 2) + | ______________^ +LL | | .is_some(); + | |______________________^ help: use `any()` instead: `any(|v| v.inner_double.bar[0][0] == 2 && v.inner.bar[0] == 2)` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:162:29 + | +LL | let _ = vfoo.iter().find(|v| v.inner[0].bar == 2).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|v| v.inner[0].bar == 2)` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:167:29 + | +LL | let _ = vfoo.iter().find(|x| (**x)[0] == 9).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| (**x)[0] == 9)` + +error: aborting due to 33 previous errors From 90a72f506c55b2c2fc1f64b13c9c048a65846bba Mon Sep 17 00:00:00 2001 From: ThibsG Date: Fri, 1 Oct 2021 16:57:57 +0200 Subject: [PATCH 15/25] Handle args taken by ref also for `MethodCall` --- clippy_lints/src/methods/search_is_some.rs | 43 +++++++++------------ tests/ui/search_is_some_fixable_none.fixed | 13 +++++++ tests/ui/search_is_some_fixable_none.rs | 13 +++++++ tests/ui/search_is_some_fixable_none.stderr | 8 +++- tests/ui/search_is_some_fixable_some.fixed | 13 +++++++ tests/ui/search_is_some_fixable_some.rs | 13 +++++++ tests/ui/search_is_some_fixable_some.stderr | 8 +++- 7 files changed, 85 insertions(+), 26 deletions(-) diff --git a/clippy_lints/src/methods/search_is_some.rs b/clippy_lints/src/methods/search_is_some.rs index 25a2c48e2699..4f8517b946a5 100644 --- a/clippy_lints/src/methods/search_is_some.rs +++ b/clippy_lints/src/methods/search_is_some.rs @@ -227,23 +227,23 @@ impl DerefDelegate<'_, 'tcx> { } fn func_takes_arg_by_ref(&self, parent_expr: &'tcx hir::Expr<'_>, cmt_hir_id: HirId) -> bool { - if_chain! { - if let ExprKind::Call(func, call_args) = parent_expr.kind; - let typ = self.cx.typeck_results().expr_ty(func); - if let ty::FnDef(..) = typ.kind(); - - then { - let mut takes_by_ref = false; - for (arg, ty) in iter::zip(call_args, typ.fn_sig(self.cx.tcx).skip_binder().inputs()) { - if arg.hir_id == cmt_hir_id { - takes_by_ref = matches!(ty.kind(), ty::Ref(_, inner, _) if inner.is_ref()); - } + let (call_args, inputs) = match parent_expr.kind { + ExprKind::MethodCall(_, _, call_args, _) => { + if let Some(method_did) = self.cx.typeck_results().type_dependent_def_id(parent_expr.hir_id) { + (call_args, self.cx.tcx.fn_sig(method_did).skip_binder().inputs()) + } else { + return false; } - takes_by_ref - } else { - false - } - } + }, + ExprKind::Call(func, call_args) => { + let typ = self.cx.typeck_results().expr_ty(func); + (call_args, typ.fn_sig(self.cx.tcx).skip_binder().inputs()) + }, + _ => return false, + }; + + iter::zip(call_args, inputs) + .any(|(arg, ty)| arg.hir_id == cmt_hir_id && matches!(ty.kind(), ty::Ref(_, inner, _) if inner.is_ref())) } } @@ -271,10 +271,6 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { let arg_ty_kind = self.cx.typeck_results().expr_ty(expr).kind(); if matches!(arg_ty_kind, ty::Ref(_, _, Mutability::Not)) { - let start_span = Span::new(self.next_pos, span.lo(), span.ctxt()); - let start_snip = - snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability); - // suggest ampersand if call function is taking args by ref let takes_arg_by_ref = self.func_takes_arg_by_ref(parent_expr, cmt.hir_id); @@ -294,14 +290,12 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { } } - // handle item projections by removing one explicit deref - // i.e.: suggest `*x` instead of `**x` let mut replacement_str = ident_str; - let mut projections_handled = false; cmt.place.projections.iter().enumerate().for_each(|(i, proj)| { match proj.kind { // Field projection like `|v| v.foo` + // no adjustment needed here, as field projections are handled by the compiler ProjectionKind::Field(idx, variant) => match cmt.place.ty_before_projection(i).kind() { ty::Adt(def, ..) => { replacement_str = format!( @@ -342,7 +336,8 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { } }); - // handle `ProjectionKind::Deref` if no special case detected + // handle `ProjectionKind::Deref` by removing one explicit deref + // if no special case was detected (i.e.: suggest `*x` instead of `**x`) if !projections_handled { let last_deref = cmt .place diff --git a/tests/ui/search_is_some_fixable_none.fixed b/tests/ui/search_is_some_fixable_none.fixed index 6b8a614bbdaa..0d1ed343c0c0 100644 --- a/tests/ui/search_is_some_fixable_none.fixed +++ b/tests/ui/search_is_some_fixable_none.fixed @@ -161,4 +161,17 @@ mod issue7392 { let vfoo = vec![&&[0, 1, 2, 3]]; let _ = !vfoo.iter().any(|x| (**x)[0] == 9); } + + fn method_call_by_ref() { + struct Foo { + bar: u32, + } + impl Foo { + pub fn by_ref(&self, x: &u32) -> bool { + *x == self.bar + } + } + let vfoo = vec![Foo { bar: 1 }]; + let _ = !vfoo.iter().any(|v| v.by_ref(&v.bar)); + } } diff --git a/tests/ui/search_is_some_fixable_none.rs b/tests/ui/search_is_some_fixable_none.rs index acaae650921f..2c638d929cb4 100644 --- a/tests/ui/search_is_some_fixable_none.rs +++ b/tests/ui/search_is_some_fixable_none.rs @@ -167,4 +167,17 @@ mod issue7392 { let vfoo = vec![&&[0, 1, 2, 3]]; let _ = vfoo.iter().find(|x| (**x)[0] == 9).is_none(); } + + fn method_call_by_ref() { + struct Foo { + bar: u32, + } + impl Foo { + pub fn by_ref(&self, x: &u32) -> bool { + *x == self.bar + } + } + let vfoo = vec![Foo { bar: 1 }]; + let _ = vfoo.iter().find(|v| v.by_ref(&v.bar)).is_none(); + } } diff --git a/tests/ui/search_is_some_fixable_none.stderr b/tests/ui/search_is_some_fixable_none.stderr index 64e81c6b94a2..0c34067c1394 100644 --- a/tests/ui/search_is_some_fixable_none.stderr +++ b/tests/ui/search_is_some_fixable_none.stderr @@ -221,5 +221,11 @@ error: called `is_none()` after searching an `Iterator` with `find` LL | let _ = vfoo.iter().find(|x| (**x)[0] == 9).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!vfoo.iter().any(|x| (**x)[0] == 9)` -error: aborting due to 33 previous errors +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:181:17 + | +LL | let _ = vfoo.iter().find(|v| v.by_ref(&v.bar)).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!vfoo.iter().any(|v| v.by_ref(&v.bar))` + +error: aborting due to 34 previous errors diff --git a/tests/ui/search_is_some_fixable_some.fixed b/tests/ui/search_is_some_fixable_some.fixed index 7eb07d639a56..43471e354939 100644 --- a/tests/ui/search_is_some_fixable_some.fixed +++ b/tests/ui/search_is_some_fixable_some.fixed @@ -163,4 +163,17 @@ mod issue7392 { let vfoo = vec![&&[0, 1, 2, 3]]; let _ = vfoo.iter().any(|x| (**x)[0] == 9); } + + fn method_call_by_ref() { + struct Foo { + bar: u32, + } + impl Foo { + pub fn by_ref(&self, x: &u32) -> bool { + *x == self.bar + } + } + let vfoo = vec![Foo { bar: 1 }]; + let _ = vfoo.iter().any(|v| v.by_ref(&v.bar)); + } } diff --git a/tests/ui/search_is_some_fixable_some.rs b/tests/ui/search_is_some_fixable_some.rs index b9ebd399a350..82d3072d2f39 100644 --- a/tests/ui/search_is_some_fixable_some.rs +++ b/tests/ui/search_is_some_fixable_some.rs @@ -166,4 +166,17 @@ mod issue7392 { let vfoo = vec![&&[0, 1, 2, 3]]; let _ = vfoo.iter().find(|x| (**x)[0] == 9).is_some(); } + + fn method_call_by_ref() { + struct Foo { + bar: u32, + } + impl Foo { + pub fn by_ref(&self, x: &u32) -> bool { + *x == self.bar + } + } + let vfoo = vec![Foo { bar: 1 }]; + let _ = vfoo.iter().find(|v| v.by_ref(&v.bar)).is_some(); + } } diff --git a/tests/ui/search_is_some_fixable_some.stderr b/tests/ui/search_is_some_fixable_some.stderr index b62cf9c47029..5e8885991ac0 100644 --- a/tests/ui/search_is_some_fixable_some.stderr +++ b/tests/ui/search_is_some_fixable_some.stderr @@ -204,5 +204,11 @@ error: called `is_some()` after searching an `Iterator` with `find` LL | let _ = vfoo.iter().find(|x| (**x)[0] == 9).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| (**x)[0] == 9)` -error: aborting due to 33 previous errors +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:180:29 + | +LL | let _ = vfoo.iter().find(|v| v.by_ref(&v.bar)).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|v| v.by_ref(&v.bar))` + +error: aborting due to 34 previous errors From 268ef407a65ba07601f8cd70bf40537d76376aa9 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Fri, 1 Oct 2021 17:08:22 +0200 Subject: [PATCH 16/25] Add test case for references bindings --- tests/ui/search_is_some_fixable_none.fixed | 5 +++++ tests/ui/search_is_some_fixable_none.rs | 5 +++++ tests/ui/search_is_some_fixable_none.stderr | 14 +++++++++++++- tests/ui/search_is_some_fixable_some.fixed | 5 +++++ tests/ui/search_is_some_fixable_some.rs | 5 +++++ tests/ui/search_is_some_fixable_some.stderr | 14 +++++++++++++- 6 files changed, 46 insertions(+), 2 deletions(-) diff --git a/tests/ui/search_is_some_fixable_none.fixed b/tests/ui/search_is_some_fixable_none.fixed index 0d1ed343c0c0..240a69578931 100644 --- a/tests/ui/search_is_some_fixable_none.fixed +++ b/tests/ui/search_is_some_fixable_none.fixed @@ -174,4 +174,9 @@ mod issue7392 { let vfoo = vec![Foo { bar: 1 }]; let _ = !vfoo.iter().any(|v| v.by_ref(&v.bar)); } + + fn ref_bindings() { + let _ = ![&(&1, 2), &(&3, 4), &(&5, 4)].iter().any(|(&x, y)| x == *y); + let _ = ![&(&1, 2), &(&3, 4), &(&5, 4)].iter().any(|(&x, y)| x == *y); + } } diff --git a/tests/ui/search_is_some_fixable_none.rs b/tests/ui/search_is_some_fixable_none.rs index 2c638d929cb4..7e8fc14a7bd4 100644 --- a/tests/ui/search_is_some_fixable_none.rs +++ b/tests/ui/search_is_some_fixable_none.rs @@ -180,4 +180,9 @@ mod issue7392 { let vfoo = vec![Foo { bar: 1 }]; let _ = vfoo.iter().find(|v| v.by_ref(&v.bar)).is_none(); } + + fn ref_bindings() { + let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|(&x, y)| x == *y).is_none(); + let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|&(&x, y)| x == *y).is_none(); + } } diff --git a/tests/ui/search_is_some_fixable_none.stderr b/tests/ui/search_is_some_fixable_none.stderr index 0c34067c1394..c78830a3b5dd 100644 --- a/tests/ui/search_is_some_fixable_none.stderr +++ b/tests/ui/search_is_some_fixable_none.stderr @@ -227,5 +227,17 @@ error: called `is_none()` after searching an `Iterator` with `find` LL | let _ = vfoo.iter().find(|v| v.by_ref(&v.bar)).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!vfoo.iter().any(|v| v.by_ref(&v.bar))` -error: aborting due to 34 previous errors +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:185:17 + | +LL | let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|(&x, y)| x == *y).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `![&(&1, 2), &(&3, 4), &(&5, 4)].iter().any(|(&x, y)| x == *y)` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:186:17 + | +LL | let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|&(&x, y)| x == *y).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `![&(&1, 2), &(&3, 4), &(&5, 4)].iter().any(|(&x, y)| x == *y)` + +error: aborting due to 36 previous errors diff --git a/tests/ui/search_is_some_fixable_some.fixed b/tests/ui/search_is_some_fixable_some.fixed index 43471e354939..267324cbe6a8 100644 --- a/tests/ui/search_is_some_fixable_some.fixed +++ b/tests/ui/search_is_some_fixable_some.fixed @@ -176,4 +176,9 @@ mod issue7392 { let vfoo = vec![Foo { bar: 1 }]; let _ = vfoo.iter().any(|v| v.by_ref(&v.bar)); } + + fn ref_bindings() { + let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().any(|(&x, y)| x == *y); + let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().any(|(&x, y)| x == *y); + } } diff --git a/tests/ui/search_is_some_fixable_some.rs b/tests/ui/search_is_some_fixable_some.rs index 82d3072d2f39..53f80e2c87ea 100644 --- a/tests/ui/search_is_some_fixable_some.rs +++ b/tests/ui/search_is_some_fixable_some.rs @@ -179,4 +179,9 @@ mod issue7392 { let vfoo = vec![Foo { bar: 1 }]; let _ = vfoo.iter().find(|v| v.by_ref(&v.bar)).is_some(); } + + fn ref_bindings() { + let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|(&x, y)| x == *y).is_some(); + let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|&(&x, y)| x == *y).is_some(); + } } diff --git a/tests/ui/search_is_some_fixable_some.stderr b/tests/ui/search_is_some_fixable_some.stderr index 5e8885991ac0..e8784bba6eb6 100644 --- a/tests/ui/search_is_some_fixable_some.stderr +++ b/tests/ui/search_is_some_fixable_some.stderr @@ -210,5 +210,17 @@ error: called `is_some()` after searching an `Iterator` with `find` LL | let _ = vfoo.iter().find(|v| v.by_ref(&v.bar)).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|v| v.by_ref(&v.bar))` -error: aborting due to 34 previous errors +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:184:55 + | +LL | let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|(&x, y)| x == *y).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|(&x, y)| x == *y)` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:185:55 + | +LL | let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|&(&x, y)| x == *y).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|(&x, y)| x == *y)` + +error: aborting due to 36 previous errors From abaaf744fdbe8106bc82ceaa34e8499d5436fc3c Mon Sep 17 00:00:00 2001 From: ThibsG Date: Fri, 1 Oct 2021 17:29:37 +0200 Subject: [PATCH 17/25] Add some notes about `MethodCall` cases --- clippy_lints/src/methods/search_is_some.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/clippy_lints/src/methods/search_is_some.rs b/clippy_lints/src/methods/search_is_some.rs index 4f8517b946a5..d08abc5b3def 100644 --- a/clippy_lints/src/methods/search_is_some.rs +++ b/clippy_lints/src/methods/search_is_some.rs @@ -221,7 +221,7 @@ struct DerefDelegate<'a, 'tcx> { impl DerefDelegate<'_, 'tcx> { pub fn finish(&mut self) -> String { - let end_span = Span::new(self.next_pos, self.closure_span.hi(), self.closure_span.ctxt()); + let end_span = Span::new(self.next_pos, self.closure_span.hi(), self.closure_span.ctxt(), None); let end_snip = snippet_with_applicability(self.cx, end_span, "..", &mut self.applicability); format!("{}{}", self.suggestion_start, end_snip) } @@ -255,7 +255,7 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { let map = self.cx.tcx.hir(); let ident_str = map.name(id).to_string(); let span = map.span(cmt.hir_id); - let start_span = Span::new(self.next_pos, span.lo(), span.ctxt()); + let start_span = Span::new(self.next_pos, span.lo(), span.ctxt(), None); let mut start_snip = snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability); if cmt.place.projections.is_empty() { @@ -263,8 +263,14 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { // i.e.: suggest `&x` instead of `x` self.suggestion_start.push_str(&format!("{}&{}", start_snip, ident_str)); } else { - // cases where a parent call is using the item + // cases where a parent `Call` or `MethodCall` is using the item // i.e.: suggest `.contains(&x)` for `.find(|x| [1, 2, 3].contains(x)).is_none()` + // + // Note about method calls: + // - compiler automatically dereference references if the target type is a reference (works also for + // function call) + // - `self` arguments in the case of `x.is_something()` are also automatically (de)referenced, and + // no projection should be suggested if let Some(parent_expr) = get_parent_expr_for_hir(self.cx, cmt.hir_id) { if let ExprKind::Call(_, call_args) | ExprKind::MethodCall(_, _, call_args, _) = parent_expr.kind { let expr = self.cx.tcx.hir().expect_expr(cmt.hir_id); @@ -316,7 +322,7 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { // so the span is set-up again to get more code, using `span.hi()` (i.e.: `foo[x]`) // instead of `span.lo()` (i.e.: `foo`) ProjectionKind::Index => { - let start_span = Span::new(self.next_pos, span.hi(), span.ctxt()); + let start_span = Span::new(self.next_pos, span.hi(), span.ctxt(), None); start_snip = snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability); replacement_str.clear(); projections_handled = true; From 7221999181c3d14b45ba10240bbaa49da7f74e72 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Wed, 20 Oct 2021 22:04:56 +0200 Subject: [PATCH 18/25] Add tests with closure --- tests/ui/search_is_some.rs | 6 ++++++ tests/ui/search_is_some.stderr | 20 ++++++++++++++++---- 2 files changed, 22 insertions(+), 4 deletions(-) diff --git a/tests/ui/search_is_some.rs b/tests/ui/search_is_some.rs index 72bc6ef35d31..72f335153c1b 100644 --- a/tests/ui/search_is_some.rs +++ b/tests/ui/search_is_some.rs @@ -36,6 +36,9 @@ fn main() { // check that we don't lint if `find()` is called with // `Pattern` that is not a string let _ = "hello world".find(|c: char| c == 'o' || c == 'l').is_some(); + + let some_closure = |x: &u32| *x == 0; + let _ = (0..1).find(some_closure).is_some(); } #[rustfmt::skip] @@ -70,4 +73,7 @@ fn is_none() { // check that we don't lint if `find()` is called with // `Pattern` that is not a string let _ = "hello world".find(|c: char| c == 'o' || c == 'l').is_none(); + + let some_closure = |x: &u32| *x == 0; + let _ = (0..1).find(some_closure).is_none(); } diff --git a/tests/ui/search_is_some.stderr b/tests/ui/search_is_some.stderr index f3c758e451ef..54760545bced 100644 --- a/tests/ui/search_is_some.stderr +++ b/tests/ui/search_is_some.stderr @@ -35,8 +35,14 @@ LL | | ).is_some(); | = help: this is more succinctly expressed by calling `any()` +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some.rs:41:20 + | +LL | let _ = (0..1).find(some_closure).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(some_closure)` + error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some.rs:48:13 + --> $DIR/search_is_some.rs:51:13 | LL | let _ = v.iter().find(|&x| { | _____________^ @@ -48,7 +54,7 @@ LL | | ).is_none(); = help: this is more succinctly expressed by calling `any()` with negation error: called `is_none()` after searching an `Iterator` with `position` - --> $DIR/search_is_some.rs:54:13 + --> $DIR/search_is_some.rs:57:13 | LL | let _ = v.iter().position(|&x| { | _____________^ @@ -60,7 +66,7 @@ LL | | ).is_none(); = help: this is more succinctly expressed by calling `any()` with negation error: called `is_none()` after searching an `Iterator` with `rposition` - --> $DIR/search_is_some.rs:60:13 + --> $DIR/search_is_some.rs:63:13 | LL | let _ = v.iter().rposition(|&x| { | _____________^ @@ -71,5 +77,11 @@ LL | | ).is_none(); | = help: this is more succinctly expressed by calling `any()` with negation -error: aborting due to 6 previous errors +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some.rs:78:13 + | +LL | let _ = (0..1).find(some_closure).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!(0..1).any(some_closure)` + +error: aborting due to 8 previous errors From 2ff702cbb547ea02381820536b27a575f14ba59e Mon Sep 17 00:00:00 2001 From: ThibsG Date: Wed, 20 Oct 2021 22:18:12 +0200 Subject: [PATCH 19/25] Rephrase the fn checking for a double ref, not only one --- clippy_lints/src/methods/search_is_some.rs | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/methods/search_is_some.rs b/clippy_lints/src/methods/search_is_some.rs index d08abc5b3def..96e040d9261e 100644 --- a/clippy_lints/src/methods/search_is_some.rs +++ b/clippy_lints/src/methods/search_is_some.rs @@ -226,7 +226,7 @@ impl DerefDelegate<'_, 'tcx> { format!("{}{}", self.suggestion_start, end_snip) } - fn func_takes_arg_by_ref(&self, parent_expr: &'tcx hir::Expr<'_>, cmt_hir_id: HirId) -> bool { + fn func_takes_arg_by_double_ref(&self, parent_expr: &'tcx hir::Expr<'_>, cmt_hir_id: HirId) -> bool { let (call_args, inputs) = match parent_expr.kind { ExprKind::MethodCall(_, _, call_args, _) => { if let Some(method_did) = self.cx.typeck_results().type_dependent_def_id(parent_expr.hir_id) { @@ -277,16 +277,18 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { let arg_ty_kind = self.cx.typeck_results().expr_ty(expr).kind(); if matches!(arg_ty_kind, ty::Ref(_, _, Mutability::Not)) { - // suggest ampersand if call function is taking args by ref - let takes_arg_by_ref = self.func_takes_arg_by_ref(parent_expr, cmt.hir_id); + // suggest ampersand if call function is taking args by double reference + let takes_arg_by_double_ref = self.func_takes_arg_by_double_ref(parent_expr, cmt.hir_id); // do not suggest ampersand if the ident is the method caller - let ident_sugg = - if !call_args.is_empty() && call_args[0].hir_id == cmt.hir_id && !takes_arg_by_ref { - format!("{}{}", start_snip, ident_str) - } else { - format!("{}&{}", start_snip, ident_str) - }; + let ident_sugg = if !call_args.is_empty() + && call_args[0].hir_id == cmt.hir_id + && !takes_arg_by_double_ref + { + format!("{}{}", start_snip, ident_str) + } else { + format!("{}&{}", start_snip, ident_str) + }; self.suggestion_start.push_str(&ident_sugg); self.next_pos = span.hi(); return; From 1176b8e5e9e697978ad563924f1561b52b330c07 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Mon, 1 Nov 2021 09:11:43 +0100 Subject: [PATCH 20/25] Handle closures with type annotations on args --- clippy_lints/src/methods/search_is_some.rs | 144 +++++++++++--------- tests/ui/search_is_some_fixable_none.fixed | 3 + tests/ui/search_is_some_fixable_none.rs | 3 + tests/ui/search_is_some_fixable_none.stderr | 28 ++-- tests/ui/search_is_some_fixable_some.fixed | 3 + tests/ui/search_is_some_fixable_some.rs | 3 + tests/ui/search_is_some_fixable_some.stderr | 28 ++-- 7 files changed, 132 insertions(+), 80 deletions(-) diff --git a/clippy_lints/src/methods/search_is_some.rs b/clippy_lints/src/methods/search_is_some.rs index 96e040d9261e..7baae4faa235 100644 --- a/clippy_lints/src/methods/search_is_some.rs +++ b/clippy_lints/src/methods/search_is_some.rs @@ -7,7 +7,7 @@ use clippy_utils::{get_parent_expr_for_hir, is_trait_method, strip_pat_refs}; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir as hir; -use rustc_hir::{self, ExprKind, HirId, PatKind}; +use rustc_hir::{self, ExprKind, HirId, MutTy, PatKind, TyKind}; use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::LateContext; use rustc_middle::hir::place::ProjectionKind; @@ -52,26 +52,12 @@ pub(super) fn check<'tcx>( then { if let hir::PatKind::Ref(..) = closure_arg.pat.kind { Some(search_snippet.replacen('&', "", 1)) - } else if let PatKind::Binding(_, binding_id, _, _) = strip_pat_refs(closure_arg.pat).kind { - // this binding is composed of at least two levels of references, so we need to remove one - let binding_type = cx.typeck_results().node_type(binding_id); - let innermost_is_ref = if let ty::Ref(_, inner,_) = binding_type.kind() { - matches!(inner.kind(), ty::Ref(_, innermost, _) if innermost.is_ref()) - } else { - false - }; - + } else if let PatKind::Binding(..) = strip_pat_refs(closure_arg.pat).kind { // `find()` provides a reference to the item, but `any` does not, // so we should fix item usages for suggestion - if let Some(closure_sugg) = get_closure_suggestion(cx, search_arg, closure_body) { + if let Some(closure_sugg) = get_closure_suggestion(cx, search_arg) { applicability = closure_sugg.applicability; - if innermost_is_ref { - Some(closure_sugg.suggestion.replacen('&', "", 1)) - } else { - Some(closure_sugg.suggestion) - } - } else if innermost_is_ref { - Some(search_snippet.replacen('&', "", 1)) + Some(closure_sugg.suggestion) } else { Some(search_snippet.to_string()) } @@ -174,6 +160,7 @@ pub(super) fn check<'tcx>( } } +#[derive(Debug)] struct ClosureSugg { applicability: Applicability, suggestion: String, @@ -182,38 +169,45 @@ struct ClosureSugg { // Build suggestion gradually by handling closure arg specific usages, // such as explicit deref and borrowing cases. // Returns `None` if no such use cases have been triggered in closure body -fn get_closure_suggestion<'tcx>( - cx: &LateContext<'_>, - search_arg: &'tcx hir::Expr<'_>, - closure_body: &hir::Body<'_>, -) -> Option { - let mut visitor = DerefDelegate { - cx, - closure_span: search_arg.span, - next_pos: search_arg.span.lo(), - suggestion_start: String::new(), - applicability: Applicability::MachineApplicable, - }; +fn get_closure_suggestion<'tcx>(cx: &LateContext<'_>, search_expr: &'tcx hir::Expr<'_>) -> Option { + if let hir::ExprKind::Closure(_, fn_decl, body_id, ..) = search_expr.kind { + let closure_body = cx.tcx.hir().body(body_id); + // is closure arg a double reference (i.e.: `|x: &&i32| ...`) + let closure_arg_is_double_ref = if let TyKind::Rptr(_, MutTy { ty, .. }) = fn_decl.inputs[0].kind { + matches!(ty.kind, TyKind::Rptr(_, MutTy { .. })) + } else { + false + }; + + let mut visitor = DerefDelegate { + cx, + closure_span: search_expr.span, + closure_arg_is_double_ref, + next_pos: search_expr.span.lo(), + suggestion_start: String::new(), + applicability: Applicability::MachineApplicable, + }; - let fn_def_id = cx.tcx.hir().local_def_id(search_arg.hir_id); - cx.tcx.infer_ctxt().enter(|infcx| { - ExprUseVisitor::new(&mut visitor, &infcx, fn_def_id, cx.param_env, cx.typeck_results()) - .consume_body(closure_body); - }); + let fn_def_id = cx.tcx.hir().local_def_id(search_expr.hir_id); + cx.tcx.infer_ctxt().enter(|infcx| { + ExprUseVisitor::new(&mut visitor, &infcx, fn_def_id, cx.param_env, cx.typeck_results()) + .consume_body(closure_body); + }); - if visitor.suggestion_start.is_empty() { - None - } else { - Some(ClosureSugg { - applicability: visitor.applicability, - suggestion: visitor.finish(), - }) + if !visitor.suggestion_start.is_empty() { + return Some(ClosureSugg { + applicability: visitor.applicability, + suggestion: visitor.finish(), + }); + } } + None } struct DerefDelegate<'a, 'tcx> { cx: &'a LateContext<'tcx>, closure_span: Span, + closure_arg_is_double_ref: bool, next_pos: BytePos, suggestion_start: String, applicability: Applicability, @@ -223,7 +217,12 @@ impl DerefDelegate<'_, 'tcx> { pub fn finish(&mut self) -> String { let end_span = Span::new(self.next_pos, self.closure_span.hi(), self.closure_span.ctxt(), None); let end_snip = snippet_with_applicability(self.cx, end_span, "..", &mut self.applicability); - format!("{}{}", self.suggestion_start, end_snip) + let sugg = format!("{}{}", self.suggestion_start, end_snip); + if self.closure_arg_is_double_ref { + sugg.replacen('&', "", 1) + } else { + sugg + } } fn func_takes_arg_by_double_ref(&self, parent_expr: &'tcx hir::Expr<'_>, cmt_hir_id: HirId) -> bool { @@ -261,6 +260,7 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { if cmt.place.projections.is_empty() { // handle item without any projection, that needs an explicit borrowing // i.e.: suggest `&x` instead of `x` + self.closure_arg_is_double_ref = false; self.suggestion_start.push_str(&format!("{}&{}", start_snip, ident_str)); } else { // cases where a parent `Call` or `MethodCall` is using the item @@ -272,29 +272,43 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { // - `self` arguments in the case of `x.is_something()` are also automatically (de)referenced, and // no projection should be suggested if let Some(parent_expr) = get_parent_expr_for_hir(self.cx, cmt.hir_id) { - if let ExprKind::Call(_, call_args) | ExprKind::MethodCall(_, _, call_args, _) = parent_expr.kind { - let expr = self.cx.tcx.hir().expect_expr(cmt.hir_id); - let arg_ty_kind = self.cx.typeck_results().expr_ty(expr).kind(); - - if matches!(arg_ty_kind, ty::Ref(_, _, Mutability::Not)) { - // suggest ampersand if call function is taking args by double reference - let takes_arg_by_double_ref = self.func_takes_arg_by_double_ref(parent_expr, cmt.hir_id); - - // do not suggest ampersand if the ident is the method caller - let ident_sugg = if !call_args.is_empty() - && call_args[0].hir_id == cmt.hir_id - && !takes_arg_by_double_ref - { - format!("{}{}", start_snip, ident_str) - } else { - format!("{}&{}", start_snip, ident_str) - }; - self.suggestion_start.push_str(&ident_sugg); + match &parent_expr.kind { + // given expression is the self argument and will be handled completely by the compiler + // i.e.: `|x| x.is_something()` + ExprKind::MethodCall(_, _, [self_expr, ..], _) if self_expr.hir_id == cmt.hir_id => { + self.suggestion_start.push_str(&format!("{}{}", start_snip, ident_str)); self.next_pos = span.hi(); return; - } + }, + // item is used in a call + // i.e.: `Call`: `|x| please(x)` or `MethodCall`: `|x| [1, 2, 3].contains(x)` + ExprKind::Call(_, [call_args @ ..]) | ExprKind::MethodCall(_, _, [_, call_args @ ..], _) => { + let expr = self.cx.tcx.hir().expect_expr(cmt.hir_id); + let arg_ty_kind = self.cx.typeck_results().expr_ty(expr).kind(); - self.applicability = Applicability::Unspecified; + if matches!(arg_ty_kind, ty::Ref(_, _, Mutability::Not)) { + // suggest ampersand if call function is taking args by double reference + let takes_arg_by_double_ref = + self.func_takes_arg_by_double_ref(parent_expr, cmt.hir_id); + + // no need to bind again if the function doesn't take arg by double ref + // and if the item is already a double ref + let ident_sugg = if !call_args.is_empty() + && !takes_arg_by_double_ref + && self.closure_arg_is_double_ref + { + format!("{}{}", start_snip, ident_str) + } else { + format!("{}&{}", start_snip, ident_str) + }; + self.suggestion_start.push_str(&ident_sugg); + self.next_pos = span.hi(); + return; + } + + self.applicability = Applicability::Unspecified; + }, + _ => (), } } @@ -346,7 +360,9 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { // handle `ProjectionKind::Deref` by removing one explicit deref // if no special case was detected (i.e.: suggest `*x` instead of `**x`) - if !projections_handled { + if projections_handled { + self.closure_arg_is_double_ref = false; + } else { let last_deref = cmt .place .projections diff --git a/tests/ui/search_is_some_fixable_none.fixed b/tests/ui/search_is_some_fixable_none.fixed index 240a69578931..adc6b2739c44 100644 --- a/tests/ui/search_is_some_fixable_none.fixed +++ b/tests/ui/search_is_some_fixable_none.fixed @@ -118,9 +118,12 @@ mod issue7392 { let v = vec![3, 2, 1, 0]; let _ = !v.iter().any(|x| deref_enough(*x)); + let _ = !v.iter().any(|x: &u32| deref_enough(*x)); #[allow(clippy::redundant_closure)] let _ = !v.iter().any(|x| arg_no_deref(&x)); + #[allow(clippy::redundant_closure)] + let _ = !v.iter().any(|x: &u32| arg_no_deref(&x)); } fn field_index_projection() { diff --git a/tests/ui/search_is_some_fixable_none.rs b/tests/ui/search_is_some_fixable_none.rs index 7e8fc14a7bd4..f0be2be4788b 100644 --- a/tests/ui/search_is_some_fixable_none.rs +++ b/tests/ui/search_is_some_fixable_none.rs @@ -122,9 +122,12 @@ mod issue7392 { let v = vec![3, 2, 1, 0]; let _ = v.iter().find(|x| deref_enough(**x)).is_none(); + let _ = v.iter().find(|x: &&u32| deref_enough(**x)).is_none(); #[allow(clippy::redundant_closure)] let _ = v.iter().find(|x| arg_no_deref(x)).is_none(); + #[allow(clippy::redundant_closure)] + let _ = v.iter().find(|x: &&u32| arg_no_deref(x)).is_none(); } fn field_index_projection() { diff --git a/tests/ui/search_is_some_fixable_none.stderr b/tests/ui/search_is_some_fixable_none.stderr index c78830a3b5dd..910d5ad37b89 100644 --- a/tests/ui/search_is_some_fixable_none.stderr +++ b/tests/ui/search_is_some_fixable_none.stderr @@ -188,13 +188,25 @@ LL | let _ = v.iter().find(|x| deref_enough(**x)).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|x| deref_enough(*x))` error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable_none.rs:127:17 + --> $DIR/search_is_some_fixable_none.rs:125:17 + | +LL | let _ = v.iter().find(|x: &&u32| deref_enough(**x)).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|x: &u32| deref_enough(*x))` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:128:17 | LL | let _ = v.iter().find(|x| arg_no_deref(x)).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|x| arg_no_deref(&x))` error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable_none.rs:147:17 + --> $DIR/search_is_some_fixable_none.rs:130:17 + | +LL | let _ = v.iter().find(|x: &&u32| arg_no_deref(x)).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|x: &u32| arg_no_deref(&x))` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:150:17 | LL | let _ = vfoo | _________________^ @@ -210,34 +222,34 @@ LL ~ .iter().any(|v| v.inner_double.bar[0][0] == 2 && v.inner.bar[0] | error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable_none.rs:163:17 + --> $DIR/search_is_some_fixable_none.rs:166:17 | LL | let _ = vfoo.iter().find(|v| v.inner[0].bar == 2).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!vfoo.iter().any(|v| v.inner[0].bar == 2)` error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable_none.rs:168:17 + --> $DIR/search_is_some_fixable_none.rs:171:17 | LL | let _ = vfoo.iter().find(|x| (**x)[0] == 9).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!vfoo.iter().any(|x| (**x)[0] == 9)` error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable_none.rs:181:17 + --> $DIR/search_is_some_fixable_none.rs:184:17 | LL | let _ = vfoo.iter().find(|v| v.by_ref(&v.bar)).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!vfoo.iter().any(|v| v.by_ref(&v.bar))` error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable_none.rs:185:17 + --> $DIR/search_is_some_fixable_none.rs:188:17 | LL | let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|(&x, y)| x == *y).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `![&(&1, 2), &(&3, 4), &(&5, 4)].iter().any(|(&x, y)| x == *y)` error: called `is_none()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable_none.rs:186:17 + --> $DIR/search_is_some_fixable_none.rs:189:17 | LL | let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|&(&x, y)| x == *y).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `![&(&1, 2), &(&3, 4), &(&5, 4)].iter().any(|(&x, y)| x == *y)` -error: aborting due to 36 previous errors +error: aborting due to 38 previous errors diff --git a/tests/ui/search_is_some_fixable_some.fixed b/tests/ui/search_is_some_fixable_some.fixed index 267324cbe6a8..2799ae0a98b8 100644 --- a/tests/ui/search_is_some_fixable_some.fixed +++ b/tests/ui/search_is_some_fixable_some.fixed @@ -119,9 +119,12 @@ mod issue7392 { let v = vec![3, 2, 1, 0]; let _ = v.iter().any(|x| deref_enough(*x)); + let _ = v.iter().any(|x: &u32| deref_enough(*x)); #[allow(clippy::redundant_closure)] let _ = v.iter().any(|x| arg_no_deref(&x)); + #[allow(clippy::redundant_closure)] + let _ = v.iter().any(|x: &u32| arg_no_deref(&x)); } fn field_index_projection() { diff --git a/tests/ui/search_is_some_fixable_some.rs b/tests/ui/search_is_some_fixable_some.rs index 53f80e2c87ea..bf9d50d90570 100644 --- a/tests/ui/search_is_some_fixable_some.rs +++ b/tests/ui/search_is_some_fixable_some.rs @@ -121,9 +121,12 @@ mod issue7392 { let v = vec![3, 2, 1, 0]; let _ = v.iter().find(|x| deref_enough(**x)).is_some(); + let _ = v.iter().find(|x: &&u32| deref_enough(**x)).is_some(); #[allow(clippy::redundant_closure)] let _ = v.iter().find(|x| arg_no_deref(x)).is_some(); + #[allow(clippy::redundant_closure)] + let _ = v.iter().find(|x: &&u32| arg_no_deref(x)).is_some(); } fn field_index_projection() { diff --git a/tests/ui/search_is_some_fixable_some.stderr b/tests/ui/search_is_some_fixable_some.stderr index e8784bba6eb6..71680bc8d2a2 100644 --- a/tests/ui/search_is_some_fixable_some.stderr +++ b/tests/ui/search_is_some_fixable_some.stderr @@ -179,13 +179,25 @@ LL | let _ = v.iter().find(|x| deref_enough(**x)).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| deref_enough(*x))` error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable_some.rs:126:26 + --> $DIR/search_is_some_fixable_some.rs:124:26 + | +LL | let _ = v.iter().find(|x: &&u32| deref_enough(**x)).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x: &u32| deref_enough(*x))` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:127:26 | LL | let _ = v.iter().find(|x| arg_no_deref(x)).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| arg_no_deref(&x))` error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable_some.rs:148:14 + --> $DIR/search_is_some_fixable_some.rs:129:26 + | +LL | let _ = v.iter().find(|x: &&u32| arg_no_deref(x)).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x: &u32| arg_no_deref(&x))` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:151:14 | LL | .find(|v| v.inner_double.bar[0][0] == 2 && v.inner.bar[0] == 2) | ______________^ @@ -193,34 +205,34 @@ LL | | .is_some(); | |______________________^ help: use `any()` instead: `any(|v| v.inner_double.bar[0][0] == 2 && v.inner.bar[0] == 2)` error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable_some.rs:162:29 + --> $DIR/search_is_some_fixable_some.rs:165:29 | LL | let _ = vfoo.iter().find(|v| v.inner[0].bar == 2).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|v| v.inner[0].bar == 2)` error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable_some.rs:167:29 + --> $DIR/search_is_some_fixable_some.rs:170:29 | LL | let _ = vfoo.iter().find(|x| (**x)[0] == 9).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|x| (**x)[0] == 9)` error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable_some.rs:180:29 + --> $DIR/search_is_some_fixable_some.rs:183:29 | LL | let _ = vfoo.iter().find(|v| v.by_ref(&v.bar)).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|v| v.by_ref(&v.bar))` error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable_some.rs:184:55 + --> $DIR/search_is_some_fixable_some.rs:187:55 | LL | let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|(&x, y)| x == *y).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|(&x, y)| x == *y)` error: called `is_some()` after searching an `Iterator` with `find` - --> $DIR/search_is_some_fixable_some.rs:185:55 + --> $DIR/search_is_some_fixable_some.rs:188:55 | LL | let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|&(&x, y)| x == *y).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|(&x, y)| x == *y)` -error: aborting due to 36 previous errors +error: aborting due to 38 previous errors From 5ebede0c14d00f50a2db3d5a8e944563e40244c7 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Sat, 20 Nov 2021 09:37:37 +0100 Subject: [PATCH 21/25] Fix full projection identifier + move applicability to `MaybeIncorrect` --- clippy_lints/src/methods/search_is_some.rs | 42 +++++++++++++-------- tests/ui/search_is_some_fixable_none.fixed | 31 +++++++++++++++ tests/ui/search_is_some_fixable_none.rs | 31 +++++++++++++++ tests/ui/search_is_some_fixable_none.stderr | 40 +++++++++++++++++++- tests/ui/search_is_some_fixable_some.fixed | 31 +++++++++++++++ tests/ui/search_is_some_fixable_some.rs | 31 +++++++++++++++ tests/ui/search_is_some_fixable_some.stderr | 40 +++++++++++++++++++- 7 files changed, 228 insertions(+), 18 deletions(-) diff --git a/clippy_lints/src/methods/search_is_some.rs b/clippy_lints/src/methods/search_is_some.rs index 7baae4faa235..a3449c1fe121 100644 --- a/clippy_lints/src/methods/search_is_some.rs +++ b/clippy_lints/src/methods/search_is_some.rs @@ -185,7 +185,7 @@ fn get_closure_suggestion<'tcx>(cx: &LateContext<'_>, search_expr: &'tcx hir::Ex closure_arg_is_double_ref, next_pos: search_expr.span.lo(), suggestion_start: String::new(), - applicability: Applicability::MachineApplicable, + applicability: Applicability::MaybeIncorrect, }; let fn_def_id = cx.tcx.hir().local_def_id(search_expr.hir_id); @@ -252,11 +252,15 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) { if let PlaceBase::Local(id) = cmt.place.base { let map = self.cx.tcx.hir(); - let ident_str = map.name(id).to_string(); let span = map.span(cmt.hir_id); let start_span = Span::new(self.next_pos, span.lo(), span.ctxt(), None); let mut start_snip = snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability); + // identifier referring to the variable currently triggered (i.e.: `fp`) + let ident_str = map.name(id).to_string(); + // full identifier that includes projection (i.e.: `fp.field`) + let ident_str_with_proj = snippet(self.cx, span, "..").to_string(); + if cmt.place.projections.is_empty() { // handle item without any projection, that needs an explicit borrowing // i.e.: suggest `&x` instead of `x` @@ -276,7 +280,8 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { // given expression is the self argument and will be handled completely by the compiler // i.e.: `|x| x.is_something()` ExprKind::MethodCall(_, _, [self_expr, ..], _) if self_expr.hir_id == cmt.hir_id => { - self.suggestion_start.push_str(&format!("{}{}", start_snip, ident_str)); + self.suggestion_start + .push_str(&format!("{}{}", start_snip, ident_str_with_proj)); self.next_pos = span.hi(); return; }, @@ -291,13 +296,26 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { let takes_arg_by_double_ref = self.func_takes_arg_by_double_ref(parent_expr, cmt.hir_id); + // compiler will automatically dereference field projection, so no need + // to suggest ampersand, but full identifier that includes projection is required + let has_field_projection = cmt + .place + .projections + .iter() + .any(|proj| matches!(proj.kind, ProjectionKind::Field(..))); + // no need to bind again if the function doesn't take arg by double ref // and if the item is already a double ref let ident_sugg = if !call_args.is_empty() && !takes_arg_by_double_ref - && self.closure_arg_is_double_ref + && (self.closure_arg_is_double_ref || has_field_projection) { - format!("{}{}", start_snip, ident_str) + let ident = if has_field_projection { + ident_str_with_proj + } else { + ident_str + }; + format!("{}{}", start_snip, ident) } else { format!("{}&{}", start_snip, ident_str) }; @@ -318,17 +336,9 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { match proj.kind { // Field projection like `|v| v.foo` // no adjustment needed here, as field projections are handled by the compiler - ProjectionKind::Field(idx, variant) => match cmt.place.ty_before_projection(i).kind() { - ty::Adt(def, ..) => { - replacement_str = format!( - "{}.{}", - replacement_str, - def.variants[variant].fields[idx as usize].ident.name.as_str() - ); - projections_handled = true; - }, - ty::Tuple(_) => { - replacement_str = format!("{}.{}", replacement_str, idx); + ProjectionKind::Field(..) => match cmt.place.ty_before_projection(i).kind() { + ty::Adt(..) | ty::Tuple(_) => { + replacement_str = ident_str_with_proj.clone(); projections_handled = true; }, _ => (), diff --git a/tests/ui/search_is_some_fixable_none.fixed b/tests/ui/search_is_some_fixable_none.fixed index adc6b2739c44..6831fb2cf59e 100644 --- a/tests/ui/search_is_some_fixable_none.fixed +++ b/tests/ui/search_is_some_fixable_none.fixed @@ -182,4 +182,35 @@ mod issue7392 { let _ = ![&(&1, 2), &(&3, 4), &(&5, 4)].iter().any(|(&x, y)| x == *y); let _ = ![&(&1, 2), &(&3, 4), &(&5, 4)].iter().any(|(&x, y)| x == *y); } + + fn test_string_1(s: &str) -> bool { + s.is_empty() + } + + fn test_u32_1(s: &u32) -> bool { + s.is_power_of_two() + } + + fn test_u32_2(s: u32) -> bool { + s.is_power_of_two() + } + + fn projection_in_args_test() { + // Index projections + let lst = &[String::from("Hello"), String::from("world")]; + let v: Vec<&[String]> = vec![lst]; + let _ = !v.iter().any(|s| s[0].is_empty()); + let _ = !v.iter().any(|s| test_string_1(&s[0])); + + // Field projections + struct FieldProjection<'a> { + field: &'a u32, + } + let field = 123456789; + let instance = FieldProjection { field: &field }; + let v = vec![instance]; + let _ = !v.iter().any(|fp| fp.field.is_power_of_two()); + let _ = !v.iter().any(|fp| test_u32_1(fp.field)); + let _ = !v.iter().any(|fp| test_u32_2(*fp.field)); + } } diff --git a/tests/ui/search_is_some_fixable_none.rs b/tests/ui/search_is_some_fixable_none.rs index f0be2be4788b..778f4f6fa257 100644 --- a/tests/ui/search_is_some_fixable_none.rs +++ b/tests/ui/search_is_some_fixable_none.rs @@ -188,4 +188,35 @@ mod issue7392 { let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|(&x, y)| x == *y).is_none(); let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|&(&x, y)| x == *y).is_none(); } + + fn test_string_1(s: &String) -> bool { + s.is_empty() + } + + fn test_u32_1(s: &u32) -> bool { + s.is_power_of_two() + } + + fn test_u32_2(s: u32) -> bool { + s.is_power_of_two() + } + + fn projection_in_args_test() { + // Index projections + let lst = &[String::from("Hello"), String::from("world")]; + let v: Vec<&[String]> = vec![lst]; + let _ = v.iter().find(|s| s[0].is_empty()).is_none(); + let _ = v.iter().find(|s| test_string_1(&s[0])).is_none(); + + // Field projections + struct FieldProjection<'a> { + field: &'a u32, + } + let field = 123456789; + let instance = FieldProjection { field: &field }; + let v = vec![instance]; + let _ = v.iter().find(|fp| fp.field.is_power_of_two()).is_none(); + let _ = v.iter().find(|fp| test_u32_1(fp.field)).is_none(); + let _ = v.iter().find(|fp| test_u32_2(*fp.field)).is_none(); + } } diff --git a/tests/ui/search_is_some_fixable_none.stderr b/tests/ui/search_is_some_fixable_none.stderr index 910d5ad37b89..7c5e5eb589c1 100644 --- a/tests/ui/search_is_some_fixable_none.stderr +++ b/tests/ui/search_is_some_fixable_none.stderr @@ -251,5 +251,43 @@ error: called `is_none()` after searching an `Iterator` with `find` LL | let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|&(&x, y)| x == *y).is_none(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `![&(&1, 2), &(&3, 4), &(&5, 4)].iter().any(|(&x, y)| x == *y)` -error: aborting due to 38 previous errors +error: writing `&String` instead of `&str` involves a new object where a slice will do + --> $DIR/search_is_some_fixable_none.rs:192:25 + | +LL | fn test_string_1(s: &String) -> bool { + | ^^^^^^^ help: change this to: `&str` + | + = note: `-D clippy::ptr-arg` implied by `-D warnings` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:208:17 + | +LL | let _ = v.iter().find(|s| s[0].is_empty()).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|s| s[0].is_empty())` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:209:17 + | +LL | let _ = v.iter().find(|s| test_string_1(&s[0])).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|s| test_string_1(&s[0]))` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:218:17 + | +LL | let _ = v.iter().find(|fp| fp.field.is_power_of_two()).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|fp| fp.field.is_power_of_two())` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:219:17 + | +LL | let _ = v.iter().find(|fp| test_u32_1(fp.field)).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|fp| test_u32_1(fp.field))` + +error: called `is_none()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_none.rs:220:17 + | +LL | let _ = v.iter().find(|fp| test_u32_2(*fp.field)).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|fp| test_u32_2(*fp.field))` + +error: aborting due to 44 previous errors diff --git a/tests/ui/search_is_some_fixable_some.fixed b/tests/ui/search_is_some_fixable_some.fixed index 2799ae0a98b8..7c940a2b069e 100644 --- a/tests/ui/search_is_some_fixable_some.fixed +++ b/tests/ui/search_is_some_fixable_some.fixed @@ -184,4 +184,35 @@ mod issue7392 { let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().any(|(&x, y)| x == *y); let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().any(|(&x, y)| x == *y); } + + fn test_string_1(s: &str) -> bool { + s.is_empty() + } + + fn test_u32_1(s: &u32) -> bool { + s.is_power_of_two() + } + + fn test_u32_2(s: u32) -> bool { + s.is_power_of_two() + } + + fn projection_in_args_test() { + // Index projections + let lst = &[String::from("Hello"), String::from("world")]; + let v: Vec<&[String]> = vec![lst]; + let _ = v.iter().any(|s| s[0].is_empty()); + let _ = v.iter().any(|s| test_string_1(&s[0])); + + // Field projections + struct FieldProjection<'a> { + field: &'a u32, + } + let field = 123456789; + let instance = FieldProjection { field: &field }; + let v = vec![instance]; + let _ = v.iter().any(|fp| fp.field.is_power_of_two()); + let _ = v.iter().any(|fp| test_u32_1(fp.field)); + let _ = v.iter().any(|fp| test_u32_2(*fp.field)); + } } diff --git a/tests/ui/search_is_some_fixable_some.rs b/tests/ui/search_is_some_fixable_some.rs index bf9d50d90570..241641fceae3 100644 --- a/tests/ui/search_is_some_fixable_some.rs +++ b/tests/ui/search_is_some_fixable_some.rs @@ -187,4 +187,35 @@ mod issue7392 { let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|(&x, y)| x == *y).is_some(); let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|&(&x, y)| x == *y).is_some(); } + + fn test_string_1(s: &String) -> bool { + s.is_empty() + } + + fn test_u32_1(s: &u32) -> bool { + s.is_power_of_two() + } + + fn test_u32_2(s: u32) -> bool { + s.is_power_of_two() + } + + fn projection_in_args_test() { + // Index projections + let lst = &[String::from("Hello"), String::from("world")]; + let v: Vec<&[String]> = vec![lst]; + let _ = v.iter().find(|s| s[0].is_empty()).is_some(); + let _ = v.iter().find(|s| test_string_1(&s[0])).is_some(); + + // Field projections + struct FieldProjection<'a> { + field: &'a u32, + } + let field = 123456789; + let instance = FieldProjection { field: &field }; + let v = vec![instance]; + let _ = v.iter().find(|fp| fp.field.is_power_of_two()).is_some(); + let _ = v.iter().find(|fp| test_u32_1(fp.field)).is_some(); + let _ = v.iter().find(|fp| test_u32_2(*fp.field)).is_some(); + } } diff --git a/tests/ui/search_is_some_fixable_some.stderr b/tests/ui/search_is_some_fixable_some.stderr index 71680bc8d2a2..9212c6e71ff5 100644 --- a/tests/ui/search_is_some_fixable_some.stderr +++ b/tests/ui/search_is_some_fixable_some.stderr @@ -234,5 +234,43 @@ error: called `is_some()` after searching an `Iterator` with `find` LL | let _ = [&(&1, 2), &(&3, 4), &(&5, 4)].iter().find(|&(&x, y)| x == *y).is_some(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|(&x, y)| x == *y)` -error: aborting due to 38 previous errors +error: writing `&String` instead of `&str` involves a new object where a slice will do + --> $DIR/search_is_some_fixable_some.rs:191:25 + | +LL | fn test_string_1(s: &String) -> bool { + | ^^^^^^^ help: change this to: `&str` + | + = note: `-D clippy::ptr-arg` implied by `-D warnings` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:207:26 + | +LL | let _ = v.iter().find(|s| s[0].is_empty()).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|s| s[0].is_empty())` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:208:26 + | +LL | let _ = v.iter().find(|s| test_string_1(&s[0])).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|s| test_string_1(&s[0]))` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:217:26 + | +LL | let _ = v.iter().find(|fp| fp.field.is_power_of_two()).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|fp| fp.field.is_power_of_two())` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:218:26 + | +LL | let _ = v.iter().find(|fp| test_u32_1(fp.field)).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|fp| test_u32_1(fp.field))` + +error: called `is_some()` after searching an `Iterator` with `find` + --> $DIR/search_is_some_fixable_some.rs:219:26 + | +LL | let _ = v.iter().find(|fp| test_u32_2(*fp.field)).is_some(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `any()` instead: `any(|fp| test_u32_2(*fp.field))` + +error: aborting due to 44 previous errors From 092fe209a6568ade22046dbf4e78455d5aeeb5fa Mon Sep 17 00:00:00 2001 From: ThibsG Date: Sat, 20 Nov 2021 10:00:50 +0100 Subject: [PATCH 22/25] Move deref closure builder to `clippy_utils::sugg` module --- clippy_lints/src/methods/search_is_some.rs | 258 +------------------ clippy_utils/src/sugg.rs | 278 ++++++++++++++++++++- 2 files changed, 279 insertions(+), 257 deletions(-) diff --git a/clippy_lints/src/methods/search_is_some.rs b/clippy_lints/src/methods/search_is_some.rs index a3449c1fe121..5ed4ba94884e 100644 --- a/clippy_lints/src/methods/search_is_some.rs +++ b/clippy_lints/src/methods/search_is_some.rs @@ -1,21 +1,16 @@ -use std::iter; - use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg}; use clippy_utils::source::{snippet, snippet_with_applicability}; +use clippy_utils::sugg::deref_closure_args; use clippy_utils::ty::is_type_diagnostic_item; -use clippy_utils::{get_parent_expr_for_hir, is_trait_method, strip_pat_refs}; +use clippy_utils::{is_trait_method, strip_pat_refs}; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir as hir; -use rustc_hir::{self, ExprKind, HirId, MutTy, PatKind, TyKind}; -use rustc_infer::infer::TyCtxtInferExt; +use rustc_hir::PatKind; use rustc_lint::LateContext; -use rustc_middle::hir::place::ProjectionKind; -use rustc_middle::mir::{FakeReadCause, Mutability}; use rustc_middle::ty; -use rustc_span::source_map::{BytePos, Span}; +use rustc_span::source_map::Span; use rustc_span::symbol::sym; -use rustc_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId}; use super::SEARCH_IS_SOME; @@ -55,7 +50,7 @@ pub(super) fn check<'tcx>( } else if let PatKind::Binding(..) = strip_pat_refs(closure_arg.pat).kind { // `find()` provides a reference to the item, but `any` does not, // so we should fix item usages for suggestion - if let Some(closure_sugg) = get_closure_suggestion(cx, search_arg) { + if let Some(closure_sugg) = deref_closure_args(cx, search_arg) { applicability = closure_sugg.applicability; Some(closure_sugg.suggestion) } else { @@ -159,246 +154,3 @@ pub(super) fn check<'tcx>( } } } - -#[derive(Debug)] -struct ClosureSugg { - applicability: Applicability, - suggestion: String, -} - -// Build suggestion gradually by handling closure arg specific usages, -// such as explicit deref and borrowing cases. -// Returns `None` if no such use cases have been triggered in closure body -fn get_closure_suggestion<'tcx>(cx: &LateContext<'_>, search_expr: &'tcx hir::Expr<'_>) -> Option { - if let hir::ExprKind::Closure(_, fn_decl, body_id, ..) = search_expr.kind { - let closure_body = cx.tcx.hir().body(body_id); - // is closure arg a double reference (i.e.: `|x: &&i32| ...`) - let closure_arg_is_double_ref = if let TyKind::Rptr(_, MutTy { ty, .. }) = fn_decl.inputs[0].kind { - matches!(ty.kind, TyKind::Rptr(_, MutTy { .. })) - } else { - false - }; - - let mut visitor = DerefDelegate { - cx, - closure_span: search_expr.span, - closure_arg_is_double_ref, - next_pos: search_expr.span.lo(), - suggestion_start: String::new(), - applicability: Applicability::MaybeIncorrect, - }; - - let fn_def_id = cx.tcx.hir().local_def_id(search_expr.hir_id); - cx.tcx.infer_ctxt().enter(|infcx| { - ExprUseVisitor::new(&mut visitor, &infcx, fn_def_id, cx.param_env, cx.typeck_results()) - .consume_body(closure_body); - }); - - if !visitor.suggestion_start.is_empty() { - return Some(ClosureSugg { - applicability: visitor.applicability, - suggestion: visitor.finish(), - }); - } - } - None -} - -struct DerefDelegate<'a, 'tcx> { - cx: &'a LateContext<'tcx>, - closure_span: Span, - closure_arg_is_double_ref: bool, - next_pos: BytePos, - suggestion_start: String, - applicability: Applicability, -} - -impl DerefDelegate<'_, 'tcx> { - pub fn finish(&mut self) -> String { - let end_span = Span::new(self.next_pos, self.closure_span.hi(), self.closure_span.ctxt(), None); - let end_snip = snippet_with_applicability(self.cx, end_span, "..", &mut self.applicability); - let sugg = format!("{}{}", self.suggestion_start, end_snip); - if self.closure_arg_is_double_ref { - sugg.replacen('&', "", 1) - } else { - sugg - } - } - - fn func_takes_arg_by_double_ref(&self, parent_expr: &'tcx hir::Expr<'_>, cmt_hir_id: HirId) -> bool { - let (call_args, inputs) = match parent_expr.kind { - ExprKind::MethodCall(_, _, call_args, _) => { - if let Some(method_did) = self.cx.typeck_results().type_dependent_def_id(parent_expr.hir_id) { - (call_args, self.cx.tcx.fn_sig(method_did).skip_binder().inputs()) - } else { - return false; - } - }, - ExprKind::Call(func, call_args) => { - let typ = self.cx.typeck_results().expr_ty(func); - (call_args, typ.fn_sig(self.cx.tcx).skip_binder().inputs()) - }, - _ => return false, - }; - - iter::zip(call_args, inputs) - .any(|(arg, ty)| arg.hir_id == cmt_hir_id && matches!(ty.kind(), ty::Ref(_, inner, _) if inner.is_ref())) - } -} - -impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { - fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {} - - fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) { - if let PlaceBase::Local(id) = cmt.place.base { - let map = self.cx.tcx.hir(); - let span = map.span(cmt.hir_id); - let start_span = Span::new(self.next_pos, span.lo(), span.ctxt(), None); - let mut start_snip = snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability); - - // identifier referring to the variable currently triggered (i.e.: `fp`) - let ident_str = map.name(id).to_string(); - // full identifier that includes projection (i.e.: `fp.field`) - let ident_str_with_proj = snippet(self.cx, span, "..").to_string(); - - if cmt.place.projections.is_empty() { - // handle item without any projection, that needs an explicit borrowing - // i.e.: suggest `&x` instead of `x` - self.closure_arg_is_double_ref = false; - self.suggestion_start.push_str(&format!("{}&{}", start_snip, ident_str)); - } else { - // cases where a parent `Call` or `MethodCall` is using the item - // i.e.: suggest `.contains(&x)` for `.find(|x| [1, 2, 3].contains(x)).is_none()` - // - // Note about method calls: - // - compiler automatically dereference references if the target type is a reference (works also for - // function call) - // - `self` arguments in the case of `x.is_something()` are also automatically (de)referenced, and - // no projection should be suggested - if let Some(parent_expr) = get_parent_expr_for_hir(self.cx, cmt.hir_id) { - match &parent_expr.kind { - // given expression is the self argument and will be handled completely by the compiler - // i.e.: `|x| x.is_something()` - ExprKind::MethodCall(_, _, [self_expr, ..], _) if self_expr.hir_id == cmt.hir_id => { - self.suggestion_start - .push_str(&format!("{}{}", start_snip, ident_str_with_proj)); - self.next_pos = span.hi(); - return; - }, - // item is used in a call - // i.e.: `Call`: `|x| please(x)` or `MethodCall`: `|x| [1, 2, 3].contains(x)` - ExprKind::Call(_, [call_args @ ..]) | ExprKind::MethodCall(_, _, [_, call_args @ ..], _) => { - let expr = self.cx.tcx.hir().expect_expr(cmt.hir_id); - let arg_ty_kind = self.cx.typeck_results().expr_ty(expr).kind(); - - if matches!(arg_ty_kind, ty::Ref(_, _, Mutability::Not)) { - // suggest ampersand if call function is taking args by double reference - let takes_arg_by_double_ref = - self.func_takes_arg_by_double_ref(parent_expr, cmt.hir_id); - - // compiler will automatically dereference field projection, so no need - // to suggest ampersand, but full identifier that includes projection is required - let has_field_projection = cmt - .place - .projections - .iter() - .any(|proj| matches!(proj.kind, ProjectionKind::Field(..))); - - // no need to bind again if the function doesn't take arg by double ref - // and if the item is already a double ref - let ident_sugg = if !call_args.is_empty() - && !takes_arg_by_double_ref - && (self.closure_arg_is_double_ref || has_field_projection) - { - let ident = if has_field_projection { - ident_str_with_proj - } else { - ident_str - }; - format!("{}{}", start_snip, ident) - } else { - format!("{}&{}", start_snip, ident_str) - }; - self.suggestion_start.push_str(&ident_sugg); - self.next_pos = span.hi(); - return; - } - - self.applicability = Applicability::Unspecified; - }, - _ => (), - } - } - - let mut replacement_str = ident_str; - let mut projections_handled = false; - cmt.place.projections.iter().enumerate().for_each(|(i, proj)| { - match proj.kind { - // Field projection like `|v| v.foo` - // no adjustment needed here, as field projections are handled by the compiler - ProjectionKind::Field(..) => match cmt.place.ty_before_projection(i).kind() { - ty::Adt(..) | ty::Tuple(_) => { - replacement_str = ident_str_with_proj.clone(); - projections_handled = true; - }, - _ => (), - }, - // Index projection like `|x| foo[x]` - // the index is dropped so we can't get it to build the suggestion, - // so the span is set-up again to get more code, using `span.hi()` (i.e.: `foo[x]`) - // instead of `span.lo()` (i.e.: `foo`) - ProjectionKind::Index => { - let start_span = Span::new(self.next_pos, span.hi(), span.ctxt(), None); - start_snip = snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability); - replacement_str.clear(); - projections_handled = true; - }, - // note: unable to trigger `Subslice` kind in tests - ProjectionKind::Subslice => (), - ProjectionKind::Deref => { - // explicit deref for arrays should be avoided in the suggestion - // i.e.: `|sub| *sub[1..4].len() == 3` is not expected - if let ty::Ref(_, inner, _) = cmt.place.ty_before_projection(i).kind() { - // dereferencing an array (i.e.: `|sub| sub[1..4].len() == 3`) - if matches!(inner.kind(), ty::Ref(_, innermost, _) if innermost.is_array()) { - projections_handled = true; - } - } - }, - } - }); - - // handle `ProjectionKind::Deref` by removing one explicit deref - // if no special case was detected (i.e.: suggest `*x` instead of `**x`) - if projections_handled { - self.closure_arg_is_double_ref = false; - } else { - let last_deref = cmt - .place - .projections - .iter() - .rposition(|proj| proj.kind == ProjectionKind::Deref); - - if let Some(pos) = last_deref { - let mut projections = cmt.place.projections.clone(); - projections.truncate(pos); - - for item in projections { - if item.kind == ProjectionKind::Deref { - replacement_str = format!("*{}", replacement_str); - } - } - } - } - - self.suggestion_start - .push_str(&format!("{}{}", start_snip, replacement_str)); - } - self.next_pos = span.hi(); - } - } - - fn mutate(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {} - - fn fake_read(&mut self, _: rustc_typeck::expr_use_visitor::Place<'tcx>, _: FakeReadCause, _: HirId) {} -} diff --git a/clippy_utils/src/sugg.rs b/clippy_utils/src/sugg.rs index 01fb944cc36f..e8a35f8c8dc3 100644 --- a/clippy_utils/src/sugg.rs +++ b/clippy_utils/src/sugg.rs @@ -1,19 +1,27 @@ //! Contains utility functions to generate suggestions. #![deny(clippy::missing_docs_in_private_items)] -use crate::higher; -use crate::source::{snippet, snippet_opt, snippet_with_context, snippet_with_macro_callsite}; +use crate::source::{ + snippet, snippet_opt, snippet_with_applicability, snippet_with_context, snippet_with_macro_callsite, +}; +use crate::{get_parent_expr_for_hir, higher}; use rustc_ast::util::parser::AssocOp; use rustc_ast::{ast, token}; use rustc_ast_pretty::pprust::token_kind_to_string; use rustc_errors::Applicability; use rustc_hir as hir; +use rustc_hir::{ExprKind, HirId, MutTy, TyKind}; +use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::{EarlyContext, LateContext, LintContext}; -use rustc_span::source_map::{CharPos, Span}; -use rustc_span::{BytePos, Pos, SyntaxContext}; +use rustc_middle::hir::place::ProjectionKind; +use rustc_middle::mir::{FakeReadCause, Mutability}; +use rustc_middle::ty; +use rustc_span::source_map::{BytePos, CharPos, Pos, Span, SyntaxContext}; +use rustc_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId}; use std::borrow::Cow; use std::convert::TryInto; use std::fmt::Display; +use std::iter; use std::ops::{Add, Neg, Not, Sub}; /// A helper type to build suggestion correctly handling parentheses. @@ -716,6 +724,268 @@ impl DiagnosticBuilderExt for rustc_errors::DiagnosticBuilder } } +/// Suggestion results for handling closure +/// args dereferencing and borrowing +pub struct DerefClosure { + /// confidence on the built suggestion + pub applicability: Applicability, + /// gradually built suggestion + pub suggestion: String, +} + +/// Build suggestion gradually by handling closure arg specific usages, +/// such as explicit deref and borrowing cases. +/// Returns `None` if no such use cases have been triggered in closure body +/// +/// note: this only works on single line immutable closures +pub fn deref_closure_args<'tcx>(cx: &LateContext<'_>, search_expr: &'tcx hir::Expr<'_>) -> Option { + if let hir::ExprKind::Closure(_, fn_decl, body_id, ..) = search_expr.kind { + let closure_body = cx.tcx.hir().body(body_id); + // is closure arg a double reference (i.e.: `|x: &&i32| ...`) + let closure_arg_is_double_ref = if let TyKind::Rptr(_, MutTy { ty, .. }) = fn_decl.inputs[0].kind { + matches!(ty.kind, TyKind::Rptr(_, MutTy { .. })) + } else { + false + }; + + let mut visitor = DerefDelegate { + cx, + closure_span: search_expr.span, + closure_arg_is_double_ref, + next_pos: search_expr.span.lo(), + suggestion_start: String::new(), + applicability: Applicability::MaybeIncorrect, + }; + + let fn_def_id = cx.tcx.hir().local_def_id(search_expr.hir_id); + cx.tcx.infer_ctxt().enter(|infcx| { + ExprUseVisitor::new(&mut visitor, &infcx, fn_def_id, cx.param_env, cx.typeck_results()) + .consume_body(closure_body); + }); + + if !visitor.suggestion_start.is_empty() { + return Some(DerefClosure { + applicability: visitor.applicability, + suggestion: visitor.finish(), + }); + } + } + None +} + +/// Visitor struct used for tracking down +/// dereferencing and borrowing of closure's args +struct DerefDelegate<'a, 'tcx> { + /// The late context of the lint + cx: &'a LateContext<'tcx>, + /// The span of the input closure to adapt + closure_span: Span, + /// Indicates if the arg of the closure is a double reference + closure_arg_is_double_ref: bool, + /// last position of the span to gradually build the suggestion + next_pos: BytePos, + /// starting part of the gradually built suggestion + suggestion_start: String, + /// confidence on the built suggestion + applicability: Applicability, +} + +impl DerefDelegate<'_, 'tcx> { + /// build final suggestion: + /// - create the ending part of suggestion + /// - concatenate starting and ending parts + /// - potentially remove needless borrowing + pub fn finish(&mut self) -> String { + let end_span = Span::new(self.next_pos, self.closure_span.hi(), self.closure_span.ctxt(), None); + let end_snip = snippet_with_applicability(self.cx, end_span, "..", &mut self.applicability); + let sugg = format!("{}{}", self.suggestion_start, end_snip); + if self.closure_arg_is_double_ref { + sugg.replacen('&', "", 1) + } else { + sugg + } + } + + /// indicates whether the function from `parent_expr` takes its args by double reference + fn func_takes_arg_by_double_ref(&self, parent_expr: &'tcx hir::Expr<'_>, cmt_hir_id: HirId) -> bool { + let (call_args, inputs) = match parent_expr.kind { + ExprKind::MethodCall(_, _, call_args, _) => { + if let Some(method_did) = self.cx.typeck_results().type_dependent_def_id(parent_expr.hir_id) { + (call_args, self.cx.tcx.fn_sig(method_did).skip_binder().inputs()) + } else { + return false; + } + }, + ExprKind::Call(func, call_args) => { + let typ = self.cx.typeck_results().expr_ty(func); + (call_args, typ.fn_sig(self.cx.tcx).skip_binder().inputs()) + }, + _ => return false, + }; + + iter::zip(call_args, inputs) + .any(|(arg, ty)| arg.hir_id == cmt_hir_id && matches!(ty.kind(), ty::Ref(_, inner, _) if inner.is_ref())) + } +} + +impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { + fn consume(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {} + + #[allow(clippy::too_many_lines)] + fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) { + if let PlaceBase::Local(id) = cmt.place.base { + let map = self.cx.tcx.hir(); + let span = map.span(cmt.hir_id); + let start_span = Span::new(self.next_pos, span.lo(), span.ctxt(), None); + let mut start_snip = snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability); + + // identifier referring to the variable currently triggered (i.e.: `fp`) + let ident_str = map.name(id).to_string(); + // full identifier that includes projection (i.e.: `fp.field`) + let ident_str_with_proj = snippet(self.cx, span, "..").to_string(); + + if cmt.place.projections.is_empty() { + // handle item without any projection, that needs an explicit borrowing + // i.e.: suggest `&x` instead of `x` + self.closure_arg_is_double_ref = false; + self.suggestion_start.push_str(&format!("{}&{}", start_snip, ident_str)); + } else { + // cases where a parent `Call` or `MethodCall` is using the item + // i.e.: suggest `.contains(&x)` for `.find(|x| [1, 2, 3].contains(x)).is_none()` + // + // Note about method calls: + // - compiler automatically dereference references if the target type is a reference (works also for + // function call) + // - `self` arguments in the case of `x.is_something()` are also automatically (de)referenced, and + // no projection should be suggested + if let Some(parent_expr) = get_parent_expr_for_hir(self.cx, cmt.hir_id) { + match &parent_expr.kind { + // given expression is the self argument and will be handled completely by the compiler + // i.e.: `|x| x.is_something()` + ExprKind::MethodCall(_, _, [self_expr, ..], _) if self_expr.hir_id == cmt.hir_id => { + self.suggestion_start + .push_str(&format!("{}{}", start_snip, ident_str_with_proj)); + self.next_pos = span.hi(); + return; + }, + // item is used in a call + // i.e.: `Call`: `|x| please(x)` or `MethodCall`: `|x| [1, 2, 3].contains(x)` + ExprKind::Call(_, [call_args @ ..]) | ExprKind::MethodCall(_, _, [_, call_args @ ..], _) => { + let expr = self.cx.tcx.hir().expect_expr(cmt.hir_id); + let arg_ty_kind = self.cx.typeck_results().expr_ty(expr).kind(); + + if matches!(arg_ty_kind, ty::Ref(_, _, Mutability::Not)) { + // suggest ampersand if call function is taking args by double reference + let takes_arg_by_double_ref = + self.func_takes_arg_by_double_ref(parent_expr, cmt.hir_id); + + // compiler will automatically dereference field projection, so no need + // to suggest ampersand, but full identifier that includes projection is required + let has_field_projection = cmt + .place + .projections + .iter() + .any(|proj| matches!(proj.kind, ProjectionKind::Field(..))); + + // no need to bind again if the function doesn't take arg by double ref + // and if the item is already a double ref + let ident_sugg = if !call_args.is_empty() + && !takes_arg_by_double_ref + && (self.closure_arg_is_double_ref || has_field_projection) + { + let ident = if has_field_projection { + ident_str_with_proj + } else { + ident_str + }; + format!("{}{}", start_snip, ident) + } else { + format!("{}&{}", start_snip, ident_str) + }; + self.suggestion_start.push_str(&ident_sugg); + self.next_pos = span.hi(); + return; + } + + self.applicability = Applicability::Unspecified; + }, + _ => (), + } + } + + let mut replacement_str = ident_str; + let mut projections_handled = false; + cmt.place.projections.iter().enumerate().for_each(|(i, proj)| { + match proj.kind { + // Field projection like `|v| v.foo` + // no adjustment needed here, as field projections are handled by the compiler + ProjectionKind::Field(..) => match cmt.place.ty_before_projection(i).kind() { + ty::Adt(..) | ty::Tuple(_) => { + replacement_str = ident_str_with_proj.clone(); + projections_handled = true; + }, + _ => (), + }, + // Index projection like `|x| foo[x]` + // the index is dropped so we can't get it to build the suggestion, + // so the span is set-up again to get more code, using `span.hi()` (i.e.: `foo[x]`) + // instead of `span.lo()` (i.e.: `foo`) + ProjectionKind::Index => { + let start_span = Span::new(self.next_pos, span.hi(), span.ctxt(), None); + start_snip = snippet_with_applicability(self.cx, start_span, "..", &mut self.applicability); + replacement_str.clear(); + projections_handled = true; + }, + // note: unable to trigger `Subslice` kind in tests + ProjectionKind::Subslice => (), + ProjectionKind::Deref => { + // explicit deref for arrays should be avoided in the suggestion + // i.e.: `|sub| *sub[1..4].len() == 3` is not expected + if let ty::Ref(_, inner, _) = cmt.place.ty_before_projection(i).kind() { + // dereferencing an array (i.e.: `|sub| sub[1..4].len() == 3`) + if matches!(inner.kind(), ty::Ref(_, innermost, _) if innermost.is_array()) { + projections_handled = true; + } + } + }, + } + }); + + // handle `ProjectionKind::Deref` by removing one explicit deref + // if no special case was detected (i.e.: suggest `*x` instead of `**x`) + if projections_handled { + self.closure_arg_is_double_ref = false; + } else { + let last_deref = cmt + .place + .projections + .iter() + .rposition(|proj| proj.kind == ProjectionKind::Deref); + + if let Some(pos) = last_deref { + let mut projections = cmt.place.projections.clone(); + projections.truncate(pos); + + for item in projections { + if item.kind == ProjectionKind::Deref { + replacement_str = format!("*{}", replacement_str); + } + } + } + } + + self.suggestion_start + .push_str(&format!("{}{}", start_snip, replacement_str)); + } + self.next_pos = span.hi(); + } + } + + fn mutate(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {} + + fn fake_read(&mut self, _: rustc_typeck::expr_use_visitor::Place<'tcx>, _: FakeReadCause, _: HirId) {} +} + #[cfg(test)] mod test { use super::Sugg; From c5ce7ff6d94000bab267324db2aa11341265291e Mon Sep 17 00:00:00 2001 From: ThibsG Date: Sat, 27 Nov 2021 19:22:19 +0100 Subject: [PATCH 23/25] Add `Index` when checking projs in a call, rename some variables and remove unneeded statements --- clippy_utils/src/sugg.rs | 32 ++++++++++++++------------------ 1 file changed, 14 insertions(+), 18 deletions(-) diff --git a/clippy_utils/src/sugg.rs b/clippy_utils/src/sugg.rs index e8a35f8c8dc3..5dd49c1d26b9 100644 --- a/clippy_utils/src/sugg.rs +++ b/clippy_utils/src/sugg.rs @@ -737,9 +737,9 @@ pub struct DerefClosure { /// such as explicit deref and borrowing cases. /// Returns `None` if no such use cases have been triggered in closure body /// -/// note: this only works on single line immutable closures -pub fn deref_closure_args<'tcx>(cx: &LateContext<'_>, search_expr: &'tcx hir::Expr<'_>) -> Option { - if let hir::ExprKind::Closure(_, fn_decl, body_id, ..) = search_expr.kind { +/// note: this only works on single line immutable closures with one exactly one input parameter. +pub fn deref_closure_args<'tcx>(cx: &LateContext<'_>, closure: &'tcx hir::Expr<'_>) -> Option { + if let hir::ExprKind::Closure(_, fn_decl, body_id, ..) = closure.kind { let closure_body = cx.tcx.hir().body(body_id); // is closure arg a double reference (i.e.: `|x: &&i32| ...`) let closure_arg_is_double_ref = if let TyKind::Rptr(_, MutTy { ty, .. }) = fn_decl.inputs[0].kind { @@ -750,14 +750,14 @@ pub fn deref_closure_args<'tcx>(cx: &LateContext<'_>, search_expr: &'tcx hir::Ex let mut visitor = DerefDelegate { cx, - closure_span: search_expr.span, + closure_span: closure.span, closure_arg_is_double_ref, - next_pos: search_expr.span.lo(), + next_pos: closure.span.lo(), suggestion_start: String::new(), applicability: Applicability::MaybeIncorrect, }; - let fn_def_id = cx.tcx.hir().local_def_id(search_expr.hir_id); + let fn_def_id = cx.tcx.hir().local_def_id(closure.hir_id); cx.tcx.infer_ctxt().enter(|infcx| { ExprUseVisitor::new(&mut visitor, &infcx, fn_def_id, cx.param_env, cx.typeck_results()) .consume_body(closure_body); @@ -847,7 +847,6 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { if cmt.place.projections.is_empty() { // handle item without any projection, that needs an explicit borrowing // i.e.: suggest `&x` instead of `x` - self.closure_arg_is_double_ref = false; self.suggestion_start.push_str(&format!("{}&{}", start_snip, ident_str)); } else { // cases where a parent `Call` or `MethodCall` is using the item @@ -879,21 +878,20 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { let takes_arg_by_double_ref = self.func_takes_arg_by_double_ref(parent_expr, cmt.hir_id); - // compiler will automatically dereference field projection, so no need + // compiler will automatically dereference field or index projection, so no need // to suggest ampersand, but full identifier that includes projection is required - let has_field_projection = cmt - .place - .projections - .iter() - .any(|proj| matches!(proj.kind, ProjectionKind::Field(..))); + let has_field_or_index_projection = + cmt.place.projections.iter().any(|proj| { + matches!(proj.kind, ProjectionKind::Field(..) | ProjectionKind::Index) + }); // no need to bind again if the function doesn't take arg by double ref // and if the item is already a double ref let ident_sugg = if !call_args.is_empty() && !takes_arg_by_double_ref - && (self.closure_arg_is_double_ref || has_field_projection) + && (self.closure_arg_is_double_ref || has_field_or_index_projection) { - let ident = if has_field_projection { + let ident = if has_field_or_index_projection { ident_str_with_proj } else { ident_str @@ -953,9 +951,7 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { // handle `ProjectionKind::Deref` by removing one explicit deref // if no special case was detected (i.e.: suggest `*x` instead of `**x`) - if projections_handled { - self.closure_arg_is_double_ref = false; - } else { + if !projections_handled { let last_deref = cmt .place .projections From 917fdb11e45c5b2d0b43566d3736fc34ad917294 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Sat, 27 Nov 2021 20:32:23 +0100 Subject: [PATCH 24/25] Rewrite comment when handling special case for `ProjectionKind::Deref` --- clippy_utils/src/sugg.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/clippy_utils/src/sugg.rs b/clippy_utils/src/sugg.rs index 5dd49c1d26b9..e1d0920590b4 100644 --- a/clippy_utils/src/sugg.rs +++ b/clippy_utils/src/sugg.rs @@ -937,10 +937,11 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { // note: unable to trigger `Subslice` kind in tests ProjectionKind::Subslice => (), ProjectionKind::Deref => { - // explicit deref for arrays should be avoided in the suggestion - // i.e.: `|sub| *sub[1..4].len() == 3` is not expected + // Explicit derefs are typically handled later on, but + // some items do not need explicit deref, such as array accesses, + // so we mark them as already processed + // i.e.: don't suggest `*sub[1..4].len()` for `|sub| sub[1..4].len() == 3` if let ty::Ref(_, inner, _) = cmt.place.ty_before_projection(i).kind() { - // dereferencing an array (i.e.: `|sub| sub[1..4].len() == 3`) if matches!(inner.kind(), ty::Ref(_, innermost, _) if innermost.is_array()) { projections_handled = true; } From a8e7fed17264bd269c55e8224991d5bf0249472c Mon Sep 17 00:00:00 2001 From: ThibsG Date: Sun, 28 Nov 2021 09:50:34 +0100 Subject: [PATCH 25/25] Add a note about type annotation on closure param --- clippy_utils/src/sugg.rs | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-) diff --git a/clippy_utils/src/sugg.rs b/clippy_utils/src/sugg.rs index e1d0920590b4..872942685f0d 100644 --- a/clippy_utils/src/sugg.rs +++ b/clippy_utils/src/sugg.rs @@ -737,12 +737,14 @@ pub struct DerefClosure { /// such as explicit deref and borrowing cases. /// Returns `None` if no such use cases have been triggered in closure body /// -/// note: this only works on single line immutable closures with one exactly one input parameter. +/// note: this only works on single line immutable closures with exactly one input parameter. pub fn deref_closure_args<'tcx>(cx: &LateContext<'_>, closure: &'tcx hir::Expr<'_>) -> Option { if let hir::ExprKind::Closure(_, fn_decl, body_id, ..) = closure.kind { let closure_body = cx.tcx.hir().body(body_id); - // is closure arg a double reference (i.e.: `|x: &&i32| ...`) - let closure_arg_is_double_ref = if let TyKind::Rptr(_, MutTy { ty, .. }) = fn_decl.inputs[0].kind { + // is closure arg a type annotated double reference (i.e.: `|x: &&i32| ...`) + // a type annotation is present if param `kind` is different from `TyKind::Infer` + let closure_arg_is_type_annotated_double_ref = if let TyKind::Rptr(_, MutTy { ty, .. }) = fn_decl.inputs[0].kind + { matches!(ty.kind, TyKind::Rptr(_, MutTy { .. })) } else { false @@ -751,7 +753,7 @@ pub fn deref_closure_args<'tcx>(cx: &LateContext<'_>, closure: &'tcx hir::Expr<' let mut visitor = DerefDelegate { cx, closure_span: closure.span, - closure_arg_is_double_ref, + closure_arg_is_type_annotated_double_ref, next_pos: closure.span.lo(), suggestion_start: String::new(), applicability: Applicability::MaybeIncorrect, @@ -780,8 +782,8 @@ struct DerefDelegate<'a, 'tcx> { cx: &'a LateContext<'tcx>, /// The span of the input closure to adapt closure_span: Span, - /// Indicates if the arg of the closure is a double reference - closure_arg_is_double_ref: bool, + /// Indicates if the arg of the closure is a type annotated double reference + closure_arg_is_type_annotated_double_ref: bool, /// last position of the span to gradually build the suggestion next_pos: BytePos, /// starting part of the gradually built suggestion @@ -799,7 +801,7 @@ impl DerefDelegate<'_, 'tcx> { let end_span = Span::new(self.next_pos, self.closure_span.hi(), self.closure_span.ctxt(), None); let end_snip = snippet_with_applicability(self.cx, end_span, "..", &mut self.applicability); let sugg = format!("{}{}", self.suggestion_start, end_snip); - if self.closure_arg_is_double_ref { + if self.closure_arg_is_type_annotated_double_ref { sugg.replacen('&', "", 1) } else { sugg @@ -889,7 +891,7 @@ impl<'tcx> Delegate<'tcx> for DerefDelegate<'_, 'tcx> { // and if the item is already a double ref let ident_sugg = if !call_args.is_empty() && !takes_arg_by_double_ref - && (self.closure_arg_is_double_ref || has_field_or_index_projection) + && (self.closure_arg_is_type_annotated_double_ref || has_field_or_index_projection) { let ident = if has_field_or_index_projection { ident_str_with_proj