From b41fc6784fc723ea52da0e0668fc5205b740f81a Mon Sep 17 00:00:00 2001
From: Guillaume Gomez <guillaume1.gomez@gmail.com>
Date: Tue, 6 Jun 2023 19:27:12 +0200
Subject: [PATCH] Add needless_pass_by_ref lint
---
CHANGELOG.md | 1 +
clippy_lints/src/declared_lints.rs | 1 +
clippy_lints/src/lib.rs | 2 +
clippy_lints/src/needless_pass_by_ref_mut.rs | 238 +++++++++++++++++++
clippy_lints/src/needless_pass_by_value.rs | 2 +-
5 files changed, 243 insertions(+), 1 deletion(-)
create mode 100644 clippy_lints/src/needless_pass_by_ref_mut.rs
diff --git a/CHANGELOG.md b/CHANGELOG.md
index f9310b4ab31..762efed5c12 100644
--- a/CHANGELOG.md
+++ b/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
diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs
index e1dbe500396..4f945ebc610 100644
--- a/clippy_lints/src/declared_lints.rs
+++ b/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,
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
index c43e0b753dc..e1e4a83b031 100644
--- a/clippy_lints/src/lib.rs
+++ b/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 {
diff --git a/clippy_lints/src/needless_pass_by_ref_mut.rs b/clippy_lints/src/needless_pass_by_ref_mut.rs
new file mode 100644
index 00000000000..c5dc87cdfa8
--- /dev/null
+++ b/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);
+ }
+}
diff --git a/clippy_lints/src/needless_pass_by_value.rs b/clippy_lints/src/needless_pass_by_value.rs
index ece10474715..5d299f9355d 100644
--- a/clippy_lints/src/needless_pass_by_value.rs
+++ b/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()