From 7a55407cc37ddb1bf1d3d95e52db7b6402259966 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Tue, 21 Sep 2021 14:54:19 +0200 Subject: [PATCH] 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 98ad7f3866f..25a2c48e269 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, + _ => (), }, - // handled previously - ProjectionKind::Index | - // note: unable to trigger `Subslice` kind in tests - ProjectionKind::Subslice => 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; + }, + // 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 c7c0b766015..6b8a614bbda 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 6b41da4f2f8..acaae650921 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 4313a2d526c..64e81c6b94a 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| simple_fn(**x)).is_none(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|x| simple_fn(*x))` +LL | let _ = v.iter().find(|x| deref_enough(**x)).is_none(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use `!_.any()` instead: `!v.iter().any(|x| deref_enough(*x))` -error: aborting due to 29 previous errors +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 _ = 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 diff --git a/tests/ui/search_is_some_fixable_some.fixed b/tests/ui/search_is_some_fixable_some.fixed index 833168cf242..7eb07d639a5 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 f9fb241de3f..b9ebd399a35 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 01b13cea2cd..b62cf9c4702 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