diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs index e67ea1f5e370..1ce4ef59817a 100644 --- a/clippy_lints/src/question_mark.rs +++ b/clippy_lints/src/question_mark.rs @@ -2,10 +2,10 @@ use crate::manual_let_else::MANUAL_LET_ELSE; use crate::question_mark_used::QUESTION_MARK_USED; use clippy_config::Conf; use clippy_config::types::MatchLintBehaviour; -use clippy_utils::diagnostics::span_lint_and_sugg; +use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then}; use clippy_utils::msrvs::{self, Msrv}; use clippy_utils::res::{MaybeDef, MaybeQPath, MaybeResPath}; -use clippy_utils::source::snippet_with_applicability; +use clippy_utils::source::{reindent_multiline, snippet_indent, snippet_with_applicability, snippet_with_context}; use clippy_utils::sugg::Sugg; use clippy_utils::ty::{implements_trait, is_copy}; use clippy_utils::usage::local_used_after_expr; @@ -17,13 +17,15 @@ use clippy_utils::{ use rustc_errors::Applicability; use rustc_hir::LangItem::{self, OptionNone, OptionSome, ResultErr, ResultOk}; use rustc_hir::def::Res; +use rustc_hir::def_id::LocalDefId; 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, intravisit, }; -use rustc_lint::{LateContext, LateLintPass}; +use rustc_lint::{LateContext, LateLintPass, LintContext}; use rustc_middle::ty::{self, Ty}; use rustc_session::impl_lint_pass; +use rustc_span::Span; use rustc_span::symbol::Symbol; declare_clippy_lint! { @@ -524,6 +526,63 @@ 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 !expr.span.in_external_macro(cx.sess().source_map()) + && 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) + { + let mut applicability = Applicability::MachineApplicable; + let receiver_str = snippet_with_context(cx, let_expr.span, expr.span.ctxt(), "..", &mut applicability).0; + let method_call_str = match by_ref { + ByRef::Yes(Mutability::Mut) => ".as_mut()", + ByRef::Yes(Mutability::Not) => ".as_ref()", + ByRef::No => "", + }; + let indent = snippet_indent(cx, if_then.span).unwrap_or_default(); + let body_str = { + let snippet = snippet_with_applicability(cx, if_then.span, "..", &mut applicability); + let Some(snippet) = snippet.strip_prefix('{').and_then(|s| s.strip_suffix('}')) else { + return; + }; + // There probably was a newline before the `}`, and it adds an extra line to the suggestion + // -- remove it + let snippet = snippet.trim_end(); + reindent_multiline(snippet, true, Some(indent.len())) + }; + let sugg = format!("let {ident} = {receiver_str}{method_call_str}?;{body_str}"); + span_lint_and_then( + cx, + QUESTION_MARK, + expr.span, + "this block may be rewritten with the `?` operator", + |diag| { + diag.span_suggestion_verbose(expr.span, "replace it with", sugg, applicability); + }, + ); + } +} + impl QuestionMark { fn inside_try_block(&self) -> bool { self.try_block_depth_stack.last() > Some(&0) @@ -603,6 +662,42 @@ 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 + fn check_fn( + &mut self, + cx: &LateContext<'tcx>, + _: intravisit::FnKind<'tcx>, + _: &'tcx FnDecl<'tcx>, + body: &'tcx Body<'tcx>, + _: Span, + local_def_id: LocalDefId, + ) { + if !is_in_const_context(cx) + && is_lint_allowed(cx, QUESTION_MARK_USED, HirId::make_owner(local_def_id)) + && self.msrv.meets(cx, msrvs::QUESTION_MARK_OPERATOR) + && let Some(return_expr) = match body.value.kind { + #[expect( + clippy::unnecessary_lazy_evaluations, + reason = "there are a bunch of if-let's, why should we evaluate them eagerly? Arguably an FP" + )] + ExprKind::Block(block, _) => block.expr.or_else(|| { + if let [.., stmt] = block.stmts + && let StmtKind::Semi(expr) = stmt.kind + && let ExprKind::Ret(Some(ret)) = expr.kind + { + Some(ret) + } else { + None + } + }), + _ => None, + } + { + check_if_let_some_as_return_val(cx, return_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 2c5ee0245038..4bd2d7230af7 100644 --- a/tests/ui/question_mark.fixed +++ b/tests/ui/question_mark.fixed @@ -1,5 +1,5 @@ #![feature(try_blocks)] -#![allow(clippy::unnecessary_wraps)] +#![allow(clippy::unnecessary_wraps, clippy::let_unit_value)] use std::sync::MutexGuard; @@ -500,3 +500,91 @@ mod issue14894 { Ok(()) } } + +mod issue13626 { + fn basic(x: Option) -> Option { + let x = x?; + //~^ question_mark + dbg!(x); + Some(x * 2) + } + + fn mut_ref(mut x: Option) -> Option { + let x = x.as_mut()?; + //~^ question_mark + dbg!(*x); + Some(*x * 2) + } + + #[allow(clippy::needless_return)] + fn explicit_return(x: Option) -> Option { + let x = x?; + //~^ question_mark + dbg!(x); + return Some(x * 2); + } + + #[deny(clippy::question_mark_used)] + fn question_mark_forbidden(x: Option) -> Option { + if let Some(x) = x { + dbg!(x); + Some(x * 2) + } else { + None + } + } + + #[clippy::msrv = "1.12"] + fn msrv_1_12(x: Option) -> Option { + if let Some(x) = x { + dbg!(x); + Some(x * 2) + } else { + None + } + } + + #[clippy::msrv = "1.13"] + fn msrv_1_13(x: Option) -> Option { + let x = x?; + //~^ question_mark + dbg!(x); + Some(x * 2) + } + + const fn no_lint_in_const(x: Option) -> Option { + if let Some(x) = x { + let x = x + 2; + Some(x * 2) + } else { + None + } + } + + fn with_macro() -> Option<()> { + macro_rules! m { + () => { + Some(()) + }; + }; + + let x = m!()?; + //~^ question_mark + dbg!(x); + Some(x) + } + + #[rustfmt::skip] + mod bad_formatting{ + fn t1(x: Option) -> Option { + let x = x?; dbg!(x); Some(x * 2) + //~^ question_mark + } + fn t2(x: Option) -> Option { + let x = x?; + //~^^ question_mark + dbg!(x); + Some(x * 2) + } + } +} diff --git a/tests/ui/question_mark.rs b/tests/ui/question_mark.rs index b9ff9d1565b2..e903f551abbf 100644 --- a/tests/ui/question_mark.rs +++ b/tests/ui/question_mark.rs @@ -1,5 +1,5 @@ #![feature(try_blocks)] -#![allow(clippy::unnecessary_wraps)] +#![allow(clippy::unnecessary_wraps, clippy::let_unit_value)] use std::sync::MutexGuard; @@ -615,3 +615,112 @@ mod issue14894 { Ok(()) } } + +mod issue13626 { + fn basic(x: Option) -> Option { + if let Some(x) = x { + //~^ question_mark + dbg!(x); + Some(x * 2) + } else { + None + } + } + + fn mut_ref(mut x: Option) -> Option { + if let Some(ref mut x) = x { + //~^ question_mark + dbg!(*x); + Some(*x * 2) + } else { + None + } + } + + #[allow(clippy::needless_return)] + fn explicit_return(x: Option) -> Option { + if let Some(x) = x { + //~^ question_mark + dbg!(x); + return Some(x * 2); + } else { + return None; + } + } + + #[deny(clippy::question_mark_used)] + fn question_mark_forbidden(x: Option) -> Option { + if let Some(x) = x { + dbg!(x); + Some(x * 2) + } else { + None + } + } + + #[clippy::msrv = "1.12"] + fn msrv_1_12(x: Option) -> Option { + if let Some(x) = x { + dbg!(x); + Some(x * 2) + } else { + None + } + } + + #[clippy::msrv = "1.13"] + fn msrv_1_13(x: Option) -> Option { + if let Some(x) = x { + //~^ question_mark + dbg!(x); + Some(x * 2) + } else { + None + } + } + + const fn no_lint_in_const(x: Option) -> Option { + if let Some(x) = x { + let x = x + 2; + Some(x * 2) + } else { + None + } + } + + fn with_macro() -> Option<()> { + macro_rules! m { + () => { + Some(()) + }; + }; + + if let Some(x) = m!() { + //~^ question_mark + dbg!(x); + Some(x) + } else { + None + } + } + + #[rustfmt::skip] + mod bad_formatting{ + fn t1(x: Option) -> Option { + if let Some(x) = x { dbg!(x); Some(x * 2) } else { None } + //~^ question_mark + } + fn t2(x: Option) -> Option { + if let Some(x) = x + { + //~^^ question_mark + dbg!(x); + Some(x * 2) + } + else + { + None + } + } + } +} diff --git a/tests/ui/question_mark.stderr b/tests/ui/question_mark.stderr index 9b2896328e66..4e3743465a48 100644 --- a/tests/ui/question_mark.stderr +++ b/tests/ui/question_mark.stderr @@ -333,5 +333,137 @@ LL | | return Err(reason); LL | | } | |_________^ help: replace it with: `result?;` -error: aborting due to 35 previous errors +error: this block may be rewritten with the `?` operator + --> tests/ui/question_mark.rs:621:9 + | +LL | / if let Some(x) = x { +LL | | +LL | | dbg!(x); +LL | | Some(x * 2) +LL | | } else { +LL | | None +LL | | } + | |_________^ + | +help: replace it with + | +LL ~ let x = x?; +LL + +LL + dbg!(x); +LL + Some(x * 2) + | + +error: this block may be rewritten with the `?` operator + --> tests/ui/question_mark.rs:631:9 + | +LL | / if let Some(ref mut x) = x { +LL | | +LL | | dbg!(*x); +LL | | Some(*x * 2) +LL | | } else { +LL | | None +LL | | } + | |_________^ + | +help: replace it with + | +LL ~ let x = x.as_mut()?; +LL + +LL + dbg!(*x); +LL + Some(*x * 2) + | + +error: this block may be rewritten with the `?` operator + --> tests/ui/question_mark.rs:642:9 + | +LL | / if let Some(x) = x { +LL | | +LL | | dbg!(x); +LL | | return Some(x * 2); +LL | | } else { +LL | | return None; +LL | | } + | |_________^ + | +help: replace it with + | +LL ~ let x = x?; +LL + +LL + dbg!(x); +LL + return Some(x * 2); + | + +error: this block may be rewritten with the `?` operator + --> tests/ui/question_mark.rs:673:9 + | +LL | / if let Some(x) = x { +LL | | +LL | | dbg!(x); +LL | | Some(x * 2) +LL | | } else { +LL | | None +LL | | } + | |_________^ + | +help: replace it with + | +LL ~ let x = x?; +LL + +LL + dbg!(x); +LL + Some(x * 2) + | + +error: this block may be rewritten with the `?` operator + --> tests/ui/question_mark.rs:698:9 + | +LL | / if let Some(x) = m!() { +LL | | +LL | | dbg!(x); +LL | | Some(x) +LL | | } else { +LL | | None +LL | | } + | |_________^ + | +help: replace it with + | +LL ~ let x = m!()?; +LL + +LL + dbg!(x); +LL + Some(x) + | + +error: this block may be rewritten with the `?` operator + --> tests/ui/question_mark.rs:710:13 + | +LL | if let Some(x) = x { dbg!(x); Some(x * 2) } else { None } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | +help: replace it with + | +LL - if let Some(x) = x { dbg!(x); Some(x * 2) } else { None } +LL + let x = x?; dbg!(x); Some(x * 2) + | + +error: this block may be rewritten with the `?` operator + --> tests/ui/question_mark.rs:714:13 + | +LL | / if let Some(x) = x +LL | | { +LL | | +LL | | dbg!(x); +... | +LL | | None +LL | | } + | |_____________^ + | +help: replace it with + | +LL ~ let x = x?; +LL + +LL + dbg!(x); +LL + Some(x * 2) + | + +error: aborting due to 42 previous errors