diff --git a/clippy_lints/src/methods/swap_with_temporary.rs b/clippy_lints/src/methods/swap_with_temporary.rs index de729fb343a3..e378cbd6ae0a 100644 --- a/clippy_lints/src/methods/swap_with_temporary.rs +++ b/clippy_lints/src/methods/swap_with_temporary.rs @@ -4,6 +4,7 @@ use rustc_ast::BorrowKind; use rustc_errors::{Applicability, Diag}; use rustc_hir::{Expr, ExprKind, Node, QPath}; use rustc_lint::LateContext; +use rustc_middle::ty::adjustment::Adjust; use rustc_span::sym; use super::SWAP_WITH_TEMPORARY; @@ -11,12 +12,12 @@ use super::SWAP_WITH_TEMPORARY; const MSG_TEMPORARY: &str = "this expression returns a temporary value"; const MSG_TEMPORARY_REFMUT: &str = "this is a mutable reference to a temporary value"; -pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, func: &Expr<'_>, args: &[Expr<'_>]) { +pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>, func: &Expr<'_>, args: &'tcx [Expr<'_>]) { if let ExprKind::Path(QPath::Resolved(_, func_path)) = func.kind && let Some(func_def_id) = func_path.res.opt_def_id() && cx.tcx.is_diagnostic_item(sym::mem_swap, func_def_id) { - match (ArgKind::new(&args[0]), ArgKind::new(&args[1])) { + match (ArgKind::new(cx, &args[0]), ArgKind::new(cx, &args[1])) { (ArgKind::RefMutToTemp(left_temp), ArgKind::RefMutToTemp(right_temp)) => { emit_lint_useless(cx, expr, &args[0], &args[1], left_temp, right_temp); }, @@ -28,10 +29,10 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, func: &Expr<'_>, args } enum ArgKind<'tcx> { - // Mutable reference to a place, coming from a macro - RefMutToPlaceAsMacro(&'tcx Expr<'tcx>), - // Place behind a mutable reference - RefMutToPlace(&'tcx Expr<'tcx>), + // Mutable reference to a place, coming from a macro, and number of dereferences to use + RefMutToPlaceAsMacro(&'tcx Expr<'tcx>, usize), + // Place behind a mutable reference, and number of dereferences to use + RefMutToPlace(&'tcx Expr<'tcx>, usize), // Temporary value behind a mutable reference RefMutToTemp(&'tcx Expr<'tcx>), // Any other case @@ -39,13 +40,29 @@ enum ArgKind<'tcx> { } impl<'tcx> ArgKind<'tcx> { - fn new(arg: &'tcx Expr<'tcx>) -> Self { - if let ExprKind::AddrOf(BorrowKind::Ref, _, target) = arg.kind { - if target.is_syntactic_place_expr() { + /// Build a new `ArgKind` from `arg`. There must be no false positive when returning a + /// `ArgKind::RefMutToTemp` variant, as this may cause a spurious lint to be emitted. + fn new(cx: &LateContext<'tcx>, arg: &'tcx Expr<'tcx>) -> Self { + if let ExprKind::AddrOf(BorrowKind::Ref, _, target) = arg.kind + && let adjustments = cx.typeck_results().expr_adjustments(arg) + && adjustments + .first() + .is_some_and(|adj| matches!(adj.kind, Adjust::Deref(None))) + && adjustments + .last() + .is_some_and(|adj| matches!(adj.kind, Adjust::Borrow(_))) + { + let extra_derefs = adjustments[1..adjustments.len() - 1] + .iter() + .filter(|adj| matches!(adj.kind, Adjust::Deref(_))) + .count(); + // If a deref is used, `arg` might be a place expression. For example, a mutex guard + // would dereference into the mutex content which is probably not temporary. + if target.is_syntactic_place_expr() || extra_derefs > 0 { if arg.span.from_expansion() { - ArgKind::RefMutToPlaceAsMacro(arg) + ArgKind::RefMutToPlaceAsMacro(arg, extra_derefs) } else { - ArgKind::RefMutToPlace(target) + ArgKind::RefMutToPlace(target, extra_derefs) } } else { ArgKind::RefMutToTemp(target) @@ -106,10 +123,15 @@ fn emit_lint_assign(cx: &LateContext<'_>, expr: &Expr<'_>, target: &ArgKind<'_>, let mut applicability = Applicability::MachineApplicable; let ctxt = expr.span.ctxt(); let assign_target = match target { - ArgKind::Expr(target) | ArgKind::RefMutToPlaceAsMacro(target) => { - Sugg::hir_with_context(cx, target, ctxt, "_", &mut applicability).deref() - }, - ArgKind::RefMutToPlace(target) => Sugg::hir_with_context(cx, target, ctxt, "_", &mut applicability), + ArgKind::Expr(target) => Sugg::hir_with_context(cx, target, ctxt, "_", &mut applicability).deref(), + ArgKind::RefMutToPlaceAsMacro(arg, derefs) => (0..*derefs).fold( + Sugg::hir_with_context(cx, arg, ctxt, "_", &mut applicability).deref(), + |sugg, _| sugg.deref(), + ), + ArgKind::RefMutToPlace(target, derefs) => (0..*derefs).fold( + Sugg::hir_with_context(cx, target, ctxt, "_", &mut applicability), + |sugg, _| sugg.deref(), + ), ArgKind::RefMutToTemp(_) => unreachable!(), }; let assign_source = Sugg::hir_with_context(cx, temp, ctxt, "_", &mut applicability); diff --git a/tests/ui/swap_with_temporary.fixed b/tests/ui/swap_with_temporary.fixed index 4007d998ba06..4b4b0d4aebd2 100644 --- a/tests/ui/swap_with_temporary.fixed +++ b/tests/ui/swap_with_temporary.fixed @@ -72,3 +72,49 @@ fn dont_lint_those(s: &mut S, v: &mut [String], w: Option<&mut String>) { swap(&mut s.t, v.get_mut(0).unwrap()); swap(w.unwrap(), &mut s.t); } + +fn issue15166() { + use std::sync::Mutex; + + struct A { + thing: Mutex>, + } + + impl A { + fn a(&self) { + let mut new_vec = vec![42]; + // Do not lint here, as neither `new_vec` nor the result of `.lock().unwrap()` are temporaries + swap(&mut new_vec, &mut self.thing.lock().unwrap()); + for v in new_vec { + // Do something with v + } + // Here `vec![42]` is temporary though, and a proper dereference will have to be used in the fix + *self.thing.lock().unwrap() = vec![42]; + //~^ ERROR: swapping with a temporary value is inefficient + } + } +} + +fn multiple_deref() { + let mut v1 = &mut &mut &mut vec![42]; + ***v1 = vec![]; + //~^ ERROR: swapping with a temporary value is inefficient + + struct Wrapper(T); + impl std::ops::Deref for Wrapper { + type Target = T; + fn deref(&self) -> &Self::Target { + &self.0 + } + } + impl std::ops::DerefMut for Wrapper { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 + } + } + + use std::sync::Mutex; + let mut v1 = Mutex::new(Wrapper(Wrapper(vec![42]))); + ***v1.lock().unwrap() = vec![]; + //~^ ERROR: swapping with a temporary value is inefficient +} diff --git a/tests/ui/swap_with_temporary.rs b/tests/ui/swap_with_temporary.rs index d403c086c0f4..8e35e6144d99 100644 --- a/tests/ui/swap_with_temporary.rs +++ b/tests/ui/swap_with_temporary.rs @@ -72,3 +72,49 @@ fn dont_lint_those(s: &mut S, v: &mut [String], w: Option<&mut String>) { swap(&mut s.t, v.get_mut(0).unwrap()); swap(w.unwrap(), &mut s.t); } + +fn issue15166() { + use std::sync::Mutex; + + struct A { + thing: Mutex>, + } + + impl A { + fn a(&self) { + let mut new_vec = vec![42]; + // Do not lint here, as neither `new_vec` nor the result of `.lock().unwrap()` are temporaries + swap(&mut new_vec, &mut self.thing.lock().unwrap()); + for v in new_vec { + // Do something with v + } + // Here `vec![42]` is temporary though, and a proper dereference will have to be used in the fix + swap(&mut vec![42], &mut self.thing.lock().unwrap()); + //~^ ERROR: swapping with a temporary value is inefficient + } + } +} + +fn multiple_deref() { + let mut v1 = &mut &mut &mut vec![42]; + swap(&mut ***v1, &mut vec![]); + //~^ ERROR: swapping with a temporary value is inefficient + + struct Wrapper(T); + impl std::ops::Deref for Wrapper { + type Target = T; + fn deref(&self) -> &Self::Target { + &self.0 + } + } + impl std::ops::DerefMut for Wrapper { + fn deref_mut(&mut self) -> &mut Self::Target { + &mut self.0 + } + } + + use std::sync::Mutex; + let mut v1 = Mutex::new(Wrapper(Wrapper(vec![42]))); + swap(&mut vec![], &mut v1.lock().unwrap()); + //~^ ERROR: swapping with a temporary value is inefficient +} diff --git a/tests/ui/swap_with_temporary.stderr b/tests/ui/swap_with_temporary.stderr index 59355771a964..5ca4fccd37a2 100644 --- a/tests/ui/swap_with_temporary.stderr +++ b/tests/ui/swap_with_temporary.stderr @@ -96,5 +96,41 @@ note: this expression returns a temporary value LL | swap(mac!(refmut y), &mut func()); | ^^^^^^ -error: aborting due to 8 previous errors +error: swapping with a temporary value is inefficient + --> tests/ui/swap_with_temporary.rs:92:13 + | +LL | swap(&mut vec![42], &mut self.thing.lock().unwrap()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use assignment instead: `*self.thing.lock().unwrap() = vec![42]` + | +note: this expression returns a temporary value + --> tests/ui/swap_with_temporary.rs:92:23 + | +LL | swap(&mut vec![42], &mut self.thing.lock().unwrap()); + | ^^^^^^^^ + +error: swapping with a temporary value is inefficient + --> tests/ui/swap_with_temporary.rs:100:5 + | +LL | swap(&mut ***v1, &mut vec![]); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use assignment instead: `***v1 = vec![]` + | +note: this expression returns a temporary value + --> tests/ui/swap_with_temporary.rs:100:27 + | +LL | swap(&mut ***v1, &mut vec![]); + | ^^^^^^ + +error: swapping with a temporary value is inefficient + --> tests/ui/swap_with_temporary.rs:118:5 + | +LL | swap(&mut vec![], &mut v1.lock().unwrap()); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use assignment instead: `***v1.lock().unwrap() = vec![]` + | +note: this expression returns a temporary value + --> tests/ui/swap_with_temporary.rs:118:15 + | +LL | swap(&mut vec![], &mut v1.lock().unwrap()); + | ^^^^^^ + +error: aborting due to 11 previous errors