Also support linting for match

clippy_lints/src/manual_let_else.rs

@@ -1,12 +1,14 @@
 use clippy_utils::diagnostics::span_lint;
+use clippy_utils::higher::IfLetOrMatch;
 use clippy_utils::visitors::{for_each_expr, Descend};
-use clippy_utils::{higher, meets_msrv, msrvs, peel_blocks};
+use clippy_utils::{meets_msrv, msrvs, peel_blocks};
 use if_chain::if_chain;
-use rustc_hir::{Expr, ExprKind, Pat, QPath, Stmt, StmtKind};
+use rustc_hir::{Expr, ExprKind, MatchSource, Pat, QPath, Stmt, StmtKind};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::lint::in_external_macro;
 use rustc_semver::RustcVersion;
 use rustc_session::{declare_tool_lint, impl_lint_pass};
+use rustc_span::Span;
 use std::ops::ControlFlow;
 
 declare_clippy_lint! {
@@ -56,19 +58,23 @@ impl_lint_pass!(ManualLetElse => [MANUAL_LET_ELSE]);
 
 impl<'tcx> LateLintPass<'tcx> for ManualLetElse {
     fn check_stmt(&mut self, cx: &LateContext<'_>, stmt: &'tcx Stmt<'tcx>) {
-        if !meets_msrv(self.msrv, msrvs::LET_ELSE) {
-            return;
-        }
-
-        if in_external_macro(cx.sess(), stmt.span) {
-            return;
-        }
-
-        if_chain! {
+        let if_let_or_match = if_chain! {
+            if meets_msrv(self.msrv, msrvs::LET_ELSE);
+            if !in_external_macro(cx.sess(), stmt.span);
             if let StmtKind::Local(local) = stmt.kind;
             if let Some(init) = local.init;
-            if let Some(higher::IfLet { let_pat, let_expr: _, if_then, if_else }) = higher::IfLet::hir(cx, init);
-            if if_then_simple_identity(let_pat, if_then);
+            if !from_different_macros(init.span, stmt.span);
+            if let Some(if_let_or_match) = IfLetOrMatch::parse(cx, init);
+            then {
+                if_let_or_match
+            } else {
+                return;
+            }
+        };
+
+        match if_let_or_match {
+            IfLetOrMatch::IfLet(_let_expr, let_pat, if_then, if_else) => if_chain! {
+                if expr_is_simple_identity(let_pat, if_then);
                 if let Some(if_else) = if_else;
                 if expr_diverges(cx, if_else);
                 then {
@@ -79,6 +85,37 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse {
                         "this could be rewritten as `let else`",
                     );
                 }
+            },
+            IfLetOrMatch::Match(_match_expr, arms, source) => {
+                if source != MatchSource::Normal {
+                    return;
+                }
+                // Any other number than two arms doesn't (neccessarily)
+                // have a trivial mapping to let else.
+                if arms.len() != 2 {
+                    return;
+                }
+                // We iterate over both arms, trying to find one that is an identity,
+                // one that diverges. Our check needs to work regardless of the order
+                // of both arms.
+                let mut found_identity_arm = false;
+                let mut found_diverging_arm = false;
+                for arm in arms {
+                    // Guards don't give us an easy mapping to let else
+                    if arm.guard.is_some() {
+                        return;
+                    }
+                    if expr_is_simple_identity(arm.pat, arm.body) {
+                        found_identity_arm = true;
+                    } else if expr_diverges(cx, arm.body) && pat_has_no_bindings(arm.pat) {
+                        found_diverging_arm = true;
+                    }
+                }
+                if !(found_identity_arm && found_diverging_arm) {
+                    return;
+                }
+                span_lint(cx, MANUAL_LET_ELSE, stmt.span, "this could be rewritten as `let else`");
+            },
         }
     }
 
@@ -139,16 +176,39 @@ fn expr_diverges(cx: &LateContext<'_>, expr: &'_ Expr<'_>) -> bool {
     .is_some()
 }
 
-/// Checks if the passed `if_then` is a simple identity
-fn if_then_simple_identity(let_pat: &'_ Pat<'_>, if_then: &'_ Expr<'_>) -> bool {
+/// Returns true if the two spans come from different macro sites,
+/// or one comes from an invocation and the other is not from a macro at all.
+fn from_different_macros(span_a: Span, span_b: Span) -> bool {
+    // This pre-check is a speed up so that we don't build outer_expn_data unless needed.
+    match (span_a.from_expansion(), span_b.from_expansion()) {
+        (false, false) => return false,
+        (true, false) | (false, true) => return true,
+        // We need to determine if both are from the same macro
+        (true, true) => (),
+    }
+    let data_for_comparison = |sp: Span| {
+        let expn_data = sp.ctxt().outer_expn_data();
+        (expn_data.kind, expn_data.call_site)
+    };
+    data_for_comparison(span_a) != data_for_comparison(span_b)
+}
+
+fn pat_has_no_bindings(pat: &'_ Pat<'_>) -> bool {
+    let mut has_no_bindings = true;
+    pat.each_binding_or_first(&mut |_, _, _, _| has_no_bindings = false);
+    has_no_bindings
+}
+
+/// Checks if the passed block is a simple identity referring to bindings created by the pattern
+fn expr_is_simple_identity(pat: &'_ Pat<'_>, expr: &'_ Expr<'_>) -> bool {
     // TODO support patterns with multiple bindings and tuples, like:
-    //   let (foo, bar) = if let (Some(foo), bar) = g() { (foo, bar) } else { ... }
+    //   let ... = if let (Some(foo), bar) = g() { (foo, bar) } else { ... }
     if_chain! {
-        if let ExprKind::Path(QPath::Resolved(_ty, path)) = &peel_blocks(if_then).kind;
+        if let ExprKind::Path(QPath::Resolved(_ty, path)) = &peel_blocks(expr).kind;
         if let [path_seg] = path.segments;
         then {
             let mut pat_bindings = Vec::new();
-            let_pat.each_binding(|_ann, _hir_id, _sp, ident| {
+            pat.each_binding_or_first(&mut |_ann, _hir_id, _sp, ident| {
                 pat_bindings.push(ident);
             });
             if let [binding] = &pat_bindings[..] {

tests/ui/manual_let_else.rs

@@ -2,8 +2,8 @@
 #![allow(
     clippy::collapsible_else_if,
     clippy::unused_unit,
-    clippy::never_loop,
-    clippy::let_unit_value
+    clippy::let_unit_value,
+    clippy::never_loop
 )]
 #![warn(clippy::manual_let_else)]
 
@@ -87,6 +87,14 @@ fn fire() {
     } else {
         return;
     };
+
+    // entirely inside macro lints
+    macro_rules! create_binding_if_some {
+        ($n:ident, $e:expr) => {
+            let $n = if let Some(v) = $e { v } else { return };
+        };
+    }
+    create_binding_if_some!(w, g());
 }
 
 fn not_fire() {
@@ -164,4 +172,20 @@ fn not_fire() {
         };
         Some(v)
     }
+
+    // Macro boundary inside let
+    macro_rules! some_or_return {
+        ($e:expr) => {
+            if let Some(v) = $e { v } else { return }
+        };
+    }
+    let v = some_or_return!(g());
+
+    // Also macro boundary inside let, but inside a macro
+    macro_rules! create_binding_if_some_nf {
+        ($n:ident, $e:expr) => {
+            let $n = some_or_return!($e);
+        };
+    }
+    create_binding_if_some_nf!(v, g());
 }

tests/ui/manual_let_else.stderr

@@ -100,5 +100,16 @@ LL | |         return;
 LL | |     };
    | |______^
 
-error: aborting due to 11 previous errors
+error: this could be rewritten as `let else`
+  --> $DIR/manual_let_else.rs:94:13
+   |
+LL |             let $n = if let Some(v) = $e { v } else { return };
+   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+...
+LL |     create_binding_if_some!(w, g());
+   |     ------------------------------- in this macro invocation
+   |
+   = note: this error originates in the macro `create_binding_if_some` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: aborting due to 12 previous errors
 

tests/ui/manual_let_else_match.rs

@@ -0,0 +1,93 @@
+#![allow(unused_braces, unused_variables, dead_code)]
+#![allow(clippy::collapsible_else_if, clippy::let_unit_value)]
+#![warn(clippy::manual_let_else)]
+// Ensure that we don't conflict with match -> if let lints
+#![warn(clippy::single_match_else, clippy::single_match)]
+
+enum Variant {
+    Foo,
+    Bar(u32),
+    Baz(u32),
+}
+
+fn f() -> Result<u32, u32> {
+    Ok(0)
+}
+
+fn g() -> Option<()> {
+    None
+}
+
+fn h() -> Variant {
+    Variant::Foo
+}
+
+fn main() {}
+
+fn fire() {
+    let v = match g() {
+        Some(v_some) => v_some,
+        None => return,
+    };
+
+    let v = match g() {
+        Some(v_some) => v_some,
+        _ => return,
+    };
+
+    loop {
+        // More complex pattern for the identity arm
+        let v = match h() {
+            Variant::Foo => continue,
+            Variant::Bar(v) | Variant::Baz(v) => v,
+        };
+    }
+
+    // There is a _ in the diverging arm
+    // TODO also support unused bindings aka _v
+    let v = match f() {
+        Ok(v) => v,
+        Err(_) => return,
+    };
+}
+
+fn not_fire() {
+    // Multiple diverging arms
+    let v = match h() {
+        Variant::Foo => panic!(),
+        Variant::Bar(_v) => return,
+        Variant::Baz(v) => v,
+    };
+
+    // Multiple identity arms
+    let v = match h() {
+        Variant::Foo => panic!(),
+        Variant::Bar(v) => v,
+        Variant::Baz(v) => v,
+    };
+
+    // No diverging arm at all, only identity arms.
+    // This is no case for let else, but destructuring assignment.
+    let v = match f() {
+        Ok(v) => v,
+        Err(e) => e,
+    };
+
+    // The identity arm has a guard
+    let v = match h() {
+        Variant::Bar(v) if g().is_none() => v,
+        _ => return,
+    };
+
+    // The diverging arm has a guard
+    let v = match f() {
+        Err(v) if v > 0 => panic!(),
+        Ok(v) | Err(v) => v,
+    };
+
+    // The diverging arm creates a binding
+    let v = match f() {
+        Ok(v) => v,
+        Err(e) => panic!("error: {e}"),
+    };
+}

tests/ui/manual_let_else_match.stderr

@@ -0,0 +1,40 @@
+error: this could be rewritten as `let else`
+  --> $DIR/manual_let_else_match.rs:28:5
+   |
+LL | /     let v = match g() {
+LL | |         Some(v_some) => v_some,
+LL | |         None => return,
+LL | |     };
+   | |______^
+   |
+   = note: `-D clippy::manual-let-else` implied by `-D warnings`
+
+error: this could be rewritten as `let else`
+  --> $DIR/manual_let_else_match.rs:33:5
+   |
+LL | /     let v = match g() {
+LL | |         Some(v_some) => v_some,
+LL | |         _ => return,
+LL | |     };
+   | |______^
+
+error: this could be rewritten as `let else`
+  --> $DIR/manual_let_else_match.rs:40:9
+   |
+LL | /         let v = match h() {
+LL | |             Variant::Foo => continue,
+LL | |             Variant::Bar(v) | Variant::Baz(v) => v,
+LL | |         };
+   | |__________^
+
+error: this could be rewritten as `let else`
+  --> $DIR/manual_let_else_match.rs:48:5
+   |
+LL | /     let v = match f() {
+LL | |         Ok(v) => v,
+LL | |         Err(_) => return,
+LL | |     };
+   | |______^
+
+error: aborting due to 4 previous errors
+