From 1d3042294503e6027759baadc92056403aa8c991 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Wed, 3 Feb 2021 11:35:16 -0600 Subject: [PATCH 1/3] Enhance LocalUsedVisitor to check closure bodies --- clippy_lints/src/collapsible_match.rs | 9 ++++----- clippy_lints/src/let_if_seq.rs | 17 +++++++++++------ clippy_lints/src/loops.rs | 10 +++++----- clippy_lints/src/matches.rs | 6 ++++-- clippy_lints/src/utils/visitors.rs | 21 +++++++++++++-------- tests/ui/collapsible_match.rs | 8 ++++++++ 6 files changed, 45 insertions(+), 26 deletions(-) diff --git a/clippy_lints/src/collapsible_match.rs b/clippy_lints/src/collapsible_match.rs index 834f294283e..75a973fe37e 100644 --- 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 index 6beaa51729a..5863eef8a26 100644 --- 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,7 +134,11 @@ 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(); @@ -141,7 +146,7 @@ fn check_assign<'tcx>(decl: hir::HirId, block: &'tcx hir::Block<'_>) -> Option<& 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 index b5a9632ee19..eb185377e20 100644 --- 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 index c4aa2b30e7b..e33001b16bc 100644 --- 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 index a4064c3e705..24409346ca6 100644 --- 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 { - NestedVisitorMap::None + NestedVisitorMap::OnlyBodies(self.hir) } } diff --git a/tests/ui/collapsible_match.rs b/tests/ui/collapsible_match.rs index a83e6c77b12..3294da7e814 100644 --- a/tests/ui/collapsible_match.rs +++ b/tests/ui/collapsible_match.rs @@ -224,6 +224,14 @@ fn negative_cases(res_opt: Result, String>, res_res: Result return, } + if let Ok(val) = res_opt { + if let Some(n) = val { + let _ = || { + // usage in closure + println!("{:?}", val); + }; + } + } } fn make() -> T { From 5db48a382a928969be301ee416df27f6508105c2 Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Wed, 3 Feb 2021 11:45:16 -0600 Subject: [PATCH 2/3] Refactor out UnusedSelfVisitor --- clippy_lints/src/unused_self.rs | 39 +++--------------------------- clippy_lints/src/utils/visitors.rs | 6 ++++- 2 files changed, 8 insertions(+), 37 deletions(-) diff --git a/clippy_lints/src/unused_self.rs b/clippy_lints/src/unused_self.rs index 5349c4f7eb8..9d61bd0cc2f 100644 --- a/clippy_lints/src/unused_self.rs +++ b/clippy_lints/src/unused_self.rs @@ -1,12 +1,10 @@ use if_chain::if_chain; -use rustc_hir::def::Res; -use rustc_hir::intravisit::{walk_path, NestedVisitorMap, Visitor}; -use rustc_hir::{HirId, Impl, ImplItem, ImplItemKind, ItemKind, Path}; +use rustc_hir::{Impl, ImplItem, ImplItemKind, ItemKind}; use rustc_lint::{LateContext, LateLintPass}; -use rustc_middle::hir::map::Map; use rustc_session::{declare_lint_pass, declare_tool_lint}; use crate::utils::span_lint_and_help; +use crate::utils::visitors::LocalUsedVisitor; declare_clippy_lint! { /// **What it does:** Checks methods that contain a `self` argument but don't use it @@ -57,13 +55,7 @@ impl<'tcx> LateLintPass<'tcx> for UnusedSelf { then { let self_param = &body.params[0]; let self_hir_id = self_param.pat.hir_id; - let mut visitor = UnusedSelfVisitor { - cx, - uses_self: false, - self_hir_id: &self_hir_id, - }; - visitor.visit_body(body); - if !visitor.uses_self { + if !LocalUsedVisitor::new(cx, self_hir_id).check_body(body) { span_lint_and_help( cx, UNUSED_SELF, @@ -78,28 +70,3 @@ impl<'tcx> LateLintPass<'tcx> for UnusedSelf { } } } - -struct UnusedSelfVisitor<'a, 'tcx> { - cx: &'a LateContext<'tcx>, - uses_self: bool, - self_hir_id: &'a HirId, -} - -impl<'a, 'tcx> Visitor<'tcx> for UnusedSelfVisitor<'a, 'tcx> { - type Map = Map<'tcx>; - - fn visit_path(&mut self, path: &'tcx Path<'_>, _id: HirId) { - if self.uses_self { - // This function already uses `self` - return; - } - if let Res::Local(hir_id) = &path.res { - self.uses_self = self.self_hir_id == hir_id - } - walk_path(self, path); - } - - fn nested_visit_map(&mut self) -> NestedVisitorMap { - NestedVisitorMap::OnlyBodies(self.cx.tcx.hir()) - } -} diff --git a/clippy_lints/src/utils/visitors.rs b/clippy_lints/src/utils/visitors.rs index 24409346ca6..085c1f9c0cb 100644 --- a/clippy_lints/src/utils/visitors.rs +++ b/clippy_lints/src/utils/visitors.rs @@ -1,7 +1,7 @@ use crate::utils::path_to_local_id; use rustc_hir as hir; use rustc_hir::intravisit::{self, walk_expr, NestedVisitorMap, Visitor}; -use rustc_hir::{Arm, Expr, HirId, Stmt}; +use rustc_hir::{Arm, Body, Expr, HirId, Stmt}; use rustc_lint::LateContext; use rustc_middle::hir::map::Map; @@ -157,6 +157,10 @@ impl<'hir> LocalUsedVisitor<'hir> { self.check(arm, Self::visit_arm) } + pub fn check_body(&mut self, body: &'hir Body<'_>) -> bool { + self.check(body, Self::visit_body) + } + pub fn check_expr(&mut self, expr: &'hir Expr<'_>) -> bool { self.check(expr, Self::visit_expr) } From 37555f8f73baf82b7761db56e7440c79a956b9ec Mon Sep 17 00:00:00 2001 From: Cameron Steffen Date: Mon, 8 Feb 2021 09:50:13 -0600 Subject: [PATCH 3/3] Use path_to_local_id --- clippy_lints/src/methods/filter_map_identity.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/methods/filter_map_identity.rs b/clippy_lints/src/methods/filter_map_identity.rs index d04e4be87ac..9e646360a40 100644 --- a/clippy_lints/src/methods/filter_map_identity.rs +++ b/clippy_lints/src/methods/filter_map_identity.rs @@ -1,4 +1,4 @@ -use crate::utils::{match_qpath, match_trait_method, paths, span_lint_and_sugg}; +use crate::utils::{match_qpath, match_trait_method, path_to_local_id, paths, span_lint_and_sugg}; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir as hir; @@ -32,12 +32,8 @@ pub(super) fn check( if let hir::ExprKind::Closure(_, _, body_id, _, _) = arg_node; let body = cx.tcx.hir().body(*body_id); - if let hir::PatKind::Binding(_, _, binding_ident, _) = body.params[0].pat.kind; - if let hir::ExprKind::Path(hir::QPath::Resolved(_, ref path)) = body.value.kind; - - if path.segments.len() == 1; - if path.segments[0].ident.name == binding_ident.name; - + if let hir::PatKind::Binding(_, binding_id, ..) = body.params[0].pat.kind; + if path_to_local_id(&body.value, binding_id); then { apply_lint("called `filter_map(|x| x)` on an `Iterator`"); }