Add needless_pass_by_ref lint

CHANGELOG.md

@@ -5047,6 +5047,7 @@ Released 2018-09-13
 [`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
+[`needless_pass_by_ref_mut`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_ref_mut
 [`needless_pass_by_value`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_pass_by_value
 [`needless_pub_self`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_pub_self
 [`needless_question_mark`]: https://rust-lang.github.io/rust-clippy/master/index.html#needless_question_mark

clippy_lints/src/declared_lints.rs

@@ -470,6 +470,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
     crate::needless_if::NEEDLESS_IF_INFO,
     crate::needless_late_init::NEEDLESS_LATE_INIT_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,
     crate::needless_question_mark::NEEDLESS_QUESTION_MARK_INFO,
     crate::needless_update::NEEDLESS_UPDATE_INFO,

clippy_lints/src/lib.rs

@@ -229,6 +229,7 @@ mod needless_for_each;
 mod needless_if;
 mod needless_late_init;
 mod needless_parens_on_range_literals;
+mod needless_pass_by_ref_mut;
 mod needless_pass_by_value;
 mod needless_question_mark;
 mod needless_update;
@@ -1057,6 +1058,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
     let stack_size_threshold = conf.stack_size_threshold;
     store.register_late_pass(move |_| Box::new(large_stack_frames::LargeStackFrames::new(stack_size_threshold)));
     store.register_late_pass(|_| Box::new(single_range_in_vec_init::SingleRangeInVecInit));
+    store.register_late_pass(|_| Box::new(needless_pass_by_ref_mut::NeedlessPassByRefMut));
     store.register_late_pass(|_| Box::new(incorrect_impls::IncorrectImpls));
     store.register_late_pass(move |_| {
         Box::new(single_call_fn::SingleCallFn {

clippy_lints/src/needless_pass_by_ref_mut.rs

@@ -0,0 +1,238 @@
+use super::needless_pass_by_value::requires_exact_signature;
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::source::snippet;
+use clippy_utils::{is_from_proc_macro, is_self};
+use if_chain::if_chain;
+use rustc_errors::Applicability;
+use rustc_hir::intravisit::FnKind;
+use rustc_hir::{Body, FnDecl, HirId, Impl, ItemKind, Mutability, Node, PatKind};
+use rustc_hir::{HirIdMap, HirIdSet};
+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, Ty};
+use rustc_session::{declare_lint_pass, declare_tool_lint};
+use rustc_span::def_id::LocalDefId;
+use rustc_span::symbol::kw;
+use rustc_span::Span;
+use rustc_target::spec::abi::Abi;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Check if a `&mut` function argument is actually used mutably.
+    ///
+    /// ### Why is this bad?
+    /// Less `mut` means less fights with the borrow checker. It can also lead to more
+    /// opportunities for parallelization.
+    ///
+    /// ### Example
+    /// ```rust
+    /// fn foo(y: &mut i32) -> i32 {
+    ///     12 + *y
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// fn foo(y: &i32) -> i32 {
+    ///     12 + *y
+    /// }
+    /// ```
+    #[clippy::version = "1.72.0"]
+    pub NEEDLESS_PASS_BY_REF_MUT,
+    suspicious,
+    "using a `&mut` argument when it's not mutated"
+}
+declare_lint_pass!(NeedlessPassByRefMut => [NEEDLESS_PASS_BY_REF_MUT]);
+
+fn should_skip<'tcx>(
+    cx: &LateContext<'tcx>,
+    input: rustc_hir::Ty<'tcx>,
+    ty: Ty<'_>,
+    arg: &rustc_hir::Param<'_>,
+) -> bool {
+    // We check if this a `&mut`. `ref_mutability` returns `None` if it's not a reference.
+    if !matches!(ty.ref_mutability(), Some(Mutability::Mut)) {
+        return true;
+    }
+
+    if is_self(arg) {
+        return true;
+    }
+
+    if let PatKind::Binding(.., name, _) = arg.pat.kind {
+        // If it's a potentially unused variable, we don't check it.
+        if name.name == kw::Underscore || name.as_str().starts_with('_') {
+            return true;
+        }
+    }
+
+    // All spans generated from a proc-macro invocation are the same...
+    is_from_proc_macro(cx, &input)
+}
+
+impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut {
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        kind: FnKind<'tcx>,
+        decl: &'tcx FnDecl<'_>,
+        body: &'tcx Body<'_>,
+        span: Span,
+        fn_def_id: LocalDefId,
+    ) {
+        if span.from_expansion() {
+            return;
+        }
+
+        let hir_id = cx.tcx.hir().local_def_id_to_hir_id(fn_def_id);
+
+        match kind {
+            FnKind::ItemFn(.., header) => {
+                let attrs = cx.tcx.hir().attrs(hir_id);
+                if header.abi != Abi::Rust || requires_exact_signature(attrs) {
+                    return;
+                }
+            },
+            FnKind::Method(..) => (),
+            FnKind::Closure => return,
+        }
+
+        // Exclude non-inherent impls
+        if let Some(Node::Item(item)) = cx.tcx.hir().find_parent(hir_id) {
+            if matches!(
+                item.kind,
+                ItemKind::Impl(Impl { of_trait: Some(_), .. }) | ItemKind::Trait(..)
+            ) {
+                return;
+            }
+        }
+
+        let fn_sig = cx.tcx.fn_sig(fn_def_id).subst_identity();
+        let fn_sig = cx.tcx.liberate_late_bound_regions(fn_def_id.to_def_id(), fn_sig);
+
+        // If there are no `&mut` argument, no need to go any further.
+        if !decl
+            .inputs
+            .iter()
+            .zip(fn_sig.inputs())
+            .zip(body.params)
+            .any(|((&input, &ty), arg)| !should_skip(cx, input, ty, arg))
+        {
+            return;
+        }
+
+        // Collect variables mutably used and spans which will need dereferencings from the
+        // function body.
+        let MutablyUsedVariablesCtxt { mutably_used_vars, .. } = {
+            let mut ctx = MutablyUsedVariablesCtxt::default();
+            let infcx = cx.tcx.infer_ctxt().build();
+            euv::ExprUseVisitor::new(&mut ctx, &infcx, fn_def_id, cx.param_env, cx.typeck_results()).consume_body(body);
+            ctx
+        };
+
+        for ((&input, &ty), arg) in decl.inputs.iter().zip(fn_sig.inputs()).zip(body.params) {
+            if should_skip(cx, input, ty, arg) {
+                continue;
+            }
+
+            // Only take `&mut` arguments.
+            if_chain! {
+                if let PatKind::Binding(_, canonical_id, ..) = arg.pat.kind;
+                if !mutably_used_vars.contains(&canonical_id);
+                if let rustc_hir::TyKind::Ref(_, inner_ty) = input.kind;
+                then {
+                    // If the argument is never used mutably, we emit the error.
+                    span_lint_and_sugg(
+                        cx,
+                        NEEDLESS_PASS_BY_REF_MUT,
+                        input.span,
+                        "this argument is a mutable reference, but not used mutably",
+                        "consider changing to",
+                        format!("&{}", snippet(cx, cx.tcx.hir().span(inner_ty.ty.hir_id), "_")),
+                        Applicability::Unspecified,
+                    );
+                }
+            }
+        }
+    }
+}
+
+#[derive(Default)]
+struct MutablyUsedVariablesCtxt {
+    mutably_used_vars: HirIdSet,
+    prev_bind: Option<HirId>,
+    aliases: HirIdMap<HirId>,
+}
+
+impl MutablyUsedVariablesCtxt {
+    fn add_mutably_used_var(&mut self, mut used_id: HirId) {
+        while let Some(id) = self.aliases.get(&used_id) {
+            self.mutably_used_vars.insert(used_id);
+            used_id = *id;
+        }
+        self.mutably_used_vars.insert(used_id);
+    }
+}
+
+impl<'tcx> euv::Delegate<'tcx> for MutablyUsedVariablesCtxt {
+    fn consume(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, _id: HirId) {
+        if let euv::Place {
+            base: euv::PlaceBase::Local(vid),
+            base_ty,
+            ..
+        } = &cmt.place
+        {
+            if let Some(bind_id) = self.prev_bind.take() {
+                self.aliases.insert(bind_id, *vid);
+            } else if matches!(base_ty.ref_mutability(), Some(Mutability::Mut)) {
+                self.add_mutably_used_var(*vid);
+            }
+        }
+    }
+
+    fn borrow(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, _id: HirId, borrow: ty::BorrowKind) {
+        self.prev_bind = None;
+        if let euv::Place {
+            base: euv::PlaceBase::Local(vid),
+            base_ty,
+            ..
+        } = &cmt.place
+        {
+            // If this is a mutable borrow, it was obviously used mutably so we add it. However
+            // for `UniqueImmBorrow`, it's interesting because if you do: `array[0] = value` inside
+            // a closure, it'll return this variant whereas if you have just an index access, it'll
+            // return `ImmBorrow`. So if there is "Unique" and it's a mutable reference, we add it
+            // to the mutably used variables set.
+            if borrow == ty::BorrowKind::MutBorrow
+                || (borrow == ty::BorrowKind::UniqueImmBorrow && base_ty.ref_mutability() == Some(Mutability::Mut))
+            {
+                self.add_mutably_used_var(*vid);
+            }
+        }
+    }
+
+    fn mutate(&mut self, cmt: &euv::PlaceWithHirId<'tcx>, _id: HirId) {
+        self.prev_bind = None;
+        if let euv::Place {
+            projections,
+            base: euv::PlaceBase::Local(vid),
+            ..
+        } = &cmt.place
+        {
+            if !projections.is_empty() {
+                self.add_mutably_used_var(*vid);
+            }
+        }
+    }
+
+    fn copy(&mut self, _cmt: &euv::PlaceWithHirId<'tcx>, _id: HirId) {
+        self.prev_bind = None;
+    }
+
+    fn fake_read(&mut self, _: &rustc_hir_typeck::expr_use_visitor::PlaceWithHirId<'tcx>, _: FakeReadCause, _: HirId) {}
+
+    fn bind(&mut self, _cmt: &euv::PlaceWithHirId<'tcx>, id: HirId) {
+        self.prev_bind = Some(id);
+    }
+}

clippy_lints/src/needless_pass_by_value.rs

@@ -289,7 +289,7 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByValue {
 }
 
 /// Functions marked with these attributes must have the exact signature.
-fn requires_exact_signature(attrs: &[Attribute]) -> bool {
+pub(crate) fn requires_exact_signature(attrs: &[Attribute]) -> bool {
     attrs.iter().any(|attr| {
         [sym::proc_macro, sym::proc_macro_attribute, sym::proc_macro_derive]
             .iter()