Skip to content

Commit 1abff3f

Browse files
J-ZhengLiJ-ZhengLi
committed
add checking for ResultErr in question_mark 
1 parent ea4db3a commit 1abff3f

File tree

4 files changed

+150
-30
lines changed

4 files changed

+150
-30
lines changed

‎clippy_lints/src/question_mark.rs‎

Lines changed: 49 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -3,14 +3,14 @@ use clippy_utils::higher;
33
use clippy_utils::source::snippet_with_applicability;
44
use clippy_utils::sugg::Sugg;
55
use clippy_utils::ty::is_type_diagnostic_item;
6-
use clippy_utils::{eq_expr_value, is_lang_ctor, path_to_local, path_to_local_id, peel_blocks, peel_blocks_with_stmt};
6+
use clippy_utils::{eq_expr_value, is_lang_ctor, path_to_local, peel_blocks, peel_blocks_with_stmt};
77
use if_chain::if_chain;
88
use rustc_errors::Applicability;
9-
use rustc_hir::LangItem::{OptionNone, OptionSome, ResultOk};
10-
use rustc_hir::{BindingAnnotation, Expr, ExprKind, PatKind};
9+
use rustc_hir::LangItem::{OptionNone, OptionSome, ResultErr, ResultOk};
10+
use rustc_hir::{BindingAnnotation, Expr, ExprKind, PatKind, QPath};
1111
use rustc_lint::{LateContext, LateLintPass};
1212
use rustc_session::{declare_lint_pass, declare_tool_lint};
13-
use rustc_span::sym;
13+
use rustc_span::{sym, symbol::Symbol};
1414

1515
declare_clippy_lint! {
1616
/// ### What it does
@@ -61,21 +61,21 @@ impl QuestionMark {
6161
if let ExprKind::MethodCall(segment, _, args, _) = &cond.kind;
6262
if let Some(subject) = args.get(0);
6363
if (Self::option_check_and_early_return(cx, subject, then) && segment.ident.name == sym!(is_none)) ||
64-
(Self::result_check_and_early_return(cx, subject, then) && segment.ident.name == sym!(is_err));
64+
(Self::result_check_and_early_return(cx, subject, then, None) && segment.ident.name == sym!(is_err));
6565
then {
6666
let mut applicability = Applicability::MachineApplicable;
67-
let receiver_str = &Sugg::hir_with_applicability(cx, subject, "..", &mut applicability);
67+
let suggestion = &Sugg::hir_with_applicability(cx, subject, "..", &mut applicability);
6868
let mut replacement: Option<String> = None;
6969
if let Some(else_inner) = r#else {
7070
if eq_expr_value(cx, subject, peel_blocks(else_inner)) {
71-
replacement = Some(format!("Some({}?)", receiver_str));
71+
replacement = Some(format!("Some({}?)", suggestion));
7272
}
7373
} else if Self::moves_by_default(cx, subject)
7474
&& !matches!(subject.kind, ExprKind::Call(..) | ExprKind::MethodCall(..))
7575
{
76-
replacement = Some(format!("{}.as_ref()?;", receiver_str));
76+
replacement = Some(format!("{}.as_ref()?;", suggestion));
7777
} else {
78-
replacement = Some(format!("{}?;", receiver_str));
78+
replacement = Some(format!("{}?;", suggestion));
7979
}
8080

8181
if let Some(replacement_str) = replacement {
@@ -95,19 +95,20 @@ impl QuestionMark {
9595

9696
fn check_if_let_some_or_err_and_early_return(cx: &LateContext<'_>, expr: &Expr<'_>) {
9797
if_chain! {
98-
if let Some(higher::IfLet { let_pat, let_expr, if_then, if_else: Some(if_else) })
98+
if let Some(higher::IfLet { let_pat, let_expr, if_then, if_else })
9999
= higher::IfLet::hir(cx, expr);
100100
if let PatKind::TupleStruct(ref path1, fields, None) = let_pat.kind;
101-
if (Self::option_check_and_early_return(cx, let_expr, if_else) && is_lang_ctor(cx, path1, OptionSome)) ||
102-
(Self::result_check_and_early_return(cx, let_expr, if_else) && is_lang_ctor(cx, path1, ResultOk));
103-
104-
if let PatKind::Binding(annot, bind_id, _, _) = fields[0].kind;
101+
// Only check one of the blocks
102+
let nested_expr = if_else.unwrap_or(if_then);
103+
if Self::check_lang_items(cx, path1, &[OptionSome, ResultOk, ResultErr]);
104+
if let PatKind::Binding(annot, _, ident, _) = fields[0].kind;
105+
if Self::result_check_and_early_return(cx, let_expr, nested_expr, Some(ident.name))
106+
|| Self::option_check_and_early_return(cx, let_expr, nested_expr);
105107
let by_ref = matches!(annot, BindingAnnotation::Ref | BindingAnnotation::RefMut);
106-
if path_to_local_id(peel_blocks(if_then), bind_id);
107108
then {
108109
let mut applicability = Applicability::MachineApplicable;
109110
let receiver_str = snippet_with_applicability(cx, let_expr.span, "..", &mut applicability);
110-
let replacement = format!("{}{}?", receiver_str, if by_ref { ".as_ref()" } else { "" },);
111+
let replacement = format!("{}{}?;", receiver_str, if by_ref { ".as_ref()" } else { "" },);
111112

112113
span_lint_and_sugg(
113114
cx,
@@ -122,8 +123,22 @@ impl QuestionMark {
122123
}
123124
}
124125

125-
fn result_check_and_early_return(cx: &LateContext<'_>, expr: &Expr<'_>, nested_expr: &Expr<'_>) -> bool {
126-
Self::is_result(cx, expr) && Self::expression_returns_unmodified_err(cx, nested_expr, expr)
126+
fn check_lang_items(cx: &LateContext<'_>, qpath: &QPath<'_>, items: &[rustc_hir::LangItem]) -> bool {
127+
for lang_item in items {
128+
if is_lang_ctor(cx, qpath, *lang_item) {
129+
return true;
130+
}
131+
}
132+
false
133+
}
134+
135+
fn result_check_and_early_return(
136+
cx: &LateContext<'_>,
137+
expr: &Expr<'_>,
138+
nested_expr: &Expr<'_>,
139+
symbol: Option<Symbol>,
140+
) -> bool {
141+
Self::is_result(cx, expr) && Self::expression_returns_unmodified_err(cx, nested_expr, expr, symbol)
127142
}
128143

129144
fn option_check_and_early_return(cx: &LateContext<'_>, expr: &Expr<'_>, nested_expr: &Expr<'_>) -> bool {
@@ -156,10 +171,23 @@ impl QuestionMark {
156171
}
157172
}
158173

159-
fn expression_returns_unmodified_err(cx: &LateContext<'_>, expr: &Expr<'_>, cond_expr: &Expr<'_>) -> bool {
160-
match peel_blocks_with_stmt(expr).kind {
161-
ExprKind::Ret(Some(ret_expr)) => Self::expression_returns_unmodified_err(cx, ret_expr, cond_expr),
174+
fn expression_returns_unmodified_err(
175+
cx: &LateContext<'_>,
176+
expr: &Expr<'_>,
177+
cond_expr: &Expr<'_>,
178+
symbol: Option<Symbol>,
179+
) -> bool {
180+
match &peel_blocks_with_stmt(expr).kind {
181+
ExprKind::Ret(Some(ret_expr)) => Self::expression_returns_unmodified_err(cx, ret_expr, cond_expr, symbol),
162182
ExprKind::Path(_) => path_to_local(expr).is_some() && path_to_local(expr) == path_to_local(cond_expr),
183+
ExprKind::Call(_, args_expr) => {
184+
if let Some(arg) = args_expr.first() {
185+
if let Some(name) = symbol {
186+
return clippy_utils::contains_name(name, arg);
187+
}
188+
}
189+
false
190+
},
163191
_ => false,
164192
}
165193
}

‎tests/ui/question_mark.fixed‎

Lines changed: 35 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@ impl CopyStruct {
5252

5353
let _ = Some(self.opt?);
5454

55-
let _ = self.opt?;
55+
let _ = self.opt?;;
5656

5757
self.opt
5858
}
@@ -82,13 +82,13 @@ impl MoveStruct {
8282
}
8383

8484
pub fn if_let_ref_func(self) -> Option<Vec<u32>> {
85-
let v: &Vec<_> = self.opt.as_ref()?;
85+
let v: &Vec<_> = self.opt.as_ref()?;;
8686

8787
Some(v.clone())
8888
}
8989

9090
pub fn if_let_mov_func(self) -> Option<Vec<u32>> {
91-
let v = self.opt?;
91+
let v = self.opt?;;
9292

9393
Some(v)
9494
}
@@ -109,7 +109,7 @@ fn func_returning_result() -> Result<i32, i32> {
109109
}
110110

111111
fn result_func(x: Result<i32, i32>) -> Result<i32, i32> {
112-
let _ = x?;
112+
let _ = x?;;
113113

114114
x?;
115115

@@ -154,6 +154,37 @@ fn f() -> NotOption {
154154
NotOption::First
155155
}
156156

157+
#[allow(dead_code)]
158+
mod issue8288 {
159+
struct CustomError;
160+
161+
fn foo() -> Result<u8, CustomError> {
162+
Ok(1)
163+
}
164+
165+
/// Should just replace the if let pattern with `foo()?;`
166+
fn bar(some_flag: bool) -> Result<u8, CustomError> {
167+
foo()?;
168+
let mut res = 1_u8;
169+
if some_flag {
170+
res = 0_u8;
171+
}
172+
Ok(res)
173+
}
174+
175+
/// No other function logic, same result, replace whole function body with `foo()`
176+
fn baz() -> Result<u8, CustomError> {
177+
foo()?;
178+
Ok(1)
179+
}
180+
181+
/// No other function logic, different result, don't replace whole function body
182+
fn qux() -> Result<u8, CustomError> {
183+
foo()?;
184+
Ok(2)
185+
}
186+
}
187+
157188
fn main() {
158189
some_func(Some(42));
159190
some_func(None);

‎tests/ui/question_mark.rs‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,43 @@ fn f() -> NotOption {
186186
NotOption::First
187187
}
188188

189+
#[allow(dead_code)]
190+
mod issue8288 {
191+
struct CustomError;
192+
193+
fn foo() -> Result<u8, CustomError> {
194+
Ok(1)
195+
}
196+
197+
/// Should just replace the if let pattern with `foo()?;`
198+
fn bar(some_flag: bool) -> Result<u8, CustomError> {
199+
if let Err(err) = foo() {
200+
return Err(err);
201+
}
202+
let mut res = 1_u8;
203+
if some_flag {
204+
res = 0_u8;
205+
}
206+
Ok(res)
207+
}
208+
209+
/// No other function logic, same result, replace whole function body with `foo()`
210+
fn baz() -> Result<u8, CustomError> {
211+
if let Err(err) = foo() {
212+
return Err(err);
213+
}
214+
Ok(1)
215+
}
216+
217+
/// No other function logic, different result, don't replace whole function body
218+
fn qux() -> Result<u8, CustomError> {
219+
if let Err(err) = foo() {
220+
return Err(err);
221+
}
222+
Ok(2)
223+
}
224+
}
225+
189226
fn main() {
190227
some_func(Some(42));
191228
some_func(None);

‎tests/ui/question_mark.stderr‎

Lines changed: 29 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ LL | | x
4444
LL | | } else {
4545
LL | | return None;
4646
LL | | };
47-
| |_________^ help: replace it with: `self.opt?`
47+
| |_________^ help: replace it with: `self.opt?;`
4848

4949
error: this block may be rewritten with the `?` operator
5050
--> $DIR/question_mark.rs:82:9
@@ -79,7 +79,7 @@ LL | | v
7979
LL | | } else {
8080
LL | | return None;
8181
LL | | };
82-
| |_________^ help: replace it with: `self.opt.as_ref()?`
82+
| |_________^ help: replace it with: `self.opt.as_ref()?;`
8383

8484
error: this if-let-else may be rewritten with the `?` operator
8585
--> $DIR/question_mark.rs:115:17
@@ -90,7 +90,7 @@ LL | | v
9090
LL | | } else {
9191
LL | | return None;
9292
LL | | };
93-
| |_________^ help: replace it with: `self.opt?`
93+
| |_________^ help: replace it with: `self.opt?;`
9494

9595
error: this block may be rewritten with the `?` operator
9696
--> $DIR/question_mark.rs:130:5
@@ -104,7 +104,7 @@ error: this if-let-else may be rewritten with the `?` operator
104104
--> $DIR/question_mark.rs:142:13
105105
|
106106
LL | let _ = if let Ok(x) = x { x } else { return x };
107-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `x?`
107+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `x?;`
108108

109109
error: this block may be rewritten with the `?` operator
110110
--> $DIR/question_mark.rs:144:5
@@ -114,5 +114,29 @@ LL | | return x;
114114
LL | | }
115115
| |_____^ help: replace it with: `x?;`
116116

117-
error: aborting due to 13 previous errors
117+
error: this if-let-else may be rewritten with the `?` operator
118+
--> $DIR/question_mark.rs:199:9
119+
|
120+
LL | / if let Err(err) = foo() {
121+
LL | | return Err(err);
122+
LL | | }
123+
| |_________^ help: replace it with: `foo()?;`
124+
125+
error: this if-let-else may be rewritten with the `?` operator
126+
--> $DIR/question_mark.rs:211:9
127+
|
128+
LL | / if let Err(err) = foo() {
129+
LL | | return Err(err);
130+
LL | | }
131+
| |_________^ help: replace it with: `foo()?;`
132+
133+
error: this if-let-else may be rewritten with the `?` operator
134+
--> $DIR/question_mark.rs:219:9
135+
|
136+
LL | / if let Err(err) = foo() {
137+
LL | | return Err(err);
138+
LL | | }
139+
| |_________^ help: replace it with: `foo()?;`
140+
141+
error: aborting due to 16 previous errors
118142

0 commit comments

Comments
 (0)