From 5e335a52bc49991f6ffd40030b9948f3390f499d Mon Sep 17 00:00:00 2001 From: Alex Macleod Date: Sun, 10 Apr 2022 14:02:58 +0100 Subject: [PATCH] Check for loops/closures in `local_used_after_expr` --- clippy_lints/src/eta_reduction.rs | 5 ++-- clippy_utils/src/usage.rs | 33 +++++++++++++++++++++++-- tests/ui/eta.fixed | 4 +++ tests/ui/eta.rs | 4 +++ tests/ui/eta.stderr | 10 ++++++-- tests/ui/needless_option_as_deref.fixed | 14 +++++++++++ tests/ui/needless_option_as_deref.rs | 14 +++++++++++ 7 files changed, 77 insertions(+), 7 deletions(-) diff --git a/clippy_lints/src/eta_reduction.rs b/clippy_lints/src/eta_reduction.rs index 845863bd2..c2e32f1d9 100644 --- a/clippy_lints/src/eta_reduction.rs +++ b/clippy_lints/src/eta_reduction.rs @@ -3,7 +3,7 @@ use clippy_utils::higher::VecArgs; use clippy_utils::source::snippet_opt; use clippy_utils::ty::is_type_diagnostic_item; use clippy_utils::usage::local_used_after_expr; -use clippy_utils::{get_enclosing_loop_or_closure, higher, path_to_local, path_to_local_id}; +use clippy_utils::{higher, path_to_local, path_to_local_id}; use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir::def_id::DefId; @@ -125,8 +125,7 @@ impl<'tcx> LateLintPass<'tcx> for EtaReduction { if_chain! { if let ty::Closure(_, substs) = callee_ty.peel_refs().kind(); if substs.as_closure().kind() == ClosureKind::FnMut; - if get_enclosing_loop_or_closure(cx.tcx, expr).is_some() - || path_to_local(callee).map_or(false, |l| local_used_after_expr(cx, l, callee)); + if path_to_local(callee).map_or(false, |l| local_used_after_expr(cx, l, expr)); then { // Mutable closure is used after current expr; we cannot consume it. diff --git a/clippy_utils/src/usage.rs b/clippy_utils/src/usage.rs index 405e30635..4236e3aae 100644 --- a/clippy_utils/src/usage.rs +++ b/clippy_utils/src/usage.rs @@ -3,7 +3,7 @@ use crate::visitors::{expr_visitor, expr_visitor_no_bodies}; use rustc_hir as hir; use rustc_hir::intravisit::{self, Visitor}; use rustc_hir::HirIdSet; -use rustc_hir::{Expr, ExprKind, HirId}; +use rustc_hir::{Expr, ExprKind, HirId, Node}; use rustc_infer::infer::TyCtxtInferExt; use rustc_lint::LateContext; use rustc_middle::hir::nested_filter; @@ -169,6 +169,32 @@ pub fn contains_return_break_continue_macro(expression: &Expr<'_>) -> bool { pub fn local_used_after_expr(cx: &LateContext<'_>, local_id: HirId, after: &Expr<'_>) -> bool { let Some(block) = utils::get_enclosing_block(cx, local_id) else { return false }; + + // for _ in 1..3 { + // local + // } + // + // let closure = || local; + // closure(); + // closure(); + let in_loop_or_closure = cx + .tcx + .hir() + .parent_iter(after.hir_id) + .take_while(|&(id, _)| id != block.hir_id) + .any(|(_, node)| { + matches!( + node, + Node::Expr(Expr { + kind: ExprKind::Loop(..) | ExprKind::Closure(..), + .. + }) + ) + }); + if in_loop_or_closure { + return true; + } + let mut used_after_expr = false; let mut past_expr = false; expr_visitor(cx, |expr| { @@ -178,7 +204,10 @@ pub fn local_used_after_expr(cx: &LateContext<'_>, local_id: HirId, after: &Expr if expr.hir_id == after.hir_id { past_expr = true; - } else if past_expr && utils::path_to_local_id(expr, local_id) { + return false; + } + + if past_expr && utils::path_to_local_id(expr, local_id) { used_after_expr = true; } !used_after_expr diff --git a/tests/ui/eta.fixed b/tests/ui/eta.fixed index 5aedbea38..873ed689a 100644 --- a/tests/ui/eta.fixed +++ b/tests/ui/eta.fixed @@ -211,6 +211,10 @@ fn mutable_closure_in_loop() { let mut closure = |n| value += n; for _ in 0..5 { Some(1).map(&mut closure); + + let mut value = 0; + let mut in_loop = |n| value += n; + Some(1).map(in_loop); } } diff --git a/tests/ui/eta.rs b/tests/ui/eta.rs index 5fdf7fb97..4cb58eec9 100644 --- a/tests/ui/eta.rs +++ b/tests/ui/eta.rs @@ -211,6 +211,10 @@ fn mutable_closure_in_loop() { let mut closure = |n| value += n; for _ in 0..5 { Some(1).map(|n| closure(n)); + + let mut value = 0; + let mut in_loop = |n| value += n; + Some(1).map(|n| in_loop(n)); } } diff --git a/tests/ui/eta.stderr b/tests/ui/eta.stderr index cda84982c..d1ae889d6 100644 --- a/tests/ui/eta.stderr +++ b/tests/ui/eta.stderr @@ -117,10 +117,16 @@ LL | Some(1).map(|n| closure(n)); | ^^^^^^^^^^^^^^ help: replace the closure with the function itself: `&mut closure` error: redundant closure - --> $DIR/eta.rs:232:21 + --> $DIR/eta.rs:217:21 + | +LL | Some(1).map(|n| in_loop(n)); + | ^^^^^^^^^^^^^^ help: replace the closure with the function itself: `in_loop` + +error: redundant closure + --> $DIR/eta.rs:236:21 | LL | map_str_to_path(|s| s.as_ref()); | ^^^^^^^^^^^^^^ help: replace the closure with the method itself: `std::convert::AsRef::as_ref` -error: aborting due to 20 previous errors +error: aborting due to 21 previous errors diff --git a/tests/ui/needless_option_as_deref.fixed b/tests/ui/needless_option_as_deref.fixed index c09b07db3..acd22c6bb 100644 --- a/tests/ui/needless_option_as_deref.fixed +++ b/tests/ui/needless_option_as_deref.fixed @@ -16,6 +16,20 @@ fn main() { let _ = Some(Box::new(1)).as_deref(); let _ = Some(Box::new(1)).as_deref_mut(); + let mut y = 0; + let mut x = Some(&mut y); + for _ in 0..3 { + let _ = x.as_deref_mut(); + } + + let mut y = 0; + let mut x = Some(&mut y); + let mut closure = || { + let _ = x.as_deref_mut(); + }; + closure(); + closure(); + // #7846 let mut i = 0; let mut opt_vec = vec![Some(&mut i)]; diff --git a/tests/ui/needless_option_as_deref.rs b/tests/ui/needless_option_as_deref.rs index c3ba27ecc..61eda5052 100644 --- a/tests/ui/needless_option_as_deref.rs +++ b/tests/ui/needless_option_as_deref.rs @@ -16,6 +16,20 @@ fn main() { let _ = Some(Box::new(1)).as_deref(); let _ = Some(Box::new(1)).as_deref_mut(); + let mut y = 0; + let mut x = Some(&mut y); + for _ in 0..3 { + let _ = x.as_deref_mut(); + } + + let mut y = 0; + let mut x = Some(&mut y); + let mut closure = || { + let _ = x.as_deref_mut(); + }; + closure(); + closure(); + // #7846 let mut i = 0; let mut opt_vec = vec![Some(&mut i)];