fix FP on Err return checking

rust-lang · Jan 28, 2022 · 03b2fdc · 03b2fdc
1 parent 1c87b2d
commit 03b2fdc
Show file tree

Hide file tree

Showing 5 changed files with 68 additions and 67 deletions.
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -5,6 +5,7 @@
 #![feature(control_flow_enum)]
 #![feature(drain_filter)]
 #![feature(iter_intersperse)]
+#![feature(let_chains)]
 #![feature(let_else)]
 #![feature(once_cell)]
 #![feature(rustc_private)]

diff --git a/clippy_lints/src/question_mark.rs b/clippy_lints/src/question_mark.rs
@@ -98,12 +98,12 @@ impl QuestionMark {
             if let Some(higher::IfLet { let_pat, let_expr, if_then, if_else })
                 = higher::IfLet::hir(cx, expr);
             if let PatKind::TupleStruct(ref path1, fields, None) = let_pat.kind;
-            // Only check one of the blocks
             let nested_expr = if_else.unwrap_or(if_then);
-            if Self::check_lang_items(cx, path1, &[OptionSome, ResultOk, ResultErr]);
             if let PatKind::Binding(annot, bind_id, ident, _) = fields[0].kind;
-            if Self::result_check_and_early_return(cx, let_expr, nested_expr, Some(ident.name))
-                || Self::option_check_and_early_return(cx, let_expr, nested_expr);
+            if (Self::result_check_and_early_return(cx, let_expr, nested_expr, Some(ident.name)) &&
+                (is_lang_ctor(cx, path1, ResultOk) || is_lang_ctor(cx, path1, ResultErr))) ||
+                (Self::option_check_and_early_return(cx, let_expr, nested_expr) &&
+                is_lang_ctor(cx, path1, OptionSome));
             let by_ref = matches!(annot, BindingAnnotation::Ref | BindingAnnotation::RefMut);
             then {
                 let mut applicability = Applicability::MachineApplicable;
@@ -128,22 +128,13 @@ impl QuestionMark {
         }
     }
 
-    fn check_lang_items(cx: &LateContext<'_>, qpath: &QPath<'_>, items: &[rustc_hir::LangItem]) -> bool {
-        for lang_item in items {
-            if is_lang_ctor(cx, qpath, *lang_item) {
-                return true;
-            }
-        }
-        false
-    }
-
     fn result_check_and_early_return(
         cx: &LateContext<'_>,
         expr: &Expr<'_>,
         nested_expr: &Expr<'_>,
-        symbol: Option<Symbol>,
+        pat_symbol: Option<Symbol>,
     ) -> bool {
-        Self::is_result(cx, expr) && Self::expression_returns_unmodified_err(cx, nested_expr, expr, symbol)
+        Self::is_result(cx, expr) && Self::expression_returns_unmodified_err(cx, nested_expr, expr, pat_symbol)
     }
 
     fn option_check_and_early_return(cx: &LateContext<'_>, expr: &Expr<'_>, nested_expr: &Expr<'_>) -> bool {
@@ -180,19 +171,22 @@ impl QuestionMark {
         cx: &LateContext<'_>,
         expr: &Expr<'_>,
         cond_expr: &Expr<'_>,
-        symbol: Option<Symbol>,
+        pat_symbol: Option<Symbol>,
     ) -> bool {
         match &peel_blocks_with_stmt(expr).kind {
-            ExprKind::Ret(Some(ret_expr)) =>
-                Self::expression_returns_unmodified_err(cx, ret_expr, cond_expr, symbol),
-            ExprKind::Path(_) =>
-                path_to_local(expr).is_some() && path_to_local(expr) == path_to_local(cond_expr),
-            ExprKind::Call(_, args_expr) => {
-                if let Some(arg) = args_expr.first() {
-                    if let Some(name) = symbol {
-                        return clippy_utils::contains_name(name, arg);
-                    }
+            ExprKind::Ret(Some(ret_expr)) => {
+                Self::expression_returns_unmodified_err(cx, ret_expr, cond_expr, pat_symbol)
+            },
+            ExprKind::Path(_) => path_to_local(expr).is_some() && path_to_local(expr) == path_to_local(cond_expr),
+            ExprKind::Call(call_expr, args_expr) => {
+                if let ExprKind::Path(qpath) = &call_expr.kind
+                    && let QPath::Resolved(_, path) = qpath
+                    && let Some(pat_sym) = pat_symbol
+                    && let Some(arg) = args_expr.first()
+                {
+                    return path.segments[0].ident.name.as_str() == "Err" && clippy_utils::contains_name(pat_sym, arg)
                 }
+
                 false
             },
             _ => false,

diff --git a/tests/ui/question_mark.fixed b/tests/ui/question_mark.fixed
@@ -158,30 +158,38 @@ fn f() -> NotOption {
 mod issue8288 {
     struct CustomError;
 
-    fn foo() -> Result<u8, CustomError> {
-        Ok(1)
+    fn do_something() {}
+
+    fn foo() -> Result<(), CustomError> {
+        Ok(())
     }
 
-    /// Should just replace the if let pattern with `foo()?;`
-    fn bar(some_flag: bool) -> Result<u8, CustomError> {
+    fn bar() -> Result<(), CustomError> {
         foo()?;
-        let mut res = 1_u8;
-        if some_flag {
-            res = 0_u8;
-        }
-        Ok(res)
+        Ok(())
     }
 
-    /// No other function logic, same result, replace whole function body with `foo()`
-    fn baz() -> Result<u8, CustomError> {
+    fn baz() -> Result<(), CustomError> {
         foo()?;
-        Ok(1)
+        do_something();
+        Ok(())
     }
 
-    /// No other function logic, different result, don't replace whole function body
-    fn qux() -> Result<u8, CustomError> {
-        foo()?;
-        Ok(2)
+    // No warning
+    fn qux() -> Result<(), CustomError> {
+        if let Err(err) = foo() {
+            do_something();
+            return Err(err);
+        }
+        Ok(())
+    }
+
+    // No warning
+    fn quux() -> Option<CustomError> {
+        if let Err(err) = foo() {
+            return Some(err);
+        }
+        None
     }
 }
 

diff --git a/tests/ui/question_mark.rs b/tests/ui/question_mark.rs
@@ -190,36 +190,42 @@ fn f() -> NotOption {
 mod issue8288 {
     struct CustomError;
 
-    fn foo() -> Result<u8, CustomError> {
-        Ok(1)
+    fn do_something() {}
+
+    fn foo() -> Result<(), CustomError> {
+        Ok(())
     }
 
-    /// Should just replace the if let pattern with `foo()?;`
-    fn bar(some_flag: bool) -> Result<u8, CustomError> {
+    fn bar() -> Result<(), CustomError> {
         if let Err(err) = foo() {
             return Err(err);
         }
-        let mut res = 1_u8;
-        if some_flag {
-            res = 0_u8;
-        }
-        Ok(res)
+        Ok(())
     }
 
-    /// No other function logic, same result, replace whole function body with `foo()`
-    fn baz() -> Result<u8, CustomError> {
+    fn baz() -> Result<(), CustomError> {
         if let Err(err) = foo() {
             return Err(err);
         }
-        Ok(1)
+        do_something();
+        Ok(())
     }
 
-    /// No other function logic, different result, don't replace whole function body
-    fn qux() -> Result<u8, CustomError> {
+    // No warning
+    fn qux() -> Result<(), CustomError> {
         if let Err(err) = foo() {
+            do_something();
             return Err(err);
         }
-        Ok(2)
+        Ok(())
+    }
+
+    // No warning
+    fn quux() -> Option<CustomError> {
+        if let Err(err) = foo() {
+            return Some(err);
+        }
+        None
     }
 }
 

diff --git a/tests/ui/question_mark.stderr b/tests/ui/question_mark.stderr
@@ -115,28 +115,20 @@ LL | |     }
    | |_____^ help: replace it with: `x?;`
 
 error: this if-let-else may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:199:9
+  --> $DIR/question_mark.rs:200:9
    |
 LL | /         if let Err(err) = foo() {
 LL | |             return Err(err);
 LL | |         }
    | |_________^ help: replace it with: `foo()?;`
 
 error: this if-let-else may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:211:9
+  --> $DIR/question_mark.rs:207:9
    |
 LL | /         if let Err(err) = foo() {
 LL | |             return Err(err);
 LL | |         }
    | |_________^ help: replace it with: `foo()?;`
 
-error: this if-let-else may be rewritten with the `?` operator
-  --> $DIR/question_mark.rs:219:9
-   |
-LL | /         if let Err(err) = foo() {
-LL | |             return Err(err);
-LL | |         }
-   | |_________^ help: replace it with: `foo()?;`
-
-error: aborting due to 16 previous errors
+error: aborting due to 15 previous errors