From e0097f5323f700768b2b8ceb217b4394ff447da6 Mon Sep 17 00:00:00 2001 From: Michael Goulet Date: Mon, 25 Dec 2023 18:01:56 +0000 Subject: [PATCH] Fix clippy's usage of Body's coroutine_kind Also fixes a bug where we weren't peeling blocks from async bodies --- clippy_lints/src/async_yields_async.rs | 23 +++++++++++----- clippy_lints/src/await_holding_invalid.rs | 18 +++++++----- clippy_lints/src/manual_async_fn.rs | 32 ++++++++++++++-------- clippy_lints/src/methods/iter_kv_map.rs | 1 - clippy_lints/src/needless_question_mark.rs | 32 ++++++++++------------ clippy_lints/src/redundant_async_block.rs | 6 ++-- clippy_lints/src/redundant_closure_call.rs | 9 +++--- clippy_lints/src/unused_async.rs | 28 +++++++++---------- clippy_lints/src/utils/author.rs | 21 +++++++++++--- tests/ui/author/blocks.stdout | 4 +-- tests/ui/author/macro_in_closure.stdout | 2 +- tests/ui/needless_question_mark.fixed | 4 +++ tests/ui/needless_question_mark.rs | 4 +++ tests/ui/needless_question_mark.stderr | 8 +++++- 14 files changed, 119 insertions(+), 73 deletions(-) diff --git a/clippy_lints/src/async_yields_async.rs b/clippy_lints/src/async_yields_async.rs index 28e6614f03f..c965341d3fd 100644 --- a/clippy_lints/src/async_yields_async.rs +++ b/clippy_lints/src/async_yields_async.rs @@ -2,7 +2,9 @@ use clippy_utils::source::snippet; use clippy_utils::ty::implements_trait; use rustc_errors::Applicability; -use rustc_hir::{Body, BodyId, CoroutineKind, CoroutineSource, CoroutineDesugaring, ExprKind, QPath}; +use rustc_hir::{ + Closure, ClosureKind, CoroutineDesugaring, CoroutineKind, CoroutineSource, Expr, ExprKind, QPath, +}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::declare_lint_pass; @@ -44,15 +46,22 @@ declare_lint_pass!(AsyncYieldsAsync => [ASYNC_YIELDS_ASYNC]); impl<'tcx> LateLintPass<'tcx> for AsyncYieldsAsync { - fn check_body(&mut self, cx: &LateContext<'tcx>, body: &'tcx Body<'_>) { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { // For functions, with explicitly defined types, don't warn. // XXXkhuey maybe we should? - if let Some(CoroutineKind::Desugared(CoroutineDesugaring::Async, CoroutineSource::Block | CoroutineSource::Closure)) = body.coroutine_kind { + if let ExprKind::Closure(Closure { + kind: + ClosureKind::Coroutine(CoroutineKind::Desugared( + CoroutineDesugaring::Async, + CoroutineSource::Block | CoroutineSource::Closure, + )), + body: body_id, + .. + }) = expr.kind + { if let Some(future_trait_def_id) = cx.tcx.lang_items().future_trait() { - let body_id = BodyId { - hir_id: body.value.hir_id, - }; - let typeck_results = cx.tcx.typeck_body(body_id); + let typeck_results = cx.tcx.typeck_body(*body_id); + let body = cx.tcx.hir().body(*body_id); let expr_ty = typeck_results.expr_ty(body.value); if implements_trait(cx, expr_ty, future_trait_def_id, &[]) { diff --git a/clippy_lints/src/await_holding_invalid.rs b/clippy_lints/src/await_holding_invalid.rs index dff6e884fa1..765cc7c0a54 100644 --- a/clippy_lints/src/await_holding_invalid.rs +++ b/clippy_lints/src/await_holding_invalid.rs @@ -2,8 +2,8 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::{match_def_path, paths}; use rustc_data_structures::fx::FxHashMap; +use rustc_hir as hir; use rustc_hir::def_id::DefId; -use rustc_hir::{Body, CoroutineKind, CoroutineDesugaring}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::mir::CoroutineLayout; use rustc_session::impl_lint_pass; @@ -183,8 +183,8 @@ pub(crate) fn new(conf_invalid_types: Vec) -> Self { } } -impl LateLintPass<'_> for AwaitHolding { - fn check_crate(&mut self, cx: &LateContext<'_>) { +impl<'tcx> LateLintPass<'tcx> for AwaitHolding { + fn check_crate(&mut self, cx: &LateContext<'tcx>) { for conf in &self.conf_invalid_types { let segs: Vec<_> = conf.path().split("::").collect(); for id in clippy_utils::def_path_def_ids(cx, &segs) { @@ -193,10 +193,14 @@ fn check_crate(&mut self, cx: &LateContext<'_>) { } } - fn check_body(&mut self, cx: &LateContext<'_>, body: &'_ Body<'_>) { - if let Some(CoroutineKind::Desugared(CoroutineDesugaring::Async, _)) = body.coroutine_kind { - let def_id = cx.tcx.hir().body_owner_def_id(body.id()); - if let Some(coroutine_layout) = cx.tcx.mir_coroutine_witnesses(def_id) { + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx hir::Expr<'tcx>) { + if let hir::ExprKind::Closure(hir::Closure { + kind: hir::ClosureKind::Coroutine(hir::CoroutineKind::Desugared(hir::CoroutineDesugaring::Async, _)), + def_id, + .. + }) = expr.kind + { + if let Some(coroutine_layout) = cx.tcx.mir_coroutine_witnesses(*def_id) { self.check_interior_types(cx, coroutine_layout); } } diff --git a/clippy_lints/src/manual_async_fn.rs b/clippy_lints/src/manual_async_fn.rs index 8982ce5e196..9ba1d3afcbe 100644 --- a/clippy_lints/src/manual_async_fn.rs +++ b/clippy_lints/src/manual_async_fn.rs @@ -3,8 +3,9 @@ use rustc_errors::Applicability; use rustc_hir::intravisit::FnKind; use rustc_hir::{ - Block, Body, Closure, CoroutineKind, CoroutineSource, CoroutineDesugaring, Expr, ExprKind, FnDecl, FnRetTy, GenericArg, GenericBound, - ImplItem, Item, ItemKind, LifetimeName, Node, Term, TraitRef, Ty, TyKind, TypeBindingKind, + Block, Body, Closure, CoroutineDesugaring, CoroutineKind, CoroutineSource, Expr, ExprKind, FnDecl, FnRetTy, + GenericArg, GenericBound, ImplItem, Item, ItemKind, LifetimeName, Node, Term, TraitRef, Ty, TyKind, + TypeBindingKind, ClosureKind, }; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::declare_lint_pass; @@ -171,16 +172,25 @@ fn captures_all_lifetimes(inputs: &[Ty<'_>], output_lifetimes: &[LifetimeName]) .all(|in_lt| output_lifetimes.iter().any(|out_lt| in_lt == out_lt)) } -fn desugared_async_block<'tcx>(cx: &LateContext<'tcx>, block: &'tcx Block<'tcx>) -> Option<&'tcx Body<'tcx>> { - if let Some(block_expr) = block.expr - && let Expr { - kind: ExprKind::Closure(&Closure { body, .. }), - .. - } = block_expr - && let closure_body = cx.tcx.hir().body(body) - && closure_body.coroutine_kind == Some(CoroutineKind::Desugared(CoroutineDesugaring::Async, CoroutineSource::Block)) +fn desugared_async_block<'tcx>( + cx: &LateContext<'tcx>, + block: &'tcx Block<'tcx>, +) -> Option<&'tcx Body<'tcx>> { + if let Some(Expr { + kind: + ExprKind::Closure(&Closure { + kind: + ClosureKind::Coroutine(CoroutineKind::Desugared( + CoroutineDesugaring::Async, + CoroutineSource::Block, + )), + body, + .. + }), + .. + }) = block.expr { - return Some(closure_body); + return Some(cx.tcx.hir().body(body)); } None diff --git a/clippy_lints/src/methods/iter_kv_map.rs b/clippy_lints/src/methods/iter_kv_map.rs index e1b934d36ea..6394f35f860 100644 --- a/clippy_lints/src/methods/iter_kv_map.rs +++ b/clippy_lints/src/methods/iter_kv_map.rs @@ -32,7 +32,6 @@ pub(super) fn check<'tcx>( && let Body { params: [p], value: body_expr, - coroutine_kind: _, } = cx.tcx.hir().body(c.body) && let PatKind::Tuple([key_pat, val_pat], _) = p.pat.kind && let (replacement_kind, annotation, bound_ident) = match (&key_pat.kind, &val_pat.kind) { diff --git a/clippy_lints/src/needless_question_mark.rs b/clippy_lints/src/needless_question_mark.rs index 350707d3a13..d7adf22ff32 100644 --- a/clippy_lints/src/needless_question_mark.rs +++ b/clippy_lints/src/needless_question_mark.rs @@ -3,7 +3,7 @@ use clippy_utils::source::snippet; use rustc_errors::Applicability; use rustc_hir::def::{DefKind, Res}; -use rustc_hir::{Block, Body, CoroutineKind, CoroutineSource, CoroutineDesugaring, Expr, ExprKind, LangItem, MatchSource, QPath}; +use rustc_hir::{Block, Body, Expr, ExprKind, LangItem, MatchSource, QPath}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::declare_lint_pass; @@ -86,22 +86,20 @@ fn check_expr(&mut self, cx: &LateContext<'_>, expr: &'_ Expr<'_>) { } fn check_body(&mut self, cx: &LateContext<'_>, body: &'_ Body<'_>) { - if let Some(CoroutineKind::Desugared(CoroutineDesugaring::Async, CoroutineSource::Fn)) = body.coroutine_kind { - if let ExprKind::Block( - Block { - expr: - Some(Expr { - kind: ExprKind::DropTemps(async_body), - .. - }), - .. - }, - _, - ) = body.value.kind - { - if let ExprKind::Block(Block { expr: Some(expr), .. }, ..) = async_body.kind { - check(cx, expr); - } + if let ExprKind::Block( + Block { + expr: + Some(Expr { + kind: ExprKind::DropTemps(async_body), + .. + }), + .. + }, + _, + ) = body.value.kind + { + if let ExprKind::Block(Block { expr: Some(expr), .. }, ..) = async_body.kind { + check(cx, expr.peel_blocks()); } } else { check(cx, body.value.peel_blocks()); diff --git a/clippy_lints/src/redundant_async_block.rs b/clippy_lints/src/redundant_async_block.rs index 4b3fe9c0bb5..b50141f048c 100644 --- a/clippy_lints/src/redundant_async_block.rs +++ b/clippy_lints/src/redundant_async_block.rs @@ -5,7 +5,7 @@ use clippy_utils::source::{snippet, walk_span_to_context}; use clippy_utils::visitors::for_each_expr; use rustc_errors::Applicability; -use rustc_hir::{Closure, CoroutineKind, CoroutineSource, CoroutineDesugaring, Expr, ExprKind, MatchSource}; +use rustc_hir::{Closure, ClosureKind, CoroutineKind, CoroutineSource, CoroutineDesugaring, Expr, ExprKind, MatchSource}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::lint::in_external_macro; use rustc_middle::ty::UpvarCapture; @@ -69,9 +69,9 @@ fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { /// If `expr` is a desugared `async` block, return the original expression if it does not capture /// any variable by ref. fn desugar_async_block<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> { - if let ExprKind::Closure(Closure { body, def_id, .. }) = expr.kind + if let ExprKind::Closure(Closure { body, def_id, kind, .. }) = expr.kind && let body = cx.tcx.hir().body(*body) - && matches!(body.coroutine_kind, Some(CoroutineKind::Desugared(CoroutineDesugaring::Async, CoroutineSource::Block))) + && matches!(kind, ClosureKind::Coroutine(CoroutineKind::Desugared(CoroutineDesugaring::Async, CoroutineSource::Block))) { cx.typeck_results() .closure_min_captures diff --git a/clippy_lints/src/redundant_closure_call.rs b/clippy_lints/src/redundant_closure_call.rs index 9312a9c89b7..16c929edb92 100644 --- a/clippy_lints/src/redundant_closure_call.rs +++ b/clippy_lints/src/redundant_closure_call.rs @@ -5,7 +5,7 @@ use rustc_errors::Applicability; use rustc_hir as hir; use rustc_hir::intravisit::{Visitor as HirVisitor, Visitor}; -use rustc_hir::{intravisit as hir_visit, CoroutineKind, CoroutineSource, CoroutineDesugaring, Node}; +use rustc_hir::{intravisit as hir_visit, CoroutineKind, CoroutineSource, CoroutineDesugaring, Node, ClosureKind}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::hir::nested_filter; use rustc_middle::lint::in_external_macro; @@ -63,11 +63,10 @@ fn visit_expr(&mut self, ex: &'tcx hir::Expr<'tcx>) { /// Checks if the body is owned by an async closure. /// Returns true for `async || whatever_expression`, but false for `|| async { whatever_expression /// }`. -fn is_async_closure(cx: &LateContext<'_>, body: &hir::Body<'_>) -> bool { +fn is_async_closure(body: &hir::Body<'_>) -> bool { if let hir::ExprKind::Closure(innermost_closure_generated_by_desugar) = body.value.kind - && let desugared_inner_closure_body = cx.tcx.hir().body(innermost_closure_generated_by_desugar.body) // checks whether it is `async || whatever_expression` - && let Some(CoroutineKind::Desugared(CoroutineDesugaring::Async, CoroutineSource::Closure)) = desugared_inner_closure_body.coroutine_kind + && let ClosureKind::Coroutine(CoroutineKind::Desugared(CoroutineDesugaring::Async, CoroutineSource::Closure)) = innermost_closure_generated_by_desugar.kind { true } else { @@ -103,7 +102,7 @@ fn find_innermost_closure<'tcx>( data = Some(( body.value, closure.fn_decl, - if is_async_closure(cx, body) { + if is_async_closure(body) { ty::Asyncness::Yes } else { ty::Asyncness::No diff --git a/clippy_lints/src/unused_async.rs b/clippy_lints/src/unused_async.rs index f71fe4e1e92..1d42375ba8e 100644 --- a/clippy_lints/src/unused_async.rs +++ b/clippy_lints/src/unused_async.rs @@ -1,7 +1,7 @@ use clippy_utils::diagnostics::span_lint_hir_and_then; use clippy_utils::is_def_id_trait_method; use rustc_hir::def::DefKind; -use rustc_hir::intravisit::{walk_body, walk_expr, walk_fn, FnKind, Visitor}; +use rustc_hir::intravisit::{walk_expr, walk_fn, FnKind, Visitor}; use rustc_hir::{Body, Expr, ExprKind, FnDecl, Node, YieldSource}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::hir::nested_filter; @@ -78,32 +78,32 @@ fn visit_expr(&mut self, ex: &'tcx Expr<'tcx>) { self.await_in_async_block = Some(ex.span); } } - walk_expr(self, ex); - } - fn nested_visit_map(&mut self) -> Self::Map { - self.cx.tcx.hir() - } - - fn visit_body(&mut self, b: &'tcx Body<'tcx>) { let is_async_block = matches!( - b.coroutine_kind, - Some(rustc_hir::CoroutineKind::Desugared( - rustc_hir::CoroutineDesugaring::Async, - _ - )) + ex.kind, + ExprKind::Closure(rustc_hir::Closure { + kind: rustc_hir::ClosureKind::Coroutine(rustc_hir::CoroutineKind::Desugared( + rustc_hir::CoroutineDesugaring::Async, + _ + )), + .. + }) ); if is_async_block { self.async_depth += 1; } - walk_body(self, b); + walk_expr(self, ex); if is_async_block { self.async_depth -= 1; } } + + fn nested_visit_map(&mut self) -> Self::Map { + self.cx.tcx.hir() + } } impl<'tcx> LateLintPass<'tcx> for UnusedAsync { diff --git a/clippy_lints/src/utils/author.rs b/clippy_lints/src/utils/author.rs index e83c04eda20..df715b12dea 100644 --- a/clippy_lints/src/utils/author.rs +++ b/clippy_lints/src/utils/author.rs @@ -7,7 +7,8 @@ use rustc_data_structures::fx::FxHashMap; use rustc_hir as hir; use rustc_hir::{ - ArrayLen, BindingAnnotation, CaptureBy, Closure, ExprKind, FnRetTy, HirId, Lit, PatKind, QPath, StmtKind, TyKind, + ArrayLen, BindingAnnotation, CaptureBy, Closure, ClosureKind, CoroutineKind, ExprKind, FnRetTy, HirId, Lit, + PatKind, QPath, StmtKind, TyKind, }; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_session::declare_lint_pass; @@ -476,7 +477,7 @@ macro_rules! kind { capture_clause, fn_decl, body: body_id, - movability, + kind, .. }) => { let capture_clause = match capture_clause { @@ -484,7 +485,17 @@ macro_rules! kind { CaptureBy::Ref => "Ref", }; - let movability = OptionPat::new(movability.map(|m| format!("Movability::{m:?}"))); + let closure_kind = match kind { + ClosureKind::Closure => "ClosureKind::Closure".to_string(), + ClosureKind::Coroutine(coroutine_kind) => match coroutine_kind { + CoroutineKind::Desugared(desugaring, source) => format!( + "ClosureKind::Coroutine(CoroutineKind::Desugared(CoroutineDesugaring::{desugaring:?}, CoroutineSource::{source:?}))" + ), + CoroutineKind::Coroutine(movability) => { + format!("ClosureKind::Coroutine(CoroutineKind::Coroutine(Movability::{movability:?})") + }, + }, + }; let ret_ty = match fn_decl.output { FnRetTy::DefaultReturn(_) => "FnRetTy::DefaultReturn(_)", @@ -492,7 +503,9 @@ macro_rules! kind { }; bind!(self, fn_decl, body_id); - kind!("Closure(CaptureBy::{capture_clause}, {fn_decl}, {body_id}, _, {movability})"); + kind!( + "Closure {{ capture_clause: CaptureBy::{capture_clause}, fn_decl: {fn_decl}, body: {body_id}, closure_kind: {closure_kind}, .. }}" + ); chain!(self, "let {ret_ty} = {fn_decl}.output"); self.body(body_id); }, diff --git a/tests/ui/author/blocks.stdout b/tests/ui/author/blocks.stdout index 140300a1673..62de661f8ff 100644 --- a/tests/ui/author/blocks.stdout +++ b/tests/ui/author/blocks.stdout @@ -40,10 +40,10 @@ if let ExprKind::Block(block, None) = expr.kind { // report your lint here } -if let ExprKind::Closure(CaptureBy::Value { .. }, fn_decl, body_id, _, None) = expr.kind +if let ExprKind::Closure { capture_clause: CaptureBy::Value { .. }, fn_decl: fn_decl, body: body_id, closure_kind: ClosureKind::Closure, .. } = expr.kind && let FnRetTy::DefaultReturn(_) = fn_decl.output && expr1 = &cx.tcx.hir().body(body_id).value - && let ExprKind::Closure(CaptureBy::Value { .. }, fn_decl1, body_id1, _, Some(Movability::Static)) = expr1.kind + && let ExprKind::Closure { capture_clause: CaptureBy::Value { .. }, fn_decl: fn_decl1, body: body_id1, closure_kind: ClosureKind::Coroutine(CoroutineKind::Desugared(CoroutineDesugaring::Async, CoroutineSource::Closure)), .. } = expr1.kind && let FnRetTy::DefaultReturn(_) = fn_decl1.output && expr2 = &cx.tcx.hir().body(body_id1).value && let ExprKind::Block(block, None) = expr2.kind diff --git a/tests/ui/author/macro_in_closure.stdout b/tests/ui/author/macro_in_closure.stdout index 9ab71986f40..06386d1d7ec 100644 --- a/tests/ui/author/macro_in_closure.stdout +++ b/tests/ui/author/macro_in_closure.stdout @@ -1,6 +1,6 @@ if let StmtKind::Local(local) = stmt.kind && let Some(init) = local.init - && let ExprKind::Closure(CaptureBy::Ref, fn_decl, body_id, _, None) = init.kind + && let ExprKind::Closure { capture_clause: CaptureBy::Ref, fn_decl: fn_decl, body: body_id, closure_kind: ClosureKind::Closure, .. } = init.kind && let FnRetTy::DefaultReturn(_) = fn_decl.output && expr = &cx.tcx.hir().body(body_id).value && let ExprKind::Block(block, None) = expr.kind diff --git a/tests/ui/needless_question_mark.fixed b/tests/ui/needless_question_mark.fixed index 07bd6b6f3c1..92f01c217c1 100644 --- a/tests/ui/needless_question_mark.fixed +++ b/tests/ui/needless_question_mark.fixed @@ -135,3 +135,7 @@ async fn async_deref_ref(s: Option<&String>) -> Option<&str> { async fn async_result_bad(s: TR) -> Result { s.magic } + +async fn async_wrapped(a: Option) -> Option { + { a } +} diff --git a/tests/ui/needless_question_mark.rs b/tests/ui/needless_question_mark.rs index fbf8a12fd50..21c858c291f 100644 --- a/tests/ui/needless_question_mark.rs +++ b/tests/ui/needless_question_mark.rs @@ -135,3 +135,7 @@ async fn async_deref_ref(s: Option<&String>) -> Option<&str> { async fn async_result_bad(s: TR) -> Result { Ok(s.magic?) } + +async fn async_wrapped(a: Option) -> Option { + { Some(a?) } +} diff --git a/tests/ui/needless_question_mark.stderr b/tests/ui/needless_question_mark.stderr index cd961a49f42..bf090302ef7 100644 --- a/tests/ui/needless_question_mark.stderr +++ b/tests/ui/needless_question_mark.stderr @@ -90,5 +90,11 @@ error: question mark operator is useless here LL | Ok(s.magic?) | ^^^^^^^^^^^^ help: try removing question mark and `Ok()`: `s.magic` -error: aborting due to 14 previous errors +error: question mark operator is useless here + --> $DIR/needless_question_mark.rs:140:7 + | +LL | { Some(a?) } + | ^^^^^^^^ help: try removing question mark and `Some()`: `a` + +error: aborting due to 15 previous errors