Skip to content

Commit 3e9a6d1

Browse files
J-ZhengLiJ-ZhengLi
committed
stop warning never-returning calls
and add more test cases
1 parent 2d9fc6d commit 3e9a6d1

File tree

4 files changed

+306
-68
lines changed

4 files changed

+306
-68
lines changed

‎clippy_lints/src/loops/infinite_loops.rs

+80-33
Original file line numberDiff line numberDiff line change
@@ -1,52 +1,64 @@
11
use clippy_utils::diagnostics::span_lint_and_then;
2-
use clippy_utils::is_lint_allowed;
2+
use clippy_utils::{fn_def_id, is_lint_allowed};
33
use hir::intravisit::{walk_expr, Visitor};
4-
use hir::{Block, Destination, Expr, ExprKind, FnRetTy, Ty, TyKind};
4+
use hir::{Expr, ExprKind, FnRetTy, FnSig, Node};
55
use rustc_ast::Label;
66
use rustc_errors::Applicability;
77
use rustc_hir as hir;
88
use rustc_lint::LateContext;
99

1010
use super::INFINITE_LOOPS;
1111

12-
pub(super) fn check(
13-
cx: &LateContext<'_>,
12+
pub(super) fn check<'tcx>(
13+
cx: &LateContext<'tcx>,
1414
expr: &Expr<'_>,
15-
loop_block: &Block<'_>,
15+
loop_block: &'tcx hir::Block<'_>,
1616
label: Option<Label>,
17-
parent_fn_ret_ty: FnRetTy<'_>,
1817
) {
19-
if is_lint_allowed(cx, INFINITE_LOOPS, expr.hir_id)
20-
|| matches!(
21-
parent_fn_ret_ty,
22-
FnRetTy::Return(Ty {
23-
kind: TyKind::Never,
24-
..
25-
})
26-
)
27-
{
18+
if is_lint_allowed(cx, INFINITE_LOOPS, expr.hir_id) {
19+
return;
20+
}
21+
22+
// Skip check if this loop is not in a function/method/closure. (In some weird case)
23+
let Some(parent_fn_ret) = get_parent_fn_ret_ty(cx, expr) else {
24+
return;
25+
};
26+
// Or, its parent function is already returning `Never`
27+
if matches!(
28+
parent_fn_ret,
29+
FnRetTy::Return(hir::Ty {
30+
kind: hir::TyKind::Never,
31+
..
32+
})
33+
) {
2834
return;
2935
}
3036

3137
// First, find any `break` or `return` without entering any inner loop,
3238
// then, find `return` or labeled `break` which breaks this loop with entering inner loop,
3339
// otherwise this loop is a infinite loop.
34-
let mut direct_br_or_ret_finder = BreakOrRetFinder::default();
35-
direct_br_or_ret_finder.visit_block(loop_block);
40+
let mut direct_visitor = LoopVisitor {
41+
cx,
42+
label,
43+
is_finite: false,
44+
enter_nested_loop: false,
45+
};
46+
direct_visitor.visit_block(loop_block);
3647

37-
let is_finite_loop = direct_br_or_ret_finder.found || {
38-
let mut inner_br_or_ret_finder = BreakOrRetFinder {
48+
let is_finite_loop = direct_visitor.is_finite || {
49+
let mut inner_loop_visitor = LoopVisitor {
50+
cx,
3951
label,
52+
is_finite: false,
4053
enter_nested_loop: true,
41-
..Default::default()
4254
};
43-
inner_br_or_ret_finder.visit_block(loop_block);
44-
inner_br_or_ret_finder.found
55+
inner_loop_visitor.visit_block(loop_block);
56+
inner_loop_visitor.is_finite
4557
};
4658

4759
if !is_finite_loop {
4860
span_lint_and_then(cx, INFINITE_LOOPS, expr.span, "infinite loop detected", |diag| {
49-
if let FnRetTy::DefaultReturn(ret_span) = parent_fn_ret_ty {
61+
if let FnRetTy::DefaultReturn(ret_span) = parent_fn_ret {
5062
diag.span_suggestion(
5163
ret_span,
5264
"if this is intentional, consider specifing `!` as function return",
@@ -56,37 +68,72 @@ pub(super) fn check(
5668
} else {
5769
diag.span_help(
5870
expr.span,
59-
"if this is not intended, add a `break` or `return` condition in this loop",
71+
"if this is not intended, try adding a `break` or `return` condition in this loop",
6072
);
6173
}
6274
});
6375
}
6476
}
6577

66-
#[derive(Default)]
67-
struct BreakOrRetFinder {
78+
fn get_parent_fn_ret_ty<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) -> Option<FnRetTy<'tcx>> {
79+
for (_, parent_node) in cx.tcx.hir().parent_iter(expr.hir_id) {
80+
match parent_node {
81+
Node::Item(hir::Item {
82+
kind: hir::ItemKind::Fn(FnSig { decl, .. }, _, _),
83+
..
84+
})
85+
| Node::TraitItem(hir::TraitItem {
86+
kind: hir::TraitItemKind::Fn(FnSig { decl, .. }, _),
87+
..
88+
})
89+
| Node::ImplItem(hir::ImplItem {
90+
kind: hir::ImplItemKind::Fn(FnSig { decl, .. }, _),
91+
..
92+
})
93+
| Node::Expr(Expr {
94+
kind: ExprKind::Closure(hir::Closure { fn_decl: decl, .. }),
95+
..
96+
}) => return Some(decl.output),
97+
_ => (),
98+
}
99+
}
100+
None
101+
}
102+
103+
struct LoopVisitor<'hir, 'tcx> {
104+
cx: &'hir LateContext<'tcx>,
68105
label: Option<Label>,
69-
found: bool,
106+
is_finite: bool,
70107
enter_nested_loop: bool,
71108
}
72109

73-
impl<'hir> Visitor<'hir> for BreakOrRetFinder {
110+
impl<'hir> Visitor<'hir> for LoopVisitor<'hir, '_> {
74111
fn visit_expr(&mut self, ex: &'hir Expr<'_>) {
75112
match &ex.kind {
76-
ExprKind::Break(Destination { label, .. }, ..) => {
113+
ExprKind::Break(hir::Destination { label, .. }, ..) => {
77114
// When entering nested loop, only by breaking this loop's label
78115
// would be considered as exiting this loop.
79116
if self.enter_nested_loop {
80117
if label.is_some() && *label == self.label {
81-
self.found = true;
118+
self.is_finite = true;
82119
}
83120
} else {
84-
self.found = true;
121+
self.is_finite = true;
85122
}
86123
},
87-
ExprKind::Ret(..) => self.found = true,
124+
ExprKind::Ret(..) => self.is_finite = true,
88125
ExprKind::Loop(..) if !self.enter_nested_loop => (),
89-
_ => walk_expr(self, ex),
126+
_ => {
127+
// Calls to a function that never return
128+
if let Some(did) = fn_def_id(self.cx, ex) {
129+
let fn_ret_ty = self.cx.tcx.fn_sig(did).skip_binder().output().skip_binder();
130+
if fn_ret_ty.is_never() {
131+
self.is_finite = true;
132+
return;
133+
}
134+
}
135+
walk_expr(self, ex);
136+
},
90137
}
91138
}
92139
}

‎clippy_lints/src/loops/mod.rs

+8-26
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ mod while_let_on_iterator;
2323

2424
use clippy_config::msrvs::Msrv;
2525
use clippy_utils::higher;
26-
use rustc_hir::{self as hir, Expr, ExprKind, LoopSource, Pat};
26+
use rustc_hir::{Expr, ExprKind, LoopSource, Pat};
2727
use rustc_lint::{LateContext, LateLintPass};
2828
use rustc_session::{declare_tool_lint, impl_lint_pass};
2929
use rustc_span::source_map::Span;
@@ -678,22 +678,20 @@ declare_clippy_lint! {
678678
"possibly unintended infinite loops"
679679
}
680680

681-
pub struct Loops<'tcx> {
681+
pub struct Loops {
682682
msrv: Msrv,
683683
enforce_iter_loop_reborrow: bool,
684-
parent_fn_ret_ty: Option<hir::FnRetTy<'tcx>>,
685684
}
686-
impl<'tcx> Loops<'tcx> {
685+
impl Loops {
687686
pub fn new(msrv: Msrv, enforce_iter_loop_reborrow: bool) -> Self {
688687
Self {
689688
msrv,
690689
enforce_iter_loop_reborrow,
691-
parent_fn_ret_ty: None,
692690
}
693691
}
694692
}
695693

696-
impl_lint_pass!(Loops<'_> => [
694+
impl_lint_pass!(Loops => [
697695
MANUAL_MEMCPY,
698696
MANUAL_FLATTEN,
699697
NEEDLESS_RANGE_LOOP,
@@ -717,7 +715,7 @@ impl_lint_pass!(Loops<'_> => [
717715
INFINITE_LOOPS,
718716
]);
719717

720-
impl<'tcx> LateLintPass<'tcx> for Loops<'tcx> {
718+
impl<'tcx> LateLintPass<'tcx> for Loops {
721719
fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
722720
let for_loop = higher::ForLoop::hir(expr);
723721
if let Some(higher::ForLoop {
@@ -757,9 +755,7 @@ impl<'tcx> LateLintPass<'tcx> for Loops<'tcx> {
757755
// also check for empty `loop {}` statements, skipping those in #[panic_handler]
758756
empty_loop::check(cx, expr, block);
759757
while_let_loop::check(cx, expr, block);
760-
if let Some(parent_fn_ret_ty) = self.parent_fn_ret_ty {
761-
infinite_loops::check(cx, expr, block, label, parent_fn_ret_ty);
762-
}
758+
infinite_loops::check(cx, expr, block, label);
763759
}
764760

765761
while_let_on_iterator::check(cx, expr);
@@ -771,25 +767,11 @@ impl<'tcx> LateLintPass<'tcx> for Loops<'tcx> {
771767
}
772768
}
773769

774-
fn check_fn(
775-
&mut self,
776-
_: &LateContext<'tcx>,
777-
kind: hir::intravisit::FnKind<'tcx>,
778-
decl: &'tcx hir::FnDecl<'tcx>,
779-
_: &'tcx hir::Body<'tcx>,
780-
_: Span,
781-
_: rustc_span::def_id::LocalDefId,
782-
) {
783-
if let hir::intravisit::FnKind::ItemFn(..) = kind {
784-
self.parent_fn_ret_ty = Some(decl.output);
785-
}
786-
}
787-
788770
extract_msrv_attr!(LateContext);
789771
}
790772

791-
impl<'tcx> Loops<'tcx> {
792-
fn check_for_loop(
773+
impl Loops {
774+
fn check_for_loop<'tcx>(
793775
&self,
794776
cx: &LateContext<'tcx>,
795777
pat: &'tcx Pat<'_>,

‎tests/ui/infinite_loops.rs

+101
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,31 @@ fn no_break() {
1111
}
1212
}
1313

14+
fn all_inf() {
15+
loop {
16+
//~^ ERROR: infinite loop detected
17+
loop {
18+
//~^ ERROR: infinite loop detected
19+
loop {
20+
//~^ ERROR: infinite loop detected
21+
do_something();
22+
}
23+
}
24+
do_something();
25+
}
26+
}
27+
28+
fn no_break_return_some_ty() -> Option<u8> {
29+
loop {
30+
do_something();
31+
return None;
32+
}
33+
loop {
34+
//~^ ERROR: infinite loop detected
35+
do_something();
36+
}
37+
}
38+
1439
fn no_break_never_ret() -> ! {
1540
loop {
1641
do_something();
@@ -256,4 +281,80 @@ fn ret_in_macro(opt: Option<u8>) {
256281
}
257282
}
258283

284+
fn panic_like_macros_1() {
285+
loop {
286+
do_something();
287+
panic!();
288+
}
289+
}
290+
291+
fn panic_like_macros_2() {
292+
let mut x = 0;
293+
294+
loop {
295+
do_something();
296+
if true {
297+
todo!();
298+
}
299+
}
300+
loop {
301+
do_something();
302+
x += 1;
303+
assert_eq!(x, 0);
304+
}
305+
loop {
306+
do_something();
307+
assert!(x % 2 == 0);
308+
}
309+
loop {
310+
do_something();
311+
match Some(1) {
312+
Some(n) => println!("{n}"),
313+
None => unreachable!("It won't happen"),
314+
}
315+
}
316+
}
317+
318+
fn exit_directly(cond: bool) {
319+
loop {
320+
if cond {
321+
std::process::exit(0);
322+
}
323+
}
324+
}
325+
326+
trait MyTrait {
327+
fn problematic_trait_method() {
328+
loop {
329+
//~^ ERROR: infinite loop detected
330+
do_something();
331+
}
332+
}
333+
fn could_be_problematic();
334+
}
335+
336+
impl MyTrait for String {
337+
fn could_be_problematic() {
338+
loop {
339+
//~^ ERROR: infinite loop detected
340+
do_something();
341+
}
342+
}
343+
}
344+
345+
fn inf_loop_in_closure() {
346+
let _loop_forever = || {
347+
loop {
348+
//~^ ERROR: infinite loop detected
349+
do_something();
350+
}
351+
};
352+
353+
let _somehow_ok = || -> ! {
354+
loop {
355+
do_something();
356+
}
357+
};
358+
}
359+
259360
fn main() {}

0 commit comments

Comments
 (0)