From 2ebe7516a898ddd44c9021b080cbaf079e32121c Mon Sep 17 00:00:00 2001 From: Jaeyong Sung Date: Sun, 13 Feb 2022 13:32:40 +0900 Subject: [PATCH] add documentation --- clippy_lints/src/only_used_in_recursion.rs | 43 ++++++++++++++++++++-- tests/ui/only_used_in_recursion.stderr | 22 +++++------ 2 files changed, 51 insertions(+), 14 deletions(-) diff --git a/clippy_lints/src/only_used_in_recursion.rs b/clippy_lints/src/only_used_in_recursion.rs index b941a2647cc..b8e0bd8acb3 100644 --- a/clippy_lints/src/only_used_in_recursion.rs +++ b/clippy_lints/src/only_used_in_recursion.rs @@ -21,7 +21,7 @@ declare_clippy_lint! { /// ### What it does - /// Checks for arguments that is only used in recursion with no side-effects. + /// Checks for arguments that are 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. @@ -30,7 +30,21 @@ /// The could contain a useless calculation and can make function simpler. /// /// ### Known problems - /// It could not catch the variable that has no side effects but only used in recursion. + /// In some cases, this would not catch all useless arguments. + /// + /// ```rust + /// fn foo(a: usize, b: usize) -> usize { + /// let f = |x| x + 1; + /// + /// if a == 0 { + /// 1 + /// } else { + /// foo(a - 1, f(b)) + /// } + /// } + /// ``` + /// + /// For example, the argument `b` is only used in recursion, but the lint would not catch it. /// /// ### Example /// ```rust @@ -111,10 +125,12 @@ fn check_fn( visitor.visit_expr(&body.value); let vars = std::mem::take(&mut visitor.ret_vars); + // this would set the return variables to side effect visitor.add_side_effect(vars); let mut queue = visitor.has_side_effect.iter().copied().collect::>(); + // a simple BFS to check all the variables that have side effect while let Some(id) = queue.pop_front() { if let Some(next) = visitor.graph.get(&id) { for i in next { @@ -134,6 +150,8 @@ fn check_fn( queue.push_back(id); + // a simple BFS to check the graph can reach to itself + // if it can't, it means the variable is never used in recursion while let Some(id) = queue.pop_front() { if let Some(next) = visitor.graph.get(&id) { for i in next { @@ -150,7 +168,7 @@ fn check_fn( cx, ONLY_USED_IN_RECURSION, span, - "parameter is only used in recursion with no side-effects", + "parameter is only used in recursion", "if this is intentional, prefix with an underscore", format!("_{}", ident.name.as_str()), Applicability::MaybeIncorrect, @@ -178,6 +196,21 @@ pub fn is_array(ty: Ty<'_>) -> bool { } } +/// This builds the graph of side effect. +/// The edge `a -> b` means if `a` has side effect, `b` will have side effect. +/// +/// There are some exmaple in following code: +/// ```rust, ignore +/// let b = 1; +/// let a = b; // a -> b +/// let (c, d) = (a, b); // c -> b, d -> b +/// +/// let e = if a == 0 { // e -> a +/// c // e -> c +/// } else { +/// d // e -> d +/// }; +/// ``` pub struct SideEffectVisit<'tcx> { graph: FxHashMap>, has_side_effect: FxHashSet, @@ -241,6 +274,7 @@ fn visit_expr(&mut self, ex: &'tcx Expr<'tcx>) { self.visit_if(bind, then_expr, else_expr); }, ExprKind::Match(expr, arms, _) => self.visit_match(expr, arms), + // since analysing the closure is not easy, just set all variables in it to side-effect ExprKind::Closure(_, _, body_id, _, _) => { let body = self.ty_ctx.hir().body(body_id); self.visit_body(body); @@ -359,6 +393,9 @@ fn visit_bin_op(&mut self, lhs: &'tcx Expr<'tcx>, rhs: &'tcx Expr<'tcx>) { let mut ret_vars = std::mem::take(&mut self.ret_vars); self.visit_expr(rhs); self.ret_vars.append(&mut ret_vars); + + // the binary operation between non primitive values are overloaded operators + // so they can have side-effects 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); diff --git a/tests/ui/only_used_in_recursion.stderr b/tests/ui/only_used_in_recursion.stderr index 29d2b403427..ed942e06f26 100644 --- a/tests/ui/only_used_in_recursion.stderr +++ b/tests/ui/only_used_in_recursion.stderr @@ -1,4 +1,4 @@ -error: parameter is only used in recursion with no side-effects +error: parameter is only used in recursion --> $DIR/only_used_in_recursion.rs:3:21 | LL | fn simple(a: usize, b: usize) -> usize { @@ -6,61 +6,61 @@ LL | fn simple(a: usize, b: usize) -> usize { | = note: `-D clippy::only-used-in-recursion` implied by `-D warnings` -error: parameter is only used in recursion with no side-effects +error: parameter is only used in recursion --> $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 +error: parameter is only used in recursion --> $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 +error: parameter is only used in recursion --> $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 +error: parameter is only used in recursion --> $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 +error: parameter is only used in recursion --> $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 +error: parameter is only used in recursion --> $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 +error: parameter is only used in recursion --> $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 +error: parameter is only used in recursion --> $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 +error: parameter is only used in recursion --> $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 +error: parameter is only used in recursion --> $DIR/only_used_in_recursion.rs:64:32 | LL | fn method(&self, a: usize, b: usize) -> usize {