[infinite_loop]: fix suggestion with unit ty return;

J-ZhengLi · J-ZhengLi · commit 3867e007fc5a · 2024-07-16T01:01:46.000+08:00
add more test cases with async clousures;
diff --git a/clippy_lints/src/loops/infinite_loop.rs b/clippy_lints/src/loops/infinite_loop.rs
@@ -1,8 +1,9 @@
+use std::ops::ControlFlow;
+
 use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::{fn_def_id, is_from_proc_macro, is_lint_allowed};
 use hir::intravisit::{walk_expr, Visitor};
 use hir::{ClosureKind, Expr, ExprKind, FnRetTy, FnSig, ItemKind, Node, Ty, TyKind};
-use rustc_ast::Label;
 use rustc_errors::Applicability;
 use rustc_hir as hir;
 use rustc_lint::{LateContext, LintContext};
@@ -15,7 +16,7 @@ pub(super) fn check<'tcx>(
     cx: &LateContext<'tcx>,
     expr: &Expr<'tcx>,
     loop_block: &'tcx hir::Block<'_>,
-    label: Option<Label>,
+    hir_id: hir::HirId,
 ) {
     if is_lint_allowed(cx, INFINITE_LOOP, expr.hir_id) {
         return;
@@ -31,21 +32,29 @@ pub(super) fn check<'tcx>(
 
     let mut loop_visitor = LoopVisitor {
         cx,
-        label,
-        is_finite: false,
-        loop_depth: 0,
+        hir_id,
+        block: loop_block,
+        nested_loop_count: 0,
     };
-    loop_visitor.visit_block(loop_block);
-
-    let is_finite_loop = loop_visitor.is_finite;
-
-    if !is_finite_loop {
+    if !loop_visitor.is_finite() {
         span_lint_and_then(cx, INFINITE_LOOP, expr.span, "infinite loop detected", |diag| {
             if let Some(span) = parent_fn_ret.sugg_span() {
+                // Check if the type span is after a `->` or not.
+                let suggestion = if cx
+                    .tcx
+                    .sess
+                    .source_map()
+                    .span_extend_to_prev_str(span, "->", false, true)
+                    .is_none()
+                {
+                    " -> !"
+                } else {
+                    "!"
+                };
                 diag.span_suggestion(
                     span,
                     "if this is intentional, consider specifying `!` as function return",
-                    " -> !",
+                    suggestion,
                     Applicability::MaybeIncorrect,
                 );
             } else {
@@ -92,38 +101,50 @@ fn get_parent_fn_ret_ty<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) -> Option
 
 struct LoopVisitor<'hir, 'tcx> {
     cx: &'hir LateContext<'tcx>,
-    label: Option<Label>,
-    loop_depth: usize,
-    is_finite: bool,
+    // `HirId` for the entry (outer most) loop.
+    hir_id: hir::HirId,
+    block: &'hir hir::Block<'tcx>,
+    nested_loop_count: usize,
+}
+
+impl LoopVisitor<'_, '_> {
+    fn is_finite(&mut self) -> bool {
+        self.visit_block(self.block) == ControlFlow::Break(())
+    }
 }
 
 impl<'hir> Visitor<'hir> for LoopVisitor<'hir, '_> {
-    fn visit_expr(&mut self, ex: &'hir Expr<'_>) {
+    type Result = ControlFlow<()>;
+
+    fn visit_expr(&mut self, ex: &'hir Expr<'_>) -> Self::Result {
         match &ex.kind {
-            ExprKind::Break(hir::Destination { label, .. }, ..) => {
-                // Assuming breaks the loop when `loop_depth` is 0,
-                // as it could only means this `break` breaks current loop or any of its upper loop.
-                // Or, the depth is not zero but the label is matched.
-                if self.loop_depth == 0 || (label.is_some() && *label == self.label) {
-                    self.is_finite = true;
+            ExprKind::Break(hir::Destination { target_id, .. }, ..) => {
+                // When there are no nested loops and we've encountered a `break`, meaning it's either breaking the
+                // current loop or any of the parent loop, assuming that this loop is not infinite.
+                let assume_is_finite = self.nested_loop_count == 0;
+
+                if assume_is_finite || matches!(target_id, Ok(target_loop_hid) if *target_loop_hid == self.hir_id) {
+                    ControlFlow::Break(())
+                } else {
+                    ControlFlow::Continue(())
                 }
             },
-            ExprKind::Ret(..) => self.is_finite = true,
+            ExprKind::Ret(..) => ControlFlow::Break(()),
             ExprKind::Loop(..) => {
-                self.loop_depth += 1;
-                walk_expr(self, ex);
-                self.loop_depth = self.loop_depth.saturating_sub(1);
+                self.nested_loop_count += 1;
+                let cf = walk_expr(self, ex);
+                self.nested_loop_count = self.nested_loop_count.saturating_sub(1);
+                cf
             },
             _ => {
                 // Calls to a function that never return
                 if let Some(did) = fn_def_id(self.cx, ex) {
                     let fn_ret_ty = self.cx.tcx.fn_sig(did).skip_binder().output().skip_binder();
                     if fn_ret_ty.is_never() {
-                        self.is_finite = true;
-                        return;
+                        return ControlFlow::Break(());
                     }
                 }
-                walk_expr(self, ex);
+                walk_expr(self, ex)
             },
         }
     }
@@ -141,35 +162,39 @@ impl<'hir> RetTy<'hir> {
         /// given `ty` is not an `OpaqueDef`.
         fn inner_<'tcx>(cx: &LateContext<'tcx>, ty: &Ty<'tcx>) -> Option<Ty<'tcx>> {
             /// Visitor to find the type binding.
-            struct BindingVisitor<'tcx> {
-                res: Option<Ty<'tcx>>,
-            }
-            impl<'tcx> Visitor<'tcx> for BindingVisitor<'tcx> {
-                fn visit_assoc_type_binding(&mut self, type_binding: &'tcx hir::TypeBinding<'tcx>) {
-                    if self.res.is_some() {
-                        return;
-                    }
-                    if let hir::TypeBindingKind::Equality {
+            struct BindingVisitor;
+
+            impl<'tcx> Visitor<'tcx> for BindingVisitor {
+                type Result = ControlFlow<Ty<'tcx>>;
+
+                fn visit_assoc_item_constraint(
+                    &mut self,
+                    constraint: &'tcx hir::AssocItemConstraint<'tcx>,
+                ) -> Self::Result {
+                    if let hir::AssocItemConstraintKind::Equality {
                         term: hir::Term::Ty(ty),
-                    } = type_binding.kind
+                    } = constraint.kind
                     {
-                        self.res = Some(*ty);
+                        ControlFlow::Break(*ty)
+                    } else {
+                        ControlFlow::Continue(())
                     }
                 }
             }
 
-            let TyKind::OpaqueDef(item_id, ..) = ty.kind else {
-                return None;
-            };
-            let opaque_ty_item = cx.tcx.hir().item(item_id);
-
-            // Sinces the `item_id` is from a `TyKind::OpaqueDef`,
-            // therefore the `Item` related to it should always be `OpaqueTy`.
-            assert!(matches!(opaque_ty_item.kind, ItemKind::OpaqueTy(_)));
-
-            let mut vis = BindingVisitor { res: None };
-            vis.visit_item(opaque_ty_item);
-            vis.res
+            if let TyKind::OpaqueDef(item_id, ..) = ty.kind
+                && let opaque_ty_item = cx.tcx.hir().item(item_id)
+                && let ItemKind::OpaqueTy(_) = opaque_ty_item.kind
+            {
+                let mut vis = BindingVisitor;
+                // Use `vis.break_value()` once it's stablized.
+                match vis.visit_item(opaque_ty_item) {
+                    ControlFlow::Break(res) => Some(res),
+                    ControlFlow::Continue(()) => None,
+                }
+            } else {
+                None
+            }
         }
 
         match fn_ret_ty {
@@ -186,7 +211,12 @@ impl<'hir> RetTy<'hir> {
         }
     }
     fn is_never(&self) -> bool {
-        let Self::Return(ty) = self else { return false };
-        matches!(ty.kind, TyKind::Never)
+        matches!(
+            self,
+            RetTy::Return(Ty {
+                kind: TyKind::Never,
+                ..
+            })
+        )
     }
 }
diff --git a/clippy_lints/src/loops/mod.rs b/clippy_lints/src/loops/mod.rs
@@ -786,11 +786,11 @@ impl<'tcx> LateLintPass<'tcx> for Loops {
         // check for `loop { if let {} else break }` that could be `while let`
         // (also matches an explicit "match" instead of "if let")
         // (even if the "match" or "if let" is used for declaration)
-        if let ExprKind::Loop(block, label, LoopSource::Loop, _) = expr.kind {
+        if let ExprKind::Loop(block, _, LoopSource::Loop, _) = expr.kind {
             // also check for empty `loop {}` statements, skipping those in #[panic_handler]
             empty_loop::check(cx, expr, block);
             while_let_loop::check(cx, expr, block);
-            infinite_loop::check(cx, expr, block, label);
+            infinite_loop::check(cx, expr, block, expr.hir_id);
         }
 
         while_let_on_iterator::check(cx, expr);
diff --git a/tests/ui/infinite_loops.rs b/tests/ui/infinite_loops.rs
@@ -1,8 +1,9 @@
 //@no-rustfix
 //@aux-build:proc_macros.rs
 
-#![allow(clippy::never_loop)]
+#![allow(clippy::never_loop, clippy::unused_unit)]
 #![warn(clippy::infinite_loop)]
+#![feature(async_closure)]
 
 extern crate proc_macros;
 use proc_macros::{external, with_span};
@@ -390,6 +391,20 @@ fn span_inside_fn() {
     }
 }
 
+fn return_unit() -> () {
+    loop {
+        //~^ ERROR: infinite loop detected
+        do_nothing();
+    }
+
+    let async_return_unit = || -> () {
+        loop {
+            //~^ ERROR: infinite loop detected
+            do_nothing();
+        }
+    };
+}
+
 // loop in async functions
 mod issue_12338 {
     use super::do_something;
@@ -405,6 +420,26 @@ mod issue_12338 {
             do_something();
         }
     }
+    // Just to make sure checks for async block works as intended.
+    fn async_closure() {
+        let _loop_forever = async || {
+            loop {
+                //~^ ERROR: infinite loop detected
+                do_something();
+            }
+        };
+        let _somehow_ok = async || -> ! {
+            loop {
+                do_something();
+            }
+        };
+        let _ret_unit = async || -> () {
+            loop {
+                //~^ ERROR: infinite loop detected
+                do_something();
+            }
+        };
+    }
 }
 
 fn main() {}
diff --git a/tests/ui/infinite_loops.stderr b/tests/ui/infinite_loops.stderr
