New lint: swap_with_temporary (rust-lang#14046)

This lint detects inefficient or useless `{std,core}::mem::swap()` calls
such as:

```rust
    // Should be `a = temp();`
    swap(&mut a, &mut temp());
    // Should be `*b = temp();`
    swap(b, &mut temp());
    // Should be `temp1(); temp2();` if we want to keep the side effects
    swap(&mut temp1(), &mut temp2());
```

It also takes care of using a form appropriate for a `()` context if
`swap()` is part of a larger expression (don't ask me why this wouldn't
happen, I have no idea), by suggesting `{ x = y; }` (statement in block)
or `{std,core}::mem::drop((temp1(), temp2())`.

changelog: [`swap_with_temporary`]: new lint

Close rust-lang#1968

Author: y21

CHANGELOG.md

@@ -6218,6 +6218,7 @@ Released 2018-09-13
 [`suspicious_unary_op_formatting`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_unary_op_formatting
 [`suspicious_xor_used_as_pow`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_xor_used_as_pow
 [`swap_ptr_to_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#swap_ptr_to_ref
+[`swap_with_temporary`]: https://rust-lang.github.io/rust-clippy/master/index.html#swap_with_temporary
 [`tabs_in_doc_comments`]: https://rust-lang.github.io/rust-clippy/master/index.html#tabs_in_doc_comments
 [`temporary_assignment`]: https://rust-lang.github.io/rust-clippy/master/index.html#temporary_assignment
 [`temporary_cstring_as_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#temporary_cstring_as_ptr

clippy_lints/src/declared_lints.rs

@@ -491,6 +491,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
     crate::methods::SUSPICIOUS_OPEN_OPTIONS_INFO,
     crate::methods::SUSPICIOUS_SPLITN_INFO,
     crate::methods::SUSPICIOUS_TO_OWNED_INFO,
+    crate::methods::SWAP_WITH_TEMPORARY_INFO,
     crate::methods::TYPE_ID_ON_BOX_INFO,
     crate::methods::UNBUFFERED_BYTES_INFO,
     crate::methods::UNINIT_ASSUMED_INIT_INFO,

clippy_lints/src/methods/mod.rs

@@ -114,6 +114,7 @@ mod suspicious_command_arg_space;
 mod suspicious_map;
 mod suspicious_splitn;
 mod suspicious_to_owned;
+mod swap_with_temporary;
 mod type_id_on_box;
 mod unbuffered_bytes;
 mod uninit_assumed_init;
@@ -4481,6 +4482,53 @@ declare_clippy_lint! {
     "calling `std::io::Error::new(std::io::ErrorKind::Other, _)`"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for usage of `std::mem::swap` with temporary values.
+    ///
+    /// ### Why is this bad?
+    /// Storing a new value in place of a temporary value which will
+    /// be dropped right after the `swap` is an inefficient way of performing
+    /// an assignment. The same result can be achieved by using a regular
+    /// assignment.
+    ///
+    /// ### Examples
+    /// ```no_run
+    /// fn replace_string(s: &mut String) {
+    ///     std::mem::swap(s, &mut String::from("replaced"));
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// fn replace_string(s: &mut String) {
+    ///     *s = String::from("replaced");
+    /// }
+    /// ```
+    ///
+    /// Also, swapping two temporary values has no effect, as they will
+    /// both be dropped right after swapping them. This is likely an indication
+    /// of a bug. For example, the following code swaps the references to
+    /// the last element of the vectors, instead of swapping the elements
+    /// themselves:
+    ///
+    /// ```no_run
+    /// fn bug(v1: &mut [i32], v2: &mut [i32]) {
+    ///     // Incorrect: swapping temporary references (`&mut &mut` passed to swap)
+    ///     std::mem::swap(&mut v1.last_mut().unwrap(), &mut v2.last_mut().unwrap());
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// fn correct(v1: &mut [i32], v2: &mut [i32]) {
+    ///     std::mem::swap(v1.last_mut().unwrap(), v2.last_mut().unwrap());
+    /// }
+    /// ```
+    #[clippy::version = "1.88.0"]
+    pub SWAP_WITH_TEMPORARY,
+    complexity,
+    "detect swap with a temporary value"
+}
+
 #[expect(clippy::struct_excessive_bools)]
 pub struct Methods {
     avoid_breaking_exported_api: bool,
@@ -4658,6 +4706,7 @@ impl_lint_pass!(Methods => [
     UNBUFFERED_BYTES,
     MANUAL_CONTAINS,
     IO_OTHER_ERROR,
+    SWAP_WITH_TEMPORARY,
 ]);
 
 /// Extracts a method call name, args, and `Span` of the method name.
@@ -4689,6 +4738,7 @@ impl<'tcx> LateLintPass<'tcx> for Methods {
                 manual_c_str_literals::check(cx, expr, func, args, self.msrv);
                 useless_nonzero_new_unchecked::check(cx, expr, func, args, self.msrv);
                 io_other_error::check(cx, expr, func, args, self.msrv);
+                swap_with_temporary::check(cx, expr, func, args);
             },
             ExprKind::MethodCall(method_call, receiver, args, _) => {
                 let method_span = method_call.ident.span;

clippy_lints/src/methods/swap_with_temporary.rs

@@ -0,0 +1,125 @@
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::sugg::Sugg;
+use rustc_ast::BorrowKind;
+use rustc_errors::{Applicability, Diag};
+use rustc_hir::{Expr, ExprKind, Node, QPath};
+use rustc_lint::LateContext;
+use rustc_span::sym;
+
+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<'_>]) {
+    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])) {
+            (ArgKind::RefMutToTemp(left_temp), ArgKind::RefMutToTemp(right_temp)) => {
+                emit_lint_useless(cx, expr, &args[0], &args[1], left_temp, right_temp);
+            },
+            (ArgKind::RefMutToTemp(left_temp), right) => emit_lint_assign(cx, expr, &right, &args[0], left_temp),
+            (left, ArgKind::RefMutToTemp(right_temp)) => emit_lint_assign(cx, expr, &left, &args[1], right_temp),
+            _ => {},
+        }
+    }
+}
+
+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>),
+    // Temporary value behind a mutable reference
+    RefMutToTemp(&'tcx Expr<'tcx>),
+    // Any other case
+    Expr(&'tcx Expr<'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() {
+                if arg.span.from_expansion() {
+                    ArgKind::RefMutToPlaceAsMacro(arg)
+                } else {
+                    ArgKind::RefMutToPlace(target)
+                }
+            } else {
+                ArgKind::RefMutToTemp(target)
+            }
+        } else {
+            ArgKind::Expr(arg)
+        }
+    }
+}
+
+// Emits a note either on the temporary expression if it can be found in the same context as the
+// base and returns `true`, or on the mutable reference to the temporary expression otherwise and
+// returns `false`.
+fn emit_note(diag: &mut Diag<'_, ()>, base: &Expr<'_>, expr: &Expr<'_>, expr_temp: &Expr<'_>) -> bool {
+    if base.span.eq_ctxt(expr.span) {
+        diag.span_note(expr_temp.span.source_callsite(), MSG_TEMPORARY);
+        true
+    } else {
+        diag.span_note(expr.span.source_callsite(), MSG_TEMPORARY_REFMUT);
+        false
+    }
+}
+
+fn emit_lint_useless(
+    cx: &LateContext<'_>,
+    expr: &Expr<'_>,
+    left: &Expr<'_>,
+    right: &Expr<'_>,
+    left_temp: &Expr<'_>,
+    right_temp: &Expr<'_>,
+) {
+    span_lint_and_then(
+        cx,
+        SWAP_WITH_TEMPORARY,
+        expr.span,
+        "swapping temporary values has no effect",
+        |diag| {
+            emit_note(diag, expr, left, left_temp);
+            emit_note(diag, expr, right, right_temp);
+        },
+    );
+}
+
+fn emit_lint_assign(cx: &LateContext<'_>, expr: &Expr<'_>, target: &ArgKind<'_>, reftemp: &Expr<'_>, temp: &Expr<'_>) {
+    span_lint_and_then(
+        cx,
+        SWAP_WITH_TEMPORARY,
+        expr.span,
+        "swapping with a temporary value is inefficient",
+        |diag| {
+            if !emit_note(diag, expr, reftemp, temp) {
+                return;
+            }
+
+            // Make the suggestion only when the original `swap()` call is a statement
+            // or the last expression in a block.
+            if matches!(cx.tcx.parent_hir_node(expr.hir_id), Node::Stmt(..) | Node::Block(..)) {
+                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::RefMutToTemp(_) => unreachable!(),
+                };
+                let assign_source = Sugg::hir_with_context(cx, temp, ctxt, "_", &mut applicability);
+                diag.span_suggestion(
+                    expr.span,
+                    "use assignment instead",
+                    format!("{assign_target} = {assign_source}"),
+                    applicability,
+                );
+            }
+        },
+    );
+}

tests/ui/swap_with_temporary.fixed

@@ -0,0 +1,74 @@
+#![warn(clippy::swap_with_temporary)]
+
+use std::mem::swap;
+
+fn func() -> String {
+    String::from("func")
+}
+
+fn func_returning_refmut(s: &mut String) -> &mut String {
+    s
+}
+
+fn main() {
+    let mut x = String::from("x");
+    let mut y = String::from("y");
+    let mut zz = String::from("zz");
+    let z = &mut zz;
+
+    // No lint
+    swap(&mut x, &mut y);
+
+    y = func();
+    //~^ ERROR: swapping with a temporary value is inefficient
+
+    x = func();
+    //~^ ERROR: swapping with a temporary value is inefficient
+
+    *z = func();
+    //~^ ERROR: swapping with a temporary value is inefficient
+
+    // No lint
+    swap(z, func_returning_refmut(&mut x));
+
+    swap(&mut y, z);
+
+    *z = func();
+    //~^ ERROR: swapping with a temporary value is inefficient
+
+    macro_rules! mac {
+        (refmut $x:expr) => {
+            &mut $x
+        };
+        (funcall $f:ident) => {
+            $f()
+        };
+        (wholeexpr) => {
+            swap(&mut 42, &mut 0)
+        };
+        (ident $v:ident) => {
+            $v
+        };
+    }
+    *z = mac!(funcall func);
+    //~^ ERROR: swapping with a temporary value is inefficient
+    *mac!(ident z) = mac!(funcall func);
+    //~^ ERROR: swapping with a temporary value is inefficient
+    *mac!(ident z) = mac!(funcall func);
+    //~^ ERROR: swapping with a temporary value is inefficient
+    *mac!(refmut y) = func();
+    //~^ ERROR: swapping with a temporary value is inefficient
+
+    // No lint if it comes from a macro as it may depend on the arguments
+    mac!(wholeexpr);
+}
+
+struct S {
+    t: String,
+}
+
+fn dont_lint_those(s: &mut S, v: &mut [String], w: Option<&mut String>) {
+    swap(&mut s.t, &mut v[0]);
+    swap(&mut s.t, v.get_mut(0).unwrap());
+    swap(w.unwrap(), &mut s.t);
+}

tests/ui/swap_with_temporary.rs

@@ -0,0 +1,74 @@
+#![warn(clippy::swap_with_temporary)]
+
+use std::mem::swap;
+
+fn func() -> String {
+    String::from("func")
+}
+
+fn func_returning_refmut(s: &mut String) -> &mut String {
+    s
+}
+
+fn main() {
+    let mut x = String::from("x");
+    let mut y = String::from("y");
+    let mut zz = String::from("zz");
+    let z = &mut zz;
+
+    // No lint
+    swap(&mut x, &mut y);
+
+    swap(&mut func(), &mut y);
+    //~^ ERROR: swapping with a temporary value is inefficient
+
+    swap(&mut x, &mut func());
+    //~^ ERROR: swapping with a temporary value is inefficient
+
+    swap(z, &mut func());
+    //~^ ERROR: swapping with a temporary value is inefficient
+
+    // No lint
+    swap(z, func_returning_refmut(&mut x));
+
+    swap(&mut y, z);
+
+    swap(&mut func(), z);
+    //~^ ERROR: swapping with a temporary value is inefficient
+
+    macro_rules! mac {
+        (refmut $x:expr) => {
+            &mut $x
+        };
+        (funcall $f:ident) => {
+            $f()
+        };
+        (wholeexpr) => {
+            swap(&mut 42, &mut 0)
+        };
+        (ident $v:ident) => {
+            $v
+        };
+    }
+    swap(&mut mac!(funcall func), z);
+    //~^ ERROR: swapping with a temporary value is inefficient
+    swap(&mut mac!(funcall func), mac!(ident z));
+    //~^ ERROR: swapping with a temporary value is inefficient
+    swap(mac!(ident z), &mut mac!(funcall func));
+    //~^ ERROR: swapping with a temporary value is inefficient
+    swap(mac!(refmut y), &mut func());
+    //~^ ERROR: swapping with a temporary value is inefficient
+
+    // No lint if it comes from a macro as it may depend on the arguments
+    mac!(wholeexpr);
+}
+
+struct S {
+    t: String,
+}
+
+fn dont_lint_those(s: &mut S, v: &mut [String], w: Option<&mut String>) {
+    swap(&mut s.t, &mut v[0]);
+    swap(&mut s.t, v.get_mut(0).unwrap());
+    swap(w.unwrap(), &mut s.t);
+}