Add needless_pass_by_ref_mut 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,11 @@ 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(move |_| {
+        Box::new(needless_pass_by_ref_mut::NeedlessPassByRefMut::new(
+            avoid_breaking_exported_api,
+        ))
+    });
     store.register_late_pass(|_| Box::new(incorrect_impls::IncorrectImpls));
     store.register_late_pass(move |_| {
         Box::new(single_call_fn::SingleCallFn {

clippy_lints/src/literal_representation.rs

@@ -264,7 +264,7 @@ impl LiteralDigitGrouping {
                     return;
                 }
 
-                if Self::is_literal_uuid_formatted(&mut num_lit) {
+                if Self::is_literal_uuid_formatted(&num_lit) {
                     return;
                 }
 
@@ -376,7 +376,7 @@ impl LiteralDigitGrouping {
     ///
     /// Returns `true` if the radix is hexadecimal, and the groups match the
     /// UUID format of 8-4-4-4-12.
-    fn is_literal_uuid_formatted(num_lit: &mut NumericLiteral<'_>) -> bool {
+    fn is_literal_uuid_formatted(num_lit: &NumericLiteral<'_>) -> bool {
         if num_lit.radix != Radix::Hexadecimal {
             return false;
         }

clippy_lints/src/needless_pass_by_ref_mut.rs

@@ -0,0 +1,274 @@
+use super::needless_pass_by_value::requires_exact_signature;
+use clippy_utils::diagnostics::span_lint_and_then;
+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_tool_lint, impl_lint_pass};
+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.
+    ///
+    /// Be careful if the function is publically reexported as it would break compatibility with
+    /// users of this function.
+    ///
+    /// ### 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"
+}
+
+#[derive(Copy, Clone)]
+pub struct NeedlessPassByRefMut {
+    avoid_breaking_exported_api: bool,
+}
+
+impl NeedlessPassByRefMut {
+    pub fn new(avoid_breaking_exported_api: bool) -> Self {
+        Self {
+            avoid_breaking_exported_api,
+        }
+    }
+}
+
+impl_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
+        };
+
+        let mut it = decl
+            .inputs
+            .iter()
+            .zip(fn_sig.inputs())
+            .zip(body.params)
+            .filter(|((&input, &ty), arg)| !should_skip(cx, input, ty, arg))
+            .peekable();
+        if it.peek().is_none() {
+            return;
+        }
+        let show_semver_warning = self.avoid_breaking_exported_api && cx.effective_visibilities.is_exported(fn_def_id);
+        for ((&input, &_), arg) in it {
+            // 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 warning.
+                    let sp = input.span;
+                    span_lint_and_then(
+                        cx,
+                        NEEDLESS_PASS_BY_REF_MUT,
+                        sp,
+                        "this argument is a mutable reference, but not used mutably",
+                        |diag| {
+                            diag.span_suggestion(
+                                sp,
+                                "consider changing to".to_string(),
+                                format!(
+                                    "&{}",
+                                    snippet(cx, cx.tcx.hir().span(inner_ty.ty.hir_id), "_"),
+                                ),
+                                Applicability::Unspecified,
+                            );
+                            if show_semver_warning {
+                                diag.warn("changing this function will impact semver compatibility");
+                            }
+                        },
+                    );
+                }
+            }
+        }
+    }
+}
+
+#[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()

clippy_lints/src/non_expressive_names.rs

@@ -91,7 +91,7 @@ struct ExistingName {
 struct SimilarNamesLocalVisitor<'a, 'tcx> {
     names: Vec<ExistingName>,
     cx: &'a EarlyContext<'tcx>,
-    lint: &'a NonExpressiveNames,
+    lint: NonExpressiveNames,
 
     /// A stack of scopes containing the single-character bindings in each scope.
     single_char_names: Vec<Vec<Ident>>,
@@ -365,7 +365,7 @@ impl EarlyLintPass for NonExpressiveNames {
             ..
         }) = item.kind
         {
-            do_check(self, cx, &item.attrs, &sig.decl, blk);
+            do_check(*self, cx, &item.attrs, &sig.decl, blk);
         }
     }
 
@@ -380,12 +380,12 @@ impl EarlyLintPass for NonExpressiveNames {
             ..
         }) = item.kind
         {
-            do_check(self, cx, &item.attrs, &sig.decl, blk);
+            do_check(*self, cx, &item.attrs, &sig.decl, blk);
         }
     }
 }
 
-fn do_check(lint: &mut NonExpressiveNames, cx: &EarlyContext<'_>, attrs: &[Attribute], decl: &FnDecl, blk: &Block) {
+fn do_check(lint: NonExpressiveNames, cx: &EarlyContext<'_>, attrs: &[Attribute], decl: &FnDecl, blk: &Block) {
     if !attrs.iter().any(|attr| attr.has_name(sym::test)) {
         let mut visitor = SimilarNamesLocalVisitor {
             names: Vec::new(),

tests/ui-toml/toml_trivially_copy/test.rs

@@ -2,6 +2,7 @@
 //@normalize-stderr-test: "\(limit: \d+ byte\)" -> "(limit: N byte)"
 
 #![warn(clippy::trivially_copy_pass_by_ref)]
+#![allow(clippy::needless_pass_by_ref_mut)]
 
 #[derive(Copy, Clone)]
 struct Foo(u8);

tests/ui-toml/toml_trivially_copy/test.stderr

@@ -1,13 +1,13 @@
 error: this argument (N byte) is passed by reference, but would be more efficient if passed by value (limit: N byte)
-  --> $DIR/test.rs:14:11
+  --> $DIR/test.rs:15:11
    |
 LL | fn bad(x: &u16, y: &Foo) {}
    |           ^^^^ help: consider passing by value instead: `u16`
    |
    = note: `-D clippy::trivially-copy-pass-by-ref` implied by `-D warnings`
 
 error: this argument (N byte) is passed by reference, but would be more efficient if passed by value (limit: N byte)
-  --> $DIR/test.rs:14:20
+  --> $DIR/test.rs:15:20
    |
 LL | fn bad(x: &u16, y: &Foo) {}
    |                    ^^^^ help: consider passing by value instead: `Foo`