about summary refs log tree commit diff
diff options
context:
space:
mode:
authorbors <bors@rust-lang.org>2021-10-26 23:46:21 +0000
committerbors <bors@rust-lang.org>2021-10-26 23:46:21 +0000
commitba2ac3e26360a827fcc0e741d3e831ca7e9aa4f4 (patch)
tree2f47b5c227c7e6ff55778186162b39bb32fe29d5
parenta48367e21cd2bd489eb01f576556a11eeb0efce9 (diff)
parentfb0fbad5caa97da949cb2c6dbc56a1185bb53af9 (diff)
downloadrust-ba2ac3e26360a827fcc0e741d3e831ca7e9aa4f4.tar.gz
rust-ba2ac3e26360a827fcc0e741d3e831ca7e9aa4f4.zip
Auto merge of #7860 - dswij:question-mark-fp, r=giraffate
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
-rw-r--r--clippy_lints/src/question_mark.rs16
-rw-r--r--tests/ui/question_mark.fixed21
-rw-r--r--tests/ui/question_mark.rs21
-rw-r--r--tests/ui/question_mark.stderr4
4 files changed, 45 insertions, 17 deletions
diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs
index 4d616e26bfc..f63ef163bcb 100644
--- a/clippy_lints/src/question_mark.rs
+++ b/clippy_lints/src/question_mark.rs
@@ -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,
         }
     }
diff --git a/tests/ui/question_mark.fixed b/tests/ui/question_mark.fixed
index ccb2e5a302e..e93469e5f55 100644
--- a/tests/ui/question_mark.fixed
+++ b/tests/ui/question_mark.fixed
@@ -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?;
@@ -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)
 }
 
diff --git a/tests/ui/question_mark.rs b/tests/ui/question_mark.rs
index ca3722371f5..dd179e9bee8 100644
--- a/tests/ui/question_mark.rs
+++ b/tests/ui/question_mark.rs
@@ -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() {
@@ -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)
 }
 
diff --git a/tests/ui/question_mark.stderr b/tests/ui/question_mark.stderr
index 161588cb73c..8d782b71dd6 100644
--- a/tests/ui/question_mark.stderr
+++ b/tests/ui/question_mark.stderr
@@ -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;