fix: manual_let_else FN when diverges on simple enum variant

Author: profetia

clippy_lints/src/manual_let_else.rs

@@ -4,11 +4,13 @@ use clippy_utils::diagnostics::span_lint_and_then;
 use clippy_utils::higher::IfLetOrMatch;
 use clippy_utils::source::snippet_with_context;
 use clippy_utils::ty::is_type_diagnostic_item;
-use clippy_utils::{is_lint_allowed, is_never_expr, msrvs, pat_and_expr_can_be_question_mark, peel_blocks};
+use clippy_utils::{
+    is_enum_variant, is_lint_allowed, is_never_expr, is_wild, msrvs, pat_and_expr_can_be_question_mark, peel_blocks,
+};
 use rustc_ast::BindingMode;
 use rustc_data_structures::fx::FxHashMap;
 use rustc_errors::Applicability;
-use rustc_hir::{Expr, ExprKind, MatchSource, Pat, PatExpr, PatExprKind, PatKind, QPath, Stmt, StmtKind};
+use rustc_hir::{Arm, Expr, ExprKind, MatchSource, Pat, PatExpr, PatExprKind, PatKind, QPath, Stmt, StmtKind};
 use rustc_lint::{LateContext, LintContext};
 
 use rustc_span::Span;
@@ -91,14 +93,15 @@ impl<'tcx> QuestionMark {
                     let Some((idx, diverging_arm)) = diverging_arm_opt else {
                         return;
                     };
+
+                    let pat_arm = &arms[1 - idx];
                     // If the non-diverging arm is the first one, its pattern can be reused in a let/else statement.
                     // However, if it arrives in second position, its pattern may cover some cases already covered
                     // by the diverging one.
-                    // TODO: accept the non-diverging arm as a second position if patterns are disjointed.
-                    if idx == 0 {
+                    if idx == 0 && !is_arms_disjointed(cx, diverging_arm, pat_arm) {
                         return;
                     }
-                    let pat_arm = &arms[1 - idx];
+
                     let Some(ident_map) = expr_simple_identity_map(local.pat, pat_arm.pat, pat_arm.body) else {
                         return;
                     };
@@ -110,6 +113,39 @@ impl<'tcx> QuestionMark {
     }
 }
 
+/// Checks if the patterns of the arms are disjointed. Currently, we only support patterns of simple
+/// enum variants without nested patterns or bindings.
+///
+/// TODO: Support more complex patterns.
+fn is_arms_disjointed(cx: &LateContext<'_>, arm1: &Arm<'_>, arm2: &Arm<'_>) -> bool {
+    if arm1.guard.is_some() || arm2.guard.is_some() {
+        return false;
+    }
+
+    if !is_simple_enum_variant(cx, arm1.pat) || !is_simple_enum_variant(cx, arm2.pat) {
+        return false;
+    }
+
+    true
+}
+
+fn is_simple_enum_variant(cx: &LateContext<'_>, pat: &Pat<'_>) -> bool {
+    if !is_enum_variant(cx, pat) {
+        return false;
+    }
+
+    match pat.kind {
+        PatKind::Struct(_, fields, _) => fields
+            .iter()
+            .all(|field| is_wild(field.pat) || matches!(field.pat.kind, PatKind::Binding(..))),
+        PatKind::TupleStruct(_, pats, _) => pats
+            .iter()
+            .all(|pat| is_wild(pat) || matches!(pat.kind, PatKind::Binding(..))),
+        PatKind::Expr(_) => true,
+        _ => false,
+    }
+}
+
 fn emit_manual_let_else(
     cx: &LateContext<'_>,
     span: Span,

clippy_utils/src/lib.rs

@@ -96,7 +96,7 @@ use rustc_data_structures::fx::FxHashMap;
 use rustc_data_structures::packed::Pu128;
 use rustc_data_structures::unhash::UnhashMap;
 use rustc_hir::LangItem::{OptionNone, OptionSome, ResultErr, ResultOk};
-use rustc_hir::def::{DefKind, Res};
+use rustc_hir::def::{CtorOf, DefKind, Res};
 use rustc_hir::def_id::{CrateNum, DefId, LOCAL_CRATE, LocalDefId, LocalModDefId};
 use rustc_hir::definitions::{DefPath, DefPathData};
 use rustc_hir::hir_id::{HirIdMap, HirIdSet};
@@ -106,7 +106,7 @@ use rustc_hir::{
     CoroutineDesugaring, CoroutineKind, Destination, Expr, ExprField, ExprKind, FnDecl, FnRetTy, GenericArg,
     GenericArgs, HirId, Impl, ImplItem, ImplItemKind, ImplItemRef, Item, ItemKind, LangItem, LetStmt, MatchSource,
     Mutability, Node, OwnerId, OwnerNode, Param, Pat, PatExpr, PatExprKind, PatKind, Path, PathSegment, PrimTy, QPath,
-    Stmt, StmtKind, TraitFn, TraitItem, TraitItemKind, TraitItemRef, TraitRef, TyKind, UnOp, def,
+    Stmt, StmtKind, TraitFn, TraitItem, TraitItemKind, TraitItemRef, TraitRef, TyKind, UnOp,
 };
 use rustc_lexer::{TokenKind, tokenize};
 use rustc_lint::{LateContext, Level, Lint, LintContext};
@@ -1838,14 +1838,15 @@ pub fn is_ctor_or_promotable_const_function(cx: &LateContext<'_>, expr: &Expr<'_
     false
 }
 
+fn is_res_enum_variant(res: Res) -> bool {
+    matches!(res, Res::Def(DefKind::Variant | DefKind::Ctor(CtorOf::Variant, _), _))
+}
+
 /// Returns `true` if a pattern is refutable.
 // TODO: should be implemented using rustc/mir_build/thir machinery
 pub fn is_refutable(cx: &LateContext<'_>, pat: &Pat<'_>) -> bool {
     fn is_enum_variant(cx: &LateContext<'_>, qpath: &QPath<'_>, id: HirId) -> bool {
-        matches!(
-            cx.qpath_res(qpath, id),
-            Res::Def(DefKind::Variant, ..) | Res::Def(DefKind::Ctor(def::CtorOf::Variant, _), _)
-        )
+        is_res_enum_variant(cx.qpath_res(qpath, id))
     }
 
     fn are_refutable<'a, I: IntoIterator<Item = &'a Pat<'a>>>(cx: &LateContext<'_>, i: I) -> bool {
@@ -3764,3 +3765,29 @@ pub fn desugar_await<'tcx>(expr: &'tcx Expr<'_>) -> Option<&'tcx Expr<'tcx>> {
         None
     }
 }
+
+/// Returns `true` if the given pattern is a variant of an enum.
+pub fn is_enum_variant(cx: &LateContext<'_>, pat: &Pat<'_>) -> bool {
+    struct Pat<'hir>(&'hir rustc_hir::Pat<'hir>);
+
+    impl<'hir> MaybePath<'hir> for Pat<'hir> {
+        fn qpath_opt(&self) -> Option<&QPath<'hir>> {
+            match self.0.kind {
+                PatKind::Struct(ref qpath, _, _)
+                | PatKind::TupleStruct(ref qpath, _, _)
+                | PatKind::Expr(&PatExpr {
+                    kind: PatExprKind::Path(ref qpath),
+                    ..
+                }) => Some(qpath),
+                _ => None,
+            }
+        }
+
+        fn hir_id(&self) -> HirId {
+            self.0.hir_id
+        }
+    }
+
+    let res = path_res(cx, &Pat(pat));
+    is_res_enum_variant(res)
+}

tests/ui/manual_let_else.rs

@@ -514,3 +514,35 @@ mod issue13768 {
         };
     }
 }
+
+mod issue14598 {
+    fn bar() -> Result<bool, &'static str> {
+        let value = match foo() {
+            //~^ manual_let_else
+            Err(_) => return Err("abc"),
+            Ok(value) => value,
+        };
+
+        let w = Some(0);
+        let v = match w {
+            //~^ manual_let_else
+            None => return Err("abc"),
+            Some(x) => x,
+        };
+
+        enum Foo<T> {
+            Foo(T),
+        }
+
+        let v = match Foo::Foo(Some(())) {
+            Foo::Foo(Some(_)) => return Err("abc"),
+            Foo::Foo(v) => v,
+        };
+
+        Ok(value == 42)
+    }
+
+    fn foo() -> Result<u32, &'static str> {
+        todo!()
+    }
+}

tests/ui/manual_let_else.stderr

@@ -529,5 +529,25 @@ LL +                 return;
 LL +             };
    |
 
-error: aborting due to 33 previous errors
+error: this could be rewritten as `let...else`
+  --> tests/ui/manual_let_else.rs:520:9
+   |
+LL | /         let value = match foo() {
+LL | |
+LL | |             Err(_) => return Err("abc"),
+LL | |             Ok(value) => value,
+LL | |         };
+   | |__________^ help: consider writing: `let Ok(value) = foo() else { return Err("abc") };`
+
+error: this could be rewritten as `let...else`
+  --> tests/ui/manual_let_else.rs:527:9
+   |
+LL | /         let v = match w {
+LL | |
+LL | |             None => return Err("abc"),
+LL | |             Some(x) => x,
+LL | |         };
+   | |__________^ help: consider writing: `let Some(v) = w else { return Err("abc") };`
+
+error: aborting due to 35 previous errors