New lint: needless_move

CHANGELOG.md

@@ -5523,6 +5523,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

@@ -527,6 +527,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/endian_bytes.rs

@@ -203,7 +203,7 @@ fn maybe_lint_endian_bytes(cx: &LateContext<'_>, expr: &Expr<'_>, prefix: Prefix
             lint.as_name(prefix),
             if prefix == Prefix::To { " method" } else { "" },
         ),
-        move |diag| {
+        |diag| {
             if let Some(help) = help {
                 diag.help(help);
             }

clippy_lints/src/fallible_impl_from.rs

@@ -116,7 +116,7 @@ fn lint_impl_body(cx: &LateContext<'_>, impl_span: Span, impl_items: &[hir::Impl
                     FALLIBLE_IMPL_FROM,
                     impl_span,
                     "consider implementing `TryFrom` instead",
-                    move |diag| {
+                    |diag| {
                         diag.help(
                             "`From` is intended for infallible conversions only. \
                             Use `TryFrom` if there's a possibility for the conversion to fail",

clippy_lints/src/lib.rs

@@ -254,6 +254,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;
@@ -716,7 +717,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     store.register_late_pass(move |_| Box::new(from_over_into::FromOverInto::new(msrv())));
     store.register_late_pass(move |_| Box::new(use_self::UseSelf::new(msrv())));
     store.register_late_pass(move |_| Box::new(missing_const_for_fn::MissingConstForFn::new(msrv())));
-    store.register_late_pass(move |_| Box::new(needless_question_mark::NeedlessQuestionMark));
+    store.register_late_pass(|_| Box::new(needless_question_mark::NeedlessQuestionMark));
     store.register_late_pass(move |_| Box::new(casts::Casts::new(msrv())));
     store.register_early_pass(move || Box::new(unnested_or_patterns::UnnestedOrPatterns::new(msrv())));
     store.register_late_pass(|_| Box::new(size_of_in_element_count::SizeOfInElementCount));
@@ -784,7 +785,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     store.register_late_pass(|_| Box::new(mixed_read_write_in_expression::EvalOrderDependence));
     store.register_late_pass(move |_| Box::new(missing_doc::MissingDoc::new(missing_docs_in_crate_items)));
     store.register_late_pass(|_| Box::new(missing_inline::MissingInline));
-    store.register_late_pass(move |_| Box::new(exhaustive_items::ExhaustiveItems));
+    store.register_late_pass(|_| Box::new(exhaustive_items::ExhaustiveItems));
     store.register_late_pass(|_| Box::new(match_result_ok::MatchResultOk));
     store.register_late_pass(|_| Box::new(partialeq_ne_impl::PartialEqNeImpl));
     store.register_late_pass(|_| Box::new(unused_io_amount::UnusedIoAmount));
@@ -932,7 +933,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     store.register_late_pass(|_| Box::new(from_str_radix_10::FromStrRadix10));
     store.register_late_pass(move |_| Box::new(if_then_some_else_none::IfThenSomeElseNone::new(msrv())));
     store.register_late_pass(|_| Box::new(bool_assert_comparison::BoolAssertComparison));
-    store.register_early_pass(move || Box::new(module_style::ModStyle));
+    store.register_early_pass(|| Box::new(module_style::ModStyle));
     store.register_late_pass(|_| Box::<unused_async::UnusedAsync>::default());
     store.register_late_pass(move |_| Box::new(disallowed_types::DisallowedTypes::new(disallowed_types.clone())));
     store.register_late_pass(move |_| {
@@ -942,9 +943,9 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     });
     store.register_early_pass(move || Box::new(disallowed_script_idents::DisallowedScriptIdents::new(allowed_scripts)));
     store.register_late_pass(|_| Box::new(strlen_on_c_strings::StrlenOnCStrings));
-    store.register_late_pass(move |_| Box::new(self_named_constructors::SelfNamedConstructors));
-    store.register_late_pass(move |_| Box::new(iter_not_returning_iterator::IterNotReturningIterator));
-    store.register_late_pass(move |_| Box::new(manual_assert::ManualAssert));
+    store.register_late_pass(|_| Box::new(self_named_constructors::SelfNamedConstructors));
+    store.register_late_pass(|_| Box::new(iter_not_returning_iterator::IterNotReturningIterator));
+    store.register_late_pass(|_| Box::new(manual_assert::ManualAssert));
     store.register_late_pass(move |_| {
         Box::new(non_send_fields_in_send_ty::NonSendFieldInSendTy::new(
             enable_raw_pointer_heuristic_for_send,
@@ -1131,6 +1132,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     store.register_late_pass(|_| Box::new(zero_repeat_side_effects::ZeroRepeatSideEffects));
     store.register_late_pass(|_| Box::new(manual_unwrap_or_default::ManualUnwrapOrDefault));
     store.register_late_pass(|_| Box::new(integer_division_remainder_used::IntegerDivisionRemainderUsed));
+    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/loops/manual_memcpy.rs

@@ -409,7 +409,7 @@ fn get_assignments<'a, 'tcx>(
     // just increases complexity. (cc #3188 and #4193)
     stmts
         .iter()
-        .filter_map(move |stmt| match stmt.kind {
+        .filter_map(|stmt| match stmt.kind {
             StmtKind::Let(..) | StmtKind::Item(..) => None,
             StmtKind::Expr(e) | StmtKind::Semi(e) => Some(e),
         })

clippy_lints/src/needless_move.rs

@@ -0,0 +1,233 @@
+//! This lint works by looking at the `min_captures` that `rustc` uses,
+//! and checks that for the expression `capture_kind_expr_id`, it would
+//! actually borrow normally, if it weren't for the move keyword.
+//!
+//! In such cases, the move keyword changes the semantics of the code (e.g.
+//! without it that capture would be a normal by reference capture, but with
+//! move it would get captured by value, and therefore we do not remove the `move`
+//! keyword from the closure).
+//!
+//! A small caveat for the approach above:
+//! 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.
+//!
+//! `rustc`, in the presence of the `move` keyword automatically assumes that if
+//! it borrows a value, it's going to move it into the closure (in the example above at L1,
+//! so `capture_kind_expr_id` would point to the use on L1), but here, in the case
+//! of this lint, we should behave a little differently, namely we should first look
+//! at all the locations where a place is captured, and if any of them actually moves it,
+//! the closure would consume the value.
+//!
+//! The logic for this is handled in `MovedVariablesCtxt::get_required_kind`, where we
+//! try to infer the actual min capture kind needed.
+
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::sugg::DiagExt;
+use rustc_errors::Applicability;
+use rustc_hir::{CaptureBy, Expr, ExprKind, HirId};
+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;
+use rustc_middle::ty::UpvarCapture;
+use rustc_session::declare_lint_pass;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for closures and `async` blocks where capturing by value (the `move` keyword) is unnecessary.
+    /// E.g. all the values are captured by value into the closure / `async` block.
+    ///
+    /// ### Why is this bad?
+    /// This pattern is not necessarily bad, but sometimes the `move` keyword is unnecessary,
+    /// for example when there's a closure which captures some variables by reference, so
+    /// the programmer adds the `move` keyword to move the variables into the closure, but
+    /// then later decides that he no longer needs the variables in question, so he removes them
+    /// from the body of the closure, but forgets to also remove the `move` keyword.
+    ///
+    /// This is really just a strict coding style issue.
+    ///
+    /// ### Caveats
+    /// There are some cases where this lint will suggest removing the `move` keyword,
+    /// but it would be considered idiomatic to keep it.
+    ///
+    /// For example, the closure passed to `std::thread::spawn` is usually always written
+    /// with the `move` keyword, even if it's not necessary:
+    ///
+    /// ```no_run
+    /// # fn function_that_does_something_with(_: String) {}
+    /// let a = String::new();
+    /// std::thread::spawn(move || {
+    ///    // ...
+    ///   function_that_does_something_with(a); // a is moved into the closure
+    /// });
+    /// ```
+    ///
+    /// ### 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.76.0"]
+    pub NEEDLESS_MOVE,
+    restriction,
+    "checks for needless `move`s on closures / `async` blocks"
+}
+
+declare_lint_pass!(NeedlessMove => [NEEDLESS_MOVE]);
+
+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;
+        };
+
+        let CaptureBy::Value { move_kw } = closure.capture_clause else {
+            return;
+        };
+
+        if move_kw.is_dummy() {
+            // async fn ...() {} convert the body to an `async move {}` block,
+            // with a DUMMY_SP for the move_kw
+            return;
+        }
+
+        // Collect moved & borrowed variables from the closure, which the closure *actually* needs.
+        let ctx = {
+            let mut ctx = MovedVariablesCtxt::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
+        };
+
+        let mut lint_result = LintResult::NothingCaptured;
+
+        for captured_place in cx.typeck_results().closure_min_captures_flattened(closure.def_id) {
+            let place = &captured_place.place;
+            if let Some(ck_expr_id) = captured_place.info.capture_kind_expr_id {
+                let required_ck = ctx.get_required_kind(place, ck_expr_id);
+                match required_ck {
+                    UpvarCapture::ByRef(_) => {
+                        // no matter what the old `lint_result` is, we keep the move.
+                        lint_result = LintResult::NeedMove;
+                    },
+                    UpvarCapture::ByValue => {
+                        lint_result = match lint_result {
+                            LintResult::NothingCaptured | LintResult::Consumed => LintResult::Consumed,
+                            LintResult::NeedMove => LintResult::NeedMove,
+                        }
+                    },
+                }
+            }
+        }
+
+        let note_msg = match lint_result {
+            LintResult::NothingCaptured => "there are no captured variables, so the `move` keyword is unnecessary",
+            LintResult::Consumed => {
+                "there are consumed variables, but no borrowed variables, so the `move` keyword is unnecessary"
+            },
+            LintResult::NeedMove => {
+                // there was a value which would be borrowed if it weren't for the move keyword,
+                // so we should keep it, as removing it would change semantics.
+                return;
+            },
+        };
+
+        span_lint_and_then(
+            cx,
+            NEEDLESS_MOVE,
+            expr.span,
+            "this closure does not need to capture by value",
+            |diag| {
+                diag.suggest_remove_item(
+                    cx,
+                    move_kw,
+                    "remove the `move` keyword",
+                    Applicability::MachineApplicable,
+                );
+                diag.note(note_msg);
+            },
+        );
+    }
+}
+
+enum LintResult {
+    /// do not remove the `move` keyword.
+    NeedMove,
+    Consumed,
+    NothingCaptured,
+}
+
+#[derive(Debug, Default)]
+struct MovedVariablesCtxt<'tcx> {
+    // for each base variable, we remember:
+    /// The places where it was captured (and consumed, e.g. moved into the closure).
+    moved: Vec<(euv::Place<'tcx>, HirId)>,
+    /// The places where it was captured by reference (and not consumed).
+    captured: Vec<(euv::Place<'tcx>, HirId, ty::BorrowKind)>,
+}
+
+impl<'tcx> MovedVariablesCtxt<'tcx> {
+    fn get_required_kind(&self, place: &euv::Place<'tcx>, ref_hir_id: HirId) -> UpvarCapture {
+        if self
+            .moved
+            .iter()
+            .any(|upvar_ref| upvar_ref.0 == *place || upvar_ref.1 == ref_hir_id)
+        {
+            UpvarCapture::ByValue
+        } else {
+            self.captured
+                .iter()
+                .find(|upvar_ref| upvar_ref.1 == ref_hir_id)
+                .map_or(UpvarCapture::ByRef(ty::BorrowKind::ImmBorrow), |it| {
+                    UpvarCapture::ByRef(it.2)
+                })
+        }
+    }
+}
+
+impl<'tcx> euv::Delegate<'tcx> for MovedVariablesCtxt<'tcx> {
+    fn consume(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, hir_id: HirId) {
+        if let euv::PlaceBase::Upvar(_) = cmt.place.base {
+            self.moved.push((cmt.place.clone(), hir_id));
+        }
+    }
+
+    fn borrow(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, hir_id: HirId, bk: ty::BorrowKind) {
+        if let euv::PlaceBase::Upvar(_) = cmt.place.base {
+            self.captured.push((cmt.place.clone(), hir_id, bk));
+        }
+    }
+
+    fn mutate(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, hir_id: HirId) {
+        self.borrow(cmt, hir_id, ty::BorrowKind::MutBorrow);
+    }
+
+    fn fake_read(&mut self, _: &euv::PlaceWithHirId<'tcx>, _: FakeReadCause, _: HirId) {}
+}

clippy_lints/src/panic_in_result_fn.rs

@@ -84,7 +84,7 @@ fn lint_impl_body<'tcx>(cx: &LateContext<'tcx>, impl_span: Span, body: &'tcx hir
             PANIC_IN_RESULT_FN,
             impl_span,
             "used `panic!()` or assertion in a function that returns `Result`",
-            move |diag| {
+            |diag| {
                 diag.help(
                     "`panic!()` or assertions should not be used in a function that returns `Result` as `Result` is expected to return an error instead of crashing",
                 );

clippy_lints/src/unwrap_in_result.rs

@@ -106,7 +106,7 @@ fn lint_impl_body<'tcx>(cx: &LateContext<'tcx>, impl_span: Span, impl_item: &'tc
                 UNWRAP_IN_RESULT,
                 impl_span,
                 "used unwrap or expect in a function that returns result or option",
-                move |diag| {
+                |diag| {
                     diag.help("unwrap and expect should not be used in a function that returns result or option");
                     diag.span_note(result, "potential non-recoverable error(s)");
                 },