diff --git a/CHANGELOG.md b/CHANGELOG.md index f6814f0a9c7e..08fda0d156a7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7068,6 +7068,7 @@ Released 2018-09-13 [`float_equality_without_abs`]: https://rust-lang.github.io/rust-clippy/main/index.html#float_equality_without_abs [`fn_address_comparisons`]: https://rust-lang.github.io/rust-clippy/main/index.html#fn_address_comparisons [`fn_null_check`]: https://rust-lang.github.io/rust-clippy/main/index.html#fn_null_check +[`fn_param_ref_cloned`]: https://rust-lang.github.io/rust-clippy/main/index.html#fn_param_ref_cloned [`fn_params_excessive_bools`]: https://rust-lang.github.io/rust-clippy/main/index.html#fn_params_excessive_bools [`fn_to_numeric_cast`]: https://rust-lang.github.io/rust-clippy/main/index.html#fn_to_numeric_cast [`fn_to_numeric_cast_any`]: https://rust-lang.github.io/rust-clippy/main/index.html#fn_to_numeric_cast_any diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index da0bd90cc50c..0456d3c8945a 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -173,6 +173,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[ crate::float_literal::LOSSY_FLOAT_LITERAL_INFO, crate::floating_point_arithmetic::IMPRECISE_FLOPS_INFO, crate::floating_point_arithmetic::SUBOPTIMAL_FLOPS_INFO, + crate::fn_param_ref_cloned::FN_PARAM_REF_CLONED_INFO, crate::format::USELESS_FORMAT_INFO, crate::format_args::FORMAT_IN_FORMAT_ARGS_INFO, crate::format_args::POINTER_FORMAT_INFO, diff --git a/clippy_lints/src/fn_param_ref_cloned.rs b/clippy_lints/src/fn_param_ref_cloned.rs new file mode 100644 index 000000000000..d7bf876ff389 --- /dev/null +++ b/clippy_lints/src/fn_param_ref_cloned.rs @@ -0,0 +1,180 @@ +use clippy_utils::res::MaybeResPath as _; +use clippy_utils::ty::{implements_trait, ty_from_hir_ty}; +use clippy_utils::visitors::{Descend, for_each_expr}; +use rustc_hir::PatKind; +use rustc_lint::{LateContext, LateLintPass, declare_lint_pass}; +use rustc_middle::ty::{Ref, Ty}; +use rustc_span::Span; +use rustc_span::def_id::DefId; +use std::ops::ControlFlow; + +declare_clippy_lint! { + /// ### Example + /// ```norun + /// #[derive(Clone)] + /// struct A; + /// + /// pub fn foo(item: &A) { + /// let cloned_ref = item.clone(); + /// } + /// ``` + /// Instead, take it by value, to demand the ownership of the data right away. + /// Or try reworking the function to use another type. + /// ```norun + /// #[derive(Clone)] + /// struct A; + /// + /// pub fn foo_by_value(item: A) { + /// let cloned_ref = item.clone(); + /// } + /// + /// pub fn foo_with_arc(item: &A) { + /// // Turns out we don't actually need ownership! + /// } + /// ``` + #[clippy::version = "1.98.0"] + pub FN_PARAM_REF_CLONED, + pedantic, + "you should pass by value instead of cloning a passed reference" +} + +declare_lint_pass!(FnParamRefCloned => [FN_PARAM_REF_CLONED]); + +type Candidates = Vec<((rustc_hir::HirId, Span), Vec<(rustc_hir::HirId, Span)>)>; + +/// Returns true if `ty` is `&T` where `T` implements any trait in `must_impl_trait` +pub fn is_candidate_ty<'a>(cx: &LateContext<'a>, ty: Ty<'a>, must_impl_trait: &[DefId]) -> bool { + if let Ref(_, ty_ref, mutability) = ty.kind() { + must_impl_trait + .iter() + .any(|def_id| implements_trait(cx, *ty_ref, *def_id, &[])) + && mutability.is_not() + } else { + false + } +} + +pub fn get_param_id_span(param: &rustc_hir::Param<'_>) -> Option<(rustc_hir::HirId, Span)> { + if let PatKind::Binding(_, hir_id, ident, _) = param.pat.kind + && !ident.span.from_expansion() + && !ident.is_reserved() + { + Some((hir_id, param.ty_span)) + } else { + None + } +} + +impl<'tcx> LateLintPass<'tcx> for FnParamRefCloned { + fn check_fn( + &mut self, + cx: &LateContext<'tcx>, + fn_kind: rustc_hir::intravisit::FnKind<'tcx>, + fn_decl: &'tcx rustc_hir::FnDecl<'tcx>, + fn_body: &'tcx rustc_hir::Body<'tcx>, + _: Span, + _: rustc_span::def_id::LocalDefId, + ) { + let mut candidates: Candidates; + + if let rustc_hir::intravisit::FnKind::Closure = fn_kind { + return; + } + + // Define which traits must be implemented for the lint to work + let must_impl_trait = [ + cx.tcx.lang_items().clone_trait().unwrap(), + cx.tcx.lang_items().drop_trait().unwrap(), + ]; + + // Get all candidates of params that implement said traits and zip them with function signature + // params + candidates = fn_decl + .inputs + .iter() + .zip(fn_body.params) + .filter_map(|(ty, param)| { + if let Some((id, span)) = get_param_id_span(param) + && is_candidate_ty(cx, ty_from_hir_ty(cx, ty), &must_impl_trait) + { + Some(((id, span), Vec::default())) + } else { + None + } + }) + .collect(); + + // Find all rebinds of param values in the function and add them to the original candidates (tuple) + let rustc_hir::ExprKind::Block(block, _) = fn_body.value.kind else { + return; + }; + + for statement in block.stmts { + if let rustc_hir::StmtKind::Let(let_stmt) = statement.kind + && let Some(expr) = let_stmt.init + && let rustc_hir::ExprKind::Path(qpath) = expr.kind + && let Some(hir_id) = qpath.res_local_id() + { + for (cand, rebinds) in &mut candidates { + if cand.0 == hir_id { + rebinds.push((let_stmt.pat.hir_id, let_stmt.span)); + } + } + } + } + + // Look whether the candidates call the `.clone()` method anywhere + _ = for_each_expr(cx.tcx, fn_body.value, move |expr| match expr.kind { + rustc_hir::ExprKind::If(_, _, _) | rustc_hir::ExprKind::Match(_, _, _) => { + ControlFlow::<(), Descend>::Continue(Descend::No) + }, + rustc_hir::ExprKind::MethodCall(method_name, receiver, args, span) + if method_name.ident.as_str() == "clone" + && args.is_empty() + && let rustc_hir::ExprKind::Path(qpath) = receiver.kind + && let Some(hir_id) = qpath.res_local_id() => + { + for (original_candidate, rebinds) in &candidates { + emit_lint(cx, span, original_candidate, rebinds, hir_id); + } + ControlFlow::<(), Descend>::Continue(Descend::Yes) + }, + + _ => ControlFlow::<(), Descend>::Continue(Descend::Yes), + }); + } +} + +fn emit_lint( + cx: &LateContext<'_>, + span: Span, + original_candidate: &(rustc_hir::HirId, Span), + rebinds: &Vec<(rustc_hir::HirId, Span)>, + hir_id: rustc_hir::HirId, +) { + if original_candidate.0 == hir_id { + clippy_utils::diagnostics::span_lint_and_note( + cx, + FN_PARAM_REF_CLONED, + span, + "function gets a parameter by reference, but you later unconditionally clone it", + Some(original_candidate.1), + "consider passing the reference by value instead", + ); + } + + for rebind in rebinds { + if rebind.0 == hir_id { + clippy_utils::diagnostics::span_lint_and_then( + cx, + FN_PARAM_REF_CLONED, + span, + "function gets a parameter by reference, but you later rebind and unconditionally clone it", + |diag| { + diag.span_note(rebind.1, "you bind the parameter into a new binding here") + .span_note(original_candidate.1, "the parameter is passed by reference..."); + }, + ); + } + } +} diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 6d49f44b466b..f6d62e7f4fdb 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -140,6 +140,7 @@ mod fallible_impl_from; mod field_scoped_visibility_modifiers; mod float_literal; mod floating_point_arithmetic; +mod fn_param_ref_cloned; mod format; mod format_args; mod format_impl; @@ -871,6 +872,7 @@ rustc_lint::late_lint_methods!( RestWhenDestructuringStruct: rest_when_destructuring_struct::RestWhenDestructuringStruct = rest_when_destructuring_struct::RestWhenDestructuringStruct, BlockScrutinee: block_scrutinee::BlockScrutinee = block_scrutinee::BlockScrutinee, NonnullUncheckedOnBoxPtr: nonnull_unchecked_on_box_ptr::NonnullUncheckedOnBoxPtr = nonnull_unchecked_on_box_ptr::NonnullUncheckedOnBoxPtr::new(conf), + FnParamRefCloned: fn_param_ref_cloned::FnParamRefCloned = fn_param_ref_cloned::FnParamRefCloned, NeedlessNonzeroGet: needless_nonzero_get::NeedlessNonzeroGet = needless_nonzero_get::NeedlessNonzeroGet::new(conf), // add late passes here, used by `cargo dev new_lint` ]] diff --git a/clippy_lints/src/unnested_or_patterns.rs b/clippy_lints/src/unnested_or_patterns.rs index 184427c4dba7..bbe28df7af92 100644 --- a/clippy_lints/src/unnested_or_patterns.rs +++ b/clippy_lints/src/unnested_or_patterns.rs @@ -62,7 +62,7 @@ impl UnnestedOrPatterns { impl EarlyLintPass for UnnestedOrPatterns { fn check_arm(&mut self, cx: &EarlyContext<'_>, a: &ast::Arm) { if self.msrv.meets(msrvs::OR_PATTERNS) { - lint_unnested_or_patterns(cx, &a.pat); + lint_unnested_or_patterns(cx, *a.pat.clone()); } } @@ -70,32 +70,32 @@ impl EarlyLintPass for UnnestedOrPatterns { if self.msrv.meets(msrvs::OR_PATTERNS) && let ast::ExprKind::Let(pat, _, _, _) = &e.kind { - lint_unnested_or_patterns(cx, pat); + lint_unnested_or_patterns(cx, *pat.clone()); } } fn check_param(&mut self, cx: &EarlyContext<'_>, p: &ast::Param) { if self.msrv.meets(msrvs::OR_PATTERNS) { - lint_unnested_or_patterns(cx, &p.pat); + lint_unnested_or_patterns(cx, *p.pat.clone()); } } fn check_local(&mut self, cx: &EarlyContext<'_>, l: &ast::Local) { if self.msrv.meets(msrvs::OR_PATTERNS) { - lint_unnested_or_patterns(cx, &l.pat); + lint_unnested_or_patterns(cx, *l.pat.clone()); } } extract_msrv_attr!(); } -fn lint_unnested_or_patterns(cx: &EarlyContext<'_>, pat: &Pat) { +fn lint_unnested_or_patterns(cx: &EarlyContext<'_>, pat: Pat) { if let Ident(.., None) | Expr(_) | Wild | Path(..) | Range(..) | Rest | MacCall(_) = pat.kind { // This is a leaf pattern, so cloning is unprofitable. return; } - let mut pat = pat.clone(); + let mut pat = pat; // Nix all the paren patterns everywhere so that they aren't in our way. remove_all_parens(&mut pat); diff --git a/tests/ui/fn_param_ref_cloned.rs b/tests/ui/fn_param_ref_cloned.rs new file mode 100644 index 000000000000..f7dfd4b07903 --- /dev/null +++ b/tests/ui/fn_param_ref_cloned.rs @@ -0,0 +1,156 @@ +#![allow(unused)] +#![warn(clippy::fn_param_ref_cloned)] +#![feature(stmt_expr_attributes)] + +// Impl methods +#[derive(Clone, Default)] +pub struct IsClone; + +#[derive(Default)] +pub struct IsNotClone; + +#[derive(Default)] +pub struct PartialNotClone { + clone_field: IsClone, + not_clone_field: IsNotClone, +} + +#[derive(Default)] +pub struct PartialClone { + clone_field: IsClone, + not_clone_field: IsNotClone, +} + +impl IsNotClone { + fn clone(&self) {} +} + +// We know about this edgecase situation. I am not entirely sure how to filter this case +fn set_cell(cell: &std::cell::Cell) { + cell.set(5); + let a = cell.clone(); + //~^ fn_param_ref_cloned +} + +fn create_cell() { + let a = std::cell::Cell::new(0); + set_cell(&a); + println!("{}", a.get()); +} + +impl IsClone { + fn this_is_not_clone(&self) {} + + pub fn no_ref(&self) { + let is_clone = IsClone; + let cloned_no_ref = is_clone.clone(); + } + + pub fn cloning_ref(&self, is_clone: &IsClone) { + let cloned_ref_param = is_clone.clone(); + //~^ fn_param_ref_cloned + } + + pub fn using_ref(&self, is_clone: &IsClone) { + let x = ""; + let b = is_clone; + } +} + +pub fn test_gen_function(is_clone_ref: &T, is_clone_owned: T) { + // lint when we clone a param that is a reference + is_clone_ref.clone(); + //~^ fn_param_ref_cloned + + // don't lint when we call clone on an owned parameter + is_clone_owned.clone(); +} + +pub fn basic_clone_function(is_not_clone_ref: &IsNotClone, is_clone_ref: &IsClone, is_clone_owned: IsClone) { + // lint when we clone a param that is a reference + is_clone_ref.clone(); + //~^ fn_param_ref_cloned + + // don't lint when we call a different method + is_clone_ref.this_is_not_clone(); + + // don't lint when we call clone on an owned parameter + is_clone_owned.clone(); + + // don't lint when we call clone on a type that doesn't implement the trait Clone + is_not_clone_ref.clone(); + + // don't lint when we clone a local variable of IsClone type + let local_is_clone = IsClone; + local_is_clone.clone(); + + // or any other method of that local variable + local_is_clone.this_is_not_clone(); + + // lint when we clone on a re-binding of a cloneable reference + let rebound = is_clone_ref; + rebound.clone(); + //~^ fn_param_ref_cloned +} + +fn partial_clone( + partial_not_clone_owned: PartialNotClone, + partial_not_clone_ref: &PartialNotClone, + partial_clone_owned: PartialClone, + partial_clone_ref: &PartialClone, +) { + // Move owned values + let moved = partial_not_clone_owned.clone_field; + let moved = partial_not_clone_owned.not_clone_field; + + let moved = partial_clone_owned.clone_field; + let moved = partial_clone_owned.not_clone_field; + + // Clone only struct fields on non-clone struct + let cloned = partial_not_clone_ref.clone_field.clone(); + + // Clone only struct fields on clone struct + let cloned = partial_clone_ref.clone_field.clone(); +} + +fn dont_check_if_stmts(clone_ref: &IsClone, if_arg: usize) { + if if_arg == 0usize { + let should_allow_clone = clone_ref.clone(); + } + + let should_not_allow_clone = clone_ref.clone(); + //~^ fn_param_ref_cloned +} + +#[derive(Clone)] +pub struct IsClone2<'a>(&'a IsClone); + +impl<'a> From<&'a IsClone> for IsClone2<'a> { + fn from(value: &'a IsClone) -> Self { + IsClone2(value) + } +} + +fn rebind(y: &IsClone) { + let x: IsClone2 = y.into(); + let z = x.clone(); +} + +fn main() { + let a = IsClone; + let b = IsClone; + let c = IsNotClone; + basic_clone_function(&c, &a, b); + + let d = PartialNotClone::default(); + let e = PartialClone::default(); + let f = PartialNotClone::default(); + let g = PartialClone::default(); + partial_clone(d, &f, e, &g); + + let h = std::cell::Cell::new(0); + set_cell(&h); + create_cell(); + dont_check_if_stmts(&a, 0usize); + rebind(&a); +} diff --git a/tests/ui/fn_param_ref_cloned.stderr b/tests/ui/fn_param_ref_cloned.stderr new file mode 100644 index 000000000000..dea758acd8c8 --- /dev/null +++ b/tests/ui/fn_param_ref_cloned.stderr @@ -0,0 +1,81 @@ +error: function gets a parameter by reference, but you later unconditionally clone it + --> tests/ui/fn_param_ref_cloned.rs:31:18 + | +LL | let a = cell.clone(); + | ^^^^^^^ + | +note: consider passing the reference by value instead + --> tests/ui/fn_param_ref_cloned.rs:29:19 + | +LL | fn set_cell(cell: &std::cell::Cell) { + | ^^^^^^^^^^^^^^^^^^^^^ + = note: `-D clippy::fn-param-ref-cloned` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::fn_param_ref_cloned)]` + +error: function gets a parameter by reference, but you later unconditionally clone it + --> tests/ui/fn_param_ref_cloned.rs:50:41 + | +LL | let cloned_ref_param = is_clone.clone(); + | ^^^^^^^ + | +note: consider passing the reference by value instead + --> tests/ui/fn_param_ref_cloned.rs:49:41 + | +LL | pub fn cloning_ref(&self, is_clone: &IsClone) { + | ^^^^^^^^ + +error: function gets a parameter by reference, but you later unconditionally clone it + --> tests/ui/fn_param_ref_cloned.rs:62:18 + | +LL | is_clone_ref.clone(); + | ^^^^^^^ + | +note: consider passing the reference by value instead + --> tests/ui/fn_param_ref_cloned.rs:60:50 + | +LL | pub fn test_gen_function(is_clone_ref: &T, is_clone_owned: T) { + | ^^ + +error: function gets a parameter by reference, but you later unconditionally clone it + --> tests/ui/fn_param_ref_cloned.rs:71:18 + | +LL | is_clone_ref.clone(); + | ^^^^^^^ + | +note: consider passing the reference by value instead + --> tests/ui/fn_param_ref_cloned.rs:69:74 + | +LL | pub fn basic_clone_function(is_not_clone_ref: &IsNotClone, is_clone_ref: &IsClone, is_clone_owned: IsClone) { + | ^^^^^^^^ + +error: function gets a parameter by reference, but you later rebind and unconditionally clone it + --> tests/ui/fn_param_ref_cloned.rs:92:13 + | +LL | rebound.clone(); + | ^^^^^^^ + | +note: you bind the parameter into a new binding here + --> tests/ui/fn_param_ref_cloned.rs:91:5 + | +LL | let rebound = is_clone_ref; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ +note: the parameter is passed by reference... + --> tests/ui/fn_param_ref_cloned.rs:69:74 + | +LL | pub fn basic_clone_function(is_not_clone_ref: &IsNotClone, is_clone_ref: &IsClone, is_clone_owned: IsClone) { + | ^^^^^^^^ + +error: function gets a parameter by reference, but you later unconditionally clone it + --> tests/ui/fn_param_ref_cloned.rs:121:44 + | +LL | let should_not_allow_clone = clone_ref.clone(); + | ^^^^^^^ + | +note: consider passing the reference by value instead + --> tests/ui/fn_param_ref_cloned.rs:116:35 + | +LL | fn dont_check_if_stmts(clone_ref: &IsClone, if_arg: usize) { + | ^^^^^^^^ + +error: aborting due to 6 previous errors +