From 13acca755ab301fb77ffecfe9155c7ca17ae8f47 Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Wed, 6 Mar 2024 01:10:46 +0800 Subject: [PATCH 1/3] [`infinite_loops`]: fix suggestion on async functions --- clippy_lints/src/loops/infinite_loop.rs | 98 ++++++++++++++++++++----- tests/ui/infinite_loops.rs | 17 +++++ tests/ui/infinite_loops.stderr | 16 +++- 3 files changed, 112 insertions(+), 19 deletions(-) diff --git a/clippy_lints/src/loops/infinite_loop.rs b/clippy_lints/src/loops/infinite_loop.rs index 5b5bb88c1790..4134944178d9 100644 --- a/clippy_lints/src/loops/infinite_loop.rs +++ b/clippy_lints/src/loops/infinite_loop.rs @@ -1,12 +1,13 @@ 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::{Expr, ExprKind, FnRetTy, FnSig, Node}; +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}; use rustc_middle::lint::in_external_macro; +use rustc_span::Span; use super::INFINITE_LOOP; @@ -24,18 +25,7 @@ pub(super) fn check<'tcx>( 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; - } - - if in_external_macro(cx.sess(), expr.span) || is_from_proc_macro(cx, expr) { + if parent_fn_ret.is_never() || in_external_macro(cx.sess(), expr.span) || is_from_proc_macro(cx, expr) { return; } @@ -51,9 +41,9 @@ pub(super) fn check<'tcx>( 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 { + if let Some(span) = parent_fn_ret.sugg_span() { diag.span_suggestion( - ret_span, + span, "if this is intentional, consider specifying `!` as function return", " -> !", Applicability::MaybeIncorrect, @@ -65,11 +55,21 @@ pub(super) fn check<'tcx>( } } -fn get_parent_fn_ret_ty<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) -> Option> { +fn get_parent_fn_ret_ty<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) -> Option> { for (_, parent_node) in cx.tcx.hir().parent_iter(expr.hir_id) { match parent_node { + // Skip `Coroutine`, these are the body of `async fn`, not the async closures. + // This is because we still need to backtrack one parent node to get the `OpaqueDef` ty. + Node::Expr(Expr { + kind: + ExprKind::Closure(hir::Closure { + kind: ClosureKind::Coroutine(_), + .. + }), + .. + }) => (), Node::Item(hir::Item { - kind: hir::ItemKind::Fn(FnSig { decl, .. }, _, _), + kind: ItemKind::Fn(FnSig { decl, .. }, _, _), .. }) | Node::TraitItem(hir::TraitItem { @@ -83,7 +83,7 @@ fn get_parent_fn_ret_ty<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>) -> Option | Node::Expr(Expr { kind: ExprKind::Closure(hir::Closure { fn_decl: decl, .. }), .. - }) => return Some(decl.output), + }) => return Some(RetTy::from_fn_ret_ty(cx, decl.output)), _ => (), } } @@ -128,3 +128,65 @@ impl<'hir> Visitor<'hir> for LoopVisitor<'hir, '_> { } } } + +/// Similar to [`FnRetTy`], but reveals the actual type of an `OpaqueDef`. +enum RetTy<'hir> { + DefaultReturn(Span), + Return(Ty<'hir>), +} + +impl<'hir> RetTy<'hir> { + fn from_fn_ret_ty(cx: &LateContext<'hir>, fn_ret_ty: FnRetTy<'hir>) -> Self { + /// Reveal and return the related type of an `opaque`, return `None` if the + /// given `ty` is not an `OpaqueDef`. + fn inner_<'tcx>(cx: &LateContext<'tcx>, ty: &Ty<'tcx>) -> Option> { + /// Visitor to find the type binding. + struct BindingVisitor<'tcx> { + res: Option>, + } + 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 { + term: hir::Term::Ty(ty), + } = type_binding.kind + { + self.res = Some(*ty); + } + } + } + + 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 + } + + match fn_ret_ty { + FnRetTy::DefaultReturn(span) => Self::DefaultReturn(span), + FnRetTy::Return(ty) => Self::Return(inner_(cx, ty).unwrap_or(*ty)), + } + } + /// Returns the span to where the suggestion should be. + fn sugg_span(&self) -> Option { + match self { + Self::DefaultReturn(span) => Some(*span), + Self::Return(ty) if matches!(ty.kind, TyKind::Tup(&[])) => Some(ty.span), + Self::Return(_) => None, + } + } + fn is_never(&self) -> bool { + let Self::Return(ty) = self else { return false }; + matches!(ty.kind, TyKind::Never) + } +} diff --git a/tests/ui/infinite_loops.rs b/tests/ui/infinite_loops.rs index b2d522fa011b..fba9200c8956 100644 --- a/tests/ui/infinite_loops.rs +++ b/tests/ui/infinite_loops.rs @@ -390,4 +390,21 @@ fn span_inside_fn() { } } +// loop in async functions +mod issue_12338 { + use super::do_something; + + async fn foo() -> ! { + loop { + do_something(); + } + } + async fn bar() { + loop { + //~^ ERROR: infinite loop detected + do_something(); + } + } +} + fn main() {} diff --git a/tests/ui/infinite_loops.stderr b/tests/ui/infinite_loops.stderr index ec6bd81dc175..10eff9c0b723 100644 --- a/tests/ui/infinite_loops.stderr +++ b/tests/ui/infinite_loops.stderr @@ -255,5 +255,19 @@ LL | | }) | = help: if this is not intended, try adding a `break` or `return` condition in the loop -error: aborting due to 17 previous errors +error: infinite loop detected + --> tests/ui/infinite_loops.rs:403:9 + | +LL | / loop { +LL | | +LL | | do_something(); +LL | | } + | |_________^ + | +help: if this is intentional, consider specifying `!` as function return + | +LL | async fn bar() -> ! { + | ++++ + +error: aborting due to 18 previous errors From 0396536ebf587d13097c6255a545ad60dc22a5cc Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Wed, 26 Jun 2024 09:59:18 +0800 Subject: [PATCH 2/3] [`infinite_loop`]: fix suggestion with unit ty return; add more test cases with async clousures; --- clippy_lints/src/loops/infinite_loop.rs | 75 +++++++++++++------- tests/ui/infinite_loops.rs | 37 +++++++++- tests/ui/infinite_loops.stderr | 94 ++++++++++++++++++++----- 3 files changed, 160 insertions(+), 46 deletions(-) diff --git a/clippy_lints/src/loops/infinite_loop.rs b/clippy_lints/src/loops/infinite_loop.rs index 4134944178d9..30c38ff513a6 100644 --- a/clippy_lints/src/loops/infinite_loop.rs +++ b/clippy_lints/src/loops/infinite_loop.rs @@ -1,3 +1,5 @@ +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}; @@ -42,10 +44,22 @@ pub(super) fn check<'tcx>( if !is_finite_loop { 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 { @@ -141,35 +155,39 @@ impl<'hir> RetTy<'hir> { /// given `ty` is not an `OpaqueDef`. fn inner_<'tcx>(cx: &LateContext<'tcx>, ty: &Ty<'tcx>) -> Option> { /// Visitor to find the type binding. - struct BindingVisitor<'tcx> { - res: Option>, - } - 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>; + + 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 +204,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/tests/ui/infinite_loops.rs b/tests/ui/infinite_loops.rs index fba9200c8956..b9297c6f7df6 100644 --- 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 index 10eff9c0b723..8749583c1323 100644 --- a/tests/ui/infinite_loops.stderr +++ b/tests/ui/infinite_loops.stderr @@ -1,5 +1,5 @@ error: infinite loop detected - --> tests/ui/infinite_loops.rs:13:5 + --> tests/ui/infinite_loops.rs:14:5 | LL | / loop { LL | | @@ -15,7 +15,7 @@ LL | fn no_break() -> ! { | ++++ error: infinite loop detected - --> tests/ui/infinite_loops.rs:20:5 + --> tests/ui/infinite_loops.rs:21:5 | LL | / loop { LL | | @@ -32,7 +32,7 @@ LL | fn all_inf() -> ! { | ++++ error: infinite loop detected - --> tests/ui/infinite_loops.rs:22:9 + --> tests/ui/infinite_loops.rs:23:9 | LL | / loop { LL | | @@ -49,7 +49,7 @@ LL | fn all_inf() -> ! { | ++++ error: infinite loop detected - --> tests/ui/infinite_loops.rs:24:13 + --> tests/ui/infinite_loops.rs:25:13 | LL | / loop { LL | | @@ -63,7 +63,7 @@ LL | fn all_inf() -> ! { | ++++ error: infinite loop detected - --> tests/ui/infinite_loops.rs:38:5 + --> tests/ui/infinite_loops.rs:39:5 | LL | / loop { LL | | @@ -74,7 +74,7 @@ LL | | } = help: if this is not intended, try adding a `break` or `return` condition in the loop error: infinite loop detected - --> tests/ui/infinite_loops.rs:51:5 + --> tests/ui/infinite_loops.rs:52:5 | LL | / loop { LL | | fn inner_fn() -> ! { @@ -90,7 +90,7 @@ LL | fn no_break_never_ret_noise() -> ! { | ++++ error: infinite loop detected - --> tests/ui/infinite_loops.rs:94:5 + --> tests/ui/infinite_loops.rs:95:5 | LL | / loop { LL | | @@ -107,7 +107,7 @@ LL | fn break_inner_but_not_outer_1(cond: bool) -> ! { | ++++ error: infinite loop detected - --> tests/ui/infinite_loops.rs:105:5 + --> tests/ui/infinite_loops.rs:106:5 | LL | / loop { LL | | @@ -124,7 +124,7 @@ LL | fn break_inner_but_not_outer_2(cond: bool) -> ! { | ++++ error: infinite loop detected - --> tests/ui/infinite_loops.rs:119:9 + --> tests/ui/infinite_loops.rs:120:9 | LL | / loop { LL | | @@ -138,7 +138,7 @@ LL | fn break_outer_but_not_inner() -> ! { | ++++ error: infinite loop detected - --> tests/ui/infinite_loops.rs:142:9 + --> tests/ui/infinite_loops.rs:143:9 | LL | / loop { LL | | @@ -155,7 +155,7 @@ LL | fn break_wrong_loop(cond: bool) -> ! { | ++++ error: infinite loop detected - --> tests/ui/infinite_loops.rs:182:5 + --> tests/ui/infinite_loops.rs:183:5 | LL | / loop { LL | | @@ -172,7 +172,7 @@ LL | fn match_like() -> ! { | ++++ error: infinite loop detected - --> tests/ui/infinite_loops.rs:223:5 + --> tests/ui/infinite_loops.rs:224:5 | LL | / loop { LL | | @@ -186,7 +186,7 @@ LL | fn match_like() -> ! { | ++++ error: infinite loop detected - --> tests/ui/infinite_loops.rs:228:5 + --> tests/ui/infinite_loops.rs:229:5 | LL | / loop { LL | | @@ -203,7 +203,7 @@ LL | fn match_like() -> ! { | ++++ error: infinite loop detected - --> tests/ui/infinite_loops.rs:333:9 + --> tests/ui/infinite_loops.rs:334:9 | LL | / loop { LL | | @@ -217,7 +217,7 @@ LL | fn problematic_trait_method() -> ! { | ++++ error: infinite loop detected - --> tests/ui/infinite_loops.rs:343:9 + --> tests/ui/infinite_loops.rs:344:9 | LL | / loop { LL | | @@ -231,7 +231,7 @@ LL | fn could_be_problematic() -> ! { | ++++ error: infinite loop detected - --> tests/ui/infinite_loops.rs:352:9 + --> tests/ui/infinite_loops.rs:353:9 | LL | / loop { LL | | @@ -245,7 +245,7 @@ LL | let _loop_forever = || -> ! { | ++++ error: infinite loop detected - --> tests/ui/infinite_loops.rs:366:8 + --> tests/ui/infinite_loops.rs:367:8 | LL | Ok(loop { | ________^ @@ -256,7 +256,35 @@ LL | | }) = help: if this is not intended, try adding a `break` or `return` condition in the loop error: infinite loop detected - --> tests/ui/infinite_loops.rs:403:9 + --> tests/ui/infinite_loops.rs:395:5 + | +LL | / loop { +LL | | +LL | | do_nothing(); +LL | | } + | |_____^ + | +help: if this is intentional, consider specifying `!` as function return + | +LL | fn return_unit() -> ! { + | ~ + +error: infinite loop detected + --> tests/ui/infinite_loops.rs:401:9 + | +LL | / loop { +LL | | +LL | | do_nothing(); +LL | | } + | |_________^ + | +help: if this is intentional, consider specifying `!` as function return + | +LL | let async_return_unit = || -> ! { + | ~ + +error: infinite loop detected + --> tests/ui/infinite_loops.rs:418:9 | LL | / loop { LL | | @@ -269,5 +297,33 @@ help: if this is intentional, consider specifying `!` as function return LL | async fn bar() -> ! { | ++++ -error: aborting due to 18 previous errors +error: infinite loop detected + --> tests/ui/infinite_loops.rs:426:13 + | +LL | / loop { +LL | | +LL | | do_something(); +LL | | } + | |_____________^ + | +help: if this is intentional, consider specifying `!` as function return + | +LL | let _loop_forever = async || -> ! { + | ++++ + +error: infinite loop detected + --> tests/ui/infinite_loops.rs:437:13 + | +LL | / loop { +LL | | +LL | | do_something(); +LL | | } + | |_____________^ + | +help: if this is intentional, consider specifying `!` as function return + | +LL | let _ret_unit = async || -> ! { + | ~ + +error: aborting due to 22 previous errors From b31fd072a5151e8ccc01c1299bc5b96a268e0dd1 Mon Sep 17 00:00:00 2001 From: J-ZhengLi Date: Tue, 16 Jul 2024 01:16:58 +0800 Subject: [PATCH 3/3] [`infinite_loop`]: give a result type to `LoopVisitor` --- clippy_lints/src/loops/infinite_loop.rs | 52 ++++++++++++------------- clippy_lints/src/loops/mod.rs | 4 +- 2 files changed, 27 insertions(+), 29 deletions(-) diff --git a/clippy_lints/src/loops/infinite_loop.rs b/clippy_lints/src/loops/infinite_loop.rs index 30c38ff513a6..d7fa7935fc6d 100644 --- a/clippy_lints/src/loops/infinite_loop.rs +++ b/clippy_lints/src/loops/infinite_loop.rs @@ -4,7 +4,6 @@ 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}; @@ -17,7 +16,7 @@ pub(super) fn check<'tcx>( cx: &LateContext<'tcx>, expr: &Expr<'tcx>, loop_block: &'tcx hir::Block<'_>, - label: Option