From 618b6b4d33429e35afdeacb6701bdfd1fb777b6b Mon Sep 17 00:00:00 2001 From: GnomedDev Date: Wed, 11 Sep 2024 23:54:37 +0100 Subject: [PATCH 1/2] 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 + From cb7c54d9f03738e78916cbac67b74f6bcae7aaba Mon Sep 17 00:00:00 2001 From: GnomedDev Date: Thu, 12 Sep 2024 00:25:37 +0100 Subject: [PATCH 2/2] Bless/fix unrelated tests --- CHANGELOG.md | 1 + book/src/lint_configuration.md | 10 ++++++++ .../toml_unknown_key/conf_unknown_key.stderr | 3 +++ tests/ui/await_holding_refcell_ref.rs | 1 + tests/ui/await_holding_refcell_ref.stderr | 24 +++++++++---------- tests/ui/clone_on_copy.fixed | 3 ++- tests/ui/clone_on_copy.rs | 3 ++- tests/ui/clone_on_copy.stderr | 18 +++++++------- 8 files changed, 40 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1770696c81f3..f0508dcd3cd4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6155,6 +6155,7 @@ Released 2018-09-13 [`excessive-nesting-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#excessive-nesting-threshold [`future-size-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#future-size-threshold [`ignore-interior-mutability`]: https://doc.rust-lang.org/clippy/lint_configuration.html#ignore-interior-mutability +[`large-cell-limit`]: https://doc.rust-lang.org/clippy/lint_configuration.html#large-cell-limit [`large-error-threshold`]: https://doc.rust-lang.org/clippy/lint_configuration.html#large-error-threshold [`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 diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md index 78348797588a..72e9ec744ef3 100644 --- a/book/src/lint_configuration.md +++ b/book/src/lint_configuration.md @@ -561,6 +561,16 @@ A list of paths to types that should be treated as if they do not contain interi * [`mutable_key_type`](https://rust-lang.github.io/rust-clippy/master/index.html#mutable_key_type) +## `large-cell-limit` +The maximum size of a `T` in `RefCell` to suggest to swap to `Cell` if applicable. + +**Default Value:** `128` + +--- +**Affected lints:** +* [`copy_refcell`](https://rust-lang.github.io/rust-clippy/master/index.html#copy_refcell) + + ## `large-error-threshold` The maximum size of the `Err`-variant in a `Result` returned from a function diff --git a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr index 5cf9c0fb2710..59dc1e3de7bb 100644 --- a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr +++ b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr @@ -44,6 +44,7 @@ error: error reading Clippy's configuration file: unknown field `foobar`, expect excessive-nesting-threshold future-size-threshold ignore-interior-mutability + large-cell-limit large-error-threshold literal-representation-threshold matches-for-let-else @@ -128,6 +129,7 @@ error: error reading Clippy's configuration file: unknown field `barfoo`, expect excessive-nesting-threshold future-size-threshold ignore-interior-mutability + large-cell-limit large-error-threshold literal-representation-threshold matches-for-let-else @@ -212,6 +214,7 @@ error: error reading Clippy's configuration file: unknown field `allow_mixed_uni excessive-nesting-threshold future-size-threshold ignore-interior-mutability + large-cell-limit large-error-threshold literal-representation-threshold matches-for-let-else diff --git a/tests/ui/await_holding_refcell_ref.rs b/tests/ui/await_holding_refcell_ref.rs index b0c92d8c1f6e..852e1bab126c 100644 --- a/tests/ui/await_holding_refcell_ref.rs +++ b/tests/ui/await_holding_refcell_ref.rs @@ -1,4 +1,5 @@ #![warn(clippy::await_holding_refcell_ref)] +#![allow(clippy::copy_refcell)] use std::cell::RefCell; diff --git a/tests/ui/await_holding_refcell_ref.stderr b/tests/ui/await_holding_refcell_ref.stderr index 6c7209c9ff9e..1133d027d8a0 100644 --- a/tests/ui/await_holding_refcell_ref.stderr +++ b/tests/ui/await_holding_refcell_ref.stderr @@ -1,12 +1,12 @@ error: this `RefCell` reference is held across an await point - --> tests/ui/await_holding_refcell_ref.rs:6:9 + --> tests/ui/await_holding_refcell_ref.rs:7:9 | LL | let b = x.borrow(); | ^ | = help: ensure the reference is dropped before calling `await` note: these are all the await points this reference is held through - --> tests/ui/await_holding_refcell_ref.rs:8:11 + --> tests/ui/await_holding_refcell_ref.rs:9:11 | LL | baz().await | ^^^^^ @@ -14,27 +14,27 @@ LL | baz().await = help: to override `-D warnings` add `#[allow(clippy::await_holding_refcell_ref)]` error: this `RefCell` reference is held across an await point - --> tests/ui/await_holding_refcell_ref.rs:12:9 + --> tests/ui/await_holding_refcell_ref.rs:13:9 | LL | let b = x.borrow_mut(); | ^ | = help: ensure the reference is dropped before calling `await` note: these are all the await points this reference is held through - --> tests/ui/await_holding_refcell_ref.rs:14:11 + --> tests/ui/await_holding_refcell_ref.rs:15:11 | LL | baz().await | ^^^^^ error: this `RefCell` reference is held across an await point - --> tests/ui/await_holding_refcell_ref.rs:34:9 + --> tests/ui/await_holding_refcell_ref.rs:35:9 | LL | let b = x.borrow_mut(); | ^ | = help: ensure the reference is dropped before calling `await` note: these are all the await points this reference is held through - --> tests/ui/await_holding_refcell_ref.rs:37:24 + --> tests/ui/await_holding_refcell_ref.rs:38:24 | LL | let second = baz().await; | ^^^^^ @@ -43,40 +43,40 @@ LL | let third = baz().await; | ^^^^^ error: this `RefCell` reference is held across an await point - --> tests/ui/await_holding_refcell_ref.rs:47:9 + --> tests/ui/await_holding_refcell_ref.rs:48:9 | LL | let b = x.borrow_mut(); | ^ | = help: ensure the reference is dropped before calling `await` note: these are all the await points this reference is held through - --> tests/ui/await_holding_refcell_ref.rs:50:24 + --> tests/ui/await_holding_refcell_ref.rs:51:24 | LL | let second = baz().await; | ^^^^^ error: this `RefCell` reference is held across an await point - --> tests/ui/await_holding_refcell_ref.rs:63:13 + --> tests/ui/await_holding_refcell_ref.rs:64:13 | LL | let b = x.borrow_mut(); | ^ | = help: ensure the reference is dropped before calling `await` note: these are all the await points this reference is held through - --> tests/ui/await_holding_refcell_ref.rs:65:15 + --> tests/ui/await_holding_refcell_ref.rs:66:15 | LL | baz().await | ^^^^^ error: this `RefCell` reference is held across an await point - --> tests/ui/await_holding_refcell_ref.rs:76:13 + --> tests/ui/await_holding_refcell_ref.rs:77:13 | LL | let b = x.borrow_mut(); | ^ | = help: ensure the reference is dropped before calling `await` note: these are all the await points this reference is held through - --> tests/ui/await_holding_refcell_ref.rs:78:15 + --> tests/ui/await_holding_refcell_ref.rs:79:15 | LL | baz().await | ^^^^^ diff --git a/tests/ui/clone_on_copy.fixed b/tests/ui/clone_on_copy.fixed index 9d9a5bf20f43..d53808113b02 100644 --- a/tests/ui/clone_on_copy.fixed +++ b/tests/ui/clone_on_copy.fixed @@ -6,7 +6,8 @@ clippy::unnecessary_operation, clippy::vec_init_then_push, clippy::toplevel_ref_arg, - clippy::needless_borrow + clippy::needless_borrow, + clippy::copy_refcell )] use std::cell::RefCell; diff --git a/tests/ui/clone_on_copy.rs b/tests/ui/clone_on_copy.rs index 305bc6816e1b..4f3010b7593b 100644 --- a/tests/ui/clone_on_copy.rs +++ b/tests/ui/clone_on_copy.rs @@ -6,7 +6,8 @@ clippy::unnecessary_operation, clippy::vec_init_then_push, clippy::toplevel_ref_arg, - clippy::needless_borrow + clippy::needless_borrow, + clippy::copy_refcell )] use std::cell::RefCell; diff --git a/tests/ui/clone_on_copy.stderr b/tests/ui/clone_on_copy.stderr index 314fd13afca4..c9e92f787bd9 100644 --- a/tests/ui/clone_on_copy.stderr +++ b/tests/ui/clone_on_copy.stderr @@ -1,5 +1,5 @@ error: using `clone` on type `i32` which implements the `Copy` trait - --> tests/ui/clone_on_copy.rs:23:5 + --> tests/ui/clone_on_copy.rs:24:5 | LL | 42.clone(); | ^^^^^^^^^^ help: try removing the `clone` call: `42` @@ -8,49 +8,49 @@ LL | 42.clone(); = help: to override `-D warnings` add `#[allow(clippy::clone_on_copy)]` error: using `clone` on type `i32` which implements the `Copy` trait - --> tests/ui/clone_on_copy.rs:27:5 + --> tests/ui/clone_on_copy.rs:28:5 | LL | (&42).clone(); | ^^^^^^^^^^^^^ help: try dereferencing it: `*(&42)` error: using `clone` on type `i32` which implements the `Copy` trait - --> tests/ui/clone_on_copy.rs:30:5 + --> tests/ui/clone_on_copy.rs:31:5 | LL | rc.borrow().clone(); | ^^^^^^^^^^^^^^^^^^^ help: try dereferencing it: `*rc.borrow()` error: using `clone` on type `u32` which implements the `Copy` trait - --> tests/ui/clone_on_copy.rs:33:5 + --> tests/ui/clone_on_copy.rs:34:5 | LL | x.clone().rotate_left(1); | ^^^^^^^^^ help: try removing the `clone` call: `x` error: using `clone` on type `i32` which implements the `Copy` trait - --> tests/ui/clone_on_copy.rs:47:5 + --> tests/ui/clone_on_copy.rs:48:5 | LL | m!(42).clone(); | ^^^^^^^^^^^^^^ help: try removing the `clone` call: `m!(42)` error: using `clone` on type `[u32; 2]` which implements the `Copy` trait - --> tests/ui/clone_on_copy.rs:57:5 + --> tests/ui/clone_on_copy.rs:58:5 | LL | x.clone()[0]; | ^^^^^^^^^ help: try dereferencing it: `(*x)` error: using `clone` on type `char` which implements the `Copy` trait - --> tests/ui/clone_on_copy.rs:67:14 + --> tests/ui/clone_on_copy.rs:68:14 | LL | is_ascii('z'.clone()); | ^^^^^^^^^^^ help: try removing the `clone` call: `'z'` error: using `clone` on type `i32` which implements the `Copy` trait - --> tests/ui/clone_on_copy.rs:71:14 + --> tests/ui/clone_on_copy.rs:72:14 | LL | vec.push(42.clone()); | ^^^^^^^^^^ help: try removing the `clone` call: `42` error: using `clone` on type `Option` which implements the `Copy` trait - --> tests/ui/clone_on_copy.rs:75:17 + --> tests/ui/clone_on_copy.rs:76:17 | LL | let value = opt.clone()?; // operator precedence needed (*opt)? | ^^^^^^^^^^^ help: try dereferencing it: `(*opt)`