Skip to content

Commit 45f61ea

Browse files
sinkuusinkuu
committed
Lint if let Some in question_mark lint
1 parent 74eae9d commit 45f61ea

File tree

4 files changed

+123
-20
lines changed

4 files changed

+123
-20
lines changed

‎clippy_lints/src/question_mark.rs‎

Lines changed: 52 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,15 @@
11
use if_chain::if_chain;
22
use rustc_errors::Applicability;
33
use rustc_hir::def::{DefKind, Res};
4-
use rustc_hir::{def, Block, Expr, ExprKind, StmtKind};
4+
use rustc_hir::{def, BindingAnnotation, Block, Expr, ExprKind, MatchSource, PatKind, StmtKind};
55
use rustc_lint::{LateContext, LateLintPass};
66
use rustc_session::{declare_lint_pass, declare_tool_lint};
77

88
use crate::utils::paths::{OPTION, OPTION_NONE};
99
use crate::utils::sugg::Sugg;
10-
use crate::utils::{higher, match_def_path, match_type, span_lint_and_then, SpanlessEq};
10+
use crate::utils::{
11+
higher, match_def_path, match_qpath, match_type, snippet_with_applicability, span_lint_and_then, SpanlessEq,
12+
};
1113

1214
declare_clippy_lint! {
1315
/// **What it does:** Checks for expressions that could be replaced by the question mark operator.
@@ -82,7 +84,7 @@ impl QuestionMark {
8284
|db| {
8385
db.span_suggestion(
8486
expr.span,
85-
"replace_it_with",
87+
"replace it with",
8688
replacement_str,
8789
Applicability::MaybeIncorrect, // snippet
8890
);
@@ -93,6 +95,52 @@ impl QuestionMark {
9395
}
9496
}
9597

98+
fn check_if_let_some_and_early_return_none(cx: &LateContext<'_, '_>, expr: &Expr<'_>) {
99+
if_chain! {
100+
if let ExprKind::Match(subject, arms, source) = &expr.kind;
101+
if *source == MatchSource::IfLetDesugar { contains_else_clause: true };
102+
if Self::is_option(cx, subject);
103+
104+
if let PatKind::TupleStruct(path1, fields, None) = &arms[0].pat.kind;
105+
if match_qpath(path1, &["Some"]);
106+
if let PatKind::Binding(annot, _, bind, _) = &fields[0].kind;
107+
let by_ref = matches!(annot, BindingAnnotation::Ref | BindingAnnotation::RefMut);
108+
109+
if let ExprKind::Block(block, None) = &arms[0].body.kind;
110+
if block.stmts.is_empty();
111+
if let Some(trailing_expr) = &block.expr;
112+
if let ExprKind::Path(path) = &trailing_expr.kind;
113+
if match_qpath(path, &[&bind.as_str()]);
114+
115+
if let PatKind::Wild = arms[1].pat.kind;
116+
if Self::expression_returns_none(cx, arms[1].body);
117+
then {
118+
let mut applicability = Applicability::MachineApplicable;
119+
let receiver_str = snippet_with_applicability(cx, subject.span, "..", &mut applicability);
120+
let replacement = format!(
121+
"{}{}?",
122+
receiver_str,
123+
if by_ref { ".as_ref()" } else { "" },
124+
);
125+
126+
span_lint_and_then(
127+
cx,
128+
QUESTION_MARK,
129+
expr.span,
130+
"this if-let-else may be rewritten with the `?` operator",
131+
|db| {
132+
db.span_suggestion(
133+
expr.span,
134+
"replace it with",
135+
replacement,
136+
applicability,
137+
);
138+
}
139+
)
140+
}
141+
}
142+
}
143+
96144
fn moves_by_default(cx: &LateContext<'_, '_>, expression: &Expr<'_>) -> bool {
97145
let expr_ty = cx.tables.expr_ty(expression);
98146

@@ -158,5 +206,6 @@ impl QuestionMark {
158206
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for QuestionMark {
159207
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) {
160208
Self::check_is_none_and_early_return_none(cx, expr);
209+
Self::check_if_let_some_and_early_return_none(cx, expr);
161210
}
162211
}

‎clippy_lints/src/types.rs‎

Lines changed: 1 addition & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1698,12 +1698,7 @@ fn detect_absurd_comparison<'a, 'tcx>(
16981698
return None;
16991699
}
17001700

1701-
let normalized = normalize_comparison(op, lhs, rhs);
1702-
let (rel, normalized_lhs, normalized_rhs) = if let Some(val) = normalized {
1703-
val
1704-
} else {
1705-
return None;
1706-
};
1701+
let (rel, normalized_lhs, normalized_rhs) = normalize_comparison(op, lhs, rhs)?;
17071702

17081703
let lx = detect_extreme_expr(cx, normalized_lhs);
17091704
let rx = detect_extreme_expr(cx, normalized_rhs);

‎tests/ui/question_mark.rs‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,12 @@ impl CopyStruct {
5858
self.opt
5959
};
6060

61+
let _ = if let Some(x) = self.opt {
62+
x
63+
} else {
64+
return None;
65+
};
66+
6167
self.opt
6268
}
6369
}
@@ -90,6 +96,26 @@ impl MoveStruct {
9096
}
9197
Some(Vec::new())
9298
}
99+
100+
pub fn if_let_ref_func(self) -> Option<Vec<u32>> {
101+
let mut v: &Vec<_> = if let Some(ref v) = self.opt {
102+
v
103+
} else {
104+
return None;
105+
};
106+
107+
Some(v.clone())
108+
}
109+
110+
pub fn if_let_mov_func(self) -> Option<Vec<u32>> {
111+
let mut v = if let Some(v) = self.opt {
112+
v
113+
} else {
114+
return None;
115+
};
116+
117+
Some(v)
118+
}
93119
}
94120

95121
fn main() {

‎tests/ui/question_mark.stderr‎

Lines changed: 44 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4,7 +4,7 @@ error: this block may be rewritten with the `?` operator
44
LL | / if a.is_none() {
55
LL | | return None;
66
LL | | }
7-
| |_____^ help: replace_it_with: `a?;`
7+
| |_____^ help: replace it with: `a?;`
88
|
99
= note: `-D clippy::question-mark` implied by `-D warnings`
1010

@@ -14,15 +14,15 @@ error: this block may be rewritten with the `?` operator
1414
LL | / if (self.opt).is_none() {
1515
LL | | return None;
1616
LL | | }
17-
| |_________^ help: replace_it_with: `(self.opt)?;`
17+
| |_________^ help: replace it with: `(self.opt)?;`
1818

1919
error: this block may be rewritten with the `?` operator
2020
--> $DIR/question_mark.rs:51:9
2121
|
2222
LL | / if self.opt.is_none() {
2323
LL | | return None
2424
LL | | }
25-
| |_________^ help: replace_it_with: `self.opt?;`
25+
| |_________^ help: replace it with: `self.opt?;`
2626

2727
error: this block may be rewritten with the `?` operator
2828
--> $DIR/question_mark.rs:55:17
@@ -33,31 +33,64 @@ LL | | return None;
3333
LL | | } else {
3434
LL | | self.opt
3535
LL | | };
36-
| |_________^ help: replace_it_with: `Some(self.opt?)`
36+
| |_________^ help: replace it with: `Some(self.opt?)`
37+
38+
error: this if-let-else may be rewritten with the `?` operator
39+
--> $DIR/question_mark.rs:61:17
40+
|
41+
LL | let _ = if let Some(x) = self.opt {
42+
| _________________^
43+
LL | | x
44+
LL | | } else {
45+
LL | | return None;
46+
LL | | };
47+
| |_________^ help: replace it with: `self.opt?`
3748

3849
error: this block may be rewritten with the `?` operator
39-
--> $DIR/question_mark.rs:72:9
50+
--> $DIR/question_mark.rs:78:9
4051
|
4152
LL | / if self.opt.is_none() {
4253
LL | | return None;
4354
LL | | }
44-
| |_________^ help: replace_it_with: `self.opt.as_ref()?;`
55+
| |_________^ help: replace it with: `self.opt.as_ref()?;`
4556

4657
error: this block may be rewritten with the `?` operator
47-
--> $DIR/question_mark.rs:80:9
58+
--> $DIR/question_mark.rs:86:9
4859
|
4960
LL | / if self.opt.is_none() {
5061
LL | | return None;
5162
LL | | }
52-
| |_________^ help: replace_it_with: `self.opt.as_ref()?;`
63+
| |_________^ help: replace it with: `self.opt.as_ref()?;`
5364

5465
error: this block may be rewritten with the `?` operator
55-
--> $DIR/question_mark.rs:88:9
66+
--> $DIR/question_mark.rs:94:9
5667
|
5768
LL | / if self.opt.is_none() {
5869
LL | | return None;
5970
LL | | }
60-
| |_________^ help: replace_it_with: `self.opt.as_ref()?;`
71+
| |_________^ help: replace it with: `self.opt.as_ref()?;`
72+
73+
error: this if-let-else may be rewritten with the `?` operator
74+
--> $DIR/question_mark.rs:101:30
75+
|
76+
LL | let mut v: &Vec<_> = if let Some(ref v) = self.opt {
77+
| ______________________________^
78+
LL | | v
79+
LL | | } else {
80+
LL | | return None;
81+
LL | | };
82+
| |_________^ help: replace it with: `self.opt.as_ref()?`
83+
84+
error: this if-let-else may be rewritten with the `?` operator
85+
--> $DIR/question_mark.rs:111:21
86+
|
87+
LL | let mut v = if let Some(v) = self.opt {
88+
| _____________________^
89+
LL | | v
90+
LL | | } else {
91+
LL | | return None;
92+
LL | | };
93+
| |_________^ help: replace it with: `self.opt?`
6194

62-
error: aborting due to 7 previous errors
95+
error: aborting due to 10 previous errors
6396

0 commit comments

Comments
 (0)