Implement a lint for RefCell<impl Copy>

Author: GnomedDev

CHANGELOG.md

@@ -5374,6 +5374,7 @@ Released 2018-09-13
 [`const_is_empty`]: https://rust-lang.github.io/rust-clippy/master/index.html#const_is_empty
 [`const_static_lifetime`]: https://rust-lang.github.io/rust-clippy/master/index.html#const_static_lifetime
 [`copy_iterator`]: https://rust-lang.github.io/rust-clippy/master/index.html#copy_iterator
+[`copy_refcell`]: https://rust-lang.github.io/rust-clippy/master/index.html#copy_refcell
 [`crate_in_macro_def`]: https://rust-lang.github.io/rust-clippy/master/index.html#crate_in_macro_def
 [`create_dir`]: https://rust-lang.github.io/rust-clippy/master/index.html#create_dir
 [`crosspointer_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#crosspointer_transmute

clippy_config/src/conf.rs

@@ -471,6 +471,9 @@ define_Conf! {
     /// A list of paths to types that should be treated as if they do not contain interior mutability
     #[lints(borrow_interior_mutable_const, declare_interior_mutable_const, ifs_same_cond, mutable_key_type)]
     ignore_interior_mutability: Vec<String> = Vec::from(["bytes::Bytes".into()]),
+    /// The maximum size of a `T` in `RefCell<T>` to suggest to swap to `Cell` if applicable.
+    #[lints(copy_refcell)]
+    large_cell_limit: u64 = 128,
     /// The maximum size of the `Err`-variant in a `Result` returned from a function
     #[lints(result_large_err)]
     large_error_threshold: u64 = 128,

clippy_lints/src/copy_refcell.rs

@@ -0,0 +1,112 @@
+use clippy_config::Conf;
+use rustc_hir::{FieldDef, LetStmt, LocalSource};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty;
+use rustc_middle::ty::layout::TyAndLayout;
+use rustc_session::impl_lint_pass;
+use rustc_span::symbol::sym;
+use rustc_span::Span;
+
+use clippy_utils::diagnostics::span_lint_and_help;
+use clippy_utils::ty::implements_trait;
+
+declare_clippy_lint! {
+    /// ### What it does
+    ///
+    /// Detects crate-local usage of `RefCell<T>` where `T` implements `Copy`
+    ///
+    /// ### Why is this bad?
+    ///
+    /// `RefCell` has to perform extra book-keeping in order to support references, whereas `Cell` does not.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// let interior_mutable = std::cell::RefCell::new(0_u8);
+    ///
+    /// *interior_mutable.borrow_mut() = 1;
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// let interior_mutable = std::cell::Cell::new(0_u8);
+    ///
+    /// interior_mutable.set(1);
+    /// ```
+    #[clippy::version = "1.83.0"]
+    pub COPY_REFCELL,
+    perf,
+    "usage of `RefCell<T>` where `T` implements `Copy`"
+}
+
+pub struct CopyRefcell {
+    maximum_size: u64,
+}
+
+impl CopyRefcell {
+    pub fn new(conf: &'static Conf) -> Self {
+        Self {
+            maximum_size: conf.large_cell_limit,
+        }
+    }
+
+    fn check_copy_refcell<'tcx>(&self, cx: &LateContext<'tcx>, ty: ty::Ty<'tcx>, span: Span) {
+        let Some(copy_def_id) = cx.tcx.get_diagnostic_item(sym::Copy) else {
+            return;
+        };
+
+        let ty::Adt(adt, generics) = ty.kind() else {
+            return;
+        };
+
+        if !cx.tcx.is_diagnostic_item(sym::RefCell, adt.did()) {
+            return;
+        }
+
+        let inner_ty = generics.type_at(0);
+        let Ok(TyAndLayout { layout, .. }) = cx.tcx.layout_of(cx.param_env.and(inner_ty)) else {
+            return;
+        };
+
+        if layout.size().bytes() >= self.maximum_size {
+            return;
+        }
+
+        if implements_trait(cx, inner_ty, copy_def_id, &[]) {
+            span_lint_and_help(
+                cx,
+                COPY_REFCELL,
+                span,
+                "std::cell::RefCell used with a type that implements Copy",
+                None,
+                "replace the usage of RefCell with Cell, which is simpler and zero-cost",
+            );
+        }
+    }
+}
+
+impl_lint_pass!(CopyRefcell => [COPY_REFCELL]);
+
+impl<'tcx> LateLintPass<'tcx> for CopyRefcell {
+    fn check_field_def(&mut self, cx: &LateContext<'tcx>, field_def: &'tcx FieldDef<'tcx>) {
+        // Don't want to lint macro expansions.
+        if field_def.span.from_expansion() {
+            return;
+        }
+
+        let field_ty = rustc_hir_analysis::lower_ty(cx.tcx, field_def.ty);
+        self.check_copy_refcell(cx, field_ty, field_def.ty.span);
+    }
+
+    fn check_local(&mut self, cx: &LateContext<'tcx>, local_def: &'tcx LetStmt<'tcx>) {
+        // Don't want to lint macro expansions or desugaring.
+        if local_def.span.from_expansion() || !matches!(local_def.source, LocalSource::Normal) {
+            return;
+        }
+
+        let Some(init_expr) = local_def.init else {
+            return;
+        };
+
+        let init_ty = cx.typeck_results().expr_ty(init_expr);
+        self.check_copy_refcell(cx, init_ty, init_expr.span);
+    }
+}

clippy_lints/src/declared_lints.rs

@@ -108,6 +108,7 @@ pub static LINTS: &[&crate::LintInfo] = &[
     crate::copies::IF_SAME_THEN_ELSE_INFO,
     crate::copies::SAME_FUNCTIONS_IN_IF_CONDITION_INFO,
     crate::copy_iterator::COPY_ITERATOR_INFO,
+    crate::copy_refcell::COPY_REFCELL_INFO,
     crate::crate_in_macro_def::CRATE_IN_MACRO_DEF_INFO,
     crate::create_dir::CREATE_DIR_INFO,
     crate::dbg_macro::DBG_MACRO_INFO,

clippy_lints/src/lib.rs

@@ -99,6 +99,7 @@ mod collection_is_never_read;
 mod comparison_chain;
 mod copies;
 mod copy_iterator;
+mod copy_refcell;
 mod crate_in_macro_def;
 mod create_dir;
 mod dbg_macro;
@@ -942,5 +943,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     store.register_late_pass(move |_| Box::new(manual_div_ceil::ManualDivCeil::new(conf)));
     store.register_late_pass(|_| Box::new(manual_is_power_of_two::ManualIsPowerOfTwo));
     store.register_late_pass(|_| Box::new(non_zero_suggestions::NonZeroSuggestions));
+    store.register_late_pass(move |_| Box::new(copy_refcell::CopyRefcell::new(conf)));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }

tests/ui/copy_refcell.rs

@@ -0,0 +1,12 @@
+#![warn(clippy::copy_refcell)]
+
+use std::cell::RefCell;
+
+struct MyStruct {
+    field: RefCell<u8>,
+}
+
+fn main() {
+    let local = RefCell::new(0_u8);
+    let large_local = RefCell::new([0_u8; 1024]);
+}

tests/ui/copy_refcell.stderr

@@ -0,0 +1,20 @@
+error: std::cell::RefCell used with a type that implements Copy
+  --> tests/ui/copy_refcell.rs:6:12
+   |
+LL |     field: RefCell<u8>,
+   |            ^^^^^^^^^^^
+   |
+   = help: replace the usage of RefCell with Cell, which is simpler and zero-cost
+   = note: `-D clippy::copy-refcell` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::copy_refcell)]`
+
+error: std::cell::RefCell used with a type that implements Copy
+  --> tests/ui/copy_refcell.rs:10:17
+   |
+LL |     let local = RefCell::new(0_u8);
+   |                 ^^^^^^^^^^^^^^^^^^
+   |
+   = help: replace the usage of RefCell with Cell, which is simpler and zero-cost
+
+error: aborting due to 2 previous errors
+