diff --git a/CHANGELOG.md b/CHANGELOG.md index 62c8a63baed2..23ab08bddddb 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/book/src/lint_configuration.md b/book/src/lint_configuration.md index 92a27c5fe598..ce29750ea7c8 100644 --- a/book/src/lint_configuration.md +++ b/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. diff --git a/clippy_config/src/conf.rs b/clippy_config/src/conf.rs index 13c73e944581..ee4da0bc7da9 100644 --- a/clippy_config/src/conf.rs +++ b/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)] diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 5fd28aae0bee..8ed84a1c84f3 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/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, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 8ab462c05374..b03bf35890cf 100644 --- a/clippy_lints/src/lib.rs +++ b/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` ]] ); diff --git a/clippy_lints/src/refcell_cell.rs b/clippy_lints/src/refcell_cell.rs new file mode 100644 index 000000000000..fee1bddca137 --- /dev/null +++ b/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 for details. + /// + /// ### Example + /// ```no_run + /// use std::cell::RefCell; + /// + /// struct Counter(RefCell); + /// + /// let _ = RefCell::new(1); + /// let _ = RefCell::::default(); + /// ``` + /// Use instead: + /// ```no_run + /// use std::cell::Cell; + /// + /// struct Counter(Cell); + /// + /// let _ = Cell::new(1); + /// let _ = Cell::::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 + && 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 + && 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 + && 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 { + 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() +} diff --git a/tests/ui-toml/refcell_cell/clippy.toml b/tests/ui-toml/refcell_cell/clippy.toml new file mode 100644 index 000000000000..259eeec251c6 --- /dev/null +++ b/tests/ui-toml/refcell_cell/clippy.toml @@ -0,0 +1 @@ +max-cheap-copy-size = 64 \ No newline at end of file diff --git a/tests/ui-toml/refcell_cell/tests.rs b/tests/ui-toml/refcell_cell/tests.rs new file mode 100644 index 000000000000..70541e73c63b --- /dev/null +++ b/tests/ui-toml/refcell_cell/tests.rs @@ -0,0 +1,9 @@ +//@no-rustfix +#![warn(clippy::refcell_cell)] + +use std::cell::RefCell; + +fn main() { + let _ = RefCell::new([0_u32; 16]); //~ refcell_cell + let _ = RefCell::new([0_u32; 17]); +} diff --git a/tests/ui-toml/refcell_cell/tests.stderr b/tests/ui-toml/refcell_cell/tests.stderr new file mode 100644 index 000000000000..46408af08365 --- /dev/null +++ b/tests/ui-toml/refcell_cell/tests.stderr @@ -0,0 +1,11 @@ +error: using a `RefCell` for a `Copy` type + --> tests/ui-toml/refcell_cell/tests.rs:7:13 + | +LL | let _ = RefCell::new([0_u32; 16]); + | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Cell::new([0_u32; 16])` + | + = note: `-D clippy::refcell-cell` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::refcell_cell)]` + +error: aborting due to 1 previous error + 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 bee9e63ef742..c648522be510 100644 --- a/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr +++ b/tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr @@ -65,6 +65,7 @@ LL | foobar = 42 lint-commented-code literal-representation-threshold matches-for-let-else + max-cheap-copy-size max-fn-params-bools max-include-file-size max-struct-bools diff --git a/tests/ui/await_holding_refcell_ref.rs b/tests/ui/await_holding_refcell_ref.rs index e1db2e387497..aa1ccc4ec65a 100644 --- a/tests/ui/await_holding_refcell_ref.rs +++ b/tests/ui/await_holding_refcell_ref.rs @@ -2,6 +2,7 @@ use std::cell::RefCell; +#[expect(clippy::refcell_cell, reason = "User may intentionally use `RefCell` over `Cell`.")] async fn bad(x: &RefCell) -> u32 { let b = x.borrow(); //~^ await_holding_refcell_ref @@ -9,6 +10,7 @@ async fn bad(x: &RefCell) -> u32 { baz().await } +#[expect(clippy::refcell_cell, reason = "User may intentionally use `RefCell` over `Cell`.")] async fn bad_mut(x: &RefCell) -> u32 { let b = x.borrow_mut(); //~^ await_holding_refcell_ref @@ -16,6 +18,7 @@ async fn bad_mut(x: &RefCell) -> u32 { baz().await } +#[expect(clippy::refcell_cell, reason = "User may intentionally use `RefCell` over `Cell`.")] async fn good(x: &RefCell) -> u32 { { let b = x.borrow_mut(); @@ -30,6 +33,7 @@ async fn baz() -> u32 { 42 } +#[expect(clippy::refcell_cell, reason = "User may intentionally use `RefCell` over `Cell`.")] async fn also_bad(x: &RefCell) -> u32 { let first = baz().await; @@ -43,6 +47,7 @@ async fn also_bad(x: &RefCell) -> u32 { first + second + third } +#[expect(clippy::refcell_cell, reason = "User may intentionally use `RefCell` over `Cell`.")] async fn less_bad(x: &RefCell) -> u32 { let first = baz().await; @@ -58,6 +63,7 @@ async fn less_bad(x: &RefCell) -> u32 { first + second + third } +#[expect(clippy::refcell_cell, reason = "User may intentionally use `RefCell` over `Cell`.")] async fn not_good(x: &RefCell) -> u32 { let first = baz().await; @@ -73,6 +79,7 @@ async fn not_good(x: &RefCell) -> u32 { first + second + third } +#[expect(clippy::refcell_cell, reason = "User may intentionally use `RefCell` over `Cell`.")] #[allow(clippy::manual_async_fn)] fn block_bad(x: &RefCell) -> impl std::future::Future + '_ { async move { diff --git a/tests/ui/await_holding_refcell_ref.stderr b/tests/ui/await_holding_refcell_ref.stderr index ed8aa747bd07..bed836ab617c 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:9:11 + --> tests/ui/await_holding_refcell_ref.rs:10: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:13:9 + --> tests/ui/await_holding_refcell_ref.rs:15: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:16:11 + --> tests/ui/await_holding_refcell_ref.rs:18:11 | LL | baz().await | ^^^^^ error: this `RefCell` reference is held across an await point - --> tests/ui/await_holding_refcell_ref.rs:36:9 + --> tests/ui/await_holding_refcell_ref.rs:40: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:39:24 + --> tests/ui/await_holding_refcell_ref.rs:43: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:49:9 + --> tests/ui/await_holding_refcell_ref.rs:54: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:52:24 + --> tests/ui/await_holding_refcell_ref.rs:57:24 | LL | let second = baz().await; | ^^^^^ error: this `RefCell` reference is held across an await point - --> tests/ui/await_holding_refcell_ref.rs:65:13 + --> tests/ui/await_holding_refcell_ref.rs:71: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:68:15 + --> tests/ui/await_holding_refcell_ref.rs:74:15 | LL | baz().await | ^^^^^ error: this `RefCell` reference is held across an await point - --> tests/ui/await_holding_refcell_ref.rs:79:13 + --> tests/ui/await_holding_refcell_ref.rs:86: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:82:15 + --> tests/ui/await_holding_refcell_ref.rs:89:15 | LL | baz().await | ^^^^^ diff --git a/tests/ui/refcell_cell.rs b/tests/ui/refcell_cell.rs new file mode 100644 index 000000000000..8fa1eae424f1 --- /dev/null +++ b/tests/ui/refcell_cell.rs @@ -0,0 +1,206 @@ +//@no-rustfix +#![warn(clippy::refcell_cell)] + +use std::cell::{Cell, RefCell}; +use std::hint::black_box; + +struct NameFieldStruct<'a, T: Copy, U> { + ng: RefCell, //~ refcell_cell + ok: RefCell, + ng_ref: &'a RefCell, //~ refcell_cell + ok_ref: &'a RefCell, + large_copy: RefCell<[i32; 1024]>, +} + +struct TupleStruct( + RefCell, //~ refcell_cell + RefCell, +); + +enum Enum { + NG(RefCell), //~ refcell_cell + OK(RefCell), + Named { + ng: RefCell, //~ refcell_cell + ok: RefCell, + }, +} + +type AliasNG = RefCell; //~ refcell_cell + +type AliasOK = RefCell; + +trait Trait { + // associated type defaults are unstable + type NG; + type OK; + + fn ng(ng: RefCell); //~ refcell_cell + fn ok(ok: RefCell); + fn ng_ref(ng: &RefCell); //~ refcell_cell + fn ok_ref(ok: &RefCell); + fn ng_ret() -> RefCell; //~ refcell_cell + fn ok_ret() -> RefCell; + fn ok_ret_ref<'a>() -> &'a RefCell; // Should be linted at definition site +} + +struct Dummy; + +impl Trait for Dummy { + type NG = RefCell; //~ refcell_cell + type OK = RefCell; + + // Should be linted at definition site + fn ng(ng: RefCell) {} + fn ok(ok: RefCell) {} + fn ng_ref(ng: &RefCell) {} + fn ok_ref(ok: &RefCell) {} + fn ng_ret() -> RefCell { + todo!() + } + fn ok_ret() -> RefCell { + todo!() + } + fn ok_ret_ref<'a>() -> &'a RefCell { + todo!() + } +} + +fn func<'a, T: Copy, U>( + ng: RefCell, //~ refcell_cell + ok: RefCell, + ng_ref: &'a RefCell, //~ refcell_cell + ok_ref: &'a RefCell, +) -> ( + RefCell, //~ refcell_cell + RefCell, + &'a RefCell, // Should be linted at definition site + &'a RefCell, +) { + todo!() +} + +impl Dummy { + fn func( + ng: RefCell, //~ refcell_cell + ok: RefCell, + ) -> ( + RefCell, //~ refcell_cell + RefCell, + ) { + todo!() + } +} + +fn main() { + // Simple constructors + { + let _ = RefCell::new(1); //~ refcell_cell + let _ = RefCell::::new(1); //~ refcell_cell + let _ = RefCell::<_>::new(1); //~ refcell_cell + // non-`Copy` type + let _ = RefCell::new(vec![1]); + // large `Copy` type + let _ = RefCell::new([1; 1024]); + + let _ = RefCell::from(1); //~ refcell_cell + let _ = RefCell::::from(1); //~ refcell_cell + + let _: RefCell = Default::default(); //~ refcell_cell + let _ = as Default>::default(); //~ refcell_cell + + // False negative: This lint does not check trait bounds on call site. + let _ = black_box(RefCell::new(1)); + } + + // Macro expansion + { + macro_rules! black_box { + ( $e:expr ) => { + black_box($e) + }; + } + let _ = RefCell::new(black_box!(1)); + let _ = RefCell::from(black_box!(1)); + let _ = black_box!(RefCell::new(1)); + + macro_rules! ignore_me { + () => { + let _ = RefCell::new(1); + }; + } + ignore_me!(); + } +} + +// Suppress false positives for the simple ctor pattern +mod usage_analysis { + use std::cell::RefCell; + use std::hint::black_box; + + fn callee(x: RefCell) -> T { + todo!() + } + + fn callee_ref(x: &RefCell) -> T { + todo!() + } + + fn check_calls() { + { + let x = RefCell::new(1); //~ refcell_cell + let _ = black_box(x); + } + { + let x = RefCell::new(1); //~ refcell_cell + let _ = black_box(&x); + } + // Should be linted at definition of `callee` and `callee_ref` + { + let x = RefCell::new(1); + let _ = callee(x); + } + { + let x = RefCell::new(1); + let _ = callee_ref(&x); + } + } + + struct MethodCall {} + + impl MethodCall { + fn black_box(&self, x: T) -> T { + todo!() + } + + fn callee(&self, x: RefCell) -> T { + todo!() + } + + fn callee_ref(&self, x: &RefCell) -> T { + todo!() + } + } + + fn check_method_calls() { + let m = MethodCall {}; + + { + let x = RefCell::new(1); //~ refcell_cell + let _ = m.black_box(x); + } + { + let x = RefCell::new(1); //~ refcell_cell + let _ = m.black_box(&x); + } + // Should be linted at definition of `callee` and `callee_ref` + { + let x = RefCell::new(1); + let _ = m.callee(x); + } + { + let x = RefCell::new(1); + let _ = m.callee_ref(&x); + } + } +} diff --git a/tests/ui/refcell_cell.stderr b/tests/ui/refcell_cell.stderr new file mode 100644 index 000000000000..389d78a3e702 --- /dev/null +++ b/tests/ui/refcell_cell.stderr @@ -0,0 +1,161 @@ +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:8:9 + | +LL | ng: RefCell, + | ^^^^^^^^^^ help: try: `Cell` + | + = note: `-D clippy::refcell-cell` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::refcell_cell)]` + +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:10:17 + | +LL | ng_ref: &'a RefCell, + | ^^^^^^^^^^ help: try: `Cell` + +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:16:5 + | +LL | RefCell, + | ^^^^^^^^^^ help: try: `Cell` + +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:21:8 + | +LL | NG(RefCell), + | ^^^^^^^^^^ help: try: `Cell` + +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:24:13 + | +LL | ng: RefCell, + | ^^^^^^^^^^ help: try: `Cell` + +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:29:16 + | +LL | type AliasNG = RefCell; + | ^^^^^^^^^^^^ help: try: `Cell` + +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:38:15 + | +LL | fn ng(ng: RefCell); + | ^^^^^^^^^^ help: try: `Cell` + +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:40:20 + | +LL | fn ng_ref(ng: &RefCell); + | ^^^^^^^^^^ help: try: `Cell` + +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:42:20 + | +LL | fn ng_ret() -> RefCell; + | ^^^^^^^^^^ help: try: `Cell` + +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:50:15 + | +LL | type NG = RefCell; + | ^^^^^^^^^^ help: try: `Cell` + +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:70:9 + | +LL | ng: RefCell, + | ^^^^^^^^^^ help: try: `Cell` + +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:72:17 + | +LL | ng_ref: &'a RefCell, + | ^^^^^^^^^^ help: try: `Cell` + +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:75:5 + | +LL | RefCell, + | ^^^^^^^^^^ help: try: `Cell` + +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:85:13 + | +LL | ng: RefCell, + | ^^^^^^^^^^ help: try: `Cell` + +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:88:9 + | +LL | RefCell, + | ^^^^^^^^^^ help: try: `Cell` + +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:98:17 + | +LL | let _ = RefCell::new(1); + | ^^^^^^^^^^^^^^^ help: try: `Cell::new(1)` + +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:99:17 + | +LL | let _ = RefCell::::new(1); + | ^^^^^^^^^^^^^^^^^^^^^^ help: try: `Cell::::new(1)` + +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:100:17 + | +LL | let _ = RefCell::<_>::new(1); + | ^^^^^^^^^^^^^^^^^^^^ help: try: `Cell::new(1)` + +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:106:17 + | +LL | let _ = RefCell::from(1); + | ^^^^^^^^^^^^^^^^ help: try: `Cell::from(1)` + +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:107:17 + | +LL | let _ = RefCell::::from(1); + | ^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Cell::::from(1)` + +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:109:16 + | +LL | let _: RefCell = Default::default(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Cell = Cell::default();` + +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:110:17 + | +LL | let _ = as Default>::default(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `Cell::::default()` + +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:151:21 + | +LL | let x = RefCell::new(1); + | ^^^^^^^^^^^^^^^ help: try: `Cell::new(1)` + +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:155:21 + | +LL | let x = RefCell::new(1); + | ^^^^^^^^^^^^^^^ help: try: `Cell::new(1)` + +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:189:21 + | +LL | let x = RefCell::new(1); + | ^^^^^^^^^^^^^^^ help: try: `Cell::new(1)` + +error: using a `RefCell` for a `Copy` type + --> tests/ui/refcell_cell.rs:193:21 + | +LL | let x = RefCell::new(1); + | ^^^^^^^^^^^^^^^ help: try: `Cell::new(1)` + +error: aborting due to 26 previous errors +