New lint: swap_with_temporary

Author: samueltardieu

CHANGELOG.md

@@ -6153,6 +6153,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

@@ -485,6 +485,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;
@@ -4484,6 +4485,34 @@ 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 inefficient. The same
+    /// result can be achieved by using an assignment, or dropping
+    /// the swap arguments.
+    ///
+    /// ### Example
+    /// ```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");
+    /// }
+    /// ```
+    #[clippy::version = "1.86.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,
@@ -4661,6 +4690,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.
@@ -4691,6 +4721,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,142 @@
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::is_no_std_crate;
+use clippy_utils::sugg::Sugg;
+use rustc_ast::BorrowKind;
+use rustc_errors::Applicability;
+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";
+
+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, func, &args[0], &args[1], left_temp, right_temp);
+            },
+            (ArgKind::RefMutToTemp(left_temp), right) => emit_lint_assign(cx, expr, &right, left_temp),
+            (left, ArgKind::RefMutToTemp(right_temp)) => emit_lint_assign(cx, expr, &left, 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)
+        }
+    }
+}
+
+fn emit_lint_useless(
+    cx: &LateContext<'_>,
+    expr: &Expr<'_>,
+    func: &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| {
+            const DROP_MSG: &str = "drop them if creating these temporary expressions is necessary";
+
+            diag.span_note(left_temp.span, MSG_TEMPORARY);
+            diag.span_note(right_temp.span, MSG_TEMPORARY);
+
+            // If the `swap()` is a statement by itself, just propose to replace `swap(&mut a, &mut b)` by `a;
+            // b;` in order to drop `a` and `b` while acknowledging their side effects. If the
+            // `swap()` call is part of a larger expression, replace it by `{core,
+            // std}::mem::drop((a, b))`.
+            if matches!(cx.tcx.parent_hir_node(expr.hir_id), Node::Stmt(..)) {
+                diag.multipart_suggestion(
+                    DROP_MSG,
+                    vec![
+                        (func.span.with_hi(left_temp.span.lo()), String::new()),
+                        (left_temp.span.between(right_temp.span), String::from("; ")),
+                        (expr.span.with_lo(right_temp.span.hi()), String::new()),
+                    ],
+                    Applicability::MachineApplicable,
+                );
+            } else {
+                diag.multipart_suggestion(
+                    DROP_MSG,
+                    vec![
+                        (
+                            func.span,
+                            format!("{}::mem::drop(", if is_no_std_crate(cx) { "core" } else { "std" }),
+                        ),
+                        (left.span.with_hi(left_temp.span.lo()), String::new()),
+                        (right.span.with_hi(right_temp.span.lo()), String::new()),
+                        (expr.span.shrink_to_hi(), String::from(")")),
+                    ],
+                    Applicability::MachineApplicable,
+                );
+            }
+        },
+    );
+}
+
+fn emit_lint_assign(cx: &LateContext<'_>, expr: &Expr<'_>, target: &ArgKind<'_>, temp: &Expr<'_>) {
+    span_lint_and_then(
+        cx,
+        SWAP_WITH_TEMPORARY,
+        expr.span,
+        "swapping with a temporary value is inefficient",
+        |diag| {
+            diag.span_note(temp.span, MSG_TEMPORARY);
+            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);
+
+            // If the `swap()` is a statement by itself, propose to replace it by `a = b`. Otherwise, when part
+            // of a larger expression, surround the assignment with a block to make it `()`.
+            let suggestion = format!("{assign_target} = {assign_source}");
+            let suggestion = if matches!(cx.tcx.parent_hir_node(expr.hir_id), Node::Stmt(..)) {
+                suggestion
+            } else {
+                format!("{{ {suggestion}; }}")
+            };
+            diag.span_suggestion(expr.span, "use assignment instead", suggestion, applicability);
+        },
+    );
+}

tests/ui/swap_with_temporary.fixed

@@ -0,0 +1,77 @@
+#![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);
+
+    func(); func();
+    //~^ ERROR: swapping temporary values has no effect
+
+    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
+
+    // Check rewrites as part of a larger expression
+    if matches!(std::mem::drop((func(), func())), ()) {
+        //~^ ERROR: swapping temporary values has no effect
+        println!("Yeah");
+    }
+    if matches!({ *z = func(); }, ()) {
+        //~^ ERROR: swapping with a temporary value is inefficient
+        println!("Yeah");
+    }
+
+    macro_rules! mac {
+        (refmut $x:expr) => {
+            &mut $x
+        };
+        (funcall $f:ident) => {
+            $f()
+        };
+        (wholeexpr) => {
+            swap(&mut 42, &mut 0)
+        };
+        (ident $v:ident) => {
+            $v
+        };
+    }
+    *z = func();
+    //~^ ERROR: swapping with a temporary value is inefficient
+    *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);
+}

tests/ui/swap_with_temporary.rs

@@ -0,0 +1,77 @@
+#![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 func());
+    //~^ ERROR: swapping temporary values has no effect
+
+    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
+
+    // Check rewrites as part of a larger expression
+    if matches!(swap(&mut func(), &mut func()), ()) {
+        //~^ ERROR: swapping temporary values has no effect
+        println!("Yeah");
+    }
+    if matches!(swap(z, &mut func()), ()) {
+        //~^ ERROR: swapping with a temporary value is inefficient
+        println!("Yeah");
+    }
+
+    macro_rules! mac {
+        (refmut $x:expr) => {
+            &mut $x
+        };
+        (funcall $f:ident) => {
+            $f()
+        };
+        (wholeexpr) => {
+            swap(&mut 42, &mut 0)
+        };
+        (ident $v:ident) => {
+            $v
+        };
+    }
+    swap(mac!(refmut func()), z);
+    //~^ ERROR: swapping with a temporary value is inefficient
+    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!(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);
+}