Skip to content

Commit 7c034ec

Browse files
J-ZhengLiJ-ZhengLi
committed
fix false positive when checking Err returns
1 parent 19ce85a commit 7c034ec

File tree

5 files changed

+68
-67
lines changed

5 files changed

+68
-67
lines changed

‎clippy_lints/src/lib.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
#![feature(control_flow_enum)]
66
#![feature(drain_filter)]
77
#![feature(iter_intersperse)]
8+
#![feature(let_chains)]
89
#![feature(let_else)]
910
#![feature(once_cell)]
1011
#![feature(rustc_private)]

‎clippy_lints/src/question_mark.rs‎

Lines changed: 19 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -98,12 +98,12 @@ impl QuestionMark {
9898
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-
// Only check one of the blocks
102101
let nested_expr = if_else.unwrap_or(if_then);
103-
if Self::check_lang_items(cx, path1, &[OptionSome, ResultOk, ResultErr]);
104102
if let PatKind::Binding(annot, bind_id, 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);
103+
if (Self::result_check_and_early_return(cx, let_expr, nested_expr, Some(ident.name)) &&
104+
(is_lang_ctor(cx, path1, ResultOk) || is_lang_ctor(cx, path1, ResultErr))) ||
105+
(Self::option_check_and_early_return(cx, let_expr, nested_expr) &&
106+
is_lang_ctor(cx, path1, OptionSome));
107107
let by_ref = matches!(annot, BindingAnnotation::Ref | BindingAnnotation::RefMut);
108108
then {
109109
let mut applicability = Applicability::MachineApplicable;
@@ -128,22 +128,13 @@ impl QuestionMark {
128128
}
129129
}
130130

131-
fn check_lang_items(cx: &LateContext<'_>, qpath: &QPath<'_>, items: &[rustc_hir::LangItem]) -> bool {
132-
for lang_item in items {
133-
if is_lang_ctor(cx, qpath, *lang_item) {
134-
return true;
135-
}
136-
}
137-
false
138-
}
139-
140131
fn result_check_and_early_return(
141132
cx: &LateContext<'_>,
142133
expr: &Expr<'_>,
143134
nested_expr: &Expr<'_>,
144-
symbol: Option<Symbol>,
135+
pat_symbol: Option<Symbol>,
145136
) -> bool {
146-
Self::is_result(cx, expr) && Self::expression_returns_unmodified_err(cx, nested_expr, expr, symbol)
137+
Self::is_result(cx, expr) && Self::expression_returns_unmodified_err(cx, nested_expr, expr, pat_symbol)
147138
}
148139

149140
fn option_check_and_early_return(cx: &LateContext<'_>, expr: &Expr<'_>, nested_expr: &Expr<'_>) -> bool {
@@ -180,19 +171,22 @@ impl QuestionMark {
180171
cx: &LateContext<'_>,
181172
expr: &Expr<'_>,
182173
cond_expr: &Expr<'_>,
183-
symbol: Option<Symbol>,
174+
pat_symbol: Option<Symbol>,
184175
) -> bool {
185176
match &peel_blocks_with_stmt(expr).kind {
186-
ExprKind::Ret(Some(ret_expr)) =>
187-
Self::expression_returns_unmodified_err(cx, ret_expr, cond_expr, symbol),
188-
ExprKind::Path(_) =>
189-
path_to_local(expr).is_some() && path_to_local(expr) == path_to_local(cond_expr),
190-
ExprKind::Call(_, args_expr) => {
191-
if let Some(arg) = args_expr.first() {
192-
if let Some(name) = symbol {
193-
return clippy_utils::contains_name(name, arg);
194-
}
177+
ExprKind::Ret(Some(ret_expr)) => {
178+
Self::expression_returns_unmodified_err(cx, ret_expr, cond_expr, pat_symbol)
179+
},
180+
ExprKind::Path(_) => path_to_local(expr).is_some() && path_to_local(expr) == path_to_local(cond_expr),
181+
ExprKind::Call(call_expr, args_expr) => {
182+
if let ExprKind::Path(qpath) = &call_expr.kind
183+
&& let QPath::Resolved(_, path) = qpath
184+
&& let Some(pat_sym) = pat_symbol
185+
&& let Some(arg) = args_expr.first()
186+
{
187+
return path.segments[0].ident.name.as_str() == "Err" && clippy_utils::contains_name(pat_sym, arg)
195188
}
189+
196190
false
197191
},
198192
_ => false,

‎tests/ui/question_mark.fixed‎

Lines changed: 24 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -158,30 +158,38 @@ fn f() -> NotOption {
158158
mod issue8288 {
159159
struct CustomError;
160160

161-
fn foo() -> Result<u8, CustomError> {
162-
Ok(1)
161+
fn do_something() {}
162+
163+
fn foo() -> Result<(), CustomError> {
164+
Ok(())
163165
}
164166

165-
/// Should just replace the if let pattern with `foo()?;`
166-
fn bar(some_flag: bool) -> Result<u8, CustomError> {
167+
fn bar() -> Result<(), CustomError> {
167168
foo()?;
168-
let mut res = 1_u8;
169-
if some_flag {
170-
res = 0_u8;
171-
}
172-
Ok(res)
169+
Ok(())
173170
}
174171

175-
/// No other function logic, same result, replace whole function body with `foo()`
176-
fn baz() -> Result<u8, CustomError> {
172+
fn baz() -> Result<(), CustomError> {
177173
foo()?;
178-
Ok(1)
174+
do_something();
175+
Ok(())
179176
}
180177

181-
/// No other function logic, different result, don't replace whole function body
182-
fn qux() -> Result<u8, CustomError> {
183-
foo()?;
184-
Ok(2)
178+
// No warning
179+
fn qux() -> Result<(), CustomError> {
180+
if let Err(err) = foo() {
181+
do_something();
182+
return Err(err);
183+
}
184+
Ok(())
185+
}
186+
187+
// No warning
188+
fn quux() -> Option<CustomError> {
189+
if let Err(err) = foo() {
190+
return Some(err);
191+
}
192+
None
185193
}
186194
}
187195

‎tests/ui/question_mark.rs‎

Lines changed: 21 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -190,36 +190,42 @@ fn f() -> NotOption {
190190
mod issue8288 {
191191
struct CustomError;
192192

193-
fn foo() -> Result<u8, CustomError> {
194-
Ok(1)
193+
fn do_something() {}
194+
195+
fn foo() -> Result<(), CustomError> {
196+
Ok(())
195197
}
196198

197-
/// Should just replace the if let pattern with `foo()?;`
198-
fn bar(some_flag: bool) -> Result<u8, CustomError> {
199+
fn bar() -> Result<(), CustomError> {
199200
if let Err(err) = foo() {
200201
return Err(err);
201202
}
202-
let mut res = 1_u8;
203-
if some_flag {
204-
res = 0_u8;
205-
}
206-
Ok(res)
203+
Ok(())
207204
}
208205

209-
/// No other function logic, same result, replace whole function body with `foo()`
210-
fn baz() -> Result<u8, CustomError> {
206+
fn baz() -> Result<(), CustomError> {
211207
if let Err(err) = foo() {
212208
return Err(err);
213209
}
214-
Ok(1)
210+
do_something();
211+
Ok(())
215212
}
216213

217-
/// No other function logic, different result, don't replace whole function body
218-
fn qux() -> Result<u8, CustomError> {
214+
// No warning
215+
fn qux() -> Result<(), CustomError> {
219216
if let Err(err) = foo() {
217+
do_something();
220218
return Err(err);
221219
}
222-
Ok(2)
220+
Ok(())
221+
}
222+
223+
// No warning
224+
fn quux() -> Option<CustomError> {
225+
if let Err(err) = foo() {
226+
return Some(err);
227+
}
228+
None
223229
}
224230
}
225231

‎tests/ui/question_mark.stderr‎

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -115,28 +115,20 @@ LL | | }
115115
| |_____^ help: replace it with: `x?;`
116116

117117
error: this if-let-else may be rewritten with the `?` operator
118-
--> $DIR/question_mark.rs:199:9
118+
--> $DIR/question_mark.rs:200:9
119119
|
120120
LL | / if let Err(err) = foo() {
121121
LL | | return Err(err);
122122
LL | | }
123123
| |_________^ help: replace it with: `foo()?;`
124124

125125
error: this if-let-else may be rewritten with the `?` operator
126-
--> $DIR/question_mark.rs:211:9
126+
--> $DIR/question_mark.rs:207:9
127127
|
128128
LL | / if let Err(err) = foo() {
129129
LL | | return Err(err);
130130
LL | | }
131131
| |_________^ help: replace it with: `foo()?;`
132132

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
133+
error: aborting due to 15 previous errors
142134

0 commit comments

Comments
 (0)