Auto merge of #8762 - Jarcho:visitor, r=Jarcho

Replace `expr_visitor` with  `for_each_expr`

This is a minor change which uses `ControlFlow` rather than a boolean. This also runs the visitor rather than returning the visitor, which results in a small readability win as well.

changelog: None

5 files changed, 151 insertions, 199 deletions

diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs
index 8f67fa109fc..3caa6380e09 100644
--- a/clippy_utils/src/lib.rs
+++ b/clippy_utils/src/lib.rs
@@ -3,6 +3,7 @@
 #![feature(control_flow_enum)]
 #![feature(let_chains)]
 #![feature(lint_reasons)]
+#![feature(never_type)]
 #![feature(once_cell)]
 #![feature(rustc_private)]
 #![recursion_limit = "512"]
@@ -65,6 +66,7 @@ pub use self::hir_utils::{
     both, count_eq, eq_expr_value, hash_expr, hash_stmt, over, HirEqInterExpr, SpanlessEq, SpanlessHash,
 };
 
+use core::ops::ControlFlow;
 use std::collections::hash_map::Entry;
 use std::hash::BuildHasherDefault;
 use std::sync::OnceLock;
@@ -113,7 +115,7 @@ use rustc_target::abi::Integer;
 
 use crate::consts::{constant, Constant};
 use crate::ty::{can_partially_move_ty, expr_sig, is_copy, is_recursively_primitive_type, ty_is_fn_once_param};
-use crate::visitors::expr_visitor_no_bodies;
+use crate::visitors::for_each_expr;
 
 pub fn parse_msrv(msrv: &str, sess: Option<&Session>, span: Option<Span>) -> Option<RustcVersion> {
     if let Ok(version) = RustcVersion::parse(msrv) {
@@ -1193,17 +1195,14 @@ pub fn contains_name(name: Symbol, expr: &Expr<'_>) -> bool {
 
 /// Returns `true` if `expr` contains a return expression
 pub fn contains_return(expr: &hir::Expr<'_>) -> bool {
-    let mut found = false;
-    expr_visitor_no_bodies(|expr| {
-        if !found {
-            if let hir::ExprKind::Ret(..) = &expr.kind {
-                found = true;
-            }
+    for_each_expr(expr, |e| {
+        if matches!(e.kind, hir::ExprKind::Ret(..)) {
+            ControlFlow::Break(())
+        } else {
+            ControlFlow::Continue(())
         }
-        !found
     })
-    .visit_expr(expr);
-    found
+    .is_some()
 }
 
 /// Extends the span to the beginning of the spans line, incl. whitespaces.
diff --git a/clippy_utils/src/macros.rs b/clippy_utils/src/macros.rs
index 079c8f50f12..dda21b90390 100644
--- a/clippy_utils/src/macros.rs
+++ b/clippy_utils/src/macros.rs
@@ -2,7 +2,7 @@
 
 use crate::is_path_diagnostic_item;
 use crate::source::snippet_opt;
-use crate::visitors::expr_visitor_no_bodies;
+use crate::visitors::{for_each_expr, Descend};
 
 use arrayvec::ArrayVec;
 use itertools::{izip, Either, Itertools};
@@ -270,20 +270,19 @@ fn find_assert_args_inner<'a, const N: usize>(
     };
     let mut args = ArrayVec::new();
     let mut panic_expn = None;
-    expr_visitor_no_bodies(|e| {
+    let _: Option<!> = for_each_expr(expr, |e| {
         if args.is_full() {
             if panic_expn.is_none() && e.span.ctxt() != expr.span.ctxt() {
                 panic_expn = PanicExpn::parse(cx, e);
             }
-            panic_expn.is_none()
+            ControlFlow::Continue(Descend::from(panic_expn.is_none()))
         } else if is_assert_arg(cx, e, expn) {
             args.push(e);
-            false
+            ControlFlow::Continue(Descend::No)
         } else {
-            true
+            ControlFlow::Continue(Descend::Yes)
         }
-    })
-    .visit_expr(expr);
+    });
     let args = args.into_inner().ok()?;
     // if no `panic!(..)` is found, use `PanicExpn::Empty`
     // to indicate that the default assertion message is used
@@ -297,22 +296,19 @@ fn find_assert_within_debug_assert<'a>(
     expn: ExpnId,
     assert_name: Symbol,
 ) -> Option<(&'a Expr<'a>, ExpnId)> {
-    let mut found = None;
-    expr_visitor_no_bodies(|e| {
-        if found.is_some() || !e.span.from_expansion() {
-            return false;
+    for_each_expr(expr, |e| {
+        if !e.span.from_expansion() {
+            return ControlFlow::Continue(Descend::No);
         }
         let e_expn = e.span.ctxt().outer_expn();
         if e_expn == expn {
-            return true;
-        }
-        if e_expn.expn_data().macro_def_id.map(|id| cx.tcx.item_name(id)) == Some(assert_name) {
-            found = Some((e, e_expn));
+            ControlFlow::Continue(Descend::Yes)
+        } else if e_expn.expn_data().macro_def_id.map(|id| cx.tcx.item_name(id)) == Some(assert_name) {
+            ControlFlow::Break((e, e_expn))
+        } else {
+            ControlFlow::Continue(Descend::No)
         }
-        false
     })
-    .visit_expr(expr);
-    found
 }
 
 fn is_assert_arg(cx: &LateContext<'_>, expr: &Expr<'_>, assert_expn: ExpnId) -> bool {
@@ -396,16 +392,14 @@ impl FormatString {
         });
 
         let mut parts = Vec::new();
-        expr_visitor_no_bodies(|expr| {
-            if let ExprKind::Lit(lit) = &expr.kind {
-                if let LitKind::Str(symbol, _) = lit.node {
-                    parts.push(symbol);
-                }
+        let _: Option<!> = for_each_expr(pieces, |expr| {
+            if let ExprKind::Lit(lit) = &expr.kind
+                && let LitKind::Str(symbol, _) = lit.node
+            {
+                parts.push(symbol);
             }
-
-            true
-        })
-        .visit_expr(pieces);
+            ControlFlow::Continue(())
+        });
 
         Some(Self {
             span,
@@ -431,7 +425,7 @@ impl<'tcx> FormatArgsValues<'tcx> {
     fn new(args: &'tcx Expr<'tcx>, format_string_span: SpanData) -> Self {
         let mut pos_to_value_index = Vec::new();
         let mut value_args = Vec::new();
-        expr_visitor_no_bodies(|expr| {
+        let _: Option<!> = for_each_expr(args, |expr| {
             if expr.span.ctxt() == args.span.ctxt() {
                 // ArgumentV1::new_<format_trait>(<val>)
                 // ArgumentV1::from_usize(<val>)
@@ -453,16 +447,13 @@ impl<'tcx> FormatArgsValues<'tcx> {
 
                     pos_to_value_index.push(val_idx);
                 }
-
-                true
+                ControlFlow::Continue(Descend::Yes)
             } else {
                 // assume that any expr with a differing span is a value
                 value_args.push(expr);
-
-                false
+                ControlFlow::Continue(Descend::No)
             }
-        })
-        .visit_expr(args);
+        });
 
         Self {
             value_args,
@@ -866,22 +857,20 @@ impl<'tcx> FormatArgsExpn<'tcx> {
     }
 
     pub fn find_nested(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, expn_id: ExpnId) -> Option<Self> {
-        let mut format_args = None;
-        expr_visitor_no_bodies(|e| {
-            if format_args.is_some() {
-                return false;
-            }
+        for_each_expr(expr, |e| {
             let e_ctxt = e.span.ctxt();
             if e_ctxt == expr.span.ctxt() {
-                return true;
-            }
-            if e_ctxt.outer_expn().is_descendant_of(expn_id) {
-                format_args = FormatArgsExpn::parse(cx, e);
+                ControlFlow::Continue(Descend::Yes)
+            } else if e_ctxt.outer_expn().is_descendant_of(expn_id) {
+                if let Some(args) = FormatArgsExpn::parse(cx, e) {
+                    ControlFlow::Break(args)
+                } else {
+                    ControlFlow::Continue(Descend::No)
+                }
+            } else {
+                ControlFlow::Continue(Descend::No)
             }
-            false
         })
-        .visit_expr(expr);
-        format_args
     }
 
     /// Source callsite span of all inputs
diff --git a/clippy_utils/src/ptr.rs b/clippy_utils/src/ptr.rs
index 0226f74906b..88837d8a143 100644
--- a/clippy_utils/src/ptr.rs
+++ b/clippy_utils/src/ptr.rs
@@ -1,7 +1,7 @@
 use crate::source::snippet;
-use crate::visitors::expr_visitor_no_bodies;
+use crate::visitors::{for_each_expr, Descend};
 use crate::{path_to_local_id, strip_pat_refs};
-use rustc_hir::intravisit::Visitor;
+use core::ops::ControlFlow;
 use rustc_hir::{Body, BodyId, ExprKind, HirId, PatKind};
 use rustc_lint::LateContext;
 use rustc_span::Span;
@@ -30,28 +30,23 @@ fn extract_clone_suggestions<'tcx>(
     replace: &[(&'static str, &'static str)],
     body: &'tcx Body<'_>,
 ) -> Option<Vec<(Span, Cow<'static, str>)>> {
-    let mut abort = false;
     let mut spans = Vec::new();
-    expr_visitor_no_bodies(|expr| {
-        if abort {
-            return false;
-        }
-        if let ExprKind::MethodCall(seg, recv, [], _) = expr.kind {
-            if path_to_local_id(recv, id) {
-                if seg.ident.name.as_str() == "capacity" {
-                    abort = true;
-                    return false;
-                }
-                for &(fn_name, suffix) in replace {
-                    if seg.ident.name.as_str() == fn_name {
-                        spans.push((expr.span, snippet(cx, recv.span, "_") + suffix));
-                        return false;
-                    }
+    for_each_expr(body, |e| {
+        if let ExprKind::MethodCall(seg, recv, [], _) = e.kind
+            && path_to_local_id(recv, id)
+        {
+            if seg.ident.as_str() == "capacity" {
+                return ControlFlow::Break(());
+            }
+            for &(fn_name, suffix) in replace {
+                if seg.ident.as_str() == fn_name {
+                    spans.push((e.span, snippet(cx, recv.span, "_") + suffix));
+                    return ControlFlow::Continue(Descend::No);
                 }
             }
         }
-        !abort
+        ControlFlow::Continue(Descend::Yes)
     })
-    .visit_body(body);
-    if abort { None } else { Some(spans) }
+    .is_none()
+    .then_some(spans)
 }
diff --git a/clippy_utils/src/usage.rs b/clippy_utils/src/usage.rs
index 3221b82aed4..b5ec3fef3e0 100644
--- a/clippy_utils/src/usage.rs
+++ b/clippy_utils/src/usage.rs
@@ -1,5 +1,6 @@
 use crate as utils;
-use crate::visitors::{expr_visitor, expr_visitor_no_bodies};
+use crate::visitors::{for_each_expr, for_each_expr_with_closures, Descend};
+use core::ops::ControlFlow;
 use rustc_hir as hir;
 use rustc_hir::intravisit::{self, Visitor};
 use rustc_hir::HirIdSet;
@@ -148,28 +149,17 @@ impl<'a, 'tcx> intravisit::Visitor<'tcx> for BindingUsageFinder<'a, 'tcx> {
 }
 
 pub fn contains_return_break_continue_macro(expression: &Expr<'_>) -> bool {
-    let mut seen_return_break_continue = false;
-    expr_visitor_no_bodies(|ex| {
-        if seen_return_break_continue {
-            return false;
-        }
-        match &ex.kind {
-            ExprKind::Ret(..) | ExprKind::Break(..) | ExprKind::Continue(..) => {
-                seen_return_break_continue = true;
-            },
+    for_each_expr(expression, |e| {
+        match e.kind {
+            ExprKind::Ret(..) | ExprKind::Break(..) | ExprKind::Continue(..) => ControlFlow::Break(()),
             // Something special could be done here to handle while or for loop
             // desugaring, as this will detect a break if there's a while loop
             // or a for loop inside the expression.
-            _ => {
-                if ex.span.from_expansion() {
-                    seen_return_break_continue = true;
-                }
-            },
+            _ if e.span.from_expansion() => ControlFlow::Break(()),
+            _ => ControlFlow::Continue(()),
         }
-        !seen_return_break_continue
     })
-    .visit_expr(expression);
-    seen_return_break_continue
+    .is_some()
 }
 
 pub fn local_used_after_expr(cx: &LateContext<'_>, local_id: HirId, after: &Expr<'_>) -> bool {
@@ -200,23 +190,16 @@ pub fn local_used_after_expr(cx: &LateContext<'_>, local_id: HirId, after: &Expr
         return true;
     }
 
-    let mut used_after_expr = false;
     let mut past_expr = false;
-    expr_visitor(cx, |expr| {
-        if used_after_expr {
-            return false;
-        }
-
-        if expr.hir_id == after.hir_id {
+    for_each_expr_with_closures(cx, block, |e| {
+        if e.hir_id == after.hir_id {
             past_expr = true;
-            return false;
-        }
-
-        if past_expr && utils::path_to_local_id(expr, local_id) {
-            used_after_expr = true;
+            ControlFlow::Continue(Descend::No)
+        } else if past_expr && utils::path_to_local_id(e, local_id) {
+            ControlFlow::Break(())
+        } else {
+            ControlFlow::Continue(Descend::Yes)
         }
-        !used_after_expr
     })
-    .visit_block(block);
-    used_after_expr
+    .is_some()
 }
diff --git a/clippy_utils/src/visitors.rs b/clippy_utils/src/visitors.rs
index 232d571902b..d4294f18fd5 100644
--- a/clippy_utils/src/visitors.rs
+++ b/clippy_utils/src/visitors.rs
@@ -5,14 +5,13 @@ use rustc_hir as hir;
 use rustc_hir::def::{CtorKind, DefKind, Res};
 use rustc_hir::intravisit::{self, walk_block, walk_expr, Visitor};
 use rustc_hir::{
-    Arm, Block, BlockCheckMode, Body, BodyId, Expr, ExprKind, HirId, ItemId, ItemKind, Let, Pat, QPath, Stmt, UnOp,
-    UnsafeSource, Unsafety,
+    AnonConst, Arm, Block, BlockCheckMode, Body, BodyId, Expr, ExprKind, HirId, ItemId, ItemKind, Let, Pat, QPath,
+    Stmt, UnOp, UnsafeSource, Unsafety,
 };
 use rustc_lint::LateContext;
-use rustc_middle::hir::map::Map;
 use rustc_middle::hir::nested_filter;
 use rustc_middle::ty::adjustment::Adjust;
-use rustc_middle::ty::{self, Ty, TypeckResults};
+use rustc_middle::ty::{self, Ty, TyCtxt, TypeckResults};
 use rustc_span::Span;
 
 mod internal {
@@ -48,6 +47,26 @@ impl Continue for Descend {
     }
 }
 
+/// A type which can be visited.
+pub trait Visitable<'tcx> {
+    /// Calls the corresponding `visit_*` function on the visitor.
+    fn visit<V: Visitor<'tcx>>(self, visitor: &mut V);
+}
+macro_rules! visitable_ref {
+    ($t:ident, $f:ident) => {
+        impl<'tcx> Visitable<'tcx> for &'tcx $t<'tcx> {
+            fn visit<V: Visitor<'tcx>>(self, visitor: &mut V) {
+                visitor.$f(self);
+            }
+        }
+    };
+}
+visitable_ref!(Arm, visit_arm);
+visitable_ref!(Block, visit_block);
+visitable_ref!(Body, visit_body);
+visitable_ref!(Expr, visit_expr);
+visitable_ref!(Stmt, visit_stmt);
+
 /// Calls the given function once for each expression contained. This does not enter any bodies or
 /// nested items.
 pub fn for_each_expr<'tcx, B, C: Continue>(
@@ -82,57 +101,63 @@ pub fn for_each_expr<'tcx, B, C: Continue>(
     v.res
 }
 
-/// Convenience method for creating a `Visitor` with just `visit_expr` overridden and nested
-/// bodies (i.e. closures) are visited.
-/// If the callback returns `true`, the expr just provided to the callback is walked.
-#[must_use]
-pub fn expr_visitor<'tcx>(cx: &LateContext<'tcx>, f: impl FnMut(&'tcx Expr<'tcx>) -> bool) -> impl Visitor<'tcx> {
-    struct V<'tcx, F> {
-        hir: Map<'tcx>,
+/// Calls the given function once for each expression contained. This will enter bodies, but not
+/// nested items.
+pub fn for_each_expr_with_closures<'tcx, B, C: Continue>(
+    cx: &LateContext<'tcx>,
+    node: impl Visitable<'tcx>,
+    f: impl FnMut(&'tcx Expr<'tcx>) -> ControlFlow<B, C>,
+) -> Option<B> {
+    struct V<'tcx, B, F> {
+        tcx: TyCtxt<'tcx>,
         f: F,
+        res: Option<B>,
     }
-    impl<'tcx, F: FnMut(&'tcx Expr<'tcx>) -> bool> Visitor<'tcx> for V<'tcx, F> {
+    impl<'tcx, B, C: Continue, F: FnMut(&'tcx Expr<'tcx>) -> ControlFlow<B, C>> Visitor<'tcx> for V<'tcx, B, F> {
         type NestedFilter = nested_filter::OnlyBodies;
         fn nested_visit_map(&mut self) -> Self::Map {
-            self.hir
+            self.tcx.hir()
         }
 
-        fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) {
-            if (self.f)(expr) {
-                walk_expr(self, expr);
+        fn visit_expr(&mut self, e: &'tcx Expr<'tcx>) {
+            if self.res.is_some() {
+                return;
             }
-        }
-    }
-    V { hir: cx.tcx.hir(), f }
-}
-
-/// Convenience method for creating a `Visitor` with just `visit_expr` overridden and nested
-/// bodies (i.e. closures) are not visited.
-/// If the callback returns `true`, the expr just provided to the callback is walked.
-#[must_use]
-pub fn expr_visitor_no_bodies<'tcx>(f: impl FnMut(&'tcx Expr<'tcx>) -> bool) -> impl Visitor<'tcx> {
-    struct V<F>(F);
-    impl<'tcx, F: FnMut(&'tcx Expr<'tcx>) -> bool> Visitor<'tcx> for V<F> {
-        fn visit_expr(&mut self, e: &'tcx Expr<'_>) {
-            if (self.0)(e) {
-                walk_expr(self, e);
+            match (self.f)(e) {
+                ControlFlow::Continue(c) if c.descend() => walk_expr(self, e),
+                ControlFlow::Break(b) => self.res = Some(b),
+                ControlFlow::Continue(_) => (),
             }
         }
+
+        // Only walk closures
+        fn visit_anon_const(&mut self, _: &'tcx AnonConst) {}
+        // Avoid unnecessary `walk_*` calls.
+        fn visit_ty(&mut self, _: &'tcx hir::Ty<'tcx>) {}
+        fn visit_pat(&mut self, _: &'tcx Pat<'tcx>) {}
+        fn visit_qpath(&mut self, _: &'tcx QPath<'tcx>, _: HirId, _: Span) {}
+        // Avoid monomorphising all `visit_*` functions.
+        fn visit_nested_item(&mut self, _: ItemId) {}
     }
-    V(f)
+    let mut v = V {
+        tcx: cx.tcx,
+        f,
+        res: None,
+    };
+    node.visit(&mut v);
+    v.res
 }
 
 /// returns `true` if expr contains match expr desugared from try
 fn contains_try(expr: &hir::Expr<'_>) -> bool {
-    let mut found = false;
-    expr_visitor_no_bodies(|e| {
-        if !found {
-            found = matches!(e.kind, hir::ExprKind::Match(_, _, hir::MatchSource::TryDesugar));
+    for_each_expr(expr, |e| {
+        if matches!(e.kind, hir::ExprKind::Match(_, _, hir::MatchSource::TryDesugar)) {
+            ControlFlow::Break(())
+        } else {
+            ControlFlow::Continue(())
         }
-        !found
     })
-    .visit_expr(expr);
-    found
+    .is_some()
 }
 
 pub fn find_all_ret_expressions<'hir, F>(_cx: &LateContext<'_>, expr: &'hir hir::Expr<'hir>, callback: F) -> bool
@@ -228,68 +253,29 @@ where
     }
 }
 
-/// A type which can be visited.
-pub trait Visitable<'tcx> {
-    /// Calls the corresponding `visit_*` function on the visitor.
-    fn visit<V: Visitor<'tcx>>(self, visitor: &mut V);
-}
-macro_rules! visitable_ref {
-    ($t:ident, $f:ident) => {
-        impl<'tcx> Visitable<'tcx> for &'tcx $t<'tcx> {
-            fn visit<V: Visitor<'tcx>>(self, visitor: &mut V) {
-                visitor.$f(self);
-            }
-        }
-    };
-}
-visitable_ref!(Arm, visit_arm);
-visitable_ref!(Block, visit_block);
-visitable_ref!(Body, visit_body);
-visitable_ref!(Expr, visit_expr);
-visitable_ref!(Stmt, visit_stmt);
-
-// impl<'tcx, I: IntoIterator> Visitable<'tcx> for I
-// where
-//     I::Item: Visitable<'tcx>,
-// {
-//     fn visit<V: Visitor<'tcx>>(self, visitor: &mut V) {
-//         for x in self {
-//             x.visit(visitor);
-//         }
-//     }
-// }
-
 /// Checks if the given resolved path is used in the given body.
 pub fn is_res_used(cx: &LateContext<'_>, res: Res, body: BodyId) -> bool {
-    let mut found = false;
-    expr_visitor(cx, |e| {
-        if found {
-            return false;
-        }
-
+    for_each_expr_with_closures(cx, cx.tcx.hir().body(body).value, |e| {
         if let ExprKind::Path(p) = &e.kind {
             if cx.qpath_res(p, e.hir_id) == res {
-                found = true;
+                return ControlFlow::Break(());
             }
         }
-        !found
+        ControlFlow::Continue(())
     })
-    .visit_expr(cx.tcx.hir().body(body).value);
-    found
+    .is_some()
 }
 
 /// Checks if the given local is used.
 pub fn is_local_used<'tcx>(cx: &LateContext<'tcx>, visitable: impl Visitable<'tcx>, id: HirId) -> bool {
-    let mut is_used = false;
-    let mut visitor = expr_visitor(cx, |expr| {
-        if !is_used {
-            is_used = path_to_local_id(expr, id);
+    for_each_expr_with_closures(cx, visitable, |e| {
+        if path_to_local_id(e, id) {
+            ControlFlow::Break(())
+        } else {
+            ControlFlow::Continue(())
         }
-        !is_used
-    });
-    visitable.visit(&mut visitor);
-    drop(visitor);
-    is_used
+    })
+    .is_some()
 }
 
 /// Checks if the given expression is a constant.