New lint: mutable_borrow_of_copy

Author: samueltardieu

CHANGELOG.md

@@ -6065,6 +6065,7 @@ Released 2018-09-13
 [`mut_mut`]: https://rust-lang.github.io/rust-clippy/master/index.html#mut_mut
 [`mut_mutex_lock`]: https://rust-lang.github.io/rust-clippy/master/index.html#mut_mutex_lock
 [`mut_range_bound`]: https://rust-lang.github.io/rust-clippy/master/index.html#mut_range_bound
+[`mutable_borrow_of_copy`]: https://rust-lang.github.io/rust-clippy/master/index.html#mutable_borrow_of_copy
 [`mutable_key_type`]: https://rust-lang.github.io/rust-clippy/master/index.html#mutable_key_type
 [`mutex_atomic`]: https://rust-lang.github.io/rust-clippy/master/index.html#mutex_atomic
 [`mutex_integer`]: https://rust-lang.github.io/rust-clippy/master/index.html#mutex_integer
@@ -6512,6 +6513,7 @@ Released 2018-09-13
 [`cargo-ignore-publish`]: https://doc.rust-lang.org/clippy/lint_configuration.html#cargo-ignore-publish
 [`check-incompatible-msrv-in-tests`]: https://doc.rust-lang.org/clippy/lint_configuration.html#check-incompatible-msrv-in-tests
 [`check-inconsistent-struct-field-initializers`]: https://doc.rust-lang.org/clippy/lint_configuration.html#check-inconsistent-struct-field-initializers
+[`check-mutable-borrow-of-copy-in-tests`]: https://doc.rust-lang.org/clippy/lint_configuration.html#check-mutable-borrow-of-copy-in-tests
 [`check-private-items`]: https://doc.rust-lang.org/clippy/lint_configuration.html#check-private-items
 [`cognitive-complexity-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#cognitive-complexity-threshold
 [`disallowed-macros`]: https://doc.rust-lang.org/clippy/lint_configuration.html#disallowed-macros

book/src/lint_configuration.md

@@ -462,6 +462,16 @@ fn main() {
 * [`inconsistent_struct_constructor`](https://rust-lang.github.io/rust-clippy/master/index.html#inconsistent_struct_constructor)
 
 
+## `check-mutable-borrow-of-copy-in-tests`
+Whether to search for mutable borrows of freshly copied data in tests.
+
+**Default Value:** `true`
+
+---
+**Affected lints:**
+* [`mutable_borrow_of_copy`](https://rust-lang.github.io/rust-clippy/master/index.html#mutable_borrow_of_copy)
+
+
 ## `check-private-items`
 Whether to also run the listed lints on private items.
 

clippy_config/src/conf.rs

@@ -563,6 +563,9 @@ define_Conf! {
     /// [from rust-clippy#11846]: https://github.com/rust-lang/rust-clippy/issues/11846#issuecomment-1820747924
     #[lints(inconsistent_struct_constructor)]
     check_inconsistent_struct_field_initializers: bool = false,
+    /// Whether to search for mutable borrows of freshly copied data in tests.
+    #[lints(mutable_borrow_of_copy)]
+    check_mutable_borrow_of_copy_in_tests: bool = true,
     /// Whether to also run the listed lints on private items.
     #[lints(missing_errors_doc, missing_panics_doc, missing_safety_doc, unnecessary_safety_doc)]
     check_private_items: bool = false,

clippy_lints/src/declared_lints.rs

@@ -527,6 +527,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
     crate::mut_key::MUTABLE_KEY_TYPE_INFO,
     crate::mut_mut::MUT_MUT_INFO,
     crate::mut_reference::UNNECESSARY_MUT_PASSED_INFO,
+    crate::mutable_borrow_of_copy::MUTABLE_BORROW_OF_COPY_INFO,
     crate::mutable_debug_assertion::DEBUG_ASSERT_WITH_MUT_CALL_INFO,
     crate::mutex_atomic::MUTEX_ATOMIC_INFO,
     crate::mutex_atomic::MUTEX_INTEGER_INFO,

clippy_lints/src/lib.rs

@@ -252,6 +252,7 @@ mod multiple_unsafe_ops_per_block;
 mod mut_key;
 mod mut_mut;
 mod mut_reference;
+mod mutable_borrow_of_copy;
 mod mutable_debug_assertion;
 mod mutex_atomic;
 mod needless_arbitrary_self_type;
@@ -946,5 +947,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     store.register_late_pass(|_| Box::new(single_option_map::SingleOptionMap));
     store.register_late_pass(move |_| Box::new(redundant_test_prefix::RedundantTestPrefix));
     store.register_late_pass(|_| Box::new(cloned_ref_to_slice_refs::ClonedRefToSliceRefs::new(conf)));
+    store.register_late_pass(|_| Box::new(mutable_borrow_of_copy::MutableBorrowOfCopy::new(conf)));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }

clippy_lints/src/mutable_borrow_of_copy.rs

@@ -0,0 +1,113 @@
+use clippy_config::Conf;
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::ty::is_copy;
+use clippy_utils::{get_enclosing_block, is_in_test, path_to_local};
+use rustc_ast::BindingMode;
+use rustc_errors::Applicability;
+use rustc_hir::{Block, BorrowKind, Expr, ExprKind, Mutability, Node, PatKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::impl_lint_pass;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for taking a mutable reference on a freshly copied variable due to the use of a block returning a value implementing  `Copy`.
+    ///
+    /// ### Why is this bad?
+    /// Using a block will make a copy of the block result if its type
+    /// implements `Copy`. This might be an indication of a failed attempt
+    /// to borrow a variable.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// # unsafe fn unsafe_func(_: &mut i32) {}
+    /// let mut a = 10;
+    /// let double_a_ref = &mut unsafe { // Unsafe block needed to call `unsafe_func`
+    ///    unsafe_func(&mut a);
+    ///    a
+    /// };
+    /// ```
+    /// If you intend to take a reference on `a` and you need the block,
+    /// create the reference inside the block instead:
+    /// ```no_run
+    /// # unsafe fn unsafe_func(_: &mut i32) {}
+    /// let mut a = 10;
+    /// let double_a_ref = unsafe { // Unsafe block needed to call `unsafe_func`
+    ///    unsafe_func(&mut a);
+    ///    &mut a
+    /// };
+    /// ```
+    #[clippy::version = "1.89.0"]
+    pub MUTABLE_BORROW_OF_COPY,
+    suspicious,
+    "mutable borrow of a data which was just copied"
+}
+
+pub struct MutableBorrowOfCopy {
+    check_in_tests: bool,
+}
+
+impl MutableBorrowOfCopy {
+    pub const fn new(conf: &Conf) -> Self {
+        Self {
+            check_in_tests: conf.check_mutable_borrow_of_copy_in_tests,
+        }
+    }
+}
+
+impl_lint_pass!(MutableBorrowOfCopy => [MUTABLE_BORROW_OF_COPY]);
+
+impl LateLintPass<'_> for MutableBorrowOfCopy {
+    fn check_expr(&mut self, cx: &LateContext<'_>, expr: &Expr<'_>) {
+        if !expr.span.from_expansion()
+            && let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, sub_expr) = expr.kind
+            && let ExprKind::Block(block, _) = sub_expr.kind
+            && !block.targeted_by_break
+            && block.span.eq_ctxt(expr.span)
+            && let Some(block_expr) = block.expr
+            && let block_ty = cx.typeck_results().expr_ty_adjusted(block_expr)
+            && is_copy(cx, block_ty)
+            && is_copied_defined_outside_block(cx, block_expr, block)
+            && (self.check_in_tests || !is_in_test(cx.tcx, expr.hir_id))
+        {
+            span_lint_and_then(
+                cx,
+                MUTABLE_BORROW_OF_COPY,
+                expr.span,
+                "mutable borrow of a value which was just copied",
+                |diag| {
+                    diag.multipart_suggestion(
+                        "try building the reference inside the block",
+                        vec![
+                            (expr.span.until(block.span), String::new()),
+                            (block_expr.span.shrink_to_lo(), String::from("&mut ")),
+                        ],
+                        Applicability::MaybeIncorrect,
+                    );
+                },
+            );
+        }
+    }
+}
+
+/// Checks if `expr` denotes a mutable variable defined outside `block`. This peels away field
+/// accesses or indexing of such a variable first.
+fn is_copied_defined_outside_block(cx: &LateContext<'_>, mut expr: &Expr<'_>, block: &Block<'_>) -> bool {
+    while let ExprKind::Field(base, _) | ExprKind::Index(base, _, _) = expr.kind {
+        expr = base;
+    }
+    if let Some(mut current) = path_to_local(expr)
+        && let Node::Pat(pat) = cx.tcx.hir_node(current)
+        && matches!(pat.kind, PatKind::Binding(BindingMode::MUT, ..))
+    {
+        // Scan enclosing blocks until we find `block` (if so, the local is defined within it), or we loop
+        // or can't find blocks anymore.
+        loop {
+            match get_enclosing_block(cx, current).map(|b| b.hir_id) {
+                Some(parent) if parent == block.hir_id => return false,
+                Some(parent) if parent != current => current = parent,
+                _ => return true,
+            }
+        }
+    }
+    false
+}

tests/ui-toml/mutable_borrow_of_copy/clippy.toml

@@ -0,0 +1 @@
+check-mutable-borrow-of-copy-in-tests = false

tests/ui-toml/mutable_borrow_of_copy/mutable_borrow_of_copy.fixed

@@ -0,0 +1,10 @@
+#[test]
+fn in_test() {
+    let mut a = [10; 2];
+    let _ = &mut { a }; // Do not lint
+}
+
+fn main() {
+    let mut a = [10; 2];
+    let _ = { &mut a }; //~ mutable_borrow_of_copy
+}

tests/ui-toml/mutable_borrow_of_copy/mutable_borrow_of_copy.rs

@@ -0,0 +1,10 @@
+#[test]
+fn in_test() {
+    let mut a = [10; 2];
+    let _ = &mut { a }; // Do not lint
+}
+
+fn main() {
+    let mut a = [10; 2];
+    let _ = &mut { a }; //~ mutable_borrow_of_copy
+}

tests/ui-toml/mutable_borrow_of_copy/mutable_borrow_of_copy.stderr

@@ -0,0 +1,16 @@
+error: mutable borrow of a value which was just copied
+  --> tests/ui-toml/mutable_borrow_of_copy/mutable_borrow_of_copy.rs:9:13
+   |
+LL |     let _ = &mut { a };
+   |             ^^^^^^^^^^
+   |
+   = note: `-D clippy::mutable-borrow-of-copy` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::mutable_borrow_of_copy)]`
+help: try building the reference inside the block
+   |
+LL -     let _ = &mut { a };
+LL +     let _ = { &mut a };
+   |
+
+error: aborting due to 1 previous error
+

tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr

@@ -33,6 +33,7 @@ error: error reading Clippy's configuration file: unknown field `foobar`, expect
            cargo-ignore-publish
            check-incompatible-msrv-in-tests
            check-inconsistent-struct-field-initializers
+           check-mutable-borrow-of-copy-in-tests
            check-private-items
            cognitive-complexity-threshold
            disallowed-macros
@@ -127,6 +128,7 @@ error: error reading Clippy's configuration file: unknown field `barfoo`, expect
            cargo-ignore-publish
            check-incompatible-msrv-in-tests
            check-inconsistent-struct-field-initializers
+           check-mutable-borrow-of-copy-in-tests
            check-private-items
            cognitive-complexity-threshold
            disallowed-macros
@@ -221,6 +223,7 @@ error: error reading Clippy's configuration file: unknown field `allow_mixed_uni
            cargo-ignore-publish
            check-incompatible-msrv-in-tests
            check-inconsistent-struct-field-initializers
+           check-mutable-borrow-of-copy-in-tests
            check-private-items
            cognitive-complexity-threshold
            disallowed-macros

tests/ui/mutable_borrow_of_copy.fixed

@@ -0,0 +1,109 @@
+#![warn(clippy::mutable_borrow_of_copy)]
+#![allow(clippy::deref_addrof)]
+
+fn main() {
+    let mut a = [0u8; 2];
+    let _ = { &mut a }; //~ mutable_borrow_of_copy
+
+    let _ = &mut 'label: {
+        // Block is targeted by break
+        if a[0] == 1 {
+            break 'label 42u8;
+        }
+        a[0]
+    };
+
+    let mut a = vec![0u8; 2];
+    let _ = &mut { a }; // `a` is not `Copy`
+
+    let a = [0u8; 2];
+    let _ = &mut { a }; // `a` is not mutable
+
+    let _ = &mut { 42 }; // Do not lint on non-place expression
+
+    let _ = &mut {}; // Do not lint on empty block
+
+    macro_rules! mac {
+        ($a:expr) => {{ a }};
+    }
+    let _ = &mut mac!(a); // Do not lint on borrowed macro result
+
+    macro_rules! mac2 {
+        // Do not lint, as it depends on `Copy`-ness of `a`
+        ($x:expr) => {
+            &mut unsafe { $x }
+        };
+    }
+    let mut a = 0u8;
+    let _ = &mut mac2!(a);
+
+    let _ = &mut {
+        // Do not lint, the variable is defined inside the block
+        let mut a: [i32; 5] = (1, 2, 3, 4, 5).into();
+        a
+    };
+
+    let _ = &mut {
+        // Do not lint, the variable is defined inside the block
+        {
+            let mut a: [i32; 5] = (1, 2, 3, 4, 5).into();
+            a
+        }
+    };
+
+    struct S {
+        a: u32,
+    }
+
+    let mut s = S { a: 0 };
+    let _ = {
+        //~^ mutable_borrow_of_copy
+        s.a = 32;
+        &mut s.a
+    };
+
+    let _ = &mut {
+        // Do not lint, the variable is defined inside the block
+        let mut s = S { a: 0 };
+        s.a
+    };
+
+    let mut c = (10, 20);
+    let _ = {
+        //~^ ERROR: mutable borrow
+        &mut c.0
+    };
+
+    let _ = &mut {
+        // Do not lint, the variable is defined inside the block
+        let mut c = (10, 20);
+        c.0
+    };
+
+    let mut t = [10, 20];
+    let _ = {
+        //~^ ERROR: mutable borrow
+        &mut t[0]
+    };
+
+    let _ = &mut {
+        // Do not lint, the variable is defined inside the block
+        let mut t = [10, 20];
+        t[0]
+    };
+
+    unsafe fn unsafe_func(_: &mut i32) {}
+    let mut a = 10;
+    // Unsafe block needed to call `unsafe_func`
+    let double_a_ref = unsafe {
+        //~^ ERROR: mutable borrow
+        unsafe_func(&mut a);
+        &mut a
+    };
+}
+
+#[test]
+fn in_test() {
+    let mut a = [10; 2];
+    let _ = { &mut a }; //~ ERROR: mutable borrow
+}