New lint: Suggests using Cell<T: Copy> instead of RefCell<T: Copy>

CHANGELOG.md

@@ -7394,6 +7394,7 @@ Released 2018-09-13
 [`ref_option`]: https://rust-lang.github.io/rust-clippy/main/index.html#ref_option
 [`ref_option_ref`]: https://rust-lang.github.io/rust-clippy/main/index.html#ref_option_ref
 [`ref_patterns`]: https://rust-lang.github.io/rust-clippy/main/index.html#ref_patterns
+[`refcell_cell`]: https://rust-lang.github.io/rust-clippy/main/index.html#refcell_cell
 [`regex_creation_in_loops`]: https://rust-lang.github.io/rust-clippy/main/index.html#regex_creation_in_loops
 [`regex_macro`]: https://rust-lang.github.io/rust-clippy/main/index.html#regex_macro
 [`renamed_function_params`]: https://rust-lang.github.io/rust-clippy/main/index.html#renamed_function_params
@@ -7722,6 +7723,7 @@ Released 2018-09-13
 [`lint-commented-code`]: https://doc.rust-lang.org/clippy/lint_configuration.html#lint-commented-code
 [`literal-representation-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#literal-representation-threshold
 [`matches-for-let-else`]: https://doc.rust-lang.org/clippy/lint_configuration.html#matches-for-let-else
+[`max-cheap-copy-size`]: https://doc.rust-lang.org/clippy/lint_configuration.html#max-cheap-copy-size
 [`max-fn-params-bools`]: https://doc.rust-lang.org/clippy/lint_configuration.html#max-fn-params-bools
 [`max-include-file-size`]: https://doc.rust-lang.org/clippy/lint_configuration.html#max-include-file-size
 [`max-struct-bools`]: https://doc.rust-lang.org/clippy/lint_configuration.html#max-struct-bools

book/src/lint_configuration.md

@@ -810,6 +810,16 @@ be filtering for common types.
 * [`manual_let_else`](https://rust-lang.github.io/rust-clippy/main/index.html#manual_let_else)
 
 
+## `max-cheap-copy-size`
+Maximum size in bytes for a `Copy` type to be considered cheap to copy.
+
+**Default Value:** `16`
+
+---
+**Affected lints:**
+* [`refcell_cell`](https://rust-lang.github.io/rust-clippy/main/index.html#refcell_cell)
+
+
 ## `max-fn-params-bools`
 The maximum number of bool parameters a function can have.
 Use `0` to lint on any function with a bool parameter.

clippy_config/src/conf.rs

@@ -597,6 +597,9 @@ define_Conf! {
     /// be filtering for common types.
     #[lints(manual_let_else)]
     matches_for_let_else("matches-for-let-else"): MatchLintBehaviour = MatchLintBehaviour::WellKnownTypes,
+    /// Maximum size in bytes for a `Copy` type to be considered cheap to copy.
+    #[lints(refcell_cell)]
+    max_cheap_copy_size("max-cheap-copy-size"): u64 = 16,
     /// The maximum number of bool parameters a function can have.
     /// Use `0` to lint on any function with a bool parameter.
     #[lints(fn_params_excessive_bools)]

clippy_lints/src/declared_lints.rs

@@ -682,6 +682,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
     crate::redundant_type_annotations::REDUNDANT_TYPE_ANNOTATIONS_INFO,
     crate::ref_option_ref::REF_OPTION_REF_INFO,
     crate::ref_patterns::REF_PATTERNS_INFO,
+    crate::refcell_cell::REFCELL_CELL_INFO,
     crate::reference::DEREF_ADDROF_INFO,
     crate::regex::INVALID_REGEX_INFO,
     crate::regex::REGEX_CREATION_IN_LOOPS_INFO,

clippy_lints/src/lib.rs

@@ -322,6 +322,7 @@ mod redundant_test_prefix;
 mod redundant_type_annotations;
 mod ref_option_ref;
 mod ref_patterns;
+mod refcell_cell;
 mod reference;
 mod regex;
 mod repeat_vec_with_capacity;
@@ -869,6 +870,7 @@ rustc_lint::late_lint_methods!(
         BlockScrutinee: block_scrutinee::BlockScrutinee = block_scrutinee::BlockScrutinee,
         NonnullUncheckedOnBoxPtr: nonnull_unchecked_on_box_ptr::NonnullUncheckedOnBoxPtr = nonnull_unchecked_on_box_ptr::NonnullUncheckedOnBoxPtr::new(conf),
         NeedlessNonzeroGet: needless_nonzero_get::NeedlessNonzeroGet = needless_nonzero_get::NeedlessNonzeroGet::new(conf),
+        RefcellCell: refcell_cell::RefcellCell = refcell_cell::RefcellCell::new(conf),
         // add late passes here, used by `cargo dev new_lint`
     ]]
 );

clippy_lints/src/refcell_cell.rs

@@ -0,0 +1,329 @@
+use std::ops::ControlFlow::{Break, Continue};
+
+use clippy_config::Conf;
+use clippy_utils::diagnostics::span_lint_and_sugg;
+use clippy_utils::res::{MaybeDef as _, MaybeQPath as _, MaybeResPath as _};
+use clippy_utils::source::{snippet_with_applicability, snippet_with_context};
+use clippy_utils::ty::{approx_ty_size, is_copy, ty_from_hir_ty};
+use clippy_utils::visitors::for_each_local_use_after_expr;
+use clippy_utils::{ExprUseNode, expr_use_sites, is_trait_impl_item, qpath_generic_tys, sym};
+use rustc_errors::Applicability;
+use rustc_hir::def_id::LocalDefId;
+use rustc_hir::intravisit::FnKind;
+use rustc_hir::{
+    self, Body, Expr, ExprKind, FieldDef, FnDecl, FnRetTy, GenericArg, ImplItem, ImplItemKind, Item, ItemKind, Pat,
+    QPath, Stmt, StmtKind, TraitItem, TraitItemKind, Ty, TyKind,
+};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_middle::ty;
+use rustc_session::impl_lint_pass;
+use rustc_span::Span;
+use rustc_span::sym::RefCell;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// This lint suggests using `Cell` for `Copy` types instead of `RefCell`.
+    ///
+    /// ### Why is this bad?
+    /// `RefCell` avoids cloning at the cost of additional memory usage and
+    /// instructions, which isn't worth it for **small** `Copy` types.
+    ///
+    /// ### Known problems
+    /// `RefCell` might be useful for **large** `Copy` types.
+    /// See <https://manishearth.github.io/blog/2015/05/27/wrapper-types-in-rust-choosing-your-guarantees/#cell-types> for details.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// use std::cell::RefCell;
+    ///
+    /// struct Counter(RefCell<i32>);
+    ///
+    /// let _ = RefCell::new(1);
+    /// let _ = RefCell::<i32>::default();
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// use std::cell::Cell;
+    ///
+    /// struct Counter(Cell<i32>);
+    ///
+    /// let _ = Cell::new(1);
+    /// let _ = Cell::<i32>::default();
+    /// ```
+    #[clippy::version = "1.99.0"]
+    pub REFCELL_CELL,
+    perf,
+    "using a `RefCell` for a `Copy` type"
+}
+
+impl_lint_pass!(RefcellCell => [REFCELL_CELL]);
+
+pub struct RefcellCell {
+    max_cheap_copy_size: u64,
+}
+
+impl RefcellCell {
+    pub fn new(conf: &'static Conf) -> Self {
+        Self {
+            max_cheap_copy_size: conf.max_cheap_copy_size,
+        }
+    }
+
+    fn emit_refcell_copy_def<'tcx>(&self, cx: &LateContext<'tcx>, hir_ty: &'tcx Ty<'tcx>, no_ref: bool) {
+        match hir_ty.kind {
+            TyKind::Array(hir_ty, _) | TyKind::Slice(hir_ty) => {
+                self.emit_refcell_copy_def(cx, hir_ty, no_ref);
+            },
+            TyKind::Tup(hir_tys) => {
+                for hir_ty in hir_tys {
+                    self.emit_refcell_copy_def(cx, hir_ty, no_ref);
+                }
+            },
+            TyKind::Ref(_, mut_ty) if !no_ref => {
+                self.emit_refcell_copy_def(cx, mut_ty.ty, no_ref);
+            },
+            TyKind::Path(qpath)
+                if let (None, Some(path)) = qpath.opt_res_path()
+                // RefCell<T>
+                && let [segment] = path.segments
+                && segment.res.is_diag_item(&cx.tcx, RefCell)
+                && let Some(args) = segment.args
+                && let [GenericArg::Type(hir_ty)] = args.args
+                // `T` is copyable and small
+                && {
+                    let ty = ty_from_hir_ty(cx, hir_ty.as_unambig_ty());
+                    is_copy(cx, ty) && approx_ty_size(cx, ty) <= self.max_cheap_copy_size
+                }
+                // Be conservative about macro
+                && !hir_ty.span.from_expansion() =>
+            {
+                let mut app = Applicability::MaybeIncorrect;
+                let sugg = {
+                    let hir_ty = snippet_with_applicability(cx, hir_ty.span, "_", &mut app);
+                    format!("Cell<{hir_ty}>")
+                };
+                span_lint_and_sugg(
+                    cx,
+                    REFCELL_CELL,
+                    path.span,
+                    "using a `RefCell` for a `Copy` type",
+                    "try",
+                    sugg,
+                    app,
+                );
+            },
+            _ => (),
+        }
+    }
+}
+
+impl<'tcx> LateLintPass<'tcx> for RefcellCell {
+    fn check_field_def(&mut self, cx: &LateContext<'tcx>, def: &'tcx FieldDef<'tcx>) {
+        if !def.span.from_expansion() {
+            self.emit_refcell_copy_def(cx, def.ty, false);
+        }
+    }
+
+    fn check_fn(
+        &mut self,
+        cx: &LateContext<'tcx>,
+        _: FnKind<'tcx>,
+        decl: &'tcx FnDecl<'tcx>,
+        _: &'tcx Body<'tcx>,
+        span: Span,
+        def_id: LocalDefId,
+    ) {
+        if !span.from_expansion() && !is_trait_impl_item(cx, cx.tcx.local_def_id_to_hir_id(def_id)) {
+            for hir_ty in decl.inputs {
+                self.emit_refcell_copy_def(cx, hir_ty, false);
+            }
+            if let FnRetTy::Return(hir_ty) = decl.output {
+                self.emit_refcell_copy_def(cx, hir_ty, true);
+            }
+        }
+    }
+
+    fn check_impl_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx ImplItem<'tcx>) {
+        if let ImplItemKind::Type(hir_ty) = item.kind
+            && !item.span.from_expansion()
+        {
+            self.emit_refcell_copy_def(cx, hir_ty, false);
+        }
+    }
+
+    fn check_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx Item<'tcx>) {
+        if let ItemKind::TyAlias(_, _, hir_ty) = item.kind
+            && !item.span.from_expansion()
+        {
+            self.emit_refcell_copy_def(cx, hir_ty, false);
+        }
+    }
+
+    fn check_trait_item(&mut self, cx: &LateContext<'tcx>, item: &'tcx TraitItem<'tcx>) {
+        if let TraitItemKind::Fn(sig, _) = item.kind
+            && !item.span.from_expansion()
+        {
+            for hir_ty in sig.decl.inputs {
+                self.emit_refcell_copy_def(cx, hir_ty, false);
+            }
+            if let FnRetTy::Return(hir_ty) = sig.decl.output {
+                self.emit_refcell_copy_def(cx, hir_ty, true);
+            }
+        }
+    }
+
+    fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'tcx>) {
+        if let StmtKind::Let(let_stmt) = stmt.kind
+            && !stmt.span.from_expansion()
+            && let Some(init) = let_stmt.init
+            && !callee_requires_refcell(cx, let_stmt.pat, init)
+            // RefCell<T>
+            && let Some(ty) = cx.typeck_results().expr_ty_opt(init)
+            && let ty::Adt(def, args) = ty.kind()
+            && def.is_diag_item(&cx.tcx, RefCell)
+            // `T` is a small `Copy` type
+            && is_copy(cx, args.type_at(0))
+            && approx_ty_size(cx, args.type_at(0)) <= self.max_cheap_copy_size
+            // `init` is a (simple) constructor of `RefCell`
+            && let ExprKind::Call(maybe_qpath, ctor_args) = init.kind
+            && let Some(kind) = CtorKind::new(cx, maybe_qpath, ctor_args.len())
+            // Be conservative about macro
+            && ctor_args.iter().all(|e| !e.span.from_expansion())
+        {
+            // Suggestion can be a false positive because trait bounds are not considered.
+            let mut app = Applicability::MaybeIncorrect;
+
+            let init_str = {
+                let arg = ctor_args
+                    .first()
+                    .map_or_default(|arg| snippet_with_context(cx, arg.span, stmt.span.ctxt(), "..", &mut app).0);
+                match kind {
+                    CtorKind::New => format!("Cell::new({arg})"),
+                    CtorKind::From => format!("Cell::from({arg})"),
+                    CtorKind::Default => "Cell::default()".to_string(),
+                }
+            };
+
+            let (span, sugg) = if let Some(annotation) = let_stmt.ty
+                && let TyKind::Path(qpath) = annotation.kind
+                && let Some(inner_ty) = qpath_generic_tys(&qpath).next()
+            {
+                let sugg = {
+                    let inner_ty = snippet_with_context(cx, inner_ty.span, stmt.span.ctxt(), "_", &mut app).0;
+
+                    format!(/* let pat: */ "Cell<{inner_ty}> = {init_str};")
+                };
+
+                (annotation.span.to(init.span), sugg)
+            } else {
+                let sugg = if let ExprKind::Path(qpath) = maybe_qpath.kind
+                    // <..>::method -> <..>
+                    && let Some(ty) = match qpath {
+                        QPath::Resolved(ty, _) => ty,
+                        QPath::TypeRelative(ty, _) => Some(ty),
+                    }
+                    // (..::)RefCell::<T> -> T
+                    && let TyKind::Path(qpath) = ty.kind
+                    && let Some(ty) = qpath_generic_tys(&qpath).next()
+                {
+                    let ty = snippet_with_context(cx, ty.span, stmt.span.ctxt(), "_", &mut app).0;
+                    init_str.replacen("::", &format!("::<{ty}>::"), 1)
+                } else {
+                    init_str
+                };
+
+                (init.span, sugg)
+            };
+
+            span_lint_and_sugg(
+                cx,
+                REFCELL_CELL,
+                span,
+                "using a `RefCell` for a `Copy` type",
+                "try",
+                sugg,
+                app,
+            );
+        }
+    }
+}
+
+enum CtorKind {
+    New,
+    From,
+    Default,
+}
+
+impl CtorKind {
+    fn new<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, num_args: usize) -> Option<Self> {
+        if let Some(def_id) = expr.res(cx).opt_def_id()
+            && def_id.is_diag_item(&cx.tcx, sym::from_fn)
+            && num_args == 1
+        {
+            Some(Self::From)
+        } else if let Some(def_id) = expr.res(cx).opt_def_id()
+            && def_id.is_diag_item(&cx.tcx, sym::default_fn)
+            && num_args == 0
+        {
+            Some(Self::Default)
+        } else if let ExprKind::Path(QPath::TypeRelative(hir_ty, segment)) = expr.kind
+            && segment.ident.name == sym::new
+            && hir_ty.res(cx).is_diag_item(&cx.tcx, RefCell)
+            && num_args == 1
+        {
+            Some(Self::New)
+        } else {
+            None
+        }
+    }
+}
+
+/// Checks whether the given local variable must be a `RefCell`.
+///
+/// This performs only simple analysis because a full analysis would be too expensive.
+/// Trait bounds and indirect uses are not considered.
+fn callee_requires_refcell<'tcx>(cx: &LateContext<'tcx>, pat: &'tcx Pat<'tcx>, init: &'tcx Expr<'tcx>) -> bool {
+    for_each_local_use_after_expr(cx, pat.hir_id, init.hir_id, |expr| {
+        let Some(use_site) = expr_use_sites(cx.tcx, cx.typeck_results(), expr.span.ctxt(), expr)
+            // Ignore `&`s because we want to know whether `expr` is passed to a function.
+            .find(|use_site| !matches!(use_site.use_node(cx), ExprUseNode::AddrOf(..)))
+        else {
+            return Continue(());
+        };
+
+        let (def_id, i) = match use_site.use_node(cx) {
+            ExprUseNode::FnArg(path, i) if let Some(def_id) = path.res(cx).opt_def_id() => (def_id, i),
+            ExprUseNode::MethodArg(hir_id, _, i)
+                if let Some(def_id) = cx.typeck_results().type_dependent_def_id(hir_id) =>
+            {
+                (def_id, i)
+            },
+            ExprUseNode::Callee => return Break(()),
+            _ => return Continue(()),
+        };
+
+        // Does callee require `RefCell`?
+        let Some(input_ty) = cx
+            .tcx
+            .fn_sig(def_id)
+            // Inspect the type as declared by the callee, without normalization.
+            .instantiate_identity()
+            .skip_norm_wip()
+            .skip_binder()
+            .inputs()
+            .get(i)
+        else {
+            // FIXME: This arm seems unreachable but is required to avoid a panic from
+            // an `Option::unwrap` inside `FnSig::input` when linting some crates on Linux.
+            // Why?
+            return Break(());
+        };
+
+        if input_ty.peel_refs().is_diag_item(&cx.tcx, RefCell) {
+            Break(())
+        } else {
+            Continue(())
+        }
+    })
+    .is_break()
+}

tests/ui-toml/refcell_cell/clippy.toml

@@ -0,0 +1 @@
+max-cheap-copy-size = 64