Skip to content

Commit 8c7b3ad

Browse files
borsbors
committed
Auto merge of #5266 - sinkuu:questionmark, r=flip1995
Lint `if let Some` and early return in question_mark lint Fixes #5260 changelog: lint `if let Some` and early return in `question_mark` lint
2 parents 36b6598 + a78a1fc commit 8c7b3ad

File tree

5 files changed

+244
-33
lines changed

5 files changed

+244
-33
lines changed

‎clippy_lints/src/question_mark.rs

Lines changed: 52 additions & 12 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_sugg, SpanlessEq,
12+
};
1113

1214
declare_clippy_lint! {
1315
/// **What it does:** Checks for expressions that could be replaced by the question mark operator.
@@ -55,7 +57,8 @@ impl QuestionMark {
5557
if Self::is_option(cx, subject);
5658

5759
then {
58-
let receiver_str = &Sugg::hir(cx, subject, "..");
60+
let mut applicability = Applicability::MachineApplicable;
61+
let receiver_str = &Sugg::hir_with_applicability(cx, subject, "..", &mut applicability);
5962
let mut replacement: Option<String> = None;
6063
if let Some(else_) = else_ {
6164
if_chain! {
@@ -74,25 +77,61 @@ impl QuestionMark {
7477
}
7578

7679
if let Some(replacement_str) = replacement {
77-
span_lint_and_then(
80+
span_lint_and_sugg(
7881
cx,
7982
QUESTION_MARK,
8083
expr.span,
8184
"this block may be rewritten with the `?` operator",
82-
|db| {
83-
db.span_suggestion(
84-
expr.span,
85-
"replace_it_with",
86-
replacement_str,
87-
Applicability::MaybeIncorrect, // snippet
88-
);
89-
}
85+
"replace it with",
86+
replacement_str,
87+
applicability,
9088
)
9189
}
9290
}
9391
}
9492
}
9593

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

@@ -158,5 +197,6 @@ impl QuestionMark {
158197
impl<'a, 'tcx> LateLintPass<'a, 'tcx> for QuestionMark {
159198
fn check_expr(&mut self, cx: &LateContext<'a, 'tcx>, expr: &'tcx Expr<'_>) {
160199
Self::check_is_none_and_early_return_none(cx, expr);
200+
Self::check_if_let_some_and_early_return_none(cx, expr);
161201
}
162202
}

‎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.fixed

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
// run-rustfix
2+
#![allow(unreachable_code)]
3+
4+
fn some_func(a: Option<u32>) -> Option<u32> {
5+
a?;
6+
7+
a
8+
}
9+
10+
fn some_other_func(a: Option<u32>) -> Option<u32> {
11+
if a.is_none() {
12+
return None;
13+
} else {
14+
return Some(0);
15+
}
16+
unreachable!()
17+
}
18+
19+
pub enum SeemsOption<T> {
20+
Some(T),
21+
None,
22+
}
23+
24+
impl<T> SeemsOption<T> {
25+
pub fn is_none(&self) -> bool {
26+
match *self {
27+
SeemsOption::None => true,
28+
SeemsOption::Some(_) => false,
29+
}
30+
}
31+
}
32+
33+
fn returns_something_similar_to_option(a: SeemsOption<u32>) -> SeemsOption<u32> {
34+
if a.is_none() {
35+
return SeemsOption::None;
36+
}
37+
38+
a
39+
}
40+
41+
pub struct CopyStruct {
42+
pub opt: Option<u32>,
43+
}
44+
45+
impl CopyStruct {
46+
#[rustfmt::skip]
47+
pub fn func(&self) -> Option<u32> {
48+
(self.opt)?;
49+
50+
self.opt?;
51+
52+
let _ = Some(self.opt?);
53+
54+
let _ = self.opt?;
55+
56+
self.opt
57+
}
58+
}
59+
60+
#[derive(Clone)]
61+
pub struct MoveStruct {
62+
pub opt: Option<Vec<u32>>,
63+
}
64+
65+
impl MoveStruct {
66+
pub fn ref_func(&self) -> Option<Vec<u32>> {
67+
self.opt.as_ref()?;
68+
69+
self.opt.clone()
70+
}
71+
72+
pub fn mov_func_reuse(self) -> Option<Vec<u32>> {
73+
self.opt.as_ref()?;
74+
75+
self.opt
76+
}
77+
78+
pub fn mov_func_no_use(self) -> Option<Vec<u32>> {
79+
self.opt.as_ref()?;
80+
Some(Vec::new())
81+
}
82+
83+
pub fn if_let_ref_func(self) -> Option<Vec<u32>> {
84+
let v: &Vec<_> = self.opt.as_ref()?;
85+
86+
Some(v.clone())
87+
}
88+
89+
pub fn if_let_mov_func(self) -> Option<Vec<u32>> {
90+
let v = self.opt?;
91+
92+
Some(v)
93+
}
94+
}
95+
96+
fn main() {
97+
some_func(Some(42));
98+
some_func(None);
99+
some_other_func(Some(42));
100+
101+
let copy_struct = CopyStruct { opt: Some(54) };
102+
copy_struct.func();
103+
104+
let move_struct = MoveStruct {
105+
opt: Some(vec![42, 1337]),
106+
};
107+
move_struct.ref_func();
108+
move_struct.clone().mov_func_reuse();
109+
move_struct.mov_func_no_use();
110+
111+
let so = SeemsOption::Some(45);
112+
returns_something_similar_to_option(so);
113+
}

‎tests/ui/question_mark.rs

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,6 @@
1+
// run-rustfix
2+
#![allow(unreachable_code)]
3+
14
fn some_func(a: Option<u32>) -> Option<u32> {
25
if a.is_none() {
36
return None;
@@ -58,6 +61,12 @@ impl CopyStruct {
5861
self.opt
5962
};
6063

64+
let _ = if let Some(x) = self.opt {
65+
x
66+
} else {
67+
return None;
68+
};
69+
6170
self.opt
6271
}
6372
}
@@ -90,11 +99,32 @@ impl MoveStruct {
9099
}
91100
Some(Vec::new())
92101
}
102+
103+
pub fn if_let_ref_func(self) -> Option<Vec<u32>> {
104+
let v: &Vec<_> = if let Some(ref v) = self.opt {
105+
v
106+
} else {
107+
return None;
108+
};
109+
110+
Some(v.clone())
111+
}
112+
113+
pub fn if_let_mov_func(self) -> Option<Vec<u32>> {
114+
let v = if let Some(v) = self.opt {
115+
v
116+
} else {
117+
return None;
118+
};
119+
120+
Some(v)
121+
}
93122
}
94123

95124
fn main() {
96125
some_func(Some(42));
97126
some_func(None);
127+
some_other_func(Some(42));
98128

99129
let copy_struct = CopyStruct { opt: Some(54) };
100130
copy_struct.func();

‎tests/ui/question_mark.stderr

Lines changed: 48 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -1,63 +1,96 @@
11
error: this block may be rewritten with the `?` operator
2-
--> $DIR/question_mark.rs:2:5
2+
--> $DIR/question_mark.rs:5:5
33
|
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

1111
error: this block may be rewritten with the `?` operator
12-
--> $DIR/question_mark.rs:47:9
12+
--> $DIR/question_mark.rs:50:9
1313
|
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
20-
--> $DIR/question_mark.rs:51:9
20+
--> $DIR/question_mark.rs:54: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
28-
--> $DIR/question_mark.rs:55:17
28+
--> $DIR/question_mark.rs:58:17
2929
|
3030
LL | let _ = if self.opt.is_none() {
3131
| _________________^
3232
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:64: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:81: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:89: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:97: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:104:26
75+
|
76+
LL | let 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:114:17
86+
|
87+
LL | let 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)