From 7b5ea7a810f686e225e3f165cf24f4dc23a3d11d Mon Sep 17 00:00:00 2001 From: Catherine Flores Date: Fri, 21 Jul 2023 09:14:13 -0500 Subject: [PATCH] Do not lint if used as a `fn`-like argument --- clippy_lints/src/needless_pass_by_ref_mut.rs | 120 ++++++++++++++---- tests/ui/infinite_loop.stderr | 16 +-- tests/ui/let_underscore_future.stderr | 16 +-- tests/ui/mut_key.stderr | 16 +-- tests/ui/mut_reference.stderr | 24 ++-- tests/ui/needless_pass_by_ref_mut.rs | 56 ++++++-- tests/ui/needless_pass_by_ref_mut.stderr | 26 ++-- .../ui/should_impl_trait/method_list_2.stderr | 18 +-- 8 files changed, 193 insertions(+), 99 deletions(-) diff --git a/clippy_lints/src/needless_pass_by_ref_mut.rs b/clippy_lints/src/needless_pass_by_ref_mut.rs index c399b43e2518..d01110e94b9c 100644 --- a/clippy_lints/src/needless_pass_by_ref_mut.rs +++ b/clippy_lints/src/needless_pass_by_ref_mut.rs @@ -1,16 +1,18 @@ use super::needless_pass_by_value::requires_exact_signature; -use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::diagnostics::span_lint_hir_and_then; use clippy_utils::source::snippet; -use clippy_utils::{is_from_proc_macro, is_self}; -use if_chain::if_chain; -use rustc_data_structures::fx::FxHashSet; +use clippy_utils::{get_parent_node, is_from_proc_macro, is_self}; +use rustc_data_structures::fx::{FxHashSet, FxIndexMap}; use rustc_errors::Applicability; -use rustc_hir::intravisit::FnKind; -use rustc_hir::{Body, FnDecl, HirId, HirIdMap, HirIdSet, Impl, ItemKind, Mutability, Node, PatKind}; +use rustc_hir::intravisit::{walk_qpath, FnKind, Visitor}; +use rustc_hir::{ + Body, ExprField, ExprKind, FnDecl, HirId, HirIdMap, HirIdSet, Impl, ItemKind, Mutability, Node, PatKind, QPath, +}; use rustc_hir_typeck::expr_use_visitor as euv; use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::hir::map::associated_body; +use rustc_middle::hir::nested_filter::OnlyBodies; use rustc_middle::mir::FakeReadCause; use rustc_middle::ty::{self, Ty, UpvarId, UpvarPath}; use rustc_session::{declare_tool_lint, impl_lint_pass}; @@ -48,20 +50,24 @@ declare_clippy_lint! { "using a `&mut` argument when it's not mutated" } -#[derive(Copy, Clone)] -pub struct NeedlessPassByRefMut { +#[derive(Clone)] +pub struct NeedlessPassByRefMut<'tcx> { avoid_breaking_exported_api: bool, + used_fn_def_ids: FxHashSet, + fn_def_ids_to_maybe_unused_mut: FxIndexMap>>, } -impl NeedlessPassByRefMut { +impl NeedlessPassByRefMut<'_> { pub fn new(avoid_breaking_exported_api: bool) -> Self { Self { avoid_breaking_exported_api, + used_fn_def_ids: FxHashSet::default(), + fn_def_ids_to_maybe_unused_mut: FxIndexMap::default(), } } } -impl_lint_pass!(NeedlessPassByRefMut => [NEEDLESS_PASS_BY_REF_MUT]); +impl_lint_pass!(NeedlessPassByRefMut<'_> => [NEEDLESS_PASS_BY_REF_MUT]); fn should_skip<'tcx>( cx: &LateContext<'tcx>, @@ -89,12 +95,12 @@ fn should_skip<'tcx>( is_from_proc_macro(cx, &input) } -impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut { +impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut<'tcx> { fn check_fn( &mut self, cx: &LateContext<'tcx>, kind: FnKind<'tcx>, - decl: &'tcx FnDecl<'_>, + decl: &'tcx FnDecl<'tcx>, body: &'tcx Body<'_>, span: Span, fn_def_id: LocalDefId, @@ -140,7 +146,6 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut { if it.peek().is_none() { return; } - // Collect variables mutably used and spans which will need dereferencings from the // function body. let MutablyUsedVariablesCtxt { mutably_used_vars, .. } = { @@ -165,30 +170,45 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut { } ctx }; - - 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( + if let PatKind::Binding(_, canonical_id, ..) = arg.pat.kind + && !mutably_used_vars.contains(&canonical_id) + { + self.fn_def_ids_to_maybe_unused_mut.entry(fn_def_id).or_insert(vec![]).push(input); + } + } + } + + fn check_crate_post(&mut self, cx: &LateContext<'tcx>) { + cx.tcx.hir().visit_all_item_likes_in_crate(&mut FnNeedsMutVisitor { + cx, + used_fn_def_ids: &mut self.used_fn_def_ids, + }); + + for (fn_def_id, unused) in self + .fn_def_ids_to_maybe_unused_mut + .iter() + .filter(|(def_id, _)| !self.used_fn_def_ids.contains(def_id)) + { + let show_semver_warning = + self.avoid_breaking_exported_api && cx.effective_visibilities.is_exported(*fn_def_id); + + for input in unused { + // If the argument is never used mutably, we emit the warning. + let sp = input.span; + if let rustc_hir::TyKind::Ref(_, inner_ty) = input.kind { + span_lint_hir_and_then( cx, NEEDLESS_PASS_BY_REF_MUT, + cx.tcx.hir().local_def_id_to_hir_id(*fn_def_id), 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), "_"), - ), + format!("&{}", snippet(cx, cx.tcx.hir().span(inner_ty.ty.hir_id), "_"),), Applicability::Unspecified, ); if show_semver_warning { @@ -316,3 +336,49 @@ impl<'tcx> euv::Delegate<'tcx> for MutablyUsedVariablesCtxt { self.prev_bind = Some(id); } } + +/// A final pass to check for paths referencing this function that require the argument to be +/// `&mut`, basically if the function is ever used as a `fn`-like argument. +struct FnNeedsMutVisitor<'a, 'tcx> { + cx: &'a LateContext<'tcx>, + used_fn_def_ids: &'a mut FxHashSet, +} + +impl<'tcx> Visitor<'tcx> for FnNeedsMutVisitor<'_, 'tcx> { + type NestedFilter = OnlyBodies; + + fn nested_visit_map(&mut self) -> Self::Map { + self.cx.tcx.hir() + } + + fn visit_qpath(&mut self, qpath: &'tcx QPath<'tcx>, hir_id: HirId, _: Span) { + walk_qpath(self, qpath, hir_id); + + let Self { cx, used_fn_def_ids } = self; + + if let Node::Expr(expr) = cx.tcx.hir().get(hir_id) + && let Some(parent) = get_parent_node(cx.tcx, expr.hir_id) + && let ty::FnDef(def_id, _) = cx.tcx.typeck(cx.tcx.hir().enclosing_body_owner(hir_id)).expr_ty(expr).kind() + && let Some(def_id) = def_id.as_local() + { + if let Some(e) = match parent { + // #11182 + Node::Expr(e) => Some(e), + // #11199 + Node::ExprField(ExprField { expr, .. }) => Some(*expr), + _ => None, + } + && let ExprKind::Call(call, _) = e.kind + && call.hir_id == expr.hir_id + { + return; + } + + // We don't need to check each argument individually as you cannot coerce a function + // taking `&mut` -> `&`, for some reason, so if we've gotten this far we know it's + // passed as a `fn`-like argument (or is unified) and should ignore every "unused" + // argument entirely + used_fn_def_ids.insert(def_id); + } + } +} diff --git a/tests/ui/infinite_loop.stderr b/tests/ui/infinite_loop.stderr index 701b3cd19041..04559f9ada43 100644 --- a/tests/ui/infinite_loop.stderr +++ b/tests/ui/infinite_loop.stderr @@ -1,11 +1,3 @@ -error: this argument is a mutable reference, but not used mutably - --> $DIR/infinite_loop.rs:7:17 - | -LL | fn fn_mutref(i: &mut i32) { - | ^^^^^^^^ help: consider changing to: `&i32` - | - = note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings` - error: variables in the condition are not mutated in the loop body --> $DIR/infinite_loop.rs:20:11 | @@ -99,5 +91,13 @@ LL | while y < 10 { = note: this loop contains `return`s or `break`s = help: rewrite it as `if cond { loop { } }` +error: this argument is a mutable reference, but not used mutably + --> $DIR/infinite_loop.rs:7:17 + | +LL | fn fn_mutref(i: &mut i32) { + | ^^^^^^^^ help: consider changing to: `&i32` + | + = note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings` + error: aborting due to 12 previous errors diff --git a/tests/ui/let_underscore_future.stderr b/tests/ui/let_underscore_future.stderr index 9e69fb041330..ff1e2b8c9019 100644 --- a/tests/ui/let_underscore_future.stderr +++ b/tests/ui/let_underscore_future.stderr @@ -1,11 +1,3 @@ -error: this argument is a mutable reference, but not used mutably - --> $DIR/let_underscore_future.rs:11:35 - | -LL | fn do_something_to_future(future: &mut impl Future) {} - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider changing to: `&impl Future` - | - = note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings` - error: non-binding `let` on a future --> $DIR/let_underscore_future.rs:14:5 | @@ -31,5 +23,13 @@ LL | let _ = future; | = help: consider awaiting the future or dropping explicitly with `std::mem::drop` +error: this argument is a mutable reference, but not used mutably + --> $DIR/let_underscore_future.rs:11:35 + | +LL | fn do_something_to_future(future: &mut impl Future) {} + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider changing to: `&impl Future` + | + = note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings` + error: aborting due to 4 previous errors diff --git a/tests/ui/mut_key.stderr b/tests/ui/mut_key.stderr index 02a0da86a4b8..3f756f5f0e59 100644 --- a/tests/ui/mut_key.stderr +++ b/tests/ui/mut_key.stderr @@ -12,14 +12,6 @@ error: mutable key type LL | fn should_not_take_this_arg(m: &mut HashMap, _n: usize) -> HashSet { | ^^^^^^^^^^^^ -error: this argument is a mutable reference, but not used mutably - --> $DIR/mut_key.rs:31:32 - | -LL | fn should_not_take_this_arg(m: &mut HashMap, _n: usize) -> HashSet { - | ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider changing to: `&HashMap` - | - = note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings` - error: mutable key type --> $DIR/mut_key.rs:32:5 | @@ -110,5 +102,13 @@ error: mutable key type LL | let _map = HashMap::>, usize>::new(); | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ +error: this argument is a mutable reference, but not used mutably + --> $DIR/mut_key.rs:31:32 + | +LL | fn should_not_take_this_arg(m: &mut HashMap, _n: usize) -> HashSet { + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: consider changing to: `&HashMap` + | + = note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings` + error: aborting due to 18 previous errors diff --git a/tests/ui/mut_reference.stderr b/tests/ui/mut_reference.stderr index 23c812475c2a..1fdfbf9227e3 100644 --- a/tests/ui/mut_reference.stderr +++ b/tests/ui/mut_reference.stderr @@ -1,17 +1,3 @@ -error: this argument is a mutable reference, but not used mutably - --> $DIR/mut_reference.rs:4:33 - | -LL | fn takes_a_mutable_reference(a: &mut i32) {} - | ^^^^^^^^ help: consider changing to: `&i32` - | - = note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings` - -error: this argument is a mutable reference, but not used mutably - --> $DIR/mut_reference.rs:11:44 - | -LL | fn takes_a_mutable_reference(&self, a: &mut i32) {} - | ^^^^^^^^ help: consider changing to: `&i32` - error: the function `takes_an_immutable_reference` doesn't need a mutable reference --> $DIR/mut_reference.rs:17:34 | @@ -32,5 +18,13 @@ error: the method `takes_an_immutable_reference` doesn't need a mutable referenc LL | my_struct.takes_an_immutable_reference(&mut 42); | ^^^^^^^ -error: aborting due to 5 previous errors +error: this argument is a mutable reference, but not used mutably + --> $DIR/mut_reference.rs:11:44 + | +LL | fn takes_a_mutable_reference(&self, a: &mut i32) {} + | ^^^^^^^^ help: consider changing to: `&i32` + | + = note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings` + +error: aborting due to 4 previous errors diff --git a/tests/ui/needless_pass_by_ref_mut.rs b/tests/ui/needless_pass_by_ref_mut.rs index 27e1fc075bbb..49f88978bff4 100644 --- a/tests/ui/needless_pass_by_ref_mut.rs +++ b/tests/ui/needless_pass_by_ref_mut.rs @@ -1,4 +1,5 @@ -#![allow(unused)] +#![allow(clippy::if_same_then_else, clippy::no_effect)] +#![feature(lint_reasons)] use std::ptr::NonNull; @@ -155,15 +156,48 @@ async fn a8(x: i32, a: &mut i32, y: i32, z: &mut i32) { println!("{:?}", z); } +// Should not warn (passed as closure which takes `&mut`). +fn passed_as_closure(s: &mut u32) {} + +// Should not warn. +fn passed_as_local(s: &mut u32) {} + +// Should not warn. +fn ty_unify_1(s: &mut u32) {} + +// Should not warn. +fn ty_unify_2(s: &mut u32) {} + +// Should not warn. +fn passed_as_field(s: &mut u32) {} + +fn closure_takes_mut(s: fn(&mut u32)) {} + +struct A { + s: fn(&mut u32), +} + +// Should warn. +fn used_as_path(s: &mut u32) {} + +// Make sure lint attributes work fine +#[expect(clippy::needless_pass_by_ref_mut)] +fn lint_attr(s: &mut u32) {} + fn main() { - // let mut u = 0; - // let mut v = vec![0]; - // foo(&mut v, &0, &mut u); - // foo2(&mut v); - // foo3(&mut v); - // foo4(&mut v); - // foo5(&mut v); - // alias_check(&mut v); - // alias_check2(&mut v); - // println!("{u}"); + let mut u = 0; + let mut v = vec![0]; + foo(&mut v, &0, &mut u); + foo2(&mut v); + foo3(&mut v); + foo4(&mut v); + foo5(&mut v); + alias_check(&mut v); + alias_check2(&mut v); + println!("{u}"); + closure_takes_mut(passed_as_closure); + A { s: passed_as_field }; + used_as_path; + let _: fn(&mut u32) = passed_as_local; + let _ = if v[0] == 0 { ty_unify_1 } else { ty_unify_2 }; } diff --git a/tests/ui/needless_pass_by_ref_mut.stderr b/tests/ui/needless_pass_by_ref_mut.stderr index caf47bbce86c..4ab5b5675cac 100644 --- a/tests/ui/needless_pass_by_ref_mut.stderr +++ b/tests/ui/needless_pass_by_ref_mut.stderr @@ -1,5 +1,5 @@ error: this argument is a mutable reference, but not used mutably - --> $DIR/needless_pass_by_ref_mut.rs:6:11 + --> $DIR/needless_pass_by_ref_mut.rs:7:11 | LL | fn foo(s: &mut Vec, b: &u32, x: &mut u32) { | ^^^^^^^^^^^^^ help: consider changing to: `&Vec` @@ -7,73 +7,73 @@ LL | fn foo(s: &mut Vec, b: &u32, x: &mut u32) { = note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings` error: this argument is a mutable reference, but not used mutably - --> $DIR/needless_pass_by_ref_mut.rs:31:12 + --> $DIR/needless_pass_by_ref_mut.rs:32:12 | LL | fn foo6(s: &mut Vec) { | ^^^^^^^^^^^^^ help: consider changing to: `&Vec` error: this argument is a mutable reference, but not used mutably - --> $DIR/needless_pass_by_ref_mut.rs:44:29 + --> $DIR/needless_pass_by_ref_mut.rs:45:29 | LL | fn mushroom(&self, vec: &mut Vec) -> usize { | ^^^^^^^^^^^^^ help: consider changing to: `&Vec` error: this argument is a mutable reference, but not used mutably - --> $DIR/needless_pass_by_ref_mut.rs:49:31 + --> $DIR/needless_pass_by_ref_mut.rs:50:31 | LL | fn badger(&mut self, vec: &mut Vec) -> usize { | ^^^^^^^^^^^^^ help: consider changing to: `&Vec` error: this argument is a mutable reference, but not used mutably - --> $DIR/needless_pass_by_ref_mut.rs:126:16 + --> $DIR/needless_pass_by_ref_mut.rs:127:16 | LL | async fn a1(x: &mut i32) { | ^^^^^^^^ help: consider changing to: `&i32` error: this argument is a mutable reference, but not used mutably - --> $DIR/needless_pass_by_ref_mut.rs:130:16 + --> $DIR/needless_pass_by_ref_mut.rs:131:16 | LL | async fn a2(x: &mut i32, y: String) { | ^^^^^^^^ help: consider changing to: `&i32` error: this argument is a mutable reference, but not used mutably - --> $DIR/needless_pass_by_ref_mut.rs:134:16 + --> $DIR/needless_pass_by_ref_mut.rs:135:16 | LL | async fn a3(x: &mut i32, y: String, z: String) { | ^^^^^^^^ help: consider changing to: `&i32` error: this argument is a mutable reference, but not used mutably - --> $DIR/needless_pass_by_ref_mut.rs:138:16 + --> $DIR/needless_pass_by_ref_mut.rs:139:16 | LL | async fn a4(x: &mut i32, y: i32) { | ^^^^^^^^ help: consider changing to: `&i32` error: this argument is a mutable reference, but not used mutably - --> $DIR/needless_pass_by_ref_mut.rs:142:24 + --> $DIR/needless_pass_by_ref_mut.rs:143:24 | LL | async fn a5(x: i32, y: &mut i32) { | ^^^^^^^^ help: consider changing to: `&i32` error: this argument is a mutable reference, but not used mutably - --> $DIR/needless_pass_by_ref_mut.rs:146:24 + --> $DIR/needless_pass_by_ref_mut.rs:147:24 | LL | async fn a6(x: i32, y: &mut i32) { | ^^^^^^^^ help: consider changing to: `&i32` error: this argument is a mutable reference, but not used mutably - --> $DIR/needless_pass_by_ref_mut.rs:150:32 + --> $DIR/needless_pass_by_ref_mut.rs:151:32 | LL | async fn a7(x: i32, y: i32, z: &mut i32) { | ^^^^^^^^ help: consider changing to: `&i32` error: this argument is a mutable reference, but not used mutably - --> $DIR/needless_pass_by_ref_mut.rs:154:24 + --> $DIR/needless_pass_by_ref_mut.rs:155:24 | LL | async fn a8(x: i32, a: &mut i32, y: i32, z: &mut i32) { | ^^^^^^^^ help: consider changing to: `&i32` error: this argument is a mutable reference, but not used mutably - --> $DIR/needless_pass_by_ref_mut.rs:154:45 + --> $DIR/needless_pass_by_ref_mut.rs:155:45 | LL | async fn a8(x: i32, a: &mut i32, y: i32, z: &mut i32) { | ^^^^^^^^ help: consider changing to: `&i32` diff --git a/tests/ui/should_impl_trait/method_list_2.stderr b/tests/ui/should_impl_trait/method_list_2.stderr index 2ae9fa34d142..a3bb05bf176b 100644 --- a/tests/ui/should_impl_trait/method_list_2.stderr +++ b/tests/ui/should_impl_trait/method_list_2.stderr @@ -39,15 +39,6 @@ LL | | } | = help: consider implementing the trait `std::hash::Hash` or choosing a less ambiguous method name -error: this argument is a mutable reference, but not used mutably - --> $DIR/method_list_2.rs:38:31 - | -LL | pub fn hash(&self, state: &mut T) { - | ^^^^^^ help: consider changing to: `&T` - | - = warning: changing this function will impact semver compatibility - = note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings` - error: method `index` can be confused for the standard trait method `std::ops::Index::index` --> $DIR/method_list_2.rs:42:5 | @@ -158,5 +149,14 @@ LL | | } | = help: consider implementing the trait `std::ops::Sub` or choosing a less ambiguous method name +error: this argument is a mutable reference, but not used mutably + --> $DIR/method_list_2.rs:38:31 + | +LL | pub fn hash(&self, state: &mut T) { + | ^^^^^^ help: consider changing to: `&T` + | + = warning: changing this function will impact semver compatibility + = note: `-D clippy::needless-pass-by-ref-mut` implied by `-D warnings` + error: aborting due to 16 previous errors