Don't suggest let else in match if the else arm explicitly mentions non obvious paths

Author: est31

clippy_lints/src/manual_let_else.rs

@@ -1,15 +1,17 @@
 use clippy_utils::diagnostics::span_lint;
 use clippy_utils::higher::IfLetOrMatch;
+use clippy_utils::ty::is_type_diagnostic_item;
 use clippy_utils::visitors::expr_visitor_no_bodies;
 use clippy_utils::{meets_msrv, msrvs, peel_blocks};
 use if_chain::if_chain;
+use rustc_data_structures::stable_set::FxHashSet;
 use rustc_hir::intravisit::Visitor;
-use rustc_hir::{Expr, ExprKind, MatchSource, Pat, QPath, Stmt, StmtKind};
+use rustc_hir::{Expr, ExprKind, MatchSource, Pat, PatKind, QPath, Stmt, StmtKind};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
 use rustc_middle::lint::in_external_macro;
 use rustc_semver::RustcVersion;
-use rustc_data_structures::stable_set::FxHashSet;
 use rustc_session::{declare_tool_lint, impl_lint_pass};
+use rustc_span::symbol::sym;
 use rustc_span::Span;
 
 declare_clippy_lint! {
@@ -108,7 +110,7 @@ impl<'tcx> LateLintPass<'tcx> for ManualLetElse {
                     }
                     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) {
+                    } else if expr_diverges(cx, arm.body) && pat_allowed_for_else(cx, arm.pat) {
                         found_diverging_arm = true;
                     }
                 }
@@ -199,10 +201,39 @@ fn from_different_macros(span_a: Span, span_b: Span) -> bool {
     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
+fn pat_allowed_for_else(cx: &LateContext<'_>, pat: &'_ Pat<'_>) -> bool {
+    // Check whether the pattern contains any bindings, as the
+    // binding might potentially be used in the body.
+    // TODO: only look for *used* bindings.
+    let mut has_bindings = false;
+    pat.each_binding_or_first(&mut |_, _, _, _| has_bindings = true);
+    if has_bindings {
+        return false;
+    }
+
+    // Check whether any possibly "unknown" patterns are included,
+    // because users might not which values some enum has.
+    // Well-known enums are excepted, as we assume people know them.
+    // We do a deep check, to be able to disallow Err(En::Foo(_))
+    // for usage of the En::Foo variant, as we disallow En::Foo(_),
+    // but we allow Err(_).
+    let typeck_results = cx.typeck_results();
+    let mut has_disallowed = false;
+    pat.walk_always(|pat| {
+        // Only do the check if the type is "spelled out" in the pattern
+        if !matches!(
+            pat.kind,
+            PatKind::Struct(..) | PatKind::TupleStruct(..) | PatKind::Path(..)
+        ) {
+            return;
+        };
+        let ty = typeck_results.pat_ty(pat);
+        // Option and Result are allowed, everything else isn't.
+        if !(is_type_diagnostic_item(cx, ty, sym::Option) || is_type_diagnostic_item(cx, ty, sym::Result)) {
+            has_disallowed = true;
+        }
+    });
+    !has_disallowed
 }
 
 /// Checks if the passed block is a simple identity referring to bindings created by the pattern

tests/ui/manual_let_else_match.rs

@@ -4,12 +4,6 @@
 // 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)
 }
@@ -18,7 +12,17 @@ fn g() -> Option<()> {
     None
 }
 
-fn h() -> Variant {
+fn h() -> (Option<()>, Option<()>) {
+    (None, None)
+}
+
+enum Variant {
+    Foo,
+    Bar(u32),
+    Baz(u32),
+}
+
+fn build_enum() -> Variant {
     Variant::Foo
 }
 
@@ -36,9 +40,14 @@ fn fire() {
     };
 
     loop {
-        // More complex pattern for the identity arm
+        // More complex pattern for the identity arm and diverging arm
         let v = match h() {
-            Variant::Foo => continue,
+            (Some(_), Some(_)) | (None, None) => continue,
+            (Some(v), None) | (None, Some(v)) => v,
+        };
+        // Custom enums are supported as long as the "else" arm is a simple _
+        let v = match build_enum() {
+            _ => continue,
             Variant::Bar(v) | Variant::Baz(v) => v,
         };
     }
@@ -49,21 +58,27 @@ fn fire() {
         Ok(v) => v,
         Err(_) => return,
     };
+
+    // Err(()) is an allowed pattern
+    let v = match f().map_err(|_| ()) {
+        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,
+        _ => panic!(),
+        (None, Some(_v)) => return,
+        (Some(v), None) => v,
     };
 
     // Multiple identity arms
     let v = match h() {
-        Variant::Foo => panic!(),
-        Variant::Bar(v) => v,
-        Variant::Baz(v) => v,
+        _ => panic!(),
+        (None, Some(v)) => v,
+        (Some(v), None) => v,
     };
 
     // No diverging arm at all, only identity arms.
@@ -74,8 +89,8 @@ fn not_fire() {
     };
 
     // The identity arm has a guard
-    let v = match h() {
-        Variant::Bar(v) if g().is_none() => v,
+    let v = match g() {
+        Some(v) if g().is_none() => v,
         _ => return,
     };
 
@@ -90,4 +105,17 @@ fn not_fire() {
         Ok(v) => v,
         Err(e) => panic!("error: {e}"),
     };
+
+    // Custom enum where the diverging arm
+    // explicitly mentions the variant
+    let v = match build_enum() {
+        Variant::Foo => return,
+        Variant::Bar(v) | Variant::Baz(v) => v,
+    };
+
+    // The custom enum is surrounded by an Err()
+    let v = match Err(build_enum()) {
+        Ok(v) | Err(Variant::Bar(v) | Variant::Baz(v)) => v,
+        Err(Variant::Foo) => return,
+    };
 }

tests/ui/manual_let_else_match.stderr

@@ -1,5 +1,5 @@
 error: this could be rewritten as `let else`
-  --> $DIR/manual_let_else_match.rs:28:5
+  --> $DIR/manual_let_else_match.rs:32:5
    |
 LL | /     let v = match g() {
 LL | |         Some(v_some) => v_some,
@@ -10,7 +10,7 @@ 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
+  --> $DIR/manual_let_else_match.rs:37:5
    |
 LL | /     let v = match g() {
 LL | |         Some(v_some) => v_some,
@@ -19,22 +19,40 @@ LL | |     };
    | |______^
 
 error: this could be rewritten as `let else`
-  --> $DIR/manual_let_else_match.rs:40:9
+  --> $DIR/manual_let_else_match.rs:44:9
    |
 LL | /         let v = match h() {
-LL | |             Variant::Foo => continue,
+LL | |             (Some(_), Some(_)) | (None, None) => continue,
+LL | |             (Some(v), None) | (None, Some(v)) => v,
+LL | |         };
+   | |__________^
+
+error: this could be rewritten as `let else`
+  --> $DIR/manual_let_else_match.rs:49:9
+   |
+LL | /         let v = match build_enum() {
+LL | |             _ => 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
+  --> $DIR/manual_let_else_match.rs:57:5
    |
 LL | /     let v = match f() {
 LL | |         Ok(v) => v,
 LL | |         Err(_) => return,
 LL | |     };
    | |______^
 
-error: aborting due to 4 previous errors
+error: this could be rewritten as `let else`
+  --> $DIR/manual_let_else_match.rs:63:5
+   |
+LL | /     let v = match f().map_err(|_| ()) {
+LL | |         Ok(v) => v,
+LL | |         Err(()) => return,
+LL | |     };
+   | |______^
+
+error: aborting due to 6 previous errors