Enhance LocalUsedVisitor to check closure bodies

camsteffen · camsteffen · commit 1d3042294503 · 2021-02-08T08:56:33.000-06:00
diff --git a/clippy_lints/src/collapsible_match.rs b/clippy_lints/src/collapsible_match.rs
@@ -60,7 +60,7 @@ impl<'tcx> LateLintPass<'tcx> for CollapsibleMatch {
     }
 }
 
-fn check_arm(arm: &Arm<'_>, wild_outer_arm: &Arm<'_>, cx: &LateContext<'_>) {
+fn check_arm<'tcx>(arm: &Arm<'tcx>, wild_outer_arm: &Arm<'tcx>, cx: &LateContext<'tcx>) {
     if_chain! {
         let expr = strip_singleton_blocks(arm.body);
         if let ExprKind::Match(expr_in, arms_inner, _) = expr.kind;
@@ -84,14 +84,13 @@ fn check_arm(arm: &Arm<'_>, wild_outer_arm: &Arm<'_>, cx: &LateContext<'_>) {
         // the "wild-like" branches must be equal
         if SpanlessEq::new(cx).eq_expr(wild_inner_arm.body, wild_outer_arm.body);
         // the binding must not be used in the if guard
+        let mut used_visitor = LocalUsedVisitor::new(cx, binding_id);
         if match arm.guard {
             None => true,
-            Some(Guard::If(expr) | Guard::IfLet(_, expr)) => {
-                !LocalUsedVisitor::new(binding_id).check_expr(expr)
-            }
+            Some(Guard::If(expr) | Guard::IfLet(_, expr)) => !used_visitor.check_expr(expr),
         };
         // ...or anywhere in the inner match
-        if !arms_inner.iter().any(|arm| LocalUsedVisitor::new(binding_id).check_arm(arm));
+        if !arms_inner.iter().any(|arm| used_visitor.check_arm(arm));
         then {
             span_lint_and_then(
                 cx,
diff --git a/clippy_lints/src/let_if_seq.rs b/clippy_lints/src/let_if_seq.rs
@@ -63,10 +63,11 @@ impl<'tcx> LateLintPass<'tcx> for LetIfSeq {
                 if let hir::PatKind::Binding(mode, canonical_id, ident, None) = local.pat.kind;
                 if let hir::StmtKind::Expr(ref if_) = expr.kind;
                 if let hir::ExprKind::If(ref cond, ref then, ref else_) = if_.kind;
-                if !LocalUsedVisitor::new(canonical_id).check_expr(cond);
+                let mut used_visitor = LocalUsedVisitor::new(cx, canonical_id);
+                if !used_visitor.check_expr(cond);
                 if let hir::ExprKind::Block(ref then, _) = then.kind;
-                if let Some(value) = check_assign(canonical_id, &*then);
-                if !LocalUsedVisitor::new(canonical_id).check_expr(value);
+                if let Some(value) = check_assign(cx, canonical_id, &*then);
+                if !used_visitor.check_expr(value);
                 then {
                     let span = stmt.span.to(if_.span);
 
@@ -78,7 +79,7 @@ impl<'tcx> LateLintPass<'tcx> for LetIfSeq {
 
                     let (default_multi_stmts, default) = if let Some(ref else_) = *else_ {
                         if let hir::ExprKind::Block(ref else_, _) = else_.kind {
-                            if let Some(default) = check_assign(canonical_id, else_) {
+                            if let Some(default) = check_assign(cx, canonical_id, else_) {
                                 (else_.stmts.len() > 1, default)
                             } else if let Some(ref default) = local.init {
                                 (true, &**default)
@@ -133,15 +134,19 @@ impl<'tcx> LateLintPass<'tcx> for LetIfSeq {
     }
 }
 
-fn check_assign<'tcx>(decl: hir::HirId, block: &'tcx hir::Block<'_>) -> Option<&'tcx hir::Expr<'tcx>> {
+fn check_assign<'tcx>(
+    cx: &LateContext<'tcx>,
+    decl: hir::HirId,
+    block: &'tcx hir::Block<'_>,
+) -> Option<&'tcx hir::Expr<'tcx>> {
     if_chain! {
         if block.expr.is_none();
         if let Some(expr) = block.stmts.iter().last();
         if let hir::StmtKind::Semi(ref expr) = expr.kind;
         if let hir::ExprKind::Assign(ref var, ref value, _) = expr.kind;
         if path_to_local_id(var, decl);
         then {
-            let mut v = LocalUsedVisitor::new(decl);
+            let mut v = LocalUsedVisitor::new(cx, decl);
 
             if block.stmts.iter().take(block.stmts.len()-1).any(|stmt| v.check_stmt(stmt)) {
                 return None;
diff --git a/clippy_lints/src/loops.rs b/clippy_lints/src/loops.rs
@@ -1893,8 +1893,8 @@ fn check_for_loop_over_map_kv<'tcx>(
             let arg_span = arg.span;
             let (new_pat_span, kind, ty, mutbl) = match *cx.typeck_results().expr_ty(arg).kind() {
                 ty::Ref(_, ty, mutbl) => match (&pat[0].kind, &pat[1].kind) {
-                    (key, _) if pat_is_wild(key, body) => (pat[1].span, "value", ty, mutbl),
-                    (_, value) if pat_is_wild(value, body) => (pat[0].span, "key", ty, Mutability::Not),
+                    (key, _) if pat_is_wild(cx, key, body) => (pat[1].span, "value", ty, mutbl),
+                    (_, value) if pat_is_wild(cx, value, body) => (pat[0].span, "key", ty, Mutability::Not),
                     _ => return,
                 },
                 _ => return,
@@ -2145,11 +2145,11 @@ fn check_for_mutation<'tcx>(
 }
 
 /// Returns `true` if the pattern is a `PatWild` or an ident prefixed with `_`.
-fn pat_is_wild<'tcx>(pat: &'tcx PatKind<'_>, body: &'tcx Expr<'_>) -> bool {
+fn pat_is_wild<'tcx>(cx: &LateContext<'tcx>, pat: &'tcx PatKind<'_>, body: &'tcx Expr<'_>) -> bool {
     match *pat {
         PatKind::Wild => true,
         PatKind::Binding(_, id, ident, None) if ident.as_str().starts_with('_') => {
-            !LocalUsedVisitor::new(id).check_expr(body)
+            !LocalUsedVisitor::new(cx, id).check_expr(body)
         },
         _ => false,
     }
@@ -2188,7 +2188,7 @@ impl<'a, 'tcx> VarVisitor<'a, 'tcx> {
             then {
                 let index_used_directly = path_to_local_id(idx, self.var);
                 let indexed_indirectly = {
-                    let mut used_visitor = LocalUsedVisitor::new(self.var);
+                    let mut used_visitor = LocalUsedVisitor::new(self.cx, self.var);
                     walk_expr(&mut used_visitor, idx);
                     used_visitor.used
                 };
diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs
@@ -911,7 +911,7 @@ fn check_overlapping_arms<'tcx>(cx: &LateContext<'tcx>, ex: &'tcx Expr<'_>, arms
     }
 }
 
-fn check_wild_err_arm(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>]) {
+fn check_wild_err_arm<'tcx>(cx: &LateContext<'tcx>, ex: &Expr<'tcx>, arms: &[Arm<'tcx>]) {
     let ex_ty = cx.typeck_results().expr_ty(ex).peel_refs();
     if is_type_diagnostic_item(cx, ex_ty, sym::result_type) {
         for arm in arms {
@@ -924,7 +924,9 @@ fn check_wild_err_arm(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>]) {
                         // Looking for unused bindings (i.e.: `_e`)
                         inner.iter().for_each(|pat| {
                             if let PatKind::Binding(_, id, ident, None) = pat.kind {
-                                if ident.as_str().starts_with('_') && !LocalUsedVisitor::new(id).check_expr(arm.body) {
+                                if ident.as_str().starts_with('_')
+                                    && !LocalUsedVisitor::new(cx, id).check_expr(arm.body)
+                                {
                                     ident_bind_name = (&ident.name.as_str()).to_string();
                                     matching_wild = true;
                                 }
diff --git a/clippy_lints/src/utils/visitors.rs b/clippy_lints/src/utils/visitors.rs
@@ -133,14 +133,16 @@ where
     }
 }
 
-pub struct LocalUsedVisitor {
+pub struct LocalUsedVisitor<'hir> {
+    hir: Map<'hir>,
     pub local_hir_id: HirId,
     pub used: bool,
 }
 
-impl LocalUsedVisitor {
-    pub fn new(local_hir_id: HirId) -> Self {
+impl<'hir> LocalUsedVisitor<'hir> {
+    pub fn new(cx: &LateContext<'hir>, local_hir_id: HirId) -> Self {
         Self {
+            hir: cx.tcx.hir(),
             local_hir_id,
             used: false,
         }
@@ -151,23 +153,26 @@ impl LocalUsedVisitor {
         std::mem::replace(&mut self.used, false)
     }
 
-    pub fn check_arm(&mut self, arm: &Arm<'_>) -> bool {
+    pub fn check_arm(&mut self, arm: &'hir Arm<'_>) -> bool {
         self.check(arm, Self::visit_arm)
     }
 
-    pub fn check_expr(&mut self, expr: &Expr<'_>) -> bool {
+    pub fn check_expr(&mut self, expr: &'hir Expr<'_>) -> bool {
         self.check(expr, Self::visit_expr)
     }
 
-    pub fn check_stmt(&mut self, stmt: &Stmt<'_>) -> bool {
+    pub fn check_stmt(&mut self, stmt: &'hir Stmt<'_>) -> bool {
         self.check(stmt, Self::visit_stmt)
     }
 }
 
-impl<'v> Visitor<'v> for LocalUsedVisitor {
+impl<'v> Visitor<'v> for LocalUsedVisitor<'v> {
     type Map = Map<'v>;
 
     fn visit_expr(&mut self, expr: &'v Expr<'v>) {
+        if self.used {
+            return;
+        }
         if path_to_local_id(expr, self.local_hir_id) {
             self.used = true;
         } else {
@@ -176,6 +181,6 @@ impl<'v> Visitor<'v> for LocalUsedVisitor {
     }
 
     fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {
-        NestedVisitorMap::None
+        NestedVisitorMap::OnlyBodies(self.hir)
     }
 }
diff --git a/tests/ui/collapsible_match.rs b/tests/ui/collapsible_match.rs
@@ -224,6 +224,14 @@ fn negative_cases(res_opt: Result<Option<u32>, String>, res_res: Result<Result<u
         },
         _ => return,
     }
+    if let Ok(val) = res_opt {
+        if let Some(n) = val {
+            let _ = || {
+                // usage in closure
+                println!("{:?}", val);
+            };
+        }
+    }
 }
 
 fn make<T>() -> T {

Original file line number	Diff line number	Diff line change
`@@ -911,7 +911,7 @@ fn check_overlapping_arms<'tcx>(cx: &LateContext<'tcx>, ex: &'tcx Expr<'_>, arms`
`911`	`911`	`}`
`912`	`912`	`}`
`913`	`913`
`914`		`-fn check_wild_err_arm(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>]) {`
	`914`	`+fn check_wild_err_arm<'tcx>(cx: &LateContext<'tcx>, ex: &Expr<'tcx>, arms: &[Arm<'tcx>]) {`
`915`	`915`	`let ex_ty = cx.typeck_results().expr_ty(ex).peel_refs();`
`916`	`916`	`if is_type_diagnostic_item(cx, ex_ty, sym::result_type) {`
`917`	`917`	`for arm in arms {`
`@@ -924,7 +924,9 @@ fn check_wild_err_arm(cx: &LateContext<'_>, ex: &Expr<'_>, arms: &[Arm<'_>]) {`
`924`	`924`	// Looking for unused bindings (i.e.: `_e`)
`925`	`925`	`inner.iter().for_each(\|pat\| {`
`926`	`926`	`if let PatKind::Binding(_, id, ident, None) = pat.kind {`
`927`		`- if ident.as_str().starts_with('_') && !LocalUsedVisitor::new(id).check_expr(arm.body) {`
	`927`	`+ if ident.as_str().starts_with('_')`
	`928`	`+ && !LocalUsedVisitor::new(cx, id).check_expr(arm.body)`
	`929`	`+ {`
`928`	`930`	`ident_bind_name = (&ident.name.as_str()).to_string();`
`929`	`931`	`matching_wild = true;`
`930`	`932`	`}`
Original file line number	Diff line number	Diff line change
`@@ -133,14 +133,16 @@ where`
`133`	`133`	`}`
`134`	`134`	`}`
`135`	`135`
`136`		`-pub struct LocalUsedVisitor {`
	`136`	`+pub struct LocalUsedVisitor<'hir> {`
	`137`	`+ hir: Map<'hir>,`
`137`	`138`	`pub local_hir_id: HirId,`
`138`	`139`	`pub used: bool,`
`139`	`140`	`}`
`140`	`141`
`141`		`-impl LocalUsedVisitor {`
`142`		`- pub fn new(local_hir_id: HirId) -> Self {`
	`142`	`+impl<'hir> LocalUsedVisitor<'hir> {`
	`143`	`+ pub fn new(cx: &LateContext<'hir>, local_hir_id: HirId) -> Self {`
`143`	`144`	`Self {`
	`145`	`+ hir: cx.tcx.hir(),`
`144`	`146`	`local_hir_id,`
`145`	`147`	`used: false,`
`146`	`148`	`}`
`@@ -151,23 +153,26 @@ impl LocalUsedVisitor {`
`151`	`153`	`std::mem::replace(&mut self.used, false)`
`152`	`154`	`}`
`153`	`155`
`154`		`- pub fn check_arm(&mut self, arm: &Arm<'_>) -> bool {`
	`156`	`+ pub fn check_arm(&mut self, arm: &'hir Arm<'_>) -> bool {`
`155`	`157`	`self.check(arm, Self::visit_arm)`
`156`	`158`	`}`
`157`	`159`
`158`		`- pub fn check_expr(&mut self, expr: &Expr<'_>) -> bool {`
	`160`	`+ pub fn check_expr(&mut self, expr: &'hir Expr<'_>) -> bool {`
`159`	`161`	`self.check(expr, Self::visit_expr)`
`160`	`162`	`}`
`161`	`163`
`162`		`- pub fn check_stmt(&mut self, stmt: &Stmt<'_>) -> bool {`
	`164`	`+ pub fn check_stmt(&mut self, stmt: &'hir Stmt<'_>) -> bool {`
`163`	`165`	`self.check(stmt, Self::visit_stmt)`
`164`	`166`	`}`
`165`	`167`	`}`
`166`	`168`
`167`		`-impl<'v> Visitor<'v> for LocalUsedVisitor {`
	`169`	`+impl<'v> Visitor<'v> for LocalUsedVisitor<'v> {`
`168`	`170`	`type Map = Map<'v>;`
`169`	`171`
`170`	`172`	`fn visit_expr(&mut self, expr: &'v Expr<'v>) {`
	`173`	`+ if self.used {`
	`174`	`+ return;`
	`175`	`+ }`
`171`	`176`	`if path_to_local_id(expr, self.local_hir_id) {`
`172`	`177`	`self.used = true;`
`173`	`178`	`} else {`
`@@ -176,6 +181,6 @@ impl<'v> Visitor<'v> for LocalUsedVisitor {`
`176`	`181`	`}`
`177`	`182`
`178`	`183`	`fn nested_visit_map(&mut self) -> NestedVisitorMap<Self::Map> {`
`179`		`- NestedVisitorMap::None`
	`184`	`+ NestedVisitorMap::OnlyBodies(self.hir)`
`180`	`185`	`}`
`181`	`186`	`}`
Original file line number	Diff line number	Diff line change
`@@ -224,6 +224,14 @@ fn negative_cases(res_opt: Result<Option<u32>, String>, res_res: Result<Result<u`
`224`	`224`	`},`
`225`	`225`	`_ => return,`
`226`	`226`	`}`
	`227`	`+ if let Ok(val) = res_opt {`
	`228`	`+ if let Some(n) = val {`
	`229`	`+ let _ = \|\| {`
	`230`	`+ // usage in closure`
	`231`	`+ println!("{:?}", val);`
	`232`	`+ };`
	`233`	`+ }`
	`234`	`+ }`
`227`	`235`	`}`
`228`	`236`
`229`	`237`	`fn make<T>() -> T {`