Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

new lint to detect infinite loop #11829

Merged
merged 4 commits into from
Dec 12, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5147,6 +5147,7 @@ Released 2018-09-13
[`inefficient_to_string`]: https://rust-lang.github.io/rust-clippy/master/index.html#inefficient_to_string
[`infallible_destructuring_match`]: https://rust-lang.github.io/rust-clippy/master/index.html#infallible_destructuring_match
[`infinite_iter`]: https://rust-lang.github.io/rust-clippy/master/index.html#infinite_iter
[`infinite_loop`]: https://rust-lang.github.io/rust-clippy/master/index.html#infinite_loop
[`inherent_to_string`]: https://rust-lang.github.io/rust-clippy/master/index.html#inherent_to_string
[`inherent_to_string_shadow_display`]: https://rust-lang.github.io/rust-clippy/master/index.html#inherent_to_string_shadow_display
[`init_numbered_fields`]: https://rust-lang.github.io/rust-clippy/master/index.html#init_numbered_fields
Expand Down
1 change: 1 addition & 0 deletions clippy_lints/src/declared_lints.rs
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ pub(crate) static LINTS: &[&crate::LintInfo] = &[
crate::loops::EXPLICIT_INTO_ITER_LOOP_INFO,
crate::loops::EXPLICIT_ITER_LOOP_INFO,
crate::loops::FOR_KV_MAP_INFO,
crate::loops::INFINITE_LOOP_INFO,
crate::loops::ITER_NEXT_LOOP_INFO,
crate::loops::MANUAL_FIND_INFO,
crate::loops::MANUAL_FLATTEN_INFO,
Expand Down
125 changes: 125 additions & 0 deletions clippy_lints/src/loops/infinite_loop.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,125 @@
use clippy_utils::diagnostics::span_lint_and_then;
use clippy_utils::{fn_def_id, is_lint_allowed};
use hir::intravisit::{walk_expr, Visitor};
use hir::{Expr, ExprKind, FnRetTy, FnSig, Node};
use rustc_ast::Label;
use rustc_errors::Applicability;
use rustc_hir as hir;
use rustc_lint::LateContext;

use super::INFINITE_LOOP;

pub(super) fn check<'tcx>(
cx: &LateContext<'tcx>,
expr: &Expr<'_>,
loop_block: &'tcx hir::Block<'_>,
label: Option<Label>,
) {
if is_lint_allowed(cx, INFINITE_LOOP, expr.hir_id) {
return;
}

// Skip check if this loop is not in a function/method/closure. (In some weird case)
let Some(parent_fn_ret) = get_parent_fn_ret_ty(cx, expr) else {
return;
};
// Or, its parent function is already returning `Never`
if matches!(
parent_fn_ret,
FnRetTy::Return(hir::Ty {
kind: hir::TyKind::Never,
..
})
) {
return;
}

let mut loop_visitor = LoopVisitor {
cx,
label,
is_finite: false,
loop_depth: 0,
};
loop_visitor.visit_block(loop_block);

let is_finite_loop = loop_visitor.is_finite;

if !is_finite_loop {
span_lint_and_then(cx, INFINITE_LOOP, expr.span, "infinite loop detected", |diag| {
if let FnRetTy::DefaultReturn(ret_span) = parent_fn_ret {
diag.span_suggestion(
ret_span,
"if this is intentional, consider specifing `!` as function return",
" -> !",
Applicability::MaybeIncorrect,
);
} else {
diag.help("if this is not intended, try adding a `break` or `return` condition in the loop");
}
});
}
}

fn get_parent_fn_ret_ty<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) -> Option<FnRetTy<'tcx>> {
for (_, parent_node) in cx.tcx.hir().parent_iter(expr.hir_id) {
match parent_node {
Node::Item(hir::Item {
kind: hir::ItemKind::Fn(FnSig { decl, .. }, _, _),
..
})
| Node::TraitItem(hir::TraitItem {
kind: hir::TraitItemKind::Fn(FnSig { decl, .. }, _),
..
})
| Node::ImplItem(hir::ImplItem {
kind: hir::ImplItemKind::Fn(FnSig { decl, .. }, _),
..
})
| Node::Expr(Expr {
kind: ExprKind::Closure(hir::Closure { fn_decl: decl, .. }),
..
}) => return Some(decl.output),
_ => (),
}
}
None
}

struct LoopVisitor<'hir, 'tcx> {
cx: &'hir LateContext<'tcx>,
label: Option<Label>,
loop_depth: usize,
is_finite: bool,
}

impl<'hir> Visitor<'hir> for LoopVisitor<'hir, '_> {
fn visit_expr(&mut self, ex: &'hir Expr<'_>) {
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::Ret(..) => self.is_finite = true,
ExprKind::Loop(..) => {
self.loop_depth += 1;
walk_expr(self, ex);
self.loop_depth = self.loop_depth.saturating_sub(1);
},
_ => {
// 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;
}
}
walk_expr(self, ex);
},
}
}
}
47 changes: 46 additions & 1 deletion clippy_lints/src/loops/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ mod explicit_counter_loop;
mod explicit_into_iter_loop;
mod explicit_iter_loop;
mod for_kv_map;
mod infinite_loop;
mod iter_next_loop;
mod manual_find;
mod manual_flatten;
Expand Down Expand Up @@ -635,6 +636,48 @@ declare_clippy_lint! {
"checking for emptiness of a `Vec` in the loop condition and popping an element in the body"
}

declare_clippy_lint! {
/// ### What it does
/// Checks for infinite loops in a function where the return type is not `!`
/// and lint accordingly.
///
/// ### Why is this bad?
/// A loop should be gently exited somewhere, or at least mark its parent function as
/// never return (`!`).
///
/// ### Example
/// ```no_run,ignore
/// fn run_forever() {
/// loop {
/// // do something
/// }
/// }
/// ```
/// If infinite loops are as intended:
/// ```no_run,ignore
/// fn run_forever() -> ! {
/// loop {
/// // do something
/// }
/// }
/// ```
/// Otherwise add a `break` or `return` condition:
/// ```no_run,ignore
/// fn run_forever() {
/// loop {
/// // do something
/// if condition {
/// break;
/// }
/// }
/// }
/// ```
#[clippy::version = "1.75.0"]
pub INFINITE_LOOP,
restriction,
"possibly unintended infinite loop"
}

pub struct Loops {
msrv: Msrv,
enforce_iter_loop_reborrow: bool,
Expand Down Expand Up @@ -669,6 +712,7 @@ impl_lint_pass!(Loops => [
MANUAL_FIND,
MANUAL_WHILE_LET_SOME,
UNUSED_ENUMERATE_INDEX,
INFINITE_LOOP,
]);

impl<'tcx> LateLintPass<'tcx> for Loops {
Expand Down Expand Up @@ -707,10 +751,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, _, LoopSource::Loop, _) = expr.kind {
if let ExprKind::Loop(block, label, 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);
}

while_let_on_iterator::check(cx, expr);
Expand Down
Loading