Skip to content

Commit

Permalink
Auto merge of #7860 - dswij:question-mark-fp, r=giraffate
Browse files Browse the repository at this point in the history
Fix `question_mark` FP on custom error type

Closes #7859

#7840 aims to ignore `question_mark` when the return type is custom, which is [covered here](https://github.com/rust-lang/rust-clippy/blob/df65291edd6b89a241fed483ab165c32df468746/tests/ui/question_mark.rs#L144-L149). But this fails when there is a call in conditional predicate

changelog: [`question_mark`] Fix false positive when there is call in conditional predicate
  • Loading branch information
bors committed Oct 26, 2021
2 parents a48367e + fb0fbad commit ba2ac3e
Show file tree
Hide file tree
Showing 4 changed files with 45 additions and 17 deletions.
16 changes: 5 additions & 11 deletions clippy_lints/src/question_mark.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,23 +172,17 @@ impl QuestionMark {
}
}

fn expression_returns_unmodified_err(
cx: &LateContext<'_>,
expression: &Expr<'_>,
origin_hir_id: &Expr<'_>,
) -> bool {
match expression.kind {
fn expression_returns_unmodified_err(cx: &LateContext<'_>, expr: &Expr<'_>, cond_expr: &Expr<'_>) -> bool {
match expr.kind {
ExprKind::Block(block, _) => {
if let Some(return_expression) = Self::return_expression(block) {
return Self::expression_returns_unmodified_err(cx, return_expression, origin_hir_id);
return Self::expression_returns_unmodified_err(cx, return_expression, cond_expr);
}

false
},
ExprKind::Ret(Some(expr)) | ExprKind::Call(expr, _) => {
Self::expression_returns_unmodified_err(cx, expr, origin_hir_id)
},
ExprKind::Path(_) => path_to_local(expression) == path_to_local(origin_hir_id),
ExprKind::Ret(Some(ret_expr)) => Self::expression_returns_unmodified_err(cx, ret_expr, cond_expr),
ExprKind::Path(_) => path_to_local(expr) == path_to_local(cond_expr),
_ => false,
}
}
Expand Down
21 changes: 19 additions & 2 deletions tests/ui/question_mark.fixed
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,11 @@ fn func() -> Option<i32> {
Some(0)
}

fn result_func(x: Result<i32, &str>) -> Result<i32, &str> {
fn func_returning_result() -> Result<i32, i32> {
Ok(1)
}

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

x?;
Expand All @@ -113,9 +117,22 @@ fn result_func(x: Result<i32, &str>) -> Result<i32, &str> {
let y = if let Ok(x) = x {
x
} else {
return Err("some error");
return Err(0);
};

// issue #7859
// no warning
let _ = if let Ok(x) = func_returning_result() {
x
} else {
return Err(0);
};

// no warning
if func_returning_result().is_err() {
return func_returning_result();
}

Ok(y)
}

Expand Down
21 changes: 19 additions & 2 deletions tests/ui/question_mark.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,11 @@ fn func() -> Option<i32> {
Some(0)
}

fn result_func(x: Result<i32, &str>) -> Result<i32, &str> {
fn func_returning_result() -> Result<i32, i32> {
Ok(1)
}

fn result_func(x: Result<i32, i32>) -> Result<i32, i32> {
let _ = if let Ok(x) = x { x } else { return x };

if x.is_err() {
Expand All @@ -145,9 +149,22 @@ fn result_func(x: Result<i32, &str>) -> Result<i32, &str> {
let y = if let Ok(x) = x {
x
} else {
return Err("some error");
return Err(0);
};

// issue #7859
// no warning
let _ = if let Ok(x) = func_returning_result() {
x
} else {
return Err(0);
};

// no warning
if func_returning_result().is_err() {
return func_returning_result();
}

Ok(y)
}

Expand Down
4 changes: 2 additions & 2 deletions tests/ui/question_mark.stderr
Original file line number Diff line number Diff line change
Expand Up @@ -101,13 +101,13 @@ LL | | }
| |_____^ help: replace it with: `f()?;`

error: this if-let-else may be rewritten with the `?` operator
--> $DIR/question_mark.rs:138:13
--> $DIR/question_mark.rs:142:13
|
LL | let _ = if let Ok(x) = x { x } else { return x };
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: replace it with: `x?`

error: this block may be rewritten with the `?` operator
--> $DIR/question_mark.rs:140:5
--> $DIR/question_mark.rs:144:5
|
LL | / if x.is_err() {
LL | | return x;
Expand Down

0 comments on commit ba2ac3e

Please sign in to comment.