Skip to content

Commit 6ea6295

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

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,10 +2,10 @@ 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};
77
use clippy_utils::res::{MaybeDef, MaybeQPath, MaybeResPath};
8-
use clippy_utils::source::snippet_with_applicability;
8+
use clippy_utils::source::{reindent_multiline, snippet_indent, snippet_with_applicability, snippet_with_context};
99
use clippy_utils::sugg::Sugg;
1010
use clippy_utils::ty::{implements_trait, is_copy};
1111
use clippy_utils::usage::local_used_after_expr;
@@ -17,13 +17,15 @@ use clippy_utils::{
1717
use rustc_errors::Applicability;
1818
use rustc_hir::LangItem::{self, OptionNone, OptionSome, ResultErr, ResultOk};
1919
use rustc_hir::def::Res;
20+
use rustc_hir::def_id::LocalDefId;
2021
use rustc_hir::{
21-
Arm, BindingMode, Block, Body, ByRef, Expr, ExprKind, FnRetTy, HirId, LetStmt, MatchSource, Mutability, Node, Pat,
22-
PatKind, PathSegment, QPath, Stmt, StmtKind,
22+
Arm, BindingMode, Block, Body, ByRef, Expr, ExprKind, FnDecl, FnRetTy, HirId, LetStmt, MatchSource, Mutability,
23+
Node, Pat, PatKind, PathSegment, QPath, Stmt, StmtKind, intravisit,
2324
};
24-
use rustc_lint::{LateContext, LateLintPass};
25+
use rustc_lint::{LateContext, LateLintPass, LintContext};
2526
use rustc_middle::ty::{self, Ty};
2627
use rustc_session::impl_lint_pass;
28+
use rustc_span::Span;
2729
use rustc_span::symbol::Symbol;
2830

2931
declare_clippy_lint! {
@@ -524,6 +526,63 @@ fn check_if_let_some_or_err_and_early_return<'tcx>(cx: &LateContext<'tcx>, expr:
524526
}
525527
}
526528

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

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

‎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)