Consider deref'ed argument as non-temporary (#15172)
If there are more than one dereference (there is one corresponding
matched with a borrow in any case), consider that the argument might
point to a place expression, which is the safest choice.
Also, use an appropriate number of dereferences in suggestions involving
arguments using themselves multiple dereferences.
Fixes rust-lang/rust-clippy#15166
changelog: [`swap_with_temporary`]: fix false positive leading to
different semantics being suggested, and use the right number of
dereferences in suggestion
r? y21
<!-- TRIAGEBOT_START -->
<!-- TRIAGEBOT_SUMMARY_START -->
### Summary Notes
-
[beta-nomination](https://github.com/rust-lang/rust-clippy/pull/15172#issuecomment-3016752569)
by [samueltardieu](https://github.com/samueltardieu)
*Managed by `@rustbot`—see
[help](https://forge.rust-lang.org/triagebot/note.html) for details*
<!-- TRIAGEBOT_SUMMARY_END -->
<!-- TRIAGEBOT_END -->
diff --git a/clippy_lints/src/methods/swap_with_temporary.rs b/clippy_lints/src/methods/swap_with_temporary.rs
index de729fb..e378cbd 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_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 @@
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 @@
}
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 @@
}
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 @@
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 4007d99..4b4b0d4 100644
--- a/tests/ui/swap_with_temporary.fixed
+++ b/tests/ui/swap_with_temporary.fixed
@@ -72,3 +72,49 @@
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<Vec<u8>>,
+ }
+
+ 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: ?Sized>(T);
+ impl<T: ?Sized> std::ops::Deref for Wrapper<T> {
+ type Target = T;
+ fn deref(&self) -> &Self::Target {
+ &self.0
+ }
+ }
+ impl<T: ?Sized> std::ops::DerefMut for Wrapper<T> {
+ 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 d403c08..8e35e61 100644
--- a/tests/ui/swap_with_temporary.rs
+++ b/tests/ui/swap_with_temporary.rs
@@ -72,3 +72,49 @@
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<Vec<u8>>,
+ }
+
+ 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: ?Sized>(T);
+ impl<T: ?Sized> std::ops::Deref for Wrapper<T> {
+ type Target = T;
+ fn deref(&self) -> &Self::Target {
+ &self.0
+ }
+ }
+ impl<T: ?Sized> std::ops::DerefMut for Wrapper<T> {
+ 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 5935577..5ca4fcc 100644
--- a/tests/ui/swap_with_temporary.stderr
+++ b/tests/ui/swap_with_temporary.stderr
@@ -96,5 +96,41 @@
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
