From f25f387de43c5a3d7dfbf5d20ad8d5c06be13ec7 Mon Sep 17 00:00:00 2001 From: Lukas Lueg Date: Sat, 3 Sep 2022 16:21:48 +0200 Subject: [PATCH] Initial implementation of `mut_refcell_borrow` Fixes #9044 --- CHANGELOG.md | 1 + clippy_lints/src/lib.register_all.rs | 1 + clippy_lints/src/lib.register_lints.rs | 1 + clippy_lints/src/lib.register_style.rs | 1 + clippy_lints/src/methods/mod.rs | 56 +++++++- .../src/methods/mut_refcell_borrow.rs | 123 ++++++++++++++++++ clippy_utils/src/paths.rs | 1 + src/docs.rs | 1 + src/docs/mut_refcell_borrow.txt | 40 ++++++ tests/ui/mut_refcell_borrow.rs | 44 +++++++ tests/ui/mut_refcell_borrow.stderr | 55 ++++++++ 11 files changed, 323 insertions(+), 1 deletion(-) create mode 100644 clippy_lints/src/methods/mut_refcell_borrow.rs create mode 100644 src/docs/mut_refcell_borrow.txt create mode 100644 tests/ui/mut_refcell_borrow.rs create mode 100644 tests/ui/mut_refcell_borrow.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 257add86b6e5..46447aa4a5ac 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 1f85382347aa..246444b0fdd8 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/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), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index 13b573beea1b..ec67b0fceeca 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/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, diff --git a/clippy_lints/src/lib.register_style.rs b/clippy_lints/src/lib.register_style.rs index 05d2ec2e9e1e..fdd6c4fbc177 100644 --- a/clippy_lints/src/lib.register_style.rs +++ b/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), diff --git a/clippy_lints/src/methods/mod.rs b/clippy_lints/src/methods/mod.rs index a0d190a58aff..6c5aafe1793e 100644 --- a/clippy_lints/src/methods/mod.rs +++ b/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 { + /// // 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) { + /// // 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 { + /// // No need for an implicit check + /// std::mem::replace(x.get_mut(), 42) + /// } + /// + /// fn bar(x: &mut RefCell) { + /// // 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; diff --git a/clippy_lints/src/methods/mut_refcell_borrow.rs b/clippy_lints/src/methods/mut_refcell_borrow.rs new file mode 100644 index 000000000000..bb0c192bd239 --- /dev/null +++ b/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!(), + }; + } +} diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index fb0d34e02eec..047b0261034b 100644 --- a/clippy_utils/src/paths.rs +++ b/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 diff --git a/src/docs.rs b/src/docs.rs index 69243bf4d9c9..529a47dbdad1 100644 --- a/src/docs.rs +++ b/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", diff --git a/src/docs/mut_refcell_borrow.txt b/src/docs/mut_refcell_borrow.txt new file mode 100644 index 000000000000..96719edcb508 --- /dev/null +++ b/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 { + // 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) { + // 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 { + // No need for an implicit check + std::mem::replace(x.get_mut(), 42) +} + +fn bar(x: &mut RefCell) { + // No need for an error path + *x.get_mut() = 42; +} +``` \ No newline at end of file diff --git a/tests/ui/mut_refcell_borrow.rs b/tests/ui/mut_refcell_borrow.rs new file mode 100644 index 000000000000..d152a845ccb2 --- /dev/null +++ b/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) { + x.replace(0); +} + +pub fn replace_with(x: &mut RefCell) { + x.replace_with(|&mut old| old + 1); +} + +pub fn borrow(x: &mut RefCell) { + let _: i32 = *x.borrow(); +} + +pub fn try_borrow(x: &mut RefCell) { + let _: i32 = *x.try_borrow().unwrap(); +} + +pub fn borrow_mut(x: &mut RefCell) { + *x.borrow_mut() += 1; +} + +pub fn try_borrow_mut(x: &mut RefCell) { + *x.try_borrow_mut().unwrap() += 1; +} + +pub fn take(x: &mut RefCell) { + let _: i32 = x.take(); +} + +// must not lint +pub fn deref_refcell(x: Rc>) { + *x.borrow_mut() += 1; +} + +// must not lint +pub fn mut_deref_refcell(x: &mut Rc>) { + *x.borrow_mut() += 1; +} + +fn main() {} diff --git a/tests/ui/mut_refcell_borrow.stderr b/tests/ui/mut_refcell_borrow.stderr new file mode 100644 index 000000000000..7848c527e638 --- /dev/null +++ b/tests/ui/mut_refcell_borrow.stderr @@ -0,0 +1,55 @@ +error: calling `&mut RefCell::replace()` unnecessarily performs a runtime-check that can never fail + --> $DIR/mut_refcell_borrow.rs:7:7 + | +LL | x.replace(0); + | ^^^^^^^ + | + = note: `-D clippy::mut-refcell-borrow` implied by `-D warnings` + = help: use `.get_mut()` to get a mutable reference to the value, and replace the value using `std::mem::replace()` or direct assignment + +error: calling `&mut RefCell::replace_with()` unnecessarily performs a runtime-check that can never fail + --> $DIR/mut_refcell_borrow.rs:11:7 + | +LL | x.replace_with(|&mut old| old + 1); + | ^^^^^^^^^^^^ + | + = help: use `.get_mut()` to get a mutable reference to the value, and replace the value using `std::mem::replace()` or direct assignment + +error: calling `&mut RefCell::borrow()` unnecessarily performs a runtime-check that can never fail + --> $DIR/mut_refcell_borrow.rs:15:21 + | +LL | let _: i32 = *x.borrow(); + | ^^^^^^ help: change this to: `get_mut` + +error: calling `&mut RefCell::try_borrow()` unnecessarily performs a runtime-check that can never fail + --> $DIR/mut_refcell_borrow.rs:19:21 + | +LL | let _: i32 = *x.try_borrow().unwrap(); + | ^^^^^^^^^^ + | + = help: use `.get_mut()` instead of `.try_borrow()` to get a reference to the value; remove the error-handling + +error: calling `&mut RefCell::borrow_mut()` unnecessarily performs a runtime-check that can never fail + --> $DIR/mut_refcell_borrow.rs:23:8 + | +LL | *x.borrow_mut() += 1; + | ^^^^^^^^^^ help: change this to: `get_mut` + +error: calling `&mut RefCell::try_borrow_mut()` unnecessarily performs a runtime-check that can never fail + --> $DIR/mut_refcell_borrow.rs:27:8 + | +LL | *x.try_borrow_mut().unwrap() += 1; + | ^^^^^^^^^^^^^^ + | + = help: use `.get_mut()` instead of `.try_borrow_mut()` to get a mutable reference to the value; remove the error-handling + +error: calling `&mut RefCell::take()` unnecessarily performs a runtime-check that can never fail + --> $DIR/mut_refcell_borrow.rs:31:20 + | +LL | let _: i32 = x.take(); + | ^^^^ + | + = help: use `.get_mut()` to get a mutable reference to the value, and `std::mem::take()` to get ownership via that reference + +error: aborting due to 7 previous errors +