Initial implementation of mut_refcell_borrow

Fixes #9044

Author: lukaslueg

CHANGELOG.md

@@ -3908,6 +3908,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
+[`mut_refcell_borrow`]: https://rust-lang.github.io/rust-clippy/master/index.html#mut_refcell_borrow
 [`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

clippy_lints/src/lib.register_all.rs

@@ -186,6 +186,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![
     LintId::of(methods::MAP_FLATTEN),
     LintId::of(methods::MAP_IDENTITY),
     LintId::of(methods::MUT_MUTEX_LOCK),
+    LintId::of(methods::MUT_REFCELL_BORROW),
     LintId::of(methods::NEEDLESS_OPTION_AS_DEREF),
     LintId::of(methods::NEEDLESS_OPTION_TAKE),
     LintId::of(methods::NEEDLESS_SPLITN),

clippy_lints/src/lib.register_lints.rs

@@ -334,6 +334,7 @@ store.register_lints(&[
     methods::MAP_IDENTITY,
     methods::MAP_UNWRAP_OR,
     methods::MUT_MUTEX_LOCK,
+    methods::MUT_REFCELL_BORROW,
     methods::NAIVE_BYTECOUNT,
     methods::NEEDLESS_OPTION_AS_DEREF,
     methods::NEEDLESS_OPTION_TAKE,

clippy_lints/src/lib.register_style.rs

@@ -71,6 +71,7 @@ store.register_group(true, "clippy::style", Some("clippy_style"), vec![
     LintId::of(methods::MAP_CLONE),
     LintId::of(methods::MAP_COLLECT_RESULT_UNIT),
     LintId::of(methods::MUT_MUTEX_LOCK),
+    LintId::of(methods::MUT_REFCELL_BORROW),
     LintId::of(methods::NEW_RET_NO_SELF),
     LintId::of(methods::OBFUSCATED_IF_ELSE),
     LintId::of(methods::OK_EXPECT),

clippy_lints/src/methods/mod.rs

@@ -53,6 +53,7 @@ mod map_flatten;
 mod map_identity;
 mod map_unwrap_or;
 mod mut_mutex_lock;
+mod mut_refcell_borrow;
 mod needless_option_as_deref;
 mod needless_option_take;
 mod no_effect_replace;
@@ -2765,6 +2766,53 @@ declare_clippy_lint! {
     "`&mut Mutex::lock` does unnecessary locking"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for `&mut std::cell::RefCell` method calls which perform
+    /// runtime-checks that the compiler can statically guarantee
+    ///
+    /// ### Why is this bad?
+    /// Methods on `RefCell` explicitly or implicitly perform a runtime-check
+    /// to guarantee the borrowing rules. If called on a `&mut RefCell` we
+    /// can statically guarantee that the borrowing rules are upheld.
+    ///
+    /// ### Example
+    /// ```
+    /// use std::cell::RefCell;
+    ///
+    /// fn foo(x: &mut RefCell<i32>) -> i32 {
+    ///     // This implicitly panics if the value is already borrowed. But it
+    ///     // can't be borrowed because we have an exclusive reference to it
+    ///     x.replace(42)
+    /// }
+    ///
+    /// fn bar(x: &mut RefCell<i32>) {
+    ///     // This check can never fail
+    ///     if let Ok(mut value) = x.try_borrow_mut() {
+    ///         *value = 42;
+    ///     }
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```
+    /// use std::cell::RefCell;
+    ///
+    /// fn foo(x: &mut RefCell<i32>) -> i32 {
+    ///     // No need for an implicit check
+    ///     std::mem::replace(x.get_mut(), 42)
+    /// }
+    ///
+    /// fn bar(x: &mut RefCell<i32>) {
+    ///     // No need for an error path
+    ///     *x.get_mut() = 42;
+    /// }
+    /// ```
+    #[clippy::version = "1.64.0"]
+    pub MUT_REFCELL_BORROW,
+    style,
+    "method call to `&mut RefCell` performs unnecessary runtime-check"
+}
+
 declare_clippy_lint! {
     /// ### What it does
     /// Checks for duplicate open options as well as combinations
@@ -3149,6 +3197,7 @@ impl_lint_pass!(Methods => [
     MAP_CLONE,
     MAP_ERR_IGNORE,
     MUT_MUTEX_LOCK,
+    MUT_REFCELL_BORROW,
     NONSENSICAL_OPEN_OPTIONS,
     PATH_BUF_PUSH_OVERWRITE,
     RANGE_ZIP_WITH_LEN,
@@ -3492,6 +3541,8 @@ impl Methods {
                 ("lock", []) => {
                     mut_mutex_lock::check(cx, expr, recv, span);
                 },
+                (name @ ("replace" | "replace_with"), [arg]) => mut_refcell_borrow::check(cx, expr, recv, span, name, Some(arg)),
+                (name @ ("borrow" | "try_borrow" | "borrow_mut" | "try_borrow_mut"), []) => mut_refcell_borrow::check(cx, expr, recv, span, name, None),
                 (name @ ("map" | "map_err"), [m_arg]) => {
                     if name == "map" {
                         map_clone::check(cx, expr, recv, m_arg, self.msrv);
@@ -3597,7 +3648,10 @@ impl Methods {
                         }
                     }
                 },
-                ("take", []) => needless_option_take::check(cx, expr, recv),
+                ("take", []) => {
+                    needless_option_take::check(cx, expr, recv);
+                    mut_refcell_borrow::check(cx, expr, recv, span, "take", None);
+                },
                 ("then", [arg]) => {
                     if !meets_msrv(self.msrv, msrvs::BOOL_THEN_SOME) {
                         return;

clippy_lints/src/methods/mut_refcell_borrow.rs

@@ -0,0 +1,123 @@
+use clippy_utils::diagnostics::{span_lint_and_help, span_lint_and_sugg};
+use clippy_utils::{expr_custom_deref_adjustment, match_def_path, paths};
+use rustc_errors::Applicability;
+use rustc_hir::{Expr, Mutability};
+use rustc_lint::LateContext;
+use rustc_middle::ty;
+use rustc_span::Span;
+
+use super::MUT_REFCELL_BORROW;
+
+//TODO calls to RefCell's `Clone`-impl could be replaced by `RefCell::new(foo.get_mut().clone())`
+//to circumvent the runtime-check
+
+fn emit_replace(cx: &LateContext<'_>, name_span: Span) {
+    span_lint_and_help(
+        cx,
+        MUT_REFCELL_BORROW,
+        name_span,
+        "calling `&mut RefCell::replace()` unnecessarily performs a runtime-check that can never fail",
+        None,
+        "use `.get_mut()` to get a mutable reference to the value, and replace the value using `std::mem::replace()` or direct assignment",
+    );
+}
+
+fn emit_replace_with(cx: &LateContext<'_>, name_span: Span) {
+    span_lint_and_help(
+        cx,
+        MUT_REFCELL_BORROW,
+        name_span,
+        "calling `&mut RefCell::replace_with()` unnecessarily performs a runtime-check that can never fail",
+        None,
+        "use `.get_mut()` to get a mutable reference to the value, and replace the value using `std::mem::replace()` or direct assignment",
+    );
+}
+
+fn emit_borrow(cx: &LateContext<'_>, name_span: Span) {
+    // This is not MachineApplicable as `borrow` returns a `Ref` while `get_mut` returns a
+    // `&mut T`, and we don't check surrounding types
+    span_lint_and_sugg(
+        cx,
+        MUT_REFCELL_BORROW,
+        name_span,
+        "calling `&mut RefCell::borrow()` unnecessarily performs a runtime-check that can never fail",
+        "change this to",
+        "get_mut".to_owned(),
+        Applicability::MaybeIncorrect,
+    );
+}
+
+fn emit_try_borrow(cx: &LateContext<'_>, name_span: Span) {
+    span_lint_and_help(
+        cx,
+        MUT_REFCELL_BORROW,
+        name_span,
+        "calling `&mut RefCell::try_borrow()` unnecessarily performs a runtime-check that can never fail",
+        None,
+        "use `.get_mut()` instead of `.try_borrow()` to get a reference to the value; remove the error-handling",
+    );
+}
+
+fn emit_borrow_mut(cx: &LateContext<'_>, name_span: Span) {
+    // This is not MachineApplicable as `borrow_mut` returns a different type than `get_mut`, for
+    // which we don't check
+    span_lint_and_sugg(
+        cx,
+        MUT_REFCELL_BORROW,
+        name_span,
+        "calling `&mut RefCell::borrow_mut()` unnecessarily performs a runtime-check that can never fail",
+        "change this to",
+        "get_mut".to_owned(),
+        Applicability::MaybeIncorrect,
+    );
+}
+
+fn emit_try_borrow_mut(cx: &LateContext<'_>, name_span: Span) {
+    span_lint_and_help(
+        cx,
+        MUT_REFCELL_BORROW,
+        name_span,
+        "calling `&mut RefCell::try_borrow_mut()` unnecessarily performs a runtime-check that can never fail",
+        None,
+        "use `.get_mut()` instead of `.try_borrow_mut()` to get a mutable reference to the value; remove the error-handling",
+    );
+}
+
+fn emit_take(cx: &LateContext<'_>, name_span: Span) {
+    span_lint_and_help(
+        cx,
+        MUT_REFCELL_BORROW,
+        name_span,
+        "calling `&mut RefCell::take()` unnecessarily performs a runtime-check that can never fail",
+        None,
+        "use `.get_mut()` to get a mutable reference to the value, and `std::mem::take()` to get ownership via that reference",
+    );
+}
+
+pub(super) fn check<'tcx>(
+    cx: &LateContext<'tcx>,
+    ex: &'tcx Expr<'tcx>,
+    recv: &'tcx Expr<'tcx>,
+    name_span: Span,
+    name: &'tcx str,
+    arg: Option<&'tcx Expr<'tcx>>,
+) {
+    if matches!(expr_custom_deref_adjustment(cx, recv), None | Some(Mutability::Mut))
+      && let ty::Ref(_, _, Mutability::Mut) = cx.typeck_results().expr_ty(recv).kind()
+      && let Some(method_id) = cx.typeck_results().type_dependent_def_id(ex.hir_id)
+      && let Some(impl_id) = cx.tcx.impl_of_method(method_id)
+      && match_def_path(cx, impl_id, &paths::REFCELL)
+    {
+        //TODO: Use `arg` to emit better suggestions
+        match (name, arg) {
+            ("replace", Some(_arg)) => emit_replace(cx, name_span),
+            ("replace_with", Some(_arg)) => emit_replace_with(cx, name_span),
+            ("borrow", None) => emit_borrow(cx, name_span),
+            ("try_borrow", None) => emit_try_borrow(cx, name_span),
+            ("borrow_mut", None) => emit_borrow_mut(cx, name_span),
+            ("try_borrow_mut", None) => emit_try_borrow_mut(cx, name_span),
+            ("take", None) => emit_take(cx, name_span),
+            _ => unreachable!(),
+        };
+    }
+}

clippy_utils/src/paths.rs

@@ -122,6 +122,7 @@ pub const PTR_WRITE_VOLATILE: [&str; 3] = ["core", "ptr", "write_volatile"];
 pub const PUSH_STR: [&str; 4] = ["alloc", "string", "String", "push_str"];
 pub const RANGE_ARGUMENT_TRAIT: [&str; 3] = ["core", "ops", "RangeBounds"];
 pub const RC_PTR_EQ: [&str; 4] = ["alloc", "rc", "Rc", "ptr_eq"];
+pub const REFCELL: [&str; 3] = ["core", "cell", "RefCell"];
 pub const REFCELL_REF: [&str; 3] = ["core", "cell", "Ref"];
 pub const REFCELL_REFMUT: [&str; 3] = ["core", "cell", "RefMut"];
 #[expect(clippy::invalid_paths)] // internal lints do not know about all external crates

src/docs.rs

@@ -326,6 +326,7 @@ docs! {
     "mut_mut",
     "mut_mutex_lock",
     "mut_range_bound",
+    "mut_refcell_borrow",
     "mutable_key_type",
     "mutex_atomic",
     "mutex_integer",

src/docs/mut_refcell_borrow.txt

@@ -0,0 +1,40 @@
+### What it does
+Checks for `&mut std::cell::RefCell` method calls which perform
+runtime-checks that the compiler can statically guarantee
+
+### Why is this bad?
+Methods on `RefCell` explicitly or implicitly perform a runtime-check
+to guarantee the borrowing rules. If called on a `&mut RefCell` we
+can statically guarantee that the borrowing rules are upheld.
+
+### Example
+```
+use std::cell::RefCell;
+
+fn foo(x: &mut RefCell<i32>) -> i32 {
+    // This implicitly panics if the value is already borrowed. But it
+    // can't be borrowed because we have an exclusive reference to it
+    x.replace(42)
+}
+
+fn bar(x: &mut RefCell<i32>) {
+    // This check can never fail
+    if let Ok(value) = x.try_borrow_mut() {
+        *value = 42;
+    }
+}
+```
+Use instead:
+```
+use std::cell::RefCell;
+
+fn foo(x: &mut RefCell<i32>) -> i32 {
+    // No need for an implicit check
+    std::mem::replace(x.get_mut(), 42)
+}
+
+fn bar(x: &mut RefCell<i32>) {
+    // No need for an error path
+    *x.get_mut() = 42;
+}
+```

tests/ui/mut_refcell_borrow.rs

@@ -0,0 +1,44 @@
+#![warn(clippy::mut_mutex_lock)]
+
+use std::cell::RefCell;
+use std::rc::Rc;
+
+pub fn replace(x: &mut RefCell<i32>) {
+    x.replace(0);
+}
+
+pub fn replace_with(x: &mut RefCell<i32>) {
+    x.replace_with(|&mut old| old + 1);
+}
+
+pub fn borrow(x: &mut RefCell<i32>) {
+    let _: i32 = *x.borrow();
+}
+
+pub fn try_borrow(x: &mut RefCell<i32>) {
+    let _: i32 = *x.try_borrow().unwrap();
+}
+
+pub fn borrow_mut(x: &mut RefCell<i32>) {
+    *x.borrow_mut() += 1;
+}
+
+pub fn try_borrow_mut(x: &mut RefCell<i32>) {
+    *x.try_borrow_mut().unwrap() += 1;
+}
+
+pub fn take(x: &mut RefCell<i32>) {
+    let _: i32 = x.take();
+}
+
+// must not lint
+pub fn deref_refcell(x: Rc<RefCell<i32>>) {
+    *x.borrow_mut() += 1;
+}
+
+// must not lint
+pub fn mut_deref_refcell(x: &mut Rc<RefCell<i32>>) {
+    *x.borrow_mut() += 1;
+}
+
+fn main() {}