Auto merge of #10091 - EricWu2003:manual-filter-FP, r=llogiq

fix manual_filter false positive

fixes #10088
fixes #9766

changelog: FP: [`manual_filter`]: Now ignores if expressions where the else branch has side effects or doesn't return `None`
[#10091](https://github.com/rust-lang/rust-clippy/pull/10091)
<!-- changelog_checked -->

clippy_lints/src/matches/manual_filter.rs

@@ -3,7 +3,7 @@ use clippy_utils::ty::is_type_diagnostic_item;
 use clippy_utils::visitors::contains_unsafe_block;
 use clippy_utils::{is_res_lang_ctor, path_res, path_to_local_id};
 
-use rustc_hir::LangItem::OptionSome;
+use rustc_hir::LangItem::{OptionNone, OptionSome};
 use rustc_hir::{Arm, Expr, ExprKind, HirId, Pat, PatKind};
 use rustc_lint::LateContext;
 use rustc_span::{sym, SyntaxContext};
@@ -25,15 +25,13 @@ fn get_cond_expr<'tcx>(
         if let Some(block_expr) = peels_blocks_incl_unsafe_opt(expr);
         if let ExprKind::If(cond, then_expr, Some(else_expr)) = block_expr.kind;
         if let PatKind::Binding(_,target, ..) = pat.kind;
-        if let (then_visitor, else_visitor)
-            = (is_some_expr(cx, target, ctxt, then_expr),
-                is_some_expr(cx, target, ctxt, else_expr));
-        if then_visitor != else_visitor; // check that one expr resolves to `Some(x)`, the other to `None`
+        if is_some_expr(cx, target, ctxt, then_expr) && is_none_expr(cx, else_expr)
+            || is_none_expr(cx, then_expr) && is_some_expr(cx, target, ctxt, else_expr); // check that one expr resolves to `Some(x)`, the other to `None`
         then {
             return Some(SomeExpr {
                     expr: peels_blocks_incl_unsafe(cond.peel_drop_temps()),
                     needs_unsafe_block: contains_unsafe_block(cx, expr),
-                    needs_negated: !then_visitor // if the `then_expr` resolves to `None`, need to negate the cond
+                    needs_negated: is_none_expr(cx, then_expr) // if the `then_expr` resolves to `None`, need to negate the cond
                 })
             }
     };
@@ -74,6 +72,13 @@ fn is_some_expr(cx: &LateContext<'_>, target: HirId, ctxt: SyntaxContext, expr:
     false
 }
 
+fn is_none_expr(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+    if let Some(inner_expr) = peels_blocks_incl_unsafe_opt(expr) {
+        return is_res_lang_ctor(cx, path_res(cx, inner_expr), OptionNone);
+    };
+    false
+}
+
 // given the closure: `|<pattern>| <expr>`
 // returns `|&<pattern>| <expr>`
 fn add_ampersand_if_copy(body_str: String, has_copy_trait: bool) -> String {

tests/ui/manual_filter.fixed

@@ -116,4 +116,45 @@ fn main() {
         },
         None => None,
     };
+
+    match Some(20) {
+        // Don't Lint, because `Some(3*x)` is not `None`
+        None => None,
+        Some(x) => {
+            if x > 0 {
+                Some(3 * x)
+            } else {
+                Some(x)
+            }
+        },
+    };
+
+    // Don't lint: https://github.com/rust-lang/rust-clippy/issues/10088
+    let result = if let Some(a) = maybe_some() {
+        if let Some(b) = maybe_some() {
+            Some(a + b)
+        } else {
+            Some(a)
+        }
+    } else {
+        None
+    };
+
+    let allowed_integers = vec![3, 4, 5, 6];
+    // Don't lint, since there's a side effect in the else branch
+    match Some(21) {
+        Some(x) => {
+            if allowed_integers.contains(&x) {
+                Some(x)
+            } else {
+                println!("Invalid integer: {x:?}");
+                None
+            }
+        },
+        None => None,
+    };
+}
+
+fn maybe_some() -> Option<u32> {
+    Some(0)
 }

tests/ui/manual_filter.rs

@@ -240,4 +240,45 @@ fn main() {
         },
         None => None,
     };
+
+    match Some(20) {
+        // Don't Lint, because `Some(3*x)` is not `None`
+        None => None,
+        Some(x) => {
+            if x > 0 {
+                Some(3 * x)
+            } else {
+                Some(x)
+            }
+        },
+    };
+
+    // Don't lint: https://github.com/rust-lang/rust-clippy/issues/10088
+    let result = if let Some(a) = maybe_some() {
+        if let Some(b) = maybe_some() {
+            Some(a + b)
+        } else {
+            Some(a)
+        }
+    } else {
+        None
+    };
+
+    let allowed_integers = vec![3, 4, 5, 6];
+    // Don't lint, since there's a side effect in the else branch
+    match Some(21) {
+        Some(x) => {
+            if allowed_integers.contains(&x) {
+                Some(x)
+            } else {
+                println!("Invalid integer: {x:?}");
+                None
+            }
+        },
+        None => None,
+    };
+}
+
+fn maybe_some() -> Option<u32> {
+    Some(0)
 }