Add ignored_result_err lint to detect discarded Result error variants

CHANGELOG.md

@@ -6978,6 +6978,7 @@ Released 2018-09-13
 [`if_then_some_else_none`]: https://rust-lang.github.io/rust-clippy/master/index.html#if_then_some_else_none
 [`ifs_same_cond`]: https://rust-lang.github.io/rust-clippy/master/index.html#ifs_same_cond
 [`ignore_without_reason`]: https://rust-lang.github.io/rust-clippy/master/index.html#ignore_without_reason
+[`ignored_result_err`]: https://rust-lang.github.io/rust-clippy/master/index.html#ignored_result_err
 [`ignored_unit_patterns`]: https://rust-lang.github.io/rust-clippy/master/index.html#ignored_unit_patterns
 [`impl_hash_borrow_with_str_and_bytes`]: https://rust-lang.github.io/rust-clippy/master/index.html#impl_hash_borrow_with_str_and_bytes
 [`impl_trait_in_params`]: https://rust-lang.github.io/rust-clippy/master/index.html#impl_trait_in_params
@@ -7662,6 +7663,7 @@ Released 2018-09-13
 [`allow-exact-repetitions`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-exact-repetitions
 [`allow-expect-in-consts`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-expect-in-consts
 [`allow-expect-in-tests`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-expect-in-tests
+[`allow-ignored-result-err-in-tests`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-ignored-result-err-in-tests
 [`allow-indexing-slicing-in-tests`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-indexing-slicing-in-tests
 [`allow-large-stack-frames-in-tests`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-large-stack-frames-in-tests
 [`allow-mixed-uninlined-format-args`]: https://doc.rust-lang.org/clippy/lint_configuration.html#allow-mixed-uninlined-format-args

book/src/lint_configuration.md

@@ -101,6 +101,16 @@ Whether `expect` should be allowed in test functions or `#[cfg(test)]`
 * [`expect_used`](https://rust-lang.github.io/rust-clippy/master/index.html#expect_used)
 
 
+## `allow-ignored-result-err-in-tests`
+Whether `ignored_result_err` should be allowed in test functions or `#[cfg(test)]`
+
+**Default Value:** `false`
+
+---
+**Affected lints:**
+* [`ignored_result_err`](https://rust-lang.github.io/rust-clippy/master/index.html#ignored_result_err)
+
+
 ## `allow-indexing-slicing-in-tests`
 Whether `indexing_slicing` should be allowed in test functions or `#[cfg(test)]`
 

clippy_config/src/conf.rs

@@ -371,6 +371,9 @@ define_Conf! {
     /// Whether `expect` should be allowed in test functions or `#[cfg(test)]`
     #[lints(expect_used)]
     allow_expect_in_tests: bool = false,
+    /// Whether `ignored_result_err` should be allowed in test functions or `#[cfg(test)]`
+    #[lints(ignored_result_err)]
+    allow_ignored_result_err_in_tests: bool = false,
     /// Whether `indexing_slicing` should be allowed in test functions or `#[cfg(test)]`
     #[lints(indexing_slicing)]
     allow_indexing_slicing_in_tests: bool = false,

clippy_lints/src/declared_lints.rs

@@ -213,6 +213,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
     crate::ifs::IF_SAME_THEN_ELSE_INFO,
     crate::ifs::IFS_SAME_COND_INFO,
     crate::ifs::SAME_FUNCTIONS_IN_IF_CONDITION_INFO,
+    crate::ignored_result_err::IGNORED_RESULT_ERR_INFO,
     crate::ignored_unit_patterns::IGNORED_UNIT_PATTERNS_INFO,
     crate::impl_hash_with_borrow_str_and_bytes::IMPL_HASH_BORROW_WITH_STR_AND_BYTES_INFO,
     crate::implicit_hasher::IMPLICIT_HASHER_INFO,

clippy_lints/src/ignored_result_err.rs

@@ -0,0 +1,141 @@
+use clippy_config::Conf;
+use clippy_utils::diagnostics::span_lint_and_help;
+use clippy_utils::res::MaybeDef;
+use clippy_utils::{higher, is_in_test};
+use rustc_hir::{Expr, LetStmt, Pat, PatKind, QPath};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::impl_lint_pass;
+use rustc_span::sym;
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for `if let Ok(x) = expr`, `while let Ok(x) = expr`, and
+    /// `let Ok(x) = expr else { ... }` where the `Err` variant is discarded
+    /// without binding.
+    ///
+    /// ### Why is this bad?
+    /// The error value contains context about what went wrong. Discarding it
+    /// prevents detailed logging and makes error recovery impossible.
+    ///
+    /// ### Example
+    /// ```rust,ignore
+    /// if let Ok(res) = some_call() {
+    ///     use_res(res);
+    /// } else {
+    ///     error!("Something went wrong");
+    /// }
+    ///
+    /// while let Ok(line) = reader.read_line() {
+    ///     process(line);
+    /// }
+    ///
+    /// let Ok(val) = some_call() else { return; };
+    /// ```
+    /// Use instead:
+    /// ```rust,ignore
+    /// match some_call() {
+    ///     Ok(res) => use_res(res),
+    ///     Err(e) => error!("Something went wrong: {}", e),
+    /// }
+    ///
+    /// loop {
+    ///     match reader.read_line() {
+    ///         Ok(line) => process(line),
+    ///         Err(e) => {
+    ///             error!("Read failed: {}", e);
+    ///             break;
+    ///         }
+    ///     }
+    /// }
+    ///
+    /// match some_call() {
+    ///     Ok(val) => { /* use val */ },
+    ///     Err(e) => {
+    ///         error!("Failed: {}", e);
+    ///         return;
+    ///     }
+    /// }
+    /// ```
+    ///
+    /// ### Configuration
+    /// - `allow-ignored-result-err-in-tests`: set to `true` to disable this lint in test code
+    #[clippy::version = "1.98.0"]
+    pub IGNORED_RESULT_ERR,
+    restriction,
+    "`if let Ok(x) = ...`, `while let Ok(x) = ...`, or `let Ok(x) = ... else` discards the error variant without binding it"
+}
+
+impl_lint_pass!(IgnoredResultErr => [IGNORED_RESULT_ERR]);
+
+pub struct IgnoredResultErr {
+    allow_in_tests: bool,
+}
+
+impl IgnoredResultErr {
+    pub fn new(conf: &'static Conf) -> Self {
+        Self {
+            allow_in_tests: conf.allow_ignored_result_err_in_tests,
+        }
+    }
+}
+
+fn is_ok_pattern_on_result(cx: &LateContext<'_>, pat: &Pat<'_>, scrutinee: &Expr<'_>) -> bool {
+    cx.typeck_results().expr_ty(scrutinee).is_diag_item(cx, sym::Result)
+        && matches!(
+            pat.kind,
+            PatKind::TupleStruct(QPath::Resolved(_, path), _, _)
+                if path.segments.last().is_some_and(|s| s.ident.name == sym::Ok)
+        )
+}
+
+impl<'tcx> LateLintPass<'tcx> for IgnoredResultErr {
+    fn check_local(&mut self, cx: &LateContext<'tcx>, local: &'tcx LetStmt<'_>) {
+        if self.allow_in_tests && is_in_test(cx.tcx, local.hir_id) {
+            return;
+        }
+
+        if local.els.is_some()
+            && let Some(init) = local.init
+            && is_ok_pattern_on_result(cx, local.pat, init)
+        {
+            span_lint_and_help(
+                cx,
+                IGNORED_RESULT_ERR,
+                local.span,
+                "this `let Ok(...) = ... else` discards the `Err` variant",
+                None,
+                "consider using `match` and binding the `Err` value for logging or recovery",
+            );
+        }
+    }
+
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
+        if self.allow_in_tests && is_in_test(cx.tcx, expr.hir_id) {
+            return;
+        }
+
+        if let Some(if_let) = higher::IfLet::hir(cx, expr)
+            && is_ok_pattern_on_result(cx, if_let.let_pat, if_let.let_expr)
+        {
+            span_lint_and_help(
+                cx,
+                IGNORED_RESULT_ERR,
+                expr.span,
+                "this `if let Ok(...)` discards the `Err` variant",
+                None,
+                "consider using `match` and binding the `Err` value for logging or recovery",
+            );
+        } else if let Some(while_let) = higher::WhileLet::hir(expr)
+            && is_ok_pattern_on_result(cx, while_let.let_pat, while_let.let_expr)
+        {
+            span_lint_and_help(
+                cx,
+                IGNORED_RESULT_ERR,
+                expr.span,
+                "this `while let Ok(...)` discards the `Err` variant",
+                None,
+                "consider using `loop` + `match` and binding the `Err` value for logging or recovery",
+            );
+        }
+    }
+}

clippy_lints/src/lib.rs

@@ -155,6 +155,7 @@ mod if_let_mutex;
 mod if_not_else;
 mod if_then_some_else_none;
 mod ifs;
+mod ignored_result_err;
 mod ignored_unit_patterns;
 mod impl_hash_with_borrow_str_and_bytes;
 mod implicit_hasher;
@@ -866,6 +867,7 @@ rustc_lint::late_lint_methods!(
         RefPatterns: ref_patterns::RefPatterns = ref_patterns::RefPatterns,
         RedundantElse: redundant_else::RedundantElse = redundant_else::RedundantElse,
         RestWhenDestructuringStruct: rest_when_destructuring_struct::RestWhenDestructuringStruct = rest_when_destructuring_struct::RestWhenDestructuringStruct,
+        IgnoredResultErr: ignored_result_err::IgnoredResultErr = ignored_result_err::IgnoredResultErr::new(conf),
         // add late passes here, used by `cargo dev new_lint`
     ]]
 );

tests/ui-toml/ignored_result_err/clippy.toml

@@ -0,0 +1 @@
+allow-ignored-result-err-in-tests = true

tests/ui-toml/ignored_result_err/ignored_result_err.rs

@@ -0,0 +1,33 @@
+#![warn(clippy::ignored_result_err)]
+
+fn some_call() -> Result<i32, String> {
+    Ok(42)
+}
+
+// Should lint — not in test context
+fn main() {
+    if let Ok(res) = some_call() {
+        //~^ ignored_result_err
+        println!("{res}");
+    }
+}
+
+// Should NOT lint — in a #[test] function (allow-ignored-result-err-in-tests = true)
+#[test]
+fn test_something() {
+    if let Ok(res) = some_call() {
+        println!("{res}");
+    }
+}
+
+// Should NOT lint — in a #[cfg(test)] module
+#[cfg(test)]
+mod tests {
+    use super::some_call;
+
+    fn helper() {
+        if let Ok(res) = some_call() {
+            println!("{res}");
+        }
+    }
+}

tests/ui-toml/ignored_result_err/ignored_result_err.stderr

@@ -0,0 +1,15 @@
+error: this `if let Ok(...)` discards the `Err` variant
+  --> tests/ui-toml/ignored_result_err/ignored_result_err.rs:9:5
+   |
+LL | /     if let Ok(res) = some_call() {
+LL | |
+LL | |         println!("{res}");
+LL | |     }
+   | |_____^
+   |
+   = help: consider using `match` and binding the `Err` value for logging or recovery
+   = note: `-D clippy::ignored-result-err` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::ignored_result_err)]`
+
+error: aborting due to 1 previous error
+

tests/ui-toml/toml_unknown_key/conf_unknown_key.stderr

@@ -8,6 +8,7 @@ error: error reading Clippy's configuration file: unknown field `foobar`, expect
            allow-exact-repetitions
            allow-expect-in-consts
            allow-expect-in-tests
+           allow-ignored-result-err-in-tests
            allow-indexing-slicing-in-tests
            allow-large-stack-frames-in-tests
            allow-mixed-uninlined-format-args
@@ -111,6 +112,7 @@ error: error reading Clippy's configuration file: unknown field `barfoo`, expect
            allow-exact-repetitions
            allow-expect-in-consts
            allow-expect-in-tests
+           allow-ignored-result-err-in-tests
            allow-indexing-slicing-in-tests
            allow-large-stack-frames-in-tests
            allow-mixed-uninlined-format-args
@@ -214,6 +216,7 @@ error: error reading Clippy's configuration file: unknown field `allow_mixed_uni
            allow-exact-repetitions
            allow-expect-in-consts
            allow-expect-in-tests
+           allow-ignored-result-err-in-tests
            allow-indexing-slicing-in-tests
            allow-large-stack-frames-in-tests
            allow-mixed-uninlined-format-args

tests/ui/ignored_result_err.rs

@@ -0,0 +1,40 @@
+#![warn(clippy::ignored_result_err)]
+
+fn some_call() -> Result<i32, String> {
+    Ok(42)
+}
+
+fn main() {
+    // Should lint — if let with else
+    if let Ok(res) = some_call() {
+        //~^ ignored_result_err
+        println!("{res}");
+    } else {
+        println!("something went wrong");
+    }
+
+    // Should lint — if let without else
+    if let Ok(res) = some_call() {
+        //~^ ignored_result_err
+        println!("{res}");
+    }
+
+    // Should lint — while let
+    while let Ok(res) = some_call() {
+        //~^ ignored_result_err
+        println!("{res}");
+    }
+
+    // Should lint — let else
+    let Ok(res) = some_call() else {
+        //~^ ignored_result_err
+        return;
+    };
+    println!("{res}");
+
+    // Should NOT lint — match with Err bound
+    match some_call() {
+        Ok(res) => println!("{res}"),
+        Err(e) => println!("failed: {e}"),
+    }
+}

tests/ui/ignored_result_err.stderr

@@ -0,0 +1,50 @@
+error: this `if let Ok(...)` discards the `Err` variant
+  --> tests/ui/ignored_result_err.rs:9:5
+   |
+LL | /     if let Ok(res) = some_call() {
+LL | |
+LL | |         println!("{res}");
+LL | |     } else {
+LL | |         println!("something went wrong");
+LL | |     }
+   | |_____^
+   |
+   = help: consider using `match` and binding the `Err` value for logging or recovery
+   = note: `-D clippy::ignored-result-err` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::ignored_result_err)]`
+
+error: this `if let Ok(...)` discards the `Err` variant
+  --> tests/ui/ignored_result_err.rs:17:5
+   |
+LL | /     if let Ok(res) = some_call() {
+LL | |
+LL | |         println!("{res}");
+LL | |     }
+   | |_____^
+   |
+   = help: consider using `match` and binding the `Err` value for logging or recovery
+
+error: this `while let Ok(...)` discards the `Err` variant
+  --> tests/ui/ignored_result_err.rs:23:5
+   |
+LL | /     while let Ok(res) = some_call() {
+LL | |
+LL | |         println!("{res}");
+LL | |     }
+   | |_____^
+   |
+   = help: consider using `loop` + `match` and binding the `Err` value for logging or recovery
+
+error: this `let Ok(...) = ... else` discards the `Err` variant
+  --> tests/ui/ignored_result_err.rs:29:5
+   |
+LL | /     let Ok(res) = some_call() else {
+LL | |
+LL | |         return;
+LL | |     };
+   | |______^
+   |
+   = help: consider using `match` and binding the `Err` value for logging or recovery
+
+error: aborting due to 4 previous errors
+