diff options
| -rw-r--r-- | CHANGELOG.md | 1 | ||||
| -rw-r--r-- | clippy_lints/src/lib.register_all.rs | 1 | ||||
| -rw-r--r-- | clippy_lints/src/lib.register_complexity.rs | 1 | ||||
| -rw-r--r-- | clippy_lints/src/lib.register_lints.rs | 1 | ||||
| -rw-r--r-- | clippy_lints/src/lib.rs | 2 | ||||
| -rw-r--r-- | clippy_lints/src/methods/option_map_or_none.rs | 9 | ||||
| -rw-r--r-- | clippy_lints/src/only_used_in_recursion.rs | 563 | ||||
| -rw-r--r-- | clippy_lints/src/unnecessary_sort_by.rs | 45 | ||||
| -rw-r--r-- | tests/ui/only_used_in_recursion.rs | 73 | ||||
| -rw-r--r-- | tests/ui/only_used_in_recursion.stderr | 70 |
10 files changed, 732 insertions, 34 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index c9adf77c0d6..46f002aeeb3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3349,6 +3349,7 @@ Released 2018-09-13 [`not_unsafe_ptr_arg_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#not_unsafe_ptr_arg_deref [`octal_escapes`]: https://rust-lang.github.io/rust-clippy/master/index.html#octal_escapes [`ok_expect`]: https://rust-lang.github.io/rust-clippy/master/index.html#ok_expect +[`only_used_in_recursion`]: https://rust-lang.github.io/rust-clippy/master/index.html#only_used_in_recursion [`op_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#op_ref [`option_as_ref_deref`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_as_ref_deref [`option_env_unwrap`]: https://rust-lang.github.io/rust-clippy/master/index.html#option_env_unwrap diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index d93e34e76b4..7410d965ec1 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -226,6 +226,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(non_expressive_names::JUST_UNDERSCORES_AND_DIGITS), LintId::of(non_octal_unix_permissions::NON_OCTAL_UNIX_PERMISSIONS), LintId::of(octal_escapes::OCTAL_ESCAPES), + LintId::of(only_used_in_recursion::ONLY_USED_IN_RECURSION), LintId::of(open_options::NONSENSICAL_OPEN_OPTIONS), LintId::of(option_env_unwrap::OPTION_ENV_UNWRAP), LintId::of(overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL), diff --git a/clippy_lints/src/lib.register_complexity.rs b/clippy_lints/src/lib.register_complexity.rs index bd5ff613447..0cdf0cd1917 100644 --- a/clippy_lints/src/lib.register_complexity.rs +++ b/clippy_lints/src/lib.register_complexity.rs @@ -63,6 +63,7 @@ store.register_group(true, "clippy::complexity", Some("clippy_complexity"), vec! LintId::of(neg_cmp_op_on_partial_ord::NEG_CMP_OP_ON_PARTIAL_ORD), LintId::of(no_effect::NO_EFFECT), LintId::of(no_effect::UNNECESSARY_OPERATION), + LintId::of(only_used_in_recursion::ONLY_USED_IN_RECURSION), LintId::of(overflow_check_conditional::OVERFLOW_CHECK_CONDITIONAL), LintId::of(partialeq_ne_impl::PARTIALEQ_NE_IMPL), LintId::of(precedence::PRECEDENCE), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index a80320a578f..431bd622702 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -388,6 +388,7 @@ store.register_lints(&[ non_send_fields_in_send_ty::NON_SEND_FIELDS_IN_SEND_TY, nonstandard_macro_braces::NONSTANDARD_MACRO_BRACES, octal_escapes::OCTAL_ESCAPES, + only_used_in_recursion::ONLY_USED_IN_RECURSION, open_options::NONSENSICAL_OPEN_OPTIONS, option_env_unwrap::OPTION_ENV_UNWRAP, option_if_let_else::OPTION_IF_LET_ELSE, diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 0b07726519e..bd49ad0652c 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -316,6 +316,7 @@ mod non_octal_unix_permissions; mod non_send_fields_in_send_ty; mod nonstandard_macro_braces; mod octal_escapes; +mod only_used_in_recursion; mod open_options; mod option_env_unwrap; mod option_if_let_else; @@ -862,6 +863,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(move || Box::new(borrow_as_ptr::BorrowAsPtr::new(msrv))); store.register_late_pass(move || Box::new(manual_bits::ManualBits::new(msrv))); store.register_late_pass(|| Box::new(default_union_representation::DefaultUnionRepresentation)); + store.register_late_pass(|| Box::new(only_used_in_recursion::OnlyUsedInRecursion)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/methods/option_map_or_none.rs b/clippy_lints/src/methods/option_map_or_none.rs index bdf8cea1207..2a5ab6e625c 100644 --- a/clippy_lints/src/methods/option_map_or_none.rs +++ b/clippy_lints/src/methods/option_map_or_none.rs @@ -14,10 +14,7 @@ use super::RESULT_MAP_OR_INTO_OPTION; // The expression inside a closure may or may not have surrounding braces // which causes problems when generating a suggestion. -fn reduce_unit_expression<'a>( - cx: &LateContext<'_>, - expr: &'a hir::Expr<'_>, -) -> Option<(&'a hir::Expr<'a>, &'a [hir::Expr<'a>])> { +fn reduce_unit_expression<'a>(expr: &'a hir::Expr<'_>) -> Option<(&'a hir::Expr<'a>, &'a [hir::Expr<'a>])> { match expr.kind { hir::ExprKind::Call(func, arg_char) => Some((func, arg_char)), hir::ExprKind::Block(block, _) => { @@ -25,7 +22,7 @@ fn reduce_unit_expression<'a>( (&[], Some(inner_expr)) => { // If block only contains an expression, // reduce `|x| { x + 1 }` to `|x| x + 1` - reduce_unit_expression(cx, inner_expr) + reduce_unit_expression(inner_expr) }, _ => None, } @@ -77,7 +74,7 @@ pub(super) fn check<'tcx>( if let hir::ExprKind::Closure(_, _, id, span, _) = map_arg.kind; let arg_snippet = snippet(cx, span, ".."); let body = cx.tcx.hir().body(id); - if let Some((func, [arg_char])) = reduce_unit_expression(cx, &body.value); + if let Some((func, [arg_char])) = reduce_unit_expression(&body.value); if let Some(id) = path_def_id(cx, func).and_then(|ctor_id| cx.tcx.parent(ctor_id)); if Some(id) == cx.tcx.lang_items().option_some_variant(); then { diff --git a/clippy_lints/src/only_used_in_recursion.rs b/clippy_lints/src/only_used_in_recursion.rs new file mode 100644 index 00000000000..ace00333bd1 --- /dev/null +++ b/clippy_lints/src/only_used_in_recursion.rs @@ -0,0 +1,563 @@ +use std::collections::VecDeque; + +use clippy_utils::diagnostics::span_lint_and_sugg; +use itertools::{izip, Itertools}; +use rustc_ast::{walk_list, Label, Mutability}; +use rustc_data_structures::fx::{FxHashMap, FxHashSet}; +use rustc_errors::Applicability; +use rustc_hir::def::Res; +use rustc_hir::intravisit::{walk_expr, FnKind, Visitor}; +use rustc_hir::{ + Arm, Block, Body, Expr, ExprKind, Guard, HirId, Let, Local, Pat, PatKind, Path, PathSegment, QPath, Stmt, StmtKind, + UnOp, +}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_middle::ty; +use rustc_middle::ty::{Ty, TyCtxt, TypeckResults}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::symbol::kw; +use rustc_span::symbol::Ident; +use rustc_span::Span; + +declare_clippy_lint! { + /// ### What it does + /// Checks for arguments that is only used in recursion with no side-effects. + /// The arguments can be involved in calculations and assignments but as long as + /// the calculations have no side-effects (function calls or mutating dereference) + /// and the assigned variables are also only in recursion, it is useless. + /// + /// ### Why is this bad? + /// The could contain a useless calculation and can make function simpler. + /// + /// ### Known Issues + /// It could not catch the variable that has no side effects but only used in recursion. + /// + /// ### Example + /// ```rust + /// fn f(a: usize, b: usize) -> usize { + /// if a == 0 { + /// 1 + /// } else { + /// f(a - 1, b + 1) + /// } + /// } + /// # fn main() { + /// # print!("{}", f(1, 1)); + /// # } + /// ``` + /// Use instead: + /// ```rust + /// fn f(a: usize) -> usize { + /// if a == 0 { + /// 1 + /// } else { + /// f(a - 1) + /// } + /// } + /// # fn main() { + /// # print!("{}", f(1)); + /// # } + /// ``` + #[clippy::version = "1.60.0"] + pub ONLY_USED_IN_RECURSION, + complexity, + "default lint description" +} +declare_lint_pass!(OnlyUsedInRecursion => [ONLY_USED_IN_RECURSION]); + +impl<'tcx> LateLintPass<'tcx> for OnlyUsedInRecursion { + fn check_fn( + &mut self, + cx: &LateContext<'tcx>, + kind: FnKind<'tcx>, + _: &'tcx rustc_hir::FnDecl<'tcx>, + body: &'tcx Body<'tcx>, + _: Span, + _: HirId, + ) { + if let FnKind::ItemFn(ident, ..) | FnKind::Method(ident, ..) = kind { + let ty_res = cx.typeck_results(); + let param_span = body + .params + .iter() + .flat_map(|param| { + let mut v = Vec::new(); + param.pat.each_binding(|_, hir_id, span, ident| { + v.push((hir_id, span, ident)); + }); + v + }) + .skip(match kind { + FnKind::Method(..) => 1, + _ => 0, + }) + .filter(|(_, _, ident)| !ident.name.as_str().starts_with('_')) + .collect_vec(); + + let params = body.params.iter().map(|param| param.pat).collect(); + + let mut visitor = SideEffectVisit { + graph: FxHashMap::default(), + has_side_effect: FxHashSet::default(), + ret_vars: Vec::new(), + contains_side_effect: false, + break_vars: FxHashMap::default(), + params, + fn_ident: ident, + is_method: matches!(kind, FnKind::Method(..)), + ty_res, + ty_ctx: cx.tcx, + }; + + visitor.visit_expr(&body.value); + let vars = std::mem::take(&mut visitor.ret_vars); + visitor.add_side_effect(vars); + + let mut queue = visitor.has_side_effect.iter().copied().collect::<VecDeque<_>>(); + + while let Some(id) = queue.pop_front() { + if let Some(next) = visitor.graph.get(&id) { + for i in next { + if !visitor.has_side_effect.contains(i) { + visitor.has_side_effect.insert(*i); + queue.push_back(*i); + } + } + } + } + + for (id, span, ident) in param_span { + if !visitor.has_side_effect.contains(&id) { + span_lint_and_sugg( + cx, + ONLY_USED_IN_RECURSION, + span, + "parameter is only used in recursion with no side-effects", + "if this is intentional, prefix with an underscore", + format!("_{}", ident.name.as_str()), + Applicability::MaybeIncorrect, + ); + } + } + } + } +} + +pub fn is_primitive(ty: Ty<'_>) -> bool { + match ty.kind() { + ty::Bool | ty::Char | ty::Int(_) | ty::Uint(_) | ty::Float(_) | ty::Str => true, + ty::Ref(_, t, _) => is_primitive(t), + _ => false, + } +} + +pub fn is_array(ty: Ty<'_>) -> bool { + match ty.kind() { + ty::Array(..) | ty::Slice(..) => true, + ty::Ref(_, t, _) => is_array(t), + _ => false, + } +} + +pub struct SideEffectVisit<'tcx> { + graph: FxHashMap<HirId, FxHashSet<HirId>>, + has_side_effect: FxHashSet<HirId>, + // bool for if the variable was dereferenced from mutable reference + ret_vars: Vec<(HirId, bool)>, + contains_side_effect: bool, + // break label + break_vars: FxHashMap<Ident, Vec<(HirId, bool)>>, + params: Vec<&'tcx Pat<'tcx>>, + fn_ident: Ident, + is_method: bool, + ty_res: &'tcx TypeckResults<'tcx>, + ty_ctx: TyCtxt<'tcx>, +} + +impl<'tcx> Visitor<'tcx> for SideEffectVisit<'tcx> { + fn visit_block(&mut self, b: &'tcx Block<'tcx>) { + b.stmts.iter().for_each(|stmt| { + self.visit_stmt(stmt); + self.ret_vars.clear(); + }); + walk_list!(self, visit_expr, b.expr); + } + + fn visit_stmt(&mut self, s: &'tcx Stmt<'tcx>) { + match s.kind { + StmtKind::Local(Local { + pat, init: Some(init), .. + }) => { + self.visit_pat_expr(pat, init); + }, + StmtKind::Item(i) => { + let item = self.ty_ctx.hir().item(i); + self.visit_item(item); + }, + StmtKind::Expr(e) | StmtKind::Semi(e) => self.visit_expr(e), + StmtKind::Local(_) => {}, + } + } + + fn visit_expr(&mut self, ex: &'tcx Expr<'tcx>) { + debug_assert!(self.ret_vars.is_empty()); + match ex.kind { + ExprKind::Array(exprs) | ExprKind::Tup(exprs) => { + self.ret_vars = exprs + .iter() + .flat_map(|expr| { + self.visit_expr(expr); + std::mem::take(&mut self.ret_vars) + }) + .collect(); + }, + ExprKind::Call(callee, args) => self.visit_fn(callee, args), + ExprKind::MethodCall(path, args, _) => self.visit_method_call(path, args), + ExprKind::Binary(_, lhs, rhs) => { + self.visit_bin_op(lhs, rhs); + }, + ExprKind::Unary(op, expr) => self.visit_un_op(op, expr), + ExprKind::Let(Let { pat, init, .. }) => self.visit_pat_expr(pat, init), + ExprKind::If(bind, then_expr, else_expr) => { + self.visit_if(bind, then_expr, else_expr); + }, + ExprKind::Match(expr, arms, _) => self.visit_match(expr, arms), + ExprKind::Closure(_, _, body_id, _, _) => { + let body = self.ty_ctx.hir().body(body_id); + self.visit_body(body); + let vars = std::mem::take(&mut self.ret_vars); + self.add_side_effect(vars); + }, + ExprKind::Loop(block, label, _, _) | ExprKind::Block(block, label) => { + self.visit_block_label(block, label); + }, + ExprKind::Assign(bind, expr, _) => { + self.visit_assign(bind, expr); + }, + ExprKind::AssignOp(_, bind, expr) => { + self.visit_assign(bind, expr); + self.visit_bin_op(bind, expr); + }, + ExprKind::Field(expr, _) => { + self.visit_expr(expr); + if matches!(self.ty_res.expr_ty(expr).kind(), ty::Ref(_, _, Mutability::Mut)) { + self.ret_vars.iter_mut().for_each(|(_, b)| *b = true); + } + }, + ExprKind::Index(expr, index) => { + self.visit_expr(expr); + let mut vars = std::mem::take(&mut self.ret_vars); + self.visit_expr(index); + self.ret_vars.append(&mut vars); + + if !is_array(self.ty_res.expr_ty(expr)) { + self.add_side_effect(self.ret_vars.clone()); + } else if matches!(self.ty_res.expr_ty(expr).kind(), ty::Ref(_, _, Mutability::Mut)) { + self.ret_vars.iter_mut().for_each(|(_, b)| *b = true); + } + }, + ExprKind::Break(dest, Some(expr)) => { + self.visit_expr(expr); + if let Some(label) = dest.label { + self.break_vars + .entry(label.ident) + .or_insert(Vec::new()) + .append(&mut self.ret_vars); + } + self.contains_side_effect = true; + }, + ExprKind::Ret(Some(expr)) => { + self.visit_expr(expr); + let vars = std::mem::take(&mut self.ret_vars); + self.add_side_effect(vars); + self.contains_side_effect = true; + }, + ExprKind::Break(_, None) | ExprKind::Continue(_) | ExprKind::Ret(None) => { + self.contains_side_effect = true; + }, + ExprKind::Struct(_, exprs, expr) => { + let mut ret_vars = exprs + .iter() + .flat_map(|field| { + self.visit_expr(field.expr); + std::mem::take(&mut self.ret_vars) + }) + .collect(); + + walk_list!(self, visit_expr, expr); + self.ret_vars.append(&mut ret_vars); + }, + _ => walk_expr(self, ex), + } + } + + fn visit_path(&mut self, path: &'tcx Path<'tcx>, _id: HirId) { + if let Res::Local(id) = path.res { + self.ret_vars.push((id, false)); + } + } +} + +impl<'tcx> SideEffectVisit<'tcx> { + fn visit_assign(&mut self, lhs: &'tcx Expr<'tcx>, rhs: &'tcx Expr<'tcx>) { + // Just support array and tuple unwrapping for now. + // + // ex) `(a, b) = (c, d);` + // The graph would look like this: + // a -> c + // b -> d + // + // This would minimize the connection of the side-effect graph. + match (&lhs.kind, &rhs.kind) { + (ExprKind::Array(lhs), ExprKind::Array(rhs)) | (ExprKind::Tup(lhs), ExprKind::Tup(rhs)) => { + // if not, it is a compile error + debug_assert!(lhs.len() == rhs.len()); + izip!(*lhs, *rhs).for_each(|(lhs, rhs)| self.visit_assign(lhs, rhs)); + }, + // in other assigns, we have to connect all each other + // because they can be connected somehow + _ => { + self.visit_expr(lhs); + let lhs_vars = std::mem::take(&mut self.ret_vars); + self.visit_expr(rhs); + let rhs_vars = std::mem::take(&mut self.ret_vars); + self.connect_assign(&lhs_vars, &rhs_vars); + }, + } + } + + fn visit_block_label(&mut self, block: &'tcx Block<'tcx>, label: Option<Label>) { + self.visit_block(block); + let _ = label.and_then(|label| { + self.break_vars + .remove(&label.ident) + .map(|mut break_vars| self.ret_vars.append(&mut break_vars)) + }); + } + + fn visit_bin_op(&mut self, lhs: &'tcx Expr<'tcx>, rhs: &'tcx Expr<'tcx>) { + self.visit_expr(lhs); + let mut ret_vars = std::mem::take(&mut self.ret_vars); + self.visit_expr(rhs); + self.ret_vars.append(&mut ret_vars); + if !is_primitive(self.ty_res.expr_ty(lhs)) || !is_primitive(self.ty_res.expr_ty(rhs)) { + self.ret_vars.iter().for_each(|id| { + self.has_side_effect.insert(id.0); + }); + self.contains_side_effect = true; + } + } + + fn visit_un_op(&mut self, op: UnOp, expr: &'tcx Expr<'tcx>) { + self.visit_expr(expr); + let ty = self.ty_res.expr_ty(expr); + // dereferencing a reference has no side-effect + if !is_primitive(ty) && !matches!((op, ty.kind()), (UnOp::Deref, ty::Ref(..))) { + self.add_side_effect(self.ret_vars.clone()); + } + + if matches!((op, ty.kind()), (UnOp::Deref, ty::Ref(_, _, Mutability::Mut))) { + self.ret_vars.iter_mut().for_each(|(_, b)| *b = true); + } + } + + fn visit_pat_expr(&mut self, pat: &'tcx Pat<'tcx>, expr: &'tcx Expr<'tcx>) { + match (&pat.kind, &expr.kind) { + (PatKind::Tuple(pats, _), ExprKind::Tup(exprs)) => { + self.ret_vars = izip!(*pats, *exprs) + .flat_map(|(pat, expr)| { + self.visit_pat_expr(pat, expr); + std::mem::take(&mut self.ret_vars) + }) + .collect(); + }, + (PatKind::Slice(front_exprs, _, back_exprs), ExprKind::Array(exprs)) => { + let mut vars = izip!(*front_exprs, *exprs) + .flat_map(|(pat, expr)| { + self.visit_pat_expr(pat, expr); + std::mem::take(&mut self.ret_vars) + }) + .collect(); + self.ret_vars = izip!(back_exprs.iter().rev(), exprs.iter().rev()) + .flat_map(|(pat, expr)| { + self.visit_pat_expr(pat, expr); + std::mem::take(&mut self.ret_vars) + }) + .collect(); + self.ret_vars.append(&mut vars); + }, + _ => { + let mut lhs_vars = Vec::new(); + pat.each_binding(|_, id, _, _| lhs_vars.push((id, false))); + self.visit_expr(expr); + let rhs_vars = std::mem::take(&mut self.ret_vars); + self.connect_assign(&lhs_vars, &rhs_vars); + self.ret_vars = rhs_vars; + }, + } + } + + fn visit_fn(&mut self, callee: &'tcx Expr<'tcx>, args: &'tcx [Expr<'tcx>]) { + self.visit_expr(callee); + let mut ret_vars = std::mem::take(&mut self.ret_vars); + self.add_side_effect(ret_vars.clone()); + + if_chain! { + if !self.is_method; + if let ExprKind::Path(QPath::Resolved(_, path)) = callee.kind; + if let Res::Def(..) = path.res; + if path.segments.len() == 1; + let ident = path.segments.last().unwrap().ident; + if ident == self.fn_ident; + then { + izip!(self.params.clone(), args) + .for_each(|(pat, expr)| { + self.visit_pat_expr(pat, expr); + self.ret_vars.clear(); + }); + } else { + // This would set arguments used in closure that does not have side-effect. + // Closure itself can be detected whether there is a side-effect, but the + // value of variable that is holding closure can change. + // So, we just check the variables. + self.ret_vars = args + .iter() + .flat_map(|expr| { + self.visit_expr(expr); + std::mem::take(&mut self.ret_vars) + }) + .collect_vec() + .into_iter() + .map(|id| { + self.has_side_effect.insert(id.0); + id + }) + .collect(); + self.contains_side_effect = true; + } + } + + self.ret_vars.append(&mut ret_vars); + } + + fn visit_method_call(&mut self, path: &'tcx PathSegment<'tcx>, args: &'tcx [Expr<'tcx>]) { + if_chain! { + if self.is_method; + if path.ident == self.fn_ident; + if let ExprKind::Path(QPath::Resolved(_, path)) = args.first().unwrap().kind; + if let Res::Local(..) = path.res; + let ident = path.segments.last().unwrap().ident; + if ident.name == kw::SelfLower; + then { + izip!(self.params.clone(), args.iter()) + .for_each(|(pat, expr)| { + self.visit_pat_expr(pat, expr); + self.ret_vars.clear(); + }); + } else { + self.ret_vars = args + .iter() + .flat_map(|expr| { + self.visit_expr(expr); + std::mem::take(&mut self.ret_vars) + }) + .collect_vec() + .into_iter() + .map(|a| { + self.has_side_effect.insert(a.0); + a + }) + .collect(); + self.contains_side_effect = true; + } + } + } + + fn visit_if(&mut self, bind: &'tcx Expr<'tcx>, then_expr: &'tcx Expr<'tcx>, else_expr: Option<&'tcx Expr<'tcx>>) { + let contains_side_effect = self.contains_side_effect; + self.contains_side_effect = false; + self.visit_expr(bind); + let mut vars = std::mem::take(&mut self.ret_vars); + self.visit_expr(then_expr); + let mut then_vars = std::mem::take(&mut self.ret_vars); + walk_list!(self, visit_expr, else_expr); + if self.contains_side_effect { + self.add_side_effect(vars.clone()); + } + self.contains_side_effect |= contains_side_effect; + self.ret_vars.append(&mut vars); + self.ret_vars.append(&mut then_vars); + } + + fn visit_match(&mut self, expr: &'tcx Expr<'tcx>, arms: &'tcx [Arm<'tcx>]) { + self.visit_expr(expr); + let mut expr_vars = std::mem::take(&mut self.ret_vars); + self.ret_vars = arms + .iter() + .flat_map(|arm| { + let contains_side_effect = self.contains_side_effect; + self.contains_side_effect = false; + // this would visit `expr` multiple times + // but couldn't think of a better way + self.visit_pat_expr(arm.pat, expr); + let mut vars = std::mem::take(&mut self.ret_vars); + let _ = arm.guard.as_ref().map(|guard| { + self.visit_expr(match guard { + Guard::If(expr) | Guard::IfLet(_, expr) => expr, + }); + vars.append(&mut self.ret_vars); + }); + self.visit_expr(arm.body); + if self.contains_side_effect { + self.add_side_effect(vars.clone()); + self.add_side_effect(expr_vars.clone()); + } + self.contains_side_effect |= contains_side_effect; + vars.append(&mut self.ret_vars); + vars + }) + .collect(); + self.ret_vars.append(&mut expr_vars); + } + + fn connect_assign(&mut self, lhs: &[(HirId, bool)], rhs: &[(HirId, bool)]) { + // if mutable dereference is on assignment it can have side-effect + // (this can lead to parameter mutable dereference and change the original value) + // too hard to detect whether this value is from parameter, so this would all + // check mutable dereference assignment to side effect + lhs.iter().filter(|(_, b)| *b).for_each(|(id, _)| { + self.has_side_effect.insert(*id); + self.contains_side_effect = true; + }); + + // there is no connection + if lhs.is_empty() || rhs.is_empty() { + return; + } + + // by connected rhs in cycle, the connections would decrease + // from `n * m` to `n + m` + // where `n` and `m` are length of `lhs` and `rhs`. + + // unwrap is possible since rhs is not empty + let rhs_first = rhs.first().unwrap(); + for (id, _) in lhs.iter() { + self.graph + .entry(*id) + .or_insert_with(FxHashSet::default) + .insert(rhs_first.0); + } + + let rhs = rhs.iter(); + izip!(rhs.clone().cycle().skip(1), rhs).for_each(|(from, to)| { + self.graph.entry(from.0).or_insert_with(FxHashSet::default).insert(to.0); + }); + } + + fn add_side_effect(&mut self, v: Vec<(HirId, bool)>) { + for (id, _) in v { + self.has_side_effect.insert(id); + self.contains_side_effect = true; + } + } +} diff --git a/clippy_lints/src/unnecessary_sort_by.rs b/clippy_lints/src/unnecessary_sort_by.rs index e6c260ed96a..fbc601becac 100644 --- a/clippy_lints/src/unnecessary_sort_by.rs +++ b/clippy_lints/src/unnecessary_sort_by.rs @@ -67,59 +67,51 @@ struct SortByKeyDetection { /// Detect if the two expressions are mirrored (identical, except one /// contains a and the other replaces it with b) -fn mirrored_exprs( - cx: &LateContext<'_>, - a_expr: &Expr<'_>, - a_ident: &Ident, - b_expr: &Expr<'_>, - b_ident: &Ident, -) -> bool { +fn mirrored_exprs(a_expr: &Expr<'_>, a_ident: &Ident, b_expr: &Expr<'_>, b_ident: &Ident) -> bool { match (&a_expr.kind, &b_expr.kind) { // Two boxes with mirrored contents (ExprKind::Box(left_expr), ExprKind::Box(right_expr)) => { - mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident) + mirrored_exprs(left_expr, a_ident, right_expr, b_ident) }, // Two arrays with mirrored contents (ExprKind::Array(left_exprs), ExprKind::Array(right_exprs)) => { - iter::zip(*left_exprs, *right_exprs).all(|(left, right)| mirrored_exprs(cx, left, a_ident, right, b_ident)) + iter::zip(*left_exprs, *right_exprs).all(|(left, right)| mirrored_exprs(left, a_ident, right, b_ident)) }, // The two exprs are function calls. // Check to see that the function itself and its arguments are mirrored (ExprKind::Call(left_expr, left_args), ExprKind::Call(right_expr, right_args)) => { - mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident) - && iter::zip(*left_args, *right_args) - .all(|(left, right)| mirrored_exprs(cx, left, a_ident, right, b_ident)) + mirrored_exprs(left_expr, a_ident, right_expr, b_ident) + && iter::zip(*left_args, *right_args).all(|(left, right)| mirrored_exprs(left, a_ident, right, b_ident)) }, // The two exprs are method calls. // Check to see that the function is the same and the arguments are mirrored // This is enough because the receiver of the method is listed in the arguments (ExprKind::MethodCall(left_segment, left_args, _), ExprKind::MethodCall(right_segment, right_args, _)) => { left_segment.ident == right_segment.ident - && iter::zip(*left_args, *right_args) - .all(|(left, right)| mirrored_exprs(cx, left, a_ident, right, b_ident)) + && iter::zip(*left_args, *right_args).all(|(left, right)| mirrored_exprs(left, a_ident, right, b_ident)) }, // Two tuples with mirrored contents (ExprKind::Tup(left_exprs), ExprKind::Tup(right_exprs)) => { - iter::zip(*left_exprs, *right_exprs).all(|(left, right)| mirrored_exprs(cx, left, a_ident, right, b_ident)) + iter::zip(*left_exprs, *right_exprs).all(|(left, right)| mirrored_exprs(left, a_ident, right, b_ident)) }, // Two binary ops, which are the same operation and which have mirrored arguments (ExprKind::Binary(left_op, left_left, left_right), ExprKind::Binary(right_op, right_left, right_right)) => { left_op.node == right_op.node - && mirrored_exprs(cx, left_left, a_ident, right_left, b_ident) - && mirrored_exprs(cx, left_right, a_ident, right_right, b_ident) + && mirrored_exprs(left_left, a_ident, right_left, b_ident) + && mirrored_exprs(left_right, a_ident, right_right, b_ident) }, // Two unary ops, which are the same operation and which have the same argument (ExprKind::Unary(left_op, left_expr), ExprKind::Unary(right_op, right_expr)) => { - left_op == right_op && mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident) + left_op == right_op && mirrored_exprs(left_expr, a_ident, right_expr, b_ident) }, // The two exprs are literals of some kind (ExprKind::Lit(left_lit), ExprKind::Lit(right_lit)) => left_lit.node == right_lit.node, - (ExprKind::Cast(left, _), ExprKind::Cast(right, _)) => mirrored_exprs(cx, left, a_ident, right, b_ident), + (ExprKind::Cast(left, _), ExprKind::Cast(right, _)) => mirrored_exprs(left, a_ident, right, b_ident), (ExprKind::DropTemps(left_block), ExprKind::DropTemps(right_block)) => { - mirrored_exprs(cx, left_block, a_ident, right_block, b_ident) + mirrored_exprs(left_block, a_ident, right_block, b_ident) }, (ExprKind::Field(left_expr, left_ident), ExprKind::Field(right_expr, right_ident)) => { - left_ident.name == right_ident.name && mirrored_exprs(cx, left_expr, a_ident, right_expr, right_ident) + left_ident.name == right_ident.name && mirrored_exprs(left_expr, a_ident, right_expr, right_ident) }, // Two paths: either one is a and the other is b, or they're identical to each other ( @@ -151,11 +143,9 @@ fn mirrored_exprs( ( ExprKind::AddrOf(left_kind, Mutability::Not, left_expr), ExprKind::AddrOf(right_kind, Mutability::Not, right_expr), - ) => left_kind == right_kind && mirrored_exprs(cx, left_expr, a_ident, right_expr, b_ident), - (_, ExprKind::AddrOf(_, Mutability::Not, right_expr)) => { - mirrored_exprs(cx, a_expr, a_ident, right_expr, b_ident) - }, - (ExprKind::AddrOf(_, Mutability::Not, left_expr), _) => mirrored_exprs(cx, left_expr, a_ident, b_expr, b_ident), + ) => left_kind == right_kind && mirrored_exprs(left_expr, a_ident, right_expr, b_ident), + (_, ExprKind::AddrOf(_, Mutability::Not, right_expr)) => mirrored_exprs(a_expr, a_ident, right_expr, b_ident), + (ExprKind::AddrOf(_, Mutability::Not, left_expr), _) => mirrored_exprs(left_expr, a_ident, b_expr, b_ident), _ => false, } } @@ -176,14 +166,13 @@ fn detect_lint(cx: &LateContext<'_>, expr: &Expr<'_>) -> Option<LintTrigger> { if method_path.ident.name == sym::cmp; then { let (closure_body, closure_arg, reverse) = if mirrored_exprs( - cx, left_expr, left_ident, right_expr, right_ident ) { (Sugg::hir(cx, left_expr, "..").to_string(), left_ident.name.to_string(), false) - } else if mirrored_exprs(cx, left_expr, right_ident, right_expr, left_ident) { + } else if mirrored_exprs(left_expr, right_ident, right_expr, left_ident) { (Sugg::hir(cx, left_expr, "..").to_string(), right_ident.name.to_string(), true) } else { return None; diff --git a/tests/ui/only_used_in_recursion.rs b/tests/ui/only_used_in_recursion.rs new file mode 100644 index 00000000000..a0c40eb0826 --- /dev/null +++ b/tests/ui/only_used_in_recursion.rs @@ -0,0 +1,73 @@ +#![warn(clippy::only_used_in_recursion)] + +fn simple(a: usize, b: usize) -> usize { + if a == 0 { 1 } else { simple(a - 1, b) } +} + +fn with_calc(a: usize, b: isize) -> usize { + if a == 0 { 1 } else { with_calc(a - 1, -b + 1) } +} + +fn tuple((a, b): (usize, usize)) -> usize { + if a == 0 { 1 } else { tuple((a - 1, b + 1)) } +} + +fn let_tuple(a: usize, b: usize) -> usize { + let (c, d) = (a, b); + if c == 0 { 1 } else { let_tuple(c - 1, d + 1) } +} + +fn array([a, b]: [usize; 2]) -> usize { + if a == 0 { 1 } else { array([a - 1, b + 1]) } +} + +fn index(a: usize, mut b: &[usize], c: usize) -> usize { + if a == 0 { 1 } else { index(a - 1, b, c + b[0]) } +} + +fn break_(a: usize, mut b: usize, mut c: usize) -> usize { + let c = loop { + b += 1; + c += 1; + if c == 10 { + break b; + } + }; + + if a == 0 { 1 } else { break_(a - 1, c, c) } +} + +// this has a side effect +fn mut_ref(a: usize, b: &mut usize) -> usize { + *b = 1; + if a == 0 { 1 } else { mut_ref(a - 1, b) } +} + +fn mut_ref2(a: usize, b: &mut usize) -> usize { + let mut c = *b; + if a == 0 { 1 } else { mut_ref2(a - 1, &mut c) } +} + +fn not_primitive(a: usize, b: String) -> usize { + if a == 0 { 1 } else { not_primitive(a - 1, b) } +} + +// this doesn't have a side effect, +// but `String` is not primitive. +fn not_primitive_op(a: usize, b: String, c: &str) -> usize { + if a == 1 { 1 } else { not_primitive_op(a, b + c, c) } +} + +struct A; + +impl A { + fn method(&self, a: usize, b: usize) -> usize { + if a == 0 { 1 } else { self.method(a - 1, b + 1) } + } +} + +fn ignore(a: usize, _: usize) -> usize { + if a == 1 { 1 } else { ignore(a - 1, 0) } +} + +fn main() {} diff --git a/tests/ui/only_used_in_recursion.stderr b/tests/ui/only_used_in_recursion.stderr new file mode 100644 index 00000000000..29d2b403427 --- /dev/null +++ b/tests/ui/only_used_in_recursion.stderr @@ -0,0 +1,70 @@ +error: parameter is only used in recursion with no side-effects + --> $DIR/only_used_in_recursion.rs:3:21 + | +LL | fn simple(a: usize, b: usize) -> usize { + | ^ help: if this is intentional, prefix with an underscore: `_b` + | + = note: `-D clippy::only-used-in-recursion` implied by `-D warnings` + +error: parameter is only used in recursion with no side-effects + --> $DIR/only_used_in_recursion.rs:7:24 + | +LL | fn with_calc(a: usize, b: isize) -> usize { + | ^ help: if this is intentional, prefix with an underscore: `_b` + +error: parameter is only used in recursion with no side-effects + --> $DIR/only_used_in_recursion.rs:11:14 + | +LL | fn tuple((a, b): (usize, usize)) -> usize { + | ^ help: if this is intentional, prefix with an underscore: `_b` + +error: parameter is only used in recursion with no side-effects + --> $DIR/only_used_in_recursion.rs:15:24 + | +LL | fn let_tuple(a: usize, b: usize) -> usize { + | ^ help: if this is intentional, prefix with an underscore: `_b` + +error: parameter is only used in recursion with no side-effects + --> $DIR/only_used_in_recursion.rs:20:14 + | +LL | fn array([a, b]: [usize; 2]) -> usize { + | ^ help: if this is intentional, prefix with an underscore: `_b` + +error: parameter is only used in recursion with no side-effects + --> $DIR/only_used_in_recursion.rs:24:20 + | +LL | fn index(a: usize, mut b: &[usize], c: usize) -> usize { + | ^^^^^ help: if this is intentional, prefix with an underscore: `_b` + +error: parameter is only used in recursion with no side-effects + --> $DIR/only_used_in_recursion.rs:24:37 + | +LL | fn index(a: usize, mut b: &[usize], c: usize) -> usize { + | ^ help: if this is intentional, prefix with an underscore: `_c` + +error: parameter is only used in recursion with no side-effects + --> $DIR/only_used_in_recursion.rs:28:21 + | +LL | fn break_(a: usize, mut b: usize, mut c: usize) -> usize { + | ^^^^^ help: if this is intentional, prefix with an underscore: `_b` + +error: parameter is only used in recursion with no side-effects + --> $DIR/only_used_in_recursion.rs:46:23 + | +LL | fn mut_ref2(a: usize, b: &mut usize) -> usize { + | ^ help: if this is intentional, prefix with an underscore: `_b` + +error: parameter is only used in recursion with no side-effects + --> $DIR/only_used_in_recursion.rs:51:28 + | +LL | fn not_primitive(a: usize, b: String) -> usize { + | ^ help: if this is intentional, prefix with an underscore: `_b` + +error: parameter is only used in recursion with no side-effects + --> $DIR/only_used_in_recursion.rs:64:32 + | +LL | fn method(&self, a: usize, b: usize) -> usize { + | ^ help: if this is intentional, prefix with an underscore: `_b` + +error: aborting due to 11 previous errors + |