From 1176b8e5e9e697978ad563924f1561b52b330c07 Mon Sep 17 00:00:00 2001 From: ThibsG Date: Mon, 1 Nov 2021 09:11:43 +0100 Subject: [PATCH] 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 96e040d9261..7baae4faa23 100644 --- a/clippy_lints/src/methods/search_is_some.rs +++ b/clippy_lints/src/methods/search_is_some.rs @@ -7,7 +7,7 @@ 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 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 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, + }; - if visitor.suggestion_start.is_empty() { - None - } else { - Some(ClosureSugg { - applicability: visitor.applicability, - suggestion: visitor.finish(), - }) + 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, @@ -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 @@ fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) { 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 @@ fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) { // - `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 @@ fn borrow(&mut self, cmt: &PlaceWithHirId<'tcx>, _: HirId, _: ty::BorrowKind) { // 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 240a6957893..adc6b2739c4 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 7e8fc14a7bd..f0be2be4788 100644 --- a/tests/ui/search_is_some_fixable_none.rs +++ b/tests/ui/search_is_some_fixable_none.rs @@ -122,9 +122,12 @@ fn more_projections() { 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 c78830a3b5d..910d5ad37b8 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 267324cbe6a..2799ae0a98b 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 53f80e2c87e..bf9d50d9057 100644 --- a/tests/ui/search_is_some_fixable_some.rs +++ b/tests/ui/search_is_some_fixable_some.rs @@ -121,9 +121,12 @@ fn more_projections() { 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 e8784bba6eb..71680bc8d2a 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