Skip to content

Commit b981c55

Browse files
ada4aada4a
committed
feat(question_mark): lint if let Some(_) = _ { ..; _ } else { None } as final expr
1 parent 7b0c3d0 commit b981c55

File tree

4 files changed

+432
-8
lines changed

4 files changed

+432
-8
lines changed

‎clippy_lints/src/question_mark.rs‎

Lines changed: 100 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,9 @@ use crate::manual_let_else::MANUAL_LET_ELSE;
22
use crate::question_mark_used::QUESTION_MARK_USED;
33
use clippy_config::Conf;
44
use clippy_config::types::MatchLintBehaviour;
5-
use clippy_utils::diagnostics::span_lint_and_sugg;
5+
use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
66
use clippy_utils::msrvs::{self, Msrv};
7-
use clippy_utils::source::snippet_with_applicability;
7+
use clippy_utils::source::{reindent_multiline, snippet_indent, snippet_with_applicability, snippet_with_context};
88
use clippy_utils::sugg::Sugg;
99
use clippy_utils::ty::{implements_trait, is_copy, is_type_diagnostic_item};
1010
use clippy_utils::usage::local_used_after_expr;
@@ -16,13 +16,15 @@ use clippy_utils::{
1616
use rustc_errors::Applicability;
1717
use rustc_hir::LangItem::{self, OptionNone, OptionSome, ResultErr, ResultOk};
1818
use rustc_hir::def::Res;
19+
use rustc_hir::def_id::LocalDefId;
1920
use rustc_hir::{
20-
Arm, BindingMode, Block, Body, ByRef, Expr, ExprKind, FnRetTy, HirId, LetStmt, MatchSource, Mutability, Node, Pat,
21-
PatKind, PathSegment, QPath, Stmt, StmtKind,
21+
Arm, BindingMode, Block, Body, ByRef, Expr, ExprKind, FnDecl, FnRetTy, HirId, LetStmt, MatchSource, Mutability,
22+
Node, Pat, PatKind, PathSegment, QPath, Stmt, StmtKind, intravisit,
2223
};
23-
use rustc_lint::{LateContext, LateLintPass};
24+
use rustc_lint::{LateContext, LateLintPass, LintContext};
2425
use rustc_middle::ty::{self, Ty};
2526
use rustc_session::impl_lint_pass;
27+
use rustc_span::Span;
2628
use rustc_span::symbol::Symbol;
2729

2830
declare_clippy_lint! {
@@ -515,6 +517,63 @@ fn check_if_let_some_or_err_and_early_return<'tcx>(cx: &LateContext<'tcx>, expr:
515517
}
516518
}
517519

520+
fn check_if_let_some_as_return_val<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'tcx>) {
521+
if !expr.span.in_external_macro(cx.sess().source_map())
522+
&& let Some(higher::IfLet {
523+
let_pat,
524+
let_expr,
525+
if_then,
526+
if_else,
527+
..
528+
}) = higher::IfLet::hir(cx, expr)
529+
&& let PatKind::TupleStruct(ref path1, [field], ddpos) = let_pat.kind
530+
&& ddpos.as_opt_usize().is_none()
531+
&& let PatKind::Binding(BindingMode(by_ref, _), _, ident, None) = field.kind
532+
&& let caller_ty = cx.typeck_results().expr_ty(let_expr)
533+
&& let if_block = IfBlockType::IfLet(
534+
cx.qpath_res(path1, let_pat.hir_id),
535+
caller_ty,
536+
ident.name,
537+
let_expr,
538+
if_then,
539+
if_else,
540+
)
541+
&& let ExprKind::Block(if_then_block, _) = if_then.kind
542+
// Don't consider case where if-then branch has only one statement/expression
543+
&& (if_then_block.stmts.len() >= 2 || if_then_block.stmts.len() == 1 && if_then_block.expr.is_some())
544+
&& is_early_return(sym::Option, cx, &if_block)
545+
{
546+
let mut applicability = Applicability::MachineApplicable;
547+
let receiver_str = snippet_with_context(cx, let_expr.span, expr.span.ctxt(), "..", &mut applicability).0;
548+
let method_call_str = match by_ref {
549+
ByRef::Yes(Mutability::Mut) => ".as_mut()",
550+
ByRef::Yes(Mutability::Not) => ".as_ref()",
551+
ByRef::No => "",
552+
};
553+
let indent = snippet_indent(cx, if_then.span).unwrap_or_default();
554+
let body_str = {
555+
let snippet = snippet_with_applicability(cx, if_then.span, "..", &mut applicability);
556+
let Some(snippet) = snippet.strip_prefix('{').and_then(|s| s.strip_suffix('}')) else {
557+
return;
558+
};
559+
// There probably was a newline before the `}`, and it adds an extra line to the suggestion
560+
// -- remove it
561+
let snippet = snippet.trim_end();
562+
reindent_multiline(snippet, true, Some(indent.len()))
563+
};
564+
let sugg = format!("let {ident} = {receiver_str}{method_call_str}?;{body_str}");
565+
span_lint_and_then(
566+
cx,
567+
QUESTION_MARK,
568+
expr.span,
569+
"this block may be rewritten with the `?` operator",
570+
|diag| {
571+
diag.span_suggestion_verbose(expr.span, "replace it with", sugg, applicability);
572+
},
573+
);
574+
}
575+
}
576+
518577
impl QuestionMark {
519578
fn inside_try_block(&self) -> bool {
520579
self.try_block_depth_stack.last() > Some(&0)
@@ -594,6 +653,42 @@ impl<'tcx> LateLintPass<'tcx> for QuestionMark {
594653
self.try_block_depth_stack.push(0);
595654
}
596655

656+
// Defining `check_fn` instead of using `check_body` to avoid accidentally triggering on
657+
// const expressions
658+
fn check_fn(
659+
&mut self,
660+
cx: &LateContext<'tcx>,
661+
_: intravisit::FnKind<'tcx>,
662+
_: &'tcx FnDecl<'tcx>,
663+
body: &'tcx Body<'tcx>,
664+
_: Span,
665+
local_def_id: LocalDefId,
666+
) {
667+
if !is_in_const_context(cx)
668+
&& is_lint_allowed(cx, QUESTION_MARK_USED, HirId::make_owner(local_def_id))
669+
&& self.msrv.meets(cx, msrvs::QUESTION_MARK_OPERATOR)
670+
&& let Some(return_expr) = match body.value.kind {
671+
#[expect(
672+
clippy::unnecessary_lazy_evaluations,
673+
reason = "there are a bunch of if-let's, why should we evaluate them eagerly? Arguably an FP"
674+
)]
675+
ExprKind::Block(block, _) => block.expr.or_else(|| {
676+
if let [.., stmt] = block.stmts
677+
&& let StmtKind::Semi(expr) = stmt.kind
678+
&& let ExprKind::Ret(Some(ret)) = expr.kind
679+
{
680+
Some(ret)
681+
} else {
682+
None
683+
}
684+
}),
685+
_ => None,
686+
}
687+
{
688+
check_if_let_some_as_return_val(cx, return_expr);
689+
}
690+
}
691+
597692
fn check_body_post(&mut self, _: &LateContext<'tcx>, _: &Body<'tcx>) {
598693
self.try_block_depth_stack.pop();
599694
}

‎tests/ui/question_mark.fixed‎

Lines changed: 89 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
#![feature(try_blocks)]
2-
#![allow(clippy::unnecessary_wraps)]
2+
#![allow(clippy::unnecessary_wraps, clippy::let_unit_value)]
33

44
use std::sync::MutexGuard;
55

@@ -500,3 +500,91 @@ mod issue14894 {
500500
Ok(())
501501
}
502502
}
503+
504+
mod issue13626 {
505+
fn basic(x: Option<u32>) -> Option<u32> {
506+
let x = x?;
507+
//~^ question_mark
508+
dbg!(x);
509+
Some(x * 2)
510+
}
511+
512+
fn mut_ref(mut x: Option<u32>) -> Option<u32> {
513+
let x = x.as_mut()?;
514+
//~^ question_mark
515+
dbg!(*x);
516+
Some(*x * 2)
517+
}
518+
519+
#[allow(clippy::needless_return)]
520+
fn explicit_return(x: Option<u32>) -> Option<u32> {
521+
let x = x?;
522+
//~^ question_mark
523+
dbg!(x);
524+
return Some(x * 2);
525+
}
526+
527+
#[deny(clippy::question_mark_used)]
528+
fn question_mark_forbidden(x: Option<u32>) -> Option<u32> {
529+
if let Some(x) = x {
530+
dbg!(x);
531+
Some(x * 2)
532+
} else {
533+
None
534+
}
535+
}
536+
537+
#[clippy::msrv = "1.12"]
538+
fn msrv_1_12(x: Option<u32>) -> Option<u32> {
539+
if let Some(x) = x {
540+
dbg!(x);
541+
Some(x * 2)
542+
} else {
543+
None
544+
}
545+
}
546+
547+
#[clippy::msrv = "1.13"]
548+
fn msrv_1_13(x: Option<u32>) -> Option<u32> {
549+
let x = x?;
550+
//~^ question_mark
551+
dbg!(x);
552+
Some(x * 2)
553+
}
554+
555+
const fn no_lint_in_const(x: Option<u32>) -> Option<u32> {
556+
if let Some(x) = x {
557+
let x = x + 2;
558+
Some(x * 2)
559+
} else {
560+
None
561+
}
562+
}
563+
564+
fn with_macro() -> Option<()> {
565+
macro_rules! m {
566+
() => {
567+
Some(())
568+
};
569+
};
570+
571+
let x = m!()?;
572+
//~^ question_mark
573+
dbg!(x);
574+
Some(x)
575+
}
576+
577+
#[rustfmt::skip]
578+
mod bad_formatting{
579+
fn t1(x: Option<u32>) -> Option<u32> {
580+
let x = x?; dbg!(x); Some(x * 2)
581+
//~^ question_mark
582+
}
583+
fn t2(x: Option<u32>) -> Option<u32> {
584+
let x = x?;
585+
//~^^ question_mark
586+
dbg!(x);
587+
Some(x * 2)
588+
}
589+
}
590+
}

‎tests/ui/question_mark.rs‎

Lines changed: 110 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
#![feature(try_blocks)]
2-
#![allow(clippy::unnecessary_wraps)]
2+
#![allow(clippy::unnecessary_wraps, clippy::let_unit_value)]
33

44
use std::sync::MutexGuard;
55

@@ -615,3 +615,112 @@ mod issue14894 {
615615
Ok(())
616616
}
617617
}
618+
619+
mod issue13626 {
620+
fn basic(x: Option<u32>) -> Option<u32> {
621+
if let Some(x) = x {
622+
//~^ question_mark
623+
dbg!(x);
624+
Some(x * 2)
625+
} else {
626+
None
627+
}
628+
}
629+
630+
fn mut_ref(mut x: Option<u32>) -> Option<u32> {
631+
if let Some(ref mut x) = x {
632+
//~^ question_mark
633+
dbg!(*x);
634+
Some(*x * 2)
635+
} else {
636+
None
637+
}
638+
}
639+
640+
#[allow(clippy::needless_return)]
641+
fn explicit_return(x: Option<u32>) -> Option<u32> {
642+
if let Some(x) = x {
643+
//~^ question_mark
644+
dbg!(x);
645+
return Some(x * 2);
646+
} else {
647+
return None;
648+
}
649+
}
650+
651+
#[deny(clippy::question_mark_used)]
652+
fn question_mark_forbidden(x: Option<u32>) -> Option<u32> {
653+
if let Some(x) = x {
654+
dbg!(x);
655+
Some(x * 2)
656+
} else {
657+
None
658+
}
659+
}
660+
661+
#[clippy::msrv = "1.12"]
662+
fn msrv_1_12(x: Option<u32>) -> Option<u32> {
663+
if let Some(x) = x {
664+
dbg!(x);
665+
Some(x * 2)
666+
} else {
667+
None
668+
}
669+
}
670+
671+
#[clippy::msrv = "1.13"]
672+
fn msrv_1_13(x: Option<u32>) -> Option<u32> {
673+
if let Some(x) = x {
674+
//~^ question_mark
675+
dbg!(x);
676+
Some(x * 2)
677+
} else {
678+
None
679+
}
680+
}
681+
682+
const fn no_lint_in_const(x: Option<u32>) -> Option<u32> {
683+
if let Some(x) = x {
684+
let x = x + 2;
685+
Some(x * 2)
686+
} else {
687+
None
688+
}
689+
}
690+
691+
fn with_macro() -> Option<()> {
692+
macro_rules! m {
693+
() => {
694+
Some(())
695+
};
696+
};
697+
698+
if let Some(x) = m!() {
699+
//~^ question_mark
700+
dbg!(x);
701+
Some(x)
702+
} else {
703+
None
704+
}
705+
}
706+
707+
#[rustfmt::skip]
708+
mod bad_formatting{
709+
fn t1(x: Option<u32>) -> Option<u32> {
710+
if let Some(x) = x { dbg!(x); Some(x * 2) } else { None }
711+
//~^ question_mark
712+
}
713+
fn t2(x: Option<u32>) -> Option<u32> {
714+
if let Some(x) = x
715+
{
716+
//~^^ question_mark
717+
dbg!(x);
718+
Some(x * 2)
719+
}
720+
else
721+
{
722+
None
723+
}
724+
}
725+
}
726+
}

0 commit comments

Comments
 (0)