diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
index 87329ee5e14..c43e0b753dc 100644
--- a/clippy_lints/src/lib.rs
+++ b/clippy_lints/src/lib.rs
@@ -663,7 +663,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())));
@@ -772,7 +771,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
store.register_late_pass(|_| Box::<useless_conversion::UselessConversion>::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::<question_mark::QuestionMark>::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 59e421c1622..ade85e23bd3 100644
--- a/clippy_lints/src/manual_let_else.rs
+++ b/clippy_lints/src/manual_let_else.rs
@@ -1,18 +1,17 @@
+use crate::question_mark::{QuestionMark, QUESTION_MARK};
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 if_chain::if_chain;
+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};
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;
@@ -50,25 +49,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;
}
@@ -81,11 +63,14 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse {
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);
- 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);
}
},
@@ -128,8 +113,6 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse {
}
};
}
-
- extract_msrv_attr!(LateContext);
}
fn emit_manual_let_else(
diff --git a/clippy_lints/src/manual_rem_euclid.rs b/clippy_lints/src/manual_rem_euclid.rs
index aafee92713f..0e89ca132ef 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<u128> {
- 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),
diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs
index e3d940ad2a4..9840d464b75 100644
--- a/clippy_lints/src/question_mark.rs
+++ b/clippy_lints/src/question_mark.rs
@@ -1,16 +1,20 @@
+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_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;
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;
@@ -42,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
@@ -51,7 +56,19 @@ pub struct QuestionMark {
/// As for why we need this in the first place: <https://github.com/rust-lang/rust-clippy/issues/8628>
try_block_depth_stack: Vec<u32>,
}
-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.
@@ -78,6 +95,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 +299,12 @@ 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);
+ }
+ 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) {
self.check_is_none_or_err_and_early_return(cx, expr);
@@ -291,4 +337,5 @@ impl<'tcx> LateLintPass<'tcx> for QuestionMark {
.expect("blocks are always part of bodies and must have a depth") -= 1;
}
}
+ extract_msrv_attr!(LateContext);
}
diff --git a/clippy_utils/src/lib.rs b/clippy_utils/src/lib.rs
index 272228cebd0..1ac106d5d59 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,
@@ -2543,6 +2544,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`
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 00000000000..02308bc7c4c
--- /dev/null
+++ b/tests/ui/manual_let_else_question_mark.fixed
@@ -0,0 +1,63 @@
+//@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 };
+ }
+
+ // 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
new file mode 100644
index 00000000000..9c7ad386dc9
--- /dev/null
+++ b/tests/ui/manual_let_else_question_mark.rs
@@ -0,0 +1,68 @@
+//@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,
+ };
+ }
+
+ // 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
new file mode 100644
index 00000000000..d7d2e127ea3
--- /dev/null
+++ b/tests/ui/manual_let_else_question_mark.stderr
@@ -0,0 +1,55 @@
+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: 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
+