From d1b3fdf0ba631df47f1a52a92286a38146e4606a Mon Sep 17 00:00:00 2001 From: Dinu Blanovschi Date: Sat, 4 Nov 2023 17:10:53 +0100 Subject: [PATCH] feat: needless_move lint An implementation for the lint described in https://github.com/rust-lang/rust-clippy/issues/11721 --- CHANGELOG.md | 1 + clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + clippy_lints/src/needless_move.rs | 176 +++++++++++++++++++ tests/ui/needless_move.fixed | 261 +++++++++++++++++++++++++++++ tests/ui/needless_move.rs | 261 +++++++++++++++++++++++++++++ tests/ui/needless_move.stderr | 125 ++++++++++++++ 7 files changed, 827 insertions(+) create mode 100644 clippy_lints/src/needless_move.rs create mode 100644 tests/ui/needless_move.fixed create mode 100644 tests/ui/needless_move.rs create mode 100644 tests/ui/needless_move.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 87a96bdeba65..e2a0902579f6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5274,6 +5274,7 @@ Released 2018-09-13 [`needless_late_init`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_late_init [`needless_lifetimes`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_lifetimes [`needless_match`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_match +[`needless_move`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_move [`needless_option_as_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_option_as_deref [`needless_option_take`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_option_take [`needless_parens_on_range_literals`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_parens_on_range_literals diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 1a646ba38c35..b4e17edbb2c1 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -496,6 +496,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[ crate::needless_for_each::NEEDLESS_FOR_EACH_INFO, crate::needless_if::NEEDLESS_IF_INFO, crate::needless_late_init::NEEDLESS_LATE_INIT_INFO, + crate::needless_move::NEEDLESS_MOVE_INFO, crate::needless_parens_on_range_literals::NEEDLESS_PARENS_ON_RANGE_LITERALS_INFO, crate::needless_pass_by_ref_mut::NEEDLESS_PASS_BY_REF_MUT_INFO, crate::needless_pass_by_value::NEEDLESS_PASS_BY_VALUE_INFO, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index ab978a677c23..4e478b230743 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -234,6 +234,7 @@ mod needless_else; mod needless_for_each; mod needless_if; mod needless_late_init; +mod needless_move; mod needless_parens_on_range_literals; mod needless_pass_by_ref_mut; mod needless_pass_by_value; @@ -1066,6 +1067,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: }); store.register_late_pass(move |_| Box::new(manual_hash_one::ManualHashOne::new(msrv()))); store.register_late_pass(|_| Box::new(iter_without_into_iter::IterWithoutIntoIter)); + store.register_late_pass(|_| Box::new(needless_move::NeedlessMove)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/needless_move.rs b/clippy_lints/src/needless_move.rs new file mode 100644 index 000000000000..aed0a35c93c9 --- /dev/null +++ b/clippy_lints/src/needless_move.rs @@ -0,0 +1,176 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::sugg::DiagnosticExt; +use clippy_utils::ty::is_copy; +use rustc_data_structures::fx::FxIndexMap; +use rustc_errors::Applicability; +use rustc_hir::*; +use rustc_hir_typeck::expr_use_visitor as euv; +use rustc_infer::infer::TyCtxtInferExt; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::mir::FakeReadCause; +use rustc_middle::ty::{self, UpvarId}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; + +declare_clippy_lint! { + /// ### What it does + /// Checks for closures and `async` blocks where the `move` is not necessary. + /// E.g. all the values are captured by value into the closure / `async` block. + /// + /// ### Why is this bad? + /// Pedantry + /// ### Example + /// ```no_run + /// let a = String::new(); + /// let closure = move || { + /// drop(a); + /// }; + /// ``` + /// Use instead: + /// ```no_run + /// let a = String::new(); + /// let closure = || { + /// drop(a); + /// }; + /// ``` + #[clippy::version = "1.75.0"] + pub NEEDLESS_MOVE, + pedantic, + "checks for needless `move`s on closures / `async` blocks" +} + +declare_lint_pass!(NeedlessMove => [NEEDLESS_MOVE]); + +impl NeedlessMove { + fn check_closure<'tcx>(&self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, closure: &'tcx Closure<'tcx>) { + let CaptureBy::Value { move_kw } = closure.capture_clause else { + return; + }; + + // Collect moved & borrowed variables from the closure, which the closure *actually* needs. + let MovedVariablesCtxt { + moved_vars, + mut captured_vars, + } = { + let mut ctx = MovedVariablesCtxt { + captured_vars: Default::default(), + moved_vars: Default::default(), + }; + let body = cx.tcx.hir().body(closure.body); + let infcx = cx.tcx.infer_ctxt().build(); + euv::ExprUseVisitor::new(&mut ctx, &infcx, closure.def_id, cx.param_env, cx.typeck_results()) + .consume_body(body); + ctx + }; + + // Remove the captured vars which were also `move`d. + // See special case 1. below. + for (hir_id, _upvars) in moved_vars.iter() { + let Some(vars) = captured_vars.get_mut(hir_id) else { + continue; + }; + + if vars + .iter() + .any(|(_, hir_borrow_id, _)| is_copy(cx, cx.typeck_results().node_type(*hir_borrow_id))) + { + continue; + } + + captured_vars.remove(hir_id); + } + + let lint = |note_msg: &'static str| { + span_lint_and_then( + cx, + NEEDLESS_MOVE, + expr.span, + "you seem to use `move`, but the `move` is unnecessary", + |diag| { + diag.suggest_remove_item(cx, move_kw, "remove the `move`", Applicability::MachineApplicable); + diag.note(note_msg); + }, + ); + }; + + match (moved_vars.is_empty(), captured_vars.is_empty()) { + (true, true) => lint("there were no captured variables, so the `move` is unnecessary"), + (false, true) => lint("there were consumed variables, but no borrowed variables, so the `move` is unnecessary"), + (_, false) => { + // captured_vars is not empty, so `move` actually makes a difference and we + // should not remove it from this closure. + }, + } + } +} + +impl<'tcx> LateLintPass<'tcx> for NeedlessMove { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { + if expr.span.from_expansion() { + return; + } + + let ExprKind::Closure(closure) = &expr.kind else { + return; + }; + + self.check_closure(cx, expr, closure); + } +} + +struct MovedVariablesCtxt { + // for each base variable, we remember: + /// The places where it was captured (and consumed, e.g. moved into the closure). + moved_vars: FxIndexMap>, + /// The places where it was captured by reference (and not consumed). + /// If this vector is not empty, then the `move` keyword makes a difference. + /// There are a few special cases though, such as: + /// 1. We also handle the case in which there's both a borrow and a move of the + /// same value into the closure, e.g.: + /// + /// ```no_run + /// let x = String::new(); + /// let closure = move || { + /// let s = x.as_str(); // L1 + /// println!("{s}"); + /// drop(x); // L2 + /// }; + /// ``` + /// + /// In this case, the `x` `String` gets moved into the closure (because of L2), but + /// it is also borrowed prior to that at L1. + /// + /// How we handle this is by removing the entries that point to `x` if it was captured + /// by value (and therefore moved into the closure). + captured_vars: FxIndexMap>, +} + +impl MovedVariablesCtxt { + fn move_common(&mut self, cmt: &euv::PlaceWithHirId<'_>, _: HirId) { + if let euv::PlaceBase::Upvar(vid) = cmt.place.base { + self.moved_vars.entry(vid.var_path.hir_id).or_default().push(vid); + } + } + + fn borrow_common(&mut self, cmt: &euv::PlaceWithHirId<'_>, borrow_hir_id: HirId, bk: ty::BorrowKind) { + if let euv::PlaceBase::Upvar(vid) = cmt.place.base { + self.captured_vars + .entry(vid.var_path.hir_id) + .or_default() + .push((vid, borrow_hir_id, bk)); + } + } +} + +impl<'tcx> euv::Delegate<'tcx> for MovedVariablesCtxt { + fn consume(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, hir_id: HirId) { + self.move_common(cmt, hir_id); + } + + fn borrow(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, hir_id: HirId, bk: ty::BorrowKind) { + self.borrow_common(cmt, hir_id, bk); + } + + fn mutate(&mut self, _: &euv::PlaceWithHirId<'tcx>, _: HirId) {} + + fn fake_read(&mut self, _: &rustc_hir_typeck::expr_use_visitor::PlaceWithHirId<'tcx>, _: FakeReadCause, _: HirId) {} +} diff --git a/tests/ui/needless_move.fixed b/tests/ui/needless_move.fixed new file mode 100644 index 000000000000..5a3b8031e4b3 --- /dev/null +++ b/tests/ui/needless_move.fixed @@ -0,0 +1,261 @@ +//! To properly check that the `needless_move` lint is complete, go to the +//! `.fixed` file of this test and check that the code fails to compile if +//! any of the `move`s are removed. + +#![warn(clippy::needless_move)] + +#[derive(Copy, Clone)] +struct Copy; + +struct NonCopy; + +struct Composite { + copy: Copy, + non_copy: NonCopy, +} + +impl Composite { + fn new() -> Self { + Self { + copy: Copy, + non_copy: NonCopy, + } + } +} + +fn with_owned(_: T) {} +fn with_ref(_: &T) {} +fn with_ref_mut(_: &mut T) {} +fn assert_static(v: T) -> T { + v +} + +fn main() { + // doesn't trigger on non-move closures or async blocks + let a = NonCopy; + let b = Copy; + let closure = || { + with_owned(a); + with_owned(b); + }; + + let a = NonCopy; + let b = Copy; + let fut = async { + with_owned(a); + with_owned(b); + }; + + // triggers on move closures and async blocks which do not capture anything + let closure = assert_static(|| {}); + let fut = assert_static(async {}); + + // owned + NonCopy + let a = NonCopy; + let closure = assert_static(|| { + with_owned(a); + }); + + // owned + Copy + let a = Copy; + let closure = assert_static(move || { + with_owned(a); + }); + + // ref + NonCopy + let a = NonCopy; + let closure = assert_static(move || { + with_ref(&a); + }); + + // ref + Copy + let a = Copy; + let closure = assert_static(move || { + with_ref(&a); + }); + + // ref mut + NonCopy + let mut a = NonCopy; + let closure = assert_static(move || { + with_ref_mut(&mut a); + }); + + // ref mut + Copy + let mut a = Copy; + let closure = assert_static(move || { + with_ref_mut(&mut a); + }); + + // with async + + // doesn't trigger if not capturing with `move` + let a = NonCopy; + let b = Copy; + let fut = async { + with_owned(a); + with_owned(b); + }; + + // owned + non-copy + let a = NonCopy; + let fut = assert_static(async { + with_owned(a); + }); + + // owned + copy + let a = Copy; + let fut = assert_static(async move { + with_owned(a); + }); + + // ref + non-copy + let a = NonCopy; + let fut = assert_static(async move { + with_ref(&a); + }); + + // ref + copy + let a = Copy; + let fut = assert_static(async move { + with_ref(&a); + }); + + // ref mut + non-copy + let mut a = NonCopy; + let fut = assert_static(async move { + with_ref_mut(&mut a); + }); + + // ref mut + copy + let mut a = Copy; + let fut = assert_static(async move { + with_ref_mut(&mut a); + }); + + // triggers on ref + owned combinations + // ref + owned + non copy + let a = NonCopy; + let closure = assert_static(|| { + with_ref(&a); + with_owned(a); + }); + + // ref + owned + copy + let a = Copy; + let closure = assert_static(move || { + with_ref(&a); + with_owned(a); + }); + + // ref mut + owned + non copy + let mut a = NonCopy; + let closure = assert_static(|| { + with_ref_mut(&mut a); + with_owned(a); + }); + + // ref mut + owned + copy + let mut a = Copy; + let closure = assert_static(move || { + with_ref_mut(&mut a); + with_owned(a); + }); + + // ref + owned + non copy + other owned capture in between + let a = NonCopy; + let b = NonCopy; + let closure = assert_static(|| { + with_ref(&a); + with_owned(b); + with_owned(a); + }); + + // ref + owned + copy + other owned capture in between + let a = Copy; + let b = NonCopy; + let closure = assert_static(move || { + with_ref(&a); + with_owned(b); + with_owned(a); + }); + + // with composite structures + disjoint captures + + // owned + let a = Composite::new(); + let closure = assert_static(|| { + with_owned(a); + }); + + // ref + let a = Composite::new(); + let closure = assert_static(move || { + with_ref(&a); + }); + + // ref mut + let mut a = Composite::new(); + let closure = assert_static(move || { + with_ref_mut(&mut a); + }); + + // capturing only the copy part + // owned + let a = Composite::new(); + let closure = assert_static(move || { + with_owned(a.copy); + }); + + // ref + let a = Composite::new(); + let closure = assert_static(move || { + with_ref(&a.copy); + }); + + // ref mut + let mut a = Composite::new(); + let closure = assert_static(move || { + with_ref_mut(&mut a.copy); + }); + + // capturing only the non-copy part + // owned + let a = Composite::new(); + let closure = assert_static(|| { + with_owned(a.non_copy); + }); + + // ref + let a = Composite::new(); + let closure = assert_static(move || { + with_ref(&a.non_copy); + }); + + // ref mut + let mut a = Composite::new(); + let closure = assert_static(move || { + with_ref_mut(&mut a.non_copy); + }); + + // capturing both parts + // owned + let a = Composite::new(); + let closure = assert_static(move || { + with_owned(a.copy); + with_owned(a.non_copy); + }); + + // ref + let a = Composite::new(); + let closure = assert_static(move || { + with_ref(&a.copy); + with_ref(&a.non_copy); + }); + + // ref mut + let mut a = Composite::new(); + let closure = assert_static(move || { + with_ref_mut(&mut a.copy); + with_ref_mut(&mut a.non_copy); + }); +} diff --git a/tests/ui/needless_move.rs b/tests/ui/needless_move.rs new file mode 100644 index 000000000000..07acf40dfde0 --- /dev/null +++ b/tests/ui/needless_move.rs @@ -0,0 +1,261 @@ +//! To properly check that the `needless_move` lint is complete, go to the +//! `.fixed` file of this test and check that the code fails to compile if +//! any of the `move`s are removed. + +#![warn(clippy::needless_move)] + +#[derive(Copy, Clone)] +struct Copy; + +struct NonCopy; + +struct Composite { + copy: Copy, + non_copy: NonCopy, +} + +impl Composite { + fn new() -> Self { + Self { + copy: Copy, + non_copy: NonCopy, + } + } +} + +fn with_owned(_: T) {} +fn with_ref(_: &T) {} +fn with_ref_mut(_: &mut T) {} +fn assert_static(v: T) -> T { + v +} + +fn main() { + // doesn't trigger on non-move closures or async blocks + let a = NonCopy; + let b = Copy; + let closure = || { + with_owned(a); + with_owned(b); + }; + + let a = NonCopy; + let b = Copy; + let fut = async { + with_owned(a); + with_owned(b); + }; + + // triggers on move closures and async blocks which do not capture anything + let closure = assert_static(move || {}); + let fut = assert_static(async move {}); + + // owned + NonCopy + let a = NonCopy; + let closure = assert_static(move || { + with_owned(a); + }); + + // owned + Copy + let a = Copy; + let closure = assert_static(move || { + with_owned(a); + }); + + // ref + NonCopy + let a = NonCopy; + let closure = assert_static(move || { + with_ref(&a); + }); + + // ref + Copy + let a = Copy; + let closure = assert_static(move || { + with_ref(&a); + }); + + // ref mut + NonCopy + let mut a = NonCopy; + let closure = assert_static(move || { + with_ref_mut(&mut a); + }); + + // ref mut + Copy + let mut a = Copy; + let closure = assert_static(move || { + with_ref_mut(&mut a); + }); + + // with async + + // doesn't trigger if not capturing with `move` + let a = NonCopy; + let b = Copy; + let fut = async { + with_owned(a); + with_owned(b); + }; + + // owned + non-copy + let a = NonCopy; + let fut = assert_static(async move { + with_owned(a); + }); + + // owned + copy + let a = Copy; + let fut = assert_static(async move { + with_owned(a); + }); + + // ref + non-copy + let a = NonCopy; + let fut = assert_static(async move { + with_ref(&a); + }); + + // ref + copy + let a = Copy; + let fut = assert_static(async move { + with_ref(&a); + }); + + // ref mut + non-copy + let mut a = NonCopy; + let fut = assert_static(async move { + with_ref_mut(&mut a); + }); + + // ref mut + copy + let mut a = Copy; + let fut = assert_static(async move { + with_ref_mut(&mut a); + }); + + // triggers on ref + owned combinations + // ref + owned + non copy + let a = NonCopy; + let closure = assert_static(move || { + with_ref(&a); + with_owned(a); + }); + + // ref + owned + copy + let a = Copy; + let closure = assert_static(move || { + with_ref(&a); + with_owned(a); + }); + + // ref mut + owned + non copy + let mut a = NonCopy; + let closure = assert_static(move || { + with_ref_mut(&mut a); + with_owned(a); + }); + + // ref mut + owned + copy + let mut a = Copy; + let closure = assert_static(move || { + with_ref_mut(&mut a); + with_owned(a); + }); + + // ref + owned + non copy + other owned capture in between + let a = NonCopy; + let b = NonCopy; + let closure = assert_static(move || { + with_ref(&a); + with_owned(b); + with_owned(a); + }); + + // ref + owned + copy + other owned capture in between + let a = Copy; + let b = NonCopy; + let closure = assert_static(move || { + with_ref(&a); + with_owned(b); + with_owned(a); + }); + + // with composite structures + disjoint captures + + // owned + let a = Composite::new(); + let closure = assert_static(move || { + with_owned(a); + }); + + // ref + let a = Composite::new(); + let closure = assert_static(move || { + with_ref(&a); + }); + + // ref mut + let mut a = Composite::new(); + let closure = assert_static(move || { + with_ref_mut(&mut a); + }); + + // capturing only the copy part + // owned + let a = Composite::new(); + let closure = assert_static(move || { + with_owned(a.copy); + }); + + // ref + let a = Composite::new(); + let closure = assert_static(move || { + with_ref(&a.copy); + }); + + // ref mut + let mut a = Composite::new(); + let closure = assert_static(move || { + with_ref_mut(&mut a.copy); + }); + + // capturing only the non-copy part + // owned + let a = Composite::new(); + let closure = assert_static(move || { + with_owned(a.non_copy); + }); + + // ref + let a = Composite::new(); + let closure = assert_static(move || { + with_ref(&a.non_copy); + }); + + // ref mut + let mut a = Composite::new(); + let closure = assert_static(move || { + with_ref_mut(&mut a.non_copy); + }); + + // capturing both parts + // owned + let a = Composite::new(); + let closure = assert_static(move || { + with_owned(a.copy); + with_owned(a.non_copy); + }); + + // ref + let a = Composite::new(); + let closure = assert_static(move || { + with_ref(&a.copy); + with_ref(&a.non_copy); + }); + + // ref mut + let mut a = Composite::new(); + let closure = assert_static(move || { + with_ref_mut(&mut a.copy); + with_ref_mut(&mut a.non_copy); + }); +} diff --git a/tests/ui/needless_move.stderr b/tests/ui/needless_move.stderr new file mode 100644 index 000000000000..ef6c1d720079 --- /dev/null +++ b/tests/ui/needless_move.stderr @@ -0,0 +1,125 @@ +error: you seem to use `move`, but the `move` is unnecessary + --> $DIR/needless_move.rs:50:33 + | +LL | let closure = assert_static(move || {}); + | -----^^^^^ + | | + | help: remove the `move` + | + = note: there were no captured variables, so the `move` is unnecessary + = note: `-D clippy::needless-move` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::needless_move)]` + +error: you seem to use `move`, but the `move` is unnecessary + --> $DIR/needless_move.rs:51:29 + | +LL | let fut = assert_static(async move {}); + | ^^^^^^-----^^ + | | + | help: remove the `move` + | + = note: there were no captured variables, so the `move` is unnecessary + +error: you seem to use `move`, but the `move` is unnecessary + --> $DIR/needless_move.rs:55:33 + | +LL | let closure = assert_static(move || { + | ^---- + | | + | _________________________________help: remove the `move` + | | +LL | | with_owned(a); +LL | | }); + | |_____^ + | + = note: there were consumed variables, but no borrowed variables, so the `move` is unnecessary + +error: you seem to use `move`, but the `move` is unnecessary + --> $DIR/needless_move.rs:101:29 + | +LL | let fut = assert_static(async move { + | ^ ----- help: remove the `move` + | _____________________________| + | | +LL | | with_owned(a); +LL | | }); + | |_____^ + | + = note: there were consumed variables, but no borrowed variables, so the `move` is unnecessary + +error: you seem to use `move`, but the `move` is unnecessary + --> $DIR/needless_move.rs:138:33 + | +LL | let closure = assert_static(move || { + | ^---- + | | + | _________________________________help: remove the `move` + | | +LL | | with_ref(&a); +LL | | with_owned(a); +LL | | }); + | |_____^ + | + = note: there were consumed variables, but no borrowed variables, so the `move` is unnecessary + +error: you seem to use `move`, but the `move` is unnecessary + --> $DIR/needless_move.rs:152:33 + | +LL | let closure = assert_static(move || { + | ^---- + | | + | _________________________________help: remove the `move` + | | +LL | | with_ref_mut(&mut a); +LL | | with_owned(a); +LL | | }); + | |_____^ + | + = note: there were consumed variables, but no borrowed variables, so the `move` is unnecessary + +error: you seem to use `move`, but the `move` is unnecessary + --> $DIR/needless_move.rs:167:33 + | +LL | let closure = assert_static(move || { + | ^---- + | | + | _________________________________help: remove the `move` + | | +LL | | with_ref(&a); +LL | | with_owned(b); +LL | | with_owned(a); +LL | | }); + | |_____^ + | + = note: there were consumed variables, but no borrowed variables, so the `move` is unnecessary + +error: you seem to use `move`, but the `move` is unnecessary + --> $DIR/needless_move.rs:186:33 + | +LL | let closure = assert_static(move || { + | ^---- + | | + | _________________________________help: remove the `move` + | | +LL | | with_owned(a); +LL | | }); + | |_____^ + | + = note: there were consumed variables, but no borrowed variables, so the `move` is unnecessary + +error: you seem to use `move`, but the `move` is unnecessary + --> $DIR/needless_move.rs:224:33 + | +LL | let closure = assert_static(move || { + | ^---- + | | + | _________________________________help: remove the `move` + | | +LL | | with_owned(a.non_copy); +LL | | }); + | |_____^ + | + = note: there were consumed variables, but no borrowed variables, so the `move` is unnecessary + +error: aborting due to 9 previous errors +