From 618b6b4d33429e35afdeacb6701bdfd1fb777b6b Mon Sep 17 00:00:00 2001 From: GnomedDev Date: Wed, 11 Sep 2024 23:54:37 +0100 Subject: [PATCH] Implement a lint for RefCell --- CHANGELOG.md | 1 + clippy_config/src/conf.rs | 3 + clippy_lints/src/copy_refcell.rs | 118 +++++++++++++++++++++++++++++ clippy_lints/src/declared_lints.rs | 1 + clippy_lints/src/lib.rs | 2 + tests/ui/copy_refcell.rs | 12 +++ tests/ui/copy_refcell.stderr | 20 +++++ 7 files changed, 157 insertions(+) create mode 100644 clippy_lints/src/copy_refcell.rs create mode 100644 tests/ui/copy_refcell.rs create mode 100644 tests/ui/copy_refcell.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 0e5d1688e4a7..1770696c81f3 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index 6c1dd232593a..4afa21d1ddff 100644 --- a/clippy_config/src/conf.rs +++ b/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 = Vec::from(["bytes::Bytes".into()]), + /// The maximum size of a `T` in `RefCell` 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, diff --git a/clippy_lints/src/copy_refcell.rs b/clippy_lints/src/copy_refcell.rs new file mode 100644 index 000000000000..44749c97a48b --- /dev/null +++ b/clippy_lints/src/copy_refcell.rs @@ -0,0 +1,118 @@ +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` 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, + pedantic, + "usage of `RefCell` where `T` implements `Copy`" +} + +pub struct CopyRefCell { + maximum_size: u64, + avoid_breaking_exported_api: bool, +} + +impl CopyRefCell { + pub fn new(conf: &'static Conf) -> Self { + Self { + maximum_size: conf.large_cell_limit, + avoid_breaking_exported_api: conf.avoid_breaking_exported_api, + } + } + + 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, + "`RefCell` used with a type that implements `Copy`", + None, + "replace the usage of `RefCell` with `Cell`, which does not have to track borrowing at runtime", + ); + } + } +} + +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; + } + + if self.avoid_breaking_exported_api && cx.effective_visibilities.is_exported(field_def.def_id) { + 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); + } +} diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 16c64830e70d..73ef462da2c5 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/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, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 3604090b68cc..ef03a613da7e 100644 --- a/clippy_lints/src/lib.rs +++ b/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` } diff --git a/tests/ui/copy_refcell.rs b/tests/ui/copy_refcell.rs new file mode 100644 index 000000000000..98fc5a8439aa --- /dev/null +++ b/tests/ui/copy_refcell.rs @@ -0,0 +1,12 @@ +#![warn(clippy::copy_refcell)] + +use std::cell::RefCell; + +struct MyStruct { + field: RefCell, +} + +fn main() { + let local = RefCell::new(0_u8); + let large_local = RefCell::new([0_u8; 1024]); +} diff --git a/tests/ui/copy_refcell.stderr b/tests/ui/copy_refcell.stderr new file mode 100644 index 000000000000..0ebdf66c44ef --- /dev/null +++ b/tests/ui/copy_refcell.stderr @@ -0,0 +1,20 @@ +error: `RefCell` used with a type that implements `Copy` + --> tests/ui/copy_refcell.rs:6:12 + | +LL | field: RefCell, + | ^^^^^^^^^^^ + | + = help: replace the usage of `RefCell` with `Cell`, which does not have to track borrowing at runtime + = note: `-D clippy::copy-refcell` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::copy_refcell)]` + +error: `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 does not have to track borrowing at runtime + +error: aborting due to 2 previous errors +