Make the lint more specialized

Centri3 · Centri3 · commit 0a6e3ae12680 · 2023-06-20T14:38:57.000-05:00
diff --git a/clippy_lints/src/let_else_on_result_ok.rs b/clippy_lints/src/let_else_on_result_ok.rs
@@ -1,16 +1,21 @@
 use clippy_utils::{
-    diagnostics::span_lint_and_note,
-    is_from_proc_macro, is_lang_item_or_ctor,
+    diagnostics::span_lint_and_then,
+    is_from_proc_macro, is_path_diagnostic_item,
     msrvs::{self, Msrv},
+    ty::is_type_diagnostic_item,
+    visitors::for_each_expr,
 };
-use rustc_hir::{LangItem, PatKind, Stmt, StmtKind};
+use rustc_hir::{ExprKind, FnRetTy, ItemKind, OwnerNode, Stmt, StmtKind};
 use rustc_lint::{LateContext, LateLintPass, LintContext};
-use rustc_middle::lint::in_external_macro;
+use rustc_middle::{lint::in_external_macro, ty};
 use rustc_session::{declare_tool_lint, impl_lint_pass};
+use rustc_span::sym;
+use std::ops::ControlFlow;
 
 declare_clippy_lint! {
     /// ### What it does
-    /// Checks for the usage of `Ok` in a `let...else` statement.
+    /// Checks for the usage of `Ok` in a `let...else` statement in a function that returns
+    /// `Result`.
     ///
     /// ### Why is this bad?
     /// This will ignore the contents of the `Err` variant, which is generally unintended and not
@@ -48,7 +53,7 @@ declare_clippy_lint! {
     /// # fn foo() -> Result<(), ()> {
     /// #    Err(())
     /// # }
-    /// // If you want to ignore the contents of the `Err` variant:
+    /// // If you want to explicitly ignore the contents of the `Err` variant:
     /// let Some(foo) = foo().ok() else {
     ///     return;
     /// };
@@ -77,13 +82,32 @@ impl<'tcx> LateLintPass<'tcx> for LetElseOnResultOk {
             return;
         }
 
-        if let StmtKind::Local(local) = stmt.kind && let Some(els) = local.els {
+        if let StmtKind::Local(local) = stmt.kind
+            && let Some(els) = local.els
+            && let OwnerNode::Item(item) = cx.tcx.hir().owner(cx.tcx.hir().get_parent_item(stmt.hir_id))
+            && let ItemKind::Fn(sig, _, _) = item.kind
+            && let FnRetTy::Return(ret_ty) = sig.decl.output
+            && is_path_diagnostic_item(cx, ret_ty, sym::Result)
+            // Only lint if we return from it
+            && for_each_expr(els, |expr| {
+                if matches!(expr.kind, ExprKind::Ret(..)) {
+                    return ControlFlow::Break(());
+                }
+
+                ControlFlow::Continue(())
+            })
+            .is_some()
+        {
             let spans = {
                 let mut spans = vec![];
                 local.pat.walk_always(|pat| {
-                    if let PatKind::TupleStruct(qpath, _, _) = pat.kind
-                        && let Some(def_id) = cx.qpath_res(&qpath, pat.hir_id).opt_def_id()
-                        && is_lang_item_or_ctor(cx, def_id, LangItem::ResultOk)
+                    let ty = cx.typeck_results().pat_ty(pat);
+                    if is_type_diagnostic_item(cx, ty, sym::Result)
+                        && let ty::Adt(_, substs) = ty.kind()
+                        && let [_, err_ty] = substs.as_slice()
+                        && let Some(err_ty) = err_ty.as_type()
+                        && let Some(err_def) = err_ty.ty_adt_def()
+                        && err_def.all_fields().count() != 0
                     {
                         spans.push(pat.span);
                     }
@@ -96,13 +120,15 @@ impl<'tcx> LateLintPass<'tcx> for LetElseOnResultOk {
             };
 
             for span in spans {
-                span_lint_and_note(
+                span_lint_and_then(
                     cx,
                     LET_ELSE_ON_RESULT_OK,
                     span,
-                    "usage of `let...else` on `Err`",
-                    None,
-                    "consider handling the `Err` variant gracefully or propagating it to the caller",
+                    "usage of `let...else` on `Ok`",
+                    |diag| {
+                        diag.note("this will ignore the contents of the `Err` variant");
+                        diag.help("consider using a `match` instead, or propagating it to the caller");
+                    }
                 );
             }
         }
diff --git a/tests/ui/let_else_on_result_ok.rs b/tests/ui/let_else_on_result_ok.rs
@@ -11,10 +11,52 @@ enum AnEnum {
     A(Result<&'static A, ()>),
 }
 
+struct B;
+
+struct C(u32);
+
+enum D {
+    A,
+    B,
+}
+
+enum E {
+    A,
+    B(u32),
+}
+
+#[repr(C)]
+union F {
+    a: u32,
+}
+
 fn a() -> Result<(), ()> {
     Ok(())
 }
 
+fn b() -> Result<(), ()> {
+    let (Ok(_), 1) = (a(), 1) else {
+        return Err::<(), _>(());
+    };
+    Ok(())
+}
+
+fn c() -> Result<(), C> {
+    todo!()
+}
+
+fn d() -> Result<(), D> {
+    todo!()
+}
+
+fn e() -> Result<(), E> {
+    todo!()
+}
+
+fn f() -> Result<(), F> {
+    todo!()
+}
+
 fn a_constructor() -> A {
     todo!();
 }
@@ -23,34 +65,56 @@ fn an_enum_constructor() -> AnEnum {
     todo!();
 }
 
-fn main() {
+fn main() -> Result<(), Box<dyn std::error::Error>> {
     // Lint
-    let Ok(_) = a() else {
-        return;
+    let Ok(_) = c() else {
+        return Ok(());
     };
-    let (Ok(_), true) = (a(), true) else {
-        return;
+    let Ok(_) = e() else {
+        return Ok(());
     };
-    let [Ok(_), Ok(_)] = [a(), Err(())] else {
-        return;
-    };
-    let A(Ok(A(Ok(A(Ok(A(Ok(_)))))))) = a_constructor() else {
-        return;
-    };
-    let AnEnum::A(Ok(A(Err(_)))) = an_enum_constructor() else {
-        return;
+    let Ok(_) = f() else {
+        return Ok(());
     };
     // Don't lint
+    loop {
+        let Ok(_) = c() else {
+            continue;
+        };
+    }
     let Err(_) = a() else {
-        return;
+        return Ok(());
+    };
+    let Ok(_) = a() else {
+        return Ok(());
+    };
+    let Ok(_) = b() else {
+        return Ok(());
+    };
+    let Ok(_) = d() else {
+        return Ok(());
     };
     match a() {
         Ok(a) => a,
         Err(e) => eprintln!("{e:#?}"),
     };
     external! {
         let Ok(_) = a() else {
-            return;
+            return Ok(());
         };
     }
+    Ok(())
+}
+
+fn no_result_main() {
+    // Don't lint
+    let Ok(_) = c() else {
+        return;
+    };
+    let Ok(_) = e() else {
+        return;
+    };
+    let Ok(_) = f() else {
+        return;
+    };
 }
diff --git a/tests/ui/let_else_on_result_ok.stderr b/tests/ui/let_else_on_result_ok.stderr
@@ -1,75 +1,30 @@
-error: usage of `let...else` on `Err`
-  --> $DIR/let_else_on_result_ok.rs:28:9
+error: usage of `let...else` on `Ok`
+  --> $DIR/let_else_on_result_ok.rs:70:9
    |
-LL |     let Ok(_) = a() else {
+LL |     let Ok(_) = c() else {
    |         ^^^^^
    |
-   = note: consider handling the `Err` variant gracefully or propagating it to the caller
+   = note: this will ignore the contents of the `Err` variant
+   = help: consider using a `match` instead, or propagating it to the caller
    = note: `-D clippy::let-else-on-result-ok` implied by `-D warnings`
 
-error: usage of `let...else` on `Err`
-  --> $DIR/let_else_on_result_ok.rs:31:10
+error: usage of `let...else` on `Ok`
+  --> $DIR/let_else_on_result_ok.rs:73:9
    |
-LL |     let (Ok(_), true) = (a(), true) else {
-   |          ^^^^^
-   |
-   = note: consider handling the `Err` variant gracefully or propagating it to the caller
-
-error: usage of `let...else` on `Err`
-  --> $DIR/let_else_on_result_ok.rs:34:10
-   |
-LL |     let [Ok(_), Ok(_)] = [a(), Err(())] else {
-   |          ^^^^^
-   |
-   = note: consider handling the `Err` variant gracefully or propagating it to the caller
-
-error: usage of `let...else` on `Err`
-  --> $DIR/let_else_on_result_ok.rs:34:17
-   |
-LL |     let [Ok(_), Ok(_)] = [a(), Err(())] else {
-   |                 ^^^^^
-   |
-   = note: consider handling the `Err` variant gracefully or propagating it to the caller
-
-error: usage of `let...else` on `Err`
-  --> $DIR/let_else_on_result_ok.rs:37:11
-   |
-LL |     let A(Ok(A(Ok(A(Ok(A(Ok(_)))))))) = a_constructor() else {
-   |           ^^^^^^^^^^^^^^^^^^^^^^^^^^
-   |
-   = note: consider handling the `Err` variant gracefully or propagating it to the caller
-
-error: usage of `let...else` on `Err`
-  --> $DIR/let_else_on_result_ok.rs:37:16
-   |
-LL |     let A(Ok(A(Ok(A(Ok(A(Ok(_)))))))) = a_constructor() else {
-   |                ^^^^^^^^^^^^^^^^^^^
-   |
-   = note: consider handling the `Err` variant gracefully or propagating it to the caller
-
-error: usage of `let...else` on `Err`
-  --> $DIR/let_else_on_result_ok.rs:37:21
-   |
-LL |     let A(Ok(A(Ok(A(Ok(A(Ok(_)))))))) = a_constructor() else {
-   |                     ^^^^^^^^^^^^
-   |
-   = note: consider handling the `Err` variant gracefully or propagating it to the caller
-
-error: usage of `let...else` on `Err`
-  --> $DIR/let_else_on_result_ok.rs:37:26
-   |
-LL |     let A(Ok(A(Ok(A(Ok(A(Ok(_)))))))) = a_constructor() else {
-   |                          ^^^^^
+LL |     let Ok(_) = e() else {
+   |         ^^^^^
    |
-   = note: consider handling the `Err` variant gracefully or propagating it to the caller
+   = note: this will ignore the contents of the `Err` variant
+   = help: consider using a `match` instead, or propagating it to the caller
 
-error: usage of `let...else` on `Err`
-  --> $DIR/let_else_on_result_ok.rs:40:19
+error: usage of `let...else` on `Ok`
+  --> $DIR/let_else_on_result_ok.rs:76:9
    |
-LL |     let AnEnum::A(Ok(A(Err(_)))) = an_enum_constructor() else {
-   |                   ^^^^^^^^^^^^^
+LL |     let Ok(_) = f() else {
+   |         ^^^^^
    |
-   = note: consider handling the `Err` variant gracefully or propagating it to the caller
+   = note: this will ignore the contents of the `Err` variant
+   = help: consider using a `match` instead, or propagating it to the caller
 
-error: aborting due to 9 previous errors
+error: aborting due to 3 previous errors
 
