From 929e266e71ed7c34377bcc3fa1a7e460416daa8b 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 | 119 ++++++++++++++---- tests/ui/infinite_loop.stderr | 16 +-- tests/ui/let_underscore_future.stderr | 16 +-- tests/ui/mut_key.stderr | 16 +-- tests/ui/mut_reference.stderr | 28 ++--- tests/ui/needless_pass_by_ref_mut.rs | 25 +++- tests/ui/needless_pass_by_ref_mut.stderr | 28 +++-- .../ui/should_impl_trait/method_list_2.stderr | 18 +-- 8 files changed, 183 insertions(+), 83 deletions(-) diff --git a/clippy_lints/src/needless_pass_by_ref_mut.rs b/clippy_lints/src/needless_pass_by_ref_mut.rs index cdb7ee483054..119de844294a 100644 --- a/clippy_lints/src/needless_pass_by_ref_mut.rs +++ b/clippy_lints/src/needless_pass_by_ref_mut.rs @@ -1,14 +1,17 @@ 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 clippy_utils::{get_parent_node, is_from_proc_macro, is_self}; +use rustc_data_structures::fx::{FxHashMap, FxHashSet}; 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::nested_filter::OnlyBodies; use rustc_middle::mir::FakeReadCause; use rustc_middle::ty::{self, Ty}; use rustc_session::{declare_tool_lint, impl_lint_pass}; @@ -46,20 +49,26 @@ 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, + // TODO(Centri3): Change this to a `Vec`. This will cause a performance deficit but it'll make the output a lot + // more consistent, as adding or removing a function won't change the order of them + fn_def_ids_to_maybe_unused_mut: FxHashMap>>, } -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: FxHashMap::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>, @@ -87,12 +96,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, @@ -157,29 +166,45 @@ impl<'tcx> LateLintPass<'tcx> for NeedlessPassByRefMut { 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( + 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 { @@ -271,3 +296,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() + { + let Some(e) = (match parent { + // #11182 + Node::Expr(e) => Some(e), + // #11199 + Node::ExprField(ExprField { expr, .. }) => Some(*expr), + _ => None, + }) else { + return; + }; + + if matches!(e.kind, ExprKind::Call(call, _) if 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 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..717f0d16eaaf 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,19 @@ error: the method `takes_an_immutable_reference` doesn't need a mutable referenc LL | my_struct.takes_an_immutable_reference(&mut 42); | ^^^^^^^ +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: aborting due to 5 previous errors diff --git a/tests/ui/needless_pass_by_ref_mut.rs b/tests/ui/needless_pass_by_ref_mut.rs index 5e7280995c60..90046b9d5928 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::no_effect)] +#![feature(lint_reasons)] use std::ptr::NonNull; @@ -91,6 +92,25 @@ impl Mut { // Should not warn. fn unused(_: &mut u32, _b: &mut u8) {} +// Should not warn (passed as closure which takes `&mut`). +fn passed_as_closure(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]; @@ -102,4 +122,7 @@ fn main() { alias_check(&mut v); alias_check2(&mut v); println!("{u}"); + closure_takes_mut(passed_as_closure); + A { s: passed_as_field }; + used_as_path; } diff --git a/tests/ui/needless_pass_by_ref_mut.stderr b/tests/ui/needless_pass_by_ref_mut.stderr index 5e9d80bb6c48..4fcfc65e765e 100644 --- a/tests/ui/needless_pass_by_ref_mut.stderr +++ b/tests/ui/needless_pass_by_ref_mut.stderr @@ -1,28 +1,34 @@ 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:50:31 | -LL | fn foo(s: &mut Vec, b: &u32, x: &mut u32) { - | ^^^^^^^^^^^^^ help: consider changing to: `&Vec` +LL | fn badger(&mut self, vec: &mut Vec) -> usize { + | ^^^^^^^^^^^^^ help: consider changing to: `&Vec` | = 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:7:11 | -LL | fn foo6(s: &mut Vec) { - | ^^^^^^^^^^^^^ help: consider changing to: `&Vec` +LL | fn foo(s: &mut Vec, b: &u32, x: &mut u32) { + | ^^^^^^^^^^^^^ 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:32:12 | -LL | fn badger(&mut self, vec: &mut Vec) -> usize { - | ^^^^^^^^^^^^^ help: consider changing to: `&Vec` +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:108:20 + | +LL | fn used_as_path(s: &mut u32) {} + | ^^^^^^^^ help: consider changing to: `&u32` -error: aborting due to 4 previous errors +error: aborting due to 5 previous errors 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