[iter_overeager_cloned
]: detect .cloned().map() and .cloned().for_each()
key idea: for `f` in `.map(f)` and `.for_each(f)`: 1. `f` must be a closure with one parameter 2. don't lint if mutable paramter in clsure `f`: `|mut x| ...` 3. don't lint if parameter is moved
This commit is contained in:
parent
b7cad50eca
commit
e440065a0f
@ -1,14 +1,18 @@
|
|||||||
use clippy_utils::diagnostics::span_lint_and_then;
|
use clippy_utils::diagnostics::span_lint_and_then;
|
||||||
use clippy_utils::source::snippet_opt;
|
use clippy_utils::source::snippet_opt;
|
||||||
use clippy_utils::ty::{implements_trait, is_copy};
|
use clippy_utils::ty::{implements_trait, is_copy};
|
||||||
|
use rustc_ast::BindingAnnotation;
|
||||||
use rustc_errors::Applicability;
|
use rustc_errors::Applicability;
|
||||||
use rustc_hir::{Expr, ExprKind};
|
use rustc_hir::{Body, Expr, ExprKind, HirId, HirIdSet, PatKind};
|
||||||
|
use rustc_hir_typeck::expr_use_visitor::{Delegate, ExprUseVisitor, PlaceBase, PlaceWithHirId};
|
||||||
use rustc_lint::LateContext;
|
use rustc_lint::LateContext;
|
||||||
use rustc_middle::ty;
|
use rustc_middle::mir::{FakeReadCause, Mutability};
|
||||||
|
use rustc_middle::ty::{self, BorrowKind};
|
||||||
use rustc_span::sym;
|
use rustc_span::sym;
|
||||||
|
|
||||||
use super::ITER_OVEREAGER_CLONED;
|
use super::ITER_OVEREAGER_CLONED;
|
||||||
use crate::redundant_clone::REDUNDANT_CLONE;
|
use crate::redundant_clone::REDUNDANT_CLONE;
|
||||||
|
use crate::rustc_trait_selection::infer::TyCtxtInferExt;
|
||||||
|
|
||||||
#[derive(Clone, Copy)]
|
#[derive(Clone, Copy)]
|
||||||
pub(super) enum Op<'a> {
|
pub(super) enum Op<'a> {
|
||||||
@ -16,6 +20,10 @@ pub(super) enum Op<'a> {
|
|||||||
// e.g. `count`
|
// e.g. `count`
|
||||||
RmCloned,
|
RmCloned,
|
||||||
|
|
||||||
|
// rm `.cloned()`
|
||||||
|
// e.g. `map` `for_each`
|
||||||
|
NeedlessMove(&'a str, &'a Expr<'a>),
|
||||||
|
|
||||||
// later `.cloned()`
|
// later `.cloned()`
|
||||||
// and add `&` to the parameter of closure parameter
|
// and add `&` to the parameter of closure parameter
|
||||||
// e.g. `find` `filter`
|
// e.g. `find` `filter`
|
||||||
@ -51,8 +59,46 @@ pub(super) fn check<'tcx>(
|
|||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
if let Op::NeedlessMove(_, expr) = op {
|
||||||
|
let rustc_hir::ExprKind::Closure(closure) = expr.kind else { return } ;
|
||||||
|
let body @ Body { params: [p], .. } = cx.tcx.hir().body(closure.body) else { return };
|
||||||
|
let mut delegate = MoveDelegate {used_move : HirIdSet::default()};
|
||||||
|
let infcx = cx.tcx.infer_ctxt().build();
|
||||||
|
|
||||||
|
ExprUseVisitor::new(
|
||||||
|
&mut delegate,
|
||||||
|
&infcx,
|
||||||
|
closure.body.hir_id.owner.def_id,
|
||||||
|
cx.param_env,
|
||||||
|
cx.typeck_results(),
|
||||||
|
)
|
||||||
|
.consume_body(body);
|
||||||
|
|
||||||
|
let mut to_be_discarded = false;
|
||||||
|
|
||||||
|
p.pat.walk(|it| {
|
||||||
|
if delegate.used_move.contains(&it.hir_id){
|
||||||
|
to_be_discarded = true;
|
||||||
|
return false;
|
||||||
|
}
|
||||||
|
|
||||||
|
match it.kind {
|
||||||
|
PatKind::Binding(BindingAnnotation(_, Mutability::Mut), _, _, _)
|
||||||
|
| PatKind::Ref(_, Mutability::Mut) => {
|
||||||
|
to_be_discarded = true;
|
||||||
|
false
|
||||||
|
}
|
||||||
|
_ => { true }
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
if to_be_discarded {
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
let (lint, msg, trailing_clone) = match op {
|
let (lint, msg, trailing_clone) = match op {
|
||||||
Op::RmCloned => (REDUNDANT_CLONE, "unneeded cloning of iterator items", ""),
|
Op::RmCloned | Op::NeedlessMove(_, _) => (REDUNDANT_CLONE, "unneeded cloning of iterator items", ""),
|
||||||
Op::LaterCloned | Op::FixClosure(_, _) => (ITER_OVEREAGER_CLONED, "unnecessarily eager cloning of iterator items", ".cloned()"),
|
Op::LaterCloned | Op::FixClosure(_, _) => (ITER_OVEREAGER_CLONED, "unnecessarily eager cloning of iterator items", ".cloned()"),
|
||||||
};
|
};
|
||||||
|
|
||||||
@ -83,8 +129,33 @@ pub(super) fn check<'tcx>(
|
|||||||
diag.span_suggestion(replace_span, "try", snip, Applicability::MachineApplicable);
|
diag.span_suggestion(replace_span, "try", snip, Applicability::MachineApplicable);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
Op::NeedlessMove(_, _) => {
|
||||||
|
let method_span = expr.span.with_lo(cloned_call.span.hi());
|
||||||
|
if let Some(snip) = snippet_opt(cx, method_span) {
|
||||||
|
let replace_span = expr.span.with_lo(cloned_recv.span.hi());
|
||||||
|
diag.span_suggestion(replace_span, "try", snip, Applicability::MaybeIncorrect);
|
||||||
|
}
|
||||||
|
}
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
struct MoveDelegate {
|
||||||
|
used_move: HirIdSet,
|
||||||
|
}
|
||||||
|
|
||||||
|
impl<'tcx> Delegate<'tcx> for MoveDelegate {
|
||||||
|
fn consume(&mut self, place_with_id: &PlaceWithHirId<'tcx>, _: HirId) {
|
||||||
|
if let PlaceBase::Local(l) = place_with_id.place.base {
|
||||||
|
self.used_move.insert(l);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
fn borrow(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId, _: BorrowKind) {}
|
||||||
|
|
||||||
|
fn mutate(&mut self, _: &PlaceWithHirId<'tcx>, _: HirId) {}
|
||||||
|
|
||||||
|
fn fake_read(&mut self, _: &PlaceWithHirId<'tcx>, _: FakeReadCause, _: HirId) {}
|
||||||
|
}
|
||||||
|
@ -4001,9 +4001,11 @@ fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
|
|||||||
manual_try_fold::check(cx, expr, init, acc, call_span, &self.msrv);
|
manual_try_fold::check(cx, expr, init, acc, call_span, &self.msrv);
|
||||||
unnecessary_fold::check(cx, expr, init, acc, span);
|
unnecessary_fold::check(cx, expr, init, acc, span);
|
||||||
},
|
},
|
||||||
("for_each", [_]) => {
|
("for_each", [arg]) => {
|
||||||
if let Some(("inspect", _, [_], span2, _)) = method_call(recv) {
|
match method_call(recv) {
|
||||||
inspect_for_each::check(cx, expr, span2);
|
Some(("inspect", _, [_], span2, _)) => inspect_for_each::check(cx, expr, span2),
|
||||||
|
Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, iter_overeager_cloned::Op::NeedlessMove(name, arg), false),
|
||||||
|
_ => {}
|
||||||
}
|
}
|
||||||
},
|
},
|
||||||
("get", [arg]) => {
|
("get", [arg]) => {
|
||||||
@ -4038,8 +4040,10 @@ fn check_methods<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
|
|||||||
(name @ ("map" | "map_err"), [m_arg]) => {
|
(name @ ("map" | "map_err"), [m_arg]) => {
|
||||||
if name == "map" {
|
if name == "map" {
|
||||||
map_clone::check(cx, expr, recv, m_arg, &self.msrv);
|
map_clone::check(cx, expr, recv, m_arg, &self.msrv);
|
||||||
if let Some((map_name @ ("iter" | "into_iter"), recv2, _, _, _)) = method_call(recv) {
|
match method_call(recv) {
|
||||||
iter_kv_map::check(cx, map_name, expr, recv2, m_arg);
|
Some((map_name @ ("iter" | "into_iter"), recv2, _, _, _)) => iter_kv_map::check(cx, map_name, expr, recv2, m_arg),
|
||||||
|
Some(("cloned", recv2, [], _, _)) => iter_overeager_cloned::check(cx, expr, recv, recv2, iter_overeager_cloned::Op::NeedlessMove(name, m_arg), false),
|
||||||
|
_ => {}
|
||||||
}
|
}
|
||||||
} else {
|
} else {
|
||||||
map_err_ignore::check(cx, expr, m_arg);
|
map_err_ignore::check(cx, expr, m_arg);
|
||||||
|
@ -56,14 +56,12 @@ fn main() {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Not implemented yet
|
let _ = vec.iter().map(|x| x.len());
|
||||||
let _ = vec.iter().cloned().map(|x| x.len());
|
|
||||||
|
|
||||||
// This would fail if changed.
|
// This would fail if changed.
|
||||||
let _ = vec.iter().cloned().map(|x| x + "2");
|
let _ = vec.iter().cloned().map(|x| x + "2");
|
||||||
|
|
||||||
// Not implemented yet
|
let _ = vec.iter().for_each(|x| assert!(!x.is_empty()));
|
||||||
let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty()));
|
|
||||||
|
|
||||||
// Not implemented yet
|
// Not implemented yet
|
||||||
let _ = vec.iter().cloned().all(|x| x.len() == 1);
|
let _ = vec.iter().cloned().all(|x| x.len() == 1);
|
||||||
|
@ -57,13 +57,11 @@ fn bar<'a>(iter: impl Iterator<Item = &'a S<'a>> + 'a, target: String) -> impl I
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Not implemented yet
|
|
||||||
let _ = vec.iter().cloned().map(|x| x.len());
|
let _ = vec.iter().cloned().map(|x| x.len());
|
||||||
|
|
||||||
// This would fail if changed.
|
// This would fail if changed.
|
||||||
let _ = vec.iter().cloned().map(|x| x + "2");
|
let _ = vec.iter().cloned().map(|x| x + "2");
|
||||||
|
|
||||||
// Not implemented yet
|
|
||||||
let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty()));
|
let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty()));
|
||||||
|
|
||||||
// Not implemented yet
|
// Not implemented yet
|
||||||
|
@ -130,5 +130,21 @@ LL | iter.cloned().filter(move |S { a, b }| **a == 1 && b == &target
|
|||||||
| |
|
| |
|
||||||
| help: try: `.filter(move |&S { a, b }| **a == 1 && b == &target).cloned()`
|
| help: try: `.filter(move |&S { a, b }| **a == 1 && b == &target).cloned()`
|
||||||
|
|
||||||
error: aborting due to 15 previous errors
|
error: unneeded cloning of iterator items
|
||||||
|
--> $DIR/iter_overeager_cloned.rs:60:13
|
||||||
|
|
|
||||||
|
LL | let _ = vec.iter().cloned().map(|x| x.len());
|
||||||
|
| ^^^^^^^^^^--------------------------
|
||||||
|
| |
|
||||||
|
| help: try: `.map(|x| x.len())`
|
||||||
|
|
||||||
|
error: unneeded cloning of iterator items
|
||||||
|
--> $DIR/iter_overeager_cloned.rs:65:13
|
||||||
|
|
|
||||||
|
LL | let _ = vec.iter().cloned().for_each(|x| assert!(!x.is_empty()));
|
||||||
|
| ^^^^^^^^^^----------------------------------------------
|
||||||
|
| |
|
||||||
|
| help: try: `.for_each(|x| assert!(!x.is_empty()))`
|
||||||
|
|
||||||
|
error: aborting due to 17 previous errors
|
||||||
|
|
||||||
|
Loading…
Reference in New Issue
Block a user