diff options
| author | J-ZhengLi <lizheng135@huawei.com> | 2023-11-17 18:10:50 +0800 |
|---|---|---|
| committer | J-ZhengLi <lizheng135@huawei.com> | 2023-11-17 18:10:50 +0800 |
| commit | 2d9fc6dfc8160f47f13da9be3fb5fc48ef62880f (patch) | |
| tree | 3d57fc75f22459f2afdef2e2f4fc8ff65793ba90 | |
| parent | 406d953820f48947216d6f3a31bef690d5798dca (diff) | |
| download | rust-2d9fc6dfc8160f47f13da9be3fb5fc48ef62880f.tar.gz rust-2d9fc6dfc8160f47f13da9be3fb5fc48ef62880f.zip | |
implement unoptimized code logic for [`infinite_loops`]
| -rw-r--r-- | CHANGELOG.md | 1 | ||||
| -rw-r--r-- | clippy_lints/src/declared_lints.rs | 1 | ||||
| -rw-r--r-- | clippy_lints/src/loops/infinite_loops.rs | 92 | ||||
| -rw-r--r-- | clippy_lints/src/loops/mod.rs | 79 | ||||
| -rw-r--r-- | tests/ui/infinite_loops.rs | 259 | ||||
| -rw-r--r-- | tests/ui/infinite_loops.stderr | 147 |
6 files changed, 571 insertions, 8 deletions
diff --git a/CHANGELOG.md b/CHANGELOG.md index e74df808e06..5c62cf51559 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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_loops`]: https://rust-lang.github.io/rust-clippy/master/index.html#infinite_loops [`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 diff --git a/clippy_lints/src/declared_lints.rs b/clippy_lints/src/declared_lints.rs index 85854a0dfb7..7520b57891e 100644 --- a/clippy_lints/src/declared_lints.rs +++ b/clippy_lints/src/declared_lints.rs @@ -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_LOOPS_INFO, crate::loops::ITER_NEXT_LOOP_INFO, crate::loops::MANUAL_FIND_INFO, crate::loops::MANUAL_FLATTEN_INFO, diff --git a/clippy_lints/src/loops/infinite_loops.rs b/clippy_lints/src/loops/infinite_loops.rs new file mode 100644 index 00000000000..b97cd8bd006 --- /dev/null +++ b/clippy_lints/src/loops/infinite_loops.rs @@ -0,0 +1,92 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::is_lint_allowed; +use hir::intravisit::{walk_expr, Visitor}; +use hir::{Block, Destination, Expr, ExprKind, FnRetTy, Ty, TyKind}; +use rustc_ast::Label; +use rustc_errors::Applicability; +use rustc_hir as hir; +use rustc_lint::LateContext; + +use super::INFINITE_LOOPS; + +pub(super) fn check( + cx: &LateContext<'_>, + expr: &Expr<'_>, + loop_block: &Block<'_>, + label: Option<Label>, + parent_fn_ret_ty: FnRetTy<'_>, +) { + if is_lint_allowed(cx, INFINITE_LOOPS, expr.hir_id) + || matches!( + parent_fn_ret_ty, + FnRetTy::Return(Ty { + kind: TyKind::Never, + .. + }) + ) + { + return; + } + + // First, find any `break` or `return` without entering any inner loop, + // then, find `return` or labeled `break` which breaks this loop with entering inner loop, + // otherwise this loop is a infinite loop. + let mut direct_br_or_ret_finder = BreakOrRetFinder::default(); + direct_br_or_ret_finder.visit_block(loop_block); + + let is_finite_loop = direct_br_or_ret_finder.found || { + let mut inner_br_or_ret_finder = BreakOrRetFinder { + label, + enter_nested_loop: true, + ..Default::default() + }; + inner_br_or_ret_finder.visit_block(loop_block); + inner_br_or_ret_finder.found + }; + + if !is_finite_loop { + span_lint_and_then(cx, INFINITE_LOOPS, expr.span, "infinite loop detected", |diag| { + if let FnRetTy::DefaultReturn(ret_span) = parent_fn_ret_ty { + diag.span_suggestion( + ret_span, + "if this is intentional, consider specifing `!` as function return", + " -> !", + Applicability::MaybeIncorrect, + ); + } else { + diag.span_help( + expr.span, + "if this is not intended, add a `break` or `return` condition in this loop", + ); + } + }); + } +} + +#[derive(Default)] +struct BreakOrRetFinder { + label: Option<Label>, + found: bool, + enter_nested_loop: bool, +} + +impl<'hir> Visitor<'hir> for BreakOrRetFinder { + fn visit_expr(&mut self, ex: &'hir Expr<'_>) { + match &ex.kind { + ExprKind::Break(Destination { label, .. }, ..) => { + // When entering nested loop, only by breaking this loop's label + // would be considered as exiting this loop. + if self.enter_nested_loop { + if label.is_some() && *label == self.label { + self.found = true; + } + } else { + self.found = true; + } + }, + ExprKind::Ret(..) => self.found = true, + ExprKind::Loop(..) if !self.enter_nested_loop => (), + _ => walk_expr(self, ex), + } + } +} diff --git a/clippy_lints/src/loops/mod.rs b/clippy_lints/src/loops/mod.rs index b99e0fd814f..c69d0b68cf4 100644 --- a/clippy_lints/src/loops/mod.rs +++ b/clippy_lints/src/loops/mod.rs @@ -3,6 +3,7 @@ mod explicit_counter_loop; mod explicit_into_iter_loop; mod explicit_iter_loop; mod for_kv_map; +mod infinite_loops; mod iter_next_loop; mod manual_find; mod manual_flatten; @@ -22,7 +23,7 @@ mod while_let_on_iterator; use clippy_config::msrvs::Msrv; use clippy_utils::higher; -use rustc_hir::{Expr, ExprKind, LoopSource, Pat}; +use rustc_hir::{self as hir, Expr, ExprKind, LoopSource, Pat}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::{declare_tool_lint, impl_lint_pass}; use rustc_span::source_map::Span; @@ -635,20 +636,64 @@ declare_clippy_lint! { "checking for emptiness of a `Vec` in the loop condition and popping an element in the body" } -pub struct Loops { +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 lease 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_LOOPS, + restriction, + "possibly unintended infinite loops" +} + +pub struct Loops<'tcx> { msrv: Msrv, enforce_iter_loop_reborrow: bool, + parent_fn_ret_ty: Option<hir::FnRetTy<'tcx>>, } -impl Loops { +impl<'tcx> Loops<'tcx> { pub fn new(msrv: Msrv, enforce_iter_loop_reborrow: bool) -> Self { Self { msrv, enforce_iter_loop_reborrow, + parent_fn_ret_ty: None, } } } -impl_lint_pass!(Loops => [ +impl_lint_pass!(Loops<'_> => [ MANUAL_MEMCPY, MANUAL_FLATTEN, NEEDLESS_RANGE_LOOP, @@ -669,9 +714,10 @@ impl_lint_pass!(Loops => [ MANUAL_FIND, MANUAL_WHILE_LET_SOME, UNUSED_ENUMERATE_INDEX, + INFINITE_LOOPS, ]); -impl<'tcx> LateLintPass<'tcx> for Loops { +impl<'tcx> LateLintPass<'tcx> for Loops<'tcx> { fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { let for_loop = higher::ForLoop::hir(expr); if let Some(higher::ForLoop { @@ -707,10 +753,13 @@ 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); + if let Some(parent_fn_ret_ty) = self.parent_fn_ret_ty { + infinite_loops::check(cx, expr, block, label, parent_fn_ret_ty); + } } while_let_on_iterator::check(cx, expr); @@ -722,11 +771,25 @@ impl<'tcx> LateLintPass<'tcx> for Loops { } } + fn check_fn( + &mut self, + _: &LateContext<'tcx>, + kind: hir::intravisit::FnKind<'tcx>, + decl: &'tcx hir::FnDecl<'tcx>, + _: &'tcx hir::Body<'tcx>, + _: Span, + _: rustc_span::def_id::LocalDefId, + ) { + if let hir::intravisit::FnKind::ItemFn(..) = kind { + self.parent_fn_ret_ty = Some(decl.output); + } + } + extract_msrv_attr!(LateContext); } -impl Loops { - fn check_for_loop<'tcx>( +impl<'tcx> Loops<'tcx> { + fn check_for_loop( &self, cx: &LateContext<'tcx>, pat: &'tcx Pat<'_>, diff --git a/tests/ui/infinite_loops.rs b/tests/ui/infinite_loops.rs new file mode 100644 index 00000000000..54ab63d80dd --- /dev/null +++ b/tests/ui/infinite_loops.rs @@ -0,0 +1,259 @@ +//@no-rustfix +#![allow(clippy::never_loop)] +#![warn(clippy::infinite_loops)] + +fn do_something() {} + +fn no_break() { + loop { + //~^ ERROR: infinite loop detected + do_something(); + } +} + +fn no_break_never_ret() -> ! { + loop { + do_something(); + } +} + +fn no_break_never_ret_noise() { + loop { + fn inner_fn() -> ! { + std::process::exit(0); + } + do_something(); + } +} + +fn has_direct_break_1() { + loop { + do_something(); + break; + } +} + +fn has_direct_break_2() { + 'outer: loop { + do_something(); + break 'outer; + } +} + +fn has_indirect_break_1(cond: bool) { + 'outer: loop { + loop { + if cond { + break 'outer; + } + } + } +} + +fn has_indirect_break_2(stop_num: i32) { + 'outer: loop { + for x in 0..5 { + if x == stop_num { + break 'outer; + } + } + } +} + +fn break_inner_but_not_outer_1(cond: bool) { + loop { + //~^ ERROR: infinite loop detected + loop { + if cond { + break; + } + } + } +} + +fn break_inner_but_not_outer_2(cond: bool) { + loop { + //~^ ERROR: infinite loop detected + 'inner: loop { + loop { + if cond { + break 'inner; + } + } + } + } +} + +fn break_outer_but_not_inner() { + loop { + loop { + //~^ ERROR: infinite loop detected + do_something(); + } + break; + } +} + +fn can_break_both_inner_and_outer(cond: bool) { + 'outer: loop { + loop { + if cond { + break 'outer; + } else { + break; + } + } + } +} + +fn break_wrong_loop(cond: bool) { + // 'inner has statement to break 'outer loop, but it was breaked early by a labeled child loop + 'outer: loop { + loop { + //~^ ERROR: infinite loop detected + 'inner: loop { + loop { + loop { + break 'inner; + } + break 'outer; + } + } + } + } +} + +fn has_direct_return(cond: bool) { + loop { + if cond { + return; + } + } +} + +fn ret_in_inner(cond: bool) { + loop { + loop { + if cond { + return; + } + } + } +} + +enum Foo { + A, + B, + C, +} + +fn match_like() { + let opt: Option<u8> = Some(1); + loop { + //~^ ERROR: infinite loop detected + match opt { + Some(v) => { + println!("{v}"); + }, + None => { + do_something(); + }, + } + } + + loop { + match opt { + Some(v) => { + println!("{v}"); + }, + None => { + do_something(); + break; + }, + } + } + + let result: Result<u8, u16> = Ok(1); + loop { + let _val = match result { + Ok(1) => 1 + 1, + Ok(v) => v / 2, + Err(_) => return, + }; + } + + loop { + let Ok(_val) = result else { return }; + } + + loop { + let Ok(_val) = result.map(|v| 10) else { break }; + } + + loop { + //~^ ERROR: infinite loop detected + let _x = matches!(result, Ok(v) if v != 0).then_some(0); + } + + loop { + //~^ ERROR: infinite loop detected + // This `return` does not return the function, so it doesn't count + let _x = matches!(result, Ok(v) if v != 0).then(|| { + if true { + return; + } + do_something(); + }); + } + + let mut val = 0; + let mut fooc = Foo::C; + + loop { + val = match fooc { + Foo::A => 0, + Foo::B => { + fooc = Foo::C; + 1 + }, + Foo::C => break, + }; + } + + loop { + val = match fooc { + Foo::A => 0, + Foo::B => 1, + Foo::C => { + break; + }, + }; + } +} + +macro_rules! set_or_ret { + ($opt:expr, $a:expr) => {{ + match $opt { + Some(val) => $a = val, + None => return, + } + }}; +} + +fn ret_in_macro(opt: Option<u8>) { + let opt: Option<u8> = Some(1); + let mut a: u8 = 0; + loop { + set_or_ret!(opt, a); + } + + let res: Result<bool, u8> = Ok(true); + loop { + match res { + Ok(true) => set_or_ret!(opt, a), + _ => do_something(), + } + } +} + +fn main() {} diff --git a/tests/ui/infinite_loops.stderr b/tests/ui/infinite_loops.stderr new file mode 100644 index 00000000000..f6bca43f1a4 --- /dev/null +++ b/tests/ui/infinite_loops.stderr @@ -0,0 +1,147 @@ +error: infinite loop detected + --> $DIR/infinite_loops.rs:8:5 + | +LL | / loop { +LL | | +LL | | do_something(); +LL | | } + | |_____^ + | + = note: `-D clippy::infinite-loops` implied by `-D warnings` + = help: to override `-D warnings` add `#[allow(clippy::infinite_loops)]` +help: if this is intentional, consider specifing `!` as function return + | +LL | fn no_break() -> ! { + | ++++ + +error: infinite loop detected + --> $DIR/infinite_loops.rs:21:5 + | +LL | / loop { +LL | | fn inner_fn() -> ! { +LL | | std::process::exit(0); +LL | | } +LL | | do_something(); +LL | | } + | |_____^ + | +help: if this is intentional, consider specifing `!` as function return + | +LL | fn no_break_never_ret_noise() -> ! { + | ++++ + +error: infinite loop detected + --> $DIR/infinite_loops.rs:64:5 + | +LL | / loop { +LL | | +LL | | loop { +LL | | if cond { +... | +LL | | } +LL | | } + | |_____^ + | +help: if this is intentional, consider specifing `!` as function return + | +LL | fn break_inner_but_not_outer_1(cond: bool) -> ! { + | ++++ + +error: infinite loop detected + --> $DIR/infinite_loops.rs:75:5 + | +LL | / loop { +LL | | +LL | | 'inner: loop { +LL | | loop { +... | +LL | | } +LL | | } + | |_____^ + | +help: if this is intentional, consider specifing `!` as function return + | +LL | fn break_inner_but_not_outer_2(cond: bool) -> ! { + | ++++ + +error: infinite loop detected + --> $DIR/infinite_loops.rs:89:9 + | +LL | / loop { +LL | | +LL | | do_something(); +LL | | } + | |_________^ + | +help: if this is intentional, consider specifing `!` as function return + | +LL | fn break_outer_but_not_inner() -> ! { + | ++++ + +error: infinite loop detected + --> $DIR/infinite_loops.rs:112:9 + | +LL | / loop { +LL | | +LL | | 'inner: loop { +LL | | loop { +... | +LL | | } +LL | | } + | |_________^ + | +help: if this is intentional, consider specifing `!` as function return + | +LL | fn break_wrong_loop(cond: bool) -> ! { + | ++++ + +error: infinite loop detected + --> $DIR/infinite_loops.rs:152:5 + | +LL | / loop { +LL | | +LL | | match opt { +LL | | Some(v) => { +... | +LL | | } +LL | | } + | |_____^ + | +help: if this is intentional, consider specifing `!` as function return + | +LL | fn match_like() -> ! { + | ++++ + +error: infinite loop detected + --> $DIR/infinite_loops.rs:193:5 + | +LL | / loop { +LL | | +LL | | let _x = matches!(result, Ok(v) if v != 0).then_some(0); +LL | | } + | |_____^ + | +help: if this is intentional, consider specifing `!` as function return + | +LL | fn match_like() -> ! { + | ++++ + +error: infinite loop detected + --> $DIR/infinite_loops.rs:198:5 + | +LL | / loop { +LL | | +LL | | // This `return` does not return the function, so it doesn't count +LL | | let _x = matches!(result, Ok(v) if v != 0).then(|| { +... | +LL | | }); +LL | | } + | |_____^ + | +help: if this is intentional, consider specifing `!` as function return + | +LL | fn match_like() -> ! { + | ++++ + +error: aborting due to 9 previous errors + |
