diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index 4b96858d8f66..f7098401dcba 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -11,18 +11,21 @@ use clippy_utils::{ pat_and_expr_can_be_question_mark, path_res, path_to_local, path_to_local_id, peel_blocks, peel_blocks_with_stmt, span_contains_cfg, span_contains_comment, }; +use itertools::Itertools; use rustc_errors::Applicability; use rustc_hir::LangItem::{self, OptionNone, OptionSome, ResultErr, ResultOk}; use rustc_hir::def::Res; +use rustc_hir::intravisit::FnKind; use rustc_hir::{ - Arm, BindingMode, Block, Body, ByRef, Expr, ExprKind, FnRetTy, HirId, LetStmt, MatchSource, Mutability, Node, Pat, - PatKind, PathSegment, QPath, Stmt, StmtKind, + Arm, BindingMode, Block, Body, ByRef, Expr, ExprKind, FnDecl, FnRetTy, HirId, LetStmt, MatchSource, Mutability, + Node, Pat, PatKind, PathSegment, QPath, Stmt, StmtKind, }; use rustc_lint::{LateContext, LateLintPass}; use rustc_middle::ty::{self, Ty}; use rustc_session::impl_lint_pass; -use rustc_span::sym; +use rustc_span::def_id::LocalDefId; use rustc_span::symbol::Symbol; +use rustc_span::{Span, sym}; declare_clippy_lint! { /// ### What it does @@ -456,6 +459,61 @@ fn check_if_let_some_or_err_and_early_return<'tcx>(cx: &LateContext<'tcx>, expr: } } +fn check_if_let_some_as_return_val<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) { + if let Some(higher::IfLet { + let_pat, + let_expr, + if_then, + if_else, + .. + }) = higher::IfLet::hir(cx, expr) + && let PatKind::TupleStruct(ref path1, [field], ddpos) = let_pat.kind + && ddpos.as_opt_usize().is_none() + && let PatKind::Binding(BindingMode(by_ref, _), _, ident, None) = field.kind + && let caller_ty = cx.typeck_results().expr_ty(let_expr) + && let if_block = IfBlockType::IfLet( + cx.qpath_res(path1, let_pat.hir_id), + caller_ty, + ident.name, + let_expr, + if_then, + if_else, + ) + && let ExprKind::Block(if_then_block, _) = if_then.kind + // Don't consider case where if-then branch has only one statement/expression + && (if_then_block.stmts.len() >= 2 || if_then_block.stmts.len() == 1 && if_then_block.expr.is_some()) + && (is_early_return(sym::Option, cx, &if_block)) + && if_else + .map(|e| eq_expr_value(cx, let_expr, peel_blocks(e))) + .filter(|e| *e) + .is_none() + { + let mut applicability = Applicability::MachineApplicable; + let receiver_str = snippet_with_applicability(cx, let_expr.span, "..", &mut applicability); + let method_call_str = match by_ref { + ByRef::Yes(Mutability::Mut) => ".as_mut()", + ByRef::Yes(Mutability::Not) => ".as_ref()", + ByRef::No => "", + }; + + let body = snippet_with_applicability(cx, if_then.span, "..", &mut applicability); + let mut body_lines = body.lines().map(|line| &line[line.len().min(4)..]); + let (_, _) = (body_lines.next(), body_lines.next_back()); + let body_str = body_lines.join("\n"); + + let sugg = format!("let {ident} = {receiver_str}{method_call_str}?;\n{body_str}",); + span_lint_and_sugg( + cx, + QUESTION_MARK, + expr.span, + "this block may be rewritten with the `?` operator", + "replace it with", + sugg, + applicability, + ); + } +} + impl QuestionMark { fn inside_try_block(&self) -> bool { self.try_block_depth_stack.last() > Some(&0) @@ -531,6 +589,37 @@ impl<'tcx> LateLintPass<'tcx> for QuestionMark { self.try_block_depth_stack.push(0); } + // Defining check_fn instead of using check_body to avoid accidentally triggering on + // const expressions, not sure if this is necessary + fn check_fn( + &mut self, + cx: &LateContext<'tcx>, + _: FnKind<'tcx>, + _: &'tcx FnDecl<'tcx>, + body: &'tcx Body<'tcx>, + _: Span, + _: LocalDefId, + ) { + let return_expr = match body.value.kind { + ExprKind::Block(block, _) => block.expr.or_else(|| { + block.stmts.iter().last().and_then(|stmt| { + if let StmtKind::Semi(expr) = stmt.kind + && let ExprKind::Ret(Some(ret)) = expr.kind + { + Some(ret) + } else { + None + } + }) + }), + _ => None, + }; + + if let Some(expr) = return_expr { + check_if_let_some_as_return_val(cx, expr); + } + } + fn check_body_post(&mut self, _: &LateContext<'tcx>, _: &Body<'tcx>) { self.try_block_depth_stack.pop(); } diff --git a/tests/ui/question_mark.fixed b/tests/ui/question_mark.fixed index b6e148e9f772..0f666cf94ed3 100644 --- a/tests/ui/question_mark.fixed +++ b/tests/ui/question_mark.fixed @@ -373,3 +373,24 @@ fn issue12412(foo: &Foo, bar: &Bar) -> Option<()> { let v = bar.foo.owned.clone()?; Some(()) } + +mod issue13626 { + fn basic_test(x: Option) -> Option { + let x = x?; + dbg!(x); + Some(x * 2) + } + + fn mut_ref_test(mut x: Option) -> Option { + let x = x.as_mut()?; + dbg!(*x); + Some(*x * 2) + } + + #[allow(clippy::needless_return)] + fn explicit_return_test(x: Option) -> Option { + let x = x?; + dbg!(x); + return Some(x * 2); + } +} diff --git a/tests/ui/question_mark.rs b/tests/ui/question_mark.rs index 48dc9eb0a626..f6f8189d63c5 100644 --- a/tests/ui/question_mark.rs +++ b/tests/ui/question_mark.rs @@ -430,3 +430,33 @@ fn issue12412(foo: &Foo, bar: &Bar) -> Option<()> { }; Some(()) } + +mod issue13626 { + fn basic_test(x: Option) -> Option { + if let Some(x) = x { + dbg!(x); + Some(x * 2) + } else { + None + } + } + + fn mut_ref_test(mut x: Option) -> Option { + if let Some(ref mut x) = x { + dbg!(*x); + Some(*x * 2) + } else { + None + } + } + + #[allow(clippy::needless_return)] + fn explicit_return_test(x: Option) -> Option { + if let Some(x) = x { + dbg!(x); + return Some(x * 2); + } else { + return None; + } + } +} diff --git a/tests/ui/question_mark.stderr b/tests/ui/question_mark.stderr index 0a48c4e80cb6..4eee0966a5cd 100644 --- a/tests/ui/question_mark.stderr +++ b/tests/ui/question_mark.stderr @@ -196,5 +196,59 @@ LL | | return None; LL | | }; | |______^ help: replace it with: `let v = bar.foo.owned.clone()?;` -error: aborting due to 22 previous errors +error: this block may be rewritten with the `?` operator + --> tests/ui/question_mark.rs:436:9 + | +LL | / if let Some(x) = x { +LL | | dbg!(x); +LL | | Some(x * 2) +LL | | } else { +LL | | None +LL | | } + | |_________^ + | +help: replace it with + | +LL ~ let x = x?; +LL + dbg!(x); +LL + Some(x * 2) + | + +error: this block may be rewritten with the `?` operator + --> tests/ui/question_mark.rs:445:9 + | +LL | / if let Some(ref mut x) = x { +LL | | dbg!(*x); +LL | | Some(*x * 2) +LL | | } else { +LL | | None +LL | | } + | |_________^ + | +help: replace it with + | +LL ~ let x = x.as_mut()?; +LL + dbg!(*x); +LL + Some(*x * 2) + | + +error: this block may be rewritten with the `?` operator + --> tests/ui/question_mark.rs:455:9 + | +LL | / if let Some(x) = x { +LL | | dbg!(x); +LL | | return Some(x * 2); +LL | | } else { +LL | | return None; +LL | | } + | |_________^ + | +help: replace it with + | +LL ~ let x = x?; +LL + dbg!(x); +LL + return Some(x * 2); + | + +error: aborting due to 25 previous errors