feat: needless_move lint

An implementation for the lint described in
rust-lang#11721

Author: dnbln

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

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,

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`
 }
 

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<HirId, Vec<UpvarId>>,
+    /// 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<HirId, Vec<(UpvarId, HirId, ty::BorrowKind)>>,
+}
+
+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) {}
+}