From 86391abc704555610af8ba33d910bdd16a877ddb Mon Sep 17 00:00:00 2001 From: est31 Date: Sun, 11 Jun 2023 06:35:29 +0200 Subject: [PATCH 1/5] Don't lint manual_let_else in cases where the question mark operator would work Also, lint question_mark for `let...else` clauses that can be simplified to use `?`. This lint isn't perfect as it doesn't support the unstable try blocks. --- clippy_lints/src/manual_let_else.rs | 48 ++++++++++++++- clippy_lints/src/question_mark.rs | 33 +++++++++- tests/ui/manual_let_else_question_mark.fixed | 56 +++++++++++++++++ tests/ui/manual_let_else_question_mark.rs | 61 +++++++++++++++++++ tests/ui/manual_let_else_question_mark.stderr | 49 +++++++++++++++ 5 files changed, 245 insertions(+), 2 deletions(-) create mode 100644 tests/ui/manual_let_else_question_mark.fixed create mode 100644 tests/ui/manual_let_else_question_mark.rs create mode 100644 tests/ui/manual_let_else_question_mark.stderr diff --git a/clippy_lints/src/manual_let_else.rs b/clippy_lints/src/manual_let_else.rs index 59e421c16220..dcfc365a3483 100644 --- a/clippy_lints/src/manual_let_else.rs +++ b/clippy_lints/src/manual_let_else.rs @@ -1,14 +1,15 @@ use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::higher::IfLetOrMatch; use clippy_utils::msrvs::{self, Msrv}; -use clippy_utils::peel_blocks; use clippy_utils::source::snippet_with_context; use clippy_utils::ty::is_type_diagnostic_item; use clippy_utils::visitors::{Descend, Visitable}; +use clippy_utils::{is_refutable, is_res_lang_ctor, peel_blocks}; use if_chain::if_chain; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_errors::Applicability; use rustc_hir::intravisit::{walk_expr, Visitor}; +use rustc_hir::LangItem::{OptionNone, OptionSome}; use rustc_hir::{Expr, ExprKind, HirId, ItemId, Local, MatchSource, Pat, PatKind, QPath, Stmt, StmtKind, Ty}; use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::lint::in_external_macro; @@ -85,6 +86,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse { if let Some(ident_map) = expr_simple_identity_map(local.pat, let_pat, if_then); if let Some(if_else) = if_else; if expr_diverges(cx, if_else); + if pat_and_expr_can_be_question_mark(cx, let_pat, if_else).is_none(); then { emit_manual_let_else(cx, stmt.span, if_let_expr, &ident_map, let_pat, if_else); } @@ -167,6 +169,50 @@ fn emit_manual_let_else( ); } +/// Returns whether the given let pattern and else body can be turned into a question mark +/// +/// For this example: +/// ```ignore +/// let FooBar { a, b } = if let Some(a) = ex { a } else { return None }; +/// ``` +/// We get as parameters: +/// ```ignore +/// pat: Some(a) +/// else_body: return None +/// ``` + +/// And for this example: +/// ```ignore +/// let Some(FooBar { a, b }) = ex else { return None }; +/// ``` +/// We get as parameters: +/// ```ignore +/// pat: Some(FooBar { a, b }) +/// else_body: return None +/// ``` + +/// We output `Some(a)` in the first instance, and `Some(FooBar { a, b })` in the second, because +/// the question mark operator is applicable here. Callers have to check whether we are in a +/// constant or not. +pub fn pat_and_expr_can_be_question_mark<'a, 'hir>( + cx: &LateContext<'_>, + pat: &'a Pat<'hir>, + else_body: &Expr<'_>, +) -> Option<&'a Pat<'hir>> { + if let PatKind::TupleStruct(pat_path, [inner_pat], _) = pat.kind && + is_res_lang_ctor(cx, cx.qpath_res(&pat_path, pat.hir_id), OptionSome) && + !is_refutable(cx, inner_pat) && + let else_body = peel_blocks(else_body) && + let ExprKind::Ret(Some(ret_val)) = else_body.kind && + let ExprKind::Path(ret_path) = ret_val.kind && + is_res_lang_ctor(cx, cx.qpath_res(&ret_path, ret_val.hir_id), OptionNone) + { + Some(inner_pat) + } else { + None + } +} + /// Replaces the locals in the pattern /// /// For this example: diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index e3d940ad2a44..b1eeedc26bdb 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -1,3 +1,4 @@ +use crate::manual_let_else::pat_and_expr_can_be_question_mark; use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::snippet_with_applicability; use clippy_utils::ty::is_type_diagnostic_item; @@ -10,7 +11,9 @@ use if_chain::if_chain; use rustc_errors::Applicability; use rustc_hir::def::Res; use rustc_hir::LangItem::{self, OptionNone, OptionSome, ResultErr, ResultOk}; -use rustc_hir::{BindingAnnotation, ByRef, Expr, ExprKind, Node, PatKind, PathSegment, QPath}; +use rustc_hir::{ + BindingAnnotation, Block, ByRef, Expr, ExprKind, Local, Node, PatKind, PathSegment, QPath, Stmt, StmtKind, +}; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::Ty; use rustc_session::declare_tool_lint; @@ -78,6 +81,29 @@ enum IfBlockType<'hir> { ), } +fn check_let_some_else_return_none(cx: &LateContext<'_>, stmt: &Stmt<'_>) { + if let StmtKind::Local(Local { pat, init: Some(init_expr), els: Some(els), .. }) = stmt.kind && + let Block { stmts: &[], expr: Some(els), .. } = els && + let Some(inner_pat) = pat_and_expr_can_be_question_mark(cx, pat, els) + { + let mut applicability = Applicability::MaybeIncorrect; + let init_expr_str = snippet_with_applicability(cx, init_expr.span, "..", &mut applicability); + let receiver_str = snippet_with_applicability(cx, inner_pat.span, "..", &mut applicability); + let sugg = format!( + "let {receiver_str} = {init_expr_str}?;", + ); + span_lint_and_sugg( + cx, + QUESTION_MARK, + stmt.span, + "this `let...else` may be rewritten with the `?` operator", + "replace it with", + sugg, + applicability, + ); + } +} + fn is_early_return(smbl: Symbol, cx: &LateContext<'_>, if_block: &IfBlockType<'_>) -> bool { match *if_block { IfBlockType::IfIs(caller, caller_ty, call_sym, if_then, _) => { @@ -259,6 +285,11 @@ fn is_try_block(cx: &LateContext<'_>, bl: &rustc_hir::Block<'_>) -> bool { } impl<'tcx> LateLintPass<'tcx> for QuestionMark { + fn check_stmt(&mut self, cx: &LateContext<'tcx>, stmt: &'tcx Stmt<'_>) { + if !in_constant(cx, stmt.hir_id) { + check_let_some_else_return_none(cx, stmt); + } + } fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { if !in_constant(cx, expr.hir_id) { self.check_is_none_or_err_and_early_return(cx, expr); diff --git a/tests/ui/manual_let_else_question_mark.fixed b/tests/ui/manual_let_else_question_mark.fixed new file mode 100644 index 000000000000..617f127b9a86 --- /dev/null +++ b/tests/ui/manual_let_else_question_mark.fixed @@ -0,0 +1,56 @@ +//@run-rustfix +#![allow(unused_braces, unused_variables, dead_code)] +#![allow( + clippy::collapsible_else_if, + clippy::unused_unit, + clippy::let_unit_value, + clippy::match_single_binding, + clippy::never_loop +)] +#![warn(clippy::manual_let_else, clippy::question_mark)] + +enum Variant { + A(usize, usize), + B(usize), + C, +} + +fn g() -> Option<(u8, u8)> { + None +} + +fn e() -> Variant { + Variant::A(0, 0) +} + +fn main() {} + +fn foo() -> Option<()> { + // Fire here, normal case + let v = g()?; + + // Don't fire here, the pattern is refutable + let Variant::A(v, w) = e() else { return None }; + + // Fire here, the pattern is irrefutable + let (v, w) = g()?; + + // Don't fire manual_let_else in this instance: question mark can be used instead. + let v = g()?; + + // Do fire manual_let_else in this instance: question mark cannot be used here due to the return + // body. + let Some(v) = g() else { + return Some(()); + }; + + // Here we could also fire the question_mark lint, but we don't (as it's a match and not an if let). + // So we still emit manual_let_else here. For the *resulting* code, we *do* emit the question_mark + // lint, so for rustfix reasons, we allow the question_mark lint here. + #[allow(clippy::question_mark)] + { + let Some(v) = g() else { return None }; + } + + Some(()) +} diff --git a/tests/ui/manual_let_else_question_mark.rs b/tests/ui/manual_let_else_question_mark.rs new file mode 100644 index 000000000000..3cf1eca86734 --- /dev/null +++ b/tests/ui/manual_let_else_question_mark.rs @@ -0,0 +1,61 @@ +//@run-rustfix +#![allow(unused_braces, unused_variables, dead_code)] +#![allow( + clippy::collapsible_else_if, + clippy::unused_unit, + clippy::let_unit_value, + clippy::match_single_binding, + clippy::never_loop +)] +#![warn(clippy::manual_let_else, clippy::question_mark)] + +enum Variant { + A(usize, usize), + B(usize), + C, +} + +fn g() -> Option<(u8, u8)> { + None +} + +fn e() -> Variant { + Variant::A(0, 0) +} + +fn main() {} + +fn foo() -> Option<()> { + // Fire here, normal case + let Some(v) = g() else { return None }; + + // Don't fire here, the pattern is refutable + let Variant::A(v, w) = e() else { return None }; + + // Fire here, the pattern is irrefutable + let Some((v, w)) = g() else { return None }; + + // Don't fire manual_let_else in this instance: question mark can be used instead. + let v = if let Some(v_some) = g() { v_some } else { return None }; + + // Do fire manual_let_else in this instance: question mark cannot be used here due to the return + // body. + let v = if let Some(v_some) = g() { + v_some + } else { + return Some(()); + }; + + // Here we could also fire the question_mark lint, but we don't (as it's a match and not an if let). + // So we still emit manual_let_else here. For the *resulting* code, we *do* emit the question_mark + // lint, so for rustfix reasons, we allow the question_mark lint here. + #[allow(clippy::question_mark)] + { + let v = match g() { + Some(v_some) => v_some, + _ => return None, + }; + } + + Some(()) +} diff --git a/tests/ui/manual_let_else_question_mark.stderr b/tests/ui/manual_let_else_question_mark.stderr new file mode 100644 index 000000000000..31052acf75cd --- /dev/null +++ b/tests/ui/manual_let_else_question_mark.stderr @@ -0,0 +1,49 @@ +error: this `let...else` may be rewritten with the `?` operator + --> $DIR/manual_let_else_question_mark.rs:30:5 + | +LL | let Some(v) = g() else { return None }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `let v = g()?;` + | + = note: `-D clippy::question-mark` implied by `-D warnings` + +error: this `let...else` may be rewritten with the `?` operator + --> $DIR/manual_let_else_question_mark.rs:36:5 + | +LL | let Some((v, w)) = g() else { return None }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `let (v, w) = g()?;` + +error: this block may be rewritten with the `?` operator + --> $DIR/manual_let_else_question_mark.rs:39:13 + | +LL | let v = if let Some(v_some) = g() { v_some } else { return None }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `g()?` + +error: this could be rewritten as `let...else` + --> $DIR/manual_let_else_question_mark.rs:43:5 + | +LL | / let v = if let Some(v_some) = g() { +LL | | v_some +LL | | } else { +LL | | return Some(()); +LL | | }; + | |______^ + | + = note: `-D clippy::manual-let-else` implied by `-D warnings` +help: consider writing + | +LL ~ let Some(v) = g() else { +LL + return Some(()); +LL + }; + | + +error: this could be rewritten as `let...else` + --> $DIR/manual_let_else_question_mark.rs:54:9 + | +LL | / let v = match g() { +LL | | Some(v_some) => v_some, +LL | | _ => return None, +LL | | }; + | |__________^ help: consider writing: `let Some(v) = g() else { return None };` + +error: aborting due to 5 previous errors + From c6be62159ff6c100786a86b6a5687ded883ace5f Mon Sep 17 00:00:00 2001 From: est31 Date: Sun, 11 Jun 2023 07:11:47 +0200 Subject: [PATCH 2/5] Fix the now stricter lint in manual_rem_euclid.rs --- clippy_lints/src/manual_rem_euclid.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/manual_rem_euclid.rs b/clippy_lints/src/manual_rem_euclid.rs index aafee92713fe..0e89ca132ef9 100644 --- a/clippy_lints/src/manual_rem_euclid.rs +++ b/clippy_lints/src/manual_rem_euclid.rs @@ -119,7 +119,7 @@ fn check_for_either_unsigned_int_constant<'a>( } fn check_for_unsigned_int_constant<'a>(cx: &'a LateContext<'_>, expr: &'a Expr<'_>) -> Option { - let Some(int_const) = constant_full_int(cx, cx.typeck_results(), expr) else { return None }; + let int_const = constant_full_int(cx, cx.typeck_results(), expr)?; match int_const { FullInt::S(s) => s.try_into().ok(), FullInt::U(u) => Some(u), From 20dfaba0351d81748f2c1bbbb8a5e1d8bd77c3cb Mon Sep 17 00:00:00 2001 From: est31 Date: Fri, 30 Jun 2023 18:41:10 +0200 Subject: [PATCH 3/5] Put into one pass --- clippy_lints/src/lib.rs | 3 +- clippy_lints/src/manual_let_else.rs | 77 +++-------------------------- clippy_lints/src/question_mark.rs | 72 ++++++++++++++++++++++++--- 3 files changed, 74 insertions(+), 78 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 5ab28b5c70c1..771f13bda15f 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -662,7 +662,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: }); store.register_late_pass(move |_| Box::new(matches::Matches::new(msrv()))); let matches_for_let_else = conf.matches_for_let_else; - store.register_late_pass(move |_| Box::new(manual_let_else::ManualLetElse::new(msrv(), matches_for_let_else))); store.register_early_pass(move || Box::new(manual_non_exhaustive::ManualNonExhaustiveStruct::new(msrv()))); store.register_late_pass(move |_| Box::new(manual_non_exhaustive::ManualNonExhaustiveEnum::new(msrv()))); store.register_late_pass(move |_| Box::new(manual_strip::ManualStrip::new(msrv()))); @@ -771,7 +770,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|_| Box::::default()); store.register_late_pass(|_| Box::new(implicit_hasher::ImplicitHasher)); store.register_late_pass(|_| Box::new(fallible_impl_from::FallibleImplFrom)); - store.register_late_pass(|_| Box::::default()); + store.register_late_pass(move |_| Box::new(question_mark::QuestionMark::new(msrv(), matches_for_let_else))); store.register_late_pass(|_| Box::new(question_mark_used::QuestionMarkUsed)); store.register_early_pass(|| Box::new(suspicious_operation_groupings::SuspiciousOperationGroupings)); store.register_late_pass(|_| Box::new(suspicious_trait_impl::SuspiciousImpl)); diff --git a/clippy_lints/src/manual_let_else.rs b/clippy_lints/src/manual_let_else.rs index dcfc365a3483..363018f13819 100644 --- a/clippy_lints/src/manual_let_else.rs +++ b/clippy_lints/src/manual_let_else.rs @@ -1,19 +1,19 @@ +use crate::question_mark::{pat_and_expr_can_be_question_mark, QuestionMark}; use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::higher::IfLetOrMatch; -use clippy_utils::msrvs::{self, Msrv}; +use clippy_utils::msrvs; +use clippy_utils::peel_blocks; use clippy_utils::source::snippet_with_context; use clippy_utils::ty::is_type_diagnostic_item; use clippy_utils::visitors::{Descend, Visitable}; -use clippy_utils::{is_refutable, is_res_lang_ctor, peel_blocks}; use if_chain::if_chain; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_errors::Applicability; use rustc_hir::intravisit::{walk_expr, Visitor}; -use rustc_hir::LangItem::{OptionNone, OptionSome}; use rustc_hir::{Expr, ExprKind, HirId, ItemId, Local, MatchSource, Pat, PatKind, QPath, Stmt, StmtKind, Ty}; -use rustc_lint::{LateContext, LateLintPass, LintContext}; +use rustc_lint::{LateContext, LintContext}; use rustc_middle::lint::in_external_macro; -use rustc_session::{declare_tool_lint, impl_lint_pass}; +use rustc_session::declare_tool_lint; use rustc_span::symbol::{sym, Symbol}; use rustc_span::Span; use serde::Deserialize; @@ -51,25 +51,8 @@ declare_clippy_lint! { "manual implementation of a let...else statement" } -pub struct ManualLetElse { - msrv: Msrv, - matches_behaviour: MatchLintBehaviour, -} - -impl ManualLetElse { - #[must_use] - pub fn new(msrv: Msrv, matches_behaviour: MatchLintBehaviour) -> Self { - Self { - msrv, - matches_behaviour, - } - } -} - -impl_lint_pass!(ManualLetElse => [MANUAL_LET_ELSE]); - -impl<'tcx> LateLintPass<'tcx> for ManualLetElse { - fn check_stmt(&mut self, cx: &LateContext<'_>, stmt: &'tcx Stmt<'tcx>) { +impl<'tcx> QuestionMark { + pub(crate) fn check_manual_let_else(&mut self, cx: &LateContext<'_>, stmt: &'tcx Stmt<'tcx>) { if !self.msrv.meets(msrvs::LET_ELSE) || in_external_macro(cx.sess(), stmt.span) { return; } @@ -130,8 +113,6 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse { } }; } - - extract_msrv_attr!(LateContext); } fn emit_manual_let_else( @@ -169,50 +150,6 @@ fn emit_manual_let_else( ); } -/// Returns whether the given let pattern and else body can be turned into a question mark -/// -/// For this example: -/// ```ignore -/// let FooBar { a, b } = if let Some(a) = ex { a } else { return None }; -/// ``` -/// We get as parameters: -/// ```ignore -/// pat: Some(a) -/// else_body: return None -/// ``` - -/// And for this example: -/// ```ignore -/// let Some(FooBar { a, b }) = ex else { return None }; -/// ``` -/// We get as parameters: -/// ```ignore -/// pat: Some(FooBar { a, b }) -/// else_body: return None -/// ``` - -/// We output `Some(a)` in the first instance, and `Some(FooBar { a, b })` in the second, because -/// the question mark operator is applicable here. Callers have to check whether we are in a -/// constant or not. -pub fn pat_and_expr_can_be_question_mark<'a, 'hir>( - cx: &LateContext<'_>, - pat: &'a Pat<'hir>, - else_body: &Expr<'_>, -) -> Option<&'a Pat<'hir>> { - if let PatKind::TupleStruct(pat_path, [inner_pat], _) = pat.kind && - is_res_lang_ctor(cx, cx.qpath_res(&pat_path, pat.hir_id), OptionSome) && - !is_refutable(cx, inner_pat) && - let else_body = peel_blocks(else_body) && - let ExprKind::Ret(Some(ret_val)) = else_body.kind && - let ExprKind::Path(ret_path) = ret_val.kind && - is_res_lang_ctor(cx, cx.qpath_res(&ret_path, ret_val.hir_id), OptionNone) - { - Some(inner_pat) - } else { - None - } -} - /// Replaces the locals in the pattern /// /// For this example: diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index b1eeedc26bdb..d473b6fd5c43 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -1,10 +1,11 @@ -use crate::manual_let_else::pat_and_expr_can_be_question_mark; +use crate::manual_let_else::{MatchLintBehaviour, MANUAL_LET_ELSE}; use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::msrvs::Msrv; use clippy_utils::source::snippet_with_applicability; use clippy_utils::ty::is_type_diagnostic_item; use clippy_utils::{ - eq_expr_value, get_parent_node, in_constant, is_else_clause, is_res_lang_ctor, path_to_local, path_to_local_id, - peel_blocks, peel_blocks_with_stmt, + eq_expr_value, get_parent_node, in_constant, is_else_clause, is_refutable, is_res_lang_ctor, path_to_local, + path_to_local_id, peel_blocks, peel_blocks_with_stmt, }; use clippy_utils::{higher, is_path_lang_item}; use if_chain::if_chain; @@ -12,7 +13,7 @@ use rustc_errors::Applicability; use rustc_hir::def::Res; use rustc_hir::LangItem::{self, OptionNone, OptionSome, ResultErr, ResultOk}; use rustc_hir::{ - BindingAnnotation, Block, ByRef, Expr, ExprKind, Local, Node, PatKind, PathSegment, QPath, Stmt, StmtKind, + BindingAnnotation, Block, ByRef, Expr, ExprKind, Local, Node, Pat, PatKind, PathSegment, QPath, Stmt, StmtKind, }; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::Ty; @@ -45,8 +46,9 @@ declare_clippy_lint! { "checks for expressions that could be replaced by the question mark operator" } -#[derive(Default)] pub struct QuestionMark { + pub(crate) msrv: Msrv, + pub(crate) matches_behaviour: MatchLintBehaviour, /// Keeps track of how many try blocks we are in at any point during linting. /// This allows us to answer the question "are we inside of a try block" /// very quickly, without having to walk up the parent chain, by simply checking @@ -54,7 +56,19 @@ pub struct QuestionMark { /// As for why we need this in the first place: try_block_depth_stack: Vec, } -impl_lint_pass!(QuestionMark => [QUESTION_MARK]); + +impl_lint_pass!(QuestionMark => [QUESTION_MARK, MANUAL_LET_ELSE]); + +impl QuestionMark { + #[must_use] + pub fn new(msrv: Msrv, matches_behaviour: MatchLintBehaviour) -> Self { + Self { + msrv, + matches_behaviour, + try_block_depth_stack: Vec::new(), + } + } +} enum IfBlockType<'hir> { /// An `if x.is_xxx() { a } else { b } ` expression. @@ -81,6 +95,50 @@ enum IfBlockType<'hir> { ), } +/// Returns whether the given let pattern and else body can be turned into a question mark +/// +/// For this example: +/// ```ignore +/// let FooBar { a, b } = if let Some(a) = ex { a } else { return None }; +/// ``` +/// We get as parameters: +/// ```ignore +/// pat: Some(a) +/// else_body: return None +/// ``` + +/// And for this example: +/// ```ignore +/// let Some(FooBar { a, b }) = ex else { return None }; +/// ``` +/// We get as parameters: +/// ```ignore +/// pat: Some(FooBar { a, b }) +/// else_body: return None +/// ``` + +/// We output `Some(a)` in the first instance, and `Some(FooBar { a, b })` in the second, because +/// the question mark operator is applicable here. Callers have to check whether we are in a +/// constant or not. +pub(crate) fn pat_and_expr_can_be_question_mark<'a, 'hir>( + cx: &LateContext<'_>, + pat: &'a Pat<'hir>, + else_body: &Expr<'_>, +) -> Option<&'a Pat<'hir>> { + if let PatKind::TupleStruct(pat_path, [inner_pat], _) = pat.kind && + is_res_lang_ctor(cx, cx.qpath_res(&pat_path, pat.hir_id), OptionSome) && + !is_refutable(cx, inner_pat) && + let else_body = peel_blocks(else_body) && + let ExprKind::Ret(Some(ret_val)) = else_body.kind && + let ExprKind::Path(ret_path) = ret_val.kind && + is_res_lang_ctor(cx, cx.qpath_res(&ret_path, ret_val.hir_id), OptionNone) + { + Some(inner_pat) + } else { + None + } +} + fn check_let_some_else_return_none(cx: &LateContext<'_>, stmt: &Stmt<'_>) { if let StmtKind::Local(Local { pat, init: Some(init_expr), els: Some(els), .. }) = stmt.kind && let Block { stmts: &[], expr: Some(els), .. } = els && @@ -289,6 +347,7 @@ impl<'tcx> LateLintPass<'tcx> for QuestionMark { if !in_constant(cx, stmt.hir_id) { check_let_some_else_return_none(cx, stmt); } + self.check_manual_let_else(cx, stmt); } fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) { if !in_constant(cx, expr.hir_id) { @@ -322,4 +381,5 @@ impl<'tcx> LateLintPass<'tcx> for QuestionMark { .expect("blocks are always part of bodies and must have a depth") -= 1; } } + extract_msrv_attr!(LateContext); } From 6990eaa9720e45b3b09eb00d9476b89839935c10 Mon Sep 17 00:00:00 2001 From: est31 Date: Sun, 2 Jul 2023 04:03:20 +0200 Subject: [PATCH 4/5] Don't suppress manual_let_else if question_mark is allowed If question_mark is allowed, there is no overlap any more, so we can just not suppress it. --- clippy_lints/src/manual_let_else.rs | 20 +++++++++---------- tests/ui/manual_let_else_question_mark.fixed | 7 +++++++ tests/ui/manual_let_else_question_mark.rs | 7 +++++++ tests/ui/manual_let_else_question_mark.stderr | 8 +++++++- 4 files changed, 31 insertions(+), 11 deletions(-) diff --git a/clippy_lints/src/manual_let_else.rs b/clippy_lints/src/manual_let_else.rs index 363018f13819..18739e9d8e20 100644 --- a/clippy_lints/src/manual_let_else.rs +++ b/clippy_lints/src/manual_let_else.rs @@ -1,12 +1,10 @@ -use crate::question_mark::{pat_and_expr_can_be_question_mark, QuestionMark}; +use crate::question_mark::{pat_and_expr_can_be_question_mark, QuestionMark, QUESTION_MARK}; use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::higher::IfLetOrMatch; -use clippy_utils::msrvs; -use clippy_utils::peel_blocks; use clippy_utils::source::snippet_with_context; use clippy_utils::ty::is_type_diagnostic_item; use clippy_utils::visitors::{Descend, Visitable}; -use if_chain::if_chain; +use clippy_utils::{is_lint_allowed, msrvs, peel_blocks}; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_errors::Applicability; use rustc_hir::intravisit::{walk_expr, Visitor}; @@ -65,12 +63,14 @@ impl<'tcx> QuestionMark { let Some(if_let_or_match) = IfLetOrMatch::parse(cx, init) { match if_let_or_match { - IfLetOrMatch::IfLet(if_let_expr, let_pat, if_then, if_else) => if_chain! { - if let Some(ident_map) = expr_simple_identity_map(local.pat, let_pat, if_then); - if let Some(if_else) = if_else; - if expr_diverges(cx, if_else); - if pat_and_expr_can_be_question_mark(cx, let_pat, if_else).is_none(); - then { + IfLetOrMatch::IfLet(if_let_expr, let_pat, if_then, if_else) => { + if + let Some(ident_map) = expr_simple_identity_map(local.pat, let_pat, if_then) && + let Some(if_else) = if_else && + expr_diverges(cx, if_else) && + let qm_allowed = is_lint_allowed(cx, QUESTION_MARK, stmt.hir_id) && + (qm_allowed || pat_and_expr_can_be_question_mark(cx, let_pat, if_else).is_none()) + { emit_manual_let_else(cx, stmt.span, if_let_expr, &ident_map, let_pat, if_else); } }, diff --git a/tests/ui/manual_let_else_question_mark.fixed b/tests/ui/manual_let_else_question_mark.fixed index 617f127b9a86..02308bc7c4c1 100644 --- a/tests/ui/manual_let_else_question_mark.fixed +++ b/tests/ui/manual_let_else_question_mark.fixed @@ -52,5 +52,12 @@ fn foo() -> Option<()> { let Some(v) = g() else { return None }; } + // This is a copy of the case above where we'd fire the question_mark lint, but here we have allowed + // it. Make sure that manual_let_else is fired as the fallback. + #[allow(clippy::question_mark)] + { + let Some(v) = g() else { return None }; + } + Some(()) } diff --git a/tests/ui/manual_let_else_question_mark.rs b/tests/ui/manual_let_else_question_mark.rs index 3cf1eca86734..9c7ad386dc99 100644 --- a/tests/ui/manual_let_else_question_mark.rs +++ b/tests/ui/manual_let_else_question_mark.rs @@ -57,5 +57,12 @@ fn foo() -> Option<()> { }; } + // This is a copy of the case above where we'd fire the question_mark lint, but here we have allowed + // it. Make sure that manual_let_else is fired as the fallback. + #[allow(clippy::question_mark)] + { + let v = if let Some(v_some) = g() { v_some } else { return None }; + } + Some(()) } diff --git a/tests/ui/manual_let_else_question_mark.stderr b/tests/ui/manual_let_else_question_mark.stderr index 31052acf75cd..d7d2e127ea3f 100644 --- a/tests/ui/manual_let_else_question_mark.stderr +++ b/tests/ui/manual_let_else_question_mark.stderr @@ -45,5 +45,11 @@ LL | | _ => return None, LL | | }; | |__________^ help: consider writing: `let Some(v) = g() else { return None };` -error: aborting due to 5 previous errors +error: this could be rewritten as `let...else` + --> $DIR/manual_let_else_question_mark.rs:64:9 + | +LL | let v = if let Some(v_some) = g() { v_some } else { return None }; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: consider writing: `let Some(v) = g() else { return None };` + +error: aborting due to 6 previous errors From d80581c7d2db05a899e3363c8fc937012311de69 Mon Sep 17 00:00:00 2001 From: est31 Date: Mon, 3 Jul 2023 09:42:43 +0200 Subject: [PATCH 5/5] Move pat_and_expr_can_be_question_mark into clippy_utils --- clippy_lints/src/manual_let_else.rs | 4 +-- clippy_lints/src/question_mark.rs | 50 ++--------------------------- clippy_utils/src/lib.rs | 45 ++++++++++++++++++++++++++ 3 files changed, 50 insertions(+), 49 deletions(-) diff --git a/clippy_lints/src/manual_let_else.rs b/clippy_lints/src/manual_let_else.rs index 18739e9d8e20..ade85e23bd3a 100644 --- a/clippy_lints/src/manual_let_else.rs +++ b/clippy_lints/src/manual_let_else.rs @@ -1,10 +1,10 @@ -use crate::question_mark::{pat_and_expr_can_be_question_mark, QuestionMark, QUESTION_MARK}; +use crate::question_mark::{QuestionMark, QUESTION_MARK}; use clippy_utils::diagnostics::span_lint_and_then; use clippy_utils::higher::IfLetOrMatch; use clippy_utils::source::snippet_with_context; use clippy_utils::ty::is_type_diagnostic_item; use clippy_utils::visitors::{Descend, Visitable}; -use clippy_utils::{is_lint_allowed, msrvs, peel_blocks}; +use clippy_utils::{is_lint_allowed, msrvs, pat_and_expr_can_be_question_mark, peel_blocks}; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_errors::Applicability; use rustc_hir::intravisit::{walk_expr, Visitor}; diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index d473b6fd5c43..9840d464b75f 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -4,8 +4,8 @@ use clippy_utils::msrvs::Msrv; use clippy_utils::source::snippet_with_applicability; use clippy_utils::ty::is_type_diagnostic_item; use clippy_utils::{ - eq_expr_value, get_parent_node, in_constant, is_else_clause, is_refutable, is_res_lang_ctor, path_to_local, - path_to_local_id, peel_blocks, peel_blocks_with_stmt, + eq_expr_value, get_parent_node, in_constant, is_else_clause, is_res_lang_ctor, pat_and_expr_can_be_question_mark, + path_to_local, path_to_local_id, peel_blocks, peel_blocks_with_stmt, }; use clippy_utils::{higher, is_path_lang_item}; use if_chain::if_chain; @@ -13,7 +13,7 @@ use rustc_errors::Applicability; use rustc_hir::def::Res; use rustc_hir::LangItem::{self, OptionNone, OptionSome, ResultErr, ResultOk}; use rustc_hir::{ - BindingAnnotation, Block, ByRef, Expr, ExprKind, Local, Node, Pat, PatKind, PathSegment, QPath, Stmt, StmtKind, + BindingAnnotation, Block, ByRef, Expr, ExprKind, Local, Node, PatKind, PathSegment, QPath, Stmt, StmtKind, }; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::Ty; @@ -95,50 +95,6 @@ enum IfBlockType<'hir> { ), } -/// Returns whether the given let pattern and else body can be turned into a question mark -/// -/// For this example: -/// ```ignore -/// let FooBar { a, b } = if let Some(a) = ex { a } else { return None }; -/// ``` -/// We get as parameters: -/// ```ignore -/// pat: Some(a) -/// else_body: return None -/// ``` - -/// And for this example: -/// ```ignore -/// let Some(FooBar { a, b }) = ex else { return None }; -/// ``` -/// We get as parameters: -/// ```ignore -/// pat: Some(FooBar { a, b }) -/// else_body: return None -/// ``` - -/// We output `Some(a)` in the first instance, and `Some(FooBar { a, b })` in the second, because -/// the question mark operator is applicable here. Callers have to check whether we are in a -/// constant or not. -pub(crate) fn pat_and_expr_can_be_question_mark<'a, 'hir>( - cx: &LateContext<'_>, - pat: &'a Pat<'hir>, - else_body: &Expr<'_>, -) -> Option<&'a Pat<'hir>> { - if let PatKind::TupleStruct(pat_path, [inner_pat], _) = pat.kind && - is_res_lang_ctor(cx, cx.qpath_res(&pat_path, pat.hir_id), OptionSome) && - !is_refutable(cx, inner_pat) && - let else_body = peel_blocks(else_body) && - let ExprKind::Ret(Some(ret_val)) = else_body.kind && - let ExprKind::Path(ret_path) = ret_val.kind && - is_res_lang_ctor(cx, cx.qpath_res(&ret_path, ret_val.hir_id), OptionNone) - { - Some(inner_pat) - } else { - None - } -} - fn check_let_some_else_return_none(cx: &LateContext<'_>, stmt: &Stmt<'_>) { if let StmtKind::Local(Local { pat, init: Some(init_expr), els: Some(els), .. }) = stmt.kind && let Block { stmts: &[], expr: Some(els), .. } = els && diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs index 727b59f1f432..02ab1b31e72a 100644 --- a/clippy_utils/src/lib.rs +++ b/clippy_utils/src/lib.rs @@ -87,6 +87,7 @@ use rustc_hir::def::{DefKind, Res}; use rustc_hir::def_id::{CrateNum, DefId, LocalDefId, LOCAL_CRATE}; use rustc_hir::hir_id::{HirIdMap, HirIdSet}; use rustc_hir::intravisit::{walk_expr, FnKind, Visitor}; +use rustc_hir::LangItem::OptionSome; use rustc_hir::LangItem::{OptionNone, ResultErr, ResultOk}; use rustc_hir::{ self as hir, def, Arm, ArrayLen, BindingAnnotation, Block, BlockCheckMode, Body, Closure, Destination, Expr, @@ -2542,6 +2543,50 @@ pub fn span_find_starting_semi(sm: &SourceMap, span: Span) -> Span { sm.span_take_while(span, |&ch| ch == ' ' || ch == ';') } +/// Returns whether the given let pattern and else body can be turned into a question mark +/// +/// For this example: +/// ```ignore +/// let FooBar { a, b } = if let Some(a) = ex { a } else { return None }; +/// ``` +/// We get as parameters: +/// ```ignore +/// pat: Some(a) +/// else_body: return None +/// ``` + +/// And for this example: +/// ```ignore +/// let Some(FooBar { a, b }) = ex else { return None }; +/// ``` +/// We get as parameters: +/// ```ignore +/// pat: Some(FooBar { a, b }) +/// else_body: return None +/// ``` + +/// We output `Some(a)` in the first instance, and `Some(FooBar { a, b })` in the second, because +/// the question mark operator is applicable here. Callers have to check whether we are in a +/// constant or not. +pub fn pat_and_expr_can_be_question_mark<'a, 'hir>( + cx: &LateContext<'_>, + pat: &'a Pat<'hir>, + else_body: &Expr<'_>, +) -> Option<&'a Pat<'hir>> { + if let PatKind::TupleStruct(pat_path, [inner_pat], _) = pat.kind && + is_res_lang_ctor(cx, cx.qpath_res(&pat_path, pat.hir_id), OptionSome) && + !is_refutable(cx, inner_pat) && + let else_body = peel_blocks(else_body) && + let ExprKind::Ret(Some(ret_val)) = else_body.kind && + let ExprKind::Path(ret_path) = ret_val.kind && + is_res_lang_ctor(cx, cx.qpath_res(&ret_path, ret_val.hir_id), OptionNone) + { + Some(inner_pat) + } else { + None + } +} + macro_rules! op_utils { ($($name:ident $assign:ident)*) => { /// Binary operation traits like `LangItem::Add`